Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
486 changes: 485 additions & 1 deletion packages/acp-bridge/src/bridge.test.ts

Large diffs are not rendered by default.

343 changes: 279 additions & 64 deletions packages/acp-bridge/src/bridge.ts

Large diffs are not rendered by default.

16 changes: 16 additions & 0 deletions packages/acp-bridge/src/bridgeErrors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,22 @@ export class PromptQueueFullError extends Error {
}
}

/**
* Rejected by `sendPrompt` when an accepted prompt exceeds its wallclock
* deadline (`BridgeClientRequestContext.deadlineMs`). The bridge publishes a
* `turn_error{code:'prompt_deadline_exceeded'}` terminal, releases the FIFO,
* and best-effort cancels the agent — the agent may still be executing.
* Exported so tests and routes can match on the class identity.
*/
export class PromptDeadlineExceededError extends Error {
readonly deadlineMs: number;
constructor(deadlineMs: number) {
super(`prompt exceeded the ${deadlineMs}ms deadline`);
this.name = 'PromptDeadlineExceededError';
this.deadlineMs = deadlineMs;
}
}

/**
* Thrown by `spawnOrAttach` when the requested `workspaceCwd` doesn't
* canonicalize to the bridge's bound workspace. Every bridge instance is bound
Expand Down
14 changes: 14 additions & 0 deletions packages/acp-bridge/src/bridgeTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,14 @@ export interface BridgeClientRequestContext {
* smuggle a continuation through the prompt path.
*/
continue?: boolean;
/**
* Absolute wallclock budget (ms) for this prompt, measured from admission
* (the 202 semantic point) and covering queue wait. When exceeded, the
* bridge publishes a `turn_error{code:'prompt_deadline_exceeded'}` terminal,
* releases the FIFO, and best-effort cancels the agent. Populated by the
* REST prompt route from `resolvePromptDeadlineMs(serverMs, requestMs)`.
*/
deadlineMs?: number;
}

