From 857c5e50cb12eec7fa1e403bb99a81583daa56e0 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Thu, 9 Jul 2026 22:06:46 +0200 Subject: [PATCH] fix(files,server): age-gate stale-artifact sweep to stop it deleting live concurrent exports sweep_stale_artifacts previously matched staging/displaced siblings by name alone, with no way to tell a genuine crash orphan from another live process's in-flight export. A concurrent export of the same target could delete the first export's staging directory outright, and if the timing landed between swap_into_place's two renames, the displaced original too, defeating the rollback and permanently losing the target with no recovery path. The sweep now only removes a staging/stale sibling once it is at least STALE_ARTIFACT_MIN_AGE (5 minutes) old by mtime, which reliably distinguishes a crash leftover from a live sibling's artifacts. swap_into_place's displaced backup has its mtime explicitly refreshed before the rename (a rename preserves the source's mtime rather than resetting it), so it is covered by the same age gate as staging instead of inheriting the original target's already-stale mtime. GeneratorService::save_categorized_tools also gains a keyed Arc> per output_dir, mirroring the existing per-server-id introspectors lock, so two JSON-RPC calls targeting the same output directory no longer race on the underlying staging/swap. Fixes #169 --- CHANGELOG.md | 16 ++ crates/mcp-files/src/filesystem.rs | 243 ++++++++++++++++++++++++++--- crates/mcp-server/src/service.rs | 144 ++++++++++++++++- 3 files changed, 377 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6567baa..551c4ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -123,6 +123,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 a structured `SetupResult` (Node.js version, MCP config path/found, files made executable) and routes it through `format_output`, so `--format json`/`text` produce structured output while the default `pretty` format keeps today's human-readable summary (#165). +- **`mcp-execution-files`**: `FileSystem::sweep_stale_artifacts` (added in #159) now only removes a + staging/stale sibling once it is at least 5 minutes old (`STALE_ARTIFACT_MIN_AGE`), instead of + matching purely by name. Previously, a concurrent export of the same target could delete another + in-flight export's staging directory outright, and — if the timing landed between the final + swap's two renames — the displaced original too, defeating the rollback and permanently losing + the target with no recovery path. The age gate distinguishes a genuine crash orphan (always well + past the window in which a live export completes) from a live sibling's in-flight artifacts. + `swap_into_place`'s displaced backup now also has its mtime explicitly refreshed + (`FileSystem::touch_dir`) before it is renamed aside, since a rename preserves the source's mtime + rather than resetting it — without this, a displaced backup of a long-lived target would inherit + that target's (already stale) mtime and be immediately eligible for a concurrent sibling's sweep, + leaving the age gate protecting only the staging half of the swap (#169). +- **`mcp-execution-server`**: `GeneratorService::save_categorized_tools` now serializes concurrent + exports to the same `output_dir` behind a keyed `Arc>` (mirroring the existing + per-server-id `introspectors` lock), so two JSON-RPC calls targeting the same output directory no + longer race on the underlying staging/swap (#169). - **`mcp-execution-files`**: `FileSystem::export_to_filesystem` now stages the entire export tree in a sibling temporary directory and publishes it via a single atomic directory rename, instead of writing files directly into the target directory. A process interrupted mid-`generate` (e.g. diff --git a/crates/mcp-files/src/filesystem.rs b/crates/mcp-files/src/filesystem.rs index 89110f3..64c620a 100644 --- a/crates/mcp-files/src/filesystem.rs +++ b/crates/mcp-files/src/filesystem.rs @@ -66,9 +66,38 @@ use std::fs; use std::io::Write; use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU64, Ordering}; -use std::time::{SystemTime, UNIX_EPOCH}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; use tempfile::TempDir; +/// Minimum age (by mtime) a staging/stale sibling must have before +/// [`FileSystem::sweep_stale_artifacts`] will remove it. +/// +/// A genuinely concurrent sibling export's staging and displaced +/// directories are kept younger than this: `staging` is freshly created via +/// `tempfile` for every export, and [`FileSystem::displace_existing_target`] +/// refreshes `target`'s mtime via [`FileSystem::touch_dir`] immediately +/// *before* renaming it aside to `displaced` — a rename preserves the +/// source's mtime, so `displaced` carries that fresh timestamp from the +/// instant it exists, rather than inheriting `target`'s (typically much +/// older) original mtime and being immediately sweep-eligible. All of the +/// work between +/// creating/refreshing one of these directories and either publishing or +/// rolling it back happens within a single +/// `export_to_filesystem_with_options` call, which completes in well under +/// this window even for large tool sets — assuming that call keeps adding +/// or removing immediate children of `staging` as it writes, since that is +/// what keeps its mtime fresh on the filesystems this crate supports. +/// Gating on age means the sweep can only ever reclaim directories that are +/// old enough to be leftovers from a process that was killed (e.g. +/// `SIGKILL`) before it could clean up after itself — never a live +/// sibling's in-flight artifacts. This is what closes the data-loss race +/// described in issue #169: previously the sweep matched purely on name, so +/// a concurrent export of the same target could delete another in-flight +/// export's staging directory, and — if the timing landed between +/// `swap_into_place`'s two renames — the displaced original too, defeating +/// the rollback and permanently losing the target. +const STALE_ARTIFACT_MIN_AGE: Duration = Duration::from_mins(5); + /// An in-memory virtual filesystem for MCP tool definitions. /// /// `FileSystem` provides a read-only filesystem structure that stores generated @@ -389,9 +418,20 @@ impl FileSystem { /// left exactly as it was — either untouched or, if it did not exist yet, /// still absent. See the [module-level docs](self) for details. /// - /// Concurrent exports of the *same* `base_path` are not supported and may - /// race (there is no locking); concurrent exports of different targets - /// sharing a parent directory are safe. + /// Concurrent exports of the *same* `base_path` from different processes + /// are not locked against each other and can still race on the final + /// swap — one export's result may be silently overwritten by another's + /// (a lost update). The age-gated sweep of orphaned staging/displaced + /// directories (`sweep_stale_artifacts`) guarantees neither export's + /// staging/displaced directories can be deleted out from under it before + /// its own rollback path runs, so this can no longer result in the + /// target, its staging directory, and its displaced backup all being + /// lost at once. Callers that need stronger-than-last-write-wins + /// semantics for the same target should serialize writes themselves + /// (e.g. a per-target lock) — see `mcp-execution-server`'s + /// `introspector_for` for the pattern this project already uses for a + /// similar problem. Concurrent exports of different targets sharing a + /// parent directory are safe. /// /// # Errors /// @@ -633,12 +673,7 @@ impl FileSystem { let parent = target.parent().ok_or_else(|| FilesError::InvalidPath { path: format!("Target path has no parent directory: {}", target.display()), })?; - let displaced = parent.join(Self::unique_sibling_name(target)); - - fs::rename(target, &displaced).map_err(|e| FilesError::IoError { - path: target.display().to_string(), - source: e, - })?; + let displaced = Self::displace_existing_target(target, parent)?; if let Err(e) = fs::rename(staging_path, target) { // Roll back so `target` is never left missing. @@ -653,6 +688,53 @@ impl FileSystem { Ok(()) } + /// Moves an existing `target` aside to a unique sibling path in `parent` + /// so the staged directory can be renamed into `target`'s place, + /// returning the displaced path. + /// + /// A directory rename does not reset the renamed directory's own mtime, + /// so without an explicit refresh `displaced` would inherit `target`'s + /// original mtime — almost always well past [`STALE_ARTIFACT_MIN_AGE`], + /// since a target being regenerated has typically existed since a prior + /// export. That would make it immediately eligible for a concurrent + /// sibling's [`Self::sweep_stale_artifacts`] pass, defeating the age + /// gate for exactly the directory [`Self::swap_into_place`]'s own + /// rollback depends on (issue #169). [`Self::touch_dir`] refreshes + /// `target`'s mtime *before* the rename rather than `displaced`'s + /// *after* it, since a rename preserves the source's mtime — so + /// `displaced` carries a fresh mtime from the instant it exists, with + /// no window in which it is stale even momentarily. + fn displace_existing_target(target: &Path, parent: &Path) -> Result { + let displaced = parent.join(Self::unique_sibling_name(target)); + + Self::touch_dir(target); + + fs::rename(target, &displaced).map_err(|e| FilesError::IoError { + path: target.display().to_string(), + source: e, + })?; + + Ok(displaced) + } + + /// Best-effort refresh of `dir`'s modification time. + /// + /// Deliberately avoids opening `dir` as a `File` to call + /// [`std::fs::File::set_times`], since opening a directory for write + /// access is not portable across every platform this crate supports. + /// Instead, creating and immediately removing a marker file inside + /// `dir` achieves the same effect portably: any filesystem that tracks + /// directory mtimes bumps them when an immediate child is added or + /// removed. Best-effort like [`Self::sweep_stale_artifacts`]: failure + /// (e.g. a read-only directory) is silently ignored rather than failing + /// the export. + fn touch_dir(dir: &Path) { + let marker = dir.join(format!(".mtime-touch-{}", std::process::id())); + if fs::File::create(&marker).is_ok() { + let _ = fs::remove_file(&marker); + } + } + /// Returns `target`'s file name for use as a namespacing stem in sibling /// artifact names, so that concurrent exports of *different* targets in /// the same parent directory (e.g. two `generate` runs publishing into @@ -697,7 +779,20 @@ impl FileSystem { /// Best-effort: this is a hygiene pass, not part of the export's /// correctness, so any I/O error while scanning or removing an entry is /// silently ignored rather than failing the export. + /// + /// Only removes entries at least [`STALE_ARTIFACT_MIN_AGE`] old (by + /// mtime) — see that constant's doc for why this age gate is what makes + /// the sweep safe to run against a concurrently in-flight sibling + /// export. fn sweep_stale_artifacts(parent: &Path, target: &Path) { + Self::sweep_stale_artifacts_older_than(parent, target, STALE_ARTIFACT_MIN_AGE); + } + + /// Inner implementation of [`Self::sweep_stale_artifacts`], parameterized + /// over the age threshold so tests can exercise the sweep logic without + /// backdating real file mtimes (pass [`Duration::ZERO`] to sweep + /// unconditionally). + fn sweep_stale_artifacts_older_than(parent: &Path, target: &Path, min_age: Duration) { let stem = Self::target_stem(target); let staging_prefix = Self::staging_prefix(target); let stale_prefix = format!(".{stem}.stale-"); @@ -710,8 +805,26 @@ impl FileSystem { let is_orphan = entry.file_name().to_str().is_some_and(|name| { name.starts_with(&staging_prefix) || name.starts_with(&stale_prefix) }); + if !is_orphan { + continue; + } - if is_orphan { + // Too young to be a crash orphan — could be a live sibling + // export's in-flight artifacts; leave it for a later sweep once + // it ages past `min_age`. Metadata errors (entry vanished, + // permission issue) are treated the same way — skip rather than + // guess, since this is a best-effort hygiene pass, not part of + // export correctness. + let old_enough = entry + .metadata() + .and_then(|m| m.modified()) + .is_ok_and(|modified| { + SystemTime::now() + .duration_since(modified) + .is_ok_and(|age| age >= min_age) + }); + + if old_enough { let _ = fs::remove_dir_all(entry.path()); } } @@ -1185,7 +1298,9 @@ mod tests { fs::create_dir_all(&unrelated).unwrap(); fs::create_dir_all(&other_target_staging).unwrap(); - FileSystem::sweep_stale_artifacts(temp.path(), &target); + // Bypass the production age gate (`Duration::ZERO`) to exercise the + // name-matching logic in isolation, without backdating real mtimes. + FileSystem::sweep_stale_artifacts_older_than(temp.path(), &target, Duration::ZERO); assert!(!orphan_staging.exists()); assert!(!orphan_stale.exists()); @@ -1194,18 +1309,37 @@ mod tests { } #[test] - fn test_export_sweeps_orphaned_artifacts_from_prior_crash() { + fn test_export_does_not_sweep_recent_sibling_artifacts() { let temp = TempDir::new().unwrap(); let target = temp.path().join("out"); fs::create_dir_all(&target).unwrap(); - // Simulate a `SIGKILL`'d prior run: a staging dir that was never - // cleaned up, and a displaced directory from a swap that never - // finished removing it. - let orphan_staging = temp.path().join(".out.staging-leftover"); - let orphan_stale = temp.path().join(".out.stale-1-2-3"); - fs::create_dir_all(orphan_staging.join("nested")).unwrap(); - fs::create_dir_all(&orphan_stale).unwrap(); + // Simulate a concurrent sibling export's in-flight staging + // directory, built the same way `export_to_filesystem_with_options` + // builds it (fresh via `tempfile`). + let sibling_staging = tempfile::Builder::new() + .prefix(&FileSystem::staging_prefix(&target)) + .tempdir_in(temp.path()) + .unwrap(); + let sibling_staging_path = sibling_staging.path().to_path_buf(); + + // Simulate a concurrent sibling export's displaced backup, built the + // same way `swap_into_place` builds it: an existing (decoy) target + // renamed aside via `displace_existing_target`. Regression test for + // issue #169: previously `sweep_stale_artifacts` matched purely by + // name, so a real export through this same public entry point would + // delete this sibling's staging and displaced directories out from + // under it. `decoy_target` is fresh at test time, so this does not + // itself exercise the S1 mtime-inheritance defect (a fresh target's + // displaced copy would read young even without `touch_dir`) — that + // is covered by `displace_existing_target_refreshes_displaced_mtime` + // below. This test instead proves the sweep spares young siblings + // through the full public `export_to_filesystem` path, not just the + // private sweep helper. + let decoy_target = temp.path().join("decoy-out"); + fs::create_dir_all(&decoy_target).unwrap(); + let sibling_displaced = + FileSystem::displace_existing_target(&decoy_target, temp.path()).unwrap(); let vfs = FilesBuilder::new() .add_file("/tool.ts", "export {}") @@ -1213,11 +1347,76 @@ mod tests { .unwrap(); vfs.export_to_filesystem(&target).unwrap(); - assert!(!orphan_staging.exists()); - assert!(!orphan_stale.exists()); + assert!( + sibling_staging_path.exists(), + "too young to be a crash orphan" + ); + assert!( + sibling_displaced.exists(), + "too young to be a crash orphan (mtime just refreshed)" + ); assert!(target.join("tool.ts").exists()); } + #[test] + fn displace_existing_target_refreshes_displaced_mtime() { + let tmp = TempDir::new().unwrap(); + let target = tmp.path().join("out"); + fs::create_dir_all(&target).unwrap(); + let target_created_at = fs::metadata(&target).unwrap().modified().unwrap(); + + // Give the filesystem's mtime clock room to move past `target`'s + // creation time before it gets renamed aside, so the comparison + // below can distinguish "displaced kept target's original mtime" + // (issue #169's S1 gap: a rename alone does not reset mtime) from + // "displaced's mtime was refreshed by `touch_dir`" (the fix). + std::thread::sleep(Duration::from_millis(1100)); + + let displaced = FileSystem::displace_existing_target(&target, tmp.path()).unwrap(); + let displaced_modified = fs::metadata(&displaced).unwrap().modified().unwrap(); + + assert!( + displaced_modified > target_created_at, + "displaced's mtime must be refreshed around the rename, not inherited from target" + ); + } + + #[test] + fn sweep_stale_artifacts_skips_recent_orphans() { + let tmp = TempDir::new().unwrap(); + let target = tmp.path().join("out"); + + let staging = tempfile::Builder::new() + .prefix(&FileSystem::staging_prefix(&target)) + .tempdir_in(tmp.path()) + .unwrap(); + let staging_path = staging.path().to_path_buf(); + + // Real production threshold: a just-created dir must never be swept. + FileSystem::sweep_stale_artifacts(tmp.path(), &target); + + assert!(staging_path.exists(), "a fresh sibling must not be swept"); + } + + #[test] + fn sweep_stale_artifacts_older_than_removes_aged_orphans() { + let tmp = TempDir::new().unwrap(); + let target = tmp.path().join("out"); + + let staging = tempfile::Builder::new() + .prefix(&FileSystem::staging_prefix(&target)) + .tempdir_in(tmp.path()) + .unwrap(); + let staging_path = staging.keep(); // disown so the assert below is meaningful + + FileSystem::sweep_stale_artifacts_older_than(tmp.path(), &target, Duration::ZERO); + + assert!( + !staging_path.exists(), + "an orphan past the age threshold must be swept" + ); + } + #[test] fn test_export_to_nonexistent_target_directory() { let temp = TempDir::new().unwrap(); diff --git a/crates/mcp-server/src/service.rs b/crates/mcp-server/src/service.rs index 3d0c1cb..40bdf91 100644 --- a/crates/mcp-server/src/service.rs +++ b/crates/mcp-server/src/service.rs @@ -28,7 +28,7 @@ use rmcp::model::{ }; use rmcp::{ErrorData as McpError, tool, tool_handler, tool_router}; use std::collections::{HashMap, HashSet}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use tokio::sync::Mutex; @@ -73,6 +73,13 @@ pub struct GeneratorService { /// the `discover_server` await point. introspectors: Arc>>>>, + /// Per-output-directory export locks, keyed by the (uncanonicalized) + /// output path as supplied to `introspect_server` / stored on the + /// pending generation. Same rationale as `introspectors`: keying by the + /// contended resource means an export for one `output_dir` never blocks + /// an export for a different one. + exports: Arc>>>>, + /// Clock used to construct pending generations (shared with `state`) clock: Arc, @@ -98,6 +105,7 @@ impl GeneratorService { Self { state: Arc::new(StateManager::with_clock(Arc::clone(&clock))), introspectors: Arc::new(Mutex::new(HashMap::new())), + exports: Arc::new(Mutex::new(HashMap::new())), clock, tool_router: Self::tool_router(), } @@ -138,6 +146,48 @@ impl GeneratorService { entry.remove(); } } + + /// Returns the per-output-directory export lock, creating one if absent. + /// + /// Mirrors [`Self::introspector_for`]: the outer map lock is released + /// before the returned handle is awaited on, so exports to unrelated + /// output directories never contend on it. Holding this lock across an + /// [`mcp_execution_files::FileSystem::export_to_filesystem`] call + /// serializes any two concurrent `save_categorized_tools` calls for the + /// same `output_dir` that overlap while holding the same handle, + /// narrowing the in-process trigger for the data-loss race described in + /// issue #169. This is not an unconditional guarantee across three or + /// more overlapping calls: a call that fetches a fresh handle only + /// after an earlier holder has already evicted its own can still run + /// concurrently with a still-in-flight call holding the stale handle + /// (same eviction-boundary gap as [`Self::evict_introspector`]). The + /// age-gated sweep in `mcp-execution-files` is what ultimately prevents + /// data loss if that happens. + async fn export_lock_for(&self, output_dir: &Path) -> Arc> { + let mut exports = self.exports.lock().await; + exports + .entry(output_dir.to_path_buf()) + .or_insert_with(|| Arc::new(Mutex::new(()))) + .clone() + } + + /// Evicts the per-output-directory export lock after use, but only if + /// the map still holds the exact handle the caller obtained. + /// + /// Same identity-checked eviction as [`Self::evict_introspector`] and + /// for the same reason: `output_dir` values are caller-supplied, so + /// without eviction the map grows without bound, and an unconditional + /// `remove` keyed only by path would be a TOCTOU bug against a + /// concurrently inserted fresh handle. + async fn evict_export_lock(&self, output_dir: &Path, handle: &Arc>) { + let mut exports = self.exports.lock().await; + if let std::collections::hash_map::Entry::Occupied(entry) = + exports.entry(output_dir.to_path_buf()) + && Arc::ptr_eq(entry.get(), handle) + { + entry.remove(); + } + } } impl Default for GeneratorService { @@ -339,10 +389,22 @@ impl GeneratorService { })?; } - // Export to filesystem (blocking operation wrapped in spawn_blocking) + // Export to filesystem (blocking operation wrapped in spawn_blocking). + // Held across the export so a second concurrent call for the same + // output_dir blocks until the first finishes, rather than racing on + // the underlying staging/swap (see `export_lock_for`). + let export_lock = self.export_lock_for(&pending.output_dir).await; + let export_guard = export_lock.lock().await; + let output_dir = pending.output_dir.clone(); - tokio::task::spawn_blocking(move || vfs.export_to_filesystem(&output_dir)) - .await + let export_result = + tokio::task::spawn_blocking(move || vfs.export_to_filesystem(&output_dir)).await; + + drop(export_guard); + self.evict_export_lock(&pending.output_dir, &export_lock) + .await; + + export_result .map_err(|e| McpError::internal_error(format!("Task join error: {e}"), None))? .map_err(|e| McpError::internal_error(format!("Failed to export files: {e}"), None))?; @@ -877,12 +939,14 @@ mod tests { fn test_generator_service_new() { let service = GeneratorService::new(); assert!(service.introspectors.try_lock().is_ok()); + assert!(service.exports.try_lock().is_ok()); } #[test] fn test_generator_service_default() { let service = GeneratorService::default(); assert!(service.introspectors.try_lock().is_ok()); + assert!(service.exports.try_lock().is_ok()); } #[test] @@ -1261,6 +1325,78 @@ mod tests { ); } + // ======================================================================== + // Per-output-directory export locking Tests (issue #169) + // + // Mirrors the `introspector_for` tests above: these exercise the exact + // `Arc>` handles and keyed-lock pattern that + // `save_categorized_tools` relies on via `export_lock_for`, without + // driving a real export through the filesystem. + // ======================================================================== + + /// Same `output_dir` must resolve to the same export lock, so a second + /// concurrent export for that directory cannot proceed until the first + /// releases it. + #[tokio::test] + async fn test_export_lock_for_same_output_dir_shares_one_lock() { + let service = GeneratorService::new(); + let output_dir = PathBuf::from("/tmp/same-output-dir-lock-test"); + + let handle_a = service.export_lock_for(&output_dir).await; + let handle_b = service.export_lock_for(&output_dir).await; + + assert!( + Arc::ptr_eq(&handle_a, &handle_b), + "the same output_dir must reuse one export lock" + ); + } + + /// Different `output_dir`s must resolve to independent export locks, so + /// exports for unrelated directories never contend on the same mutex. + #[tokio::test] + async fn test_export_lock_for_different_output_dirs_get_independent_locks() { + let service = GeneratorService::new(); + + let handle_a = service + .export_lock_for(&PathBuf::from("/tmp/diff-output-dir-lock-a")) + .await; + let handle_b = service + .export_lock_for(&PathBuf::from("/tmp/diff-output-dir-lock-b")) + .await; + + assert!( + !Arc::ptr_eq(&handle_a, &handle_b), + "different output_dirs must get independent export locks" + ); + } + + /// `evict_export_lock` must be identity-checked, not just keyed by + /// `output_dir`, mirroring `test_stale_eviction_does_not_remove_unrelated_entry`. + #[tokio::test] + async fn test_export_lock_stale_eviction_does_not_remove_unrelated_entry() { + let service = GeneratorService::new(); + let output_dir = PathBuf::from("/tmp/toctou-export-lock-test"); + + let handle_a = service.export_lock_for(&output_dir).await; + let handle_b = service.export_lock_for(&output_dir).await; + assert!(Arc::ptr_eq(&handle_a, &handle_b)); + + service.evict_export_lock(&output_dir, &handle_a).await; + assert!(service.exports.lock().await.is_empty()); + + let handle_c = service.export_lock_for(&output_dir).await; + assert!(!Arc::ptr_eq(&handle_b, &handle_c)); + + // B's stale eviction attempt must be a no-op: C's live entry survives. + service.evict_export_lock(&output_dir, &handle_b).await; + let exports = service.exports.lock().await; + let current = exports + .get(&output_dir) + .expect("C's entry must survive B's stale eviction attempt"); + assert!(Arc::ptr_eq(current, &handle_c)); + drop(exports); + } + // ======================================================================== // save_categorized_tools Error Tests // ========================================================================