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
This commit is contained in:
Sung 2025-10-26 17:54:24 -07:00 committed by GitHub
commit 6314749263
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 354 additions and 10 deletions

View file

@ -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 {

View file

@ -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)

View file

@ -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)

View file

@ -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")
})
}

View file

@ -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")
}

View file

@ -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(),

View file

@ -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