From 2e454cc8460c2b0707efdac9a0e0f0a52e8f0056 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Tue, 20 Oct 2026 15:44:22 +0000 Subject: [PATCH] fix(koreader): rank synced books by when the device read them Stamp reading_progress.last_read_at with the device's reading time instead of the server's write time. A sweep pushes a whole shelf at once, so sorting by last read came out in processing order. Keep the later of the stored and pushed times, so a late or offline push never sinks a newer read from another client. Fixes #1433 --- .../koreader/koreader.repository.test.ts | 75 +++++++++++++------ .../modules/koreader/koreader.repository.ts | 10 ++- .../modules/koreader/koreader.service.test.ts | 32 +++++++- .../src/modules/koreader/koreader.service.ts | 4 +- .../koreader-progress-position.e2e-spec.ts | 49 ++++++++++++ 5 files changed, 145 insertions(+), 25 deletions(-) diff --git a/server/src/modules/koreader/koreader.repository.test.ts b/server/src/modules/koreader/koreader.repository.test.ts index e1294871c..953d6ed43 100644 --- a/server/src/modules/koreader/koreader.repository.test.ts +++ b/server/src/modules/koreader/koreader.repository.test.ts @@ -1296,46 +1296,79 @@ describe('KoreaderRepository', () => { // freezes on purpose so it stays a "last local write" marker. A KOReader-only reader therefore // kept the timestamp of their first ever sync forever and the sort looked random. describe('last-read timestamps in the compiled SQL', () => { - async function compileUpsert() { + type Query = { sql: string; params: unknown[] }; + + async function compileUpsert(timing: { readAt?: Date; lastReadAt?: Date } = {}) { const { db: capturingDb, queries } = createCapturingDb(); const capturingRepo = new KoreaderRepository(capturingDb as never); - await capturingRepo.upsertReadingProgress({ bookFileId: 44, userId: 12, percentage: 41.25 }); + await capturingRepo.upsertReadingProgress({ bookFileId: 44, userId: 12, percentage: 41.25, ...timing }); expect(queries).toHaveLength(1); return queries[0]!; } - function stampedLastReadAt({ sql: text, params }: { sql: string; params: unknown[] }) { - const match = /"last_read_at" = \$(\d+)/.exec(text); - expect(match).not.toBeNull(); - return new Date(String(params[Number(match![1]) - 1])).getTime(); + function paramTime({ params }: Query, placeholder: string | undefined) { + expect(placeholder).toMatch(/^\$\d+$/); + return new Date(String(params[Number(placeholder!.slice(1)) - 1])).getTime(); } - it('freezes updated_at but advances last_read_at on conflict', async () => { + function insertedTime(query: Query, column: string) { + const match = /^insert into "reading_progress" \(([^)]*)\) values \(([^)]*)\)/.exec(query.sql); + expect(match).not.toBeNull(); + const columns = match![1]!.split(', '); + return paramTime(query, match![2]!.split(', ')[columns.indexOf(`"${column}"`)]); + } + + function conflictLastReadAt(query: Query) { + const match = /"last_read_at" = greatest\("reading_progress"\."last_read_at", (\$\d+)::timestamptz\)/.exec(query.sql); + expect(match).not.toBeNull(); + return paramTime(query, match![1]); + } + + function conflictTextUpdatedAt(query: Query) { + return paramTime(query, /"text_updated_at" = (\$\d+)/.exec(query.sql)?.[1]); + } + + it('freezes updated_at and only ever advances last_read_at on conflict', async () => { const query = await compileUpsert(); expect(query.sql).toContain('"updated_at" = "reading_progress"."updated_at"'); - expect(query.sql).toMatch(/"last_read_at" = \$\d+/); - expect(query.sql).not.toMatch(/"last_read_at" = "reading_progress"\."last_read_at"/); - expect(stampedLastReadAt(query)).not.toBeNaN(); + expect(conflictLastReadAt(query)).not.toBeNaN(); }); - it('stamps last_read_at close to now rather than reusing a stored value', async () => { + // Regression: a sweep pushes a device's whole shelf at once, and stamping each + // write with the server's clock ranked every book in it as read in the same few seconds. + it('stamps last_read_at with when the device read, not when the push arrived', async () => { + const readAt = new Date('2026-03-04T21:15:00.000Z'); + + const query = await compileUpsert({ readAt }); + + expect(insertedTime(query, 'last_read_at')).toBe(readAt.getTime()); + expect(conflictLastReadAt(query)).toBe(readAt.getTime()); + }); + + it('keeps the reading time for last_read_at when the text position is stamped later', async () => { + const readAt = new Date('2026-10-17T06:44:16.000Z'); + const lastReadAt = new Date('2026-10-10T17:00:00.000Z'); + + const query = await compileUpsert({ readAt, lastReadAt }); + + expect(insertedTime(query, 'last_read_at')).toBe(lastReadAt.getTime()); + expect(conflictLastReadAt(query)).toBe(lastReadAt.getTime()); + expect(insertedTime(query, 'text_updated_at')).toBe(readAt.getTime()); + expect(conflictTextUpdatedAt(query)).toBe(readAt.getTime()); + }); + + it("falls back to the write's own time when the device gives none", async () => { const before = Date.now(); const query = await compileUpsert(); const after = Date.now(); - const stamped = stampedLastReadAt(query); - expect(stamped).toBeGreaterThanOrEqual(before); - expect(stamped).toBeLessThanOrEqual(after); - }); - - it('lets the insert branch default last_read_at instead of leaving it unset', async () => { - const { sql: text } = await compileUpsert(); - - expect(text).toContain('"last_read_at"'); - expect(text.indexOf('"last_read_at"')).toBeLessThan(text.indexOf('on conflict')); + for (const stamped of [insertedTime(query, 'last_read_at'), conflictLastReadAt(query)]) { + expect(stamped).toBeGreaterThanOrEqual(before); + expect(stamped).toBeLessThanOrEqual(after); + } }); }); }); diff --git a/server/src/modules/koreader/koreader.repository.ts b/server/src/modules/koreader/koreader.repository.ts index 51beb5103..988b6b408 100644 --- a/server/src/modules/koreader/koreader.repository.ts +++ b/server/src/modules/koreader/koreader.repository.ts @@ -82,6 +82,8 @@ export interface ReadingProgressUpsert { pageNumber?: number | null; /** When the device read to this position; the write's own time when it does not say. */ readAt?: Date; + /** When the reading happened, for last_read_at; defaults to readAt. Never moves the stored value back. */ + lastReadAt?: Date; } export interface DeviceProgressUpsert { @@ -720,11 +722,11 @@ export class KoreaderRepository { } async upsertReadingProgress(entry: ReadingProgressUpsert) { - const { bookFileId, userId, percentage, cfi = null, xpointer = null, pageNumber = null, readAt = new Date() } = entry; + const { bookFileId, userId, percentage, cfi = null, xpointer = null, pageNumber = null, readAt = new Date(), lastReadAt = readAt } = entry; await this.db .insert(schema.readingProgress) - .values({ bookFileId, userId, percentage, cfi, pageNumber, koreaderProgress: xpointer, textUpdatedAt: readAt }) + .values({ bookFileId, userId, percentage, cfi, pageNumber, koreaderProgress: xpointer, textUpdatedAt: readAt, lastReadAt }) .onConflictDoUpdate({ target: [schema.readingProgress.bookFileId, schema.readingProgress.userId], // Deliberately do NOT update updatedAt here. reading_progress.updatedAt must only @@ -747,6 +749,10 @@ export class KoreaderRepository { koboContentSourceProgressPercent: null, textUpdatedAt: readAt, updatedAt: sql`"reading_progress"."updated_at"`, + // The device's reading time, not the write's: a sweep pushes a whole shelf at once and + // an offline book arrives on reconnection, so the write's own time ranked them all as + // just read. Held to the stored value so an older push never sinks a newer read elsewhere. + lastReadAt: sql`greatest("reading_progress"."last_read_at", ${lastReadAt.toISOString()}::timestamptz)`, }, }); } diff --git a/server/src/modules/koreader/koreader.service.test.ts b/server/src/modules/koreader/koreader.service.test.ts index a6cae3255..f94a081bd 100644 --- a/server/src/modules/koreader/koreader.service.test.ts +++ b/server/src/modules/koreader/koreader.service.test.ts @@ -421,9 +421,11 @@ describe('KoreaderService', () => { }); expect(mockRepo.upsertDeviceProgress).toHaveBeenCalled(); - const written = mockRepo.upsertReadingProgress.mock.calls[0]![0] as { percentage: number; readAt: Date }; + const written = mockRepo.upsertReadingProgress.mock.calls[0]![0] as { percentage: number; readAt: Date; lastReadAt: Date }; expect(written.percentage).toBe(70); expect(written.readAt.getTime()).toBeGreaterThanOrEqual(before); + // Still ranked by when it was read, so reconnecting does not lift it above books read since. + expect(written.lastReadAt).toEqual(new Date('2026-10-10T17:00:00Z')); expect(mockBookService.syncAudioProgressForExternalEbookProgress).toHaveBeenCalledWith( 12, 55, @@ -524,6 +526,7 @@ describe('KoreaderService', () => { xpointer: '/body/DocFragment[7]', pageNumber: null, readAt: new Date(1700000000 * 1000), + lastReadAt: new Date(1700000000 * 1000), }); expect(mockRepo.restoreDevice).toHaveBeenCalledWith(12, 'device-12'); expect(mockBookService.syncKoboReadingStateForExternalProgress).toHaveBeenCalledWith(12, 44, 50); @@ -738,6 +741,33 @@ describe('KoreaderService', () => { expect(written.get(12)).toEqual(expect.objectContaining({ pageNumber: 31, cfi: null })); }); + // Regression: a first sweep pushes every book on the device, and each one was ranked as read + // the moment the sweep arrived, so sorting by last read came out in processing order. + it("stamps each swept book's last read with when the device last opened it", async () => { + const appliedAt = Math.floor(Date.now() / 1000); + + await service.applyBulkProgress( + 7, + [ + { bookFile: bookFile(10, 100), percentage: 0.5, timestamp: 1700000000 }, + { bookFile: bookFile(11, 110), percentage: 0.2, timestamp: 1750000000 }, + { bookFile: bookFile(12, 120), percentage: 0.9 }, + ], + device, + ); + + const written = new Map( + mockRepo.upsertReadingProgress.mock.calls.map((call) => { + const entry = call[0] as { bookFileId: number; lastReadAt: Date }; + return [entry.bookFileId, entry.lastReadAt]; + }), + ); + + expect(written.get(10)).toEqual(new Date(1700000000 * 1000)); + expect(written.get(11)).toEqual(new Date(1750000000 * 1000)); + expect(written.get(12)!.getTime()).toBeGreaterThanOrEqual(appliedAt * 1000); + }); + it('issues no query for an empty entry list', async () => { await expect(service.applyBulkProgress(7, [], device)).resolves.toEqual({ shared: 0, stale: 0, held: 0 }); diff --git a/server/src/modules/koreader/koreader.service.ts b/server/src/modules/koreader/koreader.service.ts index ef81a48fa..83e169320 100644 --- a/server/src/modules/koreader/koreader.service.ts +++ b/server/src/modules/koreader/koreader.service.ts @@ -501,8 +501,10 @@ export class KoreaderService { xpointer: data.progress ?? null, pageNumber, // A late push moves the page now, which is what other clients need to see; the narration - // below still judges it by when it was read, so newer listening keeps its marker. + // below and last-read ordering still judge it by when it was read, so newer listening + // keeps its marker. readAt: late ? new Date() : readAt, + lastReadAt: readAt, }); await this.bookService.syncKoboReadingStateForExternalProgress(userId, bookFile.id, bookorbitPercentage).catch(() => undefined); await this.bookService diff --git a/server/test/koreader-progress-position.e2e-spec.ts b/server/test/koreader-progress-position.e2e-spec.ts index 91d75b3f0..b33124589 100644 --- a/server/test/koreader-progress-position.e2e-spec.ts +++ b/server/test/koreader-progress-position.e2e-spec.ts @@ -504,4 +504,53 @@ describe('KOReader progress position routing (e2e)', { timeout: 180_000 }, () => expect(pull.json()).toEqual(expect.objectContaining({ percentage: 0.7, progress: null })); } }); + + // Regression: a sweep pushes the device's whole shelf at once, and every book in it was ranked + // as read the moment it arrived, so sorting by last read came out in processing order. + it('ranks swept books by when the device read them, and never below a newer read elsewhere', async () => { + const [olderPath, newerPath] = await Promise.all([ + createEpubFixture(library.folderPath, 'last-read/Older.epub', { title: `Last Read Older ${randomUUID()}`, uid: `urn:uuid:${randomUUID()}` }), + createEpubFixture(library.folderPath, 'last-read/Newer.epub', { title: `Last Read Newer ${randomUUID()}`, uid: `urn:uuid:${randomUUID()}` }), + ]); + await triggerAndWaitForLibraryScan(ctx, library.libraryId); + const older = await locateBookByAbsolutePath(ctx, olderPath); + const newer = await locateBookByAbsolutePath(ctx, newerPath); + const lastReadAt = async (bookFileId: number) => { + const [row] = await ctx.db + .select({ lastReadAt: schema.readingProgress.lastReadAt }) + .from(schema.readingProgress) + .where(eq(schema.readingProgress.bookFileId, bookFileId)); + return row?.lastReadAt.getTime(); + }; + const now = Math.floor(Date.now() / 1000); + const olderReadAt = now - 30 * 86400; + const newerReadAt = now - 2 * 86400; + + const sweep = await ctx.app.inject({ + method: 'POST', + url: '/api/v1/koreader/plugin/progress', + headers: deviceHeaders(), + payload: { + deviceId: DEVICE_ID, + deviceModel: 'Regression device', + pluginVersion: '1.7.0', + items: [ + { hash: await fileHashFor(older.bookFileId), percentage: 0.4, timestamp: olderReadAt }, + { hash: await fileHashFor(newer.bookFileId), percentage: 0.3, timestamp: newerReadAt }, + ], + }, + }); + expect(sweep.statusCode).toBe(201); + expect(await lastReadAt(older.bookFileId)).toBe(olderReadAt * 1000); + expect(await lastReadAt(newer.bookFileId)).toBe(newerReadAt * 1000); + + await saveFromWebReader(older.bookFileId, { percentage: 45 }); + const webReadAt = await lastReadAt(older.bookFileId); + expect(webReadAt).toBeGreaterThanOrEqual(now * 1000); + + // Read further offline an hour ago and pushed late: the page is taken, the newer web read stays the last one. + await syncFromDevice(await fileHashFor(older.bookFileId), 0.7, undefined, now - 3600); + expect((await storedProgress(older.bookFileId))?.percentage).toBeCloseTo(70, 5); + expect(await lastReadAt(older.bookFileId)).toBe(webReadAt); + }); });