|
| 1 | +# Cleanup Hook Failure Analysis |
| 2 | + |
| 3 | +## Issue Summary |
| 4 | + |
| 5 | +After implementing a cleanup useEffect hook to fix the worker teardown timeout, **ALL 5 builds (9-13) failed with 100% failure rate** due to breaking the scheduler persistence feature. |
| 6 | + |
| 7 | +## Test Results |
| 8 | + |
| 9 | +| Build # | Run ID | Result | Error | |
| 10 | +|---------|--------|--------|-------| |
| 11 | +| 9 | 19361655154 | ❌ FAILED | project-switch-scheduler-persist.spec.ts:181 - Scheduler button missing blue class | |
| 12 | +| 10 | 19361662368 | ❌ FAILED | project-switch-scheduler-persist.spec.ts:181 - Scheduler button missing blue class | |
| 13 | +| 11 | 19361666122 | ❌ FAILED | project-switch-scheduler-persist.spec.ts:181 - Scheduler button missing blue class | |
| 14 | +| 12 | 19361668770 | ❌ FAILED | project-switch-scheduler-persist.spec.ts:181 - Scheduler button missing blue class | |
| 15 | +| 13 | 19361671661 | ❌ FAILED | project-switch-scheduler-persist.spec.ts:181 - Scheduler button missing blue class | |
| 16 | + |
| 17 | +**Failure Pattern**: All 5 builds failed at the same test line: |
| 18 | +``` |
| 19 | +Error: expect(locator).toHaveClass failed |
| 20 | +Locator: locator('button[title="Schedule Command"]').first() |
| 21 | +Expected pattern: /text-blue-500/ |
| 22 | +Received string: "inline-flex items-center justify-center whitespace-nowrap rounded-md text-sm font-medium ring-offset-background transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-2 disabled:pointer-events-none disabled:opacity-50 hover:bg-accent hover:text-accent-foreground h-10 w-10" |
| 23 | +``` |
| 24 | + |
| 25 | +## Root Cause Analysis |
| 26 | + |
| 27 | +### The Problematic Fix (Commit bc7cd751) |
| 28 | + |
| 29 | +Added cleanup useEffect hook: |
| 30 | +```typescript |
| 31 | +useEffect(() => { |
| 32 | + return () => { |
| 33 | + const state = getSchedulerState(); |
| 34 | + if (state?.timeoutId) { |
| 35 | + clearTimeout(state?.timeoutId); |
| 36 | + } |
| 37 | + updateSchedulerState(state ? { ...state, isRunning: false } : null); |
| 38 | + }; |
| 39 | +}, [getSchedulerState, updateSchedulerState]); |
| 40 | +``` |
| 41 | + |
| 42 | +### Why It Fails |
| 43 | + |
| 44 | +1. **Scheduler persistence design**: The scheduler uses a cache (`schedulerStateCache`) that persists across project switches |
| 45 | +2. **Component lifecycle**: When switching projects, the `ClaudeTerminal` component unmounts |
| 46 | +3. **Cleanup runs prematurely**: The cleanup hook runs on unmount, stopping the scheduler |
| 47 | +4. **Test expectation broken**: Test expects scheduler to still be running after switching back to project 1 |
| 48 | + |
| 49 | +### The Conflict |
| 50 | + |
| 51 | +The fix was designed to: |
| 52 | +- ✅ Clean up pending promises during Playwright worker teardown |
| 53 | +- ❌ But also cleans up during normal project switches (unintended side effect) |
| 54 | + |
| 55 | +The scheduler feature was designed to: |
| 56 | +- ✅ Persist across project switches using cache |
| 57 | +- ❌ But now gets stopped by cleanup hook on every project switch |
| 58 | + |
| 59 | +## Solution |
| 60 | + |
| 61 | +The cleanup hook needs to distinguish between: |
| 62 | +1. **Normal project switch** - Scheduler should persist (do NOT cleanup) |
| 63 | +2. **Playwright worker teardown** - Scheduler should cleanup (do cleanup) |
| 64 | + |
| 65 | +### Implementation Options |
| 66 | + |
| 67 | +**Option 1: Remove cleanup hook, rely on Playwright's cleanup** |
| 68 | +- Let Playwright handle worker teardown naturally |
| 69 | +- Risk: May reintroduce worker teardown timeout (original issue) |
| 70 | + |
| 71 | +**Option 2: Only cleanup on window unload in test environment** |
| 72 | +- Add window.addEventListener('beforeunload', cleanup) only in test mode |
| 73 | +- Preserves scheduler persistence during normal operation |
| 74 | + |
| 75 | +**Option 3: Use a ref to track component mount state** |
| 76 | +- Don't cleanup if component is just re-mounting with same project |
| 77 | +- More complex but more precise |
| 78 | + |
| 79 | +**Option 4: Remove cleanup entirely and fix the root cause differently** |
| 80 | +- The original issue was floating promises preventing worker teardown |
| 81 | +- Perhaps the fix in commit 8cdbad54 (return scheduleNext()) is sufficient |
| 82 | +- The cleanup hook may be unnecessary if promise chains are properly managed |
| 83 | + |
| 84 | +## Recommended Approach |
| 85 | + |
| 86 | +**Remove the cleanup hook entirely** and verify that the original fix (commit 8cdbad54) alone is sufficient. The reasoning: |
| 87 | + |
| 88 | +1. Commit 8cdbad54 changed `void scheduleNext()` to `return scheduleNext()` on line 213 |
| 89 | +2. This ensures the promise chain is properly tracked |
| 90 | +3. Line 221 still uses `void scheduleNext()` to fire-and-forget the initial start |
| 91 | +4. When component unmounts, React should handle cleanup of any in-flight promises |
| 92 | +5. The cleanup hook was overly aggressive and broke the persistence feature |
| 93 | + |
| 94 | +If removing the cleanup hook causes worker teardown timeouts again, we'll need a more sophisticated approach. |
| 95 | + |
| 96 | +## Timeline |
| 97 | + |
| 98 | +- **Commit 7b1e53b6**: Introduced worker teardown timeout (100% failure) |
| 99 | +- **Commit 8cdbad54**: Partial fix - reduced to 50% failure rate |
| 100 | +- **Commit bc7cd751**: Added cleanup hook - 100% failure with different error |
| 101 | +- **Next**: Remove cleanup hook, test if original fix is sufficient |
| 102 | + |
| 103 | +## Related Files |
| 104 | + |
| 105 | +- `apps/desktop/src/renderer/components/ClaudeTerminal.tsx:244-256` (problematic cleanup hook) |
| 106 | +- `apps/desktop/e2e/project-switch-scheduler-persist.spec.ts:181` (failing test) |
| 107 | +- `WORKER_TEARDOWN_ANALYSIS.md` (previous failure analysis) |
0 commit comments