From e8f9099caf48f9a0cb5ab220122f9bce50d4f062 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 06:59:57 +0000 Subject: [PATCH 1/4] Start sweep of core api/ blob, date, retry, types Assisted-by: Claude Code:claude-opus-5-5 From 3dbc6707a2184120fe2559b18b6e7d9b3971e66c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 07:03:06 +0000 Subject: [PATCH 2/4] Sweep core api/ blob_fetcher, date, retry, types blob_fetcher: inline format_fetch_result_for into its one public caller, trim history and restating comments, and collapse 11 duplicate format tests into 3 that compare whole output. date: the chronological check asserted on its own loop counter; rename the test to the round trip it really checks. retry: make the client-only helpers and the retry ceiling private, inline the timeout constants (pub only for a tautology test), and cut the module doc that restated the policy field docs. types: delete 10 serialize-side and round-trip tests that the payload-deserialize tests already cover. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-core/src/api/blob_fetcher.rs | 383 +++--------------- crates/socket-patch-core/src/api/date.rs | 32 +- crates/socket-patch-core/src/api/retry.rs | 62 +-- crates/socket-patch-core/src/api/types.rs | 236 +---------- .../tests/api_timeout_e2e.rs | 18 +- 5 files changed, 101 insertions(+), 630 deletions(-) diff --git a/crates/socket-patch-core/src/api/blob_fetcher.rs b/crates/socket-patch-core/src/api/blob_fetcher.rs index 987cfe2d5..ec0e4769d 100644 --- a/crates/socket-patch-core/src/api/blob_fetcher.rs +++ b/crates/socket-patch-core/src/api/blob_fetcher.rs @@ -29,8 +29,6 @@ pub struct FetchMissingBlobsResult { /// Called with `(hash, one_based_index, total)` for each blob. pub type OnProgress = Box; -// ── Public API ──────────────────────────────────────────────────────── - /// Determine which `afterHash` blobs referenced in the manifest are /// missing from disk. /// @@ -52,17 +50,8 @@ pub async fn get_missing_blobs(manifest: &PatchManifest, blobs_path: &Path) -> H } /// Download all missing `afterHash` blobs referenced in the manifest. -/// -/// Creates the `blobs_path` directory if it does not exist. -/// -/// # Arguments -/// -/// * `manifest` – Patch manifest whose `afterHash` blobs to check. -/// * `blobs_path` – Directory where blob files are stored (one file per -/// hash). -/// * `client` – [`ApiClient`] used to fetch blobs from the server. -/// * `on_progress` – Optional callback invoked before each download with -/// `(hash, 1-based index, total)`. +/// `on_progress` is called before each download with +/// `(hash, 1-based index, total)`. pub async fn fetch_missing_blobs( manifest: &PatchManifest, blobs_path: &Path, @@ -82,12 +71,8 @@ pub async fn fetch_missing_blobs( download_entries(&hashes, blobs_path, client, on_progress).await } -/// Download specific blobs identified by their hashes. -/// -/// Useful for fetching `beforeHash` blobs during rollback, where only a -/// subset of hashes is required. -/// -/// Blobs that already exist on disk are skipped (counted in `skipped`). +/// Download the blobs `hashes` (rollback's `beforeHash` set). Blobs that +/// already exist on disk are counted in `skipped`. pub async fn fetch_blobs_by_hash( hashes: &HashSet, blobs_path: &Path, @@ -98,9 +83,6 @@ pub async fn fetch_blobs_by_hash( return FetchMissingBlobsResult::default(); } - // Filter out hashes that already exist on disk (an absent `blobs_path` - // simply means none do; the dir is created by the first successful - // write, never up front). let mut to_download: Vec = Vec::new(); let mut skipped: usize = 0; let mut results: Vec = Vec::new(); @@ -199,27 +181,19 @@ pub const PACKAGE_ARCHIVE: ArtifactNoun = ArtifactNoun { /// How many failures a fetch result lists before "... and N more". const MAX_LISTED_FAILURES: usize = 5; -/// Format a [`FetchMissingBlobsResult`] of per-file blobs as a -/// human-readable string (see [`format_fetch_result_for`]). -pub fn format_fetch_result(result: &FetchMissingBlobsResult) -> String { - format_fetch_result_for(result, BLOB) -} - -/// Format a [`FetchMissingBlobsResult`] counting `noun`s: the success -/// counts first, then the failures sorted by id (the result's order comes +/// Format a blob [`FetchMissingBlobsResult`] for humans: the success +/// counts first, then the failures sorted by hash (the result's order comes /// from a `HashSet`), at most five listed. -pub fn format_fetch_result_for(result: &FetchMissingBlobsResult, noun: ArtifactNoun) -> String { - let mut lines = format_fetch_successes(result, noun); - lines.extend(format_fetch_failures(result, noun)); +pub fn format_fetch_result(result: &FetchMissingBlobsResult) -> String { + let mut lines = format_fetch_successes(result, BLOB); + lines.extend(format_fetch_failures(result, BLOB)); if lines.is_empty() { - // `total > 0` with nothing downloaded, skipped, or failed should - // not be reachable; never emit a misleading blank string. - return format!("All {} are present locally.", noun.many); + return "All blobs are present locally.".to_string(); } lines.join("\n") } -/// The success half of [`format_fetch_result_for`] ("Downloaded 2 blobs", +/// The success half of [`format_fetch_result`] ("Downloaded 2 blobs", /// "1 blob already present locally"), for callers that route failures to /// a different stream. Empty when nothing succeeded. pub fn format_fetch_successes(result: &FetchMissingBlobsResult, noun: ArtifactNoun) -> Vec { @@ -236,7 +210,7 @@ pub fn format_fetch_successes(result: &FetchMissingBlobsResult, noun: ArtifactNo lines } -/// The failure half of [`format_fetch_result_for`]: a "Failed to download +/// The failure half of [`format_fetch_result`]: a "Failed to download /// N s" header and up to five ` - : ` lines, sorted by /// id. Empty when nothing failed. pub fn format_fetch_failures(result: &FetchMissingBlobsResult, noun: ArtifactNoun) -> Vec { @@ -354,24 +328,14 @@ fn guard_cache_entry(dest: &Path) -> std::io::Result<()> { crate::utils::containment::ensure_unlinked(root, dest, "write") } -// ── Internal helpers ────────────────────────────────────────────────── - /// Stream `body` to `dest` atomically: copy it chunk by chunk into a temp /// file in the same directory, check it against `expected_hash` (a blob's /// git-sha256 name) when given, then `rename(2)` it over `dest`. The body is -/// never held in memory whole (#571). +/// never held in memory whole. /// -/// The destinations here are *content-addressed* cache entries — -/// `blobs/`. A plain `tokio::fs::write` -/// truncates-then-writes in place, so an interrupted write (ENOSPC, crash, -/// killed process) can leave a partial file at the final path. Because the -/// "is it already downloaded?" check ([`get_missing_blobs`]) only tests -/// for presence, such a truncated file -/// is then trusted forever — its content no longer hashes to its name, yet -/// it is never re-downloaded. Staging in the same directory and renaming -/// makes the final path always either the complete bytes or absent, never a -/// torn intermediate, matching the stage+rename discipline used by the -/// patch-apply and copy-on-write write paths. +/// [`get_missing_blobs`] only tests for presence, so a torn entry left by +/// an in-place write would be trusted forever; stage+rename makes the final +/// path either the complete bytes or absent. /// /// Deliberately LIGHTER than [`crate::utils::fs::atomic_write_bytes`] (no /// file fsync, no dir fsync, `.socket-dl-` prefix): these are re-downloadable @@ -389,8 +353,8 @@ async fn stream_cache_entry_atomic( "cache entry path has no parent directory", )) })?; - // The cache directory (`.socket/blobs/`) is created - // here, by the first download, and nowhere earlier. A fetch that lands + // The cache directory (`.socket/blobs/`) is created here, by the first + // download, and nowhere earlier. A fetch that lands // nothing (all 404, offline, every hash mismatched, every body cut // short) must not leave an empty directory behind for the user to // commit, so a failure removes the directories this call created, while @@ -430,7 +394,6 @@ async fn stream_cache_entry_atomic( } .await; if result.is_err() { - // A partial stage would otherwise leak as a `.socket-dl-*` turd. let _ = tokio::fs::remove_file(&stage).await; // Deepest first; `remove_dir` only succeeds while a dir is empty. for dir in created_dirs { @@ -468,25 +431,14 @@ enum EntryError { HashMismatch(String), } -/// Compare an expected blob hash against the hash computed from the -/// downloaded bytes. -/// -/// Git object hashes are hex, and hex is case-insensitive. The content -/// hasher ([`compute_git_sha256_from_reader`]) always emits lowercase, but -/// [`ApiClient::fetch_blob`]'s validator accepts uppercase hex too — so a -/// manifest (or server) that uses uppercase would download byte-for-byte -/// correct content and then be wrongly rejected by a case-sensitive -/// comparison. Compare ignoring ASCII case to keep the two consistent. -/// -/// [`compute_git_sha256_from_reader`]: crate::hash::git_sha256::compute_git_sha256_from_reader +/// The hasher emits lowercase hex, but [`ApiClient::fetch_blob`] accepts an +/// uppercase manifest hash too, so the comparison ignores ASCII case. fn blob_hash_matches(expected: &str, actual: &str) -> bool { expected.eq_ignore_ascii_case(actual) } -/// Download the blobs `ids` sequentially, streaming each into -/// `dir/` and verifying it against its git-sha256 name (see -/// [`stream_cache_entry_atomic`]). The one download loop behind -/// [`fetch_missing_blobs`] and [`fetch_blobs_by_hash`]. +/// Download the blobs `ids` sequentially into `dir/`, each verified +/// against its git-sha256 name. async fn download_entries( ids: &[String], dir: &Path, @@ -627,256 +579,67 @@ mod tests { assert!(missing.is_empty()); } - #[test] - fn test_format_fetch_result_all_present() { - let result = FetchMissingBlobsResult { - total: 0, - downloaded: 0, - failed: 0, - skipped: 0, - results: Vec::new(), - }; - assert_eq!( - format_fetch_result(&result), - "All blobs are present locally." - ); + fn ok(id: &str) -> BlobFetchResult { + BlobFetchResult { + hash: id.to_string(), + success: true, + error: None, + } } #[test] - fn test_format_fetch_result_some_downloaded() { + fn format_fetch_result_lists_successes_then_failures() { let result = FetchMissingBlobsResult { total: 3, downloaded: 2, failed: 1, skipped: 0, results: vec![ - BlobFetchResult { - hash: "a".repeat(64), - success: true, - error: None, - }, - BlobFetchResult { - hash: "b".repeat(64), - success: true, - error: None, - }, - BlobFetchResult { - hash: "c".repeat(64), - success: false, - error: Some("Blob not found on server".to_string()), - }, + ok(&"a".repeat(64)), + ok(&"b".repeat(64)), + failure(&"c".repeat(64), "Blob not found on server"), ], }; - let output = format_fetch_result(&result); assert_eq!( - output, + format_fetch_result(&result), "Downloaded 2 blobs\nFailed to download 1 blob\n - cccccccccccc...: Blob not found on server" ); } #[test] - fn test_format_fetch_result_truncates_at_5() { - let results: Vec = (0..8) - .map(|i| BlobFetchResult { - hash: format!("{:0>64}", i), - success: false, - error: Some(format!("error {}", i)), - }) - .collect(); - - let result = FetchMissingBlobsResult { - total: 8, - downloaded: 0, - failed: 8, - skipped: 0, - results, - }; - let output = format_fetch_result(&result); - assert!(output.contains("... and 3 more")); - } - - // ── Group 8: format edge cases ─────────────────────────────────── - - #[test] - fn test_format_only_downloaded() { - let result = FetchMissingBlobsResult { - total: 3, - downloaded: 3, - failed: 0, - skipped: 0, - results: vec![ - BlobFetchResult { - hash: "a".repeat(64), - success: true, - error: None, - }, - BlobFetchResult { - hash: "b".repeat(64), - success: true, - error: None, - }, - BlobFetchResult { - hash: "c".repeat(64), - success: true, - error: None, - }, - ], - }; - let output = format_fetch_result(&result); - assert_eq!(output, "Downloaded 3 blobs"); - assert!(!output.contains("Failed")); - } - - #[test] - fn test_format_short_hash() { - let result = FetchMissingBlobsResult { - total: 1, - downloaded: 0, - failed: 1, - skipped: 0, - results: vec![BlobFetchResult { - hash: "abc".into(), - success: false, - error: Some("not found".into()), - }], - }; - let output = format_fetch_result(&result); - // Hash is < 12 chars: shown in full, with no false ellipsis. - assert_eq!(output, "Failed to download 1 blob\n - abc: not found"); - } - - #[test] - fn test_format_multibyte_hash_does_not_panic() { - // Regression: the failed-blob detail line truncated `hash` with a - // byte slice (`&r.hash[..12]`). The hash field carries arbitrary - // manifest strings (afterHash / patch uuid); when byte 12 falls - // inside a multibyte char the slice panicked ("byte index 12 is not - // a char boundary"), crashing apply/repair/rollback human output - // instead of reporting the failed download. - let hash = format!("{}→tail-of-corrupted-hash", "a".repeat(11)); - let result = FetchMissingBlobsResult { - total: 1, - downloaded: 0, - failed: 1, - skipped: 0, - results: vec![BlobFetchResult { - hash, - success: false, - error: Some("Invalid hash format".into()), - }], + fn format_fetch_result_without_failures() { + let counts = |downloaded, skipped| FetchMissingBlobsResult { + total: downloaded + skipped, + downloaded, + skipped, + ..Default::default() }; - let output = format_fetch_result(&result); - assert!(output.contains("Failed to download 1 blob\n")); - assert!( - output.contains("aaaaaaaaaaa→..."), - "12-char prefix expected: {output:?}" + assert_eq!( + format_fetch_result(&counts(0, 0)), + "All blobs are present locally." + ); + assert_eq!(format_fetch_result(&counts(3, 0)), "Downloaded 3 blobs"); + // Every blob already on disk is a line of its own, never blank. + assert_eq!( + format_fetch_result(&counts(0, 2)), + "2 blobs already present locally" + ); + assert_eq!( + format_fetch_result(&counts(1, 2)), + "Downloaded 1 blob\n2 blobs already present locally" ); } #[test] - fn test_format_error_none() { - let result = FetchMissingBlobsResult { - total: 1, - downloaded: 0, - failed: 1, - skipped: 0, - results: vec![BlobFetchResult { - hash: "d".repeat(64), - success: false, - error: None, - }], - }; - let output = format_fetch_result(&result); - assert!(output.contains("unknown error")); - } - - // ── Regression: skipped accounting in format ───────────────────── - - #[test] - fn test_format_all_skipped_is_not_blank() { - // Regression: `fetch_blobs_by_hash` can return total>0 with every - // blob already on disk (downloaded=0, failed=0, skipped=N). The - // formatter must surface that rather than returning a blank line. - let result = FetchMissingBlobsResult { - total: 2, - downloaded: 0, - failed: 0, - skipped: 2, - results: vec![ - BlobFetchResult { - hash: "a".repeat(64), - success: true, - error: None, - }, - BlobFetchResult { - hash: "b".repeat(64), - success: true, - error: None, - }, - ], - }; - let output = format_fetch_result(&result); - assert!(!output.trim().is_empty(), "must not be blank: {:?}", output); - assert_eq!(output, "2 blobs already present locally"); - assert!(!output.contains("Downloaded")); - assert!(!output.contains("Failed")); - } - - #[test] - fn test_format_downloaded_and_skipped_mix() { - let result = FetchMissingBlobsResult { - total: 3, - downloaded: 1, - failed: 0, - skipped: 2, - results: vec![ - BlobFetchResult { - hash: "a".repeat(64), - success: true, - error: None, - }, - BlobFetchResult { - hash: "b".repeat(64), - success: true, - error: None, - }, - BlobFetchResult { - hash: "c".repeat(64), - success: true, - error: None, - }, - ], - }; - let output = format_fetch_result(&result); - assert_eq!(output, "Downloaded 1 blob\n2 blobs already present locally"); - } - - // ── Regression: hash comparison is case-insensitive ────────────── - - #[test] - fn test_blob_hash_matches_is_case_insensitive() { - // Hex is case-insensitive. `compute_git_sha256_from_bytes` emits - // lowercase, but `is_valid_sha256_hex` accepts uppercase, so the - // verification must treat the two as equal (otherwise valid - // uppercase-hash content is wrongly rejected as a mismatch). - let lower = "abc123".to_string() + &"0".repeat(58); - let upper = lower.to_ascii_uppercase(); - assert!(blob_hash_matches(&upper, &lower)); - assert!(blob_hash_matches(&lower, &upper)); - assert!(blob_hash_matches(&lower, &lower)); - } - - #[test] - fn test_blob_hash_matches_rejects_genuine_mismatch() { - let a = "a".repeat(64); - let b = "b".repeat(64); - assert!(!blob_hash_matches(&a, &b)); - // Differing length is still a mismatch. - assert!(!blob_hash_matches(&a, "aa")); + fn a_failure_without_an_error_reads_unknown_error() { + let mut nameless = failure("d", ""); + nameless.error = None; + assert_eq!( + format_fetch_failures(&failed_result(vec![nameless]), BLOB), + vec!["Failed to download 1 blob", " - d: unknown error"] + ); } - // ── Atomic cache-entry write ───────────────────────────────────── - /// A [`BinaryBody`] streaming `bytes` from a local mock server. The /// server is returned so it outlives the read. async fn served_body(bytes: &[u8]) -> (wiremock::MockServer, BinaryBody) { @@ -1060,34 +823,6 @@ mod tests { ); } - #[test] - fn test_format_only_failed() { - let result = FetchMissingBlobsResult { - total: 2, - downloaded: 0, - failed: 2, - skipped: 0, - results: vec![ - BlobFetchResult { - hash: "a".repeat(64), - success: false, - error: Some("timeout".into()), - }, - BlobFetchResult { - hash: "b".repeat(64), - success: false, - error: Some("timeout".into()), - }, - ], - }; - let output = format_fetch_result(&result); - assert!(!output.contains("Downloaded")); - assert!( - output.starts_with("Failed to download 2 blobs\n"), - "{output}" - ); - } - fn failure(id: &str, error: &str) -> BlobFetchResult { BlobFetchResult { hash: id.to_string(), diff --git a/crates/socket-patch-core/src/api/date.rs b/crates/socket-patch-core/src/api/date.rs index 46de16c06..60d5493bb 100644 --- a/crates/socket-patch-core/src/api/date.rs +++ b/crates/socket-patch-core/src/api/date.rs @@ -41,10 +41,8 @@ pub fn parse_timestamp_secs(s: &str) -> Option { /// Days since 1970-01-01 for a civil (proleptic Gregorian) date. /// /// Howard Hinnant's `days_from_civil` (public domain): -/// . -/// This is the exact inverse of the `civil_from_days` half of -/// [`crate::vex::time::unix_to_ymdhms`]; the round-trip is pinned by -/// `days_from_civil_inverts_unix_to_ymdhms` below. +/// , +/// the inverse of [`crate::vex::time::unix_to_ymdhms`]'s `civil_from_days`. fn days_from_civil(year: i64, month: u32, day: u32) -> i64 { let y = if month <= 2 { year - 1 } else { year }; let era = if y >= 0 { y } else { y - 399 } / 400; @@ -90,10 +88,7 @@ fn month_from_abbrev(abbrev: &str) -> Option { /// the malformed-but-common no-zone spelling). fn parse_rfc2822(s: &str) -> Option { // Drop the optional `Fri,` day-of-week prefix. - let rest = match s.split_once(',') { - Some((_dow, rest)) => rest, - None => s, - }; + let rest = s.split_once(',').map_or(s, |(_dow, rest)| rest); let mut parts = rest.split_ascii_whitespace(); let day: u32 = parts.next()?.parse().ok()?; @@ -120,8 +115,6 @@ fn parse_rfc2822(s: &str) -> Option { fn apply_zone(base: u64, zone: &str) -> Option { let offset_secs = match zone { "GMT" | "UTC" | "UT" | "Z" | "+0000" | "-0000" => 0, - // An unrecognized token is an obsolete RFC 822 named zone, which - // RFC 2822 §4.3 says to read as `-0000` — i.e. no shift. _ => parse_numeric_offset(zone).unwrap_or_default(), }; // The parsed fields were wall-clock in `zone`; UTC is that minus the @@ -208,8 +201,6 @@ mod tests { use super::*; use crate::vex::time::unix_to_ymdhms; - // ── RFC 2822 / HTTP-date: the format production actually emits ── - /// Verbatim payloads captured from /// `GET https://patches-api.socket.dev/patch/by-package/` on /// 2026-08-04. If this shape ever stops parsing, patch ranking @@ -375,8 +366,6 @@ mod tests { } } - // ── RFC 3339 / ISO 8601: the format every in-repo fixture uses ── - #[test] fn parses_rfc3339_fixture_format() { let secs = parse_timestamp_secs("2024-01-01T00:00:00Z").unwrap(); @@ -420,8 +409,6 @@ mod tests { assert_eq!(unix_to_ymdhms(secs), (2024, 2, 29, 0, 0, 0)); } - // ── Rejection ───────────────────────────────────────────────────── - #[test] fn rejects_unparseable_input() { for s in [ @@ -483,8 +470,6 @@ mod tests { } } - // ── Cross-checks against the existing formatter ─────────────────── - /// `days_from_civil` must invert the `civil_from_days` half of /// `vex::time::unix_to_ymdhms` exactly. Swept across ~1265 years so /// every leap rule and century boundary is covered. @@ -500,17 +485,16 @@ mod tests { } } - /// Parsing must be monotonic: a later instant always yields a larger - /// epoch value, in both wire formats. Oracle-free guard against a - /// scrambled field or a dropped carry. + /// Every instant `unix_to_ymdhms` formats, in either wire format, + /// parses back to exactly that instant: a scrambled field or a dropped + /// carry shows up as a mismatch. #[test] - fn parsed_order_is_chronological_in_both_formats() { + fn formatted_instants_parse_back_exactly_in_both_formats() { const MONTHS: [&str; 12] = [ "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", ]; const STRIDE: u64 = 147_853; // ~1.71 days let mut secs = 0u64; - let mut prev_2822 = 0u64; while secs < 1_900_000_000 { let (y, m, d, h, mi, s) = unix_to_ymdhms(secs); let iso = format!("{y:04}-{m:02}-{d:02}T{h:02}:{mi:02}:{s:02}Z"); @@ -520,8 +504,6 @@ mod tests { ); assert_eq!(parse_timestamp_secs(&iso), Some(secs), "iso={iso}"); assert_eq!(parse_timestamp_secs(&rfc), Some(secs), "rfc={rfc}"); - assert!(secs > prev_2822 || secs == 0); - prev_2822 = secs; secs += STRIDE; } } diff --git a/crates/socket-patch-core/src/api/retry.rs b/crates/socket-patch-core/src/api/retry.rs index 84af87241..4caf6549c 100644 --- a/crates/socket-patch-core/src/api/retry.rs +++ b/crates/socket-patch-core/src/api/retry.rs @@ -3,29 +3,11 @@ //! `scan` keeps up to 32 patch-API requests in flight, so a throttling //! server (or a proxy / WAF in front of it) answering `429 Too Many //! Requests` or `503 Service Unavailable` is expected, not exotic. Each -//! such answer is retried a bounded number of times before it becomes the -//! request's error: -//! -//! - at most [`ApiRetryPolicy::max_retries`] retries per request (3 by -//! default, [`API_MAX_RETRIES_ENV`] overrides, `0` disables); -//! - the wait honors the server's `Retry-After` (delta-seconds or an -//! HTTP-date). One longer than [`ApiRetryPolicy::max_retry_after`] -//! (30 s) is not waited out at all: the answer is final at once (retrying -//! early would only be refused again). One shorter than the jittered -//! first backoff step (`Retry-After: 0`, a date already past) waits that -//! step instead, so a server saying "now" is not hammered; -//! - without a `Retry-After` the wait is exponential from -//! [`ApiRetryPolicy::base_delay`] (500 ms, 1 s, 2 s, ... capped at -//! [`ApiRetryPolicy::max_backoff`]) with "equal jitter" — each wait lands -//! in `[d/2, d)`, the sample derived deterministically from a seed, the -//! request and the retry number; -//! - all retries share one run-wide WALL-CLOCK window -//! ([`ApiRetryPolicy::retry_window`], 60 s) that opens with the run's -//! first retry: a retry whose wait would end after the window closes is -//! refused and the answer is final. Concurrent requests wait in parallel, -//! so 32 throttled requests each still get their retries (their waits -//! overlap rather than add up), while a throttled run as a whole adds at -//! most about the window's length of waiting. +//! such answer is retried under [`ApiRetryPolicy`]: the server's +//! `Retry-After` is honored up to a cap, otherwise the wait is a jittered +//! exponential backoff. All retries share one run-wide wall-clock window, +//! so concurrent requests wait in parallel while a throttled run as a whole +//! adds at most about the window's length of waiting. //! //! A connection the peer drops while it is being established (reset, //! aborted, or closed mid-TLS-handshake — [`is_retryable_transport`]) is @@ -36,7 +18,7 @@ //! Nothing else is retried here: other 4xx (401/403 keep driving the proxy //! fallback), other 5xx and every other transport error (refused, timed //! out, DNS, TLS certificate, or a failure after the request went out) -//! surface on the first answer, exactly as before. Retries happen inside +//! surface on the first answer. Retries happen inside //! each request's future, so the callers' ordered folds //! (`ordered_concurrent`) see the same sequence of results a clean run //! produces. @@ -50,12 +32,11 @@ use reqwest::header::{HeaderMap, RETRY_AFTER}; use reqwest::StatusCode; /// Env override for [`ApiRetryPolicy::max_retries`]: a non-negative -/// integer, clamped to [`MAX_RETRIES_CEILING`]; `0` turns retries off. An -/// unset, empty or unparsable value keeps the default. +/// integer, clamped to 10; `0` turns retries off. An unset, empty or +/// unparsable value keeps the default. pub const API_MAX_RETRIES_ENV: &str = "SOCKET_API_MAX_RETRIES"; -/// The most retries [`API_MAX_RETRIES_ENV`] can ask for per request. -pub const MAX_RETRIES_CEILING: u32 = 10; +const MAX_RETRIES_CEILING: u32 = 10; /// Retry policy for the patch API's JSON calls (see the module docs). #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -87,8 +68,7 @@ impl Default for ApiRetryPolicy { } impl ApiRetryPolicy { - /// No retries: every 429 / 503 is final on the first answer (the - /// behavior before retries existed). + /// No retries: every 429 / 503 is final on the first answer. pub fn none() -> Self { Self { max_retries: 0, @@ -135,13 +115,6 @@ impl ApiRetryPolicy { } } -/// Default [`ApiTimeouts::connect`]: TCP connect plus the TLS handshake. -pub const API_CONNECT_TIMEOUT: Duration = Duration::from_secs(10); - -/// Default [`ApiTimeouts::read`]: the longest silence on an open -/// connection (waiting for the response headers, or between body chunks). -pub const API_READ_TIMEOUT: Duration = Duration::from_secs(60); - /// Transport bounds for every patch-API request: the JSON calls, the /// blob/diff downloads and the vendoring service, on both of /// [`crate::api::client::ApiClient`]'s HTTP clients. @@ -155,17 +128,18 @@ pub const API_READ_TIMEOUT: Duration = Duration::from_secs(60); /// on top. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct ApiTimeouts { - /// Bound on establishing a connection. + /// Bound on TCP connect plus the TLS handshake. pub connect: Duration, - /// Bound on one read waiting for data. + /// The longest silence on an open connection (waiting for the response + /// headers, or between body chunks). pub read: Duration, } impl Default for ApiTimeouts { fn default() -> Self { Self { - connect: API_CONNECT_TIMEOUT, - read: API_READ_TIMEOUT, + connect: Duration::from_secs(10), + read: Duration::from_secs(60), } } } @@ -187,7 +161,7 @@ fn max_retries_override(raw: Option<&str>) -> Option { } /// Is this answer one the retry loop may repeat? (429 and 503 only.) -pub fn is_retryable_status(status: StatusCode) -> bool { +pub(crate) fn is_retryable_status(status: StatusCode) -> bool { matches!( status, StatusCode::TOO_MANY_REQUESTS | StatusCode::SERVICE_UNAVAILABLE @@ -203,7 +177,7 @@ pub fn is_retryable_status(status: StatusCode) -> bool { /// failing fast), a connect or read timeout (a stall is not repeated, see /// [`ApiTimeouts`]), DNS and certificate errors, and any failure after the /// request went out. -pub fn is_retryable_transport(error: &reqwest::Error) -> bool { +pub(crate) fn is_retryable_transport(error: &reqwest::Error) -> bool { error.is_connect() && !error.is_timeout() && is_dropped_connection(error) } @@ -239,7 +213,7 @@ fn is_dropped_connection(error: &(dyn std::error::Error + 'static)) -> bool { /// (`Retry-After: 7`) or an HTTP-date (`Retry-After: Fri, 27 Mar 2026 /// 19:12:42 GMT`; a date already past waits zero). `None` when absent or /// unreadable — the caller then backs off exponentially. -pub fn parse_retry_after(headers: &HeaderMap, now_unix_secs: u64) -> Option { +pub(crate) fn parse_retry_after(headers: &HeaderMap, now_unix_secs: u64) -> Option { let raw = headers.get(RETRY_AFTER)?.to_str().ok()?.trim(); if raw.bytes().all(|b| b.is_ascii_digit()) { return raw.parse::().ok().map(Duration::from_secs); diff --git a/crates/socket-patch-core/src/api/types.rs b/crates/socket-patch-core/src/api/types.rs index 7b3378a47..727e8405e 100644 --- a/crates/socket-patch-core/src/api/types.rs +++ b/crates/socket-patch-core/src/api/types.rs @@ -190,162 +190,8 @@ pub struct PackageVendorArtifact { mod tests { use super::*; - #[test] - fn test_patch_response_camel_case() { - let pr = PatchResponse { - uuid: "u1".into(), - purl: "pkg:npm/x@1".into(), - published_at: "2024-01-01".into(), - files: HashMap::new(), - vulnerabilities: HashMap::new(), - description: "desc".into(), - license: "MIT".into(), - tier: "free".into(), - }; - let json = serde_json::to_string(&pr).unwrap(); - assert!(json.contains("publishedAt")); - assert!(!json.contains("published_at")); - } - - #[test] - fn test_patch_response_deserialize() { - let json = r#"{ - "uuid": "u1", - "purl": "pkg:npm/x@1", - "publishedAt": "2024-01-01", - "files": {}, - "vulnerabilities": {}, - "description": "A patch", - "license": "MIT", - "tier": "free" - }"#; - let pr: PatchResponse = serde_json::from_str(json).unwrap(); - assert_eq!(pr.uuid, "u1"); - assert_eq!(pr.published_at, "2024-01-01"); - } - - #[test] - fn test_patch_file_response_optional_fields() { - let pfr = PatchFileResponse { - before_hash: None, - after_hash: None, - socket_blob: None, - blob_content: None, - before_blob_content: None, - }; - let json = serde_json::to_string(&pfr).unwrap(); - let back: PatchFileResponse = serde_json::from_str(&json).unwrap(); - assert!(back.before_hash.is_none()); - assert!(back.after_hash.is_none()); - assert!(back.socket_blob.is_none()); - assert!(back.blob_content.is_none()); - assert!(back.before_blob_content.is_none()); - // Verify camelCase field names - assert!(json.contains("beforeHash")); - assert!(json.contains("afterHash")); - assert!(json.contains("socketBlob")); - assert!(json.contains("blobContent")); - assert!(json.contains("beforeBlobContent")); - } - - #[test] - fn test_search_response_camel_case() { - let sr = SearchResponse { - patches: Vec::new(), - can_access_paid_patches: true, - }; - let json = serde_json::to_string(&sr).unwrap(); - assert!(json.contains("canAccessPaidPatches")); - assert!(!json.contains("can_access_paid_patches")); - } - - #[test] - fn test_batch_search_response_roundtrip() { - let bsr = BatchSearchResponse { - packages: vec![BatchPackagePatches { - purl: "pkg:npm/x@1".into(), - patches: vec![BatchPatchInfo { - uuid: "u1".into(), - purl: "pkg:npm/x@1".into(), - tier: "free".into(), - cve_ids: vec!["CVE-2024-0001".into()], - ghsa_ids: vec!["GHSA-1111-2222-3333".into()], - severity: Some("high".into()), - title: "Test".into(), - published_at: None, - }], - }], - can_access_paid_patches: false, - }; - let json = serde_json::to_string(&bsr).unwrap(); - let back: BatchSearchResponse = serde_json::from_str(&json).unwrap(); - assert_eq!(back.packages.len(), 1); - assert_eq!(back.packages[0].patches.len(), 1); - assert!(!back.can_access_paid_patches); - } - - #[test] - fn test_batch_patch_info_camel_case() { - let bpi = BatchPatchInfo { - uuid: "u1".into(), - purl: "pkg:npm/x@1".into(), - tier: "free".into(), - cve_ids: vec!["CVE-2024-0001".into()], - ghsa_ids: vec!["GHSA-1111-2222-3333".into()], - severity: Some("high".into()), - title: "Test".into(), - published_at: None, - }; - let json = serde_json::to_string(&bpi).unwrap(); - assert!(json.contains("cveIds")); - assert!(json.contains("ghsaIds")); - assert!(!json.contains("cve_ids")); - assert!(!json.contains("ghsa_ids")); - } - - #[test] - fn test_vulnerability_response_no_rename() { - // VulnerabilityResponse does NOT have rename_all, so fields are snake_case - let vr = VulnerabilityResponse { - cves: vec!["CVE-2024-0001".into()], - summary: "Test".into(), - severity: "high".into(), - description: "A vulnerability".into(), - }; - let json = serde_json::to_string(&vr).unwrap(); - // Without rename_all, field names stay as-is (already lowercase single-word) - assert!(json.contains("\"cves\"")); - assert!(json.contains("\"summary\"")); - assert!(json.contains("\"severity\"")); - assert!(json.contains("\"description\"")); - } - - #[test] - fn test_patch_search_result_roundtrip() { - let psr = PatchSearchResult { - uuid: "u1".into(), - purl: "pkg:npm/test@1.0.0".into(), - published_at: "2024-06-15".into(), - description: "A test patch".into(), - license: "MIT".into(), - tier: "free".into(), - vulnerabilities: HashMap::new(), - }; - let json = serde_json::to_string(&psr).unwrap(); - let back: PatchSearchResult = serde_json::from_str(&json).unwrap(); - assert_eq!(back.uuid, "u1"); - assert_eq!(back.published_at, "2024-06-15"); - assert!(json.contains("publishedAt")); - } - - // ── Regression: deserialize from realistic, server-shaped payloads ── - // - // The structs above are pure serde DTOs, so the only thing that can - // break is the JSON field-name contract with the Socket API. The tests - // below pin that contract by deserializing payloads in the *exact* - // camelCase shape the live endpoints emit (mirroring the integration - // fixtures under crates/socket-patch-cli/tests). A dropped or mistyped - // `rename_all` / field rename would fail these. + // The structs are serde DTOs: these pin the JSON field-name contract by + // deserializing payloads in the exact shape the live endpoints emit. #[test] fn test_patch_response_full_view_payload_deserialize() { @@ -404,10 +250,7 @@ mod tests { #[test] fn test_patch_file_response_absent_optional_keys_are_none() { - // serde treats absent Option fields as None. The existing optional- - // fields test only round-trips explicit `null`s; this pins the - // (distinct) absent-key path the server actually uses when a blob - // isn't inlined. + // The server omits the keys when a blob isn't inlined. let pfr: PatchFileResponse = serde_json::from_str("{}").unwrap(); assert!(pfr.before_hash.is_none()); assert!(pfr.after_hash.is_none()); @@ -512,31 +355,9 @@ mod tests { ); } - #[test] - fn test_batch_package_patches_field_names() { - // BatchPackagePatches deliberately has no rename_all (both fields are - // single lowercase words). Pin the on-the-wire key names in both - // directions so an accidental rename can't silently break the - // batch-endpoint contract. - let bpp = BatchPackagePatches { - purl: "pkg:npm/x@1.0.0".into(), - patches: Vec::new(), - }; - let json = serde_json::to_string(&bpp).unwrap(); - assert!(json.contains("\"purl\"")); - assert!(json.contains("\"patches\"")); - - let back: BatchPackagePatches = - serde_json::from_str(r#"{"purl":"pkg:npm/y@2","patches":[]}"#).unwrap(); - assert_eq!(back.purl, "pkg:npm/y@2"); - assert!(back.patches.is_empty()); - } - #[test] fn test_vulnerability_response_deserialize_standalone() { - // VulnerabilityResponse has no rename_all; confirm it deserializes - // from the snake/lowercase keys the API emits (the existing test only - // exercised the serialize direction in isolation). + // VulnerabilityResponse has no rename_all. let json = r#"{ "cves": ["CVE-2024-0001", "CVE-2024-0002"], "summary": "Prototype pollution", @@ -550,31 +371,6 @@ mod tests { assert_eq!(vr.description, "A prototype pollution vulnerability"); } - #[test] - fn test_search_response_populated_roundtrip() { - // The existing camelCase test round-trips an *empty* patches vec; this - // pins a populated PatchSearchResult survives a full serialize -> - // deserialize cycle inside its SearchResponse envelope. - let sr = SearchResponse { - patches: vec![PatchSearchResult { - uuid: "u1".into(), - purl: "pkg:npm/test@1.0.0".into(), - published_at: "2024-06-15T00:00:00Z".into(), - description: "A test patch".into(), - license: "MIT".into(), - tier: "free".into(), - vulnerabilities: HashMap::new(), - }], - can_access_paid_patches: true, - }; - let json = serde_json::to_string(&sr).unwrap(); - let back: SearchResponse = serde_json::from_str(&json).unwrap(); - assert_eq!(back.patches.len(), 1); - assert_eq!(back.patches[0].uuid, "u1"); - assert_eq!(back.patches[0].published_at, "2024-06-15T00:00:00Z"); - assert!(back.can_access_paid_patches); - } - #[test] fn test_search_response_api_payload_deserialize() { // Mirrors GET /v0/orgs//patches/by-package/. @@ -597,17 +393,11 @@ mod tests { } #[test] - fn published_at_is_per_patch_not_per_package() { + fn deserializes_a_live_by_package_payload() { // Verbatim production payload for // `GET /patch/by-package/pkg%3Anpm%2Faxios%401.6.0`, captured - // 2026-08-04 (description/summary bodies elided). - // - // The point: ONE package version, TWO patches, TWO different - // `publishedAt` values. That is only possible if the field - // describes the patch. If it ever described the package release - // (axios@1.6.0 shipped 2023-10-26) both entries would carry the - // same date, and `api::ranking`'s recency rung would silently - // collapse into the UUID tiebreak. + // 2026-08-04 (description/summary bodies elided): RFC 2822 dates + // and uppercase severities. let json = r#"{ "canAccessPaidPatches": false, "patches": [ @@ -643,18 +433,8 @@ mod tests { }"#; let sr: SearchResponse = serde_json::from_str(json).unwrap(); assert_eq!(sr.patches.len(), 2); - assert_eq!( - sr.patches[0].purl, sr.patches[1].purl, - "fixture must be two patches for the SAME package version" - ); - assert_ne!( - sr.patches[0].published_at, sr.patches[1].published_at, - "publishedAt must vary per patch, not per package" - ); - assert_eq!(sr.patches[0].published_at, "Fri, 27 Mar 2026 19:12:42 GMT"); + assert_eq(sr.patches[0].published_at, "Fri, 27 Mar 2026 19:12:42 GMT"); assert_eq!(sr.patches[1].published_at, "Mon, 03 Aug 2026 20:23:06 GMT"); - // Production spells severity in uppercase; every ladder in the - // workspace lowercases before matching. assert_eq!( sr.patches[0].vulnerabilities["GHSA-4hjh-wcwx-xvwj"].severity, "HIGH" diff --git a/crates/socket-patch-core/tests/api_timeout_e2e.rs b/crates/socket-patch-core/tests/api_timeout_e2e.rs index f6d4af53a..0cb13655c 100644 --- a/crates/socket-patch-core/tests/api_timeout_e2e.rs +++ b/crates/socket-patch-core/tests/api_timeout_e2e.rs @@ -12,9 +12,7 @@ use std::time::{Duration, Instant}; use socket_patch_core::api::client::{ApiClient, ApiClientOptions, ApiError, BinaryBody}; -use socket_patch_core::api::retry::{ - ApiRetryPolicy, ApiTimeouts, RetryHooks, API_CONNECT_TIMEOUT, API_READ_TIMEOUT, -}; +use socket_patch_core::api::retry::{ApiRetryPolicy, ApiTimeouts, RetryHooks}; use tokio::io::{AsyncReadExt as _, AsyncWriteExt as _}; use tokio::net::TcpListener; @@ -321,10 +319,12 @@ async fn a_body_that_keeps_streaming_past_the_bound_still_arrives() { } #[test] -fn default_bounds_are_the_named_constants() { - let t = ApiTimeouts::default(); - assert_eq!(t.connect, API_CONNECT_TIMEOUT); - assert_eq!(t.read, API_READ_TIMEOUT); - assert_eq!(API_CONNECT_TIMEOUT, Duration::from_secs(10)); - assert_eq!(API_READ_TIMEOUT, Duration::from_secs(60)); +fn default_bounds_are_10s_connect_60s_read() { + assert_eq!( + ApiTimeouts::default(), + ApiTimeouts { + connect: Duration::from_secs(10), + read: Duration::from_secs(60), + } + ); } From 904c10af80e34700a12503ea11400e952924b2c9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 07:13:45 +0000 Subject: [PATCH 3/4] Fix a missing macro bang in the types test Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-core/src/api/types.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/socket-patch-core/src/api/types.rs b/crates/socket-patch-core/src/api/types.rs index 727e8405e..12cb718ea 100644 --- a/crates/socket-patch-core/src/api/types.rs +++ b/crates/socket-patch-core/src/api/types.rs @@ -433,7 +433,7 @@ mod tests { }"#; let sr: SearchResponse = serde_json::from_str(json).unwrap(); assert_eq!(sr.patches.len(), 2); - assert_eq(sr.patches[0].published_at, "Fri, 27 Mar 2026 19:12:42 GMT"); + assert_eq!(sr.patches[0].published_at, "Fri, 27 Mar 2026 19:12:42 GMT"); assert_eq!(sr.patches[1].published_at, "Mon, 03 Aug 2026 20:23:06 GMT"); assert_eq!( sr.patches[0].vulnerabilities["GHSA-4hjh-wcwx-xvwj"].severity, From 0b99f82a77f510751288620cde69b277d939e0bb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 07:26:22 +0000 Subject: [PATCH 4/4] Pin the batch packages' serialized keys scan --json writes BatchPackagePatches verbatim, so its serialized keys are output: compare the whole value instead of the deleted .contains checks. Also unlink a now-private item from the public module doc and pin the documented retry ceiling of 10. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-core/src/api/blob_fetcher.rs | 7 ++-- crates/socket-patch-core/src/api/retry.rs | 8 ++--- crates/socket-patch-core/src/api/types.rs | 34 +++++++++++++++++++ 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/crates/socket-patch-core/src/api/blob_fetcher.rs b/crates/socket-patch-core/src/api/blob_fetcher.rs index ec0e4769d..6cf67cb63 100644 --- a/crates/socket-patch-core/src/api/blob_fetcher.rs +++ b/crates/socket-patch-core/src/api/blob_fetcher.rs @@ -632,8 +632,11 @@ mod tests { #[test] fn a_failure_without_an_error_reads_unknown_error() { - let mut nameless = failure("d", ""); - nameless.error = None; + let nameless = BlobFetchResult { + hash: "d".into(), + success: false, + error: None, + }; assert_eq!( format_fetch_failures(&failed_result(vec![nameless]), BLOB), vec!["Failed to download 1 blob", " - d: unknown error"] diff --git a/crates/socket-patch-core/src/api/retry.rs b/crates/socket-patch-core/src/api/retry.rs index 4caf6549c..daf0fb5e9 100644 --- a/crates/socket-patch-core/src/api/retry.rs +++ b/crates/socket-patch-core/src/api/retry.rs @@ -10,7 +10,7 @@ //! adds at most about the window's length of waiting. //! //! A connection the peer drops while it is being established (reset, -//! aborted, or closed mid-TLS-handshake — [`is_retryable_transport`]) is +//! aborted, or closed mid-TLS-handshake — `is_retryable_transport`) is //! retried under the same budget and window, with the exponential backoff: //! the request was never sent, and a load balancer or proxy resetting a //! fresh connection is a transient blip, not an answer. @@ -463,10 +463,8 @@ mod tests { assert_eq!(max_retries_override(Some("-1")), None); assert_eq!(max_retries_override(Some("0")), Some(0)); assert_eq!(max_retries_override(Some(" 5 ")), Some(5)); - assert_eq!( - max_retries_override(Some("1000")), - Some(MAX_RETRIES_CEILING) - ); + // The ceiling `API_MAX_RETRIES_ENV`'s doc promises. + assert_eq!(max_retries_override(Some("1000")), Some(10)); } /// A hand-driven monotonic clock (milliseconds). diff --git a/crates/socket-patch-core/src/api/types.rs b/crates/socket-patch-core/src/api/types.rs index 12cb718ea..307d743c5 100644 --- a/crates/socket-patch-core/src/api/types.rs +++ b/crates/socket-patch-core/src/api/types.rs @@ -300,6 +300,40 @@ mod tests { assert!(patches[1].cve_ids.is_empty()); } + /// `scan --json` serializes the batch packages verbatim, so their + /// serialized keys are output. + #[test] + fn batch_packages_serialize_with_the_wire_keys() { + let package = BatchPackagePatches { + purl: "pkg:npm/x@1.0.0".into(), + patches: vec![BatchPatchInfo { + uuid: "u1".into(), + purl: "pkg:npm/x@1.0.0".into(), + tier: "free".into(), + cve_ids: vec!["CVE-2024-0001".into()], + ghsa_ids: vec!["GHSA-1111-2222-3333".into()], + severity: None, + title: "Patch one".into(), + published_at: None, + }], + }; + assert_eq!( + serde_json::to_value(&package).unwrap(), + serde_json::json!({ + "purl": "pkg:npm/x@1.0.0", + "patches": [{ + "uuid": "u1", + "purl": "pkg:npm/x@1.0.0", + "tier": "free", + "cveIds": ["CVE-2024-0001"], + "ghsaIds": ["GHSA-1111-2222-3333"], + "severity": null, + "title": "Patch one" + }] + }) + ); + } + #[test] fn test_organizations_response_deserialize() { // Mirrors GET /v0/organizations: an object keyed by org id. `name`