Repository navigation
fix(machines): orphan-teardown reconciler + hard-purge guard - #2070
Conversation
deleteMachine tears Sprites down best-effort and documents a failed kill as "a recoverable state a background reconciler can reclaim" — no such reconciler existed. A failed kill left a live, billing microVM with no owner reachable from inside the app, and the 30-day hard purge then FK-cascade-deleted its tracking row, destroying the only pointer to it. - machine_branches rows are now deleted only after a confirmed kill (mirroring machine_sessions), making "row exists + page trashed" a uniform, migration-free pending-teardown signal. - MachineHost.kill is idempotent: an already-gone Sprite is a success, not a failure (mirrors attach's not-found handling). - New reconcileOrphanSprites (pure core + injected deps, per-row failure isolation) behind a new 30-minute cron. - purgeExpiredTrashedPages now skips a page that still has a live Sprite-tracking row; countStaleBlockedTrashedPages surfaces a stuck orphan as a growing signal instead of silently destroying the pointer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
📝 WalkthroughWalkthroughAdds an outbox-backed orphan Sprite reconciler with database triggers, teardown intent markers, instance-aware CAS cleanup, strict idempotent kill handling, an authenticated cron route, scheduled execution, and unit/integration coverage. ChangesOrphan Sprite Reconciliation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Cron
participant Route
participant Reconciler
participant Database
participant SpriteHost
Cron->>Route: Send signed reconcile request
Route->>Reconciler: Run orphan reconciliation
Reconciler->>Database: Load reclaim candidates
Reconciler->>SpriteHost: Kill Sprite with expected instance ID
SpriteHost-->>Reconciler: Return kill result
Reconciler->>Database: Release, stamp, or record candidate outcome
Route-->>Cron: Return metrics and audit result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f4e321db0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const branch of branchRows) { | ||
| try { | ||
| await host.kill({ machineId: branch.sandboxId }); | ||
| await db.delete(machineBranches).where(eq(machineBranches.id, branch.id)); |
There was a problem hiding this comment.
When a Machine has branch-scoped agent terminals, this hard-deletes its machine_branches row as part of a reversible page trash. machine_agent_terminals.machineBranchId is declared with onDelete: 'cascade' in packages/db/src/schema/machine-agent-terminals.ts, so this also removes those terminal records immediately, despite this file's contract that machine_agent_terminals metadata survives until hard purge. Restoring the Machine then brings back the page without its branch terminal configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 7a02db6 — this was a real regression, and the cascade you spotted was only half of it.
You are right that machine_agent_terminals.machineBranchId is onDelete: cascade, so deleting the branch row on a reversible trash destroyed the user's branch terminals. Digging further, the branch row is not merely a pointer at all: spawnBranch has an explicit // Vanished — fall through and re-provision under the SAME session key path that re-clones from the project's repoUrl and re-checks-out the branch. So a branch row whose Sprite is dead is re-creatable configuration, exactly like machine_projects — which is precisely what this file's header said it preserves.
So I stopped deleting it. New nullable machine_branches.spriteTornDownAt (migration 0207): teardown kills the Sprite and stamps the row. "Sprite is live" is now spriteTornDownAt IS NULL rather than row existence, which is what the reconciler's candidate query and the hard-purge guard both key on.
Two consequences worth calling out, both now pinned by tests:
- The hard-purge guard had to change with it. Blocking on row existence would have made every torn-down Machine permanently unpurgeable — silently breaking the 30-day Art. 17 retention promise. It now blocks only on unstamped rows (
purge-trashed-pages-guard.integration.test.ts, real Postgres). updateSandboxId(the sole re-provision write path) must clear the stamp, or a brand-new Sprite would be invisible to both the reconciler and the purge guard — i.e. orphaned and billed forever, the exact bug this PR exists to fix. Locked in by a new test inmachine-branches.test.ts.
machine_sessions still deletes its row on a confirmed kill, deliberately: it is a pure live-Sprite pointer, and the storage reconcile bills every row it finds, so a row outliving its Sprite would bill storage for a destroyed VM.
|
|
||
| for (const row of rows) { | ||
| try { | ||
| const killed = await deps.killSprite(row.sandboxId); |
There was a problem hiding this comment.
If a user restores a trashed Machine while this cron batch is running, candidates are selected once before the loop, while the restore route independently flips pages.isTrashed back to false. A restore that commits after listOrphanCandidates() but before this line still has its Sprite killed and its tracking row removed, leaving a restored Machine without the live session/branch it should have kept. Recheck or claim the row with pages.isTrashed = true immediately before killing/removing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed and fixed in 7a02db6. This is the one genuinely irreversible mistake the cron could make — killing a restored Machine's Sprite destroys that VM's filesystem — so I did both things you suggested rather than picking one.
- Recheck: a new
isStillTrashed(pageId)dep re-reads the owning page's trash state immediately before the kill. A restore that landed sincelistOrphanCandidates()skips the row entirely. - Claim: a recheck alone still leaves a window between the check and the write, so both release writes are now compare-and-swaps, conditional on the page still being trashed and
sandboxIdstill being the one we just killed (owningPageStillTrashed(...)inmachine-orphan-reconcile-runtime.ts). This closes a second race the recheck does not: a concurrentspawnBranchre-provision can CAS a live replacement Sprite into the same branch row, and marking that row torn-down would hide a live Sprite from both this cron and the hard-purge guard — orphaning it permanently.
Both outcomes are counted as skipped (new field on the result, surfaced in the cron response and audit details) rather than silently folded into success. A lost race self-heals: the row keeps pointing at a dead sandboxId, and the next attach re-provisions it via spawnBranch's vanished path.
Covered by two new unit tests in machine-orphan-reconcile.test.ts: NEVER kills the Sprite of a page restored since the candidate list was read, and counts a CAS that loses to a concurrent restore/re-provision as skipped, not torn down.
…rd the restore race Addresses both Codex review findings on #2070. P1 — deleting a machine_branches row on a REVERSIBLE soft-delete destroyed user config: the row is re-creatable (spawnBranch re-provisions a vanished branch under the same sessionKey and re-clones), and its branch-scoped machine_agent_terminals FK-cascade off it, so a restore came back without its branch terminals. Teardown now STAMPS a new machine_branches .spriteTornDownAt column and keeps the row. "Live Sprite" is therefore `spriteTornDownAt IS NULL`, not row existence — and the hard-purge guard only blocks on unstamped rows, so a reclaimed Machine stays purgeable (a row-existence guard would have made it unpurgeable forever, breaking the 30-day Art. 17 retention promise). P2 — a restore committing mid-run could have its LIVE Sprite killed. Two guards: isStillTrashed is re-read immediately before each kill, and both release writes are now CAS (page still trashed AND sandboxId unchanged), so a restore or concurrent re-provision can never have a live Sprite recorded as dead. New `skipped` count surfaces both. updateSandboxId (the sole re-provision write path) clears the stamp, or a fresh Sprite would be invisible to the reconciler and the purge guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
The candidate query and both compare-and-swap release writes — the heart of the orphan reconcile and of the restore-race fix — were only exercised against fakes, so a wrong exists(...) subquery or a dropped WHERE clause would have sailed through the unit tests and silently broken the cron in production. Adds a real-DB sibling covering: candidates are only Sprites believed LIVE under a TRASHED page (never a live page's, never an already-stamped branch row); isStillTrashed; and that both CAS writes REFUSE to fire once the page has been restored or the sandboxId changed under us (a concurrent re-provision), so a live Sprite can never be recorded as dead. Mutation-checked: dropping the still-trashed condition from either write fails exactly the two restore tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
The comment still described the old row-deletion design. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
…tamp at that layer teardownOneMachine selected every branch row, so a trash -> restore -> trash cycle re-killed Sprites already confirmed dead (a wasted API round-trip per row). It now selects only rows whose Sprite is still believed live (spriteTornDownAt IS NULL) — the same signal the reconciler uses. Adds tests pinning the P1 fix at this layer: a confirmed kill STAMPS the branch row (never deletes it), and a FAILED kill leaves it unstamped so the reconciler can still find the orphaned sandboxId and retry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
… classification Three orphan/data-loss defects found by adversarial review of the reconciler. 1) DATA LOSS (introduced by this PR). The reconciler keyed on `pages.isTrashed` alone, but pageService.trashPage — the generic page DELETE, bulk delete and folder cascade-trash — trashes a MACHINE page with NO teardown, and that trash is REVERSIBLE. A host.kill is an irreversible DESTROY, so the cron would have wiped the disk (repos, uncommitted work, credentials) of every Machine anyone merely dragged to the trash, within 30 minutes. Reclaiming now requires one of two tiers: teardownRequestedAt IS NOT NULL (a new column on both tracking tables, stamped by teardownOneMachine BEFORE it kills, so a crash mid-teardown still leaves the row reclaimable), or the page being past the hard-purge cutoff — at which point it is beyond restore and about to be erased, so leaving the Sprite alive would strand it forever when the FK cascade takes its pointer. TRASH_RETENTION_MS is now shared with the purge so the two cannot drift. 2) `kill()` treated ANY isSpriteNotFoundError as success — including ENOTFOUND (a DNS failure) and loose message matches. That classifier is calibrated for the READ path, where a false positive costs a redundant provision. On the destroy path a false positive is destructive: one DNS blip during a cron run would report every kill in the batch as confirmed, and the reconciler would release the only pointer to every one of those still-running Sprites. kill() now accepts only an authoritative 404/410 (isSpriteGoneStatus). 3) Two post-kill row deletes were not compare-and-swaps: teardownOneMachine's session removal (sessionKey is deterministic and save() upserts on it) and killBranch's name-keyed removal. A concurrent re-provision landing between the kill and the delete would have its brand-new LIVE Sprite's row destroyed — leaving it billing with no pointer at all. Both now CAS on sandboxId via new removeIfSandbox store methods. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
The reconciler kills serially, one network round-trip per row, inside a cron request — so an unbounded batch (a drive with hundreds of Machines trashed at once) would run until the request timed out mid-flight and repeat the same work every tick, never draining. Caps each table's candidate query at 200, oldest-trashed first (longest-billing orphans go first; nothing starves), and surfaces `capped` in the result, log and audit details: a silent truncation would read exactly like "nothing left to reclaim" while Sprites kept billing. Also closes three test gaps found in review: a page whose session row must still be released when one of its OWN branch kills fails; capped reporting; and an end-to-end idempotence test that runs the reconciler TWICE against a real Postgres (faking only the sprite kill) to evidence the "no advisory lock needed" decision — the second sweep finds nothing left, and the branch row survives stamped rather than deleted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
Adversarial review pass — findings + one open scope questionI ran two independent adversarial reviews over the diff. They found five real issues; four are fixed in Fixed
Open question — three hard-delete paths still bypass the guardThe invariant this PR establishes ("never destroy the only pointer to a Sprite that may be live") is enforced in
For the two trash routes the fix is small and reuses what's already here (tear down first, then delete). The erasure path needs a real decision — block-and-retry isn't acceptable for Art. 17, so it likely wants either a teardown-first pass or a small Happy to do any of it in this PR or a follow-up — your call on scope. |
…ng N delete paths
Answers "what is the objectively right architecture" — the guard was a patch on
one path, and review found three more (permanent delete from trash, permanent
drive delete, GDPR account erasure) that cascade the tracking rows away with no
teardown. Guarding each is unenforceable (there is always one more), and it
cannot work for erasure at all: Art. 17 must never be blocked by a Sprite we
failed to kill.
The real invariant is that the POINTER must outlive the RESOURCE. Today it is the
opposite: machine_sessions/machine_branches FK-cascade off pages.id (and
users.id), so destroying a page destroys the only way to find a running VM.
So the fix goes where the pointer actually dies — the database. A new
machine_sprite_reclaims outbox (no foreign keys, so nothing can cascade IT away)
plus AFTER DELETE triggers on both tracking tables (migration 0210). Postgres
fires row triggers for CASCADE-deleted rows too, so the sandboxId is rescued
inside the deleting transaction no matter which path started it — the 30-day
purge, "delete permanently", a drive delete, account erasure, or a hand-run
DELETE FROM pages. The reconciler drains the outbox with the idempotent kill and
drops a row only once its Sprite is CONFIRMED gone.
Consequences, all simplifications:
• The hard-purge guard is GONE (page-repository is back to master's version).
It is unnecessary now, and it carried its own Art. 17 hazard: a page whose
Sprite could not be killed would have been unpurgeable forever.
• So is the reclaim-vs-retention cutoff coupling. A merely-trashed Machine now
keeps its hibernating disk until its page is actually purged — at which point
the trigger routes its Sprite through the outbox.
• Deletes that follow a CONFIRMED kill drop the rescued pointer in the same
transaction, so the self-healing costs no redundant kill.
Proven against real Postgres, once per destroying path: deleting a PAGE, a DRIVE,
or a USER cascades the tracking rows away and every sandboxId lands in the
outbox; an already-reclaimed branch is not re-enqueued; the reconciler drains it,
is idempotent across runs, and KEEPS a row (recording attempts/lastError) when
the kill fails, because it is the last pointer in existence.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
Re: the open scope question — I rearchitected it insteadYou asked what the objectively right fix is. Having thought it through: neither option I offered was right. Both were variations on "remember to guard the delete," and that is the wrong shape. The real invariant is that the pointer must outlive the resource. Right now it's the exact opposite — So the fix now lives where the pointer actually dies — in the database:
All four paths are now safe, and none of them had to change — including the account-erasure worker, which is exactly the one a guard could never have protected. The same is true of the path nobody has written yet, and of a hand-run It also deleted code: the hard-purge guard is gone ( Proven against real Postgres, once per destroying path — deleting a page, a drive, or a user cascades the tracking rows away and every One deliberate non-change worth your eye: a Machine merely dragged to the trash is left alone. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/lib/machines/machine-orphan-reconcile-runtime.ts`:
- Around line 73-127: Update the candidate queries in the orphan reconciliation
flow to fetch MAX_CANDIDATES_PER_TABLE + 1 rows, then trim each result to the
first MAX_CANDIDATES_PER_TABLE rows before mapping them into the returned rows.
In the capped calculation, check whether any untrimmed source result contains
more than MAX_CANDIDATES_PER_TABLE rows rather than using length equality.
In `@packages/db/drizzle/0210_sprite_reclaim_triggers.sql`:
- Around line 27-41: Update safeRemove() in
packages/lib/src/services/sandbox/machine-session-manager.ts:192-203 to use
removeIfSandbox({ sessionKey, sandboxId: plan.sandboxId }) instead of deleting
by sessionKey alone, preserving the CAS protection during teardown. The trigger
in packages/db/drizzle/0210_sprite_reclaim_triggers.sql:27-41 requires no direct
change; it exposes the reclaim behavior affected by the unsafe delete.
In `@packages/lib/src/services/machines/machine-orphan-reconcile.ts`:
- Around line 183-198: Update the failed-kill handling in the orphan
reconciliation flow so a rejection from deps.noteReclaimFailure does not escape
and increment failed again. Isolate metadata recording, preserve the single
failed increment per candidate, and always log the original killed.error through
the existing teardown error path. Add a regression test covering a rejecting
noteReclaimFailure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c750b0a-7ec3-4cfe-a220-5ae86611b35d
📒 Files selected for processing (34)
apps/web/src/app/api/cron/reconcile-orphaned-sprites/__tests__/route.test.tsapps/web/src/app/api/cron/reconcile-orphaned-sprites/route.tsapps/web/src/lib/machines/__tests__/machine-orphan-reconcile-runtime.integration.test.tsapps/web/src/lib/machines/__tests__/machine-settings-runtime.test.tsapps/web/src/lib/machines/machine-branches-runtime.tsapps/web/src/lib/machines/machine-orphan-reconcile-runtime.tsapps/web/src/lib/machines/machine-settings-runtime.tsdocker/cron/crontabpackages/db/drizzle/0207_wide_gargoyle.sqlpackages/db/drizzle/0208_material_thor.sqlpackages/db/drizzle/0209_perfect_lady_ursula.sqlpackages/db/drizzle/0210_sprite_reclaim_triggers.sqlpackages/db/drizzle/meta/0207_snapshot.jsonpackages/db/drizzle/meta/0208_snapshot.jsonpackages/db/drizzle/meta/0209_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/package.jsonpackages/db/src/schema.tspackages/db/src/schema/machine-branches.tspackages/db/src/schema/machine-sessions.tspackages/db/src/schema/machine-sprite-reclaims.tspackages/lib/package.jsonpackages/lib/src/services/machines/__tests__/machine-branches.test.tspackages/lib/src/services/machines/__tests__/machine-orphan-reconcile.test.tspackages/lib/src/services/machines/machine-branches-store.tspackages/lib/src/services/machines/machine-branches.tspackages/lib/src/services/machines/machine-orphan-reconcile.tspackages/lib/src/services/sandbox/__tests__/machine-session-manager.test.tspackages/lib/src/services/sandbox/__tests__/machine-session.test.tspackages/lib/src/services/sandbox/__tests__/persistent-machine-fs.test.tspackages/lib/src/services/sandbox/machine-session-manager.tspackages/lib/src/services/sandbox/sandbox-client/__tests__/sprite-machine-host.test.tspackages/lib/src/services/sandbox/sandbox-client/sprite-machine-host.tspackages/lib/src/services/sandbox/sandbox-client/sprites.ts
Adversarial review of the outbox design found that `sandboxId` is not an identity at all: it is `sprite.name` — our own HMAC session key — and the SDK resume-or-creates UNDER that name. The codebase already knew this (the egress token is keyed on the instance id precisely because "a Sprite destroyed and re-created under the same name is a different VM"), but the teardown path did not. Three consequences, each of which recreates the bug this PR exists to fix: 1) Every CAS in the previous commits was a no-op against the very race it documented. A replacement Sprite provisioned under the same session key has the SAME sandboxId, so `WHERE sandboxId = ...` passes and we delete the pointer to a LIVE VM. Concretely: deleteMachine kills, an attached terminal re-acquires and upserts a brand-new live VM into the same row, and removeIfSandbox then drops both that row and its rescued outbox pointer. Live VM, zero pointers — the production orphan, reproduced by the fix. 2) The kill itself is name-keyed (`deleteSprite(name)`), so an outbox row said "destroy whatever holds this name" — which would destroy a replacement VM that legitimately took the name after our target was already gone. 3) The branch trigger's `spriteTornDownAt IS NULL` skip could skip a row whose Sprite was alive (stamped after a name-CAS'd "confirmed" kill that a re-provision had already replaced). So identity is now the instance id: persisted on both tracking tables and rescued into the outbox (0211/0212), every CAS keys on it, and `MachineHost.kill` takes an `expectedInstanceId` — it reads who actually holds the name first and treats "a different VM lives here now" as success (our target is already gone) rather than destroying the newcomer. Also fixes, from the same review: - `teardownRequestedAt` was never cleared, so a stale intent (delete → restore → re-provision) would turn a LATER, reversible trash into an irreversible kill of a live VM. Cleared on every live-Sprite write, plus the reconciler now requires the intent to postdate the trash it is licensing. - spawnBranch leaked a live Sprite with NO row at all on any non-unique-violation create failure (connection blip, aborted tx) — nothing to delete, so no trigger, no outbox, no reconciler: an immortal orphan. It is now killed. The ABA is pinned by a real-Postgres test, mutation-checked: with the old name-only CAS the release fires and the live replacement loses its last pointer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
…sifier Closes the last two review findings. The triggers now sit on the critical path of every page/drive/user delete, including Art. 17 erasure — so they run SECURITY DEFINER with a pinned search_path. As SECURITY INVOKER, any role that may DELETE from `pages` but lacks INSERT on the outbox would have its delete FAIL, which is precisely the outcome this design exists to prevent (erasure blocked by a Sprite we could not kill). The zero-trust/least-privilege workstream would have hit this. `isSpriteGoneStatus` is now pinned against the SDK's REAL error, constructed with its own parseAPIError: the classifier is status-only (no message heuristics), which is right for a destroy path but useless if the SDK's 404 carried no numeric status — nothing would ever classify as gone, the outbox would never drain, and the "cannot be killed" alarm would fire on Sprites that are already dead. Verified: deleteSprite throws APIError with statusCode. A future SDK bump that changes that shape now fails loudly here instead of silently disabling the drain. Also asserts both triggers exist in pg_trigger. They are hand-written SQL and invisible to drizzle's schema diff, so a future migration that recreates either tracking table would drop them silently — and every other test would still pass while production went back to stranding billing VMs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
Third adversarial review — found the design's own hidden assumptionI had the new architecture reviewed adversarially. It found a critical false assumption underneath everything I'd built, and the fix changes the identity model. All findings are now addressed; details for the record. 🔴
|
The 'teardown' lifecycle branch stopped the VM and then did a key-only store.remove before immediately re-provisioning under the SAME session key. A name is reused across re-creates, so that delete could remove a row the re-provision (or a concurrent acquire) had already pointed at a NEW, live VM — orphaning it with no pointer at all. Unreachable today (nothing passes intent: 'end'), but it is the same landmine the other three removals just had, and it sits one flag away from being armed. Now CASes on the instance it actually stopped; losing the CAS is the correct outcome, since whoever owns the row owns a live Sprite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
…unts once Two review findings from CodeRabbit. An exactly-full result proved nothing about a 201st candidate, so `capped` cried wolf on every exactly-full sweep. Each source now fetches one row PAST the cap purely to learn whether a backlog remains; only a row beyond it reports one. If noteReclaimFailure itself threw, the row was counted failed twice and the kill error we actually came to report was lost. The bookkeeping write is now isolated: the row counts once, and the Sprite is retried next run either way. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
0210 created the reclaim triggers and 0212 immediately dropped and recreated them (to carry the Sprite instance id) — so a fresh database installed a trigger only to replace it a moment later, and a reviewer had to read both to know what actually ships. Neither is merged, and custom SQL migrations carry no drizzle snapshot, so folding them is safe. One migration now installs the final triggers and runs the backfill. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
Adversarial review of the identity rework found two coupled defects: one made the whole session-side guard inert in production, the other made it destructive the moment the first was fixed. Neither could ship alone. 1) adaptMachineHandleToExecutableSandbox DROPPED spriteInstanceId. That adapter is the production session client, so every machine_sessions row stored NULL, every CAS fell back to comparing the (reused) sprite NAME, and kill() skipped the identity check entirely — the ABA protection was inert while typechecking clean. The field was optional, which is what let the drop compile; it is now REQUIRED (nullable), so a future drop is a compile error, and the fake client that hid this carries it too. 2) The reconnect path re-provisions a vanished Sprite under the same name (getOrCreate) — a NEW VM with the same sandboxId — and only touched the row, never recording the new instance. The row then named a dead VM while a live one held the name. With guard (1) working, teardown would ask to kill the dead id, the host would rightly decline, report success, and the CAS — comparing against the stale id the row still held — would MATCH and delete the row AND its rescued outbox pointer. A live VM, billing forever, with nothing pointing at it: the exact orphan this workstream exists to kill, manufactured by its own fix. touch() now records a moved instance (and voids the stale teardown intent with it). Belt to that brace: a kill whose target has been REPLACED now throws (MachineSpriteReplacedError) instead of reporting success. Callers treat success as proof of death and release the last pointer, so any residual staleness must surface as a retry, never as a dropped pointer. Both are pinned by mutation-checked tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
Master took migration slot 0207, so this branch's five migrations were rebased onto its chain and regenerated as two: one schema migration (0208) carrying every column and the reclaim outbox table, and one hand-written trigger migration (0209). The schema TypeScript is the source of truth and did not change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
Fourth review round — the identity rework had two coupled P0s (fixed)An adversarial pass over the instance-id change (which was itself new) found a pair of defects that only bite in production, and that had shipped while every test passed:
Both are pinned by mutation-checked tests: an adapter test that fails if the id is dropped, and a reconnect test that fails if the moved instance isn't persisted. Also this round: the six migrations were folded (0210 created the triggers only for 0212 to recreate them), and then rebased onto master — which had taken slot 0207 — so they are now one schema migration + one trigger migration on top of master's chain. Verified applying cleanly from an empty DB. Clean categories the reviewer confirmed: the CI re-running on the merged commit; will confirm green. |
…er-retry
MachineSpriteReplacedError means a different live VM holds the name now, so the
INSTANCE we were tracking is already gone — the outcome the reconciler wants. But
killSprite caught it as { ok: false }, so the outbox row was retried every tick
with a growing `attempts` against a target that no longer exists, and its pointer
could never be released. The live newcomer has its own fresh tracking row, so
releasing ours never orphans it.
killSprite now maps MachineSpriteReplacedError to { ok: true } (release); any other
error still reports failure (the target's fate is genuinely unknown, keep the
pointer). Pinned by a mutation-checked kill-map test, plus host-level tests that
the identity guard throws on a mismatch and kills on a match.
teardownOneMachine and killBranch deliberately keep reporting the replaced case as
a failure: there a different VM really is alive under the name, so spriteTornDown
=false / reason:'error' is the honest answer, and the reconciler is the backstop.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
Verification of the P0 fixes — plus one stuck-row bug found and fixedI had the two P0 fixes independently verified end-to-end, and traced the new Both fixes confirmed complete. Every production constructor of a One real bug the verification surfaced, now fixed (
CI re-running on |
…ffort kills
A /simplify pass (4 parallel cleanup agents over the diff) surfaced two
high-confidence, behaviour-preserving cleanups, plus a latent gap.
- The null-safe instance CAS predicate `id === null ? isNull(col) : eq(col, id)`
was copy-pasted at 5 sites. The rule it encodes — "two live VMs can share one
reused name, so match the INSTANCE" — is the crux the whole teardown workstream
enforces, and having it in 5 places means a future refinement (or a missed
site, exactly how the dropped-adapter-field bug happened once) silently
degrades the ABA guard. Extracted as `eqOrIsNull` in @pagespace/db/operators.
- `safeKillProvisionedSprite` (added by this PR) duplicated the file's existing
`safeKillSprite`. Merged them — and in doing so closed the one remaining
name-only kill: `reconcileProvisionCollision` was destroying its just-
provisioned redundant Sprite by name while holding the handle with its
instance id, the same ABA window this PR hardens everywhere else. safeKillSprite
now takes the handle and passes expectedInstanceId at both call sites.
Deliberately NOT changed (considered, rejected): the removeIfSandbox transaction
shape recurs in two lazily-db-importing store modules — real cross-module
friction for ~6 lines, so left as-is. And MachineSpriteReplacedError is
intentionally mapped to success in the reconciler but to failure in
teardownOneMachine/killBranch: those ask different questions ("is MY target
gone?" vs "is the machine's compute gone?"), and a live replacement genuinely IS
billing, so spriteTornDown=false is the honest answer — forcing uniformity would
make the delete result lie.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ
1 participant
Task 2 of 2 in the Sprites Idle-Cost Remediation epic.
Root cause
This month's Sprites bill was $50.85 of RAM against $0.99 of CPU for very little real usage. We found a live orphaned Sprite (
pgs-sbx-…) stuck inrunningwith zero matching row in either tracking table — unowned, unreachable from inside the app, silently billing RAM. We killed it by hand.Two mechanisms produced it:
deleteMachine()tears Sprites down best-effort and a failed kill was documented as reclaimable by a reconciler that did not exist; and both tracking tables FK-cascade offpages.id(andusers.id), so any hard delete of a page destroys the only record of a VM that may still be running.The architecture
Guarding the purge was the wrong shape — review found three more paths that cascade the pointer away with no teardown (permanent delete, drive delete, GDPR account erasure), and guarding each is unenforceable (and impossible for erasure). The real invariant is that the pointer must outlive the resource, so the fix lives in the database:
machine_sprite_reclaims— an outbox with no foreign keys, so nothing can cascade it away.AFTER DELETEtriggers on both tracking tables (SECURITY DEFINER, pinnedsearch_path). Postgres fires row triggers for FK-cascade deletes, so the pointer is rescued inside the deleting transaction — whichever path started it, including one nobody has written yet. Either the pointer moves, or the delete doesn't commit.Every delete path is safe by construction, and none had to change. It also deleted code: the hard-purge guard is gone (
page-repository.tsis back to master).Identity: the instance, not the name
sandboxIdis not an identity — it issprite.name, an HMAC of (tenant, drive, page/branch), and the SDK resume-or-creates under that name, so a re-provisioned Sprite answers to the samesandboxIdwhile being a different VM. That made every compare-and-swap a no-op against the race it documented and made the name-keyed kill able to destroy a replacement VM.Identity is now the Sprite instance id:
spriteInstanceId, persisted on both tables and the outbox, keyed on by every CAS, and required-nullable (so the compiler catches a drop).MachineHost.killtakes anexpectedInstanceIdand — if a different VM holds the name — throws (MachineSpriteReplacedError) rather than destroying the newcomer or falsely reporting success. Two follow-on P0s from this rework are also fixed: the production adapter had been dropping the id (every session row was NULL → the guard inert), and the reconnect path re-provisioned a VM without recording its new id (a stale id would let teardown delete a live VM's last pointer). Both pinned by mutation-checked tests.What the reconciler reclaims
(A) The outbox — pointers whose page is gone. Kill. (B) Tracking rows under a trashed page whose teardown was requested (
teardownRequestedAt, stamped before the kill; must postdate the trash). Never a Machine merely dragged to the trash: a kill is irreversible, a trash is reversible, andpageService.trashPagetears down nothing — an earlier draft keyed onisTrashedalone and would have wiped the disk of every trashed Machine within 30 minutes.Other fixes from review
kill()no longer swallows transport errors (ENOTFOUND) as "already gone" — only an authoritative 404/410, pinned against the SDK's realAPIError. A staleteardownRequestedAtcan't turn a later reversible trash into a kill.spawnBranchno longer leaks an immortal Sprite on a non-unique-violation DB failure. Branch rows are stamped, not deleted (they're re-creatable config; their agent terminals cascade off them). The batch is capped (200/source, oldest-first, one-row lookahead for an honestcappedflag). A failed bookkeeping write counts a row once.Verification
The load-bearing claim — the pointer survives every way its page can die — is proven against real Postgres, once per path (page / drive / user delete → rescued; already-reclaimed branch → not re-enqueued; both triggers asserted present in
pg_trigger). Plus, all real-DB: the ABA (replacement Sprite, same name) is refused by the CAS (mutation-checked); the reconciler drains the outbox and is idempotent across runs (the evidence behind "no advisory lock"); a failed kill keeps its row and recordsattempts/lastError; teardown records intent before killing; the adapter carries the instance id and the reconnect persists a moved one (both mutation-checked).bun run lint(14/14) andtsc --noEmitpass forpackages/db,packages/lib,apps/web,apps/realtime. Migrations (0208schema +0209triggers, rebased onto master's chain) apply cleanly from an empty database. Full suites: packages/lib 8,251 passing, apps/web 13,389 passing, with 2 failures (gate-callsites.guard,grouping) that reproduce identically on a clean master tree — pre-existing and unrelated.Known gaps (documented, not fixed here)
resolveScopeLocationhands it to the PTY; only add branch re-provisions. Pre-existing in shape, systematic now, and it finally has the signal needed to fix it.spawnBranchhas noisTrashedguard, so spawning a branch on a trashed Machine creates a live VM under a trashed page — bounded (the purge trigger routes it to the outbox), worth a separate guard.🤖 Generated with Claude Code
https://claude.ai/code/session_01AiXs9DZgobznUMJFtPrxeJ