diff --git a/bun.lock b/bun.lock index e461f483904..1721dbf13e7 100644 --- a/bun.lock +++ b/bun.lock @@ -1078,6 +1078,7 @@ "devDependencies": { "@superset/typescript": "workspace:*", "@types/node": "^24.9.1", + "bun-types": "^1.3.1", "typescript": "^5.9.3", }, }, diff --git a/packages/host-service/package.json b/packages/host-service/package.json index 0ea876c7e00..d48953e4b65 100644 --- a/packages/host-service/package.json +++ b/packages/host-service/package.json @@ -50,6 +50,7 @@ "test:integration": "bun test --pass-with-no-tests test/integration", "test:integration:daemon": "node --experimental-strip-types --test src/terminal/DaemonClient/DaemonClient.node-test.ts src/daemon/DaemonSupervisor.node-test.ts", "test:e2e": "bun run scripts/test-e2e.ts", + "bench": "bun test test/integration/pull-requests-scaling.bench.ts", "typecheck": "tsc --noEmit --emitDeclarationOnly false" }, "dependencies": { diff --git a/packages/host-service/src/app.ts b/packages/host-service/src/app.ts index 8bb005de33f..47c0ce85a1e 100644 --- a/packages/host-service/src/app.ts +++ b/packages/host-service/src/app.ts @@ -7,7 +7,7 @@ import { Hono } from "hono"; import { cors } from "hono/cors"; import { createApiClient } from "./api"; import { createDb, type HostDb } from "./db"; -import { EventBus, registerEventBusRoute } from "./events"; +import { EventBus, GitWatcher, registerEventBusRoute } from "./events"; import type { ApiAuthProvider } from "./providers/auth"; import type { HostAuthProvider } from "./providers/host-auth"; import type { ModelProviderRuntimeResolver } from "./providers/model-providers"; @@ -76,13 +76,19 @@ export function createApp(options: CreateAppOptions): CreateAppResult { return new Octokit({ auth: token }); }); + const filesystem = new WorkspaceFilesystemManager({ db }); + // GitWatcher is the single source of truth for `.git/` and worktree fs + // activity per workspace. Both EventBus (broadcasts to clients) and the + // pull-requests runtime (event-driven branch sync) subscribe to it. + const gitWatcher = new GitWatcher(db, filesystem); + gitWatcher.start(); const pullRequestRuntime = new PullRequestRuntimeManager({ db, git, github, + gitWatcher, }); pullRequestRuntime.start(); - const filesystem = new WorkspaceFilesystemManager({ db }); const chatRuntime = options.chatRuntime ?? new ChatRuntimeManager({ @@ -111,7 +117,7 @@ export function createApp(options: CreateAppOptions): CreateAppResult { }), ); - const eventBus = new EventBus({ db, filesystem }); + const eventBus = new EventBus({ db, filesystem, gitWatcher }); eventBus.start(); // Backfill `kind='main'` v2 workspaces for projects already set up before @@ -180,6 +186,11 @@ export function createApp(options: CreateAppOptions): CreateAppResult { } catch (err) { console.warn("[host-service] eventBus.close failed:", err); } + try { + gitWatcher.close(); + } catch (err) { + console.warn("[host-service] gitWatcher.close failed:", err); + } if (ownsDb) { try { (db as unknown as { $client?: { close: () => void } }).$client?.close(); diff --git a/packages/host-service/src/events/event-bus.test.ts b/packages/host-service/src/events/event-bus.test.ts index 0d0f5cd2d92..60a4b617576 100644 --- a/packages/host-service/src/events/event-bus.test.ts +++ b/packages/host-service/src/events/event-bus.test.ts @@ -4,6 +4,7 @@ import type { HostDb } from "../db"; import { portManager } from "../ports/port-manager"; import type { WorkspaceFilesystemManager } from "../runtime/filesystem"; import { EventBus } from "./event-bus"; +import type { GitWatcher } from "./git-watcher"; function createEventBus(): EventBus { return new EventBus({ @@ -11,6 +12,9 @@ function createEventBus(): EventBus { filesystem: { resolveWorkspaceRoot: () => "/tmp/missing-workspace", } as unknown as WorkspaceFilesystemManager, + gitWatcher: { + onChanged: () => () => {}, + } as unknown as GitWatcher, }); } diff --git a/packages/host-service/src/events/event-bus.ts b/packages/host-service/src/events/event-bus.ts index 5d96c3cdd4f..a010a88f04e 100644 --- a/packages/host-service/src/events/event-bus.ts +++ b/packages/host-service/src/events/event-bus.ts @@ -6,7 +6,7 @@ import type { HostDb } from "../db/index.ts"; import { portManager } from "../ports/port-manager.ts"; import { getLabelsForWorkspace } from "../ports/static-ports.ts"; import type { WorkspaceFilesystemManager } from "../runtime/filesystem/index.ts"; -import { GitWatcher } from "./git-watcher.ts"; +import type { GitWatcher } from "./git-watcher.ts"; import type { ClientMessage, ServerMessage } from "./types.ts"; type WsSocket = { @@ -52,6 +52,7 @@ function parseClientMessage(data: unknown): ClientMessage | null { export interface EventBusOptions { db: HostDb; filesystem: WorkspaceFilesystemManager; + gitWatcher: GitWatcher; } /** @@ -71,13 +72,12 @@ export class EventBus { constructor(options: EventBusOptions) { this.filesystem = options.filesystem; - this.gitWatcher = new GitWatcher(options.db, options.filesystem); + this.gitWatcher = options.gitWatcher; } start(): void { if (this.removeGitListener || this.removePortListeners) return; - this.gitWatcher.start(); this.removeGitListener = this.gitWatcher.onChanged((event) => { this.broadcast({ type: "git:changed", @@ -105,7 +105,6 @@ export class EventBus { this.removeGitListener = null; this.removePortListeners?.(); this.removePortListeners = null; - this.gitWatcher.close(); for (const [socket, state] of this.clients) { this.cleanupClient(socket, state); } diff --git a/packages/host-service/src/events/index.ts b/packages/host-service/src/events/index.ts index fbe6b206c97..585bde04ada 100644 --- a/packages/host-service/src/events/index.ts +++ b/packages/host-service/src/events/index.ts @@ -1,4 +1,5 @@ export { EventBus, registerEventBusRoute } from "./event-bus.ts"; +export { type GitChangedEvent, GitWatcher } from "./git-watcher.ts"; export { type AgentLifecycleEventType, mapEventType, diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts index e74050c70e0..4280b20a990 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.test.ts @@ -148,6 +148,7 @@ function createManager(state: FakeState) { github: async () => { throw new Error("github should not be used for direct PR linking"); }, + gitWatcher: { onChanged: () => () => {} } as never, }); } diff --git a/packages/host-service/src/runtime/pull-requests/pull-requests.ts b/packages/host-service/src/runtime/pull-requests/pull-requests.ts index 728227f845e..522fec6f399 100644 --- a/packages/host-service/src/runtime/pull-requests/pull-requests.ts +++ b/packages/host-service/src/runtime/pull-requests/pull-requests.ts @@ -4,6 +4,7 @@ import { parseGitHubRemote } from "@superset/shared/github-remote"; import { and, eq, inArray } from "drizzle-orm"; import type { HostDb } from "../../db"; import { projects, pullRequests, workspaces } from "../../db/schema"; +import type { GitWatcher } from "../../events/git-watcher"; import type { GitFactory } from "../git"; import { fetchRepositoryPullRequests } from "./utils/github-query"; import type { GraphQLPullRequestNode } from "./utils/github-query/types"; @@ -22,9 +23,17 @@ import { type ReviewDecision, } from "./utils/pull-request-mappers"; -const BRANCH_SYNC_INTERVAL_MS = 30_000; -const PROJECT_REFRESH_INTERVAL_MS = 20_000; -// Must exceed every polling interval that hits this cache (BRANCH_SYNC and +// Long-cadence sweep that catches anything `GitWatcher` might miss +// (overflow, fs.watch errors, transient watcher failures). Steady-state +// branch syncs are event-driven via `GitWatcher.onChanged`; this is a +// belt-and-braces backup, not the primary path. +const SAFETY_NET_INTERVAL_MS = 5 * 60_000; +// Long-cadence safety net for project-level PR refresh. Steady-state +// refreshes are triggered by `syncOneWorkspace` whenever a workspace's +// branch/HEAD/upstream changes. The 60s repo-PR cache deduplicates across +// concurrent triggers. +const PROJECT_REFRESH_INTERVAL_MS = 5 * 60_000; +// Must exceed every polling interval that hits this cache (SAFETY_NET and // PROJECT_REFRESH). Otherwise the cache is always stale at poll time and // each tick fires a fresh GraphQL call per repo. Multiple projects can // target the same GitHub repo; this collapses them into one call per repo @@ -187,6 +196,7 @@ export interface PullRequestRuntimeManagerOptions { db: HostDb; git: GitFactory; github: () => Promise; + gitWatcher: GitWatcher; } interface NormalizedRepoIdentity { @@ -240,9 +250,15 @@ export class PullRequestRuntimeManager { private readonly db: HostDb; private readonly git: GitFactory; private readonly github: () => Promise; - private branchSyncTimer: ReturnType | null = null; + private readonly gitWatcher: GitWatcher; + private safetyNetTimer: ReturnType | null = null; private projectRefreshTimer: ReturnType | null = null; + private unsubscribeFromGitWatcher: (() => void) | null = null; private readonly inFlightProjects = new Map>(); + private readonly workspaceSyncState = new Map< + string, + { running: Promise; rerunPending: boolean } + >(); private readonly repoPullRequestCache = new Map< string, { promise: Promise; fetchedAt: number } @@ -252,27 +268,48 @@ export class PullRequestRuntimeManager { this.db = options.db; this.git = options.git; this.github = options.github; + this.gitWatcher = options.gitWatcher; } start() { - if (this.branchSyncTimer || this.projectRefreshTimer) return; + if ( + this.safetyNetTimer || + this.projectRefreshTimer || + this.unsubscribeFromGitWatcher + ) + return; + + // One initial sweep so workspaces that existed before this manager + // started have correct branch/sha/upstream rows even if no `.git/` + // activity has happened since the last process boot. + void this.syncWorkspaceBranches(); + void this.refreshEligibleProjects(); - this.branchSyncTimer = setInterval(() => { + // Steady-state: react to real `.git/` activity per workspace. Per-workspace + // debounce lives in `GitWatcher` (300 ms), and concurrent project refreshes + // are deduplicated by `inFlightProjects`. We additionally serialize per + // workspace so two debounce-separated bursts can't race their git reads + // and have the slower one overwrite the newer snapshot. + this.unsubscribeFromGitWatcher = this.gitWatcher.onChanged((event) => { + void this.enqueueWorkspaceSync(event.workspaceId); + }); + + // Long-cadence safety net for `GitWatcher` overflow / error paths. + this.safetyNetTimer = setInterval(() => { void this.syncWorkspaceBranches(); - }, BRANCH_SYNC_INTERVAL_MS); + }, SAFETY_NET_INTERVAL_MS); this.projectRefreshTimer = setInterval(() => { void this.refreshEligibleProjects(); }, PROJECT_REFRESH_INTERVAL_MS); - - void this.syncWorkspaceBranches(); - void this.refreshEligibleProjects(); } stop() { - if (this.branchSyncTimer) clearInterval(this.branchSyncTimer); + if (this.safetyNetTimer) clearInterval(this.safetyNetTimer); if (this.projectRefreshTimer) clearInterval(this.projectRefreshTimer); - this.branchSyncTimer = null; + this.unsubscribeFromGitWatcher?.(); + this.safetyNetTimer = null; this.projectRefreshTimer = null; + this.unsubscribeFromGitWatcher = null; } async getPullRequestsByWorkspaces( @@ -400,71 +437,122 @@ export class PullRequestRuntimeManager { } private async syncWorkspaceBranches(): Promise { - const allWorkspaces = this.db.select().from(workspaces).all(); - const changedProjectIds = new Set(); + // Route every workspace through the same per-workspace queue as the + // watcher path, so a concurrent watcher-triggered sync can't race the + // sweep's read+write and clobber the newer snapshot. enqueueWorkspaceSync + // coalesces — if a sync is already running for a workspace, this just + // flips its rerunPending flag. + const ids = this.db.select({ id: workspaces.id }).from(workspaces).all(); + + // Sequential to keep git subprocess concurrency bounded; matches the + // original sweep's behavior. refreshProject inside each sync still + // dedupes across workspaces in the same project via inFlightProjects. + for (const row of ids) { + await this.enqueueWorkspaceSync(row.id); + } + } - for (const workspace of allWorkspaces) { + private enqueueWorkspaceSync(workspaceId: string): Promise { + // Coalesce: if a sync is already running for this workspace, just mark + // "rerun pending" — there's no value in queuing N back-to-back syncs + // when only the final state matters. At most one sync runs and one + // rerun is queued, regardless of how many events fire. + const existing = this.workspaceSyncState.get(workspaceId); + if (existing) { + existing.rerunPending = true; + return existing.running; + } + + const run = async (): Promise => { try { - const git = await this.git(workspace.worktreePath); - const branch = await getCurrentBranchName(git); - if (!branch) { - continue; - } - const headSha = await getHeadSha(git); - const upstream = await resolveWorkspaceUpstream(git, branch); - const upstreamOwner = upstream?.owner ?? null; - const upstreamRepo = upstream?.name ?? null; - const upstreamBranch = upstream?.branch ?? null; - const pullRequestId = - upstream || - this.pullRequestHeadMatches(workspace.pullRequestId, headSha) - ? workspace.pullRequestId - : null; + do { + const state = this.workspaceSyncState.get(workspaceId); + if (state) state.rerunPending = false; + await this.syncOneWorkspace(workspaceId); + } while (this.workspaceSyncState.get(workspaceId)?.rerunPending); + } finally { + this.workspaceSyncState.delete(workspaceId); + } + }; - if ( - branch === workspace.branch && - headSha === workspace.headSha && - upstreamOwner === workspace.upstreamOwner && - upstreamRepo === workspace.upstreamRepo && - upstreamBranch === workspace.upstreamBranch && - pullRequestId === workspace.pullRequestId - ) { - continue; - } + const running = run(); + this.workspaceSyncState.set(workspaceId, { + running, + rerunPending: false, + }); + return running; + } - this.db - .update(workspaces) - .set({ - branch, - headSha, - upstreamOwner, - upstreamRepo, - upstreamBranch, - pullRequestId, - }) - .where(eq(workspaces.id, workspace.id)) - .run(); - - changedProjectIds.add(workspace.projectId); - } catch (error) { - console.warn( - "[host-service:pull-request-runtime] Failed to sync workspace branch", - { - workspaceId: workspace.id, - worktreePath: workspace.worktreePath, - error, - }, - ); + private async syncOneWorkspace(workspaceId: string): Promise { + // Look up the row fresh — the workspace may have been deleted between + // the GitWatcher event firing and this handler running. That's expected + // during teardown / workspace removal; silently no-op. + const workspace = this.db + .select() + .from(workspaces) + .where(eq(workspaces.id, workspaceId)) + .get(); + if (!workspace) return; + + const projectId = await this.syncWorkspaceRow(workspace); + if (projectId) await this.refreshProject(projectId); + } + + private async syncWorkspaceRow( + workspace: typeof workspaces.$inferSelect, + ): Promise { + try { + const git = await this.git(workspace.worktreePath); + const branch = await getCurrentBranchName(git); + if (!branch) return null; + + const headSha = await getHeadSha(git); + const upstream = await resolveWorkspaceUpstream(git, branch); + const upstreamOwner = upstream?.owner ?? null; + const upstreamRepo = upstream?.name ?? null; + const upstreamBranch = upstream?.branch ?? null; + const pullRequestId = + upstream || + this.pullRequestHeadMatches(workspace.pullRequestId, headSha) + ? workspace.pullRequestId + : null; + + if ( + branch === workspace.branch && + headSha === workspace.headSha && + upstreamOwner === workspace.upstreamOwner && + upstreamRepo === workspace.upstreamRepo && + upstreamBranch === workspace.upstreamBranch && + pullRequestId === workspace.pullRequestId + ) { + return null; } - } - // Branch changes use the shared 60s cache rather than bypassing it. - // The next refreshEligibleProjects tick will pick up newly-opened PRs; - // up to TTL_MS lag on attaching a brand-new external PR is acceptable - // and keeps high-churn workspaces from multiplying GraphQL traffic. - await Promise.all( - [...changedProjectIds].map((projectId) => this.refreshProject(projectId)), - ); + this.db + .update(workspaces) + .set({ + branch, + headSha, + upstreamOwner, + upstreamRepo, + upstreamBranch, + pullRequestId, + }) + .where(eq(workspaces.id, workspace.id)) + .run(); + + return workspace.projectId; + } catch (error) { + console.warn( + "[host-service:pull-request-runtime] Failed to sync workspace branch", + { + workspaceId: workspace.id, + worktreePath: workspace.worktreePath, + error, + }, + ); + return null; + } } private async refreshEligibleProjects(): Promise { diff --git a/packages/host-service/test/integration/pull-requests-scaling.bench.ts b/packages/host-service/test/integration/pull-requests-scaling.bench.ts new file mode 100644 index 00000000000..510fa6354ab --- /dev/null +++ b/packages/host-service/test/integration/pull-requests-scaling.bench.ts @@ -0,0 +1,249 @@ +import { afterEach, describe, test } from "bun:test"; +import { eq } from "drizzle-orm"; +import simpleGit, { type SimpleGit } from "simple-git"; +import type { HostDb } from "../../src/db"; +import { workspaces } from "../../src/db/schema"; +import { GitWatcher } from "../../src/events/git-watcher"; +import { WorkspaceFilesystemManager } from "../../src/runtime/filesystem"; +import { PullRequestRuntimeManager } from "../../src/runtime/pull-requests/pull-requests"; +import { createTestHost, type TestHost } from "../helpers/createTestHost"; +import { createGitFixture, type GitFixture } from "../helpers/git-fixture"; +import { seedProject, seedWorkspace } from "../helpers/seed"; + +/** + * BENCHMARK companion to `pull-requests-scaling.integration.test.ts`. + * + * Two measurements relevant after Fix #1 (event-driven branch sync): + * + * 1. **Event-to-DB-update latency** — wall-clock time from a real `git + * commit` until the workspaces.headSha row is updated. This is the new + * primary cost: paid only on real `.git/` activity, regardless of how + * many idle worktrees exist. + * + * 2. **Safety-net sweep cost** — wall-clock time for the long-cadence + * `syncWorkspaceBranches` call at N ∈ {1, 5, 20}. The sweep still does + * O(N) work *if it fires*, but now fires every 5 min instead of every + * 30 s, so daily wall-clock waste drops by 10×. + * + * Output goes through `console.log`; assertions are minimal so the + * benchmark doesn't fail on slow CI runners. + */ + +interface OpCounter { + count: number; +} + +function instrumentGit(realGit: SimpleGit, counter: OpCounter): SimpleGit { + return new Proxy(realGit, { + get(target, prop, receiver) { + if (prop === "raw" || prop === "revparse" || prop === "remote") { + return (args: string[]) => { + counter.count++; + // biome-ignore lint/suspicious/noExplicitAny: dispatching on a known SimpleGit method + return (target as any)[prop](args); + }; + } + return Reflect.get(target, prop, receiver); + }, + }); +} + +interface BenchScenario { + host: TestHost; + repos: GitFixture[]; + workspaceIds: string[]; + manager: PullRequestRuntimeManager; + gitWatcher: GitWatcher; + filesystem: WorkspaceFilesystemManager; + counter: OpCounter; + dispose: () => Promise; +} + +async function setup(workspaceCount: number): Promise { + const host = await createTestHost(); + const repos: GitFixture[] = []; + const workspaceIds: string[] = []; + + for (let i = 0; i < workspaceCount; i++) { + const repo = await createGitFixture(); + repos.push(repo); + const { id: projectId } = seedProject(host, { repoPath: repo.repoPath }); + const headSha = (await repo.git.revparse(["HEAD"])).trim(); + const { id } = seedWorkspace(host, { + projectId, + worktreePath: repo.repoPath, + branch: "main", + headSha, + }); + workspaceIds.push(id); + } + + const counter: OpCounter = { count: 0 }; + const filesystem = new WorkspaceFilesystemManager({ db: host.db as HostDb }); + const gitWatcher = new GitWatcher(host.db as HostDb, filesystem); + const manager = new PullRequestRuntimeManager({ + db: host.db as HostDb, + git: async (worktreePath: string) => + instrumentGit(simpleGit(worktreePath), counter), + github: async () => ({}) as never, + gitWatcher, + }); + + ( + manager as unknown as { refreshProject: () => Promise } + ).refreshProject = async () => undefined; + + const dispose = async () => { + manager.stop(); + gitWatcher.close(); + await filesystem.close(); + for (const repo of repos) repo.dispose(); + await host.dispose(); + }; + + return { + host, + repos, + workspaceIds, + manager, + gitWatcher, + filesystem, + counter, + dispose, + }; +} + +async function runSafetyNetTick(scenario: BenchScenario): Promise { + await ( + scenario.manager as unknown as { + syncWorkspaceBranches: () => Promise; + } + ).syncWorkspaceBranches(); +} + +async function waitFor( + predicate: () => boolean, + { timeoutMs = 10_000, pollMs = 25 } = {}, +): Promise { + const deadline = Date.now() + timeoutMs; + while (!predicate()) { + if (Date.now() > deadline) { + throw new Error("Timed out waiting for predicate"); + } + await new Promise((r) => setTimeout(r, pollMs)); + } +} + +describe("BENCH: pull-requests runtime — post-fix steady state", () => { + let scenarios: BenchScenario[] = []; + + afterEach(async () => { + await Promise.all(scenarios.map((s) => s.dispose())); + scenarios = []; + }); + + test("event-to-DB-update latency for a single git commit", async () => { + const scenario = await setup(5); + scenarios.push(scenario); + + scenario.gitWatcher.start(); + scenario.manager.start(); + + // Wait for initial sweep to settle. + await waitFor(() => scenario.counter.count >= 1, { timeoutMs: 10_000 }); + await new Promise((r) => setTimeout(r, 200)); + + const targetWorkspaceId = scenario.workspaceIds[2]; + const targetRepo = scenario.repos[2]; + if (!targetWorkspaceId || !targetRepo) throw new Error("missing target"); + + const expectedSha = ( + await targetRepo.commit("bench commit", { "bench.txt": "x" }) + ).trim(); + + const t0 = performance.now(); + await waitFor( + () => { + const row = scenario.host.db + .select({ headSha: workspaces.headSha }) + .from(workspaces) + .where(eq(workspaces.id, targetWorkspaceId)) + .get(); + return row?.headSha === expectedSha; + }, + { timeoutMs: 15_000, pollMs: 25 }, + ); + const latencyMs = performance.now() - t0; + + console.log("\n=== Event-to-DB-update latency ==="); + console.log( + `commit → workspaces.headSha update: ${latencyMs.toFixed(0)}ms`, + ); + console.log( + "(includes 300ms GitWatcher debounce + git subprocesses + sqlite write)", + ); + console.log("===\n"); + }, 60_000); + + test("safety-net sweep wall-clock for N ∈ {1, 5, 20}", async () => { + const sizes = [1, 5, 20]; + const rows: Array<{ + n: number; + warmupMs: number; + measuredMs: number; + ops: number; + msPerOp: number; + }> = []; + + for (const n of sizes) { + const scenario = await setup(n); + scenarios.push(scenario); + + // Warmup: first tick may pay JIT / disk-cache costs. + const t0 = performance.now(); + await runSafetyNetTick(scenario); + const warmupMs = performance.now() - t0; + void warmupMs; + + // Measured: second tick is the steady-state sweep cost. + scenario.counter.count = 0; + const t1 = performance.now(); + await runSafetyNetTick(scenario); + const measuredMs = performance.now() - t1; + const ops = scenario.counter.count; + + rows.push({ + n, + warmupMs: +warmupMs.toFixed(1), + measuredMs: +measuredMs.toFixed(1), + ops, + msPerOp: +(measuredMs / ops).toFixed(2), + }); + } + + console.log("\n=== Safety-net sweep wall-clock benchmark ==="); + console.log("N\twarmup ms\tsteady ms\tgit ops\tms/op\tprojected/5min tick"); + for (const r of rows) { + console.log( + `${r.n}\t${r.warmupMs}\t\t${r.measuredMs}\t\t${r.ops}\t${r.msPerOp}\t${r.measuredMs.toFixed(0)}ms / 5-min sweep`, + ); + } + + const last = rows[rows.length - 1]; + if (last) { + const msPerWorkspace = last.measuredMs / last.n; + const dailyTicks = (24 * 60) / 5; + console.log( + `\nExtrapolation @ ${msPerWorkspace.toFixed(1)} ms/workspace/sweep:`, + ); + for (const projN of [50, 100]) { + const projectedMs = msPerWorkspace * projN; + const projectedDailyMs = projectedMs * dailyTicks; + console.log( + ` N=${projN}: ~${projectedMs.toFixed(0)}ms/sweep × ${dailyTicks} sweeps/day = ~${(projectedDailyMs / 1000).toFixed(1)}s/day total sweep cost`, + ); + } + } + console.log("===\n"); + }, 60_000); +}); diff --git a/packages/host-service/test/integration/pull-requests-scaling.integration.test.ts b/packages/host-service/test/integration/pull-requests-scaling.integration.test.ts new file mode 100644 index 00000000000..a5028f1d9e0 --- /dev/null +++ b/packages/host-service/test/integration/pull-requests-scaling.integration.test.ts @@ -0,0 +1,221 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import simpleGit, { type SimpleGit } from "simple-git"; +import type { HostDb } from "../../src/db"; +import { GitWatcher } from "../../src/events/git-watcher"; +import { WorkspaceFilesystemManager } from "../../src/runtime/filesystem"; +import { PullRequestRuntimeManager } from "../../src/runtime/pull-requests/pull-requests"; +import { createTestHost, type TestHost } from "../helpers/createTestHost"; +import { createGitFixture, type GitFixture } from "../helpers/git-fixture"; +import { seedProject, seedWorkspace } from "../helpers/seed"; + +/** + * INTEGRATION coverage for the event-driven path of finding #1 in + * `plans/v2-paths-worktree-perf-findings.md`. + * + * Wires a real `GitWatcher` into the runtime, lets the initial sweep settle, + * then fires a single real `git commit` in one workspace. Asserts that ONLY + * that workspace's sync runs — the other N-1 stay quiet. This is the + * post-fix steady state: idle workspaces do zero git work. + * + * The safety-net sweep's per-workspace cost (linearity + always-walks-N) is + * covered by the mock-based unit test in + * `packages/host-service/test/pull-requests-scaling.test.ts`. Doing it here + * with real simple-git would just multiply the cost without adding signal, + * since the unit test already pins the shape. + */ + +interface GitOpLog { + worktreePath: string; + method: "raw" | "revparse" | "remote"; + args: string[]; +} + +function instrumentGit( + realGit: SimpleGit, + log: GitOpLog[], + worktreePath: string, +): SimpleGit { + return new Proxy(realGit, { + get(target, prop, receiver) { + if (prop === "raw" || prop === "revparse" || prop === "remote") { + return (args: string[]) => { + log.push({ + worktreePath, + method: prop as GitOpLog["method"], + args: [...args], + }); + // biome-ignore lint/suspicious/noExplicitAny: dispatching on a known SimpleGit method + return (target as any)[prop](args); + }; + } + return Reflect.get(target, prop, receiver); + }, + }); +} + +interface EventDrivenScenario { + host: TestHost; + repos: GitFixture[]; + workspaceIds: string[]; + gitOpLog: GitOpLog[]; + manager: PullRequestRuntimeManager; + gitWatcher: GitWatcher; + filesystem: WorkspaceFilesystemManager; + dispose: () => Promise; +} + +async function createEventDrivenScenario( + workspaceCount: number, +): Promise { + const host = await createTestHost(); + const repos: GitFixture[] = []; + const workspaceIds: string[] = []; + + for (let i = 0; i < workspaceCount; i++) { + const repo = await createGitFixture(); + repos.push(repo); + const { id: projectId } = seedProject(host, { repoPath: repo.repoPath }); + const headSha = (await repo.git.revparse(["HEAD"])).trim(); + const { id } = seedWorkspace(host, { + projectId, + worktreePath: repo.repoPath, + branch: "main", + headSha, + }); + workspaceIds.push(id); + } + + const gitOpLog: GitOpLog[] = []; + const filesystem = new WorkspaceFilesystemManager({ db: host.db as HostDb }); + const gitWatcher = new GitWatcher(host.db as HostDb, filesystem); + + const manager = new PullRequestRuntimeManager({ + db: host.db as HostDb, + git: async (worktreePath: string) => + instrumentGit(simpleGit(worktreePath), gitOpLog, worktreePath), + github: async () => ({}) as never, + gitWatcher, + }); + + ( + manager as unknown as { refreshProject: () => Promise } + ).refreshProject = async () => undefined; + + const dispose = async () => { + manager.stop(); + gitWatcher.close(); + await filesystem.close(); + for (const repo of repos) repo.dispose(); + await host.dispose(); + }; + + return { + host, + repos, + workspaceIds, + gitOpLog, + manager, + gitWatcher, + filesystem, + dispose, + }; +} + +async function waitFor( + predicate: () => boolean, + { timeoutMs = 5000, pollMs = 50 } = {}, +): Promise { + const deadline = Date.now() + timeoutMs; + while (!predicate()) { + if (Date.now() > deadline) { + throw new Error("Timed out waiting for predicate"); + } + await new Promise((r) => setTimeout(r, pollMs)); + } +} + +/** + * Wait until `getValue()` stops growing for `quietMs` consecutive ms — i.e. + * the system has settled. We need this when the initial sweep + concurrent + * GitWatcher debounce flushes are still trickling in: a fixed-time sleep + * after `start()` might snapshot mid-flush, leaving leftover ops to count + * against later assertions. + */ +async function waitUntilQuiet( + getValue: () => number, + { quietMs = 750, timeoutMs = 15_000, pollMs = 50 } = {}, +): Promise { + const deadline = Date.now() + timeoutMs; + let lastValue = getValue(); + let lastChangeAt = Date.now(); + while (Date.now() - lastChangeAt < quietMs) { + if (Date.now() > deadline) { + throw new Error("Timed out waiting for system to quiesce"); + } + await new Promise((r) => setTimeout(r, pollMs)); + const current = getValue(); + if (current !== lastValue) { + lastValue = current; + lastChangeAt = Date.now(); + } + } +} + +describe("PullRequestRuntimeManager event-driven steady state", () => { + let scenarios: EventDrivenScenario[] = []; + + afterEach(async () => { + await Promise.all(scenarios.map((s) => s.dispose())); + scenarios = []; + }); + + test("git:changed in one workspace triggers a single-workspace sync, not a full sweep", async () => { + // 3 worktrees is enough to prove "only the target's worktree got ops" — + // the other 2 must stay quiet. Larger N just multiplies setup cost. + const scenario = await createEventDrivenScenario(3); + scenarios.push(scenario); + + scenario.gitWatcher.start(); + scenario.manager.start(); + + // Wait until the initial sweep AND any startup-related GitWatcher + // events have fully drained — otherwise we'd snapshot mid-flush and + // see leftover ops from another workspace counted as "event-driven". + await waitFor(() => scenario.gitOpLog.length > 0, { timeoutMs: 10_000 }); + await waitUntilQuiet(() => scenario.gitOpLog.length, { + quietMs: 1_000, + timeoutMs: 15_000, + }); + const baselineLogLength = scenario.gitOpLog.length; + + // Commit in one workspace only. + const targetIndex = 1; + const targetRepo = scenario.repos[targetIndex]; + if (!targetRepo) throw new Error("missing target repo"); + await targetRepo.commit("event-driven change", { + "event.txt": "trigger", + }); + + // GitWatcher debounces 300 ms; wait for sync to fire and then settle. + await waitFor(() => scenario.gitOpLog.length > baselineLogLength, { + timeoutMs: 10_000, + }); + await waitUntilQuiet(() => scenario.gitOpLog.length, { + quietMs: 1_000, + timeoutMs: 10_000, + }); + + const eventDrivenOps = scenario.gitOpLog.slice(baselineLogLength); + const touchedWorktrees = new Set( + eventDrivenOps.map((op) => op.worktreePath), + ); + + // Only the target workspace should have been synced. + expect(touchedWorktrees.size).toBe(1); + expect(touchedWorktrees.has(targetRepo.repoPath)).toBe(true); + + console.log( + `[event-driven] commit in 1/${scenario.repos.length} workspaces → ${eventDrivenOps.length} git ops touching ${touchedWorktrees.size} worktree`, + ); + }, 30_000); +}); diff --git a/packages/host-service/test/pull-requests-scaling.test.ts b/packages/host-service/test/pull-requests-scaling.test.ts new file mode 100644 index 00000000000..601da07f0d8 --- /dev/null +++ b/packages/host-service/test/pull-requests-scaling.test.ts @@ -0,0 +1,201 @@ +import { describe, expect, mock, test } from "bun:test"; +import { PullRequestRuntimeManager } from "../src/runtime/pull-requests/pull-requests"; + +/** + * Pins the cost of the **safety-net sweep** that runs every + * `SAFETY_NET_INTERVAL_MS` (5 min) — the long-cadence backup for the + * event-driven `GitWatcher` subscription. When `syncWorkspaceBranches` + * runs, it still walks every workspace and spawns ~5 git subprocesses + * for each one. The fix from finding #1 doesn't change the safety-net + * sweep's cost; it only ensures that path runs at 5 min instead of 30 s. + * + * The steady-state idle behavior — zero git ops when nothing changed in + * any `.git/` directory — is covered by + * `pull-requests-scaling.integration.test.ts`. + */ + +interface RawCallLog { + worktreePath: string; + args: string[]; +} + +function buildWorkspace(index: number) { + return { + id: `ws-${index}`, + projectId: `project-${index}`, + worktreePath: `/tmp/worktree-${index}`, + // Match what the git mock will return so syncWorkspaceBranches treats + // every workspace as unchanged. This is the realistic steady-state: + // nothing changed, but we still pay full git-subprocess cost per tick. + branch: "main", + headSha: "deadbeef", + upstreamOwner: "acme", + upstreamRepo: "repo", + upstreamBranch: "main", + pullRequestId: null, + createdAt: Date.now(), + }; +} + +function buildGitMock(rawCalls: RawCallLog[], worktreePath: string) { + const recordingRaw = mock(async (args: string[]) => { + rawCalls.push({ worktreePath, args }); + + // symbolic-ref --short HEAD → branch name + if (args[0] === "symbolic-ref") return "main\n"; + + // rev-parse --abbrev-ref BRANCH@{push} → push ref + if (args[0] === "rev-parse" && args[1] === "--abbrev-ref") { + return "origin/main\n"; + } + + // remote get-url + if (args[0] === "remote" && args[1] === "get-url") { + return "https://github.com/acme/repo.git\n"; + } + + // config --get + if (args[0] === "config") { + return ""; + } + + throw new Error(`Unexpected raw args: ${args.join(" ")}`); + }); + + return { + raw: recordingRaw, + revparse: mock(async (args: string[]) => { + rawCalls.push({ worktreePath, args: ["revparse", ...args] }); + if (args[0] === "HEAD") return "deadbeef\n"; + throw new Error(`Unexpected revparse args: ${args.join(" ")}`); + }), + remote: mock(async (args: string[]) => { + rawCalls.push({ worktreePath, args: ["remote", ...args] }); + if (args[0] === "get-url") return "https://github.com/acme/repo.git\n"; + throw new Error(`Unexpected remote args: ${args.join(" ")}`); + }), + }; +} + +async function runSync(workspaceCount: number) { + const workspaces = Array.from({ length: workspaceCount }, (_, i) => + buildWorkspace(i), + ); + + const rawCalls: RawCallLog[] = []; + + const workspacesById = new Map(workspaces.map((w) => [w.id, w])); + const db = { + select: mock(() => ({ + from: mock(() => ({ + all: mock(() => workspaces), + })), + })), + // syncWorkspaceBranches only writes when state changed; nothing should change here. + update: mock(() => { + throw new Error("update should not be called when state is unchanged"); + }), + }; + + const gitFactoryCalls: string[] = []; + const git = mock(async (worktreePath: string) => { + gitFactoryCalls.push(worktreePath); + return buildGitMock(rawCalls, worktreePath); + }); + + const manager = new PullRequestRuntimeManager({ + db: db as never, + git: git as never, + github: async () => ({}) as never, + gitWatcher: { onChanged: () => () => {} } as never, + }); + + // `syncWorkspaceBranches` calls `refreshProject` only for changed projects; + // stub it to a no-op so the test focuses purely on per-workspace git cost. + ( + manager as unknown as { + refreshProject: () => Promise; + } + ).refreshProject = mock(async () => undefined); + + // The sweep now routes through enqueueWorkspaceSync → syncOneWorkspace, + // which re-reads each workspace via `select().from().where().get()`. + // Bypass the drizzle .where() chain (awkward to mock) by feeding rows + // from our local map; syncWorkspaceRow still drives the real git work. + ( + manager as unknown as { + syncOneWorkspace: (id: string) => Promise; + } + ).syncOneWorkspace = async (id: string) => { + const workspace = workspacesById.get(id); + if (!workspace) return; + await ( + manager as unknown as { + syncWorkspaceRow: ( + w: ReturnType, + ) => Promise; + } + ).syncWorkspaceRow(workspace); + }; + + await ( + manager as unknown as { syncWorkspaceBranches: () => Promise } + ).syncWorkspaceBranches(); + + return { rawCalls, gitFactoryCalls }; +} + +describe("syncWorkspaceBranches safety-net sweep — worktree-scaling", () => { + test("git subprocess count grows linearly with workspace count", async () => { + const small = await runSync(2); + const large = await runSync(20); + + // One git factory invocation per workspace per tick + expect(small.gitFactoryCalls.length).toBe(2); + expect(large.gitFactoryCalls.length).toBe(20); + + // Each workspace issues the same fixed number of git ops on an unchanged + // repo (branch lookup + HEAD + push-ref + remote URL). The exact count + // is implementation-defined; what we assert is *linearity*: the cost + // for 20 workspaces is 10× the cost for 2. + const perWorkspaceSmall = small.rawCalls.length / 2; + const perWorkspaceLarge = large.rawCalls.length / 20; + expect(perWorkspaceSmall).toBe(perWorkspaceLarge); + + // Per-workspace cost is non-trivial — at least a branch lookup, HEAD, + // and push-ref resolution. If this drops below 3 the runtime probably + // dropped some git work and this scaling concern is partially fixed. + expect(perWorkspaceSmall).toBeGreaterThanOrEqual(3); + + // Print the actual per-tick cost so the test output documents the + // scaling factor for future readers. + console.log( + `[scaling] per-tick git ops: 2 workspaces=${small.rawCalls.length}, 20 workspaces=${large.rawCalls.length}, per-workspace=${perWorkspaceSmall}`, + ); + }); + + test("safety-net sweep calls all N git factories even when zero workspaces changed", async () => { + // The safety-net sweep still walks every workspace — that's its job. + // What changed in finding #1 is the **cadence**: this used to fire every + // 30s; now it fires every 5 min, and the steady-state per-workspace + // sync runs only on real `.git/` activity. + const { gitFactoryCalls, rawCalls } = await runSync(10); + + expect(gitFactoryCalls.length).toBe(10); + expect(new Set(gitFactoryCalls).size).toBe(10); + expect(rawCalls.length).toBeGreaterThanOrEqual(30); // ≥3 ops × 10 workspaces + + // Each workspace got its share of the work — no batching, no shortcut. + const callsByWorktree = new Map(); + for (const call of rawCalls) { + callsByWorktree.set( + call.worktreePath, + (callsByWorktree.get(call.worktreePath) ?? 0) + 1, + ); + } + expect(callsByWorktree.size).toBe(10); + for (const count of callsByWorktree.values()) { + expect(count).toBeGreaterThanOrEqual(3); + } + }); +}); diff --git a/packages/host-service/test/pull-requests.test.ts b/packages/host-service/test/pull-requests.test.ts index 164c9acb714..dd3f05520d8 100644 --- a/packages/host-service/test/pull-requests.test.ts +++ b/packages/host-service/test/pull-requests.test.ts @@ -50,12 +50,30 @@ describe("PullRequestRuntimeManager branch sync", () => { db: db as never, git: git as never, github: async () => ({}) as never, + gitWatcher: { onChanged: () => () => {} } as never, }); const refreshProjectMock = mock(async () => undefined); ( manager as unknown as { refreshProject: typeof refreshProjectMock } ).refreshProject = refreshProjectMock; + // The sweep now routes through enqueueWorkspaceSync → syncOneWorkspace, + // which re-reads each workspace via `select().from().where().get()`. + // Bypass the drizzle .where() chain (awkward to mock) by feeding the + // known row directly; syncWorkspaceRow still runs the production logic. + ( + manager as unknown as { + syncOneWorkspace: (id: string) => Promise; + } + ).syncOneWorkspace = async () => { + const projectId = await ( + manager as unknown as { + syncWorkspaceRow: (w: typeof workspace) => Promise; + } + ).syncWorkspaceRow(workspace); + if (projectId) await refreshProjectMock(projectId); + }; + await ( manager as unknown as { syncWorkspaceBranches: () => Promise } ).syncWorkspaceBranches(); @@ -111,11 +129,29 @@ describe("PullRequestRuntimeManager branch sync", () => { db: db as never, git: git as never, github: async () => ({}) as never, + gitWatcher: { onChanged: () => () => {} } as never, }); const refreshProjectMock = mock(async () => undefined); ( manager as unknown as { refreshProject: typeof refreshProjectMock } ).refreshProject = refreshProjectMock; + + // The sweep now routes through enqueueWorkspaceSync → syncOneWorkspace, + // which re-reads each workspace via `select().from().where().get()`. + // Bypass the drizzle .where() chain (awkward to mock) by feeding the + // known row directly; syncWorkspaceRow still runs the production logic. + ( + manager as unknown as { + syncOneWorkspace: (id: string) => Promise; + } + ).syncOneWorkspace = async () => { + const projectId = await ( + manager as unknown as { + syncWorkspaceRow: (w: typeof workspace) => Promise; + } + ).syncWorkspaceRow(workspace); + if (projectId) await refreshProjectMock(projectId); + }; const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); await ( @@ -124,6 +160,7 @@ describe("PullRequestRuntimeManager branch sync", () => { expect(setMock).not.toHaveBeenCalled(); expect(refreshProjectMock).not.toHaveBeenCalled(); - expect(warnSpy).toHaveBeenCalled(); + // Pin to the sync-failure path so an unrelated console.warn can't pass. + expect(warnSpy.mock.calls[0]?.[0]).toContain("Failed to sync workspace"); }); }); diff --git a/packages/workspace-fs/package.json b/packages/workspace-fs/package.json index b982f1d4845..9093915e96f 100644 --- a/packages/workspace-fs/package.json +++ b/packages/workspace-fs/package.json @@ -27,7 +27,8 @@ }, "scripts": { "typecheck": "tsc --noEmit --emitDeclarationOnly false", - "test": "bun test --pass-with-no-tests" + "test": "bun test --pass-with-no-tests", + "bench": "bun test src/cache-and-paths-memory.bench.ts" }, "dependencies": { "@parcel/watcher": "^2.5.6", @@ -36,6 +37,7 @@ "devDependencies": { "@superset/typescript": "workspace:*", "@types/node": "^24.9.1", + "bun-types": "^1.3.1", "typescript": "^5.9.3" } } diff --git a/packages/workspace-fs/src/bun-test.d.ts b/packages/workspace-fs/src/bun-test.d.ts deleted file mode 100644 index 6200aca8cbe..00000000000 --- a/packages/workspace-fs/src/bun-test.d.ts +++ /dev/null @@ -1,18 +0,0 @@ -declare module "bun:test" { - export function afterEach(callback: () => void | Promise): void; - - export function describe( - name: string, - callback: () => void | Promise, - ): void; - - export function it(name: string, callback: () => void | Promise): void; - - export function expect(actual: T): { - toContain(expected: unknown): void; - toEqual(expected: unknown): void; - toHaveLength(expected: number): void; - toBeNull(): void; - toBeTruthy(): void; - }; -} diff --git a/packages/workspace-fs/src/cache-and-paths-memory.bench.ts b/packages/workspace-fs/src/cache-and-paths-memory.bench.ts new file mode 100644 index 00000000000..8322b35208d --- /dev/null +++ b/packages/workspace-fs/src/cache-and-paths-memory.bench.ts @@ -0,0 +1,219 @@ +import { afterEach, describe, test } from "bun:test"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { getSearchIndex, invalidateAllSearchIndexes } from "./search"; +import { FsWatcherManager } from "./watch"; + +/** + * BENCHMARK companions to the integration tests for findings #2 and #3. + * + * Quotes real heap deltas (via `Bun.gc(true)` + `process.memoryUsage`) so + * memory creep claims are anchored to measured RSS-adjacent numbers, not + * just structural arguments ("entries persist forever, therefore memory + * grows"). Output goes through `console.log`; assertions are minimal so + * the benchmark doesn't fail on noisy machines. + * + * Caveat: these measure JS heap, not RSS. Native FSEvents/parcel allocations + * outside the JS heap don't show up here. For RSS you'd want `process.rss` + * at the OS level, which is noisy and often dominated by other allocations. + */ + +interface HeapSample { + heapMb: number; +} + +async function gcAndSample(): Promise { + // Multiple GC passes with microtask yields between — single Bun.gc(true) + // can leave incremental work pending; double-pumping gets us a stable + // reading for benchmark output. + if (typeof Bun !== "undefined" && typeof Bun.gc === "function") { + Bun.gc(true); + await new Promise((resolve) => setTimeout(resolve, 20)); + Bun.gc(true); + } + const used = process.memoryUsage().heapUsed; + return { heapMb: +(used / 1024 / 1024).toFixed(2) }; +} + +const tempRoots: string[] = []; + +afterEach(async () => { + invalidateAllSearchIndexes(); + await Promise.all( + tempRoots.splice(0).map(async (rootPath) => { + await fs.rm(rootPath, { recursive: true, force: true }); + }), + ); +}); + +async function createWorktreeWith( + fileCount: number, + prefix = "bench-cache-", +): Promise { + const tempPath = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + const rootPath = await fs.realpath(tempPath); + tempRoots.push(rootPath); + + for (let i = 0; i < fileCount; i++) { + await fs.writeFile( + path.join(rootPath, `file-${i}.ts`), + `export const value${i} = ${i};\n// padding to make a realistic file size\nconst _x${i} = "abcdef";\n`, + ); + } + + return rootPath; +} + +describe("BENCH: searchIndexCache heap delta vs N worktrees", () => { + test("prints heap MB after building 5/25/100 worktree indexes", async () => { + const stages = [ + { count: 5, filesPerWorktree: 200 }, + { count: 25, filesPerWorktree: 200 }, + { count: 100, filesPerWorktree: 200 }, + ]; + + console.log("\n=== searchIndexCache heap benchmark ==="); + console.log("worktrees\tfiles/wt\theap MB\tdelta MB\ttotal entries"); + + const baseline = await gcAndSample(); + console.log(`baseline\t-\t\t${baseline.heapMb}\t-\t\t-`); + + let cumulativeWorktrees = 0; + let cumulativeEntries = 0; + let prevHeap = baseline.heapMb; + + for (const stage of stages) { + for (let i = 0; i < stage.count; i++) { + const root = await createWorktreeWith( + stage.filesPerWorktree, + `bench-cache-${cumulativeWorktrees + i}-`, + ); + const index = await getSearchIndex({ + rootPath: root, + includeHidden: false, + }); + cumulativeEntries += index.length; + } + cumulativeWorktrees += stage.count; + + const sample = await gcAndSample(); + console.log( + `${cumulativeWorktrees}\t\t${stage.filesPerWorktree}\t\t${sample.heapMb}\t+${(sample.heapMb - prevHeap).toFixed(2)}\t\t${cumulativeEntries}`, + ); + prevHeap = sample.heapMb; + } + + const final = await gcAndSample(); + console.log( + `\nTotal heap delta: +${(final.heapMb - baseline.heapMb).toFixed(2)} MB for ${cumulativeWorktrees} cached worktree indexes (${cumulativeEntries} entries)`, + ); + console.log( + `Per-worktree: ~${((final.heapMb - baseline.heapMb) / cumulativeWorktrees).toFixed(3)} MB`, + ); + console.log( + `Per-entry: ~${(((final.heapMb - baseline.heapMb) * 1024) / cumulativeEntries).toFixed(2)} KB`, + ); + + // Confirm cache holds: invalidating frees memory. + invalidateAllSearchIndexes(); + const afterInvalidate = await gcAndSample(); + console.log( + `After invalidateAllSearchIndexes: ${afterInvalidate.heapMb} MB (freed ${(final.heapMb - afterInvalidate.heapMb).toFixed(2)} MB)`, + ); + console.log("===\n"); + }, 60_000); +}); + +describe("BENCH: pathTypes heap delta vs unique paths", () => { + test("prints heap MB after creating 1k/5k/20k unique paths in one worktree", async () => { + const tempPath = await fs.mkdtemp( + path.join(os.tmpdir(), "bench-pathtypes-"), + ); + const rootPath = await fs.realpath(tempPath); + tempRoots.push(rootPath); + + const manager = new FsWatcherManager({ debounceMs: 50 }); + let createCount = 0; + const unsubscribe = await manager.subscribe( + { absolutePath: rootPath, recursive: true }, + (batch) => { + for (const event of batch.events) { + if (event.kind === "create") createCount++; + } + }, + ); + + interface ManagerInternal { + watchers: Map< + string, + { + filePaths: Map; + directoryPaths: Set; + } + >; + } + const getPathTypesSize = (): number => { + const internal = manager as unknown as ManagerInternal; + const state = internal.watchers.get(rootPath); + if (!state) return 0; + return state.filePaths.size + state.directoryPaths.size; + }; + + const stages = [1_000, 5_000, 20_000]; + + console.log("\n=== pathTypes heap benchmark ==="); + console.log( + "unique paths\tcreate events\tpathTypes size\theap MB\tdelta MB", + ); + + const baseline = await gcAndSample(); + console.log(`baseline\t-\t\t-\t\t${baseline.heapMb}\t-`); + + let prevHeap = baseline.heapMb; + let totalCreated = 0; + + for (const target of stages) { + const toCreate = target - totalCreated; + for (let i = 0; i < toCreate; i++) { + await fs.writeFile( + path.join(rootPath, `unique-${totalCreated + i}.tmp`), + `${totalCreated + i}`, + ); + } + totalCreated = target; + + // Wait for parcel watcher to catch up. We don't need every event + // to flush — we just want pathTypes to reflect the bulk. Once + // past the 10k file cap the size plateaus, so we cap the + // predicate target to avoid spinning the deadline. + const FILE_PATHS_MAX = 10_000; + const expectedSize = Math.min(target, FILE_PATHS_MAX); + const deadline = Date.now() + 30_000; + while ( + getPathTypesSize() < expectedSize * 0.95 && + Date.now() < deadline + ) { + await new Promise((resolve) => setTimeout(resolve, 200)); + } + + const sample = await gcAndSample(); + console.log( + `${target}\t\t${createCount}\t\t${getPathTypesSize()}\t\t${sample.heapMb}\t+${(sample.heapMb - prevHeap).toFixed(2)}`, + ); + prevHeap = sample.heapMb; + } + + const final = await gcAndSample(); + console.log( + `\nTotal heap delta: +${(final.heapMb - baseline.heapMb).toFixed(2)} MB for ${getPathTypesSize()} pathTypes entries`, + ); + console.log( + `Per 10k paths: ~${(((final.heapMb - baseline.heapMb) * 10_000) / Math.max(getPathTypesSize(), 1)).toFixed(2)} MB`, + ); + console.log("===\n"); + + await unsubscribe(); + await manager.close(); + }, 120_000); +}); diff --git a/packages/workspace-fs/src/search-cache-eviction.test.ts b/packages/workspace-fs/src/search-cache-eviction.test.ts new file mode 100644 index 00000000000..f5ba4e24e5c --- /dev/null +++ b/packages/workspace-fs/src/search-cache-eviction.test.ts @@ -0,0 +1,159 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { getSearchIndex, invalidateAllSearchIndexes } from "./search"; + +/** + * Verifies finding #2's fix from `plans/v2-paths-worktree-perf-fix-plan.md`: + * + * `searchIndexCache` now has a hard LRU cap (`SEARCH_INDEX_CACHE_MAX = 12`) + * plus a 30-min idle TTL. These tests confirm: + * - active worktrees stay cached (LRU bump on access) + * - exceeding the cap evicts least-recently-used entries + * - a worktree touched once and abandoned eventually loses its index + * + * The cap is intentionally small (~12) because realistic concurrent search + * activity rarely spans more than a handful of worktrees. Cold-search latency + * after eviction is one fast-glob walk (~50–200 ms for a 5k-file repo). + */ + +// Must match SEARCH_INDEX_CACHE_MAX in search.ts. If you change the constant, +// update this and the assertions below. +const CACHE_MAX = 12; + +const tempRoots: string[] = []; + +afterEach(async () => { + invalidateAllSearchIndexes(); + await Promise.all( + tempRoots.splice(0, tempRoots.length).map(async (rootPath) => { + await fs.rm(rootPath, { recursive: true, force: true }); + }), + ); +}); + +async function createTempWorktree(fileCount: number): Promise { + const rootPath = await fs.mkdtemp( + path.join(os.tmpdir(), "workspace-fs-cache-"), + ); + tempRoots.push(rootPath); + + for (let i = 0; i < fileCount; i++) { + await fs.writeFile( + path.join(rootPath, `file-${i}.ts`), + `export const value = ${i};\n`, + ); + } + + return rootPath; +} + +describe("searchIndexCache LRU eviction", () => { + it("retains up to CACHE_MAX recently-used indexes", async () => { + // Build exactly CACHE_MAX indexes — none should be evicted. + const roots: string[] = []; + for (let i = 0; i < CACHE_MAX; i++) { + roots.push(await createTempWorktree(3)); + } + + const firstReads: Awaited>[] = []; + for (const root of roots) { + firstReads.push( + await getSearchIndex({ rootPath: root, includeHidden: false }), + ); + } + + // Re-read in REVERSE order so the first-built entry isn't auto-promoted. + // All entries should still be live (size === CACHE_MAX, none evicted). + for (let i = roots.length - 1; i >= 0; i--) { + const root = roots[i]; + if (!root) throw new Error("missing root"); + const cached = await getSearchIndex({ + rootPath: root, + includeHidden: false, + }); + const expected = firstReads[i]; + if (!expected) throw new Error("missing first read"); + expect(cached).toBe(expected); + } + }); + + it("evicts the least-recently-used entry when adding a (CACHE_MAX+1)th worktree", async () => { + // Build CACHE_MAX worktrees in order; entry 0 is the oldest. + const roots: string[] = []; + const initialReads: Awaited>[] = []; + for (let i = 0; i < CACHE_MAX; i++) { + const root = await createTempWorktree(2); + roots.push(root); + initialReads.push( + await getSearchIndex({ rootPath: root, includeHidden: false }), + ); + } + + // Add one more worktree — this should evict entry 0 (oldest). + const extraRoot = await createTempWorktree(2); + await getSearchIndex({ rootPath: extraRoot, includeHidden: false }); + + const root0 = roots[0]; + if (!root0) throw new Error("missing root"); + const refetchedRoot0 = await getSearchIndex({ + rootPath: root0, + includeHidden: false, + }); + + // Entry 0 was evicted, then rebuilt — different array reference. + expect(refetchedRoot0).not.toBe(initialReads[0]); + expect(refetchedRoot0.length).toBe(2); + }); + + it("LRU bump on access — touching the oldest keeps it alive", async () => { + const roots: string[] = []; + const initialReads: Awaited>[] = []; + for (let i = 0; i < CACHE_MAX; i++) { + const root = await createTempWorktree(2); + roots.push(root); + initialReads.push( + await getSearchIndex({ rootPath: root, includeHidden: false }), + ); + } + + // Touch entry 0 — bumps it to MRU. Entry 1 becomes LRU. + const root0 = roots[0]; + if (!root0) throw new Error("missing root"); + await getSearchIndex({ rootPath: root0, includeHidden: false }); + + // Add a new worktree — entry 1 should be evicted, not entry 0. + const extraRoot = await createTempWorktree(2); + await getSearchIndex({ rootPath: extraRoot, includeHidden: false }); + + // Entry 0 still cached. + const reread0 = await getSearchIndex({ + rootPath: root0, + includeHidden: false, + }); + const expected0 = initialReads[0]; + if (!expected0) throw new Error("missing initial read"); + expect(reread0).toBe(expected0); + + // Entry 1 evicted. + const root1 = roots[1]; + if (!root1) throw new Error("missing root"); + const reread1 = await getSearchIndex({ + rootPath: root1, + includeHidden: false, + }); + expect(reread1).not.toBe(initialReads[1]); + }); + + it("explicit invalidation still works", async () => { + const rootPath = await createTempWorktree(2); + const before = await getSearchIndex({ rootPath, includeHidden: false }); + + invalidateAllSearchIndexes(); + + const after = await getSearchIndex({ rootPath, includeHidden: false }); + expect(after).not.toBe(before); + expect(after.length).toBe(before.length); + }); +}); diff --git a/packages/workspace-fs/src/search.ts b/packages/workspace-fs/src/search.ts index 05b0da45cb8..6810965c858 100644 --- a/packages/workspace-fs/src/search.ts +++ b/packages/workspace-fs/src/search.ts @@ -97,9 +97,30 @@ export interface SearchContentOptions { ) => Promise<{ stdout: string }>; } -const searchIndexCache = new Map(); +// LRU + idle-TTL on the index cache: bound JS heap as worktree count grows. +// Inactive worktrees pay a fresh fast-glob walk on next search (~50–200 ms +// for a 5k-file repo) — cheap relative to keeping every index resident. +const SEARCH_INDEX_CACHE_MAX = 12; +const SEARCH_INDEX_CACHE_TTL_MS = 30 * 60_000; + +interface CachedIndex { + items: SearchIndexEntry[]; + lastAccessedAt: number; +} + +const searchIndexCache = new Map(); const searchIndexBuilds = new Map>(); +function evictLruSearchIndexEntries(): void { + // Map iteration is insertion-order; re-inserting on hit moves an entry to + // the end, so the first key is least-recently-used. + while (searchIndexCache.size >= SEARCH_INDEX_CACHE_MAX) { + const oldestKey = searchIndexCache.keys().next().value; + if (!oldestKey) break; + searchIndexCache.delete(oldestKey); + } +} + function createSearchIndexEntry( rootPath: string, relativePath: string, @@ -276,7 +297,14 @@ export async function getSearchIndex( const cached = searchIndexCache.get(cacheKey); if (cached) { - return cached; + // TTL is the freshness contract — bypassing it on hits would let a hot + // key serve indefinitely-stale data. Memory is already bounded by LRU. + searchIndexCache.delete(cacheKey); + if (Date.now() - cached.lastAccessedAt <= SEARCH_INDEX_CACHE_TTL_MS) { + cached.lastAccessedAt = Date.now(); + searchIndexCache.set(cacheKey, cached); // re-insert at MRU position + return cached.items; + } } const inFlight = searchIndexBuilds.get(cacheKey); @@ -286,7 +314,11 @@ export async function getSearchIndex( const buildPromise = buildSearchIndex(options) .then((items) => { - searchIndexCache.set(cacheKey, items); + evictLruSearchIndexEntries(); + searchIndexCache.set(cacheKey, { + items, + lastAccessedAt: Date.now(), + }); searchIndexBuilds.delete(cacheKey); return items; }) @@ -701,7 +733,7 @@ export function patchSearchIndexesForRoot( } const nextItemsByPath = new Map( - cached.map((item) => [item.absolutePath, item]), + cached.items.map((item) => [item.absolutePath, item]), ); for (const event of events) { applySearchPatchEvent({ @@ -712,7 +744,12 @@ export function patchSearchIndexesForRoot( }); } - searchIndexCache.set(cacheKey, Array.from(nextItemsByPath.values())); + // Patches imply the worktree is alive — bump to MRU and refresh access time. + searchIndexCache.delete(cacheKey); + searchIndexCache.set(cacheKey, { + items: Array.from(nextItemsByPath.values()), + lastAccessedAt: Date.now(), + }); } } diff --git a/packages/workspace-fs/src/watch-pathtypes-growth.test.ts b/packages/workspace-fs/src/watch-pathtypes-growth.test.ts new file mode 100644 index 00000000000..8b6c37b8015 --- /dev/null +++ b/packages/workspace-fs/src/watch-pathtypes-growth.test.ts @@ -0,0 +1,294 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { FsWatcherManager } from "./watch"; + +/** + * INTEGRATION reproduction of finding #3 in + * `plans/v2-paths-worktree-perf-findings.md`. + * + * `WatcherState.pathTypes` (private to `FsWatcherManager`) accumulates one + * entry per path that has been seen via `create` / `update` / `rename` events. + * Only `delete` events remove entries. New file paths created over time + * (logs, build artifacts that escape DEFAULT_IGNORE_PATTERNS, generated + * assets, dev-server tmp files) leak in monotonically. + * + * Uses a real `FsWatcherManager` with the real `@parcel/watcher` backend and + * real fs writes. Reaches into private `watchers` map after events flush + * to assert pathTypes growth — the same pattern other tests in this repo + * use to inspect manager-internal state. + */ + +interface WatcherStateView { + filePaths: Map; + directoryPaths: Set; +} + +interface FsWatcherManagerInternal { + watchers: Map; +} + +const tempRoots: string[] = []; +const managers: FsWatcherManager[] = []; + +afterEach(async () => { + // Close managers first (releases parcel subscriptions and frees the dir + // handles) so fs.rm doesn't race a live watcher. Tests don't bother with + // inline cleanup — if an assert throws, this still runs. + await Promise.all(managers.splice(0).map((m) => m.close())); + await Promise.all( + tempRoots + .splice(0) + .map((rootPath) => fs.rm(rootPath, { recursive: true, force: true })), + ); +}); + +async function createTempRoot(): Promise { + const tempPath = await fs.mkdtemp(path.join(os.tmpdir(), "watch-pathtypes-")); + // Resolve symlinks (e.g. macOS /var → /private/var) so absolute paths + // inside `pathTypes` match what we compute in the test. + return fs.realpath(tempPath); +} + +function createManager(options?: { debounceMs?: number }): FsWatcherManager { + const manager = new FsWatcherManager(options); + managers.push(manager); + return manager; +} + +function getPathTypes( + manager: FsWatcherManager, + rootPath: string, +): Map { + const internal = manager as unknown as FsWatcherManagerInternal; + const state = internal.watchers.get(rootPath); + if (!state) { + throw new Error(`No WatcherState for ${rootPath}`); + } + const merged = new Map(); + for (const filePath of state.filePaths.keys()) merged.set(filePath, false); + for (const dirPath of state.directoryPaths) merged.set(dirPath, true); + return merged; +} + +function getFilePathsSize(manager: FsWatcherManager, rootPath: string): number { + const internal = manager as unknown as FsWatcherManagerInternal; + const state = internal.watchers.get(rootPath); + if (!state) { + throw new Error(`No WatcherState for ${rootPath}`); + } + return state.filePaths.size; +} + +async function waitForCondition( + check: () => boolean, + timeoutMs = 4000, + pollMs = 50, +): Promise { + const deadline = Date.now() + timeoutMs; + while (!check()) { + if (Date.now() > deadline) { + throw new Error("Timed out waiting for watcher condition"); + } + await new Promise((resolve) => setTimeout(resolve, pollMs)); + } +} + +describe("FsWatcherManager.pathTypes — monotonic growth", () => { + it("creating N new files adds N entries to pathTypes", async () => { + const rootPath = await createTempRoot(); + tempRoots.push(rootPath); + + const manager = createManager({ debounceMs: 50 }); + const events: string[] = []; + await manager.subscribe( + { absolutePath: rootPath, recursive: true }, + (batch) => { + for (const event of batch.events) { + if (event.kind !== "overflow") { + events.push(`${event.kind}:${event.absolutePath}`); + } + } + }, + ); + + const fileCount = 12; + for (let i = 0; i < fileCount; i++) { + await fs.writeFile(path.join(rootPath, `gen-${i}.log`), `${i}\n`); + } + + await waitForCondition(() => events.length >= fileCount); + + const pathTypes = getPathTypes(manager, rootPath); + expect(pathTypes.size).toBeGreaterThanOrEqual(fileCount); + + // Each tracked path is one of the files we created. + for (let i = 0; i < fileCount; i++) { + expect(pathTypes.has(path.join(rootPath, `gen-${i}.log`))).toBe(true); + } + }); + + it("updates to existing files do not grow pathTypes", async () => { + const rootPath = await createTempRoot(); + tempRoots.push(rootPath); + + const manager = createManager({ debounceMs: 50 }); + const events: string[] = []; + await manager.subscribe( + { absolutePath: rootPath, recursive: true }, + (batch) => { + for (const event of batch.events) { + if (event.kind !== "overflow") events.push(event.kind); + } + }, + ); + + // Create one file, wait for it to be tracked. + const filePath = path.join(rootPath, "stable.txt"); + await fs.writeFile(filePath, "v1"); + await waitForCondition(() => events.includes("create")); + + const sizeAfterCreate = getPathTypes(manager, rootPath).size; + + // Now mutate the same file 10 times. pathTypes should not grow. + for (let i = 0; i < 10; i++) { + await fs.writeFile(filePath, `v${i + 2}`); + } + await waitForCondition( + () => events.filter((k) => k === "update").length >= 1, + ); + + const sizeAfterUpdates = getPathTypes(manager, rootPath).size; + expect(sizeAfterUpdates).toBe(sizeAfterCreate); + }); + + it("delete events remove entries; create-new events add fresh ones", async () => { + const rootPath = await createTempRoot(); + tempRoots.push(rootPath); + + const manager = createManager({ debounceMs: 50 }); + const events: string[] = []; + await manager.subscribe( + { absolutePath: rootPath, recursive: true }, + (batch) => { + for (const event of batch.events) { + if (event.kind !== "overflow") { + events.push(`${event.kind}:${event.absolutePath}`); + } + } + }, + ); + + // Phase 1: create 5 files. + for (let i = 0; i < 5; i++) { + await fs.writeFile(path.join(rootPath, `phase1-${i}.txt`), "x"); + } + await waitForCondition( + () => events.filter((e) => e.startsWith("create:")).length >= 5, + ); + + const sizeAfterPhase1 = getPathTypes(manager, rootPath).size; + expect(sizeAfterPhase1).toBeGreaterThanOrEqual(5); + + // Phase 2: delete those 5 files. pathTypes should shrink. + for (let i = 0; i < 5; i++) { + await fs.rm(path.join(rootPath, `phase1-${i}.txt`)); + } + await waitForCondition( + () => events.filter((e) => e.startsWith("delete:")).length >= 5, + ); + + const sizeAfterDeletes = getPathTypes(manager, rootPath).size; + expect(sizeAfterDeletes).toBeLessThan(sizeAfterPhase1); + + // Phase 3: create 5 NEW files with different names. pathTypes grows + // again. This is the leak shape: log rotation / dev-server tmp / + // hashed build artifacts produce a stream of unique paths whose + // older deletes happen sometime — but in the meantime pathTypes + // climbs and climbs. + for (let i = 0; i < 5; i++) { + await fs.writeFile(path.join(rootPath, `phase3-${i}.txt`), "x"); + } + await waitForCondition( + () => events.filter((e) => e.startsWith("create:")).length >= 10, + ); + + const sizeAfterPhase3 = getPathTypes(manager, rootPath).size; + expect(sizeAfterPhase3).toBeGreaterThanOrEqual(sizeAfterDeletes + 5); + }); + + it("caps pathTypes at PATH_TYPES_MAX (10k) — older entries evicted on overflow", async () => { + // After Fix #3 lands, creating 10k+ unique files should plateau at the + // cap rather than growing without bound. Slightly above the cap (10,200) + // to exercise eviction without slowing the test more than necessary. + const rootPath = await createTempRoot(); + tempRoots.push(rootPath); + + const manager = createManager({ debounceMs: 50 }); + await manager.subscribe( + { absolutePath: rootPath, recursive: true }, + () => {}, + ); + + const PATH_TYPES_MAX = 10_000; + const total = PATH_TYPES_MAX + 200; + + for (let i = 0; i < total; i++) { + await fs.writeFile(path.join(rootPath, `cap-${i}.tmp`), `${i}`); + } + + // Poll on the actual eviction outcome rather than the event count — + // hitting 95% of events doesn't strictly imply the cap was exceeded + // (could land at 9_690/10_000 and stall under coalesced delivery). + const firstPath = path.join(rootPath, "cap-0.tmp"); + await waitForCondition( + () => !getPathTypes(manager, rootPath).has(firstPath), + 60_000, + ); + + // File entries are the LRU-capped axis; directories are tracked + // separately and aren't counted toward the cap. + const cappedFileSize = getFilePathsSize(manager, rootPath); + expect(cappedFileSize).toBeLessThanOrEqual(PATH_TYPES_MAX); + + // Earliest paths (cap-0..cap-199) should have been evicted. + expect(getPathTypes(manager, rootPath).has(firstPath)).toBe(false); + + // Most-recent paths should still be in the map. + const lastPath = path.join(rootPath, `cap-${total - 1}.tmp`); + expect(getPathTypes(manager, rootPath).has(lastPath)).toBe(true); + }, 120_000); + + it("repeated create/delete with unique names grows pathTypes monotonically until delete catches up", async () => { + // The most realistic leak scenario: a process keeps creating files + // with NEW unique names (think rotating logs, hashed build outputs). + // Even if old files eventually get cleaned up, the *peak* size of + // pathTypes during the watcher's lifetime is unbounded — there's + // no LRU or size cap to keep it from spiking. + const rootPath = await createTempRoot(); + tempRoots.push(rootPath); + + const manager = createManager({ debounceMs: 50 }); + let createCount = 0; + await manager.subscribe( + { absolutePath: rootPath, recursive: true }, + (batch) => { + for (const event of batch.events) { + if (event.kind === "create") createCount++; + } + }, + ); + + // Burst: create 30 unique paths before any delete fires. Without + // a cap, pathTypes holds all 30 simultaneously. + const totalUnique = 30; + for (let i = 0; i < totalUnique; i++) { + await fs.writeFile(path.join(rootPath, `unique-${i}.tmp`), `${i}`); + } + await waitForCondition(() => createCount >= totalUnique); + + const peakSize = getPathTypes(manager, rootPath).size; + expect(peakSize).toBeGreaterThanOrEqual(totalUnique); + }); +}); diff --git a/packages/workspace-fs/src/watch.ts b/packages/workspace-fs/src/watch.ts index ed72525eebc..42422429739 100644 --- a/packages/workspace-fs/src/watch.ts +++ b/packages/workspace-fs/src/watch.ts @@ -15,6 +15,15 @@ import { } from "./search"; import type { FsWatchEvent } from "./types"; +// Cap per-watcher file-path memory so a monotonic stream of unique paths +// (log rotation, hashed build artifacts) doesn't grow JS heap unbounded. +// Directories are tracked separately and uncapped — directory count per +// worktree is bounded by repo structure (O(100s) even for huge repos), and +// losing a directory hint causes a delete event to fall back to file-only +// search-index pruning, leaving stale descendant entries until the next +// full rebuild. +const FILE_PATHS_MAX = 10_000; + export interface WatchPathOptions { absolutePath: string; recursive?: boolean; @@ -33,7 +42,8 @@ interface WatcherState { absolutePath: string; subscription: AsyncSubscription; listeners: Set; - pathTypes: Map; + filePaths: Map; + directoryPaths: Set; pendingEvents: ParcelWatcherEvent[]; flushTimer: ReturnType | null; } @@ -368,7 +378,8 @@ export class FsWatcherManager { absolutePath: normalizeAbsolutePath(absolutePath), subscription: null as unknown as AsyncSubscription, listeners: new Set(), - pathTypes: new Map(), + filePaths: new Map(), + directoryPaths: new Set(), pendingEvents: [], flushTimer: null, }; @@ -450,9 +461,13 @@ export class FsWatcherManager { return; } - const internalEvents = await Promise.all( - coalescedEvents.map((event) => this.normalizeEvent(state, event)), - ); + // Sequential so LRU mutations land in event order, not stat-completion + // order. Batches are small (debounced ~75 ms) and stat is fast on a + // warm fs, so the parallelism wasn't worth the eviction nondeterminism. + const internalEvents: InternalWatchEvent[] = []; + for (const event of coalescedEvents) { + internalEvents.push(await this.normalizeEvent(state, event)); + } const reconciledEvents = reconcileRenameEvents(internalEvents); const searchPatchEvents = reconciledEvents @@ -469,17 +484,32 @@ export class FsWatcherManager { event: ParcelWatcherEvent, ): Promise { const absolutePath = normalizeAbsolutePath(event.path); - let isDirectory = state.pathTypes.get(absolutePath) ?? false; + let isDirectory = state.directoryPaths.has(absolutePath); if (event.type === "delete") { - state.pathTypes.delete(absolutePath); + state.filePaths.delete(absolutePath); + state.directoryPaths.delete(absolutePath); } else { try { const stats = await stat(absolutePath); isDirectory = stats.isDirectory(); - state.pathTypes.set(absolutePath, isDirectory); + if (isDirectory) { + // Directories are uncapped (bounded by repo structure). + state.directoryPaths.add(absolutePath); + state.filePaths.delete(absolutePath); + } else { + // LRU bump + evict oldest file when at cap. Map iteration is + // insertion-order, so the first key is least-recently-used. + state.filePaths.delete(absolutePath); + if (state.filePaths.size >= FILE_PATHS_MAX) { + const oldestKey = state.filePaths.keys().next().value; + if (oldestKey) state.filePaths.delete(oldestKey); + } + state.filePaths.set(absolutePath, true); + state.directoryPaths.delete(absolutePath); + } } catch { - isDirectory = state.pathTypes.get(absolutePath) ?? false; + isDirectory = state.directoryPaths.has(absolutePath); } } diff --git a/packages/workspace-fs/tsconfig.json b/packages/workspace-fs/tsconfig.json index 840bf8c9aaf..2b801a37647 100644 --- a/packages/workspace-fs/tsconfig.json +++ b/packages/workspace-fs/tsconfig.json @@ -1,5 +1,8 @@ { "extends": "@superset/typescript/internal-package.json", + "compilerOptions": { + "types": ["bun-types"] + }, "include": ["src"], "exclude": ["node_modules", "dist"] } diff --git a/plans/v2-paths-worktree-perf-findings.md b/plans/v2-paths-worktree-perf-findings.md new file mode 100644 index 00000000000..a8f18dcd843 --- /dev/null +++ b/plans/v2-paths-worktree-perf-findings.md @@ -0,0 +1,166 @@ +# V2 paths — worktree-scaling perf audit + +**Branch:** `v2-paths-worktree-perf` +**Date:** 2026-05-02 + +The user observed: "adding worktrees should not add overhead, but it does." This audit walks the v2 code paths to find the work that scales with worktree count, **especially over time** (boot cost is acceptable; recurring or monotonically-growing cost is not). + +Each big finding has a reproduction test under the corresponding package's `test/` directory. + +--- + +## Steady-state scaling — ranked + +### 🔴 1. `syncWorkspaceBranches` — O(N) git subprocesses every 30s, forever + +**Location:** `packages/host-service/src/runtime/pull-requests/pull-requests.ts:221-228, 306-365` + +A `setInterval` (`BRANCH_SYNC_INTERVAL_MS = 30_000`) fires `syncWorkspaceBranches`, which iterates `db.select().from(workspaces).all()` and for **every** workspace spawns ~5–7 `git` subprocesses to detect branch / HEAD / upstream changes: + +| Step | Helper | git call | +| ---- | ------ | -------- | +| Branch name | `getCurrentBranchName` (lines 41–55) | `git symbolic-ref --short HEAD` (or fallback `rev-parse --abbrev-ref HEAD`) | +| HEAD SHA | `getHeadSha` (lines 57–75) | `git rev-parse HEAD` | +| Upstream (push) | `resolveWorkspaceUpstream` (lines 91–137) | `git rev-parse --abbrev-ref BRANCH@{push}` + `git remote get-url …` | +| Upstream (fallback) | same | `git config --get branch.X.merge`, `branch.X.pushRemote` / `remote.pushDefault` / `branch.X.remote` | + +**Why it bites over time:** the work runs every 30s **regardless of whether anything changed**. Most ticks find unchanged state and exit at line 322–331 — pure waste. + +**Cost arithmetic (measured, not extrapolated):** + +Wall-clock per tick from `pull-requests-scaling.bench.test.ts` on the dev machine: + +| N worktrees | git ops/tick | wall-clock/tick | ms/op | +| ----------: | -----------: | --------------: | ----: | +| 1 | 4 | 74ms | 18.6 | +| 5 | 20 | 419ms | 20.9 | +| 20 | 80 | 1542ms | 19.3 | + +Linear in N, ~19 ms per real `simple-git` subprocess (fork/exec/IPC dominant). At N=20 the runtime is burning ~1.5 seconds of pure CPU every 30s tick on subprocess overhead alone — ~3% of every clock cycle for nothing. Extrapolated: N=50 ≈ 3.9 s/tick (~13% of every 30s window), N=100 ≈ 7.7 s/tick (~26%). + +**Why it's redundant:** `GitWatcher` (`packages/host-service/src/events/git-watcher.ts:42-86, 234-237`) already watches `.git/` recursively per workspace and emits a debounced `git:changed` event with the workspaceId. Branch / HEAD / upstream changes always touch `.git/`. **Verified:** `pull-requests.ts` does not subscribe to `GitWatcher.onChanged` (no `git:changed` reference in that runtime). + +**Fix shape:** subscribe `pull-requests` to `GitWatcher.onChanged`. Re-derive branch only for the workspace whose `.git/` actually changed. Keep a 5-min sweep as safety net rather than the primary signal. + +**Reproduction test:** `packages/host-service/test/pull-requests-scaling.test.ts` — proves git invocations grow linearly with workspace count. + +--- + +### 🔴 2. `searchIndexCache` — monotonic memory growth, no eviction + +**Location:** `packages/workspace-fs/src/search.ts:19, 100, 287-299, 659-674` + +```ts +// No TTL — index is kept current via patchSearchIndexesForRoot from file watcher +const searchIndexCache = new Map(); +``` + +The cache key is `${rootPath}::${includeHidden}`. Once populated for a workspace, **the entry lives for the lifetime of the host-service process.** No LRU, no TTL, no idle eviction — the only paths that remove entries are explicit `invalidateSearchIndex*` calls (overflow handler in `watch.ts:408`, or external invalidation after rename / config changes). + +**Why it bites over time:** every worktree the user touches contributes a full file list (`SearchIndexEntry` per source file, ~kB each in JS). After a week, N worktrees × file count per worktree sits in resident memory. + +**Measured (`cache-and-paths-memory.bench.test.ts`):** 130 cached worktree indexes × 200 files each = 26,000 cached entries → **+6.87 MB heap** (~53 KB per worktree, ~0.27 KB per entry). Calling `invalidateAllSearchIndexes` frees ~5.4 MB, confirming the entries are the load. Repos with realistic file counts (5k–10k files) would multiply this by ~25×. + +**Why it's also a CPU hazard:** the file watcher's overflow path (`watch.ts:399-410`) calls `invalidateSearchIndexesForRoot` and the next access does another full `fast-glob('**/*')`. Overflow is more likely as worktrees share parent directories and FSEvents queues saturate, so the cache becomes a "build it, lose it, rebuild" cycle under heavy churn. + +**Fix shape:** cap with an LRU (8–16 most-recent worktrees) **or** drop entries after K minutes of no access. The "kept current via patches" claim is true for the active worktree; idle workspaces don't need their full file list resident. + +**Reproduction test:** `packages/workspace-fs/src/search-cache-no-eviction.test.ts` — proves the cache holds 50 distinct indexes simultaneously without auto-eviction. + +--- + +### 🔴 3. `FsWatcherManager.pathTypes` — monotonic per-watcher map growth + +**Location:** `packages/workspace-fs/src/watch.ts:36, 371, 472-482` + +Each `WatcherState` carries a `Map` of every path the watcher has seen. The state is updated in `applyDirectoryHint`: + +```ts +if (next.kind === "delete") { + state.pathTypes.delete(absolutePath); +} else { + state.pathTypes.set(absolutePath, isDirectory); +} +``` + +**Why it bites over time:** every `create` / `update` / `rename` event adds the path to the map; only `delete` events remove. New files that don't get cleaned up (logs, build artifacts that escape `DEFAULT_IGNORE_PATTERNS`, generated assets, dev-server tmp files, sourcemap rotations) accumulate forever. The map is per-worktree, so total leak ≈ N worktrees × new-paths-touched-per-day. + +`DEFAULT_IGNORE_PATTERNS` (search.ts:28-36) covers `node_modules`, `.git`, `dist`, `build`, `.next`, `.turbo`, `coverage` — but not `.cache`, `tmp`, `logs`, app-specific build outputs, etc. + +**Measured (`cache-and-paths-memory.bench.test.ts`):** 20,000 unique paths in a single watcher's `pathTypes` map → **+8.69 MB heap** (~4.3 MB per 10k entries, ~430 bytes per entry). Per-worktree, with active dev servers / log rotation churning unique filenames, easily reaches 10k+ paths/day. With 20 worktrees, that's ~85 MB/day of pathTypes-only growth, never reclaimed unless deletes catch up exactly. + +**Fix shape:** bound `pathTypes` (LRU 10k entries per watcher) or scrub it on the debounce flush — the directory-type hint is only needed for the immediate event window, not the entire watcher lifetime. + +**Reproduction test:** `packages/workspace-fs/src/watch-pathtypes-growth.test.ts` — uses a real `FsWatcherManager` + real `@parcel/watcher` + real fs writes. Reaches into the manager's private `watchers` map (same `as unknown as` pattern other tests use) to assert `pathTypes` grows monotonically with `create` events, stays flat on `update`, shrinks on `delete`, and peaks at N when N unique paths are created in a burst. + +--- + +### 🟡 4. `refreshEligibleProjects` — small but constant ticking + +**Location:** `pull-requests.ts:224-226, 367-378` + +Every 20s: scan all workspaces, dedupe to projects, fan out `refreshProject(projectId)`. The 60s GraphQL cache (`REPO_PULL_REQUEST_CACHE_TTL_MS = 60_000`, line 32, after fix in commit 5291207fc) means most ticks are network no-ops. + +**Why it still costs:** the DB scan + `Promise.all` orchestration runs every 20s regardless of activity. Bounded by unique project count, not worktree count, so it scales weakly with N. + +**Fix shape:** once #1 is event-driven (a real branch change triggers a targeted `refreshProject`), this tick can drop to a 5-min freshness floor instead of 20s. + +--- + +### 🟡 5. Active worktrees compound file-event traffic + +Not strictly "scales with N worktrees" but worth flagging. A parcel watcher per worktree means each running dev server / build / log writer in a different worktree produces its own debounce flush stream. With 20 worktrees and 5 of them running dev servers, host CPU wakes up per worktree every 75–300ms. + +**Fix shape:** **lazy** GitWatcher registration — only register watchers for workspaces with active subscribers. Today, `GitWatcher.start()` registers watchers for every workspace in the DB regardless of whether anyone is listening. Background worktrees should stop generating event traffic when nothing is listening. + +--- + +## Re-derated (boot-only, not steady-state) + +These look bad at first glance but only hurt once per session and are not the user's "over time" pain: + +- **Search-index pre-warm in `getServiceForRootPath`** (`packages/host-service/src/runtime/filesystem/filesystem.ts:65`) — pay once per workspace, cached afterwards. Boot-only. +- **`useDiffStats` fan-out at sidebar mount** (`apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsx:42`) — N concurrent `git.getStatus` IPC calls when the sidebar mounts. Once mounted, only refetches on real `git:changed` for its own workspace. Boot/mount-only. +- **Renderer-side polling** — every `refetchInterval` in `apps/desktop/src/renderer` lives under `v2-workspace/$workspaceId/...` (only fires for the active workspace) or is global (auth, host-service health). None fan out per worktree. + +--- + +## Not actually a bug + +- **`usePortsData`** (`apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/PortsList/hooks/usePortsData.ts:18, 23`) — earlier review claimed `ports.getAll(undefined)` / `ports.subscribe(undefined)` was passing invalid input. Verified false: `apps/desktop/src/lib/trpc/routers/ports/ports.ts:16, 28` defines both as `publicProcedure.query(...)` / `publicProcedure.subscription(...)` with no `.input()`. Passing `undefined` is correct. +- **`FsWatcherManager` multiplexing** (`packages/workspace-fs/src/watch.ts:318-351`) — multiple subscribers to the same path share one native watcher. Working as intended. +- **`useWorkspaceFileEventBridge`** (`apps/desktop/src/renderer/screens/main/components/WorkspaceView/hooks/useWorkspaceFileEvents/useWorkspaceFileEvents.ts:106-117`) — gated on `listenerCount > 0`. Lazy and correct. + +--- + +## Recommended fix order + +1. **Wire `pull-requests` runtime into `GitWatcher.onChanged`** and kill the 30s `syncWorkspaceBranches` polling. Single change, biggest CPU win. +2. **Cap `searchIndexCache` (LRU + TTL).** ~10 lines, kills the memory creep. +3. **Bound `pathTypes` per watcher.** Memory fix. +4. **Make `GitWatcher` lazy-register watchers** based on subscriber refcount (the `bus.watchFs` pattern already exists for `fs:events` in `apps/desktop/src/renderer/hooks/host-service/useWorkspaceEvent/useWorkspaceEvent.ts:73-83` — generalize it). Bigger refactor, biggest steady-state win on multi-worktree machines. +5. **Loosen `refreshEligibleProjects` to a 5-min safety net** once #1 is event-driven. + +After these, host-service idle CPU + RSS should be roughly flat regardless of worktree count. + +--- + +## Reproduction tests + +All three findings have integration-level tests that exercise real subsystems (real git subprocesses, real `fast-glob`, real `@parcel/watcher`) — no fakes for the component under test. + +| Finding | Test file | Style | What it asserts | +| ------- | --------- | ----- | --------------- | +| #1 syncWorkspaceBranches scales O(N) | `packages/host-service/test/pull-requests-scaling.test.ts` | unit (mocked db + git) | per-tick git call count = N × 4; idle ticks pay full cost | +| #1 syncWorkspaceBranches scales O(N) | `packages/host-service/test/integration/pull-requests-scaling.integration.test.ts` | **integration** — real bun:sqlite DB, real git repos via `createGitFixture`, real `simple-git` subprocesses (only `GitFactory` boundary instrumented for counting) | per-tick git call count = N × 4 with real subprocesses; second idle tick pays the same cost as the first | +| #2 searchIndexCache never evicts | `packages/workspace-fs/src/search-cache-no-eviction.test.ts` | **integration** — real fs, real `fast-glob` walks | after building 50 indexes the first one stays cached; even after 100 newer indexes the first array reference is still returned (no LRU); only `invalidateAllSearchIndexes` removes entries | +| #3 pathTypes growth | `packages/workspace-fs/src/watch-pathtypes-growth.test.ts` | **integration** — real `FsWatcherManager` + real `@parcel/watcher` + real fs writes | `pathTypes.size` grows ≥N when N unique files are created; stays flat on updates to existing files; shrinks on deletes; spikes back up when new unique paths arrive (the "log rotation" leak shape) | + +### Benchmarks (real wall-clock + heap) + +These print measurements; assertions are minimal so they don't fail on noisy CI runners. Run them when you want hard numbers, not as part of every test loop. + +| Benchmark | Measures | +| --------- | -------- | +| `packages/host-service/test/integration/pull-requests-scaling.bench.test.ts` | wall-clock ms per `syncWorkspaceBranches` tick at N ∈ {1, 5, 20} with real `simple-git` subprocesses | +| `packages/workspace-fs/src/cache-and-paths-memory.bench.test.ts` | JS heap delta (via `Bun.gc(true)` + `process.memoryUsage`) for `searchIndexCache` at 5/30/130 worktrees and `pathTypes` at 1k/5k/20k unique paths | diff --git a/plans/v2-paths-worktree-perf-fix-plan.md b/plans/v2-paths-worktree-perf-fix-plan.md new file mode 100644 index 00000000000..b1873e0f3e2 --- /dev/null +++ b/plans/v2-paths-worktree-perf-fix-plan.md @@ -0,0 +1,355 @@ +# V2 paths — worktree-scaling perf fix plan + +**Branch:** `v2-paths-worktree-perf` +**Date:** 2026-05-02 +**Companion doc:** [`v2-paths-worktree-perf-findings.md`](./v2-paths-worktree-perf-findings.md) + +This plan addresses the steady-state worktree-scaling costs identified in the findings audit. The goal: host-service idle CPU and JS heap should be roughly **flat** as worktree count grows, not linear. + +Each fix has a verification step against the existing reproduction tests / benchmarks. After all fixes land, those benchmarks should show the post-fix numbers cited in the "target" rows. + +--- + +## Current state + +- **Branch:** `v2-paths-worktree-perf`. +- **All 4 in-scope fixes landed.** Fix #5 remains deferred (measure post-merge before re-scoping). +- **Suite status:** `packages/host-service` 460/460 passing including the new event-driven steady-state integration test. `packages/workspace-fs` 43/43 passing. + +--- + +## Fix order + +| # | Fix | Severity | Effort | Where | Status | +|---|-----|----------|--------|-------|--------| +| 1 | Event-driven `pull-requests` runtime via `GitWatcher.onChanged` | 🔴 CRITICAL | Medium | `packages/host-service` | ✅ landed | +| 2 | LRU + idle-TTL cap on `searchIndexCache` | 🔴 IMPORTANT | Small | `packages/workspace-fs` | ✅ landed | +| 3 | LRU cap on per-watcher `pathTypes` | 🔴 IMPORTANT | Small | `packages/workspace-fs` | ✅ landed | +| 4 | Loosen `refreshEligibleProjects` to 5-min safety net | 🟡 LOW | Trivial | `packages/host-service` | ✅ landed | +| 5 | (Deferred) Lazy GitWatcher registration | ⚪ DEFER | Large | `packages/host-service` | deferred | + +### Measured impact of landed fixes + +Workspace-fs (`cache-and-paths-memory.bench.test.ts`): + +| Metric | Before | After | Reduction | +|--------|--------|-------|-----------| +| Heap @ 130 cached worktree indexes | +6.87 MB | +2.02 MB | 71% | +| Heap @ 20k unique paths in `pathTypes` | +8.69 MB | +2.54 MB | 71% | +| `pathTypes.size` @ 20k unique paths | 20,000 | 10,000 (capped) | hard cap | +| `searchIndexCache` retained entries @ 130 worktrees | 130 (linear) | 12 (cap) | hard cap | + +Host-service pull-requests runtime (`pull-requests-scaling.bench.test.ts`): + +| Metric | Before | After | +|--------|--------|-------| +| Idle tick @ N=20 | 1450 ms / 30 s = **48 ms/s** of git-subprocess waste | 0 ms (no idle ticks) | +| Real branch change → DB update latency | ≤ 30 s | **427 ms** (one measurement, N=5) | +| Safety-net sweep cadence | every 30 s | every 5 min | +| Daily safety-net cost @ N=20 | 1450 ms × 2880 ticks/day = **70 min/day** | 1450 ms × 288 sweeps/day = **7 min/day** | + +The big shift: idle worktrees now cost 0 git subprocesses. Branch change latency dropped from 30 s p99 to ~430 ms. The remaining sweep cost is 10× smaller and only there as a belt-and-braces backup for `GitWatcher` overflow / error paths. + +--- + +## Fix 1 — Event-driven `pull-requests` runtime ✅ landed + +**Goal:** turn the unconditional 30s `syncWorkspaceBranches` polling into a `git:changed` subscription, so idle ticks cost ~0 git subprocesses regardless of worktree count. + +### Changes + +1. **Inject `GitWatcher` into `PullRequestRuntimeManager`** — extend `PullRequestRuntimeManagerOptions` with a `gitWatcher: GitWatcher` field. Wire it through `packages/host-service/src/app.ts:85+` where the runtime is constructed alongside the existing `GitWatcher`. + +2. **Replace the polling timer in `start()`** (`packages/host-service/src/runtime/pull-requests/pull-requests.ts:218-230`): + + ```ts + start() { + if (this.unsubscribeFromGitWatcher) return; + + // One initial sweep so existing workspaces have correct branch/sha/upstream + // even if no .git/ changes have happened since the last process start. + void this.syncWorkspaceBranches(); + void this.refreshEligibleProjects(); + + // Steady-state: react to real .git/ changes per workspace. + this.unsubscribeFromGitWatcher = this.gitWatcher.onChanged((event) => { + void this.syncOneWorkspace(event.workspaceId); + }); + + // Long-cadence safety net for events the watcher might miss + // (overflow, fs.watch errors). 5 min, not 30 s. + this.safetyNetTimer = setInterval( + () => void this.syncWorkspaceBranches(), + SAFETY_NET_INTERVAL_MS, + ); + } + ``` + +3. **Add `syncOneWorkspace(workspaceId)`** — the existing `syncWorkspaceBranches` loop body (lines 310–356) extracted to operate on a single workspace by id. Reuses every existing helper (`getCurrentBranchName`, `getHeadSha`, `resolveWorkspaceUpstream`). + +4. **Drop `BRANCH_SYNC_INTERVAL_MS = 30_000`**, add `SAFETY_NET_INTERVAL_MS = 5 * 60_000`. The 30 s timer goes away. + +5. **`stop()`** unsubscribes from the GitWatcher and clears the safety-net timer. + +### Why this is safe + +- `GitWatcher` already debounces `.git/` changes per workspace at 300 ms (`git-watcher.ts:12, 136-162`). Branch / HEAD / upstream changes always touch `.git/` (refs, HEAD pointer, config), so this catches everything `syncWorkspaceBranches` catches today, with **lower** latency (300 ms vs 30 s). +- The 5-min safety net handles the rare overflow/error path where `GitWatcher` resets a watcher and might miss an event. +- Initial `syncWorkspaceBranches` call on `start()` ensures workspaces created before the runtime started are caught up. + +### Implementation notes (gotchas the next session will hit) + +**App.ts wiring order.** `GitWatcher` must be **constructed and started before** `PullRequestRuntimeManager.start()`, otherwise the subscription registers but the watcher hasn't begun emitting yet. In `packages/host-service/src/app.ts` the existing flow constructs `GitWatcher` already (search for `new GitWatcher`); just thread the same instance into `PullRequestRuntimeManager` constructor options and call `gitWatcher.start()` first. + +**Concurrency is already safe.** Multiple `git:changed` events for different workspaces will fire concurrent `syncOneWorkspace(workspaceId)` calls. Each calls `refreshProject(projectId)` if it detected a change. The existing `inFlightProjects` guard at `pull-requests.ts:384-388` already deduplicates concurrent refreshes for the same project. No new locking required. + +**Workspace deleted between event fire and sync handler.** If a workspace is deleted while a `git:changed` event is in flight, the `syncOneWorkspace` lookup against the workspaces table returns nothing — early-return is the right behavior. Don't throw. + +**`.git/config` changes.** Upstream resolution depends on `git config branch.X.merge` etc. `GitWatcher` watches `.git/` recursively (line 234), so config edits trigger `git:changed`. The `paths` field on `GitChangedEvent` will be absent (it's a `.git/*` event, see `git-watcher.ts:146-148`), but that's fine — we re-derive everything anyway. + +**Debounce window.** `GitWatcher` debounces at 300 ms (`git-watcher.ts:12`). Real branch-change latency under the new design: ~300 ms, vs up-to-30s under polling. Net win. + +**`syncWorkspaceBranches` stays.** Don't delete the existing method — extract `syncOneWorkspace(workspaceId)` and have `syncWorkspaceBranches` call it for each workspace. The full-sweep version is now used only for: +1. The one-time call from `start()` (initial state catch-up). +2. The 5-min safety-net interval (covers `GitWatcher` error/overflow paths). + +### Verification + +Each existing test/benchmark needs an update. Map of changes: + +| File | Current behavior | Update | +|------|------------------|--------| +| `packages/host-service/test/pull-requests-scaling.test.ts` | unit test, mocks db + git, calls `syncWorkspaceBranches` directly. Asserts O(N) git ops per call. | KEEP AS-IS — the safety-net path still exists and still does O(N) when invoked. Rename the describe to clarify it tests "the safety-net sweep" rather than "the 30s tick." | +| `packages/host-service/test/integration/pull-requests-scaling.integration.test.ts` | "idle tick still issues git calls for every workspace" — asserts `totalAfterTwoTicks === firstTickCount * 2`. | ADD a new test for event-driven path: construct manager with a real `GitWatcher`, `start()` it, do `git commit` in one of N fixture repos, assert only that workspace's git-op counter incremented (the other N-1 stay at 0). KEEP the existing test — it now tests the safety-net sweep behavior. | +| `packages/host-service/test/integration/pull-requests-scaling.bench.test.ts` | measures wall-clock of `syncWorkspaceBranches` ticks. | REPLACE with a benchmark that measures event-to-DB-update latency for a single `git commit` event, plus the safety-net sweep cost. The "ms per polling tick" measurement no longer corresponds to runtime behavior. | +| `packages/host-service/test/pull-requests.test.ts` | existing pre-audit unit tests for `syncWorkspaceBranches`. | LIKELY UNCHANGED — they call `syncWorkspaceBranches` directly which still exists. Run to confirm. | + +### New test for the event-driven path (sketch) + +```ts +test("git:changed event triggers single-workspace sync, not full sweep", async () => { + // 5 fixture repos, 5 workspaces seeded. + const scenario = await createScalingScenario(5); + + // Wire a real GitWatcher against the test host's filesystem manager. + const gitWatcher = new GitWatcher(scenario.host.db, scenario.host.runtime.filesystem); + // ...inject into manager... + scenario.manager.start(); + await waitFor(() => gitWatcher.isWatchingAll(scenario.workspaceIds)); + + scenario.gitOpLog.length = 0; + + // Commit in workspace 2 only. + await scenario.repos[2].commit("change"); + + // Wait for debounce window (300ms) + a small buffer. + await waitFor(() => scenario.gitOpLog.length > 0, { timeout: 2000 }); + await new Promise((r) => setTimeout(r, 100)); + + // Only workspace 2's worktreePath should appear in the log. + const touched = new Set(scenario.gitOpLog.map((c) => c.worktreePath)); + expect(touched.size).toBe(1); + expect(touched.has(scenario.repos[2].repoPath)).toBe(true); +}); +``` + +### Target numbers + +| Scenario | Before | After | +|----------|--------|-------| +| Idle tick @ N=20 worktrees | 1542 ms (80 git ops) | 0 ms (0 git ops) | +| Single branch change @ any N | ≤ 30 s wait + ~80 ms work | ~300 ms wait + ~80 ms work | +| Daily git subprocess count @ N=20 | ~230k | proportional to actual branch changes (~10s–100s/day) | + +--- + +## Fix 2 — LRU + idle-TTL cap on `searchIndexCache` ✅ landed + +**Goal:** bound JS heap growth by capping the number of cached worktree indexes and evicting idle entries. + +### Changes + +In `packages/workspace-fs/src/search.ts:100`: + +```ts +const SEARCH_INDEX_CACHE_MAX = 12; +const SEARCH_INDEX_CACHE_TTL_MS = 30 * 60_000; + +interface CachedIndex { + items: SearchIndexEntry[]; + lastAccessedAt: number; +} + +// Replace plain Map with an LRU + TTL. +const searchIndexCache = new Map(); + +function evictStaleEntries(): void { + const now = Date.now(); + for (const [key, cached] of searchIndexCache) { + if (now - cached.lastAccessedAt > SEARCH_INDEX_CACHE_TTL_MS) { + searchIndexCache.delete(key); + } + } +} + +function evictLruIfFull(): void { + while (searchIndexCache.size >= SEARCH_INDEX_CACHE_MAX) { + // Map iteration is insertion-order; LRU bump moves entries to the end + // (delete + set). The first key in the Map is the least-recently-used. + const oldestKey = searchIndexCache.keys().next().value; + if (!oldestKey) break; + searchIndexCache.delete(oldestKey); + } +} +``` + +In `getSearchIndex` (lines 272–300): +- On hit, `delete` then re-`set` the entry to bump it to most-recently-used in insertion order, and update `lastAccessedAt`. +- On miss, after `buildSearchIndex` resolves, run `evictLruIfFull()` before inserting; opportunistically `evictStaleEntries()` too. + +`patchSearchIndexesForRoot` and `invalidateSearchIndex*` need minor updates to read/write the `CachedIndex` shape. + +### Why this is safe + +- `patchSearchIndexesForRoot` from the file watcher keeps the active worktree's index current — no behavior change for active worktrees. +- After eviction, the next search for that worktree pays a fresh `fast-glob` walk (~50–200 ms for a 5k-file repo). That's acceptable cold-cost for a worktree the user hasn't searched in 30 minutes. +- `searchIndexBuilds` (line 101) already deduplicates concurrent builds; eviction can race with an in-flight build, but the deduplication map handles it. + +### Verification + +- **`search-cache-no-eviction.test.ts`** — flip the assertions: after building 13 indexes, the *first* one should NOT be `===` to its initial reference (it got evicted). The "100 newer worktrees" test should fail-as-designed. Update the test name to `search-cache-eviction.test.ts` and rewrite assertions. +- **`cache-and-paths-memory.bench.test.ts`** — re-run; heap delta at 130 worktrees should drop from ~6.87 MB to whatever 12 worktrees × ~53 KB ≈ 0.6 MB. + +### Target numbers + +| Scenario | Before | After | +|----------|--------|-------| +| Heap @ 130 cached indexes | +6.87 MB | +0.6 MB (only 12 retained) | +| Heap growth rate | linear in N | bounded by cap | +| Cold-search latency on evicted worktree | n/a | +50–200 ms | + +--- + +## Fix 3 — LRU cap on per-watcher `pathTypes` ✅ landed + +**Goal:** stop unbounded growth of `WatcherState.pathTypes` when worktrees see continuous unique-path creation (logs, hashed build artifacts). + +### Changes + +In `packages/workspace-fs/src/watch.ts:32-39, 472-484`: + +```ts +const PATH_TYPES_MAX = 10_000; + +interface WatcherState { + // ...existing fields... + pathTypes: Map; +} + +// In normalizeEvent (line 467-491): +if (event.type === "delete") { + state.pathTypes.delete(absolutePath); +} else { + try { + const stats = await stat(absolutePath); + isDirectory = stats.isDirectory(); + + // LRU bump: re-insertion moves to most-recently-used position. + state.pathTypes.delete(absolutePath); + if (state.pathTypes.size >= PATH_TYPES_MAX) { + const oldest = state.pathTypes.keys().next().value; + if (oldest) state.pathTypes.delete(oldest); + } + state.pathTypes.set(absolutePath, isDirectory); + } catch { + isDirectory = state.pathTypes.get(absolutePath) ?? false; + } +} +``` + +### Why this is safe + +- `pathTypes` is a directory-type hint to avoid `stat()` on every event for the same path. Evicting an entry means the next event for that path falls into the existing `try { await stat() } catch` branch — i.e., the existing slow path, not a bug. +- The cap is per-watcher, so the worst case is one worktree thrashing its own cache while others are unaffected. + +### Verification + +- **`watch-pathtypes-growth.test.ts`** — the "30 unique paths" test still passes (30 < cap). Add a new test: create 10,001 unique paths and assert `pathTypes.size === 10_000` with the oldest entry evicted. +- **`cache-and-paths-memory.bench.test.ts`** — at 20k unique paths, heap should plateau at ~5 MB (10k entries × ~430 bytes) instead of climbing to ~9 MB. + +### Target numbers + +| Scenario | Before | After | +|----------|--------|-------| +| `pathTypes.size` after 20k unique paths | 20,000 | 10,000 (capped) | +| Heap @ 20k paths | +8.69 MB | +4.3 MB (capped) | +| Daily heap growth @ 20 active worktrees | ~85 MB/day | bounded ~85 MB total | + +--- + +## Fix 4 — Loosen `refreshEligibleProjects` to 5-min safety net ✅ landed + +**Goal:** drop the constant 20s ticking once Fix 1 makes branch changes event-driven. + +### Changes + +In `packages/host-service/src/runtime/pull-requests/pull-requests.ts:25-26`: + +```ts +const PROJECT_REFRESH_INTERVAL_MS = 5 * 60_000; // was 20_000 +``` + +Optionally, drop the timer entirely and rely on `refreshProject` calls from `syncOneWorkspace` (Fix 1) to keep the GraphQL cache warm. The 60s repo-PR cache (line 32) already absorbs duplicate fetches. + +### Why this is safe + +- Fix 1's event-driven `syncOneWorkspace` calls `refreshProject` whenever a branch change is detected, so PR state for active workspaces stays current without polling. +- The 5-min safety net catches PRs opened on GitHub without a corresponding local branch change (rare — the local fetch would trigger `git:changed`). + +### Verification + +- No new tests required. Existing `pull-requests.test.ts` integration tests should still pass. +- The host-service idle CPU profile should show no measurable activity in the runtime when no workspaces have `.git/` activity. + +--- + +## Fix 5 — (Deferred) Lazy GitWatcher registration + +After Fixes 1–3 land, re-measure idle host-service CPU and RSS at N=20 worktrees. If they're already flat, this fix is unnecessary — the per-watcher native cost is small in the absence of file events. + +If they're not flat (e.g. background dev servers in many worktrees still cause measurable wakeups), revisit by: +- Adding a refcount to `GitWatcher.watchWorkspace` keyed on subscriber count. +- Generalizing the `bus.watchFs(workspaceId)` pattern from `apps/desktop/src/renderer/hooks/host-service/useWorkspaceEvent/useWorkspaceEvent.ts:73-83` to git events. +- BUT: the pull-requests runtime (post-Fix-1) is itself a subscriber to all workspaces' `git:changed`, so refcount-based laziness needs a way to skip the runtime's "always-on" subscription, or the runtime needs to subscribe lazily based on PR-tracked workspaces only. + +This is a meaningful refactor; defer until measurements justify it. + +--- + +## Sequencing & rollout + +These fixes are internal to host-service / workspace-fs and don't touch the renderer or any tRPC contracts. No feature flags required. Land them as separate PRs in this order: + +1. **Fix 2 + 3 first** (workspace-fs LRU caps) — small, isolated, no behavior change for active worktrees, easy to revert. Get the "memory creep stops" win quickly. +2. **Fix 1** (event-driven pull-requests) — bigger change, depends on `GitWatcher` already being constructed in `app.ts` (it is). Verify with the existing integration tests + a new "real branch change triggers single-workspace sync" test. +3. **Fix 4** — one-line change after #1 lands. Bundle with #1's PR if the integration test for #1 demonstrates the project refresh fan-out is no longer hot. + +Each PR should re-run the corresponding benchmark from the findings doc and paste the before/after numbers in the description. + +--- + +## Out of scope + +- **Renderer-side `useDiffStats` fan-out** — already demoted to "boot/mount cost" in the findings audit. If sidebar-mount latency becomes a complaint, add a `git.getDiffStats` host endpoint that returns just `git diff --shortstat HEAD` per workspace, and switch `useDiffStats` to it. Separate effort. +- **`useChangesTab` / `useReviewTab` / `usePRFlowState` 10–30s `refetchInterval`s** — verified to fire only for the active workspace, not per-worktree. No change. + +--- + +## Acceptance criteria + +After Fixes 1–4 land: + +- `pull-requests-scaling.integration.test.ts` retains the safety-net sweep coverage and adds a single-workspace event-driven sync case asserting that a commit in one of N worktrees only spawns git ops for that one worktree. +- `pull-requests-scaling.bench.test.ts` reports two metrics: (1) commit → DB-update latency (~430 ms at N=5), (2) safety-net sweep wall-clock at N ∈ {1, 5, 20}. +- `cache-and-paths-memory.bench.test.ts` reports plateau heap deltas (~2 MB cache cap, ~2.5 MB pathTypes cap) regardless of input size. +- Manual smoke: open 20 worktrees, leave the host-service idle for 10 minutes, verify CPU baseline is ≤ 1% and RSS is stable.