feat(status): expose row-copy progress as a structured field on Progress

[aparajon]

Why

status.Progress already carries the ETA (ETA), the checksum counters (Checksum), the throttle state, and per-table row counts, but the runner-wide row-copy progress only reached callers inside Summary, as text: 1031251/16370180 6.30% copyRows ETA 5m. A wrapper that wanted those two numbers had to parse the string back out, which is the one thing Summary was never meant for. The numeric type already exists (status.CopyProgress is what the copier's CopyProgress() returns for the periodic status block); it just was not on Progress.

What

• Adds Copy status.CopyProgress to status.Progress, next to Checksum. It is the sum of Tables, so the two reconcile by construction: both count settled rows against the tables' cardinality estimates. It is populated as soon as the copy chunker exists and keeps its final reading through the later phases, so a caller can read how much the run copied at any point. RowsTotal is an estimate, so RowsCopied can exceed it, exactly as it already can per table.
• It is deliberately not the copier's own CopyProgress(). On the chunker Spirit selects for a single auto_increment key, that measures keyspace distance against the auto_increment max rather than rows, and summing it across a multi-table run adds an id to a row count. A MySQL-backed test on that path (TestProgressCopyReconcilesWithTablesOnAutoIncrementKey) pins Copy against Tables on a table whose ids are sparse, where the copier's own measure differs by orders of magnitude.
• The periodic status block's copier row is derived from the same chunker snapshot, so the log line and the API report one measure on the same tick. Each runner reads the chunker once per call through a small copyTables helper (migrate, move) or its existing snapshot (sync).
• The migrate, move, and sync runners render Summary from that same reading and from a single GetETAState() call, so the copy fraction, the ETA text, and the ETA field describe one instant and a poll takes the copier lock once instead of three times. status.ETA gains a String() for this, and the copier's GetETA() now delegates to it.
• Behavior change in Summary and the status log block: on an auto_increment key the copy fraction now reports rows, the same numbers as Tables, instead of keyspace distance. On a table whose ids are sparse after years of deletes, the old text could read 0.50% with half the rows copied. The ETA is unchanged: it is still derived from the copier's keyspace pacing, which is the right basis for time remaining.
• Two caveats are documented on the field rather than hidden: RowsCopied counts rows settled by this run, so like the per-table counts it can exclude work from before a resume, and the ETA (including DUE) stays paced on the keyspace, so the two halves of Summary can disagree on how close a copy over a sparse key range is. The status and copier READMEs and the migrate guide are updated to match.
• A shared copiertest.Stub replaces the three per-package copier stubs in the runner tests. It lives beside the copier rather than in testutils because the copier's own tests import testutils, so a stub there would be an import cycle.

With this, every value that appears in Summary has a typed counterpart on Progress, so a consumer never has a reason to parse it.

The copier's GetProgress() string is no longer called anywhere in Spirit outside its own implementation. It is left in place here because it is part of the exported copier.Copier interface; retiring it is a separate, breaking change.

Opened by Claude (Fable 5).

[morgo]

🤖 Automated second-pass review on Morgan's behalf. CI is green at 8f5b9a28 and the mechanics are clean — all three Progress() producers are updated, every status.Progress literal in the tree is keyed so the new field breaks no construction, and GetProgress() is literally CopyProgress().String() (buffered.go:630), so Summary is byte-identical before and after. The stated goal is right, too: a consumer should never have to parse Summary.

Not approving yet, for one reason. The numbers being promoted are not row counts on Spirit's most common code path, and the new doc comment says they are.

table.NewChunker picks chunkerOptimistic for any table with a single auto_increment key (pkg/table/chunker.go:168) — the default for most production MySQL tables. That chunker's Progress() returns:

• numerator t.rowsCopied, declared at chunker_optimistic.go:70 as // The sum of chunkSize: distance travelled, not a row count, and a different field from actualRowsCopied, which is what RowsCopied() returns
• denominator maxValue — the auto_increment max value, not a row estimate

buffered.CopyProgress() passes both straight into status.CopyProgress{RowsCopied, RowsTotal}. Meanwhile Progress.Tables[] is built from CopyRowCounts(), which returns settled rows and Ti.EstimatedRows. So on a table with 1M live rows whose IDs are sparse to 100M, one snapshot reports Copy: 500000/100000000 (0.50%) and Tables[0]: 500000/1000000 (50%) — same struct, same field names, two orders of magnitude apart.

pkg/table/row_counts.go:8-9 already warns about precisely this: "Progress instead measures keyspace distance for optimistic chunkers, so its numerator and denominator must not be presented as literal row counts."

That warning was survivable while these numbers only existed inside a human-readable Summary. Naming them RowsCopied/RowsTotal on the public Progress struct, next to Tables[], is what invites a consumer to do arithmetic on them and to reconcile the two — which they can't. Two ways out, either fine by me:

1. source Copy from CopyRowCounts semantics so it reconciles with Tables[], or
2. keep the current source and name it for what it is (Position/Extent, or keep CopyProgress but document the provenance honestly).

Why this survived review: the only test pinning Copy next to Tables is TestE2EBinlogSubscribingCompositeKey, and a composite key routes to chunkerComposite — the one implementation where Progress() and RowsCopied() read the same field, so they cannot disagree. It asserts {1000,1200} for both and passes. The divergent default path has no coverage at all.

The remaining notes are inline and none of them block: a multi-table unit-mixing consequence of the same root cause, Copy zeroing at the phase boundary while Tables keeps the totals, three copier-lock acquisitions per call, a duplicated test stub, and a %v-on-Stringer nit.

Worth saying: everything else about the change is the right shape. Reading the copier once and rendering Summary from that same reading is a real improvement — it removes a genuine disagreement window — and placing Copy beside Checksum with matching doc structure is consistent with the existing API. The problem is one level down in what the copier hands you, not in this PR's structure.

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1220, 97fd3af.

Verdict: 8 findings — 0 blocking, 5 non-blocking (a resume-path undercount, two runners whose derivation is entirely unpinned, and a docs sweep the PR skipped), 3 suggestions. The reconciliation itself is right, and pkg/migration genuinely proves it.

Non-blocking

1. On resume, Copy.RowsCopied restarts from zero while Tables does not. CopyRowCounts reads chunkerOptimistic.RowsCopied(), i.e. actualRowsCopied — and OpenAtWatermark re-seeds only rowsCopied (the keyspace counter the pre-PR numerator used), leaving actualRowsCopied at 0. So a resumed migration reports Copy counting only post-resume rows against a full RowsTotal. The composite chunker is immune (chunker_composite.go:301), Resume: true ships in the same snapshot, and TableProgress.RowsCopied already documents the caveat — so this is a known-shaped gap, but Copy is the new headline number and inherits it silently.

2. The operator log block still renders the measure this PR declares wrong — on the same tick. migration/runner.go:1989 feeds Status() from r.copier.CopyProgress() (keyspace) while Progress().Summary now renders settled rows, and status/task.go:48,53 calls both on the same 30s tick. On the sparse-auto_increment table progress_copy_test.go builds, the API says 500/501 99.80% while the log line prints copier 0.20% 2000/1000000. Same pattern at move/runner.go:1547 and datasync/runner.go:1672; no test compares the two, since every Status() assertion is substring-only.

3. move and datasync would not notice their Copy reverting to the keyspace position. Swapping move/runner.go:1995 and datasync/runner.go:1607 back to chunker.Progress() passes both full suites — their Copy assertions pin only RowsTotal, with RowsCopied always 0. Only pkg/migration has a test that distinguishes the two measures. The fix needs no database: MockChunker keeps rowsCopied (fed by Feedback) separate from currentPosition (fed by SimulateProgress), so one Feedback(nil, 0, 50) per test makes them diverge.

4. Multi-table RowsCopied summation — the point of CopyFromTables — is unpinned everywhere. Summing only tables[0] at status/tables.go:37 passes the full pkg/migration (62s), pkg/move, pkg/datasync and pkg/status suites. An 8-table atomic migration would report ~12% in Copy forever while Tables reads 100% — exactly the reconciliation this PR exists to establish, silently broken with CI green.

5. The PR touches zero .md files, and five doc sites now describe the old measure. pkg/status/README.md:89 omits Copy from the Progress field list; :72 and docs/migrate.md:672 describe the keyspace measure in row-count language; :131 says the ETA is cleared after CopyRows while Copy now deliberately persists past it; and pkg/copier/README.md:61,170 still recommends GetProgress(), which this PR leaves with zero production callers.

General suggestions

6. Summary mixes a row-based fraction with a keyspace-gated DUE. The percentage now comes from settled rows, but DUE is still gated on the keyspace percentage at copier.go:34, so the string can read near 100% for hours without ever reaching DUE — and conversely show 80% beside ETA DUE. This is disclosed, deliberate design (two verifiers refused to call it a bug), but the two halves of one line answering different questions is worth a sentence in the field doc.

7. Both newly exported symbols landed without a test in their own package. CopyFromTables (+13) and ETA.String() (+25) live in pkg/status, which already has DB-free tables_test.go and progress_test.go, and neither was extended — gutting either survives go test ./pkg/status/.... Since every go test in this repo runs inside the Docker+MySQL matrix, a contributor running pkg/status locally gets a false green. Relatedly, copiertest.Stub's new Copy/Chunk fields are set by datasync's test but read by no assertion.

8. progress_copy_test.go:68 pins the chunker's internals rather than the property. require.Equal(status.CopyProgress{RowsCopied: 2000, RowsTotal: 1000000}, ...) couples the only test that distinguishes the two measures to the optimistic chunker's default first chunk size and open-lower-bound behaviour. The load-bearing assertion is p.Copy.RowsTotal < 1000000 on line 62; a chunker tweak turns line 68 red, and the natural "fix" is to update the constant — quietly disarming the test.

The one thing that could have broken, verified

The DUE/TBD switch was lifted out of copier.GetETA() into status.ETA.String(), while GetETA() itself is still called at three sites to render the human log block — a silent behaviour change there would print eta=0s through an entire copy. It holds: the rewrite is behaviour-identical including the ETANone fallthrough and the locking, and gutting ETA.String() to e.Duration.String() is caught by two pkg/migration tests. Worth noting the full pkg/copier suite (15s) passes that mutation, so the guarantee rests entirely on the MySQL-matrix job.

Verified correct

• No test was weakened to hide a regression: binlog_test.go:165,180,211 now pin Copy by whole-struct equality, including a deliberate 1201/1200 100.08% over-100% case.
• The require.Empty(p.Copy) → require.Equal({RowsTotal: 100}) flips in all three runners are the intended behaviour change (the reading outlives the copy phase), not a loosening.
• No divide-by-zero or NaN: status.fraction() guards total == 0, so a zeroed RowsTotal renders 500/0 0.00%.
• The Progress() sweep is symmetric across migration, move and datasync — no runner was left on the old derivation.
• Deriving Copy from the copier's own measure instead of CopyFromTables is caught in both move and datasync, so the "not the copier's own measure" half of the contract is genuinely pinned.
• No new data race; -race green across pkg/status, pkg/copier, pkg/migration, pkg/move, pkg/datasync.

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

[aparajon]

🤖 Thanks, all eight taken in 8672498. The status block's copier row now comes from the same chunker snapshot as Progress, so the log and the API report one measure on the same tick. The move and datasync tests feed settled rows into the mock chunkers so a revert to the copier's measure fails, and the multi-table cases sum both counters. ETA.String and CopyFromTables have unit tests in pkg/status, and the auto_increment test asserts the property instead of the default chunk size. The docs sweep covers the field doc, both READMEs, and the migrate guide, including the resume caveat and the keyspace-paced DUE beside a row-based fraction.

On finding 1: the optimistic chunker has no persisted settled-row count to re-seed on resume, so I documented the gap alongside Resume rather than fabricate a number. Happy to take that as a follow-up.

Claude (Fable 5)

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1220, 515ae34.

Verdict: re-review of the delta 97fd3afe..515ae348 only — 8 findings, 0 blocking, 6 non-blocking, 2 suggestions. The switch from the copier's keyspace measure to the per-table settled counts is correct and now well covered in two of the three runners; the increment 86724981..515ae348 is a genuine flake fix, not a loosening.

Non-blocking

1. On an auto_increment resume the copier log row now restarts near 0% while eta= does not. RowsCopied() returns actualRowsCopied, which chunker_optimistic.go:692 zeroes in open() and OpenAtWatermark re-seeds only for t.rowsCopied (:371) — so post-PR the percentage and n/m restart from zero on a resumed run while the ETA still continues from the resumed keyspace position. This is log-only (nothing parses Status()) and the semantics are the pre-existing documented contract of Chunker.RowsCopied, so it is not a regression in behaviour — but it leaves one row of the block internally inconsistent on exactly the path the PR set out to make consistent. docs/migrate.md:672 carries no resume caveat and the log block has no Resume indicator though Progress does; composite chunkers are genuinely unaffected.

2. The move runner's share of the change is completely untested. Reverting pkg/move/runner.go:1539 back to r.copier.CopyProgress() — undoing the PR's entire purpose for that runner — leaves the whole pkg/move suite green, because move.Runner.Status() has zero test callers repo-wide. The same mutation dies immediately in migration (progress_copy_test.go:77) and datasync (progress_test.go:46); one require.Contains(t, r.Status(), "50/100") in TestMoveProgress closes it with no new fixture.

3. The new log-block assertions pin n/m but not the leading percentage. Sourcing the percentage from the copier while keeping the counts from the tables passes both new assertions; tightening datasync's to "77.78% 70/300 chunk-size=25 eta=1m0s" confirms that render is reachable — a keyspace-derived 77.78% printed next to 70/300 settled rows (23.33%). That is precisely the two-measures-in-one-row confusion this PR exists to remove, sitting three characters outside the assertion.

4. Chunker's godoc and the copier README still describe the old contract. pkg/copier/copier.go:68 still documents CopyProgress as the progress source; the README's interface block omits GetETAState, CopyProgress and ChunkSize while its Methods list documents all three, and its monitoring example calls CopyProgress() nine lines after warning against using it. After this PR CopyProgress() has no production callers inside spirit at all.

5. Datasync's cp != nil guard now gates figures that no longer come from the copier. pkg/datasync/runner.go:1671 reads the chunker for the copy figures but the surrounding nil-check is still on the copier, so a nil copier with a live chunker silently drops real progress. Reading the chunker under progMu is correct here — that is datasync's documented guard and it has no chunkerMu.

6. Datasync inlines the helper the other two runners extracted. Migration and move both grew copyTables(); datasync open-codes the same two lines. AGENTS.md's runner-triplet rule asks the three to stay shaped alike, and the asymmetry is what makes the cp != nil mismatch above easy to miss.

7. [increment] The resume test's denominator is now self-fulfilling. estimatedRows is read from the same TableInfo.EstimatedRows the production path prints via CopyRowCounts, so multiplying the estimate by 7 at its only write site (tableinfo.go:210) leaves TestCheckpoint green. The old literal was not a usable oracle either — see below — so this is a trade, not a regression; SELECT COUNT(*) FROM cpt1 would restore an independent bound. The stale comment at resume_test.go:95 claiming the fixture produces "exactly 11040 rows" should go.

General suggestions

8. Two test-strength nits in the increment. progress_copy_test.go:70 is now the only assertion on copier.CopyProgress() anywhere in the repo and bounds its numerator only from below — a 100× error survives; an exact require.EqualValues(t, 2000, own.RowsCopied) is deterministic here (2 chunks × the fixed 1000-row chunk size). Separately the require.Eventually at :212 computes wantCopier into a local, so a failure costs 10s and never prints the expected string — append it to the message.

The one thing that could have broken, verified

Redirecting the API's Copy figures from the copier to the summed per-table counts. Mutating each runner's Status() back to r.copier.CopyProgress() dies in migration and datasync; dropping the RowsCopied accumulation in CopyFromTables dies at five independent sites; dropping the chunkerMu guard in copyTables() dies in both runners under -race, which spirit's CI runs. Only move survives — finding 2.

Verified correct

• The increment 86724981..515ae348 is test-only and a real flake fix: TestCheckpoint at 86724981 fails unmodified against mysql:8.0.45 (rendered 0/10546, then 0/11092, against the hardcoded 0/11040), because the denominator is InnoDB's sampled information_schema.table_rows. With the increment it passed 8/8.
• The %6.2f width arithmetic in the new assertion reproduces production alignment at 1-, 2- and 3-digit percentages, including ≥100% (reachable, since RowsCopied can exceed RowsTotal).
• The settled-rows numerator is a genuinely independent oracle — it queries _cpt1_new directly, and doubling the production numerator inside CopyRowCounts fails the assertion.
• ETA.String()'s DUE short-circuit is properly pinned; the due_ignores_a_leftover_duration subtest is the only thing distinguishing it from a coincidentally-zero duration.
• The earlier claim that the composite-chunker ETA docs are now wrong is refuted — both doc sites are explicitly scoped to an auto_increment / sparse-id key.
• copiertest.Stub's move to Copy{7,9} is what makes three previously-surviving mutants die; the value is deliberately unmistakable against any plausible table sum.

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

[aparajon]

🤖 Thanks for the second pass. Addressed in ce54a2c, per finding:

1. Resume restarts the copier row near 0% while the ETA continues — documented, not changed. The checkpoint persists the copy position, not a settled-row count, so re-seeding actualRowsCopied on resume would need a new persisted value. The n/m row in docs/migrate.md now states the behaviour; happy to take the re-seed as a follow-up if you want it.
2. move.Runner.Status() had no test caller — copiertest.Stub now reports a throttler.Noop, and TestMoveProgress asserts the rendered copier row (50.00% 50/100 chunk-size=0 eta=1m0s throttled=false) and that the stub's own figures do not appear.
3. Percentage not pinned — the datasync and migration log-block assertions now include the leading percentage.
4. Stale CopyProgress godoc and README — the godoc now describes it as the chunker's Progress (keyspace distance on the optimistic chunker, the measure the ETA is paced on) and points row-reporting callers at status.CopyFromTables; the README interface block lists GetETAState, GetProgress, CopyProgress, ChunkSize, and the monitoring example sums the chunker's tables instead of calling CopyProgress().
5. Datasync guard on cp gates chunker-derived figures — the row now renders whenever the chunker is published; chunk-size and ETA read 0 / TBD until the copier exists. A new assertion covers the tick between the two publications.
6. Datasync inlines what migrate/move put in copyTables() — declined. Datasync snapshots every published field under one progMu acquisition; a copyTables() helper would take the lock a second time. Kept inline with the snapshot.
7. Resume denominator self-fulfilling — the test now bounds EstimatedRows against SELECT COUNT(*) FROM cpt1 (InEpsilon 0.2), and the fixture comment no longer claims an exact row count.
8. Literal expectations — the copier's own numerator is asserted as 2*m.copier.ChunkSize(), and the resume test's Eventually failure message includes the expected row.

Claude (Fable 5)

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1220, ce54a2c.

Verdict: re-review of the delta 515ae348..ce54a2c2 only — 5 findings, 0 blocking, 2 non-blocking, 3 suggestions. Seven of the eight findings from the previous round are fixed and independently mutation-verified; the remaining items are one leftover asymmetry and two small doc gaps.

Non-blocking

1. The API and the log block now disagree in the new no-copier window. datasync/runner.go:1675 substitutes ETAMeasuring (renders TBD) when the chunker is published but the copier is not, while Progress() in the same window returns a zero ETA — state ETANone, documented as "there is no copy ETA because the migration is not in the row-copy phase", though the phase is CopyRows. Confirmed empirically: on one tick Status() renders eta=TBD while Progress() returns ETA{State:"", Duration:0}. ETAMeasuring is the right choice for the log block (ETANone renders 0s, which would falsely read as "no time remaining"), so the fix belongs on the Progress() side.

2. The resume caveat landed one row too low. The new sentence is in the n/m row, but docs/migrate.md:672 — the % row directly above, which a reader hits first — still says the percentage "can drift slightly", the opposite of what a resumed auto_increment copy does. Neither row states the sharper consequence: on a resume n/m never reaches 100%, since n counts only post-restart rows against the whole-table estimate.

General suggestions

3. Datasync still inlines the helper the other two runners extracted. datasync/runner.go:1673 (and again at :1602/:1609) open-codes status.CopyFromTables(status.TablesFromChunker(chunker)) while migration and move both have copyTables(). This was the one previous finding not addressed, and AGENTS.md's runner-triplet rule asks for the port or a shared extraction.

4. The nil-copier guard was added to one runner only. Migration and move dereference r.copier unconditionally in their CopyRows branches. I found no reachable window in either (both publish the copier before setting CopyRows), so this is consistency rather than a bug — but it is the one place the delta's own reasoning, "a status tick can land with either missing", was applied to a single runner.

5. The README refresh is half-done in its own file. ChunkSize() uint64 was added to the interface block at README.md:46 but the Methods bullet list below it still has no ChunkSize entry, so a reader looking up what chunk-size= means falls through to the Go source.

The one thing that could have broken, verified

Whether the new assertions actually kill what survived last round. They do, in every runner: sourcing the percentage from the copier while keeping the counts from the tables now dies in migration, datasync and move; reverting move's Status() to r.copier.CopyProgress() dies at progress_test.go:57; inflating the copier's own numerator 100× dies at progress_copy_test.go:72; reverting datasync's guard to cp != nil and weakening the stand-in ETA to ETANone both die too. Five mutants that survived at 515ae348 are now killed.

Verified correct

• The 2*ChunkSize() pin is genuine, not tautological — ChunkSize() reads the sampled chunk size while RowsCopied comes from the chunker's Progress.
• require.InEpsilon(actualRows, estimatedRows, 0.2) has real headroom: across 28 runs against a fresh mysql:8.0.45 the estimate took four values (11092/10810/10546/10264) against a true count of 11010 — worst error 6.8%, so ~3× margin. Argument order and uint64 handling are correct.
• The old "exactly 11040 rows" comment was itself wrong (the seeds produce 11010), so replacing it was right.
• The estimate is not re-read mid-test: AutoUpdateStatistics only starts from Run(), which this hand-stepped test never calls.
• The caveat's scoping to auto_increment is correct — the composite chunker restores rowsCopied from the watermark, so it is genuinely unaffected.
• The README interface block now matches copier.go exactly, and the monitoring example no longer contradicts the warning above it.
• copiertest.Stub's new GetThrottler is what lets move's Status() render throttled=false without reaching the embedded nil interface.

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

[aparajon]

🤖 Thanks for the third pass. Addressed in 1fc647e, per finding:

1. API and log block disagree in the no-copier window — fixed on the Progress() side as suggested. Datasync Progress() now reports ETAMeasuring and a Summary of <n/m pct%> copyRows ETA TBD when the chunker is published but the copier is not, matching Status(). The test asserts both in that window. The comment on the guard also no longer says a tick "can land with either missing": all three runners enter CopyRows through status.Do(CopyRows, r.copier.Run), so the copier exists whenever the state is CopyRows, and the guards are defensive.
2. Resume caveat in the wrong row — moved to the % row, which no longer says the percentage "can drift slightly". It now states that after a resume on an auto_increment key the count restarts at the rows settled since the restart against the whole-table estimate, so % restarts low and finishes short of 100% while eta continues.
3. Datasync inlines what migrate/move extracted — declined again, with a sharper reason. The shared extraction the runner-triplet rule asks for already exists and all three runners use it: status.TablesFromChunker and status.CopyFromTables in pkg/status. The runner-local copyTables() in migrate and move exists only to own chunkerMu around the chunker read; datasync has no lock to own there because the chunker is read as part of the single progMu snapshot that also takes the copier, replication client, and applier. A datasync copyTables() would be a wrapper around two shared calls with nothing of its own.
4. Nil-copier guard added to one runner only — declined. As in 1, none of the three runners can be in CopyRows without a copier, since entering the state dereferences it. Datasync's guards predate this PR; porting a guard against an unreachable state to migrate and move would add two branches nothing can take.
5. README ChunkSize bullet missing — added, describing it as the chunk-size= field of the status block and why it moves during a copy.

Claude (Fable 5)