fix: surface FL runs that fail after submission (#1001) - #1003
Draft
atriaybagur wants to merge 4 commits into
Draft
fix: surface FL runs that fail after submission (#1001)#1003atriaybagur wants to merge 4 commits into
atriaybagur wants to merge 4 commits into
Conversation
A Flower run whose ServerApp dies is indistinguishable from one that is
merely slow: fl-api submits, records the run id and never asks about it
again, so the model sits at INITIATED forever and the cause is visible
only via `flwr log` inside the fl-api container.
Poll from the hub rather than in fl-api. The hub already holds the job id
and the net the job is pinned to, already owns model status transitions
(and the net release that rides on them), and needs no new credentials —
fl-api-flower holds no INTERNAL_SERVICE_KEY in any environment today, and
an in-process poller there would also lose its state on restart.
flip-api gains a once-a-minute sweep (reconcile_failed_jobs.py) over
in-flight FL jobs. Only a backend-reported FAILED acts: it writes the tail
of the run log to the model's activity feed with success=false and moves
the model to ERROR, which completes the job and frees the net. FINISHED is
left alone — a finished ServerApp is routinely still uploading results.
The model's status is re-read after the (network) status call so a result
landing mid-poll wins, and every job is best-effort so one unreachable net
cannot stop the sweep.
fl-api-flower gains GET /run_logs/{run_id}, running
`flwr log <id> local --show` and returning a bounded tail — the head of a
Flower run log is the per-run dependency install, the cause is at the far
end. Credential-shaped substrings are masked first: a run log is whatever
researcher-supplied ServerApp code printed, in a container that holds a
hub service key.
Covers the ServerApp only; a ClientApp dying at a trust logs to that
trust's SuperNode, which the hub cannot read.
Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
5 tasks
|
✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description. |
|
✅ Acceptance criteria have been automatically imported from the linked issue(s) and added to the PR description. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…run fallback, readable feed (#1001) Review hardening of the failed-run sweep: - Add UNKNOWN as the shared job-metadata contract's sixth value (#490). An unmapped native status (i.e. a framework upgrade added one) now surfaces as UNKNOWN instead of being guessed into FAILED — which the sweep acts on destructively (model to ERROR, net freed). UNKNOWN is a no-op everywhere: the sweep skips it, the abort path already filters to RUNNING, and _find_terminal_run no longer treats an uninterpretable status as terminal. Deploy the hub before the fl-api images: an old hub rejects UNKNOWN (only reachable after a framework upgrade adds a status, as both maps are exhaustive today — now pinned by tests against flwr and nvflare). - Resolve runs the backend no longer lists: the SuperLink keeps run state in memory, so a restart forgets every run — the same silent death, with the evidence gone too. Past FL_JOB_UNLISTED_GRACE_MINUTES (default 30) the model is errored with an honest 'backend restarted, run and log lost' feed row. - Branch the manual-fallback wording by the net's backend: an NVFLARE failure no longer advises running flwr log. - Make the feed-row + status write atomic (add_log transaction=session), so a failed status write cannot leave an orphan row for the next tick to duplicate. - Integration coverage: the FLJob→FLScheduler→FLNets→Model join (a wrong join is a silent no-op sweep) and the job-COMPLETED/net-AVAILABLE resolution now run against the throwaway Postgres. - Render failure rows with whitespace-pre-wrap in Timeline.vue so the stored traceback keeps its line structure — the difference between meeting the 'readable without shelling into a container' criterion and storing bytes. - Document SCHEDULER_FL_JOB_RECONCILE_RATE / FL_JOB_UNLISTED_GRACE_MINUTES in .env.development.example and the fl-nodes docs. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
4 tasks
The Makefile exports env-file keys with sed 's/=.*//', which strips the value from every line including commented-out ones, so merely documenting SCHEDULER_FL_JOB_RECONCILE_RATE / FL_JOB_UNLISTED_GRACE_MINUTES in .env.development.example delivers them to Settings as empty strings — which pydantic rejects against int, taking down the app at import (exactly the CI failure mode pinned by test_scan_int_settings_empty_string_falls_back_to_default). Add both fields to the established empty-string coercer (renamed coerce_empty_interval_int — it is no longer scan-only) and extend the pin test. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
6 tasks
atriaybagur
added a commit
that referenced
this pull request
Aug 19, 2026
…#1006) Both Flower templates' main() bodies now run inside a relay shell: any exception escaping the run is reported to the hub — the full traceback via send_handled_exception (server-side, no trust boundary crossed) and the model settled to ERROR — before being re-raised so Flower still records finished:failed and the #1003 failed-job sweep stays consistent as the backstop. Each relay step is guarded separately so an unreachable hub or a non-UUID tutorial model id cannot mask the original failure. The researcher-supplied models.py import moves from module scope into the guarded body: a broken models.py — the most likely researcher error — now reports its traceback to the activity feed instead of killing the ServerApp before it can say anything (the FLIP#1001 canonical case, previously visible only to the hub's log-tail poll). The relay helper is deliberately duplicated per template rather than added to flip.flower: templates ship in the flip-api image while flip-utils ships in the FL images, so a new flip.flower symbol would die with ImportError at module scope on any FL image older than the template — the exact failure mode this change exists to end. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
This was referenced Aug 19, 2026
The sweep's feed row opened with the FL run log tail, which buries the reason it exists to surface: on a real failed Flower run the stored row is 5,278 bytes, of which ~3,400 is the single `Installed: [...]` line from the per-run `uv sync`, and the rendered element is ~2,300px tall — the traceback sits well below the fold of a fixed-height activity panel. `flwr ls` already reports a one-line cause per run, in the very response the sweep reads for the status. The shared #490 job-metadata contract therefore gains an optional `status_details`, and the row leads with `Reported cause: …` before the tail. No extra request, no subprocess, no new endpoint; the tail still follows, since it carries the file and line number the one-liner lacks. fl-api-flower populates it, normalising flwr's literal "N/A" sentinel to absent, collapsing whitespace, applying the same redact_secrets masking as the run log and bounding it at 500 chars — the text is a researcher-authored exception message from a container holding a hub service key. fl-api-base declares the field but always leaves it None: NVFLARE's JobMetaKey has no error/details key (job_deploy_detail reads 'server: OK' even on a job that then died with FINISHED:EXECUTION_EXCEPTION). Optional with a default, so unlike the UNKNOWN status value this half of the contract is deploy-order-safe in both directions. Also corrects the NVFLARE manual-fallback hint, which named two destinations that do not work: `POST /<job-id>/show_errors/server` returns 500 against the installed NVFLARE (filed as #1032), and the job workspace holds opaque blobs rather than a readable log — a filesystem-wide search of a failed run's fl-server matched only the uploaded app bundle. The hint now names the fl-server container's output, which is where the traceback actually lives. Verified live on the dev stack, both backends. Signed-off-by: at24_bioeng625-pc <alexandre.triay_bagur@kcl.ac.uk>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1001.
The failure mode
fl-apisubmits a job, gets arun-id, writes it tofl_job.fl_backend_job_idand never asks about it again. Everything the hub knows about a run in progress is reported by the run, through theflippackage. A run that dies before it can report anything — the canonical case being an exception at module scope inserver_app.py, which kills the ServerApp the instant it starts — therefore reports nothing at all:INITIATEDindefinitely,fl_logsholds only the queue position,Started task/Finished taskand nothing else,flwr log <run-id> localinside thefl-apicontainer.Where the polling lives, and why
In the hub (flip-api), as a new scheduled sweep —
flip_api/fl_services/reconcile_failed_jobs.py, registered on the existingBackgroundScheduleratSCHEDULER_FL_JOB_RECONCILE_RATE(default 1 min).The issue left this open between a background task in
fl-apiand the hub's FL scheduler. Reasons for the hub:fl_logsand moving the model toERROR. Fromfl-apithat is an authenticated call toflip-api, andfl-api-flowerholds noINTERNAL_SERVICE_KEY/FLIP_API_INTERNAL_URLin any environment today (dev compose, prod compose, ECS task definitions). Onlyfl-serverdoes. Adding them would put a hub service key in a second container to fix a reporting gap.update_model_status(ERROR)is the same call the prepare-failure path uses, and its existing terminal-status branch completes theFLJoband releases the net. Doing that fromfl-apiwould mean duplicating or re-implementing that sequencing across an HTTP boundary.fl-apidies with its container and forgets the run it was watching.extract_current_job_datacalls the net's/list_jobsduring aborts; the shared job-metadata contract ([Bug]: Converge the FL-backend job-metadata contract across flip-api, fl-api-flower, fl-api-base #490) already normalises both backends' statuses.What
fl-api-flowergains is the one thing only it can do:GET /run_logs/{run_id}, which runsflwr log <id> local --showand returns a bounded, secret-masked tail.The hub's lookup (
get_backend_job_metadata) returns the whole/list_jobscontract item rather than just the status, so it also picks upstatus_details— see below — without a second call.How it behaves
FLJob.status == IN_PROGRESS, a non-null backend job id, and a model still inINITIATED/PREPARED/RUNNING. Anything already settled is not even polled — which also makes the sweep idempotent, since theERRORit writes takes the model out of that set.FAILED;FL_JOB_UNLISTED_GRACE_MINUTES(default 30). The SuperLink keeps run state in memory (no--databaseis passed anywhere in-tree), so a SuperLink restart forgets every run — the same silent death, with the evidence gone too. The feed row says so honestly ("backend restarted, run and log lost"); no log fetch is attempted, since the backend that forgot the run forgot its log with it. The grace period keeps a transient listing hiccup from erroring a healthy run.FINISHEDis deliberately ignored: a run whose ServerApp has finished is routinely still uploading results, and the hub's ownRESULTS_UPLOADEDcallback is the authority there.PENDING/RUNNING/STOPPEDare no-ops, and so isUNKNOWN(below).ERRORwrite commit atomically (add_log(..., transaction=session)defers toupdate_model_status's commit), so a failed status write can never leave an orphan feed row for the next tick to duplicate.The
UNKNOWNcontract value. This PR gives the destructive meaning ofFAILEDa second look: previously both adapters'normalize_statusdefaulted unmapped native statuses toFAILED— written for the abort path, where "assume failed" was conservative. The sweep makes that default destructive (model toERROR, net freed while the real run may still be executing). The shared #490 contract therefore gains a sixth value: an unmapped native status — i.e. one added by a framework upgrade — now surfaces asUNKNOWN, which is a no-op everywhere: the sweep skips it, the abort path already filters toRUNNING, and_find_terminal_runno longer treats an uninterpretable status as proof a run is terminal (an abort that can't be confirmed now 500s loudly instead of no-op'ing). Both maps are pinned exhaustive by tests against the installed frameworks (flwr.common.constant.Status/SubStatus,nvflare.apis.job_def.RunStatus), soUNKNOWNis unreachable until a framework upgrade — but deploy the hub before the FL API images all the same, since an old hub'sIJobMetaDatawould reject it.Log handling. The tail, not the head: a Flower run log opens with the per-run
uv syncover the app's whole dependency set and the traceback is at the far end.fl-api-flowertruncates toFLOWER_RUN_LOG_MAX_CHARS(default 8000), advancing the cut to a line boundary and flaggingtruncated; flip-api caps again at 8000 as belt-and-braces. If the log can't be retrieved the feed row names the manual fallback for the net's backend:flwr log … local --showon a Flower net,docker logs fl-server-net-<n>on an NVFLARE net (see The NVFLARE fallback below — the two more obvious destinations don't work).Leading with the cause (
status_details). The log tail alone buries the reason. Measured on a realfailed run: the stored row is 5,278 bytes, of which ~3,400 is the single
Installed: [...]line from theper-run
uv sync, and the rendered<p>is ~2,300px tall — the traceback sits far below the fold of afixed-height feed panel. But
flwr lsalready reports a one-line cause per run, in the very response thesweep reads for the status:
So the shared #490 contract gains an optional
status_details, and the feed row leads withReported cause: …before the tail. This costs no extra call, nosubprocess, no new endpoint. The tailstill follows, because it carries the file and line number that the one-liner does not.
fl-api-flowerfills it fromstatus-details, normalising flwr's literal"N/A"(written for a run withnothing to say) to absent, collapsing whitespace, applying the same
redact_secretsmasking as the run log,and bounding it at 500 chars — the text is a researcher-authored exception message from a container holding
a hub service key. NVFLARE leaves it
None:nvflare.apis.job_def.JobMetaKeyhas no error/details keyat all (the nearest,
job_deploy_detail, reads['server: OK', 'Trust_2: OK']even on a job that then diedwith
FINISHED:EXECUTION_EXCEPTION). UnlikeUNKNOWN, this half of the contract is deploy-order-safe inboth directions: the field is optional with a default, and
IJobMetaDataalready ignores extra fields.The NVFLARE fallback. Its wording is now
docker logs fl-server-net-<n>, because the two destinationsthe original hint named are both dead ends — verified live, not assumed.
POST /<job-id>/show_errors/serverreturns HTTP 500 against the installed NVFLARE (
FLIP_Session._do_command() got an unexpected keyword argument 'enforce_meta'), filed as #1032. And the job workspace holds opaquedata/meta/workspaceblobs, not a readable log: a filesystem-widegrep -rlfor the failing exception string acrossa failed run's
fl-servermatched only the uploaded app bundle, never the traceback. The container's stdoutis where it actually lives.
Rendering.
Timeline.vuenow renders failure rows withwhitespace-pre-wrap(success rows keep the default), so the stored traceback keeps its line structure in the activity feed — the difference between meeting the "readable without shelling into a container" criterion and technically storing the bytes.Secrets. The issue's brief mentioned an existing
scrub_argshelper infl_api/app.py— there is no such helper anywhere in the repo (grep -rn scrub_argsis empty), so I wrote one:fl_api/utils/redaction.py. A run log is whatever researcher-supplied ServerApp code wrote to stdout/stderr, in a container that holds a hub service key, so it is not trusted to be secret-free. It masks SigV4 query parameters on presigned URLs (individually, so the object path stays readable),…key|token|secret|password|credentialfollowed by:/=, and bareAKIA/ASIAaccess key ids. Redaction runs before truncation, so a cut landing inside akey=<secret>pair can't strip the keyword the matcher needs. It is biased towards over-redaction and is damage limitation, not a guarantee — a secret printed with no recognisable keyword still gets through.What is and isn't covered
Covered: the ServerApp. A run that fails at any point after a successful submit — including a backend restart that loses the run — on either backend (see below).
Not covered: a ClientApp that dies at a trust. Its logs live on that trust's SuperNode, which the hub cannot read — out of scope per the issue, worth a follow-up.
Both backends, deliberately — but the exposure differs. The sweep talks only to the shared
/list_jobscontract, so it runs for NVFLARE nets too. On Flower the gap is wide open: the templateserver_app.pyhas no top-level exception reporting, so most failures say nothing. On NVFLARE the gap is narrower —flip_server_event_handleralready reportsFATAL_SYSTEM_ERRORandEND_RUNterminal statuses — and what the sweep adds there is the jobs that never reachEND_RUN(FINISHED:CAN_NOT_SCHEDULE,FAILED_TO_RUN,ABANDONED, a server crash, a failure before the handler is built). The remaining asymmetry is diagnostic, not behavioural. The sweep itself never branches on backend — same poll, sameFAILED/unlisted conditions, sameERROR, same net release. What differs is how much a row can say:/run_logsexists only onfl-api-flower, andstatus_detailsonly Flower populates.fetch_run_logsdegrades toNoneon any failure (including the 404 an NVFLARE net returns), so an NVFLARE failure surfaces status-only, and picks up logs for free iffl-api-baseever grows the endpoint. Both gaps are properties of what each framework exposes, not of this design.Follow-ups (separate PRs/issues, deliberately not here):
@app.main()in try/except →send_handled_exception+ERROR— the in-run half of this fix, higher-fidelity for in-main()failures, and its own review.--database) across dev/secure compose and the ECS task defs — an operational change; the unlisted-grace fallback here removes the urgency.show_errors500s onfl-api-base(NVFLARE API drift). Pre-existing; this PR routes around it rather than depending on it.[92mINFO [0m:junk in the feed. Found by eyeballing a real row in the browser (below). Deliberately not fixed here to keep this diff to one concern.Verified — unit / static
make -C fl-services/flower/fl-api-flower local_test— ruff + mypy clean, 139 passed, 0 failed (test_run_logs.py,test_redaction.py, the flwr status-map exhaustiveness pin, and 4 newstatus_detailscases: the happy path, flwr's"N/A"sentinel, collapse+redact+bound, and an absent key on an older flwr).make -C fl-services/nvflare/fl-api-base local_test— ruff + mypy clean, 166 passed, 0 failed.flip-api:ruff check .clean,mypy .clean (364 files),pytest tests/unit— 1559 passed, 0 failed;pytest tests/integration— 127 passed, 0 failed, of which 3 are new (test_fl_reconcile_db_flow.py: the in-flight join, the settled-job filter, and the unlisted-past-grace resolution, all against the throwaway Testcontainers Postgres — the join is exactly the thing a mocked session can't catch going silently wrong).IJobMetaDataplus both adapters'JobMetadata— so a field added on one side only fails a test rather than going quietly missing.flip-ui:npm run lint(0 errors; 3 pre-existing max-len warnings inEditProjectDrawer.vue, untouched here),npm run test:unit— 1250 passed (Timeline pre-wrap spec added test-first).scripts/check_fl_api_validation_sync.shstill passes.detect-secrets scan --baseline .secrets.baselineover the new files: no new findings.flwr1.32 source read to confirm the mechanics behind--show:flwr/cli/log.py::print_logsbreaks after the first streamed message, and the SuperLink'sStreamLogsservicer yields the whole accumulated log sinceafter_timestamp=0in a single message and ends the stream once the run isFINISHED(which includesfinished:failed). Also confirmed the SuperLink's--databasedefault is the in-memory sentinel (flwr/server/app.py), which is what makes the unlisted-run fallback necessary.Verified — live on the dev stack
The four gaps the earlier revision of this description listed as unverified have now been driven on a real stack (hub on this branch, both FL backends in turn, project reused so DICOM was pulled once). The fault injected on both backends is the canonical #1001 shape: a module-scope
raise ImportErrorin the researcher's ownmodels.py.Flower — end to end. Run
8621698533979188955reachedfinished:failed; the sweep resolved it within a tick — modelINITIATED→ERROR,FLJob→COMPLETED, scheduler back toAVAILABLE.GET /run_logstalked to a real SuperLink for the first time and returned the genuine traceback, ending:NVFLARE — end to end, and it earns its keep. Job
55c8e4d8-…reachedFINISHED:EXECUTION_EXCEPTION→FAILED; the sweep errored the model and freed the net in ~60s. Worth noting why nothing else reported it: the server config buildspersistorfrommodels.get_modelatcomponents.#1, andflip_server_event_handleriscomponents.#5— so the job died at component construction before the reporting handler existed. This is precisely the class of NVFLARE failure the sweep exists for.fetch_run_logsdegraded exactly as designed against the live NVFLARE FL API (404 →None→ status-only row plus the backend hint).A healthy run is untouched — checked, not assumed. An unmodified NVFLARE run went
INITIATED→PREPARED→RUNNINGwith the sweep polling it across several ticks and taking no action. When it later failed for an unrelated reason (a trust-side data-mount mismatch in my test environment), NVFLARE's own client reporter handled it — no sweep row, no duplicate, no interference.status_details, live. Confirmed on the rebuiltflower-fl-api:devagainst a real SuperLink: a running run reportsstatus-details: "N/A"(normalised to absent, so noReported cause: N/Ain the feed), and a failed one reports the ServerApp's exception verbatim, which the feed row now leads with.Timeline rendering, in a browser. The live route is blocked by a deliberate
vite.config.mtsguard refusingVITE_E2E=truein development mode, so this ran through the docs harness with the real stored bytes as the fixture.whitespace-pre-wrapapplies to the failure row and not the success row, and the traceback keeps its line structure. It also surfaced two things only a browser shows: the ANSI junk now filed as #1033, and the sheer size of the row (~2,300px, mostly dependency list) — which is what motivatedstatus_detailsabove.Still not verified
RUNNINGjob alone and defers to the run's own reporting, but that particular run ended in an unrelated environment failure rather thanRESULTS_UPLOADED.FLJob.started, not on when the run was first seen missing — so a job older than the window has no protection from a single transient omission. Low practical risk (both adapters raise rather than return a short list when the backend is unreachable, so a 200 without the run really does mean the backend lost it), but the docstring's "keeps a transient listing hiccup from erroring a healthy run" only holds for jobs younger than the grace period. Flagged rather than changed — adding first-unlisted state to make it exact is a bigger call than this PR should take.Also worth knowing
fl-services/flower/fl-api-flower/fl_api/utils/upload.pycarries no Apache 2.0 header, contrary to the repo rule. Untouched here — separate fix.Acceptance Criteria
Imported from issue #1001
model at
INITIATED. The model reaches a terminal state and the failure is visible in the UI.fl_logs(which already carries asuccessboolean), so the cause is readable without shelling into a container.fl-services/flower/fl-api-flower/tests/alongside the existingtest_submit_run.py/test_check_status.py.flwr log <run-id> <superlink>command is documented as the manual fallback.