refactor(serve): model a per-child heap partition of the daemon budget - #8508
Conversation
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
@doudouOUC Thanks for continuing this series — the content here is thorough, but this gate is about shape, not substance: the body doesn't follow the PR template, so requesting changes on that before review proceeds.
What's missing:
## What this PR does/## Why it's needed— the existing## What/## Whycontent maps over as-is.## Reviewer Test Plan(### How to verify,### Evidence (Before & After),### Tested on) — the one that matters most here.## Verificationdescribes what you ran; this is the first behaviour-changing PR in the line, and reviewers need steps they can run themselves — e.g. how to watchlimits.memory.childHeap.refusalsclimb underobserve, and how to trip aChildHeapPoolExhaustedErrorspawn refusal underenforceon a small budget. #8245 earlier in this same series shows the level.## Risk & Scope— "Two things deliberately not done" and "The contract change" cover most of it; it just needs the template's three bullets (main risk/tradeoff, not validated / out of scope, breaking changes — thelimits.memory.enforcedliteral change belongs in that last one).## Linked Issues—Refs #8182is in prose; give it the section.- The Chinese
<details>translation.
Once the body is on the template, re-run triage (@qwen-code /triage) and it picks up from here.
中文说明
@doudouOUC 感谢继续推进这个系列——内容一如既往地扎实，但这一关卡的是格式而非内容：PR 正文没有遵循 PR 模板，所以在进入评审前先就此提出修改请求。
缺失的部分：
## What this PR does/## Why it's needed—— 现有的## What/## Why内容可以直接搬过去。## Reviewer Test Plan（### How to verify、### Evidence (Before & After)、### Tested on）—— 这是本 PR 最关键的一项。## Verification描述的是你跑过的内容；而这是该系列中第一个改变行为的 PR，评审者需要自己能执行的步骤——例如如何在observe模式下观察limits.memory.childHeap.refusals增长，以及如何在enforce模式、小预算下触发ChildHeapPoolExhaustedError拒绝派生。同系列早些时候的 #8245 是一个示范。## Risk & Scope—— “Two things deliberately not done” 和 “The contract change” 已覆盖大部分内容；只需按模板的三项整理（主要风险/权衡、未验证/超出范围、破坏性变更——limits.memory.enforced字面量的变化应写在最后一项）。## Linked Issues——Refs #8182目前散在正文里，请放进专门小节。- 中文
<details>翻译。
正文改为模板格式后，重新运行 triage（@qwen-code /triage）即可从这里继续。
— Qwen Code · qwen3.8-max
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the exact head c37affa. The first inline finding is a blocking correctness issue: enforce does not currently maintain the advertised aggregate child-pool bound. The other comments cover an inaccurate status path, missing wire-contract tests, and stale public documentation. Local build and 434 relevant unit tests passed, but none of those tests asserts the aggregate grant invariant. Please address the blocking finding before merging.
|
|
||
| return { | ||
| // Reported in both modes; only `enforce` lets the caller apply it. | ||
| ceilingMb: recommendedChildShareMb(budget, children), |
There was a problem hiding this comment.
[Critical] These per-spawn shares do not form an aggregate bound because only the new child receives P/n; the immutable ceilings already passed to live children are never reduced. The grants therefore accumulate as P + floor(P/2) + ... rather than staying at or below P. With 8 GB available, P is 3687 MB and the seven admitted children receive ceilings totaling 9557 MB. With 32 GB, P is 15360 MB and the repository maximum of 25 children receives 58608 MB in total without a refusal. This still authorizes more old-space than the host and contradicts the central enforce contract. Please use a fixed safe partition or grant/admission accounting that maintains sum(live ceilings) <= childPoolMb, and add a test for that invariant.
There was a problem hiding this comment.
Agreed, and fixed in c0dc50a7f — the arithmetic in this finding is correct, including both worked examples (9557 MB against a 3687 MB pool at seven children on 8 GB; 61355 MB against 15360 MB on 32 GB). The root cause is exactly as stated: V8 cannot lower a running child's ceiling, so per-spawn shares accumulate as P × H(n) instead of staying inside P.
The model is now a fixed partition rather than a share of the pool divided by whoever is live at that instant: every child would receive the same perChildCeilingMb, and maxConcurrentChildren is capped so maxConcurrentChildren × perChildCeilingMb ≤ childPoolMb holds by construction — no ledger, and no dependence on arrival order. That invariant is now asserted across 2/8/32/256 GB hosts.
| }), | ||
| getMetricsSeries: () => metricsRing.snapshot(), | ||
| getTotalSessionAdmissionSnapshot: totalSessionAdmission.snapshot, | ||
| getChildHeapPolicySnapshot: () => managedChildHeapPolicy?.snapshot(), |
There was a problem hiding this comment.
[P2] managedChildHeapPolicy is also created when deps.bridge is injected, but that bridge bypasses the channelFactory carrying this policy. This status callback can consequently report enforced: true while no child is being sized by the policy. Please publish this snapshot only when the daemon-created bridge actually owns the factory, or let the injected bridge supply its real policy state, with a regression test for the injected-bridge path.
| res.set('Retry-After', '5'); | ||
| res.status(503).json({ | ||
| error: err.message, | ||
| code: 'child_heap_pool_exhausted', |
There was a problem hiding this comment.
[Suggestion] Please add direct tests for this REST mapping and the ACP counterpart in dispatch.ts. The new contract includes 503, Retry-After, child_heap_pool_exhausted, and the data fields; the current spawn-policy tests cannot catch a transport-layer mapping regression. The existing error-response.test.ts and dispatch-error.test.ts are the natural coverage points.
| | `--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. Observed and reported under `limits.memory` in daemon status; it does not size any child process. 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 \| enforce` | `observe` | How the per-child heap share derived from the budget is used. `enforce` sizes each child by the live child count and refuses a spawn the pool cannot cover; `observe` computes both but applies neither. | |
There was a problem hiding this comment.
[Suggestion] The adjacent memory-budget row still says that the budget does not size any child, which is false in enforce mode. The same obsolete observation-only contract remains in the qwen serve help text, 20-quickstart-operations.md, qwen-serve.md, daemon/CLI type comments, and the TypeScript SDK memory comments. Please update these public descriptions together so operators and SDK users do not receive contradictory guidance.
Groundwork for #8182 step 2. Nothing calls any of this yet, so no child is sized differently and no spawn is refused. `ProcessRegistry.committedProcessCount` counts attached children plus reservations that have not attached. That is the figure admission has to key on: `reserve()` inserts its token synchronously before `spawn()`, so two racing spawns each see the other, while neither appears in `activeProcessCount` until its child attaches. A child leaves the count on exit rather than when `terminate()` starts, so a channel swap counts twice while the old process winds down — deliberate, since its memory is still resident. `getAcpMemoryArgs(explicitMb?)` takes an optional share that bypasses both the module cache and the raise-only guard. Both bypasses are load-bearing. The cache, because the share depends on how many children are live now rather than on the host. The guard, because a budget-derived share is normally *below* the daemon's own heap limit, so routing it through `targetMB > currentLimitMB` would drop the flag, silently restore the overcommit, and leave every test green — the trap #8182 calls out. The regression test asserts the flag survives 614 MB against a multi-GB runner, and mutation-checking it by reinstating the guard fails two tests. `createChildHeapPolicy` holds the mode, the budget, and the would-be refusal counter, and answers `decide(concurrentChildren)`. The refusal is derived from the unclamped quotient, not from `recommendedChildShareMb`, because that function clamps *up* to the 512 MB floor: past the point where the pool stops covering the count its answer saturates and can no longer distinguish "barely does not fit" from "wildly does not fit". `ChildHeapPoolExhaustedError` with both transport mappings — REST 503 with Retry-After, ACP `child_heap_pool_exhausted` — added together, since the two mappings are hand-written and drift silently otherwise. Refusing at spawn rather than at registration is the correction #8182 demands: registration allocates nothing, so this surfaces as "no new session in this workspace right now", which is true and retryable. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wires the primitives from the previous commit into the spawn path, behind `--child-heap-mode off | observe | enforce`, default `observe`. Under `enforce` a child's `--max-old-space-size` is a share of the child pool divided by the children concurrently committed at the moment it spawns — read from the shared ProcessRegistry after `reserve()`, so two racing spawns each see the other. When the pool cannot cover another child at the 512 MB floor the spawn is refused with ChildHeapPoolExhaustedError, which is what turns a per-child ceiling into an aggregate bound: concurrent children can never exceed pool/512. Keyed on concurrency, never on registrations. A dormant workspace has no child, so it costs nothing — the specific correction #8182 records against the withdrawn proposal, which would have shrunk a lone live child to 614 MB because of 24 idle registrations. Default `observe` computes the share and the admission decision and applies neither, counting the refusals that would have happened. The divisor has never been checked against a real multi-workspace deployment, and a non-zero count is how an operator learns enforcement would have broken them without being broken. It also catches the case worth worrying about: a channel swap counts the dying child alongside its replacement, so on a saturated pool enforcement could refuse a restart and leave that workspace with no child at all. Excluding terminating children would authorise real overcommit to dodge a hypothetical refusal, so the count reports it instead. Ceilings already granted are not revisited — V8 cannot lower them — so granted ceilings transiently exceed the pool. Acceptable: the flag is a ceiling, not a reservation, and a workspace with no live sessions has no child and picks up the current share on its next spawn. `limits.memory.enforced` stops being a required literal `false`. #8245 made it one so a client could never mistake that namespace for enforcement that had not shipped; it has now, so the field is a boolean derived from the mode — and stays `false` under `observe`, which applies nothing. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two sentences in the protocol doc described the memory section as unconditionally observational: "a required `enforced: false`", and "no child spawn argument derives from these values, and no request is refused on their basis". Both are false under `--child-heap-mode enforce`, so both are rewritten rather than left to rot — `enforced` is now documented as the boolean that answers exactly this, and the refusal is documented with its wire shape on both transports. Also documents `childHeap.refusals` as the calibration signal, since a would-be-refusal count is useless if operators do not know to read it before switching to `enforce`; the flag row in the three operator docs; and the design doc's Part 1, which listed applying a share as a compatibility risk without recording how that was resolved. The end-to-end test asserts the policy reaches a real booted daemon's status with `enforced: false` under the default mode — the wire type in that test is a hand-written mirror, so its `enforced: false` literal had to widen too, which is the check that caught the type not being widened everywhere. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`enforced` was only ever asserted false — the unit tests build no policy and the end-to-end daemon runs the default `observe` mode, so the branch that makes the field worth having was untested. Hardcoding it back to `false` passed everything. Also pins `childHeap: null` as distinct from a policy in `off` mode: the first says no policy exists (direct-embed, or the bootstrap window before the runtime is built), the second says one exists and computes nothing. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review was right that the previous design did not deliver the aggregate bound it claimed. 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 x H(n). Reproduced exactly — 9557 MB authorised against a 3687 MB pool at seven children on an 8 GB host, and 61355 MB against 15360 MB at the limit on 32 GB. That is 2.6x and 4x the pool, which is what the policy exists to prevent. Grant accounting alone does not fix it: the first child would take the whole pool and the second would be refused immediately. Keeping the invariant requires early children not to receive the whole pool, so the ceiling is now a fixed partition — childPoolMb / maxConcurrentChildren, constant for every child, with maxConcurrentChildren itself derived from the pool and capped at MAX_DAEMON_WORKSPACES. The sum is then n x ceiling <= pool by construction, with no ledger of outstanding grants and no dependence on arrival order. Tested as an invariant across four host sizes: fill the daemon to its admission limit and the authorised total still fits. The cost is deliberate and now documented rather than hidden: a lone workspace on a 32 GB host gets 614 MB rather than the pool, because any child may still be running when the house fills. An 8 GB host admits seven concurrent children at 526 MB each. Also from review: - The policy is no longer built for an injected `deps.bridge`. That bridge carries its own channel and never reaches the factory the policy rides on, so status could report `enforced: true` while nothing was being sized. - Both transport mappings now have direct tests. They are hand-written beside each other and drift silently; the spawn-policy tests cannot catch a wire regression. - Swept the "does not size any child" claim, which enforce makes false, out of the CLI help text, ServeOptions docs, the two operator tables, and the e2e header comment. The 17-configuration table realigns wholesale because that cell was its widest — whitespace only. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c37affa to
c0dc50a
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）为单个提交。 |
|
All four addressed — [Critical] The aggregate bound — you're right, and my claim was wrong. Reproduced your arithmetic exactly. Sizing each child by the count live at its spawn bounds the child count but not the memory, because V8 cannot lower a running child's ceiling — grants accumulate as
(The 8 GB figure matches yours to the megabyte.) I conflated two things in the PR description: I bounded the count and called it an aggregate bound. The design doc did say granted ceilings "transiently" exceed the pool, but that word was wrong too — for a monotonically growing set it is the steady state. Worth recording why plain grant accounting does not fix it: the first child takes the whole pool, so the second sees zero remaining and is refused immediately. Holding the invariant requires early children not to receive the whole pool. So the ceiling is now a fixed partition — The cost is real and now stated rather than buried: a lone workspace on a 32 GB host gets 614 MB instead of the pool, because any child may still be running when the house fills. An 8 GB host admits 7 concurrent children at 526 MB each — pinned in a test, since that is the number an operator plans against. [P2] Injected bridge — fixed. The policy is no longer constructed when [Suggestion] Transport mappings — both now have direct tests in [Suggestion] Obsolete observation-only text — swept: the CLI help text, the |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-reviewed the full 30-file diff at exact head c0dc50a against stacked base 361413a. The fixed partition repairs the previous aggregate-bound defect for normal positive pools, and the injected-bridge and REST/ACP mapping fixes are sound. However, two new blocking correctness issues remain: a zero child pool produces an ineffective zero V8 ceiling, and the observe rollout signal cannot detect whether workloads fit the much smaller fixed ceiling. A third comment covers the now-inaccurate refusal contract, and a fourth covers stale public documentation and PR metadata. 119 targeted tests passed; the missing boundary and calibration assertions are the gaps described inline.
| // The cost is real and deliberate: a lone workspace on a 32 GB host gets | ||
| // 614 MB rather than the whole pool. Every admitted child is sized for a | ||
| // full house, because any child may still be running when the house fills. | ||
| const maxConcurrentChildren = Math.max( |
There was a problem hiding this comment.
[Critical] Forcing maxConcurrentChildren to at least one breaks the aggregate invariant when the resolved child pool is zero. With 512 MB available, the derived budget is 256 MB, the root reserve consumes all 256 MB, and childPoolMb is 0. This code then computes maxConcurrentChildren=1 and perChildCeilingMb=0, so decide(1) admits the child and the spawn path passes --max-old-space-size=0. On the required Node 22 runtime, that value means the default heap rather than a zero ceiling (4144 MB on Node 22.22.3), so enforce authorizes gigabytes against a zero pool. At 1024 MB available it similarly admits a 256 MB child despite the documented 512 MB minimum. Please represent zero/sub-floor capacity as no admissible child or reject enforce for an insufficient budget, and cover both the zero-pool argv behavior and the sub-floor boundary.
There was a problem hiding this comment.
Agreed, and fixed in 25e69d2c0. The clamp was mine and the consequence is worse than the finding even states: --max-old-space-size=0 is not a zero ceiling, it is V8's default heap — roughly 4 GB — so forcing "at least one child" against a pool of 0 authorised gigabytes rather than nothing.
Math.max(1, …) is gone. A pool that cannot cover one child at MIN_CHILD_HEAP_MB now reports maxConcurrentChildren: 0 and perChildCeilingMb: **null** — null rather than 0, precisely so no caller can pass the zero through to V8 and get the default heap.
Worth flagging for the next round: the test that should have caught this had enshrined the bug as expected behaviour — it was named "always admits at least one child, however small the pool". It is inverted now, and the 512 MB case (256 MB budget, root reserve consuming all of it, pool 0) is pinned directly.
| childHeap: { | ||
| mode: 'off' | 'observe' | 'enforce'; | ||
| /** | ||
| * Spawns refused, or — under `observe` — that would have been refused. |
There was a problem hiding this comment.
[Critical] This counter cannot establish that enabling enforce is safe after switching to a fixed partition. On a 32 GB host, observe leaves a single child on the 16384 MB legacy ceiling and records zero refusals, while enforce immediately gives that same child only 614 MB. A workload using more than 614 MB old space is therefore healthy with refusals=0 in observe and OOMs after the switch. The status mapping also drops the already-computed perChildCeilingMb and maxConcurrentChildren, and no heap-used/high-water value is compared with the proposed cap. Zero refusals proves only that the spawn count stayed within the admission limit. Please stop presenting it as an enforcement-safety signal; at minimum publish the proposed fixed ceiling and document that workload validation is still required, or add observation that can detect ceiling incompatibility.
There was a problem hiding this comment.
Agreed, and this one changed the shape of the PR: enforce is removed entirely in 25e69d2c0.
The finding is right that the counter measures the wrong thing. While observing, children run on the host-derived ceiling (16384 MB on a 32 GB host), so a workload needing more old space than perChildCeilingMb is perfectly healthy with refusals: 0 and OOMs the instant the partition is applied. refusals is admission pressure, not ceiling adequacy — and enforcing on a signal that cannot answer the question it is being read for is how a healthy daemon gets switched into an OOM loop.
Rather than keep enforce behind a warning, it is gone: ChildHeapMode is now 'off' | 'observe', and the machinery that existed only to apply the partition went with it instead of shipping unreachable — getAcpMemoryArgs(explicitMb?), ChildHeapPoolExhaustedError and both transport mappings, and limits.memory.enforced, which reverts to the required literal false. Net −310 lines.
The enforcing mode will ship with the measurement that justifies it: peak old-space per child (not rss, which is what the child self-reports today, and not heapUsed, which includes the new generation), compared against perChildCeilingMb. That measurement has to be taken inside the child — an outside sampler misses GC-time peaks — so it is its own change. Recorded in #8182 and #8051.
What operators get instead is perChildCeilingMb and maxConcurrentChildren published, so the partition can be judged against a known workload rather than inferred from a counter that cannot judge it. Every claim that refusals: 0 implies enforcement is safe has been removed from the flag help, the three operator docs, the protocol doc, and the design doc.
| minChildHeapMb: number, | ||
| ) { | ||
| super( | ||
| `Daemon child heap pool (${childPoolMb} MB) cannot cover ` + |
There was a problem hiding this comment.
[P2] This error describes a condition the fixed-partition policy no longer tests. On a 32 GB host the partition is 25 children at 614 MB; reservation 26 is refused, but 26 * 512 MB is 13312 MB, which is below the 15360 MB pool. The response therefore says the pool cannot cover 26 children at the 512 MB minimum when it demonstrably can, and raising the budget may not help because the admission limit is also capped at the repository workspace maximum. Please report the actual fixed-partition limit and per-child ceiling, and update the REST/ACP payload tests accordingly.
| | `--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`; sizes each ACP child only under `--child-heap-mode enforce`. 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 \| enforce` | `observe` | How the per-child heap share derived from the budget is used. `enforce` sizes each child by the live child count and refuses a spawn the pool cannot cover; `observe` computes both but applies neither. | |
There was a problem hiding this comment.
[Suggestion] This still documents the superseded pool/live-child algorithm; enforce now gives every child one fixed partition independent of the live count. The same old algorithm or observation-only contract remains in 20-quickstart-operations.md, qwen-serve-protocol.md, docs/users/qwen-serve.md, ServeOptions and daemon-status comments, and the TypeScript SDK. The PR title and body also still describe the pre-fix dynamic policy, including the claim that existing grants exceed the pool. Please update the full public contract and PR description together; the existing PR-template Changes Requested is also still outstanding.
Review established that the refusal counter cannot tell an operator whether enforcement is safe, and that is the ground the enforcing mode stood on. 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 with zero refusals and OOMs the moment a 614 MB partition is applied. The counter measures admission pressure, not ceiling adequacy. Rather than ship a switch with no safe way to decide when to turn it on, `enforce` is removed. `--child-heap-mode` is `off | observe`, and the mode that would apply the partition arrives with the measurement that justifies it: peak old-space per child, compared against the modeled ceiling. That is a real measurement chain — the child reports rss and cpu today, and `--max-old-space-size` bounds old space specifically, so neither rss nor heapUsed answers the question. With nothing applying the partition, the machinery that existed only to apply it goes too rather than shipping unreachable: `getAcpMemoryArgs(explicitMb?)`, `ChildHeapPoolExhaustedError` and both transport mappings, and `limits.memory.enforced` reverts to the required literal `false` it was before. The spawn path is untouched again; the factory asks the policy what it would decide purely so the count is real. Also fixes the zero-pool defect review found, which the removed clamp caused: forcing at least one admissible child on a 512 MB host — where the root reserve consumes the whole 256 MB budget — produced a ceiling of 0, and `--max-old-space-size=0` is V8's *default* heap, not a zero ceiling. A pool that cannot cover one child at the floor now reports `maxConcurrentChildren: 0` and `perChildCeilingMb: null`, and the test that enshrined the old behaviour is inverted. Status now publishes `maxConcurrentChildren` and `perChildCeilingMb`, so an operator can judge the partition against their own workload — the substitute for a counter that cannot judge it for them. Every claim that a zero refusal count means the partition is safe to apply is removed from the flag help, the operator docs, the protocol doc, and the design doc. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
doudouOUC
changed the title
|
Took all four, and the second one changed the shape of the PR — [Critical] The refusal counter cannot establish enforcement safety. You're right, and this is the one that mattered most. While observing, children sit on the host-derived ceiling — 16384 MB on a 32 GB host — so a workload needing 2 GB of old space reports That was the ground Since nothing applies the partition now, everything that existed only to apply it went too rather than shipping unreachable: [Critical] Zero/sub-floor pool. Confirmed and fixed — and it was my clamp that caused it. Forcing at least one admissible child meant a 512 MB host, where the root reserve consumes the entire 256 MB budget, modeled a ceiling of 0; [P2] The error message described an untested condition. Correct: 26 × 512 = 13312 < 15360, so the pool demonstrably could cover them and the real limit was the partition. Moot now that the error is gone, and the enforcing PR will carry a message stating the actual partition limit and ceiling. [Suggestion] Public contract. Swept: flag help, On the status mapping dropping the computed figures — fixed, and it turned out to be the substitute for the counter you took apart: |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed the updated modeling-only revision at 25e69d2c. The previous enforcement and zero-pool findings are fixed or made moot. I found three reproducible correctness issues plus one stale public contract comment; details are inline.
|
|
||
| return { | ||
| decide(concurrentChildren) { | ||
| if (mode === 'off') return { refuse: false }; |
There was a problem hiding this comment.
[P2] off only skips incrementing refusals here; maxConcurrentChildren and perChildCeilingMb were already computed, and snapshot() still returns them. Because runQwenServe constructs this policy for childHeapMode: 'off' and daemon status publishes the snapshot, an 8 GiB host still reports a 7-child / 526 MB partition even though the CLI help, ServeOptions, and user docs all promise that off models nothing. Please make the off path omit/disable the partition (and assert the full snapshot/status shape), or change the public contract consistently if static modeling is intended to remain enabled.
There was a problem hiding this comment.
Confirmed — snapshot() returns maxConcurrentChildren and perChildCeilingMb unconditionally, so a daemon run with --child-heap-mode off still publishes a partition it was told not to model. The flag's own documentation says "off — do not model it", so the code contradicts its stated contract.
Not fixed in e95dda03e, because the honest fix is a wire decision rather than a local one, and the finding points at something larger: now that enforce is gone, off and observe differ only in whether refusals increments. Both publish the same model and neither applies anything.
Three ways out, none obviously right:
- Do not construct the policy when the mode is
off, so status omitschildHeapentirely. Simplest, but a client can no longer distinguishofffrom a daemon predating the field. - Widen
maxConcurrentChildrentonumber | nulland report both figures asnullunderoff, keepingmode: 'off'visible. Most accurate, but it changes the wire type and the SDK mirror. Reporting0/nullinstead would collide with the zero-pool state, which this policy deliberately distinguishes. - Change the documentation to say
offonly stops the counting, and let the model stay published.
Leaning toward 2, since the same tradeoff already produced perChildCeilingMb: number | null rather than a 0 that V8 would read as its default heap — the field saying something true was worth the type change there too. Flagging it for a decision rather than picking unilaterally on already-merged code; happy to take direction.
There was a problem hiding this comment.
Fixed in 4eb1a85f2 (on agent/daemon-memory-observe, where this code now lives).
Both figures are null under off. Widening maxConcurrentChildren to number | null was the part worth deciding rather than assuming: 0 was available but already means something else — the computed answer for a pool too small to host one child at the 512 MB floor — so reusing it would tell an operator who disabled the model that their host cannot run anything. Same reasoning that made perChildCeilingMb a null instead of a 0 earlier in this PR.
That leaves three states a client can now distinguish, each with a test:
| state | childHeap |
|---|---|
| no policy built (direct-embed, bootstrap window) | null |
| policy exists, models nothing | { mode: 'off', maxConcurrentChildren: null, perChildCeilingMb: null, refusals: 0 } |
| policy models the partition | { mode: 'observe', maxConcurrentChildren: 7, perChildCeilingMb: 526, … } |
You also found a test whose name outran its assertions: models nothing at all when off checked only refusals, which is why this shipped. It now asserts the figures, and a sibling test pins 7 / 526 under observe on the same budget — so nulling them unconditionally fails the second test rather than quietly satisfying the first. Mutation-verified both ways (modeled = true fails the off test; modeled = false fails four observe tests).
Docs updated: the protocol doc's childHeap description, 17-configuration.md, 20-quickstart-operations.md, and qwen-serve.md.
| // Observation only: the policy is asked what it *would* decide so the | ||
| // refusal count is real, but nothing here acts on the answer — no derived | ||
| // ceiling reaches the child and no spawn is refused. | ||
| options.childHeapPolicy?.decide(processRegistry.committedProcessCount); |
There was a problem hiding this comment.
[P2] This call is outside the try that cancels the reservation. A supplied ChildHeapPolicy is part of the public createSpawnChannelFactory options, so if decide() throws, the factory rejects but the reservation created on line 156 remains forever; reproduced with committedProcessCount === 1 after the rejection. That permanently inflates every later observation (and would become a false admission refusal if this seam is later enforced). Please include policy evaluation in the reservation-cancellation failure path and add the throwing-policy regression test.
There was a problem hiding this comment.
Agreed and fixed in e95dda03e. Both decide() and getAcpMemoryArgs() now run inside the try that cancels the reservation.
The reachability argument is the part worth keeping: our own decide() is pure arithmetic and cannot throw, so this is not a live defect today — but childHeapPolicy is a public createSpawnChannelFactory option, which makes decide() caller code. A supplied implementation that throws leaks the token for the process lifetime, and every later spawn then sees an inflated committedProcessCount. That is worth closing on a public extension point regardless of what our own implementation happens to do.
The regression test is mutation-verified rather than merely green — reverting the move reproduces your figure exactly:
× releases the reservation when a supplied policy throws
→ expected 1 to be +0
| // `enforced` has to stay false or the field means "the feature exists" | ||
| // rather than "children are being sized by this". | ||
| expect(memory?.enforced).toBe(false); | ||
| expect(memory?.childHeap).toEqual({ mode: 'observe', refusals: 0 }); |
There was a problem hiding this comment.
[P1] This exact equality assertion fails on the current head because the production response intentionally also contains maxConcurrentChildren: 7 and perChildCeilingMb: 526. Running npx vitest run src/serve/run-qwen-serve.test.ts --disableConsoleIntercept --coverage.enabled=false gives 217 passed / 1 failed at this line. Please assert the full published shape (or use toMatchObject if extras are intentionally irrelevant) so the updated suite is green and the new wire fields are actually covered by this end-to-end status test.
There was a problem hiding this comment.
Confirmed and fixed in e95dda03e — reproduced exactly as reported:
AssertionError: expected { mode: 'observe', …(3) } to deeply equal { mode: 'observe', refusals: +0 }
+ "maxConcurrentChildren": 6,
+ "perChildCeilingMb": 524,
Tests 1 failed | 217 passed (218)
(6 / 524 rather than 7 / 526 here — this suite boots a real daemon, so the pool follows the machine, which is itself part of the fix.)
The local type restating the wire shape was short the same two fields, so nothing flagged the drift. Both are filled in now.
The assertion stays toEqual rather than relaxing to toMatchObject, so an unannounced field added to the wire still fails here; only the two host-derived figures get matchers. What they have to satisfy is pinned separately instead of by value:
expect(
(memory?.childHeap?.maxConcurrentChildren ?? 0) *
(memory?.childHeap?.perChildCeilingMb ?? 0),
).toBeLessThanOrEqual(memory?.modeled.childPoolMb ?? 0);That product is the reason a fixed ceiling bounds anything at all, so it is worth an assertion in its own right — more than the two literals were.
218 passed / 0 failed on this branch.
| * plus every `qwen --acp` child it spawns. When unset, derived as half of | ||
| * the cgroup-constrained or host memory. Currently observed and reported | ||
| * only; it does not yet size any child. | ||
| * only until `childHeapMode` is `enforce`, which sizes every ACP child from |
There was a problem hiding this comment.
[Suggestion] This public option comment still promises a future childHeapMode: 'enforce' that sizes/refuses children, but the following union has no such mode and this revision explicitly removes it until a separate old-space measurement exists. Please restore the observation-only description here; the newly added bootstrap/policy comments in run-qwen-serve.ts and the childHeapPolicy option comment in spawnChannel.ts also retain claims about sizing/enforced state from the removed implementation and should be updated in the same cleanup.
There was a problem hiding this comment.
Agreed and fixed in e95dda03e. The comment promised a childHeapMode: 'enforce' that sizes children and refuses spawns; no such mode exists after this revision. It now states the observation-only contract and says what applying the partition is waiting on:
Observed and reported only. No child is sized from it and no spawn is refused on its basis:
childHeapMode: 'observe'models a partition of it and publishes the model, but there is no mode that applies one. Sizing children arrives with the peak old-space measurement that can tell an operator beforehand whether their workload fits the partition.
|
Body is now on the template: One correction to the triage note, since it changes what the test plan should contain: the review read the pre-revision body.
Step 2 is the one worth a reviewer's time: child argv must be byte-identical under @qwen-code /triage |
8e4b6f8
into
agent/daemon-memory-observe
Three findings review raised against #8508 after the partition became observation-only, all still live on this branch now that it has merged. The status assertion in `run-qwen-serve.test.ts` failed on head: it used `toEqual` against `{ mode, refusals }` while the wire also carries `maxConcurrentChildren` and `perChildCeilingMb`, so the suite was red at 217 passed / 1 failed. The local type restating the wire shape was short the same two fields. Both are filled in, and the assertion stays `toEqual` so an unannounced field still fails it — the two derived figures get matchers because this suite boots a real daemon and the pool follows the machine. What they have to satisfy is now pinned separately: a fixed ceiling times the number admitted must fit inside the pool it partitions, which is the whole reason the partition bounds anything. `decide()` and `getAcpMemoryArgs()` ran between `reserve()` and the `try` that cancels the reservation. `childHeapPolicy` is a public `createSpawnChannelFactory` option, so `decide()` is caller code and may throw; the spawn then rejected with the token held for the process lifetime, inflating `committedProcessCount` for every later spawn. Both calls move inside the `try`. The regression test is mutation-verified — reverting the move gives `expected 1 to be +0`. `ServeOptions.memoryBudgetMb` still promised a `childHeapMode: 'enforce'` that sizes children and refuses spawns. No such mode exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#8508) * feat(serve): add the child-heap admission primitives, unwired Groundwork for #8182 step 2. Nothing calls any of this yet, so no child is sized differently and no spawn is refused. `ProcessRegistry.committedProcessCount` counts attached children plus reservations that have not attached. That is the figure admission has to key on: `reserve()` inserts its token synchronously before `spawn()`, so two racing spawns each see the other, while neither appears in `activeProcessCount` until its child attaches. A child leaves the count on exit rather than when `terminate()` starts, so a channel swap counts twice while the old process winds down — deliberate, since its memory is still resident. `getAcpMemoryArgs(explicitMb?)` takes an optional share that bypasses both the module cache and the raise-only guard. Both bypasses are load-bearing. The cache, because the share depends on how many children are live now rather than on the host. The guard, because a budget-derived share is normally *below* the daemon's own heap limit, so routing it through `targetMB > currentLimitMB` would drop the flag, silently restore the overcommit, and leave every test green — the trap against a multi-GB runner, and mutation-checking it by reinstating the guard fails two tests. `createChildHeapPolicy` holds the mode, the budget, and the would-be refusal counter, and answers `decide(concurrentChildren)`. The refusal is derived from the unclamped quotient, not from `recommendedChildShareMb`, because that function clamps *up* to the 512 MB floor: past the point where the pool stops covering the count its answer saturates and can no longer distinguish "barely does not fit" from "wildly does not fit". `ChildHeapPoolExhaustedError` with both transport mappings — REST 503 with Retry-After, ACP `child_heap_pool_exhausted` — added together, since the two mappings are hand-written and drift silently otherwise. Refusing at spawn rather than at registration is the correction #8182 demands: registration allocates nothing, so this surfaces as "no new session in this workspace right now", which is true and retryable. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(serve): size each ACP child by concurrently live children Wires the primitives from the previous commit into the spawn path, behind `--child-heap-mode off | observe | enforce`, default `observe`. Under `enforce` a child's `--max-old-space-size` is a share of the child pool divided by the children concurrently committed at the moment it spawns — read from the shared ProcessRegistry after `reserve()`, so two racing spawns each see the other. When the pool cannot cover another child at the 512 MB floor the spawn is refused with ChildHeapPoolExhaustedError, which is what turns a per-child ceiling into an aggregate bound: concurrent children can never exceed pool/512. Keyed on concurrency, never on registrations. A dormant workspace has no child, so it costs nothing — the specific correction #8182 records against the withdrawn proposal, which would have shrunk a lone live child to 614 MB because of 24 idle registrations. Default `observe` computes the share and the admission decision and applies neither, counting the refusals that would have happened. The divisor has never been checked against a real multi-workspace deployment, and a non-zero count is how an operator learns enforcement would have broken them without being broken. It also catches the case worth worrying about: a channel swap counts the dying child alongside its replacement, so on a saturated pool enforcement could refuse a restart and leave that workspace with no child at all. Excluding terminating children would authorise real overcommit to dodge a hypothetical refusal, so the count reports it instead. Ceilings already granted are not revisited — V8 cannot lower them — so granted ceilings transiently exceed the pool. Acceptable: the flag is a ceiling, not a reservation, and a workspace with no live sessions has no child and picks up the current share on its next spawn. `limits.memory.enforced` stops being a required literal `false`. #8245 made it one so a client could never mistake that namespace for enforcement that had not shipped; it has now, so the field is a boolean derived from the mode — and stays `false` under `observe`, which applies nothing. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(serve): correct the claims child-heap enforcement makes false Two sentences in the protocol doc described the memory section as unconditionally observational: "a required `enforced: false`", and "no child spawn argument derives from these values, and no request is refused on their basis". Both are false under `--child-heap-mode enforce`, so both are rewritten rather than left to rot — `enforced` is now documented as the boolean that answers exactly this, and the refusal is documented with its wire shape on both transports. Also documents `childHeap.refusals` as the calibration signal, since a would-be-refusal count is useless if operators do not know to read it before switching to `enforce`; the flag row in the three operator docs; and the design doc's Part 1, which listed applying a share as a compatibility risk without recording how that was resolved. The end-to-end test asserts the policy reaches a real booted daemon's status with `enforced: false` under the default mode — the wire type in that test is a hand-written mirror, so its `enforced: false` literal had to widen too, which is the check that caught the type not being widened everywhere. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(serve): cover both branches of the enforced tripwire `enforced` was only ever asserted false — the unit tests build no policy and the end-to-end daemon runs the default `observe` mode, so the branch that makes the field worth having was untested. Hardcoding it back to `false` passed everything. Also pins `childHeap: null` as distinct from a policy in `off` mode: the first says no policy exists (direct-embed, or the bootstrap window before the runtime is built), the second says one exists and computes nothing. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): partition the child pool so granted ceilings stay inside it Review was right that the previous design did not deliver the aggregate bound it claimed. 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 x H(n). Reproduced exactly — 9557 MB authorised against a 3687 MB pool at seven children on an 8 GB host, and 61355 MB against 15360 MB at the limit on 32 GB. That is 2.6x and 4x the pool, which is what the policy exists to prevent. Grant accounting alone does not fix it: the first child would take the whole pool and the second would be refused immediately. Keeping the invariant requires early children not to receive the whole pool, so the ceiling is now a fixed partition — childPoolMb / maxConcurrentChildren, constant for every child, with maxConcurrentChildren itself derived from the pool and capped at MAX_DAEMON_WORKSPACES. The sum is then n x ceiling <= pool by construction, with no ledger of outstanding grants and no dependence on arrival order. Tested as an invariant across four host sizes: fill the daemon to its admission limit and the authorised total still fits. The cost is deliberate and now documented rather than hidden: a lone workspace on a 32 GB host gets 614 MB rather than the pool, because any child may still be running when the house fills. An 8 GB host admits seven concurrent children at 526 MB each. Also from review: - The policy is no longer built for an injected `deps.bridge`. That bridge carries its own channel and never reaches the factory the policy rides on, so status could report `enforced: true` while nothing was being sized. - Both transport mappings now have direct tests. They are hand-written beside each other and drift silently; the spawn-policy tests cannot catch a wire regression. - Swept the "does not size any child" claim, which enforce makes false, out of the CLI help text, ServeOptions docs, the two operator tables, and the e2e header comment. The 17-configuration table realigns wholesale because that cell was its widest — whitespace only. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(serve): model the child heap partition, defer applying it Review established that the refusal counter cannot tell an operator whether enforcement is safe, and that is the ground the enforcing mode stood on. 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 with zero refusals and OOMs the moment a 614 MB partition is applied. The counter measures admission pressure, not ceiling adequacy. Rather than ship a switch with no safe way to decide when to turn it on, `enforce` is removed. `--child-heap-mode` is `off | observe`, and the mode that would apply the partition arrives with the measurement that justifies it: peak old-space per child, compared against the modeled ceiling. That is a real measurement chain — the child reports rss and cpu today, and `--max-old-space-size` bounds old space specifically, so neither rss nor heapUsed answers the question. With nothing applying the partition, the machinery that existed only to apply it goes too rather than shipping unreachable: `getAcpMemoryArgs(explicitMb?)`, `ChildHeapPoolExhaustedError` and both transport mappings, and `limits.memory.enforced` reverts to the required literal `false` it was before. The spawn path is untouched again; the factory asks the policy what it would decide purely so the count is real. Also fixes the zero-pool defect review found, which the removed clamp caused: forcing at least one admissible child on a 512 MB host — where the root reserve consumes the whole 256 MB budget — produced a ceiling of 0, and `--max-old-space-size=0` is V8's *default* heap, not a zero ceiling. A pool that cannot cover one child at the floor now reports `maxConcurrentChildren: 0` and `perChildCeilingMb: null`, and the test that enshrined the old behaviour is inverted. Status now publishes `maxConcurrentChildren` and `perChildCeilingMb`, so an operator can judge the partition against their own workload — the substitute for a counter that cannot judge it for them. Every claim that a zero refusal count means the partition is safe to apply is removed from the flag help, the operator docs, the protocol doc, and the design doc. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Three findings review raised against #8508 after the partition became observation-only, all still live on this branch now that it has merged. The status assertion in `run-qwen-serve.test.ts` failed on head: it used `toEqual` against `{ mode, refusals }` while the wire also carries `maxConcurrentChildren` and `perChildCeilingMb`, so the suite was red at 217 passed / 1 failed. The local type restating the wire shape was short the same two fields. Both are filled in, and the assertion stays `toEqual` so an unannounced field still fails it — the two derived figures get matchers because this suite boots a real daemon and the pool follows the machine. What they have to satisfy is now pinned separately: a fixed ceiling times the number admitted must fit inside the pool it partitions, which is the whole reason the partition bounds anything. `decide()` and `getAcpMemoryArgs()` ran between `reserve()` and the `try` that cancels the reservation. `childHeapPolicy` is a public `createSpawnChannelFactory` option, so `decide()` is caller code and may throw; the spawn then rejected with the token held for the process lifetime, inflating `committedProcessCount` for every later spawn. Both calls move inside the `try`. The regression test is mutation-verified — reverting the move gives `expected 1 to be +0`. `ServeOptions.memoryBudgetMb` still promised a `childHeapMode: 'enforce'` that sizes children and refuses spawns. No such mode exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#8508) * feat(serve): add the child-heap admission primitives, unwired Groundwork for #8182 step 2. Nothing calls any of this yet, so no child is sized differently and no spawn is refused. `ProcessRegistry.committedProcessCount` counts attached children plus reservations that have not attached. That is the figure admission has to key on: `reserve()` inserts its token synchronously before `spawn()`, so two racing spawns each see the other, while neither appears in `activeProcessCount` until its child attaches. A child leaves the count on exit rather than when `terminate()` starts, so a channel swap counts twice while the old process winds down — deliberate, since its memory is still resident. `getAcpMemoryArgs(explicitMb?)` takes an optional share that bypasses both the module cache and the raise-only guard. Both bypasses are load-bearing. The cache, because the share depends on how many children are live now rather than on the host. The guard, because a budget-derived share is normally *below* the daemon's own heap limit, so routing it through `targetMB > currentLimitMB` would drop the flag, silently restore the overcommit, and leave every test green — the trap against a multi-GB runner, and mutation-checking it by reinstating the guard fails two tests. `createChildHeapPolicy` holds the mode, the budget, and the would-be refusal counter, and answers `decide(concurrentChildren)`. The refusal is derived from the unclamped quotient, not from `recommendedChildShareMb`, because that function clamps *up* to the 512 MB floor: past the point where the pool stops covering the count its answer saturates and can no longer distinguish "barely does not fit" from "wildly does not fit". `ChildHeapPoolExhaustedError` with both transport mappings — REST 503 with Retry-After, ACP `child_heap_pool_exhausted` — added together, since the two mappings are hand-written and drift silently otherwise. Refusing at spawn rather than at registration is the correction #8182 demands: registration allocates nothing, so this surfaces as "no new session in this workspace right now", which is true and retryable. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(serve): size each ACP child by concurrently live children Wires the primitives from the previous commit into the spawn path, behind `--child-heap-mode off | observe | enforce`, default `observe`. Under `enforce` a child's `--max-old-space-size` is a share of the child pool divided by the children concurrently committed at the moment it spawns — read from the shared ProcessRegistry after `reserve()`, so two racing spawns each see the other. When the pool cannot cover another child at the 512 MB floor the spawn is refused with ChildHeapPoolExhaustedError, which is what turns a per-child ceiling into an aggregate bound: concurrent children can never exceed pool/512. Keyed on concurrency, never on registrations. A dormant workspace has no child, so it costs nothing — the specific correction #8182 records against the withdrawn proposal, which would have shrunk a lone live child to 614 MB because of 24 idle registrations. Default `observe` computes the share and the admission decision and applies neither, counting the refusals that would have happened. The divisor has never been checked against a real multi-workspace deployment, and a non-zero count is how an operator learns enforcement would have broken them without being broken. It also catches the case worth worrying about: a channel swap counts the dying child alongside its replacement, so on a saturated pool enforcement could refuse a restart and leave that workspace with no child at all. Excluding terminating children would authorise real overcommit to dodge a hypothetical refusal, so the count reports it instead. Ceilings already granted are not revisited — V8 cannot lower them — so granted ceilings transiently exceed the pool. Acceptable: the flag is a ceiling, not a reservation, and a workspace with no live sessions has no child and picks up the current share on its next spawn. `limits.memory.enforced` stops being a required literal `false`. #8245 made it one so a client could never mistake that namespace for enforcement that had not shipped; it has now, so the field is a boolean derived from the mode — and stays `false` under `observe`, which applies nothing. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(serve): correct the claims child-heap enforcement makes false Two sentences in the protocol doc described the memory section as unconditionally observational: "a required `enforced: false`", and "no child spawn argument derives from these values, and no request is refused on their basis". Both are false under `--child-heap-mode enforce`, so both are rewritten rather than left to rot — `enforced` is now documented as the boolean that answers exactly this, and the refusal is documented with its wire shape on both transports. Also documents `childHeap.refusals` as the calibration signal, since a would-be-refusal count is useless if operators do not know to read it before switching to `enforce`; the flag row in the three operator docs; and the design doc's Part 1, which listed applying a share as a compatibility risk without recording how that was resolved. The end-to-end test asserts the policy reaches a real booted daemon's status with `enforced: false` under the default mode — the wire type in that test is a hand-written mirror, so its `enforced: false` literal had to widen too, which is the check that caught the type not being widened everywhere. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(serve): cover both branches of the enforced tripwire `enforced` was only ever asserted false — the unit tests build no policy and the end-to-end daemon runs the default `observe` mode, so the branch that makes the field worth having was untested. Hardcoding it back to `false` passed everything. Also pins `childHeap: null` as distinct from a policy in `off` mode: the first says no policy exists (direct-embed, or the bootstrap window before the runtime is built), the second says one exists and computes nothing. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): partition the child pool so granted ceilings stay inside it Review was right that the previous design did not deliver the aggregate bound it claimed. 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 x H(n). Reproduced exactly — 9557 MB authorised against a 3687 MB pool at seven children on an 8 GB host, and 61355 MB against 15360 MB at the limit on 32 GB. That is 2.6x and 4x the pool, which is what the policy exists to prevent. Grant accounting alone does not fix it: the first child would take the whole pool and the second would be refused immediately. Keeping the invariant requires early children not to receive the whole pool, so the ceiling is now a fixed partition — childPoolMb / maxConcurrentChildren, constant for every child, with maxConcurrentChildren itself derived from the pool and capped at MAX_DAEMON_WORKSPACES. The sum is then n x ceiling <= pool by construction, with no ledger of outstanding grants and no dependence on arrival order. Tested as an invariant across four host sizes: fill the daemon to its admission limit and the authorised total still fits. The cost is deliberate and now documented rather than hidden: a lone workspace on a 32 GB host gets 614 MB rather than the pool, because any child may still be running when the house fills. An 8 GB host admits seven concurrent children at 526 MB each. Also from review: - The policy is no longer built for an injected `deps.bridge`. That bridge carries its own channel and never reaches the factory the policy rides on, so status could report `enforced: true` while nothing was being sized. - Both transport mappings now have direct tests. They are hand-written beside each other and drift silently; the spawn-policy tests cannot catch a wire regression. - Swept the "does not size any child" claim, which enforce makes false, out of the CLI help text, ServeOptions docs, the two operator tables, and the e2e header comment. The 17-configuration table realigns wholesale because that cell was its widest — whitespace only. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(serve): model the child heap partition, defer applying it Review established that the refusal counter cannot tell an operator whether enforcement is safe, and that is the ground the enforcing mode stood on. 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 with zero refusals and OOMs the moment a 614 MB partition is applied. The counter measures admission pressure, not ceiling adequacy. Rather than ship a switch with no safe way to decide when to turn it on, `enforce` is removed. `--child-heap-mode` is `off | observe`, and the mode that would apply the partition arrives with the measurement that justifies it: peak old-space per child, compared against the modeled ceiling. That is a real measurement chain — the child reports rss and cpu today, and `--max-old-space-size` bounds old space specifically, so neither rss nor heapUsed answers the question. With nothing applying the partition, the machinery that existed only to apply it goes too rather than shipping unreachable: `getAcpMemoryArgs(explicitMb?)`, `ChildHeapPoolExhaustedError` and both transport mappings, and `limits.memory.enforced` reverts to the required literal `false` it was before. The spawn path is untouched again; the factory asks the policy what it would decide purely so the count is real. Also fixes the zero-pool defect review found, which the removed clamp caused: forcing at least one admissible child on a 512 MB host — where the root reserve consumes the whole 256 MB budget — produced a ceiling of 0, and `--max-old-space-size=0` is V8's *default* heap, not a zero ceiling. A pool that cannot cover one child at the floor now reports `maxConcurrentChildren: 0` and `perChildCeilingMb: null`, and the test that enshrined the old behaviour is inverted. Status now publishes `maxConcurrentChildren` and `perChildCeilingMb`, so an operator can judge the partition against their own workload — the substitute for a counter that cannot judge it for them. Every claim that a zero refusal count means the partition is safe to apply is removed from the flag help, the operator docs, the protocol doc, and the design doc. Refs #8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Three findings review raised against #8508 after the partition became observation-only, all still live on this branch now that it has merged. The status assertion in `run-qwen-serve.test.ts` failed on head: it used `toEqual` against `{ mode, refusals }` while the wire also carries `maxConcurrentChildren` and `perChildCeilingMb`, so the suite was red at 217 passed / 1 failed. The local type restating the wire shape was short the same two fields. Both are filled in, and the assertion stays `toEqual` so an unannounced field still fails it — the two derived figures get matchers because this suite boots a real daemon and the pool follows the machine. What they have to satisfy is now pinned separately: a fixed ceiling times the number admitted must fit inside the pool it partitions, which is the whole reason the partition bounds anything. `decide()` and `getAcpMemoryArgs()` ran between `reserve()` and the `try` that cancels the reservation. `childHeapPolicy` is a public `createSpawnChannelFactory` option, so `decide()` is caller code and may throw; the spawn then rejected with the token held for the process lifetime, inflating `committedProcessCount` for every later spawn. Both calls move inside the `try`. The regression test is mutation-verified — reverting the move gives `expected 1 to be +0`. `ServeOptions.memoryBudgetMb` still promised a `childHeapMode: 'enforce'` that sizes children and refuses spawns. No such mode exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QwenLM#8423) * feat(serve): observe daemon memory pressure against a real denominator The daemon samples its own RSS and heap but has nothing to divide them by, so nothing in `/daemon/status` says whether a figure is fine or nearly fatal. QwenLM#8245 landed the denominator (`limits.memory`); this turns it into a reading. `runtime.memory.pressure` reports `level`, `ratio`, `source`, and the six raw figures behind them. The level is the worse of two independent ratios, because the two 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 actually heading for. `source` names which ratio produced the level, and `unknown` says the daemon could not measure itself — which a consumer must not read as healthy. The denominator is `availableMemoryMb`, not `effectiveBudgetMb`: 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, so classifying against it would report `critical` for a daemon in no danger. `--memory-pressure-mode` is `off | observe`, default `observe`. Both modes report every figure; only `observe` also raises the `daemon_memory_pressure` warning, so `off` leaves the top-level `status` rollup untouched — the thresholds are inherited from an interactive-CLI monitor and are not yet calibrated for a long-running daemon, and a deployment that alerts on `status` needs the reading without the verdict. There is deliberately no `enforce`: nothing here remediates, and a value a caller can pass but never use is a dead switch. It arrives with the enforcement. Scope is the daemon root process only. `childRssCoverage` still reads `primary_only` and says so on the wire; aggregate child RSS and channel workers are separate measurements and land separately. Severity is `warning` at every level including `critical`, because `error` would make `rollupStatus` return `error` for the whole daemon — too strong a claim to stake on uncalibrated thresholds. Refs QwenLM#8051. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(serve): report aggregate ACP child RSS, not just the primary's (QwenLM#8462) * test(serve): close the under-determined assertions review probed The automated review mutation-probed this diff and found several assertions that were live but under-determined — each mutant it names kept the whole suite green. All confirmed locally, and all now fail: - Deleting `level !== 'normal'` from the issue gate raised daemon_memory_pressure on a healthy daemon and flipped top-level status to warning on every response — the exact false positive `--memory-pressure-mode off` exists to opt out of. Now covered on both sides: nothing raised at a realistic denominator, exactly one warning at a denominator sized to land this process in `soft`. - Summing children over `list()` instead of `listManaged()` dropped a draining-but-process-holding workspace while `activeAcpChildren` still counted it. The draining bridge now reports RSS, so the byte count can only come from that child. - The message's denominator ternary had no coverage; inverting it sent an operator hunting RSS growth during a heap-driven incident. - A truthiness guard on `ageMs` turned a measured-fresh reading (age exactly 0, when a status read lands in the sampler's millisecond) into `null`, which the field's own docs say never means fresh. - The multi-contributor age test listed ages ascending, so a plain-overwrite accumulator produced the same answer as Math.max. Reordered descending, which kills last-wins and first-wins both. Two declaration-only hunks — the issue-code union member and the `pressure` field — were guarded by tsc alone, which vitest does not run. Both are now pinned at runtime by asserting the code string and the full key set. Also fixes a real display defect: `toFixed(0)` renders a ratio of 0.795 as "hard at 80%", and 80% is critical's documented threshold. One decimal, so the number and the level cannot contradict each other. And corrects a JSDoc claim of mine that was simply wrong: `pressure` is absent not only for direct-embed but on the bootstrap /daemon/status route, which omits runtime.memory wholesale even though the budget is resolved — and that window is not just startup, since a daemon whose runtime fails to start serves the bootstrap app for its lifetime. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(serve): model a per-child heap partition of the daemon budget (QwenLM#8508) * feat(serve): add the child-heap admission primitives, unwired Groundwork for QwenLM#8182 step 2. Nothing calls any of this yet, so no child is sized differently and no spawn is refused. `ProcessRegistry.committedProcessCount` counts attached children plus reservations that have not attached. That is the figure admission has to key on: `reserve()` inserts its token synchronously before `spawn()`, so two racing spawns each see the other, while neither appears in `activeProcessCount` until its child attaches. A child leaves the count on exit rather than when `terminate()` starts, so a channel swap counts twice while the old process winds down — deliberate, since its memory is still resident. `getAcpMemoryArgs(explicitMb?)` takes an optional share that bypasses both the module cache and the raise-only guard. Both bypasses are load-bearing. The cache, because the share depends on how many children are live now rather than on the host. The guard, because a budget-derived share is normally *below* the daemon's own heap limit, so routing it through `targetMB > currentLimitMB` would drop the flag, silently restore the overcommit, and leave every test green — the trap against a multi-GB runner, and mutation-checking it by reinstating the guard fails two tests. `createChildHeapPolicy` holds the mode, the budget, and the would-be refusal counter, and answers `decide(concurrentChildren)`. The refusal is derived from the unclamped quotient, not from `recommendedChildShareMb`, because that function clamps *up* to the 512 MB floor: past the point where the pool stops covering the count its answer saturates and can no longer distinguish "barely does not fit" from "wildly does not fit". `ChildHeapPoolExhaustedError` with both transport mappings — REST 503 with Retry-After, ACP `child_heap_pool_exhausted` — added together, since the two mappings are hand-written and drift silently otherwise. Refusing at spawn rather than at registration is the correction QwenLM#8182 demands: registration allocates nothing, so this surfaces as "no new session in this workspace right now", which is true and retryable. Refs QwenLM#8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(serve): size each ACP child by concurrently live children Wires the primitives from the previous commit into the spawn path, behind `--child-heap-mode off | observe | enforce`, default `observe`. Under `enforce` a child's `--max-old-space-size` is a share of the child pool divided by the children concurrently committed at the moment it spawns — read from the shared ProcessRegistry after `reserve()`, so two racing spawns each see the other. When the pool cannot cover another child at the 512 MB floor the spawn is refused with ChildHeapPoolExhaustedError, which is what turns a per-child ceiling into an aggregate bound: concurrent children can never exceed pool/512. Keyed on concurrency, never on registrations. A dormant workspace has no child, so it costs nothing — the specific correction QwenLM#8182 records against the withdrawn proposal, which would have shrunk a lone live child to 614 MB because of 24 idle registrations. Default `observe` computes the share and the admission decision and applies neither, counting the refusals that would have happened. The divisor has never been checked against a real multi-workspace deployment, and a non-zero count is how an operator learns enforcement would have broken them without being broken. It also catches the case worth worrying about: a channel swap counts the dying child alongside its replacement, so on a saturated pool enforcement could refuse a restart and leave that workspace with no child at all. Excluding terminating children would authorise real overcommit to dodge a hypothetical refusal, so the count reports it instead. Ceilings already granted are not revisited — V8 cannot lower them — so granted ceilings transiently exceed the pool. Acceptable: the flag is a ceiling, not a reservation, and a workspace with no live sessions has no child and picks up the current share on its next spawn. `limits.memory.enforced` stops being a required literal `false`. QwenLM#8245 made it one so a client could never mistake that namespace for enforcement that had not shipped; it has now, so the field is a boolean derived from the mode — and stays `false` under `observe`, which applies nothing. Refs QwenLM#8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(serve): correct the claims child-heap enforcement makes false Two sentences in the protocol doc described the memory section as unconditionally observational: "a required `enforced: false`", and "no child spawn argument derives from these values, and no request is refused on their basis". Both are false under `--child-heap-mode enforce`, so both are rewritten rather than left to rot — `enforced` is now documented as the boolean that answers exactly this, and the refusal is documented with its wire shape on both transports. Also documents `childHeap.refusals` as the calibration signal, since a would-be-refusal count is useless if operators do not know to read it before switching to `enforce`; the flag row in the three operator docs; and the design doc's Part 1, which listed applying a share as a compatibility risk without recording how that was resolved. The end-to-end test asserts the policy reaches a real booted daemon's status with `enforced: false` under the default mode — the wire type in that test is a hand-written mirror, so its `enforced: false` literal had to widen too, which is the check that caught the type not being widened everywhere. Refs QwenLM#8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(serve): cover both branches of the enforced tripwire `enforced` was only ever asserted false — the unit tests build no policy and the end-to-end daemon runs the default `observe` mode, so the branch that makes the field worth having was untested. Hardcoding it back to `false` passed everything. Also pins `childHeap: null` as distinct from a policy in `off` mode: the first says no policy exists (direct-embed, or the bootstrap window before the runtime is built), the second says one exists and computes nothing. Refs QwenLM#8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): partition the child pool so granted ceilings stay inside it Review was right that the previous design did not deliver the aggregate bound it claimed. 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 x H(n). Reproduced exactly — 9557 MB authorised against a 3687 MB pool at seven children on an 8 GB host, and 61355 MB against 15360 MB at the limit on 32 GB. That is 2.6x and 4x the pool, which is what the policy exists to prevent. Grant accounting alone does not fix it: the first child would take the whole pool and the second would be refused immediately. Keeping the invariant requires early children not to receive the whole pool, so the ceiling is now a fixed partition — childPoolMb / maxConcurrentChildren, constant for every child, with maxConcurrentChildren itself derived from the pool and capped at MAX_DAEMON_WORKSPACES. The sum is then n x ceiling <= pool by construction, with no ledger of outstanding grants and no dependence on arrival order. Tested as an invariant across four host sizes: fill the daemon to its admission limit and the authorised total still fits. The cost is deliberate and now documented rather than hidden: a lone workspace on a 32 GB host gets 614 MB rather than the pool, because any child may still be running when the house fills. An 8 GB host admits seven concurrent children at 526 MB each. Also from review: - The policy is no longer built for an injected `deps.bridge`. That bridge carries its own channel and never reaches the factory the policy rides on, so status could report `enforced: true` while nothing was being sized. - Both transport mappings now have direct tests. They are hand-written beside each other and drift silently; the spawn-policy tests cannot catch a wire regression. - Swept the "does not size any child" claim, which enforce makes false, out of the CLI help text, ServeOptions docs, the two operator tables, and the e2e header comment. The 17-configuration table realigns wholesale because that cell was its widest — whitespace only. Refs QwenLM#8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(serve): model the child heap partition, defer applying it Review established that the refusal counter cannot tell an operator whether enforcement is safe, and that is the ground the enforcing mode stood on. 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 with zero refusals and OOMs the moment a 614 MB partition is applied. The counter measures admission pressure, not ceiling adequacy. Rather than ship a switch with no safe way to decide when to turn it on, `enforce` is removed. `--child-heap-mode` is `off | observe`, and the mode that would apply the partition arrives with the measurement that justifies it: peak old-space per child, compared against the modeled ceiling. That is a real measurement chain — the child reports rss and cpu today, and `--max-old-space-size` bounds old space specifically, so neither rss nor heapUsed answers the question. With nothing applying the partition, the machinery that existed only to apply it goes too rather than shipping unreachable: `getAcpMemoryArgs(explicitMb?)`, `ChildHeapPoolExhaustedError` and both transport mappings, and `limits.memory.enforced` reverts to the required literal `false` it was before. The spawn path is untouched again; the factory asks the policy what it would decide purely so the count is real. Also fixes the zero-pool defect review found, which the removed clamp caused: forcing at least one admissible child on a 512 MB host — where the root reserve consumes the whole 256 MB budget — produced a ceiling of 0, and `--max-old-space-size=0` is V8's *default* heap, not a zero ceiling. A pool that cannot cover one child at the floor now reports `maxConcurrentChildren: 0` and `perChildCeilingMb: null`, and the test that enshrined the old behaviour is inverted. Status now publishes `maxConcurrentChildren` and `perChildCeilingMb`, so an operator can judge the partition against their own workload — the substitute for a counter that cannot judge it for them. Every claim that a zero refusal count means the partition is safe to apply is removed from the flag help, the operator docs, the protocol doc, and the design doc. Refs QwenLM#8182. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix(serve): repair the child-heap assertion and the reservation leak Three findings review raised against QwenLM#8508 after the partition became observation-only, all still live on this branch now that it has merged. The status assertion in `run-qwen-serve.test.ts` failed on head: it used `toEqual` against `{ mode, refusals }` while the wire also carries `maxConcurrentChildren` and `perChildCeilingMb`, so the suite was red at 217 passed / 1 failed. The local type restating the wire shape was short the same two fields. Both are filled in, and the assertion stays `toEqual` so an unannounced field still fails it — the two derived figures get matchers because this suite boots a real daemon and the pool follows the machine. What they have to satisfy is now pinned separately: a fixed ceiling times the number admitted must fit inside the pool it partitions, which is the whole reason the partition bounds anything. `decide()` and `getAcpMemoryArgs()` ran between `reserve()` and the `try` that cancels the reservation. `childHeapPolicy` is a public `createSpawnChannelFactory` option, so `decide()` is caller code and may throw; the spawn then rejected with the token held for the process lifetime, inflating `committedProcessCount` for every later spawn. Both calls move inside the `try`. The regression test is mutation-verified — reverting the move gives `expected 1 to be +0`. `ServeOptions.memoryBudgetMb` still promised a `childHeapMode: 'enforce'` that sizes children and refuses spawns. No such mode exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): report no child-heap partition under `off` `snapshot()` returned `maxConcurrentChildren` and `perChildCeilingMb` unconditionally, so a daemon run with `--child-heap-mode off` still published a partition — 7 children at 526 MB on an 8 GB host — under a mode whose documentation says "do not model it". Review raised it, and it mattered more than it looked: with `enforce` gone, `off` and `observe` differed only in whether `refusals` incremented, so nothing on the wire distinguished a model that was switched off from one in force. Both figures are now `null` under `off`, which required widening `maxConcurrentChildren` to `number | null` in the daemon type and the SDK mirror. `null` rather than `0`: zero is already the computed answer for a pool too small to host one child at the 512 MB floor, and collapsing the two would tell an operator who disabled the model that their host cannot run anything. That leaves three distinguishable states — no policy at all (`childHeap: null`), a policy modeling nothing (`mode: 'off'` with null figures), and a live model — and each now has a test. The `off` unit test previously asserted only `refusals`, so its name ("models nothing at all when off") promised more than it checked. It now covers the figures, with a sibling test pinning 7 / 526 under `observe` on the same budget so nulling them unconditionally cannot satisfy both. Mutation-verified in both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): never model a child heap ceiling below the documented minimum `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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
2 participants
What this PR does
Models a per-child heap partition of the daemon's memory budget and reports it. It applies nothing — no child is sized from the budget, no spawn is refused.
--child-heap-modeisoff | observe, defaultobserve. Status gainslimits.memory.childHeap:maxConcurrentChildren,perChildCeilingMb, andrefusals(spawns that would have exceeded the modeled limit).It also fixes a zero-pool defect introduced by an earlier revision of this PR: forcing at least one admissible child meant a 512 MB host — where the root reserve consumes the entire 256 MB budget, leaving a pool of 0 — modeled a ceiling of 0.
--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, and the test that had enshrined the old behaviour is inverted.Why it's needed
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. That is the defect #8182 records. This PR does not close it — it publishes the partition that would close it, so the partition can be judged before anything depends on it.Why it stops at modeling. The earlier revision of this PR applied the partition and refused spawns. Review established two things that took that apart, both correct:
The per-spawn share was not an aggregate bound. 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). Reproduced: 9557 MB authorised against a 3687 MB pool at seven children on 8 GB; 61355 MB against 15360 MB on 32 GB. That is fixed — the model is now one constant ceiling for every child, with admission capped so the total stays inside the pool by construction, verified as an invariant across four host sizes.The refusal counter could not tell an operator when enforcing was safe. 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 with
refusals: 0and OOMs the instant a 614 MB partition is applied. The counter measures admission pressure, not ceiling adequacy.That second point is what removed
enforce. Its inclusion rested on "observe first, then enforce", and that path does not exist yet: answering it needs peak old-space per child (not rss, which is what the child reports today, and notheapUsed, which includes new space) compared against the modeled ceiling. That is a measurement chain of its own, 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.Since nothing applies the partition, the machinery that existed only to apply it went with it rather than shipping unreachable:
getAcpMemoryArgs(explicitMb?),ChildHeapPoolExhaustedErrorand both transport mappings, andlimits.memory.enforcedreverts to the required literalfalse. The spawn path is untouched; the factory asks the policy what it would decide purely so the count is real. Net −310 lines against the previous revision.What an operator gets instead.
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. Every claim thatrefusals: 0means the partition is safe to apply is gone from the flag help, the three operator docs, the protocol doc, and the design doc.Reviewer Test Plan
How to verify
1. The modeled partition is reported, and it fits inside the pool. Boot a daemon and read the new block:
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 holds on any host:2. Nothing is applied — this is the check that matters. Open a session so a
qwen --acpchild spawns, then read its argv under each mode:The value must be the host-derived ceiling (e.g. 16384 on a 32 GB host) and must be byte-identical under
--child-heap-mode observeand--child-heap-mode off. In particular it must not equalperChildCeilingMb. If those two runs differ, this PR's central claim is false.3.
limits.memory.enforcedis still the literalfalse. The previous revision widened it toboolean; that is reverted, and clients may keep relying on it:4. The enforcing mode is genuinely absent, not merely defaulted off. Both the yargs path and the fast path must reject it:
qwen serve --child-heap-mode enforce # error: Invalid values: Argument: child-heap-mode, Given: "enforce", Choices: "off", "observe"5. Unit suites.
Evidence (Before & After)
N/A — no user-visible or TUI change. The observable difference is the
/daemon/statuspayload:"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, and step 2 above is how a reviewer confirms it.
Tested on
Locally on Linux: 48 acp-bridge + 175 cli tests, prettier, eslint, and
tsconacp-bridge,cli, andsdk-typescript. The invariant (maxConcurrentChildren × perChildCeilingMb ≤ childPoolMb) is tested 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. macOS and Windows are left to CI — the change is arithmetic and status plumbing with no platform-specific paths.Environment (optional)
npm run devdaemon on Linux; unit tests only for the assertions above.Risk & Scope
refusals: 0may conclude the partition is safe to enforce. It is not, for the reason in Why it's needed, and that caveat is now stated in the flag help, all three operator docs, and the protocol doc. The tradeoff accepted here 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.--max-old-space-sizecovers only V8 old space and never Buffers, native allocations, or descendants.limits.memory.childHeapis additive, andlimits.memory.enforcedremains the required literalfalse— the earlier revision of this PR widened it toboolean, and that widening is reverted here, so no client contract changes.--child-heap-modeis a new flag defaulting toobserve;enforceis not an accepted value.Linked Issues
Refs #8182. Stacked on #8423.
中文说明
这个 PR 做了什么
对 daemon 内存预算按子进程建模出一份堆分区，并把它报告出来。它不应用任何东西——没有子进程按预算调整大小，也没有派生被拒绝。
--child-heap-mode取值off | observe，默认observe。状态新增limits.memory.childHeap：maxConcurrentChildren、perChildCeilingMb，以及refusals（本会超出建模上限的派生次数）。同时修复了本 PR 早前版本引入的「零池」缺陷：强制至少允许一个子进程，意味着在 512 MB 主机上——root 预留吃掉了全部 256 MB 预算，池为 0——会建模出上限 0。而
--max-old-space-size=0表示 V8 的默认堆，约 4 GB，并非零上限。现在，连一个 512 MB 下限子进程都容纳不下的池会报告maxConcurrentChildren: 0和perChildCeilingMb: null，那个把旧行为固化成预期的测试也已被反转。为什么需要它
getAcpMemoryArgs()从主机内存算出一个上限，然后把同一个值发给每个子进程。daemon 每个 workspace 跑一个子进程，最多注册 25 个，于是一台 32 GB 主机授权了 25 × 16 GB = 400 GB 的子进程堆。这就是 #8182 记录的缺陷。本 PR 并未关闭它——而是把能关闭它的那份分区先发布出来，好让这份分区在被依赖之前先接受检验。为什么止步于建模。 本 PR 的早前版本应用了分区并拒绝派生。评审确立了两点，把它拆掉了，且两点都成立：
按派生时刻分摊不构成总量约束。 按各自派生时刻的存活数给每个子进程定额，约束的是子进程数量而非内存——V8 无法下调运行中子进程的上限，于是授权累加为
P + P/2 + P/3 + … = P × H(n)。已复现：8 GB 主机上七个子进程时，对 3687 MB 的池授权了 9557 MB；32 GB 上是 61355 MB 对 15360 MB。这一点已修正——模型现在是给每个子进程一个恒定上限，并对准入设上限，使总量按构造落在池内，并作为不变量在四种主机规格上验证。拒绝计数无法告诉运维何时开启强制是安全的。 观察期间子进程跑在主机推导的上限上——32 GB 主机是 16384 MB——所以一个需要 2 GB 老生代的负载在
refusals: 0下完全健康，而一旦应用 614 MB 的分区就会立刻 OOM。这个计数衡量的是准入压力，不是上限是否够用。第二点正是移除
enforce的原因。它的存在依赖「先观察、再强制」，而这条路径尚不存在：要回答它，需要每个子进程的峰值老生代（不是子进程今天上报的 rss，也不是包含新生代的heapUsed）与建模上限对比。那本身是一条独立的测量链，强制模式将与它一同发布。发布一个无法安全判断何时开启的开关，比不发布这个开关更糟。既然没有任何东西应用这份分区，那些只为应用它而存在的机制也随之移除，而不是以不可达的形式发布：
getAcpMemoryArgs(explicitMb?)、ChildHeapPoolExhaustedError及其两处传输层映射，以及limits.memory.enforced回退为必需的字面量false。派生路径未被改动；工厂询问策略「会做什么决定」，纯粹是为了让计数真实。相对上一版净减 310 行。运维实际得到什么。 发布
perChildCeilingMb和maxConcurrentChildren，是为了让这份分区能对照已知负载来判断——这是那个无法做判断的计数的替代品。8 GB 主机建模为 7 个子进程、每个 526 MB；32 GB 建模为 25 个、每个 614 MB，两者都在测试中固定，因为这正是做容量规划时依据的数字。所有「refusals: 0意味着分区可以安全应用」的说法，已从 flag 帮助、三份运维文档、协议文档和设计文档中清除。评审者测试计划
如何验证
1. 建模分区被报告出来，且落在池内。 启动 daemon 并读取新字段：
8 GB 主机上预期
{"mode":"observe","maxConcurrentChildren":7,"perChildCeilingMb":526,"refusals":0}；32 GB 上是25与614。使其成为总量约束的那个不变量在任何主机上都成立：2. 什么都没被应用——这是最关键的检查。 打开一个 session 让
qwen --acp子进程派生出来，然后在两种模式下分别读它的 argv：该值必须是主机推导的上限（例如 32 GB 主机上是 16384），并且在
--child-heap-mode observe与--child-heap-mode off下逐字节一致。尤其不能等于perChildCeilingMb。如果这两次运行有差异，本 PR 的核心主张就是错的。3.
limits.memory.enforced仍是字面量false。 上一版把它放宽为boolean，此处已回退，客户端可以继续依赖它：4. 强制模式是真的不存在，而不只是默认关闭。 yargs 路径与 fast path 都必须拒绝它：
qwen serve --child-heap-mode enforce # error: Invalid values: Argument: child-heap-mode, Given: "enforce", Choices: "off", "observe"5. 单元测试套件。
证据（前后对比）
N/A —— 无用户可见或 TUI 变更。可观察的差异是
/daemon/status的载荷："limits": { "memory": { "enforced": false, + "childHeap": { "mode": "observe", "maxConcurrentChildren": 7, "perChildCeilingMb": 526, "refusals": 0 }, "modeled": { "childPoolMb": 3687, ... } } }子进程 argv 在两个方向上都未改变——这正是重点，上面第 2 步就是评审者确认它的方式。
测试平台
Linux 本地：48 个 acp-bridge 测试 + 175 个 cli 测试，prettier、eslint，以及对
acp-bridge、cli、sdk-typescript的tsc。不变量（maxConcurrentChildren × perChildCeilingMb ≤ childPoolMb）在 2/8/32/256 GB 主机规格上均有测试；observe被断言使派生 argv 与「无策略工厂」逐字节一致；enforce被断言在 yargs 与 fast path 两处均被拒绝。macOS 与 Windows 交由 CI——本变更是算术与状态管道，不含平台相关路径。环境（可选）
Linux 上的
npm run devdaemon；上述断言仅涉及单元测试。风险与范围
refusals: 0可能会认为分区可以安全强制。并非如此，原因见为什么需要它，该警告现已写入 flag 帮助、三份运维文档和协议文档。此处接受的权衡是：发布一个不约束任何东西的模型——bug(serve): daemon authorises each ACP child 50% of host memory, never divided by child count #8182 的 400 GB 超额授权在强制模式连同其测量手段落地之前仍然敞开。--max-old-space-size只覆盖 V8 老生代，从不包含 Buffers、原生分配或子孙进程。limits.memory.childHeap是新增字段，limits.memory.enforced仍为必需的字面量false——本 PR 早前版本曾将其放宽为boolean，该放宽在此已回退，因此客户端契约没有变化。--child-heap-mode是默认observe的新 flag；enforce不是可接受的取值。关联 Issue
Refs #8182。叠在 #8423 之上。
🤖 Generated with Claude Code