From 0d6c817afaf248559e92962425acb2a8e998c222 Mon Sep 17 00:00:00 2001 From: tekkaya <86028633+tekkaya@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:42:54 -0700 Subject: [PATCH] Support starting multiple activities from a synchronous Nexus operation 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 gets the inbound Nexus request's 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 or a reused request ID -- an earlier attempt at this fix also gave bypass-path activity starts the invocation's ambient request ID (first raw, then via a per-call derived counter to avoid collisions between two starts sharing an activity ID), but both versions were dropped: correctly identifying a start as "the same call, redelivered" vs. "a new call that happens to land on the same ordinal position" requires assuming the handler reissues an identical sequence of calls on every retry, an assumption the SDK has no way to verify. Under non-determinism this can silently fail to dedup a genuinely redelivered start, reintroducing the very duplicate-execution bug the reuse was meant to prevent. sdk-python reached the same conclusion independently (temporalio/sdk-python#1722, "Only send request ID when in a backing nexus context when starting a SAA") and made the same choice: only the one guarded backing start reuses the Nexus task's request ID; every bypass-path start gets its own fresh, uncorrelated one. This change aligns Java with that. 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. It also covers the same-activity-ID restart pattern from sdk-python's regression test: starting a fresh run with an already-used activity ID within one invocation must not resolve to the stale, already-completed run. RootActivityClientInvokerTest covers the ambient-links-but-fresh-request-ID case, the outside-Nexus-context case, and two bypass-path starts for the same activity ID getting distinct request IDs, at the unit level. --- .../client/RootActivityClientInvoker.java | 36 ++- .../nexus/InternalNexusOperationContext.java | 16 + .../internal/nexus/NexusTaskHandlerImpl.java | 3 + .../client/RootActivityClientInvokerTest.java | 63 +++- .../nexus/ActivityOperationLinkingTest.java | 295 ++++++++++++++++++ 5 files changed, 401 insertions(+), 12 deletions(-) create mode 100644 temporal-sdk/src/test/java/io/temporal/workflow/nexus/ActivityOperationLinkingTest.java diff --git a/temporal-sdk/src/main/java/io/temporal/internal/client/RootActivityClientInvoker.java b/temporal-sdk/src/main/java/io/temporal/internal/client/RootActivityClientInvoker.java index 225228e6a2..db75ea5a94 100644 --- a/temporal-sdk/src/main/java/io/temporal/internal/client/RootActivityClientInvoker.java +++ b/temporal-sdk/src/main/java/io/temporal/internal/client/RootActivityClientInvoker.java @@ -64,14 +64,16 @@ public StartActivityOutput startActivity(StartActivityInput input) { NexusOperationMetadata nexusOperationMetadata = nexusContext == null ? null : nexusContext.getNexusOperationMetadata(); + String requestId = + nexusOperationMetadata != null + ? nexusOperationMetadata.requestId + : UUID.randomUUID().toString(); + StartActivityExecutionRequest.Builder request = StartActivityExecutionRequest.newBuilder() .setNamespace(clientOptions.getNamespace()) .setIdentity(clientOptions.getIdentity()) - .setRequestId( - nexusOperationMetadata == null - ? UUID.randomUUID().toString() - : nexusOperationMetadata.requestId) + .setRequestId(requestId) .setActivityId(options.getId()) .setActivityType(ActivityType.newBuilder().setName(input.getActivityType()).build()) .setTaskQueue(TaskQueue.newBuilder().setName(options.getTaskQueue()).build()) @@ -121,14 +123,28 @@ public StartActivityOutput startActivity(StartActivityInput input) { io.temporal.api.common.v1.Header grpcHeader = HeaderUtils.toHeaderGrpc(input.getHeader(), null); request.setHeader(grpcHeader); - if (nexusOperationMetadata != null) { - List protoLinks = nexusContext.getRequestLinks(); + List protoLinks = Collections.emptyList(); + if (nexusContext != null) { + // Every activity start made on this operation-handler thread gets the inbound links, + // including starts through a raw ActivityClient -- this is deliberately broader than + // nexusOperationMetadata below, since links carry no completion semantics, unlike a + // completion callback. attach_request_id is set unconditionally too, mirroring on-conflict + // handling for every activity start; it's only meaningful for the one guarded call, whose + // request ID is the stable, reused one (see the requestId derivation above) -- a bypass-path + // start's fresh, uncorrelated ID makes the flag a no-op for it, which is harmless. + protoLinks = nexusContext.getRequestLinks(); request.addAllLinks(protoLinks); - request.setOnConflictOptions( + io.temporal.api.common.v1.OnConflictOptions.Builder onConflictOptions = io.temporal.api.common.v1.OnConflictOptions.newBuilder() .setAttachRequestId(true) - .setAttachLinks(true) - .setAttachCompletionCallbacks(true)); + .setAttachLinks(true); + if (nexusOperationMetadata != null) { + onConflictOptions.setAttachCompletionCallbacks(true); + } + request.setOnConflictOptions(onConflictOptions); + } + + if (nexusOperationMetadata != null) { // Generate the operation token from the user-supplied activity ID and namespace so the // dual OPERATION_ID + OPERATION_TOKEN headers can be injected before the start RPC fires. try { @@ -168,7 +184,7 @@ public StartActivityOutput startActivity(StartActivityInput input) { throw e; } - if (nexusOperationMetadata != null && response.hasLink()) { + if (nexusContext != null && response.hasLink()) { nexusContext.addResponseLink(response.getLink()); } diff --git a/temporal-sdk/src/main/java/io/temporal/internal/nexus/InternalNexusOperationContext.java b/temporal-sdk/src/main/java/io/temporal/internal/nexus/InternalNexusOperationContext.java index 97ab4f5ed2..3c5a6b0af8 100644 --- a/temporal-sdk/src/main/java/io/temporal/internal/nexus/InternalNexusOperationContext.java +++ b/temporal-sdk/src/main/java/io/temporal/internal/nexus/InternalNexusOperationContext.java @@ -27,6 +27,12 @@ public class InternalNexusOperationContext { // workflow client can attach them to the outgoing requests it issues (e.g. signal, // signalWithStart) via the request's links field. private List requestLinks = Collections.emptyList(); + // The inbound Nexus task's request ID, captured at the task-handler boundary and available to + // clients executing on the operation-handler thread. RootActivityClientInvoker reuses it for + // redelivery-safe activity-start deduplication. It is deliberately independent of + // nexusOperationMetadata, which is scoped to the single backing start because it carries + // completion-callback semantics. + private String requestId; // Links returned by outbound RPCs the operation handler issues (such as // SignalWorkflowExecutionResponse.link or SignalWithStartWorkflowExecutionResponse.signal_link). // One entry per outbound RPC that returned a link. Drained @@ -106,6 +112,16 @@ public void setRequestLinks(List links) { return Collections.unmodifiableList(requestLinks); } + /** Set the request ID of the inbound Nexus task, ambient for the whole invocation. */ + public void setRequestId(String requestId) { + this.requestId = requestId; + } + + /** The inbound Nexus task's request ID; {@code null} if not set. */ + public String getRequestId() { + return requestId; + } + public void setStartWorkflowResponseLink(Link link) { this.startWorkflowResponseLink = link; } diff --git a/temporal-sdk/src/main/java/io/temporal/internal/nexus/NexusTaskHandlerImpl.java b/temporal-sdk/src/main/java/io/temporal/internal/nexus/NexusTaskHandlerImpl.java index 4d40183c27..5ef945ec79 100644 --- a/temporal-sdk/src/main/java/io/temporal/internal/nexus/NexusTaskHandlerImpl.java +++ b/temporal-sdk/src/main/java/io/temporal/internal/nexus/NexusTaskHandlerImpl.java @@ -313,6 +313,9 @@ private StartOperationResponse handleStartOperation( } }); CurrentNexusOperationContext.get().setRequestLinks(inboundCommonLinks); + // Ambient for the whole operation-handler invocation, independent of NexusOperationMetadata. + // see InternalNexusOperationContext.requestId. + CurrentNexusOperationContext.get().setRequestId(task.getRequestId()); HandlerInputContent.Builder input = HandlerInputContent.newBuilder().setDataStream(task.getPayload().toByteString().newInput()); diff --git a/temporal-sdk/src/test/java/io/temporal/internal/client/RootActivityClientInvokerTest.java b/temporal-sdk/src/test/java/io/temporal/internal/client/RootActivityClientInvokerTest.java index 16ba8b7f36..f775e5695a 100644 --- a/temporal-sdk/src/test/java/io/temporal/internal/client/RootActivityClientInvokerTest.java +++ b/temporal-sdk/src/test/java/io/temporal/internal/client/RootActivityClientInvokerTest.java @@ -2,6 +2,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -22,6 +23,7 @@ import java.time.Duration; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; import org.junit.After; import org.junit.Assert; @@ -132,8 +134,65 @@ public void nexusMetadataWithEmptyCallbackUrlOmitsCompletionCallback() { } @Test - public void nexusContextWithoutMetadataStartsOrdinaryActivity() { - nexusContext.setRequestLinks(Collections.singletonList(workflowEventLink())); + public void nexusContextWithoutMetadataGetsAmbientLinksButFreshRequestIdAndNoCallback() { + Link link = workflowEventLink(); + nexusContext.setRequestLinks(Collections.singletonList(link)); + nexusContext.setRequestId("ambient-nexus-request-id"); + + invoker.startActivity(newStartActivityInput()); + + ArgumentCaptor captor = + ArgumentCaptor.forClass(StartActivityExecutionRequest.class); + verify(genericClient).startActivity(captor.capture()); + StartActivityExecutionRequest request = captor.getValue(); + Assert.assertFalse(request.getRequestId().isEmpty()); + Assert.assertNotEquals("ambient-nexus-request-id", request.getRequestId()); + Assert.assertEquals(Collections.singletonList(link), request.getLinksList()); + Assert.assertEquals(0, request.getCompletionCallbacksCount()); + Assert.assertTrue(request.getOnConflictOptions().getAttachRequestId()); + Assert.assertTrue(request.getOnConflictOptions().getAttachLinks()); + Assert.assertFalse(request.getOnConflictOptions().getAttachCompletionCallbacks()); + Assert.assertEquals(Collections.singletonList(activityLink()), nexusContext.getResponseLinks()); + } + + @Test + public void twoStartsForSameActivityIdGetDistinctFreshRequestIds() { + nexusContext.setRequestId("ambient-nexus-request-id"); + + invoker.startActivity(newStartActivityInput()); + invoker.startActivity(newStartActivityInput()); + + ArgumentCaptor captor = + ArgumentCaptor.forClass(StartActivityExecutionRequest.class); + verify(genericClient, times(2)).startActivity(captor.capture()); + List requests = captor.getAllValues(); + String firstRequestId = requests.get(0).getRequestId(); + String secondRequestId = requests.get(1).getRequestId(); + Assert.assertNotEquals(firstRequestId, secondRequestId); + Assert.assertNotEquals("ambient-nexus-request-id", firstRequestId); + Assert.assertNotEquals("ambient-nexus-request-id", secondRequestId); + } + + @Test + public void nexusContextWithoutAmbientStateStartsOrdinaryActivity() { + invoker.startActivity(newStartActivityInput()); + + ArgumentCaptor captor = + ArgumentCaptor.forClass(StartActivityExecutionRequest.class); + verify(genericClient).startActivity(captor.capture()); + StartActivityExecutionRequest request = captor.getValue(); + Assert.assertFalse(request.getRequestId().isEmpty()); + Assert.assertEquals(0, request.getLinksCount()); + Assert.assertEquals(0, request.getCompletionCallbacksCount()); + Assert.assertTrue(request.getOnConflictOptions().getAttachRequestId()); + Assert.assertTrue(request.getOnConflictOptions().getAttachLinks()); + Assert.assertFalse(request.getOnConflictOptions().getAttachCompletionCallbacks()); + Assert.assertEquals(Collections.singletonList(activityLink()), nexusContext.getResponseLinks()); + } + + @Test + public void outsideNexusContextStartsOrdinaryActivity() { + CurrentNexusOperationContext.unset(); invoker.startActivity(newStartActivityInput()); diff --git a/temporal-sdk/src/test/java/io/temporal/workflow/nexus/ActivityOperationLinkingTest.java b/temporal-sdk/src/test/java/io/temporal/workflow/nexus/ActivityOperationLinkingTest.java new file mode 100644 index 0000000000..80117eed08 --- /dev/null +++ b/temporal-sdk/src/test/java/io/temporal/workflow/nexus/ActivityOperationLinkingTest.java @@ -0,0 +1,295 @@ +package io.temporal.workflow.nexus; + +import static io.temporal.internal.common.WorkflowExecutionUtils.getEventOfType; +import static org.junit.Assume.assumeTrue; + +import io.nexusrpc.Operation; +import io.nexusrpc.Service; +import io.nexusrpc.handler.OperationHandler; +import io.nexusrpc.handler.OperationImpl; +import io.nexusrpc.handler.ServiceImpl; +import io.temporal.activity.ActivityInterface; +import io.temporal.activity.ActivityMethod; +import io.temporal.api.common.v1.Link; +import io.temporal.api.enums.v1.EventType; +import io.temporal.api.history.v1.History; +import io.temporal.api.history.v1.HistoryEvent; +import io.temporal.client.ActivityClient; +import io.temporal.client.ActivityClientOptions; +import io.temporal.client.ActivityExecutionDescription; +import io.temporal.client.ActivityHandle; +import io.temporal.client.StartActivityOptions; +import io.temporal.client.WorkflowStub; +import io.temporal.nexus.Nexus; +import io.temporal.testing.internal.SDKTestWorkflowRule; +import io.temporal.workflow.NexusOperationOptions; +import io.temporal.workflow.NexusServiceOptions; +import io.temporal.workflow.Workflow; +import io.temporal.workflow.WorkflowInterface; +import io.temporal.workflow.WorkflowMethod; +import io.temporal.workflow.shared.TestNexusServices; +import io.temporal.workflow.shared.TestWorkflows; +import java.time.Duration; +import java.util.HashSet; +import java.util.Set; +import java.util.UUID; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.Test; + +/** + * Verifies link propagation with activities when a synchronous Nexus operation handler starts more + * than one activity via a raw {@link ActivityClient} obtained from {@link + * Nexus#getOperationContext()}. + * + *
    + *
  • Forward direction: each activity's own record links back to the caller's {@code + * NexusOperationScheduled} event. + *
  • Backward direction: both activities' completions land as response links on the caller's + * single {@code NexusOperationCompleted} event. + *
+ * + *

Requires a real server; the in-process test server does not implement {@code + * StartActivityExecution} (see {@link AsyncActivityOperationTest}, which has the same gate). + */ +public class ActivityOperationLinkingTest { + + @Rule + public SDKTestWorkflowRule testWorkflowRule = + SDKTestWorkflowRule.newBuilder() + .setWorkflowTypes(TestNexus.class, TestNexusRestart.class) + .setActivityImplementations(new TestActivityImpl()) + .setNexusServiceImplementation( + new TestNexusServiceImpl(), new TestNexusRestartServiceImpl()) + .build(); + + @BeforeClass + public static void requireExternalService() { + assumeTrue( + "standalone-activity Nexus links require a real server", + SDKTestWorkflowRule.useExternalService); + } + + @Test + public void testTwoActivitiesBothLinkToOperation() { + String input = "world-" + UUID.randomUUID(); + TestWorkflows.TestWorkflow1 workflowStub = + testWorkflowRule.newWorkflowStubTimeoutOptions(TestWorkflows.TestWorkflow1.class); + String result = workflowStub.execute(input); + Assert.assertEquals("hello " + input + "-a|hello " + input + "-b", result); + + String callerWorkflowId = WorkflowStub.fromTyped(workflowStub).getExecution().getWorkflowId(); + History callerHistory = + testWorkflowRule.getWorkflowClient().fetchHistory(callerWorkflowId).getHistory(); + + // Backward direction: both activities' completions must land on the caller's single + // NexusOperationCompleted event as response links, not just the guarded/first one. + HistoryEvent completed = + getEventOfType(callerHistory, EventType.EVENT_TYPE_NEXUS_OPERATION_COMPLETED); + Assert.assertNotNull("expected a NexusOperationCompleted event", completed); + Assert.assertEquals( + "expected one response link per activity", 2, completed.getLinksCount()); + Set linkedActivityIds = new HashSet<>(); + for (int i = 0; i < completed.getLinksCount(); i++) { + Link.Activity activityLink = completed.getLinks(i).getActivity(); + Assert.assertNotNull("expected an Activity-typed response link", activityLink); + linkedActivityIds.add(activityLink.getActivityId()); + } + Assert.assertTrue(linkedActivityIds.contains("act-" + input + "-a")); + Assert.assertTrue(linkedActivityIds.contains("act-" + input + "-b")); + + // Forward direction: each activity's own record links back to the caller's + // NexusOperationScheduled event, not just the guarded/first one. + ActivityClient activityClient = + ActivityClient.newInstance( + testWorkflowRule.getWorkflowServiceStubs(), + ActivityClientOptions.newBuilder().setNamespace(SDKTestWorkflowRule.NAMESPACE).build()); + for (String suffix : new String[] {"a", "b"}) { + String activityId = "act-" + input + "-" + suffix; + ActivityExecutionDescription description = + activityClient.getHandle(activityId, null).describe(); + Assert.assertTrue( + "expected at least one link on activity " + activityId, + description.getRawInfo().getLinksCount() >= 1); + Link.WorkflowEvent forwardLink = description.getRawInfo().getLinks(0).getWorkflowEvent(); + Assert.assertNotNull( + "expected a WorkflowEvent-typed forward link on activity " + activityId, forwardLink); + Assert.assertEquals(callerWorkflowId, forwardLink.getWorkflowId()); + Assert.assertEquals( + EventType.EVENT_TYPE_NEXUS_OPERATION_SCHEDULED, forwardLink.getEventRef().getEventType()); + } + } + + @Test + public void testSameActivityIdRestartedWithinOneInvocationStartsANewRun() { + String input = "restart-" + UUID.randomUUID(); + RestartWorkflow workflowStub = + testWorkflowRule.newWorkflowStubTimeoutOptions(RestartWorkflow.class); + String result = workflowStub.execute(input); + + // If the two starts had collided, the second result would equal the first instead of being + // distinct. + Assert.assertEquals("hello " + input + "-1|hello " + input + "-2", result); + } + + public static class TestNexus implements TestWorkflows.TestWorkflow1 { + @Override + public String execute(String input) { + NexusServiceOptions serviceOptions = + NexusServiceOptions.newBuilder() + .setOperationOptions( + NexusOperationOptions.newBuilder() + .setScheduleToCloseTimeout(Duration.ofSeconds(30)) + .build()) + .build(); + TestNexusServices.TestNexusService1 stub = + Workflow.newNexusServiceStub(TestNexusServices.TestNexusService1.class, serviceOptions); + return stub.operation(input); + } + } + + @ActivityInterface + public interface TestActivity { + @ActivityMethod + String process(String input); + } + + public static class TestActivityImpl implements TestActivity { + @Override + public String process(String input) { + return "hello " + input; + } + } + + /** + * Starts two activities inline via a raw {@link ActivityClient} obtained from {@link + * Nexus#getOperationContext()} instead of {@code TemporalOperationHandler}'s single-guarded-call + * {@code TemporalNexusClient} -- the only way to start more than one activity synchronously in + * one Nexus operation invocation. + */ + @ServiceImpl(service = TestNexusServices.TestNexusService1.class) + public class TestNexusServiceImpl { + @OperationImpl + public OperationHandler operation() { + return OperationHandler.sync( + (ctx, details, input) -> { + ActivityClient activityClient = + ActivityClient.newInstance( + Nexus.getOperationContext().getWorkflowClient().getWorkflowServiceStubs(), + ActivityClientOptions.newBuilder() + .setNamespace(Nexus.getOperationContext().getInfo().getNamespace()) + .build()); + String taskQueue = Nexus.getOperationContext().getInfo().getTaskQueue(); + + ActivityHandle first = + activityClient.start( + TestActivity.class, + TestActivity::process, + StartActivityOptions.newBuilder() + .setId("act-" + input + "-a") + .setTaskQueue(taskQueue) + .setStartToCloseTimeout(Duration.ofSeconds(10)) + .build(), + input + "-a"); + ActivityHandle second = + activityClient.start( + TestActivity.class, + TestActivity::process, + StartActivityOptions.newBuilder() + .setId("act-" + input + "-b") + .setTaskQueue(taskQueue) + .setStartToCloseTimeout(Duration.ofSeconds(10)) + .build(), + input + "-b"); + return first.getResult() + "|" + second.getResult(); + }); + } + } + + @WorkflowInterface + public interface RestartWorkflow { + @WorkflowMethod + String execute(String arg); + } + + @Service + public interface RestartNexusService { + @Operation + String operation(String input); + } + + public static class TestNexusRestart implements RestartWorkflow { + @Override + public String execute(String input) { + NexusServiceOptions serviceOptions = + NexusServiceOptions.newBuilder() + .setOperationOptions( + NexusOperationOptions.newBuilder() + .setScheduleToCloseTimeout(Duration.ofSeconds(30)) + .build()) + .build(); + RestartNexusService stub = + Workflow.newNexusServiceStub(RestartNexusService.class, serviceOptions); + return stub.operation(input); + } + } + + /** + * Starts an activity with a given ID via a raw {@link ActivityClient}, waits for it to complete, + * then starts a *second*, fresh run using the *same* activity ID. + */ + @ServiceImpl(service = RestartNexusService.class) + public class TestNexusRestartServiceImpl { + @OperationImpl + public OperationHandler operation() { + return OperationHandler.sync( + (ctx, details, input) -> { + ActivityClient activityClient = + ActivityClient.newInstance( + Nexus.getOperationContext().getWorkflowClient().getWorkflowServiceStubs(), + ActivityClientOptions.newBuilder() + .setNamespace(Nexus.getOperationContext().getInfo().getNamespace()) + .build()); + String taskQueue = Nexus.getOperationContext().getInfo().getTaskQueue(); + String activityId = "restart-act-" + input; + + ActivityHandle first = + activityClient.start( + TestActivity.class, + TestActivity::process, + StartActivityOptions.newBuilder() + .setId(activityId) + .setTaskQueue(taskQueue) + .setStartToCloseTimeout(Duration.ofSeconds(10)) + .build(), + input + "-1"); + String firstRunId = first.getActivityRunId(); + String firstResult = first.getResult(); + + ActivityHandle second = + activityClient.start( + TestActivity.class, + TestActivity::process, + StartActivityOptions.newBuilder() + .setId(activityId) + .setTaskQueue(taskQueue) + .setStartToCloseTimeout(Duration.ofSeconds(10)) + .build(), + input + "-2"); + String secondRunId = second.getActivityRunId(); + String secondResult = second.getResult(); + + if (firstRunId != null && firstRunId.equals(secondRunId)) { + // The two starts collided into a single run instead of the second one starting a + // fresh run -- exactly the bug this test guards against. + throw new IllegalStateException( + "expected the second start to create a new run, but it reused runId " + + firstRunId); + } + + return firstResult + "|" + secondResult; + }); + } + } +}