[GC] Removed throwOnInactiveLoad GC option and the config to disable autorecovery (#22107)

agarwal-navin · web-flow · commit 2e4e9b2cfcdd · 2024-08-02T16:43:05.000-07:00
Removed the following:
- `throwOnInactiveLoad` GC container runtime option and
`throwOnInactiveLoad` internal GC config - These were for a feature to
thrown an error when an inactive data store is loaded. This was an
experimental feature added during sweep development. However, it was
never used.
- `"Fluid.GarbageCollection.DisableAutoRecovery"` feature config and
`tombstoneAutorecoveryEnabled` internal GC config - These were added
while shipping auto recovery feature to turn it off in case it caused
issues. This has been enabled for a while and we want it to always be on
if a tombstone data store is loaded.
diff --git a/.changeset/ready-windows-switch.md b/.changeset/ready-windows-switch.md
@@ -0,0 +1,10 @@
+---
+"@fluidframework/container-runtime": minor
+---
+---
+section: deprecation
+---
+
+InactiveResponseHeaderKey header is deprecated
+
+The header `InactiveResponseHeaderKey` is deprecated and will be removed in the future. It was part of an experimental feature where loading an inactive data store would result in returning a 404 with this header set to true. This feature is no longer supported.
diff --git a/packages/runtime/container-runtime/api-report/container-runtime.legacy.alpha.api.md b/packages/runtime/container-runtime/api-report/container-runtime.legacy.alpha.api.md
@@ -490,7 +490,7 @@ export interface INackSummaryResult {
     readonly summaryNackOp: ISummaryNackMessage;
 }
 
-// @alpha
+// @alpha @deprecated
 export const InactiveResponseHeaderKey = "isInactive";
 
 // @alpha (undocumented)
diff --git a/packages/runtime/container-runtime/package.json b/packages/runtime/container-runtime/package.json
@@ -230,6 +230,10 @@
 		"typescript": "~5.4.5"
 	},
 	"typeValidation": {
-		"broken": {}
+		"broken": {
+			"RemovedVariable_AllowInactiveRequestHeaderKey": {
+				"backCompat": false
+			}
+		}
 	}
 }
diff --git a/packages/runtime/container-runtime/src/channelCollection.ts b/packages/runtime/container-runtime/src/channelCollection.ts
@@ -119,11 +119,6 @@ export enum RuntimeHeaders {
  * @alpha
  */
 export const AllowTombstoneRequestHeaderKey = "allowTombstone"; // Belongs in the enum above, but avoiding the breaking change
-/**
- * [IRRELEVANT IF throwOnInactiveLoad OPTION NOT SET] True if an inactive object should be returned without erroring
- * @internal
- */
-export const AllowInactiveRequestHeaderKey = "allowInactive"; // Belongs in the enum above, but avoiding the breaking change
 
 type PendingAliasResolve = (success: boolean) => void;
 
@@ -1460,14 +1455,10 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable {
 		if (typeof request.headers?.[AllowTombstoneRequestHeaderKey] === "boolean") {
 			headerData.allowTombstone = request.headers[AllowTombstoneRequestHeaderKey];
 		}
-		if (typeof request.headers?.[AllowInactiveRequestHeaderKey] === "boolean") {
-			headerData.allowInactive = request.headers[AllowInactiveRequestHeaderKey];
-		}
 
-		// We allow Tombstone/Inactive requests for sub-DataStore objects
+		// We allow Tombstone requests for sub-DataStore objects
 		if (requestForChild) {
 			headerData.allowTombstone = true;
-			headerData.allowInactive = true;
 		}
 
 		await this.waitIfPendingAlias(id);
diff --git a/packages/runtime/container-runtime/src/containerRuntime.ts b/packages/runtime/container-runtime/src/containerRuntime.ts
@@ -523,6 +523,9 @@ export const TombstoneResponseHeaderKey = "isTombstoned";
  * Inactive error responses will have this header set to true
  * @legacy
  * @alpha
+ *
+ * @deprecated this header is deprecated and will be removed in the future. The functionality corresponding
+ * to this was experimental and is no longer supported.
  */
 export const InactiveResponseHeaderKey = "isInactive";
 
@@ -534,15 +537,13 @@ export interface RuntimeHeaderData {
 	wait?: boolean;
 	viaHandle?: boolean;
 	allowTombstone?: boolean;
-	allowInactive?: boolean;
 }
 
 /** Default values for Runtime Headers */
 export const defaultRuntimeHeaderData: Required<RuntimeHeaderData> = {
 	wait: true,
 	viaHandle: false,
 	allowTombstone: false,
-	allowInactive: false,
 };
 
 /**
diff --git a/packages/runtime/container-runtime/src/gc/garbageCollection.ts b/packages/runtime/container-runtime/src/gc/garbageCollection.ts
@@ -27,7 +27,7 @@ import {
 } from "@fluidframework/telemetry-utils/internal";
 
 import { blobManagerBasePath } from "../blobManager/index.js";
-import { InactiveResponseHeaderKey, TombstoneResponseHeaderKey } from "../containerRuntime.js";
+import { TombstoneResponseHeaderKey } from "../containerRuntime.js";
 import { ClientSessionExpiredError } from "../error.js";
 import { ContainerMessageType, ContainerRuntimeGCMessage } from "../messageTypes.js";
 import { IRefreshSummaryResult } from "../summary/index.js";
@@ -47,7 +47,6 @@ import {
 	IMarkPhaseStats,
 	ISweepPhaseStats,
 	UnreferencedState,
-	disableAutoRecoveryKey,
 	type IGCNodeUpdatedProps,
 } from "./gcDefinitions.js";
 import {
@@ -883,13 +882,6 @@ export class GarbageCollector implements IGarbageCollector {
 				break;
 			}
 			case GarbageCollectionMessageType.TombstoneLoaded: {
-				if (
-					!this.configs.tombstoneAutorecoveryEnabled ||
-					this.mc.config.getBoolean(disableAutoRecoveryKey) === true
-				) {
-					break;
-				}
-
 				// Mark the node as referenced to ensure it isn't Swept
 				const tombstonedNodePath = message.contents.nodePath;
 				this.addedOutboundReference(
@@ -985,8 +977,6 @@ export class GarbageCollector implements IGarbageCollector {
 		// trackedId will be either DataStore or Blob ID (not sub-DataStore ID, since some of those are unrecognized by GC)
 		const trackedId = node.path;
 		const isTombstoned = this.tombstones.includes(trackedId);
-		const isInactive = this.unreferencedNodesState.get(trackedId)?.state === "Inactive";
-
 		const fullPath = request !== undefined ? urlToGCNodePath(request.url) : trackedId;
 
 		// This will log if appropriate
@@ -1037,22 +1027,6 @@ export class GarbageCollector implements IGarbageCollector {
 				errorRequest,
 			);
 		}
-
-		// If the object is inactive and inactive enforcement is configured, throw an error.
-		if (isInactive) {
-			const shouldThrowOnInactiveLoad =
-				!this.isSummarizerClient &&
-				this.configs.throwOnInactiveLoad === true &&
-				headerData?.allowInactive !== true;
-			if (shouldThrowOnInactiveLoad) {
-				throw responseToException(
-					createResponseError(404, `${nodeType} is inactive`, errorRequest, {
-						[InactiveResponseHeaderKey]: true,
-					}),
-					errorRequest,
-				);
-			}
-		}
 	}
 
 	/**
@@ -1065,10 +1039,9 @@ export class GarbageCollector implements IGarbageCollector {
 	 * before runnint GC next.
 	 */
 	private triggerAutoRecovery(nodePath: string) {
-		if (
-			!this.configs.tombstoneAutorecoveryEnabled ||
-			this.mc.config.getBoolean(disableAutoRecoveryKey) === true
-		) {
+		// If sweep isn't enabled, auto-recovery isn't needed since its purpose is to prevent this object from being
+		// deleted. It also would end up sending a GC op which can break clients running FF version 1.x.
+		if (!this.configs.sweepEnabled) {
 			return;
 		}
 
diff --git a/packages/runtime/container-runtime/src/gc/gcConfigs.ts b/packages/runtime/container-runtime/src/gc/gcConfigs.ts
@@ -115,10 +115,6 @@ export function generateGCConfigs(
 			? false
 			: sweepAllowed && createParams.gcOptions.enableGCSweep === true;
 
-	// If we aren't running sweep, also disable AutoRecovery which also emits the GC op.
-	// This gives a simple control surface for compatibility concerns around introducing the new op.
-	const tombstoneAutorecoveryEnabled = sweepEnabled;
-
 	// Override inactive timeout if test config or gc options to override it is set.
 	const inactiveTimeoutMs =
 		mc.config.getNumber("Fluid.GarbageCollection.TestOverride.InactiveTimeoutMs") ??
@@ -141,7 +137,6 @@ export function generateGCConfigs(
 		sweepGracePeriodMs,
 	});
 
-	const throwOnInactiveLoad: boolean | undefined = createParams.gcOptions.throwOnInactiveLoad;
 	const throwOnTombstoneLoad =
 		mc.config.getBoolean(disableThrowOnTombstoneLoadKey) !== true &&
 		sweepEnabled &&
@@ -151,7 +146,6 @@ export function generateGCConfigs(
 		gcAllowed, // For this document
 		sweepAllowed, // For this document
 		sweepEnabled, // For this session
-		tombstoneAutorecoveryEnabled,
 		runFullGC,
 		testMode,
 		sessionExpiryTimeoutMs,
@@ -161,7 +155,6 @@ export function generateGCConfigs(
 		persistedGcFeatureMatrix,
 		gcVersionInBaseSnapshot,
 		gcVersionInEffect: getGCVersionInEffect(mc.config),
-		throwOnInactiveLoad,
 		throwOnTombstoneLoad,
 	};
 }
diff --git a/packages/runtime/container-runtime/src/gc/gcDefinitions.ts b/packages/runtime/container-runtime/src/gc/gcDefinitions.ts
@@ -59,8 +59,6 @@ export const disableThrowOnTombstoneLoadKey =
 	"Fluid.GarbageCollection.DisableThrowOnTombstoneLoad";
 /** Config key to enable GC version upgrade. */
 export const gcVersionUpgradeToV4Key = "Fluid.GarbageCollection.GCVersionUpgradeToV4";
-/** Config key to disable auto-recovery mechanism that protects Tombstones that are loaded from being swept (use true) */
-export const disableAutoRecoveryKey = "Fluid.GarbageCollection.DisableAutoRecovery";
 
 // One day in milliseconds.
 export const oneDayMs = 1 * 24 * 60 * 60 * 1000;
@@ -465,8 +463,6 @@ export interface IGarbageCollectorConfigs {
 	readonly sweepAllowed: boolean;
 	/** Tracks if sweep phase is enabled to run in this session or not */
 	readonly sweepEnabled: boolean;
-	/** Is Tombstone AutoRecovery enabled? Useful for preventing the GC "TombstoneLoaded" op, for compatibility reasons */
-	readonly tombstoneAutorecoveryEnabled: boolean;
 	/**
 	 * If true, bypass optimizations and generate GC data for all nodes irrespective of whether a node changed or not.
 	 */
@@ -490,8 +486,6 @@ export interface IGarbageCollectorConfigs {
 	readonly gcVersionInBaseSnapshot: GCVersion | undefined;
 	/** The current version of GC data in the running code */
 	readonly gcVersionInEffect: GCVersion;
-	/** It is easier for users to diagnose InactiveObject usage if we throw on load, which this option enables */
-	readonly throwOnInactiveLoad: boolean | undefined;
 	/** If true, throw an error when a tombstone data store is retrieved */
 	readonly throwOnTombstoneLoad: boolean;
 }
diff --git a/packages/runtime/container-runtime/src/gc/index.ts b/packages/runtime/container-runtime/src/gc/index.ts
@@ -30,7 +30,6 @@ export {
 	oneDayMs,
 	runSessionExpiryKey,
 	stableGCVersion,
-	disableAutoRecoveryKey,
 	UnreferencedState,
 	disableThrowOnTombstoneLoadKey,
 	GarbageCollectionMessage,
diff --git a/packages/runtime/container-runtime/src/index.ts b/packages/runtime/container-runtime/src/index.ts
@@ -37,7 +37,6 @@ export {
 	RuntimeHeaders,
 	ChannelCollectionFactory,
 	AllowTombstoneRequestHeaderKey,
-	AllowInactiveRequestHeaderKey,
 } from "./channelCollection.js";
 export {
 	GCNodeType,
diff --git a/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts b/packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts
@@ -548,23 +548,6 @@ describe("Garbage Collection Tests", () => {
 				"node 0 should not be unreferenced after repairing GC Data",
 			);
 		});
-
-		it("Autorecovery disabled if enableGCSweep not set", async () => {
-			gc = createGarbageCollector({
-				createParams: { gcOptions: { enableGCSweep: undefined } },
-			});
-			const spies = {
-				gc: {
-					submitMessage: spy(gc, "submitMessage"),
-				},
-			};
-
-			gc.triggerAutoRecovery(""); // nodePath is irrelevant
-			assert(
-				spies.gc.submitMessage.notCalled,
-				"triggerAutoRecovery should no-op if gcOp is not supported in schema",
-			);
-		});
 	});
 
 	describe("errors when unreferenced objects are used after they are inactive / deleted", () => {
diff --git a/packages/runtime/container-runtime/src/test/gc/gcTelemetry.spec.ts b/packages/runtime/container-runtime/src/test/gc/gcTelemetry.spec.ts
@@ -75,15 +75,13 @@ describe("GC Telemetry Tracker", () => {
 			gcAllowed: true,
 			sweepAllowed: false,
 			sweepEnabled: false,
-			tombstoneAutorecoveryEnabled: false,
 			runFullGC: false,
 			testMode: false,
 			inactiveTimeoutMs,
 			sessionExpiryTimeoutMs: defaultSessionExpiryDurationMs,
 			tombstoneTimeoutMs: enableSweep ? tombstoneTimeoutMs : undefined,
 			sweepGracePeriodMs,
 			throwOnTombstoneLoad: false,
-			throwOnInactiveLoad: false,
 			persistedGcFeatureMatrix: undefined,
 			gcVersionInBaseSnapshot: stableGCVersion,
 			gcVersionInEffect: stableGCVersion,
diff --git a/packages/runtime/container-runtime/src/test/types/validateContainerRuntimePrevious.generated.ts b/packages/runtime/container-runtime/src/test/types/validateContainerRuntimePrevious.generated.ts
@@ -20,9 +20,8 @@ declare type MakeUnusedImportErrorsGoAway<T> = TypeOnly<T> | MinimalType<T> | Fu
  * If this test starts failing, it indicates a change that is not backward compatible.
  * To acknowledge the breaking change, add the following to package.json under
  * typeValidation.broken:
- * "Variable_AllowInactiveRequestHeaderKey": {"backCompat": false}
+ * "RemovedVariable_AllowInactiveRequestHeaderKey": {"backCompat": false}
  */
-declare type current_as_old_for_Variable_AllowInactiveRequestHeaderKey = requireAssignableTo<TypeOnly<typeof current.AllowInactiveRequestHeaderKey>, TypeOnly<typeof old.AllowInactiveRequestHeaderKey>>
 
 /*
  * Validate backward compatibility by using the current type in place of the old type.
diff --git a/packages/service-clients/azure-client/src/test/AzureClient.spec.ts b/packages/service-clients/azure-client/src/test/AzureClient.spec.ts
@@ -292,18 +292,17 @@ for (const compatibilityMode of ["1", "2"] as const) {
 				compatibilityMode,
 			);
 			// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-			const { sweepEnabled, tombstoneAutorecoveryEnabled, throwOnTombstoneLoad } =
+			const { sweepEnabled, throwOnTombstoneLoad } =
 				// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
 				(container_defaultConfig as any).container._runtime.garbageCollector.configs;
 
 			const expectedConfigs = {
 				sweepEnabled: false,
-				tombstoneAutorecoveryEnabled: false,
 				throwOnTombstoneLoad: false,
 			};
 			assert.deepStrictEqual(
 				// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-				{ sweepEnabled, tombstoneAutorecoveryEnabled, throwOnTombstoneLoad },
+				{ sweepEnabled, throwOnTombstoneLoad },
 				expectedConfigs,
 				"Expected GC to be disabled per compatibilityModeRuntimeOptions",
 			);
diff --git a/packages/service-clients/odsp-client/src/test/odspClient.spec.ts b/packages/service-clients/odsp-client/src/test/odspClient.spec.ts
@@ -99,18 +99,17 @@ describe("OdspClient", () => {
 	it("GC is disabled by default", async () => {
 		const { container: container_defaultConfig } = await client.createContainer(schema);
 		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-		const { sweepEnabled, tombstoneAutorecoveryEnabled, throwOnTombstoneLoad } =
+		const { sweepEnabled, throwOnTombstoneLoad } =
 			// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
 			(container_defaultConfig as any).container._runtime.garbageCollector.configs;
 
 		const expectedConfigs = {
 			sweepEnabled: false,
-			tombstoneAutorecoveryEnabled: false,
 			throwOnTombstoneLoad: false,
 		};
 		assert.deepStrictEqual(
 			// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-			{ sweepEnabled, tombstoneAutorecoveryEnabled, throwOnTombstoneLoad },
+			{ sweepEnabled, throwOnTombstoneLoad },
 			expectedConfigs,
 			"Expected GC to be disabled per compatibilityModeRuntimeOptions",
 		);
diff --git a/packages/test/test-end-to-end-tests/src/test/gc/gcInactiveNodes.spec.ts b/packages/test/test-end-to-end-tests/src/test/gc/gcInactiveNodes.spec.ts
diff --git a/packages/test/test-end-to-end-tests/src/test/gc/gcTombstoneDataStores.spec.ts b/packages/test/test-end-to-end-tests/src/test/gc/gcTombstoneDataStores.spec.ts

Original file line number	Diff line number	Diff line change
`@@ -490,7 +490,7 @@ export interface INackSummaryResult {`
`490`	`490`	`readonly summaryNackOp: ISummaryNackMessage;`
`491`	`491`	`}`
`492`	`492`
`493`		`-// @alpha`
	`493`	`+// @alpha @deprecated`
`494`	`494`	`export const InactiveResponseHeaderKey = "isInactive";`
`495`	`495`
`496`	`496`	`// @alpha (undocumented)`
Original file line number	Diff line number	Diff line change
`@@ -230,6 +230,10 @@`
`230`	`230`	`"typescript": "~5.4.5"`
`231`	`231`	`},`
`232`	`232`	`"typeValidation": {`
`233`		`- "broken": {}`
	`233`	`+ "broken": {`
	`234`	`+ "RemovedVariable_AllowInactiveRequestHeaderKey": {`
	`235`	`+ "backCompat": false`
	`236`	`+ }`
	`237`	`+ }`
`234`	`238`	`}`
`235`	`239`	`}`