fix: Convert Continue-As-New suggestion reason symbol to int#469
Open
jasonligg wants to merge 2 commits into
Open
fix: Convert Continue-As-New suggestion reason symbol to int#469jasonligg wants to merge 2 commits into
jasonligg wants to merge 2 commits into
Conversation
6742d73 to
16361c6
Compare
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
This PR proposes two changes each as their own commit:
WorkflowInstance#activateto convert bridge enum symbols to integers usingProtoUtils.enum_to_intinstead of calling.to_idirectly onsuggest_continue_as_new_reasons.HistoryInfoWorkflow) to also assert onsuggest_continue_as_new_reasons.CHANGELOG.mdto move the existing changes under a release for1.5.0. This was probably forgotten for the most recent release.proto_gen.rb) to preserve repeated enum field type information in generated RBS signatures (using a new_RepeatedEnumFieldinterface)._ActivationRBS interface forWorkflowInstance#activate, replacing the previousuntypedactivation parameter so Steep can see the repeated enum reason types used by the workflow instance code.Why?
The first commit addresses the actual problem encountered during workflow execution. When a workflow's history grows large enough for Temporal to suggest continue-as-new, the bridge returns
suggest_continue_as_new_reasonsas an array of protobuf enum symbols (e.g.,:SUGGEST_CONTINUE_AS_NEW_REASON_TOO_MANY_HISTORY_EVENTS). The existing code called.to_ion these symbols, which raisesundefined method 'to_i' for an instance of Symbol. This caused workflow tasks to fail repeatedly, leaving workflows permanently stuck.The second commit is a bit out-of-scope but I was thinking about how this could be avoided in the future. I understand that there are parts of the SDK that are intentionally kept wide or even untyped so this change may not be welcome. I'm not super familiar with Steep as I use Sorbet a lot at work so I get the gist (footguns 😅) of adding types to an untyped language like Ruby or JS/TS.
💡 I'm fine with reverting that commit if it isn't appropriate!
Checklist
Closes [Bug] Symbol Conversion on suggestContinueAsNew event. #463
How was this tested:
WorkflowInstanceTest#test_continue_as_new_suggestion_reasons_are_visible_as_workflow_enum_ints) that exercises the activation path with symbol enum reasons.test_history_infointegration test to verifysuggest_continue_as_new_reasonsis returned correctly.Any docs updates needed?