fix(install): ride out a CDN blip instead of losing the run to it - #906
fix(install): ride out a CDN blip instead of losing the run to it#906tend-agent wants to merge 1 commit into
Conversation
tend-agent
left a comment
There was a problem hiding this comment.
Reviewing my own PR, so this is a COMMENT rather than a verdict.
The sibling install step still carries the pre-fix loop. shared/steps/install-proxy-uv.sh has the same idiom this PR replaces — for i in 1 2 3, sleep $((i * 5)), failed to install uv ${UV_VERSION} after 3 attempts — and it shares every property the rationale here rests on. It runs in the same job (claude/action.yaml, the run: bash .../install-proxy-uv.sh step, ~80 lines ahead of the install-claude-binary.sh one), so it's equally ahead of the agent step and equally invisible to Report failure; it fetches over the same runner egress on all five review-reviewers legs at once; and its three attempts span the same ~15s. A 403 burst from astral.sh instead of claude.ai loses the leg identically, with no outage row — the incident in the PR body with one hostname swapped. Fixing one of the two leaves the described exposure roughly halved rather than closed.
I'd rather not fold it into this PR: either script's loop could be reverted without touching the other, which is the repo's test for "separate PRs". Happy to open the follow-up — same five-attempt jittered shape, plus the fixture pattern these tests establish (there's currently no test coverage for install-proxy-uv.sh at all). Say the word, or close it as not worth the churn given how rare the blip is.
The worst-case ceiling grows more than the body claims. "~90s longer to say so" holds for the observed shape, where curl returns 403 in well under a second and the added cost is just the extra backoff. It doesn't hold when the CDN hangs: each attempt can burn the full timeout 60, so the step's ceiling goes from 3×60+15 ≈ 195s to 5×60+111 ≈ 411s. Not an argument against the change — but if the open question is whether 5 attempts / 40s is the right ceiling, ~7 minutes for a wedged install is the number to weigh, not ~90s.
Verification notes
- Confirmed the tests discriminate: with
shared/steps/install-claude-binary.shreverted to0f35b6bin place,test_install_claude_binary_rides_out_a_403_burst,..._backs_off_exponentially, and..._reddens_when_every_attempt_failsfail (backoff did not double: [5, 10],after 3 attempts), and..._installs_first_try_without_sleepingpasses. Matches the table in the PR body. pytest.approx([5, 10, 20, 40], abs=9)is exactly the jitter width, so the tolerance is tight rather than loose, and the separate floor assertion pins the lower bound — a flat or shortened sequence still fails. No flake risk:sleepis faked.- Arithmetic checks out under
set -euo pipefail:**binds tighter than*and%, so5 * 2 ** (i - 1) + RANDOM % 10is(5 * 2^(i-1)) + (RANDOM % 10)→ floors 5/10/20/40, andRANDOMis always set so-udoesn't bite. - The only other unretried network install in the actions is
npm install -g "@openai/codex@..."incodex/action.yaml, which npm already retries internally — no gap there.
|
Both land. Taking them in reverse order, since only one of them changes anything here. The ceiling. You're right, and the body was wrong in a way worth fixing rather than arguing: ~90s was the fast-failure number stated as though it were the general one. Checked against the pre-fix script at Having weighed the real number I'd still keep five attempts, and said so in the body: reaching 411s takes all five attempts hanging, a much rarer shape than the fast 403 the change exists to ride out, and in that case the leg is lost either way — the extra minutes cost runner time but change no outcome. Trimming the count or the ceiling re-narrows exactly the window this widens. That's a judgement rather than a proof, so it stays an open question for whoever merges — but now with the right number attached to it.
One correction to your note: it isn't uncovered. I'd rather sequence that follow-up after this lands than open it now: if the five-attempt shape gets dialled back here, the second PR wants whatever this one settles on, and the fixture pattern it would reuse doesn't exist until this merges. Opening both at once means reworking two PRs to resolve one question. So I'll open it once this is merged — say the word if you'd rather skip it as churn, and I won't. |
Problem
review-reviewersrun 31297986524 lost itsnumbagg/numbaggleg to a transient 403 from the claude installer CDN. All three install attempts landed inside ~15.7s and all three got the same 403, while the other four matrix legs — installing the same version from the same runner at the same moment — succeeded:Two things make this worse than a lost leg. The blip outlived the retry window: three attempts at a flat 5s backoff spend everything inside ~15s, which is short enough that a CDN hiccup takes the whole run rather than costing it a few seconds. And the failure is silent —
Report failureis gated onsteps.claude.outcome == 'failure', so a step that fails ahead of the agent files notend-outagerow and appends nothing to the open tracker (#905). The run went red with no record anywhere a maintainer looks. That gating is #857's subject and is not touched here.The concurrency angle points the same way:
review-reviewersinstalls on five matrix legs at once from one runner's egress address, so a rate limit hits them together — and with an unjittered backoff, every leg retries together too, reproducing the burst that tripped it.Change
shared/steps/install-claude-binary.shgoes from three attempts at a flat 5s to five backing off exponentially with jitter — 5, 10, 20, 40 seconds plus 0–9s each, so the window spans roughly 75–110s instead of 15s, and sibling legs spread out instead of retrying in lockstep. The happy path is unchanged: one fetch, no sleep.Tests
Four new tests in
generator/tests/test_shared_steps.py, following the existing fake-binaries-on-PATH pattern (fakesudodrops the privilege change, fakecurlfails a configurable number of times then emits an installer, fakesleeprecords the delay instead of paying it). Three of the four fail against the pre-fix script:main[5, 10]Full generator suite: 383 passed.
shellcheck -S warningclean on the modified script.Uncertainty
One observed occurrence — I swept the 20 most recent failed
tend-*/review-*runs and this is the only install-time 403 among them, so the blip is rare rather than chronic.The cost of being wrong about that frequency depends on how the install fails, and it splits into two very different numbers. Against a fast failure like the observed 403,
curlreturns in well under a second, so the added cost is only the extra backoff: the step goes from ~15s to ~75–110s. Against a CDN that hangs, each attempt burns the fulltimeout 60, so the step's ceiling goes from 3×60+15 ≈ 195s to 5×60+111 ≈ 411s — about 3.6 minutes longer, not the ~90s an earlier revision of this section claimed (that figure only ever held for the fast-failure shape).Weighing ~7 minutes for a wedged install rather than ~90s, I'd still keep five attempts: reaching that ceiling takes all five attempts hanging, a much rarer shape than the fast 403 this exists to ride out, and in that case the run is lost either way — the extra minutes cost runner time but change no outcome. Trimming the count or the ceiling re-narrows the window the change exists to widen. Still happy to dial it back if you'd rather bound the wedged case.