Skip to content

API surface area for worker callbacks#836

Draft
chrsmith wants to merge 5 commits into
mainfrom
chrsmith/worker-callbacks-review
Draft

API surface area for worker callbacks#836
chrsmith wants to merge 5 commits into
mainfrom
chrsmith/worker-callbacks-review

Conversation

@chrsmith

Copy link
Copy Markdown

⚠️ This isn't meant to be merged any time soon. This is just part of the worker callbacks implementation. After the code review, I'll replace the feature/worker-callbacks branch with this.


What changed?

Adds the API surface area for the WIP worker callbacks feature.

Specifically, the changes are:

  1. Add a completion_callbacks field to StartNexusOperationExecutionRequest. (How worker callbacks will be sent to the server.)
  2. Introduce a new Worker variant of commonpb.Callback. (How worker callbacks will be encoded.)
  3. Add a new notificationservice/v1/request_response.proto to define the input/output shape for worker callbacks. (The payload that will be delivered to workers.)

Why?

Today it is cumbersome to durably invoke Nexus operations and execute logic based on their results. "Worker callbacks" is a capability being developed that would allow developers to attach a Nexus operation to a SANO. Later, when the SANO reaches a terminal state, the attached Nexus operation would be invoked.

Breaking changes

None.

Server PR

TBD. The current prototype (based on the feature/worker-callbacks branch in this repo) can be found at:
https://github.com/temporalio/temporal/compare/feature/worker-callbacks?expand=1

@bergundy bergundy left a comment

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.

You're missing a new link definition to a callback on an execution.

Comment thread temporal/api/common/v1/message.proto
Comment thread temporal/api/common/v1/message.proto Outdated
// Nexus task queue the Temporal worker is listening on.
//
// NOTE: This is not a temporal.api.taskqueue.v1.TaskQueue to avoid a circular dependency.
string taskqueue_name = 1;

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.

nit: we call this field task_queue everywhere, it will generate better across languages with the underscore. Rename to task_queue_name.

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.

Asking because I'm not sure: is it a problem that we aren't using the proper TaskQueue proto type here?

This is the full proto definition. We'd be missing out on the kind and normal_name of task queue. And just assuming it's TASK_QUEUE_KIND_NORMAL. (And never TASK_QUEUE_KIND_WORKER_COMMANDS or TASK_QUEUE_KIND_STICKY.)

Could that be a problem down the road?

Comment thread temporal/api/common/v1/message.proto Outdated
Comment thread temporal/api/notificationservice/v1/request_response.proto Outdated
temporal.api.sdk.v1.UserMetadata user_metadata = 16;

// Completion callbacks to be invoked once the Nexus operation reaches a terminal state.
repeated temporal.api.common.v1.Callback completion_callbacks = 17;

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.

You'll need to add the full attachment support that we have for request IDs and on conflict options for workflows and activities.

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.

Would you mind elaborating on what you mean by this? What do you mean by "full attachment" here?

I'm patterning this after the way we added completion callbacks to StartActivityExecutionRequest and StartWorkflowExecutionRequest. I see in both of those other request types there are id_conflict_policy / id_reuse_policy fields. But StartNexusOperationExecutionRequest has similar fields as well.

Are you saying that each individual commonpb.Callbacks should have its own (potentially user-supplied) callback_id ? And ID Conflict/Reuse policies supplied too? That doesn't make sense to me. Since if the SANO is accepted by Temporal, the callbacks should be accepted as well. (And I don't see any sort of UpdateNexusOperationExecution-type command, that would allow us to merge/update callbacks?)

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.

Let's sync up on this

@chrsmith
chrsmith force-pushed the chrsmith/worker-callbacks-review branch from 334ee1f to 1329a40 Compare July 21, 2026 20:39
@chrsmith

chrsmith commented Jul 21, 2026

Copy link
Copy Markdown
Author

@bergundy Thanks for the review, please take another look.

  • Expose a new CallbackExecutionInfo proto from DescribeNexusOperationExecutionResponse. (It's a subset of what was going to be added for Manual Completion in Add Standalone Callbacks (rebase) #774.)
  • Add a new Callback variant of Link.

Comment thread temporal/api/common/v1/message.proto Outdated
string request_id = 3;
}

string callback_id = 1;

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 callback_id to refer to in all of our models.

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.

A SANO operation could have multiple worker callbacks associated with it. So in order for the link to not be ambiguous, we need some sort of unique identifier for which callback on the SANO.

The user would be able to see those IDs on the workflowservice.CallbackExecutionInfo messages, returned from DescribeNexusOperationExecution.

Does that sound right? Or is it that we don't actually need to have callback_id here?

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.

That's what the request ID is for.

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.

I don't follow. Would you mind elaborating on that? ... in fact, what does NexusOperationCompletion::request_id mean in this context?

Would that be the request_id that was sent when we go and invoke the worker callback? (e.g. the nexuspb.StartOperationRequest::request_id that is sent to the worker?)

Comment thread temporal/api/common/v1/message.proto
Activity activity = 3;
NexusOperation nexus_operation = 4;
Workflow workflow = 5;
Callback callback = 6;

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.

Maybe just flatten the Callback into this variant? NexusOperationCompletionCallback? Or just NexusOperationCallback?

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.

I can be convinced otherwise, but I'm inclined to keep it as-is. Let me know if you disagree.

When we later support attaching worker callbacks to Workflows and Activities, it would be kind of lame to have unique kinds of links for those. e.g. WorkflowCompletionCallback and ActivityCompletionCallback. Do we want to treat those as three different kinds of Links or just a Link to a callback, which can be pointing to different resources?

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 works too.

Comment thread temporal/api/notificationservice/v1/request_response.proto Outdated
temporal.api.sdk.v1.UserMetadata user_metadata = 16;

// Completion callbacks to be invoked once the Nexus operation reaches a terminal state.
repeated temporal.api.common.v1.Callback completion_callbacks = 17;

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.

Let's sync up on this

Comment thread temporal/api/workflowservice/v1/message.proto Outdated
Comment thread temporal/api/workflowservice/v1/message.proto Outdated
@chrsmith
chrsmith force-pushed the chrsmith/worker-callbacks-review branch from 1a13720 to c176d77 Compare July 24, 2026 20:33
@chrsmith

Copy link
Copy Markdown
Author

Rebased and force-pushed. @bergundy PTAL. Though that last one re: "full attachment" will probably need a sync conversation, as I'm not sure what you're asking for there.

Here are the open questions/threads remaining:

Comment on lines +38 to +57
}

message CallbackExecutionOutcome {
oneof value {
// The callback completed successfully. (Which may include delivering a "failed" result successfully.)
google.protobuf.Empty success = 1;
// The failure if the callback was not able to complete successfully. e.g. timed out, received an
// unretriable error, etc.
temporal.api.failure.v1.Failure failure = 2;
}
}

message CallbackExecutionInfo {
// Identifier of the callback's execution. Unique within the containing namespace.
string callback_id = 1;

CallbackInfo info = 2;
CallbackExecutionOutcome outcome = 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
}
message CallbackExecutionOutcome {
oneof value {
// The callback completed successfully. (Which may include delivering a "failed" result successfully.)
google.protobuf.Empty success = 1;
// The failure if the callback was not able to complete successfully. e.g. timed out, received an
// unretriable error, etc.
temporal.api.failure.v1.Failure failure = 2;
}
}
message CallbackExecutionInfo {
// Identifier of the callback's execution. Unique within the containing namespace.
string callback_id = 1;
CallbackInfo info = 2;
CallbackExecutionOutcome outcome = 3;
}
// TODO document me
CallbackExecutionOutcome outcome = 9;
}
message CallbackExecutionOutcome {
oneof value {
// The callback completed successfully. (Which may include delivering a "failed" result successfully.)
google.protobuf.Empty success = 1;
// The failure if the callback was not able to complete successfully. e.g. timed out, received an
// unretriable error, etc.
temporal.api.failure.v1.Failure failure = 2;
}
}

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.

I wasn't sure how concerned we'd be with including the failure objects in CallbackInfo, so I created the separate -ExecutionInfo. But I'm happy to make the change to just add the -ExecutionOutcome as a field to CallbackInfo. (And existing callsites would just leave it blank, and maybe we'd wire through a bool include_outcome field in the request object or something.)

Comment thread temporal/api/common/v1/message.proto Outdated
string request_id = 3;
}

string callback_id = 1;

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.

That's what the request ID is for.

Activity activity = 3;
NexusOperation nexus_operation = 4;
Workflow workflow = 5;
Callback callback = 6;

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 works too.

message OnCompleteRequest {

// The outcome of the source operation.
message Outcome {

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.

Might be worth defining Outcome in common at this point.

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.

I tried, but it would require some more refactoring since that would lead to a circular dependency between commonpb and failurepb. Maybe we could expose some other or simpler error type in the Outcome::failure case. But the failurepb.Failure seems kinda useful in how it strongly types various categories of errors.


// Completion callbacks to be invoked once the Nexus operation reaches a terminal state.
// They will remain in the CALLBACK_STATE_STANDBY state until the Nexus operation is finished.
repeated temporal.api.callback.v1.CallbackExecutionInfo completion_callbacks = 7;

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
repeated temporal.api.callback.v1.CallbackExecutionInfo completion_callbacks = 7;
repeated temporal.api.nexusoperation.v1.CallbackInfo callbacks = 7;

And copy from

message CallbackInfo {

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.

Ack, will get to in the next rev. And having different CallbackInfo objects defined in different namespaces (e.g. /activity, /nexusoperation will sidestep some other problems we might have.)

Co-authored-by: Roey Berman <roey.berman@gmail.com>
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