Skip to content

pool: write archive-delete markers at delete attempts, not archive time - #65

Merged
CMGS merged 3 commits into
mainfrom
fix/archive-delete-marker-narrowing
Aug 11, 2026
Merged

pool: write archive-delete markers at delete attempts, not archive time#65
CMGS merged 3 commits into
mainfrom
fix/archive-delete-marker-narrowing

Conversation

@CMGS

@CMGS CMGS commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Closes #62.

Implements the narrowing as designed in #62, with one correction the implementation surfaced, then a follow-up commit that closes the remaining recovery windows by making the invariant uniform.

Correction to the issue's window-2 analysis

reclaimOrphanArchiveCks deliberately skips Archive-flagged checkpoints whose sandbox is absent from the journal — on a shared store they may back another node's live claim. So once a release/reap-purge claims commit lands, the journal no longer identifies the ck as ours, and a failure between that commit and the first markArchiveCk inside deleteArchiveCk leaves the ck with no marker, no journal reference, and no API that can reach it (listings, deletes and the TTL sweep all filter Archive records). The issue's claim that window 2 "recovers via reclaimOrphanArchiveCks either way" does not hold there — and the window is reachable without a crash: an unwritable marker dir fails the pre-commit mark and the purge's internal mark, so the delete never runs while the release still succeeds (reproduced in a test before the fix).

Design as landed

One invariant on all three delete paths: the delete-intent marker durably exists before the claims journal drops or clears its last reference to the ck, and a mark failure aborts before anything commits.

  • archive() no longer marks; Reconcile no longer re-marks adopted claims (no restart rewrites).
  • releaseResolved: marks under m.mu before removing the claim — a mark failure aborts with nothing to roll back; the commit-failure rollback clears the marker.
  • reapOnce: two-phase — collect without mutating, mark outside the lock (a failed mark just keeps that victim), then re-check claimed[id]==sb && ArchiveCk==ck under m.mu before pinning and removing; the batch rollback clears the marked cks.
  • wakeArchived: marks after provision, before commitWake; a mark failure destroys the built VM and fails the wake. A failed consumed-delete stays retryable via the marker; success clears it. This also covers the release-races-mid-wake orphan that journal-based recovery alone missed.
  • Reconcile gained clearCanceledArchiveDeletes: a marker naming a live journaled claim's current ArchiveCk is a removal that never committed — cleared at startup, so crashed rollbacks cannot leave the retry tick paying the pinned scan forever.
  • purgeArchiveCk deletes and clears without re-marking (its callers guarantee the mark); deleteArchiveCk keeps mark→delete→clear for the unmarked callers (orphan cleanup, restart reclaim). retryArchiveDeletes/retryArchiveDelete and the pinned re-check are unchanged; marker listing is shared via one archiveDeleteMarkers helper.

Stale markers (a consumed or already-deleted ck) self-heal on the next tick: store Delete is idempotent on a missing id for both backends, so the retry deletes nothing and clears the marker. The one known residual — a persist-failure wake rollback leaves a pinned marker until the next lifecycle event or restart — is bounded (one non-free tick loop) and deliberately not patched inline: every inline clear variant reintroduces a leak window against a racing removal's marker.

Steady state: the marker dir holds no entries, so the 5s retry tick early-returns before touching m.mu; restarts write no markers. The pre-commit marking makes the diff net-positive in lines rather than the issue's −15 estimate — the delta is the leak fix above.

Tests

  • TestReconcileAdoptsLegacyArchiveMarkerTestReconcileClearsLiveArchiveMarker: archive leaves no marker, and a seeded marker on a live archived claim is cleared by restart.
  • New TestArchiveRemovalRequiresDeleteMarker / TestArchiveWakeRequiresDeleteMarker: an unwritable marker dir aborts release, reap-purge and wake before their commits; healing the dir lets each complete.
  • New TestArchiveWakeDeleteFailureRetries: a failed consumed-delete retains ck + marker; the retry tick converges.
  • TestArchiveDeleteRetryRechecksWakeRollback: now exercises the real pre-commit wake marker (no synthetic seeding) against the recLock serialization and the pinned re-check, and asserts the post-rollback wake converges.
  • TestArchivePublishWindowPinsCheckpoint drops its now-vacuous mid-publish retry call; TestArchiveDeleteRetryAfterRestart and TestArchiveRemovalCommitPinsCheckpoint pass unchanged.

