[PR #108] [MERGED] Fix sandbox lifecycle bugs and improve network policy handling #183

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/108
Author: @LSX-s-Software
Created: 8/3/2026
Status: ✅ Merged
Merged: 8/3/2026
Merged by: @LSX-s-Software

Base: main ← Head: fix/sandbox-lifecycle

📝 Commits (4)

e025555 fix(sandbox): slot resource leak caused by a failure to set egress policy
6fee769 fix(orchestrator): handle artifact root allocation error
4a2589e fix(orchestrator): make network policy and custom extension param updates cancellation-safe
a28c065 feat(network): make egress policy update atomic

📊 Changes

7 files changed (+295 additions, -32 deletions)

View changed files

➕ src/image/cache/mock.rs (+54 -0)
📝 src/image/cache/mod.rs (+4 -0)
📝 src/orchestrator/service.rs (+66 -13)
📝 src/orchestrator/tests.rs (+108 -2)
📝 src/sandbox/firecracker/sandbox.rs (+5 -2)
📝 src/sandbox/network/iptables_util.rs (+0 -7)
📝 src/sandbox/network/policy.rs (+58 -8)

📄 Description

What

Fixes three sandbox lifecycle bugs (network-slot leak on egress-policy failure, unhandled artifact-root allocation error during pause, and cancellation-unsafe network/custom-extension updates) and makes egress policy replacement atomic via a single iptables-restore transaction.

Why

Several lifecycle paths left the sandbox in an inconsistent state on failure or cancellation:

A failed set_egress_policy call dropped the network slot before it was attached to self.network_slot, leaking the underlying network resources.
allocate_artifact_root failure during pause occurred after the handle/route were already detached, with no rollback of image refs or sandbox state.
replace_sandbox_network_policy and patch_sandbox_custom_extension_params were not wrapped in the cancellation-safe execution path used by other lifecycle operations (create/fork/delete/pause/resume/snapshot), so a client disconnecting mid-request could leave the update half-applied.
Egress policy updates flushed the USER_EGRESS_CHAIN and installed new rules as two separate iptables-restore invocations, leaving a brief window with inconsistent (or no) egress rules.

Closes #42
Closes #99

Scope and non-goals

Included: sandbox network-slot leak fix, pause artifact-root allocation error handling, cancellation-safety for network policy/custom extension param updates, atomic egress policy replacement.
Excluded: no serialization for concurrent network policy/custom extension param updates.

Design and behavior changes

FirecrackerSandbox: network_slot is now assigned to self.network_slot before set_egress_policy is invoked, so a failed call still leaves the slot reachable by the normal cleanup path instead of being dropped in place.
OrchestratorService::pause_sandbox_inner: allocate_artifact_root is now called before detaching the sandbox handle/route (while it does not mutate the backend). On allocation failure, image refs for RuntimeImageOwner::PausedSandbox are released and the state machine is rolled back from Pausing to Running.
run_lifecycle_operation renamed to run_cancellation_safe; replace_sandbox_network_policy and patch_sandbox_custom_extension_params are now split into public wrapper + _inner implementation and routed through run_cancellation_safe, so a caller cancelling the request no longer aborts the update mid-flight — the background task still runs to completion.
build_user_egress_commands now takes a replace: bool flag; when replacing, a FlushChain command is prepended to the same command batch sent to apply_iptables_commands, so flush + rule installation happen as a single atomic iptables-restore transaction. The standalone flush_table_chain helper was removed as it's no longer needed.

Compatibility and operations

Public API or generated protocol: N/A — no API surface changes.
Configuration or defaults: N/A — no config changes.
Snapshot manifest, artifact layout, or storage format: N/A — no format changes.
Upgrade and rollback: No special steps; purely internal lifecycle/network behavior fixes.
Host requirements, permissions, ports, or dependencies: N/A — no new host requirements.

Validation

make fmt
make clippy
make test-unit
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 test-agent # All passed

Skipped checks and reasons:

services/ unchanged, so make -C services test skipped.

Risks and reviewer notes

Concurrent network policy/custom extension param updates are deliberately NOT serialized so that the orchestrator is kept simple.
In-memory metadata update failures are deliberately IGNORED due to its rareness and the difficulty to implement a fully transactional architecture.

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.
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.

_{🔄 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/108 **Author:** [@LSX-s-Software](https://github.com/LSX-s-Software) **Created:** 8/3/2026 **Status:** ✅ Merged **Merged:** 8/3/2026 **Merged by:** [@LSX-s-Software](https://github.com/LSX-s-Software) **Base:** `main` ← **Head:** `fix/sandbox-lifecycle` --- ### 📝 Commits (4) - [`e025555`](https://github.com/kvcache-ai/AgentENV/commit/e025555254126dfab2e2b7bfb642655e0e0829a8) fix(sandbox): slot resource leak caused by a failure to set egress policy - [`6fee769`](https://github.com/kvcache-ai/AgentENV/commit/6fee7694fd4b65490d2788860dbec7344dfdbf47) fix(orchestrator): handle artifact root allocation error - [`4a2589e`](https://github.com/kvcache-ai/AgentENV/commit/4a2589e527d3a24f9e8f317c2d8a63b7fec40e49) fix(orchestrator): make network policy and custom extension param updates cancellation-safe - [`a28c065`](https://github.com/kvcache-ai/AgentENV/commit/a28c0658af7c6506bbd0c8b7a7dbf56d632a6c31) feat(network): make egress policy update atomic ### 📊 Changes **7 files changed** (+295 additions, -32 deletions) <details> <summary>View changed files</summary> ➕ `src/image/cache/mock.rs` (+54 -0) 📝 `src/image/cache/mod.rs` (+4 -0) 📝 `src/orchestrator/service.rs` (+66 -13) 📝 `src/orchestrator/tests.rs` (+108 -2) 📝 `src/sandbox/firecracker/sandbox.rs` (+5 -2) 📝 `src/sandbox/network/iptables_util.rs` (+0 -7) 📝 `src/sandbox/network/policy.rs` (+58 -8) </details> ### 📄 Description ## What Fixes three sandbox lifecycle bugs (network-slot leak on egress-policy failure, unhandled artifact-root allocation error during pause, and cancellation-unsafe network/custom-extension updates) and makes egress policy replacement atomic via a single `iptables-restore` transaction. ## Why Several lifecycle paths left the sandbox in an inconsistent state on failure or cancellation: - A failed `set_egress_policy` call dropped the network slot before it was attached to `self.network_slot`, leaking the underlying network resources. - `allocate_artifact_root` failure during pause occurred after the handle/route were already detached, with no rollback of image refs or sandbox state. - `replace_sandbox_network_policy` and `patch_sandbox_custom_extension_params` were not wrapped in the cancellation-safe execution path used by other lifecycle operations (create/fork/delete/pause/resume/snapshot), so a client disconnecting mid-request could leave the update half-applied. - Egress policy updates flushed the `USER_EGRESS_CHAIN` and installed new rules as two separate `iptables-restore` invocations, leaving a brief window with inconsistent (or no) egress rules. ## Related issue Closes #42 Closes #99 ## Scope and non-goals Included: sandbox network-slot leak fix, pause artifact-root allocation error handling, cancellation-safety for network policy/custom extension param updates, atomic egress policy replacement. Excluded: no serialization for concurrent network policy/custom extension param updates. ## Design and behavior changes - `FirecrackerSandbox`: `network_slot` is now assigned to `self.network_slot` **before** `set_egress_policy` is invoked, so a failed call still leaves the slot reachable by the normal cleanup path instead of being dropped in place. - `OrchestratorService::pause_sandbox_inner`: `allocate_artifact_root` is now called **before** detaching the sandbox handle/route (while it does not mutate the backend). On allocation failure, image refs for `RuntimeImageOwner::PausedSandbox` are released and the state machine is rolled back from `Pausing` to `Running`. - `run_lifecycle_operation` renamed to `run_cancellation_safe`; `replace_sandbox_network_policy` and `patch_sandbox_custom_extension_params` are now split into public wrapper + `_inner` implementation and routed through `run_cancellation_safe`, so a caller cancelling the request no longer aborts the update mid-flight — the background task still runs to completion. - `build_user_egress_commands` now takes a `replace: bool` flag; when replacing, a `FlushChain` command is prepended to the same command batch sent to `apply_iptables_commands`, so flush + rule installation happen as a single atomic `iptables-restore` transaction. The standalone `flush_table_chain` helper was removed as it's no longer needed. ## Compatibility and operations - Public API or generated protocol: N/A — no API surface changes. - Configuration or defaults: N/A — no config changes. - Snapshot manifest, artifact layout, or storage format: N/A — no format changes. - Upgrade and rollback: No special steps; purely internal lifecycle/network behavior fixes. - Host requirements, permissions, ports, or dependencies: N/A — no new host requirements. ## Validation - [x] `make fmt` - [x] `make clippy` - [x] `make test-unit` - [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: ```shell make test-agent # All passed ``` Skipped checks and reasons: - `services/` unchanged, so `make -C services test` skipped. ## Risks and reviewer notes - Concurrent network policy/custom extension param updates are deliberately NOT serialized so that the orchestrator is kept simple. - In-memory metadata update failures are deliberately IGNORED due to its rareness and the difficulty to implement a fully transactional architecture. ## 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. - [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. --- <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:45 +00:00