diff --git a/src/storage.rs b/src/storage.rs index f4dd09d..8a6ca1a 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -1033,21 +1033,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>> { @@ -1210,14 +1202,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); @@ -1237,18 +1251,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 { @@ -1312,7 +1387,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()); } @@ -1330,7 +1405,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); @@ -1377,7 +1452,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()); } @@ -1398,12 +1473,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 b9a272e..5bc9e2a 100644 --- a/tests/carrier_git.rs +++ b/tests/carrier_git.rs @@ -1807,6 +1807,209 @@ 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}" + ); +} + #[test] fn carrier_recovery_classifies_missing_head_manifest_as_unavailable() { let temporary = tempfile::tempdir().unwrap();