[PR #265] [CLOSED] fix(sandbox): clean up empty serial logs and expire inactive logs #301

New issue

Closed

opened 2026-10-29 14:21:58 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:21:58 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/265
Author: @yingdi-shan
Created: 9/8/2026
Status: ❌ Closed

Base: main ← Head: fix/firecracker-log-retention

📝 Commits (1)

6b14029 fix(sandbox): clean up empty serial logs and expire inactive logs

📊 Changes

17 files changed (+761 additions, -24 deletions)

View changed files

📝 config/default.toml (+4 -0)
📝 docs/src/configuration/env-vars.md (+1 -0)
📝 docs/src/configuration/reference.md (+23 -0)
📝 docs/src/internals/networking.md (+1 -1)
📝 docs/src/internals/persistence-artifact-inventory.md (+2 -2)
📝 docs/src/internals/sandbox-testing.md (+5 -2)
📝 src/bin/server.rs (+11 -1)
📝 src/cfg.rs (+7 -0)
📝 src/orchestrator/service.rs (+0 -7)
📝 src/sandbox/firecracker/instance.rs (+10 -5)
📝 src/sandbox/firecracker/mod.rs (+2 -0)
📝 src/sandbox/firecracker/sandbox.rs (+154 -1)
➕ src/sandbox/firecracker/serial_logs.rs (+513 -0)
📝 src/sandbox/mod.rs (+4 -3)
📝 src/sandbox/network/mod.rs (+9 -0)
📝 tests/integration/orchestrator.rs (+14 -0)
📝 tests/orchestrator_integration.rs (+1 -2)

📄 Description

What

Remove empty managed Firecracker logs after confirmed process exit and expire inactive nonempty logs after a configurable retention period. Keep the existing {serial_dir}/{sandbox_id}/ layout.

Why

Silent snapshot launches leave permanent directories and empty log files behind, growing the serial root with every sandbox created. Cleaning stopped sandboxes prevents this accumulation during normal operation.

Closes #260. Alternative to #261.

Scope and non-goals

Includes cleanup after stop, retention, live-writer protection, documentation, and regression tests. No sharding, lazy capture, active-log rotation, or throughput improvement claim. Legacy directories and logs left after an abnormal drop or server crash require offline cleanup.

Design and behavior changes

Hold a directory lock before opening or relocating managed logs. Lock contention yields asynchronously and can be cancelled.
After stop confirms process exit, create an .inactive marker under the lock and remove empty known logs. Resuming an ID clears the marker before opening logs. Abnormal drop leaves logs unmarked, preserving late internal-logger output even with custom stdout/stderr paths.
The background collector only handles marked sandbox-ID directories, taking a nonblocking exclusive directory lock. It preserves symlink entries and custom files, and never recursively deletes directories or scans host process descriptors.
Start a sweep with the server and repeat 60 seconds after each pass. Filesystem work runs on the blocking pool; shutdown cancels between directory entries and joins the worker. Entries disappearing during a sweep are skipped.
Retain inactive nonempty logs for firecracker.serial_log_retention_secs, default 604800 (seven days). Zero disables expiry. Retention uses the newest directory/log modification time, refreshed after confirmed exit.
Preserve startup diagnostics and append to retained stdout/stderr on resume by bypassing the warm process pool when necessary.
Keep process-wide networking available across individual orchestrator shutdowns. The server shuts it down after sandboxes and the Firecracker pool; the restart integration test exercises a fresh process and retained stderr history.

Compatibility and operations

Public API or generated protocol: no HTTP or schema changes. Adds a Rust process-shutdown helper; an individual orchestrator shutdown no longer stops shared networking permanently.
Configuration or defaults: adds firecracker.serial_log_retention_secs and AENV_FIRECRACKER_SERIAL_LOG_RETENTION_SECS, default seven days.
Snapshot manifest, artifact layout, or storage format: snapshot formats unchanged. Managed serial directories may contain an internal .inactive marker after stop.
Upgrade and rollback: unmarked legacy/crash logs are preserved. Clean those manually only after stopping all servers and Firecracker processes using the root. Do not share a serial root with older servers that do not honor the marker protocol. Rollback stops collection but cannot restore expired logs.
Host requirements, permissions, ports, or dependencies: no new dependencies. The serial root must be node-local and writable only by the trusted runtime user. Explicit stdout/stderr overrides outside the managed tree are not collected.

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:

cargo test -p agentenv --lib serial --no-fail-fast
  24 passed
make fmt
  passed
make clippy
  passed (workspace, all targets/features, -D warnings)
make test-unit AENV_TEST_STATE_DIR=/tmp/aenv-serial-retention-tests AENV_TEST_DEPS_PATH=/tmp/aenv-serial-retention-tests/deps
  passed: 839 agent tests, 4 capability tests, 3 linux-cap tests,
  8 ublk tests, 46 daemon tests, and both shell verification scripts
make test-agent-integration PROFILE=debug AENV_TEST_STATE_DIR=/tmp/aenv-serial-retention-tests AENV_TEST_DEPS_PATH=/tmp/aenv-serial-retention-tests/deps
  passed: 18 VM integration tests, 2 orchestrator tests, 5 snapshot OSS tests

All builds and Rust tests run as the non-root workspace user. Test state and dependencies are isolated under /tmp/aenv-serial-retention-tests; privileged tests use the repository capability runner.

Skipped checks and reasons:

Go tests and API regeneration: no Go, schema, or generated source changes.
Broader envd/storage integration suites: not rerun; this change affects Firecracker log lifecycle.
Throughput benchmarks: no performance measurements are claimed.

Risks and reviewer notes

The main review area is src/sandbox/firecracker/serial_logs.rs: eligibility after confirmed exit, directory locking, and retention. Regression coverage includes late unlocked logger writes after abnormal drop, interrupted resume, cancellable lock contention, collection during repeated launches/stops, symlinks/custom entries, startup failure, retained history, and warm-log relocation.

Unmarked logs need offline cleanup. Retention bounds eligible inactive-log lifetime; it does not limit active-log size, peak directory count, or compact an already expanded filesystem directory inode. Filesystem syscalls themselves cannot be interrupted by worker cancellation.

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/265 **Author:** [@yingdi-shan](https://github.com/yingdi-shan) **Created:** 9/8/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/firecracker-log-retention` --- ### 📝 Commits (1) - [`6b14029`](https://github.com/kvcache-ai/AgentENV/commit/6b140291eccf0d40a03d6564b36b22b82cde76cd) fix(sandbox): clean up empty serial logs and expire inactive logs ### 📊 Changes **17 files changed** (+761 additions, -24 deletions) <details> <summary>View changed files</summary> 📝 `config/default.toml` (+4 -0) 📝 `docs/src/configuration/env-vars.md` (+1 -0) 📝 `docs/src/configuration/reference.md` (+23 -0) 📝 `docs/src/internals/networking.md` (+1 -1) 📝 `docs/src/internals/persistence-artifact-inventory.md` (+2 -2) 📝 `docs/src/internals/sandbox-testing.md` (+5 -2) 📝 `src/bin/server.rs` (+11 -1) 📝 `src/cfg.rs` (+7 -0) 📝 `src/orchestrator/service.rs` (+0 -7) 📝 `src/sandbox/firecracker/instance.rs` (+10 -5) 📝 `src/sandbox/firecracker/mod.rs` (+2 -0) 📝 `src/sandbox/firecracker/sandbox.rs` (+154 -1) ➕ `src/sandbox/firecracker/serial_logs.rs` (+513 -0) 📝 `src/sandbox/mod.rs` (+4 -3) 📝 `src/sandbox/network/mod.rs` (+9 -0) 📝 `tests/integration/orchestrator.rs` (+14 -0) 📝 `tests/orchestrator_integration.rs` (+1 -2) </details> ### 📄 Description ## What Remove empty managed Firecracker logs after confirmed process exit and expire inactive nonempty logs after a configurable retention period. Keep the existing `{serial_dir}/{sandbox_id}/` layout. ## Why Silent snapshot launches leave permanent directories and empty log files behind, growing the serial root with every sandbox created. Cleaning stopped sandboxes prevents this accumulation during normal operation. ## Related issue Closes #260. Alternative to #261. ## Scope and non-goals Includes cleanup after stop, retention, live-writer protection, documentation, and regression tests. No sharding, lazy capture, active-log rotation, or throughput improvement claim. Legacy directories and logs left after an abnormal drop or server crash require offline cleanup. ## Design and behavior changes - Hold a directory lock before opening or relocating managed logs. Lock contention yields asynchronously and can be cancelled. - After stop confirms process exit, create an `.inactive` marker under the lock and remove empty known logs. Resuming an ID clears the marker before opening logs. Abnormal drop leaves logs unmarked, preserving late internal-logger output even with custom stdout/stderr paths. - The background collector only handles marked sandbox-ID directories, taking a nonblocking exclusive directory lock. It preserves symlink entries and custom files, and never recursively deletes directories or scans host process descriptors. - Start a sweep with the server and repeat 60 seconds after each pass. Filesystem work runs on the blocking pool; shutdown cancels between directory entries and joins the worker. Entries disappearing during a sweep are skipped. - Retain inactive nonempty logs for `firecracker.serial_log_retention_secs`, default 604800 (seven days). Zero disables expiry. Retention uses the newest directory/log modification time, refreshed after confirmed exit. - Preserve startup diagnostics and append to retained stdout/stderr on resume by bypassing the warm process pool when necessary. - Keep process-wide networking available across individual orchestrator shutdowns. The server shuts it down after sandboxes and the Firecracker pool; the restart integration test exercises a fresh process and retained stderr history. ## Compatibility and operations - Public API or generated protocol: no HTTP or schema changes. Adds a Rust process-shutdown helper; an individual orchestrator shutdown no longer stops shared networking permanently. - Configuration or defaults: adds `firecracker.serial_log_retention_secs` and `AENV_FIRECRACKER_SERIAL_LOG_RETENTION_SECS`, default seven days. - Snapshot manifest, artifact layout, or storage format: snapshot formats unchanged. Managed serial directories may contain an internal `.inactive` marker after stop. - Upgrade and rollback: unmarked legacy/crash logs are preserved. Clean those manually only after stopping all servers and Firecracker processes using the root. Do not share a serial root with older servers that do not honor the marker protocol. Rollback stops collection but cannot restore expired logs. - Host requirements, permissions, ports, or dependencies: no new dependencies. The serial root must be node-local and writable only by the trusted runtime user. Explicit stdout/stderr overrides outside the managed tree are not collected. ## 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 - [x] Documentation updated - [ ] Benchmarks or performance comparison completed Commands and results: ```text cargo test -p agentenv --lib serial --no-fail-fast 24 passed make fmt passed make clippy passed (workspace, all targets/features, -D warnings) make test-unit AENV_TEST_STATE_DIR=/tmp/aenv-serial-retention-tests AENV_TEST_DEPS_PATH=/tmp/aenv-serial-retention-tests/deps passed: 839 agent tests, 4 capability tests, 3 linux-cap tests, 8 ublk tests, 46 daemon tests, and both shell verification scripts make test-agent-integration PROFILE=debug AENV_TEST_STATE_DIR=/tmp/aenv-serial-retention-tests AENV_TEST_DEPS_PATH=/tmp/aenv-serial-retention-tests/deps passed: 18 VM integration tests, 2 orchestrator tests, 5 snapshot OSS tests ``` All builds and Rust tests run as the non-root workspace user. Test state and dependencies are isolated under `/tmp/aenv-serial-retention-tests`; privileged tests use the repository capability runner. Skipped checks and reasons: - Go tests and API regeneration: no Go, schema, or generated source changes. - Broader envd/storage integration suites: not rerun; this change affects Firecracker log lifecycle. - Throughput benchmarks: no performance measurements are claimed. ## Risks and reviewer notes The main review area is `src/sandbox/firecracker/serial_logs.rs`: eligibility after confirmed exit, directory locking, and retention. Regression coverage includes late unlocked logger writes after abnormal drop, interrupted resume, cancellable lock contention, collection during repeated launches/stops, symlinks/custom entries, startup failure, retained history, and warm-log relocation. Unmarked logs need offline cleanup. Retention bounds eligible inactive-log lifetime; it does not limit active-log size, peak directory count, or compact an already expanded filesystem directory inode. Filesystem syscalls themselves cannot be interrupted by worker cancellation. ## 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:21:58 +00:00