From 0c181ad219fc44345fd16676c9fb52726a67cb97 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Mon, 24 Aug 2026 08:39:24 +0200 Subject: [PATCH] fix(session): make Save As sidecar cleanup transactional --- src/backend/wayland/session/tests.rs | 7 + src/session/snapshot/save/save_as.rs | 112 +------ .../snapshot/save/save_as/transaction.rs | 304 ++++++++++++++++++ .../save/save_as/transaction/tests.rs | 152 +++++++++ 4 files changed, 474 insertions(+), 101 deletions(-) create mode 100644 src/session/snapshot/save/save_as/transaction.rs create mode 100644 src/session/snapshot/save/save_as/transaction/tests.rs diff --git a/src/backend/wayland/session/tests.rs b/src/backend/wayland/session/tests.rs index 5f85025b..22590991 100644 --- a/src/backend/wayland/session/tests.rs +++ b/src/backend/wayland/session/tests.rs @@ -463,6 +463,12 @@ fn runtime_save_as_confirmed_overwrite_removes_stale_sidecars_but_keeps_lock() { .recovery_file_path() .with_extension("recovery.preserved"); std::fs::write(&preserved_recovery, b"old preserved recovery").expect("old preserved recovery"); + let preserved_newer = PathBuf::from(format!( + "{}.v99-preserved-0123456789abcdef", + target_options.session_file_path().display() + )); + std::fs::write(&preserved_newer, b"old newer-version session") + .expect("old newer-version session"); std::fs::write(target_options.clear_marker_file_path(), b"old clear").expect("old clear"); std::fs::write(target_options.lock_file_path(), b"lock").expect("old lock"); @@ -490,6 +496,7 @@ fn runtime_save_as_confirmed_overwrite_removes_stale_sidecars_but_keeps_lock() { assert!(!target_options.backup_file_path().exists()); assert!(!target_options.recovery_file_path().exists()); assert!(!preserved_recovery.exists()); + assert!(!preserved_newer.exists()); assert!(!target_options.clear_marker_file_path().exists()); assert!( target_options.lock_file_path().exists(), diff --git a/src/session/snapshot/save/save_as.rs b/src/session/snapshot/save/save_as.rs index 4f02f0e4..8e164e17 100644 --- a/src/session/snapshot/save/save_as.rs +++ b/src/session/snapshot/save/save_as.rs @@ -1,6 +1,10 @@ use super::*; use std::os::unix::fs::OpenOptionsExt; +mod transaction; + +use transaction::commit_save_as_target; + #[allow(dead_code)] pub(crate) fn save_snapshot_as_with_report( snapshot: &SessionSnapshot, @@ -119,28 +123,10 @@ pub(crate) fn save_snapshot_as_with_report( crate::session::validate_named_session_file_for_foreground(&session_path)?; let lock_time_artifacts = collect_save_as_artifacts(options)?; ensure_save_as_overwrite_allowed(&lock_time_artifacts, overwrite, &session_path)?; - let removed_sidecars = matches!(overwrite, SaveAsOverwrite::ConfirmReplace) - && !lock_time_artifacts.sidecars.is_empty(); - if matches!(overwrite, SaveAsOverwrite::ConfirmReplace) { - remove_save_as_sidecars(&lock_time_artifacts.sidecars)?; - } - if let Err(err) = fs::rename(&tmp_path, &session_path) { - let context = if removed_sidecars { - format!( - "partial destructive Save Session As failure for {}: stale sidecars were removed but failed to move temporary file {} into place", - session_path.display(), - tmp_path.display() - ) - } else { - format!( - "failed to move temporary Save Session As file {} -> {}", - tmp_path.display(), - session_path.display() - ) - }; - return Err(err).with_context(|| context); - } - sync_session_parent_dir(&session_path, "Save Session As file")?; + let sidecars = matches!(overwrite, SaveAsOverwrite::ConfirmReplace) + .then_some(lock_time_artifacts.sidecars.as_slice()) + .unwrap_or_default(); + commit_save_as_target(&tmp_path, &session_path, sidecars)?; Ok::<(), anyhow::Error>(()) })(); @@ -239,84 +225,8 @@ fn artifact_path_exists(path: &Path) -> Result { } fn save_as_non_lock_sidecar_paths(options: &SessionOptions) -> Result> { - let mut paths = vec![ - options.backup_file_path(), - options.backup_recovery_marker_file_path(), - options.recovery_file_path(), - options.recovery_recoverable_marker_file_path(), - options.clear_marker_file_path(), - ]; - - let recovery_path = options.recovery_file_path(); - let Some(recovery_name) = recovery_path.file_name().and_then(|name| name.to_str()) else { - dedupe_paths(&mut paths); - return Ok(paths); - }; - let Some(parent) = recovery_path.parent() else { - dedupe_paths(&mut paths); - return Ok(paths); - }; - match fs::read_dir(parent) { - Ok(entries) => { - let preserved_prefix = format!("{recovery_name}."); - for entry in entries { - let entry = entry.with_context(|| { - format!( - "failed to inspect Save Session As sidecars under {}", - parent.display() - ) - })?; - let path = entry.path(); - let Some(name) = path.file_name().and_then(|name| name.to_str()) else { - continue; - }; - if name == recovery_name || name.starts_with(&preserved_prefix) { - paths.push(path); - } - } - } - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} - Err(err) => { - return Err(err).with_context(|| { - format!( - "failed to scan Save Session As sidecars under {}", - parent.display() - ) - }); - } - } - - dedupe_paths(&mut paths); + let session_path = options.session_file_path(); + let mut paths = crate::session::named_session_non_lock_artifact_paths(&session_path)?; + paths.retain(|path| path != &session_path); Ok(paths) } - -fn dedupe_paths(paths: &mut Vec) { - let mut deduped = Vec::with_capacity(paths.len()); - for path in paths.drain(..) { - if !deduped.contains(&path) { - deduped.push(path); - } - } - *paths = deduped; -} - -fn remove_save_as_sidecars(sidecars: &[PathBuf]) -> Result<()> { - for path in sidecars { - match fs::remove_file(path) { - Ok(()) => info!( - "Removed stale Save Session As sidecar before commit: {}", - path.display() - ), - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} - Err(err) => { - return Err(err).with_context(|| { - format!( - "failed to remove stale Save Session As sidecar {}", - path.display() - ) - }); - } - } - } - Ok(()) -} diff --git a/src/session/snapshot/save/save_as/transaction.rs b/src/session/snapshot/save/save_as/transaction.rs new file mode 100644 index 00000000..3efaf70e --- /dev/null +++ b/src/session/snapshot/save/save_as/transaction.rs @@ -0,0 +1,304 @@ +use super::super::sync_session_parent_dir; +use anyhow::{Context, Result, anyhow}; +use log::{info, warn}; +use std::ffi::OsString; +use std::fs; +use std::io::ErrorKind; +use std::os::unix::fs::DirBuilderExt; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +const SAVE_AS_STAGING_PREFIX: &str = ".wayscriber-save-as-sidecars"; +static NEXT_SAVE_AS_STAGING_ID: AtomicU64 = AtomicU64::new(0); + +#[derive(Debug)] +struct QuarantinedSidecar { + original: PathBuf, + staged: PathBuf, +} + +#[derive(Debug)] +struct SaveAsSidecarQuarantine { + directory: Option, + sidecars: Vec, +} + +impl SaveAsSidecarQuarantine { + fn empty() -> Self { + Self { + directory: None, + sidecars: Vec::new(), + } + } + + fn directory(&self) -> Option<&Path> { + self.directory.as_deref() + } + + fn rollback(self) -> Result<()> { + let mut failures = Vec::new(); + for sidecar in self.sidecars.iter().rev() { + if let Err(err) = crate::session::artifacts::rename_artifact_no_replace( + &sidecar.staged, + &sidecar.original, + ) { + failures.push(format!( + "{} -> {}: {err}", + sidecar.staged.display(), + sidecar.original.display() + )); + } + } + + if failures.is_empty() + && let Some(directory) = &self.directory + { + fs::remove_dir(directory).with_context(|| { + format!( + "failed to remove rolled-back Save Session As quarantine {}", + directory.display() + ) + })?; + crate::durable_io::sync_parent_dir(directory).with_context(|| { + format!( + "failed to sync restored Save Session As sidecars under {}", + directory.display() + ) + })?; + } + + if failures.is_empty() { + Ok(()) + } else { + Err(anyhow!( + "failed to restore quarantined Save Session As sidecars: {}", + failures.join("; ") + )) + } + } + + fn remove_after_commit(self) -> Result<()> { + let mut failures = Vec::new(); + for sidecar in &self.sidecars { + match fs::remove_file(&sidecar.staged) { + Ok(()) => info!( + "Removed stale Save Session As sidecar after commit: {}", + sidecar.original.display() + ), + Err(err) if err.kind() == ErrorKind::NotFound => {} + Err(err) => failures.push(format!("{}: {err}", sidecar.staged.display())), + } + } + + if failures.is_empty() + && let Some(directory) = &self.directory + { + fs::remove_dir(directory).with_context(|| { + format!( + "failed to remove Save Session As sidecar quarantine {}", + directory.display() + ) + })?; + crate::durable_io::sync_parent_dir(directory).with_context(|| { + format!( + "failed to sync Save Session As sidecar cleanup under {}", + directory.display() + ) + })?; + } + + if failures.is_empty() { + Ok(()) + } else { + Err(anyhow!( + "failed to remove quarantined Save Session As sidecars: {}", + failures.join("; ") + )) + } + } +} + +pub(super) fn commit_save_as_target( + tmp_path: &Path, + session_path: &Path, + sidecars: &[PathBuf], +) -> Result<()> { + commit_save_as_target_with( + tmp_path, + session_path, + sidecars, + |source, target| fs::rename(source, target), + || sync_session_parent_dir(session_path, "Save Session As file"), + SaveAsSidecarQuarantine::remove_after_commit, + ) +} + +fn commit_save_as_target_with( + tmp_path: &Path, + session_path: &Path, + sidecars: &[PathBuf], + replace_primary: Replace, + sync_primary: Sync, + cleanup_sidecars: Cleanup, +) -> Result<()> +where + Replace: FnOnce(&Path, &Path) -> std::io::Result<()>, + Sync: FnOnce() -> Result<()>, + Cleanup: FnOnce(SaveAsSidecarQuarantine) -> Result<()>, +{ + let quarantine = quarantine_save_as_sidecars(sidecars, session_path)?; + let had_quarantined_sidecars = !quarantine.sidecars.is_empty(); + if let Err(err) = replace_primary(tmp_path, session_path) { + return match quarantine.rollback() { + Ok(()) => Err(err).with_context(|| { + if had_quarantined_sidecars { + format!( + "failed to move temporary Save Session As file {} -> {}; restored stale sidecars", + tmp_path.display(), + session_path.display() + ) + } else { + format!( + "failed to move temporary Save Session As file {} -> {}", + tmp_path.display(), + session_path.display() + ) + } + }), + Err(rollback_err) => Err(err).with_context(|| { + format!( + "partial Save Session As failure for {}: primary replacement failed and sidecar rollback also failed: {rollback_err:#}", + session_path.display() + ) + }), + }; + } + + let sync_result = sync_primary(); + let quarantine_path = quarantine.directory().map(Path::to_path_buf); + if let Err(err) = cleanup_sidecars(quarantine) { + warn!( + "Save Session As primary committed to {}, but stale sidecar quarantine cleanup failed{}: {err:#}", + session_path.display(), + quarantine_path + .as_deref() + .map(|path| format!(" for {}", path.display())) + .unwrap_or_default() + ); + } + sync_result +} + +fn quarantine_save_as_sidecars( + sidecars: &[PathBuf], + session_path: &Path, +) -> Result { + let Some(parent) = session_path.parent() else { + return Err(anyhow!( + "Save Session As target has no parent directory: {}", + session_path.display() + )); + }; + + let mut present = Vec::new(); + for path in sidecars { + if path.parent() != Some(parent) { + return Err(anyhow!( + "Save Session As sidecar is outside the target directory: {}", + path.display() + )); + } + let Some(file_name) = path.file_name() else { + return Err(anyhow!( + "Save Session As sidecar has no file name: {}", + path.display() + )); + }; + match fs::symlink_metadata(path) { + Ok(metadata) if metadata.file_type().is_dir() => { + return Err(anyhow!( + "refusing to replace Save Session As sidecar directory {}", + path.display() + )); + } + Ok(_) => present.push((path.clone(), file_name.to_os_string())), + Err(err) if err.kind() == ErrorKind::NotFound => {} + Err(err) => { + return Err(err).with_context(|| { + format!( + "failed to inspect Save Session As sidecar {}", + path.display() + ) + }); + } + } + } + if present.is_empty() { + return Ok(SaveAsSidecarQuarantine::empty()); + } + + let directory = create_save_as_staging_dir(parent)?; + let mut quarantine = SaveAsSidecarQuarantine { + directory: Some(directory.clone()), + sidecars: Vec::with_capacity(present.len()), + }; + for (index, (original, file_name)) in present.into_iter().enumerate() { + let mut staged_name = OsString::from(format!("{index}-")); + staged_name.push(file_name); + let staged = directory.join(staged_name); + match crate::session::artifacts::rename_artifact_no_replace(&original, &staged) { + Ok(()) => quarantine + .sidecars + .push(QuarantinedSidecar { original, staged }), + Err(err) if err.kind() == ErrorKind::NotFound => {} + Err(err) => { + return match quarantine.rollback() { + Ok(()) => Err(err).with_context(|| { + format!( + "failed to quarantine Save Session As sidecar {}; restored earlier sidecars", + original.display() + ) + }), + Err(rollback_err) => Err(err).with_context(|| { + format!( + "failed to quarantine Save Session As sidecar {}, and rollback also failed: {rollback_err:#}", + original.display() + ) + }), + }; + } + } + } + Ok(quarantine) +} + +fn create_save_as_staging_dir(parent: &Path) -> Result { + for _ in 0..1024 { + let id = NEXT_SAVE_AS_STAGING_ID.fetch_add(1, Ordering::Relaxed); + let candidate = parent.join(format!( + "{SAVE_AS_STAGING_PREFIX}-{}-{id}", + std::process::id() + )); + let mut builder = fs::DirBuilder::new(); + builder.mode(0o700); + match builder.create(&candidate) { + Ok(()) => return Ok(candidate), + Err(err) if err.kind() == ErrorKind::AlreadyExists => {} + Err(err) => { + return Err(err).with_context(|| { + format!( + "failed to create Save Session As sidecar quarantine {}", + candidate.display() + ) + }); + } + } + } + Err(anyhow!( + "failed to allocate a unique Save Session As sidecar quarantine under {}", + parent.display() + )) +} + +#[cfg(test)] +mod tests; diff --git a/src/session/snapshot/save/save_as/transaction/tests.rs b/src/session/snapshot/save/save_as/transaction/tests.rs new file mode 100644 index 00000000..f15e784f --- /dev/null +++ b/src/session/snapshot/save/save_as/transaction/tests.rs @@ -0,0 +1,152 @@ +use super::*; +use std::cell::RefCell; +use std::os::unix::fs::PermissionsExt; + +fn staging_directories(parent: &Path) -> Vec { + fs::read_dir(parent) + .expect("read test directory") + .filter_map(|entry| entry.ok().map(|entry| entry.path())) + .filter(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with(SAVE_AS_STAGING_PREFIX)) + }) + .collect() +} + +#[test] +fn quarantine_validation_failure_preserves_every_sidecar() { + let temp = crate::test_temp::tempdir().expect("tempdir"); + let session = temp.path().join("target.wayscriber-session"); + let backup = temp.path().join("target.wayscriber-session.bak"); + let recovery = temp.path().join("target.wayscriber-session.recovery"); + fs::write(&backup, b"backup").expect("write backup"); + fs::create_dir(&recovery).expect("create invalid recovery directory"); + + let err = quarantine_save_as_sidecars(&[backup.clone(), recovery.clone()], &session) + .expect_err("sidecar directory must fail validation"); + + assert!(format!("{err:#}").contains("refusing to replace")); + assert_eq!(fs::read(&backup).expect("backup preserved"), b"backup"); + assert!(recovery.is_dir()); + assert!(staging_directories(temp.path()).is_empty()); +} + +#[test] +fn primary_rename_failure_restores_quarantined_sidecars() { + let temp = crate::test_temp::tempdir().expect("tempdir"); + let session = temp.path().join("target.wayscriber-session"); + let tmp = temp.path().join("target.tmp"); + let backup = temp.path().join("target.wayscriber-session.bak"); + let recovery = temp.path().join("target.wayscriber-session.recovery"); + fs::write(&session, b"old primary").expect("write primary"); + fs::write(&tmp, b"new primary").expect("write temporary primary"); + fs::write(&backup, b"backup").expect("write backup"); + fs::write(&recovery, b"recovery").expect("write recovery"); + + let err = commit_save_as_target_with( + &tmp, + &session, + &[backup.clone(), recovery.clone()], + |_, _| Err(std::io::Error::other("injected primary rename failure")), + || -> Result<()> { panic!("sync must not run before primary replacement") }, + |_| -> Result<()> { panic!("cleanup must not run before primary replacement") }, + ) + .expect_err("injected rename failure"); + + assert!(format!("{err:#}").contains("restored stale sidecars")); + assert_eq!( + fs::read(&session).expect("old primary preserved"), + b"old primary" + ); + assert_eq!( + fs::read(&tmp).expect("temporary primary preserved"), + b"new primary" + ); + assert_eq!(fs::read(&backup).expect("backup restored"), b"backup"); + assert_eq!(fs::read(&recovery).expect("recovery restored"), b"recovery"); + assert!(staging_directories(temp.path()).is_empty()); +} + +#[test] +fn successful_commit_removes_quarantined_sidecars() { + let temp = crate::test_temp::tempdir().expect("tempdir"); + let session = temp.path().join("target.wayscriber-session"); + let tmp = temp.path().join("target.tmp"); + let backup = temp.path().join("target.wayscriber-session.bak"); + let recovery = temp.path().join("target.wayscriber-session.recovery"); + fs::write(&session, b"old primary").expect("write primary"); + fs::write(&tmp, b"new primary").expect("write temporary primary"); + fs::write(&backup, b"backup").expect("write backup"); + fs::write(&recovery, b"recovery").expect("write recovery"); + + commit_save_as_target(&tmp, &session, &[backup.clone(), recovery.clone()]) + .expect("commit Save As target"); + + assert_eq!( + fs::read(&session).expect("new primary committed"), + b"new primary" + ); + assert!(!tmp.exists()); + assert!(!backup.exists()); + assert!(!recovery.exists()); + assert!(staging_directories(temp.path()).is_empty()); +} + +#[test] +fn postcommit_cleanup_failure_keeps_recovery_bytes_quarantined() { + let temp = crate::test_temp::tempdir().expect("tempdir"); + let session = temp.path().join("target.wayscriber-session"); + let tmp = temp.path().join("target.tmp"); + let recovery = temp.path().join("target.wayscriber-session.recovery"); + fs::write(&session, b"old primary").expect("write primary"); + fs::write(&tmp, b"new primary").expect("write temporary primary"); + fs::write(&recovery, b"recovery").expect("write recovery"); + let staged_directory = RefCell::new(None); + + commit_save_as_target_with( + &tmp, + &session, + std::slice::from_ref(&recovery), + |source, target| fs::rename(source, target), + || Ok(()), + |quarantine| { + *staged_directory.borrow_mut() = quarantine.directory().map(Path::to_path_buf); + Err(anyhow!("injected postcommit cleanup failure")) + }, + ) + .expect("cleanup failure must not roll back a committed primary"); + + assert_eq!( + fs::read(&session).expect("new primary committed"), + b"new primary" + ); + assert!(!recovery.exists()); + let staged_directory = staged_directory + .into_inner() + .expect("quarantine directory recorded"); + assert_eq!( + fs::metadata(&staged_directory) + .expect("quarantine metadata") + .permissions() + .mode() + & 0o777, + 0o700 + ); + let staged_files = fs::read_dir(&staged_directory) + .expect("quarantine retained") + .map(|entry| entry.expect("staged entry").path()) + .collect::>(); + assert_eq!(staged_files.len(), 1); + assert!( + staged_files[0] + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.contains("target.wayscriber-session.recovery")), + "quarantined files should retain their original name for manual recovery" + ); + assert_eq!( + fs::read(&staged_files[0]).expect("recovery retained"), + b"recovery" + ); +}