Make Nexus bypass-path signal/signalWithStart request IDs redelivery-safe without colliding - #3050
Closed
tekkaya wants to merge 1 commit into
Closed
Make Nexus bypass-path signal/signalWithStart request IDs redelivery-safe without colliding#3050tekkaya wants to merge 1 commit into
tekkaya wants to merge 1 commit into
Conversation
…safe without colliding RootWorkflowClientInvoker.signal/signalWithStart always generate a fresh UUID for the outgoing request_id, even when issued from inside a Nexus operation handler. That means a redelivered Nexus task can apply the same signal twice: the retried handler re-issues the call with a brand-new random ID, so the server has nothing to dedup against. The naive fix -- reuse the ambient Nexus request ID the same way RootActivityClientInvoker.startActivity already does for bypass-path activity starts -- is unsafe here. Confirmed by reading the server's dedup code directly (service/history/api/signalworkflow/api.go and signalwithstartworkflow/signal_with_start_workflow.go, both keying off mutableState.IsSignalRequested/AddSignalRequested in mutable_state_impl.go): request_id on Signal(WithStart)WorkflowExecutionRequest is a pure dedup key with no awareness of signal name, payload, or target. Unlike activities, which already carry their own unique activity_id so sharing one ambient request ID across multiple starts in one invocation never collides, a signal has no such identifier. Reusing the same raw ambient ID for two different signal-class calls to the same workflow in one invocation would make the server treat the second as a duplicate of the first and silently drop it -- never delivered, no error. InternalNexusOperationContext.nextSignalRequestId() derives a per-call ID instead: ambientRequestId + "-" + N, where N is a per-invocation counter. This keeps the Nth signal-class call's ID stable across a redelivery (each redelivery attempt gets a fresh InternalNexusOperationContext but the same ambient request ID, and the handler is expected to reissue the same sequence of calls), while giving distinct calls within one invocation distinct IDs so they can't collide. RootWorkflowClientInvokerTest covers: ID derived-but-not-verbatim for both signal and signalWithStart; two calls (including a mixed signal + signalWithStart pair) in one invocation getting distinct IDs; redelivery stability across two separate context instances sharing the same ambient ID; and the existing fallback-to-fresh-UUID behavior outside a Nexus context or with no ambient ID set.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
RootWorkflowClientInvoker.signal/signalWithStartnow derive a redelivery-saferequest_idwhenissued from inside a Nexus operation handler, via a new
InternalNexusOperationContext.nextSignalRequestId().Why?
Stacked on #3048, which made bypass-path activity starts from a synchronous Nexus operation handler
redelivery-safe by reusing an ambient request ID.
signal()/signalWithStart()have the same gap —they always generate a fresh random
request_id, so a redelivered Nexus task can apply the samesignal twice.
The naive fix (reuse the ambient ID verbatim, like the activity fix does) is unsafe for signals. I
confirmed by reading the server's dedup code directly:
signalworkflow/api.go#L45and
signalwithstartworkflow/signal_with_start_workflow.go#L333both short-circuit purely on
mutableState.IsSignalRequested(requestID),keyed only on the raw request ID — no signal name, no payload, no target. Activities are safe to
share one ambient ID across multiple starts in one invocation because each has its own unique
activity_id; signals have no such per-call identifier. Reusing the same ambient ID for twodifferent signal-class calls to the same workflow in one invocation would make the server treat the
second as a duplicate of the first and silently drop it — never delivered, no error.
nextSignalRequestId()derivesambientRequestId + "-" + Ninstead, whereNis a per-invocationcounter. This keeps the Nth signal-class call's ID stable across a Nexus task redelivery (same
ambient ID, same call ordinal, assuming the handler reissues the same sequence of calls on retry —
the same determinism assumption the activity fix already relies on), while giving distinct calls
within one invocation distinct IDs so they can't collide.
Checklist
Closes: N/A — tracked as a follow-up finding from review of Support starting multiple activities from a synchronous Nexus operation handler #3048
How was this tested:
RootWorkflowClientInvokerTest(unit, mockedGenericWorkflowClient) covers: ID derived-but-not-verbatim for both
signalandsignalWithStart; two calls (including a mixed signal +signalWithStart pair) in one invocation getting distinct IDs; redelivery stability across two
separate context instances sharing the same ambient ID; and the existing fallback-to-fresh-UUID
behavior outside a Nexus context or with no ambient ID set.
./gradlew :temporal-sdk:test --tests "io.temporal.internal.client.*"passes (28 tests). No functional/useExternalServicetest — thisis a unit-level concern, fully verifiable with mocks, unlike the activity link-forwarding behavior
in Support starting multiple activities from a synchronous Nexus operation handler #3048 which needed a real server.
Any docs updates needed?
No.