fix(koplugin): fold duplicate stats book rows so synced time shows in KOReader (#4895)

The stats pull keyed the statistics book table by md5 alone, while KOReader's native statistics plugin keys rows by exact (title, authors, md5). When the two parsers extract slightly different metadata for the same file, the first native open creates a second, zeroed book row that the KOReader UI reads, and the reading time synced from Readest stays stranded on the sync-created row. applyRemote now inserts a book row only for an md5 the DB has never seen, attaches pulled events to the row the native plugin reads (native rows always set pages and last_open; sync-created rows leave pages NULL), and folds never-adopted duplicate rows into the surviving row on every pull, so existing databases heal and the stranded time reappears. Adopted rows and the live session's cached book id are never deleted, and the totals recompute no longer regresses last_open below a real native open timestamp. Fixes #4861 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-07-03 01:31:06 +09:00
parent 849f151166
commit bd415a8501
3 changed files with 238 additions and 8 deletions
@@ -579,7 +579,7 @@ function ReadestSync:pullBookStats(interactive)
        return
    end
    SyncStats:pull(self.settings, client, interactive,
-        function() SyncAuth:logout(self.settings, self.path) end)
+        function() SyncAuth:logout(self.settings, self.path) end, self.ui)
 end

 -- ── Annotation sync ────────────────────────────────────────────────
@@ -41,16 +41,86 @@ function SyncStats:collectSince(cursor)
    return books, pages
 end

+-- Fold duplicate book rows sharing one md5 into the row KOReader's native
+-- statistics plugin actually reads. That plugin keys rows by (title,
+-- authors, md5) — UNIQUE INDEX book_title_authors_md5 — so when its
+-- extracted metadata drifts from Readest's, the first native open adds a
+-- second, zeroed row and the synced reading time is stranded on the
+-- sync-created one (#4861). Only rows the native plugin never adopted
+-- (pages IS NULL; it always sets pages on the rows it creates or updates)
+-- are folded, and an adopted row is never deleted: an open reader session
+-- may hold its cached book id. live_book_id is the id the current session's
+-- statistics module cached — a session that adopted a sync-created row keeps
+-- pages NULL until its first close, so that row must not be folded either.
+local function mergeDuplicateBooks(conn, touched, live_book_id)
+    local dup_stmt = conn:prepare([[
+        SELECT md5 FROM book WHERE md5 IS NOT NULL AND md5 != ''
+        GROUP BY md5 HAVING COUNT(*) > 1;]])
+    local dups = {}
+    local row = dup_stmt:step()
+    while row ~= nil do
+        table.insert(dups, row[1])
+        row = dup_stmt:step()
+    end
+    dup_stmt:close()
+    if #dups == 0 then return end
+    local survivor_stmt = conn:prepare([[
+        SELECT id FROM book WHERE md5 = ?
+        ORDER BY (pages IS NOT NULL) DESC, last_open DESC, id ASC LIMIT 1;]])
+    local stranded_stmt = conn:prepare("SELECT id FROM book WHERE md5 = ? AND id != ? AND id != ? AND pages IS NULL;")
+    local move_pages = conn:prepare([[
+        INSERT INTO page_stat_data (id_book, page, start_time, duration, total_pages)
+        SELECT ?, page, start_time, duration, total_pages
+        FROM page_stat_data WHERE id_book = ?
+        ON CONFLICT(id_book, page, start_time)
+        DO UPDATE SET duration = max(duration, excluded.duration), total_pages = excluded.total_pages;]])
+    local del_pages = conn:prepare("DELETE FROM page_stat_data WHERE id_book = ?;")
+    local del_book = conn:prepare("DELETE FROM book WHERE id = ?;")
+    for _, md5 in ipairs(dups) do
+        local keep = tonumber(survivor_stmt:reset():bind(md5):step()[1])
+        local stranded = {}
+        stranded_stmt:reset():bind(md5, keep, tonumber(live_book_id) or -1)
+        local r = stranded_stmt:step()
+        while r ~= nil do
+            table.insert(stranded, tonumber(r[1]))
+            r = stranded_stmt:step()
+        end
+        for _, dead in ipairs(stranded) do
+            move_pages:reset():bind(keep, dead):step()
+            del_pages:reset():bind(dead):step()
+            del_book:reset():bind(dead):step()
+            touched[dead] = nil
+            touched[keep] = true
+            logger.dbg("ReadestStats applyRemote: folded duplicate book row "
+                .. dead .. " into " .. keep .. " (md5=" .. md5 .. ")")
+        end
+    end
+    survivor_stmt:close()
+    stranded_stmt:close()
+    move_pages:close()
+    del_pages:close()
+    del_book:close()
+end
+
 -- Upsert pulled rows into the local statistics.sqlite3 (union / longer-duration).
-function SyncStats:applyRemote(books, pages)
+function SyncStats:applyRemote(books, pages, live_book_id)
    local conn = SQ3.open(db_path())
    conn:exec("BEGIN;")
-    local insert_book = conn:prepare("INSERT OR IGNORE INTO book (title, authors, md5) VALUES (?, ?, ?);")
+    -- Insert a row only for an md5 this DB has never seen: if KOReader
+    -- already tracks the book under its own (possibly drifted) metadata,
+    -- adding one keyed on Readest's would duplicate it (#4861).
+    local insert_book = conn:prepare([[
+        INSERT INTO book (title, authors, md5)
+        SELECT ?, ?, ? WHERE NOT EXISTS (SELECT 1 FROM book WHERE md5 = ?);]])
    for _, b in ipairs(books or {}) do
-        insert_book:reset():bind(b.title or "", b.authors or "", b.book_hash):step()
+        insert_book:reset():bind(b.title or "", b.authors or "", b.book_hash, b.book_hash):step()
    end
    insert_book:close()
-    local find_id = conn:prepare("SELECT id FROM book WHERE md5 = ? LIMIT 1;")
+    -- Attach events to the row the native statistics plugin reads: it always
+    -- sets pages and last_open on its rows; sync-created rows leave them NULL.
+    local find_id = conn:prepare([[
+        SELECT id FROM book WHERE md5 = ?
+        ORDER BY (pages IS NOT NULL) DESC, last_open DESC, id ASC LIMIT 1;]])
    local insert_page = conn:prepare([[
        INSERT INTO page_stat_data (id_book, page, start_time, duration, total_pages)
        VALUES (?, ?, ?, ?, ?)
@@ -71,14 +141,18 @@ function SyncStats:applyRemote(books, pages)
    end
    find_id:close()
    insert_page:close()
+    mergeDuplicateBooks(conn, touched, live_book_id)
    -- Mirror the Readest app's recomputeBookTotals so a KOReader device shows
    -- fresh totals right after a pull (id is a trusted integer from the DB).
+    -- last_open never regresses: on native rows it is a real open timestamp
+    -- that can be newer than the last synced reading event.
    for id in pairs(touched) do
        conn:exec(string.format([[
            UPDATE book SET
                total_read_time  = COALESCE((SELECT SUM(duration) FROM page_stat_data WHERE id_book = %d), 0),
                total_read_pages = COALESCE((SELECT COUNT(DISTINCT page) FROM page_stat_data WHERE id_book = %d), 0),
-                last_open        = COALESCE((SELECT MAX(start_time + duration) FROM page_stat_data WHERE id_book = %d), last_open)
+                last_open        = MAX(COALESCE(last_open, 0),
+                                       COALESCE((SELECT MAX(start_time + duration) FROM page_stat_data WHERE id_book = %d), 0))
            WHERE id = %d;]], id, id, id, id))
    end
    conn:exec("COMMIT;")
@@ -130,7 +204,7 @@ function SyncStats:push(settings, client, interactive)
        end)
 end

-function SyncStats:pull(settings, client, interactive, logout_fn)
+function SyncStats:pull(settings, client, interactive, logout_fn, ui)
    local since = settings.stats_pull_cursor or 0
    logger.dbg("ReadestStats pull: since=" .. tostring(since)
        .. " interactive=" .. tostring(interactive))
@@ -152,7 +226,10 @@ function SyncStats:pull(settings, client, interactive, logout_fn)
            local nbooks = response and response.statBooks and #response.statBooks or 0
            local npages = response and response.statPages and #response.statPages or 0
            logger.dbg("ReadestStats pull: applying statBooks=" .. nbooks .. " statPages=" .. npages)
-            self:applyRemote(response.statBooks, response.statPages)
+            -- Resolved inside the callback so it reflects the session state
+            -- at apply time, after the async network round-trip.
+            local live_book_id = ui and ui.statistics and ui.statistics.id_curr_book or nil
+            self:applyRemote(response.statBooks, response.statPages, live_book_id)
            local newest = since
            for _, p in ipairs(response.statPages or {}) do
                local u = tonumber(p.updated_at_ms) or 0
@@ -129,6 +129,159 @@ describe("readest_syncstats", function()
        assert.are.equal(0, tonumber(pcount))
    end)

+    -- #4861: KOReader's statistics plugin keys book rows by (title, authors,
+    -- md5) — UNIQUE INDEX book_title_authors_md5 — while the sync pull keys
+    -- by md5 alone. When KOReader's extracted metadata drifts from Readest's,
+    -- the first native open adds a second, zeroed row (always with pages and
+    -- last_open set) and the synced reading time is stranded on the
+    -- sync-created row (pages NULL) that nothing reads anymore.
+    it("folds a stranded sync-created duplicate into the native book row (#4861)", function()
+        local conn = SQ3.open(statsDbPath())
+        -- a pull that ran before the book was first opened natively created
+        -- this row from Readest's metadata, holding the Readest reading time
+        conn:exec("INSERT INTO book (title, authors, md5) VALUES ('Book: subtitle', 'Auth', 'md5-dup');")
+        conn:exec("INSERT INTO page_stat_data (id_book, page, start_time, duration, total_pages) VALUES (1, 1, 100, 60, 10);")
+        conn:exec("INSERT INTO page_stat_data (id_book, page, start_time, duration, total_pages) VALUES (1, 2, 200, 60, 10);")
+        -- the first native open then missed on (title, authors, md5) and
+        -- inserted its own zeroed row
+        conn:exec([[INSERT INTO book (title, authors, md5, pages, last_open, total_read_time, total_read_pages)
+            VALUES ('Book - subtitle', 'Auth', 'md5-dup', 10, 1000, 0, 0);]])
+        conn:close()
+
+        SyncStats:applyRemote({}, {}) -- a pull with nothing new still heals
+
+        local conn2 = SQ3.open(statsDbPath())
+        local rows = conn2:rowexec("SELECT COUNT(*) FROM book WHERE md5 = 'md5-dup';")
+        local title = conn2:rowexec("SELECT title FROM book WHERE md5 = 'md5-dup';")
+        local total = conn2:rowexec("SELECT total_read_time FROM book WHERE md5 = 'md5-dup';")
+        local last_open = conn2:rowexec("SELECT last_open FROM book WHERE md5 = 'md5-dup';")
+        local orphans = conn2:rowexec(
+            "SELECT COUNT(*) FROM page_stat_data WHERE id_book NOT IN (SELECT id FROM book);")
+        conn2:close()
+        assert.are.equal(1, tonumber(rows))
+        assert.are.equal("Book - subtitle", title) -- the native row survives
+        assert.are.equal(120, tonumber(total))
+        assert.are.equal(1000, tonumber(last_open)) -- native open time not regressed
+        assert.are.equal(0, tonumber(orphans))
+    end)
+
+    it("keeps the longer duration when duplicate rows hold the same page event", function()
+        local conn = SQ3.open(statsDbPath())
+        conn:exec([[INSERT INTO book (title, authors, md5, pages, last_open, total_read_time, total_read_pages)
+            VALUES ('Native', 'A', 'md5-ov', 10, 50, 30, 1);]])
+        conn:exec("INSERT INTO page_stat_data (id_book, page, start_time, duration, total_pages) VALUES (1, 1, 100, 30, 10);")
+        conn:exec("INSERT INTO book (title, authors, md5) VALUES ('Synced', 'A', 'md5-ov');")
+        conn:exec("INSERT INTO page_stat_data (id_book, page, start_time, duration, total_pages) VALUES (2, 1, 100, 90, 10);")
+        conn:close()
+
+        SyncStats:applyRemote({}, {})
+
+        local conn2 = SQ3.open(statsDbPath())
+        local rows = conn2:rowexec("SELECT COUNT(*) FROM book WHERE md5 = 'md5-ov';")
+        local total = conn2:rowexec("SELECT total_read_time FROM book WHERE md5 = 'md5-ov';")
+        conn2:close()
+        assert.are.equal(1, tonumber(rows))
+        assert.are.equal(90, tonumber(total))
+    end)
+
+    it("does not create a duplicate row when the md5 exists under drifted metadata", function()
+        local conn = SQ3.open(statsDbPath())
+        conn:exec([[INSERT INTO book (title, authors, md5, pages, last_open, total_read_time, total_read_pages)
+            VALUES ('KO Title', 'KO Author', 'md5-x', 42, 1000, 0, 0);]])
+        conn:close()
+
+        SyncStats:applyRemote(
+            { { book_hash = "md5-x", title = "Readest Title", authors = "Readest Author" } },
+            { { book_hash = "md5-x", page = 3, start_time = 500, duration = 30, total_pages = 42 } })
+
+        local conn2 = SQ3.open(statsDbPath())
+        local rows = conn2:rowexec("SELECT COUNT(*) FROM book WHERE md5 = 'md5-x';")
+        local title = conn2:rowexec("SELECT title FROM book WHERE md5 = 'md5-x';")
+        local total = conn2:rowexec("SELECT total_read_time FROM book WHERE md5 = 'md5-x';")
+        conn2:close()
+        assert.are.equal(1, tonumber(rows))
+        assert.are.equal("KO Title", title)
+        assert.are.equal(30, tonumber(total))
+    end)
+
+    -- Rows KOReader itself created (pages set) are never deleted: an open
+    -- reader session may hold a cached id_book pointing at either of them.
+    it("leaves duplicate rows that KOReader itself adopted untouched", function()
+        local conn = SQ3.open(statsDbPath())
+        conn:exec([[INSERT INTO book (title, authors, md5, pages, last_open, total_read_time, total_read_pages)
+            VALUES ('Old Title', 'A', 'md5-nat', 10, 100, 5, 1);]])
+        conn:exec([[INSERT INTO book (title, authors, md5, pages, last_open, total_read_time, total_read_pages)
+            VALUES ('New Title', 'A', 'md5-nat', 10, 200, 7, 1);]])
+        conn:close()
+
+        SyncStats:applyRemote({}, {})
+
+        local conn2 = SQ3.open(statsDbPath())
+        local rows = conn2:rowexec("SELECT COUNT(*) FROM book WHERE md5 = 'md5-nat';")
+        conn2:close()
+        assert.are.equal(2, tonumber(rows))
+    end)
+
+    -- While a book is open, KOReader's statistics session holds a cached book
+    -- id (ui.statistics.id_curr_book). If that session adopted a sync-created
+    -- row (pages stays NULL until the first close writes it), folding must not
+    -- delete the row out from under the session, or the rest of its stats
+    -- would be written to a dangling id.
+    it("never folds the row cached by the live statistics session", function()
+        local conn = SQ3.open(statsDbPath())
+        -- legacy sync-created row the current session adopted (pages NULL)
+        conn:exec("INSERT INTO book (title, authors, md5) VALUES ('T', 'A', 'md5-live');")
+        conn:exec("INSERT INTO page_stat_data (id_book, page, start_time, duration, total_pages) VALUES (1, 1, 100, 10, 10);")
+        -- older native row left by a previous KOReader version's metadata
+        conn:exec([[INSERT INTO book (title, authors, md5, pages, last_open, total_read_time, total_read_pages)
+            VALUES ('T (old)', 'A', 'md5-live', 10, 50, 0, 0);]])
+        conn:close()
+
+        SyncStats:applyRemote({}, {}, 1) -- id 1 is the live session's row
+
+        local conn2 = SQ3.open(statsDbPath())
+        local live = conn2:rowexec("SELECT COUNT(*) FROM book WHERE id = 1;")
+        conn2:close()
+        assert.are.equal(1, tonumber(live))
+    end)
+
+    it("derives the live statistics book id from ui during pull", function()
+        local conn = SQ3.open(statsDbPath())
+        conn:exec("INSERT INTO book (title, authors, md5) VALUES ('T', 'A', 'md5-ui');")
+        conn:exec([[INSERT INTO book (title, authors, md5, pages, last_open, total_read_time, total_read_pages)
+            VALUES ('T2', 'A', 'md5-ui', 10, 50, 0, 0);]])
+        conn:close()
+        local client = { pullChanges = function(_, _params, cb)
+            cb(true, { statBooks = {}, statPages = {} }, 200)
+        end }
+        local ui = { statistics = { id_curr_book = 1 } }
+
+        SyncStats:pull({ stats_pull_cursor = 0 }, client, false, nil, ui)
+
+        local conn2 = SQ3.open(statsDbPath())
+        local live = conn2:rowexec("SELECT COUNT(*) FROM book WHERE id = 1;")
+        conn2:close()
+        assert.are.equal(1, tonumber(live))
+    end)
+
+    it("merges duplicate sync-created rows when KOReader never opened the book", function()
+        local conn = SQ3.open(statsDbPath())
+        conn:exec("INSERT INTO book (title, authors, md5) VALUES ('Title v1', 'A', 'md5-s');")
+        conn:exec("INSERT INTO page_stat_data (id_book, page, start_time, duration, total_pages) VALUES (1, 1, 100, 10, 10);")
+        conn:exec("INSERT INTO book (title, authors, md5) VALUES ('Title v2', 'A', 'md5-s');")
+        conn:exec("INSERT INTO page_stat_data (id_book, page, start_time, duration, total_pages) VALUES (2, 2, 200, 20, 10);")
+        conn:close()
+
+        SyncStats:applyRemote({}, {})
+
+        local conn2 = SQ3.open(statsDbPath())
+        local rows = conn2:rowexec("SELECT COUNT(*) FROM book WHERE md5 = 'md5-s';")
+        local total = conn2:rowexec("SELECT total_read_time FROM book WHERE md5 = 'md5-s';")
+        conn2:close()
+        assert.are.equal(1, tonumber(rows))
+        assert.are.equal(30, tonumber(total))
+    end)
+
    it("recomputes book totals after applying remote events", function()
        SyncStats:applyRemote(
            { { book_hash = "md5-tot", title = "Tot", authors = "A" } },