[2.7] Fix hierarchical FL startup failures: deployment timeouts, selective client exclusion, and dead-detection debounce

[chesterxgchen]

Problem

Large-scale hierarchical FL jobs (e.g. BERT NER, 144 clients, 6 relays on Frontier) abort in
Round 0 due to a cascading startup failure chain. The root sequence is:

1. F3 streaming HOL stall (PR [2.7] Mitigate F3 streaming Head-of-Line (HOL) stalls and add guardrails #4206) delays deployment ACKs from relay-connected clients
2. _deploy_job() treats reply=None (timeout) as "unknown" — not a failure — so
   timed-out clients silently appear to have been deployed
3. _start_run() tries to start those clients; they again time out, and
   check_client_replies() ignores the None reply
4. _sync_client_jobs() fires dead-job notification on the very first heartbeat with
   no startup grace period
5. FedAvg requires 144/144 — one or two missing clients → abort
6. A late-starting CJ crashes with TypeError: 'NoneType' object is not iterable when
   get_job_clients() receives None metadata from an already-aborted job

PRs #4206, #4204, #4174, #4172, #4186, #4211, #4210 (all merged in 2.7.2) address the
transport layer. This PR addresses the remaining job lifecycle layer.

---

Fixes Included

1 — _deploy_job(): Treat deployment timeout as failure (job_runner.py)

Root bug: reply=None was logged as "unknown" and excluded from failed_clients,
so timed-out clients counted as "successfully deployed" for the min_sites check.

Fix: Add timed-out clients to failed_clients with a "deployment timeout" label.
The existing min_sites / required_sites logic then correctly decides whether to abort.

2 — check_client_replies(): Return timed-out clients instead of raising (admin.py)

Root bug: In strict mode, any timeout raised immediately, aborting the whole job even
when the remaining active clients satisfied min_sites.

Fix: In strict mode, collect timed-out clients into a return list rather than raising.
Explicit errors (non-OK return code or error body) still raise. Also fixes the non-strict
mode to use name-keyed dict lookup instead of fragile positional zip().

New signature: check_client_replies(...) -> List[str] (timed-out client names; empty = none).

3 — _start_run(): Selective exclusion with min_sites re-evaluation (job_runner.py)

Root bug: A start-job timeout under strict mode aborted the entire job with no
tolerance for stragglers within min_sites bounds.

Fix: Use the returned timed-out list from check_client_replies(). If remaining
active clients >= min_sites, log a warning and proceed. Only abort when below tolerance.

4 — _sync_client_jobs(): Require-prior-report default changed to True (fed_server.py)

Root bug: SYNC_CLIENT_JOBS_REQUIRE_PREVIOUS_REPORT defaulted to False, meaning
the bug fix was opt-in and the unsafe behaviour remained the default.

Fix: Default changed to True. Operators who want the aggressive legacy detection
can set it to False explicitly.

5 — _sync_client_jobs(): Move _reported_clients out of job_info dict (fed_server.py)

Root bug: Positive-observation tracking was stored as job_info["_reported_clients"],
injecting algorithm state into a data dict with no corresponding RunProcessKey constant.

Fix: Tracking moved to self._job_reported_clients: Dict[str, set] on FederatedServer.
Stale entries are purged whenever a job is no longer in run_processes.

6 — ClientRunManager.get_job_clients(): Explicit meta validation (client_run_manager.py)

Raises RuntimeError with a descriptive message instead of an opaque TypeError when
JOB_CLIENTS is absent or the wrong type.

---

Configuration Recommendations (No Code Change Needed)

Setting	Recommended value	Effect
FedAvg(min_clients=...)	96-98% of num_clients	Tolerates a few startup stragglers
runner_sync_timeout	120 s	Allows Lustre-backed deployments time to complete
strict_start_job_reply_check	true	Start-job timeouts surfaced, straggler clients excluded
sync_client_jobs_require_previous_report	true (now the default)	Prevents premature dead-job from startup delay
SFM_CLOSE_STALLED_CONNECTION (PR #4206)	true after staging	Disconnects stalled relay connections

---

Files Changed

• nvflare/private/fed/server/job_runner.py — _deploy_job() timeout as failure; _start_run() selective exclusion
• nvflare/private/fed/server/admin.py — check_client_replies() returns timed-out list; dict-keyed non-strict path
• nvflare/private/fed/server/fed_server.py — _sync_client_jobs() default True; _job_reported_clients attr; stale cleanup
• nvflare/private/fed/client/client_run_manager.py — explicit meta validation in get_job_clients()

---

Test Coverage

New and updated unit tests with both positive and negative cases:

File	Tests	What they cover
admin_test.py	8	Timeout returned not raised; dict lookup; error still raises; reorder OK
job_runner_test.py	4	strict flag wiring; timeout within tolerance → warn; timeout below tolerance → raise
job_runner_deploy_test.py	9 (new file)	Timeout counted as failure; OK reply not failed; mixed outcomes; detail label; min_sites with timeouts; integration sequence
fed_server_test.py	5	Default requires-prior-report; legacy explicit-False still fires; tracking in server attr not job_info; stale cleanup

All 29 targeted unit tests pass.

Test Plan

• Unit tests for each changed function (positive + negative)
• New job_runner_deploy_test.py covering deployment timeout classification end-to-end
• All 29 targeted unit tests pass
• Hierarchical staging run with all flags at default
• Hierarchical staging run with strict_start_job_reply_check=true and reduced min_clients
• Verify no regression on standard (non-hierarchical) FL jobs

[greptile-apps]

Greptile Summary

This PR addresses cascading startup failures in large-scale hierarchical FL by fixing timeout handling and client exclusion logic across the job lifecycle layer.

Key Changes:

• _deploy_job() now treats deployment timeouts (reply=None) as failures rather than unknown status, enabling proper min_sites/required_sites validation
• check_client_replies() refactored to return timed-out client list in strict mode instead of raising, allowing selective exclusion within tolerance bounds; also switched from fragile zip() to dict-based lookup
• _start_run() implements selective client exclusion with required_sites and min_sites re-evaluation after timeouts, and correctly handles metadata in both strict and non-strict modes
• _sync_client_jobs() default changed to require prior positive heartbeat before firing dead-job notifications (prevents false positives during startup), and tracking moved from job_info dict to clean instance variable with stale entry cleanup
• get_job_clients() now validates metadata explicitly to prevent opaque TypeError crashes

All previous review concerns addressed:

• Non-strict mode now correctly excludes timed-out clients from JOB_CLIENTS metadata (lines 307-320 in job_runner.py)
• required_sites validation added to start phase, consistent with deployment phase
• Double metadata assignment is intentional: set before start_client_job for serialization, then updated after timeout resolution

Test Coverage: 29 new/updated unit tests with comprehensive positive and negative cases validate all fixes.

Confidence Score: 5/5

• This PR is safe to merge with minimal risk - all fixes are well-architected, thoroughly tested, and address real production failures
• Score reflects comprehensive fix of critical production issue with excellent test coverage (29 tests), all previous review concerns properly addressed, clean refactoring that improves maintainability, and no breaking changes to existing functionality
• No files require special attention - all changes are well-tested and properly validated

Important Files Changed

Filename	Overview
nvflare/private/fed/server/job_runner.py	Deployment timeout classification, selective client exclusion with min_sites/required_sites validation, and proper JOB_CLIENTS metadata management in both strict and non-strict modes
nvflare/private/fed/server/admin.py	Refactored check_client_replies to return timed-out clients list instead of raising, with dict-based lookup and proper type checking
nvflare/private/fed/server/fed_server.py	Changed default for require_previous_report to True, moved _job_reported_clients tracking to instance variable with proper cleanup, preventing premature dead-job detection
nvflare/private/fed/client/client_run_manager.py	Added explicit validation for JOB_CLIENTS metadata to prevent TypeError when metadata is None or wrong type

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    Start[Job Deployment Start] --> Deploy[_deploy_job: Send to clients]
    Deploy --> CheckDeploy{Check replies}
    CheckDeploy -->|reply=None| Timeout1[Add to failed_clients with 'deployment timeout']
    CheckDeploy -->|reply.rc!=OK| Fail1[Add to failed_clients with error detail]
    CheckDeploy -->|reply.rc=OK| Success1[Mark as deployed OK]
    
    Timeout1 --> Validate1{Validate min_sites/<br/>required_sites}
    Fail1 --> Validate1
    Success1 --> Validate1
    
    Validate1 -->|Failed client in<br/>required_sites| Abort1[Abort job]
    Validate1 -->|Active < min_sites| Abort1
    Validate1 -->|Within tolerance| StartPhase[_start_run: Start job]
    
    StartPhase --> SetMeta1[Set JOB_CLIENTS metadata<br/>before start_client_job]
    SetMeta1 --> StartClients[Call start_client_job]
    StartClients --> CheckStart{check_client_replies<br/>strict mode?}
    
    CheckStart -->|strict=True| StrictPath[Returns timed_out list]
    CheckStart -->|strict=False| NonStrictPath[Returns empty list]
    
    StrictPath --> HasTimeout{timed_out<br/>non-empty?}
    HasTimeout -->|Yes| CheckRequired2{Timed-out client<br/>in required_sites?}
    CheckRequired2 -->|Yes| Abort2[Abort job]
    CheckRequired2 -->|No| CheckMin2{Active >= min_sites?}
    CheckMin2 -->|No| Abort2
    CheckMin2 -->|Yes| Warn[Log warning,<br/>exclude timed-out clients]
    HasTimeout -->|No| UpdateMeta
    
    NonStrictPath --> RebuildFromReplies[Rebuild active_client_sites<br/>from actual replies]
    RebuildFromReplies --> UpdateMeta[Update JOB_CLIENTS metadata<br/>with active clients only]
    Warn --> UpdateMeta
    
    UpdateMeta --> Running[Job Running]
    Running --> Heartbeat[Client heartbeat]
    Heartbeat --> SyncJobs{_sync_client_jobs}
    
    SyncJobs --> CheckPrior{require_previous_report<br/>default=True}
    CheckPrior -->|Job on server<br/>but not client| HasReported{Client reported<br/>this job before?}
    HasReported -->|Yes| DeadJob[Fire dead-job notification]
    HasReported -->|No| SkipNotify[Skip notification<br/>still starting up]
    
    CheckPrior -->|Job on client<br/>but not server| AbortClient[Tell client to abort job]

Loading

_{Last reviewed commit: 71d200e}

[greptile-apps]

_{11 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

/build

[greptile-apps]

_{12 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

nvflare/private/fed/server/job_runner.py
job.meta[JobMetaKey.JOB_CLIENTS] is populated with all clients from client_sites (line 264-265), but after timeout exclusion (line 295), some clients may be removed from the active set. This creates a mismatch: timed-out clients are in JOB_CLIENTS metadata but won't actually start. Downstream code expecting JOB_CLIENTS to reflect active participants may break.

        # After timeout exclusion, rebuild job.meta[JobMetaKey.JOB_CLIENTS] with only active clients
        if timed_out:
            active_count = len(client_sites_names) - len(timed_out)
            if job.min_sites and active_count < job.min_sites:
                raise RuntimeError(
                    f"start job ({job_id}): {len(timed_out)} client(s) timed out and remaining "
                    f"{active_count} < min_sites {job.min_sites}: {timed_out}"
                )
            self.log_warning(
                fl_ctx,
                f"start job ({job_id}): {len(timed_out)} client(s) timed out at start-job: {timed_out}; "
                f"{active_count} of {len(client_sites_names)} clients started successfully.",
            )
            client_sites_names = [c for c in client_sites_names if c not in timed_out]
            # Update JOB_CLIENTS to reflect only active participants
            active_clients = [c.to_dict() for token, c in job_clients.items() 
                            if c.name in client_sites_names]
            job.meta[JobMetaKey.JOB_CLIENTS] = active_clients

[chesterxgchen]

Addressed the metadata consistency feedback in commit e0af9ac.

Changes made:

• JobRunner._start_run() now rebuilds job.meta[JobMetaKey.JOB_CLIENTS] after timeout exclusion so metadata reflects active participants only.
• Added unit tests to cover:
  • metadata update when clients time out (test_start_run_updates_job_clients_meta_after_timeout_exclusion)
  • metadata unchanged when no timeouts (test_start_run_keeps_job_clients_meta_when_no_timeouts)
  • deploy->start filtering path now actually executes _start_run() and verifies only deployable clients are started.

Validation:

• pytest -q tests/unit_test/private/fed/server/job_runner_test.py
• pytest -q tests/unit_test/private/fed/server/job_runner_deploy_test.py

[greptile-apps]

_{12 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

Follow-up added in commit df7c06b:

• Added an integration-style _start_run test that uses the real check_client_replies timeout path (no mock for reply-check behavior) and verifies JOB_CLIENTS is reduced to active clients after timeout exclusion.

Validation on updated test scope:

• flake8 tests/unit_test/private/fed/server/job_runner_test.py
• pytest -q tests/unit_test/private/fed/server/job_runner_test.py tests/unit_test/private/fed/server/job_runner_deploy_test.py (16 passed)

[greptile-apps]

_{12 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

/build

[chesterxgchen]

/build

[greptile-apps]

_{11 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

/build

[greptile-apps]

_{13 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

/build

[chesterxgchen]

/build

[greptile-apps]

_{13 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{13 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

/build