Repository navigation
Conversation
AdaAibaby
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
November 11, 2026 03:54
…ting the pool Re: e2b-dev#3681. The client-proxy routes every sandbox's ingress through one shared keepalive connection pool (pool.Destination.ConnectionKey is the constant "client-proxy", deliberately, so connections are reused across sandboxes). It passed a nil ConnectionLimitConfig to the shared reverse proxy, so there was no per-sandbox bound on concurrent in-flight requests — the only ceiling was the process-wide pool cap (maxClientConns=16384) and the global resume parking lot. One sandbox or team could occupy the shared edge without per-sandbox accounting, the boundary e2b-dev#3681 asks about. The shared reverse proxy already keys both the connection pool AND the ingress limiter off ConnectionKey, so simply keying the limiter per sandbox would also fragment the pool (one pool per sandbox — the opposite of the intended reuse). Decouple the two keys: - pool.Destination gains LimitKey + LimiterKey(): the limiter accounts against LimitKey when set, else falls back to ConnectionKey. The pool keeps using ConnectionKey. Embedders that key the pool per lifecycle (orchestrator) set no LimitKey and are byte-identical to before. - handler.go limiter TryAcquire/Release now use d.LimiterKey(). - client-proxy wires a non-nil ConnectionLimitConfig and sets LimitKey to the sandbox ID, so limiting is per sandbox while the pool stays shared. - New flag ClientProxyMaxConnectionsPerSandbox, default -1 (no limit): behavior is unchanged until an operator sets a finite cap, symmetric with SandboxMaxIncomingConnections on the orchestrator proxy. Tests: pool.Destination.LimiterKey fallback/override; shared proxy package and orchestrator proxy package unaffected (the fallback makes the orchestrator path identical).
AdaAibaby
force-pushed
the
fix/clientproxy-per-sandbox-connlimit
branch
from
November 11, 2026 04:03
9dbee6a to
23cee2d
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Give the client-proxy a per-sandbox bound on concurrent in-flight ingress requests, without fragmenting its shared keepalive connection pool. Gated behind a new flag that defaults to unlimited, so behavior is unchanged until an operator opts in.
Why
Re #3681 (shared-host-service resource accounting). The client-proxy routes every sandbox's ingress through a single shared keepalive pool —
pool.Destination.ConnectionKeyis the constant"client-proxy"(packages/shared/pkg/proxy/pool/pool.go), deliberately, so connections are reused across sandboxes. It passed a nilConnectionLimitConfigto the shared reverse proxy (packages/client-proxy/internal/proxy/proxy.go), so there was no per-sandbox bound on concurrent in-flight requests. The only ceilings were process-wide: the pool capmaxClientConns = 16384and the global resume parking lot (parkingLot.activeis oneatomic.Int64, default size 512 — not keyed per sandbox, and only for the resume-retry subset).So one sandbox/team driving many concurrent slow routes consumes client-proxy goroutines/connections bounded only by those global ceilings, not attributed per sandbox — exactly the boundary #3681 asks about. (The orchestrator-side proxy already limits per sandbox via
SandboxMaxIncomingConnections; the client-proxy edge had no equivalent.)The coupling that made this non-trivial
The shared reverse proxy keys both the connection pool (
pool.go:p.pool.Upsert(d.ConnectionKey, ...)) and the ingress limiter (handler.go:limiter.TryAcquire(d.ConnectionKey, ...)) off the sameConnectionKey. Naively keying the limiter per sandbox would therefore also split the pool into one-per-sandbox — the opposite of the intended reuse (fd/connection blow-up).Fix — decouple the limiter key from the pool key
pool.DestinationgainsLimitKey+LimiterKey(): the limiter accounts againstLimitKeywhen set, else falls back toConnectionKey. The pool keeps usingConnectionKey.handler.golimiterTryAcquire/Releaseused.LimiterKey().LimitKey→LimiterKey()returnsConnectionKey→ byte-identical to before.ConnectionLimitConfigand setsDestination.LimitKey = sandboxId, so limiting is per sandbox whileConnectionKeystays the shared"client-proxy"(pool intact).ClientProxyMaxConnectionsPerSandbox, default-1(no limit) — symmetric withSandboxMaxIncomingConnections. Inert until an operator sets a finite cap.Changes
shared/pkg/proxy/pool/destination.goLimitKey+LimiterKey()(limiter key ≠ pool key)shared/pkg/proxy/handler.god.LimiterKey()shared/pkg/featureflags/flags.goClientProxyMaxConnectionsPerSandbox(default -1)client-proxy/internal/proxy/proxy.goConnectionLimitConfig;LimitKey = sandboxIdshared/pkg/proxy/pool/destination_limiterkey_test.goLimiterKey()fallback/overrideTesting
go test ./packages/shared/pkg/proxy/pool -run LimiterKey✅,go test ./packages/shared/pkg/proxy✅go test ./packages/orchestrator/pkg/proxy✅ (fallback makes the orchestrator path identical)go build/go vet/gofmtclean for shared + client-proxyTestResumeFlights_...panic (redis: invalid URL scheme, needsREDIS_URL) reproduces on cleanmain; unrelated to this change.Scope / follow-ups (not in this PR)
ReadHeaderTimeout— a slow-header bound (cf. the orchestrator tcpfirewall classification work) is a separate change.