Skip to content

fix(installer): memory truth — machine RAM vs Docker's budget, advice clamped (bash half of #417) - #445

Merged
LukasWodka merged 10 commits into
developfrom
fix/417-memory-truth-bash
Aug 3, 2026
Merged

fix(installer): memory truth — machine RAM vs Docker's budget, advice clamped (bash half of #417)#445
LukasWodka merged 10 commits into
developfrom
fix/417-memory-truth-bash

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Bash half of #417 (Windows half: #444) — same three bugs, same design:

  1. Flip-flop:_pf_total_mem_kb preferred docker info MemTotal — the code's own test even called it "the Mac trap" — so a Mac reported 16 GB (host) cold and 6 GB (Docker VM) warm.
  2. Unachievable advice: remediation used the raw PF_REC_MEM_GB=16 regardless of the machine.
  3. Diverging copy: the preflight warning and the post-Docker _pf_recheck_runtime_mem warning disagreed, and both could fire in one run.

Change

  • _pf_memory gates on the machine (native Linux: daemon sees all host RAM → hard-fail gate unchanged; the 64 MiB grace and the Linux MemAvailable check are preserved) and prints Memory: N GB (machine); a second Docker's memory budget: N GB line appears only when a runtime is up with a meaningfully smaller budget (the VM case) — never as a duplicate on native Linux.
  • _pf_mem_targets clamps budget advice to host − 2 GB (floor 4); macOS hint covers both Docker Desktop and colima. Hardware statements stay unclamped.
  • _pf_runtime_mem_status is the single shared copy (preflight + recheck) with a PF_RUNTIME_MEM_WARNED latch.
  • _pf_hw_summary_line now reports host RAM (it had the same flip-flop in miniature).
  • The _pf_total_mem_kb selector and its trap-codifying tests are deleted; 6 new tests cover the clamp matrix, the two-line output, the Linux no-duplicate case, the latch, and the summary line.
  • manifest.sha256 regenerated.

Labels, clamp math, and latch semantics mirror #444 exactly — a ready-made row for the #435 facts spec.

Stacked on #441 (both touch preflight.sh); merge #441 first. Same local-macOS test-flake note as #442 applies (3 pre-existing, ubuntu CI is the arbiter).

Type

Bug fix

Test plan

preflight.bats fully green including 6 new tests; full bats suite green except the 3 documented pre-existing local-macOS failures; bash -n + shellcheck --severity=error clean; copy catalog unchanged; gen-manifest.sh --check clean.

🤖 Generated with Claude Code


Note

Medium Risk
Changes installer preflight gating and user-facing failure/warning text across OS paths; behavior is heavily tested but affects install abort decisions when Docker VM memory is sub-floor.

Overview
Bash preflight no longer conflates host RAM with Docker’s memory budget, fixing cold/warm flip-flop, contradictory grades, and duplicate warnings.

The _pf_total_mem_kb runtime-preferring selector is removed. _pf_memory gates on _pf_host_mem_kb as Memory: N GB (machine) and adds a separate Docker's memory budget line only when a VM budget is meaningfully smaller than the host (not duplicated on native Linux). _pf_display_gb_from_mib, _pf_host_too_small_for_floor, and _pf_runtime_mem_status centralize grace-aware GB display, floor+reserve “too small” logic, clamped resize hints (Desktop/colima vs Linux cgroup), and a PF_RUNTIME_MEM_WARNED latch so preflight and _pf_recheck_runtime_mem share one copy and threshold set. The hardware summary uses host RAM and the same converter. manifest.sha256 is updated; preflight.bats switches stubs to _pf_host_mem_kb and adds extensive regression tests.

Reviewed by Cursor Bugbot for commit d5b4172. Bugbot is set up for automated code reviews on this repo. Configure here.

@LukasWodkaLukasWodka self-assigned this Jul 27, 2026
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Comment threadscripts/lib/preflight.sh
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Converted to draft to disable the merge button while the base is a feature branch (the #438-#440 mis-merge trap). Plan: once #441 merges to develop, rebase this onto develop, retarget, regen manifest.sha256, re-run bats, flip ready.

@LukasWodka
LukasWodkaforce-pushed the fix/417-memory-truth-bash branch from 35a5930 to 22426edCompareJuly 27, 2026 15:35
@LukasWodka
LukasWodka changed the base branch from fix/432-nfs-guard-order to developJuly 27, 2026 15:35
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Rebased onto develop as a single clean commit (#441's content is already in via its squash). Manifest regenerated; preflight+common bats green locally (modulo the known macOS-only flake). bugbot run

Comment threadscripts/lib/preflight.sh
Comment threadscripts/lib/preflight.sh Outdated
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

LukasWodka added a commit that referenced this pull request Jul 27, 2026
…p hint on Linux (Bugbot #445)
1. _pf_mem_targets floored budget advice at 4 GB while the status line
says the client needs PF_MIN_MEM_GB (5) — on 5-6 GB machines the
operator was told 'below the 5 GB the client needs' and 'Give Docker
4 GB' in the same breath. The floor is now PF_MIN_MEM_GB.
2. The non-Darwin remediation said 'Docker Desktop -> Settings' — a
dead end on headless/engine-only Linux, where a low budget is the
machine's RAM or a VM/cgroup limit. Linux now gets that guidance.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

LukasWodka added a commit that referenced this pull request Jul 27, 2026
…gbot #444)
Get-PfMemTargets used Max(4, host-2): on <=6 GB hosts it paired 'below
the 5 GB the client needs' with 'give Docker 4 GB' (advice that can
never clear the warning), and on <=4 GB hosts the floor exceeded the
machine. The floor is now PF_MIN_MEM_GB (5, env-overridable) — the
exact twin of the bash fix on #445.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 581f804. Configure here.

…lines (#417, bash half)
Rebuilt on top of #513, which landed the CLAMP half of this work while this PR
sat open. #513 already gives every SHOWN figure `_pf_clamp_mem_gb` (physical −
PF_OS_RESERVE_GB, floored at PF_MIN_MEM_GB) and hard-fails a sub-floor Docker VM
in the post-Docker recheck. This PR's own `_pf_mem_targets` clamp helper is
therefore dropped as redundant — it reuses #513's helper instead.
What #513 did NOT fix, and this does: the flip-flop. `_pf_total_mem_kb` preferred
the runtime view over the host, so the SAME machine reported "16 GB (host)" on a
cold run and "6 GB (Docker VM)" on a warm one, purely on whether Docker happened
to be running. Two of its tests asserted that behaviour — one was literally named
"the Mac trap".
- the `_pf_total_mem_kb` memory selector is deleted. Memory has two distinct
truths and each caller now names the one it means: `_pf_host_mem_kb` for a
hardware fact, `_pf_runtime_mem_kb` for the budget the pods actually get.
(CPU keeps its fallback selector — there is no equivalent advice split.)
- `_pf_memory` gates on the MACHINE and prints `Memory: N GB (machine)`. The
Linux hard-fail gate, the 64 MiB grace and the MemAvailable check are all
unchanged. On a machine below the floor the macOS branch no longer offers a
Docker resize remedy — no Docker setting fixes too little physical RAM.
- Docker's budget becomes its OWN second line via `_pf_runtime_mem_status`, shown
only when a runtime is up AND its budget is meaningfully smaller than the
machine (the VM case). Native Linux, where the daemon sees all host RAM, no
longer repeats the same number twice.
- `_pf_hw_summary_line` reports host RAM — it had the same flip-flop in miniature
("7 GB memory" on a 15 GB WSL2 box).
- Linux budget hints drop the Docker Desktop dead end (Bugbot #445): a headless
box has no Desktop UI, so the remedy names the VM/cgroup limit instead.
- `PF_RUNTIME_MEM_WARNED` latches the budget warning so one run never warns twice
about the identical condition. It is tested INSIDE the warn branch, never at the
top of `_pf_recheck_runtime_mem`, so it can never gate #513's sub-floor
hard-fail; a test pins that (latch set + sub-floor VM still exits non-zero).
#513's reviewed recheck copy is left exactly as-is.
Tests: preflight.bats 89/89 (11 new — the (machine) label, the two-line output,
the Linux no-duplicate case, host-unreadable fallback, the clamped/floored advice,
both OS hint shapes, the latch, and the latch-can't-gate-the-hard-fail guard; the
two Mac-trap selector tests are replaced by a guard that the selector stays gone).
Full suite 659/660 — the one failure is `validate_config: valid config passes`,
pre-existing on clean develop and macOS-only (the /var symlink; fixed by #443).
bash -n + shellcheck --severity=error + check-style clean; manifest regenerated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka
LukasWodkaforce-pushed the fix/417-memory-truth-bash branch from 581f804 to c50df6bCompareJuly 31, 2026 11:08
Comment threadscripts/lib/preflight.sh Outdated
Comment threadscripts/lib/preflight.sh Outdated
…ad-end advice on a tiny host (Bugbot #445 r2)
Two findings from Bugbot on c50df6b, both reproduced and both real.
1) The recheck never actually used the "shared" copy, and graded differently.
`_pf_runtime_mem_status` was documented as the single copy for preflight AND
the post-Docker recheck, but the recheck still printed its own text — so the
COLD install path (Docker starts mid-run, the common case) got no colima
guidance on macOS and no hint at all on Linux. Worse, the two compared against
different thresholds: the helper against the clamped target, the recheck
against the raw PF_WARN_MEM_GB. Measured on an 8 GB host (clamped warn = 6)
with a 6 GB budget, one run printed both:
✔ Docker's memory budget: 6 GB
⚠ Docker is running with 6 GB — recommended ≥ 6 GB (6 GB to train)
Grading now lives only in the helper, and the recheck calls it. The helper
takes MiB so it uses the same PF_VM_MEM_GRACE_MIB tolerance as the recheck —
rounding to whole GB first misgraded a VM sized to exactly the documented
floor (4900 MiB guest) as sub-floor. A `quiet_ok` flag keeps the recheck
silent on a healthy budget, so no run prints the same ✔ twice.
2) A machine too small for the floor was still told to resize Docker.
On a 4 GB Mac the budget line advised "colima start --memory 5" — more than
the machine has, undercutting the honest "use a larger machine" stop the
recheck owns. The helper now detects host − PF_OS_RESERVE_GB < PF_MIN_MEM_GB
and points at the machine instead. This mirrors the same fix on the PowerShell
side (#444), so both installers now agree on the same hardware.
The sub-floor HARD-FAIL is untouched and still unconditional: the latch is tested
inside the warn branch only, and a test pins that a set latch plus a sub-floor VM
still exits non-zero.
Tests: preflight.bats 94/94 (5 new — host-too-small gets no resize, a host that
CAN reach the floor still does, the preflight-OK'd budget is never re-warned, the
recheck is silent when healthy, and the cold path carries the colima guidance).
Full suite 664/665 — the one failure is the pre-existing macOS-only
`validate_config` case fixed by #443. shellcheck --severity=error and --warning
both clean on preflight.sh; bash -n, check-style and check-drift clean; manifest
regenerated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka added a commit that referenced this pull request Jul 31, 2026
… too-small host says so (#417 residual)
This PR's ORIGINAL scope is superseded. #483 ("report host RAM consistently +
achievable memory advice (#417)") merged to develop on 2026-07-30 and closed#417, delivering the same Windows half by a different route: `Get-PfMemGb`
returns host RAM only, `Get-PfRuntimeMemGb` is shown as its own labelled line,
`Get-PfMemRecommendation` caps at host − 2 GB, and `Show-MemoryStatus` is the
single copy shared by Step 1 and the post-Docker recheck. Re-landing this
branch's `Get-PfHostMemGb` / `Get-PfMemTargets` / `Write-PfRuntimeMemStatus`
would only rename what already works.
What survives is the finding behind this branch's second commit, which is STILL
LIVE on develop: `Get-PfMemRecommendation` floors at 1 GB, so on a small host it
returns a number below the client's own minimum. Measured on develop:
Show-MemoryStatus -HostGb 6 -BudgetGb 3
⚠ Memory: 6 GB (Docker's current share: 3 GB) - below the 5 GB the client needs; it will OOM.
Give Docker at least 5 GB (up to 4 GB): ... [wsl2] memory=4GB ...
"at least 5 GB (up to 4 GB)" is an empty range, and the concrete value it tells
the operator to write is below the 5 GB the same sentence demands — the warning
cannot be cleared by following the advice. #483's Bugbot pass fixed this only for
the host-RAM-unreadable case; a KNOWN small host still hits it.
- `Get-PfMemRecommendation` now floors at PF_MIN_MEM_GB instead of 1, matching
bash's `_pf_clamp_mem_gb` exactly so both installers advise the same on the
same hardware.
- `Show-MemoryStatus` treats a host that cannot reach the floor even with the OS
reserve honoured (host − PF_OS_RESERVE_GB < PF_MIN_MEM_GB) as NOT a budget
bottleneck, so it gets the honest "use a larger machine" line rather than a
resize remedy it can never satisfy. This mirrors the bash recheck's
host-too-small branch, and the sibling fix in #445.
After:
Show-MemoryStatus -HostGb 6 -BudgetGb 3
⚠ Memory: 6 GB (Docker's current share: 3 GB) - below the 5 GB the client needs; it will OOM.
This machine has 6 GB of RAM total; the client needs at least 5 GB. Free up memory or use a larger machine.
Hosts that can reach the floor are unchanged (8 GB host still offers the resize;
15 GB → 13 and 16 GB → 14 recommendations are untouched).
Tests: 7 new/updated — the floor is the client minimum not 1, a 6 GB host never
yields a sub-floor number, PF_MIN_MEM_GB overrides the floor, an invariant sweep
over hosts 1..24 GB, the too-small host gets "larger machine" with no memory=1-4GB
value, and a host that CAN reach the floor still gets the resize hint. The former
"floors at 1 GB" assertion is replaced — it pinned the bug. Pester 374 passed /
0 failed / 9 skipped; PSScriptAnalyzer 0 errors; check-style, check-drift and
gen-manifest --check clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
aptracebloc
aptracebloc previously approved these changes Jul 31, 2026

@aptraceblocaptracebloc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the memory-truth redesign is sound (host RAM vs Docker's VM budget separated; one shared budget-status copy for preflight + recheck). All five Bugbot findings across r1/r2 are addressed in the current code (MiB+grace, Linux non-Desktop hint, recheck routed through the one copy, host-too-small branch) and Bugbot passes on head. CI fully green.

LukasWodka added a commit that referenced this pull request Jul 31, 2026
…n unachievable range (#444)
* fix(installer): memory advice never drops below the client minimum; a too-small host says so (#417 residual)
This PR's ORIGINAL scope is superseded. #483 ("report host RAM consistently +
achievable memory advice (#417)") merged to develop on 2026-07-30 and closed#417, delivering the same Windows half by a different route: `Get-PfMemGb`
returns host RAM only, `Get-PfRuntimeMemGb` is shown as its own labelled line,
`Get-PfMemRecommendation` caps at host − 2 GB, and `Show-MemoryStatus` is the
single copy shared by Step 1 and the post-Docker recheck. Re-landing this
branch's `Get-PfHostMemGb` / `Get-PfMemTargets` / `Write-PfRuntimeMemStatus`
would only rename what already works.
What survives is the finding behind this branch's second commit, which is STILL
LIVE on develop: `Get-PfMemRecommendation` floors at 1 GB, so on a small host it
returns a number below the client's own minimum. Measured on develop:
Show-MemoryStatus -HostGb 6 -BudgetGb 3
⚠ Memory: 6 GB (Docker's current share: 3 GB) - below the 5 GB the client needs; it will OOM.
Give Docker at least 5 GB (up to 4 GB): ... [wsl2] memory=4GB ...
"at least 5 GB (up to 4 GB)" is an empty range, and the concrete value it tells
the operator to write is below the 5 GB the same sentence demands — the warning
cannot be cleared by following the advice. #483's Bugbot pass fixed this only for
the host-RAM-unreadable case; a KNOWN small host still hits it.
- `Get-PfMemRecommendation` now floors at PF_MIN_MEM_GB instead of 1, matching
bash's `_pf_clamp_mem_gb` exactly so both installers advise the same on the
same hardware.
- `Show-MemoryStatus` treats a host that cannot reach the floor even with the OS
reserve honoured (host − PF_OS_RESERVE_GB < PF_MIN_MEM_GB) as NOT a budget
bottleneck, so it gets the honest "use a larger machine" line rather than a
resize remedy it can never satisfy. This mirrors the bash recheck's
host-too-small branch, and the sibling fix in #445.
After:
Show-MemoryStatus -HostGb 6 -BudgetGb 3
⚠ Memory: 6 GB (Docker's current share: 3 GB) - below the 5 GB the client needs; it will OOM.
This machine has 6 GB of RAM total; the client needs at least 5 GB. Free up memory or use a larger machine.
Hosts that can reach the floor are unchanged (8 GB host still offers the resize;
15 GB → 13 and 16 GB → 14 recommendations are untouched).
Tests: 7 new/updated — the floor is the client minimum not 1, a 6 GB host never
yields a sub-floor number, PF_MIN_MEM_GB overrides the floor, an invariant sweep
over hosts 1..24 GB, the too-small host gets "larger machine" with no memory=1-4GB
value, and a host that CAN reach the floor still gets the resize hint. The former
"floors at 1 GB" assertion is replaced — it pinned the bug. Pester 374 passed /
0 failed / 9 skipped; PSScriptAnalyzer 0 errors; check-style, check-drift and
gen-manifest --check clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(installer): name the OS reserve in the host-too-small hint (Bugbot #444)
The previous commit routed 5-6 GB hosts into `Show-MemoryStatus`'s generic
too-small hint, which compares total RAM to the floor and stops there:
host=6 GB, budget=3
⚠ Memory: 6 GB (Docker's current share: 3 GB) - below the 5 GB the client needs; it will OOM.
This machine has 6 GB of RAM total; the client needs at least 5 GB. Free up memory or use a larger machine.
6 >= 5, so as written the operator is told they have enough and still need a
bigger machine. The shortfall only adds up once the ~2 GB the OS needs is named —
which is exactly what bash's `_pf_recheck_runtime_mem` already says. That hint
predates this PR, but this PR is what made 5-6 GB hosts reach it, so it fixes it.
host=6 GB, budget=3 # after
⚠ Memory: 6 GB (Docker's current share: 3 GB) - below the 5 GB the client needs; it will OOM.
This machine has 6 GB of RAM total - too little for tracebloc: the client needs a 5 GB Docker budget and the OS needs ~2 GB, so about 7 GB physical is the practical minimum. Use a larger machine.
Hosts that can reach the floor are untouched (8 GB still gets the resize remedy;
16 GB still gets the training recommendation), and a host below the floor outright
keeps the plain copy.
Tests: 3 new — the reserve and the practical minimum are both named, and the
arithmetic is explained for every too-small host (4-6 GB). Pester 376 passed /
0 failed / 9 skipped; PSScriptAnalyzer 0 errors; parse, check-style, check-drift
and gen-manifest --check all clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(installer): the too-small-host guard also covers the training branch (Bugbot #444 r2)
`hostTooSmall` was consulted only inside the below-floor branch, so a 5-6 GB host
with Docker DOWN graded as "enough to run" and fell into the TRAINING branch,
which printed a concrete budget to write:
host=6 GB, budget=<none>
⚠ Memory: 6 GB - enough to run the client, but training (~8 GB/job) may OOM; 5 GB recommended to train locally.
For local training, give Docker up to 5 GB: WSL2 backend - [wsl2] memory=5GB ...
memory=5GB on a 6 GB machine leaves the OS 1 GB — a budget this same function
calls unachievable two branches up. The previous commit's floor change is what
lifted that number from 4 to 5 and made it reachable, so this closes the hole it
opened rather than trading one inconsistency for another.
Such a machine cannot be tuned into a training box at all, so the branch now says
that instead of printing a number:
host=6 GB, budget=<none> # after
⚠ Memory: 6 GB - enough to run the client, but too little to train locally (~8 GB/job).
This machine has 6 GB of RAM total and the OS needs ~2 GB, so it cannot give Docker a training-sized budget. Run the client here and train on a larger machine.
Machines that CAN reach the floor keep the actionable number (16 GB host with a
6 GB budget still gets "give Docker up to 14 GB" / memory=14GB), and the healthy
paths are untouched.
Tests: 4 new, including the invariant that closes this class for good — across
every branch and every budget shape (host 1-6 GB x budget none/1-6), no branch may
emit a concrete memory= value for a host that cannot reach the floor while keeping
the OS reserve. Pester 379 passed / 0 failed / 9 skipped; PSScriptAnalyzer
0 errors; parse, check-style, check-drift, gen-manifest --check clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

develop advanced between resolving the first merge and pushing it: #521 (macOS
installs pinned kubectl/k3d/helm via the shared verified path) and #522 (SIGPIPE
under pipefail made the chart guard skip a real chart change). Both touch
scripts/, so the install-k8s.ps1 hash line in scripts/manifest.sha256 collided
again — the fifth time today.
Only the manifest conflicted. Neither PR touches scripts/lib/preflight.sh
(they changed setup-macos.sh, gpu-nvidia.sh, install-client-helm.sh), so there is
no semantic overlap with this branch's memory work; re-verified that
_pf_total_mem_kb is still undefined and that no file carries conflict markers.
Gates re-run on the merged tree, not carried over from the previous merge:
bats scripts/tests/*.bats -> plan 692, ok 692, not ok 0 (complete TAP run; the
suite grew from 683 because #521 added setup-macos.bats);
shellcheck --severity=error over the CI file set -> rc=0; Pester -> 403 passed /
0 failed; check-style clean; check-drift no drift; gen-manifest.sh --check current.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Comment threadscripts/lib/preflight.sh Outdated
Comment threadscripts/lib/preflight.sh
Comment threadscripts/lib/preflight.sh
… predicate
Bugbot r3 on #445. All three were the SAME shape as the bug this PR exists
to remove — two copies of one judgement disagreeing — surviving on paths
the first pass missed.
1. Sub-floor remedy contradicted the hard-fail. On a warm run with a
sub-floor budget, _pf_runtime_mem_status hinted "Give Docker <rec>"
while _pf_recheck_runtime_mem hard-failed with "raise to <warn>" —
the latch suppresses a duplicate warning, deliberately never the
hard-fail, so both printed. The remedy now quotes the size that
failure quotes; the between-floor-and-warn branch, where no hard-fail
follows, still aims at the train figure.
2. Budget GB omitted the VM grace. rt_mib/1024 showed a VM configured at
exactly the documented floor as one GB BELOW it — graded correctly by
the grace-aware thresholds, displayed as a contradiction. Now
(mib + grace)/1024, matching the PowerShell peer.
3. "Enough to run" contradicted "use a larger machine". _pf_memory
compared host RAM straight against the Docker floor, ignoring the OS
reserve, so a 5-6 GB Mac was graded enough-to-run on one line and told
to use a larger machine two lines later. Both now read ONE predicate,
_pf_host_too_small_for_floor, which fails safe on unknown input. Native
Linux keeps its original wording: the daemon sees host RAM, so the
reserve arithmetic does not apply.
696 bats pass (4 new: sub-floor remedy agreement, floor-sized VM display,
the machine-line verdict, and the predicate incl. junk input).
shellcheck clean; manifest regenerated.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

All three valid, all fixed in 718af66 — and worth naming that all three were the same shape as the bug this PR exists to remove: two copies of one judgement disagreeing, surviving on paths the first pass didn't reach.

1. Sub-floor remedy contradicted the hard-fail

You pinpointed it exactly: the latch suppresses a duplicate warning and deliberately never the hard-fail, so on a warm sub-floor run both printed — Give Docker <rec> then raise to <warn>. One condition, two sizes.

The remedy now quotes the size the failure quotes. The between-floor-and-warn branch, where no hard-fail follows, still aims at the train figure — that divergence is intentional and harmless.

sub-floor (3000 MiB), 32 GB host:
WARN: budget: 3 GB — below the 5 GB the client needs
HINT: Give Docker 6 GB … --memory 6 <- was 8, now matches the hard-fail

2. Budget GB omitted the VM grace

Now (mib + grace) / 1024, matching the PowerShell peer:

VM configured at the 5 GB floor (guest 4900 MiB):
before: "budget: 4 GB" <- looked sub-floor while grading correctly as fine
after: "budget: 5 GB"

3. "Enough to run" contradicted "use a larger machine"

The sharpest of the three, because it crossed function boundaries. _pf_memory compared host RAM straight against the Docker floor with no OS reserve, so a 5–6 GB Mac was graded enough to run while the budget line two lines later correctly said use a larger machine.

Fixed with one predicate_pf_host_too_small_for_floor — read by both sites, so they cannot reach different verdicts again. It fails safe on unknown/junk input (never claims too-small from an unreadable host), and native Linux keeps its original wording since the daemon sees host RAM directly and the reserve arithmetic doesn't apply there.

Verification

696 bats pass, zero failing. Four new tests, because an unpinned fix is a future regression:

  • sub-floor remedy must quote the hard-fail's size (and must not quote the recommendation)
  • a floor-sized VM must not display as sub-floor
  • a 6 GB host must never print "enough to run", and must print "larger machine"
  • the predicate itself, including "", "unknown" and 0 failing safe

shellcheck --shell=bash clean; scripts/manifest.sha256 regenerated for the R8 gate.

Three rounds on this PR now, and each round found a genuine survivor of the same class — which is a fair signal that "two copies of one number" was the right thing to go after here.

bugbot run

Comment threadscripts/lib/preflight.sh
Comment threadscripts/lib/preflight.sh
…nforce
Two gaps in 718af66, which otherwise stands as-is — the shared
_pf_host_too_small_for_floor predicate is the right shape and is kept.
1. The display became grace-aware but the WARN threshold did not (only the floor
one was), so the same self-contradiction reopened one boundary up. Measured on
718af66 with a 32 GB host (warn_eff 8):
rt_mib=7680 -> ⚠ budget: 8 GB — recommended ≥ 8 GB
rt_mib=8000 -> ⚠ budget: 8 GB — recommended ≥ 8 GB
rt_mib=8191 -> ⚠ budget: 8 GB — recommended ≥ 8 GB
rt_mib=8192 -> ✔ budget: 8 GB
A ~512 MiB band telling the operator to raise a budget to the size it already
reports — and Docker Desktop's own defaults land in it. The warn threshold now
carries the same grace, so shown == target implies the ✔ branch at BOTH
boundaries rather than just the floor.
2. The four r3 tests were only partially enforcing. Under Bats 1.13 a failing bare
`[[ ]]` that is not the LAST command in a test body does not fail the test, so
`budget: 5 GB`, `!= enough to run`, `Give Docker <warn_eff>` and every line but
the last of the predicate test were advisory — they would have passed against
broken code. All 16 assertions in those tests now carry `|| return 1`.
Verified by mutation, not by inspection:
- reverting the warn-threshold grace (i.e. 718af66's shipped state) fails the new
boundary test — so this is a real gap, not a hypothetical one;
- reverting the display grace fails 2 tests;
- neutering _pf_host_too_small_for_floor fails 3, including the r3 predicate test
that only became capable of failing once hardened.
Baseline and restored are clean in every case.
Scope note: only the r3 tests are hardened here. 170 of ~698 tests in this suite
share the un-hardened pattern; that sweep needs its own PR because hardening will
surface previously-vacuous failures that each need triage (real installer bug vs
stale assertion), and burying that in this PR would hide it.
Gates: bats scripts/tests/*.bats -> plan 697, ok 697, not ok 0 (complete TAP run);
shellcheck --severity=error over the CI file set -> rc=0; check-style clean;
check-drift no drift; gen-manifest.sh --check current.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Pushed c0d7b8b on top of your 718af66 (fast-forward, nothing overwritten). I'd independently fixed the same three findings and dropped that commit — the shared _pf_host_too_small_for_floor predicate is the better shape and is kept as-is. Two gaps remained:

1. The display became grace-aware but the WARN threshold didn't. Only the floor threshold carried the grace, so the same self-contradiction reopened one boundary up. Measured on 718af66, 32 GB host (warn_eff 8):

rt_miboutput
7680⚠ budget: 8 GB — recommended ≥ 8 GB
8000⚠ budget: 8 GB — recommended ≥ 8 GB
8191⚠ budget: 8 GB — recommended ≥ 8 GB
8192✔ budget: 8 GB

A ~512 MiB band telling the operator to raise a budget to the size it already reports — and Docker Desktop's defaults land in it. The warn threshold now carries the same grace, so shown == target implies the ✔ branch at both boundaries.

2. The four r3 tests were only partially enforcing. Under Bats 1.13 a failing bare [[ ]] that isn't the last command in a test body does not fail the test. So budget: 5 GB, != enough to run, Give Docker ${warn_eff} and every line but the last of the predicate test were advisory — they'd have passed against broken code. All 16 assertions in those tests now carry || return 1.

Verified by mutation rather than inspection:

  • reverting the warn-threshold grace — i.e. this branch's state before c0d7b8bfails the new boundary test, so the gap was real, not hypothetical
  • reverting the display grace fails 2 tests
  • neutering _pf_host_too_small_for_floor fails 3, including the r3 predicate test that only became capable of failing once hardened

Baseline and restored clean in every case.

Scope note: only the r3 tests are hardened here. 170 of ~698 tests in this suite share the un-hardened pattern — that needs its own PR, because hardening will surface previously-vacuous failures that each need triage (real installer bug vs stale assertion), and burying that here would hide it. Tracked separately.

Gates: bats scripts/tests/*.bats → plan 697, ok 697, not ok 0 (complete TAP run, plan line checked); shellcheck --severity=error over the CI file set → rc=0; check-style clean; check-drift no drift; gen-manifest.sh --check current.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

Comment threadscripts/lib/preflight.sh
Merge: the only conflict was scripts/manifest.sha256, a generated file —
resolved by regenerating it over the merged sources rather than picking a
side. develop also already made the warn band grace-aware, which settles
half of the r4 grading finding.
- $gb was passed to _pf_host_too_small_for_floor, but _pf_memory falls
back to the VM budget (label="Docker VM") when host RAM is unreadable,
so a large machine with a small VM could be called too small and lose
its resize remedy. The branch now requires label==machine.
- The recheck's hard-fail still printed raw floored GB while the status
line printed the grace-adjusted figure, so one budget could appear as
two numbers across the two messages. Both now print the same value.
Verified behaviourally, not just asserted: a VM configured at exactly the
warn target now reports "budget: 8 GB" instead of the self-contradicting
"budget: 8 GB — recommended >= 8 GB".
711 bats pass (2 new: the unreadable-host fallback, and the at-warn-target
display). shellcheck clean; manifest regenerated.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Both r4 findings fixed in 0d98736 — and both were caused by my own r3 changes, which is worth stating plainly.

VM size misread as host RAM — correct. _pf_memory falls back to the VM budget when host RAM is unreadable (label="Docker VM"), and I passed that $gb straight into _pf_host_too_small_for_floor. So a large machine with a small VM could be declared too small and lose its resize remedy — worse than the inconsistency I was fixing. The branch now requires label == "machine", so only real host RAM is ever graded as host RAM.

Hard-fail omits display grace — also correct, and it's the same half-applied-fix shape: r3 made the status line grace-adjusted but left the recheck's hard-fail printing raw floored GB, so one budget could still surface as two numbers across the two messages. Both now print the same figure.

One thing the merge settled rather than me: develop had already made the warn band grace-aware (- PF_VM_MEM_GRACE_MIB), so after merging, grading and display are already equivalent — rt_mib < warn*1024 - grace is exactly rt_mib + grace < warn*1024. I verified that behaviourally instead of assuming it:

VM configured at exactly the warn target (8 GB, guest 7900 MiB):
before: "budget: 8 GB — recommended ≥ 8 GB" <- self-contradicting
after: "OK: Docker's memory budget: 8 GB"

Merge: the only conflict was scripts/manifest.sha256, a generated file — resolved by regenerating it over the merged sources rather than picking a side.

Verification:711 bats pass, zero failing. Two new pins (the unreadable-host fallback, and the at-warn-target display), on top of r3's four. shellcheck --shell=bash clean; manifest regenerated for the R8 gate.

Four rounds, nine findings, and every round has surfaced a genuine survivor of the same class — two copies of one number disagreeing. Three of the last four were regressions I introduced while fixing the previous round, which is a fair argument that this file wanted the shared-predicate treatment rather than another round of local patches.

bugbot run

Comment threadscripts/lib/preflight.sh Outdated
…display
Bugbot r5 (High), and correcting my own assumption: the grace on
_pf_memory's shown GB came in from develop via the merge, not from my r4
edit. Either way the halves disagreed — the display added
PF_VM_MEM_GRACE_MIB while the floor gate kept a 64 MiB tolerance, so a
5 GB VM reporting ~4900 MiB printed "Memory: 5 GB — below the 5 GB the
client needs" and, on Linux, hard-failed on it.
- floor_mib and warn_mib now use PF_VM_MEM_GRACE_MIB, the same tolerance
the display uses and the same one _pf_runtime_mem_status already used
for its floor and warn tests. All three now agree on the boundaries.
- _pf_host_too_small_for_floor is now fed $(_pf_host_mem_gb) instead of
$gb. A shared predicate only prevents divergence if both call sites
pass the same input; _pf_memory was passing a grace-adjusted
VM-or-host figure while the status path passed raw host GB.
MemAvailable is deliberately left ungraced: it is a live measurement, not
a configured size, so adjusting it would mask a real shortage.
713 bats pass (2 new: the floor-sized-VM message, and a guard that both
call sites feed the predicate the same figure). shellcheck clean; manifest
regenerated.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Valid, and High is the right severity — on Linux this path hard-fails. Fixed in 8886f8c.

One correction to my own assumption: I initially doubted this, because I didn't add grace to _pf_memory's display. Checking the file, it's there — it came in from develop via the merge, not from my r4 edit. Either way the two halves disagreed, and the finding stands:

5 GB VM reporting ~4900 MiB MemTotal:
display: (4900 + 512) / 1024 = 5 GB
gate: 4900 < 5*1024 - 64 = 5056 = sub-floor
result: "Memory: 5 GB — below the 5 GB the client needs" (and a hard-fail on Linux)

Fixes:

  1. floor_mib and warn_mib now use PF_VM_MEM_GRACE_MIB — the same tolerance the display uses, and the same one _pf_runtime_mem_status already used for its own floor and warn tests. All three now agree on where the boundaries are.
  2. _pf_host_too_small_for_floor is fed $(_pf_host_mem_gb) instead of $gb. This was the sharper half of your finding: a shared predicate only prevents divergence if both call sites pass it the same input, and _pf_memory was passing a grace-adjusted VM-or-host figure while the status path passed raw host GB. The predicate was shared; the inputs weren't.

Verified on the exact case:

before: "Memory: 5 GB (machine) — below the 5 GB the client needs; it will OOM."
after: "Memory: 5 GB (machine) — enough to run, but training (≈8 GB/job) may OOM…"

One thing deliberately left raw:MemAvailable. It's a live measurement, not a configured size, so grace-adjusting it would mask a genuine shortage rather than recover a configured figure.

713 bats pass, zero failing. Two new pins — the floor-sized-VM message, and a source-level guard that both call sites feed the predicate the same figure, so this specific divergence can't return. shellcheck clean; manifest regenerated.

Five rounds, ten findings, every round a real one. The recurring lesson is narrower than "copies drift": I twice fixed a display without the matching threshold, and once shared a predicate without sharing its inputs. Both are the same mistake — changing one side of a comparison.

bugbot run

Comment threadscripts/lib/preflight.sh
Bugbot r6 found a FOURTH site: _pf_hw_summary_line computed its own
memory GB, so the collapsed summary could print a different size from the
memory line in the same preflight.
Investigating it turned up something worse, and it corrects the record on
r4/r5: my r4 fix for _pf_recheck_runtime_mem anchored on a two-line
pattern that also existed in _pf_memory, and the replace took the FIRST
match — so the fix landed in _pf_memory and the recheck never got it. The
grace on _pf_memory's display, which I attributed on the PR to develop
via the merge, was actually that misapplied edit. The r5 High finding was
a direct consequence.
Structural fix rather than a fifth patch: _pf_display_gb_from_mib is now
the single definition, used by _pf_memory, _pf_runtime_mem_status,
_pf_recheck_runtime_mem and _pf_hw_summary_line. rt_gb goes through it too
— it feeds the 'is the VM meaningfully smaller than the machine'
comparison, and grading one grace-adjusted side against a raw other side
is precisely the mistake these six rounds keep rediscovering.
Deliberately still raw: MemAvailable (a live measurement — inflating it
would hide a real shortage), disk, and _pf_host_mem_gb (physical RAM needs
no compensation, and it is the input _pf_host_too_small_for_floor grades).
Every replacement in this commit asserted its anchor matched EXACTLY once
and refused otherwise — the guard that would have caught r4's error.
715 bats pass (2 new: a source-level invariant that no site renders its own
memory GB, and summary-vs-memory-line agreement). shellcheck clean;
manifest regenerated.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Fixed in 1d43a50 — and this round turned up an error of mine that I need to correct on the record.

The r6 finding: valid, a FOURTH render site

_pf_hw_summary_line computed its own memory GB, so the collapsed summary could print a different size from the memory line in the same preflight, breaking its "never disagree" invariant.

The correction: my r4 fix landed in the wrong function, and I mis-attributed it

Investigating r6 turned up why r5 happened at all. My r4 fix for _pf_recheck_runtime_mem anchored on this two-line pattern:

gb=0
mib=0

That pattern also existed in _pf_memory, and my replace took the first match. So the fix landed in _pf_memory and the recheck never received it. The grace on _pf_memory's display — which I told you on this PR came from develop via the merge — was actually my own misapplied edit. That claim was wrong, and the r5 High finding ("grace inflates host floor display") was a direct consequence of it. Apologies for the misdiagnosis; the r5 fix itself was still correct, it just wasn't cleaning up develop's work, it was cleaning up mine.

Structural fix, not a fifth patch

_pf_display_gb_from_mib is now the single definition, used by all four sites. rt_gb goes through it too — it feeds the "is the VM meaningfully smaller than the machine" comparison, and grading one grace-adjusted side against a raw other side is exactly the mistake these six rounds keep rediscovering.

Deliberately still raw, each for a reason: MemAvailable (a live measurement — inflating it would hide a genuine shortage), disk, and _pf_host_mem_gb (physical RAM needs no compensation, and it is the input _pf_host_too_small_for_floor grades).

Process change: every replacement in this commit asserted its anchor matched exactly once and refused otherwise. That guard fired twice while writing this fix — on _pf_runtime_mem_status, where it revealed my r4 edit had never landed either — and it is precisely what would have caught r4's error.

Verification

715 bats pass, zero failing. Two new tests, both structural rather than symptomatic: a source-level invariant that no site renders its own memory GB, and summary-line-vs-memory-line agreement on a just-under-a-round-GB host. shellcheck clean; manifest regenerated.

Six rounds, eleven findings. The through-line is one mistake in three costumes: changing one side of a comparison — a display without its threshold, a predicate without its inputs, a converter without its callers. The fix now makes that structurally hard rather than relying on me noticing.

bugbot run

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1d43a50. Configure here.

Comment threadscripts/lib/preflight.sh
Bugbot r7 (High). Two divergences in one branch: the reserve arithmetic
was inlined instead of calling _pf_host_too_small_for_floor, and the branch
was gated OS != Linux while _pf_runtime_mem_status applies it everywhere.
On a warm Linux install with a sub-floor cgroup/VM budget, preflight said
'use a larger machine' and this hard-fail then advised raising Docker to a
size that machine cannot give.
Now calls the shared predicate on every OS, with an OS-appropriate noun so
the Mac wording is preserved. Verified both messages agree on a 6 GB Linux
host with a 3 GB budget: both say 'use a larger machine'.
Same class as r1-r6 on a new axis: not two values disagreeing but two
OS GATES disagreeing about when one judgement applies.
717 bats pass (2 new: the Linux path, and an invariant that the reserve
arithmetic exists in exactly one place). shellcheck clean; manifest
regenerated.
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Valid, High deserved — fixed in d5b4172. Two divergences in one branch:

  1. The reserve arithmetic was inlined ($(( phys_gb - PF_OS_RESERVE_GB )) -lt PF_MIN_MEM_GB) instead of calling _pf_host_too_small_for_floor — so the predicate I introduced in r3 to stop exactly this had a fourth call site that wasn't using it.
  2. That branch was gated OS != Linux while _pf_runtime_mem_status applies the predicate on every OS.

Consequence on a warm Linux install with a sub-floor cgroup/VM budget, which is the common shape on a shared box:

preflight: "This machine has 6 GB … Free up memory or use a larger machine."
hard-fail: "Free memory (or raise the VM) to >= 6 GB" <- impossible on that machine

Now the shared predicate runs on every OS, with an OS-appropriate noun so the Mac wording survives. Verified on the cited case (6 GB Linux host, 3 GB budget) — both messages now say use a larger machine, and the impossible-raise remedy is gone.

This is the same class as r1–r6 on a new axis. Not two values disagreeing, but two OS gates disagreeing about when one judgement applies. Worth naming because my earlier framing — "changing one side of a comparison" — wouldn't have caught it; the values agreed here, the conditions didn't.

717 bats pass. Two new tests, one behavioural (the Linux path) and one structural: an invariant that the reserve arithmetic exists in exactly one place, so a fifth inlined copy fails CI rather than waiting for a review round. shellcheck clean; manifest regenerated.

bugbot run

@LukasWodka
LukasWodka requested review from shujaatTracebloc and removed request for saadqbalAugust 2, 2026 15:13
@LukasWodka
LukasWodka merged commit 747cab7 into developAug 3, 2026
36 of 37 checks passed
@cursorcursorBot mentioned this pull request Aug 3, 2026
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

/fr-pass

@LukasWodka
LukasWodka deleted the fix/417-memory-truth-bash branch August 14, 2026 13:53
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@LukasWodka@saadqbal@aptracebloc