You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Automated test and performance infrastructure to lock in Epic 5 gains (zero-allocation hot paths, sub-300µs latency), plus instrumentation and pools to prevent regressions. Aligns with EPIC-6 by adding benchmarks, unit tests, and CI gates that verify allocations and latency on every PR.
New Features
Benchmark suite using BenchmarkDotNet for BarUpdate, OrderCallbacks, and SIMA dispatch; replaces StandaloneBench.
Unit tests for FSM/Actor, order management, ThreadStatic safety, and snapshot concurrency (18/18 passing).
GitHub Actions workflow to run tests and benchmarks on PRs/pushes (.github/workflows/epic6-testing.yml).
Perf utilities: LatencyProbe and LatencyHistogram; zero-allocation LogBuffer (ThreadStatic) replacing string.Format in hot paths.
Object pools: UI snapshot pool and Order[] array pool to eliminate transient allocations in UI and SIMA propagation.
BUILD_TAG updated to 1111.011-epic6-testing for traceability.
Bug Fixes
StickyState version handling: after checksum passes, version mismatches now soft-migrate instead of looping on “integrity check failed”.
Added performance benchmarking infrastructure with latency monitoring for critical execution paths.
Introduced zero-allocation logging for hot paths to reduce memory overhead.
Added object pooling for UI snapshots and order arrays to optimize throughput.
Added NEXUS SYNC command for state synchronization.
New CI/CD workflow for performance lock-in testing.
Bug Fixes
Fixed infinite rollback loop when strategy version changes.
Chores
Updated build tag and project configuration for testing phase.
Greptile Summary
This PR introduces the EPIC-6 automated testing infrastructure: zero-allocation perf primitives (LatencyProbe, LogBuffer, LatencyHistogram, OrderArrayPool, UISnapshotPool), latency instrumentation on OnBarUpdate/OnMarketData/MonitorRmaProximity, BenchmarkDotNet harnesses, xUnit test suites, and a GitHub Actions CI workflow with DNA compliance gates.
LogBuffer.Format contains a silent correctness bug: alignment-only specifiers like {0,-28} are neither substituted nor forwarded to string.Format, so format placeholder text appears verbatim in dispatch logs instead of account names.
CI Lock-Free Audit is broken: -SimpleMatch causes the pattern 'lock\\s*\\(' to be matched as a literal string, so actual lock (...) statements would silently pass the gate.
LogBuffer.Format still allocates (params object[], boxing for every value-type arg, and new string(...)) on the claimed "zero-allocation" path, contradicting the Jane Street ALLOCATION IS A BUG principle for the hot paths it instruments.
Confidence Score: 2/5
The PR introduces a correctness bug in the central LogBuffer.Format abstraction that silently corrupts every dispatch log line using column-alignment formatting, and the CI Lock-Free Audit gate is misconfigured in a way that would pass even if lock() were reintroduced.
LogBuffer.Format is the foundational primitive adopted across SIMA.Execution, SIMA.Dispatch, Entries.RMA, and SignalBroadcaster. Its failure to substitute {0,-28}-style arguments without triggering a fallback means account names are replaced with raw placeholder text in the dispatched order log — the exact output used for fleet auditing. Separately, the Lock-Free Audit in CI uses -SimpleMatch with a regex pattern, rendering the gate inert. Both issues need to be fixed before this infrastructure can serve as a reliable performance lock-in baseline.
src/V12_002.Perf.LogBuffer.cs (the FormatInternal fallback logic for alignment specifiers) and .github/workflows/epic6-testing.yml (the Lock-Free Audit step) require fixes before merge.
Important Files Changed
Filename
Overview
src/V12_002.Perf.LogBuffer.cs
New zero-allocation string formatter with two bugs: alignment specifiers like {0,-28} are emitted verbatim instead of substituted, corrupting dispatch logs; and the zero-allocation claim is false due to params object[] boxing and new string() allocation on every call.
.github/workflows/epic6-testing.yml
New CI workflow; the Lock-Free Audit step uses -SimpleMatch with a regex pattern, making the gate ineffective — it would never detect actual lock() statements. Complexity audit silently skips if the script is absent.
src/V12_002.Perf.LatencyHistogram.cs
New lock-free histogram using Interlocked operations; GetPercentile allocates a redundant local long[] on every call and the Buckets property is mutable despite the class being documented as an immutable snapshot.
src/V12_002.Perf.LatencyProbe.cs
New zero-allocation struct probe using Stopwatch.GetTimestamp(); correctly immutable, AggressiveInlining applied, Start/Stop pattern sound.
src/V12_002.Orders.ArrayPool.cs
New Order[1] array pool backed by ConcurrentBag, pre-warmed with 20 instances; design is sound but callers must ensure Return() is called after Rent() in all code paths to avoid pool starvation.
src/V12_002.Entries.RMA.cs
MonitorRmaProximity refactored into smaller helpers and instrumented with LatencyProbe, but the probe is missing a finally block — the fast-return path when RmaIntelligenceEnabled is false is never recorded.
src/V12_002.BarUpdate.cs
LatencyProbe added to OnBarUpdate with correct try/finally pattern; LogBuffer.Format adopted throughout with simple {N} patterns that are correctly handled.
src/V12_002.SIMA.Execution.cs
Migrated to LogBuffer.Format throughout, but most log lines use {0,-28} alignment specifiers which LogBuffer silently emits verbatim (account names lost from dispatch logs) due to the bug in FormatInternal.
tests/V12_Performance.Tests/Core/FSMActorTests.cs
FSM Actor tests covering enqueue, ordering, and concurrency; StateTransition_AtomicUpdate test passes trivially because all threads set the same WORKING state, so it does not expose real race conditions.
src/V12_002.UI.SnapshotPool.cs
New ConcurrentBag-backed snapshot pool with pre-warming and pool-size cap; design is reasonable but there is a TOCTOU between the Volatile.Read check and the subsequent Add in ReturnPooledSnapshot.
Sequence Diagram
sequenceDiagram
participant NT as NinjaTrader Thread
participant BU as OnBarUpdate
participant MD as OnMarketData
participant RMA as MonitorRmaProximity
participant LB as LogBuffer.Format
participant LP as LatencyProbe
participant LH as LatencyHistogram
NT->>BU: bar tick
BU->>LP: Start()
BU->>LB: Format(msg, args...)
Note over LB: params object[] allocated<br/>value types boxed<br/>{0,-28} emitted verbatim
LB-->>BU: new string(buffer)
BU->>BU: strategy logic
BU->>LP: Stop()
BU->>LH: Record(probe)
NT->>MD: market tick
MD->>LP: Start()
MD->>MD: strategy logic
MD->>LP: Stop()
MD->>LH: Record(probe)
NT->>RMA: MonitorRmaProximity()
RMA->>LP: Start()
alt "RmaIntelligenceEnabled = false"
RMA-->>NT: return (probe never recorded)
else enabled
RMA->>RMA: iterate orders
Note over RMA: no finally block
end
Adds perf instrumentation utilities and integrates them across strategy hot paths, introduces pooling for UI snapshots and order arrays, refactors order callbacks and propagation, updates lifecycle and broadcasting, adds benchmarks and unit tests, creates a CI workflow, and updates extensive epic documentation and command specs.
Implements LatencyProbe/Histogram and LogBuffer; adds UI snapshot and order array pools; refactors callbacks/propagation/lifecycle and broadcasting; adds BenchmarkDotNet suites and xUnit tests; introduces CI workflow; broad documentation and command updates.
A rabbit taps keys in the moon’s faint glow,
Hops through histograms where latencies flow.
Pools for snapshots, arrays that return,
Benchmarks thrum softly as progress we learn.
Logs buffer whispers, CI lights green—
Epic by epic, we polish the machine. 🐇✨
✨ Finishing Touches📝 Generate docstrings
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch feature/epic5-perf-optimization
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
This PR introduces comprehensive testing infrastructure for EPIC-6, including benchmark harnesses, unit tests for lock-free patterns, and CI/CD integration. While the testing approach is solid, I've identified 3 critical defects that must be fixed before merge:
Critical Issues (Must Fix)
Race condition in LogBuffer.ValidateThreadAffinity - ThreadStatic field updates lack synchronization, causing duplicate thread affinity warnings
Logic error in OrderManagementTests.CancelOrder - Interlocked.CompareExchange operates on a local variable copy, breaking the atomic guarantee and allowing multiple concurrent cancellations to succeed
Memory leak in OrderArrayPool.Return - Unbounded ConcurrentBag growth from double-returns can cause OutOfMemoryException under sustained load
Testing Infrastructure Assessment
Strengths:
Comprehensive test coverage for FSM/Actor patterns and concurrent order management
BenchmarkDotNet harnesses ready for performance assertions
CI/CD workflow includes DNA compliance gates (ASCII-only, lock-free audit)
Concerns:
The test mock for CancelOrder doesn't correctly demonstrate the atomic compare-exchange pattern it claims to validate
Missing test coverage for OrderArrayPool double-return scenarios
Action Required
Address the 3 critical defects before merging. The Interlocked.CompareExchange fix is particularly important as it affects the core lock-free guarantees being tested by this infrastructure.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
⚠️ This PR contains more than 30 files. Amazon Q is better at reviewing smaller PRs, and may miss issues in larger changesets.
The reason will be displayed to describe this comment to others. Learn more.
🛑 Race Condition: ThreadStatic fields _threadId and _threadIdInitialized lack synchronized access in ValidateThreadAffinity. The assignment on line 132 _threadId = currentThreadId after detecting a mismatch can race with the check on line 128, causing the same thread ID change to be detected and logged multiple times.
The reason will be displayed to describe this comment to others. Learn more.
🛑 Logic Error: Interlocked.CompareExchange on line 205 operates on a local variable currentState which is copied from data.State on line 201. The local variable update does not propagate back to data.State. Line 209 manually updates data.State, but this creates a window where multiple threads could both read the same value, pass the comparison, and return true (violating the idempotent single-cancellation guarantee).
// Atomic compare-exchange: only cancel if currently WorkingintworkingState=(int)OrderState.Working;intcancelledState=(int)OrderState.Cancelled;// Cast to int reference for atomic operation
unsafe
{
fixed (OrderState*statePtr=&data.State){int*intStatePtr=(int*)statePtr;intoriginal=Interlocked.CompareExchange(ref*intStatePtr,cancelledState,workingState);returnoriginal==workingState;}}
The reason will be displayed to describe this comment to others. Learn more.
🛑 Crash Risk: OrderArrayPool.Return lacks null validation on the array element. If array[0] is never set or is already null, the assignment on line 51 is safe, but the unbounded growth of the ConcurrentBag when the pool is "returned" arrays that were never rented (or returned multiple times) creates a memory leak that can eventually cause OutOfMemoryException under sustained high-frequency operations.
Suggested change
publicvoidReturn(Order[]array)
{
if(array!=null&&array.Length==1)
{
array[0]=null;// Clear reference to prevent holding stale Order objects
_pool.Add(array);
}
}
publicvoidReturn(Order[]array)
{
if(array!=null&&array.Length==1)
{
array[0]=null;// Clear reference to prevent holding stale Order objects
// Limit pool size to prevent unbounded growth from double-returns
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fails to meet the stated 'zero-allocation' requirements and contains evidence of compilation errors (CS0111, CS0121) as documented in the provided error grid. Codacy analysis indicates the submission is not up to standards due to 261 new issues and significant structural debt in the core order state machine and SIMA execution engine.\n\nThe primary architectural concern is the LogBuffer implementation, which violates performance goals by performing heap allocations through string creation and boxing. Furthermore, the PR is missing the 18/18 tests and BenchmarkDotNet harnesses claimed in the summary, making the performance improvements unverified. Significant refactoring is required to address cyclomatic complexity in 'ProcessOnOrderUpdate' and code duplication across execution paths.
About this PR
The included 'NinjaTrader Grid.csv' documentation flags multiple compilation errors (CS0111, CS0121) related to ambiguous or duplicate members, suggesting the code is not stable in the target environment.
The PR implementation contradicts the 'zero-allocation' performance goal by using heap-allocating string operations in the LogBuffer. Additionally, the unit tests and benchmarks mentioned in the PR description are missing from the file changes.
1 comment outside of the diffsrc/V12_002.SIMA.Execution.cs
line 71 🟡 MEDIUM RISK
Redundant code detected for profit cap checks and report generation between Market and Bracket execution paths. Extract this logic into shared private methods in src/V12_002.SIMA.Execution.cs.
Test suggestions
LatencyProbe struct correctly calculates elapsed microseconds and handles invalid Start/Stop sequences.\n- [ ] LogBuffer.Format correctly replaces positional placeholders ({0}, {1}) using the ThreadStatic buffer.\n- [ ] LogBuffer.Format falls back to string.Format for complex specifiers (e.g., {0:F2}) or buffer overflows.\n- [ ] OrderArrayPool Rent/Return lifecycle clears references to prevent memory leaks while avoiding allocations.\n- [ ] UISnapshotPool preserves nested object allocations (Config, Compliance, LivePosition) across rentals.\n- [ ] StickyState.ValidateSnapshotIntegrity returns true when StrategyVersion differs but SHA256 checksum matches.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. LatencyProbe struct correctly calculates elapsed microseconds and handles invalid Start/Stop sequences.\n- [ ] LogBuffer.Format correctly replaces positional placeholders ({0}, {1}) using the ThreadStatic buffer.\n- [ ] LogBuffer.Format falls back to string.Format for complex specifiers (e.g., {0:F2}) or buffer overflows.\n- [ ] OrderArrayPool Rent/Return lifecycle clears references to prevent memory leaks while avoiding allocations.\n- [ ] UISnapshotPool preserves nested object allocations (Config, Compliance, LivePosition) across rentals.\n- [ ] StickyState.ValidateSnapshotIntegrity returns true when StrategyVersion differs but SHA256 checksum matches.
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
This call is reported as ambiguous in the attached NinjaTrader Grid.csv (CS0121), likely due to a duplicate member definition in another partial class file.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Method 'SubmitTrendSplitBrackets' is too long (148 lines), obscuring control flow. Extract the duplicate logic for EMA9 and EMA15 entry leg submission into a single helper method.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Method 'ProcessOnOrderUpdate' is too complex (CCN 16) and has too many parameters. Refactor by grouping parameters into a DTO and extracting state-specific handlers for Filled, Rejected, and Cancelled states.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The OrderArrayPool should implement a maximum capacity check in the Return method to prevent unbounded growth of the ConcurrentBag during high-activity periods. Additionally, telemetry metrics (RentCount, ReturnCount) are missing.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
The LogBuffer returns a 'new string', which is a heap allocation. This violates the 'zero-allocation' requirement for hot paths mentioned in the EPIC-5 objectives.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The current logic in FormatInternal triggers a fallback to string.Format when a colon is detected. Since hot-path logs frequently format prices with :F1 or :F2, this optimization is effectively disabled where it is needed most.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: Change this public constant to a 'static readonly' property to ensure consumers fetch the value at runtime. This prevents build synchronization issues and ensures the StickyState migration logic behaves predictably during upgrades.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
The ValidateThreadAffinity logic is flawed because it compares a [ThreadStatic] value against the ManagedThreadId. Once initialized, the value will always match the current thread, making it impossible to detect if strategy execution has migrated between threads.
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request implements the EPIC-5 performance optimization and EPIC-6 testing infrastructure, focusing on achieving zero-allocation hot paths and sub-300μs latency. Key changes include the introduction of LatencyProbe, LogBuffer, and object pooling for UIStateSnapshot and Order arrays, alongside extensive BenchmarkDotNet harnesses and unit tests. Feedback identifies several critical areas for improvement: the LogBuffer implementation currently causes heap allocations through boxing and ToString() calls, and its thread affinity validation is ineffective due to the use of [ThreadStatic] fields. Additionally, the use of a non-thread-safe HashSet for proximity tags violates the lock-free mandate, and the LogBuffer's fallback mechanism for format specifiers significantly limits its performance benefits.
The reason will be displayed to describe this comment to others. Learn more.
The call to args[argIndex]?.ToString() on this line causes a heap allocation for every argument processed, which contradicts the zero-allocation goal for the hot path. For value types like double or int, this also involves boxing. To achieve true zero-allocation performance, consider implementing custom formatting logic that writes primitive types directly into the char[] buffer.
The reason will be displayed to describe this comment to others. Learn more.
The telemetry fields _threadId and _threadIdInitialized are marked as [ThreadStatic], which means they are unique to each thread and initialized to their default values when a new thread accesses them. This makes the ValidateThreadAffinity logic at line 118 ineffective for detecting if multiple threads are accessing the LogBuffer class, as each thread will only ever see its own state and believe it is the first to initialize it.
The reason will be displayed to describe this comment to others. Learn more.
The ValidateThreadAffinity method fails to detect cross-thread usage because it relies on [ThreadStatic] state. To correctly implement this telemetry, use a shared static field with Interlocked operations to capture the ID of the first thread that uses the buffer and compare it against subsequent callers.
The reason will be displayed to describe this comment to others. Learn more.
HashSet<string> is not thread-safe. While the current architecture aims for single-threaded execution on the strategy thread, NinjaTrader's multi-threaded environment (including UI and IPC interactions) makes HashSet risky for state that might be accessed across boundaries. Per the repository style guide's mandate for lock-free kernels, consider using a ConcurrentDictionary<string, byte> to ensure thread-safe management of proximity tags without using banned internal locks.
References
Legacy lock(stateLock) is BANNED for internal logic. Use lock-free patterns. (link)
The reason will be displayed to describe this comment to others. Learn more.
The fallback to string.Format when a format specifier (e.g., :F2) is detected means that the LogBuffer optimization is bypassed for a large portion of hot-path log messages. This results in a performance penalty due to redundant parsing and telemetry increments before the eventual allocation-heavy fallback. Consider supporting basic numeric specifiers directly in FormatInternal to maintain zero-allocation performance for the most frequent log patterns.
FormatInternal only falls back to string.Format when it encounters a : inside a placeholder. Alignment-only specifiers like {0,-28} or {0,8} contain no colon, so the method does NOT trigger the fallback. Instead it outputs {, then the characters 0,-28} as individual literals, printing the raw placeholder text instead of the argument value. Any call-site using width/alignment formatting will silently log {0,-28} in place of the account name or other value.
Affected hot-path log lines in V12_002.SIMA.Execution.cs and V12_002.SIMA.Dispatch.cs include " SKIP | {0,-28} | Inactive", " OK | {0,-28} | Market | submitted", and " QUEUE | {0,-28} | Market+{1}orders | PENDING" — all will emit the raw placeholder rather than the account name. The PR CANNOT be merged until FormatInternal either detects non-colon format extras (, after the digit) and falls back, or the call-sites are changed to use simple {N} patterns.
The reason will be displayed to describe this comment to others. Learn more.
Lock-Free Audit gate is broken — always passes
Select-String with -SimpleMatch performs a literal string search, not a regex search. The pattern 'lock\s*\(' is therefore matched literally as the 9-character sequence lock\s*( (including the backslash-s). Normal lock ( or lock( usage in C# would never match this literal, so the gate silently passes even when lock() statements are present. The flag must be removed so PowerShell treats the string as a proper regular expression.
The reason will be displayed to describe this comment to others. Learn more.
[CRITICAL-JS-VIOLATION] "Zero-allocation" claim is false — hot-path allocations remain
LogBuffer.Format still performs heap allocations on the fast path: (1) the params object[] argument array is heap-allocated on each call, (2) every value-type argument (double, int, long) is boxed into an object on the heap, and (3) the final new string(_buffer, 0, length) always allocates a new string. The only allocation eliminated versus string.Format is the internal StringBuilder. For all the format strings in this codebase that contain :F2, :F3, or alignment specifiers (i.e., the vast majority of log calls in SIMA.Dispatch, SIMA.Execution, Entries.RMA), the method falls back to string.Format anyway, providing zero benefit. Jane Street Principle 1 — "Allocation is a Bug" — is violated on the documented hot paths (OnBarUpdate, OnMarketData). PR CANNOT be merged with this claim unaddressed.
The reason will be displayed to describe this comment to others. Learn more.
MonitorRmaProximity probe is never recorded on the fast-return path
LatencyProbe.Start() is called at method entry but there is no finally block. When RmaIntelligenceEnabled is false the method returns inside the try body and probe.Stop() / histogram recording are never reached. The fast-path timing — exactly the measurement needed for the claimed 65–100 µs P50 validation — is silently dropped from the histogram. A finally block mirroring the pattern used in OnBarUpdate and OnMarketData is needed.
The reason will be displayed to describe this comment to others. Learn more.
GetPercentile allocates a new long[] on every call
long[] boundaries = { 10, 50, 100, 500, 1000, 5000, long.MaxValue } creates a new heap-allocated array each time GetPercentile is invoked. ToAsciiString calls it three times per invocation (p50, p95, p99), yielding three allocations per report. This array duplicates BucketBoundaries from the outer class. Declaring it as private static readonly long[] (or reusing BucketBoundaries with long.MaxValue appended) eliminates the allocation and removes the maintenance hazard of the two arrays drifting apart.
The reason will be displayed to describe this comment to others. Learn more.
ValidateThreadAffinity is dead code — the warning branch can never fire
_threadId and _threadIdInitialized are both [ThreadStatic], meaning each OS thread has its own isolated copy. After the first call on any given thread, _threadId is set to that thread's ManagedThreadId. On every subsequent call from the same thread, Thread.CurrentThread.ManagedThreadId will always equal _threadId, so _threadId != currentThreadId is structurally impossible. _threadAffinityWarnings will permanently read zero and the T01B early-warning system described in comments is inoperative.
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Percentile calculation can return the wrong bucket because targetCount is truncated to zero for small sample sizes (for example, p50 with one sample). That makes the first bucket match immediately even when all samples are in higher buckets. Compute the rank as at least 1 (or use a ceil-based rank) before scanning buckets. [logic error]
1. Run the V12_002 strategy so that latency samples are recorded via
`LatencyProbe.Start()` and `_histOnBarUpdate.Record(probe)` in
`src/V12_002.BarUpdate.cs:240-247,347` (verified by Grep showing
`_histOnBarUpdate.Record(probe)` at line 347).
2. In a test or diagnostic helper for `V12_002` (same partial class in
`src/V12_002.cs:814-820` where the `LatencyHistogram` fields are declared), obtain a
snapshot after a single recorded sample by calling `_histOnBarUpdate.GetSnapshot()`, which
executes `HistogramSnapshot.GetSnapshot()` at
`src/V12_002.Perf.LatencyHistogram.cs:50-63`.
3. Call `snapshot.GetPercentile(50)` on that snapshot; execution enters `GetPercentile` at
`src/V12_002.Perf.LatencyHistogram.cs:116-137`, where `TotalSamples == 1` and `percentile
== 50`.
4. At `src/V12_002.Perf.LatencyHistogram.cs:123-131`, `targetCount` is computed as
`(long)(1 * 0.5)` which truncates to `0`, so on the first loop iteration `cumulativeCount`
(initially 0) satisfies `cumulativeCount >= targetCount` immediately, returning the first
boundary (10µs) even when the single sample was in a higher bucket, producing an incorrect
p50 value observable in debugger or logs.
This is a comment left during a code review.
**Path:** src/V12_002.Perf.LatencyHistogram.cs
**Line:** 123:124
**Comment:***Logic Error: Percentile calculation can return the wrong bucket because `targetCount` is truncated to zero for small sample sizes (for example, p50 with one sample). That makes the first bucket match immediately even when all samples are in higher buckets. Compute the rank as at least 1 (or use a ceil-based rank) before scanning buckets.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Returning long.MaxValue for the overflow bucket makes reported percentiles explode to an unusable value instead of a realistic upper-bound bucket value. Use a finite representative boundary for the last bucket (for example 5000) or a special "5000+us" sentinel in formatting logic. [logic error]
Severity Level: Major ⚠️
- ⚠️ High-percentile latencies print as 9e18 microseconds.
- ⚠️ Performance logs from `ToAsciiString()` become unusable.
- ⚠️ Operators cannot distinguish 5ms vs 50ms tail latencies.
Steps of Reproduction ✅
1. In a diagnostic or unit-test context, instantiate `LatencyHistogram` (defined at
`src/V12_002.Perf.LatencyHistogram.cs:11-27`) and record at least one latency sample whose
`ElapsedMicroseconds` exceeds 5000µs using `Record(LatencyProbe probe)` at lines 32-45,
which increments the overflow bucket (index 6 from `GetBucketIndex` at lines 80-90).
2. Obtain a snapshot by calling `GetSnapshot()` on that histogram (lines 50-63), yielding
a `HistogramSnapshot` where `TotalSamples > 0` and `Buckets[6]` contains the recorded
count, while earlier buckets may be zero.
3. Call `GetPercentile(99)` on the snapshot, which runs `HistogramSnapshot.GetPercentile`
at `src/V12_002.Perf.LatencyHistogram.cs:116-137`; `boundaries` is initialized with
`long.MaxValue` for the last bucket at line 126.
4. As the loop at lines 128-135 accumulates counts, the 99th percentile falls into the
overflow bucket, so `boundaries[6]` (i.e., `long.MaxValue`) is returned, and any caller
(including `ToAsciiString` at lines 140-166) will report p99 as `9223372036854775807us`,
an unusable and misleading latency figure.
This is a comment left during a code review.
**Path:** src/V12_002.Perf.LatencyHistogram.cs
**Line:** 126:135
**Comment:***Logic Error: Returning `long.MaxValue` for the overflow bucket makes reported percentiles explode to an unusable value instead of a realistic upper-bound bucket value. Use a finite representative boundary for the last bucket (for example 5000) or a special "5000+us" sentinel in formatting logic.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The new success log line is outside the order != null guard, so accounts where CreateOrder returns null are still logged as "submitted". This creates false-positive operational telemetry and hides failed dispatches; move the success log inside the same branch that actually submits. [incorrect condition logic]
1. Trigger a SIMA fleet market broadcast from the IPC layer by issuing a fleet command
that routes to `TryHandleFleet_SIMA` (or equivalent) and then calls
`ExecuteMultiAccountMarket(orderAction, qty, \"SIMA_\" + action)` in
`src/V12_002.UI.IPC.Commands.Fleet.cs:11-20`, which enqueues and eventually invokes
`ExecuteMultiAccountMarket` in `src/V12_002.SIMA.Execution.cs:41-157`.
2. Ensure there is at least one fleet account where `IsFleetAccount(acct)` and
`activeFleetAccounts[acct.Name]` are true (guarded at lines 62-69) but the account is
disconnected or has an invalid instrument pairing; per the comment in
`ProcessSingleFleetRMAAccount` at `src/V12_002.SIMA.Execution.cs:567-568`,
`acct.CreateOrder` returns null for such disconnected/invalid account/instrument pairs.
3. When `ExecuteMultiAccountMarket` iterates `Account.All`, it calls `Order order =
acct.CreateOrder(...)` at lines 87-98; for the misconfigured account, `order` is null so
the `if (order != null)` block at lines 100-108 (which reserves expected positions and
calls `acct.Submit`) is skipped, meaning no order is submitted for that account.
4. Despite the skipped submission, the code at lines 110-113 unconditionally increments
`successCount++` and appends `"OK | ... | Market | submitted"` to `dispatchLog`, so the
final forensic report built at lines 135-155 reports the account as successfully submitted
even though no order was created, leading operators and telemetry to falsely believe the
broadcast succeeded on that account.
This is a comment left during a code review.
**Path:** src/V12_002.SIMA.Execution.cs
**Line:** 87:113
**Comment:***Incorrect Condition Logic: The new success log line is outside the `order != null` guard, so accounts where `CreateOrder` returns null are still logged as "submitted". This creates false-positive operational telemetry and hides failed dispatches; move the success log inside the same branch that actually submits.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:LogBuffer.Format claims to be a drop-in replacement for string.Format, but alignment placeholders like {0,-28} are not detected as format specifiers and are emitted as literal text instead of substituting values. Add alignment-specifier handling (, in braces) or explicitly fall back to string.Format whenever a placeholder is not the strict {N} form. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ SIMA forensic latency reports show misformatted account columns.
- ⚠️ Queue diagnostics harder to read during incident investigation.
Steps of Reproduction ✅
1. Observe `LogBuffer.Format` implementation in `src/V12_002.Perf.LogBuffer.cs:56-112`,
where `FormatInternal()` scans for `':'` between `{` and `}` (lines 64-73) but does not
treat `','` (alignment) as a special case and only recognizes strict `{N}` patterns (lines
79-99).
2. Note that if no `':'` is found, `FormatInternal()` does not return `-1` and then only
replaces placeholders when `format[formatPos + 2] == '}'` (line 80), so a token like
`"{0,-28}"` fails this check and falls through to the "copy literal character" path (lines
102-108), leaving the `{0,-28}` text in the output.
3. Find real call sites using alignment placeholders without format specifiers, e.g. in
`src/V12_002.SIMA.Dispatch.cs:867` and `:1020` where
`dispatchLog.AppendLine(LogBuffer.Format(" QUEUE | {0,-28} | Market+{1}orders | PENDING",
acct.Name, ordersToSubmit.Count));` and `dispatchLog.AppendLine(LogBuffer.Format(" QUEUE |
{0,-28} | Limit | PENDING", acct.Name));` are used to build the SIMA forensic report.
4. Run a scenario that enqueues fleet dispatches (e.g. a SIMA smart dispatch entry that
reaches the code around `src/V12_002.SIMA.Dispatch.cs:850-889` and `:1010-1039`); when
those paths execute, the resulting report lines contain the literal `{0,-28}` placeholder
text instead of the left-aligned account name, because `LogBuffer.Format` does not
recognize the alignment form and never falls back to `string.Format` for this pattern.
This is a comment left during a code review.
**Path:** src/V12_002.Perf.LogBuffer.cs
**Line:** 69:77
**Comment:***Api Mismatch: `LogBuffer.Format` claims to be a drop-in replacement for `string.Format`, but alignment placeholders like `{0,-28}` are not detected as format specifiers and are emitted as literal text instead of substituting values. Add alignment-specifier handling (`,` in braces) or explicitly fall back to `string.Format` whenever a placeholder is not the strict `{N}` form.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Placeholder parsing only supports a single-digit index ({0}..{9}), so tokens like {10}/{11} are treated as literals and never replaced. This already breaks existing call sites that use 10+ arguments; parse full numeric indices (all digits until }) or fall back to string.Format when index length is more than one digit. [logic error]
Severity Level: Major ⚠️
- ⚠️ RMA deactivation CONFIG messages sent with invalid placeholders.
- ⚠️ Remote control panel may misparse or ignore configuration.
Steps of Reproduction ✅
1. Inspect `FormatInternal()` in `src/V12_002.Perf.LogBuffer.cs:56-112`; placeholder
parsing only recognizes `{N}` where `N` is a single digit and `format[formatPos + 2] ==
'}'` (lines 80-85). For `"{10}"`, `format[formatPos + 2]` is `'0'`, so the placeholder is
not recognized.
2. Because the `while (closingBrace ...)` loop (lines 69-77) only checks for `':'` and not
for multi-digit indices, `"{10}"` passes through without triggering the `return -1`
fallback and the `{` character is treated as a literal (lines 102-108), leaving the braces
and digits in the output.
3. See an actual multi-digit use in `src/V12_002.Entries.RMA.cs:13-27` inside
`DeactivateRMAMode()`, where `string deactivateConfig =
LogBuffer.Format("CONFIG|OR|COUNT:{0};T1:{1};T1TYPE:{2};T2:{3};T2TYPE:{4};T3:{5};T3TYPE:{6};T4:{7};T4TYPE:{8};T5:{9};T5TYPE:{10};STR:{11};MAX:{12};",
minContracts, Target1Value, ..., StopMultiplier, MaxRiskAmount);` uses indexes `{0}`
through `{12}`.
4. When RMA mode is deactivated via `ActivateMOMOMode()` in
`src/V12_002.Entries.MOMO.cs:5-13` (which calls `DeactivateRMAMode()` when
`isRMAModeActive` is true) and `SendResponseToRemote(deactivateConfig)` is executed in
`src/V12_002.Entries.RMA.cs:29-30`, the resulting IPC message sent by
`SendResponseToRemote()` (`src/V12_002.UI.IPC.Commands.Misc.cs:12-21`) contains unreplaced
`{10}`, `{11}`, `{12}` tokens instead of the expected numeric values, breaking the CONFIG
message contract with connected panel clients.
This is a comment left during a code review.
**Path:** src/V12_002.Perf.LogBuffer.cs
**Line:** 80:99
**Comment:***Logic Error: Placeholder parsing only supports a single-digit index (`{0}`..`{9}`), so tokens like `{10}`/`{11}` are treated as literals and never replaced. This already breaks existing call sites that use 10+ arguments; parse full numeric indices (all digits until `}`) or fall back to `string.Format` when index length is more than one digit.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Version mismatches are now accepted unconditionally with a "migrating state" message, but no migration step is actually performed before restoring fields. This can silently load incompatible snapshot schemas after upgrades and apply stale/default values. Either perform an explicit migration here or fail validation when versions differ. [incomplete implementation]
Severity Level: Critical 🚨
- ⚠️ Upgraded strategies silently hydrate state from older schema.
- ⚠️ Position size and enable flags may reflect outdated configuration.
Steps of Reproduction ✅
1. On an older build, `SaveStickyState()` in `src/V12_002.StickyState.cs:26-43` writes a
JSON snapshot via `WriteSnapshotAtomic()`, with `StateSnapshot.StrategyVersion` set to the
old `BUILD_TAG` (lines 39-48, 71-76).
2. After upgrading to a new build (different `BUILD_TAG`),
`OnStateChange(State.DataLoaded)` in `src/V12_002.Lifecycle.cs:773-776` sets
`_stickyStatePath` and calls `LoadStickyState()`, which in turn calls
`LoadStateSnapshot()` (lines 9-23, 16-21 in `V12_002.StickyState.cs:270-349`).
3.`LoadStateSnapshot()` calls `ValidateSnapshotIntegrity(snapshot, json)` (lines 117-120
and 17-27), which first validates the checksum (lines 21-39) and then executes the version
check at `src/V12_002.StickyState.cs:170-180`: when `snapshot.StrategyVersion !=
BUILD_TAG`, it logs `"[STICKY] Version mismatch detected: {0} -> {1}. Migrating state."`
and returns `true` without mutating `snapshot` or performing any migration.
4. Because `ValidateSnapshotIntegrity` returned `true`, `LoadStateSnapshot()` returns the
unmodified snapshot (line 30), and `LoadStickyState()` calls
`RestoreFromSnapshot(snapshot)` at `src/V12_002.StickyState.cs:16-20`, applying
potentially stale or schema-incompatible fields (e.g., `PositionSize`, `EnableSIMA`,
`AccountPositions`) from the previous version despite the logged "migrating state" message
and without any version-gated migration logic.
This is a comment left during a code review.
**Path:** src/V12_002.StickyState.cs
**Line:** 170:180
**Comment:***Incomplete Implementation: Version mismatches are now accepted unconditionally with a "migrating state" message, but no migration step is actually performed before restoring fields. This can silently load incompatible snapshot schemas after upgrades and apply stale/default values. Either perform an explicit migration here or fail validation when versions differ.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Returning the previously published snapshot to the pool immediately after swapping _uiSnapshot creates a race with UI readers that still hold a reference to the old snapshot. That object can be cleared/reused and mutated while the dispatcher is still rendering from it, leading to torn/inconsistent UI state. Keep old snapshots alive until readers are guaranteed done (or use immutable snapshots without pooling of published instances). [race condition]
Severity Level: Major ⚠️
- ⚠️ UI readers can observe partially cleared telemetry snapshots.
- ⚠️ Operator panel may briefly show inconsistent or blank state.
Steps of Reproduction ✅
1. The strategy declares a volatile UI cache `private volatile UIStateSnapshot _uiSnapshot
= new UIStateSnapshot();` in `src/V12_002.cs:182`, explicitly documented as a "Thread-safe
Value Cache for UI Telemetry", implying it is read from UI/dispatcher threads while being
updated on the data thread.
2. Market data ticks trigger `OnMarketData` in `src/V12_002.Lifecycle.cs:920-959`; after
updating `lastKnownPrice`, the handler rate-gates UI publishing and, every 5 ticks, calls
`PublishUiSnapshot()` at lines 929-931.
3. Inside `PublishUiSnapshot()` (`src/V12_002.UI.Snapshot.cs:22-80`), the method saves a
reference to the current snapshot (`UIStateSnapshot oldSnapshot = _uiSnapshot;` line 30),
builds a new snapshot from the pool, assigns `_uiSnapshot = snapshot;` (line 69), and then
immediately calls `ReturnPooledSnapshot(oldSnapshot)` if `oldSnapshot` is non-null (lines
71-73).
4.`ReturnPooledSnapshot()` in `src/V12_002.UI.SnapshotPool.cs:5-21` calls
`ClearSnapshotForReuse(snapshot)` (lines 10-11), which zeroes out all primitive fields and
strings on the reused `UIStateSnapshot` (lines 29-49). Any UI consumer that previously
captured a reference to the old `_uiSnapshot` instance (consistent with the "value cache"
design) can still be reading its fields while they are being reset, leading to transient,
torn, or inconsistent UI state (e.g., EMA values and status message briefly reverting to
defaults) during concurrent access.
This is a comment left during a code review.
**Path:** src/V12_002.UI.Snapshot.cs
**Line:** 257:262
**Comment:***Race Condition: Returning the previously published snapshot to the pool immediately after swapping `_uiSnapshot` creates a race with UI readers that still hold a reference to the old snapshot. That object can be cleared/reused and mutated while the dispatcher is still rendering from it, leading to torn/inconsistent UI state. Keep old snapshots alive until readers are guaranteed done (or use immutable snapshots without pooling of published instances).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The benchmark labeled as the OnBarUpdate hot path never invokes the real strategy OnBarUpdate logic and only reads mock fields, so this harness can pass even when the actual production hot path regresses in latency or allocations. Wire the benchmark to call the real execution path (or a thin adapter around it) instead of synthetic local-variable reads. [incomplete implementation]
Severity Level: Critical 🚨
- ⚠️ OnBarUpdate benchmark ignores real V12_002.OnBarUpdate logic.
- ⚠️ CI performance gates may miss OnBarUpdate latency regressions.
- ⚠️ Allocation regressions in OnBarUpdate remain undetected by benchmarks.
Steps of Reproduction ✅
1. Run the BenchmarkDotNet entrypoint in `benchmarks/Program.cs:11-15`
(`BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args)`), which discovers
and executes `BarUpdateBenchmark` in `benchmarks/BarUpdateBenchmark.cs`.
2. Observe that `BarUpdateBenchmark.Setup()` at `benchmarks/BarUpdateBenchmark.cs:20-43`
only initializes `_bar`, `_account`, and `_order` with `V12_Performance.Tests.Mocks`
instances and never references the real strategy class `V12_002` or its `OnBarUpdate`
implementation in `src/V12_002.BarUpdate.cs:238-309`.
3. For each iteration, BenchmarkDotNet invokes `OnBarUpdate_HotPath()` at
`benchmarks/BarUpdateBenchmark.cs:46-62`, which reads a few mock properties (`_bar.Time`,
`_bar.Close`, `_account.CashValue`, `_order.LimitPrice`) and performs a simple guard
check, but never calls `V12_002.OnBarUpdate()` or any adapter/wrapper around it.
4. If a performance regression is introduced inside the real `V12_002.OnBarUpdate()` body
(e.g., extra allocations in `DrainAccountMailbox`, `ProcessORWindowBuilding`, or
additional telemetry at `src/V12_002.BarUpdate.cs:31-40, 72-80, 92-99`), the
`OnBarUpdate_HotPath` benchmark continues to report the original allocation and latency
characteristics, so the EPIC-6 performance gate cannot detect regressions on the true
production hot path.
This is a comment left during a code review.
**Path:** benchmarks/BarUpdateBenchmark.cs
**Line:** 46:62
**Comment:***Incomplete Implementation: The benchmark labeled as the `OnBarUpdate` hot path never invokes the real strategy `OnBarUpdate` logic and only reads mock fields, so this harness can pass even when the actual production hot path regresses in latency or allocations. Wire the benchmark to call the real execution path (or a thin adapter around it) instead of synthetic local-variable reads.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The callback benchmarks do not call the actual order/execution callback handlers and instead benchmark only mock property reads, which gives false confidence for the production callback path and misses real regressions in callback logic. Benchmark the real callback entrypoints (or a direct extracted callback core) to make the performance gate meaningful. [incomplete implementation]
Severity Level: Critical 🚨
- ⚠️ Order callback benchmarks ignore actual OnAccountOrderUpdate path.
- ⚠️ CI performance gates may miss callback latency regressions.
- ⚠️ Allocation regressions in account callbacks stay undetected.
Steps of Reproduction ✅
1. Execute the benchmarks via `benchmarks/Program.Main` at `benchmarks/Program.cs:11-15`,
which uses `BenchmarkSwitcher` to run all benchmark classes including
`OrderCallbacksBenchmark` in `benchmarks/OrderCallbacksBenchmark.cs`.
2. In `OrderCallbacksBenchmark.Setup()` at `benchmarks/OrderCallbacksBenchmark.cs:20-40`,
note that `_order`, `_execution`, and `_account` are initialized with static mock data and
no reference is made to the real strategy `V12_002` or its account-order callback logic in
`src/V12_002.Orders.Callbacks.AccountOrders.cs`.
3. For each iteration of the `OnOrderUpdate_HotPath` benchmark at
`benchmarks/OrderCallbacksBenchmark.cs:43-57`, only simple property reads (`_order.Name`,
`_order.OrderState`, `_order.Quantity`, `_order.LimitPrice`) and a guard check are
executed; the real broker callback entrypoint `V12_002.OnAccountOrderUpdate(object sender,
OrderEventArgs e)` at `src/V12_002.Orders.Callbacks.AccountOrders.cs:8-50` and its core
helpers (`ProcessAccountOrder_UpdateMasterExpected`,
`ProcessAccountOrder_UpdateFleetExpected`, `ProcessAccountOrder_EnqueueTerminalUpdate` at
lines 52-123) are never invoked.
4. A latency or allocation regression in the production callback path—for example, more
expensive logic inside `ProcessAccountOrder_UpdateMasterExpected` at
`src/V12_002.Orders.Callbacks.AccountOrders.cs:52-86` or increased
enqueuing/TriggerCustomEvent overhead in `ProcessAccountOrder_EnqueueTerminalUpdate` at
lines 125-147—will not affect the timings of `OnOrderUpdate_HotPath`, so the EPIC-6
callback benchmarks will still pass while the real order/execution callbacks have
regressed.
This is a comment left during a code review.
**Path:** benchmarks/OrderCallbacksBenchmark.cs
**Line:** 43:57
**Comment:***Incomplete Implementation: The callback benchmarks do not call the actual order/execution callback handlers and instead benchmark only mock property reads, which gives false confidence for the production callback path and misses real regressions in callback logic. Benchmark the real callback entrypoints (or a direct extracted callback core) to make the performance gate meaningful.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The new latency logging executes outside the subscriber-isolation try/catch, so any exception from formatting or output emission will escape SafeInvoke and break broadcasting even though subscriber failures are supposed to be isolated. Keep logging inside a guarded block so diagnostics cannot take down signal fan-out. [missing cleanup]
Severity Level: Major ⚠️
- ⚠️ Diagnostic logging can break future signal fan-out isolation.
- ⚠️ Flatten/stop broadcasts risk exceptions from logging path.
Steps of Reproduction ✅
1. Inspect the broadcast API in `src/SignalBroadcaster.cs:241-259` (e.g.,
`BroadcastTradeSignal`, `BroadcastFlatten`, `BroadcastEntryUpdate`); note that while the
current repo has no call sites for these methods (verified via Grep on
`BroadcastTradeSignal`/`BroadcastFlatten`), they are public static entrypoints intended
for master/slave fan-out as documented in the class summary at
`src/SignalBroadcaster.cs:6-9`.
2. All broadcast methods call the shared `SafeInvoke<T>` helper at
`src/SignalBroadcaster.cs:200-239`, which iterates `handler.GetInvocationList()` and
invokes each subscriber inside a `try/catch` block at lines 14-23 to enforce subscriber
isolation.
3. After the subscriber loop, `SafeInvoke` performs latency logging: it stops the probe
and computes `micros = probe.ElapsedMicroseconds` and, if `micros > 1000`, executes
`NinjaTrader.Code.Output.Process(LogBuffer.Format("[LATENCY_FANOUT]{0}: {1:F2}ms across
{2} subscribers", typeof(T).Name, micros / 1000.0, invocationList.Length),
PrintTo.OutputTab1);` at `src/SignalBroadcaster.cs:25-38`, with no surrounding
`try/catch`.
4. If either `LogBuffer.Format(...)` (implemented in `src/V12_002.Perf.LogBuffer.cs:9-30`)
or `NinjaTrader.Code.Output.Process` throws—for example due to an Output window lifecycle
issue or a future formatting change—the exception will bubble out of `SafeInvoke` and the
broadcast method (e.g., a future call to `BroadcastFlatten`), contradicting the SafeInvoke
contract at `src/SignalBroadcaster.cs:201-205` that subscriber failures are isolated and
should not break the fan-out chain.
This is a comment left during a code review.
**Path:** src/SignalBroadcaster.cs
**Line:** 227:238
**Comment:***Missing Cleanup: The new latency logging executes outside the subscriber-isolation `try/catch`, so any exception from formatting or output emission will escape `SafeInvoke` and break broadcasting even though subscriber failures are supposed to be isolated. Keep logging inside a guarded block so diagnostics cannot take down signal fan-out.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
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
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![codefactor-io[bot]](https://avatars.githubusercontent.com/in/25603?s=60&v=4){kind=small}
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
coderabbitai
Bot
added
size:XXL
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
EPIC-6 Phase 1 Complete
Summary
Automated testing infrastructure to lock in Epic 5 performance gains (43M+ allocations/year eliminated, P50 65-100μs latency).
Deliverables
Test Results
F5 Verification
✅ BUILD_TAG \1111.011-epic6-testing\ verified in NinjaTrader IDE
Files Changed
32 files, 4,882 insertions, 507 deletions
Commit: 6d669d5
Summary by cubic
Automated test and performance infrastructure to lock in Epic 5 gains (zero-allocation hot paths, sub-300µs latency), plus instrumentation and pools to prevent regressions. Aligns with EPIC-6 by adding benchmarks, unit tests, and CI gates that verify allocations and latency on every PR.
New Features
BenchmarkDotNetfor BarUpdate, OrderCallbacks, and SIMA dispatch; replaces StandaloneBench.LogBuffer(ThreadStatic) replacing string.Format in hot paths.Bug Fixes
Written for commit 6d669d5. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Greptile Summary
This PR introduces the EPIC-6 automated testing infrastructure: zero-allocation perf primitives (
LatencyProbe,LogBuffer,LatencyHistogram,OrderArrayPool,UISnapshotPool), latency instrumentation onOnBarUpdate/OnMarketData/MonitorRmaProximity, BenchmarkDotNet harnesses, xUnit test suites, and a GitHub Actions CI workflow with DNA compliance gates.LogBuffer.Formatcontains a silent correctness bug: alignment-only specifiers like{0,-28}are neither substituted nor forwarded tostring.Format, so format placeholder text appears verbatim in dispatch logs instead of account names.-SimpleMatchcauses the pattern'lock\\s*\\('to be matched as a literal string, so actuallock (...)statements would silently pass the gate.LogBuffer.Formatstill allocates (params object[], boxing for every value-type arg, andnew string(...)) on the claimed "zero-allocation" path, contradicting the Jane Street ALLOCATION IS A BUG principle for the hot paths it instruments.Confidence Score: 2/5
The PR introduces a correctness bug in the central LogBuffer.Format abstraction that silently corrupts every dispatch log line using column-alignment formatting, and the CI Lock-Free Audit gate is misconfigured in a way that would pass even if lock() were reintroduced.
LogBuffer.Format is the foundational primitive adopted across SIMA.Execution, SIMA.Dispatch, Entries.RMA, and SignalBroadcaster. Its failure to substitute {0,-28}-style arguments without triggering a fallback means account names are replaced with raw placeholder text in the dispatched order log — the exact output used for fleet auditing. Separately, the Lock-Free Audit in CI uses -SimpleMatch with a regex pattern, rendering the gate inert. Both issues need to be fixed before this infrastructure can serve as a reliable performance lock-in baseline.
src/V12_002.Perf.LogBuffer.cs (the FormatInternal fallback logic for alignment specifiers) and .github/workflows/epic6-testing.yml (the Lock-Free Audit step) require fixes before merge.
Important Files Changed
Sequence Diagram
sequenceDiagram participant NT as NinjaTrader Thread participant BU as OnBarUpdate participant MD as OnMarketData participant RMA as MonitorRmaProximity participant LB as LogBuffer.Format participant LP as LatencyProbe participant LH as LatencyHistogram NT->>BU: bar tick BU->>LP: Start() BU->>LB: Format(msg, args...) Note over LB: params object[] allocated<br/>value types boxed<br/>{0,-28} emitted verbatim LB-->>BU: new string(buffer) BU->>BU: strategy logic BU->>LP: Stop() BU->>LH: Record(probe) NT->>MD: market tick MD->>LP: Start() MD->>MD: strategy logic MD->>LP: Stop() MD->>LH: Record(probe) NT->>RMA: MonitorRmaProximity() RMA->>LP: Start() alt "RmaIntelligenceEnabled = false" RMA-->>NT: return (probe never recorded) else enabled RMA->>RMA: iterate orders Note over RMA: no finally block endReviews (1): Last reviewed commit: "[EPIC-6] Performance Lock-In - Automated..." | Re-trigger Greptile
Context used:
MANDATORY: ZERO TOLERANCE FOR JANE STREET ALIGNM... (source)