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.
chore(container-runtime): remove DisableBatchIdTracking kill-switch #27296
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
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
chore(container-runtime): remove DisableBatchIdTracking kill-switch #27296
Changes from all commits
9957a022cf9384File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
Check failure on line 6 in .changeset/calm-batches-track.md
Check failure on line 6 in .changeset/calm-batches-track.md
Check failure on line 8 in .changeset/calm-batches-track.md
Check failure on line 8 in .changeset/calm-batches-track.md
Check failure on line 8 in .changeset/calm-batches-track.md
Check failure on line 8 in .changeset/calm-batches-track.md
Check warning on line 8 in .changeset/calm-batches-track.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deep Review: The new runtime gate is strictly narrower than the loader's offline opt-in, which silently regresses fork detection for
Fluid.Container.enableOfflineLoadconsumers.The divergence. Runtime now derives
offlineEnabledfrom a single config:But the loader treats offline-load as enabled when any of three signals is set (
packages/loader/container-loader/src/container.ts:964-968):The first arm —
Fluid.Container.enableOfflineLoad— is a live (non-deprecated) config flag. Only the third typed-options arm is@deprecated Do not use(packages/common/container-definitions/src/loader.ts:608-616).Concrete consequence. A host opted into offline load via
Fluid.Container.enableOfflineLoadcontinues to wire upSerializedStateManager/closeAndGetPendingLocalStateat the loader, but the runtime no longer instantiatesDuplicateBatchDetector(this file, theif (this.offlineEnabled)allocation) and no longer stampsbatchIdon resubmit (batchId: this.offlineEnabled ? batchId : undefined). The exact forked-container scenarioDuplicateBatchDetectorwas built to catch (#21767, #22497) goes silently undetected.Why pre-PR was safe. Pre-PR, runtime tracking was default-on (
TurnBased+ grouped batching ⇒ tracking on), accidentally papering over the divergence soenableOfflineLoadconsumers still got the detector. This PR removes that safety net without unifying the gates or documenting the migration. The changeset only states "Forked-container duplicate detection now requires the Offline Load opt-in" — implying a single opt-in when there are three at the loader layer.Historical. tyler-cai-microsoft flagged this exact multi-flag/loader-runtime back-compat concern in #21767 inline on
containerRuntime.ts:4134(2024-07-10); markfields replied "I'll follow up... since none of this has shipped yet." The follow-up never landed; this PR cements the divergence.Precedent for the union.
packages/loader/container-loader/src/snapshotRefresher.ts:98-101already accepts eitherFluid.Container.enableOfflineSnapshotRefreshorFluid.Container.enableOfflineFull— the codebase has a pattern for honoring multiple offline opt-ins at downstream layers.Pick one:
enableOfflineLoad ?? enableOfflineFull, mirroringsnapshotRefresher.ts:98-101. Lowest-risk — preserves pre-PR behavior for legacy-flag consumers without re-introducing the kill-switch.offlineLoadEnabled(the full three-arm union fromcontainer.ts:964-968) into the runtime viaIContainerContextso both layers always agree. Bigger surface change, eliminates the divergence permanently..changeset/calm-batches-track.mdand theofflineEnableddoc-comment (containerRuntime.ts:1457-1464) to call out that hosts usingFluid.Container.enableOfflineLoad(or relying onIContainerLoadMode.enableOfflineLoad: true) must migrate toFluid.Container.enableOfflineFullto retainDuplicateBatchDetectorand resubmit batchId stamping. Add aUsageErroror telemetry warning when the loader has serialized-pending-state plumbing engaged but the runtime gate is off. If you go this route, also clarify theoutbox.ts:358-368"we do it always" comment to referenceenableOfflineFull, sinceflushAll's placeholder grouped-batch path is now transitively gated on the same flag.Question for you: for consumers currently enabling offline load via
Fluid.Container.enableOfflineLoad(the config flag, not the deprecated typed option), is the intent that they migrate toFluid.Container.enableOfflineFull, or should the runtime honor the union?Uh oh!
There was an error while loading. Please reload this page.