Fix test server ignoring whole-second retry deadlines - #3096
Merged
maciejdudko merged 2 commits intoSep 30, 2026
Merged
maciejdudko merged 2 commits into
maciejdudko merged 2 commits into
Conversation
maciejdudko
reviewed
Sep 29, 2026
maciejdudko
left a comment
Contributor
There was a problem hiding this comment.
Hi @alvinsee, thank you for your contribution! The code looks good and the tests pass, I'm ready to approve it. But we just introduced a new process where every PR should add a note to the new CHANGELOG.md file. Please update the branch and add a note under Fixes section. You can look at other SDK repos for guidance, e.g. https://github.com/temporalio/sdk-dotnet/blob/main/CHANGELOG.md?plain=1
TestServiceRetryState skipped the expiration check whenever the deadline's nanos field was zero. The guard appears to have been meant to detect an unset expiration, but the constructor already replaces an unset expiration with Timestamps.MAX_VALUE, which never compares as expired. As a result, the guard only suppressed real deadlines that happened to fall exactly on a second boundary, so retries were scheduled past their expiration instead of returning RETRY_STATE_TIMEOUT. Drop the nanos guard and rely on the timestamp comparison alone. Add unit tests covering whole-second, subsecond, unset, and not-yet-reached expirations.
alvinsee
force-pushed
the
alvinsee/fix-test-server-retry-expiration
branch
from
September 29, 2026 22:57
8c48b0c to
43428f8
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Thanks @maciejdudko! I rebased the branch onto the latest main and added an entry under |
maciejdudko
approved these changes
Sep 30, 2026
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.
TestServiceRetryState skipped the expiration check whenever the deadline's nanos field was zero. The guard appears to have been meant to detect an unset expiration, but the constructor already replaces an unset expiration with Timestamps.MAX_VALUE, which never compares as expired. As a result, the guard only suppressed real deadlines that happened to fall exactly on a second boundary, so retries were scheduled past their expiration instead of returning RETRY_STATE_TIMEOUT.
Drop the nanos guard and rely on the timestamp comparison alone. Add unit tests covering whole-second, sub-second, unset, and not-yet-reached expirations.
What was changed
Removed the
expirationTime.getNanos() != 0guard from the expiration check inTestServiceRetryState.getBackoffIntervalInSeconds, and addedTestServiceRetryStateTest.Why?
The guard only looked at the subsecond field of the protobuf
Timestamp. An unset expiration is already turned intoTimestamps.MAX_VALUEin the constructor, and that never compares as expired. So the guard never protected the "no deadline" case. Its only effect was to skip the timeout when a real deadline fell exactly on a whole second. In that case the test server kept scheduling retries past the deadline instead of returningRETRY_STATE_TIMEOUT.Checklist
How was this tested:
The new test
expirationOnWholeSecondTimesOutfails without the fix(expected:<RETRY_STATE_TIMEOUT> but was:<RETRY_STATE_IN_PROGRESS>) and passes with it. The full:temporal-test-server:testsuite passes.