fix: format embedded code in-process so the binary stops hanging on .vue#51
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a patch script, patch-oxfmt-inprocess.mjs, to rewrite oxfmt's CLI so that it formats embedded code in-process rather than using a Tinypool worker pool, resolving process hangs in bun-compiled binaries. The patch is integrated into the release staging pipeline, and a Vue SFC smoke test is added to prevent regressions. Feedback on the changes suggests making the shim functions asynchronous to properly await underlying API calls before passing them to synchronous helpers, and making the import alias parsing more robust to handle cases where as aliases are not used.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function formatFile(options, code) { | ||
| return toFormatFileResult(__fmtkitFormatFile({ options, code })); | ||
| } | ||
| function formatEmbeddedCode(options, code) { | ||
| return toNullable(__fmtkitFormatEmbeddedCode({ options, code })); | ||
| } | ||
| function formatEmbeddedDoc(options, texts) { | ||
| return toNullable(__fmtkitFormatEmbeddedDoc({ options, texts })); | ||
| } | ||
| function sortTailwindClasses(options, classes) { | ||
| return toNullable(__fmtkitSortTailwindClasses({ options, classes })); | ||
| } |
There was a problem hiding this comment.
The original pooled functions in worker-proxy.ts returned Promises because they were executed via Tinypool. If the underlying oxfmt API functions (like __fmtkitFormatFile) are asynchronous (e.g., if they call Prettier v3 APIs which are async), they will return Promises. Passing a Promise directly to synchronous helper functions like toFormatFileResult or toNullable will cause them to fail or behave incorrectly.
By making the shim functions async and using await on the inner API calls, we guarantee that:
- The helper functions always receive the resolved value (whether the API is sync or async).
- The shim functions always return a Promise, preserving the exact Promise-returning contract of the original pooled functions.
| function formatFile(options, code) { | |
| return toFormatFileResult(__fmtkitFormatFile({ options, code })); | |
| } | |
| function formatEmbeddedCode(options, code) { | |
| return toNullable(__fmtkitFormatEmbeddedCode({ options, code })); | |
| } | |
| function formatEmbeddedDoc(options, texts) { | |
| return toNullable(__fmtkitFormatEmbeddedDoc({ options, texts })); | |
| } | |
| function sortTailwindClasses(options, classes) { | |
| return toNullable(__fmtkitSortTailwindClasses({ options, classes })); | |
| } | |
| async function formatFile(options, code) { | |
| return toFormatFileResult(await __fmtkitFormatFile({ options, code })); | |
| } | |
| async function formatEmbeddedCode(options, code) { | |
| return toNullable(await __fmtkitFormatEmbeddedCode({ options, code })); | |
| } | |
| async function formatEmbeddedDoc(options, texts) { | |
| return toNullable(await __fmtkitFormatEmbeddedDoc({ options, texts })); | |
| } | |
| async function sortTailwindClasses(options, classes) { | |
| return toNullable(await __fmtkitSortTailwindClasses({ options, classes })); | |
| } |
There was a problem hiding this comment.
Thanks — I checked this against the real helpers, and it's the opposite way round: toFormatFileResult and toNullable are not synchronous. They're Promise wrappers (oxfmt/dist/napi-callbacks-*.js):
async function toFormatFileResult(run) {
return run.then((code) => ({ ok: true, code })).catch((err) => ({ ok: false, error: errorToMessage(err) }));
}
async function toNullable(run) {
return run.catch(() => null);
}They call run.then(...) / run.catch(...), so they need the Promise itself. Awaiting first hands them a resolved string, which has no .then. Running both variants against those exact helpers:
A (this PR: pass the Promise) ok -> { ok: true, code: 'formatted:x' }
A err -> { ok: false, error: 'Error: boom' }
A toNullable -> null
B (suggested: await first) ok -> THROWS: TypeError: run.then is not a function
B err -> THROWS (rejection escapes): boom
So the suggestion breaks it twice over: the immediate TypeError, and — even were that fixed — rejections would escape as exceptions instead of becoming { ok: false, ... } / null, which is the fallback behaviour the Rust caller relies on.
The premise is right that the API functions are async; that's exactly why passing the Promise through is correct. pool.run(...) returned a Promise before, and an async API call returns a Promise now — the contract is preserved by construction, without needing async on the shim.
Covered end-to-end by the Vue fixture in test-binary-smoke.sh: the <style>/<template> blocks only format if this path works. Leaving as-is, with the reasoning documented on ShimSource.workerProxyRegion() so the next reader doesn't have to re-derive it.
The self-contained binary (bun build --compile) hung forever on any file with embedded code — .vue (<template>/<style>), markdown, HTML — while plain .ts/.tsx were unaffected. Regression from #48 (the Docker→binary switch); the Docker image formatted these fine. Root cause: oxfmt formats embedded blocks through a Tinypool `runtime: "child_process"` pool. The pool forks worker entry scripts (oxfmt/dist/cli-worker.js, tinypool/dist/entry/process.js) that only exist as virtual /$bunfs/root/ paths inside the compiled binary and are never carved out as loadable files, so every worker dies on startup ("Worker exited unexpectedly") and Tinypool.destroy() then awaits an exit event that never fires — the process wedges until SIGKILL. Plain .ts never initialises the pool, which is why it slipped through: the smoke fixture had no embedded-code file. The four pooled functions (formatFile / formatEmbeddedCode / formatEmbeddedDoc / sortTailwindClasses) are stateless prettier calls; the pool only bought cross-file parallelism, not isolation. Rewrite oxfmt's worker-proxy at release-staging time to call them directly on the main thread, dropping the unusable worker layer. Behaviour is identical (the embedded-code fraction loses parallelism but also sheds the per-worker prettier startup cost). - infra/scripts/release/patch-oxfmt-inprocess.mjs: discovers oxfmt's hashed API module + export aliases, asserts the exact worker-proxy structure, and swaps the Tinypool block for in-process calls. Exits non-zero on any drift so an oxfmt bump can't silently reintroduce the hang. - stage-ts-assets.sh: run the patch after npm install, before bun --compile (once; all four targets share the patched workdir). - test-binary-smoke.sh: add a Vue SFC fixture exercising all three sections, so the embedded path is covered and a future hang fails the smoke test. Verified: rebuilt the host binary; `fmtkit ts <sfc>.vue` now exits 0 in ~2s with <script>/<template>/<style> all formatted; edgekit's 21 .vue files no longer hang; all four release targets cross-compile; go test ./... and the smoke test pass.
8cf6198 to
f58cda6
Compare
Replaces the single patch-oxfmt-inprocess.mjs with a TypeScript concern slice,
per the repository TypeScript standards (class-based, organised by concern,
behaviour behind narrow interfaces, wired by constructor injection).
infra/scripts/release/oxfmt-inprocess/
result.ts Result vocabulary (stays functional — it is a value type)
errors.ts tagged errors, each carrying the anchor/path it looked for
text-files.ts TextFiles port + NodeTextFiles adapter (the only I/O)
api-bindings.ts ApiBindings DTO: private ctor, static parse() -> Result,
readonly getters
shim-source.ts static-only renderer for the generated shim source
cli-patcher.ts OxfmtCliPatcher orchestrator: injected TextFiles, guards,
rewrite
index.ts barrel
patch-oxfmt-inprocess.ts is now a thin composition root: parse argv, construct
the adapter, call the service, report. Expected failures are values
(Result<PatchOutcome, OxfmtPatchError>) rather than process.exit() scattered
through the logic, so every drift in oxfmt's internals reports what it was
looking for.
Imports go through a #oxfmt-inprocess/* subpath map (new
infra/scripts/release/package.json) rather than ./ or ../ specifiers, mirroring
packages/devx/scripts/package.json. node runs the entrypoint directly via type
stripping, so staging still needs only node/npm/bun — no tsx.
Review feedback addressed: binding parsing now accepts a bare `localName` as
well as `exported as localName`, so a bundler that stops aliasing the API
re-exports no longer fails the patch.
Verified: tsc --strict (plus noUncheckedIndexedAccess,
exactOptionalPropertyTypes, noImplicitOverride, noFallthroughCasesInSwitch)
clean; `pnpm -C packages/devx check` (blank-lines + oxlint + oxfmt over all 59
tracked files) clean; go test ./... and test-binary-smoke.sh (including the Vue
fixture) pass; the staged binary still formats .vue in ~2s without hanging.
The bug
The self-contained binary hangs forever (needs
SIGKILL) on any file with embedded code —.vue(<template>/<style>), markdown, HTML. Plain.ts/.tsxare unaffected.Reproduced with a 6-line SFC in a fresh git repo, so it is not project-specific:
Regression from #48 (Docker → single binary). The Docker image formatted these same files fine, so this is v0.5.x-only.
Root cause
.vuemakes oxfmt use its external/embedded formatter, a Tinypool pool withruntime: "child_process"(oxfmt/dist/cli.js). That pool forks worker entry scripts:oxfmt/dist/cli-worker.js— referenced vianew URL("./cli-worker.js", import.meta.url)tinypool/dist/entry/process.js— forked viafileURLToPath(import.meta.url + "/../entry/process.js")Both are reached through runtime URL strings, so
bun build --compilenever traces or embeds them — they resolve to non-existent/$bunfs/root/…paths. Every forked worker dies on startup → Tinypool emitsWorker exited unexpectedly→Tinypool.destroy()awaits anonce(worker, 'exit')that never settles → the process wedges. (fork()also re-execs the compiled binary itself, which compounds it.)The misleading
Expected at least one target file…line is oxfmt's native diagnostic for thread-pool spawn failure, printed once per dead worker — not an ignore-rules problem.Plain
.tsnever initialises the pool, and the smoke fixture had no embedded-code file, which is why this shipped.The fix
The four pooled functions (
formatFile,formatEmbeddedCode,formatEmbeddedDoc,sortTailwindClasses) are stateless prettier calls — the pool only bought cross-file parallelism, not isolation. So instead of wrestling worker entries into the bundle, this rewrites oxfmt'sworker-proxyat release-staging time to call them directly on the main thread, deleting the unusable worker layer.infra/scripts/release/patch-oxfmt-inprocess.mjs(new) — discovers oxfmt's hashed API module + export aliases fromcli-worker.js, asserts the exact worker-proxy structure, and swaps the Tinypool block for in-process calls. Exits non-zero on any drift, so an oxfmt bump can't silently reintroduce the hang.stage-ts-assets.sh— runs the patch afternpm install, beforebun --compile(once; all four targets share the patched workdir).test-binary-smoke.sh— adds a Vue SFC fixture exercising all three sections, closing the coverage gap that let this ship.Same function names and Promise-returning shape as the originals, so the rest of
cli.js(therunCliwiring, thedisposeExternalFormattercall) is untouched.Trade-off
Embedded-code formatting is now single-threaded within each oxfmt process. For typical repos this is a wash or faster — it also sheds the per-worker prettier startup cost — but a very large Vue/markdown codebase loses that parallelism. A proper
worker_threads-based pool could restore it later if profiling calls for it; correctness first, since the current state is "hangs forever".Verification
fmtkit ts <sfc>.vue→ exit 0 in ~2s,<script>/<template>/<style>all formatted (the<style>CSS reformatting proves the embedded path runs)..vuefiles → no hang, exit 0.darwin_*,linux_*) cross-compile.go test ./...green;test-binary-smoke.shgreen (now including the Vue fixture).Sidecar grows ~2.2MB (70.6 → 72.8MB darwin_arm64): the prettier/embedded-language stack is now genuinely reachable and bundled — previously it was neither embedded nor loadable, which is the other half of why the worker path could never have worked.