[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1869

Summary

This is register row A27 (living document, Part 3: duplication). api/client.rs fetched a grant-tokenized serve URL through two near-copies:

• download_vendor_archive_once serves the vendor archive and the crate-internal download_artifact.
• download_artifact_capped_once serves PatchApi::download_artifact, which the hosted and in-memory engine use.

On top of those, the file hand-wrote three vendor retry loops: one around each GET and one around the package-reference POST. The two copies had drifted on the cap-breach error variant, the status text and the transport text. This PR leaves one GET and one loop.

Changes

• One serve GET. serve_get_once(url, max_bytes, noun) replaces both GETs. It returns Result<ServeDownload, VendorAttemptError>, and ServeDownload loses its Failed variant; Ok is renamed Bytes.
• One retry loop. retry_vendor(key, what, deferred, once) replaces the three loops. It also takes the deferred first attempt of a split pypi download, so download_artifact_resuming still spends one budget across the split.
• Deleted:
  • download_vendor_archive_retrying, download_vendor_archive_from, vendor_download_retry;
  • download_artifact_capped, download_artifact_capped_once, the CappedAttempt alias, and the second impl ApiClient block.
• Kept: download_vendor_archive(url, deferred) remains as the archive entry point, because three callers use it: step 2, download_artifact and download_artifact_resuming.
• Retry policy values are unchanged. I did not touch the Retry-After-over-cap disagreement (ApiRetryPolicy gives up, VendorRetryPolicy clamps to 4 s). It is a behavior change, so it belongs in a follow-up.

Tests

• cap_breach_is_not_retried now asserts the exact unified error, Other("vendor package exceeds the 268435456-byte limit"), where it used to accept any Network error containing "too large".
• every_vendor_retry_loop_… is renamed every_vendor_retry_path_…. It still covers the reference POST, the archive GET (through download_artifact), the PatchApi GET and a resumed deferral.
• New: trait_download_error_body_is_capped. A 500's JSON message shows when the body fits the caller's cap, and an over-cap body is dropped. It is red when the error body is read with uncapped resp.text().
• New: trait_download_error_body_is_capped_at_64_kib (a >64 KiB error body under a 1 MiB caller cap shows only the status) and trait_download_stalled_error_body_is_bounded_in_time (a 503 whose body stalls after the headers returns at the body timeout). Both are red without 7fdad36.
• Red→green for the merged loop. I ignored deferred in retry_vendor, and both resuming_a_deferral_spends_one_budget_not_two and every_vendor_retry_path_honors_an_http_date_retry_after failed.

Diff

api/client.rs +155 / −244: production +119 / −238 (net −119), tests +36 / −6.

Behavior change

The error text and variant are now unified, as the issue's step 1 asked:

	vendor archive / download_artifact	PatchApi::download_artifact
size-cap breach	was Network("vendor package too large: declared N bytes > M cap"), now Other("vendor package exceeds the M-byte limit")	unchanged
other status	goes through status_error: the JSON message is shown when present, and an empty body no longer leaves a trailing ": "	was artifact download failed with status N, now also carries the body's message
error body	now read under min(caller's cap, 64 KiB) and the body timeout; an oversized or stalled body is dropped and only the status is shown	now read, under the caller's cap
--debug	unchanged	now logs GET artifact <url> and artifact download attempt N failed: …

Nothing outside client.rs matches on ApiError::Network from these paths. The retry decision is unchanged because a cap breach was never retried on either path. No test outside this file pins the old text.

Verification

• cargo clippy --workspace --all-features -- -D warnings is clean.
• cargo test -p socket-patch-core --lib api::: 264 passed.
• --test api_retry_e2e --test api_timeout_e2e: 24 passed.
• cargo test -p socket-patch-core --lib: 6994 passed and 4 failed. All 4 depend on permissions and fail because the sandbox runs as root; none touches api/:
  • copy_tree::relax_loop_must_not_traverse_symlinked_root
  • vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry
  • pypi_poetry::wire_write_failure_maps_error_and_leaves_lock_untouched
  • pypi_requirements::wire_failure_rolls_back_already_written_files

/code-review high

• Fixed: the shared GET read the error body uncapped, which the old PatchApi path never did. It is now read under min(caller's cap, 64 KiB) and the body timeout.
• Not changed, by design:
  • The richer error text on the PatchApi path and the cap-breach Network → Other change are the unification api/client.rs downloads a served artifact through two near-copy GETs and hand-writes four retry loops under two retry policies #1869 asks for.
  • "A deferred attempt with attempts == 1 makes a second request": only download_artifact_first_attempt builds a DeferredAttempt, and only when attempts > 1.
  • "The archive GET test now goes through download_artifact": that runs the same download_vendor_archive call that step 2 makes.

Risk

Low to medium. The change is confined to one file and contains no platform-specific code. The only observable differences are the error texts above.

🤖 Generated with Claude Code

https://claude.ai/code/session_011f9aeQpioTouRYwdeoj9jh

---

Note

Medium Risk
Changes HTTP client retry, timeout, and error mapping for vendor and artifact downloads; behavior is mostly equivalent but error types/text and error-body handling differ observably.

Overview
Consolidates duplicated grant-tokenized serve downloads and vendor retry logic in api/client.rs into serve_get_once (one GET with configurable byte cap and error labels) and retry_vendor (shared loop for package-reference POST, archive GET, and PatchApi::download_artifact, including deferred split-download budgets).

ServeDownload no longer carries failures inline (Failed removed; success is Bytes). Callers get Result<…, (ApiError, bool)> instead. Non-OK responses now read error bodies under min(caller cap, 64 KiB) and body_timeout, so huge or stalled bodies cannot hang or blow up messages; successful reads still use the caller's cap.

Observable error tweaks: size-cap breaches use ApiError::Other with a unified "… exceeds the N-byte limit" string (was Network on the archive path); PatchApi::download_artifact can include JSON message from error bodies where the old path only reported status.

^{Reviewed by Cursor Bugbot for commit 95ceea0. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Burn-down agent: labeled Ready for review at 90d4d405.

• CI: 32/32 run checks green (50 skipped by path filters); mergeable, no conflicts.
• Bugbot: reviewed 90d4d40 with no issues; no open threads.
• Reviewer note: unifying the paths changes observable errors: archive size-cap breaches now return ApiError::Other instead of Network, and ServeDownload loses its Failed variant.
• Slack announcement could not be sent from this run; the next run will retry.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief: 90d4d40

What it does. api/client.rs had three hand-written vendor retry loops (the reference POST, the archive GET with its deferred resume, and the PatchApi capped artifact GET) and two copies of the serve GET. This PR folds them into one generic retry_vendor and one serve_get_once(url, max_bytes, noun). The file shrinks by about 90 lines.

Risk: low. I compared each old loop against the new helper. Attempt counts, which failures retry, backoff keys and Retry-After handling, the deferred attempt's numbering, timeouts and the no-auth client are all unchanged. What does change is error text: some messages, one error kind and some debug lines (listed below). Nothing outside this file branches on the error kind; the vendor outcome is only displayed, and no CLI_CONTRACT text pins these messages.

Look here

• client.rs:1627 retry_vendor: the single retry loop, including the deferred-first-attempt path
• client.rs:1673 serve_get_once:
  • the non-http refusal;
  • 404/410/408 are terminal;
  • classify_auth_error runs first;
  • a truncated body is retried.
• client.rs:1733-1761: the error body is now read capped and goes through status_error. An oversize body is now ApiError::Other("{noun} exceeds the {max_bytes}-byte limit").
• client.rs:1829 artifact_download_result: the 404 and 408 messages are byte-identical to the old capped path.

Verified

• Read the full diff (1 file, +155/−244). CHANGELOG.md is untouched.
• Ran cargo test -p socket-patch-core --lib api::: 264/264 pass, including the new trait_download_error_body_is_capped.
• Ran --test api_retry_e2e (17/17) and --test api_timeout_e2e (7/7).
• Ran --lib vendor::: 2818 pass and 2 fail. Both failures are root-only permission tests in files this PR doesn't touch.
• Retry with an HTTP-date Retry-After is still covered on all four retry paths. download_artifact_errors_never_carry_a_credential passes, and the new debug lines go through URL redaction.
• CI on the head: ci-ok and clippy are green. Bugbot: success. No open review threads.

Changes I made: none.

Open questions (non-blocking): the PR presents this as a refactor, but four user-visible changes are worth knowing about:

1. A vendor or crate download over the size cap is now ApiError::Other ("vendor package exceeds the N-byte limit") instead of Network with the declared size. It still doesn't retry.
2. Vendor non-2xx messages now go through status_error: the JSON message is extracted, there is no trailing ": " on an empty body, and the body read is now capped instead of unbounded.
3. PatchApi::download_artifact non-2xx now includes the body text. That body is read up to the caller's max_bytes without the body_timeout wrapper. A small fixed cap for error bodies, say 64 KiB, would be tighter. The old vendor path had the same gap.
4. There are new debug lines: "GET artifact " and "artifact download attempt N failed".

Auto-merge (squash) is armed, so approving sends this straight to the merge queue.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

[agent] Final-review point 3 is addressed in 7fdad36: the error body is now read under min(max_bytes, 64 KiB) and inside body_timeout. A longer or stalled body is dropped and only the status is reported. api:: tests (264) pass and clippy is clean. Points 1, 2 and 4 are the unification #1869 asked for, as listed under "Behavior change" in the description.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

Stale Bugbot comment from a previous run.

[Mikola Lysenko (mikolalysenko)]

Final reviewer: disarmed auto-merge. The approver brief covered 90d4d40, and 7fdad36 ("Bound the serve GET's error body read in time") is a code change landed after it. I am reviewing the new commit now and will re-arm auto-merge with an updated brief if nothing blocks.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

bugbot run

Final reviewer: I pushed 95ceea0, which adds two tests for 7fdad36. One sends a >64 KiB error body under a 1 MiB caller cap. The other sends a 503 whose body stalls after the headers. Before I added them, nothing covered the new 64 KiB ceiling or the timeout. A reviewer confirmed both tests fail without 7fdad36, and with it they pass in 0.3 s.

The push also updates body_timeout's doc comment and the description's "Behavior change" row. I removed Ready for review until CI and Bugbot are green on the new head. Auto-merge stays off until I post a fresh brief.

---

Generated by Claude Code

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 95ceea0. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Ready for review at 95ceea0 (burn-down agent).

• CI: 50/50 checks green on the head (31 success, 19 path-skipped), no failures.
• Bugbot: reviewed 95ceea0, no new issues; no unresolved review threads.
• Mergeable, no conflicts; no CHANGELOG.md change.

---

Generated by Claude Code