From 9e8caef6fc268396f8039901dd90ab0bc9914209 Mon Sep 17 00:00:00 2001 From: "jinye.djy" Date: Thu, 27 Aug 2026 17:03:58 +0800 Subject: [PATCH 1/6] fix(daemon): cancel timed-out session initialization Propagate the daemon initialization deadline through ACP session startup and SessionStart hooks, while containing late results from older agents without disrupting shared-channel siblings. Co-authored-by: Qwen-Coder --- .../acp-session-initialization-deadline.md | 81 ++++ docs/developers/daemon/18-error-taxonomy.md | 2 +- docs/developers/qwen-serve-protocol.md | 4 +- docs/users/qwen-serve.md | 2 +- packages/acp-bridge/src/bridge.test.ts | 219 +++++++++++ packages/acp-bridge/src/bridge.ts | 367 ++++++++++++++++-- packages/acp-bridge/src/bridgeErrors.ts | 20 +- packages/acp-bridge/src/bridgeTypes.ts | 4 + .../cli/src/acp-integration/acpAgent.test.ts | 62 +++ packages/cli/src/acp-integration/acpAgent.ts | 75 +++- .../src/serve/acp-http/dispatch-error.test.ts | 25 +- packages/cli/src/serve/server.test.ts | 53 +-- packages/core/src/config/config.ts | 15 +- packages/core/src/core/client.test.ts | 29 ++ packages/core/src/core/client.ts | 40 +- 15 files changed, 903 insertions(+), 95 deletions(-) create mode 100644 docs/design/acp-session-initialization-deadline.md diff --git a/docs/design/acp-session-initialization-deadline.md b/docs/design/acp-session-initialization-deadline.md new file mode 100644 index 00000000000..c082fc7504b --- /dev/null +++ b/docs/design/acp-session-initialization-deadline.md @@ -0,0 +1,81 @@ +# ACP session initialization deadline + +Status: implemented for PR3 + +Proposed PR title: `fix(daemon): Cancel timed-out session initialization` + +## Problem + +The daemon bounds `newSession`, but the timeout historically rejected only the Bridge wrapper. The ACP request continued inside the child. A slow `SessionStart` command hook could therefore finish after the caller had already received `init_timeout`, publish a real child Session that the Bridge never registered, and leave hook descendants or other session resources alive. + +This is different from an ACP channel teardown. A shared channel can own healthy sibling Sessions, so killing the channel at the first session timeout would turn one failed create into unrelated session loss. It is also different from the HookRunner and ACP process-tree fixes: those changes provide tree-aware cancellation and channel cleanup, but neither decides when session initialization should be cancelled. + +## Scope + +This change makes the existing Bridge initialization budget authoritative for the standard trusted daemon-to-ACP path: + +- the Bridge sends an absolute initialization deadline with each `newSession` request; +- the managed ACP Agent converts that deadline into an `AbortSignal`; +- Config and Gemini initialization forward the signal into `SessionStart` hook execution and check it at initialization boundaries; +- the Agent rejects before publishing a timed-out Session; +- the Bridge retains a compatibility lifecycle for an older Agent that ignores the deadline and settles late. + +The public API remains the existing `init_timeout` failure. The private deadline and internal child error kind do not become HTTP or SDK fields. + +## Deadline contract + +The Bridge writes `qwen.daemon.sessionInitializationDeadlineMs` into `_meta` immediately before dispatching the actual ACP request. Its value is an absolute Unix timestamp derived from the configured `initializeTimeoutMs`. An absolute deadline prevents serialization, transport, and child scheduling time from accidentally granting a new full budget at each layer. + +Only an ACP Agent that completed the private managed-parent capability handshake reads the field. An untrusted or standalone ACP caller cannot use request metadata to cancel initialization. A trusted value must be a positive safe integer within Node's supported timer range; malformed values fail before settings or session state is created. + +The Agent owns one request-scoped `AbortController`. Its timer is unreferenced and cleared in `finally`. The signal is not stored on the resulting Session and cannot cancel later turns. + +## Cancellation path + +The signal follows the existing initialization ownership path: + +`ACP newSession -> Config.initialize -> GeminiClient.initialize -> startChat -> SessionStart HookSystem -> HookRunner` + +Config checks cancellation before registering initialization state and after awaited initialization phases. GeminiClient passes the signal into `SessionStart`, checks it after the hook result, and rethrows its abort reason instead of applying the hook's ordinary best-effort error policy. This is required because HookSystem can aggregate cancellation into a result instead of throwing it directly. + +Before `QwenAgent` publishes the new Session in its session map, it checks the signal again. The child reports the private `session_initialization_timeout` error kind, and the Bridge maps it back to the existing `BridgeTimeoutError('newSession')` contract. + +The change does not race the whole Config initialization against a rejecting wrapper Promise. Doing so would let cleanup run concurrently with initialization code that still owns the same Config. Operations without an AbortSignal API finish normally and are followed by a cancellation checkpoint; the Bridge compatibility lifecycle remains the outer containment boundary. + +## Late-result compatibility + +The Bridge observes the raw ACP request after its public timer fires. This protects rolling upgrades and other Agents that do not yet consume the private deadline. + +- A late failure releases the hidden-work accounting and any caller-supplied ID fence. +- A late success is never registered. The Bridge sends one bounded `qwen/control/session/close` for the returned Session ID, and only `closed: true` is accepted as proof that cleanup completed. +- Resource-not-found means cleanup is already complete. +- A close failure quarantines only fresh session admission on that channel. Existing sibling Sessions continue until they drain, after which the channel is reaped. +- If the raw request remains unsettled for one additional initialization budget, the channel similarly refuses fresh Sessions until it drains. +- An empty timed-out channel follows the existing immediate teardown path; a shared channel is not killed while siblings remain. + +The Bridge holds the fresh-session admission reservation and a caller-supplied ID reservation until the raw request and cleanup settle. Abandoned requests count toward `maxSessions`, and shutdown awaits their settlement after initiating channel teardown. This prevents retries from overcommitting resources or reclaiming an ID that a late child response can still create. + +## Failure semantics + +- A deadline enforced by the new Agent and a wrapper timeout from an older Agent both surface as the existing public initialization timeout. +- A normal initialization failure remains unchanged. +- A timed-out Session is never inserted into the Agent or Bridge session maps. +- Existing Sessions on a shared channel remain usable during late settlement and cleanup. +- Cleanup uncertainty fails closed for new session creation but does not broaden into daemon-wide or sibling-session termination. +- Channel exit settles abandoned work through the existing transport-close race. + +## Non-goals + +- Changing the configured initialization timeout or adding per-endpoint deadlines. +- Cancelling load or resume, prompt execution, authentication APIs, MCP discovery APIs, or arbitrary extension initialization. +- Changing HookRunner timeout defaults or HookRunner process ownership. +- Replacing ACP process-group cleanup with cgroups, Windows Job Objects, or another OS supervisor. +- Changing standalone binding, session ownership, bridge routing, or HTTP response shapes. + +## Verification + +- Reproduce the baseline with a shared channel whose second `newSession` times out and later succeeds: the child creates a Session that the Bridge cannot see. +- Verify the Bridge sends an absolute deadline, preserves the public timeout type, closes a late-created Session, keeps a sibling alive, fences a requested ID, counts abandoned work against the cap, and quarantines fresh admission on cleanup failure or overdue settlement. +- Verify only a trusted managed parent can activate the Agent deadline and that abort rejects before Session publication. +- Verify Gemini initialization forwards the signal into `SessionStart` and does not swallow cancellation as an ordinary hook failure. +- Run the affected Bridge, Agent, Config/client, HookRunner, and process-tree tests, followed by build, typecheck, lint, formatting, and clean-diff audits. diff --git a/docs/developers/daemon/18-error-taxonomy.md b/docs/developers/daemon/18-error-taxonomy.md index 053b96b2a3f..420a4b9ff99 100644 --- a/docs/developers/daemon/18-error-taxonomy.md +++ b/docs/developers/daemon/18-error-taxonomy.md @@ -59,7 +59,7 @@ Typed classes thrown by the bridge / mediator. Most carry an HTTP status via the | `BridgeChannelClosedError` | 503 | ACP child channel closed mid-call. | Reconnect / retry; check `session_died` for cause. | | `BridgeTimeoutError` | 504 | Bridge-level wallclock exceeded. | Retry; investigate underlying slowness. | | `SessionRestoreTimeoutError` | 504 | ACP session load/resume exceeded its dedicated restore budget. | Retry after the advertised delay; inspect restore stage traces before raising the budget. | -| `BridgeChannelQuarantinedError` | 503 | Abandoned-restore cleanup was inconclusive (`restore_cleanup_failed`), or an abandoned restore has not settled a full budget after its deadline (`restore_settlement_overdue`); either way the workspace channel refuses fresh sessions until it drains. The 503 body carries `reason` and `retryAfterSeconds`. | Keep using existing sessions, wait for the channel to recycle, then retry fresh session work. | +| `BridgeChannelQuarantinedError` | 503 | `reason` is `restore_cleanup_failed`, `restore_settlement_overdue`, `new_session_cleanup_failed`, or `new_session_settlement_overdue`; fresh session work is refused until the workspace channel drains. The 503 body also carries `retryAfterSeconds`. Reasons mark cleanup uncertainty or overdue settlement. | Keep using existing sessions, wait for the channel to recycle, then retry fresh session work. | | `MissingCliEntryError` | 500 | The `qwen` CLI entry file is missing (defined in `status.ts`, not `bridgeErrors.ts`). | Confirm the CLI install is complete; check that `packages/cli/index.ts` exists. | ## Boot-time configuration errors (`packages/cli/src/serve/run-qwen-serve.ts`) diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index b94eb0b785f..af5743c73d8 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -2172,7 +2172,7 @@ Response: **History replay over SSE.** While `loadSession` is in flight on the agent side, the agent may emit `session_update` notifications for persisted turns, or return bulk replay updates in the response metadata. The daemon seeds those events into the session's bounded replay snapshot window before the route response returns. For live sessions, `POST /session/:id/load` only promises that bounded window (`compactedReplay`, `liveJournal`, `lastEventId`), not the full transcript. The window is byte-capped by `--compacted-replay-max-bytes` (default 4 MiB, maximum 256 MiB); if older replay entries were dropped, `compactedReplay[0]` is an id-less `history_truncated` marker. The in-flight `liveJournal` is separately capped by `--max-journal-events` (default 10 000 replay entries) and `--max-journal-bytes` (default 8 MiB of serialized source events). These are per-session **baseline** caps. When an in-flight turn outgrows them, the daemon first tries adaptive growth: it raises that session's caps toward double (up to a per-session hard cap of 256 MiB, entries scaled proportionally, limited by the remaining pool headroom) while the growth granted across every live session fits in one daemon-wide growth pool sized at 5% of the daemon's effective memory budget — the `--memory-budget-mb` value when passed, capped at resolved available memory, otherwise 50% of auto-detected memory — capped at `1024` MB. Accounting is daemon-wide — a multi-workspace daemon runs one bridge per workspace and all of them share the single pool. Growth is on demand and only as far as the pool allows; an operator-pinned `--max-journal-events` or `--max-journal-bytes` disables it, as does a host whose effective budget falls below the 1024 MB minimum (`insufficientMemory`): the pool is 0 and adaptive growth is disabled outright. Consecutive compatible `agent_message_chunk` or `agent_thought_chunk` source events share a replay entry, up to 256 source events per entry, while tool, attribution, provenance, and discrete-message boundaries remain intact. When the journal still exceeds its (possibly grown) caps after the growth the pool allows — including when no headroom is granted or a grant covers only part of the overshoot — the oldest entries are dropped whole (so the retained tail can be much smaller than the byte cap) and a `history_truncated` marker with `scope: 'live_journal'` is prepended; its `truncatedEvents` and `retainedEvents` fields count source events, not replay entries, and its `maxBytes` / `maxEvents` reflect the caps in force (which may already have grown). Clients should render that marker as status and continue applying retained events. Full persisted transcript access is exposed separately through `GET /session/:id/transcript`. -The replay-window byte caps apply after the child has reconstructed the persisted transcript; they do not cap the on-disk JSONL read. A restore that exceeds the daemon budget returns `504` with a `Retry-After` derived from the restore budget (clamped to 5-120s) and `{code: "session_restore_timeout", errorKind: "restore_timeout", retryable: true, sessionId, action, timeoutMs}`. The daemon fences the still-running ACP request and cleans up any late session instead of registering it. A retry for the same id returns `409 restore_in_progress` with `reason: "awaiting_abandoned_cleanup"` and a `Retry-After` of the restore budget (clamped to 5-120s) until that cleanup settles. If late cleanup is uncertain, or the abandoned restore has still not settled a full restore budget after its deadline, new sessions on that workspace return `503 acp_channel_unavailable` with `reason: "restore_cleanup_failed"` or `"restore_settlement_overdue"`; already-live sessions remain usable while the channel drains. +The replay-window byte caps apply after the child has reconstructed the persisted transcript; they do not cap the on-disk JSONL read. A restore that exceeds the daemon budget returns `504` with a `Retry-After` derived from the restore budget (clamped to 5-120s) and `{code: "session_restore_timeout", errorKind: "restore_timeout", retryable: true, sessionId, action, timeoutMs}`. The daemon fences the still-running ACP request and cleans up any late session instead of registering it. A retry for the same id returns `409 restore_in_progress` with `reason: "awaiting_abandoned_cleanup"` and a `Retry-After` of the restore budget (clamped to 5-120s) until that cleanup settles. If late restore cleanup is uncertain, or the abandoned restore has still not settled a full restore budget after its deadline, new sessions on that workspace return `503 acp_channel_unavailable` with `reason: "restore_cleanup_failed"` or `"restore_settlement_overdue"`. A timed-out session initialization follows the same fail-closed admission policy for an older ACP child that settles late: inconclusive cleanup returns `reason: "new_session_cleanup_failed"`, while a request that remains unsettled for one further initialization budget returns `reason: "new_session_settlement_overdue"`. Already-live sessions remain usable while the channel drains for all four reasons. **Errors:** @@ -2181,7 +2181,7 @@ The replay-window byte caps apply after the child has reconstructed the persiste - `403` — `untrusted_workspace` when `cwd` targets an untrusted non-primary workspace. - `503` — `session_limit_exceeded` (counts against `--max-sessions`; in-flight restores are accounted for too). - `504` — `session_restore_timeout`; retryable, with a `Retry-After` derived from the restore budget (clamped to 5-120s) because the same session id stays fenced until late cleanup settles. -- `503` — `acp_channel_unavailable` when the workspace channel is closed to new session work. `reason` says why: `restore_cleanup_failed` when an abandoned restore could not be cleaned up conclusively, or `restore_settlement_overdue` when an abandoned restore has still not settled one full restore budget after its deadline. In both cases existing sessions remain available, and new session work may be retried after the workspace channel drains — the body carries `retryAfterSeconds` and the header a matching budget-derived `Retry-After`, because quarantine outlives the fence and a fresh id never sees the 409 that would carry the hint. +- `503` — `acp_channel_unavailable` when the workspace channel is closed to new session work. `reason` says why: `restore_cleanup_failed` when an abandoned restore could not be cleaned up conclusively; `restore_settlement_overdue` when an abandoned restore has still not settled one full restore budget after its deadline; `new_session_cleanup_failed` when a session created after the public initialization timeout could not be closed conclusively; or `new_session_settlement_overdue` when the timed-out initialization has still not settled one further initialization budget after its deadline. In all four cases existing sessions remain available, and new session work may be retried after the workspace channel drains — the body carries `retryAfterSeconds` and the header a matching budget-derived `Retry-After`, because quarantine outlives the fence and a fresh id never sees the 409 that would carry the hint. - `409` — `restore_in_progress` (a `session/resume` for the same id is already in flight, or a fresh spawn supplied an id a restore owns). `Retry-After: 5` while the restore is active; a budget-derived hint once it is fenced as `awaiting_abandoned_cleanup`. Same-action races (two concurrent `session/load` for the same id) coalesce — exactly one returns `attached: false`, the rest return `attached: true` with the same `state`. - `409` — `session_workspace_conflict` when the same session id is already live or being restored by another workspace runtime. - `409` — `session_archived` when the id exists only under `chats/archive/`; call `POST /sessions/unarchive` before `load` or `resume`. diff --git a/docs/users/qwen-serve.md b/docs/users/qwen-serve.md index 7a492021f39..5dc941e83cd 100644 --- a/docs/users/qwen-serve.md +++ b/docs/users/qwen-serve.md @@ -752,7 +752,7 @@ for await (const event of session.events()) { } ``` -Pre-flight `caps.features.session_load`, `caps.features.session_resume`, or `caps.features.session_transcript` before calling the matching route — older daemons return `404`. `unstable_session_resume` is still advertised as a deprecated compatibility alias. Concurrent same-action requests for the same id coalesce; cross-action races (a `load` racing a `resume`) and caller-supplied-id spawns racing a restore get `409 restore_in_progress` with `Retry-After: 5`. A restore that exceeds `limits.sessionRestoreTimeoutMs` gets retryable `504 session_restore_timeout` with a budget-derived `Retry-After` (clamped to 5-120s); the still-running child request remains fenced until cleanup settles, and same-id retries during that window get `409 restore_in_progress` with `reason: awaiting_abandoned_cleanup` and a budget-derived `Retry-After` clamped to 5-120 seconds instead of a fixed 5-second delay. If cleanup is uncertain, or the abandoned restore has still not settled a full restore budget after its deadline, fresh session work temporarily gets `503 acp_channel_unavailable` with `reason: restore_cleanup_failed` or `restore_settlement_overdue`, while already-live sessions remain usable. See the [protocol reference](../developers/qwen-serve-protocol.md) for the full error envelope. +Pre-flight `caps.features.session_load`, `caps.features.session_resume`, or `caps.features.session_transcript` before calling the matching route — older daemons return `404`. `unstable_session_resume` is still advertised as a deprecated compatibility alias. Concurrent same-action requests for the same id coalesce; cross-action races (a `load` racing a `resume`) and caller-supplied-id spawns racing a restore get `409 restore_in_progress` with `Retry-After: 5`. A restore that exceeds `limits.sessionRestoreTimeoutMs` gets retryable `504 session_restore_timeout` with a budget-derived `Retry-After` (clamped to 5-120s); the still-running child request remains fenced until cleanup settles, and same-id retries during that window get `409 restore_in_progress` with `reason: awaiting_abandoned_cleanup` and a budget-derived `Retry-After` clamped to 5-120 seconds instead of a fixed 5-second delay. If cleanup is uncertain, or the abandoned restore has still not settled a full restore budget after its deadline, fresh session work temporarily gets `503 acp_channel_unavailable` with `reason: restore_cleanup_failed` or `restore_settlement_overdue`. A session initialization that times out but does not settle promptly can similarly return `reason: new_session_cleanup_failed` when late cleanup is inconclusive or `new_session_settlement_overdue` when the child request remains outstanding for another initialization budget. Already-live sessions remain usable for all four reasons. See the [protocol reference](../developers/qwen-serve-protocol.md) for the full error envelope. For full persisted replay, page with `DaemonClient.getSessionTranscriptPage(sessionId, { cursor, limit })` or the raw REST route: diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index c969bb21cb2..d6c11083f00 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -37,6 +37,7 @@ import { PromptDeadlineExceededError, PromptQueueFullError, RestoreInProgressError, + SessionLimitExceededError, SessionShellClientRequiredError, SessionShellDisabledError, SessionBusyError, @@ -131,6 +132,7 @@ import { SessionAttachmentStore } from './sessionAttachments.js'; import { MultiClientPermissionMediator } from './permissionMediator.js'; import { REQUESTED_SESSION_ID_META_KEY, + SESSION_INITIALIZATION_DEADLINE_META_KEY, MID_TURN_QUEUE_DRAIN_METHOD, MID_TURN_RECONCILIATION_RING_SIZE, PROMPT_CANCEL_METHOD, @@ -12720,6 +12722,214 @@ describe('createAcpSessionBridge', () => { expect(retry.sessionId).toContain('c1'); }); + it('closes a session created after the public newSession deadline', async () => { + const late = deferred(); + const closeCalls: Array> = []; + const handle = makeChannel({ + newSessionImpl: (params, agent) => + agent.newSessionCalls.length === 2 + ? late.promise + : { sessionId: `visible-${agent.newSessionCalls.length}` }, + extMethodImpl: (method, params) => { + if (method === SERVE_CONTROL_EXT_METHODS.sessionClose) { + closeCalls.push(params); + return { closed: true }; + } + return {}; + }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + maxSessions: 2, + sessionScope: 'thread', + }); + const sibling = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toBeInstanceOf(BridgeTimeoutError); + expect(handle.killed).toBe(false); + expect(bridge.sessionCount).toBe(1); + expect( + handle.agent.newSessionCalls[1]?._meta?.[ + SESSION_INITIALIZATION_DEADLINE_META_KEY + ], + ).toEqual(expect.any(Number)); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toBeInstanceOf(SessionLimitExceededError); + + late.resolve({ sessionId: 'hidden-late' }); + await vi.waitFor(() => + expect(closeCalls).toContainEqual( + expect.objectContaining({ sessionId: 'hidden-late' }), + ), + ); + expect(bridge.sessionCount).toBe(1); + + const next = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + expect(next.sessionId).toBe('visible-3'); + expect(bridge.sessionCount).toBe(2); + expect(sibling.sessionId).toBe('visible-1'); + await bridge.shutdown(); + }); + + it('holds a requested id until an abandoned newSession settles', async () => { + const late = deferred(); + const handle = makeChannel({ + newSessionImpl: (_params, agent) => + agent.newSessionCalls.length === 2 + ? late.promise + : { sessionId: `visible-${agent.newSessionCalls.length}` }, + extMethodImpl: () => ({ closed: true }), + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + sessionScope: 'thread', + }); + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.spawnOrAttach({ + workspaceCwd: WS_A, + sessionId: 'requested-late', + }), + ).rejects.toBeInstanceOf(BridgeTimeoutError); + await expect( + bridge.spawnOrAttach({ + workspaceCwd: WS_A, + sessionId: 'requested-late', + }), + ).rejects.toMatchObject({ + activeAction: 'spawn', + requestedAction: 'spawn', + }); + + late.resolve({ sessionId: 'requested-late' }); + await vi.waitFor(() => + expect(handle.agent.extMethodCalls).toContainEqual( + expect.objectContaining({ + method: SERVE_CONTROL_EXT_METHODS.sessionClose, + params: expect.objectContaining({ sessionId: 'requested-late' }), + }), + ), + ); + await expect( + bridge.spawnOrAttach({ + workspaceCwd: WS_A, + sessionId: 'requested-late', + }), + ).resolves.toMatchObject({ sessionId: 'visible-3' }); + await bridge.shutdown(); + }); + + it('keeps the public timeout contract when the agent enforces the deadline', async () => { + const handle = makeChannel({ + newSessionImpl: (_params, agent) => { + if (agent.newSessionCalls.length === 1) { + return { sessionId: 'visible-sibling' }; + } + throw new RequestError( + -32603, + 'Session initialization deadline exceeded', + { errorKind: 'session_initialization_timeout' }, + ); + }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 1_000, + sessionScope: 'thread', + }); + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ + name: 'BridgeTimeoutError', + label: 'newSession', + timeoutMs: 1_000, + }); + expect(handle.killed).toBe(false); + expect(bridge.sessionCount).toBe(1); + await bridge.shutdown(); + }); + + it('refuses fresh sessions when late newSession close is refused', async () => { + const late = deferred(); + const handle = makeChannel({ + newSessionImpl: (_params, agent) => + agent.newSessionCalls.length === 2 + ? late.promise + : { sessionId: `visible-${agent.newSessionCalls.length}` }, + extMethodImpl: (method) => { + if (method === SERVE_CONTROL_EXT_METHODS.sessionClose) { + return { closed: false, holds: [agentHold('late-agent')] }; + } + return {}; + }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + sessionScope: 'thread', + }); + const sibling = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toBeInstanceOf(BridgeTimeoutError); + + late.resolve({ sessionId: 'hidden-late' }); + await vi.waitFor(() => + expect(handle.agent.extMethodCalls).toContainEqual( + expect.objectContaining({ + method: SERVE_CONTROL_EXT_METHODS.sessionClose, + }), + ), + ); + await new Promise((resolve) => setImmediate(resolve)); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ reason: 'new_session_cleanup_failed' }); + await expect( + bridge.sendPrompt(sibling.sessionId, { + sessionId: sibling.sessionId, + prompt: [{ type: 'text', text: 'still alive' }], + }), + ).resolves.toMatchObject({ stopReason: 'end_turn' }); + expect(handle.killed).toBe(false); + expect(bridge.sessionCount).toBe(1); + await bridge.shutdown(); + }); + + it('refuses fresh sessions when an abandoned newSession does not settle', async () => { + const handle = makeChannel({ + newSessionImpl: (_params, agent) => + agent.newSessionCalls.length === 2 + ? new Promise(() => {}) + : { sessionId: `visible-${agent.newSessionCalls.length}` }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + sessionScope: 'thread', + }); + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toBeInstanceOf(BridgeTimeoutError); + + await new Promise((resolve) => setTimeout(resolve, 30)); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ reason: 'new_session_settlement_overdue' }); + expect(handle.killed).toBe(false); + expect(bridge.sessionCount).toBe(1); + await bridge.shutdown(); + }); + it('killAllSync force-kills BOTH the dying channel AND the fresh attach-target (BkUyD overwrite race)', async () => { // The killSession → spawnOrAttach race opens a window where two // channels are simultaneously "alive" from the daemon's @@ -20920,6 +21130,15 @@ describe('createAcpSessionBridge', () => { expect(() => makeBridge({ initializeTimeoutMs: -1 })).toThrow( /initializeTimeoutMs/, ); + expect(() => makeBridge({ initializeTimeoutMs: Number.NaN })).toThrow( + /initializeTimeoutMs/, + ); + expect(() => makeBridge({ initializeTimeoutMs: Infinity })).toThrow( + /initializeTimeoutMs/, + ); + expect(() => makeBridge({ initializeTimeoutMs: 2_147_483_648 })).toThrow( + /initializeTimeoutMs/, + ); }); it('rejects NaN maxSessions (BRApy: silent fail-OPEN guard)', () => { diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 25ae83e8844..d3c5073e7e8 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -165,6 +165,8 @@ import { MID_TURN_RECONCILIATION_RING_SIZE, PROMPT_CANCEL_METHOD, REQUESTED_SESSION_ID_META_KEY, + SESSION_INITIALIZATION_DEADLINE_META_KEY, + SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND, TODO_STOP_GUARD_QUEUE_RELEASE_METHOD, WORKTREE_MCP_DEFER_META_KEY, isValidTrustedModelPrompt, @@ -939,6 +941,14 @@ interface ChannelInfo { restoreSettlementOverdue: boolean; /** Grace timers armed at restore abandonment, keyed by session id. */ restoreSettlementTimers: Map; + /** Timed-out newSession requests whose underlying ACP call is still live. */ + unsettledAbandonedNewSessions: Set; + /** Set once an abandoned newSession outlives one further init budget. */ + newSessionSettlementOverdue: boolean; + /** Grace timers armed at newSession abandonment. */ + newSessionSettlementTimers: Map; + /** A late-created session could not be closed deterministically. */ + newSessionCleanupFailed: boolean; /** Transport guard fired before the child process exited. */ transportFailed: boolean; /** The transport guard, rather than an existing teardown, condemned it. */ @@ -2641,6 +2651,12 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { if (ci.restoreSettlementOverdue) { return { channel: ci, reason: 'restore_settlement_overdue' }; } + if (ci.newSessionCleanupFailed) { + return { channel: ci, reason: 'new_session_cleanup_failed' }; + } + if (ci.newSessionSettlementOverdue) { + return { channel: ci, reason: 'new_session_settlement_overdue' }; + } } return undefined; }; @@ -2649,7 +2665,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { if (blocker) { throw new BridgeChannelQuarantinedError( blocker.reason, - abandonedRestoreRetryAfterSeconds, + blocker.reason.startsWith('new_session_') + ? abandonedNewSessionRetryAfterSeconds + : abandonedRestoreRetryAfterSeconds, ); } }; @@ -2768,12 +2786,21 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { opts.childEnvOverrides ? Object.freeze({ ...opts.childEnvOverrides }) : Object.freeze({}); - const initTimeoutMs = opts.initializeTimeoutMs ?? DEFAULT_INIT_TIMEOUT_MS; - if (initTimeoutMs <= 0) { + const rawInitTimeoutMs = opts.initializeTimeoutMs ?? DEFAULT_INIT_TIMEOUT_MS; + if (!Number.isFinite(rawInitTimeoutMs) || rawInitTimeoutMs <= 0) { + throw new TypeError( + `Invalid initializeTimeoutMs: ${rawInitTimeoutMs}. Must be a finite number > 0.`, + ); + } + const initTimeoutMs = Math.ceil(rawInitTimeoutMs); + if (initTimeoutMs > 2_147_483_647) { throw new TypeError( - `Invalid initializeTimeoutMs: ${initTimeoutMs}. Must be > 0.`, + `Invalid initializeTimeoutMs: ${rawInitTimeoutMs}. Must not exceed the supported timer range.`, ); } + const newSessionSettlementGraceMs = initTimeoutMs; + const abandonedNewSessionRetryAfterSeconds = + restoreRetryAfterSeconds(initTimeoutMs); const sessionRestoreTimeoutMs = resolveSessionRestoreTimeoutMs(opts); // Retry hint for an id fenced behind an abandoned restore. The underlying // ACP request already exceeded the full budget, so the next useful retry is @@ -3175,7 +3202,14 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // precisely the one that cannot answer this round trip inside // `ACTIVE_WORK_CLOSE_TIMEOUT_MS`. Nothing is attached to this session // (`maybeCloseIdleSession` gates on that), so proceed to local teardown. - if (info.isQuarantined || info.restoreSettlementOverdue) return true; + if ( + info.isQuarantined || + info.restoreSettlementOverdue || + info.newSessionCleanupFailed || + info.newSessionSettlementOverdue + ) { + return true; + } try { const response = await withTimeout( entry.connection.extMethod(SERVE_CONTROL_EXT_METHODS.sessionClose, { @@ -3447,7 +3481,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { return ( ci.emptyReapPending || ci.unsettledAbandonedRestores.size > 0 || - ci.isQuarantined + ci.unsettledAbandonedNewSessions.size > 0 || + ci.isQuarantined || + ci.newSessionCleanupFailed ); } @@ -3491,6 +3527,34 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ci.restoreSettlementTimers.set(sessionId, timer); } + function armNewSessionSettlementGrace( + ci: ChannelInfo, + token: symbol, + requestedSessionId: string | undefined, + ): void { + if (ci.newSessionSettlementTimers.has(token)) return; + const timer = setTimeout(() => { + ci.newSessionSettlementTimers.delete(token); + if (!ci.unsettledAbandonedNewSessions.has(token)) return; + if (ci.isDying || !aliveChannels.has(ci)) return; + ci.newSessionSettlementOverdue = true; + writeStderrLine( + `qwen serve: abandoned newSession${requestedSessionId ? ` for ${JSON.stringify(requestedSessionId)}` : ''} has not settled ` + + `${newSessionSettlementGraceMs}ms after its deadline; refusing fresh sessions on channel ${ci.id} until it drains`, + ); + telemetry.event('session.new.settlement_overdue', { + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.session_new.settlement_grace_ms': + newSessionSettlementGraceMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + ...(requestedSessionId ? { 'session.id': requestedSessionId } : {}), + }); + void reapPendingEmptyChannel(ci); + }, newSessionSettlementGraceMs); + timer.unref(); + ci.newSessionSettlementTimers.set(token, timer); + } + async function reapPendingEmptyChannel(ci: ChannelInfo): Promise { if (!channelShouldReapWhenIdle(ci) || !hasNoChannelWork(ci)) return; ci.emptyReapPending = false; @@ -3807,6 +3871,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // simultaneous calls don't collide while still being awaitable from // `shutdown()`. const inFlightSpawns = new Map>(); + const abandonedNewSessionSettlements = new Set>(); // Reserves caller-supplied ids before `doSpawn` reaches its first await. // Restore admission checks the same set, closing the opposite race from // `inFlightRestores`: whichever operation reserves the id first owns its @@ -4182,6 +4247,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { unsettledAbandonedRestores: new Set(), restoreSettlementOverdue: false, restoreSettlementTimers: new Map(), + unsettledAbandonedNewSessions: new Set(), + newSessionSettlementOverdue: false, + newSessionSettlementTimers: new Map(), + newSessionCleanupFailed: false, transportFailed: false, transportFailureInitiatedTeardown: false, isDying: false, @@ -4258,6 +4327,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { clearTimeout(timer); } info.restoreSettlementTimers.clear(); + for (const timer of info.newSessionSettlementTimers.values()) { + clearTimeout(timer); + } + info.newSessionSettlementTimers.clear(); aliveChannels.delete(info); if (channelInfo === info) channelInfo = undefined; const sessions = Array.from(info.sessionIds); @@ -4539,6 +4612,120 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } } + async function settleAbandonedNewSession( + ci: ChannelInfo, + token: symbol, + lateSessionId: string | undefined, + requestedSessionId: string | undefined, + ): Promise { + telemetry.event('session.new.late_result', { + 'qwen-code.daemon.session_new.result': lateSessionId + ? 'success' + : 'failure', + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + ...(lateSessionId + ? { 'session.id': lateSessionId } + : requestedSessionId + ? { 'session.id': requestedSessionId } + : {}), + }); + try { + if (!lateSessionId) return; + if (byId.has(lateSessionId)) { + writeStderrLine( + `qwen serve: skipping abandoned newSession cleanup for ${JSON.stringify(lateSessionId)}: the id is owned by a live session`, + ); + telemetry.event('session.new.cleanup', { + 'qwen-code.daemon.session_new.cleanup_result': 'id_reclaimed', + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + 'session.id': lateSessionId, + }); + return; + } + if (ci.isDying || !aliveChannels.has(ci)) { + await ci.channel.exited; + telemetry.event('session.new.cleanup', { + 'qwen-code.daemon.session_new.cleanup_result': 'transport_closed', + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + 'session.id': lateSessionId, + }); + return; + } + try { + const closeResult = await Promise.race([ + withTimeout( + ci.connection.extMethod(SERVE_CONTROL_EXT_METHODS.sessionClose, { + sessionId: lateSessionId, + drainTimeoutMs: sessionCloseDrainBudgetMs(initTimeoutMs), + }), + initTimeoutMs, + 'abandonedNewSessionClose', + ), + getChannelClosedReject(ci), + ]); + if (!isRecord(closeResult) || closeResult['closed'] !== true) { + throw new Error('ACP child refused abandoned newSession cleanup'); + } + telemetry.event('session.new.cleanup', { + 'qwen-code.daemon.session_new.cleanup_result': 'closed', + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + 'session.id': lateSessionId, + }); + } catch (error) { + if (isAcpSessionResourceNotFound(error, lateSessionId)) { + telemetry.event('session.new.cleanup', { + 'qwen-code.daemon.session_new.cleanup_result': 'not_found', + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + 'session.id': lateSessionId, + }); + return; + } + if (ci.isDying || !aliveChannels.has(ci)) { + await ci.channel.exited; + telemetry.event('session.new.cleanup', { + 'qwen-code.daemon.session_new.cleanup_result': 'transport_closed', + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + 'session.id': lateSessionId, + }); + return; + } + ci.newSessionCleanupFailed = true; + writeStderrLine( + `qwen serve: quarantining ACP channel after timed-out newSession cleanup failed for ${JSON.stringify(lateSessionId)}: ${extractErrorMessage(error)}`, + ); + telemetry.event('session.new.cleanup', { + 'qwen-code.daemon.session_new.cleanup_result': 'quarantined', + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + 'session.id': lateSessionId, + }); + if (hasNoChannelWork(ci)) { + void killChannelWithLog(ci, 'abandoned newSession cleanup'); + } + await ci.channel.exited; + } finally { + ci.client.markSessionClosed(lateSessionId); + } + } finally { + ci.unsettledAbandonedNewSessions.delete(token); + const graceTimer = ci.newSessionSettlementTimers.get(token); + if (graceTimer !== undefined) { + clearTimeout(graceTimer); + ci.newSessionSettlementTimers.delete(token); + } + if (ci.unsettledAbandonedNewSessions.size === 0) { + ci.newSessionSettlementOverdue = false; + } + void reapPendingEmptyChannel(ci); + } + } + async function doSpawn( modelServiceId: string | undefined, effectiveScope: 'single' | 'thread', @@ -4553,6 +4740,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { requestedSessionId?: string, daemonOwnedStandaloneCreation = false, onNewSessionDispatch?: () => void, + onNewSessionAbandoned?: (settlement: Promise) => void, ): Promise { // Get-or-create the daemon's single channel, then call // `connection.newSession()` on it. Sessions share the child's @@ -4598,6 +4786,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { let sessionRegistered = false; let sessionRemovedDuringInitialization = false; let initializedSessionId: string | undefined; + const abandonedToken = Symbol(requestedSessionId ?? 'newSession'); let newSessionResp: { sessionId: string; models?: { currentModelId?: unknown } | null; @@ -4619,22 +4808,20 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const request = telemetry.injectPromptContext({ cwd: boundWorkspace, mcpServers: [], - ...(requestedSessionId || sourceType - ? { - _meta: { - ...sessionSourceRequestMeta( - sourceType, - sourceId, - daemonOwnedStandaloneCreation, - ), - ...(requestedSessionId - ? { - [REQUESTED_SESSION_ID_META_KEY]: requestedSessionId, - } - : {}), - }, - } - : {}), + _meta: { + ...sessionSourceRequestMeta( + sourceType, + sourceId, + daemonOwnedStandaloneCreation, + ), + ...(requestedSessionId + ? { + [REQUESTED_SESSION_ID_META_KEY]: requestedSessionId, + } + : {}), + [SESSION_INITIALIZATION_DEADLINE_META_KEY]: + Date.now() + initTimeoutMs, + }, }); const newSessionRequest = worktree ? { @@ -4646,13 +4833,95 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } : request; onNewSessionDispatch?.(); - const response = await withTimeout( - Promise.race([ - ci.connection.newSession(newSessionRequest), - channelUnavailableReject(ci.channel, 'during newSession'), - ]), - initTimeoutMs, - 'newSession', + const rawNewSession = Promise.race([ + ci.connection.newSession(newSessionRequest), + channelUnavailableReject(ci.channel, 'during newSession'), + ]); + const lifecycle: { + phase: 'active' | 'abandoned'; + resolveSettlement?: () => void; + } = { phase: 'active' }; + const response = await new Promise>( + (resolve, reject) => { + const timer = setTimeout(() => { + if (lifecycle.phase !== 'active') return; + lifecycle.phase = 'abandoned'; + ci.unsettledAbandonedNewSessions.add(abandonedToken); + const settlement = new Promise((resolveSettlement) => { + lifecycle.resolveSettlement = resolveSettlement; + }); + abandonedNewSessionSettlements.add(settlement); + void settlement.finally(() => { + abandonedNewSessionSettlements.delete(settlement); + }); + onNewSessionAbandoned?.(settlement); + const channelWasEmpty = hasNoChannelWork(ci, { + ignoreCurrentSessionSpawn: true, + }); + telemetry.event('session.new.public_result', { + 'qwen-code.daemon.session_new.result': 'timeout', + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + 'qwen-code.daemon.session_new.channel_was_empty': + channelWasEmpty, + ...(requestedSessionId + ? { 'session.id': requestedSessionId } + : {}), + }); + writeStderrLine( + `qwen serve: newSession timed out after ${initTimeoutMs}ms${requestedSessionId ? ` for ${JSON.stringify(requestedSessionId)}` : ''} on channel ${ci.id}; decision=${channelWasEmpty ? 'kill_empty' : 'fence_shared'}`, + ); + if (!channelWasEmpty) { + armNewSessionSettlementGrace( + ci, + abandonedToken, + requestedSessionId, + ); + } + reject(new BridgeTimeoutError('newSession', initTimeoutMs)); + }, initTimeoutMs); + timer.unref(); + + void rawNewSession.then( + (value) => { + if (lifecycle.phase === 'active') { + clearTimeout(timer); + resolve(value); + return; + } + void settleAbandonedNewSession( + ci, + abandonedToken, + value.sessionId, + requestedSessionId, + ).then( + () => lifecycle.resolveSettlement?.(), + () => lifecycle.resolveSettlement?.(), + ); + }, + (error: unknown) => { + if (lifecycle.phase === 'active') { + clearTimeout(timer); + reject( + extractJsonRpcErrorField(error, 'errorKind') === + SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND + ? new BridgeTimeoutError('newSession', initTimeoutMs) + : error, + ); + return; + } + void settleAbandonedNewSession( + ci, + abandonedToken, + undefined, + requestedSessionId, + ).then( + () => lifecycle.resolveSettlement?.(), + () => lifecycle.resolveSettlement?.(), + ); + }, + ); + }, ); telemetry.event('session.new.completed', { 'session.id': response.sessionId, @@ -7054,7 +7323,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { assertFreshSessionsAvailable(); if ( - byId.size + inFlightSpawns.size + inFlightRestores.size >= + byId.size + + inFlightSpawns.size + + inFlightRestores.size + + abandonedNewSessionSettlements.size >= maxSessions ) { throw new SessionLimitExceededError(maxSessions); @@ -8560,7 +8832,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // returned above bypass this — only NEW children are gated. assertFreshSessionsAvailable(); if ( - byId.size + inFlightSpawns.size + inFlightRestores.size >= + byId.size + + inFlightSpawns.size + + inFlightRestores.size + + abandonedNewSessionSettlements.size >= maxSessions ) { throw new SessionLimitExceededError(maxSessions); @@ -8604,6 +8879,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { admissionReleased = true; releaseFreshSessionReservation(admission); }; + let abandonedSettlement: Promise | undefined; const promise = doSpawn( req.modelServiceId, effectiveScope, @@ -8622,6 +8898,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { trustedStandaloneSpawn.dispatched = true; } : undefined, + (settlement) => { + abandonedSettlement = settlement; + }, ); // Track in-flight spawns regardless of scope. Under `single` // this also serves the coalescing path above (a parallel @@ -8643,13 +8922,26 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { try { return await promise; } finally { - releaseAdmissionOnce(); + if (abandonedSettlement) { + void abandonedSettlement.then( + () => { + releaseAdmissionOnce(); + releaseRequestedSessionRegistration(); + }, + () => { + releaseAdmissionOnce(); + releaseRequestedSessionRegistration(); + }, + ); + } else { + releaseAdmissionOnce(); + releaseRequestedSessionRegistration(); + } // Always clear the in-flight slot whether the spawn resolved // or rejected — leaving a rejected promise behind would // poison every future coalescing-path call for this // workspace (single-scope) or grow unbounded (thread-scope). inFlightSpawns.delete(tracker); - releaseRequestedSessionRegistration(); } }, @@ -9721,7 +10013,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { let admission: ReturnType | undefined; if (restoreBranch) { if ( - byId.size + inFlightSpawns.size + inFlightRestores.size >= + byId.size + + inFlightSpawns.size + + inFlightRestores.size + + abandonedNewSessionSettlements.size >= maxSessions ) { throw new SessionLimitExceededError(maxSessions); @@ -13224,6 +13519,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { () => undefined, ), ); + const abandonedNewSessionAwaits = Array.from( + abandonedNewSessionSettlements, + ); const inFlightChannelAwait: Promise = inFlightChannelSpawn ? inFlightChannelSpawn.then( () => undefined, @@ -13235,6 +13533,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ...[...byId.values()].map((entry) => entry.attachments.close()), ...inFlightSessionAwaits, ...inFlightRestoreAwaits, + ...abandonedNewSessionAwaits, inFlightChannelAwait, ]); const teardownFailures = teardownResults.flatMap((result) => diff --git a/packages/acp-bridge/src/bridgeErrors.ts b/packages/acp-bridge/src/bridgeErrors.ts index 2286c2790c0..8d545e60dca 100644 --- a/packages/acp-bridge/src/bridgeErrors.ts +++ b/packages/acp-bridge/src/bridgeErrors.ts @@ -631,16 +631,16 @@ export class WorkspaceDrainingError extends Error { } /** - * Why a channel is closed to new session work. `restore_cleanup_failed`: the - * cleanup of a timed-out restore failed, so the child's state is unknown. - * `restore_settlement_overdue`: an abandoned restore blew past its settlement - * grace period, so the child still holds work the bridge can neither cancel - * nor account for. Both keep existing sessions usable and clear once the - * channel drains and is recycled. + * Why a channel is closed to new session work. Cleanup failures mean the + * child's state is unknown; settlement-overdue states mean the child still + * holds work the bridge can neither cancel nor account for. Existing sessions + * remain usable while the channel drains and is recycled. */ export type BridgeChannelUnavailableReason = | 'restore_cleanup_failed' - | 'restore_settlement_overdue'; + | 'restore_settlement_overdue' + | 'new_session_cleanup_failed' + | 'new_session_settlement_overdue'; export class BridgeChannelQuarantinedError extends Error { readonly reason: BridgeChannelUnavailableReason; @@ -659,7 +659,11 @@ export class BridgeChannelQuarantinedError extends Error { super( reason === 'restore_settlement_overdue' ? 'The ACP channel is unavailable for new sessions while an abandoned session restore has not settled' - : 'The ACP channel is unavailable for new sessions while timed-out restore cleanup is pending', + : reason === 'new_session_settlement_overdue' + ? 'The ACP channel is unavailable for new sessions while an abandoned session initialization has not settled' + : reason === 'new_session_cleanup_failed' + ? 'The ACP channel is unavailable for new sessions while timed-out session initialization cleanup is pending' + : 'The ACP channel is unavailable for new sessions while timed-out restore cleanup is pending', ); this.name = 'BridgeChannelQuarantinedError'; this.reason = reason; diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index 5a8de489d35..3729556dc01 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -267,6 +267,10 @@ export const LOAD_REPLAY_MAX_BYTES = 32 * 1024 * 1024; export const LOAD_REPLAY_MAX_UPDATES = 10_000; export const REQUESTED_SESSION_ID_META_KEY = 'qwen-code/sessionId'; +export const SESSION_INITIALIZATION_DEADLINE_META_KEY = + 'qwen.daemon.sessionInitializationDeadlineMs'; +export const SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND = + 'session_initialization_timeout'; export const CHANNEL_STARTUP_PROFILE_META_KEY = 'qwen.daemon.channelStartupProfile'; diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 049212e7422..8b17978d351 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -1050,6 +1050,8 @@ import { CHANNEL_STARTUP_PROFILE_META_KEY, CHANNEL_STARTUP_PROFILE_VERSION, PROMPT_CANCEL_METHOD, + SESSION_INITIALIZATION_DEADLINE_META_KEY, + SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND, TODO_STOP_GUARD_QUEUE_RELEASE_METHOD, WORKTREE_MCP_DEFER_META_KEY, } from '@qwen-code/acp-bridge/bridgeTypes'; @@ -2313,6 +2315,66 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); + it('aborts trusted session initialization at the bridge deadline', async () => { + const innerConfig = await setupSessionMocks('deadline-session'); + vi.mocked(innerConfig.initialize).mockImplementationOnce( + async (options) => { + const signal = options?.signal; + expect(signal).toBeInstanceOf(AbortSignal); + await new Promise((resolve, reject) => { + if (signal?.aborted) { + reject(signal.reason); + return; + } + signal?.addEventListener('abort', () => reject(signal.reason), { + once: true, + }); + }); + }, + ); + const { agent, agentPromise } = await bootInitializedAcpAgent( + makeSessionSettings(), + 'expected-capability', + ); + + await expect( + agent.newSession({ + cwd: '/tmp', + mcpServers: [], + _meta: { + [SESSION_INITIALIZATION_DEADLINE_META_KEY]: Date.now() + 20, + }, + }), + ).rejects.toMatchObject({ + code: -32603, + data: { errorKind: SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND }, + }); + expect(vi.mocked(Session)).not.toHaveBeenCalled(); + + mockConnectionState.resolve(); + await agentPromise; + }); + + it('ignores a forged session initialization deadline from an untrusted parent', async () => { + await setupSessionMocks('untrusted-deadline-session'); + const { agent, agentPromise } = await bootInitializedAcpAgent( + makeSessionSettings(), + ); + + await expect( + agent.newSession({ + cwd: '/tmp', + mcpServers: [], + _meta: { + [SESSION_INITIALIZATION_DEADLINE_META_KEY]: Date.now() - 1, + }, + }), + ).resolves.toMatchObject({ sessionId: 'untrusted-deadline-session' }); + + mockConnectionState.resolve(); + await agentPromise; + }); + it('strips channel classification from untrusted callers', async () => { // `qwen.channel.prompt` marks a turn as a channel turn, opting it out // of loop-detected rejection and the repeated-failure guard, and diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 821df1920fb..c2429e1dccb 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -398,6 +398,8 @@ import { LOAD_REPLAY_VERSION, PROMPT_CANCEL_METHOD, REQUESTED_SESSION_ID_META_KEY, + SESSION_INITIALIZATION_DEADLINE_META_KEY, + SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND, TODO_STOP_GUARD_QUEUE_RELEASE_METHOD, isValidTrustedModelPrompt, WORKTREE_MCP_DEFER_META_KEY, @@ -4685,6 +4687,49 @@ class QwenAgent implements Agent { }; } + private createSessionInitializationDeadline(raw: unknown): + | { + signal: AbortSignal; + dispose: () => void; + } + | undefined { + if (raw === undefined || !this.isTrustedManagedParent()) return undefined; + if (typeof raw !== 'number' || !Number.isSafeInteger(raw) || raw <= 0) { + throw RequestError.invalidParams( + { errorKind: 'invalid_session_initialization_deadline' }, + `\`_meta["${SESSION_INITIALIZATION_DEADLINE_META_KEY}"]\` must be a positive safe integer`, + ); + } + + const remainingMs = raw - Date.now(); + if (remainingMs > 2_147_483_647) { + throw RequestError.invalidParams( + { errorKind: 'invalid_session_initialization_deadline' }, + `\`_meta["${SESSION_INITIALIZATION_DEADLINE_META_KEY}"]\` exceeds the supported timer range`, + ); + } + + const controller = new AbortController(); + const timeoutError = new RequestError( + ACP_ERROR_CODES.INTERNAL_ERROR, + 'Session initialization deadline exceeded', + { errorKind: SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND }, + ); + let timer: NodeJS.Timeout | undefined; + if (remainingMs <= 0) { + controller.abort(timeoutError); + } else { + timer = setTimeout(() => controller.abort(timeoutError), remainingMs); + timer.unref(); + } + return { + signal: controller.signal, + dispose: () => { + if (timer) clearTimeout(timer); + }, + }; + } + async newSession(params: NewSessionRequest): Promise { const { cwd, mcpServers } = params; const parsedSessionId = parseCallerSuppliedSessionId( @@ -4702,7 +4747,14 @@ class QwenAgent implements Agent { const releaseStartingSessionId = requestedSessionId ? this.reserveStartingSessionId(requestedSessionId) : undefined; + let initializationDeadline: + | { signal: AbortSignal; dispose: () => void } + | undefined; try { + initializationDeadline = this.createSessionInitializationDeadline( + params._meta?.[SESSION_INITIALIZATION_DEADLINE_META_KEY], + ); + initializationDeadline?.signal.throwIfAborted(); const sessionSource = getSessionSource(params); const provisionalStandalone = isReservedStandaloneSessionSourceType( sessionSource?.sourceType, @@ -4734,6 +4786,7 @@ class QwenAgent implements Agent { loadSettingsCached(cwd), ); this.settings = settings; + const deferMcpDiscovery = shouldDeferMcpDiscovery(params); const config = await profiler.time('config_setup', () => this.newSessionConfig( cwd, @@ -4742,17 +4795,24 @@ class QwenAgent implements Agent { sessionSource, requestedSessionId, undefined, - shouldDeferMcpDiscovery(params) - ? { skipMcpDiscovery: true } + initializationDeadline || deferMcpDiscovery + ? { + ...(initializationDeadline + ? { signal: initializationDeadline.signal } + : {}), + ...(deferMcpDiscovery ? { skipMcpDiscovery: true } : {}), + } : undefined, ), ); let session: Session; try { + initializationDeadline?.signal.throwIfAborted(); if (!provisionalStandalone) { await profiler.time('auth', () => this.ensureAuthenticated(config), ); + initializationDeadline?.signal.throwIfAborted(); profiler.timeSync('file_system_setup', () => this.setupFileSystem(config), ); @@ -4760,6 +4820,9 @@ class QwenAgent implements Agent { session = await profiler.time('session_register', () => this.createAndStoreSession(config, settings, undefined, { deferWorkspaceActivation: provisionalStandalone, + ...(initializationDeadline + ? { signal: initializationDeadline.signal } + : {}), }), ); } catch (error) { @@ -4784,6 +4847,7 @@ class QwenAgent implements Agent { parentContext ? { parentContext } : {}, ); } finally { + initializationDeadline?.dispose(); releaseStartingSessionId?.(); } } @@ -12709,16 +12773,19 @@ class QwenAgent implements Agent { beforeSessionCreate?: () => void; primeSession?: (session: Session) => void; beforeStartPostReplayServices?: (session: Session) => Promise; + signal?: AbortSignal; } = {}, ): Promise { + options.signal?.throwIfAborted(); this.assertManagedSessionAdmission(); const sessionId = normalizeSessionIdForLookup(config.getSessionId()); const llmClient = config.getLlmClient(); const needsInitialize = !llmClient.isInitialized(); if (needsInitialize && options.deferWorkspaceActivation !== true) { - await llmClient.initialize(); + await llmClient.initialize(undefined, options.signal); } + options.signal?.throwIfAborted(); this.assertManagedSessionAdmission(); if (this.sessions.has(sessionId)) { @@ -12730,6 +12797,7 @@ class QwenAgent implements Agent { } await options.prepareBeforeSessionCreate?.(); + options.signal?.throwIfAborted(); this.assertManagedSessionAdmission(); if (this.sessions.has(sessionId)) { throw new RequestError( @@ -12807,6 +12875,7 @@ class QwenAgent implements Agent { // the session is published: the permission check is async, and the tool // must be declared before the first prompt can be served. await registerCreateSubSessionTool(config); + options.signal?.throwIfAborted(); options.primeSession?.(session); if (options.deferWorkspaceActivation !== true) { config.hydrateSessionRestoreFileHistory?.(); diff --git a/packages/cli/src/serve/acp-http/dispatch-error.test.ts b/packages/cli/src/serve/acp-http/dispatch-error.test.ts index 3289009d7e3..6cbd2ef4fe0 100644 --- a/packages/cli/src/serve/acp-http/dispatch-error.test.ts +++ b/packages/cli/src/serve/acp-http/dispatch-error.test.ts @@ -110,21 +110,24 @@ describe('toRpcError', () => { }); }); - it('carries the quarantine backoff hint for a settlement-overdue channel', () => { + it('carries every quarantine reason and its backoff hint', () => { // Quarantine outlives the fence, and a fresh-id request never reaches the // 409 that carries the real hint — so this payload is the only backoff // signal such a caller gets. - const error = new BridgeChannelQuarantinedError( + for (const reason of [ 'restore_settlement_overdue', - 90, - ); - expect(toRpcError(error)).toMatchObject({ - data: { - reason: 'restore_settlement_overdue', - retryAfterSeconds: 90, - httpStatus: 503, - }, - }); + 'new_session_cleanup_failed', + 'new_session_settlement_overdue', + ] as const) { + const error = new BridgeChannelQuarantinedError(reason, 90); + expect(toRpcError(error)).toMatchObject({ + data: { + reason, + retryAfterSeconds: 90, + httpStatus: 503, + }, + }); + } }); it('maps invalid session metadata to the REST-equivalent invalid_metadata contract', () => { diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index c81b09d3a51..34251953839 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -13916,32 +13916,35 @@ describe('createServeApp', () => { }); }); - it('503s fresh session work while restore cleanup is quarantined', async () => { - const bridge = fakeBridge({ - resumeImpl: async () => { - throw new BridgeChannelQuarantinedError( - 'restore_settlement_overdue', - 90, - ); - }, - }); - const app = createServeApp(baseOpts, undefined, { bridge }); - const res = await request(app) - .post('/session/persisted-quarantined/resume') - .set('Host', `127.0.0.1:${baseOpts.port}`) - .send({}); + it('503s fresh session work for every channel quarantine reason', async () => { + for (const reason of [ + 'restore_settlement_overdue', + 'new_session_cleanup_failed', + 'new_session_settlement_overdue', + ] as const) { + const bridge = fakeBridge({ + resumeImpl: async () => { + throw new BridgeChannelQuarantinedError(reason, 90); + }, + }); + const app = createServeApp(baseOpts, undefined, { bridge }); + const res = await request(app) + .post('/session/persisted-quarantined/resume') + .set('Host', `127.0.0.1:${baseOpts.port}`) + .send({}); - expect(res.status).toBe(503); - // Quarantine lasts until the channel drains — strictly longer than the - // fence — and a fresh-id caller never sees the 409 that would tell it so. - expect(res.headers['retry-after']).toBe('90'); - expect(res.body).toMatchObject({ - code: 'acp_channel_unavailable', - errorKind: 'acp_channel_unavailable', - retryable: true, - reason: 'restore_settlement_overdue', - retryAfterSeconds: 90, - }); + expect(res.status).toBe(503); + // Quarantine lasts until the channel drains — strictly longer than the + // fence — and a fresh-id caller never sees the 409 that would tell it so. + expect(res.headers['retry-after']).toBe('90'); + expect(res.body).toMatchObject({ + code: 'acp_channel_unavailable', + errorKind: 'acp_channel_unavailable', + retryable: true, + reason, + retryAfterSeconds: 90, + }); + } }); it('400 workspace_mismatch before touching the bridge for non-primary cwd', async () => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 22d83257d1b..c4b508c0851 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -1618,6 +1618,8 @@ function readMemoryPressureRatioEnv(envName: string, fallback: number): number { * Options for Config.initialize() */ export interface ConfigInitializeOptions { + /** Cancels request-scoped initialization without becoming a session signal. */ + signal?: AbortSignal; /** * Callback for sending MCP messages to SDK servers via control plane. * Required for SDK MCP server support in SDK mode. @@ -3024,6 +3026,7 @@ export class Config { if (this.shutdownRequested) { throw Error('Config is shutting down'); } + options?.signal?.throwIfAborted(); this.initialized = true; const initialization = this.initializeOnce(options); this.initializationPromise = initialization; @@ -3076,6 +3079,7 @@ export class Config { this.sessionWriterActivationPromise = undefined; } } + options?.signal?.throwIfAborted(); registerSessionProjectDir(this.sessionId, this.storage.getProjectDir()); this.sessionProjectDirRegistered = true; await this.initializeInternal(options); @@ -3107,6 +3111,7 @@ export class Config { ): Promise { this.debugLogger.info('Config initialization started'); await this.proxyDispatcherReady; + options?.signal?.throwIfAborted(); if (options?.skipFileCheckpointing === true) { this.fileCheckpointingEnabled = false; this.fileHistoryService = undefined; @@ -3135,6 +3140,7 @@ export class Config { }); } recordStartupEvent('config_initialize_extensions_initial_end'); + options?.signal?.throwIfAborted(); this.debugLogger.debug('Extension manager initialized'); // Bare mode and read-only replay helpers skip all hook loading and execution. @@ -3391,6 +3397,7 @@ export class Config { this.debugLogger.debug('Hook system disabled, skipping initialization'); } recordStartupEvent('config_initialize_hooks_end'); + options?.signal?.throwIfAborted(); this.subagentManager = new SubagentManager(this); recordStartupEvent('config_initialize_skills_start'); @@ -3427,6 +3434,7 @@ export class Config { this.debugLogger.debug('Skill manager skipped'); } recordStartupEvent('config_initialize_skills_end'); + options?.signal?.throwIfAborted(); this.memoryPressureConfig = loadMemoryPressureConfig(); this.memoryPressureMonitor = new MemoryPressureMonitor( @@ -3448,6 +3456,7 @@ export class Config { await this.extensionManager.refreshCache(); } recordStartupEvent('config_initialize_extensions_final_end'); + options?.signal?.throwIfAborted(); if (!this.provisionalWorkspace) { recordStartupEvent('config_initialize_hierarchical_memory_start'); @@ -3455,6 +3464,7 @@ export class Config { recordStartupEvent('config_initialize_hierarchical_memory_end'); this.debugLogger.debug('Hierarchical memory loaded'); } + options?.signal?.throwIfAborted(); // Progressive MCP availability: skip MCP discovery in the synchronous // tool-registry construction path and kick it off in the background @@ -3481,6 +3491,7 @@ export class Config { options?.sendSdkMcpMessage, skipInlineMcpDiscovery ? { skipDiscovery: true } : undefined, ); + options?.signal?.throwIfAborted(); recordStartupEvent('config_initialize_tool_registry_end'); recordStartupEvent('tool_registry_created', { toolCount: this.toolRegistry.getAllToolNames().length, @@ -3494,7 +3505,7 @@ export class Config { !(options?.skipLlmInitialization ?? options?.skipGeminiInitialization) && !this.provisionalWorkspace ) { - await this.llmClient.initialize(); + await this.llmClient.initialize(undefined, options?.signal); this.debugLogger.info('LLM client initialized'); } else { this.debugLogger.info('LLM client initialization skipped'); @@ -3513,6 +3524,7 @@ export class Config { await this.toolRegistry.warmAll({ strict: options?.lenientToolWarmup !== true, }); + options?.signal?.throwIfAborted(); recordStartupEvent('config_initialize_tool_warmup_end'); } @@ -3550,6 +3562,7 @@ export class Config { } if (!this.provisionalWorkspace) { + options?.signal?.throwIfAborted(); logStartSession(this, new StartSessionEvent(this)); } this.debugLogger.info('Config initialization completed'); diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 9823cae252c..e2980c12813 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -1102,6 +1102,35 @@ describe('Gemini Client (client.ts)', () => { 'SessionStart hook failed: Error: hook failed', ); }); + + it('passes cancellation to SessionStart hooks and does not swallow it', async () => { + const controller = new AbortController(); + const timeoutError = new Error('session initialization timed out'); + const fireSessionStartEvent = vi.fn(async (...args: unknown[]) => { + expect(args[4]).toBe(controller.signal); + controller.abort(timeoutError); + throw timeoutError; + }); + vi.mocked(mockConfig.getDisableAllHooks).mockReturnValue(false); + vi.mocked(mockConfig.hasHooksForEvent).mockReturnValue(true); + vi.mocked(mockConfig.getHookSystem).mockReturnValue({ + fireSessionStartEvent, + } as unknown as ReturnType); + + await expect( + client['fireSessionStartHook']( + SessionStartSource.Startup, + controller.signal, + ), + ).rejects.toBe(timeoutError); + expect(fireSessionStartEvent).toHaveBeenCalledWith( + SessionStartSource.Startup, + 'test-model', + PermissionMode.Default, + undefined, + controller.signal, + ); + }); }); describe('startChat — session start profiling', () => { diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index f41e1694079..cb40d3793f6 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -495,7 +495,11 @@ export class LlmClient { this.loopDetector = new LoopDetectionService(config); } - async initialize(sessionStartSource?: SessionStartSource) { + async initialize( + sessionStartSource?: SessionStartSource, + signal?: AbortSignal, + ) { + signal?.throwIfAborted(); const sessionId = this.config.getSessionId(); this.lastPromptId = sessionId; @@ -516,6 +520,7 @@ export class LlmClient { await this.startChat( restoreRuntime.apiHistory, sessionStartSource ?? SessionStartSource.Resume, + signal, ); this.restoreLoadedSkillsFromHistory(restoreRuntime.apiHistory); const chat = this.getChat(); @@ -547,6 +552,7 @@ export class LlmClient { await this.startChat( resumedHistory, sessionStartSource ?? SessionStartSource.Resume, + signal, ); this.restoreLoadedSkillsFromHistory(resumedHistory); const chat = this.getChat(); @@ -566,12 +572,13 @@ export class LlmClient { this.restoreAttributionFromSession(resumedSessionData.conversation); } else { if (sessionStartSource !== undefined) { - await this.startChat(undefined, sessionStartSource); + await this.startChat(undefined, sessionStartSource, signal); } else { - await this.startChat(); + await this.startChat(undefined, undefined, signal); } } + signal?.throwIfAborted(); this.initializedSessionId = sessionId; // Clean up stale tool result files from previous sessions (fire-and-forget) @@ -1973,6 +1980,7 @@ export class LlmClient { private async fireSessionStartHook( source: SessionStartSource, + signal?: AbortSignal, ): Promise { const hookSystem = this.config.getHookSystem(); if ( @@ -1984,13 +1992,23 @@ export class LlmClient { } try { - const output = await hookSystem.fireSessionStartEvent( - source, - this.config.getModel() ?? '', - this.toPermissionMode(this.config.getApprovalMode()), - ); + const output = signal + ? await hookSystem.fireSessionStartEvent( + source, + this.config.getModel() ?? '', + this.toPermissionMode(this.config.getApprovalMode()), + undefined, + signal, + ) + : await hookSystem.fireSessionStartEvent( + source, + this.config.getModel() ?? '', + this.toPermissionMode(this.config.getApprovalMode()), + ); + signal?.throwIfAborted(); return output?.getAdditionalContext()?.trim() || undefined; } catch (err) { + signal?.throwIfAborted(); this.config.getDebugLogger().warn(`SessionStart hook failed: ${err}`); return undefined; } @@ -2001,7 +2019,9 @@ export class LlmClient { sessionStartSource = extraHistory ? SessionStartSource.Resume : SessionStartSource.Startup, + signal?: AbortSignal, ): Promise { + signal?.throwIfAborted(); this.forceFullIdeContext = true; this.lastInjectedDate = undefined; // Clear stale cache params on session reset to prevent cross-session leakage @@ -2114,7 +2134,7 @@ export class LlmClient { const sessionStartAdditionalContext = await profiler.time( 'session_start_hook', - () => this.fireSessionStartHook(sessionStartSource), + () => this.fireSessionStartHook(sessionStartSource, signal), ); this.lastSessionStartContext = sessionStartAdditionalContext; this.lastSessionStartSource = sessionStartAdditionalContext @@ -2135,11 +2155,13 @@ export class LlmClient { await profiler.time('set_tools', () => this.setTools({ skipHistoryReveal: true }), ); + signal?.throwIfAborted(); finishProfile(true); return this.chat; } catch (error) { finishProfile(false); + signal?.throwIfAborted(); await reportError( error, 'Error initializing chat session.', From dbdeeda9e1702cb3b18fb052cfca96c4109e77d3 Mon Sep 17 00:00:00 2001 From: "jinye.djy" Date: Thu, 27 Aug 2026 21:22:43 +0800 Subject: [PATCH 2/6] codex: address PR review feedback (#10268) Co-authored-by: Qwen-Coder --- .../acp-session-initialization-deadline.md | 2 +- docs/developers/daemon/18-error-taxonomy.md | 2 +- docs/developers/qwen-serve-protocol.md | 4 +- packages/acp-bridge/src/bridge.test.ts | 344 ++++++++++++++++-- packages/acp-bridge/src/bridge.ts | 131 ++++--- packages/acp-bridge/src/bridgeErrors.ts | 18 +- .../cli/src/acp-integration/acpAgent.test.ts | 35 ++ .../src/serve/acp-http/dispatch-error.test.ts | 6 +- packages/cli/src/serve/server.test.ts | 5 +- packages/core/src/config/config.test.ts | 59 +++ packages/core/src/core/client.test.ts | 3 +- 11 files changed, 510 insertions(+), 99 deletions(-) diff --git a/docs/design/acp-session-initialization-deadline.md b/docs/design/acp-session-initialization-deadline.md index c082fc7504b..ff085e507bd 100644 --- a/docs/design/acp-session-initialization-deadline.md +++ b/docs/design/acp-session-initialization-deadline.md @@ -50,7 +50,7 @@ The Bridge observes the raw ACP request after its public timer fires. This prote - A late success is never registered. The Bridge sends one bounded `qwen/control/session/close` for the returned Session ID, and only `closed: true` is accepted as proof that cleanup completed. - Resource-not-found means cleanup is already complete. - A close failure quarantines only fresh session admission on that channel. Existing sibling Sessions continue until they drain, after which the channel is reaped. -- If the raw request remains unsettled for one additional initialization budget, the channel similarly refuses fresh Sessions until it drains. +- If the raw request remains unsettled for one additional initialization budget, the channel similarly refuses fresh Sessions until the request settles or the channel drains. - An empty timed-out channel follows the existing immediate teardown path; a shared channel is not killed while siblings remain. The Bridge holds the fresh-session admission reservation and a caller-supplied ID reservation until the raw request and cleanup settle. Abandoned requests count toward `maxSessions`, and shutdown awaits their settlement after initiating channel teardown. This prevents retries from overcommitting resources or reclaiming an ID that a late child response can still create. diff --git a/docs/developers/daemon/18-error-taxonomy.md b/docs/developers/daemon/18-error-taxonomy.md index 420a4b9ff99..9ad4fadb40f 100644 --- a/docs/developers/daemon/18-error-taxonomy.md +++ b/docs/developers/daemon/18-error-taxonomy.md @@ -59,7 +59,7 @@ Typed classes thrown by the bridge / mediator. Most carry an HTTP status via the | `BridgeChannelClosedError` | 503 | ACP child channel closed mid-call. | Reconnect / retry; check `session_died` for cause. | | `BridgeTimeoutError` | 504 | Bridge-level wallclock exceeded. | Retry; investigate underlying slowness. | | `SessionRestoreTimeoutError` | 504 | ACP session load/resume exceeded its dedicated restore budget. | Retry after the advertised delay; inspect restore stage traces before raising the budget. | -| `BridgeChannelQuarantinedError` | 503 | `reason` is `restore_cleanup_failed`, `restore_settlement_overdue`, `new_session_cleanup_failed`, or `new_session_settlement_overdue`; fresh session work is refused until the workspace channel drains. The 503 body also carries `retryAfterSeconds`. Reasons mark cleanup uncertainty or overdue settlement. | Keep using existing sessions, wait for the channel to recycle, then retry fresh session work. | +| `BridgeChannelQuarantinedError` | 503 | `reason` is `restore_cleanup_failed`, `restore_settlement_overdue`, `new_session_cleanup_failed`, or `new_session_settlement_overdue`. Cleanup-failed states last until the workspace channel drains; settlement-overdue states clear when the abandoned request settles or the channel drains. The 503 body also carries `retryAfterSeconds`. | Keep using existing sessions and retry after the advertised delay; cleanup-failed states require channel recycle, while settlement-overdue states may recover when the request settles. | | `MissingCliEntryError` | 500 | The `qwen` CLI entry file is missing (defined in `status.ts`, not `bridgeErrors.ts`). | Confirm the CLI install is complete; check that `packages/cli/index.ts` exists. | ## Boot-time configuration errors (`packages/cli/src/serve/run-qwen-serve.ts`) diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index af5743c73d8..ee448f9cab9 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -2172,7 +2172,7 @@ Response: **History replay over SSE.** While `loadSession` is in flight on the agent side, the agent may emit `session_update` notifications for persisted turns, or return bulk replay updates in the response metadata. The daemon seeds those events into the session's bounded replay snapshot window before the route response returns. For live sessions, `POST /session/:id/load` only promises that bounded window (`compactedReplay`, `liveJournal`, `lastEventId`), not the full transcript. The window is byte-capped by `--compacted-replay-max-bytes` (default 4 MiB, maximum 256 MiB); if older replay entries were dropped, `compactedReplay[0]` is an id-less `history_truncated` marker. The in-flight `liveJournal` is separately capped by `--max-journal-events` (default 10 000 replay entries) and `--max-journal-bytes` (default 8 MiB of serialized source events). These are per-session **baseline** caps. When an in-flight turn outgrows them, the daemon first tries adaptive growth: it raises that session's caps toward double (up to a per-session hard cap of 256 MiB, entries scaled proportionally, limited by the remaining pool headroom) while the growth granted across every live session fits in one daemon-wide growth pool sized at 5% of the daemon's effective memory budget — the `--memory-budget-mb` value when passed, capped at resolved available memory, otherwise 50% of auto-detected memory — capped at `1024` MB. Accounting is daemon-wide — a multi-workspace daemon runs one bridge per workspace and all of them share the single pool. Growth is on demand and only as far as the pool allows; an operator-pinned `--max-journal-events` or `--max-journal-bytes` disables it, as does a host whose effective budget falls below the 1024 MB minimum (`insufficientMemory`): the pool is 0 and adaptive growth is disabled outright. Consecutive compatible `agent_message_chunk` or `agent_thought_chunk` source events share a replay entry, up to 256 source events per entry, while tool, attribution, provenance, and discrete-message boundaries remain intact. When the journal still exceeds its (possibly grown) caps after the growth the pool allows — including when no headroom is granted or a grant covers only part of the overshoot — the oldest entries are dropped whole (so the retained tail can be much smaller than the byte cap) and a `history_truncated` marker with `scope: 'live_journal'` is prepended; its `truncatedEvents` and `retainedEvents` fields count source events, not replay entries, and its `maxBytes` / `maxEvents` reflect the caps in force (which may already have grown). Clients should render that marker as status and continue applying retained events. Full persisted transcript access is exposed separately through `GET /session/:id/transcript`. -The replay-window byte caps apply after the child has reconstructed the persisted transcript; they do not cap the on-disk JSONL read. A restore that exceeds the daemon budget returns `504` with a `Retry-After` derived from the restore budget (clamped to 5-120s) and `{code: "session_restore_timeout", errorKind: "restore_timeout", retryable: true, sessionId, action, timeoutMs}`. The daemon fences the still-running ACP request and cleans up any late session instead of registering it. A retry for the same id returns `409 restore_in_progress` with `reason: "awaiting_abandoned_cleanup"` and a `Retry-After` of the restore budget (clamped to 5-120s) until that cleanup settles. If late restore cleanup is uncertain, or the abandoned restore has still not settled a full restore budget after its deadline, new sessions on that workspace return `503 acp_channel_unavailable` with `reason: "restore_cleanup_failed"` or `"restore_settlement_overdue"`. A timed-out session initialization follows the same fail-closed admission policy for an older ACP child that settles late: inconclusive cleanup returns `reason: "new_session_cleanup_failed"`, while a request that remains unsettled for one further initialization budget returns `reason: "new_session_settlement_overdue"`. Already-live sessions remain usable while the channel drains for all four reasons. +The replay-window byte caps apply after the child has reconstructed the persisted transcript; they do not cap the on-disk JSONL read. A restore that exceeds the daemon budget returns `504` with a `Retry-After` derived from the restore budget (clamped to 5-120s) and `{code: "session_restore_timeout", errorKind: "restore_timeout", retryable: true, sessionId, action, timeoutMs}`. The daemon fences the still-running ACP request and cleans up any late session instead of registering it. A retry for the same id returns `409 restore_in_progress` with `reason: "awaiting_abandoned_cleanup"` and a `Retry-After` of the restore budget (clamped to 5-120s) until that cleanup settles. If late restore cleanup is uncertain, or the abandoned restore has still not settled a full restore budget after its deadline, new sessions on that workspace return `503 acp_channel_unavailable` with `reason: "restore_cleanup_failed"` or `"restore_settlement_overdue"`. A timed-out session initialization follows the same fail-closed admission policy for an older ACP child that settles late: inconclusive cleanup returns `reason: "new_session_cleanup_failed"`, while a request that remains unsettled for one further initialization budget returns `reason: "new_session_settlement_overdue"`. Cleanup-failed states last until the workspace channel drains and is recycled; settlement-overdue states clear if the abandoned request settles cleanly first. Already-live sessions remain usable in either case. **Errors:** @@ -2181,7 +2181,7 @@ The replay-window byte caps apply after the child has reconstructed the persiste - `403` — `untrusted_workspace` when `cwd` targets an untrusted non-primary workspace. - `503` — `session_limit_exceeded` (counts against `--max-sessions`; in-flight restores are accounted for too). - `504` — `session_restore_timeout`; retryable, with a `Retry-After` derived from the restore budget (clamped to 5-120s) because the same session id stays fenced until late cleanup settles. -- `503` — `acp_channel_unavailable` when the workspace channel is closed to new session work. `reason` says why: `restore_cleanup_failed` when an abandoned restore could not be cleaned up conclusively; `restore_settlement_overdue` when an abandoned restore has still not settled one full restore budget after its deadline; `new_session_cleanup_failed` when a session created after the public initialization timeout could not be closed conclusively; or `new_session_settlement_overdue` when the timed-out initialization has still not settled one further initialization budget after its deadline. In all four cases existing sessions remain available, and new session work may be retried after the workspace channel drains — the body carries `retryAfterSeconds` and the header a matching budget-derived `Retry-After`, because quarantine outlives the fence and a fresh id never sees the 409 that would carry the hint. +- `503` — `acp_channel_unavailable` when the workspace channel is closed to new session work. `reason` says why: `restore_cleanup_failed` when an abandoned restore could not be cleaned up conclusively; `restore_settlement_overdue` when an abandoned restore has still not settled one full restore budget after its deadline; `new_session_cleanup_failed` when a session created after the public initialization timeout could not be closed conclusively; or `new_session_settlement_overdue` when the timed-out initialization has still not settled one further initialization budget after its deadline. Existing sessions remain available. Cleanup-failed states require the workspace channel to drain and recycle; settlement-overdue states clear when the abandoned request settles or the channel drains. The body carries `retryAfterSeconds` and the header a matching budget-derived `Retry-After` so clients use an operation-budget-scale backoff instead of polling at the ordinary 5-second cadence. - `409` — `restore_in_progress` (a `session/resume` for the same id is already in flight, or a fresh spawn supplied an id a restore owns). `Retry-After: 5` while the restore is active; a budget-derived hint once it is fenced as `awaiting_abandoned_cleanup`. Same-action races (two concurrent `session/load` for the same id) coalesce — exactly one returns `attached: false`, the rest return `attached: true` with the same `state`. - `409` — `session_workspace_conflict` when the same session id is already live or being restored by another workspace runtime. - `409` — `session_archived` when the id exists only under `chats/archive/`; call `POST /sessions/unarchive` before `load` or `resume`. diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index d6c11083f00..6a404b75df9 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -239,6 +239,31 @@ function recordRestoreEvents(): { }; } +function recordNewSessionEvents(): { + telemetry: BridgeTelemetry; + named: (suffix: string) => Array>; +} { + const events: Array<{ name: string; attributes: Record }> = + []; + return { + named: (suffix) => + events + .filter((event) => event.name === `session.new.${suffix}`) + .map((event) => event.attributes), + telemetry: { + captureContext: () => undefined, + runWithContext: async (_captured, fn) => await fn(), + withSpan: async (_operation, _attributes, fn) => await fn(), + event: (name, attributes) => { + if (name.startsWith('session.new.')) { + events.push({ name, attributes: { ...attributes } }); + } + }, + injectPromptContext: (request) => request, + }, + }; +} + async function advanceRestoreDeadline(timeoutMs: number): Promise { await vi.advanceTimersByTimeAsync(0); await vi.advanceTimersByTimeAsync(timeoutMs); @@ -329,6 +354,20 @@ function reportingGrade(bridge: { } describe('createAcpSessionBridge', () => { + it.each([undefined, 60_000])( + 'rejects a fractional initialization timeout with restore timeout %s', + (sessionRestoreTimeoutMs) => { + expect(() => + makeBridge({ + initializeTimeoutMs: 20.5, + ...(sessionRestoreTimeoutMs !== undefined + ? { sessionRestoreTimeoutMs } + : {}), + }), + ).toThrow(/positive integer/); + }, + ); + describe('active work', () => { it('negotiates the capability and counts accepted prompts locally', async () => { const prompt = deferred(); @@ -12725,6 +12764,7 @@ describe('createAcpSessionBridge', () => { it('closes a session created after the public newSession deadline', async () => { const late = deferred(); const closeCalls: Array> = []; + const newSessionEvents = recordNewSessionEvents(); const handle = makeChannel({ newSessionImpl: (params, agent) => agent.newSessionCalls.length === 2 @@ -12743,6 +12783,7 @@ describe('createAcpSessionBridge', () => { initializeTimeoutMs: 20, maxSessions: 2, sessionScope: 'thread', + telemetry: newSessionEvents.telemetry, }); const sibling = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); @@ -12751,6 +12792,12 @@ describe('createAcpSessionBridge', () => { ).rejects.toBeInstanceOf(BridgeTimeoutError); expect(handle.killed).toBe(false); expect(bridge.sessionCount).toBe(1); + expect(newSessionEvents.named('public_result')).toEqual([ + expect.objectContaining({ + 'qwen-code.daemon.session_new.result': 'timeout', + 'qwen-code.daemon.session_new.channel_was_empty': false, + }), + ]); expect( handle.agent.newSessionCalls[1]?._meta?.[ SESSION_INITIALIZATION_DEADLINE_META_KEY @@ -12775,6 +12822,87 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('reports and reaps a timed-out newSession on an empty channel', async () => { + vi.useFakeTimers(); + const newSessionEvents = recordNewSessionEvents(); + const handle = makeChannel({ + newSessionImpl: () => new Promise(() => {}), + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + telemetry: newSessionEvents.telemetry, + }); + try { + const timedOut = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(20); + + expect(await timedOut).toBeInstanceOf(BridgeTimeoutError); + expect(newSessionEvents.named('public_result')).toEqual([ + expect.objectContaining({ + 'qwen-code.daemon.session_new.result': 'timeout', + 'qwen-code.daemon.session_new.channel_was_empty': true, + }), + ]); + expect(handle.killed).toBe(true); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } + }); + + it('treats a missing late newSession as conclusively cleaned up', async () => { + vi.useFakeTimers(); + const late = deferred(); + const newSessionEvents = recordNewSessionEvents(); + const handle = makeChannel({ + newSessionImpl: (_params, agent) => + agent.newSessionCalls.length === 2 + ? late.promise + : { sessionId: `visible-${agent.newSessionCalls.length}` }, + extMethodImpl: (method, params) => { + if (method === SERVE_CONTROL_EXT_METHODS.sessionClose) { + throw RequestError.resourceNotFound( + `session:${String(params['sessionId'])}`, + ); + } + return {}; + }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + maxSessions: 2, + sessionScope: 'thread', + telemetry: newSessionEvents.telemetry, + }); + try { + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const timedOut = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(20); + expect(await timedOut).toBeInstanceOf(BridgeTimeoutError); + + late.resolve({ sessionId: 'already-gone' }); + await vi.advanceTimersByTimeAsync(0); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).resolves.toMatchObject({ sessionId: 'visible-3' }); + expect(newSessionEvents.named('cleanup')).toEqual([ + expect.objectContaining({ + 'qwen-code.daemon.session_new.cleanup_result': 'not_found', + 'session.id': 'already-gone', + }), + ]); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } + }); + it('holds a requested id until an abandoned newSession settles', async () => { const late = deferred(); const handle = makeChannel({ @@ -12805,6 +12933,19 @@ describe('createAcpSessionBridge', () => { ).rejects.toMatchObject({ activeAction: 'spawn', requestedAction: 'spawn', + reason: 'awaiting_abandoned_cleanup', + retryAfterSeconds: 5, + }); + await expect( + bridge.loadSession({ + sessionId: 'requested-late', + workspaceCwd: WS_A, + }), + ).rejects.toMatchObject({ + activeAction: 'spawn', + requestedAction: 'load', + reason: 'awaiting_abandoned_cleanup', + retryAfterSeconds: 5, }); late.resolve({ sessionId: 'requested-late' }); @@ -12826,6 +12967,7 @@ describe('createAcpSessionBridge', () => { }); it('keeps the public timeout contract when the agent enforces the deadline', async () => { + const newSessionEvents = recordNewSessionEvents(); const handle = makeChannel({ newSessionImpl: (_params, agent) => { if (agent.newSessionCalls.length === 1) { @@ -12842,6 +12984,7 @@ describe('createAcpSessionBridge', () => { channelFactory: async () => handle.channel, initializeTimeoutMs: 1_000, sessionScope: 'thread', + telemetry: newSessionEvents.telemetry, }); await bridge.spawnOrAttach({ workspaceCwd: WS_A }); @@ -12854,82 +12997,207 @@ describe('createAcpSessionBridge', () => { }); expect(handle.killed).toBe(false); expect(bridge.sessionCount).toBe(1); + expect(newSessionEvents.named('public_result')).toEqual([ + expect.objectContaining({ + 'qwen-code.daemon.session_new.result': 'timeout', + 'qwen-code.daemon.session_new.timeout_ms': 1_000, + 'qwen-code.daemon.session_new.channel_was_empty': false, + }), + ]); await bridge.shutdown(); }); it('refuses fresh sessions when late newSession close is refused', async () => { + vi.useFakeTimers(); const late = deferred(); const handle = makeChannel({ newSessionImpl: (_params, agent) => agent.newSessionCalls.length === 2 ? late.promise : { sessionId: `visible-${agent.newSessionCalls.length}` }, - extMethodImpl: (method) => { + initializeImpl: () => + activeWorkInitializeResponse({ + categories: [...ACTIVE_WORK_LEGACY_HOLD_CATEGORIES], + }), + extMethodImpl: (method, params) => { if (method === SERVE_CONTROL_EXT_METHODS.sessionClose) { - return { closed: false, holds: [agentHold('late-agent')] }; + return params['sessionId'] === 'hidden-late' + ? { closed: false, holds: [agentHold('late-agent')] } + : new Promise>(() => {}); } return {}; }, }); const bridge = makeBridge({ channelFactory: async () => handle.channel, - initializeTimeoutMs: 20, + initializeTimeoutMs: 20_000, sessionScope: 'thread', }); - const sibling = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await expect( - bridge.spawnOrAttach({ workspaceCwd: WS_A }), - ).rejects.toBeInstanceOf(BridgeTimeoutError); + try { + const sibling = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const timedOut = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(20_000); + expect(await timedOut).toBeInstanceOf(BridgeTimeoutError); - late.resolve({ sessionId: 'hidden-late' }); - await vi.waitFor(() => - expect(handle.agent.extMethodCalls).toContainEqual( - expect.objectContaining({ - method: SERVE_CONTROL_EXT_METHODS.sessionClose, + late.resolve({ sessionId: 'hidden-late' }); + await vi.advanceTimersByTimeAsync(0); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ reason: 'new_session_cleanup_failed' }); + await expect( + bridge.sendPrompt(sibling.sessionId, { + sessionId: sibling.sessionId, + prompt: [{ type: 'text', text: 'still alive' }], }), - ), - ); - await new Promise((resolve) => setImmediate(resolve)); - await expect( - bridge.spawnOrAttach({ workspaceCwd: WS_A }), - ).rejects.toMatchObject({ reason: 'new_session_cleanup_failed' }); - await expect( - bridge.sendPrompt(sibling.sessionId, { - sessionId: sibling.sessionId, - prompt: [{ type: 'text', text: 'still alive' }], - }), - ).resolves.toMatchObject({ stopReason: 'end_turn' }); - expect(handle.killed).toBe(false); - expect(bridge.sessionCount).toBe(1); - await bridge.shutdown(); + ).resolves.toMatchObject({ stopReason: 'end_turn' }); + expect(handle.killed).toBe(false); + + const detached = bridge.detachClient(sibling.sessionId, sibling.clientId); + await vi.advanceTimersByTimeAsync(ACTIVE_WORK_CLOSE_TIMEOUT_MS); + await detached; + expect(handle.killed).toBe(true); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } }); it('refuses fresh sessions when an abandoned newSession does not settle', async () => { + vi.useFakeTimers(); const handle = makeChannel({ + initializeImpl: () => + activeWorkInitializeResponse({ + categories: [...ACTIVE_WORK_LEGACY_HOLD_CATEGORIES], + }), newSessionImpl: (_params, agent) => agent.newSessionCalls.length === 2 ? new Promise(() => {}) : { sessionId: `visible-${agent.newSessionCalls.length}` }, + extMethodImpl: (method) => + method === SERVE_CONTROL_EXT_METHODS.sessionClose + ? new Promise>(() => {}) + : {}, }); const bridge = makeBridge({ channelFactory: async () => handle.channel, initializeTimeoutMs: 20, sessionScope: 'thread', }); - await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - await expect( - bridge.spawnOrAttach({ workspaceCwd: WS_A }), - ).rejects.toBeInstanceOf(BridgeTimeoutError); + try { + const sibling = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const timedOut = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(20); + expect(await timedOut).toBeInstanceOf(BridgeTimeoutError); + await vi.advanceTimersByTimeAsync(20); - await new Promise((resolve) => setTimeout(resolve, 30)); - await expect( - bridge.spawnOrAttach({ workspaceCwd: WS_A }), - ).rejects.toMatchObject({ reason: 'new_session_settlement_overdue' }); - expect(handle.killed).toBe(false); - expect(bridge.sessionCount).toBe(1); - await bridge.shutdown(); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ reason: 'new_session_settlement_overdue' }); + await expect( + bridge.sendPrompt(sibling.sessionId, { + sessionId: sibling.sessionId, + prompt: [{ type: 'text', text: 'still alive' }], + }), + ).resolves.toMatchObject({ stopReason: 'end_turn' }); + expect(handle.killed).toBe(false); + + const detached = bridge.detachClient(sibling.sessionId, sibling.clientId); + await vi.advanceTimersByTimeAsync(ACTIVE_WORK_CLOSE_TIMEOUT_MS); + await detached; + expect(handle.killed).toBe(true); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } }); + it('reopens fresh admission when an overdue newSession settles cleanly', async () => { + vi.useFakeTimers(); + const late = deferred(); + const handle = makeChannel({ + newSessionImpl: (_params, agent) => + agent.newSessionCalls.length === 2 + ? late.promise + : { sessionId: `visible-${agent.newSessionCalls.length}` }, + extMethodImpl: (method) => + method === SERVE_CONTROL_EXT_METHODS.sessionClose + ? { closed: true } + : {}, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + sessionScope: 'thread', + }); + try { + const sibling = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const timedOut = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(20); + expect(await timedOut).toBeInstanceOf(BridgeTimeoutError); + await vi.advanceTimersByTimeAsync(20); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ reason: 'new_session_settlement_overdue' }); + + late.resolve({ sessionId: 'hidden-late' }); + await vi.advanceTimersByTimeAsync(0); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).resolves.toMatchObject({ sessionId: 'visible-3' }); + expect(handle.killed).toBe(false); + expect(sibling.sessionId).toBe('visible-1'); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } + }); + + it.each(['load', 'resume'] as const)( + 'counts an abandoned newSession against %s admission capacity', + async (action) => { + vi.useFakeTimers(); + const handle = makeChannel({ + newSessionImpl: (_params, agent) => + agent.newSessionCalls.length === 2 + ? new Promise(() => {}) + : { sessionId: `visible-${agent.newSessionCalls.length}` }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + maxSessions: 2, + sessionScope: 'thread', + }); + try { + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const timedOut = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(20); + expect(await timedOut).toBeInstanceOf(BridgeTimeoutError); + + const request = { + sessionId: `capacity-${action}`, + workspaceCwd: WS_A, + }; + const result = + action === 'load' + ? bridge.loadSession(request) + : bridge.resumeSession(request); + await expect(result).rejects.toBeInstanceOf(SessionLimitExceededError); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } + }, + ); + it('killAllSync force-kills BOTH the dying channel AND the fresh attach-target (BkUyD overwrite race)', async () => { // The killSession → spawnOrAttach race opens a window where two // channels are simultaneously "alive" from the daemon's diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index d3c5073e7e8..e8aba987452 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -2786,16 +2786,15 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { opts.childEnvOverrides ? Object.freeze({ ...opts.childEnvOverrides }) : Object.freeze({}); - const rawInitTimeoutMs = opts.initializeTimeoutMs ?? DEFAULT_INIT_TIMEOUT_MS; - if (!Number.isFinite(rawInitTimeoutMs) || rawInitTimeoutMs <= 0) { + const initTimeoutMs = opts.initializeTimeoutMs ?? DEFAULT_INIT_TIMEOUT_MS; + if (!Number.isInteger(initTimeoutMs) || initTimeoutMs <= 0) { throw new TypeError( - `Invalid initializeTimeoutMs: ${rawInitTimeoutMs}. Must be a finite number > 0.`, + `Invalid initializeTimeoutMs: ${initTimeoutMs}. Must be a positive integer.`, ); } - const initTimeoutMs = Math.ceil(rawInitTimeoutMs); if (initTimeoutMs > 2_147_483_647) { throw new TypeError( - `Invalid initializeTimeoutMs: ${rawInitTimeoutMs}. Must not exceed the supported timer range.`, + `Invalid initializeTimeoutMs: ${initTimeoutMs}. Must not exceed the supported timer range.`, ); } const newSessionSettlementGraceMs = initTimeoutMs; @@ -3041,8 +3040,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const capability = owner?.activeWork; if ( capability && - !owner.isQuarantined && - !owner.restoreSettlementOverdue && + !channelIsCondemned(owner) && ACTIVE_WORK_HOLD_CATEGORIES.some( (category) => !capability.categories.includes(category), ) @@ -3150,8 +3148,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // that teardown is exactly what the drain is waiting for. const condemnedOwner = channelInfoForEntry(entry); const agentCloseTimeoutMs = - condemnedOwner?.isQuarantined === true || - condemnedOwner?.restoreSettlementOverdue === true + condemnedOwner !== undefined && channelIsCondemned(condemnedOwner) ? ACTIVE_WORK_CLOSE_TIMEOUT_MS : undefined; await closeSessionImpl(entry.sessionId, undefined, { @@ -3194,20 +3191,15 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const info = channelInfoForEntry(entry); if (!info?.activeWork) return true; if (info.isDying) return false; - // A channel the restore lifecycle has already condemned is waiting to be + // A channel the session lifecycle has already condemned is waiting to be // reaped as soon as its visible work drains — that teardown is the only - // thing that can release a non-cancellable restore we have given up on. + // thing that can release a non-cancellable request we have given up on. // Deferring to the child here would make the drain depend on the very - // process we have declared unreliable, and a child wedged mid-restore is + // process we have declared unreliable, and a child wedged mid-request is // precisely the one that cannot answer this round trip inside // `ACTIVE_WORK_CLOSE_TIMEOUT_MS`. Nothing is attached to this session // (`maybeCloseIdleSession` gates on that), so proceed to local teardown. - if ( - info.isQuarantined || - info.restoreSettlementOverdue || - info.newSessionCleanupFailed || - info.newSessionSettlementOverdue - ) { + if (channelIsCondemned(info)) { return true; } try { @@ -3487,6 +3479,15 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ); } + function channelIsCondemned(ci: ChannelInfo): boolean { + return ( + ci.isQuarantined || + ci.restoreSettlementOverdue || + ci.newSessionCleanupFailed || + ci.newSessionSettlementOverdue + ); + } + /** * A restore that blew its public deadline is left running because the ACP * request cannot be cancelled — but "cannot cancel" must not mean "wait @@ -3511,7 +3512,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ci.restoreSettlementOverdue = true; writeStderrLine( `qwen serve: abandoned session/${action} for ${JSON.stringify(sessionId)} has not settled ` + - `${restoreSettlementGraceMs}ms after its deadline; refusing fresh sessions on channel ${ci.id} until it drains`, + `${restoreSettlementGraceMs}ms after its deadline; refusing fresh sessions on channel ${ci.id} until it settles or drains`, ); telemetry.event('session.restore.settlement_overdue', { 'qwen-code.daemon.session_restore.action': action, @@ -3540,7 +3541,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ci.newSessionSettlementOverdue = true; writeStderrLine( `qwen serve: abandoned newSession${requestedSessionId ? ` for ${JSON.stringify(requestedSessionId)}` : ''} has not settled ` + - `${newSessionSettlementGraceMs}ms after its deadline; refusing fresh sessions on channel ${ci.id} until it drains`, + `${newSessionSettlementGraceMs}ms after its deadline; refusing fresh sessions on channel ${ci.id} until it settles or drains`, ); telemetry.event('session.new.settlement_overdue', { 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, @@ -3877,6 +3878,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // `inFlightRestores`: whichever operation reserves the id first owns its // registration window. const inFlightRequestedSessionSpawns = new Map(); + const abandonedRequestedSessionSpawns = new Set(); interface InFlightRestore { action: 'load' | 'resume'; @@ -4612,6 +4614,26 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } } + function recordNewSessionPublicTimeout( + ci: ChannelInfo, + requestedSessionId: string | undefined, + ): boolean { + const channelWasEmpty = hasNoChannelWork(ci, { + ignoreCurrentSessionSpawn: true, + }); + telemetry.event('session.new.public_result', { + 'qwen-code.daemon.session_new.result': 'timeout', + 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, + 'qwen-code.daemon.acp_channel.id': ci.id, + 'qwen-code.daemon.session_new.channel_was_empty': channelWasEmpty, + ...(requestedSessionId ? { 'session.id': requestedSessionId } : {}), + }); + writeStderrLine( + `qwen serve: newSession timed out after ${initTimeoutMs}ms${requestedSessionId ? ` for ${JSON.stringify(requestedSessionId)}` : ''} on channel ${ci.id}; decision=${channelWasEmpty ? 'kill_empty' : 'fence_shared'}`, + ); + return channelWasEmpty; + } + async function settleAbandonedNewSession( ci: ChannelInfo, token: symbol, @@ -4855,21 +4877,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { abandonedNewSessionSettlements.delete(settlement); }); onNewSessionAbandoned?.(settlement); - const channelWasEmpty = hasNoChannelWork(ci, { - ignoreCurrentSessionSpawn: true, - }); - telemetry.event('session.new.public_result', { - 'qwen-code.daemon.session_new.result': 'timeout', - 'qwen-code.daemon.session_new.timeout_ms': initTimeoutMs, - 'qwen-code.daemon.acp_channel.id': ci.id, - 'qwen-code.daemon.session_new.channel_was_empty': - channelWasEmpty, - ...(requestedSessionId - ? { 'session.id': requestedSessionId } - : {}), - }); - writeStderrLine( - `qwen serve: newSession timed out after ${initTimeoutMs}ms${requestedSessionId ? ` for ${JSON.stringify(requestedSessionId)}` : ''} on channel ${ci.id}; decision=${channelWasEmpty ? 'kill_empty' : 'fence_shared'}`, + const channelWasEmpty = recordNewSessionPublicTimeout( + ci, + requestedSessionId, ); if (!channelWasEmpty) { armNewSessionSettlementGrace( @@ -4902,12 +4912,17 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { (error: unknown) => { if (lifecycle.phase === 'active') { clearTimeout(timer); - reject( + if ( extractJsonRpcErrorField(error, 'errorKind') === - SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND - ? new BridgeTimeoutError('newSession', initTimeoutMs) - : error, - ); + SESSION_INITIALIZATION_TIMEOUT_ERROR_KIND + ) { + recordNewSessionPublicTimeout(ci, requestedSessionId); + reject( + new BridgeTimeoutError('newSession', initTimeoutMs), + ); + } else { + reject(error); + } return; } void settleAbandonedNewSession( @@ -7179,7 +7194,18 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } if (inFlightRequestedSessionSpawns.has(req.sessionId)) { - throw new RestoreInProgressError(req.sessionId, 'spawn', action); + const owner = inFlightRequestedSessionSpawns.get(req.sessionId); + throw new RestoreInProgressError( + req.sessionId, + 'spawn', + action, + owner !== undefined && abandonedRequestedSessionSpawns.has(owner) + ? { + reason: 'awaiting_abandoned_cleanup', + retryAfterSeconds: abandonedNewSessionRetryAfterSeconds, + } + : undefined, + ); } const inFlight = inFlightRestores.get(req.sessionId); @@ -8823,7 +8849,18 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ); } if (inFlightRequestedSessionSpawns.has(req.sessionId)) { - throw new RestoreInProgressError(req.sessionId, 'spawn', 'spawn'); + const owner = inFlightRequestedSessionSpawns.get(req.sessionId); + throw new RestoreInProgressError( + req.sessionId, + 'spawn', + 'spawn', + owner !== undefined && abandonedRequestedSessionSpawns.has(owner) + ? { + reason: 'awaiting_abandoned_cleanup', + retryAfterSeconds: abandonedNewSessionRetryAfterSeconds, + } + : undefined, + ); } } // Cap check: count both registered sessions and in-flight spawns @@ -8853,6 +8890,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ); } const releaseRequestedSessionRegistration = () => { + if (requestedSessionRegistrationOwner !== undefined) { + abandonedRequestedSessionSpawns.delete( + requestedSessionRegistrationOwner, + ); + } if ( req.sessionId !== undefined && requestedSessionRegistrationOwner !== undefined && @@ -8900,6 +8942,11 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { : undefined, (settlement) => { abandonedSettlement = settlement; + if (requestedSessionRegistrationOwner !== undefined) { + abandonedRequestedSessionSpawns.add( + requestedSessionRegistrationOwner, + ); + } }, ); // Track in-flight spawns regardless of scope. Under `single` diff --git a/packages/acp-bridge/src/bridgeErrors.ts b/packages/acp-bridge/src/bridgeErrors.ts index 8d545e60dca..38416355270 100644 --- a/packages/acp-bridge/src/bridgeErrors.ts +++ b/packages/acp-bridge/src/bridgeErrors.ts @@ -139,9 +139,9 @@ export class SessionArchivingError extends Error { * * `restore_in_progress` is the ordinary case: a restore is running and the * caller can retry shortly. `awaiting_abandoned_cleanup` means the public - * caller already received a timeout, but the non-cancellable ACP request (and - * its cleanup) has not settled yet — retrying at the ordinary cadence just - * re-hits the fence, so clients must back off much further. + * caller already received a timeout, but the non-cancellable ACP registration + * request (and its cleanup) has not settled yet — retrying at the ordinary + * cadence just re-hits the fence, so clients must back off much further. */ export type RestoreInProgressReason = | 'restore_in_progress' @@ -182,7 +182,7 @@ export class RestoreInProgressError extends Error { : `session/${activeAction}`; super( reason === 'awaiting_abandoned_cleanup' - ? `Session "${sessionId}" timed out during ${activeTarget} and its abandoned restore has not settled yet; retry ${retryTarget} once cleanup completes` + ? `Session "${sessionId}" timed out during ${activeTarget} and its abandoned registration has not settled yet; retry ${retryTarget} once cleanup completes` : activeAction === 'spawn' ? `Session "${sessionId}" is already being registered by ${activeTarget}; retry ${retryTarget} after it completes` : `Session "${sessionId}" is already being restored via ${activeTarget}; retry ${retryTarget} after it completes`, @@ -634,7 +634,8 @@ export class WorkspaceDrainingError extends Error { * Why a channel is closed to new session work. Cleanup failures mean the * child's state is unknown; settlement-overdue states mean the child still * holds work the bridge can neither cancel nor account for. Existing sessions - * remain usable while the channel drains and is recycled. + * remain usable. Cleanup-failed states last until channel recycle; + * settlement-overdue states may clear when the abandoned request settles. */ export type BridgeChannelUnavailableReason = | 'restore_cleanup_failed' @@ -645,10 +646,9 @@ export type BridgeChannelUnavailableReason = export class BridgeChannelQuarantinedError extends Error { readonly reason: BridgeChannelUnavailableReason; /** - * How long the caller should wait before retrying fresh session work. This - * state persists until the workspace channel drains, which is at least a - * restore budget away — the ordinary 5-second cadence would poll identical - * 503s, and a fresh id never reaches the 409 that carries the real hint. + * How long the caller should wait before retrying fresh session work. The + * operation-budget-derived hint avoids polling these longer-lived states at + * the ordinary 5-second cadence. */ readonly retryAfterSeconds: number; diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 8b17978d351..17c27a22cb9 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -2355,6 +2355,41 @@ describe('QwenAgent MCP SSE/HTTP support', () => { await agentPromise; }); + it.each([ + ['non-positive', () => 0], + ['non-integer', () => Date.now() + 0.5], + ['non-safe', () => Number.MAX_SAFE_INTEGER + 1], + ['beyond the timer range', () => Date.now() + 2_147_483_648], + ])( + 'rejects a %s trusted session initialization deadline before creating state', + async (_label, deadline) => { + await setupSessionMocks('invalid-deadline-session'); + const { agent, agentPromise } = await bootInitializedAcpAgent( + makeSessionSettings(), + 'expected-capability', + ); + + await expect( + agent.newSession({ + cwd: '/tmp', + mcpServers: [], + _meta: { + [SESSION_INITIALIZATION_DEADLINE_META_KEY]: deadline(), + }, + }), + ).rejects.toMatchObject({ + errorKind: 'invalid_session_initialization_deadline', + }); + expect(vi.mocked(Session)).not.toHaveBeenCalled(); + + await expect( + agent.newSession({ cwd: '/tmp', mcpServers: [] }), + ).resolves.toMatchObject({ sessionId: 'invalid-deadline-session' }); + mockConnectionState.resolve(); + await agentPromise; + }, + ); + it('ignores a forged session initialization deadline from an untrusted parent', async () => { await setupSessionMocks('untrusted-deadline-session'); const { agent, agentPromise } = await bootInitializedAcpAgent( diff --git a/packages/cli/src/serve/acp-http/dispatch-error.test.ts b/packages/cli/src/serve/acp-http/dispatch-error.test.ts index 6cbd2ef4fe0..afedc0bbe5f 100644 --- a/packages/cli/src/serve/acp-http/dispatch-error.test.ts +++ b/packages/cli/src/serve/acp-http/dispatch-error.test.ts @@ -111,10 +111,10 @@ describe('toRpcError', () => { }); it('carries every quarantine reason and its backoff hint', () => { - // Quarantine outlives the fence, and a fresh-id request never reaches the - // 409 that carries the real hint — so this payload is the only backoff - // signal such a caller gets. + // A fresh-id request never reaches the same-id 409, so this payload is the + // only operation-budget-scale backoff signal such a caller gets. for (const reason of [ + 'restore_cleanup_failed', 'restore_settlement_overdue', 'new_session_cleanup_failed', 'new_session_settlement_overdue', diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 34251953839..94d40d367e3 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -13918,6 +13918,7 @@ describe('createServeApp', () => { it('503s fresh session work for every channel quarantine reason', async () => { for (const reason of [ + 'restore_cleanup_failed', 'restore_settlement_overdue', 'new_session_cleanup_failed', 'new_session_settlement_overdue', @@ -13934,8 +13935,8 @@ describe('createServeApp', () => { .send({}); expect(res.status).toBe(503); - // Quarantine lasts until the channel drains — strictly longer than the - // fence — and a fresh-id caller never sees the 409 that would tell it so. + // Fresh-id callers never see the same-id 409, so the 503 must carry the + // operation-budget-scale retry hint itself. expect(res.headers['retry-after']).toBe('90'); expect(res.body).toMatchObject({ code: 'acp_channel_unavailable', diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index c7c8ed41147..f5b5b6cd2b4 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -4347,6 +4347,65 @@ describe('Server Config (config.ts)', () => { ); }); + it('rejects a pre-aborted initialization without consuming the Config', async () => { + const config = new Config(baseParams); + const controller = new AbortController(); + const abortReason = new Error('initialization cancelled before start'); + controller.abort(abortReason); + + await expect( + config.initialize({ signal: controller.signal }), + ).rejects.toBe(abortReason); + + const initializeInternal = vi + .spyOn( + config as unknown as { + initializeInternal: () => Promise; + }, + 'initializeInternal', + ) + .mockResolvedValue(undefined); + await expect(config.initialize()).resolves.toBeUndefined(); + expect(initializeInternal).toHaveBeenCalledOnce(); + await config.shutdown({ shutdownTelemetry: false }); + }); + + it('forwards cancellation into Gemini client initialization', async () => { + const config = new Config(baseParams); + const controller = new AbortController(); + const abortReason = new Error('initialization deadline exceeded'); + let markGeminiEntered!: () => void; + const geminiEntered = new Promise((resolve) => { + markGeminiEntered = resolve; + }); + const geminiInitialize = vi + .spyOn(config.getGeminiClient(), 'initialize') + .mockImplementation(async (_source, signal) => { + expect(signal).toBe(controller.signal); + markGeminiEntered(); + await new Promise((_resolve, reject) => { + if (signal?.aborted) { + reject(signal.reason); + return; + } + signal?.addEventListener('abort', () => reject(signal.reason), { + once: true, + }); + }); + }); + + const initialization = config.initialize({ signal: controller.signal }); + await geminiEntered; + controller.abort(abortReason); + + await expect(initialization).rejects.toBe(abortReason); + expect(geminiInitialize).toHaveBeenCalledWith( + undefined, + controller.signal, + ); + await config.shutdown({ shutdownTelemetry: false }); + }); + it('preserves graceful writer finalization after successful initialization', async () => { const config = new Config(baseParams); vi.spyOn( diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index e2980c12813..5c0ea65b13a 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -1106,10 +1106,11 @@ describe('Gemini Client (client.ts)', () => { it('passes cancellation to SessionStart hooks and does not swallow it', async () => { const controller = new AbortController(); const timeoutError = new Error('session initialization timed out'); + const hookError = new Error('hook exploded independently'); const fireSessionStartEvent = vi.fn(async (...args: unknown[]) => { expect(args[4]).toBe(controller.signal); controller.abort(timeoutError); - throw timeoutError; + throw hookError; }); vi.mocked(mockConfig.getDisableAllHooks).mockReturnValue(false); vi.mocked(mockConfig.hasHooksForEvent).mockReturnValue(true); From a88b29e398947ac4024e331b98fe0ef2a9dc2c64 Mon Sep 17 00:00:00 2001 From: "jinye.djy" Date: Fri, 28 Aug 2026 01:09:24 +0800 Subject: [PATCH 3/6] codex: address PR review feedback (#10268) Co-authored-by: Qwen-Coder --- docs/developers/qwen-serve-protocol.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index ee448f9cab9..890ea5df4e9 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -124,10 +124,10 @@ Fired when a `session/load` is issued for an id that already has a `session/resu `reason` distinguishes two fences that share this code, and the `Retry-After` header tracks it: -| `reason` | Meaning | `Retry-After` | -| ---------------------------- | ---------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------- | -| `restore_in_progress` | An ordinary restore is running. | `5` (matching `session_limit_exceeded`) | -| `awaiting_abandoned_cleanup` | The public caller already got a `504` and the non-cancellable ACP request plus its cleanup have not settled yet. | the effective restore budget in seconds, clamped to `5`–`120` | +| `reason` | Meaning | `Retry-After` | +| ---------------------------- | ---------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------ | +| `restore_in_progress` | An ordinary restore is running. | `5` (matching `session_limit_exceeded`) | +| `awaiting_abandoned_cleanup` | The public caller already got a `504` and the non-cancellable ACP request plus its cleanup have not settled yet. | the timed-out operation's budget in seconds — restore, or initialization for a caller-supplied-id spawn — clamped to `5`–`120` | The public restore request is governed by `limits.sessionRestoreTimeoutMs` (default 60s). After a `504` the id stays fenced until the late ACP request and cleanup settle, so a client that keeps retrying at the ordinary 5-second cadence would spin against a 409 it cannot clear — honor the budget-derived hint that comes with `awaiting_abandoned_cleanup`. From 9bfba01068680021c2db4581d9f668670a3e1aa0 Mon Sep 17 00:00:00 2001 From: "jinye.djy" Date: Fri, 28 Aug 2026 10:19:45 +0800 Subject: [PATCH 4/6] codex: address PR review feedback (#10268) Co-authored-by: Qwen-Coder --- docs/developers/daemon/18-error-taxonomy.md | 2 +- docs/developers/qwen-serve-protocol.md | 12 +- packages/acp-bridge/src/bridge.test.ts | 185 ++++++++++++++++++++ packages/acp-bridge/src/bridge.ts | 117 +++++++++---- packages/core/src/config/config.test.ts | 28 +++ packages/core/src/config/config.ts | 10 ++ 6 files changed, 312 insertions(+), 42 deletions(-) diff --git a/docs/developers/daemon/18-error-taxonomy.md b/docs/developers/daemon/18-error-taxonomy.md index 9ad4fadb40f..c4f15b7320a 100644 --- a/docs/developers/daemon/18-error-taxonomy.md +++ b/docs/developers/daemon/18-error-taxonomy.md @@ -59,7 +59,7 @@ Typed classes thrown by the bridge / mediator. Most carry an HTTP status via the | `BridgeChannelClosedError` | 503 | ACP child channel closed mid-call. | Reconnect / retry; check `session_died` for cause. | | `BridgeTimeoutError` | 504 | Bridge-level wallclock exceeded. | Retry; investigate underlying slowness. | | `SessionRestoreTimeoutError` | 504 | ACP session load/resume exceeded its dedicated restore budget. | Retry after the advertised delay; inspect restore stage traces before raising the budget. | -| `BridgeChannelQuarantinedError` | 503 | `reason` is `restore_cleanup_failed`, `restore_settlement_overdue`, `new_session_cleanup_failed`, or `new_session_settlement_overdue`. Cleanup-failed states last until the workspace channel drains; settlement-overdue states clear when the abandoned request settles or the channel drains. The 503 body also carries `retryAfterSeconds`. | Keep using existing sessions and retry after the advertised delay; cleanup-failed states require channel recycle, while settlement-overdue states may recover when the request settles. | +| `BridgeChannelQuarantinedError` | 503 | `reason` is `restore_cleanup_failed`, `restore_settlement_overdue`, `new_session_cleanup_failed`, or `new_session_settlement_overdue`. A settlement-overdue state clears after a late failure settles or a late success completes exact-ID cleanup; inconclusive cleanup transitions to the matching cleanup-failed state. Cleanup-failed states last until the workspace channel drains. The 503 body also carries `retryAfterSeconds`. | Keep using existing sessions and retry after the advertised delay; cleanup-failed states require channel recycle, while settlement-overdue states may recover after settlement and any required cleanup complete. | | `MissingCliEntryError` | 500 | The `qwen` CLI entry file is missing (defined in `status.ts`, not `bridgeErrors.ts`). | Confirm the CLI install is complete; check that `packages/cli/index.ts` exists. | ## Boot-time configuration errors (`packages/cli/src/serve/run-qwen-serve.ts`) diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 890ea5df4e9..e5a1b1c5f1f 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -124,10 +124,10 @@ Fired when a `session/load` is issued for an id that already has a `session/resu `reason` distinguishes two fences that share this code, and the `Retry-After` header tracks it: -| `reason` | Meaning | `Retry-After` | -| ---------------------------- | ---------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------ | -| `restore_in_progress` | An ordinary restore is running. | `5` (matching `session_limit_exceeded`) | -| `awaiting_abandoned_cleanup` | The public caller already got a `504` and the non-cancellable ACP request plus its cleanup have not settled yet. | the timed-out operation's budget in seconds — restore, or initialization for a caller-supplied-id spawn — clamped to `5`–`120` | +| `reason` | Meaning | `Retry-After` | +| ---------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------ | +| `restore_in_progress` | An ordinary restore is running. | `5` (matching `session_limit_exceeded`) | +| `awaiting_abandoned_cleanup` | The public caller already got a timeout (`504` for restore, or `init_timeout` for session initialization) and the non-cancellable ACP request plus its cleanup have not settled yet. | the timed-out operation's budget in seconds — restore, or initialization for a caller-supplied-id spawn — clamped to `5`–`120` | The public restore request is governed by `limits.sessionRestoreTimeoutMs` (default 60s). After a `504` the id stays fenced until the late ACP request and cleanup settle, so a client that keeps retrying at the ordinary 5-second cadence would spin against a 409 it cannot clear — honor the budget-derived hint that comes with `awaiting_abandoned_cleanup`. @@ -2172,7 +2172,7 @@ Response: **History replay over SSE.** While `loadSession` is in flight on the agent side, the agent may emit `session_update` notifications for persisted turns, or return bulk replay updates in the response metadata. The daemon seeds those events into the session's bounded replay snapshot window before the route response returns. For live sessions, `POST /session/:id/load` only promises that bounded window (`compactedReplay`, `liveJournal`, `lastEventId`), not the full transcript. The window is byte-capped by `--compacted-replay-max-bytes` (default 4 MiB, maximum 256 MiB); if older replay entries were dropped, `compactedReplay[0]` is an id-less `history_truncated` marker. The in-flight `liveJournal` is separately capped by `--max-journal-events` (default 10 000 replay entries) and `--max-journal-bytes` (default 8 MiB of serialized source events). These are per-session **baseline** caps. When an in-flight turn outgrows them, the daemon first tries adaptive growth: it raises that session's caps toward double (up to a per-session hard cap of 256 MiB, entries scaled proportionally, limited by the remaining pool headroom) while the growth granted across every live session fits in one daemon-wide growth pool sized at 5% of the daemon's effective memory budget — the `--memory-budget-mb` value when passed, capped at resolved available memory, otherwise 50% of auto-detected memory — capped at `1024` MB. Accounting is daemon-wide — a multi-workspace daemon runs one bridge per workspace and all of them share the single pool. Growth is on demand and only as far as the pool allows; an operator-pinned `--max-journal-events` or `--max-journal-bytes` disables it, as does a host whose effective budget falls below the 1024 MB minimum (`insufficientMemory`): the pool is 0 and adaptive growth is disabled outright. Consecutive compatible `agent_message_chunk` or `agent_thought_chunk` source events share a replay entry, up to 256 source events per entry, while tool, attribution, provenance, and discrete-message boundaries remain intact. When the journal still exceeds its (possibly grown) caps after the growth the pool allows — including when no headroom is granted or a grant covers only part of the overshoot — the oldest entries are dropped whole (so the retained tail can be much smaller than the byte cap) and a `history_truncated` marker with `scope: 'live_journal'` is prepended; its `truncatedEvents` and `retainedEvents` fields count source events, not replay entries, and its `maxBytes` / `maxEvents` reflect the caps in force (which may already have grown). Clients should render that marker as status and continue applying retained events. Full persisted transcript access is exposed separately through `GET /session/:id/transcript`. -The replay-window byte caps apply after the child has reconstructed the persisted transcript; they do not cap the on-disk JSONL read. A restore that exceeds the daemon budget returns `504` with a `Retry-After` derived from the restore budget (clamped to 5-120s) and `{code: "session_restore_timeout", errorKind: "restore_timeout", retryable: true, sessionId, action, timeoutMs}`. The daemon fences the still-running ACP request and cleans up any late session instead of registering it. A retry for the same id returns `409 restore_in_progress` with `reason: "awaiting_abandoned_cleanup"` and a `Retry-After` of the restore budget (clamped to 5-120s) until that cleanup settles. If late restore cleanup is uncertain, or the abandoned restore has still not settled a full restore budget after its deadline, new sessions on that workspace return `503 acp_channel_unavailable` with `reason: "restore_cleanup_failed"` or `"restore_settlement_overdue"`. A timed-out session initialization follows the same fail-closed admission policy for an older ACP child that settles late: inconclusive cleanup returns `reason: "new_session_cleanup_failed"`, while a request that remains unsettled for one further initialization budget returns `reason: "new_session_settlement_overdue"`. Cleanup-failed states last until the workspace channel drains and is recycled; settlement-overdue states clear if the abandoned request settles cleanly first. Already-live sessions remain usable in either case. +The replay-window byte caps apply after the child has reconstructed the persisted transcript; they do not cap the on-disk JSONL read. A restore that exceeds the daemon budget returns `504` with a `Retry-After` derived from the restore budget (clamped to 5-120s) and `{code: "session_restore_timeout", errorKind: "restore_timeout", retryable: true, sessionId, action, timeoutMs}`. The daemon fences the still-running ACP request and cleans up any late session instead of registering it. A retry for the same id returns `409 restore_in_progress` with `reason: "awaiting_abandoned_cleanup"` and a `Retry-After` of the restore budget (clamped to 5-120s) until that cleanup settles. If late restore cleanup is uncertain, or the abandoned restore has still not settled a full restore budget after its deadline, new sessions on that workspace return `503 acp_channel_unavailable` with `reason: "restore_cleanup_failed"` or `"restore_settlement_overdue"`. A timed-out session initialization follows the same fail-closed admission policy for an older ACP child that settles late: inconclusive cleanup returns `reason: "new_session_cleanup_failed"`, while a request that remains unsettled for one further initialization budget returns `reason: "new_session_settlement_overdue"`. A settlement-overdue state clears immediately after a late failure settles, or after a late success completes its exact-ID cleanup; inconclusive cleanup transitions to the corresponding cleanup-failed state instead. Cleanup-failed states last until the workspace channel drains and is recycled. Already-live sessions remain usable in either case. **Errors:** @@ -2181,7 +2181,7 @@ The replay-window byte caps apply after the child has reconstructed the persiste - `403` — `untrusted_workspace` when `cwd` targets an untrusted non-primary workspace. - `503` — `session_limit_exceeded` (counts against `--max-sessions`; in-flight restores are accounted for too). - `504` — `session_restore_timeout`; retryable, with a `Retry-After` derived from the restore budget (clamped to 5-120s) because the same session id stays fenced until late cleanup settles. -- `503` — `acp_channel_unavailable` when the workspace channel is closed to new session work. `reason` says why: `restore_cleanup_failed` when an abandoned restore could not be cleaned up conclusively; `restore_settlement_overdue` when an abandoned restore has still not settled one full restore budget after its deadline; `new_session_cleanup_failed` when a session created after the public initialization timeout could not be closed conclusively; or `new_session_settlement_overdue` when the timed-out initialization has still not settled one further initialization budget after its deadline. Existing sessions remain available. Cleanup-failed states require the workspace channel to drain and recycle; settlement-overdue states clear when the abandoned request settles or the channel drains. The body carries `retryAfterSeconds` and the header a matching budget-derived `Retry-After` so clients use an operation-budget-scale backoff instead of polling at the ordinary 5-second cadence. +- `503` — `acp_channel_unavailable` when the workspace channel is closed to new session work. `reason` says why: `restore_cleanup_failed` when an abandoned restore could not be cleaned up conclusively; `restore_settlement_overdue` when an abandoned restore has still not settled one full restore budget after its deadline; `new_session_cleanup_failed` when a session created after the public initialization timeout could not be closed conclusively; or `new_session_settlement_overdue` when the timed-out initialization has still not settled one further initialization budget after its deadline. Existing sessions remain available. A settlement-overdue state clears after a late failure settles or a late success completes its exact-ID cleanup; inconclusive cleanup transitions to the matching cleanup-failed state, which requires the workspace channel to drain and recycle. The body carries `retryAfterSeconds` and the header a matching budget-derived `Retry-After` so clients use an operation-budget-scale backoff instead of polling at the ordinary 5-second cadence. - `409` — `restore_in_progress` (a `session/resume` for the same id is already in flight, or a fresh spawn supplied an id a restore owns). `Retry-After: 5` while the restore is active; a budget-derived hint once it is fenced as `awaiting_abandoned_cleanup`. Same-action races (two concurrent `session/load` for the same id) coalesce — exactly one returns `attached: false`, the rest return `attached: true` with the same `state`. - `409` — `session_workspace_conflict` when the same session id is already live or being restored by another workspace runtime. - `409` — `session_archived` when the id exists only under `chats/archive/`; call `POST /sessions/unarchive` before `load` or `resume`. diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 6a404b75df9..99b6a506aef 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -12853,6 +12853,31 @@ describe('createAcpSessionBridge', () => { } }); + it('returns an empty-channel timeout without waiting for channel kill', async () => { + vi.useFakeTimers(); + const handle = makeChannel({ + newSessionImpl: () => new Promise(() => {}), + }); + const kill = vi.fn(() => new Promise(() => {})); + handle.channel = { ...handle.channel, kill }; + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + }); + try { + const timedOut = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(20); + + expect(await timedOut).toBeInstanceOf(BridgeTimeoutError); + expect(kill).toHaveBeenCalledOnce(); + } finally { + handle.channel.killSync(); + vi.useRealTimers(); + } + }); + it('treats a missing late newSession as conclusively cleaned up', async () => { vi.useFakeTimers(); const late = deferred(); @@ -12966,6 +12991,117 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('does not close a late newSession id owned by an in-flight restore', async () => { + const lateNewSession = deferred(); + const restoring = deferred(); + const closeCalls: Array> = []; + const handle = makeChannel({ + newSessionImpl: (_params, agent) => + agent.newSessionCalls.length === 2 + ? lateNewSession.promise + : { sessionId: `visible-${agent.newSessionCalls.length}` }, + loadSessionImpl: () => restoring.promise, + extMethodImpl: (method, params) => { + if (method === SERVE_CONTROL_EXT_METHODS.sessionClose) { + closeCalls.push(params); + return { closed: true }; + } + return {}; + }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + sessionScope: 'thread', + }); + try { + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toBeInstanceOf(BridgeTimeoutError); + + const restore = bridge.loadSession({ + sessionId: 'restoring', + workspaceCwd: WS_A, + }); + await vi.waitFor(() => + expect(handle.agent.loadSessionCalls).toHaveLength(1), + ); + + lateNewSession.resolve({ sessionId: 'restoring' }); + await new Promise((resolve) => setImmediate(resolve)); + expect(closeCalls).toHaveLength(0); + + restoring.resolve({}); + await expect(restore).resolves.toMatchObject({ + sessionId: 'restoring', + }); + await new Promise((resolve) => setImmediate(resolve)); + expect(closeCalls).toHaveLength(0); + expect(() => bridge.getSessionSummary('restoring')).not.toThrow(); + } finally { + await bridge.shutdown(); + } + }); + + it('does not close a late newSession id owned by an in-flight spawn', async () => { + vi.useFakeTimers(); + const lateAnonymous = deferred(); + const requestedSpawn = deferred(); + const closeCalls: Array> = []; + const handle = makeChannel({ + newSessionImpl: (_params, agent) => { + if (agent.newSessionCalls.length === 2) return lateAnonymous.promise; + if (agent.newSessionCalls.length === 3) return requestedSpawn.promise; + return { sessionId: `visible-${agent.newSessionCalls.length}` }; + }, + extMethodImpl: (method, params) => { + if (method === SERVE_CONTROL_EXT_METHODS.sessionClose) { + closeCalls.push(params); + return { closed: true }; + } + return {}; + }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + sessionScope: 'thread', + }); + try { + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const timedOut = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(20); + expect(await timedOut).toBeInstanceOf(BridgeTimeoutError); + + const spawning = bridge.spawnOrAttach({ + workspaceCwd: WS_A, + sessionId: 'requested-in-flight', + }); + await vi.advanceTimersByTimeAsync(0); + expect(handle.agent.newSessionCalls).toHaveLength(3); + + lateAnonymous.resolve({ sessionId: 'requested-in-flight' }); + await vi.advanceTimersByTimeAsync(0); + expect(closeCalls).toHaveLength(0); + + requestedSpawn.resolve({ sessionId: 'requested-in-flight' }); + await expect(spawning).resolves.toMatchObject({ + sessionId: 'requested-in-flight', + }); + await vi.advanceTimersByTimeAsync(0); + expect(closeCalls).toHaveLength(0); + expect(() => + bridge.getSessionSummary('requested-in-flight'), + ).not.toThrow(); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } + }); + it('keeps the public timeout contract when the agent enforces the deadline', async () => { const newSessionEvents = recordNewSessionEvents(); const handle = makeChannel({ @@ -13158,6 +13294,55 @@ describe('createAcpSessionBridge', () => { } }); + it('tracks settlement overdue state per abandoned newSession', async () => { + vi.useFakeTimers(); + const lateA = deferred(); + const lateB = deferred(); + const handle = makeChannel({ + newSessionImpl: (_params, agent) => { + if (agent.newSessionCalls.length === 2) return lateA.promise; + if (agent.newSessionCalls.length === 3) return lateB.promise; + return { sessionId: `visible-${agent.newSessionCalls.length}` }; + }, + }); + const bridge = makeBridge({ + channelFactory: async () => handle.channel, + initializeTimeoutMs: 20, + sessionScope: 'thread', + }); + try { + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const abandonedA = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(10); + const abandonedB = bridge + .spawnOrAttach({ workspaceCwd: WS_A }) + .catch((error: unknown) => error); + + await vi.advanceTimersByTimeAsync(10); + expect(await abandonedA).toBeInstanceOf(BridgeTimeoutError); + await vi.advanceTimersByTimeAsync(10); + expect(await abandonedB).toBeInstanceOf(BridgeTimeoutError); + await vi.advanceTimersByTimeAsync(10); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ reason: 'new_session_settlement_overdue' }); + + lateA.reject(new Error('late failure A')); + await vi.advanceTimersByTimeAsync(0); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).resolves.toMatchObject({ sessionId: 'visible-4' }); + + lateB.reject(new Error('late failure B')); + await vi.advanceTimersByTimeAsync(0); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } + }); + it.each(['load', 'resume'] as const)( 'counts an abandoned newSession against %s admission capacity', async (action) => { diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index e8aba987452..b320502739b 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -943,8 +943,8 @@ interface ChannelInfo { restoreSettlementTimers: Map; /** Timed-out newSession requests whose underlying ACP call is still live. */ unsettledAbandonedNewSessions: Set; - /** Set once an abandoned newSession outlives one further init budget. */ - newSessionSettlementOverdue: boolean; + /** Abandoned newSession requests that outlived one further init budget. */ + overdueAbandonedNewSessions: Set; /** Grace timers armed at newSession abandonment. */ newSessionSettlementTimers: Map; /** A late-created session could not be closed deterministically. */ @@ -2654,7 +2654,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { if (ci.newSessionCleanupFailed) { return { channel: ci, reason: 'new_session_cleanup_failed' }; } - if (ci.newSessionSettlementOverdue) { + if (ci.overdueAbandonedNewSessions.size > 0) { return { channel: ci, reason: 'new_session_settlement_overdue' }; } } @@ -3484,7 +3484,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ci.isQuarantined || ci.restoreSettlementOverdue || ci.newSessionCleanupFailed || - ci.newSessionSettlementOverdue + ci.overdueAbandonedNewSessions.size > 0 ); } @@ -3538,7 +3538,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ci.newSessionSettlementTimers.delete(token); if (!ci.unsettledAbandonedNewSessions.has(token)) return; if (ci.isDying || !aliveChannels.has(ci)) return; - ci.newSessionSettlementOverdue = true; + ci.overdueAbandonedNewSessions.add(token); writeStderrLine( `qwen serve: abandoned newSession${requestedSessionId ? ` for ${JSON.stringify(requestedSessionId)}` : ''} has not settled ` + `${newSessionSettlementGraceMs}ms after its deadline; refusing fresh sessions on channel ${ci.id} until it settles or drains`, @@ -3873,12 +3873,15 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // `shutdown()`. const inFlightSpawns = new Map>(); const abandonedNewSessionSettlements = new Set>(); - // Reserves caller-supplied ids before `doSpawn` reaches its first await. - // Restore admission checks the same set, closing the opposite race from - // `inFlightRestores`: whichever operation reserves the id first owns its - // registration window. - const inFlightRequestedSessionSpawns = new Map(); - const abandonedRequestedSessionSpawns = new Set(); + // Reserves ids before caller-supplied spawns and exact-id late cleanup + // reach their first await. Restore admission checks the same map, closing + // the opposite race from `inFlightRestores`: whichever operation reserves + // the id first owns its registration window. + const inFlightSessionIdReservations = new Map< + string, + { token: symbol; settlementPromise: Promise } + >(); + const abandonedSessionIdReservations = new Set(); interface InFlightRestore { action: 'load' | 'resume'; @@ -4250,7 +4253,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { restoreSettlementOverdue: false, restoreSettlementTimers: new Map(), unsettledAbandonedNewSessions: new Set(), - newSessionSettlementOverdue: false, + overdueAbandonedNewSessions: new Set(), newSessionSettlementTimers: new Map(), newSessionCleanupFailed: false, transportFailed: false, @@ -4652,8 +4655,34 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ? { 'session.id': requestedSessionId } : {}), }); + let cleanupReservation: symbol | undefined; + let resolveCleanupReservation: (() => void) | undefined; try { if (!lateSessionId) return; + while (!byId.has(lateSessionId)) { + const restoreOwner = inFlightRestores.get(lateSessionId); + if (restoreOwner) { + await restoreOwner.settlementPromise.catch(() => undefined); + continue; + } + const spawnOwner = inFlightSessionIdReservations.get(lateSessionId); + if (spawnOwner && lateSessionId !== requestedSessionId) { + await spawnOwner.settlementPromise; + continue; + } + if (!spawnOwner) { + cleanupReservation = Symbol(lateSessionId); + const cleanupSettlement = new Promise((resolve) => { + resolveCleanupReservation = resolve; + }); + inFlightSessionIdReservations.set(lateSessionId, { + token: cleanupReservation, + settlementPromise: cleanupSettlement, + }); + abandonedSessionIdReservations.add(cleanupReservation); + } + break; + } if (byId.has(lateSessionId)) { writeStderrLine( `qwen serve: skipping abandoned newSession cleanup for ${JSON.stringify(lateSessionId)}: the id is owned by a live session`, @@ -4735,15 +4764,24 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ci.client.markSessionClosed(lateSessionId); } } finally { + if (cleanupReservation !== undefined) { + if ( + lateSessionId !== undefined && + inFlightSessionIdReservations.get(lateSessionId)?.token === + cleanupReservation + ) { + inFlightSessionIdReservations.delete(lateSessionId); + } + abandonedSessionIdReservations.delete(cleanupReservation); + resolveCleanupReservation?.(); + } ci.unsettledAbandonedNewSessions.delete(token); + ci.overdueAbandonedNewSessions.delete(token); const graceTimer = ci.newSessionSettlementTimers.get(token); if (graceTimer !== undefined) { clearTimeout(graceTimer); ci.newSessionSettlementTimers.delete(token); } - if (ci.unsettledAbandonedNewSessions.size === 0) { - ci.newSessionSettlementOverdue = false; - } void reapPendingEmptyChannel(ci); } } @@ -4807,6 +4845,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } let sessionRegistered = false; let sessionRemovedDuringInitialization = false; + let emptyFailureTeardownStarted = false; let initializedSessionId: string | undefined; const abandonedToken = Symbol(requestedSessionId ?? 'newSession'); let newSessionResp: { @@ -4957,11 +4996,8 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // attaching to the one we're about to tear down. `channelInfo` // stays set until OS reap so `killAllSync` mid-SIGTERM still // finds a target (BkUyD invariant). - ci.isDying = true; - ci.channelLiveness?.stop(); - await ci.channel.kill().catch(() => { - /* best-effort — channel.exited handler still runs */ - }); + emptyFailureTeardownStarted = true; + void killChannelWithLog(ci, 'empty newSession failure'); } else { ci.emptyReapPending = true; } @@ -5200,7 +5236,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { } ci.sessionSpawnsInFlight = Math.max(0, ci.sessionSpawnsInFlight - 1); if (!sessionRegistered) { - await reapPendingEmptyChannel(ci); + if (!emptyFailureTeardownStarted) { + await reapPendingEmptyChannel(ci); + } } else if (sessionRemovedDuringInitialization && hasNoChannelWork(ci)) { await reapPendingEmptyChannel(ci); if (!ci.isDying) { @@ -7193,13 +7231,13 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { }; } - if (inFlightRequestedSessionSpawns.has(req.sessionId)) { - const owner = inFlightRequestedSessionSpawns.get(req.sessionId); + if (inFlightSessionIdReservations.has(req.sessionId)) { + const owner = inFlightSessionIdReservations.get(req.sessionId); throw new RestoreInProgressError( req.sessionId, 'spawn', action, - owner !== undefined && abandonedRequestedSessionSpawns.has(owner) + owner !== undefined && abandonedSessionIdReservations.has(owner.token) ? { reason: 'awaiting_abandoned_cleanup', retryAfterSeconds: abandonedNewSessionRetryAfterSeconds, @@ -8848,13 +8886,14 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { : undefined, ); } - if (inFlightRequestedSessionSpawns.has(req.sessionId)) { - const owner = inFlightRequestedSessionSpawns.get(req.sessionId); + if (inFlightSessionIdReservations.has(req.sessionId)) { + const owner = inFlightSessionIdReservations.get(req.sessionId); throw new RestoreInProgressError( req.sessionId, 'spawn', 'spawn', - owner !== undefined && abandonedRequestedSessionSpawns.has(owner) + owner !== undefined && + abandonedSessionIdReservations.has(owner.token) ? { reason: 'awaiting_abandoned_cleanup', retryAfterSeconds: abandonedNewSessionRetryAfterSeconds, @@ -8880,28 +8919,36 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { const requestedSessionRegistrationOwner = req.sessionId !== undefined ? Symbol(req.sessionId) : undefined; + let resolveRequestedSessionSpawnSettlement: (() => void) | undefined; if ( req.sessionId !== undefined && requestedSessionRegistrationOwner !== undefined ) { - inFlightRequestedSessionSpawns.set( - req.sessionId, - requestedSessionRegistrationOwner, - ); + const requestedSessionSpawnSettlement = new Promise((resolve) => { + resolveRequestedSessionSpawnSettlement = resolve; + }); + inFlightSessionIdReservations.set(req.sessionId, { + token: requestedSessionRegistrationOwner, + settlementPromise: requestedSessionSpawnSettlement, + }); } const releaseRequestedSessionRegistration = () => { if (requestedSessionRegistrationOwner !== undefined) { - abandonedRequestedSessionSpawns.delete( + abandonedSessionIdReservations.delete( requestedSessionRegistrationOwner, ); } if ( req.sessionId !== undefined && requestedSessionRegistrationOwner !== undefined && - inFlightRequestedSessionSpawns.get(req.sessionId) === + inFlightSessionIdReservations.get(req.sessionId)?.token === requestedSessionRegistrationOwner ) { - inFlightRequestedSessionSpawns.delete(req.sessionId); + inFlightSessionIdReservations.delete(req.sessionId); + } + if (requestedSessionRegistrationOwner !== undefined) { + resolveRequestedSessionSpawnSettlement?.(); + resolveRequestedSessionSpawnSettlement = undefined; } }; let admission: BridgeFreshSessionReservation | undefined; @@ -8943,7 +8990,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { (settlement) => { abandonedSettlement = settlement; if (requestedSessionRegistrationOwner !== undefined) { - abandonedRequestedSessionSpawns.add( + abandonedSessionIdReservations.add( requestedSessionRegistrationOwner, ); } diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index f5b5b6cd2b4..011cbe9efd9 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -3984,6 +3984,34 @@ describe('Server Config (config.ts)', () => { ).toEqual([initializationError, closeError]); }); + it('preserves initialization cancellation when recording close fails', async () => { + const config = new Config(baseParams); + const controller = new AbortController(); + const abortReason = new Error('session initialization deadline exceeded'); + const closeError = new Error('recording close failed'); + vi.spyOn( + config as unknown as { + initializeInternal: (options?: { + signal?: AbortSignal; + }) => Promise; + }, + 'initializeInternal', + ).mockImplementation(async (options) => { + controller.abort(abortReason); + options?.signal?.throwIfAborted(); + }); + const close = vi + .spyOn(config, 'closeSessionWriter') + .mockRejectedValue(closeError); + + const result = await config + .initialize({ signal: controller.signal }) + .catch((error: unknown) => error); + + expect(result).toBe(abortReason); + expect(close).toHaveBeenCalledOnce(); + }); + it('runs due auto-skill curation before loading skills when enabled', async () => { const config = new Config({ ...baseParams, enableAutoSkill: true }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index c4b508c0851..caede021eff 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -3092,6 +3092,16 @@ export class Config { try { await this.closeSessionWriter(); } catch (closeError) { + if ( + options?.signal?.aborted && + containsErrorByIdentity(error, options.signal.reason) + ) { + this.debugLogger.warn( + 'Chat recording close failed after initialization was aborted:', + closeError, + ); + options.signal.throwIfAborted(); + } if (containsErrorByIdentity(error, closeError)) { throw error; } From 23a2637d261981b9365f24af18313f07d81f0807 Mon Sep 17 00:00:00 2001 From: "jinye.djy" Date: Fri, 28 Aug 2026 15:38:11 +0800 Subject: [PATCH 5/6] codex: address PR review feedback (#10268) Co-authored-by: Qwen-Coder --- .../src/serve/acp-http/dispatch-error.test.ts | 17 +++++++++++++++++ packages/cli/src/serve/acp-http/dispatch.ts | 16 ++++++++++++++++ packages/cli/src/serve/acp-session-bridge.ts | 5 ++++- .../src/serve/server/error-response.test.ts | 18 ++++++++++++++++++ .../cli/src/serve/server/error-response.ts | 13 +++++++++++++ 5 files changed, 68 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/serve/acp-http/dispatch-error.test.ts b/packages/cli/src/serve/acp-http/dispatch-error.test.ts index afedc0bbe5f..19a8875e910 100644 --- a/packages/cli/src/serve/acp-http/dispatch-error.test.ts +++ b/packages/cli/src/serve/acp-http/dispatch-error.test.ts @@ -10,6 +10,7 @@ import { DaemonDrainingError } from '../server/session-archive.js'; import { StandaloneSessionServiceError } from '../conversations/standalone-session-service.js'; import { BridgeChannelQuarantinedError, + BridgeTimeoutError, InvalidSessionMetadataError, RestoreInProgressError, SessionRestoreTimeoutError, @@ -69,6 +70,22 @@ describe('toRpcError', () => { }); }); + it('maps session initialization timeouts with the public retry contract', () => { + const error = new BridgeTimeoutError('newSession', 10_000); + expect(toRpcError(error)).toEqual({ + code: RPC.INTERNAL_ERROR, + message: error.message, + data: { + code: 'init_timeout', + errorKind: 'init_timeout', + httpStatus: 504, + retryable: true, + retryAfterSeconds: 10, + timeoutMs: 10_000, + }, + }); + }); + it('maps the abandoned-restore fence with its reason and hint', () => { // SDK transport negotiation prefers acp-ws and acp-http over REST, so // without this mapping the default arm turns a retryable fence into an diff --git a/packages/cli/src/serve/acp-http/dispatch.ts b/packages/cli/src/serve/acp-http/dispatch.ts index 1c6b6e6dd3b..64e1c5548e9 100644 --- a/packages/cli/src/serve/acp-http/dispatch.ts +++ b/packages/cli/src/serve/acp-http/dispatch.ts @@ -40,6 +40,7 @@ import { } from '../acp-session-bridge.js'; import type { BridgeChannelQuarantinedError, + BridgeTimeoutError, RestoreInProgressError, SessionRestoreTimeoutError, } from '../acp-session-bridge.js'; @@ -837,6 +838,21 @@ export function toRpcError(err: unknown): { }, }; } + case 'BridgeTimeoutError': { + const timeoutError = err as BridgeTimeoutError; + return { + code: RPC.INTERNAL_ERROR, + message: timeoutError.message, + data: { + code: 'init_timeout', + errorKind: 'init_timeout', + httpStatus: 504, + retryable: true, + retryAfterSeconds: restoreRetryAfterSeconds(timeoutError.timeoutMs), + timeoutMs: timeoutError.timeoutMs, + }, + }; + } case 'RestoreInProgressError': { // Without this case the fence degrades to the default arm — an opaque // `internal` 500 with no code, reason, or hint. SDK transport diff --git a/packages/cli/src/serve/acp-session-bridge.ts b/packages/cli/src/serve/acp-session-bridge.ts index dd6b8bdcc0f..d9ea2498826 100644 --- a/packages/cli/src/serve/acp-session-bridge.ts +++ b/packages/cli/src/serve/acp-session-bridge.ts @@ -148,7 +148,10 @@ export { SessionShellDisabledError, } from '@qwen-code/acp-bridge/bridgeErrors'; -export { SessionRestoreTimeoutError } from '@qwen-code/acp-bridge/status'; +export { + BridgeTimeoutError, + SessionRestoreTimeoutError, +} from '@qwen-code/acp-bridge/status'; export { MAX_WORKSPACE_PATH_LENGTH, diff --git a/packages/cli/src/serve/server/error-response.test.ts b/packages/cli/src/serve/server/error-response.test.ts index 4271c898049..18c8ba5be79 100644 --- a/packages/cli/src/serve/server/error-response.test.ts +++ b/packages/cli/src/serve/server/error-response.test.ts @@ -16,6 +16,7 @@ import { } from '@qwen-code/qwen-code-core'; import { sendBridgeError } from './error-response.js'; import { DaemonDrainingError } from './session-archive.js'; +import { BridgeTimeoutError } from '../acp-session-bridge.js'; import { StandaloneSessionServiceError } from '../conversations/standalone-session-service.js'; import { ConversationRuntimeOwnershipError } from '../conversations/conversation-runtime-errors.js'; import type { DaemonLogger } from '../daemon-logger.js'; @@ -71,6 +72,23 @@ describe('sendBridgeError session writer errors', () => { }); }); + it('maps session initialization timeouts with the public retry contract', () => { + const { response, status, json, set } = responseMock(); + const error = new BridgeTimeoutError('newSession', 10_000); + + sendBridgeError(response, error); + + expect(set).toHaveBeenCalledWith('Retry-After', '10'); + expect(status).toHaveBeenCalledWith(504); + expect(json).toHaveBeenCalledWith({ + error: error.message, + code: 'init_timeout', + errorKind: 'init_timeout', + retryable: true, + timeoutMs: 10_000, + }); + }); + it.each([ ['conversation_runtime_in_use', true], ['conversation_runtime_unavailable', true], diff --git a/packages/cli/src/serve/server/error-response.ts b/packages/cli/src/serve/server/error-response.ts index ae8ff458955..0104282d215 100644 --- a/packages/cli/src/serve/server/error-response.ts +++ b/packages/cli/src/serve/server/error-response.ts @@ -22,6 +22,7 @@ import { writeStderrLine } from '../../utils/stdioHelpers.js'; import { BranchWhilePromptActiveError, BridgeChannelQuarantinedError, + BridgeTimeoutError, CancelSentinelCollisionError, CdWhilePromptActiveError, InvalidClientIdError, @@ -213,6 +214,18 @@ export function sendBridgeError( }); return; } + if (err instanceof BridgeTimeoutError) { + recordExpectedBridgeError(err, ctx, daemonLog); + res.set('Retry-After', String(restoreRetryAfterSeconds(err.timeoutMs))); + res.status(504).json({ + error: err.message, + code: 'init_timeout', + errorKind: 'init_timeout', + retryable: true, + timeoutMs: err.timeoutMs, + }); + return; + } if (err instanceof BridgeChannelQuarantinedError) { recordExpectedBridgeError(err, ctx, daemonLog); // Quarantine lasts until the channel drains, which is strictly longer than From 93c909cede7056c5566b291ef0ad361de5c52a4c Mon Sep 17 00:00:00 2001 From: "jinye.djy" Date: Sat, 29 Aug 2026 06:48:28 +0800 Subject: [PATCH 6/6] codex: address PR review feedback (#10268) Co-authored-by: Qwen-Coder --- packages/acp-bridge/src/bridge.test.ts | 110 ++++++++++++++++++ packages/acp-bridge/src/bridge.ts | 32 +++-- .../src/serve/acp-http/dispatch-error.test.ts | 8 ++ packages/cli/src/serve/acp-http/dispatch.ts | 7 ++ .../src/serve/server/error-response.test.ts | 11 ++ .../cli/src/serve/server/error-response.ts | 2 +- packages/core/src/config/config.test.ts | 14 ++- packages/core/src/config/config.ts | 4 +- .../hooks/instructionsLoadedCallback.test.ts | 3 + .../src/hooks/instructionsLoadedCallback.ts | 2 + 10 files changed, 172 insertions(+), 21 deletions(-) diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index 99b6a506aef..6e82a6694a6 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -10341,6 +10341,7 @@ describe('createAcpSessionBridge', () => { const releaseAdmission = vi.fn(); const handle = makeChannel({ loadSessionImpl: () => lateRestore.promise, + extMethodImpl: activeWorkCloseImpl, }); const restoreEvents = recordRestoreEvents(); const bridge = makeBridge({ @@ -10654,6 +10655,7 @@ describe('createAcpSessionBridge', () => { const lateRestore = deferred(); const handle = makeChannel({ loadSessionImpl: () => lateRestore.promise, + extMethodImpl: activeWorkCloseImpl, newSessionImpl: async (params) => { const requestedSessionId = typeof params._meta === 'object' && params._meta !== null @@ -10724,6 +10726,7 @@ describe('createAcpSessionBridge', () => { const lateRestore = deferred(); const handle = makeChannel({ loadSessionImpl: () => lateRestore.promise, + extMethodImpl: activeWorkCloseImpl, newSessionImpl: async (params) => { const requestedSessionId = typeof params._meta === 'object' && params._meta !== null @@ -10927,11 +10930,67 @@ describe('createAcpSessionBridge', () => { } }); + it('clears overdue state per restore while another remains within grace', async () => { + vi.useFakeTimers(); + const lateA = deferred(); + const lateB = deferred(); + const handle = makeChannel({ + loadSessionImpl: (params) => + params.sessionId === 'overdue-a' ? lateA.promise : lateB.promise, + extMethodImpl: (method) => + method === SERVE_CONTROL_EXT_METHODS.sessionClose + ? { closed: true } + : {}, + }); + const bridge = makeBridge({ + sessionScope: 'thread', + maxSessions: 5, + sessionRestoreTimeoutMs: 20, + channelFactory: async () => handle.channel, + }); + try { + await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const restoreA = bridge + .loadSession({ sessionId: 'overdue-a', workspaceCwd: WS_A }) + .catch((error: unknown) => error); + await vi.advanceTimersByTimeAsync(10); + const restoreB = bridge + .loadSession({ sessionId: 'within-grace-b', workspaceCwd: WS_A }) + .catch((error: unknown) => error); + + await vi.advanceTimersByTimeAsync(10); + expect(await restoreA).toBeInstanceOf(SessionRestoreTimeoutError); + await vi.advanceTimersByTimeAsync(10); + expect(await restoreB).toBeInstanceOf(SessionRestoreTimeoutError); + await vi.advanceTimersByTimeAsync(10); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ reason: 'restore_settlement_overdue' }); + + lateA.resolve({}); + await vi.advanceTimersByTimeAsync(0); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).resolves.toMatchObject({ sessionId: expect.any(String) }); + + await vi.advanceTimersByTimeAsync(10); + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ reason: 'restore_settlement_overdue' }); + lateB.reject(new Error('late failure')); + await vi.advanceTimersByTimeAsync(0); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } + }); + it('stops condemning a channel once its abandoned restore settles cleanly', async () => { vi.useFakeTimers(); const lateRestore = deferred(); const handle = makeChannel({ loadSessionImpl: () => lateRestore.promise, + extMethodImpl: activeWorkCloseImpl, }); const bridge = makeBridge({ sessionScope: 'thread', @@ -11205,6 +11264,7 @@ describe('createAcpSessionBridge', () => { params.sessionId === 'restore-first' ? firstRestore.promise : secondRestore.promise, + extMethodImpl: activeWorkCloseImpl, }); const bridge = makeBridge({ sessionScope: 'thread', @@ -11312,6 +11372,56 @@ describe('createAcpSessionBridge', () => { } }); + it('quarantines fresh work when late restore cleanup is refused', async () => { + vi.useFakeTimers(); + const lateRestore = deferred(); + const handle = makeChannel({ + loadSessionImpl: () => lateRestore.promise, + extMethodImpl: (method, params) => { + if ( + method === SERVE_CONTROL_EXT_METHODS.sessionClose && + params['sessionId'] === 'restore-close-refused' + ) { + return { closed: false, holds: [agentHold('late-agent')] }; + } + return { closed: true, holds: [] }; + }, + }); + const bridge = makeBridge({ + sessionScope: 'thread', + sessionRestoreTimeoutMs: 20, + channelFactory: async () => handle.channel, + }); + try { + const sibling = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); + const timedOut = bridge + .loadSession({ + sessionId: 'restore-close-refused', + workspaceCwd: WS_A, + }) + .catch((error: unknown) => error); + await advanceRestoreDeadline(20); + expect(await timedOut).toBeInstanceOf(SessionRestoreTimeoutError); + + lateRestore.resolve({}); + await vi.advanceTimersByTimeAsync(0); + + await expect( + bridge.spawnOrAttach({ workspaceCwd: WS_A }), + ).rejects.toMatchObject({ reason: 'restore_cleanup_failed' }); + await expect( + bridge.sendPrompt(sibling.sessionId, { + sessionId: sibling.sessionId, + prompt: [{ type: 'text', text: 'still alive' }], + }), + ).resolves.toMatchObject({ stopReason: 'end_turn' }); + expect(handle.killed).toBe(false); + } finally { + await bridge.shutdown(); + vi.useRealTimers(); + } + }); + it('quarantines only fresh work when late restore cleanup fails', async () => { vi.useFakeTimers(); const lateRestore = deferred(); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index b320502739b..638dfd2cd65 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -933,12 +933,11 @@ interface ChannelInfo { */ unsettledAbandonedRestores: Set; /** - * Set when an abandoned restore has outlived its settlement grace period. - * Existing sessions keep working; fresh session work is refused so the - * channel can drain and be recycled, which is what finally closes the - * transport out from under the request we cannot cancel. + * Abandoned restore ids that outlived their settlement grace. Existing + * sessions keep working; fresh session work is refused while this is + * non-empty so the channel can drain and be recycled. */ - restoreSettlementOverdue: boolean; + overdueAbandonedRestores: Set; /** Grace timers armed at restore abandonment, keyed by session id. */ restoreSettlementTimers: Map; /** Timed-out newSession requests whose underlying ACP call is still live. */ @@ -2648,7 +2647,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { if (ci.isQuarantined) { return { channel: ci, reason: 'restore_cleanup_failed' }; } - if (ci.restoreSettlementOverdue) { + if (ci.overdueAbandonedRestores.size > 0) { return { channel: ci, reason: 'restore_settlement_overdue' }; } if (ci.newSessionCleanupFailed) { @@ -3482,7 +3481,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { function channelIsCondemned(ci: ChannelInfo): boolean { return ( ci.isQuarantined || - ci.restoreSettlementOverdue || + ci.overdueAbandonedRestores.size > 0 || ci.newSessionCleanupFailed || ci.overdueAbandonedNewSessions.size > 0 ); @@ -3509,7 +3508,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ci.restoreSettlementTimers.delete(sessionId); if (!ci.unsettledAbandonedRestores.has(sessionId)) return; if (ci.isDying || !aliveChannels.has(ci)) return; - ci.restoreSettlementOverdue = true; + ci.overdueAbandonedRestores.add(sessionId); writeStderrLine( `qwen serve: abandoned session/${action} for ${JSON.stringify(sessionId)} has not settled ` + `${restoreSettlementGraceMs}ms after its deadline; refusing fresh sessions on channel ${ci.id} until it settles or drains`, @@ -4250,7 +4249,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { workspaceMcpAuthenticationTimers: new Map(), emptyReapPending: false, unsettledAbandonedRestores: new Set(), - restoreSettlementOverdue: false, + overdueAbandonedRestores: new Set(), restoreSettlementTimers: new Map(), unsettledAbandonedNewSessions: new Set(), overdueAbandonedNewSessions: new Set(), @@ -7462,9 +7461,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { clearTimeout(reclaimedTimer); channel.restoreSettlementTimers.delete(req.sessionId); } - if (channel.unsettledAbandonedRestores.size === 0) { - channel.restoreSettlementOverdue = false; - } + channel.overdueAbandonedRestores.delete(req.sessionId); releaseAdmissionOnce(); resolveSettlement(); return; @@ -7484,7 +7481,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { return; } try { - await Promise.race([ + const closeResult = await Promise.race([ withTimeout( channel.connection.extMethod( SERVE_CONTROL_EXT_METHODS.sessionClose, @@ -7498,6 +7495,9 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { ), getChannelClosedReject(channel), ]); + if (!isRecord(closeResult) || closeResult['closed'] !== true) { + throw new Error('ACP child refused abandoned restore cleanup'); + } telemetry.event('session.restore.cleanup', { 'qwen-code.daemon.session_restore.action': action, 'qwen-code.daemon.session_restore.cleanup_result': 'closed', @@ -7565,11 +7565,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { clearTimeout(graceTimer); channel.restoreSettlementTimers.delete(req.sessionId); } - // Only the last overdue restore lifts the admission block; a second - // one still outstanding keeps the channel closed to fresh work. - if (channel.unsettledAbandonedRestores.size === 0) { - channel.restoreSettlementOverdue = false; - } + channel.overdueAbandonedRestores.delete(req.sessionId); releaseAdmissionOnce(); resolveSettlement(); } diff --git a/packages/cli/src/serve/acp-http/dispatch-error.test.ts b/packages/cli/src/serve/acp-http/dispatch-error.test.ts index 19a8875e910..d04019c82bf 100644 --- a/packages/cli/src/serve/acp-http/dispatch-error.test.ts +++ b/packages/cli/src/serve/acp-http/dispatch-error.test.ts @@ -86,6 +86,14 @@ describe('toRpcError', () => { }); }); + it('leaves non-session-initialization bridge timeouts on the generic path', () => { + expect(toRpcError(new BridgeTimeoutError('initialize', 10_000))).toEqual({ + code: RPC.INTERNAL_ERROR, + message: 'Internal error', + data: { errorKind: 'internal' }, + }); + }); + it('maps the abandoned-restore fence with its reason and hint', () => { // SDK transport negotiation prefers acp-ws and acp-http over REST, so // without this mapping the default arm turns a retryable fence into an diff --git a/packages/cli/src/serve/acp-http/dispatch.ts b/packages/cli/src/serve/acp-http/dispatch.ts index 64e1c5548e9..1561f3e754d 100644 --- a/packages/cli/src/serve/acp-http/dispatch.ts +++ b/packages/cli/src/serve/acp-http/dispatch.ts @@ -840,6 +840,13 @@ export function toRpcError(err: unknown): { } case 'BridgeTimeoutError': { const timeoutError = err as BridgeTimeoutError; + if (timeoutError.label !== 'newSession') { + return { + code: RPC.INTERNAL_ERROR, + message: 'Internal error', + data: { errorKind: 'internal' }, + }; + } return { code: RPC.INTERNAL_ERROR, message: timeoutError.message, diff --git a/packages/cli/src/serve/server/error-response.test.ts b/packages/cli/src/serve/server/error-response.test.ts index 18c8ba5be79..890e1f4f69d 100644 --- a/packages/cli/src/serve/server/error-response.test.ts +++ b/packages/cli/src/serve/server/error-response.test.ts @@ -89,6 +89,17 @@ describe('sendBridgeError session writer errors', () => { }); }); + it('leaves non-session-initialization bridge timeouts on the generic path', () => { + const { response, status, json, set } = responseMock(); + const error = new BridgeTimeoutError('initialize', 10_000); + + sendBridgeError(response, error); + + expect(set).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(500); + expect(json).toHaveBeenCalledWith({ error: error.message }); + }); + it.each([ ['conversation_runtime_in_use', true], ['conversation_runtime_unavailable', true], diff --git a/packages/cli/src/serve/server/error-response.ts b/packages/cli/src/serve/server/error-response.ts index 0104282d215..8c09e2068d8 100644 --- a/packages/cli/src/serve/server/error-response.ts +++ b/packages/cli/src/serve/server/error-response.ts @@ -214,7 +214,7 @@ export function sendBridgeError( }); return; } - if (err instanceof BridgeTimeoutError) { + if (err instanceof BridgeTimeoutError && err.label === 'newSession') { recordExpectedBridgeError(err, ctx, daemonLog); res.set('Retry-After', String(restoreRetryAfterSeconds(err.timeoutMs))); res.status(504).json({ diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 011cbe9efd9..064b65dbb0c 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -236,6 +236,7 @@ vi.mock('../hooks/index.js', () => { getHookSystem: () => { fireInstructionsLoadedEvent?: (...args: unknown[]) => unknown; }, + signal?: AbortSignal, ) => async (notification: { filePath: string; @@ -252,6 +253,7 @@ vi.mock('../hooks/index.js', () => { triggerFilePath: notification.triggerFilePath, parentFilePath: notification.parentFilePath, }, + signal, ); }, }; @@ -4402,6 +4404,10 @@ describe('Server Config (config.ts)', () => { const config = new Config(baseParams); const controller = new AbortController(); const abortReason = new Error('initialization deadline exceeded'); + const refreshHierarchicalMemory = vi.spyOn( + config, + 'refreshHierarchicalMemory', + ); let markGeminiEntered!: () => void; const geminiEntered = new Promise((resolve) => { markGeminiEntered = resolve; @@ -4431,6 +4437,10 @@ describe('Server Config (config.ts)', () => { undefined, controller.signal, ); + expect(refreshHierarchicalMemory).toHaveBeenCalledWith( + 'session_start', + controller.signal, + ); await config.shutdown({ shutdownTelemetry: false }); }); @@ -7898,6 +7908,7 @@ describe('Server Config (config.ts)', () => { it('refreshHierarchicalMemory should fire InstructionsLoaded hooks from memory notifications', async () => { const config = new Config(baseParams); const fireInstructionsLoadedEvent = vi.fn().mockResolvedValue(undefined); + const signal = new AbortController().signal; config['hookSystem'] = { fireInstructionsLoadedEvent, } as unknown as HookSystem; @@ -7911,7 +7922,7 @@ describe('Server Config (config.ts)', () => { projectRoot: '/tmp', }); - await config.refreshHierarchicalMemory(); + await config.refreshHierarchicalMemory('session_start', signal); const lastCall = vi.mocked(loadServerHierarchicalMemory).mock.calls.at(-1); const options = lastCall?.at(-1) as @@ -7935,6 +7946,7 @@ describe('Server Config (config.ts)', () => { triggerFilePath: '/tmp/project/AGENTS.md', parentFilePath: '/tmp/project/AGENTS.md', }, + signal, ); }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index caede021eff..d1aee9907db 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -3470,7 +3470,7 @@ export class Config { if (!this.provisionalWorkspace) { recordStartupEvent('config_initialize_hierarchical_memory_start'); - await this.refreshHierarchicalMemory('session_start'); + await this.refreshHierarchicalMemory('session_start', options?.signal); recordStartupEvent('config_initialize_hierarchical_memory_end'); this.debugLogger.debug('Hierarchical memory loaded'); } @@ -3933,6 +3933,7 @@ export class Config { async refreshHierarchicalMemory( loadReason: Exclude = 'refresh', + signal?: AbortSignal, ): Promise { // Safe mode: skip all context file loading (QWEN.md, AGENTS.md, rules) if (this.isSafeMode()) { @@ -3965,6 +3966,7 @@ export class Config { loadReason, onInstructionsLoaded: createInstructionsLoadedCallback( () => this.hookSystem, + signal, ), }, ); diff --git a/packages/core/src/hooks/instructionsLoadedCallback.test.ts b/packages/core/src/hooks/instructionsLoadedCallback.test.ts index 63352f20d67..529c2b62216 100644 --- a/packages/core/src/hooks/instructionsLoadedCallback.test.ts +++ b/packages/core/src/hooks/instructionsLoadedCallback.test.ts @@ -11,12 +11,14 @@ import type { HookSystem } from './hookSystem.js'; describe('createInstructionsLoadedCallback', () => { it('forwards instruction load metadata to the hook system', async () => { const fireInstructionsLoadedEvent = vi.fn().mockResolvedValue(undefined); + const signal = new AbortController().signal; const callback = createInstructionsLoadedCallback( () => ({ hasHooksForEvent: vi.fn().mockReturnValue(true), fireInstructionsLoadedEvent, }) as unknown as HookSystem, + signal, ); await callback({ @@ -35,6 +37,7 @@ describe('createInstructionsLoadedCallback', () => { triggerFilePath: '/repo/src/app.ts', parentFilePath: '/repo/AGENTS.md', }, + signal, ); }); diff --git a/packages/core/src/hooks/instructionsLoadedCallback.ts b/packages/core/src/hooks/instructionsLoadedCallback.ts index 65077a8b93b..c628e832cd8 100644 --- a/packages/core/src/hooks/instructionsLoadedCallback.ts +++ b/packages/core/src/hooks/instructionsLoadedCallback.ts @@ -19,6 +19,7 @@ export type InstructionsLoadedCallback = ( */ export function createInstructionsLoadedCallback( getHookSystem: () => HookSystem | undefined, + signal?: AbortSignal, ): InstructionsLoadedCallback { return async (notification: InstructionsLoadedNotification) => { const hookSystem = getHookSystem(); @@ -34,6 +35,7 @@ export function createInstructionsLoadedCallback( triggerFilePath: notification.triggerFilePath, parentFilePath: notification.parentFilePath, }, + signal, ); }; }