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
Surgical fixes for IPC ghost connection, RETEST null order leak, and activeMode normalization. Injected Section 12 into Standards Manifesto for Maximum Autonomy and Protected Legacy Archive.
- Add TargetSnapshot class for bracket state capture
- Extend PendingStopReplacement with CapturedTargets
- Fix follower stop-cancel black hole in HandleMatchedFollowerOrder
- Add RestoreCascadedTargets via TriggerCustomEvent
- Fix CreateNewStopOrder to re-link OcoGroupId
- Add bracket restore to V8.30 timeout fallback path
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e fallback
Fix 1 (V12_001.cs): Stub ConnectToStrategy() with early return -- strategy no
longer hosts IPC server after Phase 6 pruning. Panel runs in standalone UI mode;
SendCommand calls safely no-op via tcpStream null guard.
Fix 2 (V12_002.Entries.Retest.cs): Insert return; inside null-order guard after
SubmitOrderUnmanaged fails. Prevents retestFiredThisSession latch and SIMA
dispatch from arming on a failed order submission.
Fix 3 (V12_001.cs): Add else branch in LoadConfig() to normalize both activeMode
and selectedConfigMode to RMA when a deprecated/pruned mode is found in saved
config. Prevents GetSettings() from loading garbage data from a deleted slot.
Build tag incremented to 954.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
We reviewed changes in 24ec680...7fc6b4b on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
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
Mixed responsibilities inside single files
Multiple namespaces and sprawling methods indicate files bundle unrelated concerns, which produces large, hard-to-reason blobs; splitting types and namespaces into focused files restores cohesion and keeps methods small and targeted.
Scattershot null-guarding instead of a single contract
Repeated, simplifiable nullable checks appear because validation is sprinkled throughout logic rather than enforced at a boundary; adopt nullable-reference types or centralized guard helpers and validate at entry points to remove redundant checks.
Accreted branching left without cleanup
Extremely high branch counts and unreachable statements come from piling new cases onto existing methods instead of extracting behavior, leaving dead code behind; break the pattern by extracting well-named helpers, preferring early returns, and pruning unreachable paths.
The reason will be displayed to describe this comment to others. Learn more.
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.
The reason will be displayed to describe this comment to others. Learn more.
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.
The reason will be displayed to describe this comment to others. Learn more.
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.
The reason will be displayed to describe this comment to others. Learn more.
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.
This PR delivers three surgical Build 954 fixes — IPC ghost-connection removal, RETEST null-order state leak, and deprecated activeMode fallback — on top of the larger Build 950 OCO cascade bracket-restoration FSM that was already partially landed. The bracket restoration work spans five files: a new TargetSnapshot class, extended PendingStopReplacement, snapshot capture in UpdateStopOrder, restoration dispatch in both the master and follower cancel callbacks, and a new RestoreCascadedTargets method.
Key changes and concerns:
V12_002.Trailing.cs — race condition in target snapshot timing: In both TryAdd paths, newPending is inserted into the shared pendingStopReplacements dictionary before CapturedTargets / BracketRestorationNeeded are assigned. A stop-cancel callback arriving in that window will see an incomplete record and skip bracket restoration entirely — the exact scenario this build targets.
V12_002.Orders.Callbacks.cs — follower stop path TOCTOU: RemainingContracts is read once without stateLock for the guard check, then re-read under the lock for the actual value passed to CreateNewStopOrder. A fill in that window could produce a 0-quantity stop order.
V12_001.cs — IPC intentionally disabled: ConnectToStrategy() early-returns, leaving the full connection body as dead code. The intent is documented, but isConnected and retry-state variables remain at their initial values permanently, which may mislead any status-display logic that reads them.
V12_002.Entries.Retest.cs: Clean, isolated fix — return after null order prevents unintended session latch and SIMA dispatch.
V12_002.Orders.Management.cs: RestoreCascadedTargets and OcoGroupId threading into CreateNewStopOrder are well-structured and run on the strategy thread, avoiding additional locking needs.
Confidence Score: 2/5
Not safe to merge as-is — the race condition in V12_002.Trailing.cs can silently prevent OCO bracket restoration in the exact scenario this build is designed to fix.
The core Build 954 fixes (RETEST null leak, deprecated mode fallback) are correct and isolated. However, the Build 950 bracket restoration — the most complex and consequential logic in this PR — has a race condition: CapturedTargets is populated after the PendingStopReplacement is inserted into the shared dictionary. A stop-cancel callback in that window sees BracketRestorationNeeded = false and silently skips restoration, leaving a naked position with no targets. The follower callback also has a minor TOCTOU on RemainingContracts. These are real trading-safety issues on a live multi-account strategy.
src/V12_002.Trailing.cs (race condition in both TryAdd paths) and src/V12_002.Orders.Callbacks.cs (TOCTOU in follower RemainingContracts read) need attention before merge.
Important Files Changed
Filename
Overview
src/V12_002.Trailing.cs
Adds OCO target snapshot capture before stop cancel. Contains a race condition where CapturedTargets is set on newPending after it has already been inserted into the shared pendingStopReplacements dictionary, allowing a concurrent callback to see an incomplete record and skip bracket restoration.
src/V12_002.Orders.Callbacks.cs
Adds follower stop replacement handling and master-path bracket restoration via TriggerCustomEvent. Follower path has a TOCTOU read of RemainingContracts (once without lock, once with lock); the window is narrow but inconsistent with best practices.
src/V12_002.Orders.Management.cs
Adds RestoreCascadedTargets method and threads OcoGroupId into replacement stop order submissions for both fleet-follower and local paths. Logic is sound; target dict updates run on strategy thread via TriggerCustomEvent so no additional locking is needed there.
src/V12_001.cs
IPC ConnectToStrategy disabled via early return, leaving the entire connection body as dead code. Deprecated-mode fallback to "RMA" is correct. Dead code may mislead future readers about actual runtime state of isConnected/tcpStream.
src/V12_002.Entries.Retest.cs
Adds return after null order error to prevent session latch and spurious SIMA dispatch. Clean, targeted fix with no side-effects.
src/V12_002.cs
Updates BUILD_TAG to "954", adds TargetSnapshot nested class, and extends PendingStopReplacement with CapturedTargets / BracketRestorationNeeded. Structural changes are clean.
docs/brain/walkthrough.md
Documentation update for Build 950 OCO Cascade Fix. Accurately describes the two-part FSM fix, affected files, and verification steps.
Sequence Diagram
sequenceDiagram
participant TS as UpdateStopOrder<br/>(Trailing.cs)
participant Dict as pendingStopReplacements<br/>(ConcurrentDictionary)
participant OUC as OnOrderUpdate/<br/>OnAccountOrderUpdate
participant HOC as HandleOrderCancelled /<br/>HandleMatchedFollowerOrder
participant CSO as CreateNewStopOrder
participant RCT as RestoreCascadedTargets
TS->>TS: Snapshot Working targets into _b950Targets
Note over TS: ⚠️ BUG: snapshot happens AFTER TryAdd
TS->>Dict: TryAdd(entryName, newPending)<br/>[CapturedTargets=null at this point]
TS->>Dict: newPending.CapturedTargets = snapshot<br/>[race window between these two lines]
OUC->>HOC: stop cancel event arrives
HOC->>Dict: lookup pendingStopReplacements
alt CapturedTargets already set (no race)
HOC->>CSO: CreateNewStopOrder(entryName, qty, price)
HOC->>RCT: TriggerCustomEvent → RestoreCascadedTargets
RCT->>RCT: Check each snap.CapturedOrder.OrderState
RCT->>RCT: Re-submit Cancelled targets via SubmitOrderUnmanaged / Account.Submit
RCT->>RCT: tDict[entryName] = newTarget
else Race: CapturedTargets=null (BracketRestorationNeeded=false)
HOC->>CSO: CreateNewStopOrder(entryName, qty, price)
Note over HOC: RestoreCascadedTargets skipped — bracket not restored
end
Race condition: CapturedTargets populated after TryAdd
When TryAdd succeeds at line 528, newPending is placed into the shared pendingStopReplacements dictionary with CapturedTargets = null and BracketRestorationNeeded = false. The unconditional snapshot block (lines 562-574) only populates CapturedTargetsafter the entry is already visible to other threads.
If the broker's stop-cancel callback fires (on OnOrderUpdate / OnAccountOrderUpdate) between the moment TryAdd returns and the moment newPending.CapturedTargets is assigned, both HandleOrderCancelled and HandleMatchedFollowerOrder will see BracketRestorationNeeded == false and skip RestoreCascadedTargets entirely — defeating the entire OCO cascade restore feature in a tight-window race.
The same issue exists in the second TryAdd block around line 595-618.
The snapshot should be captured beforeTryAdd, so the object entering the dictionary is already fully initialised:
// Build 950: Snapshot before TryAdd so any callback sees a complete record.var_b950Targets=newSystem.Collections.Generic.List<TargetSnapshot>();for(int_t=1;_t<=5;_t++){var_tD=GetTargetOrdersDictionary(_t);Order_tO;if(_tD!=null&&_tD.TryGetValue(entryName,out_tO)&&_tO!=null&&(_tO.OrderState==OrderState.Working||_tO.OrderState==OrderState.Accepted))_b950Targets.Add(newTargetSnapshot{TargetNum=_t,Price=_tO.LimitPrice,Qty=_tO.Quantity,CapturedOrder=_tO});}varnewPending=newPendingStopReplacement{EntryName=entryName,Quantity=pos.RemainingContracts,StopPrice=validatedStopPrice,Direction=pos.Direction,OldOrder=currentStop,CreatedTime=DateTime.Now,CapturedTargets=_b950Targets.Count>0?_b950Targets.ToArray():null,BracketRestorationNeeded=_b950Targets.Count>0};if(pendingStopReplacements.TryAdd(entryName,newPending)){ ...}
The same correction should be applied to the second TryAdd block (around line 584-618).
The reason will be displayed to describe this comment to others. Learn more.
Dead code left in place after early return
The return; at line 3003 leaves the entire try/catch block unreachable. While the comment is clear about the intent, the dead code includes non-trivial logic: thread creation (receiveThread), TcpClient/TcpStream allocation, and a catch that resets isConnected / increments _ipcRetryCount. If isConnected or _ipcRetryCount are read elsewhere (e.g. a status display), they will permanently show their initial/stale values rather than reflecting "intentionally offline".
Consider either removing the body entirely (leaving only the comment + return) or adding a one-time log line that records the intentional no-op state so the status LED / connection state surface the right information.
The reason will be displayed to describe this comment to others. Learn more.
TOCTOU on RemainingContracts in follower path
The guard at line 601 reads _rPos.RemainingContracts > 0 without holding stateLock, then immediately re-reads the same field under the lock at line 604. Between these two reads another thread could decrement RemainingContracts to 0 (e.g. a fill callback), so _rQty could be 0 or negative despite the earlier check passing, and CreateNewStopOrder would be called with _rQty = 0.
The simpler — and consistent — fix is to read once under the lock:
Note: the master path in HandleOrderCancelled (line 372-374) has the same pattern without any lock at all — consistency across both paths would be worth aligning.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Build 954 focuses on hardening order-management edge cases (especially stop replacement/OCO bracket integrity) and remediating a RETEST null-order leak, plus normalizing deprecated UI config modes and deprecating IPC usage in the panel.
Changes:
Add bracket target snapshot/restore logic to recover from broker OCO cascade cancellations during stop replacement (Build 950 behavior carried into 954).
Prevent RETEST from latching session/dispatching SIMA when entry submission returns null.
Normalize deprecated/unrecognized activeMode values to RMA, and short-circuit panel IPC connection logic.
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; introduces TargetSnapshot and extends PendingStopReplacement for bracket restoration metadata.
src/V12_002.Trailing.cs
Captures target snapshots before stop cancel and triggers target restoration after emergency stop creation.
src/V12_002.Orders.Management.cs
Re-links replacement stops to the original OCO group; adds RestoreCascadedTargets to re-submit cascade-cancelled targets.
src/V12_002.Orders.Callbacks.cs
Restores targets after master stop replacement; adds follower stop-cancel handling via OnAccountOrderUpdate path.
src/V12_002.Entries.Retest.cs
Early-return after NULL unmanaged submit to avoid subsequent state latching/dispatch.
The reason will be displayed to describe this comment to others. Learn more.
In the unmanaged/local restore path, the target signal name ("T{n}_" + entryName) is not trimmed and can exceed the established max length for target signals. Use the same trimming approach as the follower path (e.g., SymmetryTrim to 40 chars / ensure <= 50) to avoid NinjaTrader signal truncation/collisions.
The reason will be displayed to describe this comment to others. Learn more.
ConnectToStrategy now has an unconditional return before a try block, leaving the subsequent statements unreachable. This will fail compilation (unreachable code). If IPC is deprecated, remove/comment the old implementation or gate it behind a conditional compilation symbol instead of returning mid-method.
The reason will be displayed to describe this comment to others. Learn more.
walkthrough.md references BUILD_TAG = "950" and suggests verification via IPC commands, but this PR updates BUILD_TAG to 954 and also disables IPC connectivity in the panel. Please update the walkthrough to match the current build number and the current (non-IPC) verification procedure so operators aren’t following a stale runbook.
The reason will be displayed to describe this comment to others. Learn more.
In the follower branch, the result of ExecutingAccount.CreateOrder is submitted immediately, before checking for null. If CreateOrder returns null, Submit(new[] { newStop }) can throw and the later null-handling/flatten path is bypassed, leaving the position potentially unprotected. Null-guard the order before Submit (and consider wrapping Submit in try/catch consistent with the repo’s CreateOrder-null convention).
Suggested change
pos.ExecutingAccount.Submit(new[]{newStop});
// Guard against null CreateOrder results and submit failures to preserve later null-handling logic.
if(newStop!=null)
{
try
{
pos.ExecutingAccount.Submit(new[]{newStop});
}
catch(ExceptionsubmitEx)
{
// On submit failure, fall back to the same path as a null order (newStop == null).
Print("Error submitting follower replacement stop for "+entryName+": "+submitEx.Message);
The reason will be displayed to describe this comment to others. Learn more.
When pendingStopReplacements.TryAdd fails, the snapshot block populates CapturedTargets on the local newPending object, which is not the instance stored in pendingStopReplacements. This can drop the captured target snapshot entirely on the TryAdd-failed path. Populate CapturedTargets/BracketRestorationNeeded on the actual pending instance in the dictionary (either the one returned by TryGetValue or via AddOrUpdate pattern).
The reason will be displayed to describe this comment to others. Learn more.
Remove unconditional return in ConnectToStrategy
ConnectToStrategy() now exits immediately, so SendCommand() can never establish tcpStream and every panel action silently becomes a no-op. This is a functional regression because the strategy still starts and uses the IPC server (StartIpcServer() in src/V12_002.cs and command handling in src/V12_002.UI.IPC.cs), so disabling connection at the indicator side breaks runtime control/sync flows rather than removing dead code.
FINDING 1 (CRITICAL) -- V12_002.Trailing.cs:
- Snapshot CapturedTargets/BracketRestorationNeeded BEFORE pendingStopReplacements.TryAdd
in both TryAdd paths (CancelPending/Submitted and Working/Accepted state handlers).
- Eliminated race window where stop-cancel callback could read a partially-constructed
PendingStopReplacement with BracketRestorationNeeded=false and skip RestoreCascadedTargets.
- Removed now-redundant post-TryAdd snapshot blocks (were dead/duplicate after fix).
FINDING 2 (HIGH) -- V12_002.Orders.Callbacks.cs:
- HandleOrderCancelled (master path): snapshot RemainingContracts under stateLock once;
use same snapshot for guard check and CreateNewStopOrder call. Eliminates 0-qty stop risk.
- OnAccountOrderUpdate (follower path): move RemainingContracts guard inside stateLock so
check and use are atomic. Eliminates TOCTOU on follower stop replacement qty.
FINDING 3 (LOW) -- V12_001.cs:
- Remove unreachable try/catch body after intentional return stub in ConnectToStrategy().
SendCommand() safely no-ops via tcpStream null guard -- no behavior change.
Bump BUILD_TAG -> 955. ASCII gate: PASS.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This PR contained a critical race condition in V12_002.Trailing.cs identified by Greptile (Confidence 2/5 -- not safe to merge): CapturedTargets was populated AFTER TryAdd, leaving a window where a stop-cancel callback would see BracketRestorationNeeded = false and silently skip OCO bracket restoration.
FINDING 1 (CRITICAL) -- V12_002.Trailing.cs:
- Snapshot CapturedTargets/BracketRestorationNeeded BEFORE pendingStopReplacements.TryAdd
in both TryAdd paths (CancelPending/Submitted and Working/Accepted state handlers).
- Eliminated race window where stop-cancel callback could read a partially-constructed
PendingStopReplacement with BracketRestorationNeeded=false and skip RestoreCascadedTargets.
- Removed now-redundant post-TryAdd snapshot blocks (were dead/duplicate after fix).
FINDING 2 (HIGH) -- V12_002.Orders.Callbacks.cs:
- HandleOrderCancelled (master path): snapshot RemainingContracts under stateLock once;
use same snapshot for guard check and CreateNewStopOrder call. Eliminates 0-qty stop risk.
- OnAccountOrderUpdate (follower path): move RemainingContracts guard inside stateLock so
check and use are atomic. Eliminates TOCTOU on follower stop replacement qty.
FINDING 3 (LOW) -- V12_001.cs:
- Remove unreachable try/catch body after intentional return stub in ConnectToStrategy().
SendCommand() safely no-ops via tcpStream null guard -- no behavior change.
Bump BUILD_TAG -> 955. ASCII gate: PASS.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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
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.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Surgical fixes for IPC ghost connection, RETEST null order leak, and activeMode normalization. Injected Section 12 into Standards Manifesto for Maximum Autonomy and Protected Legacy Archive.