From 11fbbc583b2c7aff9458a8b32885d21b31db16d9 Mon Sep 17 00:00:00 2001 From: Sung Date: Sun, 12 Oct 2025 10:04:24 -0700 Subject: [PATCH] Simplify --- pkg/cli/client/client.go | 20 +++- pkg/cli/client/client_test.go | 18 ++++ pkg/cli/cmd/root/root.go | 2 +- pkg/cli/cmd/sync/sync.go | 77 +++++++-------- pkg/cli/main_test.go | 4 +- pkg/cli/testutils/main.go | 160 ++++++++++++++++++++------------ pkg/e2e/sync_test.go | 170 +++++++++++++++++++++------------- 7 files changed, 284 insertions(+), 167 deletions(-) diff --git a/pkg/cli/client/client.go b/pkg/cli/client/client.go index b24c432e..021afe7d 100644 --- a/pkg/cli/client/client.go +++ b/pkg/cli/client/client.go @@ -42,6 +42,21 @@ var ErrInvalidLogin = errors.New("wrong credentials") // ErrContentTypeMismatch is an error for invalid credentials for login var ErrContentTypeMismatch = errors.New("content type mismatch") +// HTTPError represents an HTTP error response from the server +type HTTPError struct { + StatusCode int + Message string +} + +func (e *HTTPError) Error() string { + return fmt.Sprintf(`response %d "%s"`, e.StatusCode, e.Message) +} + +// IsConflict returns true if the error is a 409 Conflict error +func (e *HTTPError) IsConflict() bool { + return e.StatusCode == 409 +} + var contentTypeApplicationJSON = "application/json" var contentTypeNone = "" @@ -137,7 +152,10 @@ func checkRespErr(res *http.Response) error { } bodyStr := string(body) - return errors.Errorf(`response %d "%s"`, res.StatusCode, strings.TrimRight(bodyStr, "\n")) + return &HTTPError{ + StatusCode: res.StatusCode, + Message: strings.TrimRight(bodyStr, "\n"), + } } func checkContentType(res *http.Response, options *requestOptions) error { diff --git a/pkg/cli/client/client_test.go b/pkg/cli/client/client_test.go index c5467ad6..31b6d6c8 100644 --- a/pkg/cli/client/client_test.go +++ b/pkg/cli/client/client_test.go @@ -205,3 +205,21 @@ func TestRateLimitedTransport(t *testing.T) { assert.Equal(t, int(requestCount.Load()), 10, "request count mismatch") } + +func TestHTTPError(t *testing.T) { + t.Run("IsConflict returns true for 409", func(t *testing.T) { + conflictErr := &HTTPError{ + StatusCode: 409, + Message: "Conflict", + } + + assert.Equal(t, conflictErr.IsConflict(), true, "IsConflict() should return true for 409") + + notFoundErr := &HTTPError{ + StatusCode: 404, + Message: "Not Found", + } + + assert.Equal(t, notFoundErr.IsConflict(), false, "IsConflict() should return false for 404") + }) +} diff --git a/pkg/cli/cmd/root/root.go b/pkg/cli/cmd/root/root.go index 2ff893b6..750629e4 100644 --- a/pkg/cli/cmd/root/root.go +++ b/pkg/cli/cmd/root/root.go @@ -35,7 +35,7 @@ var root = &cobra.Command{ } func init() { - root.PersistentFlags().StringVar(&apiEndpointFlag, "api-endpoint", "", "override API endpoint") + root.PersistentFlags().StringVar(&apiEndpointFlag, "api-endpoint", "", "the API endpoint to connect to (defaults to value in config)") } // GetRoot returns the root command diff --git a/pkg/cli/cmd/sync/sync.go b/pkg/cli/cmd/sync/sync.go index 87b35f67..ff8e8a5f 100644 --- a/pkg/cli/cmd/sync/sync.go +++ b/pkg/cli/cmd/sync/sync.go @@ -21,7 +21,6 @@ package sync import ( "database/sql" "fmt" - "strings" "github.com/dnote/dnote/pkg/cli/client" "github.com/dnote/dnote/pkg/cli/consts" @@ -636,16 +635,21 @@ func isConflictError(err error) bool { if err == nil { return false } - return strings.Contains(err.Error(), "response 409") + + var httpErr *client.HTTPError + if errors.As(err, &httpErr) { + return httpErr.IsConflict() + } + + return false } -func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, map[string]bool, error) { +func sendBooks(ctx context.DnoteCtx, tx *database.DB) (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, skippedBooks, errors.Wrap(err, "getting syncable books") + return isBehind, errors.Wrap(err, "getting syncable books") } defer rows.Close() @@ -653,7 +657,7 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, map[string]bool, er var book database.Book if err = rows.Scan(&book.UUID, &book.Label, &book.USN, &book.Deleted); err != nil { - return isBehind, skippedBooks, errors.Wrap(err, "scanning a syncable book") + return isBehind, errors.Wrap(err, "scanning a syncable book") } log.Debug("sending book %s\n", book.UUID) @@ -665,39 +669,37 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, map[string]bool, er if book.Deleted { err = book.Expunge(tx) if err != nil { - return isBehind, skippedBooks, errors.Wrap(err, "expunging a book locally") + return isBehind, errors.Wrap(err, "expunging a book locally") } continue } else { resp, err := client.CreateBook(ctx, book.Label) 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 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 - skippedBooks[book.UUID] = true continue } - return isBehind, skippedBooks, errors.Wrap(err, "creating a book") + return isBehind, 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, skippedBooks, errors.Wrap(err, "updating book_uuids of notes") + return isBehind, 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, skippedBooks, errors.Wrap(err, "marking book dirty") + return isBehind, errors.Wrap(err, "marking book dirty") } err = book.UpdateUUID(tx, resp.Book.UUID) if err != nil { - return isBehind, skippedBooks, errors.Wrap(err, "updating book uuid") + return isBehind, errors.Wrap(err, "updating book uuid") } respUSN = resp.Book.USN @@ -706,26 +708,26 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, map[string]bool, er if book.Deleted { resp, err := client.DeleteBook(ctx, book.UUID) if err != nil { - return isBehind, skippedBooks, errors.Wrap(err, "deleting a book") + return isBehind, errors.Wrap(err, "deleting a book") } err = book.Expunge(tx) if err != nil { - return isBehind, skippedBooks, errors.Wrap(err, "expunging a book locally") + return isBehind, 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, skippedBooks, errors.Wrap(err, "updating a book") + return isBehind, errors.Wrap(err, "updating a book") } book.Dirty = false book.USN = resp.Book.USN err = book.Update(tx) if err != nil { - return isBehind, skippedBooks, errors.Wrap(err, "marking book dirty") + return isBehind, errors.Wrap(err, "marking book dirty") } respUSN = resp.Book.USN @@ -734,7 +736,7 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, map[string]bool, er lastMaxUSN, err := getLastMaxUSN(tx) if err != nil { - return isBehind, skippedBooks, errors.Wrap(err, "getting last max usn") + return isBehind, errors.Wrap(err, "getting last max usn") } log.Debug("sent book %s. response USN %d. last max usn: %d\n", book.UUID, respUSN, lastMaxUSN) @@ -742,17 +744,17 @@ func sendBooks(ctx context.DnoteCtx, tx *database.DB) (bool, map[string]bool, er if respUSN == lastMaxUSN+1 { err = updateLastMaxUSN(tx, lastMaxUSN+1) if err != nil { - return isBehind, skippedBooks, errors.Wrap(err, "updating last max usn") + return isBehind, errors.Wrap(err, "updating last max usn") } } else { isBehind = true } } - return isBehind, skippedBooks, nil + return isBehind, nil } -func sendNotes(ctx context.DnoteCtx, tx *database.DB, skippedBooks map[string]bool) (bool, error) { +func sendNotes(ctx context.DnoteCtx, tx *database.DB) (bool, error) { isBehind := false rows, err := tx.Query("SELECT uuid, book_uuid, body, public, deleted, usn, added_on FROM notes WHERE dirty") @@ -768,12 +770,6 @@ func sendNotes(ctx context.DnoteCtx, tx *database.DB, skippedBooks map[string]bo 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 @@ -791,14 +787,10 @@ func sendNotes(ctx context.DnoteCtx, tx *database.DB, skippedBooks map[string]bo } 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") + // If we get a 409 conflict, it means another client uploaded data. + log.Debug("error creating note (will retry after sync): %v\n", err) + isBehind = true + continue } note.Dirty = false @@ -873,12 +865,12 @@ func sendChanges(ctx context.DnoteCtx, tx *database.DB) (bool, error) { fmt.Printf(" (total %d).", delta) - behind1, skippedBooks, err := sendBooks(ctx, tx) + behind1, err := sendBooks(ctx, tx) if err != nil { return behind1, errors.Wrap(err, "sending books") } - behind2, err := sendNotes(ctx, tx, skippedBooks) + behind2, err := sendNotes(ctx, tx) if err != nil { return behind2, errors.Wrap(err, "sending notes") } @@ -976,13 +968,14 @@ func newRun(ctx context.DnoteCtx) infra.RunEFunc { return errors.Wrap(err, "counting local notes") } - // Only trigger empty server prompt if client has previously synced (lastMaxUSN > 0) - // This distinguishes between first sync (lastMaxUSN=0) and server switch (lastMaxUSN>0) + // If a client has previously synced (lastMaxUSN > 0) but the server was never synced to (MaxUSN = 0), + // and the client has undeleted books or notes, allow to upload all data to the server. + // The client might have switched servers or the server might need to be restored for any reasons. if syncState.MaxUSN == 0 && lastMaxUSN > 0 && (bookCount > 0 || noteCount > 0) { log.Debug("empty server detected: server.MaxUSN=%d, local.MaxUSN=%d, books=%d, notes=%d\n", syncState.MaxUSN, lastMaxUSN, bookCount, noteCount) - log.Warnf("The server is empty but you have local data.\n") + log.Warnf("The server is empty but you have local data. Maybe you switched servers?\n") log.Debug("server state: MaxUSN = 0 (empty)\n") log.Debug("local state: %d books, %d notes (MaxUSN = %d)\n", bookCount, noteCount, lastMaxUSN) @@ -997,6 +990,8 @@ func newRun(ctx context.DnoteCtx) infra.RunEFunc { return errors.New("sync cancelled by user") } + fmt.Println() // Add newline after confirmation. + if err := prepareEmptyServerSync(tx); err != nil { return errors.Wrap(err, "preparing for empty server sync") } diff --git a/pkg/cli/main_test.go b/pkg/cli/main_test.go index bef14de0..151d4cbf 100644 --- a/pkg/cli/main_test.go +++ b/pkg/cli/main_test.go @@ -349,7 +349,7 @@ func TestRemoveNote(t *testing.T) { if tc.yesFlag { testutils.RunDnoteCmd(t, opts, binaryName, "remove", "-y", "1") } else { - testutils.MustWaitDnoteCmd(t, opts, testutils.UserConfirm, binaryName, "remove", "1") + testutils.MustWaitDnoteCmd(t, opts, testutils.ConfirmRemoveNote, binaryName, "remove", "1") } defer testutils.RemoveDir(t, testDir) @@ -436,7 +436,7 @@ func TestRemoveBook(t *testing.T) { if tc.yesFlag { testutils.RunDnoteCmd(t, opts, binaryName, "remove", "-y", "js") } else { - testutils.MustWaitDnoteCmd(t, opts, testutils.UserConfirm, binaryName, "remove", "js") + testutils.MustWaitDnoteCmd(t, opts, testutils.ConfirmRemoveBook, binaryName, "remove", "js") } defer testutils.RemoveDir(t, testDir) diff --git a/pkg/cli/testutils/main.go b/pkg/cli/testutils/main.go index abd49d9f..9b4e945d 100644 --- a/pkg/cli/testutils/main.go +++ b/pkg/cli/testutils/main.go @@ -38,6 +38,16 @@ import ( "github.com/pkg/errors" ) +// Prompts for user input +const ( + PromptRemoveNote = "remove this note?" + PromptDeleteBook = "delete book" + PromptEmptyServer = "The server is empty but you have local data" +) + +// Timeout for waiting for prompts in tests +const promptTimeout = 10 * time.Second + // Login simulates a logged in user by inserting credentials in the local database func Login(t *testing.T, ctx *context.DnoteCtx) { db := ctx.DB @@ -154,8 +164,8 @@ func RunDnoteCmd(t *testing.T, opts RunDnoteCmdOptions, binaryName string, arg . t.Logf("\n%s", stdout) } -// WaitDnoteCmdOutput runs a dnote command and passes stdout to the callback. -func WaitDnoteCmdOutput(t *testing.T, opts RunDnoteCmdOptions, runFunc func(io.WriteCloser, io.Reader) error, binaryName string, arg ...string) (string, error) { +// WaitDnoteCmd runs a dnote command and passes stdout to the callback. +func WaitDnoteCmd(t *testing.T, opts RunDnoteCmdOptions, runFunc func(io.Reader, io.WriteCloser) error, binaryName string, arg ...string) (string, error) { t.Logf("running: %s %s", binaryName, strings.Join(arg, " ")) binaryPath, err := filepath.Abs(binaryName) @@ -187,7 +197,7 @@ func WaitDnoteCmdOutput(t *testing.T, opts RunDnoteCmdOptions, runFunc func(io.W var output bytes.Buffer tee := io.TeeReader(stdout, &output) - err = runFunc(stdin, tee) + err = runFunc(tee, stdin) if err != nil { t.Logf("\n%s", output.String()) return output.String(), errors.Wrap(err, "running callback") @@ -204,81 +214,117 @@ func WaitDnoteCmdOutput(t *testing.T, opts RunDnoteCmdOptions, runFunc func(io.W return output.String(), nil } -func MustWaitDnoteCmdOutput(t *testing.T, opts RunDnoteCmdOptions, runFunc func(io.WriteCloser, io.Reader) error, binaryName string, arg ...string) string { - output, err := WaitDnoteCmdOutput(t, opts, runFunc, binaryName, arg...) - if err != nil { - t.Fatal(err) - } - - return output -} - -// WaitDnoteCmd runs a dnote command and waits until the command is exited. -// Returns the stdout output as a string and any error that occurred. -func WaitDnoteCmd(t *testing.T, opts RunDnoteCmdOptions, runFunc func(io.WriteCloser) error, binaryName string, arg ...string) (string, error) { - return WaitDnoteCmdOutput(t, opts, func(stdin io.WriteCloser, stdout io.Reader) error { - return runFunc(stdin) - }, binaryName, arg...) -} - -// MustWaitDnoteCmd runs a dnote command and waits until the command is exited. -// If there is an error, it fails the test. Returns the stdout output. -func MustWaitDnoteCmd(t *testing.T, opts RunDnoteCmdOptions, runFunc func(io.WriteCloser) error, binaryName string, arg ...string) string { +func MustWaitDnoteCmd(t *testing.T, opts RunDnoteCmdOptions, runFunc func(io.Reader, io.WriteCloser) error, binaryName string, arg ...string) string { output, err := WaitDnoteCmd(t, opts, runFunc, binaryName, arg...) if err != nil { t.Fatal(err) } + return output } -// UserConfirm simulates confirmation from the user by writing to stdin -func UserConfirmOutput(stdin io.WriteCloser, stdout io.Reader, expectedPrompt string) error { - scanner := bufio.NewScanner(stdout) - found := false - for scanner.Scan() { - line := scanner.Text() - if strings.Contains(line, expectedPrompt) { - found = true - break +// waitForPrompt waits for an expected prompt to appear in stdout with a timeout. +// Returns an error if the prompt is not found within the timeout period. +// Handles prompts with or without newlines by reading character by character. +func waitForPrompt(stdout io.Reader, expectedPrompt string, timeout time.Duration) error { + type result struct { + found bool + err error + } + resultCh := make(chan result, 1) + + go func() { + reader := bufio.NewReader(stdout) + var buffer strings.Builder + found := false + + for { + b, err := reader.ReadByte() + if err != nil { + resultCh <- result{found: found, err: err} + return + } + + buffer.WriteByte(b) + if strings.Contains(buffer.String(), expectedPrompt) { + found = true + break + } } + + resultCh <- result{found: found, err: nil} + }() + + select { + case res := <-resultCh: + if res.err != nil && res.err != io.EOF { + return errors.Wrap(res.err, "reading stdout") + } + if !res.found { + return errors.Errorf("expected prompt '%s' not found in stdout", expectedPrompt) + } + return nil + case <-time.After(timeout): + return errors.Errorf("timeout waiting for prompt '%s'", expectedPrompt) } - if err := scanner.Err(); err != nil { - return errors.Wrap(err, "reading stdout") +} + +// MustWaitForPrompt waits for an expected prompt with a default timeout. +// Fails the test if the prompt is not found or an error occurs. +func MustWaitForPrompt(t *testing.T, stdout io.Reader, expectedPrompt string) { + if err := waitForPrompt(stdout, expectedPrompt, promptTimeout); err != nil { + t.Fatal(err) } - if !found { - return errors.New("expected prompt not found in stdout") +} + +// userRespondToPrompt is a helper that waits for a prompt and sends a response. +func userRespondToPrompt(stdout io.Reader, stdin io.WriteCloser, expectedPrompt, response, action string) error { + if err := waitForPrompt(stdout, expectedPrompt, promptTimeout); err != nil { + return err } - // confirm - if _, err := io.WriteString(stdin, "y\n"); err != nil { - return errors.Wrap(err, "indicating confirmation in stdin") + if _, err := io.WriteString(stdin, response); err != nil { + return errors.Wrapf(err, "indicating %s in stdin", action) } return nil } -// UserConfirm simulates confirmation from the user by writing to stdin -func UserConfirm(stdin io.WriteCloser) error { - // confirm - if _, err := io.WriteString(stdin, "y\n"); err != nil { - return errors.Wrap(err, "indicating confirmation in stdin") - } - - return nil +// userConfirmOutput simulates confirmation from the user by writing to stdin. +// It waits for the expected prompt with a timeout to prevent deadlocks. +func userConfirmOutput(stdout io.Reader, stdin io.WriteCloser, expectedPrompt string) error { + return userRespondToPrompt(stdout, stdin, expectedPrompt, "y\n", "confirmation") } -// UserCancel simulates cancellation from the user by writing to stdin -func UserCancel(stdin io.WriteCloser) error { - // cancel - if _, err := io.WriteString(stdin, "n\n"); err != nil { - return errors.Wrap(err, "indicating cancellation in stdin") - } - - return nil +// userCancelOutput simulates cancellation from the user by writing to stdin. +// It waits for the expected prompt with a timeout to prevent deadlocks. +func userCancelOutput(stdout io.Reader, stdin io.WriteCloser, expectedPrompt string) error { + return userRespondToPrompt(stdout, stdin, expectedPrompt, "n\n", "cancellation") } -// UserContent simulates content from the user by writing to stdin -func UserContent(stdin io.WriteCloser) error { +// ConfirmRemoveNote waits for prompt for removing a note and confirms. +func ConfirmRemoveNote(stdout io.Reader, stdin io.WriteCloser) error { + return userConfirmOutput(stdout, stdin, PromptRemoveNote) +} + +// ConfirmRemoveBook waits for prompt for deleting a book confirms. +func ConfirmRemoveBook(stdout io.Reader, stdin io.WriteCloser) error { + return userConfirmOutput(stdout, stdin, PromptDeleteBook) +} + +// UserConfirmEmptyServerSync waits for an empty server prompt and confirms. +func UserConfirmEmptyServerSync(stdout io.Reader, stdin io.WriteCloser) error { + return userConfirmOutput(stdout, stdin, PromptEmptyServer) +} + +// UserCancelEmptyServerSync waits for an empty server prompt and confirms. +func UserCancelEmptyServerSync(stdout io.Reader, stdin io.WriteCloser) error { + return userCancelOutput(stdout, stdin, PromptEmptyServer) +} + +// UserContent simulates content from the user by writing to stdin. +// This is used for piped input where no prompt is shown. +func UserContent(stdout io.Reader, stdin io.WriteCloser) error { longText := `Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.` diff --git a/pkg/e2e/sync_test.go b/pkg/e2e/sync_test.go index cbd369cd..b4fec91f 100644 --- a/pkg/e2e/sync_test.go +++ b/pkg/e2e/sync_test.go @@ -19,7 +19,6 @@ package main import ( - "bufio" "bytes" "encoding/json" "fmt" @@ -428,7 +427,7 @@ func TestSync_oneway(t *testing.T) { cliDatabase.MustScan(t, "getting id of note to delete", cliDB.QueryRow("SELECT rowid FROM notes WHERE body = ?", "css2"), &nid2) clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "edit", "js", nid, "-c", "js3-edited") - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "css", nid2) + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveNote, cliBinaryName, "remove", "css", nid2) clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "add", "css", "-c", "css3") clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "add", "css", "-c", "css4") @@ -793,9 +792,9 @@ func TestSync_twoway(t *testing.T) { var nid string cliDatabase.MustScan(t, "getting id of note to remove", cliDB.QueryRow("SELECT rowid FROM notes WHERE body = ?", "js3"), &nid) - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "algorithms") + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveBook, cliBinaryName, "remove", "algorithms") clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "add", "css", "-c", "css4") - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js", nid) + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveNote, cliBinaryName, "remove", "js", nid) return map[string]string{ "jsBookUUID": jsBookUUID, @@ -1005,7 +1004,7 @@ func TestSync_twoway(t *testing.T) { // 2. on cli clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "add", "js", "-c", "js2") - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js") + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveBook, cliBinaryName, "remove", "js") clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "add", "math", "-c", "math1") var nid string @@ -1353,7 +1352,7 @@ func TestSync(t *testing.T) { // 2. on cli clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "sync") - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js") + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveBook, cliBinaryName, "remove", "js") return map[string]string{ "jsBookUUID": jsBookUUID, @@ -1407,7 +1406,7 @@ func TestSync(t *testing.T) { clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "sync") var nid string cliDatabase.MustScan(t, "getting id of note to remove", cliDB.QueryRow("SELECT rowid FROM notes WHERE uuid = ?", jsNote1UUID), &nid) - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js", nid) + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveNote, cliBinaryName, "remove", "js", nid) return map[string]string{ "jsBookUUID": jsBookUUID, @@ -2025,7 +2024,7 @@ func TestSync(t *testing.T) { apiDeleteBook(t, user, jsBookUUID, "deleting js book") // 4. on cli - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js") + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveBook, cliBinaryName, "remove", "js") return map[string]string{ "jsBookUUID": jsBookUUID, @@ -2085,7 +2084,7 @@ func TestSync(t *testing.T) { // 4. on cli var nid string cliDatabase.MustScan(t, "getting id of note to remove", cliDB.QueryRow("SELECT rowid FROM notes WHERE body = ?", "js1"), &nid) - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js", nid) + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveNote, cliBinaryName, "remove", "js", nid) return map[string]string{ "jsBookUUID": jsBookUUID, @@ -2630,7 +2629,7 @@ func TestSync(t *testing.T) { clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "sync") var nid string cliDatabase.MustScan(t, "getting id of note to remove", cliDB.QueryRow("SELECT rowid FROM notes WHERE uuid = ?", jsNote1UUID), &nid) - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js", nid) + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveNote, cliBinaryName, "remove", "js", nid) // 3. on server apiPatchNote(t, user, jsNote1UUID, fmt.Sprintf(`{"content": "%s"}`, "js1-edited"), "editing js note 1") @@ -2704,7 +2703,7 @@ func TestSync(t *testing.T) { clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "sync") var nid string cliDatabase.MustScan(t, "getting id of note to remove", cliDB.QueryRow("SELECT rowid FROM notes WHERE uuid = ?", jsNote1UUID), &nid) - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js", nid) + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveNote, cliBinaryName, "remove", "js", nid) // 3. on server apiPatchNote(t, user, jsNote1UUID, fmt.Sprintf(`{"book_uuid": "%s"}`, cssBookUUID), "moving js note 1 to css book") @@ -3005,7 +3004,7 @@ func TestSync(t *testing.T) { // 2. on cli clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "sync") - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js") + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveBook, cliBinaryName, "remove", "js") // 3. on server apiPatchBook(t, user, jsBookUUID, fmt.Sprintf(`{"name": "%s"}`, "js-edited"), "editing js book") @@ -3076,7 +3075,7 @@ func TestSync(t *testing.T) { apiPatchNote(t, user, jsNote1UUID, fmt.Sprintf(`{"content": "%s"}`, "js1-edited"), "editing js1 note") // 4. on cli - clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirm, cliBinaryName, "remove", "js") + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.ConfirmRemoveBook, cliBinaryName, "remove", "js") return map[string]string{ "jsBookUUID": jsBookUUID, @@ -3880,11 +3879,6 @@ func TestFullSync(t *testing.T) { } func TestSync_EmptyServer(t *testing.T) { - emptyServerPrompt := "The server is empty but you have local data" - emptyServerCallback := func(stdin io.WriteCloser, stdout io.Reader) error { - return clitest.UserConfirmOutput(stdin, stdout, emptyServerPrompt) - } - t.Run("sync to empty server after syncing to non-empty server", func(t *testing.T) { // Test server data loss/wipe scenario (disaster recovery): // Verify empty server detection works when the server loses all its data @@ -3923,7 +3917,7 @@ func TestSync_EmptyServer(t *testing.T) { // Step 3: Sync again - should detect empty server and prompt user // User confirms with "y" - clitest.MustWaitDnoteCmdOutput(t, dnoteCmdOpts, emptyServerCallback, cliBinaryName, "sync") + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirmEmptyServerSync, cliBinaryName, "sync") // Step 4: Verify data was uploaded to the empty server checkState(t, ctx, user, systemState{ @@ -3996,13 +3990,13 @@ func TestSync_EmptyServer(t *testing.T) { user = setupUser(t, &ctx) // Step 3: Sync again but user cancels with "n" - output, err := clitest.WaitDnoteCmd(t, dnoteCmdOpts, clitest.UserCancel, cliBinaryName, "sync") + output, err := clitest.WaitDnoteCmd(t, dnoteCmdOpts, clitest.UserCancelEmptyServerSync, cliBinaryName, "sync") if err == nil { t.Fatal("Expected sync to fail when user cancels, but it succeeded") } // Verify the prompt appeared - if !strings.Contains(output, "The server is empty but you have local data") { + if !strings.Contains(output, clitest.PromptEmptyServer) { t.Fatalf("Expected empty server warning in output, got: %s", output) } @@ -4094,15 +4088,22 @@ func TestSync_EmptyServer(t *testing.T) { }) t.Run("race condition - other client uploads first", func(t *testing.T) { - // Test race condition: Client A detects empty server and prompts user, - // but while waiting for confirmation, Client B uploads the same data via API. + // This test exercises a race condition that can occur during sync: + // While Client A is waiting for user input, Client B uploads data to the server. // - // Expected behavior: Client A's sync should handle the 409 conflict gracefully by: - // 1. Detecting the 409 error when trying to CREATE books that already exist - // 2. Running stepSync to pull the server's books (js, css) - // 3. mergeBook renames local conflicts (js→js_2, css→css_2) - // 4. Retrying sendChanges to upload the renamed books - // Result: Both clients' data is preserved (4 books total) + // The empty server scenario is the natural place to test this because + // an empty server detection triggers a prompt, at which point the test + // can make client B upload data. We trigger the race condition deterministically. + // + // Test flow: + // - Client A detects empty server and prompts user + // - While waiting for confirmation, Client B uploads the same data via API + // - Client A continues and handles the 409 conflict gracefully by: + // 1. Detecting the 409 error when trying to CREATE books that already exist + // 2. Running stepSync to pull the server's books (js, css) + // 3. mergeBook renames local conflicts (js→js_2, css→css_2) + // 4. Retrying sendChanges to upload the renamed books + // - Result: Both clients' data is preserved (4 books total) // Clean up apitest.ClearData(serverDb) @@ -4135,29 +4136,15 @@ func TestSync_EmptyServer(t *testing.T) { user = setupUser(t, &ctx) // Step 3: Trigger sync which will detect empty server and prompt user - // Inside the callback (before confirming), we simulate Client B uploading via API + // Inside the callback (before confirming), we simulate Client B uploading via API. // We wait for the empty server prompt to ensure Client B uploads AFTER // GetSyncState but BEFORE the sync decision, creating the race condition deterministically - raceCallback := func(stdin io.WriteCloser, stdout io.Reader) error { - // First, wait for the prompt to ensure Client A has called GetSyncState - // Block until stdout contains the string "The server is empty but you have local data" - scanner := bufio.NewScanner(stdout) - found := false - for scanner.Scan() { - line := scanner.Text() - if strings.Contains(line, emptyServerPrompt) { - found = true - break - } - } - if err := scanner.Err(); err != nil { - return errors.Wrap(err, "reading stdout") - } - if !found { - return errors.New("expected prompt not found in stdout") - } + raceCallback := func(stdout io.Reader, stdin io.WriteCloser) error { + // First, wait for the prompt to ensure Client A has obtained the sync state from the server. + clitest.MustWaitForPrompt(t, stdout, clitest.PromptEmptyServer) - // Now Client B uploads the same data via API (after GetSyncState, before sync decision) + // Now Client B uploads the same data via API (after Client A got the sync state from the server + // but before its sync decision) // This creates the race condition: Client A thinks server is empty, but Client B uploads data jsBookUUID := apiCreateBook(t, user, "js", "client B creating js book") cssBookUUID := apiCreateBook(t, user, "css", "client B creating css book") @@ -4178,8 +4165,8 @@ func TestSync_EmptyServer(t *testing.T) { // - 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") + // - Both clients' data is preserved. + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, raceCallback, cliBinaryName, "sync") // Verify final state - both clients' data preserved checkStateWithDB(t, ctx, user, serverDb, systemState{ @@ -4291,7 +4278,7 @@ func TestSync_EmptyServer(t *testing.T) { cliDatabase.MustExec(t, "updating session_key_expiry for B", ctx.DB, "UPDATE system SET value = ? WHERE key = ?", sessionB.ExpiresAt.Unix(), consts.SystemSessionKeyExpiry) // Should detect empty server and prompt - clitest.MustWaitDnoteCmdOutput(t, dnoteCmdOpts, emptyServerCallback, cliBinaryName, "--api-endpoint", apiEndpointB, "sync") + clitest.MustWaitDnoteCmd(t, dnoteCmdOpts, clitest.UserConfirmEmptyServerSync, cliBinaryName, "--api-endpoint", apiEndpointB, "sync") // Verify Server B now has data checkStateWithDB(t, ctx, userB, serverDbB, systemState{ @@ -4353,10 +4340,6 @@ func TestSync_FreshClientConcurrent(t *testing.T) { // // Expected: Client A should pull server data first, detect duplicate book names, // rename local books to avoid conflicts (js→js_2), then upload successfully. - // - // Bug: When lastMaxUSN=0 and syncState.MaxUSN=0 (if GetSyncState is called before - // Client B uploads), the sync logic skips stepSync and goes straight to sendChanges, - // causing 409 "duplicate book exists" errors. // Clean up apitest.ClearData(serverDb) @@ -4382,20 +4365,77 @@ func TestSync_FreshClientConcurrent(t *testing.T) { // Expected: pulls server data, renames local books to js_2/css_2, uploads successfully clitest.RunDnoteCmd(t, dnoteCmdOpts, cliBinaryName, "sync") - // Verify: Should have 4 books on server (js, css from B, js_2, css_2 from A) - var serverBookCount int64 - apitest.MustExec(t, serverDb.Model(&database.Book{}).Count(&serverBookCount), "counting server books") - assert.Equal(t, int(serverBookCount), 4, "server should have 4 books") + // Verify: Should have 4 books and 4 notes on both client and server + // USN breakdown: 2 books + 2 notes from Client B (USN 1-4), then 2 books + 2 notes from Client A (USN 5-8) + checkStateWithDB(t, ctx, user, serverDb, systemState{ + clientNoteCount: 4, + clientBookCount: 4, + clientLastMaxUSN: 8, + clientLastSyncAt: serverTime.Unix(), + serverNoteCount: 4, + serverBookCount: 4, + serverUserMaxUSN: 8, + }) - // Verify books exist with correct names + // Verify server has all 4 books with correct names 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'") - // Verify all 4 notes exist - var serverNoteCount int64 - apitest.MustExec(t, serverDb.Model(&database.Note{}).Count(&serverNoteCount), "counting server notes") - assert.Equal(t, int(serverNoteCount), 4, "server should have 4 notes") + 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 has all 4 notes with correct content + var svrNoteJS1, svrNoteJS2, svrNoteCSS1, svrNoteCSS2 database.Note + apitest.MustExec(t, serverDb.Where("body = ?", "js1").First(&svrNoteJS1), "finding server note 'js1'") + apitest.MustExec(t, serverDb.Where("body = ?", "js2").First(&svrNoteJS2), "finding server note 'js2'") + apitest.MustExec(t, serverDb.Where("body = ?", "css1").First(&svrNoteCSS1), "finding server note 'css1'") + apitest.MustExec(t, serverDb.Where("body = ?", "css2").First(&svrNoteCSS2), "finding server note 'css2'") + + assert.Equal(t, svrNoteJS1.BookUUID, svrBookJS2.UUID, "note 'js1' should belong to book 'js_2' (Client A)") + assert.Equal(t, svrNoteJS2.BookUUID, svrBookJS.UUID, "note 'js2' should belong to book 'js' (Client B)") + assert.Equal(t, svrNoteCSS1.BookUUID, svrBookCSS2.UUID, "note 'css1' should belong to book 'css_2' (Client A)") + assert.Equal(t, svrNoteCSS2.BookUUID, svrBookCSS.UUID, "note 'css2' should belong to book 'css' (Client B)") + + // Verify client has all 4 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) + + // 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 books 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") + + // Verify client has all 4 notes + var cliNoteJS1, cliNoteJS2, cliNoteCSS1, cliNoteCSS2 cliDatabase.Note + cliDatabase.MustScan(t, "finding client note 'js1'", ctx.DB.QueryRow("SELECT uuid, body, usn FROM notes WHERE body = ?", "js1"), &cliNoteJS1.UUID, &cliNoteJS1.Body, &cliNoteJS1.USN) + cliDatabase.MustScan(t, "finding client note 'js2'", ctx.DB.QueryRow("SELECT uuid, body, usn FROM notes WHERE body = ?", "js2"), &cliNoteJS2.UUID, &cliNoteJS2.Body, &cliNoteJS2.USN) + cliDatabase.MustScan(t, "finding client note 'css1'", ctx.DB.QueryRow("SELECT uuid, body, usn FROM notes WHERE body = ?", "css1"), &cliNoteCSS1.UUID, &cliNoteCSS1.Body, &cliNoteCSS1.USN) + cliDatabase.MustScan(t, "finding client note 'css2'", ctx.DB.QueryRow("SELECT uuid, body, usn FROM notes WHERE body = ?", "css2"), &cliNoteCSS2.UUID, &cliNoteCSS2.Body, &cliNoteCSS2.USN) + + // Verify client note UUIDs match server + assert.Equal(t, cliNoteJS1.UUID, svrNoteJS1.UUID, "client note 'js1' UUID should match server") + assert.Equal(t, cliNoteJS2.UUID, svrNoteJS2.UUID, "client note 'js2' UUID should match server") + assert.Equal(t, cliNoteCSS1.UUID, svrNoteCSS1.UUID, "client note 'css1' UUID should match server") + assert.Equal(t, cliNoteCSS2.UUID, svrNoteCSS2.UUID, "client note 'css2' UUID should match server") + + // Verify all notes have non-zero USN (synced successfully) + assert.NotEqual(t, cliNoteJS1.USN, 0, "client note 'js1' should have non-zero USN") + assert.NotEqual(t, cliNoteJS2.USN, 0, "client note 'js2' should have non-zero USN") + assert.NotEqual(t, cliNoteCSS1.USN, 0, "client note 'css1' should have non-zero USN") + assert.NotEqual(t, cliNoteCSS2.USN, 0, "client note 'css2' should have non-zero USN") }