fix(workers): replaced file-URL import pattern with compiled-binary-aware spawn
- Replaced `with { type: "file" }` worker imports with `isCompiledBinary()` hybrid: literal string for `--compile` static analysis, `new URL(import.meta.url)` for dev portability.
- Added worker entrypoints as explicit `--compile` args in `build-binary.ts` so Bun emits them into bunfs.
- Added `smokeTestSyncWorker` and `omp --smoke-test` to catch silent worker-load failures in compiled binaries (fixes #1011, #1027).
- Added `isCompiledBinary()` utility to `@oh-my-pi/pi-utils` detecting bunfs path markers.
This commit is contained in:
can1357
@@ -29,7 +29,16 @@ This repo contains multiple packages, but **`packages/coding-agent/`** is the pr
|
||||
- **Class privacy**: use ES `#private` fields; leave externally accessible members bare. **No `private`/`protected`/`public` keyword on fields or methods**, except on **constructor parameter properties** where TypeScript requires it (e.g. `constructor(private readonly session: ToolSession)`).
|
||||
- **Promises**: use `Promise.withResolvers()` instead of `new Promise((resolve, reject) => ...)`.
|
||||
- **Prompts**: never build prompts in code (no inline strings, template literals, or concatenation). Prompts live in static `.md` files; use Handlebars for dynamic content. Import them via `import content from "./prompt.md" with { type: "text" }` — not `readFile`.
|
||||
- **Worker scripts**: reference the entry via `import workerUrl from "./worker.ts" with { type: "file" }` so the path survives bundling. tsgo flags it (TS1192/TS5097) — suppress with `// @ts-expect-error -- Bun file-URL import` directly above the line (see `tab-supervisor.ts`, `context-manager.ts`, `aggregator.ts`).
|
||||
- **Worker scripts**: spawn workers with the dev/compile-safe hybrid pattern. `with { type: "file" }` only copies the entry as a raw asset and does **not** bundle its imports — workers crashed silently in compiled binaries on every prior incarnation of that pattern (issues #1011, #1027). Use this shape instead:
|
||||
```ts
|
||||
import { isCompiledBinary } from "@oh-my-pi/pi-utils";
|
||||
const worker = isCompiledBinary()
|
||||
? new Worker("./packages/<pkg>/src/<worker>.ts", { type: "module" })
|
||||
: new Worker(new URL("./<worker>.ts", import.meta.url).href, { type: "module" });
|
||||
```
|
||||
The literal in the compiled branch is what Bun's `--compile` static analyzer needs to discover the worker — its path is **`--root`-relative** (repo root, since `build-binary.ts` passes `--root ../..`), so it must start with `./packages/...`. The `new URL` form in the dev branch keeps spawns portable across cwds.
|
||||
In addition, every worker entry **MUST** be listed as an extra `--compile` entrypoint in `packages/coding-agent/scripts/build-binary.ts`. Without that the analyzer sees the literal but the worker never gets emitted into bunfs. The three current entries (`sync-worker.ts`, `tab-worker-entry.ts`, `worker-entry.ts`) live there as the working reference.
|
||||
Validate any new worker with the dedicated smoke probe: `omp --smoke-test` spawns the stats sync worker, pings it, and exits — it's wired into `ci:test:smoke` and `scripts/install-tests/run-ci.sh` so binary, source-link, and tarball installs all exercise it. Add a sibling smoke if the new worker is on a different module graph.
|
||||
|
||||
## Bun Over Node
|
||||
|
||||
|
||||
+1
-1
@@ -109,7 +109,7 @@
|
||||
"ci:check:full": "bun run check:ts",
|
||||
"ci:build:native": "bun scripts/ci-build-native.ts",
|
||||
"ci:test:full": "bun run test",
|
||||
"ci:test:smoke": "bun packages/coding-agent/src/cli.ts --version && bun packages/coding-agent/src/cli.ts --help && bun packages/coding-agent/src/cli.ts stats --help",
|
||||
"ci:test:smoke": "bun packages/coding-agent/src/cli.ts --version && bun packages/coding-agent/src/cli.ts --help && bun packages/coding-agent/src/cli.ts stats --help && bun packages/coding-agent/src/cli.ts --smoke-test",
|
||||
"ci:test:install-methods": "bash scripts/install-tests/run-ci.sh",
|
||||
"ci:release:build-binaries": "bun scripts/ci-release-build-binaries.ts",
|
||||
"ci:release:publish": "bun scripts/ci-release-publish.ts",
|
||||
|
||||
@@ -40,6 +40,17 @@ async function main(): Promise<void> {
|
||||
"--root",
|
||||
"../..",
|
||||
"./src/cli.ts",
|
||||
// Worker entrypoints. Bun's `--compile` discovers the literal in
|
||||
// `new Worker("…", …)` at each spawn site, but only actually
|
||||
// emits the worker into the bunfs root when it is listed here as
|
||||
// an explicit additional entry. Paths are relative to this
|
||||
// script's cwd (packages/coding-agent) and the `--root` above
|
||||
// (../..) makes them appear inside the binary at
|
||||
// `/$bunfs/root/packages/<pkg>/src/<worker>.js`, which is
|
||||
// exactly what the literals at the spawn sites resolve to.
|
||||
"../stats/src/sync-worker.ts",
|
||||
"./src/tools/browser/tab-worker-entry.ts",
|
||||
"./src/eval/js/worker-entry.ts",
|
||||
"--outfile",
|
||||
"dist/omp",
|
||||
],
|
||||
|
||||
@@ -84,8 +84,30 @@ function isSubcommand(first: string | undefined): boolean {
|
||||
return commands.some(e => e.name === first || e.aliases?.includes(first));
|
||||
}
|
||||
|
||||
/**
|
||||
* Smoke-test entry. Spawns the stats sync worker, pings it, exits.
|
||||
*
|
||||
* Purpose: catch the silent worker-load regressions that hit compiled
|
||||
* binaries (issues #1011 and #1027). Neither `--version` nor
|
||||
* `stats --summary` actually spawns a Worker on a fresh install — the
|
||||
* sync path early-returns when no session files exist. This probe is the
|
||||
* minimal end-to-end test that proves `new Worker(...)` resolves and the
|
||||
* bundled worker module evaluates successfully. Wired into
|
||||
* `scripts/install-tests/run-ci.sh` so binary / source-link / tarball
|
||||
* installs all exercise it on every CI run.
|
||||
*/
|
||||
async function runSmokeTest(): Promise<void> {
|
||||
const { smokeTestSyncWorker } = await import("@oh-my-pi/omp-stats");
|
||||
await smokeTestSyncWorker();
|
||||
process.stdout.write("smoke-test: ok\n");
|
||||
}
|
||||
|
||||
/** Run the CLI with the given argv (no `process.argv` prefix). */
|
||||
export function runCli(argv: string[]): Promise<void> {
|
||||
export async function runCli(argv: string[]): Promise<void> {
|
||||
if (argv[0] === "--smoke-test") {
|
||||
await runSmokeTest();
|
||||
return;
|
||||
}
|
||||
// --help and --version are handled by run() directly, don't rewrite those.
|
||||
// Everything else that isn't a known subcommand routes to "launch".
|
||||
const first = argv[0];
|
||||
|
||||
@@ -1,14 +1,13 @@
|
||||
import { logger, Snowflake } from "@oh-my-pi/pi-utils";
|
||||
import { isCompiledBinary, logger, Snowflake } from "@oh-my-pi/pi-utils";
|
||||
import type { ToolSession } from "../../tools";
|
||||
import { ToolAbortError, ToolError } from "../../tools/tool-errors";
|
||||
import { callSessionTool, type JsStatusEvent } from "./tool-bridge";
|
||||
import { WorkerCore } from "./worker-core";
|
||||
// Imported with `type: "file"` so Bun's bundler statically discovers the worker entry and
|
||||
// embeds it inside `bun build --compile` single-file binaries. Mirrors the browser tab
|
||||
// worker setup; see packages/coding-agent/src/tools/browser/tab-supervisor.ts for the
|
||||
// rationale.
|
||||
// @ts-expect-error -- Bun file-URL import attribute is not modeled by tsgo.
|
||||
import jsWorkerEntryUrl from "./worker-entry.ts" with { type: "file" };
|
||||
// Worker entry. See `tab-supervisor.ts` for the rationale behind the
|
||||
// literal-string + `new URL(import.meta.url)` hybrid: the literal is what
|
||||
// Bun's `--compile` bundler discovers, the `new URL` form is what makes dev
|
||||
// runs portable across cwds. The worker is registered as an additional
|
||||
// `--compile` entrypoint in `scripts/build-binary.ts`.
|
||||
import type {
|
||||
JsDisplayOutput,
|
||||
RunErrorPayload,
|
||||
@@ -344,7 +343,9 @@ async function raceWithTimeout<T>(promise: Promise<T>, timeoutMs: number, reason
|
||||
|
||||
async function spawnJsWorker(): Promise<WorkerHandle> {
|
||||
try {
|
||||
const worker = new Worker(jsWorkerEntryUrl, { type: "module" });
|
||||
const worker = isCompiledBinary()
|
||||
? new Worker("./packages/coding-agent/src/eval/js/worker-entry.ts", { type: "module" })
|
||||
: new Worker(new URL("./worker-entry.ts", import.meta.url).href, { type: "module" });
|
||||
return wrapBunWorker(worker);
|
||||
} catch (err) {
|
||||
logger.warn("Bun Worker spawn failed; using inline JS eval worker (no sync-loop guard)", {
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { getPuppeteerDir, logger, Snowflake } from "@oh-my-pi/pi-utils";
|
||||
import { getPuppeteerDir, isCompiledBinary, logger, Snowflake } from "@oh-my-pi/pi-utils";
|
||||
import type { Page, Target } from "puppeteer-core";
|
||||
import { callSessionTool } from "../../eval/js/tool-bridge";
|
||||
import type { ToolSession } from "../../sdk";
|
||||
@@ -17,17 +17,15 @@ import type {
|
||||
WorkerInitPayload,
|
||||
WorkerOutbound,
|
||||
} from "./tab-protocol";
|
||||
// Imported with `type: "file"` so Bun's bundler statically discovers the
|
||||
// worker entry and embeds it inside `bun build --compile` single-file
|
||||
// binaries. Without this attribute the bundler cannot reach the entry through
|
||||
// a `new URL(..., import.meta.url)` literal stored in a local variable, and
|
||||
// the prebuilt binary surfaces `Timed out initializing browser tab worker`
|
||||
// (issue #1011) because `/$bunfs/root/tab-worker-entry.ts` is missing.
|
||||
// tsgo doesn't recognize Bun's `with { type: "file" }` attribute and treats
|
||||
// this as a normal TS source import, raising TS1192/TS5097. Bun's bundler
|
||||
// (and runtime) honors the attribute and returns the embedded file URL.
|
||||
// @ts-expect-error -- Bun file-URL import (see comment above).
|
||||
import tabWorkerEntryUrl from "./tab-worker-entry.ts" with { type: "file" };
|
||||
|
||||
// Worker entry. The literal string in `new Worker("./packages/coding-agent/src/tools/browser/tab-worker-entry.ts", …)`
|
||||
// below is what Bun's `--compile` static analyzer needs to bundle the worker
|
||||
// (registered as an additional entrypoint in `scripts/build-binary.ts`); in
|
||||
// dev we resolve the same source via `import.meta.url`. Replaces the older
|
||||
// `with { type: "file" }` pattern, which only copied the entry as a raw
|
||||
// asset and could not resolve the worker's relative imports inside a
|
||||
// compiled binary (issue #1011 was a false-positive fix — the regression
|
||||
// test only checked emission, not actual worker startup).
|
||||
|
||||
interface WorkerHandle {
|
||||
send(msg: WorkerInbound, transferList?: Transferable[]): void;
|
||||
@@ -456,7 +454,9 @@ async function raceWithTimeout<T>(
|
||||
|
||||
async function spawnTabWorker(): Promise<WorkerHandle> {
|
||||
try {
|
||||
const worker = new Worker(tabWorkerEntryUrl, { type: "module" });
|
||||
const worker = isCompiledBinary()
|
||||
? new Worker("./packages/coding-agent/src/tools/browser/tab-worker-entry.ts", { type: "module" })
|
||||
: new Worker(new URL("./tab-worker-entry.ts", import.meta.url).href, { type: "module" });
|
||||
return wrapBunWorker(worker);
|
||||
} catch (err) {
|
||||
logger.warn("Bun Worker spawn failed; using inline tab worker (no sync-loop guard)", {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import * as fs from "node:fs";
|
||||
import { isCompiledBinary } from "@oh-my-pi/pi-utils";
|
||||
import {
|
||||
getRecentErrors as dbGetRecentErrors,
|
||||
getRecentRequests as dbGetRecentRequests,
|
||||
@@ -23,13 +24,15 @@ import {
|
||||
} from "./db";
|
||||
import { getSessionEntry, listAllSessionFiles, type ParseSessionResult } from "./parser";
|
||||
import type { SyncWorkerRequest, SyncWorkerResponse } from "./sync-worker";
|
||||
// `with { type: "file" }` resolves to the worker's absolute path at runtime
|
||||
// (dev) and survives bundling (the asset is copied alongside the build).
|
||||
// tsgo doesn't recognize Bun's file-URL import attribute and would raise
|
||||
// TS1192/TS5097 here; Bun honors it. Same suppression pattern lives in
|
||||
// `tab-supervisor.ts` and `context-manager.ts`.
|
||||
// @ts-expect-error -- Bun file-URL import (see comment above).
|
||||
import syncWorkerUrl from "./sync-worker.ts" with { type: "file" };
|
||||
// Worker entry. Bun's `--compile` bundler statically discovers the string
|
||||
// literal in `new Worker("./packages/stats/src/sync-worker.ts", …)` below and
|
||||
// emits the worker as an additional entrypoint (registered in
|
||||
// `packages/coding-agent/scripts/build-binary.ts`). In dev runs we resolve
|
||||
// the same source file through `import.meta.url`, so the literal only has to
|
||||
// be valid relative to the `--root` directory (repo root). Importing the
|
||||
// source as `with { type: "file" }` is NOT sufficient — that copies the file
|
||||
// as a raw asset and does not bundle the worker's relative imports, so the
|
||||
// worker would crash on first `import` (issue #1011, PR #1027).
|
||||
import type { BehaviorDashboardStats, DashboardStats, MessageStats, RequestDetails } from "./types";
|
||||
|
||||
/**
|
||||
@@ -85,8 +88,22 @@ interface WorkerHandle {
|
||||
reject: ((err: Error) => void) | null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Create a fresh sync worker. In a `--compile` binary the literal-string
|
||||
* specifier is what Bun's static analyzer needs (the file is also listed as
|
||||
* an additional `--compile` entrypoint in
|
||||
* `packages/coding-agent/scripts/build-binary.ts`). In dev runs we resolve
|
||||
* the source URL via `import.meta.url` so the worker survives `cwd` changes
|
||||
* by callers.
|
||||
*/
|
||||
function createSyncWorker(): Worker {
|
||||
return isCompiledBinary()
|
||||
? new Worker("./packages/stats/src/sync-worker.ts", { type: "module" })
|
||||
: new Worker(new URL("./sync-worker.ts", import.meta.url).href, { type: "module" });
|
||||
}
|
||||
|
||||
function spawnWorker(): WorkerHandle {
|
||||
const worker = new Worker(syncWorkerUrl, { type: "module" });
|
||||
const worker = createSyncWorker();
|
||||
const handle: WorkerHandle = { worker, busy: false, resolve: null, reject: null };
|
||||
worker.onmessage = (event: MessageEvent<SyncWorkerResponse>) => {
|
||||
const { resolve, reject } = handle;
|
||||
@@ -94,8 +111,16 @@ function spawnWorker(): WorkerHandle {
|
||||
handle.reject = null;
|
||||
handle.busy = false;
|
||||
if (!resolve || !reject) return;
|
||||
if (event.data.ok) resolve(event.data.result);
|
||||
else reject(new Error(event.data.error));
|
||||
const data = event.data;
|
||||
if (!data.ok) {
|
||||
reject(new Error(data.error));
|
||||
return;
|
||||
}
|
||||
if (data.kind === "pong") {
|
||||
reject(new Error("sync worker: unexpected pong on parse channel"));
|
||||
return;
|
||||
}
|
||||
resolve(data.result);
|
||||
};
|
||||
worker.onerror = (event: ErrorEvent) => {
|
||||
const { reject } = handle;
|
||||
@@ -119,6 +144,45 @@ function dispatch(handle: WorkerHandle, request: SyncWorkerRequest): Promise<Par
|
||||
return promise;
|
||||
}
|
||||
|
||||
/**
|
||||
* Smoke test: spawns one sync worker, pings it, asserts the pong response,
|
||||
* then terminates. Used by `omp --smoke-test` so the install-method CI jobs
|
||||
* catch the silent worker-load failure that hit compiled binaries in #1011
|
||||
* and #1027 — neither `--version` nor `stats --summary` exercises the worker
|
||||
* spawn path on a fresh install (no session files = early return), so a
|
||||
* dedicated probe is the only reliable signal.
|
||||
*
|
||||
* Resolves with the worker's `import.meta.url` (caller-visible diagnostics);
|
||||
* rejects on transport error, error response, or timeout.
|
||||
*/
|
||||
export async function smokeTestSyncWorker({ timeoutMs = 5_000 }: { timeoutMs?: number } = {}): Promise<void> {
|
||||
const worker = createSyncWorker();
|
||||
const { promise, resolve, reject } = Promise.withResolvers<void>();
|
||||
const timer = setTimeout(() => reject(new Error(`sync worker did not pong within ${timeoutMs}ms`)), timeoutMs);
|
||||
worker.onmessage = (event: MessageEvent<SyncWorkerResponse>) => {
|
||||
const data = event.data;
|
||||
if (!data.ok) {
|
||||
reject(new Error(data.error));
|
||||
return;
|
||||
}
|
||||
if (data.kind !== "pong") {
|
||||
reject(new Error(`sync worker: expected pong, got ${JSON.stringify(data)}`));
|
||||
return;
|
||||
}
|
||||
resolve();
|
||||
};
|
||||
worker.onerror = (event: ErrorEvent) => {
|
||||
reject(event.error instanceof Error ? event.error : new Error(event.message || "worker error"));
|
||||
};
|
||||
try {
|
||||
worker.postMessage({ kind: "ping" } satisfies SyncWorkerRequest);
|
||||
await promise;
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
worker.terminate();
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Sync all session files to the database.
|
||||
*
|
||||
|
||||
@@ -11,6 +11,7 @@ export {
|
||||
getTotalMessageCount,
|
||||
type SyncOptions,
|
||||
type SyncProgress,
|
||||
smokeTestSyncWorker,
|
||||
syncAllSessions,
|
||||
} from "./aggregator";
|
||||
export { closeDb } from "./db";
|
||||
|
||||
@@ -4,25 +4,34 @@
|
||||
* `parseSessionFile` (which is pure I/O + CPU, no DB), and post the
|
||||
* structured-clone-safe result back. One in-flight request per worker so
|
||||
* the main thread can fan jobs out 1:1 with the pool size.
|
||||
*
|
||||
* A `{ kind: "ping" }` request is also accepted and replies with
|
||||
* `{ ok: true, kind: "pong" }` — used by `smokeTestSyncWorker` to prove the
|
||||
* worker actually spawns and runs in compiled binaries (regression coverage
|
||||
* for issue #1011 / PR #1027, where the worker silently failed to load).
|
||||
*/
|
||||
|
||||
import { type ParseSessionResult, parseSessionFile } from "./parser";
|
||||
|
||||
export interface SyncWorkerRequest {
|
||||
sessionFile: string;
|
||||
fromOffset: number;
|
||||
}
|
||||
export type SyncWorkerRequest = { kind?: "parse"; sessionFile: string; fromOffset: number } | { kind: "ping" };
|
||||
|
||||
export type SyncWorkerResponse = { ok: true; result: ParseSessionResult } | { ok: false; error: string };
|
||||
export type SyncWorkerResponse =
|
||||
| { ok: true; kind?: "parse"; result: ParseSessionResult }
|
||||
| { ok: true; kind: "pong" }
|
||||
| { ok: false; error: string };
|
||||
|
||||
declare const self: Worker & {
|
||||
onmessage: ((event: MessageEvent<SyncWorkerRequest>) => void) | null;
|
||||
};
|
||||
|
||||
self.onmessage = async event => {
|
||||
const { sessionFile, fromOffset } = event.data;
|
||||
const request = event.data;
|
||||
try {
|
||||
const result = await parseSessionFile(sessionFile, fromOffset);
|
||||
if (request.kind === "ping") {
|
||||
self.postMessage({ ok: true, kind: "pong" } satisfies SyncWorkerResponse);
|
||||
return;
|
||||
}
|
||||
const result = await parseSessionFile(request.sessionFile, request.fromOffset);
|
||||
self.postMessage({ ok: true, result } satisfies SyncWorkerResponse);
|
||||
} catch (err) {
|
||||
const error = err instanceof Error ? (err.stack ?? err.message) : String(err);
|
||||
|
||||
@@ -100,6 +100,20 @@ export function isBunTestRuntime(): boolean {
|
||||
return Bun.env.BUN_ENV === "test" || Bun.env.NODE_ENV === "test";
|
||||
}
|
||||
|
||||
/**
|
||||
* True when this code is running inside a `bun build --compile` standalone
|
||||
* binary. Detects via the embedded virtual-filesystem path markers
|
||||
* (`$bunfs`, `~BUN`, or its URL-encoded form `%7EBUN`) in `import.meta.url`,
|
||||
* which Bun rewrites for every module bundled into the executable. The
|
||||
* `PI_COMPILED` env var (set by the build script's `--define`) is checked
|
||||
* first for cheap fast-path detection.
|
||||
*/
|
||||
export function isCompiledBinary(): boolean {
|
||||
if (Bun.env.PI_COMPILED) return true;
|
||||
const url = import.meta.url;
|
||||
return url.includes("$bunfs") || url.includes("~BUN") || url.includes("%7EBUN");
|
||||
}
|
||||
|
||||
const TRUTHY: Dict<boolean> = { "1": true, Y: true, TRUE: true, YES: true, ON: true };
|
||||
export function $flag(name: string, def: boolean = false): boolean {
|
||||
const value = $env[name];
|
||||
|
||||
@@ -21,6 +21,13 @@ smoke_cli() {
|
||||
XDG_DATA_HOME="$runtime_dir/xdg" HOME="$runtime_dir/home" "$omp_bin" --version
|
||||
XDG_DATA_HOME="$runtime_dir/xdg" HOME="$runtime_dir/home" "$omp_bin" --help >/dev/null
|
||||
XDG_DATA_HOME="$runtime_dir/xdg" HOME="$runtime_dir/home" "$omp_bin" stats --summary >/dev/null
|
||||
# Spawns the stats sync worker via `new Worker(...)` and waits for a pong.
|
||||
# Regression probe for #1011 (browser tab worker) and #1027 (stats sync
|
||||
# worker) — both broke silently in compiled binaries because the `with
|
||||
# { type: "file" }` import pattern only copies the worker as a raw asset
|
||||
# without bundling its imports. `stats --summary` doesn't catch this on a
|
||||
# fresh install (no session files = no Worker spawn).
|
||||
XDG_DATA_HOME="$runtime_dir/xdg" HOME="$runtime_dir/home" "$omp_bin" --smoke-test
|
||||
}
|
||||
|
||||
find_tarball() {
|
||||
|
||||
Reference in New Issue
Block a user