DBI-798: Safer check on mirror validation with MySQL sources setting server_id by pfcoperez · Pull Request #4564 · PeerDB-io/peerdb

pfcoperez · 2026-07-10T18:16:23Z

It adds a check on the utilization of the same server_id in the source MySQL server.
This offer an extra layer of protection against collisions with other replication actors.

…server_id It adds a check on the utilization in the source DB of the server_id.

claude · 2026-07-10T18:29:40Z

Code review

Found 1 issue. Checked for bugs and CLAUDE.md compliance.

Bug: nil pointer dereference at flow/cmd/validate_mirror.go:199

mysqlConn.Conn() returns nil at this call site, causing a panic inside HasReplicaWithServerId.

MySqlConnector uses lazy connection initialization. NewMySqlConnector stores conn as nil and never calls connect(). The actual TCP connection is only established on-demand via Execute()/withRetries(), which internally call connect().

Conn() is just c.conn.Load() — a bare atomic load that returns nil until a connection has been established. Since connectors.GetAs only calls the constructor (no queries), Conn() returns nil. HasReplicaWithServerId then calls conn.Execute(SHOW REPLICAS) on nil, which panics.

Suggested fix: Change HasReplicaWithServerId to accept the MySqlConnector (or a context + an interface with Execute) so it goes through the lazy-connect path, or trigger connection establishment before calling Conn().

See: NewMySqlConnector, Conn(), HasReplicaWithServerId

jgao54 · 2026-07-10T18:30:18Z


+	// beyond other PeerDB mirrors (checked below), the pinned server_id must not be already registered
+	// as a replica on the source DB itself.
+	mysqlConn, mysqlClose, err := connectors.GetAs[*connmysql.MySqlConnector](ctx, cfg.Env, peer)


nit: can avoid an extra connect/disconnect by moving mysql_validation.HasReplicaWithServerId inside ValidateMirrorSource and pass in an already instantiated conn, although i understand that having this here put server_id validation check together. Your call.

I followed your suggestion at 19f4325, it not only saves the extra connection cycle but also cleans-up several boundary leaks.

codecov · 2026-07-10T18:36:43Z

❌ 2 Tests Failed:

Tests completed	Failed	Passed	Skipped
3025	2	3023	395

View the top 2 failed test(s) by shortest run time

github.com/PeerDB-io/peerdb/flow/e2e::TestPeerFlowE2ETestSuiteMariaDB_CH

Stack Traces | 0.01s run time

=== RUN   TestPeerFlowE2ETestSuiteMariaDB_CH
=== PAUSE TestPeerFlowE2ETestSuiteMariaDB_CH
=== CONT  TestPeerFlowE2ETestSuiteMariaDB_CH
--- FAIL: TestPeerFlowE2ETestSuiteMariaDB_CH (0.01s)

github.com/PeerDB-io/peerdb/flow/e2e::TestPeerFlowE2ETestSuiteMariaDB_CH/Test_MariaDB_PartialRowEvent

Stack Traces | 40s run time

=== RUN   TestPeerFlowE2ETestSuiteMariaDB_CH/Test_MariaDB_PartialRowEvent
=== PAUSE TestPeerFlowE2ETestSuiteMariaDB_CH/Test_MariaDB_PartialRowEvent
=== CONT  TestPeerFlowE2ETestSuiteMariaDB_CH/Test_MariaDB_PartialRowEvent
2026/07/13 09:44:58 INFO Received AWS credentials from peer for connector: ci x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
2026/07/13 09:44:58 INFO Received AWS credentials from peer for connector: clickhouse x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
2026/07/13 09:44:58 INFO fetched schema x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN} table=e2e_test_mach_httj25do.test_my_enum
    clickhouse_mysql_test.go:2226: 
        	Error Trace:	.../flow/e2e/mysql.go:130
        	            				.../flow/e2e/clickhouse_mysql_test.go:2226
        	            				.../hostedtoolcache/go/1.26.4.../src/runtime/asm_amd64.s:1771
        	Error:      	Received unexpected error:
        	            	MySQL execute error: readInitialHandshake: io.ReadFull(header) failed. err EOF: connection was bad
        	Test:       	TestPeerFlowE2ETestSuiteMariaDB_CH/Test_MariaDB_PartialRowEvent
2026/07/13 09:45:08 INFO fetched schema x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN} table=e2e_test_mach_w1r8soly.test_extra_ch_cols
--- FAIL: TestPeerFlowE2ETestSuiteMariaDB_CH/Test_MariaDB_PartialRowEvent (39.96s)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

github-actions · 2026-07-10T18:40:42Z

❌ Test Failure

Analysis: The PR's own new test TestValidateCDCMirror_ServerIDPeerReuse fails deterministically across all three MySQL matrix jobs with an "internal server error" RPC response, indicating a real bug in the new server_id validation code rather than a flaky/timing/network issue.
Confidence: 0.94

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

jgao54 · 2026-07-10T22:48:02Z

(pending integrations test fixes)

claude · 2026-07-12T16:32:34Z

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

…nector `ValidateMirrorSource` method. This not only saves a connection-disconnection cycle as suggested by the PR comment but also reduces the leaks of concrete connector details in mirror.

github-actions · 2026-07-13T09:55:25Z

🔄 Possible Flaky Test

Analysis: Only the mysql-pos e2e matrix failed (near the 1200s timeout with no captured --- FAIL/panic and a job that hung on teardown) while both mysql-gtid matrices passed on the same commit, pointing to a timing/infra flake rather than a server_id-validation regression that would affect both modes.
Confidence: 0.62

⚠️ Confidence too low (0.62) to retry automatically - manual review recommended

View workflow run

DBI-798: Safer check on mirror validation with MySQL sources setting …

19c30f3

…server_id It adds a check on the utilization in the source DB of the server_id.

pfcoperez requested a review from a team as a code owner July 10, 2026 18:16

jgao54 approved these changes Jul 10, 2026

View reviewed changes

jgao54 reviewed Jul 10, 2026

View reviewed changes

pfcoperez added 2 commits July 12, 2026 09:27

Reorder code

5b0ffb0

Skip check if mirror exists (resync cases)

977ccc3

pfcoperez added 2 commits July 13, 2026 10:48

Following a PR comment, I moved the server_id use check under the con…

19f4325

…nector `ValidateMirrorSource` method. This not only saves a connection-disconnection cycle as suggested by the PR comment but also reduces the leaks of concrete connector details in mirror.

Included e2e tests for server_id collisions

4e8ef97

pfcoperez merged commit 5db1ce0 into main Jul 13, 2026
20 of 21 checks passed

pfcoperez deleted the DBI-798/connectors/mysql/improved-server-id-check branch July 13, 2026 10:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DBI-798: Safer check on mirror validation with MySQL sources setting server_id#4564

DBI-798: Safer check on mirror validation with MySQL sources setting server_id#4564
pfcoperez merged 5 commits into
mainfrom
DBI-798/connectors/mysql/improved-server-id-check

pfcoperez commented Jul 10, 2026

Uh oh!

claude Bot commented Jul 10, 2026

Uh oh!

jgao54 Jul 10, 2026

Uh oh!

pfcoperez Jul 13, 2026

Uh oh!

codecov Bot commented Jul 10, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 10, 2026

Uh oh!

jgao54 commented Jul 10, 2026

Uh oh!

claude Bot commented Jul 12, 2026

Uh oh!

github-actions Bot commented Jul 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

pfcoperez commented Jul 10, 2026

Uh oh!

claude Bot commented Jul 10, 2026

Code review

Uh oh!

jgao54 Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

pfcoperez Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 2 Tests Failed:

Uh oh!

github-actions Bot commented Jul 10, 2026

❌ Test Failure

Uh oh!

jgao54 commented Jul 10, 2026

Uh oh!

claude Bot commented Jul 12, 2026

Code review

Uh oh!

github-actions Bot commented Jul 13, 2026

🔄 Possible Flaky Test

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 10, 2026 •

edited

Loading