Isolate study faults on both market-data dispatchers and stop NaN reaching the resilience score - #77
Merged
Conversation
…he resilience score Order-book dispatch no longer rethrows after logging a faulting subscriber. The rethrow ran on the market connector's producer thread at a call site that does not guard itself, so an unhandled exception on a non-UI thread terminated the process, and it unwound the dispatch loop, starving every subscriber after the faulting one of order-book data. Trade dispatch had no per-subscriber guard at all and now gets the same treatment, with a new OnException event and a Reset() to match the order-book helper. The study base class subscribes its fault handler to both helpers, so a study faulting on the trade stream is stopped and marked failed instead of sitting at its previous status while receiving no data. The Market Resilience score can no longer leave [0, 1] or become NaN. A recovery component whose denominator is zero is omitted rather than computed as 0/0; zero-duration samples no longer enter the recovery histories, where they pulled the baseline down for the rest of the session; the trade component is clamped at both ends and skipped when the size dispersion is below a dimensionless relative epsilon of the mean; and the final cast to decimal is guarded by a finiteness check, because a non-finite quotient survives a clamp untouched and then throws. A cycle with no usable evidence at all now publishes nothing instead of the top of the scale. Tests: numerical-stability facts for the calculator, including hand-computed worked examples, plus dispatch-isolation and fault-wiring facts for the two helpers and the study base class.
There was a problem hiding this comment.
🟡 Changes recommended
The helpers’ fire-and-forget exception publishing still risks unobserved task failures and noisy repeated faults, and should be hardened before merge.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR hardens the market-data dispatch path (order books + trades) so a faulting subscriber no longer crashes the process or starves other subscribers, and it strengthens the Market Resilience score calculation to prevent NaN/out-of-range values from being published.
Changes:
- Isolates per-subscriber faults in
HelperOrderBookandHelperTrade, raising anOnExceptionsignal instead of allowing exceptions to escape the producer thread. - Wires
BasePluginStudyto consume exceptions from both market-data streams and stop/mark studies failed consistently. - Makes
MarketResilienceCalculatornumerically stable (omit unusable components, avoid NaN propagation, guard non-finite casts) and adds targeted unit tests + xUnit non-parallel config.
File summaries
| File | Description |
|---|---|
| VisualHFT.Plugins/Studies.MarketResilience/Model/MarketResilienceCalculator.cs | Sanitizes MR component math (omit zero-denominator cases, clamp safely, guard non-finite results). |
| VisualHFT.Commons/PluginManager/BasePluginStudy.cs | Subscribes to both helpers’ OnException so trade-stream faults stop the study too. |
| VisualHFT.Commons/Helpers/HelperTrade.cs | Adds OnException, Reset(), and per-subscriber exception isolation for trade dispatch. |
| VisualHFT.Commons/Helpers/HelperOrderBook.cs | Removes rethrow so order-book subscriber faults don’t terminate the process. |
| tests/Unit/VisualHFT.Commons.Tests/HelperTradeDispatchIsolationTests.cs | Verifies trade dispatch isolation + OnException signaling contract. |
| tests/Unit/VisualHFT.Commons.Tests/BasePluginStudyHelperExceptionWiringTests.cs | Verifies studies stop/mark failed for faults from both streams. |
| tests/Unit/Studies.MarketResilience.Test/xunit.runner.json | Disables parallel execution due to process-wide clock usage in tests. |
| tests/Unit/Studies.MarketResilience.Test/Studies.MarketResilience.Test.csproj | Copies xUnit runner config into test output. |
| tests/Unit/Studies.MarketResilience.Test/MarketResilienceCalculatorNumericalStabilityTests.cs | Adds invariants + worked examples and randomized regression coverage for MR stability. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
75
to
79
| Task.Run(() => | ||
| { | ||
| log.Error(ex); | ||
| OnException?.Invoke(new VisualHFT.Commons.Model.ErrorEventArgs(ex, subscriber.Target)); | ||
| }); |
Comment on lines
+80
to
+84
| Task.Run(() => | ||
| { | ||
| log.Error(ex); | ||
| OnException?.Invoke(new VisualHFT.Commons.Model.ErrorEventArgs(ex, subscriber.Target)); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three fixes on the market-data dispatch path and in the Market Resilience score.
1. A faulting subscriber no longer kills the process
HelperOrderBook.DispatchToSubscriberslogged the exception and then rethrew.That rethrow ran on the market connector's producer thread, at a call site that
does not guard itself: an unhandled exception on a non-UI thread terminates the
process with no dialog. It also unwound the dispatch loop, so one faulting study
starved every subscriber after it of order-book data.
HelperTrade.DispatchToSubscribershad no per-subscriber guard at all, with thesame two consequences on the trade stream.
Both now isolate each subscriber, log the fault, raise it on
OnExceptionwiththe subscriber that caused it, and continue to the next subscriber.
HelperTradegains the
OnExceptionevent and aReset()so it matches the order-book helper.2. A study that faults on the trade stream is now stopped
BasePluginStudysubscribed its fault handler only to the order-book helper.Raising the event on the trade helper would have surfaced nothing: a study
faulting on trades stayed at its previous status while receiving no data. The
same handler is now subscribed to and unsubscribed from both helpers, and is
renamed to reflect that it serves both.
3. The Market Resilience score can no longer be NaN or out of range
CurrentMRScoreis documented and consumed as a value in [0, 1]. Four problemscould take it outside that range, and the last of them threw:
avgHistory / (avgHistory + duration). With an empty history the numerator isset to the duration, so a zero-duration recovery evaluates
0.0 / 0.0= NaN.Math.Min/Math.Maxpropagate NaN, so the existing clamp did not sanitise it.A zero denominator is now treated as an absence of evidence: the component is
omitted and the weighted normalisation reweights what remains.
historical average at zero. That average is the numerator above, so every later
genuine recovery scored lower for the rest of the session. Only a measured
recovery joins the history now.
1 - z/6at the low end only, unlike its threesiblings. The shock trade is anchored when flagged and scored later against
whatever the window holds at trigger time, so its z-score can be large and
negative and the component could exceed 1. It is now clamped at both ends, and
it is skipped when the window has no positive mean or when the dispersion is
below a dimensionless relative epsilon of that mean, where a z-score carries no
information.
(decimal)of a non-finite double throwsOverflowException. The final cast isnow guarded by
double.IsFinite, which is the backstop for any future producerof a non-finite value in that method.
A cycle in which every component was omitted previously published
1.0, the topof the resilience scale, at the moment liquidity vanished. It now publishes
nothing: the last measured score stands until something is actually measured.
Tests
MarketResilienceCalculatorNumericalStabilityTestscovers the score invariants,the two recovery paths under a data-driven clock, a randomised equity tape, and
four hand-computed worked examples (a normal shock and recovery, a near-constant
trade window, a zero-duration recovery against an empty history, and an
all-omitted cycle).
HelperTradeDispatchIsolationTestsandBasePluginStudyHelperExceptionWiringTestscover the dispatch guard and the study-side wiring for both streams.
tests/Unit/Studies.MarketResilience.Testgets anxunit.runner.jsonthatserialises the assembly, because the calculator tests drive a process-wide clock.
Verification
dotnet buildonVisualHFT.Commons,Studies.MarketResilience,tests/Unit/Studies.MarketResilience.Testandtests/Unit/VisualHFT.Commons.Tests:0 errors.
Studies.MarketResilience.Test: 67 passed, 0 failed.VisualHFT.Commons.Tests: 87 passed, 0 failed.