fix(change): refuse a wrapped LogPos and unknown rows-event subtypes

[morgo]

Two robustness fixes in pkg/change binlog event handling, both in the "silent data loss" class: row changes that arrive on the stream and never make it into the buffer.

Bug A: a wrapped LogPos makes the replay-skip guard discard live row events

Problem. A binlog event header's end position is a 4-byte field, and a binlog file only rotates on a transaction boundary — so one transaction writing more than 4GiB of row images past the max_binlog_size point grows the file beyond what a uint32 can address, and positions restart near zero. MySQL documents the consequence under max_binlog_cache_size: with gtid_mode off, "the maximum recommended value is 4GB, because in that case MySQL cannot work with binary log positions greater than 4GB". max_binlog_cache_size defaults to 16EB on 64-bit, so nothing stops a single transaction from doing this.

Past the wrap, setBufferedPos's monotonicity freezes bufferedPos just under the wrap point, every later RowsEvent compares at or below it, and shouldSkipReplayedEvent classifies them all as post-reconnect replays and discards them. The checkpoint stops advancing at the same moment. The lost changes were only caught by the checksum, where enabled.

Scope. Only the binlog (file + position) client. The GTID client does not deduplicate by position and has no bufferedPos — which is exactly why MySQL's own caveat is scoped to gtid_mode being off. Since #1139 auto-detects GTIDs, this bites servers that have them disabled.

Fix. readStream detects the wrap directly rather than inferring it. Within a binlog file an event's end position only ever increases, so a backwards step is the signature of the field wrapping. The detector resets on every RotateEvent — which is also what keeps recreateStreamer's replay from position 4 from looking like a wrap, since the server prefaces every dump with an artificial rotate. Positions the server synthesized rather than read from the file are excluded: LogPos == 0, events flagged LOG_EVENT_ARTIFICIAL_F, and heartbeats (whose position can name a file the reader has not rotated into yet).

On detection the stream fails with a new FatalReasonLogPosWrapped. That reason invalidates the caller's checkpoint, alongside FatalReasonUnsupportedXA: the checkpointed position is technically readable, but resuming from it streams straight back into the same oversized transaction and wraps again, so a resume can never make progress. A fresh run started once the file has rotated gets usable coordinates.

Design note. An earlier revision of this PR tried to keep the migration running by classifying wrapped events heuristically (a gap of more than 2^31 below bufferedPos, suppressed inside a post-reconnect replay window) and re-delivering them. That was dropped for two reasons. It needed a threshold and a replaying state machine in the hot path to approximate something the rotate reset answers exactly, and it was only a half-fix: past the wrap bufferedPos/flushedPos cannot advance until the next rotation, so the run would continue with a checkpoint that silently stopped tracking reality. Refusing is the same posture as #1079's XA refusal — do not apply what cannot be tracked correctly.

Residual, documented in code. A run that starts at an already-wrapped position (a fresh run against a server currently mid-oversized-transaction) reads from a garbage offset; that is a property of 32-bit coordinates, not something this guard can see. The recommendation in the error message is to enable GTIDs, or lower max_binlog_cache_size so no transaction can grow a file past 4GiB.

Bug B: unknown rows-event subtypes were dropped with only an error log

Problem. go-mysql parses more rows-event subtypes into a plain *replication.RowsEvent than this package knows how to apply — parser.go:323 routes PARTIAL_UPDATE_ROWS_EVENT there alongside the recognized ones. The server emits it once binlog_row_value_options=PARTIAL_JSON is set, and preflight reads that global once at start, so it can change afterwards. Such an event reached processRowsEvent (both clients), fell into the default: branch, and had its rows skipped with only logger.Error("unknown event type") — no fatalError(). That is asymmetric with the minimal-row-image check immediately above it, which hard-fails.

Fix. An unrecognized rows-event subtype for a subscribed table is now a hard error from processRowsEvent in both clients, naming the type both ways (PartialUpdateRowsEvent (0x27) — an event type new enough that go-mysql has no name for it renders as "Unknown", and the header byte is then the only identifier in a log line) and the table. It flows through the existing fatal path exactly like the minimal-row-image case. The insert/delete loop's default: branch becomes a hard error too, so a future eventType addition cannot reintroduce a silent drop. Events for unsubscribed tables (e.g. _new tables) are still ignored.

Testing

• TestLogPosTracker — the detector in isolation: ascending positions and repeated positions are clean, a backwards step reports a wrap and does not advance the tracker, a rotate reset makes a replay from position 4 clean, and positionless/artificial/heartbeat events are ignored.
• TestBinlogClientLogPosWraparoundGuard — drives the real readStream through an injected streamer: pre-wrap event buffers, post-wrap event reports FatalReasonLogPosWrapped, nothing past the wrap is buffered, and the position does not advance into wrapped space.
• TestBinlogClientReplayIsNotAWraparound — the false-positive guard: a high-water mark, an artificial rotate back to the same file, replayed low positions, then a genuinely new event. Must buffer and must not report a wrap.
• TestBinlogProcessRowsEventUnknownSubtype / TestGTIDProcessRowsEventUnknownSubtype — a synthetic PARTIAL_UPDATE_ROWS_EVENT returns an error matching errUnsupportedRowsEvent and naming the type and table, buffers nothing; a recognized subtype still buffers; unsubscribed tables stay ignored.
• TestFatalReasonForStreamError — pins the error-to-reason mapping.

Each new test was verified red against the pre-fix behavior by fault injection (guards reverted to their previous form, go build confirmed, sources restored and cmp-verified byte-identical afterwards).

go build, gofmt, go vet, golangci-lint run ./pkg/change/... ./pkg/migration/... ./pkg/move/... (0 issues). Full ./pkg/change suite against MySQL 8.0.45, plus the new tests under -race -count=3, and the Fatal|XA|Checkpoint subsets of ./pkg/migration ./pkg/move ./pkg/datasync.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Checkpoint invalidation and zero-position wraparound handling have unresolved review findings.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Invalidate datasync checkpoint after log position wrap · New
• Detect real zero-position events after log position wrap · New

What changed in this PR

Hardens binlog handling against LogPos wraparound and unsupported row-event subtypes to prevent silent data loss.

Changes:

• Detects and rejects wrapped 32-bit binlog positions.
• Fails on unsupported subscribed-table row-event subtypes.
• Adds fatal-reason handling and focused tests.

File	Reviewed changes
pkg/​move/​runner.go	Invalidates checkpoints on wrapped positions.
pkg/​migration/​runner.go	Invalidates checkpoints on wrapped positions.
pkg/​change/​utils.go	Adds position tracking and fatal-reason mapping.
pkg/​change/​utils_test.go	Tests position tracking and reason mapping.
pkg/​change/​gtid.go	Rejects unsupported row-event subtypes.
pkg/​change/​gtid_test.go	Tests GTID subtype handling.
pkg/​change/​event_type.go	Adds unsupported-event error handling.
pkg/​change/​config.go	Defines the wrapped-position fatal reason.
pkg/​change/​binlog.go	Detects position wraparound and rejects unknown subtypes.
pkg/​change/​binlog_test.go	Tests wraparound and subtype handling.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[aparajon]

🤖 Adversarial correctness review of 8f031d1. I ran pkg/change (full), pkg/move, pkg/datasync and pkg/migration against MySQL 8.0 locally, and ran thirteen mutations of the new guards.

0 blocking, 1 non-blocking.

The LogPos tracker holds up. Each of these mutations is caught by a test in this PR:

• dropping rotated() on a rotate
• < → <=
• dropping the artificial-flag exclusion
• dropping the heartbeat exclusion
• dropping the LogPos == 0 handling
• dropping the errLogPosWrapped mapping
• reporting the wrap as FatalReasonStreamError
• the four variants of the new zero-offset branch in 8f031d1 (always false, always true, dropping TransactionPayloadEvent, treating every zero as row-bearing)

