From 6f4dd050e1edf630424a7ad70021ab807d59cd5c Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 3 Sep 2026 14:32:27 +0100 Subject: [PATCH 1/2] =?UTF-8?q?feat(fl):=20tolerate=20unreachable=20fl-ser?= =?UTF-8?q?ver=20at=20boot=20=E2=80=94=20per-job=20Phase=200=20(#735)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 0 of #735: decouple fl-api and flip-api from the assumption that the fl-server is always up, behind the PER_JOB_FL_SERVER flag (default false). - FLIP_Session: lazy first-use connect via _reconnect (fresh Session.__init__), not try_connect. _connected tracks "currently connected"; _do_command handles NoConnection; check_server_status reports STOPPED when unreachable. - session_manager: tolerate transport-down and not-ready at boot when the flag is on; auth/identity failures still raise. - flip-api: skip keep-alive ping when the flag is on and no net is BUSY. - PER_JOB_FL_SERVER threaded through both services, Terraform, NVFLARE composes, .env.example; root Makefile no longer exports commented .env lines. Refs #735 Signed-off-by: R. Garcia-Dias --- .env.development.example | 4 ++ Makefile | 2 +- deploy/compose.development.nvflare.yml | 2 + deploy/compose.production.nvflare.yml | 1 + deploy/providers/AWS/locals.tf | 2 + deploy/providers/AWS/variables.tf | 6 +++ fl-services/nvflare/compose.dev.yml | 1 + .../nvflare/fl-api-base/fl_api/config.py | 16 ++++++ .../fl_api/startup/session_manager.py | 40 ++++++++++++-- .../fl-api-base/fl_api/utils/flip_session.py | 44 +++++++++++++++- .../tests/startup/test_session_manager.py | 47 +++++++++++++++++ .../tests/utils/test_flip_session.py | 52 ++++++++++++++++++- flip-api/src/flip_api/config.py | 16 ++++++ .../fl_services/services/fl_service.py | 11 ++++ .../fl_services/services/test_fl_service.py | 38 ++++++++++++++ 15 files changed, 274 insertions(+), 8 deletions(-) diff --git a/.env.development.example b/.env.development.example index 5734f79f8..414c58415 100644 --- a/.env.development.example +++ b/.env.development.example @@ -149,6 +149,10 @@ LOCAL_DEV=true # Directory inside the FL container for admin/startup files FL_ADMIN_DIRECTORY=/app/admin +# Per-job fl-server scale-to-zero (FLIP#735 Phase 0). Off by default. When on, fl-api tolerates +# an unreachable fl-server at boot and connects lazily on first use; flip-api skips the keep-alive +# ping for scaled-to-zero nets. Leave commented (or empty) for false. +# PER_JOB_FL_SERVER=false # Ports for the FL API, server, and client containers on the Central Hub FL_API_PORT=8000 FL_SERVER_PORT=8002 diff --git a/Makefile b/Makefile index e73313994..0cc58942d 100644 --- a/Makefile +++ b/Makefile @@ -42,7 +42,7 @@ $(info Using MAIN_ENV_FILE: $(MAIN_ENV_FILE)) # replace environment variables by the values from the .env files ifneq ("$(wildcard $(MAIN_ENV_FILE))","") include $(MAIN_ENV_FILE) -export $(shell sed 's/=.*//' $(MAIN_ENV_FILE)) +export $(shell grep -v '^[[:space:]]*#' $(MAIN_ENV_FILE) | sed 's/=.*//') endif include deploy/fl_backend.mk diff --git a/deploy/compose.development.nvflare.yml b/deploy/compose.development.nvflare.yml index 848fff36e..3836ff2b4 100644 --- a/deploy/compose.development.nvflare.yml +++ b/deploy/compose.development.nvflare.yml @@ -25,6 +25,7 @@ services: environment: - FL_ADMIN_DIRECTORY=${FL_ADMIN_DIRECTORY} - DEBUG=${DEBUG} + - PER_JOB_FL_SERVER=${PER_JOB_FL_SERVER:-false} # Shared volume where the FL API stages a de-bundled eval checkpoint (mirrors Flower's /app/src). - SERVER_CHECKPOINT_ROOT=/app/server-checkpoints # Per-job GPU request written into meta.json resource_spec by configure_meta. Default 0 => @@ -62,6 +63,7 @@ services: environment: - FL_ADMIN_DIRECTORY=${FL_ADMIN_DIRECTORY} - DEBUG=${DEBUG} + - PER_JOB_FL_SERVER=${PER_JOB_FL_SERVER:-false} # Shared volume where the FL API stages a de-bundled eval checkpoint (mirrors Flower's /app/src). - SERVER_CHECKPOINT_ROOT=/app/server-checkpoints # Per-job GPU request (see fl-api-net-1). Default 0 => CPU; set >0 to train on the GPU. diff --git a/deploy/compose.production.nvflare.yml b/deploy/compose.production.nvflare.yml index 9213933a8..352225493 100644 --- a/deploy/compose.production.nvflare.yml +++ b/deploy/compose.production.nvflare.yml @@ -30,6 +30,7 @@ services: image: ${DOCKER_FL_REGISTRY}${DOCKER_FL_API_NAME}:${DOCKER_FL_TAG} environment: - FL_ADMIN_DIRECTORY=${FL_ADMIN_DIRECTORY} + - PER_JOB_FL_SERVER=${PER_JOB_FL_SERVER:-false} # Shared volume where the FL API stages a de-bundled eval checkpoint (mirrors Flower's /app/src). - SERVER_CHECKPOINT_ROOT=/app/server-checkpoints # Per-job GPU request written into meta.json resource_spec by configure_meta. Default 0 => diff --git a/deploy/providers/AWS/locals.tf b/deploy/providers/AWS/locals.tf index d48e0961b..7b5fac722 100644 --- a/deploy/providers/AWS/locals.tf +++ b/deploy/providers/AWS/locals.tf @@ -127,6 +127,7 @@ locals { FL_APP_DESTINATION_BUCKET = local.fl_app_destination_uri NET_ENDPOINTS = local.net_endpoints_json FL_BACKEND = var.fl_backend + PER_JOB_FL_SERVER = tostring(var.PER_JOB_FL_SERVER) # Raise the per-file model-upload cap from the 100 MiB Settings default # to 5 GB. This is the practical ceiling for the current upload path: a # browser presigned POST (services.tf) is a *single* S3 POST, and S3 @@ -166,6 +167,7 @@ locals { # startup is a dead container (replaced by ECS), not a zombie (FLIP#593 pt.1). ENV = "production" FL_ADMIN_DIRECTORY = var.FL_ADMIN_DIRECTORY + PER_JOB_FL_SERVER = tostring(var.PER_JOB_FL_SERVER) # Writer side of the shared checkpoint-staging volume: fl-api de-bundles a # large eval checkpoint out of the client app and writes it to # // for the fl-server to load (FLIP#695). Same path the diff --git a/deploy/providers/AWS/variables.tf b/deploy/providers/AWS/variables.tf index a4415b9bc..10c07002c 100644 --- a/deploy/providers/AWS/variables.tf +++ b/deploy/providers/AWS/variables.tf @@ -179,6 +179,12 @@ variable "MIN_CLIENTS" { default = 1 } +variable "PER_JOB_FL_SERVER" { + description = "Per-job fl-server scale-to-zero (FLIP#735 Phase 0). When true, fl-api tolerates an unreachable fl-server at boot and connects lazily on first use, and flip-api skips the keep-alive ping for scaled-to-zero nets. Off by default." + type = bool + default = false +} + # Per-job GPU resource spec requested by the fl-api when it builds an NVFLARE # job's meta (mirrors JOB_RESOURCE_SPEC_* in compose.production.nvflare.yml). # This drives client-side GPU allocation — the hub's Fargate tasks are CPU-only; diff --git a/fl-services/nvflare/compose.dev.yml b/fl-services/nvflare/compose.dev.yml index 7f0f97357..105b6cb4e 100644 --- a/fl-services/nvflare/compose.dev.yml +++ b/fl-services/nvflare/compose.dev.yml @@ -68,6 +68,7 @@ services: - FL_ADMIN_DIRECTORY=/app/admin - DEBUG=${DEBUG} - LOG_LEVEL=${LOG_LEVEL} + - PER_JOB_FL_SERVER=${PER_JOB_FL_SERVER:-false} ports: - "${FL_API_PORT}:8000" - "5679:5679" diff --git a/fl-services/nvflare/fl-api-base/fl_api/config.py b/fl-services/nvflare/fl-api-base/fl_api/config.py index 586759336..979a1fb8f 100644 --- a/fl-services/nvflare/fl-api-base/fl_api/config.py +++ b/fl-services/nvflare/fl-api-base/fl_api/config.py @@ -10,6 +10,7 @@ # limitations under the License. # +from pydantic import field_validator from pydantic_settings import BaseSettings, SettingsConfigDict @@ -25,6 +26,21 @@ class Settings(BaseSettings): TIMEOUT_SESSION_CONNECT: float = 20.0 + # Per-job fl-server scale-to-zero (FLIP#735 Phase 0). Default off. When on, fl-api tolerates + # an unreachable fl-server at boot and connects lazily on first use. Parsed leniently so the + # empty string the root Makefile can export for a commented .env line means False — and so + # "yes"/"on"/"true"/"1" mean True identically in BOTH fl-api-base and flip-api. + PER_JOB_FL_SERVER: bool = False + + @field_validator("PER_JOB_FL_SERVER", mode="before") + @classmethod + def coerce_empty_per_job_fl_server(cls, v: str | bool | None) -> bool: + if isinstance(v, bool): + return v + if v is None: + return False + return str(v).strip().lower() in ("true", "1", "yes", "on") + # GPU resources that the submitted NVFLARE jobs need in order to schedule correctly. # TODO Currently this is set globally for all jobs, but we should allow per-job overrides in the future. # See https://github.com/londonaicentre/flip/issues/41 diff --git a/fl-services/nvflare/fl-api-base/fl_api/startup/session_manager.py b/fl-services/nvflare/fl-api-base/fl_api/startup/session_manager.py index 1aa0dfd17..0366b0cbd 100644 --- a/fl-services/nvflare/fl-api-base/fl_api/startup/session_manager.py +++ b/fl-services/nvflare/fl-api-base/fl_api/startup/session_manager.py @@ -11,6 +11,9 @@ # +from nvflare.apis.fl_exception import FLCommunicationError +from nvflare.fuel.flare_api.api_spec import InternalError, NoConnection + from fl_api.config import get_settings from fl_api.utils.flip_session import FLIP_Session from fl_api.utils.logger import logger @@ -43,10 +46,41 @@ def create_fl_session() -> FLIP_Session: debug=debug, ) - # Try connecting the session here, so that we can catch any connection issues at startup - session.try_connect(get_settings().TIMEOUT_SESSION_CONNECT) - logger.info(f"Upload directory set to: {session.upload_dir}") logger.info(f"Download directory set to: {session.download_dir}") + # Try connecting the session here, so that we can catch any connection issues at startup. + # + # With PER_JOB_FL_SERVER off, an unreachable fl-server at boot is fatal (unchanged) — the + # normal alerting signal for an unplanned outage. With it on, the server is expected to be + # down most of the time (scaled to zero between jobs, FLIP#735), so transport failures are + # tolerated and the session connects lazily on first use (FLIP_Session._do_command). + # + # Discrimination is deliberate: transport-down and not-ready are tolerable; auth/identity + # failures are not. A wrong admin kit (AuthenticationError) or a server-identity mismatch + # (FLCommunicationError, or NoConnection with "cannot authenticate") still raise either way. + try: + session.try_connect(get_settings().TIMEOUT_SESSION_CONNECT) + except NoConnection as e: + if "cannot authenticate" in str(e) or not get_settings().PER_JOB_FL_SERVER: + raise + logger.warning( + "fl-server unreachable at boot (cannot connect); PER_JOB_FL_SERVER is on — treating " + "this as the normal idle-between-jobs state. Will connect lazily on first use. " + "Reason: %s", + e, + ) + except InternalError as e: + if not get_settings().PER_JOB_FL_SERVER: + raise + logger.warning( + "fl-server login failed at boot (server up but not ready); PER_JOB_FL_SERVER is on — " + "treating this as a cold start. Will connect lazily on first use. Reason: %s", + e, + ) + except FLCommunicationError: + # Rejected registration / server-identity mismatch — a misconfiguration, never the + # idle-between-jobs state. Fatal regardless of the flag. + raise + return session diff --git a/fl-services/nvflare/fl-api-base/fl_api/utils/flip_session.py b/fl-services/nvflare/fl-api-base/fl_api/utils/flip_session.py index 52ff94d99..f8f9a3b0e 100644 --- a/fl-services/nvflare/fl-api-base/fl_api/utils/flip_session.py +++ b/fl-services/nvflare/fl-api-base/fl_api/utils/flip_session.py @@ -13,7 +13,8 @@ from typing import Any -from nvflare.fuel.flare_api.api_spec import InternalError, SessionClosed +from nvflare.apis.fl_exception import FLCommunicationError +from nvflare.fuel.flare_api.api_spec import InternalError, NoConnection, SessionClosed from nvflare.fuel.flare_api.flare_api import Session from fl_api.utils.logger import logger @@ -35,6 +36,24 @@ class FLIP_Session(Session): ``test_flip_session.py`` pins this for every override. """ + # Defaults False so a bare ``FLIP_Session(...)`` (no ``__init__`` override, FLIP#1032) starts + # disconnected; ``try_connect`` flips it True on success. "Currently connected", not "ever + # connected": a failed reconnect must leave it False so the next command re-connects instead + # of reusing a half-built session. + _connected: bool = False + + def try_connect(self, timeout: float) -> None: + """Connect the underlying admin API, tracking success for ``_do_command``'s lazy + first-use connect (see ``PER_JOB_FL_SERVER`` in ``session_manager.py``).""" + self._connected = False + super().try_connect(timeout) + self._connected = True + + @property + def is_connected(self) -> bool: + """Whether the admin session is currently connected (not merely ever-was).""" + return self._connected + def _reconnect(self) -> None: """Re-initialise the underlying admin API and log in again after the session was closed. @@ -82,6 +101,14 @@ def _do_command(self, command: str, *args: Any, **kwargs: Any) -> Any: *args (Any): Positional arguments forwarded to the base implementation. **kwargs (Any): Keyword arguments forwarded to the base implementation. """ + if not self._connected: + # Lazy first-use connect (PER_JOB_FL_SERVER let boot proceed with the server down). + # ``_reconnect`` (fresh ``Session.__init__``), not ``try_connect``: a failed boot-time + # connect already assigned+started the cell before auth raised, so ``try_connect``'s + # ``if self.cell: return`` would short-circuit re-authentication on the stale cell. + logger.info("Session not connected; connecting now before command: %s", command) + self._reconnect() + try: return super()._do_command(command, *args, **kwargs) except InternalError as e: @@ -90,6 +117,15 @@ def _do_command(self, command: str, *args: Any, **kwargs: Any) -> Any: self.try_connect(timeout=5.0) return super()._do_command(command, *args, **kwargs) raise e + except NoConnection: + logger.warning("No connection to FL server; reconnecting and retrying command.") + self._connected = False + self._reconnect() + try: + return super()._do_command(command, *args, **kwargs) + except Exception: + logger.error("Retry after reconnect failed for command: %s", command) + raise except SessionClosed: logger.warning("Session closed; attempting to reconnect and retry command.") self._reconnect() @@ -109,7 +145,11 @@ def check_server_status(self) -> ServerInfoModel: Returns: ServerInfoModel: a ServerInfoModel object containing the server status and start time. """ - return self.get_system_info().server_info + try: + return self.get_system_info().server_info + except (NoConnection, SessionClosed, InternalError, FLCommunicationError) as e: + logger.warning("FL server unreachable; reporting STOPPED: %s", e) + return ServerInfoModel(status="STOPPED") def check_client_status(self, target: list[str] | None = None) -> list[ClientInfoModel]: """ diff --git a/fl-services/nvflare/fl-api-base/tests/startup/test_session_manager.py b/fl-services/nvflare/fl-api-base/tests/startup/test_session_manager.py index 36f42634c..9e97ac0ea 100644 --- a/fl-services/nvflare/fl-api-base/tests/startup/test_session_manager.py +++ b/fl-services/nvflare/fl-api-base/tests/startup/test_session_manager.py @@ -14,6 +14,8 @@ from unittest.mock import MagicMock, patch import pytest +from nvflare.apis.fl_exception import FLCommunicationError +from nvflare.fuel.flare_api.api_spec import NoConnection from fl_api.startup.session_manager import create_fl_session @@ -30,6 +32,7 @@ class Settings: JOB_RESOURCE_SPEC_NUM_GPUS = 2 JOB_RESOURCE_SPEC_MEM_PER_GPU_IN_GIB = 8 TIMEOUT_SESSION_CONNECT = 5.0 + PER_JOB_FL_SERVER = False os.makedirs(Settings.FL_ADMIN_DIRECTORY, exist_ok=True) return Settings() @@ -50,3 +53,47 @@ def test_create_fl_session_success(fake_settings): assert session == mock_session assert session.upload_dir == "/tmp/upload" assert session.download_dir == "/tmp/download" + + +def test_create_fl_session_tolerates_unreachable_server_when_flag_on(fake_settings): + """✅ PER_JOB_FL_SERVER on: transport-down at boot is tolerated (lazy connect later).""" + fake_settings.PER_JOB_FL_SERVER = True + mock_session = MagicMock() + mock_session.upload_dir = "/tmp/upload" + mock_session.download_dir = "/tmp/download" + mock_session.try_connect.side_effect = NoConnection("cannot connect to server") + + with ( + patch("fl_api.startup.session_manager.get_settings", return_value=fake_settings), + patch("fl_api.startup.session_manager.FLIP_Session", return_value=mock_session), + ): + session = create_fl_session() + + assert session == mock_session + + +def test_create_fl_session_raises_unreachable_when_flag_off(fake_settings): + """✅ PER_JOB_FL_SERVER off (default): transport-down at boot stays fatal.""" + mock_session = MagicMock() + mock_session.try_connect.side_effect = NoConnection("cannot connect to server") + + with ( + patch("fl_api.startup.session_manager.get_settings", return_value=fake_settings), + patch("fl_api.startup.session_manager.FLIP_Session", return_value=mock_session), + ): + with pytest.raises(NoConnection): + create_fl_session() + + +def test_create_fl_session_raises_on_identity_mismatch_even_when_flag_on(fake_settings): + """✅ PER_JOB_FL_SERVER on: identity mismatch is a misconfiguration, always fatal.""" + fake_settings.PER_JOB_FL_SERVER = True + mock_session = MagicMock() + mock_session.try_connect.side_effect = FLCommunicationError("rejected registration") + + with ( + patch("fl_api.startup.session_manager.get_settings", return_value=fake_settings), + patch("fl_api.startup.session_manager.FLIP_Session", return_value=mock_session), + ): + with pytest.raises(FLCommunicationError): + create_fl_session() diff --git a/fl-services/nvflare/fl-api-base/tests/utils/test_flip_session.py b/fl-services/nvflare/fl-api-base/tests/utils/test_flip_session.py index 9f1ccc783..01439c461 100644 --- a/fl-services/nvflare/fl-api-base/tests/utils/test_flip_session.py +++ b/fl-services/nvflare/fl-api-base/tests/utils/test_flip_session.py @@ -16,7 +16,14 @@ from unittest.mock import call, patch import pytest -from nvflare.fuel.flare_api.api_spec import ClientInfo, InternalError, ServerInfo, SessionClosed, SystemInfo +from nvflare.fuel.flare_api.api_spec import ( + ClientInfo, + InternalError, + NoConnection, + ServerInfo, + SessionClosed, + SystemInfo, +) from nvflare.fuel.flare_api.flare_api import Session from fl_api.utils.flip_session import FLIP_Session @@ -32,7 +39,10 @@ def session(mock_session_init): Create a FLIP_Session without invoking real NVFlare Session initialization. """ mock_session_init.return_value = None - return FLIP_Session(username="u", startup_path="p", secure_mode=False, debug=False) + s = FLIP_Session(username="u", startup_path="p", secure_mode=False, debug=False) + # Most tests exercise post-connect behaviour; the lazy first-use connect is tested separately. + s._connected = True + return s def test_do_command_retries_once_on_session_inactive(session): @@ -86,6 +96,44 @@ def test_do_command_propagates_exception_if_retry_fails_after_reconnect(session) assert parent_do_command.call_count == 2 +def test_do_command_lazy_connects_when_not_connected(session): + """✅ Never-connected session (PER_JOB_FL_SERVER boot) reconnects before first command.""" + session._connected = False + with ( + patch("nvflare.fuel.flare_api.flare_api.Session._do_command", return_value="ok") as parent_do_command, + patch.object(session, "_reconnect") as mock_reconnect, + ): + result = session._do_command("CMD") + + mock_reconnect.assert_called_once_with() + parent_do_command.assert_called_once_with("CMD") + assert result == "ok" + + +def test_do_command_retries_once_on_no_connection(session): + """✅ NoConnection (server went away) → full reconnect + retry once.""" + with ( + patch( + "nvflare.fuel.flare_api.flare_api.Session._do_command", + side_effect=[NoConnection("cannot connect to server"), "ok"], + ) as parent_do_command, + patch.object(session, "_reconnect") as mock_reconnect, + ): + result = session._do_command("CMD") + + mock_reconnect.assert_called_once_with() + assert parent_do_command.call_count == 2 + assert result == "ok" + + +def test_check_server_status_reports_stopped_when_unreachable(session): + with patch.object(session, "get_system_info", side_effect=NoConnection("cannot connect to server")): + out = session.check_server_status() + + assert isinstance(out, ServerInfoModel) + assert out.status == "STOPPED" + + def test_check_server_status_returns_server_info(session): sys_info = SystemInfoModel( server_info=ServerInfoModel(status="running", start_time=123.0), diff --git a/flip-api/src/flip_api/config.py b/flip-api/src/flip_api/config.py index c84b50c33..2a3c09ea5 100644 --- a/flip-api/src/flip_api/config.py +++ b/flip-api/src/flip_api/config.py @@ -154,6 +154,22 @@ class Settings(BaseSettings): # it on locally to verify MFA end-to-end without pretending to be prod. ENFORCE_MFA: bool = True + # Per-job fl-server scale-to-zero (FLIP#735 Phase 0). Default off. When on, the keep-alive + # scheduler skips nets whose fl-server is scaled to zero. Parsed leniently so the empty string + # the root Makefile can export for a commented .env line means False — and so + # "yes"/"on"/"true"/"1" mean True identically in BOTH flip-api and fl-api-base. + PER_JOB_FL_SERVER: bool = False + + @field_validator("PER_JOB_FL_SERVER", mode="before") + @classmethod + def coerce_empty_per_job_fl_server(cls, v: str | bool | None) -> bool: + """Treat empty/None as False; accept true/1/yes/on (matching fl-api-base).""" + if v is None or v == "": + return False + if isinstance(v, bool): + return v + return v.lower() in ("true", "1", "yes", "on") # type: ignore[union-attr] + @field_validator("ENV", mode="before") @classmethod def coerce_empty_env(cls, v: str) -> str: diff --git a/flip-api/src/flip_api/fl_services/services/fl_service.py b/flip-api/src/flip_api/fl_services/services/fl_service.py index f5023ef36..6b327db14 100644 --- a/flip-api/src/flip_api/fl_services/services/fl_service.py +++ b/flip-api/src/flip_api/fl_services/services/fl_service.py @@ -1136,6 +1136,8 @@ def keep_fl_api_session_alive() -> None: TODO This was developed for the NVFLARE backend and might need to be revisited for the Flower backend. See https://github.com/NVIDIA/NVFlare/discussions/3526#discussioncomment-13574644 """ + from flip_api.db.models.main_models import FLScheduler + from flip_api.domain.schemas.status import NetStatus from flip_api.fl_services.get_status import fetch_server_status from flip_api.fl_services.services import fl_scheduler_service @@ -1149,6 +1151,15 @@ def keep_fl_api_session_alive() -> None: with Session(get_engine()) as db: nets = fl_scheduler_service.get_nets(db) + # PER_JOB_FL_SERVER (FLIP#735 Phase 0): an idle net's fl-server is scaled to zero, so + # there is no session to keep alive. When the flag is on and no net is BUSY, skip the + # pings rather than logging "no response" every 2 minutes for servers down by design. + if get_settings().PER_JOB_FL_SERVER: + busy = db.exec(select(FLScheduler).where(FLScheduler.status == NetStatus.BUSY).limit(1)).first() + if not busy: + logger.debug("PER_JOB_FL_SERVER on and no net is BUSY; skipping keep-alive pings.") + return + for net in nets: try: fetch_server_status(net.endpoint) diff --git a/flip-api/tests/unit/fl_services/services/test_fl_service.py b/flip-api/tests/unit/fl_services/services/test_fl_service.py index 3d742d46a..87907d60a 100644 --- a/flip-api/tests/unit/fl_services/services/test_fl_service.py +++ b/flip-api/tests/unit/fl_services/services/test_fl_service.py @@ -1209,6 +1209,44 @@ def test_keep_fl_api_session_alive_swallows_errors(mock_session, mock_get_nets, mock_fetch.assert_called_once_with("http://net1:8000") +@patch("flip_api.fl_services.services.fl_service.get_settings") +@patch("flip_api.fl_services.get_status.fetch_server_status") +@patch("flip_api.fl_services.services.fl_scheduler_service.get_nets") +@patch("flip_api.fl_services.services.fl_service.Session") +def test_keep_fl_api_session_alive_skips_idle_when_flag_on( + mock_session, mock_get_nets, mock_fetch, mock_get_settings +): + # PER_JOB_FL_SERVER on + no BUSY net: servers are scaled to zero, nothing to keep alive. + mock_get_settings.return_value.PER_JOB_FL_SERVER = True + db = MagicMock() + db.exec.return_value.first.return_value = None + mock_session.return_value.__enter__.return_value = db + mock_get_nets.return_value = [MagicMock(endpoint="http://net1:8000")] + + fl_service.keep_fl_api_session_alive() + + mock_fetch.assert_not_called() + + +@patch("flip_api.fl_services.services.fl_service.get_settings") +@patch("flip_api.fl_services.get_status.fetch_server_status") +@patch("flip_api.fl_services.services.fl_scheduler_service.get_nets") +@patch("flip_api.fl_services.services.fl_service.Session") +def test_keep_fl_api_session_alive_pings_busy_when_flag_on( + mock_session, mock_get_nets, mock_fetch, mock_get_settings +): + # PER_JOB_FL_SERVER on + a BUSY net: the live server's session is still worth keeping alive. + mock_get_settings.return_value.PER_JOB_FL_SERVER = True + db = MagicMock() + db.exec.return_value.first.return_value = MagicMock() + mock_session.return_value.__enter__.return_value = db + mock_get_nets.return_value = [MagicMock(endpoint="http://net1:8000")] + + fl_service.keep_fl_api_session_alive() + + mock_fetch.assert_called_once_with("http://net1:8000") + + # --- submit_job success path --------------------------------------------------------------------- From b897ae4ea831e49cc46dc966f7436761e924c991 Mon Sep 17 00:00:00 2001 From: "R. Garcia-Dias" Date: Thu, 3 Sep 2026 14:44:13 +0100 Subject: [PATCH 2/2] =?UTF-8?q?test(fl):=20cover=20per-job=20tolerate=20br?= =?UTF-8?q?anches=20=E2=80=94=20try=5Fconnect,=20boot-tolerance,=20coerce?= =?UTF-8?q?=20(#735)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the codecov gaps on the Phase 0 draft: try_connect connected-latch behaviour, the InternalError (not-ready) and cannot-authenticate boot branches, and the PER_JOB_FL_SERVER coerce spellings. Refs #735 Signed-off-by: R. Garcia-Dias --- .../tests/startup/test_session_manager.py | 46 ++++++++++++++++++- .../nvflare/fl-api-base/tests/test_config.py | 30 ++++++++++++ .../tests/utils/test_flip_session.py | 21 +++++++++ 3 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 fl-services/nvflare/fl-api-base/tests/test_config.py diff --git a/fl-services/nvflare/fl-api-base/tests/startup/test_session_manager.py b/fl-services/nvflare/fl-api-base/tests/startup/test_session_manager.py index 9e97ac0ea..5322fec67 100644 --- a/fl-services/nvflare/fl-api-base/tests/startup/test_session_manager.py +++ b/fl-services/nvflare/fl-api-base/tests/startup/test_session_manager.py @@ -15,7 +15,7 @@ import pytest from nvflare.apis.fl_exception import FLCommunicationError -from nvflare.fuel.flare_api.api_spec import NoConnection +from nvflare.fuel.flare_api.api_spec import InternalError, NoConnection from fl_api.startup.session_manager import create_fl_session @@ -85,6 +85,50 @@ def test_create_fl_session_raises_unreachable_when_flag_off(fake_settings): create_fl_session() +def test_create_fl_session_tolerates_internal_error_when_flag_on(fake_settings): + """✅ PER_JOB_FL_SERVER on: login-failed InternalError (server up, not ready) tolerated.""" + fake_settings.PER_JOB_FL_SERVER = True + mock_session = MagicMock() + mock_session.upload_dir = "/tmp/upload" + mock_session.download_dir = "/tmp/download" + mock_session.try_connect.side_effect = InternalError("login failed: ERROR_RUNTIME") + + with ( + patch("fl_api.startup.session_manager.get_settings", return_value=fake_settings), + patch("fl_api.startup.session_manager.FLIP_Session", return_value=mock_session), + ): + session = create_fl_session() + + assert session == mock_session + + +def test_create_fl_session_raises_internal_error_when_flag_off(fake_settings): + """✅ PER_JOB_FL_SERVER off: login-failed InternalError at boot stays fatal.""" + mock_session = MagicMock() + mock_session.try_connect.side_effect = InternalError("login failed: ERROR_RUNTIME") + + with ( + patch("fl_api.startup.session_manager.get_settings", return_value=fake_settings), + patch("fl_api.startup.session_manager.FLIP_Session", return_value=mock_session), + ): + with pytest.raises(InternalError): + create_fl_session() + + +def test_create_fl_session_raises_on_cannot_authenticate_even_when_flag_on(fake_settings): + """✅ PER_JOB_FL_SERVER on: NoConnection("cannot authenticate") is identity, always fatal.""" + fake_settings.PER_JOB_FL_SERVER = True + mock_session = MagicMock() + mock_session.try_connect.side_effect = NoConnection("cannot authenticate to server") + + with ( + patch("fl_api.startup.session_manager.get_settings", return_value=fake_settings), + patch("fl_api.startup.session_manager.FLIP_Session", return_value=mock_session), + ): + with pytest.raises(NoConnection): + create_fl_session() + + def test_create_fl_session_raises_on_identity_mismatch_even_when_flag_on(fake_settings): """✅ PER_JOB_FL_SERVER on: identity mismatch is a misconfiguration, always fatal.""" fake_settings.PER_JOB_FL_SERVER = True diff --git a/fl-services/nvflare/fl-api-base/tests/test_config.py b/fl-services/nvflare/fl-api-base/tests/test_config.py new file mode 100644 index 000000000..52c8a5e47 --- /dev/null +++ b/fl-services/nvflare/fl-api-base/tests/test_config.py @@ -0,0 +1,30 @@ +# Copyright (c) 2026 Guy's and St Thomas' NHS Foundation Trust & King's College London +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# http://www.apache.org/licenses/LICENSE-2.0 +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +from fl_api.config import Settings + + +def test_coerce_empty_per_job_fl_server_truthy(): + """PER_JOB_FL_SERVER accepts the same truthy spellings in fl-api-base and flip-api.""" + for value in ("true", "1", "yes", "on", "True", "YES"): + assert Settings.coerce_empty_per_job_fl_server(value) is True, value + + +def test_coerce_empty_per_job_fl_server_falsy(): + """Empty/commented .env lines and every falsy spelling mean False.""" + for value in ("", "false", "0", "no", "off", "FALSE", None): + assert Settings.coerce_empty_per_job_fl_server(value) is False, value + + +def test_coerce_empty_per_job_fl_server_passthrough_bool(): + assert Settings.coerce_empty_per_job_fl_server(True) is True + assert Settings.coerce_empty_per_job_fl_server(False) is False diff --git a/fl-services/nvflare/fl-api-base/tests/utils/test_flip_session.py b/fl-services/nvflare/fl-api-base/tests/utils/test_flip_session.py index 01439c461..9e482fc2b 100644 --- a/fl-services/nvflare/fl-api-base/tests/utils/test_flip_session.py +++ b/fl-services/nvflare/fl-api-base/tests/utils/test_flip_session.py @@ -126,6 +126,27 @@ def test_do_command_retries_once_on_no_connection(session): assert result == "ok" +def test_try_connect_sets_connected(session): + """✅ try_connect success flips _connected True; is_connected reflects it.""" + assert session.is_connected is True # fixture starts connected + session._connected = False + with patch("nvflare.fuel.flare_api.flare_api.Session.try_connect", return_value=None): + session.try_connect(timeout=5.0) + assert session.is_connected is True + + +def test_try_connect_leaves_disconnected_on_failure(session): + """✅ try_connect failure leaves _connected False ("currently connected", not "ever").""" + session._connected = True + with patch( + "nvflare.fuel.flare_api.flare_api.Session.try_connect", + side_effect=NoConnection("cannot connect to server"), + ): + with pytest.raises(NoConnection): + session.try_connect(timeout=5.0) + assert session.is_connected is False + + def test_check_server_status_reports_stopped_when_unreachable(session): with patch.object(session, "get_system_info", side_effect=NoConnection("cannot connect to server")): out = session.check_server_status()