feat(control,worker,sdk): client & worker failover across control-plane nodes (I9b W3–W5) - #169
Conversation
…ne nodes
Milestone I9b W3-W5 (`docs/plan.md` §17 task 7, `docs/phase-5-plan.md`
§VII.3), completing I9b.
I9a stood up a real multi-node control plane and I9b W1/W2 made its leader
redirect dialable. This closes the three remaining gaps that made the cluster
unusable from the outside.
**A completed action is never discarded for a routing accident** (decision D1,
owner 2026-07-30). The scheduler's post-execution action-cache write is now
best-effort *on `NotLeader` only*: the build returns its real result flagged
uncached. Previously a `--raft` follower failed the whole Execute with
INTERNAL **after** the sandbox had already run.
The recorded objection to best-effort was that it hides a routing problem as a
cache-hit-rate problem, so that objection is implemented as work: a
`uncached_results_not_leader` counter, a `warn` naming digest and leader, and
the fact surfaced to the *client* in `ExecuteResponse.message` so "ran and
cached" and "ran, not cached" are distinguishable without server logs. And it
is narrow -- a `Redb` or throughput error still fails the RPC, with a named
test for exactly that, because best-effort must not become best-ignored. No
retry: this node is not the leader and the only store reachable from it is the
one that just refused; forwarding is §VII.2 option (b), a different design.
**Workers survive losing the node they attached to.** `--control` and
`--worker-control` are repeatable, `WorkerConfig` carries a
`Vec<ControlPlane>`, and `run_worker` rotates on failure and re-registers --
necessary because the worker registry is per-node and ephemeral by ADR design,
so a new node has never heard of this worker. The policy is a pure function:
the whole first cycle runs with zero delay (the survivors are up *now*;
backing off before trying them adds latency for nothing), then exponential
backoff per completed cycle capped at 30s with attempt-derived jitter so
co-restarted fleets do not reconnect in lockstep. A session that registered
resets the backoff, or a worker running for hours would inherit an old
outage's delay and crawl through a failover it should sail through.
**Clients follow the redirect.** New `brokkr_sdk::redirect` classifies a
refusal and `BrokkrClient::{get_action_result, update_action_result}` follow
it, bounded by `MAX_LEADER_HOPS` and keeping the followed connection so steady
state is one hop. A redirect is identified by its *metadata*, not by
FAILED_PRECONDITION alone -- the scheduler already returns that code for "no
eligible worker", so code-only matching would retry an unrelated failure
against a different node. `hint_to_url` inherits the caller's scheme so a
redirect can never downgrade https to plaintext. `connect_any` and a
repeatable `brokk run --control` use the first node that answers, reporting
the last transport error rather than a synthetic aggregate that would hide
whether the cause was refusal or a bad certificate.
Stated plainly rather than glossed: because D1 means Execute no longer fails
on `NotLeader`, and the CLI never calls the ActionCache directly, no in-tree
client RPC returns a redirect through `brokk run`. The surface that matters is
the REAPI one an external client (Bazel) drives -- hence the new SDK
ActionCache methods, so the policy has a real consumer instead of being
exported and called done.
Extracting `rotation_plan` and `redirect::classify` as pure functions and
testing them exhaustively caught four bugs that no happy-path integration test
reaches: a `1 << n` overflow panic after ~64 reconnect cycles, a
modulo-by-zero on an empty endpoint set, an https->http downgrade, and a
redirect cycle that hung instead of terminating.
Tests (19 new, each written before its implementation): 2 scheduler tests for
D1; 6 for `rotation_plan`; 2 for endpoint pairing; 6 for
`classify`/`hint_to_url`; and 3 integration tests in `leader_redirect.rs`
driving real `ActionCacheService` servers -- a client on a follower reaches
the leader for read and write, an unroutable hint surfaces the original
refusal with its metadata intact, and a self-referential hint terminates on
the hop budget.
`BASE_BACKOFF`/`MAX_BACKOFF` are public: they are the policy, and public docs
cannot link private items under `RUSTDOCFLAGS=-Dwarnings`.
Verified in WSL2: fmt, `clippy --workspace --all-targets -D warnings`,
`RUSTDOCFLAGS=-Dwarnings cargo doc --no-deps` (exit 0), and
`cargo test --workspace --no-fail-fast` -- 48 suites, the only failures being
the 6 `evil_seccomp_caps` tests that need a real kernel for seccomp argument
filters.
No new dependency. `--raft` off remains the single-node path.
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughClients and workers now support multiple control-plane endpoints. SDK clients follow bounded leader redirects while preserving metadata and transport settings. Successful executions remain successful when action-cache writes return ChangesHA control-plane failover
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Follower
participant Leader
participant Worker
Client->>Follower: request action result
Follower-->>Client: NotLeader with leader hint
Client->>Leader: follow bounded redirect
Worker->>Leader: register after endpoint rotation
Leader-->>Worker: worker session
Leader-->>Client: action result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/brokkr-worker/src/endpoint.rs (1)
91-93: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
completed_cycles as u32truncates instead of saturating.For absurdly large
attemptvalues (the case the comment claims to defend against) theusize → u32cast wraps, so the shift exponent can land back in range and yield a small backoff rather than the cap. Practically unreachable, but the saturating intent is easy to make literal:♻️ Saturating cast
- let factor = 1u64 - .checked_shl(completed_cycles as u32 - 1) - .unwrap_or(u64::MAX); + let exponent = u32::try_from(completed_cycles) + .unwrap_or(u32::MAX) + .saturating_sub(1); + let factor = 1u64.checked_shl(exponent).unwrap_or(u64::MAX);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/brokkr-worker/src/endpoint.rs` around lines 91 - 93, Update the shift calculation in the backoff logic around factor so the completed_cycles-to-u32 conversion saturates at u32::MAX instead of truncating on overflow, preserving the intended u64::MAX cap for excessively large attempt values.crates/brokkr-worker/src/worker.rs (1)
236-247: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment: it still explains the removed
worker_endpoint/control_endpointfields.Lines 241‑242 and the
derive_default_worker_endpoint/--worker-controlnote now describe config that no longer exists onWorkerConfig. Sincecas_urlis justplane.worker_url()again, the comment can shrink to the single fact that matters (CAS is reached over the worker port in split-port mode, issue#139), andworker_urlcan be reused rather than recomputed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/brokkr-worker/src/worker.rs` around lines 236 - 247, Update the CAS URL setup around plane.worker_url() to reuse the existing worker_url value instead of recomputing it, and replace the stale multi-port configuration explanation with a concise comment stating that split-port mode reaches CAS through the worker port (issue `#139`). Remove references to worker_endpoint, control_endpoint, derive_default_worker_endpoint, and --worker-control.crates/brokkr-sdk/src/client.rs (2)
266-310: 📐 Maintainability & Code Quality | 🔵 TrivialMissing tracing spans on new hot-path async helpers.
reconnect_toandfollow_redirectare new async functions invoked on every leader-redirect retry, but unlike every other new async fn in this diff they have no#[tracing::instrument].As per coding guidelines, "Add a tracing span for any new RPC handler or async function on a hot path."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/brokkr-sdk/src/client.rs` around lines 266 - 310, The new hot-path async helpers lack tracing spans. Add #[tracing::instrument] attributes to both reconnect_to and follow_redirect, preserving their existing behavior and avoiding sensitive authentication values in captured fields.Source: Coding guidelines
231-264: 📐 Maintainability & Code Quality | 🔵 Trivial
connect_anycan't do whatbrokkr-cli'sconnect_firstalready re-implements.
connect_anyonly callsSelf::connect(...), so it has no way to select the first reachable endpoint when TLS or a bearer token is configured.brokkr-cli/src/main.rs::connect_firstre-implements the identical "iterate endpoints, keep the last transport error" loop specifically to cover that matrix. Consider extendingconnect_anyto acceptOption<TlsConfig>/Option<bearer>(mirroringreconnect_to's matrix) so the CLI can call the SDK primitive instead of duplicating it.♻️ Sketch: extend connect_any to cover the auth matrix
pub async fn connect_any<S: Into<String>>( endpoints: impl IntoIterator<Item = S>, tls: Option<TlsConfig>, bearer: Option<String>, ) -> Result<Self, ClientError> { let endpoints: Vec<String> = endpoints.into_iter().map(Into::into).collect(); let mut last_error = None; for endpoint in &endpoints { let attempt = match (tls.clone(), bearer.clone()) { (Some(t), Some(b)) => Self::connect_with_tls_and_bearer(endpoint.clone(), t, b).await, (Some(t), None) => Self::connect_with_tls(endpoint.clone(), t).await, (None, Some(b)) => Self::connect_with_bearer(endpoint.clone(), b).await, (None, None) => Self::connect(endpoint.clone()).await, }; match attempt { Ok(client) => return Ok(client), Err(e) => last_error = Some(e), } } Err(last_error.unwrap_or_else(|| ClientError::InvalidEndpoint("no control-plane endpoints configured".to_string()))) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/brokkr-sdk/src/client.rs` around lines 231 - 264, Extend Client::connect_any to accept optional TlsConfig and bearer credentials, and select the corresponding connection method for each TLS/bearer combination on every endpoint attempt. Preserve the existing ordered iteration, success return, last-error propagation, and empty-endpoint fallback, then update brokkr-cli’s connect_first caller to use this SDK method instead of duplicating the connection loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/brokkr-control/src/scheduler.rs`:
- Around line 1313-1316: Move the newly added tests, including
not_leader_returns_the_result_uncached_and_counts_it, below the complete
disconnect test and its documentation. Restore the full “§16 DoD” doc comment to
the disconnect test and ensure
disconnect_reassigns_in_flight_job_to_another_worker receives only its intended
documentation.
In `@crates/brokkr-worker/src/worker.rs`:
- Around line 200-208: Update the attempt handling in the worker session loop so
end.registered resets backoff only when the session remained healthy for the
established minimum attached duration; otherwise increment attempt with
saturating_add(1). Preserve the existing immediate reconnect reset for genuinely
useful sessions while ensuring sessions that fail immediately after Register
continue through rotation_plan backoff.
---
Nitpick comments:
In `@crates/brokkr-sdk/src/client.rs`:
- Around line 266-310: The new hot-path async helpers lack tracing spans. Add
#[tracing::instrument] attributes to both reconnect_to and follow_redirect,
preserving their existing behavior and avoiding sensitive authentication values
in captured fields.
- Around line 231-264: Extend Client::connect_any to accept optional TlsConfig
and bearer credentials, and select the corresponding connection method for each
TLS/bearer combination on every endpoint attempt. Preserve the existing ordered
iteration, success return, last-error propagation, and empty-endpoint fallback,
then update brokkr-cli’s connect_first caller to use this SDK method instead of
duplicating the connection loop.
In `@crates/brokkr-worker/src/endpoint.rs`:
- Around line 91-93: Update the shift calculation in the backoff logic around
factor so the completed_cycles-to-u32 conversion saturates at u32::MAX instead
of truncating on overflow, preserving the intended u64::MAX cap for excessively
large attempt values.
In `@crates/brokkr-worker/src/worker.rs`:
- Around line 236-247: Update the CAS URL setup around plane.worker_url() to
reuse the existing worker_url value instead of recomputing it, and replace the
stale multi-port configuration explanation with a concise comment stating that
split-port mode reaches CAS through the worker port (issue `#139`). Remove
references to worker_endpoint, control_endpoint, derive_default_worker_endpoint,
and --worker-control.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e40b2a8-bf1a-491b-baab-fd846d3f487a
📒 Files selected for processing (14)
CHANGELOG.mdcrates/brokkr-cli/src/main.rscrates/brokkr-control/src/scheduler.rscrates/brokkr-control/src/services/execution.rscrates/brokkr-control/tests/common/mod.rscrates/brokkr-control/tests/leader_redirect.rscrates/brokkr-sdk/src/client.rscrates/brokkr-sdk/src/lib.rscrates/brokkr-sdk/src/redirect.rscrates/brokkr-worker/src/endpoint.rscrates/brokkr-worker/src/lib.rscrates/brokkr-worker/src/main.rscrates/brokkr-worker/src/worker.rsdocs/journal/phase-5.md
| /// Decision D1 (I9b): on a `--raft` follower the post-execution | ||
| /// action-cache write is refused with `NotLeader`. The action already ran, | ||
| /// so the build must **succeed** with the real result, flagged uncached and | ||
| /// counted — never fail after paying for a sandbox run. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The new tests were inserted inside an existing doc comment, splitting it.
Lines 1311‑1312 (/// §16 DoD: a worker that dies mid-job … disconnect A) now attach to not_leader_returns_the_result_uncached_and_counts_it, and the remainder (/// (simulated crash), confirm the job is re-dispatched to B… at Line 1433) becomes the opening of disconnect_reassigns_in_flight_job_to_another_worker's doc. Move the new tests below the disconnect test (or after its complete doc block) so both doc comments read correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/brokkr-control/src/scheduler.rs` around lines 1313 - 1316, Move the
newly added tests, including
not_leader_returns_the_result_uncached_and_counts_it, below the complete
disconnect test and its documentation. Restore the full “§16 DoD” doc comment to
the disconnect test and ensure
disconnect_reassigns_in_flight_job_to_another_worker receives only its intended
documentation.
| // A session that registered proves the cluster is reachable, so the | ||
| // next outage starts its search fresh with no delay. Without this | ||
| // reset a long-lived worker would inherit the backoff from an outage | ||
| // hours earlier and crawl through a failover it should sail through. | ||
| attempt = if end.registered { | ||
| 0 | ||
| } else { | ||
| attempt.saturating_add(1) | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
A session that registers and then immediately fails resets the backoff, producing a hot reconnect loop.
attempt is reset to 0 whenever end.registered is true, and rotation_plan(_, 0) returns Duration::ZERO. A node that accepts Register but then drops the stream (or fails on stream/heartbeat right after) makes every iteration reset the counter, so the worker reconnects with no delay, indefinitely — the exact flapping case backoff exists for. Gate the reset on the session having actually been useful (e.g. minimum attached duration), not merely on registration.
🔒️ Sketch: reset only after a session that stayed up
+ let started = tokio::time::Instant::now();
let end = run_session(&cfg, plane).await;
+ let stable = started.elapsed() >= MIN_STABLE_SESSION; // e.g. 10s
@@
- attempt = if end.registered {
+ attempt = if end.registered && stable {
0
} else {
attempt.saturating_add(1)
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/brokkr-worker/src/worker.rs` around lines 200 - 208, Update the
attempt handling in the worker session loop so end.registered resets backoff
only when the session remained healthy for the established minimum attached
duration; otherwise increment attempt with saturating_add(1). Preserve the
existing immediate reconnect reset for genuinely useful sessions while ensuring
sessions that fail immediately after Register continue through rotation_plan
backoff.
CI's `cargo clippy --workspace --all-targets --locked -- -D warnings` rejected
`clippy::items_after_test_module`: the `clone_status` helper sat after
`#[cfg(test)] mod tests` in `client.rs`, because the edit that introduced it
appended to the end of the file.
Worth recording why local verification missed it: cargo does **not re-emit**
diagnostics for crates it considers fresh, so a clippy run made right after
touching only `brokkr-worker` stayed silent about an error already present in
`brokkr-sdk`. A green local clippy proves nothing unless the crate was
actually re-checked.
Re-verified with CI's commands verbatim, after touching every crate root to
force a real re-check:
cargo fmt --all --check -> ok
cargo clippy --workspace --all-targets --locked -- -D warnings -> 0
cargo doc --workspace --no-deps --all-features --locked -> 0
cargo test --workspace --all-targets --locked -> only the
6 known evil_seccomp_caps failures (real kernel required)
cargo test --workspace --doc --exclude brokkr-proto --locked -> 0
No behaviour change: the function is byte-identical, only relocated.
Motivation
I9a stood up a real multi-node control plane; I9b W1/W2 made its leader
redirect dialable. This closes the three remaining gaps from
docs/phase-5-plan.md§VII.1 that made the cluster unusable from outside —after this, a client or worker pointed at any node keeps working across a
leader kill.
What changed
1. A completed action is never discarded for a routing accident (D1)
The scheduler's post-execution action-cache write is best-effort on
NotLeaderonly: the build returns its real result, flagged uncached.Previously a
--raftfollower failed the whole Execute withINTERNALafter the sandbox had already run.
The recorded objection to best-effort was that it hides a routing problem as a
cache-hit-rate problem — so that objection is implemented as work:
Scheduler::uncached_results_not_leadercounter,warnnaming the digest, leader id and address, andExecuteResponse.message, so "ran andcached" and "ran, not cached" are distinguishable without server logs.
And it is narrow: a
Redbor throughput error still fails the RPC, with anamed test for exactly that. Best-effort must not become best-ignored.
2. Workers survive losing the node they attached to
--control/--worker-controlare repeatable;run_workerrotates onfailure and re-registers (the registry is per-node and ephemeral by ADR
design, so a new node has never heard of this worker).
rotation_plan(len, attempt)is a pure function: the whole first cycle runswith zero delay — the survivors are up now, so backing off before trying
them adds latency for nothing — then exponential backoff per completed cycle
capped at 30 s, with attempt-derived jitter so co-restarted fleets do not
reconnect in lockstep. A session that registered resets the backoff, or a
worker running for hours would inherit an old outage's delay and crawl through
a failover it should sail through.
3. Clients follow the redirect
brokkr_sdk::redirectclassifies a refusal;BrokkrClient::{get_action_result, update_action_result}follow it, bounded byMAX_LEADER_HOPS, keeping the followed connection so steady state is one hop.Two details worth review:
FAILED_PRECONDITIONalone — the scheduler already returns that code for "no eligible worker", so
code-only matching would retry an unrelated failure against a different
node.
hint_to_urlinherits the caller's scheme, so a redirect can neverdowngrade an https session to plaintext.
connect_anyand a repeatablebrokk run --controluse the first node thatanswers, reporting the last transport error rather than a synthetic
aggregate that hides whether the cause was refusal or a bad certificate.
A consequence stated plainly
Because D1 means
Executeno longer fails onNotLeader, and the CLI nevercalls the ActionCache directly (the scheduler does cache lookups server-side),
no in-tree client RPC returns a leader redirect through
brokk run. Thesurface that matters is the REAPI one an external client (Bazel) drives — hence
the new SDK ActionCache methods, so the policy has a real consumer rather than
being exported and called done.
How it was tested
19 new tests, each written before its implementation. Extracting
rotation_planandredirect::classifyas pure functions and testing themexhaustively caught four bugs no happy-path integration test reaches: a
1 << noverflow panic after ~64 reconnect cycles, a modulo-by-zero on anempty endpoint set, an https→http downgrade, and a redirect cycle that hung
instead of terminating.
non-
NotLeaderfailure still fails the RPC)rotation_plan; 2 for endpoint pairingclassify/hint_to_urlleader_redirect.rsdriving realActionCacheServiceservers: a client on a follower reaches the leader forread and write; an unroutable hint surfaces the original refusal with its
metadata intact; a self-referential hint terminates on the hop budget.
Verified in WSL2:
fmt,clippy --workspace --all-targets -D warnings,RUSTDOCFLAGS=-Dwarnings cargo doc --no-deps(exit 0), andcargo test --workspace --no-fail-fast— 48 suites, the only failuresbeing the 6
evil_seccomp_capstests that need a real kernel for seccompargument filters (
docs/brokkr-development-chronicle.md:280).No new dependency.
--raftoff remains the single-node path.Related
docs/plan.md§17 task 7;docs/phase-5-plan.md§VII.2 (decision D1) and§VII.3 (W3–W5); ADRs 0008–0010 (why the worker registry is ephemeral), 0011
(auth planes), 0013 (custom Raft);
docs/journal/phase-5.md(## I9b W3–W5).Next is W6 — the real-process kill-the-leader end-to-end.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation