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 bc6be240..6f1a54c0 100644 --- a/internal/carts/server.go +++ b/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) } diff --git a/internal/carts/server_test.go b/internal/carts/server_test.go new file mode 100644 index 00000000..fa742fa0 --- /dev/null +++ b/internal/carts/server_test.go @@ -0,0 +1,236 @@ +package carts + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "strconv" + "testing" + "time" +) + +// 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 + } + } + 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("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) + } + }) +} + +// 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())) + }, + }, + } + + 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) + } + }) + } +} + +func TestRunningServerPortAcceptsHealthyServer(t *testing.T) { + dir := t.TempDir() + port := freePort(t) + stubProbe(t, port) + writeState(t, dir, os.Getpid(), port) + + got, err := runningServerPort(dir) + if err != nil { + t.Fatalf("expected reuse of a healthy server, got error: %v", err) + } + if got != port { + t.Errorf("expected port %d, got %d", port, got) + } +} + +// 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) + + 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") + } + }) +} + +// 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)) + mustWrite(t, filepath.Join(dir, portFileName), strconv.Itoa(port)) +} + +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) + } +} 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 {