fix(ci): de-flake the ETXTBSY retry test and harden the Windows PATH join - #1377
Merged
Conversation
Closes #1373. I filed this blaming a dylint/soldr/compiletest interaction over `CARGO_BUILD_TARGET`. That was wrong. The bug is in the workflow, and it is mine — the `PATH` the step exported was corrupt on Windows. ## What actually happened The step did: export PATH="${CARGO_HOME}/bin:${PATH}" On Windows `CARGO_HOME` is a native path (`C:\Users\runneradmin\.cargo`) while `$PATH` inside Git Bash is POSIX and `:`-separated. Joining them yields C:\Users\runneradmin\.cargo/bin:/usr/bin:... whose drive-letter colon reads as a separator. The failing run's own log shows the result — a bare `C` entry followed by a bogus `D:\Users\runneradmin\.cargo\bin` — which I had noted as "looks mangled" and then discounted. With the cargo proxy unreachable, the compiletest driver could not resolve the lint library's dependencies, and every ui fixture failed with could not load library `...\ban_manual_slash_normalize@nightly-2026-04-16.dll`: LoadLibraryExW failed That is a *dependent*-DLL failure, not a missing file — the distinction the issue said to confirm before assuming a path problem. Confirming it is what found this. ## Fix Normalize through `cygpath -u` before prepending, giving `/c/Users/.../.cargo/bin`, which is safe to join with `:`. `cygpath` is absent on Linux and macOS, where the path is already POSIX, so the conversion is guarded. The sweep step was never affected: it invokes `"${CARGO_HOME}/bin/cargo-dylint"` as a single argument rather than through `PATH`. Noted there so the asymmetry does not look accidental. ## The step now runs on Windows #1359 scoped "Test Dylint libraries" to ubuntu because of this failure. That reasoning no longer holds, so the scoping is removed and the lint crates' own ui fixtures run on both legs — which matters more than it sounds: several of these lints are about path spelling, and their diagnostic rendering is exactly the kind of thing that differs per-OS. The four remaining ubuntu-only steps stay that way on the original grounds: they are Python validators and a rustfmt check, and learn nothing from a second OS. ## Verified The mangling mechanism is reproduced locally — the bad expression yields `C:\Users\runneradmin\.cargo/bin:/usr/bin`, matching the runner's log — and the corrected script produces `/c/Users/.../.cargo/bin`. Running the step's shell verbatim over all 27 lint crates on a Windows host: 27 pass, 0 failures. The runner is still the final oracle for a runner-specific interaction, which is why the step is enabled in the same change rather than after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe Dylint workflow now runs library tests on both matrix platforms. It converts Windows Cargo paths to POSIX format before updating ChangesCross-platform Dylint execution
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change fixes Windows PATH handling and restores the lint UI tests on Windows; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
The first attempt fixed a real PATH-mangling hazard but not this failure —
the Windows leg failed identically, which the PR's own Windows run showed.
The log makes the mechanism explicit. compiletest invokes the driver with
PATH=...target/dylint-tests\x86_64-pc-windows-msvc\debug;...\debug\deps;...
while the lint library it is told to load is at
target/dylint-tests\debug\ban_manual_slash_normalize@nightly-2026-04-16.dll
Two target-dir layouts coexist: soldr builds the test binary under
`<target>/<triple>/debug`, and each lint's `fn ui` clears
`CARGO_BUILD_TARGET` so the *library* it rebuilds lands in `<target>/debug` —
the only place Dylint 6.0.1 looks. Both directories exist, which is
reproducible locally.
So the library loads from one tree while its sibling rustc dylibs are searched
for in the other. `LoadLibraryExW failed` names the library and reads like a
missing file; it is a missing *dependency*. That is the distinction the issue
said to confirm before assuming a path problem, and it is what the first
attempt got wrong.
Adding the untargeted `debug` and `debug/deps` to PATH makes both trees
resolvable whichever one an artifact came from. Harmless on Linux and macOS,
where the loader uses rpath and the directories go unused.
Keeping the `cygpath -u` normalization from the first commit: joining a native
`CARGO_HOME` to a POSIX `$PATH` with `:` really does corrupt the variable —
the runner's own log shows a bare `C` entry — it just was not what broke this.
Refs #1373
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
First attempt was wrong, and this PR's own Windows leg said so — which is why I enabled the step in the same change rather than after it. The cygpath fix addresses a real hazard (the runner's log genuinely shows a mangled Two target-dir layouts coexist. soldr builds the test binary under So the library loads from one tree while its sibling rustc dylibs are searched for in the other. Fix: put the untargeted I can't prove this one locally — the ui tests already pass on my Windows box because the dependencies happen to resolve there, which is precisely why the bug is runner-specific. CI is the oracle again. |
Two things, and the second is a retreat. ## De-flake the retry test (the important one) `a_handle_released_mid_window_lets_the_retry_through`, which I added in #1368, failed on this PR's ubuntu leg. It is my test, in a required check, so it can redden anyone's PR — the exact class of problem #1366 was about, reintroduced by the fix for it. The test raced a task that releases a write handle after 10 ms against a second spawn attempt at 25 ms. I called that "a loaded runner can delay the release several times over and this still passes". A 2.5x margin was not the margin I thought it was. The backoff is now injectable, and the test uses 500 ms against the same 10 ms hold — fifty times later, not two and a half. Production still uses the 25 ms constant. Verified on Linux, where the test is not a no-op: 23 pass, 0.51 s. ## Stop guessing at #1373 The Windows ui-test step goes back to ubuntu-only. Two hypotheses tried, two CI rounds, same failure both times: - normalizing the PATH separator with `cygpath -u` — a real hazard, the runner's log genuinely shows a mangled `C` entry, but not this bug; - adding the untargeted `debug` and `debug/deps` to PATH — no change at all. What is established and now recorded on the issue: two target-dir layouts coexist (reproduced locally), the library is at `<target>/debug` while compiletest's PATH points at `<target>/<triple>/debug/deps`, and the failure is `LoadLibraryExW` on the library itself — a missing dependency, not a missing file. Whatever that dependency is, it is not in either directory I tried. The `cygpath` normalization stays: it fixes a genuine latent corruption on Windows even though it did not fix this. The scoping comment now says what was ruled out, so the next attempt does not repeat it. The Windows leg keeps doing the thing #1359 added it for — compiling Windows-gated workspace source so the lints can see it. Refs #1373 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
zackees
changed the title
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.
Does not close #1373 — retitled and rescoped after two failed attempts. What it does:
1. De-flakes a test I introduced (the important part)
a_handle_released_mid_window_lets_the_retry_through, added in #1368, failed on this PR's own ubuntu leg. It's my test, in a required check, so it can redden anyone's PR — the exact class of problem #1366 was about, reintroduced by the fix for it.The test raced a task releasing a write handle after 10 ms against a second spawn attempt at 25 ms. I wrote that "a loaded runner can delay the release several times over and this still passes". A 2.5x margin was not the margin I thought it was.
The backoff is now injectable and the test uses 500 ms against the same 10 ms hold — fifty times later. Production keeps the 25 ms constant. Verified on Linux (where the test isn't a no-op): 23 pass, 0.51 s.
2. Keeps the
cygpathhardeningJoining a native
CARGO_HOMEto a POSIX$PATHwith:really does corrupt the variable — the runner's log shows a bareCentry and a bogusD:\Users\runneradmin\.cargo\bin. Worth fixing on its own; it just wasn't what broke the ui tests.3. Reverts the Windows ui-test experiment, and records what was ruled out
Two hypotheses, two CI rounds, identical failure both times:
cygpath -u)debug+debug/depsto PATHEstablished (and now on #1373): two target-dir layouts coexist — reproduced locally, both directories exist — with the library at
<target>/debugwhile compiletest's PATH points at<target>/<triple>/debug/deps. The failure isLoadLibraryExWon the library itself, i.e. a missing dependency, not a missing file. Whatever that dependency is, it is not in either directory I tried.I'd rather leave the issue open with hard evidence than keep spending CI rounds on guesses. The scoping comment in the workflow now says what was ruled out so the next attempt doesn't repeat it.
The Windows leg keeps doing what #1359 added it for — compiling Windows-gated workspace source so the lints can see it.
🤖 Generated with Claude Code