Repository navigation
feat(coord): the durable teleport state machine; TeleportSession and RetireHost plan it (ADR 0123 B) - #1591
Conversation
|
👀 engrams is reviewing 97fbc2f. |
|
The latest Buf updates on your PR. Results from workflow CI / buf (pull_request).
|
There was a problem hiding this comment.
Engrams review
Verdict: 5 findings included in this summary.
Severity: Critical 0 · High 0 · Medium 4 · Low 1
Categories: 🩺 Stability & Availability: 2 · 📐 Maintainability & Code Quality: 2 · 🗄️ Data Integrity & Integration: 1
Findings on the review page
crates/engram-coordinator/src/api/admin.rs:L406— Plain admin drain silently leaves Active sessions on the cordoned host, with no retry and a false "planned" reportWHAT:
admin_drain_host_coreonly cordons the host (viaset_host_cordon, which does not setretire_requested_at) and then fire-and-forgets per-session teleport ops throughplan_host_teleports(AdminDrain); a session that cannot be relocated is still reported inDrainHostResponse.plannedyet never actually moves, and nothing ever retries it.WHEN: Two routes reach the same stranded outcome, sharing one root cause — a cordon-only drain has no durable backstop:
- Fleet at capacity (no cancellation needed).
plan_host_teleportsenqueues a deferred teleport op for every Active session and counts each inreport.planned. When the op runs,drive_innercallssteps::admit(None, AdminDrain); if placement returnsTeleportAdmitOutcome::NoFit,drive_inner(teleport.rs:260-267) returnsOpOutcome::Done. The op completes "successfully", nosession_teleportsrow is created, the session staysActiveon the cordoned host, and the operator sees it listed underplanned— indistinguishable from a real in-flight move.- Request cancelled mid-plan.
plan_host_teleports_with_limitis awaited inline in the gRPC/HTTP handler (the old code ran this fan-out on a detached task — the deleted comment: "This guard is preserved exactly" — but the new code dropped it). If the client disconnects mid-loop, sessions after the cut-off never get an op enqueued.Neither case self-heals: unlike
RetireHost(which setsretire_requested_at, sorun_once'slist_retiring_hostssweep re-plans the host every tick), a plain drain leavesretire_requested_atNULL, solist_retiring_hosts(WHERE retire_requested_at IS NOT NULL) never re-sweeps it, andreconciledeliberately excludesEvacuating/resident Active mid-move from strike accounting. The operator believes the host is drained and removes it, killing the still-resident sessions.Trigger likelihood: routine
crates/engram-coordinator/src/teleport.rs:L228— Teleport ops have no attempt-budget ceiling, so a persistently-failing move retries forever and permanently consumes a destination slotWHAT: The
Teleportverb never bounds its retries:driveturns everyErrfromdrive_innerintoOpOutcome::Retry(teleport.rs:223-227), and the step bodies returnRetryAfterunconditionally (e.g.rollback'shost.resume/migration_abortfailure → 5 s retry at 1147-1152;release's source-destroy failure → 10 s retry at 1064-1069). The only attempts check in the whole file is the narrowctx.op.attempts <= 3on one restore-error class (535). Every sibling verb has an explicit ceiling that routes to a terminal fallback —RESUME_MAX_ATTEMPTS=60,EVICT_MAX_ATTEMPTS=20,CREATE_BOOT_MAX_ATTEMPTS=30,QUARANTINE_EVICT_MAX_ATTEMPTS=3(session_verbs.rs).WHEN: A source or destination host that is reachable enough to stay out of
Dead/Retiredbut whoseresume/migration_abort/destroyRPC keeps failing (a wedged-but-heartbeating host-agent, a stuck sandbox) drives the row intoRollingBack(or leaves it atAttached) and keeps failing:
- The session is stuck in
Evacuatingindefinitely — the user cannot use it — becauserollbackcan only finish by first resuming the source back toActive, which keeps failing.- The
session_teleportsrow stays in a non-terminal phase, andteleport_admit's cap query counts everyphase NOT IN ('done','aborted','failed')row againstmax_open_per_dest(default 1). So a single stuck row permanently consumes the destination host's teleport-receive slot, silently blocking all future teleports to that host, with no metric or terminal state to signal it.
source_gone()only rescues the row if the source host independently transitions toDead/Retired; a host that merely misbehaves never triggers that.Trigger likelihood: plausible-fault
web/src/test-utils.tsx:L74— adminDrainHost test mock still returns the old (removed) AdminDrainHostResponse shapeWHAT: The regenerated
AdminDrainHostResponsedroppedevacuating/failuresand now hasplanned/descended/skipped(fleet.proto +web/src/gen/.../fleet_pb.ts), but theadminDrainHostmock intestTransportstill returns{ hostId: "", evacuating: [], failures: [] }. The siblingevacuateSession → teleportSessionmock on the next line was migrated correctly; this one was missed.WHEN: The mock is an object literal returned from a typed connect-es (
@connectrpc/connectv2 /@bufbuild/protobufv2) service implementation, soevacuating/failuresare excess properties not on the new message init shape andplanned/descended/skippedare absent. The most likely outcome is atsc -bexcess-property error that failspnpm buildin the web CI lane; if TS does not flag it (e.g. a looser inferred return type), it is instead a silently-wrong fixture that any future test reading.planned/.descended/.skippedwill getundefinedfrom. Either way the response contract is now duplicated between the proto and this stub with only one side updated.Trigger likelihood: routine
deploy/migrations/0122_teleport_machine.sql:L5— Sessions mid-evacuation under the old scanner are permanently stranded in Evacuating after this deployWHAT: This PR removes the only driver for
status='evacuating'sessions (deletesevac_resumer.rs, and thelist_evacuating_sessions/bump_evac_attemptstrait methods, and dropsevac_attempts+idx_sessions_evacuating), replacing it with a scanner that drives relocation solely fromsession_teleportsrows (run_once→list_open_teleports). Any session already sitting inEvacuatingat deploy time with no matchingsession_teleportsrow has nothing left to advance or fail it.WHEN: At the base revision, evacuations were driven by
evac_resumer, which sweptsessions WHERE status='evacuating'directly and had nosession_teleportsrow for that session. If the new binary takes over while a session is mid-evacuation (a rolling coordinator deploy overlapping a host drain / dead-host relocation — both routine operations), that session is leftEvacuatingforever:
- the new
run_onceonly enqueues sessions it finds vialist_open_teleports, and this one has no open teleport row;dead_hostno longer routes intoEvacuatingand does not recover sessions already in it;reconciledeliberately excludesEvacuatingfrom missing-sandbox strike accounting ("mid-move"), so it is never flipped toHostLost.The session is stuck and unusable until manual intervention; the dropped
evac_attemptscolumn means the old logic cannot even be resurrected. I found no pre-deploy drain gate or boot reconciler for orphanedEvacuatingrows in the diff; if the team enforces "zero evacuating sessions before applying 0122" operationally, that is the mitigation, but it is not visible in the change.Trigger likelihood: plausible-fault
crates/engram-coordinator/src/teleport.rs:L536— Bundled low: misleading failure label and stale comments left by the evac→teleport renameWHAT: A bundle of trail/comment items from the refactor, each of which misleads a future reader or operator (grouped per the one-low-finding rule):
teleport.rs:535-536— after the restore-retry budget,finish_live_restorediscards the real error and records the fixed literal"dest_lost_after_blackout"regardless of the actual cause (OOM, corrupt manifest, backend bug). This label is then surfaced inSessionDiagnosticsand theteleport_finished.errorfield, so an operator debugging a failed teleport reads a specific-but-false root cause. The oldlive_migration.rspreserved the real error text end to end.session_verbs.rs:31— theCheckpointFinalize/Teleportcomment still points recovery at "the ADR 0018 parachute / evac scanner for a torn teleport"; that scanner (evac_resumer) is deleted in this PR.crates/engram-host-agent/src/pooled_backend.rs:812— the doc block describinginflight_snapshots("Cleared bycommit_snapshot/abort_snapshot; … orphan dir leak …") now sits directly above the newly-insertedheld_swap_policyfield, so it attaches to and mis-describesheld_swap_policy, whileinflight_snapshotsis left undocumented.crates/engram-protocol/proto/engram/app/v1/fleet.proto:45— theTeleportSessionRPC still carries the comment// ADR 0018 async evacuation (POST /api/admin/sessions/:id/evacuate).; that HTTP route andevacuate_session_corewere deleted (the CLI now calls the gRPC RPC directly).WHEN: The next operator or maintainer trusts a label/comment that no longer matches the code — misdiagnosing a teleport failure (1), looking for a deleted recovery path (2), reasoning about the wrong field's lifecycle (3), or hitting a removed route (4).
Trigger likelihood: routine
…ttles orphaned Evacuating rows Review findings on #1591: - `AdminDrainHost` was a cordon plus a one-shot plan: a resident that did not fit was reported as planned and never retried. It is now the retirement request with `owner = admin`, shared with `RetireHost` through `request_host_retirement_core`; the scanner re-plans it every tick and grants it when the host is empty. `UncordonHost{admin}` cancels the request before the grant. - Migration 0122 settles sessions the retired evacuation scanner left in `evacuating` with no `session_teleports` row: tombstone the bound sandbox, then Idle with a recoverable snapshot and Dead without one. - The restore-budget failure keeps the real error behind the `dest_lost_after_blackout` label. - Stale comments: the Teleport verb's recovery note, the `inflight_snapshots` doc block, the `TeleportSession` proto comment (generated code follows). - The web test mock returns the current `AdminDrainHostResponse` shape. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01965DMBwLXzE9baCmj1Wp8Q
|
Addressed in b937d23 and 9baba34:
|
…RetireHost plan it (ADR 0123 B) Rebuilt onto the rebuilt readiness branch after the squash merges of #1583-#1586; content unchanged. Includes the review fixes for #1591. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01965DMBwLXzE9baCmj1Wp8Q
72069fa to
dd69420
Compare
|
Adversarial review round (my pass + three Codex Astra passes) landed in dd69420, on top of the branch rebuilt onto the squashed main:
ADR 0123's divergence log carries the full list. Local gate: |
dd69420 to
eb37d82
Compare
…RetireHost plan it (ADR 0123 B) Rebuilt onto main after the squash merge of #1590; content unchanged. Includes the review fixes (fenced teleport writes, one-transaction settlement, source reservation, move-owned sandboxes, held-source role). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01965DMBwLXzE9baCmj1Wp8Q
eb37d82 to
97fbc2f
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
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. Claude-Session: https://claude.ai/code/session_01965DMBwLXzE9baCmj1Wp8Q Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Problem
Issue #1549's control-plane half (ADR 0123 findings 4, 7, 8, 9, 10, and the UI teleport). Three modules each owned part of a move with unfenced writes, in-memory phase state, detached tasks and a give-up path to Idle:
evacuation.rswrote host, sandbox and state in three separate statements after the restore;evac_resumer.rsburned a 20-attempt budget confirming a source destroy on a host that no longer existed and dropped the session to Idle;live_migration.rsran a synchronous verb with a detached finalize, a process-local host gate, a rollback that declared Active while the source stayed paused, and an illegalEvacuating → Failededge. Destination capacity was checked, never reserved. The UI'sEvacuateSessionignored its target, never attempted a live move, and hid errors behind an optimistic status flip.Change
One durable machine.
session_teleports(C1) is the journal and the reservation;OpKind::Teleportis driven by the existingsession_opsexecutor (claim, fence, heartbeat, reclaim);teleport.rsis the one driver for every entry point. Phasesadmitted → captured → restored → committed → attached → done, withrolling_back → abortedandfailed; every step ends in one fenced CAS and a successor resumes from the row.snapshot_holdon the host keeps the VM paused after capture and does not re-arm swap;resumere-arms once;destroyclears the hold. Without this the source kept emitting after the capture point and its events collided with the destination's replays under the same epoch and seq. A live move persists its full presetup result (live_payload) in the same CAS asexport_id, so a successor never calls presetup twice.binding_epoch + 1, phase. No tombstone (a live source is still the page server).Evacuating → Active. A deterministic spawn failure rests at Created and the move still releases the source.migration_abortand a held snapshot throughresume, and staysrolling_backuntil the source answers.Entry points.
RetireHostplans teleports for its residents before evaluating the grant (the C1 TODO).AdminDrainHostcordons as admin and plans without awaiting moves.TeleportSession{session_id, target_host?}replacesEvacuateSession: synchronous admission under the op claim, honors the target under the full 2D, cordon and capability checks, returns{teleport_id, kind, dest_host_id}, FAILED_PRECONDITION with the reason otherwise.teleport_finishedis a session event; the web shows it instead of flipping status optimistically.Deleted.
evacuation.rs,evac_resumer.rs,live_migration.rs, the evacuation types, the attempt budget, the teleport pin columns (migration 0122),MIGRATION_GATE,walk_back_to_active,parachute_or_kill, the drain JoinSet and placement preview, the unlocked reserved pick, the Evacuating target of the evict pipeline,ENGRAM_LIVE_TELEPORT.Test
Conformance (sim + Postgres): admission reserves under lock (NoFit, SessionNotActive, Fenced, Conflict, cordoned dest, per-dest cap, pinned no-fit), phase CAS fenced and legal with payload preservation, one-statement commit (no tombstone, reservation moves), release entombs the source once, rollback keeps the dest reserved, dead-host orphan skips machine-owned sources, fail/abort fenced. Scripted scenarios on sim and live Postgres: end to end, crash at every phase resumed by a successor, presetup payload reuse, rollback pending until the source answers, live rollback through
migration_abort, lost live export rolls back, live refusal downgrades, source dead at release, fenced step stops, no-fit stays Active, two per destination, retire grants only after an empty heartbeat then DeleteHost. Unit: attach waits for the generation, attach failure rests at Created and still releases,snapshot_holdstays paused without re-arm, resume re-arms once. DST drives the scanner with capacity and quiescence oracles. e2e (KVM lane):e2e_teleport_session_honors_target_host,e2e_retire_host_relocates_and_grants.Local: clippy on 11 crates, 1320 unit tests, 72 PG-conformance, 160 live-PG (including the new
teleport_live_pg),just check2679, Linux cross clippy, web tsc/tests/build, orchestrator and CLI typecheck. KVM e2e runs in CI.Stacked on #1590 (C3). Host follow-up noted in the ADR: make the export lifetime teleport-driven instead of TTL-driven.
🤖 Generated with Claude Code