[PR #110] [CLOSED] perf(network): skip empty egress programming on clean slots #186

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/110
Author: @cleverhu
Created: 8/3/2026
Status: ❌ Closed

Base: main ← Head: perf/skip-empty-egress

📝 Commits (1)

a3f898b perf(network): skip empty egress programming on clean slots

📊 Changes

1 file changed (+43 additions, -2 deletions)

View changed files

📝 src/sandbox/network/slot.rs (+43 -2)

📄 Description

What

Skip the egress-policy apply when the requested policy is empty and the namespace's user egress chain is already empty. Slots track whether their chain currently holds rules so recycled slots are still cleared.

Why

set_egress_policy unconditionally spawns a thread, enters the sandbox network namespace, and runs iptables-restore. When a sandbox is started with no egress policy — the common case — that entire sequence programs an empty policy into an already empty chain: a no-op that still pays for a process spawn and a netns switch.

On a sandbox start that otherwise hits every warm pool (network slot, block device, Firecracker), this is the single most expensive remaining step. Removing it takes the whole call to a couple of atomic loads.

None. The change is local to one function and comes with unit tests.

Scope and non-goals

Included: the fast path plus the state tracking that makes it safe.

Excluded:

Any change to what rules a non-empty policy programs.
The slow path itself; when rules must be applied or cleared, the existing code runs unchanged.
Broader warm-pool or sandbox-start restructuring.

Design and behavior changes

Slot gains user_egress_rules_present: AtomicBool. A freshly created namespace starts with an empty chain, so it initializes to false.

set_egress_policy returns early only when both the requested policy has no runtime egress rules and the flag is false. Otherwise it takes the existing path and then stores result.is_err() || wants_rules.

The is_err() term is deliberate: a failed apply may have flushed the chain and stopped partway, so the slot is assumed to hold rules. That is the conservative direction — the next tenant does a redundant clear rather than inheriting the previous tenant's rules.

This matters because slots are recycled through the warm pool without their rules being cleared at release time. A recycled slot that held rules still gets cleared by the next tenant's empty policy.

Compatibility and operations

Public API or generated protocol: N/A.
Configuration or defaults: N/A.
Snapshot manifest, artifact layout, or storage format: N/A.
Upgrade and rollback: transparent. The flag is in-memory per slot and rebuilt from false on restart, which matches a fresh namespace.
Host requirements, permissions, ports, or dependencies: unchanged.

Validation

make fmt
make clippy
make test-unit (targeted; 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:

$ cargo fmt --all -- --check
(clean)

$ cargo clippy --workspace --all-targets -- -D warnings
Finished `dev` profile

$ cargo test -p agentenv --lib egress_policy
running 2 tests
test sandbox::network::slot::tests::empty_egress_policy_on_a_clean_slot_is_a_no_op ... ok
test sandbox::network::slot::tests::empty_egress_policy_still_clears_rules_a_previous_tenant_left ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 675 filtered out

Two tests are added:

empty_egress_policy_on_a_clean_slot_is_a_no_op — no namespace is ever created for the slot, so any path other than the fast one would fail entering it. Passing proves the fast path was taken.
empty_egress_policy_still_clears_rules_a_previous_tenant_left — with the flag forced to true, the call must be attempted (and therefore fail, since there is no namespace) rather than skipped.

Skipped checks and reasons:

Full make test-unit not run; only the two relevant tests plus a full --all-targets build. The remaining phases need capabilities and a provisioned host.
No before/after timing is included. The justification here is structural — one fewer process spawn and netns switch per start — not measured. I did not have a cleanly provisioned node to produce trustworthy numbers, and I would rather not attach numbers from a noisy host. scripts/bench.py create on a warmed pool is the way to quantify it.

Risks and reviewer notes

The correctness question is the state machine on user_egress_rules_present, so please review that closely:

Initial false is correct only if a new namespace really starts with an empty user egress chain.
result.is_err() || wants_rules errs toward assuming rules exist. The cost of being wrong in that direction is a redundant clear; the cost in the other direction would be a leaked policy across tenants, which is a security issue.
Slots recycled through the warm pool are covered by the flag persisting on the Slot, not being reset on release.

The security-relevant scenario is a slot that held a restrictive policy being reused by a tenant with no policy. That is exactly the second test.

Most important file: src/sandbox/network/slot.rs.

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/110 **Author:** [@cleverhu](https://github.com/cleverhu) **Created:** 8/3/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `perf/skip-empty-egress` --- ### 📝 Commits (1) - [`a3f898b`](https://github.com/kvcache-ai/AgentENV/commit/a3f898b8f2299bd75ac94c2c9bcefde919677693) perf(network): skip empty egress programming on clean slots ### 📊 Changes **1 file changed** (+43 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/sandbox/network/slot.rs` (+43 -2) </details> ### 📄 Description ## What Skip the egress-policy apply when the requested policy is empty and the namespace's user egress chain is already empty. Slots track whether their chain currently holds rules so recycled slots are still cleared. ## Why `set_egress_policy` unconditionally spawns a thread, enters the sandbox network namespace, and runs `iptables-restore`. When a sandbox is started with no egress policy — the common case — that entire sequence programs an empty policy into an already empty chain: a no-op that still pays for a process spawn and a netns switch. On a sandbox start that otherwise hits every warm pool (network slot, block device, Firecracker), this is the single most expensive remaining step. Removing it takes the whole call to a couple of atomic loads. ## Related issue None. The change is local to one function and comes with unit tests. ## Scope and non-goals Included: the fast path plus the state tracking that makes it safe. Excluded: - Any change to what rules a non-empty policy programs. - The slow path itself; when rules must be applied or cleared, the existing code runs unchanged. - Broader warm-pool or sandbox-start restructuring. ## Design and behavior changes `Slot` gains `user_egress_rules_present: AtomicBool`. A freshly created namespace starts with an empty chain, so it initializes to `false`. `set_egress_policy` returns early only when both the requested policy has no runtime egress rules *and* the flag is `false`. Otherwise it takes the existing path and then stores `result.is_err() || wants_rules`. The `is_err()` term is deliberate: a failed apply may have flushed the chain and stopped partway, so the slot is assumed to hold rules. That is the conservative direction — the next tenant does a redundant clear rather than inheriting the previous tenant's rules. This matters because slots are recycled through the warm pool without their rules being cleared at release time. A recycled slot that held rules still gets cleared by the next tenant's empty policy. ## Compatibility and operations - Public API or generated protocol: N/A. - Configuration or defaults: N/A. - Snapshot manifest, artifact layout, or storage format: N/A. - Upgrade and rollback: transparent. The flag is in-memory per slot and rebuilt from `false` on restart, which matches a fresh namespace. - Host requirements, permissions, ports, or dependencies: unchanged. ## Validation - [x] `make fmt` - [x] `make clippy` - [x] `make test-unit` (targeted; 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: ```text $ cargo fmt --all -- --check (clean) $ cargo clippy --workspace --all-targets -- -D warnings Finished `dev` profile $ cargo test -p agentenv --lib egress_policy running 2 tests test sandbox::network::slot::tests::empty_egress_policy_on_a_clean_slot_is_a_no_op ... ok test sandbox::network::slot::tests::empty_egress_policy_still_clears_rules_a_previous_tenant_left ... ok test result: ok. 2 passed; 0 failed; 0 ignored; 675 filtered out ``` Two tests are added: - `empty_egress_policy_on_a_clean_slot_is_a_no_op` — no namespace is ever created for the slot, so any path other than the fast one would fail entering it. Passing proves the fast path was taken. - `empty_egress_policy_still_clears_rules_a_previous_tenant_left` — with the flag forced to `true`, the call must be attempted (and therefore fail, since there is no namespace) rather than skipped. Skipped checks and reasons: - Full `make test-unit` not run; only the two relevant tests plus a full `--all-targets` build. The remaining phases need capabilities and a provisioned host. - **No before/after timing is included.** The justification here is structural — one fewer process spawn and netns switch per start — not measured. I did not have a cleanly provisioned node to produce trustworthy numbers, and I would rather not attach numbers from a noisy host. `scripts/bench.py create` on a warmed pool is the way to quantify it. ## Risks and reviewer notes The correctness question is the state machine on `user_egress_rules_present`, so please review that closely: - Initial `false` is correct only if a new namespace really starts with an empty user egress chain. - `result.is_err() || wants_rules` errs toward assuming rules exist. The cost of being wrong in that direction is a redundant clear; the cost in the other direction would be a leaked policy across tenants, which is a security issue. - Slots recycled through the warm pool are covered by the flag persisting on the `Slot`, not being reset on release. The security-relevant scenario is a slot that held a restrictive policy being reused by a tenant with no policy. That is exactly the second test. Most important file: `src/sandbox/network/slot.rs`. ## 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:46 +00:00