fix(compression): terminate idle workers instead of dropping them - #12542
Open
pacocartones wants to merge 2 commits into
Open
fix(compression): terminate idle workers instead of dropping them#12542pacocartones wants to merge 2 commits into
pacocartones wants to merge 2 commits into
Conversation
`CompressionWorkerPool.finish()` evicted an idle worker with
`remove(slot, false)`, which deleted the slot from the pool set but never
called `worker.terminate()`. The worker keeps a `parentPort` listener, so
its event loop and V8 isolate stayed alive forever, and `dispatch()` then
spawned a replacement because `workers.size < size`. The size cap held in
bookkeeping only: every idle cycle leaked one live thread.
`remove()` now always terminates the thread; the `terminate` parameter is
gone because every remaining caller wanted it. The regression test spies
on `Worker.prototype.terminate` around a `{ size: 1, idleMs: 50 }` pool
and checks the idle eviction terminates the thread and does not retain
its MessagePort.
Closes diegosouzapw#11804
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
CompressionWorkerPool.finish()(open-sse/services/compression/compressionWorkerPool.ts:188) evicted an idle worker withthis.remove(slot, false), andremove()(:198-203) only calledworker.terminate()if (terminate). The idle path therefore deleted the slot from the pool set but never terminated the thread;compressionWorker.ts:13keeps aparentPort.on("message")listener, so the worker's event loop and V8 isolate stayed alive forever, anddispatch()(:156) spawned a replacement becauseworkers.size < size. TheOMNI_COMPRESSION_WORKERScap held in bookkeeping only: every idle cycle leaked one live thread. This is the mechanism confirmed on fix(backend): memory spike causing OOM (~1.5GB → ~16GB) when routing requests via combo #11804 (request-driven thread growth, ~90 threads parked inep_poll, RSS far outside the main heap) and the one-line patch the reporter measured on a live instance: 57 leaked workers / 72 threads / 5.77 GB climbing before, 0 leaked / 15 threads / 846 MB flat after.remove()now always terminates the thread. Theterminateparameter is removed rather than flipped at the idle call site:close()andfail()already passedtrue, so the parameter only existed to express the buggy case. A short comment onremove()records why dropping the slot alone is not enough.close(),fail(), timeouts and the queue/dispatch logic are otherwise unchanged;terminate()failures are still swallowed as before.callLogArtifactWorkerhypothesis raised mid-thread (later marked unconfirmed by its author), the six bundled copies of the pool in the Next.js server chunks (they pick this change up at build time), and any change to the idle timeout or pool size defaults.Related Issues
Validation
open-sse/)tests/unit/compression/compression-worker.test.ts+tests/unit/compression/compression-worker-file-resolution.test.ts17/17 (4 suites),node scripts/check/check-complexity-ratchets.mjs --base-ref origin/release/v3.8.51OK (0 violations, base 0),npm run check:changelog-integrityOK,npm run typecheck:coreexit 0npm run lint— eslint run on the two touched source files only (--suppressions-location config/quality/eslint-suppressions.json), exit 0; the repository-wide command was not runrelease/v3.8.51(1a0375fba); focused checks rerun afterwardRed/green: the new case fails on the base with
idle eviction must terminate the worker thread: 0 !== 1and passes with the change (three consecutive runs, 8/8 in the file).Tests Added Or Updated
tests/unit/compression/compression-worker.test.ts(1 new case, "terminates an idle worker instead of only dropping it from the pool"): spies onWorker.prototype.terminateandWorker.prototype.postMessagearound a local{ size: 1, idleMs: 50 }pool, runs one job, waits past the idle window, and asserts that exactly one worker was spawned, thatterminate()was called once, and that the process does not retain the worker'sMessagePort(process.getActiveResourcesInfo()). Thefinallyblock restores both prototypes, closes the pool, and terminates any worker the pool forgot, so a future regression fails the assertion instead of keeping the test runner alive. The seven existing cases are untouched.stryker.conf.jsonchange:open-sse/services/compression/is not in themutatelist.Coverage Notes
open-sse/services/compression/compressionWorkerPool.ts: the idle path offinish()and the unconditionalterminate()inremove()are exercised by the new case;close()by the new case'sfinallyand the existing timeout case;fail()by the existing "fails open" case. No touched file lost coverage.Reviewer Notes
close()andfail()already did. Nothing about job routing changes, so a request arriving after eviction still gets a freshly spawned worker exactly as before.open-sse/mcp-server/server.js), as documented on the issue; a normal build from this branch covers all of them.