From edecfdb5bcc92b3149a4f3350df6878d6fb78d49 Mon Sep 17 00:00:00 2001 From: Alex Newman Date: Mon, 9 Feb 2026 22:06:09 -0500 Subject: [PATCH] Remove PLAN.md as it is no longer needed --- PLAN.md | 154 -------------------------------------------------------- 1 file changed, 154 deletions(-) delete mode 100644 PLAN.md diff --git a/PLAN.md b/PLAN.md deleted file mode 100644 index 0303f753..00000000 --- a/PLAN.md +++ /dev/null @@ -1,154 +0,0 @@ -# Plan: Address PR #856 Review Feedback - -## Summary of Review Feedback - -Multiple reviewers identified the same core issues: - -1. **Race Condition in Idle Detection** (Medium-High Priority) - - When timeout fires at 3:00 but last message was at 2:59, `idleDuration` is 1 second, check fails - - Need to either remove redundant check or reset `lastActivityTime` on timeout - -2. **Missing Test Coverage** (High Priority) - - No tests for SessionQueueProcessor timeout logic - - Critical fix for high-impact bug (79 processes, 13.4GB swap) - -3. **Minor: Optional Chaining** (Low Priority) - - Use `onIdleTimeout?.()` instead of `if (onIdleTimeout) { onIdleTimeout() }` - -4. **Minor: Logging Enhancement** (Low Priority) - - Add timeout threshold to log message for debugging - ---- - -## Phase 0: Documentation Discovery (COMPLETE) - -### Sources Consulted -- PR #856 comments from claude, greptile-apps reviewers -- `src/services/queue/SessionQueueProcessor.ts` (current implementation) - -### Allowed APIs -- `waitForMessage(signal, timeoutMs)` → Promise -- `logger.info('SESSION', ...)` for logging - -### The Fix Strategy - -The reviewers suggest two options: - -**Option A**: Remove redundant check since `waitForMessage` enforces timeout -```typescript -if (!receivedMessage && !signal.aborted) { - // Timeout occurred - exit gracefully - const idleDuration = Date.now() - lastActivityTime; - logger.info('SESSION', 'Exiting queue iterator due to idle timeout', { ... }); - onIdleTimeout?.(); - return; -} -``` - -**Option B**: Reset `lastActivityTime` on timeout to handle edge cases -```typescript -if (!receivedMessage && !signal.aborted) { - const idleDuration = Date.now() - lastActivityTime; - if (idleDuration >= IDLE_TIMEOUT_MS) { - logger.info('SESSION', 'Exiting...', { ... }); - onIdleTimeout?.(); - return; - } - // CRITICAL: Reset timer since we know queue is empty now - lastActivityTime = Date.now(); -} -``` - -**Decision**: Use Option B - it's defensive and handles spurious wakeups correctly. - ---- - -## Phase 1: Fix Race Condition in SessionQueueProcessor - -### What to Implement -Fix the idle timeout logic to reset `lastActivityTime` when timeout occurs but duration check fails. - -### Tasks -1. In `createIterator()` at lines 50-62, add `lastActivityTime = Date.now()` after the duration check fails -2. Use optional chaining for `onIdleTimeout?.()` -3. Add timeout threshold to log message - -### Pattern to Follow -```typescript -if (!receivedMessage && !signal.aborted) { - const idleDuration = Date.now() - lastActivityTime; - if (idleDuration >= IDLE_TIMEOUT_MS) { - logger.info('SESSION', 'Idle timeout reached, triggering abort to kill subprocess', { - sessionDbId, - idleDurationMs: idleDuration, - thresholdMs: IDLE_TIMEOUT_MS - }); - onIdleTimeout?.(); - return; - } - // Reset timer on spurious wakeup - queue is empty but duration check failed - lastActivityTime = Date.now(); -} -``` - -### Verification -```bash -npm run build -grep -A10 "idleDuration >= IDLE_TIMEOUT_MS" src/services/queue/SessionQueueProcessor.ts -``` - ---- - -## Phase 2: Add Unit Tests for SessionQueueProcessor - -### What to Implement -Create test file covering the idle timeout behavior. - -### Test Cases Required -1. Iterator exits after idle timeout when no messages arrive -2. `onIdleTimeout` callback is invoked on timeout -3. Message arrival resets the idle timer -4. Abort signal takes precedence over timeout -5. Event listener cleanup happens correctly - -### Location -`tests/services/queue/SessionQueueProcessor.test.ts` - -### Verification -```bash -npm run test -- SessionQueueProcessor -``` - ---- - -## Phase 3: Build and Verify - -### Tasks -1. Run `npm run build` - verify no TypeScript errors -2. Run tests to ensure timeout behavior works -3. Commit changes to fix/observer-idle-timeout branch -4. Push to update PR #856 - -### Verification -```bash -npm run build -npm run test -git diff --stat -``` - ---- - -## Phase 4: Update PR Description - -### Tasks -1. Update test plan checkboxes in PR description -2. Add note about race condition fix - ---- - -## Summary of Changes - -| File | Change | -|------|--------| -| `src/services/queue/SessionQueueProcessor.ts` | Fix race condition, optional chaining, enhanced logging | -| `tests/services/queue/SessionQueueProcessor.test.ts` | New test file for timeout behavior |