Skip to content
Open
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
29 changes: 29 additions & 0 deletions .github/workflows/interpreter-example-test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
name: Interpreter Example Test

on:
pull_request:
paths:
- 'examples/run-code-env-inheritance/**'
- '.github/workflows/interpreter-example-test.yml'
push:
branches: [master]
paths:
- 'examples/run-code-env-inheritance/**'
- '.github/workflows/interpreter-example-test.yml'
workflow_dispatch:

permissions:
contents: read

jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6
- uses: actions/setup-python@v6
with:
python-version: '3.11'
- name: Install interpreter test dependencies
run: python -m pip install fastapi==0.119.0 uvicorn==0.38.0 httpx==0.28.1 websockets==15.0.1
- name: Run startup and environment regression tests
run: PYTHONPATH=examples/run-code-env-inheritance/lightweight-code-interpreter python -m unittest discover -s examples/run-code-env-inheritance/tests -v

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The PR's central mechanism — real uvicorn.Server adopting the pre-bound socket via run(sockets=[...]) — is never exercised: every launch.py test replaces uvicorn.Server with a Mock. If the image's uvicorn version behaves differently (signature, socket ownership/create_server(sock=...) requirements), CI stays green and the container fails at start. Since uvicorn is already installed here, a small test using a trivial ASGI app (uvicorn.Config(app, host="127.0.0.1", port=0)) with Server.run(sockets=[prebound]) in a thread and a GET / would close that gap without needing Jupyter.

Separately, these pins are inline in the workflow while the README recommends running the tests inside the built image to use the same dependencies as the runtime; a shared requirements file (or running the suite in the image) would keep the two from silently diverging.

7 changes: 7 additions & 0 deletions examples/run-code-env-inheritance/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,10 @@ ARG SANDBOX_CODE_IMAGE=cube-sandbox-cn.tencentcloudcr.com/cube-sandbox/sandbox-c
FROM ${SANDBOX_CODE_IMAGE}

COPY lightweight-code-interpreter/server.py /opt/lightweight-code-interpreter/server.py
COPY lightweight-code-interpreter/launch.py /opt/lightweight-code-interpreter/launch.py

RUN test -x /usr/bin/tini && command -v python && command -v jupyter \
&& python -c "import uvicorn; import jupyter_server"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The build check does not actually validate the "server imports" it is meant to guard. launch.py hands server:app to Uvicorn, and server.py imports fastapi, httpx, starlette, pydantic and from websockets.client import WebSocketClientProtocol, connect — none of which are covered by import uvicorn; import jupyter_server.

That matters here because the README explicitly supports overriding SANDBOX_CODE_IMAGE (international registry, pinned base). A base image with an incompatible/moved websockets (the legacy websockets.client API is the fragile one) or a missing fastapi passes this check and then dies at Config.load() inside Uvicorn — exactly the failure this build-time check exists to catch. Importing the copied module covers the full runtime dependency set:

RUN test -x /usr/bin/tini && command -v python && command -v jupyter \
    && python -c "import sys; sys.path.insert(0, '/opt/lightweight-code-interpreter'); import server"


