Repository navigation
Conversation
🦋 Changeset detectedLatest commit: 9b3a4de The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34e3d0c821
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
34e3d0c to
4c018c0
Compare
|
The fix is correct and the tests confirm the retry behavior. One maintainability suggestion on where the cleanup lives — not a blocker. Right now the failure-recovery side effect is attached inside Since let loading: Promise<(() => T) | undefined>;
const asyncLoadProcess = async () => {
try {
const factory = await targetShared.get!();
addUseIn(targetShared, host.options.name);
targetShared.loaded = true;
targetShared.lib = factory;
return factory as () => T;
} catch (e) {
if (targetShared.loading === loading) {
targetShared.loading = null;
}
throw e;
}
};
loading = asyncLoadProcess();The same pattern applies to the unregistered/resolver path. Current code works — this is purely about cohesion/future-proofing. |
|
@2heal1 Thanks for the suggestion. Addressed in the latest changes. The rejected-load cleanup is now handled inside the I also preserved the unregistered resolver path and synchronous |
|
I don't think we should merge this as is. Retrying a shared load adds another way for two versions of a singleton to end up in use. I added an integration test with a runtime host and a webpack remote loaded over HTTP:
The test increments a counter from each side. Each side reads 1. With one shared instance, the second side would read 2. If we retry, every consumer has to wait for the same singleton resolution. One consumer can't start using a version while another consumer's pending retry later resolves to a different one. The same race already exists on the first load. This change makes it reachable after a failed load as well, which is why I'm wary of adding retries here. The control case passes, where the host finishes loading before the remote arrives. The failing tests are in aa04ccb, with no runtime changes. The focused run has 12 passing tests and 2 failing race cases: pnpm --filter @module-federation/runtime-core exec rstest run --include '**/__tests__/shared-retry-integration.spec.ts' --include '**/__tests__/shared-diagnostics.spec.ts' |
|
@ScriptedAlchemy Thanks for the additional reproduction. I was able to reproduce both the The current fix only removes rejected loading entries; it does not reserve singleton resolution. As a result, a remote can select version 2 while the host retry is still resolving version 1, causing two singleton instances to be used. I propose serializing singleton resolution across runtime-core and the Webpack share scope:
The alternative would be disabling retries for singleton shared modules, but that would retain the original transient-failure behavior for those modules. Does this direction align with the expected behavior? I can implement it and keep the new integration cases as regression coverage. |
|
Need to discuss this with @2heal1 as sharing is very sensitive. I don't think we should have reflect and rollback implemented on this. Too many race conditions for rollback. |
This branch has not been deployed
3 participants
Description
This change fixes an issue where
SharedHandler.loadShare()stores the Promise for an asynchronous shared-module load in the registered share entry but leaves the rejected Promise in the share scope after the load fails. After a transient failure, a laterloadShare()call that selects the same provider/version reuses the same rejected Promise instead of invoking the provider'sget()again.This does not introduce automatic retries. It clears the failed in-flight Promise so the next consumer request can start a new load.
Changes:
setShared()clearloadingon the actual registered share entry when an asynchronous shared load rejects.loadShare()retries the provider after a transient failure.@module-federation/runtime-core.Related Issue
globalLoadingTypes of changes
Validation
pnpm --filter @module-federation/runtime-core exec rstest __tests__/shared-diagnostics.spec.ts— 9/9 passedpnpm exec turbo run test --filter=@module-federation/runtime-core— 128 tests passedpnpm exec turbo run test --filter=@module-federation/runtime— 93 tests passedpnpm exec turbo run build --filter=@module-federation/runtime-core --filter=@module-federation/runtime— passedpnpm --filter @module-federation/runtime-core run lint— passedpnpm exec prettier --check packages/runtime-core/src/shared/index.ts packages/runtime-core/__tests__/shared-diagnostics.spec.ts .changeset/runtime-shared-retry.md— passedChecklist