Close the container by default (#16612)

[AB#5129](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/5129) In light of #15140, this moves the close container logic to automatically close the container by default instead of hiding it behind a feature flag. The default behavior should be to close the container after 5 seconds The new behavior is that instead of making a network refresh, we will close the container by default. I ran into an interesting piece of behavior, we will still need a lot of the refresh code, but it's only for updating state on the current container. --------- Co-authored-by: Alex Villarreal <[email protected]>
microsoft · Aug 9, 2023 · fc20a97 · fc20a97
1 parent 9c42b1e
commit fc20a97
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 20 deletions.
diff --git a/packages/runtime/container-runtime/src/containerRuntime.ts b/packages/runtime/container-runtime/src/containerRuntime.ts
@@ -542,7 +542,7 @@ const defaultChunkSizeInBytes = 204800;
  * of the current system, we should close the summarizer and let it recover.
  * This delay's goal is to prevent tight restart loops
  */
-const defaultCloseSummarizerDelayMs = 10000; // 10 seconds
+const defaultCloseSummarizerDelayMs = 5000; // 5 seconds
 
 /**
  * @deprecated - use ContainerRuntimeMessage instead
@@ -3481,7 +3481,7 @@ export class ContainerRuntime
 			 * change that started fetching latest snapshot always.
 			 */
 			if (fetchResult.latestSnapshotRefSeq < summaryRefSeq) {
-				fetchResult = await this.fetchSnapshotFromStorage(
+				fetchResult = await this.fetchSnapshotFromStorageAndClose(
 					summaryLogger,
 					{
 						eventName: "RefreshLatestSummaryAckFetchBackCompat",
@@ -3588,10 +3588,15 @@ export class ContainerRuntime
 		event: ITelemetryGenericEvent,
 		readAndParseBlob: ReadAndParseBlob,
 	): Promise<{ snapshotTree: ISnapshotTree; versionId: string; latestSnapshotRefSeq: number }> {
-		return this.fetchSnapshotFromStorage(logger, event, readAndParseBlob, null /* latest */);
+		return this.fetchSnapshotFromStorageAndClose(
+			logger,
+			event,
+			readAndParseBlob,
+			null /* latest */,
+		);
 	}
 
-	private async fetchSnapshotFromStorage(
+	private async fetchSnapshotFromStorageAndClose(
 		logger: ITelemetryLoggerExt,
 		event: ITelemetryGenericEvent,
 		readAndParseBlob: ReadAndParseBlob,
@@ -3647,7 +3652,7 @@ export class ContainerRuntime
 		// We choose to close the summarizer after the snapshot cache is updated to avoid
 		// situations which the main client (which is likely to be re-elected as the leader again)
 		// loads the summarizer from cache.
-		if (this.summaryStateUpdateMethod === "restart") {
+		if (this.summaryStateUpdateMethod !== "refreshFromSnapshot") {
 			this.mc.logger.sendTelemetryEvent(
 				{
 					...event,
@@ -3660,10 +3665,10 @@ export class ContainerRuntime
 				new GenericError("Restarting summarizer instead of refreshing"),
 			);
 
-			// Delay 10 seconds before restarting summarizer to prevent the summarizer from restarting too frequently.
+			// Delay before restarting summarizer to prevent the summarizer from restarting too frequently.
 			await delay(this.closeSummarizerDelayMs);
 			this._summarizer?.stop("latestSummaryStateStale");
-			this.closeFn();
+			this.disposeFn();
 		}
 
 		return snapshotResults;

diff --git a/packages/test/test-end-to-end-tests/src/test/gc/gcLoadingFromOlderSummaries.spec.ts b/packages/test/test-end-to-end-tests/src/test/gc/gcLoadingFromOlderSummaries.spec.ts
@@ -16,7 +16,9 @@ import { SharedMap } from "@fluidframework/map";
 import { requestFluidObject } from "@fluidframework/runtime-utils";
 import {
 	createSummarizer,
+	ITestContainerConfig,
 	ITestObjectProvider,
+	mockConfigProvider,
 	summarizeNow,
 	waitForContainerConnection,
 } from "@fluidframework/test-utils";
@@ -39,6 +41,14 @@ describeNoCompat("GC loading from older summaries", (getTestObjectProvider) => {
 	let containerRuntime: IContainerRuntime;
 	let dataStoreA: ITestDataObject;
 
+	const settings = {
+		"Fluid.ContainerRuntime.Test.SummaryStateUpdateMethodV2": "refreshFromSnapshot",
+	};
+	const testConfig: ITestContainerConfig = {
+		...defaultGCConfig,
+		loaderProps: { configProvider: mockConfigProvider(settings) },
+	};
+
 	/**
 	 * Returns the reference state for all the nodes in the given summary tree.
 	 * If a node is referenced, its value is true. If it's unreferenced, its value is false.
@@ -101,7 +111,7 @@ describeNoCompat("GC loading from older summaries", (getTestObjectProvider) => {
 
 	beforeEach(async function () {
 		provider = getTestObjectProvider({ syncSummarizer: true });
-		mainContainer = await provider.makeTestContainer(defaultGCConfig);
+		mainContainer = await provider.makeTestContainer(testConfig);
 		const defaultDataStore = await requestFluidObject<ITestDataObject>(
 			mainContainer,
 			"default",
@@ -147,7 +157,7 @@ describeNoCompat("GC loading from older summaries", (getTestObjectProvider) => {
 		const { container: container2, summarizer: summarizer2 } = await createSummarizer(
 			provider,
 			mainContainer,
-			undefined,
+			{ loaderProps: { configProvider: mockConfigProvider(settings) } },
 			summaryResult1.summaryVersion,
 		);
 
@@ -224,7 +234,7 @@ describeNoCompat("GC loading from older summaries", (getTestObjectProvider) => {
 		const { container: container2, summarizer: summarizer2 } = await createSummarizer(
 			provider,
 			mainContainer,
-			undefined,
+			{ loaderProps: { configProvider: mockConfigProvider(settings) } },
 			summaryResult1.summaryVersion,
 		);
 
@@ -311,7 +321,7 @@ describeNoCompat("GC loading from older summaries", (getTestObjectProvider) => {
 		const { container: container2, summarizer: summarizer2 } = await createSummarizer(
 			provider,
 			mainContainer,
-			undefined,
+			{ loaderProps: { configProvider: mockConfigProvider(settings) } },
 			summaryResult1.summaryVersion,
 		);
 
@@ -391,7 +401,7 @@ describeNoCompat("GC loading from older summaries", (getTestObjectProvider) => {
 		const { container: container2, summarizer: summarizer2 } = await createSummarizer(
 			provider,
 			mainContainer,
-			undefined,
+			{ loaderProps: { configProvider: mockConfigProvider(settings) } },
 			summaryResult1.summaryVersion,
 		);
 
@@ -466,7 +476,7 @@ describeNoCompat("GC loading from older summaries", (getTestObjectProvider) => {
 		const { container: container2, summarizer: summarizer2 } = await createSummarizer(
 			provider,
 			mainContainer,
-			undefined,
+			{ loaderProps: { configProvider: mockConfigProvider(settings) } },
 			summaryResult1.summaryVersion,
 		);
 

diff --git a/packages/test/test-end-to-end-tests/src/test/gc/gcStateResetInSummaries.spec.ts b/packages/test/test-end-to-end-tests/src/test/gc/gcStateResetInSummaries.spec.ts
@@ -13,6 +13,7 @@ import {
 	ITestContainerConfig,
 	ITestObjectProvider,
 	createSummarizer,
+	mockConfigProvider,
 	waitForContainerConnection,
 } from "@fluidframework/test-utils";
 import {
@@ -33,6 +34,9 @@ import { getGCStateFromSummary } from "./gcTestSummaryUtils.js";
 describeNoCompat("GC state reset in summaries", (getTestObjectProvider) => {
 	let provider: ITestObjectProvider;
 	let mainContainer: IContainer;
+	const settings = {
+		"Fluid.ContainerRuntime.Test.SummaryStateUpdateMethodV2": "refreshFromSnapshot",
+	};
 
 	/** Creates a new container with the GC enabled / disabled as per gcAllowed param. */
 	const createContainer = async (gcAllowed: boolean): Promise<IContainer> => {
@@ -44,6 +48,7 @@ describeNoCompat("GC state reset in summaries", (getTestObjectProvider) => {
 					gcAllowed,
 				},
 			},
+			loaderProps: { configProvider: mockConfigProvider(settings) },
 		};
 		return provider.makeTestContainer(testContainerConfig);
 	};
@@ -336,11 +341,21 @@ describeNoCompat("GC state reset in summaries", (getTestObjectProvider) => {
 			await createSummarizer(
 				provider,
 				mainContainer,
-				{ runtimeOptions: { gcOptions: { disableGC: true } } },
+				{
+					runtimeOptions: { gcOptions: { disableGC: true } },
+					loaderProps: { configProvider: mockConfigProvider(settings) },
+				},
 				summaryVersion,
 			);
 		const { container: containerGCEnabled2, summarizer: summarizerGCEnabled2 } =
-			await createSummarizer(provider, mainContainer, undefined, summaryVersion);
+			await createSummarizer(
+				provider,
+				mainContainer,
+				{
+					loaderProps: { configProvider: mockConfigProvider(settings) },
+				},
+				summaryVersion,
+			);
 
 		// Close the previous summarizer such that the summarizer with GC disabled is chosen as the current summarizer.
 		summarizerGCEnabled.close();

diff --git a/packages/test/test-service-load/testConfig.json b/packages/test/test-service-load/testConfig.json
@@ -27,20 +27,17 @@
 			"optionOverrides": {
 				"odsp": {
 					"configurations": {
-						"Fluid.ContainerRuntime.Test.SummaryStateUpdateMethodV2": ["restart", "default"],
 						"Fluid.ContainerRuntime.Test.ValidateSummaryBeforeUpload": [true, false]
 					}
 				},
 				"odsp-odsp-df": {
 					"configurations": {
 						"Fluid.Driver.Odsp.snapshotFormatFetchType": [2],
-						"Fluid.ContainerRuntime.Test.SummaryStateUpdateMethodV2": ["restart", "default"],
 						"Fluid.ContainerRuntime.Test.ValidateSummaryBeforeUpload": [true, false]
 					}
 				},
 				"tinylicious": {
 					"configurations": {
-						"Fluid.ContainerRuntime.Test.SummaryStateUpdateMethodV2": ["restart", "default"],
 						"Fluid.ContainerRuntime.Test.ValidateSummaryBeforeUpload": [true, false]
 					}
 				}
@@ -71,7 +68,6 @@
 			"optionOverrides": {
 				"routerlicious": {
 					"configurations": {
-						"Fluid.ContainerRuntime.Test.SummaryStateUpdateMethodV2": ["restart", "default"],
 						"Fluid.ContainerRuntime.Test.ValidateSummaryBeforeUpload": [true, false]
 					}
 				}
@@ -101,7 +97,6 @@
 			"optionOverrides": {
 				"odsp": {
 					"configurations": {
-						"Fluid.ContainerRuntime.Test.SummaryStateUpdateMethodV2": ["restart", "default"],
 						"Fluid.ContainerRuntime.Test.ValidateSummaryBeforeUpload": [true, false]
 					}
 				}