Repository navigation
coordinator: evacuation retains the source binding until teardown is confirmed - #896
Conversation
…confirmed A drain evict acknowledged its source-host destroy, detached the binding in the flip transaction, and finished while the host's teardown effect was still deferred — so the evac resumer saw an unowned Evacuating row and restored a second sandbox alongside the still-live source (ADR 0090 single-ownership violation, nightly seed 33058131). Keep the outgoing sandbox bound through Evacuating; the resumer re-issues the idempotent destroy, independently probes the source, and only a confirmed-absent probe authorizes the fenced binding clear before the peer restore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ engrams review — complete. 1 finding posted. · View details |
…hip-evac # Conflicts: # crates/engram-dst/tests/regression_seeds.rs
Review finding on #896: confirm_source_teardown errors returned before both the budget check and the bump, so a source that stays connected but can never positively confirm teardown wedged the session in Evacuating forever. Confirmation failures now count against the same budget as pipeline failures and the exhaustion check runs first, so the session reaches the existing Idle fallback (binding deliberately retained). An old host-agent's probe answering Unsupported proceeds on the confirmed destroy, matching reconcile::flip_missing's documented posture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ engrams review — complete. 1 finding posted. · View details |
Review finding on #896 (HIGH): the budget-exhaustion fallback lands Idle with the unconfirmed source binding retained, and that row was not recoverable — the guarded bind 409s on a bound row, and worse, a retry after a failed first attempt hits the crash-resume shortcut (recorded "bind" step + a live binding) and false-finishes the session Active onto the possibly-mid-teardown stale VM. resume_from_idle now runs the same confirm-teardown gate the evac resumer uses BEFORE any restore or step recording: a confirmed-gone source authorizes the fenced clear and the restore proceeds; an unconfirmable one fails 503-retryable until the teardown lands or the dead-host lane clears the binding. Ownership of an unconfirmed sandbox is never released, in exactly one seam. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ engrams review — complete. 1 finding posted. · View details |
| if let (Some(source_host), Some(source_sandbox)) = (session.host_id, session.sandbox_id) { | ||
| crate::evac_resumer::confirm_source_teardown( | ||
| state, | ||
| source_host, | ||
| source_sandbox, | ||
| ctx.fence(), | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| ApiError::Unavailable(format!( | ||
| "resume: retained source binding could not be confirmed torn down: {e}" | ||
| )) | ||
| })?; | ||
| match state | ||
| .services | ||
| .meta | ||
| .fenced_assign_sandbox(id, ctx.fence().epoch as i64, None, Some(source_host)) | ||
| .await | ||
| { | ||
| Ok(true) => { | ||
| state.host_registry.invalidate_sandbox(source_sandbox); | ||
| session.sandbox_id = None; | ||
| } | ||
| Ok(false) => { | ||
| crate::metrics::note_fenced_write(); | ||
| return Err(ApiError::Conflict( | ||
| "resume: fenced by a newer op while clearing the stale source binding".into(), | ||
| )); | ||
| } | ||
| Err(e) => return Err(e.into()), | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness · MEDIUM — resume_from_idle stale-binding gate skips host_id=None rows, leaving a (None, Some sandbox) session permanently un-resumable
WHAT: The new stale-binding gate is guarded by if let (Some(source_host), Some(source_sandbox)) = (session.host_id, session.sandbox_id). A session that lands Idle with host_id = None but sandbox_id = Some skips the gate entirely, so the retained sandbox_id is never cleared and every resume attempt then 409s forever — the exact "pre-gate 409-forever wedge" this PR set out to remove.
WHEN: The evac_resumer explicitly handles the (None, Some(source_sandbox)) shape (evac_resumer.rs:353-363) by burning the retry budget, with the comment "burn the budget so this (should-be-impossible) shape also reaches the exhaustion fallback instead of wedging." But the exhaustion fallback (evac_resumer.rs:264-305) calls transition_session(Idle), which touches neither host_id nor sandbox_id (engram-postgres/src/lib.rs:2274-2294). So the residue lands Idle as (host_id = None, sandbox_id = Some). On /resume:
resume_from_idle's gate requires bothhost_idandsandbox_idto beSome; withhost_id = Noneit is skipped, sosandbox_idstays set.- The restore then calls
bind_resumed_session, which usesrebind_session_guarded(..., expected_current = Some(None), ...)(snapshot.rs:2144). That guard requiressandbox_id IS NULL(engram-postgres/src/lib.rs:3300). - With
sandbox_idstill set, the guard matches 0 rows →Conflict→bind_resumed_sessiondestroys the fresh VM and returns 409 (snapshot.rs:2148-2172). Every retry repeats → the session is permanently un-resumable.
So the (None, Some) defense in the resumer only relocates the wedge from Evacuating to Idle; it does not make the row recoverable, contradicting its own comment. This is contingent on reaching the (None, Some) state, which the author documents as should-be-impossible — hence low confidence — but if it is ever reached (e.g. a future write that clears host_id while leaving sandbox_id), the session is stuck with no terminal state and no recovery path.
| if let (Some(source_host), Some(source_sandbox)) = (session.host_id, session.sandbox_id) { | |
| crate::evac_resumer::confirm_source_teardown( | |
| state, | |
| source_host, | |
| source_sandbox, | |
| ctx.fence(), | |
| ) | |
| .await | |
| .map_err(|e| { | |
| ApiError::Unavailable(format!( | |
| "resume: retained source binding could not be confirmed torn down: {e}" | |
| )) | |
| })?; | |
| match state | |
| .services | |
| .meta | |
| .fenced_assign_sandbox(id, ctx.fence().epoch as i64, None, Some(source_host)) | |
| .await | |
| { | |
| Ok(true) => { | |
| state.host_registry.invalidate_sandbox(source_sandbox); | |
| session.sandbox_id = None; | |
| } | |
| Ok(false) => { | |
| crate::metrics::note_fenced_write(); | |
| return Err(ApiError::Conflict( | |
| "resume: fenced by a newer op while clearing the stale source binding".into(), | |
| )); | |
| } | |
| Err(e) => return Err(e.into()), | |
| } | |
| } | |
| When `host_id` is `None` but `sandbox_id` is `Some`, there is no source host to double-book with, so the binding can be cleared unconditionally (it is unconfirmable by construction). Either extend the gate to a separate arm that fenced-clears `sandbox_id` for the `(None, Some)` case, or have the exhaustion fallback normalize this shape to fully-unbound before landing Idle. |
Problem
Nightly sim seed 33058131 (#883) violates
single-ownershipat step 79: a session owns two live sandboxes across the fleet. Mechanism (pinned via instrumented replay): the drain's evict pipeline issued a destroy to the source host, and the host acknowledged the verb durably but had not yet applied its teardown effect. The pipeline's Evacuating flip detached the sandbox binding in the same transaction, so the evac resumer's next claim saw an unownedEvacuatingrow and restored the session onto a peer — while the original sandbox was still live on the (crashed-and-restarted) source host. Ownership was released on an acknowledgement, not on confirmed teardown.Fix
Per ADR 0090 (coordinator truth owns the outgoing VM):
idle_evictor: the evict pipeline's fused flip now detaches the binding only for the Idle path;Evacuatingretains the source binding (detach_sandbox = target_state == Idle).evac_resumer: before restoring, a claimedEvacuatingrow with a retained binding re-issues the idempotent destroy through the source backend, then independently probes the sandbox; only a confirmed-absent probe (or NotFound) authorizes the fenced binding clear (respecting the claim epoch) ahead of the peer restore. Any unconfirmed state retains ownership and retries.Liveness was checked adversarially: a source host that dies mid-drain is handled by the existing dead-host path —
mark_host_dead_and_orphan_sessionsorphans every non-terminal status (evacuatingincluded), clearinghost_id/sandbox_id, and stage 2 routes to Idle/Dead, so the resumer'sstatus != Evacuatingre-read exits cleanly. Known residual edge (pre-existing fallback, now with a retained binding): if the source host stays connected but destroy/probe fails through the retry budget, the Idle fallback leaves a bound row — operator-visible via the existing budget-exhausted warn; strictly safer than releasing an unconfirmed sandbox. Can be hardened in a follow-up if we want the fallback to clear-or-hold explicitly.Test
crates/engram-dst/tests/regression_seeds.rs(chaos, 5000 steps — the full nightly shape). Fails pre-fix at step 79; passes post-fix at both 80 and 5000 steps — verified by independent replay.cargo nextest run -p engram-dst -p engram-coordinatorgreen.just checkgreen.Closes #883.
(#883's body also embeds a second, unrelated
quiescence-no-op-mintviolation — seed 33058255 — fixed by #895.)🤖 Generated with Claude Code