max / makenotwork
- Co-Authored-By
- Claude Opus 5 (1M context) <noreply@anthropic.com>
2 files changed,
+126 insertions,
-23 deletions
| @@ -521,12 +521,19 @@ | |||
| 521 | 521 | /// has checked out. Fetch + checkout stream into the current step's log; | |
| 522 | 522 | /// the sha comes from a separate `rev-parse` so its stdout is only the sha. | |
| 523 | 523 | fn checkout_sha(self: &Arc<Self>, host: &str) -> Result<String> { | |
| 524 | - | let (code, _) = self.run(host, &git_fetch_checkout_cmd(&self.repo, &self.version))?; | |
| 525 | - | anyhow::ensure!( | |
| 526 | - | code == 0, | |
| 527 | - | "checkout of v{} failed on `{host}` (is the tag pushed?)", | |
| 528 | - | self.version | |
| 529 | - | ); | |
| 524 | + | // A failing mirror is not a failing release: fetch is advisory, and only | |
| 525 | + | // the checkout decides. Its output still streams into the step log, so an | |
| 526 | + | // unreachable remote stays visible without being fatal. | |
| 527 | + | let _ = self.run(host, &git_fetch_cmd(&self.repo))?; | |
| 528 | + | let (code, _) = self.run(host, &git_checkout_tag_cmd(&self.repo, &self.version))?; | |
| 529 | + | if code != 0 { | |
| 530 | + | let (probe, _) = self.run(host, &git_tag_exists_cmd(&self.repo, &self.version))?; | |
| 531 | + | anyhow::bail!( | |
| 532 | + | "checkout of v{} failed on `{host}`: {}", | |
| 533 | + | self.version, | |
| 534 | + | checkout_failure_reason(&self.version, probe == 0) | |
| 535 | + | ); | |
| 536 | + | } | |
| 530 | 537 | let (code, tail) = self.run(host, &git_rev_parse_cmd(&self.repo))?; | |
| 531 | 538 | anyhow::ensure!(code == 0, "rev-parse failed on `{host}`"); | |
| 532 | 539 | Ok(tail.trim().to_string()) | |
| @@ -848,14 +855,50 @@ | |||
| 848 | 855 | s | |
| 849 | 856 | } | |
| 850 | 857 | ||
| 851 | - | /// The command that pins a host's checkout to the release tag: fetch everything | |
| 852 | - | /// (no branch/upstream assumptions — a bare `git pull --ff-only` needs a | |
| 853 | - | /// tracking branch the release path shouldn't depend on), then check out the | |
| 854 | - | /// tag. `repo` is interpolated UNQUOTED so a leading `~` is expanded by the | |
| 855 | - | /// remote host's shell (the checkout path is trusted topology config, not user | |
| 856 | - | /// input), matching how the recipes `cd` into it. | |
| 857 | - | pub fn git_fetch_checkout_cmd(repo: &str, version: &Version) -> String { | |
| 858 | - | format!("git -C {repo} fetch --all --tags --prune && git -C {repo} checkout \"v{version}\"") | |
| 858 | + | /// Refresh every remote's refs and tags, so the tag a release names is present | |
| 859 | + | /// locally however it was pushed. No branch/upstream assumptions — a bare | |
| 860 | + | /// `git pull --ff-only` needs a tracking branch the release path shouldn't | |
| 861 | + | /// depend on. | |
| 862 | + | /// | |
| 863 | + | /// Best-effort on purpose. `fetch --all` exits non-zero if ANY remote fails, and | |
| 864 | + | /// the library repos carry three (`astra`, `mnw`, `srht`), so chaining this into | |
| 865 | + | /// the checkout with `&&` meant one unreachable mirror aborted the release and | |
| 866 | + | /// reported it as a missing tag. The checkout below is the step allowed to fail; | |
| 867 | + | /// this one only has to try. See [`git_checkout_tag_cmd`]. | |
| 868 | + | /// | |
| 869 | + | /// `repo` is interpolated UNQUOTED so a leading `~` is expanded by the remote | |
| 870 | + | /// host's shell (the checkout path is trusted topology config, not user input), | |
| 871 | + | /// matching how the recipes `cd` into it. | |
| 872 | + | pub fn git_fetch_cmd(repo: &str) -> String { | |
| 873 | + | format!("git -C {repo} fetch --all --tags --prune") | |
| 874 | + | } | |
| 875 | + | ||
| 876 | + | /// Pin the host's checkout to the release tag. Runs after [`git_fetch_cmd`], and | |
| 877 | + | /// is the operation whose exit code decides whether the release proceeds. | |
| 878 | + | pub fn git_checkout_tag_cmd(repo: &str, version: &Version) -> String { | |
| 879 | + | format!("git -C {repo} checkout \"v{version}\"") | |
| 880 | + | } | |
| 881 | + | ||
| 882 | + | /// Does `v<version>` resolve to a commit in this checkout? Run only when the | |
| 883 | + | /// checkout has already failed, to say WHY: an absent tag is an untagged or | |
| 884 | + | /// unpushed release, while a tag that resolves fine means the checkout was | |
| 885 | + | /// refused for a local reason (a dirty tree, most often) and the operator needs | |
| 886 | + | /// to hear that instead. | |
| 887 | + | pub fn git_tag_exists_cmd(repo: &str, version: &Version) -> String { | |
| 888 | + | format!("git -C {repo} rev-parse -q --verify \"refs/tags/v{version}^{{commit}}\"") | |
| 889 | + | } | |
| 890 | + | ||
| 891 | + | /// The operator-facing explanation for a failed tag checkout, given whether the | |
| 892 | + | /// tag turned out to exist locally. | |
| 893 | + | pub fn checkout_failure_reason(version: &Version, tag_exists: bool) -> String { | |
| 894 | + | if tag_exists { | |
| 895 | + | format!( | |
| 896 | + | "tag v{version} exists but could not be checked out \ | |
| 897 | + | (uncommitted changes in the checkout?)" | |
| 898 | + | ) | |
| 899 | + | } else { | |
| 900 | + | format!("tag v{version} does not exist there (is it created and pushed?)") | |
| 901 | + | } | |
| 859 | 902 | } | |
| 860 | 903 | ||
| 861 | 904 | /// The command a host runs to report the commit it has checked out, for the |
| @@ -61,19 +61,30 @@ | |||
| 61 | 61 | .ok_or_else(|| anyhow::anyhow!("no executor for host `{host}`"))?; | |
| 62 | 62 | let mut sink = DiscardSink; | |
| 63 | 63 | ||
| 64 | - | let checkout = OpStep::shell( | |
| 65 | - | Action::Build, | |
| 66 | - | engine::git_fetch_checkout_cmd(&repo, version), | |
| 67 | - | ); | |
| 64 | + | // Advisory: `fetch --all` is non-zero if any one of a repo's remotes is | |
| 65 | + | // unreachable, and blaming the tag for a dead mirror is what made this | |
| 66 | + | // preflight misreport. Only the checkout below is allowed to fail. | |
| 67 | + | let fetch = OpStep::shell(Action::Build, engine::git_fetch_cmd(&repo)); | |
| 68 | + | let _ = exec.run_streaming(&fetch, &mut sink).await; | |
| 69 | + | ||
| 70 | + | let checkout = OpStep::shell(Action::Build, engine::git_checkout_tag_cmd(&repo, version)); | |
| 68 | 71 | let out = exec | |
| 69 | 72 | .run_streaming(&checkout, &mut sink) | |
| 70 | 73 | .await | |
| 71 | 74 | .with_context(|| format!("release preflight: checkout on `{host}`"))?; | |
| 72 | - | anyhow::ensure!( | |
| 73 | - | out.status.success(), | |
| 74 | - | "release preflight: `git checkout v{version}` failed on `{host}` \ | |
| 75 | - | (is the tag pushed to a remote every host can fetch?)" | |
| 76 | - | ); | |
| 75 | + | if !out.status.success() { | |
| 76 | + | // Ask git why before telling the operator. The two causes need | |
| 77 | + | // opposite responses: tag-and-push, versus clean the working tree. | |
| 78 | + | let probe = OpStep::shell(Action::Build, engine::git_tag_exists_cmd(&repo, version)); | |
| 79 | + | let tag_exists = exec | |
| 80 | + | .run_streaming(&probe, &mut sink) | |
| 81 | + | .await | |
| 82 | + | .is_ok_and(|o| o.status.success()); | |
| 83 | + | anyhow::bail!( | |
| 84 | + | "release preflight: `git checkout v{version}` failed on `{host}`: {}", | |
| 85 | + | engine::checkout_failure_reason(version, tag_exists) | |
| 86 | + | ); | |
| 87 | + | } | |
| 77 | 88 | ||
| 78 | 89 | let rev = OpStep::shell(Action::Build, engine::git_rev_parse_cmd(&repo)); | |
| 79 | 90 | let out = exec | |
| @@ -1970,6 +1981,10 @@ | |||
| 1970 | 1981 | msg.contains("release preflight") && msg.contains("v0.0.2"), | |
| 1971 | 1982 | "expected a preflight tag error, got: {msg}" | |
| 1972 | 1983 | ); | |
| 1984 | + | assert!( | |
| 1985 | + | msg.contains("does not exist"), | |
| 1986 | + | "the error should name the absent tag as the cause, got: {msg}" | |
| 1987 | + | ); | |
| 1973 | 1988 | // Refused before anything was recorded. | |
| 1974 | 1989 | let builds: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM builds") | |
| 1975 | 1990 | .fetch_one(&pool) | |
| @@ -1978,6 +1993,51 @@ | |||
| 1978 | 1993 | assert_eq!(builds, 0, "a refused preflight writes no build row"); | |
| 1979 | 1994 | } | |
| 1980 | 1995 | ||
| 1996 | + | /// An unreachable remote does not fail a release whose tag is present. | |
| 1997 | + | /// | |
| 1998 | + | /// The preflight used to run `fetch --all --tags --prune && checkout`, and | |
| 1999 | + | /// `fetch --all` is non-zero if ANY remote fails. Every library repo carries | |
| 2000 | + | /// three (`astra`, `mnw`, `srht`), so one dead mirror aborted the release and | |
| 2001 | + | /// blamed it on a missing tag — which is how makeover v2.1.1 failed on | |
| 2002 | + | /// 2026-07-26 four minutes after its tag was created. Fetch is advisory now; | |
| 2003 | + | /// only the checkout decides. | |
| 2004 | + | #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | |
| 2005 | + | async fn a_dead_remote_does_not_fail_a_release_whose_tag_exists() { | |
| 2006 | + | let tmp = tempfile::tempdir().unwrap(); | |
| 2007 | + | let repo = tmp.path().join("demo"); | |
| 2008 | + | init_git_app(&repo, "0.0.1", Some("v0.0.1")); | |
| 2009 | + | // A remote that cannot possibly be fetched, standing in for an offline | |
| 2010 | + | // astra or an srht mirror the repo was never pushed to. | |
| 2011 | + | let out = std::process::Command::new("git") | |
| 2012 | + | .args([ | |
| 2013 | + | "remote", | |
| 2014 | + | "add", | |
| 2015 | + | "srht", | |
| 2016 | + | &tmp.path().join("nowhere.git").display().to_string(), | |
| 2017 | + | ]) | |
| 2018 | + | .current_dir(&repo) | |
| 2019 | + | .env("GIT_CONFIG_GLOBAL", "/dev/null") | |
| 2020 | + | .env("GIT_CONFIG_SYSTEM", "/dev/null") | |
| 2021 | + | .output() | |
| 2022 | + | .expect("git runs"); | |
| 2023 | + | assert!(out.status.success()); | |
| 2024 | + | ||
| 2025 | + | let mut cfg = Config::for_tests(tmp.path()); | |
| 2026 | + | cfg.pin_release_sha = true; | |
| 2027 | + | let pool = crate::db::open(&cfg.db_path).await.unwrap(); | |
| 2028 | + | let state = test_state(pool.clone(), one_host_topo(&repo), cfg); | |
| 2029 | + | let build_id = start_build( | |
| 2030 | + | state, | |
| 2031 | + | AppId::new("demo"), | |
| 2032 | + | Version::parse("0.0.1").unwrap(), | |
| 2033 | + | vec!["linux/x86_64".parse().unwrap()], | |
| 2034 | + | ) | |
| 2035 | + | .await | |
| 2036 | + | .expect("a dead mirror must not refuse the release"); | |
| 2037 | + | let (status, error) = await_target(&pool, build_id).await; | |
| 2038 | + | assert_eq!(status, "ok", "the tag is present, so this builds ({error})"); | |
| 2039 | + | } | |
| 2040 | + | ||
| 1981 | 2041 | /// The audit fix: a `build` step dispatched to a host whose executor lacks the | |
| 1982 | 2042 | /// `build` capability is denied at the transport BEFORE the command runs. This | |
| 1983 | 2043 | /// is the structural guarantee behind "never build on prod" — a recipe naming |