Draft: dispatch immutable Benchmarks release images - #472
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
callumreid
left a comment
There was a problem hiding this comment.
Both release paths — runner-release.yml (Switchboard dispatch) and tag.yml (v* tag push) — now capture the exact manifest digest each docker/build-push-action emits, pass it between jobs via a short-lived artifact, and dispatch .../coval-bench-{runner,api}@sha256:... to benchmark-infra instead of a mutable :<sha> / :<version> tag. The correlation-matching loop in dispatch_infra loses its "newest recent run" fallback and now fails closed.
What I verified against the tree
- The correlation match is exact, not lucky. benchmark-infra's
bump-images.ymlonmainsetsrun-name: format('Image bump {0} {1}', sha||ref||run_id, switchboard_correlation_id||run_id), so a dispatched run'sdisplayTitlegenuinely ends with a space followed by the correlation id. The newendswith(" " + $correlation)predicate is therefore correct, and tighteningcontains→endswithalso closes the case where one correlation id is a substring of another. Dropping thegh run view --loggrep removes a 30-run log download per poll iteration — a real cost win, not just cleanup. - Removing the fallback is the point of the PR and is safe. The old branch adopted the newest
repository_dispatchrun after attempt 6, which — under benchmark-infra'sbump-imagesconcurrency group — could attachgh run watchto an unrelated rollout and then fast-forwardproduction-runneron its result. New behaviour is 30x5s of polling then a hardexit 1, sopromote_markernever runs (it gates onneeds.dispatch_infra.result == 'success'). Strictly fail-closed. - Digest plumbing holds. Both matrix legs are
runnerandapi; artifact names are per-image and per-workflow (benchmarks-image-digest-*vsbenchmarks-tag-image-digest-*), so a concurrent tag release cannot cross-contaminate;if-no-files-found: errorprevents a silently empty download; the^sha256:[0-9a-f]{64}$check is applied at both the producer and the consumer.resolve_images'permissions: contents: readis sufficient — same-rundownload-artifact@v4uses the Actions runtime token, notactions: read. - Skip path preserved. With
context.outputs.skip == 'true',build_and_pushis skipped, soresolve_imagesis skipped, sodispatch_infra'sneeds.resolve_images.result == 'success'is false. Same behaviour as the previousbuild_and_push.resultgate.summarizestill usesif: always()with the new job added toneeds. - Test file resolves correctly.
runner/tests/unit/test_release_workflow_contracts.pyusesparents[3], which fromrunner/tests/unit/<file>is the repo root. The negative assertions (fallback_run_id,newest repository_dispatch run,gh run view "$candidate_id"all absent) are the useful ones — they would fail if someone reintroduced the ambiguous-run fallback. These are text-matching contract tests rather than behavioural ones, which is the right tool here since GitHub Actions expressions aren't otherwise executable in CI. - Claimed validation run is real. Run
31285901223isworkflow_dispatch,conclusion: success, on head SHAe865d9b2exactly — and it is theSwitchboardworkflow, consistent with the body's claim that no release path was invoked.
Known gate (not a reason to withhold approval, but do not merge before it clears): benchmark-infra main currently validates the incoming payload with ^...coval-bench-runner:[A-Za-z0-9._-]{1,128}$ — a colon-tag-only regex that rejects an @sha256: reference outright. Merging this PR before coval-ai/benchmark-infra#91 lands would make every dispatch fail at "Validate payload". The PR body names this dependency and the PR is correctly still a draft, so this is ordering, not a defect. Land infra#91 first, then this.
Clean, well-scoped supply-chain hardening. CI is fully green on the head SHA (Analyze Python, Runner CI, CodeQL, Plan checks, Switchboard Gate); CodeRabbit is skipped only because the PR is a draft.
| DEPLOY_SHA: ${{ needs.context.outputs.deploy_sha }} | ||
| GH_TOKEN: ${{ secrets.INFRA_DISPATCH_TOKEN }} | ||
| RUNNER_IMAGE: ${{ needs.context.outputs.runner_image }} | ||
| RUNNER_IMAGE: ${{ needs.resolve_images.outputs.runner_image }} |
There was a problem hiding this comment.
P3 - context job's runner_image / api_image outputs are now dead, and its step summary still advertises them as the deployed images.
These two lines were the last consumers of needs.context.outputs.runner_image / .api_image. After this change nothing reads them, but the context job still computes both :<deploy_sha> tag refs, still exports them as job outputs, and still writes them to $GITHUB_STEP_SUMMARY as - Runner image: / - API image:. Someone doing release forensics from the run summary will read a mutable tag reference that is no longer what was dispatched to benchmark-infra — the summarize job reports only job results, not the resolved digests, so the tag refs are the only image strings a reader sees.
Existing checks miss this because dead workflow outputs are not a lint failure and test_release_workflow_contracts.py only asserts on the new digest wiring, never on what context still emits.
Smallest correction: drop runner_image / api_image from the context job's outputs: and from its step-summary block, and instead echo ${{ needs.resolve_images.outputs.runner_image }} / .api_image in the summarize job so the summary names the reference that was actually deployed. Non-blocking — nothing behaves incorrectly today.
Summary
Retain the exact runner and API digests emitted by each build and dispatch those immutable
@sha256:references to Benchmark Infra. Release/version tags remain metadata only.The caller discovers the infra run by the exact release correlation and fails closed if that run is absent; it cannot treat an unrelated recent success as this transaction.
Dependency
Consumer cutover and backwards-compatible tag-to-digest resolution are consolidated in coval-ai/benchmark-infra#91. The combined Infra draft safely accepts the current tag caller until this PR lands, then uses the already-retained digests directly.
Safety and validation