diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 58a0ca7..1e16db1 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -3548,6 +3548,82 @@ trade.realizedPnL += actualRealizedPnL // NOT: result.realizedPnL from SDK - **Git commit:** b19f156 "critical: Fix Layer 2 ghost detection causing duplicate Telegram notifications" - **Lesson:** ALL code paths that detect external closures must check `closingInProgress` flag, not just the primary close verification path. Rate limit storms can cause monitoring loop to detect same closure dozens of times if flag isn't checked everywhere. +60. **Stale array snapshot in monitoring loop causes duplicate processing (CRITICAL - Fixed Nov 23, 2025):** + - **Symptom:** Manual closure sends duplicate "POSITION CLOSED" Telegram notifications with identical content + - **Root Cause:** Position Manager creates array snapshot before async processing, removed trades stay in array during iteration + - **Real incident (Nov 23, 07:05 CET):** + * Trade cmibdii4k0004pe07nzfmturo (SHORT SOL-PERP) + * Entry: $128.85, Exit: $128.79, P&L: +$6.44 (+0.05%) + * Hold time: 7 seconds + * Exit reason: MANUAL + * Size reduction: 97% ($12,940.98 → $388.95) + * Logs showed "Manual closure recorded" TWICE + * Two identical Telegram notifications sent + - **Bug mechanism:** + ```typescript + // handlePriceUpdate (line 531) - BROKEN FLOW: + const tradesForSymbol = Array.from(this.activeTrades.values()) // Snapshot created + + for (const trade of tradesForSymbol) { // Loop over snapshot + await this.checkTradeConditions(trade, update.price) // Async call + // First iteration: Detects 97% reduction → handleManualClosure() + // handleManualClosure: Updates DB, sends Telegram, calls activeTrades.delete() + // Second iteration: Same trade object (stale reference) processed AGAIN + // No guard to check if still in map → duplicate DB update + Telegram + } + ``` + - **Why it happens:** + 1. `Array.from(activeTrades.values())` creates snapshot at start of loop + 2. First iteration detects size reduction, calls `handleManualClosure` + 3. `handleManualClosure` removes trade via `activeTrades.delete(trade.id)` + 4. But loop continues with original array that still contains removed trade + 5. Second iteration processes same trade object (stale reference) + 6. No check if trade still in monitoring → duplicate processing + - **Impact:** Every manual closure vulnerable to duplicate notifications if multiple trades in monitoring loop + - **Fix (lib/trading/position-manager.ts line 545):** + ```typescript + private async checkTradeConditions( + trade: ActiveTrade, + currentPrice: number + ): Promise { + // CRITICAL FIX (Nov 23, 2025): Check if trade still in monitoring + if (!this.activeTrades.has(trade.id)) { + console.log(`⏭️ Skipping ${trade.symbol} - already removed from monitoring`) + return + } + + // Continue with normal processing... + trade.lastPrice = currentPrice + trade.lastUpdateTime = Date.now() + trade.priceCheckCount++ + // ... rest of function + } + ``` + - **Behavior now:** + * First iteration: Processes trade, removes from map + * Second iteration: Guard checks map, trade not found, returns immediately + * Prevents duplicate database updates and Telegram notifications + - **Why Common Pitfall #59 didn't cover this:** + * Pitfall #59: Layer 2 ghost detection (failureCount > 20) during rate limit storms + * This bug (#60): Normal monitoring loop during manual closures + * Different code paths, different triggers, same pattern needed everywhere + - **Configuration discrepancy discovered same session:** + * User reported Trade #9 (quality 90, -$22.41) should've been blocked + * `.env` had `MIN_SIGNAL_QUALITY_SCORE=81` (outdated) + * Code raised threshold to 91 on Nov 21, but `.env` not updated + * Fixed: `sed -i 's/MIN_SIGNAL_QUALITY_SCORE=81/91/' .env` + * Container restart required for settings changes to propagate + - **Settings UI enhancement:** + * Added console warning: "⚠️ Container restart recommended" + * Changed comment from "immediate effect" to "temporary, may not persist" + * Future: Need auto-restart trigger or UI notification + - **Files changed:** + * `lib/trading/position-manager.ts` (guard added) + * `app/api/settings/route.ts` (restart warning added) + - **Git commit:** a7c5930 "critical: Fix duplicate Telegram notifications + settings UI restart requirement" + - **Deployed:** Nov 23, container rebuilt (71.8s), all services running + - **Lesson:** When async processing modifies collections during iteration, always guard against stale references. Array snapshots don't protect against this - need explicit membership checks. ALL monitoring code paths need duplicate prevention, not just error scenarios. + ## File Conventions - **API routes:** `app/api/[feature]/[action]/route.ts` (Next.js 15 App Router)