fix(955): Race condition & TOCTOU remediation -- PR #24 audit fixes

[mkalhitti-cloud]

Summary

Supersedes PR #24. Addresses all three findings from the Greptile / DeepSource / Codex automated review of Build 954.

FINDING 1 -- CRITICAL: Race condition in V12_002.Trailing.cs (Greptile, Confidence 2/5 block)

Root cause: Both TryAdd paths in UpdateStopOrder inserted a PendingStopReplacement into pendingStopReplacements with CapturedTargets = null and BracketRestorationNeeded = false, then populated those fields ~24-34 lines later. A stop-cancel callback arriving in that window saw the incomplete record and silently skipped RestoreCascadedTargets -- defeating the entire Build 950 OCO cascade restoration feature.

Fix: Build the TargetSnapshot list and set CapturedTargets/BracketRestorationNeeded in the object initializer BEFORE calling TryAdd. Object is fully initialized when it becomes visible to other threads. Removed the now-redundant post-TryAdd snapshot blocks.

FINDING 2 -- HIGH: TOCTOU in V12_002.Orders.Callbacks.cs

Root cause (master path): pos.RemainingContracts was read twice without a lock (guard at line 372, use at line 374). A concurrent fill between reads could produce a 0-qty stop order.

Root cause (follower path): Guard check _rPos.RemainingContracts > 0 was outside stateLock while the actual snapshot was inside -- decision made on unlocked read.

Fix: Snapshot RemainingContracts once under stateLock; use the snapshot for both guard and CreateNewStopOrder call in both paths.

FINDING 3 -- LOW: IPC dead code in V12_001.cs (DeepSource hygiene)

Root cause: Build 954 added return; to stub out ConnectToStrategy() but left the entire try/catch body as unreachable dead code (~60 lines). isConnected stayed permanently false, causing every SendCommand() call to fire a no-op Task.Run.

Fix: Removed the unreachable body. SendCommand() safely no-ops via existing tcpStream != null guard.

Changed Files

File	Change
src/V12_002.Trailing.cs	Snapshot-before-TryAdd in both state-handler paths
src/V12_002.Orders.Callbacks.cs	Atomic stateLock snapshot for RemainingContracts in master + follower paths
src/V12_001.cs	Remove unreachable IPC dead code from ConnectToStrategy()
src/V12_002.cs	BUILD_TAG -> "955"

Verification

• ASCII gate: PASS (deploy-sync.ps1)
• Static check: no TryAdd call on pendingStopReplacements has a post-add field mutation of CapturedTargets
• F5 compile in NT8 (pending human verification)
• OCO bracket restoration smoke test -- trigger stop replacement, confirm RestoreCascadedTargets dispatched

Related

Closes (supersedes) #24

---

Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[deepsource-io]

DeepSource Code Review

We reviewed changes in 24ec680...e35028f on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade   Focus Area: Hygiene

Security   Reliability   Complexity   Hygiene

Feedback

• No unified time model
  • Local-time usage plus mutable time-tracking creates subtle bugs and brittle tests; centralize time by introducing a single TimeProvider/ IClock that returns UTC and make temporal state updates explicit and immutable where possible.
• Monolithic files and fat methods
  • Multiple namespaces in one file and a 51-point method reflect accretion of responsibilities into single units; split files by namespace and extract small, focused methods or strategy objects to reduce branching and cyclomatic complexity.
• Uneven adoption of modern C# idioms
  • Repeated verbose null checks and mutable fields indicate inconsistent use of pattern matching, nullability, and readonly semantics; prefer nullable reference types, null-coalescing/pattern matching, and readonly for fields that shouldn't mutate to simplify code and prevent errors.

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
C#		Mar 6, 2026 11:35p.m.	Review ↗
Secrets		Mar 6, 2026 11:35p.m.	Review ↗

[deepsource-io]

Null check can be simplified

The ?. operator allows you to access a property or a member only if it is non-null. Consider simplifying the highlighted expression, i.e. the object and its member's null checks via the usage of this ?. operator.

