[kixelated]

Problem

Nightly 42236107954 kills benchmark_socket_queues at its 60-second per-test hang limit. The single test contains 117 measurements with one second of offered load each, plus draining. Its progress log shows it was measuring normally when terminated.

Approach

Make each of the ten sweep axes an ignored benchmark test. Keep the same cells, loads, accounting, and three interleaved rounds across both receive paths. Each axis reserves all four slots of the existing io-uring nextest group, serializing it with other axes and ring tests without escaping the shared ring budget (nextest group semantics). When io_uring is unavailable outside strict CI, the pool-only test emits an explicit capability skip that the report accepts while retaining Tokio measurements.

Impact

• Benchmark organization only; no public API or wire changes.
• Test names gain an axis suffix. Nightly’s existing filter still selects the entire grid.
• Repetitions interleave within each axis instead of across the entire grid. No timeout increase or workload reduction.

Alternatives

Raising the hang limit hides slow tests. Shortening each cell or reducing rounds would change the measurement; partitioning preserves coverage and load.

Follow-ups

The independent memlock failures are owned by moq-dev/moq#8051.

Validation

• just rs test -p moq-uring --release -E 'test(benchmark_socket_queues)' --run-ignored only --success-output immediate: all ten tests and all 117 CSV rows passed, each axis 8–24 seconds with no SLOW/TIMEOUT.
• Verified all three rounds of every axis on Tokio and io_uring (pool remains io_uring-only).
• Rechecked the default axis including the coverage assertion; the capability-skip follow-up leaves supported-path workloads unchanged.
• Final configuration uses the existing four-slot io-uring group. Each axis requires all four slots, preserving the ring budget in mixed local runs as well as Nightly’s global single-thread run.
• Reproduced the old report failure with a header-only pool testcase. The JUnit regression fixture now preserves Tokio measurements beside the explicit pool capability skip, and rejects empty, header-only, bare-marker, and mixed marker/table outputs. All 16 report tests pass.
• just ci passed at 05c889eaa4, including 11,924 Rust tests and the Nightly selector check.
• At the final shared-group configuration, just ci passed all non-Rust-test modules; three unchanged relay ring tests failed allocation with ENOMEM / per-user RLIMIT_MEMLOCK=8192 KiB. On unchanged main (f2f9bc935e), CI_STRICT=1 NEXTEST_PROFILE=ci just rs test -p moq-relay --features io-uring -E 'test(uring::tests::pending_admission_is_abandoned_at_the_deadline)' reproduced the same startup failure. This is the shared-host limit already owned by moq-dev/moq#8051.
• The final two-axis probe with --test-threads 8 also stopped at ring allocation before measurement, so runtime overlap verification remains blocked by that host limit. The configuration preserves the documented shared budget; the previously measured grid is unchanged. Final Markdown check passed.

Review

Claude Code / Claude Opus 5.5, configured medium effort, reviewed fa1a85d64b: clean. This fourth round checked the shared-group correction and its effects on local and Nightly scheduling. Its design judgment favors reserving the existing ring budget over a separate group. Exclusivity follows from configuration and nextest semantics; the runtime probe remains blocked by the base-equivalent host limit recorded above. Prior rounds fixed axis-list drift and the unsupported-io_uring report regression. No timeout increase or workload reduction.

(Written by GPT-6.1 Sol with xhigh effort)

[coderabbitai]

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 11 minutes.

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: 7ba73474-010c-43ce-aa3d-cb419a2cdf23

Commits

Reviewing files that changed from the base of the PR and between 36cbc21 and fa1a85d.

Files selected for processing: 7

• .config/nextest.toml
• bench/README.md
• rs/moq-uring/README.md
• rs/moq-uring/tests/integration/socket_queues.rs
• sh/gh/bench-report.py
• sh/gh/bench-report.test.py
• sh/gh/nightly.test.py

• 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

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed head: 0868cefaf56eb57ea876f118b8252f56f7c7bd58.

Found one P2 regression in the unsupported-io_uring fallback, detailed inline. Splitting the sweep by axis preserves the current cells and round counts and is consistent with the hang-budget direction; please preserve report generation for Tokio-only runs as well.

Scope: all four changed files, surrounding benchmark setup/measurement code, nextest overrides, Nightly selection, and JUnit-to-CSV report parsing. Verification limits: GitHub-only static review; I did not execute Rust benchmarks, Python tests, or just ci, and have not independently verified the reported timings. The head's Check workflow was still in progress at the final check. Rechecked open/non-draft state, exact head, and existing reviews/comments immediately before submission.

[kixelated]

[P2] Preserve reporting when the standalone pool axis cannot run

On a Linux host where uring_supported() returns false and CI_STRICT is unset, this new pool test has only Path::Tokio, and the existing skip at lines 284–286 removes every cell. It therefore passes with a CSV header but no measurement rows. sh/gh/bench-report.py:206–213 feeds every JUnit testcase into printed(), which raises ValueError("benchmark produced no measurements") at lines 178–179 for this output. Consequently just gh nightly rust-test fails to generate its entire report, including otherwise valid Tokio measurements. Before the split, those measurements shared the same testcase and kept the report nonempty. Please make the unavailable pool case explicitly skippable by the reporting pipeline (without accepting arbitrary empty benchmarks), and add a JUnit/report regression fixture for the unsupported-io_uring path.

[kixelated]

