fix(carts): repair dolt server reuse, auth, and start race - #736
Conversation
Co-Authored-By: SageOx <ox@sageox.ai>
📝 WalkthroughWalkthroughCart server startup now validates process liveness and authenticated port readiness. Concurrent starters detect a winner, clean up losing or failed child processes, and publish state only after readiness. Tests cover these paths. Dolt connections use ChangesServer lifecycle validation
Dolt SQL authentication
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant startServer
participant ChildProcess
participant ReadinessProbe
participant StateFiles
startServer->>ChildProcess: start server process
startServer->>ReadinessProbe: wait for local or peer readiness
alt local server is ready
ReadinessProbe-->>startServer: local port
startServer->>StateFiles: publish PID and port
else peer server is ready
ReadinessProbe-->>startServer: peer port
startServer->>ChildProcess: kill and reap child
else readiness fails
ReadinessProbe-->>startServer: timeout or error
startServer->>ChildProcess: kill and reap child
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/carts/server_test.go (1)
62-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the live-PID, non-serving-port path.
The stale-PID case exits at
Signal(0), so it never exercises the new readiness check. Add a table-driven case using the test process PID plus an intentionally closed local port, and assertrunningServerPortrejects it.As per coding guidelines,
*_test.gotests should “use table-driven tests” and “test error paths.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/carts/server_test.go` around lines 62 - 84, The existing tests cover stale and missing state, but not a live PID whose recorded port is not serving. Extend the runningServerPort tests with a table-driven case using the current test process PID and an intentionally closed local port, then assert that runningServerPort returns an error for that case while preserving the existing error-path coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/carts/server_test.go`:
- Around line 41-55: Update the “reaped process is detected as dead” and related
liveness subtests to spawn the current test binary via os.Args[0] with a no-op
test filter instead of relying on the external “true” executable. Replace the
conditional t.Skipf path with a test failure when cmd.Start cannot start,
ensuring the liveness assertions always execute.
In `@internal/carts/server.go`:
- Around line 114-126: Extend the cleanup lifecycle around the process started
by cmd.Start: arm a deferred reap-and-state-cleanup handler immediately after
startup, and disarm it only after both subsequent state-file writes complete
successfully. On either write failure, remove any partial state, terminate and
wait for the child, and return the write error wrapped with contextual
information.
- Around line 114-125: The startup path around waitForServer must handle
concurrent callers so a loser does not return an error when another process has
successfully started Dolt. Serialize startup with an inter-process lock, or
after killing and reaping the failed process, re-read runningServerPort and
return the existing winner’s port when available; otherwise preserve the current
startup error.
---
Nitpick comments:
In `@internal/carts/server_test.go`:
- Around line 62-84: The existing tests cover stale and missing state, but not a
live PID whose recorded port is not serving. Extend the runningServerPort tests
with a table-driven case using the current test process PID and an intentionally
closed local port, then assert that runningServerPort returns an error for that
case while preserving the existing error-path coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 06fb8aa8-283d-451b-9735-39034d19b596
📒 Files selected for processing (3)
internal/carts/server.gointernal/carts/server_test.gointernal/carts/store.go
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis change improves carts Dolt server startup and reuse by using the root SQL account, checking server availability before reuse, and allowing concurrent starters to join the winning server. A focused runtime check found that the shutdown path can still interrupt an unrelated local process when stale carts metadata names a recycled PID. Confidence Score: 3/5Not safe to merge until T-Rex findings are addressed. The startup and reuse behavior is improved, but a reproduced cleanup failure can terminate an unrelated local process when stale metadata references a live recycled PID. T-Rex reproduced 2 failing behaviors at runtime in internal/carts/server.go; the change needs fixes before it is safe to merge. Files Needing Attention: internal/carts/server.go
 T-Rex LogsWhat T-Rex did
