Conversation
correct() read the record with no lock, appended provenance under one lock acquisition, and persisted the stale snapshot with a second, so a concurrent correct could land in between and have its changes and provenance silently dropped. delete() had the same unlocked find-invalidate-write shape and could clobber a record another writer had just updated. Both now hold the store lock across the whole read-modify-write and persist through the new _write_locked helper, which reuses the existing write machinery without re-acquiring the lock — the public write() still takes the lock itself, so external callers are unchanged and nothing deadlocks.
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.
Fixes #16
What was wrong
Store.correcthad a TOCTOU race: it read the record viaself.find(name)while holding no lock, took the store lock only for the provenance append, and
then called
self.write(), which acquires a second, independent lock topersist what was by then a stale snapshot. A concurrent writer — another
correct, or any pass mutating the same memory — could land between thefind()and the finalwrite(), andcorrect()would silently overwrite itschanges: lost edits, and lost provenance.
delete()has the same shape — anunlocked
find, an in-memory invalidate, then a separately lockedwrite—so a delete racing a writer could clobber freshly appended provenance or
write stale content over another writer's update.
The fix
correctanddeletenow take the store lock once and hold it across thewhole read-modify-write: the
find, every mutation, and the persist._write_lockedhelper that reuses theexisting write machinery (validate, canonicalise dates, write, reproject)
without re-acquiring the lock, so a caller that already holds the lock
cannot double-lock or deadlock. The public
write()still acquires the lockitself and delegates to the same helper, so external callers are unchanged.
Tests
tests/system/test_write_races.pyspawns real OS processes (multiprocessingwith the
spawnstart method) against one shared memory:test_concurrent_corrects_keep_every_provenanceruns four correctors, 12provenance excerpts each, and pins the accumulation invariant: every one of
the 48 excerpts is present in the final record and
len(provenance) == 48— nothing dropped, nothing truncated.test_delete_racing_corrects_never_ends_activeruns a deleter against thesame four correctors and pins delete's legal terminal state: whatever the
interleaving, the record never ends active — it is invalid with
invalid_atset, and every worker exits cleanly.Verification
Verification (Linux, Python 3.12, WSL, from this branch):
uv run pytest -q: ............................................. [ 18%]
........................................................................ [ 36%]
........................................................................ [ 55%]
........................................................................ [ 73%]
........................................................................ [ 92%]
............................ [100%]
390 passed in 47.12s
w s l : : 纇Km0R l o c a l h o s t 鉔tM憂FO*g\曄P0R W S L0N A T !j_N剉 W S L N/e c l o c a l h o s t 鉔t0
uv run ruff check .: All checks passed!
w s l : : 纇Km0R l o c a l h o s t 鉔tM憂FO*g\曄P0R W S L0N A T !j_N剉 W S L N/e c l o c a l h o s t 鉔t0
uv run mypy: Success: no issues found in 66 source files
w s l : : 纇Km0R l o c a l h o s t 鉔tM憂FO*g\曄P0R W S L0N A T !j_N剉 W S L N/e c l o c a l h o s t 鉔t0