fix(#746): KeyAboveHighWatermark must not silently drop in IsNil state

[morgo]

Fixes #746.

Root cause

chunkerOptimistic.KeyAboveHighWatermark (and chunkerComposite.KeyAboveHighWatermark) returned true when the chunker hadn't dispatched any chunks yet (chunkPtr.IsNil() && checkpointHighPtr.IsNil()), with the comment "every key is above because we haven't started copying." This caused deltaMap.HasChanged and bufferedMap.HasChanged to silently drop every binlog event during the window between runner.go's SetWatermarkOptimization(true) (filter on, just before copier startup) and the first chunker.Next() (chunkPtr set).

The intent was: "the chunker's later SELECT will pick the row up from source." That's only true for rows committed before the SELECT's snapshot. A writer that commits AFTER the snapshot but BEFORE chunkPtr is updated is in a dead zone — invisible to the SELECT (too late) AND silently dropped by the applier (KeyAboveHighWatermark returns true). The row is permanently lost.

Likelihood in practical workloads

For a real workload to lose a row, all of the following must coincide:

1. The migration must run while the source table is actively receiving writes — idle or low-traffic migrations are not affected.
2. A writer must commit during a window of a few milliseconds between SetWatermarkOptimization(true) and the first chunker.Next() advancing chunkPtr. This window stretches under CPU/I/O pressure (Docker, race detector, noisy neighbours).
3. The committed row must be one whose chunk has not yet been read by the copier's snapshot. (Rows committed before the SELECT's snapshot are picked up by the copier directly and aren't affected.)
4. The downstream checksum (FixDifferences=true, default in production) must fail to repair the divergence. Checksum re-checks every chunk and re-copies any whose CRC doesn't match — so the bug is masked the vast majority of the time.

Empirically this matched the failure rate observed in CI: across 99 failed docker-compose runs from 2026-04-30/05-01, only 2 hits on TestCutoverAtomicityWithConcurrentWrites (the test purpose-built to provoke this). And before PR #764 disabled FixDifferences for that test, even those two hits sometimes recovered after the checksum repair — the original #746 report was a 2-row deficit, not all four PKs in the dead zone.

In a production OLTP migration the practical risk is small but not zero: enough sustained write traffic during setup gives a non-zero chance of one or more silent row losses. The mode is silent data loss (rows in source missing from target after cutover), so it warrants a fix at any frequency. Most existing users on busy schemas have probably not been bitten, but anyone who has should re-checksum source vs the migrated table.

This is what makes a v0.12.1 cherry-pick the right call.

Why it took so long to pin down

In production, FixDifferences=true re-copies any chunk whose CRC doesn't match, so this bug was almost always masked. The rare flake of TestCutoverAtomicityWithConcurrentWrites was the slice that escaped repair.

PR #764 disabled FixDifferences for that test specifically so copy-phase loss would surface at the checksum step instead. After that landed, the failure became deterministic across #801, #797, #795, #804 with a consistent signature: a contiguous run of 3-5 PKs missing from chunk [1, 1001) (pk=62-65; pk=102-105; etc.) — exactly the shape of several writer transactions committing during the dead zone, autoinc funneling them through in PK order, all silently dropped.

What this PR does

1. Optimistic chunker (chunker_optimistic.go): the IsNil branch now returns false (buffer the change) instead of true (drop). Per the method's own contract: "if there is any ambiguity, it's important to return FALSE."

2. Composite chunker (chunker_composite.go): same fix in two places. The len(chunkPtrs) == 0 && checkpointHighPtr.IsNil() branch and the len(chunkPtrs) == 0 && key > checkpointHighPtr resume branch — both now return false for the same reason.

3. Test coverage extended to 4 variants. TestCutoverAtomicityWithConcurrentWrites previously only exercised the optimistic chunker in unbuffered copier mode. The cutover-atomicity probe has proven its value as the canonical detector for issue-flaky test: TestCutoverAtomicityWithConcurrentWrites #746-shaped bugs, and both the composite chunker and the buffered copier path have the same dead-zone window between SetWatermarkOptimization(true) and the chunker's first Next(). The test body is now extracted into runCutoverAtomicityTest(t, tableName, schemaTmpl, buffered) and invoked four times via t.Run:

   variant	chunker	copier mode
   optimistic_unbuffered	optimistic (single-col auto_inc PK)	unbuffered
   optimistic_buffered	optimistic	buffered
   composite_unbuffered	composite (forced via (id, x_token) PK)	unbuffered
   composite_buffered	composite	buffered

   The composite variants use a PRIMARY KEY (id, x_token) so NewChunker selects the composite chunker (which only handles single-column auto_increment otherwise). Buffered variants opt in via WithBuffered(true) and skip on the minimal-RBR runner since buffered requires binlog_row_image=FULL. All four subtests run in parallel under t.Parallel() with unique t1concurrent_* table names so they don't collide on _<name>_old / _<name>_new artifacts.

4. Test updates for the four tests that encoded the buggy behavior:

   • TestReplClientComplex — previously expected GetDeltaLen() == 0 after a DELETE issued before the chunker's first Next(). Now expects the deletes to accumulate in the delta map and be drained when feedback advances the watermark. End-state row count assertion unchanged.
   • TestOptimisticChunkerBasic — flipped KeyAboveHighWatermark(1) assertion from TRUE to FALSE for the pre-Next state.
   • TestCompositeChunkerWatermarkOptimizations — same flip for both pre-Open and pre-Next states.
   • TestCompositeChunkerCheckpointHighPtr — same flip for the pre-OpenAtWatermark state.

5. Reverts two prior workarounds that the root-cause fix makes unnecessary:

   • require.Eventually(5s, 250ms, ...) in the cutover atomicity test. The wrapper was added under the theory that "Docker MySQL has variable I/O latency that delays final event delivery" (commits 9ab22fe, f0f44fb), but a 1-row deficit cannot be masked by 5 s of polling once cutover has completed — it stays a deficit. Replaced with a strict single-shot require.Equal.
   • The second FlushUnderTableLock in executeRenameUnderLock (commit 3001db2). Each call already does flush → BlockWait → flush internally, so the flush's own binlog events (REPLACE INTO _new / DELETE FROM _new) are read back in the same call's BlockWait. The doubled call was a speculative hedge for the same divergence the IsNil bug actually caused. A single call is sufficient.

Verified locally

• go test -count=5 -run TestCutoverAtomicityWithConcurrentWrites ./pkg/migration/ — all 4 variants × 5 iterations PASS.
• A single run produces:

  --- PASS: TestCutoverAtomicityWithConcurrentWrites/composite_unbuffered (2.86s)
  --- PASS: TestCutoverAtomicityWithConcurrentWrites/optimistic_buffered  (3.04s)
  --- PASS: TestCutoverAtomicityWithConcurrentWrites/optimistic_unbuffered (3.21s)
  --- PASS: TestCutoverAtomicityWithConcurrentWrites/composite_buffered   (3.22s)
  

• Full pkg/migration suite — PASS (~3 min).
• Full pkg/repl, pkg/copier, pkg/table suites — PASS.

Cherry-pick

Recommend cherry-picking the chunker fixes (commit 045117c for optimistic + the chunker portion of the composite-and-reverts commit) to v0.12.1. The two reverts (require.Eventually wrapper, second FlushUnderTableLock) are cleanups and don't strictly need to ride along. The 4-variant test extension is independent and not needed in the patch release — but harmless if cherry-picking the whole branch is simpler.

Refs

• Closes flaky test: TestCutoverAtomicityWithConcurrentWrites #746
• Surfaced in CI on test(#784): migrate pkg/dbconn tests to require.* #801, test(#790): migrate pkg/applier tests to require.* #797, test(#783): migrate pkg/lint tests to require.* #795, test(#752,#792): unique table names + require.* in pkg/repl #804 (all unrelated PRs that just happened to inherit the now-strict test from main).

🤖 Generated with Claude Code