feat: configurable Modal sandbox CPU and memory#693
Merged
ColeMurray merged 10 commits intoJun 9, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional sandbox resource reservation fields ChangesSandbox Resource Reservations
Sequence Diagram(s)sequenceDiagram
participant User
participant SandboxSettingsComponent
participant APIEndpoint
participant SettingsStore
User->>SandboxSettingsComponent: Edit CPU cores or memory
SandboxSettingsComponent->>SandboxSettingsComponent: Update local state (resolvedCpuCores/resolvedMemoryMib)
SandboxSettingsComponent->>SandboxSettingsComponent: Enable Save button when values differ
User->>SandboxSettingsComponent: Click Save
SandboxSettingsComponent->>SandboxSettingsComponent: Validate cpuCores positive and memoryMib positive integer
alt Validation fails
SandboxSettingsComponent->>User: Show error message
else Validation passes
SandboxSettingsComponent->>APIEndpoint: PUT with conditional cpuCores/memoryMib payload
APIEndpoint->>SettingsStore: Persist resource settings
APIEndpoint-->>SandboxSettingsComponent: Success response
SandboxSettingsComponent->>SandboxSettingsComponent: Reset cpu/memory edit state
SandboxSettingsComponent->>User: Show success feedback
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/control-plane/test/integration/integration-settings.test.ts (1)
578-602: ⚡ Quick winConsider adding a positive test case for configured resource reservations.
The test correctly validates that unset resource reservations resolve to
null. However, it would strengthen coverage to also test the case wherecpuCoresandmemoryMibare explicitly configured (e.g., via global or repo settings) and verify they're correctly returned in the resolved config response.Other integration tests in this file follow this pattern—testing both unconfigured defaults and configured overrides (see lines 200-242 for the GitHub settings example).
📋 Example test case
it("returns configured resource reservations in resolved config", async () => { const headers = await authHeaders(); // Configure global resource settings await SELF.fetch("https://test.local/integration-settings/sandbox", { method: "PUT", headers, body: JSON.stringify({ settings: { defaults: { cpuCores: 2, memoryMib: 4096 }, }, }), }); const res = await SELF.fetch( "https://test.local/integration-settings/sandbox/resolved/acme/widgets", { headers } ); expect(res.status).toBe(200); const body = await res.json<{ config: { cpuCores: number | null; memoryMib: number | null; }; }>(); expect(body.config.cpuCores).toBe(2); expect(body.config.memoryMib).toBe(4096); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/control-plane/test/integration/integration-settings.test.ts` around lines 578 - 602, Add a positive test that configures resource reservations and verifies they appear in the resolved config: use authHeaders() and SELF.fetch to PUT a settings payload to "https://test.local/integration-settings/sandbox" (set defaults.cpuCores and defaults.memoryMib), then call SELF.fetch on the resolved endpoint used in the existing test (e.g., "https://test.local/integration-settings/sandbox/resolved/testowner/testrepo"), assert 200, parse JSON and assert body.config.cpuCores and body.config.memoryMib match the configured values (similar to the existing assertions around DEFAULT_MAX_CONCURRENT_CHILD_SESSIONS / DEFAULT_MAX_TOTAL_CHILD_SESSIONS).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/modal-infra/src/sandbox/manager.py`:
- Around line 42-63: The _resource_kwargs function currently accepts cpu_cores
without rejecting NaN/Infinity; update the cpu_cores validation in
_resource_kwargs to also require a finite value (use
math.isfinite(float(cpu_cores))) before setting kwargs["cpu"], and import math
at the top of the module; keep the existing checks for numeric types and
positivity (i.e., ensure isinstance(cpu_cores, (int, float)), not
isinstance(cpu_cores, bool), math.isfinite(...), and cpu_cores > 0).
---
Nitpick comments:
In `@packages/control-plane/test/integration/integration-settings.test.ts`:
- Around line 578-602: Add a positive test that configures resource reservations
and verifies they appear in the resolved config: use authHeaders() and
SELF.fetch to PUT a settings payload to
"https://test.local/integration-settings/sandbox" (set defaults.cpuCores and
defaults.memoryMib), then call SELF.fetch on the resolved endpoint used in the
existing test (e.g.,
"https://test.local/integration-settings/sandbox/resolved/testowner/testrepo"),
assert 200, parse JSON and assert body.config.cpuCores and body.config.memoryMib
match the configured values (similar to the existing assertions around
DEFAULT_MAX_CONCURRENT_CHILD_SESSIONS / DEFAULT_MAX_TOTAL_CHILD_SESSIONS).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 429844ff-2f27-4e1c-96f0-c88a935bc20a
📒 Files selected for processing (11)
packages/control-plane/src/db/integration-settings.test.tspackages/control-plane/src/db/integration-settings.tspackages/control-plane/src/routes/integration-settings.tspackages/control-plane/src/sandbox/lifecycle/manager.test.tspackages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/test/integration/integration-settings.test.tspackages/modal-infra/src/sandbox/manager.pypackages/modal-infra/tests/test_sandbox_resources.pypackages/shared/src/types/integrations.tspackages/web/src/components/settings/sandbox-settings.test.tsxpackages/web/src/components/settings/sandbox-settings.tsx
ColeMurray
reviewed
Jun 1, 2026
ColeMurray
reviewed
Jun 1, 2026
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/control-plane/src/sandbox/settings.ts`:
- Around line 127-143: The current loop always assigns result.tunnelPorts =
ports.slice(0, MAX_TUNNEL_PORTS) even when the input array yields no valid
ports, causing malformed overrides like ["bad"] to become an explicit empty
array and override defaults; change the logic to only assign result.tunnelPorts
when there is at least one validated port (ports.length > 0) or when you
explicitly intend to allow empty overrides—i.e., after the validation and
MAX_TUNNEL_PORTS check, wrap the assignment in a conditional that checks
ports.length > 0 (and keep using ports.slice(0, MAX_TUNNEL_PORTS)) so
invalid-only inputs are omitted instead of normalized to [].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76d04a98-c4f3-4887-bb30-8122bc6b402e
📒 Files selected for processing (11)
packages/control-plane/src/db/integration-settings.test.tspackages/control-plane/src/db/integration-settings.tspackages/control-plane/src/sandbox/lifecycle/manager.tspackages/control-plane/src/sandbox/settings.test.tspackages/control-plane/src/sandbox/settings.tspackages/control-plane/test/integration/integration-settings.test.tspackages/modal-infra/src/sandbox/manager.pypackages/modal-infra/tests/test_sandbox_resources.pypackages/shared/src/types/integrations.tspackages/web/src/components/settings/sandbox-settings.test.tsxpackages/web/src/components/settings/sandbox-settings.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/types/integrations.ts
- packages/web/src/components/settings/sandbox-settings.tsx
- packages/control-plane/src/db/integration-settings.test.ts
Contributor
Author
|
I addressed your feedback @ColeMurray, please take a look again 🙏 |
Contributor
Author
|
checking back in @ColeMurray -- anythng missing from this PR? |
ColeMurray
approved these changes
Jun 9, 2026
ColeMurray
added a commit
that referenced
this pull request
Jun 9, 2026
…#720) ## What Follow-up cleanup to #693 (configurable Modal sandbox CPU/memory). Two behavior-preserving changes: - **web (`sandbox-settings.tsx`):** the cpu/memory override logic was implemented with a heavier, parallel mechanism than the adjacent child-session-limit fields — four helpers including a 7-argument `applyResourcePayload` and a 3-field intermediate object. Collapsed to two small helpers (`resourceDisplayValue`, `resourcePayloadValue`); the inherit / explicit-override / explicit-`null` (provider-default) tri-state is unchanged. - **control-plane (`integration-settings.test.ts`):** added a regression test pinning the `getResolvedConfig` merge-time `normalizeSandboxSettings` pass. ## Why The resource fields and the child-session-limit fields solve the same inheritance-guard problem; #693 introduced a second, heavier pattern for it. This narrows them and removes the bespoke indirection (net **−47 lines**). The regression test closes a gap: the merge-time normalize in `getResolvedConfig` is the **only** enforcement of `maxConcurrent <= maxTotal` when the global defaults and the repo override are each individually valid but the *merged* result is not. It had no committed test, so a future refactor could silently drop it. The test is mutation-verified (it fails if that normalize pass is removed). ## Behavior No behavior change. The `prior !== undefined` reasoning relies on stored JSON values never being `undefined`, so an inherited-only field is skipped and an explicit `null` override is preserved — exactly as before. Modal's `_resource_kwargs` mapping and every normalization pass are untouched. ## Testing - web: 25/25 `sandbox-settings` tests pass unchanged - control-plane: `integration-settings` unit suite passes, +1 new regression test (mutation-verified) - typecheck clean (against real shared types), eslint clean <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added a sandbox integration test that verifies config normalization handles cross-field validation after merging defaults and overrides. * Ensures invalid inverted concurrent/total limit cases are dropped by the merged configuration. * **Refactor** * Improved resource override/inheritance handling for CPU and memory in sandbox settings. * Updated editor logic to correctly display values and persist payloads while preserving prior override semantics. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
2 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.
What
Adds two optional fields to
SandboxSettings—cpuCoresandmemoryMib— letting an operator reserve CPU and memory per sandbox. They map to Modal'scpu/memorykwargs onSandbox.create.Why
Sandboxes previously ran at Modal's ~0.125-core floor with bursting, with no way to guarantee resources for heavier install/build/test workloads. This is particularly important when trying to build/test complex applications in compiled languages such as Java in the sandbox.
Behavior & design decisions
max(reservation, actual usage), so a default would have raised the billed floor for every sandbox; I intentionally avoided that.cpuCoresmust be a positive number (fractional allowed);memoryMiba positive integer. I do not impose arbitrary min/max ranges — Modal is the source of truth for real limits and rejects anything it can't honor at create time.What About Daytona
I havent used Daytona before, so i interrogated Claude and there is an eventual path to have Daytona respect cpu/memory config via hot-resize. The only wrinkle is that Daytona ties resources to the snapshot, so the cpu/memory config would be applied with a POST /sandbox/:id/resize AFTER create rather than at create time. CPU/Memory increases in Daytona are "hot" -- they work on a running sandbox with no stop.
How settings flow
You set CPU/memory the same way as any other sandbox setting — globally or per-repo through the integration-settings API, which stores them in D1.
When a session starts, we resolve the effective settings (repo overrides on top of global defaults) and save the whole object on the session row as-is. The values aren't touched again until the sandbox actually spins up: at that point we re-validate them — the stored blob can come from older or untrusted writes, so the spawn path doesn't trust it — and pass anything still valid to Modal as
cpu/memory.Snapshot restores take the same path, so a resumed session gets the same reservation as a fresh one.
Changes by layer
shared/.../integrations.tscpuCores/memoryMibonSandboxSettings, doc'd as advisorycontrol-plane/.../db/integration-settings.tscontrol-plane/.../routes/integration-settings.tscontrol-plane/.../lifecycle/manager.tsparseSandboxSettingsforwards positive valuesmodal-infra/.../manager.py_resource_kwargs→ Modal kwargs at both create sitesweb/.../sandbox-settings.tsxInheritance guard
Per-repo saves only persist a resource value when it's global, explicitly edited, or already an override — so saving unrelated repo settings no longer pins an inherited global default as a repo override (mirrors the existing child-session-limit behavior). Covered by two regression tests.
Testing
(resolved-config defaults) — pass
_resource_kwargsmapping + create/restore forwarding) — passNotes / out of scope
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests