From fca2ddb4c1d97cc5226bf6a509b34660aacbfabe Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 15:37:54 +1100 Subject: [PATCH 1/2] fix(staged): cap the private npm cache so ~/.staged/packages stops growing Nothing ever deleted from the npm download cache at ~/.staged/packages/npm-prefix/cache/_cacache. Every floating bridge install added the new version's tarballs beside all the old ones, and npm left an orphaned body behind for each package document it revalidated, so the cache grew by one to two gigabytes a month. The reconcile epilogue now wipes `_cacache` whole once it holds more than 1 GiB. The cache has no correctness role, so the only cost is one re-download of the bridges' tarballs on the next pass. - Add `managed_acp_tools/cache.rs` with the single definition of the cache path, the 1 GiB cap and `prune_npm_cache_at`. The prune removes `_cacache.trash-*` leftovers from an interrupted run, sums file sizes without following symlinks, and renames an oversized `_cacache` aside before deleting it so npm never sees a half-deleted cache. It is best-effort and logs failures at warn level. - Call it from `finish_reconcile_at` inside the locked scope, after the outcome is recorded, on the blocking pool. It runs on every pass, not only successful ones, so a bridge that keeps failing cannot switch the cleanup off. - Use the cache path helper in the bridge installer and in the managed npm env pairs. - Lock Doctor's npm-backed fixes and updates, and the deprecated-bridge fix, with a new `NpmWriteLock` that takes the tool-install mutex and then the packages flock. It is taken only after the managed Node runtime ensure returns, because that call takes the same flock. A fix can now wait behind another instance's in-flight install. - The `npm view` and registry probes stay unlocked: one that races a wipe fails once and already reports the latest version as unknown. - Describe the cap in the `managed_acp_tools` layout comment and the reconciler header. No fields are added to `state.json`, and `node/`, `tools/`, `_logs` and `npm-prefix/lib` are left alone. Tests cover a cache under, at and over the cap, survival of everything else under the packages root, leftover trash removal, a missing cache, a `_cacache` that is a plain file or a symlink, symlinks inside the cache, pruning on a failed pass, and the epilogue waiting for a second descriptor's flock before it prunes. All use temporary directories. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- .../src-tauri/src/acp_tools_reconciler.rs | 7 + apps/staged/src-tauri/src/doctor/mod.rs | 42 +++- .../src-tauri/src/managed_acp_tools/cache.rs | 188 ++++++++++++++++ .../src/managed_acp_tools/install.rs | 3 +- .../src-tauri/src/managed_acp_tools/lock.rs | 48 +++++ .../src-tauri/src/managed_acp_tools/mod.rs | 12 +- .../src/managed_acp_tools/reconcile.rs | 37 +++- .../src/managed_acp_tools/tests/cache.rs | 202 ++++++++++++++++++ .../src/managed_acp_tools/tests/lock.rs | 19 ++ .../src/managed_acp_tools/tests/reconcile.rs | 98 ++++++++- .../src/managed_acp_tools/tests/support.rs | 51 +++++ .../src/managed_acp_tools/tests/validation.rs | 9 +- apps/staged/src-tauri/src/managed_node.rs | 2 +- 13 files changed, 697 insertions(+), 21 deletions(-) create mode 100644 apps/staged/src-tauri/src/managed_acp_tools/cache.rs create mode 100644 apps/staged/src-tauri/src/managed_acp_tools/lock.rs create mode 100644 apps/staged/src-tauri/src/managed_acp_tools/tests/cache.rs create mode 100644 apps/staged/src-tauri/src/managed_acp_tools/tests/lock.rs diff --git a/apps/staged/src-tauri/src/acp_tools_reconciler.rs b/apps/staged/src-tauri/src/acp_tools_reconciler.rs index 57365b8a0..d565f1d6d 100644 --- a/apps/staged/src-tauri/src/acp_tools_reconciler.rs +++ b/apps/staged/src-tauri/src/acp_tools_reconciler.rs @@ -15,6 +15,13 @@ //! shim execs its Node by absolute versioned path, so an old runtime must //! outlive the last shim that references it. //! +//! The same epilogue caps the private npm download cache +//! (`~/.staged/packages/npm-prefix/cache/_cacache`), which npm itself never +//! evicts from: once it holds more than 1 GiB it is wiped whole, under the +//! install locks, on every pass — a failed one included, so a bridge that +//! keeps failing cannot switch the cleanup off. The cost is one re-download +//! of the bridges' tarballs on the following pass. +//! //! Silent when there is nothing to manage: the `STAGED_ACP_TOOLS_DIR` dev //! override is active, the `no-managed-acp-tools` build feature is set, or //! the target is unsupported. diff --git a/apps/staged/src-tauri/src/doctor/mod.rs b/apps/staged/src-tauri/src/doctor/mod.rs index f4966e5cd..f7acdf0fc 100644 --- a/apps/staged/src-tauri/src/doctor/mod.rs +++ b/apps/staged/src-tauri/src/doctor/mod.rs @@ -355,15 +355,17 @@ pub async fn run_doctor_fix( // check definition, so the npm-backed gate below can't see it — ensure the // managed runtime here, since the fix shells out to npm. if check_id == DEPRECATED_CLAUDE_ACP_CHECK_ID { - ensure_managed_node_runtime_for_fix().await?; + let _npm_write_lock = prepare_npm_fix().await?; return run_deprecated_claude_acp_fix(fix_type, env_vars).await; } let npm_backed = doctor::agents::lookup_fix_command(&check_id, &fix_type) .as_deref() .is_some_and(crate::managed_acp_tools::is_npm_backed_command); - if npm_backed { - ensure_managed_node_runtime_for_fix().await?; - } + let _npm_write_lock = if npm_backed { + Some(prepare_npm_fix().await?) + } else { + None + }; execute_fix_with_gateway_fallback(check_id, fix_type, None, npm_backed, env_vars).await } @@ -436,6 +438,27 @@ async fn ensure_managed_node_runtime_for_fix() -> Result<(), String> { .map_err(|error| error.to_string()) } +/// Everything an npm-backed fix or update needs before its command runs: +/// the managed Node.js runtime, then the lock that keeps the shared packages +/// tree to one npm writer at a time (see +/// [`crate::managed_acp_tools::lock_npm_writes`]). The fix's `npm install -g` +/// writes the private prefix and its download cache, so without the lock it +/// could overlap a bridge install or have the reconciler's cache prune land +/// mid-install — in this or another Staged instance. The caller holds the +/// returned guard until the command, gateway retry included, has finished. +/// +/// The order is load-bearing: the runtime ensure takes the packages flock +/// itself, so the lock is acquired only after it returns — nesting the two +/// would deadlock. Fixes that go through the managed installer +/// ([`install_managed_tool_logged`]) must not call this at all, since the +/// installer takes the same locks. +async fn prepare_npm_fix() -> Result { + ensure_managed_node_runtime_for_fix().await?; + crate::managed_acp_tools::lock_npm_writes() + .await + .map_err(|error| error.to_string()) +} + /// Run a source-aware update for a single readout (main CLI or ACP bridge). /// /// Unlike [`run_doctor_fix`], update commands (`UpdateMain`/`UpdateBridge`) are @@ -483,11 +506,14 @@ pub async fn run_doctor_update( )); } // npm-backed updates run the managed npm into the private prefix, so the - // managed runtime must exist before the command does. + // managed runtime must exist before the command does — and the command + // runs under the npm write lock. let npm_backed = crate::managed_acp_tools::is_npm_backed_command(&expected); - if npm_backed { - ensure_managed_node_runtime_for_fix().await?; - } + let _npm_write_lock = if npm_backed { + Some(prepare_npm_fix().await?) + } else { + None + }; // Run the backend-derived `expected`, not the frontend-supplied `command`. // They are equal past the guard above, but executing `expected` makes the // command that runs provably the one the backend derived — no dependence on diff --git a/apps/staged/src-tauri/src/managed_acp_tools/cache.rs b/apps/staged/src-tauri/src/managed_acp_tools/cache.rs new file mode 100644 index 000000000..e8e488276 --- /dev/null +++ b/apps/staged/src-tauri/src/managed_acp_tools/cache.rs @@ -0,0 +1,188 @@ +//! The private npm download cache (`npm-prefix/cache`) and its size cap. +//! +//! Every npm invocation Staged spawns — bridge installs, Doctor's +//! `npm install -g` fixes, the registry probes — shares one cacache under +//! `/npm-prefix/cache/_cacache`. npm never evicts from it: each +//! floating install adds the new version's tarballs beside every older one, +//! and each revalidated package document leaves its previous body behind +//! unreferenced. The cache has no correctness role — a bridge at runtime +//! never reads it, and an install against an empty cache only re-downloads — +//! so the reconcile epilogue wipes it whole once it outgrows +//! [`NPM_CACHE_MAX_BYTES`] (see [`prune_npm_cache_at`]). + +use std::path::{Path, PathBuf}; + +#[cfg(test)] +#[path = "tests/cache.rs"] +mod tests; + +/// Size cap for the private npm cache. A cache refilled by one reconcile +/// pass is roughly 0.4 GB, so this leaves room for a couple of weeks of +/// bridge releases between wipes. +pub(super) const NPM_CACHE_MAX_BYTES: u64 = 1024 * 1024 * 1024; + +/// npm's content-addressed store inside the cache dir — the only part that +/// grows without bound (`_logs` beside it is capped by npm itself). +const CACACHE_DIRNAME: &str = "_cacache"; + +/// Prefix of the sibling an oversized `_cacache` is renamed to before it is +/// deleted (`_cacache.trash-`). +const CACACHE_TRASH_PREFIX: &str = "_cacache.trash-"; + +/// `/cache` — the one definition of where the private npm cache +/// lives, shared by the env pairs Doctor's fixes run under and the bridge +/// installer. +pub(super) fn npm_cache_dir_in_prefix(npm_prefix: &Path) -> PathBuf { + npm_prefix.join("cache") +} + +/// `/npm-prefix/cache` — [`npm_cache_dir_in_prefix`] for a +/// packages root. +pub(super) fn npm_cache_dir(packages_root: &Path) -> PathBuf { + npm_cache_dir_in_prefix(&packages_root.join("npm-prefix")) +} + +/// Wipe the private npm cache's `_cacache` when it holds more than +/// `max_bytes`. Best-effort: returns nothing and logs every failure at warn +/// level, since a cache that could not be pruned costs disk, never +/// correctness, and is retried on the next reconcile pass. +/// +/// Three moves: +/// +/// 1. Remove any `_cacache.trash-*` sibling an interrupted prune left behind. +/// 2. Sum the file sizes under `_cacache` without following symlinks, and +/// stop when the total is within the cap. +/// 3. Rename `_cacache` to `_cacache.trash-` and delete that. The rename +/// is atomic, so npm sees either the whole cache or none of it — never a +/// half-deleted one whose index points at missing content. +/// +/// The caller holds the tool-install mutex and the packages flock, so no +/// Staged-spawned npm install is writing the cache meanwhile. Blocking +/// filesystem work: call it from `spawn_blocking`. +pub(super) fn prune_npm_cache_at(packages_root: &Path, max_bytes: u64) { + let cache_dir = npm_cache_dir(packages_root); + remove_leftover_trash(&cache_dir); + + let cacache = cache_dir.join(CACACHE_DIRNAME); + match std::fs::symlink_metadata(&cacache) { + Ok(meta) if meta.is_dir() => {} + // A plain file or a symlink where npm's store should be is not ours + // to size or delete; npm reports it on the next install. + Ok(_) => return, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return, + Err(error) => { + log::warn!( + "skipping npm cache prune: cannot inspect {}: {error}", + cacache.display() + ); + return; + } + } + + let size = dir_size(&cacache); + if size <= max_bytes { + return; + } + + let trash = cache_dir.join(format!("{CACACHE_TRASH_PREFIX}{}", std::process::id())); + if let Err(error) = std::fs::rename(&cacache, &trash) { + log::warn!( + "failed to prune the npm cache ({size} bytes, cap {max_bytes}): move {} aside: {error}", + cacache.display() + ); + return; + } + match std::fs::remove_dir_all(&trash) { + Ok(()) => log::info!( + "pruned the npm cache at {}: reclaimed {size} bytes (cap {max_bytes} bytes)", + cache_dir.display() + ), + // The cache itself is already gone from npm's point of view; what is + // left of the trash dir is swept by the next prune. + Err(error) => log::warn!( + "npm cache ({size} bytes, cap {max_bytes}) was moved aside but {} could not be fully deleted: {error}", + trash.display() + ), + } +} + +/// Remove every `_cacache.trash-*` entry in the cache dir — what a prune that +/// was interrupted (crash, quit, power loss) between its rename and the end +/// of its delete left behind. +fn remove_leftover_trash(cache_dir: &Path) { + let entries = match std::fs::read_dir(cache_dir) { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return, + Err(error) => { + log::warn!( + "cannot scan {} for interrupted npm cache prunes: {error}", + cache_dir.display() + ); + return; + } + }; + for entry in entries.flatten() { + if !entry + .file_name() + .to_string_lossy() + .starts_with(CACACHE_TRASH_PREFIX) + { + continue; + } + let path = entry.path(); + // `file_type` does not follow symlinks, so a symlink named like a + // trash dir is unlinked rather than traversed. + let result = match entry.file_type() { + Ok(file_type) if file_type.is_dir() => std::fs::remove_dir_all(&path), + Ok(_) => std::fs::remove_file(&path), + Err(error) => Err(error), + }; + if let Err(error) = result { + log::warn!( + "failed to remove interrupted npm cache prune leftover {}: {error}", + path.display() + ); + } + } +} + +/// Total size in bytes of the regular files under `dir`. Symlinks are +/// counted as nothing and never followed, so a link out of the cache can +/// neither inflate the total nor send the walk elsewhere. An unreadable +/// entry is logged and skipped — the total is then a lower bound, which can +/// only delay a prune. +fn dir_size(dir: &Path) -> u64 { + let mut total = 0u64; + let mut pending = vec![dir.to_path_buf()]; + while let Some(current) = pending.pop() { + let entries = match std::fs::read_dir(¤t) { + Ok(entries) => entries, + Err(error) => { + log::warn!( + "npm cache sizing skipped unreadable {}: {error}", + current.display() + ); + continue; + } + }; + for entry in entries.flatten() { + // `DirEntry::metadata` does not traverse symlinks. + let meta = match entry.metadata() { + Ok(meta) => meta, + Err(error) => { + log::warn!( + "npm cache sizing skipped unreadable {}: {error}", + entry.path().display() + ); + continue; + } + }; + if meta.is_dir() { + pending.push(entry.path()); + } else if meta.is_file() { + total = total.saturating_add(meta.len()); + } + } + } + total +} diff --git a/apps/staged/src-tauri/src/managed_acp_tools/install.rs b/apps/staged/src-tauri/src/managed_acp_tools/install.rs index 0d727a246..f3c9897cd 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/install.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/install.rs @@ -9,6 +9,7 @@ use tokio::io::AsyncBufReadExt; use crate::managed_node; +use super::cache::npm_cache_dir; use super::probe::snapshot_var; use super::registry::{ fallback_npm_registry, gateway_fallback_registry, gateway_retry_failed_message, @@ -514,7 +515,7 @@ async fn run_floating_npm_install_capturing( } // Share the private prefix's download cache; `--prefix` on the command // line outranks any inherited prefix config. - let cache = packages_root.join("npm-prefix").join("cache"); + let cache = npm_cache_dir(packages_root); command.env("NPM_CONFIG_CACHE", &cache); command.env("npm_config_cache", &cache); command diff --git a/apps/staged/src-tauri/src/managed_acp_tools/lock.rs b/apps/staged/src-tauri/src/managed_acp_tools/lock.rs new file mode 100644 index 000000000..c962b5cc0 --- /dev/null +++ b/apps/staged/src-tauri/src/managed_acp_tools/lock.rs @@ -0,0 +1,48 @@ +//! The combined lock for npm writers outside the managed installer. + +use std::path::Path; + +use crate::managed_node::{self, ManagedNodeError, PackagesDirLock}; + +use super::install::tool_install_lock; + +#[cfg(test)] +#[path = "tests/lock.rs"] +mod tests; + +/// Held while something other than the managed installer writes the shared +/// packages tree through npm — Doctor's `npm install -g` fixes and updates. +/// It is the same pair the installer and the reconcile epilogue hold (the +/// in-process tool-install mutex, then the cross-process packages flock), so +/// such a fix can neither overlap a bridge install nor have the npm cache +/// pruned out from under it (see `cache::prune_npm_cache_at`), in this or any +/// other Staged process sharing the tree. +pub(crate) struct NpmWriteLock { + // Field order is drop order: the flock is released before the mutex, the + // reverse of acquisition. + _packages_lock: PackagesDirLock, + _guard: tokio::sync::MutexGuard<'static, ()>, +} + +/// Take the [`NpmWriteLock`] for the real `~/.staged/packages`. Waits for +/// whatever holds either lock — worst case a whole bridge install in another +/// Staged instance. Never call while a [`PackagesDirLock`] is held, and only +/// after `managed_node::ensure_managed_node_runtime` has returned: that call +/// takes the same flock, and nesting it deadlocks (see +/// `managed_node::lock_packages_dir`). +pub(crate) async fn lock_npm_writes() -> Result { + let packages_root = crate::paths::packages_dir() + .ok_or_else(|| ManagedNodeError::DataDir("home directory is unavailable".to_string()))?; + lock_npm_writes_at(&packages_root).await +} + +pub(super) async fn lock_npm_writes_at( + packages_root: &Path, +) -> Result { + let guard = tool_install_lock().lock().await; + let packages_lock = managed_node::lock_packages_dir(packages_root).await?; + Ok(NpmWriteLock { + _packages_lock: packages_lock, + _guard: guard, + }) +} diff --git a/apps/staged/src-tauri/src/managed_acp_tools/mod.rs b/apps/staged/src-tauri/src/managed_acp_tools/mod.rs index 1b1d1fae5..c0b1d71f7 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/mod.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/mod.rs @@ -24,6 +24,11 @@ //! module's in-process tool-install mutex): //! //! - `npm-prefix/` — the private npm global prefix. +//! - `npm-prefix/cache/` — the npm download cache every Staged-spawned npm +//! shares. npm never evicts from it, so the reconcile epilogue wipes its +//! `_cacache` whole once it outgrows 1 GiB (see `cache`); the next install +//! just re-downloads. Doctor's npm fixes hold the same locks +//! ([`lock_npm_writes`]) so a wipe never lands mid-install. //! - `node///` — managed Node runtimes (`managed_node`). //! - `tools//` — per-bridge npm `--prefix` trees for the managed ACP //! bridges (claude-acp, codex-acp). @@ -43,7 +48,9 @@ use std::path::{Path, PathBuf}; use crate::managed_node; +mod cache; mod install; +mod lock; mod probe; mod reconcile; mod registry; @@ -57,6 +64,7 @@ pub(crate) mod test_support; mod tests; pub use install::{install_managed_tool, InstallLineFn, ManagedToolError}; +pub(crate) use lock::{lock_npm_writes, NpmWriteLock}; pub(crate) use probe::configured_npm_registry; pub use probe::{installed_tool_version, latest_published_version}; pub(crate) use reconcile::{finish_reconcile, now_ms}; @@ -224,7 +232,9 @@ pub fn managed_npm_env() -> Vec<(String, String)> { pub fn managed_npm_env_at(prefix: &Path) -> Vec<(String, String)> { let prefix_value = prefix.to_string_lossy().into_owned(); - let cache_value = prefix.join("cache").to_string_lossy().into_owned(); + let cache_value = cache::npm_cache_dir_in_prefix(prefix) + .to_string_lossy() + .into_owned(); let corepack_value = prefix.join("corepack").to_string_lossy().into_owned(); vec![ ("NPM_CONFIG_PREFIX".to_string(), prefix_value.clone()), diff --git a/apps/staged/src-tauri/src/managed_acp_tools/reconcile.rs b/apps/staged/src-tauri/src/managed_acp_tools/reconcile.rs index 95ac3b0e5..87fc9bedd 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/reconcile.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/reconcile.rs @@ -1,10 +1,11 @@ -//! Reconcile epilogue — prune stale ids, record the outcome, gate the Node -//! prune. +//! Reconcile epilogue — prune stale ids, record the outcome, cap the npm +//! cache, gate the Node prune. use std::path::Path; use crate::managed_node; +use super::cache::{prune_npm_cache_at, NPM_CACHE_MAX_BYTES}; use super::install::tool_install_lock; use super::{read_state, shim_bin_dir, tools_root, write_state, ManagedTool, ReconcileRecord}; @@ -13,26 +14,31 @@ use super::{read_state, shim_bin_dir, tools_root, write_state, ManagedTool, Reco mod tests; // ============================================================================= -// Reconcile epilogue — prune stale ids, record the outcome, gate the Node prune +// Reconcile epilogue — prune stale ids, record the outcome, cap the npm cache, +// gate the Node prune // ============================================================================= /// Reconcile epilogue: drop installs for ids no longer in the managed set /// (their shims, tool dirs, and state entries), record the run's outcome in -/// `state.json`, and — only when every managed bridge installed cleanly — -/// prune superseded managed Node runtimes. Takes the install mutex and the -/// cross-process flock so it cannot race an in-flight install in this or any -/// other Staged process. +/// `state.json`, wipe the private npm cache when it has outgrown its size cap +/// (on every pass, failed or not), and — only when every managed bridge +/// installed cleanly — prune superseded managed Node runtimes. Takes the +/// install mutex and the cross-process flock so it cannot race an in-flight +/// install in this or any other Staged process. pub(crate) async fn finish_reconcile(managed: &[ManagedTool], errors: Vec) { let Some(packages_root) = crate::paths::packages_dir() else { return; }; - finish_reconcile_at(&packages_root, managed, errors).await; + finish_reconcile_at(&packages_root, managed, errors, NPM_CACHE_MAX_BYTES).await; } +/// [`finish_reconcile`] for a given packages root; `npm_cache_max_bytes` is +/// the npm cache cap, a parameter so tests can exceed it with a few bytes. pub(super) async fn finish_reconcile_at( packages_root: &Path, managed: &[ManagedTool], errors: Vec, + npm_cache_max_bytes: u64, ) { let all_installed = errors.is_empty(); { @@ -46,6 +52,21 @@ pub(super) async fn finish_reconcile_at( }; prune_stale_managed_tools(packages_root, managed); record_reconcile(packages_root, errors); + // Cap the npm cache while the locks still exclude every Staged-spawned + // npm install. Deliberately not gated on `all_installed`: a bridge + // that fails every day would otherwise switch cleanup off while the + // cache keeps growing. It runs after the state write, so recording + // the outcome never waits on a multi-gigabyte delete, and on the + // blocking pool, so a panic in cleanup surfaces here as a join error + // instead of unwinding through the pass. + let cache_root = packages_root.to_path_buf(); + if let Err(error) = tokio::task::spawn_blocking(move || { + prune_npm_cache_at(&cache_root, npm_cache_max_bytes) + }) + .await + { + log::warn!("npm cache prune task failed: {error}"); + } } // Success-gated Node prune: `errors` empty means every managed bridge // reinstalled this run, so every shim now embeds the pinned runtime's diff --git a/apps/staged/src-tauri/src/managed_acp_tools/tests/cache.rs b/apps/staged/src-tauri/src/managed_acp_tools/tests/cache.rs new file mode 100644 index 000000000..9b2fd58a0 --- /dev/null +++ b/apps/staged/src-tauri/src/managed_acp_tools/tests/cache.rs @@ -0,0 +1,202 @@ +use super::*; +use crate::managed_acp_tools::test_support::*; +use crate::managed_acp_tools::*; + +/// Names of the entries directly inside the cache dir, sorted. +fn cache_dir_entries(packages_root: &Path) -> Vec { + let mut names: Vec = std::fs::read_dir(npm_cache_dir(packages_root)) + .unwrap() + .map(|entry| entry.unwrap().file_name().to_string_lossy().into_owned()) + .collect(); + names.sort(); + names +} + +#[test] +fn the_cache_dir_is_the_one_the_managed_npm_env_names() { + let packages_root = Path::new("/staged/packages"); + let cache_dir = npm_cache_dir(packages_root); + assert_eq!(cache_dir, packages_root.join("npm-prefix").join("cache")); + + let env = managed_npm_env_at(&packages_root.join("npm-prefix")); + for key in ["NPM_CONFIG_CACHE", "npm_config_cache"] { + let value = env + .iter() + .find(|(name, _)| name == key) + .map(|(_, value)| value.as_str()); + assert_eq!(value, Some(cache_dir.to_string_lossy().as_ref()), "{key}"); + } +} + +#[test] +fn a_cache_within_the_cap_is_untouched() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path(); + let tarball = write_cacache_file(packages_root, "content-v2/sha512/aa/bb/tarball", 60); + let index = write_cacache_file(packages_root, "index-v5/cc/dd/entry", 40); + + // Under the cap, and exactly at it: only a cache *over* the cap goes. + prune_npm_cache_at(packages_root, 1000); + prune_npm_cache_at(packages_root, 100); + + assert_eq!(std::fs::read(&tarball).unwrap().len(), 60); + assert_eq!(std::fs::read(&index).unwrap().len(), 40); + assert_eq!(cache_dir_entries(packages_root), vec!["_cacache"]); +} + +#[test] +fn a_cache_over_the_cap_is_removed_and_nothing_else_is() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path(); + write_cacache_file(packages_root, "content-v2/sha512/aa/bb/tarball", 60); + write_cacache_file(packages_root, "index-v5/cc/dd/entry", 40); + write_cacache_file(packages_root, "tmp/leftover", 1); + + // Everything else that lives under the packages root. + let npm_prefix = packages_root.join("npm-prefix"); + let npm_log = npm_cache_dir(packages_root) + .join("_logs") + .join("debug-0.log"); + let global_package = npm_prefix + .join("lib") + .join("node_modules") + .join("@github") + .join("copilot") + .join("package.json"); + let global_bin = npm_prefix.join("bin").join("copilot"); + for path in [&npm_log, &global_package, &global_bin] { + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(path, "keep").unwrap(); + } + let tool = test_tool(); + let node_install_dir = packages_root + .join("node") + .join(TEST_NODE_VERSION) + .join("plat"); + write_installed_tool(packages_root, &node_install_dir, &tool); + let state_before = std::fs::read_to_string(state_path(packages_root)).unwrap(); + + prune_npm_cache_at(packages_root, 100); + + assert!(!cacache_dir(packages_root).exists()); + // No trash dir is left behind either: `_logs` is all that remains. + assert_eq!(cache_dir_entries(packages_root), vec!["_logs"]); + for path in [&npm_log, &global_package, &global_bin] { + assert_eq!( + std::fs::read_to_string(path).unwrap(), + "keep", + "{}", + path.display() + ); + } + let install_dir = tool_install_dir(packages_root, tool.id); + assert!( + crate::managed_acp_tools::install::npm_entrypoint(&install_dir, tool.package).is_file() + ); + assert!(shim_bin_dir(packages_root).join(tool.binary).is_file()); + assert_eq!( + std::fs::read_to_string(state_path(packages_root)).unwrap(), + state_before + ); +} + +#[test] +fn leftover_trash_is_removed_even_when_the_cache_is_within_the_cap() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path(); + let tarball = write_cacache_file(packages_root, "content-v2/sha512/aa/bb/tarball", 10); + let cache_dir = npm_cache_dir(packages_root); + // An interrupted prune from another (dead) process, mid-delete. + let trash_dir = cache_dir.join("_cacache.trash-4242"); + std::fs::create_dir_all(trash_dir.join("content-v2").join("sha512")).unwrap(); + std::fs::write(trash_dir.join("content-v2").join("half-deleted"), "x").unwrap(); + let trash_file = cache_dir.join("_cacache.trash-7"); + std::fs::write(&trash_file, "x").unwrap(); + // A neighbour that only resembles the trash prefix stays. + let lookalike = cache_dir.join("_cacache.trashed"); + std::fs::create_dir_all(&lookalike).unwrap(); + + prune_npm_cache_at(packages_root, 1000); + + assert!(!trash_dir.exists()); + assert!(!trash_file.exists()); + assert!(lookalike.is_dir()); + assert_eq!(std::fs::read(&tarball).unwrap().len(), 10); +} + +#[test] +fn a_missing_cache_is_a_no_op() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + + // No packages root at all, then a cache dir with no `_cacache` in it. + prune_npm_cache_at(&packages_root, 0); + assert!(!packages_root.exists()); + + std::fs::create_dir_all(npm_cache_dir(&packages_root)).unwrap(); + prune_npm_cache_at(&packages_root, 0); + assert!(cache_dir_entries(&packages_root).is_empty()); +} + +#[test] +fn a_cacache_that_is_a_plain_file_is_left_alone() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path(); + let cacache = cacache_dir(packages_root); + std::fs::create_dir_all(cacache.parent().unwrap()).unwrap(); + std::fs::write(&cacache, "not a directory").unwrap(); + + prune_npm_cache_at(packages_root, 0); + + assert_eq!( + std::fs::read_to_string(&cacache).unwrap(), + "not a directory" + ); + assert_eq!(cache_dir_entries(packages_root), vec!["_cacache"]); +} + +#[test] +fn symlinks_inside_the_cache_are_neither_sized_nor_followed() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + // Outside the cache: far more than the cap, reachable only through links. + let outside_dir = dir.path().join("outside"); + std::fs::create_dir_all(&outside_dir).unwrap(); + let outside_file = outside_dir.join("big"); + std::fs::write(&outside_file, vec![b'x'; 5000]).unwrap(); + + let tarball = write_cacache_file(&packages_root, "content-v2/tarball", 10); + let cacache = cacache_dir(&packages_root); + std::os::unix::fs::symlink(&outside_dir, cacache.join("dir-link")).unwrap(); + std::os::unix::fs::symlink(&outside_file, cacache.join("content-v2").join("file-link")) + .unwrap(); + + // Sizing: the 5000 linked bytes do not count, so 10 bytes is within a + // 100-byte cap. + prune_npm_cache_at(&packages_root, 100); + assert!(tarball.is_file()); + assert!(cacache.join("dir-link").is_symlink()); + + // Deleting: the links go with the cache, their targets stay. + prune_npm_cache_at(&packages_root, 5); + assert!(!cacache.exists()); + assert_eq!(std::fs::read(&outside_file).unwrap().len(), 5000); +} + +#[test] +fn a_cacache_that_is_itself_a_symlink_is_left_alone() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + let elsewhere = dir.path().join("elsewhere"); + std::fs::create_dir_all(&elsewhere).unwrap(); + let elsewhere_file = elsewhere.join("tarball"); + std::fs::write(&elsewhere_file, vec![b'x'; 50]).unwrap(); + let cacache = cacache_dir(&packages_root); + std::fs::create_dir_all(cacache.parent().unwrap()).unwrap(); + std::os::unix::fs::symlink(&elsewhere, &cacache).unwrap(); + + prune_npm_cache_at(&packages_root, 0); + + assert!(cacache.is_symlink()); + assert_eq!(std::fs::read(&elsewhere_file).unwrap().len(), 50); +} diff --git a/apps/staged/src-tauri/src/managed_acp_tools/tests/lock.rs b/apps/staged/src-tauri/src/managed_acp_tools/tests/lock.rs new file mode 100644 index 000000000..46e1c915f --- /dev/null +++ b/apps/staged/src-tauri/src/managed_acp_tools/tests/lock.rs @@ -0,0 +1,19 @@ +use super::*; +use crate::managed_acp_tools::test_support::*; + +#[tokio::test] +async fn npm_write_lock_holds_the_packages_flock_until_dropped() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + + let lock = lock_npm_writes_at(&packages_root).await.unwrap(); + // Another Staged process — a bridge install, a reconcile epilogue about + // to prune the npm cache — cannot take the flock while a fix holds it. + let contender = open_packages_lock_contender(&packages_root); + assert!(!try_packages_flock(&contender)); + // Nor can anything in this process take the tool-install mutex. + assert!(tool_install_lock().try_lock().is_err()); + + drop(lock); + acquire_packages_flock(&contender).await; +} diff --git a/apps/staged/src-tauri/src/managed_acp_tools/tests/reconcile.rs b/apps/staged/src-tauri/src/managed_acp_tools/tests/reconcile.rs index d8cefb95c..ae1727a37 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/tests/reconcile.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/tests/reconcile.rs @@ -2,6 +2,10 @@ use super::*; use crate::managed_acp_tools::test_support::*; use crate::managed_acp_tools::*; +/// The npm cache cap the epilogue tests run with; fixture caches are a few +/// bytes either side of it. +const TEST_NPM_CACHE_MAX_BYTES: u64 = 100; + #[test] fn prune_removes_installs_dropped_from_the_managed_set() { let dir = tempfile::tempdir().unwrap(); @@ -52,7 +56,13 @@ async fn clean_reconcile_prunes_the_superseded_node_runtime() { let tool = test_tool(); write_installed_tool(&packages_root, &pinned_dir, &tool); - finish_reconcile_at(&packages_root, std::slice::from_ref(&tool), Vec::new()).await; + finish_reconcile_at( + &packages_root, + std::slice::from_ref(&tool), + Vec::new(), + TEST_NPM_CACHE_MAX_BYTES, + ) + .await; assert!(pinned_dir.exists()); assert!(!superseded_dir.exists()); @@ -72,6 +82,7 @@ async fn partial_failure_keeps_the_superseded_node_runtime() { &packages_root, std::slice::from_ref(&tool), vec![format!("{}: npm install failed", tool.id)], + TEST_NPM_CACHE_MAX_BYTES, ) .await; @@ -81,3 +92,88 @@ async fn partial_failure_keeps_the_superseded_node_runtime() { assert!(shim.contains(&superseded_dir.to_string_lossy().into_owned())); assert!(!read_state(&packages_root).last_reconcile.unwrap().ok); } + +#[tokio::test] +async fn failed_reconcile_still_prunes_an_oversized_npm_cache() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + let tool = test_tool(); + write_cacache_file(&packages_root, "content-v2/sha512/aa/bb/tarball", 150); + let error = format!("{}: npm install failed", tool.id); + + finish_reconcile_at( + &packages_root, + std::slice::from_ref(&tool), + vec![error.clone()], + TEST_NPM_CACHE_MAX_BYTES, + ) + .await; + + // The cleanup is not success-gated — a bridge that fails every pass must + // not switch it off — and the failed pass is still recorded. + assert!(!cacache_dir(&packages_root).exists()); + let record = read_state(&packages_root).last_reconcile.unwrap(); + assert!(!record.ok); + assert_eq!(record.errors, vec![error]); +} + +#[tokio::test] +async fn reconcile_keeps_an_npm_cache_within_the_cap() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + let tool = test_tool(); + let tarball = write_cacache_file(&packages_root, "content-v2/sha512/aa/bb/tarball", 50); + + finish_reconcile_at( + &packages_root, + std::slice::from_ref(&tool), + Vec::new(), + TEST_NPM_CACHE_MAX_BYTES, + ) + .await; + + assert!(tarball.is_file()); + assert!(read_state(&packages_root).last_reconcile.unwrap().ok); +} + +#[tokio::test] +async fn epilogue_prunes_the_npm_cache_only_once_it_holds_the_packages_flock() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + let tool = test_tool(); + write_cacache_file(&packages_root, "content-v2/sha512/aa/bb/tarball", 150); + + // Another Staged process mid-install: a second open file description + // holding the packages flock. + let contender = open_packages_lock_contender(&packages_root); + acquire_packages_flock(&contender).await; + + let epilogue = tokio::spawn({ + let packages_root = packages_root.clone(); + async move { + finish_reconcile_at( + &packages_root, + std::slice::from_ref(&tool), + Vec::new(), + TEST_NPM_CACHE_MAX_BYTES, + ) + .await; + } + }); + + // While the other holder lives the epilogue stays parked on the flock: + // nothing is recorded and the oversized cache is not touched. + tokio::time::sleep(std::time::Duration::from_millis(300)).await; + assert!(!epilogue.is_finished()); + assert!(cacache_dir(&packages_root).is_dir()); + assert!(read_state(&packages_root).last_reconcile.is_none()); + + drop(contender); + tokio::time::timeout(std::time::Duration::from_secs(30), epilogue) + .await + .expect("epilogue never finished after the packages lock was released") + .unwrap(); + + assert!(!cacache_dir(&packages_root).exists()); + assert!(read_state(&packages_root).last_reconcile.unwrap().ok); +} diff --git a/apps/staged/src-tauri/src/managed_acp_tools/tests/support.rs b/apps/staged/src-tauri/src/managed_acp_tools/tests/support.rs index a0c22cdff..56c3feee3 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/tests/support.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/tests/support.rs @@ -5,6 +5,7 @@ use std::path::{Path, PathBuf}; use std::time::Duration; +use super::cache::npm_cache_dir; use super::install::{node_binary, npm_entrypoint, package_dir, shim_contents, write_shim}; use super::*; @@ -178,3 +179,53 @@ pub(super) fn write_node_bump_leftovers(dir: &Path) -> (PathBuf, PathBuf, PathBu std::fs::create_dir_all(&superseded_dir).unwrap(); (packages_root, pinned_dir, superseded_dir) } + +/// `/npm-prefix/cache/_cacache` — npm's content store, the part of +/// the private cache the size cap applies to. +pub(super) fn cacache_dir(packages_root: &Path) -> PathBuf { + npm_cache_dir(packages_root).join("_cacache") +} + +/// Write a `len`-byte file at `relative` inside the fixture `_cacache`, +/// standing in for a cached tarball or package document. +pub(super) fn write_cacache_file(packages_root: &Path, relative: &str, len: usize) -> PathBuf { + let path = cacache_dir(packages_root).join(relative); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, vec![b'x'; len]).unwrap(); + path +} + +/// A second open file description on `/.lock` — the same shape +/// another Staged process's flock takes — for contending with the packages +/// lock from inside one test process. +pub(super) fn open_packages_lock_contender(packages_root: &Path) -> std::fs::File { + std::fs::create_dir_all(packages_root).unwrap(); + std::fs::OpenOptions::new() + .create(true) + .write(true) + .truncate(false) + .open(packages_root.join(managed_node::PACKAGES_LOCK_FILENAME)) + .unwrap() +} + +/// One non-blocking attempt at the exclusive packages flock on `contender`. +pub(super) fn try_packages_flock(contender: &std::fs::File) -> bool { + use std::os::unix::io::AsRawFd; + unsafe { libc::flock(contender.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) == 0 } +} + +/// Take the packages flock on `contender`, retrying briefly instead of +/// asserting a single non-blocking attempt: a child process forked by a +/// concurrently-running test can hold a duplicate of a just-closed lock fd +/// until its exec closes it (CLOEXEC), keeping the flock alive for a moment +/// after its owner dropped it. +pub(super) async fn acquire_packages_flock(contender: &std::fs::File) { + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while !try_packages_flock(contender) { + assert!( + std::time::Instant::now() < deadline, + "the packages lock was never released to the contender" + ); + tokio::time::sleep(Duration::from_millis(10)).await; + } +} diff --git a/apps/staged/src-tauri/src/managed_acp_tools/tests/validation.rs b/apps/staged/src-tauri/src/managed_acp_tools/tests/validation.rs index 2a37d3d0e..c9f47bd87 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/tests/validation.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/tests/validation.rs @@ -2,6 +2,7 @@ use std::sync::Mutex; use std::time::Instant; use super::*; +use crate::managed_acp_tools::cache::NPM_CACHE_MAX_BYTES; use crate::managed_acp_tools::install::{install_npm_tool, shim_contents, write_shim}; use crate::managed_acp_tools::reconcile::finish_reconcile_at; use crate::managed_acp_tools::test_support::*; @@ -199,7 +200,13 @@ async fn missing_native_binary_preserves_live_install_and_partial_reconcile_runt .await .unwrap(); } - finish_reconcile_at(&root, MANAGED_TOOLS, vec![format!("{}: {error}", tool.id)]).await; + finish_reconcile_at( + &root, + MANAGED_TOOLS, + vec![format!("{}: {error}", tool.id)], + NPM_CACHE_MAX_BYTES, + ) + .await; let state = read_state(&root); let record = state.last_reconcile.unwrap(); diff --git a/apps/staged/src-tauri/src/managed_node.rs b/apps/staged/src-tauri/src/managed_node.rs index ec712a534..bb9222549 100644 --- a/apps/staged/src-tauri/src/managed_node.rs +++ b/apps/staged/src-tauri/src/managed_node.rs @@ -34,7 +34,7 @@ const UPSTREAM_NODE_DIST_BASE_URL: &str = "https://nodejs.org/dist"; /// `https://nodejs.org/dist` (`//`). pub const NODE_DIST_URL_ENV: &str = "STAGED_NODE_DIST_URL"; -const PACKAGES_LOCK_FILENAME: &str = ".lock"; +pub(crate) const PACKAGES_LOCK_FILENAME: &str = ".lock"; /// Hard cap on the compressed tarball; the largest pinned artifact today is /// ~55 MiB, so anything near this is a wrong or corrupted download. From 4b23d16bb8f1fa4b4fd6abdfc519887b68b85bbb Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 13 Nov 2026 15:52:05 +1100 Subject: [PATCH 2/2] fix(staged): cap the npm cache from Doctor fixes and keep the prune inside the packages tree Follow-up to fca2ddb4, addressing the review of that commit. Restricted builds never capped the cache. The reconcile loop returns before its first pass when the managed set is empty, so with the `no-managed-acp-tools` feature, with `STAGED_ACP_TOOLS_DIR` set, or on an unsupported target the epilogue's prune never ran, while Doctor's `npm install -g` fixes and updates kept filling the same cache. - `NpmWriteLock` now remembers its packages root and gains `prune_npm_cache`, which applies the 1 GiB cap while the lock is held. - Doctor's npm-backed fixes and updates, and the deprecated-bridge fix, end in a new `finish_npm_fix`. It prunes under the still-held lock, after a failed command as well as a successful one, and returns the fix's own result untouched. The prune is best-effort and returns nothing, so it cannot fail a fix. - The spawn-blocking wrapper around the prune moves into `cache.rs` as `prune_npm_cache_on_blocking_pool`, shared with the reconcile epilogue. The prune could leave the packages tree. Only the final `_cacache` component was checked for a symlink, so a `cache` or `npm-prefix` linked elsewhere sent the leftover sweep and the wipe to the link target, for example a `~/.npm` that the user's own npm writes without any Staged lock. `prune_npm_cache_at` now stops, with a warning, unless both are real directories. A missing component stays a silent no-op, and a symlinked packages root is still capped because the whole tree and its lock file sit behind it. The wait for the npm write lock was silent. `prepare_npm_fix` now logs at info level when it starts waiting and when it has the lock, with the check id and the elapsed time. The first line is written before the managed Node runtime ensure, because that call can queue behind the same flock. Locking behaviour is unchanged and no timeout is added. The reconciler header, the `managed_acp_tools` layout comment and the cache module docs now say that "silent when there is nothing to manage" includes the cache cap, and that Doctor's fixes cover it there. Tests, all in temporary directories: - a symlinked `cache`, a symlinked `npm-prefix` and a plain-file `cache` are left alone, including the linked cache's trash leftover - a symlinked packages root is still capped - a `_cacache.trash-*` symlink is unlinked and its target survives - the write lock prunes an oversized cache and still holds both locks afterwards, and leaves a cache within the cap alone - `finish_npm_fix` prunes at the real cap for a passing and a failing fix and returns each result unchanged, leaves a cache exactly at the cap, and passes a result through when no lock is held. A sparse file stands in for a cache over 1 GiB. `lock_npm_writes_at` becomes `pub(crate)` and is re-exported under `cfg(test)` so the Doctor tests can lock a temporary root. The one new field is the in-memory `packages_root` on the lock guard; nothing is added to `state.json` or any stored model. Not covered by a test: that `run_doctor_fix` and `run_doctor_update` call `finish_npm_fix`. Those commands resolve the real packages root and spawn real commands, so that wiring is checked by reading only. Signed-off-by: Matt Toohey Co-Authored-By: Claude Fable 5.2 --- .../src-tauri/src/acp_tools_reconciler.rs | 6 +- apps/staged/src-tauri/src/doctor/mod.rs | 71 +++++++++--- .../src-tauri/src/doctor/tests/commands.rs | 65 +++++++++++ .../src-tauri/src/managed_acp_tools/cache.rs | 76 +++++++++++-- .../src-tauri/src/managed_acp_tools/lock.rs | 40 ++++++- .../src-tauri/src/managed_acp_tools/mod.rs | 13 ++- .../src/managed_acp_tools/reconcile.rs | 15 +-- .../src/managed_acp_tools/tests/cache.rs | 102 ++++++++++++++++++ .../src/managed_acp_tools/tests/lock.rs | 53 +++++++++ 9 files changed, 402 insertions(+), 39 deletions(-) diff --git a/apps/staged/src-tauri/src/acp_tools_reconciler.rs b/apps/staged/src-tauri/src/acp_tools_reconciler.rs index d565f1d6d..84c7a41c3 100644 --- a/apps/staged/src-tauri/src/acp_tools_reconciler.rs +++ b/apps/staged/src-tauri/src/acp_tools_reconciler.rs @@ -24,7 +24,11 @@ //! //! Silent when there is nothing to manage: the `STAGED_ACP_TOOLS_DIR` dev //! override is active, the `no-managed-acp-tools` build feature is set, or -//! the target is unsupported. +//! the target is unsupported. Silent there includes the cache cap — the loop +//! returns before its first pass, so the epilogue never runs. Such a build +//! still fills the same cache through Doctor's `npm install -g` fixes and +//! updates, so those cap it themselves as each one finishes, under the same +//! locks (`NpmWriteLock::prune_npm_cache`). //! //! Completion is broadcast to the renderer as [`ACP_TOOLS_RECONCILED_EVENT`]: //! on a fresh profile the frontend caches its doctor report and provider diff --git a/apps/staged/src-tauri/src/doctor/mod.rs b/apps/staged/src-tauri/src/doctor/mod.rs index f7acdf0fc..9b41e28e4 100644 --- a/apps/staged/src-tauri/src/doctor/mod.rs +++ b/apps/staged/src-tauri/src/doctor/mod.rs @@ -355,18 +355,21 @@ pub async fn run_doctor_fix( // check definition, so the npm-backed gate below can't see it — ensure the // managed runtime here, since the fix shells out to npm. if check_id == DEPRECATED_CLAUDE_ACP_CHECK_ID { - let _npm_write_lock = prepare_npm_fix().await?; - return run_deprecated_claude_acp_fix(fix_type, env_vars).await; + let npm_write_lock = Some(prepare_npm_fix(&check_id).await?); + let result = run_deprecated_claude_acp_fix(fix_type, env_vars).await; + return finish_npm_fix(npm_write_lock, result).await; } let npm_backed = doctor::agents::lookup_fix_command(&check_id, &fix_type) .as_deref() .is_some_and(crate::managed_acp_tools::is_npm_backed_command); - let _npm_write_lock = if npm_backed { - Some(prepare_npm_fix().await?) + let npm_write_lock = if npm_backed { + Some(prepare_npm_fix(&check_id).await?) } else { None }; - execute_fix_with_gateway_fallback(check_id, fix_type, None, npm_backed, env_vars).await + let result = + execute_fix_with_gateway_fallback(check_id, fix_type, None, npm_backed, env_vars).await; + finish_npm_fix(npm_write_lock, result).await } /// The install id of the ACP bridge a doctor check fronts, for the two checks @@ -445,18 +448,58 @@ async fn ensure_managed_node_runtime_for_fix() -> Result<(), String> { /// writes the private prefix and its download cache, so without the lock it /// could overlap a bridge install or have the reconciler's cache prune land /// mid-install — in this or another Staged instance. The caller holds the -/// returned guard until the command, gateway retry included, has finished. +/// returned guard until the command, gateway retry included, has finished, +/// and then hands it to [`finish_npm_fix`]. /// /// The order is load-bearing: the runtime ensure takes the packages flock /// itself, so the lock is acquired only after it returns — nesting the two /// would deadlock. Fixes that go through the managed installer /// ([`install_managed_tool_logged`]) must not call this at all, since the /// installer takes the same locks. -async fn prepare_npm_fix() -> Result { +/// +/// Both steps can wait, unbounded and uncancellable, behind whatever holds +/// the packages locks — on a fresh profile, the launch reconcile downloading +/// Node and installing two bridges — while the UI shows only the fix button's +/// spinner. The two info lines bracket that wait so a stuck spinner can be +/// diagnosed from the log: a "waiting" line with no "acquired" line after it +/// means the fix is still queued behind another npm writer. +async fn prepare_npm_fix(check_id: &str) -> Result { + let started = std::time::Instant::now(); + log::info!( + "[doctor fix {check_id}] waiting for the managed Node.js runtime and the npm write lock \ + (queues behind any bridge install or other npm fix, in this or another Staged instance)" + ); ensure_managed_node_runtime_for_fix().await?; - crate::managed_acp_tools::lock_npm_writes() + let npm_write_lock = crate::managed_acp_tools::lock_npm_writes() .await - .map_err(|error| error.to_string()) + .map_err(|error| error.to_string())?; + log::info!( + "[doctor fix {check_id}] acquired the npm write lock after {} ms", + started.elapsed().as_millis() + ); + Ok(npm_write_lock) +} + +/// Epilogue of an npm-backed fix or update: cap the private npm cache while +/// the write lock from [`prepare_npm_fix`] still excludes every other Staged +/// npm writer, then release the lock and hand back the fix's own `result` +/// untouched. +/// +/// The fix's `npm install -g` has just written that cache, and on a build +/// that manages no bridges this is the only place it is ever capped — the +/// reconciler runs no pass there (see `acp_tools_reconciler`). It runs after +/// a failed fix as well as a successful one, since a failing install still +/// downloads. The prune is best-effort and returns nothing, so it cannot +/// turn a fix that worked into one that failed, or mask the error of one +/// that did not. `None` is a fix that never touched npm: nothing to cap. +async fn finish_npm_fix( + npm_write_lock: Option, + result: Result<(), String>, +) -> Result<(), String> { + if let Some(npm_write_lock) = npm_write_lock { + npm_write_lock.prune_npm_cache().await; + } + result } /// Run a source-aware update for a single readout (main CLI or ACP bridge). @@ -509,8 +552,8 @@ pub async fn run_doctor_update( // managed runtime must exist before the command does — and the command // runs under the npm write lock. let npm_backed = crate::managed_acp_tools::is_npm_backed_command(&expected); - let _npm_write_lock = if npm_backed { - Some(prepare_npm_fix().await?) + let npm_write_lock = if npm_backed { + Some(prepare_npm_fix(&check_id).await?) } else { None }; @@ -518,8 +561,10 @@ pub async fn run_doctor_update( // They are equal past the guard above, but executing `expected` makes the // command that runs provably the one the backend derived — no dependence on // the equality check surviving future edits. - execute_fix_with_gateway_fallback(check_id, fix_type, Some(expected), npm_backed, env_vars) - .await + let result = + execute_fix_with_gateway_fallback(check_id, fix_type, Some(expected), npm_backed, env_vars) + .await; + finish_npm_fix(npm_write_lock, result).await } /// Re-run freshness and return the authoritative update command for the given diff --git a/apps/staged/src-tauri/src/doctor/tests/commands.rs b/apps/staged/src-tauri/src/doctor/tests/commands.rs index 83f53bd50..ee15fe384 100644 --- a/apps/staged/src-tauri/src/doctor/tests/commands.rs +++ b/apps/staged/src-tauri/src/doctor/tests/commands.rs @@ -158,3 +158,68 @@ fn doctor_options_route_npm_through_the_registry_override() { ); assert_eq!(execute_fix_options(None, Vec::new()).npm_registry, expected); } + +/// `/npm-prefix/cache/_cacache` in a temporary packages root. +fn fixture_cacache(packages_root: &std::path::Path) -> PathBuf { + packages_root + .join("npm-prefix") + .join("cache") + .join("_cacache") +} + +/// Put one file of `len` bytes in the fixture `_cacache`. The file is sparse +/// — `set_len` allocates nothing — so a cache "over" the real 1 GiB cap costs +/// no disk and no time. +fn write_sparse_cacache_file(packages_root: &std::path::Path, len: u64) -> PathBuf { + let path = fixture_cacache(packages_root) + .join("content-v2") + .join("tarball"); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::File::create(&path).unwrap().set_len(len).unwrap(); + path +} + +const ONE_GIB: u64 = 1024 * 1024 * 1024; + +/// The epilogue of an npm-backed fix caps the cache at the real cap — the +/// only place a build that manages no bridges ever does — and hands the +/// fix's own outcome back untouched, whether the fix worked or not. +#[tokio::test] +async fn finishing_an_npm_fix_prunes_an_oversized_cache_and_keeps_the_fix_result() { + for fix_result in [Ok(()), Err("npm install failed".to_string())] { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + write_sparse_cacache_file(&packages_root, ONE_GIB + 1); + let lock = crate::managed_acp_tools::lock_npm_writes_at(&packages_root) + .await + .unwrap(); + + let result = finish_npm_fix(Some(lock), fix_result.clone()).await; + + assert_eq!(result, fix_result); + assert!(!fixture_cacache(&packages_root).exists(), "{fix_result:?}"); + } +} + +#[tokio::test] +async fn finishing_an_npm_fix_leaves_a_cache_at_the_cap() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + let tarball = write_sparse_cacache_file(&packages_root, ONE_GIB); + let lock = crate::managed_acp_tools::lock_npm_writes_at(&packages_root) + .await + .unwrap(); + + assert_eq!(finish_npm_fix(Some(lock), Ok(())).await, Ok(())); + + assert_eq!(tarball.metadata().unwrap().len(), ONE_GIB); +} + +/// A fix that never touched npm holds no lock: its result passes straight +/// through and there is nothing to cap. +#[tokio::test] +async fn finishing_a_fix_without_the_npm_lock_only_returns_its_result() { + assert_eq!(finish_npm_fix(None, Ok(())).await, Ok(())); + let failed = Err("not an npm fix".to_string()); + assert_eq!(finish_npm_fix(None, failed.clone()).await, failed); +} diff --git a/apps/staged/src-tauri/src/managed_acp_tools/cache.rs b/apps/staged/src-tauri/src/managed_acp_tools/cache.rs index e8e488276..50595fa43 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/cache.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/cache.rs @@ -7,8 +7,11 @@ //! and each revalidated package document leaves its previous body behind //! unreferenced. The cache has no correctness role — a bridge at runtime //! never reads it, and an install against an empty cache only re-downloads — -//! so the reconcile epilogue wipes it whole once it outgrows -//! [`NPM_CACHE_MAX_BYTES`] (see [`prune_npm_cache_at`]). +//! so it is wiped whole once it outgrows [`NPM_CACHE_MAX_BYTES`] (see +//! [`prune_npm_cache_at`]). Two callers cap it, each under the install locks: +//! the reconcile epilogue on every pass, and Doctor at the end of each +//! npm-backed fix or update — the only writer left on a build that manages +//! no bridges and so never reconciles. use std::path::{Path, PathBuf}; @@ -45,21 +48,27 @@ pub(super) fn npm_cache_dir(packages_root: &Path) -> PathBuf { /// Wipe the private npm cache's `_cacache` when it holds more than /// `max_bytes`. Best-effort: returns nothing and logs every failure at warn /// level, since a cache that could not be pruned costs disk, never -/// correctness, and is retried on the next reconcile pass. +/// correctness, and is retried on the next reconcile pass or Doctor npm fix. /// -/// Three moves: +/// Four moves: /// -/// 1. Remove any `_cacache.trash-*` sibling an interrupted prune left behind. -/// 2. Sum the file sizes under `_cacache` without following symlinks, and +/// 1. Stop unless `npm-prefix` and `npm-prefix/cache` are real directories +/// (see [`cache_dir_is_inside_packages_tree`]). +/// 2. Remove any `_cacache.trash-*` sibling an interrupted prune left behind. +/// 3. Sum the file sizes under `_cacache` without following symlinks, and /// stop when the total is within the cap. -/// 3. Rename `_cacache` to `_cacache.trash-` and delete that. The rename +/// 4. Rename `_cacache` to `_cacache.trash-` and delete that. The rename /// is atomic, so npm sees either the whole cache or none of it — never a /// half-deleted one whose index points at missing content. /// /// The caller holds the tool-install mutex and the packages flock, so no /// Staged-spawned npm install is writing the cache meanwhile. Blocking -/// filesystem work: call it from `spawn_blocking`. +/// filesystem work: call it from `spawn_blocking` (or through +/// [`prune_npm_cache_on_blocking_pool`]). pub(super) fn prune_npm_cache_at(packages_root: &Path, max_bytes: u64) { + if !cache_dir_is_inside_packages_tree(packages_root) { + return; + } let cache_dir = npm_cache_dir(packages_root); remove_leftover_trash(&cache_dir); @@ -106,6 +115,57 @@ pub(super) fn prune_npm_cache_at(packages_root: &Path, max_bytes: u64) { } } +/// [`prune_npm_cache_at`] on the blocking pool, for async callers that hold +/// the install locks. Also best-effort: a panic in the cleanup surfaces here +/// as a join error and is logged instead of unwinding through the caller. +pub(super) async fn prune_npm_cache_on_blocking_pool(packages_root: &Path, max_bytes: u64) { + let packages_root = packages_root.to_path_buf(); + if let Err(error) = + tokio::task::spawn_blocking(move || prune_npm_cache_at(&packages_root, max_bytes)).await + { + log::warn!("npm cache prune task failed: {error}"); + } +} + +/// Whether `/npm-prefix` and `/npm-prefix/cache` are both +/// real directories, so everything the prune sizes, renames and deletes is +/// inside the packages tree the caller's locks cover. +/// +/// The OS resolves a symlink in either component, which would send the +/// leftover sweep and the wipe to the link's target — a `cache` linked to +/// `~/.npm`, say, where the user's own npm processes write without holding +/// any Staged lock. A cache redirected like that is somebody else's to size +/// and evict, so the prune leaves it alone and says so. Missing components +/// are the ordinary nothing-to-prune case and stay silent. The packages root +/// itself may be a symlink: the whole tree, lock file included, moves with +/// it. +fn cache_dir_is_inside_packages_tree(packages_root: &Path) -> bool { + let npm_prefix = packages_root.join("npm-prefix"); + let cache_dir = npm_cache_dir_in_prefix(&npm_prefix); + for dir in [npm_prefix, cache_dir] { + match std::fs::symlink_metadata(&dir) { + Ok(meta) if meta.is_dir() => {} + Ok(_) => { + log::warn!( + "skipping npm cache prune: {} is a symlink or a file, not a directory, so \ + the cache it leads to is not capped", + dir.display() + ); + return false; + } + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return false, + Err(error) => { + log::warn!( + "skipping npm cache prune: cannot inspect {}: {error}", + dir.display() + ); + return false; + } + } + } + true +} + /// Remove every `_cacache.trash-*` entry in the cache dir — what a prune that /// was interrupted (crash, quit, power loss) between its rename and the end /// of its delete left behind. diff --git a/apps/staged/src-tauri/src/managed_acp_tools/lock.rs b/apps/staged/src-tauri/src/managed_acp_tools/lock.rs index c962b5cc0..c58176069 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/lock.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/lock.rs @@ -1,9 +1,10 @@ //! The combined lock for npm writers outside the managed installer. -use std::path::Path; +use std::path::{Path, PathBuf}; use crate::managed_node::{self, ManagedNodeError, PackagesDirLock}; +use super::cache::{prune_npm_cache_on_blocking_pool, NPM_CACHE_MAX_BYTES}; use super::install::tool_install_lock; #[cfg(test)] @@ -17,13 +18,46 @@ mod tests; /// such a fix can neither overlap a bridge install nor have the npm cache /// pruned out from under it (see `cache::prune_npm_cache_at`), in this or any /// other Staged process sharing the tree. +/// +/// Because the holder is by construction the only Staged npm writer, it is +/// also in a position to cap the cache it just wrote: see +/// [`NpmWriteLock::prune_npm_cache`]. pub(crate) struct NpmWriteLock { + /// The packages tree the two locks below cover — the one whose npm cache + /// [`NpmWriteLock::prune_npm_cache`] caps. + packages_root: PathBuf, // Field order is drop order: the flock is released before the mutex, the // reverse of acquisition. _packages_lock: PackagesDirLock, _guard: tokio::sync::MutexGuard<'static, ()>, } +impl NpmWriteLock { + /// Wipe the private npm cache if it has outgrown its size cap, while this + /// lock still excludes every other Staged npm writer. Call it after the + /// fix's command has finished, whatever its outcome, and before dropping + /// the lock. + /// + /// This is what caps the cache on a build that manages no bridges + /// (`no-managed-acp-tools`, the `STAGED_ACP_TOOLS_DIR` override, an + /// unsupported target): the reconciler there never runs a pass, so its + /// epilogue never prunes, yet Doctor's `npm install -g` fixes and updates + /// still fill the same cache. On a managed build it is a second, cheap + /// chance at the same cap. + /// + /// Best-effort, like the epilogue's prune: it returns nothing and logs + /// its failures, so it can never fail the fix it follows. + pub(crate) async fn prune_npm_cache(&self) { + self.prune_npm_cache_over(NPM_CACHE_MAX_BYTES).await; + } + + /// [`NpmWriteLock::prune_npm_cache`] with the cap as a parameter, so + /// tests can exceed it with a few bytes. + pub(super) async fn prune_npm_cache_over(&self, max_bytes: u64) { + prune_npm_cache_on_blocking_pool(&self.packages_root, max_bytes).await; + } +} + /// Take the [`NpmWriteLock`] for the real `~/.staged/packages`. Waits for /// whatever holds either lock — worst case a whole bridge install in another /// Staged instance. Never call while a [`PackagesDirLock`] is held, and only @@ -36,12 +70,14 @@ pub(crate) async fn lock_npm_writes() -> Result lock_npm_writes_at(&packages_root).await } -pub(super) async fn lock_npm_writes_at( +/// [`lock_npm_writes`] for a given packages root. +pub(crate) async fn lock_npm_writes_at( packages_root: &Path, ) -> Result { let guard = tool_install_lock().lock().await; let packages_lock = managed_node::lock_packages_dir(packages_root).await?; Ok(NpmWriteLock { + packages_root: packages_root.to_path_buf(), _packages_lock: packages_lock, _guard: guard, }) diff --git a/apps/staged/src-tauri/src/managed_acp_tools/mod.rs b/apps/staged/src-tauri/src/managed_acp_tools/mod.rs index c0b1d71f7..6bb3731d0 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/mod.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/mod.rs @@ -25,10 +25,13 @@ //! //! - `npm-prefix/` — the private npm global prefix. //! - `npm-prefix/cache/` — the npm download cache every Staged-spawned npm -//! shares. npm never evicts from it, so the reconcile epilogue wipes its -//! `_cacache` whole once it outgrows 1 GiB (see `cache`); the next install -//! just re-downloads. Doctor's npm fixes hold the same locks -//! ([`lock_npm_writes`]) so a wipe never lands mid-install. +//! shares. npm never evicts from it, so its `_cacache` is wiped whole once +//! it outgrows 1 GiB (see `cache`); the next install just re-downloads. +//! The reconcile epilogue applies the cap on every pass, and Doctor's npm +//! fixes apply it as they finish — which is what covers a build that +//! manages no bridges and so never reconciles. Those fixes hold the same +//! locks as the installer ([`lock_npm_writes`]), so a wipe never lands +//! mid-install. //! - `node///` — managed Node runtimes (`managed_node`). //! - `tools//` — per-bridge npm `--prefix` trees for the managed ACP //! bridges (claude-acp, codex-acp). @@ -64,6 +67,8 @@ pub(crate) mod test_support; mod tests; pub use install::{install_managed_tool, InstallLineFn, ManagedToolError}; +#[cfg(test)] +pub(crate) use lock::lock_npm_writes_at; pub(crate) use lock::{lock_npm_writes, NpmWriteLock}; pub(crate) use probe::configured_npm_registry; pub use probe::{installed_tool_version, latest_published_version}; diff --git a/apps/staged/src-tauri/src/managed_acp_tools/reconcile.rs b/apps/staged/src-tauri/src/managed_acp_tools/reconcile.rs index 87fc9bedd..e881a9fe2 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/reconcile.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/reconcile.rs @@ -5,7 +5,7 @@ use std::path::Path; use crate::managed_node; -use super::cache::{prune_npm_cache_at, NPM_CACHE_MAX_BYTES}; +use super::cache::{prune_npm_cache_on_blocking_pool, NPM_CACHE_MAX_BYTES}; use super::install::tool_install_lock; use super::{read_state, shim_bin_dir, tools_root, write_state, ManagedTool, ReconcileRecord}; @@ -57,16 +57,9 @@ pub(super) async fn finish_reconcile_at( // that fails every day would otherwise switch cleanup off while the // cache keeps growing. It runs after the state write, so recording // the outcome never waits on a multi-gigabyte delete, and on the - // blocking pool, so a panic in cleanup surfaces here as a join error - // instead of unwinding through the pass. - let cache_root = packages_root.to_path_buf(); - if let Err(error) = tokio::task::spawn_blocking(move || { - prune_npm_cache_at(&cache_root, npm_cache_max_bytes) - }) - .await - { - log::warn!("npm cache prune task failed: {error}"); - } + // blocking pool, so a panic in cleanup is logged instead of unwinding + // through the pass. + prune_npm_cache_on_blocking_pool(packages_root, npm_cache_max_bytes).await; } // Success-gated Node prune: `errors` empty means every managed bridge // reinstalled this run, so every shim now embeds the pinned runtime's diff --git a/apps/staged/src-tauri/src/managed_acp_tools/tests/cache.rs b/apps/staged/src-tauri/src/managed_acp_tools/tests/cache.rs index 9b2fd58a0..4c04d4bd6 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/tests/cache.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/tests/cache.rs @@ -112,6 +112,14 @@ fn leftover_trash_is_removed_even_when_the_cache_is_within_the_cap() { std::fs::write(trash_dir.join("content-v2").join("half-deleted"), "x").unwrap(); let trash_file = cache_dir.join("_cacache.trash-7"); std::fs::write(&trash_file, "x").unwrap(); + // A symlink named like a trash dir, pointing at a directory outside the + // cache: the link is unlinked, never traversed. + let outside_dir = dir.path().join("outside"); + std::fs::create_dir_all(&outside_dir).unwrap(); + let outside_file = outside_dir.join("keep"); + std::fs::write(&outside_file, "keep").unwrap(); + let trash_link = cache_dir.join("_cacache.trash-99"); + std::os::unix::fs::symlink(&outside_dir, &trash_link).unwrap(); // A neighbour that only resembles the trash prefix stays. let lookalike = cache_dir.join("_cacache.trashed"); std::fs::create_dir_all(&lookalike).unwrap(); @@ -120,6 +128,9 @@ fn leftover_trash_is_removed_even_when_the_cache_is_within_the_cap() { assert!(!trash_dir.exists()); assert!(!trash_file.exists()); + assert!(std::fs::symlink_metadata(&trash_link).is_err()); + assert!(outside_dir.is_dir()); + assert_eq!(std::fs::read_to_string(&outside_file).unwrap(), "keep"); assert!(lookalike.is_dir()); assert_eq!(std::fs::read(&tarball).unwrap().len(), 10); } @@ -200,3 +211,94 @@ fn a_cacache_that_is_itself_a_symlink_is_left_alone() { assert!(cacache.is_symlink()); assert_eq!(std::fs::read(&elsewhere_file).unwrap().len(), 50); } + +/// A cache laid out outside the packages tree, reached only through a symlink +/// standing in for one of the cache dir's ancestors: an oversized `_cacache` +/// and an interrupted prune's leftover. Returns `(tarball, trash_file)`. +fn write_linked_cache(real_cache_dir: &Path) -> (PathBuf, PathBuf) { + let tarball = real_cache_dir + .join("_cacache") + .join("content-v2") + .join("tarball"); + std::fs::create_dir_all(tarball.parent().unwrap()).unwrap(); + std::fs::write(&tarball, vec![b'x'; 50]).unwrap(); + let trash_file = real_cache_dir + .join("_cacache.trash-4242") + .join("half-deleted"); + std::fs::create_dir_all(trash_file.parent().unwrap()).unwrap(); + std::fs::write(&trash_file, "x").unwrap(); + (tarball, trash_file) +} + +#[test] +fn a_cache_dir_that_is_a_symlink_is_left_alone() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + // `npm-prefix/cache` -> a cache somebody else owns (think `~/.npm`). + let elsewhere = dir.path().join("elsewhere"); + let (tarball, trash_file) = write_linked_cache(&elsewhere); + let cache_dir = npm_cache_dir(&packages_root); + std::fs::create_dir_all(cache_dir.parent().unwrap()).unwrap(); + std::os::unix::fs::symlink(&elsewhere, &cache_dir).unwrap(); + // The fixture is reachable through the link, so only the guard keeps the + // prune out of it. + assert!(cacache_dir(&packages_root).is_dir()); + + prune_npm_cache_at(&packages_root, 0); + + assert!(cache_dir.is_symlink()); + // Neither the wipe nor the leftover sweep followed the link. + assert_eq!(std::fs::read(&tarball).unwrap().len(), 50); + assert!(trash_file.is_file()); +} + +#[test] +fn an_npm_prefix_that_is_a_symlink_is_left_alone() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + // `npm-prefix` -> a prefix outside the packages tree. + let elsewhere = dir.path().join("elsewhere"); + let (tarball, trash_file) = write_linked_cache(&elsewhere.join("cache")); + std::fs::create_dir_all(&packages_root).unwrap(); + let npm_prefix = packages_root.join("npm-prefix"); + std::os::unix::fs::symlink(&elsewhere, &npm_prefix).unwrap(); + assert!(cacache_dir(&packages_root).is_dir()); + + prune_npm_cache_at(&packages_root, 0); + + assert!(npm_prefix.is_symlink()); + assert_eq!(std::fs::read(&tarball).unwrap().len(), 50); + assert!(trash_file.is_file()); +} + +#[test] +fn a_cache_dir_that_is_a_plain_file_is_left_alone() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path(); + let cache_dir = npm_cache_dir(packages_root); + std::fs::create_dir_all(cache_dir.parent().unwrap()).unwrap(); + std::fs::write(&cache_dir, "not a directory").unwrap(); + + prune_npm_cache_at(packages_root, 0); + + assert_eq!( + std::fs::read_to_string(&cache_dir).unwrap(), + "not a directory" + ); +} + +/// A packages root that is itself a symlink is fine: the whole tree — lock +/// file included — lives behind it, so the cap still applies. +#[test] +fn a_packages_root_that_is_a_symlink_is_still_capped() { + let dir = tempfile::tempdir().unwrap(); + let real_root = dir.path().join("real-packages"); + write_cacache_file(&real_root, "content-v2/tarball", 50); + let packages_root = dir.path().join("packages"); + std::os::unix::fs::symlink(&real_root, &packages_root).unwrap(); + + prune_npm_cache_at(&packages_root, 10); + + assert!(!cacache_dir(&real_root).exists()); + assert!(npm_cache_dir(&real_root).is_dir()); +} diff --git a/apps/staged/src-tauri/src/managed_acp_tools/tests/lock.rs b/apps/staged/src-tauri/src/managed_acp_tools/tests/lock.rs index 46e1c915f..a85b57a1c 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/tests/lock.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/tests/lock.rs @@ -17,3 +17,56 @@ async fn npm_write_lock_holds_the_packages_flock_until_dropped() { drop(lock); acquire_packages_flock(&contender).await; } + +/// The Doctor-path cap: a fix that holds the npm write lock wipes an +/// oversized cache before releasing it, and nothing else under the root. +#[tokio::test] +async fn npm_write_lock_prunes_an_oversized_cache_while_still_held() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + write_cacache_file(&packages_root, "content-v2/sha512/aa/bb/tarball", 60); + write_cacache_file(&packages_root, "index-v5/cc/dd/entry", 41); + // What the fix's `npm install -g` just put in the private prefix. + let global_package = packages_root + .join("npm-prefix") + .join("lib") + .join("node_modules") + .join("@github") + .join("copilot") + .join("package.json"); + std::fs::create_dir_all(global_package.parent().unwrap()).unwrap(); + std::fs::write(&global_package, "keep").unwrap(); + + let lock = lock_npm_writes_at(&packages_root).await.unwrap(); + lock.prune_npm_cache_over(100).await; + + assert!(!cacache_dir(&packages_root).exists()); + assert_eq!(std::fs::read_to_string(&global_package).unwrap(), "keep"); + // The prune did not give the locks up: no other Staged process can start + // an install until the fix drops them. + let contender = open_packages_lock_contender(&packages_root); + assert!(!try_packages_flock(&contender)); + assert!(tool_install_lock().try_lock().is_err()); + + drop(lock); + acquire_packages_flock(&contender).await; +} + +#[tokio::test] +async fn npm_write_lock_leaves_a_cache_within_the_cap() { + let dir = tempfile::tempdir().unwrap(); + let packages_root = dir.path().join("packages"); + let tarball = write_cacache_file(&packages_root, "content-v2/sha512/aa/bb/tarball", 100); + + let lock = lock_npm_writes_at(&packages_root).await.unwrap(); + lock.prune_npm_cache_over(100).await; + // A root with no npm prefix at all — a fix that failed before npm wrote + // anything — is a no-op too. + drop(lock); + let empty_root = dir.path().join("empty"); + let lock = lock_npm_writes_at(&empty_root).await.unwrap(); + lock.prune_npm_cache_over(0).await; + + assert_eq!(std::fs::read(&tarball).unwrap().len(), 100); + assert!(!empty_root.join("npm-prefix").exists()); +}