use promise for the sandbox init - #1710
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| }) | ||
|
|
||
| // Prefetching | ||
| go func() { |
There was a problem hiding this comment.
The goroutine at line 410 silently swallows errors from t.Memfile(ctx) and t.Metadata(). If these operations fail, the prefetcher simply won't run without any visibility. Consider logging these errors before returning.
| var wg errgroup.Group | ||
| // Uffd initialization | ||
| fcUffdPath := sandboxFiles.SandboxUffdSocketPath() | ||
| uffdPromise := utils.NewPromise(func() (*uffd.Uffd, error) { |
There was a problem hiding this comment.
The Promise function closures capture ctx directly (line 392, 452, 476), but these closures execute in separate goroutines that may outlive the original context. If ResumeSandbox returns early due to an error, ctx may be cancelled while the promise goroutines are still running, potentially causing all promises to fail with context.Canceled instead of completing their work.
| ips := <-ipsCh | ||
| if ips.err != nil { | ||
| return nil, fmt.Errorf("failed to get network slot: %w", ips.err) | ||
| rootfs, err := t.Rootfs() |
There was a problem hiding this comment.
Redundant call to t.Rootfs() - this was already fetched and stored in the overlayPromise at line 476. You're fetching it again here when you could reuse the readonlyRootfs from the promise closure, wasting an I/O operation.
| } | ||
|
|
||
| telemetry.ReportEvent(ctx, "got network slot") | ||
| meta, err := t.Metadata() |
There was a problem hiding this comment.
Similarly, t.Metadata() is already called in the prefetch goroutine. This creates redundant work if the metadata fetch is expensive. Consider restructuring to fetch once and reuse.
|
|
||
| telemetry.ReportEvent(ctx, "got snapfile") | ||
|
|
||
| fcUffd, err := uffdPromise.Wait(ctx) |
There was a problem hiding this comment.
The uffdPromise.Wait() call here creates a potential race - if uffdPromise fails before this point, fcUffd will be set to the error value. However, you've already started using fcUffd in the prefetch goroutine (line 426) which may have succeeded with the promise earlier. This could cause inconsistent behavior if promise failures happen at different times.
![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: 2014e95fb2
ℹ️ 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".
| go func(ctx context.Context) { | ||
| returnErr := f.networkPool.Return(ctx, slot) |
There was a problem hiding this comment.
Cleanup now passes the same ctx into networkPool.Return, but Pool.Return short-circuits on ctx.Done() (see packages/orchestrator/internal/sandbox/network/pool.go), so any sandbox teardown that happens after a cancellation/error will fail to return the slot and leak pool capacity. This is a regression from the prior context.WithoutCancel usage in the cleanup path and can accumulate if ResumeSandbox is retried or canceled. Consider wrapping the context (or using a background context) before calling Return so cleanup always returns slots.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| }) | ||
|
|
||
| var meta metadata.Template | ||
| wg.Go(func() error { |
There was a problem hiding this comment.
Resource leak when promises complete after cleanup runs
High Severity
The migration from errgroup to promises removes synchronization that prevents resource leaks. When any promise fails early (e.g., overlayPromise fails), the deferred cleanup.Run() executes immediately. Other promises still running in goroutines (like ipsPromise or uffdPromise) will eventually complete and call cleanup.Add(), but since hasRun is already true, their cleanup functions are silently ignored. This leaks network slots (not returned to pool), uffd handlers (not closed), and rootfs overlays (not closed). The old code had a cleanup function that explicitly waited on the channel before cleanup could proceed, ensuring resources were registered before cleanup ran.
Additional Locations (2)
| if returnErr != nil { | ||
| logger.L().Error(ctx, "failed to return network slot", zap.Error(returnErr)) | ||
| } | ||
| }(ctx) |
There was a problem hiding this comment.
Missing context.WithoutCancel causes network slot return failures
Medium Severity
The network slot cleanup goroutine passes ctx directly instead of context.WithoutCancel(ctx). During cleanup, the parent context is often already canceled. The old code in getNetworkSlotAsync (line 1036) explicitly used context.WithoutCancel(ctx) to ensure networkPool.Return completes even when the context is canceled. Without this protection, the return operation may fail with a context cancellation error, potentially leaking network slots from the pool.
3 participants
Note
Modernizes sandbox resume flow with Promise-based parallel initialization and introduces a shared Promise utility.
utils.Promiseto initializeuffd, network slot acquisition, rootfs overlay, and memory serving in parallel; start prefetch onceuffdis readySlotandoverlay, fetching metadata/rootfs as needed and waiting on promises beforeResumepackages/shared/pkg/utils/promise(generic) withWait/Done/Resultand comprehensive unit testsWritten by Cursor Bugbot for commit 7608780. This will update automatically on new commits. Configure here.