Skip to content

fix(nexus): clear operation callbacks on 'abandon' cancellation. - #2485

Merged
cconstable merged 6 commits into
mainfrom
nexus/fix-abandon-double-set
Jul 28, 2026
Merged

fix(nexus): clear operation callbacks on 'abandon' cancellation.#2485
cconstable merged 6 commits into
mainfrom
nexus/fix-abandon-double-set

Conversation

@cconstable

@cconstable cconstable commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Was seeing a flakey test (TestAsyncOperationFromWorkflow_CancellationTypes/Abandon) and traced it back to a bug.

What was changed

  • Cancelling withNexusOperationCancellationTypeAbandon now correctly clears nexus callbacks.
  • Added a small helper for getting the nexus operation command.

Why?

When you cancel a workflow running a nexus operation with NexusOperationCancellationTypeAbandon, it operations futures directly but doesn't clear the startedCallback or completedCallback. Then, when a NexusOperationStarted (or completed) event is received, the stale callbacks fires and attempts to set an already resolved future which panics, which WFT fails.

Fixes #2495

Checklist

  • Added tests to verify that we clear callbacks.

@cconstable
cconstable requested a review from a team as a code owner July 23, 2026 21:30
@cconstable
cconstable marked this pull request as draft July 23, 2026 21:30
@cconstable
cconstable force-pushed the nexus/fix-abandon-double-set branch 6 times, most recently from d8d1427 to 00a803c Compare July 24, 2026 15:18
}

func (h *commandsHelper) getNexusOperationCommand(seq int64) commandStateMachine {
return h.getCommand(makeCommandID(commandTypeNexusOperation, strconv.FormatInt(seq, 10)))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a small helper since this manual formatting was repeated in a number of places.

Comment thread internal/workflow.go
if !executionFuture.IsReady() {
executionSettable.Set(nil, ErrCanceled)
}
wc.env.AbandonNexusOperation(seq)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fix. clears out the callbacks so we don't try to resolve an already resolved future.

@cconstable
cconstable marked this pull request as ready for review July 24, 2026 15:31
@Quinn-With-Two-Ns

Quinn-With-Two-Ns commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

We have the same problem for Child Workflows that already does handle this, is there a reason you solved this in a different way then Child Workflows?

if childWorkflow.handled {

@cconstable
cconstable force-pushed the nexus/fix-abandon-double-set branch from 2286b50 to f9333ad Compare July 27, 2026 16:12
@cconstable

Copy link
Copy Markdown
Contributor Author

@Quinn-With-Two-Ns I matched the existing nexus pattern of nilling out the callbacks:

func (wc *workflowEnvironmentImpl) RequestCancelNexusOperation(seq int64) {
    ...
    data.startedCallback = nil
}

The handled flag in child workflows looks like it means we hit a terminal state and we can skip any extra completions. I don't think this maps to nexus directly since nexus operations can complete without a started event. So I think the options are:

  1. follow the existing nexus pattern of nilling out the callbacks (what is in the PR)
  2. add a handled flag and some new logic to all the nexus lifecycle handlers to nil out the started callbacks after firing it (seems like too much scope for this PR)
  3. add a new abandoned flag and use it for this case to skip completion callbacks but now this nexus lifecycle handler will diverge from what the others are doing

I actually prefer the idea of having flags/enums for the state here but I feel like that should be it's own PR and would probably need more discussion. I could open another to make that change.

@cconstable
cconstable force-pushed the nexus/fix-abandon-double-set branch from f9333ad to 6873404 Compare July 27, 2026 18:24
@cconstable
cconstable enabled auto-merge (squash) July 28, 2026 15:37
@cconstable
cconstable merged commit fa3c7f1 into main Jul 28, 2026
38 checks passed
@cconstable
cconstable deleted the nexus/fix-abandon-double-set branch July 28, 2026 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] nexus start/complete callbacks not cleared on abandon

3 participants