Skip to content

ci: Nightly's Rust benchmark tests build twice instead of twelve times - #8522

Merged
moq-bot[bot] merged 4 commits into
mainfrom
quest/m0/ci/nightly-rust-test-hour
Nov 12, 2026
Merged

moq-bot[bot] merged 4 commits into
mainfrom
quest/m0/ci/nightly-rust-test-hour

Conversation

@kixelated

@kixelated kixelated commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Nightly (Rust benchmark tests) hit its 60-minute timeout on every scheduled run since 11-07. In the 11-11 job the benchmarks themselves ran for about 4 minutes. The rest went to twelve separate release builds. Release is fat LTO with codegen-units = 1, so every test binary a build selects gets a slow single-threaded link. The moq-relay/moq-c/moq-mux/moq-cli invocation alone took 25 minutes (21 of them on moq-cli's test binaries) to run 5 tests.

The same log shows bench-feed-fetch ran 0 tests. benchmark_feed_fetch_wakes is behind moq-net's async feature, which went opt-in, and nextest passes a filter that matches nothing.

Approach

  • sh/gh/nightly.sh runs each benchmark-test group as one just rs bench-tests release build. It selects only the targets that hold benchmarks (--lib, --bin moq-bench/--bin moq, --test integration), so cargo links one binary per target instead of every test target of every package.
  • The work splits by layer into two matrix entries, each with its own hour:
    • rust-test: moq-bench, moq-congestion, moq-quic, moq-net (async), moq-tokio, moq-uring
    • rust-test-apps (new job Nightly (Rust application benchmark tests)): moq-audio (playback), moq-mux, moq-c, moq-relay, moq-cli, moq-ffi
  • Every benchmark that runs today still runs in a release build, one at a time (NEXTEST_TEST_THREADS=1), under the bench profile with one JUnit report per run, and both groups publish rust-test reports.
  • nightly.sh now fails a group when any test its filter names is missing from the JUnit report. This catches a test hidden behind a missing feature, which the static name check in nightly.test.py can't.
  • just rs bench-tests replaces bench-latency, bench-playback and bench-feed-fetch. With no CI caller left, the test inventory rejects those three recipes, and bench-tests covers the same local runs. Their references (moq-bench README, #[ignore] reasons, two audio quests) are updated.
  • sh/gh/test-inventory.py fixes a bug the change surfaced. When a [[bin]] named src/main.rs, the inventory still added a phantom binary named after the package, so --bin moq didn't cover moq-cli's tests. Cargo doesn't infer that binary, and a new inventory test covers the case.

Measurements

These are local from-scratch builds at -j4, to mimic the 4-vCPU runner:

build local CI
old moq-relay/moq-c/moq-mux/moq-cli invocation 1066 s 25 min
new rust-test 783 s ~18 min projected
new rust-test-apps 853 s ~20 min projected

The projections scale by the 1.4x ratio from the first row. The Rust cache only shortens these. The tests themselves ran for 262 s (rust-test, 19 tests) and 63 s (rust-test-apps, 9 tests) locally, and every test passed. The real JUnit reports pass the new name check and produce 1073 and 138 report entries.

Impact

  • No public API or wire change.
  • Nightly gains one job. The benchmark site prefixes series with the artifact name, so the benchmarks that moved to bench-rust-test-apps start a new history. Their old bench-rust-test series go stale.
  • benchmark_feed_fetch_wakes gets measured for the first time since moq-net's async went opt-in.
  • just rs bench-latency, bench-playback and bench-feed-fetch are gone. Use just rs bench-tests -p PACKAGE [--features ...] --run-ignored only -E 'test(NAME)' instead.

Alternatives

  • Thin or no LTO for benchmark tests: this would have been cheaper still, but it gives up the "release build" the quest requires and shifts every series.
  • One job for both groups: about 38 min of projected build time plus the run, which isn't "well inside" the hour.
  • Keeping the per-benchmark recipes and allowlisting them in the test inventory: the inventory requires a quest per allowlist entry, and bench-tests already covers those runs.

Choices made without asking

  • The new group is named rust-test-apps, and rust-test keeps the transport stack so most existing series keep their history.
  • moq-ffi now builds with default features in the apps group, sharing that group's codec build, instead of --no-default-features.

Caused by

  • moq-dev/moq#7607 - made moq-net's async opt-in without enabling it for bench-feed-fetch, so that benchmark silently ran nothing.
  • The timeout itself has no single cause: per-benchmark release builds accumulated as benchmarks were added.

Follow-ups

  • None. hakari follow-ups is updated: its baseline builds no longer exist in this job.

Review

mechanical fix by codex, gpt-6.1-sol, medium effort: Merged base and re·solved sh/gh/nightly.sh by retaining base's route benchmarks in the PR's grouped command; ran git diff --check base HEAD and nix develop --no-write-lock-file --option substituters '' -c just ci base (passed). (head 3b734b1)

Rewritten by quest review after each round.

  • Reviewer: Codex, gpt-6.1-sol, medium effort
  • Session: 01a1cd27-3c67-7c70-832c-f48f0b944a85, round 1
  • Head: e55f63e
  • Base: origin/main at 284c977
  • Score: 9/10, The split preserves benchmark coverage, reporting, and serial release-mode execution with no actionable defects found, though validation was source-only and no builds or tests were run.
  • Findings: none

Models

Size: M
Implementer: Claude Code, Opus 5.5, medium effort

(Written by Opus 5.5 with medium)

kixelated and others added 2 commits November 11, 2026 13:58
Each benchmark test ran in its own release build, and a release build links
every selected test binary with fat LTO, so the job spent its hour linking.
Two groups now build only the targets holding their benchmarks, in separate
Nightly jobs. A filter that runs no test now fails its group, which caught
the feed fetch benchmark measuring nothing since moq-net's async went opt-in.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title Nightly's Rust benchmark tests fit their hour ci: Nightly's Rust benchmark tests build twice instead of twelve times Nov 12, 2026
@kixelated
kixelated marked this pull request as ready for review November 12, 2026 00:28
@coderabbitai

coderabbitai Bot commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 54 seconds.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: be2d69a0-65f3-4b38-8e8b-6cad81bd2bda
Commits

Reviewing files that changed from the base of the PR and between 8cd4127 and 3b734b1.

Files selected for processing: 16

  • .config/nextest.toml
  • .github/workflows/nightly.yml
  • bench/README.md
  • quest/m0/ci/hakari-followups.md
  • quest/m0/ci/nightly-relay-session-hour.md
  • quest/m0/ci/nightly-rust-test-hour.md
  • quest/m1/audio-jitter-target/calibrate.md
  • rs/justfile
  • rs/moq-audio/src/encode/producer.rs
  • rs/moq-audio/src/playback/mixer.rs
  • rs/moq-audio/src/resample.rs
  • rs/moq-bench/README.md
  • sh/gh/nightly.sh
  • sh/gh/nightly.test.py
  • sh/gh/test-inventory.py
  • sh/gh/test-inventory.test.py

No actionable comments were generated in the recent review.

Walkthrough

The changes add a release-mode bench-tests recipe and replace dedicated benchmark recipes. Nightly Rust benchmark tests now run in transport/network and application groups, with workflow matrix entries and report validation for missing reports and unmatched test filters. Benchmark annotations, commands, and planning notes are updated. Cargo binary discovery now avoids inferring a package-named binary when an explicit target already claims src/main.rs; a regression test covers this case.

Merge Risk: Minimal · previous review 8cd41

This change reduces nightly Rust benchmark builds by splitting them into two groups and adds checks for missing reports and unmatched test names. No concrete merge-blocking risk was found. The remaining uncertainty is how long the new nightly jobs take on hosted runners.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the main change: the nightly Rust benchmark job now runs about two release builds instead of twelve. The raw summary confirms the split into grouped bench-tests builds in nightly.sh …
Description check Passed The description is directly related to the changeset. It explains the nightly timeout, the zero-test bench-feed-fetch recipe, and the split into rust-test and rust-test-apps matrix entries. It a…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: the nightly Rust benchmark job now runs about two release builds instead of twelve. The raw summary confirms the split into grouped bench-tests builds in nightly.sh and the workflow, which matches the title's focus. The title is short and specific, though its 'twice instead of twelve' phrasing is slightly informal.

Full details: Description check

Explanation

The description is directly related to the changeset. It explains the nightly timeout, the zero-test bench-feed-fetch recipe, and the split into rust-test and rust-test-apps matrix entries. It also covers the bench-tests recipe that replaces the removed recipes, the nightly.sh failure check, and the test-inventory.py fix. These all match the raw summary of file changes.

Finishing Touches

Simplify code

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c5990e0d-c18f-4743-95d2-9553d26ed53a
Commits

Reviewing files that changed from the base of the PR and between e55f63e and 8cd4127.

Files selected for processing: 4

  • .github/workflows/nightly.yml
  • rs/moq-audio/src/playback/mixer.rs
  • sh/gh/nightly.test.py
  • sh/gh/test-inventory.test.py
Files skipped from review as they are similar to previous changes: 1

  • rs/moq-audio/src/playback/mixer.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes

  • One build per benchmark group: sh/gh/nightly.sh now runs rust-test as a single just rs bench-tests release build that selects only the targets holding benchmarks (--lib, --bin moq-bench/--bin moq, --test integration), and splits the work into rust-test (transport stack) and a new rust-test-apps group (audio/c/mux/relay/cli/ffi).
  • Missing-test guard: nextest() now fails the group when any test(NAME) in the filter has no matching <testcase> in the JUnit report, so a benchmark behind an absent feature no longer passes silently.
  • Workflow split: .github/workflows/nightly.yml gains the Rust application benchmark tests matrix entry with its own artifact/path (bench-rust-test-apps), both groups reporting as rust-test.
  • Recipe consolidation: bench-latency, bench-playback, and bench-feed-fetch are replaced by a generic bench-tests *args; the inventory rejects the removed recipes and the docs, #[ignore] reasons, and audio quests are updated to the surviving commands.
  • Inventory fix: sh/gh/test-inventory.py no longer invents a package-named binary when a [[bin]] already claims src/main.rs, which is what made --bin moq look like it did not cover moq-cli's tests; a new inventory test covers the renamed-binary case.
  • Test coverage: sh/gh/nightly.test.py parametrizes both groups, makes the JUnit stub emit one <testcase> per filtered name, and adds a case asserting a filter that matches nothing fails the group.

I traced every benchmark name in both filters to a selected target: lib tests for moq-bench, moq-congestion, moq-quic, moq-net, moq-audio, moq-mux, moq-c, moq-relay, moq-cli, and moq-ffi; tests/integration for moq-net's mesh_sweep, moq-tokio's receive_credit/datagram_delivery, and moq-uring's benchmark_socket_queues module; and the moq-bench/moq-cli bins. sh/gh/nightly.test.py NightlyTest passes (14 tests) in this sandbox; sh/gh/test-inventory.test.py could not run here because just is not installed, which is an environment limitation rather than a PR issue.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

…ust-test-hour

# Conflicts:
#	quest/m1/audio-jitter-target/stretch.md
@kixelated
kixelated added this pull request to the merge queue Nov 12, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 12, 2026
@moq-bot

moq-bot Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at 8cd4127 because it conflicts with main (merge_conflict).

The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.

(written by quest land watch)

@moq-bot
moq-bot Bot enabled auto-merge November 12, 2026 03:58
@moq-bot

moq-bot Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

quest land watch: armed auto-merge on this pull request at 3b734b1, so it joins the back of the merge queue once its required checks pass, with the land fixer's fix (codex, gpt-6.1-sol, medium effort): Merged base and re·solved sh/gh/nightly.sh by retaining base's route benchmarks in the PR's grouped command; ran git diff --check base HEAD and nix develop --no-write-lock-file --option substituters '' -c just ci base (passed)..

(written by quest land watch)

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 12, 2026
Merged via the queue into main with commit 9341532 Nov 12, 2026
8 checks passed
@moq-bot
moq-bot Bot deleted the quest/m0/ci/nightly-rust-test-hour branch November 12, 2026 04:14
kixelated added a commit that referenced this pull request Nov 12, 2026
…8522

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.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