[PR #383] [MERGED] refactor: break the module dependency cycles and enforce layering #392

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: refactor/module-layering

📝 Commits (4)

4121cec style(image): satisfy rustfmt and clippy in is_valid_digest_algorithm
902ff5a refactor: break the dependency cycles between top-level modules
86fd03f test: fail on dependency cycles between top-level modules
dc3b617 fix(repository): back off when an OSS document changes while being read

📊 Changes

70 files changed (+1858 additions, -1588 deletions)

View changed files

📝 .github/actions/sandbox-ci-setup/action.yml (+2 -2)
📝 CLAUDE.md (+3 -0)
📝 crates/e2e-tests/tests/snapshot_oss_e2e_test.rs (+6 -6)
📝 deploy/docker/Dockerfile.agentenv (+1 -1)
📝 services/shared/observability/buckets.go (+1 -1)
📝 src/api/impls/sandbox.rs (+3 -2)
📝 src/api/proxy.rs (+1 -1)
📝 src/api/server.rs (+1 -1)
📝 src/cfg.rs (+14 -6)
📝 src/deps/mod.rs (+18 -13)
📝 src/deps/overlaybd_tools.rs (+3 -3)
📝 src/image/buildkit/history.rs (+21 -26)
📝 src/image/cache/mock.rs (+1 -1)
📝 src/image/cache/store.rs (+1 -1)
📝 src/image/oci_image.rs (+1 -1)
📝 src/image/resolver.rs (+2 -2)
📝 src/lib.rs (+4 -0)
📝 src/logging.rs (+181 -4)
➕ src/module_cycles.rs (+210 -0)
📝 src/observability/mod.rs (+0 -1)

...and 50 more files

📄 Description

What

Remove every dependency cycle between the top-level modules of the agentenv crate, then add a test that keeps them layered. #382 already took repository off snapshot, image, volume and sandbox. This PR does the rest in four commits:

style(image): the rustfmt/clippy fix main needs (item 8 below).
refactor: breaks the cycles (items 1–6).
test: the module-cycle test (item 7).
fix(repository): the OSS document-read race that failed CI (item 9).

The cycles it breaks:

cfg: P2pTransportKind and MAX_VOLUME_MOUNTS move into cfg, which no longer imports p2p or volume.
logging: the tracing layer that copies build_id-span events into build logs moves from template to logging, and so does BuildLogger, which now writes to a BuildLogSink. Builds register that sink instead. template::logs re-exports BuildLogger; the buffer, size limits and repository store stay in template.
observability: the Prometheus helpers (MetricGuard, SandboxStageTimer, HTTP middleware) move unchanged to crate::prometheus. repository, image and the ublk manager no longer reach the orchestrator through observability.
image: RuntimeArtifactSet moves to types. The BuildKit history client stays in image::buildkit and takes its BuildLogger from logging, so image no longer depends on template and code in image can still drive BuildKit builds. Its reconnect test records log lines with an in-memory sink instead of a repository-backed BuildLogs session.
setup: the asset installer (setup/deps.rs, setup/overlaybd.rs) moves unchanged to a new crate::deps. Tools-image resolution becomes sandbox::resolve_tools_image, which passes ImageResolver to deps::prepare_tools_image.
snapshot → sandbox, volume: snapshot becomes the lower layer. ExtraDrive, CustomExtensionParams and VolumeMode move to types, and SandboxSnapshotManifest moves into snapshot. Layer compaction moves to crate::overlaybd. publish_captured and the runtime-version probe move to sandbox.
test: src/module_cycles.rs builds the module graph from lib.rs and every crate:: path, grouped use crate::{..} import and doc link, then fails on any cycle. It names each edge of the cycle and the file that creates it. No module list is kept, so adding, renaming or removing a module needs no test change. It replaces the repository-only test. It reads mod declarations with any visibility (pub, pub(crate), pub(in ..)).
style(image): fixes the rustfmt/clippy failure in is_valid_digest_algorithm that came in with d243f6e on main.
fix(repository): concurrent_document_updates_on_oss_lose_none failed in CI with object changed too often while reading its version. get_bytes_with_etag retried its stat plus If-Match read five times in a row, with no wait, so 16 writers on one record could make every attempt lose. It now reads once and reports the race. update_if_unchanged then retries with its existing jittered backoff, up to 32 attempts.

Resulting module graph, bottom to top (for review; the test does not hard-code it):

digest local_store logging managed_secret privileges prometheus proto types virtualization
cfg
api_key deps identity
p2p
image repository
overlaybd
snapshot
sandbox
setup template volume
orchestrator
observability
api

Why

Before this PR the crate's modules formed one strongly connected component of 14 modules. A few edges caused most of it: cfg → p2p/volume, logging → template, and repository/image/sandbox → observability → orchestrator. The sandbox ↔ snapshot and setup ↔ image/sandbox cycles were real design ones. As a result, no module could be read, tested or changed in isolation, and nothing kept new back-edges out.

None. This follows up on the code-quality review that produced #380 and #382.

Scope and non-goals

Moves and dependency inversions only, except for item 9 and two review fixes. In the refactor, one signature changes: BuildLogSink::log gains a step argument (the tracing layer passes None), and the buffer-append code from BuildLogger::log moves unchanged into the Mutex<Buffer> sink impl. The rest is moved code or call sites updated to new paths.
Review fixes: route_build_logs drops dead sinks when it registers one, so an aborted build can't leave its entry behind. The compaction tests now look for the UUID-suffixed temp file that compaction actually writes; before, they checked a path it never creates.
Public paths are kept where the rest of the crate uses them. sandbox re-exports ExtraDrive, RuntimeArtifactSet, CustomExtensionParams, SandboxSnapshotManifest and FIRECRACKER_BACKEND, and volume re-exports VolumeMode.
Not included: splitting modules into crates, and the orchestrator lifecycle refactor.

Design and behavior changes

The intended behavior change is none. Points worth a look:

sandbox ↔ snapshot: the dependency now runs one way, from sandbox to snapshot. Snapshot types are data that sandbox backends consume. Operations that drive a sandbox move up into sandbox: publish_captured, which downcasts to Firecracker artifacts and starts the startup-pack recording VM, and probe_runtime_versions, which runs commands in the guest. sandbox::publish_captured(&manager, metadata, captured) keeps the old argument order and still holds the capture until publication finishes.
logging: stop_build_log_routing compares weak pointers. As before, it leaves a newer sink registered under the same build id in place.
setup: deps::prepare_tools_image keeps the locking, legacy-drive and install logic of the old resolve_tools_image. Only the image lookup is now passed in by the caller.
Tracing: the publish_captured span is kept and now wraps a nested publish span.

Compatibility and operations

Public API or generated protocol: no HTTP or protobuf change. Rust library paths changed for setup::overlaybd, observability::prometheus, sandbox::manifest, SnapshotManager::publish_captured and SnapshotRuntimeVersions::probe. All in-repo callers are updated.
Configuration or defaults: N/A, no key or default changes.
Snapshot manifest, artifact layout, or storage format: N/A, the manifest type moved without serde changes.
Upgrade and rollback: N/A.
Host requirements, permissions, ports, or dependencies: N/A. The CI sandbox cache key (.github/actions/sandbox-ci-setup/action.yml) now hashes src/deps/*, so the first run after merge rebuilds that cache once.

Validation

make fmt
make clippy
make test-unit
Relevant Rust integration tests
make -C services test (one Go comment changed)
Generated clients/server regenerated with the documented make target (no schema changes)
Documentation updated (CLAUDE.md layering rule, Dockerfile comment, Go comment path)
Benchmarks or performance comparison completed

Commands and results:

make fmt                     # ok
make clippy                  # ok (workspace, all targets/features, -D warnings)
make test-unit               # ok: agentenv lib 987 passed, 4 ignored (+ envd, linux-cap, aenv,
                             #     ublk crates, install-script checks)
cargo test -p agentenv --lib module_cycles   # 3 passed; fails as expected on an injected
                                             # sandbox -> orchestrator doc link, naming both edges
make test-agent-integration  # KVM + ublk, isolated AENV_TEST_STATE_DIR / AENV_TEST_DEPS_PATH:
                             #   integration 21 passed, 1 ignored
                             #   orchestrator_integration 2 passed
                             #   (includes publish_captured, probe_runtime_versions,
                             #    OCI tools drive resolution, attached drives, volumes)
make -C services test        # ok
(cd services && go test ./...) && make -C services fmt-check vet   # ok

After squashing and the review fixes:

make fmt && make clippy      # ok
cargo test -p agentenv --lib -- module_cycles logging template::logs overlaybd::compact repository::backends::oss
                             # 29 passed
cargo test -p agentenv-e2e-tests --test snapshot_oss_e2e_test -- --ignored   # 9 passed (MinIO)
# concurrent_document_updates_on_oss_lose_none, test pinned to one CPU with taskset -c 0:
#   before the fix: 1 of 15 runs failed with the CI error
#   after the fix:  30 of 30 passed

After keeping the BuildKit history client in image:

make fmt && make clippy      # ok
cargo test -p agentenv --lib -- module_cycles logging template::logs image::buildkit api::impls::image_build
                             # 35 passed
make test-unit               # ok: agentenv lib 988 passed, 4 ignored

Skipped checks and reasons:

make test-agent-integration was not rerun after the squash or the BuildKit history change. Those changes touch build logging, the OSS document read and the compaction tests, not VM or storage paths.

Risks and reviewer notes

Commit 6 is the large one, but almost all of it is moves. git show --color-moved makes that easy to confirm. The moved code itself is unchanged; the edits are in sandbox/backend.rs (publish_captured), sandbox/runtime_versions.rs and the types/extra_drive.rs split.
Commit 2 is the only one that touches runtime routing (build logs). template::logs tests cover routing, truncation and session cleanup.
module_cycles.rs scans source text, comments included. Doc links create real rustdoc dependencies, so counting them is intended, and a false positive fails loudly instead of hiding a cycle. It does not follow super::super:: paths across top-level modules, and none exist today.

Checklist

The PR contains one coherent change and no unrelated formatting or refactoring. (The oci_image.rs style commit is the exception; make fmt and make clippy fail on main without it.)
New behavior is covered by tests, or I explained why testing is impractical. (No new behavior. The layering test covers the new rule.)
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/383 **Author:** [@yingdi-shan](https://github.com/yingdi-shan) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/14/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `refactor/module-layering` --- ### 📝 Commits (4) - [`4121cec`](https://github.com/kvcache-ai/AgentENV/commit/4121cecbb1f33b2fe8d7caee8e8d5bf92a287095) style(image): satisfy rustfmt and clippy in is_valid_digest_algorithm - [`902ff5a`](https://github.com/kvcache-ai/AgentENV/commit/902ff5a67ae04a604e0c8359dcd3b72510e27e25) refactor: break the dependency cycles between top-level modules - [`86fd03f`](https://github.com/kvcache-ai/AgentENV/commit/86fd03f33fedc085c43370e25696fae2a3ee82bd) test: fail on dependency cycles between top-level modules - [`dc3b617`](https://github.com/kvcache-ai/AgentENV/commit/dc3b617d5a50380e875e24ca9e90c32d402b096d) fix(repository): back off when an OSS document changes while being read ### 📊 Changes **70 files changed** (+1858 additions, -1588 deletions) <details> <summary>View changed files</summary> 📝 `.github/actions/sandbox-ci-setup/action.yml` (+2 -2) 📝 `CLAUDE.md` (+3 -0) 📝 `crates/e2e-tests/tests/snapshot_oss_e2e_test.rs` (+6 -6) 📝 `deploy/docker/Dockerfile.agentenv` (+1 -1) 📝 `services/shared/observability/buckets.go` (+1 -1) 📝 `src/api/impls/sandbox.rs` (+3 -2) 📝 `src/api/proxy.rs` (+1 -1) 📝 `src/api/server.rs` (+1 -1) 📝 `src/cfg.rs` (+14 -6) 📝 `src/deps/mod.rs` (+18 -13) 📝 `src/deps/overlaybd_tools.rs` (+3 -3) 📝 `src/image/buildkit/history.rs` (+21 -26) 📝 `src/image/cache/mock.rs` (+1 -1) 📝 `src/image/cache/store.rs` (+1 -1) 📝 `src/image/oci_image.rs` (+1 -1) 📝 `src/image/resolver.rs` (+2 -2) 📝 `src/lib.rs` (+4 -0) 📝 `src/logging.rs` (+181 -4) ➕ `src/module_cycles.rs` (+210 -0) 📝 `src/observability/mod.rs` (+0 -1) _...and 50 more files_ </details> ### 📄 Description ## What Remove every dependency cycle between the top-level modules of the `agentenv` crate, then add a test that keeps them layered. #382 already took `repository` off `snapshot`, `image`, `volume` and `sandbox`. This PR does the rest in four commits: - `style(image)`: the rustfmt/clippy fix `main` needs (item 8 below). - `refactor`: breaks the cycles (items 1–6). - `test`: the module-cycle test (item 7). - `fix(repository)`: the OSS document-read race that failed CI (item 9). The cycles it breaks: 1. **cfg**: `P2pTransportKind` and `MAX_VOLUME_MOUNTS` move into `cfg`, which no longer imports `p2p` or `volume`. 2. **logging**: the tracing layer that copies `build_id`-span events into build logs moves from `template` to `logging`, and so does `BuildLogger`, which now writes to a `BuildLogSink`. Builds register that sink instead. `template::logs` re-exports `BuildLogger`; the buffer, size limits and repository store stay in `template`. 3. **observability**: the Prometheus helpers (`MetricGuard`, `SandboxStageTimer`, HTTP middleware) move unchanged to `crate::prometheus`. `repository`, `image` and the ublk manager no longer reach the orchestrator through `observability`. 4. **image**: `RuntimeArtifactSet` moves to `types`. The BuildKit history client stays in `image::buildkit` and takes its `BuildLogger` from `logging`, so `image` no longer depends on `template` and code in `image` can still drive BuildKit builds. Its reconnect test records log lines with an in-memory sink instead of a repository-backed `BuildLogs` session. 5. **setup**: the asset installer (`setup/deps.rs`, `setup/overlaybd.rs`) moves unchanged to a new `crate::deps`. Tools-image resolution becomes `sandbox::resolve_tools_image`, which passes `ImageResolver` to `deps::prepare_tools_image`. 6. **snapshot → sandbox, volume**: `snapshot` becomes the lower layer. `ExtraDrive`, `CustomExtensionParams` and `VolumeMode` move to `types`, and `SandboxSnapshotManifest` moves into `snapshot`. Layer compaction moves to `crate::overlaybd`. `publish_captured` and the runtime-version probe move to `sandbox`. 7. **test**: `src/module_cycles.rs` builds the module graph from `lib.rs` and every `crate::` path, grouped `use crate::{..}` import and doc link, then fails on any cycle. It names each edge of the cycle and the file that creates it. No module list is kept, so adding, renaming or removing a module needs no test change. It replaces the repository-only test. It reads `mod` declarations with any visibility (`pub`, `pub(crate)`, `pub(in ..)`). 8. **style(image)**: fixes the rustfmt/clippy failure in `is_valid_digest_algorithm` that came in with d243f6e on `main`. 9. **fix(repository)**: `concurrent_document_updates_on_oss_lose_none` failed in [CI](https://github.com/kvcache-ai/AgentENV/actions/runs/38515986051/attempts/1) with `object changed too often while reading its version`. `get_bytes_with_etag` retried its stat plus `If-Match` read five times in a row, with no wait, so 16 writers on one record could make every attempt lose. It now reads once and reports the race. `update_if_unchanged` then retries with its existing jittered backoff, up to 32 attempts. Resulting module graph, bottom to top (for review; the test does not hard-code it): ```text digest local_store logging managed_secret privileges prometheus proto types virtualization cfg api_key deps identity p2p image repository overlaybd snapshot sandbox setup template volume orchestrator observability api ``` ## Why Before this PR the crate's modules formed one strongly connected component of 14 modules. A few edges caused most of it: `cfg → p2p/volume`, `logging → template`, and `repository/image/sandbox → observability → orchestrator`. The `sandbox ↔ snapshot` and `setup ↔ image/sandbox` cycles were real design ones. As a result, no module could be read, tested or changed in isolation, and nothing kept new back-edges out. ## Related issue None. This follows up on the code-quality review that produced #380 and #382. ## Scope and non-goals - Moves and dependency inversions only, except for item 9 and two review fixes. In the refactor, one signature changes: `BuildLogSink::log` gains a `step` argument (the tracing layer passes `None`), and the buffer-append code from `BuildLogger::log` moves unchanged into the `Mutex<Buffer>` sink impl. The rest is moved code or call sites updated to new paths. - Review fixes: `route_build_logs` drops dead sinks when it registers one, so an aborted build can't leave its entry behind. The compaction tests now look for the UUID-suffixed temp file that compaction actually writes; before, they checked a path it never creates. - Public paths are kept where the rest of the crate uses them. `sandbox` re-exports `ExtraDrive`, `RuntimeArtifactSet`, `CustomExtensionParams`, `SandboxSnapshotManifest` and `FIRECRACKER_BACKEND`, and `volume` re-exports `VolumeMode`. - Not included: splitting modules into crates, and the orchestrator lifecycle refactor. ## Design and behavior changes The intended behavior change is none. Points worth a look: - **sandbox ↔ snapshot**: the dependency now runs one way, from sandbox to snapshot. Snapshot types are data that sandbox backends consume. Operations that drive a sandbox move up into `sandbox`: `publish_captured`, which downcasts to Firecracker artifacts and starts the startup-pack recording VM, and `probe_runtime_versions`, which runs commands in the guest. `sandbox::publish_captured(&manager, metadata, captured)` keeps the old argument order and still holds the capture until publication finishes. - **logging**: `stop_build_log_routing` compares weak pointers. As before, it leaves a newer sink registered under the same build id in place. - **setup**: `deps::prepare_tools_image` keeps the locking, legacy-drive and install logic of the old `resolve_tools_image`. Only the image lookup is now passed in by the caller. - Tracing: the `publish_captured` span is kept and now wraps a nested `publish` span. ## Compatibility and operations - Public API or generated protocol: no HTTP or protobuf change. Rust library paths changed for `setup::overlaybd`, `observability::prometheus`, `sandbox::manifest`, `SnapshotManager::publish_captured` and `SnapshotRuntimeVersions::probe`. All in-repo callers are updated. - Configuration or defaults: N/A, no key or default changes. - Snapshot manifest, artifact layout, or storage format: N/A, the manifest type moved without serde changes. - Upgrade and rollback: N/A. - Host requirements, permissions, ports, or dependencies: N/A. The CI sandbox cache key (`.github/actions/sandbox-ci-setup/action.yml`) now hashes `src/deps/*`, so the first run after merge rebuilds that cache once. ## Validation - [x] `make fmt` - [x] `make clippy` - [x] `make test-unit` - [x] Relevant Rust integration tests - [x] `make -C services test` (one Go comment changed) - [ ] Generated clients/server regenerated with the documented `make` target (no schema changes) - [x] Documentation updated (CLAUDE.md layering rule, Dockerfile comment, Go comment path) - [ ] Benchmarks or performance comparison completed Commands and results: ```text make fmt # ok make clippy # ok (workspace, all targets/features, -D warnings) make test-unit # ok: agentenv lib 987 passed, 4 ignored (+ envd, linux-cap, aenv, # ublk crates, install-script checks) cargo test -p agentenv --lib module_cycles # 3 passed; fails as expected on an injected # sandbox -> orchestrator doc link, naming both edges make test-agent-integration # KVM + ublk, isolated AENV_TEST_STATE_DIR / AENV_TEST_DEPS_PATH: # integration 21 passed, 1 ignored # orchestrator_integration 2 passed # (includes publish_captured, probe_runtime_versions, # OCI tools drive resolution, attached drives, volumes) make -C services test # ok (cd services && go test ./...) && make -C services fmt-check vet # ok ``` After squashing and the review fixes: ```text make fmt && make clippy # ok cargo test -p agentenv --lib -- module_cycles logging template::logs overlaybd::compact repository::backends::oss # 29 passed cargo test -p agentenv-e2e-tests --test snapshot_oss_e2e_test -- --ignored # 9 passed (MinIO) # concurrent_document_updates_on_oss_lose_none, test pinned to one CPU with taskset -c 0: # before the fix: 1 of 15 runs failed with the CI error # after the fix: 30 of 30 passed ``` After keeping the BuildKit history client in `image`: ```text make fmt && make clippy # ok cargo test -p agentenv --lib -- module_cycles logging template::logs image::buildkit api::impls::image_build # 35 passed make test-unit # ok: agentenv lib 988 passed, 4 ignored ``` Skipped checks and reasons: - `make test-agent-integration` was not rerun after the squash or the BuildKit history change. Those changes touch build logging, the OSS document read and the compaction tests, not VM or storage paths. ## Risks and reviewer notes - Commit 6 is the large one, but almost all of it is moves. `git show --color-moved` makes that easy to confirm. The moved code itself is unchanged; the edits are in `sandbox/backend.rs` (`publish_captured`), `sandbox/runtime_versions.rs` and the `types/extra_drive.rs` split. - Commit 2 is the only one that touches runtime routing (build logs). `template::logs` tests cover routing, truncation and session cleanup. - `module_cycles.rs` scans source text, comments included. Doc links create real rustdoc dependencies, so counting them is intended, and a false positive fails loudly instead of hiding a cycle. It does not follow `super::super::` paths across top-level modules, and none exist today. ## Checklist - [x] The PR contains one coherent change and no unrelated formatting or refactoring. (The `oci_image.rs` style commit is the exception; `make fmt` and `make clippy` fail on `main` without it.) - [x] New behavior is covered by tests, or I explained why testing is impractical. (No new behavior. The layering test covers the new rule.) - [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:55 +00:00