H3 census bugs #2 and #3: both web PUTs used plain queries that
bypassed the sync-aware layer entirely — no LWW, no source bookkeeping,
and locator fields the client couldn't even send. The highlight PUT was
the only one already routed (via HighlightID); the other two now mirror
it:
- SaveBookmarkRequest gains BookmarkID, SaveNoteRequest gains NoteID:
id-targeted resolution reads the exact row, verifies ownership,
LWWs under its stored dedup key, and resurrects tombstones when the
edit is newer (same doctrine and comment structure as the highlight
path).
- Fields the PUT omits are coalesced from the stored row before the
full-column sync update — the legacy web PUT sends only
title/notes/position, and zeroing cfi/percentage/page on every edit
would have been data loss. Verified both directions by
TestH3_BookmarkPUTPreservesLocators (omitted survives, explicit
lands).
- The bookmark PUT's wire DTO gains the create endpoint's locator keys
as optional fields (documented additive request keys in
contract_pins.md); the note PUT keeps its content+position wire.
UpdateMediaBookmark/UpdateMediaNote plain queries are now uncalled
(the generated queries stay until the H4 query sweep).
Pin-first batch for H3: freeze web annotation CRUD response shapes
(create 201 / update 200 / delete 204; full sqlc row models) before
the stack consolidation touches anything.
The new pins immediately caught a live server bug the 28 unit tests
could never see (annotations_test.go is pure-unit: keys and compare
logic, no DB): applyNoteLWW built UpdateMediaNoteForSyncParams without
UserID, so the query's WHERE id=$1 AND user_id=$13 compared against
zero — every dedup-hit note re-save (same content+position) returned
500 "update note: no rows in result set" and the update never landed.
The bookmark twin passes UserID and works; the note stack is the clone
that dropped the field.
Fix: pass UserID in the params (one line). Pins go green and become
the regression net; changelog records the new surfaces.
Book-side twin of the audio-survival test, closing H1's verification
list. Scenario: web push carries percentage + epubcfi + context_text;
a koreader-style percentage-only push from a second source within the
window detects a conflict; the user resolves with the CFI-carrying
side. The funneled resolution must land the winner's ENTIRE locator
pair — percentage 0.5, epubcfi, context_text together (the old direct
writer NULLed context_text and character_offset, breaking the coherent
pair the far-anchor work established) — and must not spawn a duplicate
pending conflict.
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 3b — THE data fix. Conflict resolution was the one live
erasure writer: applyProgressResolution and its verbatim twin
applyResolution both wrote the winner via a raw UpdateUniversalProgress
whose param list omits audio_offset_seconds/context_text/viewports, and
the SQL's DO UPDATE SET ... = EXCLUDED... NULLed exactly those. The web
UI resolves through POST /api/conflicts/:id/resolve and
/api/conflicts/bulk-resolve (main.js resolveConflict/bulkResolve), and
the reader's mark-as-read fires a resolution first when the reader SSR
stamps data-conflict-id/-winner — so every resolution today erased the
audiobook position and reader context.
Now: ProgressService is a required ConflictHandler dependency; BOTH
call sites (single resolve + bulk resolve) share one
applyProgressResolution that funnels through SaveProgress (the verbatim
twin is deleted). Deliberate choices, per the plan:
- SkipConflictDetection on the resolution write: both flows apply
BEFORE the conflict row is marked resolved, so the funnel's LWW
detector would otherwise file a duplicate pending conflict
(HasRecentConflictResolution is false at apply time).
- Source "conflict_resolution" — the row's last-write attribution; the
winner stays durably recorded in resolution_data.winner.
- Broadcast: true — live devices jump to the winning position.
- The winner snapshot maps percentage/epubcfi/context_text/chapter/
character/page/total_pages; a winner without a CFI clears the locator
pair (ClearLocator) instead of re-pairing with the loser's stale
anchor. Everything the snapshot doesn't carry SURVIVES via the LWW
seed: audio_offset_seconds above all. chapter_progress keeps its
stored value (was overwritten with the book percentage), an absent
percentage survives (was written as 0%).
- Accepted: the resolution captures one reading_history session
(attributed to conflict_resolution; the 2h idle ceiling applies).
Verification: TestH1_ConflictResolutionPreservesAudio drives the real
flow — web push with audio 300s, diverging push from a second source
detects a conflict, resolve with the second source as winner, GET shows
percentage 0.3 AND audio_offset_seconds 300 (previously NULLed), and no
duplicate pending conflict remains. Full contract pin suite green.
The host-run TestConflict*/TestProgress* failures are environmental
(container-only /app/uploads path + db hostname; fail identically at
baseline b4ae423) — the same suites pass in the test-runner container.
Batch H0 of the hardening plan: freeze the JSON key SETS of every
load-bearing surface so H1-H7 can only change them additively.
- cmd/server/tests/contract_pins_test.go: 14 tests / ~30 subtests pinning
progress GET+PUT, the KOReader plugin surfaces (progress push/pull,
library, bookmarks), the Kobo adapter shapes (initialization, markup,
sync-from-server), collections detail all four branches + preview,
series JSON, search rows, dashboard sections, media list BOTH branches
(raw sqlc rows vs resolved 38-key map), and SSR smoke for
bookshelf/collections/series/tags.
- docs/contract_pins.md: the pinned-shape inventory + the rule that a
batch widening a shape must widen the pin and this doc in one commit.
Suite is green on the unmodified tree at b4ae423. KOReader/Kobo/progress
pins are the hard freeze (the plugin is an external client).
John O'Keefe