diff --git a/src-tauri/src/app.rs b/src-tauri/src/app.rs index aa1229a8..eff358e9 100644 --- a/src-tauri/src/app.rs +++ b/src-tauri/src/app.rs @@ -235,6 +235,7 @@ pub fn run() { commands::canonicalize_path, commands::read_file_as_data_url, commands::save_file_content, + commands::sweep_temp_files, commands::export_pdf_windows, commands::print_pdf, commands::is_win11, diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index fe0c74a6..6f502c0c 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -5,7 +5,7 @@ use crate::fs_safety::{ atomic_write, canonical_identity, encode_text, ensure_path_within_root, read_to_string_lossy, - resolve_image_directory, safe_path_component, + resolve_image_directory, safe_path_component, sweep_document_temps, }; use crate::markdown::{build_markdown_preview, convert_markdown, heading_anchors, HeadingAnchor}; use crate::window_runtime::{self, WatcherState}; @@ -226,6 +226,30 @@ pub async fn save_file_content( .unwrap_or_else(|e| Err(e.to_string())) } +/// Clear temp files earlier runs left beside the documents this window knows +/// about — the recent-file list, and whatever the session restored. +/// +/// A save already sweeps the document it is writing, and that is the sweep +/// that matters while someone is working. It cannot reach a document nobody +/// opens again, whose leftovers would otherwise stay in the user's folder for +/// good, so startup asks for the documents Markpad remembers (#722). +/// +/// Every window calls this and only the first pays for it: the sweep is once +/// per document per run, and Markpad's windows share a process. +/// +/// Async and off the main thread: this is a `read_dir` per document on folders +/// that may live on a network volume, and nothing waits for the result. +#[tauri::command] +pub async fn sweep_temp_files(paths: Vec) -> Result<(), String> { + tauri::async_runtime::spawn_blocking(move || { + for path in paths { + sweep_document_temps(Path::new(&path)); + } + }) + .await + .map_err(|e| e.to_string()) +} + /// Resolve `path` to the identity the filesystem gives it — see /// `canonical_identity` for what that folds and why the filesystem, rather /// than a platform guess, is the thing being asked. diff --git a/src-tauri/src/fs_safety.rs b/src-tauri/src/fs_safety.rs index 5b55e2ad..389ab522 100644 --- a/src-tauri/src/fs_safety.rs +++ b/src-tauri/src/fs_safety.rs @@ -3,17 +3,182 @@ //! //! Split out of `lib.rs`; the code and its tests are unchanged. +use std::collections::HashSet; 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::sync::{LazyLock, Mutex}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; + +/// DIAGNOSTIC BUILD for #722 — never merge. Appends one line per event to +/// `%TEMP%\markpad-diag.log` (`std::env::temp_dir()`), so the reporter can +/// show what `remove_file` returns on their machine. +pub(crate) fn diag(msg: &str) { + let ms = SystemTime::now() + .duration_since(UNIX_EPOCH) + .map(|d| d.as_millis()) + .unwrap_or(0); + if let Ok(mut f) = fs::OpenOptions::new() + .append(true) + .create(true) + .open(std::env::temp_dir().join("markpad-diag.log")) + { + let _ = writeln!(f, "{ms} pid={} {msg}", std::process::id()); + } +} + +fn diag_remove(what: &str, path: &Path) { + let exists = path.exists(); + let len = fs::metadata(path).map(|m| m.len()).ok(); + if !exists { + diag(&format!("{what}: {} exists=false", path.display())); + return; + } + match fs::remove_file(path) { + Ok(()) => diag(&format!( + "{what}: {} exists=true len={len:?} remove=Ok still_exists={}", + path.display(), + path.exists() + )), + Err(e) => diag(&format!( + "{what}: {} exists=true len={len:?} remove=Err({e}) os_error={:?} still_exists={}", + path.display(), + e.raw_os_error(), + path.exists() + )), + } +} /// Distinguishes the temp files of `atomic_write` calls that share a process. /// Never reset, and read with `fetch_add` so no two callers can be handed the /// same value — the property the wall clock could not supply. static TEMP_FILE_SEQ: AtomicU64 = AtomicU64::new(0); +/// Temp-name prefixes already swept this run — see `sweep_abandoned_temps`. +static SWEPT_TEMP_PREFIXES: LazyLock>> = + LazyLock::new(|| Mutex::new(HashSet::new())); + +/// Below this, a temp file could belong to a write that is still running — +/// this process's own or another Markpad's — and nothing may touch it. +const IN_FLIGHT_GRACE: Duration = Duration::from_secs(60); + +/// How old a temp file that still has contents has to be before it counts as +/// abandoned. Long, because such a file can be the only copy of a document a +/// crash caught between the `fsync` and the rename. +const ABANDONED_TEMP_AGE: Duration = Duration::from_secs(60 * 60); + +/// Remove the temp file this call created, and if that fails, say so in the +/// error being returned. +/// +/// The two are one thing on purpose. A write or rename that fails leaves the +/// temp file behind, and the reason the CLEANUP failed is the interesting half +/// — the failure that reaches the user is "could not save", while the file +/// they then find in their folder is explained by an error nobody kept +/// (#722). `let _ =` discarded exactly that. +fn remove_temp_or_say_why(error: std::io::Error, temp_path: &Path) -> std::io::Error { + match fs::remove_file(temp_path) { + Ok(()) => error, + // Already gone is the outcome this wanted. + Err(cleanup) if cleanup.kind() == std::io::ErrorKind::NotFound => error, + Err(cleanup) => std::io::Error::new( + error.kind(), + format!( + "{error} — and {} could not be cleaned up either: {cleanup}", + temp_path.display() + ), + ), + } +} + +/// Clear temp files earlier runs left beside `document`, once per document +/// per run. +/// +/// Two callers, because one of them cannot reach everything. A save sweeps the +/// document it is writing, which clears a folder the next time the user edits +/// what is in it — and never clears the folder of a document nobody edits +/// again. Startup sweeps the documents Markpad already knows about (the recent +/// list, and whatever the session restored), which is where a leftover would +/// otherwise sit for good. +/// +/// Once per document per run because the scan only ever finds garbage from +/// earlier runs: repeating it finds nothing new, and it would put a `read_dir` +/// in front of every 1.5s auto-save, which on a network folder is the kind of +/// cost that shows up as a stutter while typing. +pub(crate) fn sweep_document_temps(document: &Path) { + let Some(file_name) = document + .file_name() + .map(|name| name.to_string_lossy().into_owned()) + else { + return; + }; + let parent = match document.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent.to_path_buf(), + _ => PathBuf::from("."), + }; + let first_time = SWEPT_TEMP_PREFIXES + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .insert(parent.join(&file_name)); + diag(&format!("sweep_document_temps: {} first_time={first_time}", document.display())); + if first_time { + sweep_abandoned_temps(&parent, &file_name); + } +} + +/// Delete temp files for `file_name` that an earlier run left in `parent`. +/// +/// Cleanup at the time of the failure is the first line and stays the main +/// one, but it cannot run at all when the process dies mid-write, and it can +/// be refused by whatever refused the write. Both leave a file the user has +/// to find and delete by hand, which is what #722 is: a portable Windows +/// build accumulating `.doc.md.markpad-tmp-…` siblings across sessions. +/// +/// `IN_FLIGHT_GRACE` is what keeps this off a write that is still happening — +/// a temp file lives for one `write_all` and one `fsync`, so a minute is four +/// orders of magnitude past any of them, on any volume. Past that, two kinds +/// of leftover with different stakes: +/// +/// - **Empty**: there is nothing in it to lose, so it goes. This is the #722 +/// shape, and waiting an hour to clear a file that can never be anything but +/// garbage only means the user finds it first. +/// - **Not empty**: it may be the only copy of a document, from a process that +/// died between the `fsync` and the rename. `ABANDONED_TEMP_AGE` gives that +/// a wide berth before Markpad decides it is garbage. +fn sweep_abandoned_temps(parent: &Path, file_name: &str) { + let prefix = format!(".{file_name}.markpad-tmp-"); + let Ok(entries) = fs::read_dir(parent) else { + return; + }; + for entry in entries.flatten() { + let name = entry.file_name(); + let Some(name) = name.to_str() else { continue }; + if !name.starts_with(&prefix) { + continue; + } + let Ok(meta) = entry.metadata() else { continue }; + let Some(age) = meta + .modified() + .ok() + .and_then(|modified| SystemTime::now().duration_since(modified).ok()) + else { + continue; + }; + diag(&format!( + "sweep candidate: {} len={} age_s={}", + entry.path().display(), + meta.len(), + age.as_secs() + )); + if age < IN_FLIGHT_GRACE { + continue; + } + if meta.len() == 0 || age >= ABANDONED_TEMP_AGE { + diag_remove("sweep", &entry.path()); + } + } +} + /// Write `bytes` to `target` durably and atomically: write to a sibling temp /// file, fsync it, then rename over the target. Atomic on both Unix and /// modern Windows — `std::fs::rename` calls `MoveFileExW` with @@ -81,6 +246,8 @@ pub(crate) fn atomic_write(target: &Path, bytes: &[u8]) -> std::io::Result<()> { .map(|n| n.to_string_lossy().into_owned()) .unwrap_or_else(|| "markpad".to_string()); + sweep_document_temps(target); + // Claim a temp file nobody else holds. The name must be unique or two // concurrent writers of the same target collide, and a collision is not a // harmless retry: the loser used to delete `temp_path` on its way out, @@ -126,9 +293,18 @@ pub(crate) fn atomic_write(target: &Path, bytes: &[u8]) -> std::io::Result<()> { Ok(()) })(); + // Closed before the rename and before any cleanup, rather than at the end + // of the function. Both of those operations are ones an open handle takes + // part in on Windows: `MoveFileExW` and `DeleteFileW` fail with a sharing + // violation when a handle without `FILE_SHARE_DELETE` is open on the file, + // and our own handle joins whatever an antivirus scanner or indexer is + // already holding on a file created one instruction ago. A `DeleteFileW` + // that does get through with a handle still open only marks the file + // delete-pending, so it stays visible in the directory meanwhile. + drop(file); + if let Err(e) = write_result { - let _ = fs::remove_file(&temp_path); - return Err(e); + return Err(remove_temp_or_say_why(e, &temp_path)); } // Atomic on both Unix and modern Windows: std::fs::rename uses @@ -138,9 +314,43 @@ pub(crate) fn atomic_write(target: &Path, bytes: &[u8]) -> std::io::Result<()> { // rename fails (e.g. target locked by another process on Windows), // we clean up the temp file and surface the original error without // touching the target. - if let Err(e) = fs::rename(&temp_path, target) { + let rename_result = fs::rename(&temp_path, target); + diag(&format!( + "rename: {} -> {} result={:?} os_error={:?}", + temp_path.display(), + target.display(), + rename_result.as_ref().map(|_| ()).map_err(|e| e.to_string()), + rename_result.as_ref().err().and_then(|e| e.raw_os_error()) + )); + if let Err(e) = rename_result { + return Err(remove_temp_or_say_why(e, &temp_path)); + } + diag_remove("post-rename", &temp_path); + { + let later = temp_path.clone(); + std::thread::spawn(move || { + for ms in [200u64, 2_000, 10_000] { + std::thread::sleep(Duration::from_millis(ms)); + diag_remove(&format!("recheck+{ms}ms"), &later); + } + }); + } + + // A rename that reported success has left nothing at the old name, and on + // every machine this has been run on that is what happens. #722 is a + // machine where it does not: an endpoint agent (亚信安全 TrustOne) sits in + // the filesystem, and the reporter gets one 0-byte `.doc.md.markpad-tmp-…` + // per save while the save itself succeeds — no error, the tab's modified + // dot clears, the document holds the new text. Whatever the agent is doing + // with the source name, this call is the moment we still know it, and one + // `remove_file` on a path that is normally already gone is a cheaper way + // to find out than any amount of reasoning about filter drivers. + // + // Best-effort, and deliberately not folded into the error above: the write + // has landed, the document is correct, and a save must not start failing + // over a file that should not be there in the first place. + if temp_path.exists() { let _ = fs::remove_file(&temp_path); - return Err(e); } // Best-effort restore of the original mode bits. If this fails (e.g. the @@ -759,6 +969,123 @@ pub(crate) mod tests { fs::remove_dir_all(dir).unwrap(); } + #[test] + fn a_temp_file_an_earlier_run_abandoned_is_swept_by_the_next_write() { + // #722: temp files accumulating beside a document across sessions. + // The next save of that document is the moment Markpad knows both the + // folder and the name, and the only moment it is guaranteed to be + // looking at them. + let dir = temp_path("atomic-sweep"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("notes.md"); + fs::write(&path, b"body").unwrap(); + + // Old, and with contents — the case that has to wait out + // `ABANDONED_TEMP_AGE`, because a temp file with a document in it can + // be the only copy of that document. + let stale = dir.join(".notes.md.markpad-tmp-4242-1787728728439865400-4"); + fs::write(&stale, b"a whole document").unwrap(); + set_modified_ago(&stale, Duration::from_secs(3 * 60 * 60)); + + // Empty, and only minutes old. Nothing in it can be lost, and this is + // the shape #722 produces one of per save. + let empty = dir.join(".notes.md.markpad-tmp-4242-1787728728439865400-5"); + fs::write(&empty, b"").unwrap(); + set_modified_ago(&empty, Duration::from_secs(5 * 60)); + + // Another document's leftovers, equally stale. The sweep is scoped to + // the file being written, so a folder full of documents is cleaned by + // the saves that touch each of them rather than by the first save to + // reach the folder. + let neighbour = dir.join(".other.md.markpad-tmp-4242-1787728728439865400-6"); + fs::write(&neighbour, b"").unwrap(); + set_modified_ago(&neighbour, Duration::from_secs(3 * 60 * 60)); + + atomic_write(&path, b"edited").unwrap(); + + assert!( + !stale.exists(), + "an abandoned temp file must not survive a save" + ); + assert!( + !empty.exists(), + "an empty temp file has nothing in it to wait for" + ); + assert!( + neighbour.exists(), + "the sweep must not reach past the document being written", + ); + assert_eq!(fs::read(&path).unwrap(), b"edited"); + + fs::remove_dir_all(dir).unwrap(); + } + + #[test] + fn a_document_nobody_edits_again_is_still_swept() { + // The reason `sweep_document_temps` is reachable without a write: the + // per-save sweep only ever clears the folder of a document someone is + // still editing. Startup calls this for the recent list and the + // restored tabs, which is the only thing that reaches leftovers beside + // a document that is never saved again. + let dir = temp_path("atomic-sweep-startup"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("archived.md"); + fs::write(&path, b"body").unwrap(); + + let leftover = dir.join(".archived.md.markpad-tmp-4242-1787728728439865400-4"); + fs::write(&leftover, b"").unwrap(); + set_modified_ago(&leftover, Duration::from_secs(5 * 60)); + + sweep_document_temps(&path); + + assert!(!leftover.exists()); + assert_eq!( + fs::read(&path).unwrap(), + b"body", + "sweeping must not touch the document itself", + ); + + fs::remove_dir_all(dir).unwrap(); + } + + #[test] + fn a_temp_file_that_could_still_be_in_flight_is_left_alone() { + // The dangerous half of a sweep: another Markpad, or another thread + // here, is between `create_new` and `rename` right now. Deleting its + // temp file fails its rename with ENOENT — the exact breakage + // `concurrent_atomic_writes_to_one_target_all_succeed` exists for. A + // write in that window is milliseconds old, and it is EMPTY for the + // stretch between `create_new` and `write_all`, so age has to be the + // first question the sweep asks and the empty-file rule the second. + let dir = temp_path("atomic-sweep-live"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("notes.md"); + fs::write(&path, b"body").unwrap(); + + let just_created = dir.join(".notes.md.markpad-tmp-4242-1787728728439865400-4"); + fs::write(&just_created, b"").unwrap(); + let half_written = dir.join(".notes.md.markpad-tmp-4242-1787728728439865400-5"); + fs::write(&half_written, b"half a document").unwrap(); + + atomic_write(&path, b"edited").unwrap(); + + assert!( + just_created.exists(), + "a temp file young enough to be a live write must survive, empty or not", + ); + assert!(half_written.exists()); + + fs::remove_dir_all(dir).unwrap(); + } + + /// Backdate `path`'s mtime, which is what the sweep reads to tell an + /// abandoned temp file from a live one. + fn set_modified_ago(path: &Path, ago: Duration) { + let file = fs::OpenOptions::new().write(true).open(path).unwrap(); + file.set_times(fs::FileTimes::new().set_modified(SystemTime::now() - ago)) + .unwrap(); + } + #[test] fn concurrent_atomic_writes_to_one_target_all_succeed() { // Two threads used to derive the same temp name from the same clock diff --git a/src/lib/MarkdownViewer.svelte b/src/lib/MarkdownViewer.svelte index 6656cbce..e3b9a7cc 100644 --- a/src/lib/MarkdownViewer.svelte +++ b/src/lib/MarkdownViewer.svelte @@ -3244,6 +3244,24 @@ import { createDocumentSession, type LoadMarkdownOptions } from './sessions/docu await windowSession.claimTransferredTab(); if (isDisposed) return; + // Clear temp files an earlier run left beside the documents this + // window knows about. A save sweeps the document it writes, which + // covers everything the user is working on; this covers the one a + // save cannot reach — a document nobody edits again, whose + // leftovers would otherwise stay in the folder for good (#722). + // + // Not awaited, and its failure is not the user's problem: nothing + // on screen depends on it, and a folder that cannot be read is a + // folder with nothing to clean. + void invoke('sweep_temp_files', { + paths: [ + ...new Set([ + ...recentFiles, + ...tabManager.tabs.map((tab) => tab.path).filter(hasRealFilePath), + ]), + ], + }).catch((error) => console.error('Failed to sweep temp files', error)); + const urlParams = new URLSearchParams(window.location.search); const fileParam = urlParams.get('file');