tox-dev / tox-dev/peryx

Cover the forwarded client-write failure in consensus runtime

Closed
#2,201 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

area:tests priority:P2 type:test
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:

  1. Build the multi-node fixture: two nodes, the non-leader configured with a peer_token and a leader endpoint that refuses connections, so the forwarded send fails on connect.
  2. Add a seam. The RaftRpcClient is 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 way commit_transfer already takes &dyn ControlExecutor and let #2166 cover authority_transfer.rs:154 without 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:913 is covered, and the assertion names the Unavailable error 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_token and forward_target guards 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.