From 072c6f348324909263963f80f2873295f4742c61 Mon Sep 17 00:00:00 2001 From: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com> Date: Tue, 25 Jun 2024 16:03:40 -0700 Subject: [PATCH] Stop using short data store IDs in detached container (#21613) ## 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 | 39 ++++++----- .../src/test/idCompressor.spec.ts | 66 ++++++++++++++++--- 3 files changed, 104 insertions(+), 36 deletions(-) diff --git a/packages/runtime/container-runtime/src/channelCollection.ts b/packages/runtime/container-runtime/src/channelCollection.ts index 71d55dcbf7ae4..1f2dcabe5df83 100644 --- a/packages/runtime/container-runtime/src/channelCollection.ts +++ b/packages/runtime/container-runtime/src/channelCollection.ts @@ -70,6 +70,7 @@ import { extractSafePropertiesFromMessage, tagCodeArtifacts, } from "@fluidframework/telemetry-utils/internal"; +import { v4 as uuid } from "uuid"; import { DeletedResponseHeaderKey, @@ -610,20 +611,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 aeeb07789167d..db2403e013b42 100644 --- a/packages/runtime/datastore/src/dataStoreRuntime.ts +++ b/packages/runtime/datastore/src/dataStoreRuntime.ts @@ -64,13 +64,13 @@ import { convertSummaryTreeToITree, create404Response, createResponseError, - encodeCompactIdToString, exceptionToResponse, generateHandleContextPath, processAttachMessageGCData, toFluidHandleInternal, unpackChildNodesUsedRoutes, toDeltaManagerErased, + encodeCompactIdToString, } from "@fluidframework/runtime-utils/internal"; import { ITelemetryLoggerExt, @@ -439,21 +439,30 @@ 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 753a5caaeb4d0..ecad65c6d87b7 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 @@ -7,7 +7,11 @@ import { strict as assert } from "assert"; import { stringToBuffer } from "@fluid-internal/client-utils"; import { generatePairwiseOptions } from "@fluid-private/test-pairwise-generator"; -import { ITestDataObject, describeCompat } from "@fluid-private/test-version-utils"; +import { + ITestDataObject, + TestDataObjectType, + describeCompat, +} from "@fluid-private/test-version-utils"; import type { ISharedCell } from "@fluidframework/cell/internal"; import { AttachState } from "@fluidframework/container-definitions"; import { @@ -789,7 +793,7 @@ describeCompat( }, ); -describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider) => { +describeCompat("IdCompressor Summaries", "NoCompat", (getTestObjectProvider, compatAPIs) => { let provider: ITestObjectProvider; const disableConfig: ITestContainerConfig = { runtimeOptions: { enableRuntimeIdCompressor: undefined }, @@ -934,13 +938,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", ); @@ -979,15 +995,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", ); @@ -1005,7 +1020,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"); @@ -1023,7 +1038,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 () => { @@ -1033,4 +1048,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"); + }); });