Fix process callbacks, PID capture, and getLogs race

[ghostwriternr]

Summary

Fixes #262

Problem

Three related issues with process execution:

1. PID always undefined in onStart: The process record wasn't being updated with the actual PID after spawn
2. Callbacks never firing: Status and output listeners were lost when the process record was updated
3. getLogs returns empty for fast commands: Background execution wrote the exit code before output was fully captured

Solution

1. PID capture: Ensure the process record is updated with PID immediately after spawn
2. Listener preservation: Update process records in a way that preserves existing listener references
3. Output synchronization: Wait for output labelers to complete before reading final logs, using a marker file to signal completion

[changeset-bot]

🦋 Changeset detected

Latest commit: bbc5345

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@cloudflare/sandbox	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/sandbox-sdk/@cloudflare/sandbox@267

commit: bbc5345

[github-actions]

🐳 Docker Images Published

Default (no Python):

FROM cloudflare/sandbox:0.0.0-pr-267-6f24c60

With Python:

FROM cloudflare/sandbox:0.0.0-pr-267-6f24c60-python

Version: 0.0.0-pr-267-6f24c60

Use the -python variant if you need Python code execution.

[claude]

Claude Code Review

Verdict: Ready to merge. The implementation correctly fixes all three stated problems with architecturally sound solutions.

What This PR Fixes

1. PID capture: session.ts:347-363 now waits for PID file before yielding start event, and process-service.ts:153-154 stores it immediately
2. Listener preservation: process-store.ts:64-65 mutates in-place with Object.assign() instead of creating new objects, preserving ProcessService's listener references
3. Output synchronization: Shell script creates labelers.done marker file, session.ts:419-443 waits for it, and getProcess() awaits streamingComplete before returning logs

Architecture Alignment

Excellent. All changes follow existing patterns:

• StreamingComplete promise for async coordination
• Marker files for IPC (following pid/exit file pattern)
• Reference preservation for event routing
• Synchronous first-event processing in SessionManager

Edge Cases Handled Well

• PID file timeout: logs warning, continues with undefined (process still works)
• Labelers timeout: logs warning, proceeds (5s is generous)
• Process read from disk: correctly skips streamingComplete await
• Early SDK streaming: safe because PID captured before return

Minor Note: PID Reuse

process-service.ts:289-298 uses process.kill(pid, 0) to detect dead processes. This has theoretical PID reuse risk (OS reuses PID for new process between check and await). However, the impact is minimal because:

• Terminal status check is the primary gate
• PID check is only a fallback for fast commands
• Time window is tiny (microseconds)
• labelers.done provides deterministic sync

This is a pragmatic tradeoff and unlikely to cause real issues.

Suggestions (Non-blocking)

1. Add comment explaining PID reuse limitation at process-service.ts:286
2. Consider unit test explicitly validating callback firing (currently covered implicitly by E2E tests)

Testing

E2E test properly validates:

• PID availability (line 244-246)
• Fast command log capture (line 258-259)

Changeset correctly formatted and scoped to @cloudflare/sandbox only.