Fix ErrStateMachineNotFound handling in HSM state replication #7032
Conversation
justinp-tt
changed the title
justinp-tt
changed the title
| // Based on 1 and 2, node should always be found here. | ||
| // The node may not be found if: | ||
| // 1. The state machine was deleted (e.g. terminal state cleanup) | ||
| // 2. We're missing events that created this node |
There was a problem hiding this comment.
That's not true based on the comment that you deleted.
Already done history resend if needed before,
// and node creation today always associated with an event
I would also clarify that creation and deletion are always associated with an event.
| tag.WorkflowID(mutableState.GetExecutionInfo().WorkflowId), | ||
| tag.WorkflowRunID(mutableState.GetExecutionInfo().OriginalExecutionRunId), | ||
| ) | ||
| return nil |
There was a problem hiding this comment.
nit: I'd add a sanity check that version history in the mutable state is > the one in the request (same as the one on L265. or just return that info from compareVersionHistory), and return an error otherwise.
There was a problem hiding this comment.
Don't we already check this in compareVersionHistory? In other words, an error will be returned by compareVersionHistory if the condition you mention exists, so we won't even get to the point of a ErrStateMachineNotFound error
What changed?
HSMStateReplicatorImpl.syncHSMNode()to handleErrStateMachineNotFoundgracefullyOriginalExecutionRunIdTestSyncHSM_StateMachineNotFoundto verify behaviorWhy?
After adding support for state deletion in terminal states in Nexus, nightly tests started failing when sync HSM tasks tried to replicate state machines that had been legitimately deleted. Since the deletion is intentional for terminal states, we should gracefully handle these cases by logging and continuing replication of other state machines.
How did you test it?
ErrStateMachineNotFoundPotential risks
ErrStateMachineNotFoundcould potentially mask other issues if the error occurs for unexpected reasonsDocumentation
No documentation changes required as this is an internal implementation detail handling error cases in the replication path.
Is hotfix candidate?
No - while this fixes test failures, it's not causing production issues that would warrant a hotfix.