Fix yarn classic hosted scan lock rescans (#1423)

crates/socket-patch-core/src/formats/yarn/classic_deps.rs

@@ -53,13 +53,17 @@ pub(crate) fn with_manifest_dep_maps(lines: &[String], pkg: &Value) -> Vec<Strin
 /// block of `blocks` is keyed by, sorted and deduplicated: the ones a lock
 /// rewritten to the patched manifest would leave unresolved.
 pub(crate) fn unlocked_descriptors(blocks: &[LockBlock], pkg: &Value) -> Vec<String> {
-    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<String> {
+    sorted_missing(manifest_descriptors(pkg), |d| locked.contains(d))
 }
 
 /// Every descriptor of `descriptors` no block of `blocks` is keyed by,
@@ -72,9 +76,23 @@ pub(crate) fn unlocked_among(
         .iter()
         .flat_map(|b| split_key_patterns(&b.key))
         .collect();
-    let mut missing: Vec<String> = descriptors
-        .filter(|descriptor| !locked.contains(descriptor))
-        .collect();
+    sorted_missing(descriptors, |d| locked.contains(d))
+}
+
+/// The descriptors `pkg`'s dependency maps declare, as `name@range`.
+fn manifest_descriptors(pkg: &Value) -> impl Iterator<Item = String> {
+    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<Item = String>,
+    is_locked: impl Fn(&str) -> bool,
+) -> Vec<String> {
+    let mut missing: Vec<String> = descriptors.filter(|d| !is_locked(d)).collect();
     missing.sort();
     missing.dedup();
     missing
@@ -213,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<String> = 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

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,82 @@ pub fn yarn_classic_artifact_targets<'a>(
     if crate::patch::redirect::yarn_classic_hosted_refused(files, yarn_outer) {
         return Vec::new();
     }
+    select_classic_artifacts(candidates, lock, classic_registry_copies)
+}
+
+/// [`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, 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 mut pinned: Option<HashSet<&str>> = None;
+    let mut read_copies = Some(read_copies);
+    let mut registry_copies: Option<HashSet<(String, String)>> = 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()
+                || !pinned
+                    .get_or_insert_with(|| quoted_fragment_urls(lock))
+                    .contains(dep.artifact_url.as_str())
         })
         .filter(|dep| {
-            dep.integrity.sha1.is_none() || !lock.contains(&format!("\"{}#", dep.artifact_url))
+            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()
 }
 
-/// Whether a classic `lock` has a registry block of `name@version`: the
-/// only copy a hosted pin rewrites (a git, `file:`, `link:` or remote
+/// Every `<url>` that `text` writes as `"<url>#…`: 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
 /// 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 +3130,104 @@ 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,
+        }
+    }
+
+    #[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
+    /// at all on a rescan whose deps are all pinned already.
+    #[test]
+    fn classic_artifact_targets_read_the_lock_blocks_at_most_once() {
+        let mut lock = String::from("# yarn lockfile v1\n\n");
+        let n = 200;
+        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<Candidate> = (0..n)
+            .map(|i| classic_candidate(&format!("pkg-{i}"), &format!("1.0.{i}"), Some("s1")))
+            .collect();
+        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));
+        }
+        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 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 got: Vec<String> = 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));
+
+        // 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
+        );
     }
 
     #[test]

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<bool> = 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<std::collections::HashSet<&str>> = None;
     let mut segments: Option<ClassicSegments> = 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::<Value>(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