Make standalone activity completion callback attachment idempotent after closure

[Quinn-With-Two-Ns]

What changed?

Fixing a bug identified by AI. Make standalone activity completion callback attachment idempotent after closure

Why?

So we respect request ID and use it properly as an idempotency key

How did you test it?

• built
• run locally and tested manually
• covered by existing tests
• added new unit test(s)
• added new functional test(s)

---

Note

Low Risk
Small behavioral fix in standalone activity callback attachment with a dedicated functional test; no auth or broad API surface changes.

Overview
Standalone activity completion callbacks now treat requestId as an idempotency key even when the activity is already closed.

addCompletionCallbacks checks existing callbacks for a matching requestId before rejecting closed activities, matching the pattern already used for attachLinks. Retries of StartActivityExecution with USE_EXISTING and the same requestId succeed without duplicating callbacks or returning FailedPrecondition.

A functional test covers completing an activity, then retrying start with the same requestId and completion callback after closure.

^{Reviewed by Cursor Bugbot for commit d2d70b9. Bugbot is set up for automated code reviews on this repo. Configure here.}

[Evanthx]

LGTM

[chrsmith]

This was something I had noticed earlier as well when adding completion callbacks to SANOs, and is on the ACT team's radar. (See https://app.notion.com/p/temporalio/SAA-bug-claims-3a08fc5677388009a690cff570aeaa45?source=copy_link "🐛 FT C9", and just communicate that you've taken care of this.)

[chrsmith]

What you have here is OK for now, but I think it's the wrong approach. (But could be convinced otherwise.)

The root problem is seen later in this function:

The problem is that we are attaching multiple Callback objects each using the same requestID (matching what is coming in from the frontend request).

		// requestID (unique per API call) + idx (position within the request) ensures unique,idempotent callback IDs.
		id := fmt.Sprintf("%s-%d", requestID, idx)
		callbackObj := callback.NewCallback(requestID, registrationTime, &callbackspb.CallbackState{}, chasmCB)
		a.Callbacks[id] = chasm.NewComponentField(ctx, callbackObj)

So if multiple callbacks are attached, that are to be sent to the same destination, the handler-side cannot differentiate "multiple callback invocations" from "retried delivery attempt".

I think the correct behavior would be to make two changes:

1. Promote the fmt.Sprintf("%s-%d", requestID, idx) to a local function, e.g. completionCallbackID(requestID string, index int) string.
2. Use that to see if a callback has already been attached to the Activity object's Callbacks map. (Rather than loading each Callback individually.)

This is how I've implemented this behavior in my PR to add completion callbacks to SANOs. It's not perfect, since it gets the wrong behavior if you attach a different number of completion callbacks across two requests with the same request ID.

	// Attaching is atomic, so the presence of the first key means this request already attached all of
	// its callbacks. See the note above on why this precedes the closed check.
	if _, ok := o.Callbacks[completionCallbackID(requestID, 0)]; ok {
		return nil
	}

And, if you want to go for the gold, a 3rd change would be to fix the bug regarding reusing the same Callback::RequestID. And on line 478 have:

- id := fmt.Sprintf("%s-%d", requestID, idx)
- callbackObj := callback.NewCallback(requestID, registrationTime, &callbackspb.CallbackState{}, chasmCB)
+ id := completionCallbackID(requestID, idx)
+ callbackObj := callback.NewCallback(id, registrationTime, &callbackspb.CallbackState{}, chasmCB)
a.Callbacks[id] = chasm.NewComponentField(ctx, callbackObj)

[Quinn-With-Two-Ns]

Yeah this isn't perfect, but actually for a different reason I would argue. Really CHASM should be tracking the requestID per SAA run and deduplicating the entire request instead of us having to deduplicate specific field adds like we are doing now. There is a know issue for that and something the foundations team will hopefully start soon.

[bergundy]

Hmm... that's a good point. I see that WithRequestID is supposed to now dedupe on UpdateComponent. I have not tested this behavior though.

[bergundy]

Really what we want is to use UpdateWithStartExecution but @yycptt says it's not as efficient as separate start and update calls (where the state could be mutated between those two separate calls).

[Quinn-With-Two-Ns]

Yes there is a TODO to use UpdateWithStartExecution