diff --git a/pkg/cli/cmd/sync/sync.go b/pkg/cli/cmd/sync/sync.go index e7a03ffa..87b35f67 100644 --- a/pkg/cli/cmd/sync/sync.go +++ b/pkg/cli/cmd/sync/sync.go @@ -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() diff --git a/pkg/e2e/sync_test.go b/pkg/e2e/sync_test.go index 6e9a022d..cbd369cd 100644 --- a/pkg/e2e/sync_test.go +++ b/pkg/e2e/sync_test.go @@ -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) {