Repository navigation
fix(layout): a faulting fed stream is surfaced at its area, never rethrown on the pool - #5968
Conversation
…hrown on the pool Template.Bind / BindMany subscribed the fed stream with no error arm, so a fault (a query stall, a projection throwing on an empty cube, a denied read) reached Rx's default OnError and rethrew on the producer's thread — the #5650 crash shape. Every view converted in the data-binding wave sits behind that subscription. LayoutAreaHost.FeedData / SubscribeFeed is now the one way the layout machinery subscribes a data feed: a late fault is logged with area + data id (classified like a render fault) and the bound control is replaced by the standard localized error frame; a fault during Subscribe is rendered into the store being built (Bind/BindMany) or rethrown into the view builder's existing area-error path. Swept the other bare feed subscriptions: SubscribeToDataStream, the Domain catalog/edit feeds (one projection throws by design), and the editor dimension/display-label feeds. BindFeedFaultTest: late fault (Bind, BindMany, SubscribeToDataStream), fault during Subscribe, healthy-feed negative control; 3 of them fail on the unfixed Template.cs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 2)1 032 tests 1 032 ✅ 3m 37s ⏱️ Results for commit 543affc. ♻️ This comment has been updated with latest results. |
Test Results (shard 1) 1 files 1 suites 4m 31s ⏱️ Results for commit 543affc. ♻️ This comment has been updated with latest results. |
Test Results (shard 0) 1 files 1 suites 3m 8s ⏱️ Results for commit 543affc. ♻️ This comment has been updated with latest results. |
Test Results (shard 3)2 212 tests 2 212 ✅ 8m 13s ⏱️ Results for commit 543affc. ♻️ This comment has been updated with latest results. |
Test Results (shard 5) 4 files 4 suites 10m 51s ⏱️ Results for commit 543affc. ♻️ This comment has been updated with latest results. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Converts every layout data-feed subscription to a single error-armed path: the new internal `LayoutAreaHost.FeedData`/`SubscribeFeed` pair does `feed.Subscribe(onNext, onError)` behind a small phase latch - a fault after Subscribe is classified and logged (Error; Warning for denials/missing nodes; Debug for disposal races) and the bound area is replaced by the standard localized error frame, while a fault during Subscribe is rendered into the store being built (`Bind`/`BindMany`) or rethrown into the view builder (`FeedData`). `Template.Bind`/`BindMany`, `SubscribeToDataStream`, the DomainCatalog and Edit areas, the two dimension-select feeds and the three read-only label feeds are converted; `FailRendering`'s visible half is extracted into the shared `SurfaceRenderError`; a doc section and `BindFeedFaultTest` (5 executing tests, healthy negative control, producer-side `NotThrow` pin) are added. Checked from the diff: latch claim/once semantics across the interleavings traced; onNext behavior, registration keys and disposal scope unchanged except the two domain areas moving to area-scoped registration (as stated); no new async/await, no new `!`, no hand-built HTML, no public-surface change; the doc matches the three named APIs. Not verifiable from the diff: `resolvedArea`; the `RemoveViews`/`UpdateControl`/`CreateRenderErrorControl`/`AreaErrorClassifier` signatures; `SurfaceRenderError`'s unchanged tail; whether the existing suite exercises `FeedData`'s rethrow arm; the claimed 560/560 and warnaserror builds (CI gates those).
Findings: 0 blocking · 1 should-fix · 1 question · 0 nit
Internal review of a34ba04c51fd04e9fe9b82e65fa8b26e460d85ba — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| var subscription = feed.Subscribe(onNext, ex => | ||
| { | ||
| if (!latch.TryClaimForSubscriber(ex)) | ||
| FailFeed(ex, area, dataId); |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
The error arm added in `SubscribeFeed` runs `FailFeed` unguarded inside the Rx `OnError` callback, i.e. on the producer's thread (lines 1195-1198). `FailFeed` (lines 1223-1227) then mutates the store and ships updates via `SurfaceRenderError` (line 1670) after a single `Hub.IsServiceScopeDisposed()` read; an exception on that path - a hub teardown racing past the check, or a failure while building or delivering the error frame - escapes the callback and rethrows at the producer, the same process-killing shape this change removes. `BindFeedFaultTest` pins only that the fault itself no longer rethrows. A try/catch around the surface half that logs at Error would keep the error arm from reintroducing the crash it exists to prevent.
There was a problem hiding this comment.
Agreed — fixed in the follow-up commit. FailFeed now wraps the surface half in a try/catch that reports at Error ("Could not surface the fault of data feed …"), the same way RenderRenderingError guards its placeholder; the fault itself is logged before it. Nothing thrown while drawing the frame can reach the producer any more.
| if (synchronousFault is null) | ||
| return subscription; | ||
| subscription.Dispose(); | ||
| System.Runtime.ExceptionServices.ExceptionDispatchInfo.Capture(synchronousFault).Throw(); |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
`FeedData`'s synchronous-fault arm - dispose the subscription, rethrow via `ExceptionDispatchInfo` (lines 1178-1182) - is the path the `DomainCatalogLayoutArea` and `EditLayoutArea` conversions now depend on (`EditLayoutArea`'s projection throws by design on a missing entity), yet nothing in the new `BindFeedFaultTest` reaches it: the immediate-fault test goes through `Bind`'s `RenderFeedFault`, and the late-fault tests never rethrow. Whether any existing test pins a cold-faulting feed at those two areas cannot be told from this diff; if none does, the rethrow arm ships untested.
There was a problem hiding this comment.
Good catch — it was untested. Added AHostFeedThatFaultsWhileBeingSubscribed_FailsTheViewThroughTheAreaErrorPath: a view builder calls SubscribeToDataStream with a projection that throws on subscribe (the EditLayoutArea shape); the area must show the error frame. Negative control: with the rethrow removed it FAILS (the view's own control overwrites the error); with it, 6/6 pass.
Test Results (shard 4)1 222 tests 1 222 ✅ 8m 15s ⏱️ Results for commit 543affc. ♻️ This comment has been updated with latest results. |
Test Results 14 files 14 suites 38m 37s ⏱️ Results for commit 543affc. ♻️ This comment has been updated with latest results. |
… rethrow arm Review on #5968: FailFeed runs inside OnError on the producer's thread, so a failure while drawing the error frame would rethrow there — now reported at Error instead, as RenderRenderingError guards its placeholder. New test pins FeedData's fault-during-subscribe arm (fails when the rethrow is removed: the view's own control overwrites the error). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… reaches an error arm FeedDimensionDisplayName and FeedDimensionOptions subscribed with a bare .Subscribe(onNext), the #5650 crash shape (heads-up from #5968). Both now go through host.FeedData(null, id, stream, onNext): a fault on a later emission is logged at its classified level instead of escaping on the hub. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 participant
Refs triage
rbuergi/Feedback/template-bind-subscribes-projection-without-error-arm(no GitHub issue yet) · related #5650Root cause
Template.Bind(IObservable<T>, …)andBindMany(IObservable<IEnumerable<T>>, …)subscribed the fed stream withstream.Subscribe(onNext)and no error arm. A fault (a query stall, a projection throwing on an empty cube, a denied read) reached Rx's defaultOnError, which rethrows on the producer's thread — off the pool that is an unhandled exception, the shape that killed memex-cloud replicas in #5650. Every view converted in the data-binding wave (#5940 and its stacks) feeds its template through exactly this subscription.Fix
LayoutAreaHost.FeedData/SubscribeFeed(internal) is now the one way the layout machinery subscribes a data feed:CreateRenderErrorControl,error.areaFailedetc.; no new strings). Siblings keep rendering.Bind/BindManyrender the error frame into the store the buildup is building (aStream.Updatemid-render would be overwritten by the render's own result);FeedDatarethrows into the view builder, i.e. the existing area-error path.FailRenderingwas split into log +SurfaceRenderErrorso both paths share the visible half.Sweep of the binding machinery (
src/MeshWeaver.Layout)Converted:
Template.Bind,Template.BindMany,LayoutAreaHost.SubscribeToDataStream(public, signature unchanged — fault shows on the host's area),DomainCatalogLayoutArea(now area-scoped disposal),EditLayoutArea(its projection throws by design on a null entity — a live instance of the crash shape),EditorExtensionsdimension-select feeds (×2) and the options/date/number read-only label feeds (×3; no area known → logged only).Deliberately NOT touched (overlap):
EditorExtensions.BuildDimensionReadOnlyLabelandCreateDimensionSelectControl's options feed — #5944 moves both intoFeed*helpers. Will note on #5944 to route them throughhost.FeedData.Out of scope, same shape, not binding feeds: click-handler one-shot reads
GetDataStream(...).Take(1).Subscribe(...)inEditorExtensions(Remove/AddCollectionItem, add dialog) andPromptCell.Evidence
BindFeedFaultTest(5): late fault inBind,BindMany,SubscribeToDataStream→ the producer'sOnErrorreturns (no rethrow), area shows the error frame with the cause, sibling intact, one Error record naming area + data id; fault during Subscribe → error frame instead of the template; negative control: healthy feed renders its template, binds its value, reports nothing.Template.cs: 3 of the Bind/BindMany tests FAIL (NotThrowtrips — the fault is rethrown at the producer; the sync case never resolves). With the fix: 5/5.MeshWeaver.Layout.Test: 560/560. Release-warnaserror: MeshWeaver.Layout, MeshWeaver.Layout.Test, MeshWeaver.Graph — 0 warnings, 0 errors.Doc/GUI/DataBinding→ "When a fed stream faults".Deploy
Framework code only; no per-node recycle beyond the ordinary roll. No NodeType source changed.
Pairs-with: none — no public type or member removed (
SubscribeToDataStreamkeeps its signature; new members are internal).🤖 Generated with Claude Code