feat(workflow): unify status lifecycle reporting - #1116
Conversation
96cacd1 to
830f57f
Compare
830f57f to
f28e675
Compare
aparajon
left a comment
There was a problem hiding this comment.
🤖 Companion review to my #1115/#1118 comments, from the SchemaBot/Strata integration seat. Note: no Go test workflow ran at this head (only DCO/Semgrep/zizmor) — the local -race runs below are the test signal (go build ./...; pkg/status full; migration/move lifecycle-scoped suites against MySQL).
| # | Tier | Item |
|---|---|---|
| 1 | Fix before merge | CutOver — the operator-critical phase — emits no lifecycle event in any runner (migration runner.go:495; move runner.go:1182, 1290 — plain status.Set, only post-success DurableMutation). A failed cutover's event stream ends at "WaitingOnSentinelTable finished succeeded": a span consumer can't build a cutover span and can't distinguish "failed in cutover" from "hung pre-cutover". #1118 brackets CutOver in both runners, both paths. |
| 2 | Fix before merge | datasync is not instrumented at all (bare status.State, plain Sets at runner.go:294/317/351/422; no SetWorkflowObserver). Either instrument it or scope it out explicitly in the body — "unify" currently implies coverage. |
| 3 | Fix before merge | Panic anywhere except ReverseWindow leaks a dangling "started" event — only finishReverseWindowAttempt (reversewindow.go:118-133) is panic-safe; a panic in copier.Run, the catch-up flush, or checker.Run propagates with the attempt permanently open. 1 of 9 attempt sites protected, in the same PR that built elaborate panic handling for that one site. |
| 4 | Small | Four engine behavior changes bundled into an observability PR — each defensible, none observability-only; suggest splitting: (a) errRenameRollbackFailed now aborts outer cutover retries (cutover.go:143-147); (b) phaseReverting persist flips best-effort → fail-closed after feed.Close(), so a checkpoint-write failure aborts a reverse cutover at a point the revert can't simply be re-waited (reversewindow.go:362-366); (c) new persisted checkpoint phase "reverse_finalized" (reversewindow.go:27, runner.go:831-834) — an older spirit reading it falls into the resume switch's default and attempts a forward resume of a move whose ownership was just restored to the source; needs at least a comment + release-note flag; (d) the runCopy nil-on-cancelled-ctx guard duplicated verbatim in two runners — right goal, but a copier returning nil on a cancelled unfinished copy is the copier's bug. |
| 5 | Small | Hours-long unevented gaps by design: move's post-copy Set chain (ApplyChangeset→RestoreSecondaryIndexes→AnalyzeTable, runner.go:1669-1690 — the code's own comment says the deferred-index ALTER "can run for hours") emits nothing, so the stream reads as a hang while status.State says otherwise; migration's AnalyzeTable/PostChecksum likewise — a PostChecksum flush failure exits the run after a "Checksum finished succeeded" event. |
| 6 | Small | finishReverseWindowAttempt converts runtime.Goexit into panic(nil) (completedNormally false + nil recover() → runtime.PanicNilError). Also: ctx should lead the parameter list. |
| 7 | Follow-up | Dual-context threading tax, quantified: 8 signatures now carry (parentCtx, ctx) plus 2 test-only shim wrappers; the waitStarted flag + closure-captured attempt around sentinel.Wait is duplicated in both runners. Every future phase pays this again — this is the strongest empirical argument for #1118's bracket. |
Coverage vs #1118 (the decision artifact): #1118 sees more phases — CutOver, PostChecksum, index-restore, AnalyzeTable, datasync — panic-inclusively, plus per-state wall-clock timing; #1116 has richer per-event evidence — outcome classification, copy totals, DurableMutation/TerminalOwnership, exact parent ctx for tracing — on fewer phases. The strongest #1116-only pieces (DurableMutation-before-fallible-cleanup, reverse-ownership classification) are exactly what Strata needs and are awkward as a bare bracket; the strongest #1118-only property (every phase closed on every exit, incl. panic) is what this PR delivers for only one site. The obvious synthesis: Tracker-style bracketing as the spine, this PR's evidence events attached inside Do/Set.
Poller impact (what SchemaBot sees): status.State names/ordinals untouched; three benign boundary shifts, all arguably more truthful — applyChangeset becomes visible before watermark-optimization disable (the long buffered-map drain no longer shows as copyRows); no spurious waitingOnSentinelTable window when RespectSentinel is set but no sentinel exists; reverseWindow set marginally earlier. The #844 asks (Resume/Throttled/Utilization on Progress) are untouched by this whole stack — tracking them separately.
Verified solid: DurableMutation emitted after cutover.Run success and before fallible cleanup, once-per-run, pinned by tests; Finish emits on the captured parent ctx (run-ctx only classifies outcome) — no lost events under cancellation; move's Terminal defer ordering correct; sentinel-wait callback is same-goroutine (no race, -race clean); sentinelWaitStartTime written before the atomic state store.
This review was drafted by Armand's AI agent (Claude Fable 5).
Summary
status.Lifecycleowner.panic(nil)underGODEBUG=panicnil=1.Verification
MYSQL_DSN=tsandbox:msandbox@tcp(127.0.0.1:8033)/test go test -race -count=1 ./pkg/status ./pkg/migration ./pkg/movego test -race -count=1 ./pkg/movego build ./...make lintReview order
This PR is part of the dependency-ordered replacement for #1111:
Review and merge in order; each PR after #1112 is based on its predecessor. The stack supersedes #1111.