fix(ci): restore green main — dylint allowlist, spawn markers, macOS unused_mut, rustdoc links - #1216
Merged
Merged
Conversation
… unused_mut, rustdoc links main is red on six checks. They are unrelated to each other and accumulated because the workflows that run them are `pull_request`-triggered, so nothing re-ran them on main after the merges that broke them (#1197, #1206, #1207). Found while verifying an unrelated dependency bump (#1215); every failure there reproduced identically on main. Dylint: drop `crates/fbuild-cli/src/cli/clangd_config.rs` from ban_unrooted_tempdir's allowlist -- the file was deleted in #1197. Lint subprocess spawns: annotate three deliberate direct spawns with `allow-direct-spawn:` markers (clangd test driver, interactive gdb that must inherit terminal stdio, detached editor launch that must outlive the CLI). The scanner only inspects the hit line and the one line directly above it, so the marker must be a single line -- a wrapped comment silently fails to register. This also fixes Check (ubuntu-latest), which runs the same script. Check (macos-latest): `ports.rs` bound `let mut ports` unconditionally while only `cfg(target_os = "linux")` mutates it, so every other unix target tripped `-D unused-mut`. The `mut` now exists only inside the Linux cfg via a shadowing block, rather than an `#[allow]`. Documentation: six rustdoc errors across three crates. URL templates such as `?query=<optional filter>` parsed as unclosed HTML tags; several intra-doc links pointed at `cfg(target_os = "linux")` items that do not exist to resolve against when docs build on a non-Linux host; three more pointed at private items. Unresolvable links become code spans, each with a short note on why it is deliberately not a link. CH32V006 (`build / build`) is also red on main but is a vendor-C compile error in the openwch Arduino core, not CI hygiene, and is left for a targeted fix. Verified locally on Windows, all exit 0: check_dylint_allowlists.py, find_direct_subprocess.py --fail, cargo doc with RUSTDOCFLAGS=-D warnings, clippy -D warnings, cargo fmt --check, and `bash test` (77 suites, 0 failed). The macOS fix could not be verified locally: cross-compiling fbuild-serial to x86_64-apple-darwin fails on ring's build script needing a macOS `cc`. The cfg-shadowing pattern was verified in an isolated cargo project (clean under -D warnings with the cfg both on and off); Check (macos-latest) on this PR is the real proof. Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
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 |
zackees
added a commit
that referenced
this pull request
Aug 1, 2026
Neither test was a regression; both surfaced once #1216/#1221 stopped the macOS job aborting before it reached the test step. Both assert exact outcomes over real OS resources under timing pressure. ## spawn_lock_is_exclusive_within_process `try_acquire_spawn_lock_at` used `.ok().flatten()`, collapsing a hard I/O error into the same `None` that means "another process holds the lock" — so when the third acquire failed, there was no way to tell which had happened. - Split into `try_acquire_spawn_lock_result_at` (propagates) and the existing swallowing wrapper (logs errno + kind, then returns `None`). The "a broken filesystem must never gate progress" contract for the production caller is unchanged; the failure is just no longer silent. - The test now uses the error-preserving variant, so a hard error reports the actual errno instead of a bare `assertion failed`. - Retry `EINTR` in `file_lock::try_acquire`, around both the `open` and the `flock`. `flock()` is interruptible by signals on macOS/BSD and fs2 returns the raw OS error, which our classifier doesn't recognize as contention — so an EINTR became a hard error. Correct regardless of whether it was the cause here. ## streaming_download_retries_chunk_stalls_five_times_without_output This was the only test combining `#[tokio::test(start_paused = true)]` with a real `TcpListener`, and it was the one that flaked. Paused time auto-advances whenever the runtime looks idle, but socket readiness comes from the OS reactor, not the virtual clock — so with the client parked on `chunk()` and the server on `sleep`, the clock could jump past a connection that was about to reach `accept()`, leaving `request_count` short of 5. Took the issue's preferred fix rather than the stopgap: extract the retry durations into a `RetryTiming` struct so the test drives the *same* retry logic on millisecond durations in real time. No `start_paused`, no reactor race, and the assertion stays at exactly 5 attempts instead of being weakened to `>= 1`. Runs in ~1s. `production_retry_timing_matches_the_constants` pins the seam to the real constants so the injection can't drift into a behavior change. Verified: the chunk-stall test passes 5/5 consecutive runs locally; clippy clean on all four crates. Co-Authored-By: Claude <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Aug 1, 2026
Neither test was a regression; both surfaced once #1216/#1221 stopped the macOS job aborting before it reached the test step. Both assert exact outcomes over real OS resources under timing pressure. ## spawn_lock_is_exclusive_within_process `try_acquire_spawn_lock_at` used `.ok().flatten()`, collapsing a hard I/O error into the same `None` that means "another process holds the lock" — so when the third acquire failed, there was no way to tell which had happened. - Split into `try_acquire_spawn_lock_result_at` (propagates) and the existing swallowing wrapper (logs errno + kind, then returns `None`). The "a broken filesystem must never gate progress" contract for the production caller is unchanged; the failure is just no longer silent. - The test now uses the error-preserving variant, so a hard error reports the actual errno instead of a bare `assertion failed`. - Retry `EINTR` in `file_lock::try_acquire`, around both the `open` and the `flock`. `flock()` is interruptible by signals on macOS/BSD and fs2 returns the raw OS error, which our classifier doesn't recognize as contention — so an EINTR became a hard error. Correct regardless of whether it was the cause here. ## streaming_download_retries_chunk_stalls_five_times_without_output This was the only test combining `#[tokio::test(start_paused = true)]` with a real `TcpListener`, and it was the one that flaked. Paused time auto-advances whenever the runtime looks idle, but socket readiness comes from the OS reactor, not the virtual clock — so with the client parked on `chunk()` and the server on `sleep`, the clock could jump past a connection that was about to reach `accept()`, leaving `request_count` short of 5. Took the issue's preferred fix rather than the stopgap: extract the retry durations into a `RetryTiming` struct so the test drives the *same* retry logic on millisecond durations in real time. No `start_paused`, no reactor race, and the assertion stays at exactly 5 attempts instead of being weakened to `>= 1`. Runs in ~1s. `production_retry_timing_matches_the_constants` pins the seam to the real constants so the injection can't drift into a behavior change. Verified: the chunk-stall test passes 5/5 consecutive runs locally; clippy clean on all four crates. Co-Authored-By: Claude <noreply@anthropic.com>
zackees
added a commit
that referenced
this pull request
Aug 1, 2026
Neither test was a regression; both surfaced once #1216/#1221 stopped the macOS job aborting before it reached the test step. Both assert exact outcomes over real OS resources under timing pressure. ## spawn_lock_is_exclusive_within_process `try_acquire_spawn_lock_at` used `.ok().flatten()`, collapsing a hard I/O error into the same `None` that means "another process holds the lock" — so when the third acquire failed, there was no way to tell which had happened. - Split into `try_acquire_spawn_lock_result_at` (propagates) and the existing swallowing wrapper (logs errno + kind, then returns `None`). The "a broken filesystem must never gate progress" contract for the production caller is unchanged; the failure is just no longer silent. - The test now uses the error-preserving variant, so a hard error reports the actual errno instead of a bare `assertion failed`. - Retry `EINTR` in `file_lock::try_acquire`, around both the `open` and the `flock`. `flock()` is interruptible by signals on macOS/BSD and fs2 returns the raw OS error, which our classifier doesn't recognize as contention — so an EINTR became a hard error. Correct regardless of whether it was the cause here. ## streaming_download_retries_chunk_stalls_five_times_without_output This was the only test combining `#[tokio::test(start_paused = true)]` with a real `TcpListener`, and it was the one that flaked. Paused time auto-advances whenever the runtime looks idle, but socket readiness comes from the OS reactor, not the virtual clock — so with the client parked on `chunk()` and the server on `sleep`, the clock could jump past a connection that was about to reach `accept()`, leaving `request_count` short of 5. Took the issue's preferred fix rather than the stopgap: extract the retry durations into a `RetryTiming` struct so the test drives the *same* retry logic on millisecond durations in real time. No `start_paused`, no reactor race, and the assertion stays at exactly 5 attempts instead of being weakened to `>= 1`. Runs in ~1s. `production_retry_timing_matches_the_constants` pins the seam to the real constants so the injection can't drift into a behavior change. Verified: the chunk-stall test passes 5/5 consecutive runs locally; clippy clean on all four crates. Co-Authored-By: Claude <noreply@anthropic.com>
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
1 participant
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.
Why
mainis red on six checks. None of them relate to each other; they accumulated because the workflows that run them arepull_request-triggered, so nothing re-ran them onmainafter the merges that broke them (#1197 ide/clangd, #1206 debug, #1207 serial sysfs enrichment).Found while verifying an unrelated dependency bump (#1215) — every failure there reproduced identically on
main.What this fixes
1. Dylint — stale allowlist path
dylints/ban_unrooted_tempdir/src/allowlist.txt:53still listedcrates/fbuild-cli/src/cli/clangd_config.rs, deleted in #1197. Entry removed.2. Lint subprocess spawns — 3 unannotated
Command::newAnnotated with
allow-direct-spawn:markers; each is a deliberate direct spawn:clangd_check_parity.rs:242— integration test driver invoking the clangd binary.debug.rs:539— interactive gdb must inherit the user's terminal stdio; the capturing helpers would break the session.ide.rs:311— editor launched detached, must outlive the CLI.Note for future annotators: the scanner only checks the hit line and the one line immediately above, so the marker has to be a single line. A wrapped multi-line comment silently fails to register.
This also fixes Check (ubuntu-latest), which runs the same script as a step.
3. Check (macos-latest) —
unused_mutports.rsboundlet mut portsunconditionally, but onlycfg(target_os = "linux")mutates it (sysfs health enrichment), so every other unix target tripped-D unused-mut. Themutnow exists only inside the Linux cfg, via a shadowing block — no#[allow].4. Documentation — 6 rustdoc errors across 3 crates
fbuild-daemonboards.rs/libraries.rs: URL templates like?query=<optional filter>parsed as unclosed HTML tags. Wrapped in backticks.fbuild-serialports.rs/sysfs_usb.rs: intra-doc links tocfg(target_os = "linux")items, which do not exist to resolve against when docs build on a non-Linux host, plus one link to a private item.fbuild-build-engineclang.rs/source_scanner.rs: three links to private items.Unresolvable links became code spans, each with a short note saying why it is deliberately not a link, so nobody "helpfully" converts them back.
Not fixed here
build / build(CH32V006) is also red onmain, but it is a vendor-C compile error (implicit declaration of function 'SetSysClockTo_48MHZ_HSI') in the openwch Arduino core, not CI hygiene. Out of scope — left for a targeted board fix.Verification
Locally on Windows, all exit 0:
uv run python ci/check_dylint_allowlists.pyuv run python ci/find_direct_subprocess.py --failRUSTDOCFLAGS="-D warnings" soldr cargo doc --workspace --no-depssoldr cargo clippy --workspace --all-targets -- -D warningssoldr cargo fmt --all -- --checkbash testCaveat on the macOS fix: it could not be verified locally — cross-compiling
fbuild-serialtox86_64-apple-darwinfails onring's build script needing a macOScc. The cfg-shadowing pattern was verified in an isolated cargo project (clean under-D warningswith the cfg both on and off), butCheck (macos-latest)on this PR is the real proof.