Fix XtermPane IPC listener leak on unmount-during-spawn/adopt

Pre-release audit finding: after `unlistenData = await onPaneData(...)` (and
the exit listener) there was no destroyed re-check, so if the pane unmounted
during the await the sync cleanup captured a null unlisten and the
pane://{id}/data subscription leaked. Unlisten before returning in both the
adopt and spawn paths.

Also logs the deferred (low-risk) transfer-refcount leak as a known follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
megaproxy 2026-05-28 20:34:36 +01:00
parent e6d0040021
commit 309b6024d4
2 changed files with 28 additions and 1 deletions

View file

@ -127,6 +127,13 @@ Smoke test on Windows revealed bugs specific to detached (non-main) windows. Mai
- **Native scrollbars (fixed).** `::-webkit-scrollbar` theming was scoped to `.xterm-viewport` only; made it global (`*::-webkit-scrollbar` + `* { scrollbar-width/color }`) so the tab strip / panels / menus match the dark theme.
- **Capability fix recap:** `default.json` `"windows": ["main", "pane-window-*"]` — the load-bearing fix for the whole detached-window feature (B2B5). Confirmed: app-defined Tauri commands aren't individually permission-gated; they're available to any window the capability is *applied* to (listed in `windows`).
**Pre-release audit (2026-05-28, 3-agent fan-out) — findings + dispositions:**
- **(FIXED, medium) XtermPane IPC listener leak on unmount-during-await.** After `unlistenData = await onPaneData(...)` / `unlistenExit = await onPaneExit(...)` there was no `destroyed` re-check, so if the pane unmounted during the await (StrictMode, fast moveToNewWindow/closeTab) the sync cleanup had already captured a null unlisten and the `pane://{id}/data`/exit subscription leaked. Added `if (destroyed) { unlistenData?.(); unlistenExit?.(); return; }` after each assignment in both adopt and spawn paths.
- **(DEFERRED, high — known low-risk) Transferred-PTY/refcount leak if a detached window closes mid-adopt.** `mark_pane_transferring` bumps a refcount that suppresses `kill`; only `claim_pane` (from the target XtermPane mount) drops it. The `CloseRequested` handler (lib.rs:74) forgets workspaces + pending-init but never releases the refcount or kills the pane → if the window closes before adopt's `claim_pane`, that PTY + reader thread leak for the app lifetime. **In practice very low-probability**: adopt of a transferred pane is near-instant (paneId known synchronously, no spawn wait), so `claim` runs within ms of mount — by the time a user sees and closes the window, it's already claimed. User chose ship-now. **Proper fix when revisited:** keep a `label→paneId` "adopting" registry (set when `take_pending_window_init` consumes the payload, cleared by `claim_pane`), and have the close handler force-kill (drop refcount + kill) any still-unclaimed paneId for the closing label. The unconsumed-pending-init subset can be handled more cheaply (close handler already has the PendingInit.pane_id when the entry is still present).
- **(NOT FIXING, low) waitForPaneRegistration doesn't settle on early unmount** — `registerPaneId(leafId, null)` doesn't reject a pending waiter, so moveToNewWindow/MCP-spawn stalls until the timeout instead of failing fast. Functionally safe (timeout fires).
- tabs/LeafPane/TabStrip reviewer: no findings.
2. `pnpm tauri dev` — smoke test:
- Existing workspace loads as one tab named "Default" ✓ migrate
- Ctrl+T spawns new tab with default-shell pane

View file

@ -186,11 +186,22 @@ export default function XtermPane({
term?.write(b64ToBytes(b64));
onDataReceivedRef.current?.();
});
if (destroyed) return;
// `destroyed` may have flipped during the await — the sync cleanup
// already ran and captured a null unlisten, so unlisten here or the
// subscription leaks.
if (destroyed) {
unlistenData?.();
return;
}
unlistenExit = await onPaneExit(paneId, () => {
term?.write("\r\n\x1b[33m[pane exited]\x1b[0m\r\n");
onStatusRef.current?.(`pane ${paneId} exited`, false);
});
if (destroyed) {
unlistenData?.();
unlistenExit?.();
return;
}
// Match the PTY to our cell grid (the source window may have had
// different dimensions).
try {
@ -227,11 +238,20 @@ export default function XtermPane({
term?.write(b64ToBytes(b64));
onDataReceivedRef.current?.();
});
if (destroyed) {
unlistenData?.();
return;
}
unlistenExit = await onPaneExit(paneId, () => {
term?.write("\r\n\x1b[33m[pane exited]\x1b[0m\r\n");
onStatusRef.current?.(`pane ${paneId} exited`, false);
});
if (destroyed) {
unlistenData?.();
unlistenExit?.();
return;
}
}
term?.onData((data) => {