fix(#1763): address argus review on PR #2430

- regenerate docs/tools/hivectl-cli.md for the new `subvol snapshot send` verb
- close the TOCTOU on the no-overwrite guard: File::options().create_new(true)
  (O_CREAT|O_EXCL) instead of exists()-then-create, so the guarantee is
  atomic against a concurrent request racing the same dest filename
- warn (not silently swallow) if cleaning up a partial export after a
  failed btrfs send itself fails, so a stuck garbage file masquerading
  as a completed export is visible in the log
This commit is contained in:
atlas 2026-07-14 19:16:49 +02:00 committed by mara
commit 60a253a2f6
2 changed files with 45 additions and 8 deletions

View file

@ -1220,15 +1220,23 @@ async fn send_agent_snapshot_to_file(
std::fs::create_dir_all(MIGRATE_STAGING_ROOT)
.with_context(|| format!("create {MIGRATE_STAGING_ROOT}"))?;
let dest = Path::new(MIGRATE_STAGING_ROOT).join(dest_file_name);
if dest.exists() {
bail!(
// `create_new` (O_CREAT|O_EXCL) makes the no-overwrite guarantee atomic
// instead of a check-then-create race against a concurrent request.
let dest_file = match std::fs::File::options()
.write(true)
.create_new(true)
.open(&dest)
{
Ok(f) => f,
Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => bail!(
"{} already exists — pick a different destination or remove it first \
(send never overwrites an existing export)",
dest.display()
);
}
let dest_file =
std::fs::File::create(&dest).with_context(|| format!("create {}", dest.display()))?;
),
Err(e) => {
return Err(e).with_context(|| format!("create {}", dest.display()));
}
};
let mut cmd = Command::new("btrfs");
cmd.arg("send");
@ -1254,8 +1262,17 @@ async fn send_agent_snapshot_to_file(
.with_context(|| format!("wait on btrfs send {}", snap.display()))?;
if !out.status.success() {
// Clean up a partial/failed export so a retry doesn't trip the
// "already exists" guard on garbage.
let _ = std::fs::remove_file(&dest);
// "already exists" guard on garbage. Best-effort: warn (don't fail
// the whole call over it) if removal itself fails, so a stuck
// partial file that later masquerades as a completed export is at
// least visible in the log.
if let Err(rm_err) = std::fs::remove_file(&dest) {
tracing::warn!(
dest = %dest.display(), error = %rm_err,
"failed to remove partial export after btrfs send failure — \
next attempt at this dest will hit the already-exists guard"
);
}
bail!(
"btrfs send {} failed: {}",
snap.display(),