Skip to content

fix: bind the packed matrix to its sig (the #81 torn-read fix missed the matrix) - #83

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

thorwhalen merged 1 commit into
masterfrom
fix/packed-cache-bind-matrix

Conversation

@thorwhalen

Copy link
Copy Markdown
Member

Post-merge refute review of #81.

Defect

#81's content_sig hashes ids.json + metas.json only; the matrix is checked by shape alone. The interleaving the PR itself describes still passes every check:

  1. writer A np.save(matrix.npy)
  2. writer B writes matrix, ids, metas, sig (a same-size corpus, e.g. listed in another order)
  3. writer A writes ids, metas, sig

Disk: B's matrix beside A's ids/metas and a sig that vouches for A's ids/metas. Shapes agree, the sig agrees, so _load_packed serves rows under the wrong ids. The #81 tests only mutate ids/metas without rewriting the sig, so they never exercise this.

Fix

Each _save_packed writes matrix-<gen>.npy, ids-<gen>.json, metas-<gen>.json under a fresh uuid generation that only that writer touches, then publishes sig.json (naming the generation, plus content_sig and shape) last via an atomic os.replace. A reader follows the sig to exactly one writer's complete set, so a mixed set can't exist on disk. This is option 1 of #77 done with a file-level pointer rather than a directory swap. It also stops a writer truncating a matrix.npy that another process has memory-mapped, because files are never rewritten in place.

  • Older generations (and stray sig-*.tmp) are swept after publishing, keeping this writer's set and whatever set sig.json names by then. A sweep that races another writer can only cause a cache miss, never wrong rows.
  • Legacy flat-layout caches (matrix.npy + sig with no generation) still load, so no forced rebuild. Older ir reading a new-layout cache finds no matrix.npy, treats it as a miss and rebuilds.
  • No public name, signature or return type changes. Only module-private packed helpers are touched. Dependents raglab (never sets packed_dir) and truffle (not on box) can't reach this path through anything public.

Tests

  • test_interleaved_packed_writers_never_serve_mismatched_rows runs the real interleaving through _save_packed by patching np.save. It fails on master (row for r1 served as r2's) and passes here.
  • test_legacy_flat_packed_layout_still_loads and test_republishing_sweeps_older_generations are new.
  • Detect a torn packed-cache read instead of serving wrong rows #81's torn/shape tests now target the files the sig points at, so they keep testing the right thing.
  • Local: 566 passed, 8 skipped (pytest + doctests, py3.13), ruff clean.

Not addressed (still open under #77): a staleness race. If a writer builds a matrix, then another process does put_record (clearing the cache), then the first writer publishes, the cache misses the new record until the next write. The rows are stale but still correct for their ids.

Self-reviewed only (the worker was told not to spawn a sub-agent reviewer). This needs a post-merge review.

🤖 Generated with Claude Code

The content_sig added in #81 hashes ids/metas only. The interleaving
"writer A saves matrix.npy, writer B saves a whole set, A then writes
ids/metas/sig" leaves B's same-shape matrix under A's ids and a sig that
vouches for them, so rows were still served under the wrong ids.

Each save now writes matrix/ids/metas under a fresh generation token that
only that writer touches, and publishes sig.json last via os.replace; a
reader follows the sig to exactly one writer's complete set. Older
generations are swept after publishing. Legacy flat-layout caches still
load. Regression test drives the real interleaving through _save_packed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@thorwhalen
thorwhalen merged commit deec6ac into master Sep 22, 2026
12 checks passed
@thorwhalen
thorwhalen deleted the fix/packed-cache-bind-matrix branch September 22, 2026 14:10
thorwhalen added a commit that referenced this pull request Sep 22, 2026
…#84)

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

Copy link
Copy Markdown
Member Author

Post-merge review: three defects fixed in #84. (1) A writer could load another process's older packed set and miss records it had just written, because the _packed_stale guard skips the disk clear. (2) EOFError from an empty or truncated .npy escaped _load_packed, so every matrix() call raised. (3) There was no fsync before the os.replace publish. Correction to this PR's dependents note: raglab does reach the packed path, via ir.as_retriever → CorpusStore.local (which sets packed_dir). Its tests pass against #84. Design concern that remains open under #77: a process that only writes, and never calls matrix(), can still leave another process's older build published.

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