fix/2568 oom guard fail closed - #2576
Closed
noahgift wants to merge 2 commits into
Closed
Conversation
noahgift
commented
Aug 22, 2026
noahgift
commented
Contributor
- fix(serve): the F32 dequant OOM guard failed OPEN on macOS (OOM guard fails OPEN on macOS: /proc/meminfo + unwrap_or(u64::MAX) makes the threshold 12.8 exabytes #2568)
- fix(serve): the macOS OOM-guard fix broke Windows — add the third probe
`system_memory_bytes()` read /proc/meminfo and nothing else. macOS has no
/proc, so it returned None there, and `validate_f32_dequant_limits` turned
that None into `u64::MAX` — making the "80% of RAM" threshold ~12.8 EiB and
`estimated_peak > threshold` unsatisfiable. The guard that exists to refuse
a dequant larger than the machine could never fire on any Mac.
Reproduced on the real host (macOS 26.5.2, arm64, 16 GiB) by building the
shipped 0.63.0 logic verbatim with rustc there:
system_memory_bytes() -> None
guard verdict for 340 GB peak: Ok(()) <- admitted, on a 16 GiB box
The same extract on x86_64 Linux refuses it, which is the OS-not-ISA proof:
the aarch64 Linux box has /proc and the guard works. The shipped binary
corroborates it — `strings ~/.cargo/bin/apr` on that Mac has 2 hits for
/proc/meminfo and 0 for hw.memsize.
Also found while reproducing: in a DEBUG build the `u64::MAX * 80` in the
threshold overflows and panics (rc=101) rather than failing open, so on
macOS debug builds this aborted the load instead of skipping the check.
Three changes, in ascending order of importance:
1. Probe: add a `sysctl -n hw.memsize` fallback for macOS. Absolute paths,
never PATH-resolved — a shadowed `sysctl` would feed an arbitrary number
into a safety threshold. Every other OS is documented as "no probe".
2. FAIL CLOSED. The decision moves into `dequant_verdict(file, dequant,
Option<u64>)`, which refuses when memory is unknown and says which
measurement is missing. An unknown limit is not an infinite one. The
threshold is now `mem/5*4`, which cannot overflow for any u64.
3. Delete the `if cfg!(target_os = "linux")` skip from the probe test. The
skip excluded exactly the platform where the code was broken, so it read
as coverage while proving nothing. The assertion now runs everywhere and
names the platform when it fails.
Falsifiers (all run on every target — the decision is separated from the
probe so no target_os can skip them):
test_dequant_verdict_fails_closed_when_memory_unknown
test_dequant_verdict_fails_closed_even_for_a_tiny_model
test_dequant_verdict_{refuses_over,allows_under}_80_percent
test_dequant_verdict_threshold_does_not_overflow
test_system_memory_bytes_is_measurable_on_this_platform
test_parse_meminfo_total_bytes / test_parse_sysctl_memsize
test_run_sysctl_memsize_against_a_stub — drives the macOS probe's
exec+exit-status+parse path on Linux via a stub, so the only part not
exercised off-Mac is the kernel key itself
Mutation-verified in both directions:
* restoring `unwrap_or(u64::MAX)` turns the two fail-closed tests RED and
leaves the other three GREEN (engagement is targeted, not blanket).
* breaking the probe to return None (simulating "no /proc") turns the new
unconditional test RED while the old skipping test, with its cfg! set to
what it evaluates to on a Mac, reports **ok** in the same run — the
skip's blindness, demonstrated side by side.
* the fixed logic on the real Mac: Some(17179869184), 340 GB refused,
2 GB still allowed, unknown still refused — release AND debug.
Behaviour change: on a host where RAM cannot be measured (no /proc, no
sysctl — e.g. a container with /proc unmounted), APR model loads now fail
with a resource_limits error instead of silently skipping the check.
Contract: apr-qa-chaos-v1 gains `oom_guard_fails_closed` plus F-CHAOS-006
and F-CHAOS-007. It already cited GH-478, the gate this repairs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VX2s9nQPbWDB3Vze4JM3Wg
Found by adversarial review of the #2568 fix, before merge. WHAT WENT WRONG. Making an unknown memory total REFUSE a dequant is the right principle -- an unknown limit is not an unlimited one. But `system_memory_bytes()` knew only `/proc/meminfo` and macOS `sysctl`, so on Windows it returned `None` and the guard refused EVERY dequant, hard-breaking the APR v2 load path on a target this repo builds and packages (.github/workflows/nightly.yml, x86_64-pc-windows-msvc). Fixing one platform in isolation broke another. That is the multi-platform dogfood gate (#2573) earning its keep against the very change that accompanied it -- and no CI job would have caught it, because none of them run this code on Windows. THE FIX. A third probe via `sysinfo` (already a workspace dependency at 0.32), declared under `[target.'cfg(windows)'.dependencies]` so Linux and macOS builds carry nothing new -- their native probes are cheaper than a crate. THE UNIT TRAP, ASSERTED RATHER THAN TRUSTED. `sysinfo::System::total_memory` returns BYTES. The Win32 primitive `GetPhysicallyInstalledSystemMemory` returns KILOBYTES. Reading one as the other lands ~1024x off and silently disarms the threshold -- the same class of dead guard #2568 is about, from the other direction. `windows_probe_reports_bytes_not_kilobytes` pins it. THE FALSIFIER THE REVIEW ASKED FOR. The old test asserted only that a failed probe refuses -- which PASSED on the build that broke Windows, because it proved the fail-closed branch and said nothing about whether a probe branch existed. `memory_probe_exists_on_every_shipped_platform` asserts a probe EXISTS on whatever platform it runs on, plus a 256 MiB..8 TiB plausibility band whose job is catching a unit error. Between the Linux runners, the macOS box and the Windows nightly it is asserted on all three. VERIFIED: cargo check -p aprender-serve --target x86_64-pc-windows-msvc rc=0 (rustup target added for this; it fails "can't find crate for core" without it, which is why the defect shipped unnoticed -- nobody could compile the Windows path locally) cargo test -p aprender-serve --lib contract_gate rc=0, 40 passed cargo fmt --all -- --check rc=0 MUTATION, and I am reporting the honest result rather than the flattering one: force ALL probe arms to None -> rc=101, "no memory probe on this platform (linux). Fail-closed is only safe where a probe EXISTS" -- the falsifier discriminates. remove ONLY the windows arm -> rc=0 on Linux, correctly: the /proc arm still answers here. That mutation proves the LINUX arm, not the Windows one. STATED LIMIT: the Windows probe is verified to COMPILE, not to RUN. No Windows machine is in this fleet, so `windows_total_bytes()` returning a plausible value is asserted by a test that has not yet executed on Windows. The nightly Windows job is where that assertion first fires. Calling this "verified on Windows" would be exactly the over-claim this repo keeps finding. Refs #2568, #2566, #2573 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
noahgift
enabled auto-merge
August 22, 2026 13:17
Contributor
Author
|
Superseded by #2613, the 0.64.0 integration batch. This PR's commits are merged into Why batched rather than landed individually: one Batching also found four defects that were invisible to every individual PR — most Closing now, deliberately: an open PR that merges first moves #2613's base and forces |
auto-merge was automatically disabled
August 22, 2026 18:09
Pull request was closed
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
1 participant
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.