From 2366956fba41cb4424bec8d76fc7807cf5f039e4 Mon Sep 17 00:00:00 2001 From: Rupak Majumdar Date: Thu, 30 Jul 2026 16:24:20 +0200 Subject: [PATCH 1/3] fix(carts): repair dolt server reuse, auth, and start race Co-Authored-By: SageOx --- internal/carts/server.go | 34 ++++++++++--- internal/carts/server_test.go | 91 +++++++++++++++++++++++++++++++++++ internal/carts/store.go | 13 ++--- 3 files changed, 125 insertions(+), 13 deletions(-) create mode 100644 internal/carts/server_test.go diff --git a/internal/carts/server.go b/internal/carts/server.go index bc6be240..fb7d1602 100644 --- a/internal/carts/server.go +++ b/internal/carts/server.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strconv" "strings" + "syscall" "time" _ "github.com/go-sql-driver/mysql" @@ -47,8 +48,12 @@ func runningServerPort(cartsDir string) (int, error) { if err != nil { return 0, err } - // Signal 0 checks if process exists - if err := proc.Signal(os.Signal(nil)); err != nil { + // Signal 0 checks if the process exists. It must be syscall.Signal(0), not a + // nil os.Signal: os.Process.Signal type-asserts its argument to syscall.Signal, + // which nil fails, so a nil signal ALWAYS errored. That made reuse dead code — + // every invocation started another sql-server and all but the first died on + // dolt's exclusive write lock. + if err := proc.Signal(syscall.Signal(0)); err != nil { return 0, fmt.Errorf("server process %d not running: %w", pid, err) } @@ -60,6 +65,13 @@ func runningServerPort(cartsDir string) (int, error) { if err != nil { return 0, err } + + // A live PID is not proof the server is ours — the OS recycles PIDs, so a + // stale file can name an unrelated process. Confirm something is actually + // serving on the recorded port before handing it back. + if err := waitForServer("127.0.0.1", port, 2*time.Second); err != nil { + return 0, fmt.Errorf("server process %d is not serving port %d: %w", pid, port, err) + } return port, nil } @@ -99,6 +111,19 @@ func startServer(cartsDir string) (int, error) { } logFile.Close() + // Wait for readiness BEFORE recording state. Writing the files first let a + // starter that was about to lose dolt's write-lock race overwrite the + // metadata of the server that won it, permanently orphaning the healthy + // server: every later invocation read the loser's dead PID and started yet + // another doomed one. + if err := waitForServer("127.0.0.1", port, 30*time.Second); err != nil { + // Reap the process we spawned; otherwise a failed start leaves a + // process holding the lock that nothing has a PID file for. + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() + return 0, fmt.Errorf("server failed to start: %w", err) + } + // Write state files if err := os.WriteFile(filepath.Join(cartsDir, pidFileName), []byte(strconv.Itoa(cmd.Process.Pid)), 0o644); err != nil { return 0, err @@ -107,11 +132,6 @@ func startServer(cartsDir string) (int, error) { return 0, err } - // Wait for server to be ready - if err := waitForServer("127.0.0.1", port, 30*time.Second); err != nil { - return 0, fmt.Errorf("server failed to start: %w", err) - } - return port, nil } diff --git a/internal/carts/server_test.go b/internal/carts/server_test.go new file mode 100644 index 00000000..b15ee9b8 --- /dev/null +++ b/internal/carts/server_test.go @@ -0,0 +1,91 @@ +package carts + +import ( + "os" + "os/exec" + "path/filepath" + "strconv" + "syscall" + "testing" +) + +// The liveness probe in runningServerPort must use syscall.Signal(0). It was +// written as proc.Signal(os.Signal(nil)), and os.Process.Signal type-asserts its +// argument to syscall.Signal — nil fails that assertion, so the call ALWAYS +// returned an error. Server reuse became dead code: every ox carts invocation +// started another dolt sql-server, and all but the first died on dolt's +// exclusive write lock. This asserts the two behaviors reuse depends on. +func TestSignalZeroDetectsLiveness(t *testing.T) { + t.Run("live process is detected as alive", func(t *testing.T) { + proc, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatalf("FindProcess(self): %v", err) + } + if err := proc.Signal(syscall.Signal(0)); err != nil { + t.Errorf("signal 0 on the running test process should succeed, got %v", err) + } + }) + + t.Run("nil signal always errors", func(t *testing.T) { + // Guards against reintroducing the original bug: a nil signal reports + // even a definitely-live process as dead. + proc, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatalf("FindProcess(self): %v", err) + } + if err := proc.Signal(os.Signal(nil)); err == nil { + t.Error("expected nil signal to error; if this now works, the guard below is moot") + } + }) + + t.Run("reaped process is detected as dead", func(t *testing.T) { + cmd := exec.Command("true") + if err := cmd.Start(); err != nil { + t.Skipf("cannot spawn helper process: %v", err) + } + pid := cmd.Process.Pid + _ = cmd.Wait() // reap, so the PID no longer names a live process + + proc, err := os.FindProcess(pid) + if err != nil { + t.Fatalf("FindProcess(%d): %v", pid, err) + } + if err := proc.Signal(syscall.Signal(0)); err == nil { + t.Errorf("signal 0 on reaped pid %d should fail", pid) + } + }) +} + +// A stale PID file must not be trusted. Before the fix a losing starter could +// overwrite the state files of the server that won the lock race, leaving a +// dead PID recorded and the healthy server orphaned. +func TestRunningServerPortRejectsStalePID(t *testing.T) { + dir := t.TempDir() + + cmd := exec.Command("true") + if err := cmd.Start(); err != nil { + t.Skipf("cannot spawn helper process: %v", err) + } + deadPID := cmd.Process.Pid + _ = cmd.Wait() + + write(t, filepath.Join(dir, pidFileName), strconv.Itoa(deadPID)) + write(t, filepath.Join(dir, portFileName), "1") + + if _, err := runningServerPort(dir); err == nil { + t.Fatal("expected an error for a dead PID, got nil (caller would reuse a nonexistent server)") + } +} + +func TestRunningServerPortErrorsWithoutStateFiles(t *testing.T) { + if _, err := runningServerPort(t.TempDir()); err == nil { + t.Fatal("expected an error when no PID file exists") + } +} + +func write(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } +} diff --git a/internal/carts/store.go b/internal/carts/store.go index 3e32052c..98002e32 100644 --- a/internal/carts/store.go +++ b/internal/carts/store.go @@ -55,12 +55,13 @@ func New(ctx context.Context, cfg *Config) (*Store, error) { } } - user := cfg.CommitterName - if user == "" { - user = "root" - } - dsn := fmt.Sprintf("%s@tcp(%s:%d)/?parseTime=true&interpolateParams=true&timeout=5s&readTimeout=30s&writeTimeout=30s", - user, cfg.ServerHost, port) + // Always connect as root: `dolt sql-server` only ever provisions root, and + // EnsureServer's readiness probe authenticates as root too. CommitterName is + // a git author name ("Ada Lovelace"), not a SQL account — using it here made + // every command fail with "Access denied for user ''" unless the user + // happened to be called root. It stays reserved for the DOLT_COMMIT author. + dsn := fmt.Sprintf("root@tcp(%s:%d)/?parseTime=true&interpolateParams=true&timeout=5s&readTimeout=30s&writeTimeout=30s", + cfg.ServerHost, port) db, err := sql.Open("mysql", dsn) if err != nil { From 909a73800fa589ba078e3b4ce6a9e5c0e5609793 Mon Sep 17 00:00:00 2001 From: Rupak Majumdar Date: Fri, 31 Jul 2026 10:55:36 +0200 Subject: [PATCH 2/3] fix(carts): join winning server on race, portable liveness, reap on write fail Co-Authored-By: SageOx --- internal/carts/proc_unix.go | 26 ++++ internal/carts/proc_windows.go | 36 ++++++ internal/carts/server.go | 106 +++++++++++----- internal/carts/server_test.go | 224 ++++++++++++++++++++++++--------- 4 files changed, 306 insertions(+), 86 deletions(-) create mode 100644 internal/carts/proc_unix.go create mode 100644 internal/carts/proc_windows.go diff --git a/internal/carts/proc_unix.go b/internal/carts/proc_unix.go new file mode 100644 index 00000000..1d20c0b8 --- /dev/null +++ b/internal/carts/proc_unix.go @@ -0,0 +1,26 @@ +//go:build !windows + +package carts + +import ( + "os" + "syscall" +) + +// processAlive reports whether pid names a live process. +// +// Signal 0 is the POSIX process-existence convention: it runs the kernel's +// permission and existence checks without delivering anything. It must be +// syscall.Signal(0) and not a nil os.Signal — os.Process.Signal type-asserts its +// argument to syscall.Signal, which nil fails, so a nil signal always reports +// even a live process as dead. +// +// A true result is necessary but not sufficient for reuse: PIDs are recycled, so +// callers must still confirm the recorded port is actually serving. +func processAlive(pid int) bool { + proc, err := os.FindProcess(pid) + if err != nil { + return false + } + return proc.Signal(syscall.Signal(0)) == nil +} diff --git a/internal/carts/proc_windows.go b/internal/carts/proc_windows.go new file mode 100644 index 00000000..45e8b492 --- /dev/null +++ b/internal/carts/proc_windows.go @@ -0,0 +1,36 @@ +//go:build windows + +package carts + +import ( + "golang.org/x/sys/windows" +) + +// stillActive is the exit code GetExitCodeProcess reports for a process that has +// not exited (STILL_ACTIVE in the Win32 headers, 259). x/sys/windows exposes the +// same value as STATUS_PENDING but not under the STILL_ACTIVE name, so spell it +// out rather than borrowing a constant that means something else. +const stillActive = 259 + +// processAlive reports whether pid names a live process. +// +// Signal 0 is meaningless here: Go's Windows os.Process.Signal supports only +// os.Kill and returns syscall.EWINDOWS for anything else, so the POSIX probe +// would report every live process as dead and defeat server reuse entirely. +// Instead open the process and ask for its exit code — STILL_ACTIVE means running. +// +// A true result is necessary but not sufficient for reuse: PIDs are recycled, so +// callers must still confirm the recorded port is actually serving. +func processAlive(pid int) bool { + h, err := windows.OpenProcess(windows.PROCESS_QUERY_LIMITED_INFORMATION, false, uint32(pid)) + if err != nil { + return false + } + defer windows.CloseHandle(h) + + var code uint32 + if err := windows.GetExitCodeProcess(h, &code); err != nil { + return false + } + return code == stillActive +} diff --git a/internal/carts/server.go b/internal/carts/server.go index fb7d1602..151c1c97 100644 --- a/internal/carts/server.go +++ b/internal/carts/server.go @@ -9,7 +9,6 @@ import ( "path/filepath" "strconv" "strings" - "syscall" "time" _ "github.com/go-sql-driver/mysql" @@ -43,18 +42,12 @@ func runningServerPort(cartsDir string) (int, error) { if err != nil { return 0, err } - // Check if process is alive - proc, err := os.FindProcess(pid) - if err != nil { - return 0, err - } - // Signal 0 checks if the process exists. It must be syscall.Signal(0), not a - // nil os.Signal: os.Process.Signal type-asserts its argument to syscall.Signal, - // which nil fails, so a nil signal ALWAYS errored. That made reuse dead code — - // every invocation started another sql-server and all but the first died on - // dolt's exclusive write lock. - if err := proc.Signal(syscall.Signal(0)); err != nil { - return 0, fmt.Errorf("server process %d not running: %w", pid, err) + // Liveness is platform-specific (see proc_unix.go / proc_windows.go). It was + // written here as proc.Signal(os.Signal(nil)), which always errors, so reuse + // was dead code — every invocation started another sql-server and all but the + // first died on dolt's exclusive write lock. + if !processAlive(pid) { + return 0, fmt.Errorf("server process %d not running", pid) } portData, err := os.ReadFile(filepath.Join(cartsDir, portFileName)) @@ -111,30 +104,74 @@ func startServer(cartsDir string) (int, error) { } logFile.Close() + // reap tears down the process we spawned and any half-written state. It stays + // armed from here until BOTH state files are on disk: a ready server with + // incomplete metadata is worse than no server, because nothing can find it to + // reuse and every later call starts another contender for dolt's lock. + reap := func() { + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() + _ = os.Remove(filepath.Join(cartsDir, pidFileName)) + _ = os.Remove(filepath.Join(cartsDir, portFileName)) + } + // Wait for readiness BEFORE recording state. Writing the files first let a // starter that was about to lose dolt's write-lock race overwrite the // metadata of the server that won it, permanently orphaning the healthy // server: every later invocation read the loser's dead PID and started yet // another doomed one. - if err := waitForServer("127.0.0.1", port, 30*time.Second); err != nil { - // Reap the process we spawned; otherwise a failed start leaves a - // process holding the lock that nothing has a PID file for. - _ = cmd.Process.Kill() - _, _ = cmd.Process.Wait() + // + // Losing that race is expected, not exceptional: dolt permits exactly one + // writer, so when two processes start concurrently one of them cannot come + // up. waitForOwnServerOrWinner therefore also watches for a peer recording a + // healthy server, and hands back the winner's port instead of failing. + winner, err := waitForOwnServerOrWinner(cartsDir, port, 30*time.Second) + if err != nil { + reap() return 0, fmt.Errorf("server failed to start: %w", err) } + if winner != 0 { + // A peer won the lock and published a ready server; ours never came up. + reap() + return winner, nil + } - // Write state files if err := os.WriteFile(filepath.Join(cartsDir, pidFileName), []byte(strconv.Itoa(cmd.Process.Pid)), 0o644); err != nil { - return 0, err + reap() + return 0, fmt.Errorf("write %s: %w", pidFileName, err) } if err := os.WriteFile(filepath.Join(cartsDir, portFileName), []byte(strconv.Itoa(port)), 0o644); err != nil { - return 0, err + reap() + return 0, fmt.Errorf("write %s: %w", portFileName, err) } return port, nil } +// waitForOwnServerOrWinner polls until either the server we started on ourPort +// accepts connections (returns 0, nil) or a concurrently-started peer publishes +// a ready server (returns that peer's port, nil). It returns an error only if +// neither happens before the timeout. +// +// Polling for the peer matters for latency as much as correctness: without it a +// process that lost dolt's write-lock race blocks for the full timeout before +// failing, even though a healthy server it could have used appeared seconds in. +func waitForOwnServerOrWinner(cartsDir string, ourPort int, timeout time.Duration) (int, error) { + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + if err := probeServer("127.0.0.1", ourPort); err == nil { + return 0, nil + } + // Ignore the error: a missing or stale peer record simply means no winner + // yet, which is the common case on the first iteration. + if peer, err := runningServerPort(cartsDir); err == nil && peer != ourPort { + return peer, nil + } + time.Sleep(200 * time.Millisecond) + } + return 0, fmt.Errorf("timeout waiting for dolt server on port %d", ourPort) +} + // ensureDoltInit ensures the dolt directory is initialized. func ensureDoltInit(doltDir string) error { if err := os.MkdirAll(doltDir, 0o750); err != nil { @@ -166,18 +203,29 @@ func allocateEphemeralPort(host string) (int, error) { return port, nil } +// probeServer is the liveness probe used by the reuse and readiness paths. +// Indirected through a variable so tests can exercise the concurrency logic +// without standing up a real dolt sql-server to satisfy a MySQL handshake. +var probeServer = pingServer + +// pingServer reports whether a dolt sql-server accepts an authenticated +// connection on host:port right now. Connects as root — dolt provisions no other +// account, and the git author name is not a SQL user. +func pingServer(host string, port int) error { + db, err := sql.Open("mysql", fmt.Sprintf("root@tcp(%s:%d)/", host, port)) + if err != nil { + return err + } + defer db.Close() + return db.Ping() +} + // waitForServer polls until the server accepts connections. func waitForServer(host string, port int, timeout time.Duration) error { deadline := time.Now().Add(timeout) - dsn := fmt.Sprintf("root@tcp(%s:%d)/", host, port) for time.Now().Before(deadline) { - db, err := sql.Open("mysql", dsn) - if err == nil { - if err := db.Ping(); err == nil { - db.Close() - return nil - } - db.Close() + if err := probeServer(host, port); err == nil { + return nil } time.Sleep(200 * time.Millisecond) } diff --git a/internal/carts/server_test.go b/internal/carts/server_test.go index b15ee9b8..9129ad9e 100644 --- a/internal/carts/server_test.go +++ b/internal/carts/server_test.go @@ -1,89 +1,199 @@ package carts import ( + "errors" "os" "os/exec" "path/filepath" "strconv" - "syscall" "testing" + "time" ) -// The liveness probe in runningServerPort must use syscall.Signal(0). It was -// written as proc.Signal(os.Signal(nil)), and os.Process.Signal type-asserts its -// argument to syscall.Signal — nil fails that assertion, so the call ALWAYS -// returned an error. Server reuse became dead code: every ox carts invocation -// started another dolt sql-server, and all but the first died on dolt's -// exclusive write lock. This asserts the two behaviors reuse depends on. -func TestSignalZeroDetectsLiveness(t *testing.T) { - t.Run("live process is detected as alive", func(t *testing.T) { - proc, err := os.FindProcess(os.Getpid()) - if err != nil { - t.Fatalf("FindProcess(self): %v", err) +// stubProbe replaces the MySQL liveness probe for the duration of a test so the +// concurrency logic can be exercised without standing up a real dolt sql-server. +// healthy lists the ports that should answer; everything else is refused. +func stubProbe(t *testing.T, healthy ...int) { + t.Helper() + original := probeServer + t.Cleanup(func() { probeServer = original }) + probeServer = func(_ string, port int) error { + for _, h := range healthy { + if h == port { + return nil + } } - if err := proc.Signal(syscall.Signal(0)); err != nil { - t.Errorf("signal 0 on the running test process should succeed, got %v", err) + return errors.New("connection refused") + } +} + +// deadPID returns the PID of a process that has exited and been reaped, so the +// number is valid but names nothing running. +// +// It re-executes the test binary with a filter that matches no test rather than +// shelling out to `true`: a missing external binary would otherwise turn the +// liveness assertions into a skip, letting them pass vacuously on exactly the +// hosts where they might regress. +func deadPID(t *testing.T) int { + t.Helper() + cmd := exec.Command(os.Args[0], "-test.run=^$") + if err := cmd.Start(); err != nil { + t.Fatalf("start helper process: %v", err) + } + pid := cmd.Process.Pid + _ = cmd.Wait() // reap, so the PID no longer names a live process + return pid +} + +// freePort returns a port number with nothing listening on it. +func freePort(t *testing.T) int { + t.Helper() + port, err := allocateEphemeralPort("127.0.0.1") + if err != nil { + t.Fatalf("allocate port: %v", err) + } + return port +} + +// processAlive must report liveness truthfully. It was written as +// proc.Signal(os.Signal(nil)), and os.Process.Signal type-asserts its argument to +// syscall.Signal — nil fails that assertion, so the call ALWAYS returned an +// error. Server reuse became dead code: every ox carts invocation started another +// dolt sql-server, and all but the first died on dolt's exclusive write lock. +func TestProcessAlive(t *testing.T) { + t.Run("running process is alive", func(t *testing.T) { + if !processAlive(os.Getpid()) { + t.Error("the running test process should report as alive") } }) - t.Run("nil signal always errors", func(t *testing.T) { - // Guards against reintroducing the original bug: a nil signal reports - // even a definitely-live process as dead. - proc, err := os.FindProcess(os.Getpid()) - if err != nil { - t.Fatalf("FindProcess(self): %v", err) - } - if err := proc.Signal(os.Signal(nil)); err == nil { - t.Error("expected nil signal to error; if this now works, the guard below is moot") + t.Run("reaped process is not alive", func(t *testing.T) { + pid := deadPID(t) + if processAlive(pid) { + t.Errorf("reaped pid %d should report as dead", pid) } }) +} - t.Run("reaped process is detected as dead", func(t *testing.T) { - cmd := exec.Command("true") - if err := cmd.Start(); err != nil { - t.Skipf("cannot spawn helper process: %v", err) - } - pid := cmd.Process.Pid - _ = cmd.Wait() // reap, so the PID no longer names a live process +// runningServerPort gates server reuse. Every rejection path matters: handing +// back a port for a server that is not really there sends the caller into a +// confusing connection error instead of a clean restart. +func TestRunningServerPortRejections(t *testing.T) { + tests := []struct { + name string + // setup populates the carts dir; nil leaves it empty. + setup func(t *testing.T, dir string) + }{ + { + name: "no state files", + setup: nil, + }, + { + name: "pid file names a reaped process", + setup: func(t *testing.T, dir string) { + writeState(t, dir, deadPID(t), freePort(t)) + }, + }, + { + name: "live pid but nothing serving the recorded port", + setup: func(t *testing.T, dir string) { + // PID recycling means a live PID is not proof the server is ours. + // This test process is definitely alive and definitely not dolt. + writeState(t, dir, os.Getpid(), freePort(t)) + }, + }, + { + name: "pid file is not a number", + setup: func(t *testing.T, dir string) { + mustWrite(t, filepath.Join(dir, pidFileName), "not-a-pid") + mustWrite(t, filepath.Join(dir, portFileName), "1") + }, + }, + { + name: "port file is missing", + setup: func(t *testing.T, dir string) { + mustWrite(t, filepath.Join(dir, pidFileName), strconv.Itoa(os.Getpid())) + }, + }, + } - proc, err := os.FindProcess(pid) - if err != nil { - t.Fatalf("FindProcess(%d): %v", pid, err) - } - if err := proc.Signal(syscall.Signal(0)); err == nil { - t.Errorf("signal 0 on reaped pid %d should fail", pid) - } - }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stubProbe(t) // nothing is healthy + dir := t.TempDir() + if tt.setup != nil { + tt.setup(t, dir) + } + if port, err := runningServerPort(dir); err == nil { + t.Fatalf("expected an error, got port %d", port) + } + }) + } } -// A stale PID file must not be trusted. Before the fix a losing starter could -// overwrite the state files of the server that won the lock race, leaving a -// dead PID recorded and the healthy server orphaned. -func TestRunningServerPortRejectsStalePID(t *testing.T) { +func TestRunningServerPortAcceptsHealthyServer(t *testing.T) { dir := t.TempDir() + port := freePort(t) + stubProbe(t, port) + writeState(t, dir, os.Getpid(), port) - cmd := exec.Command("true") - if err := cmd.Start(); err != nil { - t.Skipf("cannot spawn helper process: %v", err) + got, err := runningServerPort(dir) + if err != nil { + t.Fatalf("expected reuse of a healthy server, got error: %v", err) } - deadPID := cmd.Process.Pid - _ = cmd.Wait() + if got != port { + t.Errorf("expected port %d, got %d", port, got) + } +} - write(t, filepath.Join(dir, pidFileName), strconv.Itoa(deadPID)) - write(t, filepath.Join(dir, portFileName), "1") +// waitForOwnServerOrWinner is what lets a process that lost dolt's single-writer +// race reuse the winner instead of failing after a full timeout. +func TestWaitForOwnServerOrWinner(t *testing.T) { + t.Run("reports our own server once it comes up", func(t *testing.T) { + ourPort := freePort(t) + stubProbe(t, ourPort) - if _, err := runningServerPort(dir); err == nil { - t.Fatal("expected an error for a dead PID, got nil (caller would reuse a nonexistent server)") - } + winner, err := waitForOwnServerOrWinner(t.TempDir(), ourPort, time.Second) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if winner != 0 { + t.Errorf("expected 0 (our own server won), got peer port %d", winner) + } + }) + + t.Run("returns the peer port when a peer publishes a ready server", func(t *testing.T) { + dir := t.TempDir() + ourPort := freePort(t) + peerPort := freePort(t) + // Only the peer answers; ours never comes up, as when we lose the lock race. + stubProbe(t, peerPort) + writeState(t, dir, os.Getpid(), peerPort) + + winner, err := waitForOwnServerOrWinner(dir, ourPort, time.Second) + if err != nil { + t.Fatalf("expected the peer's port, got error: %v", err) + } + if winner != peerPort { + t.Errorf("expected peer port %d, got %d", peerPort, winner) + } + }) + + t.Run("errors when neither our server nor a peer appears", func(t *testing.T) { + stubProbe(t) // nothing is healthy + if _, err := waitForOwnServerOrWinner(t.TempDir(), freePort(t), 300*time.Millisecond); err == nil { + t.Fatal("expected a timeout error when nothing is serving") + } + }) } -func TestRunningServerPortErrorsWithoutStateFiles(t *testing.T) { - if _, err := runningServerPort(t.TempDir()); err == nil { - t.Fatal("expected an error when no PID file exists") - } +func writeState(t *testing.T, dir string, pid, port int) { + t.Helper() + mustWrite(t, filepath.Join(dir, pidFileName), strconv.Itoa(pid)) + mustWrite(t, filepath.Join(dir, portFileName), strconv.Itoa(port)) } -func write(t *testing.T, path, content string) { +func mustWrite(t *testing.T, path, content string) { t.Helper() if err := os.WriteFile(path, []byte(content), 0o644); err != nil { t.Fatalf("write %s: %v", path, err) From 84c9bc35cd81325e7bc117799fe8bd457500f754 Mon Sep 17 00:00:00 2001 From: Rupak Majumdar Date: Fri, 31 Jul 2026 11:06:46 +0200 Subject: [PATCH 3/3] fix(carts): never delete a peer's server metadata when losing the start race Co-Authored-By: SageOx --- internal/carts/server.go | 45 ++++++++++++++++++++++------------- internal/carts/server_test.go | 35 +++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 17 deletions(-) diff --git a/internal/carts/server.go b/internal/carts/server.go index 151c1c97..6f1a54c0 100644 --- a/internal/carts/server.go +++ b/internal/carts/server.go @@ -104,17 +104,6 @@ func startServer(cartsDir string) (int, error) { } logFile.Close() - // reap tears down the process we spawned and any half-written state. It stays - // armed from here until BOTH state files are on disk: a ready server with - // incomplete metadata is worse than no server, because nothing can find it to - // reuse and every later call starts another contender for dolt's lock. - reap := func() { - _ = cmd.Process.Kill() - _, _ = cmd.Process.Wait() - _ = os.Remove(filepath.Join(cartsDir, pidFileName)) - _ = os.Remove(filepath.Join(cartsDir, portFileName)) - } - // Wait for readiness BEFORE recording state. Writing the files first let a // starter that was about to lose dolt's write-lock race overwrite the // metadata of the server that won it, permanently orphaning the healthy @@ -127,27 +116,49 @@ func startServer(cartsDir string) (int, error) { // healthy server, and hands back the winner's port instead of failing. winner, err := waitForOwnServerOrWinner(cartsDir, port, 30*time.Second) if err != nil { - reap() + // Nothing published yet, so there is no state of ours to unwind — only the + // child. Deleting state files here would delete someone else's. + killChild(cmd) return 0, fmt.Errorf("server failed to start: %w", err) } if winner != 0 { // A peer won the lock and published a ready server; ours never came up. - reap() + // Kill only our child — the state files on disk are the WINNER's, and + // removing them would orphan the healthy server and send every later + // invocation into another doomed startup. + killChild(cmd) return winner, nil } - if err := os.WriteFile(filepath.Join(cartsDir, pidFileName), []byte(strconv.Itoa(cmd.Process.Pid)), 0o644); err != nil { - reap() + // From here the state files are ours, so a failed write must unwind both the + // child and whatever we already published: a ready server with incomplete + // metadata is worse than no server, because nothing can find it to reuse. + pidPath := filepath.Join(cartsDir, pidFileName) + portPath := filepath.Join(cartsDir, portFileName) + + if err := os.WriteFile(pidPath, []byte(strconv.Itoa(cmd.Process.Pid)), 0o644); err != nil { + killChild(cmd) + _ = os.Remove(pidPath) // may be partially written return 0, fmt.Errorf("write %s: %w", pidFileName, err) } - if err := os.WriteFile(filepath.Join(cartsDir, portFileName), []byte(strconv.Itoa(port)), 0o644); err != nil { - reap() + if err := os.WriteFile(portPath, []byte(strconv.Itoa(port)), 0o644); err != nil { + killChild(cmd) + _ = os.Remove(portPath) + _ = os.Remove(pidPath) // written by us moments ago; would name a dead process return 0, fmt.Errorf("write %s: %w", portFileName, err) } return port, nil } +// killChild terminates and reaps a dolt sql-server we spawned. It deliberately +// touches no state files: at every call site the published metadata either +// belongs to a peer or does not exist yet. +func killChild(cmd *exec.Cmd) { + _ = cmd.Process.Kill() + _, _ = cmd.Process.Wait() +} + // waitForOwnServerOrWinner polls until either the server we started on ourPort // accepts connections (returns 0, nil) or a concurrently-started peer publishes // a ready server (returns that peer's port, nil). It returns an error only if diff --git a/internal/carts/server_test.go b/internal/carts/server_test.go index 9129ad9e..fa742fa0 100644 --- a/internal/carts/server_test.go +++ b/internal/carts/server_test.go @@ -187,6 +187,41 @@ func TestWaitForOwnServerOrWinner(t *testing.T) { }) } +// A process that loses dolt's write-lock race must tear down only its own child. +// The state files at that moment belong to the WINNER; deleting them orphans the +// healthy server and sends every later invocation into another doomed startup. +func TestKillChildLeavesPublishedStateIntact(t *testing.T) { + dir := t.TempDir() + winnerPID, winnerPort := 4242, 54321 + writeState(t, dir, winnerPID, winnerPort) + + // A real child to stand in for the dolt server we started and lost with. + cmd := exec.Command(os.Args[0], "-test.run=^$") + if err := cmd.Start(); err != nil { + t.Fatalf("start helper process: %v", err) + } + pid := cmd.Process.Pid + + killChild(cmd) + + if processAlive(pid) { + t.Errorf("our child pid %d should have been reaped", pid) + } + assertFile(t, filepath.Join(dir, pidFileName), strconv.Itoa(winnerPID)) + assertFile(t, filepath.Join(dir, portFileName), strconv.Itoa(winnerPort)) +} + +func assertFile(t *testing.T, path, want string) { + t.Helper() + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s (winner metadata must survive): %v", filepath.Base(path), err) + } + if string(got) != want { + t.Errorf("%s = %q, want %q", filepath.Base(path), got, want) + } +} + func writeState(t *testing.T, dir string, pid, port int) { t.Helper() mustWrite(t, filepath.Join(dir, pidFileName), strconv.Itoa(pid))