Skip to content

fix(github): render row-copy progress percentages at their true precision - #1239

Merged
aparajon merged 4 commits into
mainfrom
armand/subpercent-progress
Sep 1, 2026
Merged

fix(github): render row-copy progress percentages at their true precision#1239
aparajon merged 4 commits into
mainfrom
armand/subpercent-progress

Conversation

@aparajon

@aparajon aparajon commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Why this matters

Progress percents render as rounded whole numbers, and a copy that has begun but not yet reached 1% rounds up to 1% — an operator watching 13M of 43B rows copied sees "1%" when the engine reports 0.03%, a 30x overstatement, sitting next to a multi-day ETA that contradicts it. Above 1% the rounding hides movement instead: on a huge table consecutive refreshes read the same whole number for hours even though the copy is advancing. Operators make stop/keep-waiting decisions from this line; it should show the figure the engine actually measured.

What it does

Row counts become the source of truth for every textual percent. Wherever a render site has the copied/total counts — and every row-copy site does — ui.FormatRowCopyPercent recomputes the percent from them and renders two decimals (45.37%, 0.03%), so the displayed figure always agrees with the row counts beside it. The engine's whole-number percent is only a fallback for the rare site without counts. Progress bars and threshold comparisons keep their integer widths, so a started copy still lights its first bar segment.

Two display invariants worth calling out:

  • Never reads as not started. The fraction is floored at 0.01%: once rows have copied, the text can't show 0.00% and masquerade as an idle copy.
  • Never reads as finished early. The fraction is capped at 99.99% while copied rows still trail the total: a copy in its final stretch can't show 100.00% beside a live ETA and masquerade as done.
  • Progress comparisons agree with the display. ui.RowCopyFraction gives renderers the same row-derived value for ordering, so slowest-shard and least-progressed-table selection can tell two sub-1% copies apart instead of comparing rounded integers that read equal.

Covers the PR comment (running summary, table lines, checksum progress, shard summaries, recovering, stopped/failed labels) and the CLI (progress view, shard lines, watch TUI, the apply command's streaming log lines). The webhook shard summary gains row counts — the storage task already carries them — so per-shard percents get the same precision. Also swaps a literal failed glyph in the sharded status labels for glyph.Failed, which the severityglyphs lint requires.

How it moves us toward the northstar

The progress comment is the operator's window into a change that can run for days — its authority depends on showing what the engine measured, not a rounding of it. A percent that agrees with the row counts at every magnitude keeps that window trustworthy exactly when the stakes are highest: the biggest tables, where a rounded percent is off by a day of wall-clock.

PR comment — sub-1% copy on a 43B-row table

Schema Change Status — Production

Database: boardgames | Apply ID: apply-7aa13cf03496454b

Applied by @operator at 2026-09-01 15:14:06 UTC

Status: In Progress

events: 🟦⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜ 0.03%

  • Rows: 13,186,540 / 43,234,523,345 · ETA: 2d 5h
PR comment — mid-copy multi-table apply (TEMPLATES.md preview)

📊 1/3 complete · 1 running (62.38%) · 1 queued

Schema testapp

users: 🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦⬜⬜⬜⬜⬜⬜⬜⬜ 62.38%

ALTER TABLE `users` ADD INDEX `idx_email`(`email`);
  • Rows: 914,707 / 1,466,232 · ETA: 3m 15s

products: ⏳ Queued

ALTER TABLE `products` ADD INDEX `idx_price`(`price_cents`);

orders: 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Complete

CLI progress view — sub-1% copy, same table as above
     ~ events: 🟦⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜⬜ 0.03%
       • Rows: 13,186,540 / 43,234,523,345 · ETA: 2d 5h

Opened by Claude (Fable 5).

Copilot AI lite review requested due to automatic review settings September 1, 2026 15:15

Copilot AI left a comment

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.

Pull request overview

Improves operator-facing progress reporting for long-running schema changes by rendering sub-1% row-copy progress as an accurate fractional percentage (e.g., 0.03%) instead of rounding up to 1%, keeping early progress and ETA consistent across the GitHub PR comment and CLI views.

Changes:

  • Add ui.FormatRowCopyPercent and update templates/CLI rendering paths to use it for textual row-copy (and related) percent displays while keeping progress-bar thresholds on whole-number display percent.
  • Update PR comment and CLI progress templates to print fractional percents for sub-1% progress (including shard lines and checksum progress in the CLI).
  • Refresh/rename affected tests and swap a literal failed glyph for glyph.Failed to satisfy lint expectations.

Reviewed changes

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

Show a summary per file
File Description
pkg/webhook/templates/sharded_apply.go Use glyph.Failed for failed status phrase to satisfy lint and keep glyphs consistent.
pkg/webhook/templates/apply.go Route row-copy percent text through ui.FormatRowCopyPercent across multiple PR-comment render paths (running/recovering/stopped/failed summaries).
pkg/webhook/templates/apply_test.go Update PR-comment assertions to expect fractional sub-1% output and rename the test accordingly.
pkg/ui/format.go Introduce FormatRowCopyPercent and clarify RowCopyDisplayPercent’s role (bars/thresholds vs. textual percent).
pkg/ui/format_test.go Add unit tests for FormatRowCopyPercent covering whole percents, sub-1% fractions, clamps, and missing totals.
pkg/cmd/internal/templates/progress.go Use FormatRowCopyPercent in CLI progress output for row copy, checksum verification, stopped/cancelled labels, and parsed engine progress.
pkg/cmd/internal/templates/progress_states_test.go Update CLI progress-state tests to expect fractional sub-1% output (including checksum “just started”).
pkg/cmd/internal/templates/progress_shard.go Render shard copy percent via FormatRowCopyPercent using shard row counts for sub-1% accuracy.
pkg/cmd/internal/templates/progress_shard_test.go Update shard progress tests to assert fractional sub-1% output.

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

Comment thread pkg/webhook/templates/apply.go Outdated
@aparajon aparajon changed the title fix(github): show sub-1% row-copy progress as its true fraction fix(github): render row-copy progress percentages at their true precision Sep 1, 2026
@aparajon
aparajon marked this pull request as ready for review September 1, 2026 15:44
aparajon and others added 3 commits September 1, 2026 11:46
A copy that has begun but not yet reached 1% rendered as a rounded-up 1%,
so early progress on a huge table (13M of 43B rows = 0.03%) read as 30x
the real figure. The engine's whole-number percent has already lost the
fraction, but every render site also carries the row counts, so the new
ui.FormatRowCopyPercent recomputes the fraction from rows and renders two
decimals ("0.03%"), clamped away from 0.00% (copying has begun) and 1.00%
(the whole-number percent still says 0). Progress bars and threshold
comparisons keep the integer bump so a started copy still lights its
first segment.

Applies to the PR comment (running summary, table lines, recovering,
stopped/failed labels) and the CLI progress view (table lines, shard
lines, checksum progress).

Also swaps a literal failed glyph in the sharded status labels for
glyph.Failed, which the severityglyphs analyzer requires.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Row counts become the source of truth for all textual percents, not just
the sub-1% case: whenever a render site has the copied/total counts, the
percent is recomputed from them and shown with two decimals ("45.37%",
"0.03%"), so the displayed figure always agrees with the rows beside it.
The engine's whole-number percent is only a fallback for sites without
row counts. RowCopyFraction gives renderers the same value for
comparisons, so slowest-shard and least-progressed-table selection can
now tell two sub-1% copies apart instead of comparing bumped integers.

The webhook shard summary gains row counts (the storage task already
carries them), so per-shard and slowest-shard percents get the same
precision, as do the CLI apply command's streaming progress lines.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aparajon
aparajon force-pushed the armand/subpercent-progress branch from 6d87cee to cd48fc3 Compare September 1, 2026 15:49

@morgo morgo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤖 Approving on Morgan's behalf (automated review).

Checked the helper split at cd48fc36 across every render site rather than taking the description's word for it, and the discipline holds: RowCopyDisplayPercent (integer) drives bars and threshold switches, FormatRowCopyPercent drives text, RowCopyFraction drives ordering. All three are applied consistently in webhook/templates/apply.go, cmd/internal/templates/progress.go, and watch_tui_view.go — no site mixes a rounded integer into a comparison. The Checksumming percent at apply.go:783 divides by ChecksumRowsTotal, but it's inside if table.ChecksumRowsTotal > 0, so no div-by-zero. RowCopyFraction clamps overshoot to 100, which matches the existing ClampRows/EstimateExceeded convention for Spirit reporting copied > estimated total.

Reproduced the headline claim — 13,186,540 / 43,234,523,345 renders 0.03%, exactly as advertised. CI green, display-only, off the MySQL data path.

One finding: the floor invariant has no ceiling counterpart, so an unfinished copy can read 100.00%.

FormatRowCopyPercent floors at 0.01 so a started copy never reads as idle, but nothing caps the top:

frac := math.Max(RowCopyFraction(pct, rowsCopied, rowsTotal), 0.01)
return fmt.Sprintf("%.2f%%", frac)

Anything from ~99.995% up rounds to 100.00%. Ran it:

999999999/1000000000       -> 100.00%   (exact 99.99999990)
99999/100000               -> 100.00%   (exact 99.99900000)
43234523344/43234523345    -> 100.00%   (exact 100.00000000)
9999/10000                 ->  99.99%   (exact 99.99000000)

On this PR's own 43B-row example that's the last ~2.16M rows — the copy displays 100.00% next to a live ETA for a stretch that, at the rate implied by the example, is not brief. The old integer render showed 99% there, so this is a narrow regression in the direction that reads worst: "100%" reads as done, and an operator deciding whether to keep waiting is exactly who this PR is for.

The symmetric guard is expressible from the counts the function already has:

if rowsCopied < rowsTotal {
    frac = math.Min(frac, 99.99)
}

That preserves "never reads as not started" and adds "never reads as finished before it is." Worth a follow-up, not a revision.

Coordination note: the sharded_apply.go literal-glyph → glyph.Failed one-liner in here is the third copy of the same drive-by — #910 and #1240 both carry it too. Whichever two land second will need that hunk dropped.

The two-decimal render floors at 0.01% so a started copy never reads
as idle, but had no ceiling: from ~99.995% an unfinished copy rounded
to 100.00% beside a live ETA, and an operator deciding whether to keep
waiting takes 100.00% as done. Cap the display at 99.99% while copied
rows still trail the total, the symmetric bound derived from the same
counts. Ordering by RowCopyFraction keeps the true fraction.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aparajon

aparajon commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator Author

🤖 Addressing morgo's review — the missing ceiling was a real gap, and you're right that it's strictly worse than the integer rendering it replaced: the old code could never say a copy was done early, and the new precision made that possible from ~99.995%.

Fixed in this PR rather than a follow-up: 2d0607e adds your exact guard — FormatRowCopyPercent now caps at 99.99% while rowsCopied < rowsTotal — and the doc comment states both bounds as one invariant: the rendering never reads as not-started and never reads as finished while the counts say otherwise. Your three probe values are pinned as tests, including the 43B-row pair from the PR body.

Deliberately not capped: RowCopyFraction still returns the true fraction. It feeds ordering (slowest-shard, least-progressed-table), where clamping two near-done copies to the same value would make the selection arbitrary exactly when it matters. Only the display caps.

On the glyph drive-by triplicated with #910/#1240: agreed, no action — it's byte-identical in all three, so whichever land later rebase the duplicate hunk away cleanly.

Reply generated by Claude Code (Claude Fable 5).

@aparajon
aparajon merged commit baf81da into main Sep 1, 2026
38 checks passed
@aparajon
aparajon deleted the armand/subpercent-progress branch September 1, 2026 17:32
Kiran01bm added a commit that referenced this pull request Sep 1, 2026
…dcolumn-ddl-seam

* origin/main: (28 commits)
  docs: document the PostgreSQL support envelope (#1144)
  fix(engine): report why a Vitess schema change failed (#1242)
  feat(ddl): detect statements whose cost scales with table size (#1237)
  fix(operator): keep a multi-table apply running while tables are queued behind a cutover (#1241)
  fix(storage): index the webhook inbox claim ordering (#1196)
  fix(github): drop the cutover duration promise from progress surfaces (#1240)
  fix(github): render row-copy progress percentages at their true precision (#1239)
  fix(observability): do not report a shutdown as a claim failure (#1233)
  fix(github): tell an operator why a refused apply's database is busy (#1224)
  fix(engine): do not mark an apply failed when its driver shuts down (#1234)
  feat(github): render live row-copy progress on sharded table lines (#1191)
  feat(ui): add approximate row and byte formatters (#1236)
  fix(planetscale): delete the branch an apply created when it fails before its deploy request (#963)
  feat(api): app grouping field on database config (#1226)
  feat(cli): filter pulled tables with --table (#1235)
  fix(github): refuse a Vitess foreign key at plan time instead of at apply time (#966)
  feat(lint): add severityglyphs analyzer to keep the severity vocabulary in pkg/glyph (#1153)
  feat: remove the volume control operation end to end in favor of autoscaling (#1225)
  ci: give the k8s e2e job budget room for setup plus go test's timeout (#1232)
  fix(storage): canonicalize lock and check identity keys (#1216)
  ...

# Conflicts:
#	pkg/ddl/parser.go
#	pkg/ddl/parser_test.go
Kiran01bm added a commit that referenced this pull request Sep 2, 2026
…ditive-convergence

* origin/main: (33 commits)
  feat(postgres): add ADD COLUMN synthesis to the statement parser seam (#1212)
  feat(cli): add storage canonicalize-identity-keys admin subcommand (#1231)
  fix(storage): canonicalize remaining identity keys (#1218)
  fix(storage): canonicalize apply and task identity keys (#1217)
  fix(webhook): canonicalize repository identity at ingress (#1213)
  docs: document the PostgreSQL support envelope (#1144)
  fix(engine): report why a Vitess schema change failed (#1242)
  feat(ddl): detect statements whose cost scales with table size (#1237)
  fix(operator): keep a multi-table apply running while tables are queued behind a cutover (#1241)
  fix(storage): index the webhook inbox claim ordering (#1196)
  fix(github): drop the cutover duration promise from progress surfaces (#1240)
  fix(github): render row-copy progress percentages at their true precision (#1239)
  fix(observability): do not report a shutdown as a claim failure (#1233)
  fix(github): tell an operator why a refused apply's database is busy (#1224)
  fix(engine): do not mark an apply failed when its driver shuts down (#1234)
  feat(github): render live row-copy progress on sharded table lines (#1191)
  feat(ui): add approximate row and byte formatters (#1236)
  fix(planetscale): delete the branch an apply created when it fails before its deploy request (#963)
  feat(api): app grouping field on database config (#1226)
  feat(cli): filter pulled tables with --table (#1235)
  ...

# Conflicts:
#	docs/configuration.md
#	pkg/ddl/postgres_parser.go
#	pkg/ddl/postgres_parser_test.go
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.

3 participants