fix(#418): .wslconfig memory can't be written below the client's own floor - #516
Merged
Conversation
…floor
Get-WslConfigMemoryGb writes the WSL2 memory budget into the daily user's
.wslconfig during elevated provisioning. It did its own arithmetic -- physical
RAM minus a PRIVATE 4 GB reserve, floored at 1 GB -- so it could persist a
budget the client cannot run in, and it contradicted the installer's own advice
in the same run.
Measured before ($env:TB_PESTER="1"; . scripts/install-k8s.ps1):
host writes advises (run / train)
8 4 6 / 6 <- below the 5 GB floor: guaranteed OOM crashloop
16 12 8 / 14
32 28 8 / 16 <- over-committed: Windows left 4 GB
6 2 4 / 4 <- doomed budget persisted as if intentional
4 1 2 / 2
8 GB is the important case: a perfectly viable host (8 - 2 GB OS reserve = 6,
clear of the 5 GB floor) got memory=4GB while Show-MemoryStatus in the same run
said "give Docker up to 6 GB".
Rather than re-derive a floor here, the function now DELEGATES to
Get-PfMemRecommendation -- the existing single-source helper the advice path
uses. The written budget is therefore the advised budget by construction, not
by two calculations agreeing:
host writes advises (train)
7 memory=5GB 5 (= the floor; 5+2 is the stated practical minimum)
8 memory=6GB 6 (was 4 -> OOM)
16 memory=14GB 14 (was 12)
32 memory=16GB 16 (was 28; Windows keeps 16, and the client cannot
64 memory=16GB 16 use more than the recommended training budget)
6 skipped -
4 skipped -
Reserve reconciled: the private ReserveGb=4 is gone and the PARAMETER is gone
with it, so no caller can reintroduce the drift. $script:PfOsReserveGb (2) is
now the only reserve -- which is what its own comment already claimed ("used to
cap recommendations AND to reason about the achievable budget in one place, so
the two can't drift", #417 reviewer). Three accessors (Get-PfOsReserveGb /
Get-PfMinMemGb / Get-PfRecMemGb) give the printing and writing paths one read
path for the same numbers; the reserve accessor fails closed (never 0, which
would hand WSL2 the entire host).
Host genuinely too small (physical - reserve < floor): the function returns 0
and the caller SKIPS the memory setting instead of persisting a budget known to
OOM -- which would also bake the failure into the daily account for every later
run and read as intentional to whoever debugs it next. It says so plainly
instead ("about 7 GB physical is the practical minimum ... use a larger
machine"), the same honest framing the macOS path landed in #513. Still
warn-only: Set-DailyUserProvisioning is documented never to fail the install,
and Windows memory preflight is warn-only throughout.
No headroom fudge is needed on Windows, unlike colima in #513: only a 7 GB host
lands exactly on the floor, 7 GB is precisely the practical minimum this code
reports, and the Windows recheck warns rather than hard-failing -- so there is
no catch-22 where the installer hard-fails on the size it just chose.
Scope: Show-MemoryStatus and Get-PfMemRecommendation are deliberately untouched
so this does not collide with the open #444 (which rewrites both). Verified
forward-compatible -- with #444's floored recommender simulated, every host
above returns an identical result, because the too-small gate tests the
achievable ceiling rather than the recommender's output.
Tests: the two assertions that pinned the old cap/floor are replaced by the new
contract -- never below the floor, 8 GB -> 6 not 4, 0 (don't write) when the
host can't reach the floor, never over-commits, caps at the training budget,
PF_MIN_MEM_GB override honoured, no ReserveGb parameter, and a lock asserting
written == advised across seven host sizes. Plus source guards that the caller
gates the write on 0 and prints the honest too-small message.
Gates: Invoke-Pester scripts/tests/ -> 377 passed / 0 failed;
Invoke-ScriptAnalyzer (as CI scopes it) -> 0 errors; check-style.sh clean;
check-drift.sh no drift; manifest.sha256 regenerated.
Refs #418
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
bugbot run |
Contributor
Author
|
👋 Heads-up — Code review queue is at 36 / 30 Above the WIP limit. The team convention is to review existing PRs before opening new work. Open PRs currently in Code review (oldest first):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 28a89cf. Configure here.
shujaatTracebloc
approved these changes
Jul 31, 2026
shujaatTracebloc
left a comment
shujaatTracebloc
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the logic end-to-end — LGTM. ✅
Correctness verified. The fix delegates Get-WslConfigMemoryGb to the single-source Get-PfMemRecommendation = min(DesiredGb=16, HostGb − reserve), and the too-small gate returns 0 whenever HostGb − reserve < floor. So for any host that clears the gate, the written budget is in [floor, 16] by construction — never below the client's own PF_MIN_MEM_GB floor, never over the OS reserve. Traced every host size and each reproduces the tables exactly: 7→5, 8→6 (was the OOM-guaranteed 4), 16→14, 32→16 (was 28), 64→16; 6/4/2/0 → skip. The 0-return is gated in Set-DailyUserProvisioning before .wslconfig is touched, so no known-OOM budget is ever persisted — it warns with the honest practical minimum instead.
Reserve reconciled. The private ReserveGb=4 param is gone, leaving $script:PfOsReserveGb=2 as the only reserve (accessor fails closed). Confirmed the caller at line ~1948 is the sole production call site, so removing the param breaks nothing.
Tests. The two old assertions pinned the buggy 16→12 / 32→28 / 4→1 behavior and are correctly replaced by the new contract (never-below-floor, 8→6-not-4, 0-on-too-small, never-over-commit, written==advised drift lock, PF_MIN_MEM_GB override) — net coverage 2→10 It blocks.
Gates green: Pester 377/0, Lint, Static analysis, drift, manifest regenerated, full E2E matrix, Bugbot clean on 28a89cf with no open threads.
Nicely scoped away from #444 too. 👍
5 tasks
LukasWodka
added a commit
that referenced
this pull request
Jul 31, 2026
…or each #516 merged the WRITE half of this story (the daily-user .wslconfig could be sized below the client's own floor) and, to fix it, added accessors next to $script:PfOsReserveGb: Get-PfOsReserveGb / Get-PfMinMemGb / Get-PfRecMemGb. This branch had independently added its own inline reads of the same numbers, so the merge left three duplicate read paths for values whose whole problem was drifting apart. Only scripts/manifest.sha256 conflicted textually (both sides changed the install-k8s.ps1 hash); the duplication was the real conflict. Consolidated, so each threshold now has exactly ONE read site: - Get-PfMemRecommendation: inline $env:PF_MIN_MEM_GB read and the raw $script:PfOsReserveGb -> Get-PfMinMemGb / Get-PfOsReserveGb. The accessor also fails closed on the reserve (never 0, which would advise handing Docker the whole host). - Show-MemoryStatus: its three inline threshold reads -> accessors, plus one local $reserveGb for the hostTooSmall arithmetic and the two hints that name the reserve. - Added Get-PfWarnMemGb so all four thresholds are uniform. PF_WARN_MEM_GB was the only one #516 didn't need an accessor for, and leaving it inline is exactly the asymmetry that invites the next drift. Say the word if you'd rather keep this branch minimal and drop it. Verified the two halves compose, on the real merged code rather than by simulation. #444 floors Get-PfMemRecommendation at the client minimum, and #516's Get-WslConfigMemoryGb DELEGATES to that function -- so the floor now feeds the path that writes real config. On a 6 GB host the recommender returns 5 (its floor), and writing that would leave Windows 1 GB; the write path refuses anyway because it gates on the achievable ceiling (physical - reserve) rather than on the recommender's output. Combined result: host .wslconfig advice(train) agree 7 memory=5GB 5 yes 8 memory=6GB 6 yes 16 memory=14GB 14 yes 32 memory=16GB 16 yes 2/4/6 skipped - n/a (and no branch prints a memory= value) New no-drift guard (Describe "Memory thresholds are single-sourced"): each PF_* threshold is read on exactly one LINE (counting lines, not occurrences -- an accessor names its var twice on one line, in the truthiness test and the [int] cast), the reserve is read only by its accessor, the accessors match the documented defaults, and the advice path and the write path agree for every host that can reach the floor while writing nothing for every host that can't. Mutation-tested the guard rather than trusting it: reintroducing the inline $env:PF_MIN_MEM_GB read and the raw $script:PfOsReserveGb fails both drift assertions, and restoring passes all four. Gates: Invoke-Pester scripts/tests/ -> 392 passed / 0 failed (this branch's tests and #516's together); Invoke-ScriptAnalyzer as CI scopes it -> 0 errors; check-style.sh clean; check-drift.sh no drift; manifest.sha256 regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Jul 31, 2026
develop moved again while this branch sat waiting for review: #517 (drop the dead Get-WslConfigContent + its test) and #434 (RFC-CLIENT-0003 docs). Both touch the same install-k8s.ps1 hash line in scripts/manifest.sha256, which is the only textual conflict; the PowerShell and test files auto-merged. Verified the merge rather than trusting it: - #517's removal came through cleanly -- Get-WslConfigContent is gone from both the installer and the test file (0 references in each). - This branch's + #516's accessor work is intact (15 accessor references). - Pester 391 passed / 0 failed. One fewer than the 392 before this merge, which is exactly the Get-WslConfigContent test #517 deleted -- not a lost assertion. Gates: Invoke-ScriptAnalyzer as CI scopes it -> 0 errors; check-style.sh clean; check-drift.sh no drift; manifest.sha256 regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Jul 31, 2026
Bugbot caught a self-contradiction I had reasoned about and wrongly accepted: the grade was computed from floor($mib / 1024), so a VM configured at exactly the 5 GB floor (reporting ~4800 MiB) became budget 4, and Show-MemoryStatus printed hard-floor "it will OOM" copy plus a resize hint -- for a machine the grace-aware gate immediately ACCEPTED. We told a correctly configured box it would crash and then carried on. That is precisely the "installer contradicts itself in the same run" pattern #418/#516 existed to remove; bash classifies that band warn-only. I had rejected rounding because Step-1 floors and #417 exists so the reported figure doesn't flip-flop. The fix avoids that trade-off entirely: fold the SAME grace in before flooring. $budget = floor(($mib + $grace) / 1024) - 4800 + 512 -> 5: reports the CONFIGURED size (what the user set and can change), grades in the warn band, gate passes. Consistent. - 4096 + 512 -> 4: still sub-floor, still "it will OOM", gate still fails. Consistent. Because the grade and the gate now pivot on the same constant, their boundaries are the same boundary -- (floor * 1024 - grace) MiB. There is no band that warns "will OOM" yet proceeds, and none that passes while being called sub-floor. The contradiction is impossible by construction, not merely absent at the values I happened to test. Tests: the floor-sized VM is asserted NOT to be told it will OOM and to report its configured 5 GB; plus a boundary-coincidence test sweeping 4096/4607/4608/ 4800/5120 that asserts at EVERY point the copy and the gate agree. Mutation-tested: reverting the grade to floor($mib / 1024) fails both new tests; restoring passes all 13. Gates: Invoke-Pester scripts/tests/ -> 403 passed / 0 failed; Invoke-ScriptAnalyzer as CI scopes it -> 0 errors; check-style.sh clean; check-drift.sh no drift; manifest.sha256 regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Jul 31, 2026
… proceed (#520) * fix(installer): Windows was the one OS that let a sub-floor Docker VM proceed #513 decided a Docker VM below the client's memory floor must STOP the install rather than proceed and OOM-crashloop -- "proceeding is worse than the jarring stop the WARN path used to avoid" -- and implemented that in bash for every OS (_pf_recheck_runtime_mem -> error -> exit 1). The Windows installer never got it. Test-PreflightRuntimeMem just called Show-MemoryStatus, which is warn-only, so Windows printed "it will OOM" and then carried on and OOM-crashlooped. The platform this whole memory story (#417/#418/#428/#444/#516) is about was the one platform still shipping the crash. Enforcement now lives in Test-PreflightRuntimeMem, mirroring bash's split: Show-MemoryStatus stays purely presentational (its documented job -- and the function two PRs just contended over), the recheck grades then enforces. It runs as New-K3dCluster's FIRST statement, so exiting leaves no half-built cluster. The subtlety that makes this safe: Get-PfRuntimeMemGb floors to whole GB, and a guest reports a few hundred MiB BELOW its configured size, so a VM set to exactly the documented 5 GB floor reports ~4.8 and floors to 4. A bare `-lt 5` would have hard-failed a correctly configured machine -- the same trap #513's reviewer caught in bash, but worse here because flooring to whole GB discards up to a GB. So the gate compares MiB against floor - grace: - New Get-PfRuntimeMemMib: the same `docker info` value at MiB precision. - New Get-PfVmMemGraceMib (512, PF_VM_MEM_GRACE_MIB) -- the same constant and the same comparison bash uses, so both installers put the floor in the same place. - The recheck now reads the budget ONCE, in MiB, and derives GB from it, so the number printed and the number enforced on cannot disagree. Flooring (not rounding) is kept deliberately: Step-1 floors too, and #417 exists so the reported figure doesn't flip-flop between the two reads. Remedies stay honest and achievable, matching the copy the advice path already prints: a host that CAN reach the floor gets a resize target clamped to it (min(warn, physical - reserve) -- bash's clamped warn target); a host that cannot (physical - reserve < floor) gets the practical minimum and "run the client on a larger machine", never a resize that repeats an impossible size. A between-floor-and-warn budget still only warns -- it can run, just tightly. TRACEBLOC_SKIP_PREFLIGHT still overrides, and Err names it. Tests: the Describe that asserted warn-only is replaced by the new contract -- sub-floor 4 GB hard-fails; a floor-sized VM reporting 4800 MiB passes; the grace band is bounded on both sides (4607 fails, 4608 passes); daemon-silent is a no-op; between-floor-and-warn only warns; the rec is still capped at host RAM; big-host vs host-too-small remedies; host RAM unreadable still fails; the skip env overrides; and the budget is read exactly once. Plus a parity Describe that reads BOTH sources and asserts bash still hard-fails, Windows hard-fails too and is no longer warn-only, and both name the same grace constant -- so the next divergence fails a test instead of shipping. The old tests mocked Get-PfRuntimeMemGb, which this no longer calls; left as-is they would have passed while testing nothing, so they now mock the MiB reader. Mutation-tested rather than trusted: neutering the gate back to warn-only fails 4 enforcement tests; restoring passes all 11. Gates: Invoke-Pester scripts/tests/ -> 401 passed / 0 failed; Invoke-ScriptAnalyzer as CI scopes it -> 0 errors; check-style.sh clean; check-drift.sh no drift; manifest.sha256 regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): make the grade and the gate share one boundary (Bugbot) Bugbot caught a self-contradiction I had reasoned about and wrongly accepted: the grade was computed from floor($mib / 1024), so a VM configured at exactly the 5 GB floor (reporting ~4800 MiB) became budget 4, and Show-MemoryStatus printed hard-floor "it will OOM" copy plus a resize hint -- for a machine the grace-aware gate immediately ACCEPTED. We told a correctly configured box it would crash and then carried on. That is precisely the "installer contradicts itself in the same run" pattern #418/#516 existed to remove; bash classifies that band warn-only. I had rejected rounding because Step-1 floors and #417 exists so the reported figure doesn't flip-flop. The fix avoids that trade-off entirely: fold the SAME grace in before flooring. $budget = floor(($mib + $grace) / 1024) - 4800 + 512 -> 5: reports the CONFIGURED size (what the user set and can change), grades in the warn band, gate passes. Consistent. - 4096 + 512 -> 4: still sub-floor, still "it will OOM", gate still fails. Consistent. Because the grade and the gate now pivot on the same constant, their boundaries are the same boundary -- (floor * 1024 - grace) MiB. There is no band that warns "will OOM" yet proceeds, and none that passes while being called sub-floor. The contradiction is impossible by construction, not merely absent at the values I happened to test. Tests: the floor-sized VM is asserted NOT to be told it will OOM and to report its configured 5 GB; plus a boundary-coincidence test sweeping 4096/4607/4608/ 4800/5120 that asserts at EVERY point the copy and the gate agree. Mutation-tested: reverting the grade to floor($mib / 1024) fails both new tests; restoring passes all 13. Gates: Invoke-Pester scripts/tests/ -> 403 passed / 0 failed; Invoke-ScriptAnalyzer as CI scopes it -> 0 errors; check-style.sh clean; check-drift.sh no drift; manifest.sha256 regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
added a commit
that referenced
this pull request
Jul 31, 2026
develop moved a long way while this sat open (#513 macOS memory floor, #518 storage remedy, #516/#444/#517/#520 on the Windows side, #434 RFC docs). Only scripts/manifest.sha256 conflicted textually — the same single install-k8s.ps1 hash line that has now collided four times today. preflight.sh and preflight.bats auto-merged. Resolution: regenerate the manifest (it is a DERIVED artifact — 18 digests, no secrets; authenticity comes from the release workflow's cosign signature, not from git), so regenerating is the only correct resolution. Taking either side would leave a wrong digest, which the R8 gate then rejects. Verified the auto-merge rather than trusting it. The real hazard here was not the conflict but the clean-looking merge: this branch DELETES _pf_total_mem_kb (the "prefer the runtime" memory selector whose conflation of host RAM and VM budget is the bug it fixes), so any caller that landed on develop meanwhile would have merged into a call to a function that no longer exists — a silent break git reports as success. - _pf_total_mem_kb: undefined and unreferenced after the merge; the only mentions are this branch's own guard test asserting its absence, and a comment. - #518's _pf_storage_type network-FS remedy survived intact. - This branch's _pf_runtime_mem_status is present and still wired into both _pf_memory and _pf_recheck_runtime_mem. Gates: bats scripts/tests/*.bats -> 683 ok / 0 not ok (full TAP plan reported, not a truncated read); shellcheck --severity=error over the CI file set -> rc=0; bash -n clean; Pester -> 403 passed / 0 failed (install-k8s.ps1 arrived via this merge); check-style clean; check-drift no drift; gen-manifest.sh --check current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
/fr-pass |
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
2 participants
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.
Summary
Get-WslConfigMemoryGbwrites the WSL2 memory budget into the daily user's.wslconfigduring elevated provisioning (#418). It did its own arithmetic — physical RAM minus a private 4 GB reserve, floored at 1 GB — so it could persist a budget the client cannot run in, and it contradicted the installer's own advice in the same run.Measured on
develop($env:TB_PESTER="1"; . scripts/install-k8s.ps1):.wslconfigwrites8 GB is the case that matters: a perfectly viable host (8 − 2 GB OS reserve = 6, clear of the floor) got
memory=4GB, whileShow-MemoryStatusin the same run said "give Docker up to 6 GB".The fix
Rather than re-derive a floor here, the function now delegates to
Get-PfMemRecommendation— the existing single-source helper the advice path already uses. The written budget is the advised budget by construction, not by two calculations happening to agree:memory=5GBmemory=6GBmemory=14GBmemory=16GBmemory=16GBReserve reconciled. The private
ReserveGb = 4is gone, and the parameter is gone with it, so no caller can reintroduce the drift.$script:PfOsReserveGb(2) is now the only reserve — which is what its own comment already claimed: "used to cap recommendations AND to reason about the achievable budget in one place, so the two can't drift" (#417 reviewer). Three accessors (Get-PfOsReserveGb/Get-PfMinMemGb/Get-PfRecMemGb) give the printing and writing paths one read path for the same numbers; the reserve accessor fails closed (never 0, which would hand WSL2 the entire host).Host genuinely too small (physical − reserve < floor): the function returns
0and the caller skips the memory setting rather than persist a budget known to OOM — which would also bake the failure into the daily account for every later run, and read as intentional to whoever debugs it next. It says so plainly instead ("about 7 GB physical is the practical minimum … use a larger machine"), the same honest framing the macOS path landed in #513.Still warn-only:
Set-DailyUserProvisioningis documented never to fail the install, and Windows memory preflight is warn-only throughout.Two judgement calls worth reviewing
No headroom fudge, unlike colima in #513. That PR had to size at
floor + 1because a guest'sMemTotalruns a few hundred MiB under its configured size and the bash recheck hard-fails — so a floor-sized VM tripped on the size the installer had just chosen. Windows doesn't have that catch-22: only a 7 GB host lands exactly on the floor, 7 GB is precisely the practical minimum this code reports, and the Windows recheck warns rather than hard-failing. Afloor + 1rule here would instead declare 7 GB "too small" and contradict our own stated minimum.Deliberately no overlap with #444.
Show-MemoryStatusandGet-PfMemRecommendationare untouched, since open #444 rewrites both — this is not stacked on it and does not need it. Verified forward-compatible: with #444's floored recommender simulated, every host in the table returns an identical result, because the too-small gate tests the achievable ceiling rather than the recommender's output. (#444 may want to adopt the three accessors instead of its inline$env:PF_MIN_MEM_GBreads.)Test plan
The two assertions in
Describe "Daily-user provisioning (#418)"that pinned the old cap/floor (16 -> 12,32 -> 28,4 -> 1) are replaced by the new contract:PF_MIN_MEM_GBoverride honouredReserveGbparameter is absent, so the drift can't returnPlus source guards that the caller gates the write on
0, and prints the honest too-small message with the practical minimum.Gates
Invoke-Pester scripts/tests/Invoke-ScriptAnalyzer(as CI scopes it:install.ps1+install-k8s.ps1)bash scripts/check-style.shbash scripts/tests/check-drift.shbash scripts/gen-manifest.shscripts/manifest.sha256regenerated + committed (R8)Refs #418
🤖 Generated with Claude Code
Note
Low Risk
Installer-only provisioning and preflight alignment; warn-only behavior unchanged, with expanded Pester tests locking the contract.
Overview
Daily-user
.wslconfigmemory no longer uses a separate formula (old 4 GB OS reserve, floor at 1 GB).Get-WslConfigMemoryGbnow callsGet-PfMemRecommendationwith the same reserve, min, and recommended training values as preflight, so writtenmemory=matches on-screen advice (e.g. 8 GB host → 6 GB, not 4).Too-small hosts get
0from that helper; provisioning skips writing memory, warns with the practical minimum RAM, and does not persist a budget below the client floor.Single source of truth: new accessors
Get-PfOsReserveGb,Get-PfMinMemGb, andGet-PfRecMemGb; the privateReserveGbparameter is removed. Pester coverage andscripts/manifest.sha256are updated accordingly.Reviewed by Cursor Bugbot for commit 28a89cf. Bugbot is set up for automated code reviews on this repo. Configure here.