[deepsource-io]

Nullable check expression can be simplified

While it is common to explicitly check an object's nullability before accessing its value, you can further simplify this expression by
either using the conditional access operator ? or pattern matching instead. These proposed solutions are succinct and reduce your
keystrokes.

[deepsource-io]

Consider using `DateTime.UtcNow` over `DateTime.Now`

DateTime.Now refers to the date and time that is local and specific to the computer on which the program is running.
Because you may either misinterpret this information or miss out on any additional information, it is recommended that you use DateTime.UtcNow.
This ensures that your program, irrespective of which computer it runs on, utilizes information that is kept consistent.

[deepsource-io]

Nullable check expression can be simplified

While it is common to explicitly check an object's nullability before accessing its value, you can further simplify this expression by
either using the conditional access operator ? or pattern matching instead. These proposed solutions are succinct and reduce your
keystrokes.

[deepsource-io]

Nullable check expression can be simplified

While it is common to explicitly check an object's nullability before accessing its value, you can further simplify this expression by
either using the conditional access operator ? or pattern matching instead. These proposed solutions are succinct and reduce your
keystrokes.

[deepsource-io]

Consider using `DateTime.UtcNow` over `DateTime.Now`

DateTime.Now refers to the date and time that is local and specific to the computer on which the program is running.
Because you may either misinterpret this information or miss out on any additional information, it is recommended that you use DateTime.UtcNow.
This ensures that your program, irrespective of which computer it runs on, utilizes information that is kept consistent.

[Copilot]

Pull request overview

Remediates audit findings for Build 955 in the NinjaTrader 8 strategy by tightening stop-replacement concurrency, removing TOCTOU reads around RemainingContracts, and pruning deprecated IPC UI-panel code.

Changes:

• Initialize PendingStopReplacement target snapshots before publishing to pendingStopReplacements (race window reduction) and restore cascaded targets after emergency stop fallback.
• Snapshot RemainingContracts under stateLock to avoid TOCTOU in both master and follower cancellation/replacement paths.
• Remove unreachable IPC connection code from the V12 panel and update build tag/documentation.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/V12_002.cs	Bumps BUILD_TAG to 955 and adds TargetSnapshot + pending replacement fields for bracket restoration state.
src/V12_002.Trailing.cs	Snapshots targets before TryAdd and triggers target restoration after V8.30 stale-replacement emergency stop.
src/V12_002.Orders.Management.cs	Re-links replacement stops to OcoGroupId and adds RestoreCascadedTargets() to resubmit OCO-cancelled targets.
src/V12_002.Orders.Callbacks.cs	Uses atomic qty snapshots under stateLock and adds follower stop-cancel handling to mirror master behavior.
src/V12_002.Entries.Retest.cs	Early-return on null unmanaged submit to prevent latching/dispatch after failed order.
src/V12_001.cs	Stubs ConnectToStrategy() (IPC deprecated) and normalizes unknown saved UI mode to RMA.
docs/brain/walkthrough.md	Documents bracket replacement / OCO cascade restoration workflow (needs build-tag correction).

[Copilot]

This section claims BUILD_TAG = "950" in the listed file changes, but the strategy BUILD_TAG was updated to 955 in this PR. Please update the walkthrough to reflect the correct build tag (or make the note build-agnostic) to avoid future confusion during verification/audits.

