Fix recordHeartbeat: swallowed cancel/reset/pause exceptions - #2990
Open
brainlimitexceeded wants to merge 5 commits into
Open
Fix recordHeartbeat: swallowed cancel/reset/pause exceptions#2990brainlimitexceeded wants to merge 5 commits into
brainlimitexceeded wants to merge 5 commits into
Conversation
…ons and missing retry Two related bugs in the same method: Fixes temporalio#2983: recordHeartbeat wrapped the RPC call and the response-flag checks in a single try block, so its own ActivityCanceledException / ActivityResetException / ActivityPausedException were caught by the generic catch (Exception e) and turned into ActivityCompletionFailureException by processException. Callers could not tell a cancelled/reset/paused activity apart from a failed RPC without unwrapping getCause(). The interface also declared `throws CanceledFailure`, a type the method has never actually thrown (CanceledFailure and ActivityCompletionException are unrelated siblings under TemporalException) -- changed to the type it genuinely throws, ActivityCompletionException. Fixes temporalio#2984: recordHeartbeat was the only one of this class's four RPC methods (complete/fail/reportCancellation/recordHeartbeat) that didn't go through grpcRetryer.retryWithResult(...). A single transient error (RESOURCE_EXHAUSTED from namespace rate limiting, DEADLINE_EXCEEDED, UNAVAILABLE) could fail the heartbeat outright, which for async completion can cost a long-running activity via heartbeat timeout. Now wrapped in the same retry helper the sibling methods already use. Both fixes land in the same restructure: the RPC call is now isolated in its own try/catch (wrapped in retryWithResult), and the cancel/reset/paused flag checks happen outside that catch so the correct exception always reaches the caller.
|
|
…Response container
dplyukhin
changed the title
dplyukhin
force-pushed
the
fix/heartbeat-swallowed-exceptions-and-retry
branch
from
August 24, 2026 22:43
7720ba0 to
cb0373a
Compare
Contributor
|
Thanks for the contrib @brainlimitexceeded! I removed the retry part of the PR because that's complicated logic I want to handle separately. Can you please sign the CLA? |
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
4 participants
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.
Summary
Closes #2983. Fixes
ManualActivityCompletionClientImpl'srecordHeartbeatto raiseActivityCanceledException/ActivityResetException/ActivityPausedExceptioninstead of an incorrectActivityCompletionFailureException.What changed
ActivityHeartbeatResponse: New wrapper class to reduce code duplication. Technically a breaking change, but only to internals that users shouldn't be relying on.ManualActivityCompletionClient.recordHeartbeatManualActivityCompletionClientImplto propagate exceptions correctlyManualActivityCompletionClientImplTestfor each possible thrown exception