diff --git a/packages/pi-coding-agent/src/modes/interactive/components/tool-execution.ts b/packages/pi-coding-agent/src/modes/interactive/components/tool-execution.ts index 10bd5f02c..a5f80da87 100644 --- a/packages/pi-coding-agent/src/modes/interactive/components/tool-execution.ts +++ b/packages/pi-coding-agent/src/modes/interactive/components/tool-execution.ts @@ -137,6 +137,15 @@ export class ToolExecutionComponent extends Container { return isBuiltInName && !hasCustomRenderers; } + dispose(): void { + this.convertedImages.clear(); + this.imageComponents = []; + this.imageSpacers = []; + this.editDiffPreview = undefined; + this.writeHighlightCache = undefined; + this.result = undefined; + } + updateArgs(args: any): void { this.args = args; if (this.toolName === "write" && this.isPartial) { diff --git a/packages/pi-coding-agent/src/modes/interactive/interactive-mode.ts b/packages/pi-coding-agent/src/modes/interactive/interactive-mode.ts index d3bd71f27..ca98db678 100644 --- a/packages/pi-coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/pi-coding-agent/src/modes/interactive/interactive-mode.ts @@ -7,6 +7,7 @@ import * as crypto from "node:crypto"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; +import { listDescendants } from "@gsd/native"; import type { AgentMessage } from "@gsd/pi-agent-core"; import type { AssistantMessage, ImageContent, Message, Model, OAuthProviderId } from "@gsd/pi-ai"; import type { @@ -157,6 +158,10 @@ export interface InteractiveModeOptions { } export class InteractiveMode { + // Cap rendered chat components to prevent unbounded memory/CPU growth. + // Only render-components are removed — session transcript stays on disk. + private static readonly MAX_CHAT_COMPONENTS = 100; + private session: AgentSession; private ui: TUI; private chatContainer: Container; @@ -2138,6 +2143,18 @@ export class InteractiveMode { const _exhaustive: never = message; } } + this.trimChatHistory(); + } + + /** + * Remove oldest components when chat exceeds MAX_CHAT_COMPONENTS. + * Only render-components are removed — session data stays in SessionManager. + */ + private trimChatHistory(): void { + while (this.chatContainer.children.length > InteractiveMode.MAX_CHAT_COMPONENTS) { + const oldest = this.chatContainer.children[0]; + this.chatContainer.removeChild(oldest); + } } /** @@ -2232,6 +2249,7 @@ export class InteractiveMode { } this.pendingTools.clear(); + this.trimChatHistory(); this.ui.requestRender(); } @@ -2325,6 +2343,21 @@ export class InteractiveMode { if (shutdownBehavior === "stop_ui") { return; } + + // Kill ALL descendant processes to prevent orphans (next-server, pnpm dev, etc.) + try { + const descendants = listDescendants(process.pid); + for (const childPid of descendants) { + try { process.kill(childPid, "SIGTERM"); } catch {} + } + if (descendants.length > 0) { + await new Promise(resolve => setTimeout(resolve, 500)); + for (const childPid of descendants) { + try { process.kill(childPid, "SIGKILL"); } catch {} + } + } + } catch {} + process.exit(0); } diff --git a/packages/pi-tui/src/components/loader.ts b/packages/pi-tui/src/components/loader.ts index a55a2570c..5115f8337 100644 --- a/packages/pi-tui/src/components/loader.ts +++ b/packages/pi-tui/src/components/loader.ts @@ -2,13 +2,16 @@ import type { TUI } from "../tui.js"; import { Text } from "./text.js"; /** - * Loader component that updates every 80ms with spinning animation + * Loader component that updates every 80ms with spinning animation. + * Frame rotation is isolated from message text to avoid invalidating + * Text's render cache (wrapTextWithAnsi, visibleWidth) on every tick. */ export class Loader extends Text { private frames = ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]; private currentFrame = 0; private intervalId: NodeJS.Timeout | null = null; private ui: TUI | null = null; + private _lastMessage: string = ""; constructor( ui: TUI, @@ -22,18 +25,38 @@ export class Loader extends Text { } render(width: number): string[] { - return ["", ...super.render(width)]; + // Only update Text content when message actually changes — + // frame rotation is prepended below without touching the cache + if (this.message !== this._lastMessage) { + this.setText(this.messageColorFn(this.message)); + this._lastMessage = this.message; + } + const messageLines = super.render(width); + // Shallow copy so we don't mutate cachedLines from Text + const result = ["", ...messageLines]; + // Prepend spinner frame to first content line + if (result.length > 1) { + const frame = this.frames[this.currentFrame]; + result[1] = this.spinnerColorFn(frame) + " " + result[1]; + } + return result; } start() { if (this.intervalId) { clearInterval(this.intervalId); } - this.updateDisplay(); + this.currentFrame = 0; this.intervalId = setInterval(() => { this.currentFrame = (this.currentFrame + 1) % this.frames.length; - this.updateDisplay(); + if (this.ui) { + this.ui.requestRender(); + } }, 80); + // Trigger initial render + if (this.ui) { + this.ui.requestRender(); + } } stop() { @@ -50,12 +73,6 @@ export class Loader extends Text { setMessage(message: string) { this.message = message; - this.updateDisplay(); - } - - private updateDisplay() { - const frame = this.frames[this.currentFrame]; - this.setText(`${this.spinnerColorFn(frame)} ${this.messageColorFn(this.message)}`); if (this.ui) { this.ui.requestRender(); } diff --git a/packages/pi-tui/src/components/text.ts b/packages/pi-tui/src/components/text.ts index efcf25b45..a9519bfdf 100644 --- a/packages/pi-tui/src/components/text.ts +++ b/packages/pi-tui/src/components/text.ts @@ -23,6 +23,7 @@ export class Text implements Component { } setText(text: string): void { + if (this.text === text) return; this.text = text; this.cachedText = undefined; this.cachedWidth = undefined; diff --git a/packages/pi-tui/src/tui.ts b/packages/pi-tui/src/tui.ts index 8e3db6f05..493c1c727 100644 --- a/packages/pi-tui/src/tui.ts +++ b/packages/pi-tui/src/tui.ts @@ -166,20 +166,33 @@ export interface OverlayHandle { */ export class Container implements Component { children: Component[] = []; + private _prevRender: string[] | null = null; addChild(component: Component): void { this.children.push(component); + this._prevRender = null; } removeChild(component: Component): void { const index = this.children.indexOf(component); if (index !== -1) { + const child = this.children[index]; this.children.splice(index, 1); + if ('dispose' in child && typeof (child as any).dispose === 'function') { + (child as any).dispose(); + } + this._prevRender = null; } } clear(): void { + for (const child of this.children) { + if ('dispose' in child && typeof (child as any).dispose === 'function') { + (child as any).dispose(); + } + } this.children = []; + this._prevRender = null; } invalidate(): void { @@ -194,6 +207,17 @@ export class Container implements Component { const rendered = child.render(width); for (let i = 0; i < rendered.length; i++) lines.push(rendered[i]); } + // Return stable reference if output unchanged — allows doRender() + // to skip ALL post-processing (isImageLine, applyLineResets, diffs) + const prev = this._prevRender; + if (prev && prev.length === lines.length) { + let same = true; + for (let i = 0; i < lines.length; i++) { + if (lines[i] !== prev[i]) { same = false; break; } + } + if (same) return prev; + } + this._prevRender = lines; return lines; } } @@ -222,6 +246,7 @@ export class TUI extends Container { private previousViewportTop = 0; // Track previous viewport top for resize-aware cursor moves private fullRedrawCount = 0; private stopped = false; + private _lastRenderedComponents: string[] | null = null; // Overlay stack for modal components rendered on top of base content private focusOrderCounter = 0; @@ -599,6 +624,13 @@ export class TUI extends Container { // Render all components to get new lines let newLines = this.render(width); + // Skip ALL post-processing if component output is unchanged. + // Container.render() returns the same array reference when stable. + if (newLines === this._lastRenderedComponents && this.overlayStack.length === 0) { + return; + } + this._lastRenderedComponents = newLines; + // Composite overlays into the rendered lines (before differential compare) if (this.overlayStack.length > 0) { newLines = compositeOverlays(newLines, this.overlayStack, width, height, this.maxLinesRendered); diff --git a/src/resources/extensions/bg-shell/bg-shell-lifecycle.ts b/src/resources/extensions/bg-shell/bg-shell-lifecycle.ts index 688db06c4..32ee56455 100644 --- a/src/resources/extensions/bg-shell/bg-shell-lifecycle.ts +++ b/src/resources/extensions/bg-shell/bg-shell-lifecycle.ts @@ -16,6 +16,7 @@ import { import { processes, pendingAlerts, + pushAlert, cleanupAll, cleanupSessionProcesses, persistManifest, @@ -37,19 +38,30 @@ export function registerBgShellLifecycle(pi: ExtensionAPI, state: BgShellSharedS } } - // Clean up on session shutdown - pi.on("session_shutdown", async () => { - cleanupAll(); - }); - // Register signal handlers to clean up bg processes on unexpected exit (fixes #428) const signalCleanup = () => { cleanupAll(); + // Also kill bash-tool spawned children that bg-shell doesn't track + try { + const { listDescendants } = require("@gsd/native") as typeof import("@gsd/native"); + const descendants = listDescendants(process.pid); + for (const childPid of descendants) { + try { process.kill(childPid, "SIGKILL"); } catch {} + } + } catch {} }; process.on("SIGTERM", signalCleanup); process.on("SIGINT", signalCleanup); process.on("beforeExit", signalCleanup); + // Clean up on session shutdown — remove signal handlers to prevent accumulation + pi.on("session_shutdown", async () => { + process.off("SIGTERM", signalCleanup); + process.off("SIGINT", signalCleanup); + process.off("beforeExit", signalCleanup); + cleanupAll(); + }); + // ── Compaction Awareness: Survive Context Resets ─────────────── /** Build a compact state summary of all alive processes for context re-injection */ @@ -65,7 +77,7 @@ export function registerBgShellLifecycle(pi: ExtensionAPI, state: BgShellSharedS return ` - id:${p.id} "${p.label}" [${p.processType}] status:${p.status} uptime:${formatUptime(Date.now() - p.startedAt)}${portInfo}${urlInfo}${errInfo}${groupInfo}`; }).join("\n"); - pendingAlerts.push( + pushAlert(null, `${reason} ${alive.length} background process(es) are still running:\n${processSummaries}\nUse bg_shell digest/output/kill with these IDs.` ); } @@ -150,7 +162,7 @@ export function registerBgShellLifecycle(pi: ExtensionAPI, state: BgShellSharedS ` - ${s.id}: ${s.label} (pid ${s.pid}, type: ${s.processType}${s.group ? `, group: ${s.group}` : ""})` ).join("\n"); - pendingAlerts.push( + pushAlert(null, `${surviving.length} background process(es) from previous session still running:\n${summary}\n Note: These processes are outside bg_shell's control. Kill them manually if needed.` ); } diff --git a/src/resources/extensions/bg-shell/process-manager.ts b/src/resources/extensions/bg-shell/process-manager.ts index db707fb40..659f13e26 100644 --- a/src/resources/extensions/bg-shell/process-manager.ts +++ b/src/resources/extensions/bg-shell/process-manager.ts @@ -33,6 +33,8 @@ export const processes = new Map(); /** Pending alerts to inject into the next agent context */ export let pendingAlerts: string[] = []; +const MAX_PENDING_ALERTS = 50; + /** Replace the pendingAlerts array (used by the extension entry point) */ export function setPendingAlerts(alerts: string[]): void { pendingAlerts = alerts; @@ -58,8 +60,12 @@ export function addEvent(bg: BgProcess, event: Omit): } } -export function pushAlert(bg: BgProcess, message: string): void { - pendingAlerts.push(`[bg:${bg.id} ${bg.label}] ${message}`); +export function pushAlert(bg: BgProcess | null, message: string): void { + const prefix = bg ? `[bg:${bg.id} ${bg.label}] ` : ""; + pendingAlerts.push(`${prefix}${message}`); + if (pendingAlerts.length > MAX_PENDING_ALERTS) { + pendingAlerts.splice(0, pendingAlerts.length - MAX_PENDING_ALERTS); + } } export function getInfo(p: BgProcess): BgProcessInfo { diff --git a/src/tests/session-memory-leaks.test.ts b/src/tests/session-memory-leaks.test.ts new file mode 100644 index 000000000..17a3590bb --- /dev/null +++ b/src/tests/session-memory-leaks.test.ts @@ -0,0 +1,144 @@ +/** + * Regression tests for CPU/memory leak fixes in long-running sessions. + * + * Structural tests that verify the fix patterns are present in source — + * NOT runtime integration tests. This approach is chosen because: + * - The leaks manifest over hours of real usage, not in unit test timescales + * - The fixes are defensive guards (caps, disposal, handler cleanup) + * - Structural verification catches regressions when code is refactored + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +// ── Helpers ────────────────────────────────────────────────────────── + +function readSource(relativePath: string): string { + return readFileSync(join(import.meta.dirname, "..", "..", relativePath), "utf-8"); +} + +function extractFunctionBody(src: string, name: string): string { + const fnStart = src.indexOf(name); + assert.ok(fnStart > -1, `${name} must exist in source`); + let depth = 0; + let fnEnd = -1; + for (let i = src.indexOf("{", fnStart); i < src.length; i++) { + if (src[i] === "{") depth++; + if (src[i] === "}") depth--; + if (depth === 0) { fnEnd = i; break; } + } + return src.slice(fnStart, fnEnd + 1); +} + +// ── TUI render-skip ───────────────────────────────────────────────── + +test("Container caches render output for stable-reference comparison", () => { + const src = readSource("packages/pi-tui/src/tui.ts"); + assert.ok( + src.includes("_prevRender"), + "Container must have _prevRender cache for render-skip optimization", + ); +}); + +test("TUI skips post-processing when component output is unchanged", () => { + const src = readSource("packages/pi-tui/src/tui.ts"); + assert.ok( + src.includes("_lastRenderedComponents"), + "TUI must track _lastRenderedComponents for reference-equality skip", + ); +}); + +// ── Loader frame isolation ────────────────────────────────────────── + +test("Loader does not call setText on every spinner tick", () => { + const src = readSource("packages/pi-tui/src/components/loader.ts"); + // The old pattern was: setText(`${frame} ${message}`) inside the interval + // The new pattern: only update Text when message changes, prepend frame in render() + assert.ok( + src.includes("_lastMessage"), + "Loader must track _lastMessage to avoid setText on every tick", + ); + // Verify the interval does NOT call setText or updateDisplay + const intervalMatch = src.match(/setInterval\s*\(\s*\(\)\s*=>\s*\{([^}]+)\}/s); + assert.ok(intervalMatch, "Loader must have a setInterval callback"); + const intervalBody = intervalMatch[1]; + assert.ok( + !intervalBody.includes("setText") && !intervalBody.includes("updateDisplay"), + "Loader interval must NOT call setText or updateDisplay — " + + "frame rotation should only trigger requestRender()", + ); +}); + +// ── Text cache guard ──────────────────────────────────────────────── + +test("Text.setText returns early when text is unchanged", () => { + const src = readSource("packages/pi-tui/src/components/text.ts"); + const setTextBody = extractFunctionBody(src, "setText("); + assert.ok( + setTextBody.includes("if (this.text === text) return"), + "setText must early-return when text is identical to prevent cache invalidation", + ); +}); + +// ── Chat component cap ────────────────────────────────────────────── + +test("InteractiveMode caps rendered chat components", () => { + const src = readSource("packages/pi-coding-agent/src/modes/interactive/interactive-mode.ts"); + assert.ok( + src.includes("MAX_CHAT_COMPONENTS"), + "InteractiveMode must define MAX_CHAT_COMPONENTS to prevent unbounded growth", + ); + assert.ok( + src.includes("trimChatHistory"), + "InteractiveMode must call trimChatHistory to enforce the cap", + ); +}); + +// ── ToolExecution dispose ─────────────────────────────────────────── + +test("ToolExecutionComponent has dispose() to clear heavy references", () => { + const src = readSource("packages/pi-coding-agent/src/modes/interactive/components/tool-execution.ts"); + assert.ok( + src.includes("dispose()"), + "ToolExecutionComponent must have dispose() for GC of image maps, diff previews, etc.", + ); +}); + +// ── Orphan process prevention ─────────────────────────────────────── + +test("InteractiveMode kills descendant processes on shutdown", () => { + const src = readSource("packages/pi-coding-agent/src/modes/interactive/interactive-mode.ts"); + assert.ok( + src.includes("listDescendants"), + "Shutdown must use listDescendants to find orphan child processes", + ); + assert.ok( + src.includes("SIGTERM") && src.includes("SIGKILL"), + "Shutdown must send SIGTERM then SIGKILL to descendants", + ); +}); + +// ── Signal handler accumulation ───────────────────────────────────── + +test("bg-shell removes signal handlers on session_shutdown", () => { + const src = readSource("src/resources/extensions/bg-shell/bg-shell-lifecycle.ts"); + assert.ok( + src.includes('process.off("SIGTERM"') || src.includes("process.off('SIGTERM'"), + "session_shutdown must remove SIGTERM handler to prevent accumulation", + ); + assert.ok( + src.includes('process.off("SIGINT"') || src.includes("process.off('SIGINT'"), + "session_shutdown must remove SIGINT handler to prevent accumulation", + ); +}); + +// ── Alert queue cap ───────────────────────────────────────────────── + +test("pendingAlerts has a maximum size cap", () => { + const src = readSource("src/resources/extensions/bg-shell/process-manager.ts"); + assert.ok( + src.includes("MAX_PENDING_ALERTS"), + "process-manager must cap pendingAlerts to prevent unbounded growth", + ); +});