From c8fb4b594940935cdf1cf28dc95a94bf2e72c715 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Tue, 14 Jan 2025 21:06:43 +0000 Subject: [PATCH 01/54] Cherry-pick loader and container-definitions from #23268 --- .../common/container-definitions/CHANGELOG.md | 4 ++ .../container-definitions.legacy.alpha.api.md | 14 +++++- .../common/container-definitions/package.json | 4 +- .../common/container-definitions/src/index.ts | 2 +- .../container-definitions/src/loader.ts | 20 +++++++++ packages/loader/container-loader/CHANGELOG.md | 4 ++ packages/loader/container-loader/package.json | 4 +- .../container-loader/src/connectionManager.ts | 43 +++++++++++++++--- .../loader/container-loader/src/container.ts | 45 ++++++++++++------- .../loader/container-loader/src/contracts.ts | 7 +++ .../container-loader/src/deltaManager.ts | 26 ++++++----- .../loader/container-loader/src/loadPaused.ts | 5 ++- .../container-loader/src/packageVersion.ts | 2 +- .../src/test/deltaManager.spec.ts | 3 +- 14 files changed, 141 insertions(+), 42 deletions(-) diff --git a/packages/common/container-definitions/CHANGELOG.md b/packages/common/container-definitions/CHANGELOG.md index 75ca4a07dd24..c3e4b6585341 100644 --- a/packages/common/container-definitions/CHANGELOG.md +++ b/packages/common/container-definitions/CHANGELOG.md @@ -1,5 +1,9 @@ # @fluidframework/container-definitions +## 2.13.0 + +Dependency updates only. + ## 2.12.0 Dependency updates only. diff --git a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md index ca624b2574a8..d583633faa62 100644 --- a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md +++ b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md @@ -40,6 +40,16 @@ export interface ContainerWarning extends IErrorBase_2 { logged?: boolean; } +// @alpha +export const DisconnectReason: { + readonly Expected: "Expected"; + readonly Corruption: "Corruption"; + readonly Unknown: "Unknown"; +}; + +// @alpha +export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; + // @public export interface IAudience extends IEventProvider { getMember(clientId: string): IClient | undefined; @@ -98,14 +108,14 @@ export interface IContainer extends IEventProvider { readonly attachState: AttachState; readonly audience: IAudience; readonly clientId?: string | undefined; - close(error?: ICriticalContainerError): void; + close(disconnectReason: DisconnectReason, error?: ICriticalContainerError): void; readonly closed: boolean; connect(): void; readonly connectionState: ConnectionState; containerMetadata: Record; deltaManager: IDeltaManager; disconnect(): void; - dispose(error?: ICriticalContainerError): void; + dispose(disconnectReason: DisconnectReason, error?: ICriticalContainerError): void; readonly disposed?: boolean; forceReadonly?(readonly: boolean): any; getAbsoluteUrl(relativeUrl: string): Promise; diff --git a/packages/common/container-definitions/package.json b/packages/common/container-definitions/package.json index 556671911a61..f79e76dc5c11 100644 --- a/packages/common/container-definitions/package.json +++ b/packages/common/container-definitions/package.json @@ -1,6 +1,6 @@ { "name": "@fluidframework/container-definitions", - "version": "2.13.0", + "version": "2.20.0", "description": "Fluid container definitions", "homepage": "https://fluidframework.com", "repository": { @@ -102,7 +102,7 @@ "@fluid-tools/build-cli": "^0.51.0", "@fluidframework/build-common": "^2.0.3", "@fluidframework/build-tools": "^0.51.0", - "@fluidframework/container-definitions-previous": "npm:@fluidframework/container-definitions@2.12.0", + "@fluidframework/container-definitions-previous": "npm:@fluidframework/container-definitions@2.13.0", "@fluidframework/eslint-config-fluid": "^5.6.0", "@microsoft/api-extractor": "7.47.8", "concurrently": "^8.2.1", diff --git a/packages/common/container-definitions/src/index.ts b/packages/common/container-definitions/src/index.ts index 6e587f42fa65..2eba983ee51f 100644 --- a/packages/common/container-definitions/src/index.ts +++ b/packages/common/container-definitions/src/index.ts @@ -45,7 +45,7 @@ export type { IResolvedFluidCodeDetails, ISnapshotTreeWithBlobContents, } from "./loader.js"; -export { LoaderHeader } from "./loader.js"; +export { DisconnectReason, LoaderHeader } from "./loader.js"; export type { IFluidModule } from "./fluidModule.js"; export type { IFluidPackage, diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 9742b3b3f569..a193895cb077 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -271,6 +271,24 @@ export interface IContainerEvents extends IEvent { (event: "metadataUpdate", listener: (metadata: Record) => void); } +/** + * DisconnectReason of disconnect events emitted by the {@link IContainer}. + * @legacy + * @alpha + */ +export const DisconnectReason = { + Expected: "Expected", + Corruption: "Corruption", + Unknown: "Unknown", +} as const; + +/** + * {@inheritDoc (DisconnectReason:variable)} + * @legacy + * @alpha + */ +export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; + /** * Namespace for the different connection states a container can be in. * PLEASE NOTE: The sequence of the numerical values does no correspond to the typical connection state progression. @@ -387,6 +405,7 @@ export interface IContainer extends IEventProvider { * resulted in disposing it. */ dispose(error?: ICriticalContainerError): void; + dispose(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; /** * Closes the container. @@ -395,6 +414,7 @@ export interface IContainer extends IEventProvider { * resulted in closing it. */ close(error?: ICriticalContainerError): void; + close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; /** * Propose new code details that define the code to be loaded for this container's runtime. diff --git a/packages/loader/container-loader/CHANGELOG.md b/packages/loader/container-loader/CHANGELOG.md index 8115be013ca6..bb719404c0dc 100644 --- a/packages/loader/container-loader/CHANGELOG.md +++ b/packages/loader/container-loader/CHANGELOG.md @@ -1,5 +1,9 @@ # @fluidframework/container-loader +## 2.13.0 + +Dependency updates only. + ## 2.12.0 ### Minor Changes diff --git a/packages/loader/container-loader/package.json b/packages/loader/container-loader/package.json index 1190575d4841..c67f7aa2417d 100644 --- a/packages/loader/container-loader/package.json +++ b/packages/loader/container-loader/package.json @@ -1,6 +1,6 @@ { "name": "@fluidframework/container-loader", - "version": "2.13.0", + "version": "2.20.0", "description": "Fluid container loader", "homepage": "https://fluidframework.com", "repository": { @@ -192,7 +192,7 @@ "@fluid-tools/build-cli": "^0.51.0", "@fluidframework/build-common": "^2.0.3", "@fluidframework/build-tools": "^0.51.0", - "@fluidframework/container-loader-previous": "npm:@fluidframework/container-loader@2.12.0", + "@fluidframework/container-loader-previous": "npm:@fluidframework/container-loader@2.13.0", "@fluidframework/eslint-config-fluid": "^5.6.0", "@microsoft/api-extractor": "7.47.8", "@types/debug": "^4.1.5", diff --git a/packages/loader/container-loader/src/connectionManager.ts b/packages/loader/container-loader/src/connectionManager.ts index 711e00e8d25b..7d1d35f90fd1 100644 --- a/packages/loader/container-loader/src/connectionManager.ts +++ b/packages/loader/container-loader/src/connectionManager.ts @@ -5,7 +5,11 @@ import { TypedEventEmitter, performance } from "@fluid-internal/client-utils"; import { ICriticalContainerError } from "@fluidframework/container-definitions"; -import { IDeltaQueue, ReadOnlyInfo } from "@fluidframework/container-definitions/internal"; +import { + IDeltaQueue, + ReadOnlyInfo, + DisconnectReason, +} from "@fluidframework/container-definitions/internal"; import { IDisposable, ITelemetryBaseProperties, @@ -425,7 +429,11 @@ export class ConnectionManager implements IConnectionManager { }); } - public dispose(error?: ICriticalContainerError, switchToReadonly: boolean = true): void { + public dispose( + disconnectReasonOrError?: DisconnectReason | ICriticalContainerError, + errorOrSwitchToReadonly?: ICriticalContainerError | boolean, + switchToReadonly: boolean = true, + ): void { if (this._disposed) { return; } @@ -436,16 +444,37 @@ export class ConnectionManager implements IConnectionManager { this._outbound.clear(); + const text = + typeof disconnectReasonOrError === "string" + ? disconnectReasonOrError + : "Closing DeltaManager"; + + const finalSwitchToReadonly = + typeof errorOrSwitchToReadonly === "boolean" + ? errorOrSwitchToReadonly + : switchToReadonly; + + const error: ICriticalContainerError | undefined = + typeof errorOrSwitchToReadonly === "boolean" && + disconnectReasonOrError !== undefined && + typeof disconnectReasonOrError !== "string" + ? disconnectReasonOrError + : undefined; + const disconnectReason: IConnectionStateChangeReason = { - text: "Closing DeltaManager", + text, error, + disconnectReason: + typeof disconnectReasonOrError === "string" + ? disconnectReasonOrError + : DisconnectReason.Unknown, }; const oldReadonlyValue = this.readonly; // This raises "disconnect" event if we have active connection. this.disconnectFromDeltaStream(disconnectReason); - if (switchToReadonly) { + if (finalSwitchToReadonly) { // Notify everyone we are in read-only state. // Useful for data stores in case we hit some critical error, // to switch to a mode where user edits are not accepted @@ -811,7 +840,11 @@ export class ConnectionManager implements IConnectionManager { // eslint-disable-next-line @typescript-eslint/no-floating-promises this._outbound.pause(); this._outbound.clear(); - connection.dispose(); + connection.dispose({ + ...reason.error, + name: "disconnectFromDeltaStream", + message: reason.disconnectReason ?? DisconnectReason.Unknown, + }); this.props.disconnectHandler(reason); diff --git a/packages/loader/container-loader/src/container.ts b/packages/loader/container-loader/src/container.ts index 35b747f10a20..7b63639dc056 100644 --- a/packages/loader/container-loader/src/container.ts +++ b/packages/loader/container-loader/src/container.ts @@ -28,6 +28,7 @@ import { isFluidCodeDetails, IDeltaManager, ReadOnlyInfo, + DisconnectReason, type ILoader, } from "@fluidframework/container-definitions/internal"; import { @@ -421,8 +422,8 @@ export class Container // Note: We could only dispose the container instead of just close but that would // the telemetry where users sometimes search for ContainerClose event to look // for load failures. So not removing this at this time. - container.close(err); - container.dispose(err); + container.close(DisconnectReason.Unknown, err); + container.dispose(DisconnectReason.Unknown, err); onClosed(err); }, ); @@ -787,7 +788,7 @@ export class Container }, error, ); - this.close(normalizeError(error)); + this.close(DisconnectReason.Unknown, normalizeError(error)); }); const { @@ -968,7 +969,7 @@ export class Container this.clientsWhoShouldHaveLeft.add(clientId); }, onCriticalError: (error: unknown) => { - this.close(normalizeError(error)); + this.close(DisconnectReason.Unknown, normalizeError(error)); }, }, this.deltaManager, @@ -1053,16 +1054,28 @@ export class Container return this.protocolHandler.quorum; } - public dispose(error?: ICriticalContainerError): void { - this.verifyClosedAfter(() => this._deltaManager.dispose(error)); + public dispose(errorOrReason?: ICriticalContainerError | DisconnectReason, error?: ICriticalContainerError): void { + if (typeof errorOrReason === 'string') { + // Called as dispose(disconnectReason, error) + this.verifyClosedAfter(() => this._deltaManager.dispose(errorOrReason, error)); + } else { + // Called as dispose(error) + this.verifyClosedAfter(() => this._deltaManager.dispose(undefined, errorOrReason)); + } } - public close(error?: ICriticalContainerError): void { + public close(errorOrReason?: ICriticalContainerError | DisconnectReason, error?: ICriticalContainerError): void { // 1. Ensure that close sequence is exactly the same no matter if it's initiated by host or by DeltaManager // 2. We need to ensure that we deliver disconnect event to runtime properly. See connectionStateChanged // handler. We only deliver events if container fully loaded. Transitioning from "loading" -> // "closing" will lose that info (can also solve by tracking extra state). - this.verifyClosedAfter(() => this._deltaManager.close(error)); + if (typeof errorOrReason === 'string') { + // Called as close(disconnectReason, error) + this.verifyClosedAfter(() => this._deltaManager.close(errorOrReason, error)); + } else { + // Called as close(error) + this.verifyClosedAfter(() => this._deltaManager.close(undefined, errorOrReason)); + } } private verifyClosedAfterCalls = 0; @@ -1131,7 +1144,7 @@ export class Container // There is no user for summarizer, so we need to ensure dispose is called if (this.client.details.type === summarizerClientType) { - this.dispose(error); + this.dispose(DisconnectReason.Unknown, error); } } } @@ -1199,7 +1212,7 @@ export class Container notifyImminentClosure: true, stopBlobAttachingSignal, }); - this.close(); + this.close(DisconnectReason.Expected); return pendingState; } @@ -1299,7 +1312,7 @@ export class Container const normalizeErrorAndClose = (error: unknown): IFluidErrorBase => { const newError = normalizeError(error); - this.close(newError); + this.close(DisconnectReason.Unknown, newError); // add resolved URL on error object so that host has the ability to find this document and delete it newError.addTelemetryProperties({ resolvedUrl: this.service?.resolvedUrl?.url, @@ -1544,7 +1557,7 @@ export class Container // pre-0.58 error message: existingContextDoesNotSatisfyIncomingProposal const error = new GenericError("Existing context does not satisfy incoming proposal"); - this.close(error); + this.close(DisconnectReason.Unknown, error); } /** @@ -1937,7 +1950,7 @@ export class Container } this.processCodeProposal().catch((error) => { const normalizedError = normalizeError(error); - this.close(normalizedError); + this.close(DisconnectReason.Unknown, normalizedError); throw error; }); } @@ -2257,7 +2270,7 @@ export class Container undefined /* error */, { messageType: type }, ); - this.close(newError); + this.close(DisconnectReason.Unknown, newError); return -1; } } @@ -2446,8 +2459,8 @@ export class Container (batch: IBatchMessage[], referenceSequenceNumber?: number) => this.submitBatch(batch, referenceSequenceNumber), (content, targetClientId) => this.submitSignal(content, targetClientId), - (error?: ICriticalContainerError) => this.dispose(error), - (error?: ICriticalContainerError) => this.close(error), + (error?: ICriticalContainerError) => this.dispose(DisconnectReason.Unknown, error), + (error?: ICriticalContainerError) => this.close(DisconnectReason.Unknown, error), this.updateDirtyContainerState, this.getAbsoluteUrl, () => this.resolvedUrl?.id, diff --git a/packages/loader/container-loader/src/contracts.ts b/packages/loader/container-loader/src/contracts.ts index f4a38a124a16..511c3d65318c 100644 --- a/packages/loader/container-loader/src/contracts.ts +++ b/packages/loader/container-loader/src/contracts.ts @@ -10,6 +10,7 @@ import { IFluidCodeDetails, isFluidPackage, IConnectionDetails, + type DisconnectReason, } from "@fluidframework/container-definitions/internal"; import { IErrorBase, ITelemetryBaseProperties } from "@fluidframework/core-interfaces"; import { ConnectionMode, IClientDetails } from "@fluidframework/driver-definitions"; @@ -31,6 +32,7 @@ export enum ReconnectMode { export interface IConnectionStateChangeReason { text: string; error?: T; + disconnectReason?: DisconnectReason; } /** @@ -125,6 +127,11 @@ export interface IConnectionManager { /** * Disposed connection manager */ + dispose( + disconnectReason?: DisconnectReason, + error?: ICriticalContainerError, + switchToReadonly?: boolean, + ): void; dispose(error?: ICriticalContainerError, switchToReadonly?: boolean): void; get connectionMode(): ConnectionMode; diff --git a/packages/loader/container-loader/src/deltaManager.ts b/packages/loader/container-loader/src/deltaManager.ts index 450ee5084fa8..a8f8707036ef 100644 --- a/packages/loader/container-loader/src/deltaManager.ts +++ b/packages/loader/container-loader/src/deltaManager.ts @@ -8,6 +8,7 @@ import { IDeltaManagerEvents, IDeltaManagerFull, IDeltaQueue, + DisconnectReason, type IDeltaSender, type ReadOnlyInfo, } from "@fluidframework/container-definitions/internal"; @@ -422,7 +423,7 @@ export class DeltaManager }, error, ); - this.close(normalizeError(error)); + this.close(DisconnectReason.Unknown, normalizeError(error)); }); const props: IConnectionManagerFactoryArgs = { incomingOpHandler: (messages: ISequencedDocumentMessage[], reason: string) => { @@ -430,7 +431,7 @@ export class DeltaManager this.enqueueMessages(messages, reason); } catch (error) { this.logger.sendErrorEvent({ eventName: "EnqueueMessages_Exception" }, error); - this.close(normalizeError(error)); + this.close(DisconnectReason.Unknown, normalizeError(error)); } }, signalHandler: (signals: ISignalMessage[]) => { @@ -440,7 +441,8 @@ export class DeltaManager }, reconnectionDelayHandler: (delayMs: number, error: unknown) => this.emitDelayInfo(this.deltaStreamDelayId, delayMs, error), - closeHandler: (error: ICriticalContainerError | undefined) => this.close(error), + closeHandler: (error: ICriticalContainerError | undefined) => + this.close(DisconnectReason.Unknown, error), disconnectHandler: (reason: IConnectionStateChangeReason) => this.disconnectHandler(reason), connectHandler: (connection: IConnectionDetailsInternal) => @@ -465,6 +467,7 @@ export class DeltaManager this._inbound.on("error", (error) => { this.close( + DisconnectReason.Unknown, DataProcessingError.wrapIfUnrecognized( error, "deltaManagerInboundErrorHandler", @@ -486,7 +489,7 @@ export class DeltaManager }); this._inboundSignal.on("error", (error) => { - this.close(normalizeError(error)); + this.close(DisconnectReason.Unknown, normalizeError(error)); }); // Initially, all queues are created paused. @@ -759,13 +762,13 @@ export class DeltaManager * - close emits "closed" * - close cannot be called after dispose */ - public close(error?: ICriticalContainerError): void { + public close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void { if (this._closed) { return; } this._closed = true; - this.connectionManager.dispose(error, true /* switchToReadonly */); + this.connectionManager.dispose(disconnectReason, error, true /* switchToReadonly */); this.clearQueues(); this.emit("closed", error); } @@ -778,7 +781,10 @@ export class DeltaManager * - dispose will remove all listeners * - dispose can be called after closure */ - public dispose(error?: Error | ICriticalContainerError): void { + public dispose( + disconnectReason?: DisconnectReason, + error?: Error | ICriticalContainerError, + ): void { if (this._disposed) { return; } @@ -789,7 +795,7 @@ export class DeltaManager this._disposed = true; this._closed = true; // We consider "disposed" as a further state than "closed" - this.connectionManager.dispose(error, false /* switchToReadonly */); + this.connectionManager.dispose(disconnectReason, error, false /* switchToReadonly */); this.clearQueues(); // This needs to be the last thing we do (before removing listeners), as it causes @@ -991,7 +997,7 @@ export class DeltaManager driverVersion: undefined, }, ); - this.close(error); + this.close(DisconnectReason.Unknown, error); } } } else if (message.sequenceNumber === this.lastQueuedSequenceNumber + 1) { @@ -1175,7 +1181,7 @@ export class DeltaManager ); } catch (error) { this.logger.sendErrorEvent({ eventName: "GetDeltas_Exception" }, error); - this.close(normalizeError(error)); + this.close(DisconnectReason.Unknown, normalizeError(error)); } finally { this.refreshDelayInfo(this.deltaStorageDelayId); this.fetchReason = undefined; diff --git a/packages/loader/container-loader/src/loadPaused.ts b/packages/loader/container-loader/src/loadPaused.ts index c73f5aab5db8..33660a52ea46 100644 --- a/packages/loader/container-loader/src/loadPaused.ts +++ b/packages/loader/container-loader/src/loadPaused.ts @@ -4,6 +4,7 @@ */ import { + DisconnectReason, isIDeltaManagerFull, LoaderHeader, type IContainer, @@ -95,7 +96,7 @@ export async function loadContainerPaused( const error = new GenericError( "Cannot satisfy request to pause the container at the specified sequence number. Most recent snapshot is newer than the specified sequence number.", ); - container.close(error); + container.close(DisconnectReason.Unknown, error); throw error; } @@ -139,7 +140,7 @@ export async function loadContainerPaused( // eslint-disable-next-line @typescript-eslint/no-explicit-any .catch((error: any) => { // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - container.close(error); + container.close(DisconnectReason.Unknown, error); throw error; }) .finally(() => { diff --git a/packages/loader/container-loader/src/packageVersion.ts b/packages/loader/container-loader/src/packageVersion.ts index 23798118f167..3b10d191fed4 100644 --- a/packages/loader/container-loader/src/packageVersion.ts +++ b/packages/loader/container-loader/src/packageVersion.ts @@ -6,4 +6,4 @@ */ export const pkgName = "@fluidframework/container-loader"; -export const pkgVersion = "2.13.0"; +export const pkgVersion = "2.20.0"; diff --git a/packages/loader/container-loader/src/test/deltaManager.spec.ts b/packages/loader/container-loader/src/test/deltaManager.spec.ts index 622b1b837ba5..2dae7ea104dd 100644 --- a/packages/loader/container-loader/src/test/deltaManager.spec.ts +++ b/packages/loader/container-loader/src/test/deltaManager.spec.ts @@ -10,6 +10,7 @@ import { MockDocumentDeltaConnection, MockDocumentService, } from "@fluid-private/test-loader-utils"; +import { DisconnectReason } from "@fluidframework/container-definitions/internal"; import { IClient } from "@fluidframework/driver-definitions"; import { IDocumentDeltaStorageService, @@ -534,7 +535,7 @@ describe("Loader", () => { })); // Dispose will trigger abort - deltaManager.dispose(); + deltaManager.dispose(DisconnectReason.Expected); await flushPromises(); mockLogger.assertMatch([ From 36988b352139055819eceef4c64ba18d9fa16dff Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Tue, 14 Jan 2025 21:26:50 +0000 Subject: [PATCH 02/54] api extractor, format and dev tools --- .../container-definitions.legacy.alpha.api.md | 8 ++++++-- .../common/container-definitions/src/loader.ts | 2 -- packages/loader/container-loader/src/container.ts | 14 ++++++++++---- .../devtools/devtools-core/src/test/Utilities.ts | 3 ++- 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md index d583633faa62..f50624bb72a2 100644 --- a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md +++ b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md @@ -108,14 +108,18 @@ export interface IContainer extends IEventProvider { readonly attachState: AttachState; readonly audience: IAudience; readonly clientId?: string | undefined; - close(disconnectReason: DisconnectReason, error?: ICriticalContainerError): void; + close(error?: ICriticalContainerError): void; + // (undocumented) + close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; readonly closed: boolean; connect(): void; readonly connectionState: ConnectionState; containerMetadata: Record; deltaManager: IDeltaManager; disconnect(): void; - dispose(disconnectReason: DisconnectReason, error?: ICriticalContainerError): void; + dispose(error?: ICriticalContainerError): void; + // (undocumented) + dispose(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; readonly disposed?: boolean; forceReadonly?(readonly: boolean): any; getAbsoluteUrl(relativeUrl: string): Promise; diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index a193895cb077..cf80b3829a9e 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -194,7 +194,6 @@ export interface IContainerEvents extends IEvent { * * - `error`: If the container was closed due to error, this will contain details about the error that caused it. * - * @see {@link IContainer.close} */ (event: "closed", listener: (error?: ICriticalContainerError) => void); @@ -205,7 +204,6 @@ export interface IContainerEvents extends IEvent { * * - `error`: If the container was disposed due to error, this will contain details about the error that caused it. * - * @see {@link IContainer.dispose} */ (event: "disposed", listener: (error?: ICriticalContainerError) => void); diff --git a/packages/loader/container-loader/src/container.ts b/packages/loader/container-loader/src/container.ts index 7b63639dc056..ca79e13021d2 100644 --- a/packages/loader/container-loader/src/container.ts +++ b/packages/loader/container-loader/src/container.ts @@ -1054,8 +1054,11 @@ export class Container return this.protocolHandler.quorum; } - public dispose(errorOrReason?: ICriticalContainerError | DisconnectReason, error?: ICriticalContainerError): void { - if (typeof errorOrReason === 'string') { + public dispose( + errorOrReason?: ICriticalContainerError | DisconnectReason, + error?: ICriticalContainerError, + ): void { + if (typeof errorOrReason === "string") { // Called as dispose(disconnectReason, error) this.verifyClosedAfter(() => this._deltaManager.dispose(errorOrReason, error)); } else { @@ -1064,12 +1067,15 @@ export class Container } } - public close(errorOrReason?: ICriticalContainerError | DisconnectReason, error?: ICriticalContainerError): void { + public close( + errorOrReason?: ICriticalContainerError | DisconnectReason, + error?: ICriticalContainerError, + ): void { // 1. Ensure that close sequence is exactly the same no matter if it's initiated by host or by DeltaManager // 2. We need to ensure that we deliver disconnect event to runtime properly. See connectionStateChanged // handler. We only deliver events if container fully loaded. Transitioning from "loading" -> // "closing" will lose that info (can also solve by tracking extra state). - if (typeof errorOrReason === 'string') { + if (typeof errorOrReason === "string") { // Called as close(disconnectReason, error) this.verifyClosedAfter(() => this._deltaManager.close(errorOrReason, error)); } else { diff --git a/packages/tools/devtools/devtools-core/src/test/Utilities.ts b/packages/tools/devtools/devtools-core/src/test/Utilities.ts index 1f980c3c9b4c..db0206304f4d 100644 --- a/packages/tools/devtools/devtools-core/src/test/Utilities.ts +++ b/packages/tools/devtools/devtools-core/src/test/Utilities.ts @@ -6,6 +6,7 @@ import { TypedEventEmitter } from "@fluid-internal/client-utils"; import type { IAudience } from "@fluidframework/container-definitions"; import type { + DisconnectReason, IContainer, IContainerEvents, } from "@fluidframework/container-definitions/internal"; @@ -45,7 +46,7 @@ class MockContainer this.emit("attached"); } - public dispose(error?: IErrorBase | undefined): void { + public dispose(errorOrReason?: IErrorBase | DisconnectReason, error?: IErrorBase): void { this.emit("disposed"); } From b3664f9353834e8219e08a0f09198dee4979534c Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Tue, 14 Jan 2025 22:18:31 +0000 Subject: [PATCH 03/54] document delta connector cherry-pick --- .../drivers/driver-base/src/documentDeltaConnection.ts | 4 ++-- .../routerlicious-driver/src/documentDeltaConnection.ts | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 522b96c64b91..8010b57e0c61 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -401,7 +401,7 @@ export class DocumentDeltaConnection * However the OdspDocumentDeltaConnection differ in dispose as in there we don't close the socket. There is no * multiplexing here, so we need to close the socket here. */ - public dispose() { + public dispose(error?: Error) { this.logger.sendTelemetryEvent({ eventName: "ClientClosingDeltaConnection", driverVersion, @@ -412,7 +412,7 @@ export class DocumentDeltaConnection this.disconnect( createGenericNetworkError( // pre-0.58 error message: clientClosingConnection - "Client closing delta connection", + error?.message ?? "Client closing delta connection", { canRetry: true }, { driverVersion }, ), diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index 22e0d08cfc69..b8f605b2bfa7 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -107,4 +107,13 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { url: getUrlForTelemetry(this.url, socketIoPath), }; } + + /** + * Disconnect from the websocket + */ + protected disconnectCore(err: IAnyDriverError): void { + // tell the server we are disconnecting this client from the document + this.socket.emit("disconnect_document", this.clientId, this.documentId, err); + super.disconnectCore(err); + } } From f1bd57513365b07e3288aca0299ef881985f35bf Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 15 Jan 2025 17:46:08 +0000 Subject: [PATCH 04/54] cherry-pick- server fluidContainer --- packages/framework/fluid-static/src/fluidContainer.ts | 4 ++-- .../loader/container-loader/src/connectionManager.ts | 7 +------ .../routerlicious/packages/lambdas/src/nexus/index.ts | 11 +++++++++++ 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/framework/fluid-static/src/fluidContainer.ts b/packages/framework/fluid-static/src/fluidContainer.ts index 27620fccc445..582c8d630248 100644 --- a/packages/framework/fluid-static/src/fluidContainer.ts +++ b/packages/framework/fluid-static/src/fluidContainer.ts @@ -9,7 +9,7 @@ import { type ConnectionState, type ICriticalContainerError, } from "@fluidframework/container-definitions"; -import type { IContainer } from "@fluidframework/container-definitions/internal"; +import { DisconnectReason, type IContainer } from "@fluidframework/container-definitions/internal"; import type { IEvent, IEventProvider, IFluidLoadable } from "@fluidframework/core-interfaces"; import type { SharedObjectKind } from "@fluidframework/shared-object-base"; @@ -356,7 +356,7 @@ class FluidContainer } public dispose(): void { - this.container.close(); + this.container.close(DisconnectReason.Expected); this.container.off("connected", this.connectedHandler); this.container.off("closed", this.disposedHandler); this.container.off("disconnected", this.disconnectedHandler); diff --git a/packages/loader/container-loader/src/connectionManager.ts b/packages/loader/container-loader/src/connectionManager.ts index 7d1d35f90fd1..9641839cf37a 100644 --- a/packages/loader/container-loader/src/connectionManager.ts +++ b/packages/loader/container-loader/src/connectionManager.ts @@ -444,11 +444,6 @@ export class ConnectionManager implements IConnectionManager { this._outbound.clear(); - const text = - typeof disconnectReasonOrError === "string" - ? disconnectReasonOrError - : "Closing DeltaManager"; - const finalSwitchToReadonly = typeof errorOrSwitchToReadonly === "boolean" ? errorOrSwitchToReadonly @@ -462,7 +457,7 @@ export class ConnectionManager implements IConnectionManager { : undefined; const disconnectReason: IConnectionStateChangeReason = { - text, + text: "Closing DeltaManager", error, disconnectReason: typeof disconnectReasonOrError === "string" diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index 6587b8d27bf8..f5a384f7c174 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -637,5 +637,16 @@ export function configureWebSocketServices( } disposers.splice(0, disposers.length); }); + + socket.on( + "disconnect_document", + (clientId: string, documentId: string, disconnectReason: string) => { + Lumberjack.error( + "Client disconnected due to error", + { clientId, documentId }, + disconnectReason, + ); + }, + ); }); } From b5bf87d025751daeaea5e984a6bdb5982d2765ff Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 15 Jan 2025 18:03:48 +0000 Subject: [PATCH 05/54] add jsdoc and format --- packages/common/container-definitions/src/loader.ts | 5 +++++ packages/framework/fluid-static/src/fluidContainer.ts | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index cf80b3829a9e..0b63b96be08f 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -194,6 +194,7 @@ export interface IContainerEvents extends IEvent { * * - `error`: If the container was closed due to error, this will contain details about the error that caused it. * + * @see {@link IContainer.(close:1)} */ (event: "closed", listener: (error?: ICriticalContainerError) => void); @@ -204,6 +205,7 @@ export interface IContainerEvents extends IEvent { * * - `error`: If the container was disposed due to error, this will contain details about the error that caused it. * + * @see {@link IContainer.(dispose:1)} */ (event: "disposed", listener: (error?: ICriticalContainerError) => void); @@ -410,6 +412,9 @@ export interface IContainer extends IEventProvider { * * @param error - If the container is being closed due to error, this provides details about the error that * resulted in closing it. + * @param disconnectReason - The reason for disconnecting the container + * + * @see {@link @fluidframework/container-definitions#IContainer.(close:1)} */ close(error?: ICriticalContainerError): void; close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; diff --git a/packages/framework/fluid-static/src/fluidContainer.ts b/packages/framework/fluid-static/src/fluidContainer.ts index 582c8d630248..f4a3d34e0dfe 100644 --- a/packages/framework/fluid-static/src/fluidContainer.ts +++ b/packages/framework/fluid-static/src/fluidContainer.ts @@ -9,7 +9,10 @@ import { type ConnectionState, type ICriticalContainerError, } from "@fluidframework/container-definitions"; -import { DisconnectReason, type IContainer } from "@fluidframework/container-definitions/internal"; +import { + DisconnectReason, + type IContainer, +} from "@fluidframework/container-definitions/internal"; import type { IEvent, IEventProvider, IFluidLoadable } from "@fluidframework/core-interfaces"; import type { SharedObjectKind } from "@fluidframework/shared-object-base"; From 9214ea2acae7eeb0eb31fcb2a8e41f131ef498f2 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 15 Jan 2025 18:37:38 +0000 Subject: [PATCH 06/54] extract string from error in disconnectCore --- .../drivers/routerlicious-driver/src/documentDeltaConnection.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index b8f605b2bfa7..eb9808b083fd 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -113,7 +113,7 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { */ protected disconnectCore(err: IAnyDriverError): void { // tell the server we are disconnecting this client from the document - this.socket.emit("disconnect_document", this.clientId, this.documentId, err); + this.socket.emit("disconnect_document", this.clientId, this.documentId, err.message); super.disconnectCore(err); } } From 590d0914e8df4b7323bf4e105e7acd081cba2a93 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 15 Jan 2025 18:39:22 +0000 Subject: [PATCH 07/54] remove extra jsdoc --- packages/common/container-definitions/src/loader.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 0b63b96be08f..146d1f5a5b89 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -413,8 +413,6 @@ export interface IContainer extends IEventProvider { * @param error - If the container is being closed due to error, this provides details about the error that * resulted in closing it. * @param disconnectReason - The reason for disconnecting the container - * - * @see {@link @fluidframework/container-definitions#IContainer.(close:1)} */ close(error?: ICriticalContainerError): void; close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; From c19ab832799a1298e3626963d70d0ddeb7220423 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 15 Jan 2025 20:32:13 +0000 Subject: [PATCH 08/54] move DisconnectReason to core-interfaces --- .../container-definitions.legacy.alpha.api.md | 10 ------- .../common/container-definitions/src/index.ts | 2 +- .../container-definitions/src/loader.ts | 19 +------------- .../core-interfaces.legacy.alpha.api.md | 10 +++++++ .../core-interfaces/src/disconnectReason.ts | 22 ++++++++++++++++ packages/common/core-interfaces/src/index.ts | 1 + .../src/documentDeltaConnection.ts | 18 ++++++++++++- .../fluid-static/src/fluidContainer.ts | 6 ++--- .../container-loader/src/connectionManager.ts | 7 ++--- .../loader/container-loader/src/container.ts | 6 +++-- .../loader/container-loader/src/contracts.ts | 2 +- .../container-loader/src/deltaManager.ts | 26 +++++++++++++++---- .../loader/container-loader/src/loadPaused.ts | 2 +- .../src/test/deltaManager.spec.ts | 2 +- .../devtools-core/src/test/Utilities.ts | 2 +- 15 files changed, 85 insertions(+), 50 deletions(-) create mode 100644 packages/common/core-interfaces/src/disconnectReason.ts diff --git a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md index f50624bb72a2..f9cbe35cfdf8 100644 --- a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md +++ b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md @@ -40,16 +40,6 @@ export interface ContainerWarning extends IErrorBase_2 { logged?: boolean; } -// @alpha -export const DisconnectReason: { - readonly Expected: "Expected"; - readonly Corruption: "Corruption"; - readonly Unknown: "Unknown"; -}; - -// @alpha -export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; - // @public export interface IAudience extends IEventProvider { getMember(clientId: string): IClient | undefined; diff --git a/packages/common/container-definitions/src/index.ts b/packages/common/container-definitions/src/index.ts index 2eba983ee51f..6e587f42fa65 100644 --- a/packages/common/container-definitions/src/index.ts +++ b/packages/common/container-definitions/src/index.ts @@ -45,7 +45,7 @@ export type { IResolvedFluidCodeDetails, ISnapshotTreeWithBlobContents, } from "./loader.js"; -export { DisconnectReason, LoaderHeader } from "./loader.js"; +export { LoaderHeader } from "./loader.js"; export type { IFluidModule } from "./fluidModule.js"; export type { IFluidPackage, diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 146d1f5a5b89..5229bdec1f3f 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -9,6 +9,7 @@ import type { IEventProvider, IRequest, } from "@fluidframework/core-interfaces"; +import type { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import type { IClient, IClientDetails, @@ -271,24 +272,6 @@ export interface IContainerEvents extends IEvent { (event: "metadataUpdate", listener: (metadata: Record) => void); } -/** - * DisconnectReason of disconnect events emitted by the {@link IContainer}. - * @legacy - * @alpha - */ -export const DisconnectReason = { - Expected: "Expected", - Corruption: "Corruption", - Unknown: "Unknown", -} as const; - -/** - * {@inheritDoc (DisconnectReason:variable)} - * @legacy - * @alpha - */ -export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; - /** * Namespace for the different connection states a container can be in. * PLEASE NOTE: The sequence of the numerical values does no correspond to the typical connection state progression. diff --git a/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md b/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md index 8adbe20f8ea7..7f590edb4ce8 100644 --- a/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md +++ b/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md @@ -7,6 +7,16 @@ // @public export type ConfigTypes = string | number | boolean | number[] | string[] | boolean[] | undefined; +// @alpha +export const DisconnectReason: { + readonly Expected: "Expected"; + readonly Corruption: "Corruption"; + readonly Unknown: "Unknown"; +}; + +// @alpha +export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; + // @public @sealed export abstract class ErasedType { static [Symbol.hasInstance](value: never): value is never; diff --git a/packages/common/core-interfaces/src/disconnectReason.ts b/packages/common/core-interfaces/src/disconnectReason.ts new file mode 100644 index 000000000000..255ebc7cafa7 --- /dev/null +++ b/packages/common/core-interfaces/src/disconnectReason.ts @@ -0,0 +1,22 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +/** + * DisconnectReason of disconnect events emitted by the {@link @fluidframework/container-definitions#IContainer}. + * @legacy + * @alpha + */ +export const DisconnectReason = { + Expected: "Expected", + Corruption: "Corruption", + Unknown: "Unknown", +} as const; + +/** + * {@inheritDoc (DisconnectReason:variable)} + * @legacy + * @alpha + */ +export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; diff --git a/packages/common/core-interfaces/src/index.ts b/packages/common/core-interfaces/src/index.ts index 06796dab9223..cd42eec3148b 100644 --- a/packages/common/core-interfaces/src/index.ts +++ b/packages/common/core-interfaces/src/index.ts @@ -4,6 +4,7 @@ */ export type { IDisposable } from "./disposable.js"; +export { DisconnectReason } from "./disconnectReason.js"; export type { IErrorBase, IGenericError, IUsageError, IThrottlingWarning } from "./error.js"; export { FluidErrorTypes } from "./error.js"; diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 8010b57e0c61..f90cfeed701c 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -8,6 +8,7 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; +import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode } from "@fluidframework/driver-definitions"; import { @@ -409,16 +410,31 @@ export class DocumentDeltaConnection ...this.getConnectionDetailsProps(), }), }); + this.disconnect( createGenericNetworkError( // pre-0.58 error message: clientClosingConnection - error?.message ?? "Client closing delta connection", + this.getDisconnectionMessage(error), { canRetry: true }, { driverVersion }, ), ); } + private getDisconnectionMessage(error?: Error) { + const DEFAULT_MESSAGE = "Client closing delta connection"; + + if (!error) { + return DEFAULT_MESSAGE; + } + + const hasDisconnectReason = Object.values(DisconnectReason).some((reason) => + error.message?.includes(reason), + ); + + return hasDisconnectReason ? error.message : DEFAULT_MESSAGE; + } + protected disconnect(err: IAnyDriverError) { // Can't check this.disposed here, as we get here on socket closure, // so _disposed & socket.connected might be not in sync while processing diff --git a/packages/framework/fluid-static/src/fluidContainer.ts b/packages/framework/fluid-static/src/fluidContainer.ts index f4a3d34e0dfe..35d945c66d10 100644 --- a/packages/framework/fluid-static/src/fluidContainer.ts +++ b/packages/framework/fluid-static/src/fluidContainer.ts @@ -9,11 +9,9 @@ import { type ConnectionState, type ICriticalContainerError, } from "@fluidframework/container-definitions"; -import { - DisconnectReason, - type IContainer, -} from "@fluidframework/container-definitions/internal"; +import type { IContainer } from "@fluidframework/container-definitions/internal"; import type { IEvent, IEventProvider, IFluidLoadable } from "@fluidframework/core-interfaces"; +import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import type { SharedObjectKind } from "@fluidframework/shared-object-base"; import type { ContainerAttachProps, ContainerSchema, IRootDataObject } from "./types.js"; diff --git a/packages/loader/container-loader/src/connectionManager.ts b/packages/loader/container-loader/src/connectionManager.ts index 9641839cf37a..89ed2ddfc157 100644 --- a/packages/loader/container-loader/src/connectionManager.ts +++ b/packages/loader/container-loader/src/connectionManager.ts @@ -5,16 +5,13 @@ import { TypedEventEmitter, performance } from "@fluid-internal/client-utils"; import { ICriticalContainerError } from "@fluidframework/container-definitions"; -import { - IDeltaQueue, - ReadOnlyInfo, - DisconnectReason, -} from "@fluidframework/container-definitions/internal"; +import { IDeltaQueue, ReadOnlyInfo } from "@fluidframework/container-definitions/internal"; import { IDisposable, ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; +import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode, IClient, IClientDetails } from "@fluidframework/driver-definitions"; import { diff --git a/packages/loader/container-loader/src/container.ts b/packages/loader/container-loader/src/container.ts index ca79e13021d2..ca86e7914c38 100644 --- a/packages/loader/container-loader/src/container.ts +++ b/packages/loader/container-loader/src/container.ts @@ -28,7 +28,6 @@ import { isFluidCodeDetails, IDeltaManager, ReadOnlyInfo, - DisconnectReason, type ILoader, } from "@fluidframework/container-definitions/internal"; import { @@ -38,7 +37,10 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { type ISignalEnvelope } from "@fluidframework/core-interfaces/internal"; +import { + DisconnectReason, + type ISignalEnvelope, +} from "@fluidframework/core-interfaces/internal"; import { assert, isPromiseLike, unreachableCase } from "@fluidframework/core-utils/internal"; import { IClient, diff --git a/packages/loader/container-loader/src/contracts.ts b/packages/loader/container-loader/src/contracts.ts index 511c3d65318c..082420245090 100644 --- a/packages/loader/container-loader/src/contracts.ts +++ b/packages/loader/container-loader/src/contracts.ts @@ -10,9 +10,9 @@ import { IFluidCodeDetails, isFluidPackage, IConnectionDetails, - type DisconnectReason, } from "@fluidframework/container-definitions/internal"; import { IErrorBase, ITelemetryBaseProperties } from "@fluidframework/core-interfaces"; +import type { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { ConnectionMode, IClientDetails } from "@fluidframework/driver-definitions"; import { IContainerPackageInfo, diff --git a/packages/loader/container-loader/src/deltaManager.ts b/packages/loader/container-loader/src/deltaManager.ts index a8f8707036ef..d787dd8011c1 100644 --- a/packages/loader/container-loader/src/deltaManager.ts +++ b/packages/loader/container-loader/src/deltaManager.ts @@ -8,7 +8,6 @@ import { IDeltaManagerEvents, IDeltaManagerFull, IDeltaQueue, - DisconnectReason, type IDeltaSender, type ReadOnlyInfo, } from "@fluidframework/container-definitions/internal"; @@ -17,7 +16,10 @@ import { type ITelemetryBaseEvent, ITelemetryBaseProperties, } from "@fluidframework/core-interfaces"; -import { IThrottlingWarning } from "@fluidframework/core-interfaces/internal"; +import { + DisconnectReason, + IThrottlingWarning, +} from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode } from "@fluidframework/driver-definitions"; import { @@ -762,15 +764,29 @@ export class DeltaManager * - close emits "closed" * - close cannot be called after dispose */ - public close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void { + public close( + errorOrReason?: ICriticalContainerError | DisconnectReason, + error?: ICriticalContainerError, + ): void { if (this._closed) { return; } this._closed = true; - this.connectionManager.dispose(disconnectReason, error, true /* switchToReadonly */); + let finalError: ICriticalContainerError | undefined; + let disconnectReason: DisconnectReason | undefined; + + if (typeof errorOrReason === "string") { + finalError = error; + disconnectReason = errorOrReason; + } else { + finalError = errorOrReason; + disconnectReason = undefined; + } + + this.connectionManager.dispose(disconnectReason, finalError, true /* switchToReadonly */); this.clearQueues(); - this.emit("closed", error); + this.emit("closed", finalError); } /** diff --git a/packages/loader/container-loader/src/loadPaused.ts b/packages/loader/container-loader/src/loadPaused.ts index 33660a52ea46..64ded60214fa 100644 --- a/packages/loader/container-loader/src/loadPaused.ts +++ b/packages/loader/container-loader/src/loadPaused.ts @@ -4,13 +4,13 @@ */ import { - DisconnectReason, isIDeltaManagerFull, LoaderHeader, type IContainer, } from "@fluidframework/container-definitions/internal"; import { IRequest } from "@fluidframework/core-interfaces"; import type { IErrorBase } from "@fluidframework/core-interfaces"; +import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { GenericError } from "@fluidframework/telemetry-utils/internal"; diff --git a/packages/loader/container-loader/src/test/deltaManager.spec.ts b/packages/loader/container-loader/src/test/deltaManager.spec.ts index 2dae7ea104dd..0a5230b96ef0 100644 --- a/packages/loader/container-loader/src/test/deltaManager.spec.ts +++ b/packages/loader/container-loader/src/test/deltaManager.spec.ts @@ -10,7 +10,7 @@ import { MockDocumentDeltaConnection, MockDocumentService, } from "@fluid-private/test-loader-utils"; -import { DisconnectReason } from "@fluidframework/container-definitions/internal"; +import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { IClient } from "@fluidframework/driver-definitions"; import { IDocumentDeltaStorageService, diff --git a/packages/tools/devtools/devtools-core/src/test/Utilities.ts b/packages/tools/devtools/devtools-core/src/test/Utilities.ts index db0206304f4d..cc791a1a7c46 100644 --- a/packages/tools/devtools/devtools-core/src/test/Utilities.ts +++ b/packages/tools/devtools/devtools-core/src/test/Utilities.ts @@ -6,12 +6,12 @@ import { TypedEventEmitter } from "@fluid-internal/client-utils"; import type { IAudience } from "@fluidframework/container-definitions"; import type { - DisconnectReason, IContainer, IContainerEvents, } from "@fluidframework/container-definitions/internal"; import { ConnectionState } from "@fluidframework/container-loader"; import type { IErrorBase, IRequest } from "@fluidframework/core-interfaces"; +import type { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import type { IClient } from "@fluidframework/driver-definitions"; import { MockAudience } from "@fluidframework/test-runtime-utils/internal"; From 1a482d54b966c89c2e738daf161a16425f70603f Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 15 Jan 2025 20:51:42 +0000 Subject: [PATCH 09/54] remove type --- packages/loader/container-loader/src/contracts.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/loader/container-loader/src/contracts.ts b/packages/loader/container-loader/src/contracts.ts index 082420245090..f0addc5d6351 100644 --- a/packages/loader/container-loader/src/contracts.ts +++ b/packages/loader/container-loader/src/contracts.ts @@ -12,7 +12,7 @@ import { IConnectionDetails, } from "@fluidframework/container-definitions/internal"; import { IErrorBase, ITelemetryBaseProperties } from "@fluidframework/core-interfaces"; -import type { DisconnectReason } from "@fluidframework/core-interfaces/internal"; +import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { ConnectionMode, IClientDetails } from "@fluidframework/driver-definitions"; import { IContainerPackageInfo, From 3a78424bf82770522214805ddb93650f25b49a53 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 15 Jan 2025 16:24:59 -0500 Subject: [PATCH 10/54] Update packages/common/core-interfaces/src/disconnectReason.ts Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com> --- packages/common/core-interfaces/src/disconnectReason.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/common/core-interfaces/src/disconnectReason.ts b/packages/common/core-interfaces/src/disconnectReason.ts index 255ebc7cafa7..0f945abc9038 100644 --- a/packages/common/core-interfaces/src/disconnectReason.ts +++ b/packages/common/core-interfaces/src/disconnectReason.ts @@ -4,7 +4,7 @@ */ /** - * DisconnectReason of disconnect events emitted by the {@link @fluidframework/container-definitions#IContainer}. + * Potential reasons for disconnect events emitted by {@link @fluidframework/container-definitions#IContainer}. * @legacy * @alpha */ From 49b31253e211b3f7cc4aadab9e9fccfff7a5ae61 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 15 Jan 2025 22:18:45 +0000 Subject: [PATCH 11/54] feedback --- .../container-definitions.legacy.alpha.api.md | 2 -- .../container-definitions/src/loader.ts | 18 +++++++++++++++++- .../core-interfaces.legacy.alpha.api.md | 3 +++ .../core-interfaces/src/disconnectReason.ts | 14 ++++++++++++++ packages/common/core-interfaces/src/index.ts | 2 +- .../src/documentDeltaConnection.ts | 19 ++++++------------- .../fluid-static/src/fluidContainer.ts | 2 +- .../container-loader/src/deltaManager.ts | 7 +++++-- .../packages/lambdas/src/nexus/index.ts | 7 ++++++- 9 files changed, 53 insertions(+), 21 deletions(-) diff --git a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md index f9cbe35cfdf8..a0c1d462a155 100644 --- a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md +++ b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md @@ -99,7 +99,6 @@ export interface IContainer extends IEventProvider { readonly audience: IAudience; readonly clientId?: string | undefined; close(error?: ICriticalContainerError): void; - // (undocumented) close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; readonly closed: boolean; connect(): void; @@ -108,7 +107,6 @@ export interface IContainer extends IEventProvider { deltaManager: IDeltaManager; disconnect(): void; dispose(error?: ICriticalContainerError): void; - // (undocumented) dispose(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; readonly disposed?: boolean; forceReadonly?(readonly: boolean): any; diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 5229bdec1f3f..715f032f93de 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -388,6 +388,15 @@ export interface IContainer extends IEventProvider { * resulted in disposing it. */ dispose(error?: ICriticalContainerError): void; + + /** + * Disposes the container. If not already closed, this acts as a closure and then disposes runtime resources. + * The container is not expected to be used anymore once it is disposed. + * + * @param disconnectReason - The reason for disconnecting the container + * @param error - OIf the container is being disposed due to error, this provides details about the error that + * resulted in disposing it. + */ dispose(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; /** @@ -395,9 +404,16 @@ export interface IContainer extends IEventProvider { * * @param error - If the container is being closed due to error, this provides details about the error that * resulted in closing it. - * @param disconnectReason - The reason for disconnecting the container */ close(error?: ICriticalContainerError): void; + + /** + * Closes the container. + * + * @param disconnectReason - The reason for disconnecting the container + * @param error - If the container is being closed due to error, this provides details about the error that + * resulted in closing it. + */ close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; /** diff --git a/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md b/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md index 7f590edb4ce8..3e4ef0d5af31 100644 --- a/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md +++ b/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md @@ -345,6 +345,9 @@ export interface IResponse { value: any; } +// @alpha +export const isDisconnectReason: (value: unknown) => boolean; + // @public export type IsListener = TListener extends (...args: any[]) => void ? true : false; diff --git a/packages/common/core-interfaces/src/disconnectReason.ts b/packages/common/core-interfaces/src/disconnectReason.ts index 0f945abc9038..ddfaae52c24a 100644 --- a/packages/common/core-interfaces/src/disconnectReason.ts +++ b/packages/common/core-interfaces/src/disconnectReason.ts @@ -5,6 +5,7 @@ /** * Potential reasons for disconnect events emitted by {@link @fluidframework/container-definitions#IContainer}. + * * @legacy * @alpha */ @@ -20,3 +21,16 @@ export const DisconnectReason = { * @alpha */ export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; + +/** + * Type guard that checks if a value is a valid DisconnectReason. + * + * @param value - The value to check + * @returns True if the value is a valid DisconnectReason, false otherwise + * @legacy + * @alpha + */ +export const isDisconnectReason = (value: unknown): boolean => + value !== undefined && + typeof value === "string" && + Object.values(DisconnectReason).includes(value as DisconnectReason); diff --git a/packages/common/core-interfaces/src/index.ts b/packages/common/core-interfaces/src/index.ts index cd42eec3148b..7b3988d84931 100644 --- a/packages/common/core-interfaces/src/index.ts +++ b/packages/common/core-interfaces/src/index.ts @@ -4,7 +4,7 @@ */ export type { IDisposable } from "./disposable.js"; -export { DisconnectReason } from "./disconnectReason.js"; +export { DisconnectReason, isDisconnectReason } from "./disconnectReason.js"; export type { IErrorBase, IGenericError, IUsageError, IThrottlingWarning } from "./error.js"; export { FluidErrorTypes } from "./error.js"; diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index f90cfeed701c..8ea2f191bfde 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -8,7 +8,7 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; +import { isDisconnectReason } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode } from "@fluidframework/driver-definitions"; import { @@ -414,25 +414,18 @@ export class DocumentDeltaConnection this.disconnect( createGenericNetworkError( // pre-0.58 error message: clientClosingConnection - this.getDisconnectionMessage(error), + this.getDisconnectMessage(error), { canRetry: true }, { driverVersion }, ), ); } - private getDisconnectionMessage(error?: Error) { - const DEFAULT_MESSAGE = "Client closing delta connection"; - - if (!error) { - return DEFAULT_MESSAGE; + private getDisconnectMessage(error?: Error): string { + if (error?.message && isDisconnectReason(error.message)) { + return error.message; } - - const hasDisconnectReason = Object.values(DisconnectReason).some((reason) => - error.message?.includes(reason), - ); - - return hasDisconnectReason ? error.message : DEFAULT_MESSAGE; + return "Client closing delta connection"; } protected disconnect(err: IAnyDriverError) { diff --git a/packages/framework/fluid-static/src/fluidContainer.ts b/packages/framework/fluid-static/src/fluidContainer.ts index 35d945c66d10..cc9416c9e95c 100644 --- a/packages/framework/fluid-static/src/fluidContainer.ts +++ b/packages/framework/fluid-static/src/fluidContainer.ts @@ -357,7 +357,7 @@ class FluidContainer } public dispose(): void { - this.container.close(DisconnectReason.Expected); + this.container.close(DisconnectReason.Unknown); this.container.off("connected", this.connectedHandler); this.container.off("closed", this.disposedHandler); this.container.off("disconnected", this.disconnectedHandler); diff --git a/packages/loader/container-loader/src/deltaManager.ts b/packages/loader/container-loader/src/deltaManager.ts index d787dd8011c1..b7e4824333f6 100644 --- a/packages/loader/container-loader/src/deltaManager.ts +++ b/packages/loader/container-loader/src/deltaManager.ts @@ -776,11 +776,14 @@ export class DeltaManager let finalError: ICriticalContainerError | undefined; let disconnectReason: DisconnectReason | undefined; - if (typeof errorOrReason === "string") { + if ( + typeof errorOrReason === "string" && + Object.values(DisconnectReason).some((reason) => errorOrReason.includes(reason)) + ) { finalError = error; disconnectReason = errorOrReason; } else { - finalError = errorOrReason; + finalError = typeof errorOrReason === "object" ? errorOrReason : undefined; disconnectReason = undefined; } diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index f5a384f7c174..a3b751444ea5 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -641,8 +641,13 @@ export function configureWebSocketServices( socket.on( "disconnect_document", (clientId: string, documentId: string, disconnectReason: string) => { + const disconnectReasonDisplay: Record = { + "Expected": "Client disconnected normally", + "Corruption": "Client disconnected due to data corruption", + "Unknown": "Client disconnected unexpectedly" + }; Lumberjack.error( - "Client disconnected due to error", + disconnectReasonDisplay[disconnectReason] || "Client disconnected unexpectedly", { clientId, documentId }, disconnectReason, ); From 863e7d5895f6f40ae434ef8da7fd3e67e5ff464e Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 15 Jan 2025 23:22:56 +0000 Subject: [PATCH 12/54] enforce error object when Corruption reason --- .../container-definitions.legacy.alpha.api.md | 6 +++-- .../container-definitions/src/loader.ts | 26 ++++++++++++++++--- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md index a0c1d462a155..e118c6a86f06 100644 --- a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md +++ b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md @@ -99,7 +99,8 @@ export interface IContainer extends IEventProvider { readonly audience: IAudience; readonly clientId?: string | undefined; close(error?: ICriticalContainerError): void; - close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; + close(disconnectReason: "Corruption", error: ICriticalContainerError): void; + close(disconnectReason: "Expected" | "Unknown", error?: ICriticalContainerError): void; readonly closed: boolean; connect(): void; readonly connectionState: ConnectionState; @@ -107,7 +108,8 @@ export interface IContainer extends IEventProvider { deltaManager: IDeltaManager; disconnect(): void; dispose(error?: ICriticalContainerError): void; - dispose(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; + dispose(disconnectReason: "Corruption", error: ICriticalContainerError): void; + dispose(disconnectReason: "Expected" | "Unknown", error?: ICriticalContainerError): void; readonly disposed?: boolean; forceReadonly?(readonly: boolean): any; getAbsoluteUrl(relativeUrl: string): Promise; diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 715f032f93de..23a062d0c1dd 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -9,7 +9,6 @@ import type { IEventProvider, IRequest, } from "@fluidframework/core-interfaces"; -import type { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import type { IClient, IClientDetails, @@ -394,10 +393,20 @@ export interface IContainer extends IEventProvider { * The container is not expected to be used anymore once it is disposed. * * @param disconnectReason - The reason for disconnecting the container - * @param error - OIf the container is being disposed due to error, this provides details about the error that + * @param error - If the container is being disposed due to error, this provides details about the error that + * resulted in disposing it. + */ + dispose(disconnectReason: "Corruption", error: ICriticalContainerError): void; + + /** + * Disposes the container. If not already closed, this acts as a closure and then disposes runtime resources. + * The container is not expected to be used anymore once it is disposed. + * + * @param disconnectReason - The reason for disconnecting the container + * @param error - If the container is being disposed due to error, this provides details about the error that * resulted in disposing it. */ - dispose(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; + dispose(disconnectReason: "Expected" | "Unknown", error?: ICriticalContainerError): void; /** * Closes the container. @@ -414,7 +423,16 @@ export interface IContainer extends IEventProvider { * @param error - If the container is being closed due to error, this provides details about the error that * resulted in closing it. */ - close(disconnectReason?: DisconnectReason, error?: ICriticalContainerError): void; + close(disconnectReason: "Corruption", error: ICriticalContainerError): void; + + /** + * Closes the container. + * + * @param disconnectReason - The reason for disconnecting the container + * @param error - If the container is being closed due to error, this provides details about the error that + * resulted in closing it. + */ + close(disconnectReason: "Expected" | "Unknown", error?: ICriticalContainerError): void; /** * Propose new code details that define the code to be loaded for this container's runtime. From 842172125ff0599887ac31d258a1b946cf1e8fce Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 17:34:47 +0000 Subject: [PATCH 13/54] wip --- .../core-interfaces.legacy.alpha.api.md | 2 +- .../core-interfaces/src/disconnectReason.ts | 2 +- .../container-loader/src/connectionManager.ts | 27 +++++++++---------- .../container-loader/src/deltaManager.ts | 12 ++++----- 4 files changed, 19 insertions(+), 24 deletions(-) diff --git a/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md b/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md index 3e4ef0d5af31..1137855347f8 100644 --- a/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md +++ b/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md @@ -346,7 +346,7 @@ export interface IResponse { } // @alpha -export const isDisconnectReason: (value: unknown) => boolean; +export const isDisconnectReason: (value: unknown) => value is DisconnectReason; // @public export type IsListener = TListener extends (...args: any[]) => void ? true : false; diff --git a/packages/common/core-interfaces/src/disconnectReason.ts b/packages/common/core-interfaces/src/disconnectReason.ts index ddfaae52c24a..3d87c14ec742 100644 --- a/packages/common/core-interfaces/src/disconnectReason.ts +++ b/packages/common/core-interfaces/src/disconnectReason.ts @@ -30,7 +30,7 @@ export type DisconnectReason = (typeof DisconnectReason)[keyof typeof Disconnect * @legacy * @alpha */ -export const isDisconnectReason = (value: unknown): boolean => +export const isDisconnectReason = (value: unknown): value is DisconnectReason => value !== undefined && typeof value === "string" && Object.values(DisconnectReason).includes(value as DisconnectReason); diff --git a/packages/loader/container-loader/src/connectionManager.ts b/packages/loader/container-loader/src/connectionManager.ts index 89ed2ddfc157..b4be27b9106c 100644 --- a/packages/loader/container-loader/src/connectionManager.ts +++ b/packages/loader/container-loader/src/connectionManager.ts @@ -11,7 +11,7 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; +import { DisconnectReason, isDisconnectReason } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode, IClient, IClientDetails } from "@fluidframework/driver-definitions"; import { @@ -440,26 +440,23 @@ export class ConnectionManager implements IConnectionManager { this._reconnectMode = ReconnectMode.Never; this._outbound.clear(); + let disconnectReasonParam: DisconnectReason; + let error: ICriticalContainerError | undefined; + if (isDisconnectReason(disconnectReasonOrError)) { + error = typeof errorOrSwitchToReadonly === "boolean" ? undefined : errorOrSwitchToReadonly + disconnectReasonParam = disconnectReasonOrError; + } else { + error = disconnectReasonOrError; + disconnectReasonParam = DisconnectReason.Unknown; + } const finalSwitchToReadonly = - typeof errorOrSwitchToReadonly === "boolean" - ? errorOrSwitchToReadonly - : switchToReadonly; - - const error: ICriticalContainerError | undefined = - typeof errorOrSwitchToReadonly === "boolean" && - disconnectReasonOrError !== undefined && - typeof disconnectReasonOrError !== "string" - ? disconnectReasonOrError - : undefined; + typeof errorOrSwitchToReadonly === "boolean" ? errorOrSwitchToReadonly : switchToReadonly; const disconnectReason: IConnectionStateChangeReason = { text: "Closing DeltaManager", error, - disconnectReason: - typeof disconnectReasonOrError === "string" - ? disconnectReasonOrError - : DisconnectReason.Unknown, + disconnectReason: disconnectReasonParam }; const oldReadonlyValue = this.readonly; diff --git a/packages/loader/container-loader/src/deltaManager.ts b/packages/loader/container-loader/src/deltaManager.ts index b7e4824333f6..784a6edd7b49 100644 --- a/packages/loader/container-loader/src/deltaManager.ts +++ b/packages/loader/container-loader/src/deltaManager.ts @@ -19,6 +19,7 @@ import { import { DisconnectReason, IThrottlingWarning, + isDisconnectReason, } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode } from "@fluidframework/driver-definitions"; @@ -774,17 +775,14 @@ export class DeltaManager this._closed = true; let finalError: ICriticalContainerError | undefined; - let disconnectReason: DisconnectReason | undefined; + let disconnectReason: DisconnectReason; - if ( - typeof errorOrReason === "string" && - Object.values(DisconnectReason).some((reason) => errorOrReason.includes(reason)) - ) { + if (isDisconnectReason(errorOrReason)) { finalError = error; disconnectReason = errorOrReason; } else { - finalError = typeof errorOrReason === "object" ? errorOrReason : undefined; - disconnectReason = undefined; + finalError = errorOrReason; + disconnectReason = DisconnectReason.Unknown; } this.connectionManager.dispose(disconnectReason, finalError, true /* switchToReadonly */); From 0f7d687b463a1a3addcc0460b522f66551156c56 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 17:47:06 +0000 Subject: [PATCH 14/54] format --- .../container-loader/src/connectionManager.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/loader/container-loader/src/connectionManager.ts b/packages/loader/container-loader/src/connectionManager.ts index b4be27b9106c..c2bfe1c385fb 100644 --- a/packages/loader/container-loader/src/connectionManager.ts +++ b/packages/loader/container-loader/src/connectionManager.ts @@ -11,7 +11,10 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { DisconnectReason, isDisconnectReason } from "@fluidframework/core-interfaces/internal"; +import { + DisconnectReason, + isDisconnectReason, +} from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode, IClient, IClientDetails } from "@fluidframework/driver-definitions"; import { @@ -443,7 +446,8 @@ export class ConnectionManager implements IConnectionManager { let disconnectReasonParam: DisconnectReason; let error: ICriticalContainerError | undefined; if (isDisconnectReason(disconnectReasonOrError)) { - error = typeof errorOrSwitchToReadonly === "boolean" ? undefined : errorOrSwitchToReadonly + error = + typeof errorOrSwitchToReadonly === "boolean" ? undefined : errorOrSwitchToReadonly; disconnectReasonParam = disconnectReasonOrError; } else { error = disconnectReasonOrError; @@ -451,12 +455,14 @@ export class ConnectionManager implements IConnectionManager { } const finalSwitchToReadonly = - typeof errorOrSwitchToReadonly === "boolean" ? errorOrSwitchToReadonly : switchToReadonly; + typeof errorOrSwitchToReadonly === "boolean" + ? errorOrSwitchToReadonly + : switchToReadonly; const disconnectReason: IConnectionStateChangeReason = { text: "Closing DeltaManager", error, - disconnectReason: disconnectReasonParam + disconnectReason: disconnectReasonParam, }; const oldReadonlyValue = this.readonly; From deff9fffeb465df7118b325d8035b8d5eb8eab2d Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 18:54:48 +0000 Subject: [PATCH 15/54] fix back compat --- .../container-definitions.legacy.alpha.api.md | 8 +-- .../container-definitions/src/loader.ts | 45 +++----------- .../fluid-static/src/fluidContainer.ts | 2 +- .../container-loader/src/connectionManager.ts | 34 ++--------- .../loader/container-loader/src/container.ts | 61 ++++++------------- .../loader/container-loader/src/contracts.ts | 5 +- .../container-loader/src/deltaManager.ts | 36 ++++------- .../loader/container-loader/src/loadPaused.ts | 4 +- .../src/test/deltaManager.spec.ts | 2 +- .../devtools-core/src/test/Utilities.ts | 2 +- 10 files changed, 53 insertions(+), 146 deletions(-) diff --git a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md index 14cada26a5f3..bf698447a779 100644 --- a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md +++ b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md @@ -99,8 +99,8 @@ export interface IContainer extends IEventProvider { readonly audience: IAudience; readonly clientId?: string | undefined; close(error?: ICriticalContainerError): void; - close(disconnectReason: "Corruption", error: ICriticalContainerError): void; - close(disconnectReason: "Expected" | "Unknown", error?: ICriticalContainerError): void; + // (undocumented) + close(error?: ICriticalContainerError, disconnectReason?: DisconnectReason): void; readonly closed: boolean; connect(): void; readonly connectionState: ConnectionState; @@ -108,8 +108,8 @@ export interface IContainer extends IEventProvider { deltaManager: IDeltaManager; disconnect(): void; dispose(error?: ICriticalContainerError): void; - dispose(disconnectReason: "Corruption", error: ICriticalContainerError): void; - dispose(disconnectReason: "Expected" | "Unknown", error?: ICriticalContainerError): void; + // (undocumented) + dispose(error?: ICriticalContainerError, disconnectReason?: DisconnectReason): void; readonly disposed?: boolean; forceReadonly?(readonly: boolean): any; getAbsoluteUrl(relativeUrl: string): Promise; diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 18085fc3efab..f7654f4ba520 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -9,6 +9,7 @@ import type { IEventProvider, IRequest, } from "@fluidframework/core-interfaces"; +import type { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import type { IClient, IClientDetails, @@ -387,26 +388,7 @@ export interface IContainer extends IEventProvider { * resulted in disposing it. */ dispose(error?: ICriticalContainerError): void; - - /** - * Disposes the container. If not already closed, this acts as a closure and then disposes runtime resources. - * The container is not expected to be used anymore once it is disposed. - * - * @param disconnectReason - The reason for disconnecting the container - * @param error - If the container is being disposed due to error, this provides details about the error that - * resulted in disposing it. - */ - dispose(disconnectReason: "Corruption", error: ICriticalContainerError): void; - - /** - * Disposes the container. If not already closed, this acts as a closure and then disposes runtime resources. - * The container is not expected to be used anymore once it is disposed. - * - * @param disconnectReason - The reason for disconnecting the container - * @param error - If the container is being disposed due to error, this provides details about the error that - * resulted in disposing it. - */ - dispose(disconnectReason: "Expected" | "Unknown", error?: ICriticalContainerError): void; + dispose(error?: ICriticalContainerError, disconnectReason?: DisconnectReason): void; /** * Closes the container. @@ -415,24 +397,7 @@ export interface IContainer extends IEventProvider { * resulted in closing it. */ close(error?: ICriticalContainerError): void; - - /** - * Closes the container. - * - * @param disconnectReason - The reason for disconnecting the container - * @param error - If the container is being closed due to error, this provides details about the error that - * resulted in closing it. - */ - close(disconnectReason: "Corruption", error: ICriticalContainerError): void; - - /** - * Closes the container. - * - * @param disconnectReason - The reason for disconnecting the container - * @param error - If the container is being closed due to error, this provides details about the error that - * resulted in closing it. - */ - close(disconnectReason: "Expected" | "Unknown", error?: ICriticalContainerError): void; + close(error?: ICriticalContainerError, disconnectReason?: DisconnectReason): void; /** * Propose new code details that define the code to be loaded for this container's runtime. @@ -764,6 +729,10 @@ export interface IContainerLoadMode { * @internal */ export interface ILoaderHeader { + /** + * @deprecated This header has been deprecated and will be removed in a future release + */ + [LoaderHeader.cache]: boolean; [LoaderHeader.clientDetails]: IClientDetails; [LoaderHeader.loadMode]: IContainerLoadMode; [LoaderHeader.reconnect]: boolean; diff --git a/packages/framework/fluid-static/src/fluidContainer.ts b/packages/framework/fluid-static/src/fluidContainer.ts index cc9416c9e95c..2a32e863c4e3 100644 --- a/packages/framework/fluid-static/src/fluidContainer.ts +++ b/packages/framework/fluid-static/src/fluidContainer.ts @@ -357,7 +357,7 @@ class FluidContainer } public dispose(): void { - this.container.close(DisconnectReason.Unknown); + this.container.close(undefined, DisconnectReason.Unknown); this.container.off("connected", this.connectedHandler); this.container.off("closed", this.disposedHandler); this.container.off("disconnected", this.disconnectedHandler); diff --git a/packages/loader/container-loader/src/connectionManager.ts b/packages/loader/container-loader/src/connectionManager.ts index c2bfe1c385fb..6473c3b8c112 100644 --- a/packages/loader/container-loader/src/connectionManager.ts +++ b/packages/loader/container-loader/src/connectionManager.ts @@ -11,10 +11,7 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { - DisconnectReason, - isDisconnectReason, -} from "@fluidframework/core-interfaces/internal"; +import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode, IClient, IClientDetails } from "@fluidframework/driver-definitions"; import { @@ -430,9 +427,9 @@ export class ConnectionManager implements IConnectionManager { } public dispose( - disconnectReasonOrError?: DisconnectReason | ICriticalContainerError, - errorOrSwitchToReadonly?: ICriticalContainerError | boolean, + error?: ICriticalContainerError, switchToReadonly: boolean = true, + reason: DisconnectReason = DisconnectReason.Unknown, ): void { if (this._disposed) { return; @@ -443,33 +440,18 @@ export class ConnectionManager implements IConnectionManager { this._reconnectMode = ReconnectMode.Never; this._outbound.clear(); - let disconnectReasonParam: DisconnectReason; - let error: ICriticalContainerError | undefined; - if (isDisconnectReason(disconnectReasonOrError)) { - error = - typeof errorOrSwitchToReadonly === "boolean" ? undefined : errorOrSwitchToReadonly; - disconnectReasonParam = disconnectReasonOrError; - } else { - error = disconnectReasonOrError; - disconnectReasonParam = DisconnectReason.Unknown; - } - - const finalSwitchToReadonly = - typeof errorOrSwitchToReadonly === "boolean" - ? errorOrSwitchToReadonly - : switchToReadonly; const disconnectReason: IConnectionStateChangeReason = { text: "Closing DeltaManager", error, - disconnectReason: disconnectReasonParam, + disconnectReason: reason, }; const oldReadonlyValue = this.readonly; // This raises "disconnect" event if we have active connection. this.disconnectFromDeltaStream(disconnectReason); - if (finalSwitchToReadonly) { + if (switchToReadonly) { // Notify everyone we are in read-only state. // Useful for data stores in case we hit some critical error, // to switch to a mode where user edits are not accepted @@ -835,11 +817,7 @@ export class ConnectionManager implements IConnectionManager { // eslint-disable-next-line @typescript-eslint/no-floating-promises this._outbound.pause(); this._outbound.clear(); - connection.dispose({ - ...reason.error, - name: "disconnectFromDeltaStream", - message: reason.disconnectReason ?? DisconnectReason.Unknown, - }); + connection.dispose(); this.props.disconnectHandler(reason); diff --git a/packages/loader/container-loader/src/container.ts b/packages/loader/container-loader/src/container.ts index 3faabbc92430..d5c087ae68f1 100644 --- a/packages/loader/container-loader/src/container.ts +++ b/packages/loader/container-loader/src/container.ts @@ -5,11 +5,7 @@ /* eslint-disable unicorn/consistent-function-scoping */ -import { - TypedEventEmitter, - performance, - type ILayerCompatDetails, -} from "@fluid-internal/client-utils"; +import { TypedEventEmitter, performance } from "@fluid-internal/client-utils"; import { AttachState, IAudience, @@ -130,7 +126,6 @@ import { getPackageName, } from "./contracts.js"; import { DeltaManager, IConnectionArgs } from "./deltaManager.js"; -import { validateRuntimeCompatibility } from "./layerCompatState.js"; // eslint-disable-next-line import/no-deprecated import { IDetachedBlobStorage, ILoaderOptions, RelativeLoader } from "./loader.js"; import { @@ -429,8 +424,8 @@ export class Container // Note: We could only dispose the container instead of just close but that would // the telemetry where users sometimes search for ContainerClose event to look // for load failures. So not removing this at this time. - container.close(DisconnectReason.Unknown, err); - container.dispose(DisconnectReason.Unknown, err); + container.close(err); + container.dispose(err); onClosed(err); }, ); @@ -795,7 +790,7 @@ export class Container }, error, ); - this.close(DisconnectReason.Unknown, normalizeError(error)); + this.close(normalizeError(error)); }); const { @@ -976,7 +971,7 @@ export class Container this.clientsWhoShouldHaveLeft.add(clientId); }, onCriticalError: (error: unknown) => { - this.close(DisconnectReason.Unknown, normalizeError(error)); + this.close(normalizeError(error)); }, }, this.deltaManager, @@ -1062,33 +1057,21 @@ export class Container } public dispose( - errorOrReason?: ICriticalContainerError | DisconnectReason, error?: ICriticalContainerError, + reason: DisconnectReason = DisconnectReason.Unknown, ): void { - if (typeof errorOrReason === "string") { - // Called as dispose(disconnectReason, error) - this.verifyClosedAfter(() => this._deltaManager.dispose(errorOrReason, error)); - } else { - // Called as dispose(error) - this.verifyClosedAfter(() => this._deltaManager.dispose(undefined, errorOrReason)); - } + this.verifyClosedAfter(() => this._deltaManager.dispose(error, reason)); } public close( - errorOrReason?: ICriticalContainerError | DisconnectReason, error?: ICriticalContainerError, + reason: DisconnectReason = DisconnectReason.Unknown, ): void { // 1. Ensure that close sequence is exactly the same no matter if it's initiated by host or by DeltaManager // 2. We need to ensure that we deliver disconnect event to runtime properly. See connectionStateChanged // handler. We only deliver events if container fully loaded. Transitioning from "loading" -> // "closing" will lose that info (can also solve by tracking extra state). - if (typeof errorOrReason === "string") { - // Called as close(disconnectReason, error) - this.verifyClosedAfter(() => this._deltaManager.close(errorOrReason, error)); - } else { - // Called as close(error) - this.verifyClosedAfter(() => this._deltaManager.close(undefined, errorOrReason)); - } + this.verifyClosedAfter(() => this._deltaManager.close(error, reason)); } private verifyClosedAfterCalls = 0; @@ -1157,7 +1140,7 @@ export class Container // There is no user for summarizer, so we need to ensure dispose is called if (this.client.details.type === summarizerClientType) { - this.dispose(DisconnectReason.Unknown, error); + this.dispose(error); } } } @@ -1225,7 +1208,7 @@ export class Container notifyImminentClosure: true, stopBlobAttachingSignal, }); - this.close(DisconnectReason.Expected); + this.close(); return pendingState; } @@ -1325,7 +1308,7 @@ export class Container const normalizeErrorAndClose = (error: unknown): IFluidErrorBase => { const newError = normalizeError(error); - this.close(DisconnectReason.Unknown, newError); + this.close(newError); // add resolved URL on error object so that host has the ability to find this document and delete it newError.addTelemetryProperties({ resolvedUrl: this.service?.resolvedUrl?.url, @@ -1570,7 +1553,7 @@ export class Container // pre-0.58 error message: existingContextDoesNotSatisfyIncomingProposal const error = new GenericError("Existing context does not satisfy incoming proposal"); - this.close(DisconnectReason.Unknown, error); + this.close(error); } /** @@ -1963,7 +1946,7 @@ export class Container } this.processCodeProposal().catch((error) => { const normalizedError = normalizeError(error); - this.close(DisconnectReason.Unknown, normalizedError); + this.close(normalizedError); throw error; }); } @@ -2283,7 +2266,7 @@ export class Container undefined /* error */, { messageType: type }, ); - this.close(DisconnectReason.Unknown, newError); + this.close(newError); return -1; } } @@ -2472,8 +2455,8 @@ export class Container (batch: IBatchMessage[], referenceSequenceNumber?: number) => this.submitBatch(batch, referenceSequenceNumber), (content, targetClientId) => this.submitSignal(content, targetClientId), - (error?: ICriticalContainerError) => this.dispose(DisconnectReason.Unknown, error), - (error?: ICriticalContainerError) => this.close(DisconnectReason.Unknown, error), + (error?: ICriticalContainerError) => this.dispose(error), + (error?: ICriticalContainerError) => this.close(error), this.updateDirtyContainerState, this.getAbsoluteUrl, () => this.resolvedUrl?.id, @@ -2487,19 +2470,11 @@ export class Container snapshot, ); - const runtime = await PerformanceEvent.timedExecAsync( + this._runtime = await PerformanceEvent.timedExecAsync( this.subLogger, { eventName: "InstantiateRuntime" }, async () => runtimeFactory.instantiateRuntime(context, existing), ); - - const maybeRuntimeCompatDetails = runtime as FluidObject; - validateRuntimeCompatibility(maybeRuntimeCompatDetails.ILayerCompatDetails, (error) => - this.dispose(error), - ); - - this._runtime = runtime; - this._lifecycleEvents.emit("runtimeInstantiated"); this._loadedCodeDetails = codeDetails; diff --git a/packages/loader/container-loader/src/contracts.ts b/packages/loader/container-loader/src/contracts.ts index f0addc5d6351..7bcdd03cb499 100644 --- a/packages/loader/container-loader/src/contracts.ts +++ b/packages/loader/container-loader/src/contracts.ts @@ -127,13 +127,12 @@ export interface IConnectionManager { /** * Disposed connection manager */ + dispose(error?: ICriticalContainerError, switchToReadonly?: boolean): void; dispose( - disconnectReason?: DisconnectReason, error?: ICriticalContainerError, switchToReadonly?: boolean, + disconnectReason?: DisconnectReason, ): void; - dispose(error?: ICriticalContainerError, switchToReadonly?: boolean): void; - get connectionMode(): ConnectionMode; } diff --git a/packages/loader/container-loader/src/deltaManager.ts b/packages/loader/container-loader/src/deltaManager.ts index 784a6edd7b49..9f9bcc42d361 100644 --- a/packages/loader/container-loader/src/deltaManager.ts +++ b/packages/loader/container-loader/src/deltaManager.ts @@ -19,7 +19,6 @@ import { import { DisconnectReason, IThrottlingWarning, - isDisconnectReason, } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode } from "@fluidframework/driver-definitions"; @@ -426,7 +425,7 @@ export class DeltaManager }, error, ); - this.close(DisconnectReason.Unknown, normalizeError(error)); + this.close(normalizeError(error)); }); const props: IConnectionManagerFactoryArgs = { incomingOpHandler: (messages: ISequencedDocumentMessage[], reason: string) => { @@ -434,7 +433,7 @@ export class DeltaManager this.enqueueMessages(messages, reason); } catch (error) { this.logger.sendErrorEvent({ eventName: "EnqueueMessages_Exception" }, error); - this.close(DisconnectReason.Unknown, normalizeError(error)); + this.close(normalizeError(error)); } }, signalHandler: (signals: ISignalMessage[]) => { @@ -444,8 +443,7 @@ export class DeltaManager }, reconnectionDelayHandler: (delayMs: number, error: unknown) => this.emitDelayInfo(this.deltaStreamDelayId, delayMs, error), - closeHandler: (error: ICriticalContainerError | undefined) => - this.close(DisconnectReason.Unknown, error), + closeHandler: (error: ICriticalContainerError | undefined) => this.close(error), disconnectHandler: (reason: IConnectionStateChangeReason) => this.disconnectHandler(reason), connectHandler: (connection: IConnectionDetailsInternal) => @@ -470,7 +468,6 @@ export class DeltaManager this._inbound.on("error", (error) => { this.close( - DisconnectReason.Unknown, DataProcessingError.wrapIfUnrecognized( error, "deltaManagerInboundErrorHandler", @@ -492,7 +489,7 @@ export class DeltaManager }); this._inboundSignal.on("error", (error) => { - this.close(DisconnectReason.Unknown, normalizeError(error)); + this.close(normalizeError(error)); }); // Initially, all queues are created paused. @@ -766,28 +763,17 @@ export class DeltaManager * - close cannot be called after dispose */ public close( - errorOrReason?: ICriticalContainerError | DisconnectReason, error?: ICriticalContainerError, + disconnectReason: DisconnectReason = DisconnectReason.Unknown, ): void { if (this._closed) { return; } this._closed = true; - let finalError: ICriticalContainerError | undefined; - let disconnectReason: DisconnectReason; - - if (isDisconnectReason(errorOrReason)) { - finalError = error; - disconnectReason = errorOrReason; - } else { - finalError = errorOrReason; - disconnectReason = DisconnectReason.Unknown; - } - - this.connectionManager.dispose(disconnectReason, finalError, true /* switchToReadonly */); + this.connectionManager.dispose(error, true /* switchToReadonly */, disconnectReason); this.clearQueues(); - this.emit("closed", finalError); + this.emit("closed", error); } /** @@ -799,8 +785,8 @@ export class DeltaManager * - dispose can be called after closure */ public dispose( - disconnectReason?: DisconnectReason, error?: Error | ICriticalContainerError, + disconnectReason: DisconnectReason = DisconnectReason.Unknown, ): void { if (this._disposed) { return; @@ -812,7 +798,7 @@ export class DeltaManager this._disposed = true; this._closed = true; // We consider "disposed" as a further state than "closed" - this.connectionManager.dispose(disconnectReason, error, false /* switchToReadonly */); + this.connectionManager.dispose(error, false /* switchToReadonly */, disconnectReason); this.clearQueues(); // This needs to be the last thing we do (before removing listeners), as it causes @@ -1014,7 +1000,7 @@ export class DeltaManager driverVersion: undefined, }, ); - this.close(DisconnectReason.Unknown, error); + this.close(error); } } } else if (message.sequenceNumber === this.lastQueuedSequenceNumber + 1) { @@ -1198,7 +1184,7 @@ export class DeltaManager ); } catch (error) { this.logger.sendErrorEvent({ eventName: "GetDeltas_Exception" }, error); - this.close(DisconnectReason.Unknown, normalizeError(error)); + this.close(normalizeError(error)); } finally { this.refreshDelayInfo(this.deltaStorageDelayId); this.fetchReason = undefined; diff --git a/packages/loader/container-loader/src/loadPaused.ts b/packages/loader/container-loader/src/loadPaused.ts index 64ded60214fa..a5c10c46413b 100644 --- a/packages/loader/container-loader/src/loadPaused.ts +++ b/packages/loader/container-loader/src/loadPaused.ts @@ -96,7 +96,7 @@ export async function loadContainerPaused( const error = new GenericError( "Cannot satisfy request to pause the container at the specified sequence number. Most recent snapshot is newer than the specified sequence number.", ); - container.close(DisconnectReason.Unknown, error); + container.close(error, DisconnectReason.Unknown); throw error; } @@ -140,7 +140,7 @@ export async function loadContainerPaused( // eslint-disable-next-line @typescript-eslint/no-explicit-any .catch((error: any) => { // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - container.close(DisconnectReason.Unknown, error); + container.close(error, DisconnectReason.Unknown); throw error; }) .finally(() => { diff --git a/packages/loader/container-loader/src/test/deltaManager.spec.ts b/packages/loader/container-loader/src/test/deltaManager.spec.ts index 0a5230b96ef0..15f8b3f5aed9 100644 --- a/packages/loader/container-loader/src/test/deltaManager.spec.ts +++ b/packages/loader/container-loader/src/test/deltaManager.spec.ts @@ -535,7 +535,7 @@ describe("Loader", () => { })); // Dispose will trigger abort - deltaManager.dispose(DisconnectReason.Expected); + deltaManager.dispose(undefined, DisconnectReason.Expected); await flushPromises(); mockLogger.assertMatch([ diff --git a/packages/tools/devtools/devtools-core/src/test/Utilities.ts b/packages/tools/devtools/devtools-core/src/test/Utilities.ts index cc791a1a7c46..d7479562e5a6 100644 --- a/packages/tools/devtools/devtools-core/src/test/Utilities.ts +++ b/packages/tools/devtools/devtools-core/src/test/Utilities.ts @@ -46,7 +46,7 @@ class MockContainer this.emit("attached"); } - public dispose(errorOrReason?: IErrorBase | DisconnectReason, error?: IErrorBase): void { + public dispose(error?: IErrorBase, disconnectReason?: DisconnectReason): void { this.emit("disposed"); } From eb1a10fc43746ca523dca4521512492a02f381fe Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 19:05:48 +0000 Subject: [PATCH 16/54] undo random delete --- .../loader/container-loader/src/container.ts | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/packages/loader/container-loader/src/container.ts b/packages/loader/container-loader/src/container.ts index d5c087ae68f1..786eef6dd9df 100644 --- a/packages/loader/container-loader/src/container.ts +++ b/packages/loader/container-loader/src/container.ts @@ -5,7 +5,11 @@ /* eslint-disable unicorn/consistent-function-scoping */ -import { TypedEventEmitter, performance } from "@fluid-internal/client-utils"; +import { + TypedEventEmitter, + performance, + type ILayerCompatDetails, +} from "@fluid-internal/client-utils"; import { AttachState, IAudience, @@ -37,10 +41,7 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { - DisconnectReason, - type ISignalEnvelope, -} from "@fluidframework/core-interfaces/internal"; +import { DisconnectReason, type ISignalEnvelope } from "@fluidframework/core-interfaces/internal"; import { assert, isPromiseLike, unreachableCase } from "@fluidframework/core-utils/internal"; import { IClient, @@ -126,6 +127,7 @@ import { getPackageName, } from "./contracts.js"; import { DeltaManager, IConnectionArgs } from "./deltaManager.js"; +import { validateRuntimeCompatibility } from "./layerCompatState.js"; // eslint-disable-next-line import/no-deprecated import { IDetachedBlobStorage, ILoaderOptions, RelativeLoader } from "./loader.js"; import { @@ -2470,11 +2472,19 @@ export class Container snapshot, ); - this._runtime = await PerformanceEvent.timedExecAsync( + const runtime = await PerformanceEvent.timedExecAsync( this.subLogger, { eventName: "InstantiateRuntime" }, async () => runtimeFactory.instantiateRuntime(context, existing), ); + + const maybeRuntimeCompatDetails = runtime as FluidObject; + validateRuntimeCompatibility(maybeRuntimeCompatDetails.ILayerCompatDetails, (error) => + this.dispose(error), + ); + + this._runtime = runtime; + this._lifecycleEvents.emit("runtimeInstantiated"); this._loadedCodeDetails = codeDetails; From 95f63ed3c7d349a41b12d23770bc61baa0459f93 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 19:07:52 +0000 Subject: [PATCH 17/54] format --- packages/loader/container-loader/src/container.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/loader/container-loader/src/container.ts b/packages/loader/container-loader/src/container.ts index 786eef6dd9df..8e514bf83825 100644 --- a/packages/loader/container-loader/src/container.ts +++ b/packages/loader/container-loader/src/container.ts @@ -41,7 +41,10 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { DisconnectReason, type ISignalEnvelope } from "@fluidframework/core-interfaces/internal"; +import { + DisconnectReason, + type ISignalEnvelope, +} from "@fluidframework/core-interfaces/internal"; import { assert, isPromiseLike, unreachableCase } from "@fluidframework/core-utils/internal"; import { IClient, From 19ebadbf77db4ad98146edba80446dd43a7d5184 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 19:20:11 +0000 Subject: [PATCH 18/54] prettier for server --- .../routerlicious/packages/lambdas/src/nexus/index.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index a3b751444ea5..e63d000ad07b 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -641,11 +641,11 @@ export function configureWebSocketServices( socket.on( "disconnect_document", (clientId: string, documentId: string, disconnectReason: string) => { - const disconnectReasonDisplay: Record = { - "Expected": "Client disconnected normally", - "Corruption": "Client disconnected due to data corruption", - "Unknown": "Client disconnected unexpectedly" - }; + const disconnectReasonDisplay: Record = { + Expected: "Client disconnected normally", + Corruption: "Client disconnected due to data corruption", + Unknown: "Client disconnected unexpectedly", + }; Lumberjack.error( disconnectReasonDisplay[disconnectReason] || "Client disconnected unexpectedly", { clientId, documentId }, From 1d5e99c7746257e167b72c291478cc1131826cdb Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 19:28:39 +0000 Subject: [PATCH 19/54] undo loader changes --- .../container-definitions/src/loader.ts | 41 +++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index f7654f4ba520..266d1aa70f70 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -9,7 +9,6 @@ import type { IEventProvider, IRequest, } from "@fluidframework/core-interfaces"; -import type { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import type { IClient, IClientDetails, @@ -388,7 +387,26 @@ export interface IContainer extends IEventProvider { * resulted in disposing it. */ dispose(error?: ICriticalContainerError): void; - dispose(error?: ICriticalContainerError, disconnectReason?: DisconnectReason): void; + + /** + * Disposes the container. If not already closed, this acts as a closure and then disposes runtime resources. + * The container is not expected to be used anymore once it is disposed. + * + * @param disconnectReason - The reason for disconnecting the container + * @param error - If the container is being disposed due to error, this provides details about the error that + * resulted in disposing it. + */ + dispose(disconnectReason: "Corruption", error: ICriticalContainerError): void; + + /** + * Disposes the container. If not already closed, this acts as a closure and then disposes runtime resources. + * The container is not expected to be used anymore once it is disposed. + * + * @param disconnectReason - The reason for disconnecting the container + * @param error - If the container is being disposed due to error, this provides details about the error that + * resulted in disposing it. + */ + dispose(error?: ICriticalContainerError, disconnectReason?: "Expected" | "Unknown"): void; /** * Closes the container. @@ -397,7 +415,24 @@ export interface IContainer extends IEventProvider { * resulted in closing it. */ close(error?: ICriticalContainerError): void; - close(error?: ICriticalContainerError, disconnectReason?: DisconnectReason): void; + + /** + * Closes the container. + * + * @param disconnectReason - The reason for disconnecting the container + * @param error - If the container is being closed due to error, this provides details about the error that + * resulted in closing it. + */ + close(error: ICriticalContainerError, disconnectReason: "Corruption"): void; + + /** + * Closes the container. + * + * @param disconnectReason - The reason for disconnecting the container + * @param error - If the container is being closed due to error, this provides details about the error that + * resulted in closing it. + */ + close(error?: ICriticalContainerError, disconnectReason?: "Expected" | "Unknown"): void; /** * Propose new code details that define the code to be loaded for this container's runtime. From 8730cb1ad6727fa06243f0d7a2a028120793afd6 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 19:35:51 +0000 Subject: [PATCH 20/54] fix typo --- .../api-report/container-definitions.legacy.alpha.api.md | 8 ++++---- packages/common/container-definitions/src/loader.ts | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md index bf698447a779..af2c90a91db5 100644 --- a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md +++ b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md @@ -99,8 +99,8 @@ export interface IContainer extends IEventProvider { readonly audience: IAudience; readonly clientId?: string | undefined; close(error?: ICriticalContainerError): void; - // (undocumented) - close(error?: ICriticalContainerError, disconnectReason?: DisconnectReason): void; + close(error: ICriticalContainerError, disconnectReason: "Corruption"): void; + close(error?: ICriticalContainerError, disconnectReason?: "Expected" | "Unknown"): void; readonly closed: boolean; connect(): void; readonly connectionState: ConnectionState; @@ -108,8 +108,8 @@ export interface IContainer extends IEventProvider { deltaManager: IDeltaManager; disconnect(): void; dispose(error?: ICriticalContainerError): void; - // (undocumented) - dispose(error?: ICriticalContainerError, disconnectReason?: DisconnectReason): void; + dispose(disconnectReason: "Corruption", error: ICriticalContainerError): void; + dispose(error?: ICriticalContainerError, disconnectReason?: "Expected" | "Unknown"): void; readonly disposed?: boolean; forceReadonly?(readonly: boolean): any; getAbsoluteUrl(relativeUrl: string): Promise; diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 266d1aa70f70..ba91c507db08 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -396,7 +396,7 @@ export interface IContainer extends IEventProvider { * @param error - If the container is being disposed due to error, this provides details about the error that * resulted in disposing it. */ - dispose(disconnectReason: "Corruption", error: ICriticalContainerError): void; + dispose(error: ICriticalContainerError, disconnectReason: "Corruption"): void; /** * Disposes the container. If not already closed, this acts as a closure and then disposes runtime resources. From 65960f4d4dcad141f1782d0f2bf6d0ca28967179 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 19:37:32 +0000 Subject: [PATCH 21/54] api-doc --- .../api-report/container-definitions.legacy.alpha.api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md index af2c90a91db5..ec771b36de90 100644 --- a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md +++ b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md @@ -108,7 +108,7 @@ export interface IContainer extends IEventProvider { deltaManager: IDeltaManager; disconnect(): void; dispose(error?: ICriticalContainerError): void; - dispose(disconnectReason: "Corruption", error: ICriticalContainerError): void; + dispose(error: ICriticalContainerError, disconnectReason: "Corruption"): void; dispose(error?: ICriticalContainerError, disconnectReason?: "Expected" | "Unknown"): void; readonly disposed?: boolean; forceReadonly?(readonly: boolean): any; From a5d3b357733445cfc579e94d848feb31b3d274b2 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 21:26:40 +0000 Subject: [PATCH 22/54] remove IContainer changes --- .../container-definitions.legacy.alpha.api.md | 4 -- .../container-definitions/src/loader.ts | 42 ------------------- .../fluid-static/src/fluidContainer.ts | 3 +- .../loader/container-loader/src/loadPaused.ts | 5 +-- 4 files changed, 3 insertions(+), 51 deletions(-) diff --git a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md index ec771b36de90..e51af312c11a 100644 --- a/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md +++ b/packages/common/container-definitions/api-report/container-definitions.legacy.alpha.api.md @@ -99,8 +99,6 @@ export interface IContainer extends IEventProvider { readonly audience: IAudience; readonly clientId?: string | undefined; close(error?: ICriticalContainerError): void; - close(error: ICriticalContainerError, disconnectReason: "Corruption"): void; - close(error?: ICriticalContainerError, disconnectReason?: "Expected" | "Unknown"): void; readonly closed: boolean; connect(): void; readonly connectionState: ConnectionState; @@ -108,8 +106,6 @@ export interface IContainer extends IEventProvider { deltaManager: IDeltaManager; disconnect(): void; dispose(error?: ICriticalContainerError): void; - dispose(error: ICriticalContainerError, disconnectReason: "Corruption"): void; - dispose(error?: ICriticalContainerError, disconnectReason?: "Expected" | "Unknown"): void; readonly disposed?: boolean; forceReadonly?(readonly: boolean): any; getAbsoluteUrl(relativeUrl: string): Promise; diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index ba91c507db08..43431aaaeb15 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -388,26 +388,6 @@ export interface IContainer extends IEventProvider { */ dispose(error?: ICriticalContainerError): void; - /** - * Disposes the container. If not already closed, this acts as a closure and then disposes runtime resources. - * The container is not expected to be used anymore once it is disposed. - * - * @param disconnectReason - The reason for disconnecting the container - * @param error - If the container is being disposed due to error, this provides details about the error that - * resulted in disposing it. - */ - dispose(error: ICriticalContainerError, disconnectReason: "Corruption"): void; - - /** - * Disposes the container. If not already closed, this acts as a closure and then disposes runtime resources. - * The container is not expected to be used anymore once it is disposed. - * - * @param disconnectReason - The reason for disconnecting the container - * @param error - If the container is being disposed due to error, this provides details about the error that - * resulted in disposing it. - */ - dispose(error?: ICriticalContainerError, disconnectReason?: "Expected" | "Unknown"): void; - /** * Closes the container. * @@ -416,24 +396,6 @@ export interface IContainer extends IEventProvider { */ close(error?: ICriticalContainerError): void; - /** - * Closes the container. - * - * @param disconnectReason - The reason for disconnecting the container - * @param error - If the container is being closed due to error, this provides details about the error that - * resulted in closing it. - */ - close(error: ICriticalContainerError, disconnectReason: "Corruption"): void; - - /** - * Closes the container. - * - * @param disconnectReason - The reason for disconnecting the container - * @param error - If the container is being closed due to error, this provides details about the error that - * resulted in closing it. - */ - close(error?: ICriticalContainerError, disconnectReason?: "Expected" | "Unknown"): void; - /** * Propose new code details that define the code to be loaded for this container's runtime. * @@ -764,10 +726,6 @@ export interface IContainerLoadMode { * @internal */ export interface ILoaderHeader { - /** - * @deprecated This header has been deprecated and will be removed in a future release - */ - [LoaderHeader.cache]: boolean; [LoaderHeader.clientDetails]: IClientDetails; [LoaderHeader.loadMode]: IContainerLoadMode; [LoaderHeader.reconnect]: boolean; diff --git a/packages/framework/fluid-static/src/fluidContainer.ts b/packages/framework/fluid-static/src/fluidContainer.ts index 2a32e863c4e3..27620fccc445 100644 --- a/packages/framework/fluid-static/src/fluidContainer.ts +++ b/packages/framework/fluid-static/src/fluidContainer.ts @@ -11,7 +11,6 @@ import { } from "@fluidframework/container-definitions"; import type { IContainer } from "@fluidframework/container-definitions/internal"; import type { IEvent, IEventProvider, IFluidLoadable } from "@fluidframework/core-interfaces"; -import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import type { SharedObjectKind } from "@fluidframework/shared-object-base"; import type { ContainerAttachProps, ContainerSchema, IRootDataObject } from "./types.js"; @@ -357,7 +356,7 @@ class FluidContainer } public dispose(): void { - this.container.close(undefined, DisconnectReason.Unknown); + this.container.close(); this.container.off("connected", this.connectedHandler); this.container.off("closed", this.disposedHandler); this.container.off("disconnected", this.disconnectedHandler); diff --git a/packages/loader/container-loader/src/loadPaused.ts b/packages/loader/container-loader/src/loadPaused.ts index a5c10c46413b..c73f5aab5db8 100644 --- a/packages/loader/container-loader/src/loadPaused.ts +++ b/packages/loader/container-loader/src/loadPaused.ts @@ -10,7 +10,6 @@ import { } from "@fluidframework/container-definitions/internal"; import { IRequest } from "@fluidframework/core-interfaces"; import type { IErrorBase } from "@fluidframework/core-interfaces"; -import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { GenericError } from "@fluidframework/telemetry-utils/internal"; @@ -96,7 +95,7 @@ export async function loadContainerPaused( const error = new GenericError( "Cannot satisfy request to pause the container at the specified sequence number. Most recent snapshot is newer than the specified sequence number.", ); - container.close(error, DisconnectReason.Unknown); + container.close(error); throw error; } @@ -140,7 +139,7 @@ export async function loadContainerPaused( // eslint-disable-next-line @typescript-eslint/no-explicit-any .catch((error: any) => { // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - container.close(error, DisconnectReason.Unknown); + container.close(error); throw error; }) .finally(() => { From fe68cc72649db3888c880bb216a7c8e013d72af9 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 27 Jan 2025 22:53:25 +0000 Subject: [PATCH 23/54] change to internal --- .../api-report/core-interfaces.legacy.alpha.api.md | 13 ------------- .../common/core-interfaces/src/disconnectReason.ts | 6 +++--- 2 files changed, 3 insertions(+), 16 deletions(-) diff --git a/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md b/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md index 1137855347f8..8adbe20f8ea7 100644 --- a/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md +++ b/packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md @@ -7,16 +7,6 @@ // @public export type ConfigTypes = string | number | boolean | number[] | string[] | boolean[] | undefined; -// @alpha -export const DisconnectReason: { - readonly Expected: "Expected"; - readonly Corruption: "Corruption"; - readonly Unknown: "Unknown"; -}; - -// @alpha -export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; - // @public @sealed export abstract class ErasedType { static [Symbol.hasInstance](value: never): value is never; @@ -345,9 +335,6 @@ export interface IResponse { value: any; } -// @alpha -export const isDisconnectReason: (value: unknown) => value is DisconnectReason; - // @public export type IsListener = TListener extends (...args: any[]) => void ? true : false; diff --git a/packages/common/core-interfaces/src/disconnectReason.ts b/packages/common/core-interfaces/src/disconnectReason.ts index 3d87c14ec742..36c86b28f602 100644 --- a/packages/common/core-interfaces/src/disconnectReason.ts +++ b/packages/common/core-interfaces/src/disconnectReason.ts @@ -7,7 +7,7 @@ * Potential reasons for disconnect events emitted by {@link @fluidframework/container-definitions#IContainer}. * * @legacy - * @alpha + * @internal */ export const DisconnectReason = { Expected: "Expected", @@ -18,7 +18,7 @@ export const DisconnectReason = { /** * {@inheritDoc (DisconnectReason:variable)} * @legacy - * @alpha + * @internal */ export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; @@ -28,7 +28,7 @@ export type DisconnectReason = (typeof DisconnectReason)[keyof typeof Disconnect * @param value - The value to check * @returns True if the value is a valid DisconnectReason, false otherwise * @legacy - * @alpha + * @internal */ export const isDisconnectReason = (value: unknown): value is DisconnectReason => value !== undefined && From 03db41d934a25f206e64187876d8db272ec8ae08 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 29 Jan 2025 21:23:57 +0000 Subject: [PATCH 24/54] wip --- .../core-interfaces/src/disconnectReason.ts | 36 ------------------- packages/common/core-interfaces/src/index.ts | 1 - .../src/documentDeltaConnection.ts | 13 ++----- .../container-loader/src/connectionManager.ts | 8 +---- .../loader/container-loader/src/container.ts | 19 +++------- .../loader/container-loader/src/contracts.ts | 8 +---- .../container-loader/src/deltaManager.ts | 19 +++------- .../src/test/deltaManager.spec.ts | 3 +- .../devtools-core/src/test/Utilities.ts | 3 +- 9 files changed, 16 insertions(+), 94 deletions(-) delete mode 100644 packages/common/core-interfaces/src/disconnectReason.ts diff --git a/packages/common/core-interfaces/src/disconnectReason.ts b/packages/common/core-interfaces/src/disconnectReason.ts deleted file mode 100644 index 36c86b28f602..000000000000 --- a/packages/common/core-interfaces/src/disconnectReason.ts +++ /dev/null @@ -1,36 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -/** - * Potential reasons for disconnect events emitted by {@link @fluidframework/container-definitions#IContainer}. - * - * @legacy - * @internal - */ -export const DisconnectReason = { - Expected: "Expected", - Corruption: "Corruption", - Unknown: "Unknown", -} as const; - -/** - * {@inheritDoc (DisconnectReason:variable)} - * @legacy - * @internal - */ -export type DisconnectReason = (typeof DisconnectReason)[keyof typeof DisconnectReason]; - -/** - * Type guard that checks if a value is a valid DisconnectReason. - * - * @param value - The value to check - * @returns True if the value is a valid DisconnectReason, false otherwise - * @legacy - * @internal - */ -export const isDisconnectReason = (value: unknown): value is DisconnectReason => - value !== undefined && - typeof value === "string" && - Object.values(DisconnectReason).includes(value as DisconnectReason); diff --git a/packages/common/core-interfaces/src/index.ts b/packages/common/core-interfaces/src/index.ts index 7b3988d84931..06796dab9223 100644 --- a/packages/common/core-interfaces/src/index.ts +++ b/packages/common/core-interfaces/src/index.ts @@ -4,7 +4,6 @@ */ export type { IDisposable } from "./disposable.js"; -export { DisconnectReason, isDisconnectReason } from "./disconnectReason.js"; export type { IErrorBase, IGenericError, IUsageError, IThrottlingWarning } from "./error.js"; export { FluidErrorTypes } from "./error.js"; diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 8ea2f191bfde..522b96c64b91 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -8,7 +8,6 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { isDisconnectReason } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode } from "@fluidframework/driver-definitions"; import { @@ -402,7 +401,7 @@ export class DocumentDeltaConnection * However the OdspDocumentDeltaConnection differ in dispose as in there we don't close the socket. There is no * multiplexing here, so we need to close the socket here. */ - public dispose(error?: Error) { + public dispose() { this.logger.sendTelemetryEvent({ eventName: "ClientClosingDeltaConnection", driverVersion, @@ -410,24 +409,16 @@ export class DocumentDeltaConnection ...this.getConnectionDetailsProps(), }), }); - this.disconnect( createGenericNetworkError( // pre-0.58 error message: clientClosingConnection - this.getDisconnectMessage(error), + "Client closing delta connection", { canRetry: true }, { driverVersion }, ), ); } - private getDisconnectMessage(error?: Error): string { - if (error?.message && isDisconnectReason(error.message)) { - return error.message; - } - return "Client closing delta connection"; - } - protected disconnect(err: IAnyDriverError) { // Can't check this.disposed here, as we get here on socket closure, // so _disposed & socket.connected might be not in sync while processing diff --git a/packages/loader/container-loader/src/connectionManager.ts b/packages/loader/container-loader/src/connectionManager.ts index 6473c3b8c112..711e00e8d25b 100644 --- a/packages/loader/container-loader/src/connectionManager.ts +++ b/packages/loader/container-loader/src/connectionManager.ts @@ -11,7 +11,6 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode, IClient, IClientDetails } from "@fluidframework/driver-definitions"; import { @@ -426,11 +425,7 @@ export class ConnectionManager implements IConnectionManager { }); } - public dispose( - error?: ICriticalContainerError, - switchToReadonly: boolean = true, - reason: DisconnectReason = DisconnectReason.Unknown, - ): void { + public dispose(error?: ICriticalContainerError, switchToReadonly: boolean = true): void { if (this._disposed) { return; } @@ -444,7 +439,6 @@ export class ConnectionManager implements IConnectionManager { const disconnectReason: IConnectionStateChangeReason = { text: "Closing DeltaManager", error, - disconnectReason: reason, }; const oldReadonlyValue = this.readonly; diff --git a/packages/loader/container-loader/src/container.ts b/packages/loader/container-loader/src/container.ts index 8e514bf83825..f8c24ff506eb 100644 --- a/packages/loader/container-loader/src/container.ts +++ b/packages/loader/container-loader/src/container.ts @@ -41,10 +41,7 @@ import { ITelemetryBaseProperties, LogLevel, } from "@fluidframework/core-interfaces"; -import { - DisconnectReason, - type ISignalEnvelope, -} from "@fluidframework/core-interfaces/internal"; +import { type ISignalEnvelope } from "@fluidframework/core-interfaces/internal"; import { assert, isPromiseLike, unreachableCase } from "@fluidframework/core-utils/internal"; import { IClient, @@ -1061,22 +1058,16 @@ export class Container return this.protocolHandler.quorum; } - public dispose( - error?: ICriticalContainerError, - reason: DisconnectReason = DisconnectReason.Unknown, - ): void { - this.verifyClosedAfter(() => this._deltaManager.dispose(error, reason)); + public dispose(error?: ICriticalContainerError): void { + this.verifyClosedAfter(() => this._deltaManager.dispose(error)); } - public close( - error?: ICriticalContainerError, - reason: DisconnectReason = DisconnectReason.Unknown, - ): void { + public close(error?: ICriticalContainerError): void { // 1. Ensure that close sequence is exactly the same no matter if it's initiated by host or by DeltaManager // 2. We need to ensure that we deliver disconnect event to runtime properly. See connectionStateChanged // handler. We only deliver events if container fully loaded. Transitioning from "loading" -> // "closing" will lose that info (can also solve by tracking extra state). - this.verifyClosedAfter(() => this._deltaManager.close(error, reason)); + this.verifyClosedAfter(() => this._deltaManager.close(error)); } private verifyClosedAfterCalls = 0; diff --git a/packages/loader/container-loader/src/contracts.ts b/packages/loader/container-loader/src/contracts.ts index 7bcdd03cb499..325ae75e6528 100644 --- a/packages/loader/container-loader/src/contracts.ts +++ b/packages/loader/container-loader/src/contracts.ts @@ -12,7 +12,6 @@ import { IConnectionDetails, } from "@fluidframework/container-definitions/internal"; import { IErrorBase, ITelemetryBaseProperties } from "@fluidframework/core-interfaces"; -import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { ConnectionMode, IClientDetails } from "@fluidframework/driver-definitions"; import { IContainerPackageInfo, @@ -32,7 +31,6 @@ export enum ReconnectMode { export interface IConnectionStateChangeReason { text: string; error?: T; - disconnectReason?: DisconnectReason; } /** @@ -128,11 +126,7 @@ export interface IConnectionManager { * Disposed connection manager */ dispose(error?: ICriticalContainerError, switchToReadonly?: boolean): void; - dispose( - error?: ICriticalContainerError, - switchToReadonly?: boolean, - disconnectReason?: DisconnectReason, - ): void; + dispose(error?: ICriticalContainerError, switchToReadonly?: boolean): void; get connectionMode(): ConnectionMode; } diff --git a/packages/loader/container-loader/src/deltaManager.ts b/packages/loader/container-loader/src/deltaManager.ts index 9f9bcc42d361..450ee5084fa8 100644 --- a/packages/loader/container-loader/src/deltaManager.ts +++ b/packages/loader/container-loader/src/deltaManager.ts @@ -16,10 +16,7 @@ import { type ITelemetryBaseEvent, ITelemetryBaseProperties, } from "@fluidframework/core-interfaces"; -import { - DisconnectReason, - IThrottlingWarning, -} from "@fluidframework/core-interfaces/internal"; +import { IThrottlingWarning } from "@fluidframework/core-interfaces/internal"; import { assert } from "@fluidframework/core-utils/internal"; import { ConnectionMode } from "@fluidframework/driver-definitions"; import { @@ -762,16 +759,13 @@ export class DeltaManager * - close emits "closed" * - close cannot be called after dispose */ - public close( - error?: ICriticalContainerError, - disconnectReason: DisconnectReason = DisconnectReason.Unknown, - ): void { + public close(error?: ICriticalContainerError): void { if (this._closed) { return; } this._closed = true; - this.connectionManager.dispose(error, true /* switchToReadonly */, disconnectReason); + this.connectionManager.dispose(error, true /* switchToReadonly */); this.clearQueues(); this.emit("closed", error); } @@ -784,10 +778,7 @@ export class DeltaManager * - dispose will remove all listeners * - dispose can be called after closure */ - public dispose( - error?: Error | ICriticalContainerError, - disconnectReason: DisconnectReason = DisconnectReason.Unknown, - ): void { + public dispose(error?: Error | ICriticalContainerError): void { if (this._disposed) { return; } @@ -798,7 +789,7 @@ export class DeltaManager this._disposed = true; this._closed = true; // We consider "disposed" as a further state than "closed" - this.connectionManager.dispose(error, false /* switchToReadonly */, disconnectReason); + this.connectionManager.dispose(error, false /* switchToReadonly */); this.clearQueues(); // This needs to be the last thing we do (before removing listeners), as it causes diff --git a/packages/loader/container-loader/src/test/deltaManager.spec.ts b/packages/loader/container-loader/src/test/deltaManager.spec.ts index 15f8b3f5aed9..622b1b837ba5 100644 --- a/packages/loader/container-loader/src/test/deltaManager.spec.ts +++ b/packages/loader/container-loader/src/test/deltaManager.spec.ts @@ -10,7 +10,6 @@ import { MockDocumentDeltaConnection, MockDocumentService, } from "@fluid-private/test-loader-utils"; -import { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import { IClient } from "@fluidframework/driver-definitions"; import { IDocumentDeltaStorageService, @@ -535,7 +534,7 @@ describe("Loader", () => { })); // Dispose will trigger abort - deltaManager.dispose(undefined, DisconnectReason.Expected); + deltaManager.dispose(); await flushPromises(); mockLogger.assertMatch([ diff --git a/packages/tools/devtools/devtools-core/src/test/Utilities.ts b/packages/tools/devtools/devtools-core/src/test/Utilities.ts index d7479562e5a6..40b812db1abe 100644 --- a/packages/tools/devtools/devtools-core/src/test/Utilities.ts +++ b/packages/tools/devtools/devtools-core/src/test/Utilities.ts @@ -11,7 +11,6 @@ import type { } from "@fluidframework/container-definitions/internal"; import { ConnectionState } from "@fluidframework/container-loader"; import type { IErrorBase, IRequest } from "@fluidframework/core-interfaces"; -import type { DisconnectReason } from "@fluidframework/core-interfaces/internal"; import type { IClient } from "@fluidframework/driver-definitions"; import { MockAudience } from "@fluidframework/test-runtime-utils/internal"; @@ -46,7 +45,7 @@ class MockContainer this.emit("attached"); } - public dispose(error?: IErrorBase, disconnectReason?: DisconnectReason): void { + public dispose(error?: IErrorBase): void { this.emit("disposed"); } From bf9844af552cbb740928e6bfce295834561e08cb Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 29 Jan 2025 22:25:22 +0000 Subject: [PATCH 25/54] isCorruption --- packages/common/container-definitions/src/loader.ts | 8 ++++++-- .../src/documentDeltaConnection.ts | 6 +++++- packages/loader/container-loader/src/contracts.ts | 2 +- .../devtools/devtools-core/src/test/Utilities.ts | 2 +- .../routerlicious/packages/lambdas/src/nexus/index.ts | 11 ++--------- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 43431aaaeb15..9742b3b3f569 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -194,7 +194,7 @@ export interface IContainerEvents extends IEvent { * * - `error`: If the container was closed due to error, this will contain details about the error that caused it. * - * @see {@link IContainer.(close:1)} + * @see {@link IContainer.close} */ (event: "closed", listener: (error?: ICriticalContainerError) => void); @@ -205,7 +205,7 @@ export interface IContainerEvents extends IEvent { * * - `error`: If the container was disposed due to error, this will contain details about the error that caused it. * - * @see {@link IContainer.(dispose:1)} + * @see {@link IContainer.dispose} */ (event: "disposed", listener: (error?: ICriticalContainerError) => void); @@ -726,6 +726,10 @@ export interface IContainerLoadMode { * @internal */ export interface ILoaderHeader { + /** + * @deprecated This header has been deprecated and will be removed in a future release + */ + [LoaderHeader.cache]: boolean; [LoaderHeader.clientDetails]: IClientDetails; [LoaderHeader.loadMode]: IContainerLoadMode; [LoaderHeader.reconnect]: boolean; diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index eb9808b083fd..4c46769844ac 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -3,6 +3,7 @@ * Licensed under the MIT License. */ +import { FluidErrorTypes } from "@fluidframework/core-interfaces/internal"; import { DocumentDeltaConnection } from "@fluidframework/driver-base/internal"; import { IClient } from "@fluidframework/driver-definitions"; import { @@ -113,7 +114,10 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { */ protected disconnectCore(err: IAnyDriverError): void { // tell the server we are disconnecting this client from the document - this.socket.emit("disconnect_document", this.clientId, this.documentId, err.message); + const isCorruption = + err.errorType === FluidErrorTypes.dataCorruptionError || + err.errorType === FluidErrorTypes.dataProcessingError; + this.socket.emit("disconnect_document", this.clientId, this.documentId, isCorruption); super.disconnectCore(err); } } diff --git a/packages/loader/container-loader/src/contracts.ts b/packages/loader/container-loader/src/contracts.ts index 325ae75e6528..f4a38a124a16 100644 --- a/packages/loader/container-loader/src/contracts.ts +++ b/packages/loader/container-loader/src/contracts.ts @@ -126,7 +126,7 @@ export interface IConnectionManager { * Disposed connection manager */ dispose(error?: ICriticalContainerError, switchToReadonly?: boolean): void; - dispose(error?: ICriticalContainerError, switchToReadonly?: boolean): void; + get connectionMode(): ConnectionMode; } diff --git a/packages/tools/devtools/devtools-core/src/test/Utilities.ts b/packages/tools/devtools/devtools-core/src/test/Utilities.ts index 40b812db1abe..1f980c3c9b4c 100644 --- a/packages/tools/devtools/devtools-core/src/test/Utilities.ts +++ b/packages/tools/devtools/devtools-core/src/test/Utilities.ts @@ -45,7 +45,7 @@ class MockContainer this.emit("attached"); } - public dispose(error?: IErrorBase): void { + public dispose(error?: IErrorBase | undefined): void { this.emit("disposed"); } diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index e63d000ad07b..9ebeb1958bbf 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -640,16 +640,9 @@ export function configureWebSocketServices( socket.on( "disconnect_document", - (clientId: string, documentId: string, disconnectReason: string) => { - const disconnectReasonDisplay: Record = { - Expected: "Client disconnected normally", - Corruption: "Client disconnected due to data corruption", - Unknown: "Client disconnected unexpectedly", - }; + (clientId: string, documentId: string, isCorruption: boolean) => { Lumberjack.error( - disconnectReasonDisplay[disconnectReason] || "Client disconnected unexpectedly", - { clientId, documentId }, - disconnectReason, + { clientId, documentId, isCorruption }, ); }, ); From 767ffe2b600083c24e41ca2ef648c6efe1a5ed0c Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 29 Jan 2025 22:26:41 +0000 Subject: [PATCH 26/54] undo loader change --- packages/common/container-definitions/src/loader.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/common/container-definitions/src/loader.ts b/packages/common/container-definitions/src/loader.ts index 9742b3b3f569..e3bd5335a1af 100644 --- a/packages/common/container-definitions/src/loader.ts +++ b/packages/common/container-definitions/src/loader.ts @@ -726,10 +726,6 @@ export interface IContainerLoadMode { * @internal */ export interface ILoaderHeader { - /** - * @deprecated This header has been deprecated and will be removed in a future release - */ - [LoaderHeader.cache]: boolean; [LoaderHeader.clientDetails]: IClientDetails; [LoaderHeader.loadMode]: IContainerLoadMode; [LoaderHeader.reconnect]: boolean; From 7006e3a72809003f12ab9444721a4bf5880a2e5f Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 29 Jan 2025 22:37:03 +0000 Subject: [PATCH 27/54] wip --- .../routerlicious-driver/src/documentDeltaConnection.ts | 4 +++- server/routerlicious/packages/lambdas/src/nexus/index.ts | 6 ++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index 4c46769844ac..02aaf6676f78 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -117,7 +117,9 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { const isCorruption = err.errorType === FluidErrorTypes.dataCorruptionError || err.errorType === FluidErrorTypes.dataProcessingError; - this.socket.emit("disconnect_document", this.clientId, this.documentId, isCorruption); + if (isCorruption) { + this.socket.emit("disconnect_document", this.clientId, this.documentId, err.errorType); + } super.disconnectCore(err); } } diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index 9ebeb1958bbf..74e31c5716e1 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -640,10 +640,8 @@ export function configureWebSocketServices( socket.on( "disconnect_document", - (clientId: string, documentId: string, isCorruption: boolean) => { - Lumberjack.error( - { clientId, documentId, isCorruption }, - ); + (clientId: string, documentId: string, errorType: string) => { + Lumberjack.error({ clientId, documentId, errorType }); }, ); }); From 403e769932e5d3c8e56dbc321ed7b647f8ea4e5a Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 29 Jan 2025 22:46:40 +0000 Subject: [PATCH 28/54] wip --- .../src/documentDeltaConnection.ts | 10 +++++++--- .../routerlicious/packages/lambdas/src/nexus/index.ts | 4 ++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index 02aaf6676f78..bc5725323957 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -117,9 +117,13 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { const isCorruption = err.errorType === FluidErrorTypes.dataCorruptionError || err.errorType === FluidErrorTypes.dataProcessingError; - if (isCorruption) { - this.socket.emit("disconnect_document", this.clientId, this.documentId, err.errorType); - } + this.socket.emit( + "disconnect_document", + this.clientId, + this.documentId, + err.errorType, + isCorruption, + ); super.disconnectCore(err); } } diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index 74e31c5716e1..98b356bb3290 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -640,8 +640,8 @@ export function configureWebSocketServices( socket.on( "disconnect_document", - (clientId: string, documentId: string, errorType: string) => { - Lumberjack.error({ clientId, documentId, errorType }); + (clientId: string, documentId: string, errorType: string, isCorruption: boolean) => { + Lumberjack.error({ clientId, documentId, errorType, isCorruption }); }, ); }); From bf681b11bcfaed781f1a12a771ea333b1b3c583e Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Wed, 29 Jan 2025 23:18:43 +0000 Subject: [PATCH 29/54] to string --- server/routerlicious/packages/lambdas/src/nexus/index.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index 98b356bb3290..84f76e06ceb7 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -641,7 +641,11 @@ export function configureWebSocketServices( socket.on( "disconnect_document", (clientId: string, documentId: string, errorType: string, isCorruption: boolean) => { - Lumberjack.error({ clientId, documentId, errorType, isCorruption }); + Lumberjack.error( + `Error for client ${clientId}, ${ + isCorruption ? "Corrupted" : "Not corrupted" + } document ${documentId}: ${errorType}`, + ); }, ); }); From a38ae82e4a7562ccb877c4790513e32f0d4c2acd Mon Sep 17 00:00:00 2001 From: Rishhi Date: Fri, 7 Feb 2025 14:41:58 -0500 Subject: [PATCH 30/54] attempt at tests --- .../src/test/r11sSocketTests.spec.ts | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index e7c37f2a4579..84464d88ad0e 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -5,6 +5,7 @@ import { strict as assert } from "assert"; +import { FluidErrorTypes } from "@fluidframework/core-interfaces/internal"; import { IClient } from "@fluidframework/driver-definitions"; import { DriverErrorTypes, @@ -194,4 +195,111 @@ describe("R11s Socket Tests", () => { "Error internal code should be R11sServiceClusterDrainingErrorCode", ); }); + + it("Socket error with Data Corruption error", async () => { + const errorToThrow = { + code: 500, + message: "Data corruption detected", + retryAfterMs: 0, + internalErrorCode: "DataCorruption", + errorType: FluidErrorTypes.dataCorruptionError, + canRetry: false, + }; + const errorEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName }, + }); + + const connection = await mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ); + let error: IAnyDriverError | undefined; + connection.on("disconnect", (reason: IAnyDriverError) => { + error = reason; + }); + + // Send Data Corruption error + socket.sendErrorEvent(errorToThrow); + + assert( + error?.errorType === FluidErrorTypes.dataCorruptionError, + "Error type should be dataCorruptionError", + ); + assert(error.scenarioName === "error", "Error scenario name should be error"); + assert( + (error as any).internalErrorCode === "DataCorruption", + "Error internal code should be DataCorruption", + ); + }); + + it("Socket error with Data Processing error", async () => { + const errorToThrow = { + code: 500, + message: "Data processing error", + retryAfterMs: 0, + internalErrorCode: "DataProcessing", + errorType: FluidErrorTypes.dataProcessingError, + canRetry: false, + }; + const errorEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName }, + }); + + const connection = await mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ); + let error: IAnyDriverError | undefined; + connection.on("disconnect", (reason: IAnyDriverError) => { + error = reason; + }); + + // Send Data Processing error + socket.sendErrorEvent(errorToThrow); + + assert( + error?.errorType === "dataProcessingError", + "Error type should be dataProcessingError", + ); + assert(error.scenarioName === "error", "Error scenario name should be error"); + assert( + (error as any).internalErrorCode === "DataProcessing", + "Error internal code should be DataProcessing", + ); + }); + + it("Verifies disconnect_document event is emitted with corruption flag for data corruption", async () => { + const errorToThrow = { + code: 500, + message: "Data corruption detected", + retryAfterMs: 0, + internalErrorCode: "DataCorruption", + errorType: "dataCorruptionError", + canRetry: false, + }; + const errorEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName }, + }); + + await mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ); + + let disconnectDocumentCalled = false; + let isCorruptionFlag = false; + socket.on( + "disconnect_document", + (clientId: string, docId: string, errorType: string, isCorruption: boolean) => { + disconnectDocumentCalled = true; + isCorruptionFlag = isCorruption; + }, + ); + + // Send Data Corruption error + socket.sendErrorEvent(errorToThrow); + + assert(disconnectDocumentCalled, "disconnect_document event should be emitted"); + assert(isCorruptionFlag, "isCorruption flag should be true for data corruption error"); + }); }); From d2d82c8fb0a34fbfc87ef83a611d75146107ac3e Mon Sep 17 00:00:00 2001 From: Rishhi Date: Fri, 7 Feb 2025 15:31:17 -0500 Subject: [PATCH 31/54] wip --- .../src/test/r11sSocketTests.spec.ts | 143 +++++++++--------- 1 file changed, 70 insertions(+), 73 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 84464d88ad0e..d67d1623b9ef 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -197,109 +197,106 @@ describe("R11s Socket Tests", () => { }); it("Socket error with Data Corruption error", async () => { - const errorToThrow = { - code: 500, - message: "Data corruption detected", - retryAfterMs: 0, - internalErrorCode: "DataCorruption", - errorType: FluidErrorTypes.dataCorruptionError, - canRetry: false, + const clientError = new Error("Data corruption detected") as Error & { + code: number; + retryAfterMs: number; + internalErrorCode: string; + errorType: string; + canRetry: boolean; }; - const errorEventName = "connect_document_success"; + clientError.code = 500; + clientError.retryAfterMs = 0; + clientError.internalErrorCode = "DataCorruption"; + clientError.errorType = FluidErrorTypes.dataCorruptionError; + clientError.canRetry = false; + + const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, + connect_document: { eventToEmit: socketEventName }, }); const connection = await mockSocket(socket as unknown as Socket, async () => documentService.connectToDeltaStream(client), ); - let error: IAnyDriverError | undefined; - connection.on("disconnect", (reason: IAnyDriverError) => { - error = reason; - }); - - // Send Data Corruption error - socket.sendErrorEvent(errorToThrow); - assert( - error?.errorType === FluidErrorTypes.dataCorruptionError, - "Error type should be dataCorruptionError", - ); - assert(error.scenarioName === "error", "Error scenario name should be error"); - assert( - (error as any).internalErrorCode === "DataCorruption", - "Error internal code should be DataCorruption", - ); - }); - - it("Socket error with Data Processing error", async () => { - const errorToThrow = { - code: 500, - message: "Data processing error", - retryAfterMs: 0, - internalErrorCode: "DataProcessing", - errorType: FluidErrorTypes.dataProcessingError, - canRetry: false, - }; - const errorEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, - }); + // Track disconnect_document events from client + let disconnectDocumentCalled = false; + let receivedClientId: string | undefined; + let receivedErrorType: string | undefined; + let receivedIsCorruption: boolean | undefined; - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), + socket.on( + "disconnect_document", + (clientId: string, _: string, errorType: string, isCorruption: boolean) => { + disconnectDocumentCalled = true; + receivedClientId = clientId; + receivedErrorType = errorType; + receivedIsCorruption = isCorruption; + }, ); - let error: IAnyDriverError | undefined; - connection.on("disconnect", (reason: IAnyDriverError) => { - error = reason; - }); - // Send Data Processing error - socket.sendErrorEvent(errorToThrow); + // Simulate client detecting corruption and disconnecting + connection.dispose(clientError); - assert( - error?.errorType === "dataProcessingError", - "Error type should be dataProcessingError", - ); - assert(error.scenarioName === "error", "Error scenario name should be error"); - assert( - (error as any).internalErrorCode === "DataProcessing", - "Error internal code should be DataProcessing", + // Verify the client sent the correct disconnect_document event + assert(disconnectDocumentCalled, "disconnect_document event should be emitted"); + assert.strictEqual(receivedClientId, connection.clientId, "Client ID should match"); + assert.strictEqual( + receivedErrorType, + FluidErrorTypes.dataCorruptionError, + "Error type should be dataCorruptionError" ); + assert(receivedIsCorruption, "isCorruption flag should be true"); }); - it("Verifies disconnect_document event is emitted with corruption flag for data corruption", async () => { - const errorToThrow = { - code: 500, - message: "Data corruption detected", - retryAfterMs: 0, - internalErrorCode: "DataCorruption", - errorType: "dataCorruptionError", - canRetry: false, + it("Socket error with Data Processing error", async () => { + const clientError = new Error("Data processing error") as Error & { + code: number; + retryAfterMs: number; + internalErrorCode: string; + errorType: string; + canRetry: boolean; }; - const errorEventName = "connect_document_success"; + clientError.code = 500; + clientError.retryAfterMs = 0; + clientError.internalErrorCode = "DataProcessing"; + clientError.errorType = FluidErrorTypes.dataProcessingError; + clientError.canRetry = false; + + const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, + connect_document: { eventToEmit: socketEventName }, }); - await mockSocket(socket as unknown as Socket, async () => + const connection = await mockSocket(socket as unknown as Socket, async () => documentService.connectToDeltaStream(client), ); + // Track disconnect_document events from client let disconnectDocumentCalled = false; - let isCorruptionFlag = false; + let receivedClientId: string | undefined; + let receivedErrorType: string | undefined; + let receivedIsCorruption: boolean | undefined; + socket.on( "disconnect_document", - (clientId: string, docId: string, errorType: string, isCorruption: boolean) => { + (clientId: string, _: string, errorType: string, isCorruption: boolean) => { disconnectDocumentCalled = true; - isCorruptionFlag = isCorruption; + receivedClientId = clientId; + receivedErrorType = errorType; + receivedIsCorruption = isCorruption; }, ); - // Send Data Corruption error - socket.sendErrorEvent(errorToThrow); + connection.dispose(clientError); assert(disconnectDocumentCalled, "disconnect_document event should be emitted"); - assert(isCorruptionFlag, "isCorruption flag should be true for data corruption error"); + assert.strictEqual(receivedClientId, connection.clientId, "Client ID should match"); + assert.strictEqual( + receivedErrorType, + FluidErrorTypes.dataProcessingError, + "Error type should be dataProcessingError" + ); + assert(receivedIsCorruption, "isCorruption flag should be true for data processing error"); }); }); From 59df842564ef0638244f6ddef58014fe50c40a4a Mon Sep 17 00:00:00 2001 From: Rishhi Date: Fri, 7 Feb 2025 15:59:17 -0500 Subject: [PATCH 32/54] use correct error objects --- .../src/test/r11sSocketTests.spec.ts | 30 ++++--------------- 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index d67d1623b9ef..2f52e5705e12 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -12,6 +12,7 @@ import { IResolvedUrl, type IAnyDriverError, } from "@fluidframework/driver-definitions/internal"; +import { DataCorruptionError, DataProcessingError } from "@fluidframework/telemetry-utils/internal"; import { stub } from "sinon"; import { Socket } from "socket.io-client"; @@ -197,18 +198,7 @@ describe("R11s Socket Tests", () => { }); it("Socket error with Data Corruption error", async () => { - const clientError = new Error("Data corruption detected") as Error & { - code: number; - retryAfterMs: number; - internalErrorCode: string; - errorType: string; - canRetry: boolean; - }; - clientError.code = 500; - clientError.retryAfterMs = 0; - clientError.internalErrorCode = "DataCorruption"; - clientError.errorType = FluidErrorTypes.dataCorruptionError; - clientError.canRetry = false; + const clientError = new DataCorruptionError("Data corruption error", {}); const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ @@ -250,18 +240,10 @@ describe("R11s Socket Tests", () => { }); it("Socket error with Data Processing error", async () => { - const clientError = new Error("Data processing error") as Error & { - code: number; - retryAfterMs: number; - internalErrorCode: string; - errorType: string; - canRetry: boolean; - }; - clientError.code = 500; - clientError.retryAfterMs = 0; - clientError.internalErrorCode = "DataProcessing"; - clientError.errorType = FluidErrorTypes.dataProcessingError; - clientError.canRetry = false; + const clientError = DataProcessingError.create( + "DataProcessingError", + "test" + );; const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ From ebd26f0888cdee8d44ab4940ae7852d37170bb7a Mon Sep 17 00:00:00 2001 From: Rishhi Date: Mon, 10 Feb 2025 11:41:57 -0500 Subject: [PATCH 33/54] change to promise --- .../src/test/r11sSocketTests.spec.ts | 72 +++++++++---------- 1 file changed, 32 insertions(+), 40 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 2f52e5705e12..12df3dbfa665 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -209,41 +209,36 @@ describe("R11s Socket Tests", () => { documentService.connectToDeltaStream(client), ); - // Track disconnect_document events from client - let disconnectDocumentCalled = false; - let receivedClientId: string | undefined; - let receivedErrorType: string | undefined; - let receivedIsCorruption: boolean | undefined; - - socket.on( - "disconnect_document", - (clientId: string, _: string, errorType: string, isCorruption: boolean) => { - disconnectDocumentCalled = true; - receivedClientId = clientId; - receivedErrorType = errorType; - receivedIsCorruption = isCorruption; - }, - ); + // Set up Promise to await disconnect_document event + const disconnectEventP = new Promise<{clientId: string, errorType: string, isCorruption: boolean}>((resolve) => { + assert(socket !== undefined, "Socket should be defined"); + socket.on( + "disconnect_document", + (clientId: string, _: string, errorType: string, isCorruption: boolean) => { + resolve({clientId, errorType, isCorruption}); + }, + ); + }); // Simulate client detecting corruption and disconnecting connection.dispose(clientError); - // Verify the client sent the correct disconnect_document event - assert(disconnectDocumentCalled, "disconnect_document event should be emitted"); - assert.strictEqual(receivedClientId, connection.clientId, "Client ID should match"); + // Wait for and verify the disconnect_document event + const disconnectResult = await disconnectEventP; + assert.strictEqual(disconnectResult.clientId, connection.clientId, "Client ID should match"); assert.strictEqual( - receivedErrorType, + disconnectResult.errorType, FluidErrorTypes.dataCorruptionError, "Error type should be dataCorruptionError" ); - assert(receivedIsCorruption, "isCorruption flag should be true"); + assert(disconnectResult.isCorruption, "isCorruption flag should be true"); }); it("Socket error with Data Processing error", async () => { const clientError = DataProcessingError.create( "DataProcessingError", "test" - );; + ); const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ @@ -254,31 +249,28 @@ describe("R11s Socket Tests", () => { documentService.connectToDeltaStream(client), ); - // Track disconnect_document events from client - let disconnectDocumentCalled = false; - let receivedClientId: string | undefined; - let receivedErrorType: string | undefined; - let receivedIsCorruption: boolean | undefined; - - socket.on( - "disconnect_document", - (clientId: string, _: string, errorType: string, isCorruption: boolean) => { - disconnectDocumentCalled = true; - receivedClientId = clientId; - receivedErrorType = errorType; - receivedIsCorruption = isCorruption; - }, - ); + // Set up Promise to await disconnect_document event + const disconnectEventP = new Promise<{clientId: string, errorType: string, isCorruption: boolean}>((resolve) => { + assert(socket !== undefined, "Socket should be defined"); + socket.on( + "disconnect_document", + (clientId: string, _: string, errorType: string, isCorruption: boolean) => { + resolve({clientId, errorType, isCorruption}); + }, + ); + }); + // Simulate client detecting processing error and disconnecting connection.dispose(clientError); - assert(disconnectDocumentCalled, "disconnect_document event should be emitted"); - assert.strictEqual(receivedClientId, connection.clientId, "Client ID should match"); + // Wait for and verify the disconnect_document event + const disconnectResult = await disconnectEventP; + assert.strictEqual(disconnectResult.clientId, connection.clientId, "Client ID should match"); assert.strictEqual( - receivedErrorType, + disconnectResult.errorType, FluidErrorTypes.dataProcessingError, "Error type should be dataProcessingError" ); - assert(receivedIsCorruption, "isCorruption flag should be true for data processing error"); + assert(disconnectResult.isCorruption, "isCorruption flag should be true for data processing error"); }); }); From 175d8cab4f5f100d67c07f1c75f3c598a4abc867 Mon Sep 17 00:00:00 2001 From: Rishhi Date: Mon, 10 Feb 2025 13:19:09 -0500 Subject: [PATCH 34/54] format --- .../src/test/r11sSocketTests.spec.ts | 47 +++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 12df3dbfa665..499693cbde46 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -12,7 +12,10 @@ import { IResolvedUrl, type IAnyDriverError, } from "@fluidframework/driver-definitions/internal"; -import { DataCorruptionError, DataProcessingError } from "@fluidframework/telemetry-utils/internal"; +import { + DataCorruptionError, + DataProcessingError, +} from "@fluidframework/telemetry-utils/internal"; import { stub } from "sinon"; import { Socket } from "socket.io-client"; @@ -210,12 +213,16 @@ describe("R11s Socket Tests", () => { ); // Set up Promise to await disconnect_document event - const disconnectEventP = new Promise<{clientId: string, errorType: string, isCorruption: boolean}>((resolve) => { + const disconnectEventP = new Promise<{ + clientId: string; + errorType: string; + isCorruption: boolean; + }>((resolve) => { assert(socket !== undefined, "Socket should be defined"); socket.on( "disconnect_document", (clientId: string, _: string, errorType: string, isCorruption: boolean) => { - resolve({clientId, errorType, isCorruption}); + resolve({ clientId, errorType, isCorruption }); }, ); }); @@ -225,20 +232,21 @@ describe("R11s Socket Tests", () => { // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; - assert.strictEqual(disconnectResult.clientId, connection.clientId, "Client ID should match"); + assert.strictEqual( + disconnectResult.clientId, + connection.clientId, + "Client ID should match", + ); assert.strictEqual( disconnectResult.errorType, FluidErrorTypes.dataCorruptionError, - "Error type should be dataCorruptionError" + "Error type should be dataCorruptionError", ); assert(disconnectResult.isCorruption, "isCorruption flag should be true"); }); it("Socket error with Data Processing error", async () => { - const clientError = DataProcessingError.create( - "DataProcessingError", - "test" - ); + const clientError = DataProcessingError.create("DataProcessingError", "test"); const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ @@ -250,12 +258,16 @@ describe("R11s Socket Tests", () => { ); // Set up Promise to await disconnect_document event - const disconnectEventP = new Promise<{clientId: string, errorType: string, isCorruption: boolean}>((resolve) => { + const disconnectEventP = new Promise<{ + clientId: string; + errorType: string; + isCorruption: boolean; + }>((resolve) => { assert(socket !== undefined, "Socket should be defined"); socket.on( "disconnect_document", (clientId: string, _: string, errorType: string, isCorruption: boolean) => { - resolve({clientId, errorType, isCorruption}); + resolve({ clientId, errorType, isCorruption }); }, ); }); @@ -265,12 +277,19 @@ describe("R11s Socket Tests", () => { // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; - assert.strictEqual(disconnectResult.clientId, connection.clientId, "Client ID should match"); + assert.strictEqual( + disconnectResult.clientId, + connection.clientId, + "Client ID should match", + ); assert.strictEqual( disconnectResult.errorType, FluidErrorTypes.dataProcessingError, - "Error type should be dataProcessingError" + "Error type should be dataProcessingError", + ); + assert( + disconnectResult.isCorruption, + "isCorruption flag should be true for data processing error", ); - assert(disconnectResult.isCorruption, "isCorruption flag should be true for data processing error"); }); }); From 007829ea2e7a13e2752b56b6d329c3a439767996 Mon Sep 17 00:00:00 2001 From: Rishhi Date: Mon, 10 Feb 2025 13:20:01 -0500 Subject: [PATCH 35/54] move comment --- .../drivers/routerlicious-driver/src/documentDeltaConnection.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index bc5725323957..aca7482fda64 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -113,10 +113,10 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { * Disconnect from the websocket */ protected disconnectCore(err: IAnyDriverError): void { - // tell the server we are disconnecting this client from the document const isCorruption = err.errorType === FluidErrorTypes.dataCorruptionError || err.errorType === FluidErrorTypes.dataProcessingError; + // tell the server we are disconnecting this client from the document this.socket.emit( "disconnect_document", this.clientId, From 51007f5cfbcd02247819205d2e768c3370abd723 Mon Sep 17 00:00:00 2001 From: Rishhi Date: Mon, 10 Feb 2025 15:21:49 -0500 Subject: [PATCH 36/54] directly disconnect with Corrution error instead of flowing it from dispose --- .../src/documentDeltaConnection.ts | 4 +- .../src/test/r11sSocketTests.spec.ts | 37 +++++++++++++------ 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 522b96c64b91..8010b57e0c61 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -401,7 +401,7 @@ export class DocumentDeltaConnection * However the OdspDocumentDeltaConnection differ in dispose as in there we don't close the socket. There is no * multiplexing here, so we need to close the socket here. */ - public dispose() { + public dispose(error?: Error) { this.logger.sendTelemetryEvent({ eventName: "ClientClosingDeltaConnection", driverVersion, @@ -412,7 +412,7 @@ export class DocumentDeltaConnection this.disconnect( createGenericNetworkError( // pre-0.58 error message: clientClosingConnection - "Client closing delta connection", + error?.message ?? "Client closing delta connection", { canRetry: true }, { driverVersion }, ), diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 499693cbde46..326a4498a312 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -10,7 +10,7 @@ import { IClient } from "@fluidframework/driver-definitions"; import { DriverErrorTypes, IResolvedUrl, - type IAnyDriverError, + IAnyDriverError, } from "@fluidframework/driver-definitions/internal"; import { DataCorruptionError, @@ -201,7 +201,9 @@ describe("R11s Socket Tests", () => { }); it("Socket error with Data Corruption error", async () => { - const clientError = new DataCorruptionError("Data corruption error", {}); + const clientError = new DataCorruptionError("Data corruption error", { + driverVersion: "1.0", + }); const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ @@ -221,14 +223,18 @@ describe("R11s Socket Tests", () => { assert(socket !== undefined, "Socket should be defined"); socket.on( "disconnect_document", - (clientId: string, _: string, errorType: string, isCorruption: boolean) => { - resolve({ clientId, errorType, isCorruption }); + (clientId: string, documentId: string, errorType: string, isCorruption: boolean) => { + resolve({ + clientId, + errorType, + isCorruption, + }); }, ); }); - // Simulate client detecting corruption and disconnecting - connection.dispose(clientError); + // Call disconnect directly with the error + (connection as any).disconnect(clientError); // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; @@ -246,7 +252,12 @@ describe("R11s Socket Tests", () => { }); it("Socket error with Data Processing error", async () => { - const clientError = DataProcessingError.create("DataProcessingError", "test"); + const clientError = DataProcessingError.create( + "Data processing error", + "test", + undefined, + { driverVersion: "1.0" }, + ); const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ @@ -266,14 +277,18 @@ describe("R11s Socket Tests", () => { assert(socket !== undefined, "Socket should be defined"); socket.on( "disconnect_document", - (clientId: string, _: string, errorType: string, isCorruption: boolean) => { - resolve({ clientId, errorType, isCorruption }); + (clientId: string, documentId: string, errorType: string, isCorruption: boolean) => { + resolve({ + clientId, + errorType, + isCorruption, + }); }, ); }); - // Simulate client detecting processing error and disconnecting - connection.dispose(clientError); + // Call disconnect directly with the error + (connection as any).disconnect(clientError); // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; From 8a33aa0ce69f5146a816b006ae65e181ab6e9c64 Mon Sep 17 00:00:00 2001 From: Rishhi Date: Mon, 10 Feb 2025 15:50:12 -0500 Subject: [PATCH 37/54] remove error on dispose --- packages/drivers/driver-base/src/documentDeltaConnection.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 8010b57e0c61..522b96c64b91 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -401,7 +401,7 @@ export class DocumentDeltaConnection * However the OdspDocumentDeltaConnection differ in dispose as in there we don't close the socket. There is no * multiplexing here, so we need to close the socket here. */ - public dispose(error?: Error) { + public dispose() { this.logger.sendTelemetryEvent({ eventName: "ClientClosingDeltaConnection", driverVersion, @@ -412,7 +412,7 @@ export class DocumentDeltaConnection this.disconnect( createGenericNetworkError( // pre-0.58 error message: clientClosingConnection - error?.message ?? "Client closing delta connection", + "Client closing delta connection", { canRetry: true }, { driverVersion }, ), From 3e8df2e4266834b751d76e61d6dffabada305831 Mon Sep 17 00:00:00 2001 From: Rishhi Date: Thu, 13 Feb 2025 16:21:14 -0500 Subject: [PATCH 38/54] use helpers to generate tests --- .../src/test/r11sSocketTests.spec.ts | 228 +++++++----------- 1 file changed, 81 insertions(+), 147 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 326a4498a312..8139dc328fb7 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -10,7 +10,7 @@ import { IClient } from "@fluidframework/driver-definitions"; import { DriverErrorTypes, IResolvedUrl, - IAnyDriverError, + type IAnyDriverError, } from "@fluidframework/driver-definitions/internal"; import { DataCorruptionError, @@ -74,81 +74,59 @@ describe("R11s Socket Tests", () => { )) as DocumentService; }); - it("connect_document_error with Token Revoked error", async () => { - const errorToThrow = { - code: 403, - message: "TokenRevokedError", - retryAfterMs: 10, - internalErrorCode: "TokenRevoked", - errorType: DriverErrorTypes.authorizationError, - canRetry: false, + interface ErrorTestCase { + name: string; + error: { + code: number; + message: string; + retryAfterMs: number; + internalErrorCode: string; + errorType: string; + canRetry: boolean; }; - const errorEventName = "connect_document_error"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName, errorToThrow }, - }); + expectedError: { + errorType: string; + internalErrorCode: string; + }; + } - await assert.rejects( - mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ), - { + const errorTestCases: ErrorTestCase[] = [ + { + name: "Token Revoked error", + error: { + code: 403, + message: "TokenRevokedError", + retryAfterMs: 10, + internalErrorCode: "TokenRevoked", + errorType: DriverErrorTypes.authorizationError, + canRetry: false, + }, + expectedError: { errorType: DriverErrorTypes.authorizationError, - scenarioName: "connect_document_error", internalErrorCode: "TokenRevoked", }, - "Error should have occurred", - ); - }); - - it("Socket error with Token Revoked error", async () => { - const errorToThrow = { - code: 403, - message: "TokenRevokedError", - retryAfterMs: 10, - internalErrorCode: "TokenRevoked", - errorType: DriverErrorTypes.authorizationError, - canRetry: false, - }; - const errorEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, - }); - - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); - let error: IAnyDriverError | undefined; - connection.on("disconnect", (reason: IAnyDriverError) => { - error = reason; - }); - - // Send Token Revoked error - socket.sendErrorEvent(errorToThrow); - - assert( - error?.errorType === DriverErrorTypes.authorizationError, - "Error type should be authorizationError", - ); - assert(error.scenarioName === "error", "Error scenario name should be error"); - assert( - (error as any).internalErrorCode === "TokenRevoked", - "Error internal code should be TokenRevoked", - ); - }); + }, + { + name: "Cluster Draining error", + error: { + code: 503, + message: "ClusterDrainingError", + retryAfterMs: 1000, + internalErrorCode: R11sServiceClusterDrainingErrorCode, + errorType: RouterliciousErrorTypes.clusterDrainingError, + canRetry: true, + }, + expectedError: { + errorType: RouterliciousErrorTypes.clusterDrainingError, + internalErrorCode: R11sServiceClusterDrainingErrorCode, + }, + }, + ]; - it("connect_document_error with Cluster Draining error", async () => { - const errorToThrow = { - code: 503, - message: "ClusterDrainingError", - retryAfterMs: 1000, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - errorType: RouterliciousErrorTypes.clusterDrainingError, - canRetry: true, - }; + async function testConnectDocumentError(testCase: ErrorTestCase) { const errorEventName = "connect_document_error"; socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName, errorToThrow }, + connect_document: { eventToEmit: errorEventName, errorToThrow: testCase.error }, }); await assert.rejects( @@ -156,23 +134,15 @@ describe("R11s Socket Tests", () => { documentService.connectToDeltaStream(client), ), { - errorType: RouterliciousErrorTypes.clusterDrainingError, + errorType: testCase.expectedError.errorType, scenarioName: "connect_document_error", - internalErrorCode: R11sServiceClusterDrainingErrorCode, + internalErrorCode: testCase.expectedError.internalErrorCode, }, "Error should have occurred", ); - }); + } - it("Socket error with Cluster Draining error", async () => { - const errorToThrow = { - code: 503, - message: "ClusterDrainingError", - retryAfterMs: 1000, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - errorType: RouterliciousErrorTypes.clusterDrainingError, - canRetry: true, - }; + async function testSocketError(testCase: ErrorTestCase) { const errorEventName = "connect_document_success"; socket = new ClientSocketMock({ connect_document: { eventToEmit: errorEventName }, @@ -186,78 +156,37 @@ describe("R11s Socket Tests", () => { error = reason; }); - // Send Token Revoked error - socket.sendErrorEvent(errorToThrow); + socket.sendErrorEvent(testCase.error); assert( - error?.errorType === RouterliciousErrorTypes.clusterDrainingError, - "Error type should be clusterDrainingError", + error?.errorType === testCase.expectedError.errorType, + `Error type should be ${testCase.expectedError.errorType}`, ); assert(error.scenarioName === "error", "Error scenario name should be error"); assert( - (error as any).internalErrorCode === R11sServiceClusterDrainingErrorCode, - "Error internal code should be R11sServiceClusterDrainingErrorCode", + (error as any).internalErrorCode === testCase.expectedError.internalErrorCode, + `Error internal code should be ${testCase.expectedError.internalErrorCode}`, ); - }); + } - it("Socket error with Data Corruption error", async () => { - const clientError = new DataCorruptionError("Data corruption error", { - driverVersion: "1.0", + // Generate tests for each error case + for (const testCase of errorTestCases) { + it(`connect_document_error with ${testCase.name}`, async () => { + await testConnectDocumentError(testCase); }); - const socketEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: socketEventName }, + it(`Socket error with ${testCase.name}`, async () => { + await testSocketError(testCase); }); + } - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); - - // Set up Promise to await disconnect_document event - const disconnectEventP = new Promise<{ - clientId: string; - errorType: string; - isCorruption: boolean; - }>((resolve) => { - assert(socket !== undefined, "Socket should be defined"); - socket.on( - "disconnect_document", - (clientId: string, documentId: string, errorType: string, isCorruption: boolean) => { - resolve({ - clientId, - errorType, - isCorruption, - }); - }, - ); - }); - - // Call disconnect directly with the error - (connection as any).disconnect(clientError); - - // Wait for and verify the disconnect_document event - const disconnectResult = await disconnectEventP; - assert.strictEqual( - disconnectResult.clientId, - connection.clientId, - "Client ID should match", - ); - assert.strictEqual( - disconnectResult.errorType, - FluidErrorTypes.dataCorruptionError, - "Error type should be dataCorruptionError", - ); - assert(disconnectResult.isCorruption, "isCorruption flag should be true"); - }); - - it("Socket error with Data Processing error", async () => { - const clientError = DataProcessingError.create( - "Data processing error", - "test", - undefined, - { driverVersion: "1.0" }, - ); + async function testDataError( + errorConstructor: typeof DataCorruptionError | typeof DataProcessingError, + expectedErrorType: string, + ) { + const clientError = errorConstructor === DataCorruptionError + ? new DataCorruptionError("Data corruption error", { driverVersion: "1.0" }) + : DataProcessingError.create("Data processing error", "test", undefined, { driverVersion: "1.0" }); const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ @@ -268,7 +197,6 @@ describe("R11s Socket Tests", () => { documentService.connectToDeltaStream(client), ); - // Set up Promise to await disconnect_document event const disconnectEventP = new Promise<{ clientId: string; errorType: string; @@ -287,10 +215,8 @@ describe("R11s Socket Tests", () => { ); }); - // Call disconnect directly with the error (connection as any).disconnect(clientError); - // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; assert.strictEqual( disconnectResult.clientId, @@ -299,12 +225,20 @@ describe("R11s Socket Tests", () => { ); assert.strictEqual( disconnectResult.errorType, - FluidErrorTypes.dataProcessingError, - "Error type should be dataProcessingError", + expectedErrorType, + `Error type should be ${expectedErrorType}`, ); assert( disconnectResult.isCorruption, - "isCorruption flag should be true for data processing error", + "isCorruption flag should be true", ); + } + + it("Socket error with Data Corruption error", async () => { + await testDataError(DataCorruptionError, FluidErrorTypes.dataCorruptionError); + }); + + it("Socket error with Data Processing error", async () => { + await testDataError(DataProcessingError, FluidErrorTypes.dataProcessingError); }); }); From e756050eaa394085cd1994dbe6a15d69ffde0842 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Thu, 13 Feb 2025 21:49:05 +0000 Subject: [PATCH 39/54] format --- .../src/test/r11sSocketTests.spec.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 8139dc328fb7..f02cccb72d83 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -184,9 +184,12 @@ describe("R11s Socket Tests", () => { errorConstructor: typeof DataCorruptionError | typeof DataProcessingError, expectedErrorType: string, ) { - const clientError = errorConstructor === DataCorruptionError - ? new DataCorruptionError("Data corruption error", { driverVersion: "1.0" }) - : DataProcessingError.create("Data processing error", "test", undefined, { driverVersion: "1.0" }); + const clientError = + errorConstructor === DataCorruptionError + ? new DataCorruptionError("Data corruption error", { driverVersion: "1.0" }) + : DataProcessingError.create("Data processing error", "test", undefined, { + driverVersion: "1.0", + }); const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ @@ -228,10 +231,7 @@ describe("R11s Socket Tests", () => { expectedErrorType, `Error type should be ${expectedErrorType}`, ); - assert( - disconnectResult.isCorruption, - "isCorruption flag should be true", - ); + assert(disconnectResult.isCorruption, "isCorruption flag should be true"); } it("Socket error with Data Corruption error", async () => { From c49ad139338324d97f8fd5bd08b152d3a6240bd4 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Tue, 18 Feb 2025 18:53:40 +0000 Subject: [PATCH 40/54] Revert "use helpers to generate tests" This reverts commit 3e8df2e4266834b751d76e61d6dffabada305831. --- .../src/documentDeltaConnection.ts | 13 +- .../src/documentDeltaConnection.ts | 10 +- .../src/test/r11sSocketTests.spec.ts | 232 +++++++++++------- 3 files changed, 158 insertions(+), 97 deletions(-) diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 522b96c64b91..823a4ed40aec 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -409,17 +409,10 @@ export class DocumentDeltaConnection ...this.getConnectionDetailsProps(), }), }); - this.disconnect( - createGenericNetworkError( - // pre-0.58 error message: clientClosingConnection - "Client closing delta connection", - { canRetry: true }, - { driverVersion }, - ), - ); + this.disconnect(); } - protected disconnect(err: IAnyDriverError) { + protected disconnect(err?: IAnyDriverError) { // Can't check this.disposed here, as we get here on socket closure, // so _disposed & socket.connected might be not in sync while processing // "dispose" event. @@ -455,7 +448,7 @@ export class DocumentDeltaConnection * Disconnect from the websocket. * @param reason - reason for disconnect */ - protected disconnectCore(err: IAnyDriverError) { + protected disconnectCore(err?: IAnyDriverError) { this.socket.disconnect(); } diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index aca7482fda64..04db4f7841a8 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -112,16 +112,18 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { /** * Disconnect from the websocket */ - protected disconnectCore(err: IAnyDriverError): void { + protected disconnectCore(err?: IAnyDriverError): void { + const errorMsg = err?.errorType ?? "Clean disconnect"; const isCorruption = - err.errorType === FluidErrorTypes.dataCorruptionError || - err.errorType === FluidErrorTypes.dataProcessingError; + err !== undefined && + (err.errorType === FluidErrorTypes.dataCorruptionError || + err.errorType === FluidErrorTypes.dataProcessingError); // tell the server we are disconnecting this client from the document this.socket.emit( "disconnect_document", this.clientId, this.documentId, - err.errorType, + errorMsg, isCorruption, ); super.disconnectCore(err); diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index f02cccb72d83..326a4498a312 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -10,7 +10,7 @@ import { IClient } from "@fluidframework/driver-definitions"; import { DriverErrorTypes, IResolvedUrl, - type IAnyDriverError, + IAnyDriverError, } from "@fluidframework/driver-definitions/internal"; import { DataCorruptionError, @@ -74,59 +74,81 @@ describe("R11s Socket Tests", () => { )) as DocumentService; }); - interface ErrorTestCase { - name: string; - error: { - code: number; - message: string; - retryAfterMs: number; - internalErrorCode: string; - errorType: string; - canRetry: boolean; - }; - expectedError: { - errorType: string; - internalErrorCode: string; + it("connect_document_error with Token Revoked error", async () => { + const errorToThrow = { + code: 403, + message: "TokenRevokedError", + retryAfterMs: 10, + internalErrorCode: "TokenRevoked", + errorType: DriverErrorTypes.authorizationError, + canRetry: false, }; - } + const errorEventName = "connect_document_error"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName, errorToThrow }, + }); - const errorTestCases: ErrorTestCase[] = [ - { - name: "Token Revoked error", - error: { - code: 403, - message: "TokenRevokedError", - retryAfterMs: 10, - internalErrorCode: "TokenRevoked", - errorType: DriverErrorTypes.authorizationError, - canRetry: false, - }, - expectedError: { + await assert.rejects( + mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ), + { errorType: DriverErrorTypes.authorizationError, + scenarioName: "connect_document_error", internalErrorCode: "TokenRevoked", }, - }, - { - name: "Cluster Draining error", - error: { - code: 503, - message: "ClusterDrainingError", - retryAfterMs: 1000, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - errorType: RouterliciousErrorTypes.clusterDrainingError, - canRetry: true, - }, - expectedError: { - errorType: RouterliciousErrorTypes.clusterDrainingError, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - }, - }, - ]; + "Error should have occurred", + ); + }); - async function testConnectDocumentError(testCase: ErrorTestCase) { + it("Socket error with Token Revoked error", async () => { + const errorToThrow = { + code: 403, + message: "TokenRevokedError", + retryAfterMs: 10, + internalErrorCode: "TokenRevoked", + errorType: DriverErrorTypes.authorizationError, + canRetry: false, + }; + const errorEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName }, + }); + + const connection = await mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ); + let error: IAnyDriverError | undefined; + connection.on("disconnect", (reason: IAnyDriverError) => { + error = reason; + }); + + // Send Token Revoked error + socket.sendErrorEvent(errorToThrow); + + assert( + error?.errorType === DriverErrorTypes.authorizationError, + "Error type should be authorizationError", + ); + assert(error.scenarioName === "error", "Error scenario name should be error"); + assert( + (error as any).internalErrorCode === "TokenRevoked", + "Error internal code should be TokenRevoked", + ); + }); + + it("connect_document_error with Cluster Draining error", async () => { + const errorToThrow = { + code: 503, + message: "ClusterDrainingError", + retryAfterMs: 1000, + internalErrorCode: R11sServiceClusterDrainingErrorCode, + errorType: RouterliciousErrorTypes.clusterDrainingError, + canRetry: true, + }; const errorEventName = "connect_document_error"; socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName, errorToThrow: testCase.error }, + connect_document: { eventToEmit: errorEventName, errorToThrow }, }); await assert.rejects( @@ -134,15 +156,23 @@ describe("R11s Socket Tests", () => { documentService.connectToDeltaStream(client), ), { - errorType: testCase.expectedError.errorType, + errorType: RouterliciousErrorTypes.clusterDrainingError, scenarioName: "connect_document_error", - internalErrorCode: testCase.expectedError.internalErrorCode, + internalErrorCode: R11sServiceClusterDrainingErrorCode, }, "Error should have occurred", ); - } + }); - async function testSocketError(testCase: ErrorTestCase) { + it("Socket error with Cluster Draining error", async () => { + const errorToThrow = { + code: 503, + message: "ClusterDrainingError", + retryAfterMs: 1000, + internalErrorCode: R11sServiceClusterDrainingErrorCode, + errorType: RouterliciousErrorTypes.clusterDrainingError, + canRetry: true, + }; const errorEventName = "connect_document_success"; socket = new ClientSocketMock({ connect_document: { eventToEmit: errorEventName }, @@ -156,40 +186,24 @@ describe("R11s Socket Tests", () => { error = reason; }); - socket.sendErrorEvent(testCase.error); + // Send Token Revoked error + socket.sendErrorEvent(errorToThrow); assert( - error?.errorType === testCase.expectedError.errorType, - `Error type should be ${testCase.expectedError.errorType}`, + error?.errorType === RouterliciousErrorTypes.clusterDrainingError, + "Error type should be clusterDrainingError", ); assert(error.scenarioName === "error", "Error scenario name should be error"); assert( - (error as any).internalErrorCode === testCase.expectedError.internalErrorCode, - `Error internal code should be ${testCase.expectedError.internalErrorCode}`, + (error as any).internalErrorCode === R11sServiceClusterDrainingErrorCode, + "Error internal code should be R11sServiceClusterDrainingErrorCode", ); - } - - // Generate tests for each error case - for (const testCase of errorTestCases) { - it(`connect_document_error with ${testCase.name}`, async () => { - await testConnectDocumentError(testCase); - }); + }); - it(`Socket error with ${testCase.name}`, async () => { - await testSocketError(testCase); + it("Socket error with Data Corruption error", async () => { + const clientError = new DataCorruptionError("Data corruption error", { + driverVersion: "1.0", }); - } - - async function testDataError( - errorConstructor: typeof DataCorruptionError | typeof DataProcessingError, - expectedErrorType: string, - ) { - const clientError = - errorConstructor === DataCorruptionError - ? new DataCorruptionError("Data corruption error", { driverVersion: "1.0" }) - : DataProcessingError.create("Data processing error", "test", undefined, { - driverVersion: "1.0", - }); const socketEventName = "connect_document_success"; socket = new ClientSocketMock({ @@ -200,6 +214,7 @@ describe("R11s Socket Tests", () => { documentService.connectToDeltaStream(client), ); + // Set up Promise to await disconnect_document event const disconnectEventP = new Promise<{ clientId: string; errorType: string; @@ -218,8 +233,10 @@ describe("R11s Socket Tests", () => { ); }); + // Call disconnect directly with the error (connection as any).disconnect(clientError); + // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; assert.strictEqual( disconnectResult.clientId, @@ -228,17 +245,66 @@ describe("R11s Socket Tests", () => { ); assert.strictEqual( disconnectResult.errorType, - expectedErrorType, - `Error type should be ${expectedErrorType}`, + FluidErrorTypes.dataCorruptionError, + "Error type should be dataCorruptionError", ); assert(disconnectResult.isCorruption, "isCorruption flag should be true"); - } - - it("Socket error with Data Corruption error", async () => { - await testDataError(DataCorruptionError, FluidErrorTypes.dataCorruptionError); }); it("Socket error with Data Processing error", async () => { - await testDataError(DataProcessingError, FluidErrorTypes.dataProcessingError); + const clientError = DataProcessingError.create( + "Data processing error", + "test", + undefined, + { driverVersion: "1.0" }, + ); + + const socketEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: socketEventName }, + }); + + const connection = await mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ); + + // Set up Promise to await disconnect_document event + const disconnectEventP = new Promise<{ + clientId: string; + errorType: string; + isCorruption: boolean; + }>((resolve) => { + assert(socket !== undefined, "Socket should be defined"); + socket.on( + "disconnect_document", + (clientId: string, documentId: string, errorType: string, isCorruption: boolean) => { + resolve({ + clientId, + errorType, + isCorruption, + }); + }, + ); + }); + + // Call disconnect directly with the error + (connection as any).disconnect(clientError); + + // Wait for and verify the disconnect_document event + const disconnectResult = await disconnectEventP; + assert.strictEqual( + disconnectResult.clientId, + connection.clientId, + "Client ID should match", + ); + assert.strictEqual( + disconnectResult.errorType, + FluidErrorTypes.dataProcessingError, + "Error type should be dataProcessingError", + ); + assert( + disconnectResult.isCorruption, + "isCorruption flag should be true for data processing error", + ); }); }); From 69e6b181940c943168f50dd2b4c08f15f75dfa5e Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Thu, 20 Feb 2025 16:31:40 +0000 Subject: [PATCH 41/54] add error back in --- .../drivers/driver-base/src/documentDeltaConnection.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 823a4ed40aec..9fdfda0464a1 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -409,7 +409,14 @@ export class DocumentDeltaConnection ...this.getConnectionDetailsProps(), }), }); - this.disconnect(); + this.disconnect( + createGenericNetworkError( + // pre-0.58 error message: clientClosingConnection + "Client closing delta connection", + { canRetry: true }, + { driverVersion }, + ), + ); } protected disconnect(err?: IAnyDriverError) { From 87e872e1d08209bb663834e96cfad19664e3215d Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Mon, 24 Feb 2025 00:59:51 +0000 Subject: [PATCH 42/54] remove data processing error --- .../routerlicious-driver/src/documentDeltaConnection.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index 04db4f7841a8..9f6edff997f2 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -115,9 +115,7 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { protected disconnectCore(err?: IAnyDriverError): void { const errorMsg = err?.errorType ?? "Clean disconnect"; const isCorruption = - err !== undefined && - (err.errorType === FluidErrorTypes.dataCorruptionError || - err.errorType === FluidErrorTypes.dataProcessingError); + err !== undefined && err.errorType === FluidErrorTypes.dataCorruptionError; // tell the server we are disconnecting this client from the document this.socket.emit( "disconnect_document", From ff6b536b5533f02ba0caa9114c4288168b2d2bff Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Thu, 27 Feb 2025 00:14:02 +0000 Subject: [PATCH 43/54] design change --- .../src/documentDeltaConnection.ts | 12 +----------- .../packages/lambdas/src/nexus/index.ts | 6 ++---- 2 files changed, 3 insertions(+), 15 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index 9f6edff997f2..1b4f6016ed6d 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -3,7 +3,6 @@ * Licensed under the MIT License. */ -import { FluidErrorTypes } from "@fluidframework/core-interfaces/internal"; import { DocumentDeltaConnection } from "@fluidframework/driver-base/internal"; import { IClient } from "@fluidframework/driver-definitions"; import { @@ -113,17 +112,8 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { * Disconnect from the websocket */ protected disconnectCore(err?: IAnyDriverError): void { - const errorMsg = err?.errorType ?? "Clean disconnect"; - const isCorruption = - err !== undefined && err.errorType === FluidErrorTypes.dataCorruptionError; // tell the server we are disconnecting this client from the document - this.socket.emit( - "disconnect_document", - this.clientId, - this.documentId, - errorMsg, - isCorruption, - ); + this.socket.emit("disconnect_document", this.clientId, this.documentId, err?.errorType); super.disconnectCore(err); } } diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index 440df2daf243..fadeefa3e960 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -645,11 +645,9 @@ export function configureWebSocketServices( socket.on( "disconnect_document", - (clientId: string, documentId: string, errorType: string, isCorruption: boolean) => { + (clientId: string, documentId: string, errorType?: string) => { Lumberjack.error( - `Error for client ${clientId}, ${ - isCorruption ? "Corrupted" : "Not corrupted" - } document ${documentId}: ${errorType}`, + `Error for client ${clientId}, document ${documentId}: ${errorType}`, ); }, ); From 61b36ed7d99c137a7a39b7c73cbba67bc0749d2c Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Thu, 27 Feb 2025 16:46:24 +0000 Subject: [PATCH 44/54] fix tests --- .../src/test/r11sSocketTests.spec.ts | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 326a4498a312..1674cec8495a 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -218,16 +218,14 @@ describe("R11s Socket Tests", () => { const disconnectEventP = new Promise<{ clientId: string; errorType: string; - isCorruption: boolean; }>((resolve) => { assert(socket !== undefined, "Socket should be defined"); socket.on( "disconnect_document", - (clientId: string, documentId: string, errorType: string, isCorruption: boolean) => { + (clientId: string, documentId: string, errorType: string) => { resolve({ clientId, errorType, - isCorruption, }); }, ); @@ -248,7 +246,6 @@ describe("R11s Socket Tests", () => { FluidErrorTypes.dataCorruptionError, "Error type should be dataCorruptionError", ); - assert(disconnectResult.isCorruption, "isCorruption flag should be true"); }); it("Socket error with Data Processing error", async () => { @@ -272,16 +269,14 @@ describe("R11s Socket Tests", () => { const disconnectEventP = new Promise<{ clientId: string; errorType: string; - isCorruption: boolean; }>((resolve) => { assert(socket !== undefined, "Socket should be defined"); socket.on( "disconnect_document", - (clientId: string, documentId: string, errorType: string, isCorruption: boolean) => { + (clientId: string, documentId: string, errorType: string) => { resolve({ clientId, errorType, - isCorruption, }); }, ); @@ -302,9 +297,5 @@ describe("R11s Socket Tests", () => { FluidErrorTypes.dataProcessingError, "Error type should be dataProcessingError", ); - assert( - disconnectResult.isCorruption, - "isCorruption flag should be true for data processing error", - ); }); }); From 3b83f4ad67394c6b0c6efc4347e35d60e21f23de Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Thu, 27 Feb 2025 18:36:33 +0000 Subject: [PATCH 45/54] dont log if errorType is undefined --- server/routerlicious/packages/lambdas/src/nexus/index.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index fadeefa3e960..52cabc0a893a 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -646,6 +646,9 @@ export function configureWebSocketServices( socket.on( "disconnect_document", (clientId: string, documentId: string, errorType?: string) => { + if (errorType === undefined) { + return; + } Lumberjack.error( `Error for client ${clientId}, document ${documentId}: ${errorType}`, ); From a61344fd4efee3185bcaf509722264072be479bb Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Thu, 27 Feb 2025 20:08:45 +0000 Subject: [PATCH 46/54] update to use dispose instead of disconnect --- .../driver-definitions.legacy.alpha.api.md | 3 + .../driver-definitions/src/driverError.ts | 15 +++ .../common/driver-definitions/src/index.ts | 2 +- .../src/documentDeltaConnection.ts | 10 +- .../src/test/r11sSocketTests.spec.ts | 111 +----------------- 5 files changed, 28 insertions(+), 113 deletions(-) diff --git a/packages/common/driver-definitions/api-report/driver-definitions.legacy.alpha.api.md b/packages/common/driver-definitions/api-report/driver-definitions.legacy.alpha.api.md index 7d8a99a691de..694ddc9f3b1c 100644 --- a/packages/common/driver-definitions/api-report/driver-definitions.legacy.alpha.api.md +++ b/packages/common/driver-definitions/api-report/driver-definitions.legacy.alpha.api.md @@ -427,6 +427,9 @@ export interface IResolvedUrl { url: string; } +// @alpha +export function isDriverErrorType(message?: string): message is DriverErrorTypes; + // @public export interface ISequencedClient { client: IClient; diff --git a/packages/common/driver-definitions/src/driverError.ts b/packages/common/driver-definitions/src/driverError.ts index 7e0d54311c79..495fd55f4add 100644 --- a/packages/common/driver-definitions/src/driverError.ts +++ b/packages/common/driver-definitions/src/driverError.ts @@ -113,6 +113,21 @@ export const DriverErrorTypes = { */ export type DriverErrorTypes = (typeof DriverErrorTypes)[keyof typeof DriverErrorTypes]; +/** + * Checks if the provided message is one of the defined driver error types + * + * @param message - The error message to check + * @returns A boolean indicating if the message is a valid DriverErrorType + * @legacy + * @alpha + */ +export function isDriverErrorType(message?: string): message is DriverErrorTypes { + if (message === undefined) return false; + + // Check if the message matches any value in the DriverErrorTypes object + return Object.values(DriverErrorTypes).includes(message as DriverErrorTypes); +} + /** * Interface describing errors and warnings raised by any driver code. * diff --git a/packages/common/driver-definitions/src/index.ts b/packages/common/driver-definitions/src/index.ts index 28cf934f3528..b3bc8c6d7bec 100644 --- a/packages/common/driver-definitions/src/index.ts +++ b/packages/common/driver-definitions/src/index.ts @@ -13,7 +13,7 @@ export type { ILocationRedirectionError, IThrottlingWarning, } from "./driverError.js"; -export { DriverErrorTypes } from "./driverError.js"; +export { DriverErrorTypes, isDriverErrorType } from "./driverError.js"; export type { FiveDaysMs, IDeltasFetchResult, diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 9fdfda0464a1..e748e9fc378a 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -24,6 +24,7 @@ import { ScopeType, ISequencedDocumentMessage, ISignalMessage, + isDriverErrorType, } from "@fluidframework/driver-definitions/internal"; import { UsageError, @@ -401,7 +402,7 @@ export class DocumentDeltaConnection * However the OdspDocumentDeltaConnection differ in dispose as in there we don't close the socket. There is no * multiplexing here, so we need to close the socket here. */ - public dispose() { + public dispose(error?: Error) { this.logger.sendTelemetryEvent({ eventName: "ClientClosingDeltaConnection", driverVersion, @@ -409,11 +410,12 @@ export class DocumentDeltaConnection ...this.getConnectionDetailsProps(), }), }); + + const isDriverError = isDriverErrorType(error?.message); this.disconnect( createGenericNetworkError( - // pre-0.58 error message: clientClosingConnection - "Client closing delta connection", - { canRetry: true }, + isDriverError ? error.message : "Client closing delta connection", + { canRetry: !isDriverError }, { driverVersion }, ), ); diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 1674cec8495a..32f3dacd5e56 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -7,11 +7,7 @@ import { strict as assert } from "assert"; import { FluidErrorTypes } from "@fluidframework/core-interfaces/internal"; import { IClient } from "@fluidframework/driver-definitions"; -import { - DriverErrorTypes, - IResolvedUrl, - IAnyDriverError, -} from "@fluidframework/driver-definitions/internal"; +import { DriverErrorTypes, IResolvedUrl } from "@fluidframework/driver-definitions/internal"; import { DataCorruptionError, DataProcessingError, @@ -19,11 +15,9 @@ import { import { stub } from "sinon"; import { Socket } from "socket.io-client"; -import { R11sServiceClusterDrainingErrorCode } from "../contracts.js"; import { DefaultTokenProvider } from "../defaultTokenProvider.js"; import { DocumentService } from "../documentService.js"; import { RouterliciousDocumentServiceFactory } from "../documentServiceFactory.js"; -import { RouterliciousErrorTypes } from "../errorUtils.js"; import * as socketModule from "../socketModule.js"; // eslint-disable-next-line import/no-internal-modules import { ClientSocketMock } from "../test/socketTestUtils.ts/socketMock.js"; @@ -101,105 +95,6 @@ describe("R11s Socket Tests", () => { ); }); - it("Socket error with Token Revoked error", async () => { - const errorToThrow = { - code: 403, - message: "TokenRevokedError", - retryAfterMs: 10, - internalErrorCode: "TokenRevoked", - errorType: DriverErrorTypes.authorizationError, - canRetry: false, - }; - const errorEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, - }); - - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); - let error: IAnyDriverError | undefined; - connection.on("disconnect", (reason: IAnyDriverError) => { - error = reason; - }); - - // Send Token Revoked error - socket.sendErrorEvent(errorToThrow); - - assert( - error?.errorType === DriverErrorTypes.authorizationError, - "Error type should be authorizationError", - ); - assert(error.scenarioName === "error", "Error scenario name should be error"); - assert( - (error as any).internalErrorCode === "TokenRevoked", - "Error internal code should be TokenRevoked", - ); - }); - - it("connect_document_error with Cluster Draining error", async () => { - const errorToThrow = { - code: 503, - message: "ClusterDrainingError", - retryAfterMs: 1000, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - errorType: RouterliciousErrorTypes.clusterDrainingError, - canRetry: true, - }; - const errorEventName = "connect_document_error"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName, errorToThrow }, - }); - - await assert.rejects( - mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ), - { - errorType: RouterliciousErrorTypes.clusterDrainingError, - scenarioName: "connect_document_error", - internalErrorCode: R11sServiceClusterDrainingErrorCode, - }, - "Error should have occurred", - ); - }); - - it("Socket error with Cluster Draining error", async () => { - const errorToThrow = { - code: 503, - message: "ClusterDrainingError", - retryAfterMs: 1000, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - errorType: RouterliciousErrorTypes.clusterDrainingError, - canRetry: true, - }; - const errorEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, - }); - - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); - let error: IAnyDriverError | undefined; - connection.on("disconnect", (reason: IAnyDriverError) => { - error = reason; - }); - - // Send Token Revoked error - socket.sendErrorEvent(errorToThrow); - - assert( - error?.errorType === RouterliciousErrorTypes.clusterDrainingError, - "Error type should be clusterDrainingError", - ); - assert(error.scenarioName === "error", "Error scenario name should be error"); - assert( - (error as any).internalErrorCode === R11sServiceClusterDrainingErrorCode, - "Error internal code should be R11sServiceClusterDrainingErrorCode", - ); - }); - it("Socket error with Data Corruption error", async () => { const clientError = new DataCorruptionError("Data corruption error", { driverVersion: "1.0", @@ -232,7 +127,7 @@ describe("R11s Socket Tests", () => { }); // Call disconnect directly with the error - (connection as any).disconnect(clientError); + connection.dispose(clientError); // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; @@ -283,7 +178,7 @@ describe("R11s Socket Tests", () => { }); // Call disconnect directly with the error - (connection as any).disconnect(clientError); + connection.dispose(clientError); // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; From 7701d533252fa0573e1a1a19a1b94fe57eca63e7 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Thu, 27 Feb 2025 20:16:26 +0000 Subject: [PATCH 47/54] fix tests --- .../src/test/r11sSocketTests.spec.ts | 113 +++++++++++++++++- 1 file changed, 109 insertions(+), 4 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 32f3dacd5e56..e31c75210c47 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -7,17 +7,23 @@ import { strict as assert } from "assert"; import { FluidErrorTypes } from "@fluidframework/core-interfaces/internal"; import { IClient } from "@fluidframework/driver-definitions"; -import { DriverErrorTypes, IResolvedUrl } from "@fluidframework/driver-definitions/internal"; import { - DataCorruptionError, + DriverErrorTypes, + IResolvedUrl, + type IAnyDriverError, +} from "@fluidframework/driver-definitions/internal"; +import { DataProcessingError, + DataCorruptionError, } from "@fluidframework/telemetry-utils/internal"; import { stub } from "sinon"; import { Socket } from "socket.io-client"; +import { R11sServiceClusterDrainingErrorCode } from "../contracts.js"; import { DefaultTokenProvider } from "../defaultTokenProvider.js"; import { DocumentService } from "../documentService.js"; import { RouterliciousDocumentServiceFactory } from "../documentServiceFactory.js"; +import { RouterliciousErrorTypes } from "../errorUtils.js"; import * as socketModule from "../socketModule.js"; // eslint-disable-next-line import/no-internal-modules import { ClientSocketMock } from "../test/socketTestUtils.ts/socketMock.js"; @@ -95,7 +101,106 @@ describe("R11s Socket Tests", () => { ); }); - it("Socket error with Data Corruption error", async () => { + it("Socket error with Token Revoked error", async () => { + const errorToThrow = { + code: 403, + message: "TokenRevokedError", + retryAfterMs: 10, + internalErrorCode: "TokenRevoked", + errorType: DriverErrorTypes.authorizationError, + canRetry: false, + }; + const errorEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName }, + }); + + const connection = await mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ); + let error: IAnyDriverError | undefined; + connection.on("disconnect", (reason: IAnyDriverError) => { + error = reason; + }); + + // Send Token Revoked error + socket.sendErrorEvent(errorToThrow); + + assert( + error?.errorType === DriverErrorTypes.authorizationError, + "Error type should be authorizationError", + ); + assert(error.scenarioName === "error", "Error scenario name should be error"); + assert( + (error as any).internalErrorCode === "TokenRevoked", + "Error internal code should be TokenRevoked", + ); + }); + + it("connect_document_error with Cluster Draining error", async () => { + const errorToThrow = { + code: 503, + message: "ClusterDrainingError", + retryAfterMs: 1000, + internalErrorCode: R11sServiceClusterDrainingErrorCode, + errorType: RouterliciousErrorTypes.clusterDrainingError, + canRetry: true, + }; + const errorEventName = "connect_document_error"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName, errorToThrow }, + }); + + await assert.rejects( + mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ), + { + errorType: RouterliciousErrorTypes.clusterDrainingError, + scenarioName: "connect_document_error", + internalErrorCode: R11sServiceClusterDrainingErrorCode, + }, + "Error should have occurred", + ); + }); + + it("Socket error with Cluster Draining error", async () => { + const errorToThrow = { + code: 503, + message: "ClusterDrainingError", + retryAfterMs: 1000, + internalErrorCode: R11sServiceClusterDrainingErrorCode, + errorType: RouterliciousErrorTypes.clusterDrainingError, + canRetry: true, + }; + const errorEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName }, + }); + + const connection = await mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ); + let error: IAnyDriverError | undefined; + connection.on("disconnect", (reason: IAnyDriverError) => { + error = reason; + }); + + // Send Token Revoked error + socket.sendErrorEvent(errorToThrow); + + assert( + error?.errorType === RouterliciousErrorTypes.clusterDrainingError, + "Error type should be clusterDrainingError", + ); + assert(error.scenarioName === "error", "Error scenario name should be error"); + assert( + (error as any).internalErrorCode === R11sServiceClusterDrainingErrorCode, + "Error internal code should be R11sServiceClusterDrainingErrorCode", + ); + }); + + it("Client Data Corruption error", async () => { const clientError = new DataCorruptionError("Data corruption error", { driverVersion: "1.0", }); @@ -143,7 +248,7 @@ describe("R11s Socket Tests", () => { ); }); - it("Socket error with Data Processing error", async () => { + it("Client Data Processing error", async () => { const clientError = DataProcessingError.create( "Data processing error", "test", From f6550cc1597f643e20c9314fc55d74ab6b3a7358 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Thu, 27 Feb 2025 22:22:15 +0000 Subject: [PATCH 48/54] Revert "update to use dispose instead of disconnect" This reverts commit a61344fd4efee3185bcaf509722264072be479bb. --- .../driver-definitions.legacy.alpha.api.md | 3 --- .../common/driver-definitions/src/driverError.ts | 15 --------------- packages/common/driver-definitions/src/index.ts | 2 +- .../driver-base/src/documentDeltaConnection.ts | 10 ++++------ .../src/test/r11sSocketTests.spec.ts | 4 ++-- 5 files changed, 7 insertions(+), 27 deletions(-) diff --git a/packages/common/driver-definitions/api-report/driver-definitions.legacy.alpha.api.md b/packages/common/driver-definitions/api-report/driver-definitions.legacy.alpha.api.md index 694ddc9f3b1c..7d8a99a691de 100644 --- a/packages/common/driver-definitions/api-report/driver-definitions.legacy.alpha.api.md +++ b/packages/common/driver-definitions/api-report/driver-definitions.legacy.alpha.api.md @@ -427,9 +427,6 @@ export interface IResolvedUrl { url: string; } -// @alpha -export function isDriverErrorType(message?: string): message is DriverErrorTypes; - // @public export interface ISequencedClient { client: IClient; diff --git a/packages/common/driver-definitions/src/driverError.ts b/packages/common/driver-definitions/src/driverError.ts index 495fd55f4add..7e0d54311c79 100644 --- a/packages/common/driver-definitions/src/driverError.ts +++ b/packages/common/driver-definitions/src/driverError.ts @@ -113,21 +113,6 @@ export const DriverErrorTypes = { */ export type DriverErrorTypes = (typeof DriverErrorTypes)[keyof typeof DriverErrorTypes]; -/** - * Checks if the provided message is one of the defined driver error types - * - * @param message - The error message to check - * @returns A boolean indicating if the message is a valid DriverErrorType - * @legacy - * @alpha - */ -export function isDriverErrorType(message?: string): message is DriverErrorTypes { - if (message === undefined) return false; - - // Check if the message matches any value in the DriverErrorTypes object - return Object.values(DriverErrorTypes).includes(message as DriverErrorTypes); -} - /** * Interface describing errors and warnings raised by any driver code. * diff --git a/packages/common/driver-definitions/src/index.ts b/packages/common/driver-definitions/src/index.ts index b3bc8c6d7bec..28cf934f3528 100644 --- a/packages/common/driver-definitions/src/index.ts +++ b/packages/common/driver-definitions/src/index.ts @@ -13,7 +13,7 @@ export type { ILocationRedirectionError, IThrottlingWarning, } from "./driverError.js"; -export { DriverErrorTypes, isDriverErrorType } from "./driverError.js"; +export { DriverErrorTypes } from "./driverError.js"; export type { FiveDaysMs, IDeltasFetchResult, diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index e748e9fc378a..9fdfda0464a1 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -24,7 +24,6 @@ import { ScopeType, ISequencedDocumentMessage, ISignalMessage, - isDriverErrorType, } from "@fluidframework/driver-definitions/internal"; import { UsageError, @@ -402,7 +401,7 @@ export class DocumentDeltaConnection * However the OdspDocumentDeltaConnection differ in dispose as in there we don't close the socket. There is no * multiplexing here, so we need to close the socket here. */ - public dispose(error?: Error) { + public dispose() { this.logger.sendTelemetryEvent({ eventName: "ClientClosingDeltaConnection", driverVersion, @@ -410,12 +409,11 @@ export class DocumentDeltaConnection ...this.getConnectionDetailsProps(), }), }); - - const isDriverError = isDriverErrorType(error?.message); this.disconnect( createGenericNetworkError( - isDriverError ? error.message : "Client closing delta connection", - { canRetry: !isDriverError }, + // pre-0.58 error message: clientClosingConnection + "Client closing delta connection", + { canRetry: true }, { driverVersion }, ), ); diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index e31c75210c47..48737e7aec68 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -232,7 +232,7 @@ describe("R11s Socket Tests", () => { }); // Call disconnect directly with the error - connection.dispose(clientError); + (connection as any).disconnect(clientError); // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; @@ -283,7 +283,7 @@ describe("R11s Socket Tests", () => { }); // Call disconnect directly with the error - connection.dispose(clientError); + (connection as any).disconnect(clientError); // Wait for and verify the disconnect_document event const disconnectResult = await disconnectEventP; From 3e3b0528736c44158c9210a7a6c31cc90529d899 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Thu, 27 Feb 2025 22:28:42 +0000 Subject: [PATCH 49/54] generate tests --- .../src/test/r11sSocketTests.spec.ts | 381 ++++++++---------- 1 file changed, 172 insertions(+), 209 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index 48737e7aec68..f76a0c9adf62 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -74,228 +74,191 @@ describe("R11s Socket Tests", () => { )) as DocumentService; }); - it("connect_document_error with Token Revoked error", async () => { - const errorToThrow = { - code: 403, - message: "TokenRevokedError", - retryAfterMs: 10, - internalErrorCode: "TokenRevoked", - errorType: DriverErrorTypes.authorizationError, - canRetry: false, - }; - const errorEventName = "connect_document_error"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName, errorToThrow }, + function runConnectDocumentErrorTest( + description: string, + errorToThrow: any, + expectedErrorType: string, + expectedInternalErrorCode: string, + ) { + it(description, async () => { + const errorEventName = "connect_document_error"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName, errorToThrow }, + }); + await assert.rejects( + mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ), + { + errorType: expectedErrorType, + scenarioName: errorEventName, + internalErrorCode: expectedInternalErrorCode, + }, + "Error should have occurred", + ); }); + } - await assert.rejects( - mockSocket(socket as unknown as Socket, async () => + function runSocketErrorTest( + description: string, + errorToThrow: any, + expectedErrorType: string, + expectedInternalErrorCode: string, + ) { + it(description, async () => { + const errorEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName }, + }); + const connection = await mockSocket(socket as unknown as Socket, async () => documentService.connectToDeltaStream(client), - ), - { - errorType: DriverErrorTypes.authorizationError, - scenarioName: "connect_document_error", - internalErrorCode: "TokenRevoked", - }, - "Error should have occurred", - ); - }); - - it("Socket error with Token Revoked error", async () => { - const errorToThrow = { - code: 403, - message: "TokenRevokedError", - retryAfterMs: 10, - internalErrorCode: "TokenRevoked", - errorType: DriverErrorTypes.authorizationError, - canRetry: false, - }; - const errorEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, - }); - - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); - let error: IAnyDriverError | undefined; - connection.on("disconnect", (reason: IAnyDriverError) => { - error = reason; - }); - - // Send Token Revoked error - socket.sendErrorEvent(errorToThrow); - - assert( - error?.errorType === DriverErrorTypes.authorizationError, - "Error type should be authorizationError", - ); - assert(error.scenarioName === "error", "Error scenario name should be error"); - assert( - (error as any).internalErrorCode === "TokenRevoked", - "Error internal code should be TokenRevoked", - ); - }); - - it("connect_document_error with Cluster Draining error", async () => { - const errorToThrow = { - code: 503, - message: "ClusterDrainingError", - retryAfterMs: 1000, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - errorType: RouterliciousErrorTypes.clusterDrainingError, - canRetry: true, - }; - const errorEventName = "connect_document_error"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName, errorToThrow }, + ); + let error: IAnyDriverError | undefined; + connection.on("disconnect", (reason: IAnyDriverError) => { + error = reason; + }); + socket.sendErrorEvent(errorToThrow); + assert( + error?.errorType === expectedErrorType, + `Error type should be ${expectedErrorType}`, + ); + assert(error.scenarioName === "error", "Error scenario name should be error"); + assert( + (error as any).internalErrorCode === expectedInternalErrorCode, + `Error internal code should be ${expectedInternalErrorCode}`, + ); }); + } - await assert.rejects( - mockSocket(socket as unknown as Socket, async () => + function runClientErrorTest( + description: string, + clientError: any, + expectedErrorType: string, + ) { + it(description, async () => { + const socketEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: socketEventName }, + }); + const connection = await mockSocket(socket as unknown as Socket, async () => documentService.connectToDeltaStream(client), - ), - { - errorType: RouterliciousErrorTypes.clusterDrainingError, - scenarioName: "connect_document_error", - internalErrorCode: R11sServiceClusterDrainingErrorCode, - }, - "Error should have occurred", - ); - }); - - it("Socket error with Cluster Draining error", async () => { - const errorToThrow = { - code: 503, - message: "ClusterDrainingError", - retryAfterMs: 1000, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - errorType: RouterliciousErrorTypes.clusterDrainingError, - canRetry: true, - }; - const errorEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, - }); - - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); - let error: IAnyDriverError | undefined; - connection.on("disconnect", (reason: IAnyDriverError) => { - error = reason; - }); - - // Send Token Revoked error - socket.sendErrorEvent(errorToThrow); - - assert( - error?.errorType === RouterliciousErrorTypes.clusterDrainingError, - "Error type should be clusterDrainingError", - ); - assert(error.scenarioName === "error", "Error scenario name should be error"); - assert( - (error as any).internalErrorCode === R11sServiceClusterDrainingErrorCode, - "Error internal code should be R11sServiceClusterDrainingErrorCode", - ); - }); - - it("Client Data Corruption error", async () => { - const clientError = new DataCorruptionError("Data corruption error", { - driverVersion: "1.0", - }); - - const socketEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: socketEventName }, - }); - - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); - - // Set up Promise to await disconnect_document event - const disconnectEventP = new Promise<{ - clientId: string; - errorType: string; - }>((resolve) => { - assert(socket !== undefined, "Socket should be defined"); - socket.on( - "disconnect_document", - (clientId: string, documentId: string, errorType: string) => { - resolve({ - clientId, - errorType, - }); + ); + const disconnectEventP = new Promise<{ clientId: string; errorType: string }>( + (resolve) => { + assert(socket !== undefined, "Socket should be defined"); + socket.on( + "disconnect_document", + (clientId: string, _documentId: string, errorType: string) => { + resolve({ clientId, errorType }); + }, + ); }, ); + (connection as any).disconnect(clientError); + const disconnectResult = await disconnectEventP; + assert.strictEqual( + disconnectResult.clientId, + connection.clientId, + "Client ID should match", + ); + assert.strictEqual( + disconnectResult.errorType, + expectedErrorType, + `Error type should be ${expectedErrorType}`, + ); }); + } - // Call disconnect directly with the error - (connection as any).disconnect(clientError); - - // Wait for and verify the disconnect_document event - const disconnectResult = await disconnectEventP; - assert.strictEqual( - disconnectResult.clientId, - connection.clientId, - "Client ID should match", - ); - assert.strictEqual( - disconnectResult.errorType, - FluidErrorTypes.dataCorruptionError, - "Error type should be dataCorruptionError", - ); - }); - - it("Client Data Processing error", async () => { - const clientError = DataProcessingError.create( - "Data processing error", - "test", - undefined, - { driverVersion: "1.0" }, - ); + // connect_document_error tests + const connectErrorTests = [ + { + description: "connect_document_error with Token Revoked error", + errorToThrow: { + code: 403, + message: "TokenRevokedError", + retryAfterMs: 10, + internalErrorCode: "TokenRevoked", + errorType: DriverErrorTypes.authorizationError, + canRetry: false, + }, + expectedErrorType: DriverErrorTypes.authorizationError, + expectedInternalErrorCode: "TokenRevoked", + }, + { + description: "connect_document_error with Cluster Draining error", + errorToThrow: { + code: 503, + message: "ClusterDrainingError", + retryAfterMs: 1000, + internalErrorCode: R11sServiceClusterDrainingErrorCode, + errorType: RouterliciousErrorTypes.clusterDrainingError, + canRetry: true, + }, + expectedErrorType: RouterliciousErrorTypes.clusterDrainingError, + expectedInternalErrorCode: R11sServiceClusterDrainingErrorCode, + }, + ]; - const socketEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: socketEventName }, - }); + connectErrorTests.forEach((test) => + runConnectDocumentErrorTest( + test.description, + test.errorToThrow, + test.expectedErrorType, + test.expectedInternalErrorCode, + ), + ); - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); + // Socket error tests + const socketErrorTests = [ + { + description: "Socket error with Token Revoked error", + errorToThrow: { + code: 403, + message: "TokenRevokedError", + retryAfterMs: 10, + internalErrorCode: "TokenRevoked", + errorType: DriverErrorTypes.authorizationError, + canRetry: false, + }, + expectedErrorType: DriverErrorTypes.authorizationError, + expectedInternalErrorCode: "TokenRevoked", + }, + { + description: "Socket error with Cluster Draining error", + errorToThrow: { + code: 503, + message: "ClusterDrainingError", + retryAfterMs: 1000, + internalErrorCode: R11sServiceClusterDrainingErrorCode, + errorType: RouterliciousErrorTypes.clusterDrainingError, + canRetry: true, + }, + expectedErrorType: RouterliciousErrorTypes.clusterDrainingError, + expectedInternalErrorCode: R11sServiceClusterDrainingErrorCode, + }, + ]; - // Set up Promise to await disconnect_document event - const disconnectEventP = new Promise<{ - clientId: string; - errorType: string; - }>((resolve) => { - assert(socket !== undefined, "Socket should be defined"); - socket.on( - "disconnect_document", - (clientId: string, documentId: string, errorType: string) => { - resolve({ - clientId, - errorType, - }); - }, - ); - }); + socketErrorTests.forEach((test) => + runSocketErrorTest( + test.description, + test.errorToThrow, + test.expectedErrorType, + test.expectedInternalErrorCode, + ), + ); - // Call disconnect directly with the error - (connection as any).disconnect(clientError); + // Client error tests + runClientErrorTest( + "Client Data Corruption error", + new DataCorruptionError("Data corruption error", { driverVersion: "1.0" }), + FluidErrorTypes.dataCorruptionError, + ); - // Wait for and verify the disconnect_document event - const disconnectResult = await disconnectEventP; - assert.strictEqual( - disconnectResult.clientId, - connection.clientId, - "Client ID should match", - ); - assert.strictEqual( - disconnectResult.errorType, - FluidErrorTypes.dataProcessingError, - "Error type should be dataProcessingError", - ); - }); + runClientErrorTest( + "Client Data Processing error", + DataProcessingError.create("Data processing error", "test", undefined, { + driverVersion: "1.0", + }), + FluidErrorTypes.dataProcessingError, + ); }); From 159ad19b41d7bdd3ef590647ccc9ff52c906ab0f Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Fri, 28 Feb 2025 18:56:34 +0000 Subject: [PATCH 50/54] zach's feedback --- server/routerlicious/packages/lambdas/src/nexus/index.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index 52cabc0a893a..3c03433eac7b 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -650,7 +650,9 @@ export function configureWebSocketServices( return; } Lumberjack.error( - `Error for client ${clientId}, document ${documentId}: ${errorType}`, + "Client disconnected due to error", + { clientId, documentId, errorType }, + new Error(errorType), ); }, ); From 054c761841d4bd9f5dc61e28ac0be09da088aace Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Fri, 28 Feb 2025 21:44:17 +0000 Subject: [PATCH 51/54] jason's feedback --- packages/drivers/driver-base/src/documentDeltaConnection.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 9fdfda0464a1..29377ba5f69e 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -419,7 +419,7 @@ export class DocumentDeltaConnection ); } - protected disconnect(err?: IAnyDriverError) { + protected disconnect = (err: IAnyDriverError) => { // Can't check this.disposed here, as we get here on socket closure, // so _disposed & socket.connected might be not in sync while processing // "dispose" event. @@ -455,7 +455,7 @@ export class DocumentDeltaConnection * Disconnect from the websocket. * @param reason - reason for disconnect */ - protected disconnectCore(err?: IAnyDriverError) { + protected disconnectCore = (err: IAnyDriverError) => { this.socket.disconnect(); } From b4add467f61a0d7578947f29cfaf1d1d66fca946 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Sun, 9 Mar 2025 18:08:43 +0000 Subject: [PATCH 52/54] format --- .../drivers/driver-base/src/documentDeltaConnection.ts | 4 ++-- .../routerlicious-driver/src/documentDeltaConnection.ts | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index 29377ba5f69e..c900879f36da 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -449,7 +449,7 @@ export class DocumentDeltaConnection // Let user of connection object know about disconnect. this.emit("disconnect", err); - } + }; /** * Disconnect from the websocket. @@ -457,7 +457,7 @@ export class DocumentDeltaConnection */ protected disconnectCore = (err: IAnyDriverError) => { this.socket.disconnect(); - } + }; protected async initialize(connectMessage: IConnect, timeout: number) { this.socket.on("op", this.earlyOpHandler); diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index 22e0d08cfc69..45f685035f14 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -107,4 +107,13 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { url: getUrlForTelemetry(this.url, socketIoPath), }; } + + /** + * Disconnect from the websocket + */ + protected disconnectCore = (err: IAnyDriverError): void => { + // tell the server we are disconnecting this client from the document + this.socket.emit("disconnect_document", this.clientId, this.documentId, err?.errorType); + super.disconnectCore(err); + }; } From 6c1df191371083d9081335774e6192449cf3a663 Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Sun, 9 Mar 2025 18:19:30 +0000 Subject: [PATCH 53/54] cherry pick tests and server changes --- .../src/test/r11sSocketTests.spec.ts | 289 +++++++++++------- .../packages/lambdas/src/nexus/index.ts | 14 + 2 files changed, 192 insertions(+), 111 deletions(-) diff --git a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts index e7c37f2a4579..f76a0c9adf62 100644 --- a/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts +++ b/packages/drivers/routerlicious-driver/src/test/r11sSocketTests.spec.ts @@ -5,12 +5,17 @@ import { strict as assert } from "assert"; +import { FluidErrorTypes } from "@fluidframework/core-interfaces/internal"; import { IClient } from "@fluidframework/driver-definitions"; import { DriverErrorTypes, IResolvedUrl, type IAnyDriverError, } from "@fluidframework/driver-definitions/internal"; +import { + DataProcessingError, + DataCorruptionError, +} from "@fluidframework/telemetry-utils/internal"; import { stub } from "sinon"; import { Socket } from "socket.io-client"; @@ -69,129 +74,191 @@ describe("R11s Socket Tests", () => { )) as DocumentService; }); - it("connect_document_error with Token Revoked error", async () => { - const errorToThrow = { - code: 403, - message: "TokenRevokedError", - retryAfterMs: 10, - internalErrorCode: "TokenRevoked", - errorType: DriverErrorTypes.authorizationError, - canRetry: false, - }; - const errorEventName = "connect_document_error"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName, errorToThrow }, + function runConnectDocumentErrorTest( + description: string, + errorToThrow: any, + expectedErrorType: string, + expectedInternalErrorCode: string, + ) { + it(description, async () => { + const errorEventName = "connect_document_error"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName, errorToThrow }, + }); + await assert.rejects( + mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ), + { + errorType: expectedErrorType, + scenarioName: errorEventName, + internalErrorCode: expectedInternalErrorCode, + }, + "Error should have occurred", + ); }); + } - await assert.rejects( - mockSocket(socket as unknown as Socket, async () => + function runSocketErrorTest( + description: string, + errorToThrow: any, + expectedErrorType: string, + expectedInternalErrorCode: string, + ) { + it(description, async () => { + const errorEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: errorEventName }, + }); + const connection = await mockSocket(socket as unknown as Socket, async () => documentService.connectToDeltaStream(client), - ), - { - errorType: DriverErrorTypes.authorizationError, - scenarioName: "connect_document_error", - internalErrorCode: "TokenRevoked", - }, - "Error should have occurred", - ); - }); - - it("Socket error with Token Revoked error", async () => { - const errorToThrow = { - code: 403, - message: "TokenRevokedError", - retryAfterMs: 10, - internalErrorCode: "TokenRevoked", - errorType: DriverErrorTypes.authorizationError, - canRetry: false, - }; - const errorEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, + ); + let error: IAnyDriverError | undefined; + connection.on("disconnect", (reason: IAnyDriverError) => { + error = reason; + }); + socket.sendErrorEvent(errorToThrow); + assert( + error?.errorType === expectedErrorType, + `Error type should be ${expectedErrorType}`, + ); + assert(error.scenarioName === "error", "Error scenario name should be error"); + assert( + (error as any).internalErrorCode === expectedInternalErrorCode, + `Error internal code should be ${expectedInternalErrorCode}`, + ); }); + } - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); - let error: IAnyDriverError | undefined; - connection.on("disconnect", (reason: IAnyDriverError) => { - error = reason; + function runClientErrorTest( + description: string, + clientError: any, + expectedErrorType: string, + ) { + it(description, async () => { + const socketEventName = "connect_document_success"; + socket = new ClientSocketMock({ + connect_document: { eventToEmit: socketEventName }, + }); + const connection = await mockSocket(socket as unknown as Socket, async () => + documentService.connectToDeltaStream(client), + ); + const disconnectEventP = new Promise<{ clientId: string; errorType: string }>( + (resolve) => { + assert(socket !== undefined, "Socket should be defined"); + socket.on( + "disconnect_document", + (clientId: string, _documentId: string, errorType: string) => { + resolve({ clientId, errorType }); + }, + ); + }, + ); + (connection as any).disconnect(clientError); + const disconnectResult = await disconnectEventP; + assert.strictEqual( + disconnectResult.clientId, + connection.clientId, + "Client ID should match", + ); + assert.strictEqual( + disconnectResult.errorType, + expectedErrorType, + `Error type should be ${expectedErrorType}`, + ); }); + } - // Send Token Revoked error - socket.sendErrorEvent(errorToThrow); - - assert( - error?.errorType === DriverErrorTypes.authorizationError, - "Error type should be authorizationError", - ); - assert(error.scenarioName === "error", "Error scenario name should be error"); - assert( - (error as any).internalErrorCode === "TokenRevoked", - "Error internal code should be TokenRevoked", - ); - }); + // connect_document_error tests + const connectErrorTests = [ + { + description: "connect_document_error with Token Revoked error", + errorToThrow: { + code: 403, + message: "TokenRevokedError", + retryAfterMs: 10, + internalErrorCode: "TokenRevoked", + errorType: DriverErrorTypes.authorizationError, + canRetry: false, + }, + expectedErrorType: DriverErrorTypes.authorizationError, + expectedInternalErrorCode: "TokenRevoked", + }, + { + description: "connect_document_error with Cluster Draining error", + errorToThrow: { + code: 503, + message: "ClusterDrainingError", + retryAfterMs: 1000, + internalErrorCode: R11sServiceClusterDrainingErrorCode, + errorType: RouterliciousErrorTypes.clusterDrainingError, + canRetry: true, + }, + expectedErrorType: RouterliciousErrorTypes.clusterDrainingError, + expectedInternalErrorCode: R11sServiceClusterDrainingErrorCode, + }, + ]; - it("connect_document_error with Cluster Draining error", async () => { - const errorToThrow = { - code: 503, - message: "ClusterDrainingError", - retryAfterMs: 1000, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - errorType: RouterliciousErrorTypes.clusterDrainingError, - canRetry: true, - }; - const errorEventName = "connect_document_error"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName, errorToThrow }, - }); + connectErrorTests.forEach((test) => + runConnectDocumentErrorTest( + test.description, + test.errorToThrow, + test.expectedErrorType, + test.expectedInternalErrorCode, + ), + ); - await assert.rejects( - mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ), - { - errorType: RouterliciousErrorTypes.clusterDrainingError, - scenarioName: "connect_document_error", + // Socket error tests + const socketErrorTests = [ + { + description: "Socket error with Token Revoked error", + errorToThrow: { + code: 403, + message: "TokenRevokedError", + retryAfterMs: 10, + internalErrorCode: "TokenRevoked", + errorType: DriverErrorTypes.authorizationError, + canRetry: false, + }, + expectedErrorType: DriverErrorTypes.authorizationError, + expectedInternalErrorCode: "TokenRevoked", + }, + { + description: "Socket error with Cluster Draining error", + errorToThrow: { + code: 503, + message: "ClusterDrainingError", + retryAfterMs: 1000, internalErrorCode: R11sServiceClusterDrainingErrorCode, + errorType: RouterliciousErrorTypes.clusterDrainingError, + canRetry: true, }, - "Error should have occurred", - ); - }); - - it("Socket error with Cluster Draining error", async () => { - const errorToThrow = { - code: 503, - message: "ClusterDrainingError", - retryAfterMs: 1000, - internalErrorCode: R11sServiceClusterDrainingErrorCode, - errorType: RouterliciousErrorTypes.clusterDrainingError, - canRetry: true, - }; - const errorEventName = "connect_document_success"; - socket = new ClientSocketMock({ - connect_document: { eventToEmit: errorEventName }, - }); + expectedErrorType: RouterliciousErrorTypes.clusterDrainingError, + expectedInternalErrorCode: R11sServiceClusterDrainingErrorCode, + }, + ]; - const connection = await mockSocket(socket as unknown as Socket, async () => - documentService.connectToDeltaStream(client), - ); - let error: IAnyDriverError | undefined; - connection.on("disconnect", (reason: IAnyDriverError) => { - error = reason; - }); + socketErrorTests.forEach((test) => + runSocketErrorTest( + test.description, + test.errorToThrow, + test.expectedErrorType, + test.expectedInternalErrorCode, + ), + ); - // Send Token Revoked error - socket.sendErrorEvent(errorToThrow); + // Client error tests + runClientErrorTest( + "Client Data Corruption error", + new DataCorruptionError("Data corruption error", { driverVersion: "1.0" }), + FluidErrorTypes.dataCorruptionError, + ); - assert( - error?.errorType === RouterliciousErrorTypes.clusterDrainingError, - "Error type should be clusterDrainingError", - ); - assert(error.scenarioName === "error", "Error scenario name should be error"); - assert( - (error as any).internalErrorCode === R11sServiceClusterDrainingErrorCode, - "Error internal code should be R11sServiceClusterDrainingErrorCode", - ); - }); + runClientErrorTest( + "Client Data Processing error", + DataProcessingError.create("Data processing error", "test", undefined, { + driverVersion: "1.0", + }), + FluidErrorTypes.dataProcessingError, + ); }); diff --git a/server/routerlicious/packages/lambdas/src/nexus/index.ts b/server/routerlicious/packages/lambdas/src/nexus/index.ts index a8805b2834ed..3c03433eac7b 100644 --- a/server/routerlicious/packages/lambdas/src/nexus/index.ts +++ b/server/routerlicious/packages/lambdas/src/nexus/index.ts @@ -642,5 +642,19 @@ export function configureWebSocketServices( } disposers.splice(0, disposers.length); }); + + socket.on( + "disconnect_document", + (clientId: string, documentId: string, errorType?: string) => { + if (errorType === undefined) { + return; + } + Lumberjack.error( + "Client disconnected due to error", + { clientId, documentId, errorType }, + new Error(errorType), + ); + }, + ); }); } From d82e71267bc96150ded32de2ce85b1ecaa89e9bc Mon Sep 17 00:00:00 2001 From: Rishhi Balakrishnan <107130183+RishhiB@users.noreply.github.com> Date: Sun, 9 Mar 2025 18:33:11 +0000 Subject: [PATCH 54/54] remove arrow functions --- packages/drivers/driver-base/src/documentDeltaConnection.ts | 4 ++-- .../routerlicious-driver/src/documentDeltaConnection.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/drivers/driver-base/src/documentDeltaConnection.ts b/packages/drivers/driver-base/src/documentDeltaConnection.ts index c900879f36da..8cca73df3081 100644 --- a/packages/drivers/driver-base/src/documentDeltaConnection.ts +++ b/packages/drivers/driver-base/src/documentDeltaConnection.ts @@ -455,9 +455,9 @@ export class DocumentDeltaConnection * Disconnect from the websocket. * @param reason - reason for disconnect */ - protected disconnectCore = (err: IAnyDriverError) => { + protected disconnectCore(err: IAnyDriverError) { this.socket.disconnect(); - }; + } protected async initialize(connectMessage: IConnect, timeout: number) { this.socket.on("op", this.earlyOpHandler); diff --git a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts index 45f685035f14..62d759fb0139 100644 --- a/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts +++ b/packages/drivers/routerlicious-driver/src/documentDeltaConnection.ts @@ -111,9 +111,9 @@ export class R11sDocumentDeltaConnection extends DocumentDeltaConnection { /** * Disconnect from the websocket */ - protected disconnectCore = (err: IAnyDriverError): void => { + protected disconnectCore(err: IAnyDriverError): void { // tell the server we are disconnecting this client from the document this.socket.emit("disconnect_document", this.clientId, this.documentId, err?.errorType); super.disconnectCore(err); - }; + } }