Evidence (rerun at HEAD)

  • go test -race -count=1 ./... (sandboxd module): all 12 packages ok
  • make go-lint (run + fmt, GOOS linux+darwin, all Go modules): 0 issues
  • asl ./sandboxd/... both GOOS: zero findings on touched files (the 3 pool.go findings pre-exist on main, file untouched here)
  • Steady-state tick re-benchmarked at HEAD: 13.1–13.3 µs / 1.5 KB / 7 allocs, independent of claim count (vs 0.71 ms @1k and 88 ms @100k archived claims with lifetime markers); markArchiveCk itself is ~18 µs — the pair now paid under m.mu only on an archived claim's release.

Hot-path cost: zero on the claim/exec/wake fast paths and on the steady-state 5s tick; an archived claim's release/purge/wake pays one marker write + one clear on its own cold path.

Markers under archive-deletes/ tracked "currently archived" rather than
"delete attempted": archive() wrote one per checkpoint, Reconcile rewrote
them all on every restart, and any archived claim kept the dir non-empty,
so every 5s retry tick paid an m.mu full-claim scan for the lifetime of
the archive feature.

Narrow the marker to delete intent. Release and reap-purge mark right
before the claims commit and abort if the marker cannot land: after the
commit the journal no longer references the ck and restart reclaim skips
unknown sandboxes (shared-store safety), so an unmarked ck would leak
invisibly. A failed consumed-delete in wakeArchived marks at the failure
(a woken claim stays in the journal, so the crash case is reclaim's job).
purgeArchiveCk deletes directly, skipping deleteArchiveCk's now-redundant
re-mark. Steady state leaves the marker dir empty, making the retry tick
a free early return; convergence per failure window is unchanged.
@CMGS

CMGS commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Measured the steady-state tick cost. retryArchiveDeletes itself is untouched by this PR, so running it against a marker-per-claim directory vs an empty one on this branch is a faithful pre/post A/B. Setup: N claims with ArchiveCk set, plus (pre case) one marker file per claim; Apple M2 Max, darwin/arm64, -benchmem -count=5, throwaway benchmark (not committed).

Full tick (retryArchiveDeletes, runs every 5s):

archived claims pre (markers present) post (empty dir)
1k 0.80–0.86 ms, 250 KB, 2043 allocs 14–18 µs, 1.5 KB, 7 allocs
100k 100–150 ms, 26 MB, 200k allocs ~14 µs, 1.5 KB, 7 allocs

m.mu-held portion alone (pinnedArchiveCks: full-claim scan + pinned map build — the part that contends with the claim path):

archived claims lock hold per tick, pre post
1k ~62 µs, 109 KB allocated under the lock 0 — lock never taken
100k ~10 ms, 7 MB under the lock 0

Post-change the tick is O(1) in claim count. The issue's estimate (10–30 µs at 1k, ~ms at 100k) understated the pre-cost by roughly 2–10×, so the removed recurring work is larger than claimed. Numbers are from a mac, but the 50×–7000× deltas are far beyond file-I/O bench noise, and the structural claims (no m.mu acquisition, no per-claim restart writes) are hardware-independent.

CMGS added 2 commits August 11, 2026 20:10
clearCanceledArchiveDeletes and retryArchiveDeletes carried identical
ReadDir+filter boilerplate; one archiveDeleteMarkers helper keeps the
marker-file predicate from drifting between the startup healer and the
retry tick. The empty-dir early return stays ahead of the pinned scan.
@CMGS
CMGS merged commit 69a483d into main Aug 11, 2026
1 check passed
@CMGS
CMGS deleted the fix/archive-delete-marker-narrowing branch August 11, 2026 12:45
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.

pool: write archive-delete markers at delete attempts, not at archive time

1 participant