Make internal loadContainerRuntime take in IFluidDataStoreRegistry

[CraigMacomber]

Description

This makes the internal loadContainerRuntime more flexible, so custom IFluidDataStoreRegistry implementations can be provided. This makes custom delay loading easier (like that in #25422 )

Reviewer Guidance

The review process is outlined on this wiki page.

[Copilot]

Pull Request Overview

This PR refactors the internal loadContainerRuntime API to accept an IFluidDataStoreRegistry directly instead of registry entries, making it more flexible for custom data store registry implementations. The change enables easier custom delay loading scenarios by allowing custom registry implementations to be passed directly.

• Modified the internal ContainerRuntime.loadRuntime method to accept a registry parameter instead of registry entries
• Updated all test files to use FluidDataStoreRegistry instances instead of passing raw registry entries
• Updated the attributor mixin to work with the new registry interface

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
packages/runtime/container-runtime/src/containerRuntime.ts	Updated the internal loadRuntime method to accept IFluidDataStoreRegistry and modified the public loadContainerRuntime to wrap registry entries
packages/framework/attributor/src/mixinAttributor.ts	Updated attributor mixin to work with the new registry interface instead of modifying registry entries array
packages/tools/replay-tool/src/replayFluidFactories.ts	Updated to use FluidDataStoreRegistry constructor with registry entries
packages/test/test-utils/src/testContainerRuntimeFactory.ts	Updated test utility to use FluidDataStoreRegistry constructor
Multiple test files	Updated all test calls to use FluidDataStoreRegistry instances instead of raw registry entries

[CraigMacomber]

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?)

[Josmithr]

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.

[CraigMacomber]

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.

[CraigMacomber]

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.

[CraigMacomber]

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.

[CraigMacomber]

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.

[Josmithr]

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.

[agarwal-navin]

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).

[agarwal-navin]

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?

[scottn12]

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

[Josmithr]

Left a docs suggestion, and a suggested fix for a typo. Otherwise looks good to me!