From 59f36edd930ed19cff322e711d88d17a1356f773 Mon Sep 17 00:00:00 2001 From: Nuwan Date: Tue, 24 Feb 2026 22:44:50 +0530 Subject: [PATCH] docs(25): research memory leak audit domain Phase 25: Memory Leak Audit - Identified 4 memory leak issues in recording code - Documented cleanup patterns (useRef, ignore flag, timer cleanup) - Created verification method following Phase 23 approach - Confidence: HIGH based on React docs and codebase analysis Co-Authored-By: Claude Opus 4.5 --- .../25-memory-leak-audit/25-RESEARCH.md | 399 ++++++++++++++++++ 1 file changed, 399 insertions(+) create mode 100644 .planning/phases/25-memory-leak-audit/25-RESEARCH.md diff --git a/.planning/phases/25-memory-leak-audit/25-RESEARCH.md b/.planning/phases/25-memory-leak-audit/25-RESEARCH.md new file mode 100644 index 000000000..da00a0b55 --- /dev/null +++ b/.planning/phases/25-memory-leak-audit/25-RESEARCH.md @@ -0,0 +1,399 @@ +# Phase 25: Memory Leak Audit - Research + +**Researched:** 2026-02-24 +**Domain:** React memory leak patterns, useEffect cleanup, Chrome DevTools memory profiling +**Confidence:** HIGH + +## Summary + +This research identifies memory leak risks in the recording-related code of jam-ui and documents the standard patterns for fixing them. The audit scope includes `useRecordingHelpers.js`, `JKSessionRecordingModal.js`, related Redux state in `sessionUISlice.js`, `fakeJamClientRecordings.js`, and global `window.JK.*` callback registrations. + +The primary findings reveal: +1. **Missing useEffect cleanup functions** in `JKSessionRecordingModal.js` - both useEffect hooks lack cleanup returns +2. **Timer without cleanup on unmount** in `useRecordingHelpers.js` - the `waitingOnStopTimer` has internal cleanup but no useEffect cleanup return +3. **Global callback registration without cleanup** in `useRecordingHelpers.js` - `window.JK.*` callbacks are registered but never cleaned up + +These patterns are well-documented memory leak sources in React applications. The established fix pattern follows Phase 22's approach: use `useRef` for cleanup stability and always return cleanup functions from useEffect. + +**Primary recommendation:** Add cleanup functions to all recording-related useEffect hooks, following the established `useRef` pattern from Phase 22. + +## Standard Stack + +This phase does not introduce new libraries. The work uses existing React patterns and Chrome DevTools. + +### Core Tools +| Tool | Purpose | Why Standard | +|------|---------|--------------| +| Chrome DevTools Memory Panel | Heap snapshot analysis | Official Chrome tool for memory profiling | +| Chrome Performance Monitor | Real-time heap size tracking | Built into Chrome DevTools | +| React DevTools | Component state inspection | Official React debugging extension | + +### Patterns Used +| Pattern | Purpose | When to Use | +|---------|---------|-------------| +| useRef for cleanup | Store cleanup targets with stable reference | When cleanup function needs current values | +| AbortController | Cancel async fetch operations | When useEffect performs data fetching | +| ignore flag | Prevent state updates after unmount | When async operation may complete after unmount | + +## Architecture Patterns + +### Recommended useEffect Cleanup Pattern + +Based on Phase 22's established approach and React official documentation: + +```javascript +// Pattern: useRef for cleanup stability (from Phase 22) +const resourceRef = useRef(null); + +useEffect(() => { + // Setup + resourceRef.current = someResource; + + // Cleanup function returned + return () => { + if (resourceRef.current) { + resourceRef.current.cleanup(); + resourceRef.current = null; + } + }; +}, [dependencies]); +``` + +### Pattern 1: Timer Cleanup + +**What:** Clear timers when component unmounts or dependencies change +**When to use:** Any useEffect that calls setTimeout or setInterval +**Example:** +```javascript +// Source: React official docs - https://react.dev/learn/synchronizing-with-effects +useEffect(() => { + const timerId = setTimeout(onTick, 5000); + return () => clearTimeout(timerId); +}, [dependencies]); +``` + +### Pattern 2: Global Callback Cleanup + +**What:** Remove globally registered callbacks when component unmounts +**When to use:** When registering callbacks on window, document, or global objects +**Example:** +```javascript +// Pattern for window.JK.* callbacks +const callbackRef = useRef(null); + +useEffect(() => { + callbackRef.current = handleCallback; + window.JK = window.JK || {}; + window.JK.HandleSomething = callbackRef.current; + + return () => { + // Don't delete if another instance registered a different callback + if (window.JK?.HandleSomething === callbackRef.current) { + delete window.JK.HandleSomething; + } + }; +}, [handleCallback]); +``` + +### Pattern 3: Async Operation Cleanup with ignore Flag + +**What:** Prevent state updates after component unmounts +**When to use:** Any useEffect with async operations that call setState +**Example:** +```javascript +// Source: React official docs +useEffect(() => { + let ignore = false; + + async function fetchData() { + const result = await someAsyncOperation(); + if (!ignore) { + setState(result); + } + } + + fetchData(); + + return () => { + ignore = true; + }; +}, [dependencies]); +``` + +### Anti-Patterns to Avoid + +- **Missing cleanup return:** useEffect hooks that set up resources but don't return cleanup functions +- **Stale closure in cleanup:** Using state directly in cleanup function instead of refs +- **Conditional cleanup:** Returning cleanup only in some code paths (must always return if needed) + +## Don't Hand-Roll + +| Problem | Don't Build | Use Instead | Why | +|---------|-------------|-------------|-----| +| Async cancellation | Manual flag tracking | AbortController | Built-in browser API, works with fetch | +| Stable callback refs | Manual closure management | useRef + useCallback | React-optimized approach | +| Memory profiling | Custom tracking code | Chrome DevTools | Comprehensive, accurate, zero overhead | + +**Key insight:** React's useEffect cleanup pattern handles most memory leak scenarios. The Phase 22 useRef pattern specifically addresses stale closure issues that can cause cleanup to fail silently. + +## Common Pitfalls + +### Pitfall 1: Missing Cleanup Returns + +**What goes wrong:** useEffect sets up timers, listeners, or callbacks but doesn't return a cleanup function +**Why it happens:** Developer focuses on setup logic and forgets cleanup is needed +**How to avoid:** Treat every useEffect with side effects as requiring a cleanup return +**Warning signs:** Console warnings about "Can't perform state update on unmounted component" + +**Found in audit:** +- `JKSessionRecordingModal.js` line 54-71: useEffect fetches audio preference but has no cleanup +- `JKSessionRecordingModal.js` line 74-78: useEffect loads video sources but has no cleanup + +### Pitfall 2: Global Callbacks Without Cleanup + +**What goes wrong:** Callbacks registered on window/global objects persist after component unmounts +**Why it happens:** Callbacks seem "harmless" since they're just function references +**How to avoid:** Always clean up global registrations; use refs to track what was registered +**Warning signs:** Multiple callback instances running, callbacks referencing stale state + +**Found in audit:** +- `useRecordingHelpers.js` line 459-468: Registers 5 `window.JK.*` callbacks without cleanup + +### Pitfall 3: Timer Not Cleared on Unmount + +**What goes wrong:** setTimeout/setInterval fires after component unmounts, causing state update on unmounted component +**Why it happens:** Timer is cleared in internal logic but not in useEffect cleanup +**How to avoid:** Return cleanup function that clears any active timers +**Warning signs:** Console errors about state updates on unmounted components + +**Found in audit:** +- `useRecordingHelpers.js` line 186: `waitingOnStopTimer` has internal cleanup but no useEffect unmount cleanup + +### Pitfall 4: Async State Updates After Unmount + +**What goes wrong:** Async operations (API calls) complete and call setState after component unmounts +**Why it happens:** No mechanism to cancel or ignore outdated async results +**How to avoid:** Use ignore flag pattern or AbortController +**Warning signs:** Console warnings about state updates on unmounted components + +**Found in audit:** +- `JKSessionRecordingModal.js` async calls to `jamClient.GetAudioRecordingPreference()` and `loadAvailableVideoSources()` may complete after modal closes + +## Code Examples + +### Example 1: Proper Timer Cleanup in useRecordingHelpers + +```javascript +// Current problem (line 31 and 186): +const waitingOnStopTimer = useRef(null); +// Timer is set internally but no useEffect cleanup + +// Solution: Add cleanup useEffect +useEffect(() => { + return () => { + if (waitingOnStopTimer.current) { + clearTimeout(waitingOnStopTimer.current); + waitingOnStopTimer.current = null; + } + }; +}, []); // Empty deps - cleanup only on unmount +``` + +### Example 2: Global Callback Registration with Cleanup + +```javascript +// Current problem (line 459-468): +useEffect(() => { + if (window) { + window.JK = window.JK || {}; + window.JK.HandleRecordingStartResult = handleRecordingStartResult; + // ... more callbacks + } + // NO CLEANUP RETURNED +}, [handleRecordingStartResult, ...]); + +// Solution: Store refs and clean up conditionally +const callbackRefs = useRef({}); + +useEffect(() => { + // Store current callback references + callbackRefs.current = { + HandleRecordingStartResult: handleRecordingStartResult, + HandleRecordingStopResult: handleRecordingStopResult, + HandleRecordingStopped: handleRecordingStopped, + HandleRecordingStarted: handleRecordingStarted, + HandleRecordingAborted: handleRecordingAborted, + }; + + window.JK = window.JK || {}; + Object.assign(window.JK, callbackRefs.current); + + return () => { + // Only delete if we still own the callback + Object.keys(callbackRefs.current).forEach(key => { + if (window.JK?.[key] === callbackRefs.current[key]) { + delete window.JK[key]; + } + }); + }; +}, [handleRecordingStartResult, handleRecordingStopResult, handleRecordingStopped, handleRecordingStarted, handleRecordingAborted]); +``` + +### Example 3: Async Cleanup in Modal + +```javascript +// Current problem (JKSessionRecordingModal.js line 54-71): +useEffect(() => { + const fetchAudioStoreType = async () => { + const pref = await jamClient.GetAudioRecordingPreference(); + setAudioStoreType(/* ... */); // May run after unmount + }; + fetchAudioStoreType(); +}, [jamClient]); + +// Solution: Add ignore flag +useEffect(() => { + let ignore = false; + + const fetchAudioStoreType = async () => { + try { + if (jamClient) { + const pref = await jamClient.GetAudioRecordingPreference(); + if (!ignore) { + // Safe to update state + if (AUDIO_STORE_TYPE_MIX_AND_STEMS['backendValues'].includes(pref)) { + setAudioStoreType(AUDIO_STORE_TYPE_MIX_AND_STEMS['key']); + } else if (AUDIO_STORE_TYPE_MIX_ONLY['backendValues'].includes(pref)) { + setAudioStoreType(AUDIO_STORE_TYPE_MIX_ONLY['key']); + } + } + } + } catch (error) { + if (!ignore) { + console.error('Error fetching audio store type preference:', error); + } + } + }; + + fetchAudioStoreType(); + + return () => { + ignore = true; + }; +}, [jamClient]); +``` + +## Audit Findings Summary + +### Files in Scope + +| File | Issues Found | Severity | +|------|--------------|----------| +| `useRecordingHelpers.js` | 2 (timer + callbacks) | HIGH | +| `JKSessionRecordingModal.js` | 2 (async + missing cleanup) | MEDIUM | +| `sessionUISlice.js` | 0 | N/A (Redux, no hooks) | +| `fakeJamClientRecordings.js` | 1 (timers in class) | LOW (test only) | + +### Issue Details + +1. **useRecordingHelpers.js - Timer Cleanup (HIGH)** + - Line 31: `waitingOnStopTimer` useRef + - Line 186: Timer set but no useEffect cleanup on unmount + - Risk: Timer fires after hook unmounts, causes stale state updates + +2. **useRecordingHelpers.js - Global Callbacks (HIGH)** + - Lines 459-468: 5 window.JK.* callbacks registered + - No cleanup function returned + - Risk: Callbacks accumulate on repeated mount/unmount cycles + +3. **JKSessionRecordingModal.js - Async Without Ignore (MEDIUM)** + - Lines 54-71: Async fetch without ignore flag + - Lines 74-78: Video source loading without cleanup + - Risk: State updates after modal closes + +4. **fakeJamClientRecordings.js - Test Timers (LOW)** + - Lines 45, 78: Timers for test simulation + - Lines 199, 205: Timers cleared internally + - Risk: Minimal (test code only), but could affect test reliability + +## State of the Art + +| Old Approach | Current Approach | When Changed | Impact | +|--------------|------------------|--------------|--------| +| No cleanup, rely on GC | Always return cleanup functions | React 16.8 (hooks) | Prevents memory leaks | +| State in cleanup closures | useRef for cleanup stability | Established pattern | Avoids stale closure bugs | +| Manual async tracking | AbortController | Widespread 2020+ | Native browser support | + +**Current best practice:** React Strict Mode (development) remounts components to verify cleanup works correctly. Code should pass this test without console warnings. + +## Open Questions + +1. **Multiple hook instances sharing window.JK callbacks** + - What we know: Both `JKSessionScreen` and `JKSessionRecordingModal` call `useRecordingHelpers` + - What's unclear: Whether deleting callbacks on modal unmount affects screen + - Recommendation: Use conditional cleanup that only removes if callback reference matches + +2. **FakeJamClientRecordings test reliability** + - What we know: Timer-based test simulation could be affected by cleanup + - What's unclear: Whether tests depend on timer persistence + - Recommendation: Low priority - address if test failures occur after fixes + +## Memory Verification Method + +Following Phase 23 UAT pattern, create checklist for manual verification: + +### Chrome DevTools Steps + +1. **Baseline Measurement** + - Open DevTools > Memory panel + - Take heap snapshot (Snapshot 1) + - Record JS Heap size in Performance Monitor + +2. **Test Scenario 1: Modal Idle** + - Open recording modal + - Leave open for 15+ minutes + - Take heap snapshot (Snapshot 2) + - Compare: Growth should be < 5MB + +3. **Test Scenario 2: Active Recording** + - Start recording + - Record for 15+ minutes + - Stop recording + - Take heap snapshot (Snapshot 3) + - Compare: No unbounded growth + +4. **Test Scenario 3: Repeated Open/Close** + - Open/close recording modal 20 times + - Take heap snapshot + - Look for: Retained closures, detached DOM nodes + +### Success Metrics + +- Zero "state update on unmounted component" warnings +- Heap growth < 50% during 15-minute test +- No detached DOM nodes from modal +- Callbacks not accumulating in window.JK + +## Sources + +### Primary (HIGH confidence) +- React Official Documentation: https://react.dev/learn/synchronizing-with-effects#step-3-add-cleanup-if-needed - useEffect cleanup patterns +- Chrome DevTools Documentation: https://developer.chrome.com/docs/devtools/memory-problems/heap-snapshots - Memory profiling + +### Secondary (MEDIUM confidence) +- Phase 22 Summary: `.planning/phases/22-session-screen-fixes/22-01-SUMMARY.md` - useRef cleanup pattern +- Phase 23 UAT: `.planning/phases/23-verification/23-UAT.md` - Memory verification checklist template + +### Tertiary (LOW confidence) +- LogRocket Blog: https://blog.logrocket.com/understanding-react-useeffect-cleanup-function/ - Cleanup patterns +- StackInsight Study: https://stackinsight.dev/blog/memory-leak-empirical-study - Memory leak statistics + +## Metadata + +**Confidence breakdown:** +- Cleanup patterns: HIGH - Verified against React official docs and established Phase 22 pattern +- Issue identification: HIGH - Direct codebase analysis with line numbers +- Verification method: HIGH - Based on Phase 23 proven approach + +**Research date:** 2026-02-24 +**Valid until:** 2026-03-24 (30 days - React patterns are stable)