feat(serve): prompt absolute deadline + SSE writer idle timeout (#4514 T2.9)#4530
Merged
doudouOUC merged 1 commit intoMay 26, 2026
Conversation
Contributor
📋 Review SummaryThis PR implements issue #4514 T2.9, adding two opt-in flags ( 🔍 General Feedback
🎯 Specific Feedback🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds two opt-in reliability controls to qwen serve for remote/long-running deployments: a server-enforced prompt wallclock deadline and an SSE writer idle-timeout, including capability tags for feature detection and cross-package error-kind mirroring (acp-bridge ↔ SDK).
Changes:
- Add
--prompt-deadline-ms/QWEN_SERVE_PROMPT_DEADLINE_MSand--writer-idle-timeout-ms/QWEN_SERVE_WRITER_IDLE_TIMEOUT_MSplumbing (CLI →ServeOptions→ server behavior) and conditional capability tags. - Extend serve/SDK error-kind taxonomies (
prompt_deadline_exceeded,writer_idle_timeout) and adddeadlineMsto the SDKPromptRequesttype. - Add/extend tests and docs to describe and lock the new behavior/capabilities.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-typescript/test/unit/daemon-public-surface.test.ts | Adds drift-insurance assertions for new DAEMON_ERROR_KINDS entries. |
| packages/sdk-typescript/src/daemon/types.ts | Appends new daemon error kinds mirroring serve taxonomy. |
| packages/sdk-typescript/src/daemon/DaemonClient.ts | Adds optional deadlineMs field to PromptRequest typing/docs. |
| packages/cli/src/serve/types.ts | Introduces promptDeadlineMs and writerIdleTimeoutMs on ServeOptions (with docs). |
| packages/cli/src/serve/server.ts | Implements prompt deadline abort/reply scaffolding and SSE writer idle-timeout eviction. |
| packages/cli/src/serve/server.test.ts | Adds unit/integration tests covering capability advertising and timer behaviors. |
| packages/cli/src/serve/runQwenServe.ts | Parses env fallbacks + validates the new numeric options at boot. |
| packages/cli/src/serve/capabilities.ts | Registers new capability tags and predicates for conditional advertisement. |
| packages/cli/src/commands/serve.ts | Adds yargs options/help text for new flags and passes them into runQwenServe. |
| packages/acp-bridge/src/status.ts | Appends new serve error kinds for shared taxonomy. |
| packages/acp-bridge/src/status.test.ts | Updates ordering-pin test to include the new kinds. |
| docs/users/qwen-serve.md | Documents new flags and updates the known-gap section for phantom SSE connections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
Follow-ups surfaced by an independent simplify-review pass on the initial T2.9 commit. No behavior change; tests still 243/243. - server.ts: gate per-chunk `lastWriteAt = Date.now()` behind a `trackWriterIdle` boolean derived once from `opts.writerIdleTimeoutMs`. Skips the timestamp on every SSE frame when the flag is unset (the default), which matters for chatty tool-output streams (hundreds- to-thousands of writes per session). - runQwenServe.ts: extract `isPositiveIntegerMs` predicate; collapse the two new option-validation blocks and the env parser onto one source of truth. - server.test.ts: extract `abortableBridgePromptImpl()` helper; the 3 new T2.9 prompt-deadline tests had verbatim copies of the "Promise that resolves on abort" snippet. - server.ts + capabilities.ts: trim two repetitive call-site comments + one speculative "hypothetical future sentinel" comment whose value was already covered by the helper JSDoc / boot-time validation. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
…review fold-in) Two Copilot review findings on #4530, both genuine and within the PR's scope. 1. **Prompt deadline must be guaranteed**, not contingent on bridge cooperation. Old shape: `await bridge.sendPrompt(...)` after `abort.abort()`. A buggy agent that ignores AbortSignal could keep the HTTP request open indefinitely; the 504 would never fire — defeating T2.9's whole purpose (the PR description explicitly claims it closes the `FIXME(stage-2)` at `httpAcpBridge.ts:2689`, but it didn't). New shape: `Promise.race(bridge.sendPrompt, deadlinePromise)`. The deadline timer rejects the race independently; the 504 lands on schedule regardless of agent behavior. The orphaned bridge promise gets a tail-attached `.catch(() => undefined)` so its eventual rejection (minutes later, when the buggy agent finally settles) doesn't surface as unhandledRejection. `abort.abort` still fires as best-effort wind-down so a cooperative agent frees its FIFO slot. 2. **Writer-idle-timeout docs were wrong** about values <15s being "no-ops". The idle timer checks elapsed-since-last-flush independently of the heartbeat; with no real events, a `--writer-idle-timeout-ms 10000` evicts a healthy idle SSE connection at 10s, BEFORE the first 15s heartbeat ever refreshes `lastWriteAt`. Updated 4 spots (server.ts comment, types.ts JSDoc, serve.ts CLI help, docs/users/qwen-serve.md table) to recommend ≥30000ms for production and warn that smaller values will evict healthy connections — useful for tests / short dev sessions, dangerous in production. Tests: - New `still emits 504 when the bridge IGNORES the abort signal (race contract)` test: fakeBridge returns a never-settling Promise; route still returns 504 within budget. Locks the new Promise.race contract. - All 244 existing tests still pass (was 243; +1 new). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
wenshao
reviewed
May 26, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
Well-structured PR — the two features (prompt deadline + SSE writer idle timeout) are cleanly separated, backward-compatible (off by default), and the core logic is correct. Timer cleanup paths are thorough (every exit path covered, .unref() as defense-in-depth), the Math.min capping contract is sound, and the capability tag conditional advertisement follows the established pattern. Build passes, 292/292 tests pass.
Deterministic analysis: tsc clean, eslint clean (0 findings).
Test Coverage Gaps (pattern)
Several new code paths lack test coverage. Consolidating into one item since the pattern is the same — new private functions / validation blocks without corresponding tests:
parseDeadlineEnv(runQwenServe.ts:41-52) — no tests at all. Five branches (undefined, empty/whitespace, NaN, float, negative) are all uncovered.runQwenServestartup validation (runQwenServe.ts:346-365) — the boot-timeTypeErrorfor invalidpromptDeadlineMs/writerIdleTimeoutMsis not tested. The existingmcpClientBudgettests at lines 3519-3530 provide a ready-made pattern.sendDeadlineResponseIfFiredcatch-path (server.ts:1336) — E2E tests use a prompt impl that resolves on abort; the catch-path call is never exercised.deadlineMsbody validation (server.ts:1215-1228) — onlydeadlineMs: -5is tested. Thetypeof,isFinite, andisIntegerbranches are uncovered.- Writer idle timer with active writes — tests cover idle (no events) and disabled (no flag), but not the case where active writes refresh
lastWriteAtand eviction should NOT fire.
Needs Human Review
- Possibly:
Date.now()is not monotonic — an NTP forward-step could cause a false-positive SSE eviction.performance.now()would be immune. Low risk in practice (NTP corrections are usually sub-second, and the codebase already usesDate.now()for all timing), but worth considering for new code.
— qwen3.7-max via Qwen Code /review
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
Per wenshao's review on #4530, accepted 3 of 6 [Suggestion]s and added test coverage for the gaps called out in the review summary. Pushed back on 3 (separate reply thread per comment with the reasoning). No behavior change for the default-off case; existing 244 tests pass + 25 new. Accepts: - `sendDeadlineResponseIfFired` now writes a stderr breadcrumb with the session id before sending the 504 — symmetric with the SSE writer-idle eviction log. Operators triaging a 504 spike can now `grep "prompt deadline fired"` for the affected session instead of staring at an empty daemon log. Threaded `sessionId` through the helper signature + the direct catch-path 504 path. - `parseDeadlineEnv` no longer treats whitespace-only env vars as silently "unset". `QWEN_SERVE_PROMPT_DEADLINE_MS=" "` now hits the standard "must be a positive integer" boot error — matches the JSDoc's "fail loud on typo" promise. Drove `Number('')` / `Number(' ')` through the existing positive-integer rejection. - Upper-bound validation at `MAX_TIMER_DELAY_MS = 2_147_483_647` (2^31 - 1 ms, ~24.8 days). Node silently compresses `setTimeout` / `setInterval` delays past this to 1ms with a TimeoutOverflowWarning, so an operator setting `--prompt-deadline-ms 2592000000` (30 days) would otherwise see every prompt 504 instantly. Reject loudly at boot with a clear error pointing at the cap. Applied to `promptDeadlineMs`, `writerIdleTimeoutMs`, and the env-var parser for both. Test coverage (5 gaps from review summary): - `parseDeadlineEnv` end-to-end via env-var setting: 6 malformed cases (empty / whitespace / NaN-string / float / negative / zero) + an over-cap case + a happy-path case that asserts the capability tag flips on via the env fallback alone. - `runQwenServe` boot validation for `promptDeadlineMs` and `writerIdleTimeoutMs`: zero / negative / float / NaN via `it.each` (mirrors the existing `mcpClientBudget` test shape at L3525). - `runQwenServe` boot rejection of over-cap deadlines on both options. - Catch-path branch of `sendDeadlineResponseIfFired`: new test where the bridge throws a bare `AbortError` (no typed reason), exercising the defense-in-depth path that still emits 504 by reading `signal.reason`. - `deadlineMs` body validation: `it.each` covering every reachable branch (negative / zero / float / string / boolean / object). NaN/Infinity intentionally absent (JSON.stringify converts them to null, which the validator correctly treats as "absent"). - SSE writer idle with ACTIVE writes: new test where events at ~100ms intervals refresh `lastWriteAt` and the 300ms idle timer doesn't fire. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
|
Thanks for the thorough review, @wenshao. Folded in 3 of 6 [Suggestion]s + the test coverage gaps in c258bad; pushed back on 3 with the reasoning inline on each comment (#2, #5, #6). Summary: Accepted
Test coverage gaps closed (25 new tests, total now 269)
Deferred
Pushed as c258bad, ready for re-review when you have a moment. |
Collaborator
本地真实测试报告（merge 参考）按 PR body 的 Test plan 完成静态检查 + 三个相关 vitest suite + PR 自己列出的两条未勾的 Manual smoke（deadline 路径 + SSE idle 路径）。两个 opt-in flag 在真 daemon 上行为完全符合 PR 描述：deadline 在 ~5ms 精度内触发 504 + 测试环境
1. 静态检查与单测
2. Manual smoke — 启动 daemoncd <worktree>
node packages/cli/dist/index.js serve \
--port 18530 \
--prompt-deadline-ms 1500 \
--writer-idle-timeout-ms 3000 \
--max-sessions 4
2.1 capability 标签（条件性出现）$ curl -sS http://127.0.0.1:18530/capabilities | jq '.features | map(select(. | test("prompt_absolute_deadline|writer_idle_timeout")))'
[
"prompt_absolute_deadline",
"writer_idle_timeout"
]✅ 两个 capability 标签都因为对应 flag 被设置而出现——SDK 消费方据此 pre-flight，符合 PR body 中"silently dropped on older daemons / additive to v=1"的契约。 2.2 创建 session（用于后续两条 smoke）$ curl -sS -X POST http://127.0.0.1:18530/session -H 'content-type: application/json' -d '{}' | jq .
{
"sessionId": "a143185f-ca17-48ed-8a01-24003a917928",
"workspaceCwd": "/Users/wenshao/Work/git/qwen-code/.qwen/tmp/review-pr-4530",
"attached": false,
"clientId": "client_44494c8c-a9d6-4b8a-b635-2991939ea891",
"createdAt": "2026-05-26T06:00:32.547Z"
}3. Manual smoke 1 —
|
| PR body / Issue #4514 T2.9 声称 | 验证 |
|---|---|
| 两个 flag 都是 opt-in，未启用时单用户 loopback 行为不变 | ✅ 默认未传时 capability 不带这两个 tag（间接由 packages/cli/src/serve/capabilities.ts 的条件输出测试覆盖） |
--prompt-deadline-ms 触发 504 + errorKind: prompt_deadline_exceeded |
✅ 3.1 |
请求体 deadlineMs 可以缩短至 server cap 以下 |
✅ 3.2 |
请求体 deadlineMs 不能延长（Math.min 闸口） |
✅ 3.3 |
--writer-idle-timeout-ms 在 idle 越过阈值时触发 client_evicted 终止帧，reason: writer_idle_timeout |
✅ 4 |
errorKind 命名空间稳定（SERVE_ERROR_KINDS 锁定） |
✅ acp-bridge 44/44，sdk-typescript 422/422 包含 drift-insurance test |
Conditional capability 标签 prompt_absolute_deadline / writer_idle_timeout 仅在 flag 启用时出现 |
✅ 2.1 实测两个标签都出现在 features 列表 |
env fallback（QWEN_SERVE_PROMPT_DEADLINE_MS、QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS） |
由 packages/cli/src/serve/runQwenServe.ts 单测覆盖（在 server.test.ts 的 269 用例里），手测未跑 env 路径 |
6. 结论
- ✅ PR body Test plan 全部本地复现通过
- ✅ 之前未勾的两条 Manual smoke 全部跑通：
- Deadline 路径：1500ms 阈值 → 504 在 1.505s 触发；500ms 缩短能生效；10000ms 试图延长被
Math.min截到 1500ms（三个 case） - SSE idle 路径：3000ms 阈值 →
client_evicted{writer_idle_timeout}终止帧在 idleForMs=3004ms 落地，daemon 主动关流
- Deadline 路径：1500ms 阈值 → 504 在 1.505s 触发；500ms 缩短能生效；10000ms 试图延长被
- ✅ Conditional capability 标签按 flag 出现，符合 SDK pre-flight 契约
- ✅ Error 命名空间在 acp-bridge / sdk-typescript / server 三个 package 之间保持一致
从本地真实测试角度，该 PR 可以合并。 两个 flag 都满足"opt-in、可观测、与既有 15s 心跳互不打架、定时精度达毫秒级"的 T2.9 目标，没有看到副作用回流到默认 loopback 工作流。
wenshao
reviewed
May 26, 2026
wenshao
requested changes
May 26, 2026
wenshao
reviewed
May 26, 2026
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
Folded in all 5 items from wenshao's CHANGES_REQUESTED review on PR #4530. One Critical bug fix + 4 Suggestions, all real. 273/273 tests pass (was 269, +4 new including a Critical regression test). Critical (the block reason): - `server.ts` prompt-handler catch block: the early-return on `err instanceof PromptDeadlineExceededError && !res.writableEnded` used the AND-guard for both the body AND the return. When the client disconnected in the same tick the deadline fired, writableEnded was already true → guard failed → fell through to the AbortError branch (false; it's not a DOMException) → reached `sendBridgeError` which called `res.status(500).json(...)` on the already-ended response → `ERR_STREAM_WRITE_AFTER_END` could crash the daemon. Fix: split the guard so `return` always fires on `PromptDeadlineExceededError`; only the JSON write is gated on writableEnded. Added regression test that destroys the client socket mid-prompt and asserts no uncaught throw on the deadline race (`returns cleanly when client disconnects in the same tick the deadline fires`). Suggestions: - `server.ts` SSE eviction: wrapped `writeStderrLine` + `res.end()` in defensive try/catch. If stderr EPIPEs or `res.end()` throws on a destroyed socket, the exception would have escaped the interval callback, `cleanup()` would never run, the heartbeat + idle timers would never clear, and each subsequent tick would re-throw → permanent uncaughtException loop. Now: any throw on either is swallowed, cleanup runs unconditionally. - `server.ts` `sendDeadlineResponseIfFired` JSDoc: rewrote to match the actual call shape — "**called once** from the AbortError catch branch as defense-in-depth" — instead of the stale "BOTH success and catch path" claim left over from before the Promise.race rewrite. - `httpAcpBridge.ts` orphan-promise comment: removed the inaccurate "or the next prompt arrives" reclaim path. The next prompt actually chains BEHIND the orphan via `entry.promptQueue` (L2754 / L2664) and blocks on its eventual settlement — the FIFO is NOT self-healing from a buggy-agent orphan; only session close reclaims. - `runQwenServe.ts` (via tests): closed the `QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS` env-var test gap. Added 3 tests mirroring the prompt-deadline triad (invalid / over-cap / happy-path) so a copy-paste error reading the wrong env-var name in `runQwenServe`'s plumbing fails CI loudly. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Collaborator
Author
|
@wenshao 谢谢你完整跑了 PR body 里那两条没勾的 Manual smoke + 三个 Math.min 边界 case + capability tag 条件出现 — 报告比我自己 pre-merge 该做的还细致。"deadline 5ms 内触发 504" / "SSE eviction 帧 idleForMs=3004 落地后主动关流" 都是我没在单测覆盖到的实际 wallclock 精度，留作以后类似 PR 的 acceptance baseline。 5 项 inline 全部 fold-in 在 a1a8182（273/273 tests 过）： Critical #3 — 你说的对，writableEnded race 走到 Suggestion #1/#2/#4 — 三个都是真问题：httpAcpBridge.ts 的 FIFO 链描述错误（next prompt 是 chain BEHIND orphan，不是 reclaim）；SSE eviction 的 stderr+end 没 try/catch，EPIPE 会让 cleanup 漏掉 → 永久 uncaughtException loop； Suggestion #5 — 补了 PR 已 push
|
Collaborator
✅ 本地真实测试验证报告 (PR #4530)环境：Linux 6.12 / Node 22 / 本地 tmux + curl 真实 HTTP/SSE 探测 / 已 checkout PR 分支 HEAD 一、自动化测试
合计 739 个单测全通过。 二、tmux 真机 HTTP 探测启动 1. 条件 capability tag 仅在对应 flag 配置时出现
✅ Tag = 行为开关的契约成立；env var fallback 也能驱动 tag。 2.
|
| 请求 body | Wall | 生效 deadline |
|---|---|---|
{deadlineMs: 2000} |
2013ms | 2000ms ← 客户端可以收紧 |
{deadlineMs: 30000} |
5010ms | 5000ms ← 客户端无法放宽，cap 胜 |
无 deadlineMs |
5017ms | 5000ms |
✅ Math.min(serverCap, requestedDeadline) 行为完全符合设计文档。
4. --writer-idle-timeout-ms SSE 驱逐路径
启 --writer-idle-timeout-ms 3000，连一个 SSE 不动：
$ curl -N .../session/<id>/events
event: session_update
data: {"id":1,"v":1,"type":"session_update","data":{...}} ← 初始 frame，重置 lastWriteAt
event: client_evicted
data: {
"v":1,
"type":"client_evicted",
"data":{
"reason":"writer_idle_timeout",
"errorKind":"writer_idle_timeout",
"idleForMs":3712,
"timeoutMs":3000
}
}
Wall: 3765ms ← 3000ms 超时 + ~750ms poll grace（1/4 budget 检查间隔）✅ 终态 frame 含 reason + errorKind + idleForMs + timeoutMs；socket 主动关闭；时序符合「(timeout + ≤1/4 budget) 内一定 evict」契约。
5. Legacy 契约保护（未设 flag）
无 --writer-idle-timeout-ms 启动 → SSE 静连 6s 未触发 client_evicted。
Wall: 6011ms
PASS: no client_evicted frame in 6s
✅ Default-off 兑现，未引入回归。
6. CLI flag 优先于 env var
$ QWEN_SERVE_PROMPT_DEADLINE_MS=10000 qwen serve ... --prompt-deadline-ms 3000
Wall: 3013ms
HTTP_CODE=504 deadlineMs=3000 ← CLI 3000ms 胜，没用 env 的 10000ms7. Boot-time 入参校验
| 启动 | 输出 |
|---|---|
--prompt-deadline-ms -5 |
Invalid promptDeadlineMs: -5. Must be a positive integer (milliseconds). |
--prompt-deadline-ms 0 |
Invalid promptDeadlineMs: 0. Must be a positive integer (milliseconds). |
--prompt-deadline-ms 99999999999 |
Invalid promptDeadlineMs: 99999999999. Exceeds maximum JS timer delay of 2147483647 ms (~24.8 days); Node would silently compress longer delays to 1ms. |
✅ 负值 / 零 / MAX_TIMEOUT_MS 上限都给出清晰错误，避免「Node 静默压成 1ms」的隐藏坑（这条 >MAX_TIMEOUT_MS 的检查特别值得 —— 操作员要是一年一年地传 deadline，没这道闸他根本不知道 timer 立刻就走完了）。
8. stderr 操作员断点
每条触发路径都有 1 行可 grep 的 stderr：
qwen serve: prompt deadline fired (session 6e3bdf83-...) — deadlineMs=5000
qwen serve: prompt deadline fired (session 6e3bdf83-...) — deadlineMs=2000 ← body 收紧
qwen serve: prompt deadline fired (session 6e3bdf83-...) — deadlineMs=5000 ← body 想放宽，cap 胜
qwen serve: evicting SSE client (session 7f387b1d-...) — writer idle for 3712ms > 3000ms timeout
✅ load balancer 看到 504 spike → grep session id → 直接定位是 deadline 触发还是 bridge 真错。
三、设计要点验证
| 设计承诺（PR 描述） | 真实测试观察 |
|---|---|
| 默认关闭，对 loopback 单用户「比特对比特相同」 | ✅ baseline 路径未 evict、未额外 tag |
Math.min 上限：客户端能收紧不能放宽 |
✅ 2000ms 生效 / 30000ms 被砍到 5000ms |
| 仅当 flag 设置时才 advertise tag（SDK 可 pre-flight） | ✅ 4 种组合表全对 |
504 与 client_evicted 是「硬保证」（不依赖 agent 协作） |
✅ 即便 agent 还在 count to 100，504 仍然准时落地 |
client_evicted{reason: 'writer_idle_timeout'} 复用现有 taxonomy |
✅ 与 eventBus.ts 同 type，多一个 reason 槽 |
| 入参校验在 boot 时 loud-fail，不静默 | ✅ 三种坏值都被挡 |
四、风险与注意点
writer-idle-timeout-ms < 15000会拦截 healthy 连接：源码里--help文案 + 注释都明确写了「production 选 30000~300000，<15s 仅供测试」。本次 3000ms 探测就是利用这一点快速 reproduce —— 文档清楚，不算坑。- Orphan bridge promise tail-catch：deadline race 赢以后，underlying
bridgePromise会变成 orphan（agent 可能几分钟后才报 abort）。代码在bridgePromise.catch(() => undefined)处兜了一次，避免unhandledRejection。本次 504 之后 daemon 进程持续观察 30s+ 未见 Node warning。 - Boot 时
MAX_TIMEOUT_MS上限校验：避免 Node 把>2^31-1 ms静默压成 1ms 的坑 —— 操作员设大值时立即报错。难得见这种「替框架的隐式行为兜底」的代码。 prompt_deadline_exceeded在 bridge stderr 里以Request was aborted.出现：这是 ACP 客户端报code: -32603, message: 'Internal error', data.details: 'Request was aborted.'—— 跟 daemon 自己的qwen serve: prompt deadline fired ...是同事件的两边，相互印证。下游写监控时记得 join。
五、结论
| 维度 | 结论 |
|---|---|
| 单测（3 包 ×全套） | 739/739 通过 |
| Typecheck × 3 包 | 0 errors |
--prompt-deadline-ms 504 路径 + 错误体 |
完全符合设计 |
--writer-idle-timeout-ms SSE 驱逐 + 终态 frame |
完全符合设计 |
Math.min body / server 协商语义 |
验证 3 种组合都正确 |
| 条件 capability tag | 5 种启动组合下都正确 advertise |
| Env var fallback / CLI 优先级 | 都验证 |
| Boot 校验 / 边界 | 都验证 |
| Default-off legacy 契约 | 验证未引入回归 |
建议：可合并。 两条新增 flag 的「硬保证 + opt-in + 完整可观测面」都在真实 HTTP 探测里被复现；PR 描述里手动 smoke 的两条 TODO（504 + client_evicted）都已闭环。
wenshao
reviewed
May 26, 2026
yiliang114
previously requested changes
May 26, 2026
wenshao
previously approved these changes
May 26, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No new high-confidence issues found in this round. The timer cleanup is thorough, race conditions (deadline vs client disconnect vs bridge settlement) are well-handled with proper guards, Promise.race semantics are correct, and the test coverage is extensive (294 tests, all passing). CI green 26/26.
5 low-confidence findings identified but not posted as inline comments (need human review):
- Log injection via unsanitized
sessionIdin newwriteStderrLinecalls — matches existing file-wide pattern (lines 905, 1870, 2114, 2839 also skipsafeLogValue), so this is a pre-existing file-wide concern rather than PR-specific. - Inconsistent error class in
parseDeadlineEnv(ErrorvsTypeError) — zero functional impact, purely stylistic. - 3 test gaps:
deadlineMs: nullbody,deadlineMswith server flag unset, orphanbridgePromise.catchrejection path.
— qwen3.7-max via Qwen Code /review
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
Address remaining actionable PR #4530 review feedback: remove the unreachable prompt-deadline defense helper, keep writer-idle timeout arithmetic-only ranges unrestricted by the JS timer cap, strip route-only deadlineMs before forwarding prompt bodies, harden deadline stderr logging, clarify docs, and cover drain-path writer-idle refresh. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
approved these changes
May 26, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No high-confidence issues found in this round (R5 at 5bc40c7371). The incremental changes since R4 are clean: removed dead sendDeadlineResponseIfFired code, stripped deadlineMs from forwarded body, removed incorrect timer-cap validation for writerIdleTimeoutMs, added try/catch around stderr logging in emitPromptDeadline504, and 5 new test cases (stderr failure, drain-path refresh, deadlineMs stripping, writerIdleTimeoutMs over-cap acceptance, env-var over-cap acceptance). Timer cleanup is thorough, race conditions well-handled, 294 tests pass, CI green 30/30. 5 low-confidence Needs Human Review items (stale JSDoc cross-ref, JSDoc omits MAX_TIMEOUT_MS cap, 3 test gaps) reported in terminal only. — qwen3.7-max via Qwen Code /review
wenshao
approved these changes
May 26, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No new issues found in R6 review (qwen3.7-max). All 9 review agents + reverse audit confirm: timer lifecycle, race conditions, validation parity, and test coverage are solid. 296 tests pass, tsc + eslint clean. — qwen3.7-max via Qwen Code /review
chiga0
approved these changes
May 26, 2026
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
Follow-ups surfaced by an independent simplify-review pass on the initial T2.9 commit. No behavior change; tests still 243/243. - server.ts: gate per-chunk `lastWriteAt = Date.now()` behind a `trackWriterIdle` boolean derived once from `opts.writerIdleTimeoutMs`. Skips the timestamp on every SSE frame when the flag is unset (the default), which matters for chatty tool-output streams (hundreds- to-thousands of writes per session). - runQwenServe.ts: extract `isPositiveIntegerMs` predicate; collapse the two new option-validation blocks and the env parser onto one source of truth. - server.test.ts: extract `abortableBridgePromptImpl()` helper; the 3 new T2.9 prompt-deadline tests had verbatim copies of the "Promise that resolves on abort" snippet. - server.ts + capabilities.ts: trim two repetitive call-site comments + one speculative "hypothetical future sentinel" comment whose value was already covered by the helper JSDoc / boot-time validation. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
…review fold-in) Two Copilot review findings on #4530, both genuine and within the PR's scope. 1. **Prompt deadline must be guaranteed**, not contingent on bridge cooperation. Old shape: `await bridge.sendPrompt(...)` after `abort.abort()`. A buggy agent that ignores AbortSignal could keep the HTTP request open indefinitely; the 504 would never fire — defeating T2.9's whole purpose (the PR description explicitly claims it closes the `FIXME(stage-2)` at `httpAcpBridge.ts:2689`, but it didn't). New shape: `Promise.race(bridge.sendPrompt, deadlinePromise)`. The deadline timer rejects the race independently; the 504 lands on schedule regardless of agent behavior. The orphaned bridge promise gets a tail-attached `.catch(() => undefined)` so its eventual rejection (minutes later, when the buggy agent finally settles) doesn't surface as unhandledRejection. `abort.abort` still fires as best-effort wind-down so a cooperative agent frees its FIFO slot. 2. **Writer-idle-timeout docs were wrong** about values <15s being "no-ops". The idle timer checks elapsed-since-last-flush independently of the heartbeat; with no real events, a `--writer-idle-timeout-ms 10000` evicts a healthy idle SSE connection at 10s, BEFORE the first 15s heartbeat ever refreshes `lastWriteAt`. Updated 4 spots (server.ts comment, types.ts JSDoc, serve.ts CLI help, docs/users/qwen-serve.md table) to recommend ≥30000ms for production and warn that smaller values will evict healthy connections — useful for tests / short dev sessions, dangerous in production. Tests: - New `still emits 504 when the bridge IGNORES the abort signal (race contract)` test: fakeBridge returns a never-settling Promise; route still returns 504 within budget. Locks the new Promise.race contract. - All 244 existing tests still pass (was 243; +1 new). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
Per wenshao's review on #4530, accepted 3 of 6 [Suggestion]s and added test coverage for the gaps called out in the review summary. Pushed back on 3 (separate reply thread per comment with the reasoning). No behavior change for the default-off case; existing 244 tests pass + 25 new. Accepts: - `sendDeadlineResponseIfFired` now writes a stderr breadcrumb with the session id before sending the 504 — symmetric with the SSE writer-idle eviction log. Operators triaging a 504 spike can now `grep "prompt deadline fired"` for the affected session instead of staring at an empty daemon log. Threaded `sessionId` through the helper signature + the direct catch-path 504 path. - `parseDeadlineEnv` no longer treats whitespace-only env vars as silently "unset". `QWEN_SERVE_PROMPT_DEADLINE_MS=" "` now hits the standard "must be a positive integer" boot error — matches the JSDoc's "fail loud on typo" promise. Drove `Number('')` / `Number(' ')` through the existing positive-integer rejection. - Upper-bound validation at `MAX_TIMER_DELAY_MS = 2_147_483_647` (2^31 - 1 ms, ~24.8 days). Node silently compresses `setTimeout` / `setInterval` delays past this to 1ms with a TimeoutOverflowWarning, so an operator setting `--prompt-deadline-ms 2592000000` (30 days) would otherwise see every prompt 504 instantly. Reject loudly at boot with a clear error pointing at the cap. Applied to `promptDeadlineMs`, `writerIdleTimeoutMs`, and the env-var parser for both. Test coverage (5 gaps from review summary): - `parseDeadlineEnv` end-to-end via env-var setting: 6 malformed cases (empty / whitespace / NaN-string / float / negative / zero) + an over-cap case + a happy-path case that asserts the capability tag flips on via the env fallback alone. - `runQwenServe` boot validation for `promptDeadlineMs` and `writerIdleTimeoutMs`: zero / negative / float / NaN via `it.each` (mirrors the existing `mcpClientBudget` test shape at L3525). - `runQwenServe` boot rejection of over-cap deadlines on both options. - Catch-path branch of `sendDeadlineResponseIfFired`: new test where the bridge throws a bare `AbortError` (no typed reason), exercising the defense-in-depth path that still emits 504 by reading `signal.reason`. - `deadlineMs` body validation: `it.each` covering every reachable branch (negative / zero / float / string / boolean / object). NaN/Infinity intentionally absent (JSON.stringify converts them to null, which the validator correctly treats as "absent"). - SSE writer idle with ACTIVE writes: new test where events at ~100ms intervals refresh `lastWriteAt` and the 300ms idle timer doesn't fire. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
5bc40c7 to
ebfbc40
Compare
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
Folded in all 5 items from wenshao's CHANGES_REQUESTED review on PR #4530. One Critical bug fix + 4 Suggestions, all real. 273/273 tests pass (was 269, +4 new including a Critical regression test). Critical (the block reason): - `server.ts` prompt-handler catch block: the early-return on `err instanceof PromptDeadlineExceededError && !res.writableEnded` used the AND-guard for both the body AND the return. When the client disconnected in the same tick the deadline fired, writableEnded was already true → guard failed → fell through to the AbortError branch (false; it's not a DOMException) → reached `sendBridgeError` which called `res.status(500).json(...)` on the already-ended response → `ERR_STREAM_WRITE_AFTER_END` could crash the daemon. Fix: split the guard so `return` always fires on `PromptDeadlineExceededError`; only the JSON write is gated on writableEnded. Added regression test that destroys the client socket mid-prompt and asserts no uncaught throw on the deadline race (`returns cleanly when client disconnects in the same tick the deadline fires`). Suggestions: - `server.ts` SSE eviction: wrapped `writeStderrLine` + `res.end()` in defensive try/catch. If stderr EPIPEs or `res.end()` throws on a destroyed socket, the exception would have escaped the interval callback, `cleanup()` would never run, the heartbeat + idle timers would never clear, and each subsequent tick would re-throw → permanent uncaughtException loop. Now: any throw on either is swallowed, cleanup runs unconditionally. - `server.ts` `sendDeadlineResponseIfFired` JSDoc: rewrote to match the actual call shape — "**called once** from the AbortError catch branch as defense-in-depth" — instead of the stale "BOTH success and catch path" claim left over from before the Promise.race rewrite. - `httpAcpBridge.ts` orphan-promise comment: removed the inaccurate "or the next prompt arrives" reclaim path. The next prompt actually chains BEHIND the orphan via `entry.promptQueue` (L2754 / L2664) and blocks on its eventual settlement — the FIFO is NOT self-healing from a buggy-agent orphan; only session close reclaims. - `runQwenServe.ts` (via tests): closed the `QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS` env-var test gap. Added 3 tests mirroring the prompt-deadline triad (invalid / over-cap / happy-path) so a copy-paste error reading the wrong env-var name in `runQwenServe`'s plumbing fails CI loudly. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
doudouOUC
added a commit
that referenced
this pull request
May 26, 2026
Address remaining actionable PR #4530 review feedback: remove the unreachable prompt-deadline defense helper, keep writer-idle timeout arithmetic-only ranges unrestricted by the JS timer cap, strip route-only deadlineMs before forwarding prompt bodies, harden deadline stderr logging, clarify docs, and cover drain-path writer-idle refresh. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…T2.9) Squashed: 8 commits for clean rebase onto daemon_mode_b_main. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
ebfbc40 to
3013196
Compare
doudouOUC
added a commit
that referenced
this pull request
May 27, 2026
Squashed feature work from daemon_mode_b_main branch, rebased onto latest main to establish proper merge-base and clean PR diff. Original commits: - perf(core): F2 cleanup PR A — R9/W11/W12/R10 (post-merge follow-ups) (#4411) - refactor(acp-bridge): F1 test split — lift bridge.test.ts (6861 LOC) to acp-bridge (#4445) - fix(core): F2 cleanup PR B — self-heal observability (W133-a + W134) (#4460) - feat(sdk/daemon-ui): unified completeness follow-up to #4328 (#4353) - docs(serve): v0.16-alpha known limits + SDK QWEN_SERVER_TOKEN env fallback (PR 27) (#4473) - docs(deploy): local launch templates for v0.16-alpha (PR 30a) (#4483) - feat(daemon+sdk): cross-client real-time sync completeness (#4484) - feat(serve): add POST /session/:id/recap (#4504) - feat(daemon): add voterClientId to permission_resolved (A4) (#4539) - feat(serve): --allow-origin <pattern> CORS allowlist (T2.4 #4514) (#4527) - feat(daemon): in-session model switch reaches the bus (A1) (#4546) - feat(serve): prompt absolute deadline + SSE writer idle timeout (#4514 T2.9) (#4530) - Feat/daemon react cli (#4380)
doudouOUC
added a commit
that referenced
this pull request
May 27, 2026
…T2.9) (#4530) Squashed: 8 commits for clean rebase onto daemon_mode_b_main. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
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
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.
Summary
Closes T2.9 from issue #4514. Two opt-in flags that close the long-running / remote-deployment gaps the 15s heartbeat + AbortSignal don't cover. Both default off → single-user loopback workflows bit-for-bit unchanged.
--prompt-deadline-ms <n>(envQWEN_SERVE_PROMPT_DEADLINE_MS) — server-side wallclock cap onPOST /session/:id/prompt. On expiry, daemon aborts the AbortController and returns504witherrorKind: 'prompt_deadline_exceeded'. Per-prompt body fielddeadlineMscan SHORTEN below the cap but never EXTEND. Closes theFIXME(stage-2)athttpAcpBridge.tsabout a buggy agent holding the FIFO open indefinitely.--writer-idle-timeout-ms <n>(envQWEN_SERVE_WRITER_IDLE_TIMEOUT_MS) — per-SSE-connection idle deadline. When no write has flushed fornms (heartbeat or real event), daemon emits a terminalclient_evictedframe withreason: 'writer_idle_timeout'and closes. Closes the "Stage 2 may add" gap at the SSE handler.Conditional capability tags
prompt_absolute_deadline/writer_idle_timeoutappear only when configured, so SDK consumers pre-flight before sendingdeadlineMs(silently dropped on older daemons).Design rationale
Math.mincap, not absolute server enforcement: operators stay the upper bound. A client can ask for a tighter deadline (e.g. an IDE wanting to bail out after 30s on a 5-min server cap) but cannot loosen one a deployment has chosen.res.writein the idle path (not through thewriteChain): the chain may already be stuck on the verydrainwe're trying to detect. Writing directly gives the terminal frame a chance to land; if it can't, the nextres.end()tears the socket down.nms, force-evict." AwriterIdleTimeoutMs < 15000is documented as a no-op because the next heartbeat refresh races ahead.Files touched
src/status.ts,src/status.test.tssrc/daemon/types.ts,src/daemon/DaemonClient.ts,test/unit/daemon-public-surface.test.tsDAEMON_ERROR_KINDS; adddeadlineMstoPromptRequest; drift-insurance testsrc/commands/serve.ts,src/serve/runQwenServe.ts,src/serve/types.ts,src/serve/capabilities.ts,src/serve/server.ts,src/serve/server.test.tsServeOptionsfields; conditional capability tags; deadline timer in prompt handler; idle timer in SSE handler; 13 new test casesdocs/users/qwen-serve.mdTest plan
npm run typecheckclean across cli + sdk-typescript + acp-bridgepackages/cli/src/serve/server.test.ts— 243/243 pass (13 new T2.9 cases)packages/acp-bridge— 44/44 pass (ordering-pin test updated)packages/sdk-typescript— 422/422 pass (drift-insurance test added)qwen servebinary (deadline path):qwen serve --port 18280 --prompt-deadline-ms 5000→ send a prompt that runs > 5s → expect 504 witherrorKindqwen serve --port 18280 --writer-idle-timeout-ms 30000→ hold a slow SSE consumer → expect close after ~30s withclient_evicted{reason: 'writer_idle_timeout'}Backward compatibility
Both flags default unset. Existing tests pass without changes. SDK consumers ignoring the new capability tags see the same wire contract as before — a missing
prompt_absolute_deadlinetag means the requestdeadlineMsfield is silently dropped, matching the protocol's "additive to v=1" rule.Tracking issue: #4514
🤖 Generated with Qwen Code