fix(arithmetization): install GNU time and clear errexit in the weekly zkc metrics jobs - #3721
Open
OlivierBBB wants to merge 4 commits into
Open
fix(arithmetization): install GNU time and clear errexit in the weekly zkc metrics jobs#3721OlivierBBB wants to merge 4 commits into
OlivierBBB wants to merge 4 commits into
Conversation
…disable errexit Run 30959512082 failed both measurement jobs with exit 127 while prepare succeeded entirely. Two bugs, the second worse than the first. GNU time is not on the runner image. setup-arithmetization-riscv apt-installs it (line 53, alongside binutils-riscv64-unknown-elf), and the measurement jobs deliberately skip that heavy action to avoid pulling in Zig and Rust they do not need. Every measurement wraps its command in `/usr/bin/time -v` to capture peak RSS, so with the binary absent the step died before running anything — hence 127, and hence prepare being the only job that worked. The composite now installs it, guarded so it is a no-op when already present. The 127 then exposed the real problem. GitHub's default shell is `bash -e`, and `set -uo pipefail` does not clear errexit — verified locally: under `bash -e`, a block using that form exits at the failing command and writes no rc, no wall_s and no timedout marker, whereas the same block with `set +e` records rc=124, the wall clock and the marker. So every rc capture in this workflow was dead code on failure, which means a timed-out trace or check recorded nothing at all. That is the outcome the whole design exists to capture, and it could never have worked. The merged version has the same defect. Signed-off-by: Olivier Bégassat <olivier.begassat@consensys.net> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
…path did not run Run 30960567540 was green end to end — prepare, stats and report all succeeded and the row rendered with full provenance — but it carried a red annotation: "Artifact not found for name: zkc-metrics-guest". With run-heavy off, prepare never builds the guest, so that artifact does not exist. continue-on-error meant the report still succeeded, but an error annotation on a green run is exactly the kind of noise that makes people stop trusting the output. The download is now gated the same way the trace and trace-check downloads are. Signed-off-by: Olivier Bégassat <olivier.begassat@consensys.net> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olivier Bégassat <olivier.begassat.cours@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes reliability issues in the weekly arithmetization “zkc metrics” GitHub Actions workflow by ensuring /usr/bin/time -v is available in the measurement jobs and by disabling errexit so timeout/failure exit codes can be captured and recorded instead of aborting early.
Changes:
- Install GNU
timein the lightweight measurement setup action so measurement steps don’t fail with exit 127. - Update measurement
run:blocks to explicitly clearerrexit(set +e -uo pipefail) so rc/wall/timedout capture runs even on failure/timeout. - Gate “Download guest build measurement” to only run when the heavy path actually produced that artifact.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/arithmetization-weekly-zkc-metrics.yml |
Adjusts shell behavior to correctly capture failure/timeout outcomes and tightens artifact download conditions. |
.github/actions/setup-zkc-measurement/action.yml |
Ensures GNU time is installed for measurement jobs that use /usr/bin/time -v. |
Suppressed comments (1)
.github/workflows/arithmetization-weekly-zkc-metrics.yml:253
- Even with
set +ecapturing rc/wall_s, a failingzkc compile --statsstep exits non-zero and will skip the subsequent upload step. That means the report job will see the compile measurement as “skipped” rather than being able to render the recorded rc/wall_s for a failure.
# GitHub's default shell is `bash -e`; `set -uo pipefail` does NOT clear errexit, so
# without `set +e` the rc/wall_s/timedout capture below never runs on a failure or
# timeout — which is precisely the outcome this workflow exists to record.
set +e -uo pipefail
Comment on lines
+139
to
143
| # GitHub's default shell is `bash -e`; `set -uo pipefail` does NOT clear errexit, so | ||
| # without `set +e` the rc/wall_s/timedout capture below never runs on a failure or | ||
| # timeout — which is precisely the outcome this workflow exists to record. | ||
| set +e -uo pipefail | ||
| d="$RUN/guest"; mkdir -p "$d" |
Run 30991033984 produced every measurement and rendered the table, then failed the report job on the push: "Permission to LFDT-Lineth/lineth-monorepo.git denied to github-actions[bot]", 403 on all three attempts. The identity is the tell. The step generates the App token correctly — APP_SLUG resolved to lineth-release-bot, so the token step ran and produced its outputs — yet the push authenticated as github-actions[bot]. actions/checkout ran first and, with persist-credentials left at its default, wrote the default token into .git/config as an http.https://github.com/.extraheader. That header takes precedence over credentials embedded in a remote URL, so the `git remote set-url` with the App token was dead code. The fallback identity has only `contents: read` from the workflow's permissions block, hence the 403. component-changelog-commit.yml, which this job's comment claims to mirror, gets the order right: it generates the token before the checkout and passes it via `token:`. This job had inverted that and tried to compensate afterwards. Generate the token first, hand it to the checkout, and drop the URL rewrite. The `|| github.token` fallback covers a workflow_dispatch with commit-results off, where the token step is skipped and its output is the empty string. The push step keeps GITHUB_TOKEN in its env: `gh api` still needs it to resolve the bot's user id for the commit e-mail. Signed-off-by: Olivier Bégassat <olivier.begassat@consensys.net> Co-Authored-By: Claude Opus 5 (1M context) <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
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.
Follow-up to #3714. Run 30973980469 failed all three measurement jobs with exit 127 while
preparesucceeded. These two commits were written after #3714 was squash-merged, so they never reachedmain.GNU time is not on the runner image
Every measurement wraps its command in
/usr/bin/time -vto capture peak RSS.setup-arithmetization-riscvapt-installstime(its line 53), but the measurement jobs deliberately skip that heavy action to avoid pulling in Zig and the riscv toolchain they do not need. With the binary absent each step died before running anything: exit 127, andpreparethe only job that worked.setup-zkc-measurementnow installs it, guarded so it is a no-op when already present.set -uo pipefaildoes not clear errexitThe 127 exposed the worse bug. GitHub's default shell is
bash -e, andset -uo pipefailleaves errexit on. Verified locally: underbash -e, a block using that form exits at the failing command and writes norc, nowall_sand notimedoutmarker, whereas the same block withset +erecordsrc=124, the wall clock and the marker.So every rc capture in this workflow was dead code on failure, meaning a timed-out trace or check recorded nothing at all. That is the outcome the whole design exists to capture, and it could never have worked. This is the silent failure mode: it shows up as missing table rows, not as a red X.
Also
Download guest build measurementnow only runs when the heavy path ran, matching the condition on the step that produces the artifact. A download for a non-existent artifact left a red annotation on an otherwise green run.🤖 Generated with Claude Code