fix(evidence): hold flock + derive offset from fd to close chain-corruption - #327
Conversation
…uption
The append_event durability contract said `appended: True` meant the record
was fsync'd and `journalOffset` was the real byte offset — but the whole
read-tip → compute-chain → write → publish-offset window ran with NO lock.
Two concurrent writers observed the same `previous` tip, computed two records
that both linked to that point via `previousDigest`, and both wrote — the
chain silently BRANCHED and `verify_journal` would then break at the second
record. Same class as the ledger fix in
SocioProphet/evidence-intake-kernel/ledger.py, which wraps the whole
compute+write in `fcntl.flock(LOCK_EX)`. That's the correct pattern; this
module shipped without it.
A separate TOCTOU: `journal_offset = journal_path.stat().st_size` was taken
BEFORE the write, in a syscall separate from the append. Under concurrency
another writer can extend the file between the stat and the write, so the
returned offset points into the middle of another record — every downstream
`cairn://<task>/<offset>` handle then dereferences garbage.
Both defects fixed:
- Single `journal_path.open("a+")`. `flock(LOCK_EX)` covers the tip read,
the chain compute, the write, the fsync, and the offset publish. The
`O_APPEND` semantics stay as belt-and-suspenders.
- Tip read is inlined under the lock (calling `tip_digest()` would open its
own handle and race). The `verify_journal` reader is unchanged — it reads
the whole file, so it doesn't need the lock, but a follow-up could hold
a shared lock for on-fs consistency during writes.
- `journal_offset = handle.tell()` after `seek(0, os.SEEK_END)` — the offset
is derived from the fd inside the lock, so nothing can extend the file
between measurement and write.
New test file `tests/test_evidence_journal_concurrency.py`:
- 20 threads × 5 events = 100 concurrent appends → chain verifies clean,
count matches (blows up under the old code)
- Every returned offset points at the start of its record (pins the TOCTOU
fix — with the old stat-before-write two returns could share an offset)
- Single-writer path still works, empty-journal bootstraps to genesis
Also documented on `_strip_prefix` that accepting bare hex is a
backwards-compat concession for legacy records — future writers must
emit the `sha256:` form (which `append_event` does).
4/4 new tests pass; existing tests unaffected.
Found in the estate-wide adversarial review pass (evidence-intake-kernel's
ledger.py is the reference implementation for the pattern).
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes evidence journal append correctness under concurrency by serializing the “read tip → compute chain → append → fsync → publish offset” window and eliminating a TOCTOU offset derivation race.
Changes:
- Add
fcntl.flock(LOCK_EX)around tip read + chain compute + write + fsync inappend_event. - Derive
journalOffsetfrom the locked file descriptor viaseek(..., SEEK_END)+tell()(instead ofPath.stat()). - Add concurrency-focused tests that pin chain integrity and returned offsets under multi-threaded appends.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| tests/test_evidence_journal_concurrency.py | Adds tests to reproduce/pin chain-branching and offset TOCTOU regressions under concurrent appends. |
| evidence/append_event_stub.py | Serializes append window with flock and derives offsets from the FD inside the lock. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Bulk reply on Copilot findings (all 7 threads resolved):
|
fix(evidence): hold flock + derive offset from fd to close chain-corruption
The append_event durability contract said
appended: Truemeant the recordwas fsync'd and
journalOffsetwas the real byte offset — but the wholeread-tip → compute-chain → write → publish-offset window ran with NO lock.
Two concurrent writers observed the same
previoustip, computed two recordsthat both linked to that point via
previousDigest, and both wrote — thechain silently BRANCHED and
verify_journalwould then break at the secondrecord. Same class as the ledger fix in
SocioProphet/evidence-intake-kernel/ledger.py, which wraps the whole
compute+write in
fcntl.flock(LOCK_EX). That's the correct pattern; thismodule shipped without it.
A separate TOCTOU:
journal_offset = journal_path.stat().st_sizewas takenBEFORE the write, in a syscall separate from the append. Under concurrency
another writer can extend the file between the stat and the write, so the
returned offset points into the middle of another record — every downstream
cairn://<task>/<offset>handle then dereferences garbage.Both defects fixed:
journal_path.open("a+").flock(LOCK_EX)covers the tip read,the chain compute, the write, the fsync, and the offset publish. The
O_APPENDsemantics stay as belt-and-suspenders.tip_digest()would open itsown handle and race). The
verify_journalreader is unchanged — it readsthe whole file, so it doesn't need the lock, but a follow-up could hold
a shared lock for on-fs consistency during writes.
journal_offset = handle.tell()afterseek(0, os.SEEK_END)— the offsetis derived from the fd inside the lock, so nothing can extend the file
between measurement and write.
New test file
tests/test_evidence_journal_concurrency.py:count matches (blows up under the old code)
fix — with the old stat-before-write two returns could share an offset)
Also documented on
_strip_prefixthat accepting bare hex is abackwards-compat concession for legacy records — future writers must
emit the
sha256:form (whichappend_eventdoes).4/4 new tests pass; existing tests unaffected.
Found in the estate-wide adversarial review pass (evidence-intake-kernel's
ledger.py is the reference implementation for the pattern).