Files
orca/docs/audits/pending-split-close/fix.patch
T
69246e9b06 fix(terminal): retire explicitly closed pending split connections (#21001)
* fix(terminal): retire explicitly closed pending split connections

* test(memory): keep pending split proof compatible with formatted source

* fix(terminal): confirm pending split retirement before stopping work

* fix(terminal): restore the pending split-close gates CI checks

Three CI gates were red on this branch and all three were this branch's own.

The hook-order parity snapshot did not count the `confirmedCloseRef` this
branch adds to `use-terminal-pane-close-actions.ts`. Dumping the flattened
order against clean `main` shows exactly one added `useRef` at position 148
and no reordering, so the count moves 211 -> 212 and the digest with it.

`pending-split-close-test-fixture.ts` is Vitest support code, but it sits
outside the `*.test` / `*.spec` / `tests` globs that already switch
`anti-slop/no-module-mocking` off, so the gate failed on all twelve of its
`vi.mock` calls. It carries a file-scoped disable with the reason, matching
`work-item-search-test-harness.ts`.

`fix.patch` still described the pre-confirmation shape of the close hook, so
`reproduce.mjs` aborted with `Source changed` and the cited ablation could not
run at this head. Regenerated against the committed sources; the harness again
reports 10 pass / 14 fail before and 24 pass / 0 fail after.

Merges `main` rather than rebasing: #21005 is stacked on this branch.

---------

Co-authored-by: m4air <m4air@Mac.localdomain>
Co-authored-by: Neil <neil@stably.ai>
2026-09-17 23:52:01 -07:00

238 lines
11 KiB
Diff

diff --git a/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts b/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts
index 3023236de0e..52b983c51fd 100644
--- a/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts
+++ b/src/renderer/src/components/terminal-pane/ipc-pty-connect.ts
@@ -27,6 +27,7 @@ type IpcPtyConnectContext = {
/** True only for the one buffered exit consumed by this connect attempt. */
isExpectedExitCurrent: () => boolean
ownsPtyId: (id: string) => boolean
+ handleExplicitlyClosedConnect?: (id: string) => boolean
bind: (id: string) => void
isCurrent: (id: string) => boolean
setCallbacks: (callbacks: PtyConnectOptions['callbacks']) => void
@@ -89,6 +90,9 @@ export async function connectIpcPty(
const priorIncarnationFence = currentPreHandlerPtySequence()
const spawnResult = await spawnIpcPty(transportOptions, options, admittedSessionId)
const retireFreshSpawn = async (): Promise<void> => {
+ if (context.handleExplicitlyClosedConnect?.(spawnResult.id)) {
+ return
+ }
// A newer generation may already own a recycled id; an id-only kill would retire its PTY.
if (
!spawnResult.isReattach &&
diff --git a/src/renderer/src/components/terminal-pane/pty-transport-types.ts b/src/renderer/src/components/terminal-pane/pty-transport-types.ts
index 4d7eaf7358b..2f6ed734bbf 100644
--- a/src/renderer/src/components/terminal-pane/pty-transport-types.ts
+++ b/src/renderer/src/components/terminal-pane/pty-transport-types.ts
@@ -232,7 +232,10 @@ export type PtyTransport = {
* it also drops the transport's output processor from the pty side-effect memory census,
* so a reattached one would run untracked. Create a new transport instead. */
detach?: (options?: { preserveExitObserver?: boolean }) => void
- destroy?: () => void | Promise<void>
+ destroy?: (options?: {
+ /** Explicit close can retain retirement intent until an unbound connect settles. */
+ onAbandonedConnect?: (ptyId: string) => boolean
+ }) => void | Promise<void>
}
export type IpcPtyTransportOptions = {
diff --git a/src/renderer/src/components/terminal-pane/pty-transport.ts b/src/renderer/src/components/terminal-pane/pty-transport.ts
index f794b9a1e4d..7731e75eac5 100644
--- a/src/renderer/src/components/terminal-pane/pty-transport.ts
+++ b/src/renderer/src/components/terminal-pane/pty-transport.ts
@@ -44,6 +44,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
} = opts
let connected = false
let destroyed = false
+ let onAbandonedConnect: ((ptyId: string) => boolean) | undefined
let ptyId: string | null = null
let lifecycleGeneration = 0
let lastExitGeneration: number | null = null
@@ -137,6 +138,7 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
lastExitGeneration === lifecycleGeneration &&
lifecycleGeneration === connectGeneration + 1,
ownsPtyId: (id) => !destroyed && connected && ptyId === id,
+ handleExplicitlyClosedConnect: (id) => destroyed && (onAbandonedConnect?.(id) ?? false),
bind,
isCurrent: (id) => lifecycleGeneration === connectGeneration && connected && ptyId === id,
setCallbacks,
@@ -268,7 +270,8 @@ export function createIpcPtyTransport(opts: IpcPtyTransportOptions = {}): PtyTra
: { ...(opts.cwd ? { cwd: opts.cwd } : {}), ...(shellOverride ? { shellOverride } : {}) },
resetCrossChunkParserState: outputProcessor.resetAgentStatusCarry,
- destroy() {
+ destroy(options) {
+ onAbandonedConnect ??= options?.onAbandonedConnect
destroyed = true
try {
this.disconnect()
diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.ts
index 330b42166cd..ea85e929e81 100644
--- a/src/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.ts
+++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-close-actions.ts
@@ -1,4 +1,4 @@
-import { useCallback, useImperativeHandle } from 'react'
+import { useCallback, useImperativeHandle, useRef } from 'react'
import { useAppStore } from '../../store'
import type { PaneExternalDropTarget } from '@/lib/pane-manager/pane-manager'
import { makePaneKey } from '../../../../shared/stable-pane-id'
@@ -13,8 +13,11 @@ import {
} from './terminal-pane-tab-detach'
import { clearPaneTerminalError } from './terminal-error-accumulation'
import type { TerminalPaneBindingController } from './use-terminal-pane-layout-bindings'
+import { retireUnboundIpcTerminalPane } from './retire-unbound-ipc-terminal-pane'
+import { capturePendingTerminalPaneClose } from './terminal-pane-close-admission'
export function useTerminalPaneCloseActions(controller: TerminalPaneBindingController) {
+ const confirmedCloseRef = useRef<(() => void) | null>(null)
const {
clearSessionRestoredBannerForPane,
managerRef,
@@ -46,6 +49,13 @@ export function useTerminalPaneCloseActions(controller: TerminalPaneBindingContr
clearSessionRestoredBannerForPane(paneId)
const leafId = manager.getLeafId(paneId)
if (leafId) {
+ retireUnboundIpcTerminalPane({
+ getState: useAppStore.getState,
+ tabId,
+ leafId,
+ transport: paneTransportsRef.current.get(paneId),
+ getTransports: () => paneTransportsRef.current
+ })
useAppStore.getState().setCacheTimerStartedAt(makePaneKey(tabId, leafId), null)
useAppStore.getState().dropAgentStatus(makePaneKey(tabId, leafId), { paneRemoved: true })
}
@@ -79,12 +89,18 @@ export function useTerminalPaneCloseActions(controller: TerminalPaneBindingContr
return
}
const transport = paneTransportsRef.current.get(paneId)
- const ptyId = transport?.getPtyId()
+ const pending = capturePendingTerminalPaneClose(controller, paneId, useAppStore.getState)
+ const ptyId = transport?.getPtyId() ?? pending?.ptyId
if (!ptyId) {
executeClosePane(paneId)
return
}
const settings = useAppStore.getState().settings
+ const close = (): void => {
+ if (!pending || pending.isCurrent()) {
+ executeClosePane(paneId)
+ }
+ }
let decided = false
const decide = (act: () => void): void => {
if (decided) {
@@ -93,12 +109,23 @@ export function useTerminalPaneCloseActions(controller: TerminalPaneBindingContr
decided = true
act()
}
- const confirmClose = (): void =>
+ const confirmClose = (): void => {
+ if (pending && !pending.isCurrent()) {
+ return
+ }
+ confirmedCloseRef.current = close
setPendingCloseConfirmation({
paneId,
copyKind: getCloseDialogCopyKind(paneId)
})
- const probeTimeout = setTimeout(() => decide(confirmClose), RUNNING_CLOSE_PROBE_TIMEOUT_MS)
+ }
+ const probeTimeout = setTimeout(
+ () =>
+ decide(
+ pending && settings?.skipCloseTerminalWithRunningProcessConfirm ? close : confirmClose
+ ),
+ RUNNING_CLOSE_PROBE_TIMEOUT_MS
+ )
// Why the shared probe rather than a direct inspect: this is the same question the tab-close
// guard asks, and the two must not drift on what an unanswered host means.
void probePtyRunningWork(settings, [ptyId], { timeoutMs: RUNNING_CLOSE_PROBE_TIMEOUT_MS })
@@ -106,10 +133,10 @@ export function useTerminalPaneCloseActions(controller: TerminalPaneBindingContr
clearTimeout(probeTimeout)
decide(() => {
if (
- probes[0]?.verdict !== 'live' ||
+ (pending ? probes[0]?.verdict === 'exited' : probes[0]?.verdict !== 'live') ||
settings?.skipCloseTerminalWithRunningProcessConfirm
) {
- executeClosePane(paneId)
+ close()
} else {
confirmClose()
}
@@ -117,7 +144,9 @@ export function useTerminalPaneCloseActions(controller: TerminalPaneBindingContr
})
.catch(() => {
clearTimeout(probeTimeout)
- decide(() => executeClosePane(paneId))
+ decide(
+ pending && !settings?.skipCloseTerminalWithRunningProcessConfirm ? confirmClose : close
+ )
})
},
// oxlint-disable-next-line react-hooks/exhaustive-deps -- Preserve the pre-split dependency contract.
@@ -143,22 +172,24 @@ export function useTerminalPaneCloseActions(controller: TerminalPaneBindingContr
}, [])
const handleConfirmClose = useCallback(
(dontAskAgain: boolean) => {
- if (pendingCloseConfirmation === null) {
+ if (pendingCloseConfirmation === null || confirmedCloseRef.current === null) {
return
}
- const paneId = pendingCloseConfirmation.paneId
+ const confirmedClose = confirmedCloseRef.current
+ confirmedCloseRef.current = null
setPendingCloseConfirmation(null)
if (dontAskAgain) {
void updateSettings({
skipCloseTerminalWithRunningProcessConfirm: true
})
}
- executeClosePane(paneId)
+ confirmedClose()
},
// oxlint-disable-next-line react-hooks/exhaustive-deps -- Preserve the pre-split dependency contract.
[executeClosePane, pendingCloseConfirmation, updateSettings]
)
const handleCancelClose = useCallback(() => {
+ confirmedCloseRef.current = null
setPendingCloseConfirmation(null)
// oxlint-disable-next-line react-hooks/exhaustive-deps -- Preserve the pre-split dependency contract.
}, [])
diff --git a/src/renderer/src/store/slices/terminal-tab-retirement.ts b/src/renderer/src/store/slices/terminal-tab-retirement.ts
index 80e67824699..e2e034fa6af 100644
--- a/src/renderer/src/store/slices/terminal-tab-retirement.ts
+++ b/src/renderer/src/store/slices/terminal-tab-retirement.ts
@@ -135,6 +135,30 @@ export function isTerminalTabPresent(
return locateTerminalTab(state.tabsByWorktree, tabId) !== null
}
+export function hasTerminalPtyOwnerOutsidePane(
+ state: TerminalTabRetirementState,
+ identity: string,
+ tabId: string,
+ excludedLeafId?: string
+): boolean {
+ for (const [ownerTabId, owner] of collectLiveTerminalTabs(state)) {
+ const ids =
+ ownerTabId === tabId
+ ? Object.entries(state.terminalLayoutsByTabId[tabId]?.ptyIdsByLeafId ?? {})
+ .filter(([leafId]) => leafId !== excludedLeafId)
+ .map(([, ptyId]) => ptyId)
+ : collectPtyIdsForTab(state, ownerTabId, owner.rowPtyId)
+ if (
+ ids.some(
+ (ptyId) => getTerminalPtyOwnershipIdentity(state, ptyId, owner.worktreeId) === identity
+ )
+ ) {
+ return true
+ }
+ }
+ return false
+}
+
export function buildTerminalTabRetirementPlan(
state: TerminalTabRetirementState,
tabId: string