feat(fl): per-job fl-server — flip-api scales fl-server to zero between training jobs (NVFLARE)

[garciadias]

Description

Phase 0 of the per-job fl-server scale-to-zero effort (#735): decouple flip-api and fl-api-net-1 from the assumption that fl-server is always reachable, ahead of flip-api actually scaling it to zero between jobs.

Today, three things assume fl-server is always up:

• fl-api-net-1 opens its NVFLARE admin session once at boot and raises fatally if the server is unreachable — a dead container under the always-off entrypoint (Follow-ups from Trust_K8s stag deployment: fl-api zombie hardening, automated K8s kit provisioning, egress config #593).
• /check_server_status errors instead of reporting a status when the server can't be reached.
• keep_fl_api_session_alive pings every net every 2 minutes regardless of whether a job is running.

Once fl-server is scaled to zero between jobs (~77% of the time at measured utilization), an unreachable server stops being an outage and becomes the normal idle state — none of the above is safe to ship until the services can tell the two apart.

This PR adds a PER_JOB_FL_SERVER flag (default false), mirrored into both flip-api and fl-api-net-1's environment from a single Terraform variable so both services agree on which state they're in:

• Flag off (default, unchanged everywhere): an unreachable fl-server still crash-loops fl-api-net-1 exactly as today — that crash-loop remains the alerting signal for a real outage.
• Flag on: fl-api-net-1 tolerates an unreachable server at boot and connects lazily on first use (tracked via a new _connected flag on FLIP_Session rather than inspecting nvflare's internal state), /check_server_status reports STOPPED instead of erroring, and keep_fl_api_session_alive skips any net whose scheduler isn't BUSY.

Later phases (infra/IAM groundwork, the actual scale driver and scheduler states, observability, rollout) will follow as separate PRs against #735.

Linked Issues

Part of #735 (this PR does not close it — later phases still to come).

Checklist

• Follows the project's coding conventions and style guide
• Updates documentation
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published

Type of Change

• Non-breaking change (fix or new feature that would not break existing functionality).
• New tests added to cover the changes.
• In-line docstrings updated.
• Documentation updated, tested make -C docs/ docs.

Testing

I did the following tests to verify my changes:

• uv run pytest in fl-services/nvflare/fl-api-base — 143 passed.
• uv run pytest tests/unit in flip-api — 1195 passed, 7 skipped.
• uv run ruff check . and uv run mypy . clean in both services.
• terraform validate clean in deploy/providers/AWS (only pre-existing, unrelated deprecation warnings).

Additional Notes

Env var reference: PER_JOB_FL_SERVER (bool, default false) — documented in .env.development.example, threaded through deploy/providers/AWS/variables.tf → locals.tf (ecs_task_env.flip_api / ecs_task_env.fl_api) → both compose.production*.yml and compose.development*.yml.

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
flip-api/src/flip_api/config.py	66.66%	3 Missing ⚠️

📢 Thoughts on this report? Let us know!

[atriaybagur]

Thanks for this — and genuinely, the analysis in #735 is the best-scoped piece of design work I've read in this repo. C1–C12 anticipate almost everything that bites, the utilization evidence is honest about its own sample size, and "scale the Service, don't switch to RunTask" is exactly right. The flag discipline here is good too: every gate is pinned in both directions, and test_create_fl_session_still_raises_on_non_connectivity_error_when_flag_on is pinning the boundary that actually matters. The api.closed rationale comment is the best kind of comment — I verified it against nvflare 2.8.0 (AdminAPI.__init__ sets closed = False, only close() ever sets it True) and it's exactly right.

I'd like to suggest we move this back to draft and restructure the phasing rather than land Phase 0 on its own. My reasoning is about the seam, not the code quality.

The Phase 0 seam makes this branch unfalsifiable

Every blocking issue I found is a "what happens when the server comes back" bug — and none of them is reachable without a scale driver to bring it back:

1. The lazy reconnect can't re-authenticate. AdminAPI.connect() opens with if self.cell: return (nvflare 2.8.0, fuel/hci/client/api.py:386). On a tolerated boot failure the cell is assigned (:408) and started (:424) before authentication raises at :445, so set_add_auth_headers_filters (:453) never runs. Every later try_connect short-circuits and falls through to api.login() on an unauthenticated cell. flip_session.py:65-67 would need self._reconnect() (fresh Session.__init__) rather than try_connect().

2. _connected is a one-way latch. _do_command handles InternalError and SessionClosed; a server that goes away raises NoConnection, which propagates with _connected still True. After a scale-to-zero → scale-up cycle the next command reuses the previous task's token. The property docstring says "has ever connected" but line 65 needs "is currently connected".

3. InternalError escapes the tolerated tuple. AdminAPI.login() swallows everything into ERROR_RUNTIME (api.py:585-589) → try_connect raises InternalError (flare_api.py:221), which isn't in (NoConnection, FLCommunicationError). That's precisely the mid-cold-start window this design lives in — so fl-api can still crash-loop with the flag on.

4. The tolerated set is wider than the docstring promises. try_connect remaps "cannot authenticate to server" to NoConnection (flare_api.py:207), and authenticator.py:259/286 raises bare FLCommunicationError for a rejected registration and for a server-identity mismatch. So a wrong admin kit or wrong server is currently swallowed too. (The test pins AuthenticationError, which is the one auth path that does still raise — it's only reachable after cell auth already succeeded.)

The tests can't catch any of these, through no fault of the tests: all 17 mock try_connect or Session._do_command, because with no scale driver there's nothing real to integrate against. I ran a quick mutation check and deleting self._connected = False from __init__ leaves all 143 tests passing while production would AttributeError on every FL command.

That last one matters for the merge specifically: #1032 has since deleted that __init__ override entirely and renamed _do_command(cmd) → (command, *args, **kwargs). _connected has no class-level default, so resolving the conflict toward develop reproduces that mutant exactly — silently, with a green suite.

The intermediate state is riskier than either endpoint

Turning PER_JOB_FL_SERVER=true today suppresses the outage alerting and skips the keep-alive pings while nothing scales anything — ecs:UpdateService appears nowhere in the tree yet, though variables.tf:200-202 describes it in the present tense and .env.development.example:226 invites enabling it. Two smaller versions of the same shape: fl-api-net-2 never receives the var (compose.development.nvflare.yml:58-68), so the standard 2-net dev stack would crash-loop net-2; and the two services parse the flag differently (yes/on → True in fl-api-base via stock pydantic, False in flip-api via coerce_empty_per_job_fl_server), which is the exact divergence the comments say must not happen.

To be fair to that validator — it isn't gratuitous. Root Makefile:40 does export $(shell sed 's/=.*//' $(MAIN_ENV_FILE)), which strips values from commented lines too, so this PR's own # PER_JOB_FL_SERVER=false exports an empty string. The guard is right; it just needs to be on both twins.

Suggested restructure

Not one giant PR — #735's C1–C12 is genuinely too much for a single review. I'd cut by independently verifiable behaviour instead of by layer:

• Standalone bug fixes, valuable regardless of scale-to-zero and testable today: C2's requeue (a transient dispatch failure currently DELETEs the job with no recovery path), C7's abort path treating "server down" as "nothing to abort", and GET /fl/status not 500ing wholesale when one net is unreachable.
• Then tolerate + scale driver + readiness gate + IAM + Terraform as one PR, since those are mutually unverifiable in isolation.

Practically there's very little sunk cost in doing this now: the branch is 854 commits behind and 15 of its 20 files have moved, and flip_session.py — where four of the blockers live — has to be rewritten on top of #1032 regardless. The green checks on this PR are from 20 July, so they don't reflect current develop either.

Happy to talk through the re-cut, and glad to review the standalone fixes quickly as they come — those should be easy merges.

[garciadias]

Moving this to draft, and agreeing with the restructure rather than pushing fixes onto it.

I went back over the review's claims before deciding, and they all hold at the current head:

• The merge hazard is the important one. git merge-tree confirms a conflict in flip_session.py, and develop's version (post-fl-api-base show_errors/show_stats/reset_errors 500: FLIP_Session._do_command narrows the base signature #1032) has no __init__ override at all — its docstring states that deliberately, which is exactly what this branch adds. _connected has no class-level default there, so resolving that conflict toward develop yields an AttributeError on every FL command with a green suite. There are further conflicts in test_flip_session.py, locals.tf and test_fl_scheduler_service.py. The branch is 856 commits behind and GitHub reports CONFLICTING / DIRTY.
• Blocker 1 checks out against the nvflare 2.8.0 source. In nvflare/fuel/hci/client/api.py, connect() short-circuits at line 387 (if self.cell: return), the cell is assigned at 408 and started at 424, authentication raises at 445, and set_add_auth_headers_filters is at 453 — after the raise. So a tolerated boot failure leaves a started-but-unauthenticated cell that every later try_connect skips over, as described.
• fl-api-net-2 never receives the flag. compose.development.nvflare.yml sets PER_JOB_FL_SERVER on fl-api-net-1 only, so the standard two-net dev stack would crash-loop net-2.
• The two services parse the flag differently. flip-api has coerce_empty_per_job_fl_server, accepting only ("true", "1"); fl-api-base declares a bare PER_JOB_FL_SERVER: bool = False on stock pydantic, which also accepts yes/on/t.

Worth noting the green checks on this PR are from 20 July, so they say nothing about current develop.

I deliberately have not patched the two small items (the net-2 env var and the flag parity) or the flip_session.py blockers. Since flip_session.py has to be rewritten on top of #1032 either way, in-place fixes would be throwaway, and pushing commits to a PR that is being restructured only muddies the review signal.

Plan, following the split suggested in the review:

1. Standalone bug fixes, first and independently mergeable — the fl-api-net-2 env var and the PER_JOB_FL_SERVER parsing divergence. Neither depends on the per-job work and both are live defects today.
2. The per-job feature as one PR on top of current develop — tolerate + scale driver + readiness gate + IAM + Terraform together, with flip_session.py written against fl-api-base show_errors/show_stats/reset_errors 500: FLIP_Session._do_command narrows the base signature #1032's no-__init__ shape rather than merged into it.

I will keep this branch as the reference while (1) and (2) are cut, and close it once (2) supersedes it.