Fairness Keys & Weights - #2633
Merged
Merged
Conversation
Contributor
|
You need to update the CLI to the pre release version https://github.com/temporalio/sdk-java/blob/master/.github/workflows/ci.yml#L77 |
Quinn-With-Two-Ns
approved these changes
Aug 19, 2025
tekkaya
added a commit
that referenced
this pull request
Aug 31, 2026
…on handler TemporalNexusClient guards its own activity/workflow/update start to at most one per operation invocation, so a synchronous Nexus operation handler that starts two or more activities inline has to bypass it and use a raw ActivityClient obtained from Nexus.getOperationContext() instead. That bypass path got no request links, because RootActivityClientInvoker.startActivity derived link attachment, on-conflict dedup, and completion-callback attachment all from the same NexusOperationMetadata, which only the guarded TemporalNexusClient call ever sets. Every activity start made during the invocation now reuses the inbound Nexus task's request ID and gets its inbound links, regardless of which client object issued it. NexusOperationMetadata keeps its narrow, one-shot scope and remains the only thing that can attach a completion callback, since only the guarded start should complete the Nexus operation. When metadata is present its own request ID still takes precedence, so the guarded start's identity never depends on the ambient value also having been set. Reusing one ambient ID across every start in an invocation is a deliberate, known tradeoff: a handler that starts a fresh run under an activity ID it already used earlier in the same invocation can have that start incorrectly resolve to the stale run instead of creating a new one. Two narrower, redelivery-aware alternatives were tried and dropped -- raw ambient-ID reuse scoped to only the guarded call, then a per-call ID derived from each start's ordinal position -- because both require assuming the handler reissues an identical sequence of calls on every retry, an assumption the SDK has no way to verify. This change instead matches sdk-go's approved fix (temporalnexus/temporal_operation.go, PR #2633) and sdk-python's current behavior (temporalio/nexus/_operation_context.py): every activity start in a Nexus context reuses the ambient request ID unconditionally. sdk-python's own attempt at the narrower, backing-call-only scoping (PR #1722) was closed without merging for the same reason. ActivityOperationLinkingTest (functional, requires a real server) drives a synchronous handler that starts two bypass-path activities and asserts both the forward link (each activity's own ActivityExecutionInfo) and the backward links (both activities' completions landing on the caller's single NexusOperationCompleted event), the same way SignalOperationLinkingTest already does for signals. RootActivityClientInvokerTest covers metadata's request ID taking precedence over the ambient one, the ambient-links-and-request-ID-without-metadata case, the outside-Nexus-context case, and two bypass-path starts in one invocation sharing the ambient request ID.
tekkaya
added a commit
that referenced
this pull request
Aug 31, 2026
…on handler TemporalNexusClient guards its own activity/workflow/update start to at most one per operation invocation, so a synchronous Nexus operation handler that starts two or more activities inline has to bypass it and use a raw ActivityClient obtained from Nexus.getOperationContext() instead. That bypass path got no request links, because RootActivityClientInvoker.startActivity derived link attachment, on-conflict dedup, and completion-callback attachment all from the same NexusOperationMetadata, which only the guarded TemporalNexusClient call ever sets. Every activity start made during the invocation now reuses the inbound Nexus task's request ID and gets its inbound links, regardless of which client object issued it. NexusOperationMetadata keeps its narrow, one-shot scope and remains the only thing that can attach a completion callback, since only the guarded start should complete the Nexus operation. When metadata is present its own request ID still takes precedence, so the guarded start's identity never depends on the ambient value also having been set. Reusing one ambient ID across every start in an invocation is a deliberate, known tradeoff: a handler that starts a fresh run under an activity ID it already used earlier in the same invocation can have that start incorrectly resolve to the stale run instead of creating a new one. Two narrower, redelivery-aware alternatives were tried and dropped -- raw ambient-ID reuse scoped to only the guarded call, then a per-call ID derived from each start's ordinal position -- because both require assuming the handler reissues an identical sequence of calls on every retry, an assumption the SDK has no way to verify. This change instead matches sdk-go's approved fix (temporalnexus/temporal_operation.go, PR #2633) and sdk-python's current behavior (temporalio/nexus/_operation_context.py): every activity start in a Nexus context reuses the ambient request ID unconditionally. sdk-python's own attempt at the narrower, backing-call-only scoping (PR #1722) was closed without merging for the same reason. ActivityOperationLinkingTest (functional, requires a real server) drives a synchronous handler that starts two bypass-path activities and asserts both the forward link (each activity's own ActivityExecutionInfo) and the backward links (both activities' completions landing on the caller's single NexusOperationCompleted event), the same way SignalOperationLinkingTest already does for signals. RootActivityClientInvokerTest covers metadata's request ID taking precedence over the ambient one, the ambient-links-and-request-ID-without-metadata case, the outside-Nexus-context case, and two bypass-path starts in one invocation sharing the ambient request ID.
tekkaya
added a commit
that referenced
this pull request
Aug 31, 2026
The server rejects a StartActivityExecutionRequest whose OnConflictOptions sets attach_request_id when the request carries neither a link nor a completion callback (chasm/lib/activity/validator.go's validateOnConflictOptions: "attach_request_id requires at least one completion callback or link"). The previous code set attach_request_id and attach_links unconditionally whenever any Nexus context existed, regardless of whether the inbound task actually had links -- a bypass-path activity start issued during an invocation whose inbound Nexus task carries no links would send exactly that invalid combination and get rejected. This wasn't caught before because the existing unit tests mock the client and never exercise real server-side validation. OnConflictOptions is now only set when there's something to attach, and each flag reflects what the request actually carries, matching sdk-go's identical gating in its own fix (temporalnexus/temporal_operation.go, PR #2633). This also fixes the guarded (metadata-backed) path: it previously set attach_completion_callbacks based on whether metadata was present rather than whether a callback URL was actually set, so a guarded start with an empty callback URL and no links would hit the same rejection. RootActivityClientInvokerTest: flipped the assertion in nexusMetadataWithEmptyCallbackUrlOmitsCompletionCallback (attach_completion_ callbacks now correctly reflects the absence of a real callback), rewrote nexusContextWithoutAmbientStateStartsOrdinaryActivity to assert OnConflictOptions is entirely absent, and added metadataWithEmptyCallbackUrlAndNoLinksOmitsOnConflictOptions covering the previously-untested guarded-call variant of the same bug.
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
2 participants
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
Added fairness keys/weights to priority
Why?
New feature
Checklist
Closes
How was this tested:
Added test
Any docs updates needed?