Skip to content

Fix test server ignoring whole-second retry deadlines - #3096

Merged
maciejdudko merged 2 commits into
temporalio:mainfrom
alvinsee:alvinsee/fix-test-server-retry-expiration
Sep 30, 2026
Merged

maciejdudko merged 2 commits into
temporalio:mainfrom
alvinsee:alvinsee/fix-test-server-retry-expiration

Conversation

@alvinsee

Copy link
Copy Markdown
Contributor

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() != 0 guard from the expiration check in TestServiceRetryState.getBackoffIntervalInSeconds, and added TestServiceRetryStateTest.

Why?

The guard only looked at the subsecond field of the protobuf Timestamp. An unset expiration is already turned into Timestamps.MAX_VALUE in 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 returning RETRY_STATE_TIMEOUT.

Checklist

How was this tested:
The new test expirationOnWholeSecondTimesOut fails without the fix(expected:<RETRY_STATE_TIMEOUT> but was:<RETRY_STATE_IN_PROGRESS>) and passes with it. The full :temporal-test-server:test suite passes.

@alvinsee
alvinsee requested a review from a team as a code owner September 23, 2026 01:43
@CLAassistant

CLAassistant commented Sep 23, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@maciejdudko maciejdudko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
alvinsee force-pushed the alvinsee/fix-test-server-retry-expiration branch from 8c48b0c to 43428f8 Compare September 29, 2026 22:57
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alvinsee

Copy link
Copy Markdown
Contributor Author

Thanks @maciejdudko! I rebased the branch onto the latest main and added an entry under Fixed in CHANGELOG.md (c49c7ec). Ready for another look.

@maciejdudko
maciejdudko merged commit 4a4e6b2 into temporalio:main Sep 30, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants