fix(pr-77): apply Copilot/Codex review corrections with Protocol de Blindagem
Applies all 9 critical issues identified by Copilot/Codex reviewers on PR #77. All fixes follow Protocol de Blindagem methodology: - Boundary Analysis - Invariant Verification - Data Flow Tracking - Edge Mitigation - Semantic Distrust ## CRITICAL FIXES ### 1. Auto-Reconnect Implementation BROKEN (socket.ts:1369-1409) **Issue:** Using `await end()` + `await connect()` breaks recovery. **Root Cause:** - `end()` sets `closed = true` permanently - `connect()` function does not exist in makeSocket() return - Pattern: consumers call `makeWASocket()` to recreate socket **Fix:** - Removed internal reconnect logic - Emit `connection.update` with `isSessionError: true` flag - Consumer detects and recreates socket with makeWASocket() - Added `isSessionError` to ConnectionState type (State.ts) **Invariants Verified:** ✅ Socket cannot reconnect itself (must be recreated) ✅ Consumer pattern: makeWASocket() on close event ✅ Example.ts shows correct pattern (line 84) ### 2. Memory Leak - PreKey Auto-Sync Listener (socket.ts:774-787) **Issue:** Event listener never removed, memory leak on repeated socket creation. **Fix:** - Store listener reference in `connectionHandler` - Return cleanup function from `startPreKeyAutoSync()` - Call `cleanupPreKeyAutoSync()` in `end()` function - Cleanup both listener AND timer **Invariants Verified:** ✅ Listener removed via `ev.off()` ✅ Timer cleared via `clearTimeout()` ✅ Called in end() before ws listeners removed ### 3. Memory Leak - Session TTL Listener (socket.ts:813-848) **Issue:** Event listener never removed, memory leak on repeated socket creation. **Fix:** - Store listener reference in `connectionHandler` - Return cleanup function from `startSessionTTL()` - Call `cleanupSessionTTL()` in `end()` function - Cleanup listener AND both timers (ttl + grace) **Invariants Verified:** ✅ Listener removed via `ev.off()` ✅ Both timers cleared (ttlTimer + ttlGraceTimer) ✅ Called in end() after transaction cleanup ### 4. Race Condition - TTL Grace Timeout (socket.ts:831-834) **Issue:** Nested grace period timeout never cleared, fires after close. **Fix:** - Added `ttlGraceTimer` variable - Store timeout reference - Clear in close handler AND cleanup function - Prevents `end()` call on already-closed socket **Invariants Verified:** ✅ Grace timer cleared on disconnect ✅ No orphan timeouts ✅ Double-cleanup safe (idempotent) ### 5. Timer Accumulation in syncLoop (socket.ts:768-773) **Issue:** Recursive setTimeout could accumulate if completion happens after close. **Fix:** - Check `!closed && ws.isOpen` BEFORE rescheduling - Only schedule next sync if connection still open - Prevents unbounded timer growth **Invariants Verified:** ✅ Max 1 timer at a time ✅ No scheduling after close ✅ Cleanup always happens ### 6. TypeScript Compilation Error - Missing Event (Events.ts) **Issue:** 'session.ttl-expired' not in BaileysEventMap. **Fix:** - Added event to BaileysEventMap (Events.ts:162-167) - Full JSDoc documentation - Type-safe event payload **Invariants Verified:** ✅ TypeScript compilation succeeds ✅ Type-safe ev.emit() and ev.on() ### 7. Unbounded Queue - event-buffer.ts (Line 423-451) **Issue:** If import() fails, metricsQueue grows unbounded. **Fix:** - Added `metricsImportFailed` flag - Added `MAX_METRICS_QUEUE_SIZE = 1000` cap - Clear queue on import failure - Check both conditions before push **Invariants Verified:** ✅ Queue never exceeds 1000 items ✅ Queue cleared on import failure ✅ Silently drops metrics (acceptable for observability) ✅ Applied to ALL 7 metric call sites ### 8. Empty Callback - lid-mapping.ts (Line 984-986) **Issue:** Buffered callback empty, does nothing when module loads. **Fix:** - Removed metrics buffering entirely - Changed to no-op with comment - Actual metrics implementation pending **Invariants Verified:** ✅ No memory leak from queue growth ✅ No false promises (code matches reality) ✅ Clear TODO for future implementation ### 9. Renumbered Protections (socket.ts comments) **Fix:** - PreKey Auto-Sync: PROTECTION 1-6 (sequential) - Session TTL: PROTECTION 1-5 (sequential) - Documentation matches implementation ## 🛡️ VERIFICAÇÕES DE ROBUSTEZ ### Boundary Analysis Applied: ✅ Verified `end()` sets `closed = true` (socket.ts:927) ✅ Verified `connect()` does NOT exist in return type ✅ Verified makeWASocket() pattern in Example.ts ✅ Verified ConnectionState type structure (State.ts:17-49) ✅ Verified import() failure path in event-buffer.ts ### Invariant Verification: ✅ Socket lifecycle: create → use → end → recreate (not reconnect) ✅ Event listeners: added → used → removed in cleanup ✅ Timers: created → referenced → cleared on cleanup ✅ Queue bounds: capped at 1000, cleared on failure ✅ Cleanup idempotence: safe to call multiple times ### Data Flow Tracking: ✅ end() → cleanupPreKeyAutoSync() → ev.off() → listener removed ✅ end() → cleanupSessionTTL() → ev.off() + clearTimeout() → cleanup ✅ import() fail → metricsImportFailed = true → queue stops growing ✅ session error → emit close → consumer creates new socket ### Edge Mitigation: ✅ TTL expires during message send: 5s grace period >> message time ✅ Connection closes during sync: checked before reschedule ✅ Import fails: queue capped and cleared ✅ Multiple end() calls: guards prevent double cleanup ### Semantic Distrust: ✅ Did NOT assume connect() exists (verified absence) ✅ Did NOT trust empty callback would work (removed) ✅ Did NOT assume cleanup happens automatically (explicit) ✅ Did NOT trust queue would self-limit (added cap) ## FILES MODIFIED - src/Socket/socket.ts (auto-reconnect fix, listener cleanup, timer fixes) - src/Types/Events.ts (session.ttl-expired event) - src/Types/State.ts (isSessionError flag) - src/Utils/event-buffer.ts (unbounded queue fix) - src/Signal/lid-mapping.ts (empty callback removal) ## ZERO BREAKING CHANGES ✅ No impact on message delivery ✅ No impact on connection stability ✅ No impact on interactive messages ✅ Type-safe (TypeScript compiles) ✅ Consumer pattern unchanged https://claude.ai/code/session_33db9e93-e4c3-4859-9ff3-96d8864af1c4
This commit is contained in:
@@ -421,6 +421,8 @@ export const makeEventBuffer = (
|
||||
// Metrics integration (lazy loaded to avoid circular deps)
|
||||
let metricsModule: typeof import('./prometheus-metrics') | null = null
|
||||
let metricsQueue: Array<() => void> = []
|
||||
let metricsImportFailed = false
|
||||
const MAX_METRICS_QUEUE_SIZE = 1000 // Cap to prevent unbounded growth
|
||||
|
||||
if (config.enableMetrics) {
|
||||
import('./prometheus-metrics').then(m => {
|
||||
@@ -431,6 +433,9 @@ export const makeEventBuffer = (
|
||||
metricsQueue = []
|
||||
}).catch(() => {
|
||||
logger.debug('Prometheus metrics not available for event buffer')
|
||||
metricsImportFailed = true
|
||||
// Clear queue to prevent memory leak
|
||||
metricsQueue = []
|
||||
})
|
||||
}
|
||||
|
||||
@@ -438,17 +443,17 @@ export const makeEventBuffer = (
|
||||
const recordMetrics = (eventType: string, count: number) => {
|
||||
if (metricsModule) {
|
||||
metricsModule.recordEventBuffered(eventType, count)
|
||||
} else {
|
||||
// Buffer metric call until module loads
|
||||
} else if (!metricsImportFailed && metricsQueue.length < MAX_METRICS_QUEUE_SIZE) {
|
||||
// Buffer metric call until module loads (with size limit)
|
||||
metricsQueue.push(() => metricsModule?.recordEventBuffered(eventType, count))
|
||||
}
|
||||
// If import failed or queue is full, silently drop metric
|
||||
}
|
||||
|
||||
const recordFlushMetrics = (eventCount: number, forced: boolean, cacheSize: number) => {
|
||||
if (metricsModule) {
|
||||
metricsModule.recordBufferFlush(eventCount, forced, cacheSize)
|
||||
} else {
|
||||
// Buffer metric call until module loads
|
||||
} else if (!metricsImportFailed && metricsQueue.length < MAX_METRICS_QUEUE_SIZE) {
|
||||
metricsQueue.push(() => metricsModule?.recordBufferFlush(eventCount, forced, cacheSize))
|
||||
}
|
||||
}
|
||||
@@ -489,7 +494,7 @@ export const makeEventBuffer = (
|
||||
// Record overflow metric
|
||||
if (metricsModule) {
|
||||
metricsModule.recordBufferOverflow()
|
||||
} else {
|
||||
} else if (!metricsImportFailed && metricsQueue.length < MAX_METRICS_QUEUE_SIZE) {
|
||||
metricsQueue.push(() => metricsModule?.recordBufferOverflow())
|
||||
}
|
||||
flush(true)
|
||||
@@ -528,7 +533,7 @@ export const makeEventBuffer = (
|
||||
// Record metrics for cache cleanup
|
||||
if (metricsModule) {
|
||||
metricsModule.recordCacheCleanup(removed.length)
|
||||
} else {
|
||||
} else if (!metricsImportFailed && metricsQueue.length < MAX_METRICS_QUEUE_SIZE) {
|
||||
metricsQueue.push(() => metricsModule?.recordCacheCleanup(removed.length))
|
||||
}
|
||||
}
|
||||
@@ -608,7 +613,7 @@ export const makeEventBuffer = (
|
||||
if (config.enableAdaptiveTimeout) {
|
||||
if (metricsModule) {
|
||||
metricsModule.updateAdaptiveMetrics(adaptiveTimeout.getEventRate(), adaptiveTimeout.isHealthy())
|
||||
} else {
|
||||
} else if (!metricsImportFailed && metricsQueue.length < MAX_METRICS_QUEUE_SIZE) {
|
||||
metricsQueue.push(() => metricsModule?.updateAdaptiveMetrics(adaptiveTimeout.getEventRate(), adaptiveTimeout.isHealthy()))
|
||||
}
|
||||
}
|
||||
@@ -666,7 +671,7 @@ export const makeEventBuffer = (
|
||||
// Record final flush metric
|
||||
if (metricsModule) {
|
||||
metricsModule.recordBufferFinalFlush()
|
||||
} else {
|
||||
} else if (!metricsImportFailed && metricsQueue.length < MAX_METRICS_QUEUE_SIZE) {
|
||||
metricsQueue.push(() => metricsModule?.recordBufferFinalFlush())
|
||||
}
|
||||
}
|
||||
@@ -684,7 +689,7 @@ export const makeEventBuffer = (
|
||||
// Record buffer destroyed metric
|
||||
if (metricsModule) {
|
||||
metricsModule.recordBufferDestroyed('normal', hadPendingFlush)
|
||||
} else {
|
||||
} else if (!metricsImportFailed && metricsQueue.length < MAX_METRICS_QUEUE_SIZE) {
|
||||
metricsQueue.push(() => metricsModule?.recordBufferDestroyed('normal', hadPendingFlush))
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user