fix: reuse prepared session on explicit start#1432
Open
jcfs wants to merge 3 commits into
Open
Conversation
session.ensure creates a CREATED session via prepare when a task's step lacks auto_start_agent. The subsequent edit-dialog "Start Agent" click called LaunchSession(IntentStart) without a session_id, so launchStart fell through to StartTask which unconditionally wrote a second session row and surfaced two tabs on a fresh task. launchStart now acquires the per-task ensureLocks mutex (shared with EnsureSession to close the race), looks for a CREATED session on the task, and promotes it via launchStartCreated instead of creating a duplicate. Primary wins; newest-updated CREATED is the fallback. Regression-tested: the integration test fails (got 2) with the reuse block removed and passes with it in place.
📝 WalkthroughWalkthrough
ChangesSession reuse and deadlock prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
|
| Filename | Overview |
|---|---|
| apps/backend/internal/orchestrator/session_ensure.go | Adds ensureLockHeldCtxKey + helpers to stamp/read a context marker; stamps the context immediately after acquiring the mutex in EnsureSession so downstream launchStart can skip a recursive lock acquisition — clean, minimal change. |
| apps/backend/internal/orchestrator/session_launch.go | Adds canReusePreparedSession gate, findReusableCreatedSession helper (with warn-level error logging), and the reuse block in launchStart; lock acquisition is correctly skipped when the context marker is set, and StartTask still runs under the same mutex when reuse is considered but not taken. |
| apps/backend/internal/orchestrator/session_launch_test.go | Four new tests cover the reuse logic, profile-mismatch fallthrough, the UpdatedAt vs SQL-order tie-break, and the EnsureSession deadlock scenario; all tests rely on recover() for expected scheduler panics, which is clearly documented in comments. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as Frontend (edit dialog)
participant ES as EnsureSession
participant LS as launchStart
participant FR as findReusableCreatedSession
participant SC as launchStartCreated
participant ST as StartTask
Note over ES: Task preview opens
ES->>ES: acquireEnsureLock(taskID)
ES->>ES: "ctx = withEnsureLockHeld(ctx)"
ES->>ES: findExistingSession → nil (no sessions)
ES->>LS: LaunchSession(IntentPrepare)
LS->>LS: PrepareTaskSession → CREATED row
Note over UI: User clicks Start Agent
UI->>LS: LaunchSession(IntentStart, no session_id)
LS->>LS: canReusePreparedSession → true
alt "ensureLockHeld(ctx) == false (direct call)"
LS->>LS: acquireEnsureLock(taskID)
end
LS->>FR: findReusableCreatedSession(taskID)
FR->>FR: ListTaskSessions → [CREATED session]
FR-->>LS: session (primary or newest-updated)
alt profile matches or caller has no preference
LS->>SC: launchStartCreated(req with session.ID)
SC-->>UI: success, same session ID
else profile mismatch
LS->>ST: StartTask → new session row
ST-->>UI: success, new session ID
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as Frontend (edit dialog)
participant ES as EnsureSession
participant LS as launchStart
participant FR as findReusableCreatedSession
participant SC as launchStartCreated
participant ST as StartTask
Note over ES: Task preview opens
ES->>ES: acquireEnsureLock(taskID)
ES->>ES: "ctx = withEnsureLockHeld(ctx)"
ES->>ES: findExistingSession → nil (no sessions)
ES->>LS: LaunchSession(IntentPrepare)
LS->>LS: PrepareTaskSession → CREATED row
Note over UI: User clicks Start Agent
UI->>LS: LaunchSession(IntentStart, no session_id)
LS->>LS: canReusePreparedSession → true
alt "ensureLockHeld(ctx) == false (direct call)"
LS->>LS: acquireEnsureLock(taskID)
end
LS->>FR: findReusableCreatedSession(taskID)
FR->>FR: ListTaskSessions → [CREATED session]
FR-->>LS: session (primary or newest-updated)
alt profile matches or caller has no preference
LS->>SC: launchStartCreated(req with session.ID)
SC-->>UI: success, same session ID
else profile mismatch
LS->>ST: StartTask → new session row
ST-->>UI: success, new session ID
end
Reviews (2): Last reviewed commit: "fix: address review feedback on prepared..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Greptile P1: launchStart's acquireEnsureLock(req.TaskID) deadlocked when EnsureSession (which already holds that mutex via defer release()) re- entered through LaunchSession on auto_start_agent steps. sync.Mutex isn't reentrant. Thread an ensureLockHeld context marker so launchStart skips the inner acquire when EnsureSession owns the lock. cubic P2 (x2): tighten the reuse gate via canReusePreparedSession so an explicit ExecutorID, ExecutorProfileID, or Priority bypasses reuse (start_created carries none of those), and refuse to adopt a prepared session whose AgentProfileID disagrees with the caller's. Greptile P2: warn-log instead of silently swallowing ListTaskSessions errors in findReusableCreatedSession; a transient DB error would otherwise route the caller back into StartTask and write the duplicate row this PR exists to prevent. OpenCode: rewrite the "falls back to newest CREATED when no primary" subtest so the SQL started_at ordering disagrees with UpdatedAt — the new fixture writes both timestamps at CreateTaskSession time so only the UpdatedAt comparison can pick the right session. New regression tests: TestCanReusePreparedSession, TestLaunchStart_NoReuseOnProfileMismatch, TestEnsureSession_AutoStartDoesNotDeadlock (verified: hangs without the marker, passes with it).
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/internal/orchestrator/session_launch_test.go`:
- Around line 695-697: The timeout duration in the time.After calls within the
LaunchSession test is too long for in-process negative assertions. Replace the 2
* time.Second timeout values with a shorter duration of approximately 100 *
time.Millisecond in both locations: the case statement at line 697 and the
similar case statement around line 749-752. This aligns with the coding
guidelines that specify short in-process timeouts (~100ms) for negative
assertions to provide faster failure feedback during testing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa41b80a-3a9b-48e4-ba33-614f5942de69
📒 Files selected for processing (3)
apps/backend/internal/orchestrator/session_ensure.goapps/backend/internal/orchestrator/session_launch.goapps/backend/internal/orchestrator/session_launch_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/internal/orchestrator/session_launch.go
There was a problem hiding this comment.
2 issues found across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
CodeRabbit: drop the 2s in-process timeouts in the deadlock/hang tests
to 150ms per the repo's testing guideline ("short timeouts (~100ms)
for in-process negative assertions; multi-second waits only for
subprocess/integration tests"). Failure feedback is now near-instant.
OpenCode: the warn-log comment on findReusableCreatedSession implied
the new code prevents the duplicate row, but launchStart still falls
through to StartTask on a list error — same behavior as pre-PR. Rewrote
the comment to describe what the warn actually buys (visibility into a
transient DB failure), not a behavior change.
cubic P2 (profile compat): extract profilesCompatible — caller and
prepared-session profiles match when identical OR either side is empty.
Previously a non-empty caller profile blocked reuse against a session
with an empty profile, even though start_created could have adopted the
caller's profile. Differing non-empty profiles still skip reuse and
fall through to a fresh launch. TestProfilesCompatible pins the matrix.
cubic P2 (fail-closed on list error): rejected. Returning an error from
launchStart on a transient ListTaskSessions failure would be a UX
regression — pre-PR launchStart never depended on that lookup at all
and would have silently created a session. Warn+continue matches that
prior behavior; the warn now makes the failure visible. The reuse path
is an optimization, not a correctness gate, so failing closed would
trade a silent duplicate row for a surfaced launch error on the same
DB hiccup.
Contributor
|
OpenCode review complete OpenCode model: No suggestions found for commit |
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.
Creating a task in a workflow step without
auto_start_agentand then clicking Start Agent from the edit-task dialog left the task with two session tabs.session.ensurealready prepared a CREATED session when the preview opened, butlaunchStartunconditionally calledStartTask→PrepareSession, creating a second session row.Important Changes
launchStartnow adopts an existing CREATED session vialaunchStartCreatedwhen nosession_idis supplied. The per-taskensureLocksmutex is shared withEnsureSessionso the prepare and start paths can't race and both decide to create.findReusableCreatedSessionpicks the primary CREATED session (newest-updated CREATED as fallback); non-CREATED states are skipped to avoid implicitly reviving terminal or in-flight sessions.Validation
make -C apps/backend test lint— green.session_launch_test.go:TestFindReusableCreatedSession— state filter + primary/newest preference.TestLaunchStart_ReusesPreparedSession— confirmed it fails (got 2) with the reuse block removed and passes with it in place.pnpm --filter @kandev/web lint— green after the worktreepnpm install --frozen-lockfile.Checklist
apps/web/), I have added or updated Playwright e2e tests inapps/web/e2e/and verified them withmake test-e2e.Preview Environment
3b5e4a0