Add warning log for output capture timeout

Also removes redundant PID assignment.

Author: ghostwriternr

docs/plans/2025-12-01-process-callbacks-and-pid-design.md

@@ -0,0 +1,254 @@
+# Process Callbacks and PID Design
+
+**Date**: 2025-12-01
+**Status**: Approved
+**Issue**: [#262](https://github.com/cloudflare/sandbox-sdk/issues/262)
+
+## Overview
+
+Fix three related bugs in the process management API:
+
+1. `onOutput`/`onExit` callbacks never fire
+2. `getLogs()` returns empty for fast commands (race condition)
+3. `pid` is always `undefined`
+
+## Problem Analysis
+
+### Issue 1: Callbacks Never Fire
+
+**Location**: `packages/sandbox/src/sandbox.ts:1247-1300`
+
+The `ProcessOptions` interface defines `onOutput` and `onExit` callbacks, but `startProcess()` only calls `onStart`. The callbacks are defined in the type but never wired up.
+
+**Root cause**: No mechanism exists to deliver events after `startProcess()` returns.
+
+### Issue 2: getLogs() Race Condition
+
+**Location**: `packages/sandbox-container/src/services/session-manager.ts:190-284`
+
+For fast commands like `echo 123`:
+
+1. `startProcess()` returns after processing the first event
+2. Remaining events stream in background
+3. User calls `getLogs()` immediately
+4. Background streaming hasn't updated the ProcessStore yet
+
+**Root cause**: `executeStreamInSession` only awaits the first event, then returns while remaining events process asynchronously.
+
+### Issue 3: PID Always Undefined
+
+**Location**: `packages/sandbox-container/src/services/process-service.ts:126-130`
+
+```typescript
+const processRecordData = this.manager.createProcessRecord(
+  command,
+  undefined, // PID hardcoded as undefined
+  options
+);
+```
+
+**Root cause**: The PID is known when Bun spawns the subprocess, but this value never propagates back up through the layers.
+
+## Design
+
+### Solution 1: Background Stream for Callbacks
+
+When `onOutput` or `onExit` callbacks are provided to `startProcess()`, the SDK automatically opens a background SSE stream to route events.
+
+**SDK changes** (`packages/sandbox/src/sandbox.ts`):
+
+```typescript
+async startProcess(command: string, options?: ProcessOptions): Promise<Process> {
+  const session = sessionId ?? (await this.ensureDefaultSession());
+  const response = await this.client.processes.startProcess(command, session, requestOptions);
+
+  const processObj = this.createProcessFromDTO({...}, session);
+
+  // Call onStart immediately
+  if (options?.onStart) {
+    options.onStart(processObj);
+  }
+
+  // If callbacks provided, start background streaming
+  if (options?.onOutput || options?.onExit) {
+    this.startBackgroundStream(response.processId, options);
+  }
+
+  return processObj;
+}
+
+private async startBackgroundStream(processId: string, options: ProcessOptions): Promise<void> {
+  try {
+    const stream = await this.client.processes.streamProcessLogs(processId);
+
+    for await (const event of parseSSEStream(stream)) {
+      switch (event.type) {
+        case 'stdout':
+        case 'stderr':
+          options.onOutput?.(event.type, event.data);
+          break;
+        case 'exit':
+        case 'complete':
+          options.onExit?.(event.exitCode ?? null);
+          return; // Stream complete, exit loop
+      }
+    }
+  } catch (error) {
+    options.onError?.(error instanceof Error ? error : new Error(String(error)));
+  }
+}
+```
+
+**Lifecycle**: The background stream runs in the Durable Object context. It automatically closes when:
+
+- Process completes (receives `complete`/`exit` event)
+- Process is killed
+- An error occurs
+
+### Solution 2: Event-Based Completion Detection
+
+Modify `executeStreamInSession` to drain all events if the process completes quickly, rather than using time-based heuristics.
+
+**Container changes** (`packages/sandbox-container/src/services/session-manager.ts`):
+
+```typescript
+async executeStreamInSession(
+  sessionId: string,
+  command: string,
+  onEvent: (event: ExecEvent) => Promise<void>,
+  options: { cwd?: string; env?: Record<string, string> } = {},
+  commandId: string
+): Promise<ServiceResult<{ continueStreaming: Promise<void> }>> {
+  // ... existing setup code ...
+
+  const generator = session.execStream(command, { commandId, cwd, env });
+
+  // Process first event
+  const firstResult = await generator.next();
+  if (!firstResult.done) {
+    await onEvent(firstResult.value);
+  }
+
+  // Check if process already completed (generator exhausted or complete event)
+  if (firstResult.done || firstResult.value?.type === 'complete') {
+    // Process finished quickly - drain any remaining events synchronously
+    if (!firstResult.done) {
+      for await (const event of generator) {
+        await onEvent(event);
+      }
+    }
+    return {
+      success: true,
+      data: { continueStreaming: Promise.resolve() }
+    };
+  }
+
+  // Process still running - continue in background
+  const continueStreaming = (async () => {
+    for await (const event of generator) {
+      await onEvent(event);
+    }
+  })();
+
+  return {
+    success: true,
+    data: { continueStreaming }
+  };
+}
+```
+
+**Key insight**: We don't guess "fast vs slow" by time. We check if the generator is exhausted after the first event. If yes, the process completed quickly and we drain all events before returning.
+
+### Solution 3: Started Event with PID
+
+Add a new `started` event type that includes the PID as the first event yielded by `execStream`.
+
+**Event type addition** (`packages/shared/src/types.ts`):
+
+```typescript
+export interface ExecEvent {
+  type: 'started' | 'stdout' | 'stderr' | 'complete' | 'error';
+  data?: string;
+  exitCode?: number;
+  pid?: number; // Present on 'started' event
+}
+```
+
+**Session changes** (`packages/sandbox-container/src/session.ts`):
+
+In `execStream`, yield a `started` event immediately after spawning:
+
+```typescript
+async *execStream(command: string, options: ExecStreamOptions): AsyncGenerator<ExecEvent> {
+  const proc = Bun.spawn(['bash', '-c', command], {
+    cwd: options.cwd,
+    env: { ...process.env, ...options.env },
+    stdout: 'pipe',
+    stderr: 'pipe',
+  });
+
+  // First event: started with PID
+  yield { type: 'started', pid: proc.pid };
+
+  // ... rest of streaming logic ...
+}
+```
+
+**ProcessService changes** (`packages/sandbox-container/src/services/process-service.ts`):
+
+Capture PID from the started event:
+
+```typescript
+async (event) => {
+  if (event.type === 'started' && event.pid !== undefined) {
+    processRecord.pid = event.pid;
+    await this.store.update(processRecord.id, { pid: event.pid });
+  } else if (event.type === 'stdout' && event.data) {
+    // ... existing stdout handling ...
+  }
+  // ...
+};
+```
+
+## Implementation Order
+
+1. **Issue 3 (PID)**: Add `started` event - foundational change, minimal risk
+2. **Issue 2 (Race condition)**: Event-based completion detection - container-only change
+3. **Issue 1 (Callbacks)**: Background streaming - SDK change, depends on working container
+
+## Testing Strategy
+
+### Issue 1: Callbacks
+
+- Test callbacks fire for long-running process
+- Test callbacks fire for fast command (`echo 123`)
+- Test `onExit` receives correct exit code
+- Test error callback on process failure
+
+### Issue 2: Race Condition
+
+- Test `getLogs()` immediately after `startProcess("echo 123")` returns full output
+- Test long-running process still returns immediately
+- Test multiple rapid `getLogs()` calls return consistent data
+
+### Issue 3: PID
+
+- Test `process.pid` is defined after `startProcess()`
+- Test PID matches actual OS process (via `ps` command)
+- Test PID in `onStart` callback is defined
+
+## Affected Files
+
+**Container**:
+
+- `packages/sandbox-container/src/services/session-manager.ts`
+- `packages/sandbox-container/src/services/process-service.ts`
+- `packages/sandbox-container/src/session.ts`
+
+**SDK**:
+
+- `packages/sandbox/src/sandbox.ts`
+
+**Shared**:
+
+- `packages/shared/src/types.ts` (ExecEvent type)