ci: write releases from one paced job, not from 107 at once - #121
Conversation
Every matrix job uploaded its images to three shared releases with softprops/action-gh-release, so a nightly fired on the order of 400 concurrent asset writes at `nightly`, `latest` and the dated tag. OpenIPC/firmware ran exactly this design and the releases API answered HTTP 500 under the contention -- run 27108181857 built every board and still went red on 14 jobs, all of them in their upload step. Firmware then hit the sequel: consolidating into one job but keeping concurrent uploads tripped the per-actor secondary rate limit at 350 of 393 assets. This lands where that ended up, without the two failures in between. Matrix jobs now stage what they produced into dist/ and hand it to actions/upload-artifact. A single publish job downloads all of it and drives the release writes with gh: one asset at a time, paced ~1/s under the per-minute mutation ceiling, with exponential backoff so a transient 403/429 is retried rather than failing the nightly. The single-writer property is what fixes the 500s; the pacing is what keeps the fix from tripping the other limit. The dated release keeps the full asset set, because enrich_manifest.py only ever reads dated releases. `nightly` and `latest` are delivery aliases for flashers and now get images only: re-uploading the size sidecars to two more tags is pure rate-limit cost, and nothing reads them from those tags. Staging copies whichever of the two naming schemes the build used -- compound devices rename to <device>-<flash>.tgz in the workspace root, single-underscore ones leave openipc.<soc>-<flash>-<variant>.tgz under openipc/output/images/ -- so asset names are byte-identical to today. Checked both against enrich_manifest.py's COMPOUND_RE and SIMPLE_RE. ci-gate covers publish, so a failed release write fails the run instead of passing quietly. The staging loop uses `if` rather than a trailing `&&` chain: the step runs under `bash -e`, where a chain that tests false is only harmless because something after it happens to reset the status. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
PR Summary by QodoCI: publish GitHub releases from a single paced job
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
Two High findings on #121, both real. The download step carried continue-on-error: true, so a transient or real artifact-download failure produced an empty dist/, the publish step's count guard skipped it, the job went green, and ci-gate accepts publish=success. A nightly could silently write nothing while the run stayed green. The flag stays -- it exists so a matrix where every device failed, and so uploaded no fw-* artifact at all, does not hard-fail before the gate can report why -- but Collect now turns it back into a failure for every other case: if the matrix went green, a download that did not succeed is an error, and zero collected assets is an error. Only "matrix failed and produced nothing" is still tolerated, and the gate fails that run on the matrix result anyway. The second finding said count could read " 0" because wc pads. On GNU coreutils, which is what the runner has, it does not -- that is BSD behaviour, and `printf '' | wc -l` here gives an unpadded 0. Normalised anyway, since it costs a tr and stops the guard depending on which wc is in front of it. Chasing it did surface a worse hole it sits next to. dist/ can hold sidecars and no images -- one device that produced a size report and no .tgz is enough -- and count is then 1, so publish ran, force-moved the nightly and latest tag refs, and uploaded zero images. The two tags users flash from would point at a build with no firmware behind them. Both tags now move only when there is at least one image to move them to; otherwise they stay on yesterday's, with a warning. Checked all four states offline, since none of this path can run on a pull request: green matrix with a broken download, green matrix with an empty dist, failed matrix with nothing produced, and a normal night. The first two now fail and used to pass. Note the same continue-on-error and unnormalised wc are in OpenIPC/firmware's publish job, which is where this one came from. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review on OpenIPC/builder#121 found two holes in its publish job. That job was seeded from this one, and both holes are still here. The artifact download runs with continue-on-error, so a transient or real download failure produced an empty dist/, the count guard skipped the publish step, the job went green, and ci-gate accepts publish=success -- a nightly could silently write nothing while the run stayed green. The flag stays, because it is load-bearing for exactly one case: a matrix where every board failed uploads no fw-* artifact at all, and a hard download failure there would mask the real reason in the gate. Collect now turns it back into a failure for every other case: a green matrix with a download that did not succeed is an error, and a green matrix with zero collected assets is an error. Only "matrix failed and produced nothing" is still tolerated, and the gate fails that run on the matrix result anyway. The nightly and latest tag moves were unconditional once any asset existed. dist/ can hold sidecars and no images -- one board producing a size report and no .tgz is enough -- and the script would force-move the two tags flashers pull from to a build with no firmware behind them. Both now move only when there is at least one image to move them to; otherwise they stay on yesterday's build, with a warning. The count is also normalised through tr. On the runner's GNU coreutils wc does not pad, so this was not exploitable here -- padding is BSD behaviour -- but the guard should not depend on which wc is in front of it. While in the area, ci-matrix.py now writes $GITHUB_OUTPUT itself instead of documenting that it does while relying on the workflow to redirect stdout (the docstring inaccuracy was also flagged on builder). Under a redirect every print() is one keystroke away from corrupting the step outputs, and a crash between the first line and the last leaves a half-written file that Actions still reads. Falls back to stdout when the variable is unset; --stdin never touches the file. All four collect states and all output paths checked offline; none of the publish path can run on a pull request. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every matrix job uploads its images to three shared releases with
softprops/action-gh-release, so a nightly fires on the order of 400 concurrent asset writes atnightly,latestand the dated tag.OpenIPC/firmware ran exactly this design and the releases API answered HTTP 500 under the contention — run
27108181857built every board and still went red on 14 jobs, all of them in their upload step. Firmware then hit the sequel: consolidating into one job but keeping concurrent uploads tripped the per-actor secondary rate limit at 350 of 393 assets. This lands where that ended up, without the two failures in between.What changes
Matrix jobs stage what they produced into
dist/and hand it toactions/upload-artifact. A singlepublishjob downloads all of it and drives the release writes withgh:--clobberon upload, tag refs moved withgh api -X PATCHThe single-writer property is what fixes the 500s; the pacing is what keeps that fix from tripping the other limit. Best-effort by design: it publishes whatever the matrix produced even if some devices failed, which is what
enrich_manifest.pyalready assumes.Asset routing
enrich_manifest.pyonly ever reads dated releases (list_dated_releases()→TAG_RE = ^nightly-\d{8}-[0-9a-f]{7}$), and only parses.tgznames.nightly-YYYYMMDD-<sha>sizes.*.jsonnightly,latestRe-uploading the size sidecars to two more tags is pure rate-limit cost: nothing reads them from those tags.
Asset names are unchanged
Staging copies whichever of the two naming schemes the build used, so names are byte-identical to today:
gk7205v200_fpv(1 underscore)openipc.gk7205v200-nor-fpv.tgzssc338q_apfpvopenipc.ssc338q-nor-apfpv.tgzt31_lite_wyze-v3b(2+)t31_lite_wyze-v3b-nor.tgzhi3518ev200_lite_switcam-hs303-v2…-nand.tgzVerified every one of them still parses through
enrich_manifest.py'sCOMPOUND_RE/SIMPLE_RE— all six round-trip to the right(platform, flash)pair.Also
ci-gatenow coverspublish, so a failed release write fails the run instead of passing quietly.ifrather than a trailing&&chain. The step runs underbash -e, where a chain that tests false is only harmless because something after it happens to reset the status — I'd rather that not be load-bearing.Scope note
I'd said this PR would also record the firmware SHA in the nightly notes, as groundwork for narrowing the nightly by upstream diff. I dropped it. Recording a SHA resolved at the start of the run would be a half-truth: each of the 107 jobs clones OpenIPC/firmware at HEAD independently, so a nightly can already contain devices built from different firmware commits. Making it truthful means pinning every job via
OPENIPC_FW_REV, which costs a full clone instead of--depth=1across 107 jobs. That cost is only worth paying if the nightly-narrowing actually pays off, so it belongs in that change, measured, rather than smuggled in here.Testing
This is a
master.yml-only change, so CI narrows it to the 15-device smoke set. Note that the publish path itself cannot run on a PR (it's guarded ongithub.event_name != 'pull_request', and there are no artifacts) — so the real exercise is the first nightly after merge, or aworkflow_dispatch. What is verified here offline: theDATED/IMAGESsplit, the staging step against both naming schemes and against missing NAND/sizes, the notes block, and the manifest round-trip.🤖 Generated with Claude Code