From a7399cf11fc2068b87507f71130824b8fe9fa93e Mon Sep 17 00:00:00 2001 From: Ramzi <79337754+heyramzi@users.noreply.github.com> Date: Wed, 15 Apr 2026 05:07:48 +0100 Subject: [PATCH] fix: plug memory leaks, unbounded caches, and disk accumulation (#595) Co-authored-by: Jinwoo-H --- src/main/browser/browser-manager.ts | 68 ++++++++++++++----- src/main/opencode/hook-service.ts | 23 ++++++- src/main/stats/collector.ts | 12 ++++ .../src/components/editor/MermaidBlock.tsx | 15 +++- .../src/components/editor/useLocalImageSrc.ts | 19 +++++- .../right-sidebar/useGitStatusPolling.ts | 45 ++++++++++-- src/renderer/src/store/slices/editor.ts | 22 ++++++ src/renderer/src/store/slices/github.ts | 47 ++++++++++++- 8 files changed, 220 insertions(+), 31 deletions(-) diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index 539918bac..325a10bf2 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -81,6 +81,7 @@ class BrowserManager { private readonly grabShortcutCleanupByTabId = new Map void>() private readonly shortcutForwardingCleanupByTabId = new Map void>() private readonly policyAttachedGuestIds = new Set() + private readonly policyCleanupByGuestId = new Map void>() private readonly pendingLoadFailuresByGuestId = new Map< number, { code: number; description: string; validatedUrl: string } @@ -155,27 +156,37 @@ class BrowserManager { } } + const didFailLoadHandler = ( + _event: Electron.Event, + errorCode: number, + errorDescription: string, + validatedURL: string, + isMainFrame: boolean + ): void => { + if (!isMainFrame || errorCode === -3) { + return + } + this.forwardOrQueueGuestLoadFailure(guest.id, { + code: errorCode, + description: errorDescription || 'This site could not be reached.', + validatedUrl: validatedURL || guest.getURL() || 'about:blank' + }) + } + guest.on('will-navigate', navigationGuard) guest.on('will-redirect', navigationGuard) - guest.on( - 'did-fail-load', - ( - _event: Electron.Event, - errorCode: number, - errorDescription: string, - validatedURL: string, - isMainFrame: boolean - ) => { - if (!isMainFrame || errorCode === -3) { - return - } - this.forwardOrQueueGuestLoadFailure(guest.id, { - code: errorCode, - description: errorDescription || 'This site could not be reached.', - validatedUrl: validatedURL || guest.getURL() || 'about:blank' - }) + guest.on('did-fail-load', didFailLoadHandler) + + // Why: store cleanup so unregisterGuest can remove these listeners when the + // guest surface is torn down, preventing the callbacks from preventing GC of + // the underlying WebContents wrapper. + this.policyCleanupByGuestId.set(guest.id, () => { + if (!guest.isDestroyed()) { + guest.off('will-navigate', navigationGuard) + guest.off('will-redirect', navigationGuard) + guest.off('did-fail-load', didFailLoadHandler) } - ) + }) } registerGuest({ @@ -239,6 +250,19 @@ class BrowserManager { // instead of a dangling Promise. this.cancelGrabOp(browserTabId, 'evicted') + // Why: remove the policy listeners attached in attachGuestPolicies so the + // callbacks (which close over the guest WebContents) do not prevent GC of + // the underlying Chromium surface after the guest is destroyed. + const guestWebContentsId = this.webContentsIdByTabId.get(browserTabId) + if (guestWebContentsId !== undefined) { + const policyCleanup = this.policyCleanupByGuestId.get(guestWebContentsId) + if (policyCleanup) { + policyCleanup() + this.policyCleanupByGuestId.delete(guestWebContentsId) + } + this.policyAttachedGuestIds.delete(guestWebContentsId) + } + const cleanup = this.contextMenuCleanupByTabId.get(browserTabId) if (cleanup) { cleanup() @@ -280,6 +304,14 @@ class BrowserManager { this.unregisterGuest(browserTabId) } this.policyAttachedGuestIds.clear() + // Why: unregisterGuest only cleans up guests that were registered (have an + // entry in webContentsIdByTabId). Guests that went through + // attachGuestPolicies but were never registered still have cleanup closures + // here — invoke them so their event listeners are removed before clearing. + for (const cleanup of this.policyCleanupByGuestId.values()) { + cleanup() + } + this.policyCleanupByGuestId.clear() this.tabIdByWebContentsId.clear() this.pendingLoadFailuresByGuestId.clear() this.pendingPermissionEventsByGuestId.clear() diff --git a/src/main/opencode/hook-service.ts b/src/main/opencode/hook-service.ts index e3ee68e52..648a297a6 100644 --- a/src/main/opencode/hook-service.ts +++ b/src/main/opencode/hook-service.ts @@ -2,7 +2,7 @@ import { app, BrowserWindow } from 'electron' import { randomUUID } from 'crypto' import { createServer, type IncomingMessage, type ServerResponse } from 'http' import { join } from 'path' -import { mkdirSync, writeFileSync } from 'fs' +import { mkdirSync, writeFileSync, rmSync } from 'fs' import type { OpenCodeStatusEvent } from '../../shared/types' const ORCA_OPENCODE_PLUGIN_FILE = 'orca-opencode-status.js' @@ -174,11 +174,32 @@ export class OpenCodeHookService { this.server?.close() this.server = null this.port = 0 + // Why: clean up all remaining PTY config directories before clearing the + // in-memory tracking. Without this, directories from the current session's + // PTYs survive on disk after shutdown. + for (const ptyId of this.lastStatusByPtyId.keys()) { + const configDir = join(app.getPath('userData'), 'opencode-hooks', ptyId) + try { + rmSync(configDir, { recursive: true, force: true }) + } catch { + // best-effort + } + } this.lastStatusByPtyId.clear() } clearPty(ptyId: string): void { this.lastStatusByPtyId.delete(ptyId) + // Why: writePluginConfig creates a directory per PTY under userData. Without + // cleanup these accumulate across sessions since ptyId is a monotonically + // increasing counter. Remove the directory when the PTY is torn down. + const configDir = join(app.getPath('userData'), 'opencode-hooks', ptyId) + try { + rmSync(configDir, { recursive: true, force: true }) + } catch { + // Why: best-effort cleanup. The directory may already be gone if the user + // manually purged userData, or the OS may hold a lock briefly. + } } buildPtyEnv(ptyId: string): Record { diff --git a/src/main/stats/collector.ts b/src/main/stats/collector.ts index 84eb90afe..a30112f62 100644 --- a/src/main/stats/collector.ts +++ b/src/main/stats/collector.ts @@ -6,6 +6,11 @@ import type { StatsEvent, StatsAggregates, StatsFile } from './types' const STATS_SCHEMA_VERSION = 1 const MAX_EVENTS = 10_000 +// Why: countedPRs is a deduplication registry that grows with every PR created +// through Orca. Without a cap, a heavily-used instance accumulates thousands of +// URL strings across months. 2000 entries is about 6-12 months of active use +// for a power user, and at ~50 chars per URL the overhead is ~100KB max. +const MAX_COUNTED_PRS = 2_000 // Why 5s instead of the main store's 300ms: stat events are infrequent // (a few per session) and not latency-sensitive for the UI. const DEBOUNCE_MS = 5_000 @@ -173,6 +178,13 @@ export class StatsCollector { this.aggregates.totalPRsCreated++ if (event.meta?.prUrl) { this.aggregates.countedPRs.push(String(event.meta.prUrl)) + // Why: trim oldest entries so the dedup array does not grow without + // bound. The aggregate totalPRsCreated counter remains accurate; only + // the dedup lookup for very old PRs is lost, which is acceptable + // since PRs that old would never be re-counted in practice. + if (this.aggregates.countedPRs.length > MAX_COUNTED_PRS) { + this.aggregates.countedPRs = this.aggregates.countedPRs.slice(-MAX_COUNTED_PRS) + } } break // agent_stop duration is handled directly in onAgentStop() to avoid diff --git a/src/renderer/src/components/editor/MermaidBlock.tsx b/src/renderer/src/components/editor/MermaidBlock.tsx index 2c8577b82..96db938f4 100644 --- a/src/renderer/src/components/editor/MermaidBlock.tsx +++ b/src/renderer/src/components/editor/MermaidBlock.tsx @@ -13,8 +13,21 @@ type MermaidBlockProps = { // parser state). Running multiple renders concurrently causes race conditions // where one render can clobber another's temporary DOM node. Serializing all // render calls through a single promise chain avoids this. +// +// The queue is replaced with a fresh promise after each render completes so +// that old .then() closures (which capture containerRef, content, and id) +// become unreachable and can be GC'd. Without this, the chain grows with +// every MermaidBlock mount/unmount cycle for the lifetime of the renderer. let renderQueue: Promise = Promise.resolve() +function enqueueRender(fn: () => Promise): void { + renderQueue = renderQueue.then(fn, fn).then(() => { + // Why: collapse the chain back to a single resolved promise so previous + // closures do not remain reachable through a growing .then() chain. + renderQueue = Promise.resolve() + }) +} + /** * Renders a mermaid diagram string as SVG. Falls back to raw source with an * error banner if the syntax is invalid — never breaks the rest of the preview. @@ -61,7 +74,7 @@ export default function MermaidBlock({ // Serialize render calls through a module-level queue to avoid race // conditions from concurrent mermaid.render() invocations. - renderQueue = renderQueue.then(render, render) + enqueueRender(render) return () => { cancelled = true } diff --git a/src/renderer/src/components/editor/useLocalImageSrc.ts b/src/renderer/src/components/editor/useLocalImageSrc.ts index eba651c8d..3206e99b3 100644 --- a/src/renderer/src/components/editor/useLocalImageSrc.ts +++ b/src/renderer/src/components/editor/useLocalImageSrc.ts @@ -39,16 +39,29 @@ function base64ToBlobUrl(base64: string, mimeType: string): string { } // Why: when the user switches back to the app after deleting or replacing -// image files externally, clearing the cache ensures the preview picks up +// image files externally, clearing the cache forces the preview to pick up // the current filesystem state instead of showing stale in-memory blob URLs. -// Old blob URLs are intentionally NOT revoked so that elements keep -// displaying until the fresh IPC load completes, avoiding a visible flash. +// Old blob URLs are revoked after a short delay so that elements still +// display the old data while the fresh IPC load completes, avoiding a visible +// flash. The 5-second window is generous enough for even slow IPC reads. function invalidateImageCache(): void { + const staleUrls = Array.from(blobUrlCache.values()) blobUrlCache.clear() cacheGeneration += 1 for (const listener of cacheListeners) { listener() } + // Why: defer revocation so the browser keeps the old blob data readable + // until replacement IPC loads complete, then free the underlying memory. + // 30 seconds is generous enough to cover slow machines or large images + // without risking a visible broken-image flash. + if (staleUrls.length > 0) { + setTimeout(() => { + for (const url of staleUrls) { + URL.revokeObjectURL(url) + } + }, 30_000) + } } if (typeof window !== 'undefined') { diff --git a/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts b/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts index fbc1754c8..6c02185db 100644 --- a/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts +++ b/src/renderer/src/components/right-sidebar/useGitStatusPolling.ts @@ -84,8 +84,23 @@ export function useGitStatusPolling(): void { useEffect(() => { void fetchStatus() - const intervalId = setInterval(() => void fetchStatus(), POLL_INTERVAL_MS) - return () => clearInterval(intervalId) + // Why: skip IPC-heavy git status calls when the window is not focused. + // These intervals run at the App root level regardless of which sidebar tab + // is open, so gating on document.hasFocus() prevents wasted CPU and IPC + // traffic while the user is working in another application. + const intervalId = setInterval(() => { + if (document.hasFocus()) { + void fetchStatus() + } + }, POLL_INTERVAL_MS) + // Why: when the user returns to the window, poll immediately so the sidebar + // shows up-to-date status without waiting up to POLL_INTERVAL_MS. + const onFocus = (): void => void fetchStatus() + window.addEventListener('focus', onFocus) + return () => { + clearInterval(intervalId) + window.removeEventListener('focus', onFocus) + } }, [fetchStatus]) useEffect(() => { @@ -98,8 +113,17 @@ export function useGitStatusPolling(): void { // list so a branch change updates the sidebar's PR key instead of leaving // the previous merged PR attached to this worktree indefinitely. void fetchWorktrees(activeRepoId) - const intervalId = setInterval(() => void fetchWorktrees(activeRepoId), POLL_INTERVAL_MS) - return () => clearInterval(intervalId) + const intervalId = setInterval(() => { + if (document.hasFocus()) { + void fetchWorktrees(activeRepoId) + } + }, POLL_INTERVAL_MS) + const onFocus = (): void => void fetchWorktrees(activeRepoId) + window.addEventListener('focus', onFocus) + return () => { + clearInterval(intervalId) + window.removeEventListener('focus', onFocus) + } }, [activeRepoId, activeRepoSupportsGit, fetchWorktrees]) // Why: poll conflict operation for non-active worktrees that have a stale @@ -125,7 +149,16 @@ export function useGitStatusPolling(): void { } void pollStale() - const intervalId = setInterval(() => void pollStale(), POLL_INTERVAL_MS) - return () => clearInterval(intervalId) + const intervalId = setInterval(() => { + if (document.hasFocus()) { + void pollStale() + } + }, POLL_INTERVAL_MS) + const onFocus = (): void => void pollStale() + window.addEventListener('focus', onFocus) + return () => { + clearInterval(intervalId) + window.removeEventListener('focus', onFocus) + } }, [staleConflictWorktrees, setConflictOperation]) } diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index e02a8c80e..7edf9596b 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -446,6 +446,16 @@ export const createEditorSlice: StateCreator = (s ([fileId]) => fileId !== replacedPreview.id ) ) + // Why: editorCursorLine entries accumulate per file; clean up the + // evicted preview's entry so it does not leak across tab replacements. + const nextEditorCursorLine = + replacedPreview.id === id + ? s.editorCursorLine + : Object.fromEntries( + Object.entries(s.editorCursorLine).filter( + ([fileId]) => fileId !== replacedPreview.id + ) + ) // Replace in-place to preserve tab position newFiles = s.openFiles.map((f, i) => i === existingPreviewIdx ? { ...file, id, isDirty: false, isPreview: true } : f @@ -463,6 +473,7 @@ export const createEditorSlice: StateCreator = (s return { openFiles: newFiles, editorDrafts: nextEditorDrafts, + editorCursorLine: nextEditorCursorLine, markdownViewMode: nextMarkdownViewMode, ...previewTabBarUpdate, ...activeResult @@ -552,6 +563,11 @@ export const createEditorSlice: StateCreator = (s delete newEditorDrafts[fileId] const newMarkdownViewMode = { ...s.markdownViewMode } delete newMarkdownViewMode[fileId] + // Why: editorCursorLine entries are keyed by fileId and accumulate on + // every cursor move. Without cleanup they grow without bound across a + // long session as files are opened and closed. + const newEditorCursorLine = { ...s.editorCursorLine } + delete newEditorCursorLine[fileId] let newActiveId = s.activeFileId const newActiveFileIdByWorktree = { ...s.activeFileIdByWorktree } @@ -632,6 +648,7 @@ export const createEditorSlice: StateCreator = (s return { openFiles: newFiles, editorDrafts: newEditorDrafts, + editorCursorLine: newEditorCursorLine, activeFileId: newActiveId, // Why: if closing the last editor also leaves the worktree without any // browser or terminal surface, keep parity with the terminal/browser @@ -671,6 +688,7 @@ export const createEditorSlice: StateCreator = (s return { openFiles: [], editorDrafts: {}, + editorCursorLine: {}, activeFileId: null, activeTabType: 'terminal', markdownViewMode: {}, @@ -686,6 +704,9 @@ export const createEditorSlice: StateCreator = (s const newMarkdownViewMode = Object.fromEntries( Object.entries(s.markdownViewMode).filter(([fileId]) => remainingFileIds.has(fileId)) ) + const newEditorCursorLine = Object.fromEntries( + Object.entries(s.editorCursorLine).filter(([fileId]) => remainingFileIds.has(fileId)) + ) const newActiveFileIdByWorktree = { ...s.activeFileIdByWorktree } delete newActiveFileIdByWorktree[activeWorktreeId] const newActiveTabTypeByWorktree = { ...s.activeTabTypeByWorktree } @@ -713,6 +734,7 @@ export const createEditorSlice: StateCreator = (s return { openFiles: newFiles, editorDrafts: newEditorDrafts, + editorCursorLine: newEditorCursorLine, activeFileId: null, // Why: closing every editor in the active worktree can leave no // renderable surface at all. Clear the active worktree in that case so diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 4e722c668..32fa21461 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -32,10 +32,38 @@ const inflightChecksRequests = new Map>() const inflightCommentsRequests = new Map>() const prRequestGenerations = new Map() +// Why: 500 entries is generous enough that active developers will never hit it +// during normal use, but prevents the cache from growing without bound across +// many repos and branches over a long-running session. +const MAX_CACHE_ENTRIES = 500 + function isFresh(entry: CacheEntry | undefined, ttl = CACHE_TTL): entry is CacheEntry { return entry !== undefined && Date.now() - entry.fetchedAt < ttl } +/** + * Evict the oldest entries from a cache record when it exceeds the max size. + * Returns a pruned copy, or the original reference if no eviction was needed. + */ +function evictStaleEntries( + cache: Record>, + maxEntries = MAX_CACHE_ENTRIES +): Record> { + const keys = Object.keys(cache) + if (keys.length <= maxEntries) { + return cache + } + const sorted = keys + .map((k) => ({ key: k, fetchedAt: cache[k].fetchedAt })) + .sort((a, b) => b.fetchedAt - a.fetchedAt) + const keep = new Set(sorted.slice(0, maxEntries).map((e) => e.key)) + const pruned: Record> = {} + for (const k of keep) { + pruned[k] = cache[k] + } + return pruned +} + let saveTimer: ReturnType | null = null function debouncedSaveCache(state: AppState): void { @@ -316,8 +344,23 @@ export const createGitHubSlice: StateCreator = (s }, refreshAllGitHub: () => { - // Invalidate checks and comments caches so they refresh on next access - set({ checksCache: {}, commentsCache: {} }) + // Invalidate checks and comments caches so they refresh on next access. + // Also evict old entries from prCache and issueCache to prevent unbounded + // growth across many repos and branches over a long-running session. + set((s) => ({ + checksCache: {}, + commentsCache: {}, + prCache: evictStaleEntries(s.prCache), + issueCache: evictStaleEntries(s.issueCache) + })) + + // Why: prRequestGenerations tracks generation counters for inflight + // fetch deduplication. Pruning keys that were just evicted from prCache + // would race with inflight requests — their generation check would fail + // and silently discard valid responses. Since each entry is just a number, + // the memory overhead is negligible; let it shrink naturally as keys stop + // being fetched. The eviction on prCache/issueCache above is sufficient + // to bound the dominant source of growth. // Only re-fetch PR/issue entries that are already stale — skip fresh ones const state = get()