Skip to content

fixed test flakiness in replication tests #2943

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zachmu wants to merge 13 commits into
base: main
Choose a base branch
from
Open

fixed test flakiness in replication tests #2943

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a04a905
fixed test flakiness caused by eventual consistency of asynchronously…
zachmu Jul 16, 2026
0ba9028
more generous timeouts for replication test
zachmu Jul 16, 2026
324daf8
new dolt
zachmu Jul 16, 2026
40d51b1
merge main
zachmu Jul 17, 2026
6601ef6
new dolt]
zachmu Jul 17, 2026
36724f3
more retries
zachmu Jul 17, 2026
4e364ef
new dolt
zachmu Jul 17, 2026
5569846
new dolt
zachmu Jul 17, 2026
5d864de
new dolt
zachmu Jul 17, 2026
3e09f9e
merge main
zachmu Jul 17, 2026
e77b70a
turn macos back on
zachmu Jul 17, 2026
97f2bc1
new dolt
zachmu Jul 17, 2026
dac6ead
new dolt
zachmu Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion integration-tests/go-sql-server-driver/driver/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ var DelvePath string
const TestUserName = "Bats Tests"
const TestEmailAddress = "bats@email.fake"

const ConnectAttempts = 50
// ConnectAttempts and RetrySleepDuration are used by the test driver to retry connections to a doltgres server
// that is starting up.
const ConnectAttempts = 200
const RetrySleepDuration = 50 * time.Millisecond

// EnvDoltgresBinPath is the environment variable used to locate the doltgres
Expand Down
21 changes: 20 additions & 1 deletion integration-tests/go-sql-server-driver/testdef.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"io"
"os"
"path/filepath"
"strings"
"sync"
"testing"
"text/template"
Expand Down Expand Up @@ -629,12 +630,30 @@ func RetryTestRun(t *testing.T, attempts int, test func(TestingT)) {
rtt.try(attempts, test)
}

// statusEventualConsistencyAttempts bounds how long RunQuery waits for a cluster
// replication status query to converge to its expected result.
// Retries are quite forgiving in these tests because the default `postgres` database in most of these tests is never
// written to, which means it get replicated async by a background thread, rather than by a user write.
const statusEventualConsistencyAttempts = 300

@itoqa itoqa Bot Jul 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

View All Evidence

Minor severity Unrelated query text inflates the replication retry budget

What failed: Nine of ten STATUS-2 subtests failed. Queries such as select 1 -- see dolt_cluster_status, select 'dolt_cluster_status' as label, and select * from dolt_cluster_status_archive were classified as status queries, and the comment-only case showed the retry budget changing from 5 to 300.

Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
  • Severity: Minor Minor severity
  • Impact: Queries whose comments, string literals, or similarly named identifiers contain dolt_cluster_status can wait about 15 seconds before reporting a persistent error. The affected query eventually reports the error, with no evidence of data loss, corruption, or security exposure.
  • Steps to Reproduce:
    1. Run the STATUS-2 Go test with go test -run TestSTATUS2 -count=1 -v in integration-tests/go-sql-server-driver.
    2. Pass an ordinary query containing dolt_cluster_status only in a SQL comment, string literal, or unrelated identifier, with RetryAttempts set to 5.
    3. Observe that observesReplicationStatus returns true and RunQuery uses 300 attempts rather than the configured budget.
  • Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
  • Code Analysis: The PR-added observesReplicationStatus implementation at lines 652-655 lowercases q.Query and performs an unrestricted strings.Contains match, so it does not distinguish SQL comments or literals from referenced tables and does not enforce identifier boundaries. RunQuery at lines 640-645 consumes that result and promotes attempts below statusEventualConsistencyAttempts, which is 300 at line 637. The smallest practical fix is to parse or sanitize SQL sufficiently to remove comments and literals, then match the exact dolt_cluster_status identifier or referenced relation rather than an arbitrary substring.
  • Why this is likely a bug: The failing cases are deterministic and exercise ordinary SQL text that is not a reference to the dolt_cluster_status system table, while the source directly shows that any occurrence of the text triggers the status retry path. This causes a measurable, user-visible delay in the integration driver's normal error reporting and is not explained by setup or environment behavior.
Relevant code

integration-tests/go-sql-server-driver/testdef.go:637-645

const statusEventualConsistencyAttempts = 300; RunQuery promotes attempts to this value when observesReplicationStatus(q) is true.

integration-tests/go-sql-server-driver/testdef.go:652-655

func observesReplicationStatus(q driver.Query) bool { return q.Query != "" && strings.Contains(strings.ToLower(q.Query), "dolt_cluster_status") }
Evidence Package
Copy prompt for an agent 
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.

**Minor severity — Unrelated query text inflates the replication retry budget**

**What failed:** Nine of ten STATUS-2 subtests failed. Queries such as select 1 -- see dolt_cluster_status, select 'dolt_cluster_status' as label, and select * from dolt_cluster_status_archive were classified as status queries, and the comment-only case showed the retry budget changing from 5 to 300.

- **Impact:** Queries whose comments, string literals, or similarly named identifiers contain dolt_cluster_status can wait about 15 seconds before reporting a persistent error. The affected query eventually reports the error, with no evidence of data loss, corruption, or security exposure.
- **Steps to reproduce:**
  1. Run the STATUS-2 Go test with go test -run TestSTATUS2 -count=1 -v in integration-tests/go-sql-server-driver.
  2. Pass an ordinary query containing dolt_cluster_status only in a SQL comment, string literal, or unrelated identifier, with RetryAttempts set to 5.
  3. Observe that observesReplicationStatus returns true and RunQuery uses 300 attempts rather than the configured budget.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR-added observesReplicationStatus implementation at lines 652-655 lowercases q.Query and performs an unrestricted strings.Contains match, so it does not distinguish SQL comments or literals from referenced tables and does not enforce identifier boundaries. RunQuery at lines 640-645 consumes that result and promotes attempts below statusEventualConsistencyAttempts, which is 300 at line 637. The smallest practical fix is to parse or sanitize SQL sufficiently to remove comments and literals, then match the exact dolt_cluster_status identifier or referenced relation rather than an arbitrary substring.
- **Why this is likely a bug:** The failing cases are deterministic and exercise ordinary SQL text that is not a reference to the dolt_cluster_status system table, while the source directly shows that any occurrence of the text triggers the status retry path. This causes a measurable, user-visible delay in the integration driver's normal error reporting and is not explained by setup or environment behavior.

**Relevant code:**

`integration-tests/go-sql-server-driver/testdef.go:637-645`

~~~go
const statusEventualConsistencyAttempts = 300; RunQuery promotes attempts to this value when observesReplicationStatus(q) is true.
~~~

`integration-tests/go-sql-server-driver/testdef.go:652-655`

~~~go
func observesReplicationStatus(q driver.Query) bool { return q.Query != "" && strings.Contains(strings.ToLower(q.Query), "dolt_cluster_status") }
~~~


