Skip to content

Commit 517b4ef

Browse files
lindsnguyenlindsnguyen
authored
fix(odsp-driver): remove invalid byteOffset === 0 assumption (#28194)
## Description 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`. ### Update: Why this started failing now 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. ### Root cause The assertion was originally added by Vlad Sudzilouski in #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. ### Changes - 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. ### Reproduction and integration validation 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) ## Reviewer Guidance 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 #12058. The original PR does not contain an explanation of a required zero-offset invariant.
1 parent c1f54db commit 517b4ef

2 files changed

Lines changed: 17 additions & 1 deletion

File tree

‎packages/drivers/odsp-driver/src/test/zipItDataRepresentationTests.spec.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,23 @@ describe("Tree Representation tests", () => {
120120
validate(2 + 5);
121121
});
122122

123+
it("loads from a Uint8Array with a non-zero byte offset", () => {
124+
builder.addString("first");
125+
builder.addBlob(createLongBuffer(3));
126+
127+
const serialized = builder.serialize();
128+
const backingBuffer = new Uint8Array(serialized.length + 2);
129+
backingBuffer.set(serialized, 1);
130+
const offsetBuffer = backingBuffer.subarray(1, serialized.length + 1);
131+
assert.notStrictEqual(offsetBuffer.byteOffset, 0, "Test buffer should have a byte offset");
132+
133+
const builder2 = TreeBuilder.load(
134+
new ReadBuffer(offsetBuffer),
135+
logger.toTelemetryLogger(),
136+
).builder;
137+
compareNodes(builder, builder2);
138+
});
139+
123140
it("small const string", async () => {
124141
builder.addDictionaryString("first");
125142
validate(8 + 2);

‎packages/drivers/odsp-driver/src/zipItDataRepresentationUtils.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -541,7 +541,6 @@ export class NodeCore {
541541

542542
length = 0;
543543
const input = buffer.buffer;
544-
assert(input.byteOffset === 0, 0x3e8 /* code below assumes no offset */);
545544

546545
for (const el of stringsToResolve) {
547546
for (let it = el.startPos; it < el.endPos; it++) {

0 commit comments

Comments
 (0)