[PR #374] [MERGED] refactor(sandbox): reach guest envd through one backend method #386

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: refactor/envd-call-helper

📝 Commits (1)

7a9731c refactor(sandbox): reach guest envd through one backend method

📊 Changes

8 files changed (+165 additions, -152 deletions)

View changed files

📝 src/orchestrator/sandbox_metrics.rs (+4 -4)
📝 src/orchestrator/service.rs (+38 -36)
📝 src/orchestrator/tests.rs (+76 -9)
📝 src/sandbox/backend.rs (+6 -24)
📝 src/sandbox/envd.rs (+33 -8)
📝 src/sandbox/firecracker/sandbox.rs (+2 -22)
📝 src/sandbox/mock.rs (+5 -49)
📝 src/sandbox/mod.rs (+1 -0)

📄 Description

What

Guest calls become methods of an EnvdApi trait in src/sandbox/envd.rs, implemented by EnvdInstance.
SandboxBackend exposes that API once, through fn envd(&self) -> Option<Arc<dyn EnvdApi>>. It replaces the per-call methods metrics_sample and logs_query.
The orchestrator's request-time lookup, running check and lock handling move into one helper, running_envd().

Why

Each guest-facing feature has repeated the same plumbing. #311 (metrics) and #338 (logs) each:

added a SandboxBackend method that captures a future under the runtime lock;
implemented it again in FirecrackerSandbox and in MockSandboxBackend, with a new mock closure type (MetricsSampler, LogsReader);
copied about 40 lines of orchestrator code: look up the handle, lock it, re-check that the sandbox is running, build the future, release the lock, await.

After this PR, a new guest call (process list, file watch, and so on) adds an EnvdApi method and its caller. The backend trait, the Firecracker backend and the mock stay unchanged.

N/A. Follow-up to the review behind #367.

Scope and non-goals

Logs and metrics move to the new seam. Lifecycle methods (pause, snapshot, volume freeze and thaw, and so on) stay on SandboxBackend.
The metrics sampler keeps its own path: try_lock, skipping busy sandboxes, and generation checks. It only swaps metrics_sample() for envd()?.metrics().

Design and behavior changes

The locking contract is unchanged:
- envd() is called under the runtime lock and only clones a handle;
- guest I/O happens after the lock is released;
- request-time calls re-check that the sandbox is Running under the lock.
Orchestrator tests set a FakeEnvd on the mock backend. It runs the same test closures as before, so the existing tests still prove that guest I/O does not hold the runtime-map or backend lock.
Two internal error messages change:
- "sandbox runtime is transitioning; retry log query" loses "log query", since the helper is shared;
- "sandbox backend does not support logs" becomes "sandbox guest agent is not connected". A missing envd connection is what that error actually means.

Compatibility and operations

Public API or generated protocol: none. HTTP status codes are unchanged.
Configuration or defaults: none.
Snapshot manifest, artifact layout, or storage format: none.
Upgrade and rollback: none.
Host requirements, permissions, ports, or dependencies: none.

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                                            # clean
make clippy                                         # clean
cargo test -p agentenv -p envd -p linux-cap --lib   # agentenv: 962 passed, 4 ignored; envd: 3 passed
cargo test -p agentenv --lib -- orchestrator:: sandbox::envd   # 170 passed

Skipped checks and reasons:

make test-unit capability tests (--ignored) and the Firecracker integration suites need /dev/kvm, ublk and capabilities. The Firecracker change is a 3-line envd() that clones the existing envd_instance; the metrics and logs bodies in EnvdInstance are unchanged and keep their HTTP-level tests in src/sandbox/envd.rs.

Risks and reviewer notes

Start with src/orchestrator/service.rs (running_envd) and src/sandbox/backend.rs.
envd() is a required trait method with no default. Any out-of-tree SandboxBackend implementation must add it; there are none in this repository.

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.

🤖 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/374 **Author:** [@yingdi-shan](https://github.com/yingdi-shan) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `refactor/envd-call-helper` --- ### 📝 Commits (1) - [`7a9731c`](https://github.com/kvcache-ai/AgentENV/commit/7a9731ce419a96b8257ad73642e814a861d1632a) refactor(sandbox): reach guest envd through one backend method ### 📊 Changes **8 files changed** (+165 additions, -152 deletions) <details> <summary>View changed files</summary> 📝 `src/orchestrator/sandbox_metrics.rs` (+4 -4) 📝 `src/orchestrator/service.rs` (+38 -36) 📝 `src/orchestrator/tests.rs` (+76 -9) 📝 `src/sandbox/backend.rs` (+6 -24) 📝 `src/sandbox/envd.rs` (+33 -8) 📝 `src/sandbox/firecracker/sandbox.rs` (+2 -22) 📝 `src/sandbox/mock.rs` (+5 -49) 📝 `src/sandbox/mod.rs` (+1 -0) </details> ### 📄 Description ## What - Guest calls become methods of an `EnvdApi` trait in `src/sandbox/envd.rs`, implemented by `EnvdInstance`. - `SandboxBackend` exposes that API once, through `fn envd(&self) -> Option<Arc<dyn EnvdApi>>`. It replaces the per-call methods `metrics_sample` and `logs_query`. - The orchestrator's request-time lookup, running check and lock handling move into one helper, `running_envd()`. ## Why Each guest-facing feature has repeated the same plumbing. #311 (metrics) and #338 (logs) each: - added a `SandboxBackend` method that captures a future under the runtime lock; - implemented it again in `FirecrackerSandbox` and in `MockSandboxBackend`, with a new mock closure type (`MetricsSampler`, `LogsReader`); - copied about 40 lines of orchestrator code: look up the handle, lock it, re-check that the sandbox is running, build the future, release the lock, await. After this PR, a new guest call (process list, file watch, and so on) adds an `EnvdApi` method and its caller. The backend trait, the Firecracker backend and the mock stay unchanged. ## Related issue N/A. Follow-up to the review behind #367. ## Scope and non-goals - Logs and metrics move to the new seam. Lifecycle methods (`pause`, `snapshot`, volume freeze and thaw, and so on) stay on `SandboxBackend`. - The metrics sampler keeps its own path: `try_lock`, skipping busy sandboxes, and generation checks. It only swaps `metrics_sample()` for `envd()?.metrics()`. ## Design and behavior changes - The locking contract is unchanged: - `envd()` is called under the runtime lock and only clones a handle; - guest I/O happens after the lock is released; - request-time calls re-check that the sandbox is `Running` under the lock. - Orchestrator tests set a `FakeEnvd` on the mock backend. It runs the same test closures as before, so the existing tests still prove that guest I/O does not hold the runtime-map or backend lock. - Two internal error messages change: - "sandbox runtime is transitioning; retry log query" loses "log query", since the helper is shared; - "sandbox backend does not support logs" becomes "sandbox guest agent is not connected". A missing envd connection is what that error actually means. ## Compatibility and operations - Public API or generated protocol: none. HTTP status codes are unchanged. - Configuration or defaults: none. - Snapshot manifest, artifact layout, or storage format: none. - Upgrade and rollback: none. - Host requirements, permissions, ports, or dependencies: none. ## Validation - [x] `make fmt` - [x] `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: ```text make fmt # clean make clippy # clean cargo test -p agentenv -p envd -p linux-cap --lib # agentenv: 962 passed, 4 ignored; envd: 3 passed cargo test -p agentenv --lib -- orchestrator:: sandbox::envd # 170 passed ``` Skipped checks and reasons: - `make test-unit` capability tests (`--ignored`) and the Firecracker integration suites need `/dev/kvm`, ublk and capabilities. The Firecracker change is a 3-line `envd()` that clones the existing `envd_instance`; the metrics and logs bodies in `EnvdInstance` are unchanged and keep their HTTP-level tests in `src/sandbox/envd.rs`. ## Risks and reviewer notes - Start with `src/orchestrator/service.rs` (`running_envd`) and `src/sandbox/backend.rs`. - `envd()` is a required trait method with no default. Any out-of-tree `SandboxBackend` implementation must add it; there are none in this repository. ## 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. 🤖 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:51 +00:00