func RunQuery(t *testing.T, conn *sql.Conn, q driver.Query, ports *DynamicResources) {
RetryTestRun(t, q.RetryAttempts, func(t TestingT) {
attempts := q.RetryAttempts
// Replication status is eventually consistent, so status queries always get at
// least the generous eventual-consistency budget, even if the test set a smaller
// (or no) retry_attempts.
if observesReplicationStatus(q) && attempts < statusEventualConsistencyAttempts {
attempts = statusEventualConsistencyAttempts
}
RetryTestRun(t, attempts, func(t TestingT) {
RunQueryAttempt(t, conn, q, ports)
})
}

// observesReplicationStatus reports whether the query reads the dolt_cluster_status system table
func observesReplicationStatus(q driver.Query) bool {
return q.Query != "" && strings.Contains(strings.ToLower(q.Query), "dolt_cluster_status")
}

func RunQueryAttempt(t TestingT, conn *sql.Conn, q driver.Query, ports *DynamicResources) {
args := make([]any, len(q.Args))
for i := range q.Args {
Expand Down
271 changes: 271 additions & 0 deletions integration-tests/go-sql-server-driver/tests/sql-server-cluster.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2186,3 +2186,274 @@ tests:
result:
columns: ['read_only']
rows: [['1']]
# The tests below exercise cluster replication of the auto-created "postgres"
# default database directly (the only database many customers will ever create)
- name: postgres default database replicates to standby
multi_repos:
- name: server1
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server2_cluster"}}/{database}
bootstrap_role: primary
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server1_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server1
- name: server2
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server1_cluster"}}/{database}
bootstrap_role: standby
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server2_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server2
connections:
- on: server1
queries:
- exec: 'SET dolt_cluster_ack_writes_timeout_secs = 10'
- exec: 'CREATE TABLE vals (i INT PRIMARY KEY)'
- exec: 'INSERT INTO vals VALUES (0),(1),(2),(3),(4)'
- on: server2
queries:
- query: 'SELECT count(*) FROM vals'
result:
columns: ["count"]
rows: [["5"]]
- name: postgres default database on booted standby is read only
multi_repos:
- name: server1
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server2_cluster"}}/{database}
bootstrap_role: primary
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server1_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server1
- name: server2
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server1_cluster"}}/{database}
bootstrap_role: standby
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server2_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server2
connections:
- on: server1
queries:
- exec: 'SET dolt_cluster_ack_writes_timeout_secs = 10'
- exec: 'CREATE TABLE vals (i INT PRIMARY KEY)'
- exec: 'INSERT INTO vals VALUES (0),(1),(2)'
- on: server2
queries:
- query: 'SELECT count(*) FROM vals'
result:
columns: ["count"]
rows: [["3"]]
- exec: 'INSERT INTO vals VALUES (3)'
error_match: "postgres is read-only"
- name: postgres default database survives failover
multi_repos:
- name: server1
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server2_cluster"}}/{database}
bootstrap_role: primary
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server1_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server1
- name: server2
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server1_cluster"}}/{database}
bootstrap_role: standby
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server2_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server2
connections:
- on: server1
queries:
- exec: 'SET dolt_cluster_ack_writes_timeout_secs = 10'
- exec: 'CREATE TABLE vals (i INT PRIMARY KEY)'
- exec: 'INSERT INTO vals VALUES (1),(2),(3)'
# Fail over gracefully: demote the primary first, then promote the standby.
- on: server1
queries:
- query: "select dolt_assume_cluster_role('standby', 2)"
result:
columns: ["dolt_assume_cluster_role"]
rows: [["{0}"]]
- on: server2
queries:
- query: "select dolt_assume_cluster_role('primary', 2)"
result:
columns: ["dolt_assume_cluster_role"]
rows: [["{0}"]]
- on: server2
queries:
- exec: 'SET dolt_cluster_ack_writes_timeout_secs = 10'
- query: 'SELECT count(*) FROM vals'
result:
columns: ["count"]
rows: [["3"]]
- exec: 'INSERT INTO vals VALUES (4),(5)'
- on: server1
queries:
- query: 'SELECT count(*) FROM vals'
result:
columns: ["count"]
rows: [["5"]]
- name: postgres default database replication status
multi_repos:
- name: server1
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server2_cluster"}}/{database}
bootstrap_role: primary
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server1_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server1
- name: server2
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server1_cluster"}}/{database}
bootstrap_role: standby
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server2_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server2
connections:
- on: server1
queries:
- exec: 'SET dolt_cluster_ack_writes_timeout_secs = 10'
- exec: 'CREATE TABLE vals (i INT PRIMARY KEY)'
- exec: 'INSERT INTO vals VALUES (0),(1),(2)'
- on: server1
database: dolt_cluster
queries:
- query: "select \"database\", standby_remote, role, epoch, replication_lag_millis, current_error from dolt_cluster_status order by \"database\" asc"
result:
columns: ["database","standby_remote","role","epoch","replication_lag_millis","current_error"]
rows:
- ["postgres","standby","primary","1","0","NULL"]
- name: postgres default database sequence val maintained across failover
multi_repos:
- name: server1
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server2_cluster"}}/{database}
bootstrap_role: primary
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server1_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server1
- name: server2
with_files:
- name: server.yaml
contents: |
log_level: trace
cluster:
standby_remotes:
- name: standby
remote_url_template: http://localhost:{{get_port "server1_cluster"}}/{database}
bootstrap_role: standby
bootstrap_epoch: 1
remotesapi:
port: {{get_port "server2_cluster"}}
server:
args: ["--config", "server.yaml"]
dynamic_port: server2
connections:
- on: server1
queries:
- exec: 'SET dolt_cluster_ack_writes_timeout_secs = 10'
- exec: 'CREATE TABLE t (id SERIAL PRIMARY KEY, v INT)'
- exec: 'INSERT INTO t(v) VALUES (0),(0),(0),(0),(0)'
- on: server2
queries:
- query: 'SELECT * FROM t ORDER BY id'
result:
columns: ['id', 'v']
rows: [['1','0'], ['2','0'], ['3','0'], ['4','0'], ['5','0']]
- on: server1
queries:
- exec: "select dolt_assume_cluster_role('standby', 2)"
- on: server2
queries:
- exec: "select dolt_assume_cluster_role('primary', 2)"
- on: server2
queries:
- exec: 'SET dolt_cluster_ack_writes_timeout_secs = 10'
- exec: 'INSERT INTO t(v) VALUES (1)'
- query: 'SELECT * FROM t ORDER BY id'
result:
columns: ['id', 'v']
rows: [['1','0'], ['2','0'], ['3','0'], ['4','0'], ['5','0'], ['6','1']]
14 changes: 7 additions & 7 deletions server/connection_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,10 +307,12 @@ func (h *ConnectionHandler) chooseInitialParameters(startupMessage *pgproto3.Sta
}
}
}
// set initial database
// Set the initial database. Postgres has no concept of a session without a current database: if the client
// doesn't specify one, it defaults to the username (matching libpq). Either way, if the resolved database
// doesn't exist we must reject the connection rather than proceed with a database-less session, which would
// break assumptions throughout the engine.
db, ok := startupMessage.Parameters["database"]
dbSpecified := ok && len(db) > 0
if !dbSpecified {
if !ok || len(db) == 0 {
db = h.mysqlConn.User
}
useStmt := fmt.Sprintf("SET database TO '%s';", db)
Expand All @@ -322,13 +324,11 @@ func (h *ConnectionHandler) chooseInitialParameters(startupMessage *pgproto3.Sta
err = h.doltgresHandler.ComQuery(context.Background(), h.mysqlConn, useStmt, parsed, func(_ *sql.Context, _ *Result) error {
return nil
})
// If a database isn't specified, then we attempt to connect to a database with the same name as the user,
// ignoring any error
if err != nil && dbSpecified {
if err != nil {
_ = h.send(&pgproto3.ErrorResponse{
Severity: string(ErrorResponseSeverity_Fatal),
Code: "3D000",
Message: fmt.Sprintf(`"database "%s" does not exist"`, db),
Message: fmt.Sprintf(`database "%s" does not exist`, db),
Routine: "InitPostgres",
})
return err
Expand Down
Loading