Fix TemporalReportedProblems SA application for buffered events #8769
Conversation
| previousTotalFailures := m.ms.executionInfo.WorkflowTaskAttemptsSinceLastSuccess + m.ms.executionInfo.WorkflowTaskAttempt | ||
| m.ms.executionInfo.WorkflowTaskAttemptsSinceLastSuccess = previousTotalFailures |
There was a problem hiding this comment.
Can you update WorkflowTaskAttemptsSinceLastSuccess every time there is a WFT failure instead of only doing it when flushing buffered events? That seems more intuitive behavior to keep this attribute up-to-date.
| // Start sending signals every second in a goroutine | ||
| signalDone := make(chan struct{}) | ||
| go func() { | ||
| defer close(signalDone) |
There was a problem hiding this comment.
You could instead signal for a side effect in the workflow and immediately panic. No need to add concurrency to this test.
| ) | ||
| } | ||
|
|
||
| func (s *WorkflowTaskReportedProblemsReplicationSuite) TestWFTFailureReportedProblems_SetAndClear_FailAfterActivity() { |
There was a problem hiding this comment.
Shouldn't this be also testing the buffered signal case?
There was a problem hiding this comment.
Duplicated the wrong test here, removing this.
| signalChan := workflow.GetSignalChannel(ctx, "test-signal") | ||
|
|
||
| var signalValue string | ||
| signalChan.Receive(ctx, &signalValue) |
There was a problem hiding this comment.
You don't really need to handle the signals in the workflow, it doesn't add value in the test and prevents your workflow task from failing because you never reach the line where it panics.
| // This will create buffered. | ||
| if s.shouldFail.Load() { | ||
| // Signal ourselves to create buffered events | ||
| _ = workflow.SignalExternalWorkflow(ctx, workflow.GetInfo(ctx).WorkflowExecution.ID, "", "test-signal", "self-signal") |
There was a problem hiding this comment.
This doesn't create buffered events, you need to do it from a workflow.SideEffect: https://pkg.go.dev/go.temporal.io/sdk/workflow#hdr-SideEffect_API
There was a problem hiding this comment.
You will need to use a temporal client to issue the signal, and make sure you don't ignore errors, we should be able to tell that the signal was successfully submitted.
|
Semgrep found 1 No explicit |
spkane31
changed the title
What changed?
When buffered events are applied, like when applying signals after workflow task failures occur, preserve the workflow task attempt so the
TemporalReportedProblemssearch attribute is still properly added.Why?
Without this change the
TemporalReportedProblemssearch attribute will never be added if a workflow is consistently queried.How did you test it?
Potential risks
Minimal, this adds a new int to the workflow task execution, but it's only used in one place.