diff --git a/packages/dds/cell/src/cell.ts b/packages/dds/cell/src/cell.ts index 6888040b9fc0..efc9b8b48140 100644 --- a/packages/dds/cell/src/cell.ts +++ b/packages/dds/cell/src/cell.ts @@ -3,7 +3,12 @@ * Licensed under the MIT License. */ -import { assert, unreachableCase } from "@fluidframework/core-utils/internal"; +import { + DoublyLinkedList, + type ListNode, + assert, + unreachableCase, +} from "@fluidframework/core-utils/internal"; import type { IChannelAttributes, IFluidDataStoreRuntime, @@ -57,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"; /** @@ -85,7 +102,7 @@ export class SharedCell */ private messageIdObserved: number = -1; - private readonly pendingMessageIds: number[] = []; + private readonly pendingMessageIds = new DoublyLinkedList(); private attribution: AttributionKey | undefined; @@ -264,9 +281,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(); @@ -304,11 +319,14 @@ export class SharedCell private createLocalOpMetadata( op: ICellOperation, previousValue?: Serializable, - ): ICellLocalOpMetadata { + ): ICellPendingLocalOpMetadata { const pendingMessageId = ++this.messageId; - 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, }; return localMetadata; @@ -352,8 +370,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/test/cell.spec.ts b/packages/dds/cell/src/test/cell.spec.ts index 214c8d4e1cce..98fef9206330 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 (const _ of values) { + containerRuntimeFactory.processSomeMessages(1); + // Local cell continues to reflect its latest local write throughout. + assert.equal( + cell1.get(), + 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.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", () => { + 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;