From a77abb574b0ab5d3535b0a6b868bfc930883ffc6 Mon Sep 17 00:00:00 2001 From: arkon Date: Mon, 19 Jan 2026 11:19:13 +0100 Subject: [PATCH] fix: prevent orphaned claude processes and session memory leaks - killScreen now finds and kills all child processes before quitting screen to prevent claude processes from becoming orphaned (the main bug) - Add cleanupSession() method for comprehensive resource cleanup: respawn controllers, timers, batches, event listeners, and sessions - Fix /api/run endpoint to cleanup sessions after completion - Fix /api/quick-start error path to cleanup on failure - Add MAX_CONCURRENT_SESSIONS (50) limit to prevent unbounded growth - Update CLAUDE.md with improved documentation Co-Authored-By: Claude Opus 4.5 --- CLAUDE.md | 71 ++++++++++++++-------------- src/screen-manager.ts | 52 ++++++++++++++++++++- src/web/server.ts | 104 ++++++++++++++++++++++++++++-------------- 3 files changed, 159 insertions(+), 68 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e233fd8a..a8049902 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,7 +6,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Claudeman is a Claude Code session manager with a web interface and autonomous Ralph Loop. It spawns Claude CLI processes via PTY, streams output in real-time via SSE, and supports scheduled/timed runs. -**Tech Stack**: TypeScript (ES2022/NodeNext), Node.js, Fastify, Server-Sent Events, node-pty +**Tech Stack**: TypeScript (ES2022/NodeNext, strict mode), Node.js, Fastify, Server-Sent Events, node-pty **Requirements**: Node.js 18+, Claude CLI (`claude`) installed and available in PATH @@ -22,35 +22,42 @@ npx tsx src/index.ts web -p 8080 # Dev mode with custom port node dist/index.js web # After npm run build claudeman web # After npm link -# IMPORTANT: `npm run dev` runs the CLI help, NOT the web server +# GOTCHA: `npm run dev` runs CLI help, NOT the web server # Always use `npx tsx src/index.ts web` for development -# Testing (vitest) +# Testing (vitest with vi.mock() - no real Claude CLI spawned) npm run test # Run all tests once npm run test:watch # Watch mode npm run test:coverage # With coverage report npx vitest run test/session.test.ts # Single file npx vitest run -t "should create session" # By pattern + +# Debugging +screen -ls # List GNU screen sessions +screen -r # Attach to screen session +curl localhost:3000/api/sessions # Check active sessions ``` ## Architecture ``` src/ -├── index.ts # CLI entry point (commander) -├── cli.ts # CLI command implementations +├── index.ts # CLI entry (commander) +├── cli.ts # CLI commands ├── session.ts # Core: PTY wrapper for Claude CLI + token tracking ├── session-manager.ts # Manages multiple sessions -├── screen-manager.ts # GNU screen session persistence + process stats +├── screen-manager.ts # GNU screen persistence + process stats ├── respawn-controller.ts # Auto-respawn state machine -├── task-tracker.ts # Background task detection and tree display ├── ralph-loop.ts # Autonomous task assignment -├── task.ts / task-queue.ts # Priority queue with dependencies +├── task-queue.ts # Priority queue with dependencies ├── state-store.ts # Persistence to ~/.claudeman/state.json ├── types.ts # All TypeScript interfaces ├── web/ │ ├── server.ts # Fastify REST API + SSE + session restoration -│ └── public/ # Static frontend files (vanilla JS, xterm.js) +│ └── public/ # Vanilla JS frontend (xterm.js, no bundler) +│ ├── app.js # Main app logic, SSE handling, tab management +│ ├── styles.css # All styles including responsive/mobile +│ └── index.html # Single page with modal templates └── templates/ └── claude-md.ts # CLAUDE.md generator for new cases ``` @@ -64,18 +71,18 @@ src/ ### Key Components -- **Session** (`src/session.ts`): Wraps Claude CLI as PTY subprocess. Two modes: `runPrompt(prompt)` for one-shot, `startInteractive()` for persistent terminal. Emits `output`, `terminal`, `message`, `completion`, `exit`, `idle`, `working`, `autoClear`, `clearTerminal` events. +- **Session** (`src/session.ts`): Wraps Claude CLI as PTY subprocess. Two modes: `runPrompt(prompt)` for one-shot, `startInteractive()` for persistent terminal. Emits events: `output`, `terminal`, `message`, `completion`, `exit`, `idle`, `working`, `autoClear`, `clearTerminal`. -- **RespawnController** (`src/respawn-controller.ts`): State machine that keeps interactive sessions productive. Detects idle → sends update prompt → optionally `/clear` → optionally `/init` → repeats. +- **RespawnController** (`src/respawn-controller.ts`): State machine that keeps sessions productive. Detects idle → sends update prompt → optionally `/clear` → optionally `/init` → repeats. State flow: `WATCHING → SENDING_UPDATE → WAITING_UPDATE → SENDING_CLEAR → WAITING_CLEAR → SENDING_INIT → WAITING_INIT → WATCHING` -- **ScreenManager** (`src/screen-manager.ts`): Manages GNU screen sessions for persistent terminals. Screens survive server restarts. +- **ScreenManager** (`src/screen-manager.ts`): Wraps sessions in GNU screen for persistence across server restarts. On startup, reconciles with `screen -ls` to restore sessions. -- **WebServer** (`src/web/server.ts`): Fastify server with REST API + SSE. All endpoints under `/api/`. See file for full route list. +- **WebServer** (`src/web/server.ts`): Fastify server with REST API (`/api/*`) + SSE (`/api/events`). Wires session events to SSE broadcast. ### Session Modes -- **One-Shot** (`runPrompt(prompt)`): Execute single prompt, receive completion event, session exits -- **Interactive** (`startInteractive()`): Persistent PTY terminal with resize support, buffer persistence +- **One-Shot** (`runPrompt(prompt)`): Single prompt execution, emits completion, exits +- **Interactive** (`startInteractive()`): Persistent PTY terminal with resize support - **Shell** (`startShell()`): Plain bash/zsh terminal without Claude ## Code Patterns @@ -104,38 +111,35 @@ pty.spawn('claude', ['--dangerously-skip-permissions'], { ... }) ### Idle Detection -Session detects idle by watching for prompt character (`❯` or `\u276f`) and waiting 2 seconds without activity. RespawnController uses the same patterns plus spinner characters to detect working state. +Session detects idle by watching for prompt character (`❯` or `\u276f`) and waiting 2 seconds without activity. ### Token Tracking - **One-shot mode**: Uses `--output-format stream-json` for detailed token usage from JSON - **Interactive mode**: Parses tokens from Claude's status line (e.g., "123.4k tokens"), estimates 60/40 input/output split -### Respawn Controller State Machine +### Terminal Display Fix (Tab Switch & New Session) -``` -WATCHING → SENDING_UPDATE → WAITING_UPDATE → SENDING_CLEAR → WAITING_CLEAR → SENDING_INIT → WAITING_INIT → WATCHING -``` - -### Screen Session Initialization - -GNU screen creates blank space at top when initializing. After attaching: -- **Claude sessions**: Clear buffer, emit `clearTerminal` event, client clears xterm -- **Shell sessions**: Clear buffer, send `clear\n` command - -### Tab Switching Fix - -When switching Claude session tabs, terminal may be rendered at wrong size. Fix sequence: +When switching tabs or creating new sessions, terminal may be rendered at wrong size. Fix sequence: 1. Clear and reset xterm 2. Write terminal buffer 3. Send resize to update PTY dimensions 4. Send Ctrl+L (`\x0c`) to trigger Claude CLI redraw +Uses `pendingCtrlL` Set to track sessions needing the fix. Waits for `session:idle` or `session:working` SSE event before sending resize + Ctrl+L. + ### SSE Events All events broadcast to `/api/events` with format: `{ type: string, sessionId?: string, data: any }`. -Event prefixes: `session:`, `task:`, `respawn:`, `scheduled:`, `case:`, `init`. Key events: `session:idle`, `session:working`, `session:terminal`, `session:clearTerminal`, `session:completion`, `respawn:stateChanged`. +Event prefixes: `session:`, `task:`, `respawn:`, `scheduled:`, `case:`, `init`. Key events: `session:idle`, `session:working`, `session:terminal`, `session:clearTerminal`, `session:completion`. + +### Frontend (app.js) + +The frontend uses vanilla JS with xterm.js. Key patterns: +- **SSE handling**: `handleSSEEvent()` switch statement dispatches all event types +- **Tab management**: `switchToSession()` handles terminal buffer restore + resize +- **60fps rendering**: Server batches at 16ms intervals, client uses `requestAnimationFrame` ## Adding New Features @@ -158,7 +162,6 @@ Event prefixes: `session:`, `task:`, `respawn:`, `scheduled:`, `case:`, `init`. - State persists to `~/.claudeman/state.json` and `~/.claudeman/screens.json` - Cases created in `~/claudeman-cases/` by default -- Sessions wrapped in GNU screen for persistence across server restarts -- Tests use vitest with `vi.mock()` - no real Claude CLI spawned -- Long-running sessions (12-24+ hours) supported with automatic buffer trimming +- Kill All works on restored sessions: `Session.stop()` checks screenManager directly by session ID +- Long-running sessions (12-24+ hours) supported with automatic buffer trimming (5MB terminal, 2MB text, 1000 messages max) - E2E testing available via agent-browser (see `.claude/skills/e2e-test.md`) diff --git a/src/screen-manager.ts b/src/screen-manager.ts index 80ad371c..a0c6e4f8 100644 --- a/src/screen-manager.ts +++ b/src/screen-manager.ts @@ -115,13 +115,61 @@ export class ScreenManager extends EventEmitter { } } - // Kill a screen session + // Get all child process PIDs recursively + private getChildPids(pid: number): number[] { + const pids: number[] = []; + try { + const output = execSync(`pgrep -P ${pid}`, { + encoding: 'utf-8', + timeout: 5000 + }).trim(); + if (output) { + for (const childPid of output.split('\n').map(p => parseInt(p, 10)).filter(p => !isNaN(p))) { + pids.push(childPid); + // Recursively get grandchildren + pids.push(...this.getChildPids(childPid)); + } + } + } catch { + // No children or command failed + } + return pids; + } + + // Kill a screen session and all its child processes async killScreen(sessionId: string): Promise { const screen = this.screens.get(sessionId); if (!screen) { return false; } + // First, find and kill ALL child processes of the screen session + // This prevents orphaned claude processes when screen quits + const childPids = this.getChildPids(screen.pid); + console.log(`[ScreenManager] Killing screen ${screen.screenName} (PID ${screen.pid}) and ${childPids.length} child processes`); + + // Kill children in reverse order (deepest first) with SIGTERM then SIGKILL + for (const childPid of childPids.reverse()) { + try { + process.kill(childPid, 'SIGTERM'); + } catch { + // Process may already be dead + } + } + + // Give processes a moment to terminate gracefully + await new Promise(resolve => setTimeout(resolve, 200)); + + // Force kill any remaining children + for (const childPid of childPids) { + try { + process.kill(childPid, 'SIGKILL'); + } catch { + // Process already terminated + } + } + + // Now kill the screen session itself try { // Kill screen session by name execSync(`screen -S ${screen.screenName} -X quit`, { @@ -131,6 +179,8 @@ export class ScreenManager extends EventEmitter { // Try killing by PID if name-based kill failed try { process.kill(screen.pid, 'SIGTERM'); + await new Promise(resolve => setTimeout(resolve, 100)); + process.kill(screen.pid, 'SIGKILL'); } catch { // Already dead } diff --git a/src/web/server.ts b/src/web/server.ts index 4a24596f..03921144 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -52,6 +52,8 @@ const TASK_UPDATE_BATCH_INTERVAL = 100; const SCHEDULED_CLEANUP_INTERVAL = 5 * 60 * 1000; // Completed scheduled runs max age (1 hour) const SCHEDULED_RUN_MAX_AGE = 60 * 60 * 1000; +// Maximum concurrent sessions to prevent resource exhaustion +const MAX_CONCURRENT_SESSIONS = 50; export class WebServer extends EventEmitter { private app: FastifyInstance; @@ -128,6 +130,11 @@ export class WebServer extends EventEmitter { this.app.get('/api/sessions', async () => this.getSessionsState()); this.app.post('/api/sessions', async (req): Promise => { + // Prevent unbounded session creation + if (this.sessions.size >= MAX_CONCURRENT_SESSIONS) { + return { success: false, error: `Maximum concurrent sessions (${MAX_CONCURRENT_SESSIONS}) reached. Delete some sessions first.` }; + } + const body = req.body as CreateSessionRequest & { mode?: 'claude' | 'shell'; name?: string }; const workingDir = body.workingDir || process.cwd(); const session = new Session({ @@ -166,23 +173,12 @@ export class WebServer extends EventEmitter { const { id } = req.params as { id: string }; const query = req.query as { killScreen?: string }; const killScreen = query.killScreen !== 'false'; // Default to true - const session = this.sessions.get(id); - if (!session) { + if (!this.sessions.has(id)) { return { success: false, error: 'Session not found' }; } - // Stop respawn controller first - const controller = this.respawnControllers.get(id); - if (controller) { - controller.stop(); - this.respawnControllers.delete(id); - } - - await session.stop(killScreen); - this.sessions.delete(id); - this.terminalBatches.delete(id); - this.broadcast('session:deleted', { id }); + await this.cleanupSession(id, killScreen); return { success: true }; }); @@ -192,19 +188,8 @@ export class WebServer extends EventEmitter { let killed = 0; for (const id of sessionIds) { - const session = this.sessions.get(id); - if (session) { - // Stop respawn controller first - const controller = this.respawnControllers.get(id); - if (controller) { - controller.stop(); - this.respawnControllers.delete(id); - } - - await session.stop(); - this.sessions.delete(id); - this.terminalBatches.delete(id); - this.broadcast('session:deleted', { id }); + if (this.sessions.has(id)) { + await this.cleanupSession(id); killed++; } } @@ -530,8 +515,13 @@ export class WebServer extends EventEmitter { }; }); - // Quick run (create session, run prompt, return result) + // Quick run (create session, run prompt, return result, then cleanup) this.app.post('/api/run', async (req) => { + // Prevent unbounded session creation + if (this.sessions.size >= MAX_CONCURRENT_SESSIONS) { + return { success: false, error: `Maximum concurrent sessions (${MAX_CONCURRENT_SESSIONS}) reached.` }; + } + const { prompt, workingDir } = req.body as QuickRunRequest; const dir = workingDir || process.cwd(); @@ -543,8 +533,12 @@ export class WebServer extends EventEmitter { try { const result = await session.runPrompt(prompt); + // Clean up session after completion to prevent memory leak + await this.cleanupSession(session.id); return { success: true, sessionId: session.id, ...result }; } catch (err) { + // Clean up session on error too + await this.cleanupSession(session.id); return { success: false, sessionId: session.id, error: (err as Error).message }; } }); @@ -648,6 +642,11 @@ export class WebServer extends EventEmitter { // Quick Start: Create case (if needed) and start interactive session in one click this.app.post('/api/quick-start', async (req): Promise => { + // Prevent unbounded session creation + if (this.sessions.size >= MAX_CONCURRENT_SESSIONS) { + return { success: false, error: `Maximum concurrent sessions (${MAX_CONCURRENT_SESSIONS}) reached.` }; + } + const { caseName = 'testcase' } = req.body as QuickStartRequest; // Validate case name @@ -697,6 +696,8 @@ export class WebServer extends EventEmitter { caseName, }; } catch (err) { + // Clean up session on error to prevent orphaned resources + await this.cleanupSession(session.id); return { success: false, error: (err as Error).message }; } }); @@ -802,6 +803,40 @@ export class WebServer extends EventEmitter { } } + // Clean up all resources associated with a session + private async cleanupSession(sessionId: string, killScreen: boolean = true): Promise { + const session = this.sessions.get(sessionId); + + // Stop and remove respawn controller + const controller = this.respawnControllers.get(sessionId); + if (controller) { + controller.stop(); + controller.removeAllListeners(); + this.respawnControllers.delete(sessionId); + } + + // Clear respawn timer + const timerInfo = this.respawnTimers.get(sessionId); + if (timerInfo) { + clearTimeout(timerInfo.timer); + this.respawnTimers.delete(sessionId); + } + + // Clear batches + this.terminalBatches.delete(sessionId); + this.outputBatches.delete(sessionId); + this.taskUpdateBatches.delete(sessionId); + + // Stop session and remove listeners + if (session) { + session.removeAllListeners(); + await session.stop(killScreen); + this.sessions.delete(sessionId); + } + + this.broadcast('session:deleted', { id: sessionId }); + } + private setupSessionListeners(session: Session): void { session.on('output', (data) => { // Use batching for better performance at high throughput @@ -994,6 +1029,13 @@ export class WebServer extends EventEmitter { }; while (Date.now() < run.endAt && run.status === 'running') { + // Check session limit before creating new session + if (this.sessions.size >= MAX_CONCURRENT_SESSIONS) { + addLog(`Waiting: maximum concurrent sessions (${MAX_CONCURRENT_SESSIONS}) reached`); + await new Promise(r => setTimeout(r, 5000)); + continue; + } + let session: Session | null = null; try { // Create a session for this iteration @@ -1017,9 +1059,7 @@ export class WebServer extends EventEmitter { this.broadcast('scheduled:updated', run); // Clean up the session after iteration to prevent memory leaks - await session.stop(); - this.sessions.delete(session.id); - this.terminalBatches.delete(session.id); + await this.cleanupSession(session.id); run.sessionId = null; // Small pause between iterations @@ -1031,9 +1071,7 @@ export class WebServer extends EventEmitter { // Clean up the session on error too if (session) { try { - await session.stop(); - this.sessions.delete(session.id); - this.terminalBatches.delete(session.id); + await this.cleanupSession(session.id); } catch { // Ignore cleanup errors }