Skip to content

fix(engine): correct the macOS struct statvfs FFI layout - #1586

Merged
justinchuby merged 3 commits into
mainfrom
justinchuby-fix-macos-statvfs-layout
Aug 20, 2026
Merged

justinchuby merged 3 commits into
mainfrom
justinchuby-fix-macos-statvfs-layout

Conversation

@justinchuby

@justinchuby justinchuby commented Aug 20, 2026 •

Copy link
Copy Markdown
Owner

Root cause

platform_capacity.rs hand-declared a #[repr(C)] struct StatVfs to call statvfs(3). That declaration matches 64-bit glibc only. On macOS fsblkcnt_t/fsfilcnt_t are 4 bytes, not 8, and there is no trailing f_spare member — so a 112-byte struct was laid over a real 64-byte one.

f_blocks and f_bfree were read as one fused u64, the garbage overflowed checked_mul, that returned None, and the disk reported as unmeasurable.

Measured, not assumed

Compiled and ran a C program against <sys/statvfs.h> on arm64 macOS:

sizeof(struct statvfs) = 64        alignof = 8
sizeof(fsblkcnt_t) = 4             sizeof(fsfilcnt_t) = 4      sizeof(unsigned long) = 8
offsetof: f_bsize 0, f_frsize 8, f_blocks 16, f_bfree 20, f_bavail 24,
          f_files 28, f_ffree 32, f_favail 36, f_fsid 40, f_flag 48, f_namemax 56
live:     f_frsize=4096 f_bsize=1048576 f_blocks=242837545 f_bavail=19132442

The damage was wider than the fused block counts: f_fsid/f_flag/f_namemax were also at wrong offsets (the old declaration implied f_fsid at 64; the real offset is 40), and f_spare does not exist on macOS at all. Only f_bsize/f_frsize were correct. A fix driven purely by the reported symptom would have left two of these in place.

Fix: use libc's definition, don't hand-roll one

The struct is now libc::statvfs. libc was already in this crate's dependency graph via memmap2 and tokio — both direct dependencies — so declaring it explicitly adds no new package. The Cargo.lock delta is literally one line:

 crates/onnx-genai-engine dependencies:
+ "libc",

libc defines the layout per target, so there is no longer a layout in this crate to get wrong — on macOS, on Linux, or on the other unixes an earlier revision of this PR left with the broken widths.

This is a net deletion: two type aliases, a cfg'd-out f_spare, two compile-time size guards and a field-offset table all go away.

Verification

Before (reverse-verified against the unmodified tree)

test platform_capacity::tests::disk_capacity_is_measured_for_the_working_directory ... FAILED
panicked at crates/onnx-genai-engine/src/platform_capacity.rs:293:55: disk must be measurable here

test engine::tests::an_explicit_byte_limit_is_honored_without_a_device_query ... FAILED
panicked at crates/onnx-genai-engine/src/engine/mod.rs:1388:87:
called `Result::unwrap()` on an `Err` value: UnmeasuredCapacity { tier: "disk spill" }

After

Both pass. Full crate suite: 430 passed; 0 failed; 1 ignored.

Executed

  • The C layout probe, on this macOS host.
  • Both named tests failing before / passing after; full engine lib suite.
  • The exact CI command that went red — cargo clippy --locked --all-targets -p onnx-genai-engine --features native-backend -- -D warnings — now clean for this crate.
  • Linux was compiled, not just reasoned about. platform_capacity.rs has no crate-internal dependencies, so it was built standalone for x86_64-unknown-linux-gnu against libc's real Linux struct statvfs, under -D warnings. Clean. A full-workspace Linux cross-build is not possible here (onnx-genai-ort-sys needs a Linux C sysroot for bindgen), so this is the module, not the whole crate.
  • cargo fmt --check clean.

Not executed

Linux test execution, and Windows anything. Those need CI.

On the CI failures for this PR

main is currently red on its own, so "CI is red" is not by itself attributable here. Run 32388200098 is main at 3b3e5f79 — this PR's exact merge-base — and it already fails the same job and same step for:

Those coverage/offline lanes do not include onnx-genai-engine at all — python .github/scripts/workspace_test_packages.py cargo-args offline-cross-platform does not list it — so this PR cannot be their cause. Pre-existing, not addressed here.

Rust quality was genuinely mine. It passed on main at 3b3e5f79 and failed here. Cause: an earlier revision used u64::from(buf.f_blocks), which on Linux is a u64→u64 identity and trips clippy::useless_conversion under -D warnings — precisely the Linux-only blind spot flagged as unverified. Reproduced locally by forcing the Linux widths, and fixed: the libc version uses plain as u64 casts under one function-level allow, matching what the surrounding code already does for f_frsize/f_bsize.

