[PR #45] [CLOSED] fix(sandbox): roll back resources after start failures #137

New issue

Closed

opened 2026-10-29 14:20:05 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:20:05 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/45
Author: @morluto
Created: 7/28/2026
Status: ❌ Closed

Base: main ← Head: fix/start-failure-resource-rollback

📝 Commits (2)

d631e4d fix(sandbox): roll back failed direct starts
4ddd422 fix(sandbox): retain network slot on policy failure

📊 Changes

1 file changed (+70 additions, -5 deletions)

View changed files

📝 src/sandbox/firecracker/sandbox.rs (+70 -5)

📄 Description

Problem

Direct calls to FirecrackerSandbox::start() returned after startup or readiness errors without awaiting cleanup. Fresh startup also applied fallible egress-policy configuration before attaching the allocated network slot to the sandbox.

Together, these paths could retain daemon-managed ublk resources or leave a network slot unavailable after a failed start.

A. Public start transaction

start_nowait
  +-- error --> stop --> return original error
  `-- ok ----> wait_for_ready
                 +-- error --> stop --> return original error
                 `-- ok ----> state: Running

B. Network slot ordering

before: allocate -> set policy -> store ownership
                       |
                       `-- error -> manager slot remains allocated

after:  allocate -> store ownership -> set policy
                                      +-- error -> stop/Drop releases slot
                                      `-- ok ---> continue boot

Changes

Treat start_nowait() and wait_for_ready() as one public-start transaction.
Await stop() after either operation fails.
Preserve the primary startup error and warn if cleanup also fails.
Attach newly allocated network slots to the sandbox before applying fallible egress policy.

Existing orchestrator cleanup, Drop, and daemon shutdown behavior are unchanged.

Commit structure

fix(sandbox): roll back failed direct starts
fix(sandbox): retain network slot on policy failure

The commits are separate because public-start transactionality and network-slot ownership are independently reviewable and revertible.

Validation

The direct-start regression test failed against the previous implementation at the runtime-state ownership assertion.
cargo test -p agentenv --lib sandbox::firecracker::sandbox::tests:: — 14 passed.
cargo clippy -p agentenv --lib -- -D warnings
cargo fmt --all

The network ownership test uses the existing deterministic test-slot seam. A full base-failing start_fresh integration test would require privileged networking, ublk setup, and an additional injection seam, so this PR does not add production-wide mocking for that path.

Closes #42.

_{🔄 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/45 **Author:** [@morluto](https://github.com/morluto) **Created:** 7/28/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/start-failure-resource-rollback` --- ### 📝 Commits (2) - [`d631e4d`](https://github.com/kvcache-ai/AgentENV/commit/d631e4d4e20679aab428e3ea4ea0db4fdf9bf8e2) fix(sandbox): roll back failed direct starts - [`4ddd422`](https://github.com/kvcache-ai/AgentENV/commit/4ddd4226ba3f1dd5c1116255e07de15adde5d336) fix(sandbox): retain network slot on policy failure ### 📊 Changes **1 file changed** (+70 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `src/sandbox/firecracker/sandbox.rs` (+70 -5) </details> ### 📄 Description ## Problem Direct calls to `FirecrackerSandbox::start()` returned after startup or readiness errors without awaiting cleanup. Fresh startup also applied fallible egress-policy configuration before attaching the allocated network slot to the sandbox. Together, these paths could retain daemon-managed ublk resources or leave a network slot unavailable after a failed start. ```text A. Public start transaction start_nowait +-- error --> stop --> return original error `-- ok ----> wait_for_ready +-- error --> stop --> return original error `-- ok ----> state: Running B. Network slot ordering before: allocate -> set policy -> store ownership | `-- error -> manager slot remains allocated after: allocate -> store ownership -> set policy +-- error -> stop/Drop releases slot `-- ok ---> continue boot ``` ## Changes - Treat `start_nowait()` and `wait_for_ready()` as one public-start transaction. - Await `stop()` after either operation fails. - Preserve the primary startup error and warn if cleanup also fails. - Attach newly allocated network slots to the sandbox before applying fallible egress policy. Existing orchestrator cleanup, `Drop`, and daemon shutdown behavior are unchanged. ## Commit structure - `fix(sandbox): roll back failed direct starts` - `fix(sandbox): retain network slot on policy failure` The commits are separate because public-start transactionality and network-slot ownership are independently reviewable and revertible. ## Validation - The direct-start regression test failed against the previous implementation at the runtime-state ownership assertion. - `cargo test -p agentenv --lib sandbox::firecracker::sandbox::tests::` — 14 passed. - `cargo clippy -p agentenv --lib -- -D warnings` - `cargo fmt --all` The network ownership test uses the existing deterministic test-slot seam. A full base-failing `start_fresh` integration test would require privileged networking, ublk setup, and an additional injection seam, so this PR does not add production-wide mocking for that path. Closes #42. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-29 14:20:05 +00:00