Simplfied SummarizerNode:refreshLatestSummary logic (#17106)

## Description
#16657 removed the logic to refresh a summary's state by downloading a
snapshot. With it gone, summarizer node (and rest of the system) only
updates state from a summary that it was tracking. This PR simplifies
the logic in `SummarizerNode::refreshLatestSummary` to account for this.
It also renames the properties in the returned object to give the caller
more context on what happened and what action it can take.

Additionally, it removes logging the `PendingSummaryNotFound` telemetry.
It is currently logged every time the summarizer loaded for the summary
that is loaded from. The reason is that it processes the ack for the
summary is loaded from and since the summary was not generated by this
instance of the summarizer, this event is logged. Instead, added a
property `pendingSummaryTracked` to the refreshLatestSummary_end event.


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

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

@@ -3651,19 +3651,21 @@ export class ContainerRuntime
 	/** Implementation of ISummarizerInternalsProvider.refreshLatestSummaryAck */
 	public async refreshLatestSummaryAck(options: IRefreshSummaryAckOptions) {
 		const { proposalHandle, ackHandle, summaryRefSeq, summaryLogger } = options;
+		// proposalHandle is always passed from RunningSummarizer.
+		assert(proposalHandle !== undefined, "proposalHandle should be available");
 		const readAndParseBlob = async <T>(id: string) => readAndParse<T>(this.storage, id);
 		const result = await this.summarizerNode.refreshLatestSummary(
 			proposalHandle,
 			summaryRefSeq,
 		);
 
 		/**
-		 * When refreshing a summary ack, this check indicates a new ack of a summary that was newer than the
+		 * When refreshing a summary ack, this check indicates a new ack of a summary that is newer than the
 		 * current summary that is tracked, but this summarizer runtime did not produce/track that summary. Thus
 		 * it needs to refresh its state. Today refresh is done by fetching the latest snapshot to update the cache
 		 * and then close as the current main client is likely to be re-elected as the parent summarizer again.
 		 */
-		if (result.latestSummaryUpdated && !result.wasSummaryTracked) {
+		if (!result.isSummaryTracked && result.isSummaryNewer) {
 			const fetchResult = await this.fetchSnapshotFromStorage(
 				summaryLogger,
 				{

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

@@ -29,7 +29,7 @@ import {
 	RuntimeHeaders,
 } from "../containerRuntime";
 import { ClientSessionExpiredError } from "../error";
-import { RefreshSummaryResult } from "../summary";
+import { IRefreshSummaryResult } from "../summary";
 import { generateGCConfigs } from "./gcConfigs";
 import {
 	GCNodeType,
@@ -843,10 +843,9 @@ export class GarbageCollector implements IGarbageCollector {
 	}
 
 	/**
-	 * Called to refresh the latest summary state. This happens when either a pending summary is acked or a snapshot
-	 * is downloaded and should be used to update the state.
+	 * Called to refresh the latest summary state. This happens when either a pending summary is acked.
 	 */
-	public async refreshLatestSummary(result: RefreshSummaryResult): Promise<void> {
+	public async refreshLatestSummary(result: IRefreshSummaryResult): Promise<void> {
 		return this.summaryStateTracker.refreshLatestSummary(result);
 	}
 

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

@@ -17,7 +17,7 @@ import { ITelemetryLoggerExt } from "@fluidframework/telemetry-utils";
 import {
 	IContainerRuntimeMetadata,
 	ICreateContainerMetadata,
-	RefreshSummaryResult,
+	IRefreshSummaryResult,
 } from "../summary";
 
 export type GCVersion = number;
@@ -228,7 +228,7 @@ export interface IGarbageCollector {
 	/** Returns the GC details generated from the base snapshot. */
 	getBaseGCDetails(): Promise<IGarbageCollectionDetailsBase>;
 	/** Called when the latest summary of the system has been refreshed. */
-	refreshLatestSummary(result: RefreshSummaryResult): Promise<void>;
+	refreshLatestSummary(result: IRefreshSummaryResult): Promise<void>;
 	/** Called when a node is updated. Used to detect and log when an inactive node is changed or loaded. */
 	nodeUpdated(
 		nodePath: string,

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

@@ -13,11 +13,10 @@ import {
 	ISummaryTreeWithStats,
 } from "@fluidframework/runtime-definitions";
 import { mergeStats, SummaryTreeBuilder } from "@fluidframework/runtime-utils";
-import { RefreshSummaryResult } from "../summary";
-import { GCVersion, IGCStats } from "./gcDefinitions";
+import { IRefreshSummaryResult } from "../summary";
+import { GCVersion, IGarbageCollectorConfigs, IGCStats } from "./gcDefinitions";
 import { generateSortedGCState } from "./gcHelpers";
 import { IGarbageCollectionSnapshotData, IGarbageCollectionState } from "./gcSummaryDefinitions";
-import { IGarbageCollectorConfigs } from ".";
 
 export const gcStateBlobKey = `${gcBlobPrefix}_root`;
 
@@ -264,31 +263,27 @@ export class GCSummaryStateTracker {
 	}
 
 	/**
-	 * Called to refresh the latest summary state. This happens when either a pending summary is acked or a snapshot
-	 * is downloaded and should be used to update the state.
+	 * Called to refresh the latest summary state. This happens when either a pending summary is acked.
 	 */
-	public async refreshLatestSummary(result: RefreshSummaryResult): Promise<void> {
-		// If the latest summary was updated and the summary was tracked, this client is the one that generated this
-		// summary. So, update wasGCRunInLatestSummary.
-		// Note that this has to be updated if GC did not run too. Otherwise, `gcStateNeedsReset` will always return
-		// true in scenarios where GC is disabled but enabled in the snapshot we loaded from.
-		if (result.latestSummaryUpdated && result.wasSummaryTracked) {
-			this.wasGCRunInLatestSummary = this.configs.shouldRunGC;
-		}
-
-		if (!result.latestSummaryUpdated || !this.configs.shouldRunGC) {
+	public async refreshLatestSummary(result: IRefreshSummaryResult): Promise<void> {
+		if (!result.isSummaryTracked) {
 			return;
 		}
 
-		// If the summary was tracked by this client, it was the one that generated the summary in the first place.
-		// Update latest state from pending.
-		if (result.wasSummaryTracked) {
-			this.latestSummaryGCVersion = this.configs.gcVersionInEffect;
-			this.latestSummaryData = this.pendingSummaryData;
-			this.pendingSummaryData = undefined;
-			this.updatedDSCountSinceLastSummary = 0;
+		// If the summary is tracked, this client is the one that generated it. So, update wasGCRunInLatestSummary.
+		// Note that this has to be updated if GC did not run too. Otherwise, `gcStateNeedsReset` will always return
+		// true in scenarios where GC is currently disabled but enabled in the snapshot we loaded from.
+		this.wasGCRunInLatestSummary = this.configs.shouldRunGC;
+
+		if (!this.configs.shouldRunGC) {
 			return;
 		}
+
+		this.latestSummaryGCVersion = this.configs.gcVersionInEffect;
+		this.latestSummaryData = this.pendingSummaryData;
+		this.pendingSummaryData = undefined;
+		this.updatedDSCountSinceLastSummary = 0;
+		return;
 	}
 
 	/**

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

@@ -28,10 +28,9 @@ export { SummarizeHeuristicData, SummarizeHeuristicRunner } from "./summarizerHe
 export {
 	createRootSummarizerNode,
 	createRootSummarizerNodeWithGC,
-	IFetchSnapshotResult,
+	IRefreshSummaryResult,
 	IRootSummarizerNode,
 	IRootSummarizerNodeWithGC,
-	RefreshSummaryResult,
 } from "./summarizerNode";
 export {
 	IConnectableRuntime,

packages/runtime/container-runtime/src/summary/summarizerNode/index.ts

@@ -4,9 +4,8 @@
  */
 
 export {
-	IFetchSnapshotResult,
+	IRefreshSummaryResult,
 	ISummarizerNodeRootContract,
-	RefreshSummaryResult,
 	ValidateSummaryResult,
 } from "./summarizerNodeUtils";
 export { IRootSummarizerNode, createRootSummarizerNode } from "./summarizerNode";

packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNode.ts

@@ -35,10 +35,10 @@ import {
 	EscapedPath,
 	ICreateChildDetails,
 	IInitialSummary,
+	IRefreshSummaryResult,
 	ISummarizerNodeRootContract,
 	parseSummaryForSubtrees,
 	parseSummaryTreeForSubtrees,
-	RefreshSummaryResult,
 	SummaryNode,
 	ValidateSummaryResult,
 } from "./summarizerNodeUtils";
@@ -365,27 +365,21 @@ export class SummarizerNode implements IRootSummarizerNode {
 
 	/**
 	 * Refreshes the latest summary tracked by this node. If we have a pending summary for the given proposal handle,
-	 * it becomes the latest summary. If the current summary is already ahead (e.g., loaded from a service summary),
-	 * we skip the update. If the current summary is behind, then we do not refresh.
+	 * it becomes the latest summary. If the current summary is already ahead, we skip the update.
+	 * If the current summary is behind, then we do not refresh.
 	 *
-	 * @returns A RefreshSummaryResult type which returns information based on the following three scenarios:
-	 *
-	 * 1. The latest summary was not updated.
-	 *
-	 * 2. The latest summary was updated and the summary corresponding to the params was being tracked.
-	 *
-	 * 3. The latest summary was updated but the summary corresponding to the params was not tracked.
+	 * @returns true if the summary is tracked by this node, false otherwise.
 	 */
 	public async refreshLatestSummary(
-		proposalHandle: string | undefined,
+		proposalHandle: string,
 		summaryRefSeq: number,
-	): Promise<RefreshSummaryResult> {
+	): Promise<IRefreshSummaryResult> {
 		const eventProps: {
 			proposalHandle: string | undefined;
 			summaryRefSeq: number;
 			referenceSequenceNumber: number;
-			latestSummaryUpdated?: boolean;
-			wasSummaryTracked?: boolean;
+			isSummaryTracked?: boolean;
+			pendingSummaryFound?: boolean;
 		} = {
 			proposalHandle,
 			summaryRefSeq,
@@ -406,48 +400,22 @@ export class SummarizerNode implements IRootSummarizerNode {
 					});
 				}
 
-				if (proposalHandle !== undefined) {
-					const maybeSummaryNode = this.pendingSummaries.get(proposalHandle);
-
-					if (maybeSummaryNode !== undefined) {
-						this.refreshLatestSummaryFromPending(
-							proposalHandle,
-							maybeSummaryNode.referenceSequenceNumber,
-						);
-						eventProps.wasSummaryTracked = true;
-						eventProps.latestSummaryUpdated = true;
-						event.end(eventProps);
-						return {
-							latestSummaryUpdated: true,
-							wasSummaryTracked: true,
-							summaryRefSeq,
-						};
-					}
+				let isSummaryTracked = false;
+				let isSummaryNewer = false;
 
-					const props = {
-						summaryRefSeq,
-						pendingSize: this.pendingSummaries.size ?? undefined,
-					};
-					this.logger.sendTelemetryEvent({
-						eventName: "PendingSummaryNotFound",
-						proposalHandle,
-						referenceSequenceNumber: this.referenceSequenceNumber,
-						details: JSON.stringify(props),
-					});
+				if (summaryRefSeq > this.referenceSequenceNumber) {
+					isSummaryNewer = true;
 				}
-
-				// If the summary for which refresh is called is older than the latest tracked summary, ignore it.
-				if (this.referenceSequenceNumber >= summaryRefSeq) {
-					eventProps.latestSummaryUpdated = false;
-					event.end(eventProps);
-					return { latestSummaryUpdated: false };
+				const maybeSummaryNode = this.pendingSummaries.get(proposalHandle);
+				if (maybeSummaryNode !== undefined) {
+					this.refreshLatestSummaryFromPending(
+						proposalHandle,
+						maybeSummaryNode.referenceSequenceNumber,
+					);
+					isSummaryTracked = true;
 				}
-
-				// Note that we did not track this summary, but that the latest summary was updated.
-				eventProps.latestSummaryUpdated = true;
-				eventProps.wasSummaryTracked = false;
-				event.end(eventProps);
-				return { latestSummaryUpdated: true, wasSummaryTracked: false };
+				event.end({ ...eventProps, isSummaryNewer, pendingSummaryFound: isSummaryTracked });
+				return { isSummaryTracked, isSummaryNewer };
 			},
 			{ start: true, end: true, cancel: "error" },
 		);

packages/runtime/container-runtime/src/summary/summarizerNode/summarizerNodeUtils.ts

@@ -7,36 +7,11 @@ import { ITelemetryLoggerExt, TelemetryDataTag } from "@fluidframework/telemetry
 import { ISnapshotTree, ISummaryTree, SummaryObject } from "@fluidframework/protocol-definitions";
 import { channelsTreeName, ISummaryTreeWithStats } from "@fluidframework/runtime-definitions";
 
-/**
- * Return type of refreshSummaryAck function. There can be three different scenarios based on the passed params:
- *
- * 1. The latest summary was not updated.
- *
- * 2. The latest summary was updated and the summary corresponding to the params was tracked by this client.
- *
- * 3. The latest summary was updated but the summary corresponding to the params was not tracked. The client should
- * close
- */
-export type RefreshSummaryResult =
-	| {
-			latestSummaryUpdated: false;
-	  }
-	| {
-			latestSummaryUpdated: true;
-			wasSummaryTracked: true;
-			summaryRefSeq: number;
-	  }
-	| {
-			latestSummaryUpdated: true;
-			wasSummaryTracked: false;
-	  };
-
-/**
- * Result of snapshot fetch during refreshing latest summary state.
- */
-export interface IFetchSnapshotResult {
-	snapshotTree: ISnapshotTree;
-	snapshotRefSeq: number;
+export interface IRefreshSummaryResult {
+	/** Tells whether this summary is tracked by this client. */
+	isSummaryTracked: boolean;
+	/** Tells whether this summary is newer than the latest one tracked by this client. */
+	isSummaryNewer: boolean;
 }
 
 /**
@@ -67,9 +42,9 @@ export interface ISummarizerNodeRootContract {
 	completeSummary(proposalHandle: string, validate: boolean): void;
 	clearSummary(): void;
 	refreshLatestSummary(
-		proposalHandle: string | undefined,
+		proposalHandle: string,
 		summaryRefSeq: number,
-	): Promise<RefreshSummaryResult>;
+	): Promise<IRefreshSummaryResult>;
 }
 
 /** Path for nodes in a tree with escaped special characters */

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

@@ -53,7 +53,6 @@ import {
 	dataStoreAttributesBlobName,
 	IContainerRuntimeMetadata,
 	metadataBlobName,
-	RefreshSummaryResult,
 } from "../../summary";
 import { pkgVersion } from "../../packageVersion";
 import { configProvider } from "./gcUnitTestHelpers";
@@ -1098,12 +1097,10 @@ describe("Garbage Collection Tests", () => {
 				"Deleted nodes state should be a handle",
 			);
 
-			const refreshSummaryResult: RefreshSummaryResult = {
-				latestSummaryUpdated: true,
-				wasSummaryTracked: true,
-				summaryRefSeq: 0,
-			};
-			await garbageCollector.refreshLatestSummary(refreshSummaryResult);
+			await garbageCollector.refreshLatestSummary({
+				isSummaryTracked: true,
+				isSummaryNewer: true,
+			});
 
 			// Run GC and summarize again. The whole GC summary should now be a summary handle.
 			await garbageCollector.collectGarbage({});
@@ -1698,11 +1695,9 @@ describe("Garbage Collection Tests", () => {
 			checkGCSummaryType(tree1, SummaryType.Tree, "first");
 
 			await garbageCollector.refreshLatestSummary({
-				wasSummaryTracked: true,
-				latestSummaryUpdated: true,
-				summaryRefSeq: 0,
+				isSummaryTracked: true,
+				isSummaryNewer: true,
 			});
-
 			await garbageCollector.collectGarbage({});
 			const tree2 = garbageCollector.summarize(fullTree, trackState);