Add link field to propagate backlink in signal and signal-with-start responses#761
Conversation
7abc353 to
5c99d1b
Compare
| // Link to be associated with the WorkflowExecutionSignaled event. | ||
| // Added on the response to propagate the backlink. | ||
| temporal.api.common.v1.Link signal_link = 3; | ||
| // Workflow start link to be associated with the WorkflowExecutionStarted event. |
There was a problem hiding this comment.
Would this be propagated back to the caller if the workflow was already started? Seems like that would be confusing. I would call out the behavior in the docstring.
There was a problem hiding this comment.
IMO it should behave like the workflow event link on a normal start workflow, so link to the start if the workflow is started, link to the workflow options update if a callback was attached after the workflow was started.
There was a problem hiding this comment.
commenting for posterity: chatted w/ both offline and we decided to add this in a later PR
5c99d1b to
168fcbb
Compare
bergundy
left a comment
There was a problem hiding this comment.
Approved with a small comment.
168fcbb to
c7d4e90
Compare
c7d4e90 to
99f97eb
Compare
99f97eb to
7111b0b
Compare
| // The request ID of the Signal request, used by the server to attach this to | ||
| // the correct Event ID when generating link. | ||
| string request_id = 7; |
There was a problem hiding this comment.
commenting for posterity: discussed offline w/ server, this change (post-server-approval) is ok, see this comment for more details
## What changed? ### **High level** With temporalio/api#761 to add the linking on the Signal and Signal-with-Start responses, This PR adds logic from the server that: * Adds `requestID` from Signal and Signal-with-Start requests to the CHASM workflow tree under a new map field `IncomingSignals`, and event store, so these requestIDs stay in buffer * Return a backlink in the response that references the `requestID` * On buffer flush to the DB transaction, attach these `requestID` to a concrete `eventID`, which would allow users to later know which event correlated w/ this request. We will wire the concrete event ID to the signal request IDs stored in the workflow component CHASM tree (`IncomingSignals` map) > [!NOTE] > Feature is gated behind a new dynamicconfig `EnableCHASMSignalBacklinks`, which implicitly is only checked if `EnableChasm` is enabled. ## 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? - [ ] built - [ ] run locally and tested manually - [ ] covered by existing tests - [ ] added new unit test(s) - [x] added new functional test(s) In functional tests, I augmented existing tests for Signal and Signal-with-Start to: * Ensure that backlink is returned via the responses * Later use `DescribeWorkflow` to ensure that we get a concrete EventID (mapped when buffer flushed) * Multiple signals with the same `requestID` gets de-duped ``` $ go test ./tests/ -run TestLinksTestSuite ok go.temporal.io/server/tests 1.486s ``` ``` $ go test ./tests/ -run 'TestNexusWorkflowTestSuite' -count=1 ok go.temporal.io/server/tests 4.714s ``` ## Potential risks Need to test end-to-end to see that the link shows up correctly in the Web UI. Feature is gated behind dynamicconfig since it requires CHASM-based workflow to be enabled.
What changed?
Added
linkfields onSignalWorkflowExecutionResponseandSignalWithStartWorkflowExecutionRequest. I think a singular link should suffice but if reviewers feel otherwise (especially w.r.t.SignalWithStartWorkflowExecutionRequest) let me know.Why?
To propagate backlinks on signal and signal-with-start executions back to the caller.
Breaking changes
Not a breaking change (unused field)
Server PR
temporalio/temporal#9897