Retry the packager status probe, and always invoke its callback (#57852)

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/PackagerStatusCheck.kt

@@ -30,56 +30,94 @@ internal class PackagerStatusCheck(private val client: OkHttpClient) {
     val statusURL = createPackagerStatusURL(host)
     val request = Request.Builder().url(statusURL).build()
 
-    client
-        .newCall(request)
-        .enqueue(
-            object : Callback {
-              override fun onFailure(call: Call, e: IOException) {
-                FLog.w(
-                    ReactConstants.TAG,
-                    "The packager does not seem to be running as we got an IOException requesting its status: ${e.message}",
-                )
-                callback.onPackagerStatusFetched(false)
-              }
+    // A metro server reached over a slow link can exceed the client's connect timeout, or answer
+    // 5xx while it is still starting up. A single-shot probe reports those as "no dev server",
+    // which silently boots the bundle packaged in the app instead. Retry transport-level failures a
+    // bounded number of times before giving up. Retries are re-enqueued rather than delayed,
+    // because this client's dispatcher is shared with the bundle download and the packager
+    // websockets and must not be blocked.
+    fun attempt(attemptsLeft: Int) {
+      // Only for failures that a later attempt could plausibly resolve.
+      fun retryOrFail() {
+        if (attemptsLeft > 1) {
+          attempt(attemptsLeft - 1)
+        } else {
+          callback.onPackagerStatusFetched(false)
+        }
+      }
 
-              override fun onResponse(call: Call, response: Response) {
-                if (!response.isSuccessful) {
-                  FLog.e(
+      client
+          .newCall(request)
+          .enqueue(
+              object : Callback {
+                override fun onFailure(call: Call, e: IOException) {
+                  FLog.w(
                       ReactConstants.TAG,
-                      "Got non-success http code from packager when requesting status: ${response.code()}",
+                      "The packager does not seem to be running as we got an IOException requesting its status from $statusURL: ${e.message}",
                   )
-                  callback.onPackagerStatusFetched(false)
-                  return
+                  retryOrFail()
                 }
-                val body = response.body()
-                if (body == null) {
-                  FLog.e(
-                      ReactConstants.TAG,
-                      "Got null body response from packager when requesting status",
-                  )
-                  callback.onPackagerStatusFetched(false)
-                  return
-                }
-                val bodyString =
-                    body.string() // cannot call body.string() twice, stored it into variable.
-                // https://github.com/square/okhttp/issues/1240#issuecomment-68142603
-                if (PACKAGER_OK_STATUS != bodyString) {
-                  FLog.e(
-                      ReactConstants.TAG,
-                      "Got unexpected response from packager when requesting status: $bodyString",
-                  )
-                  callback.onPackagerStatusFetched(false)
-                  return
+
+                override fun onResponse(call: Call, response: Response) {
+                  if (!response.isSuccessful) {
+                    FLog.e(
+                        ReactConstants.TAG,
+                        "Got non-success http code from packager at $statusURL when requesting status: ${response.code()}",
+                    )
+                    // 5xx can mean the dev server is still coming up; a 4xx will not change.
+                    if (response.code() >= 500) {
+                      retryOrFail()
+                    } else {
+                      callback.onPackagerStatusFetched(false)
+                    }
+                    return
+                  }
+                  val body = response.body()
+                  if (body == null) {
+                    FLog.e(
+                        ReactConstants.TAG,
+                        "Got null body response from packager at $statusURL when requesting status",
+                    )
+                    callback.onPackagerStatusFetched(false)
+                    return
+                  }
+                  // Reading the body can still fail mid-stream (e.g. the connection drops after
+                  // the headers). OkHttp only logs a throw from onResponse, so without catching it
+                  // here the callback would never fire and bundle loading would wait forever.
+                  val bodyString =
+                      try {
+                        body.string() // cannot call body.string() twice, stored it into variable.
+                        // https://github.com/square/okhttp/issues/1240#issuecomment-68142603
+                      } catch (e: IOException) {
+                        FLog.w(
+                            ReactConstants.TAG,
+                            "Failed to read the packager status response from $statusURL: ${e.message}",
+                        )
+                        retryOrFail()
+                        return
+                      }
+                  if (PACKAGER_OK_STATUS != bodyString) {
+                    // Something other than Metro answered; retrying will not change the answer.
+                    FLog.e(
+                        ReactConstants.TAG,
+                        "Got unexpected response from packager at $statusURL when requesting status: $bodyString",
+                    )
+                    callback.onPackagerStatusFetched(false)
+                    return
+                  }
+                  callback.onPackagerStatusFetched(true)
                 }
-                callback.onPackagerStatusFetched(true)
-              }
-            },
-        )
+              },
+          )
+    }
+
+    attempt(MAX_STATUS_ATTEMPTS)
   }
 
   private companion object {
     private const val PACKAGER_OK_STATUS = "packager-status:running"
     private const val PACKAGER_STATUS_URL_TEMPLATE = "%s://%s/status"
+    private const val MAX_STATUS_ATTEMPTS = 3
 
     private fun createPackagerStatusURL(host: String): String =
         String.format(

packages/react-native/ReactAndroid/src/test/java/com/facebook/react/devsupport/PackagerStatusCheckTest.kt

@@ -0,0 +1,169 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+@file:Suppress("DEPRECATION_ERROR") // Conflicting okhttp versions
+
+package com.facebook.react.devsupport
+
+import java.io.IOException
+import java.util.concurrent.CountDownLatch
+import java.util.concurrent.TimeUnit
+import java.util.concurrent.atomic.AtomicBoolean
+import java.util.concurrent.atomic.AtomicInteger
+import okhttp3.Interceptor
+import okhttp3.MediaType
+import okhttp3.OkHttpClient
+import okhttp3.Protocol
+import okhttp3.Response
+import okhttp3.ResponseBody
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+
+// Robolectric provides a real android.util.Log; without it FLog throws "Stub!" on the OkHttp
+// dispatcher thread and the status callback never runs.
+@RunWith(RobolectricTestRunner::class)
+class PackagerStatusCheckTest {
+
+  /** One scripted answer to a single probe attempt. */
+  private sealed interface Step {
+    /** Fail at the transport layer, as a down tunnel or refused connection would. */
+    object TransportFailure : Step
+
+    /** Answer with the given HTTP status code and (optional) body. */
+    data class HttpResponse(val code: Int, val body: String = "") : Step
+  }
+
+  /**
+   * Answers every call from [steps] in order, entirely in-process. CI runs these tests in a
+   * network-restricted sandbox where a real loopback server (MockWebServer) is unreachable, so
+   * responses are synthesized by an OkHttp application interceptor that never touches a socket.
+   * [requestCount] records how many probe attempts PackagerStatusCheck issued.
+   */
+  private class ScriptedPackager(private val steps: List<Step>) : Interceptor {
+    val requestCount = AtomicInteger(0)
+
+    override fun intercept(chain: Interceptor.Chain): Response {
+      val step = steps[requestCount.getAndIncrement()]
+      return when (step) {
+        is Step.TransportFailure -> throw IOException("simulated connection failure")
+        is Step.HttpResponse ->
+            Response.Builder()
+                .request(chain.request())
+                .protocol(Protocol.HTTP_1_1)
+                .code(step.code)
+                .message(if (step.code in 200..299) "OK" else "Error")
+                .body(ResponseBody.create(TEXT_PLAIN, step.body))
+                .build()
+      }
+    }
+  }
+
+  /** Drives the check against [steps] and returns the reported status plus the attempt count. */
+  private fun check(vararg steps: Step): Pair<Boolean, Int> {
+    val packager = ScriptedPackager(steps.toList())
+    // Disable OkHttp's own connection-failure retries so the count reflects only what
+    // PackagerStatusCheck itself re-enqueued.
+    val client =
+        OkHttpClient.Builder().retryOnConnectionFailure(false).addInterceptor(packager).build()
+    val status = AtomicBoolean(false)
+    val done = CountDownLatch(1)
+
+    PackagerStatusCheck(client).run(PACKAGER_HOST) { packagerIsRunning ->
+      status.set(packagerIsRunning)
+      done.countDown()
+    }
+
+    assertThat(done.await(30, TimeUnit.SECONDS))
+        .describedAs("PackagerStatusCheck never invoked its callback")
+        .isTrue()
+    return status.get() to packager.requestCount.get()
+  }
+
+  private fun runningResponse(): Step = Step.HttpResponse(200, PACKAGER_RUNNING_BODY)
+
+  @Test
+  fun reportsRunningOnFirstSuccessfulResponse() {
+    val (isRunning, requestCount) = check(runningResponse())
+
+    assertThat(isRunning).isTrue()
+    assertThat(requestCount).isEqualTo(1)
+  }
+
+  @Test
+  fun retriesTransportFailuresBeforeGivingUp() {
+    // Every attempt fails to connect - the transport-level failure a dev server behind a down
+    // tunnel produces. The callback must still be invoked exactly once, reporting "not running".
+    val packager =
+        ScriptedPackager(
+            listOf(Step.TransportFailure, Step.TransportFailure, Step.TransportFailure)
+        )
+    val client =
+        OkHttpClient.Builder().retryOnConnectionFailure(false).addInterceptor(packager).build()
+    val callbackCount = AtomicInteger(0)
+    val status = AtomicBoolean(true)
+    val done = CountDownLatch(1)
+
+    PackagerStatusCheck(client).run(PACKAGER_HOST) { packagerIsRunning ->
+      callbackCount.incrementAndGet()
+      status.set(packagerIsRunning)
+      done.countDown()
+    }
+
+    assertThat(done.await(30, TimeUnit.SECONDS))
+        .describedAs("PackagerStatusCheck never invoked its callback")
+        .isTrue()
+    assertThat(status.get()).isFalse()
+    assertThat(callbackCount.get()).isEqualTo(1)
+    assertThat(packager.requestCount.get()).isEqualTo(3)
+  }
+
+  @Test
+  fun retriesAfterServerErrorAndSucceeds() {
+    val (isRunning, requestCount) = check(Step.HttpResponse(503), runningResponse())
+
+    assertThat(isRunning).isTrue()
+    assertThat(requestCount).isEqualTo(2)
+  }
+
+  @Test
+  fun givesUpAfterBoundedNumberOfAttempts() {
+    val (isRunning, requestCount) =
+        check(
+            Step.HttpResponse(503),
+            Step.HttpResponse(503),
+            Step.HttpResponse(503),
+        )
+
+    assertThat(isRunning).isFalse()
+    assertThat(requestCount).isEqualTo(3)
+  }
+
+  @Test
+  fun doesNotRetryClientError() {
+    val (isRunning, requestCount) = check(Step.HttpResponse(404))
+
+    assertThat(isRunning).isFalse()
+    assertThat(requestCount).isEqualTo(1)
+  }
+
+  @Test
+  fun doesNotRetryWhenSomethingOtherThanMetroResponds() {
+    val (isRunning, requestCount) = check(Step.HttpResponse(200, "not metro"))
+
+    assertThat(isRunning).isFalse()
+    assertThat(requestCount).isEqualTo(1)
+  }
+
+  private companion object {
+    private const val PACKAGER_RUNNING_BODY = "packager-status:running"
+    // Never contacted: the interceptor short-circuits every call before any socket work.
+    private const val PACKAGER_HOST = "localhost:8081"
+    private val TEXT_PLAIN: MediaType? = MediaType.parse("text/plain; charset=utf-8")
+  }
+}