Await actor alarm deletion cache flush outputGate when alarm succeeds #1601
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
kentonv
reviewed
Feb 2, 2024
src/workerd/io/worker-entrypoint.c++
Outdated
| // same alarm that just completed) from CRDB before these changes are actually made, | ||
| // rerunning it, when it shouldn't. | ||
| KJ_IF_SOME(persistent, actor.getPersistent()) { | ||
| KJ_IF_SOME(pendingFlush, persistent.onNoPendingFlush()) { |
There was a problem hiding this comment.
Should this maybe be waiting on the output gate, instead of directly on underlying storage? They are usually the same thing but if there's a case where they don't match, I would hope that breaking the output gate will prevent an alarm from being treated as successful.
There was a problem hiding this comment.
IIUC you're suggesting to await for actor->getOutputgate() instead of actor-cache::onNoPendingFlush(), since the output gate, to prevent situation where the output gate is not the same, correct?
That makes sense, since the onNoPendingFlush will return a promise that is output gate locked, if there's a flush pending.
jqmmes
force-pushed
the
joaquim/await-alarm-deletion-output-gate
branch
from
c277944 to
7808de9
Compare
jqmmes
force-pushed
the
joaquim/await-alarm-deletion-output-gate
branch
from
7808de9 to
a2cb92a
Compare
kentonv
approved these changes
Feb 5, 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.
When an alarm handler successfully completes its execution, the alarm is marked ready for deletion in actor-cache (unless it's been updated since the start of handler execution). This alarm change will only be reflected in storage, once cache flushes and changes are written.
If we have access to actor-cache, we must await for the alarm deletion or update to flush(), meaning the changes were written to storage before we return the alarm result.
If we don't await, it's possible for alarm manager to pull the wrong alarm value (the same alarm that just completed) from storage before these changes are actually made, rerunning it, when it shouldn't.