fix(id-allocation): Schedule a flush when submitting ID Allocation op before replaying pending states #24683
+65
−34
Conversation
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
github-actions
bot
added
area: runtime
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that after submitting an ID Allocation operation, a flush is scheduled to prevent ops from stalling in the Outbox and refactors the flush scheduling logic to use a unified flushPending flag and doPendingFlush helper.
- Call
scheduleFlushimmediately after submitting an ID Allocation op before replaying pending states. - Replace
flushTaskExistswithflushPendingand introduce adoPendingFlushmethod. - Update
scheduleFlushto use a switch onflushMode, removing the oldcurrentlyBatchinghelper and assertions.
Prompt: > Write a test that generates a compressed ID, then reconnects (triggering replayPendingStates), then processes an op from another client, then generates another ID and submits a simple op using that ID. It should hit the "outboxSequenceNumberCoherencyCheck" error on flush. I had to do surprisingly little modification (besides extra refactoring I wanted to)
markfields
requested review from
taylorsw04,
a team,
steffenloesch,
pragya91,
jason-ha,
jatgarg,
kian-thompson,
WillieHabi,
MarioJGMsoft,
Abe27342 and
anthony-murphy
and removed request for
steffenloesch,
pragya91,
jason-ha,
jatgarg,
kian-thompson,
WillieHabi,
MarioJGMsoft and
taylorsw04
anthony-murphy
approved these changes
May 23, 2025
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.
Description
Fixes AB#39398
In
ContainerRuntime, we submit to theOutboxdirectly fromreplayPendingStates, and so we should also callscheduleFlushto ensure that op doesn't get stuck in the Outbox. Being "stuck in the Outbox" is a problem because it's assumed that all ops in a batch come during the same JS turn and have the same reference sequence number, and this will violate that invariant.That one-line change is accompanied by some refactoring so we don't have to consider Immediate mode at that callsite (
scheduleFlushhandles it now).In-Depth
In the typical case, calling
PendingStateManager.replayPendingStateswill trigger another op(s) to submit which will schedule the flush. But here's a counterexample:This will result in the replay flow, and we will submit an ID Allocation op, but there's nothing else to resubmit. This op will be "stuck" in the Outbox.
In the current code, this is kind of ok, because ID Allocation ops don't make the container dirty, and it's ok to drop that op. BUT, as mentioned above, it can easily violate the invariant that all ops in a batch have the same reference sequence number, because that "old" op will be included in the next batch, which could be some time later after new ops have been processed (which advances the refSeq). See the unit test added here.