fix(runtime): reject invalid box resource sizing at create boundary (P1-9)#662
fix(runtime): reject invalid box resource sizing at create boundary (P1-9)#662lilongen wants to merge 1 commit into
Conversation
cpus=0 (and undersized memory/disk) were silently accepted by create():
the box was persisted in Configured status and a box_id returned, but it
could never boot — libkrun set_vm_config(0, ...) returns EINVAL — so the
box was unusable and, under auto_remove, vanished on first start. Callers
saw create() "succeed", then get_info() return None and remove() 404
("Sandbox not found"), with no error surfaced by create().
Validate cpus>=1, memory_mib>=256, disk_size_gb>=1 in
BoxOptions::validate_resources(), called from create_inner() before the
box is persisted. This fails fast with InvalidArgument at every entry
point (REST -> HTTP 400, Python/Node/Go SDK, CLI) instead of handing back
a doomed box_id. Only explicitly-set (Some) fields are checked; None keeps
the engine default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
lile seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Rejects invalid resource sizing at create() time to prevent persisting unbootable boxes (e.g., cpus=0) and returning a “dangling” box_id.
Changes:
- Add
BoxOptions::validate_resources()with minimum thresholds for CPU/memory/disk. - Call
options.validate_resources()?during runtimecreate()to fail fast. - Add unit + async runtime tests covering invalid and boundary-valid sizing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/boxlite/src/runtime/rt_impl.rs | Enforces resource validation during create() and adds regression tests for P1-9 scenarios. |
| src/boxlite/src/runtime/options.rs | Introduces resource minimum constants + validate_resources() and unit tests for validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let err = zero_cpus.validate_resources().unwrap_err(); | ||
| assert!( | ||
| matches!(err, BoxliteError::InvalidArgument(ref m) if m.contains("cpus")), | ||
| "cpus=0 should be rejected as InvalidArgument: {err:?}" | ||
| ); |
| // Reject invalid resource sizing at the boundary (P1-9). Otherwise an | ||
| // unbootable box (e.g. cpus=0) is persisted, create() returns a | ||
| // box_id, and the box silently vanishes on first start under | ||
| // auto_remove — leaving the caller chasing a dangling id. | ||
| options.validate_resources()?; |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds resource validation to the box creation flow. ChangesResource validation on box creation
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| /// Minimum valid vCPU count. `cpus=0` is silently accepted by the type | ||
| /// (`u8`) but can never boot — libkrun `set_vm_config(0, ...)` returns | ||
| /// EINVAL. | ||
| pub const MIN_CPUS: u8 = 1; |
There was a problem hiding this comment.
it's not good to add the restriction here? every service has it's own custom restriction
DorianZheng
left a comment
There was a problem hiding this comment.
I don't think it's a good idea to add global restriction to the sdk
Problem (P1-9)
Creating a box with
cpus=0(and undersized memory/disk) was silently accepted.create()persisted the box inConfiguredstatus and returned abox_id— so the SDK reported success — but the box could never boot: libkrunkrun_set_vm_config(0, ...)returnsEINVAL (-22). Underauto_remove, the unbootable box then vanished on first start, leaving the caller with a dangling id:Root cause:
create_inner()performed no resource-sizing validation before persisting. The existingBoxOptions::sanitize()only checks option combinations (auto_remove/detach, isolate_mounts) and runs in the boot path, not at create.Fix
Validate resource sizing at the create boundary so every entry point (REST, Python/Node/Go SDK, CLI) fails fast instead of handing back a doomed
box_id:BoxOptions::validate_resources()+ constsMIN_CPUS=1,MIN_MEMORY_MIB=256,MIN_DISK_SIZE_GB=1. Only explicitly-set (Some) fields are checked;Nonekeeps the engine default.create_inner()before the box is persisted; returnsInvalidArgument→ REST maps to HTTP 400 via the existing error table.Lower-bound validation only (per scope decision); no upper bound added.
Testing
All run locally:
cargo test -p boxlite --no-default-features --lib): 2 reproducers throughruntime.create()(cpus=0 / memory=128 / disk=0 rejected, valid sizing still creates) + 2 focusedvalidate_resources()unit tests (reject + accept-at-minimum boundary).create_innercall line makes both reproducers FAIL (pointing at the bug); restoring makes them PASS.boxlite serve+ curl):POST /v1/boxes {"cpus":0}→ HTTP 400invalid argument: cpus must be >= 1, got 0(fixed) vs HTTP 201 + laterkrun_set_vm_config EINVAL(-22)(unfixed).rt.create(BoxOptions(cpus=0))now raises; valid sizing still creates.make test:unit:python: 114 passed, 0 failed.cargo fmt -p boxlite -- --checkandcargo clippy -p boxlite --no-default-features --libclean.Note
The exact
get_info()=None/remove()404 symptom requiresauto_remove=true(cloud/SDK default), where the failed box is reaped. On local REST (auto_remove=false) the box is preserved asConfigured/Failed. Both share the same root cause — this fix cuts it off at create for both paths.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests