fix(conflicts): resolution writes funnel through SaveProgress — audio position survives

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.
This commit is contained in:
John O'Keefe committed 2026-10-17 10:26:22 -04:00
1 parent e83c00f869
commit 8fc74c908a
5 files changed
+166 -124

No files matched your search

+1 -1
View File
@@ -151,7 +151,7 @@ func main() {
koreaderHandler.SetAnnotationService(annotationService)
koreaderHandler.SetLibraryService(libraryService)
wsHandler := handlers.NewWSHandler(queries, connManager, cfg.JWTSecret, deviceAuthMiddleware)
conflictHandler := handlers.NewConflictHandler(queries, connManager)
conflictHandler := handlers.NewConflictHandler(queries, connManager, progressService)
analyticsHandler := handlers.NewAnalyticsHandler(queries)
statisticsService := services.NewStatisticsService(queries)
statsHandler := handlers.NewStatsHandler(statisticsService)
+104 -1
View File
@@ -146,11 +146,19 @@ func createContractFixture(t *testing.T, setup *TestServerSetup) contractFixture
// (testuser) and returns its auth token. Bearer-token auth for the
// KOReader endpoints, URL-token auth for the Kobo endpoints.
func createContractDevice(t *testing.T, setup *TestServerSetup, deviceType string) string {
return createContractDeviceNamed(t, setup, "Test Contract Device", deviceType)
}
// createContractDeviceNamed registers a device with an explicit name —
// the device NAME is what the web progress push adopts as its sync
// source (X-Device-Token attribution), so conflict tests can drive a
// second named source without a separate account.
func createContractDeviceNamed(t *testing.T, setup *TestServerSetup, name, deviceType string) string {
t.Helper()
token := fmt.Sprintf("dev_contract_%s", uuid.New().String())
device, err := setup.DB.CreateDevice(context.Background(), database.CreateDeviceParams{
UserID: pgtype.UUID{Bytes: getTestUserID(t, setup.DB), Valid: true},
DeviceName: "Test Contract Device",
DeviceName: name,
DeviceType: deviceType,
DeviceIdentifier: fmt.Sprintf("contract-%s-%d", deviceType, time.Now().UnixNano()),
AuthToken: token,
@@ -822,3 +830,98 @@ func TestContractPin_SSRPages(t *testing.T) {
assert.Contains(t, html, "- Bookhoard</title>")
})
}
// ---------------------------------------------------------------------------
// 6. H1 verification — resolving a conflict must preserve unknown fields
// ---------------------------------------------------------------------------
// TestH1_ConflictResolutionPreservesAudio is the executable check for the
// hardening plan's H1 done-when: a resolution applied via
// POST /api/conflicts/:id/resolve (the endpoint the web UI and the
// reader's mark-as-read both use) must NOT erase fields the winner's
// snapshot doesn't carry (before the funnel, the resolution write NULLed
// audio_offset_seconds), and the funneled resolution write must not file
// a duplicate pending conflict on the item it just resolved.
func TestH1_ConflictResolutionPreservesAudio(t *testing.T) {
if testing.Short() {
t.Skip("Skipping integration test in short mode")
}
setup := setupTestServer(t)
fx := createContractFixture(t, setup)
progress := progressURL(setup.Server.URL, fx.itemID)
// Push 1: web push carrying an audiobook-style position (source "web")
// — audio_offset_seconds is stored but NOT part of conflict snapshots.
resp := doReq(t, "PUT", progress, map[string]interface{}{
"percentage": 0.5,
"audio_offset_seconds": 300,
}, setup.Token)
defer resp.Body.Close()
require.Equal(t, http.StatusOK, resp.StatusCode)
// Push 2 within the 5-minute window from a second source (a
// device-token push attributes the write to the device's name) with a
// diverging percentage and NO audio field: the funnel detects the
// source mismatch + pct diff and records a conflict.
deviceToken := createContractDeviceNamed(t, setup, "koreader", "koreader")
push2Req, err := http.NewRequest("PUT", progress,
strings.NewReader(`{"percentage": 0.3}`))
require.NoError(t, err)
push2Req.Header.Set("Content-Type", "application/json")
push2Req.Header.Set("Authorization", "Bearer "+setup.Token)
push2Req.Header.Set("X-Device-Token", deviceToken)
resp2, err := (&http.Client{}).Do(push2Req)
require.NoError(t, err)
require.Equal(t, http.StatusOK, resp2.StatusCode)
resp2.Body.Close()
// Find the freshly detected progress conflict.
listResp := doReq(t, "GET", setup.Server.URL+"/api/conflicts", nil, setup.Token)
defer listResp.Body.Close()
require.Equal(t, http.StatusOK, listResp.StatusCode)
list := decodeJSON(t, listResp)
conflicts, ok := list["conflicts"].([]interface{})
require.True(t, ok, "conflicts should be an array")
var conflictID string
for _, raw := range conflicts {
col := raw.(map[string]interface{})
if col["media_item_id"] == fx.itemID && col["conflict_type"] == "progress" && col["resolution_status"] == "unresolved" {
conflictID, _ = col["id"].(string)
}
}
require.NotEmpty(t, conflictID, "the two-source push should have detected a progress conflict")
// Resolve with the second source as the winner — the endpoint and
// payload shape the web UI posts (web/static/main.js resolveConflict).
resolve := doReq(t, "POST", setup.Server.URL+"/api/conflicts/"+conflictID+"/resolve", map[string]interface{}{
"winner": "koreader",
}, setup.Token)
defer resolve.Body.Close()
require.Equal(t, http.StatusOK, resolve.StatusCode)
resolved := decodeJSON(t, resolve)
require.Equal(t, true, resolved["conflict_resolved"])
// The saved row carries the winner's percentage AND the audio field
// the winner never knew about.
get := doReq(t, "GET", progress, nil, setup.Token)
defer get.Body.Close()
require.Equal(t, http.StatusOK, get.StatusCode)
row := decodeJSON(t, get)
require.Equal(t, 0.3, getFloatField(t, row, "percentage"))
require.Equal(t, float64(300), getFloatField(t, row, "audio_offset_seconds"),
"resolution must preserve audio_offset_seconds (LWW seed), not NULL it")
// No duplicate pending conflict may remain for this item (the funnel's
// own LWW detector must skip the resolution write).
get2 := doReq(t, "GET", setup.Server.URL+"/api/conflicts", nil, setup.Token)
defer get2.Body.Close()
require.Equal(t, http.StatusOK, get2.StatusCode)
list2 := decodeJSON(t, get2)
for _, raw := range list2["conflicts"].([]interface{}) {
col := raw.(map[string]interface{})
if col["media_item_id"] == fx.itemID && col["conflict_type"] == "progress" {
require.Equal(t, "user_resolved", col["resolution_status"],
"the resolved conflict must not have spawned a duplicate pending conflict")
}
}
}
+1 -1
View File
@@ -490,7 +490,7 @@ func setupTestServer(t *testing.T) *TestServerSetup {
koreaderHandler := handlers.NewKOReaderHandler(queries, connManager, queueProcessor, progressService)
koreaderHandler.SetAnnotationService(annotationService)
wsHandler := handlers.NewWSHandler(queries, connManager, cfg.JWTSecret, deviceAuthMiddleware)
conflictHandler := handlers.NewConflictHandler(queries, connManager)
conflictHandler := handlers.NewConflictHandler(queries, connManager, progressService)
analyticsHandler := handlers.NewAnalyticsHandler(queries)
queueHandler := handlers.NewQueueHandler(queries, queueProcessor)
systemSettingsHandler := handlers.NewSystemSettingsHandler(queries)