Skip to content

Don't lose worker startup exception when its process exit is observed first (flaky test_local_cluster_redundant_kwarg)#9327

Merged
crusaderky merged 2 commits into
dask:mainfrom
crusaderky:test_local_cluster_redundant_kwarg
Jul 9, 2026
Merged

Don't lose worker startup exception when its process exit is observed first (flaky test_local_cluster_redundant_kwarg)#9327
crusaderky merged 2 commits into
dask:mainfrom
crusaderky:test_local_cluster_redundant_kwarg

Conversation

@crusaderky

@crusaderky crusaderky commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Fix race condition in Nanny when a worker fails to start.

Follow-up to #9313; fixes the remaining flakiness in test_local_cluster_redundant_kwarg (example failure).

In-depth commentary (AI-generated)

Root cause

When the worker process fails to start, the child sends the startup exception to the Nanny through init_result_q, flushes the queue, and terminates. WorkerProcess._wait_until_connected polls that queue every 50ms and, in each iteration, first bails out with None if the status is no longer Status.starting. If mark_stopped() (fired by the process exit callback) flipped the status during one of the 50ms sleeps, before the poll picked up the message, the exception was lost: instantiate() returned Status.stopped and Nanny.start_unsafe raised a bare RuntimeError("Nanny failed to start worker process") instead of chaining from the worker's original exception, breaking the test's raises_with_cause(RuntimeError, None, TypeError, "unexpected keyword argument") expectation.

Fix

  • Poll init_result_q one last time after observing that the process stopped. The child always flushes the queue before exiting (close() + join_thread()), so by the time the exit callback has run, the message — if any — is guaranteed to be readable.
  • Hold a local reference to init_result_q in WorkerProcess.start(), since mark_stopped() releases self.init_result_q.
  • Don't overwrite the status with Status.running if the process already died: mark_stopped() set it to Status.stopped and released the queues, and overwriting it would make the next kill() crash on the released queues, leaving the Nanny stuck in Status.closing (same failure mode described in Fix Nanny race conditions when worker fails to start (flaky test_failure_during_worker_initialization) #9313).

crusaderky and others added 2 commits July 9, 2026 13:50
… first

When the worker process fails to start, it sends the exception to the
Nanny through init_result_q, flushes the queue, and terminates. If
mark_stopped(), fired by the process exit callback, ran before
WorkerProcess._wait_until_connected polled the message out of the queue
(the poll interval is 50ms), the exception was lost: instantiate()
returned Status.stopped and Nanny.start_unsafe raised a bare
RuntimeError("Nanny failed to start worker process") instead of
chaining from the original exception.

Fix by polling init_result_q one last time after observing that the
process stopped; the child flushes the queue before exiting, so the
message (if any) is guaranteed to be readable by then.

Follow-up to dask#9313; fixes the remaining flakiness in
test_local_cluster_redundant_kwarg.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@crusaderky crusaderky changed the title [unsupervised AI] Don't lose worker startup exception when its process exit is observed first (flaky test_local_cluster_redundant_kwarg) Don't lose worker startup exception when its process exit is observed first (flaky test_local_cluster_redundant_kwarg) Jul 9, 2026
@crusaderky crusaderky changed the title Don't lose worker startup exception when its process exit is observed first (flaky test_local_cluster_redundant_kwarg) Don't lose worker startup exception when its process exit is observed first (flaky test_local_cluster_redundant_kwarg) Jul 9, 2026
@crusaderky crusaderky marked this pull request as ready for review July 9, 2026 14:07
@crusaderky crusaderky requested a review from fjetter as a code owner July 9, 2026 14:07
@crusaderky crusaderky merged commit 0f72f8f into dask:main Jul 9, 2026
46 of 50 checks passed
@crusaderky crusaderky deleted the test_local_cluster_redundant_kwarg branch July 9, 2026 15:37
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