feat: refresh electron's CI test-shard weights from its own timings - #210
feat: refresh electron's CI test-shard weights from its own timings#210MarshallOfSound wants to merge 2 commits into
Conversation
electron/electron packs its CI test shards from script/spec-weights.json, a table of seconds per spec file for each test job that has to be regenerated by hand from the spec-timings.json files the test jobs upload. Nobody does, so the tables drift as specs are added, removed, or change speed, and each release branch drifts on its own. Add a spec-weights roller: for main and every supported release branch it takes the last three green push runs of build.yml, reads the timing files out of their test_artifacts_* uploads (one table per job, keyed as the artifact is named; the Wayland allowlist job is left out), takes the median across runs so a single slow shard cannot skew a file, and packs the branch's spec files with the committed table and the fresh one using the same algorithm as split-tests.js. When some job's longest shard would get at least a minute shorter, a spec file has no weight, or a file's weight moved by a quarter and thirty seconds, it opens a PR on that branch (or updates its own open one), labelled semver/none and never backported since the numbers are per branch. Run as `npm run roll-spec-weights [branch]`.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked a couple of adjacent spots: the unconditional ref delete/recreate in rollSpecWeights() only runs when no PR passed the strict bot/fork ownership filter (so it can't clobber a live PR), and pickTable()'s fallback chain (all[legacy] ?? all.darwin_x64 ?? ... ?? {}) tolerates a malformed/legacy-shaped committed spec-weights.json without throwing, even though the parse itself is unchecked.
Extended reasoning...
Three confirmed findings from this review round (missing eventual-consistency delay before ref recreation, unhandled exception propagation from a single bad artifact zip aborting the whole branch's refresh, and stale/incorrect run IDs surfacing in the PR body/commit message when a sampled run yields no timing data) are being posted as inline comments and are sufficient on their own to warrant human attention before merge — this is a new roller job that writes commits and PRs directly to electron/electron on a schedule, so correctness and failure-isolation issues in the artifact-fetching and PR-mechanics paths are consequential. Given findings already exist, I am not writing a full zero-findings review; this short note only records two adjacent areas I examined and ruled out as not independently bug-worthy: (1) the orphaned-ref delete+recreate path is gated behind the same strict PR-ownership filter used elsewhere, so it cannot run while a legitimate open PR exists, and (2) pickTable()'s multi-level ?? fallback chain means an unexpected/legacy shape in the committed spec-weights.json degrades to some existing table rather than crashing, even though the initial JSON.parse of that file has no schema validation. Neither of these rises to the level of the three confirmed findings, and I have nothing further to add beyond what the inline comments already communicate.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| for (const runId of runIds) { | ||
| const jobs = await fetchRunTimings(octokit, runId); | ||
| if (jobs.length) runs.push(aggregateRun(jobs)); | ||
| else d(`run ${runId} has no timing artifacts (expired?) - ignoring`); | ||
| } |
There was a problem hiding this comment.
🟡 (optional) computeRefresh()'s per-run loop has no error isolation around fetchRunTimings(): if timingsFromArtifactZip() throws for any one artifact in any one of the 3 sampled runs (corrupt/truncated zip, unsupported compression method, non-zip response), the exception propagates out of computeRefresh and aborts the entire branch's refresh, discarding the other valid runs' data that this diff's own median-of-3 design exists to combine. Fix: wrap each run's fetchRunTimings() call in try/catch and skip (log) that run on failure, the same way an empty jobs array is already tolerated, so one bad artifact only drops one run instead of failing the whole branch.
Extended reasoning...
listTimingRuns samples 3 run IDs; for each, fetchRunTimings downloads and calls timingsFromArtifactZip (spec-weights.ts:237), which throws on 'not a zip file', 'corrupt zip central directory', or an unsupported compression method for any single artifact entry. That throw is not caught anywhere between fetchRunTimings and computeRefresh's for-loop (roll-spec-weights.ts:107-111), so it propagates through computeRefresh -> refreshBranch, where only the top-level per-branch try/catch in handleSpecWeightsCheck catches it and marks that branch failed for the whole cron run. A single flaky/interrupted upload from one CI job in one of the 3 runs therefore prevents any refresh for that branch that week, even though the other two runs' timings were perfectly usable.
Verification: nit. Real and reachable robustness gap, but low impact. computeRefresh's per-run loop (roll-spec-weights.ts:106-112) calls fetchRunTimings with no try/catch; fetchRunTimings (line 68-70) calls timingsFromArtifactZip with no guard; that function throws at spec-weights.ts:240 ('not a zip file'), :244 ('corrupt zip central directory'), and :260 ('unsupported zip compression method'). refreshBranch…
There was a problem hiding this comment.
The latest push brings this issue back at src/utils/roll-spec-weights.ts:111-123:
🟡 (optional) aggregateRun(jobs) at line 123 runs outside the try/catch (lines 113-118) that was added to isolate one bad run's data from failing the whole branch. If any job's parsed spec-timings.json lacks a usable files object (empty/older-schema/truncated content that still parses as JSON), aggregateRun throws (Object.entries on undefined), uncaught here, and kills the entire branch's refresh for the week -- the exact failure mode the surrounding try/catch was added to prevent, just via a different code path it doesn't cover. Fix: move aggregateRun(jobs) inside the try block (or validate/guard timings.files before use) so a malformed timings file only drops that run, not the branch.
Extended reasoning...
fetchRunTimings downloads and JSON.parses each artifact's spec-timings.json via timingsFromArtifactZip, returning JobTimings[] with whatever shape the file parsed to; no runtime validation of SpecTimings fields occurs. computeRefresh's loop only wraps the fetchRunTimings call in try/catch (lines 113-118); once jobs is returned successfully (even with a job whose timings.files is undefined, e.g. an interrupted write leaving {} or a future/older schema without files), line 123 calls aggregateRun(jobs) which does Object.entries(timings.files) per job -- this throws TypeError for undefined/null files, unguarded, propagating out of computeRefresh to refreshBranch's outer try/catch in spec-weights-handler.ts, which marks that whole branch failed for the week even though the other sampled runs had good data. This reopens, via a different call, the exact single-bad-input-fails-branch pattern the try/catch here was added to fix.
Verification: nit. Real, reachable code gap, low probability trigger. In computeRefresh (roll-spec-weights.ts:111-125) only fetchRunTimings is inside the try/catch (113-118); runs.push(aggregateRun(jobs)) at line 123 runs outside it. aggregateRun (spec-weights.ts:63) does Object.entries(timings.files) with no guard, and timingsFromArtifactZip (spec-weights.ts:261) pushes JSON.parse(...) results with…
A sampled run whose artifacts cannot be read (a corrupt upload, an unsupported zip) or have expired now drops out of the median instead of failing the whole branch, and the PR body lists only the runs whose timings were used. An unparseable committed spec-weights.json is treated as empty rather than aborting the refresh. Recreating an orphan roll branch waits two seconds after deleting it, as roll.ts does, since ref deletion is eventually consistent.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked two other candidates and ruled them out: fetchRunTimings() has no per-artifact isolation, but computeRefresh()'s run-level try/catch already contains a bad-artifact failure to that single run; and the PR body's uncapped "moved" file list is a verbosity concern, not a functional bug.
Extended reasoning...
This run re-verified the two issues raised in the prior review (missing 2s delay before createRef after deleting an orphan ref, and missing per-run error isolation in computeRefresh's sampling loop) and confirmed both were fixed in commit 52c2860, which added the delay and wrapped fetchRunTimings() in a try/catch per sampled run. However, that same fix introduced a new gap: aggregateRun(jobs) at roll-spec-weights.ts:123 now sits just outside the try/catch block (lines 113-118), so if a job's spec-timings.json parses as JSON but lacks a usable files object, aggregateRun's Object.entries(timings.files) throws uncaught and aborts the whole branch refresh — this is being posted as a separate CONFIRMED inline finding. I additionally examined two other candidates this run and ruled them out: fetchRunTimings() itself has no per-artifact try/catch, but that's fine because the enclosing per-run try/catch in computeRefresh already isolates any single corrupt artifact to just that run; and getSpecWeightsPRText's uncapped listing of every moved spec file is a cosmetic PR-body verbosity issue rather than a correctness bug.
The latest push brings back an issue flagged in an earlier review; replied in that comment's thread.
1 participant
Adds a weekly roller for
script/spec-weights.jsonin electron/electron, the per-job tablesscript/split-tests.jspacks CI test shards from. They are regenerated by hand today and drift as specs change; each release branch drifts on its own.What it does, for
mainand every supported release branch (npm run roll-spec-weights [branch]):pushruns ofbuild.ymlon the branch.spec-timings.jsonout of theirtest_artifacts_*zips, one table per test job keyed as the artifact is named (darwin_x64,mas_arm64,linux_x64_asan, ...). The Wayland allowlist job is skipped.spec/*-spec.tsthat exist on the branch.split-tests.js, and opens a PR only when it is material: a job's longest shard gets ≥ 60 s shorter, a spec file has no weight, or a file's weight moved ≥ 25 % and ≥ 30 s.roll.ts: branchroller/spec-weights/<branch>, only ever writes to its own PR (bot author, electron/electron head, exact branch name), honoursroller/pause, updates in place, labelssemver/noneplusno-backporton main /backport-check-skipon release branches. The body tabulates each job's longest shard before/after and links the sampled runs.Dry run against
maintoday (read-only): material on every job, e.g.linux_x64_asan14.0 → 11.3 min,mas_x6414.5 → 12.4,darwin_x6417.9 → 16.6.Depends on electron/electron#53737 (per-job tables in the sharder) landing on every target branch first; on a branch without it the roller's tables are ignored by the sharder's legacy fallback, so no harm, but no gain either.
To go live after merge: a Heroku Scheduler entry on
electron-rollerrunningnpm run roll-spec-weightsweekly, and the app needsactions: readon electron/electron for artifact downloads.No new dependencies; the zip reader is a central-directory walk over
node:zlib. Tests cover the aggregation, median, packing, materiality thresholds, fallbacks, the zip reader against a real-shaped fixture, and the PR mechanics against a mocked Octokit.