Remove PLAN.md as it is no longer needed

2026-02-09 22:06:09 -05:00
parent 74670c00a6
commit edecfdb5bc
1 changed files with 0 additions and 154 deletions
@@ -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<boolean>
 - `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 |