ci: trigger benchmarks after LLGo main updates - #2163
Conversation
There was a problem hiding this comment.
Review: ci: trigger benchmarks after LLGo main updates
The workflow is well-constructed: permissions: {} (least privilege), the if: github.repository == 'xgo-dev/llgo' fork guard, set -euo pipefail, an explicit missing-token check, and jq -cn --arg for injection-safe payload construction. One reliability finding is worth addressing before this is relied upon; the rest are optional hardening.
Reliability (see inline comment on the curl call)
- The
curlcall has no retry or timeout. A transient GitHub API blip (5xx / rate limit / network drop) makes--fail-with-bodyhard-fail the job and silently drops that benchmark dispatch — the exact revision never gets measured.
Design decision to make explicit (non-blocking)
- No
concurrencycontrol: several commits landing onmainin quick succession each fire a dispatch, causing redundant benchmark builds. Per the PR's stated intent (measure the exact LLGo version per commit), per-commit dispatch is likely intentional — but it's worth deciding deliberately. If only the latestmainmatters, add:concurrency: group: notify-benchmarks-${{ github.ref }} cancel-in-progress: true
Optional hardening (low priority, non-blocking)
- The token is set as a job-level
envvar. With a single step this is harmless, but scoping bothenventries to the step keeps the secret out of any step added later (defense in depth). - The token is passed to
curlvia--header "Authorization: Bearer ...", placing it in the process argv (visible via/proc/<pid>/cmdline). Negligible on GitHub-hosted ephemeral runners; matters only if this ever runs on self-hosted runners. - Minor doc nit: the PR body's "Required repository setup" documents
Contents: Read and write(correct forrepository_dispatch) but omits the implicitMetadata: Readthat fine-grained tokens require. GitHub usually auto-selects it, so this rarely bites.
| --arg source_run_url "https://github.com/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID}" \ | ||
| '{event_type: "llgo-main-updated", client_payload: {source_repository: $source_repository, llgo_repository: $llgo_repository, llgo_commit: $llgo_commit, source_run_url: $source_run_url}}')" | ||
|
|
||
| curl --fail-with-body --location --request POST \ |
There was a problem hiding this comment.
No retry or timeout on the dispatch call. A transient GitHub API failure (5xx, rate limit, or network drop) makes --fail-with-body hard-fail the job and silently drops this benchmark dispatch — the revision never gets measured. Consider adding resilience, e.g.:
curl --fail-with-body --location --request POST \
--retry 5 --retry-connrefused --retry-all-errors --retry-delay 2 \
--connect-timeout 10 --max-time 60 \
...
Pairing this with a timeout-minutes on the job is also a cheap guard against a hung call.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
xgo-dev/benchmarksafter every push that advancesxgo-dev/llgo:main;repository_dispatchpayload so the benchmark run pins that revision;xgo-dev/benchmarks, withBENCHMARKS_REPOSITORYavailable as a temporary migration override;Required repository setup
Add an Actions secret named
BENCHMARKS_DISPATCH_TOKENinxgo-dev/llgo. A fine-grained token should be scoped to the target benchmarks repository with repositoryContents: Read and writepermission, which is required for creating a repository dispatch event. The token is used only by this workflow and is never printed.The receiver workflow is already merged into
xgo-dev/benchmarks:main. After adding the secret, merge one small change tollgo:mainand confirm that arepository_dispatchrun appears in the benchmarks Actions history.Validation
xgo-dev/llgo:mainatd726187c;jq;git diff --checkpassed for the PR diff.