fix(llmlingua): pass a URL object to new Worker() instead of a file:// string - #12823
Closed
anhtahaylove wants to merge 1 commit into
Closed
anhtahaylove wants to merge 1 commit into
anhtahaylove wants to merge 1 commit into
Conversation
…/ string new Worker() rejects a serialized file:// string with ERR_WORKER_PATH, so the LLMLingua ONNX worker never spawned. pump() swallows the spawn error and fail-opens, so every call silently returned uncompressed text. Pass the WHATWG URL object from pathToFileURL() directly, matching how compressionWorkerPool.ts already spawns its worker.
Contributor
Author
|
Superseded by #13093, 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. |
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.
Fixes #12822.
Problem
new Worker()does not accept a serializedfile://string — it wants an absolute path, a./-relative path, or a WHATWGURLobject.worker.ts:237passedpathToFileURL(...).href, so every spawn threwERR_WORKER_PATH:pump()catches spawn failures and fail-opens, so the LLMLingua engine silently returned uncompressed text instead of surfacing an error. The engine has effectively been a no-op wherever the dependency gate passes.Fix
This matches
compressionWorkerPool.ts, which already passes theURLobject returned bypathToFileURL()and spawns correctly. The two call sites now agree.Verification
Against the real
onnxWorker.jsfrom an installed 3.8.50, Node v24.19.0:new Worker(pathToFileURL(f).href, …)(current)ERR_WORKER_PATHnew Worker(pathToFileURL(f), …)(this PR)Also exercised end-to-end through
workerBackend()with the optional deps shimmed: before the change an 80-char input came back 80 chars unchanged (fail-open); after it, the worker starts and the round-trip reaches the worker.Note on error visibility
This PR is the minimal correctness fix. Separately, the bare
catch {}aroundensureWorker()is what let a 100% spawn failure go unnoticed — worth at least a debug-level log so a broken worker path cannot masquerade as "compression ran, saved nothing". Happy to add that here or in a follow-up, whichever you prefer.Relation to my other PRs
Independent of #12813 / #12815 / #12820 — different file, no overlap, any merge order works.