Skip to content

Cover the forwarded client-write failure in consensus runtime #2201

Description

@gaborbernat

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 Close the 149 lines the coverage gate never saw #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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:testsConformance, perf tests, and observabilitypriority:P2Performance, observability, or deferred featuretype:testTesting, conformance, metrics, or validation coverage

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions