[https://nvbugs/6435642][fix] Revert killed-worker detection (#16338, #16444)

[sunnyqgg]

Summary

Reverts #16338 and its follow-up #16444 (they form one chain: #16444 only exists to patch #16338's interaction with session reuse, so a clean revert must take both).

Context: #16338 added isinstance(self.mpi_session, MpiPoolSession) in proxy.py, evaluated on every LLM() creation. The session-reuse layer (#15777) replaces that module attribute with a factory at test time, so the check raised TypeError: isinstance() arg 2 must be a type and broke every bare-LLM test on post-7/15 bases (~8000 failures across 15+ builds / 12+ PRs in two days, including post-merge main). #16444 worked around it with an exclusion-based match.

Reverting to restore a known-good proxy.py baseline while the interaction is settled properly. The killed-worker detection can re-land on top of #16520, which makes the reuse seam a real type (isinstance-transparent) and removes this hazard class — at that point the natural isinstance(..., MpiPoolSession) form is safe again.

Changes

• Revert of e158837 ([https://nvbugs/6435642][fix] handle session reuse worker registration #16444): restores the pre-workaround registration path and the 2 waives it removed
• Revert of e05790a ([https://nvbugs/6435642][fix] detect killed MPI executor workers #16338): removes worker_process_monitor.py and the proxy/worker hooks
• Both reverts are clean (no manual conflict resolution)

Test plan

• No dangling references to worker_process_monitor / WorkerProcessIdentity / _register_worker_processes after revert
• tests/unittest/executor/test_fatal_error_health_check.py collects cleanly (68 tests, pre-[https://nvbugs/6435642][fix] detect killed MPI executor workers #16338 form)
• Pre-merge CI

Summary by CodeRabbit

• Bug Fixes

  • Improved executor health and fatal-error detection for MPI workers.
  • Pending requests now fail promptly when a worker becomes unavailable.
  • Streamlined worker startup and shutdown behavior.

• Tests

  • Updated health-check and error-monitoring coverage.
  • Adjusted integration test waivers for out-of-tree model coverage.

[coderabbitai]

📝 Walkthrough

Walkthrough

The proxy now detects MPI worker failures through futures and remote-session notifications instead of OS process monitoring. Worker startup readiness payloads omit process identities, and related lifecycle wiring and tests were updated.

Changes

MPI worker liveness

Layer / File(s)	Summary
MPI future health monitoring tensorrt_llm/executor/proxy.py, tensorrt_llm/executor/worker_process_monitor.py	Proxy health checks and the background error monitor use MPI futures and remote worker-death checks; the process-monitor dependency and shutdown handling are removed.
Worker readiness contract tensorrt_llm/executor/worker.py, tensorrt_llm/executor/proxy.py	Workers stop gathering process identities and send (ready_signal, None); proxy initialization reads and handles the two-element status directly.
Health and startup test alignment tests/unittest/executor/test_fatal_error_health_check.py, tests/unittest/executor/test_proxy_fast_death.py, tests/integration/test_lists/waives.txt	Process-monitor tests and registration coverage are removed, MPI-future health checks remain, and additional modeling test waivers are added.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• NVIDIA/TensorRT-LLM#16338: Adds the worker process monitoring path that this PR removes.
• NVIDIA/TensorRT-LLM#16404: Modifies pre_shutdown() process cleanup in the same proxy error flow.
• NVIDIA/TensorRT-LLM#16419: Adjusts proxy worker initialization status handling in the same startup path.

Suggested reviewers: chienchunhung

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title matches the revert of killed-worker detection and follows the required ticket/type format.
Description check	✅ Passed	The description covers the issue, the revert scope, and test plan, though it doesn't use the template headings exactly.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tensorrt_llm/executor/proxy.py`:
- Line 269: Update the health-check flow in tensorrt_llm/executor/proxy.py at
lines 269-269 to call _check_remote_worker_death() after _check_mpi_futures()
and return unhealthy when a remote worker death is detected. In
tests/unittest/executor/test_fatal_error_health_check.py at lines 200-214,
mirror this behavior in ConcreteProxyExecutor and add a fake remote-session test
verifying immediate unhealthy status and shutdown.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 41cc7089-51d8-4d48-9672-851b5bf27874

📥 Commits

Reviewing files that changed from the base of the PR and between 1228fed and 67465e5.

📒 Files selected for processing (6)

• tensorrt_llm/executor/proxy.py
• tensorrt_llm/executor/worker.py
• tensorrt_llm/executor/worker_process_monitor.py
• tests/integration/test_lists/waives.txt
• tests/unittest/executor/test_fatal_error_health_check.py
• tests/unittest/executor/test_proxy_fast_death.py

💤 Files with no reviewable changes (2)

• tensorrt_llm/executor/worker_process_monitor.py
• tests/unittest/executor/test_proxy_fast_death.py

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Synchronously detect remote worker death. Remote sessions can have an empty mpi_futures list, so the current health check remains healthy until the monitor loop polls later.

• tensorrt_llm/executor/proxy.py#L269-L269: call _check_remote_worker_death() after the future check and return unhealthy when it reports a death.
• tests/unittest/executor/test_fatal_error_health_check.py#L200-L214: mirror that check in ConcreteProxyExecutor and add a fake remote-session test asserting immediate unhealthy status and shutdown.

As per path instructions, coverage is insufficient and needs the concrete test above.

📍 Affects 2 files

• tensorrt_llm/executor/proxy.py#L269-L269 (this comment)
• tests/unittest/executor/test_fatal_error_health_check.py#L200-L214

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tensorrt_llm/executor/proxy.py` at line 269, Update the health-check flow in
tensorrt_llm/executor/proxy.py at lines 269-269 to call
_check_remote_worker_death() after _check_mpi_futures() and return unhealthy
when a remote worker death is detected. In
tests/unittest/executor/test_fatal_error_health_check.py at lines 200-214,
mirror this behavior in ConcreteProxyExecutor and add a fake remote-session test
verifying immediate unhealthy status and shutdown.

Source: Path instructions

[sunnyqgg]

Closing: superseded — #16444 already stopped the breakage (verified: zero isinstance() arg 2 failures on any build whose base contains e158837; remaining failures are all un-rebased branches). The structural hazard (the reuse seam holding a non-type) is tracked separately in #16520, which also lets the exclusion-based check in _register_worker_processes revert to the natural isinstance(..., MpiPoolSession) form once merged.