kvcache-ai/AgentENV

Fork 0

mirror of https://github.com/kvcache-ai/AgentENV.git synced 2026-11-05 01:17:36 +00:00

[PR #261] fix(sandbox): shard the persistent serial log directory #298

New issue

Open

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/261
Author: @makhov
Created: 9/7/2026
Status: 🔄 Open

Base: main ← Head: fix/shard-serial-log-dir

📝 Commits (2)

53e4061 fix(sandbox): shard the persistent serial log directory
211c703 fix(sandbox): shard the persistent serial log directory. review fixes

📊 Changes

7 files changed (+68 additions, -9 deletions)

View changed files

📝 docs/src/configuration/env-vars.md (+1 -1)
📝 docs/src/configuration/reference.md (+1 -1)
📝 docs/src/internals/persistence-artifact-inventory.md (+2 -2)
📝 docs/src/internals/sandbox-testing.md (+3 -2)
📝 src/cfg.rs (+2 -1)
📝 src/sandbox/firecracker/config.rs (+2 -1)
📝 src/sandbox/firecracker/sandbox.rs (+57 -1)

📄 Description

What

Insert one shard directory level between the persistent serial-output root and each sandbox's own log directory, so the layout becomes {serial_dir}/{shard}/{sandbox_id}/, where the shard is the last two hex digits of the sandbox id.

Why

Serial output is deliberately persistent: {serial_dir}/{sandbox_id}/ outlives the sandbox so its boot log can be read after the fact. Nothing prunes it, so
on a busy node the root accumulates one child per sandbox ever created, at ~470 entries/sec under sustained create load. Nearly every child holds a single zero-byte firecracker-stderr.log — a guest resumed from a snapshot writes nothing to the console.

That cost is paid by every create, not by whoever reads a log: each create links a new entry into the root, under an exclusive lock on the parent inode.
At ~594k entries a node sustained 197–280 creates/sec; with the root emptied and nothing else changed, the same node sustained 460–475 creates/sec at 76–78% CPU — about 1.9× on create throughput, on a node that had simply been up a while. The inverse correlation between root size and peak create rate held across all four nodes measured.

The effect is invisible on a fresh node and grows silently with uptime, which is why it seems worth fixing in the layout rather than in an operator runbook.

Closes #260

Scope and non-goals

Included:

the path computation in FirecrackerSandbox::default_log_dir;
a serial_shard helper with the rationale for the shard key;
unit tests for the shard key, its spread across ids minted together, and the
nil-id edge case;
the two doc comments that stated the old layout.

Non-goals, deliberately:

no pruning, rotation, or retention policy. This bounds the fan-out of the root, not the total number of directories. Retention is a separate discussion, and this change does not preclude one.
no migration or cleanup of existing directories. A node that ran unsharded keeps its flat root; the change stops it regrowing. Operators who want the old entries gone still remove them once, and that is best done after deploying this — before it, the root just refills.
no change to when serial files are created, what is written, or where the root itself lives.

Design and behavior changes

default_log_dir gains one join(serial_shard(self.id)) before the per-sandbox component. All three writers of that path — firecracker_stdout_path, firecracker_stderr_path, and firecracker_log_path — already route through default_log_dir, so one edit covers every producer, and the directory is created by the same code that created it before.

The shard is the id tail, not its head. Sandbox ids are UUIDv7, whose leading bytes are a millisecond timestamp: every sandbox created in the same period shares a prefix, so head-sharding would put an entire burst — exactly the load that causes the problem — into one bucket. The trailing bytes are random. Minting 411,340 ids inside a single millisecond and sharding them both ways gives 256 buckets of at most ~1.7k entries on the tail, against one bucket holding all 411,340 on the head. serial_shard_spreads_ids_created_together is
the regression test for that property.

Two hex digits (256 buckets) is sized for the observed regime: it holds the measured ~600k directories at a few thousand children each, well inside the range where directory insertion is not the bottleneck.

Compatibility and operations

Public API or generated protocol: N/A — the path is internal and is not reported over the API.
Configuration or defaults: no key added, removed, or re-interpreted. serial_dir / AENV_FIRECRACKER_SERIAL_DIR keep their meaning; only the layout beneath the root changes. Explicit stdout_path / stderr_path overrides bypass this path entirely and are unaffected.
Snapshot manifest, artifact layout, or storage format: the on-disk layout of the serial log directory changes. Nothing in the tree reads it back by reconstructing {serial_dir}/{sandbox_id} — every use of serial_output_base_dir and serial_dir is either this computation or config plumbing — so the change is self-contained in-tree. Out-of-tree tooling that globs one level deep is the one thing that breaks; see risks.
Host requirements, permissions, ports, or dependencies: none.

Validation

make fmt
make clippy — ran the narrower equivalent for the touched crate, see below
make test-unit — ran the touched tests only, 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
  (no output: clean)

$ cargo test -p agentenv --lib serial_shard
running 3 tests
test sandbox::firecracker::sandbox::tests::serial_shard_is_the_id_tail ... ok
test sandbox::firecracker::sandbox::tests::serial_shard_handles_the_nil_id ... ok
test sandbox::firecracker::sandbox::tests::serial_shard_spreads_ids_created_together ... ok
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 815 filtered out

$ cargo clippy -p agentenv --lib --all-features -- -D warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s)
  (no warnings)

rustc 1.98.1 (48a229cea 2026-09-01), rustfmt 1.9.0-stable
Run in the project's own build environment (rust:1-bookworm with clang,
libclang-dev, libprotobuf-dev, protobuf-compiler), aarch64-unknown-linux-gnu.

Skipped checks and reasons:

make clippy and make test-unit in full: not run. I ran clippy for the crate I touched (-p agentenv --lib --all-features -- -D warnings, clean) and the three new tests, rather than the whole workspace and the full unit-test target — the latter also drives uvm-ublk and the ignored capability tests, which want a configured ublk host and root. Happy to post a full run if you would rather see one before reviewing; CI will cover it in any case.
Integration tests: not run — they need root, /dev/kvm, and network namespaces, which I do not have available for this change. The touched code is a pure path computation with no privileged behavior, and the three writers that consume it are unchanged.
make -C services test: nothing under services/ changes.
Generated code: none touched.
Documentation: no prose docs describe this layout; the two doc comments that did are updated in the diff.
Benchmarks: the throughput numbers above are production measurements of the pathological state and of the same node with the root emptied, not a repeatable in-tree benchmark — reproducing the 1.9× requires ~600k directories first. The shard-key property is covered by a unit test instead.

Happy to run something more specific if you want it.

Risks and reviewer notes

The one real compatibility risk is out-of-tree tooling that walks {serial_dir}/{sandbox_id}/. Anything globbing one level deep silently finds nothing after this change rather than erroring. If you would rather this be opt-in behind a config key, or want it called out in release notes, I am happy to add either.
This spreads, it does not bound. The root's fan-out is fixed and each bucket now grows instead — 256× the headroom, but without retention the problem returns much further out. I think that is the right split (small, safe change now; retention is a policy decision), but if you would prefer them together, say so.
Shard width is a judgment call. 256 buckets fits the measured ~600k. If you expect an order of magnitude more per node, three digits is the safer pick — trivial to change, and the tests are width-agnostic apart from the length assertion.
serial_shard falls back to "00" for an id whose string form has fewer than two hex digits, which is unreachable for a parsed UUID; the alternative was a panic on a logging path.
Most interesting file: src/sandbox/firecracker/sandbox.rs — the one-line change in default_log_dir and the serial_shard helper below 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/261 **Author:** [@makhov](https://github.com/makhov) **Created:** 9/7/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/shard-serial-log-dir` --- ### 📝 Commits (2) - [`53e4061`](https://github.com/kvcache-ai/AgentENV/commit/53e4061bed1e8a6d0205550e20274a68311e1fe5) fix(sandbox): shard the persistent serial log directory - [`211c703`](https://github.com/kvcache-ai/AgentENV/commit/211c703d900b1ed6ecdd0f4fa2029bd10d6dac34) fix(sandbox): shard the persistent serial log directory. review fixes ### 📊 Changes **7 files changed** (+68 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `docs/src/configuration/env-vars.md` (+1 -1) 📝 `docs/src/configuration/reference.md` (+1 -1) 📝 `docs/src/internals/persistence-artifact-inventory.md` (+2 -2) 📝 `docs/src/internals/sandbox-testing.md` (+3 -2) 📝 `src/cfg.rs` (+2 -1) 📝 `src/sandbox/firecracker/config.rs` (+2 -1) 📝 `src/sandbox/firecracker/sandbox.rs` (+57 -1) </details> ### 📄 Description ## What Insert one shard directory level between the persistent serial-output root and each sandbox's own log directory, so the layout becomes `{serial_dir}/{shard}/{sandbox_id}/`, where the shard is the last two hex digits of the sandbox id. ## Why Serial output is deliberately persistent: `{serial_dir}/{sandbox_id}/` outlives the sandbox so its boot log can be read after the fact. Nothing prunes it, so on a busy node the root accumulates one child per sandbox ever created, at ~470 entries/sec under sustained create load. Nearly every child holds a single zero-byte `firecracker-stderr.log` — a guest resumed from a snapshot writes nothing to the console. That cost is paid by every create, not by whoever reads a log: each create links a new entry into the root, under an exclusive lock on the parent inode. At ~594k entries a node sustained 197–280 creates/sec; with the root emptied and nothing else changed, the same node sustained 460–475 creates/sec at 76–78% CPU — about **1.9×** on create throughput, on a node that had simply been up a while. The inverse correlation between root size and peak create rate held across all four nodes measured. The effect is invisible on a fresh node and grows silently with uptime, which is why it seems worth fixing in the layout rather than in an operator runbook. ## Related issue Closes #260 ## Scope and non-goals Included: - the path computation in `FirecrackerSandbox::default_log_dir`; - a `serial_shard` helper with the rationale for the shard key; - unit tests for the shard key, its spread across ids minted together, and the nil-id edge case; - the two doc comments that stated the old layout. Non-goals, deliberately: - **no pruning, rotation, or retention policy.** This bounds the fan-out of the root, not the total number of directories. Retention is a separate discussion, and this change does not preclude one. - **no migration or cleanup of existing directories.** A node that ran unsharded keeps its flat root; the change stops it regrowing. Operators who want the old entries gone still remove them once, and that is best done after deploying this — before it, the root just refills. - no change to when serial files are created, what is written, or where the root itself lives. ## Design and behavior changes `default_log_dir` gains one `join(serial_shard(self.id))` before the per-sandbox component. All three writers of that path — `firecracker_stdout_path`, `firecracker_stderr_path`, and `firecracker_log_path` — already route through `default_log_dir`, so one edit covers every producer, and the directory is created by the same code that created it before. **The shard is the id tail, not its head.** Sandbox ids are UUIDv7, whose leading bytes are a millisecond timestamp: every sandbox created in the same period shares a prefix, so head-sharding would put an entire burst — exactly the load that causes the problem — into one bucket. The trailing bytes are random. Minting 411,340 ids inside a single millisecond and sharding them both ways gives 256 buckets of at most ~1.7k entries on the tail, against one bucket holding all 411,340 on the head. `serial_shard_spreads_ids_created_together` is the regression test for that property. Two hex digits (256 buckets) is sized for the observed regime: it holds the measured ~600k directories at a few thousand children each, well inside the range where directory insertion is not the bottleneck. ## Compatibility and operations - **Public API or generated protocol:** N/A — the path is internal and is not reported over the API. - **Configuration or defaults:** no key added, removed, or re-interpreted. `serial_dir` / `AENV_FIRECRACKER_SERIAL_DIR` keep their meaning; only the layout beneath the root changes. Explicit `stdout_path` / `stderr_path` overrides bypass this path entirely and are unaffected. - **Snapshot manifest, artifact layout, or storage format:** the on-disk layout of the serial log directory changes. Nothing in the tree reads it back by reconstructing `{serial_dir}/{sandbox_id}` — every use of `serial_output_base_dir` and `serial_dir` is either this computation or config plumbing — so the change is self-contained in-tree. Out-of-tree tooling that globs one level deep is the one thing that breaks; see risks. - **Host requirements, permissions, ports, or dependencies:** none. ## Validation - [x] `make fmt` - [ ] `make clippy` — ran the narrower equivalent for the touched crate, see below - [ ] `make test-unit` — ran the touched tests only, 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 (no output: clean) $ cargo test -p agentenv --lib serial_shard running 3 tests test sandbox::firecracker::sandbox::tests::serial_shard_is_the_id_tail ... ok test sandbox::firecracker::sandbox::tests::serial_shard_handles_the_nil_id ... ok test sandbox::firecracker::sandbox::tests::serial_shard_spreads_ids_created_together ... ok test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 815 filtered out $ cargo clippy -p agentenv --lib --all-features -- -D warnings Finished `dev` profile [unoptimized + debuginfo] target(s) (no warnings) rustc 1.98.1 (48a229cea 2026-09-01), rustfmt 1.9.0-stable Run in the project's own build environment (rust:1-bookworm with clang, libclang-dev, libprotobuf-dev, protobuf-compiler), aarch64-unknown-linux-gnu. ``` Skipped checks and reasons: - **`make clippy` and `make test-unit` in full:** not run. I ran clippy for the crate I touched (`-p agentenv --lib --all-features -- -D warnings`, clean) and the three new tests, rather than the whole workspace and the full unit-test target — the latter also drives `uvm-ublk` and the ignored capability tests, which want a configured ublk host and root. Happy to post a full run if you would rather see one before reviewing; CI will cover it in any case. - **Integration tests:** not run — they need root, `/dev/kvm`, and network namespaces, which I do not have available for this change. The touched code is a pure path computation with no privileged behavior, and the three writers that consume it are unchanged. - **`make -C services test`:** nothing under `services/` changes. - **Generated code:** none touched. - **Documentation:** no prose docs describe this layout; the two doc comments that did are updated in the diff. - **Benchmarks:** the throughput numbers above are production measurements of the pathological state and of the same node with the root emptied, not a repeatable in-tree benchmark — reproducing the 1.9× requires ~600k directories first. The shard-key property is covered by a unit test instead. Happy to run something more specific if you want it. ## Risks and reviewer notes - **The one real compatibility risk** is out-of-tree tooling that walks `{serial_dir}/{sandbox_id}/`. Anything globbing one level deep silently finds nothing after this change rather than erroring. If you would rather this be opt-in behind a config key, or want it called out in release notes, I am happy to add either. - **This spreads, it does not bound.** The root's fan-out is fixed and each bucket now grows instead — 256× the headroom, but without retention the problem returns much further out. I think that is the right split (small, safe change now; retention is a policy decision), but if you would prefer them together, say so. - **Shard width is a judgment call.** 256 buckets fits the measured ~600k. If you expect an order of magnitude more per node, three digits is the safer pick — trivial to change, and the tests are width-agnostic apart from the length assertion. - `serial_shard` falls back to `"00"` for an id whose string form has fewer than two hex digits, which is unreachable for a parsed UUID; the alternative was a panic on a logging path. - Most interesting file: `src/sandbox/firecracker/sandbox.rs` — the one-line change in `default_log_dir` and the `serial_shard` helper below it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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

bsakaguchi referenced this issue

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

[PR #298] [MERGED] feat(snapshot): accelerate cold resume with startup first-touch manifests #327

bsakaguchi referenced this issue

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

[PR #314] [MERGED] feat(snapshot): add startup manifest prefetch for POSIX snapshots #338