Fix schedule workflow to CAN after signals #5799
Merged
Conversation
This file contains 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
tdeebswihart
approved these changes
Apr 25, 2024
yycptt
pushed a commit
that referenced
this pull request
Apr 25, 2024
## What changed and why? If a schedule was paused and received a large number of signals (trigger, backfill, etc.), it wouldn't be able to continue-as-new and history size could grow past the suggested limit. For various reasons the main loop receives and processes signals across iterations, with checking for CAN in between, so a wakeup due to a signal (instead of a timer) would prevent CAN. Refactoring is hard due to the determinism requirement, so the simplest fix is to do a second check. ## How did you test it? New unit test
yycptt
pushed a commit
that referenced
this pull request
Apr 27, 2024
## What changed and why? If a schedule was paused and received a large number of signals (trigger, backfill, etc.), it wouldn't be able to continue-as-new and history size could grow past the suggested limit. For various reasons the main loop receives and processes signals across iterations, with checking for CAN in between, so a wakeup due to a signal (instead of a timer) would prevent CAN. Refactoring is hard due to the determinism requirement, so the simplest fix is to do a second check. ## How did you test it? New unit test
dnr
added a commit
to dnr/temporal
that referenced
this pull request
May 1, 2024
dnr
added a commit
that referenced
this pull request
May 31, 2024
## What changed? In the schedule workflow, we should always reprocess from the last action in order to handle getting woken up by a signal in between the nominal time and actual time. This is basically what #5381 should have been in the first place, and applies that logic to all iterations. Note that this adds the logic but doesn't activate it yet. ## Why? Fixes bug: signals (including refresh) in between nominal and actual time could lead to dropped actions. This only happens if the cache runs out or we do a CaN at just the right time, so it's not that easy to reproduce. This has also blocked activating #5799 since that makes this bug more likely. ## How did you test it? Added new unit test and replay test. Reproduced locally by disabling cache and sending frequent signals to try to disrupt a schedule. Verified that new version did not drop actions. ## Potential risks This changes workflow logic, but is pretty easy to see that the old control flow is unaffected. There's one more potential situation which isn't always handled correctly: unpause in between nominal and actual times will usually run the jittered action (this is probably the less surprising behavior), but rarely, if a CaN happens at the same time, it can get skipped because the cache will be regenerated. --------- Co-authored-by: Rodrigo Zhou <rodrigozhou@users.noreply.github.com>
pdoerner
pushed a commit
that referenced
this pull request
May 31, 2024
## What changed? In the schedule workflow, we should always reprocess from the last action in order to handle getting woken up by a signal in between the nominal time and actual time. This is basically what #5381 should have been in the first place, and applies that logic to all iterations. Note that this adds the logic but doesn't activate it yet. ## Why? Fixes bug: signals (including refresh) in between nominal and actual time could lead to dropped actions. This only happens if the cache runs out or we do a CaN at just the right time, so it's not that easy to reproduce. This has also blocked activating #5799 since that makes this bug more likely. ## How did you test it? Added new unit test and replay test. Reproduced locally by disabling cache and sending frequent signals to try to disrupt a schedule. Verified that new version did not drop actions. ## Potential risks This changes workflow logic, but is pretty easy to see that the old control flow is unaffected. There's one more potential situation which isn't always handled correctly: unpause in between nominal and actual times will usually run the jittered action (this is probably the less surprising behavior), but rarely, if a CaN happens at the same time, it can get skipped because the cache will be regenerated. --------- Co-authored-by: Rodrigo Zhou <rodrigozhou@users.noreply.github.com>
dnr
added a commit
to dnr/temporal
that referenced
this pull request
Jun 6, 2024
dnr
added a commit
that referenced
this pull request
Jun 7, 2024
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 changed and why?
If a schedule was paused and received a large number of signals (trigger, backfill, etc.), it wouldn't be able to continue-as-new and history size could grow past the suggested limit.
For various reasons the main loop receives and processes signals across iterations, with checking for CAN in between, so a wakeup due to a signal (instead of a timer) would prevent CAN. Refactoring is hard due to the determinism requirement, so the simplest fix is to do a second check.
How did you test it?
New unit test