[PR #272] [MERGED] fix(sandbox): honor disabled Firecracker logging #308

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/firecracker-logging-default

📝 Commits (1)

279ce5b fix(sandbox): honor disabled Firecracker logging

📊 Changes

9 files changed (+215 additions, -55 deletions)

View changed files

📝 config/default.toml (+4 -3)
📝 docs/src/configuration/reference.md (+2 -2)
📝 docs/src/internals/persistence-artifact-inventory.md (+2 -2)
📝 docs/src/internals/sandbox-testing.md (+13 -10)
📝 src/cfg.rs (+3 -3)
📝 src/sandbox/firecracker/config.rs (+10 -3)
📝 src/sandbox/firecracker/pool.rs (+19 -12)
📝 src/sandbox/firecracker/sandbox.rs (+161 -20)
📝 tests/integration/snapshot_attached_drive.rs (+1 -0)

📄 Description

What

Honor disabled Firecracker logging for stdout/stderr capture on fresh boots, snapshot restores, and warm process launches. With firecracker.log_level unset or empty, launches send stdout/stderr to /dev/null and create no log files or per-sandbox serial directories.

Why

The existing setting disables firecracker.log, but stdout/stderr files are still opened unconditionally. Silent snapshot launches therefore leave empty persistent directories behind. Issue #260 reports declining sandbox-create throughput as this directory grows.

Closes #260.

Related proposals: #261 (directory sharding) and #265 (log retention).

Scope and non-goals

Covers logging defaults, cold and warm spawn paths, regression tests, and documentation. Does not add sharding, retention, or cleanup of existing logs.

Design and behavior changes

A non-empty log level enables stdout/stderr capture alongside firecracker.log. Setting serial_dir alone does not enable capture. Whitespace-only levels also disable logging.
Explicit Rust stdout/stderr destinations enable only the requested streams even without a log level.
The process pool opens logs only when capture is enabled and declines requests with different capture settings, since process stdio is fixed at spawn. Disabled warm capture skips log relocation.
The integration test that inspects guest kernel messages now explicitly requests stdout capture.

Compatibility and operations

Public API or generated protocol: unchanged.
Configuration or defaults: no new keys. firecracker.log_level now controls default serial capture as well as the internal logger. Set it to Info to retain boot diagnostics and Firecracker logs.
Snapshot manifest, artifact layout, or storage format: unchanged.
Upgrade and rollback: deployments without a log level stop collecting new serial logs. Existing files are preserved. Rollback restores unconditional serial capture.
Host requirements, permissions, ports, or dependencies: 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
CARGO_TARGET_DIR=/home/ubuntu/AgentENV/target \
AENV_HOME_PATH=/tmp/aenv-issue-260-pr-tests/home \
AENV_RUNTIME_PATH=/tmp/aenv-issue-260-pr-tests/run \
AENV_DEPS_PATH=/tmp/aenv-issue-260-pr-tests/deps \
cargo test -p agentenv --lib sandbox::firecracker::
  78 passed
CARGO_TARGET_DIR=/home/ubuntu/AgentENV/target make clippy
  passed (workspace, all targets/features, -D warnings)
git diff --check
  passed

The disabled-logging regression failed before the fix because spawning created a log directory. Coverage includes unset/empty/whitespace levels, persistent and work-directory destinations, cold and warm capture, enabled output and relocation, and explicit stream overrides. Builds and tests run as the non-root workspace user with isolated test state.

Skipped checks and reasons:

Full make test-unit and VM integration suites: validation is focused on Firecracker process output and filesystem behavior; VM boot and the modified volume integration test were not rerun.
Go tests and code generation: no Go or schema changes.
Throughput benchmark: not rerun; no measured performance improvement is claimed.

Risks and reviewer notes

Default startup errors no longer include captured stderr; enable logging when boot diagnostics are needed. Enabled persistent logs still require operator-managed retention, and existing directories are not pruned. Review the capture selection in sandbox.rs and pool compatibility check in pool.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/272 **Author:** [@yingdi-shan](https://github.com/yingdi-shan) **Created:** 9/10/2026 **Status:** ✅ Merged **Merged:** 9/10/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `fix/firecracker-logging-default` --- ### 📝 Commits (1) - [`279ce5b`](https://github.com/kvcache-ai/AgentENV/commit/279ce5b4345084ca611c7607fd58828a28cb2c25) fix(sandbox): honor disabled Firecracker logging ### 📊 Changes **9 files changed** (+215 additions, -55 deletions) <details> <summary>View changed files</summary> 📝 `config/default.toml` (+4 -3) 📝 `docs/src/configuration/reference.md` (+2 -2) 📝 `docs/src/internals/persistence-artifact-inventory.md` (+2 -2) 📝 `docs/src/internals/sandbox-testing.md` (+13 -10) 📝 `src/cfg.rs` (+3 -3) 📝 `src/sandbox/firecracker/config.rs` (+10 -3) 📝 `src/sandbox/firecracker/pool.rs` (+19 -12) 📝 `src/sandbox/firecracker/sandbox.rs` (+161 -20) 📝 `tests/integration/snapshot_attached_drive.rs` (+1 -0) </details> ### 📄 Description ## What Honor disabled Firecracker logging for stdout/stderr capture on fresh boots, snapshot restores, and warm process launches. With `firecracker.log_level` unset or empty, launches send stdout/stderr to `/dev/null` and create no log files or per-sandbox serial directories. ## Why The existing setting disables `firecracker.log`, but stdout/stderr files are still opened unconditionally. Silent snapshot launches therefore leave empty persistent directories behind. Issue #260 reports declining sandbox-create throughput as this directory grows. ## Related issue Closes #260. Related proposals: #261 (directory sharding) and #265 (log retention). ## Scope and non-goals Covers logging defaults, cold and warm spawn paths, regression tests, and documentation. Does not add sharding, retention, or cleanup of existing logs. ## Design and behavior changes - A non-empty log level enables stdout/stderr capture alongside `firecracker.log`. Setting `serial_dir` alone does not enable capture. Whitespace-only levels also disable logging. - Explicit Rust stdout/stderr destinations enable only the requested streams even without a log level. - The process pool opens logs only when capture is enabled and declines requests with different capture settings, since process stdio is fixed at spawn. Disabled warm capture skips log relocation. - The integration test that inspects guest kernel messages now explicitly requests stdout capture. ## Compatibility and operations - Public API or generated protocol: unchanged. - Configuration or defaults: no new keys. `firecracker.log_level` now controls default serial capture as well as the internal logger. Set it to `Info` to retain boot diagnostics and Firecracker logs. - Snapshot manifest, artifact layout, or storage format: unchanged. - Upgrade and rollback: deployments without a log level stop collecting new serial logs. Existing files are preserved. Rollback restores unconditional serial capture. - Host requirements, permissions, ports, or dependencies: unchanged. ## 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 - [x] Documentation updated - [ ] Benchmarks or performance comparison completed Commands and results: ```text make fmt passed CARGO_TARGET_DIR=/home/ubuntu/AgentENV/target \ AENV_HOME_PATH=/tmp/aenv-issue-260-pr-tests/home \ AENV_RUNTIME_PATH=/tmp/aenv-issue-260-pr-tests/run \ AENV_DEPS_PATH=/tmp/aenv-issue-260-pr-tests/deps \ cargo test -p agentenv --lib sandbox::firecracker:: 78 passed CARGO_TARGET_DIR=/home/ubuntu/AgentENV/target make clippy passed (workspace, all targets/features, -D warnings) git diff --check passed ``` The disabled-logging regression failed before the fix because spawning created a log directory. Coverage includes unset/empty/whitespace levels, persistent and work-directory destinations, cold and warm capture, enabled output and relocation, and explicit stream overrides. Builds and tests run as the non-root workspace user with isolated test state. Skipped checks and reasons: - Full `make test-unit` and VM integration suites: validation is focused on Firecracker process output and filesystem behavior; VM boot and the modified volume integration test were not rerun. - Go tests and code generation: no Go or schema changes. - Throughput benchmark: not rerun; no measured performance improvement is claimed. ## Risks and reviewer notes Default startup errors no longer include captured stderr; enable logging when boot diagnostics are needed. Enabled persistent logs still require operator-managed retention, and existing directories are not pruned. Review the capture selection in `sandbox.rs` and pool compatibility check in `pool.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:22:03 +00:00