Skip to content

fix(install): give the proxy uv install the same retry window - #907

Open
tend-agent wants to merge 1 commit into
fix/claude-install-retry-31298989023from
fix/proxy-uv-install-retry-31298989023
Open

fix(install): give the proxy uv install the same retry window#907
tend-agent wants to merge 1 commit into
fix/claude-install-retry-31298989023from
fix/proxy-uv-install-retry-31298989023

Conversation

@tend-agent

Copy link
Copy Markdown
Collaborator

Stacked on #906 — based on its branch, so the diff here is only the install-proxy-uv.sh half. GitHub retargets this to main when #906 merges.

Problem

#906 widens the retry window on install-claude-binary.sh after a CDN 403 burst cost review-reviewers a matrix leg. Reviewing it surfaced that shared/steps/install-proxy-uv.sh carries the identical loop — for i in 1 2 3, sleep $((i * 5)), after 3 attempts — and shares every property the rationale rests on:

  • It runs in the same job (claude/action.yaml), ~80 lines ahead of the claude install, so it is equally ahead of the agent step and equally invisible to Report failure, which is gated on steps.claude.outcome == 'failure'. A run lost here files no tend-outage row.
  • It fetches over the same runner egress, on all five review-reviewers legs at once, so a rate limit hits them together and an unjittered backoff has them retry together.
  • Its three attempts span the same ~15s window the observed blip outlasted.

A 403 from astral.sh instead of claude.ai loses the leg identically. Fixing only the claude half leaves the exposure roughly halved rather than closed.

Change

Same shape as #906: five attempts, exponential backoff, jitter — 5, 10, 20, 40 seconds plus 0–9s each. Happy path unchanged (one fetch, no sleep).

Worth stating the same ceiling caveat: when curl fails fast the added cost to a doomed install is ~95s; when the CDN hangs, each attempt burns the full timeout 60 and the worst case goes from ~195s to ~411s.

Tests

install-proxy-uv.sh had no test coverage at all. Four tests added, reusing the fixture pattern #906 establishes (_fake_bin, FAKE_SLEEP_RECORDING, the attempt/sleep readers) with a curl fake that emits astral's installer shape — read on stdin by sh -s --, so it stays POSIX rather than bash. Reusing those helpers is why this is stacked rather than parallel; the alternative was a second copy of each fake and a conflict at the end of the file.

Three of the four fail against the pre-fix script:

Test Against the base branch
rides out a 403 burst (4 failures then success) fails — gives up after 3
backs off exponentially fails — sleeps [5, 10]
reddens when every attempt fails, naming the count fails — says "after 3 attempts"
installs first try without sleeping passes (happy path is unchanged)

Full generator suite: 387 passed. shellcheck -S warning clean on the modified script; ruff check and ruff format --check clean.

@tend-agent tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry loop is now byte-identical in install-claude-binary.sh and install-proxy-uv.sh with no shared source of truth, so the next adjustment to the window — or a bug in the jitter expression — has to be found and applied twice, and the two rationale comments already differ in wording while describing the same policy. The repo has the mechanism for this: shared/steps/lib/run-issue.sh is sourced by both rate-limit-preflight.sh and report-failure.sh via . "${SCRIPT_DIR}/lib/run-issue.sh". Extraction isn't free here, though — the claude copy runs inside sudo -u "$SANDBOX" env … bash <<'EOF' as the sandbox user, so a sourced helper would need the lib path threaded through that env and the file readable by the sandbox UID, which I haven't verified. A follow-up rather than a change to this PR, but worth deciding deliberately: the duplication is what lets the two windows drift apart later.

Nothing else — the loop tracks #906 line for line, shellcheck -S warning is clean on the modified script, and the four new tests fail against the base branch's script in exactly the three places the PR body claims.

@tend-agent

Copy link
Copy Markdown
Collaborator Author

Extracted, in #908, stacked on this branch so the diff there is only the extraction.

The sandbox boundary you flagged as unverified is real, and it rules out the obvious approach: install-claude-binary.sh runs its loop as the sandbox UID, and nothing grants that UID read access to the action's own checkout — setup-sandbox.sh grants traversal (o+x) on the workspace's ancestors only, and the action lives elsewhere under _actions/. So . "${SCRIPT_DIR}/lib/retry.sh" from inside the heredoc would be resting on whatever mode the runner happened to leave on the action checkout.

Concatenating the lib onto the front of the stdin stream sidesteps it — the file is read on the runner side of the privilege drop, and sudo -u/env/the pinned XDG dirs are untouched:

cat "${SCRIPT_DIR}/lib/retry.sh" - <<'EOF' \
  | sudo -u "$SANDBOX" env HOME="$AGENT_HOME" … bash
set -euo pipefail
retry_install "claude $CLAUDE_VERSION" \
  "curl -fsSL https://claude.ai/install.sh | bash -s -- '$CLAUDE_VERSION'"
EOF

One correction to the review: the two loops aren't byte-identical — the fetched command and both echo strings differ, so what's shared is the five lines of window mechanics (ATTEMPTS, the backoff expression, the jitter, the timeout 60, the set -o pipefail inside bash -c). That's still exactly the part that drifts, and the pipefail line is the one whose omission fails silently, so it's worth having once.

The eight tests from #906 and #907 pass unmodified against the lib, which is the useful signal: the window is now asserted from both sides of the sudo boundary. Full suite 387 passed, shellcheck -S warning clean.

Left this PR alone rather than folding the extraction in — but if three PRs for one window is worse than a slightly wider one, squashing #908 into this branch loses nothing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant