fix(core): Phase 2 Omni-Audit remediation -- 9 defects (A1-1..A3-3)#30
fix(core): Phase 2 Omni-Audit remediation -- 9 defects (A1-1..A3-3)#30mkalhitti-cloud wants to merge 4 commits into
Conversation
…ks across all entry modules - FFMA, MOMO, OR, RMA (ExecuteRMAEntry/Custom/TrendSplit), Trend (ExecuteTRENDEntry/ManualEntry): deferred activePositions/entryOrders writes to AFTER null check on SubmitOrderUnmanaged; wrapped both assignments in lock(stateLock); added clean rollback + return on null submit. - Trailing.cs: wrapped three stopOrders[entryName] write sites in lock(stateLock). - Orders.Management.cs: wrapped two stopOrders[entryName] write sites in lock(stateLock). - Symmetry.cs: moved pos.EntryFilled = true inside existing lock(stateLock) block so EntryFilled and RemainingContracts are mutated atomically. Resolves Build 960 audit defects: Area 1 item 1 and Area 2 item 1. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A1-2: Added StampReaperMoveGrace() before Cancel in follower target replacements to open a 5-second REAPER suppression window, matching the established PropagateMasterTargetMove pattern: - Symmetry.cs SymmetryGuardReplaceExistingFollowerTarget: guard before Cancel, wrapped dict[fleetEntryName] write in lock(stateLock). - Trailing.cs MoveSpecificTarget: guard before follower target Cancel. A1-3: Wrapped the FSM PendingCancel->Submitting state transition snapshot in lock(stateLock) inside OnAccountOrderUpdate. This closes the TOCTOU window where PropagateFollowerEntryReplace (holding stateLock) could write to PendingQty/PendingPrice simultaneously with the callback thread reading them. Resolves Build 960 audit defects: Area 1 items 2 and 3. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ncel expectedPositions rollback A2-2: Added PendingCleanup and FlattenAttemptCount fields to PositionInfo. Replaced immediate activePositions.TryRemove after RequestStopCancelLifecycleSafe in the final-target and trim-close flows with pos.PendingCleanup = true. Added deferred TryRemove logic to both HandleOrderCancelled (master stop) and OnAccountOrderUpdate (follower stop) -- purge fires only on broker-confirmed stop terminal state. A2-3: Removed DeltaExpectedPositionLocked from SymmetryGuardCascadeFollowerCleanup (was firing immediately on cancel request). Moved the confirmed-cancel delta rollback into the non-FSM follower entry cancelled branch in OnAccountOrderUpdate -- rollback now fires only after OrderState.Cancelled confirmed by the broker, eliminating the microsecond fill-race desync between the master cancel request and broker confirmation. Resolves Build 960 audit defects: Area 2 items 2 and 3. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A3-1 (V12_002.SIMA.cs): - PumpFleetDispatch: hard guard drains queue if SIMA disabled or flatten running - ApplySimaState disable path: drain _pendingFleetDispatches after Reaper stop A3-2 (V12_002.REAPER.cs): - ExecuteReaperRepair: isFlattenRunning guard as first statement - Background REAPER thread: _repairInFlight.Add moved before TriggerCustomEvent to prevent double-enqueue in next audit cycle A3-3 (V12_002.Trailing.cs): - UpdateStopOrder null-stop path: FlattenAttemptCount circuit breaker (cap at 3) - UpdateStopOrder catch path: same circuit breaker guards emergency flatten - Reset FlattenAttemptCount to 0 on successful stop submission Build 960 Phase 2 Omni-Audit -- Area 3 complete. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces thread-safe state updates and null-abort handling across order submission and callback paths. Key changes include wrapping position and order updates in stateLock blocks, adding null-checks for failed order submissions with early returns, introducing deferred cleanup logic for stop-terminal positions, and implementing a circuit-breaker mechanism for emergency flatten attempts. Changes
Sequence DiagramsequenceDiagram
participant Caller as Entry Path
participant SubmitOrder as SubmitOrderUnmanaged
participant StateMgr as State Manager
participant Lock as stateLock
participant Pos as activePositions / entryOrders
Caller->>SubmitOrder: Submit order with entryName & position
activate SubmitOrder
SubmitOrder-->>Caller: entryOrder (null or valid)
deactivate SubmitOrder
alt entryOrder is null
Caller->>Caller: Log [ENTRY_ABORT]
Caller->>Caller: Return early (no state mutation)
else entryOrder is valid
Caller->>Lock: Acquire stateLock
activate Lock
Lock->>StateMgr: Begin atomic update
StateMgr->>Pos: Update activePositions[entryName]
StateMgr->>Pos: Update entryOrders[entryName]
StateMgr-->>Lock: State synchronized
Lock->>Caller: Release lock
deactivate Lock
Caller->>Caller: Continue with visual confirmation & tracking
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
 |
|
Overall Grade Focus Area: Complexity |
Security Reliability Complexity Hygiene |
Feedback
- Monolithic handler functions accumulating business rules
- Several large handlers cram routing, validation and state changes into one method, creating many branches and duplicated logic; extract focused domain services and small pure helpers so each path is simple and testable.
- Implicit state passed through flags and parameters
- Behavior toggles via numerous booleans and scattered params, producing combinatorial branching across handlers; consolidate into an explicit state object or small state machine/strategy to make choices explicit and reduce branches.
- Error handling intertwines with the happy path
- Guards, retries and error branches are mixed into core logic, nesting and raising cyclomatic counts; isolate validation/error handling (early returns or result types) so the main flow is linear and conditional complexity falls.
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Mar 8, 2026 10:57p.m. | Review ↗ |
| Secrets |  |
Mar 8, 2026 10:57p.m. | Review ↗ |
🤖 Antigravity Auto-Auditor (Gemini 2.5 Pro via Vertex AI)Accessing GEMINI.md Standards... V12.Phase7 Protocols Engaged. AUDIT REPORTSubject: PR Audit (Build 960 Hardening Pass) This pull request represents a comprehensive and exceptionally high-quality hardening of the strategy's core concurrency and state management logic. The changes demonstrate a deep understanding of potential race conditions and strictly adhere to the project's GEMINI.md standards. Systematic Review:1.
|
|
──────────────────────────────────────────────────────────────────────────────── https://aider.chat/HISTORY.html#release-notes Initial repo scan can be slow in larger repos, but only happens once. https://platform.openai.com/docs/guides/error-codes/api-errors |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67e94d0872
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isFlattenRunning) | ||
| { | ||
| Print("[REAPER REPAIR] Aborted -- flatten in progress."); | ||
| return; |
There was a problem hiding this comment.
ExecuteReaperRepair now returns before entering the inner try/finally, but _repairInFlight is set earlier in AuditApexPositions before enqueueing the repair. If a queued repair is processed while isFlattenRunning is true, this early return leaves the key permanently in _repairInFlight, and subsequent audits skip that account (_repairInFlight.Contains(repairKey)), so follower desync repairs can be blocked indefinitely after flatten completes.
Useful? React with 👍 / 👎.
| RequestStopCancelLifecycleSafe(entryName); | ||
| activePositions.TryRemove(entryName, out _); | ||
| SymmetryGuardForgetEntry(entryName); | ||
| PositionInfo closedPos; | ||
| if (activePositions.TryGetValue(entryName, out closedPos) && closedPos != null) | ||
| closedPos.PendingCleanup = true; |
There was a problem hiding this comment.
This branch always defers cleanup by setting PendingCleanup, but RequestStopCancelLifecycleSafe immediately removes the stop reference when the stop is already terminal (Cancelled/Filled/Rejected/Unknown). In that timing, no later stop-terminal callback can match the order and run the deferred purge, so activePositions can retain zero-quantity stale entries indefinitely, which can pollute later symmetry/reaper decisions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 2 Omni-Audit remediations across the V12 strategy, focusing on hardening concurrency/state mutation rules, tightening REAPER/SIMA lifecycle behavior, and deferring metadata cleanup until broker-confirmed terminal states.
Changes:
- Wraps strategy state mutations (notably
activePositions/entryOrders/stopOrders) underlock(stateLock)and adds null-abort rollback patterns across multiple entry paths. - Adds deferred cleanup via
PositionInfo.PendingCleanupand routes final purges to broker-confirmed stop terminal/cancel handlers. - Hardens queue/thread behavior (SIMA dispatch draining/aborts, REAPER repair in-flight gating) and adds a stop-submit emergency-flatten circuit breaker.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/V12_002.cs | Extends PositionInfo with PendingCleanup and FlattenAttemptCount to support deferred purge + circuit breaker behavior. |
| src/V12_002.Trailing.cs | Locks stopOrders writes, adds emergency-flatten circuit breaker, stamps REAPER grace before follower target replace. |
| src/V12_002.Symmetry.cs | Stamps REAPER grace before follower target cancel/replace; locks follower replace dict mutation; moves EntryFilled write under stateLock; defers delta rollback. |
| src/V12_002.SIMA.cs | Drains/aborts pending fleet dispatch queue when SIMA disabled or flattening; drains queue on SIMA shutdown. |
| src/V12_002.REAPER.cs | Moves _repairInFlight marking earlier; adds flatten guard at start of repair execution; adjusts in-flight comment/flow. |
| src/V12_002.Orders.Management.cs | Wraps stopOrders mutations under stateLock in bracket/stop creation paths. |
| src/V12_002.Orders.Callbacks.cs | Adds deferred PendingCleanup purge on stop terminal; snapshots FSM fields under stateLock; defers follower expected-position rollback to confirmed cancel; sets PendingCleanup on close/trim. |
| src/V12_002.Entries.Trend.cs | Applies null-abort rollback and stateLock wraps for TREND entries; stages state after submit success. |
| src/V12_002.Entries.RMA.cs | Applies null-abort + stateLock wraps for RMA entries; adjusts TrendSplit submit handling. |
| src/V12_002.Entries.OR.cs | Applies null-abort rollback and stateLock wraps for OR entries; stages state after submit success. |
| src/V12_002.Entries.MOMO.cs | Applies null-abort rollback and stateLock wraps for MOMO entries; stages state after submit success. |
| src/V12_002.Entries.FFMA.cs | Applies null-abort handling and stateLock wraps for FFMA entries; stages state after submit success. |
| newStop = pos.ExecutingAccount.CreateOrder(Instrument, pos.Direction == MarketPosition.Long ? OrderAction.Sell : OrderAction.BuyToCover, | ||
| OrderType.StopMarket, TimeInForce.Gtc, pos.RemainingContracts, 0, validatedStopPrice, "Stop_" + entryName, "Stop_" + entryName, null); | ||
| pos.ExecutingAccount.Submit(new[] { newStop }); | ||
| stopOrders[entryName] = newStop; | ||
| // A1-1: stopOrders mutation inside stateLock (Build 960 audit fix) | ||
| lock (stateLock) { stopOrders[entryName] = newStop; } |
There was a problem hiding this comment.
In the follower-account path, CreateOrder(...) can return null, but Submit(new[] { newStop }) is called unconditionally. If newStop is null this will throw before the later newStop == null handling, leaving the position unprotected and bypassing the new circuit-breaker logic. Add a null-guard before Submit and route the failure through the existing null-stop handling path.
| // A3-3: Circuit breaker -- cap consecutive flatten attempts to 3 (Build 960 audit fix) | ||
| PositionInfo cbPos; | ||
| if (activePositions.TryGetValue(entryName, out cbPos) && cbPos != null) | ||
| { | ||
| cbPos.FlattenAttemptCount++; | ||
| if (cbPos.FlattenAttemptCount > 3) | ||
| { | ||
| Print(string.Format("[CIRCUIT BREAKER] Emergency flatten halted after 3 consecutive failures for {0}. Manual intervention required.", entryName)); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
FlattenAttemptCount is incremented/read without lock(stateLock) / Interlocked, but PositionInfo is explicitly mutated from multiple threads elsewhere (e.g., RemainingContracts is volatile). As written, the counter can race (lost increments / stale reads), making the circuit-breaker unreliable. Protect FlattenAttemptCount with Interlocked.Increment/Volatile.Read+Volatile.Write, or mutate/read it under lock(stateLock) consistently.
| // A1-1/A2-1: Null-abort + stateLock wrap for E2 (Build 960 audit fix) | ||
| if (entryOrder2 == null) | ||
| { | ||
| Print("[ENTRY_ABORT] TrendSplit E2 SubmitOrderUnmanaged returned null for " + entry2Name + ". Rolling back."); | ||
| // E1 already submitted -- log but continue without E2 tracking | ||
| } | ||
| else | ||
| { | ||
| lock (stateLock) { activePositions[entry2Name] = pos2; entryOrders[entry2Name] = entryOrder2; } | ||
| masterEntryNames.Add(entry2Name); | ||
| } |
There was a problem hiding this comment.
entryOrder2 == null is logged but the method continues using the original qty15/finalTotalQty and can still call ExecuteSmartDispatchEntry(...). This can dispatch follower quantity that includes the missing EMA15 leg, creating a master/follower mismatch and potential REAPER repair loop. On E2 submit failure, either (a) cancel/tear down E1 and abort, or (b) adjust quantities and skip SIMA dispatch for the missing leg (and avoid registering/linking E2 metadata).
| if (entryOrder1 == null) | ||
| { | ||
| Print("[ENTRY_ABORT] TrendSplit E1 SubmitOrderUnmanaged returned null for " + entry1Name + ". Rolling back."); | ||
| return; |
There was a problem hiding this comment.
This log message says "Rolling back" but there is no rollback action in this branch (no expectedPositions reservation and no state to undo). Either remove the rollback wording or add the actual rollback/cleanup that’s intended.
| PositionInfo cleanupPos; | ||
| if (activePositions.TryGetValue(kvp.Key, out cleanupPos) && cleanupPos != null | ||
| && cleanupPos.PendingCleanup && cleanupPos.RemainingContracts <= 0) | ||
| { | ||
| lock (stateLock) { activePositions.TryRemove(kvp.Key, out _); } |
There was a problem hiding this comment.
The deferred purge check reads cleanupPos.PendingCleanup (non-volatile) and cleanupPos.RemainingContracts outside lock(stateLock). If these fields can change concurrently, the purge decision can be inconsistent (missed purge or premature purge). Snapshot the fields under lock(stateLock) (or make PendingCleanup volatile and use Volatile.Read) before deciding to remove.
| PositionInfo cleanupPos; | |
| if (activePositions.TryGetValue(kvp.Key, out cleanupPos) && cleanupPos != null | |
| && cleanupPos.PendingCleanup && cleanupPos.RemainingContracts <= 0) | |
| { | |
| lock (stateLock) { activePositions.TryRemove(kvp.Key, out _); } | |
| bool shouldPurge = false; | |
| PositionInfo cleanupPos; | |
| lock (stateLock) | |
| { | |
| if (activePositions.TryGetValue(kvp.Key, out cleanupPos) && cleanupPos != null | |
| && cleanupPos.PendingCleanup && cleanupPos.RemainingContracts <= 0) | |
| { | |
| activePositions.TryRemove(kvp.Key, out _); | |
| shouldPurge = true; | |
| } | |
| } | |
| if (shouldPurge) | |
| { |
| PositionInfo _scPos; | ||
| if (activePositions.TryGetValue(_sc.Key, out _scPos) && _scPos != null | ||
| && _scPos.PendingCleanup && _scPos.RemainingContracts <= 0) | ||
| { | ||
| lock (stateLock) { activePositions.TryRemove(_sc.Key, out _); } |
There was a problem hiding this comment.
Same deferred purge race on the follower-stop terminal path: PendingCleanup/RemainingContracts are read outside synchronization before deciding to purge activePositions. Use the same snapshot/synchronization approach here as in the master-stop cancel path.
| PositionInfo _scPos; | |
| if (activePositions.TryGetValue(_sc.Key, out _scPos) && _scPos != null | |
| && _scPos.PendingCleanup && _scPos.RemainingContracts <= 0) | |
| { | |
| lock (stateLock) { activePositions.TryRemove(_sc.Key, out _); } | |
| bool _purged = false; | |
| lock (stateLock) | |
| { | |
| PositionInfo _scPos; | |
| if (activePositions.TryGetValue(_sc.Key, out _scPos) && _scPos != null | |
| && _scPos.PendingCleanup && _scPos.RemainingContracts <= 0) | |
| { | |
| activePositions.TryRemove(_sc.Key, out _); | |
| _purged = true; | |
| } | |
| } | |
| if (_purged) | |
| { |
| // A3-2: Mark in-flight BEFORE TriggerCustomEvent to block double-enqueue in next audit cycle (Build 960 audit fix) | ||
| lock (stateLock) { _repairInFlight.Add(repairKey); } | ||
| _reaperRepairQueue.Enqueue(acct.Name); | ||
| try { TriggerCustomEvent(o => ProcessReaperRepairQueue(), null); } catch { } |
There was a problem hiding this comment.
_repairInFlight is set before scheduling TriggerCustomEvent, but if TriggerCustomEvent(...) throws, the code swallows the exception and never clears _repairInFlight. That can permanently block future repairs for this repairKey (the audit loop will see it as already in-flight). Clear _repairInFlight (and optionally the queued item) in the catch path, or only set the in-flight flag after TriggerCustomEvent is successfully scheduled.
| try { TriggerCustomEvent(o => ProcessReaperRepairQueue(), null); } catch { } | |
| try | |
| { | |
| TriggerCustomEvent(o => ProcessReaperRepairQueue(), null); | |
| } | |
| catch (Exception ex) | |
| { | |
| // If scheduling fails, clear the in-flight guard so future audits can re-enqueue repairs. | |
| lock (stateLock) | |
| { | |
| _repairInFlight.Remove(repairKey); | |
| } | |
| if (shouldLog) | |
| Print($"[REAPER] ERROR: Failed to schedule repair via TriggerCustomEvent for {acct.Name}: {ex.Message}"); | |
| } |
| // A3-2: Abort immediately if a flatten is in progress (Build 960 audit fix) | ||
| if (isFlattenRunning) | ||
| { | ||
| Print("[REAPER REPAIR] Aborted -- flatten in progress."); | ||
| return; | ||
| } | ||
|
|
||
| string repairKey = accountName + "_" + Instrument.FullName; |
There was a problem hiding this comment.
ExecuteReaperRepair returns early when isFlattenRunning is true, but _repairInFlight for this account was already set on the background thread. Because this return happens before the inner try/finally that removes the flag, the in-flight guard can remain set indefinitely and block all subsequent repairs. Ensure this early-abort path removes _repairInFlight for repairKey (or defer setting it until after the flatten guard).
| // A3-2: Abort immediately if a flatten is in progress (Build 960 audit fix) | |
| if (isFlattenRunning) | |
| { | |
| Print("[REAPER REPAIR] Aborted -- flatten in progress."); | |
| return; | |
| } | |
| string repairKey = accountName + "_" + Instrument.FullName; | |
| string repairKey = accountName + "_" + Instrument.FullName; | |
| // A3-2: Abort immediately if a flatten is in progress (Build 960 audit fix) | |
| if (isFlattenRunning) | |
| { | |
| Print("[REAPER REPAIR] Aborted -- flatten in progress."); | |
| // REAPER Safety: ensure any pre-set in-flight guard is cleared on early abort | |
| _repairInFlight.TryRemove(repairKey, out _); | |
| return; | |
| } |
| // A3-3: Circuit breaker -- cap consecutive flatten attempts to 3 (Build 960 audit fix) | ||
| PositionInfo exCbPos; | ||
| bool flattenBlocked = false; | ||
| if (activePositions.TryGetValue(entryName, out exCbPos) && exCbPos != null) | ||
| { | ||
| Print(string.Format("(!) Attempting emergency flatten for {0}...", entryName)); | ||
| FlattenPositionByName(entryName); | ||
| exCbPos.FlattenAttemptCount++; | ||
| if (exCbPos.FlattenAttemptCount > 3) | ||
| { | ||
| Print(string.Format("[CIRCUIT BREAKER] Emergency flatten halted after 3 consecutive failures for {0}. Manual intervention required.", entryName)); | ||
| flattenBlocked = true; | ||
| } |
There was a problem hiding this comment.
Same race as above in the exception path: exCbPos.FlattenAttemptCount++ and the threshold check are not synchronized, so the circuit-breaker behavior can be inconsistent under concurrent access. Use the same synchronization approach (Interlocked or lock(stateLock)) here as in the non-exception path.
| if (activePositions.TryGetValue(entryName, out closedPos) && closedPos != null) | ||
| closedPos.PendingCleanup = true; | ||
| else |
There was a problem hiding this comment.
PendingCleanup is written without lock(stateLock)/Volatile.Write, but it is later read in other handlers to decide whether to purge metadata. To avoid visibility races (and to keep the stateLock discipline consistent with other PositionInfo mutations), set PendingCleanup under lock(stateLock) or make the field volatile and use Volatile.Read/Write at all access sites.
| if (activePositions.TryGetValue(entryName, out closedPos) && closedPos != null) | |
| closedPos.PendingCleanup = true; | |
| else | |
| lock (stateLock) | |
| { | |
| activePositions.TryGetValue(entryName, out closedPos); | |
| if (closedPos != null) | |
| { | |
| closedPos.PendingCleanup = true; | |
| } | |
| } | |
| if (closedPos == null) |
Greptile SummaryThis PR remediates 9 defects (A1-1 through A3-3) identified in a concurrency/safety audit across the NinjaTrader V12 strategy: it adds Key changes:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant BG as REAPER Background Thread
participant ST as Strategy Thread (ExecuteReaperRepair)
participant IFM as _repairInFlight (ConcurrentDictionary)
Note over BG: Audit detects flat account with expected ≠ 0
BG->>IFM: lock(stateLock) { Add(repairKey) } [A3-2 NEW]
BG->>BG: _reaperRepairQueue.Enqueue(acct.Name)
BG->>ST: TriggerCustomEvent → ProcessReaperRepairQueue
alt isFlattenRunning == true [NEW GUARD]
ST-->>ST: return early (line 536)
Note over IFM: repairKey NEVER removed — permanent leak
else repairPos == null [pre-existing guard]
ST-->>ST: return early (line 571)
Note over IFM: repairKey NEVER removed — permanent leak
else price fence / distance exceeded [pre-existing guards]
ST-->>ST: return early (lines 608–642)
Note over IFM: repairKey NEVER removed — permanent leak
else all guards pass
ST->>ST: enter inner try block (line 645)
ST->>ST: submit repair order
ST->>IFM: finally: Remove(repairKey) ✓ cleaned up
end
Note over BG: Next audit cycle
BG->>IFM: Contains(repairKey) → true
BG-->>BG: "repair already in-flight -- skipping" (infinite skip)
Last reviewed commit: 67e94d0 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (entryOrder2 == null) | ||
| { | ||
| AddExpectedPositionDeltaLocked(ExpKey(Account.Name), -masterDeltaE2); | ||
| Print("[ERROR][1102Y-V3] TREND E2 SubmitOrderUnmanaged NULL for " + entry2Name + " -- rolled back."); | ||
| Print("[ENTRY_ABORT] TREND E2 SubmitOrderUnmanaged NULL for " + entry2Name + " -- rolled back."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
E2 null-abort leaves E1 tracked but never fleet-dispatched
When entryOrder2 == null, the function returns immediately at line 250. At this point:
activePositions[entry1Name]andentryOrders[entry1Name]are already set (line 234).linkedTRENDEntries[entry1Name] = entry2NameandlinkedTRENDEntries[entry2Name] = entry1Nameare already set (lines 215-216).- The call to
ExecuteSmartDispatchEntry(entry1Name, masterEntryNames, ...)that follows line 252 is never reached.
This leaves E1 as a live master position in activePositions with a stale forward-link to a non-existent E2, and without a fleet dispatch. The REAPER audit will eventually detect the follower accounts as flat (expected ≠ 0) and attempt repairs indefinitely.
By contrast, the analogous TrendSplit path in RMA.cs handles E2 failure gracefully — it logs, skips E2 from masterEntryNames, and lets ExecuteSmartDispatchEntry proceed with only E1:
if (entryOrder2 == null)
{
AddExpectedPositionDeltaLocked(ExpKey(Account.Name), -masterDeltaE2);
Print("[ENTRY_ABORT] TREND E2 SubmitOrderUnmanaged NULL for " + entry2Name + " -- rolled back.");
// Do NOT return -- E1 already submitted; continue dispatch with E1-only list
}
else
{
lock (stateLock) { activePositions[entry2Name] = pos2; entryOrders[entry2Name] = entryOrder2; }
masterEntryNames.Add(entry2Name);
}The linkedTRENDEntries stale entries should also be cleaned up if E2 is skipped.
| // A3-2: Abort immediately if a flatten is in progress (Build 960 audit fix) | ||
| if (isFlattenRunning) | ||
| { | ||
| Print("[REAPER REPAIR] Aborted -- flatten in progress."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
_repairInFlight permanently leaked on early return
The isFlattenRunning guard exits before repairKey is declared (line 539) and before the inner try/finally cleanup block (line 702). Because the background thread already called _repairInFlight.Add(repairKey) at line 289 before calling TriggerCustomEvent, this early return leaves the key permanently in the set with no cleanup path.
The same leak affects every other early return inside the outer try block that precedes the inner try/finally at line 645 — specifically the guards for repairPos == null (line 571), !TryGetRepairDistanceLimitPoints (line 608), hardBoundDiff > repairLimitPoints (line 616), the market-order price fence (line 633), and targetAcct == null (line 642).
After any of these exits, _repairInFlight.Contains(repairKey) returns true forever. The REAPER audit loop skips enqueuing when the key is present (line 307 log: "repair already in-flight"), so the affected account is silently blocked from ever being repaired again — even after a flatten completes — until the strategy is restarted.
Before this PR the Add happened inside ExecuteReaperRepair at the old step-4 position, after all guards, so every early return was clean. Moving the Add to the background thread introduced this gap.
The fix is to move repairKey above the isFlattenRunning check and wrap the entire method body (not just the inner repair block) in a top-level try/finally that always calls _repairInFlight.Remove(repairKey). The inner try/finally at line 645 can then be folded into the outer one.
| // A2-3: Direction-aware delta rollback on CONFIRMED cancel -- deferred from SymmetryGuardCascadeFollowerCleanup | ||
| // to prevent REAPER desync on microsecond fill race (Build 960 audit fix). | ||
| PositionInfo cancelledFollowerPos; | ||
| if (activePositions.TryGetValue(matchedEntry, out cancelledFollowerPos) && cancelledFollowerPos != null) | ||
| { | ||
| string cancelAcctKey = cancelledFollowerPos.ExecutingAccount != null | ||
| ? cancelledFollowerPos.ExecutingAccount.Name : Account.Name; | ||
| int cancelDelta = (cancelledFollowerPos.Direction == MarketPosition.Long) | ||
| ? -cancelledFollowerPos.TotalContracts : cancelledFollowerPos.TotalContracts; | ||
| DeltaExpectedPositionLocked(ExpKey(cancelAcctKey), cancelDelta); | ||
| } |
There was a problem hiding this comment.
Delta rollback silently dropped when position has already left activePositions
The A2-3 rollback is guarded by activePositions.TryGetValue(matchedEntry, ...). If the position was already removed from activePositions before this confirmed-cancel callback fires — for instance by CleanupPosition, an early flatten, or a race between two cancel confirmations — TryGetValue returns false and DeltaExpectedPositionLocked is never called, with no log message.
In the removed SymmetryGuardCascadeFollowerCleanup code (Build 930.1 P1) the rollback used the pos parameter already in scope and was unconditional. Moving it here makes correctness dependent on activePositions still holding the entry at callback time, which is not guaranteed under the new concurrent deferred-cleanup paths added by this same PR.
If the delta is silently skipped, expectedPositions for the follower account remains non-zero while the broker is flat. The REAPER audit will continuously detect a desync and keep enqueuing repairs.
Adding a Print warning in the else branch when the position is not found would at minimum make the silent skip detectable in logs, and allow targeted debugging of any resulting desync loop.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/V12_002.Entries.Trend.cs (1)
214-216:⚠️ Potential issue | 🟠 MajorAny TREND submit failure still needs a full rollback.
The new null aborts are better, but
linkedTRENDEntriesis populated before either leg is proven live, so an E2 abort can strand a one-sided TREND pair. Separately, the dual-leg and manual paths only undo the master reservation onnull; a thrownSubmitOrderUnmanagedstill leavesexpectedPositionsdirty. Roll back the links, any already-created E1 state, and the reserved delta on every failure after E1 setup.Also applies to: 227-250, 278-280, 403-417, 442-444
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/V12_002.Entries.Trend.cs` around lines 214 - 216, linkedTRENDEntries is populated before both TREND legs are proven live, so partial failures (SubmitOrderUnmanaged or null aborts) can leave one-sided state and dirty expectedPositions; fix by deferring writes to linkedTRENDEntries until both legs are fully created/confirmed, and on any failure after E1 setup (including within dual-leg and manual paths) undo: remove any entries from linkedTRENDEntries, delete/rollback any created E1 state, and revert the reserved delta in expectedPositions (and the master reservation) so no partial reservations remain; update the failure/exception handling paths around the functions that perform E1/E2 creation and the SubmitOrderUnmanaged catch blocks to perform these compensating removals/rollbacks atomically.src/V12_002.Entries.MOMO.cs (1)
146-160:⚠️ Potential issue | 🟠 MajorRollback
masterDeltaMOMOwhenSubmitOrderUnmanagedthrows, not only when it returnsnull.The reservation happens before submit, but the outer
catchonly logs. A thrown submit leavesexpectedPositionsnon-zero even though no master order exists, which can cascade into false REAPER state downstream.Also applies to: 182-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/V12_002.Entries.MOMO.cs` around lines 146 - 160, The code reserves masterDeltaMOMO via AddExpectedPositionDeltaLocked before calling SubmitOrderUnmanaged but only rolls it back if SubmitOrderUnmanaged returns null; you must also rollback when SubmitOrderUnmanaged throws: wrap the SubmitOrderUnmanaged call in a try/catch around the place where masterDeltaMOMO is added (referencing masterDeltaMOMO, AddExpectedPositionDeltaLocked, SubmitOrderUnmanaged, entryOrder, ExpKey(Account.Name)), and in the catch call AddExpectedPositionDeltaLocked(ExpKey(Account.Name), -masterDeltaMOMO), log the exception with context (entryName) and then rethrow or return as the surrounding error handling expects; apply the same pattern to the other SubmitOrderUnmanaged usage mentioned (the similar block around the other entry path).src/V12_002.Symmetry.cs (1)
555-579:⚠️ Potential issue | 🔴 CriticalRoute this target replace through the follower Replace FSM.
This still does a raw
Cancel()followed immediately byCreateOrder/Submit(). If the cancel confirm lags, you can end up with overlapping follower targets, which is exactly the ghost-order case the existing_followerReplaceSpecsflow is meant to prevent. Based on learnings: "Follower order cancel+resubmit must use the two-phase Replace FSM with states: PendingCancel -> wait for OnAccountOrderUpdate confirm -> Submitting -> SubmitFollowerReplacement. Use_followerReplaceSpecsdictionary. Never use raw Cancel() followed by Submit()".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/V12_002.Symmetry.cs` around lines 555 - 579, This code is doing a raw cancel+create/submit (StampReaperMoveGrace(); pos.ExecutingAccount.Cancel(...); then CreateOrder/Submit) which can produce overlapping follower targets; instead route this through the follower Replace FSM using the existing _followerReplaceSpecs flow: replace the direct call to pos.ExecutingAccount.Cancel and immediate CreateOrder/Submit in the block where you call StampReaperMoveGrace() and compute newPrice/GetTargetPrice(), by creating or updating an entry in _followerReplaceSpecs for the fleetEntryName with a spec that moves the FSM to PendingCancel, wait for the cancel confirmation in OnAccountOrderUpdate, transition to Submitting, then invoke SubmitFollowerReplacement to create/submit the replacement order (keep using signalName and the rounded price logic from CreateOrder but perform it only in the SubmitFollowerReplacement step); remove the direct pos.ExecutingAccount.Submit and the dict[fleetEntryName] = replacement write and instead let the FSM manage dict/_followerReplaceSpecs lifecycle so only the FSM updates dict with the live replacement order.src/V12_002.Orders.Management.cs (1)
521-531:⚠️ Potential issue | 🔴 CriticalGuard the follower replacement stop before calling
Submit().In
CreateNewStopOrder,CreateOrder()can still returnnull, but the follower path callsSubmit(new[] { newStop })before the later null check. If that happens, the outer catch just logs and returns, so this path never reaches the emergency flatten and can leave the follower naked.Also applies to: 545-558
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/V12_002.Orders.Management.cs` around lines 521 - 531, The follower branch in CreateNewStopOrder calls pos.ExecutingAccount.Submit(new[] { newStop }) without checking if CreateOrder returned null, which can leave a follower naked; update the logic around activePositions/pos and CreateOrder/CreateOrder(...) so you capture the return value into newStop, immediately check if newStop is null before calling pos.ExecutingAccount.Submit, and if null run the same emergency flatten/error handling used later in the method (log and flatten/cleanup) instead of submitting; make the identical null-guard change for the second follower block referenced (lines ~545-558) so both follower paths avoid calling Submit with a null newStop.
🧹 Nitpick comments (1)
src/V12_002.Trailing.cs (1)
678-686: RedundantstopOrderswrite on line 686.The
stopOrders[entryName] = newStopassignment at line 686 is executed after a successful stop submission. However, this write already occurred at line 638 (ExecutingAccount path) or line 650 (SubmitOrderUnmanaged path). The code flow reaches line 686 only after passing through one of those earlier assignments.This is harmless (idempotent write of the same value) but adds unnecessary locking overhead.
Consider removing the redundant write
// A3-3: Reset circuit breaker counter on successful stop submission { PositionInfo cbReset; if (activePositions.TryGetValue(entryName, out cbReset) && cbReset != null) cbReset.FlattenAttemptCount = 0; } -// A1-1: stopOrders final write inside stateLock (Build 960 audit fix) -lock (stateLock) { stopOrders[entryName] = newStop; } pos.CurrentStopPrice = validatedStopPrice; pos.CurrentTrailLevel = newTrailLevel;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/V12_002.Trailing.cs` around lines 678 - 686, Remove the redundant write to stopOrders inside the stateLock: the assignment lock (stateLock) { stopOrders[entryName] = newStop; } is unnecessary because stopOrders[entryName] is already set earlier in the code paths (ExecutingAccount and SubmitOrderUnmanaged) before reaching this block; delete that locked assignment but keep the surrounding circuit-breaker reset code (activePositions.TryGetValue/ cbReset.FlattenAttemptCount = 0) intact to preserve the A3-3 behavior and the lock usage only where required for other state mutations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/V12_002.Entries.FFMA.cs`:
- Around line 172-185: The POSITION_ENTERED notification is sent before the
order is created, causing false-positive panel events; move the
SendResponseToRemote("POSITION_ENTERED|FFMA|{0}", contracts) call to after
SubmitOrderUnmanaged and after the null-check of entryOrder (i.e., only call
SendResponseToRemote when entryOrder != null), referencing the
SubmitOrderUnmanaged call, the entryOrder variable and entryName so the
notification is sent only on successful order creation/assignment.
In `@src/V12_002.REAPER.cs`:
- Around line 288-291: The _repairInFlight entry is added under stateLock before
enqueueing but is not always removed if TriggerCustomEvent throws or the new
flatten guard returns early; ensure every code path that aborts submitting a
repair clears the guard. Update the logic around _repairInFlight.Add(repairKey)
(protected by stateLock) and the enqueue/submit sequence (references:
_repairInFlight, stateLock, _reaperRepairQueue.Enqueue(acct.Name),
TriggerCustomEvent(o => ProcessReaperRepairQueue(), null),
ProcessReaperRepairQueue) so that removal (lock(stateLock) {
_repairInFlight.Remove(repairKey); }) happens in a finally or on every
early-return/exception including the flatten-return path and any catch after
TriggerCustomEvent; apply same fix pattern to the other occurrences noted
(around the blocks currently at lines ~532-537 and ~702-705).
In `@src/V12_002.SIMA.cs`:
- Around line 829-834: The shutdown drain currently just dequeues
_pendingFleetDispatches and drops FleetDispatchRequest entries, leaking their
reservation/barrier state; instead, replicate the rollbacking drain used in
PumpFleetDispatch's abort path: while dequeuing each FleetDispatchRequest from
_pendingFleetDispatches, invoke the same rollback/release logic that clears its
reservation from _dispatchSyncPendingExpKeys (the same code path or helper used
by PumpFleetDispatch to abort a request) before discarding it, then log the
shutdown message; reference PumpFleetDispatch, _pendingFleetDispatches, and
_dispatchSyncPendingExpKeys to locate and reuse the exact rollback behavior.
- Around line 640-646: The abort path that drains _pendingFleetDispatches when
isFlattenRunning || !EnableSIMA silently drops requests and skips the finally
that clears per-submit barriers and decrements _pendingFleetDispatchCount;
instead, when dequeuing each stale entry from _pendingFleetDispatches, perform
the same rollback used on submit failure: call
AddExpectedPositionDeltaLocked(...) with the negative of the reserved delta to
return expectedPositions, clear/remove the entry from
_dispatchSyncPendingExpKeys (remove the barrier), and decrement
_pendingFleetDispatchCount accordingly so state stays consistent; ensure this
rollback logic mirrors the Submit exception handling and that any per-submit
cleanup that normally runs in the finally still occurs (or is replicated) for
these drained items.
In `@src/V12_002.Symmetry.cs`:
- Around line 555-556: The call to StampReaperMoveGrace() writes a global REAPER
grace timestamp and must be replaced with a per-account stamp: compute the
follower's expKey via ExpKey(...) and set _accountFillGraceTicks[expKey] to the
appropriate grace value (instead of touching the legacy global stamp). Update
the code that currently calls StampReaperMoveGrace() to use ExpKey(...) and
assign into the _accountFillGraceTicks dictionary/array so REAPER fill-grace is
scoped to that account only.
---
Outside diff comments:
In `@src/V12_002.Entries.MOMO.cs`:
- Around line 146-160: The code reserves masterDeltaMOMO via
AddExpectedPositionDeltaLocked before calling SubmitOrderUnmanaged but only
rolls it back if SubmitOrderUnmanaged returns null; you must also rollback when
SubmitOrderUnmanaged throws: wrap the SubmitOrderUnmanaged call in a try/catch
around the place where masterDeltaMOMO is added (referencing masterDeltaMOMO,
AddExpectedPositionDeltaLocked, SubmitOrderUnmanaged, entryOrder,
ExpKey(Account.Name)), and in the catch call
AddExpectedPositionDeltaLocked(ExpKey(Account.Name), -masterDeltaMOMO), log the
exception with context (entryName) and then rethrow or return as the surrounding
error handling expects; apply the same pattern to the other SubmitOrderUnmanaged
usage mentioned (the similar block around the other entry path).
In `@src/V12_002.Entries.Trend.cs`:
- Around line 214-216: linkedTRENDEntries is populated before both TREND legs
are proven live, so partial failures (SubmitOrderUnmanaged or null aborts) can
leave one-sided state and dirty expectedPositions; fix by deferring writes to
linkedTRENDEntries until both legs are fully created/confirmed, and on any
failure after E1 setup (including within dual-leg and manual paths) undo: remove
any entries from linkedTRENDEntries, delete/rollback any created E1 state, and
revert the reserved delta in expectedPositions (and the master reservation) so
no partial reservations remain; update the failure/exception handling paths
around the functions that perform E1/E2 creation and the SubmitOrderUnmanaged
catch blocks to perform these compensating removals/rollbacks atomically.
In `@src/V12_002.Orders.Management.cs`:
- Around line 521-531: The follower branch in CreateNewStopOrder calls
pos.ExecutingAccount.Submit(new[] { newStop }) without checking if CreateOrder
returned null, which can leave a follower naked; update the logic around
activePositions/pos and CreateOrder/CreateOrder(...) so you capture the return
value into newStop, immediately check if newStop is null before calling
pos.ExecutingAccount.Submit, and if null run the same emergency flatten/error
handling used later in the method (log and flatten/cleanup) instead of
submitting; make the identical null-guard change for the second follower block
referenced (lines ~545-558) so both follower paths avoid calling Submit with a
null newStop.
In `@src/V12_002.Symmetry.cs`:
- Around line 555-579: This code is doing a raw cancel+create/submit
(StampReaperMoveGrace(); pos.ExecutingAccount.Cancel(...); then
CreateOrder/Submit) which can produce overlapping follower targets; instead
route this through the follower Replace FSM using the existing
_followerReplaceSpecs flow: replace the direct call to
pos.ExecutingAccount.Cancel and immediate CreateOrder/Submit in the block where
you call StampReaperMoveGrace() and compute newPrice/GetTargetPrice(), by
creating or updating an entry in _followerReplaceSpecs for the fleetEntryName
with a spec that moves the FSM to PendingCancel, wait for the cancel
confirmation in OnAccountOrderUpdate, transition to Submitting, then invoke
SubmitFollowerReplacement to create/submit the replacement order (keep using
signalName and the rounded price logic from CreateOrder but perform it only in
the SubmitFollowerReplacement step); remove the direct
pos.ExecutingAccount.Submit and the dict[fleetEntryName] = replacement write and
instead let the FSM manage dict/_followerReplaceSpecs lifecycle so only the FSM
updates dict with the live replacement order.
---
Nitpick comments:
In `@src/V12_002.Trailing.cs`:
- Around line 678-686: Remove the redundant write to stopOrders inside the
stateLock: the assignment lock (stateLock) { stopOrders[entryName] = newStop; }
is unnecessary because stopOrders[entryName] is already set earlier in the code
paths (ExecutingAccount and SubmitOrderUnmanaged) before reaching this block;
delete that locked assignment but keep the surrounding circuit-breaker reset
code (activePositions.TryGetValue/ cbReset.FlattenAttemptCount = 0) intact to
preserve the A3-3 behavior and the lock usage only where required for other
state mutations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dbd6186-e9e5-45a0-9d91-fc423d3e5b8e
📒 Files selected for processing (12)
src/V12_002.Entries.FFMA.cssrc/V12_002.Entries.MOMO.cssrc/V12_002.Entries.OR.cssrc/V12_002.Entries.RMA.cssrc/V12_002.Entries.Trend.cssrc/V12_002.Orders.Callbacks.cssrc/V12_002.Orders.Management.cssrc/V12_002.REAPER.cssrc/V12_002.SIMA.cssrc/V12_002.Symmetry.cssrc/V12_002.Trailing.cssrc/V12_002.cs
| // V12.13-D: Notify connected panel clients of position entry | ||
| string syncMsg = string.Format("POSITION_ENTERED|FFMA|{0}", contracts); | ||
| SendResponseToRemote(syncMsg); | ||
|
|
||
|
|
||
| // Submit MARKET order (immediate execution) | ||
| Order entryOrder = direction == MarketPosition.Long | ||
| ? SubmitOrderUnmanaged(0, OrderAction.Buy, OrderType.Market, contracts, 0, 0, "", entryName) | ||
| : SubmitOrderUnmanaged(0, OrderAction.SellShort, OrderType.Market, contracts, 0, 0, "", entryName); | ||
|
|
||
| entryOrders[entryName] = entryOrder; | ||
| // A1-1/A2-1: Null-abort rollback + stateLock wrap (Build 960 audit fix) | ||
| if (entryOrder == null) | ||
| { | ||
| Print("[ENTRY_ABORT] FFMA SubmitOrderUnmanaged returned null for " + entryName + ". Rolling back."); | ||
| return; |
There was a problem hiding this comment.
Delay the POSITION_ENTERED notification until the order is actually created.
SendResponseToRemote("POSITION_ENTERED|FFMA|...") fires before SubmitOrderUnmanaged. On the new abort path, the panel still gets a false-positive entry event even though no order exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.Entries.FFMA.cs` around lines 172 - 185, The POSITION_ENTERED
notification is sent before the order is created, causing false-positive panel
events; move the SendResponseToRemote("POSITION_ENTERED|FFMA|{0}", contracts)
call to after SubmitOrderUnmanaged and after the null-check of entryOrder (i.e.,
only call SendResponseToRemote when entryOrder != null), referencing the
SubmitOrderUnmanaged call, the entryOrder variable and entryName so the
notification is sent only on successful order creation/assignment.
| // A3-2: Mark in-flight BEFORE TriggerCustomEvent to block double-enqueue in next audit cycle (Build 960 audit fix) | ||
| lock (stateLock) { _repairInFlight.Add(repairKey); } | ||
| _reaperRepairQueue.Enqueue(acct.Name); | ||
| try { TriggerCustomEvent(o => ProcessReaperRepairQueue(), null); } catch { } |
There was a problem hiding this comment.
Always clear _repairInFlight when a queued repair is aborted.
The key is now added before queueing, but the new flatten guard returns before the finally that removes it. If a repair is dequeued during flatten, or TriggerCustomEvent throws after the add, that account can stay permanently "in flight" and REAPER will never retry it. Based on learnings: "_repairInFlight guard must be set before and cleared after every REAPER repair submit".
Also applies to: 532-537, 702-705
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.REAPER.cs` around lines 288 - 291, The _repairInFlight entry is
added under stateLock before enqueueing but is not always removed if
TriggerCustomEvent throws or the new flatten guard returns early; ensure every
code path that aborts submitting a repair clears the guard. Update the logic
around _repairInFlight.Add(repairKey) (protected by stateLock) and the
enqueue/submit sequence (references: _repairInFlight, stateLock,
_reaperRepairQueue.Enqueue(acct.Name), TriggerCustomEvent(o =>
ProcessReaperRepairQueue(), null), ProcessReaperRepairQueue) so that removal
(lock(stateLock) { _repairInFlight.Remove(repairKey); }) happens in a finally or
on every early-return/exception including the flatten-return path and any catch
after TriggerCustomEvent; apply same fix pattern to the other occurrences noted
(around the blocks currently at lines ~532-537 and ~702-705).
| // A3-1: Abort and drain queue if SIMA is disabled or flatten is running (Build 960 audit fix) | ||
| if (isFlattenRunning || !EnableSIMA) | ||
| { | ||
| FleetDispatchRequest stale; | ||
| while (_pendingFleetDispatches.TryDequeue(out stale)) { } | ||
| Print("[PUMP] Abort: SIMA inactive or flatten running. Queue drained."); | ||
| return; |
There was a problem hiding this comment.
Rollback drained dispatches instead of dropping them.
These requests have already reserved expectedPositions, marked _dispatchSyncPendingExpKeys, and registered tracking dictionaries before they reach the queue. This abort path silently discards them, so flatten/disable can leave ghost follower state behind, and _pendingFleetDispatchCount never gets decremented because the early return skips the existing finally.
Proposed fix
+ private void DrainPendingFleetDispatches(string logMessage)
+ {
+ FleetDispatchRequest stale;
+ while (_pendingFleetDispatches.TryDequeue(out stale))
+ {
+ ClearDispatchSyncPending(stale.ExpectedKey);
+ if (stale.ReservedDelta != 0)
+ AddExpectedPositionDeltaLocked(stale.ExpectedKey, -stale.ReservedDelta);
+
+ activePositions.TryRemove(stale.FleetEntryName, out _);
+ entryOrders.TryRemove(stale.FleetEntryName, out _);
+ stopOrders.TryRemove(stale.FleetEntryName, out _);
+ for (int tNum = 1; tNum <= 5; tNum++)
+ {
+ var targetDict = GetTargetOrdersDictionary(tNum);
+ if (targetDict != null)
+ targetDict.TryRemove(stale.FleetEntryName, out _);
+ }
+
+ Interlocked.Decrement(ref _pendingFleetDispatchCount);
+ }
+
+ Print(logMessage);
+ }
...
- if (isFlattenRunning || !EnableSIMA)
- {
- FleetDispatchRequest stale;
- while (_pendingFleetDispatches.TryDequeue(out stale)) { }
- Print("[PUMP] Abort: SIMA inactive or flatten running. Queue drained.");
- return;
- }
+ if (isFlattenRunning || !EnableSIMA)
+ {
+ DrainPendingFleetDispatches("[PUMP] Abort: SIMA inactive or flatten running. Queue drained.");
+ return;
+ }As per coding guidelines, _dispatchSyncPendingExpKeys barriers must be set before and cleared after every fleet submit, and reserved quantities must be rolled back via AddExpectedPositionDeltaLocked(key, -delta) on any fleet Submit exception.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.SIMA.cs` around lines 640 - 646, The abort path that drains
_pendingFleetDispatches when isFlattenRunning || !EnableSIMA silently drops
requests and skips the finally that clears per-submit barriers and decrements
_pendingFleetDispatchCount; instead, when dequeuing each stale entry from
_pendingFleetDispatches, perform the same rollback used on submit failure: call
AddExpectedPositionDeltaLocked(...) with the negative of the reserved delta to
return expectedPositions, clear/remove the entry from
_dispatchSyncPendingExpKeys (remove the barrier), and decrement
_pendingFleetDispatchCount accordingly so state stays consistent; ensure this
rollback logic mirrors the Submit exception handling and that any per-submit
cleanup that normally runs in the finally still occurs (or is replicated) for
these drained items.
| // A3-1: Drain ghost dispatch queue on SIMA disable (Build 960 audit fix) | ||
| { | ||
| FleetDispatchRequest ignored; | ||
| while (_pendingFleetDispatches.TryDequeue(out ignored)) { } | ||
| Print("[SIMA] Dispatch queue cleared on shutdown."); | ||
| } |
There was a problem hiding this comment.
Use the same rollbacking drain on SIMA shutdown.
This block has the same leak as the new abort path in PumpFleetDispatch: queued requests are discarded after their reservation/barrier state was created, so disabling SIMA can carry ghost entries into the next lifecycle.
Proposed fix
- {
- FleetDispatchRequest ignored;
- while (_pendingFleetDispatches.TryDequeue(out ignored)) { }
- Print("[SIMA] Dispatch queue cleared on shutdown.");
- }
+ DrainPendingFleetDispatches("[SIMA] Dispatch queue cleared on shutdown.");Based on learnings, all fleet dispatches must use the _dispatchSyncPendingExpKeys barrier for SIMA Synchronicity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.SIMA.cs` around lines 829 - 834, The shutdown drain currently
just dequeues _pendingFleetDispatches and drops FleetDispatchRequest entries,
leaking their reservation/barrier state; instead, replicate the rollbacking
drain used in PumpFleetDispatch's abort path: while dequeuing each
FleetDispatchRequest from _pendingFleetDispatches, invoke the same
rollback/release logic that clears its reservation from
_dispatchSyncPendingExpKeys (the same code path or helper used by
PumpFleetDispatch to abort a request) before discarding it, then log the
shutdown message; reference PumpFleetDispatch, _pendingFleetDispatches, and
_dispatchSyncPendingExpKeys to locate and reuse the exact rollback behavior.
| // A1-2: Stamp REAPER grace window before cancel to suppress false desync during replace gap (Build 960 audit fix) | ||
| StampReaperMoveGrace(); |
There was a problem hiding this comment.
Use an account-scoped REAPER grace stamp here.
StampReaperMoveGrace() updates the legacy global timestamp, so one follower retarget can suppress REAPER checks for unrelated accounts. This needs the per-account grace path keyed by the follower's ExpKey(...) instead. As per coding guidelines: "REAPER fill-grace must be per-account via _accountFillGraceTicks[expKey], never a single global timestamp".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.Symmetry.cs` around lines 555 - 556, The call to
StampReaperMoveGrace() writes a global REAPER grace timestamp and must be
replaced with a per-account stamp: compute the follower's expKey via ExpKey(...)
and set _accountFillGraceTicks[expKey] to the appropriate grace value (instead
of touching the legacy global stamp). Update the code that currently calls
StampReaperMoveGrace() to use ExpKey(...) and assign into the
_accountFillGraceTicks dictionary/array so REAPER fill-grace is scoped to that
account only.
|
Closing this PR to trigger a clean bot audit sweep on the new implementation. Superseded by new PR #31 |
Summary
Full remediation of all 9 defects identified by the Codex + Gemini 3.1 Pro Omni-Audit (phase_2_mission_brief.md). Standards reference: .agent/standards_manifesto.md Sections 1, 2, 6, 7, 8.
Commit 1 -- Concurrency (A1-1 + A2-1)
activePositions[entryName]andentryOrders[entryName]writes wrapped inlock(stateLock)across FFMA, MOMO, OR, RMA, Trend, Trailing, Orders.Management, Symmetryreturnon null pathpos.EntryFilled = truein Symmetry moved inside existinglock(stateLock)blockCommit 2 -- FSM (A1-2 + A1-3)
StampReaperMoveGrace()stamped before cancel to suppress false desync during the cancel-resubmit gap (mirrors PropagateMasterTargetMove pattern)_followerReplaceSpecsdict write wrapped inlock(stateLock)in SymmetryPendingQty/PendingPrice/state transition captured atomically underlock(stateLock)before TriggerCustomEventCommit 3 -- Null safety & deferred cleanup (A2-1 tail + A2-2 + A2-3)
PositionInfoextended withPendingCleanup(bool) andFlattenAttemptCount(int) fieldsRequestStopCancelLifecycleSafe: immediateactivePositions.TryRemovereplaced withpos.PendingCleanup = true; actual purge deferred to broker-confirmed terminal state in bothHandleOrderCancelled(master) andOnAccountOrderUpdate(follower)SymmetryGuardCascadeFollowerCleanup: prematureDeltaExpectedPositionLockedremoved; rollback moved toOnAccountOrderUpdateconfirmed-cancel handler (direction-aware signed delta)Commit 4 -- Queue/thread architecture (A3-1 + A3-2 + A3-3)
PumpFleetDispatch: hard guard at top drains_pendingFleetDispatchesand returns if SIMA disabled or flatten runningApplySimaState(false): queue drained afterStopReaperAudit()+UnsubscribeFromFleetAccounts()ExecuteReaperRepair:isFlattenRunningguard as very first statement_repairInFlight.Add(repairKey)moved beforeTriggerCustomEventto block double-enqueue across audit cyclesUpdateStopOrdernull-stop + catch paths:FlattenAttemptCountcircuit breaker caps consecutive emergency flattens at 3; counter reset on successful stop submissionVerification
deploy-sync.ps1-- PASS (zero non-ASCII byte warnings)activePositions[andentryOrders[writes insidelock(stateLock)isFlattenRunningis first statement inExecuteReaperRepair_pendingFleetDispatchescleared on SIMA disable and on PumpFleetDispatch abort🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores