From 1d532e06479f3de011bccb48c8c2e6a7a5a50e40 Mon Sep 17 00:00:00 2001 From: hmirin Date: Thu, 29 Oct 2026 00:37:29 +0900 Subject: [PATCH] Restore the carrier checkout after a failed publish and report real push errors A carrier compare-and-swap wrote the new HEAD into its checkout and committed it locally before pushing, and left both in place when the push failed. Reads on the same storage then saw the unpublished manifest as the current head, so in a helper push batch a ref that Git had been told failed was published together with the next ref. publish_checkout_head now restores the checkout to the last published commit on any failure, including removing the failed publication's staged objects. Every failed push was also reported as a CAS conflict and its stderr discarded, so an authentication failure or hook rejection told the user to git pull. After a failed push the branch is fetched again: a tip equal to the new commit means the push landed, a tip equal to the base means an ordinary error carrying git's stderr, and any other tip is a CAS conflict. A stale compare-and-swap against a remote whose HEAD file is malformed now reports a conflict instead of a parse error. --- src/storage.rs | 135 +++++++++++++++++++++------- tests/carrier_git.rs | 203 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 306 insertions(+), 32 deletions(-) diff --git a/src/storage.rs b/src/storage.rs index e4b0bf1..89012a2 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -1025,21 +1025,13 @@ impl GitStorage { } } - fn head_at_commit(&self, commit: Option<&str>) -> Result> { - let Some(commit) = commit else { - return Ok(None); - }; - let output = carrier_git_command() - .arg("-C") - .arg(self.root()) - .args(["show", &format!("{commit}:HEAD")]) - .output()?; - if !output.status.success() { - return Ok(None); - } - let value = String::from_utf8(output.stdout)?.trim().to_owned(); - validate_id(&value)?; - Ok(Some(value)) + /// The manifest ID a conflicting remote tip names, for diagnostics only: + /// a tip whose HEAD is missing or malformed still wins the conflict. + fn conflicting_head(&self, commit: Option<&str>) -> Option { + let bytes = self.head_bytes_at_commit(commit?).ok()??; + let value = String::from_utf8(bytes).ok()?.trim().to_owned(); + validate_id(&value).ok()?; + Some(value) } fn head_bytes_at_commit(&self, commit: &str) -> Result>> { @@ -1198,14 +1190,36 @@ impl GitStorage { Ok(true) } + /// Publishes `next` on top of `base_commit`. On failure the checkout is + /// restored to `base_commit`, so later reads through this storage never + /// see an unpublished HEAD and later publications never carry it. fn publish_checkout_head( &self, next: &str, expected: Option, - genesis: bool, + base_commit: &Option, + cache: &mut CarrierCache, + ) -> Result { + let result = self.commit_and_push_head(next, expected, base_commit, cache); + if let Err(error) = result { + return Err(match self.restore_checkout(base_commit) { + Ok(()) => error, + Err(restore) => error.context(format!( + "restoring the carrier checkout after a failed publication also failed: {restore:#}" + )), + }); + } + result + } + + fn commit_and_push_head( + &self, + next: &str, + expected: Option, + base_commit: &Option, cache: &mut CarrierCache, ) -> Result { - self.write_checkout_head(next, genesis)?; + self.write_checkout_head(next, base_commit.is_none())?; let add_timer = trace::Span::new("carrier_git_add"); stage_carrier_checkout(self.root(), &cache.path.join("objects"))?; drop(add_timer); @@ -1225,18 +1239,79 @@ impl GitStorage { .args(["push", "--quiet", "origin"]) .arg(format!("HEAD:refs/heads/{}", self.branch)) .output()?; + drop(push_timer); if !push.status.success() { - let latest = cache.refresh(&self.remote, &self.branch)?; - return Err(CasConflict { - expected, - actual: self.head_at_commit(latest.as_deref())?, + let push_error = anyhow::anyhow!( + "git push carrier branch failed: {}", + String::from_utf8_lossy(&push.stderr).trim() + ); + // Only an observed move of the remote tip is a lost race; a push + // refused for any other reason leaves the tip at `base_commit`. + let Ok(latest) = cache.refresh(&self.remote, &self.branch) else { + return Err(push_error); + }; + if latest.as_deref() != Some(new_commit.as_str()) { + if latest == *base_commit { + return Err(push_error); + } + return Err(CasConflict { + expected, + actual: self.conflicting_head(latest.as_deref()), + } + .into()); } - .into()); } - drop(push_timer); cache.update_published_tip(&new_commit)?; Ok(new_commit) } + + /// Points the local branch and index back at `base_commit` and removes + /// every working-tree file the failed publication added or changed. + fn restore_checkout(&self, base_commit: &Option) -> Result<()> { + let root = self.root(); + let local_ref = format!("refs/heads/{}", self.branch); + match base_commit { + Some(commit) => { + git_command(root, &["update-ref", &local_ref, commit])?; + git_command(root, &["read-tree", commit])?; + } + None => { + if git_rev_parse(root, &local_ref)?.is_some() { + git_command(root, &["update-ref", "-d", &local_ref])?; + } + git_command(root, &["read-tree", "--empty"])?; + } + } + // The checkout is sparse: a missing HEAD file falls back to the + // committed one. + match fs::remove_file(root.join("HEAD")) { + Err(error) if error.kind() != std::io::ErrorKind::NotFound => return Err(error.into()), + _ => {} + } + let untracked = carrier_git_command() + .arg("-C") + .arg(root) + .args([ + "ls-files", + "--others", + "-z", + "--", + "README.md", + "FORMAT", + "manifests", + "packs", + ]) + .output()?; + ensure_git_success(&untracked, "git ls-files carrier checkout")?; + for path in untracked.stdout.split(|byte| *byte == 0) { + if path.is_empty() { + continue; + } + let path = std::str::from_utf8(path).context("carrier checkout path is not UTF-8")?; + fs::remove_file(root.join(path))?; + } + Ok(()) + } } impl Storage for GitStorage { @@ -1295,7 +1370,7 @@ impl Storage for GitStorage { if remote_tip != state.base_commit { return Err(CasConflict { expected: expected.map(ToOwned::to_owned), - actual: self.head_at_commit(remote_tip.as_deref())?, + actual: self.conflicting_head(remote_tip.as_deref()), } .into()); } @@ -1313,7 +1388,7 @@ impl Storage for GitStorage { let new_commit = self.publish_checkout_head( next, expected.map(ToOwned::to_owned), - state.base_commit.is_none(), + &state.base_commit, &mut cache, )?; state.base_commit = Some(new_commit); @@ -1360,7 +1435,7 @@ impl Storage for GitStorage { if remote_tip != expected_tip { return Err(CasConflict { expected: observed.head_id.clone(), - actual: self.head_at_commit(remote_tip.as_deref())?, + actual: self.conflicting_head(remote_tip.as_deref()), } .into()); } @@ -1381,12 +1456,8 @@ impl Storage for GitStorage { if actual_bytes.is_none() { self.refuse_foreign_branch(&expected_tip)?; } - let new_commit = self.publish_checkout_head( - next, - observed.head_id.clone(), - expected_tip.is_none(), - &mut cache, - )?; + let new_commit = + self.publish_checkout_head(next, observed.head_id.clone(), &expected_tip, &mut cache)?; state.base_commit = Some(new_commit); Ok(()) } diff --git a/tests/carrier_git.rs b/tests/carrier_git.rs index 48bc576..ff734c7 100644 --- a/tests/carrier_git.rs +++ b/tests/carrier_git.rs @@ -1806,3 +1806,206 @@ fn carrier_init_refuses_an_existing_branch_and_another_branch_works() { ); assert!(!git(&carrier, &["rev-parse", "refs/heads/vault"], false).is_empty()); } + +/// Installs a pre-receive hook that rejects every push while `marker` exists, +/// removing the marker on the first rejection when `once` is set. +fn reject_pushes_while(carrier: &Path, marker: &Path, once: bool) { + use std::os::unix::fs::PermissionsExt; + let remove = if once { + format!("rm '{}'", marker.display()) + } else { + String::new() + }; + let hook = carrier.join("hooks/pre-receive"); + fs::write( + &hook, + format!( + "#!/bin/sh\nif [ -e '{marker}' ]; then\n {remove}\n echo 'carrier test hook refused the update' >&2\n exit 1\nfi\n", + marker = marker.display(), + ), + ) + .unwrap(); + fs::set_permissions(&hook, fs::Permissions::from_mode(0o755)).unwrap(); +} + +fn remote_tip(carrier: &Path) -> String { + git(carrier, &["rev-parse", "refs/heads/main"], false) +} + +#[test] +fn rejected_carrier_publish_restores_the_published_head() { + let temporary = tempfile::tempdir().unwrap(); + let _cache = isolated_cache(&temporary); + let carrier = temporary.path().join("carrier.git"); + git( + temporary.path(), + &["init", "--bare", "-q", carrier.to_str().unwrap()], + false, + ); + let published = "5".repeat(64); + let rejected = "6".repeat(64); + let next = "7".repeat(64); + let storage = GitStorage::open(carrier.to_str().unwrap()).unwrap(); + storage.compare_and_swap_head(None, &published).unwrap(); + let tip = remote_tip(&carrier); + + let marker = temporary.path().join("reject"); + fs::write(&marker, "").unwrap(); + reject_pushes_while(&carrier, &marker, false); + let error = storage + .compare_and_swap_head(Some(&published), &rejected) + .unwrap_err(); + assert_eq!(remote_tip(&carrier), tip); + assert_eq!( + storage.read_head().unwrap().as_deref(), + Some(published.as_str()) + ); + assert!( + error.downcast_ref::().is_none(), + "a hook rejection is not a compare-and-swap loss: {error:#}" + ); + assert!( + format!("{error:#}").contains("carrier test hook refused the update"), + "the push error keeps git's stderr: {error:#}" + ); + + fs::remove_file(&marker).unwrap(); + storage + .compare_and_swap_head(Some(&published), &next) + .unwrap(); + assert_eq!( + git(&carrier, &["show", "refs/heads/main:HEAD"], false), + next + ); + assert_eq!( + git(&carrier, &["rev-parse", "refs/heads/main^"], false), + tip, + "the next publication builds on the published commit, not the rejected one" + ); +} + +#[test] +fn rejected_carrier_genesis_leaves_an_uninitialized_checkout() { + let temporary = tempfile::tempdir().unwrap(); + let _cache = isolated_cache(&temporary); + let carrier = temporary.path().join("carrier.git"); + git( + temporary.path(), + &["init", "--bare", "-q", carrier.to_str().unwrap()], + false, + ); + let marker = temporary.path().join("reject"); + fs::write(&marker, "").unwrap(); + reject_pushes_while(&carrier, &marker, true); + let storage = GitStorage::open(carrier.to_str().unwrap()).unwrap(); + let rejected = "6".repeat(64); + let next = "7".repeat(64); + let error = storage.compare_and_swap_head(None, &rejected).unwrap_err(); + assert_eq!(storage.read_head().unwrap(), None); + assert!(error.downcast_ref::().is_none(), "{error:#}"); + + storage.compare_and_swap_head(None, &next).unwrap(); + assert_eq!( + git(&carrier, &["show", "refs/heads/main:HEAD"], false), + next + ); + assert_eq!( + git(&carrier, &["rev-list", "--count", "refs/heads/main"], false), + "1" + ); +} + +#[test] +fn stale_compare_and_swap_conflicts_even_when_the_remote_head_is_garbage() { + let temporary = tempfile::tempdir().unwrap(); + let _cache = isolated_cache(&temporary); + let carrier = temporary.path().join("carrier.git"); + git( + temporary.path(), + &["init", "--bare", "-q", carrier.to_str().unwrap()], + false, + ); + let published = "5".repeat(64); + GitStorage::open(carrier.to_str().unwrap()) + .unwrap() + .compare_and_swap_head(None, &published) + .unwrap(); + let stale = GitStorage::open(carrier.to_str().unwrap()).unwrap(); + edit_carrier(&carrier, &temporary.path().join("editor"), |workdir| { + fs::write(workdir.join("HEAD"), "not a manifest id\n").unwrap(); + }); + + let error = stale + .compare_and_swap_head(Some(&published), &"8".repeat(64)) + .unwrap_err(); + let conflict = error + .downcast_ref::() + .unwrap_or_else(|| panic!("expected a conflict: {error:#}")); + assert_eq!(conflict.actual, None); +} + +#[test] +fn failed_ref_in_a_helper_push_batch_is_not_published_by_the_next_ref() { + let temporary = tempfile::tempdir().unwrap(); + let _cache = isolated_cache(&temporary); + let carrier = temporary.path().join("carrier.git"); + let source = temporary.path().join("source"); + let key_path = temporary.path().join("repository.key.json"); + init_source(&source, "batch marker"); + let key = KeyFile::generate(); + key.write_new(&key_path).unwrap(); + init_carrier(temporary.path(), &carrier, &key); + let url = format!("e2ee::{}", carrier.display()); + push_through_helper(&source, &url, &key_path, temporary.path()); + + let marker = temporary.path().join("reject-once"); + fs::write(&marker, "").unwrap(); + reject_pushes_while(&carrier, &marker, true); + let output = helper_command(&source, temporary.path()) + .args([ + "push", + "--porcelain", + "private", + "main:refs/heads/a", + "main:refs/heads/b", + ]) + .output() + .unwrap(); + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(!output.status.success(), "stdout={stdout} stderr={stderr}"); + let status = |reference: &str| { + stdout + .lines() + .find(|line| line.contains(&format!(":{reference}\t"))) + .unwrap_or_else(|| panic!("no status for {reference}: {stdout}")) + .to_owned() + }; + let (failed, published) = if status("refs/heads/a").starts_with('!') { + ("refs/heads/a", "refs/heads/b") + } else { + ("refs/heads/b", "refs/heads/a") + }; + assert!(status(failed).starts_with('!'), "{stdout}"); + assert!(!status(published).starts_with('!'), "{stdout}"); + + let manifest = + EncryptedRepository::new(GitStorage::open(carrier.to_str().unwrap()).unwrap(), key) + .verify() + .unwrap(); + let names: Vec<_> = manifest.refs.keys().map(String::as_str).collect(); + assert!(names.contains(&published), "{names:?}"); + assert!( + !names.contains(&failed), + "the failed ref was published by the next one: {names:?}" + ); + + assert!( + !stdout.contains("fetch first") && !stderr.contains("fetch first"), + "a rejected publish is not reported as a race: stdout={stdout} stderr={stderr}" + ); + assert!( + stderr.contains("carrier test hook refused the update"), + "{stderr}" + ); +}