From 9323b1e205310d1f88b2ab09be192ce2cecaf871 Mon Sep 17 00:00:00 2001 From: Sung Date: Sun, 26 Oct 2025 00:18:03 -0700 Subject: [PATCH] Fix in a more correct way --- pkg/cli/cmd/sync/sync.go | 51 +++++++----- pkg/cli/cmd/sync/sync_test.go | 147 +++++++++++++++++++++++++++------- 2 files changed, 150 insertions(+), 48 deletions(-) diff --git a/pkg/cli/cmd/sync/sync.go b/pkg/cli/cmd/sync/sync.go index bdf13a19..8ce08282 100644 --- a/pkg/cli/cmd/sync/sync.go +++ b/pkg/cli/cmd/sync/sync.go @@ -90,6 +90,7 @@ type syncList struct { ExpungedNotes map[string]bool ExpungedBooks map[string]bool MaxUSN int + UserMaxUSN int // Server's actual max USN (for distinguishing empty fragment vs empty server) MaxCurrentTime int64 } @@ -104,6 +105,7 @@ func processFragments(fragments []client.SyncFragment) (syncList, error) { expungedNotes := map[string]bool{} expungedBooks := map[string]bool{} var maxUSN int + var userMaxUSN int var maxCurrentTime int64 for _, fragment := range fragments { @@ -120,12 +122,11 @@ func processFragments(fragments []client.SyncFragment) (syncList, error) { expungedNotes[uuid] = true } - // Use UserMaxUSN instead of FragMaxUSN to track the server's actual max USN. - // FragMaxUSN is the max USN in THIS fragment (can be 0 if empty), - // while UserMaxUSN is the server's current max USN (always accurate). - // This prevents last_max_usn from being reset to 0 when we get empty fragments. - if fragment.UserMaxUSN > maxUSN { - maxUSN = fragment.UserMaxUSN + if fragment.FragMaxUSN > maxUSN { + maxUSN = fragment.FragMaxUSN + } + if fragment.UserMaxUSN > userMaxUSN { + userMaxUSN = fragment.UserMaxUSN } if fragment.CurrentTime > maxCurrentTime { maxCurrentTime = fragment.CurrentTime @@ -138,6 +139,7 @@ func processFragments(fragments []client.SyncFragment) (syncList, error) { ExpungedNotes: expungedNotes, ExpungedBooks: expungedBooks, MaxUSN: maxUSN, + UserMaxUSN: userMaxUSN, MaxCurrentTime: maxCurrentTime, } @@ -581,7 +583,7 @@ func fullSync(ctx context.DnoteCtx, tx *database.DB) error { } } - err = saveSyncState(tx, list.MaxCurrentTime, list.MaxUSN) + err = saveSyncState(tx, list.MaxCurrentTime, list.MaxUSN, list.UserMaxUSN) if err != nil { return errors.Wrap(err, "saving sync state") } @@ -625,7 +627,7 @@ func stepSync(ctx context.DnoteCtx, tx *database.DB, afterUSN int) error { } } - err = saveSyncState(tx, list.MaxCurrentTime, list.MaxUSN) + err = saveSyncState(tx, list.MaxCurrentTime, list.MaxUSN, list.UserMaxUSN) if err != nil { return errors.Wrap(err, "saving sync state") } @@ -681,13 +683,9 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, error) { } else { resp, err := client.CreateBook(ctx, book.Label) if err != nil { - // If we get a 409 conflict, it means another client uploaded data. - if isConflictError(err) { - log.Debug("409 conflict creating book %s, will retry after sync\n", book.Label) - isBehind = true - continue - } - return isBehind, errors.Wrap(err, "creating a book") + log.Debug("error creating book (will retry after stepSync): %v\n", err) + isBehind = true + continue } _, err = tx.Exec("UPDATE notes SET book_uuid = ? WHERE book_uuid = ?", resp.Book.UUID, book.UUID) @@ -792,8 +790,7 @@ func sendNotes(ctx context.DnoteCtx, tx *database.DB) (bool, error) { } else { resp, err := client.CreateNote(ctx, note.BookUUID, note.Body) if err != nil { - // If we get a 409 conflict, it means another client uploaded data. - log.Debug("error creating note (will retry after sync): %v\n", err) + log.Debug("error creating note (will retry after stepSync): %v\n", err) isBehind = true continue } @@ -903,10 +900,24 @@ func updateLastSyncAt(tx *database.DB, val int64) error { return nil } -func saveSyncState(tx *database.DB, serverTime int64, serverMaxUSN int) error { - if err := updateLastMaxUSN(tx, serverMaxUSN); err != nil { - return errors.Wrap(err, "updating last max usn") +func saveSyncState(tx *database.DB, serverTime int64, serverMaxUSN int, userMaxUSN int) error { + // Handle last_max_usn update based on server state: + // - If serverMaxUSN > 0: we got data, update to serverMaxUSN + // - If serverMaxUSN == 0 && userMaxUSN > 0: empty fragment (caught up), preserve existing + // - If serverMaxUSN == 0 && userMaxUSN == 0: empty server, reset to 0 + if serverMaxUSN > 0 { + if err := updateLastMaxUSN(tx, serverMaxUSN); err != nil { + return errors.Wrap(err, "updating last max usn") + } + } else if userMaxUSN == 0 { + // Server is empty, reset to 0 + if err := updateLastMaxUSN(tx, 0); err != nil { + return errors.Wrap(err, "updating last max usn") + } } + // else: empty fragment but server has data, preserve existing last_max_usn + + // Always update last_sync_at (we did communicate with server) if err := updateLastSyncAt(tx, serverTime); err != nil { return errors.Wrap(err, "updating last sync at") } diff --git a/pkg/cli/cmd/sync/sync_test.go b/pkg/cli/cmd/sync/sync_test.go index 828179f5..6c22e926 100644 --- a/pkg/cli/cmd/sync/sync_test.go +++ b/pkg/cli/cmd/sync/sync_test.go @@ -36,7 +36,6 @@ import ( "github.com/pkg/errors" ) - func TestProcessFragments(t *testing.T) { fragments := []client.SyncFragment{ { @@ -98,6 +97,7 @@ func TestProcessFragments(t *testing.T) { ExpungedNotes: map[string]bool{}, ExpungedBooks: map[string]bool{}, MaxUSN: 10, + UserMaxUSN: 10, MaxCurrentTime: 1550436136, } @@ -1796,41 +1796,132 @@ func TestMergeBook(t *testing.T) { } func TestSaveServerState(t *testing.T) { - // set up - db := database.InitTestMemoryDB(t) - testutils.LoginDB(t, db) + t.Run("with data received", func(t *testing.T) { + // set up + db := database.InitTestMemoryDB(t) + testutils.LoginDB(t, db) - database.MustExec(t, "inserting last synced at", db, "INSERT INTO system (key, value) VALUES (?, ?)", consts.SystemLastSyncAt, int64(1231108742)) - database.MustExec(t, "inserting last max usn", db, "INSERT INTO system (key, value) VALUES (?, ?)", consts.SystemLastMaxUSN, 8) + database.MustExec(t, "inserting last synced at", db, "INSERT INTO system (key, value) VALUES (?, ?)", consts.SystemLastSyncAt, int64(1231108742)) + database.MustExec(t, "inserting last max usn", db, "INSERT INTO system (key, value) VALUES (?, ?)", consts.SystemLastMaxUSN, 8) - // execute - tx, err := db.Begin() - if err != nil { - t.Fatal(errors.Wrap(err, "beginning a transaction").Error()) - } + // execute + tx, err := db.Begin() + if err != nil { + t.Fatal(errors.Wrap(err, "beginning a transaction").Error()) + } - serverTime := int64(1541108743) - serverMaxUSN := 100 + serverTime := int64(1541108743) + serverMaxUSN := 100 + userMaxUSN := 100 - err = saveSyncState(tx, serverTime, serverMaxUSN) - if err != nil { - tx.Rollback() - t.Fatal(errors.Wrap(err, "executing").Error()) - } + err = saveSyncState(tx, serverTime, serverMaxUSN, userMaxUSN) + if err != nil { + tx.Rollback() + t.Fatal(errors.Wrap(err, "executing").Error()) + } - tx.Commit() + tx.Commit() - // test - var lastSyncedAt int64 - var lastMaxUSN int + // test + var lastSyncedAt int64 + var lastMaxUSN int - database.MustScan(t, "getting system value", - db.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastSyncAt), &lastSyncedAt) - database.MustScan(t, "getting system value", - db.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastMaxUSN), &lastMaxUSN) + database.MustScan(t, "getting system value", + db.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastSyncAt), &lastSyncedAt) + database.MustScan(t, "getting system value", + db.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastMaxUSN), &lastMaxUSN) - assert.Equal(t, lastSyncedAt, serverTime, "last synced at mismatch") - assert.Equal(t, lastMaxUSN, serverMaxUSN, "last max usn mismatch") + assert.Equal(t, lastSyncedAt, serverTime, "last synced at mismatch") + assert.Equal(t, lastMaxUSN, serverMaxUSN, "last max usn mismatch") + }) + + t.Run("with empty fragment but server has data - preserves last_max_usn", func(t *testing.T) { + // This tests the fix for the infinite sync bug where empty fragments + // would reset last_max_usn to 0, causing the client to re-download all data. + // When serverMaxUSN=0 (no data in fragment) but userMaxUSN>0 (server has data), + // we're caught up and should preserve the existing last_max_usn. + + // set up + db := database.InitTestMemoryDB(t) + testutils.LoginDB(t, db) + + existingLastMaxUSN := 100 + database.MustExec(t, "inserting last synced at", db, "INSERT INTO system (key, value) VALUES (?, ?)", consts.SystemLastSyncAt, int64(1231108742)) + database.MustExec(t, "inserting last max usn", db, "INSERT INTO system (key, value) VALUES (?, ?)", consts.SystemLastMaxUSN, existingLastMaxUSN) + + // execute + tx, err := db.Begin() + if err != nil { + t.Fatal(errors.Wrap(err, "beginning a transaction").Error()) + } + + serverTime := int64(1541108743) + serverMaxUSN := 0 // Empty fragment (no data in this sync) + userMaxUSN := 150 // Server's actual max USN (higher than ours) + + err = saveSyncState(tx, serverTime, serverMaxUSN, userMaxUSN) + if err != nil { + tx.Rollback() + t.Fatal(errors.Wrap(err, "executing").Error()) + } + + tx.Commit() + + // test + var lastSyncedAt int64 + var lastMaxUSN int + + database.MustScan(t, "getting system value", + db.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastSyncAt), &lastSyncedAt) + database.MustScan(t, "getting system value", + db.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastMaxUSN), &lastMaxUSN) + + assert.Equal(t, lastSyncedAt, serverTime, "last synced at should be updated") + // last_max_usn should NOT be updated to 0, it should preserve the existing value + assert.Equal(t, lastMaxUSN, existingLastMaxUSN, "last max usn should be preserved when fragment is empty but server has data") + }) + + t.Run("with empty server - resets last_max_usn to 0", func(t *testing.T) { + // When both serverMaxUSN=0 and userMaxUSN=0, the server is truly empty + // and we should reset last_max_usn to 0. + + // set up + db := database.InitTestMemoryDB(t) + testutils.LoginDB(t, db) + + database.MustExec(t, "inserting last synced at", db, "INSERT INTO system (key, value) VALUES (?, ?)", consts.SystemLastSyncAt, int64(1231108742)) + database.MustExec(t, "inserting last max usn", db, "INSERT INTO system (key, value) VALUES (?, ?)", consts.SystemLastMaxUSN, 50) + + // execute + tx, err := db.Begin() + if err != nil { + t.Fatal(errors.Wrap(err, "beginning a transaction").Error()) + } + + serverTime := int64(1541108743) + serverMaxUSN := 0 // Empty fragment + userMaxUSN := 0 // Server is actually empty + + err = saveSyncState(tx, serverTime, serverMaxUSN, userMaxUSN) + if err != nil { + tx.Rollback() + t.Fatal(errors.Wrap(err, "executing").Error()) + } + + tx.Commit() + + // test + var lastSyncedAt int64 + var lastMaxUSN int + + database.MustScan(t, "getting system value", + db.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastSyncAt), &lastSyncedAt) + database.MustScan(t, "getting system value", + db.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastMaxUSN), &lastMaxUSN) + + assert.Equal(t, lastSyncedAt, serverTime, "last synced at should be updated") + assert.Equal(t, lastMaxUSN, 0, "last max usn should be reset to 0 when server is empty") + }) } // TestSendBooks tests that books are put to correct 'buckets' by running a test server and recording the