Repository navigation
refactor(metadata): retire assign_session_sandbox (ADR 0123 C4) - #1592
Conversation
|
👀 engrams is reviewing 46a04ef. |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01965DMBwLXzE9baCmj1Wp8Q
There was a problem hiding this comment.
Engrams review
Verdict: 1 finding included in this summary.
Severity: Critical 0 · High 1 · Medium 0 · Low 0
Categories: 🗄️ Data Integrity & Integration: 1
Findings on the review page
deploy/migrations/0122_teleport_machine.sql:L42— Migration 0122 settle ignores and discards live_disk_manifest, marking disk-recoverable sessions DeadWHAT: The orphaned-
evacuatingsettle in migration 0122 decides Idle vs. Dead from a recoverable memory snapshot alone and then NULLslive_disk_manifest_id/version/at— but the rest of the system treats a live disk manifest as an independent, valid recovery basis, so this both mis-classifies disk-only-recoverable sessions as Dead and strips the cold-boot fallback thatresumerelies on.The settle mirrors the teleport "lost source" path per its comment, but diverges from it in two ways that cost recoverable sessions:
Wrong Idle/Dead decision (lines 28-32). The
CASEroutes to'dead'whenever no recoverable snapshot exists, even if the session has a live disk manifest. The system's honest-recoverability predicate is explicit that this is wrong:dead_host::recovery_target(dead_host.rs:504-512) returnsIdleiffhas_recoverable_snapshot || has_live_manifest— "the latter still resumes via the cold-boot path" — and routing such a session to Dead is exactly the "Dead that lies about resumability" the #777 / ADR 0098 Phase-3 "honest-Dead" work forbids. An Active session being live-migrated normally carries an ADR-0028 continuous-synclive_disk_manifestand may have no recent recoverable memory snapshot, so this hits the migration's own target population.Destroys the cold-boot fallback (lines 34-36). The code path this migration claims parity with (
teleport::steps::settle_lost_source/fail_move, teleport.rs:1081, 571) settles viafenced_transition_sessionwithBindingDisposition::Detach, whose UPDATE nulls onlysandbox_id(postgres/src/lib.rs:3024-3045) and PRESERVESlive_disk_manifest_*. The migration additionally nulls the manifest.resume_from_idlewalks snapshots newest-first and, when no snapshot artifacts are present, falls back toresume_disk_only_cold_bootviasession.live_disk_manifest(snapshot.rs:1066-1108). Nulling the manifest removes that last-resort rung even for sessions settled to Idle.Net effect: a session that the dead-host/resume machinery would recover is instead left permanently unrecoverable (user must fork), with no manifest record remaining.
WHEN:
- Deploy applies migration 0122 while sessions are mid-move under the old evac scanner. A targeted session has a live disk manifest but no recoverable memory snapshot → the migration marks it
deadand nulls its manifest → a disk-only-recoverable session is permanently lost (routine for live-migrating sessions at upgrade).- A targeted session has a recoverable snapshot (→
idle) plus a live manifest; the migration nulls the manifest; later the snapshot's artifacts are found missing at/resume(the 89f7984d class the resume chain defends against) → resume cannot fall back to disk-only cold boot and flips the session Dead instead of recovering it.Trigger likelihood: routine
a1294b8 to
5c2762c
Compare
|
Addressed before this head: the 0122 settle now uses the dead-host predicate (Idle when a recoverable memory snapshot OR a live disk manifest exists, Dead only with nothing recoverable) and keeps |
5c2762c to
254b997
Compare
254b997 to
c977109
Compare
Rebuilt onto main after the squash merge of #1591; content unchanged. Remove the blind sandbox setter from MetadataStore, both stores, and all mocks. Use the surviving fenced binding writes in test fixtures. Make reconcile compare the exact current binding, including None. Keep assign_session_host for dead-host cleanup. Preserve strike reset and live-manifest cleanup in fenced assignment; extend conformance coverage. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01965DMBwLXzE9baCmj1Wp8Q
c977109 to
46a04ef
Compare
Summary
ADR 0123 C4. Retire the blind sandbox setter
assign_session_sandboxfromMetadataStore.transition_session_created,fenced_assign_sandbox,assign_session_sandbox_guarded,rebind_session_guarded,teleport_commit.fenced_assign_sandboxnow carries the cleanup the blind setter owned: strike reset on bind and unbind, live-manifest clear andchunk_generationbump on unbind, in one statement (PG) and one lock (sim).expected_current = Some(None)), so a binding that landed between the strike decision and the clear is never wiped.unimplemented!instead of composing a read with the blind setter; both stores implement them.assign_session_hoststays fordead_host.rsonly, and says so.fenced_assign_sandbox(.., 0, ..); the binding-writer inventory drops the retired rows.Conformance (ADR 0098 D4)
binding_epoch_minted_by_binding_writesextended: the absent-binding CAS rejects a newly bound sandbox.fenced_binding_preserves_cleanup_and_strike_reset: a rejected clear keeps the pin and the streak; a bind resets strikes and mints one epoch; a clear removes the pin, bumps the generation, mints nothing.Validation
cargo fmt --check,cargo clippy -D warningsengram-simconformance against sim and live PG: 160 passedjust checkgreenStacked on #1591.
🤖 Generated with Claude Code