From 1a5205c28978ea07b89401c3581551f1635f651d Mon Sep 17 00:00:00 2001
From: mindesbunister <github_service@egonetix.de>
Date: Tue, 2 Dec 2025 23:32:09 +0100
Subject: [PATCH] critical: Fix SL/TP exit P&L compounding with atomic
 deduplication
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CRITICAL BUG FIX: Stop loss and take profit exits were sending duplicate
Telegram notifications with compounding P&L (16 duplicates, 796x inflation).

Real Incident (Dec 2, 2025):
- Manual SOL-PERP SHORT position stopped out
- 16 duplicate Telegram notifications received
- P&L compounding: $0.23 → $12.10 → $24.21 → $183.12 (796× multiplication)
- All showed identical: entry $139.64, hold 4h 5-6m, exit reason SL
- First notification: Ghost detected (handled correctly)
- Next 15 notifications: SL exit (all duplicates with compounding P&L)

Root Cause:
- Multiple monitoring loops detect SL condition simultaneously
- All call executeExit() before any can remove position from tracking
- Race condition: check closingInProgress → both true → both proceed
- Database update happens BEFORE activeTrades.delete()
- Each execution sends Telegram notification
- P&L values compound across notifications

Solution:
Applied same atomic delete pattern as ghost detection fix (commit 93dd950):
- Move activeTrades.delete() to START of executeExit() (before any async operations)
- Check wasInMap return value (only true for first caller, false for duplicates)
- Early return if already deleted (atomic deduplication guard)
- Only first loop proceeds to close, save DB, send notification
- Removed redundant removeTrade() call (already deleted at start)

Impact:
- Prevents duplicate notifications for SL, TP1, TP2, emergency stops
- Ensures accurate P&L reporting (no compounding)
- Database receives correct single exit record
- User receives ONE notification per exit (as intended)

Code Changes:
- Line ~1520: Added atomic delete guard for full closes (percentToClose >= 100)
- Line ~1651: Removed redundant removeTrade() call
- Both changes prevent race condition at function entry

Scope:
- ✅ Stop loss exits: Fixed
- ✅ Take profit 2 exits: Fixed
- ✅ Emergency stops: Fixed
- ✅ Trailing stops: Fixed
- ℹ️ Take profit 1: Not affected (partial close keeps position in monitoring)

Related:
- Ghost detection fix: commit 93dd950 (Dec 2, 2025) - same pattern, different function
- Manual trade enhancement: commit 23277b7 (Dec 2, 2025) - unrelated feature
- P&L compounding series: Common Pitfalls #48-49, #59-61, #67 in docs
---
 lib/trading/position-manager.ts | 34 ++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/lib/trading/position-manager.ts b/lib/trading/position-manager.ts
index 13da6b0..3885a97 100644
--- a/lib/trading/position-manager.ts
+++ b/lib/trading/position-manager.ts
@@ -1517,6 +1517,17 @@ export class PositionManager {
    * 
    * Rate limit handling: If 429 error occurs, marks trade for retry
    * instead of removing it from monitoring (prevents orphaned positions)
+   * 
+   * CRITICAL FIX (Dec 2, 2025): Atomic deduplication at function entry
+   * Bug: Multiple monitoring loops detect SL/TP condition simultaneously
+   * - All call executeExit() before any can mark position as closing
+   * - Race condition in later removeTrade() call
+   * - Each execution sends Telegram notification
+   * - P&L values compound across notifications (16 duplicates, 796x inflation)
+   * Fix: Delete from activeTrades FIRST using atomic Map.delete()
+   * - Only first caller gets wasInMap=true, others get false and return
+   * - Prevents duplicate database updates, notifications, P&L compounding
+   * - Same pattern as ghost detection fix (handleExternalClosure)
    */
   private async executeExit(
     trade: ActiveTrade,
@@ -1524,6 +1535,22 @@ export class PositionManager {
     reason: ExitResult['reason'],
     currentPrice: number
   ): Promise<void> {
+    // CRITICAL FIX (Dec 2, 2025): Atomic deduplication for full closes
+    // For partial closes (TP1), we DON'T delete yet (position still monitored for TP2)
+    // For full closes (100%), delete FIRST to prevent duplicate execution
+    if (percentToClose >= 100) {
+      const tradeId = trade.id
+      const wasInMap = this.activeTrades.delete(tradeId)
+      
+      if (!wasInMap) {
+        console.log(`⚠️ DUPLICATE EXIT PREVENTED: ${tradeId} already processing ${reason}`)
+        console.log(`   This prevents duplicate Telegram notifications with compounding P&L`)
+        return
+      }
+      
+      console.log(`🗑️ Removed ${trade.symbol} from monitoring (${reason}) - atomic deduplication applied`)
+    }
+    
     try {
       console.log(`🔴 Executing ${reason} for ${trade.symbol} (${percentToClose}%)`)
 
@@ -1646,7 +1673,12 @@ export class PositionManager {
           }
         }
         
-        await this.removeTrade(trade.id)
+        // CRITICAL: Trade already removed from activeTrades at function start (atomic delete)
+        // No need to call removeTrade() again - just stop monitoring if empty
+        if (this.activeTrades.size === 0 && this.isMonitoring) {
+          this.stopMonitoring()
+        }
+        
         console.log(`✅ Position closed | P&L: $${trade.realizedPnL.toFixed(2)} | Reason: ${reason}`)
         
         // Send Telegram notification