Files

T

Tianyi Cui f81fcccd93 refactor(subprocess): keep terminate() as the seam's only termination verb

Delete kill(signal?) from SubprocessHandle: consumers stop a process only
through terminate()'s tree-scoped SIGTERM→graceMs→SIGKILL escalation
(idempotent, also driven by the spec's abort signal, a no-op once the tree
is gone). The single-signal verb had exactly one consumer family —
lsp-local — and what it bought there was a private re-implementation of
the same escalation. The internal kill closure stays in spawn.ts as the
dispose ladder's tier primitive; terminate() now routes through it too.

lsp-local collapses onto the seam's escalation:
- LspConnection replaces its terminate()/kill() pair with one terminate()
  that delegates to handle.terminate(). Behavior change: the
  framing-failure path terminates instead of instant SIGKILL, so a
  misbehaving server now gets SIGTERM plus the killGraceMs window to
  flush before SIGKILL.
- ConnectionSpec.pipeDrainGraceMs becomes killGraceMs: one grace, the
  spawn spec's graceMs, drives both the escalation window and post-exit
  pipe draining (the provider already passed killGraceMs for it).
- LspInstance.forceTerminate() drops its hand-rolled bounded first wait
  (LSP_KILL_GRACE) and escalateProcessTree (deleted with its export and
  unit test): the seam's escalation already commits to SIGKILL after
  killGraceMs, so only the unbounded quiescence awaits stay load-bearing.

Tests: kill()-shaped spawn specs become terminate()-shaped or fold into
the terminate() suites (group-wide delivery; the settled no-op case was
already pinned by 'terminate() after the tree died'); tree-survivor
coverage is intact. A stderr-'inherit' disposition test completes the
stdout/stderr symmetry so the scoped subprocess+lsp coverage gate stands
alone instead of leaning on subagent-acp's cross-package runs.

Docs: SubprocessHandle type-equiv block, seam/impl/group READMEs, and the
consumer-migration Agent Note lose the kill(signal?) vocabulary (zh pairs
re-recorded); cordis api/services catalogs regenerated.

2026-07-27 04:41:04 +08:00

6.4 KiB

Raw Blame History

Agent Note: The subprocess seam goes Node-shaped and every eligible spawner rides it

Status: implemented

English | 中文

Problem

The subprocess seam shipped shaped for exactly one consumer family: batch-collected stdout/stderr, batch stdin, a single escalating kill(). That was deliberate scope control, and its own note records "migrate the other spawn sites" as rejected-for-now. Review on the introducing PR reversed that deferral: the stacked follow-up should reshape the interface toward Node's API and move the remaining process-running places onto the service. The remaining spawners each carried a private copy of some slice of the same mechanics — lsp-local had its own detached-tree signalling (POSIX group + Windows taskkill + liveness polling), subagent-subprocess had the dispose ladder and its own scrub, mcp-client and pty-local and the SDK helper each had a third/fourth/fifth copy of the credential scrub — and none of it was swappable or centrally testable.

Decision

The seam's vocabulary is now Node-shaped, and every spawner that can ride the service does:

