Skip to main content

max / makenotwork

bento: put the checkout back on its branch after a release The preflight pins every host to v<version>, which detaches HEAD, and nothing put it back. That half is correct -- a release must build the tagged commit, not the branch tip -- but leaving the tree detached is invisible: git does not warn, and commits made afterwards succeed while belonging to no branch. makeover shipped 2.3.0 from that state. Its main was three commits behind at v2.2.0, and the published commit existed only as a detached HEAD on one machine, on no branch and on none of its three remotes. A checkout of main there would have orphaned the source cargo had already uploaded. pin_release now records each host's branch before pinning and finalize_build checks it back out once the targets settle, skipping the restore while a superseding build for the same app still holds the checkout. A host that is ALREADY detached is refused: there is no branch to restore it to, and that state means an earlier release never cleaned up.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-28 23:21 UTC
Signed with PGP, not checked
Commit: 099174c84c8b19522f02579dc691af3eb89b50d9
Parent: d9262fa
2 files changed, +220 insertions, -10 deletions
M bento/daemon/src/engine.rs +20
@@ -901,6 +901,26 @@
901 901 }
902 902 }
903 903
904 + /// The branch a host's checkout is on, empty (and non-zero) on a detached HEAD.
905 + /// Read BEFORE the release pins the tag, so the checkout can be put back
906 + /// afterwards — see [`git_restore_branch_cmd`].
907 + pub fn git_current_branch_cmd(repo: &str) -> String {
908 + format!("git -C {repo} symbolic-ref -q --short HEAD")
909 + }
910 +
911 + /// Put a checkout back on the branch it was on before the release pinned it to
912 + /// the tag.
913 + ///
914 + /// The pin itself is correct and deliberate: a release must build the tagged
915 + /// commit, not the branch tip. What was missing is the other half. Leaving the
916 + /// tree detached is invisible — git does not warn, and commits made afterwards
917 + /// succeed normally while belonging to no branch. makeover shipped 2.3.0 from
918 + /// exactly that state on 2026-07-28: three commits, including the published one,
919 + /// existed only as a detached HEAD on one machine, on no branch and no remote.
920 + pub fn git_restore_branch_cmd(repo: &str, branch: &str) -> String {
921 + format!("git -C {repo} checkout \"{branch}\"")
922 + }
923 +
904 924 /// The command a host runs to report the commit it has checked out, for the
905 925 /// release preflight barrier.
906 926 pub fn git_rev_parse_cmd(repo: &str) -> String {
M bento/daemon/src/runner.rs +200 -10
@@ -31,12 +31,18 @@
31 31 /// each built whatever `main` was at pull time and the artifacts were filed
32 32 /// under the daemon host's version. This runs before any target task spawns, so
33 33 /// a mixed-source release is stopped before a single artifact is built.
34 + ///
35 + /// Returns the branch each host was on before it was pinned, for
36 + /// [`restore_branches`] to put back once the build settles. A host that is
37 + /// ALREADY detached has no branch to return to, and is refused: that state means
38 + /// some earlier release never cleaned up, and any commits made in the meantime
39 + /// are sitting on no branch at all.
34 40 async fn pin_release(
35 41 state: &AppState,
36 42 app: &AppId,
37 43 version: &Version,
38 44 targets: &[Target],
39 - ) -> Result<()> {
45 + ) -> Result<Vec<(String, String)>> {
40 46 let cfg = state
41 47 .topo
42 48 .app(app)
@@ -54,6 +60,7 @@
54 60 }
55 61
56 62 let mut shas: Vec<(String, String)> = Vec::new();
63 + let mut branches: Vec<(String, String)> = Vec::new();
57 64 for host in &hosts {
58 65 let exec = state
59 66 .executors
@@ -61,6 +68,23 @@
61 68 .ok_or_else(|| anyhow::anyhow!("no executor for host `{host}`"))?;
62 69 let mut sink = DiscardSink;
63 70
71 + // Read the branch BEFORE pinning, while there is still one to read.
72 + let branch_cmd = OpStep::shell(Action::Build, engine::git_current_branch_cmd(&repo));
73 + let out = exec
74 + .run_streaming(&branch_cmd, &mut sink)
75 + .await
76 + .with_context(|| format!("release preflight: reading branch on `{host}`"))?;
77 + let branch = String::from_utf8_lossy(&out.stdout).trim().to_string();
78 + anyhow::ensure!(
79 + out.status.success() && !branch.is_empty(),
80 + "release preflight: `{repo}` on `{host}` is on a detached HEAD, so there is \
81 + no branch to restore it to after the release. An earlier release left it \
82 + that way; any commits made since are on no branch and may exist nowhere \
83 + else. Reattach it (`git checkout <branch>`, fast-forwarding if the commits \
84 + should be kept) before releasing."
85 + );
86 + branches.push((host.clone(), branch));
87 +
64 88 // Advisory: `fetch --all` is non-zero if any one of a repo's remotes is
65 89 // unreachable, and blaming the tag for a dead mirror is what made this
66 90 // preflight misreport. Only the checkout below is allowed to fail.
@@ -115,7 +139,50 @@
115 139 mismatch.join(", "),
116 140 );
117 141 }
118 - Ok(())
142 + Ok(branches)
143 + }
144 +
145 + /// Put every host's checkout back on the branch [`pin_release`] found it on.
146 + ///
147 + /// Best-effort and non-fatal: the release itself is already decided by the time
148 + /// this runs, and failing a green build because a `git checkout` did not take
149 + /// would be worse than the detached tree it is cleaning up. A failure is logged
150 + /// loudly instead, because the state it leaves behind is the silent one.
151 + async fn restore_branches(state: &AppState, app: &AppId, branches: &[(String, String)]) {
152 + let Some(cfg) = state.topo.app(app) else {
153 + return;
154 + };
155 + for (host, branch) in branches {
156 + let Some(exec) = state.executors.get(host) else {
157 + continue;
158 + };
159 + // Serialize against a target still holding this host for its recipe run,
160 + // so the restore can't move the tree mid-build.
161 + let _host_guard = match state.host_locks.get(host).cloned() {
162 + Some(lock) => Some(lock.lock_owned().await),
163 + None => None,
164 + };
165 + let mut sink = DiscardSink;
166 + let step = OpStep::shell(
167 + Action::Build,
168 + engine::git_restore_branch_cmd(&cfg.repo, branch),
169 + );
170 + match exec.run_streaming(&step, &mut sink).await {
171 + Ok(out) if out.status.success() => {
172 + tracing::debug!(%host, %branch, "restored checkout to its branch");
173 + }
174 + Ok(_) => tracing::error!(
175 + %host, %branch, repo = %cfg.repo,
176 + "could not restore the checkout to its branch; it is left DETACHED at the \
177 + release tag, and commits made there will belong to no branch"
178 + ),
179 + Err(e) => tracing::error!(
180 + %host, %branch, repo = %cfg.repo, error = %e,
181 + "could not restore the checkout to its branch; it is left DETACHED at the \
182 + release tag, and commits made there will belong to no branch"
183 + ),
184 + }
185 + }
119 186 }
120 187
121 188 /// First 12 chars of a sha for a readable error.
@@ -191,11 +258,13 @@
191 258
192 259 // Pin every build host to the release tag and verify they agree, before any
193 260 // target task spawns. Off in tests (their repos aren't git checkouts).
194 - if state.cfg.pin_release_sha {
261 + let pinned_branches = if state.cfg.pin_release_sha {
195 262 pin_release(&state, &app, &version, &targets)
196 263 .await
197 - .context("release preflight")?;
198 - }
264 + .context("release preflight")?
265 + } else {
266 + Vec::new()
267 + };
199 268
200 269 let build_id: i64 = sqlx::query_scalar(
201 270 "INSERT INTO builds (app, version, status, created_at) VALUES (?, ?, 'running', ?) RETURNING id",
@@ -266,9 +335,9 @@
266 335 crate::metrics::set_in_flight(active.len());
267 336 }
268 337
269 - // Mark the build done once all target tasks settle. Spawned so /build
270 - // returns immediately.
271 - tokio::spawn(finalize_build(state, build_id, set));
338 + // Mark the build done once all target tasks settle, and put the pinned
339 + // checkouts back on their branches. Spawned so /build returns immediately.
340 + tokio::spawn(finalize_build(state, build_id, set, app, pinned_branches));
272 341 Ok(build_id)
273 342 }
274 343
@@ -557,7 +626,13 @@
557 626 /// the async wrappers. The old code aborted only the wrappers, which could not
558 627 /// reach the blocking bodies, so a build was marked failed while codesign kept
559 628 /// running on the mac.
560 - async fn finalize_build(state: AppState, build_id: i64, mut set: tokio::task::JoinSet<()>) {
629 + async fn finalize_build(
630 + state: AppState,
631 + build_id: i64,
632 + mut set: tokio::task::JoinSet<()>,
633 + app: AppId,
634 + pinned_branches: Vec<(String, String)>,
635 + ) {
561 636 const MAX_WAIT: std::time::Duration = std::time::Duration::from_hours(6);
562 637 let deadline = tokio::time::Instant::now() + MAX_WAIT;
563 638 loop {
@@ -594,10 +669,25 @@
594 669
595 670 // Reap this build's latest-wins slots (only our own — a superseding build
596 671 // owns a different build_id and is left intact).
597 - {
672 + let app_still_building = {
598 673 let mut active = state.active.lock().await;
599 674 active.retain(|_, slot| slot.build_id != build_id);
600 675 crate::metrics::set_in_flight(active.len());
676 + active.keys().any(|(a, _)| a == &app)
677 + };
678 +
679 + // Undo the preflight's pin. Skipped if a superseding build for this same app
680 + // still holds the checkout — it pinned the tree to ITS tag, and restoring the
681 + // branch here would move that build off the commit it is releasing.
682 + if !pinned_branches.is_empty() {
683 + if app_still_building {
684 + tracing::debug!(
685 + build_id, %app,
686 + "leaving the checkout pinned; a superseding build for this app is still running"
687 + );
688 + } else {
689 + restore_branches(&state, &app, &pinned_branches).await;
690 + }
601 691 }
602 692
603 693 let now = chrono::Utc::now().to_rfc3339();
@@ -1993,6 +2083,106 @@
1993 2083 assert_eq!(builds, 0, "a refused preflight writes no build row");
1994 2084 }
1995 2085
2086 + /// The branch a release was launched from is still checked out afterwards.
2087 + ///
2088 + /// The preflight pins every host to `v<version>`, which detaches HEAD. That
2089 + /// is correct during the build and wrong to leave behind: git does not warn,
2090 + /// and later commits succeed while belonging to no branch. makeover shipped
2091 + /// 2.3.0 from exactly that state on 2026-07-28 — the published commit existed
2092 + /// only as a detached HEAD on one machine, on no branch and no remote.
2093 + #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
2094 + async fn a_release_leaves_the_checkout_on_its_branch() {
2095 + let tmp = tempfile::tempdir().unwrap();
2096 + let repo = tmp.path().join("demo");
2097 + init_git_app(&repo, "0.0.1", Some("v0.0.1"));
2098 + let branch_before = current_branch(&repo);
2099 + assert!(!branch_before.is_empty(), "test repo starts on a branch");
2100 +
2101 + let mut cfg = Config::for_tests(tmp.path());
2102 + cfg.pin_release_sha = true;
2103 + let pool = crate::db::open(&cfg.db_path).await.unwrap();
2104 + let state = test_state(pool.clone(), one_host_topo(&repo), cfg);
2105 + let build_id = start_build(
2106 + state,
2107 + AppId::new("demo"),
2108 + Version::parse("0.0.1").unwrap(),
2109 + vec!["linux/x86_64".parse().unwrap()],
2110 + )
2111 + .await
2112 + .unwrap();
2113 + let (status, error) = await_target(&pool, build_id).await;
2114 + assert_eq!(status, "ok", "the build itself should pass ({error})");
2115 +
2116 + // finalize_build restores after the targets settle, so give it a moment.
2117 + for _ in 0..50 {
2118 + if current_branch(&repo) == branch_before {
2119 + break;
2120 + }
2121 + tokio::time::sleep(std::time::Duration::from_millis(20)).await;
2122 + }
2123 + assert_eq!(
2124 + current_branch(&repo),
2125 + branch_before,
2126 + "the release must put the checkout back on its branch, not leave it detached"
2127 + );
2128 + }
2129 +
2130 + /// A checkout that is ALREADY detached is refused, rather than released from
2131 + /// and left detached again. It means an earlier release never cleaned up, and
2132 + /// anything committed since is on no branch — which is precisely the state
2133 + /// that has to be looked at by a human before more releases pile onto it.
2134 + #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
2135 + async fn a_detached_checkout_is_refused_before_it_is_pinned_again() {
2136 + let tmp = tempfile::tempdir().unwrap();
2137 + let repo = tmp.path().join("demo");
2138 + init_git_app(&repo, "0.0.1", Some("v0.0.1"));
2139 + // Detach, standing in for a checkout an earlier release left on its tag.
2140 + let out = std::process::Command::new("git")
2141 + .args(["checkout", "--detach", "-q", "HEAD"])
2142 + .current_dir(&repo)
2143 + .env("GIT_CONFIG_GLOBAL", "/dev/null")
2144 + .env("GIT_CONFIG_SYSTEM", "/dev/null")
2145 + .output()
2146 + .expect("git runs");
2147 + assert!(out.status.success());
2148 + assert!(current_branch(&repo).is_empty(), "repo is detached");
2149 +
2150 + let mut cfg = Config::for_tests(tmp.path());
2151 + cfg.pin_release_sha = true;
2152 + let pool = crate::db::open(&cfg.db_path).await.unwrap();
2153 + let state = test_state(pool.clone(), one_host_topo(&repo), cfg);
2154 + let err = start_build(
2155 + state,
2156 + AppId::new("demo"),
2157 + Version::parse("0.0.1").unwrap(),
2158 + vec!["linux/x86_64".parse().unwrap()],
2159 + )
2160 + .await
2161 + .unwrap_err();
2162 + let msg = format!("{err:#}");
2163 + assert!(
2164 + msg.contains("detached HEAD"),
2165 + "the refusal should name the detached checkout, got: {msg}"
2166 + );
2167 + let builds: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM builds")
2168 + .fetch_one(&pool)
2169 + .await
2170 + .unwrap();
2171 + assert_eq!(builds, 0, "a refused preflight writes no build row");
2172 + }
2173 +
2174 + /// The branch `repo` is on, empty on a detached HEAD.
2175 + fn current_branch(repo: &std::path::Path) -> String {
2176 + let out = std::process::Command::new("git")
2177 + .args(["symbolic-ref", "-q", "--short", "HEAD"])
2178 + .current_dir(repo)
2179 + .env("GIT_CONFIG_GLOBAL", "/dev/null")
2180 + .env("GIT_CONFIG_SYSTEM", "/dev/null")
2181 + .output()
2182 + .expect("git runs");
2183 + String::from_utf8_lossy(&out.stdout).trim().to_string()
2184 + }
2185 +
1996 2186 /// An unreachable remote does not fail a release whose tag is present.
1997 2187 ///
1998 2188 /// The preflight used to run `fetch --all --tags --prune && checkout`, and