|
…rite fail Co-Authored-By: SageOx <ox@sageox.ai>
…rt race Co-Authored-By: SageOx <ox@sageox.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/carts/server.go (1)
162-184: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce polling overhead in the peer-check path.
Each iteration of
waitForOwnServerOrWinnercallsrunningServerPortto check for a peer.runningServerPortitself callswaitForServerwith its own 2-second internal retry loop. If a peer's PID is alive but its port is not yet serving, a single outer iteration can block up to roughly 2.2 seconds instead of the intended 200ms cadence.This does not break correctness (the 30-second outer deadline is still honored), but it slows winner detection in exactly the case this function is meant to speed up.
Use a single-shot peer probe, for example by adding a
runningServerPortOncevariant (or a timeout parameter torunningServerPort) that skips the internal retry when called from this poll loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/carts/server.go` around lines 162 - 184, Reduce blocking in waitForOwnServerOrWinner by using a single-shot peer lookup instead of runningServerPort’s 2-second retry behavior. Add a runningServerPortOnce variant or equivalent timeout-aware path that performs one immediate peer probe, and call it from the polling loop while preserving the existing 200ms cadence and timeout behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/carts/server.go`:
- Around line 217-232: Update pingServer’s MySQL DSN to include short
connection, read, and write timeouts (for example, 1s each) before calling
db.Ping. Keep the existing root authentication, host/port targeting, and cleanup
behavior unchanged so all probeServer callers fail promptly for unresponsive
peers.
---
Nitpick comments:
In `@internal/carts/server.go`:
- Around line 162-184: Reduce blocking in waitForOwnServerOrWinner by using a
single-shot peer lookup instead of runningServerPort’s 2-second retry behavior.
Add a runningServerPortOnce variant or equivalent timeout-aware path that
performs one immediate peer probe, and call it from the polling loop while
preserving the existing 200ms cadence and timeout behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9131cc93-d319-4059-ba36-58147c29c71c
📒 Files selected for processing (4)
internal/carts/proc_unix.gointernal/carts/proc_windows.gointernal/carts/server.gointernal/carts/server_test.go
| // 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() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)internal/carts/(server|store)\.go$|go\.mod$'
echo
echo "== server.go relevant outline/sections =="
if [ -f internal/carts/server.go ]; then
wc -l internal/carts/server.go
ast-grep outline internal/carts/server.go --match probeServer --view expanded || true
sed -n '1,280p' internal/carts/server.go | cat -n
fi
echo
echo "== store.go relevant DSN usage =="
if [ -f internal/carts/store.go ]; then
rg -n 'sql\.Open|dsn|timeout|readTimeout|writeTimeout|time\.Duration' internal/carts/store.go
sed -n '1,220p' internal/carts/store.go | cat -n
fi
echo
echo "== go.mod pins =="
if [ -f go.mod ]; then
rg -n 'github.com/go-sql-driver/mysql|github.com/jmoiron/sqlx|dolt|module ' go.mod
fi
echo
echo "== all mysql DSNs =="
rg -n 'sql\.Open\("mysql"|timeout=|readTimeout=|writeTimeout=' internalRepository: sageox/ox
Length of output: 19546
🌐 Web query:
github.com/go-sql-driver/mysql DSN timeout readTimeout writeTimeout documentation
💡 Result:
The go-sql-driver/mysql library supports three distinct timeout parameters in its Data Source Name (DSN) string, all of which accept a string consisting of a decimal number followed by a unit suffix (e.g., "30s", "0.5m", "1m30s") [1][2]. 1. timeout: This is the driver-side connection (dial) timeout [1][3]. It specifies how long the driver will wait to establish the initial network connection to the MySQL server [1][4]. It does not govern the duration of the entire query execution or interaction after the connection is established [4][5]. 2. readTimeout: This parameter sets the I/O read timeout for the connection [6][2]. It controls how long the driver will wait to read data from the MySQL server after a command has been sent [2][7]. Its default value is 0 (no timeout) [2][7]. 3. writeTimeout: This parameter sets the I/O write timeout for the connection [6][2]. It controls how long the driver will wait to write data to the MySQL server [2][7]. Its default value is 0 (no timeout) [2][7]. These parameters are configured within the DSN string, often using the format user:password@tcp(address)/dbname?timeout=30s&readTimeout=10s&writeTimeout=10s [8]. Note that these are distinct from the connection pool settings managed by the standard library's database/sql package, such as SetConnMaxLifetime, which governs how long a connection may be reused [1][4].
Citations:
- 1: https://github.com/go-sql-driver/MYSQL
- 2: https://github.com/go-sql-driver/mysql/blob/d7ddb8b9e324830b1ede89c5fea090c824497c51/README.md
- 3: https://chromium.googlesource.com/external/github.com/go-sql-driver/mysql/+/efc283619ef617521ca327d5253108ef06e3f969/README.md
- 4: timeout option is invalid!!! go-sql-driver/mysql#488
- 5: https://github.blog/engineering/infrastructure/three-bugs-in-the-go-mysql-driver/
- 6: https://github.com/go-sql-driver/mysql/blob/master/dsn.go
- 7: https://chromium.googlesource.com/external/github.com/go-sql-driver/mysql.git/+/refs/tags/v1.9.2/README.md
- 8: https://github.com/go-sql-driver/mysql/blob/master/dsn_test.go
Add DSN timeouts to pingServer.
probeServer used by runningServerPort, waitForOwnServerOrWinner, and waitForServer opens MySQL without timeout, readTimeout, or writeTimeout. A slow/unresponsive peer can make db.Ping() block indefinitely, so add short probe timeouts such as timeout=1s&readTimeout=1s&writeTimeout=1s.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/carts/server.go` around lines 217 - 232, Update pingServer’s MySQL
DSN to include short connection, read, and write timeouts (for example, 1s each)
before calling db.Ping. Keep the existing root authentication, host/port
targeting, and cleanup behavior unchanged so all probeServer callers fail
promptly for unresponsive peers.
1 participant
Summary
ox cartsis usable again. Before this, the very first command on a machine failed outright, and once someone got past that, every command after the first failed until they manually killed a straydoltprocess:Three independent defects in the dolt sql-server lifecycle, each of which alone breaks the command.
Approach: connect as
rootrather than as the git author name, check liveness withsyscall.Signal(0)rather than a nil signal, and record the PID/port only after the readiness probe passes.1. The SQL user was the git author name
store.gobuilt the DSN fromCommitterName— a git author name likeAda Lovelace, not a SQL account.dolt sql-serveronly ever provisionsroot, so every command failed withAccess deniedunless the user happened to be namedroot.CommitterNameis now used solely for theDOLT_COMMITauthor, which is what it was for.2. Server reuse was dead code
runningServerPortprobed liveness withproc.Signal(os.Signal(nil)).os.Process.Signaltype-asserts its argument tosyscall.Signal, andnilfails that assertion, so the call always returned an error. Reuse never once succeeded: every invocation started anotherdolt sql-server, and all but the first died on dolt's exclusive write lock.A live PID also isn't proof the server is ours — the OS recycles PIDs — so reuse now additionally confirms something is serving the recorded port.
3. State files were written before the server was known to be up
startServerwrotedolt-server.{pid,port}immediately aftercmd.Start(), then waited for readiness. A starter about to lose the write-lock race would overwrite the metadata of the server that won it, orphaning the healthy server: every later invocation read the loser's dead PID and started yet another doomed one. The files are now written after the probe passes, and a failed start reaps the process it spawned instead of leaving it holding the lock.What changed
internal/carts/store.goroot;CommitterNamereserved for the commit authorinternal/carts/server.gosyscall.Signal(0)liveness check; port-serving confirmation; state files written post-readiness; spawned process reaped on failed startinternal/carts/server_test.goTest Plan
go test ./internal/carts/— passes, 5 new cases (3 liveness subtests + 2runningServerPortcases)go vet ./internal/carts/andgofmt -l— cleanox cartsinvocations succeed,pgrep dolt sql-servershows exactly one server, and the state files match the live PID/portox carts createsucceeds anddolt logshows the correct author (Name <email>)Why this went unnoticed
Each defect masks the others. (1) fires first and stops anyone from reaching (2) or (3). Someone who works around (1) hits (2) only on their second invocation, which reads as a flake. (3) only manifests under the lock race that (2) creates — fixing (2) alone would have made (3) rare rather than permanent. The package's existing tests are pure unit tests over IDs and validation and never exercise the server lifecycle.
Summary by CodeRabbit
Bug Fixes
Tests