mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-21 06:12:17 +03:00
fix(compression): pass a URL object when spawning the LLMLingua worker (#13093)
`node:worker_threads` only treats a `URL` instance as a `file:` URL — a string must be a relative path. The silent `catch {}` in `pump()` made this degrade compression to a passthrough while still reporting success, which is the worst shape for it.
---
Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them.
- `typecheck:core` clean
- complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline
- 71 focused assertions green across the 13 test files this batch adds or touches
⚠️ base-red inherited: #12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff.
Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit.
This commit is contained in:
1
changelog.d/fixes/llmlingua-worker-spawn.md
Normal file
1
changelog.d/fixes/llmlingua-worker-spawn.md
Normal file
@@ -0,0 +1 @@
|
||||
- fix(compression): spawn the LLMLingua worker with a file URL object so compression actually runs instead of silently failing open on Node
|
||||
@@ -234,7 +234,12 @@ function ensureWorker(): Worker {
|
||||
|
||||
const { workerFile, execArgv } = resolveWorkerFile();
|
||||
const absoluteWorkerFile = path.resolve(workerFile);
|
||||
const w = new Worker(pathToFileURL(absoluteWorkerFile).href, { execArgv });
|
||||
// Pass the URL OBJECT, not `.href`. `new Worker()` treats a plain string as a
|
||||
// filesystem path, so a "file://..." string is looked up literally and throws
|
||||
// ERR_WORKER_PATH (a string arg must start with ./ or ../). Only a URL instance
|
||||
// is interpreted as a file: URL. Spawn failures are swallowed by pump()'s catch,
|
||||
// so getting this wrong silently disables compression instead of erroring.
|
||||
const w = new Worker(pathToFileURL(absoluteWorkerFile), { execArgv });
|
||||
|
||||
w.on("message", (reply: WorkerReply) => {
|
||||
const entry = pending.get(reply.id);
|
||||
|
||||
77
tests/unit/compression/llmlingua-worker-spawn-12822.test.ts
Normal file
77
tests/unit/compression/llmlingua-worker-spawn-12822.test.ts
Normal file
@@ -0,0 +1,77 @@
|
||||
/**
|
||||
* Regression guard for #12822: the LLMLingua worker must actually spawn on Node.
|
||||
*
|
||||
* Root cause: `new Worker(pathToFileURL(file).href, ...)` passes a STRING. Node treats a
|
||||
* string argument as a filesystem path (it must start with ./ or ../), so a "file://..."
|
||||
* string is looked up literally and throws ERR_WORKER_PATH. Only a URL INSTANCE is
|
||||
* interpreted as a file: URL.
|
||||
*
|
||||
* Why it was invisible: pump() wraps ensureWorker() in `catch {}` and fails open, so the
|
||||
* spawn crash silently degraded every compression call to a passthrough instead of erroring.
|
||||
*
|
||||
* This test asserts the Node contract directly against a real Worker, so it fails on the
|
||||
* old `.href` spelling and passes on the URL object.
|
||||
*/
|
||||
import { test } from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { Worker } from "node:worker_threads";
|
||||
import { fileURLToPath, pathToFileURL } from "node:url";
|
||||
|
||||
const here = path.dirname(fileURLToPath(import.meta.url));
|
||||
const WORKER_SRC = path.resolve(
|
||||
here,
|
||||
"../../../open-sse/services/compression/engines/llmlingua/worker.ts"
|
||||
);
|
||||
|
||||
function spawnWith(arg: string | URL): Promise<void> {
|
||||
return new Promise((resolve, reject) => {
|
||||
let w: Worker;
|
||||
try {
|
||||
w = new Worker(arg, {});
|
||||
} catch (err) {
|
||||
reject(err);
|
||||
return;
|
||||
}
|
||||
w.on("error", reject);
|
||||
w.on("exit", () => resolve());
|
||||
});
|
||||
}
|
||||
|
||||
test("a file: URL STRING is rejected by node:worker_threads (the #12822 crash)", async () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-worker-"));
|
||||
const child = path.join(dir, "child.mjs");
|
||||
fs.writeFileSync(child, "process.exit(0);\n");
|
||||
|
||||
await assert.rejects(
|
||||
() => spawnWith(pathToFileURL(child).href),
|
||||
(err: NodeJS.ErrnoException) => err.code === "ERR_WORKER_PATH",
|
||||
"passing .href must fail — this is exactly what shipped and was swallowed by the fail-open catch"
|
||||
);
|
||||
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("a file: URL OBJECT spawns cleanly", async () => {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-worker-"));
|
||||
const child = path.join(dir, "child.mjs");
|
||||
fs.writeFileSync(child, "process.exit(0);\n");
|
||||
|
||||
await spawnWith(pathToFileURL(child));
|
||||
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("worker.ts passes the URL object, not .href", () => {
|
||||
const code = fs.readFileSync(WORKER_SRC, "utf8");
|
||||
assert.ok(
|
||||
/new Worker\(\s*pathToFileURL\([A-Za-z0-9_]+\)\s*,/.test(code),
|
||||
"ensureWorker must pass the URL instance to new Worker()"
|
||||
);
|
||||
assert.ok(
|
||||
!/new Worker\(\s*pathToFileURL\([A-Za-z0-9_]+\)\.href/.test(code),
|
||||
"ensureWorker must not pass pathToFileURL(...).href — that throws ERR_WORKER_PATH"
|
||||
);
|
||||
});
|
||||
Reference in New Issue
Block a user