ENTRYPOINT ["/usr/bin/tini", "--"]
CMD ["python", "/opt/lightweight-code-interpreter/launch.py"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Only CMD is set, so the base image's ENTRYPOINT still runs. If it is cube-entrypoint.sh (the documented Cube contract — it always starts envd on ${ENVD_PORT}), launch.py starts a second envd on the same port; if the base ENTRYPOINT starts Jupyter/uvicorn itself, launch.py may never run at all. Since this PR does no image build or sandbox acceptance, please check the base image's Config.Entrypoint/Config.Cmd (e.g. docker inspect) and reset the entrypoint (e.g. ENTRYPOINT []) if the launcher is meant to own startup.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Verify the inherited ENTRYPOINT before relying on this CMD.

This file replaces only CMD; ENTRYPOINT is inherited from sandbox-code. CubeSandbox base images use docker/Dockerfile.cube-base:97 (ENTRYPOINT ["/usr/bin/tini","--","/usr/local/bin/cube-entrypoint.sh"]), and docker/cube-entrypoint.sh:50 unconditionally starts envd -port ${ENVD_PORT:-49983} (with -isnotfc appended) before running the CMD. If sandbox-code keeps that entrypoint, envd is already listening on 49983 when launch.py runs, so the launcher's own envd loses the bind and exits while the launcher still prints envd started on :49983 — and the exporter/log fix described in the README would apply to a process that is not the one serving /envs.

Please confirm with docker inspect --format '{{json .Config.Entrypoint}}' <registry>/sandbox-code:latest. If an entrypoint already starts envd, drop the envd spawn from launch.py; if not, keep it. This is exactly the base-image interaction the PR says was not acceptance-tested.

Also worth confirming: overriding CMD replaces the base image's entire startup command, so launch.py must replicate everything else it did (extra Jupyter/uvicorn flags, any init step), not just the interpreter.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This CMD replaces whatever startup command the base sandbox-code image ships, but the PR does not build or run the derived image (the description says so). The inherited ENTRYPOINT is still in effect: CubeSandbox images use cube-entrypoint.sh, which already starts envd ... -isnotfc in the background (docker/cube-entrypoint.sh:37-50, docs/guide/tutorials/template-build-practice.md:279-291). If sandbox-code follows that contract, launch.py will start a second envd on the same ENVD_PORT, and the one actually serving :49983 is still the entrypoint's (unsupervised) process.

Please confirm the base image's ENTRYPOINT/CMD split, and run the docker build + docker run smoke check from the README before relying on this. If the entrypoint already launches envd, drop the new Popen (or skip it when envd is already reachable) instead of racing it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Replacing both the inherited ENTRYPOINT and CMD means the base sandbox-code image's own command no longer runs, so launch.py has to reproduce it exactly. The base image is external to this repo and the PR says the derived image was never built or run, so a mismatch here would only show up as a sandbox that never starts. Please confirm before merge:

  • python is on PATH in the base image, and is the interpreter that actually has uvicorn/jupyter installed (not, e.g., only /opt/venv/bin/python).
  • /usr/bin/tini exists in sandbox-code (it does in cubesandbox-base, but sandbox-code is built elsewhere).
  • The uvicorn server:app invocation in launch.py (host/port, log_level="info", timeout_keep_alive=640) and the Jupyter flag set (--IdentityProvider.token=, --ServerApp.port_retries=0, --ServerApp.root_dir, --ServerApp.allow_root, --ServerApp.disable_check_xsrf, --ServerApp.open_browser) match what the base command used, and nothing else that command did (extra setup, user/working-dir handling, other sidecars) is lost.

A docker run --rm -p 49999:49999 -p 49983:49983 <derived image> plus curl localhost:49999/health and curl localhost:49983/health would settle all of this cheaply.

16 changes: 15 additions & 1 deletion examples/run-code-env-inheritance/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,21 @@ Sandbox-level values and per-call overrides are applied in a separate background

The next execution waits for pending cleanup so per-call values cannot leak into later executions. This lifecycle follows E2B code-interpreter. Sandbox-level environment variables are loaded only once per Jupyter kernel; this example does not provide a general runtime refresh mechanism.

This derived interpreter requires envd's `/envs` endpoint. If envd is unavailable, the endpoint is unsupported, the request exceeds `ENVD_TIMEOUT` (2 seconds by default), or the payload is invalid, `run_code` fails with HTTP 502 instead of falling back to the stock interpreter behavior. Set `ENVD_TIMEOUT` in the image environment to adjust the fetch timeout.
This derived interpreter requires envd's `/envs` endpoint. Startup waits for the endpoint within the context-creation budget and fails if it cannot become ready. After startup, an unavailable endpoint, a request exceeding `ENVD_TIMEOUT` (2 seconds by default), or an invalid payload causes `run_code` to return HTTP 502. Set `ENVD_TIMEOUT` in the image environment to adjust the fetch timeout.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

README_zh.md is not updated by this PR, so the Chinese mirror still documents the old contract (e.g. that run_code returns 502 whenever envd is unavailable, with no mention of the new startup gate / retry semantics / session cleanup). Please mirror the new paragraphs so the two translations agree.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This new paragraph (and the ones below on context-creation timeouts, the tini launcher, and JUPYTER_LOG_FILE) has no counterpart in README_zh.md, which these docs otherwise keep in sync (the two files link to each other). More importantly the Chinese text is now wrong: it still says an unavailable envd or unsupported /envs makes run_code return HTTP 502, whereas startup now fails and the interpreter never becomes ready. Please mirror the update so the two READMEs describe the same startup contract.


Jupyter context creation, WebSocket opening, and working-directory initialization each use `CODE_INTERPRETER_CONTEXT_CREATE_TIMEOUT` (60 seconds by default), rather than the HTTP and WebSocket defaults of 5 and 10 seconds during concurrent cold starts. A timed-out creation request is not replayed because the kernel may already exist. If no session ID was returned, that session cannot be reclaimed through this cleanup path and may live until the sandbox shuts down. If the session ID is known, a failed handshake closes the connection and attempts to delete that session. Startup also verifies the required envd `/envs` endpoint within the context-creation budget. Failed startup closes the HTTP client without becoming ready. User-code execution timeouts are unchanged.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Documentation parity: README_zh.md is the linked translation of this file but none of the new behavior/limits (context-create timeout, entrypoint/launcher, port reservation, -isnotfc) were added there. The Korean/Chinese readers will still see the old build-and-run story, including the old "envd must be available, otherwise run_code returns 502" wording, which no longer matches the new startup-time envd gate. Please mirror this block in README_zh.md (or drop the translation link until it is updated).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

README_zh.md is the mirrored, cross-linked Chinese version of this file but is not updated here. It still says run_code returns 502 when envd is unavailable and documents neither the new startup semantics, CODE_INTERPRETER_CONTEXT_CREATE_TIMEOUT, the entrypoint/launcher change, nor the -isnotfc rationale. The docs-bilingual-check workflow only covers docs/guide, not examples/, but every other example keeps README.md/README_zh.md in sync — please update README_zh.md in this PR.

Also, the launcher honors JUPYTER_LOG_FILE (default /var/log/jupyter.log) but only the ENVD_* knobs are listed in the "honors ..." sentence above; consider mentioning it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These four new behavior paragraphs are inserted after the ## Build and test the image heading and before From the repository root:, so the build section now starts with startup/launcher prose before its command. They belong in the environment-semantics section above, next to the changed ENVD_TIMEOUT paragraph.


The derived image replaces the inherited entrypoint with tini so the launcher owns dependency startup and does not start a second envd. It honors `ENVD_BIN`, `ENVD_EXTRA_ARGS`, and `ENVD_LOG_FILE=-`. Jupyter is configured to fail on a port conflict instead of selecting another port.

The launcher binds and listens on the interpreter port before starting envd, Jupyter, or application initialization, then passes that same socket to Uvicorn. Uvicorn normally binds after its lifespan startup; dynamic ports allocated during initialization could otherwise take the default port `49999` and terminate startup with `address already in use`. Dependency processes do not inherit the listener and are terminated and reaped on startup failure or shutdown. The image keeps `/usr/bin/tini` as PID 1 to reap orphaned grandchildren, while the Python launcher manages its direct children. Reserving the port does not mean the application is ready.

The launcher starts envd with `-isnotfc`, matching CubeSandbox's base-image entrypoint. Without this flag, envd 0.5.11 enables its Firecracker MMDS-dependent HTTP log exporter; in an environment without that metadata, the exporter can keep buffering logs without a size limit. Non-Firecracker mode avoids that exporter while preserving the existing local stdout/stderr log file. It does not suppress command logs, force garbage collection, or change command/file APIs.

Readiness probes must request the HTTP `/health` endpoint and require a successful
response. The launcher reserves the TCP listener before starting dependencies;
a successful TCP connection alone does not mean startup has completed.
`JUPYTER_LOG_FILE` selects the Jupyter log path (default `/var/log/jupyter.log`);
its parent directory is created before launching the process.

## Build and test the image

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
"""Reserve the interpreter port before starting services or Jupyter kernels."""
import os
from contextlib import ExitStack
from pathlib import Path
import signal
import shlex
import socket
import subprocess
import sys

import uvicorn


def exit_on_signal(signum, frame):
# Uvicorn restores and re-raises captured signals. Exit through Python so
# the launcher can reap children even when it is the container's PID 1.
raise SystemExit(128 + signum)


def stop_child(process):
if process.poll() is not None:
return
process.terminate()
try:
process.wait(timeout=5)
except subprocess.TimeoutExpired:
process.kill()
process.wait(timeout=5)


def stop_children(children):
pending_error = sys.exc_info()[0] is not None
first_error = None
for child in reversed(children):
try:
stop_child(child)
except (OSError, subprocess.TimeoutExpired) as error:
print(f"[lightweight-code-interpreter] child cleanup failed: {error}", file=sys.stderr, flush=True)
if first_error is None:
first_error = error
if first_error is not None and not pending_error:
raise first_error


def main():
host = os.getenv("CODE_INTERPRETER_HOST", "0.0.0.0")
port = int(os.getenv("CODE_INTERPRETER_PORT", "49999"))
workdir = Path(os.getenv("CODE_INTERPRETER_WORKDIR", "/workspace"))
jupyter_host = os.getenv("JUPYTER_HOST", "127.0.0.1")
jupyter_port = os.getenv("JUPYTER_PORT", "8888")
envd_port = os.getenv("ENVD_PORT", "49983")
url_host = f"[{jupyter_host}]" if ":" in jupyter_host else jupyter_host
os.environ["JUPYTER_BASE_URL"] = f"http://{url_host}:{jupyter_port}"
workdir.mkdir(parents=True, exist_ok=True)
config = uvicorn.Config(
"server:app", host=host, port=port, log_level="info", timeout_keep_alive=640
)
family = socket.AF_INET6 if ":" in host else socket.AF_INET
children = []
# Uvicorn's usual single-worker path binds after the application lifespan.
# Its lifespan creates a Jupyter kernel, whose dynamic ports (or an HTTP
# client's ephemeral port) may otherwise take the still-unbound service port.
with socket.socket(family, socket.SOCK_STREAM) as listener:
listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
listener.bind((host, port))
listener.listen(socket.SOMAXCONN)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Calling listen() here means TCP connections to the interpreter port now succeed from process start — i.e. throughout the entire lifespan startup, which can be up to _wait_for_jupyter (60 s) + _wait_for_envd (60 s) + session POST/WS/%cd (60 s each) in the worst case. Previously the port was not bound until uvicorn's startup() finished, so connections were refused until the app was ready.

HTTP probes (template creation in the README uses --probe 49999 --probe-path /health) are not fooled — nothing replies until uvicorn accepts — but they will now connect and then wait out their timeout instead of failing fast, and any deployment that probes 49999 with a TCP-connect-only check would report ready before the interpreter is up. Please confirm which probe semantics CubeMaster uses for this template; if TCP-only is ever used, the early listen() turns this fix into a false-positive readiness signal.

listener.set_inheritable(False)
with ExitStack() as stack:
envd_name = os.getenv("ENVD_LOG_FILE") or "/var/log/envd.log"
jupyter_name = os.getenv("JUPYTER_LOG_FILE") or "/var/log/jupyter.log"
for name in (envd_name, jupyter_name):
if name != "-":
Path(name).parent.mkdir(parents=True, exist_ok=True)
envd_log = None if envd_name == "-" else stack.enter_context(open(envd_name, "ab"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both log files are opened directly here, but unlike docker/cube-entrypoint.sh (which runs mkdir -p "$(dirname "${ENVD_LOG_FILE}")" before starting envd) the launcher never creates the parent directory. A user who sets ENVD_LOG_FILE (or the new JUPYTER_LOG_FILE) to a path whose directory does not exist, e.g. ENVD_LOG_FILE=/var/log/e2b/envd.log, now gets FileNotFoundError from open(..., "ab") before any service starts, so the sandbox never boots; the same setting worked with the inherited entrypoint. Consider creating the parent (Path(name).parent.mkdir(parents=True, exist_ok=True)) when the value is not -.

jupyter_log = None if jupyter_name == "-" else stack.enter_context(open(jupyter_name, "ab"))
previous_term = signal.signal(signal.SIGTERM, exit_on_signal)
try:
children.append(subprocess.Popen(
[os.getenv("ENVD_BIN", "/usr/bin/envd"), "-isnotfc", "-port", envd_port]
+ shlex.split(os.getenv("ENVD_EXTRA_ARGS", "")),
stdin=subprocess.DEVNULL, stdout=envd_log,
stderr=subprocess.STDOUT, close_fds=True,
))
Comment on lines +77 to +83

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

envd startup is never verified, so a failed envd is silent.

Popen succeeds even when envd exits immediately (port already in use, missing binary, rejected flag); the launcher then prints envd started and continues. The template probe targets 49999, so the sandbox looks ready while the first run_code fails with a 502 from /envs (server.py:24, ENVD_TIMEOUT). Consider checking process.poll() after a short wait, or polling http://127.0.0.1:{envd_port}/health, and failing with a clear exit code — the same way _wait_for_jupyter handles Jupyter readiness on the app side.

Related: this hardcodes /usr/bin/envd and ignores ENVD_BIN, ENVD_EXTRA_ARGS, and ENVD_LOG_FILE=- (inherit stdio, per docs/guide/tutorials/bring-your-own-image.md). Note that open(os.getenv("ENVD_LOG_FILE", ...)) would create a file literally named -. If launch.py replaces the entrypoint contract, it should accept at least these variables.

print(f"[lightweight-code-interpreter] envd launched on :{envd_port}", flush=True)
children.append(subprocess.Popen(
["jupyter", "server", "--IdentityProvider.token=",
f"--ServerApp.ip={jupyter_host}", f"--ServerApp.port={jupyter_port}",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Jupyter's default ServerApp.port_retries is 50, so if :8888 is already occupied Jupyter will silently bind 8889+ while JUPYTER_BASE_URL (and hence _wait_for_jupyter) still points at 8888. Startup then spends the full 120×0.5 s ≈ 60 s wait loop and fails with the misleading "Jupyter Server did not become ready".

Since the launcher now owns Jupyter startup, pass --ServerApp.port_retries=0 so a port conflict fails immediately with a clear log line (or parse the actual bound port from Jupyter's output).

"--ServerApp.port_retries=0",
f"--ServerApp.root_dir={workdir}", "--ServerApp.allow_root=True",
"--ServerApp.disable_check_xsrf=True", "--ServerApp.open_browser=False"],
stdin=subprocess.DEVNULL, stdout=jupyter_log,
stderr=subprocess.STDOUT, close_fds=True,
))
print(f"[lightweight-code-interpreter] Jupyter Server started on {os.environ['JUPYTER_BASE_URL']}", flush=True)
server = uvicorn.Server(config)
server.run(sockets=[listener])
if not server.started:
raise SystemExit(3)
finally:
try:
stop_children(children)
finally:
signal.signal(signal.SIGTERM, previous_term)


if __name__ == "__main__":
main()
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
ENVD_TIMEOUT = float(os.getenv("ENVD_TIMEOUT", "2"))
WORKDIR = Path(os.getenv("CODE_INTERPRETER_WORKDIR", "/workspace")).resolve()
DEFAULT_LANGUAGE = "python"
CONTEXT_CREATE_TIMEOUT = float(os.getenv("CODE_INTERPRETER_CONTEXT_CREATE_TIMEOUT", "60"))
PING_TIMEOUT = 30

logging.basicConfig(level=os.getenv("CODE_INTERPRETER_LOG_LEVEL", "INFO"))
Expand Down Expand Up @@ -156,13 +157,14 @@ async def connect(self) -> None:
ws_logger.setLevel(logging.ERROR)
self.ws = await connect(
self.ws_url,
open_timeout=CONTEXT_CREATE_TIMEOUT,
ping_timeout=PING_TIMEOUT,
max_size=None,
max_queue=None,
logger=ws_logger,
)
self.receive_task = asyncio.create_task(self._receive_messages())
await self._run_background(f"%cd {self.cwd}")
await asyncio.wait_for(self._run_background(f"%cd {self.cwd}"), timeout=CONTEXT_CREATE_TIMEOUT)

async def _run_background(self, code: str) -> None:
if self.ws is None:
Expand Down Expand Up @@ -439,6 +441,29 @@ async def _wait_for_jupyter() -> None:
raise RuntimeError("Jupyter Server did not become ready")


Copy link
Copy Markdown

Choose a reason for hiding this comment

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

_wait_for_envd retries on every HTTPException, but _fetch_sandbox_envs collapses all failures into a 502 HTTPException: HTTP status errors (including a 404 for an envd without /envs — the "endpoint unsupported" case the README calls out), connection/timeouts, and invalid payloads. For the permanent ones the loop can never make progress, so startup spins for the full CONTEXT_CREATE_TIMEOUT (60 s default) and then raises the generic envd did not become ready, discarding the actual 404/invalid-payload cause. Before this change an unsupported /envs produced an immediate 502 on run_code; now it silently costs a minute at every startup. Consider retrying only connection/timeout/5xx failures and failing fast (or surfacing the status/detail) for 4xx or invalid payloads.

async def _wait_for_envd(http_client: httpx.AsyncClient) -> None:
async def ready():
while True:
try:
await _fetch_sandbox_envs(http_client)
return
except HTTPException as exc:
cause = exc.__cause__
retryable = isinstance(cause, httpx.TransportError) or (
isinstance(cause, httpx.HTTPStatusError)
and (cause.response.status_code >= 500 or cause.response.status_code == 429)
)
if not retryable:
reason = f"{exc.detail}: {cause}" if cause else exc.detail
raise RuntimeError(f"envd startup failed: {reason}") from exc
await asyncio.sleep(0.1)

try:
await asyncio.wait_for(ready(), timeout=CONTEXT_CREATE_TIMEOUT)
except asyncio.TimeoutError as exc:
raise RuntimeError("envd did not become ready") from exc


contexts: dict[str, KernelContext] = {}
default_context_id: str | None = None
client: httpx.AsyncClient | None = None
Expand All @@ -451,6 +476,7 @@ async def create_context(language: str | None = None, cwd: str | None = None) ->

response = await client.post(
f"{JUPYTER_BASE_URL}/api/sessions",
timeout=CONTEXT_CREATE_TIMEOUT,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A POST timeout can still leave an orphan session/kernel.

httpx.ReadTimeout only means the client gave up; the Jupyter server may still create the session, and nothing reclaims it. The README correctly states the request is not replayed, but it doesn't mention that the result is leaked — a timed-out creation under exactly the cold-start burst this PR targets leaves a kernel running until the sandbox dies.

The request already sends unique UUIDs for path and name, so cleanup is possible without guessing: best-effort GET /api/sessions and delete the single session whose path/name matches those UUIDs. If that is out of scope, please at least document the residual kernel leak in the README limitation paragraph.

json={
"path": str(uuid.uuid4()),
"kernel": {"name": "python3"},
Expand All @@ -464,7 +490,20 @@ async def create_context(language: str | None = None, cwd: str | None = None) ->
session = response.json()
context_id = session["kernel"]["id"]
kernel = KernelContext(context_id, session["id"], resolved_cwd)
await kernel.connect()
try:
await kernel.connect()
except BaseException:
# The session ID is known here, so cleanup is safe without replaying
# creation. Keep the original handshake/initialization exception.
try:
await kernel.close()
except BaseException as cleanup_error:
logger.warning("failed context connection cleanup: %s", cleanup_error)
try:
await client.delete(f"{JUPYTER_BASE_URL}/api/sessions/{session['id']}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The session DELETE is only attempted when kernel.close() returns normally. Any exception from close() (websocket already closed, cancellation, a failing cleanup task) skips it and leaves the Jupyter session/kernel that POST /api/sessions just created — the failed-session leak this change is meant to prevent. Run the DELETE in its own try/except so it is independent of the websocket teardown result.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The session DELETE is skipped if kernel.close() raises.

Both cleanup steps share one try, so a failing self.ws.close() (broken connection, aborted close, cancellation during shutdown) prevents client.delete(...) from running — leaving behind the session/kernel created by the successful POST, which is the leak this block is meant to prevent. WebSocketClientProtocol.close() can raise, so this is not a hypothetical. Split the two steps (or wrap delete in finally) so the DELETE is always attempted:

try:
    await kernel.close()
except Exception as cleanup_error:
    logger.warning("failed context close: %s", cleanup_error)
try:
    await client.delete(f"{JUPYTER_BASE_URL}/api/sessions/{session['id']}")
except Exception as cleanup_error:
    logger.warning("failed session cleanup: %s", cleanup_error)
raise

The README claim "closes the connection and attempts to delete that session" then holds unconditionally.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

kernel.close() and the session DELETE share one try. If kernel.close() raises (the WebSocket close path is not guaranteed to succeed), the surrounding except Exception swallows it and client.delete(...) is skipped — leaking the very session this block was added to clean up. The DELETE response is also never checked, so a non-2xx cleanup failure is silent.

Wrap the two steps separately (or use try/finally) so the session deletion is always attempted, and log non-2xx responses.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The compensating DELETE runs on the shared client with the library default timeout (5s), even though this failure branch is entered precisely when Jupyter is slow enough to blow the newly-raised CONTEXT_CREATE_TIMEOUT (60s). Under that same cold-start pressure the cleanup request is likely to time out too, and the exception is swallowed by the except BaseException below, so the session/kernel still leaks until sandbox teardown — which is the outcome this block is trying to avoid.

Worth passing an explicit, smaller-but-not-default timeout, e.g. reuse a short bound so the compensating delete does not add 5s to every failed creation:

await client.delete(
    f"{JUPYTER_BASE_URL}/api/sessions/{session['id']}",
    timeout=min(CONTEXT_CREATE_TIMEOUT, 30),
)

(Not blocking; the current best-effort behaviour is already documented in the PR description.)

except BaseException as cleanup_error:
logger.warning("failed context session cleanup: %s", cleanup_error)
raise
contexts[context_id] = kernel
return Context(id=context_id, language=DEFAULT_LANGUAGE, cwd=resolved_cwd)

Expand All @@ -474,14 +513,18 @@ async def lifespan(app: FastAPI):
global client, default_context_id
await _wait_for_jupyter()
client = httpx.AsyncClient()
default = await create_context(DEFAULT_LANGUAGE, str(WORKDIR))
default_context_id = default.id
try:
await _wait_for_envd(client)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Making interpreter readiness depend on envd /envs changes the failure mode from degraded to unbootable. With _wait_for_envd in the lifespan, an envd that is missing, that lacks /envs, or that returns a non-retryable error now aborts uvicorn startup: launch.py exits non-zero and /health is never served, so the template's --probe 49999 --probe-path /health can never pass. Before this change the interpreter started and only run_code returned 502 (README, "Environment semantics").

The part that needs verification is timing: this now runs at container boot, before the sandbox is initialized via POST :49983/init (create-time env vars are applied at /init — see docs/guide/tutorials/bring-your-own-image.md:18). If envd answers /envs with 4xx/401/403 before /init, _wait_for_envd classifies it as non-retryable and startup fails. The added tests mock the HTTP layer, and the PR body states no live acceptance was run, so this path is unverified.

Could you confirm that /envs returns 2xx before /init in the target sandbox-code image? If it may not, consider gating readiness on /health and keeping the /envs capability check lazy, or retrying 4xx until the timeout rather than failing immediately.

# Kernel startup competes with other guests during a cold-start burst.
# Do not replay POST /sessions on timeout: its result can be unknown.
default = await create_context(DEFAULT_LANGUAGE, str(WORKDIR))
default_context_id = default.id
yield
finally:
for context in list(contexts.values()):
await context.close()
contexts.clear()
default_context_id = None
if client is not None:
await client.aclose()

Expand Down
Loading
Loading