diff --git a/packages/ui/src/sync/child-store.ts b/packages/ui/src/sync/child-store.ts index cfd23584..0b0142de 100644 --- a/packages/ui/src/sync/child-store.ts +++ b/packages/ui/src/sync/child-store.ts @@ -1,6 +1,6 @@ import { create, type StoreApi } from "zustand" import type { DirState, State } from "./types" -import { INITIAL_STATE, MAX_DIR_STORES, DIR_IDLE_TTL_MS } from "./types" +import { INITIAL_STATE, MAX_DIR_STORES, DIR_IDLE_TTL_MS, EVICTION_GRACE_MS } from "./types" import { pickDirectoriesToEvict, canDisposeDirectory, hasPendingBlockingRequests } from "./eviction" import { readDirCache, persistVcs, persistProjectMeta, persistIcon, persistSessions } from "./persist-cache" import { normalizePath } from "@/lib/pathNormalization" @@ -250,6 +250,7 @@ export class ChildStoreManager { readonly children = new Map>() private readonly lifecycle = new Map() private readonly pins = new Map() + private evictionScheduled = false private readonly disposers = new Map void>() private readonly registrySubscribers = new Set<() => void>() private readonly bootstrapSubscribers = new Set<() => void>() @@ -308,7 +309,25 @@ export class ChildStoreManager { mark(directory: string) { if (!directory) return this.lifecycle.set(directory, { lastAccessAt: Date.now() }) - this.runEviction(directory) + this.scheduleEviction() + } + + /** + * Coalesce eviction into one pass per tick. + * + * `ensureChild` runs during render, once per sidebar row, and used to sort + * and scan every directory synchronously on each call. Deferring the pass + * also lets a whole render commit — and with it every pin effect — settle + * before anything is considered for disposal. + */ + private scheduleEviction() { + if (this.evictionScheduled || this.disposed) return + this.evictionScheduled = true + queueMicrotask(() => { + this.evictionScheduled = false + if (this.disposed) return + this.runEviction() + }) } pin(directory: string) { @@ -327,6 +346,8 @@ export class ChildStoreManager { return } this.pins.delete(normalizedDirectory) + // Releasing the final consumer is an explicit lifecycle edge, not a render- + // path access, so this pass stays synchronous. this.runEviction() } @@ -621,6 +642,7 @@ export class ChildStoreManager { pins: new Set(stores.filter((d) => this.pinned(d))), max: MAX_DIR_STORES, ttl: DIR_IDLE_TTL_MS, + graceMs: EVICTION_GRACE_MS, now: Date.now(), hasPendingBlockingRequests: (dir) => this.hasPendingBlockingRequestsForDirectory(dir), }).filter((d) => d !== skip) diff --git a/packages/ui/src/sync/eviction-thrash.test.ts b/packages/ui/src/sync/eviction-thrash.test.ts new file mode 100644 index 00000000..da316215 --- /dev/null +++ b/packages/ui/src/sync/eviction-thrash.test.ts @@ -0,0 +1,110 @@ +import { describe, expect, test } from "bun:test" + +import { pickDirectoriesToEvict } from "./eviction" +import { DIR_IDLE_TTL_MS, EVICTION_GRACE_MS, MAX_DIR_STORES } from "./types" + +/** + * Regression coverage for the sidebar cache-thrash loop (issue #1472). + * + * Expanding a project with many worktrees mounts a sidebar row per directory. + * Each row calls `ensureChild` during render, while the pin that protects it is + * only taken in an effect after commit. With more live directories than the + * store limit, overflow eviction therefore disposed directories that were + * actively being rendered; the next render recreated them with a `loading` + * status, which issued another bootstrap request, and the cycle repeated + * indefinitely. + * + * The fix treats the limit as a soft target: a directory touched within the + * grace window is never an overflow victim, so a burst of live directories + * overflows the cache briefly instead of thrashing. Idle directories stay + * evictable, which is what keeps the cache bounded. + */ + +const buildState = (directories: string[], lastAccessAt: number) => + new Map(directories.map((directory) => [directory, { lastAccessAt }])) + +const directories = (count: number, prefix = "/repo/worktree-") => + Array.from({ length: count }, (_, index) => `${prefix}${index}`) + +describe("directory eviction under sidebar expansion", () => { + test("does not evict directories that are being accessed right now", () => { + const now = 1_000_000 + const stores = directories(MAX_DIR_STORES + 25) + + const evicted = pickDirectoriesToEvict({ + stores, + state: buildState(stores, now), + pins: new Set(), + max: MAX_DIR_STORES, + ttl: DIR_IDLE_TTL_MS, + graceMs: EVICTION_GRACE_MS, + now, + }) + + expect(evicted).toEqual([]) + }) + + test("still evicts overflow once directories fall outside the grace window", () => { + const now = 1_000_000 + const live = directories(MAX_DIR_STORES, "/repo/live-") + const stale = directories(5, "/repo/stale-") + const state = new Map([ + ...buildState(live, now), + ...buildState(stale, now - EVICTION_GRACE_MS - 1), + ]) + + const evicted = pickDirectoriesToEvict({ + stores: [...live, ...stale], + state, + pins: new Set(), + max: MAX_DIR_STORES, + ttl: DIR_IDLE_TTL_MS, + graceMs: EVICTION_GRACE_MS, + now, + }) + + expect([...evicted].sort()).toEqual([...stale].sort()) + }) + + test("still evicts directories idle past the TTL even inside the limit", () => { + const now = 1_000_000 + const active = directories(3, "/repo/active-") + const abandoned = directories(2, "/repo/abandoned-") + const state = new Map([ + ...buildState(active, now), + ...buildState(abandoned, now - DIR_IDLE_TTL_MS - 1), + ]) + + const evicted = pickDirectoriesToEvict({ + stores: [...active, ...abandoned], + state, + pins: new Set(), + max: MAX_DIR_STORES, + ttl: DIR_IDLE_TTL_MS, + graceMs: EVICTION_GRACE_MS, + now, + }) + + expect([...evicted].sort()).toEqual([...abandoned].sort()) + }) + + test("never evicts pinned or blocked directories regardless of overflow", () => { + const now = 1_000_000 + const stores = directories(MAX_DIR_STORES + 10) + const stale = now - EVICTION_GRACE_MS - 1 + + const evicted = pickDirectoriesToEvict({ + stores, + state: buildState(stores, stale), + pins: new Set([stores[0]]), + max: MAX_DIR_STORES, + ttl: DIR_IDLE_TTL_MS, + graceMs: EVICTION_GRACE_MS, + now, + hasPendingBlockingRequests: (directory) => directory === stores[1], + }) + + expect(evicted).not.toContain(stores[0]) + expect(evicted).not.toContain(stores[1]) + }) +}) diff --git a/packages/ui/src/sync/eviction.ts b/packages/ui/src/sync/eviction.ts index 723149a4..cb9a65a5 100644 --- a/packages/ui/src/sync/eviction.ts +++ b/packages/ui/src/sync/eviction.ts @@ -26,6 +26,7 @@ export function hasPendingBlockingRequests(state: State | undefined): boolean { export function pickDirectoriesToEvict(input: EvictPlan) { const overflow = Math.max(0, input.stores.length - input.max) let pendingOverflow = overflow + const graceMs = input.graceMs ?? 0 const sorted = input.stores .filter((dir) => !input.pins.has(dir)) .filter((dir) => !input.hasPendingBlockingRequests?.(dir)) @@ -34,8 +35,14 @@ export function pickDirectoriesToEvict(input: EvictPlan) { const output: string[] = [] for (const dir of sorted) { const last = input.state.get(dir)?.lastAccessAt ?? 0 - const idle = input.now - last >= input.ttl + const age = input.now - last + const idle = age >= input.ttl if (!idle && pendingOverflow <= 0) continue + // A directory touched moments ago is almost certainly still mounted and + // merely waiting for its pin effect to run. Evicting it starts the + // recreate/bootstrap loop this grace window exists to prevent; going over + // the limit for a while is the cheaper failure. + if (!idle && age < graceMs) continue output.push(dir) if (pendingOverflow > 0) pendingOverflow -= 1 } diff --git a/packages/ui/src/sync/types.ts b/packages/ui/src/sync/types.ts index edb88e1a..22f1301a 100644 --- a/packages/ui/src/sync/types.ts +++ b/packages/ui/src/sync/types.ts @@ -96,6 +96,7 @@ export type EvictPlan = { pins: Set max: number ttl: number + graceMs?: number now: number hasPendingBlockingRequests?: (directory: string) => boolean } @@ -110,6 +111,18 @@ export type DisposeCheck = { } export const MAX_DIR_STORES = 30 +/** + * Directories touched within this window are never overflow-eviction victims. + * + * Sidebar rows call `ensureChild` during render but only take their pin in an + * effect after commit. Without a grace window, expanding a project with more + * worktrees than `MAX_DIR_STORES` evicted directories that were actively + * rendering, which recreated them, which issued another bootstrap request, in + * an endless loop (issue #1472). The limit is therefore a soft target: a burst + * of live directories overflows briefly rather than thrashing, and the cache is + * bounded by idle-time eviction instead. + */ +export const EVICTION_GRACE_MS = 30 * 1000 export const DIR_IDLE_TTL_MS = 20 * 60 * 1000 export const SESSION_CACHE_LIMIT = 40 diff --git a/scripts/profile-idle.mjs b/scripts/profile-idle.mjs index dbe47c14..98dc67a3 100644 --- a/scripts/profile-idle.mjs +++ b/scripts/profile-idle.mjs @@ -42,6 +42,9 @@ Options: --then-tab After settling, navigate to this tab without a reload, then record. Use it to measure what a surface keeps doing after the user leaves it. + --expand-projects Expand every project in the sidebar before + recording, which mounts a row per worktree and + session directory --panel Open the context panel on this surface before recording (chat, preview, terminal, git, pr, notes, file, diff, plan, context, browser, walkthrough). @@ -70,6 +73,7 @@ const parseArgs = (argv) => { tab: null, thenTab: null, panels: [], + expandProjects: false, duration: 30, settle: 15, output: null, @@ -94,6 +98,7 @@ const parseArgs = (argv) => { else if (value === "--tab") options.tab = argv[++index] else if (value === "--then-tab") options.thenTab = argv[++index] else if (value === "--panel") options.panels.push(argv[++index]) + else if (value === "--expand-projects") options.expandProjects = true else if (value === "--label") options.label = argv[++index] else if (value === "--duration") options.duration = Number(argv[++index]) else if (value === "--settle") options.settle = Number(argv[++index]) @@ -351,8 +356,16 @@ const main = async () => { await client.send("Page.navigate", { url: options.url }) await loaded - if (options.panels.length > 0) { - await seedContextPanel(client, options.panels, options.session) + if (options.expandProjects) { + // The sidebar persists the ids of collapsed projects, so an empty list + // expands everything. This is the state in issue #1472: one mounted + // directory-bound row per worktree and session. + await evaluateValue(client, `localStorage.setItem("oc.sessions.projectCollapse", "[]")`) + console.log("Expanded every project in the sidebar.") + } + + if (options.panels.length > 0 || options.expandProjects) { + if (options.panels.length > 0) await seedContextPanel(client, options.panels, options.session) const reloaded = client.once("Page.loadEventFired", 60_000) await client.send("Page.reload", { ignoreCache: false }) await reloaded