diff --git a/src/app/automation.rs b/src/app/automation.rs index f3d6c3d08..cce204817 100644 --- a/src/app/automation.rs +++ b/src/app/automation.rs @@ -212,7 +212,7 @@ impl OpenCADStudio { let Some(path) = req["path"].as_str() else { return err("open: missing \"path\""); }; - let bytes = match std::fs::read(path) { + let bytes = match self.read_drawing(std::path::Path::new(path)) { Ok(b) => b, Err(e) => return err(format!("open: {e}")), }; @@ -981,6 +981,40 @@ mod tests { let _ = std::fs::remove_file(&b); } + /// Saving to a path that already holds a drawing leases the destination and then + /// fingerprints it, and on Windows the lease's lock is mandatory: re-opening the + /// path to read it was refused against our own lock, so every Save As over an + /// existing drawing failed with os error 33. Saving to a new path was unaffected, + /// which is the pair this test keeps. + #[test] + fn saving_over_an_existing_drawing_succeeds() { + for (label, pre_existing) in [("new path", false), ("existing drawing", true)] { + let path = std::env::temp_dir().join(format!( + "ocs_save_over_{}_{}.dxf", + std::process::id(), + pre_existing, + )); + let _ = std::fs::remove_file(&path); + if pre_existing { + std::fs::write(&path, b"a previous drawing").unwrap(); + } + + let mut app = OpenCADStudio::new_for_test(); + app.automation_op(r#"{"op":"new"}"#); + let p = path.to_string_lossy().replace('\\', "\\\\"); + let saved = app.automation_op(&format!(r#"{{"op":"save","path":"{p}"}}"#)); + assert_eq!(saved["ok"], true, "{label}: {}", saved["error"]); + + drop(app); + let sidecar = path.with_file_name(format!( + ".{}.ocs.lock", + path.file_name().unwrap().to_string_lossy() + )); + let _ = std::fs::remove_file(sidecar); + let _ = std::fs::remove_file(&path); + } + } + #[test] fn save_then_open_round_trips() { let mut app = OpenCADStudio::new_for_test(); @@ -998,3 +1032,4 @@ mod tests { let _ = std::fs::remove_file(&path); } } + diff --git a/src/app/update/file.rs b/src/app/update/file.rs index 7776ee7a8..6fbeb08bb 100644 --- a/src/app/update/file.rs +++ b/src/app/update/file.rs @@ -711,6 +711,36 @@ pub(super) fn on_open_file(&mut self) -> Task { } } + /// Read `path`, going through this session's own edit lease when one covers it. + /// + /// A leased drawing is locked for writing, and on Windows `LockFileEx` is + /// mandatory and scoped to the handle that took it: a plain read of a path this + /// editor holds open is refused against our own lock with + /// `ERROR_LOCK_VIOLATION`. `flock` on Unix is advisory and the same read + /// succeeds, which is why only Windows ever saw it. A duplicate of the lease's + /// handle shares its lock ownership and reads normally. + #[cfg(not(target_arch = "wasm32"))] + pub(in crate::app) fn read_drawing( + &self, + path: &std::path::Path, + ) -> std::io::Result> { + use std::io::{Read, Seek, SeekFrom}; + + let leased = self + .tab_showing(path) + .and_then(|i| self.tabs[i].edit_lease.as_ref()) + .and_then(|lease| lease.reader()); + let Some(mut reader) = leased else { + return std::fs::read(path); + }; + // The duplicate shares the original's file pointer, which the lease leaves + // wherever its last fingerprint read finished. + reader.seek(SeekFrom::Start(0))?; + let mut bytes = Vec::new(); + reader.read_to_end(&mut bytes)?; + Ok(bytes) + } + /// Start the next drawing a second launch handed us, if any. /// /// Must be called from EVERY path that clears `opening` — completion, error @@ -851,7 +881,7 @@ pub(super) fn on_open_file(&mut self) -> Task { )); } - let destination_lease = if path_changed { + let mut destination_lease = if path_changed { match crate::io::edit_lock::EditLease::acquire(&path) { Ok(lease) => Some(lease), Err(crate::io::edit_lock::EditLeaseError::Locked(error)) => { @@ -869,7 +899,16 @@ pub(super) fn on_open_file(&mut self) -> Task { }; let expected_fingerprint = if path_changed { - match crate::io::edit_lock::FileFingerprint::capture(&path) { + // Read through the lease's own handle when it has one. Re-opening the + // path here would collide with the exclusive lock just taken on it, + // which on Windows is mandatory rather than advisory and fails the + // whole save with ERROR_LOCK_VIOLATION. + let captured = destination_lease + .as_mut() + .and_then(|lease| lease.fingerprint()) + .unwrap_or_else(|| crate::io::edit_lock::FileFingerprint::capture(&path)); + + match captured { Ok(fingerprint) => Some(fingerprint), Err(error) if error.kind() == std::io::ErrorKind::NotFound => None, Err(error) => { @@ -894,6 +933,7 @@ pub(super) fn on_open_file(&mut self) -> Task { version, self.backup_on_save, expected_fingerprint, + destination_lease.as_ref().and_then(|lease| lease.reader()), )?; if set_current_path { @@ -1532,16 +1572,25 @@ pub(super) fn on_open_file(&mut self) -> Task { let previous_autosave = (purpose != crate::app::SavePurpose::Autosave).then(|| self.autosave_target(i)); let backup = purpose != crate::app::SavePurpose::Autosave && self.backup_on_save; + // Read through the lease taken just above where there is one: its lock is + // mandatory on Windows, so opening the same path again would fail against + // it instead of reading the bytes we mean to compare. + let mut lease = self.pending_save_leases.get_mut(&tab_id); let expected_fingerprint = if check_external_change && purpose != crate::app::SavePurpose::Autosave { if set_current_path { - crate::io::edit_lock::FileFingerprint::capture(&path).ok() + lease + .as_deref_mut() + .and_then(|lease| lease.fingerprint()) + .unwrap_or_else(|| crate::io::edit_lock::FileFingerprint::capture(&path)) + .ok() } else { self.tabs[i].disk_fingerprint.clone() } } else { None }; + let verify_reader = lease.and_then(|lease| lease.reader()); let worker_path = path.clone(); Task::perform( @@ -1572,6 +1621,7 @@ pub(super) fn on_open_file(&mut self) -> Task { version, backup, expected_fingerprint, + verify_reader, ); (result, refreshed_preview) }) diff --git a/src/io/edit_lock.rs b/src/io/edit_lock.rs index 0065cbebe..cdd94029e 100644 --- a/src/io/edit_lock.rs +++ b/src/io/edit_lock.rs @@ -22,7 +22,18 @@ pub struct FileFingerprint { impl FileFingerprint { pub fn capture(path: &Path) -> std::io::Result { - let mut file = File::open(path)?; + Self::capture_from(&mut File::open(path)?) + } + + /// Fingerprint an already-open handle. + /// + /// A save that targets an existing drawing takes an exclusive lease on it + /// first. On Windows `LockFileEx` is mandatory, so re-opening that same path + /// to read it fails with `ERROR_LOCK_VIOLATION` against our own lock — the + /// lease has to hand its handle over rather than let the fingerprint open a + /// second one. On Unix the lock is advisory and a second open would have + /// worked, which is why this only ever failed on Windows. + pub fn capture_from(file: &mut File) -> std::io::Result { let metadata = file.metadata()?; let len = metadata.len(); let modified_ns = metadata @@ -153,6 +164,27 @@ impl EditLease { self.platform_warning.as_deref() } + /// Fingerprint the leased drawing through the handle this lease already + /// holds, so the exclusive Windows lock does not block the read. + /// + /// `None` when the lease has no drawing handle — the destination did not + /// exist, or the platform lock was unavailable — which leaves the caller to + /// fall back to [`FileFingerprint::capture`]. + pub fn fingerprint(&mut self) -> Option> { + self.drawing.as_mut().map(FileFingerprint::capture_from) + } + + /// A separate handle onto the leased drawing, for a reader that outlives the + /// borrow above — the save re-checks the fingerprint just before it replaces + /// the file, on a worker that does not hold the lease. + /// + /// A duplicate shares the original's lock ownership, so it reads where a + /// fresh open of the same path would not. `None` on the same terms as + /// [`Self::fingerprint`]. + pub fn reader(&self) -> Option { + self.drawing.as_ref().and_then(|file| file.try_clone().ok()) + } + /// Atomic save replaces the path with a new file object. Move the platform /// lock to that new object; the sidecar remains locked throughout. pub fn refresh_drawing_lock(&mut self, path: &Path) -> Result<(), EditLeaseError> { @@ -255,6 +287,47 @@ mod tests { let _ = std::fs::remove_file(path); } + /// A save that targets an existing drawing leases it, then fingerprints it to + /// check nothing changed underneath. On Windows the drawing lock is mandatory and + /// scoped to the handle that took it, so opening the path a second time to read it + /// is refused against our own lock — the lease has to answer from the handle it + /// already holds. `flock` on Unix is advisory and the second open succeeds, which + /// is why the equality below held there and the save path failed only on Windows. + #[test] + fn a_lease_fingerprints_the_drawing_it_has_locked() { + let path = unique_path("leased.dwg"); + std::fs::write(&path, b"drawing bytes").unwrap(); + let unlocked = FileFingerprint::capture(&path).unwrap(); + + let mut lease = EditLease::acquire(&path).unwrap(); + let leased = lease + .fingerprint() + .expect("the lease holds a drawing handle") + .expect("the lease can read through its own lock"); + assert_eq!(unlocked, leased); + + drop(lease); + let _ = std::fs::remove_file(sidecar_path(&path)); + let _ = std::fs::remove_file(path); + } + + #[test] + fn a_lease_lends_a_reader_for_the_drawing_it_has_locked() { + let path = unique_path("leased_reader.dwg"); + std::fs::write(&path, b"drawing bytes").unwrap(); + + let lease = EditLease::acquire(&path).unwrap(); + let mut reader = lease.reader().expect("the lease holds a drawing handle"); + let mut bytes = Vec::new(); + reader.read_to_end(&mut bytes).unwrap(); + assert_eq!(b"drawing bytes".as_slice(), bytes.as_slice()); + + drop(reader); + drop(lease); + let _ = std::fs::remove_file(sidecar_path(&path)); + let _ = std::fs::remove_file(path); + } + #[test] fn fingerprint_detects_same_size_content_change() { let path = unique_path("fingerprint.dwg"); diff --git a/src/io/mod.rs b/src/io/mod.rs index bd166d7ad..2d006f25b 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -1437,6 +1437,7 @@ mod save_failure_tests { acadrust::DxfVersion::AC1032, false, Some(expected), + None, ) .unwrap_err(); @@ -1509,12 +1510,20 @@ pub fn save_owned_as_version_atomic( version: acadrust::DxfVersion, backup: bool, expected_fingerprint: Option, + verify_reader: Option, ) -> Result<(), SaveFailure> { save_owned_as_version_inner(doc, path, version, backup, 0.0, move |path| { let Some(expected) = expected_fingerprint else { return Ok(()); }; - match edit_lock::FileFingerprint::capture(path) { + // Prefer the caller's handle: it comes from the edit lease that already + // locked this path, and a fresh open would collide with that lock rather + // than read through it. + let current = match verify_reader { + Some(mut file) => edit_lock::FileFingerprint::capture_from(&mut file), + None => edit_lock::FileFingerprint::capture(path), + }; + match current { Ok(current) if current == expected => Ok(()), _ => Err(SaveFailure::externally_modified(path)), }