Local real-run verification (maintainer, macOS) — backpressure works end-to-end on every surface; needs a rebase + two merge-time notes

I built PR head 3045f0ac2, started real qwen serve daemons in tmux under four configurations (default cap, --max-pending-prompts-per-session 3, 0 = disabled, invalid values), and drove real concurrent prompts against them over raw REST, raw ACP WebSocket JSON-RPC, and the TypeScript SDK — plus a control run against a main build to separate this PR's behavior from pre-existing semantics.

Bottom line: the admission cap works exactly as documented on every surface I could reach — REST 503 shape, ACP prompt_queue_full error data, shared bridge-level accounting, SDK local reservation, SDK 503→typed-error mapping, capabilities advertisement, boot validation, and slot release all verified live. The two earlier Critical test-stability issues are fixed (CI is green; suites pass locally). What stands between this and merge: the branch is CONFLICTING with main (one test file), and the PR silently raises the SDK browser-bundle budget to a value with 3 bytes of headroom.

Environment

• macOS 26.5 (arm64), Node v22.22.2; PR head 3045f0ac2 (5 commits, branch is 19 behind main); daemon = npm run bundle → node dist/cli.js serve; clients = raw fetch, raw WebSocket, and this branch's SDK via tsx. Real model turns (excess prompts settled via POST /session/:id/cancel).

Static checks

The two subscription-mode tests I flagged as hanging/failing in my first review are fixed by the test(sdk): stabilize … commits — the suites now pass deterministically.

Live behavior matrix (all real daemons, real prompts)

The SDK 503→typed-error mapping (fix(sdk): map prompt queue full responses) directly addresses my earlier review suggestion and works live; promptNonBlocking not reserving locally is a defensible design (its callers manage their own concurrency) now that the server rejection surfaces as the same typed error.

Control experiment — the odd ACP settle shapes are pre-existing, not this PR

When I fired 6 blocking session/prompt calls concurrently over one WS connection, the non-winning prompts settled as 1×stopReason:"cancelled" + 4×generic Internal error (details: "Request was aborted."). To attribute this correctly I ran the identical probe against a main build: same shape — [0] cancelled, [1..4] internal, [5] end_turn. So concurrent blocking ACP prompts already interrupt each other on main (last writer wins, displaced ones settle as cancelled/internal); this PR only changes the over-cap prompt from "wins by interrupting" to "rejected with prompt_queue_full" — which matches the Risk note in the PR description. The generic internal shape for displaced prompts is a pre-existing wart worth a follow-up, not a defect of this PR.

Merge-time notes (the actionable bits)

1. Rebase required. GitHub reports the branch CONFLICTING; locally the conflict is confined to packages/cli/src/serve/acpHttp/transport.test.ts (other touched files merge clean). Needs a rebase onto current main (19 commits behind) before merge.
2. Undeclared bundle-budget raise with 3 bytes of headroom. The PR bumps MAX_DAEMON_BROWSER_BUNDLE_BYTES from 114 * 1024 to 115 * 1024 in packages/sdk-typescript/scripts/build.js — not mentioned anywhere in the PR description. The built browser bundle is 117,757 bytes against the new 117,760 limit. The raise itself is defensible (the SDK gains real reservation logic), but (a) it should be declared and (b) 3 bytes of headroom guarantees the next daemon-SDK change trips the gate and gets to make this same silent bump. Suggest landing it at a round value with honest headroom (e.g. 118 or 120 * 1024) in this PR.
3. (Minor, from my earlier review) A bridge-level PromptQueueFullError rejection still emits no dedicated daemon log line; the generic route log does record status=503 at WARN with the sessionId, so operators aren't blind — but a dedicated line (matching the WorkspaceMismatchError precedent) remains nice-to-have. Non-blocking.

Not covered by this run

DaemonSessionClient subscription-mode reservation/release paths are covered by the (now stable) unit suites only — I did not drive them against a live daemon. Windows/Linux untested here (CI covers both).

Recommendation

Functionally this is mergeable: every advertised behavior verified end-to-end against real daemons, including the cross-surface cap accounting that's hardest to get right. Gate merge on: (1) rebase to resolve the transport.test.ts conflict with CI re-green, and (2) either declare the bundle-budget raise in the PR description or land it with real headroom. Happy to re-verify after the rebase if anything in the conflict region looks risky.

@qwen-code /triage

Thanks for the PR!

Template looks good ✓

On direction: this is a natural and well-motivated follow-up to #4490. Prompt deadlines bounded execution time but left admission unbounded — that gap is real, and closing it with a per-session cap mirrors the existing --max-sessions / --max-connections pattern operators already understand. No CHANGELOG reference needed; the area is clearly within serve's load-management scope.

