Don't treat non-empty stderr as mergebase failure

Summary:
**What:**

- Drop the `|| !output.stderr().is_empty()` clause in `get_mergebase_details_impl` (`fbcode/eden/fs/cli_rs/sapling-client/src/mergebase.rs:110`).
- Keep stderr (and now the exit code) in the genuine-failure error string so callers still get diagnostic context.

**Why:**

- A successful `hg log` can legitimately write to stderr — any caller that sets `SL_LOG` triggers it (sapling emits `clienttelemetry: client_entry_point="sapling"` on stderr even at quiet levels).
- Exit status is the source of truth. Meerkat's own `vcs/hg.rs:run_hg_command` correctly checks only `status.success()` — that is the right contract for a Rust subprocess wrapper.

**How it surfaced:**

- D106483065 set `SL_LOG=info` on the inner atlasenv container; meerkat called `get_mergebase`.
- The spurious failure cascaded: meerkat exit 1 → `atlas-preparer` setup exit 3 (`HookResult::FAILURE`) → `cogwheel_atlas_www` retry loop until the harness gave up.
- D106494042 landed as a temporary safety net (commented out SL_LOG) and can be backed out once this fix is in the dev-docker image.

**Related:**

- Buck2's copy at `fbcode/buck2/app/buck2_file_watcher/src/edenfs/sapling.rs:78` has the same shape; `T219988735` already tracks consolidating it into this crate.

Differential Revision: D106493970

fbshipit-source-id: 9f4b4ed1db494d9b02d2b2615137ca4d0cfa88b0

Author: Liubov Dmitrieva

eden/fs/cli_rs/sapling-client/src/mergebase.rs

@@ -107,9 +107,12 @@ where
             ]);
         let output = spawner.output(&mut command).await?;
 
