Coder/deepseek-harness
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(subprocess): tree-scoped escalation and byte-exact tails (Codex round 1)

Browse Source 
(A1) terminate()/dispose()/service teardown keyed on direct-child settlement
could leak a TERM-trapping descendant that outlived the leader (Codex
reproduced it with a disowned trap-SIGTERM helper). kill()/terminate() now
gate on tree liveness instead of outcome settlement; the SIGKILL escalation
timer survives settle (unref'd, re-probing the tree); dispose's tier
quiescence is whole-tree exit via a bounded waitForExit; the service's live
set releases handles only when their tree is gone, and its teardown awaits
tree exit. Three new suites pin the survivor scenarios end to end
(terminate, dispose, service teardown).

(A2) the escalation branch is now real tested behavior — its ignore is gone;
the one remaining signalTree guard ignore states why it is unreachable
through the handle verbs.

(A3) docs contradictions fixed: the impl README's stale POSIX-only bullet
now states the contained best-effort Windows tree story; the lsp-local
README no longer claims taskkill failures stay visible (containment + the
tree-liveness wait is the actual contract); the architecture tables (en+zh)
list all three consumer families.

(B1) OutputCollector keeps a byte-exact tail across uneven chunk boundaries
(trim the head chunk instead of dropping it whole) — the LSP diagnostic-tail
contract; pinned by a cross-chunk test.

(B2) the subagent-acp coverage ignore is narrowed to exactly the
never-settling success arm.
This commit is contained in:
Tianyi Cui
2026-07-26 16:50:36 +08:00
parent c80fddf7a6
commit 79a28ad6d9
12 changed files with 195 additions and 91 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
packages/subprocess/subprocess-local/README.md
View File

@@ -20,7 +20,7 @@ No direct invalidation; the named consumers own any request-prefix changes.
## Known Limitations and Deferred Work
- **POSIX-only** — detached process groups, group kills, and SIGTERM→SIGKILL escalation are hardcoded; Windows is unsupported.
- **Windows tree support is best-effort and untested in CI** — termination routes through `taskkill /PID <pid> /T /F` with all outcomes contained (absent tree, races, missing binary), and liveness falls back to the direct-child boundary; the suites cover the routing through an injected runner only, and `packages/subprocess/*` is excluded from the Windows test matrix.
- **The credential scrub is a name heuristic** — `*KEY*`/`*SECRET*`/`*TOKEN*` only; differently-named secrets (e.g. `*PASSWORD*`) pass through, and a whitelist for over-scrubbed vars is noted future work.
- **Completed spill files are not deleted** — bounded full-output recovery files (and the private per-process spill dir) accumulate under the OS tmpdir until something external cleans them; oversize incomplete spills are discarded and deletion is attempted immediately, but a cleanup failure can leave a bounded file behind.

2
packages/subprocess/subprocess-local/package.json
View File

@@ -29,11 +29,13 @@
"peerDependencies": {
"@deepseek-ai/dsh-invariants": "^0.0.1",
"@deepseek-ai/dsh-subprocess": "^0.0.1",
"@deepseek-ai/dsh-timeout": "^0.0.1",
"cordis": "^4.0.0-rc.7"
},
"devDependencies": {
"@deepseek-ai/dsh-invariants": "workspace:^",
"@deepseek-ai/dsh-subprocess": "workspace:^",
"@deepseek-ai/dsh-timeout": "workspace:^",
"cordis": "^4.0.0-rc.7"
}
}

18
packages/subprocess/subprocess-local/src/index.ts
View File

