feat(sync): propagate annotation deletions to the server

Deleting a highlight or bookmark in KOReader previously never reached
Bookhoard: the push is upsert-only by design (absence must not mean
delete — a 'What to sync' category toggled off would otherwise wipe the
server), and the next pull re-added the annotation from the server.

Deletions are now reported explicitly and safely:

- applyServerAnnotations remembers the dedup key of every annotation it
  places or confirms locally (sidecar 'bookhoard_known_keys', mapped to
  highlight/bookmark). Keys are learned ONLY from server-served entries,
  so a device-native annotation can never be mis-flagged, and entries
  with unplaceable locators are never tracked (their absence is not a
  deletion). Legacy pos0-matched entries get the key stamped on first
  contact so identity matching and tracking work from then on.
- Pushes diff the remembered keys against the local store; vanished keys
  go out as deleted_highlights/deleted_bookmarks arrays. Gated per
  category like the annotation arrays themselves: a disabled category
  sends neither its annotations nor its deletions.
- After a successful push the sent keys are pruned; server tombstones
  received on pull clear their keys too, so a deletion is reported at
  most once per direction.

The server tombstones the rows (restorable from the web book page's
deleted-annotation history) and echoes tombstones to other devices.

BookhoardAPI.lua

@@ -126,6 +126,13 @@ function BookhoardAPI:syncProgress(book_data, sync_mode)
     }, PROGRESS_TIMEOUTS)
 end
 
+-- Read-only SHA-256 → book UUID lookup. Used to link a freshly downloaded
+-- book WITHOUT pushing progress (a push would transmit the device's
+-- first-page position and clobber/conflict with real server progress).
+function BookhoardAPI:resolveBook(sha256)
+    return self:_request("GET", "/api/sync/koreader/resolve?sha256=" .. sha256, nil, PROGRESS_TIMEOUTS)
+end
+
 function BookhoardAPI:getMetadata(uuid)
     return self:_request("GET", "/api/sync/koreader/metadata/" .. uuid, nil, PROGRESS_TIMEOUTS)
 end

main.lua

@@ -121,17 +121,13 @@ end
 function Bookhoard:onReaderReady()
     if self.settings.auto_sync then
         UIManager:nextTick(function()
-            -- On the first open of a freshly downloaded book there is no cached
-            -- bookhoard UUID yet (it is only learned from a successful push
-            -- response). Pulling now would just fail with "Push progress first".
-            -- Instead, push once to bootstrap identity: the server resolves the
-            -- book by SHA-256 (format-aware) and returns the UUID, which we then
-            -- cache. After that, push and pull both work without ordering.
-            if not self:getBookhoardUUID() then
-                self:updateProgress(true, false)
-            else
-                self:getProgress(true, false)
-            end
+            -- Pull-first on every open. A freshly downloaded book has no cached
+            -- bookhoard UUID yet; getProgress resolves it by SHA-256 with a
+            -- read-only lookup (no progress push). The previous push-to-
+            -- bootstrap strategy transmitted the device's first-page position
+            -- to the server on books already mid-read elsewhere, creating a
+            -- sync conflict before the first pull could ever happen.
+            self:getProgress(true, false)
         end)
     end
     self:registerEvents()
@@ -853,6 +849,118 @@ function Bookhoard:getAnnotationStore()
     return nil, nil
 end
 
+-- ============================================================================
+-- Deletion propagation
+--
+-- The push is upsert-only: absence from the annotation arrays must never be
+-- read as a delete (a category toggled off in "What to sync" would otherwise
+-- wipe the server). Instead the sidecar remembers the dedup keys of every
+-- annotation the server has served us (bookhoard_known_keys). When one of
+-- those keys is no longer present in the local store, the user deleted it on
+-- this device, and the next push reports it via deleted_highlights /
+-- deleted_bookmarks so the server tombstones it (restorable from the web
+-- history). Keys are only ever learned from server pulls, so a device-native
+-- annotation can never be mis-flagged.
+-- ============================================================================
+
+function Bookhoard:getKnownAnnotationKeys()
+    if not self.ui.doc_settings then return nil end
+    local known = self.ui.doc_settings:readSetting("bookhoard_known_keys")
+    if type(known) ~= "table" then return nil end
+    return known
+end
+
+-- Record one dedup key of an annotation that exists locally (applied now or
+-- already present), so a later local absence can be reported as a deliberate
+-- deletion. kind: "highlight" or "bookmark" (device notes are highlights-
+-- with-notes server-side). Returns true when the sidecar changed.
+function Bookhoard:rememberAnnotationKey(kind, key)
+    if not key or key == "" or not self.ui.doc_settings then return false end
+    local known = self:getKnownAnnotationKeys() or {}
+    if known[key] == kind then return false end
+    known[key] = kind
+    self.ui.doc_settings:saveSetting("bookhoard_known_keys", known)
+    return true
+end
+
+-- Drop keys the server tombstoned: the deletion already happened server-side,
+-- so this device must never report it again.
+function Bookhoard:forgetAnnotationKeys(tombstones)
+    if not tombstones or not self.ui.doc_settings then return false end
+    local known = self:getKnownAnnotationKeys()
+    if not known then return false end
+    local changed = false
+    for _, del in ipairs(tombstones) do
+        local key = del.dedup_key
+        if key and key ~= "" and known[key] ~= nil then
+            known[key] = nil
+            changed = true
+        end
+    end
+    if changed then
+        self.ui.doc_settings:saveSetting("bookhoard_known_keys", known)
+    end
+    return changed
+end
+
+-- Diff the remembered keys against the local annotation store. Returns
+-- arrays of { dedup_key = key } entries for highlights (incl. notes) and
+-- bookmarks whose annotations vanished locally, or nil when there is nothing
+-- to report. Requires a readable local store: with none, local absence is
+-- meaningless and nothing may be flagged.
+function Bookhoard:collectDeletions()
+    local model, entries = self:getAnnotationStore()
+    if not model then return nil, nil end
+    local known = self:getKnownAnnotationKeys()
+    if not known or next(known) == nil then return nil, nil end
+
+    local present = {}
+    for _, bm in ipairs(entries) do
+        if bm.bookhoard_dedup_key then
+            present[bm.bookhoard_dedup_key] = true
+        end
+    end
+
+    local deleted_highlights, deleted_bookmarks
+    for key, kind in pairs(known) do
+        if not present[key] then
+            if kind == "bookmark" then
+                deleted_bookmarks = deleted_bookmarks or {}
+                table.insert(deleted_bookmarks, { dedup_key = key })
+            else
+                deleted_highlights = deleted_highlights or {}
+                table.insert(deleted_highlights, { dedup_key = key })
+            end
+        end
+    end
+    return deleted_highlights, deleted_bookmarks
+end
+
+-- After a successful push the flagged deletions are now tombstones on the
+-- server; drop their keys so they are not re-reported forever (the tombstone
+-- echo on the next pull would clear them too, this just avoids the wait).
+function Bookhoard:pruneKnownAnnotationKeys(deleted_highlights, deleted_bookmarks)
+    if not self.ui.doc_settings then return end
+    if not deleted_highlights and not deleted_bookmarks then return end
+    local known = self:getKnownAnnotationKeys()
+    if not known then return end
+    local changed = false
+    for _, list in ipairs({ deleted_highlights, deleted_bookmarks }) do
+        if list then
+            for _, entry in ipairs(list) do
+                if known[entry.dedup_key] ~= nil then
+                    known[entry.dedup_key] = nil
+                    changed = true
+                end
+            end
+        end
+    end
+    if changed then
+        self.ui.doc_settings:saveSetting("bookhoard_known_keys", known)
+        self.ui.doc_settings:flush()
+    end
+end
+
 function Bookhoard:collectAnnotations()
     local bookmarks = json_util.InitArray({})
     local highlights = json_util.InitArray({})
@@ -994,7 +1102,15 @@ function Bookhoard:updateProgress(ensure_networking, interactive)
         return
     end
 
-    if not self.settings.sync_progress then return end
+    if not self.settings.sync_progress then
+        if interactive then
+            UIManager:show(InfoMessage:new{
+                text = _("Reading progress sync is disabled (see What to sync)."),
+                timeout = 3,
+            })
+        end
+        return
+    end
 
     if not self.ui.document then
         if interactive then
@@ -1011,9 +1127,19 @@ function Bookhoard:updateProgress(ensure_networking, interactive)
         return
     end
 
-    if ensure_networking
-        and NetworkMgr:willRerunWhenOnline(function() self:updateProgress(ensure_networking, interactive) end) then
-        return
+    if ensure_networking then
+        -- Same silent-drop case as getProgress: connected but no internet
+        -- means willRerunWhenOnline neither reruns nor shows any UI.
+        if interactive and NetworkMgr:isConnected() and not NetworkMgr:isOnline() then
+            UIManager:show(InfoMessage:new{
+                text = _("No internet connection. Progress was not pushed."),
+                timeout = 3,
+            })
+            return
+        end
+        if NetworkMgr:willRerunWhenOnline(function() self:updateProgress(ensure_networking, interactive) end) then
+            return
+        end
     end
 
     UIManager:scheduleIn(0.5, function()
@@ -1038,6 +1164,25 @@ function Bookhoard:_doUpdateProgress(interactive)
         book_data.notes = {}
     end
 
+    -- Report annotations deleted locally since the last pull. Gated per
+    -- category exactly like the arrays above: a disabled category sends
+    -- neither its annotations nor its deletions.
+    local deleted_hl, deleted_bm = self:collectDeletions()
+    if deleted_hl then
+        if self.settings.sync_highlights then
+            book_data.deleted_highlights = deleted_hl
+        else
+            deleted_hl = nil -- not sent: must not be pruned below
+        end
+    end
+    if deleted_bm then
+        if self.settings.sync_bookmarks then
+            book_data.deleted_bookmarks = deleted_bm
+        else
+            deleted_bm = nil
+        end
+    end
+
     local api = self:getAPI()
     local ok, result = api:syncProgress(book_data, self.settings.sync_mode)
 
@@ -1053,6 +1198,7 @@ function Bookhoard:_doUpdateProgress(interactive)
                     end
                 end
             end
+            self:pruneKnownAnnotationKeys(deleted_hl, deleted_bm)
             if interactive then
                 UIManager:show(InfoMessage:new{
                     text = _("Progress has been pushed."),
@@ -1082,30 +1228,53 @@ function Bookhoard:getProgress(ensure_networking, interactive)
         return
     end
 
-    if not self.settings.sync_progress then return end
+    if not self.settings.sync_progress then
+        if interactive then
+            UIManager:show(InfoMessage:new{
+                text = _("Reading progress sync is disabled (see What to sync)."),
+                timeout = 3,
+            })
+        end
+        return
+    end
 
     local now = UIManager:getElapsedTimeSinceBoot()
     if not interactive and now - self.pull_timestamp <= API_CALL_DEBOUNCE_DELAY then
         return
     end
 
-    if ensure_networking
-        and NetworkMgr:willRerunWhenOnline(function() self:getProgress(ensure_networking, interactive) end) then
-        return
+    if ensure_networking then
+        -- willRerunWhenOnline silently drops the action (no rerun, no UI)
+        -- when the device is connected at the IP level but has no internet
+        -- access: it only guarantees isConnected, and in that state the
+        -- framework re-connects without ever invoking the callback. Catch
+        -- that state up front so an interactive pull never no-ops silently.
+        if interactive and NetworkMgr:isConnected() and not NetworkMgr:isOnline() then
+            UIManager:show(InfoMessage:new{
+                text = _("No internet connection. Progress was not pulled."),
+                timeout = 3,
+            })
+            return
+        end
+        if NetworkMgr:willRerunWhenOnline(function() self:getProgress(ensure_networking, interactive) end) then
+            return
+        end
     end
 
     local book_uuid = self:getBookhoardUUID()
     if not book_uuid then
-        -- No cached UUID yet (first open of a freshly downloaded book). Bootstrap
-        -- it by pushing once: the server resolves the book by SHA-256 and returns
-        -- the UUID, which the push handler caches. Then retry the pull.
+        -- No cached UUID yet (first open of a freshly downloaded book). Link
+        -- the book with a read-only SHA-256 lookup, then pull. Progress is
+        -- never pushed here: the device sits on the first page of a book the
+        -- server may hold mid-read, and pushing that would overwrite/server-
+        -- conflict the real progress before the pull could deliver it.
         if interactive then
             UIManager:show(InfoMessage:new{
                 text = _("Linking this book to Bookhoard first…"),
                 timeout = 3,
             })
         end
-        self:_bootstrapUUIDThenPull(interactive)
+        self:_linkBookThenPull(interactive)
         return
     end
 
@@ -1116,9 +1285,89 @@ function Bookhoard:getProgress(ensure_networking, interactive)
     self.pull_timestamp = now
 end
 
--- Establish the bookhoard UUID for the current document by pushing once, then
--- (once cached) perform the originally-requested pull. Used when a pull is
--- requested before any push has run on a newly downloaded book.
+-- Link the current document to Bookhoard WITHOUT pushing progress, then run
+-- the pull. The UUID is learned via a read-only SHA-256 resolve; only when
+-- the server does not know the hash at all (sideloaded book) do we fall back
+-- to the legacy push-bootstrap, and only for background (non-interactive)
+-- flows — a user-initiated pull must never push this device's position.
+function Bookhoard:_linkBookThenPull(interactive)
+    local file_sha256 = self:getFileSHA256()
+    if not file_sha256 then
+        if interactive then
+            UIManager:show(InfoMessage:new{
+                text = _("Could not read this file to link it with Bookhoard."),
+                timeout = 3,
+            })
+        end
+        return
+    end
+
+    UIManager:scheduleIn(0.5, function()
+        local api = self:getAPI()
+        local ok, result = api:resolveBook(file_sha256)
+
+        UIManager:nextTick(function()
+            if not self.ui.doc_settings then return end
+
+            if ok and type(result) == "table" and result.book_uuid then
+                self.ui.doc_settings:saveSetting("bookhoard_uuid", result.book_uuid)
+                self.ui.doc_settings:flush()
+                logger.dbg("Bookhoard: linked book by SHA-256 to", result.book_uuid)
+                self:_doGetProgress(interactive)
+                return
+            end
+
+            if result and result.status == 404 then
+                -- Two very different failures share this status. The server's
+                -- resolve handler answers a book miss with JSON
+                -- {"error": "book not found"}; anything else behind a 404
+                -- (an HTML error page) means the route itself is missing —
+                -- a server image predating the resolve endpoint, a reverse
+                -- proxy, or a wrong server URL. Only the former may advise
+                -- pushing to link: acting on a stale-server 404 would push
+                -- this device's first-page position over the server's real
+                -- progress — the very conflict this flow exists to prevent.
+                if result.error == "book not found" then
+                    -- Genuinely unknown to the server (sideloaded book). The
+                    -- legacy push-bootstrap only ever linked such books via
+                    -- fuzzy title/author matching; keep that as a background
+                    -- attempt, but tell interactive users to push explicitly.
+                    if interactive then
+                        UIManager:show(InfoMessage:new{
+                            text = _("This book is not in your Bookhoard library yet. Push progress to link it."),
+                            timeout = 4,
+                        })
+                    else
+                        self:_bootstrapUUIDThenPull(interactive)
+                    end
+                else
+                    logger.warn("Bookhoard: resolve endpoint returned 404 without a book-not-found body (stale server?)")
+                    if interactive then
+                        UIManager:show(InfoMessage:new{
+                            text = _("Your Bookhoard server does not support this request and may be outdated. Update the server and try again."),
+                            timeout = 5,
+                        })
+                    end
+                end
+                return
+            end
+
+            logger.warn("Bookhoard: failed to resolve book by SHA-256")
+            if interactive then
+                UIManager:show(InfoMessage:new{
+                    text = _("Could not reach Bookhoard. Check your network connection and try again."),
+                    timeout = 4,
+                })
+            end
+        end)
+    end)
+end
+
+-- Legacy fallback: establish the bookhoard UUID for the current document by
+-- pushing once, then (once cached) perform the originally-requested pull.
+-- Only reached from background flows when the SHA-256 resolve reported the
+-- book as unknown to the server; the push mainly serves fuzzy title/author
+-- linking and device alias registration for sideloaded books.
 function Bookhoard:_bootstrapUUIDThenPull(interactive)
     self:updateProgress(false, false)
     UIManager:scheduleIn(2, function()
@@ -1144,7 +1393,7 @@ function Bookhoard:_doGetProgress(interactive)
         if not ok or not result then
             if interactive then
                 UIManager:show(InfoMessage:new{
-                    text = _("Failed to pull progress."),
+                    text = _("Failed to pull progress. Check your network connection."),
                     timeout = 3,
                 })
             end
@@ -1318,9 +1567,22 @@ function Bookhoard:applyServerAnnotations(annotations)
         if not isValidPos0(pos0) then return end
         local srv_text = server_entry.text or ""
         local srv_notes = server_entry.notes or ""
+        local srv_key = server_entry.dedup_key
+        -- Annotations that exist locally (after this call) get their key
+        -- remembered so a later deletion can be reported. Unplaceable
+        -- locators return above and are never tracked — absence of an
+        -- annotation this device never held is not a deletion.
+        local kind = has_text and "highlight" or "bookmark"
         local idx = findLocal(server_entry)
         if idx then
             local bm = entries[idx]
+            -- Legacy pos0 match on a device-native entry: stamp the key so
+            -- identity matching and deletion tracking work from now on.
+            if srv_key and srv_key ~= "" and not bm.bookhoard_dedup_key then
+                bm.bookhoard_dedup_key = srv_key
+                changed = true
+            end
+            changed = self:rememberAnnotationKey(kind, srv_key) or changed
             local cur_text, cur_note = getFields(bm)
             if cur_text ~= srv_text or cur_note ~= srv_notes then
                 setFields(bm, srv_text, srv_notes)
@@ -1366,6 +1628,7 @@ function Bookhoard:applyServerAnnotations(annotations)
             end
             local index = self.ui.annotation:addItem(entry)
             notify({ entry, index_modified = index })
+            changed = self:rememberAnnotationKey(kind, srv_key) or changed
             changed = true
         else
             local entry = {
@@ -1387,6 +1650,7 @@ function Bookhoard:applyServerAnnotations(annotations)
             end
             if server_entry.chapter and server_entry.chapter ~= "" then entry.chapter = server_entry.chapter end
             table.insert(entries, entry)
+            changed = self:rememberAnnotationKey(kind, srv_key) or changed
             changed = true
         end
     end
@@ -1435,6 +1699,7 @@ function Bookhoard:applyServerAnnotations(annotations)
     end
 
     if annotations.deleted_highlights then
+        changed = self:forgetAnnotationKeys(annotations.deleted_highlights) or changed
         for _, del in ipairs(annotations.deleted_highlights) do
             local idx = findLocalByTombstone(del, true)
             local bm = idx and entries[idx]
@@ -1448,6 +1713,7 @@ function Bookhoard:applyServerAnnotations(annotations)
     end
 
     if annotations.deleted_bookmarks then
+        changed = self:forgetAnnotationKeys(annotations.deleted_bookmarks) or changed
         for _, del in ipairs(annotations.deleted_bookmarks) do
             local idx = findLocalByTombstone(del, false)
             if idx then