Skip to content

fix: discover late workers' models on the next health check - #57

Closed
aniravich wants to merge 1 commit into
mainfrom
fix/late-worker-model-discovery
Closed

aniravich wants to merge 1 commit into
mainfrom
fix/late-worker-model-discovery

Conversation

@aniravich

@aniravich aniravich commented Sep 26, 2026 •

Copy link
Copy Markdown

Purpose

A worker that isn't serving when the router starts is registered under the placeholder model unknown, and requests that name a model only reach workers indexed under it. Such a worker got none of that traffic until the model refresh on every 5th health check (4 minutes after startup at the default 60 s interval), while sticky sessions pinned to the early workers. On a prime-rl RL run with 16 vLLM servers, all 335 opening sessions landed on 2 of them and 59% of the step-0 eval failed.

  • Late workers join on the next health check. The health checker re-reads /v1/models on every check for workers still in no real model's pool, i.e. listed only under the placeholder. Everyone else keeps the 5-check refresh, which picks up LoRA changes. Fetches run concurrently (at most 32 in flight, one per DP base URL) and each result is applied as it arrives, so unreachable workers cost a cycle one 5 s request timeout in total, not one each.
  • /workers reports live models. GET /workers and WorkerInfo gain a models field. model_id stays the label fixed at registration, which kept showing unknown after routing had recovered.
  • Re-registering a URL replaces the worker cleanly. register (through the new register_replacing) removes the old worker from every index first, including its discovered models, and returns it. RouterManager then releases the old worker's policy count before counting the replacement, and removal releases the model of the worker actually removed.
  • Registry writes are serialized. register, remove and sync_worker_models take one lock; reads stay lock-free. A model sync that races a replacement or removal can no longer put the dropped worker back into the model index.

Behavior change: registering a worker at an already-registered URL now fully replaces it. Previously the old worker stayed in its old model indexes, and its policy count was never released.

prime-rl also starts the router only once all its workers are healthy (PrimeIntellect-ai/prime-rl#3687). This PR covers workers that still aren't serving when the router starts, for example with other launchers.

Test Plan

  • cargo fmt --check, cargo clippy --all-targets -- -D warnings (Rust 1.95.0) and cargo test.
  • New tests, each of which fails on the code before its fix: test_late_worker_joins_model_index_on_next_health_check, test_reregistering_url_replaces_discovered_models, test_has_unknown_model_ignores_duplicate_placeholders, test_replacing_worker_rebalances_policy_counts.
  • End to end with the built wheel: two fake workers, one ready 8 s after start, --health-check-interval-secs 5; time until the late worker is indexed under its model, compared with the v0.2.1 wheel. Re-run on the final commit with the router binary.
  • End to end with four fake workers that accept connections but never answer, plus a late worker.
  • A reproduction (not committed) of a model sync racing a re-registration, with the sync paused right after it read the worker; plus 4 threads × 5,000 mixed register / sync / remove calls.
  • Deployed in a prime-rl RL run (Qwen3.8-27B SWE, 16 vLLM servers).

Test Result

  • fmt and clippy clean; all 737 tests pass.
  • Late worker: v0.2.1 indexed it at +20 s (the 5th health check), this branch at +10 s (the first check after it came up). The same holds on the final commit. /workers reports "models": ["m"] for it while model_id stays unknown.
  • Unreachable workers: with four workers that never answer, a health-check cycle takes about 6 s (1 s health timeout plus one 5 s /v1/models wave), and the late worker joins its model 1 s after the health checker starts.
  • Race: without the lock, the reproduction leaves the replaced worker in model_index; with it, the re-registration waits for the sync and nothing stale remains. The stress run finishes without deadlock, and every indexed worker is the one registered at its URL, listed once per model.
  • Deployment: the router started with 16 of 16 workers, and /workers listed all 16 with "models": ["Qwen/Qwen3.8-27B"]. prime-rl's launcher gate was also in place, so the late-worker path itself wasn't exercised in that run.

Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results

Comment thread src/routers/router_manager.rs Resolved
Comment thread src/core/worker_registry.rs Resolved
@macroscopeapp

macroscopeapp Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Approvability

Verdict: Approved at ad1f4b1

Macroscope's review found this PR approvable — This is a contained, tested fix that retries late workers, cleans replacement indexes and policy counts, and adds only an additive worker-models response field. The supplied unresolved Medium findings are at the repository’s blocking threshold, although the corresponding paths appear addressed in the current diff.

Notes:

  • This verdict was updated automatically after the outstanding correctness findings were resolved. Macroscope did not re-review the code.

You can add or adjust custom eligibility rules. Learn more.

@aniravich
aniravich force-pushed the fix/late-worker-model-discovery branch from ab4e5b4 to 46dcf3f Compare September 26, 2026 21:55
Comment thread src/core/worker_registry.rs Resolved
@aniravich
aniravich force-pushed the fix/late-worker-model-discovery branch from 46dcf3f to 1989b9a Compare September 26, 2026 22:15
Comment thread src/core/worker_registry.rs Resolved
Comment thread src/core/worker_registry.rs Resolved
A worker that isn't serving when the router starts is registered under the 'unknown' placeholder model, and requests that name a model only reach workers indexed under it. The late worker got none of that traffic until the model refresh on every 5th health check (4 minutes after startup at the default 60s interval), while sticky sessions pinned to the early workers; in prime-rl job 1498 all 335 opening sessions landed on 2 of 16 servers.

The health checker now re-reads /v1/models on every check for workers still under the placeholder; the rest keep the 5-check refresh for LoRA changes. GET /workers also reports each worker's live models, since model_id is fixed at registration and kept showing 'unknown' after routing recovered.

Model refreshes run concurrently, at most 32 at a time, and each result is applied as it arrives, so unreachable workers cost a health-check cycle one 5s request timeout in total rather than one each. Registering a worker at a URL that is already registered now removes the old worker from every index first, so the models discovered for it don't carry over to the replacement.

Registration, removal and model syncs now take one registry lock, so a model sync that races a replacement or removal can't put the dropped worker back into the model index.

Replacing a worker now hands back the worker it replaced, and the router manager releases that worker's model from its policy counts before counting the replacement; removal releases the model of the worker actually removed. A worker indexed only under the placeholder, even more than once, is retried on every check.
@aniravich
aniravich force-pushed the fix/late-worker-model-discovery branch from 1989b9a to ad1f4b1 Compare September 26, 2026 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant