-
Notifications
You must be signed in to change notification settings - Fork 16
sandbox: dynamic directory mount & snapshot in go + adds detach #246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
PR SummaryMedium Risk Overview Introduces experimental dynamic directory mount & snapshot for Sandboxes via a new Written by Cursor Bugbot for commit 4c46034. This will update automatically on new commits. Configure here. |
| "google.golang.org/protobuf/types/known/emptypb" | ||
| ) | ||
|
|
||
| // tlsCredsNoALPN is a TLS credential that skips ALPN enforcement, implementing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole thing should no longer be needing after @saltzm fixed something server side, so might work if you just remove it and use the default?
|
Updated PR with:
|
|
I updated PR:
While implementing this, I made some decisions that are debatable:
|
saltzm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good mod the comment about task_command_router_insecure which isn't being used anymore in the python client
modal-go/sandbox.go
Outdated
| func (sb *Sandbox) ensureAttached() error { | ||
| if sb.detached.Load() { | ||
| return SandboxDetached{Exception: "Do not call Detach or Terminate until you are done with your sandbox in this session"} | ||
| return SandboxDetached{Exception: "Do not call Detach until you are done with your sandbox in this session"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I might find this error message confusing if I do like sb.Exec() and get this back - I'll be like "why is it telling me not to call Detach, I'm calling Exec".
I'd expect something more like "Unable to perform operation on a detached sandbox" or "Operations on sandbox after Detach are not allowed" or some such thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| fmt.Printf("Started Sandbox: %s\n", sb.SandboxID) | ||
| defer func() { | ||
| if err := sb.Terminate(context.Background()); err != nil { | ||
| if err := sb.Terminate(context.Background(), true, nil); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deferred terminate fails after manual detach in examples
Medium Severity
Deferred Terminate calls fail because the target sandbox is already detached. This occurs when a sandbox is explicitly detached or terminated with detach=true before the deferred call executes, leading to a fatal log.
Additional Locations (1)
| codes.Canceled: {}, | ||
| codes.Internal: {}, | ||
| codes.Unknown: {}, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated retryable gRPC status codes map
Low Severity
commandRouterRetryableCodes is identical to retryableGrpcStatusCodes defined in client.go (lines 285-291). Both maps contain the exact same five gRPC status codes: DeadlineExceeded, Unavailable, Canceled, Internal, and Unknown. The existing retryableGrpcStatusCodes variable could be reused instead of defining a duplicate.
| type RetryableClient interface { | ||
| authContext(ctx context.Context) context.Context | ||
| refreshJwt(ctx context.Context) error | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exported interface only used internally
Low Severity
RetryableClient is exported (capitalized) but its methods authContext and refreshJwt are unexported (lowercase). This makes the interface unusable outside the package since external code cannot implement unexported methods. The interface is only used by the unexported callWithAuthRetry function, so it should be unexported as retryableClient.
|
|
||
| // SandboxTerminateParams are options for Terminate | ||
| type SandboxTerminateParams struct { | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR cherry picks files from the following PRs to just implement the dynamic directory mounts for Go:
This includes the task command router. I think this is a smaller change and does not impact any public API around
sb.exec. Once we have this in, then #242 is easier to review.