node,broker,chaos: candidate mode — the role follows the lease (#284) - #343
Conversation
The design conversation on the issue chose this shape; this commit is its core, checkpointed before the harness proofs land. Both planes bind ONCE and never move. The native listener serves over a BrokerSlot — the broker behind a candidate's socket is rebuilt at role transitions, so the slot is read per session accept: sessions in flight keep the broker they started with, which fails closed the moment it is fenced; an empty slot (a candidate not leading) refuses the socket with its own counter, because 'we are full' and 'we are not the leader' point an operator at opposite remedies. The replica listener serves a switching handler: InProcessFollower while following, the leader's status-and-transfer surface while leading, refusing placeholders mid-transition. The lease agent runs for the life of the process. Its publisher for a candidate is a VERDICT RECORDER: promotion cannot take effect in the publisher because the leader it promotes does not exist until the supervisor builds it, so promote only records and the supervisor completes it — building the replica set from peers minus self, the broker over the reopened range, and only then replaying the proven boundary into the real publisher. Demotion and suspension cannot wait for a build step, so they forward to the current role object immediately and record second. Transitions re-open the range from disk after quiescing — the two state machines that own storage take the SegmentSet by value, and the directory is the handoff, exactly as it is between processes, minus the processes. The probe's self-view generalizes to a trait (CandidateLocalView) answered by whichever role currently is. The config is symmetric: role candidate + peers (the whole set, self included, identical on every member; self filtered by node_uuid), the retirement of the per-node role edit the chart currently encodes as leaderOrdinal. No role collector in this slice, deliberately: the two existing collectors export the same metric names and the registry has no unregister path — the role-agnostic collector is the follow-up.
…efuses mid-swap The two behaviors everything else hangs off, pinned without a disk: a promotion is recorded for the supervisor and must NOT reach the role object from the publisher (the broker it authorizes does not exist yet), while a demotion forwards immediately because fail-closed has no build step to wait for; and the switching handler's mid-transition delegate refuses with a message that says WHY, so a peer retries instead of diagnosing.
…aps it found The composition proof for candidate mode, plus the three defects it surfaced — each one invisible to every unit test around it, which is the suite's whole doctrine. One: metadata grants leases only to REGISTERED nodes, and a refused acquisition is indistinguishable from a lost race on the agent side by design — three healthy candidates polled forever while metadata refused every one. The scenario documents the trap; the agent now says 'lease agent running' at start of life, because an agent that never logs is indistinguishable from an agent that never ran, and the difference cost a debugging session. Two: the watcher and the agent speak different lease dialects. A watcher translates an OBSERVED grant into promote — 'serve this epoch' — while the agent reports a rival's grant as demote. For a FOLLOWING candidate every demote IS that observation (a non-holder has no renewal to lose), so a FollowerObservationAdapter translates it back; without it a following candidate cleared its lease view on every poll and refused every append from the leader it followed — three healthy replicas, zero acks. Three: a dead leader's unexpired lease is still on record, correctly, so waiting for 'any holder' after a kill reads back the corpse: await_lease_holder_changed waits for the holder to CHANGE. The scenario's log assertions are deadline-polled (await_log_line) per the suite's own doctrine — the lease appears in metadata a beat before the winner logs its role change, and a one-shot grep raced it. What the scenario now proves, live: three identical configs; an unscripted election; 600 records quorum-acked at the winner's own address; SIGKILL mid-hold; a survivor takes the range at the next epoch with no process started, no config rewritten, and no port moved; produce resumes on the survivor's own address; every pre-kill acknowledged record intact. Scenario 09 still passes — the existing roles are untouched.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0c593c217
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switching.transitioning(); | ||
| publisher.set_target(None); |
There was a problem hiding this comment.
In candidate mode, if the lease is lost while build_leader_phase is awaiting follower connections, clearing the publisher target causes the agent's synchronous demote to reach no role object; only the queued Follow verdict remains. The builder can subsequently promote and install a broker at the stale epoch before the supervisor processes that verdict, briefly admitting native sessions after metadata has already authorized another holder. Keep a fail-closed target or recheck the current verdict before publishing/installing the broker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real — the sharpest finding on the PR, fixed in f93c92e. build_leader_phase is now PURE CONSTRUCTION (nothing installs, promotes, or becomes reachable inside it), and the supervisor rechecks the verdict before completing the promotion: a lease that moved during the build wins — the built broker is quiesced and the node rebuilds as follower, logging why. The residual instant after the recheck is closed by the queued verdict: the loop's next iteration runs the full demotion before anything else happens, and the target is installed before the promote so a demote landing in that instant fences the broker rather than vanishing.
| Err(error) => { | ||
| // The lease is held but the leader could not be | ||
| // built. Fail closed as a follower again: the | ||
| // unrenewed lease lapses and another candidate | ||
| // wins. | ||
| eprintln!("promotion failed to build the leader: {error}"); |
There was a problem hiding this comment.
When candidate leader construction fails but reopening the follower succeeds—for example, a transient failure rereading replica TLS material—the lease agent has already set its state to Held and marked this epoch verified. This branch installs a follower without notifying the agent, so subsequent rounds continue renewing the lease indefinitely while the native slot remains empty; contrary to the comment, the lease never lapses and no healthy candidate can take over. Explicitly suspend/release the hold when this recovery branch is entered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, fixed in f93c92e — with a stronger posture than suggested: a failed leader build is now FAIL-STOP. The agent had already verified the epoch and would renew a lease over an empty slot forever, and there is no agent API to un-verify; the one state worse than a dead node is a live one holding a range it cannot serve. Exiting lets the lease lapse and a healthy candidate win — and a supervisor (or Kubernetes) restarts the process into a clean campaign.
| let _ = release_lease.send(true); | ||
| let _ = tokio::time::timeout(agent_drain, &mut agent_task).await; |
There was a problem hiding this comment.
On an orderly shutdown of a leading candidate, this signals the lease agent before awaiting or otherwise confirming termination of the separately spawned native server. The release RPC can therefore authorize a successor while an admitted request is still executing under the old epoch; native requests run in spawn_blocking, so even cancellation of the server task does not necessarily stop the broker operation. Candidate shutdown needs the same ordering as run_leader: stop admission and await session termination before releasing the lease.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real, fixed in f93c92e: candidate shutdown now follows run_leader's own ordering in-process — slot.clear stops admission, the native task (whose serve joins its sessions) is awaited within the drain budget, and only then does the release fire. The invariant comment from run_leader is quoted at the site.
| tokio::spawn(async move { | ||
| if let Err(error) = native_server.serve(native_listener, native_shutdown).await { | ||
| tracing::warn!(%error, "candidate native server exited"); | ||
| } |
There was a problem hiding this comment.
In candidate mode, if the native server exits because accept fails or a connection task panics, this detached task only logs the error. The candidate supervisor and lease agent continue running, and a holder can remain /readyz-ready because readiness checks the broker slot and lease view rather than whether this task is alive, leaving metadata pinned to a leader whose client endpoint is dead. Retain and monitor the join handle so an unexpected listener exit makes the node unready or terminates the supervisor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in f93c92e: both server task handles are held and the supervisor selects on them — an early exit is fail-stop, for the reason you name: readiness reads the slot, not the task, and a ready leader with a dead endpoint pins metadata to a leader nobody can reach.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/vtop-node/src/config.rs">
<violation number="1" location="crates/vtop-node/src/config.rs:238">
P1: A duplicated non-self peer can be counted as multiple replicas, letting one reachable node satisfy a quorum intended to require distinct members; validate that candidate peer UUIDs are unique before deriving endpoints/followers.</violation>
</file>
<file name="crates/vtop-broker/src/server_metrics.rs">
<violation number="1" location="crates/vtop-broker/src/server_metrics.rs:327">
P2: The new sessions_refused_no_broker counter is recorded and exposed via sessions_refused_no_broker_total(), but no exporter consumes that getter: the sessions_refused metric family in vtop-node observe.rs still maps only capacity/unauthorized/handshake. The count the PR intends for operators ('we are not the leader') is therefore never published, so the diagnostic silently disappears. Add a 'no_broker' entry to that family so the data reaches a metric.</violation>
</file>
<file name="scripts/live-chaos/lib.sh">
<violation number="1" location="scripts/live-chaos/lib.sh:1295">
P2: Candidate-mode port derivation is outside `preflight_settings` bounds: accepted high base ports generate invalid native/metrics listener addresses and make scenario 14 fail at startup. Extend bounds and collision checks for candidate indices 11–13.
(Based on your team's feedback about derived metrics port ceilings.)</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// by hand. The follower list a promotion needs, and the transfer | ||
| /// allowlist a repair needs, are both derived as peers minus self. | ||
| #[serde(default)] | ||
| pub peers: Vec<FollowerPeerConfig>, |
There was a problem hiding this comment.
P1: A duplicated non-self peer can be counted as multiple replicas, letting one reachable node satisfy a quorum intended to require distinct members; validate that candidate peer UUIDs are unique before deriving endpoints/followers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-node/src/config.rs, line 238:
<comment>A duplicated non-self peer can be counted as multiple replicas, letting one reachable node satisfy a quorum intended to require distinct members; validate that candidate peer UUIDs are unique before deriving endpoints/followers.</comment>
<file context>
@@ -218,6 +228,14 @@ pub struct DataNodeConfig {
+ /// by hand. The follower list a promotion needs, and the transfer
+ /// allowlist a repair needs, are both derived as peers minus self.
+ #[serde(default)]
+ pub peers: Vec<FollowerPeerConfig>,
/// Identity on the replication plane (CN = node_uuid).
pub replica_tls: TlsPaths,
</file context>
There was a problem hiding this comment.
Valid, fixed in f93c92e: duplicate peer UUIDs are refused at startup — 'a quorum over duplicated members is a quorum in name only.'
| local n="$1" id="$2" pid | ||
| pid="$(start_node "data-candidate-$n" "data_node_ready" data \ | ||
| --config "$(emit_candidate_config "$n" "$id")")" | ||
| await_ready "$(data_metrics_addr $((n + 10)))" "data-candidate-$n" |
There was a problem hiding this comment.
P2: Candidate-mode port derivation is outside preflight_settings bounds: accepted high base ports generate invalid native/metrics listener addresses and make scenario 14 fail at startup. Extend bounds and collision checks for candidate indices 11–13.
(Based on your team's feedback about derived metrics port ceilings.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/live-chaos/lib.sh, line 1295:
<comment>Candidate-mode port derivation is outside `preflight_settings` bounds: accepted high base ports generate invalid native/metrics listener addresses and make scenario 14 fail at startup. Extend bounds and collision checks for candidate indices 11–13.
(Based on your team's feedback about derived metrics port ceilings.) </comment>
<file context>
@@ -1225,6 +1225,144 @@ start_colocated_node() {
+ local n="$1" id="$2" pid
+ pid="$(start_node "data-candidate-$n" "data_node_ready" data \
+ --config "$(emit_candidate_config "$n" "$id")")"
+ await_ready "$(data_metrics_addr $((n + 10)))" "data-candidate-$n"
+ echo "$pid"
+}
</file context>
There was a problem hiding this comment.
Valid, fixed in f93c92e: the native and data-metrics ceilings now leave headroom for the candidate offsets (indices 11..13), with the derivation reasoning stated beside the bounds.
| self.sessions_refused_at_capacity.load(Ordering::Relaxed) | ||
| } | ||
|
|
||
| pub fn sessions_refused_no_broker_total(&self) -> u64 { |
There was a problem hiding this comment.
P2: The new sessions_refused_no_broker counter is recorded and exposed via sessions_refused_no_broker_total(), but no exporter consumes that getter: the sessions_refused metric family in vtop-node observe.rs still maps only capacity/unauthorized/handshake. The count the PR intends for operators ('we are not the leader') is therefore never published, so the diagnostic silently disappears. Add a 'no_broker' entry to that family so the data reaches a metric.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/vtop-broker/src/server_metrics.rs, line 327:
<comment>The new sessions_refused_no_broker counter is recorded and exposed via sessions_refused_no_broker_total(), but no exporter consumes that getter: the sessions_refused metric family in vtop-node observe.rs still maps only capacity/unauthorized/handshake. The count the PR intends for operators ('we are not the leader') is therefore never published, so the diagnostic silently disappears. Add a 'no_broker' entry to that family so the data reaches a metric.</comment>
<file context>
@@ -314,6 +324,10 @@ impl ServerMetrics {
self.sessions_refused_at_capacity.load(Ordering::Relaxed)
}
+ pub fn sessions_refused_no_broker_total(&self) -> u64 {
+ self.sessions_refused_no_broker.load(Ordering::Relaxed)
+ }
</file context>
There was a problem hiding this comment.
Valid, fixed in f93c92e: the family gains a no_broker entry, so the diagnostic reaches operators instead of dying in the getter.
…oncern CI shellcheck (SC2034): the identity's second field was read into a variable nothing used; a peer entry names the uuid and where to dial it, and the underscore says so.
… and seven more Nine distinct findings from fourteen threads, every one verified real. THE RACE (P0, both reviewers): a lease lost during build_leader_phase — the follower-stream wait can take seconds — recorded its demote into a void target and queued a Follow verdict, and the builder then completed the STALE promotion: broker installed, meta view re-activated, serving at an epoch metadata had moved past until the loop's next iteration. The build is now PURE CONSTRUCTION; the supervisor rechecks the verdict before anything installs, a moved lease wins over the build (the built broker is quiesced and the node rebuilds as follower, saying why), and the residual instant after the recheck is closed by the queued verdict running the full demotion before anything else happens. FAIL-STOP, twice (both reviewers): a candidate whose leader build fails used to fall back to following while the agent — which had already verified the epoch — renewed a lease over an empty slot forever; the one state worse than a dead node is a live one holding a range it cannot serve. A failed build now exits the process, the lease lapses, and a healthy candidate wins. Same posture for a dying plane: both server task handles are held and selected on, because readiness reads the slot, not the task, and a ready leader with a dead endpoint pins metadata to a leader nobody can reach. THE DRAIN follows run_leader's own invariant in-process: stop admission, drain the native sessions, and only then release — a release racing an admitted produce would authorize a successor under a broker still acking at the old epoch. And the rest: transitions clear the switching view BEFORE dropping the old role object, so its Arc cannot keep the previous storage owner alive across the reopen — the directory has exactly one owner at a time; duplicate candidate peers are refused, because a quorum over duplicated members is a quorum in name only; the no_broker refusal actually reaches the exported metric family; its counter doc names both causes; the harness port ceilings leave headroom for candidate offsets; and scenario 14's post-failover produce bumps the PRODUCER epoch — under the old one the broker deduplicated the batch and the scenario proved nothing — with content verification bounded through the pre-kill floor, since the fresh batch's bytes are not reconstructible from offsets (the helper's own documented contract). Scenario 14 PASS end to end with all of it; 17 suites, clippy, fmt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f93c92ef74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let current = *verdict_rx.borrow(); | ||
| if current | ||
| != (RoleVerdict::Lead { |
There was a problem hiding this comment.
If the lease agent publishes Follow after this borrow but before set_target and slot.install, demotion still reaches no target; the supervisor then promotes and publishes the stale broker until its next loop iteration, allowing another task to accept a native session while metadata already names a successor. The fresh evidence is that the newly added verdict recheck remains separate from the subsequent target and slot writes, so it does not close the previously reported race; synchronize verdict updates with publication or perform a fail-closed atomic handoff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You were right to hold the line — the recheck only narrowed the window. Fixed properly in 5d086a5: complete_promotion publishes the boundary and installs the demote target under the SAME lock the demote path holds, gated on a finished-epochs ceiling that demote raises inside its own critical section. A demotion racing a completion either lands first (the ceiling refuses it — nothing was published, the supervisor rebuilds as follower) or lands second (it reaches the broker publisher just installed and fences the live broker). No third interleaving exists. Observed rival epochs cannot refuse a legitimate grant — metadata mints epochs monotonically, so a grant to this node is strictly above anything it watched a rival hold. The unit test now replays the race in miniature: demote-then-complete publishes nothing; complete-then-demote fences the target.
| let _ = tokio::time::timeout(agent_drain, &mut native_task).await; | ||
| let _ = release_lease.send(true); |
There was a problem hiding this comment.
During orderly shutdown with an admitted produce/fetch, awaiting native_task does not prove the broker operation ended: NativeServer::serve aborts the session future, while its spawn_blocking request continues after that future is cancelled. The fresh evidence is that the newly added timeout result is ignored and release proceeds on the next line even if the task has not completed; moreover, the task itself may complete after aborting a session while blocking work remains. Track and await the blocking operations before signaling lease release.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real — abort_all on the session set cannot cancel a request already inside spawn_blocking, and the finding pre-dates candidates (run_leader's own drain releases before its quiesce with the same session machinery). For the candidate path, 5d086a5 does two things: the drain timeout is logged instead of swallowed, and the final quiesce moves BEFORE the release — quiesce takes the very state mutex an admitted append holds through its critical section, so it cannot return until in-flight blocking work has committed, durable before metadata may authorize a successor. A straggler reaching the lock after that has no client left to ack (its session future is gone) and cannot reach quorum once the successor fences the followers — the SIGKILL exposure, which the protocol already tolerates by design. Full per-request tracking of blocking work would be a broker-level change shared with run_leader; noted as follow-up scope rather than grown in this PR.
| require_integer_in_range CHAOS_DATA_METRICS_BASE_PORT "$DATA_METRICS_BASE_PORT" 1024 65532 | ||
| # Data indices run 0..3 for the fixed-role scenarios and 11..13 for the | ||
| # candidate scenario (#284), so the top base is 65535-13. | ||
| require_integer_in_range CHAOS_DATA_METRICS_BASE_PORT "$DATA_METRICS_BASE_PORT" 1024 65522 |
There was a problem hiding this comment.
When operators override the chaos port bases, this updated ceiling validates candidate-derived ports but the endpoint collision loop still includes only the single fixed native address and data metrics indices 0–3. Consequently collisions involving NATIVE_PORT+11..13 or DATA_METRICS_BASE_PORT+11..13 pass preflight and scenario 14 later fails at listener bind; add those candidate native and metrics endpoints to the collision set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, fixed in 5d086a5: the collision set now includes candidate-native-1..3 and candidate-metrics-1..3 (offsets 11..13 above the shared bases), so an operator override that lands one on an occupied endpoint fails at preflight with a named pair instead of at a mid-scenario bind.
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 5 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…moves before the release The round-one recheck only NARROWED the promotion race; the reviewer correctly held that a check separate from publication leaves the window open. complete_promotion closes it: the boundary publish and the target install happen under the same lock the demote path holds, gated on a finished-epochs ceiling the demote raises inside its own critical section. A demotion racing a completion now either lands first — the ceiling refuses the completion and nothing was published — or lands second and reaches the broker publisher just installed, fencing the live broker. There is no third interleaving. Observed rival epochs cannot refuse a legitimate grant: metadata mints epochs monotonically, so a grant to this node sits strictly above every epoch it watched a rival hold. The unit test replays the race in miniature. The second finding is subtler and pre-dates candidates: aborting a session future cannot cancel a request already inside spawn_blocking, so awaiting the native drain never proved the broker work ended. The drain timeout is now said out loud instead of swallowed, and the final quiesce moves BEFORE the release: quiesce takes the very state lock an admitted append holds through its critical section, so it cannot return until in-flight blocking work has committed — durable before metadata may authorize a successor. A straggler reaching the lock after that has no client left to ack and cannot reach quorum once the successor fences the followers: the SIGKILL exposure, which the protocol already tolerates. And the preflight now checks the candidate-derived listeners (offsets 11..13 above the shared bases) for collisions, so an operator override fails at preflight with a named pair instead of at a mid-scenario bind. Scenario 14 PASS end to end; 66 vtop-node tests, clippy, fmt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d086a57a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result = &mut native_task => { | ||
| return Err(match result { | ||
| Ok(Ok(())) => "candidate native server exited early".to_owned(), | ||
| Ok(Err(error)) => error, | ||
| Err(join) => format!("candidate native server task failed: {join}"), | ||
| }); |
There was a problem hiding this comment.
When graceful shutdown fires, the same watch also signals both listener oneshots, so native_task or replica_task may finish successfully before this tokio::select! chooses shutdown.changed(). This branch then classifies the expected completion as an early exit and returns before the drain/release logic at the bottom of run_candidate, causing an orderly candidate leader to let its lease expire instead of releasing it promptly. Check the shutdown state before treating a completed listener as fatal, or funnel all three shutdown outcomes through the cleanup path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real — twin of cubic's finding, fixed in 85e61c6 (see that reply): shutdown-flag check in both listener arms, break to the drain path, done flag so the finished task is not re-polled.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…eded build is refused The fail-stop select arms watched the listener tasks for a dying plane, but the same watch that breaks the supervisor loop also stops both servers — and select does not order its arms. A listener that finished its orderly shutdown first was read as an early exit: the node returned before the drain, never released its lease, and an orderly stop turned into a lease expiry the cluster had to wait out. The arms now check the shutdown flag first; a completed listener under shutdown breaks to the drain path (with a done flag so the finished task is never polled again), and only an exit with the flag clear is the fail-stop it was built for. The publisher gains a second watermark beside the finished ceiling: the highest epoch ever RECORDED as a promotion. Suspend-then-regrant never raises the finished ceiling — suspension is deliberately not a role change — so a build still in flight for the old grant could complete after the new grant was recorded, publishing a boundary metadata had moved past. A completion below the recorded watermark is now refused under the same lock; the refusal is safe because the newer verdict is already queued, and the supervisor's next iteration builds for the grant that superseded this one. The unit test pins both refusals and the completion of the latest grant. Scenario 14 PASS end to end; 66 vtop-node tests, clippy, fmt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85e61c683d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ), | ||
| }, | ||
| 2 => vtop_observe::Readiness::not_ready("role transition in progress".to_owned()), | ||
| _ => vtop_observe::Readiness::Ready, |
There was a problem hiding this comment.
When a candidate starts while metadata is unavailable, meta remains inactive and its new follower refuses every replication append, yet this default branch reports /readyz as ready indefinitely. The static follower path explicitly gates readiness on its first metadata read for the same reason; candidate mode should likewise distinguish an initialized follower from one that has not learned the current fencing epoch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real — the false green the watcher gate exists to prevent, reintroduced on the candidate path. Fixed in c0d20f6 with the same doctrine, applied to the agent: LeaseAgent takes an optional readiness gate and opens it on its first COMPLETED metadata exchange, whatever it said (granted, refused, rival held — each tells the node where it stands; silence tells it nothing). The candidate now requires two marks — binds plus first contact — the exact shape run_follower has always had.
| let built = | ||
| build_leader_phase(&config, &range, &peers, roll, &meta).await?; |
There was a problem hiding this comment.
If either listener exits while this await is waiting up to ten seconds for follower streams, the supervisor cannot reach the tokio::select! arms that monitor its join handles, while the independent lease agent continues renewing the range. Metadata therefore remains pinned to a candidate with a dead plane until construction finishes. The fresh evidence beyond the earlier listener-monitoring comment is that the newly retained handles are not polled during this long inline await; race construction against listener completion so fail-stop remains prompt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real: the ten-second follower-stream wait made the select arms unreachable while the independent agent kept renewing — metadata pinned to a candidate with a dead plane until the build came home to notice. Fixed in c0d20f6: the build is raced in a nested select against both listener handles and the shutdown watch. A listener dying mid-build is the same fail-stop as ever, just prompt; shutdown mid-build abandons a leader nothing has published yet (the atomic completion guarantees that) and runs the ordinary drain.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…er blinds the supervisor A candidate that has never reached metadata used to answer /readyz green while its follower refused every append — the exact false green the static follower's watcher gate was built to prevent, and the same doctrine now covers the agent: the lease agent takes an optional readiness gate and opens it on its first COMPLETED metadata exchange, whatever the exchange said. Granted, refused, rival held — each tells this node where it stands; silence tells it nothing, and a node that knows nothing is not ready. The candidate requires two marks, binds plus first contact, the same shape run_follower has always had. The leader build could wait ten seconds on follower streams, and for that whole window the supervisor's select arms were unreachable — a listener could die mid-build while the independent lease agent kept renewing, pinning metadata to a candidate with a dead plane until the build came home to notice. The build is now raced against the very things those arms watch: a listener dying mid-build is the same fail-stop as ever, just prompt, and shutdown mid-build abandons a leader nothing has published yet and runs the ordinary drain. And the promotion watermark moves under the completion's own lock: recorded outside it, a newer grant's record could land between an older completion's check and its install, and the stale build would publish anyway. Inside it the two linearize — the completion either sees the record and refuses, or finishes first and the queued verdict re-promotes the standing leader at the newer epoch. Scenario 14 PASS end to end; 66 vtop-node tests, clippy, fmt.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0d20f6429
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn demote(&self, fencing_epoch: u64) { | ||
| // A rival holds the range at this epoch: to a follower, that IS the | ||
| // grant to serve under. | ||
| self.inner.promote(fencing_epoch, None); |
There was a problem hiding this comment.
When a following candidate observes rival epoch E, this translation activates its follower lease view at E. If later metadata reads time out, however, LeaseAgent::run only demotes locally for LeaseState::Held; a follower is NotHeld, so this adapter never receives suspend. The follower therefore keeps accepting replication at stale epoch E even after the lease may have turned over, and refuses a successor's fence at E+1 until metadata connectivity returns. Preserve the last rival observation and suspend it on metadata errors, as LeaseWatcher does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real — the sharpest of this round. The Wait arm fenced the view to the rival's epoch but nothing remembered it, so a metadata outage left the follower accepting replication at stale E on the memory of a read a turnover may have superseded. Fixed in 55e5682 with the watcher's exact doctrine ported to the agent: the Wait arm records the observed rival epoch, and every failing round while NOT held suspends it — fail-closed, reactivatable the moment a read lands (suspension is idempotent, so repeating it is free).
| _ => vtop_observe::Readiness::not_ready( | ||
| "leading, but the lease view is contended or the broker is absent".to_owned(), | ||
| ), |
There was a problem hiding this comment.
For a leading candidate, any produce or fetch holds MetaFencingEpoch for its entire critical section, including fsync, so try_snapshot() legitimately returns None under normal traffic. This branch converts that transient contention directly into not-ready, unlike run_leader, which caches the last decided verdict. A Kubernetes readiness probe landing during an active request can consequently remove a healthy current leader from service and cause readiness flapping; retain and serve the last verified readiness state when only the lease lock is contended.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real, fixed in 55e5682: the leading branch now mirrors run_leader's probe contract — non-blocking, and lease-view contention serves the LAST DECIDED verdict instead of guessing. A produce mid-fsync no longer converts a probe landing in its critical section into a drained leader; an absent broker still reports not-ready and resets the cached verdict.
…er, and probes stop guessing Three of these four close gaps between what the lease WATCHER learned the hard way and what the lease AGENT inherited. A range metadata says does not exist is a real answer, but not one that makes a node ready: the gate now stays closed on RangeMissing — a candidate configured against a deleted range is a configuration fault, and the watcher keeps readiness closed for exactly that case. A following candidate that loses sight of metadata now suspends the rival epoch it last observed, every failing round, reactivatable the moment a read lands. Losing sight of metadata is not the lease ending — but it is the end of knowing the observed epoch is still current, and accepting replication on the memory of a read a turnover may have superseded is the split-brain window the watcher closed years of lessons ago. Same doctrine, agent's side of the fence. The leading readiness probe now serves the LAST DECIDED verdict when the lease view is contended, exactly as run_leader's does: a produce mid-fsync holds that view for its whole critical section, and a Kubernetes probe landing in that window was converting healthy traffic into a drained leader. And in the build race, select does not order its arms: a build completing in the same instant as shutdown is still a shutdown — the unpublished leader is abandoned, and a FAILED build under shutdown no longer turns an orderly stop into a build-failure exit. Scenario 14 PASS end to end; 66 vtop-node tests, clippy, fmt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55e56820c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn demote(&self, fencing_epoch: u64) { | ||
| // A rival holds the range at this epoch: to a follower, that IS the | ||
| // grant to serve under. | ||
| self.inner.promote(fencing_epoch, None); |
There was a problem hiding this comment.
When a leading candidate observes a successor at epoch E, CandidateLeasePublisher::demote(E) first forwards to the broker, whose clear_lease(E) records E in the shared MetaFencingEpoch::released_through. The supervisor then constructs the follower with that same lease view, so this promote(E) cannot reactivate it (set requires an epoch greater than released_through). Consequently, a deposed but otherwise healthy candidate refuses every append from its successor for the successor's entire epoch, permanently reducing the replica set until another election; rebuild with a fresh role-appropriate view or avoid recording the rival's live epoch as released.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real — the sharpest finding since the atomic completion, and the follower publisher's own comment warns against exactly this clear_lease semantics. Fixed in 94b6eaa: the leading target now demotes through a dialect adapter that fences the epoch THIS node held rather than the rival's. run_leader's clear-through-rival is correct for a static broker that never follows; a candidate becomes the rival's follower next, and clearing its own held epoch fences it just as surely while leaving the successor's epoch free to activate the rebuilt follower. New unit test walks the exact sequence (grant at 1, deposed by 2, view must activate at 2) with the stake in the assertion message.
| Ok(self.config.poll_interval) | ||
| } | ||
| LeaseDecision::RangeMissing => { | ||
| self.range_missing = true; |
There was a problem hiding this comment.
Unlike the previously fixed metadata-error path, a successful metadata read returning RangeMissing only sets this readiness bookkeeping flag and demotes when self.state is Held. A following candidate therefore leaves its previously observed rival epoch active, continues reporting ready after the gate has already opened, and accepts authenticated replication appends for a range metadata has deleted. Forward a demotion for observed_rival in this branch so the follower fails closed on the definitive no-range result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real, fixed in 94b6eaa: the RangeMissing arm now fails the FOLLOWING view closed by suspending the last observed rival epoch. Suspend rather than demote, deliberately — through the candidate's observation dialect a demotion means 'rival grant, serve this epoch' (it would ACTIVATE the view), while suspension is the fail-closed verb in both dialects and stays reactivatable should the range be recreated and a new grant observed. Repeated every round the range stays missing; suspension is idempotent.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The sharpest finding on the PR since the atomic completion: demotion carried the RIVAL's epoch to clear_lease, which records it in the shared view's released_through — and the rebuilt follower then could not activate the successor's grant, because set() requires an epoch strictly above that mark. A deposed but otherwise healthy candidate refused every append for the successor's entire reign; the replica set shrank by one until the next election, silently. run_leader's static broker does the same clear and it is correct THERE — that process never follows. A candidate becomes the rival's follower NEXT, so the leading target now demotes through a dialect adapter that fences the epoch THIS node held: just as fenced, and the successor's epoch left free to activate the rebuilt follower. The unit test walks the exact sequence — grant at 1, deposed by 2, view must activate at 2 — and its assertion message states the stake. RangeMissing now reaches a FOLLOWING view too: metadata answering 'that range does not exist' left the last observed rival epoch active, accepting authenticated replication for a deleted range. Suspend, not demote — in the candidate's observation dialect a demotion means 'rival grant, serve this epoch', while suspension fails closed in both dialects and stays reactivatable should the range be recreated. The same answer also revokes readiness through a persistent gate handle — the consumable first-open mark cannot say 'no longer', and a node serving a deleted range is a configuration fault however long it has been up — restored on the first exchange that finds the range again. And the cached probe verdict now resets on every role transition: it belonged to the role that decided it, and a fresh promotion must start fail-closed, not inherit the prior leader's green. Scenario 14 PASS end to end; 67 vtop-node tests, clippy, fmt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94b6eaa3bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // EVERY Arc dropped before the directory reopens: the | ||
| // handler and view already point elsewhere, and this | ||
| // binding was the last. | ||
| drop(follower); |
There was a problem hiding this comment.
When promotion races a replication RPC that already obtained the old delegate from SwitchingReplicaHandler::current(), that RPC retains its own Arc<InProcessFollower>, so this binding is not necessarily the last one. For example, an append can be descheduled after cloning the delegate, then resume after quiesce() and after build_leader_phase has reopened the same files; because the shared lease view still accepts the previous leader's epoch until promotion completes, the old follower can mutate storage behind the new broker's already-open SegmentSet, leaving its offsets and file state stale. Fence the follower and drain outstanding replica calls before reopening the directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real — the retained Arc outlives the handler switch, and dropping our binding does not end a call already in flight. Fixed in b3ab64b using the lock discipline the append path already had: every replication call holds the shared meta view's lock from fence check through write, so suspending the view — which must take that same lock — IS the drain. The suspend cannot return until the last in-flight call finishes, every later call refuses under the guard, and only then does the transition quiesce and reopen; the retained object outlives the swap as a refusal machine, not a writer. The promotion's own set() reactivates the view at the granted epoch. The mirror transition (Leading→Following) was already closed — the forwarded demotion takes the same lock before teardown begins.
| // Two marks before /readyz goes green, same shape as the static | ||
| // follower: the binds below are one, the agent's first completed | ||
| // metadata exchange is the other (review: a candidate that has never | ||
| // reached metadata reports a follower that refuses every append). | ||
| observability.gate.require_marks(2); |
There was a problem hiding this comment.
When candidate mode is used inside colocated::run, this overwrites the shared gate's existing two-role count instead of adding the candidate's metadata-observation prerequisite. There are then three independent startup marks—the metadata role, candidate listener setup, and the lease agent—but any two open /readyz; depending on poll order, the co-located process can report ready before the candidate has ever observed an epoch or before the metadata listener has finished starting. Use an additive/component-aware readiness contract rather than resetting the shared counter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real — and true of the static follower under colocation too, not just the candidate: require_marks states a TOTAL, so the two roles sharing the gate overwrote each other's count and any two of three components could open /readyz. Fixed in b3ab64b: ReadinessGate gains add_required_marks, and both run_follower and run_candidate add their extra mark instead of declaring totals — colocated's meta-role mark stays counted in every combination.
…n, and marks that add up A replication call that took the old delegate before the handler switched still holds its own Arc, and dropping our binding does not end it — it can resume after the transition has reopened the directory and write behind the new owner's open segment set. The fix is the lock discipline the append path already had: every call holds the shared meta view's lock from fence check through write, so suspending the view — which must take that same lock — IS the drain. It cannot return until the last in-flight call has finished, and every call after it refuses under the guard; the retained object outlives the swap as a refusal machine, not a writer. The suspension sits immediately before the transition's quiesce, and the promotion's own set() reactivates the view at the granted epoch. The mirror-image transition was already closed: the forwarded demotion takes the same lock before the teardown begins. And ReadinessGate::require_marks states a TOTAL, which cannot compose: under colocated::run the gate is shared with the meta role's mark, and each data role declaring 'two' overwrote the conjunction — any two of three components could open /readyz. That was true of the static follower under colocation too, not just the candidate. The gate gains add_required_marks, and both run_follower and run_candidate now add their extra mark instead of declaring totals. Scenario 14 PASS end to end; full workspace suite, clippy, fmt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3ab64b011
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| crate::lease_agent::LeasePublisher::promote( | ||
| leader_publisher.as_ref(), | ||
| fencing_epoch, | ||
| committed_offset, | ||
| ); |
There was a problem hiding this comment.
When a re-grant is delivered while the supervisor still has Phase::Leading, a lease loss can be published after verdict_rx.borrow() but before this direct promote call; the demotion first fences the broker, then this stale promotion reactivates it until the queued Follow is processed, allowing sessions under a lease metadata has moved away. Fresh evidence beyond the earlier construction race is that this standing-leader arm bypasses CandidateLeasePublisher::complete_promotion and therefore its demotion lock/epoch ceilings; route this re-promotion through the same atomic handoff.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified real — the one promotion path still outside the atomic handoff, predating it. Fixed in 0770741: the standing-leader re-grant now routes through complete_promotion with the same demotion lock, both epoch ceilings, and the LeadingDemoteAdapter dialect as a fresh build. A refusal publishes nothing; the queued verdict decides what happens next.
…ame door The re-promotion arm was the one path still promoting directly: a re-grant delivered to a standing leader bypassed complete_promotion, so a lease lost after the verdict was read could be fenced by its demotion and then reactivated by the stale promotion until the queued Follow was processed — the same race the atomic handoff was built to close, surviving in the arm that predated it. Every promotion now takes the same door: the demotion lock, both ceilings, and the leading demotion dialect. A refusal publishes nothing, and the queued verdict decides what happens next. Scenario 14 PASS end to end; 67 vtop-node tests, clippy, fmt.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0770741040
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // writing here under an older one. Dialling its own port to be told so | ||
| // would add a failure mode without adding a fact. | ||
| let (local_committed, _) = self.broker.local_offsets(); | ||
| let local_committed = self.view.local_committed_offset(); |
There was a problem hiding this comment.
When a following candidate acquires a new lease while the former leader can still reach its replica plane, its local follower view remains active at the former epoch because the new grant is not forwarded until after this probe. The former leader can therefore append after local_committed_offset() is sampled and before the supervisor later suspends the follower; unlike every remote probe, this local vote is not taken from a stopped log. Promotion then reopens a directory containing a tail newer than the boundary it established, allowing uncommitted or stale-leader records to be carried into subsequent replication. Fence/suspend the local follower at the granted epoch before reading and counting its offset.
Useful? React with 👍 / 👎.
1 participant
Part of #284 — option (2) from the issue, the shape its own text says "is where this should end up": the role follows the lease inside the binary. Design was posted to the issue before any code. Does not close the issue: the chart slice (symmetric rendering, retiring
leaderOrdinal, and the k8s-smoke pod-delete assertion) follows on top of this.What this is
role: candidate+peers:— one config, identical on every member (self filtered bynode_uuid). Both planes bind once and never move:BrokerSlotread per session accept: sessions in flight keep the broker they started with (which fails closed the moment it is fenced), an empty slot — a candidate not leading — refuses the socket with its own counter (sessions_refused_no_broker), because "we are full" and "we are not the leader" point an operator at opposite remedies;InProcessFollowerwhile following, the leader's status-and-transfer surface while leading, refusing placeholders mid-transition.The lease agent runs for the life of the process. Its publisher is a verdict recorder: promotion cannot take effect in the publisher — the leader it promotes does not exist until the supervisor builds it — so
promoterecords and the supervisor completes it (replica set from peers minus self, broker over the reopened range, then the proven boundary replayed). Demotion and suspension forward immediately, because fail-closed has no build step. Transitions run in #280 order and re-open the range from disk after quiescing: the directory is the handoff, exactly as it is between processes, minus the processes. The §5.4.1 election restriction and stand-aside (#342) are what make N candidates safe; they were built first for exactly this reason.Scenario 14 — the composition proof, and the three traps it found
PASS, live: three identical configs; an unscripted election; 600 records quorum-acked at the winner's own address; SIGKILL mid-hold; a survivor takes the range at the next epoch with no process started, no config rewritten, and no port moved; produce resumes on the survivor's own address; every pre-kill acknowledged record intact. Scenario 09 still passes — existing roles untouched.
Each first-run failure was a real finding (the suite's doctrine, again):
lease agent runningat start of life ("an agent that never logs is indistinguishable from an agent that never ran").promote("serve this epoch"); the agent reports a rival's grant asdemote. For a following candidate every demote is that observation, so aFollowerObservationAdaptertranslates it back — without it, three healthy replicas gave zero acks.await_lease_holder_changed), and the log assertions are deadline-polled (await_log_line), the live-chaos: scenario 12's one-shot get-placement fails on a momentary ReadIndex quorum lapse #326 lesson relearned live.Deliberate scope choices
node_info{role="data-candidate"}set once; the live verdict stays in the lease gauges, never a mutating label.Verification
Protocol untouched. 17 suites green across vtop-node/vtop-broker (including the new pins: verdict-recorder ordering, mid-transition refusal); clippy
-D warnings, fmt. Scenario 14 PASS live; scenario 09 regression PASS live. Docs: scenario table + suite counts updated (15 scenarios).Summary by cubic
Enables candidate mode so
vtop-nodebinds native and replica listeners once and the role follows the lease for restart-free failover with no port moves. Readiness and leadership are hardened; transitions now drain via view suspension to prevent post-swap writes. Part of #284.New Features
vtop-node:role: candidatewith symmetricpeers(includes self); followers = peers minus self. Following ⇒ ready; leading ⇒ ready only with a live lease view at the held epoch.vtop-broker:BrokerSlotbehind the native listener; slot read per accept; empty slot refuses withsessions_refused_no_broker(exported viaServerCollector).Bug Fixes
ReadinessGate::add_required_marksadded;run_followerandrun_candidateadd their extra mark instead of overwriting totals, fixing colocation gates.Written for commit 0770741. Summary will update on new commits.