From 6f0b16953f76c172bda74141d1e6d62fbfab45f7 Mon Sep 17 00:00:00 2001 From: Abram Sanderson Date: Thu, 27 Jul 2023 10:41:07 -0700 Subject: [PATCH] Fix handling of SharedDirectory ops on subdirs created on a detached data store (#16582) ## Description SharedDirectory was using -1 all the time for locally created subdirectories. This leads to bugs when considering the logic in isMessageForCurrentInstanceOfSubDirectory when the SharedDirectory is additionally detached: no ack op will ever be received for that subdir (since an op is never sent), but clients who load using the attachment summary may start sending ops which refers to the same subdirectory. merge-tree has a similar problem which is resolved by using -1 or 0 depending on the attach state of the DDS, see UnassignedSequenceNumber vs UniversalSequenceNumber. The fix here brings SharedDirectory handling closer to merge-tree's. Cherry-pick of #16578 --- packages/dds/map/src/directory.ts | 20 +++++++++++++++++-- .../dds/map/src/test/mocha/directory.spec.ts | 20 ++++++++++++------- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/packages/dds/map/src/directory.ts b/packages/dds/map/src/directory.ts index 4b52a6379b05..e862114e5964 100644 --- a/packages/dds/map/src/directory.ts +++ b/packages/dds/map/src/directory.ts @@ -1250,7 +1250,7 @@ class SubDirectory extends TypedEventEmitter implements IDirec const isNew = this.createSubDirectoryCore( subdirName, true, - -1, + this.getLocalSeq(), this.runtime.clientId ?? "detached", ); const subDir = this._subdirectories.get(subdirName); @@ -1274,6 +1274,17 @@ class SubDirectory extends TypedEventEmitter implements IDirec return subDir; } + /** + * @returns A sequenceNumber which should be used for local changes. + * @remarks - While detached, 0 is used rather than -1 to represent a change which should be universally known (as opposed to known + * only by the local client). This ensures that if the directory is later attached, none of its data needs to be updated (the values + * last set while detached will now be known to any new client, until they are changed). + * TODO: Convert these conventions to named constants. The semantics used here match those for merge-tree. + */ + private getLocalSeq(): number { + return this.directory.isAttached() ? -1 : 0; + } + /** * {@inheritDoc IDirectory.getSubDirectory} */ @@ -1661,7 +1672,12 @@ class SubDirectory extends TypedEventEmitter implements IDirec ): ICreateSubDirLocalOpMetadata { this.throwIfDisposed(); // Create the sub directory locally first. - this.createSubDirectoryCore(op.subdirName, true, -1, this.runtime.clientId ?? "detached"); + this.createSubDirectoryCore( + op.subdirName, + true, + this.getLocalSeq(), + this.runtime.clientId ?? "detached", + ); this.updatePendingSubDirMessageCount(op); const localOpMetadata: ICreateSubDirLocalOpMetadata = { diff --git a/packages/dds/map/src/test/mocha/directory.spec.ts b/packages/dds/map/src/test/mocha/directory.spec.ts index fedf1f7e69bf..2de0b5c958d2 100644 --- a/packages/dds/map/src/test/mocha/directory.spec.ts +++ b/packages/dds/map/src/test/mocha/directory.spec.ts @@ -18,6 +18,7 @@ import { import { MapFactory } from "../../map"; import { DirectoryFactory, IDirectoryNewStorageFormat, SharedDirectory } from "../../directory"; import { IDirectory, IDirectoryValueChanged, ISharedMap } from "../../interfaces"; +import { assertEquivalentDirectories } from "./directoryEquivalenceUtils"; /* eslint-disable @typescript-eslint/no-unsafe-member-access */ @@ -444,7 +445,7 @@ describe("Directory", () => { const subMapHandleUrl = subMap.handle.absolutePath; const serialized = serialize(directory); - const expected = `{"ci":{"csn":0,"ccIds":[]},"storage":{"first":{"type":"Plain","value":"second"},"third":{"type":"Plain","value":"fourth"},"fifth":{"type":"Plain","value":"sixth"},"object":{"type":"Plain","value":{"type":"__fluid_handle__","url":"${subMapHandleUrl}"}}},"subdirectories":{"nested":{"ci":{"csn":-1,"ccIds":["${dataStoreRuntime.clientId}"]},"storage":{"deepKey1":{"type":"Plain","value":"deepValue1"}},"subdirectories":{"nested2":{"ci":{"csn":-1,"ccIds":["${dataStoreRuntime.clientId}"]},"subdirectories":{"nested3":{"ci":{"csn":-1,"ccIds":["${dataStoreRuntime.clientId}"]},"storage":{"deepKey2":{"type":"Plain","value":"deepValue2"}}}}}}}}}`; + const expected = `{"ci":{"csn":0,"ccIds":[]},"storage":{"first":{"type":"Plain","value":"second"},"third":{"type":"Plain","value":"fourth"},"fifth":{"type":"Plain","value":"sixth"},"object":{"type":"Plain","value":{"type":"__fluid_handle__","url":"${subMapHandleUrl}"}}},"subdirectories":{"nested":{"ci":{"csn":0,"ccIds":["${dataStoreRuntime.clientId}"]},"storage":{"deepKey1":{"type":"Plain","value":"deepValue1"}},"subdirectories":{"nested2":{"ci":{"csn":0,"ccIds":["${dataStoreRuntime.clientId}"]},"subdirectories":{"nested3":{"ci":{"csn":0,"ccIds":["${dataStoreRuntime.clientId}"]},"storage":{"deepKey2":{"type":"Plain","value":"deepValue2"}}}}}}}}}`; assert.equal(serialized, expected); }); @@ -466,7 +467,7 @@ describe("Directory", () => { const subMapHandleUrl = subMap.handle.absolutePath; const serialized = serialize(directory); - const expected = `{"ci":{"csn":0,"ccIds":[]},"storage":{"first":{"type":"Plain","value":"second"},"third":{"type":"Plain","value":"fourth"},"fifth":{"type":"Plain"},"object":{"type":"Plain","value":{"type":"__fluid_handle__","url":"${subMapHandleUrl}"}}},"subdirectories":{"nested":{"ci":{"csn":-1,"ccIds":["${dataStoreRuntime.clientId}"]},"storage":{"deepKey1":{"type":"Plain","value":"deepValue1"},"deepKeyUndefined":{"type":"Plain"}},"subdirectories":{"nested2":{"ci":{"csn":-1,"ccIds":["${dataStoreRuntime.clientId}"]},"subdirectories":{"nested3":{"ci":{"csn":-1,"ccIds":["${dataStoreRuntime.clientId}"]},"storage":{"deepKey2":{"type":"Plain","value":"deepValue2"}}}}}}}}}`; + const expected = `{"ci":{"csn":0,"ccIds":[]},"storage":{"first":{"type":"Plain","value":"second"},"third":{"type":"Plain","value":"fourth"},"fifth":{"type":"Plain"},"object":{"type":"Plain","value":{"type":"__fluid_handle__","url":"${subMapHandleUrl}"}}},"subdirectories":{"nested":{"ci":{"csn":0,"ccIds":["${dataStoreRuntime.clientId}"]},"storage":{"deepKey1":{"type":"Plain","value":"deepValue1"},"deepKeyUndefined":{"type":"Plain"}},"subdirectories":{"nested2":{"ci":{"csn":0,"ccIds":["${dataStoreRuntime.clientId}"]},"subdirectories":{"nested3":{"ci":{"csn":0,"ccIds":["${dataStoreRuntime.clientId}"]},"storage":{"deepKey2":{"type":"Plain","value":"deepValue2"}}}}}}}}}`; assert.equal(serialized, expected); }); }); @@ -859,7 +860,7 @@ describe("Directory", () => { ); }); - it("should correctly process a sub directory operation sent in local state", async () => { + it("should correctly process subdirectory operations sent in local state", async () => { // Set the data store runtime to local. dataStoreRuntime.local = true; @@ -899,10 +900,15 @@ describe("Directory", () => { directory.getSubDirectory(subDirName), "The first directory does not have sub directory", ); - assert.ok( - directory2.getSubDirectory(subDirName), - "The second directory does not have sub directory", - ); + const subDir2 = directory2.getSubDirectory(subDirName); + + assert.ok(subDir2, "The second directory does not have sub directory"); + + subDir2.set("foo", "bar"); + + containerRuntimeFactory.processAllMessages(); + + assertEquivalentDirectories(directory, directory2); // Delete the subdirectory in the second SharedDirectory. directory2.deleteSubDirectory(subDirName);