Repository navigation
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @renyuanc on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Code Review
The AdjustableSemaphore.Acquire implementation has a critical race condition that can lead to a permanent deadlock when the context is cancelled. Because context.AfterFunc runs its callback asynchronously, s.cond.Broadcast can be called before the acquiring goroutine actually enters s.cond.Wait. Since sync.Cond broadcasts are not buffered, the signal is lost and the goroutine blocks indefinitely. To fix this, the AfterFunc callback in resizable_semaphore.go must acquire the semaphore's mutex before calling s.cond.Broadcast to ensure the broadcast is synchronized with the wait state.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Queue builds inside the background goroutine so the gRPC handler stays | ||
| // non-blocking and the node does not oversubscribe build resources. | ||
| if s.buildLimitEnabled.Load() { | ||
| if err := s.buildLimiter.Acquire(ctx, 1); err != nil { |
There was a problem hiding this comment.
The AdjustableSemaphore.Acquire implementation has a critical race condition that can lead to a permanent deadlock when the context is cancelled. Because context.AfterFunc runs its callback asynchronously, s.cond.Broadcast can be called before the acquiring goroutine actually enters s.cond.Wait. Since sync.Cond broadcasts are not buffered, the signal is lost and the goroutine blocks indefinitely. To fix this, the AfterFunc callback in resizable_semaphore.go must acquire the semaphore's mutex before calling s.cond.Broadcast to ensure the broadcast is synchronized with the wait state.
There was a problem hiding this comment.
This is pre-existing in shared code, not introduced by this PR. Suggest to fix it in a separate PR.
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
5aad415 to
d71980e
Compare
Summary
TemplateCreatelaunches one detached goroutine per request with no concurrency limit. Each goroutine drives a full template build, extracting an ext4 rootfs and booting a provisioning Firecracker VM, so N simultaneousTemplateCreatecalls put N rootfs assemblies and N micro-VMs on a single template-manager host at once, with no backpressure or queue. Under a burst (CI fan-out, batch builds, several teams at once) this oversubscribes host disk, I/O, and RAM.This PR bounds concurrent builds per host with a weighted semaphore, gated by a feature flag.
Fixes #3070.
What this does
MaxConcurrentTemplateBuilds(max-concurrent-template-builds, default -1) toflags.go, alongside the existingMaxConcurrent*family. This closes the one heavy concurrent path that lacked a cap (template builds).Building, which the API status poll tolerates) instead of over-committing the host.<= 0, e.g.-1) disables the cap entirely (unbounded builds), matching the repo's-1 = unlimitedconvention (TCPFirewallMaxConnectionsPerSandbox,SandboxMaxIncomingConnections).Testing
go vetclean (linux/amd64, CGO).Building); with the kill switch (-1) a node ran 3 concurrently, confirming the cap is bypassed.Scope / out of scope
This caps build concurrency. It deliberately does not change build lifecycle/timeout handling. One consequence worth noting for reviewers: queuing makes a build sit in
Buildinglonger, which interacts with the API's build-status timeout and the build-cache TTL - a build can be marked failed for queuing too long That's a pre-existing lifecycle gap that this cap amplifies; it should be tracked separately