`statvfs_capacity` hand-declared a `#[repr(C)] struct StatVfs` that matches
64-bit glibc only. On macOS `fsblkcnt_t`/`fsfilcnt_t` are 4 bytes, not 8, and
there is no trailing `f_spare` member, so the declaration was a 112-byte struct
laid over a real 64-byte one: `f_blocks` and `f_bfree` were read as one fused
u64, the product overflowed `checked_mul`, and the disk reported as
unmeasurable.

Measured on arm64 macOS by compiling against <sys/statvfs.h>:
sizeof = 64, fsblkcnt_t = 4, and f_fsid at offset 40 rather than the 64 the old
declaration implied — so three fields past the block counts were also wrong,
not just the six count fields.

Gate the widths per platform. The non-macOS branch expands to exactly the types
present before, so Linux codegen is unchanged. Arithmetic moves to `u64::from`,
which widens correctly at either width.

Adds compile-time `size_of` guards (macOS 64, 64-bit Linux 112) so a wrong
layout is a build failure rather than silent runtime garbage, plus field-offset
tests. Verified the guard fires by reverting the width locally.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…s FFI

The module header justified the hand-written `unsafe extern "C"` declarations
with "the workspace carries no `sysinfo`/`libc` today". The `libc` half of that
is no longer true — it is a dependency of onnx-runtime-ep-cpu,
onnx-runtime-virtual-memory, onnx-runtime-tracer and onnx-genai-cli. Left as
written, the next reader either inherits a false premise or concludes the whole
hand-rolled approach is unmotivated.

State the actual reason: `libc` exists in the workspace but deliberately is not
a dependency of this crate, because taking one on for a single FFI struct is
not worth it. Also name the cost that choice carries — hand-declared #[repr(C)]
types have to be checked against each platform ABI rather than assumed — and
point at the compile-time guards that now discharge it.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 20, 2026
main advanced past this branch's base (#1578, #1580, #1583..#1585, #1588,
#1491, #1571, #1592). 23 files changed on main, 3 of them also touched here.

Conflict resolution, and the audit behind it:

  kernels/matmul_nbits.rs -- taken from main wholesale. This branch's blob
  is byte-identical to main's blob at 3542ae7 (#1580): the only "change"
  on our side was restoring main's probe after an earlier merge dropped it
  (the Tier 2 audit). main has since carried that same file forward through
  #1584/#1585, so our content is a strict ancestor of main's and taking
  main loses nothing. Verified by blob hash, not by reading the diff.

  .github/workflows/ci.yml -- auto-merged. Result equals main plus the four
  `-p onnx-runtime-memory-api` package selections this stack adds. main's
  two new `cargo fetch --locked` steps are both present (counted).

  pipeline/decoder_component.rs -- auto-merged. Result equals main plus the
  two `ProcessMemoryManager` lines this stack adds. main's #1592 additions
  (supports_argmax, step_argmax, decode_argmax_with_step_inputs,
  captured_step_input_greedy_supported) all survive at main's reference
  counts.

The other 20 files main changed are byte-identical to origin/main in the
merge result, checked by blob hash for every one rather than by eye.

Verified: cargo check --workspace --all-targets and
cargo check -p onnx-genai-engine --features cuda,native-backend clean;
cargo fmt --all --check clean; onnx-genai-engine native-backend suite
559 passed / 2 failed / 1 ignored, the two failures being the same macOS
statvfs pair that fails on main and is fixed by #1586.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
@codecov

codecov Bot commented Aug 20, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.78%. Comparing base (3b3e5f7) to head (f1b307d).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main    #1586    +/-   ##
========================================
  Coverage   80.77%   80.78%            
========================================
  Files         381      381            
  Lines      173645   173786   +141     
  Branches   173645   173786   +141     
========================================
+ Hits       140264   140386   +122     
- Misses      28491    28505    +14     
- Partials     4890     4895     +5     
Flag Coverage Δ
cli-ort-linux 82.60% <ø> (ø)
cli-ort-windows 82.10% <ø> (-0.10%) ⬇️
mlas 85.05% <ø> (-0.04%) ⬇️
offline 80.64% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

The previous fix kept the hand-written `#[repr(C)] struct StatVfs` and gated
the field widths per platform. That works, but it makes this crate responsible
for every platform's ABI: it needed two type aliases, a cfg'd-out `f_spare`,
two compile-time size guards and a table of field offsets, and it still only
covered macOS and Linux -- every other unix kept the widths that were wrong on
macOS.

