feat(server): progress locator coherence — percentage-only writes clear stale anchors
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.
This commit is contained in:
1 parent
f24a2d6c22
commit
69516ae165
5 files changed
+154
No files matched your search
@@ -656,3 +656,117 @@ func TestProgressWeb_EdgeCases(t *testing.T) {
|
|||||||
assert.InDelta(t, 1.5, progress.ZoomLevel.Float64, 0.01)
|
assert.InDelta(t, 1.5, progress.ZoomLevel.Float64, 0.01)
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestProgressWeb_LocatorCoherence pins the locator coherence rule: a
|
||||||
|
// percentage-only write that DECLARES no locator (ClearLocator) must clear
|
||||||
|
// any inherited stale anchor instead of re-pairing the new percentage with
|
||||||
|
// the previous writer's CFI/context — the incoherent row shape that fed the
|
||||||
|
// client-side far-anchor restore floor. Request-carried fields always win.
|
||||||
|
func TestProgressWeb_LocatorCoherence(t *testing.T) {
|
||||||
|
setup := setupTestServer(t)
|
||||||
|
ctx := context.Background()
|
||||||
|
userID := getTestUserID(t, setup.DB)
|
||||||
|
pgUserID := pgtype.UUID{Bytes: userID, Valid: true}
|
||||||
|
|
||||||
|
seedAnchor := func(t *testing.T, mediaItemID string) uuid.UUID {
|
||||||
|
mediaUUID, _ := uuid.Parse(mediaItemID)
|
||||||
|
_, err := setup.ProgressService.SaveProgress(ctx, wsync.SaveProgressRequest{
|
||||||
|
MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true},
|
||||||
|
UserID: pgUserID,
|
||||||
|
Source: "web",
|
||||||
|
Percentage: pFloat64(0.3),
|
||||||
|
Epubcfi: pStr("epubcfi(/6/4/2:first)"),
|
||||||
|
ContextText: pStr("some anchor context sentence"),
|
||||||
|
DeviceType: "web",
|
||||||
|
Broadcast: false,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
progress, err := setup.DB.GetReadingProgress(ctx, database.GetReadingProgressParams{
|
||||||
|
MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true},
|
||||||
|
UserID: pgUserID,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
require.True(t, progress.Epubcfi.Valid, "seed: anchor stored")
|
||||||
|
return mediaUUID
|
||||||
|
}
|
||||||
|
|
||||||
|
t.Run("percentage-only kobo write clears the inherited anchor", func(t *testing.T) {
|
||||||
|
mediaItemID := createTestMediaItemID(t, setup)
|
||||||
|
mediaUUID := seedAnchor(t, mediaItemID)
|
||||||
|
|
||||||
|
_, err := setup.ProgressService.SaveProgress(ctx, wsync.SaveProgressRequest{
|
||||||
|
MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true},
|
||||||
|
UserID: pgUserID,
|
||||||
|
Source: "kobo",
|
||||||
|
Percentage: pFloat64(0.6),
|
||||||
|
DeviceType: "kobo",
|
||||||
|
ClearLocator: true,
|
||||||
|
Broadcast: false,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
progress, err := setup.DB.GetReadingProgress(ctx, database.GetReadingProgressParams{
|
||||||
|
MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true},
|
||||||
|
UserID: pgUserID,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.InDelta(t, 0.6, progress.Percentage.Float64, 0.001, "percentage updated")
|
||||||
|
assert.False(t, progress.Epubcfi.Valid, "stale anchor cleared, not re-paired")
|
||||||
|
assert.False(t, progress.ContextText.Valid, "stale context cleared")
|
||||||
|
assert.Equal(t, "kobo", progress.LastSyncSource.String)
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("request-carried context survives the clear", func(t *testing.T) {
|
||||||
|
mediaItemID := createTestMediaItemID(t, setup)
|
||||||
|
mediaUUID := seedAnchor(t, mediaItemID)
|
||||||
|
|
||||||
|
_, err := setup.ProgressService.SaveProgress(ctx, wsync.SaveProgressRequest{
|
||||||
|
MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true},
|
||||||
|
UserID: pgUserID,
|
||||||
|
Source: "koreader",
|
||||||
|
Percentage: pFloat64(0.7),
|
||||||
|
ContextText: pStr("a brand new context sentence"),
|
||||||
|
DeviceType: "koreader",
|
||||||
|
ClearLocator: true,
|
||||||
|
Broadcast: false,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
progress, err := setup.DB.GetReadingProgress(ctx, database.GetReadingProgressParams{
|
||||||
|
MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true},
|
||||||
|
UserID: pgUserID,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.InDelta(t, 0.7, progress.Percentage.Float64, 0.001)
|
||||||
|
assert.True(t, progress.ContextText.Valid, "request-carried context kept")
|
||||||
|
assert.Equal(t, "a brand new context sentence", progress.ContextText.String)
|
||||||
|
// The verify gate is inert in the test harness (no media path
|
||||||
|
// resolver wired), so no anchor can be minted here; the rule
|
||||||
|
// under test is that the STALE one is gone.
|
||||||
|
assert.False(t, progress.Epubcfi.Valid, "stale anchor cleared")
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("HTTP merge semantics untouched: key-absent PUT preserves fields", func(t *testing.T) {
|
||||||
|
mediaItemID := createTestMediaItemID(t, setup)
|
||||||
|
mediaUUID := seedAnchor(t, mediaItemID)
|
||||||
|
|
||||||
|
// Service-level save WITHOUT ClearLocator mimics the HTTP merge
|
||||||
|
// path (field absent = inherit): the anchor must survive.
|
||||||
|
_, err := setup.ProgressService.SaveProgress(ctx, wsync.SaveProgressRequest{
|
||||||
|
MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true},
|
||||||
|
UserID: pgUserID,
|
||||||
|
Source: "queue",
|
||||||
|
Percentage: pFloat64(0.4),
|
||||||
|
Broadcast: false,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
progress, err := setup.DB.GetReadingProgress(ctx, database.GetReadingProgressParams{
|
||||||
|
MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true},
|
||||||
|
UserID: pgUserID,
|
||||||
|
})
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.True(t, progress.Epubcfi.Valid, "merge path preserves the anchor")
|
||||||
|
assert.Equal(t, "epubcfi(/6/4/2:first)", progress.Epubcfi.String)
|
||||||
|
})
|
||||||
|
}
|
||||||
@@ -458,6 +458,7 @@ func (h *KoboHandler) Markup(c *echo.Context) error {
|
|||||||
DeviceType: "kobo",
|
DeviceType: "kobo",
|
||||||
DeviceName: device.DeviceName,
|
DeviceName: device.DeviceName,
|
||||||
Broadcast: true,
|
Broadcast: true,
|
||||||
|
ClearLocator: true,
|
||||||
})
|
})
|
||||||
} else {
|
} else {
|
||||||
_, err = h.db.UpdateUniversalProgress(c.Request().Context(), database.UpdateUniversalProgressParams{
|
_, err = h.db.UpdateUniversalProgress(c.Request().Context(), database.UpdateUniversalProgressParams{
|
||||||
@@ -806,6 +807,7 @@ func (h *KoboHandler) AnalyticsGettests(c *echo.Context) error {
|
|||||||
DeviceType: "kobo",
|
DeviceType: "kobo",
|
||||||
DeviceName: device.DeviceName,
|
DeviceName: device.DeviceName,
|
||||||
Broadcast: true,
|
Broadcast: true,
|
||||||
|
ClearLocator: true,
|
||||||
})
|
})
|
||||||
} else {
|
} else {
|
||||||
_, err = h.db.UpdateUniversalProgress(c.Request().Context(), database.UpdateUniversalProgressParams{
|
_, err = h.db.UpdateUniversalProgress(c.Request().Context(), database.UpdateUniversalProgressParams{
|
||||||
@@ -868,6 +870,7 @@ func (h *KoboHandler) SyncFromServer(c *echo.Context) error {
|
|||||||
DeviceType: "kobo",
|
DeviceType: "kobo",
|
||||||
DeviceName: device.DeviceName,
|
DeviceName: device.DeviceName,
|
||||||
Broadcast: false,
|
Broadcast: false,
|
||||||
|
ClearLocator: true,
|
||||||
})
|
})
|
||||||
} else {
|
} else {
|
||||||
_, err = h.db.UpdateUniversalProgress(c.Request().Context(), database.UpdateUniversalProgressParams{
|
_, err = h.db.UpdateUniversalProgress(c.Request().Context(), database.UpdateUniversalProgressParams{
|
||||||
|
|||||||
@@ -862,6 +862,11 @@ func (h *KOReaderHandler) updateProgressForBook(c *echo.Context, deviceID pgtype
|
|||||||
|
|
||||||
if h.progressSvc != nil {
|
if h.progressSvc != nil {
|
||||||
epubcfi := book.Epubcfi
|
epubcfi := book.Epubcfi
|
||||||
|
// No position pointer from the device, or a conversion that
|
||||||
|
// declined below: the write is percentage-only by intent. The
|
||||||
|
// service then clears any inherited stale anchor instead of
|
||||||
|
// re-pairing it with the new percentage.
|
||||||
|
clearLocator := epubcfi == nil
|
||||||
if epubcfi != nil && wsync.IsCREXPointer(*epubcfi) {
|
if epubcfi != nil && wsync.IsCREXPointer(*epubcfi) {
|
||||||
// Same facade every annotation uses: cached converter,
|
// Same facade every annotation uses: cached converter,
|
||||||
// structural-first resolution, guarded fallbacks. The facade
|
// structural-first resolution, guarded fallbacks. The facade
|
||||||
@@ -897,6 +902,7 @@ func (h *KOReaderHandler) updateProgressForBook(c *echo.Context, deviceID pgtype
|
|||||||
// and the device keeps its own native position.
|
// and the device keeps its own native position.
|
||||||
log.Printf("Bookhoard: CRE→CFI conversion low-confidence (%s) for %s; storing percentage only", loc.Precision, *epubcfi)
|
log.Printf("Bookhoard: CRE→CFI conversion low-confidence (%s) for %s; storing percentage only", loc.Precision, *epubcfi)
|
||||||
epubcfi = nil
|
epubcfi = nil
|
||||||
|
clearLocator = true
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -916,6 +922,7 @@ func (h *KOReaderHandler) updateProgressForBook(c *echo.Context, deviceID pgtype
|
|||||||
DeviceType: "koreader",
|
DeviceType: "koreader",
|
||||||
DeviceName: deviceModel,
|
DeviceName: deviceModel,
|
||||||
Broadcast: true,
|
Broadcast: true,
|
||||||
|
ClearLocator: clearLocator,
|
||||||
}
|
}
|
||||||
|
|
||||||
_, err := h.progressSvc.SaveProgress(ctx, saveReq)
|
_, err := h.progressSvc.SaveProgress(ctx, saveReq)
|
||||||
|
|||||||
@@ -315,6 +315,16 @@ type SaveProgressRequest struct {
|
|||||||
DeviceType string
|
DeviceType string
|
||||||
DeviceName string
|
DeviceName string
|
||||||
Broadcast bool
|
Broadcast bool
|
||||||
|
|
||||||
|
// ClearLocator declares that this write INTENDS to carry no position
|
||||||
|
// locator: percentage-only writers (devices whose converter declined,
|
||||||
|
// Kobo markup percentage syncs, queue entries without CFI). Without it,
|
||||||
|
// seeding below would re-pair the new percentage with the PREVIOUS
|
||||||
|
// writer's stale CFI/context — the incoherent row shape that feeds the
|
||||||
|
// client-side far-anchor restore floor. Locater-less rows are honest:
|
||||||
|
// percentage restores are approximate, and the next anchor-bearing
|
||||||
|
// write re-establishes the pair. Request-supplied fields always win.
|
||||||
|
ClearLocator bool
|
||||||
}
|
}
|
||||||
|
|
||||||
func (s *ProgressService) SaveProgress(ctx context.Context, req SaveProgressRequest) (database.ReadingProgress, error) {
|
func (s *ProgressService) SaveProgress(ctx context.Context, req SaveProgressRequest) (database.ReadingProgress, error) {
|
||||||
@@ -407,6 +417,22 @@ func (s *ProgressService) SaveProgress(ctx context.Context, req SaveProgressRequ
|
|||||||
params.ReadingMode = pgtype.Text{String: *req.ReadingMode, Valid: true}
|
params.ReadingMode = pgtype.Text{String: *req.ReadingMode, Valid: true}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Locator coherence: a write that declares no locator must not
|
||||||
|
// silently re-pair its new percentage with the previous writer's
|
||||||
|
// stale CFI/context (inherited by the seeding above). Clear them so
|
||||||
|
// the row is an honest percentage-only position. Request-carried
|
||||||
|
// fields always win — and a request-carried context survives so the
|
||||||
|
// verify/heal pass below can mint a fresh anchor from it.
|
||||||
|
if req.ClearLocator && req.Epubcfi == nil {
|
||||||
|
params.Epubcfi = pgtype.Text{}
|
||||||
|
if req.ContextText == nil {
|
||||||
|
params.ContextText = pgtype.Text{}
|
||||||
|
}
|
||||||
|
if req.CharacterOffset == nil {
|
||||||
|
params.CharacterOffset = pgtype.Int8{}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
params.LastSyncDevice = pgtype.Text{String: req.Source, Valid: true}
|
params.LastSyncDevice = pgtype.Text{String: req.Source, Valid: true}
|
||||||
params.LastSyncSource = pgtype.Text{String: req.Source, Valid: true}
|
params.LastSyncSource = pgtype.Text{String: req.Source, Valid: true}
|
||||||
|
|
||||||
|
|||||||
@@ -443,6 +443,10 @@ func (p *SyncQueueProcessor) syncProgress(ctx context.Context, userID pgtype.UUI
|
|||||||
if v, ok := syncData["epubcfi"].(string); ok {
|
if v, ok := syncData["epubcfi"].(string); ok {
|
||||||
req.Epubcfi = &v
|
req.Epubcfi = &v
|
||||||
}
|
}
|
||||||
|
// Entries without a CFI are percentage-only by intent: declare it
|
||||||
|
// so the service clears any inherited stale anchor instead of
|
||||||
|
// re-pairing it with this percentage.
|
||||||
|
req.ClearLocator = req.Epubcfi == nil
|
||||||
if v, ok := syncData["context_text"].(string); ok {
|
if v, ok := syncData["context_text"].(string); ok {
|
||||||
req.ContextText = &v
|
req.ContextText = &v
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in new issue
Block a user