Skip to content

Make internal loadContainerRuntime take in IFluidDataStoreRegistry #25622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+201 −177
Open

Make internal loadContainerRuntime take in IFluidDataStoreRegistry #25622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e79c834
Make internal loadContainerRuntime take in FluidDataStoreRegistry
CraigMacomber Oct 6, 2025
4658473
Update packages/runtime/container-runtime/src/containerRuntime.ts
CraigMacomber Oct 6, 2025
6732268
Better comments
CraigMacomber Oct 6, 2025
2602c80
reintroduce loadRuntime2
CraigMacomber Oct 6, 2025
35c5257
Merge branch 'main' into loadContainerRuntime
CraigMacomber Oct 6, 2025
91b0a8d
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 5 additions & 16 deletions packages/framework/attributor/src/mixinAttributor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,7 @@
* Licensed under the MIT License.
*/

import type { IContainerContext } from "@fluidframework/container-definitions/internal";
import { ContainerRuntime } from "@fluidframework/container-runtime/internal";
import type { IContainerRuntimeOptions } from "@fluidframework/container-runtime/internal";
import type { IContainerRuntime } from "@fluidframework/container-runtime-definitions/internal";
import type { FluidObject } from "@fluidframework/core-interfaces";
import { assert } from "@fluidframework/core-utils/internal";
import type {
Expand Down Expand Up @@ -51,15 +48,9 @@ export const mixinAttributor = (
Base: typeof ContainerRuntime = ContainerRuntime,
): typeof ContainerRuntime =>
class ContainerRuntimeWithAttributor extends Base {
public static async loadRuntime(params: {
context: IContainerContext;
registryEntries: NamedFluidDataStoreRegistryEntries;
existing: boolean;
runtimeOptions?: IContainerRuntimeOptions;
containerScope?: FluidObject;
containerRuntimeCtor?: typeof ContainerRuntime;
provideEntryPoint: (containerRuntime: IContainerRuntime) => Promise<FluidObject>;
}): Promise<ContainerRuntime> {
public static override async loadRuntime(
params: Parameters<typeof Base.loadRuntime>[0],
): Promise<ContainerRuntime> {
const {
context,
registryEntries,
Expand All @@ -83,17 +74,15 @@ export const mixinAttributor = (
(options.attribution ??= {}).track = true;
}

// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
const runtime = (await Base.loadRuntime({
const runtime = await Base.loadRuntime({
context,
registryEntries: registryEntriesCopy,
provideEntryPoint,
runtimeOptions,
containerScope,
existing,
containerRuntimeCtor,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} as any)) as ContainerRuntimeWithAttributor;
});

let runtimeAttributor: IRuntimeAttributor | undefined;
if (shouldTrackAttribution) {
Expand Down
84 changes: 53 additions & 31 deletions packages/runtime/container-runtime/src/containerRuntime.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,7 +737,9 @@ export interface LoadContainerRuntimeParams {
*/
existing: boolean;
/**
* Additional options to be passed to the runtime
* Additional options to be passed to the runtime.
* @remarks
* Defaults to `{}`.
*/
runtimeOptions?: IContainerRuntimeOptions;
/**
Expand Down Expand Up @@ -836,37 +838,59 @@ export class ContainerRuntime
{
/**
* Load the stores from a snapshot and returns the runtime.
* @param params - An object housing the runtime properties:
* - context - Context of the container.
* - registryEntries - Mapping from data store types to their corresponding factories.
* - existing - Pass 'true' if loading from an existing snapshot.
* - requestHandler - (optional) Request handler for the request() method of the container runtime.
* Only relevant for back-compat while we remove the request() method and move fully to entryPoint as the main pattern.
* - runtimeOptions - Additional options to be passed to the runtime
* - containerScope - runtime services provided with context
* - containerRuntimeCtor - Constructor to use to create the ContainerRuntime instance.
* This allows mixin classes to leverage this method to define their own async initializer.
* - provideEntryPoint - Promise that resolves to an object which will act as entryPoint for the Container.
* - minVersionForCollab - Minimum version of the FF runtime that this runtime supports collaboration with.
* This object should provide all the functionality that the Container is expected to provide to the loader layer.
* @param params - An object housing the runtime properties.
* {@link LoadContainerRuntimeParams} except internal, while still having layer compat obligations.
* @privateRemarks
* Despite this being `@internal`, this has layer compat implications so changing it is problematic.
* Instead of changing this, {@link loadRuntime2} was added.
* This is directly invoked by `createTestContainerRuntimeFactory`:
* if that is the only cross version use,
* then `createTestContainerRuntimeFactory` could be updated to handle both versions or use the stable {@link loadContainerRuntime} API,
* and `loadRuntime` could be removed (replaced by `loadRuntime2` which could be renamed back to `loadRuntime`).
*/
public static async loadRuntime(params: {
context: IContainerContext;
registryEntries: NamedFluidDataStoreRegistryEntries;
existing: boolean;
runtimeOptions?: IContainerRuntimeOptionsInternal;
containerScope?: FluidObject;
containerRuntimeCtor?: typeof ContainerRuntime;
/**
* @deprecated Will be removed once Loader LTS version is "2.0.0-internal.7.0.0". Migrate all usage of IFluidRouter to the "entryPoint" pattern. Refer to Removing-IFluidRouter.md
*/
requestHandler?: (request: IRequest, runtime: IContainerRuntime) => Promise<IResponse>;
provideEntryPoint: (containerRuntime: IContainerRuntime) => Promise<FluidObject>;
minVersionForCollab?: MinimumVersionForCollab;
}): Promise<ContainerRuntime> {
public static async loadRuntime(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on our end to end tests, it seems like this internal API surface can't change between versions as that breaks stuff.

It seems bad that we don't do type testing, API reports, or special documentation or tagging on APIs that that might be called with incorrect types due to mixing types from one version of the package with implementations from another.

This seems very hard to maintain, and not type safe, but maybe I'm missing something?

I guess I have to put this back, and add a new API instead, but also not use that new API from places that can cross compat layer boundaries (at least until some point in the future determined by layer compat policy?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, our end-to-end tests should not use internal APIs. If we need access to an API to run such tests, then our customers likely do too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a case of them using our internal APIs, but rather a case of them using our non-internal APIs, and those APIs using our internal ones. At the very least they don't call this API directly.

This is more of a case of our own internal APIs calling each other across a layer boundary where we can load different versions of the code and doing so in such a way that there is no type checking the two sides are compatible at that level.

At least that's my guess at what I think that's what's happening: I don't have a good understanding of the situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a legacy+beta wrapper around this API (a module level function) which I did not change. I hoped leaving that alone would be enough to keep this compatible, but apparently not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm wrong. Looks like packages/test/test-utils/src/testContainerRuntimeFactory.ts calls containerRuntimeCtor.loadRuntime and that's the source of most if not all of the errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea if the issue is that test compat uses it, or if there is a real compat issue.

I think overloads here would be more of a mess as it would require the implementation to support both, and if there is a layer compat use of this API it could end up having all future versions have to support both as well, and the code which overrides (the attribution stuff) this method would get extra complicated as well by having to reimplement support for both.

Also I don't see how merging these into a single implementation would allow us to deprecate the old one. Having one combined method seems like it would make deprecation slightly harder not easier, especially given there is an override of this and potential layer compat implications.

Also if we want the test code to be updated to know about the loadRuntime2, having it have a different name would make it possible to detect at runtime, which would make it easier to migrate to in a compatible way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea if the issue is that test compat uses it, or if there is a real compat issue.

It would be good to get an answer to this before we make any changes here.

Copy link
Contributor

@agarwal-navin agarwal-navin Oct 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding @markfields here since he made this internal fairly recently (< a year ago).

I wonder if we can replace the usages in test to loadContainerRuntime, then this compat problem will go away eventually (once we are past the compat window).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the specific instance that @CraigMacomber mentioned (packages/test/test-utils/src/testContainerRuntimeFactory.ts calls containerRuntimeCtor.loadRuntime), it seems that we explicitly do this for testing compat with v1.X.
@scottn12 is there any way around this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue here is actually not the 1.x clients, since for those clients we fall back to containerRuntimeCtor.load(). I believe the issue is that we also test against 2.x minor releases (i.e. we currently test against 2.53.1), which will try to call loadRuntime() but won't have the updated function if we make changes to it

params: LoadContainerRuntimeParams & {
/**
* Constructor to use to create the ContainerRuntime instance.
* @remarks
* Defaults to {@link ContainerRuntime}.
*/
containerRuntimeCtor?: typeof ContainerRuntime;
},
): Promise<ContainerRuntime> {
return ContainerRuntime.loadRuntime2({
...params,
registry: new FluidDataStoreRegistry(params.registryEntries),
});
}

/**
* Load the stores from a snapshot and returns the runtime.
* @remarks
* Same as {@link ContainerRuntime.loadRuntime},
* but with `registry` instead of `registryEntries` and more `runtimeOptions`.
*/
public static async loadRuntime2(
params: Omit<LoadContainerRuntimeParams, "registryEntries" | "runtimeOptions"> & {
/**
* Mapping from data store types to their corresponding factories.
*/
registry: IFluidDataStoreRegistry;
/**
* Constructor to use to create the ContainerRuntime instance.
* @remarks
* Defaults to {@link ContainerRuntime}.
*/
containerRuntimeCtor?: typeof ContainerRuntime;
/**
* {@link LoadContainerRuntimeParams.runtimeOptions}, except with additional internal only options.
*/
runtimeOptions?: IContainerRuntimeOptionsInternal;
},
): Promise<ContainerRuntime> {
const {
context,
registryEntries,
registry,
existing,
requestHandler,
provideEntryPoint,
Expand Down Expand Up @@ -966,8 +990,6 @@ export class ContainerRuntime
? runtimeOptions.enableRuntimeIdCompressor
: defaultConfigs.enableRuntimeIdCompressor;

const registry = new FluidDataStoreRegistry(registryEntries);

const tryFetchBlob = async <T>(blobName: string): Promise<T | undefined> => {
const blobId = context.baseSnapshot?.blobs[blobName];
if (context.baseSnapshot !== undefined && blobId !== undefined) {
Expand Down
5 changes: 3 additions & 2 deletions packages/runtime/container-runtime/src/test/batching.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { type SinonFakeTimers, createSandbox, useFakeTimers } from "sinon";

import type { ChannelCollection } from "../channelCollection.js";
import { ContainerRuntime } from "../containerRuntime.js";
import { FluidDataStoreRegistry } from "../dataStoreRegistry.js";
import { ContainerMessageType } from "../messageTypes.js";

describe("Runtime batching", () => {
Expand Down Expand Up @@ -79,9 +80,9 @@ describe("Runtime batching", () => {

beforeEach(async () => {
mockDeltaManager = new MockDeltaManager();
containerRuntime = await ContainerRuntime.loadRuntime({
containerRuntime = await ContainerRuntime.loadRuntime2({
context: getMockContext(mockDeltaManager) as IContainerContext,
registryEntries: [],
registry: new FluidDataStoreRegistry([]),
existing: false,
runtimeOptions: {},
provideEntryPoint: mockProvideEntryPoint,
Expand Down
9 changes: 5 additions & 4 deletions packages/runtime/container-runtime/src/test/containerRuntime.extensions.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
} from "@fluidframework/test-runtime-utils/internal";

import { ContainerRuntime } from "../containerRuntime.js";
import { FluidDataStoreRegistry } from "../dataStoreRegistry.js";

const testExtensionId: ContainerExtensionId = "test:extension";

Expand Down Expand Up @@ -115,9 +116,9 @@ async function createRuntimeWithMockContext(isReadonly: boolean = false): Promis
context: MockContext;
}> {
const context = new MockContext(isReadonly);
const runtime = await ContainerRuntime.loadRuntime({
const runtime = await ContainerRuntime.loadRuntime2({
context,
registryEntries: [],
registry: new FluidDataStoreRegistry([]),
existing: false,
provideEntryPoint: async () => ({}),
});
Expand All @@ -131,9 +132,9 @@ async function createRuntimeWithoutConnectionState(isReadonly: boolean = false):
const context = new MockContext(isReadonly);
// Delete the getConnectionState method before creating the runtime
delete context.getConnectionState;
const runtime = await ContainerRuntime.loadRuntime({
const runtime = await ContainerRuntime.loadRuntime2({
context,
registryEntries: [],
registry: new FluidDataStoreRegistry([]),
existing: false,
provideEntryPoint: async () => ({}),
});
Expand Down
Loading
Loading