From 9757224349549fab13af1f7d44dfd70f6940a28e Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Thu, 27 Aug 2026 15:19:43 +0800 Subject: [PATCH] fix(subprocess): fence each signal against current process state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the shared observation defeated the very fence it fed: it carries the original PID-to-start-time pairing forward, so a recycled PID still matches it and takes a signal meant for the process that exited. Capturing it outside the per-member try also let one failed read abort a whole teardown round, breaking the synchronous host-exit contract, and an empty round paid a read for no members. signalProcess now reads ProcessInspector.isAlive immediately before delivering, from the narrowest per-identity source each platform offers; signalMembers and waitForMembers return before capturing when a round has no members. snapshot() keeps serving the readiness poll, whose per-poll table read stays at one. Windows enumerates Toolhelp32 lazily on the first tree question, so a snapshot asked only for liveness — the 25 ms teardown poll — performs no table walk at all. --- ...26-08-27-process-table-snapshots.i18n.yaml | 4 +- .../2026-08-27-process-table-snapshots.md | 16 ++++-- .../2026-08-27-process-table-snapshots.zh.md | 16 ++++-- .../subprocess-local/src/process-inspector.ts | 55 ++++++++++++++----- .../subprocess-local/src/terminal.ts | 18 +++--- .../subprocess-local/src/windows-inspector.ts | 27 +++++---- .../subprocess-local/tests/local.spec.ts | 2 + .../tests/process-exit.spec.ts | 3 +- .../tests/process-inspector.spec.ts | 31 ++++++++--- .../subprocess-local/tests/terminal.spec.ts | 47 ++++++++++++++-- .../tests/windows-inspector.spec.ts | 49 ++++++++++++----- .../terminal-bash/tests/session.spec.ts | 1 + 12 files changed, 196 insertions(+), 73 deletions(-) diff --git a/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.i18n.yaml b/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.i18n.yaml index 01a60f2417..138ca27a49 100644 --- a/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.i18n.yaml +++ b/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write .agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.md -2026-08-27-process-table-snapshots.md: 2f031cc2952ffe1c007e04acf4dabbeac0630630 -2026-08-27-process-table-snapshots.zh.md: fbad15c306d250aeb64247a301ed20777f39c4a3 +2026-08-27-process-table-snapshots.md: 27c364607ca1e03a926c309f26007477a8785636 +2026-08-27-process-table-snapshots.zh.md: 9c5fe65bb83a0d04fe5639b3ffefcf377c3588ef diff --git a/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.md b/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.md index 2f031cc295..27c364607c 100644 --- a/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.md +++ b/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.md @@ -30,19 +30,23 @@ Teardown has the same structure. `signalProcess` fences each signal against PID Each caller captures one snapshot and answers every question of a single pass from it. `LocalTerminalHandle.descendants()` takes a snapshot, reads the tree and session from it, and filters survivors through the same `alive`, so a readiness poll costs one table read regardless of descendant count. `waitForMembers` captures a fresh snapshot per polling iteration, because its whole purpose is observing change. -`signalProcess(identity, signal, observed)` takes the caller's observation rather than reading the table itself. The PID-reuse fence stays, and `signalMembers` now captures once for a whole signalling round instead of once per member. Passing the observation explicitly is what keeps Linux teardown from regressing: `alive` there is answered from a `/proc` walk the snapshot already paid for, not from a fresh walk per member. +Signalling does not share that observation. `ProcessInspector.isAlive(identity)` answers current state from the narrowest per-identity source a platform offers — one `/proc//stat` read on Linux, one `ps` table on macOS, one process-handle check on Windows — and `signalProcess` takes that fence immediately before delivering the signal. An observation cannot stand in for it: the observation preserves the original PID-to-start-time pairing, so a recycled PID would still match it and take a signal meant for the process that exited. Reading the fence per target also keeps a failed read costing one target instead of the rest of a teardown round, which is what the [synchronous exit-cleanup contract](../bug-fix/2026-08-11-synchronous-subprocess-exit-cleanup.md) requires. + +`signalMembers` and `waitForMembers` return before capturing anything when a round has no members, so a command that spawned no descendants pays no table read for its teardown sweeps. Platform differences live in how a snapshot is built, not in what it promises: - **macOS** builds it from one `ps` table. That table exposes neither a session id nor a state column, so `session` is empty and `alive` reports presence with a matching start identity. - **Linux** walks `/proc` once, carrying each entry's parent, start identity, session, and state. `alive` treats the `Z`, `X`, and `x` states as quiescent, as a per-pid `stat` read did. -- **Windows** captures the Toolhelp32 enumeration for `tree`, has no POSIX sessions, and answers `alive` from the live process handle, because wait state is not a table column there. +- **Windows** enumerates Toolhelp32 lazily, on the first `tree` question. It has no POSIX sessions, and answers `alive` from the live process handle, because wait state is not a table column there — so a snapshot asked only for liveness never enumerates. The terminal's Windows teardown polls liveness every 25 ms and would otherwise walk and discard the whole table each time. `PosixProcessSnapshot` holds both POSIX shapes: a row's `session` and `state` are `undefined` where the platform's table omits them, which is what makes the macOS answers fall out of the shared implementation instead of a second class. ## Testing -`packages/subprocess/subprocess-local/tests/terminal.spec.ts` drives a real `MacProcessInspector` over an injected `exec` and asserts one foreground inspection performs exactly one `-axo` table read at 0, 2, and 10 descendants. That count, not wall time, is the durable invariant: it holds on any host and fails the moment a caller re-reads the table per member. +`packages/subprocess/subprocess-local/tests/terminal.spec.ts` drives a real `MacProcessInspector` over an injected `exec` and asserts one foreground inspection performs exactly one `-axo` table read at 0, 2, and 10 descendants. That count, not wall time, is the durable invariant: it holds on any host and fails the moment a caller re-reads the table per member. The same file pins that a signalling round with no members captures nothing, and that a capture failure during synchronous host exit still lets the PTY root be killed. + +`process-inspector.spec.ts` pins the fence directly: an identity observed alive and then absent from the table takes no signal. `windows-inspector.spec.ts` pins that a snapshot answering only liveness performs no Toolhelp32 enumeration. ## Alternatives considered @@ -50,7 +54,7 @@ Platform differences live in how a snapshot is built, not in what it promises: **Caching the macOS table inside `MacProcessInspector` behind a short TTL.** This needs no interface change, but it makes staleness invisible: a caller cannot tell whether a liveness answer came from this instant or from the end of the previous poll, and a signal decided on a stale row is exactly what the PID-reuse fence exists to prevent. Hidden caching also conflicts with the repository's preference for explicit defaulting and explicit boundaries. -**Keeping `isAlive` on the inspector next to `snapshot()`.** This avoids touching the signalling call sites, at the cost of two ways to ask one question, where only one of them is cheap in a loop. The asymmetry would have to be re-explained at every call site. +**Fencing signals with the round's shared observation.** This removes the last per-member read and was the shape first implemented here. Review rejected it: the fence exists to defeat PID reuse, and an observation defeats the fence instead, because it carries the original PID-to-start-time pairing forward. The window is narrow — the kills in one round are microseconds apart, against a PID space of 99999 on macOS and 4194304 on Linux — but the `README` states the guarantee without qualification, and buying microseconds of teardown time by weakening it is the wrong trade. Keeping both `snapshot().alive` and `isAlive` is therefore not two ways to ask one question: one asks what the table showed, the other asks what is true now, and only the second may decide a signal. **Making `exec` asynchronous instead of reducing the read count.** An async `execFile` stops the poll from blocking the loop but still forks N+1 processes per poll; on a busy machine that trades a stall for sustained fork pressure. It remains a worthwhile follow-up on top of the reduced count, not a substitute for it. @@ -58,9 +62,9 @@ Platform differences live in how a snapshot is built, not in what it promises: A readiness poll's process-table cost is now constant in descendant count. On macOS one poll performs one full table read plus the small `tpgid` read, which is the 0-descendant cost in the table above for every descendant count. -Liveness for a single identity on Linux costs a full `/proc` walk rather than one `stat` read. Every caller that asks about several identities amortizes that walk across them, which is why `signalProcess` takes an observation rather than capturing its own; a future caller that genuinely needs one isolated liveness answer pays more than it did. +Teardown keeps its previous per-signal cost: one narrow liveness read per target, which on macOS is one `ps` fork per member. That cost was never the measured problem — a terminal tears down once, while its readiness path polls up to 600 times — so the fix deliberately spends it to keep the fence reading current state. -A snapshot is a point-in-time view, and the type's documentation says so. Holding one across an `await` and then signalling from it would widen the PID-reuse window that the fence narrows; `waitForMembers` re-captures per iteration for exactly this reason. +A snapshot is a point-in-time view, and the type's documentation says so. `waitForMembers` re-captures per iteration because observing change is its purpose, and no signal is ever decided from a captured view. Every `ProcessInspector` implementation and test fake carries the new shape, including the Windows inspector and the `dsh-terminal-bash` session fake. Test fakes that previously replaced `processTree`, `processSession`, or `isAlive` to stage a scan now replace the corresponding per-question read hook, which keeps their staging behavior and call-counting identical. diff --git a/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.zh.md b/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.zh.md index fbad15c306..9c5fe65bb8 100644 --- a/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.zh.md +++ b/.agents/notes/implemented/architecture/2026-08-27-process-table-snapshots.zh.md @@ -30,19 +30,23 @@ Status: implemented 每个调用方捕获一次快照，并从中回答本次流程的全部问题。`LocalTerminalHandle.descendants()` 取一次快照，从中读取树与会话，并用同一个 `alive` 过滤幸存者，因此一次就绪轮询无论有多少子进程都只读一次表。`waitForMembers` 每一轮轮询各捕获一次新快照，因为它的用途正是观察变化。 -`signalProcess(identity, signal, observed)` 接收调用方的观察，而不是自己去读表。PID 复用围栏保留，而 `signalMembers` 现在为整轮信号只捕获一次，而不是每个成员各一次。把观察显式传入正是 Linux 拆卸不退化的原因：那里的 `alive` 由快照已经付过代价的一次 `/proc` 遍历回答，而不是每个成员各遍历一次。 +发信号不共用这份观察。`ProcessInspector.isAlive(identity)` 用各平台最窄的按标识来源回答当前状态——Linux 读一个 `/proc//stat`、macOS 读一次 `ps` 表、Windows 查一次进程句柄——`signalProcess` 在投递信号前就地取这道围栏。观察无法代替它：观察把原始的「PID 与起始时间」配对保留了下来，因此被复用的 PID 仍会与之匹配，并领走本该发给已退出进程的信号。逐目标读取围栏还让一次失败的读取只损失一个目标，而不是整轮拆卸的其余部分，这正是[同步退出清理约定](../bug-fix/2026-08-11-synchronous-subprocess-exit-cleanup.zh.md)的要求。 + +`signalMembers` 与 `waitForMembers` 在一轮没有成员时直接返回、不做任何捕获，因此没有派生子进程的命令，其拆卸扫描不付表读取代价。 平台差异体现在快照如何构建，而不在它承诺什么： - **macOS** 由一张 `ps` 表构建。该表既不暴露会话 id 也不暴露状态列，所以 `session` 为空，`alive` 报告的是「存在且起始标识匹配」。 - **Linux** 遍历一次 `/proc`，携带每个条目的父进程、起始标识、会话与状态。`alive` 把 `Z`、`X`、`x` 状态视为静止，与按 pid 读 `stat` 的判定一致。 -- **Windows** 捕获 Toolhelp32 枚举供 `tree` 使用，没有 POSIX 会话，并且从活的进程句柄回答 `alive`，因为等待状态在那里不是表的一列。 +- **Windows** 把 Toolhelp32 枚举惰性化到第一次 `tree` 提问。它没有 POSIX 会话，并且从活的进程句柄回答 `alive`，因为等待状态在那里不是表的一列——所以只问存活的快照永不枚举。终端在 Windows 上的拆卸每 25 ms 轮询一次存活，否则每次都会遍历整张表再丢弃。 `PosixProcessSnapshot` 同时承载两种 POSIX 形态：当平台的表省略某字段时，该行的 `session` 与 `state` 为 `undefined`，这使得 macOS 的答案从共享实现中自然得出，而不必新增一个类。 ## Testing -`packages/subprocess/subprocess-local/tests/terminal.spec.ts` 通过注入的 `exec` 驱动真实的 `MacProcessInspector`，断言一次前台检查在 0、2、10 个子进程下都恰好执行一次 `-axo` 表读取。这个次数——而非墙钟时间——才是持久不变量：它在任何主机上都成立，并且在任何调用方按成员重复读表的那一刻失败。 +`packages/subprocess/subprocess-local/tests/terminal.spec.ts` 通过注入的 `exec` 驱动真实的 `MacProcessInspector`，断言一次前台检查在 0、2、10 个子进程下都恰好执行一次 `-axo` 表读取。这个次数——而非墙钟时间——才是持久不变量：它在任何主机上都成立，并且在任何调用方按成员重复读表的那一刻失败。同一文件还钉住：没有成员的一轮信号不做任何捕获；同步主机退出期间捕获失败时，PTY root 仍会被杀掉。 + +`process-inspector.spec.ts` 直接钉住围栏：一个先被观察为存活、随后从表中消失的标识不会收到信号。`windows-inspector.spec.ts` 钉住只回答存活的快照不执行 Toolhelp32 枚举。 ## Alternatives considered @@ -50,7 +54,7 @@ Status: implemented **在 `MacProcessInspector` 内部用短 TTL 缓存 macOS 的表。** 这不需要改接口，但它让陈旧性不可见：调用方无法分辨一个存活答案来自此刻还是来自上一次轮询结束时，而基于陈旧行发出的信号正是 PID 复用围栏要防止的事情。隐式缓存也与仓库偏好显式默认与显式边界的立场冲突。 -**在 `snapshot()` 旁保留 `isAlive`。** 这样不必改动发信号的调用点，代价是同一个问题有两种问法，而其中只有一种在循环里是廉价的。这种不对称将不得不在每个调用点重新解释一遍。 +**用整轮共享的观察来给信号加围栏。** 这能去掉最后一处按成员的读取，也是本次最初实现的形态。评审否决了它：围栏的存在就是为了击败 PID 复用，而观察反过来击败了围栏——因为它把原始的「PID 与起始时间」配对一路带了下来。窗口确实很窄（一轮里各次 kill 相隔微秒级，而 macOS 的 PID 空间是 99999、Linux 是 4194304），但 `README` 是无条件地声明这条保证的，用削弱它来换取微秒级的拆卸时间是错误的取舍。因此同时保留 `snapshot().alive` 与 `isAlive` 并不是同一个问题的两种问法：前者问表当时显示了什么，后者问此刻什么为真，而只有后者可以决定一次信号。 **把 `exec` 改成异步，而不是减少读取次数。** 异步的 `execFile` 能让轮询不再阻塞事件循环，但每次轮询仍然 fork N+1 个进程；在繁忙的机器上这是把一次停顿换成了持续的 fork 压力。它在减少读取次数之上仍是值得做的后续项，而不是它的替代。 @@ -58,9 +62,9 @@ Status: implemented 一次就绪轮询的进程表代价现在与子进程数量无关。在 macOS 上，一次轮询执行一次完整表读取加一次小的 `tpgid` 读取，也就是上表中 0 子进程那一行的代价，对任意子进程数量都成立。 -Linux 上查询单个标识的存活，代价从读一个 `stat` 文件变成一次完整的 `/proc` 遍历。每个要查询多个标识的调用方都会把这次遍历摊薄，这正是 `signalProcess` 接收观察而非自行捕获的原因；将来若有调用方确实只需要一次孤立的存活查询，它付出的代价会比过去高。 +拆卸保持原有的按次代价：每个目标一次窄的存活读取，在 macOS 上即每个成员一次 `ps` fork。这项代价从来不是实测到的问题——一个终端只拆卸一次，而它的就绪路径最多轮询 600 次——所以本次修复刻意付出它，以保证围栏读的是当前状态。 -快照是一个时间点视图，该类型的文档也这样声明。跨 `await` 持有一份快照再据此发信号，会扩大围栏本来要收窄的 PID 复用窗口；`waitForMembers` 每轮重新捕获正是为此。 +快照是一个时间点视图，该类型的文档也这样声明。`waitForMembers` 每轮重新捕获是因为观察变化正是它的用途；任何信号都不会从一份已捕获的视图上做决定。 每个 `ProcessInspector` 实现与测试替身都采用新形态，包括 Windows 检查器和 `dsh-terminal-bash` 的会话替身。此前通过替换 `processTree`、`processSession` 或 `isAlive` 来编排扫描的测试替身，现在替换对应的按问题读取钩子，其编排行为与调用计数保持不变。 diff --git a/packages/subprocess/subprocess-local/src/process-inspector.ts b/packages/subprocess/subprocess-local/src/process-inspector.ts index 1c89e8b5f1..08cec3b4ab 100644 --- a/packages/subprocess/subprocess-local/src/process-inspector.ts +++ b/packages/subprocess/subprocess-local/src/process-inspector.ts @@ -20,15 +20,16 @@ interface FileStatus { * One observation of the platform process table, shared by every question a * single readiness poll or teardown pass asks. * - * The table is read once, at capture — a `/bin/ps` fork on macOS, a `/proc` - * walk on Linux, a Toolhelp32 enumeration on Windows. Answering {@link tree}, - * {@link session}, or {@link alive} never re-reads it, which is what keeps a - * poll's cost independent of how many descendants the running command spawned. - * Windows liveness additionally consults the live process handle, because wait - * state is not a table column there. + * The table is read at most once, on the first question that needs it — a + * `/bin/ps` fork on macOS, a `/proc` walk on Linux, a Toolhelp32 enumeration on + * Windows. Later questions never re-read it, which is what keeps a poll's cost + * independent of how many descendants the running command spawned. Windows + * liveness needs no table at all: wait state is a per-handle question there, so + * a snapshot asked only for liveness never enumerates. * - * A snapshot is a point-in-time view. Take a fresh one per poll or teardown - * pass; a stale one must never decide that a process is still worth signalling. + * A snapshot answers what the process table showed, which is what batch + * filtering wants and what signalling must not use: {@link ProcessInspector.isAlive} + * is the fence a signal takes, because it reads current state instead. */ export interface ProcessSnapshot { /** @@ -64,17 +65,34 @@ export interface ProcessInspector { isStdinWaiting(pgid: number, shellPid: number): boolean /** * Read the process table once and answer tree, session, and liveness from it. - * @returns A point-in-time process-table observation. + * @returns A process-table observation whose reads are shared. */ snapshot(): ProcessSnapshot + /** + * Return whether the exact identity is a non-quiescent process right now. + * + * Reads the narrowest per-identity source the platform offers rather than a + * whole table, so a signalling round can re-check every target without + * paying for a scan. Callers filtering many members at once want + * {@link ProcessSnapshot.alive} instead. + * + * @param identity - PID plus start identity to match. + * @returns Whether that exact identity — not merely that PID — is running. + */ + isAlive(identity: ProcessIdentity): boolean signalGroup(pgid: number, signal: SubprocessTerminalSignal): void /** * Signal one exact process identity, fenced against PID reuse. + * + * The fence reads current state immediately before the signal. An observation + * taken earlier in the same round cannot stand in for it: the observation + * preserves the original PID-to-start-time pairing, so a recycled PID would + * still match and take a signal meant for the process that exited. + * * @param identity - PID plus start identity to signal. * @param signal - termination signal to deliver. - * @param observed - observation the identity fence reads; pass one taken for this teardown pass. */ - signalProcess(identity: ProcessIdentity, signal: 'SIGTERM' | 'SIGKILL', observed: ProcessSnapshot): void + signalProcess(identity: ProcessIdentity, signal: 'SIGTERM' | 'SIGKILL'): void } /** Testable boundary around filesystem, process-table, and signal syscalls. */ @@ -333,13 +351,14 @@ abstract class PosixProcessInspector implements ProcessInspector { abstract foregroundPgid(shellPid: number): number | undefined abstract isStdinWaiting(pgid: number, shellPid: number): boolean abstract snapshot(): ProcessSnapshot + abstract isAlive(identity: ProcessIdentity): boolean signalGroup(pgid: number, signal: SubprocessTerminalSignal): void { this.internals.kill(-pgid, signal) } - signalProcess(identity: ProcessIdentity, signal: 'SIGTERM' | 'SIGKILL', observed: ProcessSnapshot): void { - if (observed.alive(identity)) this.internals.kill(identity.pid, signal) + signalProcess(identity: ProcessIdentity, signal: 'SIGTERM' | 'SIGKILL'): void { + if (this.isAlive(identity)) this.internals.kill(identity.pid, signal) } } @@ -438,6 +457,11 @@ class LinuxProcessInspector extends PosixProcessInspector { return false } + isAlive(identity: ProcessIdentity): boolean { + const stat = readLinuxStat(this.internals, identity.pid) + return stat?.started === identity.started && !quiescent(stat.state) + } + snapshot(): ProcessSnapshot { return new PosixProcessSnapshot(numericEntries(this.internals, '/proc').flatMap((pid) => { const stat = readLinuxStat(this.internals, pid) @@ -483,6 +507,11 @@ class MacProcessInspector extends PosixProcessInspector { return false } + isAlive(identity: ProcessIdentity): boolean { + return macProcessTable(this.internals) + .some(entry => entry.pid === identity.pid && entry.started === identity.started) + } + snapshot(): ProcessSnapshot { return new PosixProcessSnapshot(macProcessTable(this.internals)) } diff --git a/packages/subprocess/subprocess-local/src/terminal.ts b/packages/subprocess/subprocess-local/src/terminal.ts index eb6bf618c1..624de54984 100644 --- a/packages/subprocess/subprocess-local/src/terminal.ts +++ b/packages/subprocess/subprocess-local/src/terminal.ts @@ -139,7 +139,7 @@ export class LocalTerminalHandle implements SubprocessTerminalHandle { if (this.exited) return if (this.rootIdentity !== undefined) { try { - this.inspector.signalProcess(this.rootIdentity, 'SIGKILL', this.inspector.snapshot()) + this.inspector.signalProcess(this.rootIdentity, 'SIGKILL') } catch (_rootExitedDuringHostExit) { // Exact identity signalling contains both exit races and PID reuse. } @@ -175,6 +175,7 @@ export class LocalTerminalHandle implements SubprocessTerminalHandle { } private async waitForMembers(members: ProcessIdentity[]): Promise { + if (members.length === 0) return [] const until = Date.now() + this.graceMs let survivors = this.survivors(members, this.inspector.snapshot()) while (survivors.length > 0 && Date.now() < until) { @@ -185,10 +186,11 @@ export class LocalTerminalHandle implements SubprocessTerminalHandle { } private signalMembers(members: ProcessIdentity[], signal: 'SIGTERM' | 'SIGKILL'): void { - const observed = this.inspector.snapshot() for (const member of members) { try { - this.inspector.signalProcess(member, signal, observed) + // Each signal reads its own identity fence, inside this try: a failed + // read must cost one target, never the rest of a teardown round. + this.inspector.signalProcess(member, signal) } catch (_alreadyExitedDuringSignal) { // The exact process identity is rechecked; a same-tick exit is success. } @@ -264,9 +266,9 @@ export class LocalTerminalHandle implements SubprocessTerminalHandle { // (the same console-list agent), so the tiers verify the shell's absence // through the inspector instead of waiting on `done` alone. const shellGone = (): boolean => - this.exited || (this.rootIdentity !== undefined && !this.inspector.snapshot().alive(this.rootIdentity)) + this.exited || (this.rootIdentity !== undefined && !this.inspector.isAlive(this.rootIdentity)) if (!shellGone() && this.rootIdentity !== undefined) { - this.inspector.signalProcess(this.rootIdentity, 'SIGTERM', this.inspector.snapshot()) + this.inspector.signalProcess(this.rootIdentity, 'SIGTERM') await this.waitForWindowsShellExit() } if (!shellGone() && this.rootIdentity === undefined) { @@ -278,7 +280,7 @@ export class LocalTerminalHandle implements SubprocessTerminalHandle { await Promise.race([this.done.then(() => undefined), delay(this.graceMs)]) } if (!shellGone() && this.rootIdentity !== undefined) { - this.inspector.signalProcess(this.rootIdentity, 'SIGKILL', this.inspector.snapshot()) + this.inspector.signalProcess(this.rootIdentity, 'SIGKILL') await this.waitForWindowsShellExit() } if (!shellGone()) throw new Error(`terminal cleanup failed; surviving pid: ${this.pid}`) @@ -287,7 +289,7 @@ export class LocalTerminalHandle implements SubprocessTerminalHandle { private async waitForWindowsShellExit(): Promise { const until = Date.now() + this.graceMs while (!this.exited && Date.now() < until) { - if (this.rootIdentity !== undefined && !this.inspector.snapshot().alive(this.rootIdentity)) return + if (this.rootIdentity !== undefined && !this.inspector.isAlive(this.rootIdentity)) return await delay(Math.min(25, Math.max(1, until - Date.now()))) } } @@ -317,7 +319,7 @@ export class LocalTerminalHandle implements SubprocessTerminalHandle { if (this.exited) return /* v8 ignore next -- stopShellWindows() verified the shell is gone or threw; the identity re-check is a defensive fence for a future caller. */ - if (this.rootIdentity !== undefined && this.inspector.snapshot().alive(this.rootIdentity)) return + if (this.rootIdentity !== undefined && this.inspector.isAlive(this.rootIdentity)) return this.exited = true this.output.end() this.outcome.resolve({ exitCode: null, signal: null }) diff --git a/packages/subprocess/subprocess-local/src/windows-inspector.ts b/packages/subprocess/subprocess-local/src/windows-inspector.ts index 7b2ae5a23d..6820505c3d 100644 --- a/packages/subprocess/subprocess-local/src/windows-inspector.ts +++ b/packages/subprocess/subprocess-local/src/windows-inspector.ts @@ -97,18 +97,25 @@ export class WindowsProcessInspector implements ProcessInspector { return false } + isAlive(identity: ProcessIdentity): boolean { + const state = this.internals.processState(identity.pid) + return state?.active === true && state.started === identity.started + } + snapshot(): ProcessSnapshot { - const entries = this.internals.snapshot() + // Enumerated on the first question that reads the table. Liveness never + // does — wait state is a per-handle question here — so the Windows + // teardown poll, which asks only for liveness, pays no Toolhelp32 walk. + let entries: ProcessEntry[] | undefined return { - tree: rootPid => windowsProcessTree(entries, rootPid, pid => this.internals.processState(pid)?.started), + tree: rootPid => windowsProcessTree( + entries ??= this.internals.snapshot(), + rootPid, + pid => this.internals.processState(pid)?.started, + ), // Windows has no POSIX sessions; the shell pid stands in as a pseudo group. session: () => [], - alive: (identity) => { - // Wait state is a per-handle question, not a Toolhelp32 column, so - // liveness reads the live process object rather than `entries`. - const state = this.internals.processState(identity.pid) - return state?.active === true && state.started === identity.started - }, + alive: identity => this.isAlive(identity), } } @@ -116,8 +123,8 @@ export class WindowsProcessInspector implements ProcessInspector { this.internals.taskkill(pgid, signal === 'SIGKILL') } - signalProcess(identity: ProcessIdentity, signal: 'SIGTERM' | 'SIGKILL', observed: ProcessSnapshot): void { - if (observed.alive(identity)) this.internals.taskkill(identity.pid, signal === 'SIGKILL') + signalProcess(identity: ProcessIdentity, signal: 'SIGTERM' | 'SIGKILL'): void { + if (this.isAlive(identity)) this.internals.taskkill(identity.pid, signal === 'SIGKILL') } } /* jscpd:ignore-end */ diff --git a/packages/subprocess/subprocess-local/tests/local.spec.ts b/packages/subprocess/subprocess-local/tests/local.spec.ts index 589102ccf8..9e4925bdb2 100644 --- a/packages/subprocess/subprocess-local/tests/local.spec.ts +++ b/packages/subprocess/subprocess-local/tests/local.spec.ts @@ -303,6 +303,7 @@ describe('LocalSubprocessRuntime', () => { foregroundPgid: () => undefined, isStdinWaiting: () => false, snapshot: () => ({ tree: () => [], session: () => [], alive: () => false }), + isAlive: () => false, signalGroup: () => {}, signalProcess: () => {}, } @@ -372,6 +373,7 @@ describe('LocalSubprocessRuntime', () => { session: () => [], alive: identity => alive.has(identity.pid), }), + isAlive: identity => alive.has(identity.pid), signalGroup: () => {}, signalProcess: () => {}, } diff --git a/packages/subprocess/subprocess-local/tests/process-exit.spec.ts b/packages/subprocess/subprocess-local/tests/process-exit.spec.ts index c00c666e33..1fc952a869 100644 --- a/packages/subprocess/subprocess-local/tests/process-exit.spec.ts +++ b/packages/subprocess/subprocess-local/tests/process-exit.spec.ts @@ -68,10 +68,9 @@ function cleanupTree(state: TreeState | undefined, identities: ProcessIdentity[] return } const inspector = createProcessInspector() - const observed = inspector.snapshot() for (const identity of identities) { try { - inspector.signalProcess(identity, 'SIGKILL', observed) + inspector.signalProcess(identity, 'SIGKILL') } catch (_alreadyGone) { // Exact start identity prevents PID-reuse cleanup from reaching another process. } diff --git a/packages/subprocess/subprocess-local/tests/process-inspector.spec.ts b/packages/subprocess/subprocess-local/tests/process-inspector.spec.ts index ac5c95c13f..797076ebfc 100644 --- a/packages/subprocess/subprocess-local/tests/process-inspector.spec.ts +++ b/packages/subprocess/subprocess-local/tests/process-inspector.spec.ts @@ -138,14 +138,15 @@ describe('Linux process inspector', () => { expect(observed.alive({ pid: 10, started: '500' })).toBe(true) expect(observed.alive({ pid: 10, started: 'old' })).toBe(false) inspector.signalGroup(40, 'SIGINT') - inspector.signalProcess({ pid: 10, started: '500' }, 'SIGTERM', observed) - inspector.signalProcess({ pid: 10, started: 'old' }, 'SIGKILL', observed) + inspector.signalProcess({ pid: 10, started: '500' }, 'SIGTERM') + inspector.signalProcess({ pid: 10, started: 'old' }, 'SIGKILL') expect(fake.kills).toEqual([[-40, 'SIGINT'], [10, 'SIGTERM']]) fake.files.set('/proc/10/stat', stat(10, 20, 30, 40, '500', 1, 'Z')) - // A zombie is present in the table but never signallable; a fresh capture sees the new state. - const afterExit = inspector.snapshot() - expect(afterExit.alive({ pid: 10, started: '500' })).toBe(false) - inspector.signalProcess({ pid: 10, started: '500' }, 'SIGKILL', afterExit) + // A zombie is present in the table but never signallable; both the batch + // view and the signal fence report it quiescent once the state changes. + expect(inspector.snapshot().alive({ pid: 10, started: '500' })).toBe(false) + expect(inspector.isAlive({ pid: 10, started: '500' })).toBe(false) + inspector.signalProcess({ pid: 10, started: '500' }, 'SIGKILL') expect(fake.kills).toEqual([[-40, 'SIGINT'], [10, 'SIGTERM']]) }) @@ -298,8 +299,8 @@ describe('macOS process inspector', () => { expect(observed.session(10)).toEqual([]) expect(observed.alive({ pid: 11, started: 'Mon Jul 21 10:00:01 2026' })).toBe(true) inspector.signalGroup(55, 'SIGTSTP') - inspector.signalProcess({ pid: 11, started: 'Mon Jul 21 10:00:01 2026' }, 'SIGKILL', observed) - inspector.signalProcess({ pid: 12, started: 'missing' }, 'SIGTERM', observed) + inspector.signalProcess({ pid: 11, started: 'Mon Jul 21 10:00:01 2026' }, 'SIGKILL') + inspector.signalProcess({ pid: 12, started: 'missing' }, 'SIGTERM') expect(fake.kills).toEqual([[-55, 'SIGTSTP'], [11, 'SIGKILL']]) fake.setPs(' 10 11 Mon Jul 21 10:00:00 2026\n 11 10 Mon Jul 21 10:00:01 2026\n') @@ -309,6 +310,20 @@ describe('macOS process inspector', () => { ]) }) + it('re-reads the process table before signalling instead of trusting an earlier observation', () => { + const fake = fakeInternals() + fake.setPs(' 11 10 Mon Jul 21 10:00:01 2026\n') + const inspector = createProcessInspector('darwin', 'arm64', fake.internals) + inspector.snapshot() + // The member exits after that observation; a recycled pid would otherwise + // inherit the observed identity and take the signal meant for the original. + fake.setPs('') + + inspector.signalProcess({ pid: 11, started: 'Mon Jul 21 10:00:01 2026' }, 'SIGKILL') + + expect(fake.kills).toEqual([]) + }) + it('returns undefined for missing or invalid foreground groups and dispatches platform inspectors', () => { const fake = fakeInternals() fake.setTpgid('-1') diff --git a/packages/subprocess/subprocess-local/tests/terminal.spec.ts b/packages/subprocess/subprocess-local/tests/terminal.spec.ts index 75beef6a34..b8f23a89c4 100644 --- a/packages/subprocess/subprocess-local/tests/terminal.spec.ts +++ b/packages/subprocess/subprocess-local/tests/terminal.spec.ts @@ -76,23 +76,29 @@ class FakeInspector implements ProcessInspector { readTree: () => ProcessIdentity[] = () => this.root === undefined ? this.members : [this.root, ...this.members] readSession: () => ProcessIdentity[] = () => this.sessionMembers readAlive: (identity: ProcessIdentity) => boolean = identity => this.alive.has(identity.pid) + /** Liveness as of right now; tests diverge it from readAlive to stage an exit between scan and signal. */ + readCurrentAlive: (identity: ProcessIdentity) => boolean = identity => this.readAlive(identity) + /** Counts process-table captures so read-amplification cases can pin them. */ + captures = 0 snapshot(): ProcessSnapshot { + this.captures += 1 return { tree: () => this.readTree(), session: () => this.readSession(), alive: identity => this.readAlive(identity), } } + + isAlive(identity: ProcessIdentity) { return this.readCurrentAlive(identity) } signalGroup(pgid: number, signal: SubprocessTerminalSignal) { if (this.throwGroup) throw new Error('group failed') this.groups.push([pgid, signal]) } - signalProcess(identity: ProcessIdentity, signal: 'SIGTERM' | 'SIGKILL', observed: ProcessSnapshot) { + signalProcess(identity: ProcessIdentity, signal: 'SIGTERM' | 'SIGKILL') { // Mirrors the real inspectors' alive-gated signalling. - if (!this.alive.has(identity.pid)) return if (this.throwProcess) throw new Error('process raced') - if (!observed.alive(identity)) return + if (!this.isAlive(identity)) return this.processes.push([identity.pid, signal]) if (this.removeOnSignal) this.alive.delete(identity.pid) } @@ -116,8 +122,8 @@ describe('LocalTerminalHandle', () => { inspector.alive.add(pty.pid) inspector.alive.add(first.pid) const signalProcess = inspector.signalProcess.bind(inspector) - inspector.signalProcess = (identity, signal, observed) => { - signalProcess(identity, signal, observed) + inspector.signalProcess = (identity, signal) => { + signalProcess(identity, signal) if (identity.pid === pty.pid) { inspector.members = [first, late] inspector.alive.add(late.pid) @@ -527,6 +533,37 @@ describe('LocalTerminalHandle on Windows', () => { }) }) +describe('signalling freshness and containment', () => { + it('keeps synchronous host exit going when the process table cannot be captured', () => { + const pty = new FakePty() + const inspector = new FakeInspector() + inspector.alive.add(pty.pid) + const handle = new LocalTerminalHandle(pty.asPty(), inspector, 10) + inspector.snapshot = () => { throw new Error('process table unavailable') } + + expect(() => { handle.terminateForHostExit() }).not.toThrow() + + // forceStopShell still runs: a failed scan must not cost the PTY root. + expect(inspector.processes).toEqual([[pty.pid, 'SIGKILL']]) + }) + + it('captures no process table for a signalling round with no members', () => { + const pty = new FakePty() + const inspector = new FakeInspector() + inspector.alive.add(pty.pid) + const handle = new LocalTerminalHandle(pty.asPty(), inspector, 10) + // Only the shell exists, so every descendant scan yields an empty round. + inspector.readTree = () => [{ pid: pty.pid, started: 'shell' }] + inspector.captures = 0 + + handle.terminateForHostExit() + + // Two descendant scans and nothing else: no capture for either empty + // signalling round, and none for the identity-fenced shell kill. + expect(inspector.captures).toBe(2) + }) +}) + describe('process-table read amplification', () => { // The macOS inspector answers every question by forking `/bin/ps`, so a // readiness poll that asks per descendant scales its blocking cost with the diff --git a/packages/subprocess/subprocess-local/tests/windows-inspector.spec.ts b/packages/subprocess/subprocess-local/tests/windows-inspector.spec.ts index c2129923b4..de8df8d535 100644 --- a/packages/subprocess/subprocess-local/tests/windows-inspector.spec.ts +++ b/packages/subprocess/subprocess-local/tests/windows-inspector.spec.ts @@ -16,10 +16,12 @@ function fakeInternals() { const entries: ProcessEntry[] = [] const states = new Map() const kills: Array<[number, boolean]> = [] + const counts = { enumerations: 0, stateReads: 0 } return { + counts, internals: { - snapshot: () => [...entries], - processState: pid => states.get(pid), + snapshot: () => { counts.enumerations += 1; return [...entries] }, + processState: (pid) => { counts.stateReads += 1; return states.get(pid) }, taskkill: (pid: number, force: boolean) => { kills.push([pid, force]) }, } satisfies WindowsProcessInspectorInternals, add(entry: ProcessEntry, started?: string, active = true): void { @@ -30,6 +32,29 @@ function fakeInternals() { } } +describe('WindowsProcessInspector table enumeration', () => { + it('enumerates the process table only for questions that need it', () => { + const fake = fakeInternals() + fake.add({ pid: 10, parentPid: 0 }, 't10') + fake.add({ pid: 11, parentPid: 10 }, 't11') + const inspector = new WindowsProcessInspector(fake.internals) + + // Liveness is a per-handle question on Windows, so a snapshot asked only + // for liveness must not pay a Toolhelp32 walk. The terminal's Windows + // teardown polls exactly this way, every 25 ms. + const observed = inspector.snapshot() + expect(observed.alive({ pid: 11, started: 't11' })).toBe(true) + expect(fake.counts.enumerations).toBe(0) + + expect(observed.tree(10)).toHaveLength(2) + expect(fake.counts.enumerations).toBe(1) + + // A second tree question reuses the same observation. + observed.tree(10) + expect(fake.counts.enumerations).toBe(1) + }) +}) + describe('windowsProcessTree', () => { it('walks a table children-first with readable identities only', () => { const started = (pid: number): string | undefined => pid === 12 ? undefined : `t${pid}` @@ -78,12 +103,12 @@ describe('WindowsProcessInspector (injected internals)', () => { { pid: 11, started: 't11' }, { pid: 10, started: 't10' }, ]) - expect(inspector.snapshot().alive({ pid: 11, started: 't11' })).toBe(true) - expect(inspector.snapshot().alive({ pid: 11, started: 'stale' })).toBe(false) - expect(inspector.snapshot().alive({ pid: 99, started: 't99' })).toBe(false) + expect(inspector.isAlive({ pid: 11, started: 't11' })).toBe(true) + expect(inspector.isAlive({ pid: 11, started: 'stale' })).toBe(false) + expect(inspector.isAlive({ pid: 99, started: 't99' })).toBe(false) fake.add({ pid: 12, parentPid: 10 }, 't12', false) - expect(inspector.snapshot().alive({ pid: 12, started: 't12' })).toBe(false) + expect(inspector.isAlive({ pid: 12, started: 't12' })).toBe(false) }) it('maps SIGKILL to a forced taskkill and other signals to the grace form', () => { @@ -100,9 +125,9 @@ describe('WindowsProcessInspector (injected internals)', () => { fake.add({ pid: 10, parentPid: 0 }, 't10') fake.add({ pid: 11, parentPid: 10 }, 't11', false) const inspector = new WindowsProcessInspector(fake.internals) - inspector.signalProcess({ pid: 10, started: 't10' }, 'SIGKILL', inspector.snapshot()) - inspector.signalProcess({ pid: 11, started: 't11' }, 'SIGKILL', inspector.snapshot()) - inspector.signalProcess({ pid: 10, started: 'stale' }, 'SIGTERM', inspector.snapshot()) + inspector.signalProcess({ pid: 10, started: 't10' }, 'SIGKILL') + inspector.signalProcess({ pid: 11, started: 't11' }, 'SIGKILL') + inspector.signalProcess({ pid: 10, started: 'stale' }, 'SIGTERM') expect(fake.kills).toEqual([[10, true]]) }) @@ -139,12 +164,10 @@ win32('WindowsProcessInspector over the real koffi bindings', () => { it('reports unreadable identities for absent processes and no-ops tree signalling', () => { const inspector = createWindowsProcessInspector() - expect(inspector.snapshot().alive({ pid: 0x7FFFFFFF, started: 'absent' })).toBe(false) + expect(inspector.isAlive({ pid: 0x7FFFFFFF, started: 'absent' })).toBe(false) expect(() => { inspector.signalGroup(0x7FFFFFFF, 'SIGKILL') }).not.toThrow() expect(() => { inspector.signalGroup(0x7FFFFFFF, 'SIGTERM') }).not.toThrow() expect(() => { inspector.signalGroup(0, 'SIGKILL') }).not.toThrow() - expect(() => { - inspector.signalProcess({ pid: 0x7FFFFFFF, started: 'absent' }, 'SIGKILL', inspector.snapshot()) - }).not.toThrow() + expect(() => { inspector.signalProcess({ pid: 0x7FFFFFFF, started: 'absent' }, 'SIGKILL') }).not.toThrow() }) }) diff --git a/packages/terminal/terminal-bash/tests/session.spec.ts b/packages/terminal/terminal-bash/tests/session.spec.ts index 02ce95db02..b28c579375 100644 --- a/packages/terminal/terminal-bash/tests/session.spec.ts +++ b/packages/terminal/terminal-bash/tests/session.spec.ts @@ -34,6 +34,7 @@ class FakeInspector implements ProcessInspector { alive: (identity: ProcessIdentity) => this.alive.has(identity.pid), } } + isAlive(identity: ProcessIdentity) { return this.alive.has(identity.pid) } signalGroup(pgid: number, signal: TerminalSignal) { if (this.throwGroup) throw new Error('group failed') this.groups.push([pgid, signal])