Standalone Activity: preserve server-generated request IDs across restarts

[dandavison]

What changed?

When generating a request ID server-side, set it on the request struct before any cloning so that the mutation is re-used by all retries.

Why?

Without this, there is a bug, although I have not attempted to repro it:

1. Request arrives at Frontend without requestID
2. Inside retry interceptor loop, requestID is generated and set on a cloned copy
3. Request proceeds to history, starts the execution, but then some networking condition in the cell causes RetryableInterceptor not to receive the Ack (it sees a context expiry)
4. Frontend retries, generating a new request ID. But meanwhile the activity has completed. This would be rare, but technically possible.
5. The default reuse policy permits a second execution to be started. This would be a bug: the second start should have been prevented by the request ID. If the user's activity lacks idempotency protection it will lead to incorrectness in the user's systems.

How did you test it?

• built
• added weak new unit test(s) for the Start case.
• manually tested:

commit fa2476c0b235a665f8846e91c0052825317be4d7
Author: Dan Davison <dan.davison@temporal.io>
Date:   2 days ago

    Add functional test for request-ID stability across server-side retries
    
    Use a package-level atomic to fail StartActivityExecution once after the
    activity is created at history, triggering the RetryableInterceptor.
    Without the fix, the retry generates a new request ID and gets
    ActivityExecutionAlreadyStarted. With the fix, the retry reuses the
    same request ID and the dedup succeeds.

diff --git a/chasm/lib/activity/frontend.go b/chasm/lib/activity/frontend.go
index c013b17c2..2a7b96bad 100644
--- a/chasm/lib/activity/frontend.go
+++ b/chasm/lib/activity/frontend.go
@@ -2,6 +2,7 @@
 
 import (
 	"context"
+	"sync/atomic"
 
 	"github.com/google/uuid"
 	apiactivitypb "go.temporal.io/api/activity/v1" //nolint:importas
@@ -35,6 +36,10 @@ type FrontendHandler interface {
 
 var ErrStandaloneActivityDisabled = serviceerror.NewUnimplemented("Standalone activity is disabled")
 
+// TestStartFailOnce, when set to true, causes the next StartActivityExecution to return Unavailable
+// after the activity is created. It fires once (CAS to false).
+var TestStartFailOnce atomic.Bool
+
 type frontendHandler struct {
 	FrontendHandler
 	client            activitypb.ActivityServiceClient
@@ -100,6 +105,13 @@ func (h *frontendHandler) StartActivityExecution(ctx context.Context, req *workf
 		NamespaceId:     namespaceID.String(),
 		FrontendRequest: modifiedReq,
 	})
+	if err != nil {
+		return nil, err
+	}
+
+	if TestStartFailOnce.CompareAndSwap(true, false) {
+		return nil, serviceerror.NewUnavailable("test: injected failure after successful creation")
+	}
 
 	return resp.GetFrontendResponse(), err
 }
diff --git a/tests/standalone_activity_test.go b/tests/standalone_activity_test.go
index 6afc7b606..d5ded28c1 100644
--- a/tests/standalone_activity_test.go
+++ b/tests/standalone_activity_test.go
@@ -270,6 +270,36 @@ func (s *standaloneActivityTestSuite) TestIDConflictPolicy() {
 	})
 }
 
+func (s *standaloneActivityTestSuite) TestServerGeneratedRequestIDStableAcrossRetries() {
+	t := s.T()
+	ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second)
+	defer cancel()
+
+	activityID := testcore.RandomizeStr(t.Name())
+	taskQueue := testcore.RandomizeStr(t.Name())
+
+	// Make the handler fail once with a retryable error so the RetryableInterceptor retries.
+	activity.TestStartFailOnce.Store(true)
+
+	resp, err := s.FrontendClient().StartActivityExecution(ctx, &workflowservice.StartActivityExecutionRequest{
+		Namespace:    s.Namespace().String(),
+		ActivityId:   activityID,
+		ActivityType: s.tv.ActivityType(),
+		Identity:     s.tv.WorkerIdentity(),
+		Input:        defaultInput,
+		TaskQueue: &taskqueuepb.TaskQueue{
+			Name: taskQueue,
+		},
+		StartToCloseTimeout: durationpb.New(defaultStartToCloseTimeout),
+		// No RequestId — server generates one.
+	})
+	// With the fix, the retry uses the same request ID, so history recognizes it as a dedup
+	// and succeeds (with Started=false). Without the fix, the retry generates a new request ID
+	// and gets ActivityExecutionAlreadyStarted.
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+}
+
 func (s *standaloneActivityTestSuite) TestPollActivityTaskQueue() {
 	t := s.T()
 	ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second)

Potential risks

Could introduce incorrectness into Standalone Activity

---

Note

Medium Risk
Changes request ID generation semantics for standalone activity Start/Cancel/Terminate paths to improve deduplication across retries; risk is moderate because it touches request mutation behavior that affects idempotency and retry interactions.

Overview
Ensures standalone activity requests reuse a single RequestId across frontend retries by generating the server-side ID before cloning/mutating the request (so subsequent retry attempts see the same ID).

Removes the prior pre-mutation cloning for TerminateActivityExecution and RequestCancelActivityExecution request-ID population, and adds a unit test (frontend_test.go) asserting StartActivityExecution keeps a stable RequestId across multiple validateAndPopulateStartRequest calls.

^{Written by Cursor Bugbot for commit 885f60d. This will update automatically on new commits. Configure here.}

[stephanos]

fwiw, in my PR now I've moved this into the validation and normalization methods like this:

    if req.GetRequestId() == "" {
		req.RequestId = uuid.NewString()
	} else if len(req.GetRequestId()) > config.MaxIDLengthLimit() {
		return serviceerror.NewInvalidArgumentf("request_id exceeds length limit. Length=%d Limit=%d",
			len(req.GetRequestId()), config.MaxIDLengthLimit())
	}

[dandavison]

I've opened a pure refactoring PR to make SAA more consistent with SNO #9795

[bergundy]

Just FTR, the duplicates can easily happen if a client retries and doesn't specify a request ID. This makes it a bit worse.

[dandavison]

@stephanos I've added a non-mergeable manual repro test to commit history and to the PR description (which will become the commit message).

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.