ci(test): run the monoio suite — CI was testing the fallback runtime, not the shipped one - #464
Conversation
… not the shipped one
Every CI job that EXECUTED tests did so under `--no-default-features
--features runtime-tokio,...`. Moon's default feature set is runtime-monoio,
and that is what ships on Linux. The result: 26 monoio integration test files
and 30 monoio-gated src/ files were unreachable by CI, and the documented local
gate in CLAUDE.md ("Local CI Parity", which runs BOTH suites) was strictly
stronger than CI itself.
That gap is not theoretical. The v0.8.6 inline-GET ACL bypass (#457) was wrong
only on the monoio dispatch path and shipped green. The RESP3 type-fidelity
work (#463) had to hand-verify one of its two enqueue sites locally, because CI
structurally could not see it.
Adds `check-monoio`: self-hosted Linux runner (the only place monoio's io_uring
driver executes at all), default feature set, `cargo nextest run --profile ci`,
its own CARGO_TARGET_DIR, no continue-on-error, MOON_NO_URING deliberately
unset.
`--profile ci` is load-bearing rather than incidental: the suite has a known
load-sensitive flake class, a bare `cargo test` has no retries, and an
intermittently-red required job gets disabled -- which is worse than no job,
because it still looks like coverage. The existing profile's retries=2 absorbs
it while still reporting FLAKY, so the signal survives.
Measured on moon-dev (kernel 6.17) before landing:
5145 passed, 1 flaky, 244 skipped, exit 0, 80.3s of test time.
VERIFIED BY NEGATIVE CONTROL, not by inspection. A CI-config change can be
green and still be worthless, so the claim was tested directly: a deliberate
defect injected on `try_inline_dispatch` (cfg(feature = "runtime-monoio"), so
tokio cannot reach it) making inline GET answer "$6\r\nBROKEN\r\n":
tokio (CI before this change) : multi_queues_inline_get 6 passed <- ships green
monoio (the new job) : multi_queues_inline_get 3 FAILED <- caught
Reverted immediately; zero residual markers, and the suite back to 6/6.
tests/ci_covers_monoio.rs guards the job itself, because the failure mode of CI
coverage is silent -- a job that stops running or is switched to the wrong
feature set looks exactly like a green build. It fails on a wrong feature set,
continue-on-error, a bare `cargo test`, a shared target dir with the tokio job,
or removal of tokio coverage.
NOTE FOR THE REVIEWER: adding the job makes it RUN, not BLOCK. It must be added
to branch protection to gate merges; until then it is advisory.
Refs: .add/tasks/monoio-ci-coverage (gate PASS), milestone v0-9-client-compat
author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe pull request adds a self-hosted Monoio CI coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant SelfHostedRunner
participant CargoNextest
participant MonoioTests
GitHubActions->>SelfHostedRunner: Start check-monoio job
SelfHostedRunner->>CargoNextest: Run default-feature tests with --profile ci
CargoNextest->>MonoioTests: Execute monoio/io_uring test suite
MonoioTests-->>CargoNextest: Return test results
CargoNextest-->>GitHubActions: Propagate pass or failure
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/ci_covers_monoio.rs`:
- Around line 57-60: Update the workflow validation tests around the
job-command, runner, and environment checks to inspect parsed YAML or only
non-comment executable fields instead of arbitrary job text. Ensure they
validate the effective run command, self-hosted runs-on value, inherited
MOON_NO_URING environment, and non-skipped if condition; add regression coverage
for commented-out commands, non-self-hosted runners, inherited MOON_NO_URING,
and skipped jobs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b023538-b21a-448d-abda-4b237842a695
📒 Files selected for processing (5)
.add/state.json.add/tasks/monoio-ci-coverage/TASK.md.github/workflows/ci.ymlCHANGELOG.mdtests/ci_covers_monoio.rs
… was a workflow-level global `check-monoio` exists for one reason: to execute the io_uring driver that actually ships on Linux. Its comment said "MOON_NO_URING is deliberately NOT set here", its job block was clean, and `ci_covers_monoio.rs` asserted that block stayed clean. All three were checking the wrong scope. `MOON_NO_URING: "1"` sat in the workflow-level `env:`, which merges into every job — and a job cannot unset an inherited key (an empty value is still a set variable to `env::var_os`). So the one job whose entire premise was io_uring ran with io_uring force-disabled, and had done since the job was written. `monoio_yield_overhead_is_microscopic` reported it: every `cooperative_yield()` fell through `uring_active()` to the `sleep(ZERO)` timer park, 290ms for 200 yields = 1.45ms/yield, against a 100ms budget. That read like a load-sensitive flake on a shared runner. It was not — same binary on moon-dev, the env var as the only delta: io_uring active -> ok, 0.00s MOON_NO_URING=1 -> FAILED, 0.59s Deterministic in both directions. The test was right and the config was wrong. Fix: MOON_NO_URING moves out of workflow-level env onto the jobs that want it — `check` (tokio; the io_uring bridge floods errors under load), `memory-steady- state` (real server on a GitHub-hosted runner where io_uring may be seccomp- restricted), and `client-compat` (kept so its recorded waiver baseline stays comparable; the differ probes wire shapes, not drivers — commented as such, so it is documented rather than silent). Dropped from macOS/Windows/msrv/console/ lint, where it was inherited dead config: no io_uring on those platforms and those jobs execute no Moon. `ci_covers_monoio.rs` gains the assertion it was missing — the workflow-level `env:` block must not define MOON_NO_URING either. Red before the yaml change for exactly that reason, green after; 5/5. Verified on moon-dev with the job's exact fixed config (default features, io_uring live, nextest --profile ci), which no CI run had ever exercised. author: Tin Dang
# Conflicts: # .add/state.json
The new job caught its own configurationFirst run of 1.45ms/yield is the So the one job whose entire premise is io_uring had been running with io_uring force-disabled Same binary on
Deterministic both directions. The test was right; the config was wrong. Fix. Verified in the job's exact fixed config, which no CI run had ever exercised — |
…malizing reader (#468) * fix(test): ci_covers_monoio must read the workflow through a CRLF-normalizing reader All 5 tests in `tests/ci_covers_monoio.rs` failed on the Windows job of the first main push after #464. Not a Windows runtime problem — the suite is a repo-config test that parses `.github/workflows/ci.yml` with the needle `"\n <job>:\n"`. A Windows checkout (`core.autocrlf=true`) ends every line `\r\n`, so the needle never matches, `job_block` returns `None`, and each caller's `.expect()` panics. Same for `workflow_env_block`. Mechanism confirmed against the real file rather than assumed: LF content, needle found: True CRLF content, needle found: False <- the panic CRLF after normalize : True `workflow()` now normalizes through `normalize_newlines`, and a new test — `the_parsers_survive_a_windows_crlf_checkout` — feeds a synthetic CRLF fixture through both parsers. It runs on every platform, which matters: Windows is skipped on all PRs, so this class is invisible until a main push. Verified non-vacuous by neutering the normalizer, which reddens exactly that test with its own message, then restoring it (6/6 green). Note for whoever reads the same main run: the Crash Matrix failure alongside this one is unrelated — it lives in `integration-tests.yml`, which #464 never touched and which sets its own workflow-level MOON_NO_URING. That failure is the known 8s-startup timeout on a hosted runner. author: Tin Dang * fix(test): the CRLF regression test made the same assumption it tests for `the_parsers_survive_a_windows_crlf_checkout` built its fixture as `read_to_string(..).replace('\n', "\r\n")`. On a Windows checkout the bytes on disk are ALREADY CRLF, so that yields `\r\r\n` — a fixture no correct normalizer can rescue — and the test failed on its own first Windows run while correctly clearing the five failures it was written to fix. Assuming the file is LF is precisely the bug under test. Normalize first, then synthesize CRLF, and assert the fixture is well-formed (`\r\n` present, `\r\r` absent) so a malformed fixture reddens loudly instead of passing vacuously. Simulated both checkout styles against the real file: LF checkout fixture_ok=True parse_ok=True (previous version: True) CRLF checkout fixture_ok=True parse_ok=True (previous version: False) author: Tin Dang
1 participant
The gap
Every CI job that executes tests does so under
--no-default-features --features runtime-tokio,…. Moon's default feature set isruntime-monoio, and that is what ships on Linux.src/files behind#[cfg(feature = "runtime-monoio")](incl. 3 test-only modules)The only mention of monoio anywhere in
ci.ymltoday is acargo clippyinvocation. CLAUDE.md's documented "Local CI Parity" command runs both suites — so the documented local gate has been strictly stronger than CI itself.This is not theoretical. The v0.8.6 inline-GET ACL bypass (#457) was wrong only on the monoio dispatch path and shipped green. #463 had to hand-verify one of its two enqueue sites locally, because CI structurally cannot see it.
The job
check-monoioon[self-hosted, moon-dev]— not a preference, it is the only Linux runner and therefore the only place monoio's io_uring driver executes at all. Default features, ownCARGO_TARGET_DIR, nocontinue-on-error,MOON_NO_URINGdeliberately unset.--profile ciis load-bearing, not incidental: the suite has a known load-sensitive flake class, a barecargo testhas no retries, and an intermittently-red required job gets disabled — which is worse than no job, because it still looks like coverage. The existing profile'sretries = 2absorbs it while still reporting FLAKY, so the signal survives.Measured on
moon-dev(kernel 6.17) before landing:80 seconds of test time. Cost was the objection I expected to have to argue against; it does not hold.
Verified by negative control, not by inspection
A CI-config change can be green and still be worthless, so the claim was tested directly. A deliberate defect was injected on
try_inline_dispatch— which iscfg(feature = "runtime-monoio"), so a tokio build cannot reach it — making inlineGETanswer$6\r\nBROKEN\r\n:multi_queues_inline_getAn incidental confirmation arrived first: the tokio leg compiled while monoio did not, because
try_inline_dispatchis cfg'd out entirely under tokio. The isolation is structural.Reverted immediately — zero residual markers in
src/, suite back to 6/6.Guarding the guard
tests/ci_covers_monoio.rs(5 tests) exists because the failure mode of CI coverage is silent: a job that stops running, or is switched to the wrong feature set, looks exactly like a green build. It fails on a wrong feature set,continue-on-error, a barecargo test, a shared target dir with the tokio job, or removal of tokio coverage.Red-first: 4 failed / 1 passed before the job existed, all four with "ci.yml has no
check-monoiojob" — red for the right reason. The fifth (the_tokio_job_is_still_covered) was written to be green before and after, so that "adding monoio" by converting the existing job shows up as a broken invariant rather than a silent trade of one blind spot for another.⚠ Needs a repo setting, not just this PR
Merging makes the job run, not block. It must be added to branch protection to gate merges. Until then it is advisory — and an advisory red job gets ignored, which would leave the blind spot open while looking closed. Flagged at contract-freeze and accepted there; the approver owns this step.
Scope
In: one new job in
ci.yml, the guard test, CHANGELOG.Out:
.config/nextest.toml(itsciprofile already covers this flake class — proven), fixing #459 or theblocking_peer_eofflake, anysrc/change, and aMOON_NO_URINGepoll leg (a later task if wanted).Task record:
.add/tasks/monoio-ci-coverage/TASK.md(gate PASS) · milestonev0-9-client-compatSummary by CodeRabbit
CI Improvements
Documentation
Task Tracking