[codex] Fix session creation D1 failure ordering#563
Conversation
📝 WalkthroughWalkthroughThis pull request reorders session creation operations in the handler to persist records to D1 before initializing SessionDO, ensuring database failures abort early. A new test suite verifies error handling and call ordering for this behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/control-plane/src/router.ts (1)
884-940:⚠️ Potential issue | 🟡 MinorOrphaned D1 row when SessionDO init fails after D1 write.
The reorder correctly prevents orphaned sandboxes when D1 fails, but the inverse case is now possible: if
stub.fetch(SessionInternalPaths.init)returns non-OK (line 938) or throws, the D1 row written at lines 887–902 withstatus: "created"is left behind. It will surface inhandleListSessions/the sidebar without a working DO, and the caller never receives thesessionIdso it cannot retry or clean up.For symmetry with the prompt-failure path in
handleSpawnChild(lines 1991, 2004 mark the child as"failed"), consider compensating on init failure—either delete the row or mark it as"failed"—so the user-visible session list stays consistent with reality.♻️ Suggested compensating cleanup on init failure
if (!initResponse.ok) { + // Compensate: mark the just-created D1 row so it doesn't appear as a live "created" session. + try { + await sessionStore.updateStatus(sessionId, "failed"); + } catch (e) { + logger.warn("Failed to mark session as failed after init failure", { + session_id: sessionId, + error: e instanceof Error ? e : String(e), + }); + } return error("Failed to create session", 500); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/control-plane/src/router.ts` around lines 884 - 940, The D1 row created by SessionIndexStore.create can be left orphaned if stub.fetch(buildSessionInternalUrl(SessionInternalPaths.init)) fails or returns non-OK; wrap the init call in a try/catch and on any failure (non-ok response or thrown error) perform a compensating update via the SessionIndexStore instance (sessionStore) to mark the session id as failed (e.g., set status: "failed" and updatedAt: Date.now()) or remove the row, then return the error—do this immediately around the await stub.fetch(...) block so sessionStore.create(...) (the call that created the row) is always reconciled on init failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/control-plane/src/router.ts`:
- Around line 884-940: The D1 row created by SessionIndexStore.create can be
left orphaned if stub.fetch(buildSessionInternalUrl(SessionInternalPaths.init))
fails or returns non-OK; wrap the init call in a try/catch and on any failure
(non-ok response or thrown error) perform a compensating update via the
SessionIndexStore instance (sessionStore) to mark the session id as failed
(e.g., set status: "failed" and updatedAt: Date.now()) or remove the row, then
return the error—do this immediately around the await stub.fetch(...) block so
sessionStore.create(...) (the call that created the row) is always reconciled on
init failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b55f0a38-098e-42b8-b5e2-132ea9086ace
📒 Files selected for processing (2)
packages/control-plane/src/router.create-session.test.tspackages/control-plane/src/router.ts
1 participant
Summary
Moves the
POST /sessionsD1 session-index write before SessionDO initialization. SessionDO init starts sandbox warming, so this makes D1 failures fail before any sandbox can be spawned.Adds a focused router regression test proving that a D1 session-index failure does not call
/internal/init, and that successful creates write D1 before initializing the SessionDO.Why
During D1 stalls, the previous order could initialize a SessionDO and start sandbox warming before the caller received a
sessionId. Bot flows send prompts in a second request afterPOST /sessionsreturns, so if the D1 write stalled or failed after DO init, a sandbox could boot without ever receiving the prompt.Validation
npm run build -w @open-inspect/sharednpm test -w @open-inspect/control-plane -- router.create-session.test.tsnpm run typecheck -w @open-inspect/control-planenpm run lint -w @open-inspect/control-planeSummary by CodeRabbit
Tests
Bug Fixes