Suggested change

	- src/V12_002.cs -- TargetSnapshot class, PendingStopReplacement extended, BUILD_TAG = "950"
	- src/V12_002.Trailing.cs -- target snapshot in UpdateStopOrder, restore in V8.30 timeout
	- src/V12_002.Orders.Callbacks.cs -- follower stop handler + master bracket restore
	- src/V12_002.Orders.Management.cs -- RestoreCascadedTargets(), CreateNewStopOrder OcoId fix
	### Verification
	1. Sim session: Enter 4-contract position, verify bracket (Stop + T1/T2/T3) all Working
	2. Send BE_CUSTOM via IPC -- confirm logs show "[B950] Target T1 restored", "[B950] Target T2 restored"
	- src/V12_002.cs -- TargetSnapshot class, PendingStopReplacement extended, BUILD_TAG = "955"
	- src/V12_002.Trailing.cs -- target snapshot in UpdateStopOrder, restore in V8.30 timeout
	- src/V12_002.Orders.Callbacks.cs -- follower stop handler + master bracket restore
	- src/V12_002.Orders.Management.cs -- RestoreCascadedTargets(), CreateNewStopOrder OcoId fix
	### Verification
	1. Sim session: Enter 4-contract position, verify bracket (Stop + T1/T2/T3) all Working
	2. Send BE_CUSTOM via IPC -- confirm logs show "[B955] Target T1 restored", "[B955] Target T2 restored"

[Copilot]

In the follower-account branch, CreateOrder() can return null (disconnected/invalid account), but Submit(new[] { newStop }) is called unconditionally. This can throw and skip the existing newStop == null flatten path, leaving the position unprotected. Null-guard the created order before Submit, and consider mirroring the existing bracket-submission pattern that treats Submit exceptions as bracket-fatal (emergency flatten).

Suggested change

	pos.ExecutingAccount.Submit(new[] { newStop });
	// Guard against null CreateOrder and treat Submit failures as bracket-fatal.
	if (newStop != null)
	{
	try
	{
	pos.ExecutingAccount.Submit(new[] { newStop });
	}
	catch (Exception)
	{
	// Mark as failed so downstream newStop == null handling can emergency-flatten.
	newStop = null;
	}
	}

[Copilot]

This CapturedTargets refresh mutates fields on an already-published PendingStopReplacement instance. That reintroduces the same race window described in the PR (callbacks can observe BracketRestorationNeeded == false / CapturedTargets == null and skip restoration). Prefer ensuring all PendingStopReplacement instances are fully initialized before insertion, or replace the dictionary value atomically (e.g., TryUpdate with a new fully-populated object) instead of post-add field mutation.

Suggested change

	pending.CapturedTargets = _b950Refresh.Count > 0 ? _b950Refresh.ToArray() : null;
	pending.BracketRestorationNeeded = _b950Refresh.Count > 0;
	// Build fully-initialized replacement to avoid mutating a published instance.
	bool hasTargets = _b950Refresh.Count > 0;
	var updatedPending = new PendingStopReplacement
	{
	// Preserve identifying information and current stop price
	EntryName = entryName,
	StopPrice = pending.StopPrice,
	// Apply refreshed bracket-restoration metadata
	CapturedTargets = hasTargets ? _b950Refresh.ToArray() : null,
	BracketRestorationNeeded = hasTargets
	};
	// Atomically publish the refreshed record; ignore failure if another thread updated first.
	pendingStopReplacements.TryUpdate(entryName, updatedPending, pending);

[Copilot]

ConnectToStrategy() is now a no-op, but SendCommand() still spins a Task.Run and calls ConnectToStrategy() whenever isConnected == false. With IPC deprecated, this means every UI action that calls SendCommand() will continue to enqueue a no-op background task (threadpool churn) and isConnected will never become true. Consider short-circuiting SendCommand() (or ConnectToStrategy()) behind an explicit ipcEnabled flag and returning synchronously without Task.Run when IPC is disabled.

[Copilot]

The follower stop-cancel match uses reference equality only (OldOrder == order). Elsewhere in this file (e.g., TryFindOrderInPosition) you already fall back to OrderId comparison to handle cases where the callback provides a different Order instance for the same broker order. Consider matching pending stop replacements by OrderId as well to avoid missing the replacement/restore path.

