From 21a5a66d44b352c9d0a59e20fc3bfcb5d9e63ea2 Mon Sep 17 00:00:00 2001 From: Justin Bishop Date: Thu, 15 Oct 2026 13:54:06 -0700 Subject: [PATCH] fix(sync): let td-sync admin create-user run while the server writes CreateUser counted users and then inserted in one deferred transaction, so a write from a running server in between made it fail at once with "database is locked". It now takes the write lock up front with BEGIN IMMEDIATE, as ApproveDeviceChallengeAsOperator does, and the docs no longer say to stop the server first. Co-Authored-By: Claude Opus 5.5 --- cmd/td-sync/admin.go | 6 +- cmd/td-sync/admin_test.go | 146 +++++++++++++++++++++++----------- docs/sync-server-ops-guide.md | 6 +- internal/serverdb/users.go | 32 ++++++-- 4 files changed, 132 insertions(+), 58 deletions(-) diff --git a/cmd/td-sync/admin.go b/cmd/td-sync/admin.go index 93bed090..466a4389 100644 --- a/cmd/td-sync/admin.go +++ b/cmd/td-sync/admin.go @@ -44,7 +44,7 @@ func printAdminUsage() { fmt.Fprintln(os.Stderr, `Usage: td-sync admin [flags] Commands: - create-user Create a user (idempotent; stop the server first) + create-user Create a user (idempotent) approve-login Approve a pending td auth login without email grant Grant admin privileges to a user revoke Revoke admin privileges from a user @@ -68,8 +68,8 @@ func openDB(dbPath string) *serverdb.ServerDB { // runAdminCreateUser provisions a user by email, for self-hosted servers and // dev/test setup (e.g. the e2e harness), where the non-enumerating device-login // flow requires the user to already exist. It is idempotent: if the user already -// exists it prints the existing ID and exits 0. Run it while the server is NOT -// holding the DB open (the e2e harness provisions users before server start). +// exists it prints the existing ID and exits 0. Safe to run while the server +// is up. func runAdminCreateUser(args []string) { fs := flag.NewFlagSet("admin create-user", flag.ExitOnError) email := fs.String("email", "", "user email address") diff --git a/cmd/td-sync/admin_test.go b/cmd/td-sync/admin_test.go index ee986224..fe025d5c 100644 --- a/cmd/td-sync/admin_test.go +++ b/cmd/td-sync/admin_test.go @@ -217,70 +217,126 @@ func TestAdminApproveLogin_SinglePendingNeedsConfirmation(t *testing.T) { } } -// TestAdminApproveLogin_WhileServerWrites runs approve-login as a separate -// process while this process holds the database open and keeps writing to it, -// as a running td-sync server would. A real server writes server.db once or -// twice per request; this writer commits hundreds of times a second but, like -// any real server, leaves the lock free between writes. A writer that never -// releases the lock can starve any second process past the busy timeout. -func TestAdminApproveLogin_WhileServerWrites(t *testing.T) { - store, dbPath := openServerStore(t) - - // create-user runs before the load starts: its transaction reads before it - // writes, so a concurrent writer can make it fail with SQLITE_BUSY. The - // docs tell operators to run it while the server is stopped. - stdout, stderr, code := runAdminProcess(t, "create-user", "--email", "live@example.com", "--db", dbPath) - if code != 0 || !strings.Contains(stdout, "created user live@example.com") { - t.Fatalf("create-user: code=%d stdout=%q stderr=%q", code, stdout, stderr) - } - u, err := store.GetUserByEmail("live@example.com") - if err != nil || u == nil { - t.Fatalf("server handle cannot see created user: %v", err) - } - selector := startDeviceLogin(t, store, u, 3) +// serverWriter keeps writing to server.db from this process, as a running +// td-sync server would, while a test runs admin commands as separate +// processes. A real server writes server.db once or twice per request; this +// writer commits hundreds of times a second but, like any real server, leaves +// the lock free between writes. A writer that never releases the lock can +// starve any second process past the busy timeout. +type serverWriter struct { + writes atomic.Int64 + writeErr atomic.Value + stopCh chan struct{} + once sync.Once + wg sync.WaitGroup +} - stop := make(chan struct{}) - var wg sync.WaitGroup - var writes atomic.Int64 - var writeErr atomic.Value - wg.Add(1) +func startServerWriter(t *testing.T, store *serverdb.ServerDB) *serverWriter { + t.Helper() + w := &serverWriter{stopCh: make(chan struct{})} + w.wg.Add(1) go func() { - defer wg.Done() + defer w.wg.Done() for { select { - case <-stop: + case <-w.stopCh: return default: } if err := store.InsertAuthEvent("load", "load@example.com", serverdb.AuthEventStarted, "{}"); err != nil { - writeErr.Store(err.Error()) + w.writeErr.Store(err.Error()) return } - writes.Add(1) + w.writes.Add(1) time.Sleep(time.Millisecond) } }() - var once sync.Once - stopWriter := func() { - once.Do(func() { close(stop) }) - wg.Wait() + t.Cleanup(w.stop) + return w +} + +func (w *serverWriter) stop() { + w.once.Do(func() { close(w.stopCh) }) + w.wg.Wait() +} + +// runWhileWriting runs an admin command as a separate process while w writes, +// and fails the test if the writer made no progress during it. +func (w *serverWriter) runWhileWriting(t *testing.T, args ...string) (stdout, stderr string, code int) { + t.Helper() + before, began := w.writes.Load(), time.Now() + stdout, stderr, code = runAdminProcess(t, args...) + t.Logf("server committed %d writes during the %v %s process", w.writes.Load()-before, time.Since(began).Round(time.Millisecond), args[0]) + if w.writes.Load() == before { + t.Fatalf("server writer made no progress while %s ran", args[0]) } - t.Cleanup(stopWriter) + return stdout, stderr, code +} - before, began := writes.Load(), time.Now() - stdout, stderr, code = runAdminProcess(t, "approve-login", "--email", "live@example.com", "--yes", "--db", dbPath) - t.Logf("server committed %d writes during the %v approve-login process", writes.Load()-before, time.Since(began).Round(time.Millisecond)) - if code != 0 || !strings.Contains(stdout, "selector: "+selector) { - t.Fatalf("approve-login while server writes: code=%d stdout=%q stderr=%q", code, stdout, stderr) +// stopAndCheck stops the writer and fails the test if any of its writes failed. +func (w *serverWriter) stopAndCheck(t *testing.T) { + t.Helper() + w.stop() + if v := w.writeErr.Load(); v != nil { + t.Fatalf("server write failed during the admin command: %v", v) + } +} + +// TestAdminCreateUser_WhileServerWrites runs create-user as separate processes +// while this process keeps writing to the database. A single run hits the +// window between its read and its write only some of the time, so it creates +// several users, then reruns one to check that it stays idempotent. +func TestAdminCreateUser_WhileServerWrites(t *testing.T) { + store, dbPath := openServerStore(t) + w := startServerWriter(t, store) + + const users = 8 + for i := range users { + email := fmt.Sprintf("live%d@example.com", i) + stdout, stderr, code := w.runWhileWriting(t, "create-user", "--email", email, "--db", dbPath) + if code != 0 || !strings.Contains(stdout, "created user "+email) { + t.Fatalf("create-user %s while server writes: code=%d stdout=%q stderr=%q", email, code, stdout, stderr) + } + } + stdout, stderr, code := w.runWhileWriting(t, "create-user", "--email", "live0@example.com", "--db", dbPath) + if code != 0 || !strings.Contains(stdout, "user already exists: live0@example.com") { + t.Fatalf("repeat create-user while server writes: code=%d stdout=%q stderr=%q", code, stdout, stderr) + } + + w.stopAndCheck(t) + for i := range users { + u, err := store.GetUserByEmail(fmt.Sprintf("live%d@example.com", i)) + if err != nil || u == nil { + t.Fatalf("server handle cannot see user %d: %v", i, err) + } + if u.IsAdmin != (i == 0) { + t.Fatalf("user %d: is_admin=%v, want only the first user to be admin", i, u.IsAdmin) + } + } +} + +// TestAdminApproveLogin_WhileServerWrites runs approve-login as a separate +// process while this process keeps writing to the database. +func TestAdminApproveLogin_WhileServerWrites(t *testing.T) { + store, dbPath := openServerStore(t) + + stdout, stderr, code := runAdminProcess(t, "create-user", "--email", "live@example.com", "--db", dbPath) + if code != 0 || !strings.Contains(stdout, "created user live@example.com") { + t.Fatalf("create-user: code=%d stdout=%q stderr=%q", code, stdout, stderr) } - if writes.Load() == before { - t.Fatal("server writer made no progress while approve-login ran") + u, err := store.GetUserByEmail("live@example.com") + if err != nil || u == nil { + t.Fatalf("server handle cannot see created user: %v", err) } + selector := startDeviceLogin(t, store, u, 3) - stopWriter() - if v := writeErr.Load(); v != nil { - t.Fatalf("server write failed while approve-login ran: %v", v) + w := startServerWriter(t, store) + stdout, stderr, code = w.runWhileWriting(t, "approve-login", "--email", "live@example.com", "--yes", "--db", dbPath) + if code != 0 || !strings.Contains(stdout, "selector: "+selector) { + t.Fatalf("approve-login while server writes: code=%d stdout=%q stderr=%q", code, stdout, stderr) } + + w.stopAndCheck(t) if got := challengeStatus(t, store, selector); got != serverdb.ChallengeStatusVerified { t.Fatalf("server handle sees status %s, want verified", got) } diff --git a/docs/sync-server-ops-guide.md b/docs/sync-server-ops-guide.md index 25759459..d99dd238 100644 --- a/docs/sync-server-ops-guide.md +++ b/docs/sync-server-ops-guide.md @@ -164,13 +164,13 @@ SYNC_EMAIL_PROVIDER=memory SYNC_DEV_EMAIL_INSPECT=1 ./td-sync A private server, such as one on a home network, can run without an email provider. Leave `SYNC_EMAIL_PROVIDER` unset (the `log` provider) and approve each login from the server's shell instead of an emailed link. Pass `--db` to the admin commands, or set `SYNC_SERVER_DB_PATH`, when the database is not at `./data/server.db`. -1. Create your account while the server is stopped. `td auth login` never creates accounts. +1. Create your account. `td auth login` never creates accounts. ```bash td-sync admin create-user --email you@example.com ``` -2. Start the server. On the client, run `td auth login` and enter the same email; it waits for approval. +2. Start the server if it is not running. On the client, run `td auth login` and enter the same email; it waits for approval. 3. On the server, review the waiting login and approve it: @@ -199,7 +199,7 @@ A private server, such as one on a home network, can run without an email provid `approve-login` needs write access to `server.db`, which `create-key` and `grant` already require, so it gives nothing to anyone without a shell on the server. The client must still present the PKCE verifier from the login it started. The approval is recorded as a `device_verified` auth event with `{"approved_by":"operator_cli"}` and does not mark the email address verified. -`approve-login` is safe to run while the server is up. If a very busy server holds the database lock past the 5-second busy timeout, the command exits with `database is locked` and changes nothing; run it again. `create-user` is less tolerant of concurrent writes, so run it while the server is stopped. +`create-user` and `approve-login` are safe to run while the server is up. If a very busy server holds the database lock past the 5-second busy timeout, the command exits with `database is locked` and changes nothing; run it again. ## Docker Deployment diff --git a/internal/serverdb/users.go b/internal/serverdb/users.go index 7dc45b81..468652e4 100644 --- a/internal/serverdb/users.go +++ b/internal/serverdb/users.go @@ -1,6 +1,7 @@ package serverdb import ( + "context" "database/sql" "fmt" "strings" @@ -19,6 +20,13 @@ type User struct { // CreateUser inserts a new user with the given email (lowercased). // The first user created is automatically made an admin. +// +// The count and the insert share one transaction so two concurrent creations +// cannot both become admin. It starts with BEGIN IMMEDIATE so the write lock +// is taken up front under the busy timeout, which makes `td-sync admin +// create-user` safe to run while a td-sync server holds the same file. A +// deferred transaction that reads first gets SQLITE_BUSY at once, without +// waiting, if another connection writes in between. func (db *ServerDB) CreateUser(email string) (*User, error) { email = strings.ToLower(strings.TrimSpace(email)) if email == "" { @@ -30,23 +38,32 @@ func (db *ServerDB) CreateUser(email string) (*User, error) { return nil, fmt.Errorf("generate user id: %w", err) } - // Use a transaction to atomically check count and insert - // to prevent TOCTOU race where two concurrent requests both become admin - tx, err := db.conn.Begin() + ctx := context.Background() + conn, err := db.conn.Conn(ctx) if err != nil { + return nil, fmt.Errorf("get connection: %w", err) + } + defer func() { _ = conn.Close() }() + + if _, err := conn.ExecContext(ctx, "BEGIN IMMEDIATE"); err != nil { return nil, fmt.Errorf("begin tx: %w", err) } - defer func() { _ = tx.Rollback() }() + committed := false + defer func() { + if !committed { + _, _ = conn.ExecContext(ctx, "ROLLBACK") + } + }() // First user becomes admin var count int - if err := tx.QueryRow("SELECT COUNT(*) FROM users").Scan(&count); err != nil { + if err := conn.QueryRowContext(ctx, "SELECT COUNT(*) FROM users").Scan(&count); err != nil { return nil, fmt.Errorf("count users: %w", err) } isAdmin := count == 0 now := time.Now().UTC() - _, err = tx.Exec( + _, err = conn.ExecContext(ctx, `INSERT INTO users (id, email, is_admin, created_at, updated_at) VALUES (?, ?, ?, ?, ?)`, id, email, isAdmin, now, now, ) @@ -54,9 +71,10 @@ func (db *ServerDB) CreateUser(email string) (*User, error) { return nil, fmt.Errorf("insert user: %w", err) } - if err := tx.Commit(); err != nil { + if _, err := conn.ExecContext(ctx, "COMMIT"); err != nil { return nil, fmt.Errorf("commit: %w", err) } + committed = true return &User{ID: id, Email: email, IsAdmin: isAdmin, CreatedAt: now, UpdatedAt: now}, nil }