Skip to content

Commit

Permalink
[GC] Fix GC cross compat tests failing for ODSP and AFR (microsoft#21443
Browse files Browse the repository at this point in the history
)

## Issue and root-cause

The test "GC summary compatibility tests" has been failing consistently
for ODSP and AFR. The test fails because it does the following:
1. Summarizer 1 generates summary at say seq#100.
2. Summarizer 2 loads and assumes that it will load from summary
seq#100.

This works fine for local server because it will return the above
summary as latest when summarizer 2 loads. However, ODSP and AFR cache
snapshots. So, summarizer 2 actually gets the previous cached summary
which is older than seq#100. Summarizer 2 then receives the ack from
summary seq#100 and it shuts down because its newer than the summary it
knows about. The summary generation from summarizer 2 fails and the test
fails.

## Fix
The fix is that summarizer 2 explcitly loads from the summary generated
by summarizer 1 via the id of the summary.

For ODSP, there is an additional fix required for back-compatibility due
to single-commit summaries. Loaders version 1.x do not support single
commit summaries so when the test runs in this compat mode, ODSP nacks
all summaries. Added logic to skip the tests for loader versions 1.x.




[AB#7948](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/7948)
  • Loading branch information
agarwal-navin authored Jun 14, 2024
1 parent 72011f4 commit bdcb3e4
Showing 1 changed file with 20 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
ITestObjectProvider,
createContainerRuntimeFactoryWithDefaultDataStore,
createSummarizerCore,
getContainerEntryPointBackCompat,
summarizeNow,
waitForContainerConnection,
} from "@fluidframework/test-utils/internal";
Expand Down Expand Up @@ -104,27 +105,24 @@ describeCompat(

beforeEach("setupContainer", async function () {
provider = getTestObjectProvider({ syncSummarizer: true });
// These tests are failing for ODSP and FRS. Disabling them for now.
if (provider.driver.type !== "local") {
this.skip();

// ODSP only supports single commit summaries now. Loaders running 1.x didn't have single commit summaries
// and only supported two commit summaries. If this test runs with 1.x loaders, summaries fail because ODSP
// nacks them. So, skip the test for those combinations.
if (provider.driver.type === "odsp") {
const loaderVersion = compatApis.loader.version;
const loaderVersionForLoading = compatApis.loaderForLoading?.version;
if (loaderVersion.startsWith("1.") || loaderVersionForLoading?.startsWith("1.")) {
this.skip();
}
}

mainContainer = await createContainer(version1Apis);
if (mainContainer.getEntryPoint !== undefined) {
dataStoreA = (await mainContainer.getEntryPoint()) as ITestFluidObject;
} else {
// Back-compat: versions of container-loader before 2.0.0-internal.3.3.0 don't have a getEntryPoint API.
const result = await (mainContainer as any).request({ url: "/" });
assert.equal(
result.status,
200,
`Request failed: ${result.value}\n${result.stack}`,
);
dataStoreA = result.value;
}
dataStoreA = await getContainerEntryPointBackCompat<ITestFluidObject>(mainContainer);
await waitForContainerConnection(mainContainer);
});

async function createSummarizer(apis: LayerApis) {
async function createSummarizer(apis: LayerApis, summaryVersion?: string) {
const runtimeFactory = await createRuntimeFactory(apis, "summarizer");
const loader = provider.createLoader(
[[provider.defaultCodeDetails, runtimeFactory]],
Expand All @@ -137,7 +135,11 @@ describeCompat(
// we need to specify this explicitly rather than rely on default behavior.
apis.loader.version === version1Apis.loader.version,
);
const { summarizer } = await createSummarizerCore(mainContainer, loader);
const { summarizer } = await createSummarizerCore(
mainContainer,
loader,
summaryVersion,
);
return summarizer;
}

Expand All @@ -161,13 +163,6 @@ describeCompat(
* be read by older / newer versions of the container runtime.
*/
it("load version validates unreferenced timestamp from summary by create version", async function () {
// TODO: This test is consistently failing when ran against FRS. See ADO:7865
if (
provider.driver.type === "routerlicious" &&
provider.driver.endpointName === "frs"
) {
this.skip();
}
// Create a new summarizer running version 1 runtime. This client will generate a summary which will be used to load
// a new client using the runtime factory version 2.
const summarizer1 = await createSummarizer(version1Apis);
Expand Down Expand Up @@ -214,9 +209,8 @@ describeCompat(

// Create a new summarizer running version 2 from the summary generated by the client running version 1.
summarizer1.close();
const summarizer2 = await createSummarizer(version2Apis);
const summarizer2 = await createSummarizer(version2Apis, summaryResult2.summaryVersion);

await provider.ensureSynchronized();
// `getUnreferencedTimestamps` assumes that the GC result isn't incremental.
// Passing fullTree explicitly ensures that.
const summaryResult3 = await summarizeNow(summarizer2, {
Expand Down

0 comments on commit bdcb3e4

Please sign in to comment.