Repository navigation
Conversation
The tag push and main's push of the same commit start together, so the old lookup for a successful CI run always came up empty and every platform recompiled. Release also published commits whose main CI failed, and Mobile rebuilt the phones on the tag only to time out waiting for the release. A wait-for-ci job now finds main's CI run for the commit, waits for it, and hands its id to the build jobs; a failed run fails the release, and no run means compiling as before. A phones job does the same with the Mobile run (starting one on the ref when main never ran it) and adds the APK and the unsigned iOS app to the release. Mobile no longer runs on tags. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A dispatched Mobile run took 2h24m while macOS runners queued, past the phones job's 120 minutes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two jobs polling every 30 seconds came close to the token's hourly API allowance. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bc49e7a to
ca0d3f8
Compare
unRekable
left a comment
unRekable
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅ (0 critical, 2 warnings — both are deliberate fail-closed residue, not correctness bugs) — read both scripts in full and compared release.yml/mobile.yml against origin/main.
⚠️ Warnings
- wait-for-run.sh:31 — the "never publish from a rejected commit" guarantee is still probabilistic: if main's push run takes longer than the ~3.5 min search window to register,
run=comes back empty and the release silently compiles itself, publishing alongside a CI run that could later fail. A final re-check of the run's conclusion inpublish(or a longer window) would make the guarantee unconditional. - wait-for-run.sh:30 — a transient
ghfailure (rate limit, network blip) inside the search propagates viaset -euand fails the whole release job, which then skips every build. Fail-closed is the right direction, but the loop only retries "run absent", not API errors.
💡 Suggestions
- wait-for-run.sh:44 — on a "Re-run all jobs" of a release that dispatched a phone build, the search (push-only) comes up empty and
gh workflow runfires a second ~2.5 h Mobile run for the same SHA. Correctness is fine (the newest run is still downloaded); when--dispatchis set, probing for an existingworkflow_dispatchrun first avoids the duplicate. - release.yml:59 —
timeout-minutes: 240for the CI wait is tighter than the phones job's 300, while the comment above itself mentions macOS queues over an hour; aligning at 300 removes an avoidable late release failure.
✅ Looks Good
- The gate is fail-closed and the conclusion is read back from the run rather than trusting
gh run watch's exit status; drop-outs are resumed; non-success exits 1 and blockspublishthroughneeds. Two long waits at 60 s cadence stay well inside the token's API allowance. - Artifact reuse is honest per platform: all four binaries are verified present (and executable) before an artifact is trusted; anything missing falls back to compiling just that platform. Artifact names match ci.yml exactly, including the new
linux-aarch64arm. - The phones job fully replaces the old tag-triggered
attach-to-release(which raced release.yml by design); versioned/unversioned names and SHA256SUMS coverage match the existing release pipeline, so there is no double-publish. - Source-run selection is pinned to
--event push --branch main(or the self-dispatched run), so PR runs and other events can never feed binaries into a release; nopull_requesttrigger on release.yml. - Permissions stay least-privilege:
contents: read/actions: readby default,actions: writegranted only tophones.
Your local validation against real runs (success and failure cases) covers the happy path; the two warnings are follow-up hardening for the rare tails. Good, well-commented change.
| run= | ||
| attempt=1 | ||
| while :; do | ||
| run=$(newest --event push --branch main) |
There was a problem hiding this comment.
run= empty and the release compiles itself — publishing in parallel with a CI run that could later fail, which is exactly the race this PR is titled against. Low probability, and fail-closed otherwise; a final re-check of the source run's conclusion inside publish (or a longer search window) would make the guarantee unconditional.
|
|
||
| run= | ||
| attempt=1 | ||
| while :; do |
There was a problem hiding this comment.
gh error here (rate limit, network blip) propagates via set -eu and fails the whole release after possibly hours of waiting. Fail-closed is correct, but the loop retries only "run absent", not API errors — retrying those too (or a single retry-after-60s) would save an otherwise doomed run.
| since=$(date -u -d "@$(($(date +%s) - 60))" +%Y-%m-%dT%H:%M:%SZ) | ||
| echo "wait-for-run.sh: no $workflow run on main for $sha; starting one on $GITHUB_REF_NAME" | ||
| gh workflow run "$workflow" --ref "$GITHUB_REF_NAME" | ||
| for attempt in 1 2 3 4 5 6 7 8 9 10; do |
There was a problem hiding this comment.
💡 Suggestion: on a "Re-run all jobs" of a release that dispatched a phone build, this push-only search comes up empty again and gh workflow run fires a second ~2.5 h Mobile run for the same SHA. Correctness is fine (the newest run is still downloaded), but when --dispatch is set, probing for an existing --event workflow_dispatch run for the SHA first avoids the duplicate.
| wait-for-ci: | ||
| if: github.event_name == 'push' || inputs.announce == '' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 240 |
There was a problem hiding this comment.
💡 Suggestion: timeout-minutes: 240 is tighter than the phones job's 300, and the comment above itself mentions macOS queues over an hour — a worst-case queue could fail the release at the last step. Aligning at 300 costs nothing.
| # The run is the one wait-for-run.sh found and waited for; release.yml passes its id. | ||
| # found=true means target/release holds tinyclient, chatd, mediad and tinyclient-login. | ||
| # A commit CI did not build (or whose CI failed) leaves found=false, and the caller compiles. | ||
| # With no run id, or a run without this platform's package, found=false and the caller compiles. |
There was a problem hiding this comment.
✅ Nice: the take() side verifies all four binaries exist (and are executable / present as .exe) after extraction before trusting the artifact, and any missing piece falls back to compiling just that one platform — a partial extraction can never pass as a built stage. Solid design.
Release promotes main's runs instead of racing them.
reuse-binaries.sh; a missing package still falls back to compiling. aarch64 reuse starts once ci/speed addstinyclient-linux-aarch64.tinyclient-$LABEL-android-debug.apkandtinyclient-$LABEL-ios-unsigned.zipasrelease-phones. On a tag those names match the existing release assets, and publish adds unversioned copies (tinyclient-android-debug.apk) like every other file.Checked locally against real runs: 1d4acaa -> run 41479383097, success, id passed on; f542c04 -> run 41506153717, failure, exit 1.
🤖 Generated with Claude Code