fix(cli): rate-limit interim monitor notification turns (#10818) - #10848
Merged
Conversation
A monitor whose command prints on every poll emits one interim <task-notification> per line, and the idle-edge drain started a new model turn per pulse. At ~0.5 Hz the session never returns to idle: Esc only aborts the in-flight turn and the next pulse starts another, while typed input never finds a clean idle edge. Add a session-level minimum interval (10s) between model turns triggered by interim (status 'running') monitor notifications. Queued pulses are not dropped: they batch-drain into a single catch-up turn when the window elapses, re-armed by a timer so a quiet monitor cannot stall the queue. Terminal notifications and cron/loop wakeups stay prompt. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-issue-patrol/jmtk89gukd8
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: observed, not theoretical — #10818 is a P1 with hard evidence: a ~2-day session transcript with 213 interim monitor notifications, a burst of 56 in ~3 minutes (~1 per 2 s), three queued/starved user messages, and concrete reproduction steps. The pulse→turn mechanism behind the DoS is well established. Direction: aligned — this is the issue's own suggested fix #1 (throttle/coalesce interim monitor notifications), and "the session must stay controllable no matter how noisy a background task gets" is core interactive-CLI mission. Size: not applicable — Approach: scope feels right. A session-level minimum interval plus a timer that re-fires the drain when the window elapses (so a monitor going quiet mid-window can't strand its queued pulses) is the minimal mechanism; terminal notifications, cron fires, and loop wakeups stay ungated when they lead the queue. Nothing I'd cut. Risk: no elevated risk signals — no high-risk-path matches in the changed files. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 真实观测到的缺陷，不是理论性加固——#10818 是 P1，证据扎实：约 2 天的会话记录中有 213 条进行中（interim）monitor 通知，其中一个突发窗口约 3 分钟内 56 条（约每 2 秒 1 条），3 条用户输入被排队饿死，并附有具体复现步骤。DoS 背后的"脉冲→模型轮次"机制已被充分证实。 方向： 对齐——这正是 issue 中建议的修复方案 #1（对进行中 monitor 通知做限流/合并），而"无论后台任务多吵，会话必须保持可控"是交互式 CLI 的核心使命。 规模： 不适用—— 方案： 范围合理。会话级最小间隔 + 窗口到期后重新触发排空的定时器（保证 monitor 在窗口期内安静下来时排队的脉冲不会卡死）是最小机制；终态通知、cron 触发、loop 唤醒在位于队首时不受限流。没有可砍的部分。 风险： 无升级风险信号——变更文件未命中任何高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewNo blockers. The mechanism checks out against the current code on
One non-blocking observation: the gate inspects only the queue head, so an item queued behind an in-cooldown interim pulse — e.g. a monitor's TestingCI evidence (fetched via API; PR code is never executed by triage): Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The unit suite ( Sandboxed verification would settle the one claim the suite cannot: Local real-scenario testing: N/A for capture — the change is timing-only (notification turns spaced ≥10 s during a storm) with no visual/UX surface, and triage policy never executes PR code, so no before/after terminal capture was driven. Not verified: live storm behavior in a real session — that is exactly what the 中文说明代码审查无阻塞问题。对照
一个非阻塞观察：门只检查队首，因此排在冷却中的 interim 脉冲后面的项——例如 monitor 打印完最后一行状态后紧接到来的 测试CI 证据（经 API 获取；triage 从不执行 PR 代码）：见上方表格。单元测试套件（运行 2 个新限流测试及该文件既有约 255 个测试）仍在进行中，CI 落定后表格会就地更新。macOS/Windows 单测跳过与当前 沙箱验证可以补上测试套件无法覆盖的最后一环： 本地真实场景测试：录屏不适用——改动仅限时序（风暴期间通知轮次间隔 ≥10 秒），无视觉/交互界面变化，且 triage 策略禁止执行 PR 代码，因此未驱动 before/after 终端录屏。未验证：真实会话中的风暴现场行为——这正是上面 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — a minimal, well-targeted fix for a P1 with hard evidence; the only gap is live-behavior verification, which CI and the Stepping back: my independent proposal for this problem was exactly the shape this PR takes — timestamp-gate interim-monitor-led turns at the idle-edge drain, with a timer so queued pulses flush even when the monitor goes quiet — and I didn't find a simpler path it missed. The problem is emphatically real (a 2-day transcript with 56 notifications in a 3-minute burst is not a hypothesis), the diff is 54 production lines with no drive-by edits, and it re-uses the existing The one reservation is the emergent half of the claim — "clean idle windows reappear, Esc and typed input win them" — which hook-level tests can't reach and which triage can't run itself (PR code is never executed). That is what CI is still running on this commit (unit suite + integration), so approval is deferred until CI lands green on 中文说明回过头看：我为这个问题独立构想的方案与本 PR 的形态完全一致——在空闲边界排空处对 interim monitor 主导的轮次做时间戳限流，并用定时器保证排队的脉冲在 monitor 安静下来后也能被清空——我没有找到它遗漏的更简路径。问题是真实无疑的（2 天的会话记录、3 分钟突发 56 条通知，不是假设），diff 只有 54 行生产代码、没有夹带改动，并复用了既有的 唯一的保留意见是声明中涌现性的那一半——"干净的空闲窗口重新出现，Esc 和键入输入能赢得窗口"——hook 级测试够不到，triage 也不能亲自运行（从不执行 PR 代码）。这正是 CI 在该提交上仍在运行（单测 + 集成），因此批准延迟到 CI 在该提交上全绿后执行——全部检查绿色完成时由 finalize 步骤处理。 — Qwen Code · qwen3.8-max Reviewed at |
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. |
qwen-code-ci-bot
approved these changes
Sep 2, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Test Plan (not a blocker): src/ui/hooks/use-llm-stream.test.tsx — no such file or directory; 257 tests pass — this review observed 28038 passed.
中文说明
已审查——无阻断问题。 建议见行内评论。
Test Plan（非阻断）：src/ui/hooks/use-llm-stream.test.tsx — no such file or directory; 257 tests pass — this review observed 28038 passed。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Comment on lines
+6198
to
+6202
| const leading = notificationQueueRef.current[0]!; | ||
| if ( | ||
| leading.sendMessageType === SendMessageType.Notification && | ||
| leading.monitor?.status === 'running' | ||
| ) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The cooldown gate inspects only queue[0] and defers the ENTIRE drain, so a terminal monitor notification or Cron/loop fire queued behind a gated interim pulse waits up to the full 10 s window — contradicting this block's own invariant ("terminal notifications and cron fires stay prompt"). This is the normal monitor-exit shape, not a corner case: monitor.ts onClose flushes a final running pulse immediately before settling the terminal completed/failed notification, so the queue at the idle edge is [running pulse…, terminal]. The completion signal the user is watching for arrives up to ~10 s late, merged into the catch-up turn. The new does not delay terminal notifications test covers only the terminal-at-queue-head shape.
Witness:
calls 300 ms after stamp (2 = terminal prompt, 1 = terminal deferred): 1
calls after full window: 2 (catch-up contains both payloads)
with the every-item gate fix: calls at 300 ms: 2
Suggested change
| const leading = notificationQueueRef.current[0]!; | |
| if ( | |
| leading.sendMessageType === SendMessageType.Notification && | |
| leading.monitor?.status === 'running' | |
| ) { | |
| const allInterimPulses = notificationQueueRef.current.every( | |
| (item) => | |
| item.sendMessageType === SendMessageType.Notification && | |
| item.monitor?.status === 'running', | |
| ); | |
| if (allInterimPulses) { |
The fix must keep the batch-membership rule at use-llm-stream.ts:6276-6281 (same sendMessageType + same todoWorkChainId) and the Cron single-item drain (~line 6244) intact. Please extend the new does not delay terminal notifications behind the interim cooldown test: drain one pulse, then enqueue a second pulse followed by a terminal notification within the window and assert mockSendMessageStream fires again without advancing fake timers by the interval — removing the every-item gate must red it; run that mutation to confirm.
中文说明
冷却门控只检查 queue[0] 并推迟整个排空，因此排在一个被限流的进行中脉冲之后的终态 monitor 通知或 Cron/loop 触发要等满最长 10 秒的窗口——与本代码块自身的不变量（"终态通知与 cron 触发保持即时"）矛盾。这不是边界情况，而是 monitor 的正常退出路径：monitor.ts 的 onClose 会先冲刷出最后一条 running 脉冲，随后才发出 completed/failed 终态通知，所以空闲边界上的队列形状是 [running 脉冲…, 终态]。用户等待的完成信号最多延迟约 10 秒、并入补充轮次才送达。新增的 does not delay terminal notifications 测试只覆盖了终态通知位于队首的场景。
Witness（探针输出）：未改动代码在打戳后 300 ms 的调用次数为 1（终态被延迟），完整窗口后为 2（补充轮次包含两条内容）；采用"全部为进行中脉冲才门控"的修复后，300 ms 时即为 2。
修复：仅当队列中所有项都是进行中（running）脉冲时才推迟排空。约束：须保留 use-llm-stream.ts:6276-6281 的批量划分规则（相同 sendMessageType + 相同 todoWorkChainId）与 Cron 单项排空（约 6244 行）。验收标准：请扩展新增的 does not delay terminal notifications behind the interim cooldown 测试——先排空一条脉冲，再在窗口期内入队第二条脉冲和一条终态通知，断言 mockSendMessageStream 无需推进整段间隔即再次触发；移除 every 门控后该测试应变红——请实际跑一次该突变确认。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Comment on lines
+6285
to
+6287
| if (batch[0]?.monitor?.status === 'running') { | ||
| lastInterimMonitorTurnAtRef.current = Date.now(); | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] The cooldown stamp keys on batch[0] only. Background task/shell/workflow completions push SendMessageType.Notification items with no monitor field and todoWorkChainId undefined, and the split loop above (lines 6276-6281) batches on same type + same chain id — so [shell-completion, interim-pulse] drains as one batch: the gate did not fire (the head is not a monitor item) and no stamp is recorded either (batch[0] has no .monitor). The turn containing the pulse consumes no cooldown, and the very next pulse starts its own turn immediately — in any session with other active notification producers, the per-pulse turn churn this PR exists to stop recurs.
Witness:
turn 1 contains shell-done: true, contains pulse-1: true (single mixed batch)
calls after pulse-2 inside window (1 = gated, 2 = churn): 2
with batch.some(...) stamping: 1
Suggested change
| if (batch[0]?.monitor?.status === 'running') { | |
| lastInterimMonitorTurnAtRef.current = Date.now(); | |
| } | |
| if (batch.some((item) => item.monitor?.status === 'running')) { | |
| lastInterimMonitorTurnAtRef.current = Date.now(); | |
| } |
Please add a test that records a cooldown, enqueues a background-shell notification followed by an interim pulse and asserts one batched turn, then enqueues another pulse immediately and asserts no turn until INTERIM_MONITOR_MIN_TURN_INTERVAL_MS elapses — reverting to the batch[0] check must red it; run that mutation to confirm.
中文说明
冷却打戳只按 batch[0] 记录。后台任务/shell/工作流完成通知推入的是不带 monitor 字段、todoWorkChainId 为 undefined 的 SendMessageType.Notification 项，而上方的批量划分循环（6276-6281 行）按相同类型 + 相同工作链 id 分批——因此 [shell 完成, 进行中脉冲] 会作为同一批排空：门控未触发（队首不是 monitor 项），打戳也不会记录（batch[0] 没有 .monitor）。包含脉冲的轮次不消耗任何冷却，下一条脉冲随即开启自己的轮次——在任何存在其他通知生产者的会话里，本 PR 要消除的"每脉冲一轮"风暴会复发。
Witness（探针输出）：未改动代码的首轮同时包含 shell-done 与 pulse-1（单一混合批次），窗口内 pulse-2 之后的调用次数为 2（风暴复发）；采用 batch.some(...) 打戳后为 1。
验收标准：请新增测试——记录冷却后，先入队一条后台 shell 通知和一条进行中脉冲，断言合并为一轮；再立即入队一条脉冲，断言在 INTERIM_MONITOR_MIN_TURN_INTERVAL_MS 结束前不再开轮；还原为 batch[0] 检查后测试应变红——请实际跑一次该突变确认。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Comment on lines
+5957
to
+5959
| // Last time an interim-monitor-led notification batch started a model turn | ||
| // (#10818 cooldown). | ||
| const lastInterimMonitorTurnAtRef = useRef(0); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This ref is billed as a "session-level" interval but survives session swaps: the sessionId-change effect (~lines 5995-6003) clears notificationQueueRef on /clear yet leaves lastInterimMonitorTurnAtRef stale. The monitor registry is config-scoped, so a still-running monitor keeps pulsing into the new session; its first pulse computes elapsed against the previous session's timestamp and waits up to ~10 s instead of draining immediately — defeating the useRef(0) "first pulse drains immediately" design for the fresh session.
Witness:
new-session first pulse calls (2 = prompt after swap, 1 = gated by old-session stamp): 1
with lastInterimMonitorTurnAtRef.current = 0 added to the swap effect: 2
// in the sessionId-change effect (~lines 6000-6003):
notificationQueueSessionIdRef.current = sessionStates.sessionId;
notificationQueueRef.current = [];
lastInterimMonitorTurnAtRef.current = 0;
autonomousLoopTickResolverRef.current?.resetCache();The reset belongs in the existing [sessionStates.sessionId]-keyed effect that owns the queue clear (use-llm-stream.ts:6000-6003), without growing its dep array. Please add a test that drains an interim turn, changes sessionStates.sessionId, fires one pulse and asserts it drains immediately — removing the reset must red it; run that mutation to confirm.
中文说明
该 ref 被称为"会话级"间隔，却在会话切换后存活：sessionId 变更 effect（约 5995-6003 行）在 /clear 时清空 notificationQueueRef，却不重置 lastInterimMonitorTurnAtRef。monitor 注册表以 config 为作用域，仍在运行的 monitor 会继续向新会话发送脉冲；新会话的第一条脉冲按上一会话的时间戳计算 elapsed，最多要等待约 10 秒而不是立即排空——useRef(0) 为全新会话保证的"首条脉冲立即送达"设计因此失效。
Witness（探针输出）：未改动代码下新会话首条脉冲的调用次数为 1（被旧会话打戳门控）；在切换 effect 中加入 lastInterimMonitorTurnAtRef.current = 0 后为 2（立即送达）。
修复：在 sessionId 变更 effect 中清空队列的同一处加上 lastInterimMonitorTurnAtRef.current = 0;。约束：重置必须放在拥有队列清理的现有 [sessionStates.sessionId] 依赖 effect（use-llm-stream.ts:6000-6003）内，且不得扩充其依赖数组。验收标准：请新增测试——先排空一次进行中轮次，然后更换 sessionStates.sessionId，触发一条脉冲并断言立即排空；移除重置后测试应变红——请实际跑一次该突变确认。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Comment on lines
+6285
to
+6287
| if (batch[0]?.monitor?.status === 'running') { | ||
| lastInterimMonitorTurnAtRef.current = Date.now(); | ||
| } |
Collaborator
There was a problem hiding this comment.
[Suggestion] The timestamp is stamped when the batch is spliced, before submitQuery's admission/goal-claim checks run. When onAdmissionFailed (~lines 3498/3510/3723) or onGoalClaimDeferred (~line 3752) unshifts the batch back onto the queue, no model turn ever ran — yet the requeued pulses see elapsed < 10 s and wait a fresh window of up to 10 s. That contradicts this ref's own comment ("Last time an interim-monitor-led notification batch started a model turn").
Witness:
(goal-claim defers the first attempt)
stream calls right after deferred claim: 0; after 1.2 s: 0; after full window: 1
with stamp-restore in the requeue callbacks: 1 call right away
const prevStamp = lastInterimMonitorTurnAtRef.current;
const batch = queue.splice(0, splitIdx);
if (batch[0]?.monitor?.status === 'running') {
lastInterimMonitorTurnAtRef.current = Date.now();
}
// then in onAdmissionFailed / onGoalClaimDeferred, for an interim-led batch:
// lastInterimMonitorTurnAtRef.current = prevStamp;Please add a test that forces admission failure of an interim-led batch and asserts the requeued batch retries promptly rather than after a full interval — removing the restore must red it; run that mutation to confirm.
中文说明
时间戳在批次被切出时就已写入，早于 submitQuery 的准入/目标认领检查。当 onAdmissionFailed（约 3498/3510/3723 行）或 onGoalClaimDeferred（约 3752 行）把批次退回队列时，模型轮次其实从未开始——但重新入队的脉冲看到 elapsed < 10 s，又要再等最长 10 秒的新窗口。这与该 ref 自身的注释（"进行中 monitor 引领的通知批次上一次启动模型轮次的时间"）矛盾。
Witness（探针输出）：未改动代码（目标认领延迟了首次尝试）下，延迟认领后的即时调用次数为 0，1.2 秒后仍为 0，完整窗口后才为 1（共 2 次认领尝试）；在退回回调中加入"还原打戳"后，立即得到 1 次调用。
修复：在切出批次前捕获旧打戳，并在进行中批次的 onAdmissionFailed/onGoalClaimDeferred 中还原它（还原捕获值而非 0——0 会让下一批完全绕过门控）。验收标准：请新增测试——强制进行中批次的准入失败，断言退回的批次及时重试而非等待整个间隔；移除还原逻辑后测试应变红——请实际跑一次该突变确认。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Comment on lines
+6208
to
+6211
| const timer = setTimeout( | ||
| () => setNotificationTrigger((n) => n + 1), | ||
| INTERIM_MONITOR_MIN_TURN_INTERVAL_MS - elapsed, | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The remaining-time scheduling (INTERVAL − elapsed) is pinned by no test. The new rate-limit test enqueues its pulses ~0.1-0.2 s after the stamp and then advances the full interval — the catch-up fires under a fixed full-interval delay too. With exactly that mutant applied, the entire 257-test file still passes; in production a pulse arriving at t≈9 s would get its catch-up turn at t≈19 s instead of t≈10 s — nearly twice the documented worst-case latency — with CI green.
Witness:
mutant (INTERVAL − elapsed → INTERVAL) + full file: Tests 257 passed (257) — mutant survives
partway-advance probe: passes intact; on the mutant: expected "spy" to be called 2 times, but got 1 times
// new test: after the first pulse drains and its turn resolves
await vi.advanceTimersByTimeAsync(9_000);
// enqueue one more interim pulse here (arrives mid-window)
await vi.advanceTimersByTimeAsync(1_100); // just past the remaining time
expect(mockSendMessageStream).toHaveBeenCalledTimes(2);The new test is its own acceptance criterion — it must red when the timer delay changes from INTERIM_MONITOR_MIN_TURN_INTERVAL_MS - elapsed to the full INTERIM_MONITOR_MIN_TURN_INTERVAL_MS; run that mutation to confirm.
中文说明
剩余时间调度（INTERVAL − elapsed）没有任何测试固定：新增的限流测试在打戳后约 0.1-0.2 秒入队脉冲，然后推进完整间隔——在"固定完整间隔"的延迟下补充轮次同样会触发。应用这个突变后，全部 257 个测试仍然通过；而在生产中，t≈9 秒到达的脉冲要到 t≈19 秒才得到补充轮次，而不是 t≈10 秒——几乎是文档承诺的最坏延迟的两倍——CI 却全绿。
Witness（探针输出）：应用突变（INTERVAL − elapsed → INTERVAL）后跑整个文件：Tests 257 passed (257)——突变存活。"推进到窗口中段"探针在未改动代码上通过；在突变上失败：expected "spy" to be called 2 times, but got 1 times。
修复：新增测试——先排空首条脉冲，把假定时器推进到窗口中段（如 9 秒），再入队一条脉冲，然后只推进略多于剩余时间（约 1.1 秒），断言补充轮次已触发——即固定"在 lastTurn + INTERVAL 触发"的语义。验收标准：该新测试本身——当定时器延迟从 INTERIM_MONITOR_MIN_TURN_INTERVAL_MS - elapsed 改为完整 INTERIM_MONITOR_MIN_TURN_INTERVAL_MS 时，它必须变红——请实际跑一次该突变确认。
— qwen3.8-max via Qwen Code /review (v0.22.3)
| * notifications and cron fires stay prompt. Queued pulses still batch-drain | ||
| * into a single catch-up turn once the window elapses, so no update is lost. | ||
| */ | ||
| export const INTERIM_MONITOR_MIN_TURN_INTERVAL_MS = 10_000; |
Collaborator
There was a problem hiding this comment.
[Suggestion] This new user-visible rate limit is undocumented while its emission-side sibling is: docs/developers/tools/monitor.md ("Output handling" bullet) documents the core-side emission throttle (excess lines are dropped), but nothing says interim pulses now start at most one model turn per 10 s in the interactive CLI. During the cooldown, queued pulses are neither delivered nor displayed (addItem runs inside the drain), so a user tailing a chatty monitor sees output batched at ≥10 s intervals with only the drop-based throttle documented to explain it — the opposite behaviour.
Witness:
witness: not run — documentation-existence claim; verified by reading docs/developers/tools/monitor.md (only rate-limit mention is the core emission throttle) and the diff's file list (monitor.md untouched); no run capability applies
<!-- docs/developers/tools/monitor.md, add to the "Output handling" bullet -->
In the interactive CLI, interim monitor pulses start at most one model turn per 10 seconds; pulses inside the window batch into a single catch-up notification. Terminal notifications are unaffected when they lead the queue.中文说明
这个新的用户可见限流没有文档，而它的"兄弟"发射端限流却有：docs/developers/tools/monitor.md（"Output handling" 条目）记录了 core 侧的发射节流（超限行会被丢弃），但没有任何地方说明交互式 CLI 现在对进行中脉冲最多每 10 秒开启一次模型轮次。冷却期间，排队中的脉冲既不送达也不展示（addItem 在排空时才执行），用户盯着一个高频输出的 monitor 会看到输出按 ≥10 秒成批出现，而唯一的 monitor 文档描述的是"丢弃"语义——行为恰恰相反——无法解释这一现象。
Witness：未运行——这是文档存在性主张；通过阅读 docs/developers/tools/monitor.md（唯一的限流描述是 core 侧发射节流）与 diff 文件列表（未改动 monitor.md）验证；没有可适用的运行能力。
修复：在 docs/developers/tools/monitor.md 的 "Output handling" 条目中补一句：在交互式 CLI 中，进行中的 monitor 脉冲最多每 10 秒开启一次模型轮次，窗口期内的脉冲会合并为一次补充通知；终态通知在位于队首时不受影响。
— qwen3.8-max via Qwen Code /review (v0.22.3)
Collaborator
tmux e2e report — A/B against the PR base, real TUI, real
|
| metric (60 s window) | base 4f212873b5 |
PR 21b1cbe975 |
|---|---|---|
| main-session model requests | 19 | 7 |
| — of which notification-led turns | 17 | 4 |
| total API requests (incl. suggestion forks) | 38 | 13 |
| median gap between main requests | 2.6 s | 7.8 s |
| minimum gap | 1.5 s | 4.6 s |
| gaps between notification-led turns | 1.5–6.4 s | 7.5 s, 10.0 s, 9.8 s |
Base notification-led turns (seconds from first request): 20.0, 23.6, 26.2, 27.7, 31.6, 38.0, 40.1, 42.4, 44.1, 46.1, 48.7, 50.7, 53.0, 55.4, 57.6, 61.0, 63.7 — a new model turn every ~2.6 s for the whole window, i.e. exactly the "session never returns to idle" behaviour described in the issue.
PR notification-led turns: 25.4, 32.9, 42.9, 52.7. The 4.6 s and 7.8 s gaps earlier in that arm are the user-prompt turn and its post-tool-result continuation (last=tool, Monitor started. id: mon_…) — not notification-led, and correctly not gated.
On screen the difference is plain: the base arm rendered a separate ◆︎ PULSE turn after every one or two Monitor "pulse-watch" event #N lines and was still mid-turn when the window closed; the PR arm rendered a single turn answering PULSE nine times in one go — the queued pulses batch-drained into one catch-up turn.
So: 2.7× fewer main-session turns, 2.9× fewer total API requests, and no pulse dropped.
Scenario 2 — does a terminal notification stay prompt?
Both arms: Monitor on for i in $(seq 1 10); do echo tick-$i; sleep 0.5; done; echo MONITOR-DONE — ~5 s of pulses, then the command exits, so the terminal notification lands right behind the interim ones.
Both arms produced exactly one notification-led turn, and in both that turn's combined prompt carried all 11 notifications — <status> values ['running' ×10, 'completed'], with tick-1…tick-10 and the MONITOR-DONE line. The terminal event is never lost, and batching is unchanged by this PR.
Timing: measured from the request carrying the Monitor started. tool result, base's combined turn went out 5.3 s later, the PR's 8.3 s later — ~3 s of extra latency. That is the queue[0]-only check: while interim pulses sit at the head, the whole batch (terminal event included) waits out the cooldown. This independently reproduces the automated review's suggestion R1-1 on a live session, and confirms it is bounded (≤ INTERIM_MONITOR_MIN_TURN_INTERVAL_MS) and delivers, so it reads as a Suggestion rather than a blocker — strictly better than base's turn-every-2.6 s.
Conclusion
Mergeable. Interim pulses stop driving one model turn each, queued pulses still collapse into a single catch-up turn with every event present, terminal notifications still arrive (≤10 s late only when queued behind interim pulses), and nothing outside the notification path is gated — the initial prompt turn and its tool-result continuation were untouched in the ledger above.
Artifacts kept locally: tmux-readable-full.log (primary — step-by-step rendered frames for both scenarios and both arms), per-arm final captures, and metrics-{pr,base}.txt.
qqqys
approved these changes
Sep 2, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Approving on the strength of the tmux e2e A/B posted above (PR head 21b1cbe975 vs its own base 4f212873b5, both built from source and driven through the same real TUI scenario).
Live results: interim monitor pulses drop from 17 notification-led model turns per 60 s to 4 (total API requests 38 -> 13, median inter-turn gap 2.6 s -> 7.8 s); queued pulses still batch-drain into a single catch-up turn with every event present, so no update is lost; and a terminal notification queued behind interim pulses is delayed by at most the 10 s cooldown (measured ~3 s in that scenario) but is always delivered. Turns outside the notification path are untouched — the initial prompt turn and its post-tool-result continuation kept their 4.6 s / 7.8 s spacing in the ledger.
All product CI lanes are green at head. The automated review reports no blockers; its six suggestions are non-blocking, including R1-1 (the queue[0]-only check), which the e2e reproduced on a live session and confirmed to be bounded and non-lossy.
Collaborator
|
Released in v0.23.0. |
qqqys
added a commit
to qqqys/qwen-code
that referenced
this pull request
Sep 9, 2026
…at was dropped (QwenLM#11351) * fix(cli): cap the background-notification queue and tell the model what was dropped The unified notification queue in the interactive TUI was an unbounded array: all six producers pushed without checking its length, and it was only ever cleared on a session switch. A noisy producer could therefore accumulate an arbitrary backlog that a single idle edge then fed into one model turn. The interim-monitor cooldown added in QwenLM#10848 limits how often pulses start a turn, but pulses still pile up unbounded inside the cooldown window. Extract the admission rule the ACP Session already had into `background-notification-queue.ts` so both front ends share one cap (20) and one eviction order, and record every loss instead of writing it only to a debug log. Eviction prefers the oldest interim monitor pulse — the monitor's next poll supersedes it — and otherwise takes the oldest unprotected entry. The TUI protects agent results, workflow results and cron prompts, none of which have a second copy anywhere; when every queued entry is protected the incoming notification is dropped instead. ACP keeps its existing protection rule (entries continuing the todo-stop-guard work chain) and its existing order: it filters interim pulses before they are queued, so pulse priority is inert there. Losses are reported once, on the next drained turn, as a single summary line naming how many notifications went and which tasks they came from — per-loss lines would reproduce the flooding the cap exists to stop. The summary is parked if that turn is rejected, so a failed admission cannot lose the only record of what was discarded. * fix(cli): let the admission rule read the arriving notification Review follow-up on the review of 8661630. `decideNotificationAdmission` never read its `incoming` parameter, which told the next reader the arriving item mattered to the decision when it did not. Neither tsc nor ESLint flags it — `noUnusedParameters` is off and the rule's default `args: after-used` ignores a parameter followed by a used one — so the signature would have shipped misleading. Give it the meaning it should have had: when the queue holds no interim pulse to evict and the arriving notification is itself a pulse, drop the newcomer rather than displace a terminal result. A pulse is superseded by the monitor's next poll, so evicting the only copy of a shell result to make room for one trades the wrong way — the same reasoning that already puts queued pulses first in line. ACP is unaffected: it filters pulses before they are ever queued, so an arriving item there is never interim. Also from the review: - Project the ACP queue for the admission decision, not just for the tally. `interim` exists only on the projection, so passing raw entries would have silently disabled pulse priority for whoever relaxes the ACP monitor filter, which is exactly what the projection's docstring promised would keep working. - Reattach the `QueuedNotification` docstring, which had been left stranded above `PendingDroppedSummary` when the two interfaces were reordered. - Record at the call site why the ACP does not park the summary across a refused turn the way the TUI does: past admission the summary shares the notification's fate, and that path does not re-queue the item either. - Stop the docs claiming "nothing is lost silently". The drain prunes pulses from cancelled monitors and can empty the queue before the summary is taken, so the summary rides along with whatever notification drains next and is discarded unreported if the session is cleared first. * fix(cli): preserve notification overflow semantics * fix(cli): preserve notification overflow reporting --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com>
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
4 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.
What this PR does
Adds a session-level minimum interval (10s,
INTERIM_MONITOR_MIN_TURN_INTERVAL_MS) between model turns triggered by interim (status: running) monitor notifications in the interactive CLI's idle-edge drain (packages/cli/src/ui/hooks/use-llm-stream.ts). Queued pulses are not dropped: they accumulate and batch-drain into a single catch-up turn when the window elapses, and a short timer re-fires the drain effect so a monitor that goes quiet mid-window cannot stall its queued updates. Terminal notifications (completed/failed), cron fires, and loop wakeups are not gated and stay prompt.Why it's needed
A monitor whose command prints on every poll emits one
<task-notification>per line, and the drain started a new model turn per pulse. At the reported ~0.5 Hz pulse rate the session never returns to idle:Esconly aborts the in-flight turn (cancelOngoingRequestnever touches the notification queue) so the next pulse starts another turn within seconds, and typed input never finds a clean idle edge to win the queue race. With the cooldown, clean idle windows exist again — Esc regains control, and queued user input wins the next idle edge via the existinghasQueuedUserMessagespriority gate. Token burn from pulse turns is capped at one turn per interval.Reviewer Test Plan
How to verify
npx vitest run src/ui/hooks/use-llm-stream.test.tsx -t "rate-limits interim"inpackages/cliagainstmainfails withexpected sendMessageStream to be called 1 time, got 2— the second interim pulse starts its own model turn immediately, demonstrating the pulse→turn mechanism behind the DoS.npx vitest run src/ui/hooks/use-llm-stream.test.tsx— all 257 tests pass (covers: first notification drains immediately, batching, per-chain separation, terminal notifications, cron/loop paths). A dedicated test asserts terminal monitor notifications are not delayed behind the interim cooldown.npx tsc --noEmit(packages/cli) and prettier/eslint on changed files: clean.Evidence (Before & After)
Before: new test fails on unpatched code (
expected "spy" to be called 1 times, but got 2 times). After: 257/257 pass. TUI behavior change is timing-only (notification turns spaced ≥10s apart during a pulse storm); no visual/UX surface changed, verified at the harness level rather than terminal-capture (headless Linux box, no TTY).Tested on
Environment (optional)
N/A — unit tests (vitest) only.
Risk & Scope
Linked Issues
Fixes #10818
中文说明
这个 PR 做了什么
在交互式 CLI 的空闲边界通知排空逻辑里（
packages/cli/src/ui/hooks/use-llm-stream.ts），为进行中（status: running）的 monitor 通知触发的模型轮次增加会话级最小间隔（10 秒，INTERIM_MONITOR_MIN_TURN_INTERVAL_MS）。排队中的脉冲不会被丢弃：它们会在窗口结束后合并成一次补充轮次批量送达，另有一个短定时器重新触发排空 effect，保证 monitor 在窗口期内安静下来时队列不会卡死。终态通知（completed/failed）、cron 触发和 loop 唤醒不受限流，保持即时。为什么需要
每轮 poll 都打印输出的 monitor 会为每行 stdout 发出一条
<task-notification>，排空逻辑此前为每个脉冲各开一次模型轮次。按报告中约 0.5 Hz 的脉冲频率，会话永远回不到空闲：Esc只能中止当前轮次（cancelOngoingRequest不触碰通知队列），下一个脉冲几秒内又开新一轮，用户键入的输入永远等不到干净的空闲边界。加了冷却后出现干净的空闲窗口——Esc 重新生效，已排队的用户输入通过现有的hasQueuedUserMessages优先门控赢得下一个空闲边界。脉冲轮次的 token 消耗也被限制在每窗口一次。审查者验证
main上于packages/cli跑npx vitest run src/ui/hooks/use-llm-stream.test.tsx -t "rate-limits interim"，失败：expected sendMessageStream to be called 1 time, got 2——第二个进行中脉冲立即开启了新的模型轮次，证实 DoS 背后的脉冲→轮次机制。npx vitest run src/ui/hooks/use-llm-stream.test.tsx全部 257 个测试通过（覆盖：首条通知立即送达、批量合并、按 work chain 分组、终态通知、cron/loop 路径）。另有专门测试断言终态 monitor 通知不被冷却延迟。npx tsc --noEmit（packages/cli）与变更文件的 prettier/eslint：全部干净。证据：修复前新测试在未打补丁代码上失败（
expected "spy" to be called 1 times, but got 2 times）；修复后 257/257 通过。TUI 行为变化仅限时序（脉冲风暴期间通知轮次间隔 ≥10 秒），无视觉/交互界面改动，在无 TTY 的 Linux 机器上以 harness 级验证代替终端录屏。风险与范围：脉冲风暴期间进行中的 monitor 更新最多延迟 10 秒到达模型（被合并而非丢失）；终态一次性通知不受影响。未覆盖：真实终端端到端录屏、Esc 文案调整、队列内按 monitor 合并（批量排空已把排队脉冲合并为一轮）。无破坏性变更。
关联 issue：Fixes #10818