Cover the forwarded client-write failure in consensus runtime
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 4
- Forks
- 1
- Avg merge
- 3h 7m
- Merged PRs (30d)
- 523
Description
Problem
crates/peryx-ha-distributed/src/consensus_runtime.rs:913 is the only uncovered line left in the crate's real half, and nothing in the unit suite can reach it:
let response: Result<...> = client
.send(RaftRpc::ClientWrite, &command, CLIENT_WRITE_TIMEOUT)
.await
.map_err(|error| OwnershipError::Unavailable(error.to_string()))?;
Three conditions have to hold at once. client_write must fail with ForwardToLeader; peer_token must be configured, or line 899 returns first; and forward_target must yield a target, or line 903 returns first. Only then is the forwarded send issued, and only a failure of that send reaches line 913.
The existing test at consensus_runtime_tests.rs:2073 exercises map_write_error on a synthetic ForwardToLeader and never reaches the forwarding path. No harness builds a non-leader node with a live peer endpoint.
Why it is filed rather than written
Producing a genuine ForwardToLeader from self.raft.client_write needs a node that is not the leader and knows who is, which means a real multi-node cluster. A single node with a dead peer never elects a leader, so forward_target returns None and the path short-circuits at 903 before reaching 913.
That is a multi-node Raft fixture for one line, and this crate's unit tests do not otherwise build one. It is the same disposition as #2197: worth doing deliberately, not worth absorbing into the coverage sweep.
Required change
Choose one and say which in the PR:
- Build the multi-node fixture: two nodes, the non-leader configured with a
peer_tokenand a leader endpoint that refuses connections, so the forwarded send fails on connect. - Add a seam. The
RaftRpcClientis constructed inline at line 905, so nothing can substitute it. Taking the client through an injectable factory would let a double fail the send directly, the waycommit_transferalready takes&dyn ControlExecutorand let #2166 coverauthority_transfer.rs:154without real consensus.
Prefer the second if it is a shape production code would accept. The precedent in this same crate suggests it is.
Acceptance criteria
consensus_runtime.rs:913is covered, and the assertion names theUnavailableerror rather than only the failure.- The test constructs the failure deterministically: a refused connection or an injected transport error, no sleeps, no retries, no artificial load.
- The
peer_tokenandforward_targetguards above it keep their existing behaviour and coverage.
Boundary
Only this line and the seam it needs. Do not restructure the forwarding path's behaviour, and do not touch map_write_error or the CheckIsLeaderError path at line 719.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with crates/peryx-ha-distributed/src/consensus_runtime.rs around lines 899-913 and the test at consensus_runtime_tests.rs:2073. Trace the peer_token and forward_target guards, then determine whether a two-node fixture or an injectable RaftRpcClient factory best fits the existing ControlExecutor precedent. Done means line 913 is covered deterministically and the test asserts the Unavailable error without changing the forwarding guards or other error paths.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- distributed-systems
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100