Fix pty.node SIGABRT crash on macOS shutdown (#1619)
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Andy
@@ -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 <token>`?
|
||||
- `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)
|
||||
@@ -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
|
||||
|
||||
@@ -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<void> {
|
||||
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<string> {
|
||||
// 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();
|
||||
});
|
||||
|
||||
@@ -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<string, ManagedPty>();
|
||||
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);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<string, TerminalProcess>,
|
||||
saveTimer: NodeJS.Timeout | null
|
||||
): Promise<NodeJS.Timeout | null> {
|
||||
// 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<void>[] = [];
|
||||
// Kill all terminals and wait for PTY exit to avoid pty.node SIGABRT on shutdown (GitHub #1469)
|
||||
const killPromises: Promise<void>[] = [];
|
||||
|
||||
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<void>((resolve) => setTimeout(resolve, DESTROY_ALL_TIMEOUT))
|
||||
]);
|
||||
|
||||
terminals.clear();
|
||||
|
||||
return saveTimer;
|
||||
|
||||
Reference in New Issue
Block a user