fix(store): matrix()/metas() no longer raise while another process writes - #87
Merged
Merged
Conversation
…ites Closes #85. - put_record writes the vector before the meta (delete_record already removes the meta first), so a listed id always has a vector. - Per-record files are written atomically (hidden temp file + os.replace, the same publish step as the packed cache's sig.json, without fsync). On Windows a replace blocked by a reader is retried, then falls back to the old in-place write. On-disk format is unchanged. - _build_matrix/metas skip a record that vanishes or cannot be decoded mid-read; a failed record is re-read once (a missing meta then means deleted, not a gap). A read that still left records out is returned but neither cached nor published as the packed set, and is logged. - delete_record tolerates a concurrent delete of the same record. Stale-cache follow-up found on the way: #86 (xfail test added). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ocess - _AtomicFiles built the write path with Path(root, key), which lets an absolute key (an artifact id can be an absolute path, e.g. links) replace the root and overwrite a file outside the store, while reads still looked under the root. The path now comes from dol.Files itself (root + key, as reads and deletes use), and a key climbing out of the root is refused. - A partial matrix (records unreadable after the retry) is now cached in-process, still never published, so one damaged record no longer makes every search in that process re-read every record file. The warning says how to clear it. - metas() logs skipped records at debug level. - The subprocess stress test uses two writers and larger records, in three rounds; on master it now fails in about 5 of 6 runs (was 1 of 5). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thorwhalen
added a commit
that referenced
this pull request
Sep 22, 2026
On Windows a drive-letter key (store\C:\...) is not a valid path, so the write raises OSError (as it did before #87) instead of landing inside the root; assert that, and that the file the key names is untouched. #87's Windows job failed on this and was missed because the Windows job does not fail the workflow. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #85.
Reproduction
A stress script (6 processes on one file-backed corpus, 200 random
put_record/delete_record/matrix()/metas()each, readers forced to rebuild) failed in 10 of 10 runs on master withKeyError(listed id without a file),EOFError(torn.npy) andJSONDecodeError(torn meta). With puts and reads only (no deletes, the #77 scenario), 6 of 6 runs failed. With this branch: 0 of 10 and 0 of 10.Change
Builds on #83/#84's publish step (write elsewhere, then
os.replace) rather than changing the packed-cache design.put_recordwrites the vector first. Ids are listed frommeta, so a listed id always has a vector.delete_recordalready removed the meta first, and now also tolerates a concurrent delete of the same record._json_store/_ndarray_storewrite through_AtomicFiles: a hidden temp file in the same directory, thenos.replace. Hidden, so thedol.Fileslisting never shows it as a key. Reads, listing and deletes still go throughdol.Filesunchanged. No fsync per record (atomic against other processes, not durable against power loss), so a bulk build does not pay one. The on-disk format is unchanged (indent=4, UTF-8 JSON;np.savebytes), so existing corpora read as before. On Windows,os.replacerefuses while another process has the file open. The write is retried with back-off, then falls back to the old in-place write rather than failing._build_matrixandmetas()treat a record that raisesKeyError/EOFError/ValueErroron read as not yet written. A failed record is read once more: if its meta is gone by then it was deleted, which is not a gap. If any record still can't be read,matrix()logs a warning naming the ids and returns the partial result. It caches that result in-process, like any build, but never publishes it as the packed set.dol.Filesitself (root + key, the same path reads and deletes use), and a key that climbs out of the root is refused.Tests
tests/test_store_concurrency.py: write order; meta-without-vector and torn meta/vector files are skipped, not cached, not published, and picked up once whole; deleted-mid-read is not a gap; no temp files left and temp files are not keys; the JSON format still matchesdol.JsonFiles; the Windows retry-then-in-place path and the POSIX re-raise; absolute keys land inside the root and escaping keys are refused. It also runs a subprocess stress test: two writers build 40 large records (with overwrites) while the test process loops onmatrix()/metas(), in three rounds. On master, 8 of the new tests fail deterministically, and the stress test fails in about 5 of 6 runs. On this branch it passed every run. Full suite: 576 passed, 5 skipped, 1 xfailed.Found on the way, not fixed here: #86
A reader that rebuilds mid-build, and reads nothing torn, publishes a packed set that looks complete but predates the writer's later records. The writer clears the cache only once per write session, so later fresh processes are served that set. That is a correctness gap in what may be published, separate from #85 (reads raising). It is filed as #86 with options, and pinned by a strict
xfailtest here.Remaining edge case
Two writers racing
put_recordanddelete_recordon the same id can still leave a meta without a vector. Reads now skip that record with a warning instead of raising. Until the record is rewritten or deleted, that corpus's matrix is not published to the packed cache, so each fresh process rebuilds it once.Dependents
fleet_dependentslistsraglabandtruffle. raglab: 49 passed against this branch (it usesCorpusStore.memory()). truffle was out of scope for this run and was not tested. The change keeps every public name and signature. The only behaviour changes are that a racing read no longer raises and that per-record files are replaced atomically.Review
An independent refute-review agent found one blocker, which is fixed in the second commit.
_AtomicFilesbuilt the write path withPath(root, key). For an absolute key (an artifact id can be one, e.g. inlinks), that overwrote a file outside the store, while reads still looked under the root. Its should-fix items are also applied: a permanently unreadable record no longer forces a full re-read on every search in the process, and the stress test is strengthened, since it used to catch master only about 1 run in 5. It confirmed that the on-disk bytes are identical todol.JsonFilesoutput, that mapping semantics (in,get,pop,len, KeyError) are unchanged, and that no path caches or publishes an incomplete set. Not changed: temp files left by a crash between write and replace are hidden but never swept; no fsync per record (by design, see above).🤖 Generated with Claude Code