[PR #185] [MERGED] fix(snapshot): revert digest tracking on compaction output paths #234

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/185
Author: @huajq
Created: 8/18/2026
Status: ✅ Merged
Merged: 8/18/2026
Merged by: @yingdi-shan

Base: main ← Head: fix/snapshot-compaction-digest-revert

📝 Commits (1)

ecdd8a9 fix(snapshot): revert digest tracking on compaction output paths

📊 Changes

6 files changed (+23 additions, -231 deletions)

View changed files

📝 src/sandbox/firecracker/overlaybd_snapshot.rs (+10 -17)
📝 src/sandbox/firecracker/sandbox.rs (+2 -3)
📝 src/sandbox/ublk/mod.rs (+1 -1)
📝 src/sandbox/ublk/overlaybd.rs (+10 -37)
➖ storage/overlaybd/src/lsmt/file/digest_tracking.rs (+0 -171)
📝 storage/overlaybd/src/lsmt/file/mod.rs (+0 -2)

📄 Description

What

Revert the digest-tracking half of 5afd9d1 (PR #177): compact_layers and publish_memory_overlaybd_layer go back to plain create_commit_args, and DigestTrackingFile / OrderedWriter are removed. The OSS multipart upload parallelization from the same commit (64 MiB parts × 8 concurrent uploads) is kept — it is unrelated to the regression.

Why

5afd9d1 wrapped every compact_to output in DigestTrackingFile + OrderedWriter so OSS/ACR publish could skip re-hashing sealed layers. On all snapshot packing paths this:

forces ordered writes, dropping compaction write concurrency from 32 to 1 on the raw output path (the default for memory snapshots, and the only mode for rootfs lower rewriting), and
adds synchronous software SHA256 over every written byte on the capture hot path.

The produced descriptor is only consumed by the OSS repository backend, the ACR source-registry publisher, and (opportunistically) P2P content-addressed layer advertisement. posix_fs and local-only snapshots pay the cost with zero benefit. Even for OSS the trade is negative in the standard flow: publish runs in the same synchronous request as capture with a warm page cache, so the saved re-read costs nothing while the serial-write penalty is real; inherited layers already carry descriptors from the parent manifest via OSS runtime resolution, so there is no per-publish re-hash multiplier either. On aarch64 hosts without SHA-256 hardware acceleration the added snapshot-creation latency is significant (#184).

Closes #184

Scope and non-goals

Included: revert of the descriptor plumbing (compact_layers, publish_memory_overlaybd_layer, build_mem_snapshot_image_config and their callers) and removal of DigestTrackingFile / OrderedWriter.
Excluded: OSS multipart upload parallelization from 5afd9d1 (kept). No changes to publish-side consumers — they already handle descriptorless layers.
A possible future re-approach (config-gated descriptor, or post-write warm-cache hashing that preserves write concurrency) is intentionally not in scope.

Design and behavior changes

Snapshot packing returns to pre-5afd9d1 behavior: raw-mode compaction writes run at full concurrency (32) and no content digest is computed during capture. Layers staged without descriptors flow through the existing publish fallbacks: OSS import_managed_layer_by_hash (or dense export for sparse layers), ACR raw/dense fallback, P2P uuid-keyed layer artifacts. The close_seal + restack descriptor path (ublk-daemon / image layer) is pre-existing and untouched.

Compatibility and operations

Public API or generated protocol: N/A — no API or protocol change.
Configuration or defaults: N/A.
Snapshot manifest, artifact layout, or storage format: staged image.json lowers no longer carry digest/size for newly compacted layers; all consumers already accept descriptorless layers (this was the pre-5afd9d1 format).
Upgrade and rollback: no migration needed; snapshots committed with descriptors remain valid.

Validation

make fmt (cargo fmt --all -- --check)
make clippy (cargo clippy -p agentenv -p overlaybd --all-targets -- -D warnings)
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 clippy -p agentenv -p overlaybd --all-targets -- -D warnings
  -> Finished, no warnings

cargo test -p agentenv --lib sandbox::ublk::overlaybd
  -> 1 passed (compact_layers_mixed_input_and_configured_output)

cargo test -p agentenv --lib sandbox::firecracker::overlaybd_snapshot
  -> 9 passed

cargo test -p overlaybd --lib compact
  -> 6 passed

make test-unit
  -> (see PR checks / comment)

Skipped checks and reasons:

Integration tests / benchmarks: no runtime behavior change beyond restoring the pre-5afd9d1 code path that integration tests already covered before PR #177; a capture-latency benchmark on aarch64 without SHA-256 extensions would be valuable follow-up confirmation of #184.
make -C services test: services/ untouched.
Codegen / docs: no generated code or documented behavior changes.

Risks and reviewer notes

OSS/ACR publish of newly compacted layers re-hashes them once at publish time (pre-5afd9d1 behavior, warm page cache); this is the deliberate trade.
Review focus: src/sandbox/ublk/overlaybd.rs and src/sandbox/firecracker/overlaybd_snapshot.rs — confirm the revert is faithful and src/snapshot/repository/backends/oss/client.rs is untouched.

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/185 **Author:** [@huajq](https://github.com/huajq) **Created:** 8/18/2026 **Status:** ✅ Merged **Merged:** 8/18/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `fix/snapshot-compaction-digest-revert` --- ### 📝 Commits (1) - [`ecdd8a9`](https://github.com/kvcache-ai/AgentENV/commit/ecdd8a977e000b8eb9b2f9d8e6c9ad0e3315d04e) fix(snapshot): revert digest tracking on compaction output paths ### 📊 Changes **6 files changed** (+23 additions, -231 deletions) <details> <summary>View changed files</summary> 📝 `src/sandbox/firecracker/overlaybd_snapshot.rs` (+10 -17) 📝 `src/sandbox/firecracker/sandbox.rs` (+2 -3) 📝 `src/sandbox/ublk/mod.rs` (+1 -1) 📝 `src/sandbox/ublk/overlaybd.rs` (+10 -37) ➖ `storage/overlaybd/src/lsmt/file/digest_tracking.rs` (+0 -171) 📝 `storage/overlaybd/src/lsmt/file/mod.rs` (+0 -2) </details> ### 📄 Description ## What Revert the digest-tracking half of 5afd9d1 (PR #177): `compact_layers` and `publish_memory_overlaybd_layer` go back to plain `create_commit_args`, and `DigestTrackingFile` / `OrderedWriter` are removed. The OSS multipart upload parallelization from the same commit (64 MiB parts × 8 concurrent uploads) is kept — it is unrelated to the regression. ## Why 5afd9d1 wrapped every `compact_to` output in `DigestTrackingFile` + `OrderedWriter` so OSS/ACR publish could skip re-hashing sealed layers. On **all** snapshot packing paths this: - forces ordered writes, dropping compaction write concurrency from 32 to 1 on the raw output path (the default for memory snapshots, and the only mode for rootfs lower rewriting), and - adds synchronous software SHA256 over every written byte on the capture hot path. The produced descriptor is only consumed by the OSS repository backend, the ACR source-registry publisher, and (opportunistically) P2P content-addressed layer advertisement. `posix_fs` and local-only snapshots pay the cost with zero benefit. Even for OSS the trade is negative in the standard flow: publish runs in the same synchronous request as capture with a warm page cache, so the saved re-read costs nothing while the serial-write penalty is real; inherited layers already carry descriptors from the parent manifest via OSS runtime resolution, so there is no per-publish re-hash multiplier either. On aarch64 hosts without SHA-256 hardware acceleration the added snapshot-creation latency is significant (#184). ## Related issue Closes #184 ## Scope and non-goals - Included: revert of the descriptor plumbing (`compact_layers`, `publish_memory_overlaybd_layer`, `build_mem_snapshot_image_config` and their callers) and removal of `DigestTrackingFile` / `OrderedWriter`. - Excluded: OSS multipart upload parallelization from 5afd9d1 (kept). No changes to publish-side consumers — they already handle descriptorless layers. - A possible future re-approach (config-gated descriptor, or post-write warm-cache hashing that preserves write concurrency) is intentionally not in scope. ## Design and behavior changes Snapshot packing returns to pre-5afd9d1 behavior: raw-mode compaction writes run at full concurrency (32) and no content digest is computed during capture. Layers staged without descriptors flow through the existing publish fallbacks: OSS `import_managed_layer_by_hash` (or dense export for sparse layers), ACR raw/dense fallback, P2P uuid-keyed layer artifacts. The `close_seal + restack` descriptor path (ublk-daemon / image layer) is pre-existing and untouched. ## Compatibility and operations - Public API or generated protocol: N/A — no API or protocol change. - Configuration or defaults: N/A. - Snapshot manifest, artifact layout, or storage format: staged `image.json` lowers no longer carry digest/size for newly compacted layers; all consumers already accept descriptorless layers (this was the pre-5afd9d1 format). - Upgrade and rollback: no migration needed; snapshots committed with descriptors remain valid. ## Validation - [x] `make fmt` (`cargo fmt --all -- --check`) - [x] `make clippy` (`cargo clippy -p agentenv -p overlaybd --all-targets -- -D warnings`) - [x] `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 cargo clippy -p agentenv -p overlaybd --all-targets -- -D warnings -> Finished, no warnings cargo test -p agentenv --lib sandbox::ublk::overlaybd -> 1 passed (compact_layers_mixed_input_and_configured_output) cargo test -p agentenv --lib sandbox::firecracker::overlaybd_snapshot -> 9 passed cargo test -p overlaybd --lib compact -> 6 passed make test-unit -> (see PR checks / comment) ``` Skipped checks and reasons: - Integration tests / benchmarks: no runtime behavior change beyond restoring the pre-5afd9d1 code path that integration tests already covered before PR #177; a capture-latency benchmark on aarch64 without SHA-256 extensions would be valuable follow-up confirmation of #184. - `make -C services test`: `services/` untouched. - Codegen / docs: no generated code or documented behavior changes. ## Risks and reviewer notes - OSS/ACR publish of newly compacted layers re-hashes them once at publish time (pre-5afd9d1 behavior, warm page cache); this is the deliberate trade. - Review focus: `src/sandbox/ublk/overlaybd.rs` and `src/sandbox/firecracker/overlaybd_snapshot.rs` — confirm the revert is faithful and `src/snapshot/repository/backends/oss/client.rs` is untouched. ## 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:19 +00:00