@@ -28,13 +28,14 @@ export class LocalSubprocessService extends SubprocessService {
constructor(ctx: Context) {
super(ctx)
ctx.effect(() => async () => {
// Terminate (escalating), then await closure so even a TERM-trapping
// child cannot outlive the fiber.
// Terminate (escalating), then await WHOLE-TREE exit — not just the
// direct child's settlement — so even a TERM-trapping descendant cannot
// outlive the fiber.
const pending: Promise<unknown>[] = []
for (const handle of this.live) {
handle.terminate()
// Spawn-failure rejections already settled and left the live set.
pending.push(handle.done.catch(() => {}))
pending.push(handle.done.catch(() => {}).then(() => handle.waitForExit()))
}
this.live.clear()
await Promise.all(pending)
@@ -44,10 +45,13 @@ export class LocalSubprocessService extends SubprocessService {
spawn(spec: SubprocessSpawnSpec): SubprocessHandle {
const handle = spawnSubprocess(spec, this.internals)
this.live.add(handle)
handle.done.then(
() => { this.live.delete(handle) },
() => { this.live.delete(handle) },
)
// Release ownership only once the whole TREE is gone, not at direct-child
// settlement — a TERM-trapping helper that outlives the leader must stay
// owned so teardown can still escalate it. For the common no-survivor
// case waitForExit resolves immediately after settlement.
const release = (): Promise<void> =>
handle.waitForExit().then(() => { this.live.delete(handle) })
handle.done.then(release, release)
return handle
}
}

146
packages/subprocess/subprocess-local/src/spawn.ts
View File

@@ -14,7 +14,8 @@ import { randomBytes } from 'node:crypto'
import { closeSync, mkdtempSync, openSync, unlinkSync, writeSync } from 'node:fs'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { setImmediate as yieldToEventLoop } from 'node:timers/promises'
import { setTimeout as sleepMs } from 'node:timers/promises'
import { deadline } from '@deepseek-ai/dsh-timeout'
import { DSH_ENV_PREFIX, scrubbedParentEnv } from '@deepseek-ai/dsh-subprocess'
import type {
CollectedOutput,
@@ -62,6 +63,14 @@ export interface SpawnInternals {
platform?: NodeJS.Platform
}
/** Timeout code marking a dispose-ladder tier bound (vs an external abort). */
const DISPOSE_TIER_TIMEOUT = 'SUBPROCESS_DISPOSE_TIER'
/** Liveness-poll cadence for tree-exit waits; unref'd so an abandoned wait cannot hold the parent's loop open. */
function sleepTick(): Promise<void> {
return sleepMs(15, undefined, { ref: false })
}
let spillCounter = 0
let defaultSpillDir: string | undefined
@@ -118,19 +127,20 @@ export class OutputCollector {
if (!this.spillDisabled && (overflows || this.spillFd !== undefined)) this.spillAll(chunk)
this.chunks.push(chunk)
this.bytes += chunk.length
while (this.bytes > this.maxBytes && this.chunks.length > 1) {
// Drop whole chunks from the head; pipe chunks are small (≤64KiB), so
// the retained tail tracks the cap closely enough for a model-facing
// truncation boundary. (length > 1 was just checked — shift() returns.)
const head = this.chunks.shift() as Buffer
this.bytes -= head.length
this.dropped = true
}
if (this.bytes > this.maxBytes && this.chunks.length === 1) {
// A single chunk larger than the cap: keep its tail.
const only = this.chunks[0] as Buffer
this.chunks[0] = only.subarray(only.length - this.maxBytes)
this.bytes = this.maxBytes
while (this.bytes > this.maxBytes) {
const head = this.chunks[0] as Buffer
const excess = this.bytes - this.maxBytes
if (head.length <= excess) {
// Drop the whole head chunk (length ≥ 1 is guaranteed while over cap).
this.chunks.shift()
this.bytes -= head.length
} else {
// Trim the head so the retained window is byte-exact at the cap — a
// diagnostic tail (an LSP server's stderr) must hold the LAST
// maxBytes regardless of how the stream was chunked.
this.chunks[0] = head.subarray(excess)
this.bytes -= excess
}
this.dropped = true
}
}
@@ -281,6 +291,7 @@ function signalTree(
taskkill(pid)
return
}
/* v8 ignore next -- kill/terminate gate on treeAlive(), which is false for pid -1; this guard protects direct callers only. */
if (pid <= 0) return
try {
process.kill(-pid, sig)
@@ -352,21 +363,50 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
// Failed spawns use pid -1 so signalling remains a no-op.
const pid = child.pid ?? -1
/** Whether the detached tree's root (or POSIX group) is still alive. */
const treeAlive = (): boolean => {
if (pid <= 0) return false
if (platform === 'win32') {
// Windows has no group-liveness probe; the direct child's exit is the
// observable boundary (taskkill /T already took the tree with it).
return child.exitCode === null && child.signalCode === null
}
try {
process.kill(-pid, 0)
return true
} catch (error) {
const code = (error as NodeJS.ErrnoException).code
/* v8 ignore next 2 -- POSIX reports an absent group as ESRCH; child-reaping timing
makes observing the other arm platform-dependent. */
if (code === 'ESRCH') return false
/* v8 ignore start -- EPERM and non-POSIX negative-pid failures are platform defenses; CI runs
tree-lifecycle tests on POSIX hosts where absence reports ESRCH. */
if (code === 'EPERM') return true
return child.exitCode === null && child.signalCode === null
/* v8 ignore stop */
}
}
const kill = (sig: NodeJS.Signals = 'SIGTERM'): void => {
// After settlement the tree is gone and the pid may be reused; callers
// commonly kill() in a finally, so this must not re-signal.
if (settled) return
// Guard on TREE liveness, not outcome settlement: a TERM-trapping helper
// can outlive the settled direct child and must stay signalable, while a
// fully-dead tree (possible pid reuse) must not be re-signalled from a
// caller's finally block.
if (!treeAlive()) return
signalTree(platform, pid, sig, child, taskkill)
}
const terminate = (): void => {
if (graceTimer !== undefined) return // escalation already in flight
if (settled) return
if (!treeAlive()) return
signalTree(platform, pid, 'SIGTERM', child, taskkill)
// The escalation must survive direct-child settlement — the leader dying
// does not mean the tree died — so the timer is unref'd rather than
// cleared at settle, and re-checks tree liveness before force-killing.
graceTimer = setTimeout(() => {
/* v8 ignore next -- the timer is cleared at settlement; only an in-flight fire racing the close event sees settled=true. */
if (!settled) signalTree(platform, pid, 'SIGKILL', child, taskkill)
if (treeAlive()) signalTree(platform, pid, 'SIGKILL', child, taskkill)
}, spec.graceMs)
graceTimer.unref()
}
// The caller owns timeout classification; this layer only reacts to abort.
@@ -408,75 +448,51 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter
})
child.on('close', settle)
function cleanup(): void {
if (graceTimer !== undefined) clearTimeout(graceTimer)
// graceTimer deliberately NOT cleared: the SIGKILL escalation must be
// able to reach tree survivors after the direct child settles.
if (pipeDrainTimer !== undefined) clearTimeout(pipeDrainTimer)
spec.signal?.removeEventListener('abort', onAbort)
}
})
/** Whether the detached tree's root (or POSIX group) is still alive. */
const treeAlive = (): boolean => {
if (pid <= 0) return false
if (platform === 'win32') {
// Windows has no group-liveness probe; the direct child's exit is the
// observable boundary (taskkill /T already took the tree with it).
return child.exitCode === null && child.signalCode === null
}
try {
process.kill(-pid, 0)
return true
} catch (error) {
const code = (error as NodeJS.ErrnoException).code
/* v8 ignore next -- POSIX reports an absent group as ESRCH; child-reaping timing
makes observing the other arm platform-dependent. */
if (code === 'ESRCH') return false
/* v8 ignore start -- EPERM and non-POSIX negative-pid failures are platform defenses; CI runs
tree-lifecycle tests on POSIX hosts where absence reports ESRCH. */
if (code === 'EPERM') return true
return child.exitCode === null && child.signalCode === null
/* v8 ignore stop */
}
}
const waitForExit = async (signal?: AbortSignal): Promise<boolean> => {
while (treeAlive()) {
if (signal?.aborted) return false
await yieldToEventLoop()
await sleepTick()
}
return true
}
/** Race settlement against a timer without leaving listeners or live timers behind. */
const settlesWithin = async (ms: number): Promise<boolean> => {
if (settled) return true
// The executor runs synchronously, so the timer is assigned before the race.
let timer!: NodeJS.Timeout
const timeout = new Promise<false>((resolve) => {
// `.unref()` so a pending grace timer never keeps the parent's loop alive.
timer = setTimeout(() => { resolve(false) }, ms)
timer.unref()
})
try {
return await Promise.race([done.then(() => true, () => true), timeout])
} finally {
clearTimeout(timer)
}
/**
* Wait, bounded, for whole-tree exit — the dispose ladder's quiescence test.
* Tree liveness, not direct-child settlement: a TERM-trapping helper that
* outlives the leader must hold the ladder on its tier until it exits.
*/
const treeExitsWithin = async (ms: number): Promise<boolean> => {
using bound = deadline(undefined, ms, DISPOSE_TIER_TIMEOUT)
return await waitForExit(bound.signal)
}
let disposal: Promise<void> | undefined
const dispose = (graces: SubprocessDisposeGraces): Promise<void> => (disposal ??= (async () => {
// A spawn failure has no process to tear down; observe the rejection so
// disposal in a finally block cannot surface it as unhandled.
if (pid <= 0) {
await done.catch(() => {})
return
}
// 1. Close a piped stdin and allow cooperative teardown and flush.
if (stdinMode === 'pipe') child.stdin?.end()
if (await settlesWithin(graces.eofGraceMs)) return
if (await treeExitsWithin(graces.eofGraceMs)) return
// 2. POSIX gets a catchable graceful signal; Windows taskkill force-terminates.
if (platform !== 'win32') {
kill('SIGTERM')
if (await settlesWithin(graces.graceMs)) return
if (await treeExitsWithin(graces.graceMs)) return
}
// 3. Force-kill the tree and await a bounded exit edge.
kill('SIGKILL')
if (!(await settlesWithin(graces.graceMs))) {
throw new Error(`child process did not exit within ${graces.graceMs}ms after forced termination`)
if (!(await treeExitsWithin(graces.graceMs))) {
throw new Error(`child process tree did not exit within ${graces.graceMs}ms after forced termination`)
}
})())

90
packages/subprocess/subprocess-local/tests/spawn.spec.ts
View File

@@ -349,6 +349,19 @@ describe('OutputCollector', () => {
expect(readFileSync(out.spillPath!, 'utf8')).toBe('0123456789abcdef')
})
it('retains a byte-exact tail across uneven chunk boundaries', () => {
// The old whole-chunk drop could under-retain; a diagnostic tail must be
// exactly the LAST maxBytes regardless of chunking.
const collector = new OutputCollector(10, undefined, 'exact-tail', spillDir)
collector.push(Buffer.from('aaaa'))
collector.push(Buffer.from('bbbbbb'))
collector.push(Buffer.from('cc'))
const out = collector.finalize()
expect(out.text).toBe('aabbbbbbcc')
expect(Buffer.byteLength(out.text)).toBe(10)
expect(out.truncated).toBe(true)
})
it('readFrom returns increments and flags lossy reads', () => {
const collector = new OutputCollector(10, 100, 'test', spillDir)
collector.push(Buffer.from('aaaaa'))
@@ -439,16 +452,18 @@ describe('killGroup', () => {
expect(() => { killGroup(running.pid, 'SIGTERM') }).not.toThrow()
})
it('handle.kill() after settlement signals nothing and starts no grace timer', async () => {
// Cleanup code commonly kills handles in a finally; after settlement the
// group is gone and the pid may be reused, so a late kill must be inert
// (no signal to a possibly-recycled pgid, no referenced timer delaying exit).
it('handle.kill() after the tree died delivers no termination signal', async () => {
// Cleanup code commonly kills handles in a finally; once the tree is gone
// the pid may be reused, so a late kill must deliver nothing (the
// liveness PROBE — signal 0 — is the only process.kill allowed).
const running = spawnSubprocess(spec('true'))
await running.done
await running.waitForExit()
const spy = vi.spyOn(process, 'kill')
try {
running.kill()
expect(spy).not.toHaveBeenCalled()
const delivered = spy.mock.calls.filter(([, sig]) => sig !== 0)
expect(delivered).toEqual([])
} finally {
spy.mockRestore()
}
@@ -575,6 +590,59 @@ describe('waitForExit', () => {
})
})
describe('tree-survivor escalation (terminate/dispose reach helpers the leader left behind)', () => {
it('terminate() SIGKILLs a TERM-trapping descendant after the direct child settles', async () => {
// The leader spawns a TERM-trapping helper with all stdio detached from
// the collected pipes, then exits: the helper holds the GROUP alive while
// the direct child settles. The escalation must still reach it.
const pidFile = join(spillDir, `survivor-${Date.now()}.pid`)
const running = spawnSubprocess(spec(
`bash -c 'trap "" TERM; echo $$ > ${pidFile}; sleep 60' >/dev/null 2>&1 & disown; wait_placeholder=; exit 0`,
{ graceMs: 300 },
))
const helper = await waitForPidFile(pidFile)
await running.done // direct child settled; helper survives in the group
expect(() => process.kill(helper, 0)).not.toThrow()
running.terminate() // SIGTERM (trapped) → grace → SIGKILL the group
await expect(running.waitForExit()).resolves.toBe(true)
await waitGone(helper)
})
it('dispose() holds each tier on whole-tree exit, not direct-child settlement', async () => {
const pidFile = join(spillDir, `survivor-dispose-${Date.now()}.pid`)
const running = spawnSubprocess(spec(
`bash -c 'trap "" TERM; echo $$ > ${pidFile}; sleep 60' >/dev/null 2>&1 & disown; exit 0`,
{ graceMs: 200 },
))
const helper = await waitForPidFile(pidFile)
await running.done
expect(() => process.kill(helper, 0)).not.toThrow()
await running.dispose({ eofGraceMs: 100, graceMs: 300 })
// The ladder only returns once the WHOLE tree is gone.
expect(() => process.kill(helper, 0)).toThrow()
})
it('service teardown awaits tree survivors, not just handle settlement', async () => {
const { Context } = await import('cordis')
const { default: LocalSubprocessService } = await import('@deepseek-ai/dsh-subprocess-local')
const ctx = new Context()
const fiber = await ctx.plugin(LocalSubprocessService)
;(ctx.subprocess as InstanceType<typeof LocalSubprocessService>).internals = { spillDir }
const pidFile = join(spillDir, `survivor-svc-${Date.now()}.pid`)
const running = ctx.subprocess.spawn(spec(
`bash -c 'trap "" TERM; echo $$ > ${pidFile}; sleep 60' >/dev/null 2>&1 & disown; exit 0`,
{ graceMs: 200 },
))
const helper = await waitForPidFile(pidFile)
await running.done
await fiber.dispose()
// Teardown itself waited for the survivor to die.
expect(() => process.kill(helper, 0)).toThrow()
})
})
describe('coverage seams', () => {
it('taskkillProcessTree ignores non-positive pids and contains a missing binary', () => {
expect(() => { taskkillProcessTree(-1) }).not.toThrow()
@@ -604,13 +672,15 @@ describe('coverage seams', () => {
expect(running.collected.stderr!.readFrom(0).text).toBe('err\n')
})
it('terminate() after settlement is a no-op', async () => {
it('terminate() after the tree died delivers no termination signal', async () => {
const running = spawnSubprocess(spec('true'))
await running.done
await running.waitForExit()
const spy = vi.spyOn(process, 'kill')
try {
running.terminate()
expect(spy).not.toHaveBeenCalled()
const delivered = spy.mock.calls.filter(([, sig]) => sig !== 0)
expect(delivered).toEqual([])
} finally {
spy.mockRestore()
}
@@ -622,13 +692,15 @@ describe('coverage seams', () => {
await expect(running.waitForExit()).resolves.toBe(true)
})
it('dispose() on an already-settled handle returns without signalling', async () => {
it('dispose() on an already-exited tree returns without delivering a signal', async () => {
const running = spawnSubprocess(spec('true'))
await running.done
await running.waitForExit()
const spy = vi.spyOn(process, 'kill')
try {
await running.dispose({ eofGraceMs: 50, graceMs: 50 })
expect(spy).not.toHaveBeenCalled()
const delivered = spy.mock.calls.filter(([, sig]) => sig !== 0)
expect(delivered).toEqual([])
} finally {
spy.mockRestore()
}

3
packages/subprocess/subprocess-local/tsconfig.json
View File

@@ -17,6 +17,9 @@
{
"path": "../subprocess"
},
{
"path": "../../util/timeout"
},
{
"path": "../../support/invariants"
}