[v2int/7.4] GC: Follow-up changes to support successful roll-out of GC Sweep (#19598)

This brings over some GC fixes and minor features requested by a 1P
partner who is preparing to ship GC Sweep:
* Some telemetry fixes that will help us to monitor the accuracy and
impact of GC better
* A new option to allow enabling sleep only for attachment blobs at
first

Porting the following commits to the `release/v2int/7.4` branch to be
released in internal.7.4.7:

* 0d05377 - previously ported but
backed out to support 1P partner's livesite, contains the following
commits from `main`:
  * 40ad2d4
  * a61a831
  * d1d5c58
* bc11aa4

Related port PRs for other releases:
* #19087
* #19591
* #19589

Co-authored-by: Navin Agarwal <[email protected]>

Author: markfields

packages/runtime/container-runtime/src/blobManager.ts

@@ -39,7 +39,6 @@ import {
 } from "@fluidframework/runtime-definitions";
 
 import { canRetryOnError, runWithRetry } from "@fluidframework/driver-utils";
-import { disableAttachmentBlobSweepKey } from "./gc";
 import { IBlobMetadata } from "./metadata";
 
 /**
@@ -740,11 +739,6 @@ export class BlobManager extends TypedEventEmitter<IBlobManagerEvents> {
 	 * @returns The routes of blobs that were deleted.
 	 */
 	public deleteSweepReadyNodes(sweepReadyBlobRoutes: readonly string[]): readonly string[] {
-		// If sweep for attachment blobs is not enabled, return empty list indicating nothing is deleted.
-		if (this.mc.config.getBoolean(disableAttachmentBlobSweepKey) === true) {
-			return [];
-		}
-
 		this.deleteBlobsFromRedirectTable(sweepReadyBlobRoutes);
 		return Array.from(sweepReadyBlobRoutes);
 	}

packages/runtime/container-runtime/src/containerRuntime.ts

@@ -1449,9 +1449,6 @@ export class ContainerRuntime
 			getNodePackagePath: async (nodePath: string) => this.getGCNodePackagePath(nodePath),
 			getLastSummaryTimestampMs: () => this.messageAtLastSummary?.timestamp,
 			readAndParseBlob: async <T>(id: string) => readAndParse<T>(this.storage, id),
-			// GC runs in summarizer client and needs access to the real (non-proxy) active information. The proxy
-			// delta manager would always return false for summarizer client.
-			activeConnection: () => this.innerDeltaManager.active,
 			submitMessage: (message: ContainerRuntimeGCMessage) => this.submit(message),
 		});
 

packages/runtime/container-runtime/src/dataStores.ts

@@ -60,8 +60,8 @@ import {
 	LocalDetachedFluidDataStoreContext,
 } from "./dataStoreContext";
 import { StorageServiceWithAttachBlobs } from "./storageServiceWithAttachBlobs";
+import { GCNodeType } from "./gc";
 import { IDataStoreAliasMessage, isDataStoreAliasMessage } from "./dataStore";
-import { GCNodeType, disableDatastoreSweepKey } from "./gc";
 import { IContainerRuntimeMetadata, nonDataStorePaths, rootHasIsolatedChannels } from "./summary";
 
 type PendingAliasResolve = (success: boolean) => void;
