A throwing flush can skip pty.kill() and orphan a shell on quit

[lukejohnson3-hub]

Pre-existing bug on main — nothing in this PR introduces it. A shell process can survive the app on quit. Bug fix only, non-breaking: 2 files, +84 −8, one commit off main.

destroyAllTerminals wraps flush, dispose and kill in a single try. If the flush throws, pty.kill() is skipped — and the this.terminals.clear() immediately below drops the last handle to that PTY, so nothing is left able to reclaim it. The fix gives each teardown step its own catch.

This is the follow-through on #561, where I described the bug and offered to send this fix on its own. You have not heard from me before, so every claim below points at code in your own tree.

The bug

main/src/services/terminalPanelManager.ts, current main:

destroyAllTerminals(): void {
  for (const [panelId, terminal] of this.terminals) {
    try {
      this.saveTerminalState(panelId);
      ...
      disposeFlowControlRecord(terminal.flowControl);
      this.flushOutputBuffer(terminal);      // ← throws here…
      terminal.screenEmulator?.dispose();

      terminal.pty.kill();                    // ← …and this never runs
    } catch (error) {
      console.error(`[TerminalPanelManager] Error killing terminal ${panelId}:`, error);
    }
  }

  this.terminals.clear();                     // ← last handle to that PTY, gone
  ...
}

The flush can throw, and the path is short enough to check by reading:

• flushOutputBuffer calls sendRendererEvent('terminal:output', …) for visible terminals, and sendDaemonEvent(…) for hidden ones.
• Those are getPaneEventSink().send(…) and getPaneDaemonEventSink().send(…).
• main/src/daemon/bootstrap.ts installs a createFanoutEventSink for both, and that sink (main/src/core/eventSink.ts) collects the first subscriber error and rethrows it after fanning out.

Concrete throw sources: the Electron sink in main/src/index.ts guards window.isDestroyed() but then calls window.webContents.send(...) — a destroyed webContents under a live window throws. On the daemon side, terminal:output is a daemon channel, and serializeJsonTransport / encodePaneDaemonFrame are not wrapped, only writeFrame is. A renderer going away during quit is not an exotic condition — it is the normal shape of shutdown.

A second, narrower route skips the sinks entirely: flushOutputBuffer → flowControlOnPtyBytes → onPause() → pausePty, which calls terminal.pty.pause() synchronously on a PTY that by this point has taken two Ctrl-Cs and a 2-second wait and may be dead. It needs pendingBytes >= HIGH_WATERMARK, but it is real.

Why it matters

destroyAllTerminals has exactly one caller, main/src/index.ts, on the quit path. The line above the call reads:

// Kill all terminal panel PTY processes so Claude doesn't survive as an orphan
terminalPanelManager.destroyAllTerminals();

The defect defeats the stated purpose of the call, in the one place every terminal is torn down at once. On Windows each Git Bash terminal is a three-process chain, so a single skipped kill leaks three processes, and they are unreachable afterwards because the map has been cleared. The try/catch sits inside the loop, so a throw costs only that one terminal — one leaked chain per throwing subscriber, not all of them. The catch does log, so the failure is not invisible — but it logs Error killing terminal <panelId> while the terminal was in fact never killed, which reads as the opposite of what happened.

The fix

Three changes, all of them error handling:

1. Each teardown step gets its own catch, so a failure in one cannot skip the others — flushOutputBuffer (warn and continue), screenEmulator?.dispose() (warn and continue; it serializes through a third-party addon and can also throw), pty.kill() (keeps the existing error log).
2. An outer catch around the whole iteration, so one terminal can never abort the loop. That failure would be wider than the one being fixed: every later PTY unkilled, the clear() calls skipped, and the caller hard-exits straight afterwards.
3. .catch() on the un-awaited this.saveTerminalState(panelId), in both destroyAllTerminals and destroyTerminal, which had the same call with no catch at all. It is async, so no synchronous try could ever have observed its rejection, and panelManager.updatePanel writes to SQLite mid-shutdown. There is no unhandledRejection handler in the main process, and Electron 41 bundles Node 24, where the default is to throw. It is deliberately a .catch and not an await: TerminalStateEmulator.dispose() documents that the save is meant to read the post-dispose snapshot, so awaiting would be a behaviour regression. Including destroyTerminal closes the class rather than one instance, in the same function family, at no extra scope.

No behaviour change when nothing throws. The loop still visits every terminal, the maps are still cleared, and the ordering of flush → dispose → kill is unchanged. That ordering is load-bearing: moving disposeFlowControlRecord after the flush would let flowControlOnPtyBytes re-arm a 5-second safety timer that nothing then clears.

One repair comes for free: dispose() now always runs, and it is what resolves the emulator's idleResolvers. Previously a throwing flush skipped dispose(), leaving saveTerminalState's await waitForIdle() pending forever — so that panel's state was silently lost on quit, on top of the orphaned shell.

Testing notes

pnpm typecheck
pnpm lint
cd main && pnpm exec vitest run

destroyAllTerminals had no test coverage at all. This adds one: two terminals, the first with a flushOutputBuffer that throws, asserting both PTYs are killed and the terminals map ends empty. It fails on current main, where the throwing terminal's pty.kill() is skipped.

Worth a maintainer's eye, and not something unit tests can approximate: this is a quit-path change, and packaged-build quit behaviour is exactly what unit tests cannot reach. A useful regression smoke test is to quit a packaged build with two or more live agent panels, then confirm no shell processes survive.

Unrelated to this PR: if you run the suite on a machine where python3 resolves to the Windows Store alias stub, main/src/services/skillCacheManager.test.ts fails, because one of its three cases is missing the it.skipIf(pythonProbe.status !== 0) its siblings carry. Pre-existing and untouched here.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Checklist

• I have read the CONTRIBUTING.md guidelines
• My code follows the code style of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation — none needed; no documented behaviour changes
• My changes generate no new warnings
• I have added tests that prove my fix is effective
• New and existing unit tests pass locally with my changes
• I have run pnpm typecheck and pnpm lint locally
• I have tested the Electron app locally with pnpm electron-dev — verified against the unit suite; the failure path needs a throwing event-sink subscriber, which the test injects directly

Critical Areas Modified

None. Session output handling, timestamp handling, state management/IPC events and diff viewer CSS are all untouched. flushOutputBuffer keeps its position and behaviour; only the error handling around it changes.

Additional Notes

I found this while investigating a bash.exe process leak on Windows, where terminal shells accumulated until the machine could not fork. That investigation produced a second, much larger change — a bound on how many terminal PTYs stay resident — which is finished and reviewed but deliberately not attached to this PR. This one stands entirely on its own, and is the more urgent of the two: it affects every quit on current main, not only a workspace that has accumulated terminals. Nothing here depends on #561 being read, or on any decision about the larger change.

Also deliberately not here:

• The WSL teardown ordering bug PTY teardown can orphan a process; nothing bounds resident terminals #561 mentions in the same area. It ships with the larger change, because its test needs harness changes this PR does not make.
• destroyAllTerminals kills WSL terminals immediately, where destroyTerminal writes exit\r and defers the kill 500 ms. That asymmetry looks intentional for a quit path that is already racing a shutdown deadline, and changing it is a separate judgment call.
• index.ts already awaits saveAllTerminalStates() for every panel in an earlier shutdown phase, so the fire-and-forget save in destroyAllTerminals is redundant on the only production call path — and it is the sole source of the rejection this PR is catching. Removing it would be the deeper fix, but it is more invasive than this change should be.

Happy to adjust anything here — scope, naming, the test, or the comment wording.