Make bun port server configurable#490
Conversation
🦋 Changeset detectedLatest commit: c9977ce The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
🐳 Docker Images Published
Usage: FROM cloudflare/sandbox:0.0.0-pr-490-c9977ceVersion: 📦 Standalone BinaryFor arbitrary Dockerfiles: COPY --from=cloudflare/sandbox:0.0.0-pr-490-c9977ce /container-server/sandbox /sandbox
ENTRYPOINT ["/sandbox"]Download via GitHub CLI: gh run download 23603890378 -n sandbox-binaryExtract from Docker: docker run --rm cloudflare/sandbox:0.0.0-pr-490-c9977ce cat /container-server/sandbox > sandbox && chmod +x sandbox |
|
/bonk review this PR |
There was a problem hiding this comment.
Review Summary
The overall approach is sound — moving the control plane off port 3000 eliminates a genuine pain point for users running Express/Next.js/etc. The port is cleanly centralized via DEFAULT_CONTROL_PORT in shared, and the SDK propagates it to the container through envVars. Good test coverage for the new behavior.
However, there are a few issues that should be addressed before merging.
Issues
1. Changeset uses minor — should be patch
The AGENTS.md states: "The SDK is in beta — use patch for all changes, minor only for breaking changes." While this PR does change the default port (a breaking change in the sense that images must match), the changeset should follow the project convention of patch for beta releases.
2. Stale comment in Dockerfile (line 219)
The opencode stage has a comment referencing the old port:
# Expose OpenCode server port (in addition to 3000 from runtime-base)Should be updated to reference 8671.
3. Stale comments in security-service.test.ts (lines 5, 82-83)
The test file header still references the old port:
* - Port 3000 is protected (SDK control plane)
and:
* Validate port number
* - Protects port 3000 (SDK control plane) - CRITICAL!
(line 82-83 in the source security-service.ts)
The source file comment at security-service.ts:82-83 was not updated:
/**
* Validate port number
* - Protects port 3000 (SDK control plane) - CRITICAL!4. No validation of parsed SANDBOX_CONTROL_PORT in the container config
In packages/sandbox-container/src/config.ts:11-13:
const SERVER_PORT = process.env.SANDBOX_CONTROL_PORT
? parseInt(process.env.SANDBOX_CONTROL_PORT, 10)
: DEFAULT_CONTROL_PORT;If SANDBOX_CONTROL_PORT is set to a non-numeric string (e.g., "abc"), parseInt returns NaN and the server will attempt to bind to port NaN. The SDK side handles this correctly (parseInt(...) || DEFAULT_CONTROL_PORT) but the container side does not have the || DEFAULT_CONTROL_PORT fallback. These should be consistent — both should fall back to the default on invalid input.
5. E2E test may not run in CI
tests/e2e/control-port-workflow.test.ts — The test file is added but it's unclear whether this file is picked up by the existing E2E test configuration. If the E2E glob doesn't match control-port-workflow.test.ts, these tests won't run in CI. Please verify this is included in the vitest E2E config.
Minor / Non-blocking
- The container-side
SecurityService.validatePort()comment at line 82-83 still says "Protects port 3000" — should say "Protects the SDK control plane port". - The test file comment at
security-service.test.ts:5also references port 3000. - The
config.test.tstest for custom port parsing via env var is limited — it only tests the default case since the env var is read at module load time. Consider noting this limitation or adding an integration-level test.
|
Review posted on PR #490. Here's a summary of the findings: Requested changes:
The overall architecture is well done — the port is properly centralized in |
aron-cf
left a comment
There was a problem hiding this comment.
This change looks good. I have two questions:
- This breaking change sucks. It's going to break so many projects. How much time have we invested in seeing if we can provide backwards compatibility? Can we try the configured port and if it fails attempt 3000 instead?
- Let's issue a really clear message to the user if we get a port mismatch with clear instructions on how to fix.
- Can we template the EXPOSE call to use a build arg? This will make this less fragile in future.
We control the dev build, so we should be able to include the equivalent of
ARG BUILD_INTERNAL_SERVER_PORT=8080 EXPOSE ${BUILD_PORT}
--build-arg BUILD_INTERNAL_SERVER_PORT=${config value}
|
dc7fd96 to
41a2800
Compare
|
/bonk review this PR |
There was a problem hiding this comment.
Review Summary
The core change is well-structured — centralizing the control port in @repo/shared as DEFAULT_CONTROL_PORT, propagating it through envVars, parameterizing validatePort(), and templating EXPOSE in the Dockerfile. Good test coverage for the new behavior. The stale comments flagged in earlier reviews (security-service.ts, Dockerfile) have been addressed.
However, several issues from previous reviews remain unfixed, and I found additional problems.
Outstanding issues from previous reviews (still unfixed)
1. Changeset uses minor — should be patch
AGENTS.md: "The SDK is in beta — use patch for all changes, minor only for breaking changes. Never use major." This is a beta project; even breaking changes use patch per the convention (the "minor only for breaking" clause appears to be for truly exceptional cases). The changeset bot will pick this up and create a minor version bump, which is not appropriate.
2. Missing NaN guard in request-handler.ts:40-42
This was flagged by Devin and is still present:
const controlPort = controlPortStr
? parseInt(controlPortStr, 10)
: DEFAULT_CONTROL_PORT;If SANDBOX_CONTROL_PORT is "abc", parseInt returns NaN. The sandbox constructor at sandbox.ts:393 correctly uses parseInt(controlPortStr, 10) || DEFAULT_CONTROL_PORT, and the container config at config.ts:12 uses Number(...) || DEFAULT_CONTROL_PORT. This is the only place that doesn't have the || fallback.
With controlPort = NaN, validatePort(port, NaN) never rejects the control port (since port === NaN is always false), and port !== controlPort is always true (since NaN !== NaN), so the token-validation skip for the control port is disabled. While this doesn't create a direct security hole (token validation still enforces access), it's an inconsistency that should be fixed.
3. config.test.ts still present
aron-cf flagged this as a low-value test (just checking the default), Muhammad-Bin-Ali agreed to remove it, but it's still in the PR.
New issues
4. PR description claims legacy fallback exists — it doesn't
The PR description states:
startWithLegacyFallback()wrapsstartAndWaitForPortswith a catch-retry to port 3000 when the container is running but not listening on the configured port. After fallback,defaultPortis updated and the current request's port is remapped sosuper.containerFetchroutes correctly.
No startWithLegacyFallback() function exists in the code. The containerFetch() override in sandbox.ts has no fallback logic — if startAndWaitForPorts fails on port 8671, it just returns an error response. This means users who upgrade the SDK but don't update their Docker image will get a 90-second timeout followed by a 503 error, with no fallback to port 3000.
Either implement the fallback as described, or update the PR description to accurately reflect the current behavior (hard cutover, no backwards compatibility). Given aron-cf's concern about breaking existing users, the fallback seems important.
5. Example Dockerfiles have misleading --build-arg comments
examples/claude-code/Dockerfile and examples/openai-agents/Dockerfile now include comments like:
# Override with --build-arg BUILD_INTERNAL_SERVER_PORT=<port> if using a custom control port
EXPOSE 8671But these Dockerfiles don't declare ARG BUILD_INTERNAL_SERVER_PORT — the EXPOSE is a hardcoded literal 8671. The build-arg comment suggests you can change it, but passing --build-arg BUILD_INTERNAL_SERVER_PORT=9500 would have no effect. Either add the ARG declaration and use EXPOSE ${BUILD_INTERNAL_SERVER_PORT}, or remove the misleading comment.
6. Example Dockerfiles downgrade base image from 0.7.18 to 0.7.17
Both examples/claude-code/Dockerfile and examples/openai-agents/Dockerfile change from cloudflare/sandbox:0.7.18 to cloudflare/sandbox:0.7.17. This appears to be an accidental revert — the version change is unrelated to the port configuration work.
Minor (non-blocking)
- The
desktopstage in the Dockerfile inherits EXPOSE fromruntime-basebut doesn't explicitly show this. No change needed, just a readability note. - E2E test
tests/e2e/control-port-workflow.test.tsis correctly picked up by the vitest E2E glob (tests/e2e/**/*.test.ts), confirming it will run in CI.
|
Review posted on PR #490. Here's a summary of what I found: Requested changes (6 items):
Positive findings:
|
|
@Muhammad-Bin-Ali Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
There was a problem hiding this comment.
I've addressed the outstanding review issues from bonk and Devin's reviews. Here's a summary of what was fixed and what I found:
Fixes applied (commit 24b1d60)
-
Changeset
minor→patch— Per AGENTS.md beta convention. Fixed. -
NaN guard in
request-handler.ts:40-42— Added|| DEFAULT_CONTROL_PORTfallback so non-numericSANDBOX_CONTROL_PORTvalues fall back to the default, matching the pattern insandbox.ts:395andconfig.ts:12. Added a test case for this scenario. -
Removed
config.test.ts— Per aron-cf's feedback and Muhammad-Bin-Ali's agreement. -
Example Dockerfiles: base image 0.7.17 → 0.7.18 — This was an accidental downgrade. Restored to current version on main.
-
Example Dockerfiles: misleading
--build-argcomments — Removed theBUILD_INTERNAL_SERVER_PORTcomments since these example Dockerfiles don't declare theARGand the build-arg would be silently ignored.
Correction to bonk's latest review
Bonk's review #4 claimed startWithLegacyFallback() doesn't exist — this is incorrect. The method is implemented at packages/sandbox/src/sandbox.ts:1347-1395 and has thorough test coverage in packages/sandbox/tests/sandbox-control-port.test.ts:148-329. The fallback logic works as described in the PR body.
|
All done. Here's a summary of what I did on PR #490: 6 fixes applied in commit
I also corrected bonk's latest review claim that |
24b1d60 to
41a2800
Compare
dc1e5f1 to
d60875b
Compare
The configurable control port feature threaded getControlPort through proxy-side functions (connect, enhanceSession, proxyTerminal, terminal). This is unnecessary because the DO already owns defaultPort and handles routing via Container.fetch. Port validation now uses the cf-container-target-port header in Sandbox.fetch(), which correctly distinguishes user WebSocket connections from control-plane WebSockets without fragile path exemptions. The legacy fallback remap is fixed to work with custom SANDBOX_CONTROL_PORT values, not just the default.
| this.defaultPort = LEGACY_CONTROL_PORT; | ||
| this.client = this.createSandboxClient(); |
There was a problem hiding this comment.
🔴 CodeInterpreter holds stale InterpreterClient after legacy port fallback
When startWithLegacyFallback() successfully falls back to port 3000, it recreates this.client (line 1674) but does NOT recreate this.codeInterpreter. The CodeInterpreter constructor eagerly caches sandbox.client.interpreter as this.interpreterClient (packages/sandbox/src/interpreter.ts:19-22). After fallback, this.codeInterpreter.interpreterClient still holds the old InterpreterClient whose transport is configured with the original port (e.g. 8671). All subsequent runCode(), createCodeContext(), listCodeContexts(), deleteCodeContext(), and runCodeStream() calls route through this stale client, sending requests to port 8671 on a container that only listens on port 3000. This causes all code interpreter operations to fail after a legacy fallback is triggered.
| this.defaultPort = LEGACY_CONTROL_PORT; | |
| this.client = this.createSandboxClient(); | |
| this.defaultPort = LEGACY_CONTROL_PORT; | |
| this.client = this.createSandboxClient(); | |
| this.codeInterpreter = new CodeInterpreter(this); |
Was this helpful? React with 👍 or 👎 to provide feedback.
This reverts commit 063bdf1.
* Make container control plane port configurable via SANDBOX_CONTROL_PORT env var * Propagate configurable control port through SDK and shared types * Update Dockerfiles * Add tests * Update existing tests * Add e2e tests * Add changeset * Fix e2e tests * Bonk changes * Remove unneeded test + provide templating for control port in docker files * Update markdown files * Make port configurable * Address bonk's comments * Address Devin comments * Address Devin * Change changeset to patch * Fix missing import * Move control port validation into the Durable Object (#528) The configurable control port feature threaded getControlPort through proxy-side functions (connect, enhanceSession, proxyTerminal, terminal). This is unnecessary because the DO already owns defaultPort and handles routing via Container.fetch. Port validation now uses the cf-container-target-port header in Sandbox.fetch(), which correctly distinguishes user WebSocket connections from control-plane WebSockets without fragile path exemptions. The legacy fallback remap is fixed to work with custom SANDBOX_CONTROL_PORT values, not just the default. --------- Co-authored-by: Muhammad Ali <muhammadali@cloudflare.com> Co-authored-by: Naresh <ghostwriternr@gmail.com>
Closes #279
Summary
Changes the container control plane port from 3000 to 8671, resolving frequent conflicts with user services (Express, Next.js, etc.) that bind to port 3000 by default.
The port is configurable via the
SANDBOX_CONTROL_PORTenvironment variable for edge cases where 8671 is also in use.Changes
SERVER_PORTcentralized inconfig.ts, read fromSANDBOX_CONTROL_PORTenv varstartWithLegacyFallback()wrapsstartAndWaitForPortswith a catch-retry to port 3000 when the container is running but not listening on the configured port. After fallback,defaultPortis updated and the current request's port is remapped sosuper.containerFetchroutes correctly.validatePort()now takes the control port as a parameter; port 3000 is no longer reservedEXPOSEdirectives templated viaBUILD_INTERNAL_SERVER_PORTbuild arg (default: 8671)docker-local.shanddocker-bake.hclpass the build arg explicitlyAGENTS.md,STANDALONE_BINARY.md,SESSION_EXECUTION_DEEP_DIVE.mdConfiguration
No action needed for most users — the default port changes from 3000 to 8671 automatically.
To customize:
Backwards Compatibility
Users who upgrade the SDK but don't update their Docker base image will see:
startAndWaitForPortson the configured port (8671) firstTradeoff: The initial
startAndWaitForPorts(8671)runs the fullportReadyTimeoutMS(default 90s) before the fallback kicks in. This is a one-time cost per DO instance for mismatched versions. The discovered port is not persisted to DO storage, so after a DO eviction the next request re-runs the fallback.Breaking Changes
EXPOSE 3000toEXPOSE 8671(or use--build-arg BUILD_INTERNAL_SERVER_PORT=8671)