[lokewate]

Note

For reviewers, lines to review (blank lines not counted): code +93 / −16, plus 65 / −26 comment lines · tests +391 / −125, mostly the existing ExecuteTaskActivity mocks now returning a TaskResult · docs +19 / −19. Diff against #353.

Summary

A step that waits for a person (a form, an officer's review, a payment) returned ErrResultPending and held its Activity open until CompleteActivation closed it with CompleteActivityByID. The open Activity was the wait, so its StartToCloseTimeout had to cover the whole wait: 365 days. That hid two problems:

• A worker that died, was redeployed or OOM-killed while running a step's plugin went unnoticed until the 365 days ran out: the case sat stuck, nothing parked, nobody was told.
• A step left waiting for a year timed out, and the retry ran its plugin again (an EXTERNAL_REVIEW re-dispatched, a payment restarted).

The Activity now only runs the plugin, and the wait moves into the workflow:

1. ExecuteTaskActivity returns a TaskResult: the step's output, or Suspended when the handler returned ErrResultPending. Either way the Activity ends.
2. A suspended step's node waits in the workflow on a future, with no timeout, the same way a node parked for an admin waits for its resolution.
3. CompleteActivation sends the CompleteActivation Workflow Update, which settles that future with the step's output. The node then maps the output and moves on as before.

An Update rather than a signal, because callers need the answer: CompleteTaskStep treats a rejected completion as a stale submission, and HandleTaskCompletion treats it as "the parent was already woken". The Update's validator rejects a completion for a step that isn't waiting, and its handler takes the step out of the waiting set before anything else runs, so of several completions for one step exactly one is accepted and the rest get ErrActivationNotPending, the guarantee Temporal gave when completing an Activity. The Update has no ID of its own: Temporal would answer a second Update with the same ID with the first one's result, so a duplicate would succeed instead of being rejected.

A step is registered as waiting before its Activity starts, so a completion that arrives while the Activity is still finishing (a fast callback) is kept. It stops taking completions as soon as it completes, fails or is cancelled, before the node moves on.

With the wait out of the Activity, taskActivityTimeout drops from 365 days to 10 minutes: an attempt whose worker died, or whose call hangs, is retried after 10 minutes under the retry policy from #353. The handlers now get the Activity's context, so a call made with it is cancelled when the attempt times out.

Stacked on #353 (base fix/bound-task-retries): it uses that PR's taskRetryPolicy and timeout constants.

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

• workflow/completion.go (new): the CompleteActivation Update handler and validator, and expectCompletion, which registers a step as waiting.
• workflow/workflow.go: pendingCompletions, the handler registered at workflow start, and handleTaskNode waiting on the step's completion when the Activity reports Suspended. taskActivityTimeout is 10 minutes.
• workflow/activities.go: TaskResult, and ExecuteTaskActivity turning ErrResultPending into Suspended. Handlers get the Activity's context.
• workflow/manager.go: CompleteActivation sends the Update and maps a rejection, or a workflow that has ended or doesn't exist, to ErrActivationNotPending. TaskActivationHandler and WorkflowCompletionHandler take a context.Context.
• Tests: the 88 ExecuteTaskActivity mocks return a TaskResult (a taskOutput adapter wraps the function-valued ones). workflow/completion_test.go covers a completion carrying the step's output, a step waiting 400 days without its plugin running again, an unknown and a duplicate completion being rejected, a completion arriving before the Activity returns, and a synchronous step refusing a completion. manager_test.go covers the Update CompleteActivation sends and how its answers map.
• Docs: README.md, taskflow/README.md and taskflow/docs/integration-guide.md show the handlers with a context, and describe the wait.

Breaking:

• TaskActivationHandler is func(ctx context.Context, payload TaskPayload) (map[string]any, error) and WorkflowCompletionHandler is func(ctx context.Context, completion WorkflowCompletion) error. A host passes the context on, e.g. to StartTask(ctx, …).
• ExecuteTaskActivity returns TaskResult instead of map[string]any.
• CompleteActivation needs Workflow Updates on the Temporal server.
• Workflows already running when this deploys can't complete their waiting steps: their nodes wait on an Activity that is no longer completed.

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

go test -race ./... passes. The real-server tests in complete_activation_integration_test.go (skipped in CI) pass against temporal server start-dev (server 1.30.1): a late completion for an earlier run of a looping node, a step that never existed, a workflow that has ended, and a new test sending five completions for one step at once, of which exactly one is accepted. TestCompleteStepRejectsAnEarlierRunOfALoopingNode already failed on main (it expected the first TASK run's Seq to be 1, but the START node takes 1); it now checks that the later run's Seq is higher.

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

Related to LSFLK/lsf-govtech-tnsw#247 (a hung or crashed step attempt is retried within its timeout, and long waits don't rely on the Activity timeout).

Screenshots/Demo

N/A

Additional Notes

• Temporal server: Workflow Updates are generally available from server 1.25 and on by default; nsw-srilanka's compose runs 1.28.3. A deployment that turned them off (frontend.enableUpdateWorkflowExecution) must turn them back on.

• nsw-srilanka follows with a core bump and the two handlers taking the context they're given.

• This makes acting on a waiting step straightforward. A waiting step is now a wait inside the workflow, not an open Activity, so anything that should interrupt or accompany the wait becomes one more branch alongside the completion. Today the same change would mean cancelling an open Activity and dealing with its retries and late completions. For example:

  • Admin action on an active node: an admin can park, reassign, complete or abort a step that's still waiting, using the same resolution signal parked nodes use, without waiting for it to fail first.
  • SLA timers: a deadline is a timer racing the step's completion. When it fires, the workflow can send a notification and keep waiting, or escalate or complete the step itself.
  • Withdrawing a step: a step whose case is withdrawn can stop waiting cleanly, without a cancelled Activity left behind.

  None of these are in this PR; it only moves the wait so they can be built on it.

🤖 Generated with Claude Code

[coderabbitai]

Important

• 🔍 Trigger review

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

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 899aa58c-d79e-425e-a10a-260a1f837052

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[Aravinda-HWK]

Note for later: workflow definition versions don't cover engine code changes

This doesn't block the PR. We have no running production workflows yet, so nothing replays on the new code.

Once there are users, though, a change like this would break their running cases. On deploy, those cases replay their history on the new engine code: finished steps would decode their old map result into an empty TaskResult, and waiting steps would accept a completion that never wakes the node.

The versions we already have don't prevent this:

• WorkflowDefinition.Version and the manifest version (_manifest_version) cover changes to the definition (nodes, edges, mappings, templates). Those are already safe, because the definition is saved in each workflow's history.
• This PR changes the engine code (handleTaskNode, the Activity result type). A case started on definition v1 replays the same definition on the new code, so neither version number changes.

For engine changes like this once we have users, we should either:

• gate the change with workflow.GetVersion so old histories keep the old path:

  v := workflow.GetVersion(ctx, "wait-outside-activity", workflow.DefaultVersion, 1)
  if v == workflow.DefaultVersion {
      // old path
  } else {
      // new path
  }

[lokewate]

yes, after we get into prod, we need to use workflow.GetVersion for anything that will alter the reply.