fix(sync): let td-sync admin create-user run while the server writes

cmd/td-sync/admin.go

@@ -44,7 +44,7 @@ func printAdminUsage() {
 	fmt.Fprintln(os.Stderr, `Usage: td-sync admin <command> [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")

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

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
 

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,33 +38,43 @@ 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,
 	)
 	if err != nil {
 		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
 }