@@ -830,16 +830,13 @@ export class DataStores implements IDisposable {
 	 * @returns The routes of data stores and its objects that were deleted.
 	 */
 	public deleteSweepReadyNodes(sweepReadyDataStoreRoutes: readonly string[]): readonly string[] {
-		// If sweep for data stores is not enabled, return empty list indicating nothing is deleted.
-		if (this.mc.config.getBoolean(disableDatastoreSweepKey) === true) {
-			return [];
-		}
 		for (const route of sweepReadyDataStoreRoutes) {
 			const pathParts = route.split("/");
 			const dataStoreId = pathParts[1];
 
 			// Ignore sub-data store routes because a data store and its sub-routes are deleted together, so, we only
 			// need to delete the data store.
+			// These routes will still be returned below as among the deleted routes
 			if (pathParts.length > 2) {
 				continue;
 			}

packages/runtime/container-runtime/src/gc/garbageCollection.ts

@@ -31,6 +31,8 @@ import {
 	gcDisableThrowOnTombstoneLoadOptionName,
 	defaultSweepGracePeriodMs,
 	gcGenerationOptionName,
+	disableDatastoreSweepKey,
+	gcDisableDataStoreSweepOptionName,
 } from "./gcDefinitions";
 import { getGCVersion, shouldAllowGcSweep } from "./gcHelpers";
 
@@ -133,16 +135,24 @@ export function generateGCConfigs(
 	 *
 	 * Assuming overall GC is enabled and sweepTimeout is provided, the following conditions have to be met to run sweep:
 	 *
-	 * 1. Sweep should be enabled for this container.
-	 * 2. Sweep should be enabled for this session.
+	 * 1. Sweep should be allowed in this container.
+	 * 2. Sweep should be enabled for this session, optionally restricted to attachment blobs only.
 	 *
 	 * These conditions can be overridden via the RunSweep feature flag.
 	 */
-	const shouldRunSweep =
+	const sweepEnabled: boolean =
 		!shouldRunGC || sweepTimeoutMs === undefined
 			? false
 			: mc.config.getBoolean(runSweepKey) ??
 			  (sweepAllowed && createParams.gcOptions.enableGCSweep === true);
+	const disableDatastoreSweep =
+		mc.config.getBoolean(disableDatastoreSweepKey) === true ||
+		createParams.gcOptions[gcDisableDataStoreSweepOptionName] === true;
+	const shouldRunSweep: IGarbageCollectorConfigs["shouldRunSweep"] = sweepEnabled
+		? disableDatastoreSweep
+			? "ONLY_BLOBS"
+			: "YES"
+		: "NO";
 
 	// Override inactive timeout if test config or gc options to override it is set.
 	const inactiveTimeoutMs =

packages/runtime/container-runtime/src/gc/gcConfigs.ts

@@ -40,6 +40,12 @@ export const nextGCVersion: GCVersion = 4;
  */
 export const gcDisableThrowOnTombstoneLoadOptionName = "gcDisableThrowOnTombstoneLoad";
 
+/**
+ * This undocumented GC Option (on ContainerRuntime Options) allows an app to enable Sweep for blobs only.
+ * Only applies if enableGCSweep option is set to true.
+ */
+export const gcDisableDataStoreSweepOptionName = "disableDataStoreSweep";
+
 /**
  * This undocumented GC Option (on ContainerRuntime Options) allows configuring which documents can have Sweep enabled.
  * This provides a way to disable both Tombstone Enforcement and Sweep.
@@ -70,10 +76,8 @@ export const throwOnTombstoneLoadOverrideKey =
 export const throwOnTombstoneUsageKey = "Fluid.GarbageCollection.ThrowOnTombstoneUsage";
 /** Config key to enable GC version upgrade. */
 export const gcVersionUpgradeToV4Key = "Fluid.GarbageCollection.GCVersionUpgradeToV4";
-/** Config key to disable GC sweep for datastores. */
+/** Config key to disable GC sweep for datastores. They'll merely be Tombstoned. */
 export const disableDatastoreSweepKey = "Fluid.GarbageCollection.DisableDataStoreSweep";
-/** Config key to disable GC sweep for attachment blobs. */
-export const disableAttachmentBlobSweepKey = "Fluid.GarbageCollection.DisableAttachmentBlobSweep";
 
 // One day in milliseconds.
 export const oneDayMs = 1 * 24 * 60 * 60 * 1000;
@@ -353,7 +357,6 @@ export interface IGarbageCollectorCreateParams {
 	readonly getNodePackagePath: (nodePath: string) => Promise<readonly string[] | undefined>;
 	readonly getLastSummaryTimestampMs: () => number | undefined;
 	readonly readAndParseBlob: ReadAndParseBlob;
-	readonly activeConnection: () => boolean;
 	readonly submitMessage: (message: ContainerRuntimeGCMessage) => void;
 }
 
@@ -426,7 +429,7 @@ export interface IGarbageCollectorConfigs {
 	 */
 	readonly gcEnabled: boolean;
 	/**
-	 * Tracks if sweep phase is enabled for this document. This is specified during document creation and doesn't change
+	 * Tracks if sweep phase is allowed for this document. This is specified during document creation and doesn't change
 	 * throughout its lifetime.
 	 */
 	readonly sweepEnabled: boolean;
@@ -436,10 +439,11 @@ export interface IGarbageCollectorConfigs {
 	 */
 	readonly shouldRunGC: boolean;
 	/**
-	 * Tracks if sweep phase should run or not. Even if the sweep phase is enabled for a document (see sweepEnabled), it
-	 * can be explicitly disabled via feature flags. It also won't run if session expiry is not enabled.
+	 * Tracks if sweep phase should run or not, or if it should run only for attachment blobs.
+	 * Even if the sweep phase is allowed for a document (see sweepEnabled), it may be disabled or partially enabled
+	 * for the session, depending on a variety of other configurations present.
 	 */
-	readonly shouldRunSweep: boolean;
+	readonly shouldRunSweep: "YES" | "ONLY_BLOBS" | "NO";
 	/**
 	 * If true, bypass optimizations and generate GC data for all nodes irrespective of whether a node changed or not.
 	 */

packages/runtime/container-runtime/src/gc/gcDefinitions.ts

@@ -111,7 +111,11 @@ export class GCSummaryStateTracker {
 	/**
 	 * Called during GC initialization. Initialize the latest summary data from the base snapshot data.
 	 */
-	public initializeBaseState(baseSnapshotData: IGarbageCollectionSnapshotData) {
+	public initializeBaseState(baseSnapshotData: IGarbageCollectionSnapshotData | undefined) {
+		if (baseSnapshotData === undefined) {
+			return;
+		}
+
 		// If tracking state across summaries, update latest summary data from the snapshot's GC data.
 		this.latestSummaryData = {
 			serializedGCState: baseSnapshotData.gcState

packages/runtime/container-runtime/src/gc/gcSummaryStateTracker.ts

@@ -240,13 +240,9 @@ export class GCTelemetryTracker {
 					gcConfigs,
 				};
 
-				// Do not log the inactive object x events as error events as they are not the best signal for
-				// detecting something wrong with GC either from the partner or from the runtime itself.
-				if (state === UnreferencedState.Inactive) {
-					this.mc.logger.sendTelemetryEvent(event);
-				} else {
-					this.mc.logger.sendErrorEvent(event);
-				}
+				// These are logged as generic events and not errors because there can be false positives. The Tombstone
+				// and Delete errors are separately logged and are reliable.
+				this.mc.logger.sendTelemetryEvent(event);
 			}
 		}
 	}
@@ -393,12 +389,7 @@ export class GCTelemetryTracker {
 						fromPkg: fromPkg?.join("/"),
 					}),
 				};
-
-				if (state === UnreferencedState.Inactive) {
-					logger.sendTelemetryEvent(event);
-				} else {
-					logger.sendErrorEvent(event);
-				}
+				logger.sendTelemetryEvent(event);
 			}
 		}
 		this.pendingEventsQueue = [];

packages/runtime/container-runtime/src/gc/gcTelemetry.ts

@@ -11,6 +11,7 @@ export {
 	defaultSessionExpiryDurationMs,
 	GCNodeType,
 	gcTestModeKey,
+	gcDisableDataStoreSweepOptionName,
 	gcDisableThrowOnTombstoneLoadOptionName,
 	gcGenerationOptionName,
 	GCFeatureMatrix,
@@ -31,7 +32,6 @@ export {
 	runSessionExpiryKey,
 	runSweepKey,
 	stableGCVersion,
-	disableAttachmentBlobSweepKey,
 	disableDatastoreSweepKey,
 	UnreferencedState,
 	throwOnTombstoneLoadOverrideKey,

packages/runtime/container-runtime/src/gc/index.ts

@@ -31,7 +31,6 @@ import {
 	LoggingError,
 } from "@fluidframework/telemetry-utils";
 import { BlobManager, IBlobManagerLoadInfo, IBlobManagerRuntime } from "../blobManager";
-import { disableAttachmentBlobSweepKey } from "../gc";
 
 const MIN_TTL = 24 * 60 * 60; // same as ODSP
 abstract class BaseMockBlobStorage
@@ -941,47 +940,33 @@ describe("BlobManager", () => {
 			);
 		});
 
-		it("disableAttachmentBlobsSweep true - DOESN'T delete unused blobs ", async () => {
-			injectedSettings[disableAttachmentBlobSweepKey] = true;
-
-			await runtime.attach();
-			await runtime.connect();
-
-			const blob1 = await createBlobAndGetIds("blob1");
-			const blob2 = await createBlobAndGetIds("blob2");
-
-			// Delete blob1's local id. The local id and the storage id should both be deleted from the redirect table
-			// since the blob only had one reference.
-			runtime.blobManager.deleteSweepReadyNodes([blob1.localGCNodeId]);
-			assert(redirectTable.has(blob1.localId));
-			assert(redirectTable.has(blob1.storageId));
-
-			// Delete blob2's local id. The local id and the storage id should both be deleted from the redirect table
-			// since the blob only had one reference.
-			runtime.blobManager.deleteSweepReadyNodes([blob2.localGCNodeId]);
-			assert(redirectTable.has(blob2.localId));
-			assert(redirectTable.has(blob2.storageId));
-		});
-
-		it("deletes unused blobs", async () => {
-			await runtime.attach();
-			await runtime.connect();
-
-			const blob1 = await createBlobAndGetIds("blob1");
-			const blob2 = await createBlobAndGetIds("blob2");
-
-			// Delete blob1's local id. The local id and the storage id should both be deleted from the redirect table
-			// since the blob only had one reference.
-			runtime.blobManager.deleteSweepReadyNodes([blob1.localGCNodeId]);
-			assert(!redirectTable.has(blob1.localId));
-			assert(!redirectTable.has(blob1.storageId));
-
-			// Delete blob2's local id. The local id and the storage id should both be deleted from the redirect table
-			// since the blob only had one reference.
-			runtime.blobManager.deleteSweepReadyNodes([blob2.localGCNodeId]);
-			assert(!redirectTable.has(blob2.localId));
-			assert(!redirectTable.has(blob2.storageId));
-		});
+		// Support for this config has been removed.
+		const legacyKey_disableAttachmentBlobSweep =
+			"Fluid.GarbageCollection.DisableAttachmentBlobSweep";
+		[true, undefined].forEach((disableAttachmentBlobsSweep) =>
+			it(`deletes unused blobs regardless of DisableAttachmentBlobsSweep setting [DisableAttachmentBlobsSweep=${disableAttachmentBlobsSweep}]`, async () => {
+				injectedSettings[legacyKey_disableAttachmentBlobSweep] =
+					disableAttachmentBlobsSweep;
+
+				await runtime.attach();
+				await runtime.connect();
+
+				const blob1 = await createBlobAndGetIds("blob1");
+				const blob2 = await createBlobAndGetIds("blob2");
+
+				// Delete blob1's local id. The local id and the storage id should both be deleted from the redirect table
+				// since the blob only had one reference.
+				runtime.blobManager.deleteSweepReadyNodes([blob1.localGCNodeId]);
+				assert(!redirectTable.has(blob1.localId));
+				assert(!redirectTable.has(blob1.storageId));
+
+				// Delete blob2's local id. The local id and the storage id should both be deleted from the redirect table
+				// since the blob only had one reference.
+				runtime.blobManager.deleteSweepReadyNodes([blob2.localGCNodeId]);
+				assert(!redirectTable.has(blob2.localId));
+				assert(!redirectTable.has(blob2.storageId));
+			}),
+		);
 
 		it("deletes unused de-duped blobs", async () => {
 			await runtime.attach();