critical: Fix ghost detection P&L compounding - delete from Map BEFORE check
Bug: Multiple monitoring loops detect ghost simultaneously - Loop 1: has(tradeId) → true → proceeds - Loop 2: has(tradeId) → true → ALSO proceeds (race condition) - Both send Telegram notifications with compounding P&L Real incident (Dec 2, 2025): - Manual SHORT at $138.84 - 23 duplicate notifications - P&L compounded: -$47.96 → -$1,129.24 (23× accumulation) - Database shows single trade with final compounded value Fix: Map.delete() returns true if key existed, false if already removed - Call delete() FIRST - Check return value proceeds - All other loops get false → skip immediately - Atomic operation prevents race condition Pattern: This is variant of Common Pitfalls #48, #49, #59, #60, #61 - All had "check then delete" pattern - All vulnerable to async timing issues - Solution: "delete then check" pattern - Map.delete() is synchronous and atomic Files changed: - lib/trading/position-manager.ts lines 390-410 Related: DUPLICATE PREVENTED message was working but too late
This commit is contained in:
@@ -390,14 +390,23 @@ export class PositionManager {
|
||||
private async handleExternalClosure(trade: ActiveTrade, reason: string): Promise<void> {
|
||||
console.log(`🧹 Handling external closure: ${trade.symbol} (${reason})`)
|
||||
|
||||
// CRITICAL: Check if already processed to prevent duplicate notifications
|
||||
// CRITICAL FIX (Dec 2, 2025): Remove from activeTrades FIRST, then check if already removed
|
||||
// Bug: Multiple monitoring loops detect ghost simultaneously
|
||||
// - Loop 1 checks has(tradeId) → true → proceeds
|
||||
// - Loop 2 checks has(tradeId) → true → also proceeds (RACE CONDITION)
|
||||
// - Both send Telegram notifications with compounding P&L
|
||||
// Fix: Delete BEFORE check, so only first loop proceeds
|
||||
const tradeId = trade.id
|
||||
if (!this.activeTrades.has(tradeId)) {
|
||||
const wasInMap = this.activeTrades.delete(tradeId)
|
||||
|
||||
if (!wasInMap) {
|
||||
console.log(`⚠️ DUPLICATE PREVENTED: Trade ${tradeId} already processed, skipping`)
|
||||
console.log(` This prevents duplicate Telegram notifications with compounding P&L`)
|
||||
return
|
||||
}
|
||||
|
||||
console.log(`🗑️ Removed ${trade.symbol} from monitoring (will not process duplicates)`)
|
||||
|
||||
// CRITICAL: Calculate P&L using originalPositionSize for accuracy
|
||||
// currentSize may be stale if Drift propagation was interrupted
|
||||
const profitPercent = this.calculateProfitPercent(
|
||||
@@ -410,10 +419,6 @@ export class PositionManager {
|
||||
|
||||
console.log(`💰 Estimated P&L: ${profitPercent.toFixed(2)}% on $${sizeForPnL.toFixed(2)} → $${estimatedPnL.toFixed(2)}`)
|
||||
|
||||
// Remove from monitoring IMMEDIATELY to prevent race conditions
|
||||
this.activeTrades.delete(tradeId)
|
||||
console.log(`🗑️ Removed ${trade.symbol} from monitoring`)
|
||||
|
||||
// Update database
|
||||
try {
|
||||
const holdTimeSeconds = Math.floor((Date.now() - trade.entryTime) / 1000)
|
||||
|
||||
Reference in New Issue
Block a user