fix(fork): wait for branches to be cancelled before the fork returns by YJ-928 · Pull Request #591 · zigflow/zigflow · GitHub
Skip to content

fix(fork): wait for branches to be cancelled before the fork returns - #591

Merged
mrsimonemms merged 1 commit into
zigflow:mainfrom
YJ-928:fix/fork-cancel-waits-for-branches
Sep 18, 2026
Merged

fix(fork): wait for branches to be cancelled before the fork returns#591
mrsimonemms merged 1 commit into
zigflow:mainfrom
YJ-928:fix/fork-cancel-waits-for-branches

Conversation

@YJ-928

@YJ-928 YJ-928 commented Sep 18, 2026

Copy link
Copy Markdown
Member

Description

When a workflow is cancelled while a non-competing fork is running, the fork's branches end as TERMINATED rather than CANCELED, and the tasks inside them never observe the cancellation, so no dev.zigflow.task.cancelled event is emitted for them.

This is a follow-up to #588. The fork waits for its branches with workflow.Await on the workflow context. Once that context is cancelled the await returns at once, so the fork closes in the same workflow task that requests the branch cancellations. A non-competing fork uses the default TERMINATE parent close policy, so the branches are terminated before the cancellation reaches them. Competing forks are not affected because their branches use REQUEST_CANCEL.

This change waits for every branch future to resolve on a disconnected context before the fork returns the cancellation. A child workflow future only resolves once the child has closed, so every branch has been cancelled by the time the workflow closes. The Gotchas section of the fork docs now says what cancelling the workflow does to the branches.

Related issue

Relates to #587. Follow-up to #588, which fixed it.

How to test

Unit test

go test ./pkg/zigflow/tasks/ -run TestForkTaskBuilderExecWaitsForBranchesWhenWorkflowCancelled -v

On main this fails because neither branch observes the cancellation. With this change it passes.

Manually

document:
  dsl: 1.0.0
  taskQueue: zigflow
  workflowType: fork-cancel
  version: 0.0.1
do:
  - parallel:
      fork:
        branches:
          - left:
              wait:
                minutes: 10
          - right:
              wait:
                minutes: 10

Start a worker with zigflow run -f workflow.yaml, then:

temporal workflow start --task-queue zigflow --type fork-cancel --workflow-id fork-cancel
# wait a few seconds for both branches to start
temporal workflow cancel --workflow-id fork-cancel
temporal workflow list
temporal workflow show --workflow-id fork-cancel_fork_left --output json

On main, fork-cancel is Canceled but fork-cancel_fork_left and fork-cancel_fork_right are Terminated. Their history ends with WorkflowExecutionTerminated, with the reason by parent close policy. With this change all three are Canceled.

Checklist

  • This PR links to an issue using Fixes #..., Closes #... or Relates to #...
  • All tests pass
  • Tests have been added or updated where behaviour changed
  • Documentation has been updated where needed

@YJ-928 YJ-928 mentioned this pull request Sep 18, 2026
Merged
4 tasks
@mrsimonemms

mrsimonemms commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

@YJ-928

YJ-928 commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

Thanks @mrsimonemms, added in 11da2ca: tests/e2e/tests/cancel-fork, following the cancel test. It cancels the workflow once both branches have started their timers, then checks that the workflow and both branches close as CANCELED and that the task after the fork never runs.

On main it fails with both branches TERMINATED; with this change it passes. cancel-fork works for me as the name.

@mrsimonemms mrsimonemms added the ai-review Trigger an AI code review for this PR label Sep 18, 2026
@github-actions

github-actions Bot commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex PR Review

Automated review note: Zigflow uses AI-assisted PR review tooling to help identify potential bugs, regressions and edge cases.
These comments are intended as supplemental review feedback only and may occasionally be incorrect or overly cautious.
Please treat them as additional input for discussion rather than authoritative project direction.

No blocking issues. Ship it.

Test execution could not be verified: the read-only environment prevents Go from creating its module cache.

@YJ-928

YJ-928 commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

Thanks for the review. Applied in f009001: fork branches now start with WaitForCancellation: true, and a new unit test checks that a branch's cancellation cleanup finishes before the fork returns.

For context, with the SDK version in go.mod (v1.48) the worker already resolves a child future only once the child has closed, so the test passes either way and the e2e runs showed branches closing before the parent. Setting the option makes that guarantee explicit rather than relying on behaviour the SDK does not document.

@mrsimonemms mrsimonemms added ai-review Trigger an AI code review for this PR and removed ai-review Trigger an AI code review for this PR labels Sep 18, 2026
@mrsimonemms

mrsimonemms commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Thanks. I'm going to do the automated review again. Because Zigflow uses rebased commits, can you squash all these into a single commit please?

I'm doing another automated review. Automated review done and looks good. I'm quite busy today, so might not get to merge it until later this afternoon.

@YJ-928 @claude
fix(fork): wait for branches to be cancelled before the fork returns
78e08c8 
When a workflow was cancelled while a non-competing fork was running, the
await on the cancelled context returned at once and the fork closed in the
same workflow task that requested the branch cancellations. The parent
close policy then terminated the branches before they received the
cancellation, so they ended as TERMINATED and their tasks never observed
it.

The fork now waits for every branch future to resolve on a disconnected
context before returning the cancellation. Branches are started with
WaitForCancellation so each future resolves only once its branch has
closed. Competing forks were not affected because their branches use the
REQUEST_CANCEL parent close policy.

Adds unit tests for branch cancellation and cancellation cleanup, a
cancel-fork e2e test, and a Gotchas line in the fork docs.

Signed-off-by: Yash B Joshi <68319416+YJ-928@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@YJ-928
YJ-928 force-pushed the fix/fork-cancel-waits-for-branches branch from f009001 to 78e08c8 Compare September 18, 2026 07:27
@mrsimonemms mrsimonemms added ai-review Trigger an AI code review for this PR and removed ai-review Trigger an AI code review for this PR labels Sep 18, 2026
@YJ-928

YJ-928 commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

@mrsimonemms squashed into a single commit, 78e08c8. The code is unchanged from what you reviewed, the tests pass and the automated review is clean. No rush on the merge, thanks for taking the time to review it.

@mrsimonemms
mrsimonemms merged commit 44d7f1c into zigflow:main Sep 18, 2026
23 checks passed
@mrsimonemms

mrsimonemms commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

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

Reviewers

No reviews

Assignees

No one assigned

Labels

ai-review Trigger an AI code review for this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@YJ-928 @mrsimonemms