fix: Bubble homeserver error back to user cleanly. Remove middleware

Author: 86667

src/shared/homeserver_admin_api.rs

@@ -53,13 +53,4 @@ impl HomeserverAdminAPI {
         response.error_for_status()?;
         Ok(())
     }
-
-    /// Checks if the homeserver is responsive by making a GET request to the root URL
-    /// This is NOT password protected so it won't validate the admin password.
-    pub async fn health_check(&self) -> Result<(), reqwest::Error> {
-        let url = self.base_url.join("/").expect("Failed to join URL path");
-        let response = self.http_client.get(url).send().await?;
-        response.error_for_status()?;
-        Ok(())
-    }
 }

src/shared/homeserver_health_middleware.rs

@@ -1,5 +1,3 @@
 mod homeserver_admin_api;
-mod homeserver_health_middleware;
 
 pub use homeserver_admin_api::HomeserverAdminAPI;
-pub use homeserver_health_middleware::check_homeserver_health;

src/shared/mod.rs

@@ -8,7 +8,6 @@ use crate::{
 pub struct AppState {
     pub db: SqlDb,
     pub sms_verification: SmsVerificationService,
-    pub homeserver_admin_api: HomeserverAdminAPI,
 }
 
 impl AppState {
@@ -28,7 +27,6 @@ impl AppState {
         Self {
             db,
             sms_verification,
-            homeserver_admin_api,
         }
     }
 }

src/sms_verification/app_state.rs

@@ -24,6 +24,9 @@ pub enum SmsVerificationError {
     #[error("External service rate limit exceeded")]
     RateLimited { retry_after: Option<u64> },
 
+    #[error("Homeserver temporarily unavailable, please retry")]
+    HomeserverUnavailable,
+
     #[error("HTTP request failed: {0}")]
     RequestFailed(#[from] reqwest::Error),
 

src/sms_verification/error.rs

@@ -2,7 +2,6 @@ use axum::{
     Json, Router,
     extract::State,
     http::StatusCode,
-    middleware,
     response::{IntoResponse, Response},
     routing::post,
 };
@@ -22,22 +21,9 @@ pub async fn router(
     db: &crate::infrastructure::sql::SqlDb,
 ) -> Result<Router, HttpServerError> {
     let state = AppState::new(config, db.clone());
-    let homeserver_api = state.homeserver_admin_api.clone();
-
-    // NOTE: SMS verification routes REQUIRE homeserver health check middleware.
-    // Unlike LN verification, SMS cannot use database transactions for atomicity because:
-    // 1. Prelude API destructively marks verification as complete when check_code() succeeds
-    // 2. If generate_signup_token() fails after that, we can't rollback Prelude's state
-    // 3. This would create an inconsistent state: Prelude says "verified", our DB says "failed"
-    //
-    // The middleware mitigates this by failing fast (503) if homeserver is unreachable.
     Ok(Router::new()
         .route("/send_code", post(send_code_handler))
         .route("/validate_code", post(validate_code_handler))
-        .route_layer(middleware::from_fn(move |req, next| {
-            let api = homeserver_api.clone();
-            crate::shared::check_homeserver_health(api, req, next)
-        }))
         .with_state(state))
 }
 
@@ -95,6 +81,10 @@ impl IntoResponse for SmsVerificationError {
                 }
                 return response;
             }
+            SmsVerificationError::HomeserverUnavailable => {
+                tracing::error!("Homeserver unavailable during SMS verification");
+                StatusCode::INTERNAL_SERVER_ERROR
+            }
             SmsVerificationError::RequestFailed(ref err) => {
                 tracing::error!(error = %err, "Failed to communicate with SMS provider");
                 StatusCode::INTERNAL_SERVER_ERROR
@@ -309,124 +299,4 @@ mod tests {
 
         // Wiremock verifies all expected calls were made
     }
-
-    /// Integration tests that verify the homeserver health middleware actually works
-    /// when integrated with the full router (not router_with_db which bypasses middleware)
-    mod middleware_integration {
-        use super::*;
-        use crate::e2e::WiremockServers;
-        use wiremock::{
-            Mock, ResponseTemplate,
-            matchers::{method, path},
-        };
-
-        /// Helper to create a router with the production path that includes middleware
-        async fn create_router_with_middleware(
-            pool: PgPool,
-            servers: &WiremockServers,
-        ) -> (TestServer, PgPool) {
-            use crate::EnvConfig;
-            use crate::infrastructure::sql::SqlDb;
-
-            let config = EnvConfig::for_test(
-                servers.prelude_server.uri().parse().unwrap(),
-                servers.homeserver_server.uri().parse().unwrap(),
-            );
-
-            let db = SqlDb::test(pool.clone()).await;
-
-            // Use the production router() function which includes middleware
-            let sms_verification_router =
-                router(&config, &db).await.expect("Failed to create router");
-
-            let router = Router::new().nest("/sms_verification", sms_verification_router);
-            let app = router.into_make_service_with_connect_info::<SocketAddr>();
-            let server = TestServer::new(app).expect("Failed to create test server");
-
-            (server, pool)
-        }
-
-        #[sqlx::test]
-        async fn middleware_blocks_send_code_when_homeserver_is_down(pool: PgPool) {
-            let servers = WiremockServers::start().await;
-
-            // Setup homeserver to be unresponsive (return 500)
-            Mock::given(method("GET"))
-                .and(path("/"))
-                .respond_with(ResponseTemplate::new(500))
-                .expect(1)
-                .mount(&servers.homeserver_server)
-                .await;
-
-            let (server, _pool) = create_router_with_middleware(pool, &servers).await;
-
-            // Attempt to send code
-            let response = server
-                .post("/sms_verification/send_code")
-                .json(&serde_json::json!({ "phoneNumber": "+30123456789" }))
-                .await;
-
-            // Should return 503 due to homeserver being down
-            response.assert_status(StatusCode::SERVICE_UNAVAILABLE);
-            assert_eq!(
-                response.text(),
-                "Homeserver temporarily unavailable, please retry"
-            );
-        }
-
-        #[sqlx::test]
-        async fn middleware_blocks_validate_code_when_homeserver_is_down(pool: PgPool) {
-            let servers = WiremockServers::start().await;
-
-            // Setup homeserver to return 500
-            Mock::given(method("GET"))
-                .and(path("/"))
-                .respond_with(ResponseTemplate::new(500))
-                .expect(1)
-                .mount(&servers.homeserver_server)
-                .await;
-
-            let (server, _pool) = create_router_with_middleware(pool, &servers).await;
-
-            // Attempt to validate code when homeserver is down
-            // Should be blocked by middleware before reaching the handler
-            let response = server
-                .post("/sms_verification/validate_code")
-                .json(&serde_json::json!({
-                    "phoneNumber": "+30123456789",
-                    "code": "123456"
-                }))
-                .await;
-
-            // Should return 503 due to homeserver being down
-            // The middleware blocks the request before it reaches the handler
-            response.assert_status(StatusCode::SERVICE_UNAVAILABLE);
-            assert_eq!(
-                response.text(),
-                "Homeserver temporarily unavailable, please retry"
-            );
-        }
-
-        #[sqlx::test]
-        async fn middleware_blocks_when_homeserver_is_unreachable(pool: PgPool) {
-            let servers = WiremockServers::start().await;
-
-            // Don't mount any mock - this simulates connection refused
-            // The middleware will try to connect and fail
-
-            let (server, _pool) = create_router_with_middleware(pool, &servers).await;
-
-            // Attempt to send code - should be blocked immediately by middleware
-            let response = server
-                .post("/sms_verification/send_code")
-                .json(&serde_json::json!({ "phoneNumber": "+30123456789" }))
-                .await;
-
-            response.assert_status(StatusCode::SERVICE_UNAVAILABLE);
-            assert_eq!(
-                response.text(),
-                "Homeserver temporarily unavailable, please retry"
-            );
-        }
-    }
 }

src/sms_verification/http.rs

@@ -159,7 +159,7 @@ impl SmsVerificationService {
             PreludeCheckCodeResponse::Success { id, .. } => {
                 let code = match self.homeserver_admin_api.generate_signup_token().await {
                     Ok(code) => code,
-                    Err(e) => {
+                    Err(_e) => {
                         if let Err(e) = SmsVerificationRepository::mark_failed(
                             &mut executor,
                             &id,
@@ -169,7 +169,7 @@ impl SmsVerificationService {
                         {
                             tracing::error!("{}", e);
                         }
-                        return Err(e.into());
+                        return Err(SmsVerificationError::HomeserverUnavailable);
                     }
                 };
 

src/sms_verification/service.rs

@@ -794,6 +794,20 @@ async fn test_service_success_but_homeserver_fails_marks_failed(pool: PgPool) {
         record.failure_reason,
         Some("homeserver_signup_token_generation_failed".to_string())
     );
+
+    // Verify that failed verification does NOT count towards quota
+    let phone_hash = test_phone_hasher().hash_phone_number(phone.as_str());
+    let failed_count = SmsVerificationRepository::count_verified_sessions_in_last_days(
+        &mut executor,
+        &phone_hash,
+        7,
+    )
+    .await
+    .unwrap();
+    assert_eq!(
+        failed_count, 0,
+        "Failed verification should NOT count towards weekly quota"
+    );
 }
 
 // This circumstance happens if validate_code() is called before send_code() - Prelude has no prelude_id for the verification session yet so it returns a dummy value which doesnt match to anything in our db.