From 9de68e1533b0c45b878a8714cf8c937736e514fc Mon Sep 17 00:00:00 2001 From: Gautier DI FOLCO Date: Thu, 2 Jul 2026 22:59:32 +0200 Subject: [PATCH 1/2] fix: guard lookupField against Null body in ensureBackendReachable When the federator returns a response with an empty/Null JSON body during backend warmup, lookupField res.json "inner" threw an uncaught assertion failure that aborted the retry loop (the surrounding E.try only catches HttpException). Wrap the call with catchAll so a Null body yields Nothing and the retry continues, instead of crashing testNotificationsForOfflineBackends. --- integration/test/Testlib/ModService.hs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integration/test/Testlib/ModService.hs b/integration/test/Testlib/ModService.hs index 1bb0390ec3..04dfd063f7 100644 --- a/integration/test/Testlib/ModService.hs +++ b/integration/test/Testlib/ModService.hs @@ -33,7 +33,7 @@ import Control.Applicative import Control.Concurrent import Control.Concurrent.Async import qualified Control.Exception as E -import Control.Monad.Catch (catch, throwM) +import Control.Monad.Catch (catch, catchAll, throwM) import Control.Monad.Codensity import Control.Monad.Extra import Control.Monad.Reader @@ -422,7 +422,7 @@ ensureBackendReachable domain = do -- If we get 533 here it means federation is not available between domains -- but ingress is working, since we're processing the request. let is200 = res.status == 200 - mInner <- lookupField res.json "inner" + mInner <- lookupField res.json "inner" `catchAll` const (pure Nothing) isFedDenied <- case mInner of Nothing -> pure False Just inner -> do From 937a843e3be43bbdf43279b5df4bb9e85443def3 Mon Sep 17 00:00:00 2001 From: Gautier DI FOLCO Date: Thu, 9 Jul 2026 16:40:38 +0200 Subject: [PATCH 2/2] fix: wait for warmup in ensureBackendReachable instead of catching errors The previous approach wrapped lookupField with catchAll to swallow the AssertionFailure thrown when the federator returns a non-object body during backend warmup (empty, JSON null, or Prometheus metrics text). That had two problems: 1. catchAll masks all exceptions, not just the AssertionFailure we care about. 2. The status check (res.status == 200) alone can false-positive when the metrics middleware returns HTTP 200, declaring the backend reachable before brig is actually serving api-version. Replace it with a body-shape guard: only consider the backend reachable when res.json is a proper JSON Object. For 200 responses, additionally require the VersionInfo "supported" field. The retry loop now genuinely waits for warmup to complete. See review feedback on #5310. --- integration/test/Testlib/ModService.hs | 32 ++++++++++++++++---------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/integration/test/Testlib/ModService.hs b/integration/test/Testlib/ModService.hs index 04dfd063f7..e40b67abef 100644 --- a/integration/test/Testlib/ModService.hs +++ b/integration/test/Testlib/ModService.hs @@ -33,7 +33,7 @@ import Control.Applicative import Control.Concurrent import Control.Concurrent.Async import qualified Control.Exception as E -import Control.Monad.Catch (catch, catchAll, throwM) +import Control.Monad.Catch (catch, throwM) import Control.Monad.Codensity import Control.Monad.Extra import Control.Monad.Reader @@ -419,17 +419,25 @@ ensureBackendReachable domain = do . (addJSONObject []) checkStatus <- appToIO $ do submit "POST" req `bindResponse` \res -> do - -- If we get 533 here it means federation is not available between domains - -- but ingress is working, since we're processing the request. - let is200 = res.status == 200 - mInner <- lookupField res.json "inner" `catchAll` const (pure Nothing) - isFedDenied <- case mInner of - Nothing -> pure False - Just inner -> do - label <- inner %. "label" & asString - pure $ res.status == 533 && label == "federation-denied" - - pure (is200 || isFedDenied) + -- During warmup the federator may transiently return a non-JSON + -- body (e.g. Prometheus metrics text), JSON null, or an empty + -- body. Only consider the backend reachable once brig is + -- actually serving the api-version endpoint, so the retry loop + -- keeps going until warmup is genuinely complete. + case res.json of + Just (Object _) + | res.status == 200 -> + isJust <$> lookupField res.json "supported" + Just (Object _) | res.status == 533 -> do + -- Federation is not available between domains, but ingress + -- is working since we're processing the request. + mInner <- lookupField res.json "inner" + case mInner of + Just inner -> do + label <- inner %. "label" & asString + pure $ label == "federation-denied" + Nothing -> pure False + _ -> pure False eith <- liftIO (E.try checkStatus) pure $ either (\(_e :: HTTP.HttpException) -> False) id eith origins = [env.domain1, env.domain2]