feat(worker): rebuild E2B bridge in Go - #14
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40fee29b7f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sandbox, err := m.backend.Create(ctx, m.template, timeout) | ||
| session.sandbox = sandbox | ||
| session.initErr = err |
There was a problem hiding this comment.
When shutdown occurs while a terminal sandbox is being created, Run can call terminalManager.Close() without waiting for detached command goroutines. Close may therefore observe session.sandbox == nil, clear the session map, and return just before this assignment publishes a successfully created sandbox; the subsequent canceled execution cannot find the session in releaseAndDestroySession, so the sandbox is never killed and remains billable until its E2B timeout. Synchronize creation with Close or track and wait for in-flight creations.
Useful? React with 👍 / 👎.
| if err := waitReconnect(ctx, reconnectDelay); err != nil { | ||
| return err | ||
| } | ||
| reconnectDelay = nextReconnectDelay(reconnectDelay) |
There was a problem hiding this comment.
After any connection-failure streak, reconnectDelay is carried through a later successful connect_ack and is only increased here. Consequently, once it reaches 15 seconds, even a connection that remained healthy for a long time waits the maximum delay after its next ordinary disconnect, and all subsequent reconnects continue doing so. Reset the backoff when a session successfully completes its handshake so exponential delay applies to consecutive failures rather than the worker's entire lifetime.
Useful? React with 👍 / 👎.
| if sandbox == nil { | ||
| return errors.New("E2B sandbox is unavailable") | ||
| } | ||
| return m.backend.SetTimeout(ctx, sandbox.ID, secondsUntil(expires)) |
There was a problem hiding this comment.
When a terminal command runs longer than its requested lease—for example, a 120-second command with lease_ttl_sec=60—this sets E2B's sandbox timeout only to the lease expiry before starting the command. Unlike the local janitor, E2B does not account for inflight, so it terminates the sandbox at 60 seconds and the otherwise-valid command fails midway. Set the remote timeout to cover both the lease and the active command deadline, then reduce it to the remaining lease after the command finishes.
Useful? React with 👍 / 👎.
| m.mu.Lock() | ||
| expires := session.leaseExpiresAt | ||
| sandbox := session.sandbox | ||
| m.mu.Unlock() |
There was a problem hiding this comment.
With concurrent calls on one session, a request can snapshot the old shorter leaseExpiresAt, another request can extend the lease and complete its longer SetTimeout first, and then the older network request can complete last and shorten E2B's timeout again. The local lease remains extended while the remote sandbox expires early, causing later calls to lose the session. Serialize timeout updates or reject stale completions so the remote expiry is monotonic with the manager's lease.
Useful? React with 👍 / 👎.
| controlHTTP: &http.Client{Transport: transport.Clone(), Timeout: requestTimeout}, | ||
| sandboxHTTP: &http.Client{Transport: transport}, |
There was a problem hiding this comment.
When envd accepts a connection but never returns response headers, WORKER_E2B_REQUEST_TIMEOUT_SEC does not bound the request because sandboxHTTP has neither a client timeout nor a transport response-header timeout. A task with a longer deadline can therefore occupy its capability and session slots for that entire deadline despite the documented E2B request timeout. Apply the configured timeout to envd connection/header establishment without imposing it on the duration of a successfully started streaming command.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,21 @@ | |||
| module github.com/onlyboxes/onlyboxes/worker/worker-bridge-e2b | |||
There was a problem hiding this comment.
The checked .github/workflows/ci.yml path filters and jobs cover console, worker-docker, worker-sys, and worker-boxlite, but never worker/worker-bridge-e2b/**. Because this commit changes only that new module, the repository CI selects no Go build, vet, or test job for it, and future bridge changes can likewise merge without even compiling. Add a bridge path-filter output and check job, and include that job in ci-ok.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99ec500fa9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if m.exportMaxBytes > 0 && probe.Size > int64(m.exportMaxBytes) { | ||
| return terminalResourceRunResult{}, newTerminalExecError(terminalResourceCodeFileTooLarge, "file exceeds export limit") | ||
| } | ||
| if err := m.exportResource(ctx, session.sandbox, filePath, signedURL, headers); err != nil { |
There was a problem hiding this comment.
When WORKER_TERMINAL_EXPORT_MAX_BYTES is nonzero and another permitted same-session command grows or replaces the file after this probe, the check still approves the old size while sandbox export streams the entire new file; worker-mode export is likewise unbounded when envd returns an unknown content length. Because same-session operations are concurrent by default, the configured export cap can be bypassed. Apply the limit to the bytes actually uploaded rather than only to preliminary metadata.
Useful? React with 👍 / 👎.
| // Version is injected at build time via: | ||
| // go build -ldflags "-X github.com/onlyboxes/onlyboxes/worker/worker-bridge-e2b/internal/buildinfo.Version=<version>" | ||
| var Version = defaultVersion |
There was a problem hiding this comment.
The checked .github/workflows/package-release.yml builds and uploads binaries for worker-docker, worker-sys, and worker-boxlite, and publish-release depends only on those worker jobs; there is no build or asset for this new module. Consequently, published releases contain no runnable E2B bridge, and this documented version injection is never performed by the release pipeline. Add a bridge build/upload job and include it in the release gate.
Useful? React with 👍 / 👎.
| encoded := base64.StdEncoding.EncodeToString([]byte(code)) | ||
| command := "printf '%s' '" + encoded + "' | base64 -d > /tmp/onlyboxes-pythonexec.py && uv run /tmp/onlyboxes-pythonexec.py" | ||
| result, err := r.backend.Run(ctx, sandbox, command, -1) |
There was a problem hiding this comment.
When submitted Python code emits large or unending stdout/stderr, passing -1 disables both limitedBuffer caps, so the worker retains the entire stream in memory. A single multi-megabyte result can also exceed gRPC's default message limit when the result is sent, interrupting the registry session, while sustained output can OOM the worker; the default concurrency permits 32 such executions. Apply a finite output limit before accumulating the envd stream.
Useful? React with 👍 / 👎.
| command := "python3 -c '" + script + "'" | ||
| output, err := m.backend.Run(ctx, session.sandbox, command, 64*1024) |
There was a problem hiding this comment.
When an earlier terminalExec modifies ~/.bash_profile or another login startup file, this resource probe runs through backend.Run, which always invokes /bin/bash -l -c. Even a harmless startup echo prefixes the expected JSON and makes decoding fail, while PATH or command hooks can also prevent later reads and exports. Execute internal resource helpers without user-controlled login-shell initialization.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2ef5e9983
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if err := stream.Err(); err != nil { | ||
| if connect.CodeOf(err) == connect.CodeNotFound { | ||
| return CommandResult{}, fmt.Errorf("%w: %v", ErrSandboxNotFound, err) | ||
| } | ||
| return CommandResult{}, fmt.Errorf("receive E2B command output: %w", err) |
There was a problem hiding this comment.
When Start has already emitted a PID and the context expires or the HTTP/2 stream fails, this returns without using the protocol's SendSignal RPC, and the receive loop never retains the PID. Since envd processes are independently addressable from their output streams, a transport failure leaves the command running while Execute releases the session normally; on deadline cancellation, sandbox destruction is also deferred whenever a same-session sibling is active. The orphaned command can therefore continue consuming resources or mutating the shared filesystem after its call has failed; retain the start PID and signal the process on premature stream termination.
Useful? React with 👍 / 👎.
| return commandErrorResult(commandID, "execution_failed", fmt.Sprintf("terminalExec execution failed: %v", err)) | ||
| } | ||
|
|
||
| resultPayload, err := json.Marshal(execResult) |
There was a problem hiding this comment.
When stdout or stderr contains JSON-sensitive bytes such as NULs, the 1 MiB raw-output limit is insufficient because json.Marshal can expand each byte to a six-byte escape; a single stream can therefore exceed 4 MiB, and two quote-heavy streams can also cross that threshold. The console uses an unconfigured grpc.NewServer() at console/internal/grpcserver/server.go:9, so its default receive limit rejects the command result and closes the registry stream instead of returning the truncated output. Apply a limit to the serialized payload or configure a matching receive size on the console.
Useful? React with 👍 / 👎.
| for _, sandbox := range sandboxes { | ||
| m.killSandbox(sandbox) |
There was a problem hiding this comment.
When the worker receives SIGTERM with many live sessions and E2B delete requests are slow or unreachable, this serial loop can spend the separate 10-second cleanup timeout on every sandbox. Even though the new createWG now waits for creations, a typical supervisor grace period can expire after only a few deletes, terminating the worker before the remaining billable sandboxes receive any kill request. Use bounded concurrent cleanup or a shared shutdown deadline so every sandbox is attempted within the grace window.
Useful? React with 👍 / 👎.
| return fmt.Errorf("enqueue heartbeat: %w", err) | ||
| } | ||
|
|
||
| ackTimer := time.NewTimer(cfg.CallTimeout) |
There was a problem hiding this comment.
When many individually valid large results are queued—for example, concurrent terminalResource reads near the default 1 MiB limit—the heartbeat can sit behind up to 64 outbound frames, but this timer starts as soon as the channel accepts it rather than when senderLoop actually calls stream.Send. On a slow console connection, the queued base64 results can consume the entire 13-second default timeout before the heartbeat reaches the wire; two such false misses force a reconnect and discard in-flight results. Prioritize heartbeat frames or have the sender acknowledge the actual send before starting the ack timer.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return pythonExecRunResult{}, fmt.Errorf("create E2B python sandbox: %w", err) | ||
| } | ||
| defer r.killSandbox(sandbox.ID) |
There was a problem hiding this comment.
When the Python process finishes shortly before its dispatch deadline, this deferred cleanup can still block Execute for the independent 10-second delete timeout. If deletion crosses the command deadline, the console unregisters the pending command and reports a timeout even though execution completed successfully; the worker later sends a result that is ignored, and its Python capacity slot was occupied solely by cleanup in the meantime. Track cleanup asynchronously, including during worker shutdown, so remote deletion does not delay the completed command result.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40d4703d68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| session := m.newSessionLocked(sessionID, leaseTarget) | ||
| m.mu.Unlock() | ||
| m.killSandbox(expiredSandbox) |
There was a problem hiding this comment.
When create_if_missing=true replaces an expired session and the E2B delete endpoint is slow or unreachable, this synchronous cleanup can consume the independent 10-second cleanup timeout before the replacement sandbox is created. A command with a shorter timeout_ms expires during the delete, so awaitSessionReady subsequently calls Create with an already-canceled context and all callers waiting on the new session share that failure, even though deleting the expired sandbox is not required to create its replacement. Perform this stale-sandbox cleanup asynchronously or otherwise keep it outside the replacement command's critical path.
Useful? React with 👍 / 👎.
| import architectureDarkUrl from '../../../../static/architecture-dark.svg?url' | ||
| import architectureUrl from '../../../../static/architecture.svg?url' | ||
| import architectureZhCnDarkUrl from '../../../../static/architecture.zh-CN-dark.svg?url' | ||
| import architectureZhCnUrl from '../../../../static/architecture.zh-CN.svg?url' |
There was a problem hiding this comment.
These imports make the website depend on the root static/architecture*.svg files, but the checked website filter in .github/workflows/ci.yml only matches .github/workflows/ci.yml and website/**. A change affecting only these assets—such as accidentally deleting one—therefore skips the website test and build jobs even though the Vite build would have a broken import. Add the imported static asset paths to the website filter.
Useful? React with 👍 / 👎.
Summary
worker-bridge-e2bthat connects to the console worker registry streamecho,pythonExec,terminalExec, andterminalResourceagainst E2B Control API and envdvalidate,read, andexportresource actions, including configurable worker-proxied or sandbox-direct streaming uploadsBehavior
pythonExecuses a one-shot E2B sandbox and always cleans it upterminalExecreuses the E2B sandbox filesystem by session ID while commands run as independent shellsterminalExecandterminalResourceshare the same per-session concurrency limitValidation
go test -race ./...go vet ./...terminalExec, resource read/export, timeout isolation, janitor cleanup, and sandbox cleanup