Skip to content

style: silence the dormant graph-retention seam #1648 left red on main - #1649

Merged
justinchuby merged 1 commit into
mainfrom
squad/roy-fmt-repair-main-1648
Aug 21, 2026
Merged

justinchuby merged 1 commit into
mainfrom
squad/roy-fmt-repair-main-1648

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

origin/main (73e6fe15a) fails a required check on an unmodified checkout:

error: methods `set_retain_decode_graph_across_spec` and
       `retain_decode_graph_across_spec` are never used
  --> crates/onnx-genai-engine/src/native_decode/cuda.rs:5516

That is the Check the native backend compiles step of Rust quality, so it blocks every open PR regardless of contents. It surfaced on an unrelated CPU-kernel PR (#1628).

Both methods are #[cfg(test)] accessors for the option-c graph-retention seam that #1648 landed as an enabling primitive — deliberately ahead of the WP4 tests that will drive them. Their #[cfg(test)] siblings either side (set_retain_graph_on_rewind, padded_query_capacity) are already called, which is why only these two trip.

Fix is #[allow(dead_code)] on the pair with the reason recorded at the site — rather than deleting a seam that is about to be used, or widening the allow to the whole impl block.

Local verification

gate result
cargo fmt --all -- --check clean
clippy -p onnx-genai-engine --features native-backend --all-targets -D warnings clean
clippy -p onnx-genai-engine --features native-cuda --all-targets -D warnings clean
clippy -p onnx-genai-cli --all-targets -D warnings clean
cargo test -p onnx-genai-engine --features native-backend 0 failed

Process note — this is the third main-is-red repair today, and I am only finding them because they land on an unrelated PR:

Each costs a full CI round-trip to discover, because the gates are sequential — the clippy steps only run once formatting passes. Running the Rust quality job on main post-merge, or a merge queue, would catch these at the source instead of on whoever's PR is open next.

`cargo clippy --locked --all-targets -p onnx-genai-engine --features
native-backend -- -D warnings` fails on an unmodified `origin/main`
(73e6fe1):

    error: methods `set_retain_decode_graph_across_spec` and
           `retain_decode_graph_across_spec` are never used
      --> crates/onnx-genai-engine/src/native_decode/cuda.rs:5516

That is CI's "Check the native backend compiles" step in `Rust quality`, a
required check, so it blocks every open PR.

Both methods are `#[cfg(test)]` accessors for the option-c graph-retention
seam #1648 landed as an enabling primitive -- deliberately ahead of the WP4
tests that will drive them. Their `#[cfg(test)]` siblings either side
(`set_retain_graph_on_rewind`, `padded_query_capacity`) are already called,
which is why only these two trip the lint.

`#[allow(dead_code)]` on the pair, with the reason recorded at the site, rather
than deleting a seam that is about to be used or widening the allow to the
whole impl block.

Verified locally on top of this change: `cargo fmt --all -- --check` clean;
clippy `-D warnings` clean for `-p onnx-genai-engine --features native-backend
--all-targets`, `--features native-cuda --all-targets`, and `-p onnx-genai-cli
--all-targets`; `cargo test -p onnx-genai-engine --features native-backend`
passes with 0 failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Aug 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.40%. Comparing base (73e6fe1) to head (c17b155).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1649      +/-   ##
==========================================
- Coverage   81.46%   81.40%   -0.07%     
==========================================
  Files         384      382       -2     
  Lines      180299   177247    -3052     
  Branches   180299   177247    -3052     
==========================================
- Hits       146886   144292    -2594     
+ Misses      28467    28025     -442     
+ Partials     4946     4930      -16     
Flag Coverage Δ
cli-ort-linux 82.60% <ø> (ø)
cli-ort-windows 82.11% <ø> (ø)
mlas ?
offline 81.36% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 3 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.

@justinchuby
justinchuby merged commit 90ddd28 into main Aug 21, 2026
14 of 16 checks passed
@justinchuby
justinchuby deleted the squad/roy-fmt-repair-main-1648 branch August 21, 2026 10:54
justinchuby added a commit that referenced this pull request Aug 21, 2026
`cargo fmt --all -- --check` fails on an **unmodified `origin/main`**
(`90ddd284e`):

```
crates/onnx-runtime-session/src/lib.rs:38
```

`pub use onnx_runtime_ep_api::DeviceGraphSlot;` was added above the
existing `WorkspaceRequirement` re-export rather than in sorted order.
One-line swap, pure `cargo fmt --all` output.

Formatting is a required check, so this blocks every open PR regardless
of contents. It surfaced on an unrelated CPU-kernel PR (#1628).

### This is the fourth main-is-red repair today

| # | PR | what was red on main | source |
|---|---|---|---|
| 1 | #1640 (not mine) | fmt + 3 clippy lints, four required gates at
once | #1637 / #1641 |
| 2 | #1642 | fmt, two sites | #1644 |
| 3 | #1649 | clippy `dead_code`, `native_decode/cuda.rs` | #1648 |
| 4 | **this** | fmt, one re-export | #1647 / #1648 |

The pattern is consistent and worth fixing at the source: quality gates
run on PR branches *before* merge but not on the merge result, so any
merge can land violations that then fail whoever opens the next PR.
Because the gates are sequential — clippy steps only run once formatting
passes — each breakage costs a full CI round-trip to even *discover*,
and they arrive one at a time.

Two concrete options: enable a merge queue (gates run on the merge
result), or run the `Rust quality` job on `main` post-merge so the break
is attributed to the PR that caused it instead of the next unrelated
one.

Also still red on main and **not** fixed here, because I cannot
reproduce it locally and it is not mine: `Rust (Windows ARM64)` → *Test
cross-platform offline crates* has been failing on main since at least
`73e6fe15a` (it is non-required, so it does not block merges).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 21, 2026
`Rust quality → Check formatting` is failing on `main` at `6923a016b`
(#1652). That job runs its steps sequentially, so while formatting is
red **every other check in it is skipped** — clippy, publish order, the
dispatch-manifest lints, feature-gate coverage, all of it. Every open PR
is blocked and none of them are getting linted.

Three sites, all in
`crates/onnx-genai-engine/src/native_decode/cuda.rs`: two
`verify_graph_phase` assignments (`:1815`, `:1823`) and one `assert_eq!`
in `verify_capture_helper_tests` (`:7042`). Straight `cargo fmt --all`
output, no hand edits.

**Verified inert.** The before/after texts are identical after stripping
whitespace *and* trailing commas — the only non-whitespace delta is
commas rustfmt adds before a closing delimiter when it breaks a call
across lines, which are semantically meaningless in Rust. Reproduced on
a pristine `origin/main` worktree first, so this is main's breakage and
not an artifact of my branch.

### This is the fifth time today

`main` has been red on formatting or clippy five separate times in one
day: #1637/#1641 (fixed by #1640), #1644 (#1642), #1648 (#1649),
#1647/#1648 (#1651), and now #1652.

The cause is structural, not carelessness. Required checks run on a PR's
**merge ref**, but nothing re-runs them on `main` **after** the merge,
so two PRs that are each green against an older base can land in
sequence and leave the result red. Because the quality job is
sequential, the breakage also masks every later step in it. The cost
lands on whoever opens the next PR, who then has to distinguish "my
change broke this" from "main was already broken" — a full CI round-trip
each time.

Two things would fix it, either one sufficient:
- a **merge queue**, which tests the actual post-merge result; or
- running **`Rust quality` on `main` post-merge**, which at least
detects it immediately and attributes it correctly.

I have now spent four PRs on this. I would rather not spend a fifth. cc
@justinchuby

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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