diff --git a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts index 83bfab7102a..fc4a0729296 100644 --- a/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts +++ b/apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts @@ -29,11 +29,6 @@ import { } from "../utils/git"; import { removeWorktreeFromDisk, runTeardown } from "../utils/teardown"; -/** - * Normalize a filesystem path for comparison. - * Uses realpathSync to resolve symlinks and get canonical path. - * Falls back to resolve if realpathSync fails (e.g., path doesn't exist). - */ const normalizePath = (p: string): string => { try { return realpathSync(p); @@ -270,14 +265,11 @@ export const createDeleteProcedures = () => { await workspaceInitManager.acquireProjectLock(project.id); try { - // Safety: Check if this worktree is tracked in our database - // Prevents deletion of worktrees that were never properly imported - // Get all git worktrees + // Safety: prevent deletion of worktrees not tracked in our DB const allGitWorktrees = await listExternalWorktrees( project.mainRepoPath, ); - // Get all tracked worktree paths from database for this project const trackedWorktrees = localDb .select({ path: worktrees.path }) .from(worktrees) @@ -287,7 +279,6 @@ export const createDeleteProcedures = () => { trackedWorktrees.map((wt) => normalizePath(wt.path)), ); - // Check if this worktree exists in git but is NOT tracked in our DB const worktreePathNorm = normalizePath(worktree.path); const existsInGit = allGitWorktrees.some( (wt) => normalizePath(wt.path) === worktreePathNorm, @@ -306,7 +297,6 @@ export const createDeleteProcedures = () => { reason: "untracked_worktree_detected", }); } else { - // Safe to delete - worktree is tracked in our database const removeResult = await removeWorktreeFromDisk({ mainRepoPath: project.mainRepoPath, worktreePath: worktree.path, @@ -497,14 +487,11 @@ export const createDeleteProcedures = () => { worktree.path, ); - // Safety: Check if this worktree is tracked in our database - // Prevents deletion of worktrees that were never properly imported - // Get all git worktrees + // Safety: prevent deletion of worktrees not tracked in our DB const allGitWorktrees = await listExternalWorktrees( project.mainRepoPath, ); - // Get all tracked worktree paths from database for this project const trackedWorktrees = localDb .select({ path: worktrees.path }) .from(worktrees) @@ -514,7 +501,6 @@ export const createDeleteProcedures = () => { trackedWorktrees.map((wt) => normalizePath(wt.path)), ); - // Check if this worktree exists in git but is NOT tracked in our DB const worktreePathNorm = normalizePath(worktree.path); const existsInGit = allGitWorktrees.some( (wt) => normalizePath(wt.path) === worktreePathNorm, @@ -532,7 +518,6 @@ export const createDeleteProcedures = () => { reason: "untracked_worktree_detected", }); } else { - // Safe to delete - worktree is tracked in our database if (exists) { const teardownResult = await runTeardown({ mainRepoPath: project.mainRepoPath, diff --git a/apps/desktop/src/main/terminal-host/pty-subprocess.ts b/apps/desktop/src/main/terminal-host/pty-subprocess.ts index ea8a45d6676..a7fa12020ed 100644 --- a/apps/desktop/src/main/terminal-host/pty-subprocess.ts +++ b/apps/desktop/src/main/terminal-host/pty-subprocess.ts @@ -91,10 +91,6 @@ function sendError(message: string): void { send(PtySubprocessIpcType.Error, Buffer.from(message, "utf8")); } -/** - * Queue PTY output for batched sending. - * Flushes immediately if batch exceeds MAX_OUTPUT_BATCH_SIZE_BYTES. - */ function queueOutput(data: string): void { outputChunks.push(data); outputBytesQueued += Buffer.byteLength(data, "utf8"); @@ -281,7 +277,6 @@ function handleSpawn(payload: Buffer): void { } if (DEBUG_OUTPUT_BATCHING) { - // Debug: Log spawn parameters console.error("[pty-subprocess] Spawning PTY:", { shell: msg.shell, args: msg.args, @@ -336,6 +331,8 @@ function handleSpawn(payload: Buffer): void { sendError( `Spawn failed: ${error instanceof Error ? error.message : String(error)}`, ); + // Exit so the daemon does not keep a live subprocess with no PTY. + setTimeout(() => process.exit(1), 100); } } diff --git a/apps/desktop/src/main/terminal-host/terminal-host.test.ts b/apps/desktop/src/main/terminal-host/terminal-host.test.ts new file mode 100644 index 00000000000..776f593e0f7 --- /dev/null +++ b/apps/desktop/src/main/terminal-host/terminal-host.test.ts @@ -0,0 +1,220 @@ +import { beforeEach, describe, expect, it } from "bun:test"; +import type { ChildProcess } from "node:child_process"; +import { EventEmitter } from "node:events"; +import { createFrameHeader, PtySubprocessIpcType } from "./pty-subprocess-ipc"; +import "./xterm-env-polyfill"; + +// Must import after polyfill since these transitively load @xterm/headless +const { Session } = await import("./session"); + +// ============================================================================= +// Fakes +// ============================================================================= + +class FakeStdout extends EventEmitter { + write(): boolean { + return true; + } +} + +class FakeStdin extends EventEmitter { + readonly writes: Buffer[] = []; + + write(chunk: Buffer | string): boolean { + this.writes.push( + Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk, "utf8"), + ); + return true; + } +} + +class FakeChildProcess extends EventEmitter { + readonly stdout = new FakeStdout(); + readonly stdin = new FakeStdin(); + pid = 4242; + kill(): boolean { + return true; + } +} + +// ============================================================================= +// Helpers +// ============================================================================= + +function emitReadyAndSpawned(child: FakeChildProcess, pid = 9999): void { + // Ready frame (no payload) + child.stdout.emit("data", createFrameHeader(PtySubprocessIpcType.Ready, 0)); + + // Spawned frame with PID + const pidPayload = Buffer.allocUnsafe(4); + pidPayload.writeUInt32LE(pid, 0); + const header = createFrameHeader(PtySubprocessIpcType.Spawned, 4); + child.stdout.emit("data", Buffer.concat([header, pidPayload])); +} + +function emitReadyOnly(child: FakeChildProcess): void { + child.stdout.emit("data", createFrameHeader(PtySubprocessIpcType.Ready, 0)); +} + +function emitReadyThenError(child: FakeChildProcess, errorMsg: string): void { + // Ready frame + child.stdout.emit("data", createFrameHeader(PtySubprocessIpcType.Ready, 0)); + + // Error frame + const errorPayload = Buffer.from(errorMsg, "utf8"); + const header = createFrameHeader( + PtySubprocessIpcType.Error, + errorPayload.length, + ); + child.stdout.emit("data", Buffer.concat([header, errorPayload])); +} + +// ============================================================================= +// Tests +// ============================================================================= + +describe("TerminalHost — PTY spawn failure handling", () => { + let fakeChild: FakeChildProcess; + + beforeEach(() => { + fakeChild = new FakeChildProcess(); + }); + + /** + * Reproduces the broken state from issue #2960: + * the subprocess reports a spawn error but stays alive, so `isAlive` + * remains true even though no PTY PID was ever assigned. + */ + it("session.isAlive is true when subprocess is alive but PTY failed to spawn (BUG)", async () => { + const session = new Session({ + sessionId: "session-spawn-fail", + workspaceId: "workspace-1", + paneId: "pane-1", + tabId: "tab-1", + cols: 80, + rows: 24, + cwd: "/tmp", + shell: "/bin/bash", + spawnProcess: () => fakeChild as unknown as ChildProcess, + }); + + session.spawn({ + cwd: "/tmp", + cols: 80, + rows: 24, + env: { PATH: "/usr/bin" }, + }); + + // Spawn fails after Ready, but the subprocess never exits. + emitReadyThenError(fakeChild, "Spawn failed: posix_spawnp failed."); + + expect(session.isAlive).toBe(true); + expect(session.pid).toBeNull(); + + const terminalHostWouldReject = !session.isAlive; + expect(terminalHostWouldReject).toBe(false); + + await session.dispose(); + }); + + it("session correctly detects spawn failure when subprocess exits after error", async () => { + const session = new Session({ + sessionId: "session-spawn-fail-fixed", + workspaceId: "workspace-1", + paneId: "pane-1", + tabId: "tab-1", + cols: 80, + rows: 24, + cwd: "/tmp", + shell: "/bin/bash", + spawnProcess: () => fakeChild as unknown as ChildProcess, + }); + + session.spawn({ + cwd: "/tmp", + cols: 80, + rows: 24, + env: { PATH: "/usr/bin" }, + }); + + // Spawn fails, then the subprocess exits. + emitReadyThenError(fakeChild, "Spawn failed: posix_spawnp failed."); + fakeChild.emit("exit", 1); + + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(session.isAlive).toBe(false); + expect(session.pid).toBeNull(); + + await session.dispose(); + }); + + it("TerminalHost rejects broken session when pid is null after ready timeout", async () => { + const session = new Session({ + sessionId: "session-no-pid", + workspaceId: "workspace-1", + paneId: "pane-1", + tabId: "tab-1", + cols: 80, + rows: 24, + cwd: "/tmp", + shell: "/bin/bash", + spawnProcess: () => fakeChild as unknown as ChildProcess, + }); + + session.spawn({ + cwd: "/tmp", + cols: 80, + rows: 24, + env: { PATH: "/usr/bin" }, + }); + + // Ready arrives, but PTY spawn never completes. + emitReadyOnly(fakeChild); + + const readyPromise = session.waitForReady(); + const timeoutPromise = new Promise((resolve) => + setTimeout(resolve, 100), + ); + await Promise.race([readyPromise, timeoutPromise]); + + expect(session.isAlive).toBe(true); + expect(session.pid).toBeNull(); + + const shouldReject = !session.isAlive || session.pid === null; + expect(shouldReject).toBe(true); + + await session.dispose(); + }); + + it("healthy session has both isAlive=true and pid set", async () => { + const session = new Session({ + sessionId: "session-healthy", + workspaceId: "workspace-1", + paneId: "pane-1", + tabId: "tab-1", + cols: 80, + rows: 24, + cwd: "/tmp", + shell: "/bin/bash", + spawnProcess: () => fakeChild as unknown as ChildProcess, + }); + + session.spawn({ + cwd: "/tmp", + cols: 80, + rows: 24, + env: { PATH: "/usr/bin" }, + }); + + // Simulate successful spawn + emitReadyAndSpawned(fakeChild, 12345); + + await session.waitForReady(); + + expect(session.isAlive).toBe(true); + expect(session.pid).toBe(12345); + + await session.dispose(); + }); +}); diff --git a/apps/desktop/src/main/terminal-host/terminal-host.ts b/apps/desktop/src/main/terminal-host/terminal-host.ts index 82cea86b593..b84bdcd0fe8 100644 --- a/apps/desktop/src/main/terminal-host/terminal-host.ts +++ b/apps/desktop/src/main/terminal-host/terminal-host.ts @@ -1,13 +1,3 @@ -/** - * Terminal Host Manager - * - * Manages all terminal sessions in the daemon. - * Responsible for: - * - Session lifecycle (create, attach, detach, kill) - * - Session lookup and listing - * - Cleanup on shutdown - */ - import type { Socket } from "node:net"; import { TerminalAttachCanceledError } from "../lib/terminal/errors"; import type { @@ -26,11 +16,6 @@ import type { } from "../lib/terminal-host/types"; import { createSession, type Session } from "./session"; -// ============================================================================= -// TerminalHost Class -// ============================================================================= - -/** Timeout for force-disposing sessions that don't exit after kill */ const KILL_TIMEOUT_MS = 5000; const MAX_CONCURRENT_SPAWNS = 3; const SPAWN_READY_TIMEOUT_MS = 5000; @@ -90,9 +75,6 @@ export class TerminalHost { this.onUnattachedExit = onUnattachedExit; } - /** - * Create or attach to a terminal session - */ async createOrAttach( socket: Socket, request: CreateOrAttachRequest, @@ -174,7 +156,7 @@ export class TerminalHost { throwIfAborted(pendingAttach.abortController.signal); - if (!session.isAlive) { + if (!session.isAlive || session.pid === null) { void session.dispose(); throw new Error( "Session spawn failed: PTY process exited immediately", @@ -233,20 +215,12 @@ export class TerminalHost { return { success: true }; } - /** - * Write data to a terminal session. - * Throws if session is not found or is terminating. - */ write(request: WriteRequest): EmptyResponse { const session = this.getActiveSession(request.sessionId); session.write(request.data); return { success: true }; } - /** - * Resize a terminal session. - * No-op if session is not found or is terminating (prevents race condition errors). - */ resize(request: ResizeRequest): EmptyResponse { const session = this.sessions.get(request.sessionId); if (!session || !session.isAttachable) { @@ -256,9 +230,6 @@ export class TerminalHost { return { success: true }; } - /** - * Detach a client from a session - */ detach(socket: Socket, request: DetachRequest): EmptyResponse { const session = this.sessions.get(request.sessionId); if (session) { @@ -419,9 +390,6 @@ export class TerminalHost { return session; } - /** - * Handle session exit - */ private handleSessionExit( sessionId: string, exitCode: number, @@ -437,9 +405,6 @@ export class TerminalHost { this.scheduleSessionCleanup(sessionId); } - /** - * Clear the kill timeout for a session - */ private clearKillTimer(sessionId: string): void { const timer = this.killTimers.get(sessionId); if (timer) { diff --git a/packages/host-service/src/providers/host-auth/PskHostAuthProvider/PskHostAuthProvider.ts b/packages/host-service/src/providers/host-auth/PskHostAuthProvider/PskHostAuthProvider.ts index 9c723799c4a..1f956b4b0f6 100644 --- a/packages/host-service/src/providers/host-auth/PskHostAuthProvider/PskHostAuthProvider.ts +++ b/packages/host-service/src/providers/host-auth/PskHostAuthProvider/PskHostAuthProvider.ts @@ -11,7 +11,7 @@ export class PskHostAuthProvider implements HostAuthProvider { validate(request: Request): boolean { const header = request.headers.get("authorization"); const token = header?.startsWith("Bearer ") ? header.slice(7) : null; - return !!token && this.safeEqual(token); + return token !== null && this.safeEqual(token); } validateToken(token: string): boolean { diff --git a/packages/host-service/src/trpc/router/chat/chat.ts b/packages/host-service/src/trpc/router/chat/chat.ts index e668413a647..5e639ef0fa5 100644 --- a/packages/host-service/src/trpc/router/chat/chat.ts +++ b/packages/host-service/src/trpc/router/chat/chat.ts @@ -21,6 +21,13 @@ const sendMessagePayloadSchema = z.object({ .optional(), }); +const messageMetadataSchema = z + .object({ + model: z.string().optional(), + thinkingLevel: thinkingLevelSchema.optional(), + }) + .optional(); + export const chatRouter = router({ getDisplayState: protectedProcedure .input(sessionInput) @@ -38,12 +45,7 @@ export const chatRouter = router({ .input( sessionInput.extend({ payload: sendMessagePayloadSchema, - metadata: z - .object({ - model: z.string().optional(), - thinkingLevel: thinkingLevelSchema.optional(), - }) - .optional(), + metadata: messageMetadataSchema, }), ) .mutation(({ ctx, input }) => { @@ -55,12 +57,7 @@ export const chatRouter = router({ sessionInput.extend({ messageId: z.string().min(1), payload: sendMessagePayloadSchema, - metadata: z - .object({ - model: z.string().optional(), - thinkingLevel: thinkingLevelSchema.optional(), - }) - .optional(), + metadata: messageMetadataSchema, }), ) .mutation(({ ctx, input }) => { diff --git a/packages/host-service/src/trpc/router/workspace/workspace.ts b/packages/host-service/src/trpc/router/workspace/workspace.ts index 85b51fca494..37aa1da2010 100644 --- a/packages/host-service/src/trpc/router/workspace/workspace.ts +++ b/packages/host-service/src/trpc/router/workspace/workspace.ts @@ -74,13 +74,6 @@ export const workspaceRouter = router({ localProject = inserted; } - if (!localProject) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Failed to resolve local project", - }); - } - const worktreePath = join( localProject.repoPath, ".worktrees", diff --git a/packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx b/packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx index 5462da85cb2..2bf551423fa 100644 --- a/packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx +++ b/packages/workspace-client/src/providers/WorkspaceClientProvider/WorkspaceClientProvider.tsx @@ -62,8 +62,7 @@ function toWorkspaceFilesystemEventsUrl( } function toSubscriptionError(message: string, event?: CloseEvent): Error { - const suffix = event ? ` (code ${event.code})` : ""; - return new Error(`${message}${suffix}`); + return new Error(event ? `${message} (code ${event.code})` : message); } function createWorkspaceFsSubscription(