Repository navigation
Conversation
Release outstanding workflow task locks on forced closure. Release child notification locks even when the parent rejects the update. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Contributor
|
We're not accepting fixes for this sort of thing, since we are planning to begin deprecation for this server. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed
The test server's time-skipping lock was not released on two paths:
scheduleWorkflowTask. Later task responses are rejected by the terminal-state check inupdatebefore reaching their normal release.updatenow releases that lock when it force-closes the workflow.childWorkflowStarted/Completed/Failed/Canceled/TimedOut,failStartChildWorkflow) release their lock inside the parent's update callback. If the parent has already closed,updatethrowsNOT_FOUNDbefore the callback runs and the lock is never released. The release now happens in afinally(updateWithTimerUnlock).Related: #3168 fixes the two paths listed below (pending activities and external signal callbacks) plus cancel-before-start; the two PRs are independent.
Known limits: Two further unbalanced paths remain and are not changed here: pending activities when their workflow closes first (the lock from
processScheduleActivityTask), and external-signal callbacks when the source workflow has closed. They need separate tests, so I kept this change to the two paths above.Why?
With a locked clock the server stops skipping time, so later timers in the same server never fire. We hit this running a large Python test suite on one shared time-skipping server: after tests that terminated workflows, a later test waiting on a virtual timer hung until the test timeout.
Checklist
Closes: no issue filed. It is a small bug fix in the test server and I found no existing issue for it.
How was this tested:
New
TestWorkflowMutableStateTimeSkippingTest(10 tests): termination with a scheduled task, with a started task, with a late task completion, idle termination (guards against a double unlock), run timeout, and each of the six child callbacks on a closed parent. Each requires a future timer to fire through automatic time skipping afterwards. Against the unpatched server 9 of 10 fail (the idle-termination control passes); with the patch all 10 pass.ChildLivesLongerThanParentTest.testAbandonChildnow also requires a following virtual sleep to complete.:temporal-test-server:testplus theChildLivesLongerThanParentTest,TestEnvironment,TimeSkipping,TerminatedWorkflowTestandTerminateTestselections intemporal-sdkpass (114 tests).This change was developed with AI assistance (Claude) and reviewed and tested locally.