-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: renderer OOM crash recovery and memory management #1843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
4457096
2c07bd6
23030cc
489dc1b
476f4b3
e7b400c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { selectThreadsToEvict, EVICTION_KEEP_COUNT, type EvictableThread } from "./threadEviction"; | ||
|
|
||
| function makeEvictable(id: string, overrides: Partial<EvictableThread> = {}): EvictableThread { | ||
| return { | ||
| id, | ||
| hydrated: true, | ||
| isActive: false, | ||
| hasRunningSession: false, | ||
| updatedAt: "2026-01-01T00:00:00.000Z", | ||
| messageCount: 0, | ||
| activityCount: 0, | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| describe("selectThreadsToEvict", () => { | ||
| it("returns empty array when thread count is within keep limit", () => { | ||
| const threads = Array.from({ length: EVICTION_KEEP_COUNT }, (_, i) => makeEvictable(`t-${i}`)); | ||
| expect(selectThreadsToEvict(threads, "t-0")).toEqual([]); | ||
| }); | ||
|
|
||
| it("never evicts the active thread", () => { | ||
| const threads = Array.from({ length: EVICTION_KEEP_COUNT + 5 }, (_, i) => | ||
| makeEvictable(`t-${i}`), | ||
| ); | ||
| const result = selectThreadsToEvict(threads, "t-0"); | ||
| expect(result).not.toContain("t-0"); | ||
| }); | ||
|
|
||
| it("never evicts threads with running sessions", () => { | ||
| const threads = [ | ||
| ...Array.from({ length: EVICTION_KEEP_COUNT + 3 }, (_, i) => makeEvictable(`t-${i}`)), | ||
| makeEvictable("t-running", { hasRunningSession: true }), | ||
| ]; | ||
| const result = selectThreadsToEvict(threads, "t-0"); | ||
| expect(result).not.toContain("t-running"); | ||
| }); | ||
|
|
||
| it("evicts oldest idle threads first", () => { | ||
| const threads = [ | ||
| makeEvictable("t-old", { updatedAt: "2026-01-01T00:00:00.000Z" }), | ||
| makeEvictable("t-new", { updatedAt: "2026-04-01T00:00:00.000Z" }), | ||
| ...Array.from({ length: EVICTION_KEEP_COUNT }, (_, i) => | ||
| makeEvictable(`t-keep-${i}`, { updatedAt: "2026-03-01T00:00:00.000Z" }), | ||
| ), | ||
| ]; | ||
| const result = selectThreadsToEvict(threads, "t-new"); | ||
| expect(result).toContain("t-old"); | ||
| expect(result).not.toContain("t-new"); | ||
| }); | ||
|
|
||
| it("skips already-dehydrated threads", () => { | ||
| const threads = [ | ||
| makeEvictable("t-dehydrated", { hydrated: false }), | ||
| ...Array.from({ length: EVICTION_KEEP_COUNT + 2 }, (_, i) => makeEvictable(`t-${i}`)), | ||
| ]; | ||
| const result = selectThreadsToEvict(threads, "t-0"); | ||
| expect(result).not.toContain("t-dehydrated"); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| /** | ||
| * Thread eviction policy for renderer memory management. | ||
| * | ||
| * Decides which threads should have their heavy data (messages, activities, | ||
| * proposedPlans, turnDiffSummaries) dropped from the Zustand store. | ||
| * Sidebar metadata is always retained. | ||
| */ | ||
|
|
||
| /** How many fully-hydrated threads to keep in memory at once. */ | ||
| export const EVICTION_KEEP_COUNT = 5; | ||
|
|
||
| export interface EvictableThread { | ||
| id: string; | ||
| hydrated: boolean; | ||
| isActive: boolean; | ||
| hasRunningSession: boolean; | ||
| updatedAt: string; | ||
| messageCount: number; | ||
| activityCount: number; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the IDs of threads that should be evicted (dehydrated). | ||
| * Never evicts: the active thread, threads with running sessions, | ||
| * or already-dehydrated threads. | ||
| */ | ||
| export function selectThreadsToEvict( | ||
| threads: ReadonlyArray<EvictableThread>, | ||
| activeThreadId: string | null, | ||
| ): string[] { | ||
| const evictable = threads.filter( | ||
| (t) => t.hydrated && !t.isActive && t.id !== activeThreadId && !t.hasRunningSession, | ||
| ); | ||
|
|
||
| const hydratedCount = threads.filter((t) => t.hydrated).length; | ||
|
|
||
| if (hydratedCount <= EVICTION_KEEP_COUNT) { | ||
| return []; | ||
| } | ||
|
|
||
| const toEvictCount = hydratedCount - EVICTION_KEEP_COUNT; | ||
|
|
||
| // Sort by updatedAt ascending — oldest idle threads get evicted first | ||
| const sorted = evictable.toSorted((a, b) => a.updatedAt.localeCompare(b.updatedAt)); | ||
|
|
||
| return sorted.slice(0, toEvictCount).map((t) => t.id); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ import { deriveOrchestrationBatchEffects } from "../orchestrationEventEffects"; | |
| import { createOrchestrationRecoveryCoordinator } from "../orchestrationRecovery"; | ||
| import { deriveReplayRetryDecision } from "../orchestrationRecovery"; | ||
| import { getWsRpcClient } from "~/wsRpcClient"; | ||
| import { selectThreadsToEvict, type EvictableThread } from "~/lib/threadEviction"; | ||
|
|
||
| export const Route = createRootRouteWithContext<{ | ||
| queryClient: QueryClient; | ||
|
|
@@ -588,5 +589,31 @@ function EventRouter() { | |
| useServerWelcomeSubscription(handleWelcome); | ||
| useServerConfigUpdatedSubscription(handleServerConfigUpdated); | ||
|
|
||
| const evictThread = useStore((store) => store.evictThreadData); | ||
| const allThreads = useStore((store) => store.threads); | ||
|
|
||
| // Evict inactive threads when navigating to keep memory bounded. | ||
| // This is a separate effect from the WS subscription lifecycle. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Critical The 🤖 Copy this AI Prompt to have your agent fix this: |
||
| useEffect(() => { | ||
| const activeThreadId = pathname.startsWith("/chat/") | ||
| ? (pathname.split("/chat/")[1]?.split("/")[0] ?? null) | ||
| : null; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrong pathname pattern makes active thread always nullHigh Severity The thread eviction effect incorrectly assumes thread URLs start with Reviewed by Cursor Bugbot for commit 489dc1b. Configure here. |
||
|
|
||
| const evictable: EvictableThread[] = allThreads.map((t) => ({ | ||
| id: t.id, | ||
| hydrated: t.hydrated, | ||
| isActive: t.id === activeThreadId, | ||
| hasRunningSession: t.session?.status === "running", | ||
| updatedAt: t.updatedAt ?? t.createdAt, | ||
| messageCount: t.messages.length, | ||
| activityCount: t.activities.length, | ||
| })); | ||
|
|
||
| const idsToEvict = selectThreadsToEvict(evictable, activeThreadId); | ||
| for (const id of idsToEvict) { | ||
| evictThread(id as any); | ||
| } | ||
| }, [pathname, evictThread, allThreads]); | ||
|
|
||
| return null; | ||
| } | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No retry limit risks infinite crash-reload loop
Medium Severity
The
render-process-gonehandler unconditionally schedules a reload after 500ms for every crash/OOM/killed event, with no maximum retry count or backoff. If the renderer crashes immediately after loading (e.g., due to corrupted persisted state or a deterministic startup error), this creates an infinite crash-reload loop at ~2Hz. The user would see a rapidly flickering white screen with no way to recover except force-quitting the app — arguably worse than the previous static white screen behavior.Reviewed by Cursor Bugbot for commit 489dc1b. Configure here.