feat: Aggregator hubs for quote/tick quantization#1875
Merged
DaveSkender merged 37 commits intov3from Mar 3, 2026
Merged
Conversation
Contributor
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds streaming aggregation and tick support: new QuoteAggregatorHub and TickAggregatorHub convert IQuote/ITick streams into time-based OHLCV bars with PeriodSize/TimeSpan constructors, optional gap-filling, deduplication, late-arrival rebuild/rollback logic, and extension methods to create aggregators from providers. Introduces ITick and a Tick record, a TickHub stream/cache implementation, changes visibility of NotifyObserversOnAdd to protected, and extensive unit tests covering hubs, aggregators, and edge cases. Also adds a small VSCode settings file. Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: DaveSkender <8432125+DaveSkender@users.noreply.github.com>
Co-authored-by: DaveSkender <8432125+DaveSkender@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add quote-stream auto-aggregator prototype
feat: Add QuoteAggregatorHub for streaming quote aggregation
Jan 5, 2026
Signed-off-by: GitHub <noreply@github.com>
DaveSkender
changed the title
DaveSkender
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
DaveSkender
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: DaveSkender <8432125+DaveSkender@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Claude
AI
changed the title
DaveSkender
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/_common/Quotes/Tick.AggregatorHub.cs (1)
59-63:⚠️ Potential issue | 🟡 MinorInclude the offending value in the exception message text.
While
ArgumentOutOfRangeExceptionreceives the value as a constructor parameter, the message text itself should include the value for consistent diagnostics per coding guidelines.Suggested fix
if (timeSpan <= TimeSpan.Zero) { throw new ArgumentOutOfRangeException(nameof(timeSpan), timeSpan, - "Aggregation period must be greater than zero."); + $"Aggregation period must be greater than zero. Was: {timeSpan}"); }As per coding guidelines: Exception messages MUST include parameter name and offending value when relevant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.AggregatorHub.cs` around lines 59 - 63, The ArgumentOutOfRangeException thrown when timeSpan <= TimeSpan.Zero should include the offending value in the exception message text in addition to passing it as the constructor value; update the throw to build a message that contains both the parameter name "timeSpan" and the actual timeSpan value (e.g., via timeSpan.ToString()) before throwing ArgumentOutOfRangeException in the method/constructor in Tick.AggregatorHub (where timeSpan is validated) so diagnostics consistently show the bad value.
🧹 Nitpick comments (1)
src/_common/Quotes/Quote.AggregatorHub.cs (1)
102-114: Remove LINQ allocations from tracker pruning paths.Both pruning blocks allocate intermediary collections with LINQ (
Where/Select/ToList). In stream-processing paths this adds avoidable allocation churn.♻️ Proposed refactor
public class QuoteAggregatorHub : QuoteProvider<IQuote, IQuote> { private readonly Dictionary<DateTime, IQuote> _inputQuoteTracker = []; + private readonly List<DateTime> _pruneKeys = []; private Quote? _currentBar; private DateTime _currentBarTimestamp; @@ if (_inputQuoteTracker.Count > 1000) { DateTime pruneThreshold = item.Timestamp.Add(-10 * AggregationPeriod); - List<DateTime> toRemove = _inputQuoteTracker - .Where(kvp => kvp.Key < pruneThreshold) - .Select(kvp => kvp.Key) - .ToList(); - - foreach (DateTime key in toRemove) - { - _inputQuoteTracker.Remove(key); - } + PruneTrackerBefore(pruneThreshold); } } @@ - List<DateTime> toRemove = _inputQuoteTracker - .Where(kvp => kvp.Key > preserveTimestamp) - .Select(kvp => kvp.Key) - .ToList(); - - foreach (DateTime key in toRemove) - { - _inputQuoteTracker.Remove(key); - } + PruneTrackerAfter(preserveTimestamp); } + + private void PruneTrackerBefore(DateTime threshold) + { + _pruneKeys.Clear(); + foreach (DateTime key in _inputQuoteTracker.Keys) + { + if (key < threshold) + { + _pruneKeys.Add(key); + } + } + + foreach (DateTime key in _pruneKeys) + { + _inputQuoteTracker.Remove(key); + } + } + + private void PruneTrackerAfter(DateTime threshold) + { + _pruneKeys.Clear(); + foreach (DateTime key in _inputQuoteTracker.Keys) + { + if (key > threshold) + { + _pruneKeys.Add(key); + } + } + + foreach (DateTime key in _pruneKeys) + { + _inputQuoteTracker.Remove(key); + } + } }As per coding guidelines, "Minimize allocations in hot paths during performance optimization" and "Avoid LINQ in performance-critical loops".
Also applies to: 261-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Quote.AggregatorHub.cs` around lines 102 - 114, The pruning code for _inputQuoteTracker currently uses LINQ (Where/Select/ToList) which creates temporary collections; replace it with an allocation-minimizing manual pass: compute pruneThreshold = item.Timestamp.Add(-10 * AggregationPeriod), iterate the _inputQuoteTracker dictionary entries (foreach KeyValuePair<DateTime, ...> in _inputQuoteTracker), add keys that are < pruneThreshold to a small List<DateTime> (or reuse a pooled list) and then remove those keys in a separate loop; apply the same replace-LINQ-with-manual-iteration change to the similar pruning block around lines 261-269 so both hot-path prunings avoid LINQ allocations while still safely collecting and removing keys from _inputQuoteTracker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/_common/Quotes/Quote.AggregatorHub.cs`:
- Around line 13-30: The XML documentation for the public QuoteAggregatorHub
constructor is missing the <exception> entry for the ArgumentException thrown
when periodSize == PeriodSize.Month; update the constructor's XML docs to add an
<exception cref="System.ArgumentException"> element that describes that Month
aggregation is not supported in streaming mode and suggests using the TimeSpan
overload for custom periods (refer to PeriodSize and the QuoteAggregatorHub
constructor signature).
In `@src/_common/Quotes/Tick.StreamHub.cs`:
- Around line 40-47: Update the ArgumentOutOfRangeException message for the
maxCacheSize check so it includes the offending value; specifically, in the
validation block referencing maxCacheSize and absoluteMaxCacheSize (the code
that constructs message and throws new
ArgumentOutOfRangeException(nameof(maxCacheSize), maxCacheSize, message)),
append or embed the actual maxCacheSize value into the message text (e.g.
include "Actual: {maxCacheSize}") so the thrown message contains both the
parameter name, bounds, and the offending value.
---
Duplicate comments:
In `@src/_common/Quotes/Tick.AggregatorHub.cs`:
- Around line 59-63: The ArgumentOutOfRangeException thrown when timeSpan <=
TimeSpan.Zero should include the offending value in the exception message text
in addition to passing it as the constructor value; update the throw to build a
message that contains both the parameter name "timeSpan" and the actual timeSpan
value (e.g., via timeSpan.ToString()) before throwing
ArgumentOutOfRangeException in the method/constructor in Tick.AggregatorHub
(where timeSpan is validated) so diagnostics consistently show the bad value.
---
Nitpick comments:
In `@src/_common/Quotes/Quote.AggregatorHub.cs`:
- Around line 102-114: The pruning code for _inputQuoteTracker currently uses
LINQ (Where/Select/ToList) which creates temporary collections; replace it with
an allocation-minimizing manual pass: compute pruneThreshold =
item.Timestamp.Add(-10 * AggregationPeriod), iterate the _inputQuoteTracker
dictionary entries (foreach KeyValuePair<DateTime, ...> in _inputQuoteTracker),
add keys that are < pruneThreshold to a small List<DateTime> (or reuse a pooled
list) and then remove those keys in a separate loop; apply the same
replace-LINQ-with-manual-iteration change to the similar pruning block around
lines 261-269 so both hot-path prunings avoid LINQ allocations while still
safely collecting and removing keys from _inputQuoteTracker.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/settings.jsonsrc/_common/Quotes/Quote.AggregatorHub.cssrc/_common/Quotes/Tick.AggregatorHub.cssrc/_common/Quotes/Tick.StreamHub.cssrc/_common/Quotes/Tick.cssrc/_common/StreamHub/StreamHub.Observable.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/_common/Quotes/Tick.cs
Signed-off-by: GitHub <noreply@github.com>
Signed-off-by: GitHub <noreply@github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/_common/Quotes/Quote.AggregatorHub.cs (1)
15-31:⚠️ Potential issue | 🟡 MinorAdd missing XML
<exception>docs on thePeriodSizeconstructor.This overload throws
ArgumentException(Lines 29-31) but the XML docs don’t declare it.🛠️ Proposed doc update
/// <summary> /// Initializes a new instance of the <see cref="QuoteAggregatorHub"/> class. /// </summary> /// <param name="provider">The quote provider.</param> /// <param name="periodSize">The period size to aggregate to.</param> /// <param name="fillGaps">Whether to fill gaps by carrying forward the last known price.</param> +/// <exception cref="ArgumentException"> +/// Thrown when <paramref name="periodSize"/> is <see cref="PeriodSize.Month"/>. +/// </exception> public QuoteAggregatorHub(As per coding guidelines, “Update XML documentation for changed public APIs” and “Ensure all public APIs have complete documentation.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Quote.AggregatorHub.cs` around lines 15 - 31, The QuoteAggregatorHub constructor overload (QuoteAggregatorHub(IQuoteProvider<IQuote>, PeriodSize, bool)) throws an ArgumentException when periodSize == PeriodSize.Month but the XML docs are missing an <exception> entry; update the constructor's XML documentation to include an <exception cref="System.ArgumentException"> describing when it is thrown (e.g., "Month aggregation is not supported in streaming mode; use TimeSpan overload for custom periods") so the public API docs reflect the thrown exception for the periodSize parameter.src/_common/Quotes/Tick.StreamHub.cs (2)
40-47:⚠️ Potential issue | 🟡 MinorInclude the supplied
maxCacheSizein the message text.The thrown message states bounds but omits the offending value, which reduces diagnosability.
🔧 Proposed fix
- string message - = $"'{nameof(maxCacheSize)}' must be greater than 0 and not over {absoluteMaxCacheSize}."; + string message + = $"'{nameof(maxCacheSize)}' must be greater than 0 and not over {absoluteMaxCacheSize}; was {maxCacheSize}.";As per coding guidelines: Exception messages MUST include parameter name and offending value when relevant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.StreamHub.cs` around lines 40 - 47, Update the exception message to include the supplied offending value so it can be diagnosed: when validating maxCacheSize against absoluteMaxCacheSize in the Tick.StreamHub code, augment the constructed message string to incorporate the actual maxCacheSize value (and keep the existing bounds info), and then throw the ArgumentOutOfRangeException as before using nameof(maxCacheSize), maxCacheSize, and the new message; ensure you reference the existing variables maxCacheSize and absoluteMaxCacheSize and the same throw site so the parameter name and value are present in logs.
114-124:⚠️ Potential issue | 🟠 MajorDo not overwrite distinct executions at the same timestamp.
Line 118 replaces the cached tick even when
ExecutionIddiffers. That drops a valid trade and can break future rebuild parity because cache no longer contains both executions.🔧 Proposed fix
if (hasExecutionId && hasCachedExecutionId && Cache[index].ExecutionId != result.ExecutionId) { // Different execution IDs at same timestamp - both are valid trades - // Persist to cache before notifying observers - Cache[index] = result; + // Persist both executions in cache before notifying observers + Cache.Insert(index + 1, result); // Notify observers with the new tick so aggregators can process it if (notify) { - NotifyObserversOnAdd(result, index); + NotifyObserversOnAdd(result, index + 1); } return; }Based on learnings: when streaming hubs receive late inserts/revisions, rebuilds must realign to full-history results using provider cache as source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.StreamHub.cs` around lines 114 - 124, The current code in the tick add path overwrites Cache[index] when hasExecutionId && hasCachedExecutionId but Cache[index].ExecutionId != result.ExecutionId, dropping a valid trade; instead modify the cache to preserve both executions for the same timestamp (e.g., change Cache storage at that key to support multiple ticks such as a small list/collection or an aggregate container) and stop assigning Cache[index] = result when ExecutionId differs. Update the write path where this check occurs and ensure NotifyObserversOnAdd (and any consumers that read Cache[index]) accept an added tick rather than assuming a single entry—keep the existing cached tick and append the new result to the per-index collection, then call NotifyObserversOnAdd with the new tick and index (or with both ticks as required) so rebuilds can use the provider cache as the full-history source.
🧹 Nitpick comments (1)
src/_common/Quotes/Tick.AggregatorHub.cs (1)
111-129: Reduce LINQ allocations in execution-id pruning hot path.
Where/Select/ToListandOrderBy/Take/ToListinsideOnAdd(under lock) add avoidable allocation and sort overhead on a high-frequency path.As per coding guidelines: Minimize allocations in hot paths during performance optimization and avoid LINQ in performance-critical loops.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.AggregatorHub.cs` around lines 111 - 129, The code is allocating via LINQ in the hot OnAdd path when pruning _processedExecutionIds; replace the Where/Select/ToList and OrderBy/Take/ToList calls with imperative loops to avoid temporary enumerables: for the age-based prune (using pruneThreshold) iterate over _processedExecutionIds with a simple foreach and add matching keys to a List<string> toRemove, then remove them; for the hard-limit branch compute numberToRemove = _processedExecutionIds.Count - (maxExecutionIdCacheSize / 2), copy entries into a List<KeyValuePair<string,long>> (or a preallocated array), call List.Sort with a Comparison that compares kvp.Value, then take the first numberToRemove keys into toRemove and remove them from _processedExecutionIds; keep these changes inside the same lock and reference the existing symbols _processedExecutionIds, pruneThreshold, maxExecutionIdCacheSize and the containing method OnAdd.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/_common/Quotes/Quote.AggregatorHub.cs`:
- Around line 105-116: The pruning currently gated by _inputQuoteTracker.Count >
maxInputTrackerSize only removes entries older than pruneThreshold (computed
from item.Timestamp and AggregationPeriod) using LINQ (.Where/.Select/.ToList),
which can leave the dictionary above the hard cap and allocates on every add;
change pruning to enforce a hard cap by removing the oldest entries until
_inputQuoteTracker.Count <= maxInputTrackerSize and avoid LINQ allocations on
the hot path—e.g., compute pruneThreshold once, iterate the dictionary with a
simple foreach or manual enumerator to identify and remove keys (or repeatedly
remove the oldest timestamp key using a maintained min-key or queue) rather than
using .Where/.Select/.ToList, and stop as soon as the cap is reached so the loop
does minimal work.
In `@src/_common/Quotes/Tick.AggregatorHub.cs`:
- Around line 35-42: The constructor currently assigns AggregationPeriod =
periodSize.ToTimeSpan() without validating periodSize, which can yield
TimeSpan.Zero for unsupported enum values and hang when FillGaps is true; fix by
calling periodSize.ToTimeSpan() into a local (e.g., var agg =
periodSize.ToTimeSpan()), validate that agg != TimeSpan.Zero (or that periodSize
== PeriodSize.Month if that is the only zero-allowed case) and throw an
ArgumentOutOfRangeException(nameof(periodSize), periodSize, "Unsupported
PeriodSize") for invalid values before assigning AggregationPeriod and computing
_executionIdRetentionPeriod; leave FillGaps logic untouched but rely on the
validated AggregationPeriod to prevent the hang.
---
Duplicate comments:
In `@src/_common/Quotes/Quote.AggregatorHub.cs`:
- Around line 15-31: The QuoteAggregatorHub constructor overload
(QuoteAggregatorHub(IQuoteProvider<IQuote>, PeriodSize, bool)) throws an
ArgumentException when periodSize == PeriodSize.Month but the XML docs are
missing an <exception> entry; update the constructor's XML documentation to
include an <exception cref="System.ArgumentException"> describing when it is
thrown (e.g., "Month aggregation is not supported in streaming mode; use
TimeSpan overload for custom periods") so the public API docs reflect the thrown
exception for the periodSize parameter.
In `@src/_common/Quotes/Tick.StreamHub.cs`:
- Around line 40-47: Update the exception message to include the supplied
offending value so it can be diagnosed: when validating maxCacheSize against
absoluteMaxCacheSize in the Tick.StreamHub code, augment the constructed message
string to incorporate the actual maxCacheSize value (and keep the existing
bounds info), and then throw the ArgumentOutOfRangeException as before using
nameof(maxCacheSize), maxCacheSize, and the new message; ensure you reference
the existing variables maxCacheSize and absoluteMaxCacheSize and the same throw
site so the parameter name and value are present in logs.
- Around line 114-124: The current code in the tick add path overwrites
Cache[index] when hasExecutionId && hasCachedExecutionId but
Cache[index].ExecutionId != result.ExecutionId, dropping a valid trade; instead
modify the cache to preserve both executions for the same timestamp (e.g.,
change Cache storage at that key to support multiple ticks such as a small
list/collection or an aggregate container) and stop assigning Cache[index] =
result when ExecutionId differs. Update the write path where this check occurs
and ensure NotifyObserversOnAdd (and any consumers that read Cache[index])
accept an added tick rather than assuming a single entry—keep the existing
cached tick and append the new result to the per-index collection, then call
NotifyObserversOnAdd with the new tick and index (or with both ticks as
required) so rebuilds can use the provider cache as the full-history source.
---
Nitpick comments:
In `@src/_common/Quotes/Tick.AggregatorHub.cs`:
- Around line 111-129: The code is allocating via LINQ in the hot OnAdd path
when pruning _processedExecutionIds; replace the Where/Select/ToList and
OrderBy/Take/ToList calls with imperative loops to avoid temporary enumerables:
for the age-based prune (using pruneThreshold) iterate over
_processedExecutionIds with a simple foreach and add matching keys to a
List<string> toRemove, then remove them; for the hard-limit branch compute
numberToRemove = _processedExecutionIds.Count - (maxExecutionIdCacheSize / 2),
copy entries into a List<KeyValuePair<string,long>> (or a preallocated array),
call List.Sort with a Comparison that compares kvp.Value, then take the first
numberToRemove keys into toRemove and remove them from _processedExecutionIds;
keep these changes inside the same lock and reference the existing symbols
_processedExecutionIds, pruneThreshold, maxExecutionIdCacheSize and the
containing method OnAdd.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/settings.jsonsrc/_common/Quotes/Quote.AggregatorHub.cssrc/_common/Quotes/Tick.AggregatorHub.cssrc/_common/Quotes/Tick.StreamHub.cssrc/_common/Quotes/Tick.cssrc/_common/StreamHub/StreamHub.Observable.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/_common/StreamHub/StreamHub.Observable.cs
Owner
|
@claude[agent] ensure adequate code coverage, it should include integration tests using the SSE server test rig. |
…atorHub - Add QuoteAggregatorHub_WithSseStream_MatchesSeriesExactly integration test - Add TickAggregatorHub_WithSseStream_MatchesSeriesExactly integration test - Add ConsumeQuotesFromSse overload for TickHub (converts quotes to ticks) - Add ApplyQuoteAction overload for TickHub (handles tick-based operations) - Use separate ports (5101, 5102) for aggregator tests to avoid conflicts Tests verify that aggregator hubs correctly process streaming quotes/ticks and produce results matching static series computed on aggregated quotes. Both tests chain aggregator -> EMA to validate end-to-end streaming pipeline. Co-authored-by: DaveSkender <8432125+DaveSkender@users.noreply.github.com>
Contributor
Added in 108edfc. Integration Tests Added:
Both tests use the SSE server test rig to stream 2000 quotes and validate that:
Test Results:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/_common/Quotes/Tick.StreamHub.cs (1)
114-123:⚠️ Potential issue | 🟠 MajorDo not overwrite same-timestamp ticks with different
ExecutionIdin standalone mode.Line 118 replaces the prior trade. In standalone mode, rebuilds replay from this cache, so overwritten ticks disappear and downstream aggregates can drift after rebuilds.
🔧 Suggested fix
if (hasExecutionId && hasCachedExecutionId && Cache[index].ExecutionId != result.ExecutionId) { - // Different execution IDs at same timestamp - both are valid trades; - // replace the cached entry and notify as a new addition so aggregators process it - Cache[index] = result; + // Different execution IDs at same timestamp - persist both trades + int insertIndex = index; + while (insertIndex + 1 < Cache.Count && Cache[insertIndex + 1].Timestamp == result.Timestamp) + { + insertIndex++; + } + Cache.Insert(insertIndex + 1, result); if (notify) { - NotifyObserversOnAdd(result, index); + NotifyObserversOnAdd(result, insertIndex + 1); } return; }Based on learnings: when streaming hubs rebuild, cache replay must realign with full-history series results; mismatches indicate a streaming hub bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.StreamHub.cs` around lines 114 - 123, When encountering same-timestamp ticks with differing ExecutionId (the if branch that checks hasExecutionId && hasCachedExecutionId && Cache[index].ExecutionId != result.ExecutionId), do not overwrite the cached entry when running in standalone/rebuild mode; instead skip replacing Cache[index] and skip calling NotifyObserversOnAdd(result, index). Modify the branch to check the standalone flag used by the stream hub (e.g., IsStandalone or _standalone) and only perform Cache[index] = result and NotifyObserversOnAdd(...) when not in standalone mode so cached ticks remain stable for replay and aggregates remain aligned.src/_common/Quotes/Quote.AggregatorHub.cs (1)
109-121:⚠️ Potential issue | 🟠 Major
_inputQuoteTrackerpruning does not guarantee bounded growth.Current pruning is age-based only, so size can remain above
maxInputTrackerSizeindefinitely on high-throughput streams. This also allocates on every prune path.🔧 Suggested direction
- if (_inputQuoteTracker.Count > maxInputTrackerSize) + if (_inputQuoteTracker.Count > maxInputTrackerSize) { DateTime pruneThreshold = item.Timestamp.Add(-10 * AggregationPeriod); - List<DateTime> toRemove = _inputQuoteTracker - .Where(kvp => kvp.Key < pruneThreshold) - .Select(kvp => kvp.Key) - .ToList(); - - foreach (DateTime key in toRemove) - { - _inputQuoteTracker.Remove(key); - } + // 1) remove stale entries + // 2) if still above cap, remove oldest until count <= maxInputTrackerSize + // (prefer allocation-light iteration over LINQ in this hot path) }As per coding guidelines: “Minimize allocations in hot paths” and “Avoid LINQ in performance-critical loops.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Quote.AggregatorHub.cs` around lines 109 - 121, The current pruning of _inputQuoteTracker uses an age-only LINQ allocation which can leave the dictionary size above maxInputTrackerSize under high throughput; change the logic in the pruning block (where _inputQuoteTracker, maxInputTrackerSize, AggregationPeriod and item.Timestamp are used) to perform a non-allocating, imperative pass: compute pruneThreshold = item.Timestamp.Add(-10 * AggregationPeriod), iterate the dictionary keys with a simple foreach/for loop (no LINQ) and remove entries that are older until the dictionary Count is <= maxInputTrackerSize or no more old entries exist, and avoid per-call allocations by not creating intermediate lists (or reuse a preallocated buffer if necessary).src/_common/Quotes/Tick.AggregatorHub.cs (1)
35-42:⚠️ Potential issue | 🔴 CriticalGuard unsupported
PeriodSizevalues before assigningAggregationPeriod.At Line 35, unmapped enum values can become
TimeSpan.Zero; withFillGaps=true, the loop at Line 158 never advances. This can hang ingestion.🔧 Suggested fix
- AggregationPeriod = periodSize.ToTimeSpan(); + TimeSpan aggregationPeriod = periodSize.ToTimeSpan(); + if (aggregationPeriod <= TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException( + nameof(periodSize), + periodSize, + $"'{nameof(periodSize)}' was '{periodSize}'. Unsupported aggregation period."); + } + + AggregationPeriod = aggregationPeriod;Also applies to: 158-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.AggregatorHub.cs` around lines 35 - 42, Validate the incoming periodSize before assigning AggregationPeriod: call periodSize.ToTimeSpan(), ensure it returns a non-zero, supported TimeSpan, and if it is zero/unsupported throw a clear ArgumentException or fallback to a safe default; also prevent setting FillGaps=true when AggregationPeriod is zero by checking both (periodSize/ToTimeSpan and FillGaps) and rejecting/adjusting the inputs. Update the constructor/initializer that sets AggregationPeriod, FillGaps, Name and _executionIdRetentionPeriod (referencing AggregationPeriod, periodSize, FillGaps, _executionIdRetentionPeriod) to perform this guard so the later gap-filling loop (the code that advances based on AggregationPeriod) cannot hang on a zero TimeSpan.
🧹 Nitpick comments (1)
src/_common/Quotes/Tick.AggregatorHub.cs (1)
111-129: Avoid LINQ-heavy pruning in the tick hot path.This pruning path allocates and sorts on frequent
OnAddcalls while holdingCacheLock. Consider an allocation-light pruning strategy (manual iteration / bounded queue) to reduce GC and lock hold time.As per coding guidelines: “Minimize allocations in hot paths” and “Avoid LINQ in performance-critical loops.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.AggregatorHub.cs` around lines 111 - 129, The current LINQ usage in the pruning code (_processedExecutionIds.Where(...).Select(...).ToList() and OrderBy(...).Take(...).Select(...).ToList()) allocates and sorts on the hot OnAdd/CacheLock path; replace both LINQ blocks with allocation-light manual iterations: (1) iterate over _processedExecutionIds with a foreach to collect keys whose Value < pruneThreshold into a List<string> and remove them, and (2) when _processedExecutionIds.Count > maxExecutionIdCacheSize compute numberToRemove = _processedExecutionIds.Count - (maxExecutionIdCacheSize / 2), copy entries into a KeyValuePair<string,long>[] (or List) and call Array.Sort with a comparer on Value, then take the first numberToRemove keys into a removal list and remove them from _processedExecutionIds; this removes LINQ allocations while keeping logic tied to _processedExecutionIds, pruneThreshold, maxExecutionIdCacheSize and CacheLock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/_common/Quotes/Quote.AggregatorHub.cs`:
- Around line 38-39: The assigned AggregationPeriod is using
periodSize.ToTimeSpan() without validating it, which can return TimeSpan.Zero
for unmapped enum values and cause the FillGaps loop to never terminate; after
setting AggregationPeriod (and before any looping logic) validate that
AggregationPeriod != TimeSpan.Zero (or map unknown PeriodSize to a safe default)
and if it is zero throw a clear ArgumentException including the offending
periodSize value; also guard the gap-filling loop that relies on
AggregationPeriod (the code referencing AggregationPeriod and FillGaps in this
class) to bail out or throw when AggregationPeriod is zero to prevent infinite
loops.
---
Duplicate comments:
In `@src/_common/Quotes/Quote.AggregatorHub.cs`:
- Around line 109-121: The current pruning of _inputQuoteTracker uses an
age-only LINQ allocation which can leave the dictionary size above
maxInputTrackerSize under high throughput; change the logic in the pruning block
(where _inputQuoteTracker, maxInputTrackerSize, AggregationPeriod and
item.Timestamp are used) to perform a non-allocating, imperative pass: compute
pruneThreshold = item.Timestamp.Add(-10 * AggregationPeriod), iterate the
dictionary keys with a simple foreach/for loop (no LINQ) and remove entries that
are older until the dictionary Count is <= maxInputTrackerSize or no more old
entries exist, and avoid per-call allocations by not creating intermediate lists
(or reuse a preallocated buffer if necessary).
In `@src/_common/Quotes/Tick.AggregatorHub.cs`:
- Around line 35-42: Validate the incoming periodSize before assigning
AggregationPeriod: call periodSize.ToTimeSpan(), ensure it returns a non-zero,
supported TimeSpan, and if it is zero/unsupported throw a clear
ArgumentException or fallback to a safe default; also prevent setting
FillGaps=true when AggregationPeriod is zero by checking both
(periodSize/ToTimeSpan and FillGaps) and rejecting/adjusting the inputs. Update
the constructor/initializer that sets AggregationPeriod, FillGaps, Name and
_executionIdRetentionPeriod (referencing AggregationPeriod, periodSize,
FillGaps, _executionIdRetentionPeriod) to perform this guard so the later
gap-filling loop (the code that advances based on AggregationPeriod) cannot hang
on a zero TimeSpan.
In `@src/_common/Quotes/Tick.StreamHub.cs`:
- Around line 114-123: When encountering same-timestamp ticks with differing
ExecutionId (the if branch that checks hasExecutionId && hasCachedExecutionId &&
Cache[index].ExecutionId != result.ExecutionId), do not overwrite the cached
entry when running in standalone/rebuild mode; instead skip replacing
Cache[index] and skip calling NotifyObserversOnAdd(result, index). Modify the
branch to check the standalone flag used by the stream hub (e.g., IsStandalone
or _standalone) and only perform Cache[index] = result and
NotifyObserversOnAdd(...) when not in standalone mode so cached ticks remain
stable for replay and aggregates remain aligned.
---
Nitpick comments:
In `@src/_common/Quotes/Tick.AggregatorHub.cs`:
- Around line 111-129: The current LINQ usage in the pruning code
(_processedExecutionIds.Where(...).Select(...).ToList() and
OrderBy(...).Take(...).Select(...).ToList()) allocates and sorts on the hot
OnAdd/CacheLock path; replace both LINQ blocks with allocation-light manual
iterations: (1) iterate over _processedExecutionIds with a foreach to collect
keys whose Value < pruneThreshold into a List<string> and remove them, and (2)
when _processedExecutionIds.Count > maxExecutionIdCacheSize compute
numberToRemove = _processedExecutionIds.Count - (maxExecutionIdCacheSize / 2),
copy entries into a KeyValuePair<string,long>[] (or List) and call Array.Sort
with a comparer on Value, then take the first numberToRemove keys into a removal
list and remove them from _processedExecutionIds; this removes LINQ allocations
while keeping logic tied to _processedExecutionIds, pruneThreshold,
maxExecutionIdCacheSize and CacheLock.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/settings.jsonsrc/_common/Quotes/Quote.AggregatorHub.cssrc/_common/Quotes/Tick.AggregatorHub.cssrc/_common/Quotes/Tick.StreamHub.cssrc/_common/Quotes/Tick.cssrc/_common/StreamHub/StreamHub.Observable.cs
Claude
AI
changed the title
DaveSkender
changed the title
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/indicators/_common/Quotes/Tick.AggregatorHub.Tests.cs (1)
361-382:⚠️ Potential issue | 🟠 Major
MatchesSeriesExactlyis still set-based here, not strict ordered parity.At Line 364 and Line 411, dictionary membership checks validate per-key values but do not enforce direct ordered sequence equality (and the pre-removal path omits an equal-count assertion). Use ordered expected lists and
IsExactly(...)before and after removal.As per coding guidelines: “StreamHub unit tests must compare results to Series with strict ordering to verify parity.”
Also applies to: 403-425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/indicators/_common/Quotes/Tick.AggregatorHub.Tests.cs` around lines 361 - 382, The test currently builds a Dictionary (referenceMap) and uses per-key assertions against afterLateArrival which enforces set membership but not strict ordering; update the MatchesSeriesExactly test to compare ordered sequences: construct an ordered expected list from referenceQuotes (preserving sequence), assert counts equal (expected.Count == afterLateArrival.Count) then replace the per-bar dictionary checks with a single strict sequence assertion using IsExactly(expected) on afterLateArrival, and repeat the same ordered IsExactly(...) check after the removal path to ensure strict ordered parity both before and after removal; reference symbols: MatchesSeriesExactly, referenceQuotes, referenceMap, afterLateArrival.src/_common/Quotes/Tick.StreamHub.cs (1)
114-144:⚠️ Potential issue | 🟠 MajorPersist additive same-timestamp ticks in cache before notifying observers.
At Line 114 and Line 129, the code replaces
Cache[index]but still notifies observers as an add. That drops previously seen trades from standalone cache, so any later rebuild can replay incomplete history and undercount aggregates.🔧 Suggested fix (preserve additive events)
if (hasExecutionId && hasCachedExecutionId && Cache[index].ExecutionId != result.ExecutionId) { - // Different execution IDs at same timestamp - both are valid trades; - // replace the cached entry and notify as a new addition so aggregators process it - Cache[index] = result; + // Different execution IDs at same timestamp are distinct trades; + // persist both so rebuild/replay remains lossless. + Cache.Insert(index + 1, result); if (notify) { - NotifyObserversOnAdd(result, index); + NotifyObserversOnAdd(result, index + 1); } return; }Based on learnings: “When streaming hubs receive late inserts or revisions … they should rebuild from that point in time using the provider cache as the source … any mismatch indicates a streaming hub bug.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.StreamHub.cs` around lines 114 - 144, The code currently overwrites Cache[index] when a tick with the same timestamp but a different ExecutionId arrives and then calls NotifyObserversOnAdd, which loses the previously cached trade; change the logic in the branch that detects hasExecutionId && hasCachedExecutionId && Cache[index].ExecutionId != result.ExecutionId so that you persist both entries (e.g., insert or append the new result into the cache at/around index or otherwise record multiple entries for that timestamp) instead of replacing the existing Cache[index], and only after the cache has been updated (preserving the old entry) call NotifyObserversOnAdd(result, index); apply the same preservation behavior for the path that treats "no ExecutionIds" as an addition so prior cached trades are not dropped when calling NotifyObserversOnAdd or NotifyObserversOnRebuild.
🧹 Nitpick comments (2)
tools/scripts/dotnet-clean.sh (2)
69-73: Restore step should be optional in a clean script.Running
dotnet restoreimmediately after cleanup repopulates generated assets, so the repo is no longer left in a truly cleaned state. Consider making restore opt-in (e.g.,--restoreflag).Proposed change
-echo "→ Restoring packages..." -dotnet restore +if [[ "${1:-}" == "--restore" ]]; then + echo "→ Restoring packages..." + dotnet restore --nologo --verbosity quiet +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/dotnet-clean.sh` around lines 69 - 73, The script currently always runs dotnet restore (the "dotnet restore" invocation) which repopulates generated assets; change it so restore is opt-in by adding a CLI flag (e.g., --restore) parsed at top of tools/scripts/dotnet-clean.sh and only execute the existing "dotnet restore" block when that flag is set; update messages around the restore step to reflect when it is skipped or performed and ensure exit codes/behavior remain unchanged when the flag is absent.
54-67: Avoid fully masking cleanup failures.
2>/dev/null || truehides real deletion errors (permissions, unexpected paths) and can produce false-positive success output. Prefer surfacing errors while continuing the loop.Proposed change
- find "${find_args[@]}" 2>/dev/null || true + if ! find "${find_args[@]}"; then + echo "⚠️ Failed to fully clean folder pattern: $folder" >&2 + fi @@ - find "${find_args[@]}" 2>/dev/null || true + if ! find "${find_args[@]}"; then + echo "⚠️ Failed to fully clean file pattern: $file" >&2 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/scripts/dotnet-clean.sh` around lines 54 - 67, The cleanup loop currently masks all find/rm failures with "2>/dev/null || true"; update the delete-files loop that builds find_args (variables DELETE_FILES, SKIPPED_FOLDERS, find_args) so failures are surfaced but do not abort the whole script: remove the blanket "|| true", capture the find command exit status, and if non-zero print a clear error message to stderr (including the file pattern and exit code) and continue to the next item; ensure you still suppress benign output if desired but do not discard stderr silently so permission or path errors are visible while the loop proceeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/indicators/_common/Quotes/Tick.AggregatorHub.Tests.cs`:
- Around line 187-225: The test only verifies structural/order properties of
aggregator.Results; update WithCachePruning_MatchesSeriesExactly to compute the
expected aggregated 1-minute bars from the original ticks (group ticks by
tick.Timestamp truncated to minute, produce Open/High/Low/Close/Timestamp per
group), then apply the cache pruning semantics (only the most recent
maxCacheSize groups retained) and assert exact numerical parity and count
between that expected list and TickAggregatorHub.Results (compare Timestamp,
Open, High, Low, Close for each entry). Locate the logic in the test method
using the existing TickHub provider.Add loop and the aggregator.Results
reference to replace/augment the current shape checks with these exact-value
assertions.
---
Duplicate comments:
In `@src/_common/Quotes/Tick.StreamHub.cs`:
- Around line 114-144: The code currently overwrites Cache[index] when a tick
with the same timestamp but a different ExecutionId arrives and then calls
NotifyObserversOnAdd, which loses the previously cached trade; change the logic
in the branch that detects hasExecutionId && hasCachedExecutionId &&
Cache[index].ExecutionId != result.ExecutionId so that you persist both entries
(e.g., insert or append the new result into the cache at/around index or
otherwise record multiple entries for that timestamp) instead of replacing the
existing Cache[index], and only after the cache has been updated (preserving the
old entry) call NotifyObserversOnAdd(result, index); apply the same preservation
behavior for the path that treats "no ExecutionIds" as an addition so prior
cached trades are not dropped when calling NotifyObserversOnAdd or
NotifyObserversOnRebuild.
In `@tests/indicators/_common/Quotes/Tick.AggregatorHub.Tests.cs`:
- Around line 361-382: The test currently builds a Dictionary (referenceMap) and
uses per-key assertions against afterLateArrival which enforces set membership
but not strict ordering; update the MatchesSeriesExactly test to compare ordered
sequences: construct an ordered expected list from referenceQuotes (preserving
sequence), assert counts equal (expected.Count == afterLateArrival.Count) then
replace the per-bar dictionary checks with a single strict sequence assertion
using IsExactly(expected) on afterLateArrival, and repeat the same ordered
IsExactly(...) check after the removal path to ensure strict ordered parity both
before and after removal; reference symbols: MatchesSeriesExactly,
referenceQuotes, referenceMap, afterLateArrival.
---
Nitpick comments:
In `@tools/scripts/dotnet-clean.sh`:
- Around line 69-73: The script currently always runs dotnet restore (the
"dotnet restore" invocation) which repopulates generated assets; change it so
restore is opt-in by adding a CLI flag (e.g., --restore) parsed at top of
tools/scripts/dotnet-clean.sh and only execute the existing "dotnet restore"
block when that flag is set; update messages around the restore step to reflect
when it is skipped or performed and ensure exit codes/behavior remain unchanged
when the flag is absent.
- Around line 54-67: The cleanup loop currently masks all find/rm failures with
"2>/dev/null || true"; update the delete-files loop that builds find_args
(variables DELETE_FILES, SKIPPED_FOLDERS, find_args) so failures are surfaced
but do not abort the whole script: remove the blanket "|| true", capture the
find command exit status, and if non-zero print a clear error message to stderr
(including the file pattern and exit code) and continue to the next item; ensure
you still suppress benign output if desired but do not discard stderr silently
so permission or path errors are visible while the loop proceeds.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/tasks.jsonsrc/_common/Quotes/Tick.StreamHub.cstests/indicators/_common/Quotes/Tick.AggregatorHub.Tests.cstests/indicators/_common/Quotes/Tick.StreamHub.Tests.cstests/integration/StreamHub.ThreadSafety.Sse.Tests.cstools/scripts/dotnet-clean.sh
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/_common/Quotes/Tick.StreamHub.cs (1)
114-125:⚠️ Potential issue | 🔴 CriticalPreserve same-timestamp ticks with different
ExecutionIdvalues in cache (do not overwrite).At Line 118, the different-
ExecutionIdbranch overwritesCache[index], but Line 122 emitsNotifyObserversOnAddas a new trade. That means live flow processes both trades, while standalone rebuilds can only replay one, causing downstream divergence afterRebuild().🐛 Suggested fix
if (hasExecutionId && hasCachedExecutionId && Cache[index].ExecutionId != result.ExecutionId) { - // Different execution IDs at same timestamp - both are valid trades; - // replace the cached entry and notify as a new addition so aggregators process it - Cache[index] = result; + // Different execution IDs at same timestamp are distinct trades; + // persist both so rebuilds can replay accurately. + int insertIndex = index; + while (insertIndex < Cache.Count && Cache[insertIndex].Timestamp == result.Timestamp) + { + insertIndex++; + } + Cache.Insert(insertIndex, result); if (notify) { - NotifyObserversOnAdd(result, index); + NotifyObserversOnAdd(result, insertIndex); } return; }Based on learnings, streaming hubs must rebuild from cached/provider history and stay aligned with full-history results; dropping one same-timestamp trade from cache breaks that contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.StreamHub.cs` around lines 114 - 125, The branch that handles hasExecutionId && hasCachedExecutionId currently overwrites Cache[index] when ExecutionIds differ, which loses one of the same-timestamp ticks; instead preserve both ticks in the cache and still emit NotifyObserversOnAdd so rebuilds match live stream. Concretely: remove the assignment Cache[index] = result in the different-ExecutionId branch and change the cache update to insert or append the new result alongside the existing entry (e.g., push/insert result at the same timestamp slot or use a per-timestamp list), then call NotifyObserversOnAdd(result, index) as before; ensure Rebuild() and any cache readers handle multiple entries per timestamp (adjust data structure access in Tick.StreamHub, Cache and related methods if necessary).
🧹 Nitpick comments (2)
src/_common/Quotes/Tick.AggregatorHub.cs (1)
115-136: Refactor execution-id pruning to avoid LINQ/sort allocations in the tick hot path.Line 118–136 allocates temporary lists and sorts dictionaries under
CacheLock. On high-rate streams this increases GC pressure and lock hold time.♻️ Suggested direction (queue-backed pruning, no LINQ in `OnAdd`)
private readonly Dictionary<string, DateTime> _processedExecutionIds = []; +private readonly Queue<(string Id, DateTime Timestamp)> _executionIdOrder = []; @@ _processedExecutionIds[item.ExecutionId] = item.Timestamp; + _executionIdOrder.Enqueue((item.ExecutionId, item.Timestamp)); - // Prune old execution IDs (by time or size) - if (_processedExecutionIds.Count > (maxExecutionIdCacheSize / 2)) - { - DateTime pruneThreshold = item.Timestamp.Add(-_executionIdRetentionPeriod); - List<string> toRemove = _processedExecutionIds - .Where(kvp => kvp.Value < pruneThreshold) - .Select(kvp => kvp.Key) - .ToList(); - - foreach (string key in toRemove) - { - _processedExecutionIds.Remove(key); - } - } - - // Hard limit: if still too large after pruning, remove oldest entries - if (_processedExecutionIds.Count > maxExecutionIdCacheSize) - { - List<string> toRemove = _processedExecutionIds - .OrderBy(kvp => kvp.Value) - .Take(_processedExecutionIds.Count - (maxExecutionIdCacheSize / 2)) - .Select(kvp => kvp.Key) - .ToList(); - - foreach (string key in toRemove) - { - _processedExecutionIds.Remove(key); - } - } + DateTime pruneThreshold = item.Timestamp.Add(-_executionIdRetentionPeriod); + while (_executionIdOrder.Count > 0) + { + (string id, DateTime ts) = _executionIdOrder.Peek(); + bool overSoftCap = _processedExecutionIds.Count > (maxExecutionIdCacheSize / 2); + bool overHardCap = _processedExecutionIds.Count > maxExecutionIdCacheSize; + bool expired = ts < pruneThreshold; + + if (!overHardCap && !(overSoftCap && expired)) + { + break; + } + + _executionIdOrder.Dequeue(); + if (_processedExecutionIds.TryGetValue(id, out DateTime keptTs) && keptTs == ts) + { + _processedExecutionIds.Remove(id); + } + }As per coding guidelines, “Minimize allocations in hot paths during performance optimization” and “Avoid LINQ in performance-critical loops.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_common/Quotes/Tick.AggregatorHub.cs` around lines 115 - 136, The pruning code inside the Tick.AggregatorHub (inside the OnAdd/OnTick hot path using CacheLock) creates temporary lists and sorts the dictionary via LINQ causing allocations and long lock holds; replace the LINQ-based Where/Select/OrderBy flows on _processedExecutionIds with allocation-free, imperative logic: while holding CacheLock iterate the dictionary once and either (a) remove stale keys directly by checking kvp.Value < pruneThreshold, and for the hard-limit case use a bounded min-heap or fixed-size array to track the oldest keys (or maintain a queue of insertion timestamps) so you can remove the required number without sorting, thus avoiding ToList/OrderBy and reducing GC pressure and lock time. Ensure you reference _processedExecutionIds, _executionIdRetentionPeriod, maxExecutionIdCacheSize and the existing pruneThreshold logic when implementing the change.tests/indicators/_common/Quotes/Tick.AggregatorHub.Tests.cs (1)
539-541: Missingaggregator.Unsubscribe()in cleanup.For consistency with other chained tests (e.g.,
ChainWithDownstreamIndicator_WorksCorrectlyat lines 181-183), the aggregator should also be unsubscribed before ending transmission.🔧 Suggested fix
// Cleanup observer.Unsubscribe(); + aggregator.Unsubscribe(); provider.EndTransmission();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/indicators/_common/Quotes/Tick.AggregatorHub.Tests.cs` around lines 539 - 541, The cleanup block is missing a call to unsubscribe the aggregator; add aggregator.Unsubscribe() before provider.EndTransmission() so the teardown mirrors other tests (e.g., ChainWithDownstreamIndicator_WorksCorrectly) — locate the test's cleanup where observer.Unsubscribe() and provider.EndTransmission() are called and insert aggregator.Unsubscribe() just after observer.Unsubscribe().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/scripts/dotnet-clean.sh`:
- Around line 69-70: The script currently runs an unconditional "dotnet restore"
in dotnet-clean.sh which breaks the clean semantics and wastes work; remove that
unconditional call or gate it behind an explicit flag/env (e.g. add parsing for
a --restore option or check an environment variable like RESTORE_PACKAGES) so
"dotnet restore" only runs when the caller requests it; update the echo line and
any usage/help text accordingly and ensure the symbol "dotnet restore" in
dotnet-clean.sh is the only place changed.
---
Duplicate comments:
In `@src/_common/Quotes/Tick.StreamHub.cs`:
- Around line 114-125: The branch that handles hasExecutionId &&
hasCachedExecutionId currently overwrites Cache[index] when ExecutionIds differ,
which loses one of the same-timestamp ticks; instead preserve both ticks in the
cache and still emit NotifyObserversOnAdd so rebuilds match live stream.
Concretely: remove the assignment Cache[index] = result in the
different-ExecutionId branch and change the cache update to insert or append the
new result alongside the existing entry (e.g., push/insert result at the same
timestamp slot or use a per-timestamp list), then call
NotifyObserversOnAdd(result, index) as before; ensure Rebuild() and any cache
readers handle multiple entries per timestamp (adjust data structure access in
Tick.StreamHub, Cache and related methods if necessary).
---
Nitpick comments:
In `@src/_common/Quotes/Tick.AggregatorHub.cs`:
- Around line 115-136: The pruning code inside the Tick.AggregatorHub (inside
the OnAdd/OnTick hot path using CacheLock) creates temporary lists and sorts the
dictionary via LINQ causing allocations and long lock holds; replace the
LINQ-based Where/Select/OrderBy flows on _processedExecutionIds with
allocation-free, imperative logic: while holding CacheLock iterate the
dictionary once and either (a) remove stale keys directly by checking kvp.Value
< pruneThreshold, and for the hard-limit case use a bounded min-heap or
fixed-size array to track the oldest keys (or maintain a queue of insertion
timestamps) so you can remove the required number without sorting, thus avoiding
ToList/OrderBy and reducing GC pressure and lock time. Ensure you reference
_processedExecutionIds, _executionIdRetentionPeriod, maxExecutionIdCacheSize and
the existing pruneThreshold logic when implementing the change.
In `@tests/indicators/_common/Quotes/Tick.AggregatorHub.Tests.cs`:
- Around line 539-541: The cleanup block is missing a call to unsubscribe the
aggregator; add aggregator.Unsubscribe() before provider.EndTransmission() so
the teardown mirrors other tests (e.g.,
ChainWithDownstreamIndicator_WorksCorrectly) — locate the test's cleanup where
observer.Unsubscribe() and provider.EndTransmission() are called and insert
aggregator.Unsubscribe() just after observer.Unsubscribe().
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.vscode/tasks.jsonsrc/_common/Quotes/Quote.AggregatorHub.cssrc/_common/Quotes/Tick.AggregatorHub.cssrc/_common/Quotes/Tick.StreamHub.cstests/indicators/_common/Quotes/Tick.AggregatorHub.Tests.cstests/indicators/_common/Quotes/Tick.StreamHub.Tests.cstests/integration/StreamHub.ThreadSafety.Sse.Tests.cstools/scripts/dotnet-clean.sh
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
Signed-off-by: Dave Skender <8432125+DaveSkender@users.noreply.github.com>
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.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.