diff --git a/docs/performance/MEMORY_LEAK_REPORT.md b/docs/performance/MEMORY_LEAK_REPORT.md new file mode 100644 index 0000000..9f528de --- /dev/null +++ b/docs/performance/MEMORY_LEAK_REPORT.md @@ -0,0 +1,83 @@ +# Memory Leak Report — Job Tracker + +**Date:** 2026-07-05 +**Investigator role:** Senior Performance Engineer (memory/browser internals/full-stack) +**Verdict:** **No confirmed memory leak.** One *resource-release correctness* bug (over-eager blob-URL +revocation) was found and fixed; it is the opposite of a leak. See [ROOT_CAUSE_ANALYSIS.md](ROOT_CAUSE_ANALYSIS.md) +and [PERFORMANCE_IMPROVEMENTS.md](PERFORMANCE_IMPROVEMENTS.md). + +> Method & honesty note. The app is a data-driven SPA that renders only after the backend answers +> `/auth/config` + `/auth/me`; headless (no backend/DB) it sits on a "Loading…" screen, so live +> DevTools heap-snapshot/allocation-timeline profiling of populated screens was **not** performed in this +> environment. Evidence here is therefore **static code analysis of every known leak vector** plus the +> existing automated test suite. Where a runtime confirmation is still advisable, it is called out +> explicitly. Per the mission's Final Rule, nothing below is reported as a leak unless the code path +> actually retains memory — and none did. + +--- + +## Phase 1–2 — Does a leak exist? Can it be reproduced? + +No leak was reproduced or evidenced. The classic React/browser leak vectors were each checked in code and +found to have correct teardown. "Memory grows while using the app" (the usual trigger for this kind of +investigation) is explained by **expected behaviour** — MUI/emulator caches, route-level component state, +and delayed GC — not by retained graphs. There is no growing global collection, no unremoved listener, no +uncleared timer, and no real-time connection to leak. + +## Phase 3 / 3.5 — Vector-by-vector evidence + +| Vector | Finding | Evidence | Verdict | +|---|---|---|---| +| **Timers / intervals** | Both `setInterval`s clear on cleanup | `App.tsx:154-155` (reminders, 60s → `clearInterval`); `ProfilePage.tsx:319-323` (extraction poll, 4s → `clearInterval`) | ✅ no leak | +| **`setTimeout`** | Used only for one-shot object-URL revokes | `BackupCard.tsx:29`, `Attachments.tsx:193`, `ImportExportJobs.tsx:21` | ✅ no leak | +| **Event listeners** | Every `addEventListener` has a matching `removeEventListener` in the effect cleanup | `App.tsx:174-175` (auth-changed), `App.tsx:185-186` (keydown), `CropImageDialog.tsx:114-124` (mouse/touch drag ×4) | ✅ no leak | +| **Object URLs (media)** | Created URLs are revoked on cleanup/timeout | `CropImageDialog.tsx:59/65`, `Attachments.tsx:111/181/193/201`, `BackupCard.tsx:18/29`, `ImportExportJobs.tsx:16/21`, `JobDetailsDialog.tsx:507/514`, `ProfilePage.tsx` (see fix) | ✅ no leak (1 over-revoke bug fixed) | +| **Observers** | None used | grep: no `ResizeObserver` / `IntersectionObserver` / `MutationObserver` in `src/` | ✅ n/a | +| **WebSocket / SSE / SignalR** | None used | grep: no `new WebSocket` / `EventSource` / SignalR client anywhere | ✅ n/a | +| **Signal/event subscriptions** | Only the `window` `"auth-changed"` custom event; unsubscribed on cleanup | `App.tsx:157-176` | ✅ no leak | +| **Global/module state (client)** | No module-level mutable collection that grows unbounded | grep for module-scope `Map`/array caches — none accumulating | ✅ no leak | +| **Client caches (localStorage)** | Bounded keys (prefs, columns, saved views); no per-event append | `App.tsx`, `SettingsView.tsx`, `SavedViewsMenu.tsx`, `themePrefs.ts` | ✅ no leak | +| **React effects w/o cleanup** | All effects reviewed return cleanup where they acquire resources | see rows above | ✅ no leak | +| **Server static collections** | All `static` collections are **fixed lookup tables** or **method return types**, never growing fields | `AttachmentsController`, `AuthController`, `ProfileCvController`, `HumanLanguageCatalog`, `StructuredCvProfileJson` | ✅ no leak | +| **Server `IMemoryCache`** | Bounded: OAuth state entries expire in 15 min and are removed on consume | `GmailOAuthService.cs:72` (`TimeSpan.FromMinutes(15)`), `:133-138` (`TryGetValue`+`Remove`) | ✅ no leak | +| **AI service (Python) caches** | `cachetools.TTLCache` (bounded by TTL + maxsize) | `tools/summarizer/app.py:4` | ✅ no leak | +| **Server timers / background** | Hosted services use scoped DI + `PeriodicTimer`/delays; no accumulating handlers | `FollowUpReminderHostedService`, `RulesHostedService`, `JobEnrichmentHostedService`, etc. | ✅ no leak | + +## Phase 3.5 — Repeated/duplicate work audit + +- **Reminders poll** (`App.tsx:151`, every 60s): correct URL `/jobapplications/reminders`, cheap, cleaned + up. (An earlier read rendered the path with backslashes — a display artifact; the source uses forward + slashes. **No bug.**) +- **Extraction-run poll** (`ProfilePage.tsx:315-324`, every 4s): effect deps `[extractionRuns, loadProfile]` + and `extractionRuns` changes each poll, so the interval is torn down + recreated every 4s while a run is + active. **Not a leak** (cleanup runs); benign churn that self-terminates when runs finish. Minor — see + [PERFORMANCE_IMPROVEMENTS.md](PERFORMANCE_IMPROVEMENTS.md). +- No duplicate subscriptions, no retry storms, no infinite render loops observed. + +## Phase 4 — Root cause +No leak → no leak root cause. The single defect found is an *over-release* (revoking blob URLs still in +use), root-caused in [ROOT_CAUSE_ANALYSIS.md](ROOT_CAUSE_ANALYSIS.md). + +## Phase 5 — Fix +`fix(profile): revoke CV-preview blob URLs on unmount, not on every change` (commit `eed9b1f`). Smallest +change: track the carousel in a ref and revoke only on unmount. + +## Phase 6 — Verification +`profile-page.test.tsx` passes **5/5** with an adequate test timeout after the fix. The broader suite's +intermittent timeouts are a **pre-existing** flakiness of the heavy RTL suites (verified: they fail +identically on the clean tree; three of them don't touch `ProfilePage`). + +## Phase 7 — Regression audit +Swept all object-URL, timer, and listener sites (table above). No other instance of the over-revoke +pattern, and no missing-cleanup pattern, was found. + +## Remaining risks / recommendations +- Live heap-snapshot profiling on a **populated** session (real backend) is still worth doing once, to + confirm the static conclusion under real navigation — see [PERFORMANCE_IMPROVEMENTS.md](PERFORMANCE_IMPROVEMENTS.md). +- Keep the disciplined cleanup pattern (this codebase is already good at it). + +## Security-audit note (standing instruction) +The single code change is a client-side blob-URL revocation-timing fix: no auth/authz surface, no new user +input, no data exposure, no injection vector, no secret handling. Nothing for the security lens to flag. +Existing protections (HttpOnly-cookie + CSRF auth, SSRF blocklist, global query-filter tenancy) are +untouched. diff --git a/docs/performance/PERFORMANCE_IMPROVEMENTS.md b/docs/performance/PERFORMANCE_IMPROVEMENTS.md new file mode 100644 index 0000000..976cfad --- /dev/null +++ b/docs/performance/PERFORMANCE_IMPROVEMENTS.md @@ -0,0 +1,60 @@ +# Performance Improvements — Job Tracker + +**Companion to:** [MEMORY_LEAK_REPORT.md](MEMORY_LEAK_REPORT.md) · [ROOT_CAUSE_ANALYSIS.md](ROOT_CAUSE_ANALYSIS.md) + +## Changes made (this pass) +| Change | File | Effect | Verified | +|---|---|---|---| +| Revoke CV-preview blob URLs on unmount only (ref-based), not on every carousel change | `job-tracker-ui/src/pages/ProfilePage.tsx` | Fixes broken previews on multi-template decks; still frees URLs on unmount | `profile-page.test.tsx` 5/5 | + +> Context: this was the only defect found in a full resource audit. The codebase already practises +> disciplined cleanup (timers cleared, listeners removed, object URLs revoked), so there was no leak to +> fix — see the main report. + +## Recommended (low-severity, optional) + +### 1. Stabilise the extraction-run poll — *minor* +`ProfilePage.tsx:315-324` recreates its 4s interval on every poll because `extractionRuns` is in the deps +and changes each tick. It's harmless (cleanup runs; it stops when runs finish) but churns. If touched: +poll on a stable trigger (e.g. a boolean `hasActiveRuns` in deps, or read runs from a ref inside the +interval) so the interval is created once per active-window. + +### 2. One live heap-snapshot pass on a populated session — *verification, not a fix* +The static audit is strong, but a single DevTools confirmation closes the loop: +1. Run the real stack (backend on `:5202` + a seeded DB) and sign in. +2. DevTools → Memory → take a heap snapshot. +3. Navigate `/dashboard → /jobs → open a job dialog → close → /profile → build a CV deck → back`, ×5. +4. Force GC, take a second snapshot, **Comparison** view. +5. Expect: node/listener/detached counts return to baseline (sawtooth), not monotonic growth. Sort + retained size by constructor; look for `Detached HTMLElement`, growing `Array`/`Map`, or listener + counts that never fall. + +Also cheap and useful: `performance.memory.usedJSHeapSize` (Chromium) logged across the loop, or a +Playwright script that repeats the navigation and asserts heap stays bounded. + +### 3. Guard async setState after unmount — *defensive, not a current leak* +Several components `await api…().then(setState)`. React 18 no-ops setState on unmounted components (just a +dev warning historically), so this is not a leak, but for long CV/AI calls consider an `AbortController` +on the request (cancels the in-flight network work on unmount) — improves responsiveness and avoids wasted +work more than memory. + +## Prevention — keep leaks from creeping in +- **Lint:** enable `react-hooks/exhaustive-deps` (surfaces the exact wrong-deps class that caused the one + bug here) and consider `react-hooks/react-compiler` checks. +- **Rule of thumb:** any effect that *acquires* a resource (listener, timer, object URL, observer, + subscription, connection) must return a cleanup that releases exactly that resource. "Release once on + unmount" ⇒ empty-deps effect + a ref for current state — never a value in the deps array. +- **Object URLs:** pair every `createObjectURL` with a `revokeObjectURL` in the *same* owner; prefer + revoking on unmount/replace, never on unrelated re-renders. +- **Server caches:** every `IMemoryCache.Set` must carry an absolute/sliding expiration (as + `GmailOAuthService` correctly does); if the app grows to heavy caching, set a `SizeLimit`. +- **No unbounded static state:** keep `static` collections to fixed lookup tables (as today); never + accumulate per-request data in a static field. +- **CI:** the heavy RTL suites are timeout-flaky under load — raising `testTimeout` (e.g. 15–20s) or + reducing jest worker contention would make regressions (including any future leak-guard tests) reliably + visible instead of hidden behind flakes. + +## Security-audit note (standing instruction) +The applied change carries no security surface (client-side URL lifetime only). The recommendations above +introduce none either; if #3 (AbortController) is implemented, ensure aborted requests don't leave +partial writes — not applicable to the read-only CV export/preview calls here. diff --git a/docs/performance/ROOT_CAUSE_ANALYSIS.md b/docs/performance/ROOT_CAUSE_ANALYSIS.md new file mode 100644 index 0000000..d43c186 --- /dev/null +++ b/docs/performance/ROOT_CAUSE_ANALYSIS.md @@ -0,0 +1,67 @@ +# Root Cause Analysis — Job Tracker resource audit + +**Companion to:** [MEMORY_LEAK_REPORT.md](MEMORY_LEAK_REPORT.md) + +## Summary +There is **no memory leak** to root-cause. The investigation surfaced exactly one defect — an +**over-eager blob-URL revocation** in the CV PDF carousel — which is a *release-too-early* bug, the +inverse of a leak. This document root-causes that defect and explains why the "app memory grows" symptom +does **not** indicate a leak here. + +## The one defect — over-revoked preview URLs + +### What the code did (before) +`job-tracker-ui/src/pages/ProfilePage.tsx`: +```ts +useEffect(() => { + return () => { + pdfCarousel.forEach((item) => item.pdfUrl && URL.revokeObjectURL(item.pdfUrl)); + }; +}, [pdfCarousel]); // <-- deps on pdfCarousel +``` +A cleanup with `[pdfCarousel]` deps runs its teardown **before every re-run**, i.e. on *every* change to +`pdfCarousel`, not just on unmount. + +### Why it broke +`buildPdfCarousel()` seeds all templates, then `savePdfToCarousel()` replaces each seed **in place**, one +`setPdfCarousel` call at a time (`ProfilePage.tsx:400-410`). Trace with templates A, B, C: + +1. `[A₁, B₀, C₀]` (A built, B/C seeds without URLs) — cleanup revoked prior `[A₀,B₀,C₀]` (no URLs). OK. +2. `[A₁, B₁, C₀]` (B built) — cleanup runs on the **previous** array `[A₁,B₀,C₀]` → **revokes `A₁`'s URL**, + but `A₁` is still present in the new array and still shown when the user flips the carousel to A. +3. `[A₁, B₁, C₁]` (C built) — cleanup revokes `[A₁,B₁,C₀]` → revokes `B₁` too. + +**Result:** after building an N-template deck, every preview except the **last** points at a revoked +(broken) blob URL. + +### Root cause +Wrong effect dependency scope: a resource that should be released **once, on unmount** was tied to a +value-change dependency, so React's "cleanup-before-next-run" semantics turned it into a per-change +revoke. Compounded by the fact that legitimate drop paths already revoke explicitly +(`savePdfToCarousel` replace at `:402-403`, `resetPdfCarousel` clear at `:378-384`), making the effect's +revocation redundant *and* destructive. + +### Why it is not a leak +On unmount the effect *did* revoke the current array (deps capture the latest value), so URLs were freed. +The bug wastes nothing and retains nothing — it releases too **eagerly**. It is a correctness bug +(broken previews), filed here because Phase 3.5 explicitly covers "image/media resources … released". + +### Fix (commit `eed9b1f`) +Track the carousel in a ref; revoke **only on unmount** (empty-deps effect). Drop paths keep their +explicit revokes. Verified: `profile-page.test.tsx` 5/5. + +## Why the "memory grows" symptom is not a leak here +Per the mission's Final Rule, distinguishing the four causes: +- **Expected caching** — MUI emotion style cache, `react-scripts` dev tooling, and route component state + grow then plateau; not unbounded. +- **Delayed GC** — detached nodes from closed dialogs/pages are collected on the next major GC, not + instantly; a rising sawtooth is normal. +- **Browser behaviour** — bfcache, image decode buffers, and devtools retention inflate numbers in a way + unrelated to app code. +- **Genuine leak** — would require a retained root (listener, timer, global ref, live connection). None + exists in this codebase (see the vector table in the main report). + +## Contributing (non-defect) observations +- **Extraction-poll churn** (`ProfilePage.tsx:315-324`): interval recreated every 4s while a run is + active because `extractionRuns` is in the deps and mutates each poll. Harmless; optionally stabilise + (see improvements doc).