Pass the pods to update as positional parameters, and end a job whose worker died silently

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,27 @@ 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:?}");
+    // Whole lines: a shell that reports the kill quotes the body's text, words and all.
+    let stdout = String::from_utf8_lossy(&output.stdout);
+    let lines = stdout.lines().collect::<Vec<_>>();
+    assert!(lines.contains(&"about to die"), "{stdout}");
+    assert!(!lines.contains(&"still alive"), "{stdout}");
+    assert!(
+        lines.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");
+}

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

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::<Vec<_>>();
+        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);
     }