feat: judge endpoint resiliency

[tdene]

Judge models are commonly hosted independently of other jobs. This allows for their reuse.

If a judge model must be rehosted, we cannot allow this to bring down the entire training cluster.

Commonly, training infrastructure have job duration limits. If a judge model meets such a limit in the middle of a training run, it will change addresses, and it needs to communicate this to the rest of the Gym setup.

This PR adds two optional arguments:

• max_connection_retries prevents infinite retries (which cause hangs)
• endpoint_file allows the judge model to communicate its address via a local filesystem; this flag comes with associated helper configs.

[tdene]

/ok to test 4a76616

[tdene]

/claude review

[claude]

SHIP WITH CARE — sound feature, well-tested; one operability tail-risk and a config-convention gap, both non-blocking.

What this does

Adds endpoint rebinding for vLLM model servers whose backend moves hosts (shared HPC serving jobs): endpoint_file publishes the live base_url, _maybe_rebind_endpoint() re-reads it on mtime change and rebuilds clients, and a new max_connection_retries bound on NeMoGymAsyncOpenAI / request() makes in-flight calls against a dead host fail fast (instead of the default retry-forever) so sessions re-resolve onto the successor.

Correctness — looks right

• request()'s new bound is applied only in the ServerDisconnectedError / ClientOSError branches, and raise re-raises the active exception correctly. Verified aiohttp's ClientConnectorError (connection refused / host moved) subclasses ClientOSError, so a dead backend does surface through the bounded path — the design's core assumption holds.
• Default _max_connection_retries=None preserves today's retry-forever behavior for every existing caller; only endpoint_file-backed clients opt into the bound. No behavior change for static base_url.
• Grace-period state machine in _note_endpoint_unpublished() is careful: empty file == missing, republish on the same host heals without rebind, and the clock resets correctly across absence→republish→absence. The tests exercise all of these transitions (including the "empty write resets nothing" edge).

Non-blocking findings (inline)

• NOTE (operability): _maybe_rebind_endpoint() does a blocking os.stat() on the event loop per inference request; on a slow network FS (Lustre, the deployment target) this is a throughput tail-risk. Suggest throttling the FS check to once per N seconds.
• NOTE (config): new defaults (endpoint_connection_retries=8, endpoint_stale_grace_s=300, endpoint_file) aren't reflected in the exemplar vllm_model.yaml; CLAUDE.md wants YAML as the source of truth for defaults.

No async-hang, data-corruption, or public-API concerns. Test coverage is good.

[claude]

NOTE (operability): _maybe_rebind_endpoint() runs a blocking os.stat() (and occasionally open()/read()) on the event loop for every _resolve_client() call — i.e. once per inference request. The feature's target is shared HPC serving jobs, so endpoint_file will typically live on a network FS (Lustre is explicitly called out as a gotcha in CLAUDE.md). A degraded/slow stat there blocks the single-threaded event loop for all concurrent requests on this model server.

BLAST RADIUS: throughput stall under high concurrency when the shared FS is slow — the exact conditions this feature is deployed in. Inference latency normally dominates a sub-ms stat, so this is a tail-risk, not a common case.

FIX: throttle the filesystem check to at most once per N seconds (track last-checked monotonic()), so bursts of requests reuse the cached mtime instead of each issuing a syscall. The grace-period semantics are unaffected since rebinds are rare.

[tdene]

You're absolutely right about this! Fixed exactly how you suggested.

[tdene]

/ok to test 73e0be9

[tdene]

/claude review

[claude]

SHIP — solid, well-tested, backward-compatible.

This adds endpoint-file rebinding to vllm_model (for shared serving jobs that migrate hosts) plus a bounded connection-retry option in server_utils.request(). I focused on the correctness-critical paths:

• Retry counter semantics (correct). In server_utils.request(), retries is incremented before the _max_connection_retries check, so _max_connection_retries=3 yields exactly 3 attempts before re-raising, and None preserves today's retry-forever behavior. Verified against test_request_bounded_connection_retries_surface_dead_endpoint (await_count == 3) and the unbounded default test. No behavior change for existing static-base_url callers — max_connection_retries defaults to None and is only set when endpoint_file is configured.
• Grace-period state machine (sound). _maybe_rebind_endpoint() / _note_endpoint_unpublished() are fully synchronous — no await between the stat, the read, and the mutation of _clients/_session_id_to_client — so there's no event-loop interleaving that could rebind mid-resolution or double-clear sessions. The absent/empty/same-host/moved cases are each handled, and the grace clock only resets on a genuine (re)publish. The lifecycle test covers absent→empty→republish→re-absent transitions well.
• Config convention (compliant). New VLLMModelConfig fields are mirrored in configs/vllm_model.yaml with matching defaults (endpoint_stale_grace_s: 300.0, endpoint_connection_retries: 8, endpoint_check_interval_s: 10.0).
• Public API (unchanged). _max_connection_retries is threaded as an underscore-prefixed kwarg through NeMoGymAsyncOpenAI._request → request(); no existing signature field is removed or made required.

One non-blocking NOTE inline about blocking file I/O (os.stat/open) on the event loop if the endpoint file lives on a networked FS — throttled to once per 10s so low-risk, author's call.

No async-HTTP, verifier/scoring, or dependency-hygiene concerns.

[claude]

NOTE — blocking file I/O on the async event loop. _maybe_rebind_endpoint() is a sync function called from _resolve_client(), which runs inside the async responses()/chat_completions() handlers. os.stat() (and the subsequent open().read()) block the single event loop, stalling all concurrent sessions on this process, not just the caller.

BLAST RADIUS: bounded in practice — the check is throttled to once per endpoint_check_interval_s (10s) and the file read only fires on an mtime change. On a local/tmpfs endpoint file this is microseconds and harmless. But this feature explicitly targets HPC shared serving jobs, and if endpoint_file lives on a networked FS (NFS/Lustre — which CLAUDE.md flags this project runs on), a metadata stall in os.stat freezes every in-flight model request on the process for the duration.

FIX (optional/defense-in-depth): if the endpoint file may live on a cluster FS, run the stat/read via asyncio.to_thread(...) (which requires making the rebind path async) or a thread executor. Given the 10s throttle, author's call — not a blocker.

[tdene]

I agree with your analysis that this is a well-bounded problem.

Your better solution that you suggest would involve a larger refactor and is not suited for this PR.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[tdene]

/ok to test 4f62ecb