Found by Codex reviewing #2249 after it merged; verified against the source here. Filing because the fix needs a decision I shouldn't make unilaterally, and the finding would otherwise live only in a PR comment.
The gap
0234_phase8_teardown_machines_world.sql rescues live Sprite pointers into the reclaim outbox with four INSERT … ON CONFLICT ("sandboxId") DO NOTHING statements.
The row-level triggers that feed the same outbox do the opposite, deliberately — 0219_project_sprite_reclaim_trigger.sql:
ON CONFLICT ("sandboxId") DO UPDATE
-- A newer generation took this name; the pointer must chase the VM that
-- is actually alive now, not the one a stale row remembers.
SET "spriteInstanceId" = COALESCE(EXCLUDED."spriteInstanceId", public.machine_sprite_reclaims."spriteInstanceId");
sandboxId is the HMAC-derived name and is reused across generations, so the conflict is exactly the case where the instance id matters. The rescue keeps whatever the outbox already had.
Why that orphans a VM
-
Outbox already holds (X, instance-A) — e.g. from an earlier trigger firing.
-
The live machine_* row for the same name holds instance-B.
-
The migration's rescue hits the conflict, DO NOTHING → outbox still says instance-A.
-
The migration drops the machine_* tables → B's tracking row is gone. The outbox row is now B's only pointer, and it names A.
-
sprite-orphan-reconcile calls killSprite({sandboxId: X, spriteInstanceId: 'instance-A'}). The host reads who actually lives at that name, sees B, and throws SandboxSpriteReplacedError — correctly, per its own comment:
THROW rather than return success: "success" means "confirmed destroyed", and every caller acts on it by releasing the Sprite's last pointer … Refusing keeps the pointer and surfaces the staleness as a retry, which is the safe way to be wrong.
-
But apps/web/src/lib/agent-sessions/agent-session-orphan-reconcile-runtime.ts:132 does:
if (error instanceof SandboxSpriteReplacedError) return { ok: true };
→ the reconciler treats it as a confirmed kill and deletes the outbox row. Sprite B now has no pointer anywhere. It bills forever, invisible.
Why this isn't a one-line fix
The migration is already on master (623e632e7), so amending it is off the table — and per CLAUDE.md we don't edit SQL under packages/db/drizzle/ regardless. That leaves two options, and they differ in blast radius:
(a) Corrective migration. Can't fully work: the machine_* tables are already dropped, so the correct instance ids are no longer readable from SQL. It could only re-point rows whose truth still exists elsewhere.
(b) Harden the reconciler. The {ok: true} is right for agent-session rows — the session row's own CAS still protects the live VM — but wrong for reclaim rows, where the outbox entry is the last pointer. For those, SandboxSpriteReplacedError should not delete the row. The machinery for this already exists: noteReclaimFailure records an attempt count precisely so that "a Sprite that cannot be killed surfaces as a growing attempt count instead of being retried silently forever."
(b) matches the host's documented intent and needs no migration. The tradeoff is that genuinely-dead outbox rows whose name was legitimately reused would linger and retry — visible via the attempt counter rather than silently dropped, which seems like the correct direction, but it is a change to production cron behaviour on merged code.
Suggested
Option (b), scoped to row.kind === 'reclaim', with a test covering: outbox row names generation A, live VM is B → row retained, attempt count incremented, VM not killed.
Raised from the same review pass as #2253.
Found by Codex reviewing #2249 after it merged; verified against the source here. Filing because the fix needs a decision I shouldn't make unilaterally, and the finding would otherwise live only in a PR comment.
The gap
0234_phase8_teardown_machines_world.sqlrescues live Sprite pointers into the reclaim outbox with fourINSERT … ON CONFLICT ("sandboxId") DO NOTHINGstatements.The row-level triggers that feed the same outbox do the opposite, deliberately —
0219_project_sprite_reclaim_trigger.sql:sandboxIdis the HMAC-derived name and is reused across generations, so the conflict is exactly the case where the instance id matters. The rescue keeps whatever the outbox already had.Why that orphans a VM
Outbox already holds
(X, instance-A)— e.g. from an earlier trigger firing.The live
machine_*row for the same name holdsinstance-B.The migration's rescue hits the conflict,
DO NOTHING→ outbox still saysinstance-A.The migration drops the
machine_*tables → B's tracking row is gone. The outbox row is now B's only pointer, and it names A.sprite-orphan-reconcilecallskillSprite({sandboxId: X, spriteInstanceId: 'instance-A'}). The host reads who actually lives at that name, sees B, and throwsSandboxSpriteReplacedError— correctly, per its own comment:But
apps/web/src/lib/agent-sessions/agent-session-orphan-reconcile-runtime.ts:132does:→ the reconciler treats it as a confirmed kill and deletes the outbox row. Sprite B now has no pointer anywhere. It bills forever, invisible.
Why this isn't a one-line fix
The migration is already on master (
623e632e7), so amending it is off the table — and perCLAUDE.mdwe don't edit SQL underpackages/db/drizzle/regardless. That leaves two options, and they differ in blast radius:(a) Corrective migration. Can't fully work: the
machine_*tables are already dropped, so the correct instance ids are no longer readable from SQL. It could only re-point rows whose truth still exists elsewhere.(b) Harden the reconciler. The
{ok: true}is right foragent-sessionrows — the session row's own CAS still protects the live VM — but wrong forreclaimrows, where the outbox entry is the last pointer. For those,SandboxSpriteReplacedErrorshould not delete the row. The machinery for this already exists:noteReclaimFailurerecords an attempt count precisely so that "a Sprite that cannot be killed surfaces as a growing attempt count instead of being retried silently forever."(b) matches the host's documented intent and needs no migration. The tradeoff is that genuinely-dead outbox rows whose name was legitimately reused would linger and retry — visible via the attempt counter rather than silently dropped, which seems like the correct direction, but it is a change to production cron behaviour on merged code.
Suggested
Option (b), scoped to
row.kind === 'reclaim', with a test covering: outbox row names generation A, live VM is B → row retained, attempt count incremented, VM not killed.Raised from the same review pass as #2253.