From beb58f196c4c31c3ccdc2a9610e39449f97bf4e0 Mon Sep 17 00:00:00 2001
From: Theodore Schnepper <tschnepper@espressosys.com>
Date: Fri, 10 Jan 2025 07:44:18 -0700
Subject: [PATCH] Adjust health check logic of `wait_for_service` function

There are a few potential issues of note with the `wait_for_service` function. This function
is intended to check if a service is ready by ensuring its health check passes before
interacting with it.

* The first potential issue is the URL construction.  The string `/healthcheck` is appended
to the end of the given URL. This assumes that the URL given does not have a path at all.
* The second potential issue has to do with how we check for a "success".  The success is
determined by receiving any response back from the server.  This means that if the server
returns an error code, such as `404`, the health check would still pass. Normally health
checks must ensure that a 200 status code is returned.

To fix these potential issues we addressed them directly.

First he URL construction was modified to use the `Url::join` method, as it is aware of
relative and absolute path references.

Second the reqwest call was re-written to ensure that the response retrieved is an Ok.
This didn't have a direct chagne to the logic, but helped with the readability of
the next portion, and it ensures that the `response` variable persists beyond a single
closure.

Third the response's status code is checked with the
`http::status::StatusCode::is_success` method.

Finally, some comments were added to provide a simple explanation of what the
`wait_for_service` function is doing, and some behavior implications.

Beyond these changes, no other behavior modifications have been made.  This means that
the health check still checks to see of the `Response` is able to retrieve a textual
representation of the response.
---
 tests/common/mod.rs | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/tests/common/mod.rs b/tests/common/mod.rs
index 1b5636af5..22b206cfb 100644
--- a/tests/common/mod.rs
+++ b/tests/common/mod.rs
@@ -237,20 +237,41 @@ pub async fn get_builder_address(url: Url) -> Address {
     panic!("Error: Failed to retrieve address from builder!");
 }
 
+/// [wait_for_service] will check to see if a service, identified by the given
+/// Url, is available, by checking it's health check endpoint.  If the health
+/// check does not any time before the timeout, then the service will return
+/// an [Err] with the relevant error.
+///
+/// > Note: This function only waits for a single health check pass before
+/// > returning an [Ok] result.
 async fn wait_for_service(url: Url, interval: u64, timeout_duration: u64) -> Result<String> {
+    // utilize the correct path for the health check
+    let Ok(url) = url.join("/healthcheck") else {
+        return Err(anyhow!("Wait for service, could not join url: {}", url));
+    };
+
     timeout(Duration::from_secs(timeout_duration), async {
         loop {
-            if let Ok(body) = reqwest::get(format!("{url}/healthcheck")).await {
-                return body.text().await.map_err(|e| {
-                    anyhow!(
-                        "Wait for service, could not decode response: ({}) {}",
-                        url,
-                        e
-                    )
-                });
-            } else {
+            // Ensure that we get a response from the server
+            let Ok(response) = reqwest::get(url.clone()).await else {
+                sleep(Duration::from_millis(interval)).await;
+                continue;
+            };
+
+            // Check the status code of the response
+            if !response.status().is_success() {
+                // The server did not return a success
                 sleep(Duration::from_millis(interval)).await;
+                continue;
             }
+
+            return response.text().await.map_err(|e| {
+                anyhow!(
+                    "Wait for service, could not decode response: ({}) {}",
+                    url,
+                    e
+                )
+            });
         }
     })
     .await