mirror of
https://github.com/0xrsydn/idx-cli.git
synced 2026-11-07 01:35:36 +00:00
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:
parent
5e0831d8fe
commit
c65982f407
4 changed files with 111 additions and 25 deletions
|
|
@ -52,19 +52,8 @@ impl Investor {
|
||||||
/// snapshot swap never happens under an open connection. Released when
|
/// snapshot swap never happens under an open connection. Released when
|
||||||
/// the returned file is dropped.
|
/// the returned file is dropped.
|
||||||
fn lock_file(&self, exclusive: bool) -> Result<fs::File, IdxError> {
|
fn lock_file(&self, exclusive: bool) -> Result<fs::File, IdxError> {
|
||||||
let db_path = self.db_path()?;
|
// Every path to one database (symlinks included) shares one lock.
|
||||||
let parent = match db_path.parent() {
|
let real = db::resolve_db_path(&self.db_path()?)?;
|
||||||
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()),
|
|
||||||
};
|
|
||||||
let mut lock_name = real.into_os_string();
|
let mut lock_name = real.into_os_string();
|
||||||
lock_name.push(".lock");
|
lock_name.push(".lock");
|
||||||
let file = fs::OpenOptions::new()
|
let file = fs::OpenOptions::new()
|
||||||
|
|
@ -86,9 +75,11 @@ impl Investor {
|
||||||
self.lock_file(true)
|
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> {
|
fn connect(&self) -> Result<ReadConnection, IdxError> {
|
||||||
let lock = self.lock_file(false)?;
|
let lock = self.lock_file(false).ok();
|
||||||
Ok(ReadConnection {
|
Ok(ReadConnection {
|
||||||
conn: self.open_db()?,
|
conn: self.open_db()?,
|
||||||
_lock: lock,
|
_lock: lock,
|
||||||
|
|
@ -268,7 +259,9 @@ impl Investor {
|
||||||
) -> Result<OwnershipSyncResult, IdxError> {
|
) -> Result<OwnershipSyncResult, IdxError> {
|
||||||
let manifest_source = snapshot::resolve_manifest_source(manifest)?;
|
let manifest_source = snapshot::resolve_manifest_source(manifest)?;
|
||||||
let db_path = self.db_path()?;
|
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
|
/// Imports one report. A release already imported (same SHA-256) is
|
||||||
|
|
@ -467,8 +460,9 @@ pub struct OwnershipStatus {
|
||||||
}
|
}
|
||||||
|
|
||||||
struct ReadConnection {
|
struct ReadConnection {
|
||||||
|
// Dropped (closed) before the lock it is declared ahead of.
|
||||||
conn: Connection,
|
conn: Connection,
|
||||||
_lock: fs::File,
|
_lock: Option<fs::File>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl std::ops::Deref for ReadConnection {
|
impl std::ops::Deref for ReadConnection {
|
||||||
|
|
|
||||||
|
|
@ -133,6 +133,43 @@ pub fn open_db() -> Result<Connection, IdxError> {
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Open the ownership database at `db_path`, creating it and migrating the schema.
|
/// 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> {
|
pub fn open_db_at(db_path: &Path) -> Result<Connection, IdxError> {
|
||||||
if let Some(parent) = db_path.parent() {
|
if let Some(parent) = db_path.parent() {
|
||||||
fs::create_dir_all(parent).map_err(|e| IdxError::DatabaseError(e.to_string()))?;
|
fs::create_dir_all(parent).map_err(|e| IdxError::DatabaseError(e.to_string()))?;
|
||||||
|
|
@ -1965,4 +2002,37 @@ mod tests {
|
||||||
.unwrap();
|
.unwrap();
|
||||||
assert_eq!(buyers, 1);
|
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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -144,22 +144,26 @@ pub fn parse_manifest(raw: &str) -> Result<OwnershipSnapshotManifest, IdxError>
|
||||||
Ok(manifest)
|
Ok(manifest)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// `lock` takes the database's exclusive writer lock; it is held only while
|
/// `lock(exclusive)` takes the database's lock: shared while inspecting the
|
||||||
/// rechecking and installing, never during the downloads.
|
/// local database, exclusive while rechecking and installing, and never held
|
||||||
|
/// during the downloads.
|
||||||
pub fn sync_snapshot<L>(
|
pub fn sync_snapshot<L>(
|
||||||
manifest_source: &str,
|
manifest_source: &str,
|
||||||
db_path: &Path,
|
db_path: &Path,
|
||||||
force: bool,
|
force: bool,
|
||||||
lock: impl FnOnce() -> Result<L, IdxError>,
|
lock: impl Fn(bool) -> Result<L, IdxError>,
|
||||||
) -> Result<OwnershipSyncResult, IdxError> {
|
) -> Result<OwnershipSyncResult, IdxError> {
|
||||||
// Results report the configured path; files are read and replaced at the
|
// Results report the configured path; files are read and replaced at the
|
||||||
// resolved one, so a symlinked path updates its target instead of being
|
// resolved one, so a symlinked path updates its target instead of being
|
||||||
// overwritten with a regular file.
|
// overwritten with a regular file.
|
||||||
let configured_db_path = db_path;
|
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 db_path = resolved_db_path.as_path();
|
||||||
let manifest = fetch_manifest(manifest_source)?;
|
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);
|
let decision = build_sync_decision(local_state.as_ref(), &manifest.snapshot, force);
|
||||||
|
|
||||||
if !decision.should_download {
|
if !decision.should_download {
|
||||||
|
|
@ -211,7 +215,7 @@ pub fn sync_snapshot<L>(
|
||||||
|
|
||||||
let install_result = (|| -> Result<(), IdxError> {
|
let install_result = (|| -> Result<(), IdxError> {
|
||||||
validate_snapshot_db(&temp_path, &manifest.snapshot)?;
|
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
|
// The decision was made before the download; an import or edit that
|
||||||
// landed since must not be overwritten without --force.
|
// landed since must not be overwritten without --force.
|
||||||
if !force && !same_local_state(local_state.as_ref(), inspect_local_db(db_path)?.as_ref()) {
|
if !force && !same_local_state(local_state.as_ref(), inspect_local_db(db_path)?.as_ref()) {
|
||||||
|
|
|
||||||
|
|
@ -127,7 +127,10 @@ fn parse_shared_strings(xml: &str) -> Result<Vec<String>, IdxError> {
|
||||||
b"rPh" => in_phonetic = true,
|
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 => {
|
Ok(Event::Text(text)) if in_text && !in_phonetic => {
|
||||||
if let Some(value) = current.as_mut() {
|
if let Some(value) = current.as_mut() {
|
||||||
let unescaped = text
|
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() {
|
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"t" => in_text = false,
|
||||||
b"rPh" => in_phonetic = 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> {
|
fn check_row_count(rows: usize) -> Result<(), IdxError> {
|
||||||
if rows >= MAX_SHEET_ROWS {
|
if rows >= MAX_SHEET_ROWS {
|
||||||
return Err(xml_error(
|
return Err(xml_error(
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue