diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index 851ca40c5..edbbc1f99 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -619,6 +619,9 @@ pub(super) async fn run_redirect( stage: &mut super::rollout::Stage, // Scan's pre-redirect lockfile discovery (see `rollout::Gate::prior`). prior: Option>, + // The withdrawn-pin sweep's reference answers (see + // `rollout::Gate::references`). + references: Option, // `--prune` / `--sync`, gated by the policy exactly as the human arm // gates it (`patches.enabled: false` writes nothing, the GC included): // only feeds the `redirect_prune_ignored` warning. @@ -685,7 +688,11 @@ pub(super) async fn run_redirect( &pairs, scan_result, npm_prior, - Some(super::rollout::Gate::new(stage, rows).with_prior(prior)), + Some( + super::rollout::Gate::new(stage, rows) + .with_prior(prior) + .with_references(references), + ), "scan", ) .await @@ -869,12 +876,33 @@ async fn run_redirect_selected_untracked( if !selected.is_empty() { let uuids: Vec = selected.iter().map(|(_, uuid)| uuid.clone()).collect(); - status.set(format!( - "Resolving hosted artifacts for {}...", - crate::ui::plural(uuids.len(), "patch", "patches") - )); - let fetched = api_client.fetch_registry_references(&uuids).await; - status.finish(); + // A rescan selects the uuids its lockfiles pin, which the + // withdrawn-pin sweep just asked about: ask only about the uuids it + // has no settled answer for. + let known = rollout.as_mut().and_then(|gate| gate.references.take()); + let to_fetch = match &known { + Some(known) => known.unknown(&uuids), + None => uuids.clone(), + }; + let fetched = if to_fetch.is_empty() { + Ok(std::collections::HashMap::new()) + } else { + status.set(format!( + "Resolving hosted artifacts for {}...", + crate::ui::plural(to_fetch.len(), "patch", "patches") + )); + let fetched = api_client.fetch_registry_references(&to_fetch).await; + status.finish(); + fetched + }; + // `build_candidates` looks up only the selected uuids, so the + // sweep's answers for pins outside the selection are inert. + let fetched = fetched.map(|mut fetched| { + if let Some(known) = known { + fetched.extend(known.into_answers()); + } + fetched + }); let references = match fetched { Ok(r) => r, // A capped run whose every row is NEW: the failure affects only diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index 33e1c1cf6..413869c28 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -2644,12 +2644,16 @@ async fn run_scan( // longer serves, before the rewrite below re-pins any replacement. // Runs ahead of every "nothing found" exit: a package whose only patch // was withdrawn has no search results at all. - let withdrawn_sweep = if hosted && !args.common.is_global() && policy.writes_allowed() { + let mut withdrawn_sweep = if hosted && !args.common.is_global() && policy.writes_allowed() { let pins = withdrawn::pins_in_scope(&hosted_pin_list, &all_purls); Some(withdrawn::sweep(&args.common, &api_client, &pins).await) } else { None }; + // The sweep's reference answers, reused by the hosted redirect. + let sweep_references = withdrawn_sweep + .as_mut() + .and_then(withdrawn::WithdrawnSweep::take_references); // Calculate patch counts let mut free_patches = 0usize; @@ -2782,6 +2786,7 @@ async fn run_scan( batch_error_count > 0, &mut stage, prior_discovery, + sweep_references, prune, ) .await; @@ -3255,7 +3260,11 @@ async fn run_scan( &pairs, None, npm_crawl.as_ref(), - Some(rollout::Gate::new(&mut stage, rows).with_prior(prior_discovery)), + Some( + rollout::Gate::new(&mut stage, rows) + .with_prior(prior_discovery) + .with_references(sweep_references), + ), "scan", ) .await; diff --git a/crates/socket-patch-cli/src/commands/scan/rollout.rs b/crates/socket-patch-cli/src/commands/scan/rollout.rs index 97b614369..b5c37ac6c 100644 --- a/crates/socket-patch-cli/src/commands/scan/rollout.rs +++ b/crates/socket-patch-cli/src/commands/scan/rollout.rs @@ -47,6 +47,9 @@ pub(crate) struct Gate<'a> { /// (with the configured patch-server origins): the rewrite's /// attribution gate reuses it when nothing changed the project since. pub(crate) prior: Option>, + /// The withdrawn-pin sweep's reference answers: the redirect asks the + /// endpoint only about the selected uuids these do not cover. + pub(crate) references: Option, } /// Scan's discovery, made BEFORE the apply lock, with the paths it read. @@ -76,6 +79,7 @@ impl<'a> Gate<'a> { stage, rows, prior: None, + references: None, } } @@ -85,6 +89,16 @@ impl<'a> Gate<'a> { self } + /// This gate carrying the sweep's reference answers (see + /// [`Self::references`]). + pub(crate) fn with_references( + mut self, + references: Option, + ) -> Self { + self.references = references; + self + } + /// `(purl, uuid)` of every NEW row, for O(1) [`Self::is_new`] checks. pub(crate) fn new_keys(&self) -> HashSet<(String, String)> { self.rows diff --git a/crates/socket-patch-cli/src/commands/scan/withdrawn.rs b/crates/socket-patch-cli/src/commands/scan/withdrawn.rs index c5ccc120b..045d64951 100644 --- a/crates/socket-patch-cli/src/commands/scan/withdrawn.rs +++ b/crates/socket-patch-cli/src/commands/scan/withdrawn.rs @@ -23,11 +23,17 @@ //! entitlement looks like. A `forbidden`, `pending_build` or `build_failed` //! answer keeps the pin too. If the reference lookup fails (the patch API //! is down, say), every pin is kept and the run warns. +//! +//! The settled answers are kept ([`WithdrawnSweep::take_references`]): on +//! a rescan the redirect selects the same uuids the lockfiles pin, and asks +//! the endpoint only about the ones the sweep has no answer for (#1606, +//! #1607). -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; use std::time::Duration; use socket_patch_core::api::client::ApiClient; +use socket_patch_core::api::types::PackageVendorResult; use socket_patch_core::patch::redirect::upstream::HostedPin; use socket_patch_core::utils::purl_key::PurlKey; @@ -60,9 +66,53 @@ pub(super) struct WithdrawnSweep { events: Vec, /// `(code, detail)` run-level warnings. warnings: Vec<(String, String)>, + /// The reference endpoint's answers, when the lookup succeeded. + references: Option, +} + +/// Reference answers the sweep already has, for the redirect to reuse. +/// Only settled answers are kept: a `pending_build` (or any status this +/// build does not know) may have changed by the time the redirect runs, and +/// a uuid the response left out (a 404 chunk answers nothing) is asked +/// again rather than read as `not_found`. +#[derive(Debug, Default)] +pub(crate) struct KnownReferences(HashMap); + +impl KnownReferences { + /// The settled answers in `results` for the uuids in `asked` (an + /// answer the request did not ask for is not trusted). + fn settled(asked: &[String], mut results: HashMap) -> Self { + results.retain(|uuid, r| { + asked.contains(uuid) + && matches!( + r.status.as_str(), + "granted" | "reused" | "withdrawn" | "not_found" | "forbidden" + ) + }); + KnownReferences(results) + } + + /// The uuids in `uuids` with no known answer. + pub(crate) fn unknown(&self, uuids: &[String]) -> Vec { + uuids + .iter() + .filter(|uuid| !self.0.contains_key(*uuid)) + .cloned() + .collect() + } + + /// The known answers, keyed by uuid as the reference fetch returns them. + pub(crate) fn into_answers(self) -> HashMap { + self.0 + } } impl WithdrawnSweep { + /// The reference answers the sweep fetched, for the redirect to reuse. + pub(super) fn take_references(&mut self) -> Option { + self.references.take() + } + /// Fold the outcome into the scan envelope. pub(super) fn record_into(self, env: &mut Envelope) { for event in self.events { @@ -140,6 +190,7 @@ pub(super) async fn sweep( is_gone(status).then(|| (pin.clone(), status.clone())) }) .collect(); + out.references = Some(KnownReferences::settled(&uuids, references)); if gone.is_empty() { return out; } @@ -271,4 +322,29 @@ mod tests { assert!(pins_in_scope(&pins, ["pkg:npm/b@2.0.1"]).is_empty()); assert!(pins_in_scope(&pins, Vec::::new()).is_empty()); } + + #[test] + fn only_settled_answers_are_known() { + let answer = |status: &str| -> PackageVendorResult { + serde_json::from_value(serde_json::json!({ "status": status })).unwrap() + }; + // `left-out`: the response did not answer it (a 404 chunk, say). + let uuids = ["granted", "withdrawn", "building", "failed", "left-out"].map(String::from); + let known = KnownReferences::settled( + &uuids, + HashMap::from([ + ("granted".to_string(), answer("granted")), + ("withdrawn".to_string(), answer("withdrawn")), + ("building".to_string(), answer("pending_build")), + ("failed".to_string(), answer("build_failed")), + // Not asked for: dropped. + ("unasked".to_string(), answer("granted")), + ]), + ); + assert_eq!(known.unknown(&uuids), ["building", "failed", "left-out"]); + assert_eq!(known.unknown(&["unasked".to_string()]), ["unasked"]); + let mut answers: Vec<_> = known.into_answers().into_keys().collect(); + answers.sort(); + assert_eq!(answers, ["granted", "withdrawn"]); + } } diff --git a/crates/socket-patch-cli/tests/in_process_scan_withdrawn.rs b/crates/socket-patch-cli/tests/in_process_scan_withdrawn.rs index a4629eab5..c51f21319 100644 --- a/crates/socket-patch-cli/tests/in_process_scan_withdrawn.rs +++ b/crates/socket-patch-cli/tests/in_process_scan_withdrawn.rs @@ -68,11 +68,19 @@ fn scan_args(cwd: &Path, api_url: String, dry_run: bool) -> ScanArgs { /// Run an in-process hosted scan with the npm registry pointed at /// `server`, scrubbing the env toggles `run` mirrors into the process. async fn scan(cwd: &Path, server: &MockServer, dry_run: bool) -> i32 { + scan_with(cwd, server, dry_run, true).await +} + +/// [`scan`], with `--json` on or off (off runs `--silent`). +async fn scan_with(cwd: &Path, server: &MockServer, dry_run: bool, json: bool) -> i32 { std::env::set_var( "SOCKET_NPM_REGISTRY", format!("{}/npm-registry", server.uri()), ); - let code = scan_run(scan_args(cwd, server.uri(), dry_run)).await; + let mut args = scan_args(cwd, server.uri(), dry_run); + args.common.json = json; + args.common.silent = !json; + let code = scan_run(args).await; for var in ["SOCKET_NPM_REGISTRY", "SOCKET_DRY_RUN"] { std::env::remove_var(var); } @@ -525,3 +533,117 @@ async fn the_envelope_reports_the_restored_pin() { assert_eq!(restored[0]["details"]["mode"], "hosted"); assert_eq!(lock(tmp.path()), pristine); } + +/// The uuids each reference request asked about, in request order. +async fn reference_uuids(server: &MockServer) -> Vec> { + server + .received_requests() + .await + .unwrap_or_default() + .iter() + .filter(|r| r.url.path().ends_with("/patches/package")) + .map(|r| { + let body: Value = serde_json::from_slice(&r.body).unwrap(); + let mut uuids: Vec = body["uuids"] + .as_array() + .unwrap() + .iter() + .map(|u| u.as_str().unwrap().to_string()) + .collect(); + uuids.sort(); + uuids + }) + .collect() +} + +/// A rescan of a pin whose patch is still offered asks the reference +/// endpoint about its uuid once: the redirect reuses the sweep's answer +/// instead of sending the same request again (#1606, #1607). +async fn rescan_asks_once(json: bool) { + let tmp = tempfile::tempdir().unwrap(); + let (_, wired) = wired_project(tmp.path()).await; + + let server = MockServer::start().await; + mock_search(&server, &[UUID]).await; + mock_references( + &server, + serde_json::json!({ UUID: granted(HOSTED_URL, PATCHED_SHA512) }), + ) + .await; + mock_view(&server, UUID).await; + assert_eq!(scan_with(tmp.path(), &server, false, json).await, 0); + + assert_eq!(lock(tmp.path()), wired, "the rescan keeps the pin"); + assert_eq!( + reference_uuids(&server).await, + vec![vec![UUID.to_string()]], + "one reference request for the pinned uuid, not one per phase" + ); +} + +#[tokio::test] +#[serial] +async fn a_json_rescan_asks_about_a_pinned_uuid_once() { + rescan_asks_once(true).await; +} + +#[tokio::test] +#[serial] +async fn a_human_rescan_asks_about_a_pinned_uuid_once() { + rescan_asks_once(false).await; +} + +/// A replacement patch the sweep never asked about is still looked up, and +/// only it: the withdrawn uuid's answer is reused. +#[tokio::test] +#[serial] +async fn the_redirect_asks_only_about_uuids_the_sweep_did_not() { + let tmp = tempfile::tempdir().unwrap(); + wired_project(tmp.path()).await; + + let server = MockServer::start().await; + mock_search(&server, &[UUID2]).await; + mock_references( + &server, + serde_json::json!({ + UUID: { "status": "withdrawn" }, + UUID2: granted(HOSTED_URL2, PATCHED_SHA512_2), + }), + ) + .await; + mock_view(&server, UUID2).await; + mock_npm_registry(&server).await; + assert_eq!(scan(tmp.path(), &server, false).await, 0); + + let wired = lock(tmp.path()); + assert!(wired.contains(HOSTED_URL2), "re-pinned:\n{wired}"); + assert_eq!( + reference_uuids(&server).await, + vec![vec![UUID.to_string()], vec![UUID2.to_string()]], + ); +} + +/// An answer that may change within the run (`pending_build`) is asked +/// again by the redirect instead of reused. +#[tokio::test] +#[serial] +async fn a_pending_build_answer_is_asked_again() { + let tmp = tempfile::tempdir().unwrap(); + let (_, wired) = wired_project(tmp.path()).await; + + let server = MockServer::start().await; + mock_search(&server, &[UUID]).await; + mock_references( + &server, + serde_json::json!({ UUID: { "status": "pending_build" } }), + ) + .await; + mock_view(&server, UUID).await; + assert_eq!(scan(tmp.path(), &server, false).await, 0); + + assert_eq!(lock(tmp.path()), wired, "a building patch keeps its pin"); + assert_eq!( + reference_uuids(&server).await, + vec![vec![UUID.to_string()], vec![UUID.to_string()]], + ); +}