Support workflow terminate rollback#14465
Conversation
🦋 Changeset detectedLatest commit: 7bb3bc4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
vaishnav-mk
force-pushed
the
vaish/terminate-rollback
branch
from
4614a06 to
f539470
Compare
|
✅ All changesets look good |
vaishnav-mk
force-pushed
the
vaish/terminate-rollback
branch
2 times, most recently
from
4ccbf75 to
01fd44d
Compare
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
vaishnav-mk
force-pushed
the
vaish/terminate-rollback
branch
5 times, most recently
from
f9b1b1a to
9ed9edf
Compare
|
@vaishnav-mk Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
vaishnav-mk
force-pushed
the
vaish/terminate-rollback
branch
3 times, most recently
from
50bf663 to
2c0a39a
Compare
vaishnav-mk
force-pushed
the
vaish/terminate-rollback
branch
2 times, most recently
from
4bafb21 to
ef038d9
Compare
workers-devprod
requested review from
a team and
james-elicx
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
/bonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Support workflow terminate rollback (re-review @ dd02374)
Re-reviewing after the two commits added since the last /bonk review (6c313a6 → dd02374). Most of the previously-raised concerns are now resolved. Details below.
Previously-raised items — status
1. Remote (production) API path sends rollback: true — now substantiated. ✅ (with one ask)
The prior review flagged utils.ts:131 as blocking because it was unclear whether the production API accepts rollback on terminate. The linked docs PR (cloudflare-docs#31769) documents the remote CLI form as a supported operation:
npx wrangler workflows instances terminate <WORKFLOW_NAME> <INSTANCE_ID> --rollbackand describes production semantics ("Workflows runs the rollback handlers registered by completed or eligible steps before the instance reaches the terminated state. Steps without rollback handlers are skipped."). That's good evidence the production API is ready and this is intentional, so I no longer consider it blocking. One remaining ask: please confirm the production PATCH .../status endpoint accepting rollback is already deployed (not gated behind the docs merge), since terminate.ts:65 prints unconditional success — if the field is silently dropped server-side today, users get a false "terminated successfully" for a rollback that never ran.
2. Devin's "failed steps won't re-register rollback handlers on replay" — resolved. ✅
dd02374 fixes this directly: the maybeError branch in context.ts:461-475 now calls #registerRollback before re-throwing the cached error, and the replay branch at context.ts:484-497 registers rollback and returns early. New test replays cached failed steps before terminate rollback when registry is empty (engine.test.ts) asserts ROLLBACK_STEP_SUCCESS fires for the failed step. Good coverage.
3. Repeated phase string-union nit — resolved. ✅
RollbackPhase type alias is now used across setRollbackPhase / rollbackPhase (engine.ts:336).
4. Double full-table scan in getEligibleRollbackSteps — resolved. ✅
Now a single SELECT ... ORDER BY id DESC pass building the sets in one loop (engine.ts:256-321).
Still open — non-blocking
5. Swallowed errors in replayRollbackRegistry.
engine.ts:988 catches and discards all replay errors with only a comment; engine.ts:1017 logs rollback failures to console.error. For a local-dev tool, "why didn't my rollback run?" is a real question. A debug-level log in the replay catch (even just the error message) would make silent zero-rollback outcomes diagnosable.
6. No user-facing signal for rollback outcome.
terminate.ts:65 prints plain success regardless of whether any rollback steps executed, were skipped, or failed. Not blocking, but surfacing a count (e.g. "ran N rollback step(s)") would make the feature observable — especially relevant given (5) and the empty-registry edge cases.
Minor / nits
resources/workflows.ts:924still useshandle as unknown as WorkflowHandleto pass terminate options, mirroring the same@cloudflare/workers-typesgap. Since the docs PR now typesterminate(options?: WorkflowInstanceTerminateOptions), worth a single tracking note so this cast is removed once the types package ships.registerRollbackFn(lib/rollback.ts) now updatesstepContext/outputin place on re-registration instead of disposing — correct for replay, but confirm the previousdupRollbackStub/disposeRollbackStublifecycle isn't leaking stubs when a cacheKey re-registers with a freshfn(the update path keepsexisting.fnand drops the newregistration.fnwithout disposing it).
Tests / changeset
- Changeset (
minorfor workflows-shared/wrangler/miniflare, patches for dependents) looks correct. - Wrangler tests assert exact request bodies for both local and remote
--rollback— good.
Overall: the design (persisted hasRollback + replay-to-rebuild-registry) is sound and the newest commit closed the last correctness gap Devin flagged. Approving pending confirmation on point (1) — production API rollback support being live.
|
Review posted to PR #14465. Summary of what I found on re-review (head commit Resolved since last review
Still open (non-blocking)
Nits
Net: sound design, last correctness gap closed; approving pending confirmation on production API readiness. |
|
/bonk rereview |
There was a problem hiding this comment.
Review: Support workflow terminate rollback (re-review @ 7b9fa6a)
Re-reviewing after the one commit added since the last /bonk review (dd02374 → 7b9fa6a, "Clarify rollback replay diagnostics"). That commit directly addresses two of the open non-blocking items from last time, so the diff is small and additive. Design remains sound.
Newly resolved since last review
Swallowed replay errors — resolved. ✅
engine.ts:988-991 now captures the replay error and emits console.debug("Rollback replay stopped:", replayErr) instead of silently discarding it. This makes the "why didn't my rollback run?" case diagnosable in local dev, which was my previous item (5). Since workflows-shared is a Worker package (not wrangler), console.debug is the right primitive here — consistent with the existing console.error/console.warn usage in this file.
registerRollbackFn re-registration stub lifecycle — clarified. ✅
lib/rollback.ts:113-114 now documents that the existing entry already owns the duped stub and duplicate registrations refresh stepContext/output only (the helper hasn't duped the incoming fn, so there's nothing to dispose). This resolves the leak concern I raised last time — confirms it's intentional, not an oversight.
Still open — non-blocking
1. Confirm production PATCH .../status accepts rollback and is deployed.
Still my only real ask. terminate.ts:65 prints unconditional 🥷 ... terminated successfully regardless of whether the remote API honored rollback: true (utils.ts:131). I checked the linked docs PR (cloudflare-docs#31769) — it's still in Draft with no approving reviews yet, so it documents intended semantics but doesn't by itself prove the production endpoint is live today. If the field is silently dropped server-side, remote users get a false success for a rollback that never ran. Please confirm the endpoint is deployed (independent of the docs merge); if it isn't, gate --rollback to --local until it is.
2. Rollback outcome still not surfaced to the user.
Nice to see executeRollbacks now returns { ranAny, allSucceeded } (lib/rollback.ts:172,225) — but both call sites (engine.ts:1016 and engine.ts:1302) discard the result, so it never reaches the CLI, and terminate.ts still prints a flat success. The plumbing is now half there; propagating a count (e.g. "ran N rollback step(s)") would close the observability gap and pairs well with item (1). Non-blocking.
Minor / nits (unchanged)
resources/workflows.ts:924still useshandle as unknown as WorkflowHandleforterminate(options), mirroring the@cloudflare/workers-typesterminate-options gap (same TODO as therestartcast at:919). Worth a single tracking note so both casts are removed once the types package ships.
Tests / changeset
- No test/changeset changes in this commit; the prior coverage (live / replayed / paused registry, failed-step re-registration asserting
ROLLBACK_STEP_SUCCESS, and exact request-body assertions for local + remote--rollback) still stands. - Changeset (
minorfor workflows-shared/wrangler/miniflare, patches for dependents) looks correct.
Overall: the last two commits closed the correctness gap (failed-step replay) and the diagnosability gap (replay debug log). The design (persisted hasRollback + replay-to-rebuild-registry) is solid. Approving pending confirmation on item (1) — production API rollback support being deployed, given the unconditional success message and the still-draft docs PR.
|
Re-review posted to PR #14465. SummaryI re-reviewed at head commit Newly resolved:
Still open (non-blocking):
Minor: the remaining Net: last two commits closed the failed-step replay correctness gap and the diagnosability gap. Design is sound; approving pending confirmation on the production API being deployed. |
vaishnav-mk
force-pushed
the
vaish/terminate-rollback
branch
from
ce29ed6 to
7bb3bc4
Compare
|
/bonk rereview |
|
UnknownError: ProviderInitError |
|
/bonk rereview |
There was a problem hiding this comment.
Review: Support workflow terminate rollback (re-review @ 7bb3bc4)
Re-reviewing after the two commits added since the last /bonk rereview (7b9fa6a → 7bb3bc4: "Cover rollback replay for unfinished steps" and "Fail rollback when replay misses eligible step"). These directly address Devin's most recent non-determinism concern, and the design is now noticeably more robust. Details below.
Newly resolved since last review
Devin's "replay could register handlers for the wrong steps / miss steps" concern — resolved. ✅
7bb3bc4 re-architects executeRollbacks (lib/rollback.ts:141-211) so execution is now driven by the persisted eligible steps (readEligibleRollbackStepsDesc()), not by whatever the in-memory rollbackRegistry happens to contain. The registry is now only the source of the rollback function; storage is the source of truth for which steps must roll back. Concretely:
- If a persisted eligible step has no handler in the registry after replay (e.g. non-deterministic control flow that didn't re-reach it), it now writes
ROLLBACK_STEP_FAILURE+ROLLBACK_FAILEDand halts, instead of silently skipping (lib/rollback.ts:162-176). That's the correct "production treats this as rollback failure" behavior. - Eligibility is computed as a
STEP_STARTwhose group hashasRollback: trueand no priorROLLBACK_STEP_SUCCESS/FAILURE(engine.ts:246-315) — so already-rolled-back steps aren't re-run, and both the live and post-restart paths now agree on the same set. Good unification. - New test
fails terminate rollback when replay cannot rebuild an eligible handlerasserts exactlyROLLBACK_STEP_FAILURE=["branchy-setup-1"],ROLLBACK_FAILED=1,ROLLBACK_COMPLETE=0. Solid coverage of the failure path.
Unfinished-step replay + latent priorityQueue crash — resolved. ✅
4ecfb4f adds coverage for started-but-unfinished steps (replays started unfinished steps before terminate rollback..., asserting ROLLBACK_STEP_SUCCESS=["started-setup-1"]). 7bb3bc4 pairs this with engine.ts:1010: this.priorityQueue ??= new TimePriorityQueue(...) before replayRollbackRegistry. This matters — during a post-restart terminate, init() hasn't run, so priorityQueue is undefined; replaying an unfinished step reaches context.ts:521-524 etc. which access this.#engine.priorityQueue under @ts-expect-error priorityQueue is initiated in init. Without this defensive init that path would throw at runtime. Good catch.
Still open — non-blocking (unchanged)
1. Confirm production PATCH .../status accepts rollback and is deployed.
Still my one real ask. terminate.ts:65 prints unconditional 🥷 ... terminated successfully regardless of whether the remote API honored rollback: true (utils.ts:131). The linked docs PR (cloudflare-docs#31769) documents intended semantics but doesn't prove the endpoint is live. If the field is silently dropped server-side, remote users get a false success for a rollback that never ran. Please confirm the endpoint is deployed; if not, gate --rollback to --local until it is.
2. Rollback outcome still not surfaced to the CLI.
executeRollbacks returns { ranAny, allSucceeded }, but both call sites (engine.ts:1017, engine.ts:1303) still discard it, and terminate.ts prints flat success. Now that a failed/missing rollback produces ROLLBACK_FAILED, surfacing a count (e.g. "ran N rollback step(s)") would make the difference between "ran", "skipped", and "failed" observable to the user. Pairs naturally with (1).
Minor / nits (unchanged)
resources/workflows.ts:924still useshandle as unknown as WorkflowHandleforterminate(options)— same@cloudflare/workers-typesterminate-options gap as therestartcast. Worth a tracking note so both are removed once the types package ships.
Tests / changeset
- New tests (started-unfinished replay, missing-handler failure) join the existing live / replayed / paused-registry and cached-failed-step coverage. Request-body assertions for local + remote
--rollbackstill stand. - Changeset (
minorfor workflows-shared/wrangler/miniflare, patches for dependents) looks correct.
Overall: the two new commits close the last correctness gap — rollback is now anchored to persisted eligible steps, missing handlers fail loudly instead of silently, and the replay path no longer risks an undefined-priorityQueue crash. Design (persisted hasRollback + replay-to-rebuild + storage-driven execution) is sound. Approving pending confirmation on item (1) — production API rollback support being deployed, given the unconditional success message.
|
Re-review posted to PR #14465. SummaryI re-reviewed at head commit Newly resolved:
Still open (non-blocking):
Minor: remaining Net: the design is sound and the last correctness gap is closed. Approving pending confirmation on item (1). I could not run the test suite (dependencies aren't installed in this environment), so my assessment is based on static analysis of the diff and surrounding code. |
|
prod api already supports this. config-service accepts rollback on terminate, engine handles terminate({ rollback }). no --local gate needed. outcome surfacing is out of scope here. prod terminate response is only { status, timestamp }; rollback result is visible later via status/logs. cast is temporary until workers-types ships terminate(options). kept local with todo.
|
There was a problem hiding this comment.
Thanks for the confirmations, @vaishnav-mk — that resolves my open items. Recording where each lands:
1. Production API rollback support — resolved. ✅ You've confirmed config-service accepts rollback on terminate and engine handles terminate({ rollback }) server-side. That's exactly the deployment signal I was asking for; with the production endpoint honoring the field, the unconditional 🥷 ... terminated successfully at terminate.ts:65 for the remote path is no longer a false-success risk, and no --local gate on --rollback is needed. The remote request-body assertion in workflows.test.ts is correct to keep.
2. Outcome surfacing — acknowledged as out of scope. ✅ Agreed this is a reasonable line to draw given the production terminate response is only { status, timestamp } and the rollback result surfaces later via status/logs. executeRollbacks still returns { ranAny, allSucceeded } and both call sites (engine.ts) discard it — leaving the plumbing in place for a future "ran N rollback step(s)" enhancement is fine; not blocking.
3. as unknown as WorkflowHandle cast — acknowledged. ✅ Confirmed it's kept local to terminate() with a TODO (resources/workflows.ts:923, wrapped-binding.worker.ts:112), mirroring the existing restart cast. Tracking note is sufficient; both come out together once @cloudflare/workers-types ships terminate/restart options.
All three prior concerns are now either resolved or accepted as scoped-out with sound rationale. The design — persisted hasRollback + replay-to-rebuild + storage-driven eligible-step execution with loud failure on missing handlers — is solid, and the test coverage (live / replayed / paused-registry / cached-failed-step / started-unfinished replay / missing-handler failure) is thorough.
Approving. 🚀
|
Done. I verified @vaishnav-mk's three responses against the actual code and posted an approving review to PR #14465. What I verified:
All prior open items are now resolved or scoped-out with sound rationale, so I approved the PR. |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
4 participants
Adds terminate rollback support across Workflows local tooling and the remote Wrangler terminate command.
This wires
rollback: truethrough the terminate path:@cloudflare/workflows-sharedbinding and local engineworkflows instances terminate --rollbackworkflows instances terminate --local --rollbackThe production Workflows API already accepts terminate rollback; this PR adds the SDK/Wrangler client surfaces and fixes local rollback recovery so rollback can run after local engine restart/eviction.
Rollback is only valid for terminate and is only serialized when explicitly set to
true.A picture of a cute animal (not mandatory, but encouraged)
wires cat