Suggested change

	if (_psr.Value.OldOrder == order)
	var _oldOrder = _psr.Value.OldOrder;
	// Match by reference when possible, but fall back to OrderId equality to handle
	// cases where the callback supplies a different Order instance for the same broker order.
	if (_oldOrder == order
	|| (_oldOrder != null
	&& order != null
	&& !string.IsNullOrEmpty(_oldOrder.OrderId)
	&& string.Equals(_oldOrder.OrderId, order.OrderId, StringComparison.Ordinal)))

[greptile-apps]

Greptile Summary

This PR addresses three audit findings from Build 954: a critical race condition in UpdateStopOrder where PendingStopReplacement was inserted into pendingStopReplacements before its CapturedTargets/BracketRestorationNeeded fields were populated (fixed by snapshot-before-TryAdd); a TOCTOU in the stop-cancel callbacks where RemainingContracts was read twice without a lock (fixed by a single atomic snapshot under stateLock); and ~60 lines of unreachable IPC dead code in ConnectToStrategy() (removed). The OCO bracket restoration feature introduced in Build 950 is also wired into the new paths.

Key changes:

• V12_002.Trailing.cs — Both PendingStopReplacement construction sites now fully initialise CapturedTargets and BracketRestorationNeeded in the object initialiser before the ConcurrentDictionary.TryAdd call, eliminating the window where a concurrent callback could observe an incomplete record.
• V12_002.Orders.Callbacks.cs — Master and follower stop-cancel paths each snapshot pos.RemainingContracts once inside stateLock and reuse that snapshot for the guard check and CreateNewStopOrder argument; a new follower stop-replacement handler mirrors the existing master path in HandleOrderCancelled.
• V12_002.Orders.Management.cs — RestoreCascadedTargets() re-submits OCO-cascade-cancelled bracket targets on the strategy thread after a new stop is confirmed; CreateNewStopOrder propagates OcoGroupId in both the local and fleet-follower paths.
• V12_001.cs — ConnectToStrategy() body removed; SendCommand() safely no-ops via existing tcpStream != null guard.

Issues found:

• Path B (Working/Accepted stop) TryAdd failure has no else clause — unlike Path A which has a Build 950 refresh block, a silent TryAdd collision in Path B leaves the existing pending with stale CapturedTargets. The same defensive else if pattern from Path A should be mirrored.
• Local-path target signal name in RestoreCascadedTargets is not length-capped — the follower branch uses SymmetryTrim(..., 40) but the SubmitOrderUnmanaged branch constructs "T" + snap.TargetNum + "_" + entryName with no truncation, which can silently produce a rejected order for long entry names.
• Follower TryRemove is unconditional on activePositions lookup — the pending record is always removed even when the position key is absent, which is asymmetric from the master path and removes the V8.30 timeout as a fallback; a diagnostic log would improve observability.

Confidence Score: 3/5

