Skip to content

fix(idalib): don't reap busy headless workers on health-probe timeout - #483

Open
lich0821 wants to merge 1 commit into
mrexodia:mainfrom
lich0821:fix/idalib-busy-worker-reap
Open

fix(idalib): don't reap busy headless workers on health-probe timeout#483
lich0821 wants to merge 1 commit into
mrexodia:mainfrom
lich0821:fix/idalib-busy-worker-reap

Conversation

@lich0821

@lich0821 lich0821 commented Jul 23, 2026

Copy link
Copy Markdown

Problem

resolve_session() probed each worker (TCP connect + JSON-RPC ping) before
forwarding a tool call, and reaped the worker on the first failed probe. A
worker mid auto_wait()/decompile is single-threaded and cannot answer the
ping until it yields, so a busy worker was misclassified as dead and torn
down — losing the live session. With aggressive warmup
(run_auto_analysis/build_caches/init_hexrays) on a large database this
is reliably reproducible: the worker gets reaped during its own analysis.

Fix

Distinguish three worker states instead of two:

  • process exited -> dead, reap ("not reachable")
  • process alive, no ping -> busy, keep and retry with backoff
  • alive + unresponsive long -> wedged, reap after a grace window

Also:

  • Health-probe timeouts widened and made env-overridable
    (IDA_MCP_HEALTH_TCP_TIMEOUT=2.0, IDA_MCP_HEALTH_RPC_TIMEOUT=10.0)
  • Retry with backoff before reaping
    (IDA_MCP_HEALTH_RETRIES=3, IDA_MCP_HEALTH_RETRY_BACKOFF=1.0)
  • Wedged grace window IDA_MCP_WEDGED_GRACE_SEC=300 (0 = never reap a live
    worker)
  • _prune_dead_worker_sessions_locked now prunes only exited processes
    (is_alive()), not unreachable-but-live ones
  • idle_ttl_sec <= 0 now means "never self-exit" for long-lived workers

All defaults preserve existing behavior for healthy workers; only the
handling of an unreachable worker changes (now conservative instead of
trigger-happy).

Test plan

  • tests/test_idalib_supervisor.py (extended with busy/wedged state
    coverage)
  • tests/test_worker_lifecycle.py

resolve_session() probed each worker (TCP + JSON-RPC ping) before forwarding
a tool call and reaped it on the first failure. A worker mid auto-analysis or
mid decompile is single-threaded and cannot answer the ping until it yields,
so a busy worker was misclassified as dead and torn down — losing the live
session. With aggressive warmup (run_auto_analysis/build_caches/init_hexrays)
on a large database this is reliably reproducible: the worker is reaped during
its own analysis.

Distinguish three worker states instead of two:
  - process exited            -> dead, reap ("not reachable")
  - process alive, no ping    -> busy, keep and retry with backoff
  - alive + unresponsive long -> wedged, reap after a grace window

Also:
  - health-probe timeouts widened and made env-overridable
    (IDA_MCP_HEALTH_TCP_TIMEOUT=2.0, IDA_MCP_HEALTH_RPC_TIMEOUT=10.0)
  - retry with backoff before reaping
    (IDA_MCP_HEALTH_RETRIES=3, IDA_MCP_HEALTH_RETRY_BACKOFF=1.0)
  - wedged grace window IDA_MCP_WEDGED_GRACE_SEC=300 (0 = never reap a
    live worker)
  - _prune_dead_worker_sessions_locked prunes only exited processes
    (is_alive), not unreachable-but-live ones
  - idle_ttl_sec <= 0 now means "never self-exit" for long-lived workers

All defaults preserve existing behavior for healthy workers; only the
handling of an unreachable worker changes (now conservative instead of
trigger-happy).
@NeKroFR

NeKroFR commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Nice fix, but the busy path is gated on session.is_alive(), which for worker-backed sessions is self.process is not None and self.process.poll() is None.
Adopted workers are constructed with process=None in _adopt_worker_instance, so is_alive() is always False for them. The retry loop breaks immediately and they get unregistered as dead even when they're just busy. (_terminate_worker no-ops for owned=False, so the process survives and only the session is lost.) Same on main today, so not a regression but it's the same bug this PR fixes, and the adopted path misses it. Keying "dead" on process.poll() is not None instead would let a missing Popen handle fall through to the probes.

Two smaller ones: on a busy worker resolve_session raises "alive but not responding ... Retry shortly", so the session survives but every call during a long analysis fails. Forwarding the request instead lets it queue on the worker and complete. And the retry loop runs before that error: 3 retries * (1s backoff + up to 12s of probe timeouts with the new 2.0/10.0 defaults) is 30s+ of waiting for a call that fails anyway.

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.

2 participants