Conversation
… against a working set, form the key with the normalized db name. Previously, this lock would accidentally allow concurrent access to writing the database working set value because a non-normalized database name like `db/main\x00/refs/heads/main` would allow access along with a normalized database name like `db\x00/refs/heads/main`. This did not impact correctness, since the working sets are safe for concurrent modification at the storage layer, but it could cause transient failures for a client if the optimistic lock retries failed sequentially enough times. Here we fix the bug so that the txLocks serialize access to the ref heads as expected.
There was a problem hiding this comment.
Pull request overview
This PR fixes transaction-commit serialization to ensure txLocks keying uses a normalized database name, preventing concurrent commit paths from bypassing the intended lock and causing transient optimistic-lock retry failures.
Changes:
- Removes stale commented-out imports from an existing regression test file.
- Adds a new integration test that stress-tests concurrent SQL writes +
DOLT_COMMITto exercise commit serialization behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| integration-tests/go-sql-server-driver/repro_10331_test.go | Cleans up commented-out imports in an existing concurrency regression test. |
| integration-tests/go-sql-server-driver/concurrent_writes_test.go | Adds a concurrent-writes integration test to validate stability under heavy commit contention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
integration-tests/go-sql-server-driver/concurrent_writes_test.go
Outdated
Show resolved
Hide resolved
integration-tests/go-sql-server-driver/concurrent_writes_test.go
Outdated
Show resolved
Hide resolved
| db, err := server.DB(driver.Connection{User: "root"}) | ||
| require.NoError(t, err) | ||
| db.SetMaxOpenConns(1) | ||
| conn, err := db.Conn(ctx) |
There was a problem hiding this comment.
Each writer goroutine creates a new *sql.DB via server.DB(...) but never closes it. Over many goroutines this can leak connections / background goroutines and make the test flaky. Close the per-worker DB (e.g., defer db.Close() after creation) or reuse a shared DB and create per-worker conns/transactions instead.
| conn, err := db.Conn(ctx) | ||
| if err != nil { | ||
| require.NoError(t, err) | ||
| } | ||
| defer func () { | ||
| require.NoError(t, conn.Close()) | ||
| }() |
There was a problem hiding this comment.
The error handling here is redundant/awkward (if err != nil { require.NoError(...) }) and the defer func () { spacing doesn’t match gofmt. Prefer a straight require.NoError(t, err) and a standard defer func() { ... }() to keep the test readable and consistent with the rest of the suite.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Previously, this lock would accidentally allow concurrent access to writing the database working set value because a non-normalized database name like
db/main\x00/refs/heads/mainwould allow access along with a normalized database name likedb\x00/refs/heads/main. This did not impact correctness, since the working sets are safe for concurrent modification at the storage layer, but it could cause transient failures for a client if the optimistic lock retries failed sequentially enough times.Here we fix the bug so that the txLocks serialize access to the ref heads as expected.