fix(worker): keep a task's session standing after it answers - #204
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughCompleted bounded orchestrator sessions are retained instead of closed. Retention appears in session rows, prevents dispatch, supports rail selection, and clears when an operator takes over the session. ChangesRetained session lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BoundedTask
participant finish_turn
participant PtyManager
participant SessionHandle
participant Rail
BoundedTask->>finish_turn: settle successful bounded turn
finish_turn->>PtyManager: retain session
PtyManager->>SessionHandle: mark session retained
SessionHandle-->>Rail: project retained session row
Rail-->>SessionHandle: resolve selected session to PTY
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
A dispatched session was torn down the moment its reply went out. The pane behind it picks what to draw in priority order — live local PTY, watched worker screen, transcript — so closing the PTY dropped it to the third, and a task that had just finished read as one that had vanished. Nothing switched views; there was simply no longer a process to render. A turn that answered now retains its session instead. The harness that did the work is still there to look at, and the operator can take it and carry on in the context it built. A turn that *failed* still closes, exactly as before. A bounded turn fails when its prompt could not be injected — the harness is sitting on whatever blocked it — so there is nothing on that screen worth keeping, and leaving it would strand a wedged process. That is why retention is gated on the turn settling rather than on the session class alone. Retention is deliberately not takeover, and the distinction is load-bearing. Marking these sessions `User` is the smaller diff and deadlocks dispatch: `checkout_writer` reads any user-held session in a directory as the writer holding that checkout, so the first task to finish in a workspace would queue every task dispatched there after it until their budgets ran out. They stay the orchestrator's and carry a separate lifecycle flag, which `try_claim` refuses — bounded dispatches never consult the idle pool, so that guard is what keeps the rule true if they ever do. Taking a session clears it: it stops being the leftover screen of finished work and becomes somewhere a person is typing. The flag rides beside `busy` and `operator_held` for the reason those are atomics — it is tested per session on every dispatch — and reaches the UI on `SessionRow`, since retention the rail cannot see is retention nothing can show. Three existing tests asserted the old teardown. The injection-failure one is unchanged and still passing, because that path still closes. The two that asserted closure on the *success* path now assert retention, and their subject is unchanged: what stops a second task landing in the first's session is the retention flag rather than the teardown that used to do it. What this does not do is guarantee the finished session is on screen. Task rows carry no local PTY (`local: None`) and `own_session_rows` lists only the operator's, so a retained one reaches the rail through its task or not at all. Keeping the process alive is the half that has to come first; what the pane then shows wants observing against a real dispatch rather than reasoning about, and is left to a follow-up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
306d392 to
e3d6d90
Compare
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Pushed
The bug in my first attempt: gating retention on
On the edited tests, since a contributor rewriting the tests that failed deserves scrutiny: none were deleted or weakened. The injection-failure one is untouched and still passing. The two success-path ones assert the same property through the new mechanism — what stops a second task landing in the first's session is now the retention flag rather than the teardown that used to do it. Both still assert one session per bounded task. Local: |
Retaining the session kept the harness alive and left it reachable by nothing, which from the operator's seat is the same as having closed it: the interactive screen went away and the pane fell back to the transcript. Two lookups go dead the moment a task settles, and both have to be understood together. `session_for_task` resolves through the daemon's *running* map, and the admission guard drops that record when the task finishes — so the streamed screen stops arriving. And a task's rail row carries no local session (`local: None`), so the cursor on one resolves no pty: nothing to draw a live screen from, nothing to attach the keyboard to. A retained session matched neither clause of `own_session_rows` — it is dispatched, not the operator's, and deliberately still the orchestrator's — so it had no row of its own either. Listing retained sessions fixes both at once, and by the better path. The row carries a real pty id, which is the pane's *first* branch: it draws the local harness directly instead of the hub's screen stream, so it does not depend on the task lookup that just went away. Put the cursor on it and it is a live harness to work in. The cost is the double listing the old comment warned about: while a task runs it shows as a task row, and once it finishes there is a session row for the same harness beside it. That is cosmetic, and it is the row the operator can actually type in. Merging the two wants a task-to-session mapping `TaskState` does not carry today, and is left alone here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
sanil-23 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Pushed Retention was working; nothing could reach it. On a running instance, four claude harnesses were alive as children of the TUI, outliving tasks that had finished minutes earlier. Before this branch they'd have been killed on reply. But the interactive screen still vanished and the pane fell back to the transcript, because two lookups go dead the moment a task settles:
So the harness was alive and reachable by nothing — which from the operator's seat is indistinguishable from having closed it. Listing retained sessions fixes both, and by the better path: the row carries a real pty id, which is the pane's first branch ( Known cosmetic cost, called out because the existing comment warned about it: while a task runs it shows as a task row, and once it finishes a session row for the same harness appears beside it. Merging them needs a task→session mapping Local: |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tui/src/worker/pty/handle/control.rs (1)
124-140: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize the retention check with the claim.
A claimant can read
retained == falseat Line 125 while the completed turn is still busy.finish_turncan then retain the session and releasebusy. The claimant can win the CAS and returntruebecause Lines 136-140 recheck only control.Hold
coldacross the final control and retention checks and thebusyCAS, afterfinish_completion_grace. Add a regression test that pauses a claim between its eligibility check and the completion release.🤖 Prompt for 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. In `@src/tui/src/worker/pty/handle/control.rs` around lines 124 - 140, Update try_claim to hold cold after finish_completion_grace while rechecking control and retention state and performing the busy CAS, so completion retention cannot race with claiming; preserve release behavior on failed control validation. Add a regression test that pauses claiming between the initial eligibility check and completion release, verifying the retained session cannot be claimed.
🤖 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 `@src/tui/src/ui/app/rail/tests.rs`:
- Around line 205-213: Update the test around the session setup to create the
session through executor dispatch instead of harnesses.open_unmanaged, ensuring
it has a non-user origin. Assert the dispatched session’s origin before calling
sessions.retain, while preserving the existing retained-session assertion.
---
Outside diff comments:
In `@src/tui/src/worker/pty/handle/control.rs`:
- Around line 124-140: Update try_claim to hold cold after
finish_completion_grace while rechecking control and retention state and
performing the busy CAS, so completion retention cannot race with claiming;
preserve release behavior on failed control validation. Add a regression test
that pauses claiming between the initial eligibility check and completion
release, verifying the retained session cannot be claimed.
🪄 Autofix
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: fd9b8150-15a6-4855-85d8-17b2fc632caf
📒 Files selected for processing (17)
src/tui/src/ui/app/changes/baseline_tests.rssrc/tui/src/ui/app/rail/mod.rssrc/tui/src/ui/app/rail/resolve.rssrc/tui/src/ui/app/rail/tests.rssrc/tui/src/ui/app/render/agents/rail/tests.rssrc/tui/src/ui/app/render/settings/status_line.rssrc/tui/src/worker/executor/run.rssrc/tui/src/worker/executor_tests/basic.rssrc/tui/src/worker/executor_tests/sessions.rssrc/tui/src/worker/pty/handle/control.rssrc/tui/src/worker/pty/handle/lifecycle.rssrc/tui/src/worker/pty/handle/state.rssrc/tui/src/worker/pty/handle/types.rssrc/tui/src/worker/pty/manager/session.rssrc/tui/src/worker/pty/tests/control.rssrc/tui/src/worker/pty/tests/types.rssrc/tui/src/worker/pty/types.rs
1 participant
What
A dispatched session was torn down the moment its reply went out, so a task that had just finished read as one that had vanished.
The pane picks what to draw in priority order — live local PTY → watched worker screen → transcript (transcript.rs:41). Closing the PTY dropped it to the third. Nothing switched views; there was no longer a process to render.
SessionClass::Boundedsays it outright: "created for one task frame, runs exactly one turn, and is torn down when the reply is sent."What changed
finish_turnretains and frees the session instead of closing it. The harness that did the work is still there to look at, and the operator can take it and keep going in the context it built.Why retention is not takeover
This is the part worth reading before simplifying it back.
Marking these sessions
SessionControl::Useris the smaller diff and deadlocks dispatch.checkout_writerreads any user-held session in a directory as the writer holding that checkout:and
session_forqueues behind it. So the first task to finish in a workspace would hold that checkout forever, and every task dispatched there afterwards would queue until its budget expired, then fail.Retained sessions therefore stay
Orchestratorand carry a separate lifecycle flag.try_claimrefuses them — bounded dispatches never consult the idle pool, so that guard is what keeps the rule true if they ever do. Taking a session clears it: it stops being the leftover screen of finished work and becomes somewhere a person is typing.The flag sits beside
busyandoperator_heldas an atomic for the same reason those are (tested per session on every dispatch), and reaches the UI onSessionRow, since retention the rail cannot see is retention nothing can show.What this does not do
It does not guarantee the finished session is on screen. Task rows carry no local PTY (
local: None), andown_session_rowslists only sessions that are the operator's — so a retained one reaches the rail through its task or not at all. The hub does cache the lastWatchedScreenper(worker, task_id), so it may already render; I have not observed it against a real dispatch and am not going to claim it.Keeping the process alive is the half that has to come first. What the pane then shows wants observing rather than reasoning about, and is left to a follow-up — likely threading the session id onto the task row so
session_id()resolves, rather than double-listing the session, since "a task is an agent session" is the model the rail already commits to.Resource note
Retained sessions are live harness processes and nothing closes them automatically —
Kkills one, and taking it makes it an ordinary operator session. This is intentional for now: because they are never checkout writers, they cannot block work, so the cost is memory and PTYs rather than correctness. A cap or a close-when-the-cycle-ends rule is the obvious follow-up if it bites.Validation
cargo clippy --all-targets -- -D warnings— cleancargo fmt --check— cleancargo test -p medulla-tui --lib worker::pty::tests::control— 16 passed, 0 failed4 new tests: a retained session is never dispatched into; it reports itself on the row while staying
Orchestrator(that assertion carries thecheckout_writerreasoning in its failure message); taking it clears the flag; retaining an unknown id is not an error.worker::pty::tests::sessionhas 3 pre-existing failures —a_launch_root_preserves_trailing_whitespace,a_session_snapshots_head_before_the_harness_can_commit,an_unborn_repository_records_its_root_without_a_launch_commit. All three are/private/varvs/varmacOS symlink assertions and fail identically on a cleanupstream/main. Not touched by this PR.🤖 Generated with Claude Code
Summary by CodeRabbit