fix(pr-77): resolve critical race conditions and listener cleanup issues
This commit addresses ALL remaining critical issues from Copilot's review,
applying Protocol de Blindagem for comprehensive correctness.
## Critical Fixes
### 1. RACE CONDITION: PreKey Timer Post-Cleanup Rescheduling
**Problem**: Timer could reschedule AFTER cleanup
- Line 773: `if (!closed && ws.isOpen) { setTimeout(...) }`
- Between check and setTimeout, cleanup() could execute
- cleanup() clears syncTimer, but syncLoop() reschedules new orphan timer
- Orphan timer continues firing even after socket destruction
**Root Cause** (Protocolo de Blindagem - Verificação de Invariantes):
- Check-then-act pattern is NOT atomic in async JavaScript
- No flag to prevent post-cleanup rescheduling
**Solution**:
- Added `cleanedUp` flag set BEFORE removing listener
- Check `cleanedUp` in both syncLoop conditions (lines 755, 773)
- Prevents timer rescheduling after cleanup initiated
- Ensures invariant: "At most one timer active OR zero if cleaned up"
### 2. CRITICAL: Listener Cleanup Order Inversion
**Problem**: Handlers removed BEFORE receiving final close event
- Line 984-987: cleanupPreKeyAutoSync() and cleanupSessionTTL() called first
- These remove 'connection.update' listeners via ev.off()
- Line 1013: Final 'close' event emitted AFTER listeners removed
- Handlers never receive final close event for internal cleanup
**Root Cause** (Protocolo de Blindagem - Rastreamento de Fluxo):
- Cleanup functions called in wrong order
- Events must be emitted BEFORE unregistering handlers
**Solution**:
- MOVED ev.emit('connection.update', 'close') to line 1011 (BEFORE cleanups)
- MOVED cleanupPreKeyAutoSync() and cleanupSessionTTL() to line 1021 (AFTER emit)
- Now handlers receive close event and execute their internal cleanup
- Then we remove the listeners (proper teardown sequence)
### 3. CRITICAL: removeAllListeners Breaks Consumer Reconnection
**Problem**: Line 1021 had `ev.removeAllListeners('connection.update')`
- Removes ALL listeners, including consumer's reconnection handler
- Consumer's Example/example.ts relies on 'connection.update' for reconnect
- Breaking consumer listeners violates library contract
**Root Cause** (Protocolo de Blindagem - Análise de Fronteira):
- removeAllListeners affects ALL listeners, not just internal ones
- Violates separation between library internals and consumer code
**Solution**:
- REMOVED ev.removeAllListeners('connection.update') entirely
- Our listeners are cleaned up explicitly via cleanup functions
- Consumer listeners remain intact for proper reconnection logic
- Added comment explaining why NOT to use removeAllListeners
### 4. LOW: Unnecessary async in creds.update Handler
**Problem**: Handler declared as async but no await used
- Changes timing characteristics without benefit
- Copilot flagged as unnecessary modification
**Solution**:
- Removed async keyword from creds.update handler (line 1425)
- Maintains original synchronous timing behavior
- sendNode() errors still caught via .catch()
## Impact Assessment
**Zero Breaking Changes**:
✓ All fixes are internal timing/cleanup improvements
✓ No API surface changes
✓ No behavior changes visible to consumers
✓ Reconnection logic preserved and enhanced
**Correctness Improvements**:
✓ Eliminates timer leaks (PreKey orphan timers)
✓ Ensures handlers receive all lifecycle events
✓ Preserves consumer listener contracts
✓ Maintains proper cleanup sequencing
## Protocol de Blindagem Applied
✓ **Verificação de Invariantes**: Timer cleanup now enforces "at most one active"
✓ **Rastreamento de Fluxo**: Event emission sequenced before listener removal
✓ **Análise de Fronteira**: removeAllListeners removed to preserve consumer contract
✓ **Mitigação de Arestas**: cleanedUp flag prevents async race conditions
## Files Modified
- src/Socket/socket.ts: All fixes applied
https://claude.ai/code/session_VMxqX
This commit is contained in:
+17
-13
@@ -742,6 +742,7 @@ export const makeSocket = (config: SocketConfig) => {
|
||||
const SYNC_INTERVAL = 6 * 60 * 60 * 1000 // 6 hours
|
||||
let syncTimer: NodeJS.Timeout | undefined
|
||||
let isRunning = false
|
||||
let cleanedUp = false // PROTECTION: Prevent rescheduling after cleanup
|
||||
|
||||
const syncLoop = async () => {
|
||||
// PROTECTION 1: Prevent overlapping runs
|
||||
@@ -750,8 +751,8 @@ export const makeSocket = (config: SocketConfig) => {
|
||||
return
|
||||
}
|
||||
|
||||
// PROTECTION 2: Check connection state
|
||||
if (closed || !ws.isOpen) {
|
||||
// PROTECTION 2: Check connection state AND cleanup flag
|
||||
if (closed || !ws.isOpen || cleanedUp) {
|
||||
logger.debug('🔑 Connection closed, stopping PreKey sync')
|
||||
return
|
||||
}
|
||||
@@ -767,9 +768,9 @@ export const makeSocket = (config: SocketConfig) => {
|
||||
isRunning = false
|
||||
}
|
||||
|
||||
// PROTECTION 3: Prevent timer accumulation
|
||||
// Only reschedule if connection is still open
|
||||
if (!closed && ws.isOpen) {
|
||||
// PROTECTION 3: Prevent timer accumulation and post-cleanup rescheduling
|
||||
// Check cleanedUp flag atomically to prevent race condition
|
||||
if (!closed && !cleanedUp && ws.isOpen) {
|
||||
syncTimer = setTimeout(syncLoop, SYNC_INTERVAL)
|
||||
}
|
||||
}
|
||||
@@ -795,6 +796,7 @@ export const makeSocket = (config: SocketConfig) => {
|
||||
|
||||
// PROTECTION 6: Return cleanup function
|
||||
return () => {
|
||||
cleanedUp = true // Set flag FIRST to prevent race with syncLoop reschedule
|
||||
ev.off('connection.update', connectionHandler)
|
||||
if (syncTimer) {
|
||||
clearTimeout(syncTimer)
|
||||
@@ -980,12 +982,6 @@ export const makeSocket = (config: SocketConfig) => {
|
||||
// Clean up transaction capability (PreKeyManager + queues)
|
||||
keys.destroy?.()
|
||||
|
||||
// Clean up PreKey auto-sync event listener
|
||||
cleanupPreKeyAutoSync()
|
||||
|
||||
// Clean up Session TTL event listener
|
||||
cleanupSessionTTL()
|
||||
|
||||
ws.removeAllListeners('close')
|
||||
ws.removeAllListeners('open')
|
||||
ws.removeAllListeners('message')
|
||||
@@ -1010,6 +1006,8 @@ export const makeSocket = (config: SocketConfig) => {
|
||||
)
|
||||
}
|
||||
|
||||
// CRITICAL: Emit close event BEFORE cleaning up listeners
|
||||
// This allows handlers (PreKey auto-sync, Session TTL) to receive the final close event
|
||||
ev.emit('connection.update', {
|
||||
connection: 'close',
|
||||
lastDisconnect: {
|
||||
@@ -1018,7 +1016,13 @@ export const makeSocket = (config: SocketConfig) => {
|
||||
},
|
||||
isSessionError
|
||||
})
|
||||
ev.removeAllListeners('connection.update')
|
||||
|
||||
// NOW clean up our internal listeners (after they've received the close event)
|
||||
cleanupPreKeyAutoSync()
|
||||
cleanupSessionTTL()
|
||||
|
||||
// IMPORTANT: Do NOT use removeAllListeners('connection.update')
|
||||
// It would remove consumer listeners, breaking their reconnection logic
|
||||
}
|
||||
|
||||
const waitForSocketOpen = async () => {
|
||||
@@ -1418,7 +1422,7 @@ export const makeSocket = (config: SocketConfig) => {
|
||||
})
|
||||
|
||||
// update credentials when required
|
||||
ev.on('creds.update', async update => {
|
||||
ev.on('creds.update', update => {
|
||||
const name = update.me?.name
|
||||
// if name has just been received
|
||||
if (creds.me?.name !== name) {
|
||||
|
||||
Reference in New Issue
Block a user