From e8bec087bf6ee86e4a4a7bee765748f171c44b81 Mon Sep 17 00:00:00 2001 From: Stackrats Date: Thu, 17 Sep 2026 22:46:32 +0700 Subject: [PATCH 1/2] Pass the pods to update as positional parameters, and end a job whose worker died silently Both faults come from the same fact, which the earlier fixes had wrong: the guest job script is the ssh remote command, so the guest's login shell runs it, and on macOS that is zsh rather than /bin/sh. The CocoaPods retry passed the pod names as an unquoted parameter and relied on sh's word splitting. zsh does not split, so `pod update` got one argument holding the name and a trailing space, and CocoaPods refused it. The names are now built into positional parameters one line at a time and passed as "$@", which every shell treats alike. The recipe's fixture test runs the script under each of /bin/sh, /bin/bash and /bin/zsh the host has. zsh also ends the worker subshell on an assignment or expansion error without running its EXIT trap at all, so no status was written and the wrapper's reader, which waited only for the status file, hung forever. The reader now watches the worker's pid too: once the worker is gone, a status it wrote is already in place, since the trap runs before exit; none means the shell skipped the trap or the worker was killed, and the reader records 1 with a line saying so. A test kills the worker outright, which no shell's trap survives. Both verified as the ssh remote command on the guest. --- .../src/android_job_tests.rs | 31 +++++-- crates/buildbridge-machines/src/device_run.rs | 19 +++- crates/buildbridge-machines/src/recipes.rs | 87 +++++++++++-------- docs/PRODUCT_AND_ARCHITECTURE.md | 4 +- 4 files changed, 95 insertions(+), 46 deletions(-) diff --git a/crates/buildbridge-machines/src/android_job_tests.rs b/crates/buildbridge-machines/src/android_job_tests.rs index 143a25d..f5850be 100644 --- a/crates/buildbridge-machines/src/android_job_tests.rs +++ b/crates/buildbridge-machines/src/android_job_tests.rs @@ -11,14 +11,7 @@ struct JobFixture { impl JobFixture { fn new() -> Self { - let nonce = SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .as_nanos(); - let directory = std::env::temp_dir().join(format!( - "buildbridge-android-job-{}-{nonce}", - std::process::id() - )); + let directory = crate::test_scripts::fixture_dir("android-job"); let tools = directory.join("tools"); let jobs = tools.join("jobs"); let workspace = directory.join("workspace"); @@ -227,3 +220,25 @@ fn completed_jobs_are_removed_for_sync_and_build_without_touching_other_state() } } } + +/// A worker whose shell ends without running its trap: killed outright here, which no shell's +/// trap survives, and what zsh does by itself on an assignment or expansion error. The +/// wrapper must notice the dead worker and record a failure rather than wait for a status +/// that is never coming. +#[test] +fn a_worker_that_dies_without_its_trap_is_recorded_as_failed_not_waited_for() { + let fixture = JobFixture::new(); + let output = fixture.run_debug_job( + "/usr/bin/printf '%s\\n' 'about to die'\n/bin/sh -c '/bin/kill -9 $PPID'\n/usr/bin/printf '%s\\n' 'still alive'", + ); + assert_eq!(output.status.code(), Some(1), "{output:?}"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!(stdout.contains("about to die"), "{stdout}"); + assert!(!stdout.contains("still alive"), "{stdout}"); + assert!( + stdout.contains("The job ended without reporting a status."), + "{stdout}" + ); + let job = fixture.jobs.join("android-debug-build"); + assert_eq!(fs::read_to_string(job.join("status")).unwrap().trim(), "1"); +} diff --git a/crates/buildbridge-machines/src/device_run.rs b/crates/buildbridge-machines/src/device_run.rs index 6e4fe53..e3e1b20 100644 --- a/crates/buildbridge-machines/src/device_run.rs +++ b/crates/buildbridge-machines/src/device_run.rs @@ -1233,9 +1233,10 @@ if /bin/test "$job_owner" -eq 1; then job_done=0 finish_job() {{ worker_status=$? - # bash 3.2, the /bin/sh of a macOS guest, runs this trap with a status of 0 - # after an assignment or expansion error has ended the shell, so a 0 is only - # believed once the body ran to its end. + # bash 3.2 runs this trap with a status of 0 after an assignment or expansion + # error has ended the shell, so a 0 is only believed once the body ran to its + # end. zsh, the login shell that runs this script on a macOS guest, skips the + # trap on those errors altogether; the reader below covers that. if /bin/test "$worker_status" -eq 0 && /bin/test "$job_done" -ne 1; then worker_status=1 fi @@ -1264,6 +1265,18 @@ while ! /bin/test -f "$job_status"; do next_line=$((line_count + 1)) fi fi + if /bin/test -n "${{job_pid-}}" && ! /bin/kill -0 "$job_pid" 2>/dev/null; then + # The worker is gone. Its trap runs before it exits, so a status it wrote is already + # in place; none means its shell ended without running the trap at all, as zsh does + # on an assignment or expansion error, or it was killed. Either is a failure, and + # without this the wait would never end. + if ! /bin/test -f "$job_status"; then + /usr/bin/printf '%s\n' 'The job ended without reporting a status.' >> "$job_log" + /usr/bin/printf '1\n' > "$job_status.incoming" + /bin/mv "$job_status.incoming" "$job_status" + fi + break + fi /bin/sleep 1 done if /bin/test -f "$job_log"; then diff --git a/crates/buildbridge-machines/src/recipes.rs b/crates/buildbridge-machines/src/recipes.rs index 52433a8..3142f96 100644 --- a/crates/buildbridge-machines/src/recipes.rs +++ b/crates/buildbridge-machines/src/recipes.rs @@ -362,7 +362,10 @@ cd {root} /// update` for exactly the pods named. The lock then differs from the committed one, the /// marker below reports it, and the test build step offers the refreshed lock for adoption. /// Names are taken from CocoaPods' own message and limited to the characters a pod name can -/// hold, so the retry receives words and never anything a shell would interpret. +/// hold, so the retry receives words and never anything a shell would interpret. They are +/// passed as positional parameters built one line at a time: the script is the ssh remote +/// command, so the guest's login shell runs it, and zsh does not split an unquoted +/// parameter into words the way sh does. fn pods_script(layout: &ProjectLayout, workspace: &str, tools: &RecipeTools) -> String { let (Some(ios), Some(pod)) = (&layout.ios, &tools.pod) else { return String::new(); @@ -391,13 +394,19 @@ if "{pod}" install --no-ansi > "$pods_log" 2>&1; then else pods_status=$? /bin/cat "$pods_log" - conflicting=$(/usr/bin/sed -n 's/.*could not find compatible versions for pod "\([A-Za-z0-9_.+\/-]*\)".*/\1/p' "$pods_log" | /usr/bin/sort -u | /usr/bin/tr '\n' ' ') + conflicting=$(/usr/bin/sed -n 's/.*could not find compatible versions for pod "\([A-Za-z0-9_.+\/-]*\)".*/\1/p' "$pods_log" | /usr/bin/sort -u) /bin/rm -f "$pods_log" if /bin/test -z "$conflicting"; then exit "$pods_status" fi - /usr/bin/printf 'The committed Podfile.lock pins %sat a version the project no longer accepts. Updating those pods in the guest only, as CocoaPods advises; the refreshed lock can be adopted into the project from the test build step.\n' "$conflicting" - "{pod}" update $conflicting --no-ansi + set -- + while read -r pod_name; do + set -- "$@" "$pod_name" + done <<__BUILDBRIDGE_PODS__ +$conflicting +__BUILDBRIDGE_PODS__ + /usr/bin/printf 'The committed Podfile.lock pins %s at a version the project no longer accepts. Updating those pods in the guest only, as CocoaPods advises; the refreshed lock can be adopted into the project from the test build step.\n' "$*" + "{pod}" update "$@" --no-ansi fi lock_after=$({lock_after}) if /bin/test "$lock_before" != "$lock_after"; then @@ -962,7 +971,7 @@ mod tests { script.contains("install --no-ansi > \"$pods_log\""), "{script}" ); - assert!(script.contains("update $conflicting --no-ansi"), "{script}"); + assert!(script.contains("update \"$@\" --no-ansi"), "{script}"); assert!(script.contains("exit \"$pods_status\""), "{script}"); let mut open = ProjectLayout::capacitor_default(); @@ -1016,10 +1025,18 @@ esac lock_before_script(&layout, workspace, &tools), pods_script(&layout, workspace, &tools) ); - let run = |mode: &str| { + // The script is the ssh remote command, so the guest's login shell runs it: zsh on + // any macOS since Catalina, which does not split an unquoted parameter into words. + // Every shell this host has stands in for it. + let shells = ["/bin/sh", "/bin/bash", "/bin/zsh"] + .into_iter() + .filter(|shell| Path::new(shell).is_file()) + .collect::>(); + let run = |shell: &str, mode: &str| { std::fs::write(podfile_dir.join("Podfile.lock"), "pinned\n").unwrap(); let calls = root.join(format!("calls-{mode}")); - let output = std::process::Command::new("/bin/sh") + let _ = std::fs::remove_file(&calls); + let output = std::process::Command::new(shell) .args(["-c", &script]) .env("FIXTURE_MODE", mode) .env("FIXTURE_CALLS", &calls) @@ -1030,35 +1047,37 @@ esac (output.status.code(), stdout, calls) }; - let (status, stdout, calls) = run("conflict"); - assert_eq!(status, Some(0), "{stdout}"); - assert_eq!( - calls, - "install --no-ansi\nupdate Firebase/Core IONCameraLib --no-ansi\n" - ); - assert!( - stdout.contains("could not find compatible versions"), - "{stdout}" - ); - assert!( - stdout.contains("pins Firebase/Core IONCameraLib at a version"), - "{stdout}" - ); - assert!( - stdout.contains("__BUILDBRIDGE_NATIVE_LOCK_UPDATED__:yes"), - "{stdout}" - ); + for shell in shells { + let (status, stdout, calls) = run(shell, "conflict"); + assert_eq!(status, Some(0), "{shell}: {stdout}"); + assert_eq!( + calls, "install --no-ansi\nupdate Firebase/Core IONCameraLib --no-ansi\n", + "{shell}" + ); + assert!( + stdout.contains("could not find compatible versions"), + "{stdout}" + ); + assert!( + stdout.contains("pins Firebase/Core IONCameraLib at a version"), + "{shell}: {stdout}" + ); + assert!( + stdout.contains("__BUILDBRIDGE_NATIVE_LOCK_UPDATED__:yes"), + "{shell}: {stdout}" + ); - let (status, stdout, calls) = run("broken"); - assert_eq!(status, Some(7), "{stdout}"); - assert_eq!(calls, "install --no-ansi\n"); - assert!(stdout.contains("The Podfile is malformed"), "{stdout}"); - assert!(!stdout.contains("NATIVE_LOCK_UPDATED"), "{stdout}"); + let (status, stdout, calls) = run(shell, "broken"); + assert_eq!(status, Some(7), "{shell}: {stdout}"); + assert_eq!(calls, "install --no-ansi\n", "{shell}"); + assert!(stdout.contains("The Podfile is malformed"), "{stdout}"); + assert!(!stdout.contains("NATIVE_LOCK_UPDATED"), "{stdout}"); - let (status, stdout, calls) = run("clean"); - assert_eq!(status, Some(0), "{stdout}"); - assert_eq!(calls, "install --no-ansi\n"); - assert!(!stdout.contains("NATIVE_LOCK_UPDATED"), "{stdout}"); + let (status, stdout, calls) = run(shell, "clean"); + assert_eq!(status, Some(0), "{shell}: {stdout}"); + assert_eq!(calls, "install --no-ansi\n", "{shell}"); + assert!(!stdout.contains("NATIVE_LOCK_UPDATED"), "{stdout}"); + } let _ = std::fs::remove_dir_all(&root); } diff --git a/docs/PRODUCT_AND_ARCHITECTURE.md b/docs/PRODUCT_AND_ARCHITECTURE.md index a7fe991..08a613d 100644 --- a/docs/PRODUCT_AND_ARCHITECTURE.md +++ b/docs/PRODUCT_AND_ARCHITECTURE.md @@ -793,12 +793,14 @@ Accepted live through `crates/buildbridge-engine/tests/android_release.rs` again | Ticking a checkbox low on a scrolled machine page shifted the whole window up and left a blank band beneath the app | The hidden real input behind the checkbox is absolutely positioned with no positioned ancestor, so it stayed at the pane's unscrolled position below the window; focusing it scrolled the document, which had spare height from hidden tooltip text placed the same way | The checkbox label and the tooltip anchor are positioned so hidden helpers stay beside their controls, and every scroll container is positioned so nothing inside it can stretch the window | Resolved 2026-09-07, reproduced and verified in WebKitGTK | | The machine page's tab went back to its default on every sidebar click, so a tab chosen a moment earlier was gone after a visit to another machine or page | Opening a machine without a named step cleared its remembered tab together with its step, treating both as "open fresh" | Obsoleted the same evening: the tabs went with the one-page redesign, and what a person opens or folds on a machine page — the step, the timeline's phases, the sections after it — is what is now kept per machine for the session | Obsolete; superseded by decisions 59 and 61 | | Choosing **Saved snapshot** on a build step did nothing: the picker fell straight back to Latest local source | The per-machine build draft was created with `drafts[id] ??= {…}`, and that operator evaluates to the raw object it assigned rather than the reactive one the store keeps. A step that captured the draft once in its setup therefore wrote its choices into an object nothing was watching | The store reads the draft back out of its record after creating it, so every caller shares the tracked object. A unit test captures a draft, writes to it, and expects a computed over it to change | Resolved 2026-09-07 and unit tested | -| The live reload PR's macOS CI job failed one `buildbridge-machines` test: the debug job that must refuse an environment file overwriting `BUILDBRIDGE_LIVE_RELOAD_URL` reported success on a Mac and failure on Linux | The guest job wrapper runs its body in a subshell under `set -eu` with an `EXIT` trap that writes `$?` to the job's status file. bash 3.2, which is `/bin/sh` on every Mac and so on every macOS guest, ends the shell on an assignment to a readonly variable or an unbound-variable expansion as POSIX asks, but runs the trap with `$?` of 0, so the job recorded 0 and the wrapper exited 0. Reproduced in the `bash:3.2` image; bash 5 and dash hand the trap the real status. **A product fault beyond the test**: a smoke build or iPhone run on the guest that died this way would have been reported as a success with its work undone, since only ordinary command failures kept their status | The wrapper trusts a 0 only once the body ran to its end: the subshell sets a `job_done` flag as its last statement and the trap turns a 0 without that flag into a 1. Every deliberate early exit in a job body is already nonzero, so nothing that succeeds is misreported. Verified under bash 3.2 for a readonly assignment, an unbound variable, a failing command, an explicit `exit 3` and a clean body | Fixed 2026-09-17 | +| The live reload PR's macOS CI job failed one `buildbridge-machines` test: the debug job that must refuse an environment file overwriting `BUILDBRIDGE_LIVE_RELOAD_URL` reported success on a Mac and failure on Linux | The guest job wrapper runs its body in a subshell under `set -eu` with an `EXIT` trap that writes `$?` to the job's status file. bash 3.2, `/bin/sh` on every Mac, ends the shell on an assignment to a readonly variable or an unbound-variable expansion as POSIX asks, but runs the trap with `$?` of 0, so the job recorded 0 and the wrapper exited 0. Reproduced in the `bash:3.2` image; bash 5 and dash hand the trap the real status. Corrected the same day: the script is the ssh remote command, so the guest's *login shell* runs it, zsh on any macOS since Catalina, and zsh skips the trap on those errors altogether (see the row below); the bash 3.2 guard stays for a guest whose login shell is bash. **A product fault beyond the test**: a smoke build or iPhone run on the guest that died this way would have been reported as a success with its work undone, since only ordinary command failures kept their status | The wrapper trusts a 0 only once the body ran to its end: the subshell sets a `job_done` flag as its last statement and the trap turns a 0 without that flag into a 1. Every deliberate early exit in a job body is already nonzero, so nothing that succeeds is misreported. Verified under bash 3.2 for a readonly assignment, an unbound variable, a failing command, an explicit `exit 3` and a clean body | Fixed 2026-09-17 | | On a host that had run an earlier release, the iPhone step warned that the udev rule "differs from the expected content", all six checks showed as failed, and the **Reinstall the host USB rule** button was disabled | Two faults. The rule check compared the whole file byte for byte, and the lowercase-brand commit had changed the comment header from "Written by BuildBridge" to "Written by buildbridge"; the rule line itself was identical, so passthrough worked while the step said it did not. And the step disabled its primary action whenever the step was pending, whichever rung it stood on, so a host waiting on its test build could not reinstall a udev rule that no build depends on | The check compares the lines udev acts on and ignores comments and blank lines, so a header change never asks a working host to reinstall, while a changed or added rule line still does. The pending gate now withholds only the signing and run rungs, which the engine refuses without the test build anyway, and the withheld button names that reason; every host and phone rung acts at any time | Fixed 2026-09-17 | | The unsigned test build failed in the guest at Resolving CocoaPods: "could not find compatible versions for pod IONCameraLib", with the lock pinning 1.0.4 and the project's `@capacitor/camera` 8.2.4 wanting 2.x | The host's `@capacitor/camera` had been upgraded, and its pod now depends on a newer IONCameraLib than the committed `ios/App/Podfile.lock` pins. `pod install` keeps every pinned version, so it cannot satisfy both and stops. The lock-drift flow the design describes, a refreshed guest lock that the step offers for adoption, was never reached, because it only fires when `pod install` itself rewrites the lock; a conflicting lock makes it fail instead. A Linux host cannot run CocoaPods, so `cap sync ios` there copies assets and updates the plugin list and can do nothing about the lock | The guest recipe runs `pod install`, and on failure reads the pods CocoaPods names in its own "could not find compatible versions" lines, limited to the characters a pod name can hold, and retries with `pod update` for exactly those, as CocoaPods advises. The lock then differs from the committed one, the existing marker reports it, and the test build step offers the refreshed lock for adoption. Any other failure still fails the build as before, and a project with no committed lock is left to plain `pod install`. Verified with a fixture `pod` on Linux and under bash 3.2. The native Mac path keeps its `--deployment` install: that host has CocoaPods, and the path refuses by design to alter the host project | Fixed 2026-09-17 | | The macOS half of CI failed one `buildbridge-machines` test with `File exists` from the Android HTTP fixture's `create_dir`, on a change that had not touched it | Every test fixture named its temp directory by the process id and a nanosecond timestamp. A macOS clock ticks in microseconds, and a test binary creates fixtures on many threads at once, so two fixtures born in the same instant took the same name and the second died. Linux clocks are fine-grained enough that it had never shown there | `test_scripts::fixture_dir` names a directory by the process id and a process-wide counter and creates it; every test fixture in the crate uses it, and none names a directory by a clock any more. Production temp names in `archive`, `android_signing` and `android_device` still use a timestamp, but each is created once per operation rather than in parallel by a test runner | Fixed 2026-09-17 | | With the CocoaPods retry shipped and installed, the same IONCameraLib failure came back unchanged | The retry never ran: the failure was earlier. Capacitor's `cap sync ios` rewrites the Podfile's plugin block and then runs `pod install` itself, in the syncing phase before buildbridge's Resolving CocoaPods phase, and it has no switch to leave that out, only `CAPACITOR_COCOAPODS_PATH` to choose which `pod` it runs. Found by reproducing in the guest over buildbridge's own SSH identity: `pod update IONCameraLib` there resolved 2.0.0 at once, so the retry was sound and simply unreached | The sync runs with `CAPACITOR_COCOAPODS_PATH=/usr/bin/true`, so Capacitor rewrites the Podfile and cleans the project but leaves CocoaPods to the next phase, where the retry lives. Verified on the guest against the project with its stale lock restored: sync passes, install refuses the pod, the update resolves it, and the lock-changed marker fires | Fixed 2026-09-17 | | The macOS half of CI failed `android_device`'s install-and-launch test with no console lines streamed, on a change to the iOS recipe | The test's stand-in for a Stop from the desktop fired as soon as the fake `adb` had been called with `logcat`, not once the run had read the two lines the fake prints, so on a slow runner the run ended before they arrived. The 2026-09-08 fix to this test had moved the Stop from a fixed delay to that call, which was later but still not late enough | The Stop waits until the run's progress has carried both lines, with the same 30-second ceiling, so it lands where a person's would: after the console is visibly streaming | Fixed 2026-09-17 | +| With Capacitor's sync kept out of CocoaPods, the retry ran and failed: "The `IONCameraLib ` Pod is not installed and cannot be updated", with a space inside the name | The guest job script is passed as the ssh remote command, so the guest's login shell runs it, and on macOS that is zsh. The retry passed the pod names as an unquoted parameter and relied on sh's word splitting; zsh does not split, so `pod update` got one argument holding the name and a trailing space. Every earlier verification had run the rendered script under `/bin/sh` on purpose, which hid it | The names are built into positional parameters one line at a time and passed as `"$@"`, which every shell treats alike. The recipe's fixture test now runs the script under each of `/bin/sh`, `/bin/bash` and `/bin/zsh` the host has, so the macOS CI runner covers zsh. Verified as the ssh remote command on the guest: install refuses the pod, the update resolves 2.0.0, the marker fires | Fixed 2026-09-17 | +| Running the job wrapper under the guest's zsh for a body with a readonly assignment hung forever: no status file, worker gone | **A product fault, and the worst of today's set.** zsh ends the worker subshell on an assignment or expansion error without running its EXIT trap at all, so no status is ever written, and the wrapper's reader waited only for the status file. A smoke build, device run or Android job that died this way held its step until someone stopped it. bash 3.2 ran the trap with a wrong status; zsh does not run it | The reader also watches the worker's pid. Once the worker is gone, a status it wrote is already in place, since the trap runs before exit; none means the shell skipped the trap or the worker was killed, and the reader records 1 with a line saying the job ended without reporting. A test kills the worker outright, which no shell's trap survives. Verified as the ssh remote command on the guest for a readonly assignment, an unbound variable, a killed worker, a failing command, an explicit exit and a clean body | Fixed 2026-09-17 | ### Automation and UI boundary From b637806880a29e2b3475e52b1636a9320a342f7b Mon Sep 17 00:00:00 2001 From: Stackrats Date: Thu, 17 Sep 2026 22:51:48 +0700 Subject: [PATCH 2/2] Judge the killed worker's output by whole lines; macOS bash quotes the body when it reports the kill --- crates/buildbridge-machines/src/android_job_tests.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/buildbridge-machines/src/android_job_tests.rs b/crates/buildbridge-machines/src/android_job_tests.rs index f5850be..e0d0131 100644 --- a/crates/buildbridge-machines/src/android_job_tests.rs +++ b/crates/buildbridge-machines/src/android_job_tests.rs @@ -232,11 +232,13 @@ fn a_worker_that_dies_without_its_trap_is_recorded_as_failed_not_waited_for() { "/usr/bin/printf '%s\\n' 'about to die'\n/bin/sh -c '/bin/kill -9 $PPID'\n/usr/bin/printf '%s\\n' 'still alive'", ); assert_eq!(output.status.code(), Some(1), "{output:?}"); + // Whole lines: a shell that reports the kill quotes the body's text, words and all. let stdout = String::from_utf8_lossy(&output.stdout); - assert!(stdout.contains("about to die"), "{stdout}"); - assert!(!stdout.contains("still alive"), "{stdout}"); + let lines = stdout.lines().collect::>(); + assert!(lines.contains(&"about to die"), "{stdout}"); + assert!(!lines.contains(&"still alive"), "{stdout}"); assert!( - stdout.contains("The job ended without reporting a status."), + lines.contains(&"The job ended without reporting a status."), "{stdout}" ); let job = fixture.jobs.join("android-debug-build");