Fixed in 05c889eaa4. An unavailable pool axis prints only an explicit capability-skip marker before any CSV header. The JUnit importer accepts that marker alone, keeps Tokio measurements, and still rejects accidental empty/header-only output. The regression fixture covers Tokio plus skipped pool and malformed skip outputs. All 16 report tests and just ci pass; the new head received a clean local Claude review.

(Written by GPT-6.1 Sol with xhigh effort)

[pullfrog]

ℹ️ Minor suggestion only: the new group's placement changes which concurrency bound the socket benchmark counts against.

Reviewed changes

• Per-axis tests — benchmark_socket_queues becomes a module whose axes! macro emits one ignored test per sweep axis, each calling the shared benchmark(axis) body, so no single process carries all 117 measurements past the per-test hang budget.
• Drift guard — benchmark() asserts every Cell.axis has a matching test, keeping cells() and the macro's AXES list in sync.
• Serialization — a socket-queues test group (max-threads = 1) assigned through an override placed ahead of the io-uring one, which first-match precedence honors.
• Nightly selector guard — sh/gh/nightly.test.py accepts mod <name> as proof a filtered name exists, matching the new module-shaped test names.
• Docs — the module docblock and rs/moq-uring/README.md describe per-axis tests, three rounds, and interleaving within an axis.

I confirmed the parts that are easy to get wrong: nextest's test() default matcher is a contains match, so nightly's -E 'test(benchmark_socket_queues)' still selects all ten tests; test-inventory.py resolves macro-named modules and fn $name to …::benchmark_socket_queues::axis and matches the nightly -E as a contains test, so none of the new ignores is an orphan; and bench-report.py reads each JUnit testcase separately, so the ten headers and print_host() blocks do not collide.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed head: 05c889eaa408478079fe8087d88ae7b442f67af9.

Re-reviewed the substantive follow-up since 0868cefaf56eb57ea876f118b8252f56f7c7bd58. My previous P2 finding is addressed: rs/moq-uring/tests/integration/socket_queues.rs:266–269 now emits the explicit skip before host/CSV output for an unavailable pool path, and sh/gh/bench-report.py:213–215 recognizes that standalone marker after removing harness lines. The fixture in sh/gh/bench-report.test.py:200–239 covers retaining Tokio measurements and rejecting empty/header-only output and malformed skips.

No new actionable findings in the follow-up. The direction remains appropriate: preserve the axis partitioning and full supported-path workload while handling unavailable capabilities explicitly.

Verification limits: GitHub-only static review of the four-file follow-up and its surrounding benchmark/reporting code, with the full PR diff rechecked. I did not run benchmarks, Python tests, or just ci; the author's reported test results were not independently reproduced. The current head's Check workflow was still in progress. Rechecked open/non-draft state, full head SHA, and reviews/comments immediately before submission.

[pullfrog]

ℹ️ No new issues in the delta. The capability-skip contract closes the unsupported-io_uring report regression. The earlier note about the socket tests leaving the io-uring group is still open, so this is not an approval; no code change is requested from this review.

Reviewed changes

• Capability skip for the pool axis — benchmark("pool") now returns before print_host() and the CSV header when io_uring is unavailable, emitting only # benchmark skipped: io_uring is unavailable; Tokio has no receive pool.
• Report tolerance for that marker — rust_tests() drops a testcase whose entire output is a # benchmark skipped: REASON line, so the other nine axes' Tokio measurements still reach the report; empty, header-only, marker-plus-table, and malformed outputs still raise.
• Regression fixture — a JUnit report pairs a measured default axis with a skipped pool axis and asserts only the Tokio entry survives, then rejects four malformed skip outputs.
• Docs — bench/README.md states the capability-skip contract: the marker alone is omitted while an empty table or missing measurements is refused.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}

[kixelated]

Automated review by review (OpenAI)

Reviewed head: fa1a85d64b75b10428c4ebe98d6a3bf7686961ee.

No new actionable findings in the two-file delta since 05c889eaa408478079fe8087d88ae7b442f67af9. The change at .config/nextest.toml:25–31 keeps the socket axes in the existing io-uring group and reserves all four slots. Nextest's documented weighting applies to both group and global limits, so this addresses the shared-budget concern without changing the measured workload. The README now describes that scope accurately. My earlier capability-skip finding remains addressed.

Direction: retaining the shared ring group is appropriate; no additional code change requested from this review.

Verification limits: static GitHub review of the delta, full PR diff, surrounding configuration, and upstream nextest documentation. I did not run tests or independently reproduce the reported memlock failures. The PR records that the final runtime overlap probe and Rust-test validation were blocked by host allocation failures, so runtime validation remains incomplete. The latest Check workflow for this head was still in progress. Rechecked open/non-draft state, exact head, and reviews/comments immediately before submission.

[pullfrog]

✅ No new issues found.

Reviewed changes

• Moved the socket axes from the dedicated socket-queues group onto the existing io-uring group with threads-required = 4, so each axis reserves the whole documented ring budget instead of escaping it.
• Removed the now-unused socket-queues group definition.
• Updated rs/moq-uring/README.md so the "Parallel test runs" section describes the shared four-slot reservation and the exclusion of other ring tests during measurement.

Verified the parts the change turns on: threads-required weighs against both the global and group pools, and nextest clamps the weight to each pool's maximum. On Nightly's NEXTEST_TEST_THREADS=1 run the axis therefore proceeds (clamped to one global slot) while still occupying all four group slots, so there is no deadlock and the mutual exclusion with other ring tests holds even on hosts with fewer than four test threads. Profile inheritance defaults to default, so the override also applies to the bench profile, and the config parses cleanly.

  ^{｜ View workflow run ｜ Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏}