Skip to content

Wire request_id through CallbackInfo for linking - #851

Open
chrsmith wants to merge 2 commits into
feature/worker-callbacksfrom
chrsmith/add-request-id-for-linking
Open

Wire request_id through CallbackInfo for linking#851
chrsmith wants to merge 2 commits into
feature/worker-callbacksfrom
chrsmith/add-request-id-for-linking

Conversation

@chrsmith

@chrsmith chrsmith commented Aug 17, 2026

Copy link
Copy Markdown

⚠️ This is to be merged into the feature/worker-callbacks branch, and not main. Only after the feature is complete will that branch be rebased and merged into main.


This PR makes three changes, all so that resources spanwed from the invocation of a worker callback can be linked correctly.

(1) Remove the Link_NexusOperationCallback variant with a more general Link_Callback proto

Previously we were scoping the feature to only be applicable for SANO callbacks. But if we are going to support worker callbacks for any async operation, having a general link type (that uses the existing Execution proto) will avoid needing to create additional link variants in the future.

(2) Add a callbackpb.CallbackInfo::request_id field

This type is used in the Describe- operations for standalone Activities and standalone Nexus operations. Without it, there would be no way to determine which completion callback is being referred to. (Instead, we couldn't be any more accurate than to have the link point to "one of these N" callbacks.)

(3) Add workflowpb.CallbackInfo::{request_id, result}

The workflowpb namespace forked rather than embedded the callbackpb.CallbackInfo message. The changes here add the missing fields, so that DescribeWorkflowExecution can disambiguate callbacks as well. (In addition to carrying the result of those callbacks.)

Why?

With these changes, the server will be able to properly cross-link resources spawned from completion callbacks.

On the Caller-side, any resources spawned from the completion callbacks would be available on the commonpb.Callback::links field. (*)

query GetSpawnedResourceLinks(workflowID: string {
  DescribeWorkflowExecution(workflowID) {
    completion_callbacks {
      callback {
        links
      }
    }
  }
}

(*) Only the resources initially created from the worker callback invocation will be present. e.g. the Workflow that backs an asynchronous Nexus handler. It would not contain links for any subsequent resources created.

On the Handler-side, a single Link_Callback would be supplied to the Nexus handler receiving the worker callback. (This would be in the form of a nexuspb.Link.)

Breaking changes

Yes, this PR contains breaking proto changes. However, in the context of a PR into a long-lived feature branch for an unshipped feature this is safe. (The protos haven't ever been persisted by a production service.)

Server PR

It isn't out yet, but will be stacked on top of this:
temporalio/temporal#11589

@chrsmith
chrsmith requested review from a team as code owners August 17, 2026 16:56
Comment thread temporal/api/common/v1/message.proto Outdated
// Source execution the callback was attached to.
Execution execution = 1;
// Request ID used for the callback's delivery.
string request_id = 2;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider creating a "callback ID" as fully supported thing. The ambiguity where the request ID can be shared across multiple callbacks feels a little awkward.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strong +1 to this. We just need a way to uniquely identify a worker callback within the scope of an execution, and having an entirely server-side generated ID removes any potential confusion.

Moreover, having it be an entirely Temporal-managed CallbackInfo::callback_id and not a user-editable Callback::id simplifies things even more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no user-editable callback::id. I'd be more than okay using the request ID as the callback ID since it is a unique identifier as long as we document it.

@chrsmith
chrsmith force-pushed the chrsmith/add-request-id-for-linking branch from 79aa66f to e5678ad Compare August 17, 2026 19:45
Comment thread temporal/api/callback/v1/message.proto Outdated
// If the state is BLOCKED, blocked reason provides additional information.
string blocked_reason = 8;

// The Request ID used when the Callback was delivered. Used as an idempotency key in case multiple deliveries

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would decouple the user provided request ID from the one the system generates. The start request is not the same as the callback delivery request and those should have different IDs. I would be confused if I saw the same request ID provided in a log for different purposes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am concerned that this will be confused with the request ID of the start request used to attach this callback. Document the semantics as we did here:

// Server-generated request ID used as an idempotency token when submitting start requests to
// the handler. Distinct from the request_id in StartNexusOperationRequest, which is the
// caller-side idempotency key for the StartNexusOperation RPC itself.
string request_id = 21;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for linking to the comment, I agree that's exactly how we'd want to define it.

Comment thread temporal/api/common/v1/message.proto Outdated
// Source execution the callback was attached to.
Execution execution = 1;
// Request ID used for the callback's delivery.
string request_id = 2;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no user-editable callback::id. I'd be more than okay using the request ID as the callback ID since it is a unique identifier as long as we document it.

// A Nexus operation execution archetype. This is reserved for standalone Nexus operations.
EXECUTION_TYPE_NEXUS = 3;
// An update workflow execution archtype.
EXECUTION_TYPE_UPDATE_WORKFLOW = 4;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an "execution", it's a component within an execution.

EXECUTION_TYPE_ACTIVITY = 2;
} No newline at end of file
// A Nexus operation execution archetype. This is reserved for standalone Nexus operations.
EXECUTION_TYPE_NEXUS = 3;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EXECUTION_TYPE_NEXUS = 3;
EXECUTION_TYPE_NEXUS_OPERATION = 3;

@chrsmith chrsmith changed the title Wire request_id through CallbackInfo for linking Wire callback_id through CallbackInfo for linking Aug 17, 2026
@chrsmith chrsmith changed the title Wire callback_id through CallbackInfo for linking Wire request_id through CallbackInfo for linking Aug 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants