Repository navigation
fix(agent-sessions): storage-measurement generation CAS + reclaim-outbox instance chase - #2274
Conversation
…box instance chase Re-lands the generation-CAS half of #2253 post-restore, adapted to current master's session-keyed model (spriteInstanceId, not sandboxId/name — the name is HMAC-derived and identical across generations). - recordStorageMeasurement now CASes on spriteInstanceId (eq-or-is-null), so a du against a torn-down generation cannot land its bytes/timestamp on the next generation's row. Required field threaded through the pure measure module to every call site (web create+warm, realtime create+resume); in-memory fakes honour the CAS. - Every measurement call site sources spriteInstanceId from the ACQUIRED sandbox handle, never a re-read of the session row, so a torn-down-and- reprovisioned session in flight cannot have generation A's bytes CAS "successfully" onto generation B. Fixes #2254: the orphan reconciler treated SandboxSpriteReplacedError as a confirmed kill for reclaim-outbox rows too, deleting the outbox pointer and orphaning a live Sprite when the outbox held a stale instance id (the 0234 rescue's ON CONFLICT DO NOTHING vs. the triggers' DO UPDATE COALESCE instance-chasing). killSprite now reports the replacement distinctly; the pure reconcile loop chases a reclaim row's pointer at the live instance (mirroring the trigger's own COALESCE chase) instead of releasing it, while an agent-session row's own identity CAS still lets a replaced instance count as confirmed-gone. Closes #2254. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DrK6Xpi8fu6jtyLxNqnTAt
![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: f5a60ce39b
ℹ️ 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".
| // chase the pointer at the live instance and retry next tick, rather | ||
| // than confirming a kill that never touched the VM actually running. | ||
| try { | ||
| await deps.chaseReclaimInstance({ sandboxId: row.sandboxId, actualInstanceId: killed.actualInstanceId }); |
There was a problem hiding this comment.
A reclaim row is not necessarily left by a deleted session: killUnreferencedOrEnqueue in agent-session-sprite.ts also queues an unrecorded generation after a failed cleanup while the session row survives. If that stale entry names instance A and the session later provisions tracked instance B under the same deterministic sandbox name, this branch rewrites the outbox to B; the next cron tick treats B as orphaned and kills the active session without the session liveness check used by agent-session candidates. Before chasing the replacement, verify that actualInstanceId is not referenced by a live session row; if it is, the stale reclaim entry should be released instead.
Useful? React with 👍 / 👎.
| { sandboxId: row.sandboxId, actualInstanceId: killed.actualInstanceId }, | ||
| ); | ||
| } | ||
| skipped += 1; |
There was a problem hiding this comment.
When chaseReclaimInstance throws, the stale pointer remains unable to target the live Sprite, but execution still increments skipped and reports failed: 0. The cron route publishes these counters to its log and audit record, so a persistent database failure here appears as a benign skip even while the orphan continues billing. Increment failed rather than skipped when the chase write fails, while retaining skipped for a successful chase that intentionally waits for the next tick.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughStorage measurements now carry ChangesGeneration-aware storage measurements
Replaced Sprite reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant reconcileOrphanSprites
participant killSprite
participant chaseReclaimInstance
participant enqueueReclaim
reconcileOrphanSprites->>killSprite: kill orphan candidate
killSprite-->>reconcileOrphanSprites: replaced with actualInstanceId
reconcileOrphanSprites->>chaseReclaimInstance: chase reclaim pointer
chaseReclaimInstance->>enqueueReclaim: enqueue actualInstanceId
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
Same correction as the PR description — #2274 is an unrelated, already-merged PR (storage-measurement CAS), not this fix's issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5FJqcnWyZXBmuc9iTFd6U
Same correction as the PR description — #2274 is an unrelated, already-merged PR (storage-measurement CAS), not this fix's issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5FJqcnWyZXBmuc9iTFd6U
Same correction as the PR description — #2274 is an unrelated, already-merged PR (storage-measurement CAS), not this fix's issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5FJqcnWyZXBmuc9iTFd6U
Same correction as the PR description — #2274 is an unrelated, already-merged PR (storage-measurement CAS), not this fix's issue. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H5FJqcnWyZXBmuc9iTFd6U
1 participant
Summary
Wave-2 of the post-#2258 audit: re-lands the generation-CAS half of #2253 (pre-restore, cannot merge as-is) onto current master's session-keyed model, and fixes #2254.
1. Storage-measurement generation CAS (#2253 fix 2)
recordStorageMeasurementwrites now CAS onspriteInstanceId(notsandboxId— that name is HMAC-derived from the session and identical across generations) viaeqOrIsNull, so a driver reporting no instance id still matches its own null row. Without this, aduagainst a torn-down generation that completes after re-provisioning could land its bytes andstorageMeasuredAton the new generation's row, silencing the new generation's real measurement for a full throttle window while the reconcile bills its interval against the wrong disk.The field is now required through
refreshSessionStorageMeasurement/PersistSessionStorageMeasurementso a call site can't silently drop it, threaded to all four call sites: web create + warm (agent-sessions-runtime.ts,sandbox-tools-runtime.ts), realtime create + resume (apps/realtime/src/index.ts). The in-memory store fake honours the same CAS as the real store.2. Handle-sourced generation id (#2253 fix 3)
Every measurement call site now sources
spriteInstanceIdfrom the acquired sandbox handle — the identity of the disk actually measured — never from a re-read of the session row, which can name a different generation than the oneduwalked (the row is read only for the throttle check).3. Issue #2254 — reclaim-outbox stale instance pointer
agent-session-orphan-reconcile-runtime.tstreatedSandboxSpriteReplacedErroras{ ok: true }unconditionally, deleting the outbox row — but for areclaimrow (whose outbox entry is the last pointer to whatever VM exists under that name), a stale instance id there orphans the live replacement forever.killSpritenow reports a replaced instance distinctly ({ ok: 'replaced', actualInstanceId }) instead of collapsing it to success. The purereconcileOrphanSpritesloop decides perrow.kind:reclaimrows: chase the pointer at the live instance viachaseReclaimInstance(reuses the store's ownenqueueReclaimupsert — the sameON CONFLICT DO UPDATE ... COALESCEthe 0209/0219/0229 triggers use) and retry next tick, rather than releasing.agent-sessionrows: unaffected — the row's own identity CAS inmarkSessionTornDownalready protects a live replacement, so a replaced instance still counts as confirmed-gone.Closes #2254. PR #2253 can now be closed (superseded by this PR, adapted to the current session-keyed model).
Test plan
bun run typecheck && bun run lint && bun run test:unit && bun run knip:check— clean (one pre-existing, unrelated TZ-environment test failure inmessages/grouping.test.ts, present on master, untouched by this diff).cd packages/lib && bun run typecheck— clean.bun run test:security— 44/50 suites pass; the same 6 pre-existing failures as master (deleted Login/Signup/Mobile-Login routes, excluded Session Service/Device Auth/Permissions suites — a documented script gap, not a regression).🤖 Generated with Claude Code
https://claude.ai/code/session_01DrK6Xpi8fu6jtyLxNqnTAt
Summary by CodeRabbit
Bug Fixes
Tests