fix(vmpool): correct the stale --cgroup comment at the jailer arg site

[pdettori]

Summary

Comment-only fix. Four comments across three files still documented a --cgroup flag that #319
deliberately removed, and did so in terms that invite putting it back.

firecrackerCgroupArgs returns only --cgroup-version 2 --parent-cgroup <rel>. Its body has said so
since #319 — its body opens "NO --cgroup (#258)" — because creating a cgroup per VM cost
195.52 ms of a 253 ms Destroy at 64 slots plus an unbounded dying-cgroup population; removing it
was worth 3.25x. The comments around that body did not get the memo.

What was stale

• launcher_firecracker.go, the call site. Described --cgroup as "load-bearing, not decorative".
• launcher_firecracker.go, firecrackerCgroupArgs' own doc comment. The worse copy, since it is
  what godoc renders and what any pointer to the function leads to. Three separate clauses: "returns
  the jailer flags that create and bound this VM's own cgroup" (it creates nothing and bounds
  nothing), "and --cgroup memory.max=<bytes> (D1: the flag that actually creates the per-VM cgroup
  at all)" (not returned), and "so a test can assert the memory bound" (that test reads
  memory.max off the pooled cgroup; what it asserts about this function is flags-only).
• launcher_chv.go. "Split out of Restore's argv construction, like firecrackerCgroupArgs,
  so a test can assert the memory bound" — an analogy that inherited the pre-perf(vmpool): reuse per-VM cgroups from a pool instead of creating one per VM (#258) #319 property. CHV's
  -p MemoryMax= does still carry a bound, so the sentence is true of itself and false of its
  comparison.
