Skip to content

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

Open
pfcoperez wants to merge 2 commits into
mainfrom
DBI-798/connectors/mysql/improved-server-id-check
Open

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

Conversation

@pfcoperez

Copy link
Copy Markdown
Member

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.
@pfcoperez pfcoperez requested a review from a team as a code owner July 10, 2026 18:16
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

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

Comment thread flow/cmd/validate_mirror.go Outdated

// 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

❌ 4 Tests Failed:

Tests completed Failed Passed Skipped
3041 4 3037 371
View the top 3 failed test(s) by shortest run time
github.com/PeerDB-io/peerdb/flow/e2e::TestApiMariaDB
Stack Traces | 0.01s run time
=== RUN   TestApiMariaDB
=== PAUSE TestApiMariaDB
=== CONT  TestApiMariaDB
--- FAIL: TestApiMariaDB (0.01s)
2026/07/10 18:27:35 INFO Received AWS credentials from peer for connector: ci x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
2026/07/10 18:27:35 INFO Received AWS credentials from peer for connector: clickhouse x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
github.com/PeerDB-io/peerdb/flow/e2e::TestApiMy
Stack Traces | 0.02s run time
=== RUN   TestApiMy
=== PAUSE TestApiMy
=== CONT  TestApiMy
--- FAIL: TestApiMy (0.02s)
2026/07/10 18:32:53 INFO Received AWS credentials from peer for connector: ci x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
2026/07/10 18:32:53 INFO Received AWS credentials from peer for connector: clickhouse x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
github.com/PeerDB-io/peerdb/flow/e2e::TestApiMy/TestValidateCDCMirror_ServerIDPeerReuse
Stack Traces | 0.05s run time
=== RUN   TestApiMy/TestValidateCDCMirror_ServerIDPeerReuse
=== PAUSE TestApiMy/TestValidateCDCMirror_ServerIDPeerReuse
=== CONT  TestApiMy/TestValidateCDCMirror_ServerIDPeerReuse
2026/07/10 18:31:25 INFO Received AWS credentials from peer for connector: ci x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
2026/07/10 18:31:25 INFO Received AWS credentials from peer for connector: clickhouse x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
    api_test.go:381: 
        	Error Trace:	.../flow/e2e/api_test.go:381
        	Error:      	Received unexpected error:
        	            	rpc error: code = Internal desc = internal server error
        	Test:       	TestApiMy/TestValidateCDCMirror_ServerIDPeerReuse
    api_test.go:51: begin tearing down postgres schema api_evlgjbe7
2026/07/10 18:31:25 INFO fetched schema x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN} table=e2e_test_mych_yawdnspt.test_bit
--- FAIL: TestApiMy/TestValidateCDCMirror_ServerIDPeerReuse (0.05s)
github.com/PeerDB-io/peerdb/flow/e2e::TestApiMariaDB/TestValidateCDCMirror_ServerIDPeerReuse
Stack Traces | 0.06s run time
=== RUN   TestApiMariaDB/TestValidateCDCMirror_ServerIDPeerReuse
=== PAUSE TestApiMariaDB/TestValidateCDCMirror_ServerIDPeerReuse
=== CONT  TestApiMariaDB/TestValidateCDCMirror_ServerIDPeerReuse
2026/07/10 18:26:00 INFO Received AWS credentials from peer for connector: ci x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
2026/07/10 18:26:00 INFO Received AWS credentials from peer for connector: clickhouse x-peerdb-additional-metadata={Operation:FLOW_OPERATION_UNKNOWN}
    api_test.go:381: 
        	Error Trace:	.../flow/e2e/api_test.go:381
        	Error:      	Received unexpected error:
        	            	rpc error: code = Internal desc = internal server error
        	Test:       	TestApiMariaDB/TestValidateCDCMirror_ServerIDPeerReuse
    api_test.go:51: begin tearing down postgres schema api_umm4t365
--- FAIL: TestApiMariaDB/TestValidateCDCMirror_ServerIDPeerReuse (0.06s)

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

Copy link
Copy Markdown
Contributor

❌ 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

jgao54 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

(pending integrations test fixes)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants