Skip to content

feat(js): guardrail against executeSync() + custom builtin deadlock #1725

@chaliy

Description

@chaliy

Context

PR #1721 added JS support for custom builtins via Bash.addBuiltin() / customBuiltins. Callbacks are dispatched as Promise<String> over a NAPI threadsafe function.

If a script that invokes a custom builtin is run via executeSync(), the call silently deadlocks:

  • executeSync blocks the JS event loop on the synchronous NAPI call.
  • The custom builtin awaits a TSFN dispatch that requires the JS event loop to be free.
  • Nothing ever drains the queue, and the call hangs until the process is killed.

The README documents this caveat ("executeSync() will deadlock if your script invokes any custom builtin — use execute()") but there's no runtime check — users hit it before they read the docs.

Proposed fix

Mirror the existing OnOutputReentryScope / reject_on_output_reentry pattern from crates/bashkit-js/src/lib.rs:

  1. Add in_sync_execute_depth: Arc<AtomicUsize> to SharedState.
  2. Add a SyncExecuteScope RAII guard that increments on entry to execute_sync (on both Bash and BashTool) and decrements on drop.
  3. Pass an Arc<AtomicUsize> clone into JsCustomBuiltinAdapter at add_builtin time.
  4. In JsCustomBuiltinAdapter::execute, if the depth is non-zero, return ExecResult::err(..., 1) with a clear message instead of awaiting the TSFN.

Failure mode becomes: exit code 1 + stderr "custom builtins require execute() (async). executeSync() would deadlock because the JS event loop is blocked while the synchronous call is in flight" — surfaced as a normal script error, not a hang.

Optional follow-ups

  • Print a console warning at addBuiltin / customBuiltins time if executeSync was already used on this instance (encourages migration without breaking).
  • Detect cross-instance deadlock (instance A in executeSync, instance B's builtin fires) — process-wide flag on the JS thread rather than per-instance. Lower priority: rare in practice, requires more infrastructure.
  • Add a test in crates/bashkit-js/__test__/custom-builtins.spec.ts asserting the error result rather than a hang.

References

  • PR feat(core,js): host-owned mutable BuiltinRegistry #1721 — JS custom builtins
  • crates/bashkit-js/src/lib.rsOnOutputReentryScope, reject_on_output_reentry (existing pattern to mirror)
  • crates/bashkit-js/src/lib.rsJsCustomBuiltinAdapter::execute (where the check goes)
  • crates/bashkit-js/README.md — the documented caveat that this guardrail upgrades to a real error

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions