Handle detached blobs more similarly to other blobs (take 2)

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

@@ -3,11 +3,7 @@
  * Licensed under the MIT License.
  */
 
-import {
-	AttachState,
-	type IContainerContext,
-} from "@fluidframework/container-definitions/internal";
-import { assert } from "@fluidframework/core-utils/internal";
+import type { IContainerContext } from "@fluidframework/container-definitions/internal";
 import { readAndParse } from "@fluidframework/driver-utils/internal";
 import type { ISummaryTreeWithStats } from "@fluidframework/runtime-definitions/internal";
 import { SummaryTreeBuilder } from "@fluidframework/runtime-utils/internal";
@@ -60,72 +56,54 @@ const loadV1 = async (
 export const toRedirectTable = (
 	blobManagerLoadInfo: IBlobManagerLoadInfo,
 	logger: ITelemetryLoggerExt,
-	attachState: AttachState,
-): Map<string, string | undefined> => {
+): Map<string, string> => {
 	logger.sendTelemetryEvent({
 		eventName: "AttachmentBlobsLoaded",
 		count: blobManagerLoadInfo.ids?.length ?? 0,
 		redirectTable: blobManagerLoadInfo.redirectTable?.length,
 	});
-	const redirectTable = new Map<string, string | undefined>(blobManagerLoadInfo.redirectTable);
-	const detached = attachState !== AttachState.Attached;
-	if (blobManagerLoadInfo.ids) {
-		// If we are detached, we don't have storage IDs yet, so set to undefined
-		// Otherwise, set identity (id -> id) entries.
-		for (const entry of blobManagerLoadInfo.ids) {
-			redirectTable.set(entry, detached ? undefined : entry);
+	const redirectTable = new Map<string, string>(blobManagerLoadInfo.redirectTable);
+	if (blobManagerLoadInfo.ids !== undefined) {
+		for (const storageId of blobManagerLoadInfo.ids) {
+			// Older versions of the runtime used the storage ID directly in the handle,
+			// rather than routing through the redirectTable. To support old handles that
+			// were created in this way but unify handling through the redirectTable, we
+			// add identity mappings to the redirect table at load. These identity entries
+			// will be excluded during summarization.
+			redirectTable.set(storageId, storageId);
 		}
 	}
 	return redirectTable;
 };
 
 export const summarizeBlobManagerState = (
-	redirectTable: Map<string, string | undefined>,
-	attachState: AttachState,
-): ISummaryTreeWithStats => summarizeV1(redirectTable, attachState);
+	redirectTable: Map<string, string>,
+): ISummaryTreeWithStats => summarizeV1(redirectTable);
 
-const summarizeV1 = (
-	redirectTable: Map<string, string | undefined>,
-	attachState: AttachState,
-): ISummaryTreeWithStats => {
-	const storageIds = getStorageIds(redirectTable, attachState);
-
-	// if storageIds is empty, it means we are detached and have only local IDs, or that there are no blobs attached
-	const blobIds = storageIds.size > 0 ? [...storageIds] : [...redirectTable.keys()];
+const summarizeV1 = (redirectTable: Map<string, string>): ISummaryTreeWithStats => {
 	const builder = new SummaryTreeBuilder();
-	for (const blobId of blobIds) {
-		builder.addAttachment(blobId);
+	const storageIds = getStorageIds(redirectTable);
+	for (const storageId of storageIds) {
+		// The Attachment is inspectable by storage, which lets it detect that the blob is referenced
+		// and therefore should not be GC'd.
+		builder.addAttachment(storageId);
 	}
 
-	// Any non-identity entries in the table need to be saved in the summary
-	if (redirectTable.size > blobIds.length) {
-		builder.addBlob(
-			redirectTableBlobName,
-			// filter out identity entries
-			JSON.stringify(
-				[...redirectTable.entries()].filter(([localId, storageId]) => localId !== storageId),
-			),
-		);
+	// Exclude identity mappings from the redirectTable summary. Note that
+	// the storageIds of the identity mappings are still included in the Attachments
+	// above, so we expect these identity mappings will be recreated at load
+	// time in toRedirectTable even if there is no non-identity mapping in
+	// the redirectTable.
+	const nonIdentityRedirectTableEntries = [...redirectTable.entries()].filter(
+		([localId, storageId]) => localId !== storageId,
+	);
+	if (nonIdentityRedirectTableEntries.length > 0) {
+		builder.addBlob(redirectTableBlobName, JSON.stringify(nonIdentityRedirectTableEntries));
 	}
 
 	return builder.getSummaryTree();
 };
 
-export const getStorageIds = (
-	redirectTable: Map<string, string | undefined>,
-	attachState: AttachState,
-): Set<string> => {
-	const ids = new Set<string | undefined>(redirectTable.values());
-
-	// If we are detached, we will not have storage IDs, only undefined
-	const undefinedValueInTable = ids.delete(undefined);
-
-	// For a detached container, entries are inserted into the redirect table with an undefined storage ID.
-	// For an attached container, entries are inserted w/storage ID after the BlobAttach op round-trips.
-	assert(
-		!undefinedValueInTable || (attachState === AttachState.Detached && ids.size === 0),
-		0x382 /* 'redirectTable' must contain only undefined while detached / defined values while attached */,
-	);
-
-	return ids as Set<string>;
+export const getStorageIds = (redirectTable: Map<string, string>): Set<string> => {
+	return new Set<string>(redirectTable.values());
 };

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

@@ -3708,7 +3708,7 @@ export class ContainerRuntime
 		telemetryContext?: ITelemetryContext,
 	): ISummaryTree {
 		if (blobRedirectTable) {
-			this.blobManager.setRedirectTable(blobRedirectTable);
+			this.blobManager.patchRedirectTable(blobRedirectTable);
 		}
 
 		// We can finalize any allocated IDs since we're the only client

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

@@ -78,7 +78,7 @@ describe("BlobHandles", () => {
 				d.resolve();
 			},
 			stashedBlobs: {},
-			localBlobIdGenerator: () => "localId",
+			localIdGenerator: () => "localId",
 			isBlobDeleted: () => false,
 			storage: failProxy<IRuntimeStorageService>({
 				createBlob: async () => {
@@ -118,7 +118,7 @@ describe("BlobHandles", () => {
 				d.resolve();
 			},
 			stashedBlobs: {},
-			localBlobIdGenerator: () => "localId",
+			localIdGenerator: () => "localId",
 			storage: failProxy<IRuntimeStorageService>({
 				createBlob: async () => {
 					count++;

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

@@ -250,7 +250,7 @@ export class MockRuntime
 				table.set(detachedId, id);
 			}
 			this.detachedStorage.blobs.clear();
-			this.blobManager.setRedirectTable(table);
+			this.blobManager.patchRedirectTable(table);
 		}
 		const summary = validateSummary(this);
 		this.attachState = AttachState.Attached;
@@ -460,7 +460,7 @@ for (const createBlobPayloadPending of [false, true]) {
 
 			const summaryData = validateSummary(runtime);
 			assert.strictEqual(summaryData.ids.length, 1);
-			assert.strictEqual(summaryData.redirectTable, undefined);
+			assert.strictEqual(summaryData.redirectTable?.length, 1);
 		});
 
 		it("detached->attached snapshot", async () => {
@@ -704,7 +704,7 @@ for (const createBlobPayloadPending of [false, true]) {
 
 			const summaryData = validateSummary(runtime);
 			assert.strictEqual(summaryData.ids.length, 1);
-			assert.strictEqual(summaryData.redirectTable, undefined);
+			assert.strictEqual(summaryData.redirectTable?.length, 2);
 		});
 
 		it("handles deduped IDs in detached->attached", async () => {
@@ -729,7 +729,7 @@ for (const createBlobPayloadPending of [false, true]) {
 
 			const summaryData = validateSummary(runtime);
 			assert.strictEqual(summaryData.ids.length, 1);
-			assert.strictEqual(summaryData.redirectTable?.length, 4);
+			assert.strictEqual(summaryData.redirectTable?.length, 6);
 		});
 
 		it("can load from summary", async () => {
@@ -756,6 +756,45 @@ for (const createBlobPayloadPending of [false, true]) {
 			assert.strictEqual(summaryData2.redirectTable?.length, 3);
 		});
 
+		it("can get blobs by requesting their storage ID", async () => {
+			await runtime.attach();
+			await runtime.connect();
+
+			const handle = runtime.createBlob(IsoBuffer.from("blob", "utf8"));
+			await runtime.processAll();
+
+			await assert.doesNotReject(handle);
+
+			// Using the summary as a simple way to grab the storage ID of the blob we just created
+			const {
+				redirectTable,
+				ids: [storageId],
+			} = validateSummary(runtime);
+			assert.strictEqual(redirectTable?.length, 1);
+			assert.strictEqual(typeof storageId, "string", "Expect storage ID to be in the summary");
+
+			const blob = await runtime.blobManager.getBlob(storageId, createBlobPayloadPending);
+
+			const runtime2 = new MockRuntime(
+				mc,
+				createBlobPayloadPending,
+				// Loading a second runtime with just the blob attachments and no redirect table
+				// lets us verify that we still correctly reconstruct the identity mapping during load.
+				{ ids: [storageId] },
+				true,
+			);
+			(runtime2.storage as unknown as BaseMockBlobStorage).blobs.set(storageId, blob);
+			await assert.doesNotReject(
+				runtime2.blobManager.getBlob(storageId, createBlobPayloadPending),
+			);
+			const {
+				redirectTable: redirectTable2,
+				ids: [storageId2],
+			} = validateSummary(runtime2);
+			assert.strictEqual(redirectTable2, undefined);
+			assert.strictEqual(storageId2, storageId, "Expect storage ID to be in the summary");
+		});
+
 		it("handles duplicate remote upload", async () => {
 			await runtime.attach();
 			await runtime.connect();
@@ -831,9 +870,10 @@ for (const createBlobPayloadPending of [false, true]) {
 		});
 
 		it("runtime disposed during readBlob - log no error", async () => {
-			const someId = "someId";
+			const someLocalId = "someLocalId";
+			const someStorageId = "someStorageId";
 			// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-call -- Accessing private property
-			(runtime.blobManager as any).setRedirection(someId, undefined); // To appease an assert
+			(runtime.blobManager as any).setRedirection(someLocalId, someStorageId); // To appease an assert
 
 			// Mock storage.readBlob to dispose the runtime and throw an error
 			Sinon.stub(runtime.storage, "readBlob").callsFake(async (_id: string) => {
@@ -842,7 +882,7 @@ for (const createBlobPayloadPending of [false, true]) {
 			});
 
 			await assert.rejects(
-				async () => runtime.blobManager.getBlob(someId, false),
+				async () => runtime.blobManager.getBlob(someLocalId, false),
 				(e: Error) => e.message === "BOOM!",
 				"Expected getBlob to throw with test error message",
 			);
@@ -1107,7 +1147,7 @@ for (const createBlobPayloadPending of [false, true]) {
 		});
 
 		describe("Garbage Collection", () => {
-			let redirectTable: Map<string, string | undefined>;
+			let redirectTable: Map<string, string>;
 
 			/**
 			 * Creates a blob with the given content and returns its local and storage id.

packages/test/test-end-to-end-tests/src/test/blobs.spec.ts

@@ -396,7 +396,7 @@ for (const createBlobPayloadPending of [undefined, true] as const) {
 				const container2 = await provider.loadTestContainer(testContainerConfig);
 				const snapshot2 = (container2 as any).runtime.blobManager.summarize();
 				assert.strictEqual(snapshot2.stats.treeNodeCount, 1);
-				assert.strictEqual(snapshot1.summary.tree[0].id, snapshot2.summary.tree[0].id);
+				assert.deepStrictEqual(snapshot1.summary.tree, snapshot2.summary.tree);
 			});
 
 			// regression test for https://github.com/microsoft/FluidFramework/issues/9702

packages/test/test-end-to-end-tests/src/test/gc/gcSweepAttachmentBlobs.spec.ts

@@ -67,7 +67,7 @@ function validateBlobStateInSummary(
 	expectDelete: boolean,
 	expectGCStateHandle: boolean,
 ) {
-	const shouldShouldNot = expectDelete ? "should" : "should not";
+	const shouldShouldNot = expectDelete ? "should not" : "should";
 
 	// Validate that the blob tree should not be in the summary since there should be no attachment blobs.
 	const blobsTree = summaryTree.tree[blobsTreeName] as ISummaryTree;