Context
From the review of #4127 (head 2fb24a89): events_after now locates the cursor with bisect_right over sequence (loopx/chat_store.py:1346-1352), replacing a full linear scan.
Problem
bisect correctness depends on an invariant that is structurally guaranteed but nowhere stated in code: flush_events allocates sequence monotonically under the file lock (chat_store.py:1311-1315, starting from rows[-1].sequence and incrementing) over an append-only JSONL, so file row order is always ascending by sequence. Gaps are fine (compact drops old prefixes; batches may skip), but descending order is not.
If someone later changes flush to parallel allocation, inserts historical rows, or rewrites the file in a different order, bisect will silently return wrong results — no exception, no test failure.
Suggested fix
- Add a comment at
flush_events (or _event_rows_locked) declaring the monotonicity invariant that events_after's bisect relies on, or
- add a cheap debug-level assertion in
events_after spot-checking order on the probed points.
Carried from two prior review rounds of #4127; non-blocking. See #4127 (comment).
Context
From the review of #4127 (head
2fb24a89):events_afternow locates the cursor withbisect_rightoversequence(loopx/chat_store.py:1346-1352), replacing a full linear scan.Problem
bisectcorrectness depends on an invariant that is structurally guaranteed but nowhere stated in code:flush_eventsallocatessequencemonotonically under the file lock (chat_store.py:1311-1315, starting fromrows[-1].sequenceand incrementing) over an append-only JSONL, so file row order is always ascending bysequence. Gaps are fine (compact drops old prefixes; batches may skip), but descending order is not.If someone later changes flush to parallel allocation, inserts historical rows, or rewrites the file in a different order,
bisectwill silently return wrong results — no exception, no test failure.Suggested fix
flush_events(or_event_rows_locked) declaring the monotonicity invariant thatevents_after's bisect relies on, orevents_afterspot-checking order on the probed points.Carried from two prior review rounds of #4127; non-blocking. See #4127 (comment).