Repository navigation
fix(ci): report unfinished nightly runs and cut the test build cost - #1017
Merged
Merged
Conversation
`nightly-verify` had two independent defects: the `cargo test` step never finished inside the 180-minute job budget, and when the job was killed by that budget nobody was told. Three of the last five runs ended at exactly 3h00m and were reported as `cancelled`, which reads in the run list as "someone cancelled it". Reporting. The old `Report a red main` step was conditioned on `failure()`, and a `timeout-minutes` kill surfaces as cancellation, where `failure()` is false. A throwaway probe on a job with `timeout-minutes: 1` established the exact semantics: after the timeout an `if: always()` step ran, an `if: cancelled()` step ran, only the `if: failure()` step was skipped, and job outputs survived. So a wider condition would have covered the timeout. Reporting still moves to a separate `report` job with `needs: verify` and `if: always()`, for three failure modes no in-job step can reach. The 2026-08-03 run is the concrete one: `verify` failed because `brew` was missing from the runner's PATH, and the in-job reporter then died the same way trying to `brew install gh`, so a genuinely red run reported nothing. The other two are outright runner loss, where no step in the job runs at all, and the bounded post-cancellation window, which a reporter that has to install `gh` is far likelier to exceed than the probe's single `echo`. The new job runs on `ubuntu-latest`, which ships `gh`, and reads `needs.verify.result` so the report distinguishes a red suite from a run that never finished. It reports on `workflow_dispatch` as well as `schedule`: the exemption assumed a manual dispatch has someone watching, and the 2026-08-02 dispatch ran three hours, timed out and told nobody. Build cost. `verify-test` now builds under `[profile.test-fast]` instead of `[profile.release]`. The budget was going to codegen and linking, not to tests: the killed run logged zero `Running tests/` lines and the runner terminated `ld` and `clang`. `opt-level = 3` is unchanged, so the optimised MLX numerics the suite depends on are the same; fat LTO and `codegen-units = 1`, which exist to tune a shipped binary, are dropped. What that gives up is a defect reproducing only under release codegen, the f16 reduction-order jitter class among them. Nothing that ships changes: release.yml still builds and links under `[profile.release]`. Docs and templates that named the old gate are updated with it: CONTRIBUTING.md, docs/installation.md, the PR template, and the pre-push checklists quoted in ci.yml and release.yml. The header comment in nightly-verify.yml carries the profile tradeoff, the reporting rationale, and a correction to its stale "~30 min" figure, which the one completed release-profile pass (2h56m) contradicts. Refs #1000. Coverage across the other three workspace members is #1007 and deliberately not touched here; adding 1754 tests to a job that could not finish would only make it time out sooner.
inureyes
added
type:bug
Three comments in the report job overstated what a `timeout-minutes` cancellation destroys, which the probe run contradicts. Job outputs do survive a timeout cancellation, so the normal timeout report is fully populated and `unknown` is reserved for outright runner loss. Separately, `runs-on: ubuntu-latest` is what answers the Homebrew fate-sharing problem, while being a separate job at all is what answers runner loss and the bounded post-cancellation window; the previous wording attributed all three to the runner choice. Comment-only, no behavior change.
The note told a reader that an `unknown` step outcome is the normal reading when the job did not finish. The probe run showed the opposite: job outputs survive a `timeout-minutes` cancellation, so a timeout report carries real per-step outcomes and `unknown` instead points at the runner being lost outright. The note now says that, which is the difference between "check the budget" and "check the runner". Comment and report-text only, no behavior change.
…it is not Two defects the first real run of this workflow exposed. The `Ensure build tools` step assumed `cmake` or, failing that, `brew` is resolvable. Neither is: a non-login shell on the self-hosted runner does not source the profile that `brew shellenv` writes. Run 30842184319 (2026-08-03 schedule) and run 30870790330 (2026-08-04 dispatch, on this branch) both died there in about one second with exit 127, the second one before any gate could run, which is why this branch had no runner measurement to show. Both Homebrew prefixes are now prepended, Apple Silicon and Intel, and they are published to GITHUB_PATH so later steps inherit them. If cmake is still missing the step now says it is a provisioning problem and prints PATH, rather than failing on a missing `brew`. The report from that run was titled "main is red" for a run that verified a pull-request branch, and its headline asserted that at least one of the three gates was genuinely red. Neither was true: the job died in a setup step, and fmt, clippy and test then reported `failure` only because the toolchain step had been skipped. The title now names the ref when it is not the default branch, which also keeps a branch run from reusing the scheduled runs' deduplicated issue, and the headline points the reader at the first failing step before the gate table. Refs #1000.
Run 30870981405 got past the Homebrew fix and then failed in both clippy and test with `xcrun: error: unable to find utility "metal", not a developer tool or in PATH`, raised by cmake while resolving the MLX kernel headers under `.cargo-target/mlxcel/debug/build/mlxcel-core-*/out/build/_deps/mlx-src`. The runner's `xcode-select` points at the Command Line Tools, which ship no Metal compiler. This is the same condition docs/mla-absorbed-decode.md and docs/sparse-paged-decode.md already tell developers to fix by exporting DEVELOPER_DIR. It stayed invisible because a warm CARGO_TARGET_DIR skips the MLX C++ build entirely, and every run until now was warm: run 30761020020 finished a full pass with a 71-second clippy on that basis. The first run that needed a cold build failed immediately. Since release.yml prunes the whole persistent target directory every 7 days, this was going to surface on its own regardless of the profile change. The step now probes `xcrun -f metal`, and only if that fails walks the installed Xcode bundles, selecting the first whose Developer directory makes metal resolvable and publishing it as DEVELOPER_DIR for the later steps. When nothing works it says the runner is under-provisioned and prints `xcode-select -p` and the Xcode installs it found, rather than letting cmake fail hundreds of lines later with a header error that reads like a code problem. Refs #1000.
Member
Author
Runner measurementDispatched on this branch after the two follow-up commits (
A 3.6x reduction on the step that was consuming the entire budget. The "after" number is an upper bound, not the steady state. The preceding run on this branch (30870981405) died in the Metal error before it could finish populating the The reporting path, verified end to end on the runnerThree observations across three dispatches, covering both directions:
The first of those is the fate-sharing case this PR's design argument rests on, and it was not hypothetical: the job died at Two runner defects this surfaced, both fixed hereNeither was caused by the profile change; both were latent and would have surfaced on their own the next time
Also corrected: the report titled itself |
This was referenced Aug 4, 2026
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
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
nightly-verifyhad two independent defects: thecargo teststep never finished inside the 180-minute job budget, and when the job was killed by that budget nobody was told. This fixes both. Coverage of the other three workspace members (#1007) is deliberately out of scope, and #964 is being fixed separately.Phase 1: make an unfinished run visible
The old
Report a red mainstep was conditioned onfailure()and reported nothing on the three-hour runs.A correction to the premise this was scoped on. The step was not skipped because a job timeout tears the job down before later steps can run. It was skipped because the condition did not match: a
timeout-minuteskill surfaces as cancellation, andfailure()is false while a job is cancelled. Later steps do still run. A throwaway probe on a one-minute job settled it directly:sleep 300cancelledalways stepif: always()cancelled stepif: cancelled()failure stepif: failure()skippedThe probe also showed job outputs survive a timeout cancellation: the downstream job read both
steps.sleeper.outcome(cancelled) and a custom output set before the sleep.So a step-level condition fix would have covered the timeout case. Reporting still moves to a separate
reportjob, for three failure modes no in-job step can reach:brewwas not on the self-hosted runner's PATH, and the in-job reporting step then died the same way,brew install ghexiting 127. A genuinely red run reported nothing, for a reason unrelated to timeouts.echo. A reporter that has to installghand make several API calls is far likelier to be cut off.The new job runs on
ubuntu-latest(shipsgh, no Homebrew needed, and keeps reporting off the scarce macOS runner), usesneeds: verify+if: always(), and readsneeds.verify.result.failureandcancelledproduce different titles, so the two kinds dedup onto separate tracking issues:failureproduces[nightly-verify] main is redcancelledproduces[nightly-verify] the run did not finishThe existing body composition is preserved (step-outcome table, commit, run URL) with per-step outcomes passed across the boundary via job
outputs, plus theverifyjob result, the trigger, and the job duration. Duration is what separates "ran out of budget" from "a person cancelled it", so the body states it and points attimeout-minutesrather than hardcoding 180, which would drift.workflow_dispatchreporting: now includedReversed from the previous rule. That rule assumed a manual dispatch has someone watching, so filing at them is noise. The 2026-08-02 dispatch disproved it: it ran three hours, timed out, told nobody, and was only found because someone went looking. Nobody watches a job for three hours. The reintroduced noise is bounded because repeat dispatches comment on one deduplicated issue rather than filing new ones, and the body names the trigger so a reader can dismiss a run they cancelled themselves.
Phase 2: cut the build cost
verify-testnow builds under[profile.test-fast]instead of[profile.release].--profile test-fastand--releaseare mutually exclusive, so the flag is replaced, not added.The budget was going to codegen and linking, not to tests: run 30733081983 logged zero
Running tests/lines and zerotest result:lines, and the processes the runner terminated wereldandclang.[profile.release]is fat LTO pluscodegen-units = 1, measured at 4 to 6 minutes per incremental rebuild of the ~390k-line main crate, paid across roughly 77 test binaries.The tradeoff, stated plainly
opt-level = 3is unchanged, so the optimized MLX numerics the suite depends on are the same. What is dropped is fat LTO andcodegen-units = 1, which exist to tune a shipped binary.Still covered: everything the suite asserts, under optimized codegen, with
metal,accelerate, on the real hardware class.panic = "unwind"is inherited from release, so thecatch_unwindisolation tests behave identically.No longer covered: a defect that reproduces only under fat LTO or single-unit codegen. The f16 reduction-order jitter class this repo has hit before is the realistic example.
cargo test --release --features metal,accelerateby hand remains the tool for chasing one, and is documented as such.Unaffected: what ships.
release.ymlstill builds and links the shipping artifacts under[profile.release].verify-clippyis left alone. It has no--releasetoday, takes 67s warm on the runner, and changing it would risk a cold debug MLX build for no measured gain.Two operational notes
OUT_DIRfor themlxcel-corebuild script, so the first run pays a full cold MLX C++ build against the persistent$HOME/.cargo-target/mlxcel. Do not read that run as a failed fix. Locally this cost was confirmed: a fresh 596Mtarget/test-fast/build/mlxcel-core-*/outwas produced.target/releaseis 9.9G andtarget/test-fastis 10G).release.yml's 7-day prune isrm -rfof the whole directory, not a per-profile clean, so both are already bounded by it and no third rule is needed. A second cost worth naming: the nightly no longer refreshes[profile.release]artifacts, sorelease.yml's first build after a prune is colder than it used to be.Local measurement, and its caveat
Development machine, M1 Ultra, not the runner. The self-hosted runner is a different and slower shared machine, so treat this as directional only.
Cold for this profile, including the full MLX C++ build and the link of every root test binary: 14m56s wall clock, exit 0, 76 test executables. For contrast, on the runner the release-profile equivalent did not finish in 179 minutes on 2026-08-02.
The runner numbers are not mine to produce. The workflow has not been dispatched from this branch. The PR is ready to have the nightly dispatched against it for before and after.
One data point that arrived after the issue was written
Run 30761020020 (2026-08-02 schedule) succeeded, the first full pass the workflow has ever completed. Its
cargo teststep ran 2h54m34s inside the 180-minute budget: about 5 minutes of headroom, roughly 3%. That does not weaken the case, it sharpens it. The gate was passing on a knife edge, and #1007 proposes adding 1754 tests and three more link steps to it.What changed
.github/workflows/nightly-verify.yml: newreportjob onubuntu-latestwithneeds: verifyandif: always();outputsonverifyto carry step outcomes across the boundary; the in-jobReport a red mainstep removed;issues: writemoved to the job that needs it; test step renamed; header comment updated for the profile tradeoff, the reporting rationale, the disk and cache-warming costs, and a correction to its stale "~30 min" figure that the 2h56m completed pass contradicts.Makefile:verify-testbuilds--profile test-fast; the "how these differ" comment block and thetest-fastsection comment updated.Cargo.toml: comment-only.[profile.test-fast]now documents that it backsverify-test, and the "do not use for anything that ships" note no longer points atverify-test.CONTRIBUTING.md,docs/installation.md,.github/PULL_REQUEST_TEMPLATE.md: the documented gate matches what the gate now runs..github/workflows/ci.yml,.github/workflows/release.yml: comment-only, the quoted pre-push checklists saidtest(release).No Rust source is touched.
Test plan
cargo fmt --all -- --checkclean.cargo clippy --all-targets --profile test-fast --features metal,accelerate -- -D warningsclean. Run undertest-fastrather than a cold debug build because the diff contains no Rust changes and theCargo.tomlchange is comment-only.cargo test --profile test-fast --features metal,accelerate --no-run: exit 0, 76 executables, 14m56s cold.if:expression resolve as intended.ghanddatein three states: timeout (cancelled, partial outputs, duration resolved to 180m21s from the real 30733081983 timings), genuine failure (failure, all outcomes present), and fully degraded (API unreachable and every output empty). All three exit 0 and emit a complete report; the degraded one reportsunknownper step rather than failing.python3 scripts/ci/check_cross_repo_refs.pyclean.Closes #1000