From 63147492638daf492c8814c6264845736775817c Mon Sep 17 00:00:00 2001 From: Sung <8265228+sungwoncho@users.noreply.github.com> Date: Sun, 26 Oct 2025 17:54:24 -0700 Subject: [PATCH] Improve empty server sync when multiple clients exist (#706) * Fix dbPath * Require full sync when after another client uploads to an empty server * Avoid orphan notes with empty sync --- pkg/cli/client/client.go | 3 + pkg/cli/cmd/sync/sync.go | 22 +++ pkg/cli/main.go | 24 ++- pkg/e2e/sync/empty_server_test.go | 282 ++++++++++++++++++++++++++++++ pkg/server/app/helpers.go | 20 ++- pkg/server/controllers/sync.go | 2 +- pkg/server/database/models.go | 11 +- 7 files changed, 354 insertions(+), 10 deletions(-) diff --git a/pkg/cli/client/client.go b/pkg/cli/client/client.go index 3fea9d21..083b9392 100644 --- a/pkg/cli/client/client.go +++ b/pkg/cli/client/client.go @@ -285,6 +285,9 @@ func GetSyncFragment(ctx context.DnoteCtx, afterUSN int) (GetSyncFragmentResp, e path := fmt.Sprintf("/v3/sync/fragment?%s", queryStr) res, err := doAuthorizedReq(ctx, "GET", path, "", nil) + if err != nil { + return GetSyncFragmentResp{}, errors.Wrap(err, "making the request") + } body, err := io.ReadAll(res.Body) if err != nil { diff --git a/pkg/cli/cmd/sync/sync.go b/pkg/cli/cmd/sync/sync.go index fcfc31fd..f8f2aec0 100644 --- a/pkg/cli/cmd/sync/sync.go +++ b/pkg/cli/cmd/sync/sync.go @@ -1173,6 +1173,28 @@ func newRun(ctx context.DnoteCtx) infra.RunEFunc { log.Debug("prepared empty server sync: marked %d books and %d notes as dirty\n", bookCount, noteCount) } + // If full sync will be triggered by FullSyncBefore (not manual --full flag), + // and client has more data than server, prepare local data for upload to avoid orphaning notes. + // The lastMaxUSN > syncState.MaxUSN check prevents duplicate uploads when switching + // back to a server that already has our data. + if !isFullSync && lastSyncAt < syncState.FullSyncBefore && lastMaxUSN > syncState.MaxUSN { + log.Debug("full sync triggered by FullSyncBefore: preparing local data for upload\n") + log.Debug("server.FullSyncBefore=%d, local.lastSyncAt=%d, local.MaxUSN=%d, server.MaxUSN=%d, books=%d, notes=%d\n", + syncState.FullSyncBefore, lastSyncAt, lastMaxUSN, syncState.MaxUSN, bookCount, noteCount) + + if err := prepareEmptyServerSync(tx); err != nil { + return errors.Wrap(err, "preparing local data for full sync") + } + + // Re-fetch lastMaxUSN after prepareEmptyServerSync + lastMaxUSN, err = getLastMaxUSN(tx) + if err != nil { + return errors.Wrap(err, "getting the last max_usn after prepare") + } + + log.Debug("prepared for full sync: marked %d books and %d notes as dirty\n", bookCount, noteCount) + } + var syncErr error if isFullSync || lastSyncAt < syncState.FullSyncBefore { syncErr = fullSync(ctx, tx) diff --git a/pkg/cli/main.go b/pkg/cli/main.go index 1ba97358..dfdac5a7 100644 --- a/pkg/cli/main.go +++ b/pkg/cli/main.go @@ -20,6 +20,7 @@ package main import ( "os" + "strings" "github.com/dnote/dnote/pkg/cli/infra" "github.com/dnote/dnote/pkg/cli/log" @@ -45,10 +46,29 @@ import ( var apiEndpoint string var versionTag = "master" +// parseDBPath extracts --dbPath flag value from command line arguments +// regardless of where it appears (before or after subcommand). +// Returns empty string if not found. +func parseDBPath(args []string) string { + for i, arg := range args { + // Handle --dbPath=value + if strings.HasPrefix(arg, "--dbPath=") { + return strings.TrimPrefix(arg, "--dbPath=") + } + // Handle --dbPath value + if arg == "--dbPath" && i+1 < len(args) { + return args[i+1] + } + } + return "" +} + func main() { // Parse flags early to get --dbPath before initializing database - root.GetRoot().ParseFlags(os.Args[1:]) - dbPath := root.GetDBPathFlag() + // We need to manually parse --dbPath because it can appear after the subcommand + // (e.g., "dnote sync --full --dbPath=./custom.db") and root.ParseFlags only + // parses flags before the subcommand. + dbPath := parseDBPath(os.Args[1:]) // Initialize context - defaultAPIEndpoint is used when creating new config file ctx, err := infra.Init(versionTag, apiEndpoint, dbPath) diff --git a/pkg/e2e/sync/empty_server_test.go b/pkg/e2e/sync/empty_server_test.go index a35086f9..ad7308e9 100644 --- a/pkg/e2e/sync/empty_server_test.go +++ b/pkg/e2e/sync/empty_server_test.go @@ -19,8 +19,11 @@ package sync import ( + "database/sql" "fmt" "io" + "os" + "strconv" "strings" "testing" @@ -446,4 +449,283 @@ func TestSync_EmptyServer(t *testing.T) { serverUserMaxUSN: 4, }) }) + + t.Run("two clients with identical copied database sync to empty server", func(t *testing.T) { + // Suppose we have two clients and server becomes empty (migration). + // After the first client sync to empty server, the second client should trigger full sync. + // Without the full sync, client2 will do step sync asking for changes after its stale USN, + // get nothing from server, and potentially orphan notes during full sync. + + // Step 1: Create client1 with data and sync to ORIGINAL server + env1 := setupTestEnv(t) + user := setupUserAndLogin(t, env1) + + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "add", "js", "-c", "js1") + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "add", "css", "-c", "css1") + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "sync") + // Add more data to create a higher USN + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "add", "go", "-c", "go1") + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "add", "rust", "-c", "rust1") + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "sync") + // Verify initial sync succeeded (now with 4 notes, 4 books, USN=8) + checkState(t, env1.DB, user, env1.ServerDB, systemState{ + clientNoteCount: 4, + clientBookCount: 4, + clientLastMaxUSN: 8, + clientLastSyncAt: serverTime.Unix(), + serverNoteCount: 4, + serverBookCount: 4, + serverUserMaxUSN: 8, + }) + + // Step 2: Create client2 by copying client1's database (simulating same DB on two devices) + env2 := setupTestEnv(t) + // Copy the database file from client1 to client2 + dbPath1 := env1.DB.Filepath + dbPath2 := env2.DB.Filepath + // Close both DBs before copying + env1.DB.Close() + env2.DB.Close() + + // Copy the database file + input, err := os.ReadFile(dbPath1) + if err != nil { + t.Fatal(errors.Wrap(err, "reading client1 database")) + } + if err := os.WriteFile(dbPath2, input, 0644); err != nil { + t.Fatal(errors.Wrap(err, "writing client2 database")) + } + + // Reopen databases + env1.DB, err = cliDatabase.Open(dbPath1) + if err != nil { + t.Fatal(errors.Wrap(err, "reopening client1 database")) + } + env2.DB, err = cliDatabase.Open(dbPath2) + if err != nil { + t.Fatal(errors.Wrap(err, "reopening client2 database")) + } + + // Verify client2 has identical data and USN=8 (stale) - same as client1 + // Note: at this point there's no server to compare against, we just check counts + var client2MaxUSN, client2NoteCount, client2BookCount int + cliDatabase.MustScan(t, "getting client2 maxUSN", + env2.DB.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastMaxUSN), + &client2MaxUSN) + cliDatabase.MustScan(t, "counting client2 notes", + env2.DB.QueryRow("SELECT count(*) FROM notes WHERE deleted = 0"), + &client2NoteCount) + cliDatabase.MustScan(t, "counting client2 books", + env2.DB.QueryRow("SELECT count(*) FROM books WHERE deleted = 0"), + &client2BookCount) + + assert.Equal(t, client2MaxUSN, 8, "client2 should have same maxUSN=8 as client1") + assert.Equal(t, client2NoteCount, 4, "client2 should have 4 notes") + assert.Equal(t, client2BookCount, 4, "client2 should have 4 books") + + // Step 3: Switch client1 to new empty server + switchToEmptyServer(t, &env1) + + // Point client2 to the same new server + env2.Server = env1.Server + env2.ServerDB = env1.ServerDB + + // Update client2's API endpoint config to point to env1's server + apiEndpoint := fmt.Sprintf("%s/api", env1.Server.URL) + updateConfigAPIEndpoint(t, env2.TmpDir, apiEndpoint) + + // Create same user on new server + user = setupUserAndLogin(t, env1) + + // Setup session for client2 (same user, same server) + login(t, env2.DB, env2.ServerDB, user) + + // Step 4: Client1 syncs ONLY FIRST 2 BOOKS to empty server (simulates partial upload) + // This creates the stale USN scenario: client2 has maxUSN=8, but server will only have maxUSN=4 + clitest.MustWaitDnoteCmd(t, env1.CmdOpts, clitest.UserConfirmEmptyServerSync, + cliBinaryName, "sync") + + // Delete the last 2 books from client1 to prevent them being on server + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "remove", "go", "-y") + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "remove", "rust", "-y") + + // Sync deletions to server + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "sync") + + // Verify server has 2 active books/notes (go/rust deleted) + var serverNoteCount, serverBookCount int64 + apitest.MustExec(t, env1.ServerDB.Model(&database.Note{}).Where("deleted = ?", false).Count(&serverNoteCount), "counting active server notes") + apitest.MustExec(t, env1.ServerDB.Model(&database.Book{}).Where("deleted = ?", false).Count(&serverBookCount), "counting active server books") + assert.Equal(t, int(serverNoteCount), 2, "server should have 2 active notes (go/rust deleted)") + assert.Equal(t, int(serverBookCount), 2, "server should have 2 active books (go/rust deleted)") + + // Step 5: Client2 syncs + // CRITICAL: Client2 has lastMaxUSN=8 (from copied DB), but server's max_usn is now ~8 but only has 2 books + // Client2 will ask for changes after USN=8, get nothing, then try to upload its 4 books + // This should trigger the orphaned notes scenario or require full sync + clitest.RunDnoteCmd(t, env2.CmdOpts, cliBinaryName, "sync") + + // Step 6: Verify client2 has all data and NO orphaned notes + var orphanedCount int + cliDatabase.MustScan(t, "checking for orphaned notes", + env2.DB.QueryRow(` + SELECT COUNT(*) FROM notes + WHERE deleted = 0 + AND book_uuid NOT IN (SELECT uuid FROM books WHERE deleted = 0) + `), &orphanedCount) + + assert.Equal(t, orphanedCount, 0, "client2 should have no orphaned notes") + + // Verify client2 converged with server state + // Note: checkState counts ALL records (including deleted ones) + // During full sync, cleanLocalBooks/cleanLocalNotes DELETE local records not on server + // So client2 ends up with only the 2 active books/notes + // Server has 4 total (2 active + 2 deleted) + var client2LastMaxUSN, client2LastSyncAt int + var serverUserMaxUSN int + cliDatabase.MustScan(t, "getting client2 lastMaxUSN", + env2.DB.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastMaxUSN), + &client2LastMaxUSN) + var lastSyncAtStr string + cliDatabase.MustScan(t, "getting client2 lastSyncAt", + env2.DB.QueryRow("SELECT value FROM system WHERE key = ?", consts.SystemLastSyncAt), + &lastSyncAtStr) + lastSyncAtInt, _ := strconv.ParseInt(lastSyncAtStr, 10, 64) + client2LastSyncAt = int(lastSyncAtInt) + + apitest.MustExec(t, env2.ServerDB.Table("users").Select("max_usn").Where("id = ?", user.ID).Scan(&serverUserMaxUSN), "getting server user max_usn") + + checkState(t, env2.DB, user, env2.ServerDB, systemState{ + clientNoteCount: 2, // Only active notes (deleted ones removed by cleanLocalNotes) + clientBookCount: 2, // Only active books (deleted ones removed by cleanLocalBooks) + clientLastMaxUSN: client2LastMaxUSN, + clientLastSyncAt: int64(client2LastSyncAt), + serverNoteCount: 4, // 2 active + 2 deleted + serverBookCount: 4, // 2 active + 2 deleted + serverUserMaxUSN: serverUserMaxUSN, + }) + + // Verify both clients have the expected books (css, js only - go/rust deleted) + var client1BookCSS, client1BookJS, client2BookCSS, client2BookJS cliDatabase.Book + cliDatabase.MustScan(t, "finding client1 book 'css'", + env1.DB.QueryRow("SELECT uuid, label FROM books WHERE label = ? AND deleted = 0", "css"), + &client1BookCSS.UUID, &client1BookCSS.Label) + cliDatabase.MustScan(t, "finding client1 book 'js'", + env1.DB.QueryRow("SELECT uuid, label FROM books WHERE label = ? AND deleted = 0", "js"), + &client1BookJS.UUID, &client1BookJS.Label) + cliDatabase.MustScan(t, "finding client2 book 'css'", + env2.DB.QueryRow("SELECT uuid, label FROM books WHERE label = ? AND deleted = 0", "css"), + &client2BookCSS.UUID, &client2BookCSS.Label) + cliDatabase.MustScan(t, "finding client2 book 'js'", + env2.DB.QueryRow("SELECT uuid, label FROM books WHERE label = ? AND deleted = 0", "js"), + &client2BookJS.UUID, &client2BookJS.Label) + + assert.Equal(t, client1BookCSS.Label, "css", "client1 should have css book") + assert.Equal(t, client1BookJS.Label, "js", "client1 should have js book") + assert.Equal(t, client2BookCSS.Label, "css", "client2 should have css book") + assert.Equal(t, client2BookJS.Label, "js", "client2 should have js book") + + // Verify go and rust books are deleted/absent on both clients + var client2BookGo, client2BookRust cliDatabase.Book + errGo := env2.DB.QueryRow("SELECT uuid, label FROM books WHERE label = ? AND deleted = 0", "go").Scan(&client2BookGo.UUID, &client2BookGo.Label) + assert.Equal(t, errGo, sql.ErrNoRows, "client2 should not have non-deleted 'go' book") + errRust := env2.DB.QueryRow("SELECT uuid, label FROM books WHERE label = ? AND deleted = 0", "rust").Scan(&client2BookRust.UUID, &client2BookRust.Label) + assert.Equal(t, errRust, sql.ErrNoRows, "client2 should not have non-deleted 'rust' book") + }) + + t.Run("client with local data syncs after another client uploads to empty server - should not orphan notes", func(t *testing.T) { + // This test reproduces the scenario where: + // 1. Client1 has local data and syncs to original server + // 2. Client2 has DIFFERENT local data and syncs to SAME original server + // 3. Both clients switch to NEW empty server + // 4. Client1 uploads to the new empty server (sets FullSyncBefore) + // 5. Client2 syncs - should trigger full sync AND upload its local data + // WITHOUT orphaning notes due to cleanLocalBooks deleting them first + + // Step 1: Create client1 with local data on original server + env1 := setupTestEnv(t) + user := setupUserAndLogin(t, env1) + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "add", "client1-book", "-c", "client1-note") + clitest.RunDnoteCmd(t, env1.CmdOpts, cliBinaryName, "sync") + + // Step 2: Create client2 with DIFFERENT local data on SAME original server + env2 := setupTestEnv(t) + // Point env2 to env1's server (the original server) + env2.Server = env1.Server + env2.ServerDB = env1.ServerDB + apiEndpoint := fmt.Sprintf("%s/api", env1.Server.URL) + updateConfigAPIEndpoint(t, env2.TmpDir, apiEndpoint) + + // Login client2 to the same server + login(t, env2.DB, env2.ServerDB, user) + clitest.RunDnoteCmd(t, env2.CmdOpts, cliBinaryName, "add", "client2-book", "-c", "client2-note") + clitest.RunDnoteCmd(t, env2.CmdOpts, cliBinaryName, "sync") + + // Step 3: Both clients switch to NEW empty server + switchToEmptyServer(t, &env1) + env2.Server = env1.Server + env2.ServerDB = env1.ServerDB + apiEndpoint = fmt.Sprintf("%s/api", env1.Server.URL) + updateConfigAPIEndpoint(t, env2.TmpDir, apiEndpoint) + + // Create same user on new server + user = setupUserAndLogin(t, env1) + login(t, env2.DB, env2.ServerDB, user) + + // Step 4: Client1 uploads to empty server + clitest.MustWaitDnoteCmd(t, env1.CmdOpts, clitest.UserConfirmEmptyServerSync, cliBinaryName, "sync") + + // Verify server has client1's data and FullSyncBefore is set + var serverUser database.User + apitest.MustExec(t, env1.ServerDB.Where("id = ?", user.ID).First(&serverUser), "getting server user state") + assert.Equal(t, serverUser.MaxUSN > 0, true, "server should have data after client1 upload") + assert.Equal(t, serverUser.FullSyncBefore > 0, true, "server should have FullSyncBefore set") + + // Step 5: Client2 syncs - should trigger full sync due to FullSyncBefore + // CRITICAL: Client2 has local data (client2-book, client2-note) that should be uploaded + // Without the fix, cleanLocalBooks will delete client2-book before upload, orphaning client2-note + clitest.RunDnoteCmd(t, env2.CmdOpts, cliBinaryName, "sync") + + // Step 6: Verify NO orphaned notes on client2 + var orphanedCount int + cliDatabase.MustScan(t, "checking for orphaned notes on client2", + env2.DB.QueryRow(` + SELECT COUNT(*) FROM notes + WHERE deleted = 0 + AND book_uuid NOT IN (SELECT uuid FROM books WHERE deleted = 0) + `), &orphanedCount) + + assert.Equal(t, orphanedCount, 0, "client2 should have no orphaned notes after sync") + + // Step 7: Verify client2's data was uploaded to server + var client2BookOnServer database.Book + err := env2.ServerDB.Where("label = ? AND deleted = ?", "client2-book", false).First(&client2BookOnServer).Error + assert.Equal(t, err, nil, "client2-book should exist on server") + + var client2NoteOnServer database.Note + err = env2.ServerDB.Where("body = ? AND deleted = ?", "client2-note", false).First(&client2NoteOnServer).Error + assert.Equal(t, err, nil, "client2-note should exist on server") + + // Step 8: Verify server has data from BOTH clients + // Note: Both clients had synced to original server, so they each have 2 books + 2 notes locally. + // When switching to new empty server, client1 uploads 2 books + 2 notes (USN 1-4). + // Client2 then does full sync, downloads client1's uploads, marks its local data as dirty, + // and uploads its version of the same 2 books + 2 notes with potentially different UUIDs. + // The exact count depends on UUID conflict resolution, but we verify both original books exist. + var serverBookCount, serverNoteCount int64 + apitest.MustExec(t, env2.ServerDB.Model(&database.Book{}).Where("deleted = ?", false).Count(&serverBookCount), "counting active server books") + apitest.MustExec(t, env2.ServerDB.Model(&database.Note{}).Where("deleted = ?", false).Count(&serverNoteCount), "counting active server notes") + + // The main assertion: both original client books should exist + var client1BookExists, client2BookExists bool + err = env2.ServerDB.Model(&database.Book{}).Where("label = ? AND deleted = ?", "client1-book", false).First(&database.Book{}).Error + client1BookExists = (err == nil) + err = env2.ServerDB.Model(&database.Book{}).Where("label = ? AND deleted = ?", "client2-book", false).First(&database.Book{}).Error + client2BookExists = (err == nil) + + assert.Equal(t, client1BookExists, true, "server should have client1-book") + assert.Equal(t, client2BookExists, true, "server should have client2-book") + assert.Equal(t, serverBookCount >= 2, true, "server should have at least 2 books") + assert.Equal(t, serverNoteCount >= 2, true, "server should have at least 2 notes") + }) } diff --git a/pkg/server/app/helpers.go b/pkg/server/app/helpers.go index 3941b2cb..361db6cb 100644 --- a/pkg/server/app/helpers.go +++ b/pkg/server/app/helpers.go @@ -19,19 +19,35 @@ package app import ( + "time" + "github.com/dnote/dnote/pkg/server/database" - "gorm.io/gorm" "github.com/pkg/errors" + "gorm.io/gorm" ) // incrementUserUSN increment the given user's max_usn by 1 // and returns the new, incremented max_usn func incrementUserUSN(tx *gorm.DB, userID int) (int, error) { + // First, get the current max_usn to detect transition from empty server + var user database.User + if err := tx.Select("max_usn, full_sync_before").Where("id = ?", userID).First(&user).Error; err != nil { + return 0, errors.Wrap(err, "getting current user state") + } + + // If transitioning from empty server (MaxUSN=0) to non-empty (MaxUSN=1), + // set full_sync_before to current timestamp to force all other clients to full sync + if user.MaxUSN == 0 && user.FullSyncBefore == 0 { + currentTime := time.Now().Unix() + if err := tx.Table("users").Where("id = ?", userID).Update("full_sync_before", currentTime).Error; err != nil { + return 0, errors.Wrap(err, "setting full_sync_before on empty server transition") + } + } + if err := tx.Table("users").Where("id = ?", userID).Update("max_usn", gorm.Expr("max_usn + 1")).Error; err != nil { return 0, errors.Wrap(err, "incrementing user max_usn") } - var user database.User if err := tx.Select("max_usn").Where("id = ?", userID).First(&user).Error; err != nil { return 0, errors.Wrap(err, "getting the updated user max_usn") } diff --git a/pkg/server/controllers/sync.go b/pkg/server/controllers/sync.go index e93be7cd..3d7d2608 100644 --- a/pkg/server/controllers/sync.go +++ b/pkg/server/controllers/sync.go @@ -301,7 +301,7 @@ func (s *Sync) GetSyncState(w http.ResponseWriter, r *http.Request) { } response := GetSyncStateResp{ - FullSyncBefore: fullSyncBefore, + FullSyncBefore: int(user.FullSyncBefore), MaxUSN: user.MaxUSN, // TODO: exposing server time means we probably shouldn't seed random generator with time? CurrentTime: s.app.Clock.Now().Unix(), diff --git a/pkg/server/database/models.go b/pkg/server/database/models.go index 576dee7f..f21e8636 100644 --- a/pkg/server/database/models.go +++ b/pkg/server/database/models.go @@ -61,11 +61,12 @@ type Note struct { // User is a model for a user type User struct { Model - UUID string `json:"uuid" gorm:"type:text;index"` - Email NullString `gorm:"index"` - Password NullString `json:"-"` - LastLoginAt *time.Time `json:"-"` - MaxUSN int `json:"-" gorm:"default:0"` + UUID string `json:"uuid" gorm:"type:text;index"` + Email NullString `gorm:"index"` + Password NullString `json:"-"` + LastLoginAt *time.Time `json:"-"` + MaxUSN int `json:"-" gorm:"default:0"` + FullSyncBefore int64 `json:"-" gorm:"default:0"` } // Token is a model for a token