fix(di): open one DI scope per session so Scoped services match the documented lifetime#74
fix(di): open one DI scope per session so Scoped services match the documented lifetime#74carldebilly wants to merge 2 commits into
Conversation
…ocumented lifetime Fixes #70. Repl never created an IServiceScope: Scoped registrations resolved from the shared root provider, so the same instance crossed commands AND distinct hosted sessions — sharing per-user state (auth context, carts) across concurrent clients, exactly what the documentation told users Scoped would prevent. - ReplApp Run* entry points now run each session inside an AsyncServiceScope (one Run* call = one session: CLI one-shot, local REPL, hosted WebSocket/ Telnet connection). Scoped disposables are released when the session ends. Hosted services keep starting from the unscoped provider (app-level lifetime). Providers without scope support run unscoped, as before. - ISessionScopedServiceProvider (internal marker): a session owner spanning several one-shot Run* calls with one provider carries its own scope and the entry points skip re-scoping (no fresh Scoped instances per command). - Repl.Testing sessions now compose their provider over a scope of the APP's services instead of an isolated near-empty local provider: user registrations become visible to session commands (previously a silent NullReferenceException), Scoped instances are shared across a session's commands and released on handle dispose. - docs/comparison.md: per-session DI claim now states the DI-scope reality. RED observed: 4 failing regressions in Given_SessionScopedServices (distinct scoped instances across sessions, shared within a session, transient per resolution, scoped disposal at session end).
![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: b99e0991b4
ℹ️ 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".
| return await run(services, cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| var scope = scopeFactory.CreateAsyncScope(); |
There was a problem hiding this comment.
When a caller passes an already-created IServiceScope.ServiceProvider to the externally managed RunAsync overloads (for example, HttpContext.RequestServices), that provider still exposes the root IServiceScopeFactory, so this creates a separate scope rather than using the supplied one. Scoped services resolved by the command will then differ from the caller's request/session-scoped instances, and the caller cannot opt out because ISessionScopedServiceProvider is internal. Keep a supplied scope intact or expose an explicit opt-out mechanism.
Useful? React with 👍 / 👎.
| var root = app.Services; | ||
| if (root.GetService(typeof(IServiceScopeFactory)) is IServiceScopeFactory scopeFactory) | ||
| { | ||
| var scope = scopeFactory.CreateAsyncScope(); | ||
| return new SessionScopedTestServices(scope, scope.ServiceProvider); |
There was a problem hiding this comment.
When ReplTestHost's appFactory creates a fresh app per OpenSessionAsync call, this materializes a new app root provider for every test session. ReplSessionHandle.DisposeAsync only disposes the child AsyncServiceScope, so disposable singleton services resolved while running the session remain undisposed for the life of the test process; before this change the test harness did not materialize the app provider at all. Track ownership of these per-session app providers and dispose them with the handle, without disposing a shared app provider.
Useful? React with 👍 / 👎.
…(review) - Replace the internal ISessionScopedServiceProvider marker with a public ReplRunOptions.SessionScope option (PerRun default, CallerOwned opt-out): a caller whose provider already represents the session scope (Blazor circuit, per-request scope, multi-run session owners) is no longer double-scoped with its scoped instances hidden — and external session owners could not use an internal marker anyway. - Repl.Testing opts out through the public option instead of the marker. - New core regression: two Run* calls against ONE shared app resolve distinct Scoped instances from the app's own root provider — exercising the scope-creation branch itself (RED verified against the pre-fix ReplApp: both runs shared one instance). The previous suite only passed through Repl.Testing's marked provider and never hit that branch. - New regression for the CallerOwned contract (same instance across runs). - ReplSessionHandle takes the command gate before disposing the session scope (no race between an orphaned command and scoped disposal), and the shared-provider comment states the Scoped nuance instead of promising module/handler instance identity for every lifetime.
|
Review pass applied (commit dd4ee1b): the internal marker was replaced by a public |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd4ee1b257
ℹ️ 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".
| if (_services is IDisposable disposable) | ||
| // Take the command gate before disposing the session scope so an in-flight command | ||
| // (e.g. an orphaned timed-out run) cannot race scoped-service disposal. | ||
| await _commandGate.WaitAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
If RunCommandAsync passes ThrowIfDisposed() and is then waiting for the gate when DisposeAsync begins, disposal can acquire the gate first. This method disposes the semaphore without releasing it, so that already-queued command awaits forever (the default command token does not cancel it). Keep the gate acquisition/release in a try/finally, or otherwise wake queued callers so they receive an ObjectDisposedException rather than hanging.
Useful? React with 👍 / 👎.
| public object? GetService(Type serviceType) => | ||
| serviceType == typeof(IReplSessionState) ? _sessionState : _inner.GetService(serviceType); |
There was a problem hiding this comment.
When a command resolves an app-registered service whose constructor depends on IReplSessionState, DI constructs that service through _inner, so it receives the app registration rather than this handle's _sessionState; only direct handler lookups are intercepted here. Consequently a ReplTestHost test can have its handler and a dependent service mutate different state objects (and a reused app can share the app-default state between test sessions), making session-state tests of newly visible user services inaccurate.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #70. Repl never created an
IServiceScope:Scopedregistrations resolved from the shared root provider, so the same instance was reused across commands and across distinct hosted sessions — sharing per-user state (auth context, carts) between concurrent WebSocket/MCP clients, exactly what the documentation told usersScopedwould prevent.Fix
Run*call = one session = oneAsyncServiceScope(CLI one-shot, local REPL, hosted WebSocket/Telnet connection).Scopedservices resolve per session; scoped disposables are released when the session ends. Hosted services keep starting from the unscoped provider (app-level lifetime). Providers without scope support run unscoped, as before.ISessionScopedServiceProvider(internal marker): a session owner spanning several one-shotRun*calls with one provider (Repl.Testing's session handle runs each command as a separate one-shot run) owns its scope itself; the entry points skip re-scoping so a logical session does not get freshScopedinstances per command.ReplSessionHandlecomposed an isolated near-empty local provider, so services registered throughReplApp.Create(services => ...)were invisible to session commands (silentNullReferenceException). Sessions now compose over a scope of the app's provider (+ per-sessionInMemorySessionState), and the scope is disposed with the handle.docs/comparison.md: the per-session DI claim now states the DI-scope reality.Not in scope: the MCP handler's session overlay (single
McpServiceProviderOverlayshared across sessions) — follow-up after PR #71 merges, as noted on the issue.The website docs (
repl-websitedependency-injection.mdx) describe the now-correct behavior; they need no change beyond removing any caveats, tracked separately since it is another repository.Test Plan
Given_SessionScopedServices— two hosted sessions resolve distinctScopedinstances; two commands of one session share theirs;Transientstays per-resolution; aScopedIDisposableis disposed when its session ends (also RED as a NullReferenceException before Repl.Testing sessions could see app services at all).Fixes #70