Skip to content

W-23459244 fix(node-binding): resolve teardown crash, streaming race, and swallowed read errors#121

Merged
mlischetti merged 1 commit into
masterfrom
fix/node-binding-ffi-crash-race
Jul 15, 2026
Merged

W-23459244 fix(node-binding): resolve teardown crash, streaming race, and swallowed read errors#121
mlischetti merged 1 commit into
masterfrom
fix/node-binding-ffi-crash-race

Conversation

@mlischetti

@mlischetti mlischetti commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Extracted from #119 so the fixes to the pre-existing Node binding can be reviewed and merged independently of the new C/Go/Rust bindings.

Changes

native-lib/node/src/addon.c

  • Teardown crash: graal_tear_down_isolate() must be called on a thread attached to the isolate. The bootstrap thread created by graal_create_isolate() is joined and exits, so its IsolateThread is invalid at cleanup
    — passing it faults, and leaving it attached blocks teardown forever waiting for a dead thread to reach a safepoint. Now the bootstrap thread is detached after create, and the cleanup thread attaches to the isolate to obtain
    a valid local IsolateThread before tearing down.
  • Swallowed read errors: when the JS read callback throws, surface the pending exception (message + stack) to stderr and signal an error (bytes_read = -1) instead of silently clearing it and reporting 0 bytes.

native-lib/node/src/index.ts

  • Streaming race: replace the single resolveChunk slot with a pendingResolves queue so every waiting consumer is woken (the single slot dropped all but the last waiter); metadata resolution drains the whole queue.
    • Memory: release consumed pre-buffered input buffers so memory is freed as async input is drained.

Notes

  • Branches off master; touches only the pre-existing Node binding — no dependency on the new bindings in Native-bindings #119.
  • Repro/regression tests intentionally left in Native-bindings #119 per the extraction scope (source fixes only).

…wed read errors

Extracted from the native-bindings work in PR #119 so the fixes to the
pre-existing Node binding can be reviewed and merged independently of the
new C/Go/Rust bindings.

addon.c:
- Fix a teardown crash: graal_tear_down_isolate() must run on a thread
  attached to the isolate. The bootstrap thread from graal_create_isolate()
  is joined and exits, so its IsolateThread is invalid at cleanup and also
  leaves a phantom attached thread that blocks teardown forever. Detach the
  bootstrap thread after create, and attach the cleanup thread before tear
  down.
- Surface the pending JS read-callback exception (message + stack) to stderr
  and signal an error (bytes_read = -1) instead of silently swallowing it.

index.ts:
- Fix a streaming race: replace the single resolveChunk slot with a
  pendingResolves queue so multiple waiting consumers are all woken; the
  metadata resolution drains the whole queue.
- Release consumed pre-buffered input buffers so memory is freed as the
  async input is drained.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mlischetti
mlischetti requested a review from a team as a code owner July 15, 2026 16:05
@mlischetti
mlischetti merged commit 7464094 into master Jul 15, 2026
3 checks passed
@mlischetti
mlischetti deleted the fix/node-binding-ffi-crash-race branch July 15, 2026 17:29
mlischetti added a commit that referenced this pull request Jul 16, 2026
The native-lib/node/repro and native-lib/python/repro folders are regression
guards for the pre-existing Node and Python bindings, covering fixes that
already merged to master independently (PR #121 read-callback exception, PR
#122 streaming queue backpressure). They are unrelated to the new C/Go/Rust
bindings this branch introduces, are not wired into any build/CI, and were
intentionally excluded when the Node/Python docs were extracted (PR #126).

Remove them so they do not re-land on master with this feature. The C and Go
repro folders remain — they exercise the new bindings added here.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mlischetti added a commit that referenced this pull request Jul 16, 2026
The native-lib/node/repro and native-lib/python/repro folders are regression
guards for the pre-existing Node and Python bindings, covering fixes that
already merged to master independently (PR #121 read-callback exception, PR
#122 streaming queue backpressure). They are unrelated to the new C/Go/Rust
bindings this branch introduces, are not wired into any build/CI, and were
intentionally excluded when the Node/Python docs were extracted (PR #126).

Remove them so they do not re-land on master with this feature. The C and Go
repro folders remain — they exercise the new bindings added here.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants