diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c1ce1f55..41f3b39f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -20,6 +20,15 @@ permissions: contents: read jobs: + windows-install: + runs-on: windows-latest + steps: + - uses: actions/checkout@v7 + - name: Local copy install, replacement, and removal + run: cargo test --locked -p skilld-native --test install_copy + - name: Store transactions and Windows file locks + run: cargo test --locked -p skilld-command --test local_store + quality: runs-on: ubuntu-24.04 steps: diff --git a/crates/skilld-command/src/local_store.rs b/crates/skilld-command/src/local_store.rs index 98d128df..99deefae 100644 --- a/crates/skilld-command/src/local_store.rs +++ b/crates/skilld-command/src/local_store.rs @@ -113,7 +113,7 @@ impl StoreError { match self { Self::CommittedCleanupPending(_) => "COMMITTED_CLEANUP_PENDING", Self::Conflict(_) => "TARGET_CONFLICT", - Self::Filesystem(_) => "SERVICE_UNAVAILABLE", + Self::Filesystem(_) => "FILESYSTEM_ERROR", Self::InvalidLockfile(_) => "INVALID_LOCKFILE", Self::InvalidSource(_) => "INVALID_SOURCE", Self::InvalidTargetPath(_) => "INVALID_TARGET", @@ -469,25 +469,23 @@ impl LocalStore { ); } - if canonical_had_existing - && let Err(error) = fs::rename(&canonical, &canonical_backup).map_err(fs_error) - { + if canonical_had_existing && let Err(error) = rename_path(&canonical, &canonical_backup) { return self.rollback_error(error, known_targets); } - if let Err(error) = fs::rename(&canonical_stage, &canonical).map_err(fs_error) { + if let Err(error) = rename_path(&canonical_stage, &canonical) { return self.rollback_error(error, known_targets); } for change in &changes { let destination = change.target.destination(&name); let backup = backup_path(&destination, &transaction)?; if change.had_existing - && let Err(error) = fs::rename(&destination, &backup).map_err(fs_error) + && let Err(error) = rename_path(&destination, &backup) { return self.rollback_error(error, known_targets); } if change.install.is_some() { let stage = stage_path(&destination, &transaction)?; - if let Err(error) = fs::rename(&stage, &destination).map_err(fs_error) { + if let Err(error) = rename_path(&stage, &destination) { return self.rollback_error(error, known_targets); } } @@ -700,22 +698,19 @@ impl LocalStore { for prepared in &prepared { let canonical_backup = backup_path(&prepared.canonical, &transaction)?; if prepared.canonical_had_existing { - fs::rename(&prepared.canonical, canonical_backup).map_err(fs_error)?; + rename_path(&prepared.canonical, canonical_backup)?; } - fs::rename( + rename_path( stage_path(&prepared.canonical, &transaction)?, &prepared.canonical, - ) - .map_err(fs_error)?; + )?; for change in &prepared.changes { let destination = change.target.destination(&prepared.update.name); if change.had_existing { - fs::rename(&destination, backup_path(&destination, &transaction)?) - .map_err(fs_error)?; + rename_path(&destination, backup_path(&destination, &transaction)?)?; } if change.install.is_some() { - fs::rename(stage_path(&destination, &transaction)?, &destination) - .map_err(fs_error)?; + rename_path(stage_path(&destination, &transaction)?, &destination)?; } } } @@ -818,14 +813,13 @@ impl LocalStore { }; self.write_journal(&journal)?; if canonical_had_existing { - fs::rename(&canonical, backup_path(&canonical, &transaction)?).map_err(fs_error)?; + rename_path(&canonical, backup_path(&canonical, &transaction)?)?; } for target in &targets { let destination = target.destination(name); if path_exists(&destination)? && let Err(error) = - fs::rename(&destination, backup_path(&destination, &transaction)?) - .map_err(fs_error) + rename_path(&destination, backup_path(&destination, &transaction)?) { return self.rollback_error(error, known_targets); } @@ -1051,10 +1045,11 @@ impl LocalStore { file.write_all(&bytes).map_err(fs_error)?; file.write_all(b"\n").map_err(fs_error)?; file.sync_all().map_err(fs_error)?; + drop(file); if path_exists(&path)? { - fs::rename(&path, &backup).map_err(fs_error)?; + rename_path(&path, &backup)?; } - fs::rename(&temporary, &path).map_err(fs_error) + rename_path(&temporary, &path) } fn write_journal(&self, journal: &Journal) -> Result<(), StoreError> { @@ -1069,18 +1064,23 @@ impl LocalStore { journal.transaction_id )); let write_result = (|| { - fs::create_dir(&stage).map_err(fs_error)?; + fs::create_dir(&stage) + .map_err(|error| fs_path_error("create directory", &stage, error))?; let state = stage.join("state.json"); let bytes = serde_json::to_vec_pretty(journal) .map_err(|error| StoreError::Filesystem(error.to_string()))?; let mut file = OpenOptions::new() .write(true) .create_new(true) - .open(state) - .map_err(fs_error)?; - file.write_all(&bytes).map_err(fs_error)?; - file.sync_all().map_err(fs_error)?; - fs::rename(&stage, &path).map_err(fs_error) + .open(&state) + .map_err(|error| fs_path_error("open", &state, error))?; + file.write_all(&bytes) + .map_err(|error| fs_path_error("write", &state, error))?; + file.sync_all() + .map_err(|error| fs_path_error("sync", &state, error))?; + // Windows cannot rename this directory while its journal file is open. + drop(file); + rename_path(&stage, &path) })(); if let Err(error) = write_result { return match remove_path(&stage) { @@ -1668,7 +1668,7 @@ fn restore_path( remove_path(stage).map_err(fs_error)?; if path_exists(backup)? { remove_path(destination).map_err(fs_error)?; - fs::rename(backup, destination).map_err(fs_error)?; + rename_path(backup, destination)?; } else if !had_existing { remove_path(destination).map_err(fs_error)?; } @@ -1682,7 +1682,7 @@ fn restore_lockfile(root: &Path, transaction: &str) -> Result<(), StoreError> { remove_path(&stage).map_err(fs_error)?; if path_exists(&backup)? { remove_path(&lockfile).map_err(fs_error)?; - fs::rename(backup, lockfile).map_err(fs_error)?; + rename_path(backup, lockfile)?; } Ok(()) } @@ -1786,14 +1786,15 @@ fn ensure_write_capability() -> Result<(), StoreError> { #[cfg(not(target_os = "wasi"))] fn acquire_store_lock(root: &Path) -> Result { + let path = root.join(LOCK_NAME); let lock = OpenOptions::new() .read(true) .write(true) .create(true) .truncate(false) - .open(root.join(LOCK_NAME)) - .map_err(fs_error)?; - fs4::FileExt::lock(&lock).map_err(fs_error)?; + .open(&path) + .map_err(|error| fs_path_error("open", &path, error))?; + fs4::FileExt::lock(&lock).map_err(|error| fs_path_error("lock", &path, error))?; Ok(StoreLock { _file: lock }) } @@ -1878,6 +1879,44 @@ fn stale_update_plan() -> StoreError { StoreError::StalePlan("The Skill store changed while the update was preparing".to_owned()) } +fn fs_path_error(operation: &str, path: &Path, error: io::Error) -> StoreError { + StoreError::Filesystem(format!("Cannot {operation} {}: {error}", path.display())) +} + +fn rename_path(from: impl AsRef, to: impl AsRef) -> Result<(), StoreError> { + let from = from.as_ref(); + let to = to.as_ref(); + #[cfg(windows)] + let result = rename_with_retry(from, to); + #[cfg(not(windows))] + let result = fs::rename(from, to); + result.map_err(|error| { + StoreError::Filesystem(format!( + "Cannot rename {} to {}: {error}", + from.display(), + to.display() + )) + }) +} + +#[cfg(windows)] +fn rename_with_retry(from: &Path, to: &Path) -> io::Result<()> { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); + loop { + match fs::rename(from, to) { + Ok(()) => return Ok(()), + // Antivirus scanners can briefly hold a file without delete sharing. + Err(error) + if matches!(error.raw_os_error(), Some(5 | 32)) + && std::time::Instant::now() < deadline => + { + std::thread::sleep(std::time::Duration::from_millis(25)); + } + Err(error) => return Err(error), + } + } +} + pub(crate) fn normalize_path(path: &Path) -> PathBuf { let mut normalized = PathBuf::new(); for component in path.components() { diff --git a/crates/skilld-command/tests/local_store.rs b/crates/skilld-command/tests/local_store.rs index 97f7a9b2..f653390d 100644 --- a/crates/skilld-command/tests/local_store.rs +++ b/crates/skilld-command/tests/local_store.rs @@ -428,6 +428,101 @@ fn local_source(path: &Path) -> LockedSource { } } +#[test] +fn store_lock_failure_reports_the_filesystem_operation_and_path() { + let temporary = tempfile::tempdir().unwrap(); + let skill = source(temporary.path(), "source", "first"); + let store = LocalStore::new(temporary.path().join("project/.skills")); + let lock = store.root().join(".skilld-store-lock"); + fs::create_dir_all(&lock).unwrap(); + + let error = store + .install_from(&skill, local_source(&skill), &[], &[]) + .unwrap_err(); + + assert_eq!(error.code(), "FILESYSTEM_ERROR"); + assert!(error.to_string().contains("open"), "{error}"); + assert!( + error.to_string().contains(&lock.display().to_string()), + "{error}" + ); +} + +#[cfg(windows)] +fn hold_without_delete_sharing(path: &Path) -> fs::File { + use std::os::windows::fs::OpenOptionsExt; + fs::OpenOptions::new() + .read(true) + .share_mode(1 | 2) // FILE_SHARE_READ | FILE_SHARE_WRITE + .open(path) + .unwrap() +} + +#[cfg(windows)] +#[test] +fn replacement_retries_a_temporary_windows_file_lock() { + let temporary = tempfile::tempdir().unwrap(); + let skill = source(temporary.path(), "source", "first"); + let store = LocalStore::new(temporary.path().join("project/.skills")); + store + .install_from(&skill, local_source(&skill), &[], &[]) + .unwrap(); + let handle = hold_without_delete_sharing(&store.root().join("example/SKILL.md")); + let release = thread::spawn(move || { + thread::sleep(Duration::from_millis(250)); + drop(handle); + }); + fs::write(skill.join("SKILL.md"), skill_text("replacement")).unwrap(); + + let result = store.install_from(&skill, local_source(&skill), &[], &[]); + release.join().unwrap(); + result.unwrap(); + + assert_eq!( + fs::read_to_string(store.root().join("example/SKILL.md")).unwrap(), + skill_text("replacement") + ); +} + +#[cfg(windows)] +#[test] +fn persistent_windows_file_lock_reports_paths_and_preserves_the_install() { + let temporary = tempfile::tempdir().unwrap(); + let skill = source(temporary.path(), "source", "first"); + let store = LocalStore::new(temporary.path().join("project/.skills")); + store + .install_from(&skill, local_source(&skill), &[], &[]) + .unwrap(); + let before = store.snapshot(&[]).unwrap(); + let handle = hold_without_delete_sharing(&store.root().join("example/SKILL.md")); + fs::write(skill.join("SKILL.md"), skill_text("replacement")).unwrap(); + + let start = std::time::Instant::now(); + let error = store + .install_from(&skill, local_source(&skill), &[], &[]) + .unwrap_err(); + drop(handle); + + assert!(start.elapsed() < Duration::from_secs(10)); + assert_eq!(error.code(), "FILESYSTEM_ERROR"); + assert!(error.to_string().contains("Cannot rename"), "{error}"); + assert!( + error + .to_string() + .contains(&store.root().join("example").display().to_string()), + "{error}" + ); + assert!( + error.to_string().contains(".skilld-backup-example-"), + "{error}" + ); + assert_eq!(store.snapshot(&[]).unwrap(), before); + assert_eq!( + fs::read_to_string(store.root().join("example/SKILL.md")).unwrap(), + skill_text("first") + ); +} + #[test] fn copy_install_lists_views_and_removes_managed_state() { let temporary = tempfile::tempdir().unwrap(); diff --git a/crates/skilld-native/tests/install_copy.rs b/crates/skilld-native/tests/install_copy.rs new file mode 100644 index 00000000..87139eae --- /dev/null +++ b/crates/skilld-native/tests/install_copy.rs @@ -0,0 +1,74 @@ +use std::fs; +use std::path::Path; +use std::process::Command; + +fn run(project: &Path, home: &Path, data: &Path, args: &[&str]) { + let output = Command::new(env!("CARGO_BIN_EXE_skilld")) + .current_dir(project) + .env("HOME", home) + .env("USERPROFILE", home) + .env("APPDATA", home.join("AppData/Roaming")) + .env("LOCALAPPDATA", home.join("AppData/Local")) + .env("XDG_CONFIG_HOME", home.join(".config")) + .env("SKILLD_DATA_DIR", data) + .env("SKILLD_NO_UPGRADE", "1") + .env("SKILLD_NO_WEEKLY", "1") + .env_remove("CLAUDE_CONFIG_DIR") + .args(args) + .output() + .unwrap(); + assert!( + output.status.success(), + "{args:?}: {}\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +#[test] +fn local_copy_install_replace_and_remove_without_symlink_privileges() { + for global in [false, true] { + let temporary = tempfile::tempdir().unwrap(); + let project = temporary.path().join("project"); + let home = temporary.path().join("home"); + let data = temporary.path().join("data"); + let source = project.join("src/demo"); + fs::create_dir_all(source.join("references")).unwrap(); + fs::create_dir_all(&home).unwrap(); + fs::write(source.join("references/check.md"), "supporting file").unwrap(); + let target = if global { &home } else { &project }.join(".claude/skills/demo"); + let mut install = vec![ + "install", + "./src/demo", + "--agent", + "claude-code", + "--mode", + "copy", + ]; + let mut remove = vec!["remove", "demo"]; + if global { + install.push("--global"); + remove.push("--global"); + } + + for content in ["first", "replacement"] { + let text = format!("---\nname: demo\ndescription: demo skill\n---\n{content}\n"); + fs::write(source.join("SKILL.md"), &text).unwrap(); + run(&project, &home, &data, &install); + assert_eq!(fs::read_to_string(target.join("SKILL.md")).unwrap(), text); + assert_eq!( + fs::read_to_string(target.join("references/check.md")).unwrap(), + "supporting file" + ); + assert!( + !fs::symlink_metadata(&target) + .unwrap() + .file_type() + .is_symlink() + ); + } + run(&project, &home, &data, &remove); + assert!(!target.exists()); + assert!(source.join("SKILL.md").exists()); + } +}