fix(storage): never treat a lock held by our own PID as stale by adityamukho · Pull Request #314 · project-minigraf/minigraf

adityamukho · 2026-08-09T17:49:37Z

Root cause of #304

FileLock::acquire treated a lock file whose holder PID equalled our own as stale, deleted it, and opened anyway:

if pid == our_pid || !Self::is_process_alive(pid) {
    let _ = std::fs::remove_file(&lock_path);
    return Self::acquire(db_path);
}

The comment justified this as "our own leaked handle". But a leaked handle requires mem::forget; the overwhelmingly more likely explanation for our own PID holding the lock is a handle that is open and in use right now, elsewhere in the process. Stealing it gives two live FileBackends on one file.

That is the bug. Each FileBackend caches its own header.page_count, allocates new pages from that count, and bounds-checks read_page against it:

fn read_page(&self, page_id: u64) -> Result<Vec<u8>> {
    if page_id >= self.header.page_count {
        anyhow::bail!("Page {} out of bounds (total pages: {})", page_id, self.header.page_count);

Two handles ⇒ two page tables that diverge. Page N out of bounds was never an allocation off-by-one — it is an addressing error against a stale cached count.

That also explains #304's negative result: the 30-worker, 42,000-page stress test found nothing because it never had two overlapping handles. File size was never the driver.

Reproduction

Deterministic, 40 rounds of interleaved writes, same payloads either way:

mode	result
one handle at a time (open → write → checkpoint → drop)	0 errors
a second handle opened while the first is live	40 errors, first round: `Page 130 out of bounds (total pages: 113)`

Later rounds degrade further — Serde Deserialization Error, stream_all_entries: expected leaf page at page_id=68 — so this is real corruption, not a transient read failure.

Second defect: the lock file itself

FileLock::drop removes self.path unconditionally, and the second acquire recreated that same path. So whichever handle drops first deletes the lock belonging to the survivor. Verified: after the duplicate drops, the surviving live handle holds no lock file at all, and a third handle opens against it freely — the sidecar stops excluding other processes too.

The fix

Refuse a lock held by our own PID, with an error that names the same-process case (the generic "another process" wording sent #304 hunting for a second process). A lock held by a different, dead process is still reclaimed — that path is unchanged and now has its own test.

Tests

test_second_open_in_same_process_is_refused — ablation-proven: restore the old condition and it fails with "a second handle in the same process must be refused".
test_reopen_after_drop_still_works — the sequential open/drop/reopen cycle temporal_reasoning uses between commits is unaffected.
test_dead_other_process_lock_is_still_reclaimed — spawns a real child, waits for it to exit, and writes its dead PID into the lock.

Full suite: 990 passed, 0 failed, 8 ignored.

Behaviour change

Callers that reopened a path they already hold open now get an error instead of silent corruption. They should reuse the existing handle — Minigraf is cheap to clone and all clones share one database.

This surfaced in the test suite: 14 crash-simulation sites use mem::forget to skip the Drop checkpoint, which also skips FileLock::drop and so leaves the lock holding our PID. That is not what a crash leaves behind — a crashed process leaves the lock holding its own, now-dead PID. Those sites now rewrite the lock to a genuinely dead PID, which models the crash faithfully and exercises the real reclamation path rather than depending on minigraf treating a live lock as stale.

Downstream

temporal_reasoning is the reporter and reaches this state through normal operation (project-minigraf/temporal_reasoning#253, #251). After this lands it will get a loud lock error where it previously corrupted the graph — the right trade, and it makes the remaining work on that side visible rather than silent.

Fixes #304

🤖 Generated with Claude Code

https://claude.ai/code/session_01QRGNkzJc5FdfpbzAh4ZZzK

FileLock::acquire removed the sidecar lock and opened anyway whenever the holder PID equalled our own, on the theory that it could only be a leaked handle. The far more likely explanation is a handle that is open and in use right now, elsewhere in this process. Stealing it produced two live FileBackends on one file. Each caches its own header.page_count, allocates new pages from that count, and bounds-checks read_page against it, so the two page tables diverge. That is the source of the intermittent "Page N out of bounds (total pages: M)" reported in #304 -- an addressing error against a stale cached count, not an allocation off-by-one. Reproduced deterministically: 40 rounds of interleaved writes through two overlapping handles fails on the first round with "Page 130 out of bounds (total pages: 113)", where the same 40 rounds through one handle at a time produce zero errors. It also corrupted the lock itself. FileLock::drop removes self.path unconditionally, and the second acquire had recreated that same path, so whichever handle dropped first deleted the lock belonging to the survivor -- leaving a live handle with no lock at all and admitting other processes. A lock held by a different, dead process is still reclaimed; that path is unchanged and now has its own test. The crash-simulation idiom in the test suite depended on the stolen lock: mem::forget skips FileLock::drop, so it leaves the lock holding OUR pid, which is not what a crash leaves behind. Those sites now rewrite the lock to a genuinely dead PID, which models a crashed holder faithfully and exercises the real reclamation path instead. Refs #304 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QRGNkzJc5FdfpbzAh4ZZzK

Two platform assumptions in the new tests only hold on procfs targets. is_process_alive cannot tell a dead PID from a live one without /proc and deliberately errs towards "alive" on macOS and Windows, so a dead PID written into a lock file still blocks there. macOS CI caught this on test_dead_other_process_lock_is_still_reclaimed, which is now gated to linux/android -- reclaiming a dead holder's lock is a procfs-only guarantee, and the platform comment in is_process_alive already says so. The same assumption was in the crash-simulation helpers. They now remove the lock file instead of rewriting its PID, which models the state the next open finds once a crashed holder is cleaned up and works everywhere. Reclamation itself stays covered by the gated test above; these tests are about WAL replay. Refs #304 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QRGNkzJc5FdfpbzAh4ZZzK

codecov · 2026-08-09T17:59:49Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

adityamukho merged commit 53e4884 into main Aug 10, 2026
17 checks passed

adityamukho deleted the fix-304-filelock-self-steal branch August 10, 2026 05:04

This was referenced Aug 10, 2026

Confirm #251 non-recurrence with an at-scale ingestion against a persistent graph path project-minigraf/temporal_reasoning#256

Closed

perf: checkpoint() is O(graph size) and flat in dirty bytes — it rebuilds all four covering indexes every call #315

Closed

Navigation Menu

Repository navigation

fix(storage): never treat a lock held by our own PID as stale - #314

fix(storage): never treat a lock held by our own PID as stale#314
adityamukho merged 2 commits into
mainfrom
fix-304-filelock-self-steal

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Root cause of #304

Reproduction

Second defect: the lock file itself

The fix

Tests

Behaviour change

Downstream

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant