H1's doctrine applied to the annotation service, which had been left on
setters: NewMediaHandler/NewKoboHandler/NewKOReaderHandler/
NewSyncQueueProcessor(WithConfig) now take it as a parameter; the four
SetAnnotationService setters are deleted; wiring updated (main.go,
router/sync.go, test harness).
With the service unskippable, the 18 nil-guards and their plain-query
fallbacks die:
- media.go: the create fallbacks for notes/highlights/bookmarks (the
bookmark one was the H3 census's Bug #1 — it dropped
percentage/chapter_reference the sync path carries), the highlight
PUT fallback, and the three delete-path plain deletes (tombstone is
now the only delete semantics).
- kobo.go: both markup-ingest annotation loops (the bookmark fallback
wrote the text into media_notes — a category error the fallback
never should have had).
- koreader.go: all three push-stack guards.
- queue.go: the three "annotation service not available" errors in
syncNote/syncHighlight/syncBookmark.
Behavior note: the deleted highlight create/PUT fallbacks were the only
paths passing note_id to the plain query — the sync service has no
NoteID field, so the web's highlight→note link was already being
dropped on the live path; this commit makes that pre-existing
limitation explicit rather than papering it over (surfacing note_id on
SaveHighlightRequest is left for a behavior commit that needs it).
Pins (now 20 incl. the 3 annotation CRUD suites) + H1 tests + both
unit suites green.
Found while verifying H1's "app offline-play -> flush -> position lands"
with the emulator path: the flush has been dead code in two independent
ways, invisible because the first break made the second unreachable.
1. ListPendingSyncQueueItems matched `device_id = $1` with a NULL $1
(the processor passes DeviceID{Valid:false} to enumerate all devices;
uuid = NULL matches no rows). The ticker therefore processed nothing,
ever. The WHERE now treats NULL as "every device" (the same
narg::uuid IS NULL idiom the visibility queries use; EXPLAIN'd — seq
scan on a bounded table, idx_sync_queue_status_priority_created
fits). The processor also dropped its fake-pagination loop, which
re-fetched the same first batch forever once the backlog hit the
batch size (no OFFSET in the query).
2. dbItemToSyncQueueItem dropped SyncData — even with a working
enumerator, every flushed item arrived with an empty payload and
failed JSON unmarshal. Dead code harboring silent breakage, exactly
the plan's thesis.
Callers updated for the renamed RowLimit param (queue handler, offline
detector). TestH1_SyncQueueFlush_LandsAndPreservesAudio drives the real
flow: web PUT seeds audio 900s, a pending checkpoint-style queue row
applies percentage 0.8 through SaveProgress, audio survives, queue row
completes.
Also fixes dashboard_test.go's stale BuildSections signature (the
package's test binary has been unbuildable) — user-approved: tests
designed against old shapes are not worth keeping.
Batch H1 step 3a. The ceiling census (exhaustive UpdateUniversalProgress
grep) found 9 nil-guarded `if progressSvc != nil { funnel } else { raw
erase-write }` fallbacks — the survey's "four" under-counted kobo.go,
which has FOUR sites (Markup ReadingSync, Markup last-read-place,
AnalyticsGettests, SyncFromServer), plus one each in media.go, queue.go
and koreader.go. All unreachable: the service is created unconditionally
at main.go:127 and wired into every handler and the queue processor; the
test harness wires them too.
The erasure pattern is now unrepresentable:
- NewMediaHandler / NewKOReaderHandler / NewKoboHandler / both
NewSyncQueueProcessor flavors take the ProgressService as a REQUIRED
constructor arg; the SetProgressService setters are gone.
- Every fallback body (raw UpdateUniversalProgress with explicit-NULL
pgtypes for viewport/scroll/panel/reading_mode/zoom and no
audio_offset_seconds/context_text) is deleted; each write funnels
through SaveProgress with the LWW seed, so fields the writer doesn't
know survive.
- Removed the now-dead koreader pg-type helpers (textPtrToPgText,
intPtrToPgInt4, int64PtrToPgInt8) and their self-tests.
Behavior deltas (stored-row content, no wire shape changes —
KOReader/Kobo response pins stayed green):
- KOReader fallback used to write explicit NULL viewports and drop the
context_text the koplugin sends; the funneled mainline already maps
book.ContextText.
- Queue flushes no longer overwrite chapter_progress with the book
percentage (bogus data; preserved via the seed).
- A web PUT without percentage no longer writes 0% via the deleted
fallback; percentage seeds from the stored row.
Contract suite: KOReader×4 + Kobo×3 pins green.
The verify/heal pipeline verified only INBOUND context-bearing writes;
stored-row coherence was never enforced. SaveProgress seeds missing
fields from the existing row, so every percentage-only write (KOReader
low-confidence conversion, KOReader no-pointer payloads, Kobo markup
ReadingSync / analytics / sync-from-server, locator-less queue entries)
silently re-paired its NEW percentage with the PREVIOUS writer's stale
CFI+context — the incoherent row shape that fed the client-side
far-anchor restore floor (and, via the read-side anchor handles, could
even hand a first-party client the OLD position's scroll address).
KOReader's converter already declared the right intent ("storing
percentage only — the row's percentage restores approximately"); the
seeding betrayed it. SaveProgressRequest gains ClearLocator, set by the
percentage-only call sites, and the seeding rule honors it: stale
epubcfi/context/character_offset clear (the upsert assigns columns
directly, so NULL sticks) unless the request carries its own. A
request-carried context survives so the verify/heal pass can still mint
a fresh anchor from it; the HTTP merge path (field-absent PUTs
preserve) and all anchor-bearing writes are untouched. The fixed-layout
branch already cleared locators unconditionally — unchanged.
Consumer audit: percentage — the only thing every sync round-trip
exchanges — is untouched on all paths; KOReader pulls on locator-less
rows already support the nil-CFI shape (builder guards
progress.Epubcfi.Valid); first-party restores go percentage-first
instead of floor-fighting a stale anchor.
Tests: TestProgressWeb_LocatorCoherence pins clear-on-declared,
request-carried-context survival, and merge-path preservation. Full
suite with the change: 218 groups, zero failures.
Split each constructor into a default-args wrapper and a config-accepting
variant so the sync queue interval/batch size and the worker pool size/
queue cap can be sourced from the settings registry at startup. These
values are constructed once at boot, so they are tagged requires_restart
in the admin UI.
queue.go:
- NewSyncQueueProcessorWithConfig(db, interval, batchSize) takes the
flush interval and batch size as parameters; NewSyncQueueProcessor
becomes a thin wrapper with the historical 5s / 50 defaults.
worker.go:
- NewWorkerWithConfig(numWorkers, queueCap, connManager) takes the
queue capacity as a parameter; NewWorker becomes a thin wrapper with
the historical cap of 100.
No behavior change for existing callers; main.go will switch to the
config-accepting variants in a follow-up wiring commit.
Wire AnnotationService into SyncQueueProcessor and implement the three
previously-stubbed execute methods:
- syncHighlight: unmarshals syncData JSON into SaveHighlightRequest,
applies CRE→CFI conversion via AnnotationService
- syncNote: unmarshals into SaveNoteRequest
- syncBookmark: unmarshals into SaveBookmarkRequest
- Add SyncTypeBookmark to executeSync switch (was hitting default error)
Add enqueue methods for future offline/batch use:
- EnqueueHighlight / EnqueueNote / EnqueueBookmark
- Shared enqueueAnnotation helper creates queue items with
PriorityCriticalNote and 3 max attempts
- Update types (HighlightUpdate, NoteUpdate, BookmarkUpdate) mirror the
existing ProgressUpdate pattern
Existing handler behavior is unchanged — annotations still sync
synchronously via AnnotationService. The queue path is available for
retry-on-failure and offline batch processing scenarios.
- SaveProgressRequest: add ContextText field
- SaveProgress: carry existing ContextText from DB, overwrite when provided
- ProgressUpdate: add ContextText field for checkpoint sync
- SyncQueueProcessor: serialize/deserialize context_text in sync data
- buildProgressSnapshot: include context_text in snapshot data
All four progress write paths now delegate to ProgressService.SaveProgress:
- MediaHandler: UpdateMediaReadingProgress uses ProgressService for web
saves with richer request body (reading_mode, zoom_level, scroll). GET
now uses GetUniversalProgress query that JOINs media_items for
format_group, total_characters, chapter_count.
- KOReaderHandler: updateProgressForBook delegates to ProgressService.
Fixed device ID bug (was using userID, now uses deviceID). Removed
duplicate UpdateDeviceLastSync with zero UUID. Added pgtype helper
functions (textPtrToPgText, intPtrToPgInt4, int64PtrToPgInt8).
- KoboHandler: all four progress write points (Markup ReadingSync, Markup
last-read-place, AnalyticsGettests, SyncFromServer) delegate to
ProgressService. Fixed empty epubcfi string now correctly set to
Valid: false. SyncFromServer preserves last_sync_source=bookhoard
and Broadcast: false.
- QueueProcessor: syncProgress delegates to ProgressService.
- main.go: creates ProgressService after ConnectionManager, injects via
SetProgressService() on all handlers and queue processor.
Handler tests cover pgtype conversion helpers (textPtrToPgText, etc.)
and device icon mapping.
Remove redundant type conversions that Go 1.26 makes unnecessary or that
were already no-ops:
- uuid.UUID(x.Bytes) → x.Bytes (uuid.UUID is [16]byte, same as pgtype UUID Bytes)
- pgtype.UUID{Bytes: [16]byte(u), Valid: true} → pgtype.UUID{Bytes: u, Valid: true}
- (*time.Time)(&x.Time) → &x.Time
- json.RawMessage(x) → x where x is already []byte
- []byte(stringVal) → stringVal where []byte is expected
- int()/int64()/byte() casts on values already of the target type
- Decompressor(fn) → fn (type is identical)
Handle previously ignored error returns:
- collections.go: check json.Unmarshal error in GetCollection
- conversion_service.go: check fileSize.Scan() error
- app_test.go: check app.Shutdown() error in benchmark
John O'Keefe
john-okeefe