fix(carts): repair dolt server reuse, auth, and start race

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

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

internal/carts/server.go

@@ -42,14 +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 process exists
-	if err := proc.Signal(os.Signal(nil)); 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))
@@ -60,6 +58,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,22 +104,85 @@ func startServer(cartsDir string) (int, error) {
 	}
 	logFile.Close()
 
-	// Write state files
-	if err := os.WriteFile(filepath.Join(cartsDir, pidFileName), []byte(strconv.Itoa(cmd.Process.Pid)), 0o644); err != nil {
-		return 0, err
+	// 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.
+	//
+	// 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 {
+		// 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 err := os.WriteFile(filepath.Join(cartsDir, portFileName), []byte(strconv.Itoa(port)), 0o644); err != nil {
-		return 0, err
+	if winner != 0 {
+		// A peer won the lock and published a ready server; ours never came up.
+		// 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
 	}
 
-	// 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)
+	// 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(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
+// 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 {
@@ -146,18 +214,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)
 	}