`libc` already defines `struct statvfs` per target, and is already in this
crate's dependency graph via memmap2 and tokio, so declaring it adds no new
package (the Cargo.lock delta is one line). Use it and delete the hand-rolled
layout: there is no longer a layout here to get wrong, on any unix.

This also drops the `useless_conversion` error that broke the Rust quality lane
on Linux, where `u64::from` on an already-64-bit count is an identity. The
casts are now plain `as u64` under one function-level allow, which is what the
surrounding code already does for `f_frsize`/`f_bsize`.

Verified: engine lib suite 430 passed; the CI clippy command
(--locked --all-targets --features native-backend -- -D warnings) clean; and
the module compiled standalone for x86_64-unknown-linux-gnu against libc's
Linux definition, so the Linux path is exercised rather than only reasoned
about.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby merged commit 79e98c4 into main Aug 20, 2026
8 of 17 checks passed
@justinchuby
justinchuby deleted the justinchuby-fix-macos-statvfs-layout branch August 20, 2026 18:38
justinchuby added a commit that referenced this pull request Aug 20, 2026
Picks up #1586, #1601 and #1602. No conflicts; Cargo.lock was the only
file both sides touched and it auto-merged.

Audited the same way as the previous two syncs rather than by reading
the diff: every main-only file verified byte-identical to origin/main by
blob hash, and for Cargo.lock every line main added over the merge-base
confirmed present in the result, with this stack's three
onnx-runtime-memory-api entries still intact.

#1586 is the reason this sync matters beyond staying mergeable. It
corrects the macOS `struct statvfs` FFI layout, which is what made
`platform_capacity::tests::disk_capacity_is_measured_for_the_working_directory`
and `engine::tests::an_explicit_byte_limit_is_honored_without_a_device_query`
fail on every local run of this branch. Those two were the only failures
in the crate suite here, and every "559 passed / 2 failed" figure
reported against this PR was that pair. They are expected to be gone
now; verified in the following run rather than assumed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
justinchuby added a commit that referenced this pull request Aug 22, 2026
…ees were destroyed (#1717)

Two decision records existed **only on this machine** — written into
`.squad/decisions/inbox/` but never committed. Recovered here.

The inbox README already names this exact failure mode as the reason the
directory was made tracked on 2026-07-29:

> **Drops survive worktree deletion.** Previously the inbox was
gitignored, so drops existed only on the machine that wrote them.
Removing a worktree before Scribe ran destroyed unmerged drops — this
cost real records more than once.

The policy fixed the *gitignore* half of the problem. It cannot fix the
half where a drop is written and simply never `git add`ed, which is what
happened to both of these. I found them while cleaning up 11 stale
worktrees; one was inside a worktree I was about to delete.

## What is being recovered

**1. `Copilot-phase-5-process-memory-manager-ownership-boundarie.md`**

Written in the `justinchuby-phase-5-process-memory-manager` worktree (PR
#1426, now closed as superseded by #1579). Copied out immediately before
that worktree was removed.

Verified it is not a duplicate before adding it:

```
$ git grep 'Phase 5 process memory manager ownership boundaries' origin/main -- .squad
(no match)
```

So the Scribe never merged it — it was not sitting redundantly next to
an existing entry in `.squad/decisions.md`.

Content-wise this one is not merely historical. It records the Phase 5
ownership split: `ProcessMemoryManager` owns process quota, canonical
authority/view mapping, holder identity, context transaction gates, loss
broadcast and settlement coordination, while **governors keep budget
policy, holders keep victim choice, and EP contexts keep
stream/copy/commit/decommit/fence/deferred-release ordering**. It also
records two invariants that are easy to get wrong later — quarantine
never infers a refund from allocation length, and confirmed context
termination is the sole device-loss discharge boundary.

That boundary is still load-bearing after #1579, and **#1186 is still
open precisely on the provider/context-pinning half of it**, so losing
this note would have removed context from work that has not happened
yet.

**2.
`coordinator-criterion-failure-is-structural-not-probabilistic.md`**

Written in this worktree. References #1579, #1586, #1533, #1600 and
three sessions that converged on it independently.

## Scope

No code changes; two files under `.squad/decisions/inbox/`. Per the
inbox contract these are transient — the Scribe merges them into
`.squad/decisions.md` and deletes the drops.

## Worth considering separately

Nothing detects an uncommitted drop. Both of these survived by
coincidence, because a human happened to be cleaning worktrees and
looked at `git status` before running `git worktree remove --force`. A
pre-removal check for untracked files under `.squad/decisions/inbox/`
would close that gap; I have not written one here.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c80f8522-983c-47f7-8241-2155a823aabe
Sign up for free to 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.

1 participant