-
Notifications
You must be signed in to change notification settings - Fork 1.3k
History: Expose and call SetContextMetadata in readonly operations #9240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
History: Expose and call SetContextMetadata in readonly operations #9240
Conversation
7497389 to
4221700
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
| } | ||
| } | ||
|
|
||
| if len(request.Commands) == 0 && len(request.Messages) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... do we have any action metric to emit if both are empty? Is workflow task heartbeat an action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this!
yycptt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need some tests ensuring those two apis handler have the context populated.
f2a2de6 to
c3b8c49
Compare
c3b8c49 to
a522a1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to change the existing tests in history_engine_tests.go?
| // Context metadata is automatically set during mutable state transaction close. For RespondWorkflowTaskCompleted | ||
| // with no commands (e.g., workflow task heartbeat or only readonly messages like `update.Rejection`), the transaction | ||
| // is never closed. We explicitly call SetContextMetadata here to ensure workflow metadata is populated in the context. | ||
| workflowLease.GetMutableState().SetContextMetadata(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, I don't think the mutable state you get from workflowLease.GetMutableState() will always be the latest mutable state because it's a separate copy in this workflowLease object, while the real one is in workflowLease.Context. Doesn't really matter for your case the can be confusing?
Why not use the ms variable instead and do it before the defer() method? similar to what you have in the Query api
What changed?
SetContextMetadata()public in MutableState interfaceMetadataKeyWorkflowType,MetadataKeyWorkflowTaskQueue)ContextHasMetadata()helper function to check if context has metadata support-
QueryWorkflow: Now explicitly callsSetContextMetadata()since queries never close transactions (readonly operation)-
RespondWorkflowTaskCompleted: CallsSetContextMetadata()when there are only messages (e.g., update rejections) and no commands, since these also don't close transactionsContextHasMetadata()helper andSetContextMetadata()functionalityWhy?
Context metadata (workflow type, task queue) is automatically populated when mutable state transactions close. However, readonly operations never close transactions, so the metadata was missing from their contexts.
This change ensures that successful readonly operations (queries, update rejections) also have workflow metadata populated in their contexts
How did you test it?
Note
Medium Risk
Touches core history API request paths and mutable state interfaces; while behavior is additive (context tagging), incorrect invocation or key usage could affect observability/metrics and tests across multiple workflows.
Overview
Ensures workflow context metadata (workflow type/task queue) is consistently populated even for readonly paths that don’t close mutable-state transactions.
This exposes
MutableState.SetContextMetadata(ctx)(renaming the internal helper), standardizes metadata keys viacontextutil.MetadataKeyWorkflowType/MetadataKeyWorkflowTaskQueue, addsContextHasMetadata()for debugging, and explicitly callsSetContextMetadatainQueryWorkflowand inRespondWorkflowTaskCompletedwhenCommandsis empty (e.g., update rejections / heartbeats). Tests are expanded to assert metadata presence across success and several error/edge cases.Written by Cursor Bugbot for commit 69c3140. This will update automatically on new commits. Configure here.