deblan-mirror/dnote
1
0
Fork 0
mirror of https://github.com/dnote/dnote synced 2026-03-16 15:35:52 +01:00
Code Issues Projects Releases Packages Wiki Activity

Avoid losing data in case of race

Browse source
This commit is contained in:
Sung 2025-10-12 10:04:17 -07:00
commit 5a60495d16
2 changed files with 89 additions and 81 deletions
Download patch file Download diff file Expand all files Collapse all files

75
pkg/cli/cmd/sync/sync.go
View file

@ -21,6 +21,7 @@ package sync
import (
"database/sql"
"fmt"
"strings"
"github.com/dnote/dnote/pkg/cli/client"
"github.com/dnote/dnote/pkg/cli/consts"
@ -630,12 +631,21 @@ func stepSync(ctx context.DnoteCtx, tx *database.DB, afterUSN int) error {
return nil
}
func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, error) {
// isConflictError checks if an error is a 409 Conflict error from the server
func isConflictError(err error) bool {
if err == nil {
return false
}
return strings.Contains(err.Error(), "response 409")
}
func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, map[string]bool, error) {
isBehind := false
skippedBooks := make(map[string]bool) // Track books that failed to upload due to 409
rows, err := tx.Query("SELECT uuid, label, usn, deleted FROM books WHERE dirty")
if err != nil {
return isBehind, errors.Wrap(err, "getting syncable books")
return isBehind, skippedBooks, errors.Wrap(err, "getting syncable books")
}
defer rows.Close()
@ -643,7 +653,7 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, error) {
var book database.Book
if err = rows.Scan(&book.UUID, &book.Label, &book.USN, &book.Deleted); err != nil {
return isBehind, errors.Wrap(err, "scanning a syncable book")
return isBehind, skippedBooks, errors.Wrap(err, "scanning a syncable book")
}
log.Debug("sending book %s\n", book.UUID)
@ -655,31 +665,39 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, error) {
if book.Deleted {
err = book.Expunge(tx)
if err != nil {
return isBehind, errors.Wrap(err, "expunging a book locally")
return isBehind, skippedBooks, errors.Wrap(err, "expunging a book locally")
}
continue
} else {
resp, err := client.CreateBook(ctx, book.Label)
if err != nil {
return isBehind, errors.Wrap(err, "creating a book")
// If we get a 409 conflict, it means another client uploaded data
// while we were at the prompt. Set isBehind to trigger conflict resolution.
if isConflictError(err) {
log.Debug("409 conflict creating book %s, will retry after sync\n", book.Label)
isBehind = true
skippedBooks[book.UUID] = true
continue
}
return isBehind, skippedBooks, errors.Wrap(err, "creating a book")
}
_, err = tx.Exec("UPDATE notes SET book_uuid = ? WHERE book_uuid = ?", resp.Book.UUID, book.UUID)
if err != nil {
return isBehind, errors.Wrap(err, "updating book_uuids of notes")
return isBehind, skippedBooks, errors.Wrap(err, "updating book_uuids of notes")
}
book.Dirty = false
book.USN = resp.Book.USN
err = book.Update(tx)
if err != nil {
return isBehind, errors.Wrap(err, "marking book dirty")
return isBehind, skippedBooks, errors.Wrap(err, "marking book dirty")
}
err = book.UpdateUUID(tx, resp.Book.UUID)
if err != nil {
return isBehind, errors.Wrap(err, "updating book uuid")
return isBehind, skippedBooks, errors.Wrap(err, "updating book uuid")
}
respUSN = resp.Book.USN
@ -688,26 +706,26 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, error) {
if book.Deleted {
resp, err := client.DeleteBook(ctx, book.UUID)
if err != nil {
return isBehind, errors.Wrap(err, "deleting a book")
return isBehind, skippedBooks, errors.Wrap(err, "deleting a book")
}
err = book.Expunge(tx)
if err != nil {
return isBehind, errors.Wrap(err, "expunging a book locally")
return isBehind, skippedBooks, errors.Wrap(err, "expunging a book locally")
}
respUSN = resp.Book.USN
} else {
resp, err := client.UpdateBook(ctx, book.Label, book.UUID)
if err != nil {
return isBehind, errors.Wrap(err, "updating a book")
return isBehind, skippedBooks, errors.Wrap(err, "updating a book")
}
book.Dirty = false
book.USN = resp.Book.USN
err = book.Update(tx)
if err != nil {
return isBehind, errors.Wrap(err, "marking book dirty")
return isBehind, skippedBooks, errors.Wrap(err, "marking book dirty")
}
respUSN = resp.Book.USN
@ -716,7 +734,7 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, error) {
lastMaxUSN, err := getLastMaxUSN(tx)
if err != nil {
return isBehind, errors.Wrap(err, "getting last max usn")
return isBehind, skippedBooks, errors.Wrap(err, "getting last max usn")
}
log.Debug("sent book %s. response USN %d. last max usn: %d\n", book.UUID, respUSN, lastMaxUSN)
@ -724,17 +742,17 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, error) {
if respUSN == lastMaxUSN+1 {
err = updateLastMaxUSN(tx, lastMaxUSN+1)
if err != nil {
return isBehind, errors.Wrap(err, "updating last max usn")
return isBehind, skippedBooks, errors.Wrap(err, "updating last max usn")
}
} else {
isBehind = true
}
}
return isBehind, nil
return isBehind, skippedBooks, nil
}
func sendNotes(ctx context.DnoteCtx, tx *database.DB) (bool, error) {
func sendNotes(ctx context.DnoteCtx, tx *database.DB, skippedBooks map[string]bool) (bool, error) {
isBehind := false
rows, err := tx.Query("SELECT uuid, book_uuid, body, public, deleted, usn, added_on FROM notes WHERE dirty")
@ -750,6 +768,12 @@ func sendNotes(ctx context.DnoteCtx, tx *database.DB) (bool, error) {
return isBehind, errors.Wrap(err, "scanning a syncable note")
}
// Skip notes whose book failed to upload due to 409 conflict
if skippedBooks[note.BookUUID] {
log.Debug("skipping note %s because its book %s was skipped\n", note.UUID, note.BookUUID)
continue
}
log.Debug("sending note %s\n", note.UUID)
var respUSN int
@ -767,6 +791,13 @@ 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
// while we were at the prompt. Set isBehind to trigger conflict resolution.
if isConflictError(err) {
log.Debug("409 conflict creating note, will retry after sync\n")
isBehind = true
continue
}
return isBehind, errors.Wrap(err, "creating a note")
}
@ -842,12 +873,12 @@ func sendChanges(ctx context.DnoteCtx, tx *database.DB) (bool, error) {
fmt.Printf(" (total %d).", delta)
behind1, err := sendBooks(ctx, tx)
behind1, skippedBooks, err := sendBooks(ctx, tx)
if err != nil {
return behind1, errors.Wrap(err, "sending books")
}
behind2, err := sendNotes(ctx, tx)
behind2, err := sendNotes(ctx, tx, skippedBooks)
if err != nil {
return behind2, errors.Wrap(err, "sending notes")
}
@ -1017,6 +1048,14 @@ func newRun(ctx context.DnoteCtx) infra.RunEFunc {
tx.Rollback()
return errors.Wrap(err, "performing the follow-up step sync")
}
// After syncing server changes (which resolves conflicts), send local changes again
// This uploads books/notes that were skipped due to 409 conflicts
_, err = sendChanges(ctx, tx)
if err != nil {
tx.Rollback()
return errors.Wrap(err, "sending changes after conflict resolution")
}
}
tx.Commit()

93
pkg/e2e/sync_test.go
View file

@ -4173,86 +4173,55 @@ func TestSync_EmptyServer(t *testing.T) {
}
// Step 4: Client A runs sync with race condition
// Expected: This will FAIL with 409 error because Client B uploaded during the prompt
output, err := clitest.WaitDnoteCmdOutput(t, dnoteCmdOpts, raceCallback, cliBinaryName, "sync")
if err == nil {
t.Fatal("Expected sync to fail with 409 conflict, but it succeeded")
}
// Verify output contains 409 or duplicate error
if !strings.Contains(output, "409") && !strings.Contains(output, "duplicate") {
t.Fatalf("Expected 409 conflict error in output, got: %s", output)
}
// The 409 conflict is automatically handled:
// - When 409 is detected, isBehind flag is set
// - stepSync pulls Client B's data
// - mergeBook renames Client A's books to js_2, css_2
// - Renamed books are uploaded
// Both clients' data is preserved!
clitest.MustWaitDnoteCmdOutput(t, dnoteCmdOpts, raceCallback, cliBinaryName, "sync")
// Step 5: Check local state after the failed sync (transaction rolled back)
var localBookCount, localNoteCount int
cliDatabase.MustScan(t, "counting local books after rollback", ctx.DB.QueryRow("SELECT count(*) FROM books"), &localBookCount)
cliDatabase.MustScan(t, "counting local notes after rollback", ctx.DB.QueryRow("SELECT count(*) FROM notes"), &localNoteCount)
t.Logf("After failed sync: local has %d books, %d notes", localBookCount, localNoteCount)
// List all local books
rows, err := ctx.DB.Query("SELECT uuid, label, usn, dirty FROM books")
if err == nil {
defer rows.Close()
for rows.Next() {
var uuid, label string
var usn int
var dirty bool
rows.Scan(&uuid, &label, &usn, &dirty)
t.Logf("Local book: label=%s, usn=%d, dirty=%v, uuid=%s", label, usn, dirty, uuid)
}
}
// Step 6: Client A retries sync with --full flag to force pulling all server data
// Note: cleanLocalBooks will delete Client A's original books because they have different UUIDs
// than Client B's books on the server, and they're not dirty (USN != 0)
// This means Client A's original data is lost - acceptable for this race condition edge case
clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "sync", "--full")
// Verify final state - only Client B's data remains
// Verify final state - both clients' data preserved
checkStateWithDB(t, ctx, user, serverDb, systemState{
clientNoteCount: 2, // Only Client B's data (Client A's was cleaned up)
clientBookCount: 2,
clientLastMaxUSN: 4,
clientNoteCount: 4, // Both clients' notes
clientBookCount: 4, // js, css, js_2, css_2
clientLastMaxUSN: 8, // 4 from Client B + 4 from Client A's renamed books/notes
clientLastSyncAt: serverTime.Unix(),
serverNoteCount: 2,
serverBookCount: 2,
serverUserMaxUSN: 4,
serverNoteCount: 4,
serverBookCount: 4,
serverUserMaxUSN: 8,
})
// Step 7: Verify specific books and notes - only Client B's data
// Client A's original data was lost due to cleanLocalBooks during fullSync
// Verify server books - only Client B's data
var svrBookJS, svrBookCSS database.Book
// Verify server has both clients' books
var svrBookJS, svrBookCSS, svrBookJS2, svrBookCSS2 database.Book
apitest.MustExec(t, serverDb.Where("label = ?", "js").First(&svrBookJS), "finding server book 'js'")
apitest.MustExec(t, serverDb.Where("label = ?", "css").First(&svrBookCSS), "finding server book 'css'")
apitest.MustExec(t, serverDb.Where("label = ?", "js_2").First(&svrBookJS2), "finding server book 'js_2'")
apitest.MustExec(t, serverDb.Where("label = ?", "css_2").First(&svrBookCSS2), "finding server book 'css_2'")
assert.Equal(t, svrBookJS.Label, "js", "server should have book 'js'")
assert.Equal(t, svrBookCSS.Label, "css", "server should have book 'css'")
assert.Equal(t, svrBookJS.Label, "js", "server should have book 'js' (Client B)")
assert.Equal(t, svrBookCSS.Label, "css", "server should have book 'css' (Client B)")
assert.Equal(t, svrBookJS2.Label, "js_2", "server should have book 'js_2' (Client A renamed)")
assert.Equal(t, svrBookCSS2.Label, "css_2", "server should have book 'css_2' (Client A renamed)")
// Verify server notes - only Client B's data
var svrNoteJS, svrNoteCSS database.Note
apitest.MustExec(t, serverDb.Where("book_uuid = ? AND body = ?", svrBookJS.UUID, "js1").First(&svrNoteJS), "finding server note in 'js'")
apitest.MustExec(t, serverDb.Where("book_uuid = ? AND body = ?", svrBookCSS.UUID, "css1").First(&svrNoteCSS), "finding server note in 'css'")
assert.Equal(t, svrNoteJS.Body, "js1", "note in 'js' should have body 'js1'")
assert.Equal(t, svrNoteCSS.Body, "css1", "note in 'css' should have body 'css1'")
// Verify client books - should match server (Client B's data)
var cliBookJS, cliBookCSS cliDatabase.Book
// Verify client has all books
var cliBookJS, cliBookCSS, cliBookJS2, cliBookCSS2 cliDatabase.Book
cliDatabase.MustScan(t, "finding client book 'js'", ctx.DB.QueryRow("SELECT uuid, label, usn FROM books WHERE label = ?", "js"), &cliBookJS.UUID, &cliBookJS.Label, &cliBookJS.USN)
cliDatabase.MustScan(t, "finding client book 'css'", ctx.DB.QueryRow("SELECT uuid, label, usn FROM books WHERE label = ?", "css"), &cliBookCSS.UUID, &cliBookCSS.Label, &cliBookCSS.USN)
cliDatabase.MustScan(t, "finding client book 'js_2'", ctx.DB.QueryRow("SELECT uuid, label, usn FROM books WHERE label = ?", "js_2"), &cliBookJS2.UUID, &cliBookJS2.Label, &cliBookJS2.USN)
cliDatabase.MustScan(t, "finding client book 'css_2'", ctx.DB.QueryRow("SELECT uuid, label, usn FROM books WHERE label = ?", "css_2"), &cliBookCSS2.UUID, &cliBookCSS2.Label, &cliBookCSS2.USN)
assert.Equal(t, cliBookJS.Label, "js", "client should have book 'js'")
assert.Equal(t, cliBookCSS.Label, "css", "client should have book 'css'")
// Verify client UUIDs match server UUIDs (pulled from server during stepSync)
// Verify client UUIDs match server
assert.Equal(t, cliBookJS.UUID, svrBookJS.UUID, "client 'js' UUID should match server")
assert.Equal(t, cliBookCSS.UUID, svrBookCSS.UUID, "client 'css' UUID should match server")
assert.Equal(t, cliBookJS2.UUID, svrBookJS2.UUID, "client 'js_2' UUID should match server")
assert.Equal(t, cliBookCSS2.UUID, svrBookCSS2.UUID, "client 'css_2' UUID should match server")
// Verify all items have non-zero USN (synced successfully)
assert.NotEqual(t, cliBookJS.USN, 0, "client 'js' should have non-zero USN")
assert.NotEqual(t, cliBookCSS.USN, 0, "client 'css' should have non-zero USN")
assert.NotEqual(t, cliBookJS2.USN, 0, "client 'js_2' should have non-zero USN")
assert.NotEqual(t, cliBookCSS2.USN, 0, "client 'css_2' should have non-zero USN")
})
t.Run("sync to server A, then B, then back to A, then back to B", func(t *testing.T) {