From b3e6b6bbd57222548a5d914da1220cb2c1a89be0 Mon Sep 17 00:00:00 2001 From: yann-achard-MS <97201204+yann-achard-MS@users.noreply.github.com> Date: Mon, 11 Dec 2023 10:53:01 -0800 Subject: [PATCH 1/4] fix(tree2): bug in transfer of nested changes (#18737) --- .../sequence-field/compose.ts | 46 +++++++------------ .../sequence-field/moveEffectTable.ts | 11 +++++ .../sequence-field/compose.spec.ts | 17 +++++++ .../shared-tree/fuzz/topLevel.fuzz.spec.ts | 1 - 4 files changed, 45 insertions(+), 30 deletions(-) diff --git a/experimental/dds/tree2/src/feature-libraries/sequence-field/compose.ts b/experimental/dds/tree2/src/feature-libraries/sequence-field/compose.ts index 47ef12631260..b94d0cbd0343 100644 --- a/experimental/dds/tree2/src/feature-libraries/sequence-field/compose.ts +++ b/experimental/dds/tree2/src/feature-libraries/sequence-field/compose.ts @@ -26,6 +26,7 @@ import { MoveEffect, isMoveIn, isMoveOut, + getMoveIn, } from "./moveEffectTable"; import { getInputLength, @@ -170,11 +171,24 @@ function composeMarks( moveEffects: MoveEffectTable, revisionMetadata: RevisionMetadataSource, ): Mark { - const nodeChange = composeChildChanges( + let nodeChange = composeChildChanges( baseMark.changes, newMark.changes === undefined ? undefined : tagChange(newMark.changes, newRev), composeChild, ); + if (nodeChange !== undefined) { + const baseSource = getMoveIn(baseMark); + if (baseSource !== undefined) { + setModifyAfter( + moveEffects, + getEndpoint(baseSource, undefined), + nodeChange, + newRev, + composeChild, + ); + nodeChange = undefined; + } + } if (isImpactfulCellRename(newMark, newRev, revisionMetadata)) { const newAttachAndDetach = asAttachAndDetach(newMark); const newDetachRevision = newAttachAndDetach.detach.revision ?? newRev; @@ -295,37 +309,14 @@ function composeMarks( } else if (!markHasCellEffect(baseMark)) { return withRevision(withNodeChange(newMark, nodeChange), newRev); } else if (!markHasCellEffect(newMark)) { - if (isMoveIn(baseMark) && nodeChange !== undefined) { - setModifyAfter( - moveEffects, - getEndpoint(baseMark, undefined), - nodeChange, - newRev, - composeChild, - ); - return baseMark; - } return withNodeChange(baseMark, nodeChange); } else if (areInputCellsEmpty(baseMark)) { assert(isDetach(newMark), 0x71c /* Unexpected mark type */); assert(isAttach(baseMark), 0x71d /* Expected generative mark */); - let localNodeChange = nodeChange; const attach = extractMarkEffect(baseMark); const detach = extractMarkEffect(withRevision(newMark, newRev)); - if (isMoveIn(attach) && nodeChange !== undefined) { - setModifyAfter( - moveEffects, - getEndpoint(attach, undefined), - nodeChange, - newRev, - composeChild, - ); - - localNodeChange = undefined; - } - if (isMoveIn(attach) && isMoveOut(detach)) { const finalSource = getEndpoint(attach, undefined); const finalDest = getEndpoint(detach, newRev); @@ -354,10 +345,7 @@ function composeMarks( if (areEqualCellIds(getOutputCellId(newMark, newRev, revisionMetadata), baseMark.cellId)) { // The output and input cell IDs are the same, so this mark has no effect. - return withNodeChange( - { count: baseMark.count, cellId: baseMark.cellId }, - localNodeChange, - ); + return withNodeChange({ count: baseMark.count, cellId: baseMark.cellId }, nodeChange); } return normalizeCellRename( { @@ -367,7 +355,7 @@ function composeMarks( attach, detach, }, - localNodeChange, + nodeChange, ); } else { if (isMoveMark(baseMark) && isMoveMark(newMark)) { diff --git a/experimental/dds/tree2/src/feature-libraries/sequence-field/moveEffectTable.ts b/experimental/dds/tree2/src/feature-libraries/sequence-field/moveEffectTable.ts index 60126b444373..e833f114801a 100644 --- a/experimental/dds/tree2/src/feature-libraries/sequence-field/moveEffectTable.ts +++ b/experimental/dds/tree2/src/feature-libraries/sequence-field/moveEffectTable.ts @@ -97,6 +97,17 @@ export function isMoveIn(effect: MarkEffect): effect is MoveIn { return effect.type === "MoveIn"; } +export function getMoveIn(effect: MarkEffect): MoveIn | undefined { + switch (effect.type) { + case "MoveIn": + return effect; + case "AttachAndDetach": + return getMoveIn(effect.attach); + default: + return undefined; + } +} + function adjustMoveEffectBasis(effect: MoveEffectWithBasis, newBasis: MoveId): MoveEffect { if (effect.basis === newBasis) { return effect; diff --git a/experimental/dds/tree2/src/test/feature-libraries/sequence-field/compose.spec.ts b/experimental/dds/tree2/src/test/feature-libraries/sequence-field/compose.spec.ts index 8367df666f34..02cf5509c605 100644 --- a/experimental/dds/tree2/src/test/feature-libraries/sequence-field/compose.spec.ts +++ b/experimental/dds/tree2/src/test/feature-libraries/sequence-field/compose.spec.ts @@ -1104,6 +1104,23 @@ describe("SequenceField - Compose", () => { assert.deepEqual(composed, expected); }); + it("move-in+delete ○ modify", () => { + const changes = TestChange.mint([], 42); + const [mo, mi] = Mark.move(1, { revision: tag1, localId: brand(1) }); + const attachDetach = Mark.attachAndDetach( + mi, + Mark.delete(1, { revision: tag2, localId: brand(2) }), + ); + const base = makeAnonChange([mo, attachDetach]); + const modify = tagChange( + [Mark.modify(changes, { revision: tag2, localId: brand(2) })], + tag3, + ); + const actual = shallowCompose([base, modify]); + const expected = [{ ...mo, changes }, attachDetach]; + assert.deepEqual(actual, expected); + }); + it("effect management for [move, modify, move]", () => { const changes = TestChange.mint([], 42); const [mo, mi] = Mark.move(1, brand(0)); diff --git a/experimental/dds/tree2/src/test/shared-tree/fuzz/topLevel.fuzz.spec.ts b/experimental/dds/tree2/src/test/shared-tree/fuzz/topLevel.fuzz.spec.ts index a371e034fa9d..c0ab1d6979d8 100644 --- a/experimental/dds/tree2/src/test/shared-tree/fuzz/topLevel.fuzz.spec.ts +++ b/experimental/dds/tree2/src/test/shared-tree/fuzz/topLevel.fuzz.spec.ts @@ -106,7 +106,6 @@ describe("Fuzz - Top-Level", () => { saveFailures: { directory: failureDirectory, }, - skip: [42], }; createDDSFuzzSuite(model, options); }); From 90912534873c3bb3106dba77deaba0ccb81d69a8 Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 11 Dec 2023 11:18:09 -0800 Subject: [PATCH 2/4] Mark Server Protocol Handler Alpha (#18747) This is used on the alpha surface of client, so also need to be marked alpha: https://github.com/microsoft/FluidFramework/blob/b3e6b6bbd57222548a5d914da1220cb2c1a89be0/packages/loader/container-loader/src/protocol.ts#L9 Co-authored-by: Tony Murphy --- .../packages/protocol-base/api-report/protocol-base.api.md | 4 ++-- server/routerlicious/packages/protocol-base/src/protocol.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/server/routerlicious/packages/protocol-base/api-report/protocol-base.api.md b/server/routerlicious/packages/protocol-base/api-report/protocol-base.api.md index d14ae165b9b4..be8d8d0f4ca2 100644 --- a/server/routerlicious/packages/protocol-base/api-report/protocol-base.api.md +++ b/server/routerlicious/packages/protocol-base/api-report/protocol-base.api.md @@ -27,7 +27,7 @@ export function getGitMode(value: SummaryObject): string; // @internal export function getGitType(value: SummaryObject): "blob" | "tree"; -// @internal (undocumented) +// @alpha (undocumented) export interface IProtocolHandler { // (undocumented) readonly attributes: IDocumentAttributes; @@ -55,7 +55,7 @@ export interface IQuorumSnapshot { values: QuorumProposalsSnapshot["values"]; } -// @internal (undocumented) +// @alpha (undocumented) export interface IScribeProtocolState { // (undocumented) members: [string, ISequencedClient][]; diff --git a/server/routerlicious/packages/protocol-base/src/protocol.ts b/server/routerlicious/packages/protocol-base/src/protocol.ts index ef2d3b222a13..ff561977be88 100644 --- a/server/routerlicious/packages/protocol-base/src/protocol.ts +++ b/server/routerlicious/packages/protocol-base/src/protocol.ts @@ -19,7 +19,7 @@ import { import { IQuorumSnapshot, Quorum } from "./quorum"; /** - * @internal + * @alpha */ export interface IScribeProtocolState { sequenceNumber: number; @@ -30,7 +30,7 @@ export interface IScribeProtocolState { } /** - * @internal + * @alpha */ export interface IProtocolHandler { readonly quorum: IQuorum; From ea8fb42081c56bf14ce982b988b82aebeb1ad049 Mon Sep 17 00:00:00 2001 From: Kian Thompson <102998837+kian-thompson@users.noreply.github.com> Date: Mon, 11 Dec 2023 13:08:30 -0800 Subject: [PATCH 3/4] Add some comments around protection of resubmitting same op (#18708) We have mechanisms in place that help prevent ops from being resubmitted if they were successfully sequenced by service (but our client didn't have time to see the ack before disconnecting). Added comments in some key places so people interested can quickly find the code that prevents this. Example scenario: | **Step** | **clientId value** | **PendingStateManager** | |--------------------------------------------|----------------|---------------------| | Client A sends op 1 | Client A | [op 1] | | Client A disconnects | Client A | [op 1] | | Client A reconnects as Client B | Client A | [op 1] | | **Start wait to see "leave" op from Client A** | Client A | [op 1] | | See ack for op 1 | Client A | **[]** | | See "leave" for Client A | **Client B** | [] | --- .../src/connectionStateHandler.ts | 19 ++++++++++++++++--- .../container-runtime/src/containerRuntime.ts | 1 + .../src/pendingStateManager.ts | 1 + 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/loader/container-loader/src/connectionStateHandler.ts b/packages/loader/container-loader/src/connectionStateHandler.ts index fafc123e1b7d..575263ad4aaf 100644 --- a/packages/loader/container-loader/src/connectionStateHandler.ts +++ b/packages/loader/container-loader/src/connectionStateHandler.ts @@ -312,12 +312,25 @@ class ConnectionStateCatchup extends ConnectionStateHandlerPassThrough { class ConnectionStateHandler implements IConnectionStateHandler { private _connectionState = ConnectionState.Disconnected; private _pendingClientId: string | undefined; + + /** + * Tracks that we observe the "leave" op within the timeout for our previous clientId (see comment on ConnectionStateHandler class) + * ! This ensures we do not switch to a new clientId until we process all potential messages from old clientId + * ! i.e. We will always see the "leave" op for a client after we have seen all the ops it has sent + * ! This check helps prevent the same op from being resubmitted by the PendingStateManager upon reconnecting + */ private readonly prevClientLeftTimer: Timer; + + /** + * Tracks that we observe our own "join" op within the timeout after receiving a "connected" event from the DeltaManager + */ private readonly joinOpTimer: Timer; + private protocol?: IProtocolHandler; private connection?: IConnectionDetailsInternal; private _clientId?: string; + /** Track how long we waited to see "leave" op for previous clientId */ private waitEvent: PerformanceEvent | undefined; public get connectionState(): ConnectionState { @@ -453,9 +466,9 @@ class ConnectionStateHandler implements IConnectionStateHandler { 0x2e2 /* "Must only wait for leave message when clientId in quorum" */, ); - // Move to connected state only if we are in Connecting state, we have seen our join op - // and there is no timer running which means we are not waiting for previous client to leave - // or timeout has occurred while doing so. + // Move to connected state only if: + // 1. We have seen our own "join" op (i.e. for this.pendingClientId) + // 2. There is no "leave" timer running, meaning this is our first connection or the previous client has left (via this.prevClientLeftTimer) if ( this.pendingClientId !== this.clientId && this.hasMember(this.pendingClientId) && diff --git a/packages/runtime/container-runtime/src/containerRuntime.ts b/packages/runtime/container-runtime/src/containerRuntime.ts index 4e90acfb6c2d..564df80b2ff6 100644 --- a/packages/runtime/container-runtime/src/containerRuntime.ts +++ b/packages/runtime/container-runtime/src/containerRuntime.ts @@ -3743,6 +3743,7 @@ export class ContainerRuntime /** * Finds the right store and asks it to resubmit the message. This typically happens when we * reconnect and there are pending messages. + * ! Note: successfully resubmitting an op that has been successfully sequenced is not possible due to checks in the ConnectionStateHandler (Loader layer) * @param message - The original LocalContainerRuntimeMessage. * @param localOpMetadata - The local metadata associated with the original message. */ diff --git a/packages/runtime/container-runtime/src/pendingStateManager.ts b/packages/runtime/container-runtime/src/pendingStateManager.ts index d4d3adcbff52..b92650497a80 100644 --- a/packages/runtime/container-runtime/src/pendingStateManager.ts +++ b/packages/runtime/container-runtime/src/pendingStateManager.ts @@ -331,6 +331,7 @@ export class PendingStateManager implements IDisposable { /** * Called when the Container's connection state changes. If the Container gets connected, it replays all the pending * states in its queue. This includes triggering resubmission of unacked ops. + * ! Note: successfully resubmitting an op that has been successfully sequenced is not possible due to checks in the ConnectionStateHandler (Loader layer) */ public replayPendingStates() { assert( From 0041dd3c0b6ea4e85ef5ff461900f5478a2a80c3 Mon Sep 17 00:00:00 2001 From: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com> Date: Mon, 11 Dec 2023 13:30:05 -0800 Subject: [PATCH 4/4] GC: Ignore incoming changes to data store that has been deleted (#18731) After a data store is deleted as a result of sweep, further modification to it must be prevented to cause any data loss. This has been added via a GC sweep op which will be received by all clients https://github.com/microsoft/FluidFramework/pull/18632. However, due to an unexpected bug, a client could have a (to be) deleted data store loaded and before it receives the GC sweep op, this data store can send an op or signal. These ops must be ignored after the data store is deleted to not cause document corruption. This PR adds this logic. An error is still logged and it can be used to identify if such case ever happens. AB#6113 --- .../container-runtime/src/dataStores.ts | 155 ++++++++++++------ .../src/test/gc/gcSweepDataStores.spec.ts | 124 ++++---------- 2 files changed, 141 insertions(+), 138 deletions(-) diff --git a/packages/runtime/container-runtime/src/dataStores.ts b/packages/runtime/container-runtime/src/dataStores.ts index 500137eb42fd..f6249a820db4 100644 --- a/packages/runtime/container-runtime/src/dataStores.ts +++ b/packages/runtime/container-runtime/src/dataStores.ts @@ -61,13 +61,8 @@ import { } from "./dataStoreContext"; import { StorageServiceWithAttachBlobs } from "./storageServiceWithAttachBlobs"; import { IDataStoreAliasMessage, isDataStoreAliasMessage } from "./dataStore"; -import { GCNodeType, disableDatastoreSweepKey, sendGCUnexpectedUsageEvent } from "./gc"; -import { - summarizerClientType, - IContainerRuntimeMetadata, - nonDataStorePaths, - rootHasIsolatedChannels, -} from "./summary"; +import { GCNodeType, disableDatastoreSweepKey } from "./gc"; +import { IContainerRuntimeMetadata, nonDataStorePaths, rootHasIsolatedChannels } from "./summary"; type PendingAliasResolve = (success: boolean) => void; @@ -281,6 +276,19 @@ export class DataStores implements IDisposable { } const context = this.contexts.get(aliasMessage.internalId); + // If the data store has been deleted, log an error and ignore this message. This helps prevent document + // corruption in case a deleted data store accidentally submitted a signal. + if ( + this.checkAndLogIfDeleted( + aliasMessage.internalId, + context, + "Changed", + "processAliasMessageCore", + ) + ) { + return false; + } + if (context === undefined) { this.mc.logger.sendErrorEvent({ eventName: "AliasFluidDataStoreNotFound", @@ -382,18 +390,43 @@ export class DataStores implements IDisposable { public resubmitDataStoreOp(envelope: IEnvelope, localOpMetadata: unknown) { const context = this.contexts.get(envelope.address); + // If the data store has been deleted, log an error and throw an error. If there are local changes for a + // deleted data store, it can otherwise lead to inconsistent state when compared to other clients. + if ( + this.checkAndLogIfDeleted(envelope.address, context, "Changed", "resubmitDataStoreOp") + ) { + throw new DataCorruptionError("Context is deleted!", { + callSite: "resubmitDataStoreOp", + ...tagCodeArtifacts({ id: envelope.address }), + }); + } assert(!!context, 0x160 /* "There should be a store context for the op" */); context.reSubmit(envelope.contents, localOpMetadata); } public rollbackDataStoreOp(envelope: IEnvelope, localOpMetadata: unknown) { const context = this.contexts.get(envelope.address); + // If the data store has been deleted, log an error and throw an error. If there are local changes for a + // deleted data store, it can otherwise lead to inconsistent state when compared to other clients. + if ( + this.checkAndLogIfDeleted(envelope.address, context, "Changed", "rollbackDataStoreOp") + ) { + throw new DataCorruptionError("Context is deleted!", { + callSite: "rollbackDataStoreOp", + ...tagCodeArtifacts({ id: envelope.address }), + }); + } assert(!!context, 0x2e8 /* "There should be a store context for the op" */); context.rollback(envelope.contents, localOpMetadata); } public async applyStashedOp(envelope: IEnvelope): Promise { const context = this.contexts.get(envelope.address); + // If the data store has been deleted, log an error and ignore this message. This helps prevent document + // corruption in case the data store that stashed the op is deleted. + if (this.checkAndLogIfDeleted(envelope.address, context, "Changed", "applyStashedOp")) { + return undefined; + } assert(!!context, 0x161 /* "There should be a store context for the op" */); return context.applyStashedOp(envelope.contents); } @@ -411,8 +444,21 @@ export class DataStores implements IDisposable { ) { const envelope = message.contents as IEnvelope; const transformed = { ...message, contents: envelope.contents }; - this.validateNotDeleted(envelope.address); const context = this.contexts.get(envelope.address); + + // If the data store has been deleted, log an error and ignore this message. This helps prevent document + // corruption in case a deleted data store accidentally submitted an op. + if ( + this.checkAndLogIfDeleted( + envelope.address, + context, + "Changed", + "processFluidDataStoreOp", + ) + ) { + return; + } + assert(!!context, 0x162 /* "There should be a store context for the op" */); context.process(transformed, local, localMessageMetadata); @@ -430,7 +476,22 @@ export class DataStores implements IDisposable { requestHeaderData: RuntimeHeaderData, ): Promise { const headerData = { ...defaultRuntimeHeaderData, ...requestHeaderData }; - this.validateNotDeleted(id, headerData); + if ( + this.checkAndLogIfDeleted( + id, + this.contexts.get(id), + "Requested", + "getDataStore", + requestHeaderData, + ) + ) { + // The requested data store has been deleted by gc. Create a 404 response exception. + const request: IRequest = { url: id }; + throw responseToException( + createResponseError(404, "DataStore was deleted", request), + request, + ); + } const context = await this.contexts.getBoundOrRemoted(id, headerData.wait); if (context === undefined) { @@ -448,8 +509,16 @@ export class DataStores implements IDisposable { id: string, requestHeaderData: RuntimeHeaderData, ): Promise { - // If the data store has been deleted, return undefined. - if (this.checkIfDeleted(id, requestHeaderData)) { + // If the data store has been deleted, log an error and return undefined. + if ( + this.checkAndLogIfDeleted( + id, + this.contexts.get(id), + "Requested", + "getDataStoreIfAvailable", + requestHeaderData, + ) + ) { return undefined; } const headerData = { ...defaultRuntimeHeaderData, ...requestHeaderData }; @@ -461,55 +530,43 @@ export class DataStores implements IDisposable { } /** - * Checks if the data store has been deleted by GC. - * @param id - data store id - * @param request - the request information to log if the validation detects the data store has been deleted - * @param requestHeaderData - the request header information to log if the validation detects the data store has been deleted + * Checks if the data store has been deleted by GC. If so, log an error. + * @param id - The data store's id. + * @param context - The data store context. + * @param callSite - The function name this is called from. + * @param requestHeaderData - The request header information to log if the data store is deleted. * @returns true if the data store is deleted. Otherwise, returns false. */ - private checkIfDeleted(id: string, requestHeaderData?: RuntimeHeaderData) { + private checkAndLogIfDeleted( + id: string, + context: FluidDataStoreContext | undefined, + deletedLogSuffix: string, + callSite: string, + requestHeaderData?: RuntimeHeaderData, + ) { const dataStoreNodePath = `/${id}`; if (!this.isDataStoreDeleted(dataStoreNodePath)) { return false; } - assert( - !this.contexts.has(id), - 0x570 /* Inconsistent state! GC says the data store is deleted, but the data store is not deleted from the runtime. */, - ); - sendGCUnexpectedUsageEvent( - this.mc, - { - eventName: "GC_Deleted_DataStore_Requested", - category: "error", - isSummarizerClient: this.runtime.clientDetails.type === summarizerClientType, - id, - headers: JSON.stringify(requestHeaderData), - gcTombstoneEnforcementAllowed: this.runtime.gcTombstoneEnforcementAllowed, - }, - undefined /* packagePath */, - ); - return true; - } - /** - * Validate that the data store had not been deleted by GC. - * @param id - data store id - * @param requestHeaderData - the request header information to log if the validation detects the data store has been deleted - */ - private validateNotDeleted(id: string, requestHeaderData?: RuntimeHeaderData) { - if (this.checkIfDeleted(id, requestHeaderData)) { - // The requested data store is removed by gc. Create a 404 gc response exception. - const request: IRequest = { url: id }; - throw responseToException( - createResponseError(404, "DataStore was deleted", request), - request, - ); - } + this.mc.logger.sendErrorEvent({ + eventName: `GC_Deleted_DataStore_${deletedLogSuffix}`, + ...tagCodeArtifacts({ id }), + callSite, + headers: JSON.stringify(requestHeaderData), + exists: context !== undefined, + }); + return true; } public processSignal(fluidDataStoreId: string, message: IInboundSignalMessage, local: boolean) { - this.validateNotDeleted(fluidDataStoreId); const context = this.contexts.get(fluidDataStoreId); + // If the data store has been deleted, log an error and ignore this message. This helps prevent document + // corruption in case a deleted data store accidentally submitted a signal. + if (this.checkAndLogIfDeleted(fluidDataStoreId, context, "Changed", "processSignal")) { + return; + } + if (!context) { // Attach message may not have been processed yet assert(!local, 0x163 /* "Missing datastore for local signal" */); diff --git a/packages/test/test-end-to-end-tests/src/test/gc/gcSweepDataStores.spec.ts b/packages/test/test-end-to-end-tests/src/test/gc/gcSweepDataStores.spec.ts index 7c6fcd7931ef..554763b7bac3 100644 --- a/packages/test/test-end-to-end-tests/src/test/gc/gcSweepDataStores.spec.ts +++ b/packages/test/test-end-to-end-tests/src/test/gc/gcSweepDataStores.spec.ts @@ -20,7 +20,6 @@ import { waitForContainerConnection, mockConfigProvider, ITestContainerConfig, - timeoutPromise, } from "@fluidframework/test-utils"; import { describeCompat, @@ -169,28 +168,6 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) }; }; - /** - * Returns a promise that will resolve when all the passed in containers have closed dur - * to "DataStore was deleted" error. - */ - const getContainersClosePromise = async (containers: IContainer[]) => { - const closePromises: Promise[] = []; - for (const container of containers) { - const promise = timeoutPromise((resolve, reject) => { - const listener = (error: IErrorBase | undefined) => { - if (!error?.message.startsWith("DataStore was deleted:")) { - reject(error); - } - container.off("closed", listener); - resolve(); - }; - container.on("closed", listener); - }); - closePromises.push(promise); - } - return Promise.all(closePromises); - }; - describe("Using swept data stores not allowed", () => { // If this test starts failing due to runtime is closed errors try first adjusting `sweepTimeoutMs` above itExpects( @@ -203,6 +180,7 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) { eventName: "fluid:telemetry:FluidDataStoreContext:GC_Deleted_DataStore_Changed", clientType: "noninteractive/summarizer", + callSite: "submitMessage", }, ], async () => { @@ -238,6 +216,7 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) { eventName: "fluid:telemetry:FluidDataStoreContext:GC_Deleted_DataStore_Changed", clientType: "noninteractive/summarizer", + callSite: "submitSignal", }, ], async () => { @@ -262,10 +241,9 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) ); }); - describe("Loading swept data stores not allowed", () => { - // TODO: Receive ops scenarios - loaded before and loaded after (are these just context loading errors?) + describe("Using deleted data stores", () => { itExpects( - "Requesting swept datastores fails in client loaded after sweep timeout and summarizing container", + "Requesting swept datastores not allowed", [ { eventName: "fluid:telemetry:Summarizer:Running:SweepReadyObject_Loaded", @@ -274,11 +252,13 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) { eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Requested", clientType: "interactive", + callSite: "getDataStore", }, - // Summarizer client's request + // Summarizer client's request logs an error { eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Requested", clientType: "noninteractive/summarizer", + callSite: "getDataStore", }, ], async () => { @@ -336,35 +316,26 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) ); itExpects( - "Receiving ops for swept datastores fails in client after sweep timeout and summarizing container", + "Ops for swept data stores is ignored but logs an error", [ { eventName: "fluid:telemetry:Summarizer:Running:SweepReadyObject_Loaded", clientType: "noninteractive/summarizer", }, { - eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Requested", - clientType: "noninteractive/summarizer", - }, - { - eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Requested", - clientType: "interactive", - }, - { - eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Requested", - clientType: "interactive", - }, - { - eventName: "fluid:telemetry:Container:ContainerClose", + eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Changed", clientType: "noninteractive/summarizer", + callSite: "processFluidDataStoreOp", }, { - eventName: "fluid:telemetry:Container:ContainerClose", + eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Changed", clientType: "interactive", + callSite: "processFluidDataStoreOp", }, { - eventName: "fluid:telemetry:Container:ContainerClose", + eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Changed", clientType: "interactive", + callSite: "processFluidDataStoreOp", }, ], async () => { @@ -391,12 +362,6 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) const { summaryVersion } = await summarize(summarizer); const receivingContainer = await loadContainer(summaryVersion); - const closePromise = getContainersClosePromise([ - sendingContainer, - summarizingContainer, - receivingContainer, - ]); - // Send an op to the swept data store dataObject._root.set("send", "op"); @@ -405,54 +370,44 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) // Wait for the GC and the above op to be processed which will close all the containers. await provider.ensureSynchronized(); - await closePromise; - // The containers should close + // The containers should not close assert( - sendingContainer.closed, - "Sending container with deleted datastore should close on receiving an op for it", + !sendingContainer.closed, + "Sending container should not close on receiving an op for deleted data store", ); assert( - summarizingContainer.closed, - "Summarizing container with deleted datastore should close on receiving an op for it", + !summarizingContainer.closed, + "Summarizing container should not close on receiving an op for deleted data store", ); assert( - receivingContainer.closed, - "Container with deleted datastore should close on receiving an op for it", + !receivingContainer.closed, + "Receiving container should close on receiving an op for deleted data store", ); }, ); itExpects( - "Receiving signals for swept datastores fails in client after sweep timeout and summarizing container", + "Signals for swept datastores are ignored but logs an error", [ { eventName: "fluid:telemetry:Summarizer:Running:SweepReadyObject_Loaded", clientType: "noninteractive/summarizer", }, { - eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Requested", - clientType: "noninteractive/summarizer", - }, - { - eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Requested", - clientType: "interactive", - }, - { - eventName: "fluid:telemetry:Container:ContainerClose", + eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Changed", clientType: "noninteractive/summarizer", + callSite: "processSignal", }, { - eventName: "fluid:telemetry:Container:ContainerClose", - clientType: "interactive", - }, - { - eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Requested", + eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Changed", clientType: "interactive", + callSite: "processSignal", }, { - eventName: "fluid:telemetry:Container:ContainerClose", + eventName: "fluid:telemetry:ContainerRuntime:GC_Deleted_DataStore_Changed", clientType: "interactive", + callSite: "processSignal", }, ], async () => { @@ -483,12 +438,6 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) const { summaryVersion } = await summarize(summarizer); const receivingContainer = await loadContainer(summaryVersion); - const closePromise = getContainersClosePromise([ - sendingContainer, - summarizingContainer, - receivingContainer, - ]); - // Send a signal to the swept data store dataObject._runtime.submitSignal("a", "signal"); @@ -499,21 +448,18 @@ describeCompat("GC data store sweep tests", "NoCompat", (getTestObjectProvider) // Once the GC op has been processed, resume the inbound signal queue so that the signal is processed. sendingContainer.deltaManager.inboundSignal.resume(); - // All the containers should have closed now. - await closePromise; - - // The containers should close + // The containers should not close assert( - sendingContainer.closed, - "Sending container with deleted datastore should close on receiving an op for it", + !sendingContainer.closed, + "Sending container should not close on receiving a signal for deleted data store", ); assert( - summarizingContainer.closed, - "Summarizing container with deleted datastore should close on receiving a signal for it", + !summarizingContainer.closed, + "Summarizing container should not close on receiving a signal for deleted data store", ); assert( - receivingContainer.closed, - "Container with deleted datastore should close on receiving a signal for it", + !receivingContainer.closed, + "Receiving container should not close on receiving a signal for deleted data store", ); }, );