Repository navigation
Conversation
SyncActivityState and SyncActivitiesState returned nil when the target cluster had no mutable state for the workflow, so the task was dropped. If a sync activity task arrived before the workflow's history, that activity state never reached the target cluster. Return a RetryReplication error instead, like the HSM state replicator does for the same case. The replication task executors then resend the history from the source cluster and apply the task again. If the source cluster no longer has the workflow, the resend path cleans up locally.
hikaru-212
left a comment
There was a problem hiding this comment.
I independently verified this PR against the base (59a7520) and HEAD (1506fa2) using focused deterministic tests covering the legacy replication executor, history/activity replicators, processor, and mutable-state transitions.
1. Verification of the intended fix
The recovery behavior works as intended in the tested scenarios:
- Source exists, target missing: HEAD resends history, reconstructs the target workflow, retries SyncActivity, and successfully applies the activity update. BASE returns nil without restoring the missing target.
- Transient source failure: When the first resend returns
Unavailable, HEAD retries through the processor and successfully recovers on the second attempt, without requesting DLQ submission.
These tests exercise the actual MutableStateImpl and verify the generated UpsertActivityInfos mutation, including heartbeat details, timestamp, attempt, and worker identity.
2. One question about legacy DLQ behavior
I also tested an already-converted SyncActivity task where both source and target return NotFound.
Under identical test conditions:
| Observation | BASE | HEAD |
|---|---|---|
| Executor result | nil |
Target NotFound |
| Processor attempts | 1 | 2 (test limit: 2) |
| DLQ write requests | 0 | 1 |
On HEAD, the resend encounters source-side NotFound, then cleanupWorkflowExecution propagates target-side NotFound. The legacy processor retries and eventually requests a DLQ write.
I noticed the PR description already acknowledges a related possibility in the stream executor. My question is specifically about the legacy path:
Is retry/DLQ the intended terminal behavior when the source is gone and the target has nothing left to clean up? Or should this be treated as successful no-op cleanup, as in the existing SyncVersionedTransition deletion handling?
This may be an intentional policy choice rather than a correctness defect; I wanted to clarify the expected contract.
Verification scope
These are production-component tests with mocked transport and persistence boundaries, not end-to-end cluster tests. The successful recovery cases verify in-memory state changes and generated persistence mutations, but not database commits. The DLQ observation is a write request, not a verified durable DLQ entry.
No activity-state corruption or data loss is claimed.
|
@hikaru-212 Right, that case should be a no-op, same as before this PR. Pushed d3888d8: the legacy executor now treats a |
|
Thanks for the fix! I independently reran the focused regression tests against the updated HEAD (268050f).
All focused tests passed using production components with mocked transport and persistence boundaries. No further concerns within the tested scope. |
What changed?
SyncActivityStateandSyncActivitiesStatenow return aRetryReplicationerror, instead of nil, when the target cluster has no mutable state for the workflow. The resend covers the whole history (empty start and end event IDs), since the workflow does not exist locally. The two existingWorkflowNotFoundtests now expect that error.If the resend finds that the source cluster no longer has the workflow either, the task is dropped.
taskExecutorImpltreats aNotFoundfrom the local cleanup as nothing to clean up, andExecutableActivityStateTask.HandleErrreturns nil whenResendreports that it cleaned up the workflow, likeExecutableSyncVersionedTransitionTaskdoes.Why?
Fixes #6144.
When a sync activity task reached the target cluster before the workflow's history, the replicator dropped it and that activity state was lost. The replication task executors, both the stream one and the older
taskExecutorImpl, already handleRetryReplication: they resend the history from the source cluster and apply the task again. If the source cluster no longer has the workflow, the resend path deletes the local copy, if any.SyncHSMStatealready does this for the same case, and the TODO inSyncActivitiesStateasked for it.How did you test it?
Updated
TestSyncActivity_WorkflowNotFoundandTestSyncActivities_WorkflowNotFoundto expect theRetryReplicationerror; both fail on main. AddedTestProcessTaskOnce_SyncActivityReplicationTask_WorkflowNotFoundInBothClustersandTestHandleErr_Resend_WorkflowDeletedInSourcefor a workflow that is gone from both clusters; both fail without the executor changes. Rango testwith and without-raceon./service/history/ndc/...and./service/history/replication/..., plusgo vetand golangci-lint.Potential risks
A sync activity task for a workflow the target cluster does not have now costs one history resend instead of being ignored. After the resend the task applies normally, and history replication tasks already trigger the same resend for a missing workflow.
If the workflow is gone from the source cluster as well, the task is dropped, as it was before this change. In the stream executor this also applies to other
RetryReplicationerrors on sync activity tasks: once the resend deletes the local workflow, the task finishes instead of being retried against a workflow that no longer exists.