Fail the workflow task when the test server refuses a command instead of stranding it - #3097
sangkyoonnam wants to merge 2 commits into
Conversation
|
Pushed f206a60 for three gaps I found while re-checking the rollback. External signals and cancellations, search-attribute upserts and memo changes were applied before the completion committed, so a command refused later in the same completion rolled back the state machines but not those effects. They now run in Tests cover the search-attribute/memo case, the external signal case and the unknown-message case. I don't have a deterministic test for the stale-update race. The body now spells out what the rollback does not cover. |
What was changed
TestWorkflowMutableStateImpl#completeWorkflowTaskrestores the state machines and the id maps when command processing throws.CompletionSnapshotcaptures the timer, activity, child, Nexus, external signal/cancel and update maps plus every state machine's transition count before the completion, and puts them back on failure (newStateMachine#transitionCount/rollbackTo). The in-memory task then matches the history that still shows it asSTARTED, and a timer or activity created by a command ahead of the refused one is gone.INVALID_ARGUMENTorFAILED_PRECONDITION) for a command that has a failure cause, a second update recordsWorkflowTaskFailedwith that cause and schedules a new task. From the second attempt on the failure is dropped and the task times out, as the search-attribute path already does. The worker getsINVALID_ARGUMENTwith<Cause>: <reason>.RefusedCommandmaps command types to the real server's causes:BAD_*_ATTRIBUTES, andSTART_TIMER_DUPLICATE_ID/SCHEDULE_ACTIVITY_DUPLICATE_IDfor the duplicate-id refusals the test server reports asFAILED_PRECONDITION.processRequestCancelActivityTaskaccepts a cancel for an activity that finished while the task was in flight. It recordsActivityTaskCancelRequestedand nothing else; the buffered finish event follows in history, as on the real server.ctx.onCommit, so a completion refused by a later command never applies them. AProtocolMessagecommand that is refused (or references a missing message) fails the task withBAD_UPDATE_WORKFLOW_EXECUTION_MESSAGE, as the real server does. The update that records the failure checks it is still the refused task (samescheduledEventId, stillSTARTED) under the workflow lock before it changes anything.RefusedCommandTestcovers: cancel of an unknown timer, cancel of an unknown activity,ScheduleActivityTaskwithout a task queue, a duplicate timer id, a cancel of an activity that completed during the task, and a refused completion whose earlierStartTimermust not break the worker's replay.Why?
completeWorkflowTaskmoved the workflow task state machine toNONEand only then processed commands. When a handler threw, theRequestContextwas discarded but the state machine stayed atNONE, sotimeoutWorkflowTaskreturned early. Nothing failed the task and nothing redelivered it; the run stayedRUNNINGforever. #3088 hits this withCancelTimerfrom the TypeScript SDK and a second user withRequestCancelActivityTaskfrom the Python SDK. Every handler that throws after the transition has the same effect.The real server validates each command and fails the workflow task with the command's cause (
handleCommandCancelTimerand the rest inworkflow_task_completed_handler.go, throughfailWorkflowTaskOnInvalidArgument), then returnsInvalidArgumentwith the cause message. This change gives the test server that contract without touching each handler.Two things I found on the way. Rolling back only the workflow task state machine is not enough: with a valid
StartTimerahead of the refused command the timer stayed in memory, and the worker's replay of thatStartTimerwas rejected as a duplicate on every attempt, hence the snapshot. And the activity race is real: an activity that completes while a task is running leaves the in-memory map at once, so the worker's cancel for it was refused and stranded the task. The timer side of that race (aTimerFiredstill buffered) is #3060; with this PR it fails the task withBAD_CANCEL_TIMER_ATTRIBUTESinstead of hanging, and #3060 makes it succeed, so the two land in either order.Checklist
Closes Test server: refused CancelTimer leaves the workflow task complete in memory but STARTED in history, so it is never timed out or redelivered #3088
How was this tested:
./gradlew :temporal-test-server:test --tests "io.temporal.testserver.functional.RefusedCommandTest". I ran the new tests onmainwithout the change: the unknown-timer case leaves history at[WORKFLOW_EXECUTION_STARTED, WORKFLOW_TASK_SCHEDULED, WORKFLOW_TASK_STARTED]with no failure or redelivery, the unknown-activity case returnsFAILED_PRECONDITIONand strands the task the same way, and the finished-activity case is refused withFAILED_PRECONDITION: ACTIVITY_UNKNOWN for scheduledEventId=5. With the change all six pass. The fulltemporal-test-serversuite passes (99 passed, 1 skipped), and thetemporal-sdkactivity, timer, cancellation, child-workflow, signal and update tests run against this server pass (273 passed, 9 skipped). After the second commit: the three added tests (search attributes and memo, external signal, unknown protocol message) fail on the first commit's source and pass with it,RefusedCommandTestis 9/9, the fulltemporal-test-serversuite is 102 passed, 1 skipped, and thetemporal-sdksignal, cancel-external, search-attribute, memo and update tests are 112 passed, 8 skipped.Not covered: this is not a complete rollback of mutable state. Fields written by transition callbacks are not copied, and other completion effects such as continue-as-new and completion callbacks are outside this change. Messages processed without a protocol command and malformed messages are not covered by the failure mapping. The tests resubmit commands over raw RPC rather than running SDK replay. The buffered timer/cancel race is #3060, and the reporter's second observation (time skipping slowing after
terminatewith a task and activity in flight) is a separate path. There is no regression test for a refused external cancellation or for a message refused insideprocessMessage; the follow-up tests cover a refused external signal, search attributes and memo, and a message with an unknown id.No.