fix(download): retry transient GitHub API/network failures during install

[MichaelTaylor3d]

DO NOT MERGE — gates pending. Rebased onto main @ 05cc71a (v0.45.0) on 2026-08-20; every check recorded before that rebase ran against a CONFLICTING head and is meaningless. The gate round has not returned.

Root cause

Every HTTP request the installer makes was single-shot, and any failure aborts the whole install and rolls back every completed step. On run 31553659321 (dc598404) the dig-app release lookup lost its TLS handshake (invalid peer certificate: Other(OtherError(UnsupportedCertVersion))) and took the install down with it.

The keychain hypothesis is disproved, not assumed away:

• ureq is declared default-features = false, features = ["tls"], so its roots come from the compiled-in webpki-roots. Cargo.lock contains no rustls-native-certs and no rustls-platform-verifier — the macOS system keychain is structurally not in the installer's trust set.
• The dig-node asset download over HTTPS succeeded 7 seconds after the CA was added to the keychain, in the same process (log 01:28:34.46 keychain, 01:28:34.93 download).

The intermittency matches: the other recent reds on this workflow are the same shape with different blips — 31438236229 a 403 on api.github.com, 31485103686 a genuinely missing digd asset (a real, separate gap, not this bug).

Fix

A bounded retry (3 attempts, 500 ms doubling backoff) around both request paths (get_text_with_token, fetch_bytes). Retries only what could answer differently: transport errors (DNS/connect/TLS handshake/dropped connection), 429, 5xx. Classification is on the typed ureq::Error, never on a formatted string.

Deliberately not retried: 404 (latest_release reads it as "no published release" and falls back to the releases list — retrying would triple every fallback), and 401 (a rejected credential).

403 is path-dependent, per review: retried on the unauthenticated asset-download path (fetch_bytes_retrying), where GitHub's 403 is an anonymous rate-limit/abuse-detection trip and transient by construction; not retried on the authenticated API path, where it means a rejected or exhausted token and no retry can change the answer. One credential binding feeds both the Authorization header and the retry policy, so the two cannot drift apart.

Regression tests

6 tests in src/download.rs driving the real ureq request against a local server that drops a controlled number of requests (counting requests, not accepted sockets) and asserting the observed wire behaviour:

• retries a dropped connection and succeeds on a later attempt (3 requests seen)
• gives up after exactly HTTP_ATTEMPTS — a retry loop must not become an unbounded hang
• a 404 is not retried and still satisfies is_release_not_found
• fetch_bytes retries a dropped connection
• a 502 is retried, a 401 is not
• fetch_bytes retries a 403 (HTTP_ATTEMPTS requests) while get_text_with_token with a token does not (exactly 1)

Proven load-bearing by reverting only the fix, on committed state (2026-08-20):

• disable the retry arm (Err(f) if false && f.retryable) → 5 of 6 fail; the 404 assertion stays green, because it is a guard rather than the fix.
• the 403 test pins the placement, from both sides: collapse (*code == 403 && !authenticated) to *code == 403 → FAILS; drop the 403 arm entirely → FAILS. A one-directional fixture would have passed one of those.

Blast radius

