GetChangesSince's millisecond cursor comparison can miss a same-millisecond write

[Yaraslaut]

Problem

A model that answers a "changes since" query by filtering on a millisecond-resolution timestamp column with a strict updatedAtMs > since comparison can silently miss a write that lands in the same millisecond as the cursor value from the previous poll. This is plausible whenever poll → write → poll executes within one clock tick (a fast machine, or a loaded CI runner). The write genuinely happened after the cursor was captured in wall-clock terms, but > treats "equal" as "not new" and excludes it.

Reproducer

// asOf captured before the query runs, so a racing write is lost across
// two polls rather than duplicated -- deliberate and correct on its own.
auto asOf = nowMs();
auto rows = query.Where("updatedAtMs", ">", since).All();

1. Poll once with an empty since → capture asOf1 as the next cursor.
2. Perform a write whose timestamp is nowMs() at that instant.
3. Poll again with since = asOf1.

If the write's timestamp equals asOf1 (same millisecond), step 3's strict > excludes it from the result, even though the write happened strictly after step 1 captured its cursor. This is a boundary/granularity bug, not a logic error in the surrounding design — capturing the cursor before running the query is the right call to avoid losing a genuinely-concurrent write; the bug is specifically that the comparison operator can't distinguish "same instant" from "not yet happened" at millisecond resolution.

Desired behavior

A "since" cursor comparison should never silently drop a write that happened after the cursor was captured, even when the write's timestamp collides with the cursor at the timestamp's own resolution.

Suggested fix direction

Neither > (under-inclusive, this bug) nor >= (over-inclusive: a same-millisecond write from a different concurrent actor would then show up twice, once on each of two consecutive polls using it as their cursor) is unconditionally correct at millisecond granularity. The real fix needs either higher-resolution/monotonic ordering (microsecond timestamps, or a per-write sequence number) or an explicit (timestamp, sequence) cursor pair so two writes in the same millisecond stay strictly orderable. Cursoring on a strictly-increasing, collision-free key (e.g. an auto-increment id) instead of a wall-clock timestamp sidesteps the problem entirely for any "changes since" shape that can be modeled as an append-only log.

Source: docs/findings/036-getchangessince-millisecond-cursor-boundary-race.md

[Yaraslaut]

Additional context (folded in from the now-removed docs/findings/036 file)

examples/bookmarks/src/models/bookmark_model.cpp:409-424's BookmarkModel::execute(const GetChangesSince&) is the concrete instance this was found against — BookmarkPresenter::getChangesSince returns only bookmarks touched after the given instant, all three backend modes (examples/bookmarks/tests/test_bookmark_presenter.cpp, Mode::Local generator case) fails intermittently under CI load, not deterministically locally, consistent with the narrow same-millisecond timing window described above.

Working precedent already in the repo: rung 3/polls' own event log, PollModel::execute(GetEventsSince&) (examples/polls/src/models/poll_model.cpp:627-663), already avoids this exact class of bug by cursoring on PollEventRecord::id — a ServerSideAutoIncrement primary key — instead of a timestamp: Where(id, ">", *action.lastEventId), ascending. An auto-increment id is inherently collision-free and strictly orderable across writes regardless of clock resolution. GetChangesSince returning full row summaries (not an append-only event log) makes porting the identical id-cursor scheme non-trivial — it would need to cursor on something like max(id) at the time of the previous poll per bookmark, or move to an outbox/event-log shape of its own — but GetEventsSince is a concrete, working example of "how this codebase already solves the identical ordering problem," not merely a hypothetical direction.

Still open, not yet attempted:

• Design how GetChangesSince's bulk-summary shape (not an append-only log) could adopt an id/sequence-based cursor instead of a timestamp.
• No dedicated regression test forces the same-millisecond collision deterministically (e.g. by overriding the ladder's injectable clock, examples/common/clock.hpp's ScopedClockOverride, to freeze nowMs() across the create-then-poll sequence) — the existing test relies on incidental timing and can pass on a lucky run.

[Yaraslaut]

Fixed on the `application-ladder` branch (commit 682174d), merged directly per the finding's own "suggested fix direction" — a compound (timestampMs, lastId) cursor:

• GetChangesSince.since / GetChangesSinceResult.asOf become a ChangesCursor (examples/bookmarks/include/bookmarks/core/types.hpp) instead of a bare Timestamp.
• The query becomes updatedAtMs > since OR (updatedAtMs = since AND id > lastId), ordered (updatedAtMs, id) ascending — a same-millisecond write with a higher id is now included, and the row that established lastId is never re-delivered on a later poll at the same instant.
• Two new regression tests in test_bookmark_model.cpp, following the same frozen-single-instant idiom as the existing BulkEdit same-millisecond regression test. Verified the "does not miss a write" case fails against the pre-fix strict-> query (0 == 1) and passes after the fix.
• Full bookmarks suite (121 cases, 826 assertions) plus polls/pastebin suites pass with no regressions.

Deliberately not the full id/sequence-cursor redesign this finding's own writeup flagged as "still open, not yet attempted" (porting rung 3's poll_events/GetEventsSince event-log pattern to GetChangesSince's bulk-summary shape) — that's a larger design change to the wire contract's return shape, tracked separately if ever pursued. This closes the boundary-condition bug itself.

docs/findings/036-getchangessince-millisecond-cursor-boundary-race.md's content is fully superseded by this fix; no further action needed there.

🤖 Generated with Claude Code