kvcache-ai/AgentENV

Fork 0

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

[PR #369] [MERGED] refactor(volume): implement the volume catalog once on repository documents #382

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: refactor/volume-catalog

📝 Commits (2)

fad9afd fix(repository): create absent OSS objects with if_not_exists
06bfab9 refactor(volume): implement the volume catalog once on repository documents

📊 Changes

16 files changed (+1008 additions, -1126 deletions)

View changed files

📝 CLAUDE.md (+1 -1)
📝 crates/e2e-tests/tests/snapshot_oss_e2e_test.rs (+129 -1)
📝 src/api/impls/image_build/tests.rs (+1 -1)
📝 src/orchestrator/tests.rs (+2 -2)
📝 src/repository/backends/oss/client.rs (+3 -1)
📝 src/repository/backends/oss/layout.rs (+0 -12)
📝 src/repository/backends/oss/repository.rs (+40 -487)
📝 src/repository/backends/posixfs/backend.rs (+11 -69)
📝 src/repository/backends/posixfs/catalog.rs (+64 -445)
📝 src/repository/backends/posixfs/layout.rs (+0 -28)
📝 src/repository/build_cache.rs (+2 -2)
📝 src/repository/interfaces.rs (+17 -70)
📝 src/repository/layout.rs (+62 -3)
📝 src/repository/mod.rs (+2 -4)
➕ src/volume/catalog.rs (+671 -0)
📝 src/volume/mod.rs (+3 -0)

📄 Description

Stacked on #368, which is stacked on #367. Review only the last two commits:

fix(repository): create absent OSS objects with if_not_exists: a bug already on main.
refactor(volume): implement the volume catalog once on repository documents

What

Fix: OssClient::put_bytes_conditionally sent If-None-Match: * for create-only-if-absent writes. opendal's S3 service rejects that argument ("use if_not_exists instead"). As a result, creating a volume and the first build-cache state write failed on every OSS backend. The fix uses opendal's if_not_exists(true).
Refactor: the volume catalog moves out of both backends into src/volume/catalog.rs. It is written once, as a VolumeCatalog extension trait over generic repository documents. src/volume.rs becomes src/volume/mod.rs.

Why

OSS and POSIX each implemented the volume catalog, about 400 and 300 lines, and the two had drifted apart:

Case	OSS (before)	POSIX (before)	Now, both
Reserve a volume that is uploading, failed, or being deleted	Rejected atomically	Accepted; only `VolumeManager`'s earlier, non-atomic check stops it	Rejected atomically
Delete	Mark as deleting, then remove the record, then the alias	One step under locks	Mark, then remove
Reserve a missing volume	`InvalidRequest`	`VolumeNotFound`	`InvalidRequest`
Change the owner of a missing or deleting volume	No-op	`VolumeNotFound`, or applied	No-op
Record disappears during a list	Error	Skipped	Error
Alias points at a missing record, on lookup	Left in place	Removed	Left in place

Both backends now follow the OSS rules, except for how volumes are created (next section).

N/A. Third of three stacked PRs from the review behind #367.

Scope and non-goals

Repository changes: adds delete_document and list_documents; update_document can now leave a document unchanged. The eight volume methods leave the Repository trait.
Publishing and materializing volume backing layers stay on the repository, since that is layer storage.
VolumeManager is unchanged apart from its import; call sites only gain an import.

Design and behavior changes

Create order. OSS claimed the name before writing the record. A concurrent create that found a name pointing at a not-yet-written record treated it as stale and took it over. Both creates then succeeded with the same name, and the name pointed at only one of them.

The shared code writes the record first (only if absent), then claims the name. A create that loses the name deletes its own record. A name whose record is missing is therefore always stale, never mid-creation.

In a test that inserted a 20 ms delay between the two writes:

The name-first order produced 2–13 volumes sharing one name, on every run.
The record-first order produced exactly one, 10 of 10 runs.

Without the delay, the race window on a local filesystem is too small for the concurrency test to hit, so that test guards the protocol rather than proving the old race.

Layout. DocumentKey keeps every existing path and lock file:

Records: volumes/records/{id}.json
Aliases: volumes/aliases/{name}.json on OSS, volumes/aliases/{name} on POSIX, via a POSIX-only path override
POSIX locks: volumes/records/{id}.lock and volumes/aliases/{name}.lock

Compatibility and operations

Public API or generated protocol: the HTTP API is unchanged. Volume reservation and deletion rules on POSIX change as in the table above.
Configuration or defaults: none.
Snapshot manifest, artifact layout, or storage format: unchanged. POSIX no longer pre-creates the empty volumes/ directories; they appear on first write.
Upgrade and rollback:
- POSIX nodes on old and new versions use the same lock files, so mixed versions still exclude each other on the same key.
- During a rolling upgrade, an old OSS node still claims the name first, so the old race can occur between an old and a new node.
Host requirements, permissions, ports, or dependencies: none.

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                        # clean
make clippy                     # clean
cargo test -p agentenv --lib    # 965 passed, 4 ignored
cargo test -p agentenv-e2e-tests --test snapshot_oss_e2e_test -- --ignored
                                # 8 passed (MinIO, including the 2 new tests)

New tests:

POSIX (src/volume/catalog.rs):
- The ported layout, lock-file, and paging test
- 16 concurrent same-name creates leave one volume
- Reservations need a live, ready volume
- Reclaiming a name that points at a missing record
MinIO (snapshot_oss_e2e_test.rs):
- The same catalog rules on OSS
- Build-cache state updates; this test fails on the parent commit with the opendal if_none_match error

The fix commit was checked on its own: clippy is clean and its MinIO test passes.

Skipped checks and reasons:

Capability tests (--ignored in make test-unit) and make test-agent-integration's VM suites were not run; nothing VM- or capability-dependent changed.

Risks and reviewer notes

Start with src/volume/catalog.rs (create_volume, claim_alias, update_record), then src/repository/layout.rs.
The fix commit can land on its own. It's a one-line change that also fixes OSS volume creation on main today. Happy to split it into its own PR.
A crash between the record write and the name claim leaves an unnamed record. It shows up in listings until it's deleted.

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.

🤖 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/369 **Author:** [@yingdi-shan](https://github.com/yingdi-shan) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `refactor/volume-catalog` --- ### 📝 Commits (2) - [`fad9afd`](https://github.com/kvcache-ai/AgentENV/commit/fad9afd9abd7b5699e19aa62e5be15ebeaf63b18) fix(repository): create absent OSS objects with if_not_exists - [`06bfab9`](https://github.com/kvcache-ai/AgentENV/commit/06bfab9ea1f15ee69b0f0d5199a13286d6e1e757) refactor(volume): implement the volume catalog once on repository documents ### 📊 Changes **16 files changed** (+1008 additions, -1126 deletions) <details> <summary>View changed files</summary> 📝 `CLAUDE.md` (+1 -1) 📝 `crates/e2e-tests/tests/snapshot_oss_e2e_test.rs` (+129 -1) 📝 `src/api/impls/image_build/tests.rs` (+1 -1) 📝 `src/orchestrator/tests.rs` (+2 -2) 📝 `src/repository/backends/oss/client.rs` (+3 -1) 📝 `src/repository/backends/oss/layout.rs` (+0 -12) 📝 `src/repository/backends/oss/repository.rs` (+40 -487) 📝 `src/repository/backends/posixfs/backend.rs` (+11 -69) 📝 `src/repository/backends/posixfs/catalog.rs` (+64 -445) 📝 `src/repository/backends/posixfs/layout.rs` (+0 -28) 📝 `src/repository/build_cache.rs` (+2 -2) 📝 `src/repository/interfaces.rs` (+17 -70) 📝 `src/repository/layout.rs` (+62 -3) 📝 `src/repository/mod.rs` (+2 -4) ➕ `src/volume/catalog.rs` (+671 -0) 📝 `src/volume/mod.rs` (+3 -0) </details> ### 📄 Description **Stacked on #368, which is stacked on #367.** Review only the last two commits: 1. `fix(repository): create absent OSS objects with if_not_exists`: a bug already on `main`. 2. `refactor(volume): implement the volume catalog once on repository documents` ## What 1. **Fix:** `OssClient::put_bytes_conditionally` sent `If-None-Match: *` for create-only-if-absent writes. opendal's S3 service rejects that argument ("use if_not_exists instead"). As a result, creating a volume and the first build-cache state write failed on every OSS backend. The fix uses opendal's `if_not_exists(true)`. 2. **Refactor:** the volume catalog moves out of both backends into `src/volume/catalog.rs`. It is written once, as a `VolumeCatalog` extension trait over generic repository documents. `src/volume.rs` becomes `src/volume/mod.rs`. ## Why OSS and POSIX each implemented the volume catalog, about 400 and 300 lines, and the two had drifted apart: | Case | OSS (before) | POSIX (before) | Now, both | |---|---|---|---| | Reserve a volume that is uploading, failed, or being deleted | Rejected atomically | Accepted; only `VolumeManager`'s earlier, non-atomic check stops it | Rejected atomically | | Delete | Mark as deleting, then remove the record, then the alias | One step under locks | Mark, then remove | | Reserve a missing volume | `InvalidRequest` | `VolumeNotFound` | `InvalidRequest` | | Change the owner of a missing or deleting volume | No-op | `VolumeNotFound`, or applied | No-op | | Record disappears during a list | Error | Skipped | Error | | Alias points at a missing record, on lookup | Left in place | Removed | Left in place | Both backends now follow the OSS rules, except for how volumes are created (next section). ## Related issue N/A. Third of three stacked PRs from the review behind #367. ## Scope and non-goals - Repository changes: adds `delete_document` and `list_documents`; `update_document` can now leave a document unchanged. The eight volume methods leave the `Repository` trait. - Publishing and materializing volume backing layers stay on the repository, since that is layer storage. - `VolumeManager` is unchanged apart from its import; call sites only gain an import. ## Design and behavior changes **Create order.** OSS claimed the name before writing the record. A concurrent create that found a name pointing at a not-yet-written record treated it as stale and took it over. Both creates then succeeded with the same name, and the name pointed at only one of them. The shared code writes the record first (only if absent), then claims the name. A create that loses the name deletes its own record. A name whose record is missing is therefore always stale, never mid-creation. In a test that inserted a 20 ms delay between the two writes: - The name-first order produced 2–13 volumes sharing one name, on every run. - The record-first order produced exactly one, 10 of 10 runs. Without the delay, the race window on a local filesystem is too small for the concurrency test to hit, so that test guards the protocol rather than proving the old race. **Layout.** `DocumentKey` keeps every existing path and lock file: - Records: `volumes/records/{id}.json` - Aliases: `volumes/aliases/{name}.json` on OSS, `volumes/aliases/{name}` on POSIX, via a POSIX-only path override - POSIX locks: `volumes/records/{id}.lock` and `volumes/aliases/{name}.lock` ## Compatibility and operations - Public API or generated protocol: the HTTP API is unchanged. Volume reservation and deletion rules on POSIX change as in the table above. - Configuration or defaults: none. - Snapshot manifest, artifact layout, or storage format: unchanged. POSIX no longer pre-creates the empty `volumes/` directories; they appear on first write. - Upgrade and rollback: - POSIX nodes on old and new versions use the same lock files, so mixed versions still exclude each other on the same key. - During a rolling upgrade, an old OSS node still claims the name first, so the old race can occur between an old and a new node. - Host requirements, permissions, ports, or dependencies: none. ## Validation - [x] `make fmt` - [x] `make clippy` - [ ] `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 make fmt # clean make clippy # clean cargo test -p agentenv --lib # 965 passed, 4 ignored cargo test -p agentenv-e2e-tests --test snapshot_oss_e2e_test -- --ignored # 8 passed (MinIO, including the 2 new tests) ``` New tests: - POSIX (`src/volume/catalog.rs`): - The ported layout, lock-file, and paging test - 16 concurrent same-name creates leave one volume - Reservations need a live, ready volume - Reclaiming a name that points at a missing record - MinIO (`snapshot_oss_e2e_test.rs`): - The same catalog rules on OSS - Build-cache state updates; this test fails on the parent commit with the opendal `if_none_match` error The fix commit was checked on its own: clippy is clean and its MinIO test passes. Skipped checks and reasons: - Capability tests (`--ignored` in `make test-unit`) and `make test-agent-integration`'s VM suites were not run; nothing VM- or capability-dependent changed. ## Risks and reviewer notes - Start with `src/volume/catalog.rs` (`create_volume`, `claim_alias`, `update_record`), then `src/repository/layout.rs`. - **The fix commit can land on its own.** It's a one-line change that also fixes OSS volume creation on `main` today. Happy to split it into its own PR. - A crash between the record write and the name claim leaves an unnamed record. It shows up in listings until it's deleted. ## 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. 🤖 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:49 +00:00