From e4f82d77942dc4e3f876028cf537aff0ced9b88c Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 27 May 2026 15:15:13 -0700 Subject: [PATCH 1/3] perf(cell): use DoublyLinkedList for pendingMessageIds and expose list node on metadata --- packages/dds/cell/src/cell.ts | 19 +++++++++++-------- packages/dds/cell/src/interfaces.ts | 8 ++++++++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/packages/dds/cell/src/cell.ts b/packages/dds/cell/src/cell.ts index 6888040b9fc0..d08b63f9770e 100644 --- a/packages/dds/cell/src/cell.ts +++ b/packages/dds/cell/src/cell.ts @@ -3,7 +3,11 @@ * Licensed under the MIT License. */ -import { assert, unreachableCase } from "@fluidframework/core-utils/internal"; +import { + DoublyLinkedList, + assert, + unreachableCase, +} from "@fluidframework/core-utils/internal"; import type { IChannelAttributes, IFluidDataStoreRuntime, @@ -85,7 +89,7 @@ export class SharedCell */ private messageIdObserved: number = -1; - private readonly pendingMessageIds: number[] = []; + private readonly pendingMessageIds = new DoublyLinkedList(); private attribution: AttributionKey | undefined; @@ -264,9 +268,7 @@ export class SharedCell 0x00c /* "messageId is incorrect from from the local client's ACK" */, ); assert( - // eslint-disable-next-line @typescript-eslint/prefer-optional-chain -- TODO: ADO#58518 Code owners should verify if this code change is safe and make it if so or update this comment otherwise - this.pendingMessageIds !== undefined && - this.pendingMessageIds[0] === cellOpMetadata.pendingMessageId, + this.pendingMessageIds.first?.data === cellOpMetadata.pendingMessageId, 0x471 /* Unexpected pending message received */, ); this.pendingMessageIds.shift(); @@ -306,9 +308,10 @@ export class SharedCell previousValue?: Serializable, ): ICellLocalOpMetadata { const pendingMessageId = ++this.messageId; - this.pendingMessageIds.push(pendingMessageId); + const { first: pendingNode } = this.pendingMessageIds.push(pendingMessageId); const localMetadata: ICellLocalOpMetadata = { pendingMessageId, + pendingNode, previousValue, }; return localMetadata; @@ -352,8 +355,8 @@ export class SharedCell this.setCore(cellOpMetadata.previousValue as Serializable); } - const lastPendingMessageId = this.pendingMessageIds.pop(); - if (lastPendingMessageId !== cellOpMetadata.pendingMessageId) { + const lastPendingNode = this.pendingMessageIds.pop(); + if (lastPendingNode?.data !== cellOpMetadata.pendingMessageId) { throw new Error("Rollback op does not match last pending"); } } else { diff --git a/packages/dds/cell/src/interfaces.ts b/packages/dds/cell/src/interfaces.ts index c1a440a0de21..89348fc1637d 100644 --- a/packages/dds/cell/src/interfaces.ts +++ b/packages/dds/cell/src/interfaces.ts @@ -3,6 +3,7 @@ * Licensed under the MIT License. */ +import type { ListNode } from "@fluidframework/core-utils/internal"; import type { Serializable } from "@fluidframework/datastore-definitions/internal"; import type { AttributionKey } from "@fluidframework/runtime-definitions/internal"; import type { @@ -125,6 +126,13 @@ export interface ICellLocalOpMetadata { */ pendingMessageId: number; + /** + * The node in the pending message list corresponding to this operation (op). + * Holding a direct reference to the node enables O(1) removal from arbitrary + * positions in the pending list, which is required for future squash support. + */ + pendingNode: ListNode; + /** * The value of the {@link ISharedCell} prior to this operation (op). */ From d12ef64eae3a25fd915d1b5a2ebc358784725f02 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 27 May 2026 17:10:45 -0700 Subject: [PATCH 2/3] fix(cell): private metadata subtype + safer push().last + regression tests Address deep-review findings on PR #27415: - Stop leaking `ListNode` onto `ICellLocalOpMetadata`. Move `pendingNode` to a private `ICellPendingLocalOpMetadata` subtype declared inside cell.ts. The public/internal interfaces module no longer imports from `@fluidframework/core-utils/internal`, decoupling it from a runtime implementation detail. - Use `last` instead of `first` when destructuring the single-node range returned by `DoublyLinkedList.push(...)`. They are equivalent today (single-item push), but `first` would be incorrect if a future change batched multiple pending ids into one push call. - Add two regression tests in cell.spec.ts: - drain-many-ACKs: queue several local sets and ACK them incrementally via processSomeMessages(1); asserts no assert-tag fires (0x471) and final value converges across clients in order. - rollback-with-multiple-pending-ops: enqueue three pending sets under TurnBased flush mode and run containerRuntime.rollback(), confirming the LIFO pop against the expected pending id does not throw. --- packages/dds/cell/src/cell.ts | 21 ++++++-- packages/dds/cell/src/interfaces.ts | 8 --- packages/dds/cell/src/test/cell.spec.ts | 66 +++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 11 deletions(-) diff --git a/packages/dds/cell/src/cell.ts b/packages/dds/cell/src/cell.ts index d08b63f9770e..efc9b8b48140 100644 --- a/packages/dds/cell/src/cell.ts +++ b/packages/dds/cell/src/cell.ts @@ -5,6 +5,7 @@ import { DoublyLinkedList, + type ListNode, assert, unreachableCase, } from "@fluidframework/core-utils/internal"; @@ -61,6 +62,18 @@ interface ICellValue { attribution?: AttributionKey; } +/** + * Internal extension of {@link ICellLocalOpMetadata} that carries a direct reference + * to the corresponding node in the pending message list. Holding the node enables + * O(1) removal from arbitrary positions in the pending list, which is required for + * future squash support. Kept private to this module so the public metadata interface + * does not leak `ListNode` (a runtime implementation detail). + */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any +interface ICellPendingLocalOpMetadata extends ICellLocalOpMetadata { + pendingNode: ListNode; +} + const snapshotFileName = "header"; /** @@ -306,10 +319,12 @@ export class SharedCell private createLocalOpMetadata( op: ICellOperation, previousValue?: Serializable, - ): ICellLocalOpMetadata { + ): ICellPendingLocalOpMetadata { const pendingMessageId = ++this.messageId; - const { first: pendingNode } = this.pendingMessageIds.push(pendingMessageId); - const localMetadata: ICellLocalOpMetadata = { + // Use `last` so this remains correct if a future change appends multiple + // pending ids in a single push call (for the single-item case `first === last`). + const { last: pendingNode } = this.pendingMessageIds.push(pendingMessageId); + const localMetadata: ICellPendingLocalOpMetadata = { pendingMessageId, pendingNode, previousValue, diff --git a/packages/dds/cell/src/interfaces.ts b/packages/dds/cell/src/interfaces.ts index 89348fc1637d..c1a440a0de21 100644 --- a/packages/dds/cell/src/interfaces.ts +++ b/packages/dds/cell/src/interfaces.ts @@ -3,7 +3,6 @@ * Licensed under the MIT License. */ -import type { ListNode } from "@fluidframework/core-utils/internal"; import type { Serializable } from "@fluidframework/datastore-definitions/internal"; import type { AttributionKey } from "@fluidframework/runtime-definitions/internal"; import type { @@ -126,13 +125,6 @@ export interface ICellLocalOpMetadata { */ pendingMessageId: number; - /** - * The node in the pending message list corresponding to this operation (op). - * Holding a direct reference to the node enables O(1) removal from arbitrary - * positions in the pending list, which is required for future squash support. - */ - pendingNode: ListNode; - /** * The value of the {@link ISharedCell} prior to this operation (op). */ diff --git a/packages/dds/cell/src/test/cell.spec.ts b/packages/dds/cell/src/test/cell.spec.ts index 214c8d4e1cce..59686363d2a7 100644 --- a/packages/dds/cell/src/test/cell.spec.ts +++ b/packages/dds/cell/src/test/cell.spec.ts @@ -8,6 +8,7 @@ import { strict as assert } from "node:assert"; import { type IGCTestProvider, runGCTests } from "@fluid-private/test-dds-utils"; import { AttachState } from "@fluidframework/container-definitions"; import { + type MockContainerRuntime, MockContainerRuntimeFactory, MockContainerRuntimeFactoryForReconnection, type MockContainerRuntimeForReconnection, @@ -488,6 +489,71 @@ describe("Cell", () => { }); }); + describe("Pending op bookkeeping", () => { + it("drains many pending sets via incremental ACKs without assert and preserves ordering", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory(); + const cell1 = createConnectedCell("cell1", containerRuntimeFactory); + const cell2 = createConnectedCell("cell2", containerRuntimeFactory); + + const values = ["v0", "v1", "v2", "v3", "v4"]; + for (const v of values) { + cell1.set(v); + } + + // Incrementally ACK each pending op one at a time. + // This exercises the per-ACK pendingMessageIds.shift() path and would assert-fail + // (0x471 "Unexpected pending message received") if the pending list order or the + // pendingNode bookkeeping were wrong. + for (let i = 0; i < values.length; i++) { + containerRuntimeFactory.processSomeMessages(1); + // Local cell continues to reflect its latest local write throughout. + assert.equal( + cell1.get(), + values[values.length - 1], + "local cell should retain latest pending value while ACKs drain", + ); + } + + // After all ACKs, both cells must converge on the final value in order. + assert.equal(cell1.get(), values[values.length - 1], "cell1 final value"); + assert.equal(cell2.get(), values[values.length - 1], "cell2 final value"); + }); + + it("rolls back multiple pending sets in LIFO order against the expected pending id", () => { + const containerRuntimeFactory = new MockContainerRuntimeFactory({ flushMode: 1 }); + const dataStoreRuntime = new MockFluidDataStoreRuntime(); + const containerRuntime: MockContainerRuntime = + containerRuntimeFactory.createContainerRuntime(dataStoreRuntime); + const services = { + deltaConnection: dataStoreRuntime.createDeltaConnection(), + objectStorage: new MockStorage(), + }; + const cell = new SharedCell("cell-rollback", dataStoreRuntime, CellFactory.Attributes); + cell.connect(services); + + // Three pending sets; nothing has been flushed/sequenced yet. + cell.set("a"); + cell.set("b"); + cell.set("c"); + assert.equal(cell.get(), "c", "latest local value should be visible before rollback"); + + // Rolls back in LIFO order; each rollback should match the last pending id (popped from the list). + // If pendingMessageIds was tracked incorrectly, rollback() would throw + // "Rollback op does not match last pending". + assert.doesNotThrow( + () => containerRuntime.rollback?.(), + "rollback should pop pending ids in LIFO order", + ); + + // After rollback, the cell value reverts to the pre-first-set value (undefined). + assert.equal( + cell.get(), + undefined, + "cell should be empty after rolling back all pending sets", + ); + }); + }); + describe("Garbage Collection", () => { class GCSharedCellProvider implements IGCTestProvider { private subCellCount = 0; From 382f2d77012ead45c8c8467b5587e85010b4f911 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 27 May 2026 19:53:52 -0700 Subject: [PATCH 3/3] fix(cell): use for-of in pending op drain test (eslint) --- packages/dds/cell/src/test/cell.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/dds/cell/src/test/cell.spec.ts b/packages/dds/cell/src/test/cell.spec.ts index 59686363d2a7..98fef9206330 100644 --- a/packages/dds/cell/src/test/cell.spec.ts +++ b/packages/dds/cell/src/test/cell.spec.ts @@ -504,19 +504,19 @@ describe("Cell", () => { // This exercises the per-ACK pendingMessageIds.shift() path and would assert-fail // (0x471 "Unexpected pending message received") if the pending list order or the // pendingNode bookkeeping were wrong. - for (let i = 0; i < values.length; i++) { + for (const _ of values) { containerRuntimeFactory.processSomeMessages(1); // Local cell continues to reflect its latest local write throughout. assert.equal( cell1.get(), - values[values.length - 1], + values.at(-1), "local cell should retain latest pending value while ACKs drain", ); } // After all ACKs, both cells must converge on the final value in order. - assert.equal(cell1.get(), values[values.length - 1], "cell1 final value"); - assert.equal(cell2.get(), values[values.length - 1], "cell2 final value"); + assert.equal(cell1.get(), values.at(-1), "cell1 final value"); + assert.equal(cell2.get(), values.at(-1), "cell2 final value"); }); it("rolls back multiple pending sets in LIFO order against the expected pending id", () => {