fix(runtime.coordinator): losing a worker fails the threads it hosted - #53
Open
laithalsaadoon wants to merge 1 commit into
Open
laithalsaadoon wants to merge 1 commit into
laithalsaadoon wants to merge 1 commit into
Conversation
A thread whose host worker is gone can never run again, so leaving it ``idle`` in the registry published a thread that ``list_threads`` offered and every routed operation refused with "not found". ``deregister_worker`` — the only signal ``CoordinatorEndpoint._serve_connection`` has when a client socket drops — now appends a ``FAILED`` event naming the lost worker to each non-terminal thread of that worker and moves it to the terminal ``ThreadStatus.FAILED``; ``_adapter_for`` raises ``WorkerLostError`` (with that worker's whole blast radius) rather than ``ThreadNotFoundError`` for a thread that is registered but unroutable; and ``_apply_lifecycle`` never downgrades a terminal status, so a ``COMPLETED`` from a cycle that was already in flight cannot resurrect a dead thread. ``LocalWorker.close`` drains the deregistrations ``_teardown`` only schedules, without which an orderly close would hand ``deregister_worker`` a registry still full of live threads and publish every one of them as failed. Wire-visible change is additive: the ``FAILED`` event is an existing kind every peer already decodes, and a remote caller learns of the loss from the ``"worker_lost"`` ``ErrorKind`` the classification registry already maps ``WorkerLostError`` to. The error stays out of ``channel._KNOWN_EXCEPTIONS`` on purpose — its ``worker_id`` and ``thread_ids`` cannot be rebuilt from an ``ErrorFrame``'s message without inventing them — so the far side sees a ``RemoteError`` whose ``kind`` names the failure rather than a ``WorkerLostError`` with fabricated fields.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
InMemoryCoordinator.deregister_workernow fails every non-terminal thread the departing worker still hosted. Before this change a thread whose host worker was gone stayedidlein the registry:list_threadskept offering it,get_thread_statuskept answeringidle, and every routed operation on it (submit,notify,cancel,terminate,fork) raisedThreadNotFoundErrorfor a thread that was registered.CoordinatorEndpoint._serve_connectioncallsderegister_workerin itsfinallywhen a client socket drops, so this is the only signal the coordinator gets when a worker process dies.Addresses #50 (the worker-death half).
WorkerLostErrorand itserror_kind = "worker_lost"classification already landed in #51; this PR adds only the coordinator and worker behavior that raises it.What changed
runtime/coordinator.py:deregister_workerappends aFailedEventnaming the lost worker to each non-terminal thread of that worker and moves the thread toThreadStatus.FAILED. The orphaned thread ids are remembered per worker so_adapter_forcan raiseWorkerLostError(worker_id, thread_ids)with the whole set for a registered-but-unroutable thread, in place ofThreadNotFoundError._apply_lifecyclereturns early when the thread is already in a terminal status, so aCOMPLETEDorFAILEDfrom a cycle that was in flight when the worker died cannot move the thread back toidle.runtime/worker.py:_teardownonly schedules each thread'sderegister_thread;LocalWorker.closenow drains those tasks before it callsderegister_worker. Without that, an orderly close would handderegister_workera registry still holding the worker's threads and publish all of them as failed.runtime/errors.py:WorkerLostErrordocstring states when the coordinator raises it and how it reaches a remote caller.protocols.py:Coordinator.deregister_workerdocuments the cascade and its postcondition; every routed method documentsWorkerLostErrorinRaises.spec/stubs mirror the docstring changes.Invariants
deregister_workerpostcondition).ThreadStatusis never downgraded by a later lifecycle event.FAILEDstill means "one cycle raised, the thread is idle and runnable"; only the coordinator writesThreadStatus.FAILED, and only fromderegister_worker.deregister_workerstays idempotent: a second call finds no non-terminal thread and appends no secondFAILEDevent.LocalWorker.closeproduces noFAILEDevents and leaveslist_threadsempty.Wire compatibility
The
FAILEDevent is an existing kind every peer decodes. A remote caller sees the loss as aRemoteErrorwithkind == "worker_lost"andremote_type == "WorkerLostError".WorkerLostErroris deliberately not added tochannel._KNOWN_EXCEPTIONS: itsworker_idandthread_idscannot be rebuilt from anErrorFrame's message, so rehydrating it would mean inventing them. No signature changes.Tests
tests/test_worker_death.py(new): a realCoordinatorEndpoint, a client hosting aLocalWorker, and a second observer client. After the host client closes its socket, the observer sees the threadFAILEDinlist_threads, a terminalFailedEventnaming the worker id, andRemoteError(kind="worker_lost")fromsubmitandnotify.tests/test_lifecycle_invariants.py(six new cases):deregister_workerfails the hosted threads and the log ends with aFailedEvent; routed operations on an orphaned thread raiseWorkerLostErrorcarrying every orphaned id; a secondderegister_workerappends nothing; a lateCompletedEventdoes not downgradeFAILED; a failed cycle leaves the threadIDLE; an orderlyclosefails nothing.Gate
Run on this branch (
f56e1e37, base1125f0c4), mcp 2.1.1: