Refuse XA workloads in the binlog stream instead of applying prepared rows

[morgo]

Why

MySQL writes row events for an XA transaction when it is prepared, before it knows whether the transaction will commit. Spirit could copy those rows to the target and leave them there after XA ROLLBACK. That can cause permanent data divergence.

What

Spirit now stops a migration when either binlog client sees an XA statement or XA_PREPARE_LOG_EVENT. This covers plain and compressed binlog events, including an XA COMMIT or XA ROLLBACK for a transaction prepared before Spirit connected.

How

Both clients parse each QueryEvent once. The parser's XA and transaction-control nodes determine whether to stop, keep a GTID pending, promote it, or notify a DDL subscriber. A migration fails on XA START, before that transaction's row events can enter a subscription buffer. Finite runs discard the checkpoint because replay would hit the same refused group.

XA tests run in their own GitHub Actions job with a separate MySQL server. The regular test jobs skip those tests. This prevents the XA events from interrupting other tests that share a server.

Risk

Any XA activity on the source server now stops the migration, including activity on an unrelated table. This is intentional while the binlog clients cannot safely apply prepared rows. Stop XA activity before starting a fresh migration or move. Datasync reports the distinct unsupported-xa reason.

Testing

No manual testing. GitHub Actions runs the MySQL integration suites, the isolated XA suite, and lint.

Generated with Codex

[Copilot]

Pull request overview

Adds runtime protection to Spirit’s binlog change stream so that any XA activity causes a fail-fast, checkpoint-preserving abort, preventing permanent target divergence from prepare-time row events being applied before an XA outcome is known.

Changes:

• Introduce a shared XA detection helper/error and use it to abort both change clients on XA QueryEvents and XA_PREPARE_LOG_EVENT (including within binlog_transaction_compression payloads).
• Update GTID client query-event processing to return fatal errors to the stream reader (instead of silently continuing).
• Add/adjust end-to-end and synthetic-stream tests ensuring the abort happens before buffering/applying any XA row events and that resume coordinates don’t advance past refused groups.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/change/utils.go	Adds shared XA guard error and helper used by both change clients.
pkg/change/gtid.go	GTID client: fail stream on XA QueryEvents / XA prepare event (incl. compressed payload path) and plumb fatal errors up to the reader loop.
pkg/change/gtid_test.go	Expands GTID tests to assert fail-fast behavior, empty buffers, preserved checkpoint semantics, and compressed-stream coverage.
pkg/change/binlog.go	Binlog (file/offset) client: add the same XA guards in both uncompressed and compressed event paths.
pkg/change/binlog_test.go	Adds binlog client tests mirroring GTID coverage for fail-fast XA handling and compressed payload guarding.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[aparajon]

🤖 Adversarial correctness review. I reviewed all nine files at b7d9303f in a worktree. I mutation-tested the four XA guards and the defense-in-depth branches, and ran all seven XA tests verbosely against an isolated MySQL 8.0.45 started from this PR's own compose.yml. I also probed the reworked processQueryEvent against origin/main for statements other than XA. Every one of the 13 workflows is green at head. The XA job's own log shows pkg/change executing tests (ok … 1.457s, not [no tests to run]), and the local -v run shows all seven running with none skipping themselves.

1 blocking, 4 non-blocking. The XA guard does what it says, and the tests pin it (below). The blocking finding is outside XA. Folding the query-event classification into one parse moved CREATE TABLE … START TRANSACTION behind an early return in the GTID client, and that return now skips the DDL notification as well as the promotion. spirit has no docs/invariants.md, so there is no invariant ID to cite here.

The XA guards are pinned: 4 of 5 mutations are caught, and the fifth is non-blocking finding 2

M1  binlog readStream: `if false && info.xa`            --- FAIL: TestBinlogClientXATransactionGuard
M2  gtid processQueryEvent: `if false && info.xa`       --- FAIL: TestGTIDClientXATransaction
                                                        --- FAIL: TestGTIDClientXATransactionCompression
                                                        --- FAIL: TestGTIDClientXAGuardStream
                                                        --- FAIL: TestGTIDProcessQueryEventXAGuard
                                                        --- FAIL: TestGTIDProcessTransactionPayloadXAGuard
M3  gtid readStream XA_PREPARE: promote + continue      --- FAIL: TestGTIDClientXAGuardStream
M4  binlog payload: `if false && info.xa`               --- FAIL: TestBinlogProcessTransactionPayloadXAGuard
M5  binlog readStream XA_PREPARE: `if false && …`       ok  (survives)

-run XA -v at head, on its own server:

--- PASS: TestBinlogClientXATransactionGuard (0.12s)
--- PASS: TestBinlogProcessTransactionPayloadXAGuard (0.05s)
--- PASS: TestGTIDClientXATransaction (0.10s)
--- PASS: TestGTIDClientXATransactionCompression (0.07s)
--- PASS: TestGTIDClientXAGuardStream (0.06s)
--- PASS: TestGTIDProcessQueryEventXAGuard (0.00s)
--- PASS: TestGTIDProcessTransactionPayloadXAGuard (0.05s)

Blocking

1. The GTID client no longer raises a DDL notification for CREATE TABLE … SELECT, so a whole-schema run (datasync, or a move with no SourceTables) stops cancelling when a table is created that way.

Since 8.0.21, MySQL binlogs CREATE TABLE … SELECT under RBR as Query("CREATE TABLE … START TRANSACTION") mid-group, as the comment at pkg/change/gtid_test.go:1372-1375 records. parseQueryEvent marks that statement opensTransaction and also returns its table. On origin/main, the GTID client deferred only the promotion for it and still notified:

// origin/main
if !opensTransaction {
    c.promotePendingGTID()
}
for _, ddlTable := range ddlTables {
    c.processDDLNotification(ddlTable.schema, ddlTable.table)
}

At head, the same flag now returns before the notification loop (pkg/change/gtid.go:800-802), so info.tables is never read for it (:816-819):

if info.opensTransaction || info.keepsTransactionOpen {
    return nil
}

Two callers depend on that notification. datasync sets DDLFilterSchema to the whole source schema (pkg/datasync/runner.go:749), and move does the same with DDLFilterTables empty when SourceTables is unset (pkg/move/runner.go:979-980). For both, any DDL in the schema is supposed to cancel the run as a schema change. After this PR, a CREATE TABLE t AS SELECT … on a GTID source goes unnoticed. The run keeps going and reports healthy, t never reaches the target, and the rows in the CTAS group's own row events belong to no subscription, so they are dropped. That is a silently incomplete target. The case is reachable on every GTID-enabled 8.0.21+ source, which covers all the GTID configurations in CI.

The binlog client did not change here: its loop over info.tables (pkg/change/binlog.go:800) has no early return, so the two clients now disagree on the same statement.

Nothing in the suite catches this. TestGTIDClientCreateTableAsSelect and the CTAS group in TestGTIDClientQueryPromotionOrdering create their tables in a schema no filter watches, and they assert only GTID promotion.

Test that fails on b7d9303f and passes on origin/main and with the fix

// A whole-schema DDL filter (datasync, or move with no SourceTables) must
// cancel on CREATE TABLE ... SELECT, which MySQL 8.0.21+ binlogs under RBR
// as "CREATE TABLE ... START TRANSACTION".
func TestGTIDClientCTASNotifiesSchemaFilter(t *testing.T) {
	empty, err := mysql.ParseMysqlGTIDSet("")
	require.NoError(t, err)
	var got []FatalReason
	c := &gtidClient{
		logger:           slog.Default(),
		subs:             newSubscriptionRegistry(),
		bufferedGTID:     empty,
		flushedGTID:      empty.Clone(),
		ddlFilterSchema:  "test",
		callerCancelFunc: func(r FatalReason) bool { got = append(got, r); return true },
	}
	require.NoError(t, c.processQueryEvent(&replication.QueryEvent{Schema: []byte("test"),
		Query: []byte("CREATE TABLE `ctas1` (\n  `a` int NOT NULL\n) START TRANSACTION")}))
	t.Logf("PROBE cancel reasons after CTAS = %v", got)
	require.Equal(t, []FatalReason{FatalReasonSchemaChange}, got,
		"CTAS in a whole-schema-filtered source must cancel as a schema change")
}

b7d9303f          PROBE cancel reasons after CTAS = []
                  --- FAIL: TestGTIDClientCTASNotifiesSchemaFilter (0.00s)
origin/main       PROBE cancel reasons after CTAS = [schema-change]
                  --- PASS
b7d9303f + fix    PROBE cancel reasons after CTAS = [schema-change]
                  --- PASS

(On origin/main, processQueryEvent returns nothing, so that run called it as a bare statement.)

The fix keeps the deferred promotion and restores the notification:

if info.keepsTransactionOpen {
    return nil
}
if info.endsTransaction {
    c.promotePendingGTID()
    return nil
}
// A statement that opens its group (CREATE TABLE ... START TRANSACTION)
// defers promotion to the group's terminator, but it is still DDL.
if !info.opensTransaction {
    c.promotePendingGTID()
}
for _, ddlTable := range info.tables {
    c.processDDLNotification(ddlTable.schema, ddlTable.table)
}

