fix(terminal): guard the queued-startup wiring the review found untested

Follow-ups from the final review pass on this branch.

- Collapse the ownership + one-shot decision into `createQueuedStartupConsumer`
  so the call site is a single call rather than inline logic no test could
  reach. Two mutants survived the whole suite before this: relaxing ownership to
  a truthiness check, and dropping the one-shot guard. Both now fail.
- Rewrite the throwing-consume test. It asserted `updateTabPtyId` had been
  called, which runs *before* the callback, so it passed with the try/catch
  deleted. It now asserts the throw does not escape into the connect promise,
  which is the invariant the try/catch actually provides.
- Correct the `onQueuedStartupSpawned` docblock. It claimed the callback is "the
  first moment the command is guaranteed to reach a shell"; the diff's own caveat
  says otherwise, since Windows runs an argv-embedded command before this fires
  and a POSIX shell can die before the shell-ready write. It marks a live shell,
  not delivery.

No behavior change: the consumer is the same predicate and the same one-shot,
moved behind one exported seam.
This commit is contained in:
Jinwoo-H
2026-08-19 17:26:54 -04:00
parent 2651da9b6f
commit a573e4e8e1
4 changed files with 84 additions and 19 deletions
@@ -167,10 +167,25 @@ describe('connectPanePty queued startup consume', () => {
expect(onQueuedStartupSpawned).not.toHaveBeenCalled()
})
it('does not strand the pane without a pty when the consume throws', async () => {
it('keeps a throwing consume from escaping into the spawn', async () => {
const { connectPanePty } = await import('./pty-connection')
const transport = createMockTransport()
spawnOnConnect(transport, 'fresh-pty')
// Mirror the transport contract: onPtySpawn is invoked from inside the connect promise, so
// anything it throws rejects that promise, strands the pane with no pty, and is far worse
// than the stale queued entry the callback exists to clear.
let escapedIntoSpawn = false
transport.connect.mockImplementation(async () => {
const onPtySpawn = createdTransportOptions[0]?.onPtySpawn as
| ((id: string) => void)
| undefined
try {
onPtySpawn?.('fresh-pty')
} catch {
escapedIntoSpawn = true
return undefined
}
return 'fresh-pty'
})
transportFactoryQueue.push(transport)
const updateTabPtyId = vi.fn()
@@ -185,8 +200,7 @@ describe('connectPanePty queued startup consume', () => {
connectPanePty(createPane(1) as never, createManager(1) as never, deps as never)
await flushAsyncTicks(20)
// onPtySpawn runs inside the connect promise: an unguarded throw would reject it and leave the
// pane with no pty at all, which is strictly worse than a stale queued entry.
expect(escapedIntoSpawn).toBe(false)
expect(updateTabPtyId).toHaveBeenCalled()
})
})
@@ -71,7 +71,9 @@ export type PtyConnectionDeps = {
>
clearTabPtyId: (tabId: string, ptyId: string) => void
/** Set only on the pane carrying the tab's queued startup command; called once its own fresh
* spawn exists, which is the first moment the command is guaranteed to reach a shell. */
* spawn exists, which is the first moment a live shell exists to receive it. Not proof of
* delivery: Windows runs an argv-embedded command before this, and a POSIX shell can die
* before the shell-ready write. */
onQueuedStartupSpawned?: () => void
consumeSuppressedPtyExit: (ptyId: string) => boolean
isPtyShutdownPending: (ptyId: string) => boolean
@@ -3,6 +3,7 @@ import {
applyTerminalPaneCloseRequest,
applyTerminalScrollbackRowsToMountedPanes,
clearQueuedInitialCwdAfterFirstPane,
createQueuedStartupConsumer,
getPreviousVisibleForTerminalPane,
isTerminalPaneVisibilityResume,
isTouchIOSUserAgent,
@@ -225,6 +226,36 @@ describe('paneOwnsQueuedStartup', () => {
})
})
describe('createQueuedStartupConsumer', () => {
it('withholds the consumer from a pane that does not own the queued command', () => {
const queuedStartup = { command: 'echo queued' }
const consume = vi.fn()
// A setup split's borrowed payload, structurally identical to the queued command.
expect(
createQueuedStartupConsumer({ command: 'echo queued' }, queuedStartup, consume)
).toBeUndefined()
expect(createQueuedStartupConsumer(null, queuedStartup, consume)).toBeUndefined()
expect(consume).not.toHaveBeenCalled()
})
// Why: onPtySpawn fires again on hibernation wake and the respawn ladder. Spending the slot on a
// later spawn would drop a command queued after the first launch, without ever delivering it.
it('spends the queued command at most once across repeated spawns', () => {
const queuedStartup = { command: 'echo queued' }
const consume = vi.fn()
const consumer = createQueuedStartupConsumer(queuedStartup, queuedStartup, consume)
expect(consumer).toBeTypeOf('function')
consumer?.()
consumer?.()
consumer?.()
expect(consume).toHaveBeenCalledTimes(1)
})
})
describe('splitPaneWithOneShotStartup', () => {
it('only exposes startup to the intentional split and clears it afterwards', () => {
const deps: { startup?: { command: string; env?: Record<string, string> } | null } = {
@@ -492,6 +492,32 @@ export function paneOwnsQueuedStartup(
return queuedStartup != null && paneStartup === queuedStartup
}
/**
* The callback that spends the tab's queued startup command, or `undefined` when this pane does
* not own it.
*
* Why one-shot: `onPtySpawn` fires on every fresh spawn a pane makes — hibernation wake, the
* respawn ladder — but only the first carried the queued command. A command queued onto the tab
* afterwards belongs to that later launch, and spending it here would drop it undelivered.
*/
export function createQueuedStartupConsumer(
paneStartup: object | null | undefined,
queuedStartup: object | null | undefined,
consume: () => void
): (() => void) | undefined {
if (!paneOwnsQueuedStartup(paneStartup, queuedStartup)) {
return undefined
}
let spent = false
return () => {
if (spent) {
return
}
spent = true
consume()
}
}
/** Scopes `deps.startup` to a single call of `splitPane()`, clearing it in `finally` so later splits do not replay the payload. */
export function splitPaneWithOneShotStartup<TPane>(
deps: SplitWithStartupDeps,
@@ -1309,22 +1335,14 @@ export function useTerminalPaneLifecycle({
}
}
applyAppearance(manager)
// Why one-shot: onPtySpawn fires on every fresh spawn this pane makes (hibernation wake,
// the respawn ladder), but only the first carried the queued command. A command queued onto
// the tab afterwards belongs to that later launch, and spending it here would drop it
// without ever delivering it.
let queuedStartupSpent = false
const consumeQueuedStartupOnce = (): void => {
if (queuedStartupSpent) {
return
}
queuedStartupSpent = true
useAppStore.getState().consumeTabStartupCommand(tabId)
}
const ownsQueuedStartup = paneOwnsQueuedStartup(ptyDeps.startup, startupWithSetupSplitWait)
const onQueuedStartupSpawned = createQueuedStartupConsumer(
ptyDeps.startup,
startupWithSetupSplitWait,
() => useAppStore.getState().consumeTabStartupCommand(tabId)
)
const panePtyBinding = connectPanePty(pane, manager, {
...ptyDeps,
...(ownsQueuedStartup ? { onQueuedStartupSpawned: consumeQueuedStartupOnce } : {}),
...(onQueuedStartupSpawned ? { onQueuedStartupSpawned } : {}),
// Why: spread order matters — spawnHints.cwd (source pane) must override ptyDeps.cwd (worktree root) so splits boot in the live cwd.
...(spawnHints?.cwd ? { cwd: spawnHints.cwd } : {}),
restoredPtyIdByLeafId: spawnHints?.ptyId