Skip to content

feat(taskflow)!: write the task's completion before waking the parent (7/8) - #243

Open
lokewate wants to merge 1 commit into
feat/taskflow-guarded-writes-5-wake-upfrom
feat/taskflow-guarded-writes-6-completion
Open

feat(taskflow)!: write the task's completion before waking the parent (7/8)#243
lokewate wants to merge 1 commit into
feat/taskflow-guarded-writes-5-wake-upfrom
feat/taskflow-guarded-writes-6-completion

Conversation

@lokewate

@lokewate lokewate commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

The task's own completion is now persisted before the parent workflow is woken, and the old buggy "already completed" short-circuit that a stale wake-up could match is removed.

Next PR: #244

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Other (please describe):

Changes Made

  • HandleTaskCompletion now writes the guarded CompleteTask (0 rows = logged, not an error — the parent is still woken) before calling back into the parent workflow, so the parent is never woken on a completion the DB never durably recorded.
  • Removes the old if record.State == "COMPLETED" { return nil } short-circuit — that was the buggy pattern this whole stack replaces (a stale wake-up could match it and silently no-op the wrong pass).
  • Drops the run-ID parameter from TaskCompletedCallback entirely (ParentRunID no longer exists on TaskRecord — confirmed unused once addressing is by workflow ID + step ID, since the engine never uses Continue-As-New).

Testing

  • I have tested this change locally
  • I have added tests that prove my fix is effective or that my feature works
  • I have tested edge cases
  • All existing tests pass

completion_test.go covers the persist-then-wake ordering and the already-resumed (ErrActivationNotPending) success path.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have checked that there are no merge conflicts

Related Issues

N/A

Screenshots/Demo

N/A

Additional Notes

Step 7 of 8 in the taskflow-guarded-writes stack — see #237 for why this stack exists. Base: feat/taskflow-guarded-writes-5-wake-up.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: e0f4aa3c-601f-479f-8fdd-af13c423db1c

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lokewate lokewate changed the title feat(taskflow)!: write the task's completion before waking the parent feat(taskflow)!: write the task's completion before waking the parent (7/8) Sep 28, 2026
@lokewate
lokewate added this pull request to stack #245 September 28, 2026 07:05
@lokewate lokewate mentioned this pull request Sep 28, 2026
Open
11 of 17 tasks
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from e643352 to 96ed4b0 Compare September 28, 2026 08:15
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from 96ed4b0 to 3202f41 Compare September 28, 2026 08:33
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from 3202f41 to c7a8657 Compare September 28, 2026 08:35
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from c7a8657 to c19f1eb Compare September 28, 2026 09:01
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from c19f1eb to 3c2bc0b Compare September 28, 2026 09:28
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from 3c2bc0b to 3686373 Compare September 28, 2026 09:37
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from 3686373 to 3c5b6c4 Compare September 30, 2026 02:26
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from 3c5b6c4 to 0801d84 Compare September 30, 2026 02:32
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from 0801d84 to 6d568ab Compare September 30, 2026 04:54
@lokewate
lokewate marked this pull request as ready for review September 30, 2026 04:54
@lokewate @claude
feat(taskflow)!: write the task's completion before waking the parent
baf56a2 
HandleTaskCompletion woke the parent workflow first and only then saved
COMPLETED, guarded by "already COMPLETED means done". A crash between the
two left the parent awake and the task not completed, and a retry then
called the parent's CompleteStep a second time, which fails because the
parent step is no longer pending.

The end of a top-level workflow now takes the next value of the step
counter and reports it (WorkflowCompletion.Seq). HandleTaskCompletion
writes COMPLETED at that seq first, guarded so it only matches if no
later step exists, and then wakes the parent. Each half can be repeated:
a completed row no longer short-circuits a retry, and a parent step that
is no longer pending means an earlier attempt already woke it, which
counts as success.

Also drop TaskRecord.ParentRunID and the run parameter from
TaskCompletedCallback: the parent workflow, like every workflow this
engine runs, never has more than one run (no Continue-As-New, no
workflow-level retry, no ID reuse — see the field's new doc comment), so
"" always correctly addresses whichever run is current. This is the same
reasoning CompleteTaskStep's wake-up already applies on the child side;
this commit is what makes the parent side consistent with it, since it's
the one rewriting how the parent is woken.

Also rename TaskStore.SaveTask to InitTask: this is the commit that
removes its last non-init caller (HandleTaskCompletion's own write, now
CompleteTask), so from here on its only caller is StartTask, creating the
row. It's still an upsert, not insert-only — StartTask is a Temporal
Activity and can be retried — so InitTask must stay safe to call twice
for the same TaskID; the doc comment says so.

BREAKING CHANGE: WorkflowCompletionHandler takes a WorkflowCompletion
(workflow ID, seq, final variables) instead of two arguments, and
TaskManager.HandleTaskCompletion takes the WorkflowCompletion.
TaskCompletedCallback drops its run-ID parameter
(parentWorkflowID, parentStepID, finalVariables) and TaskRecord.ParentRunID
is removed. TaskCompletedCallback should return CompleteStep's error
unchanged. TaskStore.SaveTask is renamed to InitTask.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@lokewate
lokewate force-pushed the feat/taskflow-guarded-writes-6-completion branch from 6d568ab to baf56a2 Compare September 30, 2026 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Aravinda-HWK Aravinda-HWK Awaiting requested review from Aravinda-HWK Aravinda-HWK is a code owner
@mushrafmim mushrafmim Awaiting requested review from mushrafmim mushrafmim is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lokewate