Propagate backlinks on Signal and Signal-with-Start responses#9897
Propagate backlinks on Signal and Signal-with-Start responses#9897long-nt-tran wants to merge 1 commit intotemporalio:mainfrom
Conversation
589224e to
4c69cc2
Compare
go.mod
Outdated
|
|
||
| replace go.temporal.io/api => github.com/long-nt-tran/api-go v0.0.0-20260410154440-a741268f8bab |
There was a problem hiding this comment.
only pointing to my fork for CI tests, will wait for temporalio/api#761 and subsequent api-go codegen to be merged and then remove these before I merge this PR
bergundy
left a comment
There was a problem hiding this comment.
Here's what's missing:
- Adding to the map that tracks request ID to event ID
- A functional test that verifies that request IDs for all of these requests are added to the map
- Ensure that the workflow start link is only populated if an execution was started
984be28 to
51b9594
Compare
51b9594 to
52c0da3
Compare
| modernc.org/memory v1.11.0 // indirect | ||
| ) | ||
|
|
||
| replace go.temporal.io/api => github.com/long-nt-tran/api-go v0.0.0-20260413175257-fd07f9923cd0 |
There was a problem hiding this comment.
temp, will delete once api PR temporalio/api#761 merges
|
Addressed feedback, I think the cleanest way to have the requestID be in the mutable state is to have it on the event is to have it be a new field in Also put all testing at functional test level so we can end-to-end assert on request ID presence + concrete event ID resolution -- edited summary to reflect this. |
That makes sense to me because the signal could be cherry picked into a new history branch on conflict resolution and reset and you'll want to carry over the same request ID when that happens. |
| }, | ||
| }, | ||
| }, | ||
| Link: api.GenerateStartedEventRefLink("", wfKey.WorkflowID, wfKey.RunID), |
There was a problem hiding this comment.
Namespace is empty here. Looks like the bug was pre-existing.
| attr.GetInput(), | ||
| attr.GetIdentity(), | ||
| attr.GetHeader(), | ||
| "", // no request ID available when reapplying from history |
There was a problem hiding this comment.
Might as well add the request ID to the tests here since it is expected to be populated on new events.
| if requestID != "" { | ||
| ms.AttachRequestID(requestID, enumspb.EVENT_TYPE_WORKFLOW_EXECUTION_SIGNALED, common.BufferedEventID) | ||
| } |
There was a problem hiding this comment.
Not quite seeing the value of attaching the buffered event ID here. It's supposed to be a temporary ID.
Also not quite sure how the request ID mapping works with signal-with-start where the same request ID maps to two history events. We shouldn't override the request ID of the WorkflowExecutionStarted event.
I think we may need to extend the proto to allow a single request ID to reference two separate events.
What changed?
With these PRs to add the linking on the Signal and Signal-with-Start responses, This PR adds logic from the server that:
requestIDfrom Signal and Signal-with-Start requests to the mutable state + event store so these requestIDs stay in bufferrequestIDrequestIDto a concreteeventID, which would allow users to later know which event correlated w/ this requestIn functional tests, I augmented existing tests for Signal and Signal-with-Start to:
DescribeWorkflowto ensure that we get a concrete EventID (mapped when buffer flushed)requestIDgets de-duppedNote
Doing it this way from discussion w/ @Quinn-With-Two-Ns since signals are buffered and we will not have a concrete event ID.
Why?
This will enable the caller of the signal to have a backlink to the cross-namespace signal invoked, which will become more relevant for Nexus SDK ergonomics.
How did you test it?
Functional tests
Potential risks
Need to test end-to-end to see that the link shows up correctly in the Web UI.