ci(windows): make the ffmpeg retry test the outcome, not choco's exit code - #1290
Merged
Conversation
… code The chocolatey feed 503'd; choco printed "Unable to find package 'ffmpeg'" and "installed 0/0 packages" — then exited 0. The retry loop added on 2026-07-20 for this exact class was `choco install ... && break`, so it broke out on attempt 1, no backoff ran, and the job died one line later on `ffmpeg: command not found`. It took #1281 red on an unrelated change. A retry that trusts a lying exit code is not a retry. The loop now exits on `command -v ffmpeg` and still fails the job loudly when ffmpeg never arrives. Tests extract the real step body from ci.yml and run it against a stubbed choco; 2 of the 4 fail against the previous loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesWindows ffmpeg retry
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 310-314: Update the ffmpeg installation retry loop so the retry
message and backoff sleep execute only when i is less than 3; after the final
failed attempt, exit the loop without logging a retry or sleeping.
In `@tests/test_ci_windows_ffmpeg_retry.py`:
- Around line 96-103: Update test_retries_when_choco_lies_about_success to
assert attempts == 2 instead of allowing any value greater than or equal to two,
ensuring the retry loop stops immediately once ffmpeg is installed on the second
attempt while preserving the successful process return-code assertion.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0dda601-99cd-49e5-a3de-8815f19e0849
📒 Files selected for processing (2)
.github/workflows/ci.ymltests/test_ci_windows_ffmpeg_retry.py
Contributor
|
| Filename | Overview |
|---|---|
| .github/workflows/ci.yml | Updates the Windows ffmpeg retry condition and introduces per-platform smoke-job timeouts without an eligible merge-blocking issue. |
| tests/test_ci_windows_ffmpeg_retry.py | Adds an isolated POSIX test harness that executes the workflow step and verifies retry and failure behavior. |
Reviews (4): Last reviewed commit: "ci(windows): skip the backoff after the ..." | Re-trigger Greptile
The harness inherited the ambient PATH, so a real ffmpeg satisfied `command -v` and the loop exited on attempt 1 — every assertion passed against a broken workflow. It happened twice: /opt/homebrew/bin locally, then /usr/bin on the Linux runner, which is what took this PR red. PATH is now the stub dir alone, with the few real tools the stubs need symlinked in, and stub shebangs are absolute (`/usr/bin/env bash` cannot resolve bash when PATH is one directory). test_harness_actually_hides_ffmpeg asserts the sandbox is a sandbox, so the next leak fails loudly instead of quietly passing. 2 of 5 fail against the old `&& break` loop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Smoke (Windows) has been dying at 10m08s inside `uv sync`, and the shared 10-minute budget made it self-perpetuating: the leg is killed before the post-step saves the uv cache, so the next run starts cold and dies the same way. Nothing primes the cache, so it never gets faster. Measured on run 30385710466 — Linux 65s, macOS 65s, Windows still installing torch when the job was killed. Windows now gets 25 minutes, priced for one cold install to finish and populate the cache; warm runs land nowhere near it. Per-leg rather than raising the shared value, so a genuine hang on Linux or macOS still fails fast instead of inheriting Windows' allowance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CodeRabbit, both valid: - The loop announced "retrying in 90s" and slept after attempt 3, though no fourth attempt exists — 90s added to an already-doomed job. - The retry tests asserted `attempts >= N`, so a regression that kept going after ffmpeg appeared would still pass. Pinned to exact counts, plus a case asserting the final attempt announces no retry. Co-Authored-By: Claude Opus 5 <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.
This is what just took #1281 red on an unrelated change.
What happened
The chocolatey community feed returned 503.
choco install ffmpegprinted:…and exited 0.
The retry loop added on 2026-07-20 for exactly this class was written as
choco install ... && break. It broke out on the first attempt, no backoff ran, and the job died one line later onffmpeg: command not found. The log proves it: nochoco attempt 1 failedline, no 30s gap.A retry that trusts a lying exit code is not a retry.
Fix
Test the outcome —
command -v ffmpeg— instead of$?. Same 3 attempts, same backoff, but the loop exits only when ffmpeg is genuinely on PATH, and still fails the job loudly when it never arrives (a silent pass would push a broken toolchain into the test run).Tests
tests/test_ci_windows_ffmpeg_retry.pyextracts the real step body fromci.yml(so it cannot drift) and runs it against a stubbedchoco:2 of the 4 fail against the previous loop.
One harness note worth keeping: the test pins a minimal
PATH. Inheriting the developer's let a real/opt/homebrew/bin/ffmpegsatisfycommand -vand silently neuter every assertion — which it did, on my first run.Updated the Windows CI dependency-install smoke job to use a per-OS timeout budget (Windows 25 minutes vs 10 for others) and changed the Windows
ffmpeginstall retry loop to retry based oncommand -v ffmpeg(withhash -rand up to three attempts/backoff) instead of Chocolatey’s exit code, which could falsely report success. Added regression tests that extract and execute the exactSystem deps (Windows)step fromci.ymlwith stubbedchoco,sleep, and a pinnedPATHso they exercise false-success, non-zero failure, persistent failure, and first-attempt success—including verifying there’s no backoff announcement after the final attempt. Human review is most important around thehash -r+command -v ffmpegcontrol flow and the test harness’s PATH isolation to ensure a real systemffmpegcan’t mask failures.