Skip to content

Commit

Permalink
[Port v2int/3.4] Remove SweepReadyUsageDetectionHandler in garbage co…
Browse files Browse the repository at this point in the history 
…llector (#14766)

Port from v2int/3.0 -
#14760

SweepReadyUsageDetectionHandler access localStorage which can fail if
access to localStorage is not allowed. We are seeing this happening in
production.
Removing the usage of SweepReadyUsageDetectionHandler in garbage
collector - It was added by
#12004 but the sweep
ready detection feature was never enabled.


[AB#3793](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/3793)
  • Loading branch information
@agarwal-navin
agarwal-navin authored Mar 24, 2023
1 parent 859c1b5 commit 12a2a05
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 148 deletions.
20 changes: 0 additions & 20 deletions packages/runtime/container-runtime/src/gc/garbageCollection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ import {
} from "./gcDefinitions";
import { getSnapshotDataFromOldSnapshotFormat, sendGCUnexpectedUsageEvent } from "./gcHelpers";
import { GCSummaryStateTracker } from "./gcSummaryStateTracker";
import { SweepReadyUsageDetectionHandler } from "./gcSweepReadyUsageDetection";
import { UnreferencedStateTracker } from "./gcUnreferencedStateTracker";

/** The event that is logged when unreferenced node is used after a certain time. */
Expand Down Expand Up @@ -156,9 +155,6 @@ export class GarbageCollector implements IGarbageCollector {
return this.summaryStateTracker.doesSummaryStateNeedReset;
}

/** Handler to respond to when a SweepReady object is used */
private readonly sweepReadyUsageHandler: SweepReadyUsageDetectionHandler;

protected constructor(createParams: IGarbageCollectorCreateParams) {
this.runtime = createParams.runtime;
this.isSummarizerClient = createParams.isSummarizerClient;
Expand All @@ -176,12 +172,6 @@ export class GarbageCollector implements IGarbageCollector {
}),
);

this.sweepReadyUsageHandler = new SweepReadyUsageDetectionHandler(
createParams.getContainerDiagnosticId(),
this.mc,
this.runtime.closeFn,
);

this.configs = generateGCConfigs(this.mc, createParams);

// If session expiry is enabled, we need to close the container when the session expiry timeout expires.
Expand Down Expand Up @@ -1203,16 +1193,6 @@ export class GarbageCollector implements IGarbageCollector {
this.mc.logger.sendErrorEvent(event);
}
}

// If SweepReady Usage Detection is enabled, the handler may close the interactive container.
// Once Sweep is fully implemented, this will be removed since the objects will be gone
// and errors will arise elsewhere in the runtime
if (state === UnreferencedState.SweepReady) {
this.sweepReadyUsageHandler.usageDetectedInInteractiveClient({
...propsToLog,
usageType,
});
}
}
}

Expand Down
128 changes: 0 additions & 128 deletions packages/runtime/container-runtime/src/test/gc/garbageCollection.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ export const configProvider = (settings: Record<string, ConfigTypes>): IConfigPr
getRawConfig: (name: string): ConfigTypes => settings[name],
});

/** @see - sweepReadyUsageDetectionSetting */
const SweepReadyUsageDetectionKey = "Fluid.GarbageCollection.Dogfood.SweepReadyUsageDetection";

type GcWithPrivates = IGarbageCollector & {
readonly configs: IGarbageCollectorConfigs;
readonly summaryStateTracker: Omit<GCSummaryStateTracker, "latestSummaryGCVersion"> & {
Expand Down Expand Up @@ -706,131 +703,6 @@ describe("Garbage Collection Tests", () => {
);
});

describe("Interactive Client Behavior", () => {
function updateAllNodes(garbageCollector) {
nodes.forEach((nodeId) => {
garbageCollector.nodeUpdated(nodeId, "Changed", Date.now(), testPkgPath);
garbageCollector.nodeUpdated(nodeId, "Loaded", Date.now(), testPkgPath);
});
}

async function interactiveClientTestCode(
timeout: number,
loadedEventName: string,
sweepReadyUsageErrorExpected: boolean,
) {
let lastCloseErrorType: string = "N/A";

// Create GC state where node 3's unreferenced time was > timeout ms ago.
// This is important since we shouldn't run GC on the interactive container,
// but rather load from a snapshot in which SweepReady state is already reached.

// Create a snapshot tree to be used as the GC snapshot tree.
const gcSnapshotTree = getDummySnapshotTree();
const gcBlobId = "root";
// Add a GC blob with key that start with `gcBlobPrefix` to the GC snapshot tree. The blob Id for this
// is generated by server in real scenarios but we use a static id here for testing.
gcSnapshotTree.blobs[`${gcBlobPrefix}_${gcBlobId}`] = gcBlobId;

// Create a base snapshot that contains the GC snapshot tree.
const baseSnapshot = getDummySnapshotTree();
baseSnapshot.trees[gcTreeKey] = gcSnapshotTree;

// Create GC state with node 3 expired. This will be returned when the garbage collector asks
// for the GC blob with `gcBlobId`.
const gcState: IGarbageCollectionState = { gcNodes: {} };
const unrefTime = Date.now() - (timeout + 100);
const node3Data: IGarbageCollectionNodeData = {
outboundRoutes: [],
unreferencedTimestampMs: unrefTime,
};
gcState.gcNodes[nodes[3]] = node3Data;

const gcBlobMap: Map<string, IGarbageCollectionState> = new Map([
[gcBlobId, gcState],
]);
const garbageCollector = createGarbageCollector(
{ baseSnapshot },
gcBlobMap,
(error) => {
lastCloseErrorType = error?.errorType ?? "NONE";
},
false /* isSummarizerClient */,
);

// Trigger loading GC state from base snapshot - but don't call GC since that's not what happens in real flow
await (garbageCollector as any).initializeGCStateFromBaseSnapshotP;

// Update nodes and validate that all events for node 3 are logged.
updateAllNodes(garbageCollector);
assert(
!mockLogger.events.some(
(event) =>
event.eventName !== loadedEventName && event.unrefTime === unrefTime,
),
"shouldn't see any unreference events besides Loaded",
);
mockLogger.assertMatch(
[
{
eventName: loadedEventName,
timeout,
id: nodes[3],
pkg: eventPkg,
unrefTime,
},
],
"all events not generated as expected",
);

const expectedErrorType = sweepReadyUsageErrorExpected
? "unreferencedObjectUsedAfterGarbageCollected"
: "N/A";
assert.equal(
lastCloseErrorType,
expectedErrorType,
"Incorrect lastCloseReason after using unreferenced nodes",
);
}

it("Inactive object used - generates events but does not close container (SweepReadyUsageDetection enabled)", async () => {
const inactiveTimeoutMs = 400;
injectedSettings["Fluid.GarbageCollection.TestOverride.InactiveTimeoutMs"] =
inactiveTimeoutMs;
injectedSettings[SweepReadyUsageDetectionKey] = "interactiveClient";

await interactiveClientTestCode(
inactiveTimeoutMs,
"GarbageCollector:InactiveObject_Loaded",
false,
);
});

it("SweepReady object used - generates events and closes container (SweepReadyUsageDetection enabled)", async () => {
const sweepTimeoutMs =
defaultSessionExpiryDurationMs + defaultSnapshotCacheExpiryMs + oneDayMs;
injectedSettings[SweepReadyUsageDetectionKey] = "interactiveClient";

await interactiveClientTestCode(
sweepTimeoutMs,
"GarbageCollector:SweepReadyObject_Loaded",
true,
);
});

it("SweepReady object used - generates events but does not close container (SweepReadyUsageDetection disabled)", async () => {
const sweepTimeoutMs =
defaultSessionExpiryDurationMs + defaultSnapshotCacheExpiryMs + oneDayMs;
injectedSettings[SweepReadyUsageDetectionKey] = "something else";

await interactiveClientTestCode(
sweepTimeoutMs,
"GarbageCollector:SweepReadyObject_Loaded",
false,
);
});
});

it("generates both inactive and sweep ready events when nodes are used after time out", async () => {
const inactiveTimeoutMs = 500;
const sweepTimeoutMs =
Expand Down

0 comments on commit 12a2a05

Please sign in to comment.