• cgroup.go. Three more, found by sweeping the package rather than the diff. The header's
  arm-agnostic paragraph pointed at firecrackerCgroupArgs while asserting jailer makes the cgroup;
  the slice tree diagram showed only the pre-perf(vmpool): reuse per-VM cgroups from a pool instead of creating one per VM (#258) #319 vm-<n> form; and vmIDPrefix claimed "The
  Firecracker arm's cgroup directory IS that id" — contradicted by pooledCgroupPrefix 28 lines
  below in the same const block, which says a vm-<n> cgroup present at startup is an orphan.

The fix

Each says what the code actually does, and keeps the part that still matters — why D1's per-VM
memory bound survives without --cgroup: cgroupPool writes memory.max = PerVMBytes on the
pooled cgroup the jailer relocates into, rather than jailer writing it on a fresh one. That is the
thing a future reader needs in order not to worry, and it now lives on the function rather than at one
call site.

The CHV comment states the asymmetry explicitly instead of the analogy, so neither arm is read off the
other. Attribution is exact throughout: #258 is the measurement issue that produced the 195.52 ms
figure, #319 is the PR that dropped the flag.

dc6fe82 is a follow-up on my own work rather than on the code: fixing an over-long line in
f8dad58 left a 31-character fragment breaking mid-sentence one line down. gofmt does not reflow
comment prose, so it passed every check twice — the same blind spot that let the stale comments
survive #319. The paragraph is reflowed end to end; prose unchanged.

What this does not fix

Nothing, because nothing was broken. cgroup_test.go:436-441 already fails if --cgroup or
memory.max= reappears in the args, with a message naming the churn #258 removed. So the cost of
these comments was a confused reader and, for anyone who believed them, a red CI run — not a latent
3.25x regression. This is a docs bug, and the reader-confusion half is the whole of what it closes.

Testing

• Comment-only, checked mechanically rather than by eye: every added and removed line in the diff
  matches ^[+-]\s*//. dc6fe82 additionally asserts the reflowed prose is byte-identical to the
  original once the lines are rejoined, so only line breaks moved.
• gofmt -l clean, go build ./... and go vet clean, make lint passes.
• Full vmpool suite passes with -count=1; all 9 remote-worker packages pass. The
  cgroup-specific filter matched 32 tests, counted explicitly rather than trusting a bare ok —
  a -run pattern that matches nothing also prints ok.

Related

Follow-up to #319. Independent of the three docs PRs in flight (#300, #301, #327) — different files,
no conflict.

Assisted-By: Claude Code

[huang195]

The correction is right, and I verified both halves of it against the head SHA: firecrackerCgroupArgs really does return only --cgroup-version 2 --parent-cgroup <rel> (line 168), and cgroupPool really does write memory.max from the value threaded in as PerVMBytes (cgroup_pool.go:112, wired at launcher_firecracker.go:189). Comment-only confirmed — every added line is inside a comment block.

One blocker: there are two stale copies of the claim, and this PR fixes one and then points at the other. Details inline on the see its own doc comment line.

Also worth sweeping (can't comment inline — file isn't in the diff)

remote-worker/internal/vmpool/launcher_chv.go:384-386:

Split out of Restore's argv construction, like firecrackerCgroupArgs, so a test can assert the memory bound agrees with PerVMBytes(cfg) without spawning systemd-run.

That analogy inherited the pre-#319 property. CHV's own -p MemoryMax= still carries a bound, so the sentence is true of this function — but the Firecracker function it compares itself to no longer carries one. Same defect class as the one being fixed here, and since this PR is precisely a #319 cross-reference sweep, it seems in scope. Non-blocking.

One correction to the PR description's threat model

The stated risk — "the plausible reaction to the stale one is to 'restore' the missing flag and reintroduce the regression" — is already guarded. cgroup_test.go:436-441 fails on --cgroup or memory.max= appearing in the args, with a message that names the churn #258 removed. So the realistic cost of a stale comment here is a confused reader and a red CI run, not a shipped 3.25x regression. That doesn't change the finding below — the reader-confusion half is exactly what the doc comment fix closes — but it's worth having in the record, since it means the stale doc comment is a docs bug rather than a latent perf bug.

---

CI: all 13 checks green. DCO signed. Assisted-By used, no Co-Authored-By. Title prefix conforms.

[huang195]

must-fix — this resolves the caller/callee contradiction by deferring to the callee, but the callee's doc comment is the second copy of the claim being removed, and this PR doesn't touch it. Lines 147-154 on this branch are byte-identical to base (I diffed the region):

// firecrackerCgroupArgs returns the jailer flags that create and bound this VM's own
// cgroup: --cgroup-version 2 (...), --parent-cgroup (...), and --cgroup
// memory.max=<bytes> (D1: the flag that actually creates the per-VM cgroup at all;
// --parent-cgroup alone only relocates the process into the shared parent). Split out
// of Restore's argv construction so a test can assert the memory bound agrees with
// PerVMBytes(cfg) without spawning jailer.

So "deliberately NOT --cgroup: see its own doc comment" sends the reader to text asserting the function returns --cgroup memory.max=<bytes>, described as "the flag that actually creates the per-VM cgroup at all" — the exact claim this PR exists to remove. The contradiction isn't eliminated, it moves one hop; and it now has an explicit pointer aimed at the stale side. Before this PR the two disagreeing comments sat ~250 lines apart with no cross-reference, so the pointer is new in this diff — which is why I'd fix it here rather than defer it.

The PR description's premise is what makes this load-bearing:

firecrackerCgroupArgs now opens with "NO --cgroup (#258)"

It doesn't. That's the first line of the function body (line 159). The doc comment is what a reader hits first when they follow this pointer, and it's the only part godoc renders.

Two further clauses in those same lines have gone stale the same way:

• "returns the jailer flags that create and bound this VM's own cgroup" — it now creates nothing and bounds nothing; both happen in cgroupPool.
• "so a test can assert the memory bound agrees with PerVMBytes(cfg)" — the test that asserts PerVMBytes reads memory.max off the pooled cgroup (cgroup_test.go:420-427). What it asserts about this function is flags-only: --parent-cgroup present, --cgroup-version 2 present, --cgroup/memory.max= absent (:429-441).

Suggested shape: fix 147-154 in this PR, and this call-site comment can then shrink to a bare pointer — most of what it's carrying belongs on the function anyway. If you'd rather do the doc comment as a follow-up, then drop "see its own doc comment" from this line, because as written the diff ships a live pointer to text that contradicts it.

[pdettori]

Fixed in f8dad58. You were right on every part of it, including the part about my own PR description.

Confirmed before fixing: lines 147-154 are byte-identical to base (git show main:...launcher_firecracker.go | sed -n '147,155p' matches the working tree exactly), so the diff really did ship a live pointer into text contradicting it.

And "opens with NO --cgroup (#258)" is line 159, the function body, not the doc comment — my description was wrong, and that error is what made the pointer look safe to write. Corrected in the description.

All three stale clauses you isolated are now gone, and I checked each against the code rather than taking the shape on faith:

• "create and bound this VM's own cgroup" — cgroupPool.create does both (cgroup_pool.go:103-120: mkdirAllCgroup then writeMemoryMax). The function creates nothing and bounds nothing.
• the --cgroup memory.max=<bytes> clause — gone from the return since perf(vmpool): reuse per-VM cgroups from a pool instead of creating one per VM (#258) #319.
• "so a test can assert the memory bound" — cgroup_test.go:420-427 reads memory.max off pool.acquire()'s directory; :429-441 is the flags-only part. The doc comment now says it that way, and points the bound at the pool.

I took your suggested shape: the substance moved onto the function, and the call site is now a two-line pointer. That also dissolved the other two nits — see the replies there.

The sweep turned up three more of the same class in a file neither of us had in the diff, cgroup.go. Worst of them: vmIDPrefix (:100) says "The Firecracker arm's cgroup directory IS that id", and pooledCgroupPrefix 28 lines below in the same const block says "a vm-<n> cgroup present at startup is an orphan to be swept". #319 added the second and left the first. Also the header's arm-agnostic paragraph (:19-20), which pointed at firecrackerCgroupArgs while asserting jailer makes the cgroup — same defect as this one, same pointer shape — and the slice tree diagram (:44), which showed only the pre-#319 vm-<n> form. All three fixed in the same commit.

No functional fallout from any of it: I checked that the sweep's allowlist does handle the pooled names (isPooledCgroupDirName, cgroup.go:221), so the stale prose was never covering a real gap.

[huang195]

nit — #258/#319 removed it attributes the removal to an issue. #258 is "P4: decompose resume (28.58 ms) and destroy (21.31 ms) into their sub-costs" — the measurement work that produced the 195.52 ms figure this line goes on to quote. #319 is the PR that actually dropped the flag.

which #319 removed (#258) would be exact, and it matches the convention already in this file: line 159 uses the bare #258 as the motivating issue, not as the change.

[pdettori]

Verified and fixed. gh api repos/.../issues/258 → issue, "P4: decompose resume (28.58 ms) and destroy (21.31 ms) into their sub-costs"; .../319 → PR, "perf(vmpool): reuse per-VM cgroups from a pool instead of creating one per VM (#258)". So #258/#319 removed it credited the removal to the measurement.

The line is gone rather than reworded — it lived in the archaeology sentence, which came out for your other nit. The distinction is now applied consistently across all four comments the commit touches: #319 wherever the removal is the subject, bare #258 where the motivating measurement is, matching line 159's existing convention.

[huang195]

nit — three of the six new lines (404-406) describe what a previous version of this comment claimed. git blame already holds that, and a comment about an obsolete comment leaves the next reader two dead states to carry instead of one.

The PR description argues for it deliberately ("records that the claim was true before #319 so the history reads correctly"), so this is a judgment call rather than a defect — noting only that the tighter version is also the shorter one: what the function appends, why --cgroup is out, and how D1's bound survives. The last of those three is the part you identified as the thing a future reader actually needs, and it's currently the shortest section.

[pdettori]

Agreed, and dropped. The archaeology is gone; the call site is now:

// firecrackerCgroupArgs appends --cgroup-version and --parent-cgroup, and deliberately
// not --cgroup: see its doc comment.

Your framing is what decided it — "two dead states to carry instead of one" is the actual cost, and git blame holds the history for free. I'd argued for it on the grounds that the history should read correctly, but that argument only works if the history is somewhere, not necessarily inline.

This also fell out of your must-fix for free: once the doc comment carries the substance, there is nothing left at the call site for the archaeology to be attached to, and the three sections you named — what it appends, why --cgroup is out, how D1's bound survives — are now the whole of the doc comment, in that order.

[pdettori]

All four items addressed in f8dad58 — the blocker, both nits, and the launcher_chv.go sweep. Plus three more of the same class that the sweep turned up in cgroup.go. Details in the threads; the PR description is rewritten to match.

On the launcher_chv.go analogy (your out-of-diff item): fixed here rather than deferred, since you're right that a #319 cross-reference sweep that skips a cross-reference to #319 isn't one. Rather than just deleting "like firecrackerCgroupArgs", the comment now states the asymmetry, because the reason the arms differ is itself worth knowing and the next reader to notice -p MemoryMax= will wonder:

// The arms are NOT symmetric about that bound any more, so do not read one off the
// other: since #319 firecrackerCgroupArgs carries no bound at all — cgroupPool writes
// memory.max and jailer gets only --parent-cgroup — whereas this argv still carries
// its own, because systemd-run creates the scope's cgroup and nothing pools it.

On your threat-model correction: accepted, and it's now in the description under its own heading. I checked cgroup_test.go:436-441 and it does exactly what you say — fails on --cgroup or memory.max=, with a message naming the churn #258 removed. My "reintroduce the regression" framing skipped the guard standing between the stale comment and a merge. Docs bug, not a latent perf bug; the reader-confusion half is the whole of it.

cgroup.go, beyond the review. I swept the package rather than the diff, on the theory that if one cross-reference was stale, the file that owns the cgroup names was worth checking. Three hits:

Site	Stale claim
:19-20	arm-agnostic header: "Firecracker's jailer makes one via --cgroup…" — and pointed at firecrackerCgroupArgs, so the same defect as the blocker, same pointer shape
:44	slice tree diagram showed only the pre-#319 vm-<n> form
:100	vmIDPrefix: "The Firecracker arm's cgroup directory IS that id"

The last is the sharpest instance in the package: pooledCgroupPrefix, 28 lines below in the same const block, says "a vm-<n> cgroup present at startup is an orphan to be swept". #319 added that entry and left vmIDPrefix asserting the opposite — two comments contradicting each other inside one block whose own header calls itself "the single authority on what a VM's cgroup DIRECTORY is called".

One thing I checked before assuming it was cosmetic: whether the sweep's allowlist actually handles the pooled names, since a stale diagram plus a missed name would have been a real orphan leak. It does — isPoolVMCgroupDirName → isPooledCgroupDirName (:221) accepts pool-<n>, and TestSweepOrphansRecognisesBothArmsCgroupNames pins it. Comments only.

Verification. Comment-only asserted mechanically, not by eye — every added and removed line in the diff matches ^[+-]\s*//. gofmt -l clean, go build ./... and go vet clean, make lint passes. Full vmpool suite green with -count=1, all 9 remote-worker packages green, and the cgroup-filtered run matched 32 tests (counted, since a -run matching nothing also prints ok).

One self-inflicted defect worth naming, because gofmt will not catch it and it nearly shipped: my first rewrap of the cgroup.go header left a 120-char comment line. gofmt does not reflow comments, so it passed clean. Caught by measuring added comment widths against each file's existing maximum; the diff now peaks at 95, inside all four files' baselines.

[huang195]

The correction is complete and I verified it rather than eyeballing the shape: firecrackerCgroupArgs returns only --cgroup-version 2 --parent-cgroup <rel> (launcher_firecracker.go:174), cgroupPool.create does both the mkdir and the memory.max write (cgroup_pool.go:104-120), and pooledCgroupPrefix really is pool- (cgroup.go:135), so the new tree diagram is right.

All three findings from my earlier review are closed:

Prior finding	Status	Verified how
must-fix: the call site pointed at a doc comment that still asserted --cgroup memory.max=<bytes> is returned	Fixed	Doc comment rewritten at :147-159. Its three replacement claims all hold: creates nothing (the pool does), the bound moved to p.memMax, and the test really is flags-only (cgroup_test.go:429-441) with the bound read off pool.acquire() (:420-427).
nit: #258/#319 removed it credited the removal to the measurement issue	Fixed	Line deleted with the archaeology. #319 for the removal, bare #258 for the motivating measurement, applied consistently across all four comments.
nit: 3 of 6 call-site lines described a previous comment	Fixed	Call site is now two lines (:409-410), a bare pointer.

And the cgroup.go sweep you added on your own initiative is the better half of the PR. vmIDPrefix contradicting pooledCgroupPrefix inside the same const block was the worst instance in the package, and you found it by sweeping rather than by following my pointer. I checked each of the three:

• Header :18-21 — "cgroupPool creates the cgroup and jailer is merely relocated into it by --parent-cgroup" ✓
• Tree diagram :46 — pool-9/ matches the const ✓
• vmIDPrefix :102-112 — "the sweep must still recognise one" is real, not decorative: isPoolVMID (:207) cuts on vmIDPrefix, and isPoolVMCgroupDirName (:242) calls it ✓

launcher_chv.go:382-390 states the asymmetry outright now instead of via the analogy, and CHV's -p MemoryMax= does still carry its own bound, so the sentence is true of both halves. Bellissimo.

One cosmetic nit inline. Nothing blocking.

Summary

Author: pdettori (MEMBER — maintainer)
Areas reviewed: Go (comments only), commit conventions, CI
Agent/IDE config (.claude/.vscode): none
Comment-only claim: verified mechanically — zero changed lines fail ^[+-]\s*//
Commits: 2, all signed-off (2/2); Assisted-By used, no Co-Authored-By
CI status: passing (13/13)

Two footnotes on the description, neither affecting the code

1. line 159 opens "NO --cgroup (#258)" is now line 164 — the doc comment grew 8→13 lines in f8dad58 and pushed the body down. Same class of staleness this PR exists to fix, which is a decent argument for the code comments' choice not to cite line numbers at all.
2. The body footer says 🤖 Generated with [Claude Code]. The org convention is Assisted-By: Claude Code in PR descriptions too. Your commit trailers are already correct, and this repo's CLAUDE.md documents only the commit rule — so convention drift rather than a rule violation.

Perfetto otherwise. Approving; this dismisses my earlier CHANGES_REQUESTED.

[huang195]

nit — mamma mia, the rewrap left a stub. This line is 31 characters, wedged between a 91-char line above and a 90-char line below, and it breaks mid-sentence:

// below only ever walk directories and read/write the two files (cgroup.procs, memory.max)
// that both mechanisms produce
// under the SAME parent slice (spec §5.3). Neither function contains one line that knows

Every other line in this header block sits at 87-92 chars.

Pull under the SAME parent slice (spec §5.3). up onto this line and reflow through where SH_VMM was changed. Purely cosmetic, and gofmt will never catch it since it does not reflow comment prose — but on a PR whose whole thesis is that comment quality is worth its own commit, this one line reads like the edit stopped mid-thought.

[pdettori]

Fixed in dc6fe82. The paragraph is reflowed end to end, 85-92 characters, no fragment.

The prose is unchanged, and I asserted that rather than eyeballing it — rejoin the reflowed lines on spaces and compare to the original string. Only line breaks moved.

Two things went against the obvious choice, both worth recording:

The reflow splits systemd-run --scope across the break. I tried to avoid that and couldn't: the quoted command is an unbreakable 21-character token, so keeping it whole forces a ~75-character line at every wrap width. I measured 86 through 93 — the best case was [85, 86, 89, 87, 75, 76, 92, 92, 90, 83, 68], which trades your 31-char fragment for a 75/76 pair and more raggedness. Then I found chvScopeUnitPrefix (:114) already breaks the same command at the same place, so the split is this file's own idiom and costs a reader nothing here. Taking the band.

Your "87-92" is about this block, not the file. cgroup.go has a dozen-odd mid-paragraph comment lines at 73-81 (:36, :51, :58, :63, :66, :74, :95). So I matched the band for this block's sake rather than promoting it to a file-wide rule — which also means the fragment was jarring because of its neighbours, as you said, not because 31 is below some threshold.

One measurement footnote, since I nearly reported the flattering number: byte and character widths diverge in this paragraph, because it carries an em-dash (3 bytes) and a section sign (2). In characters the block is 85-92; in bytes 85-94. awk length() gave me the byte figures and Python the character ones, and I'd have quoted whichever I ran last. The file's pre-existing maximum is 97 either way, so nothing here is wider than what cgroup.go already carried — but that is two numbers that can drift, which is the trap this very file's const block warns about.

On your first footnote — line 159 had drifted to 164, because the doc comment grew 8 lines to 13 and pushed the body down. Fixed by deleting the number, not by chasing it: the description now says "its body opens". You're right that it argues for the code comments' choice never to cite line numbers, and it is a neat demonstration that a stale cross-reference is cheap to write and invisible until someone follows it — which is the whole thesis of the PR, landing on the PR.

On your second footnote — switched to Assisted-By: Claude Code. My commit trailers and my description disagreed with each other, which is its own small inconsistency regardless of which convention wins, and you're the maintainer stating the org one. Worth noting CLAUDE.md documents only the commit rule, so anything generating these will keep drifting until the description rule is written down too; happy to add it there in a separate PR if you want it pinned.

CI re-running on dc6fe82.

[pdettori]

CI is 13/13 green on dc6fe82. Noting why there is a red CI run in this branch's history, so nobody has to re-derive it:

The first run of proto on dc6fe82 failed on TestAnExecWaitingOnTheRunGateCanStillBeCancelled/timeout — serialize_test.go:208: condition not met within 2s. Known load flake in internal/vmpool, not a regression. Passed on re-run with no change.

The attribution here is unusually clean, so it is worth putting in the record rather than just asserting "flake": the diff between the CI run that passed (f8dad58) and the one that failed (dc6fe82) is one Go file, comment lines only — every changed line matches ^[+-]\s*//, checked mechanically. A comment cannot change test timing, so this is proof by construction rather than by bisect. 60 local runs of both known-flaky tests in that package (3 × -count=20) also passed clean, though that is corroboration; the inert diff is what settles it.

Third time this has surfaced (#296, #308, #330), always the proto job. Two things make it costlier than it needs to be: the deadline is a hard 2 s of wall-clock with no headroom under a loaded runner, and waitFor (replenish_test.go:184) reports neither which condition failed nor the value it last saw — so the message reads like a functional break. Scaling the deadline and giving waitFor a description would turn it into liveCount never reached 1 (last saw 0).

Deliberately not fixing that here: this PR is comment-only, and folding a test change in would cost the property that makes the diff trivially reviewable — and, as above, auditable as a control when CI flakes. Happy to open it separately if you want it.

Assisted-By: Claude Code