fix(webactor): notice a worker that dies before its handshake answers - #18
Merged
Merged
Conversation
… sleeps Three tests raced the worker startup they were meant to observe. The handshake takes 49-66ms on an idle machine, so a fixed 100ms sleep left 35-50ms of slack; a loaded CI runner running 16 test files in parallel blows through that and the assertion sees nothing yet. Wait for the awaited condition instead. Liveness assertions move into vi.waitFor; the "no second restart happened" check keeps a fixed settle window, which is safe because it fails open on a slow machine. The manual-termination test had a second defect that no timeout tolerance could fix: it terminated the worker on a blind 300ms timer while its own wait was also 300ms, so on a slow machine it killed the worker before the handshake completed and the supervisor never restarted it. Terminate after a ping/pong proves the worker is actually up, so the test exercises what it claims: a live worker dies and gets replaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AStaroverov
force-pushed
the
fix/worker-supervisor-handshake-timeout
branch
2 times, most recently
from
July 28, 2026 16:29
95aded8 to
d8249fb
Compare
The liveness watch keys off a lock name carried in the handshake reply, so it cannot be armed until that reply arrives. Until then only the worker's error event could report trouble, and a worker that died silently in that window was never noticed: the handshake request retried forever and the supervisor kept a dead worker indefinitely. Measured: a worker terminated 15ms after spawn, one calling self.close() at startup, and one that stays up but never answers all produced zero restart decisions. getAbortSignal lets the caller bound that wait with the same primitive the rest of the library takes, rather than a bespoke timeout number. It is a factory because a supervisor relaunches, and one signal would already be spent by the second worker. All three cases above now reach shouldRetry, carrying whatever the signal aborted with, so AbortSignal.timeout surfaces as a TimeoutError. The catch on the handshake was swallowing the fix: it mapped aborts to a sentinel and rethrew everything else, so a failure surfaced as an unhandled rejection rather than a restart decision. It now distinguishes by source instead of by reason shape - only the supervisor's own teardown stays quiet, so a plain AbortController from the caller counts as a failed handshake rather than being mistaken for that teardown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AStaroverov
force-pushed
the
fix/worker-supervisor-handshake-timeout
branch
from
July 28, 2026 16:51
d8249fb to
fcd0eed
Compare
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.
Started from the CI failure on
tests/worker/supervisor.test.ts > should handle async shouldRetry with Promise rejection for real worker(AssertionError: expected +0 to be 1). That turned out to be a flaky test, but tracking down why it was flaky uncovered a real gap inapplyWorkerSupervisor.The flaky tests
Three tests raced the worker startup they were meant to observe. Measured with an instrumented probe, the handshake takes 49–66 ms on an idle machine, against a hard 100 ms sleep — 35–50 ms of slack. A 2–4 core runner with 16 vitest files in parallel blows through that.
Plain repetition does not reproduce it (15/15 pass locally, idle). Shrinking the wait from 100 ms to 40 ms reproduced the exact CI error on the first try.
Fixed by waiting for the condition. Liveness assertions moved into
vi.waitFor; "no second restart happened" keeps a fixed settle window, which fails open on a slow machine.The manual-termination test had a second defect that no timeout tolerance could fix: on a slow machine its blind 300 ms timer killed the worker before the handshake completed, and the supervisor genuinely never restarts that — see below. It now terminates only after a ping/pong proves the worker is up, so it tests what it claims.
The gap
Death detection in
applyWorkerSupervisorhas exactly two triggers:on(worker, 'error', …)onUnlock(threadId, …)— but the lock key is learned from the handshake reply, so the watch can only be armed after that reply arrives.Before that reply, only trigger 1 exists. And the handshake
requesthas no deadline of its own: it retries everyretryDelayforever, and its abort signal is fired only fromclose(), which is only reachable fromdecide(). So a worker dying quietly in the startup window was never noticed — no restart, no error, nothing.Probed with six scenarios, counting
shouldRetrycalls over a 3 s window:terminate()15 ms after spawnself.close()immediatelyPromise.reject(…)Note the asymmetry: after the handshake, silent death is covered by the lock watch. The library already intends to catch this — it just had a startup blind window. C and F were always caught because they surface as
errorevents; E is the realistic production shape of the gap (host kills the worker, or it closes itself).The change
applyWorkerSupervisoracceptsgetAbortSignal, consulted once per launch:A factory rather than a plain signal because a supervisor relaunches — one signal would already be spent by the second worker. There is a test pinning that a fresh signal is built per launch.
Nothing changes when it is omitted, and there is a test pinning that too: a deadline tight enough to be useful would misfire on a loaded machine, so the choice stays with the caller.
Why a signal and not a timeout number. An earlier revision of this PR added
timeouttorequestandopenTimeouttoopenChannel. Both were dropped: measurement showedAbortSignal.timeoutalready bounded the whole ofopenChannelonmain— both the request for the port and the handshake that follows — so those options added ergonomics, not capability.applyWorkerSupervisorwas the only place with a genuine hole, because its abort controller is internal and callers had no way to reach it. Building the fix on the primitive the rest of the library already takes keeps one way to bound an operation instead of two.The
catchfix is the other half, and it is not cosmetic. The handshake used.catch(catchAbortToSymbol), which maps aborts to a sentinel and rethrows the rest — so a failure became an unhandled rejection instead of a restart decision. It now distinguishes by source rather than by reason shape: only the supervisor's own teardown stays quiet. That distinction matters — with a reason-shape check, a plainAbortControllerfrom the caller aborts withAbortError, is mistaken for teardown, and gets swallowed, leaving the worker unwatched all over again. Verified: swapping the source check forisAbortfails the test covering that case.Verification
shouldRetrywith aTimeoutErrorwhen a signal is supplied, and still reach nobody when it is not.format:checkclean. e2e 32/32, devtools 32/32.Three unrelated flakes surfaced while verifying, all in tests this PR does not touch, none reproducible, and all green on
main:e2e/tests/devtools-scenarios.spec.ts:394,packages/devtools tests/extension.spec.ts:62, andpackages/devtools tests/panel.spec.ts:397. The last one failed the DevTools job on CI here and passed on a re-run with no code change; it survives 34 local runs including 4× parallel. It is the same defect class as the tests fixed above — a fixed 600 ms wait for a force-directed layout to settle, then a click at coordinates sampled before the click lands, so the node can drift out from under it on a slow machine. Worth its own fix, separately.Left alone, on purpose
In the supervisor's
.then(), a handshake reply without a stringthreadIdstill arms no watch and says nothing — permanently blind, silently. Out of scope here; worth a follow-up.🤖 Generated with Claude Code