fix: stop FLIP_Session narrowing its NVFLARE base signature (#1032)

[atriaybagur]

Closes #1032.

The bug

show_errors, show_stats and reset_errors on fl-api-base returned HTTP 500 for every call:

{"detail":"Internal server error: FLIP_Session._do_command() got an unexpected keyword argument 'enforce_meta'"}

Not upstream drift — a narrowed override in our own code. NVFLARE's base declares
_do_command(self, command, enforce_meta=True, props=None); FLIP_Session overrode it as
_do_command(self, cmd). Python dispatches to the subclass, so any upstream call passing those
keywords raises TypeError.

Which routes, exactly. NVFLARE 2.8.0 passes enforce_meta=False from eight sites; four are
reachable from this service:

Route	Reaches _do_command via
POST /<job_id>/show_errors/<target_type>	_collect_info
POST /show_stats/<target_type>/<job_id>	_collect_info
POST /reset_errors	_collect_info
GET /get_working_directory/<target>	_shell_command_on_target

Everything else calls _do_command(command) positionally with defaults, which the narrowed
signature happens to satisfy. That is why list_jobs, abort_job and submit_job were fine, and
why nothing noticed: no code in the platform called the four broken ones.

Found while live-verifying #1003, which had started pointing operators at show_errors as the
NVFLARE manual fallback.

What changed

_do_command forwards instead of narrowing. The override earns its place — the base raises
SessionClosed the instant api.closed and never reconnects, so the retry is genuinely additive —
it just takes *args/**kwargs now. Forwarding rather than restating the base signature means the
next NVFLARE bump that adds a parameter needs no edit here.

The __init__ override is deleted. An audit of the rest of the subclass found the same defect
there. The base already stores username, startup_path, secure_mode, _debug and _study; the
override re-stashed three of them under private aliases while dropping the base's study
parameter and flipping the secure_mode default from True to False. It also set an
_error_buffer that nothing in the repo ever reads.

_reconnect now reads the base attributes and passes _study through. Previously every
reconnect silently reset the session to the default study — quietly changing which jobs subsequent
commands could see, on a code path that only runs after a connection drop.

get_system_info / get_connected_client_list keep their names. They do shadow methods NVFLARE
calls internally (_client_last_connect_times, _wait_for_clients_shutdown,
_wait_for_clients_restart, restart, and its own get_connected_client_list) and return Pydantic
models rather than NVFLARE's types — which works today only because the models happen to expose the
four attributes those callers read. Renaming them is out of scope by agreement, since the FL API
routes serialise these results directly; instead the contract is written down on the methods and
pinned by test, so a future field rename fails in CI rather than inside upstream's restart path.

Tests

Two guards, both verified to fail on the historical code before being kept:

• Signature pin over every FLIP_Session override — no dropped base parameter, no changed
  default. Re-introducing the old _do_command fails the _do_command case; re-introducing the old
  __init__ fails on both the missing study and the flipped secure_mode.
• Real-chain regression for all three commands, faking only session.api so the call travels
  through NVFLARE's own show_errors → _collect_info → _do_command → api.do_command. On the
  old code it reproduces the production error at the exact upstream line (flare_api.py:1262),
  rather than a mock's idea of it.

Plus a pin on the four duck-typed attributes, and one asserting _reconnect preserves the study.

Verified

• make -C fl-services/nvflare/fl-api-base local_test — ruff + mypy clean, 187 passed, 0 failed
  (was 166; 21 new).
• Both historical defects re-introduced one at a time to confirm the new tests actually catch them.

Verified live

Dev stack switched to the NVFLARE backend with flare-fl-api:dev rebuilt from this branch
(job 5d27b9bd-… on net-2, while running):

HTTP 200 show_errors/server -> {"server":{"ScatterAndGather":{"error":"... peer=Trust_2,
                                peer_rc=TASK_ABORTED, task_name=train ..."}}}
HTTP 200 show_errors/client -> {"Trust_1":{},"Trust_2":{}}
HTTP 200 show_stats/server  -> {"server":{}}

All three were a 500 before. reset_errors was not caught live (the job finished first) but routes
through the identical _collect_info path as show_stats and is covered by the chain test.

An upstream constraint found while verifying

show_errors only works on a running job. Called against one that had already failed:

{"detail":"Internal server error: job c4fe73dd-… is not running"}

That is upstream by design — nvflare/private/fed/server/info_coll_cmd.py gates on
job_id in engine.run_processes and returns JOB_NOT_RUNNING otherwise, because the InfoCollector
lives inside the job's own process and dies with it.

So this route was never going to be the post-mortem fallback for a failed run, fixed signature or
not. PR #1003 has already repointed its operator hint at the fl-server container output; this
confirms that was correct rather than a workaround.

Separately: the FL API surfaces these semantic refusals as HTTP 500 Internal server error:
when a not-running job is really a 409 (and an unknown job a 404). Left alone here — it is a
different concern from the signature bug and deserves its own change.

Out of scope

Renaming get_system_info / get_connected_client_list so they stop shadowing base methods
altogether. It is the more thorough fix — that shadowing fails silently where this bug failed
loudly — but it touches the FL API routers and deserves its own change.

Review round

A review pass caught six things, two substantive.

The models could not survive the substitution this PR documents. NVFLARE's ServerInfo /
ClientInfo treat start_time / last_connect_time as optional — the admin meta is read with a
bare get, upstream's __str__ prints "unknown" for a missing one, and _wait_for_clients_restart
carries an explicit if previous_time is None: continue. FLIP's models declared both as required
floats, so get_system_info() raised ValidationError precisely where the base returns a usable
object — reachable from four FL API routes and from inside NVFLARE's own restart and client-shutdown
waits. Both fields are now optional with None-safe __str__. Without this, the first commit's
four-attribute "contract" was documentation of something that did not hold.

The blast radius above was wrong in the first commit — "exactly two call sites", three routes.
It is eight sites and four routes; get_working_directory was equally broken and went unmentioned.
The grep behind that claim had been truncated. Corrected in the docstring, the issue and here.

Also fixed: the override still renamed the base's first positional (command → cmd), so
_do_command(command=...) remained a TypeError — **kwargs does not repair a renamed positional,
and the class docstring's own rule forbids it.

Three of the six were defects in the new guards themselves, each re-verified by mutation:

• the signature guard asserted nothing for _do_command — accepts_var_kw short-circuited the
  name check and the default check was gated on presence, so it passed both for a renamed positional
  and for a default flipped behind **kwargs. Both now fail with specific messages.
• _overridden_methods filtered on callable(), which is False for classmethod and property
  objects, so an override of either kind produced no test case at all.
• the __init__ check rejected the forwarding style its own docstring permits, with a raw KeyError.

Plus: the chain test used bare strings where the routers pass a TargetType StrEnum and a
comma-split list; _reconnect now binds the base by keyword rather than five positionals (mirroring
NVFLARE's _new_poll_session); get_connected_client_list delegates instead of duplicating the base
body; and the spy reaches the base by name rather than __mro__[1].

Deliberately not fixed here — both pre-existing, both filed as #1035:

• _reconnect abandons the old AdminAPI without closing it, leaking a connection and threads per
  reconnect (Session._close_ignore_errors() exists upstream for this).
• the session_inactive branch calls try_connect, which itself raises SessionClosed when the API
  is closed — and raised inside an except block it escapes uncaught rather than falling through
  to the sibling handler that would do a full _reconnect.

Neither is a signature problem, and folding connection-lifecycle changes into this diff would blur
what it is for.

Second review round

One comment, on the signature guard itself, and it held up.

A base parameter demoted to keyword-only slipped through. The slot check read name not in positional as automatically acceptable, so an override that kept a parameter's name but moved it
from positional-or-keyword to keyword-only passed — while breaking every positional caller,
which is how NVFLARE calls _do_command from all eight of its sites. Mutating the real method into
(self, *args, command, **kwargs) passes the old guard 3/3 and raises
TypeError: missing 1 required keyword-only argument: 'command' at the first call.

The same expression indexed the wrong list. positional[index] used a base parameter index
against the override's positional list, so an override that dropped a middle parameter and kept a
later one positionally ((self, command, props, **kwargs)) crashed the guard with IndexError
rather than failing with its message — despite being a real narrowing, since a positional caller
binds enforce_meta's value into props.

Positional reachability is now asserted directly: a base parameter of positional kind must not be
keyword-only in the override, and must sit in the base's own slot or fall into the override's
*args (which is what carries enforce_meta/props for the real (self, command, *args, **kwargs)). Name reachability and the default check are unchanged.

The rule moved into _assert_substitutable so it is pinned by test rather than by hand-mutating
FLIP_Session: three substitutable shapes that must pass, five narrowing shapes that must fail
(keyword-only demotion, renamed first positional, dropped middle parameter, no slot for the base's
trailing positionals, flipped default). This guard has now shipped with a hole twice — the mutation
check belongs in CI, not in a reviewer's head.

Test-only; no production behaviour moves.

Third review round

Two non-blocking nits, both on the tests.

The widened fields had no regression test. Every model construction in the suite passed concrete
floats, so nothing proved None is accepted or that the "unknown" __str__ branch works -- reverting
the fields to required floats left the suite green. Three tests now pin it: the models built with no
times (None through, "unknown" printed -- the server case compared against NVFLARE's own
ServerInfo.__str__), a falsy-but-real 0.0 timestamp that must not print as "unknown", and the
real get_system_info() fed NVFLARE's own SystemInfo shape with both fields unset. Verified red by
re-requiring the fields (2 of 3 fail, the 0.0 case passing by design), then green.

The __init__ signature test is a tautology today -- with no override, it resolves to
Session.__init__ itself. Left as the forward guard it was written to be, with the docstring now
saying so; it is not idle meanwhile, since it pins the base's study keyword and secure_mode=True
default that _reconnect forwards by keyword, so an upstream rename fails here before it fails
inside _reconnect.

local_test: ruff + mypy clean, 190 passed.

Acceptance Criteria

Imported from issue #1032

• FLIP_Session._do_command is signature-transparent to its base (*args / **kwargs
  forwarded), so upstream keyword arguments pass through untouched.
• show_errors, show_stats, reset_errors and get_working_directory reach the
  transport — covered by a test driving the real chain down to api.do_command, in the argument
  shapes the routers actually pass (a TargetType StrEnum, and a comma-split targets list).
• The redundant __init__ is removed, restoring the base's study parameter and its
  secure_mode=True default. _reconnect reads the base attributes and now also preserves
  _study, which the old implementation silently reset to the default study on every reconnect.
• A guard test pins every FLIP_Session override against its base: no dropped parameter and
  no changed default. Verified to fail on both historical defects before the fix.
• get_system_info / get_connected_client_list keep their names, with the four attributes
  NVFLARE reads off the returned models (server_info.status, server_info.start_time,
  client_info[].name, client_info[].last_connect_time) documented and pinned by test — and
  start_time / last_connect_time made optional, matching NVFLARE's own types, so the
  substitution no longer raises ValidationError on the None upstream is written to expect.
• Live check done (dev stack switched to NVFLARE, flare-fl-api:dev rebuilt from the fix).
  Against a live net, on a running job: show_errors/server 200 with a real payload,
  show_errors/client 200, show_stats/server 200. Before the fix every one was a 500.

[github-actions]

✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description.

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...s/nvflare/fl-api-base/fl_api/utils/flip_session.py	87.50%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[github-actions]

✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description.

[github-actions]

✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description.

[atriaybagur]

Heads up @garciadias — scope widened since you were requested, sorry for the moving target.

#1035 is now folded in here (was PR #1043, since closed). It is the same class, an adjacent failure, and keeping them stacked meant reading this diff twice.

What it adds: restarting an fl-server left the FL API answering 500 to every request permanently, because _reconnect swaps in a fresh AdminAPI before try_connect can fail — leaving a session that was never logged in, with an empty command registry but closed = False. Every later command then fails client-side with ERROR_SYNTAX: Command list_jobs not found, which no recovery branch catches. Fix restores "unusable implies closed" so the session heals itself.

Verified live on the dev stack, deterministically (stop the server, force a reconnect while it is down, bring it back): wedged 500/500 without the fix, healed 200/200 with it, guard-fired counter 0 → 1.

Also note @ pushed 9759c269 onto this branch while I was working — a third hole closed in the signature guard. It is preserved and included.

Full title and description updated. make -C fl-services/nvflare/fl-api-base local_test: ruff + mypy clean, 195 passed.

[atriaybagur]

@garciadias — converted back to draft, please hold. Apologies for the churn.

A review pass on the reconnect work I folded in an hour ago found three regressions introduced by that change, one of which is serious:

1. Possible duplicate FL job submission. The new except (SessionClosed, NoConnection) retries any command after rebuilding the session. NoConnection is also what a 5s command timeout produces — it says nothing about whether the server acted. So a submit_job that succeeded server-side but timed out client-side gets re-issued: two identical FL jobs, two nets marked BUSY. NVFLARE deliberately restricts its own connection retry to {ABORT_JOB, SHUTDOWN} (_CONNECTION_RETRY_COMMANDS, flare_api.py:85) — both idempotent — and my blanket retry bypasses that guard for every command.

2. The wedge is still reachable. try_connect can raise InternalError (its ERROR_RUNTIME fall-through), which the session_inactive branch does not catch — so it escapes and leaves the api connected-but-not-logged-in: the exact permanent wedge this was meant to fix, via the one try_connect call site I did not harden.

3. A shared-session race made worse. app.state.session is one object and every route is a sync def on FastAPI's threadpool. Previously _reconnect only rebound self.api, so an in-flight command on the old object still completed. Adding the close means one thread can tear down the AdminAPI another thread just built.

Plus eight lower-severity findings (hardcoded 5s ignoring TIMEOUT_SESSION_CONNECT, a silent except Exception: pass on the close, exception-type replacement losing NoConnection for callers that branch on it, and a suggestion that the session_inactive branch may be dead against nvflare 2.8.0).

The #1032 signature work is unaffected and still sound. I'll fix these and re-request review.

[atriaybagur]

Reconnect work removed — this PR is back to the signature fix only.

83419fa9 (the #1035 reconnect changes) has been taken off this branch. It introduced three regressions I found on review — the serious one being a possible duplicate FL job submission, since the blanket retry covered non-idempotent commands where NVFLARE deliberately restricts its own retry to {abort_job, shutdown}.

More fundamentally, that work was the wrong shape. NVFLARE has no mechanism for keeping a long-lived session alive across a server restart — restart() closes its own session and never rebuilds it, and _poll_system_info builds a throwaway Session and discards it. Its model is that sessions are cheap and replaceable. _reconnect re-initialises one in place, which is what leaves a half-built shared object behind. #1035 will be redone as a session provider that replaces rather than repairs; the premise is now verified — three consecutive failed builds while the server was down left no residue, and a fresh build recovered cleanly.

Nothing here is affected: this branch is 9759c269, the three signature commits, unchanged and independently verified live (show_errors / show_stats / get_working_directory returning 200 against a real NVFLARE net).

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[atriaybagur]

Ready for review.

• CI: all checks green (codecov/patch included; the "Ensure PR to main originates from develop" skip is the expected develop-target behaviour).
• Tests: make -C fl-services/nvflare/fl-api-base local_test — ruff + mypy clean, 187 passed (was 166; 21 new). Both historical defects were re-introduced one at a time to confirm the new guards catch them.
• Copilot: 1 review comment (keyword-only hole in the signature guard) — confirmed real, fixed in 9759c26, thread resolved. Fresh review requested.
• Live check: dev stack on NVFLARE with flare-fl-api:dev from this branch — show_errors/show_stats 200 with real payloads (all 500 before).
• Mergeable against develop; none of the recent develop merges touch fl-services/nvflare/fl-api-base.

[garciadias]

Correct, narrowly-scoped, unusually well self-reviewed fix for the FLIP_Session signature-narrowing defect, with a strong Liskov-substitutability test suite independently verified against upstream NVFLARE 2.8.0 source. Two minor, non-blocking nits left inline (a missing None-safety regression test for the two widened Pydantic fields, and a currently-tautological signature test). Approving.