[GC] Make getAttachGCData required on IFluidDataStoreChannel and make…

… implementation consistent (#21347)

`getAttachGCData` was added to `IFluidDataStoreChannel` in 2.0.0-rc.2.0
4 months ago via #19275. This PR does the following:
- Makes `getAttachGCData` required on `IFluidDataStoreChannel`.
- Makes `getAttachGCData` logic consistent across channel collection,
data store context, data store runtime and channel contexts. I found it
confusing to follow the existing flow due to the implementation being
different for various layers. This is how it works now:
- `ChannelCollection` and `FluidDataStoreRuntime` both implements
`getAttachGCData` which iterates through all it's children and gets
their GC data by calling `getAttachGCData`.
- `FluidDataStoreContext` and `LocalChannelContext` both implement
`getAttachGCData` which returns the GC data for this node.
- Decouples `getAttachSummary` and `getAttachGCData` in
`ChannelCollection:generateAttachMessage`.


[AB#8199](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8199)

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

@@ -43,11 +43,13 @@ import {
 	NamedFluidDataStoreRegistryEntries,
 	channelsTreeName,
 	IInboundSignalMessage,
+	gcDataBlobKey,
 } from "@fluidframework/runtime-definitions/internal";
 import {
 	GCDataBuilder,
 	RequestParser,
 	SummaryTreeBuilder,
+	addBlobToSummary,
 	convertSnapshotTreeToSummaryTree,
 	convertSummaryTreeToITree,
 	create404Response,
@@ -241,6 +243,13 @@ export function wrapContextForInnerChannel(
 	return context;
 }
 
+/**
+ * Returns the type of the given local data store from its package path.
+ */
+export function getLocalDataStoreType(localDataStore: LocalFluidDataStoreContext) {
+	return localDataStore.packagePath[localDataStore.packagePath.length - 1];
+}
+
 /**
  * This class encapsulates data store handling. Currently it is only used by the container runtime,
  * but eventually could be hosted on any channel once we formalize the channel api boundary.
@@ -525,17 +534,21 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable {
 	}
 
 	/** Package up the context's attach summary etc into an IAttachMessage */
-	private generateAttachMessage(localContext: IFluidDataStoreContextInternal): IAttachMessage {
-		const { attachSummary } = localContext.getAttachData(/* includeGCData: */ true);
-		const type = localContext.packagePath[localContext.packagePath.length - 1];
+	private generateAttachMessage(localContext: LocalFluidDataStoreContext): IAttachMessage {
+		// Get the attach summary.
+		const attachSummary = localContext.getAttachSummary();
+
+		// Get the GC data and add it to the attach summary.
+		const attachGCData = localContext.getAttachGCData();
+		addBlobToSummary(attachSummary, gcDataBlobKey, JSON.stringify(attachGCData));
 
 		// Attach message needs the summary in ITree format. Convert the ISummaryTree into an ITree.
 		const snapshot = convertSummaryTreeToITree(attachSummary.summary);
 
 		return {
 			id: localContext.id,
 			snapshot,
-			type,
+			type: getLocalDataStoreType(localContext),
 		} satisfies IAttachMessage;
 	}
 
@@ -1126,10 +1139,7 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable {
 				.map(([key, value]) => {
 					let dataStoreSummary: ISummarizeResult;
 					if (value.isLoaded) {
-						dataStoreSummary = value.getAttachData(
-							/* includeGCCData: */ false,
-							telemetryContext,
-						).attachSummary;
+						dataStoreSummary = value.getAttachSummary(telemetryContext);
 					} else {
 						// If this data store is not yet loaded, then there should be no changes in the snapshot from
 						// which it was created as it is detached container. So just use the previous snapshot.
@@ -1148,6 +1158,36 @@ export class ChannelCollection implements IFluidDataStoreChannel, IDisposable {
 		return builder.getSummaryTree();
 	}
 
+	/**
+	 * Gets the GC data. Used when attaching or serializing a detached container.
+	 */
+	public getAttachGCData(telemetryContext?: ITelemetryContext): IGarbageCollectionData {
+		const builder = new GCDataBuilder();
+		// Attaching graph of some stores can cause other stores to get bound too.
+		// So keep taking summary until no new stores get bound.
+		let notBoundContextsLength: number;
+		do {
+			notBoundContextsLength = this.contexts.notBoundLength();
+			// Iterate over each data store and ask for its GC data.
+			Array.from(this.contexts)
+				.filter(
+					([key, _]) =>
+						// Take GC data of bounded data stores only.
+						!this.contexts.isNotBound(key),
+				)
+				.map(([key, value]) => {
+					const contextGCData = value.getAttachGCData(telemetryContext);
+					// Prefix the child's id to the ids of its GC nodes so they can be identified as belonging to the child.
+					// This also gradually builds the id of each node to be a path from the root.
+					builder.prefixAndAddNodes(key, contextGCData.gcNodes);
+				});
+		} while (notBoundContextsLength !== this.contexts.notBoundLength());
+
+		// Get the outbound routes (aliased data stores) and add a GC node for this channel.
+		builder.addNode("/", Array.from(this.aliasedDataStores));
+		return builder.getGCData();
+	}
+
 	/**
 	 * Generates data used for garbage collection. It does the following:
 	 *

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

@@ -52,7 +52,6 @@ import {
 	ISummarizerNodeWithGC,
 	SummarizeInternalFn,
 	channelsTreeName,
-	gcDataBlobKey,
 	IInboundSignalMessage,
 } from "@fluidframework/runtime-definitions/internal";
 import {
@@ -115,13 +114,9 @@ export interface ISnapshotDetails {
  * @internal
  */
 export interface IFluidDataStoreContextInternal extends IFluidDataStoreContext {
-	getAttachData(
-		includeGCData: boolean,
-		telemetryContext?: ITelemetryContext,
-	): {
-		attachSummary: ISummaryTreeWithStats;
-		type: string;
-	};
+	getAttachSummary(telemetryContext?: ITelemetryContext): ISummaryTreeWithStats;
+
+	getAttachGCData(telemetryContext?: ITelemetryContext): IGarbageCollectionData;
 
 	getInitialSnapshotDetails(): Promise<ISnapshotDetails>;
 
@@ -893,18 +888,16 @@ export abstract class FluidDataStoreContext
 	}
 
 	/**
-	 * Get the data required when attaching this context's DataStore.
+	 * Get the summary required when attaching this context's DataStore.
 	 * Used for both Container Attach and DataStore Attach.
-	 *
-	 * @returns the summary, type, and GC Data for this context's DataStore.
 	 */
-	public abstract getAttachData(
-		includeGCData: boolean,
-		telemetryContext?: ITelemetryContext,
-	): {
-		attachSummary: ISummaryTreeWithStats;
-		type: string;
-	};
+	public abstract getAttachSummary(telemetryContext?: ITelemetryContext): ISummaryTreeWithStats;
+
+	/**
+	 * Get the GC Data for the initial state being attached so remote clients can learn of this DataStore's
+	 * outbound routes.
+	 */
+	public abstract getAttachGCData(telemetryContext?: ITelemetryContext): IGarbageCollectionData;
 
 	public abstract getInitialSnapshotDetails(): Promise<ISnapshotDetails>;
 
@@ -1164,12 +1157,16 @@ export class RemoteFluidDataStoreContext extends FluidDataStoreContext {
 	}
 
 	/**
-	 * @see FluidDataStoreContext.getAttachData
+	 * @see FluidDataStoreContext.getAttachSummary
+	 */
+	public getAttachSummary(): ISummaryTreeWithStats {
+		throw new Error("Cannot attach remote store");
+	}
+
+	/**
+	 * @see FluidDataStoreContext.getAttachGCData
 	 */
-	public getAttachData(includeGCData: boolean): {
-		attachSummary: ISummaryTreeWithStats;
-		type: string;
-	} {
+	public getAttachGCData(telemetryContext?: ITelemetryContext): IGarbageCollectionData {
 		throw new Error("Cannot attach remote store");
 	}
 }
@@ -1245,15 +1242,9 @@ export class LocalFluidDataStoreContextBase extends FluidDataStoreContext {
 	}
 
 	/**
-	 * @see FluidDataStoreContext.getAttachData
+	 * @see FluidDataStoreContext.getAttachSummary
 	 */
-	public getAttachData(
-		includeGCData: boolean,
-		telemetryContext?: ITelemetryContext,
-	): {
-		attachSummary: ISummaryTreeWithStats;
-		type: string;
-	} {
+	public getAttachSummary(telemetryContext?: ITelemetryContext): ISummaryTreeWithStats {
 		assert(
 			this.channel !== undefined,
 			0x14f /* "There should be a channel when generating attach message" */,
@@ -1271,22 +1262,24 @@ export class LocalFluidDataStoreContextBase extends FluidDataStoreContext {
 		// Add data store's attributes to the summary.
 		const attributes = createAttributes(this.pkg, this.isInMemoryRoot());
 		addBlobToSummary(attachSummary, dataStoreAttributesBlobName, JSON.stringify(attributes));
-		if (includeGCData) {
-			const gcData = this.channel.getAttachGCData?.(telemetryContext);
-			if (gcData !== undefined) {
-				addBlobToSummary(attachSummary, gcDataBlobKey, JSON.stringify(gcData));
-			}
-		}
 
 		// Add loadingGroupId to the summary
 		if (this.loadingGroupId !== undefined) {
 			attachSummary.summary.groupId = this.loadingGroupId;
 		}
 
-		return {
-			attachSummary,
-			type: this.pkg[this.pkg.length - 1],
-		};
+		return attachSummary;
+	}
+
+	/**
+	 * @see FluidDataStoreContext.getAttachGCData
+	 */
+	public getAttachGCData(telemetryContext?: ITelemetryContext): IGarbageCollectionData {
+		assert(
+			this.channel !== undefined,
+			"There should be a channel when generating attach GC data",
+		);
+		return this.channel.getAttachGCData(telemetryContext);
 	}
 
 	private readonly initialSnapshotDetailsP = new LazyPromise<ISnapshotDetails>(async () => {

packages/runtime/container-runtime/src/test/dataStoreContext.spec.ts

@@ -48,7 +48,11 @@ import {
 	validateAssertionError,
 } from "@fluidframework/test-runtime-utils/internal";
 
-import { ChannelCollection, wrapContextForInnerChannel } from "../channelCollection.js";
+import {
+	ChannelCollection,
+	getLocalDataStoreType,
+	wrapContextForInnerChannel,
+} from "../channelCollection.js";
 import { ContainerRuntime } from "../containerRuntime.js";
 import { channelToDataStore } from "../dataStore.js";
 import {
@@ -206,11 +210,8 @@ describe("Data Store Context Tests", () => {
 				});
 
 				await localDataStoreContext.realize();
-				const {
-					attachSummary: { summary },
-					type,
-				} = localDataStoreContext.getAttachData(/* includeGCData: */ false);
-				const snapshot = convertSummaryTreeToITree(summary);
+				const attachSummary = localDataStoreContext.getAttachSummary();
+				const snapshot = convertSummaryTreeToITree(attachSummary.summary);
 
 				const attributesEntry = snapshot.entries.find(
 					(e) => e.path === dataStoreAttributesBlobName,
@@ -244,7 +245,11 @@ describe("Data Store Context Tests", () => {
 					dataStoreAttributes.isRootDataStore,
 					"Local DataStore root state does not match",
 				);
-				assert.strictEqual(type, "TestDataStore1", "Attach message type does not match.");
+				assert.strictEqual(
+					getLocalDataStoreType(localDataStoreContext),
+					"TestDataStore1",
+					"Attach message type does not match.",
+				);
 			});
 
 			it("should generate exception when incorrectly created with array of packages", async () => {
@@ -295,11 +300,8 @@ describe("Data Store Context Tests", () => {
 
 				await localDataStoreContext.realize();
 
-				const {
-					attachSummary: { summary },
-					type,
-				} = localDataStoreContext.getAttachData(/* includeGCData: */ false);
-				const snapshot = convertSummaryTreeToITree(summary);
+				const attachSummary = localDataStoreContext.getAttachSummary();
+				const snapshot = convertSummaryTreeToITree(attachSummary.summary);
 				const attributesEntry = snapshot.entries.find(
 					(e) => e.path === dataStoreAttributesBlobName,
 				);
@@ -331,7 +333,11 @@ describe("Data Store Context Tests", () => {
 					dataStoreAttributes.isRootDataStore,
 					"Local DataStore root state does not match",
 				);
-				assert.strictEqual(type, "SubComp", "Attach message type does not match.");
+				assert.strictEqual(
+					getLocalDataStoreType(localDataStoreContext),
+					"SubComp",
+					"Attach message type does not match.",
+				);
 			});
 
 			it("can correctly initialize non-root context", async () => {

packages/runtime/runtime-definitions/api-report/runtime-definitions.alpha.api.md

@@ -157,7 +157,7 @@ export interface IFluidDataStoreChannel extends IDisposable {
     // (undocumented)
     applyStashedOp(content: any): Promise<unknown>;
     readonly entryPoint: IFluidHandleInternal<FluidObject>;
-    getAttachGCData?(telemetryContext?: ITelemetryContext): IGarbageCollectionData;
+    getAttachGCData(telemetryContext?: ITelemetryContext): IGarbageCollectionData;
     getAttachSummary(telemetryContext?: ITelemetryContext): ISummaryTreeWithStats;
     getGCData(fullGC?: boolean): Promise<IGarbageCollectionData>;
     makeVisibleAndAttachGraph(): void;

packages/runtime/runtime-definitions/package.json

@@ -119,6 +119,9 @@
 			"RemovedInterfaceDeclaration_IFluidDataStoreContextEvents": {
 				"forwardCompat": false,
 				"backCompat": false
+			},
+			"InterfaceDeclaration_IFluidDataStoreChannel": {
+				"forwardCompat": false
 			}
 		}
 	}

packages/runtime/runtime-definitions/src/dataStoreContext.ts

@@ -292,7 +292,7 @@ export interface IFluidDataStoreChannel extends IDisposable {
 	/**
 	 * Synchronously retrieves GC Data (representing the outbound routes present) for the initial state of the DataStore
 	 */
-	getAttachGCData?(telemetryContext?: ITelemetryContext): IGarbageCollectionData;
+	getAttachGCData(telemetryContext?: ITelemetryContext): IGarbageCollectionData;
 
 	/**
 	 * Processes the op.

packages/runtime/runtime-definitions/src/test/types/validateRuntimeDefinitionsPrevious.generated.ts

@@ -503,6 +503,7 @@ declare function get_old_InterfaceDeclaration_IFluidDataStoreChannel():
 declare function use_current_InterfaceDeclaration_IFluidDataStoreChannel(
     use: TypeOnly<current.IFluidDataStoreChannel>): void;
 use_current_InterfaceDeclaration_IFluidDataStoreChannel(
+    // @ts-expect-error compatibility expected to be broken
     get_old_InterfaceDeclaration_IFluidDataStoreChannel());
 
 /*

packages/runtime/runtime-utils/src/summaryUtils.ts

@@ -30,6 +30,7 @@ import {
 	IGarbageCollectionData,
 	ISummarizeResult,
 	ITelemetryContextExt,
+	gcDataBlobKey,
 } from "@fluidframework/runtime-definitions/internal";
 import type { TelemetryEventPropertyTypeExt } from "@fluidframework/telemetry-utils/internal";
 
@@ -386,7 +387,7 @@ export function processAttachMessageGCData(
 	snapshot: ITree | null,
 	addedGCOutboundRoute: (fromNodeId: string, toPath: string) => void,
 ): boolean {
-	const gcDataEntry = snapshot?.entries.find((e) => e.path === ".gcdata");
+	const gcDataEntry = snapshot?.entries.find((e) => e.path === gcDataBlobKey);
 
 	// Old attach messages won't have GC Data
 	// (And REALLY old DataStore Attach messages won't even have a snapshot!)

packages/runtime/test-runtime-utils/api-report/test-runtime-utils.alpha.api.md

@@ -54,6 +54,7 @@ import { ISnapshotTree } from '@fluidframework/driver-definitions/internal';
 import { ISummaryTree } from '@fluidframework/driver-definitions';
 import { ISummaryTreeWithStats } from '@fluidframework/runtime-definitions/internal';
 import { ITelemetryBaseLogger } from '@fluidframework/core-interfaces';
+import { ITelemetryContext } from '@fluidframework/runtime-definitions/internal';
 import { ITelemetryLoggerExt } from '@fluidframework/telemetry-utils/internal';
 import { ITokenProvider } from '@fluidframework/routerlicious-driver';
 import { ITokenResponse } from '@fluidframework/routerlicious-driver';
@@ -470,6 +471,8 @@ export class MockFluidDataStoreRuntime extends EventEmitter implements IFluidDat
     // (undocumented)
     readonly existing: boolean;
     // (undocumented)
+    getAttachGCData(telemetryContext?: ITelemetryContext | undefined): IGarbageCollectionData;
+    // (undocumented)
     getAttachSnapshot(): ITreeEntry[];
     // (undocumented)
     getAttachSummary(): ISummaryTreeWithStats;

packages/runtime/test-runtime-utils/api-report/test-runtime-utils.beta.api.md

@@ -54,6 +54,7 @@ import { ISnapshotTree } from '@fluidframework/driver-definitions/internal';
 import { ISummaryTree } from '@fluidframework/driver-definitions';
 import { ISummaryTreeWithStats } from '@fluidframework/runtime-definitions/internal';
 import { ITelemetryBaseLogger } from '@fluidframework/core-interfaces';
+import { ITelemetryContext } from '@fluidframework/runtime-definitions/internal';
 import { ITelemetryLoggerExt } from '@fluidframework/telemetry-utils/internal';
 import { ITokenProvider } from '@fluidframework/routerlicious-driver';
 import { ITokenResponse } from '@fluidframework/routerlicious-driver';

packages/runtime/test-runtime-utils/api-report/test-runtime-utils.public.api.md

@@ -54,6 +54,7 @@ import { ISnapshotTree } from '@fluidframework/driver-definitions/internal';
 import { ISummaryTree } from '@fluidframework/driver-definitions';
 import { ISummaryTreeWithStats } from '@fluidframework/runtime-definitions/internal';
 import { ITelemetryBaseLogger } from '@fluidframework/core-interfaces';
+import { ITelemetryContext } from '@fluidframework/runtime-definitions/internal';
 import { ITelemetryLoggerExt } from '@fluidframework/telemetry-utils/internal';
 import { ITokenProvider } from '@fluidframework/routerlicious-driver';
 import { ITokenResponse } from '@fluidframework/routerlicious-driver';