Skip to content

[Code Health] RunHandle.wait() can permanently strand a handle if finalize raises asynchronously #1479

Description

@ChaoWao

Category

Robustness (potential edge-case failure)

Component

Host Runtime

Description

RunHandle.wait() elects exactly one waiter to cross the native fence and publish cleanup. That waiter sets _wait_in_progress = True, calls _finalize_run_handle(...), and only afterwards marks the handle terminal and wakes the other waiters. The _finalize_run_handle call site has no exception guard:

error = self._worker._finalize_run_handle(self, run_id, native_error)
with self._cv:
    self._error = error
    self._run_id = None
    self._keepalive = None
    self._terminal = True
    self._wait_in_progress = False
    self._cv.notify_all()

Inside _finalize_run_handle, each cleanup step is individually protected by the local _step() helper, which folds a BaseException into result. Its trailing block is not protected:

with self._hierarchical_start_cv:
    self._accepted_run_handles.discard(handle)
    self._hierarchical_start_cv.notify_all()
return result

If an asynchronous exception (KeyboardInterrupt, SystemExit) lands while acquiring _hierarchical_start_cv or during notify_all(), it propagates out of _finalize_run_handle uncaught. Back in wait(), _terminal stays False and _wait_in_progress stays True, and no other code path ever clears them or calls notify_all() on that handle again. Consequences:

  • every later wait() on the handle blocks forever, or times out repeatedly without progress
  • done reports False permanently (it short-circuits on _wait_in_progress)
  • the handle is never removed from _accepted_run_handles, so Worker.close() hangs indefinitely draining it — close() calls handle.wait() with no timeout

Worker.close() already guards against this exact class of hazard for its own joiners ("done is set BEFORE the CV acquire ... cannot strand a joiner"); the same protection was not applied to the RunHandle finalize path.

Not observed in practice — found by reading the code while reviewing the worker async-preparation series, and verified against main.

Location

  • python/simpler/worker.pyRunHandle.wait(), at the self._worker._finalize_run_handle(...) call site
  • python/simpler/worker.pyWorker._finalize_run_handle(), the trailing _accepted_run_handles.discard(...) / notify_all() block outside _step()

Proposed Fix

Make the terminal-state publication unconditional, mirroring the Worker.close() pattern:

  • set _terminal / _wait_in_progress (and cache _error) even when _finalize_run_handle raises, so the handle can never be left un-terminal
  • wrap _finalize_run_handle's trailing discard/notify so an async exception there cannot skip retiring the handle from _accepted_run_handles
  • preserve the raised exception as the handle's error rather than losing it

Priority

Medium (minor risk, should fix in next few releases)


Found while reviewing #1462; out of scope for that PR (the code landed in #1460).

Metadata

Metadata

Assignees

Labels

code healthTechnical debt, robustness, code quality

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions