fix(db): keep reading progress steady for sessions that moved nothing, and concurrent replays - #50
Merged
Conversation
…gress A reading session reporting only time spent (no page, percentage or locator) still re-projected the whole progress row. A reader left open on a book for a second therefore bumped updated_at to the upload time and sent the book to the top of Keep Reading, un-finished a completed book, and put an unread book on the in-progress shelf at page 0. The fold now builds the row only from sessions that move the reader: a position, or a completion. started_at and updated_at are taken over those sessions too, so a stray open neither backdates a pass nor reorders the shelves. Time-only sessions are still stored and still count towards reading statistics, which read the log directly. The fold's progress output becomes a three-way projection: a row, no row (after a reset), or leave the row as it is. The last case cannot fall back to deleting, because importing a reading-progress export writes the row directly, and a time-only session must not erase a row the log does not explain.
A client that sent its session queue twice at once got a 500 on the second request. The replay check reads before it writes, so it cannot see a sibling request's uncommitted row: both requests passed it, and on PostgreSQL the second insert then hit the reading_sessions primary key. The insert now uses ON CONFLICT (id) DO NOTHING and reports a skipped row as a duplicate, which is what a sequential replay already gets. It goes through exec rather than exec_with_returning, because only exec reports an insert that did nothing as a conflict. SQLite never reaches the insert: the losing transaction is refused the write lock instead. record_session now retries the whole transaction on SQLite lock contention, a bounded number of times with backoff, so the replay re-reads, finds the committed row and comes back as a duplicate.
API contract changesCompared against Report |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs in
POST /api/v1/reading-sessions, reported from a production session against 2.6.2.A session that moved nothing rewrote reading progress
An iPad left open on a book for under a second queued a session with time spent and no position, and uploaded it the next morning. The server re-projected the progress row from it:
updated_atjumped to the upload time, sending the book from 9th to 1st in Keep Reading with its page unchanged.completedand put it back in Keep Reading.The fold now builds the row only from sessions that move the reader: a page, percentage or locator, or a completion.
started_atandupdated_atcome from those sessions too. Time-only sessions are still stored and still count in reading statistics, which read the session log directly.The fold's progress output is now three-way: a row, no row (after a reset), or leave the row alone. The third case cannot fall back to deleting, because importing a reading-progress export writes
read_progressdirectly, and a time-only session must not erase a row the log does not explain.Every consumer of
read_progress.updated_at(shelf ordering, series last read, KomgareadDate/lastModified, OPDS2, plugin sync staleness) reads it as "when the position or state last changed"; none relied on time-only sessions bumping it. Legacy surfaces (v1 progress routes, Komga, KOReader, OPDS) always send a page, so they are unaffected.Rows already bumped by this bug are not repaired by a migration: a global refold would clobber imported progress. They correct themselves on the book's next session that moves the reader.
A concurrent replay of the same session id returned 500
The same queue was sent twice at once. The replay check reads before it writes, so neither request saw the other's uncommitted row, and on PostgreSQL the second insert failed with
duplicate key value violates unique constraint "reading_sessions_pkey"(the production log line).ON CONFLICT (id) DO NOTHINGand reports a skipped row as a duplicate, which is what a sequential replay already gets. It usesexecrather thanexec_with_returning, since onlyexecreports a do-nothing insert as a conflict.record_sessionretries the whole transaction on SQLite lock contention (bounded, with backoff, logged atwarn), so the replay re-reads and comes back as a duplicate.Tests
started_atuntouched, a pass of only time-only sessions leaves the row alone, a reset stays a reset, a positioned session still wins. Theupdated_attest now states the new rule.main, plus one pinning the boundary where a measured session absorbs position writes and does move the row.database is lockedon SQLite; with the retry disabled the SQLite case fails again.