From cf978c1346db4799dbb60585f6fac1b0d4497179 Mon Sep 17 00:00:00 2001 From: SaladDay Date: Mon, 3 Aug 2026 00:38:02 +0000 Subject: [PATCH] fix(pi): make shared file commits collision safe --- src-tauri/src/pi_config/document.rs | 56 ++- src-tauri/src/pi_config/native_settings.rs | 52 +- src-tauri/src/pi_config/shared_file.rs | 555 ++++++++++++++++++++- 3 files changed, 629 insertions(+), 34 deletions(-) diff --git a/src-tauri/src/pi_config/document.rs b/src-tauri/src/pi_config/document.rs index d94a339a3..9973d6c6f 100644 --- a/src-tauri/src/pi_config/document.rs +++ b/src-tauri/src/pi_config/document.rs @@ -13,13 +13,14 @@ use jsonc_parser::cst::{ use jsonc_parser::ParseOptions; use regex::Regex; use serde_json::Value; -use sha2::{Digest, Sha256}; use std::collections::HashMap; use std::fs::{self, File, Metadata, OpenOptions}; use std::io::{Read, Take}; use std::path::{Path, PathBuf}; use std::sync::{Arc, LazyLock, Mutex, MutexGuard}; +use super::shared_file::compare_exchange_shared_file_bytes; + const MAX_PI_MODELS_BYTES: u64 = 8 * 1024 * 1024; const EMPTY_MODELS_DOCUMENT: &str = "{\"providers\":{}}"; const MAX_MUTATION_ATTEMPTS: usize = 3; @@ -390,10 +391,6 @@ pub(super) fn read_pi_models_document(path: &Path) -> Result) -> Option<[u8; 32]> { - bytes.map(|bytes| Sha256::digest(bytes).into()) -} - fn serialize_models_mutation( path: &Path, before: Option<&[u8]>, @@ -449,7 +446,6 @@ pub(crate) fn apply_pi_provider_patch( for _ in 0..MAX_MUTATION_ATTEMPTS { let before = read_models_bytes(path)?; - let observed = fingerprint(before.as_deref()); let serialized = serialize_models_mutation(path, before.as_deref(), &|document| { let providers = document .as_object_mut() @@ -469,12 +465,18 @@ pub(crate) fn apply_pi_provider_patch( Ok(()) })?; - let current = read_models_bytes(path)?; - if fingerprint(current.as_deref()) != observed { - continue; + match compare_exchange_shared_file_bytes( + path, + before.as_deref(), + &serialized, + MAX_PI_MODELS_BYTES, + None, + "Pi models file", + ) { + Ok(_) => return Ok(()), + Err(AppError::Conflict(_)) => continue, + Err(error) => return Err(error), } - crate::config::atomic_write(path, &serialized)?; - return Ok(()); } Err(AppError::Conflict(format!( @@ -570,6 +572,38 @@ mod tests { ); } + #[test] + fn external_rename_during_patch_is_reparsed_before_owned_fields_change() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("models.json"); + fs::write( + &path, + r#"{"providers":{"managed":{"models":[{"id":"old"}]}}}"#, + ) + .expect("seed"); + crate::pi_config::shared_file::replace_before_next_compare_exchange( + &path, + br#"{ + "externalRevision": 7, + "providers": { + "native": {"models": [{"id": "external"}]}, + "managed": {"models": [{"id": "old"}]} + } +}"#, + ); + let patch = IndexMap::from([( + "managed".to_string(), + Some(serde_json::json!({"models": [{"id": "new"}]})), + )]); + + apply_pi_provider_patch(&path, &patch).expect("retry patch"); + + let saved: Value = serde_json::from_slice(&fs::read(&path).expect("read")).expect("parse"); + assert_eq!(saved["externalRevision"], 7); + assert_eq!(saved["providers"]["native"]["models"][0]["id"], "external"); + assert_eq!(saved["providers"]["managed"]["models"][0]["id"], "new"); + } + #[test] fn exact_key_delete_does_not_delete_same_content_sibling() { let temp = tempfile::tempdir().expect("tempdir"); diff --git a/src-tauri/src/pi_config/native_settings.rs b/src-tauri/src/pi_config/native_settings.rs index f5edc3b09..213bf8c27 100644 --- a/src-tauri/src/pi_config/native_settings.rs +++ b/src-tauri/src/pi_config/native_settings.rs @@ -6,12 +6,13 @@ use crate::error::AppError; use serde::{Deserialize, Serialize}; use serde_json::{Map, Value}; -use sha2::{Digest, Sha256}; use std::fs::{self, File, Metadata, OpenOptions}; use std::io::{Read, Take}; use std::path::{Path, PathBuf}; use std::sync::{LazyLock, Mutex}; +use super::shared_file::compare_exchange_shared_file_bytes; + const MAX_PI_SETTINGS_BYTES: u64 = 1024 * 1024; const MAX_WRITE_ATTEMPTS: usize = 3; static SETTINGS_WRITE_LOCK: LazyLock> = LazyLock::new(|| Mutex::new(())); @@ -130,7 +131,6 @@ fn mutate_settings_document( for _ in 0..MAX_WRITE_ATTEMPTS { let before = read_regular_bytes(path, MAX_PI_SETTINGS_BYTES)?; - let observed = fingerprint(before.as_deref()); let mut document = match before.as_deref() { Some(bytes) => { serde_json::from_slice(bytes).map_err(|error| AppError::json(path, error))? @@ -148,12 +148,18 @@ fn mutate_settings_document( .map_err(|source| AppError::JsonSerialize { source })?; serialized.push(b'\n'); - let current = read_regular_bytes(path, MAX_PI_SETTINGS_BYTES)?; - if fingerprint(current.as_deref()) != observed { - continue; + match compare_exchange_shared_file_bytes( + path, + before.as_deref(), + &serialized, + MAX_PI_SETTINGS_BYTES, + None, + "Pi settings", + ) { + Ok(_) => return Ok(()), + Err(AppError::Conflict(_)) => continue, + Err(error) => return Err(error), } - crate::config::atomic_write(path, &serialized)?; - return Ok(()); } Err(AppError::Conflict(format!( @@ -169,10 +175,6 @@ fn read_settings_document(path: &Path) -> Result { } } -fn fingerprint(bytes: Option<&[u8]>) -> Option<[u8; 32]> { - bytes.map(|bytes| Sha256::digest(bytes).into()) -} - #[cfg(unix)] fn open_read_only(path: &Path) -> std::io::Result { use std::os::unix::fs::OpenOptionsExt; @@ -286,6 +288,34 @@ mod tests { assert_eq!(saved["defaultModel"], "model"); } + #[test] + fn external_rename_during_settings_patch_is_reparsed_before_retry() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("settings.json"); + fs::write( + &path, + br#"{"theme":"before","defaultProvider":"old","defaultModel":"old"}"#, + ) + .expect("seed"); + crate::pi_config::shared_file::replace_before_next_compare_exchange( + &path, + br#"{"theme":"external","packages":["foreign"],"defaultProvider":"old","defaultModel":"old"}"#, + ); + + mutate_settings_document(&path, |root| { + root.insert("defaultProvider".into(), json!("managed")); + root.insert("defaultModel".into(), json!("model")); + Ok(()) + }) + .expect("retry mutation"); + + let saved: Value = serde_json::from_slice(&fs::read(&path).expect("read")).expect("parse"); + assert_eq!(saved["theme"], "external"); + assert_eq!(saved["packages"], json!(["foreign"])); + assert_eq!(saved["defaultProvider"], "managed"); + assert_eq!(saved["defaultModel"], "model"); + } + #[cfg(unix)] #[test] fn settings_symlink_is_rejected() { diff --git a/src-tauri/src/pi_config/shared_file.rs b/src-tauri/src/pi_config/shared_file.rs index 56ccde3f5..a3be586cc 100644 --- a/src-tauri/src/pi_config/shared_file.rs +++ b/src-tauri/src/pi_config/shared_file.rs @@ -2,19 +2,28 @@ //! //! Callers choose an exact path and size limit. This layer supplies bounded //! regular-file reads, symlink rejection, optimistic revisions, per-path -//! process locking, durable atomic replacement, and compare-before-delete. +//! process locking, and OS-backed compare/exchange replacement. The latter is +//! deliberately stronger than "read, compare, rename": the displaced path is +//! inspected after one atomic namespace operation, so an external Pi/user +//! rename in the commit window is restored instead of overwritten. use crate::error::AppError; use sha2::{Digest, Sha256}; use std::collections::HashMap; use std::fs::{self, File, Metadata, OpenOptions}; -use std::io::{Read, Take}; +use std::io::{Read, Take, Write}; use std::path::{Path, PathBuf}; use std::sync::{Arc, LazyLock, Mutex}; static FILE_LOCKS: LazyLock>>>> = LazyLock::new(|| Mutex::new(HashMap::new())); +#[cfg(test)] +type CompareExchangeHooks = HashMap>; +#[cfg(test)] +static BEFORE_COMPARE_EXCHANGE: LazyLock> = + LazyLock::new(|| Mutex::new(HashMap::new())); + #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct SharedFileSnapshot { pub revision: String, @@ -58,8 +67,14 @@ pub(crate) fn replace_shared_file( .map_err(|error| AppError::Config(format!("Pi file lock is poisoned: {error}")))?; let current = read_shared_file(path, max_bytes, label)?; ensure_revision(path, expected_revision, ¤t.revision)?; - crate::config::atomic_write_durable(path, bytes, new_file_mode)?; - read_shared_file(path, max_bytes, label) + compare_exchange_under_lock( + path, + current.bytes.as_deref(), + Some(bytes), + max_bytes, + new_file_mode, + label, + ) } pub(crate) fn delete_shared_file( @@ -77,16 +92,507 @@ pub(crate) fn delete_shared_file( if !current.exists() { return Ok(false); } - fs::remove_file(path).map_err(|error| AppError::io(path, error))?; - #[cfg(unix)] - if let Some(parent) = path.parent() { - File::open(parent) - .and_then(|directory| directory.sync_all()) - .map_err(|error| AppError::io(parent, error))?; - } + compare_exchange_under_lock(path, current.bytes.as_deref(), None, max_bytes, None, label)?; Ok(true) } +/// Atomically replace the exact bytes a caller parsed. +/// +/// This is the common commit primitive for shared Pi documents. Callers may +/// retry `Conflict` after reparsing, but other failures are fail-closed. The +/// target never gets replaced merely because a pre-rename fingerprint happened +/// to match. +pub(crate) fn compare_exchange_shared_file_bytes( + path: &Path, + expected: Option<&[u8]>, + replacement: &[u8], + max_bytes: u64, + new_file_mode: Option, + label: &str, +) -> Result { + if replacement.len() as u64 > max_bytes { + return Err(AppError::InvalidInput(format!( + "{label} exceeds the {max_bytes}-byte limit" + ))); + } + let lock = path_lock(path)?; + let _guard = lock + .lock() + .map_err(|error| AppError::Config(format!("Pi file lock is poisoned: {error}")))?; + compare_exchange_under_lock( + path, + expected, + Some(replacement), + max_bytes, + new_file_mode, + label, + ) +} + +fn compare_exchange_under_lock( + path: &Path, + expected: Option<&[u8]>, + replacement: Option<&[u8]>, + max_bytes: u64, + new_file_mode: Option, + label: &str, +) -> Result { + if replacement.is_some_and(|bytes| bytes.len() as u64 > max_bytes) { + return Err(AppError::InvalidInput(format!( + "{label} exceeds the {max_bytes}-byte limit" + ))); + } + let parent = path.parent().ok_or_else(|| { + AppError::InvalidInput(format!("{label} path has no parent: {}", path.display())) + })?; + fs::create_dir_all(parent).map_err(|error| AppError::io(parent, error))?; + + // This preflight rejects symlinks/non-regular files and avoids a namespace + // operation when the conflict is already visible. Correctness still rests + // on inspecting the displaced file after the atomic operation below. + let before = read_regular_bytes(path, max_bytes, label)?; + if before.as_deref() != expected { + return Err(concurrent_change(path, label)); + } + + let staged = replacement + .map(|bytes| stage_replacement(path, bytes, before.is_some(), new_file_mode)) + .transpose()?; + run_before_compare_exchange_hook(path)?; + + let result = match (expected, replacement, staged.as_deref()) { + (None, Some(bytes), Some(staged)) => match rename_noreplace(staged, path) { + Ok(()) => { + sync_parent(parent)?; + Ok(snapshot(Some(bytes))) + } + Err(error) if is_destination_exists(&error) => Err(concurrent_change(path, label)), + Err(error) => Err(rename_error("create", staged, path, error)), + }, + (Some(expected), Some(replacement), Some(staged)) => { + replace_existing_if_equal(path, staged, expected, replacement, max_bytes, label) + } + (Some(expected), None, None) => delete_existing_if_equal(path, expected, max_bytes, label), + (None, None, None) => Ok(snapshot(None)), + _ => Err(AppError::Config( + "invalid Pi shared-file compare/exchange state".to_string(), + )), + }; + + if let Some(staged) = staged { + // Success consumes or removes the staged path. On a failed rollback it + // intentionally remains as a recovery artifact containing data that + // must not be discarded, so only remove a file still containing our + // proposed replacement. + if replacement.is_some_and(|bytes| { + read_regular_bytes(&staged, max_bytes, label) + .ok() + .flatten() + .as_deref() + == Some(bytes) + }) { + let _ = fs::remove_file(&staged); + } + } + result +} + +fn replace_existing_if_equal( + path: &Path, + staged: &Path, + expected: &[u8], + replacement: &[u8], + max_bytes: u64, + label: &str, +) -> Result { + let displaced = match install_over_existing(staged, path) { + Ok(displaced) => displaced, + Err(error) + if matches!( + error.kind(), + std::io::ErrorKind::NotFound | std::io::ErrorKind::AlreadyExists + ) => + { + return Err(concurrent_change(path, label)); + } + Err(error) => return Err(rename_error("exchange", staged, path, error)), + }; + let displaced_bytes = read_regular_bytes(&displaced, max_bytes, label); + if matches!( + displaced_bytes, + Ok(ref bytes) if bytes.as_deref() == Some(expected) + ) { + fs::remove_file(&displaced).map_err(|error| AppError::io(&displaced, error))?; + sync_parent( + path.parent() + .expect("a path accepted by compare/exchange has a parent"), + )?; + return Ok(snapshot(Some(replacement))); + } + + let proposed_or_raced = restore_displaced(&displaced, path).map_err(|error| { + AppError::Conflict(format!( + "{label} changed during atomic replacement and could not be restored; \ + the displaced bytes remain at {}: {error}", + displaced.display() + )) + })?; + let proposed_bytes = read_regular_bytes(&proposed_or_raced, max_bytes, label)?; + if proposed_bytes.as_deref() == Some(replacement) { + fs::remove_file(&proposed_or_raced) + .map_err(|error| AppError::io(&proposed_or_raced, error))?; + } + sync_parent( + path.parent() + .expect("a path accepted by compare/exchange has a parent"), + )?; + match displaced_bytes { + Ok(_) => Err(concurrent_change(path, label)), + Err(error) => Err(AppError::Conflict(format!( + "{label} became unsafe during atomic replacement and was restored: {error}" + ))), + } +} + +fn delete_existing_if_equal( + path: &Path, + expected: &[u8], + max_bytes: u64, + label: &str, +) -> Result { + let quarantine = sibling_temp_path(path, "delete"); + rename_noreplace(path, &quarantine) + .map_err(|error| rename_error("quarantine", path, &quarantine, error))?; + let quarantined = read_regular_bytes(&quarantine, max_bytes, label); + if matches!( + quarantined, + Ok(ref bytes) if bytes.as_deref() == Some(expected) + ) { + fs::remove_file(&quarantine).map_err(|error| AppError::io(&quarantine, error))?; + sync_parent( + path.parent() + .expect("a path accepted by compare/exchange has a parent"), + )?; + return Ok(snapshot(None)); + } + + match rename_noreplace(&quarantine, path) { + Ok(()) => { + sync_parent( + path.parent() + .expect("a path accepted by compare/exchange has a parent"), + )?; + match quarantined { + Ok(_) => Err(concurrent_change(path, label)), + Err(error) => Err(AppError::Conflict(format!( + "{label} became unsafe during delete and was restored: {error}" + ))), + } + } + Err(error) => Err(AppError::Conflict(format!( + "{label} changed during delete and the displaced bytes remain at {}: {error}", + quarantine.display() + ))), + } +} + +fn stage_replacement( + path: &Path, + bytes: &[u8], + preserve_mode: bool, + new_file_mode: Option, +) -> Result { + let staged = sibling_temp_path(path, "cas"); + let mut options = OpenOptions::new(); + options.create_new(true).write(true); + #[cfg(unix)] + { + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + let mode = if preserve_mode { + fs::metadata(path) + .map(|metadata| metadata.permissions().mode()) + .unwrap_or_else(|_| new_file_mode.unwrap_or(0o666)) + } else { + new_file_mode.unwrap_or(0o666) + }; + options.mode(mode); + } + let mut file = options + .open(&staged) + .map_err(|error| AppError::io(&staged, error))?; + file.write_all(bytes) + .map_err(|error| AppError::io(&staged, error))?; + file.flush() + .and_then(|_| file.sync_all()) + .map_err(|error| AppError::io(&staged, error))?; + Ok(staged) +} + +fn sibling_temp_path(path: &Path, purpose: &str) -> PathBuf { + let parent = path + .parent() + .expect("a path accepted by compare/exchange has a parent"); + let name = path + .file_name() + .expect("a path accepted by compare/exchange has a file name") + .to_string_lossy(); + parent.join(format!( + ".{name}.{purpose}.{}", + uuid::Uuid::new_v4().simple() + )) +} + +fn snapshot(bytes: Option<&[u8]>) -> SharedFileSnapshot { + SharedFileSnapshot { + revision: revision(bytes), + bytes: bytes.map(ToOwned::to_owned), + } +} + +fn concurrent_change(path: &Path, label: &str) -> AppError { + AppError::Conflict(format!( + "{label} changed since it was read: {}", + path.display() + )) +} + +fn rename_error( + operation: &str, + source: &Path, + destination: &Path, + error: std::io::Error, +) -> AppError { + AppError::IoContext { + context: format!( + "Pi shared-file {operation} failed: {} -> {}", + source.display(), + destination.display() + ), + source: error, + } +} + +#[cfg(unix)] +fn sync_parent(parent: &Path) -> Result<(), AppError> { + File::open(parent) + .and_then(|directory| directory.sync_all()) + .map_err(|error| AppError::io(parent, error)) +} + +#[cfg(not(unix))] +fn sync_parent(_parent: &Path) -> Result<(), AppError> { + Ok(()) +} + +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn c_path(path: &Path) -> std::io::Result { + use std::os::unix::ffi::OsStrExt; + std::ffi::CString::new(path.as_os_str().as_bytes()) + .map_err(|_| std::io::Error::new(std::io::ErrorKind::InvalidInput, "path contains NUL")) +} + +#[cfg(target_os = "linux")] +fn rename_noreplace(source: &Path, destination: &Path) -> std::io::Result<()> { + let source = c_path(source)?; + let destination = c_path(destination)?; + // SAFETY: both C strings remain alive and renameat2 performs one + // synchronous namespace operation. + let result = unsafe { + libc::renameat2( + libc::AT_FDCWD, + source.as_ptr(), + libc::AT_FDCWD, + destination.as_ptr(), + libc::RENAME_NOREPLACE, + ) + }; + if result == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +#[cfg(target_os = "linux")] +fn exchange_paths(left: &Path, right: &Path) -> std::io::Result<()> { + let left = c_path(left)?; + let right = c_path(right)?; + // SAFETY: both C strings remain alive and renameat2 performs one + // synchronous namespace operation. + let result = unsafe { + libc::renameat2( + libc::AT_FDCWD, + left.as_ptr(), + libc::AT_FDCWD, + right.as_ptr(), + libc::RENAME_EXCHANGE, + ) + }; + if result == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +#[cfg(target_os = "macos")] +fn rename_with_flags(source: &Path, destination: &Path, flags: u32) -> std::io::Result<()> { + let source = c_path(source)?; + let destination = c_path(destination)?; + // SAFETY: both C strings remain alive and renamex_np performs one + // synchronous namespace operation. + let result = unsafe { libc::renamex_np(source.as_ptr(), destination.as_ptr(), flags) }; + if result == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +#[cfg(target_os = "macos")] +fn rename_noreplace(source: &Path, destination: &Path) -> std::io::Result<()> { + rename_with_flags(source, destination, libc::RENAME_EXCL) +} + +#[cfg(target_os = "macos")] +fn exchange_paths(left: &Path, right: &Path) -> std::io::Result<()> { + rename_with_flags(left, right, libc::RENAME_SWAP) +} + +#[cfg(windows)] +fn wide_path(path: &Path) -> Vec { + use std::os::windows::ffi::OsStrExt; + path.as_os_str().encode_wide().chain(Some(0)).collect() +} + +#[cfg(windows)] +fn rename_noreplace(source: &Path, destination: &Path) -> std::io::Result<()> { + use windows_sys::Win32::Storage::FileSystem::{MoveFileExW, MOVEFILE_WRITE_THROUGH}; + let source = wide_path(source); + let destination = wide_path(destination); + // SAFETY: both buffers are NUL-terminated and remain alive during the + // synchronous Win32 call. No REPLACE_EXISTING flag is supplied. + let result = unsafe { + MoveFileExW( + source.as_ptr(), + destination.as_ptr(), + MOVEFILE_WRITE_THROUGH, + ) + }; + if result != 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +#[cfg(windows)] +fn replace_file_with_backup(path: &Path, replacement: &Path, backup: &Path) -> std::io::Result<()> { + use windows_sys::Win32::Storage::FileSystem::{ReplaceFileW, REPLACEFILE_WRITE_THROUGH}; + let path = wide_path(path); + let replacement = wide_path(replacement); + let backup = wide_path(backup); + // SAFETY: all buffers are NUL-terminated and remain alive during the + // synchronous Win32 call. + let result = unsafe { + ReplaceFileW( + path.as_ptr(), + replacement.as_ptr(), + backup.as_ptr(), + REPLACEFILE_WRITE_THROUGH, + std::ptr::null(), + std::ptr::null(), + ) + }; + if result != 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn install_over_existing(staged: &Path, path: &Path) -> std::io::Result { + exchange_paths(staged, path)?; + Ok(staged.to_path_buf()) +} + +#[cfg(windows)] +fn install_over_existing(staged: &Path, path: &Path) -> std::io::Result { + let backup = sibling_temp_path(path, "displaced"); + replace_file_with_backup(path, staged, &backup)?; + Ok(backup) +} + +#[cfg(any(target_os = "linux", target_os = "macos"))] +fn restore_displaced(displaced: &Path, path: &Path) -> std::io::Result { + exchange_paths(displaced, path)?; + Ok(displaced.to_path_buf()) +} + +#[cfg(windows)] +fn restore_displaced(displaced: &Path, path: &Path) -> std::io::Result { + let backup = sibling_temp_path(path, "rejected"); + replace_file_with_backup(path, displaced, &backup)?; + Ok(backup) +} + +#[cfg(not(any(target_os = "linux", target_os = "macos", windows)))] +fn rename_noreplace(_source: &Path, _destination: &Path) -> std::io::Result<()> { + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "atomic no-replace rename is unsupported on this platform", + )) +} + +#[cfg(not(any(target_os = "linux", target_os = "macos", windows)))] +fn install_over_existing(_staged: &Path, _path: &Path) -> std::io::Result { + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "atomic file exchange is unsupported on this platform", + )) +} + +#[cfg(not(any(target_os = "linux", target_os = "macos", windows)))] +fn restore_displaced(_displaced: &Path, _path: &Path) -> std::io::Result { + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "atomic file exchange is unsupported on this platform", + )) +} + +fn is_destination_exists(error: &std::io::Error) -> bool { + error.kind() == std::io::ErrorKind::AlreadyExists + || matches!(error.raw_os_error(), Some(libc::EEXIST)) +} + +#[cfg(test)] +pub(crate) fn replace_before_next_compare_exchange(path: &Path, bytes: &[u8]) { + BEFORE_COMPARE_EXCHANGE + .lock() + .expect("compare/exchange test hook lock") + .insert(path.to_path_buf(), bytes.to_vec()); +} + +#[cfg(test)] +fn run_before_compare_exchange_hook(path: &Path) -> Result<(), AppError> { + let replacement = { + let mut hook = BEFORE_COMPARE_EXCHANGE + .lock() + .map_err(|error| AppError::Config(format!("Pi CAS test hook is poisoned: {error}")))?; + hook.remove(path) + }; + if let Some(bytes) = replacement { + crate::config::atomic_write_durable(path, &bytes, None)?; + } + Ok(()) +} + +#[cfg(not(test))] +fn run_before_compare_exchange_hook(_path: &Path) -> Result<(), AppError> { + Ok(()) +} + fn ensure_revision(path: &Path, expected: &str, actual: &str) -> Result<(), AppError> { if expected == actual { Ok(()) @@ -176,7 +682,7 @@ fn read_regular_bytes( } let mut file = open_read_only(path).map_err(|error| AppError::io(path, error))?; let opened = file.metadata().map_err(|error| AppError::io(path, error))?; - let bytes = read_limited(file.by_ref().take(max_bytes + 1), path, max_bytes)?; + let bytes = read_limited(Read::by_ref(&mut file).take(max_bytes + 1), path, max_bytes)?; let completed = file.metadata().map_err(|error| AppError::io(path, error))?; let current = fs::symlink_metadata(path).map_err(|error| AppError::io(path, error))?; if !current.file_type().is_file() @@ -216,6 +722,31 @@ mod tests { assert!(delete_shared_file(&path, &replaced.revision, 1024, "test").expect("delete")); } + #[test] + fn external_rename_in_the_commit_window_is_restored_without_data_loss() { + let temp = tempfile::tempdir().expect("tempdir"); + let path = temp.path().join("shared.md"); + fs::write(&path, b"observed").expect("seed"); + let observed = read_shared_file(&path, 1024, "test").expect("snapshot"); + + replace_before_next_compare_exchange(&path, b"external replacement"); + let replace = replace_shared_file(&path, &observed.revision, b"ours", 1024, None, "test"); + assert!(matches!(replace, Err(AppError::Conflict(_)))); + assert_eq!( + fs::read(&path).expect("external bytes restored"), + b"external replacement" + ); + + let observed = read_shared_file(&path, 1024, "test").expect("snapshot"); + replace_before_next_compare_exchange(&path, b"external before delete"); + let delete = delete_shared_file(&path, &observed.revision, 1024, "test"); + assert!(matches!(delete, Err(AppError::Conflict(_)))); + assert_eq!( + fs::read(&path).expect("external bytes restored"), + b"external before delete" + ); + } + #[cfg(unix)] #[test] fn symlink_targets_fail_closed() {