feat: police helper-process drift instead of chasing OOM spikes - #865
Merged
Conversation
The memory watchdog was built to prevent a cgroup OOM by shedding VS Code processes before the limit. That premise did not survive measurement: - every recorded kill is a 70-220 MB/s spike, idle to dead inside a minute, with an agent session or node named as the victim, never a VS Code process; - a poll loop cannot win that race - a biggest-RSS killer beats the kernel only at a 0.3s interval and is killed by oom.group when it loses; - the whole editor tree the ladder could shed is ~0.7 GiB, five seconds of that growth, and in a live spike the ladder climbed correctly and then logged no-candidates because the runaway was not in the tree it managed. So the graded L1-L4 ladder and the RLIMIT_DATA ceilings are removed, and the measurement they were built on is kept. What replaces them is the thing a poll loop is actually good at: bounding the standing population of restartable helpers, which the operator has been policing by hand for months (a python MCP server held 1.66 GB at one of the kills). The goal is runway, not rescue. What it now does: - per-role PSS budgets, each clamped between a share of the pod's memory.max and 1.5x the role's measured resting size, so a budget can never land below what a role demonstrably needs - the defect that reached enforce-readiness twice before; - a kill needs ten minutes of continuous over-budget dwell, so a language server that balloons while indexing and hands the memory back survives; - three kills of one role inside an hour disarm that role, with a loud log line. The kill loop, not the wrong kill, is what would make this harmful; - a second policed population: helpers an agent session spawned, chiefly MCP servers, which live nowhere near ~/.vscode-server. The walk stops at shells and at a change of session id - measured, because Claude Code detaches every Bash tool call into its own session, which keeps in-flight work out even when the tool call's shell has exec'd itself away; - a durable per-process sweep log with identity breadcrumbs and argv redaction. The previous version computed this table every cycle and threw it away, which is why every post-mortem in this investigation was unanswerable. Identity guards stay absolute; the two positional rules (ptyHost subtree, "not VS Code's own binary") now bound the editor selection only, so an MCP server is treated the same whether its session came from coder ssh or a VS Code terminal. memory_watchdog_mode gains a third value: observe / enforce (helpers, the new default) / enforce-all (adds the extension host and server, which restart visibly). Exercised on the test workspace as well as by fixtures: in enforce mode the watchdog killed a drifted 739 MB helper three times as its supervisor respawned it, disarmed the role on the third, and left the fourth incarnation, a detached 400 MB tool call and both session roots untouched.
…in effect memory.oom.group is fixed for a container when the kubelet creates it, so a long-lived workspace still reads 1 while a pod created after the rollout reads 0. Sampling only the former is what produced the wrong claim; both were re-checked, and a freshly created pod reads 0. The design is unchanged - a poll loop still cannot see a 43-second event, and drift policing is still the right job for one - but the acute case now rests on the kernel, with runway as the second line rather than the only one. That kubelet setting was applied to the nodes by hand and exists in no repository, so a rebuild or the Talos migration would silently restore all-or-nothing OOM behaviour. Raised as ppat/homelab-ops-kubernetes-clusters#948 and referenced from DESIGN.md, because this template's rationale now depends on it.
… resting size Two changes, both from measurement. **Action lines now leave the pod.** The cluster's log agent tails container stdout only, but PID 1 in this container is the coder agent, so /proc/1/fd/1 is that stdout: writing there needs no new infrastructure and no configuration anywhere, and the lines arrive in Loki labelled by namespace, pod and container. Only actions take that route - kills, refusals, disarms, the budgets in force, and one census line an hour. The per-process sweep stays in the local file, and a test asserts that it does: dozens of rows a minute do not belong in a log pipeline. Every line is logfmt beginning with component=memory-watchdog, so `|= "component=memory-watchdog" | logfmt` works against a very chatty stream without adding a stream label; free text is quoted into detail= by record_action rather than at each call site. The write is best-effort - one failure disables the path, records why once locally, and changes nothing else. Observe mode emits what it would have done, which is the evidence needed before arming this anywhere. **Budgets are re-anchored on resting rather than fresh measurements.** A fresh extension host is 471 MB PSS; the same process on a reconnected, idle 8 GiB workspace holds 713 MB, with the tree at 1093 MB rather than the 727 MB this design cited. "Calibrated against fresh, deployed against resting" is the error that produced a file-watcher ceiling below what an idle file watcher held, and it had crept back in. RESTING_ROLE now carries resting figures, which lifts the extension host to 1069 MiB on both pod sizes via the existing floor rule, and the derivation reports which budgets the floor lifted (floored=1) - that means the pod is too small to bound the role at its intended share, which is worth saying rather than hiding. Also: the fixture suite now refuses to run unless its stdout seam is set. The first run after the stdout path was added wrote fixture kill lines into the live workspace's container log and thus into Loki, describing kills that never happened in the format a post-mortem would trust. Nothing was signalled (kill is shadowed there), a correcting note was appended to the same stream, and a test fixture that can write to production telemetry is now a hard failure. Verified live from the test workspace: the daemon's own census and budget lines appear in Loki and parse into 18 logfmt fields, with budget_mb=1069 floored=1 for the extension host against a 512 MiB pod share.
Contributor
|
🎉 This PR is included in version 2.26.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The memory watchdog stops trying to prevent OOM kills and starts bounding the standing population of restartable helper processes. The graded L1–L4 shedding ladder and the
RLIMIT_DATAceilings are removed; the measurement underneath them is kept.Why the old purpose is gone
nodenamed as the victim in the kernel log — never a VS Code process.memory.oom.group = 1that applied at the time was killed by the event it lost to.no-candidates: the runaway was not in the tree it managed.What a poll loop is good at is MB-per-minute drift in the long-lived population — which is real, has been policed by hand for months, and had a python MCP server holding 1.66 GB at one of the kills.
Budgets
Per role, in PSS, each
max(min(role budget, memory.max / 8), resting × 1.5):extensionHostserverMaintsserverlanguageServerfileWatcherextensionHelperclaudeHelper(MCP servers)The operator's "512, maybe 256" instinct is applied unchanged to the helpers, where it is right. It is not applied to the extension host: fresh it is 471 MB, and at rest on a reconnected idle 8 GiB workspace it is 713 MB, with the tree at 1093 MB rather than the 727 MB this design originally cited. 512 would have fired on reconnection alone.
"Calibrated against fresh, deployed against resting" is the same error that produced a ceiling below what an idle file watcher already held, so the references are the resting figures and nothing else. The floor rule then does the work by itself: at 713 MB resting the extension host is lifted to 1069 MiB, above the 1024 MiB pod share, and the derivation reports each budget the floor lifted (
floored=1) — that means the pod is too small to bound the role at its intended share, which is worth saying out loud. The fixture suite asserts the property (no budget at or below resting, at any pod size) rather than the arithmetic.VmDatais no longer used: it mattered only becauseRLIMIT_DATAaccounts it.Not becoming harmful
What it may touch
Two populations, found two ways:
~/.vscode-server(unchanged).sid= its login shell's session, while every Bash tool call haspgid == sid == its own pid, because Claude Code detaches each one. That keeps an in-flight build out of the policed set even when the tool call's shell has exec'd itself away.Identity guards (
pid 1, the agent, tmux, session roots, agent payloads, watchdog kin) stay absolute. The two positional rules — the ptyHost subtree and "does not run VS Code's own binary" — now bound the editor selection only, so an MCP server is policed the same whether its session started undercoder sshor in a VS Code terminal.Sweep log, and action lines that leave the pod
~/.local/state/vscode-memory-watchdog/sweep.log(rotating, plussweep.latest,summary,top): one row per policed process and per unmanaged process above 32 MB — role, PSS, RSS, age, budget, seconds over budget, which guard claimed it, and a stable identity breadcrumb (an MCP server's module, notpython3). Secrets in argv are redacted at the point of writing.Actions also go to Loki, for free. The log agent tails container stdout and nothing else — but PID 1 in this container is the coder agent, so
/proc/1/fd/1is container stdout. Action lines written there arrive labelled by namespace, pod and container with no new infrastructure and no configuration anywhere.component=memory-watchdog, so{namespace="coder"} |= "component=memory-watchdog" | logfmtworks against a very chatty agent stream without anyone adding a stream label. Free text is quoted intodetail=by the writer, not by call sites, so a forgotten quote cannot turn a sentence into five bogus fields.event=would-kill, which is exactly the evidence needed to answer "would this have fired too often" before arming it.Verified end to end: the daemon's own lines from the test workspace are in Loki and parse into 18 logfmt fields.
Mode
memory_watchdog_modebecomesobserve/enforce(helpers — the new default) /enforce-all(adds extension host and serverMain, which restart visibly).A defect this introduced, and the guard for it
The first run of the fixture suite after the stdout path was added — before the harness set its seam — wrote fixture
event=kill role=extensionHost pss_mb=1907lines into the live workspace's container log, and thus into Loki, describing kills that never happened in the exact format a post-mortem would trust. Nothing was signalled (killis shadowed in the suite). A correcting note was appended to the same stream, andload_watchdognow setsWATCHDOG_STDOUT_PATHand the suite exits if the seam did not take. A test fixture that can write into production telemetry is a defect in the test.Evidence
Fixtures: 276 assertions, negative cases paired with the mutation that must flip them;
killis shadowed so fixture pids cannot signal real processes, and stdout is redirected to a temporary file.Live, on the disposable test workspace,
enforcewith the dwell shortened:serverMainclassified correctly and read 73 MB PSS;event=censusandevent=budgetlines reached Loki and parsed cleanly, showingbudget_mb=1069 floored=1for the extension host against apod_share_mb=512on that 4 GiB pod.Two things found live and fixed: budgets printed as
0Min the summary (an associative-array subscript inside$(( ))reads the key, not the variable), and the walk originally policed a detached tool call until the session rule was added.Where acute protection now sits
singleProcessOOMKillis in effect: a pod created after the kubelet rollout readsmemory.oom.group = 0, so an OOM takes the offending process only. A container created before it still reads1, because the value is fixed when the container is created — which is why an earlier revision of this description claimed the opposite from a single old-pod sample. The kernel is the first line for the acute case; the runway this PR protects is the second.That setting was applied to the nodes by hand and exists in no repository, so a node rebuild or the Talos migration would silently restore all-or-nothing OOM behaviour — and this template's stated rationale for not attempting acute protection would quietly become wrong. Raised as
ppat/homelab-ops-kubernetes-clusters#948and referenced from DESIGN.md.Other corrections to the premises this was started from
Open question, deliberately not answered here
Whether Claude Code respawns a stdio MCP server it did not kill itself. VS Code demonstrably respawns every helper in its own set; the agent side is assumed and untested, because testing it needs a real stdio MCP server. If it turns out not to, the fix is one parameter value (
observe) or one budget override, not a code change — and the first day ofsweep.logwill show how often it would matter.Supersedes the calibration half of #860; the observe→enforce decision that issue tracks is now this parameter.