feat(orchestrator): add NetworkAssignHook for sandbox lifecycle extensions#3290
Merged
Conversation
Sandbox editions sometimes need bounded, local prep after a sandbox's identity/network slot are assigned but before its guest runs - a gap between the two existing extension points: EgressProxy fires before identity exists, MapSubscriber's OnInsert fires only after the guest is already running. Add StartHook (BeforeStart), called once per start attempt right after AssignNetwork in CreateSandbox/ResumeSandbox, tagged with a StartReason (create/resume/reboot/throwaway_resume) since these carry different guest-state guarantees. Bounded and non-fatal by construction: runs in its own goroutine behind a timeout enforced via select, not by trusting the implementation to honor ctx; a panic is recovered. Can never delay or fail a sandbox start. No-op everywhere by default; zero behavior change until an edition wires one in. Benchmarked on the no-op path: low single-digit µs/op, 576 B/op, 7 allocs/op - noise next to a sandbox create/resume (ms-s).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a StartHook interface and a StartReason enum to allow edition-specific hooks to run before a sandbox starts (during creation, resume, reboot, or throwaway resume). The hook is integrated into the sandbox Factory and executed asynchronously with a 2-second timeout. The review feedback highlights opportunities to improve logging and error handling: specifically, suppressing redundant context cancellation/timeout warnings in the hook execution, distinguishing between parent context cancellation and hook timeouts, and implementing the fmt.Stringer interface for StartReason to prevent raw numeric logging.
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.
❌ 5 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
- suppress redundant "hook failed" logging when BeforeStart returns context.Canceled/DeadlineExceeded - already logged by the timeout/cancellation branch - distinguish parent-context cancellation from startHookTimeout actually elapsing in that branch's log message - add StartReason.String() so logging it prints a name, not a bare uint8 Adds coverage for all three.
wj-e2b
reviewed
Jul 15, 2026
wj-e2b
reviewed
Jul 15, 2026
wj-e2b
reviewed
Jul 15, 2026
…imeout Renames StartHook/BeforeStart/StartReason to NetworkAssignHook/ OnNetworkAssign/NetworkAssignReason, matching the OnInsert/ OnNetworkRelease naming convention (per PR review feedback). Also drops infra's own timeout enforcement on the hook call: it's now invoked synchronously with the caller's own ctx, unmodified, and no derived deadline. The implementation is fully responsible for constraining its own duration; infra only recovers a panic here, since there's no other recovery layer before the process's main goroutine.
golangci-lint's fatcontext linter flags any context.Context value assigned to an outer-scoped variable from within a function literal, even when nothing is actually chained via context.WithValue. Assert the context identity inline instead of capturing it, same test intent.
rguliyev
changed the title
tvi
approved these changes
Jul 15, 2026
wj-e2b
approved these changes
Jul 15, 2026
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
3 participants
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.
Sandbox editions sometimes need bounded, local prep after a sandbox's identity/network slot are assigned but before its guest runs - a gap between the two existing extension points: EgressProxy fires before identity exists, MapSubscriber's OnInsert fires only after the guest is already running.
Add NetworkAssignHook (OnNetworkAssign), called once per start attempt right after AssignNetwork in CreateSandbox/ResumeSandbox, tagged with a NetworkAssignReason since these carry different guest-state guarantees. Called synchronously with the caller's own ctx and no derived timeout: the implementation is responsible for bounding its own duration. A panic is recovered, since there is no other recovery layer between this call and the process's main goroutine. Can never fail a sandbox start; a returned error is logged and swallowed.
No-op everywhere by default; zero behavior change until an edition wires one in.
Benchmarked on the no-op path: ~10ns/op, 0 B/op, 0 allocs/op - noise next to a sandbox create/resume (ms-s).