Validate requested GPU types in Docker runtime - #2545
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2843527193
ℹ️ 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".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR changes an existing Docker startup path by validating typed GPUs after container creation and adjusting startup teardown ordering. An unresolved review concern identifies a possible leaked container when GPU verification fails, so the cleanup behavior should receive human confirmation. You can add or adjust custom eligibility rules. Learn more. |
Typed GPU requests such as
H100:2keep Docker's--gpus 2allocation and check the exposed devices before task setup. Startup fails with a clear error when the GPU type or count does not match, or when the device query cannot run.The check uses one
nvidia-smiinvocation inside the started container, so it inspects devices exposed by the selected Docker daemon. Product names match case-insensitively at whitespace and hyphen boundaries. Count-only and no-GPU requests retain their existing behavior.Separately provisioned MCP server runtimes register teardown before startup, so partially started containers are removed when startup fails or is cancelled.
Note
Medium Risk
GPU-backed sandboxes can fail at startup when the image lacks
nvidia-smior NVIDIA product names do not match the configured type string, though mis-provisioned GPUs are caught earlier with clearer errors.Overview
Docker runtime now verifies typed GPU configs (e.g.
H100:2) afterdocker runsucceeds, instead of only passing--gpuswith the parsed count.When both a GPU type and count come from
parse_gpu, the runtime runsnvidia-smiinside the new container and fails startup withSandboxErrorif the query fails, the number of exposed devices differs from the count, or any device name does not match the requested type (case-insensitive match at whitespace/hyphen boundaries viare). Count-only specs (2) and no-GPU configs are unchanged—no type check runs without a type.The
DockerConfig.gpufield docstring is updated to describe thetype[:count]format and this validation behavior.Reviewed by Cursor Bugbot for commit 2843527. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Validate requested GPU types in
DockerRuntime.startand fix cleanup ordering inmcp._serveDockerRuntime.startnow runsnvidia-smiafter container startup for typed GPU requests and rejects startup if the query fails, the exposed GPU count differs, or device names do not match the requested type (case-insensitive, whitespace/hyphen boundaries). Count-only GPU requests skip this check.DockerConfigGPU field docs to describe type-with-count and count-only forms plus thenvidia-smiverification requirement.mcp._serveregisters the runtime stop callback before awaiting startup for separately provisioned runtimes, soruntime.stop()runs during cleanup ifruntime.start()raises.SandboxError; existing configs with correct types are unaffected.Macroscope summarized a091797.