Skip to content

refactor(compression): drop the terminate flag from pool removal - #12815

Closed
anhtahaylove wants to merge 2 commits into
diegosouzapw:release/v3.8.51from
huuhungn:refactor/compression-pool-always-terminate
Closed

anhtahaylove wants to merge 2 commits into
diegosouzapw:release/v3.8.51from
huuhungn:refactor/compression-pool-always-terminate

Conversation

@anhtahaylove

Copy link
Copy Markdown
Contributor

Follow-up to #12813 — please merge that one first, as this branch is stacked on it.

Rationale

After the idle-eviction fix, all three call sites pass terminate: true:

Call site Argument
close() true
finish() idle timer true (was false — the bug in #12813)
fail() true

The parameter now only exists to express a state the pool cannot safely be in: a slot deleted from this.workers while its Worker keeps running. Once remove() has done this.workers.delete(slot), the pool has dropped its only reference — nothing can terminate that thread afterwards. Passing false is not a valid option, it is a leak.

Removing the flag makes that state unrepresentable, so the same bug cannot be reintroduced by a future caller.

Change

-  private async remove(slot: PoolWorker, terminate: boolean): Promise<void> {
+  private async remove(slot: PoolWorker): Promise<void> {
     if (!this.workers.delete(slot)) return;
     if (slot.timeout) clearTimeout(slot.timeout);
     if (slot.idle) clearTimeout(slot.idle);
-    if (terminate) await slot.worker.terminate().catch(() => undefined);
+    await slot.worker.terminate().catch(() => undefined);
   }

Plus the three call sites dropping their now-redundant argument. remove() is private, so this is not a public API change.

No behavioural change on top of #12813.

Note on the sibling pools

I checked the other two worker owners in the codebase while investigating:

  • open-sse/services/compression/engines/llmlingua/worker.ts — resetWorker() always calls w.terminate()
  • src/lib/usage/callLogArtifactWriter.ts — terminateWorker() always calls .terminate()

Both are singletons that route every exit path (idle, error, exit, close) through a single teardown function, and both null out the reference before terminating. Neither has an equivalent flag, which is why neither leaked. compressionWorkerPool.ts was the only place where "remove from pool" and "stop the thread" were decoupled — this PR closes that gap.

CompressionWorkerPool.finish() scheduled idle eviction with
remove(slot, false), which deletes the slot from this.workers without
terminating the underlying Worker. The pool then loses its only
reference, so the worker thread, its MessagePort and its private heap
survive for the lifetime of the process.

Every burst of compression traffic separated by more than idleMs
strands up to OMNI_COMPRESSION_WORKERS threads. On a 16h instance this
accumulated 55 orphaned MessagePorts, 69 OS threads and 5.7GB of commit
charge, while process.memoryUsage() still reported rss=660MB because V8
does not account for orphaned worker isolates.

The error and shutdown paths already pass true; only the success path
leaked.

Fixes diegosouzapw#12812
After the idle-eviction fix, all three call sites pass terminate=true, so
the parameter only exists to express a state the pool cannot safely be
in: a slot deleted from this.workers while its Worker keeps running.

Removing the flag makes that state unrepresentable. remove() now always
terminates, which is the only correct behaviour once the pool has
dropped its reference to the slot.

No behavioural change on top of the previous commit.
@JasonBroderick

Copy link
Copy Markdown
Contributor

Note for the maintainer: #12542 already removes the terminate parameter as part of the fix itself (with a regression test), so if #12542 is merged this follow-up becomes unnecessary. Field measurements for the fix are on #12812 and #12542.

@anhtahaylove

Copy link
Copy Markdown
Contributor Author

Superseded by #13091, which carries the identical source change plus a regression test and a changelog entry. Closing this one to keep the review queue unambiguous — no need to review both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants