Keep the hosted pin when a vendored takeover is refused (#853, #944)

crates/socket-patch-cli/src/commands/vendor.rs

@@ -121,6 +121,114 @@ fn linked_vendor_dir_refusal(project_root: &Path, purl: &str, uuid: &str) -> Opt
         .map(|link| vendor::path::vendor_dir_symlink_detail(&link))
 }
 
+/// A wet hosted → vendored takeover held open until the backend's outcome
+/// is known: the group-commit savepoint taken before the upstream restore,
+/// and what the restore reported, recorded only once the restore stands.
+struct TakeoverUndo {
+    savepoint: Option<socket_patch_core::utils::group_commit::Savepoint>,
+    advisories: Vec<VendorWarning>,
+    vlt_targets: Vec<socket_patch_core::patch::redirect::vlt_heal::LedgerTarget>,
+}
+
+impl TakeoverUndo {
+    /// The purl is not vendored: roll the restore back in `group`'s
+    /// overlay, so the hosted pin stays and nothing of the restore is
+    /// reported.
+    fn abandon(self, group: Option<&GroupCommit>) {
+        if let (Some(savepoint), Some(group)) = (self.savepoint, group) {
+            group.rollback_to(savepoint);
+        }
+    }
+
+    /// The restore stands: record its advisories and queue the vlt heal.
+    fn settle(
+        self,
+        env: &mut Envelope,
+        common: &GlobalArgs,
+        candidate: &str,
+        vlt_takeover_targets: &mut HashMap<
+            String,
+            Vec<socket_patch_core::patch::redirect::vlt_heal::LedgerTarget>,
+        >,
+    ) {
+        if !self.vlt_targets.is_empty() {
+            vlt_takeover_targets.insert(candidate.to_string(), self.vlt_targets);
+        }
+        for advisory in &self.advisories {
+            record_warning(env, candidate, advisory, common);
+        }
+    }
+}
+
+/// The dry-run twin of the wet takeover's rollback: the vendored backend's
+/// refusal over the project as `restore` would leave it, or `None` when it
+/// would vendor (or the restored project cannot be previewed). The
+/// restored text is staged in a throwaway group commit that is never
+/// committed, so nothing reaches the disk: a restore touching a file the
+/// overlay does not capture, or a binary lock (no staged text), is not
+/// previewed.
+#[allow(clippy::too_many_arguments)]
+async fn takeover_dry_refusal(
+    restore: &socket_patch_core::patch::redirect::upstream::RestoreOutcome,
+    purl: &str,
+    pkg_path: PackageSource<'_>,
+    project_root: &Path,
+    record: &PatchRecord,
+    sources: &PatchSources<'_>,
+    vendored_at: &str,
+    force: bool,
+    service: Option<&VendorServiceConfig>,
+    pipenv_version: &tokio::sync::OnceCell<Option<u32>>,
+    installed_sites: &vendor::pypi::InstalledSiteListings,
+) -> Option<(&'static str, String)> {
+    if restore.reverted_files.is_empty()
+        || !restore.reverted_files.iter().all(|f| {
+            socket_patch_core::utils::group_commit::captures(f)
+                && restore.staged_text.contains_key(f)
+        })
+    {
+        return None;
+    }
+    let probe = GroupCommit::begin(project_root);
+    for (rel, text) in &restore.staged_text {
+        let path = project_root.join(rel);
+        let staged = match text {
+            Some(text) => {
+                socket_patch_core::utils::fs::atomic_write_bytes_preserving_mode(
+                    &path,
+                    text.as_bytes(),
+                )
+                .await
+            }
+            None => socket_patch_core::utils::fs::remove_file(&path).await,
+        };
+        if staged.is_err() {
+            return None;
+        }
+    }
+    let outcome = Box::pin(dispatch_vendor_one(
+        purl,
+        pkg_path,
+        project_root,
+        record,
+        sources,
+        vendored_at,
+        true,
+        force,
+        service,
+        pipenv_version,
+        installed_sites,
+    ))
+    .await;
+    drop(probe);
+    match outcome {
+        Some(VendorOutcome::Refused { code, detail }) if !refusal_is_benign(code) => {
+            Some((code, detail))
+        }
+        _ => None,
+    }
+}
+
 /// Dispatch one purl to its ecosystem backend. `pkg_path` is the crawler's
 /// installed location (site-packages root for pypi, the package dir
 /// otherwise), or a fetched artifact the backend materialises only if it
@@ -2579,6 +2687,11 @@ pub(crate) async fn vendor_records_reusing(
             // vendor detach the PRISTINE registry entry to record. A purl
             // whose upstream entry cannot be restored is REFUSED; the cargo
             // backend's `hosted_redirect_live` guard backstops the rest.
+            // A wet takeover whose restore stayed in the group commit's
+            // overlay: the point to roll back to when the backend below
+            // does not vendor the purl, and the advisories that only hold
+            // once it does (see `TakeoverUndo`).
+            let mut takeover_undo: Option<TakeoverUndo> = None;
             if let Some(pin) = hosted_pin_of(candidate) {
                 let origins = crate::commands::rollback::patch_server_origins(common);
                 let restore_opts = socket_patch_core::patch::redirect::upstream::RestoreOptions {
@@ -2664,18 +2777,33 @@ pub(crate) async fn vendor_records_reusing(
                         )
                     })
                     .unwrap_or_default();
+                let savepoint = group.as_ref().map(GroupCommit::savepoint);
                 let restore = socket_patch_core::patch::redirect::upstream::restore_upstream(
                     &common.cwd,
                     std::slice::from_ref(pin),
                     &restore_opts,
                 )
                 .await;
+                // Undoable only when every file the restore wrote is still
+                // in the overlay (a `.socket/gradle/hosted-index.tsv` is
+                // written straight to disk).
+                let savepoint = savepoint.filter(|_| {
+                    restore
+                        .reverted_files
+                        .iter()
+                        .all(|f| socket_patch_core::utils::group_commit::captures(f))
+                });
                 let refusal = restore
                     .refused()
                     .map(|(_, why)| why.to_string())
                     .next()
                     .or_else(|| restore.flush_error.clone());
                 if let Some(detail) = refusal {
+                    // A flush that failed partway may have staged some of
+                    // the restore: put the hosted wiring back.
+                    if let (Some(savepoint), Some(group)) = (savepoint, group.as_ref()) {
+                        group.rollback_to(savepoint);
+                    }
                     has_errors = true;
                     env.record(
                         PatchEvent::new(PatchAction::Failed, candidate.clone()).with_error(
@@ -2690,15 +2818,41 @@ pub(crate) async fn vendor_records_reusing(
                     );
                     continue;
                 }
-                for (code, detail) in &restore.warnings {
-                    record_warning(
-                        env,
-                        candidate,
-                        &VendorWarning::new(code, detail.clone()),
-                        common,
-                    );
-                }
+                let mut advisories: Vec<VendorWarning> = restore
+                    .warnings
+                    .iter()
+                    .map(|(code, detail)| VendorWarning::new(code, detail.clone()))
+                    .collect();
                 if common.dry_run {
+                    // The refusal the backend would raise over the restored
+                    // project, previewed here with the wet run's code (the
+                    // wet run rolls the restore back on it, below).
+                    if let Some((code, detail)) = takeover_dry_refusal(
+                        &restore,
+                        candidate,
+                        pkg_source,
+                        &common.cwd,
+                        record,
+                        sources,
+                        &vendored_at,
+                        force,
+                        service,
+                        &pipenv_version,
+                        &installed_sites,
+                    )
+                    .await
+                    {
+                        has_errors = true;
+                        env.record(
+                            PatchEvent::new(PatchAction::Failed, candidate.clone())
+                                .with_error(code, detail.clone()),
+                        );
+                        report_vendor_failure(common, candidate, &detail);
+                        continue;
+                    }
+                    for advisory in &advisories {
+                        record_warning(env, candidate, advisory, common);
+                    }
                     record_warning(
                         env,
                         candidate,
@@ -2726,23 +2880,25 @@ pub(crate) async fn vendor_records_reusing(
                         continue;
                     }
                 } else {
-                    if !targets.is_empty() {
-                        vlt_takeover_targets.insert(candidate.clone(), targets);
-                    }
-                    record_warning(
-                        env,
-                        candidate,
-                        &VendorWarning::new(
-                            "vendor_takeover_reverted_redirect",
-                            format!(
-                                "{} was hosted; restored its upstream registry entry ({}) \
-                                 before vendoring (mode takeover)",
-                                normalize_purl(candidate),
-                                restore.reverted_files.join(", ")
-                            ),
+                    advisories.push(VendorWarning::new(
+                        "vendor_takeover_reverted_redirect",
+                        format!(
+                            "{} was hosted; restored its upstream registry entry ({}) \
+                             before vendoring (mode takeover)",
+                            normalize_purl(candidate),
+                            restore.reverted_files.join(", ")
                         ),
-                        common,
-                    );
+                    ));
+                    let undo = TakeoverUndo {
+                        savepoint,
+                        advisories,
+                        vlt_targets: targets,
+                    };
+                    if undo.savepoint.is_some() {
+                        takeover_undo = Some(undo);
+                    } else {
+                        undo.settle(env, common, candidate, &mut vlt_takeover_targets);
+                    }
                 }
             }
 
@@ -2801,6 +2957,9 @@ pub(crate) async fn vendor_records_reusing(
                                     .with_error("vendor_redownload_failed", detail.clone()),
                             );
                             report_vendor_failure(common, candidate, &detail);
+                            if let Some(undo) = takeover_undo.take() {
+                                undo.abandon(group.as_ref());
+                            }
                             continue;
                         }
                     }
@@ -2830,6 +2989,19 @@ pub(crate) async fn vendor_records_reusing(
             status.finish();
             let vendored =
                 matches!(&outcome, Some(VendorOutcome::Done { result, .. }) if result.success);
+            if let Some(undo) = takeover_undo.take() {
+                // A takeover the backend did not carry through keeps the
+                // hosted pin: its restore is rolled back in the overlay, so
+                // the purl is never left un-hosted AND unvendored (#853,
+                // #944). One the backend recorded keeps the restore.
+                let recorded =
+                    matches!(&outcome, Some(VendorOutcome::Done { entry, .. }) if entry.is_some());
+                if vendored || recorded || undo.savepoint.is_none() || group.is_none() {
+                    undo.settle(env, common, candidate, &mut vlt_takeover_targets);
+                } else {
+                    undo.abandon(group.as_ref());
+                }
+            }
 
             match outcome {
                 None => {

crates/socket-patch-cli/tests/in_process_vendor_bun_takeover/vlt.rs

@@ -496,18 +496,22 @@ async fn vlt_hosted_artifact_preflight_refuses_before_the_vendored_revert() {
     assert!(root.join(rel()).join("index.js").is_file());
 }
 
-/// A vendor that fails after the takeover's upstream restore was persisted
-/// (here a patch-service artifact failing its integrity check) still heals
-/// the hosted store copy against the restored registry pin: the lock no
-/// longer pins anything hosted, so no later run could find it again.
+/// A vendor that fails after the takeover's upstream restore (here a
+/// patch-service artifact failing its integrity check) rolls the restore
+/// back (#853, #944): the purl stays hosted-patched, the lock and the
+/// hosted store copy are left as hosted mode wrote them, and nothing asks
+/// for a reinstall.
 #[tokio::test(flavor = "multi_thread")]
-async fn vlt_failed_vendor_after_the_takeover_revert_still_heals_the_store() {
+async fn vlt_failed_vendor_after_the_takeover_revert_keeps_the_hosted_pin() {
     let tmp = tempfile::tempdir().unwrap();
     let root = tmp.path();
     let server = MockServer::start().await;
     mock_api(&server).await;
     hosted_project(root, &server).await;
     seed_manifest(root);
+    let lock = hosted::read(root, "vlt-lock.json");
+    let pkg = hosted::read(root, "package.json");
+    let store = hosted::store_dir(root, TILDE_ID).exists();
 
     let service = MockServer::start().await;
     hosted::mock_reference_at(
@@ -536,27 +540,30 @@ async fn vlt_failed_vendor_after_the_takeover_revert_still_heals_the_store() {
     assert_eq!(code, 1, "{env:#}\n{stderr}");
     let codes = all_codes(&env);
     assert!(
-        codes.contains(&"vendor_takeover_reverted_redirect".to_string()),
-        "{env:#}"
+        !codes.contains(&"vendor_takeover_reverted_redirect".to_string()),
+        "the restore is rolled back, never reported: {env:#}"
     );
     assert!(
         detail_of(&env, "apply_failed").contains("integrity"),
         "{env:#}"
     );
+    assert!(
+        !codes.contains(&"redirect_vlt_reinstall_required".to_string()),
+        "{env:#}"
+    );
     assert_eq!(
-        detail_of(&env, "redirect_vlt_reinstall_required"),
-        "restored registry pins for 1 packages; removed the patched installed copies, so \
-         node_modules is incomplete until you run `vlt install` (or `vlt ci`)"
+        hosted::read(root, "vlt-lock.json"),
+        lock,
+        "the hosted pin stays"
     );
-    assert_eq!(hosted::read(root, "vlt-lock.json"), registry_lock());
-    assert_eq!(hosted::read(root, "package.json"), PACKAGE_JSON);
+    assert_eq!(hosted::read(root, "package.json"), pkg);
     assert!(vendor_entry(root).is_none());
     assert_no_redirect_ledger(root);
-    assert!(
-        !hosted::store_dir(root, TILDE_ID).exists(),
-        "the hosted store copy is invalidated"
+    assert_eq!(
+        hosted::store_dir(root, TILDE_ID).exists(),
+        store,
+        "the hosted store copy is left alone"
     );
-    assert!(!root.join("node_modules/.vlt-lock.json").exists());
 }
 
 /// An optional hosted pin taken over by `scan --mode vendored`: the