[GH-ISSUE #96] posixfs catalog: write_commit_marker uses path.parent().unwrap() that can panic #32

New issue

Closed

opened 2026-10-29 14:18:36 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:18:36 +00:00

Owner

Originally created by @hi-neason on GitHub (Jul 31, 2026).
Original GitHub issue: https://github.com/kvcache-ai/AgentENV/issues/96

Affected component

Snapshot / template builder

AgentENV version

Version or commit: a33198e671 (main)
Local modifications: none

Host environment

OS: N/A (static analysis, any host)
Kernel: N/A
Architecture: any
Bare metal, VM, or cloud: any
ublk support: N/A
CPU: N/A
Memory: N/A
Storage/filesystem: any

Operation and configuration

When publishing a snapshot using the POSIX filesystem repository backend, PosixFsCatalog::write_commit_marker is invoked from commit_session (catalog.rs:110) and from publish_session_snapshot (catalog.rs:117) as part of the normal snapshot commit flow. The path passed in comes from self.commit_marker_path(id).

// src/snapshot/repository/backends/posixfs/catalog.rs:433-446
fn write_commit_marker(&self, id: &SnapshotId) -> RepositoryResult<()> {
    let path = self.commit_marker_path(id);
    if let Some(parent) = path.parent() {              // <-- defensive None-check
        fs::create_dir_all(parent).map_err(/*...*/)?;
    }
    let mut temp =
        tempfile::NamedTempFile::new_in(path.parent().unwrap())  // <-- panics if parent is None
        .map_err(|error| { ... })?;
    // ...
}

Steps to reproduce

A panic requires commit_marker_path(id) to yield a Path with no parent. With the current default config this does not happen because the root is an absolute directory, but the same function guards the create_dir_all call with if let Some(parent) = path.parent() six lines earlier, explicitly acknowledging that path.parent() can be None. The sibling method write_json (line 403-406) already handles the case correctly:

let parent = path.parent().ok_or_else(|| RepositoryError::Backend {
    message: format!("resolve parent for '{}'", path.display()),
    source: None,
})?;
let mut temp = tempfile::NamedTempFile::new_in(parent).map_err(/*...*/)?;

The unwrap on line 441 is therefore an inconsistency that will panic if a bare/relative root (e.g. "." or a filename-only root) is ever used, or if a future change to commit_marker_path returns a bare filename.

Static reproduction:

$ grep -n "path.parent()" src/snapshot/repository/backends/posixfs/catalog.rs
395:        if let Some(parent) = path.parent() {
403:        let parent = path.parent().ok_or_else(|| RepositoryError::Backend {
435:        if let Some(parent) = path.parent() {
441:            tempfile::NamedTempFile::new_in(path.parent().unwrap()).map_err(|error| {

Note line 395/403 (in write_json) and line 435 (in write_commit_marker itself) all handle None defensively; only line 441 unwraps.

Expected behavior

write_commit_marker should return a RepositoryError::Backend when path.parent() is None, matching the pattern already used in write_json at lines 403-406, instead of panicking the task and potentially taking down the server thread.

Actual behavior

path.parent().unwrap() will panic on a None parent with a generic "called Option::unwrap() on a None value" message, bypassing the RepositoryResult error channel.

Logs and diagnostics

N/A (static analysis finding).

Reproduction frequency

Conditional — only triggers when commit_marker_path returns a path with no parent, but the presence of the defensive check 6 lines above confirms the None case was anticipated by the author.

Regression information

Last known good: unknown
First known bad: unknown (present since introduction of posixfs backend)

Additional context

This is a small, low-risk consistency fix (~5 LOC) that replaces the .unwrap() with the same ok_or_else pattern already used in the sibling method. I am happy to send a PR if the approach looks good.

Pre-submission checklist

I searched existing open and closed issues and did not find a duplicate.
I provided a minimal reproduction or explained why one cannot be provided.
I removed credentials, tokens, private image references, and other sensitive information.
This is not a security vulnerability.

Originally created by @hi-neason on GitHub (Jul 31, 2026). Original GitHub issue: https://github.com/kvcache-ai/AgentENV/issues/96 ## Affected component Snapshot / template builder ## AgentENV version - Version or commit: a33198e6718e22e8b8e3a3a784ca12d97f63370e (main) - Local modifications: none ## Host environment - OS: N/A (static analysis, any host) - Kernel: N/A - Architecture: any - Bare metal, VM, or cloud: any - ublk support: N/A - CPU: N/A - Memory: N/A - Storage/filesystem: any ## Operation and configuration When publishing a snapshot using the POSIX filesystem repository backend, `PosixFsCatalog::write_commit_marker` is invoked from `commit_session` (catalog.rs:110) and from `publish_session_snapshot` (catalog.rs:117) as part of the normal snapshot commit flow. The path passed in comes from `self.commit_marker_path(id)`. ```rust // src/snapshot/repository/backends/posixfs/catalog.rs:433-446 fn write_commit_marker(&self, id: &SnapshotId) -> RepositoryResult<()> { let path = self.commit_marker_path(id); if let Some(parent) = path.parent() { // <-- defensive None-check fs::create_dir_all(parent).map_err(/*...*/)?; } let mut temp = tempfile::NamedTempFile::new_in(path.parent().unwrap()) // <-- panics if parent is None .map_err(|error| { ... })?; // ... } ``` ## Steps to reproduce A panic requires `commit_marker_path(id)` to yield a `Path` with no parent. With the current default config this does not happen because the root is an absolute directory, but the same function guards the `create_dir_all` call with `if let Some(parent) = path.parent()` six lines earlier, explicitly acknowledging that `path.parent()` can be `None`. The sibling method `write_json` (line 403-406) already handles the case correctly: ```rust let parent = path.parent().ok_or_else(|| RepositoryError::Backend { message: format!("resolve parent for '{}'", path.display()), source: None, })?; let mut temp = tempfile::NamedTempFile::new_in(parent).map_err(/*...*/)?; ``` The unwrap on line 441 is therefore an inconsistency that will panic if a bare/relative root (e.g. `"."` or a filename-only root) is ever used, or if a future change to `commit_marker_path` returns a bare filename. Static reproduction: ```text $ grep -n "path.parent()" src/snapshot/repository/backends/posixfs/catalog.rs 395: if let Some(parent) = path.parent() { 403: let parent = path.parent().ok_or_else(|| RepositoryError::Backend { 435: if let Some(parent) = path.parent() { 441: tempfile::NamedTempFile::new_in(path.parent().unwrap()).map_err(|error| { ``` Note line 395/403 (in `write_json`) and line 435 (in `write_commit_marker` itself) all handle `None` defensively; only line 441 unwraps. ## Expected behavior `write_commit_marker` should return a `RepositoryError::Backend` when `path.parent()` is `None`, matching the pattern already used in `write_json` at lines 403-406, instead of panicking the task and potentially taking down the server thread. ## Actual behavior `path.parent().unwrap()` will panic on a `None` parent with a generic "called `Option::unwrap()` on a `None` value" message, bypassing the `RepositoryResult` error channel. ## Logs and diagnostics N/A (static analysis finding). ## Reproduction frequency Conditional — only triggers when `commit_marker_path` returns a path with no parent, but the presence of the defensive check 6 lines above confirms the None case was anticipated by the author. ## Regression information - Last known good: unknown - First known bad: unknown (present since introduction of posixfs backend) ## Additional context This is a small, low-risk consistency fix (~5 LOC) that replaces the `.unwrap()` with the same `ok_or_else` pattern already used in the sibling method. I am happy to send a PR if the approach looks good. ## Pre-submission checklist - [X] I searched existing open and closed issues and did not find a duplicate. - [X] I provided a minimal reproduction or explained why one cannot be provided. - [X] I removed credentials, tokens, private image references, and other sensitive information. - [X] This is not a security vulnerability.

bsakaguchi closed this issue

2026-10-29 14:18:36 +00:00

bsakaguchi referenced this issue

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

[PR #32] [CLOSED] fix(snapshot): isolate registry publications on delete #130