gitnexus was not used — no per-worktree index exists for this checkout and building one was not worth the 10-minute budget for a 1-file diff; radius established by ripgrep and stated here (§2.0 bound 2, fallback declared). Callers of the changed functions: lib.rs:703/705 (the install plan's release resolution) and update.rs:280 (the update check). Both only gain retry on failures that previously aborted them. release.rs is URL construction only. No public signature changed; fetch_bytes keeps its signature. classify gained a third parameter and is private to the module.

Verification (re-run after the rebase, at 12ea3f4)

• cargo test --lib: 865 pass, 1 fail — secure::tests::the_defect_reproduces_on_a_real_directory_and_the_repair_clears_it, which panics creating its fixture with Os { code: 5, PermissionDenied } and needs an elevated Windows shell. src/secure.rs is not in this diff; environmental, pre-existing.
• cargo test --lib download::: 45/45 in 0.37 s — confirming the \r\n\r\n harness fix; the pre-fix version spent ~1 s of socket timeout per request.
• cargo clippy --all-targets -- -D warnings: clean. It was failing at the pre-rebase head (clippy::unnecessary_map_or on download.rs:247) — a required-check failure introduced by the review-fix commit and repaired in 12ea3f4.
• cargo fmt --check: clean. Also failing at the pre-rebase head, same commit.

Version

0.45.0 -> 0.45.1 — patch: a compatible reliability fix, no API change. (Was 0.43.1 before the rebase; main has since released 0.44.1 and 0.45.0.)

Refs DIG-Network/dig_ecosystem#2784

[MichaelTaylor3d]

CHANGES-REQUIRED — head reviewed: dc17ef420b281d9ba9deaafcd5d28c73370e035a.

The diagnosis holds and the fix is the right shape. I independently confirmed the disproof of the keychain hypothesis: Cargo.toml:55 declares ureq = { version = "2", default-features = false, features = ["tls", "json", "gzip"] } and Cargo.lock contains webpki-roots (x2) and neither rustls-native-certs nor rustls-platform-verifier — the platform trust store is structurally unreachable from this binary. The 404 exclusion is genuinely load-bearing (download.rs:180-190 latest_release falls back to the releases list on is_release_not_found). The retry is provably bounded: with_retry runs attempts-1 loop iterations plus one final op(), and the backoff cannot grow past 2s at HTTP_ATTEMPTS = 3. All 12 required checks asserted BY NAME from branch protection are SUCCESS, 0 unresolved threads, and e2e run 31612407531 has head_sha == dc17ef42 with all 10 jobs green including install -> health -> uninstall (macos-14).

Two findings block. Neither is about the fix being wrong; one is a mangled string literal that makes the new test harness sleep instead of parse, and one is a residual failure mode observed on this PR that the retry predicate does not cover.

Also noted, non-gating and resolved by me: with_retry(0, ..) executes one attempt rather than zero (unreachable in production, both call sites pass HTTP_ATTEMPTS); is_release_not_found string-matches any message containing 404 (pre-existing, unchanged here).

SemVer 0.43.0 -> 0.43.1 is correct (behaviour-compatible reliability fix, no signature change). SPEC.md states the retry contract including the deliberate non-retries — §4.2 satisfied except for the 403 wording called out below.

[MichaelTaylor3d]

Revived + rebased — DO NOT MERGE, gates pending

Branch loop/2784-macos-e2e-tls · head 12ea3f4 · was 0c38087 (CONFLICTING since 2026-08-12).

Done

1. Rebased onto main @ 05cc71a (v0.45.0) — rebase, not a merge, since this repo requires linear history. Four conflicts, all resolved on their merits: the stub commit dropped as empty; DEVELOPMENT_LOG.md keeps both entries (#1911's from main and this PR's #2784 one); the version lands at 0.45.1 in Cargo.toml, Cargo.lock and gui/app/src-tauri/Cargo.lock (it had been 0.43.1 against a main that has since shipped 0.44.1 and 0.45.0). git diff origin/main...HEAD greps clean for <<<<<<<.
2. All 3 gating review threads verified against the code and resolved — the mangled \r\n\r\n terminator, the bare-LF mock response, and the path-dependent 403. Each fix is really present; see the individual replies for the file:line evidence.
3. Two required checks were RED at the old head and are now clean. The review-fix commit had left cargo clippy --all-targets -- -D warnings failing (clippy::unnecessary_map_or, download.rs:247) and cargo fmt --check failing on the same hunk. Fixed in 12ea3f4, which also collapses the duplicated has_token / token.filter(..) predicate into one credential binding so the header and the retry policy read the same value.
4. Re-proved the tests are load-bearing (committed first, reverted via a file copy): disabling the retry arm fails 5 of the 6 retry tests, and the 403 test fails from both directions — retry-403-everywhere and retry-403-nowhere — so it pins the placement, not just the outcome.

State

• cargo test --lib: 865 pass, 1 fail — secure::tests::the_defect_reproduces_on_a_real_directory_and_the_repair_clears_it, Os { code: 5, PermissionDenied } creating its own fixture, needs an elevated Windows shell. src/secure.rs is not in this diff. Environmental and pre-existing.
• cargo test --lib download:: 45/45 in 0.37 s · clippy clean · fmt clean.

Remaining

The gate round. Every check on this PR before today ran against a CONFLICTING head and proves nothing; the 12 required contexts must re-run on 12ea3f4 and be asserted by name from branch protection (an absent required context is not a passing one). Staying DRAFT until those verdicts return.

Off-path, logged not fixed (§2.6)

• is_release_not_found classifies by string-matching any message containing 404, so a body or URL containing "404" would be read as a missing release. Pre-existing, untouched here, and the new test only pins the request count.
• with_retry(0, ..) executes one attempt rather than zero. Unreachable — both call sites pass HTTP_ATTEMPTS.
• The reviewer noted 31485103686 was a genuinely missing digd asset — a real separate gap this PR does not address.

[MichaelTaylor3d]

Correctness gate: PASS

Head read: 12ea3f48bb6d4aa0e12cc285a1e28680de5c801c (resolved from gh pr view 68 --json headRefOid; isDraft: true, mergeStateStatus: CLEAN). This is the correctness leg only — the credential-handling security leg runs separately.

1. Per-path 403 classification — correct

classify(url, err, authenticated) at src/download.rs:56 retries 429, >=500, and 403 only when !authenticated.

• fetch_bytes_retrying (src/download.rs:285) passes false literally, and that path provably sends no Authorization header — so an anonymous rate-limit / abuse-detection 403 on releases/download is retried. Right disposition.
• get_text_with_token_retrying (src/download.rs:251) passes credential.is_some() — derived, not restated. With a token, 403 is single-shot (rejected/exhausted credential). Without a token this path also treats 403 as retryable, which is correct rather than an oversight: an unauthenticated api.github.com 403 is the anonymous rate limit, i.e. genuinely transient.
• 401 is retryable on neither path; 404 on neither. Both asserted.

2. The one-binding collapse — behaviour-preserving

The duplicated predicate is gone: credential = token.filter(|t| !t.is_empty()) (src/download.rs:245) is the single source for both the header and the authenticated flag, so they cannot drift. The None case is byte-identical to the pre-PR code (same filter, header omitted). The empty-string case is also coherent in the new shape: no header is sent and authenticated == false, so the 403 is attributed to anonymous rate-limiting rather than to a credential — which is exactly the disagreement the old map_or/filter pair could have produced.

3. Test load-bearingness — re-proved independently by mutation, in a throwaway worktree

I did not take the claim on trust. Three mutants, each on a fresh detached worktree at 12ea3f4 (removed afterwards; no shared checkout touched):

mutant	result
*code == 403 unconditionally (403 always retryable)	fetch_bytes_retries_403_but_get_text_with_token_does_not FAILS
403 arm deleted entirely	same test FAILS
retry arm disabled (Err(f) if false && f.retryable)	5 of 6 FAIL; only get_text_does_not_retry_a_404_... survives

So the 403 test fails from both directions and therefore pins placement, not an outcome — the nearest wrong implementations (retry-everything, retry-nothing) are both rejected. The 404 test surviving is correct and expected: it is a guard against over-retrying, not a proof of the fix, and the lane characterised it that way.

Bound is also proven, not just presence: get_text_gives_up_once_the_retry_budget_is_spent asserts observed requests == HTTP_ATTEMPTS, so the loop cannot become unbounded — the other inequality of the retry guard. Assertions are on wire-observable request counts from a real local server, not on the loop's own branches, so there is no inexpressive-double surface here.

Unmutated: cargo test --lib download:: → 45 passed, 0 failed.

4. is_release_not_found substring match — not gating here

src/download.rs:174 is err.contains("404") || err.contains("Not Found"). It is pre-existing and unmodified, and this PR does not change the message format it reads (GET {url}: {err}), so the PR neither introduces nor widens the hazard. Judged safe in this context:

• Production callers only ever pass api.github.com/repos/.../releases/latest URLs, none of which contain either token.
• Production retry classification is on the typed ureq::Error, never on this string, so a false positive here cannot cause a wrong retry decision.
• The blast radius of a false positive is one extra releases list request in latest_release (src/download.rs:194), which then fails on its own merits. No wrong install, no silent success.

Correctly logged off-path under end-to-end-first. Noted as a non-gating observation below.

5. Version bump — consistent

0.45.0 → 0.45.1 in Cargo.toml, Cargo.lock, and gui/app/src-tauri/Cargo.lock. Patch is the right call: behaviour-preserving reliability fix, no API change.

dig-constants check (both directions, per §4.1)

• Does anything here belong in dig-constants? No. HTTP_ATTEMPTS = 3 and HTTP_RETRY_BACKOFF = 500ms are this installer's private retry budget — no second repo must match them, nothing observes them on a wire, and they are not an asset id / port / address / puzzle hash / genesis value / trust anchor.
• Should anything here be using dig-constants? No hardcoded literal in this diff duplicates a value dig-constants publishes. USER_AGENT derives from env!("CARGO_PKG_VERSION"), and the GitHub host/paths come from release::Repo, not from new literals.

Non-gating observations (no thread needed, recorded here)

1. src/download.rs:174 — is_release_not_found substring match, as analysed in §4. Pre-existing; safe in this codebase. Worth a comment on the tracking ticket rather than a fix in this PR.
2. src/download.rs:1442 — this assertion (is_release_not_found(&err)) would also pass if an ephemeral port number happened to contain 404. It is not the load-bearing assertion in that test (seen == 1 is), so this is a cosmetic weakness, not a false green.
3. src/download.rs:99 — with_retry called with attempts == 0 would still perform one attempt (for _ in 1..0 is empty, then the trailing op()). Unreachable in production (HTTP_ATTEMPTS is a const 3; tests pass it explicitly), so a nit only.
4. src/download.rs:262 — an into_string() failure is classified retryable, which also re-fetches on ureq's ~10 MB body cap rather than failing fast. API bodies here are small; harmless.

None of these block merge. Merge remains gated on the parallel security leg returning and the PR being undrafted only after both verdicts are in.

[MichaelTaylor3d]

loop-security: PASS

Audited head: 12ea3f48bb6d4aa0e12cc285a1e28680de5c801c (resolved from gh pr view 68 --json headRefOid; unchanged during the audit). Diff = 6 files, only src/download.rs is code.

No gating security finding. This is the credential + network-trust leg; the correctness leg runs separately.

1. Credential leakage — CLEAR

The token exists in exactly one place: src/download.rs:251, the Authorization: Bearer header. It is read at :214 from GITHUB_TOKEN and is never stored, printed, or interpolated into a URL.

Every failure path was traced, including the ones that only run on retry:

• The error strings are GET {url}: {err} (:64), read {url} (:259), read body {url} (:294) and downloaded 0 bytes from {url} (:300). All carry the URL, never a header.
• ureq 2.12.1's Display for Error (src/error.rs:210) is {response_url}: status code {n} for Status, and {url}: {kind}: {message}: {source} for Transport. No request headers and no response body ever enter the message — so an attacker-controlled response body cannot reach stderr through this path either.
• HttpFailure (:37) derives nothing, so there is no {:?} leak vector, and nothing in the crate debug-prints a ureq::Request.
• The token cannot travel in a URL: the three API builders (src/release.rs:177,186,198) are https://api.github.com/... literals, and Repo::new is called only from release.rs compile-time constructors — never from CLI or env input. An interpolated path segment cannot alter the authority, which is already fixed earlier in the string.

Redirect leak checked explicitly, since that is the classic way a Bearer header escapes: ureq::get uses the default agent, whose redirect_auth_headers defaults to RedirectAuthHeaders::Never (agent.rs:263), and unit.rs:220-224 strips both authorization and cookie on every redirect. A hostile 302 off api.github.com cannot carry the token.

2. The 403 classification — CLEAR, and structurally sound

• authenticated is derived from the same binding that sets the header (:249 credential, consumed at :250 and :255), so the retry policy cannot disagree with what actually went on the wire.
• fetch_bytes_retrying passes false at :289 as a structural truth, not an assumption — that path never sets an auth header at all.
• A 403 cannot become the no-release fallback. is_release_not_found (:174) matches 404 / Not Found; a 403 message reads ... status code 403, so latest_release (:194) propagates it instead of falling back. There is also no unauthenticated-retry fallback anywhere — get_text passes the env token once and never re-issues the request anonymously, so a 403 cannot be used to strip authentication.
• Classification is on the typed ureq::Error (:57-62), never on formatted text, so a hostile body containing 404 cannot influence retry behaviour.

3. Retry as an amplifier — CLEAR

op() runs at most attempts times regardless of what the server does (:79-91): two loop iterations plus one final call, no budget reset on progress, no server-controlled counter. Backoff is 500 ms then 1000 ms — under 1.5 s added per request. The 3x request amplification lands on a hardcoded github.com / api.github.com URL, so there is no SSRF or third-party reflection. with_retry(0, ..) would run one attempt rather than zero, but both callers pass the HTTP_ATTEMPTS const and both *_retrying functions are private — unreachable, and it errs toward making a request the caller wanted rather than toward skipping a check.

4. Integrity of the artifact under retry — CLEAR

The retry cannot produce a partial, spliced, or substituted artifact: buf is declared inside the closure (:290), so each attempt allocates a fresh buffer and drops the previous one. There is no resume, no range request, and no partial-file reuse, so peak memory is one attempt's worth rather than three. verify_and_write (:556) runs once over the whole buffer after the loop returns. A truncated body is a framing error from ureq (LimitedRead, chunked and gzip all surface UnexpectedEof), which is now retried and then fails closed — strictly better than the previous hard abort, and never accepted as complete.

5. TLS — CLEAR, nothing relaxed

The diff changes no dependency (Cargo.toml moves only the version). Verified independently rather than taking the DEVELOPMENT_LOG entry on trust:

• ureq is declared default-features = false, features = ["tls", "json", "gzip"], and the only other ureq dependent in the lock, dig-release-resolver 0.2.0, requests default-features = false, features = ["tls"]. Feature unification therefore leaves native-certs, native-tls, socks-proxy, cookies and proxy-from-env all OFF.
• Roots are the compiled-in webpki-roots; rustls-native-certs appears zero times in Cargo.lock. The installer's own CA-minting and keychain mutation structurally cannot affect its own HTTPS trust — the log entry is accurate.
• proxy-from-env being off is load-bearing and worth stating explicitly: an unprivileged user cannot point an elevated install at a proxy via HTTPS_PROXY.
• No AgentBuilder, no tls_config, no danger_*, no accept_invalid_* anywhere in src/.
• Test hygiene: the new helper binds 127.0.0.1:0 (loopback, ephemeral) rather than 0.0.0.0, and the tokens in the tests are fakes. No credential enters the repo.

---

Non-gating notes (all pre-existing; recorded, not a reason to hold this PR)

1. src/lib.rs:715 + src/lib.rs:997 — a 404-substring match can silently skip an alias binary. classify_release_error maps any message containing 404 / Not Found to ASSET_NOT_FOUND, and install_optional_alias turns ASSET_NOT_FOUND into a silent skip that still reports install success. Because the message embeds the URL, a version pin whose tag contains 404 (e.g. v0.404.0) would make a genuine transport blip during alias resolution silently omit digs / dign / digd — the user-facing CLI. Narrow and self-inflicted rather than attacker-driven (no non-TLS-compromise path controls that string), and this diff reduces its reachability by retrying transient errors before any message is produced. A follow-up should classify on a typed status rather than substring text.
2. src/download.rs:291 — read_to_end has no size cap on an in-memory download in a possibly-elevated process. Pre-existing verbatim; the retry does not compound it (fresh buffer per attempt). Exploiting it requires already controlling the server.
3. No read timeout. The default agent sets timeout_connect: 30s but leaves timeout_read / timeout_write / timeout as None. The retry loop bounds the attempt count, not the wall clock — a slowloris server can still hang an install, now with 3x the worst case. The doc comment at :69 says a retry loop must not become an unbounded hang, which is true of the count and not of the duration. A timeout_read would close this.
4. src/lib.rs:888 and :1031 pass None for expected_sha256 — neither production call site checksums or signature-verifies the downloaded binary, so TLS is the sole integrity control. That is precisely why the TLS analysis above carries the weight it does. Pre-existing and outside this diff, but it is the largest integrity gap in this file's neighbourhood.
5. classify :58 treats every Transport variant as retryable, including the deterministic ones (InvalidUrl, UnknownScheme, TooManyRedirects). Wasted attempts, not a security defect; a redirect-loop server gets 15 requests instead of 5, still bounded.
6. https_only is false, so a 302 from https to http would be followed. It grants an attacker nothing they do not already have — issuing that redirect requires controlling the TLS response, at which point they can serve the bytes directly — but https_only(true) would close the plaintext-downgrade path as defense in depth.

Required checks all green by name; zero unresolved threads. PR is correctly still a draft.