Skip to content

fix(store): packed cache hid a writer's own records; EOFError escaped; add fsync (review of #83) - #84

Merged
thorwhalen merged 1 commit into
masterfrom
fix/packed-cache-review-83
Sep 22, 2026
Merged

thorwhalen merged 1 commit into
masterfrom
fix/packed-cache-review-83

Conversation

@thorwhalen

Copy link
Copy Markdown
Member

Post-merge adversarial review of #83. Refs #77.

Defects found

  1. A writer could miss records it had just written. _invalidate_matrix clears the disk cache only on the first write after a publish (the _packed_stale guard that keeps bulk builds cheap). If another process publishes a matrix between two of this process's writes, the second write leaves that set on disk, and this process's next matrix() loaded it. Repro: P1 put_record(r1); P2 matrix() publishes {r1}; P1 put_record(r2); P1 matrix() returned ['r1']. This predates fix: bind the packed matrix to its sig (the #81 torn-read fix missed the matrix) #83 but lives in the same cache. Fix: _load_packed returns a miss while _packed_stale is set. This process then rebuilds and republishes, which also heals the cache for other readers. The cost is zero extra filesystem calls.
  2. EOFError escaped _load_packed. np.load of a zero-length .npy raises EOFError, which is neither OSError nor ValueError. So every matrix() raised for as long as that sig.json stayed, and nothing cleared it because the save path is never reached. A zero-length data file behind a durable sig is exactly what a power loss leaves when there is no fsync (see 3). Now it counts as a miss.
  3. No fsync. fix: bind the packed matrix to its sig (the #81 torn-read fix missed the matrix) #83 says "a crash mid-write leaves the previous sig in charge". That holds for a process crash but not a power loss: os.replace can reach the disk before the data blocks of the files it names. Each data file and the sig tmp are now fsynced before os.replace, and the directory after it (POSIX only; on Windows the directory fsync is skipped).

Checked and holding

  • Atomic publish: os.replace is rename(2) on Linux and macOS and MoveFileEx(REPLACE_EXISTING) on Windows. On Windows it can fail with PermissionError while another process has sig.json open. That lands in except OSError, which clears the cache, so the result is a miss and never wrong rows.
  • A reader following a sig whose generation was just swept gets FileNotFoundError, which is a miss. A reader that already has the matrix mmapped keeps the inode on POSIX. On Windows the unlink fails and a later sweep removes the file.
  • Leaks: sets from crashed writers and orphaned sig-*.tmp are swept on the next save or clear. Mixed-version use (an older ir clearing) can leave generation files behind until a newer ir saves. Minor.
  • Legacy flat layout loads, and a newer save sweeps it away.
  • Correction to fix: bind the packed matrix to its sig (the #81 torn-read fix missed the matrix) #83's dependents note: raglab does reach this path. ir_sources goes through ir.as_retriever(name) to CorpusStore.local, which sets packed_dir. raglab's tests pass against this branch (49 passed).

Tests

Three regression tests. All fail on master and pass here: own-write visibility, empty/truncated matrix counts as a miss, and fsync happens before publish. Locally: 569 passed, 8 skipped (pytest + doctests, py3.12). Ruff is clean on the changed files; tests/test_select.py has 2 lint findings that were already on master.

Not addressed: the cross-process staleness race stays open under #77. A process that only writes and never reads can still leave another process's older build published.

Self-reviewed only (this reviewer was told not to spawn a sub-agent).

🤖 Generated with Claude Code

Post-merge review of #83.

- A writer whose disk-clear was skipped by the _packed_stale guard loaded a
  packed set another process published before its latest writes, so it did
  not see records it had just written. _load_packed now treats the disk
  cache as a miss while this process has unpublished writes; the rebuild
  republishes, which also heals the cache for other readers.
- np.load of an empty/truncated .npy raises EOFError, which escaped
  _load_packed and made every matrix() call raise while that sig stayed.
- Data files and the sig tmp are fsynced before os.replace, and the
  directory after (POSIX), so a power loss cannot leave a durable sig
  naming data blocks that never reached disk.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thorwhalen
thorwhalen merged commit 963eeae into master Sep 22, 2026
12 checks passed
@thorwhalen
thorwhalen deleted the fix/packed-cache-review-83 branch September 22, 2026 14:40
@thorwhalen

Copy link
Copy Markdown
Member Author

round-3 review: no defect found. Checked: the stale-guard own-write visibility, EOFError on empty or truncated .npy, fsync ordering, the post-publish sweep racing a concurrent writer (the result is a miss, never wrong rows), 6-process put/matrix stress with a row-to-id vector check, and raglab's 49 tests against master. A pre-existing torn read in the per-record stores, one layer below this PR, is filed as #85.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant