feat(serve): observe daemon and child memory against real denominators - #8423
Conversation
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR — the description itself is unusually thorough, but it doesn't follow the PR template, so the gate stops here before any code review.
The body uses freeform sections (## Why, ## What, ## The flag, ## Scope, ## Compatibility, ## Verification) and is missing every required heading from pull_request_template.md:
## What this PR does## Why it's needed## Reviewer Test Plan— including### How to verify,### Evidence (Before & After), and the### Tested onOS matrix## Risk & Scope## Linked Issues- The Chinese translation inside the template's
<details>block
Part 1 of this series (#8245) followed the template to the letter, so this reads as an oversight rather than missing information — nearly all of the content is already written and just needs to be reshaped into the template's sections. Two concrete gaps the reshaping should close: the "Tested on" table (the Verification section doesn't say which OS the 174 unit + 6 e2e tests ran on), and explicit Linked Issues entries for #8051 / #8245, which are currently referenced only in prose.
Once the body follows the template, push an update (or re-run with @qwen-code /triage) and it will go through the gate again.
中文说明
感谢贡献——PR 描述本身写得相当详实,但没有遵循 PR 模板,因此门禁在代码审查之前就停在这里。
正文使用了自由格式的小节(## Why、## What、## The flag、## Scope、## Compatibility、## Verification),缺少 pull_request_template.md 要求的全部标题:
## What this PR does## Why it's needed## Reviewer Test Plan—— 包括### How to verify、### Evidence (Before & After)和### Tested on操作系统矩阵## Risk & Scope## Linked Issues- 模板
<details>块中的中文翻译
本系列的第 1 部分(#8245)是严格按模板写的,所以这更像是疏忽而不是信息缺失——绝大部分内容已经写好了,只需要重新组织成模板的结构。重整时应补上两个具体缺口:"Tested on" 表格(Verification 一节没有说明 174 个单测 + 6 个端到端测试是在哪些操作系统上跑的),以及把 #8051 / #8245 写进显式的 Linked Issues(目前只在正文里提到)。
正文符合模板后,推送更新(或用 @qwen-code /triage 重跑)即可重新过门禁。
— Qwen Code · qwen3.8-max-preview
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
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
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Unresolved, please confirm: [Critical] Existing CHANGES_REQUESTED review by @qwen-code-ci-bot (triage stage 1a, review 4840811017): the PR body does not follow the PR template (missing required headings incl. Reviewer Test Plan and Linked Issues). This is a PR-body/process requirement of the project's triage gate — it cannot be ruled on from the code at the reviewed commit. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| runtimeMemory && | ||
| runtimeMemory.pressure.mode === 'observe' && | ||
| runtimeMemory.pressure.level !== 'normal' |
There was a problem hiding this comment.
[Suggestion] The level !== 'normal' clause is the only part of this gate with no test on either side — the sole issue-raising test runs only at critical (the 1 MiB denominator), and no budget-resolving test asserts the issue is absent at normal or present at soft/hard. — Failure scenario: mutation-probed live: (a) deleting this clause keeps the entire suite green (including the booted-daemon e2e) while default observe mode raises daemon_memory_pressure on a healthy daemon, flipping top-level status from ok to warning on every /daemon/status response — exactly the false-positive status flip --memory-pressure-mode off exists to opt out of; (b) tightening to level === 'critical' is equally green and silently drops the warning at soft/hard. Both contradict the documented contract ("raises a daemon_memory_pressure issue whenever the level leaves normal"). Suggested fix: add (1) a budget-resolving default-mode test asserting no daemon_memory_pressure issue and status === 'ok' at a realistic denominator, and (2) a denominator that lands the test process in soft asserting one warning issue appears.
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| // Ties go to RSS. Arbitrary but deterministic, and it only arises when the | ||
| // two ratios are equal — in which case either name describes the same | ||
| // number, and `level` is unaffected either way. | ||
| else source = rssRatio >= heapRatio ? 'rss' : 'heap'; |
There was a problem hiding this comment.
[Suggestion] The documented tie-break ("ties are reported as rss", added to qwen-serve-protocol.md by this same diff) has zero test coverage — every exact-tie input in the unit suite omits source from its assertions. — Failure scenario: mutation-probed: changing >= to > here flips source from rss to heap whenever both denominators are measurable and the ratios are exactly equal, and no test fails. The suite itself constructs exact ties (2 GB/8 GB vs 1 GB/4 GB — both exactly 0.25) but asserts only the six raw figures. Consumers — including this feature's own issue-message ternary source === 'heap' ? … — then silently get the other denominator named, contradicting the wire contract. Suggested fix: add source: 'rss' to the toMatchObject of the existing exact-tie case in daemon-memory-pressure.test.ts (or add a dedicated one-line tie case).
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| export const SOFT_PRESSURE_RATIO = 0.5; | ||
| export const HARD_PRESSURE_RATIO = 0.65; | ||
| export const CRITICAL_PRESSURE_RATIO = 0.8; |
There was a problem hiding this comment.
[Suggestion] These constants duplicate core's DEFAULT_PRESSURE_CONFIG (0.5/0.65/0.8 in memoryPressureMonitor.ts) with nothing pinning the two copies together. The duplication rationale in the comment checks out (the monitor is private to Config.initialize(); extracting from packages/core/src/services/** is maintainer-gated) — the gap is the unbridged interim drift. — Failure scenario: DEFAULT_PRESSURE_CONFIG is a live tuning knob (spread in Config, validated by validateMemoryPressureConfig); if it is retuned, the daemon copy stays stale, so soft/hard/critical silently mean different things in the interactive CLI vs daemon status, and the "Thresholds mirror MemoryPressureMonitor … the established contract" claim above becomes false with no test red. Suggested fix: pin the copy — a test asserting the three constants equal the matching DEFAULT_PRESSURE_CONFIG fields (a test-time import only, so the runtime module-graph concern does not apply), and/or file the follow-up issue the comment defers the extraction to.
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| `Daemon memory pressure is ${level} at ` + | ||
| `${(ratio * 100).toFixed(0)}% of ` + |
There was a problem hiding this comment.
[Suggestion] toFixed(0) can display the next level's documented threshold while naming the current level. — Failure scenario: executed arithmetic — a ratio of 0.795–0.799 classifies as hard but the message reads "Daemon memory pressure is hard at 80% of available memory.", and 80% is precisely critical's documented threshold (0.8); 0.645–0.649 reads "soft at 65%", hard's threshold. An oncall engineer comparing the message against the documented thresholds sees a contradiction and wastes triage time doubting the classifier — in the one feature whose purpose is trustworthy triage. The band is narrow (within 0.5 percentage points below a threshold), but a daemon climbing toward a threshold passes through it continuously.
| `Daemon memory pressure is ${level} at ` + | |
| `${(ratio * 100).toFixed(0)}% of ` + | |
| `Daemon memory pressure is ${level} at ` + | |
| `${(ratio * 100).toFixed(1)}% of ` + |
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| | 'daemon_log_degraded' | ||
| | 'daemon_memory_pressure'; |
There was a problem hiding this comment.
[Suggestion] Test-efficacy probe: reverting this hunk on its own leaves every test in this diff green — no runtime test turns on this declaration; the only guard is TypeScript compilation, which CI runs via npm run build in the e2e workflow (ci.yml has no dedicated typecheck step). — Failure scenario: vitest runs without type-checking, so an edit that removes or renames this issue code while keeping runtime behavior compiling passes the whole suite green; only the compiler notices, and only on a CI path that builds. The cost is silent drift of the status wire contract. (The feature as a whole IS gated — reverting the entire source fails 5 assertions — this says only that this hunk is not what any test turns on.) Suggested fix: pin the issue emission with a runtime assertion — the existing test filters issues by code but reads only severity; extend it so the code string is test-gated, not tsc-gated only.
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
| * it. Hoisting it out would restructure the block for a path that does not | ||
| * need the reading. | ||
| */ | ||
| pressure: DaemonMemoryPressure & { mode: 'off' | 'observe' }; |
There was a problem hiding this comment.
[Suggestion] Test-efficacy probe: reverting this hunk (the pressure field declaration) on its own leaves every test in this diff green — the runtime wiring still populates the object, and the existing toEqual blocks assert only pressure: objectContaining({ mode }), which survives a declaration-only revert. As with the sibling hunk on the issue-code union, the only guard is TypeScript compilation (run in CI via the e2e workflow's npm run build, not a dedicated ci.yml typecheck step). — Failure scenario: a rename or removal here that keeps the runtime shape compiling passes the entire test suite green; shape drift of the runtime.memory.pressure wire payload is caught only by the compiler. Suggested fix: pin the wire shape with runtime assertions in daemon-status.test.ts (the full key-set of pressure — level/ratio/source/mode plus the six raw figures), so the declaration is test-gated, not tsc-gated only.
— qwen3.8-max-preview via Qwen Code /review (v0.21.4)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
[Critical] Still-standing blocker from round 1 (re-checked this round against the live PR description): the PR body does not follow the PR template — it uses freeform sections (## Why / ## What / ## The flag / ## Scope / ## Compatibility / ## Verification) and is missing every required heading from .github/pull_request_template.md (What this PR does, Why it's needed, Reviewer Test Plan with How to verify / Evidence / Tested on, Risk & Scope, Linked Issues, and the Chinese translation block). This is the PR-body/process requirement behind the existing CHANGES_REQUESTED triage review (stage 1a, review 4840811017); the body has not changed since, so the blocker still stands. The code itself is reviewed in the inline findings.
— qwen3.8-max via Qwen Code /review (v0.21.3)
| throughout — read `runtime.memory.children` beside it, which sums the live | ||
| children's own RSS (and says via `sampled` how many actually reported). |
There was a problem hiding this comment.
[Suggestion] The recipe recommends runtime.memory.children without disclosing that child-RSS sampling only runs while an SSE/WS watcher is attached (the sampler gate in run-qwen-serve.ts), and that each reading is dropped ~30s after the last watcher leaves (STALE_CHILD_RESOURCE_MS). The sibling protocol doc discloses both; this recipe cross-references neither. — Failure scenario: an operator follows the recipe's own usage pattern — a one-shot curl against /daemon/status with no streaming client connected — and within ~30s of having no watcher the response reports { rssBytes: 0, sampled: 0 } even with live, growing children: a false negative for exactly the failure mode this recipe exists to catch, with no stated reason or remedy.
| throughout — read `runtime.memory.children` beside it, which sums the live | |
| children's own RSS (and says via `sampled` how many actually reported). | |
| throughout — read `runtime.memory.children` beside it, which sums the live | |
| children's own RSS (and says via `sampled` how many actually reported). Sampling only runs while an SSE/WS client is attached, and readings age out ~30s after the last watcher leaves, so `sampled: 0` means nothing was measured — connect a watcher (e.g. a session SSE stream) and re-poll before trusting a zero sum. |
— qwen3.8-max via Qwen Code /review (v0.21.3)
| for (const managed of workspaceRegistry.listManaged()) { | ||
| void managed.bridge.refreshChildResource?.().catch((err) => { |
There was a problem hiding this comment.
[Suggestion] This sampler fan-out (refresh every managed workspace instead of primary-only) has zero test coverage. Probe-verified this round: reverting to the old primary-only call keeps the whole suite green, while a dead-comparator control proves the harness can discriminate in the one candidate test. run-qwen-serve.test.ts opens no SSE/WS watcher (its own comment says the watch gate never fires) and never references refreshChildResource. — Failure scenario: a future refactor restoring primary-only refresh passes CI silently; on a multi-workspace daemon with a watcher, non-primary children would never be polled, their cached readings would age out of the ~30s window, and children.sampled/rssBytes would shrink to the primary child alone — the precise under-count this change was made to fix.
Suggested fix: drive one sampler tick (fake timers or an injectable interval) with the watcher gate satisfied and two or more managed runtimes whose bridges expose refreshChildResource spies, and assert every managed bridge — not only the primary — is refreshed.
— qwen3.8-max via Qwen Code /review (v0.21.3)
| expect(fresh!.ageMs).toBeGreaterThanOrEqual(0); | ||
| expect(fresh!.ageMs).toBeLessThan(30_000); |
There was a problem hiding this comment.
[Suggestion] The fresh-reading assertions bound ageMs only to [0, 30_000) at reading-time ≈ 0, which holds for any positive window and any in-range computation. Probe-verified this round: both an ageMs: 0 mutant and an inverted-age mutant (STALE_CHILD_RESOURCE_MS - ageMs) in getChildResourceSnapshot keep this suite green, because the staleness-cliff half computes its own comparison and every downstream test injects ages as literals. — Failure scenario: oldestReadingAgeMs then reports a sum whose parts were taken up to 30s apart as instantaneous (constant 0) or reports the newest reading's age as the oldest — exactly the misreading the field's own comment says it exists to prevent.
Suggested fix: advance the clock via the test's existing Date.now seam to a still-fresh instant (e.g. realNow() + 5_000) and assert a bounded range around it (>= 5_000, < 10_000) — pinning direction, magnitude, and reference timestamp at once.
— qwen3.8-max via Qwen Code /review (v0.21.3)
| * This covers the daemon root process only. Aggregate child RSS is a separate | ||
| * measurement — the sampler currently reads the primary ACP child alone — so | ||
| * this figure must not be read as process-tree pressure. |
There was a problem hiding this comment.
[Suggestion] This JSDoc says 'the sampler currently reads the primary ACP child alone' of aggregate child RSS — but this same PR makes runtime.memory.children span every live child (the listManaged() refresh fan-out in run-qwen-serve.ts); only the metrics ring's childRssBytes gauge stays primary-only. The design doc and run-qwen-serve.ts both split the two measurements explicitly, so the clause is false of the aggregate within the PR itself (flagged independently by two review passes). — Failure scenario: a future reader extending pressure to the process tree, or triaging daemon memory from this module's doc, is told the sibling measurement is primary-only and under-attributes child growth, contradicting qwen-serve-protocol.md. The conclusion the sentence supports (pressure is root-only) still holds; the stated reason is wrong.
| * This covers the daemon root process only. Aggregate child RSS is a separate | |
| * measurement — the sampler currently reads the primary ACP child alone — so | |
| * this figure must not be read as process-tree pressure. | |
| * This covers the daemon root process only. Aggregate child RSS is a separate | |
| * measurement (`runtime.memory.children`, which sums every live child's cached | |
| * reading); the metrics ring's `childRssBytes` gauge stays primary-only. Either | |
| * way this figure must not be read as process-tree pressure. |
— qwen3.8-max via Qwen Code /review (v0.21.3)
| list: () => runtimes, | ||
| listManaged: () => runtimes, | ||
| listEntries: () => runtimes.map(() => ({})), |
There was a problem hiding this comment.
[Suggestion] No test pins that the children sum enumerates listManaged() rather than list() — every test that asserts children stubs the two enumerators identically. Probe-verified this round: swapping the sum loop in daemon-status.ts from managedRuntimes to the list() result keeps all 44 tests green. — Failure scenario: the two calls sit a few lines apart in buildDaemonStatusResponse (list() is active-state only, listManaged() includes draining/process-holding runtimes) — an easy swap in a future refactor. A draining workspace's live child would then silently drop out of children.rssBytes/sampled while activeAcpChildren still counts it, under-reporting child RSS in exactly the drain window and violating the sum's documented same-pass promise.
Suggested fix: in the existing 'counts a draining workspace that still holds a live child' test, give the draining bridge a getChildResourceSnapshot returning a reading and assert children includes it — pinning that the sum enumerates the process-holding set, not the active-state set.
— qwen3.8-max via Qwen Code /review (v0.21.3)
| * only reaches direct-embed callers: `runQwenServe` resolves the budget | ||
| * before the bootstrap app exists, so every daemon an operator runs reports | ||
| * it. Hoisting it out would restructure the block for a path that does not |
There was a problem hiding this comment.
[Suggestion] This JSDoc ties pressure's absence to 'no budget resolved' and claims 'every daemon an operator runs reports it' — but the bootstrap /daemon/status handler populates limits.memory from the resolved budget yet omits runtime.memory wholesale, and /daemon/status is a bootstrap route. Traced this round: runQwenServeImpl resolves opts.daemonMemoryBudget before creating the bootstrap app, so during the startup window, and for a daemon whose runtime failed to start (the delegating app falls through to the bootstrap app for the daemon's lifetime), the response carries limits.memory with no runtime.memory.pressure despite a resolved budget. — Failure scenario: a client or alert written to this guarantee (budget resolved ⇒ pressure present) dereferences a missing field exactly in the failed-runtime state where the pressure reading would best explain the failure — the triage case this PR exists for.
| * only reaches direct-embed callers: `runQwenServe` resolves the budget | |
| * before the bootstrap app exists, so every daemon an operator runs reports | |
| * it. Hoisting it out would restructure the block for a path that does not | |
| * only reaches direct-embed callers: `runQwenServe` resolves the budget | |
| * before the bootstrap app exists, so every daemon that reaches the full | |
| * status route reports it. (The bootstrap `/daemon/status` response served | |
| * during startup and after a runtime startup failure omits `runtime.memory` | |
| * wholesale, regardless of the budget.) Hoisting it out would restructure the | |
| * block for a path that does not |
Alternatively, compute pressure in the bootstrap handler too — the root-process reading needs no runtime.
— qwen3.8-max via Qwen Code /review (v0.21.3)
| pressure?: { | ||
| mode: 'off' | 'observe'; | ||
| level: 'normal' | 'soft' | 'hard' | 'critical'; |
There was a problem hiding this comment.
[Suggestion] The new pressure/children mirrors are hand copies of the daemon's wire shape with nothing pinning the two together — no contract test, no fixture, and none of this file's conventional 'Manual mirror of … keep the two field lists in sync.' marker (see DaemonMetricsBucket above). The SDK can't import the source type (it depends on acp-bridge/core, not packages/cli), so the hand copy is structural; the shapes match field-for-field today (verified), so the hazard is prospective. The daemon side is drift-proofed by referencing the computed type — its own comment explains a hand copy would not be caught. — Failure scenario: a field added or renamed in DaemonMemoryPressure compiles cleanly everywhere; the daemon updates via the type reference while this mirror silently keeps the old field list, and SDK consumers type-check against a published shape the wire no longer sends (renamed field reads undefined, typed as present).
Suggested fix: add the file's conventional sync marker to both new blocks naming the source types, and/or an SDK-side wire-contract fixture parsed through DaemonStatusReport.
— qwen3.8-max via Qwen Code /review (v0.21.3)
| expect(pressure.rssRatio).toBeCloseTo( | ||
| pressure.rssBytes / (4_096 * 1024 * 1024), | ||
| 10, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] No test pins the pressure numerator wiring (rssBytes: pressureMemory.rss, heapUsedBytes: pressureMemory.heapUsed at the single call site). Probe-verified this round: transposing those two adjacent values keeps daemon-status.test.ts + daemon-memory-pressure.test.ts + the e2e assertions all green — the arithmetic assertion below is circular (both sides read back the same echoed fields) and the e2e assertions are swap-invariant. — Failure scenario: whenever availableBytes ≠ heapLimitBytes — the common container case, since V8's heap_size_limit (~4.1 GB) exceeds small cgroup limits — the level flips: a 2 GB cgroup daemon with rss 1.9 GB / heapUsed 0.8 GB truly reads max(1.9/2, 0.8/4.1) = 0.95 → critical with an issue in observe mode, but swapped reads max(0.8/2, 1.9/4.1) = 0.46 → normal — a daemon one step from the OOM killer reports healthy. The inverse direction fabricates pressure that isn't there.
| expect(pressure.rssRatio).toBeCloseTo( | |
| pressure.rssBytes / (4_096 * 1024 * 1024), | |
| 10, | |
| ); | |
| expect(pressure.rssRatio).toBeCloseTo( | |
| pressure.rssBytes / (4_096 * 1024 * 1024), | |
| 10, | |
| ); | |
| // V8 used heap is always a proper subset of the process resident set — | |
| // pins the rss/heapUsed wiring at the call site (a transpose of the two | |
| // adjacent values otherwise keeps this whole suite green). | |
| expect(pressure.heapUsedBytes).toBeLessThan(pressure.rssBytes); |
Probe-verified: the added assertion fails against the swap mutant and holds on the real wiring.
— qwen3.8-max via Qwen Code /review (v0.21.3)
| if (read.value !== 'off' && read.value !== 'observe') { | ||
| return { kind: 'fallback' }; | ||
| } | ||
| options.memoryPressureMode = read.value; |
There was a problem hiding this comment.
[Suggestion] The fast-path mapping for --memory-pressure-mode observe has zero assertion anywhere. Probe-verified this round: a constant-'off' mutant on the assignment keeps 119/119 tests green — the dedicated test asserts only the 'off' stored value and the 'enforce' fallback, and the parity-table entry for observe checks only parsed.kind, never the stored value. — Failure scenario: tryRunServeFastPath is the production parse path for plain qwen serve --memory-pressure-mode observe; under the mutant a deployment script explicitly pinning observe silently runs as off — figures still reported but daemon_memory_pressure never raised and the rollup never flipped, diverging from the yargs path, with no red test.
Suggested fix: in parses --memory-pressure-mode and falls back on an unknown value, also assert the stored value for observe in both spellings:
for (const argv of [
['serve', '--memory-pressure-mode', 'observe'],
['serve', '--memory-pressure-mode=observe'],
]) {
expect(parseServeFastPathArgs(argv)).toMatchObject({
kind: 'serve',
options: { memoryPressureMode: 'observe' },
});
}— qwen3.8-max via Qwen Code /review (v0.21.3)
| * measured — either no watcher is gating the sampler open, or the daemon | ||
| * was built without a workspace registry to enumerate. |
There was a problem hiding this comment.
[Suggestion] This JSDoc enumerates exactly two causes for sampled: 0 with live children, phrased as complete — but omits the staleness age-out case, which is the case the staleness window exists for. Traced this round: childResourceAt is stamped only on a successful refreshChildResource poll (the bridge's own catch comment says a stuck child reads 0 rather than a frozen value), and isChannelLive() never checks reading freshness — so with a watcher attached and a registry present, a hung child yields activeAcpChildren ≥ 1 with sampled: 0. — Failure scenario: an operator triaging that state reads this JSDoc, concludes 'no watcher is gating the sampler open' (wrong — their dashboard holds one open) or 'no registry' (wrong), and chases a nonexistent configuration problem instead of seeing the actual signal: the child has been unresponsive for >30s. The omitted third cause is the only one of the three that indicates a real fault.
| * measured — either no watcher is gating the sampler open, or the daemon | |
| * was built without a workspace registry to enumerate. | |
| * measured — either no watcher is gating the sampler open, the daemon | |
| * was built without a workspace registry to enumerate, or every live | |
| * child's last reading aged out of the staleness window (a child | |
| * unresponsive to the resource poll for ~30s). |
— qwen3.8-max via Qwen Code /review (v0.21.3)
|
Worked through the review. Seven suggestions taken, one pushed back on — Taken, and each mutant re-verified locally as failing now:
Separately, the review's trace of the bootstrap route found a JSDoc claim of mine that was just wrong — I wrote that a resolved budget implies Pushed back — the threshold constants duplicating core's Agreed on the duplication, but importing is not available and the extraction is out of scope for this PR. |
|
Body is now on the template: The step worth a reviewer's time is #3: On the red
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (macos-latest / windows-latest, Node 22.x) legs were skipped in CI; platform-specific memory detection paths were exercised only on Linux.
中文说明
已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未审查:build-and-test — Test (macos-latest / windows-latest, Node 22.x) legs were skipped in CI; platform-specific memory detection paths were exercised only on Linux。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| .option('child-heap-mode', { | ||
| choices: ['off', 'observe'] as const, |
There was a problem hiding this comment.
[Suggestion] R3-1: The PR description no longer discloses the diff's full scope: #8508 (per-child heap partition — child-heap-policy.ts, --child-heap-mode, committedProcessCount, reserve-before-decide, ~600 lines of this diff) was merged into this branch (mergeCommit 8e4b6f8) after the body was last updated, but the body still says 'Two commits' and Linked Issues names only #8462. — Failure scenario: issue #8051 lists #8508 as a separate in-flight item, and the chronology confirms it (body update 09:33Z, merge 12:32Z, fix commit e95dda0 at 14:18Z whose message says findings against #8508 'all still live on this branch now that it has merged'). A maintainer merging on the stated two-commit scope lands an unannounced third scope (a new wire field, a new flag, a new public package export). The body already uses the right pattern for #8462 — the same one-liner is owed to #8508.
中文说明
PR 描述未再披露 diff 的完整范围:#8508(每子进程堆分区——child-heap-policy.ts、--child-heap-mode、committedProcessCount、先预留后决策,约 600 行属于本 diff)在正文最后更新之后合入了本分支(mergeCommit 8e4b6f8),但正文仍写「两个 commit」,Linked Issues 也只提到 #8462。失效场景:issue #8051 将 #8508 列为独立的进行中事项,时间线也证实了这一点(正文更新 09:33Z、合入 12:32Z、修复提交 e95dda0 于 14:18Z 且其提交信息称针对 #8508 的发现「在本分支合入后依然存在」)。维护者按所述的两 commit 范围合并时,会落地一个未公告的第三范围(新的 wire 字段、新的 flag、新的包公开导出)。正文对 #8462 已有正确的写法——对 #8508 也欠同样的一句说明。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| | `--max-connections <n>` | `256` | Listener-level TCP connection cap (`server.maxConnections`). Bounds raw socket count irrespective of session count — slow / phantom SSE clients get rejected at accept time once full. Raise alongside `--max-sessions` if your deployment expects many SSE subscribers per session. | | ||
| | `--memory-budget-mb <n>` | 50% of cgroup/host | Total memory budget in MB for the whole daemon process tree. When unset, derived as 50% of the cgroup limit or host memory; either way the effective value is capped at resolved available memory, and both the configured and effective figures are reported. Currently observation only — it does not change how any `qwen --acp` child is sized. Resolved figures appear under `limits.memory` in `GET /daemon/status`, alongside registered and live child counts and advisory per-child shares under `runtime.memory`. A host too small for the minimum reports `insufficientMemory` rather than being clamped upward; because the derived fraction is 50%, any host under ~2 GB trips this. Pass an explicit `--memory-budget-mb 1024` on such a host to override the derived figure (the flag still requires at least 1024 MB of available memory to clear the warning). Must be an integer in `[1024, 1048576]`. | | ||
| | `--memory-pressure-mode <mode>` | `observe` | Whether the daemon turns its own memory reading into a verdict. `observe` (default) reports the pressure level under `runtime.memory.pressure` in `GET /daemon/status` and raises a `daemon_memory_pressure` issue — a `warning`, so the overall `status` leaves `ok` — whenever the level leaves `normal`. `off` still reports every figure, including the level, but raises no issue, so the overall `status` is unchanged; use it while calibrating, or if you alert on the top-level status. The level is the worse of two ratios: RSS against available memory (what the cgroup OOM killer watches) and V8 heap used against this process's heap ceiling. It covers the daemon root process only; compare it against `runtime.memory.children.rssBytes` for the children. Nothing remediates in either mode. One of `off`, `observe`. | | ||
| | `--child-heap-mode <mode>` | `observe` | Whether the daemon models a per-child heap partition of `--memory-budget-mb`. `observe` (default) reports what it would apply — `limits.memory.childHeap.perChildCeilingMb` and `maxConcurrentChildren` — and counts spawns that would have exceeded the limit. **Nothing is applied**: no child is sized from the budget and no spawn is refused. `off` models nothing. A refusal count of 0 does **not** mean the partition would be safe to apply: children still run on the much larger host-derived ceiling, so a workload needing more old space than the modeled ceiling looks perfectly healthy here. Applying the partition ships with the measurement that can answer that. | |
There was a problem hiding this comment.
[Suggestion] R3-2-1: Pattern: 'off models nothing' contradicts the code (this is occurrence 1 of 4). Off mode still computes and reports the full partition: the policy is built whenever a budget resolves regardless of mode (run-qwen-serve.ts:3504-3511), snapshot() has no mode gate, and toDaemonStatusMemoryLimits has none either — GET /daemon/status returns {mode:'off', maxConcurrentChildren, perChildCeilingMb, refusals:0}. What off withholds is only the refusal counting. — Failure scenario: an operator sets --child-heap-mode off expecting limits.memory.childHeap to disappear; it does not, and the reported model is misread as evidence the flag was ignored. Fixing only one surface leaves the others asserting a falsehood (also at 17-configuration.md:26, commands/serve.ts:361 --help text, and serve/types.ts childHeapMode JSDoc).
| | `--child-heap-mode <mode>` | `observe` | Whether the daemon models a per-child heap partition of `--memory-budget-mb`. `observe` (default) reports what it would apply — `limits.memory.childHeap.perChildCeilingMb` and `maxConcurrentChildren` — and counts spawns that would have exceeded the limit. **Nothing is applied**: no child is sized from the budget and no spawn is refused. `off` models nothing. A refusal count of 0 does **not** mean the partition would be safe to apply: children still run on the much larger host-derived ceiling, so a workload needing more old space than the modeled ceiling looks perfectly healthy here. Applying the partition ships with the measurement that can answer that. | | |
| | `--child-heap-mode <mode>` | `observe` | Whether the daemon models a per-child heap partition of `--memory-budget-mb`. Both modes report the modeled partition under `limits.memory.childHeap`; only `observe` counts spawns that would have exceeded the limit. **Nothing is applied**: no child is sized from the budget and no spawn is refused. `off` still reports the modeled figures but counts no refusals. A refusal count of 0 does **not** mean the partition would be safe to apply: children still run on the much larger host-derived ceiling, so a workload needing more old space than the modeled ceiling looks perfectly healthy here. Applying the partition ships with the measurement that can answer that. | |
中文说明
模式问题:「off 什么都不建模」与代码矛盾(此为 4 处中的第 1 处)。off 模式仍会计算并上报完整分区:只要预算解析成功就会构建策略(run-qwen-serve.ts:3504-3511),snapshot() 无模式门控,toDaemonStatusMemoryLimits 也没有——GET /daemon/status 会返回 {mode:'off', maxConcurrentChildren, perChildCeilingMb, refusals:0}。off 唯一不做的是拒绝计数。失效场景:运维设置 --child-heap-mode off 后期望 limits.memory.childHeap 消失,实际并不会,上报的模型会被误读为 flag 未生效。只修一处会让其余各处继续断言假命题(另见 17-configuration.md:26、commands/serve.ts:361 的 --help 文本、serve/types.ts 的 childHeapMode JSDoc)。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| | `--compacted-replay-max-bytes <n>` | positive integer | `4194304` | Byte cap for the bounded in-memory replay snapshot returned by `POST /session/:id/load`; hard cap is `268435456`. | | ||
| | `--memory-budget-mb <n>` | integer in `[1024, 1048576]` | 50% of cgroup-constrained or host memory, capped at the flag maximum (1048576 MB) | Total memory budget for the daemon process tree, capped at resolved available memory. Reported under `limits.memory`, and modeled into a per-child partition. Nothing applies it. Boot rejects out-of-range values. | | ||
| | `--memory-pressure-mode <mode>` | `off` \| `observe` | `observe` | Whether the daemon derives a memory-pressure level from its own RSS and V8 heap. Both modes report `runtime.memory.pressure`; only `observe` raises `daemon_memory_pressure`. Root process only; no remediation. | | ||
| | `--child-heap-mode <mode>` | `off \| observe` | `observe` | Whether the daemon models a per-child heap partition of the budget. `observe` reports it and counts spawns past it; nothing is applied. | |
There was a problem hiding this comment.
[Suggestion] R3-2-2: Pattern: 'off models nothing' contradicts the code (occurrence 2 of 4 — see the qwen-serve.md comment for the trace). This row implies off neither models nor reports ('Whether the daemon models…; observe reports it'), but off mode still computes and reports limits.memory.childHeap; only refusal counting is disabled.
中文说明
模式问题:「off 什么都不建模」与代码矛盾(4 处中的第 2 处——推导过程见 qwen-serve.md 上的评论)。该行暗示 off 既不建模也不上报(「Whether the daemon models…; observe reports it」),但 off 模式仍会计算并上报 limits.memory.childHeap;只有拒绝计数被停用。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| 'apply — `limits.memory.childHeap.perChildCeilingMb` and ' + | ||
| '`maxConcurrentChildren` — and counts spawns that would have ' + | ||
| 'exceeded it. Nothing is applied: no child is sized from the ' + | ||
| 'budget and no spawn is refused. `off` models nothing. Note a ' + |
There was a problem hiding this comment.
[Suggestion] R3-2-3: Pattern: 'off models nothing' contradicts the code (occurrence 3 of 4, the --help text — see the qwen-serve.md comment for the trace). Restate as the sibling memory-pressure-mode help does: off still reports the modeled figures but counts no refusals.
中文说明
模式问题:「off 什么都不建模」与代码矛盾(4 处中的第 3 处,即 --help 文本——推导过程见 qwen-serve.md 上的评论)。请参照同级 memory-pressure-mode 的帮助文本改写:off 仍上报建模数值,只是不做拒绝计数。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| * needs a way to tell an operator in advance whether their workload fits | ||
| * the ceiling, and `refusals` cannot answer that: it counts admission | ||
| * pressure, while children still run on the far larger host-derived | ||
| * ceiling. `off` models nothing. |
There was a problem hiding this comment.
[Suggestion] R3-2-4: Pattern: 'off models nothing' contradicts the code (occurrence 4 of 4 — see the qwen-serve.md comment for the trace). The JSDoc's preceding sentences correctly describe observe-only counting; only this closing clause is false.
中文说明
模式问题:「off 什么都不建模」与代码矛盾(4 处中的第 4 处——推导过程见 qwen-serve.md 上的评论)。该 JSDoc 前几句对 observe 专属计数的描述是正确的,只有结尾这句为假。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // Byte counts are exact multiples of the denominator so the ratio the function | ||
| // computes is exactly the one under test — rounding between the two would put | ||
| // a boundary case on the wrong side of its threshold without failing. |
There was a problem hiding this comment.
[Suggestion] R3-20: Probe-verified: with HARD=0.65 and CRITICAL=0.8 against AVAILABLE = 8 GiB = 2^33, the boundary products are 5583457484.8 and 6871947673.6 — non-integers, so 4 of the 8 it.each rows are NOT 'exact multiples of the denominator'. The true invariant is that AVAILABLE is a power of two, making threshold scaling exact in IEEE 754 (probe: round-trips exact for 2^33, non-exact for a 12 GB denominator). — Failure scenario: a maintainer adding a threshold row or changing AVAILABLE to a realistic non-power-of-two value relies on the stated guard to judge whether boundary rows stay exact; the guard does not hold even for the shipped rows, so the edit silently drops the exactness the comment promises — weakening precisely the mutant-tight >= boundary check — and nothing tells the editor that power-of-two-ness is the property to preserve. Suggested fix: restate the true invariant (power-of-two AVAILABLE makes threshold scaling exact; a non-power-of-two AVAILABLE would let rounding land a boundary case off-threshold).
中文说明
经探针验证:HARD=0.65、CRITICAL=0.8 对 AVAILABLE = 8 GiB = 2^33 的边界乘积为 5583457484.8 与 6871947673.6——非整数,因此 8 个 it.each 行中有 4 行并非「分母的精确倍数」。真正的不变量是 AVAILABLE 为 2 的幂,使阈值缩放在 IEEE 754 下精确(探针:2^33 的往返精确,12 GB 分母则不精确)。失效场景:维护者新增阈值行或把 AVAILABLE 改成现实的非 2 的幂值时,会依赖注释所述的守卫来判断边界行是否仍精确;该守卫对已发布的行都不成立,编辑会悄悄丢掉注释承诺的精确性——恰恰削弱了变异敏感的 >= 边界检查——且没有任何东西提醒编辑者需要保持的是 2 的幂这一性质。建议:改写为真实不变量(2 的幂的 AVAILABLE 使阈值缩放精确;非 2 的幂的 AVAILABLE 会让舍入使边界用例偏离阈值)。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| `level` is `normal` / `soft` / `hard` / `critical`, classified from `ratio` — | ||
| the worse of `rssRatio` (RSS against detected cgroup/host memory, which is what |
There was a problem hiding this comment.
[Suggestion] R3-21: The recipe defines level/source/unknown/modes but never states the mapping (soft >= 0.5, hard >= 0.65, critical >= 0.8), which exists only as constants in daemon-memory-pressure.ts; this same section says 'use off while calibrating thresholds against a real workload' without saying what they are, and no flag changes them. No operator-facing doc publishes the numbers, while this same file does publish other hardcoded thresholds (slow_client_warning 0.75/0.375) — so the omission is not house style. — Failure scenario: an oncall seeing level: 'soft' must judge severity without knowing soft fires at 50% of available memory — a healthy daemon can reach it, per the code's own 'not calibrated for a long-running daemon' comment; assuming soft ~= 75% overreacts, assuming critical ~= 95% under-reacts at 80%. Suggested fix: add one sentence — thresholds are ratio >= 0.5 (soft), >= 0.65 (hard), >= 0.8 (critical), inherited from the interactive CLI's memory-pressure monitor and not yet calibrated for a long-running daemon.
中文说明
配方定义了 level/source/unknown/模式,但从未给出映射(soft >= 0.5、hard >= 0.65、critical >= 0.8)——它们只存在于 daemon-memory-pressure.ts 的常量中;同一节还写着「use off while calibrating thresholds against a real workload」却不说明阈值是多少,也没有任何 flag 可以改它们。所有面向运维的文档都没有公布这些数值,而同一文件公布了其他硬编码阈值(slow_client_warning 0.75/0.375)——因此这个遗漏不是惯例。失效场景:oncall 看到 level: 'soft' 时必须在不知道 soft 在可用内存 50% 就触发的情况下判断严重度——按代码自己「未针对长运行 daemon 校准」的注释,健康 daemon 也可能达到;以为 soft ~= 75% 会过度反应,以为 critical ~= 95% 会在 80% 时反应不足。建议:补一句——阈值为 ratio >= 0.5(soft)、>= 0.65(hard)、>= 0.8(critical),继承自交互式 CLI 的内存压力监控器,尚未针对长运行 daemon 校准。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| memoryPressureMode: argv['memory-pressure-mode'], | ||
| childHeapMode: argv['child-heap-mode'], |
There was a problem hiding this comment.
[Suggestion] R3-22: Probe-verified against the repo's real yargs parser: yargs collects duplicate occurrences of a non-array option into an array and validates each element against choices, so qwen serve --memory-pressure-mode off --memory-pressure-mode observe parses successfully as ['off','observe']. The handler forwards it unnormalized; daemon-status.ts:604 ?? 'observe' does not catch arrays, so pressure.mode serializes outside the declared union and the issue gate (mode === 'observe') is false for any array — no daemon_memory_pressure issue is ever raised, rollup stays ok at critical pressure; even observe observe (deploy template base flags + operator append) suppresses the alert. --child-heap-mode is symmetric. The fast path is last-wins on repeat; the yargs path produces the array — the two parse paths disagree. The pattern pre-exists for --mcp-budget-mode (the new options inherit it), and reaching the yargs path requires a fast-path fallback co-occurring with the duplicate. — Failure scenario: a deployment template that appends a flag the base already sets silently disables the pressure alert for the daemon's whole lifetime, with the wire reporting mode outside its declared union. Suggested fix: normalize or reject in the handler beside the existing validation (Array.isArray(mode) -> stderr + exit 1), or take the last element if last-wins is the intended semantic (matching the fast path).
中文说明
对仓库真实 yargs 解析器的探针验证:yargs 会把非数组选项的重复出现收集为数组,并对每个元素做 choices 校验,因此 qwen serve --memory-pressure-mode off --memory-pressure-mode observe 能成功解析为 ['off','observe']。handler 原样转发;daemon-status.ts:604 的 ?? 'observe' 接不住数组,于是 pressure.mode 序列化出声明的联合类型之外,且 issue 门控(mode === 'observe')对任何数组都为假——daemon_memory_pressure 永不产生,临界压力下 rollup 仍是 ok;即便 observe observe(部署模板基础 flags + 运维追加)也会压制告警。--child-heap-mode 对称。fast path 对重复取值是后者生效;yargs 路径产生数组——两条解析路径不一致。该模式在 --mcp-budget-mode 上已存在(新选项继承了它),且需要 fast-path 回退与重复取值同时发生才会走到 yargs 路径。失效场景:部署模板追加了基础已设置的 flag 时,会在 daemon 整个生命周期内悄悄禁用压力告警,且 wire 上报的 mode 超出声明的联合类型。建议:在 handler 现有校验旁归一化或拒绝(Array.isArray(mode) -> stderr + exit 1),或以最后一个元素为准(若后者生效是预期语义,与 fast path 对齐)。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // Real process figures; asserted for shape here and for arithmetic in | ||
| // the dedicated pressure tests. `toEqual` still fails on an extra key. | ||
| pressure: expect.objectContaining({ mode: 'observe' }), |
There was a problem hiding this comment.
[Suggestion] R3-23: Probe of vitest matcher semantics: toEqual with pressure: expect.objectContaining({ mode: 'observe' }) passes an extra OR missing key nested inside pressure (strictness applies only to runtime.memory's own keys), so this comment ('asserted for shape here ... toEqual still fails on an extra key') is wrong about the assertion it documents. Full-file grep confirms the pinned pressure fields are exactly level/mode/availableBytes/rssRatio; source, ratio, rssBytes, heapUsedBytes, heapRatio, heapLimitBytes have no value assertion anywhere in this file (the key-set test pins names, not wiring) — broader than ledger R2-13's numerator-wiring claim. — Failure scenario: a change that drops, renames, or transposes any of the six unpinned fields ships green, surfacing only as broken SDK consumers; a maintainer trusting this comment believes shape regressions already fail here. Suggested fix: expand the objectContaining to all ten fields with expect.any matchers (or thread a deterministic pressure input and assert exact values), or correct the comment.
中文说明
对 vitest 匹配器语义的探针:pressure: expect.objectContaining({ mode: 'observe' }) 下的 toEqual 对 pressure 内部多出或缺失的嵌套键都会通过(严格性只作用于 runtime.memory 自身的键),因此该注释(「asserted for shape here ... toEqual still fails on an extra key」)对它所描述的断言是错的。全文件 grep 确认被钉住的 pressure 字段恰好是 level/mode/availableBytes/rssRatio;source、ratio、rssBytes、heapUsedBytes、heapRatio、heapLimitBytes 在本文件没有任何值断言(键集测试钉的是名字,不是接线)——范围比台账 R2-13 的 numerator 接线主张更宽。失效场景:删除、重命名或换序六个未钉字段中的任何一个都会绿色发布,只在 SDK 消费者坏掉时暴露;信任该注释的维护者会以为形状回归在这里已经会失败。建议:把 objectContaining 扩到全部十个字段并用 expect.any 匹配(或引入确定性 pressure 输入并断言精确值),或更正注释。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // Every state a live child can be in, in one response. `sampled` is what | ||
| // separates "measured and small" from "never measured": without it, the | ||
| // three unreported children below are indistinguishable from children | ||
| // using no memory. |
There was a problem hiding this comment.
[Suggestion] R3-24: Mutant run: the suite's docblock claims 'every state a live child can be in', but no contributor has rssBytes: 0 — which the real bridge produces: refreshChildResource stamps childResourceAt even when rssBytes fails validation, and getChildResourceSnapshot returns { rssBytes: info.childRssBytes ?? 0, ageMs } — a fresh, measured-but-zero reading. The truthiness-guard mutant if (!snapshot?.rssBytes) continue passes all 48 tests while a probe with a measured-zero contributor flips (sampled 0 -> 1) — the exact defect class the adjacent ageMs test explicitly warns about. — Failure scenario: a truthiness-guard regression drops measured-zero children from sampled while the suite stays green; clients using the sampled-vs-activeAcpChildren gap signal see a gap that does not exist, and the field's documented 'how many contributed' meaning ships false. Suggested fix: add one zero-rss contributor — e.g. a sixth runtime liveWith(0, 3_000), bumping expectations to rssBytes: 300, sampled: 3, oldestReadingAgeMs: 9_000.
中文说明
变异运行:套件 docblock 声称覆盖「live child 的所有状态」,但没有任何贡献者的 rssBytes 为 0——而真实 bridge 会产生该值:即便 rssBytes 校验失败,refreshChildResource 也会盖 childResourceAt 时间戳,getChildResourceSnapshot 返回 { rssBytes: info.childRssBytes ?? 0, ageMs }——一个新鲜、测得为零的读数。truthiness 守卫变异 if (!snapshot?.rssBytes) continue 能通过全部 48 个测试,而带测得为零贡献者的探针会翻转(sampled 0 -> 1)——正是相邻 ageMs 测试明确警告的缺陷类别。失效场景:truthiness 守卫回归会把测得为零的子进程从 sampled 中丢掉而套件仍绿;使用 sampled 与 activeAcpChildren 差值信号的客户会看到不存在的缺口,字段文档化的「多少个子进程贡献了读数」含义带着错误发布。建议:补一个零 rss 贡献者——例如第六个 runtime liveWith(0, 3_000),期望值改为 rssBytes: 300、sampled: 3、oldestReadingAgeMs: 9_000。
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
Body updated again — #8508 merged into this branch after my last comment, so the description now covers what the branch actually carries. It is three parts rather than two: Part 1 the daemon root's pressure reading, Part 2 aggregate child RSS, Part 3 the per-child heap partition model from #8508. The old Two things a reviewer should know about Part 3, both stated in the body:
Also pushed The red @qwen-code /triage |
e95dda0 to
dc12afa
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。 |
ReviewThree observation-only parts on one branch: root memory pressure ( Three things worth changing, one of them a real defect. 1.
|
yiliang114
left a comment
There was a problem hiding this comment.
LGTM, no blockers. Verified observation-only as intended: decide() return discarded at spawn, argv byte-identical, no spawn refused, no GC/kill added. Multi-workspace accounting holds by construction (same listManaged + isChannelLive gate), denominators handle zero/NaN/Inf, pinned partition math recomputed correct, no security issues (additive fields on auth-gated /daemon/status). One P2 to sign off: default 'observe' flips the top-level /daemon/status rollup ok->warning on uncalibrated thresholds (borrowed from interactive CLI) — a day-one behavior change for anyone alerting on status; consider defaulting 'off' until this phase's data calibrates thresholds, or call out in release notes. Rest is documented intentional phase-1 scoping.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 121 passed · 0 failed · 121 total 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:121 通过 · 0 失败 · 121 总计 Verification reportPR 8423 Deep Verification —
|
| cell | environment | oracle | result |
|---|---|---|---|
| head-default | head build, default flags | 27 assertions: boot + runtime ready; 10 pressure fields exactly; ratio == max(rssRatio, heapRatio); source matches winner (heap won live: 0.0201 vs 0.00075); availableBytes == availableMemoryMb × 2²⁰ (253143 MB → 265439674368 B, exact on the wire); thresholds classify; childRssCoverage: active_children; idle children {0, 0, null}; sampled ≤ activeAcpChildren; childHeap {observe, 25, 5021, 0} with 25×5021=125525 ≤ pool 125547; enforced === false; no issue while normal |
27/27 |
| base-default | base build, default flags | 8 assertions: boot + runtime ready; no pressure, no children, childRssCoverage: primary_only, no childHeap key, enforced: false, head = base + exactly {pressure, children} |
8/8 |
| head-both-off | head build, --memory-pressure-mode off --child-heap-mode off |
11 assertions: pressure figures fully populated with mode: off (identical field set), no daemon_memory_pressure issue, partition nulls (maxConcurrentChildren: null, perChildCeilingMb: null) distinct from zero-pool 0, refusals: 0, enforced: false |
11/11 |
| head-enforce-* | head build, --child-heap-mode enforce / --memory-pressure-mode enforce |
daemon refuses to boot; stderr carries yargs Invalid values … Choices: "off", "observe" for both flags |
2/2 |
Session-level secondary cells (witness: 02-session-probe-observe.png, 03-session-probe-off.png): a real POST /session spawns a real qwen --acp child against a loopback fake-OpenAI peer; GET /session/:id/events held open as the SSE watch gate:
| probe | argv oracle | sampling oracle | result |
|---|---|---|---|
| observe | --max-old-space-size=16384 (host-derived), not perChildCeilingMb 5021 |
with watcher live: children {rssBytes: 215826432, sampled: 1, oldestReadingAgeMs: 934}, activeAcpChildren: 1, sampled ≤ active, partition still published, refusals: 0 |
15/15 |
| off | argv byte-identical: --max-old-space-size=16384 |
sampling still live (rssBytes: 223891456, sampled: 1), partition suppressed to nulls |
14/14 |
The pure-function matrix (witness: 04-module-matrix.png, 31/31 against real dist code) drove the boundaries the live daemon cannot reach: threshold edges 0.4999/0.5/0.6499/0.65/0.7999/0.8; source selection incl. exact tie → rss; availableBytes=0 → heap-only; both zero → unknown/normal/0 never critical; NaN/−5/Infinity sanitize to 0; PR-pinned partitions 3687→7×526 and 15360→25×614 (the 25 proven to be the MAX_DAEMON_WORKSPACES cap biting over an uncapped 30); zero-pool 256→{0, null} (the fixed defect — never a 0 ceiling); 1 TB pool ceiling capped at legacy 16384; invariant sweep over 10 pool sizes; decide() refusal counting; off mode inert.
Verified along the way (not findings)
- Bootstrap window is real and matches the code comment. The first status fetch after
listeningserves the bootstrap app:limits.memorypopulated withchildHeap: null,runtime.memoryabsent,runtime.loading: true— observed on every boot, both arms. The PR's own comment warns against "budget resolved implies pressure present"; the warning is accurate. source: heapon this host is the worse-of-two logic working as advertised (V8 limit 4.05 GB ≪ 247 GB host), withrssdirection covered at module level.
Corrections
Two descriptions in the PR body understate the measured mutation counts — the direction is favorable (more coverage than claimed), but the record should match the measurement:
- "
Math.max→Math.minon the pressure ratio fails 7" — measured 9 across the two affected suites (7 indaemon-memory-pressure.test.ts, 2 indaemon-status.test.ts). The claimed 7 matches the pressure suite alone. - "dropping the MB→bytes conversion fails 2 tests" — measured 3 in
daemon-status.test.ts, one of them named exactlyconverts the budget from megabytes when computing the ratio(failsexpected 4096 to be 4294967296, the 2²⁰ factor).
These are corrections to the description, not requests to change code.
Findings
None blocking. Every executed assertion passed; no behavioral mismatch was produced by any cell, probe, or mutant.
Not covered
- Windows / macOS — this round ran on the lane's
node:22-bookwormcontainer (Linux). The PR's stated platform-sensitive decision (child self-reported RSS instead of/proc) was not exercised on the other platforms; they ride on the repo's CI matrix. refusalsunder real admission pressure — counting >maxConcurrentChildrenlive children would need 26 concurrent sessions; covered instead by module matrixdecide()cells and the unit suite's wiring tests. This reproduces the refusal logic, not the admission load that would feed it.- WS watch path — the sampler gate is
sseCount > 0 || wsStreams > 0; only the SSE half was driven live. oldestReadingAgeMswith pre-field contributors — the "absent on older bridges" branch is unit-covered; a genuinely older bridge was not constructed live.- Per-commit attribution — the checkout is depth 2 (
git rev-list HEAD^1..HEAD^2yields only the head OID; the metadata lists 6 commits). The aggregateHEAD^1..HEADdiff is what was verified; the 6 commit messages were treated as untrusted description. - SDK type mirror (
packages/sdk-typescript/src/daemon/types.ts) — type-only changes, verified by reading and by the workflow's successful full build, not by a separate SDK compile gate or a mixed-version daemon/client pairing. - Docs — six documentation files changed; content not audited beyond the
childRssCoverage/childHeapclaims that the wire oracle settled independently. - Repo-wide suite / lint / typecheck — targeted gates only (affected suites of
packages/cli+packages/acp-bridge). The workflow's pre-runnpm run buildat head is the compile evidence. - Base-arm
npm ciwas not needed: unchanged workspaces reuse the head tree's already-built dist (diff-verified untouched); the three changed workspaces were rebuilt from base source. Stated per the environment contract so the control's purity is auditable.
Methodology
Environment: GitHub-runner container (the verify lane itself), Node v22.23.2, ~247 GB host memory (availableMemoryMb 253143, cgroup unconstrained → availableMemorySource: host), V8 heap_size_limit 4345298944 B. Base control: git worktree tmp/base-tree HEAD^1 with a symlink-farm node_modules re-pointing @qwen-code/{acp-bridge,qwen-code,sdk} into the base tree (realpath-asserted) and nested per-package node_modules mirrored; acp-bridge + sdk + cli rebuilt there (core etc. copied from head, identical per diff), then removed after the A/B cells were captured. Harnesses (all mock-free, in harness/): boot-status.mjs boots a real daemon and captures /daemon/status over real HTTP; wire-ab.mjs orchestrates the five cells above; session-probe.mjs adds a loopback fake-OpenAI peer, a real POST /session child spawn, argv extraction by PPID, and a held SSE stream; module-matrix.mjs drives the compiled modules directly; mutation-summary.mjs re-asserts each mutant's kill from the recorded logs plus the unmutated positive controls. Five mutants were applied to head source, their suites run red on the intended behavioral assertions (quoted in logs mut1…mut5), and every file restored (git status clean afterward). Session probes were run twice (once under the image capture, once for text logs); absolute byte readings vary between runs (~180–224 MB here) while every assertion held on both runs. Raw per-cell payloads: logs/cells/*.json; gate logs: logs/gates-*.log; A/B log: logs/wire-ab.log.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Gate re-run on request — the head is unchanged since the last pass (
Moving on to code review. 🔍 中文说明应请求重跑门禁——head 与上次通过时相同(
进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRe-run on request at an unchanged head — no code delta since the last review, so this pass re-verified every prior finding in the tree rather than trusting the record. All confirmed. The observation-only invariants, re-checked directly: The three earlier findings remain fixed or resolved as last described: sub-floor ceiling (refused, not shrunk), non-finite numerator ( Smaller items, carried, none blocking: the Test evidenceUnattended CI run: I have not built or executed this PR's code — the evidence below is the PR's own CI on the reviewed commit, read through the API, plus the two signals named after the table. No terminal (tmux) capture on this path; live behaviour is exercised only by the isolated jobs named below. All four
Two further signals, both stronger than CI for this PR:
中文说明代码审查应请求在不变的 head 上重跑——自上次评审以来无代码增量,因此本次直接在树中复核了此前每一项发现,而非依赖记录。全部确认无误。 仅观测不变量,逐一直接复查: 此前的三项发现维持上次结论:低于下限的上限(拒绝而非压缩)、非有限分子( 较小的遗留项,继续携带,均不阻塞: 测试证据无人值守 CI 运行:我没有构建或执行本 PR 的代码——下面的证据是通过 API 读取的本 PR 自身在受审提交上的 CI,以及表格之后列出的两项信号。此路径无终端(tmux)捕获;真实行为仅由下面点名的隔离任务执行。 本 head 上四条 另两项信号,对本 PR 而言都比 CI 更强:
— Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — still clean at this head: every prior finding remains fixed or resolved-by-documentation, all four PR-CI workflows are green with nothing pending, and a maintainer has approved this exact commit after verifying it against real daemons — but the cross-package core-size escalation caps the bot at 3/5, so this stays a maintainer merge call by policy, not by doubt. What this pass adds, on an unchanged head (
Why the bot still does not approve: this is a cross-package core change at 907 production lines, which Stage 0 escalates for maintainer awareness and caps at 3/5 regardless of review outcome — the approval stays a human's to give. The housekeeping note from the last pass also stands, re-checked this time against the review body itself: the ⏸️ Deferring to @wenshao — everything the gate can check is checked and green at this head, including your own live verification and approvals. Once the in-flight sandboxed verify lands beside the green CI, the one remaining step is dismissing the stale review state, and this is ready to merge. 中文说明置信度:3/5 —— 本 head 依然是干净的:此前所有发现或已修复、或以文档化解决,四条 PR CI 工作流全绿且无待决项,且一位维护者在真实 daemon 上验证后已批准了这个提交——但跨包核心模块的规模升级使机器人的置信度上限为 3/5,因此这仍是维护者的合并决策,是政策使然,而非心存疑虑。 在不变的 head(
机器人仍不批准的原因:这是一次 907 行生产代码的跨包核心改动,Stage 0 将其升级为维护者关注,且无论审查结果如何置信度上限都是 3/5——批准由人来给。上次的事务性提醒依然成立,且本次已对照评审正文复核:本 PR 当前显示的 ⏸️ 转交 @wenshao —— 门禁能查的一切在本 head 上均已查毕且为绿,包括你自己的实测与批准。待进行中的沙箱验证与绿色 CI 并立落地,剩下的唯一步骤就是驳回陈旧的评审状态,届时即可合入。 — Qwen Code · qwen3.8-max Reviewed at |
Stage-1a template gate is now satisfied: the PR body carries every required heading (What this PR does / Why it's needed / Reviewer Test Plan with How to verify, Evidence, Tested-on / Risk & Scope / Linked Issues) plus the Chinese translation. Dismissing this run's own stale gate review so the outstanding CHANGES_REQUESTED state reflects code findings only (see the stage-3 triage comment).
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (macos-latest / windows-latest, Node 22.x) legs were skipped in CI; platform-specific memory detection paths were exercised only on Linux.
中文说明
已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未审查:build-and-test — Test (macos-latest / windows-latest, Node 22.x) legs were skipped in CI; platform-specific memory detection paths were exercised only on Linux。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| import { | ||
| createChildHeapPolicy, | ||
| type ChildHeapPolicy, | ||
| } from '@qwen-code/acp-bridge/childHeapPolicy'; |
There was a problem hiding this comment.
[Suggestion] R5-1: The new @qwen-code/acp-bridge/childHeapPolicy subpath import (also used by daemon-status.test.ts) has no source alias in packages/cli/vitest.config.ts, unlike the sibling acp-bridge subpaths the config aliases to .ts source. npm ci runs prepare → build, so nothing fails in CI today — but the config's own comment says the aliases exist because dist may be stale or absent during development. — Failure scenario: with a stale packages/acp-bridge/dist/ present, this import executes the compiled pre-edit copy while aliased subpaths read live source, so one suite run tests two different module copies (probe-verified: with dist removed the import fails collection; with the alias added it resolves to source).
// packages/cli/vitest.config.ts — add beside the sibling aliases:
'@qwen-code/acp-bridge/childHeapPolicy': path.resolve(
__dirname,
'../acp-bridge/src/child-heap-policy.ts',
),中文说明
新的 @qwen-code/acp-bridge/childHeapPolicy 子路径导入(daemon-status.test.ts 中同样使用)在 packages/cli/vitest.config.ts 中没有源码别名,而该配置为其他 acp-bridge 子路径都配置了指向 .ts 源码的别名。npm ci 会执行 prepare → 构建,因此 CI 目前不会失败——但配置自身的注释说明这些别名存在的原因是「dist 在开发期间可能过期或缺失」。失效场景:当存在过期的 packages/acp-bridge/dist/ 时,该导入会执行编译出的旧副本,而配置了别名的子路径读取的是实时源码,导致同一次测试运行针对两份不同的模块副本(已用探针验证:移除 dist 后该导入收集失败;添加别名后解析到源码)。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| `limits.memory` is additive and reports the daemon's resolved memory figures: a required `enforced: false`, `configuredBudgetMb`, `effectiveBudgetMb` (the configured value capped at resolved cgroup/host memory), `budgetSource` (`flag` / `derived`), `availableMemoryMb`, `availableMemorySource` (`constrained` / `host`), `insufficientMemory`, and a `modeled` object holding `rootReserveMb`, `childPoolMb`, `minChildHeapMb`, `maxChildHeapMb`, and `legacyChildCeilingMb` (a conservative model of the ceiling an ACP child receives today, which can sit below the real figure). `runtime.memory` additionally reports `registeredWorkspaces` (the registration count — non-removed workspace entries, including draining, transitioning, or blocked ones; not a live-child count), `activeAcpChildren` (daemon-managed ACP children with a live, non-dying channel — includes transitioning or blocked entries, but excludes a workspace whose kill has started even if the child has not exited; not channel workers, MCP descendants, or unattached spawn reservations), `childRssCoverage` (`primary_only` today), and a `modeled` object holding `recommendedShareAtRegisteredMb` (`null` when no workspace is registered) and `recommendedShareAtActiveMb` (`null` when no child is active). Each share is capped at the legacy child ceiling, and floored at the minimum child heap only when the ceiling allows — on a small host the ceiling sits below the floor, so share × count can exceed the child pool. Read a share as advisory, not a partition of the pool. All of it is observation: no child spawn argument derives from these values, and no request is refused on their basis. On the normal `runQwenServe` path the budget is resolved before the bootstrap app is created, so `limits.memory` is already populated during the bootstrap window. It is `null` only on paths that resolve no budget (such as direct-embed bypassing `runQwenServeImpl`). The SDK type allows `null`, so correct clients cope. | ||
| `limits.memory` is additive and reports the daemon's resolved memory figures: a required `enforced: false`, a `childHeap` object (`mode`; `maxConcurrentChildren` and `perChildCeilingMb`, both `null` under `mode: 'off'`, which models nothing — and `perChildCeilingMb` additionally `null` when no child is admissible, never 0, while `maxConcurrentChildren` is `0` in that case, since a pool too small to host one child is a computed answer rather than an absent model; and `refusals`, the spawns that would have exceeded the modeled limit), `configuredBudgetMb`, `effectiveBudgetMb` (the configured value capped at resolved cgroup/host memory), `budgetSource` (`flag` / `derived`), `availableMemoryMb`, `availableMemorySource` (`constrained` / `host`), `insufficientMemory`, and a `modeled` object holding `rootReserveMb`, `childPoolMb`, `minChildHeapMb`, `maxChildHeapMb`, and `legacyChildCeilingMb` (a conservative model of the ceiling an ACP child receives today, which can sit below the real figure). `runtime.memory` additionally reports `registeredWorkspaces` (the registration count — non-removed workspace entries, including draining, transitioning, or blocked ones; not a live-child count), `activeAcpChildren` (daemon-managed ACP children with a live, non-dying channel — includes transitioning or blocked entries, but excludes a workspace whose kill has started even if the child has not exited; not channel workers, MCP descendants, or unattached spawn reservations), `childRssCoverage` (`active_children` — every ACP child with a live channel, which is the set `activeAcpChildren` counts; older daemons send `primary_only`), a `children` object described below, and a `modeled` object holding `recommendedShareAtRegisteredMb` (`null` when no workspace is registered) and `recommendedShareAtActiveMb` (`null` when no child is active). Each share is capped at the legacy child ceiling, and floored at the minimum child heap only when the ceiling allows — on a small host the ceiling sits below the floor, so share × count can exceed the child pool. Read a share as advisory, not a partition of the pool. All of it is observation: no child spawn argument derives from these values, and no request is refused on their basis. `childHeap` models a fixed partition of `modeled.childPoolMb` — every child would receive the same `perChildCeilingMb`, so the modeled total stays inside the pool rather than accumulating as a per-spawn share would. Read `refusals` as admission pressure only: a count of 0 does **not** mean the partition is safe to apply, because children run on the much larger host-derived ceiling, so a workload needing more old space than `perChildCeilingMb` is healthy here and would only fail once the partition were applied. On the normal `runQwenServe` path the budget is resolved before the bootstrap app is created, so `limits.memory` is already populated during the bootstrap window. It is `null` only on paths that resolve no budget (such as direct-embed bypassing `runQwenServeImpl`). The SDK type allows `null`, so correct clients cope. | ||
|
|
||
| `runtime.memory.children` is additive within that block and reports aggregate RSS across the children `childRssCoverage` names: `rssBytes` (their summed self-reported RSS), `sampled` (how many produced a reading), and `oldestReadingAgeMs` (the age of the oldest reading in the sum, so a caller can tell how far apart its parts were taken). The denominator for `sampled` is the sibling `activeAcpChildren`, not repeated inside the block; when `sampled` is lower, `rssBytes` is a floor rather than a total. Sampling is gated on an active SSE/WS watcher, so a status request against a daemon nobody is streaming from reports `sampled: 0` even with live children — `activeAcpChildren` beside it makes that gap visible, and `rssBytes: 0` with `sampled: 0` never means a measured zero. `oldestReadingAgeMs` is `null` when nothing was sampled and also when every contributor is a bridge predating the field, so it never means "fresh". Read the sum as an over-count and an under-count at once: summing per-process RSS double-counts pages the children share, while each child reports only its own process, so its MCP descendants and every channel worker are missing. It is not the daemon tree's memory. The field is optional in the SDK mirror because daemons reporting `primary_only` never send it. |
There was a problem hiding this comment.
[Suggestion] R5-2: This paragraph's unconditional claim — "a status request against a daemon nobody is streaming from reports sampled: 0 even with live children" — ignores the staleness window: refresh stops when the last SSE/WS watcher detaches, but each cached reading survives until it ages out (STALE_CHILD_RESOURCE_MS = 30_000, bridge.ts). — Failure scenario: a client polling /daemon/status within ~30 s of the last watcher disconnecting observes sampled > 0 with zero active watchers, contradicting this sentence; a monitor asserting the documented invariant fails, and an operator reads a stale sum as live coverage. The childRssCoverage JSDoc and the SDK mirror in this same PR ("each reading persists until it ages out (~30s)") contradict it too. Suggested rewording: "…so once the last watcher's cached readings age out (~30 s), a status request against a daemon nobody is streaming from reports sampled: 0 even with live children…"
中文说明
本段无条件表述——「对没有人在流式传输的 daemon 发起 status 请求,即使有活跃子进程也会报告 sampled: 0」——忽略了陈旧窗口:当最后一个 SSE/WS 观察者断开时刷新即停止,但每个缓存读数会存活到过期(STALE_CHILD_RESOURCE_MS = 30_000,bridge.ts)。失效场景:客户端在最后一个观察者断开后约 30 秒内轮询 /daemon/status,会在没有任何活跃观察者时观察到 sampled > 0,与本句矛盾;对该文档化不变量做断言的监控会失败,运维也会把过期的求和误读为实时覆盖。同一 PR 中 childRssCoverage 的 JSDoc 与 SDK 镜像(「每个读数会保留到过期(约 30 秒)」)也与之矛盾。建议改为:「……因此在最后一个观察者的缓存读数过期(约 30 秒)后,对没有人在流式传输的 daemon 发起 status 请求,即使有活跃子进程也会报告 sampled: 0……」
— qwen3.8-max via Qwen Code /review (v0.21.6)
| expect(memory?.childHeap).toEqual({ | ||
| mode: 'observe', | ||
| maxConcurrentChildren: expect.any(Number), | ||
| perChildCeilingMb: expect.any(Number), | ||
| refusals: 0, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R5-3: Probe-verified: this boot test's assertion is vacuous with respect to the policy→factory wiring — removing childHeapPolicy from all three createSpawnChannelFactory call sites (run-qwen-serve.ts:3606/4223/4747) keeps every suite green. The snapshot this assertion reads (managedChildHeapPolicy) exists independently of the factories, the boot uses maxSessions: 1 with a cap ≥ 1, and refusals is 0 with or without the wiring. — Failure scenario: a refactor that drops the wiring ships green, and a production daemon publishes the modeled partition with a refusal counter that can never move — silently zeroing the figure the docs call this mode's product. This is distinct from, and stronger than, the open R3-5 comment, whose premise (that the primary path is exercised by a test that could observe refusals moving) this probe measured false. Suggested fix: boot once more with a budget that models exactly one child (memoryBudgetMb: 1024 → pool 768), create two sessions so two children spawn, and assert childHeap.refusals >= 1 while both sessions still work (observe mode refuses nothing).
中文说明
经探针验证:该启动测试的断言对「策略→工厂」的接线是空洞的——把 childHeapPolicy 从全部三处 createSpawnChannelFactory 调用点(run-qwen-serve.ts:3606/4223/4747)移除后,所有套件依然绿色。该断言读取的快照(managedChildHeapPolicy)独立于工厂而存在;启动使用 maxSessions: 1 且上限 ≥ 1,无论接线是否存在 refusals 都是 0。失效场景:某次重构删除接线后会绿色发布,生产 daemon 将发布那份建模分区,但其拒绝计数永远不会移动——把文档称为该模式全部产出的数字悄悄归零。这与未解决的 R3-5 评论不同且更强:R3-5 的前提(主路径被某个能观察到 refusals 变化的测试覆盖)被本探针证伪。建议修复:再以「恰好建模一个子进程」的预算启动一次(memoryBudgetMb: 1024 → 池 768),创建两个会话使两个子进程派生,并在两个会话仍正常工作时断言 childHeap.refusals >= 1(observe 模式不会真的拒绝任何东西)。
— qwen3.8-max via Qwen Code /review (v0.21.6)
…imum
`perChildCeilingMb` is `min(floor(pool / maxConcurrentChildren),
legacyChildCeilingMb)`. The first term is at least `MIN_CHILD_HEAP_MB` by
construction; the second is `floor(available / 2)` and is not, so the
`Math.min` could publish a ceiling *below* the `minChildHeapMb` sitting beside
it in the same snapshot:
avail=768 --memory-budget-mb 1024 pool=512 legacyCeil=384 perChild=384
avail=1023 --memory-budget-mb 1024 pool=767 legacyCeil=511 perChild=511
Unreachable from a derived budget — the pool reaches 0 first — but an explicit
budget has a floor of 1024 while available memory does not, and
`docs/users/qwen-serve.md` tells operators on exactly these hosts to pass that
flag. The documented remedy is what reaches the band.
Refuse the model rather than shrink under the floor, with
`maxConcurrentChildren` zeroed in lockstep: a ceiling no child may run at is
not a partition, and "one child fits" beside a null ceiling is the same
contradiction from the other side. Nothing is applied today so the impact was a
wrong published figure, but this is the number the partition asks to be judged
by and the one an `enforce` mode would hand to `--max-old-space-size`.
The existing matrix resolves derived budgets only, which is why the mutation
sweep came back clean; add the `budgetMb` axis, asserting in each case the
shape that makes it reachable, and pin the inclusive boundary (1024/1024 ->
one child at 512) so nulling unconditionally cannot pass instead.
Also, in the same review pass:
- Split usable-gauge handling into numerator and denominator. Coercing an
unusable numerator to 0 published `rssBytes: 0, rssRatio: 0, level: 'normal',
source: 'rss'` — a daemon that measured nothing, indistinguishable from an
idle one, which is the confusion `source: 'unknown'` and `sampled: 0` exist
to prevent everywhere else here. An unusable numerator now retires its own
side. Zero stays a reading for a numerator and not for a denominator.
- Document that `rssRatio` divides by host total under
`availableMemorySource: 'host'`, so it is a lower bound on real pressure
there — a denominator problem no threshold calibration addresses.
- Document that `refusals` counts channel swaps at full occupancy (the
terminating child is counted until it exits) and equals the total spawn count
on a host too small to model a partition. Deliberately not fixed by giving
the comparison swap headroom, which would admit a 26th ceiling against a
25-child pool.
- Keep the sampler's rejection handler as a documented backstop — the shipped
`refreshChildResource` never rejects, but it is an optional `async` interface
member, so a foreign implementation throwing early would otherwise surface as
an unhandled rejection — and give it the workspace so it is attributable
across the fan-out.
- Test hygiene: drop a duplicated `enforced` assertion; replace a host-
dependent `expect.any(Number)` with a key-set pin plus a branch, since a
small host now legitimately reports no partition; use `vi.spyOn(Date, 'now')`
over direct assignment; reuse the exported `ChildHeapMode` on the child-heap
side, leaving the independent `memoryPressureMode` switch alone.
Reported by @wenshao.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…o agent/daemon-memory-observe
…o agent/daemon-memory-observe
|
Thanks — both passes worked through. Answering them together since they overlap. Nine items taken, one pushed back on, one deferred with a reason. Every fix below is mutation-verified: the new assertion fails against the pre-fix code, and where the fix could be satisfied by over-correcting, a paired assertion fails against that too. The defect —
|
| Note | Status |
|---|---|
Duplicate expect(memory?.enforced).toBe(false) |
Removed |
perChildCeilingMb: expect.any(Number) is host-dependent |
Fixed — and it was worse than noted, since the fix above adds a second way a real host reaches null. Now pins the key set (so an unannounced wire field still fails) and branches on which case the host took |
bridge.test.ts assigns Date.now directly |
Now vi.spyOn(Date, 'now') + mockRestore() |
'off' | 'observe' hand-written in six places |
Child-heap side now imports ChildHeapMode (3 sites). import type, so it is erased before the fast-path bundle closure check sees it. Left memoryPressureMode alone deliberately: same two values today, but an independent switch, and aliasing them couples whichever gains enforce first to the other. SDK mirror stays standalone as you noted |
| Markdown table renders two ways | Normalized both files to `off` | `observe` |
Release-note line for childRssCoverage |
Already covered — CHANGELOG.md is generated from GitHub Releases and marked do-not-edit, so the PR body's Risk & Scope is where this lives, and it already states that clients pinning childRssCoverage === 'primary_only' will see 'active_children'. The protocol doc carries older daemons send primary_only beside it |
Not taken: the workspaceRegistry-absent-with-live-workspaceSnapshots case. You are right it is the one shape where sampled: 0 is structural rather than watcher-gated, but the invariant holds and the field docs already say rssBytes: 0 with sampled: 0 never means a measured zero — so a test would pin the current wiring rather than a contract. Happy to add it if you would rather have it pinned.
Gate: tsc clean on acp-bridge, cli, and sdk-typescript; the child-heap, spawn-channel, pressure, daemon-status, serve-command, and bridge suites pass; prettier clean.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not reviewed: build-and-test — Test (macos-latest / windows-latest, Node 22.x) legs were skipped in CI; platform-specific memory detection paths were exercised only on Linux. Not reviewed: reverse audit — stopped before round 5 by the review time budget.
中文说明
已审查。 建议见行内评论。 未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未审查:build-and-test — Test (macos-latest / windows-latest, Node 22.x) legs were skipped in CI; platform-specific memory detection paths were exercised only on Linux。 未审查:反向审计——评审时间预算不足,未能开始第 5 轮。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| `runtime.activity` reports daemon-wide prompt activity. `activePrompts` counts sessions with an in-flight prompt. `pendingPrompts` counts all accepted prompts that have not settled yet, including the running prompt and FIFO-waiting prompts. `queuedPrompts` counts FIFO-waiting prompts that have been accepted but not dispatched. `lastActivityAt` is the ISO 8601 timestamp of the last prompt start/end or session spawn; `null` when the daemon has never processed any activity since boot. `idleSinceMs` is computed from `lastActivityAt` at response generation time. | ||
|
|
||
| `limits.memory` is additive and reports the daemon's resolved memory figures: a required `enforced: false`, `configuredBudgetMb`, `effectiveBudgetMb` (the configured value capped at resolved cgroup/host memory), `budgetSource` (`flag` / `derived`), `availableMemoryMb`, `availableMemorySource` (`constrained` / `host`), `insufficientMemory`, and a `modeled` object holding `rootReserveMb`, `childPoolMb`, `minChildHeapMb`, `maxChildHeapMb`, and `legacyChildCeilingMb` (a conservative model of the ceiling an ACP child receives today, which can sit below the real figure). `runtime.memory` additionally reports `registeredWorkspaces` (the registration count — non-removed workspace entries, including draining, transitioning, or blocked ones; not a live-child count), `activeAcpChildren` (daemon-managed ACP children with a live, non-dying channel — includes transitioning or blocked entries, but excludes a workspace whose kill has started even if the child has not exited; not channel workers, MCP descendants, or unattached spawn reservations), `childRssCoverage` (`primary_only` today), and a `modeled` object holding `recommendedShareAtRegisteredMb` (`null` when no workspace is registered) and `recommendedShareAtActiveMb` (`null` when no child is active). Each share is capped at the legacy child ceiling, and floored at the minimum child heap only when the ceiling allows — on a small host the ceiling sits below the floor, so share × count can exceed the child pool. Read a share as advisory, not a partition of the pool. All of it is observation: no child spawn argument derives from these values, and no request is refused on their basis. On the normal `runQwenServe` path the budget is resolved before the bootstrap app is created, so `limits.memory` is already populated during the bootstrap window. It is `null` only on paths that resolve no budget (such as direct-embed bypassing `runQwenServeImpl`). The SDK type allows `null`, so correct clients cope. | ||
| `limits.memory` is additive and reports the daemon's resolved memory figures: a required `enforced: false`, a `childHeap` object (`mode`; `maxConcurrentChildren` and `perChildCeilingMb`, both `null` under `mode: 'off'`, which models nothing — and `perChildCeilingMb` additionally `null` wherever no partition can be modeled within `modeled.minChildHeapMb` — either the pool cannot cover one child at that floor, or the ceiling would land under it once capped at `modeled.legacyChildCeilingMb`, which is `floor(available / 2)` and so drops under the floor on a host below 1024 MB. It is never 0, and `maxConcurrentChildren` is `0` in those cases, since a host that models no partition is a computed answer rather than an absent model; and `refusals`, the spawns that would have exceeded the modeled limit), `configuredBudgetMb`, `effectiveBudgetMb` (the configured value capped at resolved cgroup/host memory), `budgetSource` (`flag` / `derived`), `availableMemoryMb`, `availableMemorySource` (`constrained` / `host`), `insufficientMemory`, and a `modeled` object holding `rootReserveMb`, `childPoolMb`, `minChildHeapMb`, `maxChildHeapMb`, and `legacyChildCeilingMb` (a conservative model of the ceiling an ACP child receives today, which can sit below the real figure). `runtime.memory` additionally reports `registeredWorkspaces` (the registration count — non-removed workspace entries, including draining, transitioning, or blocked ones; not a live-child count), `activeAcpChildren` (daemon-managed ACP children with a live, non-dying channel — includes transitioning or blocked entries, but excludes a workspace whose kill has started even if the child has not exited; not channel workers, MCP descendants, or unattached spawn reservations), `childRssCoverage` (`active_children` — every ACP child with a live channel, which is the set `activeAcpChildren` counts; older daemons send `primary_only`), a `children` object described below, and a `modeled` object holding `recommendedShareAtRegisteredMb` (`null` when no workspace is registered) and `recommendedShareAtActiveMb` (`null` when no child is active). Each share is capped at the legacy child ceiling, and floored at the minimum child heap only when the ceiling allows — on a small host the ceiling sits below the floor, so share × count can exceed the child pool. Read a share as advisory, not a partition of the pool. All of it is observation: no child spawn argument derives from these values, and no request is refused on their basis. `childHeap` models a fixed partition of `modeled.childPoolMb` — every child would receive the same `perChildCeilingMb`, so the modeled total stays inside the pool rather than accumulating as a per-spawn share would. Read `refusals` as admission pressure only: a count of 0 does **not** mean the partition is safe to apply, because children run on the much larger host-derived ceiling, so a workload needing more old space than `perChildCeilingMb` is healthy here and would only fail once the partition were applied. Two further reasons a nonzero count need not mean capacity pressure: the admission decision counts a terminating child until it exits, so on a daemon already at `maxConcurrentChildren` every channel replacement books a refusal during the overlap window; and on a host too small to model a partition `maxConcurrentChildren` is `0`, so `refusals` equals the total ACP spawn count, with `insufficientMemory` as the field that explains it. On the normal `runQwenServe` path the budget is resolved before the bootstrap app is created, so `limits.memory` is already populated during the bootstrap window. It is `null` only on paths that resolve no budget (such as direct-embed bypassing `runQwenServeImpl`). The SDK type allows `null`, so correct clients cope. |
There was a problem hiding this comment.
[Suggestion] R6-1: The modeled.legacyChildCeilingMb definition in this paragraph — "which is floor(available / 2)" — omits the 16 GB cap. The implementation is Math.min(Math.floor(memoryMb * LEGACY_CHILD_HEAP_FRACTION), MAX_CHILD_HEAP_MB) with MAX_CHILD_HEAP_MB = 16_384 (daemon-memory-budget.ts), and the sibling JSDocs define it as "min(50% of available memory, 16 GB)". — Failure scenario: probe at the reviewed commit — on a 64 GB host the wire reports legacyChildCeilingMb: 16384 while this formula yields 32768, twice the real value on every host above ~32 GB; an operator hand-checking the partition math (which these docs invite) computes a wrong expected ceiling and reads a contradiction into the triage block. Suggested fix: "which is min(floor(available / 2), 16 GB) and so drops under the floor on a host below 1024 MB".
中文说明
本段对 modeled.legacyChildCeilingMb 的定义——「即 floor(available / 2)」——遗漏了 16 GB 上限。实现为 Math.min(Math.floor(memoryMb * LEGACY_CHILD_HEAP_FRACTION), MAX_CHILD_HEAP_MB),其中 MAX_CHILD_HEAP_MB = 16_384(daemon-memory-budget.ts),同级 JSDoc 也定义为「min(可用内存的 50%, 16 GB)」。失效场景:已在被审提交上探针验证——64 GB 主机上 wire 报告 legacyChildCeilingMb: 16384,而按此公式会算出 32768,在所有 ~32 GB 以上主机上都是真实值的两倍;按文档指引手工核对分区计算的运维会得出错误的期望上限,在排查区块里读出自相矛盾。建议改为:「即 min(floor(available / 2), 16 GB),因此在低于 1024 MB 的主机上会跌到下限之下」。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // undefined. Hence the two helpers rather than one. | ||
| const rss = usableNumerator(input.rssBytes); | ||
| const heapUsed = usableNumerator(input.heapUsedBytes); | ||
| const rssBytes = rss ?? 0; |
There was a problem hiding this comment.
[Suggestion] R6-2: Test-efficacy probe (harness validated, mutant-survived): deleting this ?? 0 fallback left every test added or changed by this diff green. The miss path (unusable/NaN rssBytes) is exercised only through the rssMeasured guard; nothing pins the fallback's output fields. — Failure scenario: when a future edit starts emitting rssBytes on the wire or using it outside the rssMeasured guard, a null/unusable value flows through silently while the 185-line test file stays green — a regression in the miss path ships. Suggested fix: add a case in daemon-memory-pressure.test.ts feeding rssBytes: NaN and asserting the resulting rssBytes/rssRatio/source, so removing the fallback turns the suite red.
中文说明
测试有效性探针(harness 已验证、变异存活):删除这个 ?? 0 兜底后,本 diff 新增或修改的所有测试仍然全绿。未命中路径(不可用/NaN 的 rssBytes)只经由 rssMeasured 门控被覆盖,没有任何测试钉住该兜底的输出字段。失效场景:未来若有改动把 rssBytes 发到 wire 上或在 rssMeasured 门控之外使用它,null/不可用值会静默流出,而这个 185 行的测试文件依旧全绿——未命中路径上的回归会绿色发布。建议:在 daemon-memory-pressure.test.ts 中补一个输入 rssBytes: NaN 的用例,断言结果的 rssBytes/rssRatio/source,使删除该兜底时测试变红。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const rss = usableNumerator(input.rssBytes); | ||
| const heapUsed = usableNumerator(input.heapUsedBytes); | ||
| const rssBytes = rss ?? 0; | ||
| const heapUsedBytes = heapUsed ?? 0; |
There was a problem hiding this comment.
[Suggestion] R6-3: Same measured gap on the heap side (test-efficacy probe, harness validated, mutant-survived): dropping this ?? 0 fallback left every affected test green — no test exercises the unusable-heapUsedBytes miss path. — Failure scenario: a future edit using heapUsedBytes outside the heapMeasured guard (or serializing it into the status payload) propagates null into the wire contract with the whole test file staying green. Suggested fix: add a case feeding an unusable heapUsedBytes and asserting the resulting heapUsedBytes/heapRatio/source.
中文说明
堆侧的同类实测缺口(测试有效性探针,harness 已验证、变异存活):删除这个 ?? 0 兜底后所有受影响测试仍全绿——没有测试覆盖 heapUsedBytes 不可用的未命中路径。失效场景:未来若有改动在 heapMeasured 门控之外使用 heapUsedBytes(或把它序列化进 status 载荷),null 会渗入 wire 契约,而整个测试文件依旧全绿。建议:补一个输入不可用 heapUsedBytes 的用例,断言结果的 heapUsedBytes/heapRatio/source。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| * A side is usable only when *both* its numerator and its denominator are, so | ||
| * this is also the field that says a reported `rssBytes: 0` / `heapUsedBytes: | ||
| * 0` is a placeholder rather than a reading. |
There was a problem hiding this comment.
[Suggestion] R6-9: This JSDoc claims source is "the field that says a reported rssBytes: 0 / heapUsedBytes: 0 is a placeholder rather than a reading", but when both sides are measured and the other side wins, source names the winner and says nothing about the loser's zero. — Failure scenario: probe at the reviewed commit — a genuine rssBytes: 0 reading beside heapRatio 0.5 produces byte-identical output (all nine fields, source: 'heap') to the rssBytes: NaN unusable case; the symmetric heap pair holds too. No wire field resolves the ambiguity, so a consumer following this rule discards a genuine zero reading — the exact confusion this module exists to prevent. The sole production caller (process.memoryUsage()) cannot emit the ambiguous input today; the defect is the contract text, which 19-observability.md repeats. Suggested fix: weaken to "unknown marks both zeros as placeholders; when one side is named, source alone cannot say whether the other side's zero is a placeholder", or drop the sentence.
中文说明
该 JSDoc 声称 source 是「能说明所报告的 rssBytes: 0 / heapUsedBytes: 0 是占位值而非真实读数的字段」,但当两侧都可测量且另一侧胜出时,source 只指出胜者,对败者的零什么也没说。失效场景:已在被审提交上探针验证——heapRatio 为 0.5 时真实的 rssBytes: 0 读数,与 rssBytes: NaN 不可用的情形产生逐字节相同的输出(全部九个字段、source: 'heap');堆侧对称情形同样成立。wire 上没有任何字段能消解这一歧义,按此规则行事的消费者会丢弃真实的零读数——正是本模块声称要防止的混淆。当前唯一的生产调用方(process.memoryUsage())不会产生这种歧义输入;缺陷在契约文本本身,且 19-observability.md 复述了同一说法。建议弱化为:「unknown 表示两个零都是占位值;当指名了某一侧时,仅凭 source 无法判断另一侧的零是否为占位值」,或直接删除该句。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| its numerator and its denominator were usable, so `source` is also what tells a | ||
| zero `rssBytes` / `heapUsedBytes` apart from a real one. |
There was a problem hiding this comment.
[Suggestion] R6-9 (occurrence 2 of 2): This sentence repeats the claim that source tells a zero rssBytes / heapUsedBytes apart from a real reading, which the wire shape cannot do whenever both sides were measurable — probe at the reviewed commit produced byte-identical outputs for a genuine zero reading and an unusable side (see the sibling comment on daemon-memory-pressure.ts). — Failure scenario: an oncall following this recipe's own instruction ("check source before acting") builds a rule that discards genuine zero readings exactly in the case this sentence exists to resolve. Fixing the JSDoc leaves this sentence wrong. Suggested fix: "source names the side that produced ratio; when it is not unknown, the other side's zero cannot be told apart from a placeholder."
中文说明
R6-9(第 2 处,共 2 处):本句复述了「source 能区分零值的 rssBytes / heapUsedBytes 是占位还是真实读数」的说法,但只要两侧都可测量,wire 形状就做不到这一点——已在被审提交上探针验证:真实零读数与不可用一侧产生逐字节相同的输出(见 daemon-memory-pressure.ts 上的姊妹评论)。失效场景:按本配方自身指引(「行动前先查 source」)行事的 oncall,会构造出一条恰好在本句所要解决的场景里丢弃真实零读数的规则。只改 JSDoc 会让本句仍然错误。建议改为:「source 指明产生 ratio 的那一侧;当它不是 unknown 时,另一侧的零无法与占位值区分」。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| * - **Hosts too small to model a partition.** `maxConcurrentChildren` is 0 | ||
| * there, so this equals the total ACP spawn count. Correct by the |
There was a problem hiding this comment.
[Suggestion] R6-15: The documented contract "on hosts too small to model a partition, refusals equals the total ACP spawn count" (stated here, in qwen-serve-protocol.md, and in the SDK mirror) has no test: no suite calls decide() on a policy whose maxConcurrentChildren is 0 — the 512 MB, 1024 MB-derived, and 768–1023 MB band tests assert the snapshot shape but never call decide(). The code honors the contract today. — Failure scenario: a future edit special-casing the non-modelable state — e.g. an early return mirroring the existing off one ("model nothing, refuse nothing") — ships green and silently zeroes the refusal counter on exactly the small hosts this JSDoc warns report alarming counts, removing the only admission-pressure signal those daemons emit. Suggested fix: extend the pool-0 or band test to call decide(1)/decide(n) and assert refuse: true and snapshot().refusals equal to the call count.
中文说明
文档契约「在太小而无法建模分区的主机上,refusals 等于 ACP 派生总数」(此处、qwen-serve-protocol.md 与 SDK 镜像均有表述)没有测试:没有任何套件对 maxConcurrentChildren 为 0 的策略调用过 decide()——512 MB、1024 MB 推导、以及 768–1023 MB 区间测试都只断言快照形状,从不调用 decide()。当前代码是履行该契约的。失效场景:未来若有改动为不可建模状态加特判——例如仿照现有 off 早退再加一个早退(「不建模就不拒绝」)——会绿色发布,恰好在这份 JSDoc 警告会报出惊人读数的小主机上把拒绝计数悄悄清零,抹掉这些 daemon 唯一的准入压力信号。建议:在 pool-0 或区间测试中调用 decide(1)/decide(n),断言 refuse: true 且 snapshot().refusals 等于调用次数。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| childRssBytesTotal += snapshot.rssBytes; | ||
| childRssSampled += 1; |
There was a problem hiding this comment.
[Suggestion] R6-16: The new cross-child sum accumulates snapshot.rssBytes with no non-negativity guard, and the bridge trust boundary does not stop a negative: refreshChildResource checks Number.isFinite but not sign — it clamps cpuPercent to [0,100] on receive under a comment naming "a misbehaving child", while rssBytes gets no sign bound. — Failure scenario: probe at the reviewed commit — a child answering workspaceResource with rssBytes: -123456, cpuPercent: -5 caches -123456 (the sibling clamps to 0 in the same payload), so one misbehaving child subtracts from every other child's reading and can drive runtime.memory.children.rssBytes negative, contradicting its own "a floor rather than a total" JSDoc, while still counting in sampled. Hardening gap, not a live bug: production-spawned children are honest. Suggested fix (probe-verified): clamp at the boundary in bridge.ts, symmetric with cpuPercent — info.childRssBytes = Math.max(0, res.rssBytes).
中文说明
新的跨子进程求和直接累加 snapshot.rssBytes,没有非负防护,而 bridge 的信任边界也拦不住负数:refreshChildResource 检查 Number.isFinite 却不检查符号——它在接收时把 cpuPercent 钳到 [0,100],注释点名威胁是「行为不端的子进程」,rssBytes 却没有符号约束。失效场景:已在被审提交上探针验证——子进程对 workspaceResource 应答 rssBytes: -123456, cpuPercent: -5 时,-123456 被缓存(同一载荷里姊妹字段被钳为 0),于是一个行为不端的子进程会从其他所有子进程的读数里扣减,可以把 runtime.memory.children.rssBytes 打成负数,与其自身「是下限而非总和」的 JSDoc 矛盾,同时仍计入 sampled。这是加固缺口而非现行 bug:生产派生的子进程是诚实的。建议(已探针验证):在 bridge.ts 的边界处对称钳制——info.childRssBytes = Math.max(0, res.rssBytes)。
— qwen3.8-max via Qwen Code /review (v0.21.6)
Local verification report — real daemons, not mocksI built both sides and ran the whole Reviewer Test Plan against live Environment
A detail worth recording, because it makes this host a good test bed: Test plan — results
Step 1 & 5 — before / after on one host
Step 2 —
|
| Mutation | Tests that fail |
|---|---|
drop the MB→bytes conversion on availableBytes |
3 |
Math.max → Math.min on the pressure ratio |
12 |
delete the mode === 'observe' gate on the issue |
1 |
sampled := activeAcpChildren |
3 |
drop the isChannelLive() gate from the sum |
1 |
| widen the staleness cliff to 10× the window | 1 (bridge) |
move decide() back outside the try |
1 — "releases the reservation when a supplied policy throws" |
| revert the sub-floor guard | 4 |
| revert the zero-pool clamp alone | 0 |
| revert both Part-3 guards together | 6 |
Every guard the PR claims is mutation-verified is caught. Counts differ from the PR body in places only because I ran different suite subsets.
The one entry worth a comment is the second-to-last: dropping Math.max(1, …) from admissible on its own fails nothing, because the rawCeilingMb >= MIN_CHILD_HEAP_MB guard subsumes it (floor(0/1) = 0 < 512). Not a defect — the zero-pool regression can only return if both guards go — but the two fixes are pinned by one predicate rather than two, which is worth knowing before either is refactored.
Bootstrap window. The caveat on the pressure type comment is real and I caught it live: polling /daemon/status from process start, there is a window where limits.memory is fully populated with enforced: false while runtime.memory is absent entirely and limits.memory.childHeap is null. Clients must not assume "budget resolved ⇒ pressure present" — the comment says so, and it is accurate.
Metrics ring unchanged. With two children sampled, the ring's childRssBytes read 185 774 080 against a children.rssBytes aggregate of 363 151 360 — the primary child alone. The published singular meaning is intact.
Two notes, neither blocking
F2 — refusals is the one childHeap field that does not distinguish "not modeled" from "zero". On the same 512 MB host: --child-heap-mode observe → refusals: 1; --child-heap-mode off → refusals: 0. Under off, decide() returns early and never counts, so 0 means "not evaluated", while an operator reads it as "no spawn would have been refused". The PR argues carefully for null over 0 on maxConcurrentChildren and perChildCeilingMb for precisely this reason; refusals is the field that did not get the same treatment. A one-line change (refusals: modeled ? refusals : null) plus the SDK mirror would make the block internally consistent. Happy for this to be follow-up.
F3 — calibration data: critical is effectively unreachable on a cgroup-bound daemon. Idle root RSS sits at 165–175 MB, so rssRatio >= 0.8 needs MemoryMax <= ~215 MB — and at 220 MB and below the kernel OOM-kills the daemon before /daemon/status can report anything. I reached soft (300 M) and hard (240 M); every attempt at critical on the RSS side died. In the container case the usable band is soft/hard, and critical will show up mostly on the heap side or not at all. That is a point for the PR's own "uncalibrated for a long-running daemon" caveat, and worth recording now for whoever does the calibration.
Not covered
macOS and Windows, left to CI as the PR states. source: "unknown" is unreachable from the production caller (process.memoryUsage() cannot return NaN) and stays covered by unit tests only. The channel-swap refusal artifact needs 25 concurrent children and was not exercised.
Verdict
The behaviour on the wire matches what the PR describes, on real daemons, including both edge cases the earlier review rounds forced out. Merge-ready from my side. The only thing I would fix before merge is the step-4 instruction in the PR body, since it will send the next reviewer looking for a bug that is not there — and it is a description change, not a code change.
中文版
本地验证报告 —— 真实 daemon,非 mock
我构建了双侧代码,针对真实运行的 qwen serve daemon(Linux)跑完了整份 Reviewer Test Plan,另加若干测试计划未覆盖的检查。七个步骤全部通过。 测试计划中有一项按其字面描述无法复现——说明写错了,代码没错,随 PR 发布的文档也是对的——另有两点提醒。详见下文。
环境
| 主机 | Linux 6.12,16 核,os.totalmem() 32 163 381 248 B → 30 673 MB,cgroup v2 |
| BASE 侧 | merge-base 2c514b50b9 的 worktree,完整 npm run build |
| PR 侧 | PR head a909ff5a14 的 worktree,完整 npm run build |
| Daemon | 11 个真实 qwen serve 进程(10 个在 PR head,1 个在 merge-base):4 个无约束,7 个跑在 systemd-run --scope -p MemoryMax=… cgroup 内(512 M / 768 M / 1024 M / 240 M / 300 M) |
| Workspace | 最多注册 3 个,真实 ACP 子进程,真实 SSE 流 |
有一个细节值得记录,正是它让这台机器成为合适的试验台:这里 process.constrainedMemory() 返回 v1 的「无限」哨兵值 18446744073709552000。detectAvailableMemoryMb() 拒绝了它并回退到主机总量(availableMemorySource: "host",30 673 MB),而 getAcpMemoryArgs() 没有拒绝,于是落到 16 GB 上限。这正是 legacyChildCeilingMb 上记录的第二处背离——是实测而非推断:legacyChildCeilingMb: 15336,而旁边的子进程实际跑着 --max-old-space-size=16384。
测试计划 —— 结果
| # | 检查项 | 结果 |
|---|---|---|
| 1 | runtime.memory.pressure,source 指明胜出的分母 |
✅ 无约束时 source: "heap"(heap 0.0192 vs rss 0.0053);进 cgroup 后翻转为 "rss"(0.659 vs 0.309)。两个方向都验证了「取更差者」 |
| 2 | off 报告全部数值,只去掉判定 |
✅ 用了最锋利的一组对照,见下 |
| 3 | childRssCoverage → active_children,sampled <= activeAcpChildren |
✅ 全程 true |
| 4 | sampled: 0 绝非测得为零 |
✅ 行为正确 —— |
| 5 | max × ceiling <= childPoolMb |
✅ 四种主机规格均成立 |
| 6 | 什么都没被应用;enforce 被拒绝 |
✅ 子进程 argv 逐字节一致,16384 ≠ perChildCeilingMb 572 |
| 7 | 单元测试套件 | ✅ 944 通过(cli 409 + acp-bridge 535),0 失败 |
cli、acp-bridge、sdk-typescript 三者所有改动文件上的 prettier --check、eslint、tsc --noEmit 全部干净。
步骤 1 与 5 —— 同一主机上的前后对比
本机 30 673 MB 上 limits.memory.childHeap 建模为 25 个子进程、每个 572 MB(25 × 572 = 14 300 ≤ 池 14 312)。这与 PR 在干净 32 768 MB 主机上钉住的 25 × 614 是同一个公式,差别只在于本机报告的是 30 673 MB。
步骤 2 —— off 保留读数,只丢掉判定
这是最容易「空过」的断言:健康 daemon 上 level 恒为 normal,即便删掉门控断言也照样成立。所以我把两个 daemon 放进完全相同的 240 MB cgroup,只改 --memory-pressure-mode。两者都因真实 RSS 压力达到 level: "hard":
observe→status: "warning",一条daemon_memory_pressure,severity 为warning(不是error)off→status: "ok",issues: [],但level: "hard"依然完整报出
pressure 块在两种模式下键对键完全一致。
步骤 3 与 4 —— 子进程汇总,以及测试计划的错处
生命周期表现与文档完全一致:空闲 daemon 上 sampled: 0;开流后 sampled: 2;观察者关闭后读数继续留存,直到 oldestReadingAgeMs 越过 ~30 000 ms,随即跌为 sampled: 0 / rssBytes: 0 / oldestReadingAgeMs: null,而 activeAcpChildren 保持 2。
F1 —— 测试计划第 4 步产生不出它所要求的结果。 它写的是:打开两个 workspace、只有一个在流式传输 → sampled: 1 对 activeAcpChildren: 2。我得到的是 sampled: 2,而这才是正确行为。采样门控是 daemon 全局的——getActiveSseCount() 是 routes/sse-events.ts 里的模块级计数器,而 run-qwen-serve.ts 会对 workspaceRegistry.listManaged() 全量做 fan-out——所以任意一处的流都会刷新每一个存活子进程。代码比说明描述得更好,错的只是说明。
该字段真正要覆盖的缺口是存在的,我也复现了:在采样器运行期间注册第三个 workspace 并开一个 session,接下来的一个 5 秒 tick 内状态读作 sampled: 2 对 activeAcpChildren: 3——新子进程还没有缓存读数,求和是个下界,而它如实说明了这一点。这才是应该写进 PR 描述的复现步骤。
需要强调,供其他阅读者参考:这一问题只存在于 PR 描述中。 docs/developers/daemon/19-observability.md、docs/developers/qwen-serve-protocol.md、docs/users/qwen-serve.md 对该门控的描述都是正确的,没有任何错误内容随代码发布。
步骤 6 —— 什么都没被应用
两种 --child-heap-mode 取值下子进程 argv 逐字节一致,都是主机推导的 16384,与建模的 572 相去甚远。这也意味着 #8182 在本机依然明晃晃地敞开着:25 个被授权的子进程 × 16 384 MB = 对 14 312 MB 的池授权了 400 GB,正如 PR 所述——它并未关闭这个问题。
--child-heap-mode enforce 与 --memory-pressure-mode enforce 都被 yargs 拒绝。我还直接驱动了 parseServeFastPathArgs:合法取值会被 fast path 正确带出,enforce 及任何非法值返回 fallback,把报错交给 yargs。
Part 3 —— 两处评审驱动的修复,在真实主机上
评审逼出来的两个缺陷都能通过真实 cgroup 触达,而不只是单元测试的 fixture:
- 512 MB,推导预算 —— root 预留 256 吃掉了全部 256 MB 预算,池为 0 →
maxConcurrentChildren: 0、perChildCeilingMb: null、insufficientMemory: true。修复前这里会建模出上限0,即 V8 约 4 GB 的默认堆。 - 768 MB +
--memory-budget-mb 1024—— 低于下限的那一段,正是通过docs/users/qwen-serve.md:398建议小主机使用的那个标志触达的。池 512 / 1 个子进程 = 512,被legacyChildCeilingMb384 压低,而 384 低于minChildHeapMb512 → 拒绝建模而非压到下限之下。max: 0、ceil: null。 - 1024 MB +
--memory-budget-mb 1024—— 闭区间边界:一个子进程,恰好 512。
测试计划之外
变异扫描。 我逐一在源码中回退各处保护,再跑相应套件,以确认测试是钉住了行为而非仅仅覆盖了代码行:
| 变异 | 失败测试数 |
|---|---|
去掉 availableBytes 的 MB→字节 换算 |
3 |
压力比值 Math.max → Math.min |
12 |
删掉 issue 上的 mode === 'observe' 门控 |
1 |
sampled := activeAcpChildren |
3 |
从求和中去掉 isChannelLive() 门控 |
1 |
| 把陈旧窗口放宽到 10 倍 | 1(bridge) |
把 decide() 移回 try 之外 |
1 —— "releases the reservation when a supplied policy throws" |
| 回退低于下限的那道保护 | 4 |
| 单独回退零池钳位 | 0 |
| 同时回退 Part 3 的两道保护 | 6 |
PR 声称经变异验证的每一处保护都被抓住了。个别数字与 PR 描述不同,仅仅因为我跑的套件子集不同。
值得一提的是倒数第二项:单独去掉 admissible 上的 Math.max(1, …) 一个测试都不会挂,因为 rawCeilingMb >= MIN_CHILD_HEAP_MB 这道保护把它吸收了(floor(0/1) = 0 < 512)。这不是缺陷——零池回归只有在两道保护都被移除时才会回来——但这两处修复实际由一个谓词钉住,而非两个;在重构任一处之前值得知道这一点。
Bootstrap 窗口。 pressure 类型注释上的那条警告是真实的,我实测捕捉到了:从进程启动就开始轮询 /daemon/status,存在一个窗口,其中 limits.memory 已完整填充且 enforced: false,而 runtime.memory 整块缺席、limits.memory.childHeap 为 null。客户端不能假设「预算已解析 ⇒ pressure 存在」——注释正是这么说的,且准确无误。
metrics ring 未变。 两个子进程被采样时,ring 的 childRssBytes 读作 185 774 080,而同期 children.rssBytes 汇总为 363 151 360——即前者仅含主 workspace 子进程。其已发布的单数含义完好无损。
两点提醒,均不阻塞合并
F2 —— refusals 是 childHeap 中唯一没有区分「未建模」与「零」的字段。 同一台 512 MB 主机上:--child-heap-mode observe → refusals: 1;--child-heap-mode off → refusals: 0。在 off 下 decide() 提前返回、从不计数,所以这个 0 的含义是「未评估」,而运维会把它读成「没有派生会被拒绝」。PR 恰恰是为了这个理由,才为 maxConcurrentChildren 与 perChildCeilingMb 论证了用 null 而非 0;refusals 是没有获得同等待遇的那一个。一行改动(refusals: modeled ? refusals : null)加上 SDK 镜像即可让整块内部自洽。作为 follow-up 处理我也没意见。
F3 —— 校准数据:受 cgroup 约束的 daemon 上 critical 基本不可达。 空闲时根进程 RSS 稳定在 165–175 MB,因此 rssRatio >= 0.8 需要 MemoryMax <= ~215 MB——而在 220 MB 及以下,内核会在 /daemon/status 能报出任何东西之前就把 daemon OOM 掉。我达到了 soft(300 M)与 hard(240 M);RSS 侧所有冲击 critical 的尝试全部进程死亡。在容器场景下真正可用的区间是 soft/hard,critical 多半只会出现在 heap 侧,或者根本见不到。这一点是支持 PR 自己那条「未针对长运行 daemon 校准」警告的,也值得现在就记录下来,留给做校准的人。
未覆盖
macOS 与 Windows,按 PR 所述交由 CI。source: "unknown" 从生产调用方不可达(process.memoryUsage() 不会返回 NaN),仍只由单元测试覆盖。channel 交换导致的 refusal 假象需要 25 个并发子进程,未予验证。
结论
线上载荷的行为与 PR 的描述一致,在真实 daemon 上得到验证,包括前几轮评审逼出来的两个边界情形。从我这边看可以合并。 合并前唯一建议修正的是 PR 描述中第 4 步的说明,因为它会让下一位评审者去找一个并不存在的 bug——而这是描述的改动,不是代码的改动。
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 134 passed · 0 failed · 134 total 中文 — 判定:✅ 通过 · 可合入(agent 判定)沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查。 脚本断言:134 通过 · 0 失败 · 134 总计 Verification reportPR 8423 Deep Verification (follow-up round 2) —
|
| # | previous item | severity | status at a909ff5a14 |
|---|---|---|---|
| 1 | verdict merge-ready, 122/122 |
verdict | re-measured: 134/134 at the new head; stands |
| 2 | C1: body "Math.max→Math.min fails 7", measured 9 | correction | stands, gap widened — re-measured 12 at this head (10 in daemon-memory-pressure.test.ts + 2 in daemon-status.test.ts); body still says "fails 7" |
| 3 | C2: body "MB→bytes fails 2", measured 3 | correction | stands — re-measured 3, incl. converts the budget from megabytes when computing the ratio (expected 1 to be 1048576); body still says "fails 2 tests" |
| 4 | bootstrap window (limits.memory populated, childHeap: null, runtime.memory absent) |
verified-along-the-way | re-verified (wire-ab head-default cell, 3 assertions on the first fetch after listening) |
| 5 | source: heap on a large host is worse-of-two working |
verified-along-the-way | re-verified live: heapRatio 0.02113 vs rssRatio 0.00078, source: "heap" |
| 6 | not-covered list | scope | unchanged; re-listed under Not covered |
| 7 | branch force-update context | context | recurred: head moved e8aafa66a2 → a909ff5a14; treated as untrusted rebase again; full re-measurement |
No previous finding was declined; there was nothing to decline (zero blocking findings in either prior round).
Central claim + A/B
Central claim: /daemon/status reports memory observation against real denominators — root pressure (worse of RSS-vs-available and heap-vs-V8-limit), aggregate child RSS with an honest sampled count, and a modeled per-child heap partition that fits the pool — while applying nothing; off modes keep every figure but drop the verdict/partition; and (this round's delta) the modeled ceiling never lands below the documented 512 MB floor, even through the explicit-budget path.
Real daemons booted from the head build and from a base build compiled at HEAD^1 (d5e47709a3): worktree + symlink-farm node_modules with the three changed workspaces re-pointed into the base tree — readlink -f asserted: acp-bridge/qwen-code/sdk → base tree, qwen-code-core (untouched by the diff) → head tree; nested per-package node_modules (third-party only, no @qwen-code entries) shared from the head install; package.json/package-lock.json untouched by the PR, so the shared root store is a clean control. Base dist identity proven: build stamp GIT_COMMIT_INFO = 'd5e47709a3' (head: '3ac2ca9da8'), zero occurrences of active_children/daemon_memory_pressure in base daemon-status.js, and no child-heap-policy.js in base acp-bridge dist. Identical scenario per cell: boot serve --port 0 --token … --workspace …, poll until runtime.memory exists, GET /daemon/status with bearer auth. Witness: 01-wire-ab-head-vs-base.png.
| cell | environment | oracle | result |
|---|---|---|---|
| head-default | head build, default flags | 24 assertions: bootstrap window (limits.memory populated, childHeap: null, runtime.memory absent on the first fetch); pressure present with exactly the 10 published keys; mode: observe; ratio == max(rssRatio, heapRatio); source = winner (live: heap 0.02113 > rss 0.00078); availableBytes == availableMemoryMb × 2²⁰ exact on the wire (253266 MB → 265568649216 B); level classifies ratio; live numerators > 0; childRssCoverage: active_children; idle children {0, 0, null}; sampled ≤ activeAcpChildren; childHeap {observe, 25, 5024, 0} recomputed from the wire's own modeled figures (pool 125609, floor 512, legacy 16384); 25×5024=125600 ≤ 125609; refusals: 0; enforced === false; pressure issue only when not normal (always warning); rollup ok |
24/24 |
| base-default | base build, default flags | 9 assertions: boot + runtime ready; no pressure, no children; childRssCoverage: primary_only; no childHeap key; enforced: false; head runtime.memory keys == base + exactly {children, pressure}; head limits.memory keys == base + exactly {childHeap} |
9/9 |
| head-both-off | head build, both flags off |
17 assertions: pressure fully populated (same 10 keys, mode: off); ratio/source/level consistent; MB→bytes exact; no daemon_memory_pressure issue regardless of level; rollup ok unchanged; childHeap object present {mode: off, maxConcurrentChildren: null, perChildCeilingMb: null, refusals: 0} (null ≠ zero-pool 0); enforced: false; sampling coverage unaffected |
17/17 |
| head-enforce-×2 | head build, --child-heap-mode enforce / --memory-pressure-mode enforce |
2×2 assertions: daemon refuses to boot (non-zero exit, no listening line); stderr carries yargs Invalid values … Choices: "off", "observe" naming the flag |
4/4 |
Session-level cells (witnesses: 02-session-probe-observe.png, 03-session-probe-off.png): real POST /session spawns a real qwen --acp child against a loopback fake-OpenAI decoy (0 requests received in both modes — the probe never prompts); GET /session/:id/events held open as the SSE watch gate:
| probe | argv oracle | sampling oracle | result |
|---|---|---|---|
| observe | /proc-scanned child parented by the daemon carries --max-old-space-size=16384 (host-derived: min(floor(253266/2), 16384)), not perChildCeilingMb 5024, and larger than it |
with watcher live: children {rssBytes: 223547392, sampled: 1, oldestReadingAgeMs: 48}, activeAcpChildren: 1, partition recomputes {observe, 25, 5024, 0}, refusals: 0 after the spawn, enforced: false |
18/18 |
| off | argv byte-identical: --max-old-space-size=16384 |
sampling still live {rssBytes: 228569088, sampled: 1, oldestReadingAgeMs: 50}; partition {off, null, null, 0} while the child is live |
15/15 |
The delta commit 6872d59023 — sub-floor ceiling
The defect: perChildCeilingMb was min(floor(pool / admissible), legacyChildCeilingMb); the legacy term is floor(available / 2) and can sit below MIN_CHILD_HEAP_MB (512). Unreachable from a derived budget (the pool hits 0 first), but reachable through --memory-budget-mb, which docs/users/qwen-serve.md recommends on exactly those hosts. Module cells on the compiled dist (witness: 04-module-matrix.png, 29/29 overall):
| cell | expectation | result |
|---|---|---|
avail ∈ {768, 900, 1000, 1023} + explicit budgetMb: 1024 |
band reachable (pool 512/644/744/767 ≥ 512; cap 384/450/500/511 < 512) and model refused: {maxConcurrentChildren: 0, perChildCeilingMb: null} with minChildHeapMb: 512 beside it |
4/4 |
| boundary 1024/1024 | inclusive: exactly {1, 512} |
1/1 |
| just above 1025/1024 | still models {1, 512} |
1/1 |
| derived small hosts 512 / 1024 MB | {0, null} (pool 0 / pool 256 < floor) — never a 0 ceiling |
2/2 |
pins 8 GB → 7×526, 32 GB → 25×614; 1 TB ceiling capped at legacy 16384; invariant sweep over 10 host sizes; decide() refusal counting; off cells |
as pinned by the PR | all pass |
Mutation A/B on the delta: reverting the guard (mut10) publishes the sub-floor partition and is killed by the four band tests (expected 384 to be null); making the boundary exclusive (mut11) is killed by the boundary test; nulling unconditionally (mut3) is killed by 14 tests including the boundary and its paired observe control — so neither over- nor under-refusal escapes the suite.
Corrections
Description-level only — the code is correct; these numbers in the PR body are stale against the merged head (test growth arrived via the two main merges). None is a request to change code.
- (carried, stands, gap widened) Body: "
Math.max→Math.minon the pressure ratio fails 7" — measured 12 at this head (mut4 decomposition: 10 indaemon-memory-pressure.test.ts[6 threshold-edge cases,reports the worse of the two denominators…, 3 sanitization cases] + 2 indaemon-status.test.ts[reports pressure figures in both modes…,raises nothing on a healthy daemon…]). Previous round measured 9 at the old head; the pressure suite grew in the merges. - (carried, stands) Body: "dropping the MB→bytes conversion fails 2 tests" — measured 3 (mut5), the named one being
converts the budget from megabytes when computing the ratiowithexpected 1 to be 1048576(the 2²⁰ factor). - (carried, stands, nit) The bootstrap comment in
run-qwen-serve.tsstill sayschildHeapis null "even when the flag saysenforce" —enforceis not an accepted value (yargs rejection re-verified in both enforce cells). The guarded invariant itself is correct and wire-verified. - (new) Body: "
sampled := activeAcpChildrenfails 2" — measured 3 (mut6):reports live child counts and advisory shares under runtime,sums only the children that actually reported, and says how many did, andcounts a draining workspace that still holds a live child. - (new) Body: "deleting the flag from the handler fails 2" — measured 4 (mut9): the two pass-through tests plus
defaults the child heap mode to observe, and rejects enforce outrightanddefaults the memory pressure mode to observe.
Confirmations (body claims re-measured and found exact at this head): "dropping the liveness gate fails 1" (mut7: 1), "widening the staleness cliff fails the bridge test" (mut8: 1), the 8 GB → 7×526 and 32 GB → 25×614 pins.
Findings
None blocking. Every executed assertion passed; no behavioral mismatch was produced by any cell, probe, or mutant.
Coverage observation (suggestion, not a merge condition): the pressure suite's threshold fixtures are parameterized from the constants under test ([SOFT_PRESSURE_RATIO * AVAILABLE - 1, 'normal'], …), which makes constant-value mutations self-neutralizing — the round's first positive control (0.5 → 0.4) survived because the fixtures moved with the mutant. What the suite pins is classify()'s inclusive comparisons (mut0b, >= → >, killed on the exact-boundary fixture). The literal threshold values (0.5/0.65/0.8) are inherited from core's MemoryPressureMonitor and documented as that contract; if pinning the values themselves matters, a fixture with literal ratios would do it. The harness's positive control was switched to the pinned clause accordingly.
Not covered
- Windows / macOS — ran on the lane's
node:22-bookwormcontainer. The PR's platform-sensitive decision (child self-reported RSS instead of/proc) rides on the repo's CI matrix. refusalsunder real admission pressure — >25 concurrent sessions not driven live; covered by module-matrixdecide()cells and the unit suites.- WS watch path — the sampler gate is
sseCount > 0 || wsStreams > 0; only the SSE half was driven live (both probes). - Two-workspace sampled-shortfall live cell (Reviewer Test Plan step 4) — the one-workspace live case (
sampled: 1ofactiveAcpChildren: 1) and the idle case were driven live; the two-workspaces-one-streaming variant is pinned only by unit tests (mut6/mut7 red tests). oldestReadingAgeMswith pre-field contributors — unit-covered; a genuinely older bridge was not constructed live.- Per-commit attribution — depth-2 checkout: the metadata lists 11 commits but
git rev-list HEAD^1..HEAD^2returns only the head merge commit, so the individual commits (incl. the merged-inmaincontent) are unreachable and the aggregateHEAD^1..HEADdiff is what was verified. The two base-tip merges since the last round brought test growth (visible in the corrected counts) but no production change to the PR's surface beyond6872d59023, which was verified directly. - SDK type mirror — read field-by-field against the observed wire (optional
pressure/children/childHeap, unionchildRssCoverage, nullable partition fields — all consistent) and compiled by the workflow's full build; no separate SDK compile gate or mixed-version daemon/client pairing. - Docs — six documentation files changed; content not audited beyond the
childRssCoverage/childHeap/pressureclaims the wire oracle settled. Thedocs/users/qwen-serve.mdrecommendation that makes the sub-floor band reachable was verified to exist, not copy-edited. - Repo-wide suite / lint / typecheck — targeted gates only (acp-bridge full: 1090/1090; cli affected serve suites: 420/420). The workflow's pre-run
npm run buildat head is the compile evidence. - Base-arm purity — unchanged workspaces resolve into the head tree (realpath-asserted; the PR diff does not touch them); nested per-package
node_modulesshared from the head install contain only third-party packages (no@qwen-codeentries, verified); the git-commit stamp is regenerated in the base tree (d5e47709a3), so stamp content is a build artifact of the control, not a diff.
Methodology
Environment: the verify lane's own container, Node v22.23.2, ~247 GB host (availableMemoryMb 253266, cgroup unconstrained → availableMemorySource: host), V8 heap_size_limit 4345298944 B, 64 cores. Base control: git worktree add tmp/base-tree HEAD^1 with a symlink-farm node_modules re-pointing @qwen-code/{acp-bridge,qwen-code,sdk} into the base tree (realpath-asserted both directions) plus the nested third-party node_modules; acp-bridge, sdk, and cli rebuilt there from base sources (base stamp d5e47709a3), worktree removed after the cells were captured. Harnesses (all mock-free, in harness/): wire-ab.mjs (5 cells, real daemons, real HTTP), session-probe.mjs (real child spawn, argv read from /proc with a PPid check against the daemon, held SSE stream, loopback fake-OpenAI decoy that counts requests), module-matrix.mjs (compiled dist modules driven directly, incl. the delta band), mutation-summary.mjs + mutation-followup.mjs (12 mutants applied as exact single-occurrence replacements, suites run red on named behavioral assertions, files restored via git checkout --, tree asserted clean at the end). Targeted gates: full acp-bridge suite and the five affected cli suites, run unmutated. Raw payloads: logs/cells/*.json, logs/probe-*-status.json, logs/probe-*-argv.json; logs: logs/build-base.log, logs/mutation-details.log. Evidence captures via scripts/verify-capture.mjs. Assertions: wire-ab 54 + session 18 + 15 + module 29 + mutation 18 = 134, all scripted, all executed, all passed.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
@qwen-code /triage |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
@qwen-code /triage |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
yiliang114
left a comment
There was a problem hiding this comment.
Re-reviewed the updated revision — still strictly observation-only, and the refinements are an improvement: the mode flag is now just off|observe (the earlier enforce draft was a dead switch and is removed, explicitly 'no forced GC, no eviction, no session closure, no process termination'); --child-heap-mode off publishes null partition (maxConcurrentChildren/perChildCeilingMb both null); the added source semantics (RSS-vs-cgroup vs V8 heap_size_limit, rssRatio as a lower bound under host source) make the two denominators' meaning precise. The reclamation keywords appear only in docs describing what it does NOT do. No new P0/P1; my earlier note (calibrate the observe thresholds from this phase's data before any future enforcement) stands.
|
Released in v0.21.8. |















What this PR does
Turns the denominator #8245 landed into actual readings, and then models — without applying — the partition those readings exist to justify. Everything here is observation: no forced GC, no eviction, no session closure, no refusal, and no child spawn argument derived from any of it.
Part 1 — the daemon root's own pressure
runtime.memory.pressurereportslevel,ratio,source, and the six raw figures behind them.Two denominators, worse one wins. The failure modes are independent: a container dies by RSS against its cgroup limit, while a process on a large host can exhaust V8's heap long before RSS is a meaningful fraction of the machine. Reporting only one hides whichever failure the deployment is heading for.
sourcenames which ratio produced the level.unknown≠ healthy. When neither denominator is measurable,sourcesaysunknown;levelisnormalthere only because there is nothing to classify.The denominator is
availableMemoryMb, noteffectiveBudgetMb. Pressure asks how close this process is to being killed, and what kills it is the cgroup limit or host memory. An operator's budget is a policy number; classifying against it would reportcriticalfor a daemon in no danger.The flag.
--memory-pressure-mode,off | observe, defaultobserve. Both modes report every figure; onlyobservealso raises thedaemon_memory_pressurewarning, sooffleaves the top-levelstatusrollup untouched. The thresholds are inherited from core's interactive-CLIMemoryPressureMonitorand are not yet calibrated for a long-running daemon — a deployment that alerts onstatusneeds the reading without the verdict.No
enforce: nothing here remediates, and a value a caller can pass but never use is a dead switch. Severity iswarningat every level includingcritical, becauseerrorwould makerollupStatusreturnerrorfor the whole daemon — too strong a claim to stake on uncalibrated thresholds.Part 2 — aggregate child RSS
The root reading above stays
normalon a daemon whose children are the ones growing.childRssCoveragehas saidprimary_onlysince #8245 to admit that blind spot; #8245 shaped it as a single string literal so it could change without inventing a field.runtime.memory.childrenreports summed RSS across every child with a live channel, plus:sampled— how many children actually produced a reading. Load-bearing, not decoration: a sum with silent gaps reads exactly like an authoritative total. Its denominator is the siblingactiveAcpChildren, so a shortfall is visible without the client knowing to look.sampled: 0withrssBytes: 0never means a measured zero.oldestReadingAgeMs— how far apart the sum's parts were taken, since the staleness window lets them span 30 s.nullwhen nothing was sampled and when every contributor predates the field, so it never means "fresh".An enumeration change, not a new mechanism. Every bridge already caches a self-reported reading behind
getChildResourceSnapshot; the sampler simply only ever called it onprimaryEntry. The sum and theactiveAcpChildrencount are taken in one synchronous pass over the same array, gating on the sameisChannelLive()predicate — sosampled <= activeAcpChildrenholds by construction rather than by trusting the hook to self-gate. (A test caught that: with the sum trusting the hook, a stub that didn't self-gate got counted. The dormant-bridge stub is now deliberately unfaithful so the gate cannot be removed silently.)What the figure is not. An over-count and an under-count at once, and the docs say so rather than calling it tree memory: summing per-process RSS double-counts shared pages, while each child reports only its own process, so its MCP descendants and every channel worker are missing. Reading RSS from the OS by PID would fix both plus hung children — the daemon does know the PIDs,
spawnChannel.tsuseschild.pidfor the stderr prefix — but/procis Linux-only and this ships on macOS and Windows (both in the CI matrix). Recorded as the natural upgrade, not attempted.Not folded into
pressure.ratio: imprecise in both directions, components sampled at different instants, and Part 1's thresholds are mid-calibration — changing that numerator would destroy the data being gathered.The metrics ring's
childRssByteskeeps its published singular meaning (the primary child's RSS) and is unchanged.Part 3 — modeling a per-child heap partition
getAcpMemoryArgs()computes one ceiling from host memory and hands the same value to every child. The daemon runs one child per workspace and registers up to 25, so a 32 GB host authorises 25 × 16 GB = 400 GB of child heap — the defect #8182 records. This part does not close it. It publishes the partition that would close it, so the partition can be judged before anything depends on it.--child-heap-modeisoff | observe, defaultobserve. Status gainslimits.memory.childHeap:maxConcurrentChildren,perChildCeilingMb, andrefusals(spawns that would have exceeded the modeled limit). Nothing is applied — no child is sized from the budget, no spawn is refused, andlimits.memory.enforcedstays the required literalfalse.A fixed partition, not a per-spawn share. Sizing each child by the count live at its spawn bounds the child count but not the memory: V8 cannot lower a running child's ceiling, so grants accumulate as
P + P/2 + P/3 + … = P × H(n). That was the first attempt, and review took it apart with numbers — 9557 MB authorised against a 3687 MB pool at seven children on 8 GB; 61355 MB against 15360 MB on 32 GB. The model is now one constant ceiling for every child, with admission capped somaxConcurrentChildren × perChildCeilingMb ≤ childPoolMbholds by construction, with no ledger and no dependence on arrival order.Why there is no
enforce. Its inclusion rested on "observe first, then enforce", and that path does not exist yet. While observing, children run on the host-derived ceiling — 16384 MB on a 32 GB host — so a workload needing 2 GB of old space is healthy withrefusals: 0and OOMs the instant a 614 MB partition is applied. The counter measures admission pressure, not ceiling adequacy. Deciding when enforcing is safe needs peak old-space per child (not rss, which is what the child self-reports today, and notheapUsed, which includes the new generation), measured inside the child so GC-time peaks are not missed. That is its own measurement chain, and the enforcing mode ships with it. Shipping a switch with no safe way to decide when to turn it on is worse than shipping no switch.Consequently the machinery that existed only to apply the partition was removed rather than shipped unreachable:
getAcpMemoryArgs(explicitMb?),ChildHeapPoolExhaustedErrorand both transport mappings, and the widening oflimits.memory.enforcedtoboolean.The zero-pool defect, also from review and caused by a clamp forcing at least one admissible child: a 512 MB host — where the root reserve consumes the entire 256 MB budget, leaving a pool of 0 — modeled a ceiling of 0, and
--max-old-space-size=0is V8's default heap, roughly 4 GB, not a zero ceiling. A pool that cannot cover one child at the 512 MB floor now reportsmaxConcurrentChildren: 0andperChildCeilingMb: null. The test that had enshrined the old behaviour ("always admits at least one child, however small the pool") is inverted.The sub-floor ceiling defect, from @wenshao's review of this revision.
perChildCeilingMbwasmin(floor(pool / max), legacyChildCeilingMb); the first term clearsMIN_CHILD_HEAP_MBby construction but the second isfloor(available / 2)and does not, so theMath.mincould publish a ceiling below theminChildHeapMbin the same snapshot —avail=768with--memory-budget-mb 1024modeled one child at 384 MB. Unreachable from a derived budget, because the pool hits 0 first; reachable through the explicit flag, whichdocs/users/qwen-serve.mdrecommends on exactly these hosts. The model is now refused rather than shrunk under the floor, withmaxConcurrentChildrenzeroed in lockstep. The existing matrix resolved derived budgets only, which is why the mutation sweep came back clean, so it gains abudgetMbaxis plus the inclusive boundary (1024/1024 → one child at 512) that stops unconditional nulling from passing instead.What an operator gets.
perChildCeilingMbandmaxConcurrentChildrenare published so the partition can be judged against a known workload — the substitute for a counter that cannot judge it. An 8 GB host models 7 children at 526 MB; a 32 GB host models 25 at 614 MB, both pinned in tests since they are the numbers one plans against.Docs
Six files.
childRssCoveragehad eight references, four carrying claims that go false once it stops readingprimary_only— including two shipped type comments (the daemon's and the SDK's) that cited it as the evidence pressure is root-only. All four now state pressure's scope on its own terms and point atchildren.rssBytes. Also a new observability triage recipe, and the design doc's Part 2 corrected to describe what shipped.Why it's needed
The daemon samples its own RSS and heap every 5 s but has nothing to divide them by, so no field in
/daemon/statussays whether a number is fine or nearly fatal. And the memory that actually matters isn't in the root process at all — per-session RSS lives in theqwen --acpchildren, which the daemon only ever measured for the primary workspace.#8245 landed the denominator (
limits.memory). This turns it into readings, for both.Part 2 of the design in
docs/design/2026-07-31-daemon-capacity-model-and-memory-bounds.md.Reviewer Test Plan
How to verify
1. The root pressure reading, and that
sourcenames the denominator that won.levelisnormal/soft/hard/critical;ratiois the worse ofrssRatioandheapRatio;sourcesays which produced it. Checksourcebefore believinglevel:unknownmeans neither denominator was measurable, sonormalthere is the absence of a reading, not evidence of health.2.
offreports everything and only drops the verdict. Boot once per mode and diff:Under
--memory-pressure-mode offthepressureblock must be fully populated and identical in shape, while thedaemon_memory_pressureissue must be absent and the top-levelstatusrollup unchanged. A mode that suppressed the figures too would defeat the purpose of the flag.3. Aggregate child RSS, and the coverage flip.
childRssCoveragemust now readactive_children(it wasprimary_only). The invariant that makeschildren.rssByteshonest:4.
sampled: 0is never a measured zero. With two workspaces open but only one streaming,children.rssBytesmust be accompanied bysampled: 1againstactiveAcpChildren: 2— the shortfall visible without knowing to look for it.5. The modeled partition fits inside the pool it partitions.
On an 8 GB host expect
{"mode":"observe","maxConcurrentChildren":7,"perChildCeilingMb":526,"refusals":0}; on 32 GB,25at614. The invariant that makes it an aggregate bound rather than a per-spawn share holds on any host:6. Nothing is applied — the check that matters for Part 3. Open a session so a
qwen --acpchild spawns, then read its argv under each mode:The value must be the host-derived ceiling and byte-identical under
--child-heap-mode observeand--child-heap-mode off. In particular it must not equalperChildCeilingMb. Confirm alongside it thatlimits.memory.enforcedis still the literalfalse, and that the enforcing mode is genuinely absent rather than merely defaulted off:qwen serve --child-heap-mode enforce # error: Invalid values: Argument: child-heap-mode, Given: "enforce", Choices: "off", "observe"7. Unit suites.
Evidence (Before & After)
N/A — no user-visible or TUI change. The observable difference is the
/daemon/statuspayload:"runtime": { "memory": { - "childRssCoverage": "primary_only", + "childRssCoverage": "active_children", + "pressure": { "level": "normal", "ratio": 0.31, "source": "rss", ... }, + "children": { "rssBytes": 402653184, "sampled": 2, "oldestReadingAgeMs": 4120 }, "activeAcpChildren": 2 } }, "limits": { "memory": { "enforced": false, + "childHeap": { "mode": "observe", "maxConcurrentChildren": 7, "perChildCeilingMb": 526, "refusals": 0 }, "modeled": { "childPoolMb": 3687, ... } } }Child argv is unchanged in both directions — that is the point of Part 3, and step 6 above is how a reviewer confirms it.
Tested on
Locally on Linux:
tscon all three ofcli,sdk-typescript, andacp-bridge.Math.max→Math.minon the pressure ratio fails 7; deleting the flag from the handler fails 2;sampled := activeAcpChildrenfails 2; dropping the liveness gate fails 1; widening the staleness cliff fails the bridge test.normaland the assertion would hold with the gate deleted.For Part 3: 49 acp-bridge tests across
child-heap-policy,process-registry, andspawnChannel, plus 218 inrun-qwen-serve. The invariantmaxConcurrentChildren × perChildCeilingMb ≤ childPoolMbis asserted across 2/8/32/256 GB hosts;observeis asserted to leave spawn argv byte-identical to a factory with no policy;enforceis asserted to be rejected by both yargs and the fast path. Two review findings against the merged revision are fixed here and both are mutation-verified: the status assertion inrun-qwen-serve.test.tswas failing on head against the four-field wire shape, anddecide()ran outside thetrythat cancels the spawn reservation, so a throwing caller-supplied policy leaked the token — reverting that move reproducesexpected 1 to be +0.macOS and Windows are left to CI. The one platform-sensitive decision is deliberate: RSS is read from each child's self-report rather than
/proc, precisely because/procis Linux-only.Environment (optional)
npm run devdaemon on Linux; unit and e2e tests for the assertions above.Risk & Scope
levelmay be wrong in both directions on real deployments — which is exactly why every level maps towarningseverity and nevererror, and whyoffkeeps the figures while dropping the verdict. The second tradeoff ischildren.rssBytesbeing an over-count and an under-count at once (shared pages double-counted; MCP descendants and channel workers missing); it is documented as such rather than presented as tree memory, and deliberately kept out ofpressure.ratio. The third belongs to Part 3: publishing a partition invites a reading it cannot support — an operator seeingrefusals: 0may conclude enforcing is safe. It is not, for the reason given under Part 3, and that caveat is now stated in the flag help, all three operator docs, and the protocol doc. The tradeoff accepted there is shipping a model that constrains nothing: bug(serve): daemon authorises each ACP child 50% of host memory, never divided by child count #8182's 400 GB overcommit stays open until the enforcing mode lands with its measurement.offpublishes no partition: bothmaxConcurrentChildrenandperChildCeilingMbarenullthere, distinct from the0that means a pool too small to host one child.pressureandchildrenare both optional in the SDK mirror because daemons that shippedruntime.memorybefore them send the block without those fields;childRssCoverageis a union there for the same reason, while the daemon's own type stays a single literal. No capability tag:daemon_statusis already baseline, and the two-edit gating contract covers onlyCONDITIONAL_SERVE_FEATURES. Clients that pinnedchildRssCoverage === 'primary_only'will see'active_children'— the field was shaped as a single literal in feat(serve): resolve and report the daemon memory budget #8245 for exactly this transition. From Part 3,limits.memory.childHeapis likewise additive andlimits.memory.enforcedremains the required literalfalse: an earlier revision widened it toboolean, and that widening was reverted, so no client contract changes.--child-heap-modeis a new flag defaulting toobserve;enforceis not an accepted value.Linked Issues
Refs #8051 (Parts 1–2) and #8182 (Part 3). Supersedes #8462 and #8508, both merged into this branch.
中文说明
这个 PR 做了什么
把 #8245 落地的分母变成真正的读数,并对这些读数所要支撑的那份分区做出建模——但不应用它。这里的一切都是观测:不强制 GC、不驱逐、不关闭 session、不拒绝请求,也没有任何子进程派生参数由此推导。
Part 1 —— daemon 根进程自身的压力
runtime.memory.pressure报告level、ratio、source,以及支撑它们的六个原始数值。两个分母,取更差者。 两种失效模式相互独立:容器是因 RSS 触及 cgroup 限制而死,而大内存主机上的进程可能在 RSS 还只占机器一小部分时就耗尽 V8 堆。只报其一,就会掩盖该部署正在走向的那种失效。
source指明是哪个比值产生了level。unknown≠ 健康。 当两个分母都无法测量时,source为unknown;此时level之所以是normal,仅仅因为没有可分类的东西。分母是
availableMemoryMb,不是effectiveBudgetMb。 压力问的是「这个进程离被杀有多近」,而杀死它的是 cgroup 限制或主机内存。运维的预算是一个策略数字;用它来分类会让毫无危险的 daemon 报出critical。这个 flag。
--memory-pressure-mode,取值off | observe,默认observe。两种模式都报告全部数值;只有observe会额外抛出daemon_memory_pressure警告,因此off不会触动顶层status汇总。阈值继承自 core 面向交互式 CLI 的MemoryPressureMonitor,尚未针对长运行 daemon 校准——依据status告警的部署需要读数,但不需要那个判定。没有
enforce:这里不做任何补救,而一个调用方能传却永远用不上的值就是死开关。所有级别(包括critical)的 severity 都是warning,因为error会让rollupStatus把整个 daemon 判为error——用未校准的阈值下这么重的结论太过了。Part 2 —— 子进程 RSS 汇总
当增长发生在子进程时,上面的根进程读数会一直保持
normal。自 #8245 起childRssCoverage就一直是primary_only,用以承认这个盲区;#8245 把它设计成单个字符串字面量,正是为了将来可以改变而无需新增字段。runtime.memory.children报告所有持有活跃 channel 的子进程的 RSS 汇总,另加:sampled—— 实际产出了读数的子进程数。这是承重字段而非装饰:一个带有静默缺口的求和,读起来和权威总数一模一样。它的分母是同级的activeAcpChildren,因此缺口无需客户端刻意查找即可见。sampled: 0配rssBytes: 0绝不表示测得为零。oldestReadingAgeMs—— 求和的各部分采样时刻相隔多远,因为陈旧窗口允许它们跨越 30 秒。当没有任何采样时为null,当所有贡献者都早于该字段存在时也为null,所以它绝不意味着「新鲜」。这是枚举方式的改变,不是新机制。 每个 bridge 早就在
getChildResourceSnapshot背后缓存了自上报读数;采样器只是从来只对primaryEntry调用它。求和与activeAcpChildren计数在同一次对同一数组的同步遍历中取得,并依据同一个isChannelLive()谓词做门控——因此sampled <= activeAcpChildren是按构造成立的,而不是靠信任 hook 自行门控。(一个测试抓到了这点:当求和信任 hook 时,一个不自行门控的 stub 被计入了。现在那个休眠 bridge 的 stub 被刻意做得不忠实,好让这道门控无法被静默移除。)这个数字不是什么。 它同时既高估又低估,文档如实说明而非把它称作进程树内存:按进程求和 RSS 会重复计算共享页,而每个子进程只报告自己这一个进程,所以它的 MCP 子孙进程和所有 channel worker 都缺失。按 PID 从操作系统读 RSS 能同时修正这两点外加卡死的子进程——daemon 确实知道这些 PID,
spawnChannel.ts就用child.pid做 stderr 前缀——但/proc仅限 Linux,而本项目发布到 macOS 与 Windows(两者都在 CI 矩阵中)。这被记录为顺理成章的后续升级,本次未做。未并入
pressure.ratio:它在两个方向上都不精确,各分量采样时刻不同,且 Part 1 的阈值正在校准中——改动那个分子会毁掉正在收集的数据。metrics ring 的
childRssBytes保持其已发布的单数含义(主 workspace 子进程的 RSS),未作改动。Part 3 —— 对子进程堆分区建模
getAcpMemoryArgs()从主机内存算出一个上限,然后把同一个值发给每个子进程。daemon 每个 workspace 跑一个子进程,最多注册 25 个,于是一台 32 GB 主机授权了 25 × 16 GB = 400 GB 的子进程堆——这就是 #8182 记录的缺陷。本部分并未关闭它,而是把能关闭它的那份分区发布出来,好让这份分区在被依赖之前先接受检验。--child-heap-mode取值off | observe,默认observe。状态新增limits.memory.childHeap:maxConcurrentChildren、perChildCeilingMb,以及refusals(本会超出建模上限的派生次数)。不应用任何东西——没有子进程按预算调整大小,没有派生被拒绝,limits.memory.enforced保持必需的字面量false。固定分区,而非按派生分摊。 按各自派生时刻的存活数给每个子进程定额,约束的是子进程数量而非内存:V8 无法下调运行中子进程的上限,于是授权累加为
P + P/2 + P/3 + … = P × H(n)。第一版就是这么做的,评审用数字把它拆掉了——8 GB 主机上七个子进程时,对 3687 MB 的池授权了 9557 MB;32 GB 上是 61355 MB 对 15360 MB。模型现在是给每个子进程一个恒定上限,并对准入设上限,使maxConcurrentChildren × perChildCeilingMb ≤ childPoolMb按构造成立,无需账本,也不依赖到达顺序。为什么没有
enforce。 它的存在依赖「先观察、再强制」,而这条路径尚不存在。观察期间子进程跑在主机推导的上限上——32 GB 主机是 16384 MB——所以一个需要 2 GB 老生代的负载在refusals: 0下完全健康,而一旦应用 614 MB 的分区就会立刻 OOM。这个计数衡量的是准入压力,不是上限是否够用。要判断何时强制是安全的,需要每个子进程的峰值老生代(不是子进程今天自上报的 rss,也不是包含新生代的heapUsed),且必须在子进程内部测量,否则会漏掉 GC 时刻的峰值。那本身是一条独立的测量链,强制模式将与它一同发布。发布一个无法安全判断何时开启的开关,比不发布这个开关更糟。因此那些只为应用分区而存在的机制被移除,而非以不可达的形式发布:
getAcpMemoryArgs(explicitMb?)、ChildHeapPoolExhaustedError及其两处传输层映射,以及把limits.memory.enforced放宽为boolean的那次改动。零池缺陷,同样来自评审,由「强制至少允许一个子进程」的钳位导致:512 MB 主机——root 预留吃掉了全部 256 MB 预算,池为 0——会建模出上限 0,而
--max-old-space-size=0表示 V8 的默认堆,约 4 GB,并非零上限。现在,连一个 512 MB 下限子进程都容纳不下的池会报告maxConcurrentChildren: 0与perChildCeilingMb: null。那个把旧行为固化成预期的测试("always admits at least one child, however small the pool")已被反转。低于下限的上限缺陷,来自 @wenshao 对本版本的评审。
perChildCeilingMb原为min(floor(pool / max), legacyChildCeilingMb);前一项按构造不低于MIN_CHILD_HEAP_MB,后一项是floor(available / 2)则不然,于是Math.min可能发布出一个低于同一快照中minChildHeapMb的上限——avail=768配合--memory-budget-mb 1024会建模出一个 384 MB 的子进程。从推导预算无法到达,因为池会先归零;经由显式标志可以到达,而docs/users/qwen-serve.md恰恰建议在这类主机上使用该标志。现在的做法是拒绝建模,而不是把上限压到下限之下,并同步把maxConcurrentChildren归零。原有的参数矩阵只解析推导预算,这正是变异扫描当初一片绿的原因,因此它新增了budgetMb这一维,外加闭区间边界(1024/1024 → 一个子进程 512 MB),以阻止「无条件置 null」蒙混过关。运维实际得到什么。 发布
perChildCeilingMb与maxConcurrentChildren,是为了让这份分区能对照已知负载来判断——这是那个无法做判断的计数的替代品。8 GB 主机建模为 7 个子进程、每个 526 MB;32 GB 建模为 25 个、每个 614 MB,两者都在测试中固定,因为这正是做容量规划时依据的数字。文档
六个文件。
childRssCoverage有八处引用,其中四处所载的说法在它不再是primary_only之后就会变假——包括两处已发布的类型注释(daemon 的与 SDK 的),它们把该字段引作压力读数仅覆盖根进程的证据。这四处现在都以压力自身的口径陈述其覆盖范围,并指向children.rssBytes。另有一份新的可观测性排查配方,以及设计文档 Part 2 的修订,使其描述实际发布的内容。为什么需要它
daemon 每 5 秒采样自身的 RSS 与堆,却没有任何东西可以做除数,因此
/daemon/status里没有任何字段能说明某个数字是正常还是濒临致命。而且真正要紧的内存根本不在根进程里——每个 session 的 RSS 位于qwen --acp子进程中,而 daemon 从来只测量主 workspace 那一个。#8245 落地了分母(
limits.memory)。本 PR 把它变成读数,两边都覆盖。设计文档
docs/design/2026-07-31-daemon-capacity-model-and-memory-bounds.md的 Part 2。评审者测试计划
如何验证
1. 根进程压力读数,以及
source指明胜出的分母。level为normal/soft/hard/critical;ratio取rssRatio与heapRatio中较差者;source说明是哪个产生的。相信level之前先看source:unknown表示两个分母都无法测量,此时的normal是读数的缺失,而非健康的证据。2.
off报告全部数值,只去掉判定。 分别以两种模式启动并对比:在
--memory-pressure-mode off下,pressure块必须完整填充且结构一致,而daemon_memory_pressure这条 issue 必须缺席,顶层status汇总不变。一个连数值也一并压制的模式会让这个 flag 失去意义。3. 子进程 RSS 汇总,以及覆盖范围的翻转。
childRssCoverage现在必须是active_children(原为primary_only)。让children.rssBytes诚实的那个不变量:4.
sampled: 0绝非测得为零。 在打开两个 workspace 但只有一个在流式传输的情况下,children.rssBytes必须伴随sampled: 1对activeAcpChildren: 2——缺口无需刻意寻找即可见。5. 建模分区落在它所划分的池内。
8 GB 主机上预期
{"mode":"observe","maxConcurrentChildren":7,"perChildCeilingMb":526,"refusals":0};32 GB 上是25与614。使其成为总量约束(而非按派生分摊)的那个不变量在任何主机上都成立:6. 什么都没被应用——这是 Part 3 最关键的检查。 打开一个 session 让
qwen --acp子进程派生出来,然后在两种模式下分别读它的 argv:该值必须是主机推导的上限,并且在
--child-heap-mode observe与--child-heap-mode off下逐字节一致,尤其不能等于perChildCeilingMb。同时确认limits.memory.enforced仍是字面量false,以及强制模式是真的不存在而非只是默认关闭:qwen serve --child-heap-mode enforce # error: Invalid values: Argument: child-heap-mode, Given: "enforce", Choices: "off", "observe"7. 单元测试套件。
证据(前后对比)
N/A —— 无用户可见或 TUI 变更。可观察的差异是
/daemon/status的载荷:"runtime": { "memory": { - "childRssCoverage": "primary_only", + "childRssCoverage": "active_children", + "pressure": { "level": "normal", "ratio": 0.31, "source": "rss", ... }, + "children": { "rssBytes": 402653184, "sampled": 2, "oldestReadingAgeMs": 4120 }, "activeAcpChildren": 2 } }, "limits": { "memory": { "enforced": false, + "childHeap": { "mode": "observe", "maxConcurrentChildren": 7, "perChildCeilingMb": 526, "refusals": 0 }, "modeled": { "childPoolMb": 3687, ... } } }子进程 argv 在两个方向上都未改变——这正是 Part 3 的重点,上面第 6 步就是评审者确认它的方式。
测试平台
Linux 本地:
cli、sdk-typescript、acp-bridge三者的tsc。Math.max改成Math.min会挂 7 个;从 handler 里删掉该 flag 会挂 2 个;令sampled := activeAcpChildren会挂 2 个;去掉存活门控会挂 1 个;放宽陈旧窗口会挂掉那个 bridge 测试。normal,即便删掉门控该断言也照样成立。Part 3 部分:
child-heap-policy、process-registry、spawnChannel三个套件共 49 个 acp-bridge 测试,外加run-qwen-serve的 218 个。不变量maxConcurrentChildren × perChildCeilingMb ≤ childPoolMb在 2/8/32/256 GB 主机规格上均有断言;observe被断言使派生 argv 与「无策略工厂」逐字节一致;enforce被断言在 yargs 与 fast path 两处均被拒绝。针对已合并版本的两条评审意见在此修复,且均经变异验证:run-qwen-serve.test.ts中的状态断言在 head 上对四字段的 wire 形状是失败的;以及decide()运行在取消派生预留的try之外,导致调用方提供的策略一旦抛出就会泄漏预留令牌——把该改动回退即可复现expected 1 to be +0。macOS 与 Windows 交由 CI。唯一与平台相关的决策是刻意为之:RSS 取自各子进程的自上报而非
/proc,正是因为/proc仅限 Linux。环境(可选)
Linux 上的
npm run devdaemon;上述断言涉及单元测试与 e2e。风险与范围
level可能在两个方向上都不准——这恰恰是所有级别都映射为warning而绝不用error的原因,也是off保留数值却去掉判定的原因。第二个权衡是children.rssBytes同时既高估又低估(共享页被重复计算;MCP 子孙进程与 channel worker 缺失);文档如实说明而非将其呈现为进程树内存,并刻意不并入pressure.ratio。第三个属于 Part 3:发布一份分区会招致它支撑不了的解读——运维看到refusals: 0可能会认为强制是安全的。并非如此,原因见 Part 3,该警告现已写入 flag 帮助、三份运维文档和协议文档。那里接受的权衡是:发布一个不约束任何东西的模型——bug(serve): daemon authorises each ACP child 50% of host memory, never divided by child count #8182 的 400 GB 超额授权在强制模式连同其测量手段落地之前仍然敞开。off不发布任何分区:maxConcurrentChildren与perChildCeilingMb在该模式下均为null,与「池小到容不下一个子进程」所对应的0相区分。pressure与children在 SDK 镜像中均为可选,因为在它们之前就发布了runtime.memory的 daemon 会发送不含这些字段的块;childRssCoverage出于同样原因在那里是联合类型,而 daemon 自身的类型保持单一字面量。无 capability 标记:daemon_status已是基线,且两处编辑的门控契约只覆盖CONDITIONAL_SERVE_FEATURES。曾把childRssCoverage === 'primary_only'写死的客户端将看到'active_children'——feat(serve): resolve and report the daemon memory budget #8245 把该字段设计成单一字面量,正是为了这次过渡。来自 Part 3 的limits.memory.childHeap同样是新增字段,limits.memory.enforced仍为必需的字面量false:早前版本曾将其放宽为boolean,该放宽已回退,因此客户端契约没有变化。--child-heap-mode是默认observe的新 flag;enforce不是可接受的取值。关联 Issue
Refs #8051(Part 1–2)与 #8182(Part 3)。取代 #8462 与 #8508,两者均已合入本分支。
🤖 Generated with Claude Code