[PR #104] [MERGED] fix(snapshot): return error instead of panicking in write_commit_marker #179

New issue

Closed

opened 2026-10-29 14:20:43 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:20:43 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/104
Author: @Loken-Lau
Created: 8/2/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @yingdi-shan

Base: main ← Head: fix/posixfs-commit-marker-parent

📝 Commits (1)

e7171b3 fix(snapshot): return error instead of panicking in write_commit_marker

📊 Changes

1 file changed (+13 additions, -12 deletions)

View changed files

📝 src/snapshot/repository/backends/posixfs/catalog.rs (+13 -12)

📄 Description

What

PosixFsCatalog::write_commit_marker guarded the create_dir_all call with
if let Some(parent) = path.parent(), but then called
path.parent().unwrap() when creating the temp commit marker file. If
commit_marker_path ever yields a parentless path, the defensive check is
bypassed and the function panics instead of returning a RepositoryError.

This change resolves the parent once up front with ok_or_else, returns a
RepositoryError::Backend when it is absent, and reuses it for both
create_dir_all and NamedTempFile::new_in — matching the pattern already
used by write_json_atomic in the same file. It also removes the last
unwrap() from posixfs/catalog.rs.

Why

A snapshot repository backend must never panic on an unexpected path shape;
commit failures should surface as typed RepositoryErrors so callers can
handle them. The mixed checked/unchecked usage was also internally
inconsistent.

Closes #96

Scope and non-goals

In scope: write_commit_marker error handling only.
Non-goals: changing commit_marker_path layout, touching other backends,
or auditing other crates for similar unwrap() patterns.

Design and behavior changes

No behavior change for existing configurations where the parent always
exists. For the previously panicking edge case, the function now returns
RepositoryError::Backend { message: "resolve parent for '<path>'" }.

Compatibility and operations

Public API or generated protocol: no changes.
Configuration or defaults: no changes.
Snapshot manifest, artifact layout, or storage format: no changes.
Upgrade and rollback: no impact; the change is rollback-safe.
Host requirements, permissions, ports, or dependencies: no changes.

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 (WSL2, Ubuntu, kernel 6.18.33.2-microsoft-standard-WSL2):

cargo fmt --all -- --check                      # clean
cargo clippy -p agentenv --lib -- -D warnings   # clean
cargo check -p agentenv --lib                   # Finished, no warnings

make test-unit                                  # PASS (exit 0)
#   cargo test -p agentenv -p envd -p linux-cap --lib
#     agentenv:  671 passed; 0 failed; 4 ignored
#     envd:      0 passed; 0 failed
#     linux-cap: 3 passed; 0 failed
#   cargo test -p agentenv --lib -- --ignored           (capability runner): PASS
#   cargo test -p uvm-ublk -p uvm-ublk-daemon --lib     (capability runner): PASS
#   scripts/tests/verify-capability-runner.sh:          PASS
#   scripts/tests/verify-install-service.sh:            PASS

make -C services test                           # PASS (exit 0; not required —
                                                # no services/ changes, ran anyway)

Skipped checks and reasons

Rust integration tests (make test-integration) and make test-ublk:
this development environment is WSL2, whose stock kernel does not ship the
ublk_drv module (/dev/ublk-control absent) and restricts io_uring, so
overlaybd/ublk-backed tests cannot run here (attempted: overlaybd tests
fail at lower-layer open with io_uring errors — an environment limitation,
not related to this change). These paths are covered by CI on proper Linux
hosts with /dev/kvm + ublk.
make test-envd: requires a running Docker daemon, not available in this
environment.
Generated clients/server: no generated code touched.
Documentation: no user-facing behavior change to document.
Benchmarks: no performance-sensitive code path affected.

All unit tests covering the touched subsystem pass, including the posixfs
catalog/backend suites (begin_and_commit_make_snapshot_visible,
publishes_gets_and_resolves_snapshot, failed_commit_cleans_uncommitted_ snapshot_directory, etc.) that exercise the commit-marker flow.

Risks and reviewer notes

Low risk: pure error-handling change in one function; the new error path is
unreachable with the current default config (repository root is an absolute
directory), so existing deployments see no behavioral difference.

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.
(The parentless-path branch is unreachable through current public
constructors; existing posixfs commit-flow tests all pass.)
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/104 **Author:** [@Loken-Lau](https://github.com/Loken-Lau) **Created:** 8/2/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@yingdi-shan](https://github.com/yingdi-shan) **Base:** `main` ← **Head:** `fix/posixfs-commit-marker-parent` --- ### 📝 Commits (1) - [`e7171b3`](https://github.com/kvcache-ai/AgentENV/commit/e7171b3e0af9c6fb53696e42bc15f2f14123beae) fix(snapshot): return error instead of panicking in write_commit_marker ### 📊 Changes **1 file changed** (+13 additions, -12 deletions) <details> <summary>View changed files</summary> 📝 `src/snapshot/repository/backends/posixfs/catalog.rs` (+13 -12) </details> ### 📄 Description ## What `PosixFsCatalog::write_commit_marker` guarded the `create_dir_all` call with `if let Some(parent) = path.parent()`, but then called `path.parent().unwrap()` when creating the temp commit marker file. If `commit_marker_path` ever yields a parentless path, the defensive check is bypassed and the function panics instead of returning a `RepositoryError`. This change resolves the parent once up front with `ok_or_else`, returns a `RepositoryError::Backend` when it is absent, and reuses it for both `create_dir_all` and `NamedTempFile::new_in` — matching the pattern already used by `write_json_atomic` in the same file. It also removes the last `unwrap()` from `posixfs/catalog.rs`. ## Why A snapshot repository backend must never panic on an unexpected path shape; commit failures should surface as typed `RepositoryError`s so callers can handle them. The mixed checked/unchecked usage was also internally inconsistent. ## Related issue Closes #96 ## Scope and non-goals - In scope: `write_commit_marker` error handling only. - Non-goals: changing `commit_marker_path` layout, touching other backends, or auditing other crates for similar `unwrap()` patterns. ## Design and behavior changes No behavior change for existing configurations where the parent always exists. For the previously panicking edge case, the function now returns `RepositoryError::Backend { message: "resolve parent for '<path>'" }`. ## Compatibility and operations - Public API or generated protocol: no changes. - Configuration or defaults: no changes. - Snapshot manifest, artifact layout, or storage format: no changes. - Upgrade and rollback: no impact; the change is rollback-safe. - Host requirements, permissions, ports, or dependencies: no changes. ## Validation - [x] `make fmt` - [x] `make clippy` - [x] `make test-unit` - [ ] Relevant Rust integration tests - [x] `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 (WSL2, Ubuntu, kernel 6.18.33.2-microsoft-standard-WSL2): ``` cargo fmt --all -- --check # clean cargo clippy -p agentenv --lib -- -D warnings # clean cargo check -p agentenv --lib # Finished, no warnings make test-unit # PASS (exit 0) # cargo test -p agentenv -p envd -p linux-cap --lib # agentenv: 671 passed; 0 failed; 4 ignored # envd: 0 passed; 0 failed # linux-cap: 3 passed; 0 failed # cargo test -p agentenv --lib -- --ignored (capability runner): PASS # cargo test -p uvm-ublk -p uvm-ublk-daemon --lib (capability runner): PASS # scripts/tests/verify-capability-runner.sh: PASS # scripts/tests/verify-install-service.sh: PASS make -C services test # PASS (exit 0; not required — # no services/ changes, ran anyway) ``` ## Skipped checks and reasons - Rust integration tests (`make test-integration`) and `make test-ublk`: this development environment is WSL2, whose stock kernel does not ship the `ublk_drv` module (`/dev/ublk-control` absent) and restricts io_uring, so overlaybd/ublk-backed tests cannot run here (attempted: overlaybd tests fail at lower-layer open with io_uring errors — an environment limitation, not related to this change). These paths are covered by CI on proper Linux hosts with `/dev/kvm` + ublk. - `make test-envd`: requires a running Docker daemon, not available in this environment. - Generated clients/server: no generated code touched. - Documentation: no user-facing behavior change to document. - Benchmarks: no performance-sensitive code path affected. All unit tests covering the touched subsystem pass, including the posixfs catalog/backend suites (`begin_and_commit_make_snapshot_visible`, `publishes_gets_and_resolves_snapshot`, `failed_commit_cleans_uncommitted_ snapshot_directory`, etc.) that exercise the commit-marker flow. ## Risks and reviewer notes Low risk: pure error-handling change in one function; the new error path is unreachable with the current default config (repository root is an absolute directory), so existing deployments see no behavioral difference. ## 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. (The parentless-path branch is unreachable through current public constructors; existing posixfs commit-flow tests all pass.) - [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:20:43 +00:00