-
Notifications
You must be signed in to change notification settings - Fork 547
improvement(container-runtime): For resubmit, don't submit ID Allocation op until submitting another op #24545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
markfields
wants to merge
10
commits into
microsoft:main
Choose a base branch
from
markfields:idc/1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
Take all Current on conflicts in containerRuntime.ts. Will sort out the merge next.
markfields
commented
May 21, 2025
This is not the priority at the moment, but I do hope to merge this to move us towards consolidating logic around op submission/batching. Maybe after the next release, there is some FUD about unintended side effects, i.e. whether the existing test coverage is sufficient since the behavior is so nuanced. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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
tl;dr - Delay submitting ID Allocation Ops until right before submitting other ops. Specifically updating the ID Allocation Op generated before replaying pending state.
Background
ID Allocation ops must be submitted before other ops to ensure compressed IDs contained in those ops can be understood by all clients. This change focuses on a special case before replaying pending state where the ID Compressor needs to re-take all unfinalized creation ranges, since any original ID Allocation ops (if any) won't be resubmitted.
Today, this op is submitted directly to the Outbox under a different codepath than we really expect to be submitting ops. In fact, there's a bug where we don't schedule a flush, so it's possible if the user stops interacting with the document at this moment it will not be submitted at all.
The fix is to still generate the op before replaying pending states (so ID Compressor internal state is correct), but don't actually submit the op until we know we have reason to. (It's ok if we generate it, then drop it, and generate it again)
One other motivation for this change: ID Allocation ops are special because they are not "dirtyable" meaning they alone don't mark the container as dirty. We've found that the dirty/saved logic is a bit inconsistent though (see the struggle in #24646), so it's advantageous to avoid submitting these ops apart from other "dirtyable" ops. In fact we may follow up and stop marking them as "non-dirtyable" after this PR.
Prerequisite bug fix in Pending State Manager
There's a fix in PendingStateManager that became necessary after the main change here, it should have been this way all along. Since ID Allocation ops aren't resubmitted, we don't care to track their batch IDs. This means we shouldn't include batchId when replaying them (they're going to get dropped anyway) - without this fix we were triggering the empty batch logic (since they're not actually submitted on replay) when we shouldn't be resubmitting an empty batch.