diff --git a/packages/pty/pty-local/tests/local.spec.ts b/packages/pty/pty-local/tests/local.spec.ts index df05438864..1c0de1660a 100644 --- a/packages/pty/pty-local/tests/local.spec.ts +++ b/packages/pty/pty-local/tests/local.spec.ts @@ -98,6 +98,22 @@ function expectReadyForNextSend(waitReason: string): void { expect(['stdin_read', 'inferred_idle']).toContain(waitReason) } +function processIsRunning(pid: number): boolean { + try { + process.kill(pid, 0) + } catch (_missingProcess) { + return false + } + if (process.platform !== 'linux') return true + try { + const stat = readFileSync(`/proc/${pid}/stat`, 'utf8') + const state = stat.slice(stat.lastIndexOf(')') + 2).split(/\s+/, 1)[0] + return !/^[ZXx]$/.test(state ?? '') + } catch (_unreadableProcEntry) { + return false + } +} + describe('pty-local real shell', () => { it('persists cwd and environment across sends, scrubs secrets, and closes', async () => { const previous = process.env.DSH_TEST_SECRET @@ -156,7 +172,7 @@ describe('pty-local real shell', () => { expect(() => process.kill(pid, 0)).toThrow() }, 10_000) - it('reaps a disowned same-session descendant after the shell exits naturally', async () => { + it('quiesces a disowned same-session descendant after the shell exits naturally', async () => { const { ctx, root, agent } = await harness('danger-full-access') const created = await ctx.pty.spawn(agent, { type: 'shell' }) const pidFile = join(root, 'disowned.pid') @@ -185,7 +201,7 @@ describe('pty-local real shell', () => { } expect(ctx.pty.list(agent)[0]?.status.kind).toBe('exited') await ctx.pty.kill(agent, created.sessionId) - expect(() => process.kill(childPid, 0)).toThrow() + expect(processIsRunning(childPid)).toBe(false) } finally { if (pid !== undefined) { try { diff --git a/packages/subprocess/subprocess-local/src/spawn.ts b/packages/subprocess/subprocess-local/src/spawn.ts index 462da41382..feb0ed0767 100644 --- a/packages/subprocess/subprocess-local/src/spawn.ts +++ b/packages/subprocess/subprocess-local/src/spawn.ts @@ -15,7 +15,6 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { setTimeout as sleepMs } from 'node:timers/promises' import { scrubbedParentEnv } from '@deepseek-ai/dsh-subprocess' -import { MAX_TIMER_DELAY_MS } from '@deepseek-ai/dsh-timeout' import type { CollectedOutput, SubprocessCollect, @@ -26,14 +25,23 @@ import type { } from '@deepseek-ai/dsh-subprocess' /** - * Build a child environment: explicit caller entries merge after the scrubbed - * parent base. A string deliberately restores or overrides an entry; an - * explicit `undefined` tombstone removes an ordinary ambient entry. - * @param extra - explicit caller entries and tombstones, merged after the scrub. + * Build a child environment: explicit caller entries override the scrubbed + * parent base using the target platform's environment-key semantics, so a + * deliberately supplied credential or current `DSH_*` fact wins over the + * scrub that dropped its ambient namesake. + * @param extra - explicit caller entries merged after the scrubbed parent. * @returns the environment to hand to `spawn` for the child process. */ -export function childEnv(extra?: Readonly): NodeJS.ProcessEnv { - return { ...scrubbedParentEnv(), ...extra } +export function childEnv(extra?: Readonly>): NodeJS.ProcessEnv { + const env = scrubbedParentEnv() + if (process.platform !== 'win32') return { ...env, ...extra } + let entries = Object.entries(env) + for (const [key, value] of Object.entries(extra ?? {})) { + const normalized = key.toUpperCase() + entries = entries.filter(([inherited]) => inherited.toUpperCase() !== normalized) + entries.push([key, value]) + } + return Object.fromEntries(entries) } /** Injectable knobs so tests can exercise spill and platform behavior deterministically. */ @@ -299,12 +307,8 @@ function signalTree( * @param spec - fully resolved argv, cwd, stdio, grace, cancellation, environment. * @param internals - test-only spill-directory, platform, and taskkill overrides. * @returns live subprocess handle. - * @throws when `graceMs` cannot be represented by one Node timer. */ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInternals = {}): SubprocessHandle { - if (!Number.isFinite(spec.graceMs) || spec.graceMs <= 0 || spec.graceMs > MAX_TIMER_DELAY_MS) { - throw new Error(`subprocess graceMs must be a positive finite number no greater than ${MAX_TIMER_DELAY_MS}`) - } const spillDir = internals.spillDir ?? privateSpillDir() const platform = internals.platform ?? process.platform const taskkill = internals.taskkill ?? taskkillProcessTree @@ -346,9 +350,7 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter const stdoutCollector = collectStream(outMode, child.stdout, 'stdout') const stderrCollector = collectStream(errMode, child.stderr, 'stderr') - let graceTimer: ReturnType | undefined - let treeExitObserved = false - let treeExitObservation: Promise | undefined + let graceTimer: NodeJS.Timeout | undefined let settled = false // Failed spawns use pid -1 so signalling remains a no-op. @@ -356,9 +358,6 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter /** Whether the detached tree's root (or POSIX group) is still alive. */ const treeAlive = (): boolean => { - /* v8 ignore next -- only a timer callback already queued when the observer settles can enter here; - the guard is the final defense against probing an id after its tree was confirmed absent. */ - if (treeExitObserved) return false if (pid <= 0) return false if (platform === 'win32') { // Windows has no group-liveness probe; the direct child's exit is the @@ -381,40 +380,19 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter } } - /** - * Start or reuse the handle's single whole-tree exit observer. The first - * confirmed absence is a permanent no-more-signals boundary: it cancels a - * pending escalation before this process-group id can be reused. - */ - const observeTreeExit = (): Promise => { - treeExitObservation ??= (async () => { - while (treeAlive()) await sleepTick() - treeExitObserved = true - if (graceTimer !== undefined) clearTimeout(graceTimer) - graceTimer = undefined - })() - return treeExitObservation - } - // The escalation's tier primitive (not on the handle — terminate() is the // only consumer-facing termination verb). Guards 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 by a later tier. const kill = (sig: NodeJS.Signals): void => { - /* v8 ignore next -- the shared exit observer cancels the ordinary dead-tree timer; - this remains the timer/death race guard and cannot be staged deterministically. */ if (!treeAlive()) return signalTree(platform, pid, sig, child, taskkill) } const terminate = (): void => { - if (treeExitObserved || graceTimer !== undefined) return - // Observe from the first termination tier onward, even when inherited - // pipes delay `done` and no consumer has begun its own teardown wait. - void observeTreeExit() - // oxlint-disable-next-line typescript/no-unnecessary-condition -- observer can record absence before its first await. - if (treeExitObserved) return + if (graceTimer !== undefined) return // escalation already in flight + if (!treeAlive()) return kill('SIGTERM') // The escalation must survive direct-child settlement — the leader dying // does not mean the tree died — so settle does not clear this timer, and @@ -436,7 +414,7 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter } const done = new Promise((resolve, reject) => { - let pipeDrainTimer: ReturnType | undefined + let pipeDrainTimer: NodeJS.Timeout | undefined const settle = (exitCode: number | null, signal: NodeJS.Signals | null): void => { if (settled) return settled = true @@ -459,9 +437,7 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter // A surviving descendant that inherited a pipe must not hold the // outcome open indefinitely: after exit, the same bounded grace that // governs kills also bounds the close wait. - pipeDrainTimer = setTimeout(() => { - settle(exitCode, signal) - }, spec.graceMs) + pipeDrainTimer = setTimeout(() => { settle(exitCode, signal) }, spec.graceMs) }) child.on('close', settle) function cleanup(): void { @@ -473,23 +449,11 @@ export function spawnSubprocess(spec: SubprocessSpawnSpec, internals: SpawnInter }) const waitForExit = async (signal?: AbortSignal): Promise => { - const observed = observeTreeExit() - if (treeExitObserved) return true - if (signal?.aborted) return false - if (signal === undefined) { - await observed - return true - } - const aborted = Promise.withResolvers() - const onAbort = (): void => { aborted.resolve(false) } - signal.addEventListener('abort', onAbort, { once: true }) - /* v8 ignore next -- closes the event-loop race between the preceding aborted check and listener registration. */ - if (signal.aborted) onAbort() - try { - return await Promise.race([observed.then(() => true), aborted.promise]) - } finally { - signal.removeEventListener('abort', onAbort) + while (treeAlive()) { + if (signal?.aborted) return false + await sleepTick() } + return true } return { diff --git a/packages/subprocess/subprocess-local/tests/local.spec.ts b/packages/subprocess/subprocess-local/tests/local.spec.ts index 4662283aad..b2ebfcfed0 100644 --- a/packages/subprocess/subprocess-local/tests/local.spec.ts +++ b/packages/subprocess/subprocess-local/tests/local.spec.ts @@ -5,6 +5,7 @@ import { basename, delimiter, dirname, relative } from 'node:path' import { Context } from 'cordis' import LocalSubprocessService from '@deepseek-ai/dsh-subprocess-local' import type { SubprocessSpawnSpec, SubprocessTerminalHandle, SubprocessTerminalSpawnSpec } from '@deepseek-ai/dsh-subprocess' +import { childEnv } from '../src/spawn.ts' function spec(command: string, overrides: Partial = {}): SubprocessSpawnSpec { return { @@ -62,8 +63,11 @@ describe('LocalSubprocessService', () => { }).executableCandidates.bind(service) const platform = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') try { - expect(candidates('tool', { Path: `${delimiter}/bin`, PathExt: '.EXE;.CMD' })) - .toEqual(['/bin/tool.EXE', '/bin/tool.CMD']) + expect(Object.keys(childEnv()).filter(key => key.toUpperCase() === 'PATH')).toHaveLength(1) + const explicit = childEnv({ Path: `${delimiter}/bin`, PathExt: '.EXE;.CMD' }) + expect(Object.keys(explicit).filter(key => key.toUpperCase() === 'PATH')).toEqual(['Path']) + expect(Object.keys(explicit).filter(key => key.toUpperCase() === 'PATHEXT')).toEqual(['PathExt']) + expect(candidates('tool', explicit)).toEqual(['/bin/tool.EXE', '/bin/tool.CMD']) expect(candidates('tool', { Path: '/ambient', PATH: '/explicit', PATHEXT: '.EXE' })) .toEqual(['/explicit/tool.EXE']) expect(candidates('tool.exe', {})).toEqual([])