ref(node): Fix flaky ANR stop-and-restart test with deterministic worker-ready signal - #23514
Merged
Conversation
Contributor
size-limit report 📦
|
mydea
force-pushed
the
fn/fix-anr-worker-restart-flake
branch
from
September 7, 2026 12:32
f7d93b2 to
05ee48e
Compare
mydea
changed the title
mydea
marked this pull request as ready for review
September 7, 2026 12:33
mydea
requested review from
andreiborza,
isaacs and
nicohrubec
and removed request for
a team
September 7, 2026 12:33
isaacs
approved these changes
Sep 8, 2026
Member
There was a problem hiding this comment.
Should probably just remove this method, since it's not used?
…y signal The `stop-and-start` ANR test restarts the worker and then blocks the event loop, expecting the restarted worker to sample and report the ANR. It previously ran `longWork` almost immediately after `startWorker()`. `waitForDebuggerReady` (polling `inspector.url()`) cannot gate this: the ANR integration opens the main-thread inspector once and never closes it, so `inspector.url()` stays truthy across restarts and the poll returns immediately. `_startWorker` is also async, so the new worker may not even be spawned yet. On slow CI the event loop can block before the new worker reconnects its inspector session, and the ANR is missed. The worker now posts a `worker-ready` message once it is fully set up, and the integration exposes `waitUntilWorkerReady()` so the restart path can await the new worker's actual readiness instead of a fixed delay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mydea
force-pushed
the
fn/fix-anr-worker-restart-flake
branch
from
September 9, 2026 11:55
05ee48e to
5ed3395
Compare
mydea
enabled auto-merge (squash)
September 9, 2026 11:55
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
92
to
98
| } | ||
| }, | ||
| stopWorker: () => { | ||
| if (worker) { | ||
| workerReady = undefined; | ||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
| worker.then(stop => { |
Contributor
There was a problem hiding this comment.
Bug: A race condition in disableAnrDetectionForCallback with a synchronous callback can permanently disable ANR detection because startWorker exits early before the old worker is fully terminated.
Severity: HIGH
Suggested Fix
To fix this, ensure worker is cleared before or at the same time as workerReady in stopWorker. For example, set workerReady to undefined inside the .then() block after the worker has been stopped and its promise cleared. Alternatively, modify startWorker to handle this state correctly and re-initialize the worker.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/node/src/integrations/anr/index.ts#L92-L98
Potential issue: A race condition occurs when `disableAnrDetectionForCallback` is used
with a synchronous callback. The `stopWorker` function sets `workerReady` to `undefined`
synchronously but only schedules the termination of the `worker` promise asynchronously
via a microtask. If the synchronous callback finishes quickly, `startWorker` is called
before the microtask runs. Inside `startWorker`, the check `if (worker)` is true,
causing it to return early without starting a new worker. When the microtask eventually
runs, it clears the `worker`, leaving the integration in a state with no worker running
and no `workerReady` promise. This permanently disables ANR detection for the
application's lifecycle.
Also affects:
packages/node/src/integrations/anr/index.ts:68~70
Did we get this right? 👍 / 👎 to inform future reviews.
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
2 participants
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.
The
anr/stop-and-startintegration test restarts the ANR worker and then blocks the event loop, expecting the restarted worker to sample and report the ANR. The gate beforelongWorkwas ineffective, so on slow CI the event loop could block before the new worker had reconnected — and the ANR was missed, making the test flaky.Root cause
waitForDebuggerReadypollsinspector.url(). That works for the initial worker start (whereinspector.open(0)flipsinspector.url()from unset to set), but it cannot gate a restart:inspector.close(), soinspector.url()stays truthy acrossstopWorker()/startWorker(). The poll returns immediately and only a fixed ~200ms delay remains._startWorkerisasync(it awaitsgetContexts), so when the poll passes the new worker may not even be spawned yet.The main thread has no observable signal that the new worker's
InspectorSessionhas reconnected, so any main-thread poll or fixed delay is a race.Change
The worker now posts a
worker-readymessage once it is fully set up (inspector session connected when capturing stack traces, watchdog armed, message handler registered). The integration tracks this and exposes an internalwaitUntilWorkerReady(), which the restart path awaits instead of relying on a fixed delay. This makes readiness deterministic rather than timing-dependent.The flake exists on
developtoo (the restart path there blocks after a baresetTimeout(..., 0)), which is why this is a standalone fix rather than part of any feature branch.