Follow-ups from the review of #2124 (leader-removal-recovery Jepsen scenario). None of them blocks the merge; each can be fixed on its own afterwards.
1. cli_test.clj: the :pre-vote-stability assertion checks nothing
selects-two-nodes-for-leader-removal in jepsen/test/jepsen/openraft/cli_test.clj asserts:
(is (nil? (:pre-vote-stability test)))
No code sets :pre-vote-stability: 8535779 renamed that key to :two-leaf-partition. The assertion passes for every test map, so it does not check what it means to check: that the two-leaf flag (which adds --enable-pre-vote in db.clj) stays off for this scenario.
Fix: assert on (:two-leaf-partition test).
2. /append-membership returns a different error shape than the other membership routes
jepsen/openraft-test-app/src/http_api.rs:
serde_json::json!(app.raft.append_membership(membership, openraft::EntryPayload::Blank, []).await)
This serializes the whole RaftError<C, ClientWriteError<C>>, so an API error arrives as {"Err": {"APIError": {"ForwardToLeader": …}}}. /change-membership and /write (App::change_membership, App::write in examples/app-http/src/app.rs) go through .decompose().unwrap() and return {"Err": {"ForwardToLeader": …}}. client/ok-value (modeled-error-variants) and recover! in leader_removal_recovery.clj (the #{:ForwardToLeader :QuorumNotEnough} match) recognize only the flat shape, so a ForwardToLeader from this route is classified as an unknown :openraft-error.
Today this is harmless, because prepare! rethrows every non-timeout error from append-membership!. It becomes wrong as soon as append-membership! is routed through with-leader! or gets retry logic.
Fix: return Result<ClientWriteResponse<TypeConfig>, ClientWriteError<TypeConfig>> through .decompose().unwrap() like the sibling handlers, and map the Membership::new failure with the existing From<MembershipError> for ChangeMembershipError (ClientWriteError::ChangeMembershipError(e.into())) instead of the ad-hoc {"Err": {"InvalidMembership": …}} object.
3. prepare! races the 300 ms leader lease
Raft::append_membership rejects a proposal once the leader lease has expired (ensure_writable_leader_handler → ForwardToLeader { reason: LeaseExpired }). The lease is election_timeout_max = 300 ms after the send time of the last quorum-acked heartbeat. In prepare!, partition! drops A's traffic to B's Raft port, so the lease starts running out at the iptables -I OUTPUT -j OR_LEADER_REMOVAL on A. After that insert, two more iptables -C round trips run on A, B's five commands finish in parallel, and only then is POST /append-membership sent.
In my local run (Docker Desktop, arm64) n1's openraft.log shows the first blocked heartbeat failing at 06:19:38.350 (heartbeat RPC timeout is 50 ms, so it was sent at about 38.300 and the last acknowledged one at about 38.250) and the F proposal received at 06:19:38.381. About 130 ms of the 300 ms lease were used. On a slower CI runner the lease can expire first; the run then ends as a harness failure (prepare! rethrows the :openraft-error). That is a correct failure, but a timing-dependent one.
Suggested fix: shorten the critical path, for example run the five iptables commands as one bash -c per node, or move iptables -C OR_LEADER_REMOVAL … before the -I OUTPUT insert so only one check follows it. The lease itself is an SUT setting and stays as it is.
4. Small style items
prepare-options in cli.clj calls liveness-node-error twice, once as the cond-> test and once as the value.partition! hard-codes (:raft-port test 22001); two_leaf_partition.clj uses http/default-raft-port for the same default.- In
recover!, the await/retry! call sits inside the try whose catch then rethrows it through the (throw e) fall-through. Moving the final-ready? check out of the try makes the retry path direct.
Non-goals: the scenario design (two explicit append_membership calls, leader restore disabled, 60 s bound, two-node cluster) follows #2123 and is not in question.
Follow-ups from the review of #2124 (
leader-removal-recoveryJepsen scenario). None of them blocks the merge; each can be fixed on its own afterwards.1.
cli_test.clj: the:pre-vote-stabilityassertion checks nothingselects-two-nodes-for-leader-removalinjepsen/test/jepsen/openraft/cli_test.cljasserts:No code sets
:pre-vote-stability: 8535779 renamed that key to:two-leaf-partition. The assertion passes for every test map, so it does not check what it means to check: that the two-leaf flag (which adds--enable-pre-voteindb.clj) stays off for this scenario.Fix: assert on
(:two-leaf-partition test).2.
/append-membershipreturns a different error shape than the other membership routesjepsen/openraft-test-app/src/http_api.rs:This serializes the whole
RaftError<C, ClientWriteError<C>>, so an API error arrives as{"Err": {"APIError": {"ForwardToLeader": …}}}./change-membershipand/write(App::change_membership,App::writeinexamples/app-http/src/app.rs) go through.decompose().unwrap()and return{"Err": {"ForwardToLeader": …}}.client/ok-value(modeled-error-variants) andrecover!inleader_removal_recovery.clj(the#{:ForwardToLeader :QuorumNotEnough}match) recognize only the flat shape, so aForwardToLeaderfrom this route is classified as an unknown:openraft-error.Today this is harmless, because
prepare!rethrows every non-timeout error fromappend-membership!. It becomes wrong as soon asappend-membership!is routed throughwith-leader!or gets retry logic.Fix: return
Result<ClientWriteResponse<TypeConfig>, ClientWriteError<TypeConfig>>through.decompose().unwrap()like the sibling handlers, and map theMembership::newfailure with the existingFrom<MembershipError> for ChangeMembershipError(ClientWriteError::ChangeMembershipError(e.into())) instead of the ad-hoc{"Err": {"InvalidMembership": …}}object.3.
prepare!races the 300 ms leader leaseRaft::append_membershiprejects a proposal once the leader lease has expired (ensure_writable_leader_handler→ForwardToLeader { reason: LeaseExpired }). The lease iselection_timeout_max= 300 ms after the send time of the last quorum-acked heartbeat. Inprepare!,partition!drops A's traffic to B's Raft port, so the lease starts running out at theiptables -I OUTPUT -j OR_LEADER_REMOVALon A. After that insert, two moreiptables -Cround trips run on A, B's five commands finish in parallel, and only then isPOST /append-membershipsent.In my local run (Docker Desktop, arm64) n1's
openraft.logshows the first blocked heartbeat failing at 06:19:38.350 (heartbeat RPC timeout is 50 ms, so it was sent at about 38.300 and the last acknowledged one at about 38.250) and the F proposal received at 06:19:38.381. About 130 ms of the 300 ms lease were used. On a slower CI runner the lease can expire first; the run then ends as a harness failure (prepare!rethrows the:openraft-error). That is a correct failure, but a timing-dependent one.Suggested fix: shorten the critical path, for example run the five iptables commands as one
bash -cper node, or moveiptables -C OR_LEADER_REMOVAL …before the-I OUTPUTinsert so only one check follows it. The lease itself is an SUT setting and stays as it is.4. Small style items
prepare-optionsincli.cljcallsliveness-node-errortwice, once as thecond->test and once as the value.partition!hard-codes(:raft-port test 22001);two_leaf_partition.cljuseshttp/default-raft-portfor the same default.recover!, theawait/retry!call sits inside thetrywhosecatchthen rethrows it through the(throw e)fall-through. Moving thefinal-ready?check out of thetrymakes the retry path direct.Non-goals: the scenario design (two explicit
append_membershipcalls, leader restore disabled, 60 s bound, two-node cluster) follows #2123 and is not in question.