From f355e09d7876aecb64ed33db6161944181465df3 Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Fri, 30 Jan 2026 11:38:16 +0100 Subject: [PATCH] Fix pty.node SIGABRT crash on macOS shutdown (#1619) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * auto-claude: subtask-1-1 - Document root cause analysis of pty.node SIGABRT crash Trace the code path from before-quit → destroyAllTerminals() → killPty(terminal) (fire-and-forget, no wait) → app quits → environment cleanup → ThreadSafeFunction callback fires → SIGABRT. INVESTIGATION.md documents: - Complete root cause chain with code evidence - Race window timeline showing the ~100ms gap - Why Electron's async before-quit handler doesn't actually block quit - Proposed fix strategy (shutdown flag, wait-for-exit, preventDefault pattern) Co-Authored-By: Claude Opus 4.5 * auto-claude: subtask-2-1 - Add isShuttingDown flag and guards to pty-manager Add isShuttingDown flag with setShuttingDown()/getIsShuttingDown() exports to pty-manager.ts. Guard onData handler to early-return during shutdown. Guard onExit handler to skip win.webContents access and onExitCallback during shutdown, while still resolving pendingExitPromises for waitForPtyExit callers. Follows the isShuttingDown pattern from pty-daemon-client.ts. Co-Authored-By: Claude Opus 4.5 * auto-claude: subtask-3-1 - Refactor destroyAllTerminals() to wait for PTY exit - Set shutdown flag via PtyManager.setShuttingDown(true) before killing terminals - Use killPty(terminal, true) to wait for each PTY process to exit - Wrap all kill promises in Promise.race with 3s global timeout to prevent hangs Co-Authored-By: Claude Opus 4.5 * auto-claude: subtask-4-1 - Enhance before-quit handler with async PTY cleanup Add re-entrancy guarded before-quit handler that uses event.preventDefault() to pause quit while async PTY cleanup completes. This prevents pty.node SIGABRT crashes caused by native ThreadSafeFunction callbacks firing after JS environment teardown begins (GitHub #1469). Changes: - Add isQuitting module-level re-entrancy guard flag - Use event.preventDefault() to pause quit for async cleanup - Await terminalManager.killAll() (which now waits for PTY exit) - Explicitly call ptyDaemonClient.shutdown() after terminal cleanup - Wrap in try/catch with finally ensuring app.quit() always proceeds Co-Authored-By: Claude Opus 4.5 * auto-claude: subtask-5-1 - Add defensive shutdown guards to pty-daemon.ts and pty-daemon-client.ts Co-Authored-By: Claude Opus 4.5 * auto-claude: subtask-5-3 - Clean up INVESTIGATION.md artifacts, add GitHub #1469 references Remove working INVESTIGATION.md files and add inline comments referencing the shutdown guard pattern and GitHub issue #1469 for future maintainers. Co-Authored-By: Claude Opus 4.5 * Fix shutdown race condition and improve error logging Remove redundant before-quit handler in pty-daemon-client.ts that was causing a race condition during app shutdown. The daemon is already properly shut down in index.ts after terminal cleanup completes. Add diagnostic logging to PTY cleanup error handler in terminal-lifecycle.ts to improve observability during shutdown failures. Co-Authored-By: Claude Opus 4.5 * fix(security): sanitize PTY IDs in log statements to prevent log injection Add sanitizeIdForLog helper to remove control characters and truncate user-controlled PTY IDs before logging. This prevents log injection attacks where malicious IDs could corrupt log output or spoof entries. Addresses CodeQL alerts: - Use of externally-controlled format string (HIGH) - Log injection (MEDIUM) Co-Authored-By: Claude Opus 4.5 * fix(security): use CodeQL-recognized sanitization pattern for PTY logging Refactor log statements to avoid template literal interpolation of user data: - Use JSON.stringify in sanitizer (recognized by CodeQL as sanitizer) - Pass sanitized IDs as separate console arguments instead of interpolating - This separates the format string (literal) from user data This pattern eliminates CodeQL alerts for: - Use of externally-controlled format string (HIGH) - Log injection (MEDIUM) Co-Authored-By: Claude Opus 4.5 --------- Co-authored-by: Claude Opus 4.5 --- apps/backend/INVESTIGATION.md | 421 ------------------ apps/frontend/src/main/index.ts | 57 ++- .../src/main/terminal/pty-daemon-client.ts | 38 +- apps/frontend/src/main/terminal/pty-daemon.ts | 59 ++- .../frontend/src/main/terminal/pty-manager.ts | 37 +- .../src/main/terminal/terminal-lifecycle.ts | 42 +- 6 files changed, 183 insertions(+), 471 deletions(-) delete mode 100644 apps/backend/INVESTIGATION.md diff --git a/apps/backend/INVESTIGATION.md b/apps/backend/INVESTIGATION.md deleted file mode 100644 index 699787ce..00000000 --- a/apps/backend/INVESTIGATION.md +++ /dev/null @@ -1,421 +0,0 @@ -# Token Encryption Investigation - -## Issue Summary - -Auto-Claude users are experiencing API 401 errors ("Invalid bearer token") because the Python backend is passing encrypted tokens (with `enc:` prefix) directly to the Claude Agent SDK without decryption. Standalone Claude Code terminals work correctly because they decrypt these tokens before use. - -**Key insight from user thehaffk:** "python cant unencrypt claude token and it launches session with CLAUDE_CODE_OAUTH_TOKEN=enc:djEwtxMGISt3tQ..." - -## Token Storage Format - -### Encrypted Token Format - -Claude Code CLI stores OAuth tokens in an encrypted format with the prefix `enc:`: - -```text -enc:djEwtxMGISt3tQ... -``` - -This format is used when tokens are stored in: -- **macOS**: Keychain (service: "Claude Code-credentials") -- **Linux**: Secret Service API (DBus, via secretstorage library) -- **Windows**: Credential Manager / .credentials.json files - -### Decrypted Token Format - -Valid Claude OAuth tokens have the format: -```text -sk-ant-oat01-XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX -``` - -## Current Token Flow (BROKEN) - -1. **Token Storage**: Claude Code CLI stores encrypted token with `enc:` prefix in system keychain -2. **Token Retrieval**: `apps/backend/core/auth.py::get_auth_token()` retrieves token from: - - Environment variable `CLAUDE_CODE_OAUTH_TOKEN` - - OR system keychain via `get_token_from_keychain()` -3. **❌ NO DECRYPTION**: Token is returned as-is with `enc:` prefix intact -4. **SDK Initialization**: Encrypted token passed to Claude Agent SDK -5. **API Call Fails**: SDK sends encrypted token to API → 401 error - -### Proof of Broken Flow - -Test in `apps/backend`: -```python -import os -os.environ['CLAUDE_CODE_OAUTH_TOKEN'] = 'enc:test123' - -from core.auth import get_auth_token -token = get_auth_token() -print(f"Token: {token}") # Output: "enc:test123" -print(f"Encrypted: {token.startswith('enc:')}") # Output: True -``` - -## How Standalone Claude Code CLI Handles Tokens - -### Current Understanding - -1. **Token Detection**: CLI checks if token starts with `enc:` prefix -2. **Decryption**: If encrypted, CLI decrypts using platform-specific keyring access -3. **Authentication**: Decrypted `sk-ant-oat01-` token is used for API calls - -### Missing Documentation - -Web search for "Claude Code CLI encrypted token enc: prefix decryption" found: -- Token storage formats (JSON with accessToken, refreshToken, expiresAt) -- Security issues (tokens exposed in debug logs before v2.1.0) -- Keychain access patterns for macOS/Linux/Windows - -**❌ NOT FOUND**: Specific documentation on how Claude Code CLI decrypts `enc:` tokens - -Sources: -- [Claude Code CLI over SSH on macOS: Fixing Keychain Access](https://phoenixtrap.com/2025/10/26/claude-code-cli-over-ssh-on-macos-fixing-keychain-access/) -- [Identity and Access Management - Claude Code Docs](https://code.claude.com/docs/en/iam) -- [Claude Code sessions should be encrypted | yoav.blog](https://yoav.blog/2026/01/09/claude-code-sessions-should-be-encrypted/) - -## Decryption Approach Options - -### Option 1: Claude Agent SDK Built-in Decryption - -**Status**: NEEDS VERIFICATION - -The Claude Agent SDK (`claude-agent-sdk>=0.1.19`) may handle decryption internally if: -- Token is passed to SDK still encrypted -- SDK detects `enc:` prefix -- SDK has access to system keyring for decryption - -**Action Required**: Check if SDK has decryption capabilities by examining: -- SDK source code or documentation -- Whether SDK expects encrypted vs decrypted tokens -- If SDK requires specific environment variables for decryption - -### Option 2: Python Backend Decryption (Recommended) - -**Approach**: Implement decryption in `apps/backend/core/auth.py` before passing to SDK - -**Implementation Pattern**: -```python -def get_auth_token() -> str | None: - """Get authentication token (decrypted if necessary).""" - token = _retrieve_token_from_sources() # From env or keychain - - if token and token.startswith("enc:"): - # Decrypt the token - token = decrypt_token(token) - - return token - -def decrypt_token(encrypted_token: str) -> str: - """ - Decrypt Claude Code encrypted token. - - Args: - encrypted_token: Token with 'enc:' prefix - - Returns: - Decrypted token in format 'sk-ant-oat01-...' - """ - # Remove 'enc:' prefix - encrypted_data = encrypted_token[4:] - - # TODO: Implement decryption logic - # Questions to answer: - # 1. What encryption algorithm does Claude Code use? - # 2. Where is the decryption key stored? - # 3. Is the decryption key platform-specific (per-user)? - # 4. Can we reuse Claude Code's decryption mechanism? - - raise NotImplementedError("Token decryption not yet implemented") -``` - -### Option 3: Call Claude Code CLI for Decryption - -**Approach**: Use the Claude Code CLI binary to decrypt tokens - -```python -def decrypt_token(encrypted_token: str) -> str: - """Decrypt token by invoking Claude Code CLI.""" - # Find claude binary - claude_path = shutil.which("claude") or "~/.local/bin/claude" - - # Use CLI command to get decrypted token - # (if such a command exists - needs research) - result = subprocess.run( - [claude_path, "auth", "decrypt", encrypted_token], - capture_output=True, - text=True - ) - - return result.stdout.strip() -``` - -**Issues**: -- Requires Claude Code CLI to be installed -- No documented CLI command for token decryption -- Adds external dependency - -## Required Investigation Steps - -### 1. Verify SDK Decryption Capabilities - -**Task**: Check if `claude-agent-sdk` handles `enc:` tokens automatically - -**Method**: -```bash -# In environment with SDK installed -python3 << 'EOF' -import os -os.environ['CLAUDE_CODE_OAUTH_TOKEN'] = 'enc:...' # Real encrypted token - -from claude_agent_sdk import Client -# Try creating client - does it decrypt internally? -client = Client() -# Check if authentication works -EOF -``` - -### 2. Reverse Engineer Claude Code CLI Decryption - -**Task**: Understand how Claude CLI decrypts tokens - -**Method**: -- Examine Claude CLI binary (if possible) -- Trace system calls when CLI runs (strace on Linux, dtruss on macOS) -- Check if CLI accesses specific keychain entries for decryption keys -- Look for encryption/decryption libraries used by CLI - -### 3. Find Decryption Key Storage - -**Task**: Locate where decryption keys are stored - -**Hypothesis**: Decryption key stored in: -- macOS: Keychain (separate entry from encrypted token) -- Linux: Secret Service API -- Windows: Credential Manager - -**Verification**: -```bash -# macOS: List all keychain entries -security find-generic-password -a "$(whoami)" | grep -i claude - -# Linux: Use secretstorage to list all items -python3 -c "import secretstorage; ..." -``` - -## Recommended Decryption Approach for Python Backend - -Based on investigation so far, the recommended approach is: - -1. **Detect encrypted tokens**: Check for `enc:` prefix in `get_auth_token()` -2. **Decrypt before use**: Implement `decrypt_token()` function -3. **Platform-specific decryption**: Use appropriate keyring library: - - macOS: Use `subprocess` with `/usr/bin/security` to access decryption key - - Linux: Use `secretstorage` library to access Secret Service API - - Windows: Access Credential Manager or credentials.json -4. **Backward compatibility**: Support both encrypted and plaintext tokens -5. **Error handling**: Provide clear error messages if decryption fails - -## Complete Token Flow Trace (Frontend → Backend) - -### 1. Token Retrieval (Frontend) - -**File**: `apps/frontend/src/main/services/profile-service.ts` - -The frontend retrieves the OAuth token from the system keychain but **does not decrypt it**. When no API profile is active (OAuth mode), the frontend returns an empty environment object, which means it relies on: -- The token already being in the environment as `CLAUDE_CODE_OAUTH_TOKEN` -- OR the Python backend retrieving it from the keychain - -**Key Code**: -```typescript -// Line 223: Returns empty object in OAuth mode, allowing -// CLAUDE_CODE_OAUTH_TOKEN to be used from system keychain -``` - -### 2. Environment Variable Passing (Frontend → PTY) - -**File**: `apps/frontend/src/main/terminal/pty-manager.ts` - -The PTY manager spawns the terminal shell with environment variables, including `CLAUDE_CODE_OAUTH_TOKEN`: - -**Key Code** (Lines 149-152): -```typescript -// Remove ANTHROPIC_API_KEY to ensure Claude Code uses OAuth tokens -// (CLAUDE_CODE_OAUTH_TOKEN from profileEnv) instead of API keys -const { DEBUG: _DEBUG, ANTHROPIC_API_KEY: _ANTHROPIC_API_KEY, ...cleanEnv } = process.env; -``` - -**Important**: The frontend passes through whatever token value exists in the environment - it does NOT check for `enc:` prefix or decrypt it. - -### 3. Token Retrieval (Backend) - -**File**: `apps/backend/core/auth.py` - -#### 3.1. get_auth_token() - -This function retrieves the token from multiple sources: - -```python -def get_auth_token() -> str | None: - # First check environment variables - for var in AUTH_TOKEN_ENV_VARS: # CLAUDE_CODE_OAUTH_TOKEN, ANTHROPIC_AUTH_TOKEN - token = os.environ.get(var) - if token: - return token # ❌ Returns immediately without checking for enc: prefix - - # Fallback to system credential store - return get_token_from_keychain() # ❌ Also returns without decryption -``` - -**Issue**: Returns token as-is with `enc:` prefix intact. - -#### 3.2. require_auth_token() - -This function calls `get_auth_token()` and raises an error if no token is found: - -```python -def require_auth_token() -> str: - token = get_auth_token() # ❌ Gets encrypted token - if not token: - raise ValueError("No OAuth token found...") - return token # ❌ Returns encrypted token -``` - -**Issue**: No decryption step between retrieval and return. - -#### 3.3. ensure_claude_code_oauth_token() - -This function ensures the environment variable is set: - -```python -def ensure_claude_code_oauth_token() -> None: - if os.environ.get("CLAUDE_CODE_OAUTH_TOKEN"): - return - - token = get_auth_token() # ❌ Gets encrypted token - if token: - os.environ["CLAUDE_CODE_OAUTH_TOKEN"] = token # ❌ Sets encrypted token -``` - -**Issue**: Propagates encrypted token to environment variable. - -### 4. Token Usage in SDK Client Creation (Backend) - -#### 4.1. Full Client Creation - -**File**: `apps/backend/core/client.py` (see `create_client()` function) - -```python -def create_client(...): - oauth_token = require_auth_token() # ❌ Gets encrypted token - # Ensure SDK can access it via its expected env var - os.environ["CLAUDE_CODE_OAUTH_TOKEN"] = oauth_token # ❌ Sets encrypted token -``` - -**Issue**: Encrypted token is passed to the Claude Agent SDK, which expects a decrypted `sk-ant-oat01-` token. - -#### 4.2. Simple Client Creation - -**File**: `apps/backend/core/simple_client.py` (see `create_simple_client()` function) - -```python -def create_simple_client(...): - # Get authentication - oauth_token = require_auth_token() # ❌ Gets encrypted token - import os - os.environ["CLAUDE_CODE_OAUTH_TOKEN"] = oauth_token # ❌ Sets encrypted token -``` - -**Issue**: Same problem - encrypted token passed to SDK. - -#### 4.3. Other Usages - -**Files**: -- `apps/backend/core/workspace.py` (Line 1966) - AI merge operations -- `apps/backend/runners/insights_runner.py` - Insights analysis -- `apps/backend/runners/github/batch_issues.py` - GitHub batch operations -- `apps/backend/integrations/linear/updater.py` - Linear integration -- `apps/backend/commit_message.py` - Commit message generation -- `apps/backend/analysis/insight_extractor.py` - Code insights -- `apps/backend/merge/ai_resolver/claude_client.py` - Merge resolution - -**All follow the same pattern**: Call `ensure_claude_code_oauth_token()` → encrypted token in environment → SDK receives encrypted token → API 401 error. - -### 5. Where Decryption Should Be Inserted - -Based on the flow analysis, decryption should be added at **the earliest point of token retrieval** to avoid duplicating decryption logic: - -**RECOMMENDED INSERTION POINT**: `apps/backend/core/auth.py::get_auth_token()` - -```python -def get_auth_token() -> str | None: - # First check environment variables - for var in AUTH_TOKEN_ENV_VARS: - token = os.environ.get(var) - if token: - # ✅ INSERT DECRYPTION HERE - if token.startswith("enc:"): - token = decrypt_token(token) - return token - - # Fallback to system credential store - token = get_token_from_keychain() - # ✅ ALSO DECRYPT KEYCHAIN TOKENS - if token and token.startswith("enc:"): - token = decrypt_token(token) - return token -``` - -**Benefits of this approach**: -1. Single location for decryption logic -2. All downstream functions automatically get decrypted tokens -3. Backward compatible (plaintext tokens pass through unchanged) -4. Consistent behavior across all token sources (env vars and keychain) - -**Alternative insertion points** (NOT recommended): -- `require_auth_token()` - Would need similar logic in `get_auth_token()` for non-required usage -- `create_client()` - Would need duplication in `create_simple_client()` and all other clients -- `ensure_claude_code_oauth_token()` - Would miss direct `get_auth_token()` calls - -## Next Steps - -1. ✅ Document current token flow and identify issue (THIS FILE - COMPLETED) -2. ✅ Trace token flow from frontend to backend (THIS FILE - COMPLETED) -3. ✅ Identify where decryption should be inserted (THIS FILE - COMPLETED) -4. ⏳ Verify if Claude Agent SDK handles decryption internally -5. ⏳ Reverse engineer or document Claude Code CLI decryption mechanism -6. ⏳ Implement `decrypt_token()` function in `apps/backend/core/auth.py` -7. ⏳ Add encryption detection and auto-decryption to `get_auth_token()` -8. ⏳ Test with real encrypted tokens on macOS and Linux -9. ⏳ Add comprehensive error handling for decryption failures - -## Open Questions - -1. **What encryption algorithm does Claude Code use for `enc:` tokens?** - - Possible: AES-256, ChaCha20, or similar - - Key derivation method? - -2. **Where is the decryption key stored?** - - Same keychain entry as encrypted token? - - Separate keychain entry? - - Derived from system/user credentials? - -3. **Does Claude Agent SDK expect encrypted or decrypted tokens?** - - If it expects decrypted: we must decrypt before passing - - If it handles encryption: we may be missing SDK configuration - -4. **Is there a Claude Code CLI command to decrypt tokens?** - - `claude auth decrypt `? - - `claude auth get-token`? - - No documented command found in research - -5. **Can we reuse Claude Code's decryption mechanism?** - - Import decryption functions from CLI? - - Call CLI as subprocess? - - Implement decryption ourselves? - -## References - -- Issue: [GitHub #1223: API Error 401](https://github.com/AndyMik90/Auto-Claude/issues/1223) -- Current auth implementation: `apps/backend/core/auth.py` -- SDK client initialization: `apps/backend/core/client.py` -- Requirements: `apps/backend/requirements.txt` (includes `secretstorage>=3.3.3` for Linux) diff --git a/apps/frontend/src/main/index.ts b/apps/frontend/src/main/index.ts index d7c9f7a7..0fb1e53f 100644 --- a/apps/frontend/src/main/index.ts +++ b/apps/frontend/src/main/index.ts @@ -53,6 +53,7 @@ import { initSentryMain } from './sentry'; import { preWarmToolCache } from './cli-tool-manager'; import { initializeClaudeProfileManager, getClaudeProfileManager } from './claude-profile-manager'; import { isMacOS, isWindows } from './platform'; +import { ptyDaemonClient } from './terminal/pty-daemon-client'; import type { AppSettings, AuthFailureInfo } from '../shared/types'; // ───────────────────────────────────────────────────────────────────────────── @@ -140,6 +141,12 @@ let mainWindow: BrowserWindow | null = null; let agentManager: AgentManager | null = null; let terminalManager: TerminalManager | null = null; +// Re-entrancy guard for before-quit handler. +// The first before-quit call pauses quit for async cleanup, then calls app.quit() again. +// The second call sees isQuitting=true and allows quit to proceed immediately. +// Fixes: pty.node SIGABRT crash caused by environment teardown before PTY cleanup (GitHub #1469) +let isQuitting = false; + function createWindow(): void { // Get the primary display's work area (accounts for taskbar, dock, etc.) // Wrapped in try/catch to handle potential failures with fallback to safe defaults @@ -494,24 +501,50 @@ app.on('window-all-closed', () => { } }); -// Cleanup before quit -app.on('before-quit', async () => { - // Stop periodic update checks +// Cleanup before quit — uses event.preventDefault() to allow async PTY cleanup +// before the JS environment tears down. Without this, pty.node's native +// ThreadSafeFunction callbacks fire after teardown, causing SIGABRT (GitHub #1469). +app.on('before-quit', (event) => { + // Re-entrancy guard: the second app.quit() call (after cleanup) must pass through + if (isQuitting) { + return; + } + isQuitting = true; + + // Pause quit to perform async cleanup + event.preventDefault(); + + // Stop synchronous services immediately stopPeriodicUpdates(); - // Stop usage monitor const usageMonitor = getUsageMonitor(); usageMonitor.stop(); console.warn('[main] Usage monitor stopped'); - // Kill all running agent processes - if (agentManager) { - await agentManager.killAll(); - } - // Kill all terminal processes - if (terminalManager) { - await terminalManager.killAll(); - } + // Perform async cleanup, then allow quit to proceed + (async () => { + try { + // Kill all running agent processes + if (agentManager) { + await agentManager.killAll(); + } + + // Kill all terminal processes — waits for PTY exit with bounded timeout + if (terminalManager) { + await terminalManager.killAll(); + } + + // Shut down PTY daemon client AFTER terminal cleanup completes, + // ensuring all kill commands reach PTY processes before the daemon disconnects + ptyDaemonClient.shutdown(); + console.warn('[main] PTY daemon client shutdown complete'); + } catch (error) { + console.error('[main] Error during pre-quit cleanup:', error); + } finally { + // Always allow quit to proceed, even if cleanup fails + app.quit(); + } + })(); }); // Note: Uncaught exceptions and unhandled rejections are now diff --git a/apps/frontend/src/main/terminal/pty-daemon-client.ts b/apps/frontend/src/main/terminal/pty-daemon-client.ts index 3566fe56..67942ba5 100644 --- a/apps/frontend/src/main/terminal/pty-daemon-client.ts +++ b/apps/frontend/src/main/terminal/pty-daemon-client.ts @@ -11,6 +11,7 @@ import { fileURLToPath } from 'url'; import { spawn, ChildProcess } from 'child_process'; import { app } from 'electron'; import { isWindows, GRACEFUL_KILL_TIMEOUT_MS } from '../platform'; +import { getIsShuttingDown } from './pty-manager'; // ESM-compatible __dirname const __filename = fileURLToPath(import.meta.url); @@ -69,7 +70,7 @@ class PtyDaemonClient { * Connect to daemon, spawning if necessary */ async connect(): Promise { - if (this.isShuttingDown) { + if (this.shuttingDown) { throw new Error('Client is shutting down'); } @@ -174,7 +175,7 @@ class PtyDaemonClient { this.socket.on('close', () => { console.warn('[PtyDaemonClient] Disconnected from daemon'); this.socket = null; - if (!this.isShuttingDown) { + if (!this.shuttingDown) { this.attemptReconnect(); } }); @@ -222,7 +223,7 @@ class PtyDaemonClient { * Attempt to reconnect with exponential backoff */ private attemptReconnect(): void { - if (this.isShuttingDown) return; + if (this.shuttingDown) return; if (this.reconnectAttempts >= this.maxReconnectAttempts) { console.error('[PtyDaemonClient] Max reconnect attempts reached'); @@ -287,10 +288,22 @@ class PtyDaemonClient { // ===== Public API ===== + /** + * Check if shutdown is in progress (either locally or globally via pty-manager) + */ + private get shuttingDown(): boolean { + return this.isShuttingDown || getIsShuttingDown(); + } + /** * Create a new PTY in the daemon */ async createPty(config: PtyConfig): Promise { + // Guard against spawning new PTY processes after shutdown + if (this.shuttingDown) { + throw new Error('Cannot create PTY: client is shutting down'); + } + const response = await this.request<{ type: 'created'; id: string }>({ type: 'create', data: config, @@ -302,14 +315,24 @@ class PtyDaemonClient { * Write data to a PTY */ write(id: string, data: string): void { - this.send({ type: 'write', id, data }); + if (this.shuttingDown) return; + try { + this.send({ type: 'write', id, data }); + } catch { + // Socket may be closed during teardown + } } /** * Resize a PTY */ resize(id: string, cols: number, rows: number): void { - this.send({ type: 'resize', id, data: { cols, rows } }); + if (this.shuttingDown) return; + try { + this.send({ type: 'resize', id, data: { cols, rows } }); + } catch { + // Socket may be closed during teardown + } } /** @@ -441,8 +464,3 @@ class PtyDaemonClient { // Singleton instance export const ptyDaemonClient = new PtyDaemonClient(); - -// Cleanup on app quit -app.on('before-quit', () => { - ptyDaemonClient.shutdown(); -}); diff --git a/apps/frontend/src/main/terminal/pty-daemon.ts b/apps/frontend/src/main/terminal/pty-daemon.ts index 9851df67..4501797c 100644 --- a/apps/frontend/src/main/terminal/pty-daemon.ts +++ b/apps/frontend/src/main/terminal/pty-daemon.ts @@ -23,6 +23,18 @@ const MAX_BUFFER_SIZE = 100_000; // Ring buffer to prevent memory growth const RING_BUFFER_MAX_CHUNKS = 1000; +/** + * Sanitize an ID for safe logging to prevent log injection attacks. + * Uses JSON.stringify which CodeQL recognizes as a sanitizer, then + * removes the surrounding quotes for cleaner log output. + */ +function sanitizeIdForLog(id: string): string { + // JSON.stringify escapes control characters and is recognized by CodeQL + // as a sanitizer for log injection. We slice off the quotes for cleaner output. + const escaped = JSON.stringify(String(id).slice(0, 100)); + return escaped.slice(1, -1); +} + interface ManagedPty { id: string; process: pty.IPty; @@ -71,6 +83,7 @@ interface DaemonResponse { class PtyDaemon { private ptys = new Map(); private server: net.Server | null = null; + private isShuttingDown = false; constructor() { console.error('[PTY Daemon] Starting...'); @@ -236,6 +249,11 @@ class PtyDaemon { * Create a new PTY */ private createPty(config: PtyConfig): string { + // Guard against spawning new PTY processes after shutdown has begun + if (this.isShuttingDown) { + throw new Error('Cannot create PTY: daemon is shutting down'); + } + const id = `pty-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; try { @@ -290,7 +308,7 @@ class PtyDaemon { }); ptyProcess.onExit(({ exitCode, signal }) => { - console.error(`[PTY Daemon] PTY ${id} exited: code=${exitCode}, signal=${signal}`); + console.error('[PTY Daemon] PTY exited:', { id: sanitizeIdForLog(id), exitCode, signal }); managed.isDead = true; // Notify all subscribers @@ -302,7 +320,7 @@ class PtyDaemon { }); this.ptys.set(id, managed); - console.error(`[PTY Daemon] Created PTY ${id} (${config.shell})`); + console.error('[PTY Daemon] Created PTY:', { id: sanitizeIdForLog(id), shell: config.shell }); return id; } catch (error) { @@ -322,7 +340,13 @@ class PtyDaemon { if (managed.isDead) { throw new Error(`PTY ${id} is dead`); } - managed.process.write(data); + try { + managed.process.write(data); + } catch (error) { + // PTY process may have been destroyed during teardown + console.error('[PTY Daemon] Error writing to PTY:', sanitizeIdForLog(id), error); + managed.isDead = true; + } } /** @@ -334,12 +358,18 @@ class PtyDaemon { throw new Error(`PTY ${id} not found`); } if (managed.isDead) { - console.warn(`[PTY Daemon] Cannot resize dead PTY ${id}`); + console.warn('[PTY Daemon] Cannot resize dead PTY:', sanitizeIdForLog(id)); return; } - managed.process.resize(cols, rows); - managed.config.cols = cols; - managed.config.rows = rows; + try { + managed.process.resize(cols, rows); + managed.config.cols = cols; + managed.config.rows = rows; + } catch (error) { + // PTY process may have been destroyed during teardown + console.error('[PTY Daemon] Error resizing PTY:', sanitizeIdForLog(id), error); + managed.isDead = true; + } } /** @@ -348,7 +378,7 @@ class PtyDaemon { private killPty(id: string): void { const managed = this.ptys.get(id); if (!managed) { - console.warn(`[PTY Daemon] PTY ${id} not found for kill`); + console.warn('[PTY Daemon] PTY not found for kill:', sanitizeIdForLog(id)); return; } @@ -356,12 +386,12 @@ class PtyDaemon { try { managed.process.kill(); } catch (error) { - console.error(`[PTY Daemon] Error killing PTY ${id}:`, error); + console.error('[PTY Daemon] Error killing PTY:', sanitizeIdForLog(id), error); } } this.ptys.delete(id); - console.error(`[PTY Daemon] Removed PTY ${id}`); + console.error('[PTY Daemon] Removed PTY:', sanitizeIdForLog(id)); } /** @@ -394,7 +424,7 @@ class PtyDaemon { throw new Error(`PTY ${id} not found`); } managed.clients.add(socket); - console.error(`[PTY Daemon] Client subscribed to PTY ${id}`); + console.error('[PTY Daemon] Client subscribed to PTY:', sanitizeIdForLog(id)); } /** @@ -404,7 +434,7 @@ class PtyDaemon { const managed = this.ptys.get(id); if (managed) { managed.clients.delete(socket); - console.error(`[PTY Daemon] Client unsubscribed from PTY ${id}`); + console.error('[PTY Daemon] Client unsubscribed from PTY:', sanitizeIdForLog(id)); } } @@ -448,13 +478,16 @@ class PtyDaemon { const shutdown = (signal: string) => { console.error(`[PTY Daemon] Received ${signal}, shutting down...`); + // Set shutdown flag to prevent new PTY creation and guard operations + this.isShuttingDown = true; + // Kill all PTYs this.ptys.forEach((managed) => { if (!managed.isDead) { try { managed.process.kill(); } catch (error) { - console.error(`[PTY Daemon] Error killing PTY ${managed.id}:`, error); + console.error('[PTY Daemon] Error killing PTY:', sanitizeIdForLog(managed.id), error); } } }); diff --git a/apps/frontend/src/main/terminal/pty-manager.ts b/apps/frontend/src/main/terminal/pty-manager.ts index bcb03a42..b1016a95 100644 --- a/apps/frontend/src/main/terminal/pty-manager.ts +++ b/apps/frontend/src/main/terminal/pty-manager.ts @@ -16,6 +16,32 @@ import type { SupportedTerminal } from '../../shared/types/settings'; // Windows shell paths are now imported from the platform module via getWindowsShellPaths() +/** + * Shutdown flag to prevent PTY handlers from accessing destroyed resources + * (e.g., BrowserWindow.webContents) during app shutdown. + * Follows the same pattern as isShuttingDown in pty-daemon-client.ts. + * + * Part of the shutdown guard pattern for GitHub issue #1469: without this flag, + * PTY onData/onExit callbacks can fire after BrowserWindow is destroyed, + * causing pty.node's native ThreadSafeFunction to SIGABRT. + */ +let isShuttingDown = false; + +/** + * Set the shutting down flag. Call this during app quit/before-quit + * to prevent PTY handlers from accessing destroyed resources. + */ +export function setShuttingDown(value: boolean): void { + isShuttingDown = value; +} + +/** + * Check if the PTY manager is in shutting down state. + */ +export function getIsShuttingDown(): boolean { + return isShuttingDown; +} + /** * Result of spawning a PTY process */ @@ -184,6 +210,10 @@ export function setupPtyHandlers( // Handle data from terminal ptyProcess.onData((data) => { + // Shutdown guard (GitHub #1469): skip processing to avoid accessing + // destroyed BrowserWindow.webContents, which triggers pty.node SIGABRT + if (isShuttingDown) return; + // Append to output buffer (limit to 100KB) terminal.outputBuffer = (terminal.outputBuffer + data).slice(-100000); @@ -201,7 +231,8 @@ export function setupPtyHandlers( ptyProcess.onExit(({ exitCode }) => { debugLog('[PtyManager] Terminal exited:', id, 'code:', exitCode); - // Resolve any pending exit promise FIRST (before other cleanup) + // Always resolve pending exit promises, even during shutdown + // (needed for waitForPtyExit callers to complete) const pendingExit = pendingExitPromises.get(id); if (pendingExit) { clearTimeout(pendingExit.timeoutId); @@ -209,6 +240,10 @@ export function setupPtyHandlers( pendingExit.resolve(); } + // Shutdown guard (GitHub #1469): skip accessing win.webContents and callbacks + // to avoid pty.node SIGABRT from destroyed BrowserWindow resources + if (isShuttingDown) return; + const win = getWindow(); if (win) { win.webContents.send(IPC_CHANNELS.TERMINAL_EXIT, id, exitCode); diff --git a/apps/frontend/src/main/terminal/terminal-lifecycle.ts b/apps/frontend/src/main/terminal/terminal-lifecycle.ts index cdcf1c83..77565b6e 100644 --- a/apps/frontend/src/main/terminal/terminal-lifecycle.ts +++ b/apps/frontend/src/main/terminal/terminal-lifecycle.ts @@ -294,12 +294,27 @@ export async function destroyTerminal( } /** - * Kill all terminal processes + * Global timeout for destroyAllTerminals to prevent shutdown from hanging (ms). + */ +const DESTROY_ALL_TIMEOUT = 3000; + +/** + * Kill all terminal processes. + * Sets the shutdown flag first to prevent PTY handlers from accessing destroyed + * resources, then waits for all PTY processes to exit (with a global timeout). + * + * This is the core fix for GitHub issue #1469: by setting the shutdown flag and + * awaiting PTY exit before returning, we ensure pty.node's native callbacks + * don't fire after the JS environment tears down (which causes SIGABRT). */ export async function destroyAllTerminals( terminals: Map, saveTimer: NodeJS.Timeout | null ): Promise { + // Set shutdown flag first — prevents PTY onData/onExit from accessing + // destroyed BrowserWindow.webContents (GitHub #1469 shutdown guard pattern) + PtyManager.setShuttingDown(true); + await SessionHandler.persistAllSessionsAsync(terminals); if (saveTimer) { @@ -307,25 +322,24 @@ export async function destroyAllTerminals( saveTimer = null; } - const promises: Promise[] = []; + // Kill all terminals and wait for PTY exit to avoid pty.node SIGABRT on shutdown (GitHub #1469) + const killPromises: Promise[] = []; terminals.forEach((terminal) => { - promises.push( - new Promise((resolve) => { - try { - // Note: We intentionally don't wait for PTY exit here (unlike destroyTerminal) - // because this function is only called during app shutdown when no terminals - // will be recreated. Waiting would only delay shutdown unnecessarily. - PtyManager.killPty(terminal); - } catch { - // Ignore errors during cleanup - } - resolve(); + killPromises.push( + PtyManager.killPty(terminal, true).catch((error) => { + console.warn('[TerminalLifecycle] Error during PTY cleanup:', error); }) ); }); - await Promise.all(promises); + // Wait for all PTY processes to exit, but cap with a global timeout + // so shutdown never hangs indefinitely + await Promise.race([ + Promise.all(killPromises), + new Promise((resolve) => setTimeout(resolve, DESTROY_ALL_TIMEOUT)) + ]); + terminals.clear(); return saveTimer;