Skip to content

fix(migrate): emit progress line before first tick - #1478

Open
kalbasit wants to merge 1 commit into
mainfrom
fix-migrate-progress-log-flake
Open

fix(migrate): emit progress line before first tick#1478
kalbasit wants to merge 1 commit into
mainfrom
fix-migrate-progress-log-flake

Conversation

@kalbasit

Copy link
Copy Markdown
Owner

Summary

The chunks-to-nar progress reporter was driven purely by a ticker, so a migration
that finished inside a single interval emitted no progress line at all — the ticker
never fired.

That is a real reporting gap for short migrations, and it made
TestMigrateChunksToNar_CLI_ProgressLogEmitted non-deterministic. With the interval
overridden to 1ms and a one-item migration, the work raced the first tick and lost
roughly four runs in five. Verified on a clean tree with unrelated changes
stashed, so this is pre-existing on main and not caused by anything stacked above it.

Approach

Extract the emission into a logProgress closure and call it once up front when there
is work to do, then once per tick as before.

  • Guarantees at least one progress line whenever total > 0, which removes the race.
  • Gives operators immediate feedback instead of a silent first interval.
  • Gated on total > 0 so an empty run stays completely silent, preserving
    TestMigrateChunksToNar_CLI_NoProgressLogOnEmptyRun.

Test plan

  • task fmt exits 0
  • task lint exits 0
  • task test exits 0
  • 12/12 consecutive runs of both progress tests pass (was 4/5 failing before)

@kalbasit

kalbasit commented Aug 27, 2026

Copy link
Copy Markdown
Owner Author

This change is part of the following stack:

Change managed by git-spice.

@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. go Pull requests that update go code labels Aug 27, 2026
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d615af7c-6595-4503-82b2-1ff385b36c01

📥 Commits

Reviewing files that changed from the base of the PR and between b891f33 and 88e3be2.

📒 Files selected for processing (1)
  • pkg/ncps/migrate_chunks_to_nar.go

Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.


📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Migration progress is now reported immediately when work begins.
    • Progress updates use consistent reporting throughout the migration.
    • Migrations with no work remain silent to avoid unnecessary messages.

Walkthrough

The migration progress logic now uses a reusable logProgress closure. Non-empty migrations emit an immediate progress message before ticker-based updates. Empty migrations produce no progress output.

Changes

Migration progress reporting

Layer / File(s) Summary
Immediate and ticker-based progress logging
pkg/ncps/migrate_chunks_to_nar.go
The migration calculates and logs progress through logProgress. Non-empty migrations report before the ticker starts, while ticker events reuse the same logic. Empty migrations remain silent.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 88e3b

The change emits progress immediately for non-empty migrations while preserving empty-run behavior, with no actionable merge-blocking risk remaining after normal checks and review.

Poem

A rabbit checks the migration trail
Immediate progress marks the scale
The ticker follows with steady feet
Empty paths leave logs discreet
One shared closure keeps time neat
Hop, hop, the chunks complete:VEVENT

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: emitting a migration progress line before the first ticker event.
Description check ✅ Passed The description directly explains the reporting gap, the implementation approach, preserved empty-run behavior, and test results.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

The chunks-to-nar progress reporter was driven purely by a ticker, so a
migration that finished inside a single interval emitted no progress line
at all -- the ticker simply never fired. That is a real reporting gap for
short migrations, and it made
TestMigrateChunksToNar_CLI_ProgressLogEmitted non-deterministic: with the
interval overridden to 1ms and a one-item migration, the work raced the
first tick and lost roughly four runs in five (verified on a clean tree
with unrelated changes stashed).

Extract the emission into a logProgress closure and call it once up front
when there is work to do, then once per tick as before. The immediate
call makes at least one progress line guaranteed whenever total > 0, which
removes the race, and gives operators feedback instead of a silent first
interval.

The call is gated on total > 0 so an empty run stays completely silent,
preserving TestMigrateChunksToNar_CLI_NoProgressLogOnEmptyRun.

Verified: 12/12 consecutive runs of both progress tests pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014zzHofsGpUn34b21AeP3yP
@kalbasit
kalbasit force-pushed the fix-migrate-progress-log-flake branch from 88e3be2 to ef5be3b Compare August 27, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update go code size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant