Repository navigation
[release/3.0] fix(odsp-driver): remove invalid byteOffset === 0 assumption - #28199
Merged
Lindsey Nguyen (lindsnguyen) merged 2 commits intoSep 11, 2026
Conversation
…ption (microsoft#28194) Fixes compact snapshot parsing when the input is a valid `Uint8Array` view with a non-zero `byteOffset`. The ODSP compact snapshot parser asserted that its input must begin at offset zero: ```ts assert(input.byteOffset === 0, 0x3e8 /* code below assumes no offset */); ``` The affected snapshots were valid, but their binary data was represented by a view into a larger backing buffer. The assertion closed the Fluid container during loading and caused eight of the 29 Office eDiscovery fixtures to fail with assertion `0x3e8`. The assertion has existed since 2022, but the relevant inputs previously happened to have a zero `byteOffset`, leaving the invalid assumption hidden. The failure started after [Node.js 24.18.0 increased the default `Buffer.poolSize` from 8 KiB to 64 KiB](nodejs/node#63597). Node uses this pool for `Buffer.allocUnsafe()` allocations smaller than half the pool size. This increased the pooling threshold from approximately 4 KiB to 32 KiB. The Office eDiscovery export path reads binary snapshots using: ```ts fs.readFileSync(filePath) ``` Node's `fs.readFileSync()` allocates the returned `Buffer` using `Buffer.allocUnsafe(fileSize)`. A Node `Buffer` is also a `Uint8Array` view. Before Node 24.18: - The default buffer pool was 8 KiB. - Allocations of approximately 4 KiB or larger bypassed the pool. - Relevant snapshot reads therefore received dedicated backing buffers, normally with `byteOffset === 0`. Beginning with Node 24.18: - The default buffer pool is 64 KiB. - Allocations smaller than approximately 32 KiB can come from the shared pool. - Those buffers can be slices of the shared backing allocation and therefore have a valid non-zero `byteOffset`. Node 24.20 and 24.21 inherited this behavior. This explains why the failure appeared when the Office External Partner pipeline moved from Node 24.11 to newer Node 24 releases. The Office External Partner integration pipeline selected Node using this floating range: ```yaml - task: UseNode@1 inputs: version: ">=24.11.0 <25.0.0" ``` The range selects the newest available matching Node 24 release, not the lowest version in the range. Normal Office CI did not initially reproduce the issue because its managed build image still provided Node 24.11.0. Controlled runs confirmed the Node version as the differentiating variable: - Node 24.21.0 with Fluid build 422789: eight eDiscovery failures. - Node 24.11.0 with the same Fluid build and Office test code: all 29 eDiscovery tests passed. Node did not introduce malformed snapshot data or change the `Uint8Array` contract. The larger buffer pool caused more file reads to use valid offset views, exposing Fluid's existing assumption about the backing-buffer layout. Pinning Node 24.11 would avoid the expanded pooling threshold and can serve as a temporary mitigation. However, it would leave Fluid dependent on an allocation detail that Node does not guarantee. Other consumers can also provide valid non-zero-offset `Uint8Array` views independently of Node's file-reading behavior. The assertion was originally added by Vlad Sudzilouski in microsoft#12058 as part of a string-parsing performance optimization. The PR discusses null-character handling and differences between browser and Node string decoding, but it does not document why a zero byte offset was required. The parser does not require a zero-offset backing buffer: - `Uint8Array` indexing is relative to the view. - `Uint8Array.subarray()` is relative to the view. - `ReadBuffer` positions and reads are relative to the supplied `Uint8Array`. - Existing `BlobShallowCopy` handling explicitly accounts for `data.byteOffset`. For example, if a view begins at offset 100 in its backing buffer, `input[0]` still reads the first byte of that view, not byte zero of the backing buffer. The string positions recorded by the parser are also relative to that same view. A non-zero offset is therefore a supported memory layout rather than evidence of a malformed or corrupted snapshot. The assertion rejected valid input without providing additional snapshot-integrity protection. This change removes that assertion. It does not remove validation of snapshot structure, string lengths, tree markers, skipped ranges, or unexpected end-of-file conditions. Copying the input into a new zero-offset buffer would also avoid the assertion, but it would introduce an unnecessary full-snapshot allocation and preserve the incorrect parser restriction. - Remove the `byteOffset === 0` assertion from compact snapshot string loading. - Add a regression test that: - Serializes a compact snapshot. - Embeds it inside a larger backing buffer. - Passes a non-zero-offset `subarray()` to `TreeBuilder.load()`. - Confirms that the test input actually has a non-zero `byteOffset`. - Verifies that the resulting strings, blobs, and tree match the original snapshot. The issue was isolated using controlled Office External Partner pipeline runs: | Office build | Node | Fluid build | eDiscovery result | | --- | --- | --- | --- | | [55007732](https://office.visualstudio.com/OC/_build/results?buildId=55007732&view=results) | 24.21.0 | 422789 | 21 passed, 8 failed with `0x3e8` | | [55015480](https://office.visualstudio.com/OC/_build/results?buildId=55015480&view=results) | 24.11.0 | 422789 | 29/29 passed | | [55069760](https://office.visualstudio.com/OC/_build/results?buildId=55069760&view=results) | 24.21.0 | 422948 with this fix | 29/29 passed; pipeline succeeded | The first two runs used the same Fluid build and Office test code, with only the Node version changed. This confirmed that the larger buffer pool in newer Node versions exposes the existing parser assumption. The final run restored the triggering Node 24.21.0 environment and used Fluid build 422948 containing this change. All 29 eDiscovery tests passed, including the eight fixtures that previously failed. The overall integration pipeline also succeeded. The ODSP driver build completed successfully, including compilation, linting, API checks, and test compilation. The Tree Representation test suite passed in both ESM and CommonJS configurations. A Fluid Real Service End to End Tests run was also queued using client build 422948 to exercise the change against an actual ODSP tenant: - [Fluid Real Service E2E build 423091](https://dev.azure.com/fluidframework/internal/_build/results?buildId=423091&view=results) Please focus on whether any compact snapshot parsing operation depends on the backing buffer starting at offset zero. The relevant string positions are relative to the supplied `Uint8Array` view, and the added regression test exercises the parser using a non-zero-offset view and compares the fully parsed result with the original tree. The original assertion author was Vlad Sudzilouski (`vladsud`) in zero-offset invariant.
github-actions
Bot
added
area: tools
area: driver
Contributor
|
Warning WARNING: This PR is targeting a release branch! All changes must first be merged into Changes to release branches require approval from the Patch Triage group before merging. For more details, see our internal documentation for the patch policy and processes for |
Contributor
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (27 lines, 3 files), I've queued these reviewers:
How this works
|
Jason Hartman (jason-ha)
approved these changes
Sep 10, 2026
Contributor
|
Should this have a small changeset to note the fix? |
Contributor
Author
yeah i didn't think of that in the main pr, will make it there and add that commit to this backport pr. |
shlevari
approved these changes
Sep 11, 2026
…soft#28203) ## Description Adds the missing changeset for the compact snapshot parsing fix in [PR microsoft#28194](microsoft#28194). Although the implementation change is internal, it fixes a consumer-visible compatibility issue where valid compact snapshots could fail to load with pooled buffers returned by Node.js 24.18 and later.
Lindsey Nguyen (lindsnguyen)
requested a review
from a team
as a code owner
September 11, 2026 00:27
Contributor
Bundle size comparisonBase commit: Pending — |
Lindsey Nguyen (lindsnguyen)
enabled auto-merge (squash)
September 11, 2026 00:27
shlevari
self-requested a review
September 11, 2026 00:27
shlevari
approved these changes
Sep 11, 2026
jzaffiro
approved these changes
Sep 11, 2026
Lindsey Nguyen (lindsnguyen)
merged commit Sep 11, 2026
58071c4
into
microsoft:release/client/3.0
34 checks passed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Backport for the ODSP compact snapshot parser fix to support non-zero-offset buffers exposed by Node 24.18+.
PR for same change into main: #28194 (root cause and validation)