fix: plug memory leaks, unbounded caches, and disk accumulation (#595)
Co-authored-by: Jinwoo-H <jinwoo0825@gmail.com>
This commit is contained in:
parent
c889e95d65
commit
a7399cf11f
|
|
@ -81,6 +81,7 @@ class BrowserManager {
|
|||
private readonly grabShortcutCleanupByTabId = new Map<string, () => void>()
|
||||
private readonly shortcutForwardingCleanupByTabId = new Map<string, () => void>()
|
||||
private readonly policyAttachedGuestIds = new Set<number>()
|
||||
private readonly policyCleanupByGuestId = new Map<number, () => 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()
|
||||
|
|
|
|||
|
|
@ -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<string, string> {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<void> = Promise.resolve()
|
||||
|
||||
function enqueueRender(fn: () => Promise<void>): 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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 <img> elements keep
|
||||
// displaying until the fresh IPC load completes, avoiding a visible flash.
|
||||
// Old blob URLs are revoked after a short delay so that <img> 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') {
|
||||
|
|
|
|||
|
|
@ -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])
|
||||
}
|
||||
|
|
|
|||
|
|
@ -446,6 +446,16 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (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<AppState, [], [], EditorSlice> = (s
|
|||
return {
|
||||
openFiles: newFiles,
|
||||
editorDrafts: nextEditorDrafts,
|
||||
editorCursorLine: nextEditorCursorLine,
|
||||
markdownViewMode: nextMarkdownViewMode,
|
||||
...previewTabBarUpdate,
|
||||
...activeResult
|
||||
|
|
@ -552,6 +563,11 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (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<AppState, [], [], EditorSlice> = (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<AppState, [], [], EditorSlice> = (s
|
|||
return {
|
||||
openFiles: [],
|
||||
editorDrafts: {},
|
||||
editorCursorLine: {},
|
||||
activeFileId: null,
|
||||
activeTabType: 'terminal',
|
||||
markdownViewMode: {},
|
||||
|
|
@ -686,6 +704,9 @@ export const createEditorSlice: StateCreator<AppState, [], [], EditorSlice> = (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<AppState, [], [], EditorSlice> = (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
|
||||
|
|
|
|||
|
|
@ -32,10 +32,38 @@ const inflightChecksRequests = new Map<string, Promise<PRCheckDetail[]>>()
|
|||
const inflightCommentsRequests = new Map<string, Promise<PRComment[]>>()
|
||||
const prRequestGenerations = new Map<string, number>()
|
||||
|
||||
// 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<T>(entry: CacheEntry<T> | undefined, ttl = CACHE_TTL): entry is CacheEntry<T> {
|
||||
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<T>(
|
||||
cache: Record<string, CacheEntry<T>>,
|
||||
maxEntries = MAX_CACHE_ENTRIES
|
||||
): Record<string, CacheEntry<T>> {
|
||||
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<string, CacheEntry<T>> = {}
|
||||
for (const k of keep) {
|
||||
pruned[k] = cache[k]
|
||||
}
|
||||
return pruned
|
||||
}
|
||||
|
||||
let saveTimer: ReturnType<typeof setTimeout> | null = null
|
||||
|
||||
function debouncedSaveCache(state: AppState): void {
|
||||
|
|
@ -316,8 +344,23 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (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()
|
||||
|
|
|
|||
Loading…
Reference in New Issue