Repository navigation
Host-durable eviction finalize: commit the snapshot from on-disk artifacts, never from a live sandbox - #558
Conversation
ADR 0028 issue #529: `rewind_session_to_cursor` tombstoned every event kind past the cursor, including the coordinator's own eviction/resume lifecycle facts (`evicted`, `status_changed`, `snapshot_taken`, `resumed`, `recovered_from_checkpoint`). Prod evidence: 45/45 sampled resumes had rolled_back > 0 (median 4) even on perfectly clean evict->resume cycles, because those four lifecycle events always trail the cursor and got tombstoned as if they were guest memory. Exclude those coordinator-fact kinds from the tombstone UPDATE — they stay true regardless of what the guest remembers. Guest-derived kinds (run_*, agent_message*, tool_call_*, exec_*, prompt_*, etc.) still rewind on a genuine mid-run crash recovery. Adds a live-Postgres regression test asserting a lifecycle-only span rewinds nothing (no epoch bump) while a guest event in the same span still rolls back.
ADR 0028 / issue #529 step 2: add `paused_at: Option<DateTime<Utc>>` to `SnapshotMetadata`, stamped unconditionally in `SnapshotFinisher::finish` from `SnapshotCapture::paused_at` (the true pause instant, captured before the multi-second post-phase upload runs). Bumps WIRE_VERSION 7 -> 8 (bincode is positional; a trailing field still needs the lockstep coord+host version gate per existing discipline) and regenerates the `snapshot_metadata` wire golden. The composed eviction path (`idle_evictor.rs`) now resolves the `session_events` coherence cursor from `metadata.paused_at` when present, instead of coordinator wall-clock `now` sampled after the capture/upload completes — closing the skew window that (combined with the unscoped rewind fixed in the previous commit) made a clean evict->resume look like a rewind. Falls back to `now` when absent (mixed-version roll; VZ/Process backends run unwrapped by PooledBackend and don't set it). Every other `SnapshotMetadata` literal in the tree is either a raw (pre-PooledBackend-wrap) backend producer, which sets `paused_at: None` and lets the finisher/composed-path fallback cover it, or a restore-side reconstruction (session_boot, live_migration, evacuation, resume), where the field has no meaning and is `None`.
…; SnapshotTaken moves to the heartbeat reconcile Issue #529 step 7 (plumbing the D5 rewrite needs): `MetadataStore::record_snapshot` now returns `inserted: bool` (`RETURNING (xmax = 0)` on the Postgres upsert) instead of `()`, so a caller can tell an INSERT from a re-record UPDATE. Every MetadataStore implementor (Postgres, MiniMeta, and the per-test mocks across engram-coordinator/engram-chunk-store/engram-oci-auth) is updated honestly — no laundering the return type. Adds `CheckpointKind { Periodic, EvictionFinal }` (engram-protocol, serde-JSON additive — no WIRE_VERSION bump needed) to `CheckpointAdvert` and the host-agent's `CheckpointRecord`, defaulting to `Periodic` for mixed-roll compatibility. The heartbeat reconcile (`host_http.rs::heartbeat`) now emits `SessionEvent::SnapshotTaken` itself, iff `record_snapshot` returns `inserted && kind == EvictionFinal` — the first landing of an eviction's terminal row, regardless of which coord (if any) is up when it lands. This is prep for the next commit, which deletes the coordinator-RAM D5 finalize task that currently owns this emit; a periodic-checkpoint reconcile (kind stays `Periodic`) does not emit it. MiniMeta also gains a `get_snapshot` override (previously the trait's default `Ok(None)`) so the coordinator's forthcoming row-watcher tests can observe a row landing.
… D5, issue #529) Root cause: the D5 eviction finalize's durability depended on the conjunction of three ephemeral things -- a coordinator-RAM tokio task, a host-RAM tokio task + snapshot_waits slot, and live RPC routing to a sandbox PG already says nobody owns. Any of the three dying mid-upload (host-agent pod roll, coordinator restart, the host's own teardown reaper racing its unbind) silently dropped the snapshot; resume then fell back to a stale periodic checkpoint. Prod evidence: ~24% of idle evictions (25/104, 14d window) hit this. New invariant: once `PooledBackend::snapshot_begin` returns, the finalize is a HOST-OWNED job that is a pure function of durable on-disk artifacts -- it never touches the sandbox, the coordinator, or any in-RAM map again. A host-agent process restart re-drives it (`resume_pending_finalizes`, called at startup); nothing but node loss can lose it. - `crates/engram-host-agent/src/eviction_finalize.rs` (new): the `EvictionFinalizeRecord` (sibling of `CheckpointRecord`, persisted to `<checkpoint_dir>/finalize/<snapshot_id>.json` write+fsync+rename BEFORE `snapshot_begin` returns) and the stage-explicit legs (Captured -> DiskUploaded -> MemoryChunked -> BlobsUploaded -> terminal), each idempotent and persisted before the next runs. Terminal writes the ordinary `CheckpointRecord { kind: EvictionFinal }` (re-advertised on every heartbeat until the coord acks it -- the ONLY place the row now lands for this flavor) and best-effort destroys the sandbox. Failures retry with backoff (30s*attempt, capped 5min) up to `ENGRAM_EVICTION_FINALIZE_MAX_ATTEMPTS` (default 10), then quarantine to `finalize/failed/` -- never silent, resume falls back to the prior periodic checkpoint (the honest, checkpoint-interval-bounded floor; no "-> 0" loss claim). - `snapshot_begin` (pooled_backend.rs) rewritten: gate on `supports_diff_checkpoints() + checkpoint_dir` (unchanged InvalidSpec fallthrough to the composed path for VZ/Process/disabled hosts) -> idempotent-return a still-pending snapshot_id -> capture_phase -> durably persist the drained NBD disk-flush chunks (if any) to `dest/disk-pending/` -> persist the `EvictionFinalizeRecord` -> spawn the finalize job -> return. No longer inserts into `snapshot_waits` (that map now serves only `migration_finish_restore`) or aborts a superseded wait -- there is nothing left to supersede. - Deviation from the issue's sketch (documented in the module doc): always reconstructs the disk manifest from the persisted `disk-pending/` bytes rather than bifurcating into a "live path" that reuses `ChunkedDiskBackend::flush_upload`'s live-state rebase. The eviction flavor destroys the sandbox immediately after finalize, so nothing ever reads that rebase again -- collapsing to one code path trades a redundant O(dirty-set) NVMe write+read for meaningfully lower risk (no live backend handle / flush-pipeline guard pinned across a backgrounded, potentially long upload). - `PooledBackend::destroy()`'s self-destruction race (found during verification, not in the issue's source docs list of prior fixes) is closed as a side effect: the eviction flavor no longer touches `snapshot_waits` at all, so `destroy()`'s `wait.abort.abort()` can never cancel an eviction finalize job again. - `PooledBackend` gains a weak self-reference (`set_self_ref`, wired in `lib.rs` right after `Arc::new(p)`) so the detached finalize job can reach the FULL `destroy()` (egress unregister, NBD slot release, checkpoint-chain teardown) at its terminal stage without threading an owned `Arc<PooledBackend>` through `snapshot_begin`'s `&self`. - `resume_pending_finalizes` (called from `lib.rs` alongside the periodic checkpoint driver spawn) re-drives every un-acked record at startup -- the crash-recovery half. - New metrics: `engram_eviction_finalize_{persisted,completed, redriven,quarantined}_total`, `engram_eviction_finalize_stage_seconds{stage}`. - Unit tests (pooled_backend.rs): snapshot_begin persists the record + is idempotent under a pending finalize before returning; the happy path produces a durable `CheckpointRecord{kind:EvictionFinal}` and best-effort destroys the sandbox; `resume_pending_finalizes` redrives a hand-crafted record with the sandbox entirely absent; a terminally-failing finalize quarantines after ENGRAM_EVICTION_FINALIZE_MAX_ATTEMPTS and never fabricates a checkpoint row. Cross-checked clean on aarch64-unknown-linux-musl (the `#[cfg(target_os = "linux")]` disk-pending path is invisible to macOS clippy). - Also fixes 3 `SnapshotMetadata` literals in NBD integration tests (nbd_restore_cancel.rs, nbd_shutdown_abandon_race.rs, nbd_shutdown_final_flush.rs) that the earlier `paused_at`-field commit's macOS-only sweep missed -- they're Linux-only compiled and only surfaced under the cross-target clippy check.
…serialization Issue #529: the coordinator-RAM D5 finalize task (idle_evictor.rs, finish_eviction_background) is deleted -- the 60s lease-touch loop, the snapshot_wait await + upload-failed abort, the finalize-side record_snapshot/commit_snapshot/destroy, and the SnapshotTaken emit. The host's finalize job (previous commit) now owns all of that; the row lands exclusively via the heartbeat reconcile. What remains is a ~40-line row-watcher: hold the SessionLeaseGuard (the thing that serializes a resume against an in-flight finalize -- unchanged, a resume during upload still 409s) via `spawn_heartbeat` (the same reap-avoidance helper the resume pipeline already uses, reused here instead of hand-rolled), poll `get_snapshot(snapshot_id)` every `ENGRAM_EVICT_FINALIZE_POLL_MS` (default 2000) until the row exists, release at `ENGRAM_EVICT_FINALIZE_WAIT_SECS` (default 900) if it never does. A coordinator death mid-watch degrades gracefully: the 180s lease reaper frees resume regardless, and the row lands via heartbeat whenever the host lands it -- bounded staleness, never loss. Also deletes the abort+destroy on a failed `transition_session` in the D5 arm: the host's finalize artifacts are already durable by the time this function runs (`snapshot_begin` already returned), so there is nothing to abort -- the eviction scanner's retry hits the idempotent `snapshot_begin` and re-observes the same pending job. The composed path's abort arms (VZ/Process/InvalidSpec fallback, pre-durability failures) are untouched -- full `commit_snapshot`/`abort_snapshot` trait retirement is epic-transactional-snapshots-revive's scope, not this one's. New metric: `engram_eviction_finalize_row_wait_timeout_total` (the row-watcher hit its deadline -- not itself loss, but a signal finalize jobs are running pathologically long). Rewrites the two D5 coordinator tests to match: `D5SplitBackend` no longer implements `snapshot_wait` (the coordinator never calls it on this path) and simulates the host's heartbeat-reconcile row landing by calling `record_snapshot` directly -- the row-watcher must observe it and release without the coordinator ever calling commit_snapshot/destroy. A new test covers the deadline-release path.
…ize redrive Issue #529 Testing & CI: `crates/engram-host-agent/tests/eviction_finalize_redrive.rs`, #[ignore]'d, Linux+KVM-only, same gating as checkpoint_chain.rs. Boots a real Firecracker guest, plants a marker, and calls `snapshot_begin` against a chunk store whose blob PUTs are gated — freezing the spawned finalize job mid-leg (after the disk leg) to stand in for a host-agent process dying between the durable persist and the upload finishing. A second, entirely independent `PooledBackend` + `FirecrackerBackend` generation (disjoint capture locks, pending_finalizes, no in-memory knowledge of the first generation's sandbox) then runs the real host-agent restart sequence against the SAME work_dir: `live_attach::reattach_pass` (ADR 0044 K2) to rejoin the still-live VM, then `resume_pending_finalizes` to re-drive the finalize purely from the persisted `EvictionFinalizeRecord` + on-disk staging dir. Asserts the durable `CheckpointRecord { kind: EvictionFinal }` lands, and restores from it on a third backend to prove the marker survives byte-identical — the acceptance criterion. The first generation's frozen task is deliberately never released (a truly dead process never resumes), so generation B's redrive is the only path to completion, not a race against a still-running rival. Sized to the property, not to realism: one guest, one 1 MiB marker, one capture; no long sleeps (a `Notify` gate, not a timeout). Wired into `.github/workflows/ci.yml`'s firecracker lane, in the same `cargo nextest` batch as `checkpoint_chain` (same gating, gets it for free) -- per repo convention, an FC/NBD regression test not in that `--test` list never runs in CI. Validated with `yaml.safe_load` + actionlint (pre-existing blacksmith-label warnings only, none on the touched lines). Compiles clean under both native macOS (excluded via `#![cfg(target_os = "linux")]`, so invisible there) and the aarch64-unknown-linux-musl cross-check -- cannot be executed in this environment (no KVM); relies on the dev-vm / CI KVM runner for actual execution, same as every sibling test in this batch.
Issue #529 Testing & CI: extends (never deletes) the sole evict->resume data-preservation e2e coverage, `e2e_resume_preserves_disk_and_memory`, with the assertion the whole rewind half of this issue exists to make true: a clean evict->resume cycle emits NO `recovered_from_checkpoint` event. Pre-#529, `rewind_session_to_cursor` tombstoned every event kind past the cursor -- including the coordinator's own `evicted`/ `status_changed`/`snapshot_taken` facts this exact cycle appends -- so `apply_rung1_rewind` always saw `rolled_back > 0` and emitted the event even on a perfectly clean cycle (prod evidence: 45/45 sampled resumes). Also sanity-checks the cycle actually ran (asserts `evicted` and `snapshot_taken` ARE present) so the no-`recovered_from_checkpoint` assertion isn't vacuously true on a no-op. Adds a `Driver::list_events` helper over `SessionService.ListSessionEvents` (ADR 0060's unary, paginated, unfiltered event-log read) -- unlike the existing `StreamEvents`-based helper (`wait_for_anthropic_auth_failure`), a unary read needs no live-tail race, matching this repo's own doc comment on the RPC ("far more testable than racing the StreamEvents tail"). Deviation from the issue's Testing & CI text, documented (not silent): the OTHER item it asks for -- "a host-agent container restart mid- eviction still yields a row" -- is not implemented as an e2e test. The e2e stack's only evict path is `SessionService.EvictLocal` (`evict_local_core`, api/snapshot.rs), which requires a snapshot to ALREADY exist and just detaches+destroys the local sandbox -- it does NOT exercise the D5 async finalize pipeline (`idle_evictor.rs`) this issue rewrites at all. There is no RPC that triggers the scanner-driven idle-eviction path from app-gRPC (by design -- ADR 0051 deliberately dropped the old `evict-idle` admin trigger with no analog), so a host-agent-restart-mid-D5-finalize property cannot be exercised through this surface without inventing new out-of-scope infrastructure. That property is instead covered at the correct altitude by the new FC integration test (`crates/engram-host-agent/tests/eviction_finalize_redrive.rs`, previous commit), which drives `snapshot_begin`/`resume_pending_finalizes` directly.
Accepted (authored alongside the implementation per the ADR-bookend norm, not phased Proposed-then-Accepted since this landed as one PR). Extends ADR 0028 (periodic checkpoint durability -- the CheckpointRecord / heartbeat-advert / PG-reconcile pattern this issue reuses wholesale) and ADR 0045 Phase D5 (the begin/wait split this issue rewrites). Records the design, the documented deviation from the issue's disk-leg sketch, and the verification evidence (unit/live-PG/FC-integration/e2e tests + the confirmed hostPath durability of work_dir).
|
The latest Buf updates on your PR. Results from workflow CI / buf (pull_request).
|
nikhilunni
left a comment
There was a problem hiding this comment.
Overview. This is a faithful, high-quality implementation of issue #529 at the macro level: durable EvictionFinalizeRecord persisted before snapshot_begin returns, a stage-legged host-owned finalize job with startup redrive, the coordinator D5 task reduced to a lease-holding row-watcher, SnapshotTaken moved to the heartbeat reconcile via record_snapshot's new inserted flag, WIRE_VERSION 7→8 with regenerated goldens, kind-scoped rewind, and a well-constructed FC integration test wired into ci.yml. However, the core crash-idempotency claim of the stage legs is not actually delivered: two legs delete their durable inputs before persisting the stage bump, and the memory leg's put_manifest has no VersionConflict handling on redrive — so exactly the crash windows this PR exists to close can silently produce a memory-less snapshot (finding 1) or quarantine a fully-uploaded one (findings 2–3). The issue's Testing item (c) — crash-injection between each stage — would have caught all three; it was silently dropped. Verdict: needs rework on the leg ordering (persist-then-delete) + the memory-manifest conflict arm; everything else is solid. CI is also red on a trivial compile error (below).
CI
All 4 failing checks share one root cause — a PR compile bug, not a stale branch or infra flake:
SnapshotMetadata gained the paused_at field, but two pre-existing FC integration test files on this branch were not updated:
crates/engram-sandbox-firecracker/tests/snapshot_uffd.rs:169and:393—error[E0063]: missing field paused_atcrates/engram-sandbox-firecracker/tests/substrate_uffd_base.rs:172— same
That breaks fmt + clippy + check (clippy/check --all-targets), tests (linux), and tests (firecracker, Linux + KVM); CI Gate fails via needs:. It's invisible to the green macOS lanes (Linux-only test targets — the classic macOS-clippy blind spot). Fix: add paused_at: None (or the capture-appropriate value) to the three initializers; diff_snapshot.rs is fine (it uses ..lineage.clone() struct-update).
Issue compliance
Substantially compliant, with the PR body's declared deviations (single reconstruction disk leg, FC-test-instead-of-e2e for mid-eviction restart, live-PG test relocation) defensible and argued. Implemented per spec and verified against the branch: record persist before snapshot_begin returns; disk-pending chunk persistence; pending_finalizes idempotency; resume_pending_finalizes ordered after reattach; row-watcher replacing all four coordinator abort arms (composed-path aborts untouched); SnapshotTaken via reconcile + record_snapshot → bool (RETURNING (xmax = 0)); paused_at on SnapshotMetadata with wire bump + golden; kind-scoped rewind; metrics; no migration (correct); FC test wired into ci.yml; ADR 0067.
Gaps not declared in the PR body:
- Testing item (c) — "stage idempotency: re-drive after a crash injected between each stage yields exactly one manifest/version" — dropped, and the legs are in fact not crash-idempotent (findings 1–3 are what that test would have caught).
- The macOS/VZ-lane
InvalidSpecgate test was skipped (finding 6; partially mitigated — the coordinator's InvalidSpec→composed fallback is exercised by the trait-default mock inidle_evictor.rstests, butPooledBackend's ownsupports_diff_checkpoints/checkpoint_dirgate is untested). - The engram-postgres live-PG insert-vs-upsert test for
record_snapshot'sinsertedbool was skipped (finding 7). - The coordinator unit test "composed path stamps cursor from
metadata.paused_at" was not added. - Issue step 6's "extend the
destroy()records-survive comment" was not done.
The issue's 'unverified — confirm' hostPath item was confirmed (ADR 0067 Verification). No type/lint laundering, no weakened assertions, no scope creep observed.
Findings
-
[CONFIRMED]
crates/engram-host-agent/src/eviction_finalize.rs:497(bug, high) —run_memory_legdeletesmemory.diff(andmemory.binat :513) before the stage bump + manifest ref are durably persisted (:530–534). Ifpatch_fc_manifest_memory_refor the persist fails transiently after the delete — or the process crashes before persist — the retry/redrive takes the "already chunked"Nonearm (:499–503), never patches the sidecar, bumps toMemoryChunked, and the terminalCheckpointRecordlands withmemory_manifest: None: silent memory loss on the exact crash window this PR exists to close. Fix: persist the stage bump (with the manifest ref) BEFORE deleting the inputs; theNonearm can also recover the deterministicprev_ref.next_version()ref from the store instead of silently degrading. -
[CONFIRMED]
crates/engram-host-agent/src/eviction_finalize.rs:494(bug, high) —run_memory_leg'sput_manifest(next_ref, …)has noVersionConflictarm (unlikepublish_disk_manifest's retry loop at :387–415).ChunkStore::put_manifestreturnsVersionConflictunconditionally when the key exists (content-blind —store.rs:188–199, pinned byput_manifest_twice_at_same_version_returns_conflict). A crash betweenput_manifestsuccess andremove_filemeans every redrive recomputes the sameprev_ref.next_version(), hits the same conflict, and after 10 attempts the record is quarantined anddestdeleted — losing a snapshot whose chunks AND manifest are already durably in the store. Treat aVersionConflictat exactlynext_refas idempotent success (or verify content). -
[CONFIRMED]
crates/engram-host-agent/src/eviction_finalize.rs:458(bug, medium) —run_disk_legremovesdest/disk-pending/before persistingstage = DiskUploaded. A crash in that window leaves the persisted record atCapturedwith non-emptydisk_pending.chunksand the chunk files gone;read_disk_pending_chunksENOENTs on every redrive attempt → quarantine → the eviction's final snapshot is lost down to the prior periodic checkpoint. (Also:record.stageis mutated in RAM beforepersistcan fail, so an in-process persist failure leaves RAM and disk diverged.) Same fix shape: persist first, delete inputs after. -
[CONFIRMED]
crates/engram-host-agent/src/pooled_backend.rs:4984(bug, medium) —snapshot_begin'srecord.persistfailure arm propagates the error withoutremove_dir_all(&dest), unlike the adjacent disk-pending failure arm (:4914–4933) and the finalizer-Nonearm (:4975).unwindis already defused (:4951), so nothing cleans up. The eviction scanner retries every ~30s, each attempt minting a fresh snapshot_id + multi-GiBdest— the same disk-fill class the disk-pending arm's comment guards against. Add the same cleanup before returning the error. -
[CONFIRMED]
crates/engram-coordinator/tests/e2e_stack.rs:377(cleanup) —Driver::list_eventswas inserted mid-way throughcow_state's doc comment: line 377 ("`SessionService.GetCowState`. ADR 0016 Phase A diagnostic. Returns") now headslist_events' doc, andcow_state's doc (:410–413) starts mid-sentence. Move the stray first line back ontocow_state's block. -
[CONFIRMED]
crates/engram-host-agent/src/pooled_backend.rs:4862(convention) — the issue's Testing item "macOS/VZ lane: assertsnapshot_beginreturnsInvalidSpecover VZ" was silently skipped. The coordinator-side fallback IS covered (trait-defaultInvalidSpecvia the mock backends inidle_evictor.rstests), butPooledBackend's ownsupports_diff_checkpoints()/checkpoint_dirgate — the only thing keeping macOS dev evictions on the composed path — has no red test if a refactor changes its error variant. -
[CONFIRMED]
crates/engram-postgres/src/lib.rs:2439(convention) — the issue's test item "record_snapshotreturnsinsertedcorrectly on insert vs upsert" was not implemented against live Postgres. TheRETURNING (xmax = 0)idiom is only mirrored by hand-rolled Rust logic in the mocks (state.rsMiniMeta,api.rs,grpc_app.rs); no test exercises the actual SQL. If it misreports, the heartbeat reconcile double-emits or never emitsSnapshotTakenwith no red test. One live-PG case incheckpoint_reconcile_live_pg.rs(insert →true, re-record →false) closes it.
Automated deep review (core-ops batch); findings verified against the branch — treat PLAUSIBLE items as questions.
| .put_manifest(next_ref, &next) | ||
| .await | ||
| .map_err(|e| SandboxError::Snapshot(format!("put manifest {next_ref}: {e}")))?; | ||
| let _ = tokio::fs::remove_file(&diff_path).await; |
There was a problem hiding this comment.
[CONFIRMED, bug-high] memory.diff (and memory.bin at :513) is deleted BEFORE the stage bump + record.memory_manifest are persisted at :530-534. A transient failure in patch_fc_manifest_memory_ref/persist after this line — or a crash before persist — makes the retry/redrive take the "already chunked" None arm, skip the sidecar patch, and land a terminal CheckpointRecord with memory_manifest: None: silent memory loss on exactly the crash window #529 exists to close. Persist the stage (with the ref) first, delete the inputs after; the None arm can also recover the deterministic prev_ref.next_version() from the store.
| .map_err(|e| SandboxError::Snapshot(format!("sparse re-chunk: {e}")))?; | ||
| let next_ref = prev_ref.next_version(); | ||
| chunk_store | ||
| .put_manifest(next_ref, &next) |
There was a problem hiding this comment.
[CONFIRMED, bug-high] No VersionConflict arm here, unlike publish_disk_manifest's retry loop above. put_manifest conflicts unconditionally when the key exists (content-blind; pinned by put_manifest_twice_at_same_version_returns_conflict). A crash between this call succeeding and remove_file(memory.diff) means every redrive recomputes the same prev_ref.next_version(), conflicts forever, and quarantines a snapshot whose chunks AND manifest are already durably stored. Treat a conflict at exactly next_ref as idempotent success.
| } | ||
| } | ||
| let pending_dir = record.dest.join("disk-pending"); | ||
| let _ = tokio::fs::remove_dir_all(&pending_dir).await; |
There was a problem hiding this comment.
[CONFIRMED, bug-medium] disk-pending/ is removed before stage = DiskUploaded is persisted. Crash in this window → persisted record still Captured with non-empty disk_pending.chunks but the files gone → read_disk_pending_chunks ENOENTs on every redrive → quarantine, losing the final snapshot down to the prior periodic checkpoint. (Also record.stage mutates in RAM before persist can fail, diverging RAM from disk on an in-process persist error.) Persist first, delete after — same fix shape as the memory leg.
| .persist(&finalizer.finalize_dir()) | ||
| .await | ||
| .map_err(|e| { | ||
| SandboxError::Snapshot(format!("persist eviction finalize record: {e}")) |
There was a problem hiding this comment.
[CONFIRMED, bug-medium] This failure arm propagates without remove_dir_all(&dest), unlike the disk-pending arm at :4914-4933 and the finalizer-None arm at :4975 — and unwind is already defused at :4951, so nothing else cleans up. The eviction scanner retries ~30s apart, each attempt minting a fresh snapshot_id + multi-GiB dest: the disk-fill class the disk-pending arm's own comment guards against. Add the same cleanup before returning.
| @@ -375,6 +375,38 @@ impl Driver { | |||
| } | |||
|
|
|||
| /// `SessionService.GetCowState`. ADR 0016 Phase A diagnostic. Returns | |||
There was a problem hiding this comment.
[CONFIRMED, cleanup] list_events was inserted mid-way through cow_state's doc comment — this line (GetCowState / "Returns") now heads list_events' doc, and cow_state's doc at :410 starts mid-sentence. Move this line back onto cow_state's block.
| &self, | ||
| id: SandboxId, | ||
| ) -> Result<engram_core::types::SnapshotId, SandboxError> { | ||
| if !self.inner.supports_diff_checkpoints() || self.checkpoint_dir.is_none() { |
There was a problem hiding this comment.
[CONFIRMED, convention] The issue's Testing item "macOS/VZ lane: assert snapshot_begin returns InvalidSpec over VZ" was skipped without a PR-body mention. The coordinator's InvalidSpec→composed fallback IS covered by the trait-default mocks in idle_evictor.rs tests, but this gate itself (the only thing keeping macOS dev evictions working) has no red test if a refactor changes the error variant.
| -- recorded with a cursor, or vice versa). | ||
| events_cursor = COALESCE(EXCLUDED.events_cursor, snapshots.events_cursor), | ||
| updated_at = NOW() | ||
| RETURNING (xmax = 0) AS inserted |
There was a problem hiding this comment.
[CONFIRMED, convention] The issue's test item "record_snapshot returns inserted correctly on insert vs upsert" wasn't implemented against live PG — the (xmax = 0) idiom is only re-implemented in Rust by the mocks, so a misreport (double-emitted or never-emitted SnapshotTaken) has no red test. One live-PG case in checkpoint_reconcile_live_pg.rs (insert → true, re-record → false) closes it.
…nitializers CI-red root cause (review finding, unlabeled/compile-blocking): SnapshotMetadata gained paused_at on this branch, but three FC integration test initializers in snapshot_uffd.rs and substrate_uffd_base.rs weren't updated, breaking fmt+clippy+check (--all-targets), the Linux test lane, and the firecracker lane with E0063. diff_snapshot.rs was unaffected (it uses ..lineage.clone()). Mirrors metadata.paused_at like every other field in these restore_metadata blocks.
…g inputs Review findings 1-3 [CONFIRMED]: run_disk_leg and run_memory_leg deleted their durable on-disk inputs (disk-pending/, memory.diff, memory.bin) BEFORE persisting the stage bump that records the leg's result — exactly backwards for a redrive-safe pipeline. A crash (or a transient persist failure) between the delete and the persist made that crash window indistinguishable, on redrive, from "nothing to do this round": the memory leg silently landed a terminal CheckpointRecord with memory_manifest: None (finding 1), and the disk leg's ENOENT on the now-missing disk-pending/ files drove straight to quarantine, losing a snapshot whose manifest may already have been durably published (finding 3) - precisely the crash windows issue #529 exists to close. Fix: publish to the chunk store, THEN persist the stage bump (with the resolved manifest ref), THEN delete the leg's input. A failed persist rolls back the in-RAM stage/ref mutation so a subsequent retry doesn't trust an unpersisted change and safely redoes the (idempotent) publish. Finding 2 [CONFIRMED]: the memory leg's put_manifest(next_ref, ..) had no VersionConflict arm, unlike the disk leg's publish_disk_manifest retry loop. put_manifest conflicts exactly when the (manifest_id, version) key already exists, and next_ref is always the same deterministic ref - so a conflict here can only mean a prior, since-crashed attempt already published this exact content. Treat it as idempotent success instead of retrying into permanent quarantine of already-durable data.
… marker Discovered while validating the review-fix commit under full-workspace test load: run_eviction_finalize's quarantine arm called record.quarantine(..) (which durably persists the finalize/failed/<id>.json marker and frees `dest`) BEFORE clearing pending_finalizes. That leaves a window where a racing snapshot_begin's idempotency check (pending_finalizes.get) hands back a snapshot_id that's already permanently dead — nothing re-drives a quarantined record, so that caller would wait out the coordinator's row-watcher deadline for a row that will never land. Reorder: clear the idempotency entry first, so by the time the quarantine becomes externally observable, no caller can still be routed to it. (Surfaced locally as an intermittent finalize_quarantines_after_max_attempts failure only under full `cargo nextest run --workspace` CPU contention — the same race, just also visible to the test's own poll-then-assert.)
…sist Review finding 4 [CONFIRMED]: snapshot_begin's record.persist(..) failure arm propagated the error without remove_dir_all(&dest), unlike the disk-pending failure arm just above it and the checkpoint_dir-vanished None arm just before it (unwind is already defused by this point, so nothing else cleans up). The eviction scanner retries roughly every 30s; each attempt mints a fresh snapshot_id + multi-GiB dest, so an unhealthy finalize_dir write path turns into an unbounded disk-fill class — exactly the failure mode the disk-pending arm's own comment guards against. Mirror that arm's cleanup here.
…ion-finalize Issue #529's own testing/verification checklist step 6 (an undeclared PR-body gap per the review): extend the "durable RECORDS deliberately survive destroy" comment on ADR 0028's checkpoint-chain teardown to also name EvictionFinalizeRecord / CheckpointRecord{kind:EvictionFinal} / the pending_finalizes map, and state explicitly that destroy() touches none of them. In the normal flow this is moot (run_terminal clears its own state strictly before calling destroy()); documents why an out-of-band destroy() call would still be safe.
…ess-shaped backends Review finding 6 [CONFIRMED]: the issue's Testing item "macOS/VZ lane: assert snapshot_begin returns InvalidSpec over VZ" was silently skipped. The coordinator's InvalidSpec -> composed-path fallback IS covered by the trait-default mocks in idle_evictor.rs, but PooledBackend's own supports_diff_checkpoints()/checkpoint_dir gate — the only thing keeping a VZ/Process host on the composed snapshot() pipeline instead of the host-durable path — had no test that would go red if a refactor changed the error variant. Two unit tests: one wraps ProcessBackend (same trait-default false as VZ, neither overrides supports_diff_checkpoints) with a checkpoint_dir wired, and asserts snapshot_begin returns InvalidSpec; the other wraps an FC-shaped diff-checkpoint-capable backend with NO checkpoint_dir wired and asserts the same. Together they cover both halves of the gate.
Review finding 5 [CONFIRMED]: list_events's insertion left cow_state's opening
doc line ("SessionService.GetCowState. ADR 0016 Phase A diagnostic. Returns")
heading list_events's doc block instead, with cow_state's own doc starting
mid-sentence at "Some(state) when...". Move the line back onto cow_state's
block where it belongs.
… Postgres Review finding 7 [CONFIRMED]: the issue's test item "record_snapshot returns inserted correctly on insert vs upsert" wasn't implemented against live Postgres. The RETURNING (xmax = 0) idiom is only mirrored by hand-rolled Rust logic in the coordinator's mocks (MiniMeta, api.rs, grpc_app.rs); a misreport in the real SQL had no red test, and the heartbeat reconcile uses this bool to decide whether to emit SnapshotTaken exactly once. New live-PG case in checkpoint_reconcile_live_pg.rs: the first landing of a snapshot id INSERTs (true), an idempotent re-record of the same id UPDATEs (false), and a genuinely distinct id INSERTs again (true) even though a row already exists for the session.
A deep-review pass on PR #558 found the disk/memory legs deleting durable inputs before persisting their stage bump (backwards for a redrive-safe pipeline) plus a missing VersionConflict tolerance on the memory leg's manifest publish — exactly the crash windows this ADR's decision section claims to close. Record the pre-merge fix (persist-then-delete, with an in-RAM rollback on a failed persist, plus idempotent-success on a deterministic VersionConflict) in the Decision section so the ADR's description of "each leg idempotent, persisted before the next runs" reads honestly about what "persisted" ordering actually means within a leg.
Review-response checklistDeep-review pass findings mapped to the commit that fixed them (all [CONFIRMED], all fixed — none disputed):
Also addressed (flagged by the review as an undeclared PR-body gap, not a numbered finding):
Found while validating the fixes under full-workspace test load (not a review finding, but the same "durability-ordering" class as findings 1-3): ADR 0067 updated to describe the persist-then-delete ordering honestly → Not fixed here (scoped as follow-ups, not silently dropped)
Validation
|
…: WIRE_VERSION 8→9 + conflict resolution Conflicts: idle_evictor.rs (host-durable row-watcher supersedes the old span-parented finalize task — main had no competing implementation there), checkpoint_reconcile_live_pg.rs (kept both new test functions), host-agent metrics.rs (additive constants from both sides), postgres/lib.rs (fc_snapshot_version bind + fetch_one; composed the prompt_received + lifecycle-kind rewind exclusions into one `AND kind NOT IN (...)`), wire.rs (this branch's bump becomes v9, listed after main's v8). Also renumbers this branch's own ADR 0067 -> 0069 (0067 was already double-booked by two merged main ADRs) and threads paused_at/ fc_snapshot_version through struct literals main's merge left incomplete. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4WkZddtVw8djsWz2RCcQc
…ransaction write-set + boot-bundle cache with #556/#559/#560/#562/#564/#565 Merges main's absorbed batch (#553-#565, #558) into this branch's create-as-plan restructuring (per-image boot bundle, one-transaction session write-set, overlapped boot legs, prompt-over-wire). Conflicts were in the functions both sides restructured (reserve_and_persist_create replacing reserve_placement/enqueue_session_create, pg_listener::spawn's params, the boot_prepared/prepare_inner create path) plus several non-conflicting hunks that referenced symbols the other side renamed/removed (git auto-merged clean but left dangling references — fixed by hand, verified by the full verification gate below). Key composition decisions: - #556 PromptReceived-first ordering: intact — send_prompt_core still emits the durable receipt as the FIRST PG write, before ensure_active_and_resolve; #566's deliver_prompt runs after, unchanged in position. - #559 NOTIFY equivalence: reserve_and_persist_create's Queued arm now fires notify_placement_changed("enqueued") after commit, replacing the retired enqueue_session_create's NOTIFY (was about to be silently dropped since #566 subsumed that function). Verified live against Postgres (notify_placement_changed_fires_at_every_site). - #562 span parenting: kept on the create boot_handle spawn (.instrument(boot_span)); boot_on_reserved_host's overlapped legs use tokio::join! (same task/span), so no new spawn site needed instrumentation. - #564 CapabilityRequirements / #565 digest gate: ScheduleContext.caps and required_image_digest survived the merge already wired to the per-image boot bundle's cached fields (bundle.enabled.manifest_digest / base_snapshot_memory_manifest) after fixing two dangling bare `enabled.` refs left by a non-conflicting auto-merge hunk. - WIRE_VERSION stays at main's 9: #566's "prompt over the wire" changes are behavioral only (every prompt now rides the pre-existing HarnessCommand::Prompt frame instead of ENGRAM_INITIAL_PROMPT env) — no coord<->host-agent gRPC wire.rs shape change, no harness-proto frame shape change. - #560 GuestReady removal: reserve_and_persist_create's reserved-mib query had its own copy of the status-list (duplicated by this branch's restructuring out from under main's edit); removed 'guest_ready' by hand to match. - Migrations: this branch's 0082_session_selected_skills.sql sits cleanly after main's 0081; no renumbering needed. No ADR added by this branch (no collision). Additional fixes for auto-merged-but-now-dangling references (both test files and src, caught by clippy/cargo check, not by conflict markers): - queue_scanner_live_pg.rs: ~10 test call sites main added independently (per-fit- class + digest-gate tests) still called the retired enqueue_session_create / reserve_placement; rewired onto this branch's enqueue()/reserve() shims (added a reserve() shim mirroring placement_reservation_live_pg.rs's). Its own pg_listener::spawn call site needed the boot_bundles arg too. - boot_bundle.rs's CountingMeta test mock: record_snapshot return type (Result<()> -> Result<bool>), and SnapshotRecord/HostRecord test literals missing fc_snapshot_version / capabilities+stages_images. - Three Session{} test literals (api/prompt.rs, queue_scanner.rs, state.rs) missing selected_skills. Verification: cargo fmt --check, cargo clippy --workspace --all-targets -D warnings (clean), cargo hakari verify (clean), cargo nextest run -p engram-coordinator -p engram-host-agent -p engram-protocol -p engram-core -p engram-postgres (779/779 passed), all 17 Postgres-gated --test suites from ci.yml against a throwaway engram_test_* DB (89/89 passed, --test-threads=1), nix develop musl clippy cross-check (clean). No proto/web/orchestrator changes in this branch, so no buf regen or pnpm/bun typecheck needed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T4WkZddtVw8djsWz2RCcQc
Closes #529
Summary
Host-durable eviction finalize: once
PooledBackend::snapshot_beginreturns, the finalize is a host-owned job that is a pure function of durable on-disk artifacts — it never touches the sandbox, the coordinator, or any in-RAM map again. A host-agent process restart re-drives it from disk. Combined with apaused_at-based coherence cursor and a kind-scoped session-event rewind, this closes the ~24% idle-eviction snapshot-loss class and the 45/45 "every resume looks like a rewind" defect described in the issue.Commit chain (8 commits, one logical change each):
fix(coordinator): scope session-event rewind to guest-derived kindsfeat(protocol): carry the host's exact pause instant onSnapshotMetadata(WIRE_VERSION 7→8)feat(protocol,postgres,coordinator):record_snapshotreturnsinserted;SnapshotTakenmoves to the heartbeat reconcilefeat(host-agent): host-owned, re-drivable eviction finalize (eviction_finalize.rs, new module;snapshot_beginrewrite;resume_pending_finalizes)fix(coordinator): D5 finalize sheds durability authority, keeps only serialization (the row-watcher)test(host-agent): FC integration test for the redrive (real KVM,#[ignore]'d, wired into CI)test(e2e): extend evict→resume coverage for the kind-scoped rewind fixdocs(adr): ADR 0067See ADR 0067 (
docs/adr/0067-host-durable-eviction-finalize.md) for the full design.Review-pass fixups (2026-07-03)
A deep-review pass on this PR found the CI-red compile bug plus 7 [CONFIRMED] findings, all now fixed on top of the original 8-commit chain:
SnapshotMetadatagainedpaused_at, but 3 FC integration test initializers (snapshot_uffd.rsx2,substrate_uffd_base.rs) weren't updated →83522bad.run_disk_leg/run_memory_legdeleted their durable on-disk inputs (disk-pending/,memory.diff/memory.bin) before persisting the stage bump that records the leg's result — backwards for a redrive-safe pipeline. A crash in that exact window could silently landmemory_manifest: None(finding 1) or quarantine an already-durably-published snapshot (finding 3). Fixed: publish → persist the stage bump → delete the input, with an in-RAM rollback on a failed persist →43c8045f. Finding 2 (noVersionConflicttolerance on the memory leg'sput_manifest, so a redrive after a crash between publish and persist would conflict forever and quarantine already-durable data) fixed in the same commit.snapshot_begin'srecord.persist(..)failure arm leaked the multi-GiB staging dir (missing theremove_dir_allits sibling arms have) →c6756941.cow_state/list_eventsine2e_stack.rs→162d2663.snapshot_beginreturnsInvalidSpec" test coverage (PooledBackend's ownsupports_diff_checkpoints()/checkpoint_dirgate, both halves) →c35404d2.record_snapshotinsert-vs-upsertRETURNINGtest →df910692.destroy()"records survive destroy" comment to nameEvictionFinalizeRecord/pending_finalizesexplicitly →3c647e99.run_eviction_finalize's quarantine arm clearedpending_finalizesafterquarantine()made the failure externally observable, leaving a window where a racingsnapshot_begincould hand back a snapshot_id that's already permanently dead. Fixed →7028085c.7a24cb36.Two testing gaps the review flagged as narrative (not numbered findings) — crash-injection between each stage (issue Testing item (c)), and a coordinator unit test asserting the composed path's cursor is stamped from
metadata.paused_at(the in-memoryMiniMetamock can't currently distinguish that from wall-clocknow) — were not closed here; both need either new fault-injection test infra or a mock extension, scoped as a follow-up rather than bundled into this pass. (Tracked informally; see the fixup session's final comment on this PR for the writeup.)Also discovered, tracked separately, NOT fixed here (out of scope for #529):
cargo nextest run --workspacehas 3 pre-existing, deterministic failures unrelated to this change (disk_daemon::backend::tests::flush_write_throughs_chunks_into_local_cacheand twopooled_backendchunk-cache tests). The original Testing section below called these "a local chunk-cache environment quirk" — that characterization is incorrect; they're a real regression already tracked in #552 ("chunk write-through cache not populated (3 test failures post-#522)"), reproduced identically with and without this fixup pass's changes.just check's literal gate is red only because of #552, not anything in this PR.Acceptance criteria
resume_pending_finalizes) + proven end-to-end against a real Firecracker VM incrates/engram-host-agent/tests/eviction_finalize_redrive.rs. Corrected 2026-07-03: the review-pass fixups above (items 2-3) were required for this to be genuinely true — the original cut had exactly the silent-data-loss crash window this criterion claims to close.recovered_from_checkpoint(rolled_back == 0); lifecycle events never tombstoned. Implemented + tested (checkpoint_reconcile_live_pg.rs,e2e_stack.rs).snapshot_waits, sodestroy()'s abort can't touch it (self-destruction race closed as a side effect, found during verification).finalize_quarantines_after_max_attempts). No "→0" loss claim anywhere. Corrected 2026-07-03: the original cut over-quarantined (finding 2 — an already-durably-published manifest could hit permanentVersionConflictand quarantine needlessly); fixed in the review pass. The criterion's own letter ("never silent, falls back at most to the prior checkpoint") held even before the fix, but the fix closes a real false-quarantine class.SessionLeaseGuardsemantics unchanged; the row-watcher just holds it differently.evict_attemptsretry storms hit the idempotentsnapshot_begin(re-observe, not re-capture). Implemented + tested (snapshot_begin_is_idempotent_under_a_pending_finalize).D5 finalize: ... sandbox not found/abort_snapshot failedlog signatures going to zero over a post-deploy 7d window. Cannot be verified pre-merge.snapshot_begin; needs a real measurement post-deploy.Stale anchors found + corrected
The issue was verified against
f6602259; this worktree started at42ed9bb2(4 commits later). The only drift: an unrelatedbundle_file_extparameter threaded throughBundleStore::new/SnapshotFinisher(PR #521, browser-bundle merge) shifted line numbers by 5-15 inpooled_backend.rsbut changed no cited behavior. All other file:line anchors (idle_evictor.rs, host_registry.rs, checkpoint.rs, disk_daemon/backend.rs, host_http.rs, etc.) checked out essentially as-is. The<work_dir>hostPath-durability item the issue flagged as "unverified — confirm during implementation" was confirmed (see ADR 0067's Verification section):deploy/helm/engram-host-fleet/templates/host-agent.daemonset.yamlmounts it as ahostPath(DirectoryOrCreate), not anemptyDir.Deviations (documented, not silent)
ChunkedDiskBackend::flush_upload(preserving its live-backend-state rebase) when the NBD data plane still exists, and a "redrive path" reconstructing the manifest from persisted bytes otherwise. This implementation always reconstructs from the persisteddest/disk-pending/bytes — the eviction flavor destroys the sandbox immediately after finalize, so nothing ever readsflush_upload's rebased live state again, making the rebase unobservable for this flavor. One code path is lower-risk than keeping a live backend handle + flush-pipeline guard pinned across a backgrounded, potentially long upload window, at the cost of a redundant O(dirty-set) NVMe write+read on the non-crash path. Documented in theeviction_finalize.rsmodule doc and ADR 0067.SessionService.EvictLocal) requires a pre-existing snapshot and just detaches+destroys locally; it does not exercise the D5 async finalize pipeline this issue rewrites (no RPC triggers the scanner-driven idle-eviction path, by ADR 0051 design). That property is instead covered — at the correct altitude — by the new FC integration test drivingsnapshot_begin/resume_pending_finalizesdirectly against a real VM.cargo nextest run -p engram-postgres; live-PG integration tests forPgMetadataStoreactually live incrates/engram-coordinator/tests/*_live_pg.rs(already CI-wired,checkpoint_reconcile_live_pg), not in theengram-postgrescrate itself. Added there instead.MetadataStore::record_snapshot's signature change (Result<()>→Result<bool>) mechanically touched ~9 implementors across engram-postgres/coordinator/chunk-store/oci-auth — all updated honestly, no laundering.Testing
cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings,cargo hakari verify: all clean (re-verified after the review-pass fixups, including a Linux cross-check viacargo clippy --target aarch64-unknown-linux-musl -p engram-host-agent --all-targets).cargo nextest run --workspace --no-fail-fast: 1347/1350 passed, 101 skipped (live-PG/FC/e2e#[ignore]'d). The 3 failures are tracked in Regression on main: chunk write-through cache not populated (3 test failures post-#522) #552 (pre-existing chunk write-through regression, unrelated to this change — see "Review-pass fixups" above for the corrected characterization).pooled_backend.rs): idempotentsnapshot_begin, record-persisted-before-return, full happy path →CheckpointRecord{kind:EvictionFinal}+ destroy, crash-redrive with the sandbox entirely absent, quarantine after max attempts, plus (review-pass) the VZ/Process-shaped and unwired-checkpoint_dirInvalidSpecgate tests.idle_evictor.rs): row-watcher observes a host-landed row without ever callingcommit_snapshot/destroy; releases its lease at the deadline.checkpoint_reconcile_live_pg.rs): kind-scoped rewind, plus (review-pass)record_snapshot'sinsertedbool on insert vs. idempotent re-record vs. a distinct id.eviction_finalize_redrive.rs,#[ignore]'d, wired intoci.yml's firecracker lane) — cross-checked clean onaarch64-unknown-linux-musl(cannot execute locally, no KVM in this environment).e2e_resume_preserves_disk_and_memory) — compiles clean, cannot execute locally (needs the live stack)..github/workflows/ci.ymlvalidated withyaml.safe_load+actionlint(pre-existingblacksmith-*runner-label warnings only, none on touched lines).Note on ADR numbering
Used ADR 0067 (next free number against
42ed9bb2at the time of writing). A sibling PR in this batch (#528, chunk-cache-disk-budget) independently also picked 0067 against the same base — expected per the batch coordination model; will be renumbered at merge/rebase time.Landing order: #541 -> #536 -> #533 -> #527p1 -> #528 -> #530 -> #537 -> #540 -> #539 -> #526 -> #531 -> #538 -> #529 -> #535