Skip to content

fix(durability): govern snapshot blobs by the pin-set GC model (root-cause fix, Part A) - #85

Merged
nikhilunni merged 3 commits into
mainfrom
worktree-snapshot-blob-pinset-gc
Jun 4, 2026
Merged

nikhilunni merged 3 commits into
mainfrom
worktree-snapshot-blob-pinset-gc

Conversation

@nikhilunni

Copy link
Copy Markdown
Contributor

Root cause, not a fourth patch

#82 (commit-before-destroy + self-heal) and #84 (cross-host blob keys) patched symptoms of one signature: a snapshots row marked recoverable=true whose snapshots/<id>/{state.bin,sidecar} blobs were deleted, bricking resume. The prod test of #84 showed the eviction snapshot a02afad3 re-brick the identical way (a periodic checkpoint aborting the eviction snapshot ~350 ms before its commit). These aren't separate bugs — they're one missing abstraction:

Portable snapshot blobs were the only durable resource class not governed by the pin-set GC model. Chunks (chunk_gc) and bundles (bundle_gc) are "pinned by a live PG row → swept when unreferenced after a grace period, behind the chunk_generation barrier." Snapshot blobs instead used a host-local, single-slot-per-sandbox inflight_snapshots map + abort_prior_inflight_snapshot, which blind-deleted the blobs with no PG check. Every producer (idle eviction, periodic checkpoint, manual snapshot, SIGTERM) shares that one slot, so a concurrent producer's snapshot() aborts another's already-recorded snapshot. #82's commit ordering narrowed but couldn't close it — the race is producer-vs-producer on the shared slot, not ordering against destroy.

The fix (Part A of the plan)

