fix(certgen): stage temp dir inside output dir to fix cross-device rename#2241
Conversation
…name generate-certs stages temp files beside --output-dir using dir.with_file_name(). When --output-dir is a container or WSL volume mount, the staging dir lands on a different filesystem and std::fs::rename fails with EXDEV. Move staging inside the output dir so the rename always stays on the same filesystem, preserving atomic replacement. Fixes NVIDIA#2173 Signed-off-by: Grace Smith <grasmith@redhat.com>
|
All contributors have signed the DCO ✍️ ✅ |
|
I have read the DCO document and I hereby sign the DCO. |
|
recheck |
r3v5
left a comment
There was a problem hiding this comment.
🤖 Automatic review using OpenShell's skill
review-github-prin Claude Code with Opus 4.6 (1M context)
PR: #2241
Author: Grace Smith
Branch: fix/2173-certgen-cross-device-rename -> main
Overview
Fixes EXDEV (cross-device rename) when --output-dir is a volume mount. Temp dir was created as sibling (dir.with_file_name(...)) — lands on different filesystem from target. Now created inside output dir (dir.join(...)) so rename stays same-filesystem.
Key Design Decisions
- Temp dir placement changed from sibling to child (certgen.rs:771-773): dir.join(".certgen.tmp") instead of dir.with_file_name(name). Core fix — guarantees same-filesystem rename.
- create_dir_restricted moved before staging (certgen.rs:627-628, 690-691, 733): Output dir must exist before creating temp subdir inside it. Previously created after staging, which would fail with new approach.
- Dotfile naming convention (.certgen.tmp): Hidden prefix avoids collision with real cert artifacts.
Notable Code
crates/openshell-server/src/certgen.rs:769-773
fn staging_temp_dir(dir: &Path) -> PathBuf {
// Place temp inside dir so rename stays on the same filesystem as the destination.
dir.join(".certgen.tmp")
}
Potential Concerns
- Stale temp cleanup on crash: If process dies mid-write, .certgen.tmp now lives inside output dir rather than beside it. Next run cleans it up (existing remove_dir_all logic handles this), but operators browsing output dir might see it. Dotfile prefix mitigates visibility.
- create_dir_restricted now called twice for some dirs (e.g. server_dir, client_dir in write_local_tls_bundle): once early at line 690-691, then the original calls were removed. Net effect correct but worth confirming no ordering assumption elsewhere depends on late creation.
Clean, minimal fix. Net -2 lines. Tests updated. Looks good.
|
/ok to test d7ffd0a |
|
Label |
TaylorMutch
left a comment
There was a problem hiding this comment.
Reviewed the certgen staging change. The implementation is focused and correct, focused certgen tests pass locally, and the full labeled E2E suite completed successfully.
Summary
The temp dir was placed beside the output dir instead of inside it. When
--output-diris a volume mount, the sibling ends up on a different filesystem --std::fs::renamefails with EXDEV (os error 18). Moving the temp inside the output dir keeps both on the same filesystem.Related Issue
Fixes #2173
Changes
staging_temp_dir: usedir.join(".certgen.tmp")instead ofdir.with_file_name(...)-- places temp inside the output dirwrite_local_bundle,write_local_tls_bundle,write_local_jwt_bundle: movecreate_dir_restrictedcalls beforestaging_temp_dirso the output dir exists before staging inside itsibling_temp_dirtostaging_temp_dirto reflect the new placementTesting
mise run pre-commitpassesReproduced EXVED on WSL with Podman + named volume, confirmed patched binary succeeds with "PKI files created dir=/extvol".
Checklist