fix(acp-snapshot): await delayed Windows exit markers
A successful Windows termination request can end the process before Node publishes exitCode or signalCode. If a child error wins the shutdown race, give that accepted exit a bounded observation window before escalating or reporting fallback refusal. Cover a delayed real exit edge, preserve prompt refusal behavior for a genuinely live child, and document the launcher grace without weakening the complete stdio and parser drain boundary.
This commit is contained in:
@@ -4,7 +4,7 @@ The ACP snapshot suite kit: the shared machinery behind the keyless snapshot tie
|
|||||||
|
|
||||||
Four layers, importable separately:
|
Four layers, importable separately:
|
||||||
|
|
||||||
- **`launchAcpTestAgent` (launcher)** — boots a source agent under tsx or a built `lib` agent under plain Node from a temp cwd, connects the SDK client over a raw-byte stdout tee, collects session updates and stderr, surfaces asynchronous spawn failures through startup, fails closed on unhandled permission requests, and owns graceful or signalled shutdown. Shutdown waits for process exit, inherited stdio closure, and ACP parser exhaustion before resolving or propagating a child error, so captures are complete and callers can remove owned paths after either outcome. Snapshot and ordinary e2e suites share this process boundary; a test supplies only agent paths, cwd, environment overrides, and any permission policy.
|
- **`launchAcpTestAgent` (launcher)** — boots a source agent under tsx or a built `lib` agent under plain Node from a temp cwd, connects the SDK client over a raw-byte stdout tee, collects session updates and stderr, surfaces asynchronous spawn failures through startup, fails closed on unhandled permission requests, and owns graceful or signalled shutdown. Shutdown waits for process exit, inherited stdio closure, and ACP parser exhaustion before resolving or propagating a child error, so captures are complete and callers can remove owned paths after either outcome. When Windows accepts forced termination but publishes its exit marker asynchronously, shutdown gives that marker a bounded grace before treating fallback refusal as a second failure. Snapshot and ordinary e2e suites share this process boundary; a test supplies only agent paths, cwd, environment overrides, and any permission policy.
|
||||||
- **`runScenario` (harness)** — drives ACP JSON-RPC stdio from a deterministic `input.json` script through the launcher, tees raw stdout for the golden and purity check, and harvests every persisted session JSONL (parent and subagent children, primary-first) after graceful stdin EOF. `AgentUnderTest` supplies absolute `binScript`, optional `libBinScript`, `configPath`, and `tsconfigPath` paths because the subprocess cwd is outside the repo. Startup failures preserve captured agent stderr in the rejected diagnostic.
|
- **`runScenario` (harness)** — drives ACP JSON-RPC stdio from a deterministic `input.json` script through the launcher, tees raw stdout for the golden and purity check, and harvests every persisted session JSONL (parent and subagent children, primary-first) after graceful stdin EOF. `AgentUnderTest` supplies absolute `binScript`, optional `libBinScript`, `configPath`, and `tsconfigPath` paths because the subprocess cwd is outside the repo. Startup failures preserve captured agent stderr in the rejected diagnostic.
|
||||||
- **Normalizers** — pure functions turning the two captured surfaces into stable text: `normalizeStdout` (JSON-RPC ids → first-seen sequence; UUIDs/cwd → tokens; cwd-rooted separators selected as canonical `/` or host-native; doubles as the stdout-purity check), `normalizeSessionLog` (times zeroed, `seq` kept, the same cwd-path policy), `scrubSystemPrompts` (prompt text → `{{system}}`), `scrubToolSchemas` (schema bulk → `{{tools}}`), and `scrubRequestHeaders` (all header bulk → `{{system}}`/`{{tools}}`/`{{messagePrefix}}` outside each pin, structure kept — [pinned-header RFC](../../../docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md)).
|
- **Normalizers** — pure functions turning the two captured surfaces into stable text: `normalizeStdout` (JSON-RPC ids → first-seen sequence; UUIDs/cwd → tokens; cwd-rooted separators selected as canonical `/` or host-native; doubles as the stdout-purity check), `normalizeSessionLog` (times zeroed, `seq` kept, the same cwd-path policy), `scrubSystemPrompts` (prompt text → `{{system}}`), `scrubToolSchemas` (schema bulk → `{{tools}}`), and `scrubRequestHeaders` (all header bulk → `{{system}}`/`{{tools}}`/`{{messagePrefix}}` outside each pin, structure kept — [pinned-header RFC](../../../docs/rfc/implemented/testing/2026-07-06-pin-request-header-content-in-one-scenario.md)).
|
||||||
- **`defineAcpSnapshotSuite` (factory)** — registers the whole describe/it tree for a scenario table: per-scenario shared golden and re-persisted-log compares, optional Windows-native stdout sidecars, record/refresh fixture write-back, rejection of structured `UNKNOWN_TOOL` results, the per-header-class pin (`system-prompt.golden.md` plus `tool-schemas.golden.json`) with its live uniformity guard, and the fixture guard block (no orphan scenario dirs, required files present, exactly one pin per class, every JSONL prompt/schema-scrubbed, non-pinning fixtures fully header-scrubbed). Each scenario directory's `session.jsonl` plus contiguous `session.<n>.jsonl` siblings are the ordered primary/child inventory; the scenario table does not duplicate their count. Must be called at vitest collection time.
|
- **`defineAcpSnapshotSuite` (factory)** — registers the whole describe/it tree for a scenario table: per-scenario shared golden and re-persisted-log compares, optional Windows-native stdout sidecars, record/refresh fixture write-back, rejection of structured `UNKNOWN_TOOL` results, the per-header-class pin (`system-prompt.golden.md` plus `tool-schemas.golden.json`) with its live uniformity guard, and the fixture guard block (no orphan scenario dirs, required files present, exactly one pin per class, every JSONL prompt/schema-scrubbed, non-pinning fixtures fully header-scrubbed). Each scenario directory's `session.jsonl` plus contiguous `session.<n>.jsonl` siblings are the ordered primary/child inventory; the scenario table does not duplicate their count. Must be called at vitest collection time.
|
||||||
|
|||||||
@@ -21,6 +21,8 @@ import {
|
|||||||
} from '@agentclientprotocol/sdk'
|
} from '@agentclientprotocol/sdk'
|
||||||
import { resolveExampleLaunch } from '@deepseek-ai/dsh-loader-smoke'
|
import { resolveExampleLaunch } from '@deepseek-ai/dsh-loader-smoke'
|
||||||
|
|
||||||
|
const EXIT_MARKER_GRACE_MS = 250
|
||||||
|
|
||||||
/** The source/built agent entry, leaf config, and workspace tsconfig an ACP test boots. */
|
/** The source/built agent entry, leaf config, and workspace tsconfig an ACP test boots. */
|
||||||
export interface AgentUnderTest {
|
export interface AgentUnderTest {
|
||||||
/** The agent source bin entry (for example `packages/examples/acp-demo/src/bin.ts`). */
|
/** The agent source bin entry (for example `packages/examples/acp-demo/src/bin.ts`). */
|
||||||
@@ -238,7 +240,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe
|
|||||||
}
|
}
|
||||||
// Windows implements the supported signal names as forced termination. The exit markers
|
// Windows implements the supported signal names as forced termination. The exit markers
|
||||||
// may therefore arrive after the error wins the race above but before fallback begins.
|
// may therefore arrive after the error wins the race above but before fallback begins.
|
||||||
if (!isRunning(child)) return propagateFailureAfterDrain()
|
if (!isRunning(child) || await exitMarkerWithinGrace(exited)) return propagateFailureAfterDrain()
|
||||||
|
|
||||||
// An `error` after spawn is not an exit edge: in particular, a failed
|
// An `error` after spawn is not an exit edge: in particular, a failed
|
||||||
// signal can leave the subprocess live. Force termination, await the
|
// signal can leave the subprocess live. Force termination, await the
|
||||||
@@ -252,7 +254,7 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe
|
|||||||
// A successful earlier signal may win between the live check and this fallback call.
|
// A successful earlier signal may win between the live check and this fallback call.
|
||||||
// In that case `kill()` correctly reports no process to signal; the original child error
|
// In that case `kill()` correctly reports no process to signal; the original child error
|
||||||
// remains the shutdown result once inherited stdio and callbacks have drained.
|
// remains the shutdown result once inherited stdio and callbacks have drained.
|
||||||
if (!isRunning(child)) return propagateFailureAfterDrain()
|
if (!isRunning(child) || await exitMarkerWithinGrace(exited)) return propagateFailureAfterDrain()
|
||||||
closeUpdateStream()
|
closeUpdateStream()
|
||||||
throw new AggregateError(
|
throw new AggregateError(
|
||||||
[failure, new Error('Fallback SIGKILL was not accepted by the child process')],
|
[failure, new Error('Fallback SIGKILL was not accepted by the child process')],
|
||||||
@@ -281,6 +283,17 @@ function waitForExit(child: ChildProcessWithoutNullStreams): Promise<void> {
|
|||||||
return new Promise<void>(resolve => child.once('exit', () => { resolve() }))
|
return new Promise<void>(resolve => child.once('exit', () => { resolve() }))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Give an accepted Windows termination request a bounded window to publish its exit marker. */
|
||||||
|
function exitMarkerWithinGrace(exited: Promise<void>): Promise<boolean> {
|
||||||
|
return Promise.race([
|
||||||
|
exited.then(() => true),
|
||||||
|
new Promise<false>((resolve) => {
|
||||||
|
const timer = setTimeout(() => { resolve(false) }, EXIT_MARKER_GRACE_MS)
|
||||||
|
timer.unref()
|
||||||
|
}),
|
||||||
|
])
|
||||||
|
}
|
||||||
|
|
||||||
/** Whether the child still lacks either OS termination marker. */
|
/** Whether the child still lacks either OS termination marker. */
|
||||||
function isRunning(child: ChildProcessWithoutNullStreams): boolean {
|
function isRunning(child: ChildProcessWithoutNullStreams): boolean {
|
||||||
return child.exitCode === null && child.signalCode === null
|
return child.exitCode === null && child.signalCode === null
|
||||||
|
|||||||
@@ -202,6 +202,28 @@ describe('runScenario', () => {
|
|||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('preserves the child error when the requested signal publishes its exit marker later', async () => {
|
||||||
|
const { dir } = await scenario({})
|
||||||
|
const launched = launchAcpTestAgent({ agent: AGENT, cwd: dir })
|
||||||
|
await launched.spawned
|
||||||
|
|
||||||
|
const childFailure = Object.assign(new Error('signal failed before the delayed exit marker'), { code: 'EPERM' })
|
||||||
|
const originalKill = launched.child.kill.bind(launched.child)
|
||||||
|
const kill = vi.spyOn(launched.child, 'kill').mockImplementation((signal) => {
|
||||||
|
expect(signal).toBe('SIGTERM')
|
||||||
|
setTimeout(() => { originalKill('SIGKILL') }, 10)
|
||||||
|
return true
|
||||||
|
})
|
||||||
|
try {
|
||||||
|
launched.child.emit('error', childFailure)
|
||||||
|
await expect(launched.close('SIGTERM')).rejects.toBe(childFailure)
|
||||||
|
expect(kill).toHaveBeenCalledOnce()
|
||||||
|
} finally {
|
||||||
|
kill.mockRestore()
|
||||||
|
if (launched.child.exitCode === null && launched.child.signalCode === null) originalKill('SIGKILL')
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
it('preserves the child error when fallback refusal races with an exit marker', async () => {
|
it('preserves the child error when fallback refusal races with an exit marker', async () => {
|
||||||
const { dir } = await scenario({})
|
const { dir } = await scenario({})
|
||||||
const launched = launchAcpTestAgent({ agent: AGENT, cwd: dir })
|
const launched = launchAcpTestAgent({ agent: AGENT, cwd: dir })
|
||||||
|
|||||||
Reference in New Issue
Block a user