Document rotation's exemption and prove the fix through the real queue

Session rotation was never routed through closeAgentForRebuild: it mints a
fresh sessionId/workdir before rebuilding, so a leaked lock on the old
workdir can never be re-acquired there. Write that reasoning down at the
call site and next to closeAgentForRebuild's doc comment, since the
asymmetry across the three rebuild sites needs an explanation the next
reader can find.

Replace the helper-only regression test with one that drives the real
session-operation-queue the same way reloadIfIdle actually calls it (void
enqueue(...), no awaited return value) and asserts, via a real
process.on("unhandledRejection") listener, that the rejection is contained
and surfaces through fatalBuildError instead of escaping. reloadIfIdle
itself can't be reached in isolation without standing up the full TUI
runner; that's noted at the test.

Author: TheGreatAxios

src/tui/runner.ts

@@ -290,11 +290,15 @@ export function resumeTranscriptLoadErrorBlock(err: unknown): {
 // interrupts mid-inference, which is exactly when those paths are under
 // stress), the lock is never released — and because the agent is already
 // marked closed internally, retrying close() is a silent no-op that can
-// never release it either. Every rebuild site must treat that as fatal for
-// the current rebuild instead of calling buildAgent() again: a second
-// createAgent() for the same workdir is then guaranteed to throw
-// AgentContextLockError for a lock nothing will ever free, which is the
-// "agent already open" crash.
+// never release it either. Every rebuild site that reuses the *same* workdir
+// (interrupt, reloadIfIdle) must treat that as fatal for the current rebuild
+// instead of calling buildAgent() again: a second createAgent() for the same
+// workdir is then guaranteed to throw AgentContextLockError for a lock
+// nothing will ever free, which is the "agent already open" crash. Session
+// rotation (newSession) is the one rebuild site that does NOT route through
+// this helper: it always points buildAgent() at a freshly minted workdir
+// before rebuilding, so a leaked lock on the old workdir can never be
+// re-acquired there — see the comment at its close() call for why.
 export async function closeAgentForRebuild(agent: Agent, context: string): Promise<boolean> {
   try {
     await agent.close();
@@ -1901,6 +1905,15 @@ export async function runTUI(initialConfig: Config): Promise<number> {
           // settles, and a dead cycle's partial must land in the session that
           // produced it, not the fresh one.
           await cycleRecorder.dispose("rotation");
+          // Deliberately not routed through closeAgentForRebuild/
+          // agentRebuildFailure (unlike interrupt and reloadIfIdle, CL-5753):
+          // rotation mints a fresh sessionId/workdir below before calling
+          // buildAgent(), so even a close() that leaks the old workdir's lock
+          // (see closeAgentForRebuild's doc comment) can never cause a second
+          // acquisition on that same workdir — buildAgent() always targets
+          // the new, unlocked directory. The old lock still leaks for the
+          // rest of the process, but nothing ever tries to re-acquire it, so
+          // there is no crash to guard against here.
           await currentAgent.close().catch((err: unknown) => {
             tuiLogger.debug("agent.close during session-rotation teardown failed: {error}", {
               error: err instanceof Error ? err.message : String(err),

tests/unit/tui/runner.test.ts

@@ -9,6 +9,7 @@ import {
   loadLocalSettingsWriteBase,
   resumeTranscriptLoadErrorBlock,
 } from "../../../src/tui/runner.js";
+import { createSessionOperationQueue } from "../../../src/tui/session-operation-queue.js";
 import { createRunSink } from "../../../src/session/run-sink.js";
 
 test("createTUIEventEmitter returns an EventEmitter", () => {
@@ -160,3 +161,74 @@ test("a failed close followed by a lock error never surfaces as a raw AgentConte
   expect(rebuildError).not.toBeInstanceOf(AgentContextLockError);
   expect(rebuildError!.message).toMatch(/restart/i);
 });
+
+// reloadIfIdle itself is a closure captured inside runTUI's single ~2500-line
+// scope (currentAgent, buildAgent, streamPromise, workflowController,
+// pendingReload/inFlight, fatalBuildError, etc. are all local variables of
+// that function), with no seam to construct or call it in isolation short of
+// standing up the full TUI runner — provider config, plugin discovery, MCP
+// wiring, and a real OpenTUI host. That is out of scope for this fix; it
+// would be its own extraction. What can be driven directly, and is exactly
+// the failure this bug reports, is the real `session-operation-queue.ts`
+// queue exercised the same way every rebuild site uses it: `void
+// enqueueOp(async () => { try { ... } catch (err) { fatalBuildError = ... } })`.
+// `enqueue` is `tail = tail.then(op, op); return tail;` — if `op` rejects and
+// nothing internally catches it, that returned promise is the only thing
+// that ever observes the rejection, and `void` discards it, which is
+// precisely how the unhandled rejection in the ticket escaped.
+test("a rejecting reload op through the real session-operation-queue never triggers an unhandled rejection", async () => {
+  const { enqueue, awaitTail } = createSessionOperationQueue();
+  const agent = stubAgent(() => Promise.reject(new AgentContextLockError("/tmp/workdir")));
+
+  let unhandled: unknown = null;
+  const onUnhandledRejection = (reason: unknown): void => {
+    unhandled = reason;
+  };
+  process.on("unhandledRejection", onUnhandledRejection);
+
+  let fatalBuildError: Error | null = null;
+  try {
+    // Mirrors reloadIfIdle's body verbatim: close the current agent through
+    // closeAgentForRebuild, skip buildAgent() and throw instead of
+    // re-acquiring on a failed close, and land any failure in
+    // fatalBuildError via agentRebuildFailure — all behind `void enqueueOp`,
+    // exactly as the runner calls it.
+    void enqueue(async () => {
+      try {
+        const closedCleanly = await closeAgentForRebuild(agent, "reload");
+        if (!closedCleanly) {
+          throw new AgentContextLockError("/tmp/workdir");
+        }
+      } catch (err) {
+        fatalBuildError = agentRebuildFailure(err);
+      }
+    });
+
+    await awaitTail();
+    // Give any unhandled rejection queued by the engine a chance to fire
+    // before asserting its absence — it lands on a later microtask/macrotask
+    // than the awaited queue settlement.
+    await new Promise((resolve) => setTimeout(resolve, 0));
+  } finally {
+    process.off("unhandledRejection", onUnhandledRejection);
+  }
+
+  expect(unhandled).toBeNull();
+  expect(fatalBuildError).not.toBeNull();
+  expect(fatalBuildError).not.toBeInstanceOf(AgentContextLockError);
+  expect(fatalBuildError!.message).toMatch(/restart/i);
+});
+
+// A true negative control (reproducing reloadIfIdle's pre-fix shape — no
+// try/catch around the queued op — and asserting the rejection escapes) was
+// attempted here and deliberately removed: bun:test installs its own
+// `unhandledRejection` listener that fails whichever test is running the
+// instant one fires, regardless of what that test asserts, so a test
+// designed to prove an unhandled rejection *does* escape cannot pass in this
+// harness — it is intercepted before the assertion runs. That interception
+// is itself the strongest available evidence for the bug this fix removes:
+// the pre-fix `reloadIfIdle` body run through this exact harness fails the
+// suite outright (confirmed manually while writing this test), rather than
+// failing a single assertion. The test above is the harness-compatible half
+// of that pair: same real queue, same real helpers, proving the fixed shape
+// produces no such failure.