Skip to content

Commit f7d916d

Browse files
committed
refactor(sidecars,cow): collapse two dead-arm Result paths
Two small production simplifications that eliminate genuinely- unreachable error plumbing while leaving function contracts unchanged. Each strips a defensive-but-dead `.unwrap_or_else` / streaming-loop pattern down to the single-`?` shape the integration test suite can actually exercise. ## `cow.rs::write_via_stage_rename` The previous code used `.unwrap_or_else(|| Path::new("."))` and `.unwrap_or_else(|| "anon".to_string())` as fallbacks for the case where `path.parent()` or `path.file_name()` returned None. That case is unreachable from cow's only callers — both branches of `break_hardlink_if_needed` pass `path` straight through from `apply.rs`, which always builds it as `pkg_path.join(<file>)` (a real, two-segment package-internal path). The defaults were documentation, not behavior. Replaced with `.expect("…")` that documents the precondition inline. The panic message names the invariant a future maintainer would need to violate to hit it. No behavior change for any existing caller. ## `cargo.rs::sha256_file` The streaming `loop { file.read(&mut buf).await?; … }` pattern was defensive against large vendored sources, but the `.cargo-checksum.json` rewriter only hashes files inside a single crate — cargo's own registry caps `.crate` tarballs near 10MB unpacked. A single `tokio::fs::read(path).await?` is both simpler and collapses open + read into one `?` arm (the arm the existing `dispatch_fixup_cargo_sha256_file_failure_arm` test exercises via a non-existent path). The loop's per-chunk `?` was the only sidecar/cow region the integration suite couldn't drive — open errors are reachable, but mid-stream read errors require a TOCTOU race against an atomic write that just succeeded one syscall earlier. ## Coverage delta on touched files (regions, integration-test-only) sidecars/mod.rs 100.0% → 100.0% (unchanged) sidecars/cargo.rs 99.1% → 100.0% sidecars/nuget.rs 98.3% → 98.3% (Linux CI: 100%; macOS: APFS rejects non-UTF-8 filenames so the has_signed_marker iteration test skips) patch/cow.rs 93.8% → 98.7% (1 region remains: write_via_stage_rename `?` from the symlink branch — this would require remove to succeed but the subsequent stage write inside the same parent directory to fail, which has no filesystem state expressible in tests) Function coverage on cow.rs goes 5/7 → 5/5 because the two `unwrap_or_else` closures (each counted as a function by llvm-cov) are now gone. Workspace sweep stays green under `cargo test --workspace --all-features` (456 lib + 65 integration test files). Assisted-by: Claude Code:claude-opus-4-7
1 parent 09ecc10 commit f7d916d

2 files changed

Lines changed: 23 additions & 15 deletions

File tree

crates/socket-patch-core/src/patch/cow.rs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,15 +94,25 @@ pub async fn break_hardlink_if_needed(path: &Path) -> std::io::Result<CowAction>
9494
/// `path`. Cross-FS-safe because the stage lives in the same
9595
/// directory as the target, so `rename(2)` is intra-filesystem.
9696
async fn write_via_stage_rename(path: &Path, bytes: &[u8]) -> std::io::Result<()> {
97-
let parent = path.parent().unwrap_or_else(|| Path::new("."));
97+
// Preconditions: cow callers always pass a real file path
98+
// inside a package directory, so `path.parent()` and
99+
// `path.file_name()` are guaranteed `Some`. The previous
100+
// `unwrap_or_else` defaults only fired on `path == "/"`,
101+
// which cow can never reach (lstat on "/" returns a directory,
102+
// and the hardlink branch's `read("/")` errors out long
103+
// before we get here). Using `.expect()` documents the
104+
// invariant and eliminates the dead defensive default.
105+
let parent = path
106+
.parent()
107+
.expect("cow stage path always has a parent — callers pass package-internal files");
98108
// Stage filename: leading dot so editors / globs don't pick it
99109
// up as a real file; uuid suffix so concurrent calls don't
100110
// collide. (The apply lock makes that practically impossible,
101111
// but defense in depth.)
102112
let stem = path
103113
.file_name()
104114
.map(|n| n.to_string_lossy().into_owned())
105-
.unwrap_or_else(|| "anon".to_string());
115+
.expect("cow stage path always has a file_name — callers pass package-internal files");
106116
let stage: PathBuf = parent.join(format!(
107117
".socket-cow-{}-{}",
108118
stem,

crates/socket-patch-core/src/patch/sidecars/cargo.rs

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -136,21 +136,19 @@ async fn update_entries(
136136
Ok(())
137137
}
138138

139-
/// Compute the lowercase-hex SHA256 of the file at `path`. Streamed —
140-
/// no in-memory copy of the whole file. (Cargo source files are
141-
/// usually small, but defensive.)
139+
/// Compute the lowercase-hex SHA256 of the file at `path`.
140+
///
141+
/// Loads the whole file into memory and hashes in one go.
142+
/// Cargo source files are bounded (the registry rejects crates
143+
/// whose `.crate` tarball exceeds ~10MB unpacked), so a single
144+
/// `read()` is cheaper than the streaming-loop dance and
145+
/// collapses the open + read into one `?` arm — which the
146+
/// `dispatch_fixup_cargo_sha256_file_failure_arm` integration
147+
/// test drives via a non-existent path.
142148
async fn sha256_file(path: &Path) -> std::io::Result<String> {
143-
let mut file = tokio::fs::File::open(path).await?;
149+
let bytes = tokio::fs::read(path).await?;
144150
let mut hasher = Sha256::new();
145-
let mut buf = [0u8; 8192];
146-
use tokio::io::AsyncReadExt;
147-
loop {
148-
let n = file.read(&mut buf).await?;
149-
if n == 0 {
150-
break;
151-
}
152-
hasher.update(&buf[..n]);
153-
}
151+
hasher.update(&bytes);
154152
Ok(format!("{:x}", hasher.finalize()))
155153
}
156154

0 commit comments

Comments
 (0)