From 52dacecf170c24e9839f8a4b7a09eeb6d2698f1f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 08:23:19 +0000 Subject: [PATCH 1/8] Start fix for #1423 Assisted-by: Claude Code:claude-opus-5-5 From d9cdf55d63237b7b3a61ec09b71e534dc5cd00bd Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 08:32:08 +0000 Subject: [PATCH 2/8] Read yarn.lock once when picking classic tarballs A hosted scan of a yarn classic project re-read the whole yarn.lock once per patched package to decide which served tarballs it must fetch. On a 3000-package lock with 60 patches that made hosted scans about 2.6x slower than before #1328, even on a rescan that fetches nothing. The lock's registry copies are now collected in one pass, and the cheap "lock already pins this artifact" test runs first, so a rescan of a fully pinned lock never reads the blocks at all. The selected tarballs are unchanged. Fixes #1423 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-core/src/hosted/engine.rs | 125 ++++++++++++++---- 1 file changed, 100 insertions(+), 25 deletions(-) diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 88a7fdecd..95b49f731 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -19,7 +19,7 @@ //! network: every host effect (locking, probes, record fetches, the commit //! of the rewritten files) stays with the caller. -use std::collections::{BTreeMap, BTreeSet, HashMap}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use serde::{Deserialize, Serialize}; @@ -1212,40 +1212,47 @@ pub fn yarn_classic_artifact_targets<'a>( if crate::patch::redirect::yarn_classic_hosted_refused(files, yarn_outer) { return Vec::new(); } + // The pinned-artifact test is a substring search; the registry-copy + // test reads every block, so it runs once per lock, after it, and only + // when some dep still needs it (#1423). + let mut registry_copies: Option> = None; let mut seen = BTreeSet::new(); candidates .iter() .map(|c| &c.dep) .filter(|dep| dep.ecosystem == "npm" && dep.integrity.sha512.is_some()) .filter(|dep| { - classic_locks_registry_copy(lock, &crate::patch::redirect::full_name(dep), &dep.version) + dep.integrity.sha1.is_none() || !lock.contains(&format!("\"{}#", dep.artifact_url)) }) .filter(|dep| { - dep.integrity.sha1.is_none() || !lock.contains(&format!("\"{}#", dep.artifact_url)) + registry_copies + .get_or_insert_with(|| classic_registry_copies(lock)) + .contains(&(crate::patch::redirect::full_name(dep), dep.version.clone())) }) .filter(|dep| seen.insert(dep.artifact_url.clone())) .collect() } -/// Whether a classic `lock` has a registry block of `name@version`: the -/// only copy a hosted pin rewrites (a git, `file:`, `link:` or remote +/// The `(name, version)` of every registry block in a classic `lock`: the +/// only copies a hosted pin rewrites (a git, `file:`, `link:` or remote /// tarball copy is skipped by name, so it needs no served tarball). Read /// by the blocks' real names, as the rewriter does, so `lodash` never /// matches a `lodash.debounce` block. -fn classic_locks_registry_copy(lock: &str, name: &str, version: &str) -> bool { +fn classic_registry_copies(lock: &str) -> HashSet<(String, String)> { use crate::formats::yarn::blocks::{classic_field, scan_blocks}; use crate::formats::yarn::patterns::{classic_key_real_name, split_key_patterns}; use crate::formats::yarn::source::{classic_copy_source, CopySource}; - if !lock.contains(name) { - return false; - } - scan_blocks(lock).iter().any(|block| { - let patterns = split_key_patterns(&block.key); - classic_key_real_name(&patterns) == Some(name) - && classic_field(&block.lines, "version") == Some(version) - && classic_copy_source(&patterns, classic_field(&block.lines, "resolved")) - == CopySource::Registry - }) + scan_blocks(lock) + .iter() + .filter_map(|block| { + let patterns = split_key_patterns(&block.key); + let name = classic_key_real_name(&patterns)?; + let version = classic_field(&block.lines, "version")?; + (classic_copy_source(&patterns, classic_field(&block.lines, "resolved")) + == CopySource::Registry) + .then(|| (name.to_string(), version.to_string())) + }) + .collect() } /// Record what the served tarball at `url` yielded: its sha1 on every @@ -3088,15 +3095,83 @@ mod tests { resolved \"git+https://github.com/x/left-pad.git#abc\"\n\n\ is-odd@^3.0.0:\n version \"3.0.1\"\n \ resolved \"https://registry.yarnpkg.com/is-odd/-/is-odd-3.0.1.tgz#bb\"\n"; - assert!(!classic_locks_registry_copy(lock, "lodash", "4.17.21")); - assert!(!classic_locks_registry_copy(lock, "left-pad", "1.3.0")); - assert!(!classic_locks_registry_copy(lock, "is-odd", "3.0.0")); - assert!(classic_locks_registry_copy(lock, "is-odd", "3.0.1")); - assert!(classic_locks_registry_copy( - lock, - "lodash.debounce", - "4.17.21" - )); + let copies = classic_registry_copies(lock); + let has = |name: &str, version: &str| copies.contains(&(name.into(), version.into())); + assert!(!has("lodash", "4.17.21")); + assert!(!has("left-pad", "1.3.0")); + assert!(!has("is-odd", "3.0.0")); + assert!(has("is-odd", "3.0.1")); + assert!(has("lodash.debounce", "4.17.21")); + assert_eq!(copies.len(), 2); + } + + fn classic_candidate(name: &str, version: &str, sha1: Option<&str>) -> Candidate { + let url = format!("https://h/serve/{GRANT}/{name}-{version}.tgz"); + let mut dep = grant_dep("npm", url); + dep.name = name.into(); + dep.version = version.into(); + dep.integrity.sha512 = Some("sha512-x".into()); + dep.integrity.sha1 = sha1.map(Into::into); + Candidate { + purl: format!("pkg:npm/{name}@{version}"), + dep, + } + } + + /// #1423: the served tarball is read only for a dep the lock locks as + /// a registry copy and doesn't pin to the artifact yet; a big lock + /// whose deps are all pinned already (a rescan) selects none, and many + /// deps over a big lock stay cheap (the lock is read once, not once + /// per dep). + #[test] + fn classic_artifact_targets_select_unpinned_registry_copies() { + let mut lock = String::from("# yarn lockfile v1\n\n"); + let n = 3000; + for i in 0..n { + lock.push_str(&format!( + "pkg-{i}@^1.0.0:\n version \"1.0.{i}\"\n \ + resolved \"https://registry.yarnpkg.com/pkg-{i}/-/pkg-{i}-1.0.{i}.tgz#aa\"\n\n" + )); + } + lock.push_str( + "gitdep@git+https://github.com/x/gitdep.git:\n version \"2.0.0\"\n \ + resolved \"git+https://github.com/x/gitdep.git#abc\"\n", + ); + let mut candidates: Vec = (0..n) + .map(|i| classic_candidate(&format!("pkg-{i}"), &format!("1.0.{i}"), Some("s1"))) + .collect(); + // Every dep pinned to its artifact already: nothing to fetch. + let mut pinned = lock.clone(); + for c in &candidates { + pinned.push_str(&format!("# \"{}#s1\"\n", c.dep.artifact_url)); + } + let files = BTreeMap::from([("yarn.lock".to_string(), pinned)]); + let outer = OuterYarnMirror::default(); + let start = std::time::Instant::now(); + assert!(yarn_classic_artifact_targets(&candidates, &files, &outer).is_empty()); + + // Unpinned: every registry copy is selected once per artifact; a + // version the lock doesn't lock, a git copy and a non-npm dep never. + candidates.push(classic_candidate("pkg-0", "9.9.9", None)); + candidates.push(classic_candidate("gitdep", "2.0.0", None)); + candidates.push(classic_candidate("pkg-1", "1.0.1", Some("s1"))); + let mut pypi = classic_candidate("pkg-2", "1.0.2", None); + pypi.dep.ecosystem = "pypi".into(); + candidates.push(pypi); + let files = BTreeMap::from([("yarn.lock".to_string(), lock)]); + let got: Vec = yarn_classic_artifact_targets(&candidates, &files, &outer) + .into_iter() + .map(|d| format!("{}@{}", d.name, d.version)) + .collect(); + assert_eq!(got.len(), n); + assert_eq!(got[0], "pkg-0@1.0.0"); + assert_eq!(got[n - 1], format!("pkg-{}@1.0.{}", n - 1, n - 1)); + // Per-dep lock scans took minutes here in a debug build. + assert!( + start.elapsed() < std::time::Duration::from_secs(20), + "{:?}", + start.elapsed() + ); } #[test] From 6bb96fd90773fa40d4193fe1c1b445fcfb6160f5 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 09:03:19 +0000 Subject: [PATCH 3/8] Skip lock reads for absent names; count reads Keep the old quick "is this name in the lock at all" check before the registry-copy lookup, so a grant for a package the lock never mentions costs a substring search, not a read of every block. The blocks now come from the shared memoized scan, which the rewriter also reads later in the same run. The regression test counts lock reads instead of timing them: none on a fully pinned rescan, exactly one for many unpinned deps. It fails if the cheap filters move back behind the block read. Refs #1423 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-core/src/hosted/engine.rs | 78 ++++++++++++------- 1 file changed, 51 insertions(+), 27 deletions(-) diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 95b49f731..2550d1249 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -1212,9 +1212,21 @@ pub fn yarn_classic_artifact_targets<'a>( if crate::patch::redirect::yarn_classic_hosted_refused(files, yarn_outer) { return Vec::new(); } - // The pinned-artifact test is a substring search; the registry-copy - // test reads every block, so it runs once per lock, after it, and only - // when some dep still needs it (#1423). + select_classic_artifacts(candidates, lock, classic_registry_copies) +} + +/// [`yarn_classic_artifact_targets`] over a classic `lock`, reading its +/// registry copies through `read_copies`. The pinned-artifact and name +/// tests are substring searches; the registry-copy test reads every +/// block, so it runs after them, at most once per lock, and only when +/// some dep still needs it (#1423): a rescan whose grants all carry a +/// sha1 and are pinned already never reads the blocks. +fn select_classic_artifacts<'a>( + candidates: &'a [Candidate], + lock: &str, + read_copies: impl FnOnce(&str) -> HashSet<(String, String)>, +) -> Vec<&'a DepOverride> { + let mut read_copies = Some(read_copies); let mut registry_copies: Option> = None; let mut seen = BTreeSet::new(); candidates @@ -1225,9 +1237,13 @@ pub fn yarn_classic_artifact_targets<'a>( dep.integrity.sha1.is_none() || !lock.contains(&format!("\"{}#", dep.artifact_url)) }) .filter(|dep| { - registry_copies - .get_or_insert_with(|| classic_registry_copies(lock)) - .contains(&(crate::patch::redirect::full_name(dep), dep.version.clone())) + let name = crate::patch::redirect::full_name(dep); + lock.contains(&name) + && registry_copies + .get_or_insert_with(|| { + read_copies.take().map_or_else(HashSet::new, |r| r(lock)) + }) + .contains(&(name, dep.version.clone())) }) .filter(|dep| seen.insert(dep.artifact_url.clone())) .collect() @@ -1239,10 +1255,10 @@ pub fn yarn_classic_artifact_targets<'a>( /// by the blocks' real names, as the rewriter does, so `lodash` never /// matches a `lodash.debounce` block. fn classic_registry_copies(lock: &str) -> HashSet<(String, String)> { - use crate::formats::yarn::blocks::{classic_field, scan_blocks}; + use crate::formats::yarn::blocks::classic_field; use crate::formats::yarn::patterns::{classic_key_real_name, split_key_patterns}; use crate::formats::yarn::source::{classic_copy_source, CopySource}; - scan_blocks(lock) + crate::vendor::yarn_classic_lock::scan_blocks_shared(lock) .iter() .filter_map(|block| { let patterns = split_key_patterns(&block.key); @@ -3119,14 +3135,13 @@ mod tests { } /// #1423: the served tarball is read only for a dep the lock locks as - /// a registry copy and doesn't pin to the artifact yet; a big lock - /// whose deps are all pinned already (a rescan) selects none, and many - /// deps over a big lock stay cheap (the lock is read once, not once - /// per dep). + /// a registry copy and doesn't pin to the artifact yet. The lock's + /// blocks are read at most once however many deps there are, and not + /// at all on a rescan whose deps are all pinned already. #[test] - fn classic_artifact_targets_select_unpinned_registry_copies() { + fn classic_artifact_targets_read_the_lock_blocks_at_most_once() { let mut lock = String::from("# yarn lockfile v1\n\n"); - let n = 3000; + let n = 200; for i in 0..n { lock.push_str(&format!( "pkg-{i}@^1.0.0:\n version \"1.0.{i}\"\n \ @@ -3140,37 +3155,46 @@ mod tests { let mut candidates: Vec = (0..n) .map(|i| classic_candidate(&format!("pkg-{i}"), &format!("1.0.{i}"), Some("s1"))) .collect(); - // Every dep pinned to its artifact already: nothing to fetch. + let reads = std::cell::Cell::new(0); + let counted = |lock: &str| { + reads.set(reads.get() + 1); + classic_registry_copies(lock) + }; + + // Every dep pinned to its artifact already: nothing to fetch, and + // the blocks are never read. let mut pinned = lock.clone(); for c in &candidates { pinned.push_str(&format!("# \"{}#s1\"\n", c.dep.artifact_url)); } - let files = BTreeMap::from([("yarn.lock".to_string(), pinned)]); - let outer = OuterYarnMirror::default(); - let start = std::time::Instant::now(); - assert!(yarn_classic_artifact_targets(&candidates, &files, &outer).is_empty()); + assert!(select_classic_artifacts(&candidates, &pinned, counted).is_empty()); + assert_eq!(reads.get(), 0); // Unpinned: every registry copy is selected once per artifact; a - // version the lock doesn't lock, a git copy and a non-npm dep never. + // version the lock doesn't lock, a name it doesn't have, a git copy + // and a non-npm dep never. One read of the blocks for all of them. candidates.push(classic_candidate("pkg-0", "9.9.9", None)); + candidates.push(classic_candidate("absent", "1.0.0", None)); candidates.push(classic_candidate("gitdep", "2.0.0", None)); candidates.push(classic_candidate("pkg-1", "1.0.1", Some("s1"))); let mut pypi = classic_candidate("pkg-2", "1.0.2", None); pypi.dep.ecosystem = "pypi".into(); candidates.push(pypi); - let files = BTreeMap::from([("yarn.lock".to_string(), lock)]); - let got: Vec = yarn_classic_artifact_targets(&candidates, &files, &outer) + let got: Vec = select_classic_artifacts(&candidates, &lock, counted) .into_iter() .map(|d| format!("{}@{}", d.name, d.version)) .collect(); + assert_eq!(reads.get(), 1); assert_eq!(got.len(), n); assert_eq!(got[0], "pkg-0@1.0.0"); assert_eq!(got[n - 1], format!("pkg-{}@1.0.{}", n - 1, n - 1)); - // Per-dep lock scans took minutes here in a debug build. - assert!( - start.elapsed() < std::time::Duration::from_secs(20), - "{:?}", - start.elapsed() + + // The public entry point selects the same deps from the files. + let files = BTreeMap::from([("yarn.lock".to_string(), lock)]); + let outer = OuterYarnMirror::default(); + assert_eq!( + yarn_classic_artifact_targets(&candidates, &files, &outer).len(), + n ); } From ee84a855c0939d7761d78367a48e182d57752a23 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 09:14:45 +0000 Subject: [PATCH 4/8] Fetch yarn classic tarballs concurrently A hosted scan of a yarn classic project downloads each patched package's served tarball to check its sha1 and dependencies. Those downloads ran one at a time, so 60 patches cost 60 round trips in a row. They now run through the same ordered, bounded window the patch record fetch uses, and the results are folded in the same order as before, so pins, skips and --debug output are unchanged. A new test serves three tarballs (slow, fast, missing) and checks each pin carries its own tarball's sha1 and only the missing one is skipped. Refs #1423 Assisted-by: Claude Code:claude-opus-5-5 --- .../src/commands/scan/hosted.rs | 29 ++- .../tests/scan/hosted_yarn_classic_sha1.rs | 170 ++++++++++++++++++ 2 files changed, 191 insertions(+), 8 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index 7c5405c86..7af56f730 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -1217,22 +1217,35 @@ async fn run_redirect_selected_untracked( Some((dep.artifact_url.clone(), sha512, dep.clone())) }) .collect(); - for (url, sha512, dep) in classic_targets { + // Fetched concurrently but folded in target order (#1423), so the + // recorded sha1s, the skips and the held-back `--debug` lines come out + // exactly as the serial loop's did. + let mut fetched = std::pin::pin!(ordered_concurrent( + classic_targets.iter(), + api_concurrency_for(api_client.uses_public_proxy(), classic_targets.len()), + |(url, sha512, _)| { + hold_back_debug( + socket_patch_core::hosted::npm_manifest::fetch_hosted_classic_artifact( + api_client, url, sha512, + ), + ) + }, + )); + for (url, _, dep) in &classic_targets { status.set(format!("Fetching hosted tarball for {}...", dep.name)); - match socket_patch_core::hosted::npm_manifest::fetch_hosted_classic_artifact( - api_client, &url, &sha512, - ) - .await - { + let Some(fetch) = fetched.next().await else { + break; + }; + match fetch.release() { Ok(artifact) => engine::record_classic_artifact( &mut candidates, &mut python_metadata, - &url, + url, &artifact, ), Err(detail) => { unavailable_python_artifacts.insert(url.clone()); - skipped.push(engine::npm_tarball_unavailable(&dep, &detail)); + skipped.push(engine::npm_tarball_unavailable(dep, &detail)); } } } diff --git a/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs b/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs index 2e4c469e6..d869f4b32 100644 --- a/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs +++ b/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs @@ -341,3 +341,173 @@ async fn mock_api_with_sha1(server: &MockServer, tarball: &[u8]) { .await; mock_api(server, tarball, Some(tarball.to_vec())).await; } + +/// One patched package of [`issue_1423_tarballs_fetched_together_land_on_their_own_pins`]. +struct Served { + name: &'static str, + uuid: &'static str, + tarball: Vec, + /// Served after this delay, or 404 when `None`. + delay_ms: Option, +} + +fn served(name: &'static str, uuid: &'static str, delay_ms: Option) -> Served { + let manifest = format!(r#"{{"name":"{name}","version":"1.3.0"}}"#); + let body = format!("module.exports = '{name} patched';\n"); + Served { + name, + uuid, + tarball: tgz(&[ + ("package/package.json", manifest.as_bytes()), + ("package/index.js", body.as_bytes()), + ]), + delay_ms, + } +} + +/// #1423: the served tarballs are fetched concurrently. A slow one and a +/// fast one each pin the sha1 of their own bytes, and one that fails skips +/// only its own patch, whichever finishes first. +#[tokio::test] +async fn issue_1423_tarballs_fetched_together_land_on_their_own_pins() { + let pkgs = [ + served("aaa-pad", "11111111-1111-4111-8111-111111111111", Some(400)), + served("bbb-pad", "22222222-2222-4222-8222-222222222222", Some(0)), + served("ccc-pad", "33333333-3333-4333-8333-000000000003", None), + ]; + let server = MockServer::start().await; + let purl = |p: &Served| format!("pkg:npm/{}@1.3.0", p.name); + let artifact = |p: &Served| { + format!( + "/patch/npm/{0}/1.3.0/{TOKEN}/{1}/{0}-1.3.0.tgz", + p.name, p.uuid + ) + }; + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG}/patches/batch"))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "packages": pkgs.iter().map(|p| json!({ + "purl": purl(p), + "patches": [{ + "uuid": p.uuid, "purl": purl(p), "tier": "free", + "cveIds": [], "ghsaIds": [], "severity": "high", + "title": "fixture" + }] + })).collect::>(), + "canAccessPaidPatches": false, + }))) + .mount(&server) + .await; + let mut results = serde_json::Map::new(); + for p in &pkgs { + let url = format!("{}{}", server.uri(), artifact(p)); + results.insert( + p.uuid.into(), + json!({ + "status": "granted", "url": url, "purl": purl(p), + "artifacts": [{ + "kind": "tarball", "url": url, + "integrity": { "sha512": sha512_sri(&p.tarball) } + }], + "registryOverride": null + }), + ); + Mock::given(method("GET")) + .and(path_regex(format!( + "^/v0/orgs/{ORG}/patches/by-package/.*{}.*$", + p.name + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "patches": [{ + "uuid": p.uuid, "purl": purl(p), + "publishedAt": "2026-01-01T00:00:00Z", + "description": "x", "license": "MIT", "tier": "free", + "vulnerabilities": {} + }], + "canAccessPaidPatches": false, + }))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path(format!("/v0/orgs/{ORG}/patches/view/{}", p.uuid))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "uuid": p.uuid, "purl": purl(p), + "publishedAt": "2026-01-01T00:00:00Z", + "files": {}, "vulnerabilities": {}, + "description": "x", "license": "MIT", "tier": "free" + }))) + .mount(&server) + .await; + let response = match p.delay_ms { + Some(ms) => ResponseTemplate::new(200) + .set_body_raw(p.tarball.clone(), "application/octet-stream") + .set_delay(std::time::Duration::from_millis(ms)), + None => ResponseTemplate::new(404), + }; + Mock::given(method("GET")) + .and(path(artifact(p))) + .respond_with(response) + .mount(&server) + .await; + } + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG}/patches/package"))) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "results": results }))) + .mount(&server) + .await; + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join("proj"); + let mut lock = String::from("# yarn lockfile v1\n\n"); + let mut deps = Vec::new(); + for p in &pkgs { + let n = p.name; + std::fs::create_dir_all(root.join("node_modules").join(n)).unwrap(); + std::fs::write( + root.join("node_modules").join(n).join("package.json"), + format!(r#"{{"name":"{n}","version":"1.3.0"}}"#), + ) + .unwrap(); + lock.push_str(&format!( + "\n{n}@1.3.0:\n version \"1.3.0\"\n \ + resolved \"https://registry.yarnpkg.com/{n}/-/{n}-1.3.0.tgz\"\n" + )); + deps.push(format!("\"{n}\": \"1.3.0\"")); + } + std::fs::write( + root.join("package.json"), + format!( + "{{\"name\":\"app\",\"version\":\"1.0.0\",\"private\":true,\ + \"packageManager\":\"yarn@1.22.22\",\"dependencies\":{{{}}}}}\n", + deps.join(",") + ), + ) + .unwrap(); + std::fs::write(root.join("yarn.lock"), &lock).unwrap(); + + let (_, doc, stderr) = scan(&root, &server.uri()); + let written = std::fs::read_to_string(root.join("yarn.lock")).unwrap(); + for p in &pkgs[..2] { + let url = format!("{}{}", server.uri(), artifact(p)); + assert!( + written.contains(&format!( + " resolved \"{url}#{}\"\n integrity {}\n", + sha1_hex(&p.tarball), + sha512_sri(&p.tarball) + )), + "{} pins its own tarball's sha1:\n{written}\n{doc:#}\n{stderr}", + p.name + ); + } + assert_eq!(common::envelope::hosted_pins(&doc).len(), 2, "{doc:#}"); + let skipped: Vec<&Value> = common::envelope::mode_events(&doc, "hosted") + .into_iter() + .filter(|s| s["action"] == "skipped" && s["errorCode"] == "npm_tarball_unavailable") + .collect(); + assert_eq!(skipped.len(), 1, "{doc:#}"); + assert_eq!(skipped[0]["purl"], purl(&pkgs[2]), "{doc:#}"); + assert!( + written.contains("resolved \"https://registry.yarnpkg.com/ccc-pad/-/ccc-pad-1.3.0.tgz\""), + "{written}" + ); +} From 286280ba1fc19dc04164f3f18059f756e4c37821 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 09:26:29 +0000 Subject: [PATCH 5/8] Check served yarn deps without re-splitting keys The yarn classic dependency check (#591) runs once per patched package and split every key of yarn.lock into its patterns each time, even when the served package has no dependencies at all. On a large lock this was the biggest remaining cost of a hosted scan. It now returns at once when there is nothing to look up, and only splits the keys whose text names one of the wanted packages. The result is the same set of unlocked descriptors as before. Refs #1423 Assisted-by: Claude Code:claude-opus-5-5 --- .../src/formats/yarn/classic_deps.rs | 64 ++++++++++++++++--- 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/crates/socket-patch-core/src/formats/yarn/classic_deps.rs b/crates/socket-patch-core/src/formats/yarn/classic_deps.rs index d962d5919..add053741 100644 --- a/crates/socket-patch-core/src/formats/yarn/classic_deps.rs +++ b/crates/socket-patch-core/src/formats/yarn/classic_deps.rs @@ -63,21 +63,45 @@ pub(crate) fn unlocked_descriptors(blocks: &[LockBlock], pkg: &Value) -> Vec, ) -> Vec { - let locked: std::collections::BTreeSet = blocks + let mut missing: std::collections::BTreeSet = descriptors.collect(); + // A pattern is its key's text less the quotes, so the package name + // of a quote-free descriptor appears verbatim in any key locking it. + let names: Option> = missing .iter() - .flat_map(|b| split_key_patterns(&b.key)) + .map(|d| (!d.contains('"')).then(|| descriptor_name(d).to_string())) .collect(); - let mut missing: Vec = descriptors - .filter(|descriptor| !locked.contains(descriptor)) - .collect(); - missing.sort(); - missing.dedup(); - missing + for block in blocks { + if missing.is_empty() { + break; + } + if let Some(names) = &names { + if !names.iter().any(|name| block.key.contains(name.as_str())) { + continue; + } + } + for pattern in split_key_patterns(&block.key) { + missing.remove(&pattern); + } + } + missing.into_iter().collect() +} + +/// The package name of a `name@range` descriptor (`@scope/x` keeps its +/// leading `@`); the whole text when it has no range. +fn descriptor_name(descriptor: &str) -> &str { + match descriptor.char_indices().skip(1).find(|&(_, c)| c == '@') { + Some((at, _)) => &descriptor[..at], + None => descriptor, + } } /// The descriptors (`name@range`) the dependency sub-maps of `lines` (one @@ -197,6 +221,28 @@ is-odd@3.0.1:\n version \"3.0.1\"\n dependencies:\n is-number \"^6.0.0\"\n" text.lines().map(str::to_string).collect() } + /// #1423: only the keys naming a wanted package are split, and a + /// descriptor still resolves through any pattern of a multi-pattern, + /// quoted or scoped key. + #[test] + fn unlocked_among_finds_descriptors_in_any_key_pattern() { + let lock = "# yarn lockfile v1\n\n\ + \"@scope/x@^1.0.0\", \"@scope/x@~1.2.0\":\n version \"1.2.3\"\n\n\ + left-pad@^1.0.0, left-pad@^1.3.0:\n version \"1.3.0\"\n\n\ + lodash.debounce@^4.0.0:\n version \"4.0.8\"\n"; + let blocks = scan_blocks(lock); + let among = |ds: &[&str]| unlocked_among(&blocks, ds.iter().map(|d| d.to_string())); + assert!(among(&[]).is_empty()); + assert!(among(&["@scope/x@~1.2.0", "left-pad@^1.3.0", "left-pad@^1.0.0"]).is_empty()); + assert_eq!( + among(&["lodash@^4.0.0", "@scope/x@^2.0.0", "lodash@^4.0.0"]), + vec!["@scope/x@^2.0.0", "lodash@^4.0.0"] + ); + assert_eq!(descriptor_name("@scope/x@^1.0.0"), "@scope/x"); + assert_eq!(descriptor_name("left-pad@npm:other@^1"), "left-pad"); + assert_eq!(descriptor_name("bare"), "bare"); + } + #[test] fn unlocked_descriptors_names_only_the_unresolved_ones() { let blocks = scan_blocks(LOCK); From a60710e672b1478dd1315cc92ffc7402e5be2cbc Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 09:44:07 +0000 Subject: [PATCH 6/8] Look up pinned yarn artifacts in one lock pass Picking the served tarballs to fetch still searched the whole yarn.lock twice per patched package: once for its pinned URL and once for its name. The lock is now read once for every URL it pins with a fragment, and each package is a set lookup. Refs #1423 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-core/src/hosted/engine.rs | 52 ++++++++++++++----- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index 2550d1249..c0ef5e9bd 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -1216,9 +1216,9 @@ pub fn yarn_classic_artifact_targets<'a>( } /// [`yarn_classic_artifact_targets`] over a classic `lock`, reading its -/// registry copies through `read_copies`. The pinned-artifact and name -/// tests are substring searches; the registry-copy test reads every -/// block, so it runs after them, at most once per lock, and only when +/// registry copies through `read_copies`. The lock is read in one pass for +/// the artifacts it pins already; the registry-copy test reads every +/// block, so it runs after that, at most once per lock, and only when /// some dep still needs it (#1423): a rescan whose grants all carry a /// sha1 and are pinned already never reads the blocks. fn select_classic_artifacts<'a>( @@ -1226,6 +1226,7 @@ fn select_classic_artifacts<'a>( lock: &str, read_copies: impl FnOnce(&str) -> HashSet<(String, String)>, ) -> Vec<&'a DepOverride> { + let pinned = quoted_fragment_urls(lock); let mut read_copies = Some(read_copies); let mut registry_copies: Option> = None; let mut seen = BTreeSet::new(); @@ -1233,22 +1234,34 @@ fn select_classic_artifacts<'a>( .iter() .map(|c| &c.dep) .filter(|dep| dep.ecosystem == "npm" && dep.integrity.sha512.is_some()) + .filter(|dep| dep.integrity.sha1.is_none() || !pinned.contains(dep.artifact_url.as_str())) .filter(|dep| { - dep.integrity.sha1.is_none() || !lock.contains(&format!("\"{}#", dep.artifact_url)) - }) - .filter(|dep| { - let name = crate::patch::redirect::full_name(dep); - lock.contains(&name) - && registry_copies - .get_or_insert_with(|| { - read_copies.take().map_or_else(HashSet::new, |r| r(lock)) - }) - .contains(&(name, dep.version.clone())) + registry_copies + .get_or_insert_with(|| read_copies.take().map_or_else(HashSet::new, |r| r(lock))) + .contains(&(crate::patch::redirect::full_name(dep), dep.version.clone())) }) .filter(|dep| seen.insert(dep.artifact_url.clone())) .collect() } +/// Every `` that `text` writes as `"#…`: an opening quote, then +/// text with no quote, `#` or line break, then a `#` (a `resolved` URL +/// with its fragment). One pass, so asking whether the lock pins an +/// artifact is a set lookup rather than a search of the whole lock. +fn quoted_fragment_urls(text: &str) -> HashSet<&str> { + let mut urls = HashSet::new(); + let mut rest = text; + while let Some(open) = rest.find('"') { + let after = &rest[open + 1..]; + let end = after.find(['"', '#', '\n']).unwrap_or(after.len()); + if after[end..].starts_with('#') { + urls.insert(&after[..end]); + } + rest = &after[end..]; + } + urls +} + /// The `(name, version)` of every registry block in a classic `lock`: the /// only copies a hosted pin rewrites (a git, `file:`, `link:` or remote /// tarball copy is skipped by name, so it needs no served tarball). Read @@ -3134,6 +3147,19 @@ mod tests { } } + #[test] + fn quoted_fragment_urls_reads_each_resolved_url() { + let lock = "a@^1:\n resolved \"https://r/a.tgz#aa\"\n\ + b@^1:\n resolved \"https://r/b.tgz\"\n\ + \"c@^1\", \"c@^2\":\n resolved \"https://r/c.tgz#cc\"\n"; + let urls = quoted_fragment_urls(lock); + assert_eq!( + urls, + HashSet::from(["https://r/a.tgz", "https://r/c.tgz"]), + "{urls:?}" + ); + } + /// #1423: the served tarball is read only for a dep the lock locks as /// a registry copy and doesn't pin to the artifact yet. The lock's /// blocks are read at most once however many deps there are, and not From 117e5c58944cd6ef1f03078903d9271444cde954 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 09:52:26 +0000 Subject: [PATCH 7/8] Revert "Fetch yarn classic tarballs concurrently" This reverts commit ee84a855c0939d7761d78367a48e182d57752a23. --- .../src/commands/scan/hosted.rs | 29 +-- .../tests/scan/hosted_yarn_classic_sha1.rs | 170 ------------------ 2 files changed, 8 insertions(+), 191 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index 7af56f730..7c5405c86 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -1217,35 +1217,22 @@ async fn run_redirect_selected_untracked( Some((dep.artifact_url.clone(), sha512, dep.clone())) }) .collect(); - // Fetched concurrently but folded in target order (#1423), so the - // recorded sha1s, the skips and the held-back `--debug` lines come out - // exactly as the serial loop's did. - let mut fetched = std::pin::pin!(ordered_concurrent( - classic_targets.iter(), - api_concurrency_for(api_client.uses_public_proxy(), classic_targets.len()), - |(url, sha512, _)| { - hold_back_debug( - socket_patch_core::hosted::npm_manifest::fetch_hosted_classic_artifact( - api_client, url, sha512, - ), - ) - }, - )); - for (url, _, dep) in &classic_targets { + for (url, sha512, dep) in classic_targets { status.set(format!("Fetching hosted tarball for {}...", dep.name)); - let Some(fetch) = fetched.next().await else { - break; - }; - match fetch.release() { + match socket_patch_core::hosted::npm_manifest::fetch_hosted_classic_artifact( + api_client, &url, &sha512, + ) + .await + { Ok(artifact) => engine::record_classic_artifact( &mut candidates, &mut python_metadata, - url, + &url, &artifact, ), Err(detail) => { unavailable_python_artifacts.insert(url.clone()); - skipped.push(engine::npm_tarball_unavailable(dep, &detail)); + skipped.push(engine::npm_tarball_unavailable(&dep, &detail)); } } } diff --git a/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs b/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs index d869f4b32..2e4c469e6 100644 --- a/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs +++ b/crates/socket-patch-cli/tests/scan/hosted_yarn_classic_sha1.rs @@ -341,173 +341,3 @@ async fn mock_api_with_sha1(server: &MockServer, tarball: &[u8]) { .await; mock_api(server, tarball, Some(tarball.to_vec())).await; } - -/// One patched package of [`issue_1423_tarballs_fetched_together_land_on_their_own_pins`]. -struct Served { - name: &'static str, - uuid: &'static str, - tarball: Vec, - /// Served after this delay, or 404 when `None`. - delay_ms: Option, -} - -fn served(name: &'static str, uuid: &'static str, delay_ms: Option) -> Served { - let manifest = format!(r#"{{"name":"{name}","version":"1.3.0"}}"#); - let body = format!("module.exports = '{name} patched';\n"); - Served { - name, - uuid, - tarball: tgz(&[ - ("package/package.json", manifest.as_bytes()), - ("package/index.js", body.as_bytes()), - ]), - delay_ms, - } -} - -/// #1423: the served tarballs are fetched concurrently. A slow one and a -/// fast one each pin the sha1 of their own bytes, and one that fails skips -/// only its own patch, whichever finishes first. -#[tokio::test] -async fn issue_1423_tarballs_fetched_together_land_on_their_own_pins() { - let pkgs = [ - served("aaa-pad", "11111111-1111-4111-8111-111111111111", Some(400)), - served("bbb-pad", "22222222-2222-4222-8222-222222222222", Some(0)), - served("ccc-pad", "33333333-3333-4333-8333-000000000003", None), - ]; - let server = MockServer::start().await; - let purl = |p: &Served| format!("pkg:npm/{}@1.3.0", p.name); - let artifact = |p: &Served| { - format!( - "/patch/npm/{0}/1.3.0/{TOKEN}/{1}/{0}-1.3.0.tgz", - p.name, p.uuid - ) - }; - Mock::given(method("POST")) - .and(path(format!("/v0/orgs/{ORG}/patches/batch"))) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "packages": pkgs.iter().map(|p| json!({ - "purl": purl(p), - "patches": [{ - "uuid": p.uuid, "purl": purl(p), "tier": "free", - "cveIds": [], "ghsaIds": [], "severity": "high", - "title": "fixture" - }] - })).collect::>(), - "canAccessPaidPatches": false, - }))) - .mount(&server) - .await; - let mut results = serde_json::Map::new(); - for p in &pkgs { - let url = format!("{}{}", server.uri(), artifact(p)); - results.insert( - p.uuid.into(), - json!({ - "status": "granted", "url": url, "purl": purl(p), - "artifacts": [{ - "kind": "tarball", "url": url, - "integrity": { "sha512": sha512_sri(&p.tarball) } - }], - "registryOverride": null - }), - ); - Mock::given(method("GET")) - .and(path_regex(format!( - "^/v0/orgs/{ORG}/patches/by-package/.*{}.*$", - p.name - ))) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "patches": [{ - "uuid": p.uuid, "purl": purl(p), - "publishedAt": "2026-01-01T00:00:00Z", - "description": "x", "license": "MIT", "tier": "free", - "vulnerabilities": {} - }], - "canAccessPaidPatches": false, - }))) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path(format!("/v0/orgs/{ORG}/patches/view/{}", p.uuid))) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "uuid": p.uuid, "purl": purl(p), - "publishedAt": "2026-01-01T00:00:00Z", - "files": {}, "vulnerabilities": {}, - "description": "x", "license": "MIT", "tier": "free" - }))) - .mount(&server) - .await; - let response = match p.delay_ms { - Some(ms) => ResponseTemplate::new(200) - .set_body_raw(p.tarball.clone(), "application/octet-stream") - .set_delay(std::time::Duration::from_millis(ms)), - None => ResponseTemplate::new(404), - }; - Mock::given(method("GET")) - .and(path(artifact(p))) - .respond_with(response) - .mount(&server) - .await; - } - Mock::given(method("POST")) - .and(path(format!("/v0/orgs/{ORG}/patches/package"))) - .respond_with(ResponseTemplate::new(200).set_body_json(json!({ "results": results }))) - .mount(&server) - .await; - - let tmp = tempfile::tempdir().unwrap(); - let root = tmp.path().join("proj"); - let mut lock = String::from("# yarn lockfile v1\n\n"); - let mut deps = Vec::new(); - for p in &pkgs { - let n = p.name; - std::fs::create_dir_all(root.join("node_modules").join(n)).unwrap(); - std::fs::write( - root.join("node_modules").join(n).join("package.json"), - format!(r#"{{"name":"{n}","version":"1.3.0"}}"#), - ) - .unwrap(); - lock.push_str(&format!( - "\n{n}@1.3.0:\n version \"1.3.0\"\n \ - resolved \"https://registry.yarnpkg.com/{n}/-/{n}-1.3.0.tgz\"\n" - )); - deps.push(format!("\"{n}\": \"1.3.0\"")); - } - std::fs::write( - root.join("package.json"), - format!( - "{{\"name\":\"app\",\"version\":\"1.0.0\",\"private\":true,\ - \"packageManager\":\"yarn@1.22.22\",\"dependencies\":{{{}}}}}\n", - deps.join(",") - ), - ) - .unwrap(); - std::fs::write(root.join("yarn.lock"), &lock).unwrap(); - - let (_, doc, stderr) = scan(&root, &server.uri()); - let written = std::fs::read_to_string(root.join("yarn.lock")).unwrap(); - for p in &pkgs[..2] { - let url = format!("{}{}", server.uri(), artifact(p)); - assert!( - written.contains(&format!( - " resolved \"{url}#{}\"\n integrity {}\n", - sha1_hex(&p.tarball), - sha512_sri(&p.tarball) - )), - "{} pins its own tarball's sha1:\n{written}\n{doc:#}\n{stderr}", - p.name - ); - } - assert_eq!(common::envelope::hosted_pins(&doc).len(), 2, "{doc:#}"); - let skipped: Vec<&Value> = common::envelope::mode_events(&doc, "hosted") - .into_iter() - .filter(|s| s["action"] == "skipped" && s["errorCode"] == "npm_tarball_unavailable") - .collect(); - assert_eq!(skipped.len(), 1, "{doc:#}"); - assert_eq!(skipped[0]["purl"], purl(&pkgs[2]), "{doc:#}"); - assert!( - written.contains("resolved \"https://registry.yarnpkg.com/ccc-pad/-/ccc-pad-1.3.0.tgz\""), - "{written}" - ); -} From 9d0c1bc79c311694009d54f2e818b15c5836cd49 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 12 Oct 2026 09:55:49 +0000 Subject: [PATCH 8/8] Check served yarn deps against keys split once The yarn classic dependency check (#591) runs once per patched package and split every key of yarn.lock into its patterns each time. On a large lock that was the biggest remaining cost of a hosted scan. The rewriter already splits each key once, so the check now uses a set built from those patterns, made the first time a package needs it. The result is the same set of unlocked descriptors. Target selection also goes back to a plain block scan (the shared memo it used is not read by the hosted rewriter, so it only held a copy of the lock), and builds its pinned-URL set only once a package with a sha1 asks for it. Refs #1423 Assisted-by: Claude Code:claude-opus-5-5 --- .../src/formats/yarn/classic_deps.rs | 117 +++++++++--------- crates/socket-patch-core/src/hosted/engine.rs | 22 ++-- .../src/patch/redirect/mod.rs | 11 +- 3 files changed, 81 insertions(+), 69 deletions(-) diff --git a/crates/socket-patch-core/src/formats/yarn/classic_deps.rs b/crates/socket-patch-core/src/formats/yarn/classic_deps.rs index add053741..0ae7fd482 100644 --- a/crates/socket-patch-core/src/formats/yarn/classic_deps.rs +++ b/crates/socket-patch-core/src/formats/yarn/classic_deps.rs @@ -53,55 +53,49 @@ pub(crate) fn with_manifest_dep_maps(lines: &[String], pkg: &Value) -> Vec Vec { - unlocked_among( - blocks, - manifest_dep_maps(pkg) - .into_iter() - .flat_map(|(_, deps)| deps) - .map(|(name, range)| format!("{name}@{range}")), - ) + unlocked_among(blocks, manifest_descriptors(pkg)) +} + +/// [`unlocked_descriptors`] against `locked`, every pattern the lock's +/// keys hold: for a caller that checks many manifests against one lock +/// and splits its keys once, not once per manifest (#1423). +pub(crate) fn unlocked_descriptors_in( + locked: &std::collections::HashSet<&str>, + pkg: &Value, +) -> Vec { + sorted_missing(manifest_descriptors(pkg), |d| locked.contains(d)) } /// Every descriptor of `descriptors` no block of `blocks` is keyed by, -/// sorted and deduplicated. Only a block whose key text names one of the -/// descriptors' packages is split into its patterns, and none is when -/// there are no descriptors: the hosted flow asks once per patched -/// package, so splitting every key of a big lock each time made a scan -/// O(patched packages × lock size) (#1423). +/// sorted and deduplicated. pub(crate) fn unlocked_among( blocks: &[LockBlock], descriptors: impl Iterator, ) -> Vec { - let mut missing: std::collections::BTreeSet = descriptors.collect(); - // A pattern is its key's text less the quotes, so the package name - // of a quote-free descriptor appears verbatim in any key locking it. - let names: Option> = missing + let locked: std::collections::BTreeSet = blocks .iter() - .map(|d| (!d.contains('"')).then(|| descriptor_name(d).to_string())) + .flat_map(|b| split_key_patterns(&b.key)) .collect(); - for block in blocks { - if missing.is_empty() { - break; - } - if let Some(names) = &names { - if !names.iter().any(|name| block.key.contains(name.as_str())) { - continue; - } - } - for pattern in split_key_patterns(&block.key) { - missing.remove(&pattern); - } - } - missing.into_iter().collect() + sorted_missing(descriptors, |d| locked.contains(d)) } -/// The package name of a `name@range` descriptor (`@scope/x` keeps its -/// leading `@`); the whole text when it has no range. -fn descriptor_name(descriptor: &str) -> &str { - match descriptor.char_indices().skip(1).find(|&(_, c)| c == '@') { - Some((at, _)) => &descriptor[..at], - None => descriptor, - } +/// The descriptors `pkg`'s dependency maps declare, as `name@range`. +fn manifest_descriptors(pkg: &Value) -> impl Iterator { + manifest_dep_maps(pkg) + .into_iter() + .flat_map(|(_, deps)| deps) + .map(|(name, range)| format!("{name}@{range}")) +} + +/// The descriptors `is_locked` rejects, sorted and deduplicated. +fn sorted_missing( + descriptors: impl Iterator, + is_locked: impl Fn(&str) -> bool, +) -> Vec { + let mut missing: Vec = descriptors.filter(|d| !is_locked(d)).collect(); + missing.sort(); + missing.dedup(); + missing } /// The descriptors (`name@range`) the dependency sub-maps of `lines` (one @@ -221,28 +215,6 @@ is-odd@3.0.1:\n version \"3.0.1\"\n dependencies:\n is-number \"^6.0.0\"\n" text.lines().map(str::to_string).collect() } - /// #1423: only the keys naming a wanted package are split, and a - /// descriptor still resolves through any pattern of a multi-pattern, - /// quoted or scoped key. - #[test] - fn unlocked_among_finds_descriptors_in_any_key_pattern() { - let lock = "# yarn lockfile v1\n\n\ - \"@scope/x@^1.0.0\", \"@scope/x@~1.2.0\":\n version \"1.2.3\"\n\n\ - left-pad@^1.0.0, left-pad@^1.3.0:\n version \"1.3.0\"\n\n\ - lodash.debounce@^4.0.0:\n version \"4.0.8\"\n"; - let blocks = scan_blocks(lock); - let among = |ds: &[&str]| unlocked_among(&blocks, ds.iter().map(|d| d.to_string())); - assert!(among(&[]).is_empty()); - assert!(among(&["@scope/x@~1.2.0", "left-pad@^1.3.0", "left-pad@^1.0.0"]).is_empty()); - assert_eq!( - among(&["lodash@^4.0.0", "@scope/x@^2.0.0", "lodash@^4.0.0"]), - vec!["@scope/x@^2.0.0", "lodash@^4.0.0"] - ); - assert_eq!(descriptor_name("@scope/x@^1.0.0"), "@scope/x"); - assert_eq!(descriptor_name("left-pad@npm:other@^1"), "left-pad"); - assert_eq!(descriptor_name("bare"), "bare"); - } - #[test] fn unlocked_descriptors_names_only_the_unresolved_ones() { let blocks = scan_blocks(LOCK); @@ -259,6 +231,31 @@ is-odd@3.0.1:\n version \"3.0.1\"\n dependencies:\n is-number \"^6.0.0\"\n" assert!(unlocked_descriptors(&blocks, &serde_json::json!({})).is_empty()); } + /// #1423: the pre-split form agrees with [`unlocked_descriptors`]. + #[test] + fn unlocked_descriptors_in_matches_the_block_scan() { + let blocks = scan_blocks(LOCK); + let patterns: Vec = blocks + .iter() + .flat_map(|b| split_key_patterns(&b.key)) + .collect(); + let locked: std::collections::HashSet<&str> = patterns.iter().map(String::as_str).collect(); + for pkg in [ + serde_json::json!({ + "dependencies": {"is-number": "^7.0.0", "wow": "^1.0.0"}, + "optionalDependencies": {"@scope/opt": "^2.0.0"}, + }), + serde_json::json!({"dependencies": {"is-number": "^6.0.0"}}), + serde_json::json!({}), + ] { + assert_eq!( + unlocked_descriptors_in(&locked, &pkg), + unlocked_descriptors(&blocks, &pkg), + "{pkg}" + ); + } + } + #[test] fn block_dep_descriptors_reads_the_blocks_own_sub_maps() { // #1379: a block a pre-v5 vendor rewrote to a patched manifest diff --git a/crates/socket-patch-core/src/hosted/engine.rs b/crates/socket-patch-core/src/hosted/engine.rs index c0ef5e9bd..14e67b34d 100644 --- a/crates/socket-patch-core/src/hosted/engine.rs +++ b/crates/socket-patch-core/src/hosted/engine.rs @@ -1217,16 +1217,17 @@ pub fn yarn_classic_artifact_targets<'a>( /// [`yarn_classic_artifact_targets`] over a classic `lock`, reading its /// registry copies through `read_copies`. The lock is read in one pass for -/// the artifacts it pins already; the registry-copy test reads every -/// block, so it runs after that, at most once per lock, and only when -/// some dep still needs it (#1423): a rescan whose grants all carry a -/// sha1 and are pinned already never reads the blocks. +/// the artifacts it pins already, once a dep with a sha1 asks; the +/// registry-copy test reads every block, so it runs after that, at most +/// once per lock, and only when some dep still needs it (#1423): a rescan +/// whose grants all carry a sha1 and are pinned already never reads the +/// blocks. fn select_classic_artifacts<'a>( candidates: &'a [Candidate], lock: &str, read_copies: impl FnOnce(&str) -> HashSet<(String, String)>, ) -> Vec<&'a DepOverride> { - let pinned = quoted_fragment_urls(lock); + let mut pinned: Option> = None; let mut read_copies = Some(read_copies); let mut registry_copies: Option> = None; let mut seen = BTreeSet::new(); @@ -1234,7 +1235,12 @@ fn select_classic_artifacts<'a>( .iter() .map(|c| &c.dep) .filter(|dep| dep.ecosystem == "npm" && dep.integrity.sha512.is_some()) - .filter(|dep| dep.integrity.sha1.is_none() || !pinned.contains(dep.artifact_url.as_str())) + .filter(|dep| { + dep.integrity.sha1.is_none() + || !pinned + .get_or_insert_with(|| quoted_fragment_urls(lock)) + .contains(dep.artifact_url.as_str()) + }) .filter(|dep| { registry_copies .get_or_insert_with(|| read_copies.take().map_or_else(HashSet::new, |r| r(lock))) @@ -1268,10 +1274,10 @@ fn quoted_fragment_urls(text: &str) -> HashSet<&str> { /// by the blocks' real names, as the rewriter does, so `lodash` never /// matches a `lodash.debounce` block. fn classic_registry_copies(lock: &str) -> HashSet<(String, String)> { - use crate::formats::yarn::blocks::classic_field; + use crate::formats::yarn::blocks::{classic_field, scan_blocks}; use crate::formats::yarn::patterns::{classic_key_real_name, split_key_patterns}; use crate::formats::yarn::source::{classic_copy_source, CopySource}; - crate::vendor::yarn_classic_lock::scan_blocks_shared(lock) + scan_blocks(lock) .iter() .filter_map(|block| { let patterns = split_key_patterns(&block.key); diff --git a/crates/socket-patch-core/src/patch/redirect/mod.rs b/crates/socket-patch-core/src/patch/redirect/mod.rs index 7a8d54452..ea8687474 100644 --- a/crates/socket-patch-core/src/patch/redirect/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/mod.rs @@ -3967,6 +3967,9 @@ fn rewrite_yarn_classic_with( // byte span once, at the end: re-scanning and re-copying the whole lock // per pinned block is quadratic in a large lock. let mut pinned_blocks: Vec = vec![false; blocks.len()]; + // Every pattern the keys hold, for the served-manifest check (#591): + // gathered from `heads` once, when the first dep needs it (#1423). + let mut locked_patterns: Option> = None; let mut segments: Option = None; let mut changed = false; let mut any_pinned = false; @@ -4007,7 +4010,13 @@ fn rewrite_yarn_classic_with( .and_then(|text| serde_json::from_str::(text).ok()) .filter(Value::is_object); if let Some(pkg) = &served_manifest { - let missing = crate::formats::yarn::classic_deps::unlocked_descriptors(&blocks, pkg); + let locked = locked_patterns.get_or_insert_with(|| { + heads + .iter() + .flat_map(|(patterns, _)| patterns.iter().map(String::as_str)) + .collect() + }); + let missing = crate::formats::yarn::classic_deps::unlocked_descriptors_in(locked, pkg); if !missing.is_empty() { result .refused_yarn_classic_uuids