Add waitForExit() method to Process interface #299

whoiskatrin · 2025-12-12T12:28:10Z

Implemented waitForExit(timeout?) method to Process objects

also replaced the arbitrary setTimeout(resolve, 3000) workaround with waitForExit():

Now uses the waitForExit() API to wait for the background process to complete
Validates exit code and verifies stdout output from the WaitForExitResult
More reliable and deterministic than arbitrary delays

fixes #297

changeset-bot · 2025-12-12T12:28:14Z

🦋 Changeset detected

Latest commit: 4df089d

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	Minor

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 · 2025-12-12T12:33:01Z

Open in StackBlitz

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

commit: 4df089d

github-actions · 2025-12-12T12:33:02Z

🐳 Docker Images Published

Default:

FROM cloudflare/sandbox:0.0.0-pr-299-09e6d55

With Python:

FROM cloudflare/sandbox:0.0.0-pr-299-09e6d55-python

With OpenCode:

FROM cloudflare/sandbox:0.0.0-pr-299-09e6d55-opencode

Version: 0.0.0-pr-299-09e6d55

Use the -python variant if you need Python code execution, or -opencode for the variant with OpenCode AI coding agent pre-installed.

📦 Standalone Binary

For arbitrary Dockerfiles:

COPY --from=cloudflare/sandbox:0.0.0-pr-299-09e6d55 /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]

Download via GitHub CLI:

gh run download 20169928424 -n sandbox-binary

Extract from Docker:

docker run --rm cloudflare/sandbox:0.0.0-pr-299-09e6d55 cat /container-server/sandbox > sandbox && chmod +x sandbox

claude · 2025-12-12T12:37:27Z

tests/e2e/process-readiness-workflow.test.ts

+      {
+        method: 'POST',
+        headers,
+        body: JSON.stringify({


Tests call custom endpoint that doesn't exist in production container. Should use SDK directly:

// Instead of HTTP call to custom endpoint const result = await startData.waitForExit(30000);

Current approach tests test worker implementation, not actual SDK.

Still seeing the same pattern in the new tests. Lines 363-373 in process-readiness-workflow.test.ts use HTTP fetch calls to custom endpoints. The E2E tests should use the SDK directly to validate the actual API users will consume.

Still relevant - custom endpoint added but no E2E tests using it. Added this to my main review.

The custom endpoint was added but no E2E tests were created that actually call the SDK. The test suite needs tests that verify process.waitForExit() works end-to-end in a real Worker environment.

ghostwriternr

Looks good overall - but there's a lot of work being done to fetch logs, but I think we can separate concerns and keep waitForExit focused and leave fetching logs to the existing methods.

ghostwriternr · 2025-12-12T13:09:11Z

packages/shared/src/types.ts

+  /** Accumulated stdout output */
+  stdout: string;
+  /** Accumulated stderr output */
+  stderr: string;


Similar to the other waitFor methods, I think we can leave out accumulated logs from being returned in the response. If users need it, they anyway have access to the various log fetching utils right now. This method has a simple purpose that already gets solved without these.

ghostwriternr · 2025-12-12T13:09:58Z

packages/sandbox/src/sandbox.ts

+      const existingLogs = await this.getProcessLogs(processId);
+      collectedStdout = existingLogs.stdout;
+      collectedStderr = existingLogs.stderr;


If we remove the logs from output, we can perhaps completely get rid of all log-related logic from here.

ghostwriternr · 2025-12-12T13:10:12Z

packages/sandbox/src/sandbox.ts

+    // Stream logs and wait for exit event
+    const stream = await this.streamProcessLogs(processId);


Same as above

ghostwriternr · 2025-12-12T13:11:25Z

packages/sandbox/tests/process-readiness.test.ts

    });
  });

+  describe('waitForExit() method', () => {


Tests can be more concise. Lots of overlap with current setup.

ghostwriternr · 2025-12-12T13:12:27Z

tests/e2e/process-readiness-workflow.test.ts

    );
  }, 60000);

