Properly log errors and close container when erorrs happen inside tim… (#20704)

Part of #20703 investigation.

In one of the service runs done locally (having original "getSelf" changes applied), I've noticed that error generated as result of hitting assert in ConnectionStateHandler was printed on the screen by runner, i.e. without hitting out logging system or closing container. Examining code, it's obvious that there are a number of code paths where same can happen - most of the activities related to connectivity are run in response to timers or incoming messages / packets. We do not wrap then with try/catch - any error hit there will leave system in inconsistent state, without us every noticing directly (unless someone looks at Loop logs that capture unhandled exceptions).

Addressing these issues for all code paths that touch ConnectionStateHandler.
More might need to be done in overall space though (for example, likely same needs to be done for all methods on IConnectionManagerFactoryArgs - you can notice that we were hit by same problem as incomingOpHandler has try/catch, but not any other method). https://dev.azure.com/fluidframework/internal/_workitems/edit/7784 tracks follow up.

Author: vladsud

packages/loader/container-loader/src/connectionStateHandler.ts

@@ -3,7 +3,7 @@
  * Licensed under the MIT License.
  */
 
-import { IDeltaManager } from "@fluidframework/container-definitions";
+import { IDeltaManager, ICriticalContainerError } from "@fluidframework/container-definitions";
 import { ITelemetryBaseProperties } from "@fluidframework/core-interfaces";
 import { assert, Timer } from "@fluidframework/core-utils/internal";
 import { IAnyDriverError } from "@fluidframework/driver-definitions";
@@ -12,6 +12,7 @@ import { ITelemetryLoggerExt, type TelemetryEventCategory } from "@fluidframewor
 import {
 	PerformanceEvent,
 	loggerToMonitoringContext,
+	normalizeError,
 } from "@fluidframework/telemetry-utils/internal";
 
 import { CatchUpMonitor, ICatchUpMonitor } from "./catchUpMonitor.js";
@@ -48,6 +49,9 @@ export interface IConnectionStateHandlerInputs {
 	) => void;
 	/** Callback to note that an old local client ID is still present in the Quorum that should have left and should now be considered invalid */
 	clientShouldHaveLeft: (clientId: string) => void;
+
+	/** Some critical error was hit. Container should be closed and error logged. */
+	onCriticalError: (error: ICriticalContainerError) => void;
 }
 
 /**
@@ -196,6 +200,10 @@ class ConnectionStateHandlerPassThrough
 	public clientShouldHaveLeft(clientId: string) {
 		return this.inputs.clientShouldHaveLeft(clientId);
 	}
+
+	public onCriticalError(error: ICriticalContainerError) {
+		return this.inputs.onCriticalError(error);
+	}
 }
 
 /**
@@ -357,11 +365,15 @@ class ConnectionStateHandler implements IConnectionStateHandler {
 			// the max time on server after which leave op is sent.
 			this.handler.maxClientLeaveWaitTime ?? 300000,
 			() => {
-				assert(
-					this.connectionState !== ConnectionState.Connected,
-					0x2ac /* "Connected when timeout waiting for leave from previous session fired!" */,
-				);
-				this.applyForConnectedState("timeout");
+				try {
+					assert(
+						this.connectionState !== ConnectionState.Connected,
+						0x2ac /* "Connected when timeout waiting for leave from previous session fired!" */,
+					);
+					this.applyForConnectedState("timeout");
+				} catch (error) {
+					this.handler.onCriticalError(normalizeError(error));
+				}
 			},
 		);
 

packages/loader/container-loader/src/container.ts

@@ -372,7 +372,7 @@ export class Container
 
 		return PerformanceEvent.timedExecAsync(
 			container.mc.logger,
-			{ eventName: "Load" },
+			{ eventName: "Load", ...loadMode },
 			async (event) =>
 				new Promise<Container>((resolve, reject) => {
 					const defaultMode: IContainerLoadMode = { opsBeforeReturn: "cached" };
@@ -397,7 +397,7 @@ export class Container
 						})
 						.then(
 							(props) => {
-								event.end({ ...props, ...loadMode });
+								event.end({ ...props });
 								resolve(container);
 							},
 							(error) => {
@@ -721,6 +721,7 @@ export class Container
 				},
 				error,
 			);
+			this.close(normalizeError(error));
 		});
 
 		const {
@@ -893,6 +894,9 @@ export class Container
 				clientShouldHaveLeft: (clientId: string) => {
 					this.clientsWhoShouldHaveLeft.add(clientId);
 				},
+				onCriticalError: (error: ICriticalContainerError) => {
+					this.close(error);
+				},
 			},
 			this.deltaManager,
 			pendingLocalState?.clientId,

packages/loader/container-loader/src/deltaManager.ts

@@ -3,7 +3,6 @@
  * Licensed under the MIT License.
  */
 
-import { TypedEventEmitter } from "@fluid-internal/client-utils";
 import {
 	ICriticalContainerError,
 	IDeltaManager,
@@ -47,6 +46,7 @@ import {
 	isFluidError,
 	normalizeError,
 	safeRaiseEvent,
+	EventEmitterWithErrorHandling,
 } from "@fluidframework/telemetry-utils/internal";
 import { v4 as uuid } from "uuid";
 
@@ -149,7 +149,7 @@ function logIfFalse(
  * messages in order regardless of possible network conditions or timings causing out of order delivery.
  */
 export class DeltaManager<TConnectionManager extends IConnectionManager>
-	extends TypedEventEmitter<IDeltaManagerInternalEvents>
+	extends EventEmitterWithErrorHandling<IDeltaManagerInternalEvents>
 	implements
 		IDeltaManager<ISequencedDocumentMessage, IDocumentMessage>,
 		IEventProvider<IDeltaManagerInternalEvents>
@@ -411,7 +411,16 @@ export class DeltaManager<TConnectionManager extends IConnectionManager>
 		private readonly _active: () => boolean,
 		createConnectionManager: (props: IConnectionManagerFactoryArgs) => TConnectionManager,
 	) {
-		super();
+		super((name, error) => {
+			this.logger.sendErrorEvent(
+				{
+					eventName: "DeltaManagerEventHandlerException",
+					name: typeof name === "string" ? name : undefined,
+				},
+				error,
+			);
+			this.close(normalizeError(error));
+		});
 		const props: IConnectionManagerFactoryArgs = {
 			incomingOpHandler: (messages: ISequencedDocumentMessage[], reason: string) => {
 				try {

packages/loader/container-loader/src/test/connectionStateHandler.spec.ts

@@ -157,6 +157,10 @@ describe("ConnectionStateHandler Tests", () => {
 			connectionStateChanged: () => {},
 			logger: createChildLogger(),
 			clientShouldHaveLeft: (clientId: string) => {},
+			onCriticalError: (error) => {
+				// eslint-disable-next-line @typescript-eslint/no-throw-literal
+				throw error;
+			},
 		};
 
 		deltaManagerForCatchingUp = new MockDeltaManagerForCatchingUp();