Skip to content

Hold the store lock across find and write in correct and delete - #37

Open
HUAN2022A wants to merge 1 commit into
tigerless-labs:mainfrom
HUAN2022A:fix/correct-delete-lock-race
Open

HUAN2022A wants to merge 1 commit into
tigerless-labs:mainfrom
HUAN2022A:fix/correct-delete-lock-race

Conversation

@HUAN2022A

Copy link
Copy Markdown

Fixes #16

What was wrong

Store.correct had a TOCTOU race: it read the record via self.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 to
persist what was by then a stale snapshot. A concurrent writer — another
correct, or any pass mutating the same memory — could land between the
find() and the final write(), and correct() would silently overwrite its
changes: lost edits, and lost provenance. delete() has the same shape — an
unlocked find, an in-memory invalidate, then a separately locked write
so a delete racing a writer could clobber freshly appended provenance or
write stale content over another writer's update.

The fix

  • correct and delete now take the store lock once and hold it across the
    whole read-modify-write: the find, every mutation, and the persist.
  • The persist step goes through a new _write_locked helper that reuses the
    existing 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 lock
    itself and delegates to the same helper, so external callers are unchanged.

Tests

tests/system/test_write_races.py spawns real OS processes (multiprocessing
with the spawn start method) against one shared memory:

  • test_concurrent_corrects_keep_every_provenance runs four correctors, 12
    provenance 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_active runs a deleter against the
    same four correctors and pins delete's legal terminal state: whatever the
    interleaving, the record never ends active — it is invalid with
    invalid_at set, 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

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.
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.

Store.correct() has a TOCTOU race between find and write

1 participant