From c2dc3efa7692e6a2da79e0a9da52802ec7247aff Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Tue, 18 Aug 2026 17:44:43 -0400 Subject: [PATCH] fix(pr): decide an upload retry on the status, not on the message text `transient` matched the error's prose for `400`, `404`, `413` and friends, so a 1,404-byte image or a port number in the line abandoned an upload, while a 403 whose message did not spell the number was retried three times. It now reads jacquard's typed status off `AgentError::client_error()`. Co-Authored-By: Claude Opus 5 (1M context) --- TODO.md | 44 ++++++++--------- src/cmd/images.rs | 120 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 116 insertions(+), 48 deletions(-) diff --git a/TODO.md b/TODO.md index ecd531f..e20c999 100644 --- a/TODO.md +++ b/TODO.md @@ -950,18 +950,6 @@ `branches_exact` — because a full knot page means "or more", and `stars` is null rather than 0 since Bobbin answers 0 for a repo it never indexed -- [x] `--json` on the rest of the `auth` family: `switch`, `refresh`, `token` - and `logout`. docs/output.md already promised the flag on every command - that reads or writes something, and these four were the standing - exception — scripting a login-and-act sequence meant parsing sentences - for a DID. `switch`, `refresh` and `token` share a flattened - `WhoJson` (`did`, `handle`) so a caller reads the same two names it - already reads off an `auth status` row; `refresh` adds `session` and - `expires_at` in that command's own closed vocabulary, `token` adds - `access_token`, and `logout` reports `logged_out` and `remaining` as - arrays. Bare `auth token` is still exactly the token and a newline, - because it is piped. `login` is the one verb left out and the parse - test says why: it opens a browser and waits for a person - [x] `--json` on every other command: `auth status`, `key list`, and the whole writing surface (`pr create/resubmit/edit/close/reopen/merge/ comment`, `stack create/resubmit/merge`, `repo create/clone/ @@ -974,18 +962,28 @@ decoration off at `hyperlink::stdout_escapes_wanted`, errors left as plain text — live in `crate::term::jsonout` and in docs/output.md -- [ ] Upload retries are classified by substring-matching the error's prose. - `images.rs`'s `transient` reads any message containing `404` as a +- [x] Upload retries were classified by substring-matching the error's prose. + `images.rs`'s `transient` read any message containing `404` as a permanent client error and everything else as worth retrying, so a byte - count, a port or a CID fragment carrying those digits abandons an - upload, while a real 403 whose message does not spell them is tried - three times. That is the failure mode `exit.rs`'s module doc opens - with, applied to a retry decision. `Retry-After` is also read nowhere - in the crate, so a 429 is retried on the fixed schedule whatever the - server asked for. Wants jacquard's typed status rather than its - `Display` -- [x] `pr list` and `pr list --all` resolved their columns one round trip at - a time: one `handle_from_did_doc` per unique author DID and one + count, a port or a CID fragment carrying those digits abandoned an + upload, while a real 403 whose message did not spell them was tried + three times — `exit.rs`'s opening complaint, applied to a retry + decision. It now reads `AgentError::client_error()`'s typed status, + which covers all three shapes `process_response` produces: a status + other than 400 or 401 becomes `ClientErrorKind::Http`, a transport + failure becomes `Transport` with no status and is the one statusless + case worth retrying, and 400 and 401 are let through as the endpoint's + typed error with no `ClientError` attached, both permanent +- [ ] `Retry-After` is still read nowhere in the crate, so a 429 is retried on + the fixed doubling schedule whatever the server asked for. Not fixable + at `images.rs`'s level: the upload goes through jacquard's + `upload_blob`, and neither `ClientError` nor `AgentError` keeps the + response headers, so the value is gone before atgc sees the failure. + Either jacquard grows a header accessor, or the blob upload is + hand-rolled against `clients/http.rs` the way the other reads are — + and the second is a lot of machinery for one header +- [ ] `pr list` and `status pr` resolve their columns one round trip at a + time: one `handle_from_did_doc` per unique author DID and one `repo_name` per unique repo DID, both in plain `for` loops, so a page from fifteen authors was fifteen serial DID-document fetches, and the `--all` half worse — `repo_name` is up to three requests of its own. diff --git a/src/cmd/images.rs b/src/cmd/images.rs index 6e5730e..2f52568 100644 --- a/src/cmd/images.rs +++ b/src/cmd/images.rs @@ -338,15 +338,43 @@ const UPLOAD_BACKOFF: Duration = Duration::from_millis(500); /// must line up with the scan for the rewrite. const UPLOAD_CONCURRENCY: usize = 4; -/// Is this failure worth retrying? A 4xx is the request's own fault and a -/// second identical request earns an identical answer — except a 429, which -/// asks for exactly a pause and another try. Everything else (timeouts, -/// resets, 5xx) is the network's or the server's moment, so it is retried. -fn transient(err: &str) -> bool { - err.contains("429") - || !["400", "401", "403", "404", "413"] - .iter() - .any(|c| err.contains(c)) +/// Is this failure worth retrying? +/// +/// A 4xx is the request's own fault and a second identical request earns an +/// identical answer, except a 429, which asks for exactly a pause and another +/// try. A 5xx is the server's moment, and a failure with no HTTP response +/// behind it at all is the network's. +/// +/// Read off jacquard's typed status rather than out of the message. The +/// message is the wrong place to ask: it was matched for the substrings +/// `400`, `401`, `403`, `404`, `413` and `429`, and an error line carries a +/// byte count, a port, a CID fragment and a URL besides its status. A 1,404 +/// byte image abandoned its upload on the first hiccup, while a 403 whose +/// prose did not happen to spell the number was tried three times. That is +/// [`crate::exit`]'s opening complaint about substring-matching prose, +/// applied to a retry decision. +/// +/// The three shapes `jacquard_common::xrpc::process_response` can hand back +/// are all covered: +/// +/// - a non-success status other than 400 or 401 becomes +/// `ClientErrorKind::Http`, so the code itself decides +/// - a transport failure — connect, TLS, a stalled read — becomes +/// `ClientErrorKind::Transport` with no status, and is the one statusless +/// case worth another attempt +/// - 400 and 401 are let through to be decoded as the endpoint's own typed +/// error, so no `ClientError` is attached at all. Both are permanent: a +/// malformed request and a rejected credential do not improve by being +/// sent again. +fn transient(err: &jacquard::client::AgentError) -> bool { + use jacquard::common::error::ClientErrorKind; + match err.client_error() { + Some(client) => match client.status() { + Some(status) => status.as_u16() == 429 || status.is_server_error(), + None => matches!(client.kind(), ClientErrorKind::Transport), + }, + None => false, + } } /// One image, up to [`UPLOAD_ATTEMPTS`] times with doubling pauses between. @@ -369,7 +397,7 @@ async fn upload_one( Ok(blob) => return Ok(blob), Err(e) => { let msg = e.to_string(); - if attempt == UPLOAD_ATTEMPTS || !transient(&msg) { + if attempt == UPLOAD_ATTEMPTS || !transient(&e) { crate::logging::debug::dump_err("uploadBlob error", &e); bail!("image upload failed for {}: {msg}", img.path.display()); } @@ -837,22 +865,64 @@ mod tests { #[test] fn transient_errors_retry_and_client_errors_do_not() { use super::transient; - for retryable in [ - "connection reset by peer", - "operation timed out", - "HTTP status server error (503 Service Unavailable)", - "429 Too Many Requests", - ] { - assert!(transient(retryable), "{retryable} should be retried"); + use jacquard::client::AgentError; + use jacquard::common::error::ClientError; + + /// An `AgentError` carrying the `ClientError` a status of `code` + /// produces, which is how the upload's failures reach `transient`. + fn from_status(code: u16) -> AgentError { + let status = http::StatusCode::from_u16(code).expect("a real status"); + AgentError::new( + jacquard::client::AgentErrorKind::Client, + Some(Box::new(ClientError::from( + jacquard::common::error::HttpError { status, body: None }, + ))), + ) } - for hopeless in [ - "400 Bad Request", - "401 Unauthorized", - "403 Forbidden", - "404 Not Found", - "413 Payload Too Large", - ] { - assert!(!transient(hopeless), "{hopeless} should fail fast"); + + for retryable in [429, 500, 502, 503, 504] { + assert!(transient(&from_status(retryable)), "{retryable} retries"); + } + for hopeless in [403, 404, 413] { + assert!(!transient(&from_status(hopeless)), "{hopeless} fails fast"); + } + + // No response at all: a connect, a handshake or a stalled read. The + // one statusless case that is worth another attempt. + let transport = AgentError::new( + jacquard::client::AgentErrorKind::Client, + Some(Box::new(ClientError::transport(std::io::Error::from( + std::io::ErrorKind::ConnectionReset, + )))), + ); + assert!(transient(&transport), "a reset connection retries"); + + // 400 and 401 are decoded as the endpoint's own typed error, so no + // `ClientError` is attached. Both are permanent. + let typed = AgentError::sub_operation("upload blob", std::io::Error::other("nope")); + assert!(!transient(&typed), "a typed endpoint error fails fast"); + } + + /// The regression the typed status closes. Every one of these strings + /// carries a substring the old matcher read as a status code, and none of + /// them is one. + #[test] + fn a_status_code_hiding_in_the_prose_no_longer_decides_the_retry() { + use super::transient; + for status in [500u16, 503] { + let err = jacquard::client::AgentError::new( + jacquard::client::AgentErrorKind::Client, + Some(Box::new(jacquard::common::error::ClientError::from( + jacquard::common::error::HttpError { + status: http::StatusCode::from_u16(status).expect("a real status"), + body: Some("uploading 1404 bytes to :8413 failed".into()), + }, + ))), + ); + assert!( + transient(&err), + "{status} is retryable however the body reads" + ); } } -- 2.51.2