On approach: the three-layer design (bridge as authoritative gate, REST/ACP mapping to stable 503 shape, SDK local guard for early-fail) is sound and each layer earns its existence. The SDK guard prevents wasteful SSE connections, not just redundant HTTP requests. The branchSession exemption from the prompt cap is the right call — it's serialized on the same FIFO but isn't a prompt.

One observation: DEFAULT_MAX_PENDING_PROMPTS_PER_SESSION = 5 is duplicated across bridge.ts, server.ts, and DaemonClient.ts with "keep in sync" comments. This is a pragmatic choice to avoid cross-package coupling, but it's the kind of constant that drifts silently. Not a blocker — just flagging it for awareness.

Minor nit: the PR title says fix(serve) but this reads more like a feature addition (feat(serve)). Doesn't block anything.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code review

Reviewed the full diff (23 files, +1421/-87). The implementation is clean and well-structured — no critical blockers found.

Independent proposal comparison: my baseline was "add a pendingPromptCount counter to SessionEntry, check before enqueue, release on settlement, map to 503 in REST." The PR matches this exactly and adds sensible layers (SDK local guard, capabilities advertisement, ACP error mapping). No simpler path was missed.

Key correctness points verified in code:

• sendPrompt signature change from async to sync (returning Promise) is correct and intentional — it must throw synchronously for the REST route to catch before returning 202 Accepted.
• Slot release via result.finally(releasePromptSlot) with a promptSlotReleased guard ensures exactly-once release across all settlement paths (success, failure, abort).
• Pre-aborted signals rejected before consuming a slot — avoids wasted capacity.
• branchSession correctly exempted from the prompt cap — serialized on the same FIFO but not a prompt.
• REST 503 response shape: Retry-After: 5, code: "prompt_queue_full", no promptId — matches the design doc.
• ACP JSON-RPC error has stable data.errorKind: "prompt_queue_full" — clients can match on it.
• SDK reservePromptSlot + releasePromptSlot pattern mirrors the bridge, with releases on turn_complete, turn_error, caller abort, SSE end, and fetch failure.

Non-blocking observations:

• Error messages shortened in a few places ('SSE stream ended', 'No SSE body', 'timeout', 'subscription active'). These are test-visible changes that any downstream consumers matching on message text should be aware of. Not a bug — just a heads-up.
• Browser bundle size limit bumped 114→115 KB — reasonable for the added logic.

Test results

Unit tests (all passed)

Total: 955/955 passed, 0 failures. Typecheck passed for all packages.

Real-scenario testing

Default cap (5) — GET /capabilities

$ node packages/cli/dist/index.js serve --port 0
qwen serve: daemon log → /home/runner/.qwen/debug/daemon/serve-5272-918fec44.log
qwen serve: session reaper started (interval 60000ms, idle threshold 1800000ms)
qwen serve listening on http://127.0.0.1:39699 (mode=http-bridge, workspace=.../triage)
qwen serve: bound to workspace ".../triage"
qwen serve: bearer auth disabled (loopback default). Set QWEN_SERVER_TOKEN to enable.
qwen serve: /acp WebSocket transport enabled on /acp

$ curl -s http://127.0.0.1:39699/capabilities | jq '.limits'
{
  "maxPendingPromptsPerSession": 5
}

Disabled cap (0) — GET /capabilities

$ node packages/cli/dist/index.js serve --port 0 --max-pending-prompts-per-session 0
qwen serve listening on http://127.0.0.1:38081 (mode=http-bridge, workspace=.../triage)

$ curl -s http://127.0.0.1:38081/capabilities | jq '.limits'
{
  "maxPendingPromptsPerSession": null
}

Invalid value rejected at boot

$ node packages/cli/dist/index.js serve --port 0 --max-pending-prompts-per-session -5
qwen serve: --max-pending-prompts-per-session must be a non-negative integer (0 / Infinity = unlimited).
Exit code: 1

All three scenarios behave as specified in the design doc. The limits field is additive to the capabilities envelope (older daemons simply omit it), and the null sentinel for disabled matches the documented contract.

— Qwen Code · qwen3.7-max

Final verdict: ✅ Approve

Stepping back — this is a solid, well-executed infrastructure PR. The motivation is clear: #4490 added prompt deadlines but left the admission door wide open, and this PR closes that gap with the minimal effective change.

The counter-and-release pattern is the simplest correct solution. The three-layer design (bridge gate → REST/ACP mapping → SDK early-fail) is sound, with each layer earning its existence rather than being speculative. The slot release logic is defensive (double-release guard, .finally for all settlement paths) without being paranoid.

955 tests pass, typecheck clean, real-scenario testing confirms the capabilities endpoint advertises the limit correctly in default (5), disabled (null), and invalid (boot rejection) cases.

My independent proposal matched the PR's approach — I didn't find a simpler path it missed. The scope feels right: the SDK local guard adds complexity but prevents wasteful SSE connections, not just redundant HTTP round-trips.

Two non-blocking notes for the author's awareness (neither blocks the merge):

