feat(sandbox): --tmpfs PATH:0 opts out of mounting, keeping the path on the root overlay (#1377) - #1392
Conversation
| let rows = microsandbox_db::entity::sandbox::Entity::find() | ||
| .all(pools.read()) | ||
| .await?; | ||
| let used: std::collections::HashSet<i64> = rows.iter().map(|row| row.id as i64).collect(); |
There was a problem hiding this comment.
Stopped rows exhaust live slots
When stopped persistent sandbox records accumulate, this unfiltered query continues treating their IDs as occupied, eventually causing new networked sandboxes to fail with “network address pool exhausted” even when few or no sandboxes are running.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/rust/lib/runtime/spawn.rs
Line: 2425-2428
Comment:
**Stopped rows exhaust live slots**
When stopped persistent sandbox records accumulate, this unfiltered query continues treating their IDs as occupied, eventually causing new networked sandboxes to fail with “network address pool exhausted” even when few or no sandboxes are running.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| /// collide with a concurrently live sandbox. | ||
| #[cfg(feature = "net")] |
There was a problem hiding this comment.
Single-caller helper is misplaced
recycle_network_slot is called only by spawn_sandbox but is defined among the main functions rather than later in Functions: Helpers, breaking the repository's required function organization and making helper discovery inconsistent.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/rust/lib/runtime/spawn.rs
Line: 2419-2420
Comment:
**Single-caller helper is misplaced**
`recycle_network_slot` is called only by `spawn_sandbox` but is defined among the main functions rather than later in `Functions: Helpers`, breaking the repository's required function organization and making helper discovery inconsistent.
**Context Used:** AGENTS.md ([source](https://github.com/superradcompany/microsandbox/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Addressed by the rewrite: the helper insertion that displaced the attribute is gone from this branch entirely (it was the #1391 commit); on that PR the attribute is back above sandbox_cli_args.
appcypher
left a comment
There was a problem hiding this comment.
thanks for tackling this. the underlying issue is real, but i don’t think --tmpfs /tmp:0 is the right interface. --tmpfs means "mount an in-memory filesystem", while :0 reads like a size, not "don’t mount anything". it also leaves a fake zero-sized tmpfs in the persisted config even though the runtime skips the mount.
how about --no-auto-tmpfs instead? it says exactly what we want: disable the automatic /tmp tmpfs so /tmp uses the root filesystem. explicit tmpfs mounts would still work normally.
| // sandbox process aborts on startup. Recycle from the live set instead. | ||
| #[cfg(feature = "net")] | ||
| { | ||
| launch.sandbox_slot = recycle_network_slot(local, sandbox_id).await?; |
There was a problem hiding this comment.
looks like this commit is already being reviewed separately in #1391 and #1392 was branched from that branch. please rebase #1392 onto main to drop the #1391 commit from this PR, or change #1392’s base to fix/network-slot-recycling-1390 if this is intentionally stacked. as opened against main, the diff mixes two unrelated issues.
There was a problem hiding this comment.
Fixed — rebased onto main so this diff contains only the tmpfs work (the slot commit belongs to #1391). The layering is also redone per your other comment; see the reply there.
| // #1377: an explicitly sized-0 tmpfs is the documented "do | ||
| // not mount" — the path stays on the writable root overlay | ||
| // instead of getting a useless 0-byte RAM disk. | ||
| if *size_mib == Some(0) { |
There was a problem hiding this comment.
also, i think this is being handled at the wrong layer. sandbox_cli_args should only translate actual mounts for the guest. the opt-out belongs in apply_runtime_defaults, where the automatic /tmp mount is added, backed by a persisted no_auto_tmpfs policy. that way the config and actual mount state stay consistent.
There was a problem hiding this comment.
Both points addressed, branch rewritten from main and force-pushed (ef99627):
- Rebased onto main — the fix(runtime): recycle network slots #1391 commit is gone from this diff; the PR now contains only the tmpfs work.
- Opt-out moved to the config layer, as you suggested —
SandboxRuntimeOptionsgains a persisteddisable_auto_tmpfspolicy (serde-defaulted sibling ofdisable_metrics_sample), settable from the builder viadisable_auto_tmpfs().apply_runtime_defaultsreturns before pushing the automatic mount when the policy is set, so config and actual mount state stay consistent. The CLI maps--tmpfs /tmp:0to the policy (adding no mount); a zero-sized tmpfs at any other path also adds no mount. The spawn-layer skip from the first version is gone entirely.
Tests now pin the policy semantics: disable_auto_tmpfs suppresses the automatic mount at the config layer; without it the default applies exactly as before; the CLI parses /tmp:0 as size zero.
… root overlay (superradcompany#1377) Every OCI-image sandbox gets an automatic RAM-backed tmpfs at /tmp, sized memory/4 capped at 512 MiB. The cap makes the formula inert above 2 GiB of RAM, the space is charged to guest memory rather than the root disk, and the one place builds stage their largest artifacts gets 512 MiB no matter how large --root-disk is. There was no opt-out: --tmpfs /tmp:0 suppressed the default but then mounted a useless 0-byte RAM disk. The opt-out lives where the default is applied, as a persisted policy: - SandboxRuntimeOptions gains disable_auto_tmpfs (serde-defaulted, sibling of disable_metrics_sample), settable from the builder via disable_auto_tmpfs(). - apply_runtime_defaults returns before pushing the automatic mount when the policy is set, so the config and the actual mount state stay consistent. - The CLI maps --tmpfs /tmp:0 to the policy (and adds no mount); a zero-sized tmpfs at any other path simply adds no mount. Also documents the automatic tmpfs, its sizing, and both tuning shapes in docs/sandboxes/volumes.mdx, and gives the three sizing constants doc comments. Tests: the policy suppresses the automatic mount at the config layer; without it the default is applied exactly as before; the CLI parses /tmp:0 as size zero for the policy mapping.
fd0ce16 to
ef99627
Compare
|
Reviews (2): Last reviewed commit: "feat(sandbox): persisted disable_auto_tm..." | Re-trigger Greptile |
| // --- Tmpfs --- | ||
| for tmpfs_str in &opts.tmpfs { | ||
| let (path, size, options) = parse_tmpfs(tmpfs_str)?; | ||
| if size == Some(0) { |
There was a problem hiding this comment.
thanks, moving the policy into apply_runtime_defaults fixes the layering issue, but the main cli concern is still unresolved. --tmpfs means mount a tmpfs, so :0 should not become a hidden opt-out or a silent no-op for other paths. please expose the persisted policy directly as --no-auto-tmpfs and keep --tmpfs limited to actual tmpfs mounts.
| // #1377: `--tmpfs /tmp:0` must not add a mount; the builder-level | ||
| // effect (disable_auto_tmpfs) is asserted through the resulting | ||
| // config when the flag application path runs. | ||
| let (path, size, _options) = parse_tmpfs("/tmp:0").unwrap(); |
There was a problem hiding this comment.
this test doesn’t exercise apply_sandbox_opts_inner or assert disable_auto_tmpfs; it only verifies that the parser returns zero, so its name and comment overstate the coverage. it also has duplicate #[test] attributes, while the following parser test has lost its #[test]. please fix the attributes and test the actual cli-to-policy mapping.
Closes #1377, implementing the issue's suggestion 2 (
--tmpfs /tmp:0= do not mount) together with suggestion 4 (documentation).The gap
Every OCI-image sandbox gets an automatic RAM-backed tmpfs at
/tmp, sizedmemory/4capped at 512 MiB:/tmp— where Go link steps, cargo, npm, pip and image builds stage their largest artifacts — gets 512 MiB no matter how large--root-diskis;--tmpfs /tmp:0suppressed the default but then mounted a useless 0-byte RAM disk.The fix
An explicitly sized-0 tmpfs is now the documented "do not mount": the bootstrap skips the mount entirely, so the path stays on the writable root overlay like
/varand/root. The suppression of the automatic default is unchanged (any explicit mount at the path already won), and a zero size previously had no working meaning — nothing that worked before changes.msb create alpine --name builder --root-disk 20G --tmpfs /tmp:0 # /tmp now lives on the 20 GiB overlayAlso:
docs/sandboxes/volumes.mdxgains a section on the automatic tmpfs, its sizing, and both tuning shapes (--tmpfs /tmp:8Gfor more RAM-backed room,:0for root-overlay); the three sizing constants get the doc comments their neighborDEFAULT_BIND_QUOTA_MIBalready had.Tests
Two additions to the
sandbox_cli_argssuite:MSB_TMPFSenv is absent);cargo fmtclean. (Same caveat as my other PR: this Windows env's MSVC linker cannot complete a localcargo check— third-party build scripts fail before reaching this crate — so the compile gate is CI; the changed arms mirror the existing match shapes exactly.)Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Create sandbox row] --> B{Database ID <= 65535?} B -- Yes --> C[Use database ID as network slot] B -- No --> D[Load all sandbox row IDs] D --> E[Choose lowest absent ID as slot] E --> F[Assign slot to launch config] F --> G[Derive sandbox network identity]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(sandbox): --tmpfs PATH:0 opts out o..." | Re-trigger Greptile
Context used: