rbrooks/Circa

Watch

Fork 0

Keyset cursor uses non-unique created_at and can skip rows #88

New issue

Closed

opened 2026-07-28 06:00:30 +00:00 by claude-bot · 1 comment

claude-bot commented

2026-07-28 06:00:30 +00:00

Severity: MEDIUM (correctness at scale)

The bug

The keyset cursor filters with Photo.created_at > ref.created_at ordered by created_at alone
(backend/app/repositories/photos.py:49-54; same pattern in repositories/jobs.py:36-39).

created_at is not unique — it is datetime.utcnow() assigned per row, and bulk ingest
assigns many in rapid succession. Any tie spanning a page boundary drops the tied rows from all
subsequent pages. Ordering with no tiebreak is also nondeterministic between requests.

Why it matters

Bulk-ingested collections are exactly where ties occur, and a reviewer paging a 30,000-photo grid
would never notice the skipped photos — they would simply never appear. Silent data omission in
the primary browse path.

Fix

Order by (created_at, id) and filter with a tuple comparison:

.where(tuple_(Photo.created_at, Photo.id) > (ref.created_at, ref.id))
.order_by(Photo.created_at, Photo.id)

SQLAlchemy's tuple_() works on SQLite, and the (collection_id, created_at) index from the
index issue still serves it. Apply the same fix to JobRepository.

Each page also currently spends an extra query resolving the cursor id to a row — worth folding in.

Done when

Pagination is stable and complete across ties
Job listing has the same fix
A test bulk-inserts rows with identical timestamps and asserts every row is returned exactly
once across pages

References

backend/app/repositories/photos.py:49-54
backend/app/repositories/jobs.py:36-39

## Severity: MEDIUM (correctness at scale) ## The bug The keyset cursor filters with `Photo.created_at > ref.created_at` ordered by `created_at` alone (`backend/app/repositories/photos.py:49-54`; same pattern in `repositories/jobs.py:36-39`). `created_at` is **not unique** — it is `datetime.utcnow()` assigned per row, and bulk ingest assigns many in rapid succession. Any tie spanning a page boundary drops the tied rows from all subsequent pages. Ordering with no tiebreak is also nondeterministic between requests. ## Why it matters Bulk-ingested collections are exactly where ties occur, and a reviewer paging a 30,000-photo grid would never notice the skipped photos — they would simply never appear. Silent data omission in the primary browse path. ## Fix Order by `(created_at, id)` and filter with a tuple comparison: ```python .where(tuple_(Photo.created_at, Photo.id) > (ref.created_at, ref.id)) .order_by(Photo.created_at, Photo.id) ``` SQLAlchemy's `tuple_()` works on SQLite, and the `(collection_id, created_at)` index from the index issue still serves it. Apply the same fix to `JobRepository`. Each page also currently spends an extra query resolving the cursor id to a row — worth folding in. ## Done when - [ ] Pagination is stable and complete across ties - [ ] Job listing has the same fix - [ ] A test bulk-inserts rows with identical timestamps and asserts every row is returned exactly once across pages ## References - `backend/app/repositories/photos.py:49-54` - `backend/app/repositories/jobs.py:36-39`

claude-bot added this to the v0.2.0 milestone

2026-07-28 06:00:30 +00:00

claude-bot added the

bug

backend

labels

2026-07-28 06:00:30 +00:00

claude-bot referenced this issue

2026-07-28 06:07:50 +00:00

Performance optimization and progress tracking at collection scale #48

rbrooks referenced this issue from a commit

2026-08-02 05:54:55 +00:00

Stop pagination losing photographs

rbrooks closed this issue

2026-08-02 05:54:55 +00:00

claude-bot commented

2026-08-02 05:55:10 +00:00

Author

Fixed in 9288798.

Done when

Pagination is stable and complete across ties
Job listing has the same fix
A test bulk-inserts rows with identical timestamps and asserts every row is returned exactly once across pages

The severity is worse than MEDIUM. I wrote the reproduction before the fix, and the failure is not "rows are skipped" — it is that pagination terminates. With ten photos sharing a timestamp and a page size of one:

page 1 -> 1 photo, cursor = that photo
page 2 -> WHERE created_at > cursor.created_at  ->  0 rows, cursor = None

All ten share the timestamp, so the strict > excludes the entire tied group including the nine never seen. One photo was returned; nine vanished from every page. A batch ingested in one go — which is the normal case for this project, a box of scans at a time — could contribute a single photo to the whole browse grid.

And nothing surfaces it. The pages look complete, the missing photographs are simply absent, and there is no error, no gap, no count that disagrees. This is silent data omission in the primary browse path, on the most common ingest shape.

The fix is the row-value comparison from the issue, (created_at, id), ordered the same way. SQLite has supported row values since 3.15 (3.50.4 here) and the existing (collection_id, created_at) index still serves the ordering. JobRepository gets the same fix with the comparison reversed, since that listing is newest-first — worth calling out because it is exactly the detail that gets copied wrong when one of two call sites is fixed. Ties are if anything likelier there: a bulk rerun enqueues a job per photo in one loop.

One suggestion I did not take. The issue notes each page spends an extra query resolving the cursor id and suggests folding it in. I kept the opaque id. Resolving it is one indexed lookup per page, and in exchange the cursor is still scoped to the collection on the way in (#70) and a client cannot hand back a position we never issued. An encoded (timestamp, id) cursor would be a client-constructible position into the table. That seemed the wrong trade for one lookup, but it is a judgement call and easy to revisit if paging ever shows up in a profile.

Tests page every listing to exhaustion and compare the multiset of ids against what was inserted — at page sizes 1, 2, 3 and 5, filtered and unfiltered, both repositories. Asserting that a page "looks right" is precisely the check that missed this for the life of the code. The walk is bounded so a cursor that fails to advance fails the test rather than hanging it.

I also swept for other keyset cursors on non-unique columns; there are none.

Fixed in 9288798. **Done when** - [x] Pagination is stable and complete across ties - [x] Job listing has the same fix - [x] A test bulk-inserts rows with identical timestamps and asserts every row is returned exactly once across pages **The severity is worse than MEDIUM.** I wrote the reproduction before the fix, and the failure is not "rows are skipped" — it is that pagination *terminates*. With ten photos sharing a timestamp and a page size of one: ``` page 1 -> 1 photo, cursor = that photo page 2 -> WHERE created_at > cursor.created_at -> 0 rows, cursor = None ``` All ten share the timestamp, so the strict `>` excludes the entire tied group including the nine never seen. **One photo was returned; nine vanished from every page.** A batch ingested in one go — which is the normal case for this project, a box of scans at a time — could contribute a single photo to the whole browse grid. And nothing surfaces it. The pages look complete, the missing photographs are simply absent, and there is no error, no gap, no count that disagrees. This is silent data omission in the primary browse path, on the most common ingest shape. **The fix** is the row-value comparison from the issue, `(created_at, id)`, ordered the same way. SQLite has supported row values since 3.15 (3.50.4 here) and the existing `(collection_id, created_at)` index still serves the ordering. `JobRepository` gets the same fix with the comparison reversed, since that listing is newest-first — worth calling out because it is exactly the detail that gets copied wrong when one of two call sites is fixed. Ties are if anything likelier there: a bulk rerun enqueues a job per photo in one loop. **One suggestion I did not take.** The issue notes each page spends an extra query resolving the cursor id and suggests folding it in. I kept the opaque id. Resolving it is one indexed lookup per page, and in exchange the cursor is still scoped to the collection on the way in (#70) and a client cannot hand back a position we never issued. An encoded `(timestamp, id)` cursor would be a client-constructible position into the table. That seemed the wrong trade for one lookup, but it is a judgement call and easy to revisit if paging ever shows up in a profile. **Tests** page every listing to exhaustion and compare the multiset of ids against what was inserted — at page sizes 1, 2, 3 and 5, filtered and unfiltered, both repositories. Asserting that a page "looks right" is precisely the check that missed this for the life of the code. The walk is bounded so a cursor that fails to advance fails the test rather than hanging it. I also swept for other keyset cursors on non-unique columns; there are none.

rbrooks referenced this issue from a commit

2026-08-03 15:59:10 +00:00

Index the photo list for the order it is actually read in

claude-bot referenced this issue

2026-08-03 15:59:31 +00:00

Photo list query has no index for its sort shape (436x) #84

claude-bot referenced this issue

2026-08-03 20:46:03 +00:00

Photo browser cannot reach past the first 100 photos #94

rbrooks referenced this issue from a commit

2026-08-03 21:39:45 +00:00

Reach every photograph, and filter and sort the collection