fix(create): a create completing on a disposed hub no longer throws out of its Subscribe callback (exit 2 after a clean summary) - #5691
Conversation
…ut of its Subscribe callback After the create answers, ActivatePendingControlPlane (#3153) called hub.GetMeshNodeStream, which resolves the workspace EAGERLY from the handling hub's scope. A caller that disposes that hub once answered made it throw ObjectDisposedException synchronously out of the completion callback, onto the pool thread that completed the chain: an unhandled exception, and a test host exiting 2 after a clean summary (core run 36104469492). The activation, the post-creation chain and its rollback are now built inside Observable.Defer, so the throw is an OnError their existing arms report. ACreateCompletingOnADisposedHubTest pins it deterministically: red before (ObjectDisposedException out of LetThrough), green after. Refs #5690 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Bulk rollback can still eagerly resolve services after hub disposal, and test coverage does not verify all deferred paths.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
This PR defers service resolution during create completion to prevent disposed-hub exceptions from escaping reactive callbacks.
Changes:
- Defers activation, post-creation, and rollback operations.
- Adds a disposed-hub regression test.
- Documents the deferred activation pattern.
| File | Summary |
|---|---|
test/MeshWeaver.Graph.Test/ACreateCompletingOnADisposedHubTest.cs |
Adds a regression test for completion after hub disposal. |
src/MeshWeaver.Mesh.Contract/MeshExtensions.cs |
Defers vulnerable observable construction. |
src/MeshWeaver.Documentation/Data/Architecture/RequestViaStreamUpdate.md |
Documents the disposal-safe pattern. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "Post-creation handler chain errored at {Path} — rolling the create back (#638)", | ||
| resultNode.Path); | ||
| CompensateFailedCreate(hub, resultNode, mode, logger) | ||
| Observable.Defer(() => CompensateFailedCreate(hub, resultNode, mode, logger)) |
There was a problem hiding this comment.
Agreed — fixed in the next commit. The deferral now lives INSIDE both helpers: CompensateFailedCreate wraps its service resolution in Observable.Defer with its existing Catch outside it, so an ObjectDisposedException at either call site (single-create error arm, bulk Concat at ~2387) becomes the Undetermined outcome and the bulk rollback continues with the next ghost; RunPostCreationHandlersObs is now Observable.Defer(() => BuildPostCreationHandlers(...)). The call-site Defers were removed. Release -warnaserror clean; ACreateCompletingOnADisposedHubTest and ControlPlaneRunsWhenTheRequestArrivesByCreateTest green.
…nHandlersObs so the bulk rollback's Concat is covered too Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Refs #5690
What
HandleCreateNodeRequest's completion arm callsActivatePendingControlPlaneafter the response. The caller, once answered, may dispose the hub handling the create;GetMeshNodeStreamresolves the workspace eagerly from that hub's scope and threwObjectDisposedExceptionsynchronously out of theSubscribecallback onto the pool thread that completed the chain → unhandled → xUnit exit 2 after a clean summary (run 36104469492,MeshWeaver.Compiler.Pipeline.Test, 1102 passed,[FATAL ERROR]stack ending inActivatePendingControlPlane). In a portal the same path would terminate the process.Fix
The three builders this chain calls from inside
Subscribecallbacks and that resolve hub services eagerly —ActivatePendingControlPlane's stream,RunPostCreationHandlersObs,CompensateFailedCreate— are wrapped inObservable.Defer, so the throw becomes anOnErrortheir existing arms report (the activation's warning; the post-handler/rollback arms, whoseRespondis a no-op once the owner-disposing NACK has answered). No bound raised, nothing swallowed.Verification
ACreateCompletingOnADisposedHubTest(new, Graph.Test): parks a real post-creation handler, disposes the hub handling the create (scope closure asserted), releases the park on the test thread. Before the fix: FAIL —ObjectDisposedExceptionout ofLetThrough. After: PASS, withControlPlaneRunsWhenTheRequestArrivesByCreateTest(Control-plane request created outside the UI never runs: per-node hub is not activated on create (Store/Subscription stuck at Requested for 4.5 h) #3153) still green.dotnet build -c Release -warnaserror: MeshWeaver.Mesh.Contract, MeshWeaver.Graph.Test — 0 warnings, 0 errors.The other exit-2 sighting (
MeshWeaver.Messaging.Hub.Test, PR #5674 run 36098377607) is a different unhandled exception — a test subscriber with no error arm on a released owned connection — already fixed by #5681; details in #5690.Doc:
Doc/Architecture/RequestViaStreamUpdate(activation section).Pairs-with: none — no public surface removed.
🤖 Generated with Claude Code