[PR #380] [MERGED] refactor(sandbox): give Firecracker sandbox resources a single owner and split start paths #390

New issue

Closed

opened 2026-10-29 14:22:54 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:22:54 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/380
Author: @yingdi-shan
Created: 10/13/2026
Status: ✅ Merged
Merged: 10/16/2026
Merged by: @yingdi-shan

Base: main ← Head: refactor/firecracker-sandbox-resources

📝 Commits (4)

89a10f4 refactor(sandbox): own Firecracker host resources in SandboxResources
0004991 refactor(sandbox): share Firecracker rootfs attach and boot-arg append
3f616ab refactor(sandbox): split Firecracker start_resume into stages
cc945b5 refactor(sandbox): move guest volume helpers out of FirecrackerSandbox

📊 Changes

5 files changed (+784 additions, -697 deletions)

View changed files

📝 src/sandbox/firecracker/mod.rs (+1 -0)
➕ src/sandbox/firecracker/resources.rs (+159 -0)
📝 src/sandbox/firecracker/sandbox.rs (+434 -697)
➕ src/sandbox/guest_volumes.rs (+189 -0)
📝 src/sandbox/mod.rs (+1 -0)

📄 Description

What

Restructure src/sandbox/firecracker/sandbox.rs, which has had the most churn and the most bug fixes in the repository, in four commits that each keep behavior the same:

SandboxResources (resources.rs): the ~10 acquired host resources (network slot, ublk devices, prefetch task, extension hook guard, …) move out of FirecrackerSandbox's separate Option fields into one owner. It has a single ordered release() used by stop() and a Drop for abnormal drops.
Shared helpers: attach_user_rootfs() replaces the two copies of the rootfs overlaybd device setup in start_fresh/start_resume, including the release-on-symlink-failure path. append_boot_arg() replaces four copies of the boot-arg append match.
Stage split: the ~380-line start_resume becomes a sequence of named stages (adopt_warm_firecracker, prepare_resume_drives, create_pack_recording_mem_device / acquire_shared_mem_device, attach_new_volume_drives). allocate_network_slot, spawn_firecracker and init_envd_instance are shared with start_fresh.
src/sandbox/guest_volumes.rs: the envd-driven guest mount, sync and fsfreeze helpers move out of the Firecracker sandbox impl, unchanged, into a module that doesn't depend on any backend.

Why

The release order and the "store the handle before the next ?" rule were enforced only by comments and by keeping two hand-written teardown paths in sync. ff822fe (#42), a network slot leaked when setting the egress policy failed, is an example of what that structure allows. With one owner, a handle stored in SandboxResources is always released, and the release order lives in one place.

None. This came out of a code-quality review of the highest-churn files.

Scope and non-goals

Only src/sandbox/firecracker/ changes, plus the new src/sandbox/guest_volumes.rs and its mod line. No public types are touched.
Not included: the long lifecycle functions in orchestrator/service.rs, which can come in a follow-up.

Design and behavior changes

The goal is no behavior change. Small, intentional differences:

Errors from the resume rootfs/network steps no longer have a "for resume" suffix, because the shared helpers use one message. The start_resume tracing span still identifies the path.
start_fresh applies the egress policy right after allocating the slot, before it reads the slot's boot arg and netns path (both are plain getters).

The stop() release order (rootfs → tools → dedicated mem → extra drives → stop hook → network → prefetch → shared mem) and the Drop behavior are unchanged. FirecrackerSandbox::drop releases resources explicitly before the work dir and Firecracker process fields drop, as it did before.

Compatibility and operations

Public API or generated protocol: N/A, internal refactor only.
Configuration or defaults: N/A.
Snapshot manifest, artifact layout, or storage format: N/A, the capture paths are untouched.
Upgrade and rollback: N/A.
Host requirements, permissions, ports, or dependencies: N/A.

Validation

make fmt
make clippy
make test-unit (the agentenv parts were run; see below)
Relevant Rust integration tests
make -C services test (required when services/ changes)
Generated clients/server regenerated with the documented make target
Documentation updated
Benchmarks or performance comparison completed

Commands and results:

make fmt                                              # ok
make clippy                                           # ok
cargo test -p agentenv --lib                          # 966 passed, 4 ignored
cargo test -p agentenv --lib -- --ignored             # 4 passed (capability runner)
cargo test -p agentenv --test integration \
  --test orchestrator_integration                     # 21 + 2 passed, 1 ignored (KVM + ublk, capability runner)
cargo test -p agentenv --test integration \
  posix_startup_pack -- --ignored                     # 1 passed, with [snapshot.memory_startup_pack] enabled
                                                      # (covers the pack-recording memory device path)

The firecracker unit tests (cargo test -p agentenv --lib firecracker::, 99 tests) and cargo clippy -p agentenv --lib --tests -D warnings passed after each commit.

Skipped checks and reasons:

snapshot_oss_e2e_test needs an object-storage service, and the change does not touch repository or publication code.
The parts of make test-unit outside agentenv (envd, linux-cap, aenv, ublk crates, install scripts) were not run; this PR does not touch them.
services/ is unchanged, so its tests were not run.

Risks and reviewer notes

The main risk is teardown ordering, so compare SandboxResources::release() with the old stop() body. Commit 1 is easiest to review on its own.
In commit 3, prepare_resume_drives now returns the (slot, drive) pairs for launch-time volumes that must be bound after snapshot load. That replaces the has_new_volume_attachments flag that was checked in two places.

Checklist

The PR contains one coherent change and no unrelated formatting or refactoring.
New behavior is covered by tests, or I explained why testing is impractical. (No new behavior; existing unit and VM integration tests cover the refactored paths.)
Logs and examples contain no credentials, tokens, or private registry information.
I did not manually edit generated code without updating its source and regenerating it.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/380 **Author:** [@yingdi-shan](https://github.com/yingdi-shan) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/16/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `refactor/firecracker-sandbox-resources` --- ### 📝 Commits (4) - [`89a10f4`](https://github.com/kvcache-ai/AgentENV/commit/89a10f4785dc93e857cf7f8f54c97670d792a3b3) refactor(sandbox): own Firecracker host resources in SandboxResources - [`0004991`](https://github.com/kvcache-ai/AgentENV/commit/0004991a32bba96cc8bdf0685b7783e33a6ac044) refactor(sandbox): share Firecracker rootfs attach and boot-arg append - [`3f616ab`](https://github.com/kvcache-ai/AgentENV/commit/3f616abc2725eaeeb8a65cd019e57fb6b936b480) refactor(sandbox): split Firecracker start_resume into stages - [`cc945b5`](https://github.com/kvcache-ai/AgentENV/commit/cc945b5ce9dbf31faff46d6dfc13661732336455) refactor(sandbox): move guest volume helpers out of FirecrackerSandbox ### 📊 Changes **5 files changed** (+784 additions, -697 deletions) <details> <summary>View changed files</summary> 📝 `src/sandbox/firecracker/mod.rs` (+1 -0) ➕ `src/sandbox/firecracker/resources.rs` (+159 -0) 📝 `src/sandbox/firecracker/sandbox.rs` (+434 -697) ➕ `src/sandbox/guest_volumes.rs` (+189 -0) 📝 `src/sandbox/mod.rs` (+1 -0) </details> ### 📄 Description ## What Restructure `src/sandbox/firecracker/sandbox.rs`, which has had the most churn and the most bug fixes in the repository, in four commits that each keep behavior the same: 1. **`SandboxResources`** (`resources.rs`): the ~10 acquired host resources (network slot, ublk devices, prefetch task, extension hook guard, …) move out of `FirecrackerSandbox`'s separate `Option` fields into one owner. It has a single ordered `release()` used by `stop()` and a `Drop` for abnormal drops. 2. **Shared helpers**: `attach_user_rootfs()` replaces the two copies of the rootfs overlaybd device setup in `start_fresh`/`start_resume`, including the release-on-symlink-failure path. `append_boot_arg()` replaces four copies of the boot-arg append `match`. 3. **Stage split**: the ~380-line `start_resume` becomes a sequence of named stages (`adopt_warm_firecracker`, `prepare_resume_drives`, `create_pack_recording_mem_device` / `acquire_shared_mem_device`, `attach_new_volume_drives`). `allocate_network_slot`, `spawn_firecracker` and `init_envd_instance` are shared with `start_fresh`. 4. **`src/sandbox/guest_volumes.rs`**: the envd-driven guest mount, sync and fsfreeze helpers move out of the Firecracker sandbox impl, unchanged, into a module that doesn't depend on any backend. ## Why The release order and the "store the handle before the next `?`" rule were enforced only by comments and by keeping two hand-written teardown paths in sync. ff822fe (#42), a network slot leaked when setting the egress policy failed, is an example of what that structure allows. With one owner, a handle stored in `SandboxResources` is always released, and the release order lives in one place. ## Related issue None. This came out of a code-quality review of the highest-churn files. ## Scope and non-goals - Only `src/sandbox/firecracker/` changes, plus the new `src/sandbox/guest_volumes.rs` and its `mod` line. No public types are touched. - Not included: the long lifecycle functions in `orchestrator/service.rs`, which can come in a follow-up. ## Design and behavior changes The goal is no behavior change. Small, intentional differences: - Errors from the resume rootfs/network steps no longer have a "for resume" suffix, because the shared helpers use one message. The `start_resume` tracing span still identifies the path. - `start_fresh` applies the egress policy right after allocating the slot, before it reads the slot's boot arg and netns path (both are plain getters). The `stop()` release order (rootfs → tools → dedicated mem → extra drives → stop hook → network → prefetch → shared mem) and the `Drop` behavior are unchanged. `FirecrackerSandbox::drop` releases resources explicitly before the work dir and Firecracker process fields drop, as it did before. ## Compatibility and operations - Public API or generated protocol: N/A, internal refactor only. - Configuration or defaults: N/A. - Snapshot manifest, artifact layout, or storage format: N/A, the capture paths are untouched. - Upgrade and rollback: N/A. - Host requirements, permissions, ports, or dependencies: N/A. ## Validation - [x] `make fmt` - [x] `make clippy` - [ ] `make test-unit` (the agentenv parts were run; see below) - [x] Relevant Rust integration tests - [ ] `make -C services test` (required when `services/` changes) - [ ] Generated clients/server regenerated with the documented `make` target - [ ] Documentation updated - [ ] Benchmarks or performance comparison completed Commands and results: ```text make fmt # ok make clippy # ok cargo test -p agentenv --lib # 966 passed, 4 ignored cargo test -p agentenv --lib -- --ignored # 4 passed (capability runner) cargo test -p agentenv --test integration \ --test orchestrator_integration # 21 + 2 passed, 1 ignored (KVM + ublk, capability runner) cargo test -p agentenv --test integration \ posix_startup_pack -- --ignored # 1 passed, with [snapshot.memory_startup_pack] enabled # (covers the pack-recording memory device path) ``` The firecracker unit tests (`cargo test -p agentenv --lib firecracker::`, 99 tests) and `cargo clippy -p agentenv --lib --tests -D warnings` passed after each commit. Skipped checks and reasons: - `snapshot_oss_e2e_test` needs an object-storage service, and the change does not touch repository or publication code. - The parts of `make test-unit` outside agentenv (envd, linux-cap, aenv, ublk crates, install scripts) were not run; this PR does not touch them. - `services/` is unchanged, so its tests were not run. ## Risks and reviewer notes - The main risk is teardown ordering, so compare `SandboxResources::release()` with the old `stop()` body. Commit 1 is easiest to review on its own. - In commit 3, `prepare_resume_drives` now returns the `(slot, drive)` pairs for launch-time volumes that must be bound *after* snapshot load. That replaces the `has_new_volume_attachments` flag that was checked in two places. ## Checklist - [x] The PR contains one coherent change and no unrelated formatting or refactoring. - [x] New behavior is covered by tests, or I explained why testing is impractical. (No new behavior; existing unit and VM integration tests cover the refactored paths.) - [x] Logs and examples contain no credentials, tokens, or private registry information. - [x] I did not manually edit generated code without updating its source and regenerating it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-29 14:22:54 +00:00