fix: await ACP client callbacks during shutdown
This commit is contained in:
@@ -65,7 +65,7 @@ export interface LaunchedAcpTestAgent {
|
|||||||
stderr(): string
|
stderr(): string
|
||||||
/** Resolve when a future session update matches the predicate. */
|
/** Resolve when a future session update matches the predicate. */
|
||||||
waitForUpdate(match: (update: SessionNotification['update']) => boolean): Promise<SessionNotification['update']>
|
waitForUpdate(match: (update: SessionNotification['update']) => boolean): Promise<SessionNotification['update']>
|
||||||
/** Gracefully close stdin, or send a signal, then wait for process exit, inherited stdio closure, and ACP parser drain. */
|
/** Gracefully close stdin, or send a signal, then wait for process exit, inherited stdio closure, ACP parsing, and client callbacks. */
|
||||||
close(signal?: NodeJS.Signals): Promise<void>
|
close(signal?: NodeJS.Signals): Promise<void>
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -137,29 +137,41 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe
|
|||||||
Writable.toWeb(child.stdin) as WritableStream<Uint8Array>,
|
Writable.toWeb(child.stdin) as WritableStream<Uint8Array>,
|
||||||
Readable.toWeb(passthrough) as ReadableStream<Uint8Array>,
|
Readable.toWeb(passthrough) as ReadableStream<Uint8Array>,
|
||||||
)
|
)
|
||||||
|
const inFlightClientCallbacks = new Set<Promise<unknown>>()
|
||||||
|
const trackClientCallback = <T>(callback: () => T | PromiseLike<T>): Promise<T> => {
|
||||||
|
const pending = Promise.resolve().then(callback)
|
||||||
|
inFlightClientCallbacks.add(pending)
|
||||||
|
void pending.then(
|
||||||
|
() => { inFlightClientCallbacks.delete(pending) },
|
||||||
|
() => { inFlightClientCallbacks.delete(pending) },
|
||||||
|
)
|
||||||
|
return pending
|
||||||
|
}
|
||||||
|
const requestPermission = options.requestPermission
|
||||||
|
?? (() => Promise.resolve({ outcome: { outcome: 'cancelled' as const } }))
|
||||||
const makeClient = (_agent: AcpAgent): Client => ({
|
const makeClient = (_agent: AcpAgent): Client => ({
|
||||||
sessionUpdate(params: SessionNotification): Promise<void> {
|
sessionUpdate(params: SessionNotification): Promise<void> {
|
||||||
updates.push(params.update)
|
return trackClientCallback(() => {
|
||||||
for (let index = updateWaiters.length - 1; index >= 0; index--) {
|
updates.push(params.update)
|
||||||
const waiter = updateWaiters[index]
|
for (let index = updateWaiters.length - 1; index >= 0; index--) {
|
||||||
/* v8 ignore next 1 -- index is bounded by the array length */
|
const waiter = updateWaiters[index]
|
||||||
if (waiter === undefined) continue
|
/* v8 ignore next 1 -- index is bounded by the array length */
|
||||||
let matches: boolean
|
if (waiter === undefined) continue
|
||||||
try {
|
let matches: boolean
|
||||||
matches = waiter.match(params.update)
|
try {
|
||||||
} catch (error: unknown) {
|
matches = waiter.match(params.update)
|
||||||
|
} catch (error: unknown) {
|
||||||
|
updateWaiters.splice(index, 1)
|
||||||
|
waiter.reject(error)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if (!matches) continue
|
||||||
updateWaiters.splice(index, 1)
|
updateWaiters.splice(index, 1)
|
||||||
waiter.reject(error)
|
waiter.resolve(params.update)
|
||||||
continue
|
|
||||||
}
|
}
|
||||||
if (!matches) continue
|
})
|
||||||
updateWaiters.splice(index, 1)
|
|
||||||
waiter.resolve(params.update)
|
|
||||||
}
|
|
||||||
return Promise.resolve()
|
|
||||||
},
|
},
|
||||||
requestPermission: options.requestPermission
|
requestPermission: params => trackClientCallback(() => requestPermission(params)),
|
||||||
?? (() => Promise.resolve({ outcome: { outcome: 'cancelled' } })),
|
|
||||||
})
|
})
|
||||||
const client = new ClientSideConnection(makeClient, stream)
|
const client = new ClientSideConnection(makeClient, stream)
|
||||||
// `exit` only reports the parent process's status. Descendants may retain
|
// `exit` only reports the parent process's status. Descendants may retain
|
||||||
@@ -168,7 +180,14 @@ export function launchAcpTestAgent(options: AcpTestLaunchOptions): LaunchedAcpTe
|
|||||||
// `closed` follows parser exhaustion. Capture both eagerly so a caller that
|
// `closed` follows parser exhaustion. Capture both eagerly so a caller that
|
||||||
// invokes close after process exit still joins the complete drain boundary.
|
// invokes close after process exit still joins the complete drain boundary.
|
||||||
const stdioClosed = new Promise<void>(resolve => child.once('close', () => { resolve() }))
|
const stdioClosed = new Promise<void>(resolve => child.once('close', () => { resolve() }))
|
||||||
const drained = Promise.all([stdioClosed, client.closed]).then(() => undefined)
|
const drained = Promise.all([stdioClosed, client.closed]).then(async () => {
|
||||||
|
// The ACP SDK's readable loop dispatches client callbacks without awaiting
|
||||||
|
// them. Once `closed` settles no new callbacks can start, but callbacks
|
||||||
|
// already in flight still belong to this launch's teardown boundary.
|
||||||
|
while (inFlightClientCallbacks.size > 0) {
|
||||||
|
await Promise.allSettled([...inFlightClientCallbacks])
|
||||||
|
}
|
||||||
|
})
|
||||||
// A caller may await a pending update without calling close(). Make natural
|
// A caller may await a pending update without calling close(). Make natural
|
||||||
// stream exhaustion terminal for those waiters too, but only after the
|
// stream exhaustion terminal for those waiters too, but only after the
|
||||||
// parser has dispatched every buffered frame.
|
// parser has dispatched every buffered frame.
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import { mkdtemp, rm, writeFile } from 'node:fs/promises'
|
import { mkdtemp, rm, writeFile } from 'node:fs/promises'
|
||||||
|
import { once } from 'node:events'
|
||||||
import { tmpdir } from 'node:os'
|
import { tmpdir } from 'node:os'
|
||||||
import { delimiter, join } from 'node:path'
|
import { delimiter, join } from 'node:path'
|
||||||
import { fileURLToPath } from 'node:url'
|
import { fileURLToPath } from 'node:url'
|
||||||
@@ -112,6 +113,41 @@ describe('runScenario', () => {
|
|||||||
expect(launched.stderr()).toContain('late inherited stderr')
|
expect(launched.stderr()).toContain('late inherited stderr')
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('waits for in-flight client callbacks after the ACP stream closes', { timeout: 20_000 }, async () => {
|
||||||
|
const { dir, fixtureFile } = await scenario({ permissionProbe: true })
|
||||||
|
let releasePermission: (() => void) | undefined
|
||||||
|
const permissionReleased = new Promise<void>((resolve) => { releasePermission = resolve })
|
||||||
|
let markPermissionStarted: (() => void) | undefined
|
||||||
|
const permissionStarted = new Promise<void>((resolve) => { markPermissionStarted = resolve })
|
||||||
|
let permissionFinished = false
|
||||||
|
const launched = launchAcpTestAgent({
|
||||||
|
agent: AGENT,
|
||||||
|
cwd: dir,
|
||||||
|
env: { DSH_SNAPSHOT_FILE: fixtureFile },
|
||||||
|
async requestPermission() {
|
||||||
|
markPermissionStarted?.()
|
||||||
|
await permissionReleased
|
||||||
|
permissionFinished = true
|
||||||
|
return { outcome: { outcome: 'cancelled' } }
|
||||||
|
},
|
||||||
|
})
|
||||||
|
await launched.client.initialize({ protocolVersion: PROTOCOL_VERSION, clientCapabilities: {} })
|
||||||
|
const { sessionId } = await launched.client.newSession({ cwd: dir, mcpServers: [] })
|
||||||
|
void launched.client.prompt({ sessionId, prompt: [{ type: 'text', text: 'go' }] }).catch(() => undefined)
|
||||||
|
await permissionStarted
|
||||||
|
|
||||||
|
const childClosed = once(launched.child, 'close')
|
||||||
|
let closeSettled = false
|
||||||
|
const closing = launched.close('SIGKILL').then(() => { closeSettled = true })
|
||||||
|
await childClosed
|
||||||
|
await launched.client.closed
|
||||||
|
expect(closeSettled).toBe(false)
|
||||||
|
|
||||||
|
releasePermission?.()
|
||||||
|
await closing
|
||||||
|
expect(permissionFinished).toBe(true)
|
||||||
|
})
|
||||||
|
|
||||||
it('drives a full turn: initialize (terminal caps), session, prompt, permission stub, harvest', { timeout: 20_000 }, async () => {
|
it('drives a full turn: initialize (terminal caps), session, prompt, permission stub, harvest', { timeout: 20_000 }, async () => {
|
||||||
const { fixtureFile } = await scenario({
|
const { fixtureFile } = await scenario({
|
||||||
permissionProbe: true,
|
permissionProbe: true,
|
||||||
|
|||||||
Reference in New Issue
Block a user