Fix #107: atomic pretool latch + atomic writes in SeenStore - #123
Conversation
The PreToolUse latch was a check-then-set across two file operations, so N parallel hook processes at session start could all observe pretool_fired == False and all proceed with the full retrieval — four sqlite connections, four Ollama embeds, and four duplicate injections. The _save path also rewrote the whole file from a stale in-memory snapshot with a plain write_text, so process P4's mark_seen would clobber the entries P1-P3 had recorded and re-inject them next turn. Replace the latch with an atomic O_CREAT|O_EXCL claim on a sibling sentinel (context_seen_<sess>.pretool). Callers on the concurrent path now use SeenStore.try_claim_pretool_fired(), which returns True to exactly one process per session; the older pretool_fired / mark_pretool_fired pair is retained for compatibility and now routes through the same sentinel. Rewrite _save to write to a temp sibling then os.replace, matching the existing _atomic_write idiom in install_hooks.py, and rewrite bump_turn / mark_seen to re-read the on-disk snapshot before merging so a concurrent writer's entries are preserved. prune_stale's file regex now also matches the .pretool sentinel so long-lived state directories don't accumulate them.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 Claude BugBot Analysis
Found 1 potential bug in this PR.
medium: 1
The concurrency fix in context_seen.py correctly merges the seen dict across concurrent writers but stamps newly-marked entries with a possibly-stale local turn number instead of the freshly-read/max turn, which can cause premature reinjection under concurrent hook invocations.
mark_seen already re-reads the on-disk snapshot so a concurrent writer's `seen` entries survive, but it was still stamping newly-marked ids with the process's local `_data["turn"]`. If another process bumped the turn between our snapshot and our mark_seen, our entry landed with a turn value below the file's true current turn, so filter_unseen's `(turn - last) > reinject_turns` gap opened one turn early and the item we just marked got re-injected prematurely. Stamp both the merged entries and the top-level turn with `max(latest_turn, local_turn)` so the entry's `last_injected_turn` can never be strictly below the file's current turn. Regression test pins the exact scenario (A bumps to 1, B concurrently bumps the file to 2, A.mark_seen must stamp with 2 so a reader with reinject_turns=1 does not re-inject).
There was a problem hiding this comment.
🔴 Claude BugBot Analysis
Found 1 potential bug in this PR.
medium: 1
The previously reported mark_seen stale-turn stamping bug is fixed (entries are now stamped with max(latest, local) turn), but the newly added atomic-write helper in _save() uses a fixed tmp filename shared by all concurrent writers to the same session file, which can cause one process's write to be silently dropped under the very concurrency this PR is meant to harden against.
…ch other The atomic-write helper still derived its tmp name from the session path (`self._path.with_suffix(self._path.suffix + ".tmp")`), giving every writer to the same session the same tmp filename. When two processes' _save() calls interleaved (the very scenario this PR is meant to harden against), one could truncate the other's in-flight tmp before the first os.replace ran, and the second os.replace would then fail on the now-missing shared tmp — silently, because the BaseException guard swallows FileNotFoundError. Result: one writer's data lost. Use tempfile.mkstemp(prefix=<path>., suffix=.tmp, dir=self._dir) so each _save call gets its own tmp path, mirroring the existing pattern at runtime/session_marker.py:63. On failure the tmp is unlinked; on success os.replace consumes it. prune_stale's _FILE_RE now also matches leftover `.json.<random>.tmp` siblings so a hard-killed process between mkstemp and replace/unlink doesn't leak tmps forever. Regression test spies on os.replace and pins that two back-to-back _save calls hand it two distinct source paths.
There was a problem hiding this comment.
🟢 Claude BugBot Analysis
The diff replaces the racy pretool_fired check-then-set with an atomic O_CREAT|O_EXCL sentinel claim, switches _save() to a per-call-unique tempfile.mkstemp-based temp file, and fixes mark_seen to stamp entries with max(latest_turn, local_turn); all changes were traced through and are internally consistent with no new defects found.
No bugs were detected in this PR.
2 participants
Closes #107
Summary
The PreToolUse "one real firing per session" latch was a check-then-set across two file operations (
SeenStore.pretool_fired()followed bySeenStore.mark_pretool_fired()incontextual_inject.py:110-113), and the installed PreToolUse matcher is unscoped — every tool call fires a hook process. A parallel-tool-call batch at session start therefore had N hook processes all observepretool_fired == Falseand all proceed to run the retrieval path (N sqlite connections, N Ollama embeds, N duplicate injections). Additionally, every_saverewrote the whole file from a stale in-memory snapshot with a plainwrite_text, so process P4'smark_seenwould overwrite the entries P1-P3 recorded and those memories would re-inject next turn.This PR applies exactly the fix the issue names:
O_CREAT|O_EXCLclaim on a sibling sentinel file (context_seen_<sess>.pretool).SeenStore.try_claim_pretool_fired()returns True to exactly one process per session; the caller incontextual_inject.pynow uses it, so late-arriving hook processes short-circuit before any DB/embedder work. The oldpretool_fired/mark_pretool_firedpair is retained for compatibility and routes through the same sentinel._save→ temp file +os.replace, matching the existing_atomic_writeidiom ininstall_hooks.py:301-310(which the issue explicitly cites as "unused here").bump_turn/mark_seen→ re-read the on-disk snapshot before merging, so a concurrent writer's entries are preserved rather than clobbered.prune_stale's file regex now also matches the new.pretoolsentinel so long-lived state directories don't accumulate them.Regression tests cover: (a) exactly one of four parallel
try_claim_pretool_fired()callers returns True, (b)_saveleaves no temp sibling behind and produces well-formed JSON, (c) two writers with disjoint mark_seen entries both survive on disk, (d)prune_staleremoves stale sentinels. All 12test_context_seen.pytests, 85tests/hooks/tests, and 540tests/services/tests pass.Confidence
~90%. The fix is exactly the one the issue prescribes (atomic filesystem latch + re-read+merge + temp+
os.replace), scoped to two files plus tests, and preserves the existing public API. The one design choice — a new.pretoolsentinel file rather than an in-JSON flag — is required for the atomicity guarantee (O_CREAT|O_EXCLon a whole-file overwrite is not meaningful) and prune_stale is updated to keep the state directory tidy.This PR was generated by a scheduled Claude routine that scans open issues and opens PRs only when confidence is ≥ 90%.
Generated by Claude Code