diff --git a/docs/startup-chat-project-performance/intent.md b/docs/startup-chat-project-performance/intent.md new file mode 100644 index 0000000..4a9da04 --- /dev/null +++ b/docs/startup-chat-project-performance/intent.md @@ -0,0 +1,36 @@ +# Intent: Reduce startup, Chat, and Project loading latency +Author: SpireCode maintainer. Status: approved. + +## Problem + +SpireCode feels blocked during cold startup, when opening a Chat conversation, and when adding a Project. Read-only diagnosis found that expensive watcher, Git, Pi resource, extension, session snapshot, and durable persistence work is awaited on user-visible critical paths. + +On the current development machine, startup restores 17 Projects and 23 Worktrees. The startup path performs one serial Git lookup and watcher initialization per Worktree before loading the Renderer. Opening a cold Chat session initializes Pi resources and extensions before returning its snapshot. Adding a Project waits for watcher initialization before returning to the Renderer. + +## Proposed outcome + +1. Show the application without waiting for all persisted Worktree watchers. +2. Return newly added Projects after durable catalog registration while initializing their watchers in the background. +3. Avoid redundant Chat session scans and repeated identical resource discovery where lifecycle-safe. +4. Bound Chat snapshots in Electron Main before IPC transfer. +5. Add privacy-safe phase timing logs so startup, Project open, and Chat attach improvements are measurable. + +## Affected users and systems + +- All users starting SpireCode with persisted Projects and Worktrees. +- Users adding local Git repositories. +- Users opening restored or historical Chat sessions. +- Electron Main watcher, Git, Chat, settings/resource, diagnostics, and IPC domains. + +## Constraints + +- Preserve canonical root validation and watcher coverage. +- Project catalog durability must complete before Project open reports success. +- Background watcher failures must not become unhandled rejections or remove Projects. +- Do not share session-bound Pi state across conversations unless the SDK explicitly permits it. +- Keep Renderer sandbox and narrow IPC contracts unchanged unless a bounded DTO field is required. +- Performance logs must not include paths, prompts, credentials, message content, or extension configuration. + +## Open questions + +None. diff --git a/docs/startup-chat-project-performance/plan.md b/docs/startup-chat-project-performance/plan.md new file mode 100644 index 0000000..85c416d --- /dev/null +++ b/docs/startup-chat-project-performance/plan.md @@ -0,0 +1,41 @@ +# Plan: Reduce startup, Chat, and Project loading latency (from docs/startup-chat-project-performance/spec.md 2026-09-23) + +## Files that change + +Expected files, refined only if tests reveal a narrower boundary: + +- `electron/appState.ts` and tests — background, bounded watcher initialization and startup timing. +- `electron/domains/chat/chatService.ts` and tests — exact session open, shared in-flight lifecycle, bounded snapshots, attach timing. +- `electron/domains/chat/piAdapter.ts` and tests — expose exact-ID open support and cache only lifecycle-safe discovery work. +- `electron/domains/diagnostics/service.ts` and tests — privacy-safe performance timing helper if the existing API is insufficient. +- `docs/startup-chat-project-performance/{intent,spec,plan}.md` — approved requirement, design, and proof chain. + +## Order of work + +1. Add failing tests proving `AppState.create()` and Project open do not await injected slow watcher initialization. +2. Implement active-first, bounded background watcher scheduling with safe failure handling and shutdown behavior. +3. Add failing tests proving cold Chat attach can resolve an exact session without adapter list and duplicate cold opens share one in-flight lifecycle operation. +4. Implement exact-ID session opening through the Pi adapter without weakening cwd/session-root checks. +5. Add failing tests proving Main returns at most the newest 2,000 projected Chat timeline items. +6. Move snapshot bounding before IPC return. +7. Add privacy-safe phase duration logging and tests that reject sensitive metadata. +8. Run focused Electron domain tests, then `pnpm check`. +9. Measure the local 17-Project/23-Worktree startup path to confirm watcher Git processes are no longer in the pre-Renderer critical path. +10. Review the diff, commit, push, open a PR, and reinstall the application for interactive validation. + +## Risks + +- The most dangerous change is watcher backgrounding because shutdown or rapid Project close can race initialization. Use idempotent registry operations, disposal guards, and settled promises. +- Sharing Pi session-bound services is rejected; only exact path resolution and safe discovery/in-flight work may be cached. +- Returning Project success before watcher readiness creates a brief eventual-consistency window. Initial explicit Files/Git reads cover current state; diagnostics capture watcher failures. +- Main-side snapshot bounding can hide old UI history by design but must never alter persisted agent context. +- Unbounded parallel watcher startup is rejected because it would trade startup blocking for a process/I/O spike. + +## Proof + +- Focused watcher/AppState tests demonstrate delayed watcher promises do not delay startup or Project response. +- Focused Chat tests demonstrate no redundant list scan, in-flight deduplication, and newest-2,000 snapshot bounds. +- Diagnostics tests demonstrate timing metadata allowlisting and absence of sensitive values. +- `pnpm check` exits 0. +- `pnpm bundle` and macOS App/DMG smoke pass before local reinstall. +- `git diff --check` exits 0. diff --git a/docs/startup-chat-project-performance/spec.md b/docs/startup-chat-project-performance/spec.md new file mode 100644 index 0000000..50a3a8a --- /dev/null +++ b/docs/startup-chat-project-performance/spec.md @@ -0,0 +1,68 @@ +# Spec: Reduce startup, Chat, and Project loading latency + +## Requirements + +### Startup and watchers + +1. `BrowserWindow.loadFile()` / `loadURL()` must not wait for persisted Worktree watcher initialization. +2. Persisted Worktree watchers must initialize in the background with bounded concurrency and active Worktree priority. +3. Background failures must be logged safely and must not abort startup. +4. Shutdown must remain safe while background initialization is pending. + +### Add Project + +5. Opening a Project must still validate its Git root and durably persist the catalog before returning. +6. Watcher initialization must not delay the successful Project response. +7. Watcher initialization failures must be observable through diagnostics without invalidating the Project. + +### Chat + +8. Attaching a cold historical session must avoid a second full session-list scan when an exact session identifier/path can be resolved safely. +9. Identical in-flight adapter/session initialization must be shared rather than duplicated. +10. Settings and package/resource discovery may be cached only behind inputs that make invalidation explicit; session-bound resource loaders and extension runtimes must not be shared unsafely. +11. Main must limit timeline snapshot items before structured-clone IPC transfer. The Renderer limit remains a defensive backstop. +12. The bounded snapshot must preserve chronological order and retain the newest 2,000 timeline items. + +### Measurement + +13. Emit structured, privacy-safe duration records for startup AppState, background watcher batches, Project open phases, and Chat attach phases. +14. Timing records contain phase, duration, success, and safe counts/booleans only; no filesystem paths, project names, prompts, model credentials, or message content. + +## Design + +### Startup watcher scheduling + +`AppState.create()` loads durable services and returns immediately after constructing `AppState`. It schedules persisted Worktree watcher initialization through an internal bounded worker queue. The active Worktree, if any, is first. Remaining watchers run with a small concurrency limit so startup does not launch one Git process per Worktree simultaneously. + +`dispose()` marks the state disposed and awaits/cancels only as needed to prevent late unhandled work. Watcher registry operations remain idempotent. + +### Project watcher scheduling + +`openProject()` awaits `ProjectService.openPath()` and then schedules each Worktree watcher on the same background scheduler. The returned Project is not rolled back if watcher setup fails because watcher setup is runtime infrastructure, not catalog validity. + +### Chat attach + +Use Pi SDK exact-ID lookup when available rather than `SessionManager.list(cwd)` followed by `.find()`. The resolved path remains constrained to the SDK-owned session directory and is reopened with the expected cwd. + +Keep the global `ModelRuntime` and adapter singleton. Cache only pure discovery inputs/results with explicit settings/source fingerprints or short-lived in-flight promises. Continue creating a separate resource loader, extension runtime, and AgentSession per Chat because they own session lifecycle and subscriptions. + +At snapshot time, normalize the active branch then retain only the newest 2,000 timeline items before returning from Electron Main. + +### Timing diagnostics + +Use the existing `DiagnosticsService.log()` JSONL sink. Add a narrow performance helper that records integer duration milliseconds and allowlisted metadata. Startup timing begins in `AppState.create`; Project and Chat timings live at their domain boundaries. + +## Acceptance criteria + +- With 20+ persisted Worktrees, `AppState.create()` no longer runs one watcher Git lookup per Worktree before Renderer load. +- `openProject()` resolves before an injected slow watcher promise. +- A cold Chat attach opens an exact session without invoking adapter list. +- A snapshot with more than 2,000 projected items returns exactly the newest 2,000. +- Duplicate initialization tests prove one in-flight operation is shared where caching is introduced. +- `pnpm check` passes. + +## Concerns + +- **Concern: PiServices reuse.** `DefaultResourceLoader`, extension runtimes, and bound UI contexts are session-scoped. Reusing them across Chat sessions risks cross-session state and cleanup corruption. The implementation must prefer caching discovery data or in-flight pure work instead of reusing session-bound services. +- **Concern: watcher readiness.** Filesystem/Git invalidations may be missed during the short interval before the background watcher becomes ready. Initial file tree and Git status reads remain authoritative; watcher readiness is eventual. +- **Concern: snapshot truncation.** Truncating projected UI items must not modify persisted Pi context or SessionManager entries. It affects only the Renderer snapshot DTO. diff --git a/electron/appState.test.ts b/electron/appState.test.ts index 9d034e2..b55d304 100644 --- a/electron/appState.test.ts +++ b/electron/appState.test.ts @@ -1,6 +1,6 @@ // @vitest-environment node -import { afterEach, describe, expect, it } from "vitest"; -import { applyMemoryConfig } from "./appState.js"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { applyMemoryConfig, BackgroundWatcherScheduler } from "./appState.js"; const originalModel = process.env.PI_MEMORY_EXTRACT_MODEL; const originalPhase2Model = process.env.PI_MEMORY_PHASE2_MODEL; @@ -21,6 +21,81 @@ afterEach(() => { else process.env.PI_MEMORY_PHASE2_THINKING = originalPhase2Thinking; }); +describe("BackgroundWatcherScheduler", () => { + it("prioritizes the active worktree and bounds concurrency", async () => { + const started: string[] = []; + const releases = new Map void>(); + let running = 0; + let maxRunning = 0; + const scheduler = new BackgroundWatcherScheduler( + ({ id }) => + new Promise((resolve) => { + started.push(id); + running += 1; + maxRunning = Math.max(maxRunning, running); + releases.set(id, () => { + running -= 1; + resolve(); + }); + }), + () => undefined, + () => undefined, + 2, + ); + + scheduler.schedule( + [ + { id: "one", path: "/one" }, + { id: "two", path: "/two" }, + { id: "active", path: "/active" }, + { id: "three", path: "/three" }, + ], + "active", + ); + expect(started).toEqual(["active", "one"]); + releases.get("active")?.(); + await vi.waitFor(() => expect(started).toEqual(["active", "one", "two"])); + expect(maxRunning).toBe(2); + releases.get("one")?.(); + releases.get("two")?.(); + await Promise.resolve(); + await Promise.resolve(); + releases.get("three")?.(); + scheduler.dispose(); + }); + + it("returns immediately, contains failures, and cancels queued work", async () => { + const failures: unknown[] = []; + let release: () => void = () => undefined; + const started: string[] = []; + const scheduler = new BackgroundWatcherScheduler( + ({ id }) => { + started.push(id); + if (id === "running") + return new Promise((resolve) => { + release = resolve; + }); + return Promise.reject(new Error("private path must not escape")); + }, + (error) => failures.push(error), + () => undefined, + 1, + ); + + scheduler.schedule([ + { id: "running", path: "/running" }, + { id: "cancelled", path: "/cancelled" }, + { id: "failed", path: "/failed" }, + ]); + expect(started).toEqual(["running"]); + scheduler.cancel("cancelled"); + release(); + await vi.waitFor(() => expect(started).toEqual(["running", "failed"])); + await vi.waitFor(() => expect(failures).toHaveLength(1)); + scheduler.dispose(); + }); +}); + describe("applyMemoryConfig", () => { it("clears Memory overrides for an unconfigured clean install", () => { process.env.PI_MEMORY_EXTRACT_MODEL = "old/model"; diff --git a/electron/appState.ts b/electron/appState.ts index cbb9b21..7fb1a5b 100644 --- a/electron/appState.ts +++ b/electron/appState.ts @@ -28,6 +28,112 @@ export interface SubscriptionEvent { payload: T; } +interface WatchTarget { + id: string; + path: string; +} + +export class BackgroundWatcherScheduler { + private readonly queued: WatchTarget[] = []; + private readonly running = new Map>(); + private disposed = false; + private batchStartedAt: number | undefined; + private batchScheduled = 0; + private batchSucceeded = 0; + private batchFailed = 0; + + constructor( + private readonly initialize: (target: WatchTarget) => Promise, + private readonly onFailure: (error: unknown) => void, + private readonly onBatchComplete: (result: { + totalMs: number; + scheduled: number; + succeeded: number; + failed: number; + concurrency: number; + }) => void = () => undefined, + private readonly concurrency = 2, + ) {} + + schedule(targets: readonly WatchTarget[], priorityId?: string | null): void { + if (this.disposed) return; + if (this.running.size === 0 && this.queued.length === 0) { + this.batchStartedAt = performance.now(); + this.batchScheduled = 0; + this.batchSucceeded = 0; + this.batchFailed = 0; + } + const ordered = priorityId + ? [ + ...targets.filter(({ id }) => id === priorityId), + ...targets.filter(({ id }) => id !== priorityId), + ] + : [...targets]; + for (const target of ordered) { + if (this.running.has(target.id)) continue; + const existing = this.queued.findIndex(({ id }) => id === target.id); + const isNew = existing < 0; + if (existing >= 0) this.queued.splice(existing, 1); + if (target.id === priorityId) this.queued.unshift(target); + else this.queued.push(target); + if (isNew) this.batchScheduled += 1; + } + this.pump(); + } + + cancel(id: string): void { + const queued = this.queued.findIndex((target) => target.id === id); + if (queued >= 0) this.queued.splice(queued, 1); + } + + dispose(): void { + this.disposed = true; + this.queued.splice(0); + } + + private pump(): void { + while ( + !this.disposed && + this.running.size < this.concurrency && + this.queued.length > 0 + ) { + const target = this.queued.shift(); + if (!target || this.running.has(target.id)) continue; + const pending = this.initialize(target) + .then(() => { + this.batchSucceeded += 1; + }) + .catch((error) => { + this.batchFailed += 1; + this.onFailure(error); + }) + .finally(() => { + this.running.delete(target.id); + this.pump(); + this.finishBatchIfIdle(); + }); + this.running.set(target.id, pending); + } + } + + private finishBatchIfIdle(): void { + if ( + this.batchStartedAt === undefined || + this.running.size > 0 || + this.queued.length > 0 + ) + return; + this.onBatchComplete({ + totalMs: performance.now() - this.batchStartedAt, + scheduled: this.batchScheduled, + succeeded: this.batchSucceeded, + failed: this.batchFailed, + concurrency: this.concurrency, + }); + this.batchStartedAt = undefined; + } +} + export class AppState { readonly filesystem: FilesystemService; readonly git: GitService; @@ -39,6 +145,7 @@ export class AppState { readonly watchers = new WatcherRegistry(); readonly windowCloseGuard = new WindowCloseGuard(); readonly diagnostics: DiagnosticsService; + private readonly watcherScheduler: BackgroundWatcherScheduler; private constructor( readonly projects: ProjectService, @@ -57,32 +164,69 @@ export class AppState { }); this.chat = new ChatService(root, { trashItem: (sessionPath) => shell.trashItem(sessionPath), + performance: (event) => void this.diagnostics.logPerformance(event), }); this.worktrees = new WorktreeService(projects, this.terminals); + this.watcherScheduler = new BackgroundWatcherScheduler( + ({ id, path: rootPath }) => this.ensureWatcher(id, rootPath), + (error) => { + console.warn( + `Unable to initialize background worktree watcher (${errorName(error)})`, + ); + void this.diagnostics.log({ + level: "warn", + code: "WATCHER_INITIALIZATION_FAILED", + safeContext: { errorType: errorName(error) }, + }); + }, + (result) => + void this.diagnostics.logPerformance({ + code: "PERF_WATCHER_BATCH", + outcome: result.failed === 0 ? "ok" : "partial", + totalMs: result.totalMs, + counts: { + scheduled: result.scheduled, + succeeded: result.succeeded, + failed: result.failed, + concurrency: result.concurrency, + }, + }), + ); } static async create( dataDirectory: string, window: BrowserWindow, ): Promise { + const startedAt = performance.now(); const [projects, settings] = await Promise.all([ ProjectService.load(path.join(dataDirectory, "state.json")), SettingsService.load(path.join(dataDirectory, "extension-settings.json")), ]); applyMemoryConfig(await settings.memoryConfig()); const state = new AppState(projects, settings, window, dataDirectory); - for (const project of await projects.list()) { - for (const worktree of project.worktrees) { - try { - await state.ensureWatcher(worktree.id, worktree.path); - } catch (error) { - console.warn( - `Unable to watch persisted worktree ${worktree.id}`, - error, - ); - } - } - } + const catalog = await projects.catalog(); + state.watcherScheduler.schedule( + catalog.projects.flatMap((project) => + project.worktrees.map((worktree) => ({ + id: worktree.id, + path: worktree.path, + })), + ), + catalog.activeWorktreeId, + ); + void state.diagnostics.logPerformance({ + code: "PERF_APP_STATE", + outcome: "ok", + totalMs: performance.now() - startedAt, + counts: { + projectCount: catalog.projects.length, + worktreeCount: catalog.projects.reduce( + (count, project) => count + project.worktrees.length, + 0, + ), + }, + }); return state; } @@ -153,10 +297,32 @@ export class AppState { } async openProject(selectedPath: string) { - const project = await this.projects.openPath(selectedPath); - for (const worktree of project.worktrees) - await this.ensureWatcher(worktree.id, worktree.path); - return project; + const startedAt = performance.now(); + try { + const project = await this.projects.openPath(selectedPath); + this.watcherScheduler.schedule( + project.worktrees.map((worktree) => ({ + id: worktree.id, + path: worktree.path, + })), + project.worktrees.find((worktree) => worktree.kind === "main")?.id, + ); + void this.diagnostics.logPerformance({ + code: "PERF_PROJECT_OPEN", + outcome: "ok", + totalMs: performance.now() - startedAt, + counts: { worktreeCount: project.worktrees.length }, + }); + return project; + } catch (error) { + void this.diagnostics.logPerformance({ + code: "PERF_PROJECT_OPEN", + outcome: "error", + totalMs: performance.now() - startedAt, + counts: { worktreeCount: 0 }, + }); + throw error; + } } async openProjectDialog() { @@ -171,6 +337,7 @@ export class AppState { const project = await this.projects.project(projectId); let failure: unknown; for (const worktree of project.worktrees) { + this.watcherScheduler.cancel(worktree.id); this.terminals.closeWorktree(worktree.id); this.git.closeWorktree(worktree.id); for (const cleanup of [ @@ -210,6 +377,7 @@ export class AppState { async renameWorktree(worktreeId: string, name: string) { const original = await this.projects.worktree(worktreeId); + this.watcherScheduler.cancel(worktreeId); await this.watchers.close(worktreeId); try { const renamed = await this.worktrees.rename(worktreeId, name); @@ -233,6 +401,7 @@ export class AppState { const inspection = await this.worktrees.inspectDelete(worktreeId); if (!force && (inspection.dirty || inspection.terminalCount > 0)) return this.worktrees.delete(worktreeId, false); + this.watcherScheduler.cancel(worktreeId); this.terminals.closeWorktree(worktreeId); this.git.closeWorktree(worktreeId); const cleanup = await Promise.allSettled([ @@ -257,6 +426,7 @@ export class AppState { async dispose(): Promise { this.terminals.dispose(); + this.watcherScheduler.dispose(); const cleanup = await Promise.allSettled([ this.watchers.dispose(), withTimeout(this.chat.disposeAll(), 5_000), @@ -299,6 +469,10 @@ export function applyMemoryConfig(config: MemoryConfig | null): void { process.env.PI_MEMORY_PHASE2_THINKING = config.phase2ReasoningEffort; } +function errorName(error: unknown): string { + return error instanceof Error ? error.name : typeof error; +} + function withTimeout( operation: Promise, milliseconds: number, diff --git a/electron/domains/chat/chatService.test.ts b/electron/domains/chat/chatService.test.ts index 82a84fa..a681ef4 100644 --- a/electron/domains/chat/chatService.test.ts +++ b/electron/domains/chat/chatService.test.ts @@ -148,6 +148,10 @@ async function fixture(): Promise<{ if (!info) throw new Error("missing"); return { info, sessionRoot }; }, + async openById(cwd, sessionId) { + const record = records.get(sessionId); + return record?.cwd === cwd ? record : undefined; + }, async open(info, cwd) { const record = records.get(info.sessionId); if (!record || record.cwd !== cwd) throw new Error("missing"); @@ -473,6 +477,81 @@ describe("ChatService", () => { }); }); + it("opens a cold session by exact id without listing all transcripts", async () => { + const { root, records, adapter } = await fixture(); + const session = new FakeSession("cold"); + records.set("cold", { + session, + sessionId: "cold", + cwd: root, + title: "Cold", + createdAt: 1, + updatedAt: 2, + }); + const list = vi.spyOn(adapter, "list"); + const openById = vi.spyOn(adapter, "openById"); + const service = new ChatService(() => root, { adapter }); + + await expect( + service.attach("w1", "cold", () => undefined), + ).resolves.toMatchObject({ sessionId: "cold" }); + expect(openById).toHaveBeenCalledWith(root, "cold"); + expect(list).not.toHaveBeenCalled(); + }); + + it("coalesces concurrent attachments and routes buffered events to the latest subscriber", async () => { + const { root, records, adapter } = await fixture(); + const service = new ChatService(() => root, { adapter }); + await service.create("w1"); + const session = records.get("s1")?.session as FakeSession; + let release: () => void = () => undefined; + let started: () => void = () => undefined; + const reading = new Promise((resolve) => { + started = resolve; + }); + session.messageStarted = started; + session.messageGate = new Promise((resolve) => { + release = resolve; + }); + const firstEvents: unknown[] = []; + const latestEvents: unknown[] = []; + const first = service.attach("w1", "s1", (event) => + firstEvents.push(event), + ); + await reading; + const second = service.attach("w1", "s1", (event) => + latestEvents.push(event), + ); + session.emit({ type: "agent_start" }); + release(); + + expect(second).toBe(first); + await expect(Promise.all([first, second])).resolves.toHaveLength(2); + expect(firstEvents).toEqual([]); + expect(latestEvents).toEqual([ + { sessionId: "s1", sequence: 1, event: { type: "agent_start" } }, + ]); + }); + + it("bounds snapshots to the newest 2000 projected items", async () => { + const { root, records, adapter } = await fixture(); + const service = new ChatService(() => root, { adapter }); + await service.create("w1"); + const session = records.get("s1")?.session as FakeSession; + session.messages = Array.from({ length: 2_005 }, (_, index) => ({ + role: "user", + id: `message-${index}`, + content: `message ${index}`, + timestamp: index, + })); + + const snapshot = await service.attach("w1", "s1", () => undefined); + + expect(snapshot.items).toHaveLength(2_000); + expect(snapshot.items[0]).toMatchObject({ id: "message-5" }); + expect(snapshot.items.at(-1)).toMatchObject({ id: "message-2004" }); + }); + it("uses a snapshot fence and flushes only events newer than it", async () => { const { root, records, adapter } = await fixture(); const service = new ChatService(() => root, { adapter }); diff --git a/electron/domains/chat/chatService.ts b/electron/domains/chat/chatService.ts index 1ebd2f6..d5f6777 100644 --- a/electron/domains/chat/chatService.ts +++ b/electron/domains/chat/chatService.ts @@ -21,8 +21,15 @@ import { } from "./types.js"; const MAX_BUFFERED_EVENTS = 512; +const MAX_SNAPSHOT_ITEMS = 2_000; const MAX_PROMPT_BYTES = 64 * 1024; +interface AttachFlight { + worktreeId: string; + subscriber: ChatEventSubscriber; + promise: Promise; +} + interface SessionState extends PiSessionRecord { worktreeId: string; sequence: number; @@ -41,6 +48,13 @@ export interface ChatServiceOptions { adapter?: PiAdapter | Promise; maxBufferedEvents?: number; trashItem?: (sessionPath: string) => Promise; + performance?: (event: { + code: "PERF_CHAT_ATTACH"; + outcome: "ok" | "error"; + totalMs: number; + cold: boolean; + counts: { sourceItemCount: number; returnedItemCount: number }; + }) => void; } export class ChatService { @@ -50,8 +64,12 @@ export class ChatService { private readonly adapterFactory: () => Promise; private readonly maxBufferedEvents: number; private readonly trashItem?: (sessionPath: string) => Promise; + private readonly performance?: ChatServiceOptions["performance"]; private readonly deleting = new Set(); private readonly lifecycleTails = new Map>(); + private readonly attachFlights = new Map(); + private readonly worktreeGenerations = new Map(); + private disposed = false; constructor( private readonly rootResolver: RootResolver, @@ -61,6 +79,7 @@ export class ChatService { Promise.resolve(options.adapter ?? createPiAdapter()); this.maxBufferedEvents = options.maxBufferedEvents ?? MAX_BUFFERED_EVENTS; this.trashItem = options.trashItem; + this.performance = options.performance; } async create(worktreeId: string): Promise { @@ -100,32 +119,79 @@ export class ChatService { sessionId: string, subscriber: ChatEventSubscriber, ): Promise { - return this.runLifecycle(sessionId, () => - this.attachUnlocked(worktreeId, sessionId, subscriber), - ); + const existing = this.attachFlights.get(sessionId); + if (existing && existing.worktreeId === worktreeId) { + existing.subscriber = subscriber; + return existing.promise; + } + const generation = this.worktreeGenerations.get(worktreeId) ?? 0; + const flight = { + worktreeId, + subscriber, + promise: Promise.resolve(undefined as unknown as ChatSnapshot), + }; + flight.promise = this.runLifecycle(sessionId, () => + this.attachUnlocked(worktreeId, sessionId, generation, (event) => + flight.subscriber(event), + ), + ).finally(() => { + if (this.attachFlights.get(sessionId) === flight) + this.attachFlights.delete(sessionId); + }); + this.attachFlights.set(sessionId, flight); + return flight.promise; } private async attachUnlocked( worktreeId: string, sessionId: string, + generation: number, subscriber: ChatEventSubscriber, ): Promise { + const startedAt = performance.now(); const cwd = await this.root(worktreeId); let record = this.sessions.get(sessionId); + const cold = !record; if (record) this.assertOwner(record, worktreeId); try { if (!record) { const adapter = await this.adapter(); - const info = (await adapter.list(cwd)).find( - (item) => item.sessionId === sessionId, - ); - if (!info) throw notFound(); + const opened = await adapter.openById(cwd, sessionId); + if (!opened) throw notFound(); + if ( + this.disposed || + (this.worktreeGenerations.get(worktreeId) ?? 0) !== generation + ) { + await opened.session.dispose(); + throw notFound(); + } this.establishOwnership(sessionId, worktreeId); - record = this.register(await adapter.open(info, cwd), worktreeId, cwd); + record = this.register(opened, worktreeId, cwd); } - return await this.attachRecord(record, subscriber); + const { snapshot, sourceItemCount } = await this.attachRecord( + record, + subscriber, + ); + this.performance?.({ + code: "PERF_CHAT_ATTACH", + outcome: "ok", + totalMs: performance.now() - startedAt, + cold, + counts: { + sourceItemCount, + returnedItemCount: snapshot.items.length, + }, + }); + return snapshot; } catch (error) { + this.performance?.({ + code: "PERF_CHAT_ATTACH", + outcome: "error", + totalMs: performance.now() - startedAt, + cold, + counts: { sourceItemCount: 0, returnedItemCount: 0 }, + }); if (record) { record.subscriber = undefined; record.attaching = false; @@ -371,6 +437,10 @@ export class ChatService { } async closeWorktree(worktreeId: string): Promise { + this.worktreeGenerations.set( + worktreeId, + (this.worktreeGenerations.get(worktreeId) ?? 0) + 1, + ); const records = [...this.sessions.values()].filter( (record) => record.worktreeId === worktreeId, ); @@ -398,6 +468,12 @@ export class ChatService { } async disposeAll(): Promise<{ disposed: number }> { + this.disposed = true; + for (const flight of this.attachFlights.values()) + this.worktreeGenerations.set( + flight.worktreeId, + (this.worktreeGenerations.get(flight.worktreeId) ?? 0) + 1, + ); const records = [...this.sessions.values()]; this.sessions.clear(); this.owners.clear(); @@ -446,7 +522,7 @@ export class ChatService { private async attachRecord( record: SessionState, subscriber: ChatEventSubscriber, - ): Promise { + ): Promise<{ snapshot: ChatSnapshot; sourceItemCount: number }> { record.subscriber = subscriber; record.attaching = true; record.needsResnapshot = false; @@ -463,7 +539,8 @@ export class ChatService { record.session.getMessages(), record.session.getEntries(), ]); - const items = normalizeTimeline(messages, entries); + const normalizedItems = normalizeTimeline(messages, entries); + const items = normalizedItems.slice(-MAX_SNAPSHOT_ITEMS); if (record.needsResnapshot) throw resnapshotError(); const snapshot: ChatSnapshot = { sessionId: record.sessionId, @@ -476,7 +553,7 @@ export class ChatService { error: errorAtFence, }; this.flushAfterSnapshot(record, fence); - return snapshot; + return { snapshot, sourceItemCount: normalizedItems.length }; } private flushAfterSnapshot(record: SessionState, fence: number): void { diff --git a/electron/domains/chat/piAdapter.test.ts b/electron/domains/chat/piAdapter.test.ts index a7b0317..914eda3 100644 --- a/electron/domains/chat/piAdapter.test.ts +++ b/electron/domains/chat/piAdapter.test.ts @@ -1,10 +1,23 @@ -import { describe, expect, it, vi } from "vitest"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createPiAdapter, preferSpirecodeProviders, type PiSdk, } from "./piAdapter.js"; +const temporaryDirectories: string[] = []; + +afterEach(async () => { + await Promise.all( + temporaryDirectories + .splice(0) + .map((directory) => rm(directory, { recursive: true, force: true })), + ); +}); + function sessionFixture() { let streaming = false; let model: unknown = { @@ -110,6 +123,7 @@ function sdkFixture(options: { }; const manager = { buildSessionContext: () => ({ messages: options.existingMessages ?? [] }), + getCwd: () => "/repo", getSessionDir: () => "/sessions/current", getBranch: () => [ { @@ -132,6 +146,7 @@ function sdkFixture(options: { SessionManager: { create: () => manager, list: async () => [], + findById: () => undefined, open: () => manager, }, createAgentSessionServices: async (input: { @@ -376,6 +391,28 @@ describe("piAdapter", () => { }); }); + it("opens an exact contained session id without listing every transcript", async () => { + const root = await mkdtemp(path.join(os.tmpdir(), "spirecode-pi-adapter-")); + temporaryDirectories.push(root); + const sessions = path.join(root, "sessions"); + const sessionPath = path.join(sessions, "s1.jsonl"); + await mkdir(sessions); + await writeFile(sessionPath, "{}\n"); + const { sdk, loadResources, manager } = sdkFixture({}); + manager.getCwd = () => root; + manager.getSessionDir = () => sessions; + const list = vi.fn(async () => []); + sdk.SessionManager.list = list; + sdk.SessionManager.findById = vi.fn(() => sessionPath); + const adapter = await createPiAdapter(sdk, { loadResources }); + + await expect(adapter.openById(root, "s1")).resolves.toMatchObject({ + sessionId: "s1", + }); + expect(list).not.toHaveBeenCalled(); + expect(sdk.SessionManager.findById).toHaveBeenCalledWith(root, "s1"); + }); + it("resolves deletion only from complete raw SDK metadata", async () => { const { sdk, manager, loadResources } = sdkFixture({}); sdk.SessionManager.list = async () => [ diff --git a/electron/domains/chat/piAdapter.ts b/electron/domains/chat/piAdapter.ts index e4c13e2..fd3f46d 100644 --- a/electron/domains/chat/piAdapter.ts +++ b/electron/domains/chat/piAdapter.ts @@ -14,6 +14,8 @@ import { } from "@earendil-works/pi-coding-agent"; import { bootstrapArkApiKeyFromLoginShell } from "./shellEnvironment.js"; import { loadSpireSettings, resolveSpireResources } from "./spireSettings.js"; +import { realpath, stat } from "node:fs/promises"; +import path from "node:path"; import type { ChatSessionConfig, ChatSlashCommand, @@ -64,6 +66,10 @@ export interface PiAdapter { cwd: string, sessionId: string, ): Promise<{ info: PiSessionInfo; sessionRoot: string } | undefined>; + openById( + cwd: string, + sessionId: string, + ): Promise; open(info: PiSessionInfo, cwd: string): Promise; } @@ -87,6 +93,7 @@ export interface PiSettingsManager { interface PiSessionManager { buildSessionContext(): { messages: unknown[] }; + getCwd(): string; getBranch(): unknown[]; getSessionDir(): string; } @@ -114,6 +121,7 @@ export interface PiSdk { SessionManager: { create(cwd: string): PiSessionManager; list(cwd: string): Promise>>; + findById(cwd: string, id: string): string | undefined; open( path: string, sessionDir?: string, @@ -423,6 +431,33 @@ export async function createPiAdapter( sessionRoot: sdk.SessionManager.create(cwd).getSessionDir(), }; }, + async openById(cwd, sessionId) { + const discoveredPath = sdk.SessionManager.findById(cwd, sessionId); + if (!discoveredPath) return undefined; + const [sessionPath, sessionRoot] = await Promise.all([ + realpath(discoveredPath), + realpath(sdk.SessionManager.create(cwd).getSessionDir()), + ]); + if ( + !(await stat(sessionPath)).isFile() || + !(await stat(sessionRoot)).isDirectory() || + !isWithin(sessionRoot, sessionPath) + ) + throw new Error("Session path is unavailable"); + const sessionManager = sdk.SessionManager.open(sessionPath, sessionRoot); + const [sessionCwd, requestedCwd] = await Promise.all([ + realpath(sessionManager.getCwd()), + realpath(cwd), + ]); + if (sessionCwd !== requestedCwd) + throw new Error("Session belongs to another worktree"); + const record = await load(sessionManager, cwd); + if (record.sessionId !== sessionId) { + await record.session.dispose(); + throw new Error("Agent SDK opened an unexpected session"); + } + return record; + }, async open(info, cwd) { if (!info.path) throw new Error("Session path is unavailable"); return load( @@ -642,6 +677,16 @@ function dateValue(value: unknown): number | undefined { : undefined; } +function isWithin(root: string, candidate: string): boolean { + const relative = path.relative(root, candidate); + return ( + relative === "" || + (!relative.startsWith(`..${path.sep}`) && + relative !== ".." && + !path.isAbsolute(relative)) + ); +} + function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } diff --git a/electron/domains/diagnostics/service.test.ts b/electron/domains/diagnostics/service.test.ts index ac55df9..80da701 100644 --- a/electron/domains/diagnostics/service.test.ts +++ b/electron/domains/diagnostics/service.test.ts @@ -49,6 +49,29 @@ describe("diagnostics privacy", () => { expect(output).toContain(""); }); + it("serializes only allowlisted performance fields", async () => { + const diagnostics = await service(); + await diagnostics.logPerformance({ + code: "PERF_CHAT_ATTACH", + outcome: "ok", + totalMs: 42.6, + cold: true, + counts: { sourceItemCount: 2500, returnedItemCount: 2000 }, + path: "/Volumes/private/repository", + prompt: "do-not-copy", + sessionId: "secret-session", + } as Parameters[0]); + + const report = await diagnostics.copyText(); + + expect(report).toContain('"code":"PERF_CHAT_ATTACH"'); + expect(report).toContain('"totalMs":43'); + expect(report).toContain('"sourceItemCount":2500'); + expect(report).not.toContain("/Volumes/private/repository"); + expect(report).not.toContain("do-not-copy"); + expect(report).not.toContain("secret-session"); + }); + it("copies only sanitized bounded log events", async () => { const diagnostics = await service(); await diagnostics.log({ diff --git a/electron/domains/diagnostics/service.ts b/electron/domains/diagnostics/service.ts index 805bbb2..77fbc3c 100644 --- a/electron/domains/diagnostics/service.ts +++ b/electron/domains/diagnostics/service.ts @@ -22,6 +22,38 @@ export interface DiagnosticEvent { }>; } +export type PerformanceDiagnosticEvent = + | { + code: "PERF_APP_STATE"; + outcome: "ok" | "error"; + totalMs: number; + counts: { projectCount: number; worktreeCount: number }; + } + | { + code: "PERF_WATCHER_BATCH"; + outcome: "ok" | "partial" | "error"; + totalMs: number; + counts: { + scheduled: number; + succeeded: number; + failed: number; + concurrency: number; + }; + } + | { + code: "PERF_PROJECT_OPEN"; + outcome: "ok" | "error"; + totalMs: number; + counts: { worktreeCount: number }; + } + | { + code: "PERF_CHAT_ATTACH"; + outcome: "ok" | "error"; + totalMs: number; + cold: boolean; + counts: { sourceItemCount: number; returnedItemCount: number }; + }; + export class DiagnosticsService { readonly logsDirectory: string; private readonly logPath: string; @@ -33,6 +65,20 @@ export class DiagnosticsService { this.logPath = path.join(this.logsDirectory, "spirecode.log"); } async log(event: DiagnosticEvent): Promise { + const errorType = event.safeContext?.errorType; + const safeContext = + typeof errorType === "string" && + /^[A-Za-z][A-Za-z0-9_.-]{0,63}$/.test(errorType) + ? { errorType } + : undefined; + await this.write({ + timestamp: new Date().toISOString(), + level: event.level, + code: event.code, + ...(safeContext ? { safeContext } : {}), + }); + } + private async write(entry: object): Promise { try { await mkdir(this.logsDirectory, { recursive: true }); try { @@ -41,18 +87,6 @@ export class DiagnosticsService { } catch { /* first log */ } - const errorType = event.safeContext?.errorType; - const safeContext = - typeof errorType === "string" && - /^[A-Za-z][A-Za-z0-9_.-]{0,63}$/.test(errorType) - ? { errorType } - : undefined; - const entry = { - timestamp: new Date().toISOString(), - level: event.level, - code: event.code, - ...(safeContext ? { safeContext } : {}), - }; await appendFile( this.logPath, `${sanitize(JSON.stringify(entry))}\n`, @@ -62,6 +96,43 @@ export class DiagnosticsService { /* diagnostics must never break the app */ } } + async logPerformance(event: PerformanceDiagnosticEvent): Promise { + const duration = boundedInteger(event.totalMs); + if (duration === undefined) return; + const counts = + event.code === "PERF_APP_STATE" + ? { + projectCount: boundedInteger(event.counts.projectCount) ?? 0, + worktreeCount: boundedInteger(event.counts.worktreeCount) ?? 0, + } + : event.code === "PERF_WATCHER_BATCH" + ? { + scheduled: boundedInteger(event.counts.scheduled) ?? 0, + succeeded: boundedInteger(event.counts.succeeded) ?? 0, + failed: boundedInteger(event.counts.failed) ?? 0, + concurrency: boundedInteger(event.counts.concurrency) ?? 0, + } + : event.code === "PERF_PROJECT_OPEN" + ? { worktreeCount: boundedInteger(event.counts.worktreeCount) ?? 0 } + : { + sourceItemCount: + boundedInteger(event.counts.sourceItemCount) ?? 0, + returnedItemCount: + boundedInteger(event.counts.returnedItemCount) ?? 0, + }; + await this.write({ + timestamp: new Date().toISOString(), + level: "info", + code: event.code, + performance: { + schemaVersion: 1, + outcome: event.outcome, + totalMs: duration, + ...("cold" in event ? { cold: event.cold === true } : {}), + counts, + }, + }); + } async revealLogs(): Promise { const error = await shell.openPath(this.logsDirectory); if (error) throw new Error(error); @@ -89,6 +160,12 @@ export class DiagnosticsService { ].join("\n"); } } +function boundedInteger(value: number): number | undefined { + if (!Number.isFinite(value) || value < 0 || value > 86_400_000) + return undefined; + return Math.round(value); +} + export const sanitize = (value: string): string => { const home = os.homedir(); return value diff --git a/electron/domains/filesystem/watcher.ts b/electron/domains/filesystem/watcher.ts index 85b32d4..b602a2a 100644 --- a/electron/domains/filesystem/watcher.ts +++ b/electron/domains/filesystem/watcher.ts @@ -31,6 +31,8 @@ export function watcherBackendForPlatform( export class WatcherRegistry { private readonly sessions = new Map(); + private readonly generations = new Map(); + private disposed = false; async ensure( worktreeId: string, @@ -38,7 +40,8 @@ export class WatcherRegistry { gitDirectory: string, sink: (event: WatchEvent) => void, ): Promise { - if (this.sessions.has(worktreeId)) return; + if (this.disposed || this.sessions.has(worktreeId)) return; + const generation = this.generations.get(worktreeId) ?? 0; const root = await realpath(rootPath); let gitDir = gitDirectory; try { @@ -119,7 +122,11 @@ export class WatcherRegistry { `filesystem watcher failed: ${error instanceof Error ? error.message : String(error)}`, ); } - if (this.sessions.has(worktreeId)) { + if ( + this.disposed || + this.sessions.has(worktreeId) || + (this.generations.get(worktreeId) ?? 0) !== generation + ) { await stopWatcher(); return; } @@ -133,6 +140,10 @@ export class WatcherRegistry { } async close(worktreeId: string): Promise { + this.generations.set( + worktreeId, + (this.generations.get(worktreeId) ?? 0) + 1, + ); const session = this.sessions.get(worktreeId); if (!session) return; this.sessions.delete(worktreeId); @@ -140,6 +151,7 @@ export class WatcherRegistry { } async dispose(): Promise { + this.disposed = true; await Promise.all([...this.sessions].map(([id]) => this.close(id))); } } diff --git a/electron/ipc.ts b/electron/ipc.ts index e869860..4e4a89a 100644 --- a/electron/ipc.ts +++ b/electron/ipc.ts @@ -1,7 +1,6 @@ import { homedir } from "node:os"; import { ipcMain, type IpcMainEvent, type IpcMainInvokeEvent } from "electron"; import { isCommand, type CommandName } from "./contracts.js"; -import { KeyedQueue } from "./core/asyncQueue.js"; import { serializeError } from "./core/errors.js"; import { AppState } from "./appState.js"; import type { ChatThinkingLevel } from "./domains/chat/types.js"; @@ -77,7 +76,6 @@ export function registerIpc( locationPolicy: RendererLocationPolicy, ): () => void { const chatSubscriptions = new Map(); - const chatAttachQueues = new KeyedQueue(); const handler = async ( event: IpcMainInvokeEvent, rawCommand: unknown, @@ -91,14 +89,7 @@ export function registerIpc( validateCommandArgs(rawCommand, args); return { ok: true, - value: await invoke( - state, - rawCommand, - args, - event, - chatSubscriptions, - chatAttachQueues, - ), + value: await invoke(state, rawCommand, args, event, chatSubscriptions), }; } catch (error) { return { ok: false, error: serializeError(error) }; @@ -127,7 +118,6 @@ async function invoke( args: Args, event: IpcMainInvokeEvent, chatSubscriptions: Map, - chatAttachQueues: KeyedQueue, ): Promise { switch (command) { case "project_list": @@ -240,24 +230,22 @@ async function invoke( const sessionId = text(args, "sessionId"); const subscriptionId = text(args, "subscriptionId"); chatSubscriptions.set(sessionId, subscriptionId); - return chatAttachQueues.run(sessionId, async () => { - try { - return await state.chat.attach(worktreeId, sessionId, (payload) => { - if ( - chatSubscriptions.get(sessionId) === subscriptionId && - !event.sender.isDestroyed() - ) - event.sender.send("spire:event:chat://event", { - subscriptionId, - payload, - }); - }); - } catch (error) { - if (chatSubscriptions.get(sessionId) === subscriptionId) - chatSubscriptions.delete(sessionId); - throw error; - } - }); + try { + return await state.chat.attach(worktreeId, sessionId, (payload) => { + if ( + chatSubscriptions.get(sessionId) === subscriptionId && + !event.sender.isDestroyed() + ) + event.sender.send("spire:event:chat://event", { + subscriptionId, + payload, + }); + }); + } catch (error) { + if (chatSubscriptions.get(sessionId) === subscriptionId) + chatSubscriptions.delete(sessionId); + throw error; + } } case "chat_session_detach": { const worktreeId = text(args, "worktreeId");