Per-stream stdio dispositions on SubprocessSpawnSpec: 'pipe' (the raw Readable/Writable, for consumer-owned protocol framing), 'inherit' (diagnostics to the parent's stream), and collect mode { maxBytes, spill? } — the original bounded tail-keep shape, with the spill file now optional so a diagnostic tail (a language server's stderr) buffers without touching disk. stdin is 'ignore', 'pipe', or { data } (write-and-close batch).
SubprocessOutcome carries exit facts only (Node's close-event vocabulary); collected output stays readable through handle.collected after settlement (spill fds seal at the settle boundary), so batch and streaming callers share one access path and nothing is copied into the outcome.
Tree-scoped termination behind one verb: terminate() owns the SIGTERM→grace→SIGKILL escalation (serves the spec's abort signal too, and is a no-op once the tree is gone) — the handle exposes no single-signal kill(signal?), so a consumer cannot skip the grace window; waitForExit() polls tree liveness (POSIX group probe; direct-child boundary on Windows); dispose(graces) is the cooperative stdin-EOF→SIGTERM→SIGKILL ladder absorbed from subagent-subprocess, memoized per handle. Windows tree termination (taskkill /T, injectable) moved in from lsp-local, so tree semantics are platform-correct for every consumer.
One scrub definition: scrubbedParentEnv()/SENSITIVE_ENV_PATTERN live on the seam. Spawners that cannot route the spawn itself through the service — pty-local (node-pty owns the fork) and mcp-client (the MCP SDK owns the transport spawn) — import the function, so environment policy is single-sourced even where process ownership is not; the SDK helper's scrubEnvironment() defaults through it as well.

Migrations landed with the reshape: bash-local/bash-sandbox (collect modes + batch stdin; the bash kill() maps to terminate() so task_kill keeps escalation semantics), lsp-local (piped protocol streams + a no-spill collected stderr tail; LspConnection takes the seam's spawn function; its private tree-op helpers deleted), subagent-acp (piped ndjson streams + inherited stderr; spawn failure surfaces through done rejection into the same startup race; disposal is handle.dispose with the plugin's configured graces). dsh-subagent-subprocess is deleted — the dispose ladder and scrub are the seam's; the unused isolated-config-dir helper died with it (no consumer existed).

Compositions mounting lsp-local or subagent-acp now load dsh-subprocess-local (the plugins inject 'subprocess'); the acp/lsp test fixtures gained the row.

Alternatives considered

Keep the batch-only seam and let stream consumers stay bespoke. The introducing note's position, rejected by review: it leaves three private copies of tree signalling and five of the scrub, and any future runner (containerized executor, remote process host) would have to pick which private copy to fork. The Node-shaped dispositions cover all three observed stream shapes without widening the outcome type or buffering piped streams.

A single stdio: 'pipe' | 'inherit' | 'collect' mode for all three streams at once. Rejected: real consumers mix modes per stream (lsp: pipe/pipe/collect; acp: pipe/pipe/inherit; bash: data/collect/collect). Per-stream dispositions are exactly Node's shape and avoid a second spawn call for the mixed cases.

Migrate pty-local and mcp-client spawns too. Rejected on ownership grounds, not scope: node-pty's fork() allocates the terminal itself, and the MCP SDK's StdioClientTransport spawns internally — neither call site is ours to route. They adopt the shared scrub (the part that is policy), and their READMEs say why the spawn stays put.

Migrate the test-support launchers (acp-snapshot, loader-smoke) and the SDK package-manager runner. Rejected: the support packages are deliberately dependency-light test infrastructure that must not depend on product seams, and the SDK wizard's stdio: 'inherit'-with-redirect semantics plus its out-of-composition lifecycle (no cordis context at all) make the service a poor fit; it shares the scrub instead.

Consequences

Bought: one implementation of tree signalling, escalation, the dispose ladder, bounded collection, and the scrub, tested once in dsh-subprocess-local's suites (including injected-platform Windows coverage that lsp-local's private copy never had); lsp-local and subagent-acp shed their process plumbing and their children now survive plugin reloads and die with composition teardown like bash's; a whole package (dsh-subagent-subprocess) is gone. The seam README's "one consumer family" limitation is retired.

Cost: the seam is wider — three stdio modes and the terminate/waitForExit/dispose lifecycle surface instead of one mode and one verb — so a future backend implements more surface; the compositions for lsp-local/subagent-acp each carry the subprocess row now; and SubprocessOutcome no longer carries output, a breaking shape change inside the still-unreleased stack (the PR2 layer was updated in place rather than shimmed, per the pre-release stance). pty-local/mcp-client/SDK/test-support spawns remain outside the service by ownership, with the scrub as the shared floor.

6.4 KiB Raw Blame History