• Core race-condition and TOCTOU fixes are sound, but two edge cases in the new OCO restoration code (Path B TryAdd failure and untruncated signal names) could cause silent bracket restoration failures in production.
• The three stated findings are correctly remediated — snapshot-before-TryAdd eliminates the primary race, the atomic lock-snapshot fixes TOCTOU, and the dead code removal is clean. However, the new RestoreCascadedTargets code introduces an asymmetry (untruncated local signal name vs. follower's SymmetryTrim) that can silently produce a null order return on long entry names, and the Path B TryAdd-failure gap means a concurrent collision would leave CapturedTargets stale with no warning. Neither issue manifests on typical entry names or under normal serialisation on the strategy thread, but both represent silent failure modes rather than loud errors, which is concerning for live trading infrastructure. F5 compile and OCO smoke test checkboxes are also still open per the PR checklist.
• src/V12_002.Trailing.cs (Path B TryAdd else gap) and src/V12_002.Orders.Management.cs (RestoreCascadedTargets signal name truncation) warrant attention before merging to production.

Important Files Changed

Filename	Overview
src/V12_002.Trailing.cs	Build 955 snapshot-before-TryAdd correctly eliminates the primary race condition. However, Path B (Working/Accepted stop state) TryAdd failure has no else clause to refresh CapturedTargets on the existing record, unlike Path A which has the Build 950 refresh block. Also, the early-return path at line 501 still skips target capture entirely.
src/V12_002.Orders.Callbacks.cs	TOCTOU fix looks correct — single atomic read of RemainingContracts under stateLock for both guard and use in both master and follower paths. Follower path TryRemove is unconditional on activePositions lookup (asymmetric from master path), though this is likely intentional for cleanup when position is already gone.
src/V12_002.Orders.Management.cs	RestoreCascadedTargets is a well-structured new function. Signal name for local/master target orders (line 637) is not length-capped unlike the follower path which uses SymmetryTrim(..., 40), creating a potential rejection risk on long entryNames. OcoGroupId is correctly propagated in CreateNewStopOrder for both follower and local paths.
src/V12_001.cs	Dead code removal from ConnectToStrategy() is clean and correct. SendCommand() null-guard on tcpStream safely no-ops all calls. Unrecognized-mode fallback to RMA is a solid defensive addition.
src/V12_002.cs	BUILD_TAG bumped to 955. TargetSnapshot inner class and PendingStopReplacement extension fields are correctly defined. CapturedTargets and BracketRestorationNeeded placement in the class is appropriate.
src/V12_002.Entries.Retest.cs	Single-line guard addition — return early after a NULL SubmitOrderUnmanaged to prevent session latch / SIMA dispatch on a failed order. Correct and low-risk.

Sequence Diagram

sequenceDiagram
    participant Trail as UpdateStopOrder<br/>(Trailing.cs)
    participant PDict as pendingStopReplacements<br/>(ConcurrentDict)
    participant Broker as Broker / NT8
    participant OCB as HandleOrderCancelled<br/>(master path)
    participant FCB as HandleMatchedFollowerOrder<br/>(follower path)
    participant CNSO as CreateNewStopOrder
    participant RCT as RestoreCascadedTargets<br/>(strategy thread)

    Note over Trail: Build 955: Snapshot targets BEFORE TryAdd
    Trail->>Trail: Collect Working/Accepted target orders → _b955Targets[]
    Trail->>Trail: Build fully-initialised PendingStopReplacement<br/>(CapturedTargets + BracketRestorationNeeded set in initialiser)
    Trail->>PDict: TryAdd(entryName, newPending)
    Trail->>Broker: CancelOrder(currentStop)

    alt Master account stop cancel
        Broker-->>OCB: OnOrderUpdate(Cancelled)
        OCB->>PDict: TryGetValue — match OldOrder
        Note over OCB: Build 955: lock(stateLock) _stopQty = pos.RemainingContracts
        OCB->>CNSO: CreateNewStopOrder(key, _stopQty, stopPrice, dir)
        OCB->>RCT: TriggerCustomEvent → RestoreCascadedTargets(key, snap)
        OCB->>PDict: TryRemove(key)
    else Follower account stop cancel
        Broker-->>FCB: OnAccountOrderUpdate → ProcessQueuedAccountOrder
        FCB->>PDict: ToArray() — match OldOrder
        Note over FCB: Build 955: lock(stateLock) _rQty = pos.RemainingContracts
        FCB->>CNSO: CreateNewStopOrder(key, _rQty, stopPrice, dir)
        FCB->>RCT: TriggerCustomEvent → RestoreCascadedTargets(key, snap)
        FCB->>PDict: TryRemove(key)
    else V8.30 timeout fallback
        Trail-->>Trail: Timeout scan fires
        Trail->>CNSO: CreateNewStopOrder(key, replacementQty, ...)
        Trail->>RCT: TriggerCustomEvent → RestoreCascadedTargets(key, snap)
        Trail->>PDict: TryRemove(key)
    end

    RCT->>RCT: lock(stateLock): snapshot pos fields
    loop each CapturedTarget where OrderState == Cancelled/Rejected
        RCT->>Broker: SubmitOrderUnmanaged / Account.Submit (target limit order + OcoGroupId)
        RCT->>RCT: tDict[entryName] = newTarget
    end

Loading

Comments Outside Diff (1)

1. src/V12_002.Trailing.cs, line 603-613 (link)

   Path B TryAdd failure silently discards the new snapshot

   Path A (lines 540-570) correctly has an else if block that runs when TryAdd fails, refreshing CapturedTargets / BracketRestorationNeeded on the existing pending record. Path B (Working/Accepted state, line 604) has no corresponding else if — if TryAdd returns false, the fully-built newPending (including the fresh _b955TargetsB snapshot) is silently discarded, and the existing pending record retains stale target data.

   While the early-return at line 501 should normally intercept the existing-pending case before this point, the absence of a failure handler means there is no safety net if the early-return is somehow bypassed (e.g., a race between the TryGetValue at line 501 and the broker-thread add). Path A's defensive else if pattern should be mirrored here:

   if (pendingStopReplacements.TryAdd(entryName, newPending))
   {
       int currentCount = Interlocked.Increment(ref pendingReplacementCount);
       // ... circuit-breaker ...
   }
   else if (pendingStopReplacements.TryGetValue(entryName, out var pendingB))
   {
       pendingB.StopPrice = validatedStopPrice;
       if (!pendingB.BracketRestorationNeeded)
       {
           pendingB.CapturedTargets          = _b955TargetsB.Count > 0 ? _b955TargetsB.ToArray() : null;
           pendingB.BracketRestorationNeeded = _b955TargetsB.Count > 0;
       }
   }

_{Last reviewed commit: 2c337a7}

[greptile-apps]

Local-path target signal name not length-capped

The follower path (line 625) guards against long signal names with SymmetryTrim("T" + snap.TargetNum + "_" + entryName, 40). The local (SubmitOrderUnmanaged) path at line 637 constructs the same pattern with no length cap:

string tSig = "T" + snap.TargetNum + "_" + entryName;

CreateNewStopOrder explicitly truncates its own signal names to 50 characters. If entryName is 47+ characters (the follower stop signal is capped at 50 after "S_" prefix), "T1_" + entryName exceeds 50 characters, and SubmitOrderUnmanaged may silently reject the order or return null. The null-return logging exists (line 656 prints "WARN: Target T{n} restore NULL"), but the root cause would be obscure. A consistent truncation keeps behavior predictable:

string tSigRaw = "T" + snap.TargetNum + "_" + entryName;
string tSig = tSigRaw.Length > 50 ? tSigRaw.Substring(0, 50) : tSigRaw;

[greptile-apps]

TryRemove is unconditional on activePositions.TryGetValue — asymmetric with master path

In the master path (HandleOrderCancelled, line 370), the pending record is only removed when activePositions.TryGetValue succeeds (the TryRemove is inside the combined if condition). Here in the follower path the TryRemove at line 620 is outside the activePositions.TryGetValue block, so the pending record is always cleaned up even when the position key is missing from activePositions.

If a position is cleaned up from activePositions (e.g., by the emergency flatten) just before this callback fires, the pending stop replacement is silently dropped without a new stop being issued and without any log entry. The V8.30 timeout that would normally catch a stale pending in the master path is bypassed because the pending no longer exists.

Adding a diagnostic log when the position is missing improves observability:

if (activePositions.TryGetValue(_psr.Key, out _rPos))
{
    // ... qty check and CreateNewStopOrder ...
}
else
{
    Print(string.Format("[B955] WARN: Follower stop cancel matched pending '{0}' but position not in activePositions -- pending cleaned up without replacement.", _psr.Key));
}
if (pendingStopReplacements.TryRemove(_psr.Key, out _))
    Interlocked.Decrement(ref pendingReplacementCount);
return;

[mkalhitti-cloud]

Superseded by #26 (build/956-deepsource-remediation). PR #25 had an incomplete IPC dead code fix and missed the RETEST_MANUAL null-submit fall-through. Fresh PR opened per the Fresh PR Rule to give DeepSource a clean full-file sweep.