From c41c36f25d96fa1cd70f08dbbd4c1d75582200c9 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 6 Sep 2026 13:45:45 -0400 Subject: [PATCH] Claiming the whole project means what's stale, not everything built here after_moving (Pull and Update's shared machinery) gated a post-pull build on `pulled || is_stale(None)` and then, once open, `claim(&mut inner, None)` -- which marks every component with a build command regardless of whether it was the one that moved. `trigger_if_needed(None, ...)` had the same shape one level up: check is_stale, then hand off to build_now, which is unconditional by design. So any pull that fetched anything rebuilt every sibling beside the one that actually changed -- "it tries to update the compose apk even if nothing was done." `claim` now takes `only_stale`: false for build_now (a person pressed a specific button, and staleness has no business overruling that), true for trigger_if_needed and for the build after_moving runs after a pull. component_is_stale was already scoped correctly per component; the bug was building past that answer once any one component tripped it. MismatchedPairNote also had its own word for the same "behind" freshness freshnessNote already calls "out of date" -- "older than this build" was a second name for one measurement, which is what "pulling says something weird about the compose apk" turned out to be. Reused the existing word. Regression test: a_pull_touching_one_component_does_not_rebuild_its_sibling. Co-Authored-By: Claude Sonnet 5 --- AGENTS.md | 26 +++ .../com/example/devupdater/UpdaterScreen.kt | 9 +- server/src/build_state.rs | 178 ++++++++++++++++-- 3 files changed, 194 insertions(+), 19 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 56291d0..32160cc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1320,6 +1320,32 @@ mutable at runtime from the phone. version. It follows that a build and an install landing in the same second can briefly read as "update available"; that's inherent, not a bug to chase. +- **Claiming "the whole project" must still mean "what's actually + stale," not "everything with a command."** `after_moving` (what Pull + and Update share) used to gate a post-pull build on + `pulled || is_stale(None)` and then, once open, `claim(&mut inner, + None)` -- which marks *every* component with a build command, stale or + not. `trigger_if_needed(None, ...)` had the same shape: check + `is_stale`, then hand off to `build_now`, which is unconditional by + design (it is also the button). So any pull that fetched anything -- + a one-line fix under `server/`, say -- rebuilt the APK beside it too, + reported as "it tries to update the compose apk even if nothing was + done." `claim` now takes `only_stale`: `false` for `build_now` (a + person pressed a specific button, and staleness has no business + overruling that), `true` for `trigger_if_needed` and for the build + `after_moving` runs after a pull. `component_is_stale` was already + scoped correctly per component (`watched_paths`) -- the bug was never + in *what* counts as stale, only in building past that answer once any + one component tripped it. Regression test: + `a_pull_touching_one_component_does_not_rebuild_its_sibling`. +- **Two words for one measurement is a bug even when both are true.** + `MismatchedPairNote` said a sibling still on its old build was "older + than this build," while `freshnessNote` already has a word for that + exact freshness (`behind` → "out of date"). Same field, two names -- + which is what "pulling says something weird about the compose apk" + turned out to be: not a wrong condition, a second vocabulary for a + state the reader had already been told about elsewhere on the same + card. Reuse the existing word rather than inventing an adjacent one. ## Running the server for real diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt index 58c9e20..fdae4b5 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -3146,8 +3146,13 @@ private fun MismatchedPairNote(self: ProjectComponent, others: List, component: Option<&str>, record: RecordBuilt) { - // Before the lock, not inside it -- see `is_stale`. - if self.is_stale(component) { - self.build_now(component, record); - } + let claimed = { + let mut inner = self.inner.lock().unwrap(); + // The one thing a component still waits behind. See + // `Inner::pulling`. + if inner.pulling { + return; + } + // `true` is the whole of this method: claim only what is + // individually stale, so naming no component still means "the + // whole project" while leaving every component already + // current alone. Passing `component` through unfiltered here + // used to build every component with a command the moment any + // one of them was stale -- pressing Update to bring in a + // one-line server fix rebuilt the APK beside it every time. + self.claim(&mut inner, component, true) + }; + self.start(claimed, record); } /// Builds whether or not anything looks stale, for somebody who asked. @@ -514,7 +527,7 @@ impl BuildState { if inner.pulling { return; } - self.claim(&mut inner, component) + self.claim(&mut inner, component, false) }; self.start(claimed, record); } @@ -522,6 +535,15 @@ impl BuildState { /// Marks each component `name` selects that has something to build /// and is not already building, and answers where they are. /// + /// `only_stale` is the whole difference between a person's button and + /// an automatic trigger: `false` for `build_now`, where somebody + /// pressed a specific button and every component `name` selects + /// starts regardless of what it looks like; `true` for + /// `trigger_if_needed`, where naming no component means "the whole + /// project" and that must not mean "every component with a command, + /// stale or not" -- a pull that only touched one component's + /// directory has no business rebuilding the others beside it. + /// /// Takes the guard rather than the lock so that a caller can do this /// and something else in one step -- which the pull needs: releasing /// `pulling` and claiming what it decided to build have to happen @@ -533,7 +555,7 @@ impl BuildState { /// returns, so a status read in that window would otherwise find the /// component idle -- and idle is exactly what the phone is waiting /// for. - fn claim(&self, inner: &mut Inner, name: Option<&str>) -> Vec { + fn claim(&self, inner: &mut Inner, name: Option<&str>, only_stale: bool) -> Vec { let mut claimed = Vec::new(); for (index, component) in self.components.iter().enumerate() { if name.is_some_and(|name| component.name() != name) { @@ -545,6 +567,9 @@ impl BuildState { if inner.component_running(component.name()) { continue; } + if only_stale && !self.component_is_stale(component, &inner.built_from) { + continue; + } // Whatever the last run left against this component goes now, // so nothing from it can be read as this run's outcome -- an // old error in particular, which the phone treats as a reason @@ -899,26 +924,32 @@ impl BuildState { tracing::error!("moving the checkout failed: {}", error.message); this.fail_pull(error); } - Ok(pulled) => { - // Nothing configured to build, nothing allowed to, or - // a move that builds nothing by definition: the move - // was the whole job. + Ok(_) => { + // Nothing configured to build, or nothing allowed to: + // the move was the whole job. Which components (if + // any) actually build is `claim`'s question, not this + // one -- a pull that fetched something is not itself a + // reason to rebuild a component whose own directory + // the fetch never touched. That used to be folded in + // here as `pulled || this.is_stale(None)`, which + // meant *any* pull rebuilt *every* component with a + // command: pressing Update to bring in a one-line + // server fix rebuilt the APK beside it every time, + // whether or not `app/` had moved. // `may_build()` is called here, with the pulled // declaration on disk, for the reason in the doc - // comment above. Both it and `is_stale` take the lock - // themselves, so they are asked before it is held. - let build = then_build + // comment above. + let may_build = then_build .as_ref() .is_some_and(|(may_build, _)| may_build()) - && this.has_command() - && (pulled || this.is_stale(None)); + && this.has_command(); // Handing the run over in one step, so nothing can // observe the moment between the pull ending and the // builds it decided on starting -- see `claim`. let claimed = { let mut inner = this.inner.lock().unwrap(); - let claimed = match build { - true => this.claim(&mut inner, None), + let claimed = match may_build { + true => this.claim(&mut inner, None, true), false => Vec::new(), }; inner.pulling = false; @@ -1679,6 +1710,119 @@ mod tests { ); } + /// Update pulls the whole project, but a commit under only one + /// component's directory must not rebuild the other. + /// + /// The bug this guards: `after_moving` used to gate the whole build on + /// `pulled || is_stale(None)` and then claim *every* component with a + /// command once that gate opened, rather than only the ones + /// `component_is_stale` actually flagged. So any pull that fetched + /// anything -- a one-line fix to `server/`, say -- rebuilt every + /// sibling beside it too, which on a real project meant pressing + /// Update to bring in a backend fix also rebuilt the APK for no + /// reason. Reported as "it tries to update the compose apk even if + /// nothing was done." + #[tokio::test] + async fn a_pull_touching_one_component_does_not_rebuild_its_sibling() { + let dir = tempfile::tempdir().expect("tempdir"); + let origin = dir.path().join("origin"); + std::fs::create_dir_all(origin.join("server")).expect("mkdir"); + // Committed rather than left untracked: `pull` refuses to run over + // uncommitted changes, and `nothing_built` asks the filesystem for + // an Apk's own output regardless of git, so without something + // here the "app" component reads as never-built on every round -- + // stale for a reason that has nothing to do with what this test + // is checking. + std::fs::create_dir_all(origin.join("app/build/outputs/apk/debug")).expect("mkdir"); + std::fs::write(origin.join("app/build/outputs/apk/debug/a.apk"), b"").expect("write"); + std::fs::write(origin.join("server/main.rs"), "one").expect("write"); + std::fs::write(origin.join("app/main.kt"), "one").expect("write"); + for args in [ + &["git", "init", "-q", "-b", "main"][..], + &["git", "config", "user.email", "t@example.com"], + &["git", "config", "user.name", "Test"], + &["git", "add", "-A"], + &["git", "commit", "-qm", "one"], + ] { + run(&origin, args); + } + let clone = dir.path().join("clone"); + run( + dir.path(), + &[ + "git", + "clone", + "-q", + origin.to_str().unwrap(), + clone.to_str().unwrap(), + ], + ); + + let components = vec![ + Component::Server { + name: "backend".to_string(), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line( + "touch backend-built", + )), + cwd: Some(PathBuf::from("server")), + stale_when: None, + also_watch: Vec::new(), + service: None, + mode: None, + built_from: None, + built_mode: None, + }, + Component::Apk { + name: "app".to_string(), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line( + "touch app-built", + )), + cwd: Some(PathBuf::from("app")), + stale_when: None, + also_watch: Vec::new(), + strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + mode: None, + package: None, + built_from: None, + built_mode: None, + }, + ]; + let state = state_for(&clone, components); + + // Settle both components once, so each has a recorded commit to be + // "current" against -- otherwise both would build regardless, as + // "never built" rather than as a pull bringing something in. + state.build_now(None, Arc::new(|_: &str, _: String| {})); + while state.status().building { + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + assert!(clone.join("server/backend-built").exists()); + assert!(clone.join("app/app-built").exists()); + std::fs::remove_file(clone.join("server/backend-built")).expect("rm"); + std::fs::remove_file(clone.join("app/app-built")).expect("rm"); + + // A commit under the remote's `server/` alone, then Update. + std::fs::write(origin.join("server/main.rs"), "two").expect("write"); + run(&origin, &["git", "commit", "-qam", "server fix"]); + state.pull_and_build(false, || true, Arc::new(|_: &str, _: String| {})); + while state.status().building { + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + + assert!( + clone.join("server/backend-built").exists(), + "backend moved, so it is the one to rebuild", + ); + assert!( + !clone.join("app/app-built").exists(), + "nothing under app/ changed -- it must be left alone", + ); + } + /// Naming one component builds only that one. /// /// The case a project with more than one component exists to avoid