fix(daemon): resolve /acp permission votes across connections - #5910
Closed
chiga0 wants to merge 12 commits into
Closed
fix(daemon): resolve /acp permission votes across connections#5910chiga0 wants to merge 12 commits into
chiga0 wants to merge 12 commits into
Conversation
…d-turn content)
The `/acp` Streamable-HTTP session event stream was live-only: it emitted no
SSE `id:` sequence and ignored a `Last-Event-ID` reconnect header. When a
control-plane proxy idle-closed the long-lived SSE mid-turn, every content
frame the daemon produced during the gap (`session/update` carrying
agent_thought_chunk / agent_message_chunk) was lost — the turn still settled,
so the UI showed "done" with an empty/truncated body, and only a re-send
recovered it (tracked as §1.8 in the integration notes).
The replay engine already exists and is battle-tested on the REST surface:
EventBus assigns a monotonic per-session `id`, keeps a bounded ring, and
`subscribeEvents({ lastEventId })` replays `id > lastEventId` before live
events flow. This wires the `/acp` transport to it — no eventBus/bridge change.
- transport-stream / sse-stream / ws-stream: `send(message, id?)`. SSE emits an
`id:` line when `id` is present (mirrors REST `formatSseFrame`); WS ignores it
(stateful, no replay).
- connection-registry: `sendSession(…, id?)` threads the cursor; the pre-attach
session buffer stores `{ frame, id? }` so a buffered frame keeps its `id:`.
- dispatch: `translateEvent` passes `event.id` for bus events; `pumpSessionEvents`
forwards `lastEventId` to `subscribeEvents`.
- index: the `GET /acp` session branch reads `Last-Event-ID` (strict
decimal-only parse, same rule as REST) and passes it to the pump.
Bus-originated frames (session/update, request_permission, daemon notifies)
carry an `id:`; JSON-RPC responses and synthetic terminal frames do not, so
they don't burn a slot in the resume sequence. Backward compatible: clients
that send no `Last-Event-ID` get live-only behaviour as before, and `id:`
lines are inert for clients that ignore them.
Design: docs/design/daemon-acp-http/sse-resumable-stream.md
…reclaim + replay guards Addresses three review Criticals on the §1.8 plumbing: on its own the `id:`/`Last-Event-ID` wiring never fired in the real close-then-reconnect flow, and once it does fire two replay-correctness gaps become reachable. 1. Session-stream grace/reclaim (the core fix). A transport-level session-stream close used to run the FULL `closeSessionStream` teardown — removing ownership, aborting the in-flight prompt, detaching the bridge client. In the real EventSource/proxy order (old socket closes first, then reconnect) that meant the reconnect carrying `Last-Event-ID` was rejected 403 before the cursor was read, and the prompt was already aborted — so replay had nothing to resume. Now a transport close DETACHES (`detachSessionStream`): it stops only the stream + subscription and keeps the binding, ownership, prompt, and bridge-client alive for a grace window (`SESSION_GRACE_MS`, mirrors `CONN_GRACE_MS`). A reconnect within the window reclaims (clears the timer); otherwise the grace timer runs the full teardown, bounding runaway cost. Full teardown stays immediate for explicit `session/close` and connection destroy. The GET handler branches on `stream.isClosed` (transport close → grace; pump-ended-while-open → full close). 2. No double-delivery (buffer ↔ ring overlap). `attachSessionStream` records the max bus id flushed from the pre-attach buffer; the GET handler advances the replay cursor to `max(Last-Event-ID, lastFlushedEventId)` so the ring replay doesn't re-emit an already-flushed frame. 3. Idempotent `permission_request` under replay. `translateEvent` reuses the existing `conn.pending` entry for a `bridgeRequestId` (re-sends the same outbound id) instead of minting a second id+entry — no orphan pending, no duplicate prompt on a ring-replayed permission. Also: extract `parseLastEventId` to a shared `serve/sse-last-event-id.ts` used by both REST and `/acp` (no drift; logs the rejected value); log `lastEventId` in the pump error. Tests: real close-then-reconnect order (200 not 403 + prompt not aborted); overflow Last-Event-ID; replayed permission reuses pending id; registry grace/reclaim + buffer-flush-preserves-id. Full acp-http suite green (216).
The resumable ACP-over-HTTP transport (AcpHttpTransport, native
supportsReplay + Last-Event-ID) and the negotiateTransport factory were
reachable only from source paths inside the monorepo — the published
`@qwen-code/sdk/daemon` barrel intentionally omits them to keep its
budget-checked browser bundle lean, so external consumers (agent-web)
had no import path short of forking.
Add a separate opt-in subpath `@qwen-code/sdk/daemon/transports` that
ships AcpHttpTransport / AcpWsTransport / AutoReconnectTransport /
RestSseTransport / negotiateTransport as their own browser+node bundle.
The default `./daemon` barrel and its byte budget are unchanged, so
REST-only consumers stay tree-shaken and pay nothing for the transports.
Also add a `fetchFn` option to NegotiateTransportOptions so callers can
inject auth/proxy/test fetch instead of the hardcoded global.
- build.js: emit dist/daemon/transports.{js,cjs}; reuse the node-builtin
guard for the new browser bundle (no size budget — it legitimately
ships the transports) while keeping the default barrel's budget check.
- daemon/index.ts: update the rationale comment to point at the subpath.
- daemon-transports-surface.test.ts: lock the runtime + type surface and
the package.json exports entry.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Round-3 review (qwen-code-ci-bot) flagged a silent-frame-loss Critical in the §1.8 resume path I added: `resumeCursor = max(Last-Event-ID, lastFlushedEventId)` advances the ring-replay cursor past the buffer, but a frame sent to the now-dead socket yet never received by the client has a bus id BELOW the buffer's ids and ABOVE the client's cursor — so the max() skips it and the ring replay never re-emits it. Exactly the proxy idle-close mid-turn frame §1.8 is meant to recover. Fix without trading loss for duplicates: a buffered bus event is ALSO in the EventBus ring (it was published there to get its id), so the ring replay started at the client's cursor is the single delivery path for every bus event after the cursor. `attachSessionStream` now takes the resume cursor and, when resuming, does NOT flush id-bearing buffered frames — the ring owns them, delivering each exactly once including the in-flight-lost frame. Id-less frames (JSON-RPC replies via `replySession`, not ring events) are still flushed — their only delivery path. The GET handler sets `resumeCursor = lastEventId` verbatim; `lastFlushedEventId` is removed. Also from the same review: - sse-last-event-id `safeLogValue`: strip ALL C0 control chars + DEL (not just CR/LF) so a crafted `Last-Event-ID` can't smuggle ANSI ESC / null bytes onto an operator's terminal via stderr. - ws-stream: regression test asserting `send(msg, id)` keeps the WS wire frame bare JSON (no SSE `id:` framing leak). - connection-registry: resume-path test (id-bearing frames skipped, id-less reply still flushed); design doc updated. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Review nit (yiliang114): after the prior commit dropped the `max()` logic, `resumeCursor` is a pure alias for `lastEventId`. Use `lastEventId` directly in the `pumpSessionEvents` call and the error log; drop the alias. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Round-4 review (qwen-code-ci-bot, against the now-corrected resume model):
- connection-registry: the attachSessionStream CONTRACT comment still cited a
`promptAbort?.abort()` call in the index.ts onClose handler that an earlier
commit removed. Rewrite it to describe the current model — each stream's pump
has its own abort controller and teardown is identity-guarded in
`onPumpSettled`, so installing the new stream first makes the old stream
settle into detach-with-grace rather than tearing down the in-flight prompt.
- dispatch: the stream_error frame comment ("no bus id, so no SSE id: line")
contradicted the code passing `event.id`. Make it truthful: pass the cursor
through if present; a synthetic terminal frame has no id so none is written.
- connection-registry.test: add the explicit detach → produce gap events →
reattach → flush-exactly-once test (the PR's core value prop at the registry
layer), incl. a second reattach asserting the buffer drained.
The two Criticals in the same review referenced `resumeCursor` /
`lastFlushedEventId` / `Math.max`, all removed in prior commits — obsolete
against current code (answered + resolved on the threads).
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…n paths Round-5 review (wenshao + qwen-code-ci-bot): - [Critical, wenshao] Out-of-order completion on resume. attachSessionStream flushed id-less buffered JSON-RPC replies (e.g. a session/prompt result that landed during the detach gap) immediately — ahead of the ring replay that redelivers the content chunks preceding them, so a client could see "prompt complete" before the body (the truncated-body failure §1.8 fixes). Now on resume those id-less frames are DEFERRED in the buffer; the event pump releases them via flushBufferedSessionFrames once the replay boundary (replay_complete / state_resync_required) passes, preserving original order. Fresh connects (no cursor, no replay) still flush the whole buffer in order. - [Critical, ci-bot] Permission auto-denied during the reconnect grace window: a permission_request arriving while binding.stream is detached cancel-denies, so a client reconnecting within grace can't vote. The structural fix (defer the vote across grace) belongs with the §1.7 permission-coordination follow-up; here, log an operator breadcrumb when it fires during grace, and document the synchronous-translateEvent INVARIANT the direct binding.stream .send relies on. - [ci-bot] Stale-stream detach is now tested (reclaim installs s2; a late s1 close is a no-op — no teardown, no grace re-arm). Grace-expiry teardown now logs a breadcrumb so a vanished session is distinguishable from explicit close. TS4111: bracket-access the index-signature exports entry in the SDK surface test. transports browser bundle now has a size budget (MAX_TRANSPORTS_BROWSER_BUNDLE_BYTES = 48KB; current ~29KB). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…traction Main refactored server.ts into extracted route/helper modules (routes/session.ts, routes/sse-events.ts, server/request-helpers.ts, etc.). The PR branch's SSE resume feature (parseLastEventId extraction, isServeDebugMode import) targeted the old monolithic server.ts. Resolution: drop the PR's server.ts-only imports and inline code that main already extracted. parseLastEventId lives in server/request-helpers.ts on main and is consumed by routes/sse-events.ts. isServeDebugMode is consumed by the extracted workspace-agents.ts, workspace-memory.ts, and workspace-auth.ts modules. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
[Critical, wenshao] The published AcpHttpTransport could hang real
session/prompt + config requests. Its subscribeEvents() opened REST
GET /session/:id/events and only sendRequest's connection-scoped stream
resolved responses — but the daemon's replySession() routes session-scoped
JSON-RPC replies onto the session-scoped /acp stream, which the transport
never read. So a session/prompt reply was never observed → the pending
request never settled.
Switch subscribeEvents to the session-scoped /acp stream (GET /acp +
Acp-Session-Id) — the resumable §1.8 stream the daemon puts session replies
on — and dispatch each raw JSON-RPC frame by shape:
- response (id, no method) → resolve the shared pending map (the fix)
- notification (method, no id) → DaemonEvent via denormalizeAcpNotification,
stamped with the real bus id from the SSE
`id:` line (the synthetic denormalizer id is
not resume-compatible; supportsReplay=true
now tracks the authoritative cursor)
- session/request_permission → surfaced as a permission_request event so
consumers still see prompts (responding to
the vote is the §1.7 follow-up)
The connection-scoped stream still carries replies to connection-level
requests (initialize, session/new). Adds 4 subscribeEvents unit tests
(stream selection + headers, notification→event+busId, response consumed-not-
yielded, permission surfaced). Full SDK suite green (1062).
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…es bump Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Round-6 review (qwen-code-ci-bot), all additive / no behavioural side effects: - [Critical] Unbounded SSE buffer in the new AcpHttpTransport session-stream parser → OOM (tab crash for browser consumers). Add a 16 MiB cap mirroring parseSseStream's MAX_BUF_CHARS, and reuse parseSseStream's CRLF-aware `consumeFrames` splitter (now exported) instead of an inline `\n\n` scan — closing the CRLF, multi-line `data:` join, and trailing-CR gaps in one go. - Deferred-flush ordering race: the pump's post-loop safety flushDeferred() now runs only on a non-aborted exit. An abort means the stream was detached/reclaimed; flushing there could drain the deferred reply onto a reclaiming stream ahead of its own replay (reintroducing the out-of-order delivery the deferral prevents). On error the frames stay buffered for the next attach — never lost. - Grace reclaim now logs (detach + grace-expiry already did) so the reconnect trail is complete for operators. - sse-last-event-id doc: corrected the "shared by REST and ACP" claim — after the QwenLM#5809 serve-route split REST keeps its own copy; unifying them would touch REST, so it's deferred (this PR keeps REST untouched). Thread on a full deferred-flush integration test: the ordering invariant is already locked at the unit layer (flushBufferedSessionFrames defer test + gap-delivery test); a full-HTTP timing test against the FakeBridge would be flake-prone, so it's intentionally not added. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #5852. This branch is stacked on the current #5852 head because it builds on the §1.8
/acpresumable-stream grace/replay plumbing; until #5852 lands, reviewers should focus on the top commit (fix(daemon): resolve acp permission votes across connections).What this PR does
This PR makes ACP-over-HTTP permission votes resolve by the session-global bridge request id instead of being tied only to the connection that streamed the prompt. It adds registry-level lookup/cleanup for pending permission requests, makes daemon-issued permission JSON-RPC ids connection-qualified so cross-connection responses are unambiguous, and lets an authorized co-owner connection cast the vote while preserving the same bridge client/from-loopback authorization checks. It also wires the REST-like SDK permission vote route (
session/permission) through/acp, with 404 semantics for unknown/already-resolved votes, and defers permission requests that arrive during the reconnect grace window instead of auto-cancelling them.Why it's needed
#5852 fixes §1.8 content-stream resumption, but deliberately leaves §1.7 permission vote resolution out of scope. Without this follow-up, a client topology that streams
session/request_permissionon oneAcp-Connection-Idand posts the vote on another can get a 202 from/acpwhile the daemon silently drops the vote on a per-connection pending miss, leaving the agent prompt stuck. The same permission coordination model is also needed for the grace window introduced by #5852, where a reconnecting client should still be able to see and answer a permission request produced during the gap.Reviewer Test Plan
How to verify
Verify a multi-connection ACP session where connection A receives
session/request_permissionand connection B has also claimed the same session. A vote posted from B should resolve the bridge permission request; a vote from a connection that never claimed the session should not resolve it. Verifysession/permissionover/acpaccepts a valid session-scoped vote and returns a connection-scoped success response, while unknown/unowned votes surface as rejected rather than disappearing. Verify a permission request that arrives during session-stream reconnect grace is buffered for replay rather than being auto-cancelled. VerifyAcpHttpTransportmaps the SDK REST-like permission route tosession/permissionwith the original request id and outcome.Evidence (Before & After)
Before: cross-connection JSON-RPC responses missed the voting connection's pending map and returned without resolving the bridge mediator; permission requests in the reconnect grace gap could be cancelled before the client reattached. After: targeted tests cover co-owned cross-connection vote resolution, unauthorized cross-connection rejection,
session/permissionsuccess/reject paths, grace buffering, registry pending lookup/cleanup, and SDK route mapping.Local verification:
npx vitest run src/serve/acp-http/connection-registry.test.ts src/serve/acp-http/transport.test.tsinpackages/clipassed (193 passed).npx vitest run test/unit/AcpHttpTransport.test.tsinpackages/sdk-typescriptpassed (26 passed).npm run typecheckpassed.npm run buildpassed; it still reports existing vscode companion curly-rule warnings and Browserslist freshness warnings only. Targeted ESLint on the changed files passed. Fullnpm run lintwith the default heap hit a Node OOM in repository-wide eslint, so it was not used as the final lint signal.Tested on
Environment (optional)
Local unit/integration-style transport tests only; no real daemon/model runtime required.
Risk & Scope
session/request_permissionJSON-RPC ids remain opaque strings but now include the connection id to make them globally unique across ACP connections. Clients should already echo them opaquely.Linked Issues
Depends on #5852. Tracked internally as §1.7 in the daemon ACP integration notes.
中文说明
这个 PR 做了什么
这个 PR 让 ACP-over-HTTP 的 permission vote 按 session-global 的 bridge request id 解析，而不再只绑定到最初推送 prompt 的那个 connection。它新增 registry 级 pending permission 查找/清理，把 daemon 下发的 permission JSON-RPC id 改成带 connection 前缀以避免跨连接歧义，并允许已经 claim 同一 session 的授权 co-owner connection 投票，同时保持相同的 bridge client/from-loopback 授权检查。它还把 SDK 的 REST-like permission vote route (
session/permission) 接到/acp，对 unknown/already-resolved vote 保持 404 语义，并且让 reconnect grace 窗口里到达的 permission request 被延迟/缓冲，而不是自动 cancel。为什么需要
#5852 修复的是 §1.8 content stream resumption，并且刻意把 §1.7 permission vote resolve 留给独立 follow-up。没有这个 PR 时，如果某个客户端拓扑在 connection A 上接收
session/request_permission，却在 connection B 上 POST vote，/acp会先返回 202，但 daemon 会因为 voting connection 的 per-connection pending miss 而静默丢弃 vote，agent prompt 就会卡住。#5852 引入的 grace window 也需要同一个 permission coordination 模型，因为 reconnect gap 里产生的 permission request 应该允许客户端重连后继续看到并投票。Reviewer 测试计划
如何验证
验证一个 multi-connection ACP session：connection A 收到
session/request_permission，connection B 也 claim 了同一个 session。B 发出的 vote 应该 resolve bridge permission request；一个从未 claim 该 session 的 connection 发 vote 不应 resolve。验证/acp上的session/permission能接受合法的 session-scoped vote 并在 connection stream 上返回成功响应，unknown/unowned vote 会被拒绝而不是静默消失。验证 session-stream reconnect grace 期间到达的 permission request 会被缓冲等待 replay，而不是被自动 cancel。验证AcpHttpTransport会把 SDK 的 REST-like permission route 映射成带原始 request id 和 outcome 的session/permission。证据（Before & After）
Before：跨连接 JSON-RPC response 会 miss voting connection 的 pending map，并直接返回，bridge mediator 不会 resolve；reconnect grace gap 里的 permission request 可能在客户端重新 attach 前被 cancel。After：新增 targeted tests 覆盖 co-owned cross-connection vote resolve、unauthorized cross-connection rejection、
session/permissionsuccess/reject path、grace buffering、registry pending lookup/cleanup，以及 SDK route mapping。本地验证：在
packages/cli运行npx vitest run src/serve/acp-http/connection-registry.test.ts src/serve/acp-http/transport.test.ts通过（193 passed）。在packages/sdk-typescript运行npx vitest run test/unit/AcpHttpTransport.test.ts通过（26 passed）。npm run typecheck通过。npm run build通过；仍会报告仓库既有的 vscode companion curly-rule warnings 和 Browserslist freshness warnings。改动文件的 targeted ESLint 通过。默认 heap 下的全量npm run lint在 repository-wide eslint 阶段触发 Node OOM，因此没有把它作为最终 lint 信号。测试平台
环境（可选）
仅本地 unit / integration-style transport tests；不需要真实 daemon/model runtime。
风险与范围
session/request_permissionJSON-RPC id 仍是不透明 string，但现在包含 connection id 以保证跨 ACP connection 全局唯一。客户端本就应该 opaque echo 这个 id。关联 Issue
依赖 #5852。内部跟踪为 daemon ACP integration notes 的 §1.7。