[PR #103] [MERGED] fix(sandbox): isolate envd bootstrap connections across generations #180

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/103
Author: @leether
Created: 8/1/2026
Status: ✅ Merged
Merged: 8/1/2026
Merged by: @yingdi-shan

Base: main ← Head: leether/fix-envd-bootstrap-pr102

📝 Commits (1)

facc249 fix(sandbox): isolate envd bootstrap connections across generations

📊 Changes

1 file changed (+122 additions, -4 deletions)

View changed files

📝 src/sandbox/envd.rs (+122 -4)

📄 Description

What

disable idle TCP retention for the shared envd bootstrap HTTP client;
keep the change limited to envd GET /health and POST /init;
add a bounded behavioral regression test that requires the two bootstrap requests to use different TCP connections.

Why

AgentENV reuses interaction IPs across sandbox runtime generations, while the process-wide reqwest client pooled idle connections by HTTP origin (scheme + IP + port). The pool key does not include the sandbox runtime generation.

After snapshot restore, a later runtime can therefore receive the same interaction IP while the host client still holds a TCP connection from the previous runtime. If the restored guest also contains the matching TCP tuple, the host and guest can be in different TCP sequence spaces. An envd health or init request may then stall until the TCP user timeout instead of establishing a fresh connection.

The packet-level reproduction, controls, and full stress evidence are in #102. The issue was discovered during ARM64 snapshot validation in #101, but the lifecycle condition is not architecture-specific.

Closes #102

#56 fixed the same origin-versus-runtime-generation mismatch for the separate sandbox HTTP proxy client;
#101 is the ARM64 assembly-line RFC where this bootstrap stall was discovered.

Scope and non-goals

This PR changes only src/sandbox/envd.rs:

the envd bootstrap HTTP client no longer retains idle connections;
the client object remains shared, so construction and configuration are not repeated per sandbox;
the regression test covers observable connection behavior without inspecting reqwest internals.

This PR does not change:

sandbox HTTP proxy traffic;
gRPC process execution;
Firecracker, the guest kernel, envd, or tools.ext4;
snapshot formats, artifact layouts, or network-slot allocation;
proxy environment handling or HTTP protocol selection.

Design and behavior changes

The shared bootstrap client is now built with:

Client::builder()
    .pool_max_idle_per_host(0)
    .build()

For a successful bootstrap, health and init therefore use separate TCP connections. Failed health retries also open fresh connections. This prevents a bootstrap request from checking out a socket retained from a previous VM generation.

Command execution remains on the separately created gRPC ProcessClient, so repeated exec calls are unaffected. The patch intentionally does not add .no_proxy() or .http1_only() because proxy routing and protocol negotiation are independent behaviors.

The regression test:

answers health over a keep-alive socket and holds that socket open as the previous-generation connection;
treats a second accept() as connecting to the next VM assigned the same address;
requires init to arrive on the second connection;
bounds individual steps and the complete interaction with timeouts.

With pool_max_idle_per_host(1) as a local negative control, the test fails because init reuses the first socket. With 0, it passes.

Compatibility and operations

Public API or generated protocol: N/A — no API or generated-code changes.
Configuration or defaults: N/A — no new configuration or changed default.
Snapshot manifest, artifact layout, or storage format: N/A — no persisted-format changes.
Upgrade and rollback: no migration is required; deploying the updated server enables the behavior, and rollback is the previous server binary or a revert of this commit.
Host requirements, permissions, ports, or dependencies: N/A — unchanged.

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 fmt
  passed

make clippy
  passed for the full workspace with all targets and all features

make AENV_TEST_STATE_ID=pr102-arm64-20260801 \
  "CAPABILITY_RUNNER=CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_RUNNER=$PWD/scripts/run-with-capabilities.sh" \
  test-unit
  passed: agentenv 671, linux-cap 3, privileged agentenv 4,
  uvm-ublk 10, uvm-ublk-daemon 46, plus both verification scripts

cargo test -p agentenv --lib sandbox::envd::tests
  passed: 2

local negative control: pool_max_idle_per_host(1)
  failed in 2.03 s with the expected
  "init reused the health connection instead of opening a new one"

official snapshot benchmark, three isolated rounds of 10 cold + 10 hot
  passed: 60/60 completed; no sample exceeded 1 s; no approximately 30 s stall

The benchmark is evidence that the observed stall is removed, not a controlled A/B attribution of sub-second latency. Post-fix cold means were 168.50–182.34 ms; hot means were 119.81–124.81 ms. Full sample ranges and the deeper sequential/concurrent stress results are recorded in #102.

Skipped checks and reasons:

Separate Rust integration targets were not rerun on this clean commit; the focused behavioral test and the real snapshot benchmark exercise the changed lifecycle path. The same change had previously passed fc::multiple_resumes_have_independent_disk_state downstream.
make -C services test was not run because services/ is unchanged.
Code generation was not run because no schema or generated file changed.
Documentation was not changed because the user-facing API and configuration are unchanged.
On ARM64, the current Makefile's default capability runner variable is x86_64-specific. make test-unit was therefore run with the command-line CAPABILITY_RUNNER override shown above; this PR intentionally does not include an unrelated Makefile change.

Risks and reviewer notes

Bootstrap pays one additional host-to-guest TCP handshake, and failed health retries also use fresh sockets. The affected client performs only health/init; sustained exec and proxy traffic use other clients.
Short-lived bootstrap sockets can enter TIME_WAIT. The concurrent stress run reported in #102 returned to zero orphan sockets within 20 seconds and file-handle allocation returned close to baseline.
A per-runtime client also prevents the failure, but it would require threading runtime identity through this generated-client path solely to preserve one health-to-init idle connection. Disabling idle retention is narrower for a two-request bootstrap path.
The complete mock interaction has a five-second outer timeout, so connector or handshake regressions fail CI instead of hanging it.
Reviewer focus: the client construction and bootstrap_http_client_does_not_reuse_connections test in src/sandbox/envd.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/103 **Author:** [@leether](https://github.com/leether) **Created:** 8/1/2026 **Status:** ✅ Merged **Merged:** 8/1/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `leether/fix-envd-bootstrap-pr102` --- ### 📝 Commits (1) - [`facc249`](https://github.com/kvcache-ai/AgentENV/commit/facc24984b5e1c060b192b6874debac8464f0f92) fix(sandbox): isolate envd bootstrap connections across generations ### 📊 Changes **1 file changed** (+122 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `src/sandbox/envd.rs` (+122 -4) </details> ### 📄 Description ## What - disable idle TCP retention for the shared envd bootstrap HTTP client; - keep the change limited to envd `GET /health` and `POST /init`; - add a bounded behavioral regression test that requires the two bootstrap requests to use different TCP connections. ## Why AgentENV reuses interaction IPs across sandbox runtime generations, while the process-wide reqwest client pooled idle connections by HTTP origin (`scheme + IP + port`). The pool key does not include the sandbox runtime generation. After snapshot restore, a later runtime can therefore receive the same interaction IP while the host client still holds a TCP connection from the previous runtime. If the restored guest also contains the matching TCP tuple, the host and guest can be in different TCP sequence spaces. An envd health or init request may then stall until the TCP user timeout instead of establishing a fresh connection. The packet-level reproduction, controls, and full stress evidence are in #102. The issue was discovered during ARM64 snapshot validation in #101, but the lifecycle condition is not architecture-specific. ## Related issue Closes #102 Related: - #56 fixed the same origin-versus-runtime-generation mismatch for the separate sandbox HTTP proxy client; - #101 is the ARM64 assembly-line RFC where this bootstrap stall was discovered. ## Scope and non-goals This PR changes only `src/sandbox/envd.rs`: - the envd bootstrap HTTP client no longer retains idle connections; - the client object remains shared, so construction and configuration are not repeated per sandbox; - the regression test covers observable connection behavior without inspecting reqwest internals. This PR does not change: - sandbox HTTP proxy traffic; - gRPC process execution; - Firecracker, the guest kernel, envd, or `tools.ext4`; - snapshot formats, artifact layouts, or network-slot allocation; - proxy environment handling or HTTP protocol selection. ## Design and behavior changes The shared bootstrap client is now built with: ```rust Client::builder() .pool_max_idle_per_host(0) .build() ``` For a successful bootstrap, health and init therefore use separate TCP connections. Failed health retries also open fresh connections. This prevents a bootstrap request from checking out a socket retained from a previous VM generation. Command execution remains on the separately created gRPC `ProcessClient`, so repeated exec calls are unaffected. The patch intentionally does not add `.no_proxy()` or `.http1_only()` because proxy routing and protocol negotiation are independent behaviors. The regression test: 1. answers health over a keep-alive socket and holds that socket open as the previous-generation connection; 2. treats a second `accept()` as connecting to the next VM assigned the same address; 3. requires init to arrive on the second connection; 4. bounds individual steps and the complete interaction with timeouts. With `pool_max_idle_per_host(1)` as a local negative control, the test fails because init reuses the first socket. With `0`, it passes. ## Compatibility and operations - Public API or generated protocol: N/A — no API or generated-code changes. - Configuration or defaults: N/A — no new configuration or changed default. - Snapshot manifest, artifact layout, or storage format: N/A — no persisted-format changes. - Upgrade and rollback: no migration is required; deploying the updated server enables the behavior, and rollback is the previous server binary or a revert of this commit. - Host requirements, permissions, ports, or dependencies: N/A — unchanged. ## Validation - [x] `make fmt` - [x] `make clippy` - [x] `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 - [x] Benchmarks or performance comparison completed Commands and results: ```text make fmt passed make clippy passed for the full workspace with all targets and all features make AENV_TEST_STATE_ID=pr102-arm64-20260801 \ "CAPABILITY_RUNNER=CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_RUNNER=$PWD/scripts/run-with-capabilities.sh" \ test-unit passed: agentenv 671, linux-cap 3, privileged agentenv 4, uvm-ublk 10, uvm-ublk-daemon 46, plus both verification scripts cargo test -p agentenv --lib sandbox::envd::tests passed: 2 local negative control: pool_max_idle_per_host(1) failed in 2.03 s with the expected "init reused the health connection instead of opening a new one" official snapshot benchmark, three isolated rounds of 10 cold + 10 hot passed: 60/60 completed; no sample exceeded 1 s; no approximately 30 s stall ``` The benchmark is evidence that the observed stall is removed, not a controlled A/B attribution of sub-second latency. Post-fix cold means were `168.50–182.34 ms`; hot means were `119.81–124.81 ms`. Full sample ranges and the deeper sequential/concurrent stress results are recorded in #102. Skipped checks and reasons: - Separate Rust integration targets were not rerun on this clean commit; the focused behavioral test and the real snapshot benchmark exercise the changed lifecycle path. The same change had previously passed `fc::multiple_resumes_have_independent_disk_state` downstream. - `make -C services test` was not run because `services/` is unchanged. - Code generation was not run because no schema or generated file changed. - Documentation was not changed because the user-facing API and configuration are unchanged. - On ARM64, the current Makefile's default capability runner variable is x86_64-specific. `make test-unit` was therefore run with the command-line `CAPABILITY_RUNNER` override shown above; this PR intentionally does not include an unrelated Makefile change. ## Risks and reviewer notes - Bootstrap pays one additional host-to-guest TCP handshake, and failed health retries also use fresh sockets. The affected client performs only health/init; sustained exec and proxy traffic use other clients. - Short-lived bootstrap sockets can enter `TIME_WAIT`. The concurrent stress run reported in #102 returned to zero orphan sockets within 20 seconds and file-handle allocation returned close to baseline. - A per-runtime client also prevents the failure, but it would require threading runtime identity through this generated-client path solely to preserve one health-to-init idle connection. Disabling idle retention is narrower for a two-request bootstrap path. - The complete mock interaction has a five-second outer timeout, so connector or handshake regressions fail CI instead of hanging it. - Reviewer focus: the client construction and `bootstrap_http_client_does_not_reuse_connections` test in `src/sandbox/envd.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:43 +00:00