Skip to content

fix(cli): let /session detach from a running Turn instead of trapping the client - #3498

Merged
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch
Aug 23, 2026
Merged

fix(cli): let /session detach from a running Turn instead of trapping the client#3498
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch

Conversation

@me2seeks

@me2seeksme2seeks commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes#3380

Problem

A second TUI that switched onto a Session with an in-flight Turn (via /session or the picker) was trapped: /session was intercepted with "Cannot run /session while a turn is running", and every apparent way out was destructive — following the hint (Esc/Ctrl+C) or quitting called driver.stop(), aborting the shared Turn another client was watching. The only clean escape was kill -9.

Multi-client attach is a designed Runtime Host capability: the Turn is Host-owned and each TUI is only its viewport. Desktop already treats session switching as pure view navigation; this fix brings the CLI to the same model.

Fix: let /session detach from a running Turn instead of touching it

  • New 'switch' mid-turn slash disposition (alongside 'local' from refactor(cli): move the mid-turn slash disposition onto the command spec #3379). Routed through like 'local', but its handler must use busy-aware wrappers: idle it runs under runControl's serial lock as before; mid-turn the switch goes through the new detach path instead of silently no-oping on runControl's busy gate.
  • switchAwayMidTurn never calls driver.stop(). After driver.switchSession confirms, a monotonic turnEpoch fence orphans the in-flight drain:
    • late events and synthesized stream failures ("ended without a completion event") from the abandoned Session can no longer reach the adopted transcript;
    • the orphaned runAgentTurn tail skips all old-session continuations (queue flushes would steer the NEW Session), releases busy/activity, and starts the freshly attached Turn exactly once — whichever of the detach path or the orphan tail observes an idle runner first.
  • Interrupt guard during handoff: requestTurnInterrupt is swallowed while a detach is in flight — the driver already points at the next Session, so a stop there would abort whatever that Session has attached.
  • Escape owns the picker: while the mid-turn session picker is open, Escape closes it instead of arming the double-Escape interrupt for the Turn being left running.
  • Foreign import rows hidden mid-turn: the Claude Code/Codex import flow starts a new Session and cannot detach.

Scope notes

Testing

  • 3 new tests in pi-tui-runner.test.ts with a parking-turn driver:
    • mid-turn /session <id>: switches with zero stop() calls, replaces the transcript, fences leaked events, starts the attached Turn after the orphan unwinds, lands follow-ups on the adopted Session;
    • /session opens the picker mid-turn and Escape closes it without arming an interrupt;
    • a failed switch reports the error and leaves the running Turn streaming normally.
  • Full CLI suite: 360/360 pass; biome + typecheck clean.

AI use

  • Generative tooling made a substantive contribution
  • No generative tool made a substantive contribution

Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push. Generated-by: Maka trailers are present on the branch commits.

@me2seeks

Copy link
Copy Markdown
ContributorAuthor

Working on this PR — happy to iterate on review feedback. Local verification: CLI suite 360/360, biome + typecheck clean.

… the client
A second TUI that switched onto a Session with an in-flight Turn was
trapped: every exit path was destructive. /session was intercepted with
'Cannot run /session while a turn is running', and following the hint
(Esc/Ctrl+C) or quitting aborted the shared Turn via driver.stop() —
killing work another client was watching (apache#3380).
Multi-client attach is a designed Runtime Host capability: the Turn is
Host-owned and the TUI is only its viewport, so switching Sessions
mid-turn is view navigation, not a session mutation.
- new 'switch' mid-turn slash disposition alongside 'local': routed
through like 'local', but its handler must use the busy-aware
goToSession/openSessionPicker wrappers (runControl's serial lock is
held by the running Turn mid-turn)
- switchAwayMidTurn adopts the next Session without ever calling
driver.stop(); a turnEpoch fence orphans the in-flight drain after the
switch is confirmed so late events, synthesized stream failures, and
old-session queue flushes can never reach the adopted transcript; the
orphaned runAgentTurn tail releases busy/activity and hands the freshly
attached Turn its start exactly once
- requestTurnInterrupt is swallowed while a detach handoff is in flight:
the driver already points at the next Session, so a stop there would
abort whatever that Session has attached
- Escape closes the mid-turn session picker instead of arming the
double-Escape interrupt for the Turn being left running
- foreign-session import rows are hidden from the picker mid-turn (the
import flow starts a new Session; it cannot detach)
Generated-by: Maka
…switch recovery
- '/session <id>' mid-turn: switches without driver.stop(), replaces the
transcript with the adopted Session's history, fences late events from
the abandoned drain (no content leak, no synthesized 'ended without a
completion event' failure), starts the freshly attached Turn only
after the orphaned drain unwinds, and lands follow-up prompts on the
adopted Session
- '/session' mid-turn opens the picker; Escape closes it and must not
arm the double-Escape interrupt (stopCalls stays 0)
- a rejected switch leaves the running Turn fully live: error notice,
no stop, subsequent events still render into the same transcript
Generated-by: Maka
@me2seeks
me2seeksforce-pushed the fix/3380-mid-turn-session-switch branch from 21e2611 to 24a3a67CompareAugust 22, 2026 13:43

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this is a well-argued fix and the framing is right: in Runtime Host mode the Turn belongs to the Host and this TUI is only a viewport, so refusing /session mid-turn was trapping the user in a view rather than protecting anything. The monotonic turnEpoch fence is the correct shape for it — one counter makes every callback of the abandoned drain a no-op, instead of trying to unsubscribe them individually. Fencing only afterswitchSession resolves, so a failed switch leaves the drain fully live, is a detail that is easy to get backwards and you got it right.

Reviewed at exact head 24a3a6711870a427bd82f2b8f03b86b4f52b37c4 against base 8b60ddffa89682c03238ddce6595f531eb2b6f29. One P2 inline, plus two P3 verification gaps below. No checks have run on this head yet.

P3 — switching to the session you are already on.goToSession has no same-session short-circuit, so /session <current-id> mid-turn bumps the epoch, orphans the live drain, and prints Detached from the running Turn while the user is still on the same session. Whether this is harmless depends on what driver.switchSession does with its own session id, which we could not settle from the client side. A test pinning the intended behaviour would close it either way.

P3 — re-attaching to the same still-running Turn. The new tests cover attaching to a different session's live turn. Detaching and immediately re-attaching to the same one is the case where a double consumer would show up if the orphan tail and the new attach ever overlapped; the startPendingAttachedTurn no-op guards look like they prevent it, but nothing pins it.

Neither P3 blocks. The P2 does, and it is small to fix.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

// tail unwinds through the superseded branch and releases busy/activity,
// then either that tail or the startPendingAttachedTurn below starts the
// freshly attached Turn, whichever observes an idle runner first.
const switchAwayMidTurn = async (sessionId: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] switchAwayMidTurn is re-entrant, and a second detach clears detaching while the first is still in flight.

Mid-turn, both /session entry points deliberately skip runControl's serial lock — that is the point of the change, since the lock is held by the running Turn. Both are also invoked fire-and-forget: void goToSession(sessionId) and void openSessionPicker(). So nothing serializes switchAwayMidTurn against itself, and it has an await boundary in the middle of it (await input.driver.switchSession(sessionId)).

Sequence: the user types /session s2. While that request is in flight they type /session s3 — normal impatience, and the window is as long as the round trip. Now two switchAwayMidTurn calls are live.

The consequence we care about is the finally block. detaching is a boolean, so when the first call settles it runs detaching = false while the second is still awaiting its switch. That reopens requestTurnInterrupt, whose new guard exists precisely for this window — your own comment there says "a stop here would abort whatever that Session has attached." A Ctrl+C landing in that reopened window calls driver.stop() against the freshly adopted session, which is the exact outcome this PR promises cannot happen. The invariant is stated correctly in the code and then broken by re-entry.

Two smaller effects ride along: applySwitchResult runs twice in whatever order the responses return, so the user can end up on s2 after asking for s3; and two Detached from the running Turn notices land in the adopted transcript.

The mechanism to fix it already exists — detaching is shaped like a re-entrancy flag, it just is not read at the entry points. Rejecting the second detach is enough:

constgoToSession=async(sessionId: string): Promise<void>=>{if(!turnRunning){awaitrunControl(()=>switchSession(sessionId));return;}if(detaching)return;// a detach is already handing this view overawaitswitchAwayMidTurn(sessionId).catch(reportError);};

with the same guard on openSessionPicker. Turning detaching into a counter would also work but is more machinery than the situation needs — a second detach during a handoff has no meaningful semantics, so dropping it is the honest behaviour.

A regression test would issue a second /session before the first switchSession promise resolves and assert that driver.stop() is still never called and that exactly one Detached notice appears. The existing failNextSwitch test already shows the harness can control that promise's timing.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed both mechanisms locally before fixing (reproduced the double switchSession and the early detaching clear with a parked first switch), fixed at 332404759:

  • goToSession now rejects while detaching is held. The picker path needed no separate guard — its selection routes through goToSession (:2179), so one guard covers both entry points.
  • Pinned by a test: second mid-turn /session during a parked switch yields exactly one detach notice, no extra stop(), single adoption.

One nuance worth flagging: after the blocked re-entry, the queued command text can replay through the idle path once the adopted turn settles, producing a benign second switchSession to the same id — that is pre-existing queue behavior outside this window, not a reopen of it.

Comment on lines +1152 to +1155
// A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
// drain: from that point every callback below must stop touching shared
// runner state — the adopted Session owns it now.
const superseded = () => epoch !== turnEpoch;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This comment says every callback below stops touching shared runner state once the epoch is bumped, but two of them do not check.

superseded() is consulted at :1226 (onEvent), :1263 and :1278. It is not consulted in:

  • onPrepared (:1197-1210) — the non-attached branch ends in if (turn.summary) adoptSessionMetadata(turn.summary), which writes title/cwd/model on the runner
  • onSkillInvocation (:1211-1222) — splices state.entries and calls showSkillInvocation

Both are reachable after a switch-away, because turnRunning is set at the top of runAgentTurn while pi-tui-turn.ts:87-88 only invokes these two afterawait preparePrompt resolves. So the whole preparePrompt window is a period where /session can already enter switchAwayMidTurn, and neither callback has returned yet.

Concrete sequence:

  1. a turn stalls in preparePrompt (slow or hung)
  2. /session <other>switchSession succeeds, turnEpoch bumps, applySwitchResult replaces the transcript with the adopted session
  3. preparePrompt finally resolves
  4. onPrepared runs with the abandoned turn's summary and calls adoptSessionMetadata on it

The screen now describes the old session while the driver is on the new one, and the next prompt goes to the new one. The skill callback has the same shape: an overlay belonging to the abandoned session appearing over the adopted viewport.

shouldAbort doesn't help — it only reads closed, not the epoch.

Fix looks like the same one-liner already used in the three fenced callbacks: if (superseded()) return; at the top of both, including the onSkillInvocation on the SkillInvocationBlockedError path. A test that switches while preparePrompt is still unresolved, then asserts the metadata still belongs to the adopted session, would pin it.

This is separate from the re-entrant detaching P2 already reported inline on this head — that one is about entering the path twice, this one is about callbacks that were already in flight when it was entered once.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed before fixing: reproduced an abandoned Turn adopting its summary over the adopted session when preparePrompt resolved after a completed switch-away. Fixed at 332404759onPrepared and onSkillInvocation now open with the same superseded() fence as the other callbacks, which also covers the SkillInvocationBlockedError invocation since both call sites share the callback. New test parks preparePrompt, detaches mid-park, then resolves: the abandoned title/cwd never reach the runner metadata and the skill card cannot land on the adopted viewport. CLI suite 362/362, biome clean.

@Astro-Han

Copy link
Copy Markdown
Contributor

Second review pass on 24a3a6711870a427bd82f2b8f03b86b4f52b37c4, from a different model and a different route than the earlier one on this head. One new [P2], left inline at the epoch fence: #discussion_r3837660462.

The mechanism itself holds up, and we checked the thing that usually goes wrong with an escape path — what happens to the thing you escaped from:

  • The Host Turn is not stopped.switchAwayMidTurn never calls driver.stop() (verified: zero stop calls in the covering tests), which is the correct asymmetry against the interrupt path. In Runtime Host mode the Turn is Host-owned and this TUI was only its viewport, so detaching the view should not end the work.
  • A failed switch does not orphan the live drain.turnEpoch += 1 happens only after await switchSession resolves, so a throw leaves the in-flight drain fully live — pinned by a test asserting late deltas still land in the same transcript.
  • The adopted Turn cannot start twice.startPendingAttachedTurn no-ops while busy || turnRunning, and the old drain's finishTurnUi clears turnRunning before releasing; whichever of the two observes an idle runner first starts it.
  • Escape on the picker doesn't interrupt. Early return while sessionPickerOverlayOpen, with a test asserting no stop call.

On structure: one epoch counter, not a second authority over Turn lifetime. That's the right shape for this.

Two things noted without grading, since neither is this PR's job to fix:

  • Removing /transcript and the TranscriptViewerOverlay reference is a behaviour deletion unrelated to detach. The viewer module file itself isn't in this diff, so it will be left behind — worth a follow-up so it doesn't linger as dead code.
  • /session <current id> still detaches from itself; goToSession has no short-circuit. Already reported previously, not re-graded here.

The re-entrant detaching [P2] already on this head stands on its own; we looked at it independently and agree, and are deliberately not re-grading it. The finding above is a different failure: not entering the path twice, but callbacks that were already in flight when it was entered once.

On CI: the run on this head was sitting at action_required and had never executed — I've approved it, so there should be a real signal shortly. Not approving while the P2 above and the existing one are open.

Two review findings on the apache#3380 detach path:
- switchAwayMidTurn was re-entrant: a second mid-turn /session while the
first was still handing the view over cleared the detaching flag early,
reopening the interrupt window and double-applying adoption. The entry
now rejects while a detach is in flight (the picker selection routes
through the same guard).
- onPrepared and onSkillInvocation ran without the superseded() fence the
other callbacks use. Both are reachable after a switch-away because they
fire only after preparePrompt resolves, so an abandoned Turn could still
adopt its metadata onto the adopted Session's view and surface its skill
card over the adopted viewport.
Both fixes are pinned by tests: a parked second /session yields exactly
one detach notice, and a Turn prepared across a detach can no longer
overwrite the adopted session's title/cwd.
Generated-by: maka

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 3324047598651d6b725bdb579cb66c3b0eeb99cc. Required test is completed / success bound to that exact SHA. No P0–P2.

Re-review at the current head. Both earlier findings were re-derived from the code rather than taken as fixed.

Both [P2]s are closed.

The re-entrant detach is guarded at the entry point, and — importantly — the guard is not cleared early: if (detaching) return; at pi-tui-runner.ts:1625 sits behind a comment explaining that a second detach arriving while the first is still handing the view over would otherwise reset the flag and reopen the path. That is the actual failure mode, closed at the right place.

The epoch fence is now complete. superseded() is consulted at five call sites (:1201, :1219, :1234, :1271, :1286) rather than three, so the two callbacks that previously kept touching shared runner state after the epoch bump no longer do. The comment claiming every callback below the fence checks it is now true — before, the comment described an intent the code did not implement, which is the more dangerous of the two states.

The production increment is +12 lines. It fixes exactly these two holes and introduces nothing else.

Residual, non-blocking: the same-session detach [P3] stands; detaching and immediately reattaching to the same running Turn produces a two-consumer situation that no test currently pins; and the /transcript dead-module follow-up is unchanged.

Stated rather than implied: the local CLI suite and a real multi-client Host were not exercised here — the evidence is the hosted test on this exact head plus code-level verification of both fixes.

Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review — merge still needs a committer other than the author to give LGTM and to decide.

@Astro-Han
Astro-Han merged commit bfcd3da into apache:mainAug 23, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): a second TUI attached to a Session with a running Turn is trapped — every exit path aborts the Turn

2 participants

@me2seeks@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix(cli): let /session detach from a running Turn instead of trapping the client by me2seeks · Pull Request #3498 · apache/maka · GitHub
Skip to content

fix(cli): let /session detach from a running Turn instead of trapping the client - #3498

Merged
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch
Aug 23, 2026
Merged

fix(cli): let /session detach from a running Turn instead of trapping the client#3498
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch

Conversation

@me2seeks

@me2seeksme2seeks commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes#3380

Problem

A second TUI that switched onto a Session with an in-flight Turn (via /session or the picker) was trapped: /session was intercepted with "Cannot run /session while a turn is running", and every apparent way out was destructive — following the hint (Esc/Ctrl+C) or quitting called driver.stop(), aborting the shared Turn another client was watching. The only clean escape was kill -9.

Multi-client attach is a designed Runtime Host capability: the Turn is Host-owned and each TUI is only its viewport. Desktop already treats session switching as pure view navigation; this fix brings the CLI to the same model.

Fix: let /session detach from a running Turn instead of touching it

  • New 'switch' mid-turn slash disposition (alongside 'local' from refactor(cli): move the mid-turn slash disposition onto the command spec #3379). Routed through like 'local', but its handler must use busy-aware wrappers: idle it runs under runControl's serial lock as before; mid-turn the switch goes through the new detach path instead of silently no-oping on runControl's busy gate.
  • switchAwayMidTurn never calls driver.stop(). After driver.switchSession confirms, a monotonic turnEpoch fence orphans the in-flight drain:
    • late events and synthesized stream failures ("ended without a completion event") from the abandoned Session can no longer reach the adopted transcript;
    • the orphaned runAgentTurn tail skips all old-session continuations (queue flushes would steer the NEW Session), releases busy/activity, and starts the freshly attached Turn exactly once — whichever of the detach path or the orphan tail observes an idle runner first.
  • Interrupt guard during handoff: requestTurnInterrupt is swallowed while a detach is in flight — the driver already points at the next Session, so a stop there would abort whatever that Session has attached.
  • Escape owns the picker: while the mid-turn session picker is open, Escape closes it instead of arming the double-Escape interrupt for the Turn being left running.
  • Foreign import rows hidden mid-turn: the Claude Code/Codex import flow starts a new Session and cannot detach.

Scope notes

Testing

  • 3 new tests in pi-tui-runner.test.ts with a parking-turn driver:
    • mid-turn /session <id>: switches with zero stop() calls, replaces the transcript, fences leaked events, starts the attached Turn after the orphan unwinds, lands follow-ups on the adopted Session;
    • /session opens the picker mid-turn and Escape closes it without arming an interrupt;
    • a failed switch reports the error and leaves the running Turn streaming normally.
  • Full CLI suite: 360/360 pass; biome + typecheck clean.

AI use

  • Generative tooling made a substantive contribution
  • No generative tool made a substantive contribution

Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push. Generated-by: Maka trailers are present on the branch commits.

@me2seeks

Copy link
Copy Markdown
ContributorAuthor

Working on this PR — happy to iterate on review feedback. Local verification: CLI suite 360/360, biome + typecheck clean.

… the client
A second TUI that switched onto a Session with an in-flight Turn was
trapped: every exit path was destructive. /session was intercepted with
'Cannot run /session while a turn is running', and following the hint
(Esc/Ctrl+C) or quitting aborted the shared Turn via driver.stop() —
killing work another client was watching (apache#3380).
Multi-client attach is a designed Runtime Host capability: the Turn is
Host-owned and the TUI is only its viewport, so switching Sessions
mid-turn is view navigation, not a session mutation.
- new 'switch' mid-turn slash disposition alongside 'local': routed
through like 'local', but its handler must use the busy-aware
goToSession/openSessionPicker wrappers (runControl's serial lock is
held by the running Turn mid-turn)
- switchAwayMidTurn adopts the next Session without ever calling
driver.stop(); a turnEpoch fence orphans the in-flight drain after the
switch is confirmed so late events, synthesized stream failures, and
old-session queue flushes can never reach the adopted transcript; the
orphaned runAgentTurn tail releases busy/activity and hands the freshly
attached Turn its start exactly once
- requestTurnInterrupt is swallowed while a detach handoff is in flight:
the driver already points at the next Session, so a stop there would
abort whatever that Session has attached
- Escape closes the mid-turn session picker instead of arming the
double-Escape interrupt for the Turn being left running
- foreign-session import rows are hidden from the picker mid-turn (the
import flow starts a new Session; it cannot detach)
Generated-by: Maka
…switch recovery
- '/session <id>' mid-turn: switches without driver.stop(), replaces the
transcript with the adopted Session's history, fences late events from
the abandoned drain (no content leak, no synthesized 'ended without a
completion event' failure), starts the freshly attached Turn only
after the orphaned drain unwinds, and lands follow-up prompts on the
adopted Session
- '/session' mid-turn opens the picker; Escape closes it and must not
arm the double-Escape interrupt (stopCalls stays 0)
- a rejected switch leaves the running Turn fully live: error notice,
no stop, subsequent events still render into the same transcript
Generated-by: Maka
@me2seeks
me2seeksforce-pushed the fix/3380-mid-turn-session-switch branch from 21e2611 to 24a3a67CompareAugust 22, 2026 13:43

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this is a well-argued fix and the framing is right: in Runtime Host mode the Turn belongs to the Host and this TUI is only a viewport, so refusing /session mid-turn was trapping the user in a view rather than protecting anything. The monotonic turnEpoch fence is the correct shape for it — one counter makes every callback of the abandoned drain a no-op, instead of trying to unsubscribe them individually. Fencing only afterswitchSession resolves, so a failed switch leaves the drain fully live, is a detail that is easy to get backwards and you got it right.

Reviewed at exact head 24a3a6711870a427bd82f2b8f03b86b4f52b37c4 against base 8b60ddffa89682c03238ddce6595f531eb2b6f29. One P2 inline, plus two P3 verification gaps below. No checks have run on this head yet.

P3 — switching to the session you are already on.goToSession has no same-session short-circuit, so /session <current-id> mid-turn bumps the epoch, orphans the live drain, and prints Detached from the running Turn while the user is still on the same session. Whether this is harmless depends on what driver.switchSession does with its own session id, which we could not settle from the client side. A test pinning the intended behaviour would close it either way.

P3 — re-attaching to the same still-running Turn. The new tests cover attaching to a different session's live turn. Detaching and immediately re-attaching to the same one is the case where a double consumer would show up if the orphan tail and the new attach ever overlapped; the startPendingAttachedTurn no-op guards look like they prevent it, but nothing pins it.

Neither P3 blocks. The P2 does, and it is small to fix.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

// tail unwinds through the superseded branch and releases busy/activity,
// then either that tail or the startPendingAttachedTurn below starts the
// freshly attached Turn, whichever observes an idle runner first.
const switchAwayMidTurn = async (sessionId: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] switchAwayMidTurn is re-entrant, and a second detach clears detaching while the first is still in flight.

Mid-turn, both /session entry points deliberately skip runControl's serial lock — that is the point of the change, since the lock is held by the running Turn. Both are also invoked fire-and-forget: void goToSession(sessionId) and void openSessionPicker(). So nothing serializes switchAwayMidTurn against itself, and it has an await boundary in the middle of it (await input.driver.switchSession(sessionId)).

Sequence: the user types /session s2. While that request is in flight they type /session s3 — normal impatience, and the window is as long as the round trip. Now two switchAwayMidTurn calls are live.

The consequence we care about is the finally block. detaching is a boolean, so when the first call settles it runs detaching = false while the second is still awaiting its switch. That reopens requestTurnInterrupt, whose new guard exists precisely for this window — your own comment there says "a stop here would abort whatever that Session has attached." A Ctrl+C landing in that reopened window calls driver.stop() against the freshly adopted session, which is the exact outcome this PR promises cannot happen. The invariant is stated correctly in the code and then broken by re-entry.

Two smaller effects ride along: applySwitchResult runs twice in whatever order the responses return, so the user can end up on s2 after asking for s3; and two Detached from the running Turn notices land in the adopted transcript.

The mechanism to fix it already exists — detaching is shaped like a re-entrancy flag, it just is not read at the entry points. Rejecting the second detach is enough:

constgoToSession=async(sessionId: string): Promise<void>=>{if(!turnRunning){awaitrunControl(()=>switchSession(sessionId));return;}if(detaching)return;// a detach is already handing this view overawaitswitchAwayMidTurn(sessionId).catch(reportError);};

with the same guard on openSessionPicker. Turning detaching into a counter would also work but is more machinery than the situation needs — a second detach during a handoff has no meaningful semantics, so dropping it is the honest behaviour.

A regression test would issue a second /session before the first switchSession promise resolves and assert that driver.stop() is still never called and that exactly one Detached notice appears. The existing failNextSwitch test already shows the harness can control that promise's timing.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed both mechanisms locally before fixing (reproduced the double switchSession and the early detaching clear with a parked first switch), fixed at 332404759:

  • goToSession now rejects while detaching is held. The picker path needed no separate guard — its selection routes through goToSession (:2179), so one guard covers both entry points.
  • Pinned by a test: second mid-turn /session during a parked switch yields exactly one detach notice, no extra stop(), single adoption.

One nuance worth flagging: after the blocked re-entry, the queued command text can replay through the idle path once the adopted turn settles, producing a benign second switchSession to the same id — that is pre-existing queue behavior outside this window, not a reopen of it.

Comment on lines +1152 to +1155
// A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
// drain: from that point every callback below must stop touching shared
// runner state — the adopted Session owns it now.
const superseded = () => epoch !== turnEpoch;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This comment says every callback below stops touching shared runner state once the epoch is bumped, but two of them do not check.

superseded() is consulted at :1226 (onEvent), :1263 and :1278. It is not consulted in:

  • onPrepared (:1197-1210) — the non-attached branch ends in if (turn.summary) adoptSessionMetadata(turn.summary), which writes title/cwd/model on the runner
  • onSkillInvocation (:1211-1222) — splices state.entries and calls showSkillInvocation

Both are reachable after a switch-away, because turnRunning is set at the top of runAgentTurn while pi-tui-turn.ts:87-88 only invokes these two afterawait preparePrompt resolves. So the whole preparePrompt window is a period where /session can already enter switchAwayMidTurn, and neither callback has returned yet.

Concrete sequence:

  1. a turn stalls in preparePrompt (slow or hung)
  2. /session <other>switchSession succeeds, turnEpoch bumps, applySwitchResult replaces the transcript with the adopted session
  3. preparePrompt finally resolves
  4. onPrepared runs with the abandoned turn's summary and calls adoptSessionMetadata on it

The screen now describes the old session while the driver is on the new one, and the next prompt goes to the new one. The skill callback has the same shape: an overlay belonging to the abandoned session appearing over the adopted viewport.

shouldAbort doesn't help — it only reads closed, not the epoch.

Fix looks like the same one-liner already used in the three fenced callbacks: if (superseded()) return; at the top of both, including the onSkillInvocation on the SkillInvocationBlockedError path. A test that switches while preparePrompt is still unresolved, then asserts the metadata still belongs to the adopted session, would pin it.

This is separate from the re-entrant detaching P2 already reported inline on this head — that one is about entering the path twice, this one is about callbacks that were already in flight when it was entered once.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed before fixing: reproduced an abandoned Turn adopting its summary over the adopted session when preparePrompt resolved after a completed switch-away. Fixed at 332404759onPrepared and onSkillInvocation now open with the same superseded() fence as the other callbacks, which also covers the SkillInvocationBlockedError invocation since both call sites share the callback. New test parks preparePrompt, detaches mid-park, then resolves: the abandoned title/cwd never reach the runner metadata and the skill card cannot land on the adopted viewport. CLI suite 362/362, biome clean.

@Astro-Han

Copy link
Copy Markdown
Contributor

Second review pass on 24a3a6711870a427bd82f2b8f03b86b4f52b37c4, from a different model and a different route than the earlier one on this head. One new [P2], left inline at the epoch fence: #discussion_r3837660462.

The mechanism itself holds up, and we checked the thing that usually goes wrong with an escape path — what happens to the thing you escaped from:

  • The Host Turn is not stopped.switchAwayMidTurn never calls driver.stop() (verified: zero stop calls in the covering tests), which is the correct asymmetry against the interrupt path. In Runtime Host mode the Turn is Host-owned and this TUI was only its viewport, so detaching the view should not end the work.
  • A failed switch does not orphan the live drain.turnEpoch += 1 happens only after await switchSession resolves, so a throw leaves the in-flight drain fully live — pinned by a test asserting late deltas still land in the same transcript.
  • The adopted Turn cannot start twice.startPendingAttachedTurn no-ops while busy || turnRunning, and the old drain's finishTurnUi clears turnRunning before releasing; whichever of the two observes an idle runner first starts it.
  • Escape on the picker doesn't interrupt. Early return while sessionPickerOverlayOpen, with a test asserting no stop call.

On structure: one epoch counter, not a second authority over Turn lifetime. That's the right shape for this.

Two things noted without grading, since neither is this PR's job to fix:

  • Removing /transcript and the TranscriptViewerOverlay reference is a behaviour deletion unrelated to detach. The viewer module file itself isn't in this diff, so it will be left behind — worth a follow-up so it doesn't linger as dead code.
  • /session <current id> still detaches from itself; goToSession has no short-circuit. Already reported previously, not re-graded here.

The re-entrant detaching [P2] already on this head stands on its own; we looked at it independently and agree, and are deliberately not re-grading it. The finding above is a different failure: not entering the path twice, but callbacks that were already in flight when it was entered once.

On CI: the run on this head was sitting at action_required and had never executed — I've approved it, so there should be a real signal shortly. Not approving while the P2 above and the existing one are open.

Two review findings on the apache#3380 detach path:
- switchAwayMidTurn was re-entrant: a second mid-turn /session while the
first was still handing the view over cleared the detaching flag early,
reopening the interrupt window and double-applying adoption. The entry
now rejects while a detach is in flight (the picker selection routes
through the same guard).
- onPrepared and onSkillInvocation ran without the superseded() fence the
other callbacks use. Both are reachable after a switch-away because they
fire only after preparePrompt resolves, so an abandoned Turn could still
adopt its metadata onto the adopted Session's view and surface its skill
card over the adopted viewport.
Both fixes are pinned by tests: a parked second /session yields exactly
one detach notice, and a Turn prepared across a detach can no longer
overwrite the adopted session's title/cwd.
Generated-by: maka

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 3324047598651d6b725bdb579cb66c3b0eeb99cc. Required test is completed / success bound to that exact SHA. No P0–P2.

Re-review at the current head. Both earlier findings were re-derived from the code rather than taken as fixed.

Both [P2]s are closed.

The re-entrant detach is guarded at the entry point, and — importantly — the guard is not cleared early: if (detaching) return; at pi-tui-runner.ts:1625 sits behind a comment explaining that a second detach arriving while the first is still handing the view over would otherwise reset the flag and reopen the path. That is the actual failure mode, closed at the right place.

The epoch fence is now complete. superseded() is consulted at five call sites (:1201, :1219, :1234, :1271, :1286) rather than three, so the two callbacks that previously kept touching shared runner state after the epoch bump no longer do. The comment claiming every callback below the fence checks it is now true — before, the comment described an intent the code did not implement, which is the more dangerous of the two states.

The production increment is +12 lines. It fixes exactly these two holes and introduces nothing else.

Residual, non-blocking: the same-session detach [P3] stands; detaching and immediately reattaching to the same running Turn produces a two-consumer situation that no test currently pins; and the /transcript dead-module follow-up is unchanged.

Stated rather than implied: the local CLI suite and a real multi-client Host were not exercised here — the evidence is the hosted test on this exact head plus code-level verification of both fixes.

Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review — merge still needs a committer other than the author to give LGTM and to decide.

@Astro-Han
Astro-Han merged commit bfcd3da into apache:mainAug 23, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): a second TUI attached to a Session with a running Turn is trapped — every exit path aborts the Turn

2 participants

@me2seeks@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(cli): let /session detach from a running Turn instead of trapping the client by me2seeks · Pull Request #3498 · apache/maka · GitHub
Skip to content

fix(cli): let /session detach from a running Turn instead of trapping the client - #3498

Merged
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch
Aug 23, 2026
Merged

fix(cli): let /session detach from a running Turn instead of trapping the client#3498
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch

Conversation

@me2seeks

@me2seeksme2seeks commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes#3380

Problem

A second TUI that switched onto a Session with an in-flight Turn (via /session or the picker) was trapped: /session was intercepted with "Cannot run /session while a turn is running", and every apparent way out was destructive — following the hint (Esc/Ctrl+C) or quitting called driver.stop(), aborting the shared Turn another client was watching. The only clean escape was kill -9.

Multi-client attach is a designed Runtime Host capability: the Turn is Host-owned and each TUI is only its viewport. Desktop already treats session switching as pure view navigation; this fix brings the CLI to the same model.

Fix: let /session detach from a running Turn instead of touching it

  • New 'switch' mid-turn slash disposition (alongside 'local' from refactor(cli): move the mid-turn slash disposition onto the command spec #3379). Routed through like 'local', but its handler must use busy-aware wrappers: idle it runs under runControl's serial lock as before; mid-turn the switch goes through the new detach path instead of silently no-oping on runControl's busy gate.
  • switchAwayMidTurn never calls driver.stop(). After driver.switchSession confirms, a monotonic turnEpoch fence orphans the in-flight drain:
    • late events and synthesized stream failures ("ended without a completion event") from the abandoned Session can no longer reach the adopted transcript;
    • the orphaned runAgentTurn tail skips all old-session continuations (queue flushes would steer the NEW Session), releases busy/activity, and starts the freshly attached Turn exactly once — whichever of the detach path or the orphan tail observes an idle runner first.
  • Interrupt guard during handoff: requestTurnInterrupt is swallowed while a detach is in flight — the driver already points at the next Session, so a stop there would abort whatever that Session has attached.
  • Escape owns the picker: while the mid-turn session picker is open, Escape closes it instead of arming the double-Escape interrupt for the Turn being left running.
  • Foreign import rows hidden mid-turn: the Claude Code/Codex import flow starts a new Session and cannot detach.

Scope notes

Testing

  • 3 new tests in pi-tui-runner.test.ts with a parking-turn driver:
    • mid-turn /session <id>: switches with zero stop() calls, replaces the transcript, fences leaked events, starts the attached Turn after the orphan unwinds, lands follow-ups on the adopted Session;
    • /session opens the picker mid-turn and Escape closes it without arming an interrupt;
    • a failed switch reports the error and leaves the running Turn streaming normally.
  • Full CLI suite: 360/360 pass; biome + typecheck clean.

AI use

  • Generative tooling made a substantive contribution
  • No generative tool made a substantive contribution

Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push. Generated-by: Maka trailers are present on the branch commits.

@me2seeks

Copy link
Copy Markdown
ContributorAuthor

Working on this PR — happy to iterate on review feedback. Local verification: CLI suite 360/360, biome + typecheck clean.

… the client
A second TUI that switched onto a Session with an in-flight Turn was
trapped: every exit path was destructive. /session was intercepted with
'Cannot run /session while a turn is running', and following the hint
(Esc/Ctrl+C) or quitting aborted the shared Turn via driver.stop() —
killing work another client was watching (apache#3380).
Multi-client attach is a designed Runtime Host capability: the Turn is
Host-owned and the TUI is only its viewport, so switching Sessions
mid-turn is view navigation, not a session mutation.
- new 'switch' mid-turn slash disposition alongside 'local': routed
through like 'local', but its handler must use the busy-aware
goToSession/openSessionPicker wrappers (runControl's serial lock is
held by the running Turn mid-turn)
- switchAwayMidTurn adopts the next Session without ever calling
driver.stop(); a turnEpoch fence orphans the in-flight drain after the
switch is confirmed so late events, synthesized stream failures, and
old-session queue flushes can never reach the adopted transcript; the
orphaned runAgentTurn tail releases busy/activity and hands the freshly
attached Turn its start exactly once
- requestTurnInterrupt is swallowed while a detach handoff is in flight:
the driver already points at the next Session, so a stop there would
abort whatever that Session has attached
- Escape closes the mid-turn session picker instead of arming the
double-Escape interrupt for the Turn being left running
- foreign-session import rows are hidden from the picker mid-turn (the
import flow starts a new Session; it cannot detach)
Generated-by: Maka
…switch recovery
- '/session <id>' mid-turn: switches without driver.stop(), replaces the
transcript with the adopted Session's history, fences late events from
the abandoned drain (no content leak, no synthesized 'ended without a
completion event' failure), starts the freshly attached Turn only
after the orphaned drain unwinds, and lands follow-up prompts on the
adopted Session
- '/session' mid-turn opens the picker; Escape closes it and must not
arm the double-Escape interrupt (stopCalls stays 0)
- a rejected switch leaves the running Turn fully live: error notice,
no stop, subsequent events still render into the same transcript
Generated-by: Maka
@me2seeks
me2seeksforce-pushed the fix/3380-mid-turn-session-switch branch from 21e2611 to 24a3a67CompareAugust 22, 2026 13:43

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this is a well-argued fix and the framing is right: in Runtime Host mode the Turn belongs to the Host and this TUI is only a viewport, so refusing /session mid-turn was trapping the user in a view rather than protecting anything. The monotonic turnEpoch fence is the correct shape for it — one counter makes every callback of the abandoned drain a no-op, instead of trying to unsubscribe them individually. Fencing only afterswitchSession resolves, so a failed switch leaves the drain fully live, is a detail that is easy to get backwards and you got it right.

Reviewed at exact head 24a3a6711870a427bd82f2b8f03b86b4f52b37c4 against base 8b60ddffa89682c03238ddce6595f531eb2b6f29. One P2 inline, plus two P3 verification gaps below. No checks have run on this head yet.

P3 — switching to the session you are already on.goToSession has no same-session short-circuit, so /session <current-id> mid-turn bumps the epoch, orphans the live drain, and prints Detached from the running Turn while the user is still on the same session. Whether this is harmless depends on what driver.switchSession does with its own session id, which we could not settle from the client side. A test pinning the intended behaviour would close it either way.

P3 — re-attaching to the same still-running Turn. The new tests cover attaching to a different session's live turn. Detaching and immediately re-attaching to the same one is the case where a double consumer would show up if the orphan tail and the new attach ever overlapped; the startPendingAttachedTurn no-op guards look like they prevent it, but nothing pins it.

Neither P3 blocks. The P2 does, and it is small to fix.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

// tail unwinds through the superseded branch and releases busy/activity,
// then either that tail or the startPendingAttachedTurn below starts the
// freshly attached Turn, whichever observes an idle runner first.
const switchAwayMidTurn = async (sessionId: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] switchAwayMidTurn is re-entrant, and a second detach clears detaching while the first is still in flight.

Mid-turn, both /session entry points deliberately skip runControl's serial lock — that is the point of the change, since the lock is held by the running Turn. Both are also invoked fire-and-forget: void goToSession(sessionId) and void openSessionPicker(). So nothing serializes switchAwayMidTurn against itself, and it has an await boundary in the middle of it (await input.driver.switchSession(sessionId)).

Sequence: the user types /session s2. While that request is in flight they type /session s3 — normal impatience, and the window is as long as the round trip. Now two switchAwayMidTurn calls are live.

The consequence we care about is the finally block. detaching is a boolean, so when the first call settles it runs detaching = false while the second is still awaiting its switch. That reopens requestTurnInterrupt, whose new guard exists precisely for this window — your own comment there says "a stop here would abort whatever that Session has attached." A Ctrl+C landing in that reopened window calls driver.stop() against the freshly adopted session, which is the exact outcome this PR promises cannot happen. The invariant is stated correctly in the code and then broken by re-entry.

Two smaller effects ride along: applySwitchResult runs twice in whatever order the responses return, so the user can end up on s2 after asking for s3; and two Detached from the running Turn notices land in the adopted transcript.

The mechanism to fix it already exists — detaching is shaped like a re-entrancy flag, it just is not read at the entry points. Rejecting the second detach is enough:

constgoToSession=async(sessionId: string): Promise<void>=>{if(!turnRunning){awaitrunControl(()=>switchSession(sessionId));return;}if(detaching)return;// a detach is already handing this view overawaitswitchAwayMidTurn(sessionId).catch(reportError);};

with the same guard on openSessionPicker. Turning detaching into a counter would also work but is more machinery than the situation needs — a second detach during a handoff has no meaningful semantics, so dropping it is the honest behaviour.

A regression test would issue a second /session before the first switchSession promise resolves and assert that driver.stop() is still never called and that exactly one Detached notice appears. The existing failNextSwitch test already shows the harness can control that promise's timing.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed both mechanisms locally before fixing (reproduced the double switchSession and the early detaching clear with a parked first switch), fixed at 332404759:

  • goToSession now rejects while detaching is held. The picker path needed no separate guard — its selection routes through goToSession (:2179), so one guard covers both entry points.
  • Pinned by a test: second mid-turn /session during a parked switch yields exactly one detach notice, no extra stop(), single adoption.

One nuance worth flagging: after the blocked re-entry, the queued command text can replay through the idle path once the adopted turn settles, producing a benign second switchSession to the same id — that is pre-existing queue behavior outside this window, not a reopen of it.

Comment on lines +1152 to +1155
// A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
// drain: from that point every callback below must stop touching shared
// runner state — the adopted Session owns it now.
const superseded = () => epoch !== turnEpoch;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This comment says every callback below stops touching shared runner state once the epoch is bumped, but two of them do not check.

superseded() is consulted at :1226 (onEvent), :1263 and :1278. It is not consulted in:

  • onPrepared (:1197-1210) — the non-attached branch ends in if (turn.summary) adoptSessionMetadata(turn.summary), which writes title/cwd/model on the runner
  • onSkillInvocation (:1211-1222) — splices state.entries and calls showSkillInvocation

Both are reachable after a switch-away, because turnRunning is set at the top of runAgentTurn while pi-tui-turn.ts:87-88 only invokes these two afterawait preparePrompt resolves. So the whole preparePrompt window is a period where /session can already enter switchAwayMidTurn, and neither callback has returned yet.

Concrete sequence:

  1. a turn stalls in preparePrompt (slow or hung)
  2. /session <other>switchSession succeeds, turnEpoch bumps, applySwitchResult replaces the transcript with the adopted session
  3. preparePrompt finally resolves
  4. onPrepared runs with the abandoned turn's summary and calls adoptSessionMetadata on it

The screen now describes the old session while the driver is on the new one, and the next prompt goes to the new one. The skill callback has the same shape: an overlay belonging to the abandoned session appearing over the adopted viewport.

shouldAbort doesn't help — it only reads closed, not the epoch.

Fix looks like the same one-liner already used in the three fenced callbacks: if (superseded()) return; at the top of both, including the onSkillInvocation on the SkillInvocationBlockedError path. A test that switches while preparePrompt is still unresolved, then asserts the metadata still belongs to the adopted session, would pin it.

This is separate from the re-entrant detaching P2 already reported inline on this head — that one is about entering the path twice, this one is about callbacks that were already in flight when it was entered once.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed before fixing: reproduced an abandoned Turn adopting its summary over the adopted session when preparePrompt resolved after a completed switch-away. Fixed at 332404759onPrepared and onSkillInvocation now open with the same superseded() fence as the other callbacks, which also covers the SkillInvocationBlockedError invocation since both call sites share the callback. New test parks preparePrompt, detaches mid-park, then resolves: the abandoned title/cwd never reach the runner metadata and the skill card cannot land on the adopted viewport. CLI suite 362/362, biome clean.

@Astro-Han

Copy link
Copy Markdown
Contributor

Second review pass on 24a3a6711870a427bd82f2b8f03b86b4f52b37c4, from a different model and a different route than the earlier one on this head. One new [P2], left inline at the epoch fence: #discussion_r3837660462.

The mechanism itself holds up, and we checked the thing that usually goes wrong with an escape path — what happens to the thing you escaped from:

  • The Host Turn is not stopped.switchAwayMidTurn never calls driver.stop() (verified: zero stop calls in the covering tests), which is the correct asymmetry against the interrupt path. In Runtime Host mode the Turn is Host-owned and this TUI was only its viewport, so detaching the view should not end the work.
  • A failed switch does not orphan the live drain.turnEpoch += 1 happens only after await switchSession resolves, so a throw leaves the in-flight drain fully live — pinned by a test asserting late deltas still land in the same transcript.
  • The adopted Turn cannot start twice.startPendingAttachedTurn no-ops while busy || turnRunning, and the old drain's finishTurnUi clears turnRunning before releasing; whichever of the two observes an idle runner first starts it.
  • Escape on the picker doesn't interrupt. Early return while sessionPickerOverlayOpen, with a test asserting no stop call.

On structure: one epoch counter, not a second authority over Turn lifetime. That's the right shape for this.

Two things noted without grading, since neither is this PR's job to fix:

  • Removing /transcript and the TranscriptViewerOverlay reference is a behaviour deletion unrelated to detach. The viewer module file itself isn't in this diff, so it will be left behind — worth a follow-up so it doesn't linger as dead code.
  • /session <current id> still detaches from itself; goToSession has no short-circuit. Already reported previously, not re-graded here.

The re-entrant detaching [P2] already on this head stands on its own; we looked at it independently and agree, and are deliberately not re-grading it. The finding above is a different failure: not entering the path twice, but callbacks that were already in flight when it was entered once.

On CI: the run on this head was sitting at action_required and had never executed — I've approved it, so there should be a real signal shortly. Not approving while the P2 above and the existing one are open.

Two review findings on the apache#3380 detach path:
- switchAwayMidTurn was re-entrant: a second mid-turn /session while the
first was still handing the view over cleared the detaching flag early,
reopening the interrupt window and double-applying adoption. The entry
now rejects while a detach is in flight (the picker selection routes
through the same guard).
- onPrepared and onSkillInvocation ran without the superseded() fence the
other callbacks use. Both are reachable after a switch-away because they
fire only after preparePrompt resolves, so an abandoned Turn could still
adopt its metadata onto the adopted Session's view and surface its skill
card over the adopted viewport.
Both fixes are pinned by tests: a parked second /session yields exactly
one detach notice, and a Turn prepared across a detach can no longer
overwrite the adopted session's title/cwd.
Generated-by: maka

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 3324047598651d6b725bdb579cb66c3b0eeb99cc. Required test is completed / success bound to that exact SHA. No P0–P2.

Re-review at the current head. Both earlier findings were re-derived from the code rather than taken as fixed.

Both [P2]s are closed.

The re-entrant detach is guarded at the entry point, and — importantly — the guard is not cleared early: if (detaching) return; at pi-tui-runner.ts:1625 sits behind a comment explaining that a second detach arriving while the first is still handing the view over would otherwise reset the flag and reopen the path. That is the actual failure mode, closed at the right place.

The epoch fence is now complete. superseded() is consulted at five call sites (:1201, :1219, :1234, :1271, :1286) rather than three, so the two callbacks that previously kept touching shared runner state after the epoch bump no longer do. The comment claiming every callback below the fence checks it is now true — before, the comment described an intent the code did not implement, which is the more dangerous of the two states.

The production increment is +12 lines. It fixes exactly these two holes and introduces nothing else.

Residual, non-blocking: the same-session detach [P3] stands; detaching and immediately reattaching to the same running Turn produces a two-consumer situation that no test currently pins; and the /transcript dead-module follow-up is unchanged.

Stated rather than implied: the local CLI suite and a real multi-client Host were not exercised here — the evidence is the hosted test on this exact head plus code-level verification of both fixes.

Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review — merge still needs a committer other than the author to give LGTM and to decide.

@Astro-Han
Astro-Han merged commit bfcd3da into apache:mainAug 23, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): a second TUI attached to a Session with a running Turn is trapped — every exit path aborts the Turn

2 participants

@me2seeks@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(cli): let /session detach from a running Turn instead of trapping the client by me2seeks · Pull Request #3498 · apache/maka · GitHub
Skip to content

fix(cli): let /session detach from a running Turn instead of trapping the client - #3498

Merged
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch
Aug 23, 2026
Merged

fix(cli): let /session detach from a running Turn instead of trapping the client#3498
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch

Conversation

@me2seeks

@me2seeksme2seeks commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes#3380

Problem

A second TUI that switched onto a Session with an in-flight Turn (via /session or the picker) was trapped: /session was intercepted with "Cannot run /session while a turn is running", and every apparent way out was destructive — following the hint (Esc/Ctrl+C) or quitting called driver.stop(), aborting the shared Turn another client was watching. The only clean escape was kill -9.

Multi-client attach is a designed Runtime Host capability: the Turn is Host-owned and each TUI is only its viewport. Desktop already treats session switching as pure view navigation; this fix brings the CLI to the same model.

Fix: let /session detach from a running Turn instead of touching it

  • New 'switch' mid-turn slash disposition (alongside 'local' from refactor(cli): move the mid-turn slash disposition onto the command spec #3379). Routed through like 'local', but its handler must use busy-aware wrappers: idle it runs under runControl's serial lock as before; mid-turn the switch goes through the new detach path instead of silently no-oping on runControl's busy gate.
  • switchAwayMidTurn never calls driver.stop(). After driver.switchSession confirms, a monotonic turnEpoch fence orphans the in-flight drain:
    • late events and synthesized stream failures ("ended without a completion event") from the abandoned Session can no longer reach the adopted transcript;
    • the orphaned runAgentTurn tail skips all old-session continuations (queue flushes would steer the NEW Session), releases busy/activity, and starts the freshly attached Turn exactly once — whichever of the detach path or the orphan tail observes an idle runner first.
  • Interrupt guard during handoff: requestTurnInterrupt is swallowed while a detach is in flight — the driver already points at the next Session, so a stop there would abort whatever that Session has attached.
  • Escape owns the picker: while the mid-turn session picker is open, Escape closes it instead of arming the double-Escape interrupt for the Turn being left running.
  • Foreign import rows hidden mid-turn: the Claude Code/Codex import flow starts a new Session and cannot detach.

Scope notes

Testing

  • 3 new tests in pi-tui-runner.test.ts with a parking-turn driver:
    • mid-turn /session <id>: switches with zero stop() calls, replaces the transcript, fences leaked events, starts the attached Turn after the orphan unwinds, lands follow-ups on the adopted Session;
    • /session opens the picker mid-turn and Escape closes it without arming an interrupt;
    • a failed switch reports the error and leaves the running Turn streaming normally.
  • Full CLI suite: 360/360 pass; biome + typecheck clean.

AI use

  • Generative tooling made a substantive contribution
  • No generative tool made a substantive contribution

Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push. Generated-by: Maka trailers are present on the branch commits.

@me2seeks

Copy link
Copy Markdown
ContributorAuthor

Working on this PR — happy to iterate on review feedback. Local verification: CLI suite 360/360, biome + typecheck clean.

… the client
A second TUI that switched onto a Session with an in-flight Turn was
trapped: every exit path was destructive. /session was intercepted with
'Cannot run /session while a turn is running', and following the hint
(Esc/Ctrl+C) or quitting aborted the shared Turn via driver.stop() —
killing work another client was watching (apache#3380).
Multi-client attach is a designed Runtime Host capability: the Turn is
Host-owned and the TUI is only its viewport, so switching Sessions
mid-turn is view navigation, not a session mutation.
- new 'switch' mid-turn slash disposition alongside 'local': routed
through like 'local', but its handler must use the busy-aware
goToSession/openSessionPicker wrappers (runControl's serial lock is
held by the running Turn mid-turn)
- switchAwayMidTurn adopts the next Session without ever calling
driver.stop(); a turnEpoch fence orphans the in-flight drain after the
switch is confirmed so late events, synthesized stream failures, and
old-session queue flushes can never reach the adopted transcript; the
orphaned runAgentTurn tail releases busy/activity and hands the freshly
attached Turn its start exactly once
- requestTurnInterrupt is swallowed while a detach handoff is in flight:
the driver already points at the next Session, so a stop there would
abort whatever that Session has attached
- Escape closes the mid-turn session picker instead of arming the
double-Escape interrupt for the Turn being left running
- foreign-session import rows are hidden from the picker mid-turn (the
import flow starts a new Session; it cannot detach)
Generated-by: Maka
…switch recovery
- '/session <id>' mid-turn: switches without driver.stop(), replaces the
transcript with the adopted Session's history, fences late events from
the abandoned drain (no content leak, no synthesized 'ended without a
completion event' failure), starts the freshly attached Turn only
after the orphaned drain unwinds, and lands follow-up prompts on the
adopted Session
- '/session' mid-turn opens the picker; Escape closes it and must not
arm the double-Escape interrupt (stopCalls stays 0)
- a rejected switch leaves the running Turn fully live: error notice,
no stop, subsequent events still render into the same transcript
Generated-by: Maka
@me2seeks
me2seeksforce-pushed the fix/3380-mid-turn-session-switch branch from 21e2611 to 24a3a67CompareAugust 22, 2026 13:43

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this is a well-argued fix and the framing is right: in Runtime Host mode the Turn belongs to the Host and this TUI is only a viewport, so refusing /session mid-turn was trapping the user in a view rather than protecting anything. The monotonic turnEpoch fence is the correct shape for it — one counter makes every callback of the abandoned drain a no-op, instead of trying to unsubscribe them individually. Fencing only afterswitchSession resolves, so a failed switch leaves the drain fully live, is a detail that is easy to get backwards and you got it right.

Reviewed at exact head 24a3a6711870a427bd82f2b8f03b86b4f52b37c4 against base 8b60ddffa89682c03238ddce6595f531eb2b6f29. One P2 inline, plus two P3 verification gaps below. No checks have run on this head yet.

P3 — switching to the session you are already on.goToSession has no same-session short-circuit, so /session <current-id> mid-turn bumps the epoch, orphans the live drain, and prints Detached from the running Turn while the user is still on the same session. Whether this is harmless depends on what driver.switchSession does with its own session id, which we could not settle from the client side. A test pinning the intended behaviour would close it either way.

P3 — re-attaching to the same still-running Turn. The new tests cover attaching to a different session's live turn. Detaching and immediately re-attaching to the same one is the case where a double consumer would show up if the orphan tail and the new attach ever overlapped; the startPendingAttachedTurn no-op guards look like they prevent it, but nothing pins it.

Neither P3 blocks. The P2 does, and it is small to fix.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

// tail unwinds through the superseded branch and releases busy/activity,
// then either that tail or the startPendingAttachedTurn below starts the
// freshly attached Turn, whichever observes an idle runner first.
const switchAwayMidTurn = async (sessionId: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] switchAwayMidTurn is re-entrant, and a second detach clears detaching while the first is still in flight.

Mid-turn, both /session entry points deliberately skip runControl's serial lock — that is the point of the change, since the lock is held by the running Turn. Both are also invoked fire-and-forget: void goToSession(sessionId) and void openSessionPicker(). So nothing serializes switchAwayMidTurn against itself, and it has an await boundary in the middle of it (await input.driver.switchSession(sessionId)).

Sequence: the user types /session s2. While that request is in flight they type /session s3 — normal impatience, and the window is as long as the round trip. Now two switchAwayMidTurn calls are live.

The consequence we care about is the finally block. detaching is a boolean, so when the first call settles it runs detaching = false while the second is still awaiting its switch. That reopens requestTurnInterrupt, whose new guard exists precisely for this window — your own comment there says "a stop here would abort whatever that Session has attached." A Ctrl+C landing in that reopened window calls driver.stop() against the freshly adopted session, which is the exact outcome this PR promises cannot happen. The invariant is stated correctly in the code and then broken by re-entry.

Two smaller effects ride along: applySwitchResult runs twice in whatever order the responses return, so the user can end up on s2 after asking for s3; and two Detached from the running Turn notices land in the adopted transcript.

The mechanism to fix it already exists — detaching is shaped like a re-entrancy flag, it just is not read at the entry points. Rejecting the second detach is enough:

constgoToSession=async(sessionId: string): Promise<void>=>{if(!turnRunning){awaitrunControl(()=>switchSession(sessionId));return;}if(detaching)return;// a detach is already handing this view overawaitswitchAwayMidTurn(sessionId).catch(reportError);};

with the same guard on openSessionPicker. Turning detaching into a counter would also work but is more machinery than the situation needs — a second detach during a handoff has no meaningful semantics, so dropping it is the honest behaviour.

A regression test would issue a second /session before the first switchSession promise resolves and assert that driver.stop() is still never called and that exactly one Detached notice appears. The existing failNextSwitch test already shows the harness can control that promise's timing.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed both mechanisms locally before fixing (reproduced the double switchSession and the early detaching clear with a parked first switch), fixed at 332404759:

  • goToSession now rejects while detaching is held. The picker path needed no separate guard — its selection routes through goToSession (:2179), so one guard covers both entry points.
  • Pinned by a test: second mid-turn /session during a parked switch yields exactly one detach notice, no extra stop(), single adoption.

One nuance worth flagging: after the blocked re-entry, the queued command text can replay through the idle path once the adopted turn settles, producing a benign second switchSession to the same id — that is pre-existing queue behavior outside this window, not a reopen of it.

Comment on lines +1152 to +1155
// A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
// drain: from that point every callback below must stop touching shared
// runner state — the adopted Session owns it now.
const superseded = () => epoch !== turnEpoch;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This comment says every callback below stops touching shared runner state once the epoch is bumped, but two of them do not check.

superseded() is consulted at :1226 (onEvent), :1263 and :1278. It is not consulted in:

  • onPrepared (:1197-1210) — the non-attached branch ends in if (turn.summary) adoptSessionMetadata(turn.summary), which writes title/cwd/model on the runner
  • onSkillInvocation (:1211-1222) — splices state.entries and calls showSkillInvocation

Both are reachable after a switch-away, because turnRunning is set at the top of runAgentTurn while pi-tui-turn.ts:87-88 only invokes these two afterawait preparePrompt resolves. So the whole preparePrompt window is a period where /session can already enter switchAwayMidTurn, and neither callback has returned yet.

Concrete sequence:

  1. a turn stalls in preparePrompt (slow or hung)
  2. /session <other>switchSession succeeds, turnEpoch bumps, applySwitchResult replaces the transcript with the adopted session
  3. preparePrompt finally resolves
  4. onPrepared runs with the abandoned turn's summary and calls adoptSessionMetadata on it

The screen now describes the old session while the driver is on the new one, and the next prompt goes to the new one. The skill callback has the same shape: an overlay belonging to the abandoned session appearing over the adopted viewport.

shouldAbort doesn't help — it only reads closed, not the epoch.

Fix looks like the same one-liner already used in the three fenced callbacks: if (superseded()) return; at the top of both, including the onSkillInvocation on the SkillInvocationBlockedError path. A test that switches while preparePrompt is still unresolved, then asserts the metadata still belongs to the adopted session, would pin it.

This is separate from the re-entrant detaching P2 already reported inline on this head — that one is about entering the path twice, this one is about callbacks that were already in flight when it was entered once.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed before fixing: reproduced an abandoned Turn adopting its summary over the adopted session when preparePrompt resolved after a completed switch-away. Fixed at 332404759onPrepared and onSkillInvocation now open with the same superseded() fence as the other callbacks, which also covers the SkillInvocationBlockedError invocation since both call sites share the callback. New test parks preparePrompt, detaches mid-park, then resolves: the abandoned title/cwd never reach the runner metadata and the skill card cannot land on the adopted viewport. CLI suite 362/362, biome clean.

@Astro-Han

Copy link
Copy Markdown
Contributor

Second review pass on 24a3a6711870a427bd82f2b8f03b86b4f52b37c4, from a different model and a different route than the earlier one on this head. One new [P2], left inline at the epoch fence: #discussion_r3837660462.

The mechanism itself holds up, and we checked the thing that usually goes wrong with an escape path — what happens to the thing you escaped from:

  • The Host Turn is not stopped.switchAwayMidTurn never calls driver.stop() (verified: zero stop calls in the covering tests), which is the correct asymmetry against the interrupt path. In Runtime Host mode the Turn is Host-owned and this TUI was only its viewport, so detaching the view should not end the work.
  • A failed switch does not orphan the live drain.turnEpoch += 1 happens only after await switchSession resolves, so a throw leaves the in-flight drain fully live — pinned by a test asserting late deltas still land in the same transcript.
  • The adopted Turn cannot start twice.startPendingAttachedTurn no-ops while busy || turnRunning, and the old drain's finishTurnUi clears turnRunning before releasing; whichever of the two observes an idle runner first starts it.
  • Escape on the picker doesn't interrupt. Early return while sessionPickerOverlayOpen, with a test asserting no stop call.

On structure: one epoch counter, not a second authority over Turn lifetime. That's the right shape for this.

Two things noted without grading, since neither is this PR's job to fix:

  • Removing /transcript and the TranscriptViewerOverlay reference is a behaviour deletion unrelated to detach. The viewer module file itself isn't in this diff, so it will be left behind — worth a follow-up so it doesn't linger as dead code.
  • /session <current id> still detaches from itself; goToSession has no short-circuit. Already reported previously, not re-graded here.

The re-entrant detaching [P2] already on this head stands on its own; we looked at it independently and agree, and are deliberately not re-grading it. The finding above is a different failure: not entering the path twice, but callbacks that were already in flight when it was entered once.

On CI: the run on this head was sitting at action_required and had never executed — I've approved it, so there should be a real signal shortly. Not approving while the P2 above and the existing one are open.

Two review findings on the apache#3380 detach path:
- switchAwayMidTurn was re-entrant: a second mid-turn /session while the
first was still handing the view over cleared the detaching flag early,
reopening the interrupt window and double-applying adoption. The entry
now rejects while a detach is in flight (the picker selection routes
through the same guard).
- onPrepared and onSkillInvocation ran without the superseded() fence the
other callbacks use. Both are reachable after a switch-away because they
fire only after preparePrompt resolves, so an abandoned Turn could still
adopt its metadata onto the adopted Session's view and surface its skill
card over the adopted viewport.
Both fixes are pinned by tests: a parked second /session yields exactly
one detach notice, and a Turn prepared across a detach can no longer
overwrite the adopted session's title/cwd.
Generated-by: maka

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 3324047598651d6b725bdb579cb66c3b0eeb99cc. Required test is completed / success bound to that exact SHA. No P0–P2.

Re-review at the current head. Both earlier findings were re-derived from the code rather than taken as fixed.

Both [P2]s are closed.

The re-entrant detach is guarded at the entry point, and — importantly — the guard is not cleared early: if (detaching) return; at pi-tui-runner.ts:1625 sits behind a comment explaining that a second detach arriving while the first is still handing the view over would otherwise reset the flag and reopen the path. That is the actual failure mode, closed at the right place.

The epoch fence is now complete. superseded() is consulted at five call sites (:1201, :1219, :1234, :1271, :1286) rather than three, so the two callbacks that previously kept touching shared runner state after the epoch bump no longer do. The comment claiming every callback below the fence checks it is now true — before, the comment described an intent the code did not implement, which is the more dangerous of the two states.

The production increment is +12 lines. It fixes exactly these two holes and introduces nothing else.

Residual, non-blocking: the same-session detach [P3] stands; detaching and immediately reattaching to the same running Turn produces a two-consumer situation that no test currently pins; and the /transcript dead-module follow-up is unchanged.

Stated rather than implied: the local CLI suite and a real multi-client Host were not exercised here — the evidence is the hosted test on this exact head plus code-level verification of both fixes.

Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review — merge still needs a committer other than the author to give LGTM and to decide.

@Astro-Han
Astro-Han merged commit bfcd3da into apache:mainAug 23, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): a second TUI attached to a Session with a running Turn is trapped — every exit path aborts the Turn

2 participants

@me2seeks@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix(cli): let /session detach from a running Turn instead of trapping the client by me2seeks · Pull Request #3498 · apache/maka · GitHub
Skip to content

fix(cli): let /session detach from a running Turn instead of trapping the client - #3498

Merged
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch
Aug 23, 2026
Merged

fix(cli): let /session detach from a running Turn instead of trapping the client#3498
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch

Conversation

@me2seeks

@me2seeksme2seeks commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes#3380

Problem

A second TUI that switched onto a Session with an in-flight Turn (via /session or the picker) was trapped: /session was intercepted with "Cannot run /session while a turn is running", and every apparent way out was destructive — following the hint (Esc/Ctrl+C) or quitting called driver.stop(), aborting the shared Turn another client was watching. The only clean escape was kill -9.

Multi-client attach is a designed Runtime Host capability: the Turn is Host-owned and each TUI is only its viewport. Desktop already treats session switching as pure view navigation; this fix brings the CLI to the same model.

Fix: let /session detach from a running Turn instead of touching it

  • New 'switch' mid-turn slash disposition (alongside 'local' from refactor(cli): move the mid-turn slash disposition onto the command spec #3379). Routed through like 'local', but its handler must use busy-aware wrappers: idle it runs under runControl's serial lock as before; mid-turn the switch goes through the new detach path instead of silently no-oping on runControl's busy gate.
  • switchAwayMidTurn never calls driver.stop(). After driver.switchSession confirms, a monotonic turnEpoch fence orphans the in-flight drain:
    • late events and synthesized stream failures ("ended without a completion event") from the abandoned Session can no longer reach the adopted transcript;
    • the orphaned runAgentTurn tail skips all old-session continuations (queue flushes would steer the NEW Session), releases busy/activity, and starts the freshly attached Turn exactly once — whichever of the detach path or the orphan tail observes an idle runner first.
  • Interrupt guard during handoff: requestTurnInterrupt is swallowed while a detach is in flight — the driver already points at the next Session, so a stop there would abort whatever that Session has attached.
  • Escape owns the picker: while the mid-turn session picker is open, Escape closes it instead of arming the double-Escape interrupt for the Turn being left running.
  • Foreign import rows hidden mid-turn: the Claude Code/Codex import flow starts a new Session and cannot detach.

Scope notes

Testing

  • 3 new tests in pi-tui-runner.test.ts with a parking-turn driver:
    • mid-turn /session <id>: switches with zero stop() calls, replaces the transcript, fences leaked events, starts the attached Turn after the orphan unwinds, lands follow-ups on the adopted Session;
    • /session opens the picker mid-turn and Escape closes it without arming an interrupt;
    • a failed switch reports the error and leaves the running Turn streaming normally.
  • Full CLI suite: 360/360 pass; biome + typecheck clean.

AI use

  • Generative tooling made a substantive contribution
  • No generative tool made a substantive contribution

Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push. Generated-by: Maka trailers are present on the branch commits.

@me2seeks

Copy link
Copy Markdown
ContributorAuthor

Working on this PR — happy to iterate on review feedback. Local verification: CLI suite 360/360, biome + typecheck clean.

… the client
A second TUI that switched onto a Session with an in-flight Turn was
trapped: every exit path was destructive. /session was intercepted with
'Cannot run /session while a turn is running', and following the hint
(Esc/Ctrl+C) or quitting aborted the shared Turn via driver.stop() —
killing work another client was watching (apache#3380).
Multi-client attach is a designed Runtime Host capability: the Turn is
Host-owned and the TUI is only its viewport, so switching Sessions
mid-turn is view navigation, not a session mutation.
- new 'switch' mid-turn slash disposition alongside 'local': routed
through like 'local', but its handler must use the busy-aware
goToSession/openSessionPicker wrappers (runControl's serial lock is
held by the running Turn mid-turn)
- switchAwayMidTurn adopts the next Session without ever calling
driver.stop(); a turnEpoch fence orphans the in-flight drain after the
switch is confirmed so late events, synthesized stream failures, and
old-session queue flushes can never reach the adopted transcript; the
orphaned runAgentTurn tail releases busy/activity and hands the freshly
attached Turn its start exactly once
- requestTurnInterrupt is swallowed while a detach handoff is in flight:
the driver already points at the next Session, so a stop there would
abort whatever that Session has attached
- Escape closes the mid-turn session picker instead of arming the
double-Escape interrupt for the Turn being left running
- foreign-session import rows are hidden from the picker mid-turn (the
import flow starts a new Session; it cannot detach)
Generated-by: Maka
…switch recovery
- '/session <id>' mid-turn: switches without driver.stop(), replaces the
transcript with the adopted Session's history, fences late events from
the abandoned drain (no content leak, no synthesized 'ended without a
completion event' failure), starts the freshly attached Turn only
after the orphaned drain unwinds, and lands follow-up prompts on the
adopted Session
- '/session' mid-turn opens the picker; Escape closes it and must not
arm the double-Escape interrupt (stopCalls stays 0)
- a rejected switch leaves the running Turn fully live: error notice,
no stop, subsequent events still render into the same transcript
Generated-by: Maka
@me2seeks
me2seeksforce-pushed the fix/3380-mid-turn-session-switch branch from 21e2611 to 24a3a67CompareAugust 22, 2026 13:43

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this is a well-argued fix and the framing is right: in Runtime Host mode the Turn belongs to the Host and this TUI is only a viewport, so refusing /session mid-turn was trapping the user in a view rather than protecting anything. The monotonic turnEpoch fence is the correct shape for it — one counter makes every callback of the abandoned drain a no-op, instead of trying to unsubscribe them individually. Fencing only afterswitchSession resolves, so a failed switch leaves the drain fully live, is a detail that is easy to get backwards and you got it right.

Reviewed at exact head 24a3a6711870a427bd82f2b8f03b86b4f52b37c4 against base 8b60ddffa89682c03238ddce6595f531eb2b6f29. One P2 inline, plus two P3 verification gaps below. No checks have run on this head yet.

P3 — switching to the session you are already on.goToSession has no same-session short-circuit, so /session <current-id> mid-turn bumps the epoch, orphans the live drain, and prints Detached from the running Turn while the user is still on the same session. Whether this is harmless depends on what driver.switchSession does with its own session id, which we could not settle from the client side. A test pinning the intended behaviour would close it either way.

P3 — re-attaching to the same still-running Turn. The new tests cover attaching to a different session's live turn. Detaching and immediately re-attaching to the same one is the case where a double consumer would show up if the orphan tail and the new attach ever overlapped; the startPendingAttachedTurn no-op guards look like they prevent it, but nothing pins it.

Neither P3 blocks. The P2 does, and it is small to fix.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

// tail unwinds through the superseded branch and releases busy/activity,
// then either that tail or the startPendingAttachedTurn below starts the
// freshly attached Turn, whichever observes an idle runner first.
const switchAwayMidTurn = async (sessionId: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] switchAwayMidTurn is re-entrant, and a second detach clears detaching while the first is still in flight.

Mid-turn, both /session entry points deliberately skip runControl's serial lock — that is the point of the change, since the lock is held by the running Turn. Both are also invoked fire-and-forget: void goToSession(sessionId) and void openSessionPicker(). So nothing serializes switchAwayMidTurn against itself, and it has an await boundary in the middle of it (await input.driver.switchSession(sessionId)).

Sequence: the user types /session s2. While that request is in flight they type /session s3 — normal impatience, and the window is as long as the round trip. Now two switchAwayMidTurn calls are live.

The consequence we care about is the finally block. detaching is a boolean, so when the first call settles it runs detaching = false while the second is still awaiting its switch. That reopens requestTurnInterrupt, whose new guard exists precisely for this window — your own comment there says "a stop here would abort whatever that Session has attached." A Ctrl+C landing in that reopened window calls driver.stop() against the freshly adopted session, which is the exact outcome this PR promises cannot happen. The invariant is stated correctly in the code and then broken by re-entry.

Two smaller effects ride along: applySwitchResult runs twice in whatever order the responses return, so the user can end up on s2 after asking for s3; and two Detached from the running Turn notices land in the adopted transcript.

The mechanism to fix it already exists — detaching is shaped like a re-entrancy flag, it just is not read at the entry points. Rejecting the second detach is enough:

constgoToSession=async(sessionId: string): Promise<void>=>{if(!turnRunning){awaitrunControl(()=>switchSession(sessionId));return;}if(detaching)return;// a detach is already handing this view overawaitswitchAwayMidTurn(sessionId).catch(reportError);};

with the same guard on openSessionPicker. Turning detaching into a counter would also work but is more machinery than the situation needs — a second detach during a handoff has no meaningful semantics, so dropping it is the honest behaviour.

A regression test would issue a second /session before the first switchSession promise resolves and assert that driver.stop() is still never called and that exactly one Detached notice appears. The existing failNextSwitch test already shows the harness can control that promise's timing.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed both mechanisms locally before fixing (reproduced the double switchSession and the early detaching clear with a parked first switch), fixed at 332404759:

  • goToSession now rejects while detaching is held. The picker path needed no separate guard — its selection routes through goToSession (:2179), so one guard covers both entry points.
  • Pinned by a test: second mid-turn /session during a parked switch yields exactly one detach notice, no extra stop(), single adoption.

One nuance worth flagging: after the blocked re-entry, the queued command text can replay through the idle path once the adopted turn settles, producing a benign second switchSession to the same id — that is pre-existing queue behavior outside this window, not a reopen of it.

Comment on lines +1152 to +1155
// A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
// drain: from that point every callback below must stop touching shared
// runner state — the adopted Session owns it now.
const superseded = () => epoch !== turnEpoch;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This comment says every callback below stops touching shared runner state once the epoch is bumped, but two of them do not check.

superseded() is consulted at :1226 (onEvent), :1263 and :1278. It is not consulted in:

  • onPrepared (:1197-1210) — the non-attached branch ends in if (turn.summary) adoptSessionMetadata(turn.summary), which writes title/cwd/model on the runner
  • onSkillInvocation (:1211-1222) — splices state.entries and calls showSkillInvocation

Both are reachable after a switch-away, because turnRunning is set at the top of runAgentTurn while pi-tui-turn.ts:87-88 only invokes these two afterawait preparePrompt resolves. So the whole preparePrompt window is a period where /session can already enter switchAwayMidTurn, and neither callback has returned yet.

Concrete sequence:

  1. a turn stalls in preparePrompt (slow or hung)
  2. /session <other>switchSession succeeds, turnEpoch bumps, applySwitchResult replaces the transcript with the adopted session
  3. preparePrompt finally resolves
  4. onPrepared runs with the abandoned turn's summary and calls adoptSessionMetadata on it

The screen now describes the old session while the driver is on the new one, and the next prompt goes to the new one. The skill callback has the same shape: an overlay belonging to the abandoned session appearing over the adopted viewport.

shouldAbort doesn't help — it only reads closed, not the epoch.

Fix looks like the same one-liner already used in the three fenced callbacks: if (superseded()) return; at the top of both, including the onSkillInvocation on the SkillInvocationBlockedError path. A test that switches while preparePrompt is still unresolved, then asserts the metadata still belongs to the adopted session, would pin it.

This is separate from the re-entrant detaching P2 already reported inline on this head — that one is about entering the path twice, this one is about callbacks that were already in flight when it was entered once.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed before fixing: reproduced an abandoned Turn adopting its summary over the adopted session when preparePrompt resolved after a completed switch-away. Fixed at 332404759onPrepared and onSkillInvocation now open with the same superseded() fence as the other callbacks, which also covers the SkillInvocationBlockedError invocation since both call sites share the callback. New test parks preparePrompt, detaches mid-park, then resolves: the abandoned title/cwd never reach the runner metadata and the skill card cannot land on the adopted viewport. CLI suite 362/362, biome clean.

@Astro-Han

Copy link
Copy Markdown
Contributor

Second review pass on 24a3a6711870a427bd82f2b8f03b86b4f52b37c4, from a different model and a different route than the earlier one on this head. One new [P2], left inline at the epoch fence: #discussion_r3837660462.

The mechanism itself holds up, and we checked the thing that usually goes wrong with an escape path — what happens to the thing you escaped from:

  • The Host Turn is not stopped.switchAwayMidTurn never calls driver.stop() (verified: zero stop calls in the covering tests), which is the correct asymmetry against the interrupt path. In Runtime Host mode the Turn is Host-owned and this TUI was only its viewport, so detaching the view should not end the work.
  • A failed switch does not orphan the live drain.turnEpoch += 1 happens only after await switchSession resolves, so a throw leaves the in-flight drain fully live — pinned by a test asserting late deltas still land in the same transcript.
  • The adopted Turn cannot start twice.startPendingAttachedTurn no-ops while busy || turnRunning, and the old drain's finishTurnUi clears turnRunning before releasing; whichever of the two observes an idle runner first starts it.
  • Escape on the picker doesn't interrupt. Early return while sessionPickerOverlayOpen, with a test asserting no stop call.

On structure: one epoch counter, not a second authority over Turn lifetime. That's the right shape for this.

Two things noted without grading, since neither is this PR's job to fix:

  • Removing /transcript and the TranscriptViewerOverlay reference is a behaviour deletion unrelated to detach. The viewer module file itself isn't in this diff, so it will be left behind — worth a follow-up so it doesn't linger as dead code.
  • /session <current id> still detaches from itself; goToSession has no short-circuit. Already reported previously, not re-graded here.

The re-entrant detaching [P2] already on this head stands on its own; we looked at it independently and agree, and are deliberately not re-grading it. The finding above is a different failure: not entering the path twice, but callbacks that were already in flight when it was entered once.

On CI: the run on this head was sitting at action_required and had never executed — I've approved it, so there should be a real signal shortly. Not approving while the P2 above and the existing one are open.

Two review findings on the apache#3380 detach path:
- switchAwayMidTurn was re-entrant: a second mid-turn /session while the
first was still handing the view over cleared the detaching flag early,
reopening the interrupt window and double-applying adoption. The entry
now rejects while a detach is in flight (the picker selection routes
through the same guard).
- onPrepared and onSkillInvocation ran without the superseded() fence the
other callbacks use. Both are reachable after a switch-away because they
fire only after preparePrompt resolves, so an abandoned Turn could still
adopt its metadata onto the adopted Session's view and surface its skill
card over the adopted viewport.
Both fixes are pinned by tests: a parked second /session yields exactly
one detach notice, and a Turn prepared across a detach can no longer
overwrite the adopted session's title/cwd.
Generated-by: maka

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 3324047598651d6b725bdb579cb66c3b0eeb99cc. Required test is completed / success bound to that exact SHA. No P0–P2.

Re-review at the current head. Both earlier findings were re-derived from the code rather than taken as fixed.

Both [P2]s are closed.

The re-entrant detach is guarded at the entry point, and — importantly — the guard is not cleared early: if (detaching) return; at pi-tui-runner.ts:1625 sits behind a comment explaining that a second detach arriving while the first is still handing the view over would otherwise reset the flag and reopen the path. That is the actual failure mode, closed at the right place.

The epoch fence is now complete. superseded() is consulted at five call sites (:1201, :1219, :1234, :1271, :1286) rather than three, so the two callbacks that previously kept touching shared runner state after the epoch bump no longer do. The comment claiming every callback below the fence checks it is now true — before, the comment described an intent the code did not implement, which is the more dangerous of the two states.

The production increment is +12 lines. It fixes exactly these two holes and introduces nothing else.

Residual, non-blocking: the same-session detach [P3] stands; detaching and immediately reattaching to the same running Turn produces a two-consumer situation that no test currently pins; and the /transcript dead-module follow-up is unchanged.

Stated rather than implied: the local CLI suite and a real multi-client Host were not exercised here — the evidence is the hosted test on this exact head plus code-level verification of both fixes.

Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review — merge still needs a committer other than the author to give LGTM and to decide.

@Astro-Han
Astro-Han merged commit bfcd3da into apache:mainAug 23, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): a second TUI attached to a Session with a running Turn is trapped — every exit path aborts the Turn

2 participants

@me2seeks@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(cli): let /session detach from a running Turn instead of trapping the client by me2seeks · Pull Request #3498 · apache/maka · GitHub
Skip to content

fix(cli): let /session detach from a running Turn instead of trapping the client - #3498

Merged
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch
Aug 23, 2026
Merged

fix(cli): let /session detach from a running Turn instead of trapping the client#3498
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch

Conversation

@me2seeks

@me2seeksme2seeks commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes#3380

Problem

A second TUI that switched onto a Session with an in-flight Turn (via /session or the picker) was trapped: /session was intercepted with "Cannot run /session while a turn is running", and every apparent way out was destructive — following the hint (Esc/Ctrl+C) or quitting called driver.stop(), aborting the shared Turn another client was watching. The only clean escape was kill -9.

Multi-client attach is a designed Runtime Host capability: the Turn is Host-owned and each TUI is only its viewport. Desktop already treats session switching as pure view navigation; this fix brings the CLI to the same model.

Fix: let /session detach from a running Turn instead of touching it

  • New 'switch' mid-turn slash disposition (alongside 'local' from refactor(cli): move the mid-turn slash disposition onto the command spec #3379). Routed through like 'local', but its handler must use busy-aware wrappers: idle it runs under runControl's serial lock as before; mid-turn the switch goes through the new detach path instead of silently no-oping on runControl's busy gate.
  • switchAwayMidTurn never calls driver.stop(). After driver.switchSession confirms, a monotonic turnEpoch fence orphans the in-flight drain:
    • late events and synthesized stream failures ("ended without a completion event") from the abandoned Session can no longer reach the adopted transcript;
    • the orphaned runAgentTurn tail skips all old-session continuations (queue flushes would steer the NEW Session), releases busy/activity, and starts the freshly attached Turn exactly once — whichever of the detach path or the orphan tail observes an idle runner first.
  • Interrupt guard during handoff: requestTurnInterrupt is swallowed while a detach is in flight — the driver already points at the next Session, so a stop there would abort whatever that Session has attached.
  • Escape owns the picker: while the mid-turn session picker is open, Escape closes it instead of arming the double-Escape interrupt for the Turn being left running.
  • Foreign import rows hidden mid-turn: the Claude Code/Codex import flow starts a new Session and cannot detach.

Scope notes

Testing

  • 3 new tests in pi-tui-runner.test.ts with a parking-turn driver:
    • mid-turn /session <id>: switches with zero stop() calls, replaces the transcript, fences leaked events, starts the attached Turn after the orphan unwinds, lands follow-ups on the adopted Session;
    • /session opens the picker mid-turn and Escape closes it without arming an interrupt;
    • a failed switch reports the error and leaves the running Turn streaming normally.
  • Full CLI suite: 360/360 pass; biome + typecheck clean.

AI use

  • Generative tooling made a substantive contribution
  • No generative tool made a substantive contribution

Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push. Generated-by: Maka trailers are present on the branch commits.

@me2seeks

Copy link
Copy Markdown
ContributorAuthor

Working on this PR — happy to iterate on review feedback. Local verification: CLI suite 360/360, biome + typecheck clean.

… the client
A second TUI that switched onto a Session with an in-flight Turn was
trapped: every exit path was destructive. /session was intercepted with
'Cannot run /session while a turn is running', and following the hint
(Esc/Ctrl+C) or quitting aborted the shared Turn via driver.stop() —
killing work another client was watching (apache#3380).
Multi-client attach is a designed Runtime Host capability: the Turn is
Host-owned and the TUI is only its viewport, so switching Sessions
mid-turn is view navigation, not a session mutation.
- new 'switch' mid-turn slash disposition alongside 'local': routed
through like 'local', but its handler must use the busy-aware
goToSession/openSessionPicker wrappers (runControl's serial lock is
held by the running Turn mid-turn)
- switchAwayMidTurn adopts the next Session without ever calling
driver.stop(); a turnEpoch fence orphans the in-flight drain after the
switch is confirmed so late events, synthesized stream failures, and
old-session queue flushes can never reach the adopted transcript; the
orphaned runAgentTurn tail releases busy/activity and hands the freshly
attached Turn its start exactly once
- requestTurnInterrupt is swallowed while a detach handoff is in flight:
the driver already points at the next Session, so a stop there would
abort whatever that Session has attached
- Escape closes the mid-turn session picker instead of arming the
double-Escape interrupt for the Turn being left running
- foreign-session import rows are hidden from the picker mid-turn (the
import flow starts a new Session; it cannot detach)
Generated-by: Maka
…switch recovery
- '/session <id>' mid-turn: switches without driver.stop(), replaces the
transcript with the adopted Session's history, fences late events from
the abandoned drain (no content leak, no synthesized 'ended without a
completion event' failure), starts the freshly attached Turn only
after the orphaned drain unwinds, and lands follow-up prompts on the
adopted Session
- '/session' mid-turn opens the picker; Escape closes it and must not
arm the double-Escape interrupt (stopCalls stays 0)
- a rejected switch leaves the running Turn fully live: error notice,
no stop, subsequent events still render into the same transcript
Generated-by: Maka
@me2seeks
me2seeksforce-pushed the fix/3380-mid-turn-session-switch branch from 21e2611 to 24a3a67CompareAugust 22, 2026 13:43

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this is a well-argued fix and the framing is right: in Runtime Host mode the Turn belongs to the Host and this TUI is only a viewport, so refusing /session mid-turn was trapping the user in a view rather than protecting anything. The monotonic turnEpoch fence is the correct shape for it — one counter makes every callback of the abandoned drain a no-op, instead of trying to unsubscribe them individually. Fencing only afterswitchSession resolves, so a failed switch leaves the drain fully live, is a detail that is easy to get backwards and you got it right.

Reviewed at exact head 24a3a6711870a427bd82f2b8f03b86b4f52b37c4 against base 8b60ddffa89682c03238ddce6595f531eb2b6f29. One P2 inline, plus two P3 verification gaps below. No checks have run on this head yet.

P3 — switching to the session you are already on.goToSession has no same-session short-circuit, so /session <current-id> mid-turn bumps the epoch, orphans the live drain, and prints Detached from the running Turn while the user is still on the same session. Whether this is harmless depends on what driver.switchSession does with its own session id, which we could not settle from the client side. A test pinning the intended behaviour would close it either way.

P3 — re-attaching to the same still-running Turn. The new tests cover attaching to a different session's live turn. Detaching and immediately re-attaching to the same one is the case where a double consumer would show up if the orphan tail and the new attach ever overlapped; the startPendingAttachedTurn no-op guards look like they prevent it, but nothing pins it.

Neither P3 blocks. The P2 does, and it is small to fix.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

// tail unwinds through the superseded branch and releases busy/activity,
// then either that tail or the startPendingAttachedTurn below starts the
// freshly attached Turn, whichever observes an idle runner first.
const switchAwayMidTurn = async (sessionId: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] switchAwayMidTurn is re-entrant, and a second detach clears detaching while the first is still in flight.

Mid-turn, both /session entry points deliberately skip runControl's serial lock — that is the point of the change, since the lock is held by the running Turn. Both are also invoked fire-and-forget: void goToSession(sessionId) and void openSessionPicker(). So nothing serializes switchAwayMidTurn against itself, and it has an await boundary in the middle of it (await input.driver.switchSession(sessionId)).

Sequence: the user types /session s2. While that request is in flight they type /session s3 — normal impatience, and the window is as long as the round trip. Now two switchAwayMidTurn calls are live.

The consequence we care about is the finally block. detaching is a boolean, so when the first call settles it runs detaching = false while the second is still awaiting its switch. That reopens requestTurnInterrupt, whose new guard exists precisely for this window — your own comment there says "a stop here would abort whatever that Session has attached." A Ctrl+C landing in that reopened window calls driver.stop() against the freshly adopted session, which is the exact outcome this PR promises cannot happen. The invariant is stated correctly in the code and then broken by re-entry.

Two smaller effects ride along: applySwitchResult runs twice in whatever order the responses return, so the user can end up on s2 after asking for s3; and two Detached from the running Turn notices land in the adopted transcript.

The mechanism to fix it already exists — detaching is shaped like a re-entrancy flag, it just is not read at the entry points. Rejecting the second detach is enough:

constgoToSession=async(sessionId: string): Promise<void>=>{if(!turnRunning){awaitrunControl(()=>switchSession(sessionId));return;}if(detaching)return;// a detach is already handing this view overawaitswitchAwayMidTurn(sessionId).catch(reportError);};

with the same guard on openSessionPicker. Turning detaching into a counter would also work but is more machinery than the situation needs — a second detach during a handoff has no meaningful semantics, so dropping it is the honest behaviour.

A regression test would issue a second /session before the first switchSession promise resolves and assert that driver.stop() is still never called and that exactly one Detached notice appears. The existing failNextSwitch test already shows the harness can control that promise's timing.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed both mechanisms locally before fixing (reproduced the double switchSession and the early detaching clear with a parked first switch), fixed at 332404759:

  • goToSession now rejects while detaching is held. The picker path needed no separate guard — its selection routes through goToSession (:2179), so one guard covers both entry points.
  • Pinned by a test: second mid-turn /session during a parked switch yields exactly one detach notice, no extra stop(), single adoption.

One nuance worth flagging: after the blocked re-entry, the queued command text can replay through the idle path once the adopted turn settles, producing a benign second switchSession to the same id — that is pre-existing queue behavior outside this window, not a reopen of it.

Comment on lines +1152 to +1155
// A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
// drain: from that point every callback below must stop touching shared
// runner state — the adopted Session owns it now.
const superseded = () => epoch !== turnEpoch;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This comment says every callback below stops touching shared runner state once the epoch is bumped, but two of them do not check.

superseded() is consulted at :1226 (onEvent), :1263 and :1278. It is not consulted in:

  • onPrepared (:1197-1210) — the non-attached branch ends in if (turn.summary) adoptSessionMetadata(turn.summary), which writes title/cwd/model on the runner
  • onSkillInvocation (:1211-1222) — splices state.entries and calls showSkillInvocation

Both are reachable after a switch-away, because turnRunning is set at the top of runAgentTurn while pi-tui-turn.ts:87-88 only invokes these two afterawait preparePrompt resolves. So the whole preparePrompt window is a period where /session can already enter switchAwayMidTurn, and neither callback has returned yet.

Concrete sequence:

  1. a turn stalls in preparePrompt (slow or hung)
  2. /session <other>switchSession succeeds, turnEpoch bumps, applySwitchResult replaces the transcript with the adopted session
  3. preparePrompt finally resolves
  4. onPrepared runs with the abandoned turn's summary and calls adoptSessionMetadata on it

The screen now describes the old session while the driver is on the new one, and the next prompt goes to the new one. The skill callback has the same shape: an overlay belonging to the abandoned session appearing over the adopted viewport.

shouldAbort doesn't help — it only reads closed, not the epoch.

Fix looks like the same one-liner already used in the three fenced callbacks: if (superseded()) return; at the top of both, including the onSkillInvocation on the SkillInvocationBlockedError path. A test that switches while preparePrompt is still unresolved, then asserts the metadata still belongs to the adopted session, would pin it.

This is separate from the re-entrant detaching P2 already reported inline on this head — that one is about entering the path twice, this one is about callbacks that were already in flight when it was entered once.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed before fixing: reproduced an abandoned Turn adopting its summary over the adopted session when preparePrompt resolved after a completed switch-away. Fixed at 332404759onPrepared and onSkillInvocation now open with the same superseded() fence as the other callbacks, which also covers the SkillInvocationBlockedError invocation since both call sites share the callback. New test parks preparePrompt, detaches mid-park, then resolves: the abandoned title/cwd never reach the runner metadata and the skill card cannot land on the adopted viewport. CLI suite 362/362, biome clean.

@Astro-Han

Copy link
Copy Markdown
Contributor

Second review pass on 24a3a6711870a427bd82f2b8f03b86b4f52b37c4, from a different model and a different route than the earlier one on this head. One new [P2], left inline at the epoch fence: #discussion_r3837660462.

The mechanism itself holds up, and we checked the thing that usually goes wrong with an escape path — what happens to the thing you escaped from:

  • The Host Turn is not stopped.switchAwayMidTurn never calls driver.stop() (verified: zero stop calls in the covering tests), which is the correct asymmetry against the interrupt path. In Runtime Host mode the Turn is Host-owned and this TUI was only its viewport, so detaching the view should not end the work.
  • A failed switch does not orphan the live drain.turnEpoch += 1 happens only after await switchSession resolves, so a throw leaves the in-flight drain fully live — pinned by a test asserting late deltas still land in the same transcript.
  • The adopted Turn cannot start twice.startPendingAttachedTurn no-ops while busy || turnRunning, and the old drain's finishTurnUi clears turnRunning before releasing; whichever of the two observes an idle runner first starts it.
  • Escape on the picker doesn't interrupt. Early return while sessionPickerOverlayOpen, with a test asserting no stop call.

On structure: one epoch counter, not a second authority over Turn lifetime. That's the right shape for this.

Two things noted without grading, since neither is this PR's job to fix:

  • Removing /transcript and the TranscriptViewerOverlay reference is a behaviour deletion unrelated to detach. The viewer module file itself isn't in this diff, so it will be left behind — worth a follow-up so it doesn't linger as dead code.
  • /session <current id> still detaches from itself; goToSession has no short-circuit. Already reported previously, not re-graded here.

The re-entrant detaching [P2] already on this head stands on its own; we looked at it independently and agree, and are deliberately not re-grading it. The finding above is a different failure: not entering the path twice, but callbacks that were already in flight when it was entered once.

On CI: the run on this head was sitting at action_required and had never executed — I've approved it, so there should be a real signal shortly. Not approving while the P2 above and the existing one are open.

Two review findings on the apache#3380 detach path:
- switchAwayMidTurn was re-entrant: a second mid-turn /session while the
first was still handing the view over cleared the detaching flag early,
reopening the interrupt window and double-applying adoption. The entry
now rejects while a detach is in flight (the picker selection routes
through the same guard).
- onPrepared and onSkillInvocation ran without the superseded() fence the
other callbacks use. Both are reachable after a switch-away because they
fire only after preparePrompt resolves, so an abandoned Turn could still
adopt its metadata onto the adopted Session's view and surface its skill
card over the adopted viewport.
Both fixes are pinned by tests: a parked second /session yields exactly
one detach notice, and a Turn prepared across a detach can no longer
overwrite the adopted session's title/cwd.
Generated-by: maka

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 3324047598651d6b725bdb579cb66c3b0eeb99cc. Required test is completed / success bound to that exact SHA. No P0–P2.

Re-review at the current head. Both earlier findings were re-derived from the code rather than taken as fixed.

Both [P2]s are closed.

The re-entrant detach is guarded at the entry point, and — importantly — the guard is not cleared early: if (detaching) return; at pi-tui-runner.ts:1625 sits behind a comment explaining that a second detach arriving while the first is still handing the view over would otherwise reset the flag and reopen the path. That is the actual failure mode, closed at the right place.

The epoch fence is now complete. superseded() is consulted at five call sites (:1201, :1219, :1234, :1271, :1286) rather than three, so the two callbacks that previously kept touching shared runner state after the epoch bump no longer do. The comment claiming every callback below the fence checks it is now true — before, the comment described an intent the code did not implement, which is the more dangerous of the two states.

The production increment is +12 lines. It fixes exactly these two holes and introduces nothing else.

Residual, non-blocking: the same-session detach [P3] stands; detaching and immediately reattaching to the same running Turn produces a two-consumer situation that no test currently pins; and the /transcript dead-module follow-up is unchanged.

Stated rather than implied: the local CLI suite and a real multi-client Host were not exercised here — the evidence is the hosted test on this exact head plus code-level verification of both fixes.

Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review — merge still needs a committer other than the author to give LGTM and to decide.

@Astro-Han
Astro-Han merged commit bfcd3da into apache:mainAug 23, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): a second TUI attached to a Session with a running Turn is trapped — every exit path aborts the Turn

2 participants

@me2seeks@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix(cli): let /session detach from a running Turn instead of trapping the client by me2seeks · Pull Request #3498 · apache/maka · GitHub
Skip to content

fix(cli): let /session detach from a running Turn instead of trapping the client - #3498

Merged
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch
Aug 23, 2026
Merged

fix(cli): let /session detach from a running Turn instead of trapping the client#3498
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch

Conversation

@me2seeks

@me2seeksme2seeks commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes#3380

Problem

A second TUI that switched onto a Session with an in-flight Turn (via /session or the picker) was trapped: /session was intercepted with "Cannot run /session while a turn is running", and every apparent way out was destructive — following the hint (Esc/Ctrl+C) or quitting called driver.stop(), aborting the shared Turn another client was watching. The only clean escape was kill -9.

Multi-client attach is a designed Runtime Host capability: the Turn is Host-owned and each TUI is only its viewport. Desktop already treats session switching as pure view navigation; this fix brings the CLI to the same model.

Fix: let /session detach from a running Turn instead of touching it

  • New 'switch' mid-turn slash disposition (alongside 'local' from refactor(cli): move the mid-turn slash disposition onto the command spec #3379). Routed through like 'local', but its handler must use busy-aware wrappers: idle it runs under runControl's serial lock as before; mid-turn the switch goes through the new detach path instead of silently no-oping on runControl's busy gate.
  • switchAwayMidTurn never calls driver.stop(). After driver.switchSession confirms, a monotonic turnEpoch fence orphans the in-flight drain:
    • late events and synthesized stream failures ("ended without a completion event") from the abandoned Session can no longer reach the adopted transcript;
    • the orphaned runAgentTurn tail skips all old-session continuations (queue flushes would steer the NEW Session), releases busy/activity, and starts the freshly attached Turn exactly once — whichever of the detach path or the orphan tail observes an idle runner first.
  • Interrupt guard during handoff: requestTurnInterrupt is swallowed while a detach is in flight — the driver already points at the next Session, so a stop there would abort whatever that Session has attached.
  • Escape owns the picker: while the mid-turn session picker is open, Escape closes it instead of arming the double-Escape interrupt for the Turn being left running.
  • Foreign import rows hidden mid-turn: the Claude Code/Codex import flow starts a new Session and cannot detach.

Scope notes

Testing

  • 3 new tests in pi-tui-runner.test.ts with a parking-turn driver:
    • mid-turn /session <id>: switches with zero stop() calls, replaces the transcript, fences leaked events, starts the attached Turn after the orphan unwinds, lands follow-ups on the adopted Session;
    • /session opens the picker mid-turn and Escape closes it without arming an interrupt;
    • a failed switch reports the error and leaves the running Turn streaming normally.
  • Full CLI suite: 360/360 pass; biome + typecheck clean.

AI use

  • Generative tooling made a substantive contribution
  • No generative tool made a substantive contribution

Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push. Generated-by: Maka trailers are present on the branch commits.

@me2seeks

Copy link
Copy Markdown
ContributorAuthor

Working on this PR — happy to iterate on review feedback. Local verification: CLI suite 360/360, biome + typecheck clean.

… the client
A second TUI that switched onto a Session with an in-flight Turn was
trapped: every exit path was destructive. /session was intercepted with
'Cannot run /session while a turn is running', and following the hint
(Esc/Ctrl+C) or quitting aborted the shared Turn via driver.stop() —
killing work another client was watching (apache#3380).
Multi-client attach is a designed Runtime Host capability: the Turn is
Host-owned and the TUI is only its viewport, so switching Sessions
mid-turn is view navigation, not a session mutation.
- new 'switch' mid-turn slash disposition alongside 'local': routed
through like 'local', but its handler must use the busy-aware
goToSession/openSessionPicker wrappers (runControl's serial lock is
held by the running Turn mid-turn)
- switchAwayMidTurn adopts the next Session without ever calling
driver.stop(); a turnEpoch fence orphans the in-flight drain after the
switch is confirmed so late events, synthesized stream failures, and
old-session queue flushes can never reach the adopted transcript; the
orphaned runAgentTurn tail releases busy/activity and hands the freshly
attached Turn its start exactly once
- requestTurnInterrupt is swallowed while a detach handoff is in flight:
the driver already points at the next Session, so a stop there would
abort whatever that Session has attached
- Escape closes the mid-turn session picker instead of arming the
double-Escape interrupt for the Turn being left running
- foreign-session import rows are hidden from the picker mid-turn (the
import flow starts a new Session; it cannot detach)
Generated-by: Maka
…switch recovery
- '/session <id>' mid-turn: switches without driver.stop(), replaces the
transcript with the adopted Session's history, fences late events from
the abandoned drain (no content leak, no synthesized 'ended without a
completion event' failure), starts the freshly attached Turn only
after the orphaned drain unwinds, and lands follow-up prompts on the
adopted Session
- '/session' mid-turn opens the picker; Escape closes it and must not
arm the double-Escape interrupt (stopCalls stays 0)
- a rejected switch leaves the running Turn fully live: error notice,
no stop, subsequent events still render into the same transcript
Generated-by: Maka
@me2seeks
me2seeksforce-pushed the fix/3380-mid-turn-session-switch branch from 21e2611 to 24a3a67CompareAugust 22, 2026 13:43

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this is a well-argued fix and the framing is right: in Runtime Host mode the Turn belongs to the Host and this TUI is only a viewport, so refusing /session mid-turn was trapping the user in a view rather than protecting anything. The monotonic turnEpoch fence is the correct shape for it — one counter makes every callback of the abandoned drain a no-op, instead of trying to unsubscribe them individually. Fencing only afterswitchSession resolves, so a failed switch leaves the drain fully live, is a detail that is easy to get backwards and you got it right.

Reviewed at exact head 24a3a6711870a427bd82f2b8f03b86b4f52b37c4 against base 8b60ddffa89682c03238ddce6595f531eb2b6f29. One P2 inline, plus two P3 verification gaps below. No checks have run on this head yet.

P3 — switching to the session you are already on.goToSession has no same-session short-circuit, so /session <current-id> mid-turn bumps the epoch, orphans the live drain, and prints Detached from the running Turn while the user is still on the same session. Whether this is harmless depends on what driver.switchSession does with its own session id, which we could not settle from the client side. A test pinning the intended behaviour would close it either way.

P3 — re-attaching to the same still-running Turn. The new tests cover attaching to a different session's live turn. Detaching and immediately re-attaching to the same one is the case where a double consumer would show up if the orphan tail and the new attach ever overlapped; the startPendingAttachedTurn no-op guards look like they prevent it, but nothing pins it.

Neither P3 blocks. The P2 does, and it is small to fix.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

// tail unwinds through the superseded branch and releases busy/activity,
// then either that tail or the startPendingAttachedTurn below starts the
// freshly attached Turn, whichever observes an idle runner first.
const switchAwayMidTurn = async (sessionId: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] switchAwayMidTurn is re-entrant, and a second detach clears detaching while the first is still in flight.

Mid-turn, both /session entry points deliberately skip runControl's serial lock — that is the point of the change, since the lock is held by the running Turn. Both are also invoked fire-and-forget: void goToSession(sessionId) and void openSessionPicker(). So nothing serializes switchAwayMidTurn against itself, and it has an await boundary in the middle of it (await input.driver.switchSession(sessionId)).

Sequence: the user types /session s2. While that request is in flight they type /session s3 — normal impatience, and the window is as long as the round trip. Now two switchAwayMidTurn calls are live.

The consequence we care about is the finally block. detaching is a boolean, so when the first call settles it runs detaching = false while the second is still awaiting its switch. That reopens requestTurnInterrupt, whose new guard exists precisely for this window — your own comment there says "a stop here would abort whatever that Session has attached." A Ctrl+C landing in that reopened window calls driver.stop() against the freshly adopted session, which is the exact outcome this PR promises cannot happen. The invariant is stated correctly in the code and then broken by re-entry.

Two smaller effects ride along: applySwitchResult runs twice in whatever order the responses return, so the user can end up on s2 after asking for s3; and two Detached from the running Turn notices land in the adopted transcript.

The mechanism to fix it already exists — detaching is shaped like a re-entrancy flag, it just is not read at the entry points. Rejecting the second detach is enough:

constgoToSession=async(sessionId: string): Promise<void>=>{if(!turnRunning){awaitrunControl(()=>switchSession(sessionId));return;}if(detaching)return;// a detach is already handing this view overawaitswitchAwayMidTurn(sessionId).catch(reportError);};

with the same guard on openSessionPicker. Turning detaching into a counter would also work but is more machinery than the situation needs — a second detach during a handoff has no meaningful semantics, so dropping it is the honest behaviour.

A regression test would issue a second /session before the first switchSession promise resolves and assert that driver.stop() is still never called and that exactly one Detached notice appears. The existing failNextSwitch test already shows the harness can control that promise's timing.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed both mechanisms locally before fixing (reproduced the double switchSession and the early detaching clear with a parked first switch), fixed at 332404759:

  • goToSession now rejects while detaching is held. The picker path needed no separate guard — its selection routes through goToSession (:2179), so one guard covers both entry points.
  • Pinned by a test: second mid-turn /session during a parked switch yields exactly one detach notice, no extra stop(), single adoption.

One nuance worth flagging: after the blocked re-entry, the queued command text can replay through the idle path once the adopted turn settles, producing a benign second switchSession to the same id — that is pre-existing queue behavior outside this window, not a reopen of it.

Comment on lines +1152 to +1155
// A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
// drain: from that point every callback below must stop touching shared
// runner state — the adopted Session owns it now.
const superseded = () => epoch !== turnEpoch;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This comment says every callback below stops touching shared runner state once the epoch is bumped, but two of them do not check.

superseded() is consulted at :1226 (onEvent), :1263 and :1278. It is not consulted in:

  • onPrepared (:1197-1210) — the non-attached branch ends in if (turn.summary) adoptSessionMetadata(turn.summary), which writes title/cwd/model on the runner
  • onSkillInvocation (:1211-1222) — splices state.entries and calls showSkillInvocation

Both are reachable after a switch-away, because turnRunning is set at the top of runAgentTurn while pi-tui-turn.ts:87-88 only invokes these two afterawait preparePrompt resolves. So the whole preparePrompt window is a period where /session can already enter switchAwayMidTurn, and neither callback has returned yet.

Concrete sequence:

  1. a turn stalls in preparePrompt (slow or hung)
  2. /session <other>switchSession succeeds, turnEpoch bumps, applySwitchResult replaces the transcript with the adopted session
  3. preparePrompt finally resolves
  4. onPrepared runs with the abandoned turn's summary and calls adoptSessionMetadata on it

The screen now describes the old session while the driver is on the new one, and the next prompt goes to the new one. The skill callback has the same shape: an overlay belonging to the abandoned session appearing over the adopted viewport.

shouldAbort doesn't help — it only reads closed, not the epoch.

Fix looks like the same one-liner already used in the three fenced callbacks: if (superseded()) return; at the top of both, including the onSkillInvocation on the SkillInvocationBlockedError path. A test that switches while preparePrompt is still unresolved, then asserts the metadata still belongs to the adopted session, would pin it.

This is separate from the re-entrant detaching P2 already reported inline on this head — that one is about entering the path twice, this one is about callbacks that were already in flight when it was entered once.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed before fixing: reproduced an abandoned Turn adopting its summary over the adopted session when preparePrompt resolved after a completed switch-away. Fixed at 332404759onPrepared and onSkillInvocation now open with the same superseded() fence as the other callbacks, which also covers the SkillInvocationBlockedError invocation since both call sites share the callback. New test parks preparePrompt, detaches mid-park, then resolves: the abandoned title/cwd never reach the runner metadata and the skill card cannot land on the adopted viewport. CLI suite 362/362, biome clean.

@Astro-Han

Copy link
Copy Markdown
Contributor

Second review pass on 24a3a6711870a427bd82f2b8f03b86b4f52b37c4, from a different model and a different route than the earlier one on this head. One new [P2], left inline at the epoch fence: #discussion_r3837660462.

The mechanism itself holds up, and we checked the thing that usually goes wrong with an escape path — what happens to the thing you escaped from:

  • The Host Turn is not stopped.switchAwayMidTurn never calls driver.stop() (verified: zero stop calls in the covering tests), which is the correct asymmetry against the interrupt path. In Runtime Host mode the Turn is Host-owned and this TUI was only its viewport, so detaching the view should not end the work.
  • A failed switch does not orphan the live drain.turnEpoch += 1 happens only after await switchSession resolves, so a throw leaves the in-flight drain fully live — pinned by a test asserting late deltas still land in the same transcript.
  • The adopted Turn cannot start twice.startPendingAttachedTurn no-ops while busy || turnRunning, and the old drain's finishTurnUi clears turnRunning before releasing; whichever of the two observes an idle runner first starts it.
  • Escape on the picker doesn't interrupt. Early return while sessionPickerOverlayOpen, with a test asserting no stop call.

On structure: one epoch counter, not a second authority over Turn lifetime. That's the right shape for this.

Two things noted without grading, since neither is this PR's job to fix:

  • Removing /transcript and the TranscriptViewerOverlay reference is a behaviour deletion unrelated to detach. The viewer module file itself isn't in this diff, so it will be left behind — worth a follow-up so it doesn't linger as dead code.
  • /session <current id> still detaches from itself; goToSession has no short-circuit. Already reported previously, not re-graded here.

The re-entrant detaching [P2] already on this head stands on its own; we looked at it independently and agree, and are deliberately not re-grading it. The finding above is a different failure: not entering the path twice, but callbacks that were already in flight when it was entered once.

On CI: the run on this head was sitting at action_required and had never executed — I've approved it, so there should be a real signal shortly. Not approving while the P2 above and the existing one are open.

Two review findings on the apache#3380 detach path:
- switchAwayMidTurn was re-entrant: a second mid-turn /session while the
first was still handing the view over cleared the detaching flag early,
reopening the interrupt window and double-applying adoption. The entry
now rejects while a detach is in flight (the picker selection routes
through the same guard).
- onPrepared and onSkillInvocation ran without the superseded() fence the
other callbacks use. Both are reachable after a switch-away because they
fire only after preparePrompt resolves, so an abandoned Turn could still
adopt its metadata onto the adopted Session's view and surface its skill
card over the adopted viewport.
Both fixes are pinned by tests: a parked second /session yields exactly
one detach notice, and a Turn prepared across a detach can no longer
overwrite the adopted session's title/cwd.
Generated-by: maka

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 3324047598651d6b725bdb579cb66c3b0eeb99cc. Required test is completed / success bound to that exact SHA. No P0–P2.

Re-review at the current head. Both earlier findings were re-derived from the code rather than taken as fixed.

Both [P2]s are closed.

The re-entrant detach is guarded at the entry point, and — importantly — the guard is not cleared early: if (detaching) return; at pi-tui-runner.ts:1625 sits behind a comment explaining that a second detach arriving while the first is still handing the view over would otherwise reset the flag and reopen the path. That is the actual failure mode, closed at the right place.

The epoch fence is now complete. superseded() is consulted at five call sites (:1201, :1219, :1234, :1271, :1286) rather than three, so the two callbacks that previously kept touching shared runner state after the epoch bump no longer do. The comment claiming every callback below the fence checks it is now true — before, the comment described an intent the code did not implement, which is the more dangerous of the two states.

The production increment is +12 lines. It fixes exactly these two holes and introduces nothing else.

Residual, non-blocking: the same-session detach [P3] stands; detaching and immediately reattaching to the same running Turn produces a two-consumer situation that no test currently pins; and the /transcript dead-module follow-up is unchanged.

Stated rather than implied: the local CLI suite and a real multi-client Host were not exercised here — the evidence is the hosted test on this exact head plus code-level verification of both fixes.

Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review — merge still needs a committer other than the author to give LGTM and to decide.

@Astro-Han
Astro-Han merged commit bfcd3da into apache:mainAug 23, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): a second TUI attached to a Session with a running Turn is trapped — every exit path aborts the Turn

2 participants

@me2seeks@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix(cli): let /session detach from a running Turn instead of trapping the client by me2seeks · Pull Request #3498 · apache/maka · GitHub
Skip to content

fix(cli): let /session detach from a running Turn instead of trapping the client - #3498

Merged
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch
Aug 23, 2026
Merged

fix(cli): let /session detach from a running Turn instead of trapping the client#3498
Astro-Han merged 3 commits into
apache:mainfrom
me2seeks:fix/3380-mid-turn-session-switch

Conversation

@me2seeks

@me2seeksme2seeks commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Closes#3380

Problem

A second TUI that switched onto a Session with an in-flight Turn (via /session or the picker) was trapped: /session was intercepted with "Cannot run /session while a turn is running", and every apparent way out was destructive — following the hint (Esc/Ctrl+C) or quitting called driver.stop(), aborting the shared Turn another client was watching. The only clean escape was kill -9.

Multi-client attach is a designed Runtime Host capability: the Turn is Host-owned and each TUI is only its viewport. Desktop already treats session switching as pure view navigation; this fix brings the CLI to the same model.

Fix: let /session detach from a running Turn instead of touching it

  • New 'switch' mid-turn slash disposition (alongside 'local' from refactor(cli): move the mid-turn slash disposition onto the command spec #3379). Routed through like 'local', but its handler must use busy-aware wrappers: idle it runs under runControl's serial lock as before; mid-turn the switch goes through the new detach path instead of silently no-oping on runControl's busy gate.
  • switchAwayMidTurn never calls driver.stop(). After driver.switchSession confirms, a monotonic turnEpoch fence orphans the in-flight drain:
    • late events and synthesized stream failures ("ended without a completion event") from the abandoned Session can no longer reach the adopted transcript;
    • the orphaned runAgentTurn tail skips all old-session continuations (queue flushes would steer the NEW Session), releases busy/activity, and starts the freshly attached Turn exactly once — whichever of the detach path or the orphan tail observes an idle runner first.
  • Interrupt guard during handoff: requestTurnInterrupt is swallowed while a detach is in flight — the driver already points at the next Session, so a stop there would abort whatever that Session has attached.
  • Escape owns the picker: while the mid-turn session picker is open, Escape closes it instead of arming the double-Escape interrupt for the Turn being left running.
  • Foreign import rows hidden mid-turn: the Claude Code/Codex import flow starts a new Session and cannot detach.

Scope notes

Testing

  • 3 new tests in pi-tui-runner.test.ts with a parking-turn driver:
    • mid-turn /session <id>: switches with zero stop() calls, replaces the transcript, fences leaked events, starts the attached Turn after the orphan unwinds, lands follow-ups on the adopted Session;
    • /session opens the picker mid-turn and Escape closes it without arming an interrupt;
    • a failed switch reports the error and leaves the running Turn streaming normally.
  • Full CLI suite: 360/360 pass; biome + typecheck clean.

AI use

  • Generative tooling made a substantive contribution
  • No generative tool made a substantive contribution

Tool(s) and scope: Maka (AI coding agent) authored the implementation and tests; the diff was human-reviewed before push. Generated-by: Maka trailers are present on the branch commits.

@me2seeks

Copy link
Copy Markdown
ContributorAuthor

Working on this PR — happy to iterate on review feedback. Local verification: CLI suite 360/360, biome + typecheck clean.

… the client
A second TUI that switched onto a Session with an in-flight Turn was
trapped: every exit path was destructive. /session was intercepted with
'Cannot run /session while a turn is running', and following the hint
(Esc/Ctrl+C) or quitting aborted the shared Turn via driver.stop() —
killing work another client was watching (apache#3380).
Multi-client attach is a designed Runtime Host capability: the Turn is
Host-owned and the TUI is only its viewport, so switching Sessions
mid-turn is view navigation, not a session mutation.
- new 'switch' mid-turn slash disposition alongside 'local': routed
through like 'local', but its handler must use the busy-aware
goToSession/openSessionPicker wrappers (runControl's serial lock is
held by the running Turn mid-turn)
- switchAwayMidTurn adopts the next Session without ever calling
driver.stop(); a turnEpoch fence orphans the in-flight drain after the
switch is confirmed so late events, synthesized stream failures, and
old-session queue flushes can never reach the adopted transcript; the
orphaned runAgentTurn tail releases busy/activity and hands the freshly
attached Turn its start exactly once
- requestTurnInterrupt is swallowed while a detach handoff is in flight:
the driver already points at the next Session, so a stop there would
abort whatever that Session has attached
- Escape closes the mid-turn session picker instead of arming the
double-Escape interrupt for the Turn being left running
- foreign-session import rows are hidden from the picker mid-turn (the
import flow starts a new Session; it cannot detach)
Generated-by: Maka
…switch recovery
- '/session <id>' mid-turn: switches without driver.stop(), replaces the
transcript with the adopted Session's history, fences late events from
the abandoned drain (no content leak, no synthesized 'ended without a
completion event' failure), starts the freshly attached Turn only
after the orphaned drain unwinds, and lands follow-up prompts on the
adopted Session
- '/session' mid-turn opens the picker; Escape closes it and must not
arm the double-Escape interrupt (stopCalls stays 0)
- a rejected switch leaves the running Turn fully live: error notice,
no stop, subsequent events still render into the same transcript
Generated-by: Maka
@me2seeks
me2seeksforce-pushed the fix/3380-mid-turn-session-switch branch from 21e2611 to 24a3a67CompareAugust 22, 2026 13:43

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this is a well-argued fix and the framing is right: in Runtime Host mode the Turn belongs to the Host and this TUI is only a viewport, so refusing /session mid-turn was trapping the user in a view rather than protecting anything. The monotonic turnEpoch fence is the correct shape for it — one counter makes every callback of the abandoned drain a no-op, instead of trying to unsubscribe them individually. Fencing only afterswitchSession resolves, so a failed switch leaves the drain fully live, is a detail that is easy to get backwards and you got it right.

Reviewed at exact head 24a3a6711870a427bd82f2b8f03b86b4f52b37c4 against base 8b60ddffa89682c03238ddce6595f531eb2b6f29. One P2 inline, plus two P3 verification gaps below. No checks have run on this head yet.

P3 — switching to the session you are already on.goToSession has no same-session short-circuit, so /session <current-id> mid-turn bumps the epoch, orphans the live drain, and prints Detached from the running Turn while the user is still on the same session. Whether this is harmless depends on what driver.switchSession does with its own session id, which we could not settle from the client side. A test pinning the intended behaviour would close it either way.

P3 — re-attaching to the same still-running Turn. The new tests cover attaching to a different session's live turn. Detaching and immediately re-attaching to the same one is the case where a double consumer would show up if the orphan tail and the new attach ever overlapped; the startPendingAttachedTurn no-op guards look like they prevent it, but nothing pins it.

Neither P3 blocks. The P2 does, and it is small to fix.


This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are ours to correct — please push back where we got it wrong.

// tail unwinds through the superseded branch and releases busy/activity,
// then either that tail or the startPendingAttachedTurn below starts the
// freshly attached Turn, whichever observes an idle runner first.
const switchAwayMidTurn = async (sessionId: string) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] switchAwayMidTurn is re-entrant, and a second detach clears detaching while the first is still in flight.

Mid-turn, both /session entry points deliberately skip runControl's serial lock — that is the point of the change, since the lock is held by the running Turn. Both are also invoked fire-and-forget: void goToSession(sessionId) and void openSessionPicker(). So nothing serializes switchAwayMidTurn against itself, and it has an await boundary in the middle of it (await input.driver.switchSession(sessionId)).

Sequence: the user types /session s2. While that request is in flight they type /session s3 — normal impatience, and the window is as long as the round trip. Now two switchAwayMidTurn calls are live.

The consequence we care about is the finally block. detaching is a boolean, so when the first call settles it runs detaching = false while the second is still awaiting its switch. That reopens requestTurnInterrupt, whose new guard exists precisely for this window — your own comment there says "a stop here would abort whatever that Session has attached." A Ctrl+C landing in that reopened window calls driver.stop() against the freshly adopted session, which is the exact outcome this PR promises cannot happen. The invariant is stated correctly in the code and then broken by re-entry.

Two smaller effects ride along: applySwitchResult runs twice in whatever order the responses return, so the user can end up on s2 after asking for s3; and two Detached from the running Turn notices land in the adopted transcript.

The mechanism to fix it already exists — detaching is shaped like a re-entrancy flag, it just is not read at the entry points. Rejecting the second detach is enough:

constgoToSession=async(sessionId: string): Promise<void>=>{if(!turnRunning){awaitrunControl(()=>switchSession(sessionId));return;}if(detaching)return;// a detach is already handing this view overawaitswitchAwayMidTurn(sessionId).catch(reportError);};

with the same guard on openSessionPicker. Turning detaching into a counter would also work but is more machinery than the situation needs — a second detach during a handoff has no meaningful semantics, so dropping it is the honest behaviour.

A regression test would issue a second /session before the first switchSession promise resolves and assert that driver.stop() is still never called and that exactly one Detached notice appears. The existing failNextSwitch test already shows the harness can control that promise's timing.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed both mechanisms locally before fixing (reproduced the double switchSession and the early detaching clear with a parked first switch), fixed at 332404759:

  • goToSession now rejects while detaching is held. The picker path needed no separate guard — its selection routes through goToSession (:2179), so one guard covers both entry points.
  • Pinned by a test: second mid-turn /session during a parked switch yields exactly one detach notice, no extra stop(), single adoption.

One nuance worth flagging: after the blocked re-entry, the queued command text can replay through the idle path once the adopted turn settles, producing a benign second switchSession to the same id — that is pre-existing queue behavior outside this window, not a reopen of it.

Comment on lines +1152 to +1155
// A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
// drain: from that point every callback below must stop touching shared
// runner state — the adopted Session owns it now.
const superseded = () => epoch !== turnEpoch;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] This comment says every callback below stops touching shared runner state once the epoch is bumped, but two of them do not check.

superseded() is consulted at :1226 (onEvent), :1263 and :1278. It is not consulted in:

  • onPrepared (:1197-1210) — the non-attached branch ends in if (turn.summary) adoptSessionMetadata(turn.summary), which writes title/cwd/model on the runner
  • onSkillInvocation (:1211-1222) — splices state.entries and calls showSkillInvocation

Both are reachable after a switch-away, because turnRunning is set at the top of runAgentTurn while pi-tui-turn.ts:87-88 only invokes these two afterawait preparePrompt resolves. So the whole preparePrompt window is a period where /session can already enter switchAwayMidTurn, and neither callback has returned yet.

Concrete sequence:

  1. a turn stalls in preparePrompt (slow or hung)
  2. /session <other>switchSession succeeds, turnEpoch bumps, applySwitchResult replaces the transcript with the adopted session
  3. preparePrompt finally resolves
  4. onPrepared runs with the abandoned turn's summary and calls adoptSessionMetadata on it

The screen now describes the old session while the driver is on the new one, and the next prompt goes to the new one. The skill callback has the same shape: an overlay belonging to the abandoned session appearing over the adopted viewport.

shouldAbort doesn't help — it only reads closed, not the epoch.

Fix looks like the same one-liner already used in the three fenced callbacks: if (superseded()) return; at the top of both, including the onSkillInvocation on the SkillInvocationBlockedError path. A test that switches while preparePrompt is still unresolved, then asserts the metadata still belongs to the adopted session, would pin it.

This is separate from the re-entrant detaching P2 already reported inline on this head — that one is about entering the path twice, this one is about callbacks that were already in flight when it was entered once.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed before fixing: reproduced an abandoned Turn adopting its summary over the adopted session when preparePrompt resolved after a completed switch-away. Fixed at 332404759onPrepared and onSkillInvocation now open with the same superseded() fence as the other callbacks, which also covers the SkillInvocationBlockedError invocation since both call sites share the callback. New test parks preparePrompt, detaches mid-park, then resolves: the abandoned title/cwd never reach the runner metadata and the skill card cannot land on the adopted viewport. CLI suite 362/362, biome clean.

@Astro-Han

Copy link
Copy Markdown
Contributor

Second review pass on 24a3a6711870a427bd82f2b8f03b86b4f52b37c4, from a different model and a different route than the earlier one on this head. One new [P2], left inline at the epoch fence: #discussion_r3837660462.

The mechanism itself holds up, and we checked the thing that usually goes wrong with an escape path — what happens to the thing you escaped from:

  • The Host Turn is not stopped.switchAwayMidTurn never calls driver.stop() (verified: zero stop calls in the covering tests), which is the correct asymmetry against the interrupt path. In Runtime Host mode the Turn is Host-owned and this TUI was only its viewport, so detaching the view should not end the work.
  • A failed switch does not orphan the live drain.turnEpoch += 1 happens only after await switchSession resolves, so a throw leaves the in-flight drain fully live — pinned by a test asserting late deltas still land in the same transcript.
  • The adopted Turn cannot start twice.startPendingAttachedTurn no-ops while busy || turnRunning, and the old drain's finishTurnUi clears turnRunning before releasing; whichever of the two observes an idle runner first starts it.
  • Escape on the picker doesn't interrupt. Early return while sessionPickerOverlayOpen, with a test asserting no stop call.

On structure: one epoch counter, not a second authority over Turn lifetime. That's the right shape for this.

Two things noted without grading, since neither is this PR's job to fix:

  • Removing /transcript and the TranscriptViewerOverlay reference is a behaviour deletion unrelated to detach. The viewer module file itself isn't in this diff, so it will be left behind — worth a follow-up so it doesn't linger as dead code.
  • /session <current id> still detaches from itself; goToSession has no short-circuit. Already reported previously, not re-graded here.

The re-entrant detaching [P2] already on this head stands on its own; we looked at it independently and agree, and are deliberately not re-grading it. The finding above is a different failure: not entering the path twice, but callbacks that were already in flight when it was entered once.

On CI: the run on this head was sitting at action_required and had never executed — I've approved it, so there should be a real signal shortly. Not approving while the P2 above and the existing one are open.

Two review findings on the apache#3380 detach path:
- switchAwayMidTurn was re-entrant: a second mid-turn /session while the
first was still handing the view over cleared the detaching flag early,
reopening the interrupt window and double-applying adoption. The entry
now rejects while a detach is in flight (the picker selection routes
through the same guard).
- onPrepared and onSkillInvocation ran without the superseded() fence the
other callbacks use. Both are reachable after a switch-away because they
fire only after preparePrompt resolves, so an abandoned Turn could still
adopt its metadata onto the adopted Session's view and surface its skill
card over the adopted viewport.
Both fixes are pinned by tests: a parked second /session yields exactly
one detach notice, and a Turn prepared across a detach can no longer
overwrite the adopted session's title/cwd.
Generated-by: maka

@Astro-HanAstro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving 3324047598651d6b725bdb579cb66c3b0eeb99cc. Required test is completed / success bound to that exact SHA. No P0–P2.

Re-review at the current head. Both earlier findings were re-derived from the code rather than taken as fixed.

Both [P2]s are closed.

The re-entrant detach is guarded at the entry point, and — importantly — the guard is not cleared early: if (detaching) return; at pi-tui-runner.ts:1625 sits behind a comment explaining that a second detach arriving while the first is still handing the view over would otherwise reset the flag and reopen the path. That is the actual failure mode, closed at the right place.

The epoch fence is now complete. superseded() is consulted at five call sites (:1201, :1219, :1234, :1271, :1286) rather than three, so the two callbacks that previously kept touching shared runner state after the epoch bump no longer do. The comment claiming every callback below the fence checks it is now true — before, the comment described an intent the code did not implement, which is the more dangerous of the two states.

The production increment is +12 lines. It fixes exactly these two holes and introduces nothing else.

Residual, non-blocking: the same-session detach [P3] stands; detaching and immediately reattaching to the same running Turn produces a two-consumer situation that no test currently pins; and the /transcript dead-module follow-up is unchanged.

Stated rather than implied: the local CLI suite and a real multi-client Host were not exercised here — the evidence is the hosted test on this exact head plus code-level verification of both fixes.

Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review — merge still needs a committer other than the author to give LGTM and to decide.

@Astro-Han
Astro-Han merged commit bfcd3da into apache:mainAug 23, 2026
1 check passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(cli): a second TUI attached to a Session with a running Turn is trapped — every exit path aborts the Turn

2 participants

@me2seeks@Astro-Han