Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 2 additions & 17 deletions apps/desktop/src/lib/trpc/routers/workspaces/procedures/delete.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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,
Expand All @@ -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,
Expand Down
7 changes: 2 additions & 5 deletions apps/desktop/src/main/terminal-host/pty-subprocess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
}

Expand Down
220 changes: 220 additions & 0 deletions apps/desktop/src/main/terminal-host/terminal-host.test.ts
Original file line number Diff line number Diff line change
@@ -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();
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Using real-looking fake PIDs with session.dispose() can send SIGKILL to unrelated OS processes during test runs.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/terminal-host/terminal-host.test.ts, line 141:

<comment>Using real-looking fake PIDs with `session.dispose()` can send SIGKILL to unrelated OS processes during test runs.</comment>

<file context>
@@ -0,0 +1,263 @@
+		const terminalHostWouldReject = !session.isAlive;
+		expect(terminalHostWouldReject).toBe(false); // BUG: should be true!
+
+		await session.dispose();
+	});
+
</file context>
Fix with Cubic

});

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<void>((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();
});
});
Loading
Loading