diff --git a/src/main/http/sessions.ts b/src/main/http/sessions.ts index c0244274..72f5262c 100644 --- a/src/main/http/sessions.ts +++ b/src/main/http/sessions.ts @@ -13,6 +13,7 @@ import { createLogger } from '@shared/utils/logger'; import { coercePageLimit, validateProjectId, validateSessionId } from '../ipc/guards'; +import { stripSessionDetailMessages } from '../services/analysis/sessionDetailPayload'; import { DataCache } from '../services/infrastructure/DataCache'; import type { SessionsByIdsOptions, SessionsPaginationOptions } from '../types'; @@ -188,11 +189,11 @@ export function registerSessionRoutes(app: FastifyInstance, services: HttpServic ); session.hasSubagents = subagents.length > 0; - // Build session detail with chunks - sessionDetail = services.chunkBuilder.buildSessionDetail( - session, - parsedSession.messages, - subagents + // Build session detail with chunks. Strip the raw `messages` array before + // caching/returning — the renderer only consumes chunks/processes/session, + // and including messages doubled IPC payload size and cache footprint. + sessionDetail = stripSessionDetailMessages( + services.chunkBuilder.buildSessionDetail(session, parsedSession.messages, subagents) ); // Cache the result @@ -318,10 +319,10 @@ export function registerSessionRoutes(app: FastifyInstance, services: HttpServic parsedSession.messages ); - detail = services.chunkBuilder.buildSessionDetail( - session, - parsedSession.messages, - subagents + // Strip the raw `messages` array before caching so the waterfall path + // cannot "poison" the cache entry reused by the session-detail route. + detail = stripSessionDetailMessages( + services.chunkBuilder.buildSessionDetail(session, parsedSession.messages, subagents) ); services.dataCache.set(cacheKey, detail); } diff --git a/src/main/ipc/sessions.ts b/src/main/ipc/sessions.ts index cea9d76e..1380a863 100644 --- a/src/main/ipc/sessions.ts +++ b/src/main/ipc/sessions.ts @@ -13,6 +13,7 @@ import { createLogger } from '@shared/utils/logger'; import { type IpcMain, type IpcMainInvokeEvent } from 'electron'; import { DataCache } from '../services'; +import { stripSessionDetailMessages } from '../services/analysis/sessionDetailPayload'; import { type ConversationGroup, type PaginatedSessionsResult, @@ -247,8 +248,12 @@ async function handleGetSessionDetail( ); session.hasSubagents = subagents.length > 0; - // Build session detail with chunks - sessionDetail = chunkBuilder.buildSessionDetail(session, parsedSession.messages, subagents); + // Build session detail with chunks. Strip the raw `messages` array before + // caching/returning — the renderer only consumes chunks/processes/session, + // and including messages doubled IPC payload size and cache footprint. + sessionDetail = stripSessionDetailMessages( + chunkBuilder.buildSessionDetail(session, parsedSession.messages, subagents) + ); // Cache the result dataCache.set(cacheKey, sessionDetail); diff --git a/src/main/services/analysis/sessionDetailPayload.ts b/src/main/services/analysis/sessionDetailPayload.ts new file mode 100644 index 00000000..82f1a84d --- /dev/null +++ b/src/main/services/analysis/sessionDetailPayload.ts @@ -0,0 +1,24 @@ +import type { SessionDetail } from '../../types'; + +/** + * Strip the raw `messages` array from a `SessionDetail` before it crosses the + * IPC / HTTP boundary to the renderer. + * + * The renderer consumes `session`, `chunks`, `processes`, and `metrics` only — + * `messages` is an implementation detail that `ChunkBuilder` retains for + * internal callers. Including it in the serialized payload roughly doubled + * the IPC cost for sessions with large JSONL files (tens of MB per response) + * while also inflating the in-memory `DataCache` footprint. The field is + * preserved (as an empty array) so the shared `SessionDetail` type stays + * satisfied and downstream code can still observe `.messages.length === 0` + * without runtime type narrowing. + */ +export function stripSessionDetailMessages(detail: SessionDetail): SessionDetail { + if (detail.messages.length === 0) { + return detail; + } + return { + ...detail, + messages: [], + }; +} diff --git a/src/main/services/team/TeamProvisioningService.ts b/src/main/services/team/TeamProvisioningService.ts index 7204faa5..ec407a9e 100644 --- a/src/main/services/team/TeamProvisioningService.ts +++ b/src/main/services/team/TeamProvisioningService.ts @@ -84,6 +84,7 @@ import { buildActionModeProtocol } from './actionModeInstructions'; import { atomicWriteAsync } from './atomicWrite'; import { ClaudeBinaryResolver } from './ClaudeBinaryResolver'; import { withFileLock } from './fileLock'; +import { buildProgressAssistantOutput, buildProgressLogsTail } from './progressPayload'; import { type ClassifiedMainProcessIdle, classifyIdleNotificationForMainProcess, @@ -195,7 +196,13 @@ const VERIFY_TIMEOUT_MS = 15_000; const VERIFY_POLL_MS = 500; const STDERR_RING_LIMIT = 64 * 1024; const STDOUT_RING_LIMIT = 64 * 1024; -const LOG_PROGRESS_THROTTLE_MS = 300; +// Progress emissions fan out the latest CLI tail + assistant output to the +// renderer over IPC. Under load the previous 300ms cadence combined with an +// unbounded payload (see `emitLogsProgress`) caused renderer OOM crashes +// (≈3 full-history serializations per second, each holding thousands of +// lines). The tail cap in `emitLogsProgress` bounds each payload; we also +// slow the cadence to ~1s so Zustand can keep up on large teams. +const LOG_PROGRESS_THROTTLE_MS = 1000; const UI_LOGS_TAIL_LIMIT = 128 * 1024; const PROBE_CACHE_TTL_MS = 36 * 60 * 60 * 1000; const PREFLIGHT_BINARY_TIMEOUT_MS = 8000; @@ -2025,10 +2032,12 @@ function updateProgress( 'pid' | 'error' | 'warnings' | 'cliLogsTail' | 'configReady' | 'messageSeverity' > ): TeamProvisioningProgress { + // Cap assistant output on every progress tick. `updateProgress` is invoked + // from ~20 event-driven sites (auth retries, stall warnings, spawn events), + // and an unbounded `provisioningOutputParts.join` was part of the same OOM + // class that `emitLogsProgress` already guards against. const assistantOutput = - run.provisioningOutputParts.length > 0 - ? run.provisioningOutputParts.join('\n\n') - : run.progress.assistantOutput; + buildProgressAssistantOutput(run.provisioningOutputParts) ?? run.progress.assistantOutput; run.progress = { ...run.progress, state, @@ -2093,10 +2102,22 @@ function extractCliLogsFromRun(run: ProvisioningRun): string | undefined { return extractLogsTail(run.stdoutBuffer, run.stderrBuffer); } +/** + * Emit a throttled progress update for the renderer. Payloads are capped to a + * tail window so that the hot emission path (called every LOG_PROGRESS_THROTTLE_MS + * under streaming output) cannot accumulate into multi-megabyte IPC messages + * that would OOM the renderer's Zustand state. The full history stays in + * `run.claudeLogLines` / `run.provisioningOutputParts` for diagnostics and + * one-shot completion emissions that intentionally use `extractCliLogsFromRun`. + */ function emitLogsProgress(run: ProvisioningRun): void { - const logsTail = extractCliLogsFromRun(run); - const assistantOutput = - run.provisioningOutputParts.length > 0 ? run.provisioningOutputParts.join('\n\n') : undefined; + // Prefer the line-buffered history (already chronological with [stdout]/[stderr] + // markers) and fall back to the legacy ring-buffer tail only when no lines + // have been captured yet (early in provisioning). + const logsTail = + buildProgressLogsTail(run.claudeLogLines) ?? + extractLogsTail(run.stdoutBuffer, run.stderrBuffer); + const assistantOutput = buildProgressAssistantOutput(run.provisioningOutputParts); if (!logsTail && !assistantOutput) { return; @@ -4453,7 +4474,9 @@ export class TeamProvisioningService { message: this.buildStallProgressMessage(silenceSec, elapsed), messageSeverity: 'warning' as const, }), - assistantOutput: run.provisioningOutputParts.join('\n\n'), + assistantOutput: + buildProgressAssistantOutput(run.provisioningOutputParts) ?? + run.progress.assistantOutput, }; run.onProgress(run.progress); } catch (err) { @@ -8772,7 +8795,9 @@ export class TeamProvisioningService { updatedAt: nowIso(), message: retryText, messageSeverity: 'error' as const, - assistantOutput: run.provisioningOutputParts.join('\n\n'), + assistantOutput: + buildProgressAssistantOutput(run.provisioningOutputParts) ?? + run.progress.assistantOutput, }; run.onProgress(run.progress); } diff --git a/src/main/services/team/progressPayload.ts b/src/main/services/team/progressPayload.ts new file mode 100644 index 00000000..c2f4fce7 --- /dev/null +++ b/src/main/services/team/progressPayload.ts @@ -0,0 +1,52 @@ +/** + * Helpers that shape provisioning progress payloads before they are emitted + * to the renderer over IPC. + * + * Rationale: the renderer only renders a small "tail" preview of CLI logs + * and assistant output in ProvisioningProgressBlock / CliLogsRichView. Sending + * the full accumulated history on every throttled progress tick (≈ every + * second under load) serialized a multi-megabyte string over IPC and forced + * Zustand to produce a new immutable state object — which triggered renderer + * V8 OOM crashes for users with long-running teams. These helpers keep the + * hot emission path bounded while leaving the full history in-process for + * diagnostics and completion-time reports. + */ + +export const PROGRESS_LOG_TAIL_LINES = 200; +export const PROGRESS_OUTPUT_TAIL_PARTS = 20; + +/** + * Return the trailing `maxLines` of a line-buffered CLI log, joined with "\n" + * and trimmed. Returns `undefined` when the tail is empty so callers can + * skip emitting a noop update. + */ +export function buildProgressLogsTail( + lines: readonly string[], + maxLines: number = PROGRESS_LOG_TAIL_LINES +): string | undefined { + if (lines.length === 0) { + return undefined; + } + const effectiveMax = Math.max(1, maxLines); + const tail = lines.length > effectiveMax ? lines.slice(-effectiveMax) : lines; + const joined = tail.join('\n').trim(); + return joined.length === 0 ? undefined : joined; +} + +/** + * Return the trailing `maxParts` of assistant output parts joined with a + * blank line, matching the renderer's rendering contract. Returns `undefined` + * when no parts are available. + */ +export function buildProgressAssistantOutput( + parts: readonly string[], + maxParts: number = PROGRESS_OUTPUT_TAIL_PARTS +): string | undefined { + if (parts.length === 0) { + return undefined; + } + const effectiveMax = Math.max(1, maxParts); + const tail = parts.length > effectiveMax ? parts.slice(-effectiveMax) : parts; + const joined = tail.join('\n\n'); + return joined.trim().length === 0 ? undefined : joined; +} diff --git a/src/shared/constants/cache.ts b/src/shared/constants/cache.ts index fff6edf0..3cfa3be1 100644 --- a/src/shared/constants/cache.ts +++ b/src/shared/constants/cache.ts @@ -2,8 +2,15 @@ * Cache-related constants. */ -/** Maximum number of sessions to cache */ -export const MAX_CACHE_SESSIONS = 5; +/** + * Maximum number of session details retained in the in-memory LRU cache. + * + * Users regularly juggle dozens of sessions — the previous cap of 5 caused + * constant re-parsing of large JSONL files on every session switch. Raised + * now that the per-entry footprint is bounded (the raw `messages` array is + * stripped before caching; see `stripSessionDetailMessages`). + */ +export const MAX_CACHE_SESSIONS = 20; /** Cache TTL in minutes */ export const CACHE_TTL_MINUTES = 5; diff --git a/test/main/services/analysis/sessionDetailPayload.test.ts b/test/main/services/analysis/sessionDetailPayload.test.ts new file mode 100644 index 00000000..0c094767 --- /dev/null +++ b/test/main/services/analysis/sessionDetailPayload.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from 'vitest'; + +import { stripSessionDetailMessages } from '../../../../src/main/services/analysis/sessionDetailPayload'; +import type { ParsedMessage, SessionDetail } from '../../../../src/main/types'; + +function createDetail(overrides: Partial = {}): SessionDetail { + return { + session: { + id: 'session-1', + projectId: 'project-1', + projectPath: '/tmp/project', + isOngoing: false, + hasSubagents: false, + messageCount: 0, + createdAt: 0, + }, + messages: [], + chunks: [], + processes: [], + metrics: { + durationMs: 0, + totalTokens: 0, + inputTokens: 0, + outputTokens: 0, + cacheReadTokens: 0, + cacheCreationTokens: 0, + messageCount: 0, + }, + ...overrides, + }; +} + +describe('stripSessionDetailMessages', () => { + it('returns the same reference when messages is already empty', () => { + const detail = createDetail(); + const result = stripSessionDetailMessages(detail); + expect(result).toBe(detail); + }); + + it('drops the messages array when it is non-empty', () => { + const messages = [{ uuid: 'm-1' } as unknown as ParsedMessage]; + const detail = createDetail({ messages }); + const result = stripSessionDetailMessages(detail); + expect(result).not.toBe(detail); + expect(result.messages).toEqual([]); + }); + + it('preserves every other field (session, chunks, processes, metrics)', () => { + const messages = Array.from( + { length: 3 }, + (_, i) => ({ uuid: `m-${i}` }) as unknown as ParsedMessage + ); + const detail = createDetail({ messages }); + const result = stripSessionDetailMessages(detail); + expect(result.session).toBe(detail.session); + expect(result.chunks).toBe(detail.chunks); + expect(result.processes).toBe(detail.processes); + expect(result.metrics).toBe(detail.metrics); + }); + + it('does not mutate the input detail', () => { + const messages = [{ uuid: 'm-1' } as unknown as ParsedMessage]; + const detail = createDetail({ messages }); + stripSessionDetailMessages(detail); + expect(detail.messages).toBe(messages); + expect(detail.messages).toHaveLength(1); + }); +}); diff --git a/test/main/services/team/progressPayload.test.ts b/test/main/services/team/progressPayload.test.ts new file mode 100644 index 00000000..8265d24e --- /dev/null +++ b/test/main/services/team/progressPayload.test.ts @@ -0,0 +1,77 @@ +import { describe, expect, it } from 'vitest'; + +import { + PROGRESS_LOG_TAIL_LINES, + PROGRESS_OUTPUT_TAIL_PARTS, + buildProgressAssistantOutput, + buildProgressLogsTail, +} from '../../../../src/main/services/team/progressPayload'; + +describe('buildProgressLogsTail', () => { + it('returns undefined for an empty buffer', () => { + expect(buildProgressLogsTail([])).toBeUndefined(); + }); + + it('returns undefined when all lines are whitespace', () => { + expect(buildProgressLogsTail(['', ' ', '\t'])).toBeUndefined(); + }); + + it('returns the full buffer joined when below the limit', () => { + const lines = ['alpha', 'beta', 'gamma']; + expect(buildProgressLogsTail(lines, 10)).toBe('alpha\nbeta\ngamma'); + }); + + it('caps the payload to the last N lines once the limit is exceeded', () => { + const lines = Array.from({ length: 1_000 }, (_, i) => `line-${i}`); + const result = buildProgressLogsTail(lines, 50); + expect(result).toBeDefined(); + const parts = result!.split('\n'); + expect(parts).toHaveLength(50); + expect(parts[0]).toBe('line-950'); + expect(parts[parts.length - 1]).toBe('line-999'); + }); + + it('uses the default tail size when the caller does not override it', () => { + const lines = Array.from({ length: PROGRESS_LOG_TAIL_LINES + 250 }, (_, i) => `l${i}`); + const result = buildProgressLogsTail(lines); + expect(result).toBeDefined(); + expect(result!.split('\n')).toHaveLength(PROGRESS_LOG_TAIL_LINES); + }); + + it('keeps payload size bounded for pathological inputs (50k lines)', () => { + const lines = Array.from({ length: 50_000 }, (_, i) => `line-${i}`); + const result = buildProgressLogsTail(lines); + expect(result).toBeDefined(); + // Regression guard: a full-buffer join of 50k synthetic lines would exceed + // 400k chars. The tail must stay well below that. + expect(result!.length).toBeLessThan(50_000); + }); + + it('coerces non-positive limits to at least one line', () => { + expect(buildProgressLogsTail(['a', 'b', 'c'], 0)).toBe('c'); + expect(buildProgressLogsTail(['a', 'b', 'c'], -5)).toBe('c'); + }); +}); + +describe('buildProgressAssistantOutput', () => { + it('returns undefined when there are no parts', () => { + expect(buildProgressAssistantOutput([])).toBeUndefined(); + }); + + it('joins parts with a blank-line separator when below the limit', () => { + expect(buildProgressAssistantOutput(['first', 'second'], 10)).toBe('first\n\nsecond'); + }); + + it('caps to the last N parts once the limit is exceeded', () => { + const parts = Array.from({ length: 200 }, (_, i) => `p${i}`); + const result = buildProgressAssistantOutput(parts, 5); + expect(result).toBe('p195\n\np196\n\np197\n\np198\n\np199'); + }); + + it('uses the default tail size when the caller does not override it', () => { + const parts = Array.from({ length: PROGRESS_OUTPUT_TAIL_PARTS + 10 }, (_, i) => `p${i}`); + const result = buildProgressAssistantOutput(parts); + expect(result).toBeDefined(); + expect(result!.split('\n\n')).toHaveLength(PROGRESS_OUTPUT_TAIL_PARTS); + }); +});