With that applied, the test above passes. So do TestGTIDClientQueryPromotionOrdering, TestGTIDClientCreateTableAsSelect, TestGTIDClientSavepointPromotionOrdering, TestGTIDClientUnparseableDDL, TestDDLNotification*, TestGTIDProcessDDLNotificationMoveStyle, and all seven XA tests. A bare BEGIN still leaves its GTID pending, because it sets opensTransaction, and it notifies nothing because it names no table.

Non-blocking

1. The XA stop is reported as FatalReasonStreamError, whose contract is "a retry can resume from it", and the runners pass that on to the operator. By this PR's own description, a resume cannot succeed.

FatalReasonStreamError is documented at pkg/change/config.go:21-24 as "persisted resume state remains valid and a retry can resume from it." The migration and move runners act on exactly that. They keep the checkpoint and log

fatal replication stream error; the checkpoint has been preserved — re-run spirit to resume the migration from it

(pkg/migration/runner.go:1377, and the same text for a move at pkg/move/runner.go:1536). The PR summary says the opposite for this cause: "A resume that replays the XA group stops again; the operation needs a fresh start." The refused group is never promoted, so the preserved checkpoint always sits before it, and every re-run replays it and stops. An operator, or any tooling that retries stream errors automatically, follows the only instruction on screen and loops. The errXAUnsupported text that names the real cause appears only on the change client's own log line, because CancelFunc carries only the reason code.

This is non-blocking because every retry fails closed, so integrity holds. What it costs is an instruction that cannot work. A dedicated reason value would fix both halves at once. Both runners' default arms already treat an unrecognized reason as "invalidate the checkpoint" (the comment at pkg/move/runner.go:1538-1540 spells this out), and that is the fresh start this failure needs. It would also let datasync's recorded error (pkg/datasync/runner.go:1527) name XA, not a generic stream-error.

2. The binlog client's XA_PREPARE_LOG_EVENT defense-in-depth branch in readStream is untested: M5 above removes it and every test still passes.

The GTID twin is pinned by TestGTIDClientXAGuardStream/XA_PREPARE_LOG_EVENT, and the binlog payload twin by the second half of TestBinlogProcessTransactionPayloadXAGuard. The uncompressed branch at pkg/change/binlog.go:853 has nothing pinning it.

Test that passes on b7d9303f and fails under M5

// TestBinlogClientXAPrepareEventGuard pins readStream's defense-in-depth
// branch: a lone XA_PREPARE_LOG_EVENT must fail the stream as a stream error.
func TestBinlogClientXAPrepareEventGuard(t *testing.T) {
	db, err := dbconn.New(testutils.DSN(), dbconn.NewDBConfig())
	require.NoError(t, err)
	defer utils.CloseAndLog(db)
	cfg, err := mysql2.ParseDSN(testutils.DSN())
	require.NoError(t, err)

	var gotReason atomic.Int64
	gotReason.Store(-1)
	clientConfig := NewClientDefaultConfig()
	clientConfig.CancelFunc = func(reason FatalReason) bool {
		gotReason.Store(int64(reason))
		return true
	}
	client := NewBinlogClient(db, cfg.Addr, cfg.User, cfg.Passwd, applier.NewSingleTargetForTest(t, db), clientConfig).(*binlogClient)
	streamer := replication.NewBinlogStreamer()
	ctx, cancel := context.WithCancel(t.Context())
	client.streamer = streamer
	client.cancelFunc = cancel
	client.streamWG.Add(1)
	go client.readStream(ctx)
	defer client.Close()

	require.NoError(t, streamer.AddEventToStreamer(&replication.BinlogEvent{
		Header: &replication.EventHeader{EventType: replication.XA_PREPARE_LOG_EVENT},
		Event:  &replication.GenericEvent{},
	}))
	require.Eventually(t, func() bool { return gotReason.Load() == int64(FatalReasonStreamError) },
		5*time.Second, 5*time.Millisecond, "a lone XA_PREPARE_LOG_EVENT must fail the stream")
}

b7d9303f   --- PASS: TestBinlogClientXAPrepareEventGuard (0.03s)
M5         Error: Condition never satisfied
           --- FAIL: TestBinlogClientXAPrepareEventGuard (5.02s)

Its name contains XA, so it lands in the isolated job, as it should.

3. Every transaction's BEGIN now goes through a freshly built parser on the GTID client's single reader goroutine.

origin/main returned early on strings.EqualFold(q, "BEGIN") before any parse. At head, processQueryEvent calls parseQueryEvent first (pkg/change/gtid.go:752), and that builds a new parser.New() on every call. On this machine:

BenchmarkZZParseBegin-16       614127    1944 ns/op   17168 B/op   8 allocs/op
BenchmarkZZFastPathBegin-16 215783950    5.519 ns/op      0 B/op   0 allocs/op

That is a ~350× increase and 17 KB of garbage for every transaction on the source server, whatever schema it is in, all on the goroutine that bounds how fast the stream drains. The binlog client always parsed every QueryEvent, so it is unchanged. Keeping the BEGIN fast path ahead of parseQueryEvent in the GTID client gives the common case its old cost back and leaves the classification change intact.

4. Two comments no longer match the code.

• pkg/change/gtid_test.go:427-429 says "XA statements are also unparseable but never reach the parser." At head, they reach the parser and parse. *ast.XAStmt is how the guard detects them.
• pkg/change/gtid.go:815 is a bare //, left behind when the paragraph under it (the explanation of the opensTransaction exception) was deleted. Once blocking 1 is fixed, a sentence there belongs to the reinstated !info.opensTransaction guard.

Checked, not findings

• The CI split does what it claims. Every runner that shares a server with the general suite skips XA. That covers the compose.yml test service (the 8.0.28 + nogtid.yml job), and replication-ci.yml's test with its 8.0.42 / 8.4 / 9.7 overlays and the base 8.0.45 job. semisync.yml and the single-version runner already use narrow -run patterns, and unit-tls has no server. -skip and -run match only the top-level test name, so TestParseQueryEventClassification, whose subtests are named XA …, still runs in the general suite. The new workflow is a faithful copy of mysql8-docker.yml and adds --exit-code-from xa-test, so a failure there fails the job.
• Refusing a terminal XA COMMIT closes a data-loss gap that predates this PR. Take a transaction prepared before the stream connected. Its row events predate the start position, and the copier cannot see its uncommitted rows. On origin/main, the later XA COMMIT was promoted with nothing to apply, and the rows were lost. At head it is refused, and TestGTIDClientXAGuardStream/terminal_XA_COMMIT pins this.
• CTAS promotion deferral is preserved. CreateTableStmt.StartTransaction still sets opensTransaction, so the promotion still waits for the group's XIDEvent. Blocking 1 is only about the notification.
• Savepoint classification is now AST-based and pinned. ROLLBACK TO is told apart from ROLLBACK by SavepointName, and TestParseQueryEventClassification fails if the two are swapped.
• Copilot's one inline comment is about the isXAStatement doc comment. That helper no longer exists at head, so the comment is obsolete.

This review was generated by Claude Code (claude-opus-5-5).

[JashLal]

🤖 Adversarial correctness review on Jash’s behalf at b7d9303f.

Verdict: the XA refusal addresses prepared-row divergence, but the existing CTAS notification regression remains blocking.

1. Blocking — preserve DDL notifications for transaction-opening CREATE TABLE events. I independently reproduced the issue in the existing review: pkg/change/gtid.go:800–802 returns for opensTransaction before reaching the notification loop. A CREATE TABLE … START TRANSACTION event leaves the GTID pending as intended but does not cancel a whole-schema subscriber. Datasync and whole-schema moves can consequently continue without the newly created table. This corroborates the existing finding rather than adding a separate issue.

1. Restore DDL notification while deferring only GTID promotion for transaction-opening DDL, and add a regression test asserting both the schema-change cancellation and the still-pending GTID.
2. (Optional, already raised in the linked review) Distinguish unsupported XA from retryable stream failures in recovery guidance: replaying the preserved checkpoint reaches the same refused group again.

Verified: reviewed all nine changed files and the surrounding stream, payload, GTID promotion, DDL-filter, and runner cancellation paths; XA query rejection precedes row dispatch in both clients, compressed errors propagate to the fatal path, and terminal XA events are refused. The replaced XA-success assertions are superseded by refusal/zero-buffer assertions. Locally, go build ./..., TestParseQueryEventClassification, and TestGTIDProcessQueryEventXAGuard passed; a separate Go-overlay CTAS regression probe failed at the missing cancellation assertion while its pending-GTID assertion passed. All 17 GitHub checks are green, including the isolated XA job; the old Copilot helper-comment thread is resolved/outdated. I did not run the MySQL integration suites locally because the Docker daemon check did not respond.

Reviewed by Codex (GPT-6).

[aparajon]

🤖 Stamped: 1 blocking, 4 non-blocking. See the review comment: #1079 (comment). The blocking finding is that the GTID client drops the CTAS DDL notification; it still needs fixing before merge.

This stamp was left by Claude Code (claude-opus-5-5).