From 0e59dc8fbe3a9168e1ff01e863ede7c64a8182ed Mon Sep 17 00:00:00 2001 From: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com> Date: Tue, 25 Jun 2024 17:33:02 -0700 Subject: [PATCH] [Port rc 3.0] Stop using short data store IDs in detached container (#21613) (#21628) Porting https://github.com/microsoft/FluidFramework/commit/1c00d7e38b430c6c9df201536a95b2723a75c3bc to rc 3 branch. ## Bug Currently, data stores created in detached container always get a short ID via id compressor. We have a bug in one of our customer's app where a short data store ID created is [ but in a downloaded snapshot, it is converted to its ASCII equivalent %5B in certain conditions. So, when an op comes for this data store with id [, containers loaded with this snapshot cannot find the data store. ## Root-cause We haven't yet identified what causes the id to change from [ -> %5B. However, we know that it is caused by short IDs generated in detached container. ## Fix To mitigate the issue for customers, this PR disables the generation of short IDs for data stores and DDSes. Added tests that validate that we don't generate short IDs for data stores and DDSes in detached or attached state. --- .../src/channelCollection.ts | 35 ++++++---- .../runtime/datastore/src/dataStoreRuntime.ts | 40 ++++++----- .../src/test/idCompressor.spec.ts | 66 ++++++++++++++++--- 3 files changed, 105 insertions(+), 36 deletions(-) diff --git a/packages/runtime/container-runtime/src/channelCollection.ts b/packages/runtime/container-runtime/src/channelCollection.ts index 5d332091601f..92447d28153f 100644 --- a/packages/runtime/container-runtime/src/channelCollection.ts +++ b/packages/runtime/container-runtime/src/channelCollection.ts @@ -62,6 +62,7 @@ import { extractSafePropertiesFromMessage, tagCodeArtifacts, } from "@fluidframework/telemetry-utils/internal"; +import { v4 as uuid } from "uuid"; import { DeletedResponseHeaderKey, @@ -593,20 +594,28 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable { * Please note that above mentioned functions have the implementation they have (allowing #2) due to #1. */ protected createDataStoreId(): string { - // We use three non-overlapping namespaces: - // - detached state: even numbers - // - attached state: odd numbers - // - uuids - // In first two cases we will encode result as strings in more compact form. - if (this.parentContext.attachState === AttachState.Detached) { - // container is detached, only one client observes content, no way to hit collisions with other clients. - return encodeCompactIdToString(2 * this.contexts.size); - } - const id = this.parentContext.containerRuntime.generateDocumentUniqueId(); - if (typeof id === "number") { - return encodeCompactIdToString(2 * id + 1); + /** + * There is currently a bug where certain data store ids such as "[" are getting converted to ASCII characters + * in the snapshot. + * So, return short ids only if explicitly enabled via feature flags. Else, return uuid(); + */ + if (this.mc.config.getBoolean("Fluid.Runtime.UseShortIds") === true) { + // We use three non-overlapping namespaces: + // - detached state: even numbers + // - attached state: odd numbers + // - uuids + // In first two cases we will encode result as strings in more compact form. + if (this.parentContext.attachState === AttachState.Detached) { + // container is detached, only one client observes content, no way to hit collisions with other clients. + return encodeCompactIdToString(2 * this.contexts.size); + } + const id = this.parentContext.containerRuntime.generateDocumentUniqueId(); + if (typeof id === "number") { + return encodeCompactIdToString(2 * id + 1); + } + return id; } - return id; + return uuid(); } public createDetachedDataStore( diff --git a/packages/runtime/datastore/src/dataStoreRuntime.ts b/packages/runtime/datastore/src/dataStoreRuntime.ts index 74b811ccade7..84689216804f 100644 --- a/packages/runtime/datastore/src/dataStoreRuntime.ts +++ b/packages/runtime/datastore/src/dataStoreRuntime.ts @@ -61,11 +61,11 @@ import { convertSummaryTreeToITree, create404Response, createResponseError, - encodeCompactIdToString, exceptionToResponse, generateHandleContextPath, processAttachMessageGCData, unpackChildNodesUsedRoutes, + encodeCompactIdToString, } from "@fluidframework/runtime-utils/internal"; import { ITelemetryLoggerExt } from "@fluidframework/telemetry-utils"; import { @@ -451,21 +451,31 @@ export class FluidDataStoreRuntime id = idArg; this.validateChannelId(id); } else { - // We use three non-overlapping namespaces: - // - detached state: even numbers - // - attached state: odd numbers - // - uuids - // In first two cases we will encode result as strings in more compact form, with leading underscore, - // to ensure no overlap with user-provided DDS names (see validateChannelId()) - if (this.visibilityState !== VisibilityState.GloballyVisible) { - // container is detached, only one client observes content, no way to hit collisions with other clients. - id = encodeCompactIdToString(2 * this.contexts.size, "_"); + /** + * There is currently a bug where certain data store ids such as "[" are getting converted to ASCII characters + * in the snapshot. + * So, return short ids only if explicitly enabled via feature flags. Else, return uuid(); + */ + if (this.mc.config.getBoolean("Fluid.Runtime.UseShortIds") === true) { + // We use three non-overlapping namespaces: + // - detached state: even numbers + // - attached state: odd numbers + // - uuids + // In first two cases we will encode result as strings in more compact form, with leading underscore, + // to ensure no overlap with user-provided DDS names (see validateChannelId()) + if (this.visibilityState !== VisibilityState.GloballyVisible) { + // container is detached, only one client observes content, no way to hit collisions with other clients. + id = encodeCompactIdToString(2 * this.contexts.size, "_"); + } else { + // Due to back-compat, we could not depend yet on generateDocumentUniqueId() being there. + // We can remove the need to leverage uuid() as fall-back in couple releases. + const res = + this.dataStoreContext.containerRuntime.generateDocumentUniqueId?.() ?? + uuid(); + id = typeof res === "number" ? encodeCompactIdToString(2 * res + 1, "_") : res; + } } else { - // Due to back-compat, we could not depend yet on generateDocumentUniqueId() being there. - // We can remove the need to leverage uuid() as fall-back in couple releases. - const res = - this.dataStoreContext.containerRuntime.generateDocumentUniqueId?.() ?? uuid(); - id = typeof res === "number" ? encodeCompactIdToString(2 * res + 1, "_") : res; + id = uuid(); } assert(!id.includes("/"), 0x8fc /* slash */); } diff --git a/packages/test/test-end-to-end-tests/src/test/idCompressor.spec.ts b/packages/test/test-end-to-end-tests/src/test/idCompressor.spec.ts index 731723361fe9..078470d09f49 100644 --- a/packages/test/test-end-to-end-tests/src/test/idCompressor.spec.ts +++ b/packages/test/test-end-to-end-tests/src/test/idCompressor.spec.ts @@ -6,7 +6,11 @@ import { strict as assert } from "assert"; import { stringToBuffer } from "@fluid-internal/client-utils"; -import { ITestDataObject, describeCompat } from "@fluid-private/test-version-utils"; +import { + ITestDataObject, + TestDataObjectType, + describeCompat, +} from "@fluid-private/test-version-utils"; import type { SharedCell } from "@fluidframework/cell/internal"; import { AttachState } from "@fluidframework/container-definitions"; import { IContainer, type IFluidCodeDetails } from "@fluidframework/container-definitions/internal"; @@ -773,7 +777,7 @@ describeCompat("IdCompressor in detached container", "NoCompat", (getTestObjectP }); }); -describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider) => { +describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider, compatAPIs) => { let provider: ITestObjectProvider; const disableConfig: ITestContainerConfig = { runtimeOptions: { enableRuntimeIdCompressor: undefined }, @@ -902,13 +906,25 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider) => ); }); + /** + * Function that asserts that the value is not as expected. e have a bug in one of our customer's app where a short + * data store ID created is `[` but in a downloaded snapshot, it is converted to its ASCII equivalent `%5B` in + * certain conditions. So, when an op comes for this data store with id `[`, containers loaded with this snapshot + * cannot find the data store. + * + * While we figure out the fix, we are disabling the ability to create short IDs and this assert validates it. + */ + function assertInvert(value: boolean, message: string) { + assert(!value, message); + } + async function TestCompactIds(enableRuntimeIdCompressor: IdCompressorMode) { const container = await createContainer({ runtimeOptions: { enableRuntimeIdCompressor }, }); const defaultDataStore = (await container.getEntryPoint()) as ITestDataObject; // This data store was created in detached container, so it has to be short! - assert( + assertInvert( defaultDataStore._runtime.id.length <= 2, "short data store ID created in detached container", ); @@ -947,15 +963,14 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider) => // Check directly that ID compressor is issuing short IDs! // If it does not, the rest of the tests would fail - this helps isolate where the bug is. const idTest = defaultDataStore._context.containerRuntime.generateDocumentUniqueId(); - assert(typeof idTest === "number", "short IDs should be issued"); - assert(idTest >= 0, "finalId"); + assertInvert(typeof idTest === "number" && idTest >= 0, "short IDs should be issued"); // create another datastore const ds2 = await defaultDataStore._context.containerRuntime.createDataStore(pkg); const entryPoint2 = (await ds2.entryPoint.get()) as ITestDataObject; // This data store was created in attached container, and should have used ID compressor to assign ID! - assert( + assertInvert( entryPoint2._runtime.id.length <= 2, "short data store ID created in attached container", ); @@ -970,7 +985,7 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider) => undefined, SharedDirectory.getFactory().type, ); - assert(channel.id.length <= 2, "DDS ID created in detached data store"); + assertInvert(channel.id.length <= 2, "DDS ID created in detached data store"); // attached data store. await ds2.trySetAlias("foo"); @@ -988,7 +1003,7 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider) => undefined, SharedDirectory.getFactory().type, ); - assert(channel2.id.length <= 2, "DDS ID created in attached data store"); + assertInvert(channel2.id.length <= 2, "DDS ID created in attached data store"); } it("Container uses short DataStore & DDS IDs in delayed mode", async () => { @@ -998,4 +1013,39 @@ describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider) => it("Container uses short DataStore & DDS IDs in On mode", async () => { await TestCompactIds("on"); }); + + it("always uses short data store IDs in detached container", async () => { + const loader = provider.makeTestLoader(); + const defaultCodeDetails: IFluidCodeDetails = { + package: "defaultTestPackage", + config: {}, + }; + const container = await loader.createDetachedContainer(defaultCodeDetails); + const defaultDataStore = (await container.getEntryPoint()) as ITestFluidObject; + assertInvert( + defaultDataStore.context.id.length <= 2, + "Default data store's ID should be short", + ); + const dataStore1 = + await defaultDataStore.context.containerRuntime.createDataStore(TestDataObjectType); + const ds1 = (await dataStore1.entryPoint.get()) as ITestFluidObject; + assertInvert( + ds1.context.id.length <= 2, + "Data store's ID in detached container should not be short", + ); + const dds1 = compatAPIs.dds.SharedDirectory.create(ds1.runtime); + assertInvert(dds1.id.length <= 2, "DDS's ID in detached container should not be short"); + + await container.attach(provider.driver.createCreateNewRequest()); + + const dataStore2 = + await defaultDataStore.context.containerRuntime.createDataStore(TestDataObjectType); + const ds2 = (await dataStore2.entryPoint.get()) as ITestFluidObject; + assert( + ds2.context.id.length > 8, + "Data store's ID in attached container should not be short", + ); + const dds2 = compatAPIs.dds.SharedDirectory.create(ds2.runtime); + assert(dds2.id.length > 8, "DDS's ID in attached container should not be short"); + }); });