refactor: extract SessionInitInput type and initializeSession#587
Open
ColeMurray wants to merge 1 commit intomainfrom
Open
refactor: extract SessionInitInput type and initializeSession#587ColeMurray wants to merge 1 commit intomainfrom
ColeMurray wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughA new ChangesSession Initialization Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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. Review rate limit: 6/8 reviews remaining, refill in 9 minutes and 28 seconds.Comment |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
| }) | ||
| ); | ||
|
|
||
| if (!initResponse.ok) { |
Contributor
There was a problem hiding this comment.
Writing the session-index row before DO init is fine, but on this error path we now leave that freshly inserted row behind. That is a regression for spawned children: handleListChildren() reads directly from D1, so a failed child init will show up as a phantom child stuck in created even though its DO never finished initializing. Could we compensate here by deleting the row or marking it failed before rethrowing?
Contributor
There was a problem hiding this comment.
Summary
This PR extracts the shared session-initialization flow into session/initialize.ts and adds focused unit coverage around the new helper. The refactor is directionally good, but it introduces a child-session correctness regression on the DO-init failure path.
PR Title and number: refactor: extract SessionInitInput type and initializeSession (#587)
Author: @ColeMurray
Files changed count, additions/deletions: 5 files, +390/-113
Critical Issues
- [Functionality]
packages/control-plane/src/session/initialize.ts:128-initializeSession()now inserts the session-index row before DO init and then throws on a non-OK init response without compensating the D1 write. For spawned children,handleListChildren()reads directly from D1, so a failed child init will now appear as a phantom child stuck increated. We should delete the row or mark it failed before propagating the error.
Suggestions
- None beyond the blocking issue above.
Nitpicks
- None.
Positive Feedback
- The extraction removes a large amount of duplicated init wiring from
router.tswhile keeping the create/spawn-specific preconditions local to each handler. - The new
initializeSessiontests cover ordering, failure handling, correlation headers, and branch fallback clearly. - Preserving the D1-first invariant for both call sites makes the init flow easier to reason about.
Questions
- None.
Verdict
Request Changes
Validation performed: focused control-plane tests passed for src/session/initialize.test.ts, src/session/contracts.test.ts, and src/session/http/handlers/session-lifecycle.handler.test.ts.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/control-plane/src/session/initialize.ts`:
- Around line 128-137: The DO init failure path currently just logs and throws,
leaving the earlier-persisted D1 session row orphaned; update the failure
handling around initResponse (in initialize.ts) to perform D1 compensation: call
the D1 delete/remove method for input.sessionId (or the existing session delete
helper) and await its completion before throwing, log success or any delete
error via logger.error/ logger.info (include session_id and trace_id), and if
the delete itself fails still throw the original initialization error so callers
see the init failure.
🪄 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: 6c171ca8-81aa-433c-9249-fbb3a780a80f
📒 Files selected for processing (5)
packages/control-plane/src/router.tspackages/control-plane/src/session/contracts.test.tspackages/control-plane/src/session/http/handlers/session-lifecycle.handler.tspackages/control-plane/src/session/initialize.test.tspackages/control-plane/src/session/initialize.ts
Comment on lines
+128
to
+137
| if (!initResponse.ok) { | ||
| const errorText = await initResponse.text().catch(() => "unknown"); | ||
| logger.error("DO init failed", { | ||
| session_id: input.sessionId, | ||
| status: initResponse.status, | ||
| error: errorText, | ||
| trace_id: ctx.trace_id, | ||
| }); | ||
| throw new Error(`Failed to initialize session DO: ${initResponse.status}`); | ||
| } |
There was a problem hiding this comment.
Handle DO-init failure with D1 compensation to avoid orphaned created sessions.
Lines 69-85 persist the D1 record first, but Lines 128-137 only throw on DO init failure. That leaves stale index rows (especially harmful for child-session listing/status flows).
💡 Suggested fix
export async function initializeSession(
env: Env,
input: SessionInitInput,
ctx: RequestContext
): Promise<{ sessionId: string; status: string }> {
@@
- const initResponse = await stub.fetch(
- new Request(buildSessionInternalUrl(SessionInternalPaths.init), {
- method: "POST",
- headers,
- body: JSON.stringify({
- sessionName: input.sessionId,
- repoOwner: input.repoOwner,
- repoName: input.repoName,
- repoId: input.repoId,
- defaultBranch: input.defaultBranch,
- branch: input.branch,
- title: input.title,
- model: input.model,
- reasoningEffort: input.reasoningEffort,
- userId: input.participantUserId,
- scmLogin: input.scmLogin,
- scmName: input.scmName,
- scmEmail: input.scmEmail,
- scmTokenEncrypted: input.scmTokenEncrypted,
- scmRefreshTokenEncrypted: input.scmRefreshTokenEncrypted,
- scmTokenExpiresAt: input.scmTokenExpiresAt,
- scmUserId: input.scmUserId,
- codeServerEnabled: input.codeServerEnabled,
- sandboxSettings: input.sandboxSettings,
- parentSessionId: input.parentSessionId,
- spawnSource: input.spawnSource,
- spawnDepth: input.spawnDepth,
- }),
- })
- );
+ let initResponse: Response;
+ try {
+ initResponse = await stub.fetch(
+ new Request(buildSessionInternalUrl(SessionInternalPaths.init), {
+ method: "POST",
+ headers,
+ body: JSON.stringify({
+ sessionName: input.sessionId,
+ repoOwner: input.repoOwner,
+ repoName: input.repoName,
+ repoId: input.repoId,
+ defaultBranch: input.defaultBranch,
+ branch: input.branch,
+ title: input.title,
+ model: input.model,
+ reasoningEffort: input.reasoningEffort,
+ userId: input.participantUserId,
+ scmLogin: input.scmLogin,
+ scmName: input.scmName,
+ scmEmail: input.scmEmail,
+ scmTokenEncrypted: input.scmTokenEncrypted,
+ scmRefreshTokenEncrypted: input.scmRefreshTokenEncrypted,
+ scmTokenExpiresAt: input.scmTokenExpiresAt,
+ scmUserId: input.scmUserId,
+ codeServerEnabled: input.codeServerEnabled,
+ sandboxSettings: input.sandboxSettings,
+ parentSessionId: input.parentSessionId,
+ spawnSource: input.spawnSource,
+ spawnDepth: input.spawnDepth,
+ }),
+ })
+ );
+ } catch (error) {
+ await sessionStore.updateStatus(input.sessionId, "failed").catch((statusError) => {
+ logger.error("Failed to mark session failed after DO init transport error", {
+ session_id: input.sessionId,
+ error: statusError instanceof Error ? statusError.message : String(statusError),
+ trace_id: ctx.trace_id,
+ });
+ });
+ throw error;
+ }
if (!initResponse.ok) {
+ await sessionStore.updateStatus(input.sessionId, "failed").catch((statusError) => {
+ logger.error("Failed to mark session failed after DO init non-ok", {
+ session_id: input.sessionId,
+ error: statusError instanceof Error ? statusError.message : String(statusError),
+ trace_id: ctx.trace_id,
+ });
+ });
const errorText = await initResponse.text().catch(() => "unknown");
logger.error("DO init failed", {
session_id: input.sessionId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/control-plane/src/session/initialize.ts` around lines 128 - 137, The
DO init failure path currently just logs and throws, leaving the
earlier-persisted D1 session row orphaned; update the failure handling around
initResponse (in initialize.ts) to perform D1 compensation: call the D1
delete/remove method for input.sessionId (or the existing session delete helper)
and await its completion before throwing, log success or any delete error via
logger.error/ logger.info (include session_id and trace_id), and if the delete
itself fails still throw the original initialization error so callers see the
init failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SessionInitInputinterface andinitializeSession()function insession/initialize.tsthat bothhandleCreateSessionandhandleSpawnChildnow callhandleSpawnChildhad DO-first ordering, which could leave orphaned DOs on D1 failure)SessionInitInputtype is shared between the router and the DO handler, preventing field driftparticipantUserId(session protocol identity for the creator) andplatformUserId(canonical user for analytics attribution)What changed in
router.tsBoth
handleCreateSession(~217 lines) andhandleSpawnChild(~221 lines) had duplicate D1 write + DO init sequences with untypedJSON.stringifybodies. Each now builds aSessionInitInputfrom its own preconditions and callsinitializeSession(). Handler-specific logic (identity resolution, enrichment, guardrails, prompt enqueue) stays in the handlers.Net effect
SessionInitInputrouter.tsinitializeSessionhas 8 unit testsTest plan
initializeSessioncovering: D1-before-DO ordering, D1 failure prevents DO init, DO init failure throws, correct fields to D1 and DO, correlation headers, return value, branch fallback logicSummary by CodeRabbit
Refactor
Tests
Documentation