Skip to content

Fix backlinks in Signal[WithStart]Workflow when called from workflows#10362

Open
chrsmith wants to merge 2 commits into
mainfrom
chrsmith/add-backlinks-for-startwithsignal
Open

Fix backlinks in Signal[WithStart]Workflow when called from workflows#10362
chrsmith wants to merge 2 commits into
mainfrom
chrsmith/add-backlinks-for-startwithsignal

Conversation

@chrsmith
Copy link
Copy Markdown
Contributor

What changed?

Story time!

This PR just adds the final step, and adds tests for verifying SignalWithStart calls are linked correctly.

The only functional changes are in chasm/lib/workflow/nexus_service.go:

  1. We weren't setting the SignalLink property in workflowservice.SignalWithStartWorkflowExecutionResponse. We now do.
  2. We were manually building the commonpb.Link rather than using the SignalLink returned from the History service's response. I'm assuming trusting the History service is returning a valid SignalLink of the Link_WorkflowEvent variant. (Whether it is a EventID or RequestIDRef is immaterial.)

Why?

Fixes a gap in the new codepath for calls to SignalWithStart adds tests.

Questions

  • Is it safe to hard-code EventIDs like this? s.Equal(int64(3), gotReqInfo.GetEventId())

How did you test it?

  • built
  • run locally and tested manually
  • covered by existing tests -- Unknown. I would guess there are "SignalWithStart backlink tests" somewhere, but I didn't find any after a cursory look.
  • added new unit test(s)
  • added new functional test(s)

Potential risks

I do not know. Please be on the lookout for bad assumptions!

@chrsmith chrsmith requested review from a team as code owners May 22, 2026 17:46
@chrsmith chrsmith force-pushed the chrsmith/add-backlinks-for-startwithsignal branch from e67c46a to 055ee89 Compare May 22, 2026 17:57
Copy link
Copy Markdown
Member

@bergundy bergundy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM but can you please verify all of these cases via tests?

  1. Send a signal-with-start request, no existing workflow, get back a link and the mapping is resolved to an event that is not buffered.
  2. Send a signal-with-start request, existing workflow, no running workflow task, get back a link and the mapping is resolved to an event that is not buffered.
  3. Send a signal-with-start request, existing workflow, running workflow task, get back a link and the mapping is resolved to an event that is buffered, complete the workflow task, and verify that the mapping is resolved to an event that is not buffered.

To simulate a running workflow task send the request between PollWorkflowTaskQueue and RespondWorkflowTaskCompleted.

@chrsmith chrsmith force-pushed the chrsmith/add-backlinks-for-startwithsignal branch 2 times, most recently from 28751da to c266bf4 Compare May 27, 2026 19:48
@chrsmith chrsmith force-pushed the chrsmith/add-backlinks-for-startwithsignal branch from c266bf4 to f2e575d Compare May 27, 2026 19:53
@chrsmith
Copy link
Copy Markdown
Contributor Author

chrsmith commented May 27, 2026

@bergundy , @long-nt-tran PTAL.

After chatting with you folks, I just did some refactoring and reused the existing test scenarios we had in tests/links_tets.go. However, unifying the { SignalWorkflow, SignalWithStart[ExistingWorkflow], SignalWithStart[NoExistingWorkflow] } into a single test runner was a bridge too far.

So the refactoring went like this:

1.) Introduce a new signalWorkflowTest to eliminate some boilerplate.

New surface area:

type signalWorkflowTest struct {}
func newSignalWorkflowTest(env *testcore.TestEnv, s *LinksSuite) *signalWorkflowTest
func (...) startTargetWorkflow(ctx context.Context) startTargetWorkflowOutput
func (...) startTargetWorkflowWithWorkflowID(ctx context.Context, workflowID string) startTargetWorkflowOutput
func (...) signalWorkflow(ctx context.Context, targetWorkflowID, requestID string) *workflowservice.SignalWorkflowExecutionResponse
func (...) signalWithStartWorkflow(ctx context.Context, targetWorkflowID, requestID string) *workflowservice.SignalWithStartWorkflowExecutionResponse

2.) Refactor the relevant tests to use that new piece of infrastructure. And then for each of those tests, create a SignalWithStart- variant that covers the existing and non-existing workflow case.

 -- PASS: TestLinksTestSuite (0.00s)
    --- PASS: TestLinksTestSuite/TestSignalWorkflowExecution_LinksAttachedToEvent (0.12s)
    --- PASS: TestLinksTestSuite/TestTerminateWorkflow_LinksAttachedToEvent (0.13s)
    --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_LinksAttachedToRelevantEvents (0.13s)
-               ^--- existing test using SignalWithStart, kept as-is.
+    --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_LinksAttachedToEvent (0.00s)
+        --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_LinksAttachedToEvent/SignalStartsNewWorkflow (0.13s)
+        --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_LinksAttachedToEvent/SignalExistingWorkflow (0.13s)
     --- PASS: TestLinksTestSuite/TestRequestCancelWorkflow_LinksAttachedToEvent (0.13s)
+    --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_BufferedDuringWorkflowTask (0.00s)
+        --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_BufferedDuringWorkflowTask/SignalStartsNewWorkflow (0.12s)
+        --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_BufferedDuringWorkflowTask/SignalExistingWorkflow (0.13s)
     --- PASS: TestLinksTestSuite/TestSignalWorkflowExecution_BufferedDuringWorkflowTask (0.13s)
+    --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_BacklinkSurvivesReset (0.00s)
+        --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_BacklinkSurvivesReset/SignalExistingWorkflow (0.14s)
+        --- PASS: TestLinksTestSuite/TestSignalWithStartWorkflowExecution_BacklinkSurvivesReset/SignalStartsNewWorkflow (0.14s)
    --- PASS: TestLinksTestSuite/TestSignalWorkflowExecution_BacklinkSurvivesReset (0.14s)```

@chrsmith chrsmith requested a review from bergundy May 27, 2026 20:03
@chrsmith chrsmith force-pushed the chrsmith/add-backlinks-for-startwithsignal branch 3 times, most recently from c737a58 to ffb4b67 Compare May 27, 2026 22:33
@chrsmith chrsmith force-pushed the chrsmith/add-backlinks-for-startwithsignal branch from ffb4b67 to 06c72ca Compare May 27, 2026 22:42
@chrsmith
Copy link
Copy Markdown
Contributor Author

As it turns out I was calling SignalWithStart the wrong way. I was using the existing frontend API endpoint, and not going through the new system Nexus handler.

Unfortunately making an SDK NexusClient targeting the system Nexus handler isn't possible because of checks baked-into the code. So I had to crib a round about way to invoke the system Nexus handler from another test.

Anyways signalWithStartWorkflow.signalWithStartWorkflow(...) function is kinda crazy, but does what we want it to.

However, there were a couple of quirks with the new SignalWithStart endpoint worth calling out:

  • There isn't any way to supply the RequestID. The Nexus handler rejects requests that set the field, and it will be set at runtime. This means that there isn't any idempotent way to send the signal... but I think that might be OK and by-design.
  • Similarly, the Nexus handler doesn't allow you to specify any Links in the SignalWithStart request either. I'm not sure of the ramifications of that, but presumably that is OK and by-design as well?

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.

2 participants