Skip to content

fix(cluster): surface direct reseed progress and recent rate - #1697

Merged
caffeinated92 merged 3 commits into
developfrom
reseed-from-master
Aug 12, 2026
Merged

fix(cluster): surface direct reseed progress and recent rate#1697
caffeinated92 merged 3 commits into
developfrom
reseed-from-master

Conversation

@caffeinated92

@caffeinated92 caffeinated92 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Why this PR exists

Direct reseed from master (RejoinDirectDump / JobRejoinMysqldumpFromSource) already had byte accounting internally for its stall watchdog, but it did not surface that progress to the shared reseed progress model or the dashboard. Operators could see a reseed was in flight, but not how much data had been streamed or what speed it was moving at.

The existing reseed progress UI also only worked well for file-based restores with a known total size. Direct stream reseeds have real byte progress but no fixed total, and the prior UI could only fall back to a generic in-progress timer.

What this PR brings

Backend

  • surfaces direct reseed progress through the shared reseed progress framework
  • distinguishes direct reseed from backup-based mysqldump restore in the structured progress payload
  • reuses the same byte counter for both the stall watchdog and displayed progress, so they cannot diverge
  • adds a sampled recent/windowed reseed rate alongside the existing lifetime average rate
  • resets recent-rate sampling cleanly across reseed lifecycle boundaries

Frontend

  • improves the reseed progress modal for unknown-total byte-stream reseeds
  • shows streamed bytes, elapsed time, and rate for direct reseed instead of only a generic timer
  • shows both current-ish (now) and lifetime average (avg) rate when recent-rate samples are ready

Tests

  • adds direct reseed progress coverage in Go tests
  • adds backend reseed recent-rate sampling tests
  • adds React utility tests for byte-instrumented unknown-total reseeds and combined recent/average rate formatting

Validation

  • go test ./cluster -run 'TestRecentReseedRate|TestSampleReseedRate'
  • node "share/dashboard_react/src/utility/__tests__/reseedProgress.test.js"
  • earlier direct reseed changes were also validated with targeted cluster tests before commit

Notes

  • this PR contains only the branch commits on top of develop

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review: surface direct reseed progress and recent rate

Reviewed the diff (cluster/restore_progress.go, cluster/srv.go, cluster/srv_job_backup.go, tests, and the React modal/utility changes). The working tree here already has these commits merged, so I read the code in full context rather than only the patch.

Summary

Good, well-scoped change. It closes a real gap (direct/streamed reseed had no byte progress surfaced to the dashboard) and does it by reusing the existing dest.reseedBytes counter for both the stall watchdog and the displayed progress instead of adding a second counter — that's the right call, it removes a class of "watchdog and UI disagree" bugs by construction. The windowed "recent rate" design (fixed-size ring of (bytes, time) samples, first-vs-last delta) is simple and appropriate for a monitoring signal; using atomic.Value for single-writer/multi-reader access is correct and consistent with the existing reseedBytes/reseedTotal/reseedStart fields on ServerMonitor.

Code quality

  • Comments are excellent — they explain why (e.g. why the writer is wrapped rather than the reader for the stall watchdog, why the window is capped, why beginReseedProgress resets the window) rather than restating the code. Matches the file's existing convention.
  • sampleReseedRate/recentReseedRate/window reset on startReseedProgress/beginReseedProgress/stopReseedProgress is a clean, minimal API — no unnecessary abstraction.
  • Frontend formatRate/formatRateLine correctly handle the "measuring…" placeholder and both ready/not-ready combinations; the JS tests cover the tricky edge case (recent ready before avg is) explicitly.

Potential bugs / nits (non-blocking)

  1. Minor race window: beginReseedProgress/stopReseedProgress run on the job goroutine while sampleReseedRate runs on the per-tick monitor goroutine (assertReseedProgressStates). Since assertReseedProgressStates loads info and then unconditionally calls sv.sampleReseedRate(), there's a small window where a reset (new reseed starting, or stopReseedProgress clearing the window) could race with a sample append for the previous reseed's info. Worst case is one noisy/near-zero sample that self-heals within the next tick or two — not a functional problem (doesn't affect F2–F4 monitoring liveness), just worth knowing about.
  2. Test flakiness risk (low): TestSampleReseedRate_ClearedByLifecycle (cluster/restore_progress_test.go) calls Store/sampleReseedRate back-to-back with no time.Sleep between samples, then asserts ready (which requires elapsed > 0). The sibling test TestRecentReseedRate_NotReadyUntilTwoSamples explicitly sleeps 5ms between samples to guarantee a positive delta — this test doesn't, so in principle (extremely unlikely on Linux, but not impossible under heavy scheduler contention or a coarser clock) two consecutive time.Now() calls could tie and fail the "sanity check" assertions. Consider a small time.Sleep for consistency with the other test, cheap insurance against flakes in CI.
  3. Doc nit: the RecentRateBytesSec json-tag comment says the window spans "~reseedRateWindowSize * monitoring-ticker" — with a 3-sample window the first-to-last span is actually ~2 tick intervals, not 3. Cosmetic only.
  4. The compress/gzip import reordering in srv.go is an unrelated drive-by gofmt/import-order fix — harmless, just flagging it's out of scope of the stated change.

Performance

No concerns. The rate window is capped at 3 samples (reseedRateWindowSize), each sampleReseedRate() call allocates one small slice (bounded, not accumulating), and it only runs once per tick per server with an active reseed — negligible relative to the existing monitoring loop.

Security

No concerns. No new user input is parsed; source.URL/dest.URL embedded into the progress/backup strings follow the same pattern already used elsewhere in this file for logging.

Test coverage

Strong. restore_progress_test.go covers the not-ready-until-two-samples case, window eviction, "reflects window not whole history" (the actual point of the feature — catching a stall before the lifetime average would), and lifecycle reset. srv_job_test.go adds an end-to-end test that drives JobRejoinMysqldumpFromSource through a fake dump/client pair and polls GetReseedProgress() for real mid-flight bytes, then confirms cleanup on return. The React test file covers reseedHasBytes and both formatRate/formatRateLine edge cases. Per this repo's testing law (T13), the real gate for a reseed/failover-adjacent change is the regtest/Docker suite rather than Go unit tests alone — the PR description notes only targeted Go/JS tests were run; if there's an existing regtest scenario that exercises RejoinDirectDump, it'd be worth confirming it still passes, since this touches the stall-watchdog wiring on that path.

Verdict

Approve. No blocking issues found; the nits above are optional polish (test robustness + a comment correction).

@caffeinated92

Copy link
Copy Markdown
Collaborator Author

@claude review PR based on code changes

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @caffeinated92's task in 1m 25s —— View job


Review: surface direct reseed progress and recent rate

  • Gather context (PR diff vs origin/develop, changed files)
  • Review backend changes (cluster/restore_progress.go, cluster/srv.go, cluster/srv_job_backup.go)
  • Review backend tests
  • Review frontend changes (ReseedProgressModal, reseedProgress.js)
  • Review frontend tests
  • Post final review

I read the current diff against origin/develop in full (all 8 files, not just the summary), independent of the earlier automated review comment on this thread.

Summary

Solid, well-scoped change that does exactly what the description says. The key design decision — reusing dest.reseedBytes for both the stall watchdog (cluster/srv_job_backup.go:4188, :4213) and the displayed progress, instead of the old local pumpProgress counter — is correct and removes a real class of bug (watchdog and UI silently disagreeing). The windowed recent-rate (reseedRateWindowSize = 3, cluster/restore_progress.go:143) built on atomic.Value with single-writer/multi-reader semantics is a clean, minimal addition consistent with the existing reseedBytes/reseedTotal/reseedStart pattern on ServerMonitor.

Verified correctness details

  • beginReseedProgress/startReseedProgress/stopReseedProgress all reset reseedRateWindow (restore_progress.go:80,86,99), so a new reseed never inherits stale samples — confirmed by TestSampleReseedRate_ClearedByLifecycle.
  • dest.reseedBytes is reset to 0 in beginReseedProgress (restore_progress.go:95) before the direct-reseed call site stamps it (srv_job_backup.go:3973), so the stall watchdog and the byte counter both start from a clean 0 each run.
  • backupStallWatchdog's progress *atomic.Int64 param (srv_job_backup.go:2410) matches &dest.reseedBytes's type — no adapter/wrapper needed, straightforward swap-in.
  • The RecentRateBytesSec json-tag comment (restore_progress.go:224) already says "~(reseedRateWindowSize-1) * monitoring-ticker" — correctly reflecting a 3-sample window spanning 2 tick intervals. (This was flagged as a cosmetic nit in the earlier automated review on this thread; it's already accurate in the current diff.)
  • Frontend formatRateLine/formatRate (reseedProgress.js:61-82) correctly special-case the sub-1s "measuring…" window and the "recent ready before avg is" edge case; reseedProgress.test.js covers all four branches (not ready, avg only, both ready, recent-ready-avg-not).
  • ReseedProgressModal/index.jsx:56-63 correctly gates the new byte-streamed-but-unknown-total branch on reseedHasBytes(rp) before falling back to the old generic rp.line timer, so file-based restores with a known total are unaffected.

Minor observations (non-blocking)

  1. cluster/restore_progress.go:29-37: the added comment explaining the benign race between assertReseedProgressStates (tick goroutine) calling sampleReseedRate() and the job goroutine concurrently resetting the window via begin/stopReseedProgress is accurate and appropriately justifies not adding a lock — worst case is one stray near-zero sample that self-heals within a tick or two. Worth double-checking this reasoning holds if reseedRateWindowSize is ever increased significantly (a bigger window makes one bad sample linger longer), but at 3 it's a non-issue.
  2. Test coverage is strong: restore_progress_test.go covers not-ready-until-two-samples, window-cap eviction, "reflects window not whole history" (the actual point of the feature), and lifecycle reset. srv_job_test.go's TestJobRejoinMysqldumpFromSource_ReportsProgress drives the real code path end-to-end (fake dump/client subprocesses) and polls GetReseedProgress() for real mid-flight bytes, then confirms cleanup after return — good integration-style coverage for a Go unit test.
  3. Per this repo's testing law (T13), the regtest/Docker suite is the actual required gate for reseed/failover-adjacent behavior, not Go unit tests alone. The PR description only lists targeted go test/node runs — if there's an existing regtest scenario that exercises RejoinDirectDump, worth confirming it still passes before merge, since this touches the stall-watchdog wiring on that path.
  4. The compress/gzip import-order change in srv.go is an unrelated drive-by formatting fix — harmless, just out of scope of the stated change.

Note on test execution

I was not able to run go test or node in this sandboxed environment (both required interactive approval that wasn't available here), so the above is based on static code reading, not a live run. The logic traced out cleanly by hand (counter resets, atomic types, window math), but if you want me to actually execute the test suite, that requires updating this workflow's --allowedTools.

Verdict

Looks good — no blocking issues. The items above are optional polish / confirmation items, not required changes.

@caffeinated92
caffeinated92 merged commit fe97518 into develop Aug 12, 2026
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.

1 participant