From c88f07d21977f44f22b65aee201e7b39b850df9a Mon Sep 17 00:00:00 2001 From: NgoQuocViet2001 Date: Mon, 17 Aug 2026 22:49:54 +0700 Subject: [PATCH] fix(io): stop the drawing lock blocking our own reads on Windows Saving to a path that already holds a drawing fails on Windows with "could not verify before saving: ... (os error 33)". Save As over any existing drawing is affected; saving to a new path is not. EditLease::acquire() opens the destination and takes the platform drawing lock on it, then the save fingerprints the same path to check nothing changed underneath. LockFileEx is mandatory and scoped to the handle that took it, so that second open is refused against our own lock, lands in the non-NotFound arm and aborts the save. flock on Unix is advisory and the second open succeeds, so neither CI nor a Linux desktop sees it. Read through the handle the lease already holds instead. A duplicate shares the lock ownership, so the pre-replace re-check on the save worker can use one too rather than re-opening the path. The same collision reached the headless open op, which read a path this session may hold leased: automation's save_then_open_round_trips fails on main for that reason, one line after the save it no longer reaches. --- src/app/automation.rs | 37 ++++++++++++++++++++- src/app/update/file.rs | 56 +++++++++++++++++++++++++++++-- src/io/edit_lock.rs | 75 +++++++++++++++++++++++++++++++++++++++++- src/io/mod.rs | 11 ++++++- 4 files changed, 173 insertions(+), 6 deletions(-) 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)), }