• The shortened error messages ('SSE stream ended', 'No SSE body', etc.) are a minor public-contract change for anyone matching on message text.
• The DEFAULT_MAX_PENDING_PROMPTS_PER_SESSION = 5 constant is duplicated in three places with "keep in sync" comments — worth a thought on whether a shared constants module would be cleaner long-term.

Approving. 🚢

— Qwen Code · qwen3.7-max

@qwen-code /triage

Thanks for the PR!

Template looks good ✓ — all required headings present with bilingual body.

On direction: this is a natural and necessary follow-up to #4490. Prompt deadlines bounded execution time but left admission unbounded — a single session could enqueue unlimited accepted prompts, accumulating promises, SSE waits, and closures. Adding an explicit admission cap with retryable semantics is the right fix. The area is squarely within qwen serve's core mission as a daemon load-management feature.

On approach: the layered defense (bridge admission → REST 503 → ACP error data → SDK local reservation → capabilities advertisement) feels proportionate to the problem. Each layer has a clear role: bridge is authoritative, SDK is early-fail guard, capabilities lets clients self-configure. The default of 5 is reasonable for single-user / small-team usage and the escape hatch (--max-pending-prompts-per-session 0) is clean. branchSession correctly excluded from the prompt cap.

One observation: the PR bundles design doc, E2E test plan, docs update, and implementation in a single commit set — that's thorough but makes the diff large (+1677 across 24 files). The implementation itself is well-scoped; the extras are all justified by project conventions.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code review

Reviewed the full diff (24 files, +1677/-89). The implementation is careful and well-structured.

Independent proposal: for unbounded prompt queues, I'd add a per-session counter in the bridge, throw synchronously when the cap is hit, map that to HTTP 503 in the REST route, and optionally add SDK-side local accounting as early-fail. That's essentially what this PR does.

Findings:

The bridge changes are solid. sendPrompt correctly stays non-async so admission failures throw before the HTTP route returns 202. The slot-release logic uses an idempotent promptSlotReleased guard and attaches via result.finally(releasePromptSlot) — exactly once, no leaks. Pre-aborted signals are rejected before incrementing the counter. The PromptQueueFullError class is clean with the right fields.

The REST route wraps bridge.sendPrompt() in a try/catch (since it's no longer async) and maps PromptQueueFullError to 503 with Retry-After: 5. The ACP dispatch adds a dedicated case returning stable errorKind: "prompt_queue_full" data. Both look correct.

The SDK-side reservePromptSlot is a clean reservation system — idempotent release, proper cleanup on all exit paths (completion, turn_error, abort, SSE end, fetch failure). DaemonSessionClient.prompt() correctly uses client.reservePromptSlot() for the subscription path.

No critical blockers found. The validation logic is duplicated across three layers (bridge constructor, runQwenServe, CLI serve command) but that's the established project pattern for serve options and each layer validates independently.

Minor observation: a few error messages were shortened ('SSE stream ended without turn completion' → 'SSE stream ended', 'The operation timed out' → 'timeout'). These are cosmetic and don't affect correctness.

Test results

All focused unit tests pass:

Bridge tests:        267 passed (267)
CLI serve tests:     429 passed (429)
ACP HTTP tests:       92 passed (92)
SDK DaemonClient:    158 passed (158)
SDK SessionClient:    27 passed (27)
                    ─────────────
                    973 total, 0 failures

Typecheck:           clean (all packages)

New tests specifically validate: sixth prompt rejected at default cap, slot release after success/failure/abort/SSE-end, pre-aborted prompts skip the counter, branchSession excluded from cap, disabled cap (0/Infinity), invalid cap rejection at boot, REST 503 shape, ACP stable error data, SDK local rejection without fetch, SDK reservation release on all exit paths.

No TUI changes — this is non-UI serve/bridge/SDK behavior, so tmux testing is N/A per the PR's own test plan.

— Qwen Code · qwen3.7-max

Final verdict: ✅ Approve

Stepping back — this is a well-designed, well-tested addition to the serve layer's load-management story. The motivation is clear (unbounded admission after #4490), the implementation matches the stated design, and the test coverage is thorough across all four affected packages.

My independent proposal (per-session counter, synchronous throw, HTTP 503 mapping, optional SDK guard) matches the PR's approach closely. The PR exceeds it in polish: idempotent slot release, pre-abort fast-path, capabilities advertisement, stable ACP error data, and the branchSession exclusion — all details I'd have wanted but might not have thought of upfront.

The scope is large in file count (24) but each file has a clear purpose. The implementation code itself is tight — the bulk comes from tests and documentation, which is the right ratio for a load-management feature. The three-layer validation (bridge, runQwenServe, CLI) follows established project patterns.

973 tests pass. Typecheck clean. No correctness issues found in the code review.

LGTM, ships the feature cleanly. ✅

— Qwen Code · qwen3.7-max