Skip to content

Fix Apple Container sandbox resource limits - #1215

Merged
penso merged 4 commits into
mainfrom
almond-football
Aug 19, 2026
Merged

Fix Apple Container sandbox resource limits#1215
penso merged 4 commits into
mainfrom
almond-football

Conversation

@penso

@penso penso commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • pass configured sandbox memory and whole-CPU limits to Apple Container using --memory and --cpus
  • apply pids_max with Apple Container's native --ulimit nproc=<limit> syntax
  • reject fractional Apple Container CPU quotas explicitly instead of silently weakening the requested limit
  • document Apple Container version and CPU-granularity constraints

Closes #1188

Validation

Completed

  • cargo test -p moltis-tools apple_container_run_args
  • cargo fmt --all -- --check
  • just lint
  • container run --rm --cpus 2 --memory 512M ubuntu:25.10 true

Remaining

  • ./scripts/local-validate.sh <PR_NUMBER>
  • Live pids_max verification with Apple Container 0.9+; the local CLI is 0.8.0 and predates --ulimit

Manual QA

  1. Install Apple Container 0.9 or newer and select backend = "apple-container".
  2. Configure memory_limit = "1G", cpu_quota = 2.0, and pids_max = 512 under [tools.exec.sandbox.resource_limits].
  3. Restart Moltis and run ulimit -u plus a command that forks child processes inside the sandbox.
  4. Confirm the process limit is 512 and the command completes without Resource temporarily unavailable.
  5. Set cpu_quota = 0.5 and confirm Moltis reports that Apple Container requires a positive whole CPU count.

penso added 2 commits August 19, 2026 00:07
Apple Container sandbox creation ignored configured memory, CPU, and process limits, leaving workloads subject to backend defaults and causing fork failures under low PID ceilings. Pass memory and whole-CPU allocations through the native run flags and map pids_max to Apple Container's nproc ulimit syntax. Reject fractional CPU quotas explicitly because Apple Container cannot represent them without weakening the requested limit.
@greptile-apps

greptile-apps Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR now passes configured memory, whole-CPU, and process limits to Apple Container and validates unsupported CPU quotas before any container reuse.

  • Tracks managed-files and resource-limit policy fingerprints so changed limits trigger container recreation.
  • Records fingerprints for generated container names after successful creation or recovery.
  • Adds resource-limit argument and validation tests and documents Apple Container compatibility constraints.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
crates/tools/src/sandbox/apple.rs Applies resource-limit validation before reuse and correctly records policy fingerprints on all successful generated-name paths, resolving both prior findings.
crates/tools/src/sandbox/containers.rs Adds Apple Container memory, whole-CPU, and nproc arguments with explicit CPU validation.
crates/tools/src/sandbox/tests/apple.rs Covers generated-name policy tracking, resource-limit argument construction, and fractional CPU rejection.
docs/src/sandbox.md Documents Apple Container version requirements and whole-CPU granularity.

Reviews (3): Last reviewed commit: "fix(sandbox): retain policy after name r..." | Re-trigger Greptile

Comment thread crates/tools/src/sandbox/apple.rs
Apple Container reuse could return before validating the configured limits or confirming that the running VM was created with the current policy. Validate resource settings before inspection and associate each reusable container with a fingerprint covering mount and resource policy so changed limits force recreation.
@penso

penso commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

@greptile review

Comment thread crates/tools/src/sandbox/apple.rs Outdated
@codspeed-hq

codspeed-hq Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will degrade performance by 10.97%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 1 regressed benchmark
✅ 38 untouched benchmarks
⏩ 9 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
session_history_path[default] 6 µs 6.7 µs -10.97%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing almond-football (2cb77aa) with main (af26d7e)

Open in CodSpeed

Footnotes

  1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Record the resource-policy fingerprint only after the current Apple container name is ready. This keeps AlreadyExists generation rotation associated with the successfully created container and avoids destructive recreation on the next readiness check.
@penso

penso commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator Author

@greptile review

@penso
penso merged commit 77c89fe into main Aug 19, 2026
35 of 43 checks passed
@penso
penso deleted the almond-football branch August 19, 2026 04:34
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: resource limits not applied for apple-container backend

1 participant