Skip to content

Wire request_id through CallbackInfo for linking - #851

Merged
chrsmith merged 4 commits into
feature/worker-callbacksfrom
chrsmith/add-request-id-for-linking
Aug 19, 2026
Merged

Wire request_id through CallbackInfo for linking#851
chrsmith merged 4 commits into
feature/worker-callbacksfrom
chrsmith/add-request-id-for-linking

Conversation

@chrsmith

@chrsmithchrsmith 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. (*)

queryGetSpawnedResourceLinks(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 ownersAugust 17, 2026 16:56
Comment threadtemporal/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
chrsmithforce-pushed the chrsmith/add-request-id-for-linking branch from 79aa66f to e5678adCompareAugust 17, 2026 19:45
// 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.
stringrequest_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 threadtemporal/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.

Comment threadtemporal/api/enums/v1/common.proto Outdated
// 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.

Comment threadtemporal/api/enums/v1/common.proto Outdated
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;

@chrsmithchrsmith changed the title Wire request_id through CallbackInfo for linkingWire callback_id through CallbackInfo for linkingAug 17, 2026
@chrsmithchrsmith changed the title Wire callback_id through CallbackInfo for linkingWire request_id through CallbackInfo for linkingAug 17, 2026
@chrsmith
chrsmithforce-pushed the chrsmith/add-request-id-for-linking branch from 08c0da4 to 18f2186CompareAugust 18, 2026 17:22
@chrsmith

Copy link
Copy Markdown
Author

Addressed PR feedback. However, after the blueprint review we will scope worker callbacks to just SANO operations. So I removed the new fields added to workflowpb.CallbackInfo and avoided adding anything else to Link_Callback (which would have been required to support links to callbacks attached to workflow updates).

@chrsmith
chrsmithforce-pushed the chrsmith/add-request-id-for-linking branch from 18f2186 to 54d5276CompareAugust 18, 2026 17:26
@chrsmith

Copy link
Copy Markdown
Author

I got this all wired up server-side. Will merge so I can rebase things. It'll get another review when I send the feature/worker-callbacks branch out for review before merging into main.

@chrsmith
chrsmith merged commit 4190870 into feature/worker-callbacksAug 19, 2026
2 checks passed
@chrsmith
chrsmith deleted the chrsmith/add-request-id-for-linking branch August 19, 2026 16:50
chrsmith added a commit that referenced this pull request Aug 19, 2026
⚠️ 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](https://github.com/temporalio/api/blob/0066de621239ca9ddc6c976e091e27a6bc474752/temporal/api/common/v1/message.proto#L73-L77))
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. (*)
```graphql
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
Sign up for freeto 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

@chrsmith@bergundy@VegetarianOrc