-        if !output.status().success() || !output.stderr().is_empty() {
+        // Only treat a non-zero exit status as failure. `hg log` may write to stderr on
+        // success (e.g. when `SL_LOG` is set in the caller's environment).
+        if !output.status().success() {
             Err(SaplingError::Other(format!(
-                "Failed to obtain mergebase:\n{}",
+                "Failed to obtain mergebase (exit status {:?}):\n{}",
+                output.status().code(),
                 String::from_utf8(output.stderr().to_vec())
                     .unwrap_or("Failed to parse stderr".to_string())
             )))
@@ -154,6 +157,7 @@ fn parse_mergebase_details(mergebase_details: Vec<u8>) -> Result<Option<Mergebas
 mod tests {
     use crate::mergebase::*;
     use crate::utils::tests::get_mock_spawner;
+    use crate::utils::tests::get_mock_spawner_with_stderr;
 
     // the format is {node}\n{date}\n{global_rev}
     const DETAILS: &str = "0000111122223333444455556666777788889999\n1234567890.012345\n9876543210";
@@ -192,6 +196,58 @@ mod tests {
         Ok(())
     }
 
+    #[tokio::test]
+    async fn test_get_mergebase_details_succeeds_with_non_empty_stderr() -> Result<()> {
+        // A successful `hg log` can still write to stderr — for example when the caller's
+        // environment sets `SL_LOG=info`, or when sapling emits routine WARN lines like
+        // the `cats` preminted-key fallback. Exit status is the source of truth; non-empty
+        // stderr alone must NOT be treated as failure.
+        let stdout = DETAILS.to_owned();
+        let stderr = "2026-05-27T10:19:36Z  INFO clienttelemetry: client_entry_point=\"sapling\"\n\
+                      2026-05-27T10:19:36Z  WARN run: cats: wanted-key not found, falling back\n";
+        let spawner = get_mock_spawner_with_stderr(
+            get_sapling_executable_path(),
+            Some((
+                0,
+                Some(stdout.as_bytes().to_vec()),
+                Some(stderr.as_bytes().to_vec()),
+            )),
+        );
+
+        let details = get_mergebase_details_impl(&spawner, ".", COMMIT_ID, MERGEBASE_WITH)
+            .await?
+            .expect("mergebase should parse successfully despite stderr output");
+        assert_eq!(details.mergebase, COMMIT_ID);
+        Ok(())
+    }
+
+    #[tokio::test]
+    async fn test_get_mergebase_details_errors_on_non_zero_exit() -> Result<()> {
+        // The mirror case: real sapling failures (non-zero exit) must still surface as
+        // an error, with the exit code and stderr included in the message so the caller
+        // can diagnose. Pick a different commit/mergebase pair than the success test to
+        // bypass the module-level LRU cache.
+        let stderr = "abort: unknown revision\n";
+        let spawner = get_mock_spawner_with_stderr(
+            get_sapling_executable_path(),
+            Some((255, None, Some(stderr.as_bytes().to_vec()))),
+        );
+
+        let err = get_mergebase_details_impl(&spawner, ".", "bogus-commit", "bogus-mergebase")
+            .await
+            .expect_err("non-zero exit must propagate as an error");
+        let msg = err.to_string();
+        assert!(
+            msg.contains("Failed to obtain mergebase"),
+            "missing context in error: {msg}"
+        );
+        assert!(
+            msg.contains("abort: unknown revision"),
+            "stderr missing from error: {msg}"
+        );
+        Ok(())
+    }
+
     #[test]
     fn test_parse_mergebase_details() -> Result<()> {
         let details = parse_mergebase_details(DETAILS.as_bytes().to_vec())?.unwrap();

eden/fs/cli_rs/sapling-client/src/utils.rs

@@ -219,7 +219,7 @@ pub(crate) mod tests {
         MockCommandSpawner::with_callback(move |cmd| match cmd.program.to_str() {
             Some(cmd_program) if cmd_program == program && output.is_some() => {
                 let (exit_code, stdout_lines) = output.clone().unwrap();
-                Ok(mock_child(exit_code, stdout_lines))
+                Ok(mock_child(exit_code, stdout_lines, None))
             }
             program => Err(Error::other(anyhow::anyhow!(
                 "Not expected program: {:?}",
@@ -228,14 +228,44 @@ pub(crate) mod tests {
         })
     }
 
-    fn mock_child(exit_code: i32, stdout_lines: Option<Vec<u8>>) -> MockChild {
+    pub(crate) fn get_mock_spawner_with_stderr(
+        program: String,
+        output: Option<(i32, Option<Vec<u8>>, Option<Vec<u8>>)>,
+    ) -> MockCommandSpawner {
+        MockCommandSpawner::with_callback(move |cmd| match cmd.program.to_str() {
+            Some(cmd_program) if cmd_program == program && output.is_some() => {
+                let (exit_code, stdout_lines, stderr_lines) = output.clone().unwrap();
+                Ok(mock_child(exit_code, stdout_lines, stderr_lines))
+            }
+            program => Err(Error::other(anyhow::anyhow!(
+                "Not expected program: {:?}",
+                program
+            ))),
+        })
+    }
+
+    fn mock_child(
+        exit_code: i32,
+        stdout_lines: Option<Vec<u8>>,
+        stderr_lines: Option<Vec<u8>>,
+    ) -> MockChild {
         let handle = MockChildHandle::new();
         handle.set_status(Ok(Some(MockExitStatus::new(Some(exit_code)))));
-        if let Some(stdout_lines) = stdout_lines {
-            let stdout = MockIoBuilder::new().read(&stdout_lines).build();
-            MockChild::with_stdio(handle, Some(io::sink()), Some(stdout), Some(io::empty()))
-        } else {
-            MockChild::new(handle)
+        match (stdout_lines, stderr_lines) {
+            (None, None) => MockChild::new(handle),
+            (Some(stdout_lines), None) => {
+                let stdout = MockIoBuilder::new().read(&stdout_lines).build();
+                MockChild::with_stdio(handle, Some(io::sink()), Some(stdout), Some(io::empty()))
+            }
+            (None, Some(stderr_lines)) => {
+                let stderr = MockIoBuilder::new().read(&stderr_lines).build();
+                MockChild::with_stdio(handle, Some(io::sink()), Some(io::empty()), Some(stderr))
+            }
+            (Some(stdout_lines), Some(stderr_lines)) => {
+                let stdout = MockIoBuilder::new().read(&stdout_lines).build();
+                let stderr = MockIoBuilder::new().read(&stderr_lines).build();
+                MockChild::with_stdio(handle, Some(io::sink()), Some(stdout), Some(stderr))
+            }
         }
     }
 }