+  test('should wait for process to exit and return exit code with output', async () => {


no-op: v nice e2e tests

ghostwriternr · 2025-12-12T13:14:04Z

Minor import order merge conflicts too, sorry about that

Remove stdout/stderr accumulation from waitForExit() per review feedback. Users can fetch logs separately via getProcessLogs() or streamProcessLogs() if needed. This keeps the method focused on its single responsibility.

claude

Claude Code Review

The waitForExit() implementation is clean and follows established patterns. The exit code fallback logic is correct - I verified that 'killed' and 'error' statuses don't set exitCode in the container runtime.

Issues to address:

Missing E2E tests - Custom test endpoint exists but no tests use it. Other readiness methods (waitForLog, waitForPort) have comprehensive E2E coverage in process-readiness-workflow.test.ts. This needs equivalent tests.
Generic Error instead of typed error - Line 1559 throws plain Error for process not found. Should use ProcessNotFoundError for consistency with other process methods.

Minor improvements:

Add clarifying comment at line 1556 explaining why status is checked every iteration (unlike waitForPort which checks every 3rd to reduce latency - here the status IS what we're waiting for).
Consider enhancing JSDoc with @param, @returns, and @throws tags for consistency.

The unit tests are comprehensive and the implementation is architecturally sound. Just needs E2E coverage and the error type fix before merge.

claude · 2025-12-12T13:25:16Z

packages/sandbox/src/sandbox.ts

+      const processInfo = await this.getProcess(processId);
+
+      if (!processInfo) {
+        throw new Error(


Use ProcessNotFoundError instead of generic Error for consistency with other process methods.

if (!processInfo) { throw new ProcessNotFoundError({ code: ErrorCode.PROCESS_NOT_FOUND, message: `Process ${processId} not found. It may have been cleaned up or never existed.`, context: { processId }, httpStatus: 404, timestamp: new Date().toISOString() }); }

packages/sandbox/src/sandbox.ts

.changeset/clean-pans-switch.md

packages/sandbox/src/sandbox.ts

Streaming is more efficient - single request to container instead of repeated polling. streamProcessLogs handles already-exited processes gracefully by sending the exit event immediately.

Documents the new Process.waitForExit() method that allows waiting for background processes to complete and retrieving their exit codes. Related to cloudflare/sandbox-sdk#299

ghostwriternr · 2025-12-12T14:27:30Z

packages/sandbox/src/sandbox.ts

+        for await (const event of parseSSEStream<LogEvent>(stream)) {
+          if (event.type === 'exit') {
+            return {
+              exitCode: event.exitCode ?? 0


nit: should the fallback be 0 or 1? Assuming failure when we don't have a clean exit code might be better no?

ghostwriternr

lessgooo

Add waitForExit() method to Process interface and tests

9d9f4c1

agents-git-bot bot mentioned this pull request Dec 12, 2025

Add waitForExit() method to Process interface cloudflare/cloudflare-docs#27084

Open

This comment was marked as outdated.

Sign in to view

claude bot reviewed Dec 12, 2025

View reviewed changes

fix error handling for unexpected stream terminations in Sandbox class

6437af6

whoiskatrin marked this pull request as ready for review December 12, 2025 12:44

This comment was marked as outdated.

Sign in to view

whoiskatrin requested a review from ghostwriternr December 12, 2025 13:08

Merge branch 'main' into add-wait-for-exit

8468050

ghostwriternr requested changes Dec 12, 2025

View reviewed changes

Simplify waitForExit to return only exit code

6a027b7

Remove stdout/stderr accumulation from waitForExit() per review feedback. Users can fetch logs separately via getProcessLogs() or streamProcessLogs() if needed. This keeps the method focused on its single responsibility.

claude bot reviewed Dec 12, 2025

View reviewed changes

ghostwriternr reviewed Dec 12, 2025

View reviewed changes

.changeset/clean-pans-switch.md Outdated Show resolved Hide resolved

packages/sandbox/src/sandbox.ts Outdated Show resolved Hide resolved

whoiskatrin added 2 commits December 12, 2025 14:21

Use streamProcessLogs instead of polling for waitForExit

fcfb045

Streaming is more efficient - single request to container instead of repeated polling. streamProcessLogs handles already-exited processes gracefully by sending the exit event immediately.

Add changeset for waitForExit feature

cfeac3a

whoiskatrin added 3 commits December 12, 2025 14:25

revert original changeset change

68978f3

revert changeset

d8d6181

fix changeset

b0cd48d

ghostwriternr reviewed Dec 12, 2025

View reviewed changes

whoiskatrin added 3 commits December 12, 2025 14:28

fix changeset

55beb0f

Update clean-pans-switch.md

d83d76c

exit code fallback

4df089d

This comment was marked as outdated.

Sign in to view

ghostwriternr approved these changes Dec 12, 2025

View reviewed changes

whoiskatrin merged commit d3252dc into main Dec 12, 2025
17 checks passed

whoiskatrin deleted the add-wait-for-exit branch December 12, 2025 14:42

github-actions bot mentioned this pull request Dec 12, 2025

Version Packages #300

Merged

ghostwriternr mentioned this pull request Dec 12, 2025

Our e2e tests occasionally flake out with 500s #295

Closed

		// Stream logs and wait for exit event
		const stream = await this.streamProcessLogs(processId);

Add waitForExit() method to Process interface #299

Add waitForExit() method to Process interface #299

Conversation

whoiskatrin commented Dec 12, 2025 • edited by ghostwriternr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

pkg-pr-new bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🐳 Docker Images Published

📦 Standalone Binary

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

claude bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

ghostwriternr left a comment

Choose a reason for hiding this comment

Uh oh!

ghostwriternr Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

ghostwriternr Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

ghostwriternr Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

ghostwriternr Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

ghostwriternr Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

ghostwriternr commented Dec 12, 2025

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

claude bot Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghostwriternr Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

ghostwriternr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whoiskatrin commented Dec 12, 2025 •

edited by ghostwriternr

Loading

changeset-bot bot commented Dec 12, 2025 •

edited

Loading

pkg-pr-new bot commented Dec 12, 2025 •

edited

Loading

github-actions bot commented Dec 12, 2025 •

edited

Loading