Skip to content

Commit

Permalink
Stop using short data store IDs in detached container (#21613)
Browse files Browse the repository at this point in the history 
## 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.
  • Loading branch information
@agarwal-navin @rohandubal
agarwal-navin authored and rohandubal committed Jun 26, 2024
1 parent a0ae5ad commit 072c6f3
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 36 deletions.
35 changes: 22 additions & 13 deletions packages/runtime/container-runtime/src/channelCollection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import {
extractSafePropertiesFromMessage,
tagCodeArtifacts,
} from "@fluidframework/telemetry-utils/internal";
import { v4 as uuid } from "uuid";

import {
DeletedResponseHeaderKey,
Expand Down Expand Up @@ -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(
Expand Down
39 changes: 24 additions & 15 deletions packages/runtime/datastore/src/dataStoreRuntime.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,13 @@ import {
convertSummaryTreeToITree,
create404Response,
createResponseError,
encodeCompactIdToString,
exceptionToResponse,
generateHandleContextPath,
processAttachMessageGCData,
toFluidHandleInternal,
unpackChildNodesUsedRoutes,
toDeltaManagerErased,
encodeCompactIdToString,
} from "@fluidframework/runtime-utils/internal";
import {
ITelemetryLoggerExt,
Expand Down Expand Up @@ -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 */);
}
Expand Down
66 changes: 58 additions & 8 deletions packages/test/test-end-to-end-tests/src/test/idCompressor.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 },
Expand Down Expand Up @@ -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",
);
Expand Down Expand Up @@ -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",
);
Expand All @@ -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");
Expand All @@ -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 () => {
Expand All @@ -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");
});
});

0 comments on commit 072c6f3

Please sign in to comment.