Bring snapshots/<id>/{state.bin,sidecar,working_set} under the same pin-set model — a near-mirror of bundle_gc:

  • snapshot_blob_gc sweep + snapshot_blob_gc_candidates (migration 0055), riding the existing gc_sweep_loop tick / chunk_generation barrier / ChunkGcConfig grace; admin dry-run/sweep endpoints. No new env var.
  • Pin set = SELECT id FROM snapshots — every row, NO recoverable filter (the resume self-heal transiently demotes rows) and NO session_id filter (template/base captures are session_id NULL, FK'd from enabled_images.base_snapshot_id, with no resume self-heal backstop). Complete because base rows are never deleted (prune_session_snapshots is session_id IS NOT NULL only; the FK has no ON DELETE).
  • abort_prior_inflight_snapshot now removes only the LOCAL dir (host-disk hygiene, rebuildable from GCS); the retention sweeper's inline blob.delete (the fix: cross-host resume materialization + orphaned snapshot-blob GC (follow-up to #82) #84 stop-gap) is removed.

Net invariant: nothing but the sweep deletes a durable snapshot blob, and only when no snapshots row references it. recoverable=true is true by construction. This retires the inline abort-delete + the #84 stop-gap rather than adding a fourth patch (net −88/+64 in the refactor commit).

One snapshot-specific adaptation vs. bundle_gc: every capture has an upload-before-record window, so the promote pass re-verifies the pin set at delete time — a candidate whose row has since landed is dropped, never deleted (snapshot ids are fresh UUIDs, so a re-pin can only mean "the row was recorded after we marked it"). Strictly safer than the across-sweep barrier alone.

Scope

This is Part A (durability root cause). Part B — generalizing the cross-host-resume self-heal into the evac/dead-host path so auto-recovery doesn't give up on a bricked latest snapshot — lands separately.

Tests

  • snapshot_blob_gc unit (key parse / dedup / malformed).
  • snapshot_blob_gc_live_pg (wired into the CI live-PG lane): pin-set includes session and session_id NULL template rows; orphan swept after grace while pinned (incl. template) survive; re-pinned candidate skipped (the upload-before-record safety). All pass against a local Postgres.
  • Updated the two pooled_backend snapshot-lifecycle tests to the new contract (abort/retry keep the blobs).
  • just check green: fmt + clippy + 968 tests.

See ADR 0028 addendum (2026-06-04 #2).

🤖 Generated with Claude Code

nikhilunni and others added 3 commits June 4, 2026 14:42
The recurring "snapshots row says recoverable=true but its
snapshots/<id>/ blobs were deleted" brick (incident 89f7984d, the
a02afad3 re-brick) has one root cause: portable snapshot blobs were the
only durable resource class not under the pin-set GC model that governs
chunks (chunk_gc) and bundles (bundle_gc).

Add `snapshot_blob_gc` — a near-mirror of bundle_gc — bringing
snapshots/<id>/{state.bin,sidecar,working_set} under "pinned by a live
snapshots row, swept when unreferenced after the shared grace period,
behind the chunk_generation barrier":

- migration 0055: snapshot_blob_gc_candidates.
- MetadataStore pin-set + candidate methods (trait defaults + PG impls).
  Pin set = `SELECT id FROM snapshots`, NO recoverable filter (the resume
  self-heal transiently demotes rows) and NO session_id filter
  (template/base captures are session_id NULL, FK'd from enabled_images,
  with no resume self-heal backstop). Complete because base rows are
  never deleted.
- the sweep rides the existing gc_sweep_loop tick + admin
  dry-run/sweep endpoints (no new env var; shares ChunkGcConfig).

Snapshot-specific vs bundle_gc: every capture has an upload-before-record
window, so the promote pass re-verifies the pin set at delete time — a
candidate whose row has since landed is dropped, never deleted (ids are
fresh UUIDs, so re-pin can only mean "row recorded after marking").

Tests: sweep unit (parse/dedup/malformed) + live-PG (wired into the CI
PG lane) covering session + session_id-NULL-template pin, orphan sweep,
and the re-pinned-candidate skip.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Now that the snapshot-blob GC sweep owns blob lifecycle, retire the two
inline deleters that caused the brick:

- pooled_backend `abort_prior_inflight_snapshot` removes only the LOCAL
  dir (host-disk hygiene, rebuildable from GCS); it no longer deletes
  state.bin/sidecar/working_set. This was the engine of the race — a
  concurrent producer (periodic checkpoint vs eviction sharing the one
  per-sandbox inflight slot) aborting another's already-recorded
  snapshot.
- checkpoint_retention drops the inline blob.delete (the #84 stop-gap);
  pruned rows' blobs are now unpinned and reaped by the sweep.

Net invariant: nothing but the sweep deletes a durable snapshot blob,
and only when no snapshots row references it. Updates the two
snapshot-lifecycle tests to the new contract (abort/retry keep the
blobs).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the root-cause fix: portable snapshot blobs were the only
durable resource class outside the pin-set GC model; the inline
abort-delete was the engine of the recurring brick. Records the pin-set
completeness argument (all rows, no recoverable/session_id filter; base
rows never deleted) and the promote-time re-check for the
upload-before-record window.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nikhilunni
nikhilunni merged commit cf66cbd into main Jun 4, 2026
13 checks passed
@nikhilunni
nikhilunni deleted the worktree-snapshot-blob-pinset-gc branch June 4, 2026 22:03
nikhilunni added a commit that referenced this pull request Jun 4, 2026
Rebased onto origin/main (picks up #84 cross-host resume materialization +
#85 snapshot-blob pin-set GC). Neither touches the harness/vsock layer, so
neither bears on the open warm-restore reconnect issue (P5b) — they're
host-side snapshot-blob durability/GC.

Integration fixups:
- #85 also added migration 0055 (0055_snapshot_blob_gc.sql), colliding with
  this branch's 0055_snapshots_warm_harness.sql → renumber ours to 0056
  (+ the "migration 0055" code/doc references).
- #85's new snapshot_blob_gc_live_pg.rs builds SnapshotRecord literals that
  predate warm_harness → carry warm_harness: false through them.

dev-vm cargo clippy --workspace --all-targets -D warnings green; fmt clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nikhilunni added a commit that referenced this pull request Jun 5, 2026
Rebased onto origin/main (picks up #84 cross-host resume materialization +
#85 snapshot-blob pin-set GC). Neither touches the harness/vsock layer, so
neither bears on the open warm-restore reconnect issue (P5b) — they're
host-side snapshot-blob durability/GC.

Integration fixups:
- #85 also added migration 0055 (0055_snapshot_blob_gc.sql), colliding with
  this branch's 0055_snapshots_warm_harness.sql → renumber ours to 0056
  (+ the "migration 0055" code/doc references).
- #85's new snapshot_blob_gc_live_pg.rs builds SnapshotRecord literals that
  predate warm_harness → carry warm_harness: false through them.

dev-vm cargo clippy --workspace --all-targets -D warnings green; fmt clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant