Skip to content

feat(observer): expose typed workflow events - #1111

Closed
jonwinton wants to merge 2 commits into
mainfrom
jonwinton/add-workflow-observer
Closed

feat(observer): expose typed workflow events#1111
jonwinton wants to merge 2 commits into
mainfrom
jonwinton/add-workflow-observer

Conversation

@jonwinton

@jonwinton jonwinton commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Superseded

Do not merge this monolithic PR. Review the smaller dependency-ordered replacement stack instead:

  1. fix(dbconn): return committed retry row count #1112 — committed retry row accounting
  2. feat(copier): expose completed work totals #1113 — copier completed-work totals
  3. feat(move): return cutover mutation results #1114 — result-bearing cutover callbacks
  4. feat(status): own workflow lifecycle events #1115 — status-owned lifecycle API
  5. feat(workflow): unify status lifecycle reporting #1116 — migration and move lifecycle adoption

The sequence preserves the original exporter-neutral workflow evidence goal while separating prerequisite correctness fixes from the lifecycle API and runner integration. Each PR is independently reviewed and verified, and each PR after #1112 is based on its predecessor.

Original scope

Typed workflow stage, outcome, completed-work, durable-mutation, and terminal-ownership events for migration and move consumers such as Strata. No OpenTelemetry dependency is introduced in Spirit.

@jonwinton jonwinton added the ai-outer-loop Managed by outer-loop label Aug 5, 2026
@jonwinton
jonwinton force-pushed the jonwinton/add-workflow-observer branch from 0ba1e9d to 78254c2 Compare August 5, 2026 19:04
@jonwinton
jonwinton requested review from aparajon and morgo August 5, 2026 19:10
Comment thread pkg/migration/runner.go Outdated
Comment thread pkg/migration/workflow_observer.go Outdated
@jonwinton
jonwinton force-pushed the jonwinton/add-workflow-observer branch from 78254c2 to 5fdd552 Compare August 5, 2026 20:52
@jonwinton
jonwinton requested a review from morgo August 5, 2026 20:53
@jonwinton

Copy link
Copy Markdown
Collaborator Author

Review follow-up: the dependency contract now emits durable-mutation evidence after the schema/table cutover becomes externally visible, independent of copied-row totals. Terminal observer callbacks are also panic-isolated so caller-owned terminal accounting can still run. The focused race suites, affected package tests, lint, build, and all repository checks pass at eff242d. Strata #324 consumes this contract and remains ordered behind this PR.

@aparajon

aparajon commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head eff242d30842921fa4429585c0aa1d10ef6368d9.

Verdict: no fix-before-merge findings. I tried to break stage balance, event ordering, the durable-mutation emission point, and the move runner's terminal-ownership flags, and couldn't. Focused suites (pkg/status, pkg/copier, pkg/dbconn retry test, pkg/migration and pkg/move observer tests, the reverse-window partial-restore test) all pass -race locally against real MySQL 8.0, including the two heavyweight scenario tests. Findings below are contract-documentation and coverage-of-rarer-paths items, none blocking.

Small, worth doing here

# Finding Where
1 The event vocabulary is engine-asymmetric but the doc doesn't say so: only the migration runner emits DurableMutation, only the move runner emits TerminalOwnership pkg/status/workflow.go:116
2 workflowOutcome classifies any error as Cancelled when the run context is cancelled, so a genuine failure racing an external cancel reports Cancelled pkg/migration/workflow_observer.go, pkg/move/workflow_observer.go

1. Say which runner emits which terminal evidence. A consumer reading WorkflowEvent alone can reasonably wait for TerminalOwnership from a migration or DurableMutation from a move; neither will ever come (move's durable-mutation fact flows through CutoverResult instead). One sentence on the WorkflowEvent doc pinning the current emitters keeps consumers from building on events that don't exist — especially since the asymmetry is deliberate and correct.

2. Best-effort outcome classification under concurrent cancellation. The ctx.Err() != nil disjunct is right for the common case (checker/flush errors that don't wrap ctx errors after a cancel), but it also converts a real failure into Cancelled when an unrelated cancel lands between the failure and the observation. Fine as a heuristic — just worth one doc line so nobody treats stage outcomes as exact.

Follow-up, not blocking

Three smaller items
  • The genuinely ambiguous cutover residue has no evidence channel in the migration runner. CutOver.Run handles connection-loss renames by verifying server state, so DurableMutation can't false-positive — but when the rename may have committed and every verification attempt also fails, Run returns an error while the swap may be durable. The migration runner never emits WorkflowTerminalOwnershipAmbiguous (the type exists; the move runner uses it), so consumers classify that as a plain failure. Emitting Ambiguous from the exhausted-verification path is additive to this API and would close the last window of the class this PR fixes.
  • The direct-exec path has the same window, unverified. A single non-ALTER statement (CREATE/DROP/RENAME TABLE) runs as one dbconn.Exec with no committed-state verification and no evidence event — a connection dropped after the server commits reports failure for a durable DDL. Rarer, and fixable later with the same verification pattern the cutover already has.
  • Observer plumbing is duplicated verbatim between the two runners (observeWorkflowStageStarted/Finished, runObservedCopy, workflowOutcome). The receiver methods belong in each runner.go, but the non-receiver logic (outcome mapping, totals extraction) could live once in pkg/status next to the types.

Verified solid

  • The durable-mutation emission point is exactly right. Emitted immediately after cutover.Run returns nil and before dropOldTable/cleanup/checkpoint-drop, so an empty-table ALTER whose cutover lands and whose cleanup then fails is now provably durable to the observer — TestWorkflowObserverReportsZeroRowCutoverBeforeCleanupError pins the full scenario against real MySQL, and I ran it locally. Multi-table cutovers issue one atomic RENAME TABLE for all tables, so the single boolean cannot observe a partial swap. And because Run returns nil whenever the rename verifiably committed (including the ambiguous connection-loss path), the event can't fire for a swap that didn't happen.
  • Stage events stay balanced on every path I traced. Copy finish always fires (including the cancelled-with-nil-copier-error guard via the chunker's read-completion check); catch-up observes watermark/flush errors; checksum wraps the checker; sentinel-wait and reverse-window emit nothing unless they actually start and always finish once started (the waitStarted/started latches). Optional stages that never begin are silent by design.
  • No concurrent emission. The continuous checksum that runs during a sentinel wait uses a separate ContinuousChecker and never touches the observed checksum path; the sentinel Exists probe runs on Wait's calling goroutine; copier totals are atomics read only after copier.Run returns. Events reach the observer in authoritative order on the runner goroutine, as the interface doc promises.
  • Move terminal ownership flags flip at the right instants. Ambiguity is set at the first source un-retire rename inside the reverse cutover and cleared only in finalizeReverse before best-effort cleanup — so a marker/checkpoint drop failure can never reclassify a completed rollback; a phaseReverting resume marks ambiguous; a forward-cutover failure maps ambiguity from the cutover's own flags including the rename-rollback-failed sentinel; and terminalObserved makes the terminal event exactly-once.
  • CutoverResult protects non-idempotent traffic switches. A callback that reports a durable mutation and then fails aborts the retry loop with a manual-intervention error instead of re-running the switch.
  • RetryableTransaction now reports only the committed attempt's rows, with fatal and retries-exhausted paths returning 0 — pinned by a test that forces a real lock-wait retry.

This review was generated by Claude Code (claude-fable-5).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-outer-loop Managed by outer-loop

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants