Repository navigation
ci: retry transient compliance export failures - #14694
Conversation
Signed-off-by: xianlubird <xianlubird@gmail.com>
github-actions
Bot
added
external-contribution
|
/ok to test d5896ac |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe compliance extraction action now delegates Docker BuildKit execution to a retry wrapper. The wrapper isolates attempts, classifies failures, publishes successful output, and records logs. A Python test suite covers recovery, exhaustion, terminal failures, cleanup, and retry behavior. ChangesCompliance extraction
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The compliance export now retries recognized transient BuildKit failures while preserving successful-output publication and terminal failure behavior. No concrete merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files. (1 skipped: 1 unsupported.)
Comment |
![dynamo-review-agent[bot]](https://avatars.githubusercontent.com/u/201626793?s=60&v=4){kind=small}
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test 78a18d5 |
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test e8b93c4 |
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Review: bounded retry for the compliance export
I verified the retry predicate, the permanent-failure path, the exhausted-retry path, idempotence and the siblings. One P3, no blockers.
The predicate matches the real error, and three constructed permanent failures still fail on attempt 1
The failing line in the run the description points to is:
ERROR: failed to build: failed to solve: Unavailable: ghcr.io/astral-sh/uv:0.12.0: Unavailable: error reading from server: EOF
The transient pattern Unavailable:.*error reading from server: (unexpected )?EOF matches it, and the permanent pattern does not. So the wrapper retries the case it was written for.
The predicate is an allow list, not "any non-zero exit". An error that matches neither list exits at once. I ran three real docker buildx build failures through the wrapper on macOS with buildx v0.36.1:
| constructed failure | terminal line | attempts | exit | destination |
|---|---|---|---|---|
RUN ... exit 1 |
process "..." did not complete successfully |
1 | 1 | not created |
| image that does not exist | unexpected status from GET ...: 403 Forbidden |
1 | 1 | not created |
FRUM busybox |
dockerfile parse error on line 1 |
1 | 1 | not created |
No permanent failure ends in success. The destination directory is never created on failure, so no later step can read a half-written export.
Exhausted retries reach a red job, and pipefail does not swallow the status
The wrapper pipes the build into tee, which is the shape that can hide an exit status. It does not here. Bash pipefail returns the status of the last command that exited non-zero, so the build status wins over tee. Measured with a fake build that exits 42 and prints the transient line:
EXIT=42 wall=41s
::warning::Compliance extraction attempt 1/3 failed with a transient transport error; retrying in 10s
::warning::Compliance extraction attempt 2/3 failed with a transient transport error; retrying in 30s
The step runs under bash --noprofile --norc -e -o pipefail, and the action has no continue-on-error, so a non-zero wrapper status fails the job.
Bounds: 3 attempts at most, delays of 10s then 30s, 40s of added sleep in the worst case. The export itself took 73s on this pull request in CI and about 6 minutes in the run that failed, so a total failure costs roughly 4 to 19 minutes against a job budget of 60 to 120 minutes.
Idempotence, and no path to a misleading baseline diff
Each attempt writes to a fresh output-N directory under mktemp -d. Only a success runs rm -rf destination then mv, so the second attempt neither appends to nor merges with the first. I seeded the destination with stale files and ran a successful export: the destination held only the new file afterward.
The step no longer runs mkdir -p /tmp/compliance. That is safe. Compute OSRB diff runs under if: always() with nullglob, and I confirmed it exits 0 for both a missing and an empty /tmp/compliance. Upload compliance artifact then finds no files and warns, as before.
CI on the head commit proves the whole path: the wrapper ran a real multi-arch export in vllm-runtime / Build multi-arch cuda13.0, and the naming step listed /tmp/compliance/linux_amd64/... and /tmp/compliance/linux_arm64/.... Compliance audit passed. The --output flag now sits after the build context .; buildx accepts flags in that position, which I confirmed locally and which this CI run confirms.
The new tests do pin the behavior
All 8 tests pass, and the compliance-extract-selftest hook passed in the pre-commit job on the head commit. The hook runs under pre-commit/action, which uses --all-files, so the files: pattern does not gate it. I mutated the wrapper to check the tests are not decorative:
| mutation | result |
|---|---|
| never retry | 6 failures |
| retry on any error | 13 errors |
| drop the permanent-error list | 1 failure |
| keep a failed attempt's output | still passes |
The last mutation is not a defect. The destination is still populated only from a successful attempt, and the leftover is a temporary directory.
Unverified: I could not reproduce a live BuildKit transport failure, so the recovery path is proven with a fake build command, not a real flaky registry. The mv between $RUNNER_TEMP and /tmp is proven only on the runner filesystem this pull request already exercised.
Signed-off-by: xianlubird <xianlubird@gmail.com>
|
/ok to test 1a03cf1 |
dmitry-tokarev-nv
left a comment
dmitry-tokarev-nv
left a comment
There was a problem hiding this comment.
Re-approving at 1a03cf1f55. The earlier approval was for e8b93c48c5, so it is stale. The push added one commit on top of that tree and the merge base did not move.
The new commit wraps the /sources/ export in the same retry script, which closes my only open finding. Zero P0, zero P1, zero P2, zero P3 open.
What I re-verified at this head, by execution
Push shape by ancestry: git merge-base --is-ancestor e8b93c48c5 HEAD returns true, so this is an append and not a force-push. The merge base against main is 58f1e01f76 for both the old head and the new head.
Retry bound: 3 attempts, delays of 10s then 30s, so at most 40s of added sleep per wrapped step. The Compliance extract step at this head took 9s in a job with a 60-minute limit.
Failure classification: I ran the real wrapper against 13 different final error lines with a fake build command. The 4 transport patterns retried to the cap. Compile failures, 401, 403, denied:, manifest unknown, an unrecognized error, and an error with no ERROR: line all failed on the first attempt with no sleep.
Artifact: a retried success published a tree that diff -r reports IDENTICAL to a first-attempt success, and the partial file from the failed attempt did not reach the destination. A control that added one file reported DIFFERS.
Visibility after the last attempt: with a build that exits 17, the wrapper exited 17. The original error text appeared 3 times in the step output and was the last line printed.
Argument order: the wrapper appends --output after the build context. A real buildx 0.37.1 export with the flag after the context produced a result identical to the old form. Controls confirmed buildx parses flags in that position and rejects a second context argument.
Expression check: the Compliance extract step reports success at this head, so ${{ github.action_path }} resolves to a real path and the wrapper runs. The /sources/ step uses the same expression in the same file.
Not verified: the /sources/ step is gated on archive_sources == 'true', and every caller passes false today, so no pipeline exercises that path yet. I also did not exercise a live registry failure.
This branch was successfully deployed
4 participants
Overview
Summary
The Planner image build in #14377 succeeded, but the subsequent compliance export failed while BuildKit resolved
ghcr.io/astral-sh/uv:0.12.0withUnavailable: error reading from server: EOF. That made the build job fail and skipped its downstream tests. Failure logs.Details
Add bounded retries to the shared
/compliance/export: at most three attempts, with 10s and 30s delays, for narrowly matched transient transport errors in the final BuildKit error. Compilation failures, explicit authorization denials, missing manifests, and unrecognized errors fail immediately; exhausted retries retain a failing exit status.Each attempt exports to a fresh temporary directory. Only a successful export replaces the destination, and every attempt's output remains in the job log. This is an independent CI fix based on main; it does not depend on #14377 or change its Planner code, job dependencies, or compliance requirements.
Where should the reviewer start?
Start with
.github/actions/compliance-extract/extract-with-retry.shfor error classification and output isolation, then.pre-commit-config.yamlfor the regression-test hook. The existing Pre Merge pre-commit job runs this hook automatically; the tests use only Python's standard library and Bash.Related Issues
Validation
git diff --checkpassed.Run the regression suite with
python3 .github/actions/compliance-extract/test_extract_with_retry.py.Summary by CodeRabbit
Bug Fixes
Tests
The
compliance-extract-selftestpre-commit hook passes all 8 tests. Temporarily disabling retries makes the same hook fail; restoring the wrapper makes it pass again. Verified selection for the action, wrapper, tests, and hook configuration, with unrelated files skipped.