fix(tomesync): strict filename resolve + position uniqueness + unread reset

CHANGELOG.md

@@ -76,6 +76,30 @@ All notable changes to Tome are documented here. Format loosely follows
   folder is kept. The same whitelist governs the folder cleanup after a
   reorganise, which also no longer errors out after the moves have already
   succeeded.
+- **A sideloaded book can no longer hijack another book's reading progress.**
+  When KOReader opens a file Tome never served (a sideload, or a copy renamed
+  on the device), Tome matches it to a library book by filename as a last
+  resort. That matcher was too eager: a short title like *"It"* matched
+  *The Italian Job*, a `Foo v2` filename landed on a standalone *"Foo"*, and a
+  year in the name (`… - 1984 - …`) was misread as "volume 1984". Any of these
+  silently wrote one book's position, sessions and highlights onto another.
+  Matching is now strict — whole-word title matches, a minimum length before a
+  substring counts, and a volume number in the filename must line up with the
+  book's actual volume — and when it can't be sure it declines (the device just
+  keeps tracking locally) rather than guessing. Books downloaded from Tome are
+  unaffected: they match by content hash, not by name.
+- **Reading position no longer flaps between two values.** The position table
+  had no uniqueness guard, so a device sync and a web-reader save landing at
+  the same moment could each create a row for the same book — after which reads
+  picked an arbitrary one and the progress appeared to jump back and forth (and
+  stats double-counted it). There is now one position per book, both writers
+  update it in place, and a one-time cleanup collapses any existing duplicates
+  to the most recent.
+- **Marking a book "unread" on the web now sticks on your device too.** The
+  reset cleared the web side but left the synced position untouched, so the
+  next time the book opened on KOReader it jumped back to where you'd been and
+  flipped itself to "reading" again. Marking a book unread now also clears the
+  synced position.
 - **Hardcover sync could pin a translation's or the audiobook's edition.**
   Publishers reuse one catalogue across translations and audio, so a stored
   ISBN sometimes names the German edition (or the audiobook) of the right

backend/api/tome_sync.py

@@ -29,7 +29,7 @@
 from backend.models.tome_sync import Annotation, AnnotationTombstone, ApiKey, ReadingSession, TomeSyncPosition
 from backend.models.ko_stats import StatsImport
 from backend.models.send_queue import SendQueueItem
-from backend.services.book_progress import apply_progress_to_status
+from backend.services.book_progress import apply_progress_to_status, upsert_position
 from backend.services.hardcover_sync import nudge as hardcover_nudge
 
 router = APIRouter(tags=["tome-sync"])
@@ -83,6 +83,57 @@ def _get_position(db: Session, user_id: int, book_id: int) -> Optional[TomeSyncP
     )
 
 
+# ── Filename → book resolution helpers ────────────────────────────────────────
+# These back the heuristic fallbacks in resolve_book, which only fire for files
+# that never passed through Tome (no content-hash, no exact path). A wrong guess
+# there silently writes one book's progress and annotations onto another, so the
+# matching is deliberately strict: it would rather return 404 (device keeps
+# local-only tracking) than clobber the wrong book.
+
+# Titles/stems shorter than this may match only by exact equality — a bare
+# "It"/"Us"/"Go" substring otherwise swallows unrelated sideloaded filenames.
+_MIN_HEURISTIC_LEN = 4
+
+
+def _phrase_index(haystack: str, needle: str) -> int:
+    """Index of needle in haystack where it is not glued to an alphanumeric on
+    either side (a whole-phrase occurrence), or -1. Tolerant of punctuation in
+    needle, unlike a naive ``\\b`` regex."""
+    start = 0
+    n = len(needle)
+    while True:
+        pos = haystack.find(needle, start)
+        if pos == -1:
+            return -1
+        before = haystack[pos - 1] if pos > 0 else ""
+        after = haystack[pos + n] if pos + n < len(haystack) else ""
+        if not before.isalnum() and not after.isalnum():
+            return pos
+        start = pos + 1
+
+
+def _title_in_stem(title_l: str, stem_l: str) -> bool:
+    """Book title occurs as a whole phrase inside the filename stem. Short
+    titles must equal the stem exactly, so "It" can't match "The Italian Job"."""
+    if len(title_l) < _MIN_HEURISTIC_LEN:
+        return title_l == stem_l
+    return _phrase_index(stem_l, title_l) != -1
+
+
+def _stem_in_title(stem_l: str, title_l: str) -> bool:
+    """Filename stem is a word-boundaried fragment of the title (handles
+    truncated names). Minimum length guards against "1" matching "1984"."""
+    if len(stem_l) < _MIN_HEURISTIC_LEN:
+        return False
+    pos = title_l.find(stem_l)
+    while pos != -1:
+        before = title_l[pos - 1] if pos > 0 else ""
+        if not before.isalnum():  # stem begins at a word boundary
+            return True
+        pos = title_l.find(stem_l, pos + 1)
+    return False
+
+
 # ── Resolve endpoint ─────────────────────────────────────────────────────────
 
 @router.get("/tome-sync/resolve")
@@ -132,34 +183,36 @@ def resolve_book(
             return {"book_id": book.id}
 
     # Volume number, in any of the shapes the download paths emit:
-    #   "Vol. 12", "v01", or a separator-delimited token "Series - 02 - Title".
+    #   "Vol. 12", "Vol. 2.5", "v01", or a separator-delimited token
+    #   "Series - 02 - Title". The separator token is bounded to 1-3 digits so a
+    #   4-digit year ("Foo - 1984 - Bar") is not misread as volume 1984, and the
+    #   "Vol." form accepts a half-volume (2.5) instead of truncating to 2.
     vol_num = None
     vol_match = (
-        re.search(r'[Vv]ol\.?\s*(\d+)', stem)
+        re.search(r'[Vv]ol\.?\s*(\d+(?:\.\d+)?)', stem)
         or re.search(r'\bv(\d{1,3})\b', stem)
-        or re.search(r'[-—]\s*0*(\d+)\s*[-—]', stem)
+        or re.search(r'[-—]\s*(\d{1,3})\s*[-—]', stem)
     )
     if vol_match:
         vol_num = float(vol_match.group(1))
 
     all_active = db.query(Book).filter(Book.status == "active").all()
 
-    # 2. Forward match: the book's title appears inside the filename. This is the
-    #    strong signal — the download paths all put the title into the name.
-    candidates = [b for b in all_active if b.title and b.title.lower() in stem_l]
+    # 2. Forward match: the book's title appears as a whole phrase in the
+    #    filename (short titles must match exactly — see _title_in_stem).
+    candidates = [b for b in all_active if b.title and _title_in_stem(b.title.lower(), stem_l)]
 
-    # When the filename carries a volume number it is authoritative: it selects
-    # the right volume AND rejects any candidate whose series_index disagrees.
-    # This is what stops vol-2's filename (which contains vol-1's title when the
-    # series shares its name with its first book) from resolving to vol-1.
+    # When the filename carries a volume number it is authoritative: only a book
+    # at that exact index may resolve. An index-less (standalone) candidate must
+    # NOT win by substring — a numbered file does not belong to an unnumbered
+    # book — and two books at the same index are genuinely ambiguous.
     if vol_num is not None:
         exact = [b for b in candidates if b.series_index == vol_num]
         if len(exact) == 1:
             return {"book_id": exact[0].id}
-        candidates = [
-            b for b in candidates
-            if b.series_index is None or b.series_index == vol_num
-        ]
+        raise HTTPException(
+            status_code=404, detail="Ambiguous filename; could not resolve uniquely"
+        )
 
     if len(candidates) == 1:
         return {"book_id": candidates[0].id}
@@ -175,14 +228,12 @@ def resolve_book(
             status_code=404, detail="Ambiguous filename; could not resolve uniquely"
         )
 
-    # 3. Reverse fallback: the filename is a substring of exactly one title
-    #    (handles truncated/shortened names). Honour the volume here too.
-    reverse = [b for b in all_active if b.title and stem_l in b.title.lower()]
+    # 3. Reverse fallback: the stem is a word-boundaried fragment of exactly one
+    #    title (truncated names). Min length guards against "1" → "1984"; a
+    #    volume number, if present, still requires an exact index match.
+    reverse = [b for b in all_active if b.title and _stem_in_title(stem_l, b.title.lower())]
     if vol_num is not None:
-        reverse = [
-            b for b in reverse
-            if b.series_index is None or b.series_index == vol_num
-        ]
+        reverse = [b for b in reverse if b.series_index == vol_num]
     if len(reverse) == 1:
         return {"book_id": reverse[0].id}
 
@@ -234,21 +285,10 @@ def put_position(
     # Clamp percentage to 0-1 range
     pct = max(0.0, min(1.0, body.percentage))
 
-    pos = _get_position(db, user.id, book_id)
-    if pos:
-        pos.progress = body.progress
-        pos.percentage = pct
-        pos.device = body.device
-        pos.updated_at = datetime.utcnow()
-    else:
-        pos = TomeSyncPosition(
-            user_id=user.id,
-            book_id=book_id,
-            progress=body.progress,
-            percentage=pct,
-            device=body.device,
-        )
-        db.add(pos)
+    upsert_position(
+        db, user_id=user.id, book_id=book_id,
+        percentage=pct, progress=body.progress, device=body.device,
+    )
 
     # Keep UserBookStatus in sync via the shared sticky-completion rule.
     # Position sync tracks the device last-write-wins (monotonic=False) — a

backend/api/users.py

@@ -12,6 +12,7 @@
 from backend.core.security import get_current_user
 from backend.core.permissions import require_role
 from backend.services.hardcover_sync import nudge as hardcover_nudge
+from backend.services.book_progress import upsert_position, clear_position
 from backend.models.user import User, UserPermission
 from backend.models.user_book_status import UserBookStatus
 from backend.models.tome_sync import ReadingSession, TomeSyncPosition
@@ -134,6 +135,9 @@ def set_book_status(
         if body.status == 'unread':
             row.progress_pct = None
             row.cfi = None
+            # Drop the synced device position too, or the device re-pulls it on
+            # open and the "unread" reset silently un-does itself.
+            clear_position(db, user_id=current_user.id, book_id=book_id)
         else:
             if 'progress_pct' in raw:
                 row.progress_pct = body.progress_pct
@@ -158,25 +162,23 @@ def set_book_status(
     # progress_pct is a 0–1 fraction end-to-end (API field, UserBookStatus.progress_pct,
     # and TomeSyncPosition.percentage), so no scaling is needed.
     if body.progress_pct is not None or body.cfi is not None:
-        pos = db.query(TomeSyncPosition).filter(
+        # Upsert against the UNIQUE(user, book) constraint so a device heartbeat
+        # racing this write can't fork a duplicate position row. Preserve the
+        # partial-update semantics: only overwrite the fields actually sent.
+        existing = db.query(TomeSyncPosition).filter(
             TomeSyncPosition.user_id == current_user.id,
             TomeSyncPosition.book_id == book_id,
         ).first()
-        if pos:
-            if body.progress_pct is not None:
-                pos.percentage = body.progress_pct
-            if body.cfi is not None:
-                pos.progress = body.cfi
-            pos.device = "web"
-            pos.updated_at = datetime.utcnow()
-        else:
-            db.add(TomeSyncPosition(
-                user_id=current_user.id,
-                book_id=book_id,
-                percentage=(body.progress_pct or 0),
-                progress=body.cfi,
-                device="web",
-            ))
+        pct = body.progress_pct if body.progress_pct is not None else (
+            existing.percentage if existing else 0.0
+        )
+        cfi = body.cfi if body.cfi is not None else (
+            existing.progress if existing else None
+        )
+        upsert_position(
+            db, user_id=current_user.id, book_id=book_id,
+            percentage=pct, progress=cfi, device="web",
+        )
         db.commit()
 
     # ── Web reader session tracking ──────────────────────────────────────────

backend/main.py

@@ -145,6 +145,30 @@ async def lifespan(app: FastAPI):
         if qc_cols and "poll_token" not in qc_cols:
             conn.execute(text("ALTER TABLE quick_connect_codes ADD COLUMN poll_token VARCHAR(64)"))
             conn.commit()
+        # Enforce one reading-position row per (user, book). Concurrent device
+        # and web first-writes could previously both INSERT, leaving duplicate
+        # rows that made position reads flap and double-counted stats joins.
+        # Dedupe (keep the freshest) before adding the constraint so it can't
+        # fail on legacy data.
+        tsp_cols = {r[1] for r in conn.execute(text("PRAGMA table_info(tome_sync_positions)")).fetchall()}
+        if tsp_cols:
+            conn.execute(text("""
+                DELETE FROM tome_sync_positions
+                WHERE id NOT IN (
+                    SELECT keep_id FROM (
+                        SELECT id AS keep_id, ROW_NUMBER() OVER (
+                            PARTITION BY user_id, book_id
+                            ORDER BY updated_at DESC, id DESC
+                        ) AS rn
+                        FROM tome_sync_positions
+                    ) WHERE rn = 1
+                )
+            """))
+            conn.execute(text(
+                "CREATE UNIQUE INDEX IF NOT EXISTS uq_tspos_user_book "
+                "ON tome_sync_positions (user_id, book_id)"
+            ))
+            conn.commit()
         # Per-user ratings/reviews on user_book_status (nullable — existing rows stay unrated).
         ubs_cols = {r[1] for r in conn.execute(text("PRAGMA table_info(user_book_status)")).fetchall()}
         if ubs_cols and "rating" not in ubs_cols:

backend/models/tome_sync.py

@@ -65,6 +65,12 @@ class ReadingSession(Base):
 class TomeSyncPosition(Base):
     """Latest reading position per user+book, updated on every push."""
     __tablename__ = "tome_sync_positions"
+    __table_args__ = (
+        # One position row per user+book. Two writers (device heartbeat, web
+        # autosave) both do read-modify-write; without this they could each
+        # INSERT a first-ever row and fork into duplicates that flap on read.
+        UniqueConstraint("user_id", "book_id", name="uq_tspos_user_book"),
+    )
 
     id: Mapped[int] = mapped_column(Integer, primary_key=True)
     user_id: Mapped[int] = mapped_column(

backend/services/book_progress.py

@@ -19,11 +19,77 @@
 from datetime import datetime
 from typing import Optional
 
+from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm import Session
 
+from backend.models.tome_sync import TomeSyncPosition
 from backend.models.user_book_status import UserBookStatus
 
 
+def upsert_position(
+    db: Session,
+    *,
+    user_id: int,
+    book_id: int,
+    percentage: float,
+    progress: Optional[str],
+    device: Optional[str],
+) -> TomeSyncPosition:
+    """Insert-or-update the single (user, book) reading-position row.
+
+    Both the device heartbeat and the web reader's autosave write positions;
+    with a UNIQUE(user_id, book_id) constraint in place, a losing concurrent
+    INSERT raises IntegrityError, which we absorb (via a SAVEPOINT so the
+    caller's other pending changes survive) and retry as an UPDATE. The two
+    writers then converge on one row instead of silently forking into two.
+    Does not commit — the caller owns the transaction.
+    """
+    row = (
+        db.query(TomeSyncPosition)
+        .filter(TomeSyncPosition.user_id == user_id, TomeSyncPosition.book_id == book_id)
+        .first()
+    )
+    if row is None:
+        row = TomeSyncPosition(
+            user_id=user_id, book_id=book_id,
+            percentage=percentage, progress=progress, device=device,
+        )
+        try:
+            with db.begin_nested():
+                db.add(row)
+                db.flush()
+            return row
+        except IntegrityError:
+            # Another writer created the row between our SELECT and INSERT.
+            row = (
+                db.query(TomeSyncPosition)
+                .filter(TomeSyncPosition.user_id == user_id,
+                        TomeSyncPosition.book_id == book_id)
+                .first()
+            )
+            if row is None:
+                raise
+
+    row.percentage = percentage
+    row.progress = progress
+    row.device = device
+    row.updated_at = datetime.utcnow()
+    return row
+
+
+def clear_position(db: Session, *, user_id: int, book_id: int) -> None:
+    """Drop the synced reading position for a user+book.
+
+    Called when the web explicitly resets a book to "unread": otherwise the
+    stale position row survives, the device re-pulls it on open, and the reset
+    un-does itself. Does not commit — the caller owns the transaction.
+    """
+    db.query(TomeSyncPosition).filter(
+        TomeSyncPosition.user_id == user_id,
+        TomeSyncPosition.book_id == book_id,
+    ).delete(synchronize_session=False)
+
+
 def apply_progress_to_status(
     db: Session,
     *,