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 <noreply@anthropic.com>
This commit is contained in:
hermes 2026-10-14 22:49:52 +00:00
commit c65982f407
4 changed files with 111 additions and 25 deletions

View file

@ -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<fs::File, IdxError> {
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<ReadConnection, IdxError> {
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<OwnershipSyncResult, IdxError> {
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<fs::File>,
}
impl std::ops::Deref for ReadConnection {

View file

@ -133,6 +133,43 @@ pub fn open_db() -> Result<Connection, IdxError> {
}
/// 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<PathBuf, IdxError> {
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<Connection, IdxError> {
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);
}
}

View file

@ -144,22 +144,26 @@ pub fn parse_manifest(raw: &str) -> Result<OwnershipSnapshotManifest, IdxError>
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<L>(
manifest_source: &str,
db_path: &Path,
force: bool,
lock: impl FnOnce() -> Result<L, IdxError>,
lock: impl Fn(bool) -> Result<L, IdxError>,
) -> Result<OwnershipSyncResult, IdxError> {
// 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<L>(
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()) {

View file

@ -127,7 +127,10 @@ fn parse_shared_strings(xml: &str) -> Result<Vec<String>, 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<Vec<String>, 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(