test: fix BroadcastChannel leakage in worker.js test suite - #816
Open
krajorama wants to merge 1 commit into
Open
test: fix BroadcastChannel leakage in worker.js test suite#816krajorama wants to merge 1 commit into
krajorama wants to merge 1 commit into
Conversation
jest.resetModules() creates a fresh WorkerRegistry module instance per test, each with its own ANNOUNCEMENT_CHANNEL and per-thread channel. BroadcastChannels keep receiving messages until explicitly closed, even once nothing in JS still references them, so the previous instance's listeners stayed alive and kept reacting to later tests' messages on the same channel names. That leakage is what let "aggregates worker responses in thread id order" and two shutdown() tests pass at all: their `discovery` promise, which waits for a non-primary ANNOUNCEMENT echo, only ever resolved via a stray listener left over from an earlier test, not from anything the test itself set up. In true isolation all three failed outright. Add a guarded WorkerRegistry.resetForTesting() to close a module instance's channels and reset its state, call it everywhere a test loads a fresh instance, and replace the discovery/echo pattern with polling WorkerRegistry.workerCount(), a real signal that already existed for this purpose. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for opening this pull request! Each pull request require an update in the CHANGELOG. Please update it based on your changes. |
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.
What
Follow-up to #806. While reviewing that PR,
test/workerTest.js's "aggregates worker responses in thread id order" and twoshutdown()tests turned out to only pass by accident:jest.resetModules()creates a freshWorkerRegistrymodule instance per test, each with its ownANNOUNCEMENT_CHANNELand per-threadBroadcastChannel. Those channels keep receiving messages until explicitly closed — even once nothing in JS still references them — so a previous test's listeners stayed alive and kept reacting to later tests' messages on the same channel names.That leakage is exactly what let those three tests pass at all: their
discoverypromise, which waits for a non-primaryANNOUNCEMENTecho, only ever resolved via a stray listener left over from an earlier test — never from anything the test itself set up. Run any of the three in true isolation (-t "...") againstmainand they fail outright (timeout orInvalidStateError: BroadcastChannel is closed).How
WorkerRegistry.resetForTesting()(throws outsidetest/development, mirroring the existing guard oncluster.js'sscanListeners) that closesANNOUNCEMENT_CHANNEL, the per-thread channel, and every registered worker's channel, then resetsworkers/requests/historicMetrics/listenersAdded.jest.resetModules(), including a 30-iteration loop that previously leaked 30 channels per run and closed none.discovery/echo pattern in the two affected tests with polling the already-existingWorkerRegistry.workerCount()— a real signal, rather than hoping to observe an echo that a self-postedBroadcastChannelmessage never actually delivers back to its own sender.Test plan
npx jest— full suite passes (581/581 on currentmain)npx eslint . && npx prettier . --check && npx tsc --project .— all clean-t "...", 5/5 each) — this is the actual proof the fragility is gone, not just hiddenSigned-off-by: György Krajcsovits gyorgy.krajcsovits@grafana.com
🤖 Generated with Claude Code