From 8f0549155fe8e3fba4f7685de36fe94fed1d200e Mon Sep 17 00:00:00 2001 From: Peter Pathirana Date: Fri, 21 Aug 2026 21:48:22 +0000 Subject: [PATCH] fix: declare what each watchdog budget is anchored on, and stop arming guesses No budget value changes. What changes is that each one now says where it came from, and the default mode's arming is derived from that. The formula is asymmetric. `resting x 1.5` is a floor, so a budget can never ship below what its role was measured holding - the failure this design shipped twice - but nothing bounds a budget from above. A number derived from a measurement and a round number reached for in the absence of one therefore sat in the same column, printed the same `event=budget` line, and were enforced identically. Three of the four roles with no RESTING_ROLE entry carried exactly 512 MiB. A derived ceiling is the wrong fix and is not attempted. The whole sample is one operator, one Terraform/YAML repository, one 38-hour window in which the only member extensionHelper has ever had is a 24 MB terraform-ls. A ceiling fitted to that would put a healthy gopls or Pyright server under a limit set by a language server that was never running. So: BUDGET_BASIS declares `measured` / `adversary` / `guess` per role, `event=budget` carries `basis=`, UNKNOWN_ROLES is derived from it rather than hand-listed, and `enforce` requires both axes - low blast radius AND a budget with a measurement behind it. That moves two roles out of the default mode, and tsserver is why the rule is not a formality: VS Code launches it with --max-old-space-size=3072 (its shipped typescript.tsserver.maxTsServerMemory default, observed on both instances in the live pod, configured nowhere here). The 768 MiB budget is a quarter of the heap its own launcher sanctions, so on a large TypeScript project a healthy tsserver would dwell over budget, be killed, cost a multi-minute re-index with dead IntelliSense, and repeat until the breaker disarmed the role. Nothing in this workspace's data could contradict that: its two tsservers sat between 112 and 208 MiB for 29 hours on a repository with no TypeScript in it. extensionHelper is the same shape. Both keep their budget, dwell clock and `event=would-kill armed=no` line, which is the evidence needed to arm them later. RESTING_ROLE[fileWatcher] 88 -> 107 MiB: flat at 107 MiB for 28 straight hours, so the reference was stale. It moves no budget - 107 x 1.5 is still far below the declared 256 MiB. RESTING_ROLE[extensionHost] is deliberately NOT moved despite a 738 MiB peak against its 713 MiB figure, because its hourly minimum climbed 528 -> 686 MiB over the same window: that is drift, not staleness, and raising the reference would widen the budget of the only role in the pod that is drifting. A flat floor above the reference is a measurement to correct; a rising floor above it is a finding to report. Also fixes a wrong assertion description found while doing this: a test read "while a role with a measured resting size is armed" about extensionHelper, which has never had one. Every new assertion is paired with the production mutation that flips it red, and all eight were run: reverting the enforce clause, hand-listing UNKNOWN_ROLES, mislabelling a basis in either direction, dropping `basis=` from the log line, adding a role with no basis, dropping one from BUDGET_ORDER, and lowering a declared budget below its floor. --- CLAUDE.md | 8 +- DESIGN.md | 8 +- .../script-memory-watchdog-test.sh | 220 ++++++++++++++++- .../script-memory-watchdog.sh | 226 ++++++++++++++---- 4 files changed, 412 insertions(+), 50 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d568fc7f..0c5f23c8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,7 +75,7 @@ Quick orientation map — for what each piece is *for* and the decisions behind | `variables.tf` | `workspace_image`, `test_mode` — both supplied by the release workflow | | `script-agent-startup.sh` / `script-prepare-workspace.sh` | Scripts run on agent/workspace startup | | `script-container-entrypoint.sh` | The workspace container's `command`. Wipes `/tmp` and `exec`s Coder's generated `/workspace-init.sh` — the wipe must precede the agent, see the gotcha below | -| `script-memory-watchdog.sh` | Userspace memory watchdog — see [DESIGN.md](DESIGN.md#design-tensions-and-decisions). It bounds the **standing population of restartable helpers** (per-role PSS budgets, ten-minute dwell, per-role circuit breaker) and records every per-process sweep. It does **not** try to prevent an acute OOM. `memory_watchdog_mode` selects `observe` / `enforce` (helpers — the default) / `enforce-all` (helpers + editor + the uncalibrated `treeHelper` role) | +| `script-memory-watchdog.sh` | Userspace memory watchdog — see [DESIGN.md](DESIGN.md#design-tensions-and-decisions). It bounds the **standing population of restartable helpers** (per-role PSS budgets, ten-minute dwell, per-role circuit breaker) and records every per-process sweep. It does **not** try to prevent an acute OOM. `memory_watchdog_mode` selects `observe` / `enforce` (the default — only helpers whose budget is anchored on a measurement) / `enforce-all` (adds the editor roles and every role whose budget is a `guess`: `tsserver`, `extensionHelper`, `treeHelper`) | | `script-memory-watchdog-test.sh` | Fixture tests for the watchdog's arithmetic, process selection, budgets, dwell and circuit breaker. Run by hand (`./script-memory-watchdog-test.sh`) and by the `watchdog` job in `.github/workflows/test.yaml`. `kill` is shadowed by a function throughout — the fixture pids are real pids in whatever container runs the suite | | `script-vscode-server-gc.sh` | Weekly GC of `~/.vscode-server` (interrupted downloads, superseded server versions/extensions, orphaned CLI binaries — see the script's own header for the exact signal per class, and the `coder_script.vscode_server_gc` comment in `scripts.tf` for why it's template-owned rather than dotfiles-owned) | @@ -93,9 +93,11 @@ Things that look arbitrary in the code but are load-bearing (full reasoning in [ - `script-memory-watchdog.sh` computes headroom as `memory.max − U`, where `U` sums only the *unreclaimable* fields of `memory.stat` (`anon`, `shmem`, `unevictable`, `slab_unreclaimable`, `kernel_stack`, `pagetables`, `sec_pagetables`, `percpu`, `sock`). Do not "simplify" it to `memory.current` or to `memory.stat`'s `kernel` roll-up: on the live pod those read 96% and 42% of the limit while true `U` is 28%. Nothing acts on this number any more — it is pod-level context for the per-process rows and the honest figure published in the workspace UI. - `calibration.csv`'s `du_bytes_per_s` column is a per-sample instantaneous rate (`(M_U - PREV_U) / SAMPLE_INTERVAL`, 10s by default), not a drift rate, and naively averaging it overstates drift roughly fourfold: ~19.8 kB/s (~68 MB/hour) naive mean on the live pod versus ~18.4 MB/hour computed from `u_mb` across two `event=census` rows 26.1h apart. The naive mean is dominated by a handful of 10s allocation/GC spikes (+102 to −58 MB/s) that an hourly comparison washes out. The column and its caveat are documented together at its declaration in the script; nothing currently reads this column programmatically, so the trap is for a human, not a bug. - **The watchdog is a drift policer, not an OOM preventer, and the difference is measured.** The graded L1–L4 shedding ladder that used to be here was removed, not tuned: the recorded kills are 70–220 MB/s spikes that go from idle to dead inside a minute, a live reproduction climbed the ladder correctly and logged `no-candidates` because the runaway was not in the tree it managed, and the entire editor tree it could shed is ~0.7 GiB — five seconds of that growth. Before re-adding anything reactive, establish that a poll loop can see the event at all. What the loop *is* good at is MB-per-minute growth in the standing population, which is what it now does. -- **Budgets are per role, in PSS, and never below a role's measured *resting* size — not its fresh size.** Fresh tree: extension host 471 MB PSS, serverMain 160 MB, ptyHost 36 MB, file watcher 34 MB. Resting tree (reconnected, idle, 8 GiB pod): extension host **713 MB**, file watcher 88 MB, language server 54 MB, tree total 1093 MB. A uniform 512 MB budget would sit 40 MB above where the extension host *starts* and 200 MB below where it *lives*, which is the same class of defect as the earlier `RLIMIT_DATA` ceiling that landed below what an idle file watcher already held — "calibrated against fresh, deployed against resting" has now appeared twice in this design, so `RESTING_ROLE` holds the resting figures and nothing else. Each budget is `max(min(role budget, memory.max / 8), resting × 1.5)`, and the derivation records which budgets the floor lifted (`floored=1`), which is documented to mean the pod is too small to bound that role at its intended share. **That reading only holds while every declared budget already clears its own resting floor, and it did not: `extensionHost` was declared at 1024 MiB against a 1069 MiB floor, so `floored=1` was true at 4, 8, 16 and 64 GiB alike — a property of the constants reported as a property of the pod.** The declared number is now 1069 MiB, the one that was always in force; no effective budget changed at any pod size, and the flag now flips off at 16 GiB. The tests assert the *properties* — no budget at or below resting, and no declared budget below its own floor, at any pod size — rather than the arithmetic. Override one role with `WATCHDOG_BUDGET_`; PSS (`smaps_rollup`) is the comparison, not RSS and not `VmData`. +- **Budgets are per role, in PSS, and never below a role's measured *resting* size — not its fresh size.** Fresh tree: extension host 471 MB PSS, serverMain 160 MB, ptyHost 36 MB, file watcher 34 MB. Resting tree (reconnected, idle, 8 GiB pod): extension host **713 MiB**, file watcher **107 MiB** (corrected from 88 MiB — see the review-trigger bullet below), language server 54 MiB, tree total 1093 MB. A uniform 512 MB budget would sit 40 MB above where the extension host *starts* and 200 MB below where it *lives*, which is the same class of defect as the earlier `RLIMIT_DATA` ceiling that landed below what an idle file watcher already held — "calibrated against fresh, deployed against resting" has now appeared twice in this design, so `RESTING_ROLE` holds the resting figures and nothing else. Each budget is `max(min(role budget, memory.max / 8), resting × 1.5)`, and the derivation records which budgets the floor lifted (`floored=1`), which is documented to mean the pod is too small to bound that role at its intended share. **That reading only holds while every declared budget already clears its own resting floor, and it did not: `extensionHost` was declared at 1024 MiB against a 1069 MiB floor, so `floored=1` was true at 4, 8, 16 and 64 GiB alike — a property of the constants reported as a property of the pod.** The declared number is now 1069 MiB, the one that was always in force; no effective budget changed at any pod size, and the flag now flips off at 16 GiB. The tests assert the *properties* — no budget at or below resting, and no declared budget below its own floor, at any pod size — rather than the arithmetic. Override one role with `WATCHDOG_BUDGET_`; PSS (`smaps_rollup`) is the comparison, not RSS and not `VmData`. **Each budget also declares what it is anchored on (`BUDGET_BASIS`: `measured` / `adversary` / `guess`), and that is not documentation — the arming rule is derived from it.** The formula is asymmetric: `resting × 1.5` is a floor, so a budget can never ship *below* a role's measured resting size, but nothing bounds one from above, so a round number picked without a measurement can only err generously and used to be indistinguishable in the log from a derived one. The answer is not a derived ceiling — fitted to this sample it would put a healthy `gopls` under a limit set by a 24 MB `terraform-ls` — it is that `basis=guess` roles are **excluded from `enforce`** by construction, so a role added without a measurement lands unarmed instead of relying on somebody remembering to list it. `event=budget` carries `basis=` for every role. +- **`enforce` requires *both* axes to say yes — low blast radius *and* a budget with a measurement behind it — and `tsserver` is the case that proves the second one is not decoration.** VS Code launches tsserver with `--max-old-space-size=3072` (its shipped `typescript.tsserver.maxTsServerMemory` default; observed live on both instances in the pod, set nowhere in this workspace's settings), so the 768 MiB budget is a *quarter* of the heap the editor itself sanctions. On a large TypeScript project a perfectly healthy tsserver would dwell over budget, be killed, lose IntelliSense for a multi-minute re-index, and do it three times before the breaker disarmed the role — the exact harm the resting floor exists to prevent, arriving through a role that had no resting figure to floor it. `extensionHelper` is the same shape: its only member ever observed is a 24 MB `terraform-ls`, while the `gopls`/Pyright/`rust-analyzer` servers its own comment cites classify into it and hold hundreds of MB. Both are now reported-not-signalled under `enforce`, exactly like `treeHelper`, and armed under `enforce-all`. They keep their budget, dwell clock and `event=would-kill armed=no` line — which is the evidence needed to arm them later, and the reason this is not deferral. +- **The `RESTING_ROLE` review trigger has to ask whether the role's floor is *flat* or *rising*, because the first time it fired it fired for two roles meaning opposite things.** Over one 38-hour window: `fileWatcher` sat flat at 107 MiB for 28 straight hours against an 88 MiB reference — a stale measurement, corrected to 107 MiB, which moves no budget because 107 × 1.5 is still far below the declared 256 MiB. `extensionHost` peaked at 738 MiB against a 713 MiB reference while its hourly *minimum* climbed 528 → 686 MiB — that is drift, not staleness, and the reference was deliberately **not** moved: raising it would widen the budget of the only role in the pod that is actually drifting, in response to the drift, which is the move the circuit breaker refuses made by a human instead of by the loop. A flat floor above the reference is a measurement to correct; a rising floor above it is a finding to report. - **A kill needs ten minutes of continuous over-budget dwell, and three kills of one role inside an hour disarm that role.** The dwell is what separates drift from load — a language server that balloons while indexing and hands the memory back must survive. The breaker is what stops the failure that would make this actively harmful: kill the extension host → VS Code restarts it → it reloads every extension → it exceeds again → kill, a loop that arrives looking exactly like the watchdog working. It disarms and reports rather than widening its own budget, because a mechanism that raises the limit it is enforcing has stopped enforcing. -- **Inside the server tree, an unrecognised process is *governed*, not invisible — that default is inverted on purpose, and its risk is bought off by arming rather than by hope.** `role_of` returned `other` for three real helpers on the live tree, and `compute_policed` skips `other`, so they had no budget, no dwell clock and no warning: a `tamasfe.even-better-toml` `server.js` at **280 MB PSS** (already above the 256 MiB `languageServer` budget a matching pattern would have given it), tsserver's `typingsInstaller.js` at 61 MB, and the built-in `markdown-language-features` server at 54 MB. Widening the pattern list is the easy fix and the wrong one: the set of launch shapes is the union of every extension's own choices, so a pattern list is a permanent race against a vendor-controlled vocabulary that fails *silently*. So anything in the tree, on VS Code's own runtime, unprotected and unnamed becomes role `treeHelper`. Two things make that safe rather than merely bold: (1) `treeHelper` is in `UNKNOWN_ROLES`, armed **only** under `enforce-all`, because its 512 MiB budget is a reasoned guess and no member has been measured at rest — under `enforce` (the template default) a coverage gap arrives as `event=would-kill armed=no` in the container log instead of as silence; (2) a **direct child of a server root is excluded structurally** — core forks (`fileWatcher`, `extensionHost`, `ptyHost`) are exactly the direct children of `server-main.js`, and no extension helper is, so a VS Code upgrade that renames `--type=extensionHost` makes that process unmanaged and loud rather than managed on a 512 MiB budget it holds 685 MB against. Its sweep-log identity is `/