mirror of
https://github.com/0xrsydn/idx-cli.git
synced 2026-11-07 09:45:36 +00:00
fix: readers lock read-only instead of skipping, one path per sync
Final Codex (gpt-6.1-sol, xhigh) pass over c65982f confirmed every
earlier item fixed and found:
- high: readers skipped the lock on any error, so a lock file owned by
another user (who can still sync) let a reader run unlocked. Readers
now open an existing lock file read-only for the shared lock, and read
unlocked only when no lock file exists and none can be created.
- low: sync resolved the symlinked path for the install target and the
lock separately; it now resolves once and passes that path to the
lock callback, so both always name the same database.
Verified live: a reader with a read-only lock file waits for an
exclusive holder; install waits for readers; a symlinked path keeps its
link. CLI goldens unchanged; live A/B vs 0.2.4: 15 passed, screen hit a
transient MSN 408 on the old build only (retries pass), discover hits
the IDX 403 on both (exit 4 vs 1, intended).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
c65982f407
commit
db59744976
2 changed files with 59 additions and 29 deletions
|
|
@ -54,32 +54,23 @@ impl Investor {
|
||||||
fn lock_file(&self, exclusive: bool) -> Result<fs::File, IdxError> {
|
fn lock_file(&self, exclusive: bool) -> Result<fs::File, IdxError> {
|
||||||
// Every path to one database (symlinks included) shares one lock.
|
// Every path to one database (symlinks included) shares one lock.
|
||||||
let real = db::resolve_db_path(&self.db_path()?)?;
|
let real = db::resolve_db_path(&self.db_path()?)?;
|
||||||
let mut lock_name = real.into_os_string();
|
lock_at(&real, exclusive)
|
||||||
lock_name.push(".lock");
|
|
||||||
let file = fs::OpenOptions::new()
|
|
||||||
.create(true)
|
|
||||||
.truncate(false)
|
|
||||||
.write(true)
|
|
||||||
.open(PathBuf::from(lock_name))
|
|
||||||
.map_err(|e| IdxError::Io(format!("failed to open ownership lock: {e}")))?;
|
|
||||||
if exclusive {
|
|
||||||
file.lock()
|
|
||||||
} else {
|
|
||||||
file.lock_shared()
|
|
||||||
}
|
|
||||||
.map_err(|e| IdxError::Io(format!("failed to lock ownership database: {e}")))?;
|
|
||||||
Ok(file)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn write_lock(&self) -> Result<fs::File, IdxError> {
|
fn write_lock(&self) -> Result<fs::File, IdxError> {
|
||||||
self.lock_file(true)
|
self.lock_file(true)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A read connection holding the shared lock for its lifetime. Where no
|
/// A read connection holding the shared lock for its lifetime.
|
||||||
/// 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).ok();
|
let real = db::resolve_db_path(&self.db_path()?)?;
|
||||||
|
let lock = match lock_at(&real, false) {
|
||||||
|
Ok(lock) => Some(lock),
|
||||||
|
// No lock file and none can be created (a read-only filesystem):
|
||||||
|
// no writer can lock it to replace the database either.
|
||||||
|
Err(_) if unlockable(&real) => None,
|
||||||
|
Err(err) => return Err(err),
|
||||||
|
};
|
||||||
Ok(ReadConnection {
|
Ok(ReadConnection {
|
||||||
conn: self.open_db()?,
|
conn: self.open_db()?,
|
||||||
_lock: lock,
|
_lock: lock,
|
||||||
|
|
@ -258,10 +249,7 @@ impl Investor {
|
||||||
force: bool,
|
force: bool,
|
||||||
) -> 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()?;
|
snapshot::sync_snapshot(&manifest_source, &self.db_path()?, force, lock_at)
|
||||||
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
|
||||||
|
|
@ -459,6 +447,47 @@ pub struct OwnershipStatus {
|
||||||
pub releases: Vec<OwnershipRelease>,
|
pub releases: Vec<OwnershipRelease>,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn lock_path(db_path: &Path) -> PathBuf {
|
||||||
|
let mut name = db_path.as_os_str().to_os_string();
|
||||||
|
name.push(".lock");
|
||||||
|
PathBuf::from(name)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Locks `<db>.lock` beside the resolved database path. A shared lock does
|
||||||
|
/// not need write access, so readers fall back to opening an existing lock
|
||||||
|
/// file read-only (e.g. one created by another user).
|
||||||
|
fn lock_at(db_path: &Path, exclusive: bool) -> Result<fs::File, IdxError> {
|
||||||
|
let path = lock_path(db_path);
|
||||||
|
let opened = fs::OpenOptions::new()
|
||||||
|
.create(true)
|
||||||
|
.truncate(false)
|
||||||
|
.write(true)
|
||||||
|
.open(&path);
|
||||||
|
let file = match opened {
|
||||||
|
Ok(file) => file,
|
||||||
|
Err(_) if !exclusive && path.exists() => fs::File::open(&path)
|
||||||
|
.map_err(|e| IdxError::Io(format!("failed to open ownership lock: {e}")))?,
|
||||||
|
Err(e) => return Err(IdxError::Io(format!("failed to open ownership lock: {e}"))),
|
||||||
|
};
|
||||||
|
if exclusive {
|
||||||
|
file.lock()
|
||||||
|
} else {
|
||||||
|
file.lock_shared()
|
||||||
|
}
|
||||||
|
.map_err(|e| IdxError::Io(format!("failed to lock ownership database: {e}")))?;
|
||||||
|
Ok(file)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// True when the lock file is absent and cannot be created.
|
||||||
|
fn unlockable(db_path: &Path) -> bool {
|
||||||
|
let path = lock_path(db_path);
|
||||||
|
!path.exists()
|
||||||
|
&& matches!(
|
||||||
|
fs::OpenOptions::new().create(true).truncate(false).write(true).open(&path),
|
||||||
|
Err(e) if matches!(e.kind(), std::io::ErrorKind::PermissionDenied | std::io::ErrorKind::ReadOnlyFilesystem)
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
struct ReadConnection {
|
struct ReadConnection {
|
||||||
// Dropped (closed) before the lock it is declared ahead of.
|
// Dropped (closed) before the lock it is declared ahead of.
|
||||||
conn: Connection,
|
conn: Connection,
|
||||||
|
|
|
||||||
|
|
@ -144,14 +144,15 @@ pub fn parse_manifest(raw: &str) -> Result<OwnershipSnapshotManifest, IdxError>
|
||||||
Ok(manifest)
|
Ok(manifest)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// `lock(exclusive)` takes the database's lock: shared while inspecting the
|
/// `lock(resolved_db_path, exclusive)` takes the database's lock: shared
|
||||||
/// local database, exclusive while rechecking and installing, and never held
|
/// while inspecting the local database, exclusive while rechecking and
|
||||||
/// during the downloads.
|
/// installing, and never held during the downloads. The path is resolved
|
||||||
|
/// once, so the lock and the replaced file are always the same database.
|
||||||
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 Fn(bool) -> Result<L, IdxError>,
|
lock: impl Fn(&Path, 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
|
||||||
|
|
@ -161,7 +162,7 @@ pub fn sync_snapshot<L>(
|
||||||
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 = {
|
let local_state = {
|
||||||
let _shared = lock(false)?;
|
let _shared = lock(db_path, false)?;
|
||||||
inspect_local_db(db_path)?
|
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);
|
||||||
|
|
@ -215,7 +216,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(true)?;
|
let _lock = lock(db_path, 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()) {
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue