Repository navigation
Fix test server leaking time skipping lock when a child outlives its parent - #3142
SamuelAdigos wants to merge 1 commit into
Conversation
|
|
|
|
When a child workflow closes, the test server locks time skipping until the parent processes the close notification. The parent released that lock while handling the notification, but if the parent had already closed, as is usual for an abandoned child, its update failed with NOT_FOUND before the unlock was applied. The lock leaked and time skipping stayed disabled for the lifetime of the test server, so every later sleep, timer and retry backoff ran in real time. The child now releases the lock itself after notifying the parent, whether or not the notification succeeded. This keeps the lock and unlock together and guarantees a single release. By the time the parent's update returns it holds its own workflow task lock, so time still cannot skip before the parent reacts. Fixes temporalio#2392.
55643e8 to
2fd410e
Compare
|
I'm aware the Java test server is planned for deprecation in favor of time skipping in the real server. I'm proposing this anyway because the fix is small and the leak affects every SDK that ships this test server binary until then: after one abandoned child closes, all later timers run in real time. Happy for you to close it if it doesn't fit. |
|
Thanks for the contribution, as you noted we are not planing to invest more in the Java Test Server. Closing since the underlying issue is not planned. |
3 participants
What was changed
When a child workflow closes, the test server locks time skipping until the parent processes the child's close notification. The parent used to release that lock in
childWorkflowCompleted,childWorkflowFailed,childWorkflowCanceledandchildWorkflowTimedOut. The child now releases it itself in afinallyblock after notifying the parent, whether or not the notification succeeded.TestWorkflowMutableStateImpl: moved the release of the "notify parent" lock from the parent's handlers to the child's notification paths (complete, fail, cancel, run timeout).AbandonedChildTimeSkippingTest.CHANGELOG.mdentry under Fixed.Why?
If the parent had already closed (the usual case for a
PARENT_CLOSE_POLICY_ABANDONchild that outlives it), the parent's update fails withNOT_FOUNDbefore its unlock is applied. The child swallowed that error (// Parent might already close), so the lock leaked and the lock counter never returned to zero. Time skipping then stayed disabled for the lifetime of the test server: every later sleep, timer and retry backoff ran in real time, including in unrelated tests that share the server.Releasing the lock in the child keeps the lock and unlock in one place and guarantees exactly one release, also when the notification fails for another reason. Behavior in the normal case is unchanged: by the time the parent's update returns, the parent holds its own workflow task lock, so time cannot skip before the parent reacts.
Checklist
Closes [Bug] Test environment sleep hangs on child workflows #2392. Also fixes TestWorkflowEnvironment sleep incorrect behaviour #2642 and TestWorkflowEnvironment#sleep hangs indefinitely #2670, which have the same root cause.
How was this tested:
AbandonedChildTimeSkippingTestcovers a child that completes, fails, is canceled or times out after its parent closed, then callsTestWorkflowEnvironment.sleep(Duration.ofDays(1)). Onmainall four cases hang in that sleep until the test timeout (TestTimedOutException); with the fix they pass immediately. The test is skipped against an external server, since time skipping is a test server feature.Locally:
./gradlew clean build(tests on Java 11) and./gradlew test -PtestJavaVersion=11 -PtestServer=dev-serverpass,jackson3TestsandvirtualThreadTestspass on Java 21, and:temporal-test-server:test,:temporal-testing:testand thetemporal-sdkchild workflow and cancellation tests pass with-P edgeDepsTest -PtestJavaVersion=23.No.