From c65982f407224be8eedb996e5dcd26351137bf7a Mon Sep 17 00:00:00 2001 From: hermes Date: Wed, 14 Oct 2026 22:49:52 +0000 Subject: [PATCH] fix: lock sync's first inspection, resolve symlinks without their target Fourth Codex (gpt-6.1-sol, xhigh) pass over 5e0831d, each item verified before fixing: - sync inspects the local database under the shared lock, so another sync cannot swap it under that connection. - symlinks are resolved by reading the link (db::resolve_db_path), not canonicalize, so a symlinked path finds the same lock and install target even while the real file is missing (fresh install, mid-swap) or the link dangles. - readers go ahead unlocked when no lock file can be created; nothing can replace the database there. (A read-only data directory still fails in SQLite's WAL setup, as in 0.2.4.) - XLSX shared strings are capped at 2,000,000 entries. Verified live: an install waits for a reader's shared lock; a symlinked db_path waits on the real path's lock and stays a symlink. CLI goldens unchanged; live A/B vs 0.2.4: 16 passed, discover hits the IDX 403 on both builds (exit 4 vs 1, intended). Co-Authored-By: Claude Opus 5.5 --- crates/idx-core/src/investor.rs | 28 ++++----- crates/idx-core/src/ownership/db.rs | 70 +++++++++++++++++++++++ crates/idx-core/src/ownership/snapshot.rs | 16 ++++-- crates/idx-core/src/ownership/xlsx.rs | 22 ++++++- 4 files changed, 111 insertions(+), 25 deletions(-) diff --git a/crates/idx-core/src/investor.rs b/crates/idx-core/src/investor.rs index 001dc5c..3d233d1 100644 --- a/crates/idx-core/src/investor.rs +++ b/crates/idx-core/src/investor.rs @@ -52,19 +52,8 @@ impl Investor { /// snapshot swap never happens under an open connection. Released when /// the returned file is dropped. fn lock_file(&self, exclusive: bool) -> Result { - let db_path = self.db_path()?; - let parent = match db_path.parent() { - Some(parent) if !parent.as_os_str().is_empty() => parent.to_path_buf(), - _ => PathBuf::from("."), - }; - fs::create_dir_all(&parent).map_err(|e| IdxError::Io(e.to_string()))?; - // Resolve symlinks, so every path to one database shares one lock. - let real = match fs::canonicalize(&db_path) { - Ok(real) => real, - Err(_) => fs::canonicalize(&parent) - .map_err(|e| IdxError::Io(e.to_string()))? - .join(db_path.file_name().unwrap_or_default()), - }; + // Every path to one database (symlinks included) shares one lock. + let real = db::resolve_db_path(&self.db_path()?)?; let mut lock_name = real.into_os_string(); lock_name.push(".lock"); let file = fs::OpenOptions::new() @@ -86,9 +75,11 @@ impl Investor { self.lock_file(true) } - /// A read connection holding the shared lock for its lifetime. + /// A read connection holding the shared lock for its lifetime. Where no + /// lock file can be created (a read-only filesystem), nothing can replace + /// the database either, so reads go ahead unlocked. fn connect(&self) -> Result { - let lock = self.lock_file(false)?; + let lock = self.lock_file(false).ok(); Ok(ReadConnection { conn: self.open_db()?, _lock: lock, @@ -268,7 +259,9 @@ impl Investor { ) -> Result { let manifest_source = snapshot::resolve_manifest_source(manifest)?; let db_path = self.db_path()?; - snapshot::sync_snapshot(&manifest_source, &db_path, force, || self.write_lock()) + snapshot::sync_snapshot(&manifest_source, &db_path, force, |exclusive| { + self.lock_file(exclusive) + }) } /// Imports one report. A release already imported (same SHA-256) is @@ -467,8 +460,9 @@ pub struct OwnershipStatus { } struct ReadConnection { + // Dropped (closed) before the lock it is declared ahead of. conn: Connection, - _lock: fs::File, + _lock: Option, } impl std::ops::Deref for ReadConnection { diff --git a/crates/idx-core/src/ownership/db.rs b/crates/idx-core/src/ownership/db.rs index 3646fee..54ed388 100644 --- a/crates/idx-core/src/ownership/db.rs +++ b/crates/idx-core/src/ownership/db.rs @@ -133,6 +133,43 @@ pub fn open_db() -> Result { } /// Open the ownership database at `db_path`, creating it and migrating the schema. +/// Where the database file really lives: symlinks are followed by reading +/// the link, so this works even while the target does not exist (a fresh +/// install, or mid-replacement during sync). Creates the parent directory. +pub(crate) fn resolve_db_path(path: &Path) -> Result { + let mut current = path.to_path_buf(); + for _ in 0..16 { + let parent = match current.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent.to_path_buf(), + _ => PathBuf::from("."), + }; + fs::create_dir_all(&parent).map_err(|e| IdxError::Io(e.to_string()))?; + let parent = fs::canonicalize(&parent).map_err(|e| IdxError::Io(e.to_string()))?; + let name = current.file_name().ok_or_else(|| { + IdxError::ConfigError(format!( + "ownership database path has no file name: {}", + path.display() + )) + })?; + let candidate = parent.join(name); + match fs::symlink_metadata(&candidate) { + Ok(meta) if meta.file_type().is_symlink() => { + let target = fs::read_link(&candidate).map_err(|e| IdxError::Io(e.to_string()))?; + current = if target.is_absolute() { + target + } else { + parent.join(target) + }; + } + _ => return Ok(candidate), + } + } + Err(IdxError::Io(format!( + "too many symlink levels resolving {}", + path.display() + ))) +} + pub fn open_db_at(db_path: &Path) -> Result { if let Some(parent) = db_path.parent() { fs::create_dir_all(parent).map_err(|e| IdxError::DatabaseError(e.to_string()))?; @@ -1965,4 +2002,37 @@ mod tests { .unwrap(); assert_eq!(buyers, 1); } + + #[cfg(unix)] + #[test] + fn resolve_db_path_follows_symlinks_even_when_the_target_is_missing() { + let root = std::env::temp_dir().join(format!( + "idx-resolve-{}-{}", + std::process::id(), + fastrand::u64(..) + )); + let real_dir = root.join("real"); + std::fs::create_dir_all(&real_dir).unwrap(); + let real_dir = std::fs::canonicalize(&real_dir).unwrap(); + let link = root.join("link.db"); + // Dangling: the target does not exist yet (fresh install, mid-swap). + std::os::unix::fs::symlink(real_dir.join("ownership.db"), &link).unwrap(); + assert_eq!( + super::resolve_db_path(&link).unwrap(), + real_dir.join("ownership.db") + ); + // Relative link, resolved against the link's directory. + let relative = root.join("relative.db"); + std::os::unix::fs::symlink("real/ownership.db", &relative).unwrap(); + assert_eq!( + super::resolve_db_path(&relative).unwrap(), + real_dir.join("ownership.db") + ); + // A plain path is returned as-is (canonical parent). + assert_eq!( + super::resolve_db_path(&real_dir.join("ownership.db")).unwrap(), + real_dir.join("ownership.db") + ); + let _ = std::fs::remove_dir_all(&root); + } } diff --git a/crates/idx-core/src/ownership/snapshot.rs b/crates/idx-core/src/ownership/snapshot.rs index 9768c53..781a691 100644 --- a/crates/idx-core/src/ownership/snapshot.rs +++ b/crates/idx-core/src/ownership/snapshot.rs @@ -144,22 +144,26 @@ pub fn parse_manifest(raw: &str) -> Result Ok(manifest) } -/// `lock` takes the database's exclusive writer lock; it is held only while -/// rechecking and installing, never during the downloads. +/// `lock(exclusive)` takes the database's lock: shared while inspecting the +/// local database, exclusive while rechecking and installing, and never held +/// during the downloads. pub fn sync_snapshot( manifest_source: &str, db_path: &Path, force: bool, - lock: impl FnOnce() -> Result, + lock: impl Fn(bool) -> Result, ) -> Result { // Results report the configured path; files are read and replaced at the // resolved one, so a symlinked path updates its target instead of being // overwritten with a regular file. let configured_db_path = db_path; - let resolved_db_path = fs::canonicalize(db_path).unwrap_or_else(|_| db_path.to_path_buf()); + let resolved_db_path = db::resolve_db_path(db_path)?; let db_path = resolved_db_path.as_path(); let manifest = fetch_manifest(manifest_source)?; - let local_state = inspect_local_db(db_path)?; + let local_state = { + let _shared = lock(false)?; + inspect_local_db(db_path)? + }; let decision = build_sync_decision(local_state.as_ref(), &manifest.snapshot, force); if !decision.should_download { @@ -211,7 +215,7 @@ pub fn sync_snapshot( let install_result = (|| -> Result<(), IdxError> { validate_snapshot_db(&temp_path, &manifest.snapshot)?; - let _lock = lock()?; + let _lock = lock(true)?; // The decision was made before the download; an import or edit that // landed since must not be overwritten without --force. if !force && !same_local_state(local_state.as_ref(), inspect_local_db(db_path)?.as_ref()) { diff --git a/crates/idx-core/src/ownership/xlsx.rs b/crates/idx-core/src/ownership/xlsx.rs index 40e0a37..c8650d9 100644 --- a/crates/idx-core/src/ownership/xlsx.rs +++ b/crates/idx-core/src/ownership/xlsx.rs @@ -127,7 +127,10 @@ fn parse_shared_strings(xml: &str) -> Result, IdxError> { b"rPh" => in_phonetic = true, _ => {} }, - Ok(Event::Empty(e)) if e.local_name().as_ref() == b"si" => strings.push(String::new()), + Ok(Event::Empty(e)) if e.local_name().as_ref() == b"si" => { + check_shared_string_count(strings.len())?; + strings.push(String::new()) + } Ok(Event::Text(text)) if in_text && !in_phonetic => { if let Some(value) = current.as_mut() { let unescaped = text @@ -137,7 +140,10 @@ fn parse_shared_strings(xml: &str) -> Result, IdxError> { } } Ok(Event::End(e)) => match e.local_name().as_ref() { - b"si" => strings.push(current.take().unwrap_or_default()), + b"si" => { + check_shared_string_count(strings.len())?; + strings.push(current.take().unwrap_or_default()) + } b"t" => in_text = false, b"rPh" => in_phonetic = false, _ => {} @@ -184,6 +190,18 @@ fn text_cap_error() -> IdxError { ) } +const MAX_SHARED_STRINGS: usize = 2_000_000; + +fn check_shared_string_count(count: usize) -> Result<(), IdxError> { + if count >= MAX_SHARED_STRINGS { + return Err(xml_error( + "shared strings", + format!("more than {MAX_SHARED_STRINGS} entries"), + )); + } + Ok(()) +} + fn check_row_count(rows: usize) -> Result<(), IdxError> { if rows >= MAX_SHEET_ROWS { return Err(xml_error(