/**
Expand Down Expand Up @@ -581,6 +589,12 @@ export interface PendingPromptEntry {
text: string;
abortController: AbortController;
state: 'queued' | 'running';
/**
* Exactly-once latch for the prompt's formal terminal event
* (`turn_complete` / `turn_error`). Set by `publishPromptTerminal`;
* later publish attempts for the same prompt are suppressed.
*/
terminalPublished?: boolean;
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/serve/acp-session-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export {
InvalidSessionScopeError,
SessionLimitExceededError,
PromptQueueFullError,
PromptDeadlineExceededError,
WorkspaceMismatchError,
InvalidClientIdError,
InvalidPermissionOptionError,
Expand Down
27 changes: 10 additions & 17 deletions packages/cli/src/serve/routes/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,7 @@ import {
} from '../acp-session-bridge.js';
import type { DaemonLogger } from '../daemon-logger.js';
import type { SendBridgeError } from '../server/error-response.js';
import {
PromptDeadlineExceededError,
resolvePromptDeadlineMs,
} from '../server/prompt-deadline.js';
import { resolvePromptDeadlineMs } from '../server/prompt-deadline.js';
import {
parseClientIdHeader,
parseOptionalWorkspaceCwd,
Expand Down Expand Up @@ -2329,19 +2326,16 @@ export function registerSessionRoutes(
};
res.once('close', onResClose);
res.once('finish', onResFinish);
// The effective deadline (server cap ∩ request override) is passed
// to the bridge, which owns the deadline race: it publishes the
// formal `turn_error{code:'prompt_deadline_exceeded'}` terminal,
// releases the per-session FIFO, and best-effort cancels the agent.
// A route-side timer can't do any of that — it could only abort
// this request's signal.
const effectiveDeadlineMs = resolvePromptDeadlineMs(
promptDeadlineMs,
requestDeadlineMs,
);
let deadlineTimer: NodeJS.Timeout | undefined;
if (effectiveDeadlineMs !== undefined) {
deadlineTimer = setTimeout(() => {
if (!abort.signal.aborted) {
abort.abort(new PromptDeadlineExceededError(effectiveDeadlineMs));
}
}, effectiveDeadlineMs);
deadlineTimer.unref();
}

let promptPromise: ReturnType<AcpSessionBridge['sendPrompt']>;
try {
Expand All @@ -2356,10 +2350,12 @@ export function registerSessionRoutes(
{
...(clientId !== undefined ? { clientId } : {}),
promptId,
...(effectiveDeadlineMs !== undefined
? { deadlineMs: effectiveDeadlineMs }
: {}),
},
);
} catch (err) {
if (deadlineTimer !== undefined) clearTimeout(deadlineTimer);
res.off('close', onResClose);
res.off('finish', onResFinish);
if (daemonLog && err instanceof PromptQueueFullError) {
Expand Down Expand Up @@ -2407,9 +2403,6 @@ export function registerSessionRoutes(
}
},
)
.finally(() => {
if (deadlineTimer !== undefined) clearTimeout(deadlineTimer);
})
.catch(() => {});

if (daemonLog) {
Expand Down
131 changes: 65 additions & 66 deletions packages/cli/src/serve/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2156,15 +2156,6 @@ describe('detectFromLoopback (#4335 / 3272581557)', () => {
});
});

function abortableBridgePromptImpl(): FakeBridgeOpts['promptImpl'] {
return (_sid, _req, signal) =>
new Promise((resolve) => {
const onAbort = () => resolve({ stopReason: 'cancelled' });
if (signal?.aborted) onAbort();
else signal?.addEventListener('abort', onAbort, { once: true });
});
}

describe('createServeApp', () => {
it('rejects client-MCP over WS with an injected bridge but no matching sender registry', () => {
expect(() =>
Expand Down Expand Up @@ -22219,20 +22210,15 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
},
);

it('returns cleanly when client disconnects in the same tick the deadline fires (wenshao review #3)', async () => {
// Critical regression from wenshao's CHANGES_REQUESTED on #4530:
// when `res.writableEnded` was true at the moment the deadline
// rejection surfaced, the early code `if (err instanceof
// PromptDeadlineExceededError && !res.writableEnded) { ...
// return; }` would skip BOTH the body AND the return, fall
// through to `sendBridgeError`, and try to write 500 to an
// already-ended response → ERR_STREAM_WRITE_AFTER_END.
//
// We force the race by destroying the client socket
// immediately after the bridge starts the prompt, so by the
// time the 50ms deadline fires the response is already ended.
// The route MUST handle this without throwing — assertion is
// implicit: a thrown uncaughtException would fail the test.
it('returns cleanly when client disconnects right after admission (wenshao review #3)', async () => {
// Historic regression from wenshao's CHANGES_REQUESTED on #4530:
// the route-side deadline rejection used to race `res.writableEnded`
// and write 500 to an already-ended response →
// ERR_STREAM_WRITE_AFTER_END. The deadline timer has since moved
// into the bridge (DAEMON-003), but the socket-destroy-after-202
// scenario stays as a regression guard: the route MUST handle the
// disconnect without throwing — assertion is implicit: a thrown
// uncaughtException would fail the test.
let promptStarted: (() => void) | undefined;
const promptStartedPromise = new Promise<void>((r) => {
promptStarted = r;
Expand Down Expand Up @@ -22282,10 +22268,11 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
// block to handle the race.
await new Promise((r) => setTimeout(r, 200));
expect(bridge.promptCalls).toHaveLength(1);
// The bridge's signal MUST still have been aborted with the
// typed reason — the cleanup path still runs even though the
// response was already ended.
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(true);
// The deadline race now lives in the bridge: after admission the
// route neither arms a timer nor aborts the signal, it only
// forwards the effective deadline via `context.deadlineMs`.
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(false);
expect(bridge.promptCalls[0]?.context?.deadlineMs).toBe(50);
} finally {
await localHandle.close();
}
Expand All @@ -22309,12 +22296,16 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(false);
});

it('fires the server-side deadline and aborts the bridge signal', async () => {
// 50ms server deadline + a prompt that resolves only on abort:
// the deadline timer must abort the AbortController. With non-
// blocking prompt the HTTP response is always 202; the deadline
// outcome is delivered via `turn_error` on the SSE bus.
const bridge = fakeBridge({ promptImpl: abortableBridgePromptImpl() });
it('forwards the server-side deadline to the bridge via context.deadlineMs', async () => {
// The bridge owns the deadline race (DAEMON-003): the route only
// resolves the effective deadline and passes it through the
// sendPrompt context. With non-blocking prompt the HTTP response
// is always 202; the deadline outcome (`turn_error` with code
// `prompt_deadline_exceeded`) is published by the bridge and is
// covered by bridge.test.ts.
const bridge = fakeBridge({
promptImpl: () => new Promise(() => {}),
});
const app = createServeApp(
{ ...baseOpts, promptDeadlineMs: 50 },
undefined,
Expand All @@ -22327,13 +22318,9 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
expect(res.status).toBe(202);
expect(res.body).toHaveProperty('promptId');
expect(res.body).toHaveProperty('lastEventId');
// Wait for the deadline timer to fire asynchronously.
await new Promise((r) => setTimeout(r, 200));
expect(bridge.promptCalls).toHaveLength(1);
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(true);
expect(bridge.promptCalls[0]?.signal?.reason).toBeInstanceOf(
PromptDeadlineExceededError,
);
expect(bridge.promptCalls[0]?.context?.deadlineMs).toBe(50);
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(false);
});

it('strips route-only deadlineMs before forwarding the prompt body', async () => {
Expand Down Expand Up @@ -22367,10 +22354,11 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
});

it('caps a per-prompt `deadlineMs` override at the server flag', async () => {
// Server flag 50ms, request asks for 5000ms — effective deadline
// must be 50ms. With non-blocking prompt the HTTP response is
// always 202; we verify the abort signal fires within ~50ms.
const bridge = fakeBridge({ promptImpl: abortableBridgePromptImpl() });
// Server flag 50ms, request asks for 5000ms — the effective
// deadline forwarded to the bridge must be min(server, request).
const bridge = fakeBridge({
promptImpl: () => new Promise(() => {}),
});
const app = createServeApp(
{ ...baseOpts, promptDeadlineMs: 50 },
undefined,
Expand All @@ -22384,17 +22372,16 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
deadlineMs: 5_000,
});
expect(res.status).toBe(202);
await new Promise((r) => setTimeout(r, 200));
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(true);
expect(bridge.promptCalls[0]?.signal?.reason).toBeInstanceOf(
PromptDeadlineExceededError,
);
expect(bridge.promptCalls).toHaveLength(1);
expect(bridge.promptCalls[0]?.context?.deadlineMs).toBe(50);
});

it('uses the per-prompt override when shorter than the server flag', async () => {
// Server flag 10s, request 30ms — request wins as the tighter
// bound. Abort signal should fire within ~30ms.
const bridge = fakeBridge({ promptImpl: abortableBridgePromptImpl() });
// bound; the bridge receives the request value.
const bridge = fakeBridge({
promptImpl: () => new Promise(() => {}),
});
const app = createServeApp(
{ ...baseOpts, promptDeadlineMs: 10_000 },
undefined,
Expand All @@ -22408,17 +22395,15 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
deadlineMs: 30,
});
expect(res.status).toBe(202);
await new Promise((r) => setTimeout(r, 200));
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(true);
expect(bridge.promptCalls[0]?.signal?.reason).toBeInstanceOf(
PromptDeadlineExceededError,
);
expect(bridge.promptCalls).toHaveLength(1);
expect(bridge.promptCalls[0]?.context?.deadlineMs).toBe(30);
});

it('still aborts the signal when the bridge IGNORES the abort (non-cooperative bridge)', async () => {
// With non-blocking prompt the HTTP response is always 202. The
// deadline timer must still fire and abort the signal so the
// bridge can observe it, even if it ignores the abort.
it('never aborts the bridge signal from a route-side timer (deadline owned by the bridge)', async () => {
// Regression guard for the DAEMON-003 timer migration: even well
// past the configured deadline the route must NOT abort the
// signal — deadline enforcement (terminal publication, FIFO
// release, best-effort agent cancel) is the bridge's job.
const bridge = fakeBridge({
promptImpl: () => new Promise(() => {}),
});
Expand All @@ -22433,10 +22418,8 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
.send({ prompt: [{ type: 'text', text: 'slow' }] });
expect(res.status).toBe(202);
await new Promise((r) => setTimeout(r, 200));
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(true);
expect(bridge.promptCalls[0]?.signal?.reason).toBeInstanceOf(
PromptDeadlineExceededError,
);
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(false);
expect(bridge.promptCalls[0]?.context?.deadlineMs).toBe(50);
});

it('returns 202 without deadline when the flag is unset', async () => {
Expand All @@ -22451,10 +22434,14 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
expect(res.status).toBe(202);
expect(res.body).toHaveProperty('promptId');
expect(res.body).toHaveProperty('lastEventId');
expect(bridge.promptCalls).toHaveLength(1);
expect(bridge.promptCalls[0]?.context?.deadlineMs).toBeUndefined();
});

it('does not fire the deadline when the prompt resolves promptly', async () => {
// 5s deadline + immediate resolve: the timer must not fire.
it('forwards the deadline even when the prompt resolves promptly', async () => {
// 5s deadline + immediate resolve: the route passes the deadline
// through unconditionally; the bridge clears its own timer when
// the prompt settles (covered by bridge.test.ts).
const bridge = fakeBridge({
promptImpl: async () => ({ stopReason: 'end_turn' }),
});
Expand All @@ -22469,9 +22456,21 @@ describe('T2.9 prompt absolute deadline (issue #4514)', () => {
.send({ prompt: [{ type: 'text', text: 'hi' }] });
expect(res.status).toBe(202);
expect(res.body).toHaveProperty('promptId');
// Give enough time for a timer to fire if it were going to.
// Give enough time for a stray route-side timer to fire if one
// still existed.
await new Promise((r) => setTimeout(r, 100));
expect(bridge.promptCalls[0]?.signal?.aborted).toBe(false);
expect(bridge.promptCalls[0]?.context?.deadlineMs).toBe(5_000);
});

it('keeps re-exporting PromptDeadlineExceededError after the bridge migration', () => {
// The class definition moved into acp-bridge (DAEMON-003); the
// server module must keep exporting it so SDK consumers and
// `instanceof` checks across the package boundary stay intact.
const err = new PromptDeadlineExceededError(75);
expect(err).toBeInstanceOf(Error);
expect(err.name).toBe('PromptDeadlineExceededError');
expect(err.deadlineMs).toBe(75);
});
});

Expand Down
16 changes: 5 additions & 11 deletions packages/cli/src/serve/server/prompt-deadline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,12 @@
*/

/**
* Sentinel passed as `AbortController.abort(reason)` when a prompt
* exceeds its server-configured wallclock. Exported so tests can
* match on the class identity.
* Rejected by the bridge's `sendPrompt` when a prompt exceeds its
* wallclock deadline. The class itself lives in the acp-bridge package
* (the bridge owns the deadline race since DAEMON-003); re-exported here
* so existing `server.ts` / test imports keep working.
*/
export class PromptDeadlineExceededError extends Error {
readonly deadlineMs: number;
constructor(deadlineMs: number) {
super(`prompt exceeded the ${deadlineMs}ms deadline`);
this.name = 'PromptDeadlineExceededError';
this.deadlineMs = deadlineMs;
}
}
export { PromptDeadlineExceededError } from '../acp-session-bridge.js';

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. Moving the class into the bridge package is right, but this re-export turns what was a pure leaf utility into a module that pulls the entire bridge in transitively — anything importing this file just for resolvePromptDeadlineMs now pays for it.

No cycle today (acp-session-bridge.ts doesn't import this file), so it isn't urgent. I'll either mark it @deprecated pointing at acp-session-bridge.js, or just update the remaining import sites and drop the forward.


/**
* Resolve the effective per-prompt wallclock from the server flag +
Expand Down
Loading