ci: cache the pinned OPA binary and retry its download - #967
Merged
Conversation
Contributor
|
SandyChapman
marked this pull request as ready for review
July 29, 2026 16:26
`Build OPA policy WASM` is a required job, and it has been failing on an unretried download from openpolicyagent.org — twice in three attempts on a single PR, each time taking `Python e2e tests` (which needs its artifact) down with it. `script/build_policy_wasm.sh` already prefers a cached binary at `.cache/opa/<version>/` before downloading, but nothing in CI ever populated that directory, so every run depended on the network. The composite action now restores and saves it, keyed on the pinned version, so the fetch is only needed the first time a version is seen. The version is parsed out of the script rather than duplicated, so the key cannot drift from the pin; parsing (not sourcing) because executing the script would build the WASM a second time, and before the cache is restored. Both `curl` calls also gain `--retry 3 --retry-delay 2 --retry-all-errors`. `--retry-all-errors` matters here: the observed failures were connection-level, which plain `--retry` does not cover. Verified locally: cold cache downloads and populates `.cache/opa/<version>/`; a second run builds with no download at all. The retry flags were checked against an unreachable endpoint (4 attempts, backoff between). Regenerating policy.wasm leaves the tree clean, so the output stays reproducible. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
SandyChapman
force-pushed
the
ci-opa-download-resilience/schapman
branch
from
July 29, 2026 16:27
887edbc to
ab4ca1d
Compare
Contributor
|
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 (1)
📝 WalkthroughWalkthroughThe build action resolves and caches the pinned OPA binary by runner and version, while the build script adds retries and timeouts to OPA binary and checksum downloads. ChangesOPA WASM build dependency handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 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 `@script/build_policy_wasm.sh`:
- Around line 115-119: Add --connect-timeout and --max-time options to both curl
invocations in the OPA download logic, including the call guarded by the shown
failure check. Keep the existing retry, output, and error-handling behavior
unchanged while ensuring connection and total transfer durations are bounded.
🪄 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: Enterprise
Run ID: 2ee500cd-ccb7-47e0-87f9-44046f38fa89
📒 Files selected for processing (2)
.github/actions/build-policy-wasm/action.yamlscript/build_policy_wasm.sh
mckornfield
approved these changes
Jul 29, 2026
The retries added alongside the cache were only half a fix: with no --max-time, a stalled connection consumes the entire job and the retries never get a turn. The failure that motivated this work burned 5m40s before giving up, which is consistent with exactly that. Both curl calls now set --connect-timeout 15 with --max-time 120 for the binary (~25x headroom for ~23 MB on a CI runner) and --max-time 60 for the much smaller checksum. Retry, output and error handling are unchanged. Validated: cold cache still downloads and populates .cache/opa/<version>/; warm cache still skips the network entirely; and against a server that accepts then never responds, each attempt is now cut short and retried (curl exit 28) instead of hanging. Signed-off-by: Sandy Chapman <schapman@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
script/build_policy_wasm.sh (1)
117-124: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse exponential retry backoff.
--retry-delay 2forces a fixed two-second delay and disables curl’s default exponential backoff. Remove it or replace it with an explicitly validated backoff strategy to match the PR objective. (curl.se)Proposed fix
- if ! curl -fsSL --retry 3 --retry-delay 2 --retry-all-errors \ + if ! curl -fsSL --retry 3 --retry-all-errors \Apply the same change to the checksum download.
🤖 Prompt for 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. In `@script/build_policy_wasm.sh` around lines 117 - 124, Update both curl invocations in the OPA binary and checksum download flow to remove the fixed --retry-delay 2 option, allowing curl’s default exponential retry backoff to apply while preserving the existing retry counts and timeout settings.
🤖 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 `@script/build_policy_wasm.sh`:
- Around line 115-124: Update both curl download commands in the OPA binary and
checksum retrieval flow to cap the aggregate retry duration with
--retry-max-time, ensuring it fits within the surrounding 120-second subprocess
timeout. Keep the existing per-attempt --max-time values and retry behavior
unchanged.
---
Nitpick comments:
In `@script/build_policy_wasm.sh`:
- Around line 117-124: Update both curl invocations in the OPA binary and
checksum download flow to remove the fixed --retry-delay 2 option, allowing
curl’s default exponential retry backoff to apply while preserving the existing
retry counts and timeout settings.
🪄 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: Enterprise
Run ID: df467e26-a56d-49e9-8d81-264770f22b6a
📒 Files selected for processing (1)
script/build_policy_wasm.sh
anastasia-nesterenko
approved these changes
Jul 29, 2026
--max-time bounds one transfer and resets on every retry, so adding it alongside --retry 3 left the aggregate unbounded: worst case 4x120s + delays = 486s for the binary and 246s for the checksum, 732s combined. That collides with callers that impose their own ceiling. embedded_pdp/policy_wasm.py runs this script under DEFAULT_BUILD_TIMEOUT_SECONDS=120, so the subprocess would be killed mid-retry — the retries could never even finish, which defeats the point of adding them. Both calls now also set --retry-max-time, and the per-attempt budgets are sized to fit: binary <=45s aggregate + <=30s final attempt, checksum <=15s + <=10s, so worst case is ~100s and the `opa build` still fits inside 120s. The coupling to that caller is spelled out in a comment so the numbers are not re-tuned in isolation. Validated: cold download still succeeds (7s) and populates the cache; warm cache still skips the network; and against a server that accepts then never responds, the aggregate is cut at 62s instead of the ~126s the previous flags would have allowed. Signed-off-by: Sandy Chapman <schapman@nvidia.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
4 participants
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.
Summary
Build OPA policy WASMis a required job, and it has been failing on an unretried download fromopenpolicyagent.org— twice in three attempts on a single PR (#844), each time also taking downPython e2e tests, which needs its artifact:Two independent gaps, both one-liners.
1. The cache existed but CI never used it
script/build_policy_wasm.shalready prefers a cached binary at.cache/opa/<version>/before downloading — but nothing in CI ever populated that directory, so every run depended on the network. The composite action now restores and saves it, keyed on the pinned version, so the download is only needed the first time a given version is seen.The version is parsed out of the script rather than duplicated, so the cache key cannot drift from the pin. It parses rather than sources, because executing the script would build the WASM a second time — and before the cache is restored.
2. The download had no retry
Both
curlcalls now use--retry 3 --retry-delay 2 --retry-all-errors.--retry-all-errorsis the important part: the observed failures were connection-level, which plain--retrydoes not cover.Verification
.cache/opa/v1.8.0/opa_darwin_arm64_staticpolicy.wasmwith no download at allpolicy.wasmleaves the tree clean, so output stays reproducibleScope
Deliberately standalone rather than folded into #844, where I hit this — it is CI infrastructure unrelated to that change, and it benefits every PR.
Summary by CodeRabbit
Bug Fixes
Chores