fix(cluster): harden system-all direct reseed replay

[caffeinated92]

Summary

• classify direct mysqldump reseed streams into application SQL and system-catalog SQL
• replay extracted mysql.system-all content in a controlled second phase instead of relying on mysql --force
• add targeted unit/regtest coverage and implementation notes for the direct reseed path

Testing

• go test ./cluster -run 'Test.*(DirectReseed|Splitdump|SystemAll|CreateUser|InstallPlugin|RetryDirectReseed|CheckDirectReseed|ProtectedSystemAccount|ClassifyStream)'
• go test ./utils/dbhelper -run 'Test.*Plugin'
• go test ./utils/splitdump

Notes

• This PR fixes the direct dump-from-master/system-all path.
• Related to [Bug]: Make --system=all reseed work without relying on --force #1698; it does not claim to close the remaining older monolithic restore paths.

[claude]

Review

Reviewed the diff for fix(cluster): harden system-all direct reseed replay (18 files, +4248/-90) against doc/implementation/DEVELOPMENT_LAWS.md (esp. F2-F4 disk/unbounded-collection invariants, T1 dbhelper-only SQL) plus general correctness/test-coverage concerns. Verified each finding directly against the current diff (line numbers below are accurate as of the head commit).

1. Stuck IsReseeding="direct" state when --backup-restore-version-strict blocks a reseed (bug, should block merge)

cluster/srv_job_backup.go:4300-4314 - JobRejoinMysqldumpFromSource's new strict-mode version check returns early before defer dest.SetInReseedBackup("") is registered (line 4314). The doc comment claims this is safe because the check is "placed before any state-changing side effect ... so a strict-mode block leaves dest untouched" - but that's only true if callers never set the flag themselves first.

They do: cluster/srv_rejoin.go:496 (RejoinDirectDump) calls server.SetInReseedBackup(tool) before launching go cluster.JobRejoinMysqldumpFromSource(...) (lines 547/549). Every other early-return inside RejoinDirectDump explicitly re-clears the flag (srv_rejoin.go:500,508,521,540), but that guard lives only in the caller, not in the callee's new strict-check branch. So: with BackupRestoreVersionStrict=true and a genuine version/family mismatch, dest.IsReseeding is set to "direct" and never cleared - SetInReseedBackup/HasAnyReseedingState (srv_set.go) have no TTL, and nothing else in the codebase clears it (confirmed reconcileDeferredRejoinReseeds in srv_lostevents.go only warns, never resets). The destination server is permanently blocked from any future reseed/rejoin until the process restarts.

The new test TestJobRejoinMysqldumpFromSource_BlocksWhenStrictAndVersionMismatch (cluster/srv_job_test.go) calls JobRejoinMysqldumpFromSource directly without pre-setting SetInReseedBackup, so it passes without exercising the real caller pattern and doesn't catch this.

Suggested fix: move the SetInReseedBackup(task) call itself inside JobRejoinMysqldumpFromSource after the strict-check gate (matching the comment's actual intent), or have the strict-check branch clear the flag if already set (mirroring the pattern already used elsewhere in RejoinDirectDump).

2. Orphaned .tmp-* artifact directories are never reclaimed (F3/F4 disk-fill risk)

cluster/srv_job_reseed.go:603 - the hourly retention sweep (PurgeExpiredDirectReseedSystemArtifacts) explicitly skips every .tmp-* entry: if !e.IsDir() || strings.Contains(e.Name(), ".tmp-") { continue }. The temp artifact directory (finalDir + ".tmp-" + suffix, created in newDirectReseedSystemArtifactWriter) is only ever removed via discard(), which is called from in-process error paths in srv_job_backup.go - there's no crash-recovery/startup sweep anywhere in cluster/ or server/. If repman is OOM-killed, SIGKILLed, or the host reboots mid direct-reseed, the partial gzip artifact is orphaned forever, and repeated crashes accumulate unbounded disk usage. Per DEVELOPMENT_LAWS.md, this is exactly the class of bug F2/F3/F4 call a release blocker.

3. PurgeExpiredDirectReseedSystemArtifacts silently disables itself when backup-keep-last=0

cluster/srv_job_reseed.go:585-590 - the function reuses the backup-keep-last flag and returns immediately when keep <= 0. That's documented as "unlimited" semantics for ordinary backups, but here it also silently opts this unrelated artifact class out of all cleanup with no separate warning/log. An operator who sets backup-keep-last=0 for normal backup retention reasons gets unbounded direct-reseed-artifact growth as a side effect they didn't ask for.

4. classify.go doesn't mirror split.go's boundary-detection guard (edge case, low practical risk)

utils/splitdump/classify.go:110-116 (ClassifyStream) evaluates isSystemSectionBoundary/isNonSystemSectionHeader on every scanned line. The reference implementation it's meant to mirror, SplitDumpLineParser (utils/splitdump/split.go:338-458), only evaluates that same prefix chain while not inside a table's data section (onTableData gate) - the entire header chain is skipped between LOCK TABLES and UNLOCK TABLES;. This asymmetry isn't called out anywhere despite the doc comment implying the two share the same boundary contract.

In practice this is hard to trigger against real mysqldump/mariadb-dump output, since embedded newlines in string literals are always escaped as literal \n text (not raw 0x0A) regardless of --skip-extended-insert, so genuine dump content can't produce a physical line that spuriously starts with e.g. INSTALL PLUGIN/CREATE USER/USE. But it's a real latent gap against non-standard/hand-rolled dump producers, and worth either documenting explicitly as an accepted assumption or adding the same state guard for defense-in-depth, given this code is on the reseed data-safety path.

5. Minor: dead continueOnError branch

cluster/srv_job_backup.go - continueOnError is threaded through planAndExecSplitdump/execSplitdumpSingle/execSplitdumpBatch, but per the function's own doc comment every production call site (restoreSplitdumpFileGo, restoreSystemCatalog) now always passes false. Not a bug, but it leaves two disconnected mechanisms (the boolean flag vs. the live dbhelper INSTALL-PLUGIN-skip lookup) for a future maintainer to reason about. Consider dropping the flag from the production path if it's only exercised in unit tests.

What looks good

• New SQL access (utils/dbhelper/plugins.go, system-catalog replay path) correctly routes through dbhelper per T1 - no raw SQL found outside it.
• Good test coverage breadth: srv_job_backup_system_reseed_test.go, srv_job_reseed_test.go, splitdump/classify_test.go cover the classification and replay logic reasonably thoroughly, and there's a dedicated regtest scenario (test_direct_reseed_system_all.go) satisfying T13's real-cluster testing requirement.
• The two-phase classify -> replay approach (avoiding blind mysql --force) is a sound design for the stated problem, and the implementation status doc under doc/implementation/cluster/ is a nice touch per repo convention.

Verdict

Request changes. Finding #1 is a real regression that can permanently wedge a destination server's reseed capability in a common opt-in configuration (--backup-restore-version-strict), and finding #2 is an unbounded-disk-growth path on crash, which DEVELOPMENT_LAWS.md marks as a release blocker (F2/F3/F4 outrank other laws). Both are narrow, mechanical fixes - the surrounding design and test investment in this PR are solid otherwise.

[caffeinated92]

@claude review PR based on code changes

[claude]

Claude finished @caffeinated92's task in 2m 4s —— View job

---

Claude PR Review

• Gather context (PR diff vs origin/develop, prior review comment, DEVELOPMENT_LAWS.md)
• Re-review current diff, focusing on whether the two blocking findings from the earlier review (stuck IsReseeding="direct" state, orphaned .tmp-* artifacts) were addressed by the latest commits (f74a064, 2df2495)
• Review remaining changed files for new issues
• Post final review

I diffed f74a064..2df2495 (the "harden direct reseed cleanup and splitdump restore" follow-up commit) against the earlier review posted on this PR. All five findings from that pass are addressed:

1. Stuck IsReseeding="direct" on strict-mode block — fixed. cluster/srv_job_backup.go:4300-4310 now clears the flag itself (if dest.HasReseedingState(task) { dest.SetInReseedBackup("") }) inside the strict-check branch, before returning — matching the guard pattern already used at every other early-return in RejoinDirectDump (srv_rejoin.go). TestJobRejoinMysqldumpFromSource_BlocksWhenStrictAndVersionMismatch (cluster/srv_job_test.go) was updated to pre-set dest.SetInReseedBackup("direct") before calling the function, so it now actually exercises the real caller pattern and would have caught the original regression.

2. Orphaned .tmp-* artifact dirs never reclaimed — fixed. New reapOrphanedDirectReseedTmpDirs (cluster/srv_job_reseed.go:673-712) reaps any .tmp-* directory once !server.HasAnyReseedingState(), reasoning correctly that a writer still open in-process always keeps that flag set for its whole lifetime, so an idle server can't own a leftover temp dir from this process — anything found belongs to a dead one. It's wired into the existing hourly sweep (cluster.go:1170, heartbeats%3600==0), runs unconditionally regardless of backup-keep-last, and is path-validated via isPathWithinBase. TestPurgeExpiredDirectReseedSystemArtifactsReapsOrphanedTempDirsWhenIdle covers the crash-recovery scenario directly, and a renamed sibling test (...IgnoresTempDirsWhileReseeding) confirms an in-flight writer's temp dir is still left untouched. One residual (non-blocking) note: cleanup now happens on the hourly tick rather than at process startup, so a crash immediately after a hostile OOM-kill can still leave the orphan on disk for up to ~1h before the sweep reaps it — bounded and no longer unbounded/forever, so this satisfies F3/F4, just worth knowing it isn't instant.

3. backup-keep-last=0 silently disabling artifact cleanup — addressed via explicit warning, per the suggestion in the original finding. hasDirectReseedArtifactDir gates the new LvlWarn log to only fire for servers that actually have a published artifact affected by it, not every cluster that's never used direct reseed. TestHasDirectReseedArtifactDir covers the gate, including the "unpublished .tmp-* alone doesn't count" edge case.

4. classify.go boundary-detection asymmetry vs split.go — fixed. utils/splitdump/classify.go:108-131 now tracks an inTableData gate that mirrors SplitDumpLineParser's onTableData state in split.go: between LOCK TABLES `...` and UNLOCK TABLES;, the header-prefix chain (isSystemSectionBoundary/isNonSystemSectionHeader) is skipped entirely, matching the reference parser's contract instead of evaluating on every line. New cases added to classify_test.go.

5. Dead continueOnError branch — removed, not just left dormant. The parameter was stripped end-to-end from planAndExecSplitdump, streamSplitdumpStatements, and splitdumpExecutor.single (cluster/srv_job_backup.go), and the now-obsolete TestPlanAndExecSplitdumpContinueOnError test was replaced with TestPlanAndExecSplitdumpBatchesInserts, which documents why continue-on-error semantics live in execSplitdumpSingle's live dbhelper lookup instead. Doc comments across the touched functions were updated to match, and SYSTEM_ALL_RESEED_IMPLEMENTATION_STATUS.md's retention section was updated to describe the new warn/reap behavior accurately.

I did not find any new issues introduced by this follow-up commit — it's a narrow, mechanical set of fixes that match what was requested, with regression tests specifically targeting the failure patterns identified (real caller pre-state for the strict-mode test, crash-simulated orphan reaping, boundary-gate parity cases). I was not able to execute go build/go test in this sandboxed environment (Bash commands beyond read-only git operations required approval that wasn't available), so this is a static review of the diff — the tests referenced above look correctly targeted but weren't run here.

Verdict: looks ready. Both previously-blocking findings are resolved with direct regression coverage; the remaining items were either fully fixed or reasonably addressed via explicit warning as suggested.

[caffeinated92]

@claude review PR for one more time

[claude]

Claude finished @caffeinated92's task in 2m 13s —— View job

---

Claude PR Review

• Gather context (PR diff vs origin/develop, prior review findings, DEVELOPMENT_LAWS.md)
• Re-check whether findings from previous reviews still hold against the current head commit
• Look for any new issues in the current diff
• Post final review

I independently re-verified the current head (2df2495, unchanged since the prior review pass) against both earlier review rounds rather than just trusting the previous summary. All five previously-raised findings check out as genuinely fixed:

1. Stuck IsReseeding="direct" on strict-mode block. Confirmed at cluster/srv_job_backup.go:4300-4309 — the strict-check branch now clears dest's reseeding state (dest.HasReseedingState(task) → dest.SetInReseedBackup("")) before returning, matching the guard pattern used at every other early-return in RejoinDirectDump. TestJobRejoinMysqldumpFromSource_BlocksWhenStrictAndVersionMismatch (cluster/srv_job_test.go:818) now pre-sets dest.SetInReseedBackup("direct") before calling the function, so it exercises the real caller pattern and asserts !dest.HasAnyReseedingState() afterward — it would have caught the original regression.

2. Orphaned .tmp-* artifact dirs. Confirmed at cluster/srv_job_reseed.go:692-713 (reapOrphanedDirectReseedTmpDirs). I traced the race-freedom claim myself rather than taking it at face value: newDirectReseedSystemArtifactWriter is only ever constructed from inside JobRejoinMysqldumpFromSource (srv_job_backup.go:4465), which is only reachable after the caller (RejoinDirectDump, srv_rejoin.go:496) has already called SetInReseedBackup — so there's no window where a fresh writer's temp dir can exist before HasAnyReseedingState() observes it. The sweep is wired hourly via cluster/cluster.go:1170 (goRun(cluster.PurgeExpiredDirectReseedSystemArtifacts)), and reaping is gated correctly (skips entirely while the server has any reseeding state in flight). TestPurgeExpiredDirectReseedSystemArtifactsReapsOrphanedTempDirsWhenIdle and the sibling ...IgnoresTempDirsWhileReseeding test both the crash and in-flight cases directly. As previously noted, this bounds the orphan window to ≤1h (next hourly tick) rather than eliminating it — that's an acceptable trade given F3/F4 only require bounded, not instant, reclamation.

3. backup-keep-last=0 silently disabling artifact cleanup. Confirmed — hasDirectReseedArtifactDir (srv_job_reseed.go:576) correctly gates the LvlWarn log to only fire when a real published artifact exists, verified by TestHasDirectReseedArtifactDir's "unpublished .tmp-* alone doesn't count" case.

4. classify.go/split.go boundary-detection asymmetry. Confirmed — utils/splitdump/classify.go:107-133 now has an inTableData gate that mirrors split.go's onTableData state machine (same LOCK TABLES `/UNLOCK TABLES; entry/exit prefixes at split.go:365/339), with the header-prefix chain skipped entirely while inside table data, same as the reference parser.

5. Dead continueOnError branch. Confirmed removed — zero occurrences of continueOnError remain in cluster/srv_job_backup.go. The INSTALL-PLUGIN-skip replacement (utils/dbhelper/plugins.go's GetPluginStatusConn + resolveInstallPluginSkip in srv_job_backup.go:1490-1511) is appropriately conservative: only an unambiguous ACTIVE match is skipped, every other outcome (absent, present-but-inactive, ambiguous, lookup error) surfaces as a fatal error rather than silently proceeding.

I also spot-checked files not covered in depth by the prior two passes:

• utils/dbhelper/performance.go/plugins.go: GetPlugins was cleanly moved (not duplicated) — confirmed a single definition and both call sites (cluster/srv.go:1439, regtest/test_direct_reseed_system_all.go:74) still resolve correctly.
• config/error.go: pure gofmt realignment from map-key insertions/reordering, no semantic change.
• regtest/regtest.go / server/regtest.go: the five new testDirectReseedSystemAll* scenarios are registered and dispatched following the exact existing pattern used for every other regtest.
• cluster/cluster.go:1170: the hourly-sweep wiring is a single well-placed goRun addition alongside the existing ResticPurgeRepo call.

No new issues found. I was not able to execute go build/go test in this sandboxed environment (non-git Bash commands require approval that wasn't available here), so this remains a static review — but I verified the specific claims from the previous review's "fixed" summary by reading the actual code paths and call graphs myself rather than relying on the prior pass's word for it.

Verdict: looks ready to merge. All findings from both earlier review rounds are genuinely resolved with targeted regression coverage, and I didn't find anything new in this third pass.