Only the outer header of a compressed payload feeds the tracker (binlog.go:840-848), so inner-event offsets can't trip it.

Removing the eventTypeUnknown check at the top of either processRowsEvent survives, but that is expected: the loop's default: returns the same wrapped error, which is what its "Unreachable today" comment says.

Non-blocking

1. An unsupported rows event keeps the checkpoint and tells the operator to resume, and a resume replays the same event. Both rows paths hardcode the checkpoint-preserving reason instead of going through fatalReasonForStreamError: binlog.go:802 and gtid.go:687. So a PARTIAL_UPDATE_ROWS_EVENT reaches the runner as FatalReasonStreamError, which logs "the checkpoint has been preserved — re-run spirit to resume" (migration/runner.go:1377, move/runner.go:1536).

The checkpoint can't be past that event, since the event was never applied. Resetting binlog_row_value_options doesn't remove it from the binlog. Every resume therefore streams back into it and fails the same way, and while the option is still set, preflight refuses first. Either way the advice never leads anywhere. The only exit is dropping the checkpoint table by hand, and nothing says so.

That is the same reasoning the runner comment gives for invalidating on XA and a wrapped LogPos, "resuming from it streams straight back into the condition that killed the run". The new datasync doc at datasync/runner.go:1523 makes the same point for those two reasons.

The test's own comment chooses this deliberately (binlog_test.go:1854: "so the caller keeps its checkpoint"). I think it wants the other answer.

The smallest fix I tried:

• route both rows paths through fatalReasonForStreamError(err);
• map errUnsupportedRowsEvent to an invalidating reason (its own FatalReason, or one of the existing ones), with its own runner message: reset binlog_row_value_options and start fresh.

Test that fails on 8f031d1 and passes with that fix

// TestBinlogClientUnsupportedRowsEventInvalidatesCheckpoint drives readStream
// into a PARTIAL_UPDATE_ROWS_EVENT for a subscribed table. The checkpoint
// sits before that event, and the binlog still holds it, so a resume
// replays it and dies the same way: the caller must be told to invalidate,
// not to resume.
func TestBinlogClientUnsupportedRowsEventInvalidatesCheckpoint(t *testing.T) {
	client, streamer, gotReason := newWraparoundTestClient(t, "unsupfatalt1", "unsupfatalt2")
	defer client.Close()

	ctx, cancel := context.WithCancel(t.Context())
	client.cancelFunc = cancel
	client.streamWG.Add(1)
	go client.readStream(ctx)

	require.NoError(t, streamer.AddEventToStreamer(rotateTo("binlog.000042", 4)))
	ev, _ := mkRowsEventForTest(replication.PARTIAL_UPDATE_ROWS_EVENT, 1000, "test", "unsupfatalt1", 1)
	require.NoError(t, streamer.AddEventToStreamer(ev))

	require.Eventually(t, func() bool { return gotReason.Load() != -1 },
		5*time.Second, 5*time.Millisecond, "an unsupported rows event must be fatal")
	client.streamWG.Wait()
	require.NotEqual(t, int64(FatalReasonStreamError), gotReason.Load(),
		"StreamError tells the runner to keep the checkpoint and resume, which replays this same event")
}

Build	Test above	TestBinlogProcessRowsEventUnknownSubtype (PR)
8f031d1	--- FAIL: Should not be: 1 (FatalReasonStreamError)	PASS
both rows paths via fatalReasonForStreamError, errUnsupportedRowsEvent mapped to an invalidating reason	PASS	--- FAIL at :1854, as expected, since it pins the current choice

The GTID path at gtid.go:687 has the same shape. I didn't write a separate GTID test.

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

[aparajon]

🤖 Stamped: 0 blocking, 1 non-blocking. See the review comment above.

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