Skip to content

fix(runtime): propagate MP failures and return unhealthy status - #2837

Merged
ZhangLirong-amd merged 3 commits into
mainfrom
lirzhang/fix-worker-failure-health
Oct 30, 2026
Merged

ZhangLirong-amd merged 3 commits into
mainfrom
lirzhang/fix-worker-failure-health

Conversation

@ZhangLirong-amd

@ZhangLirong-amd ZhangLirong-amd commented Oct 30, 2026 •

Copy link
Copy Markdown
Collaborator

A failed LMCache MP future currently looks like a pending transfer until the 1,200-second deadline. A live LMCache ZMQ server can also leave a request unanswered after its handler raises. If a ModelRunner then exits, EngineCore cleanup can delay notifying the frontend while /health continues returning 200. A failed engine can therefore look healthy while requests stop progressing.

Raise LMCacheTransferUnprovable immediately when an MP future's query/result raises, retaining the pending transfer and its memory lease because an exception does not prove remote DMA has stopped. Notify the frontend of unexpected ModelRunner death before worker cleanup, retain the reported failure in shared CoreManager state, and return HTTP 503 for a missing/closed/failed engine or a dead local core.

Reduce the configurable per-transfer deadline from 1,200 to 30 seconds and the scheduler's extra completion-report margin from 60 to 10 seconds (40 seconds total by default). This bounds silent futures in ATOM without changing LMCache. Reject nonfinite/nonpositive deadline overrides. Document that server queue time counts, checks run during completion polling, and timeout never authorizes reusing potentially active DMA buffers.

The 30-second default is a failure-detection policy, not a measured end-to-end latency guarantee. Archived C1229 CPU-offload logs contained 132,447 STORE and 4,515 RETRIEVE timing records; per-instance handler P99 was 0.157–0.212 seconds and the maximum 4.362 seconds. Those timers exclude RPC queueing and do not prove GPU completion, so they do not establish that a 5–10 second deadline is safe. Slower/overloaded deployments can raise lmcache.mp.transfer_deadline_s; the new threshold still needs sustained GPU validation.

Validation on main 124c3b0 plus this change:

  • The preceding failure-propagation commit passed 264 focused CPU tests, including expected versus unexpected worker exit, notification ordering, a real ZMQ SHUTDOWN message reaching health state, and the HTTP health endpoint.
  • Final deadline update: 173 ordinary/native MP worker and scheduler CPU tests passed. Includes never-ready futures without a remote exception, retention of leases on timeout, successful completion before the deadline, the 40-second scheduler bound, and override validation.
  • Black, Ruff and git diff --check passed for the deadline update. The earlier failure-propagation change introduces no new Ruff diagnostics; four existing BLE001/S110 findings remain relative to main.

The change is independent of a new LMCache error wire format: it handles exceptions already exposed by futures and uses the shorter deadline when no reply arrives. It does not fix or claim to identify the original intermittent HIP event failure, and polling deadlines do not detect a completely blocked worker/scheduler. The earlier combined deployment completed a C1229 run, which is not a GPU validation of this exact head.

…y probes

Signed-off-by: ZhangLirong <219599949+ZhangLirong-amd@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 30, 2026 03:20
@github-actions

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every eligible PR before approval:

  • ✅ Pre Checkin: Black, Ruff, catalog schema validation, non-GPU unit tests

Heavy model tests:

  • ✅ Run after the PR is approved and Pre Checkin passes
  • ✅ Run immediately when an approval review is submitted
  • ✅ Can be requested before approval with labels
Label Tests
ci:full Run all heavy PR model tests: native ATOM, vLLM, and SGLang
ci:atom Run native ATOM model accuracy tests
ci:vllm Run ATOM vLLM OOT model accuracy tests
ci:sglang Run ATOM SGLang model accuracy tests

Heavy jobs are skipped when the PR is not approved and no matching ci:* label is present.
Add labels via the sidebar or gh pr edit 2837 --add-label <label>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The fail-stop change spans LMCache DMA and multiprocess teardown without ROCm fault-injection validation on the exact head for lease safety and prompt unhealthy reporting while GPU cleanup blocks.

No open issues 🎉

What changed in this PR

This PR connects LMCache transfer failures and unexpected ModelRunner exits to ATOM’s frontend health state, preventing failed engines from appearing healthy during slow cleanup.

Changes:

  • Raise immediately on MP future exceptions while retaining pending transfers and memory leases.
  • Notify the frontend before worker cleanup and retain failure state in CoreManager.
  • Return HTTP 503 for unavailable or unhealthy engines, with regression coverage for failure propagation and health checks.
File Description
tests/​test_lmcache_mp.py Verify immediate transfer failure without releasing pending leases.
tests/​test_lmcache_mp_native_worker.py Verify native transfer failures retain DMA source memory.
tests/​test_engine_core_mgr_shared_state.py Cover shared health state and ZMQ shutdown notification handling.
tests/​test_collective_rpc_transport.py Cover notification ordering and expected worker shutdown.
tests/​entrypoints/​test_api_server_helpers.py Check healthy and unavailable HTTP responses.
atom/​model_engine/​engine_core.py Wire worker failures to frontend notification.
atom/​model_engine/​engine_core_mgr.py Track reported failures and expose engine health checks.
atom/​model_engine/​async_proc.py Report unexpected worker death before cleanup.
atom/​kv_transfer/​offload/​mp/​transfer.py Raise on future exceptions without claiming DMA safety.
atom/​entrypoints/​openai/​api_server.py Return HTTP 503 when the engine is unhealthy or missing.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 30, 2026 04:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

The new 30-second fail-stop deadline still needs sustained GPU validation at this head under realistic MP-transfer queueing and load to assess the risk of shutting down healthy deployments.

No open issues 🎉

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings October 30, 2026 07:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

CPU tests do not establish that healthy queued GPU transfers stay below the new 30-second fatal deadline, leaving sustained validation on this exact head and human review of rollout safety necessary.

No open issues 🎉

Review effort: Balanced

@ZhangLirong-amd
ZhangLirong-amd merged commit 07b49e8 into main Oct 30, 2026
50 of 51 checks passed
@ZhangLirong-amd
ZhangLirong-amd deleted the lirzhang/fix-worker-failure-health branch October 30, 2026 09:57
PerryZhang01 pushed a commit that referenced this pull request Oct 31, 2026
Conflicts:
- atom/plugin/vllm/paged_state_checkpoint.py: main now hooks every GPU
  runner class (V1 _update_states, V2 finish_requests) through
  _note_scheduler_output, which replaces this branch's
  patch_runner_update_states. V4.1 keeps only the registration of its
  slot-owner records as a slot allocator (install_v41_slot_owner_release).
- tests/test_lmcache_mp.py: main's fail-stop on a failed future (#2837) is
  kept; the tests wait for the drain thread where this branch polls saves
  there, and a load's failure surfaces in get_finished's last poll.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants