Find the SDK without an environment, and say what broke

A service manager hands its process a scrubbed environment -- measured in
the Gentoo guest, an OpenRC user service gets 19 variables and neither
ANDROID_HOME nor ANDROID_NDK_HOME among them -- so the tool lookups have
to stand on their own. Two ways they did not:

  - $ANDROID_HOME was taken as given. This machine exports a system-wide
    /opt/android-sdk whose build-tools/36.0.0 holds nothing but
    package.xml, so the download failed with "failed to spawn
    .../zipalign", naming the tool rather than the root that was wrong.
    Each lookup now takes the first candidate that actually contains what
    it needs, and names every place it looked.

  - The NDK search took the newest child of ~/Android and filtered it
    afterwards, which can only reject that one directory. It worked by
    the luck of `Sdk` sorting before `android-ndk-r27c`; a lowercase
    neighbour would have hidden an NDK that was sitting right there.
    newest_child_where filters before taking the max.

And the failure had nowhere to go: ApiError::Internal answered with an
empty body, so a missing NDK reached the phone as "Server returned HTTP
500" while the message naming the path stayed in a log on a machine the
person holding the phone cannot see. It now answers with the same
{err:#} chain it logs. Every route is behind the bearer token of a
device somebody enrolled themselves, so there is no third party to
withhold it from.

Diagnosis from the tdep-survey session, which measured the OpenRC
environment and the /opt/android-sdk contents.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

server/src/routes.rs

@@ -158,10 +158,23 @@ impl IntoResponse for ApiError {
             Self::BadRequest(_) => StatusCode::BAD_REQUEST,
             Self::RangeNotSatisfiable => StatusCode::RANGE_NOT_SATISFIABLE,
             Self::Internal(err) => {
-                // The only variant whose real cause isn't safe to hand back
-                // verbatim, and the only one worth a log line.
+                // The one variant worth a log line, because it is the one
+                // nobody wrote for a reader. It still answers with its
+                // message: this used to be a bare 500 with an empty body,
+                // on the grounds that an internal cause is not safe to
+                // hand back -- but every route here is behind the bearer
+                // token of a device somebody enrolled themselves, so
+                // there is no third party to be careful of, and what
+                // these actually say is which tool on the build machine
+                // could not be found or would not run. Withholding that
+                // left the phone reporting "HTTP 500" for a missing NDK,
+                // with the only explanation in a log on a machine the
+                // person holding it cannot see.
+                //
+                // The whole `{err:#}` chain rather than the top context,
+                // because the tail is the part that names the path.
                 tracing::error!("{err:#}");
-                return StatusCode::INTERNAL_SERVER_ERROR.into_response();
+                return (StatusCode::INTERNAL_SERVER_ERROR, format!("{err:#}")).into_response();
             }
         };
         (status, self.to_string()).into_response()
@@ -1576,3 +1589,30 @@ async fn serve_apk(
         .body(Body::from_stream(stream))
         .expect("response with a streaming body is valid"))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    /// A tool that could not be found on the build machine is a
+    /// configuration problem there, and the person who can fix it is
+    /// holding the phone rather than reading this server's log. The
+    /// message travelled nowhere for a while -- a bare 500 with an empty
+    /// body -- and the app, which shows a failure's body whenever there
+    /// is one, could only say "Server returned HTTP 500".
+    #[tokio::test]
+    async fn an_internal_failure_still_tells_the_phone_what_broke() {
+        let err = anyhow::anyhow!("llvm-strip not found -- looked under /home/x/Android")
+            .context("stripping app-dioxus");
+        let response = ApiError::Internal(err).into_response();
+
+        assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR);
+        let body = axum::body::to_bytes(response.into_body(), usize::MAX)
+            .await
+            .expect("body");
+        let body = String::from_utf8(body.to_vec()).expect("utf-8");
+        // The whole chain: the tail is the half that names the path.
+        assert!(body.contains("stripping app-dioxus"), "{body}");
+        assert!(body.contains("/home/x/Android"), "{body}");
+    }
+}

server/src/sdk.rs

@@ -23,42 +23,105 @@ fn home_dir() -> PathBuf {
     std::env::home_dir().unwrap_or_default()
 }
 
-/// Honors `$ANDROID_HOME`/`$ANDROID_SDK_ROOT` before falling back to
-/// Android Studio's default location on Linux, so this works on a machine
-/// that keeps its SDK somewhere else without needing a flag.
-fn sdk_root() -> PathBuf {
-    std::env::var_os("ANDROID_HOME")
-        .or_else(|| std::env::var_os("ANDROID_SDK_ROOT"))
+/// Everywhere an SDK might be, best guess first: what the environment
+/// says, then Android Studio's default location on Linux.
+///
+/// Candidates rather than one answer, because **an `$ANDROID_HOME` that
+/// exists is not the same thing as one with the tools in it**, and taking
+/// it as the answer is a failure that names the wrong thing. This machine
+/// exports a system-wide `ANDROID_HOME=/opt/android-sdk` holding a
+/// `build-tools/36.0.0` with no `aapt2`, `zipalign` or `apksigner` under
+/// it at all; a server that inherited that env -- which a service started
+/// by a service manager rather than from a shell is the likely way to --
+/// answered a download with `failed to spawn
+/// /opt/android-sdk/build-tools/36.0.0/zipalign`, blaming the tool for a
+/// root that should never have been chosen. So each lookup below takes
+/// the first candidate that actually holds what it needs, and names every
+/// place it looked when none does.
+///
+/// Listing places nobody named is only safe *because* of that check, and
+/// it is what makes this work from a service: a service manager hands its
+/// process a scrubbed environment -- measured in the Gentoo guest, an
+/// OpenRC user service gets 19 variables and neither `$ANDROID_HOME` nor
+/// `$ANDROID_NDK_HOME` among them, and a systemd user unit inherits an
+/// equally bare one unless somebody has imported theirs. `HOME` does
+/// survive, which is why the home-relative candidates are the ones that
+/// carry this.
+fn sdk_roots() -> Vec<PathBuf> {
+    ["ANDROID_HOME", "ANDROID_SDK_ROOT"]
+        .into_iter()
+        .filter_map(std::env::var_os)
         .map(PathBuf::from)
-        .unwrap_or_else(|| home_dir().join("Android/Sdk"))
+        .chain([
+            home_dir().join("Android/Sdk"),
+            PathBuf::from("/opt/android-sdk"),
+        ])
+        .collect()
 }
 
-/// Highest-versioned entry directly under `dir`, by plain name sort --
-/// good enough for SDK/NDK release directories, which sort correctly as
-/// strings within a single major version scheme.
-fn newest_child(dir: &Path) -> Option<PathBuf> {
+/// Highest-versioned entry directly under `dir` that `keep` accepts, by
+/// plain name sort -- good enough for SDK/NDK release directories, which
+/// sort correctly as strings within a single major version scheme.
+///
+/// The filter is applied *before* the max is taken, which is the whole
+/// reason it is a parameter rather than something the caller does to the
+/// answer: filtering afterwards can only reject the one directory that
+/// happened to sort last, so a single unrelated neighbour hides an
+/// install that is sitting right there. `~/Android` holds `Sdk` beside
+/// `android-ndk-r27c` and worked only by the luck of `S` sorting before
+/// `a`; a lowercase `sdk`, an `avd`, or a second toolchain would have
+/// made the NDK undiscoverable while the error said "install one".
+fn newest_child_where(dir: &Path, keep: impl Fn(&Path) -> bool) -> Option<PathBuf> {
     let mut children: Vec<PathBuf> = std::fs::read_dir(dir)
         .ok()?
         .filter_map(|entry| entry.ok())
         .map(|entry| entry.path())
-        .filter(|path| path.is_dir())
+        .filter(|path| path.is_dir() && keep(path))
         .collect();
     children.sort();
     children.pop()
 }
 
-/// The newest installed `build-tools/<version>/` directory, which is where
-/// `aapt2`, `zipalign` and `apksigner` live.
+/// What this server runs out of a build-tools directory, and therefore
+/// what makes one usable. Named once here rather than at the three call
+/// sites, so a directory cannot be chosen for a tool nobody checked was
+/// in it.
+const BUILD_TOOLS: [&str; 3] = ["aapt2", "zipalign", "apksigner"];
+
+/// The newest `<version>/` directory under any of `bases` that has every
+/// one of [`BUILD_TOOLS`] in it, best base first.
+///
+/// Its own function so the rule can be exercised with directories rather
+/// than through an environment and a process-wide cache.
+fn newest_usable_build_tools(bases: &[PathBuf]) -> Option<PathBuf> {
+    bases.iter().find_map(|base| {
+        newest_child_where(base, |dir| {
+            BUILD_TOOLS.iter().all(|tool| dir.join(tool).is_file())
+        })
+    })
+}
+
+/// The newest installed `build-tools/<version>/` directory that has all of
+/// [`BUILD_TOOLS`] in it, from the first SDK candidate that offers one.
 pub fn build_tools_dir() -> Result<&'static Path> {
     static DIR: OnceLock<Result<PathBuf, String>> = OnceLock::new();
     match DIR.get_or_init(|| {
-        let base = sdk_root().join("build-tools");
-        newest_child(&base).ok_or_else(|| {
+        let bases: Vec<PathBuf> = sdk_roots()
+            .iter()
+            .map(|sdk| sdk.join("build-tools"))
+            .collect();
+        newest_usable_build_tools(&bases).ok_or_else(|| {
             format!(
-                "no Android SDK build-tools found under {} -- install one \
-                 (`android sdk install build-tools/37.0.0`, or source \
-                 ../app/android-env.sh, which does it for you)",
-                base.display(),
+                "no Android SDK build-tools with {} in them found -- looked under {}. \
+                     Install one (`android sdk install build-tools/37.0.0`, or source \
+                     ../app/android-env.sh, which does it for you), or point $ANDROID_HOME \
+                     at an SDK that has one.",
+                BUILD_TOOLS.join(", "),
+                bases
+                    .iter()
+                    .map(|base| base.display().to_string())
+                    .collect::<Vec<_>>()
+                    .join(", "),
             )
         })
     }) {
@@ -67,12 +130,10 @@ pub fn build_tools_dir() -> Result<&'static Path> {
     }
 }
 
+/// No existence check of its own: [`build_tools_dir`] only ever answers
+/// with a directory it has already found every one of [`BUILD_TOOLS`] in.
 pub fn aapt2_path() -> Result<PathBuf> {
-    let path = build_tools_dir()?.join("aapt2");
-    if !path.is_file() {
-        bail!("aapt2 not found at {}", path.display());
-    }
-    Ok(path)
+    Ok(build_tools_dir()?.join("aapt2"))
 }
 
 /// `llvm-strip` ships with the NDK, not the SDK, so this looks in the two
@@ -85,8 +146,13 @@ pub fn llvm_strip_path() -> Result<PathBuf> {
         let roots: Vec<PathBuf> = std::env::var_os("ANDROID_NDK_HOME")
             .map(PathBuf::from)
             .into_iter()
-            .chain(newest_child(&sdk_root().join("ndk")))
-            .chain(newest_child(&home_dir().join("Android")).filter(|path| {
+            .chain(
+                sdk_roots()
+                    .iter()
+                    .filter_map(|sdk| newest_child_where(&sdk.join("ndk"), |_| true))
+                    .collect::<Vec<_>>(),
+            )
+            .chain(newest_child_where(&home_dir().join("Android"), |path| {
                 path.file_name()
                     .is_some_and(|name| name.to_string_lossy().starts_with("android-ndk-"))
             }))
@@ -149,3 +215,78 @@ pub fn run_checked(cmd: &mut std::process::Command) -> Result<()> {
     }
     Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn dir(path: PathBuf) -> PathBuf {
+        std::fs::create_dir_all(&path).unwrap();
+        path
+    }
+
+    fn build_tools(base: &Path, version: &str, tools: &[&str]) {
+        let dir = dir(base.join(version));
+        for tool in tools {
+            std::fs::write(dir.join(tool), "#!/bin/sh\n").unwrap();
+        }
+    }
+
+    /// The `~/Android` case: an SDK sitting beside the NDK is not the NDK,
+    /// and it must not be able to hide it by sorting last. Filtering the
+    /// single newest child instead of the children rejects that one
+    /// directory and answers "nothing here" with the NDK in plain view.
+    #[test]
+    fn an_unrelated_neighbour_does_not_hide_the_newest_match() {
+        let home = tempfile::tempdir().unwrap();
+        dir(home.path().join("android-ndk-r27c"));
+        dir(home.path().join("sdk"));
+        dir(home.path().join("zz-scratch"));
+
+        let found = newest_child_where(home.path(), |path| {
+            path.file_name()
+                .is_some_and(|name| name.to_string_lossy().starts_with("android-ndk-"))
+        });
+        assert_eq!(found, Some(home.path().join("android-ndk-r27c")));
+    }
+
+    /// What `$ANDROID_HOME=/opt/android-sdk` looks like on this machine:
+    /// the directory is there and has a plausibly-named build-tools
+    /// version in it, with none of the tools inside. Taking it produces a
+    /// download that fails with "failed to spawn .../zipalign", naming
+    /// the tool rather than the root that was wrong.
+    #[test]
+    fn a_build_tools_directory_without_the_tools_is_not_used() {
+        let root = tempfile::tempdir().unwrap();
+        let empty = dir(root.path().join("opt/android-sdk/build-tools"));
+        build_tools(&empty, "36.0.0", &["package.xml"]);
+        let real = dir(root.path().join("home/Android/Sdk/build-tools"));
+        build_tools(&real, "37.0.0", &BUILD_TOOLS);
+
+        let bases = vec![empty, real.clone()];
+        assert_eq!(
+            newest_usable_build_tools(&bases),
+            Some(real.join("37.0.0")),
+            "the first candidate that exists is not the first that works"
+        );
+    }
+
+    /// And a half-populated one is no better than an empty one: every
+    /// tool this server runs has to be in the directory it picks, or the
+    /// failure lands on whichever one it happens to spawn first.
+    #[test]
+    fn every_tool_has_to_be_present_not_just_the_first() {
+        let base = tempfile::tempdir().unwrap();
+        build_tools(base.path(), "36.0.0", &["aapt2"]);
+        assert_eq!(
+            newest_usable_build_tools(&[base.path().to_path_buf()]),
+            None
+        );
+
+        build_tools(base.path(), "36.0.0", &BUILD_TOOLS);
+        assert_eq!(
+            newest_usable_build_tools(&[base.path().to_path_buf()]),
+            Some(base.path().join("36.0.0")),
+        );
+    }
+}