fix(arithmetization): fix weekly metrics job - #3722
Open
OlivierBBB wants to merge 7 commits into
Open
Conversation
…rate jobs The workflow failed on both of its first two runs, in two different ways, and both have the same root cause: everything happened in one job and nothing was persisted until the end. Run 30951692211: the timeout-budget guard failed, which skipped `zkc compile --stats` — the one step without `if: always()`. The guard compared the step caps against a job cap that was never exposed as an input, so raising check-timeout-min past ~290 made it unsatisfiable from the dispatch form. Cancelling then did not work either: `if: always()` on the heavy steps opts out of cancellation, which GitHub's own documentation warns about. Run 30952623752: steps 1-12 all succeeded — stats ok, guest built, trace ok in 4m08s — and then the runner vanished 16 minutes into the check. Steps 14-18 never ran despite `if: always()`, and the job's log was never finalised, so the runner was killed rather than a step failing; the ARC pod annotations point at the cluster reclaiming the pod. Two completed measurements were destroyed, and the fallback job wrote a row of dashes in their place. Each measurement now runs in its own job and uploads its result as soon as it finishes: prepare guest ELF + JSON, resolve the zkc ref to a full SHA stats compile --stats 15m cap trace trace --stats 45m cap trace-check trace --stats --check 120m cap report assembles whatever artifacts exist, commits notify Slack on failure or cancellation, main only So a lost runner costs one measurement rather than all of them, the three tasks appear separately in the Actions UI with their own durations, and the fast ones no longer wait on the slow one. The renderer needed no change: it already treats an absent step directory as "skipped", verified against a run directory with the check results withheld. The guard is deleted rather than fixed. Each job now carries its own literal cap and the input is clamped in-step to stay just below it, so the cap always fires where the outcome can be recorded. Job-level `timeout-minutes` has no precedent in this repo for expressions and cannot read `env` anyway, hence literals. `always()` becomes `!cancelled()` throughout, so cancelling works. The check cap drops from 240 to 90: on infrastructure that reclaims pods, "did not finish in 90m" is a more reliable weekly datum than an occasional heroic completion. The zkc binary deliberately does not travel between jobs as an artifact — upload-artifact drops the executable bit and nothing in this repo does download-then-execute. Instead prepare exports the resolved SHA as a job output and each measurement builds that exact commit via install-zkc-to. Only data moves through artifacts. record-lost-run is removed. It existed to prove a week had been attempted, but with per-job artifacts a partially lost run now yields a real row, and its only remaining case is covered by the new notification. That deletes a duplicated commit-back block and the bug that made its own commit fail. The five shared setup steps move into a composite action, so the split costs 44 lines of workflow rather than triple the preamble. 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>
…cheduled run Pre-review audit of the split, plus the review comments on #3714. One finding is serious and was already latent on main. On a cron run the `inputs` context is empty, and GitHub coerces both null and false to 0 when comparing across types — so `inputs.run-heavy != false` evaluates FALSE every Wednesday. The scheduled run would have skipped the trace and trace+check work entirely and, through the same expression on commit-results, never committed a row. This repo's only other schedule+dispatch workflow guards it explicitly for the same reason (slack-notify-e2e-runtime-report.yml:35). Both conditions now key off `github.event_name` instead. The remaining fixes concern what happens when a job does not complete: - stats now runs whenever prepare pinned a compiler, rather than being skipped by a failed prepare. A guest-build failure must not cost the cheapest and most-trended measurement. - trace and trace-check require prepare to have succeeded. Without the explicit result check they would still start — an `if:` overrides the implicit skip-on-failed-need — and then die on a missing artifact. - the guest JSON upload is gated on the build step actually succeeding, so `if-no-files-found: error` cannot pile a second, confusing failure on the first. - the guest JSON download is best-effort, and the measurement skips outright when the file is absent, so the renderer reads it as "skipped". - report is gated on prepare having produced provenance, so a run that died before recording anything writes no row of dashes. - the report checkout is no longer shallow: the commit step rebases onto origin/<branch>, which a depth-1 clone cannot do. Verified by simulating every path against the renderer: a full run, the check job evicted, a failed guest build, a failed compile, and run-heavy off. All five produce a usable row, and the two that previously destroyed completed work now preserve it. CodeQL's "checkout of untrusted code" alert on the measured checkout is documented rather than silenced: the workflow has no pull_request trigger, so there is no fork head to check out, and monorepo-ref can only be set by a dispatcher who already has write access. The structural mitigation is recorded next to the code — the jobs that build the measured tree never mint the App token; only report holds it, and it checks out the tooling ref alone. 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>
…ure conditions Review flagged that `report` could be skipped when a measurement job fails unless its condition includes always(). My reading is that any explicit `if:` replaces the implicit success() check, so `!cancelled()` alone already suffices and is what GitHub documents as the replacement for always(). I cannot demonstrate that from a run, though, and the repo has ten job-level precedents for `always() && !cancelled()` and none for the bare form. Since always() is always true the two are identical, so adopting the paired form costs nothing and removes the ambiguity for a reader. Step-level conditions keep the bare form, which does have precedent here. 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>
…zkc that got built Two review findings, both reachable and both worth fixing. prepare publishes its zkc_ref output before it records metadata, so an eviction between the two satisfies the report job's condition while leaving no meta.env. The renderer treats a missing metadata file as an empty map, so it would have appended `| – | – | – | – | – | – | – |` — a permanent row with no week, runner or refs. The report now fails if meta.env is absent or empty: notify reports it and the ledger stays clean. This is the same reasoning that removed record-lost-run; a row with no identity is worse than no row. arithmetization/Makefile's checkout-zkc-repo ends its fetch and checkout with `|| true`. A failed fetch therefore leaves the clone on the default branch from the initial `git clone --depth=1`, and the build silently measures a different compiler than the one the row will claim — precisely the confusion this workflow exists to prevent. The composite action now compares the built binary's stamped vcs.revision against the requested SHA and fails on a mismatch, warning only when nothing was stamped. Verified against a binary built through that very target from the Makefile's shallow detached clone: vcs.revision matches `git rev-parse v1.2.25` exactly, so the check has a real signal to work with. Not fixed here: the `|| true` itself. That Makefile is shared with several workflows that may rely on the tolerant behaviour, so tightening it belongs in its own change. 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>
…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>
Signed-off-by: Olivier Bégassat <38285177+OlivierBBB@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the weekly arithmetization “zkc metrics” automation by restructuring the GitHub Actions workflow so each measurement runs in its own job, improving resilience to runner loss/timeouts and making scheduled runs correctly execute the “heavy” measurements.
Changes:
- Split the weekly metrics workflow into
prepare,stats,trace,trace-check, andreportjobs, persisting intermediate results via artifacts. - Add a reusable composite action (
setup-zkc-measurement) to share the measurement-job preamble (checkout measured ref, install GNU time + Go, build pinnedzkc). - Update the accompanying documentation to reflect the new execution model and failure semantics (“skipped”/“TIMEOUT” instead of “run lost” placeholder rows).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
arithmetization/src/test/scripts/README-weekly-metrics.md |
Updates operator-facing docs to match the new multi-job + artifact-based workflow behavior. |
arithmetization/docs/metrics/zkc-weekly-metrics.md |
Updates ledger semantics to explain skipped/TIMEOUT interpretation and where to find failure reasons. |
.github/workflows/arithmetization-weekly-zkc-metrics.yml |
Reworks the weekly metrics workflow into separate jobs with artifact handoff, corrected schedule-safe gating, and a reporting/notifier stage. |
.github/actions/setup-zkc-measurement/action.yml |
Introduces a shared composite setup action to keep measurement jobs consistent and reduce duplication. |
Suppressed comments (1)
.github/workflows/arithmetization-weekly-zkc-metrics.yml:474
trace-checkis a job id containing a dash, soneeds.trace-check.resultis parsed as an expression (subtraction) rather than a context lookup. This condition will not evaluate as intended and can break the workflow step gating.
- name: Download trace-check measurement
if: ${{ needs.trace-check.result != 'skipped' }}
letypequividelespoubelles
approved these changes
Aug 5, 2026
letypequividelespoubelles
left a comment
letypequividelespoubelles
left a comment
Contributor
There was a problem hiding this comment.
blind approval
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.
No description provided.