Skip to content

fix(oxfmt): avoid embedded TSFN crash by returning errors as data#19742

Closed
Boshen wants to merge 2 commits intomainfrom
fix/oxfmt-embedded-error-data
Closed

fix(oxfmt): avoid embedded TSFN crash by returning errors as data#19742
Boshen wants to merge 2 commits intomainfrom
fix/oxfmt-embedded-error-data

Conversation

@Boshen
Copy link
Member

@Boshen Boshen commented Feb 26, 2026

Summary

This fixes a crash in oxfmt NAPI CLI when formatting large repos with embedded formatting enabled.

Root cause: embedded formatter errors were propagated as rejected JS promises, which become napi::Error values in Rust TSFN await paths. In heavily concurrent runs, dropping those error values could reach napi_reference_unref during teardown and trigger V8 fatal checks.

Fix:

  • Change embedded callback contract from Promise<string> to Promise<{ ok: boolean; code?: string; error?: string }>.
  • Add formatEmbeddedCodeSafe() in JS that never rejects and encodes errors as data.
  • Parse the wrapped embedded result in Rust (parse_embedded_callback_result) and keep existing fallback behavior.
  • Wire CLI worker-proxy and JS API to use the safe embedded callback path.
  • Remove the temporary waitForImmediate workaround in CLI exit path.

Details

  • JsFormatEmbeddedCb now returns Promise<Value>.
  • Embedded callback result parser accepts:
    • { ok: true, code: string }
    • { ok: false, error: string }
    • legacy plain string (backward-compatible fallback)
  • NAPI TS arg types and generated bindings.d.ts updated accordingly.

Validation

Executed locally on Node v24.12.0:

  • pnpm -C apps/oxfmt run build-dev
  • Repro loop in /Users/boshen/github/linear-app:
    • node /Users/boshen/oxc/oxc4/apps/oxfmt/dist/cli.js --check => PASS 80/80
    • same command => PASS 200/200
    • node /Users/boshen/oxc/oxc4/apps/oxfmt/dist/cli.js --check --ignore-path /tmp/oxfmt-js-only.ignore => PASS 40/40

AI Usage Disclosure

This PR was prepared with AI assistance (Codex) for investigation, implementation, and stress-test scripting. Changes were reviewed and validated in local runs before submission.

Fixes #19713

Copilot AI review requested due to automatic review settings February 26, 2026 08:41
@github-actions github-actions bot added A-cli Area - CLI A-formatter Area - Formatter C-bug Category - Bug labels Feb 26, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes an intermittent Node/V8 crash in oxfmt’s NAPI-based CLI when embedded formatting is enabled under high concurrency by preventing embedded-formatter failures from surfacing as rejected promises in TSFN await paths.

Changes:

  • Updates the embedded formatter callback contract to return a resolved “result object” ({ ok, code?/error? }) instead of rejecting.
  • Adds a JS formatEmbeddedCodeSafe() wrapper and wires both JS API and CLI worker-proxy to use the safe embedded callback path.
  • Updates Rust to accept Promise<Value> for embedded formatting and parses the wrapped result, and removes the CLI exit delay workaround.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
apps/oxfmt/src/main_napi.rs Updates NAPI TS argument types for embedded formatter callback to the new wrapped-result contract.
apps/oxfmt/src/core/external_formatter.rs Changes embedded TSFN return type to Promise<Value> and adds parsing for wrapped embedded results (with legacy string fallback).
apps/oxfmt/src-js/libs/apis.ts Introduces FormatEmbeddedCodeResult + formatEmbeddedCodeSafe() that never rejects and encodes errors as data.
apps/oxfmt/src-js/index.ts Switches the public JS API to pass the safe embedded formatter callback to NAPI.
apps/oxfmt/src-js/cli/worker-proxy.ts Wraps embedded worker formatting so it never rejects; improves error-object reconstruction handling.
apps/oxfmt/src-js/cli.ts Removes the Node-version-based exit delay workaround.
apps/oxfmt/src-js/bindings.d.ts Updates generated TS types for the embedded formatter callback signature.

Comment on lines +145 to +147
#[napi(
ts_arg_type = "(options: Record<string, any>, code: string) => Promise<{ ok: boolean; code?: string; error?: string }>"
)]
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: the Rust parser accepts legacy string results, but the generated TS arg type only permits the wrapped object. Consider widening this callback type to Promise<string | { ok: boolean; code?: string; error?: string }> (or remove the legacy parsing path if not supported).

Copilot uses AI. Check for mistakes.
init_external_formatter_cb: JsInitExternalFormatterCb,
#[napi(ts_arg_type = "(options: Record<string, any>, code: string) => Promise<string>")]
#[napi(
ts_arg_type = "(options: Record<string, any>, code: string) => Promise<{ ok: boolean; code?: string; error?: string }>"
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: this callback ts_arg_type doesn’t reflect that Rust still accepts a legacy string response. Either widen the TS type to include string or drop the legacy support so type-level and runtime contracts match.

Suggested change
ts_arg_type = "(options: Record<string, any>, code: string) => Promise<{ ok: boolean; code?: string; error?: string }>"
ts_arg_type = "(options: Record<string, any>, code: string) => Promise<string | { ok: boolean; code?: string; error?: string }>"

Copilot uses AI. Check for mistakes.
init_external_formatter_cb: JsInitExternalFormatterCb,
#[napi(ts_arg_type = "(options: Record<string, any>, code: string) => Promise<string>")]
#[napi(
ts_arg_type = "(options: Record<string, any>, code: string) => Promise<{ ok: boolean; code?: string; error?: string }>"
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Rust side now accepts a legacy plain string result for backward compatibility (parse_embedded_callback_result), but this ts_arg_type only allows the wrapped { ok, code?, error? } object. If backward compatibility is intended, update the TypeScript callback type to include Promise<string | { ok: boolean; code?: string; error?: string }>; otherwise consider removing the legacy parsing path to keep runtime and types aligned.

Suggested change
ts_arg_type = "(options: Record<string, any>, code: string) => Promise<{ ok: boolean; code?: string; error?: string }>"
ts_arg_type = "(options: Record<string, any>, code: string) => Promise<string | { ok: boolean; code?: string; error?: string }>"

Copilot uses AI. Check for mistakes.
@Boshen Boshen force-pushed the fix/oxfmt-embedded-error-data branch from 6234a4d to df05046 Compare February 26, 2026 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-cli Area - CLI A-formatter Area - Formatter C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

oxfmt fails with segmentation error sometimes

3 participants