@W-23692110: Multiple isolated DataWeave engines per process (Node + Python) - #157
@W-23692110: Multiple isolated DataWeave engines per process (Node + Python)#157mlischetti wants to merge 216 commits into
Conversation
|
Pushed remediation for the two code reviews (
A final whole-branch review across all 14 commits came back clean (no Critical/Important findings); the two Minor findings it raised (native-level test for the "Unknown engine handle" JSON contract, and stray review-notes files) are fixed in the last two commits. |
Root-cause fix for the three findings in the sixth PR #157 follow-up review: model the DataWeave instance lifecycle explicitly (uninitialized/ready/ cleaning-up) instead of a single boolean, make C-side stream/transform admission atomic under g_mutex, and validate napi_get_value_int64 at the handle-read sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root-cause fix for the three findings in the sixth PR #157 follow-up review: model the DataWeave instance lifecycle explicitly (uninitialized/ready/ cleaning-up) instead of a single boolean, make C-side stream/transform admission atomic under g_mutex, and validate napi_get_value_int64 at the handle-read sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2a038f2 to
d504c0f
Compare
Round 11 hardening pushed (
|
| # | Finding | Commit |
|---|---|---|
| — | Extract bridge_begin_op_locked (atomic admission-time pin) |
7f24fc7 |
| #2 | Pin engine at admission for streaming + transform (in_flight++ in the same locked section as g_active_ops++; released exactly once on every early-return) |
27cc714 |
| #3 | Pin engine for the synchronous runScriptEngine |
4950380 |
| #1 | Register an env cleanup hook for every engine (not just resolver-backed); extend the owner-thread destroyEngine guard to fire for any record |
fd49ad1 |
| #5 | Register process beforeExit/exit hooks once per module (never-reset guard) instead of per-singleton |
58c2690 |
| #6 | Real-addon integration tests for the *_engine unknown/destroyed-handle envelope + best-effort run-vs-destroy guard |
cacb41b, 6865803 |
Full Node suite: 885 passed / 59 skipped / 0 failed. Passed a whole-branch review (correctness of the pin across all three run paths, exactly-once release on every early-return, lifecycle-invariant preservation).
Two notes for reviewers
-
Contract change (finding Create LICENSE #1): the owner-thread
destroyEngineguard now keys on "a record exists," not on resolvernapi_refstate — because every engine now carries an env cleanup hook (env-affine state), so a resolver-less engine is also only destroyable from its creating thread. This supersedes the old "resolver-less engines are destroyable from any thread" invariant described in some earlier design docs (docs/superpowers/specs/2026-08-19-worker-teardown-dangling-resolver-ctx-design.mdand the two2026-08-18-*docs).bridge_finalize'snapi_refdeletion stays resolver-gated. -
dwlib C ABI break (finding Set in/out mimetypes as input parameters, env vars #4) is intentional and documented — no compat shims. The branch replaces the
run_script_*_with_resolverexports with the*_enginemulti-engine entrypoints (legacy singletonsrun_script/run_script_callback/run_script_input_output_callbackpreserved) and adds actxparameter toResolveModuleCallback. dwlib is consumed by this repo's own Python/Node bindings in lockstep.
Follow-up not in this round: the runTransform TOCTOU (general review #1) is made memory-safe by the C-side admission pin (worst case is a resolved Unknown engine handle envelope, not a UAF). The additional JS-side recheck/lease the reviewer suggested was deliberately not added — the authoritative pin lives in C. Can add the JS belt-and-suspenders as a separate change if desired.
Round 12 — worker ref-leak & teardown-race hardeningPushed
Concurrency invariants (all re-derived and confirmed in a whole-branch review): exactly-one Tests: 895 passed / 59 skipped / 0 failed. |
Round 13 — per-env init-reference ownership (fixes review #5)Pushed What changed. Replaced the process-global init-reference model — which assumed a strict 1 Mechanism.
Review. Each task passed a spec + quality gate; the whole-branch final review (on the most capable model) independently re-derived the Tests. Full Node suite 897 passed / 59 skipped / 0 failed (added 2 integration tests). The Java layer and Known coverage gap (documented in the test file). The two new |
Round 14 — review #5 remediation (all 7 findings)Addresses every finding from follow-up code review #5 (reviewed head
Invariant preserved throughout: Java side and the legacy singleton entrypoints are untouched this round. 🤖 Generated with Claude Code |
Round 15 — external review #6 remediationAddressed all 8 code findings from the latest follow-up review (
On #5's residual (accepted, documented in-code): if a teardown fails and no later #9 (Python-binding scope) — intentionally not split. The review noted the PR bundles Python-binding modernization alongside the Node multi-engine work. That bundling is intentional for this PR and will not be split out in this round; the Python work is being tracked as part of the same effort. Happy to revisit if a reviewer feels strongly, but flagging it here so the decision is explicit. A whole-branch review of the combined round-15 changes traced the full |
Root-cause fix for the three findings in the sixth PR #157 follow-up review: model the DataWeave instance lifecycle explicitly (uninitialized/ready/ cleaning-up) instead of a single boolean, make C-side stream/transform admission atomic under g_mutex, and validate napi_get_value_int64 at the handle-read sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aaeafb9 to
6fb5603
Compare
Review #7 remediation pushed (
|
| # | Sev | Fix | Commit |
|---|---|---|---|
| #1 | High | Detach the phantom GraalVM thread on a failed (!=0) graal_tear_down_isolate, in both cleanup_thread_fn and teardown_waiter_thread_fn (detach only on the failure branch — never on success/UAF or attach-failure) |
55b2418 |
| #2 | Med | Arm the stranded-teardown retry after a failed init-hook rollback so the next initialize() reclaims the isolate instead of wedging |
ff3b6bb |
| #3 | Med | Model initialize()'s ffi.cleanup() rollback as pending state — observe it (no unhandledRejection) and gate a concurrent initialize() with "cleanup is in progress"; still throws the original error synchronously |
ae35089 |
| #6 | Low | Track native stream rejection by settlement state (startRejected) instead of the startError !== undefined value sentinel, so Promise.reject(undefined) propagates |
eed6ee4 |
| #7 | Low | Fail the worker-lifecycle test when balancing cleanup fails on a passing body (re-throw only when bodySucceeded) |
c258463 |
| #5 | Med | State the final-reference condition in the instance cleanup() docs (resolves after full isolate teardown only when releasing the last instance) |
6462d96 |
| #4 | Med | Root README: await cleanup in the lifecycle example; document the real beforeExit+exit hook pair and signal caveat |
6911de9 |
The final review also tightened the #2 inline comment (c54ff65): that acquire-failure path leaves g_initialized==0, so the retry recovers a transient teardown failure but a persistent graal_tear_down_isolate failure strands the isolate to process exit (pre-existing best-effort degradation) — the prior "adopted by the fast path" wording overstated it.
On finding #8 (Med) — Python-binding modernization scope
Acknowledged, and we agree the Python pytest setup / module extraction / TCK / CI / docs are logically a separate concern from the Node engine-lifecycle work. We're deferring the split to its own follow-up PR rather than doing git surgery on this branch now: the Node lifecycle changes have been through seven review rounds against this exact history, and re-splitting risks disturbing that reviewed state for a packaging reorg. The follow-up PR will carry the Python modernization on its own so its CI/rollout/rollback can be reasoned about independently. This branch's remaining diff remains the Node engine/lifecycle work plus these remediations.
Review #8 remediation — all 7 findings addressed (pushed
|
| # | Finding | Fix | Commit |
|---|---|---|---|
| 1 | High — napi_initialize deadlocks after a repeated stranded-teardown failure (blocks on a uv_cond_wait nobody signals) |
After the retry, detect the unrecoverable stranded state (g_isolate != NULL && !g_initialized && TEARDOWN_NONE) and throw a deterministic error instead of entering the wait loop. g_teardown_needed stays armed and g_ref_count is untouched. |
9fc728f |
| 2 | Med — a synchronous rollback ffi.cleanup() throw strands the instance in "cleaning-up" |
Route the release through a promise boundary so a synchronous throw is normalized to a rejection that still settles the .finally state reset; the caller still synchronously sees the original engine-creation error. |
85d06ae |
| 3 | Med — worker-lifecycle balancing cleanup skips ffi.cleanup() when destroyEngine throws |
Capture the destroy error but always run ffi.cleanup(); surface a balancing failure only when the test body succeeded. |
eccc9d9 (+ 1831ccd) |
| 4 | Med — README teaches invalid terminal-metadata retrieval (generator.return() after for await) |
Document manual next() iteration capturing the terminal { done: true, value: StreamingResult }. |
faf0e78 |
| 5 | Med — README overstates runTransform async input as constant-memory |
Switch large-file examples to a synchronous generator, add the sync-vs-async pre-buffering note, and qualify the Performance claims. | c7799e2 (+ 8378029) |
| 6 | Med — external-modules.md examples omit required cleanup |
Wrap the 5 complete examples in try/finally { await dw.cleanup(); }; label the 2 genuine fragments. |
9eec6b2 |
| 7 | Med — root README overstates cleanup as an unconditional process-wide drain | Qualify the §4 and §9 comments: the drain/teardown happens only when the final shared reference is released. | 8347516 |
Notes
- On finding Create LICENSE #1 and my review Update .travis.yml #7 sign-off: my review Update .travis.yml #7 final review wrongly cleared this as a benign "lingers-to-process-exit" degradation — I conflated the
napi_initializewait path with theisolate_ref_release_n_lockedrelease-path twin (which is benign and adoptable). Finding Create LICENSE #1 was a real hard deadlock (uv_cond_waitholdingg_mutexwith no remaining signaller), and it is now closed. The final review traced the new guard against every state reachable at the wait-loop entry to confirm it fires only in the stranded case and never on a cold start, a healthy re-init, or a legitimate in-flight teardown. - Finding Unable to resolve reference to payload error #2 fix detail: the review's suggested
Promise.resolve().then(() => ffi.cleanup())defersffi.cleanup()into a microtask, which breaks two existing tests that assert synchronous invocation. The shipped fix uses an equivalenttry/catch→Promise.rejectnormalization that preserves the synchronous invocation timing while achieving the same intent. - Two Minor accuracy nits raised by the final review (test double-fault error-masking; a
readFileSyncbounded-memory caveat) are folded into1831ccd/8378029. - Scope unchanged: Node-binding-only; no Python, Java, or legacy-singleton entrypoints touched; handle width stays
long long.
Root-cause fix for the three findings in the sixth PR #157 follow-up review: model the DataWeave instance lifecycle explicitly (uninitialized/ready/ cleaning-up) instead of a single boolean, make C-side stream/transform admission atomic under g_mutex, and validate napi_get_value_int64 at the handle-read sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8378029 to
136e914
Compare
mlischetti
changed the title
svacas
left a comment
svacas
left a comment
There was a problem hiding this comment.
Findings
P1: Failed engine destruction can leave Java holding a freed resolver context
bridge_finalize_registry() (
data-weave-cli/native-lib/node/src/addon.c
Lines 290 to 338 in 1ca9891
fails, but bridge_finalize() still frees the bridge immediately afterward.
The Java registry then retains a CallbackWeaveResourceResolver whose opaque ctx points to freed memory. A later raw-FFI invocation of that handle can dereference the freed bridge in resolve_module_callback, causing
a use-after-free or crash. The resolver-less case also leaves a supposedly destroyed engine registered.
Do not free the bridge unless registry removal succeeds. Preserve it for retry or surface the destruction failure while retaining valid ownership state.
P2: Completion-sentinel OOM leaves streaming operations permanently pending
Both the streaming (
data-weave-cli/native-lib/node/src/addon.c
Lines 918 to 931 in 1ca9891
(
data-weave-cli/native-lib/node/src/addon.c
Lines 1415 to 1431 in 1ca9891
returning.
No completion is delivered, so:
- The JavaScript promise remains pending forever.
- The thread-safe function is not released.
- Its context points to the freed work record.
- A deferred bridge destruction may finalize from the wrong path while its environment cleanup hook remains registered.
Allocate the completion record before starting the worker, where allocation failure can be reported synchronously, or otherwise ensure the promise and thread-safe-function lifecycle are completed safely.
P2: Resolver tokens leak when isolate acquisition fails
NativeRuntime.initialize() (
data-weave-cli/native-lib/python/src/dataweave/native.py
Lines 232 to 249 in 1ca9891
try block. Resolver tokens are therefore removed only when engine creation fails, not when library loading, ABI binding, isolate creation, or bootstrap detachment fails.
I reproduced this with a library-load failure: the token remained in _resolver_registry, retaining the runtime and resolver indefinitely. Retrying initialization registers another token, accumulating leaked entries.
Include isolate acquisition in the rollback scope and unregister the resolver token on every failed initialization path.
P2: Resolver-backed Python initialization is no longer idempotent
DataWeave.initialize() (
data-weave-cli/native-lib/python/src/dataweave/runtime.py
Lines 43 to 46 in 1ca9891
to the idempotent native initialization method. A second call on an initialized resolver-backed instance therefore raises Cannot install a resolver after initialize().
This regresses the previous behavior, where repeated initialize() calls were harmless. Return early when _native.initialized is already true and add resolver-backed double-initialization coverage.
P2: Raw Node initialization uses an unchecked non-string library path
napi_initialize() (
data-weave-cli/native-lib/node/src/addon.c
Lines 572 to 585 in 1ca9891
napi_get_value_string_utf8. Passing a non-string through the exported raw FFI leaves lib_path unspecified before uv_dlopen uses it.
Other raw entrypoints explicitly validate their conversions, and the PR documents that guarantee. Validate this argument and add it to malformed-inputs.test.ts.
Status
Reviewed head 1ca9891. All macOS, Ubuntu, Windows, SAST, and credential-scanning checks pass. The C source also passes a syntax-only compile; git diff --check reports only a trailing blank line in native.py. I would
request changes based on the findings above.
Review #10 remediation — all findings addressed (pushed
|
Review #11 remediation — 7 of 8 findings addressed (pushed
|
mlischetti
changed the title
…review #19 #3) Twin of the Python resolver design doc that review #17 banner-superseded. Flips Status to Superseded and adds a banner linking the shipped 2026-08-07 multi-engine design, naming the three assertions PR #157 made stale: (a) resolver scope -- custom resolvers apply to run() only, not runStreaming/runTransform (built-ins resolve everywhere); (b) resolvers are per-engine and handle-addressed, not one-per-process, with multiple independent engines per process; (c) the old process-wide resolver ABI is replaced by handle-based create_engine_with_resolver + run_script_engine. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses GUS W-23692110, discovered while implementing Node.js external module support (#154). native-lib's ScriptRuntime is a static singleton with a write-once resolver, so a second DataWeave instance in one Node process silently reuses the first instance's resolver instead of getting its own. Design: turn ScriptRuntime into a handle-addressable registry of per-instance engines (one shared GraalVM isolate, following the pattern native-cli's NativeRuntime already uses), with a per-handle resolver bridge in the Node C addon. Python is out of scope here (tracked as a follow-up) since it already gets isolation via one isolate per instance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…egression test Rewires ffi.ts and dataweave.ts to call the new handle-based N-API methods (createEngine/createEngineWithResolver/destroyEngine/ runScriptEngine/runScriptStreamingEngine/runScriptTransformEngine) added in Task 3, removing runWithResolver. Each DataWeave instance now owns its own engineHandle, created on initialize() and destroyed on cleanup(), so multiple instances with different resolvers no longer cross-talk in the same process. Adds independent-engines.test.ts proving two resolver-backed instances resolve only their own modules, that a genuine script error on the new handle-based run() path surfaces as success:false rather than an unhandled throw (runScriptEngine now returns "" instead of throwing on a NULL native result), and that runStreaming/runTransform correctly thread the handle through addon.c's argument-shifted N-API wiring. Deletes the now-obsolete first-resolver-wins regression test and fixture, and rewrites dataweave-resolver.test.ts so each test builds its own minimal resolver map instead of sharing a process-wide "first resolver wins" module map.
…itialize() failure If ffi.initialize() succeeded but engine creation (createEngine/ createEngineWithResolver) then threw, this.initialized stayed false, so cleanup()'s early-return guard meant ffi.cleanup() was never called -- permanently leaking that instance's increment of the native library's ref-counted handle. initialize()'s catch block now releases that ref-count itself (ffi.cleanup()) when ffi.initialize() already succeeded, before wrapping and re-throwing. Adds tests/unit/dataweave-initialize.test.ts, a new unit-lane test (mocked ffi module, no dwlib required) exercising this exact sequencing bug plus the surrounding invariants: no cleanup() call when ffi.initialize() itself fails, no residual state after a failed attempt, and no spurious cleanup() call on the successful path.
…ps (F1, F2) Resolver-backed engine bridges could be freed while a background streaming/ transform uv_thread still dereferenced them via resolve_module_callback (F1), and napi_cleanup deleted thread-affine napi_refs from whatever thread made the last release (F2, undefined behavior across Workers). F1: add in_flight/destroy_pending accounting (under g_mutex). Streaming/transform setup pins the bridge via bridge_begin_op before spawning the worker thread; the completion sentinel releases it via bridge_end_op on the owner thread. destroyEngine unlinks immediately but defers the free (napi_ref delete + struct free) to the last draining op when in_flight > 0. F2: register a per-env cleanup hook (napi_add_env_cleanup_hook) per bridge at creation so each Worker/main env disposes its own napi_ref on its own thread; destroyEngine removes the hook before an early free. napi_cleanup no longer touches g_bridges and only performs the process-global GraalVM isolate teardown once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…k (F3, F4) create_engine/create_engine_with_resolver are GraalVM @CEntryPoints; if Java construction throws, the entrypoint returns the long long default value (0) instead of propagating. Treat any handle <= 0 as invalid: throw an N-API error and unwind the bridge (delete napi_ref, free struct) before it's ever linked into g_bridges or given a cleanup hook, instead of returning/inserting a bogus handle. Also fix a resolver-source buffer leak: if the malloc for the tracking node itself fails, the buffer was previously left untracked and unfreeable. resolver_results_track now reports tracking failure so resolve_module_callback can free the buffer and report "unresolved" instead of leaking it.
…ts worker (review #16 #2) Both teardown-failure branches (_release_isolate and _retry_pending_teardown_locked) detached the just-attached worker with `try: graal_detach_thread(worker) except Exception: pass`, discarding the nonzero STATUS graal_detach_thread returns on failure. A failed detach left the worker attached while _teardown_needed stayed armed, so the next retry attached ANOTHER worker on top of it -- and graal_tear_down_isolate, needing the sole attached thread, was then permanently blocked. Inspect the detach status; on a teardown-plus-detach double failure, transition to the same explicit unrecoverable-leak state as the bootstrap double failure: null the globals, do NOT arm a retry, retain no thread. A later initialize() builds a fresh isolate. The detach-succeeds branch (retain live + arm retry) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…leak window (review #16 #3) The spec claimed the isolate exists iff the refcount > 0, but both bindings retain a live isolate at zero refs after a failed teardown, and Python leaks one at zero refs on a bootstrap or (review #16 #2) release double failure. Redefine the count as outstanding ownership/init references: positive requires a live isolate; zero may temporarily retain one pending retry or leave one leaked after the unrecoverable path. Document the new release double-failure leak in §7.2 and §10. Verified the public READMEs and test_ci_structure.py never made the iff claim (no change needed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…py's header (review #16 #3) Finding #16 #3's remediation redefined the "isolate exists iff refcount > 0" invariant in the design spec, READMEs, and test_ci_structure but missed the source-of-truth file's own header comment, which still asserted "_isolate is not None iff the count > 0". That contradicts the retention/leak behavior implemented in the same file (a detach-succeeds teardown failure leaves _isolate non-None at refcount 0 with _teardown_needed armed; a double failure leaks it). Align the comment with §7.1: the count is outstanding ownership/init references -- positive requires a live isolate; zero may temporarily retain one pending retry or leak one after an unrecoverable teardown. Comment-only; unit suite unchanged (117 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…re (review #17 #1) cleanup_thread_fn and teardown_waiter_thread_fn ignored fn_detach_thread's return after a failed graal_tear_down_isolate. On a double failure the exiting worker stayed attached while g_teardown_needed was armed, so future retries attached more workers and teardown became permanently impossible. Add a third teardown outcome (CLEANUP_UNRECOVERABLE) threaded through the shared helper, its four synchronous callers, and the async waiter. On the double failure, abandon_unrecoverable_isolate_locked() clears the published globals so a later initialize() builds a fresh isolate, does NOT arm the retry, emits a stderr diagnostic, and leaks the old isolate for the process lifetime -- the Node twin of the Python policy shipped in review #16 #2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…etry machinery (review #17 #2) §5 and §7 intro claimed Python needs "none of Node's retry machinery," contradicting §7.1/§7.2 and shipped native.py, which implement _teardown_needed + a synchronous teardown retry. Clarify: Python needs none of Node's ASYNCHRONOUS waiter/PENDING_WAIT/ adoption machinery, but does implement a simpler SYNCHRONOUS retry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…k contract (review #17 #3) Both READMEs described teardown failure as flatly retryable, omitting the intentional unrecoverable double-failure branch (teardown+detach, or the bootstrap double failure) that resets published state, leaks the isolate for the process lifetime, and lets a future initialization build a fresh one. Document both contracts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iew #17 #4) The 2026-08-24 Python-only resolver design predates the handle-based shared-isolate model. Its run_script_with_resolver ABI, "no Java/Node changes," dedicated-isolate, and resolve-on-first-run claims are all stale. Add a superseded banner pointing at 2026-08-07-native-lib-multi-engine-design.md and listing the invalidated assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…6 (review #17 final) §6.1/§6.2 (Node) described only the two-outcome teardown model and claimed the retry signal is armed on any teardown failure, contradicting the new Node leak-and-continue on the teardown-plus-detach double failure shipped in 9efb0d1. Add the unrecoverable-leak branch to §6.2 and the zero-count leak clause to the §6.1 invariant, mirroring the Python §7.1/§7.2/§10 treatment already present. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nding-specific (review #18 #1) The shared root README said ordinary failures retry "on the next initialization or engine release" -- inaccurate for both bindings. Split by binding: Node retries at the next initialization or async op-completion drain; Python retries synchronously at the next initialization. Also attribute the bootstrap double-failure to Python only (Node has no bootstrap-detach retry path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ysical reclamation (review #18 #2) The Node package README said a final-reference cleanup() resolves once teardown "has actually finished." Both failure paths still resolve the promise (ordinary retryable failure, or unrecoverable double-failure leak). Document that cleanup() guarantees logical release and completion of the teardown attempt (after draining in-flight ops), not necessarily physical reclamation; ordinary failures retry where safe, double failures intentionally leak with a diagnostic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…physical reclamation (review #19 #1 #2) The root README code-example comment, the exported TypeScript cleanup() TSDoc, its coalescing comment, and the unit-test rationale all promised that a final-reference cleanup() resolves only once the isolate 'has actually finished tearing down'. The shipped contract resolves once the teardown ATTEMPT completes: it guarantees logical release, not physical reclamation. An ordinary failure retains the live isolate and retries where safe (later init or async op-completion drain); an unrecoverable teardown-plus-detach double failure leaks the isolate until process exit with a stderr diagnostic. Reworded all four spots to match. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…review #19 #3) Twin of the Python resolver design doc that review #17 banner-superseded. Flips Status to Superseded and adds a banner linking the shipped 2026-08-07 multi-engine design, naming the three assertions PR #157 made stale: (a) resolver scope -- custom resolvers apply to run() only, not runStreaming/runTransform (built-ins resolve everywhere); (b) resolvers are per-engine and handle-addressed, not one-per-process, with multiple independent engines per process; (c) the old process-wide resolver ABI is replaced by handle-based create_engine_with_resolver + run_script_engine. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…20 low-pri) - dataweave.ts: module-level coalescing comment quoted the old 'resolves once native teardown has finished' contract; now 'resolves once the native teardown attempt has completed', matching the instance-level twin corrected in review #19. - dataweave-initialize.test.ts: drop the reference to the absent task-4-report.md planning artifact, and reword comments/test title that named a removed boolean 'initialized' field and the guard 'if (this.initialized) return;' -- the class now uses the string state machine ('uninitialized' | 'ready' | 'cleaning-up'). - engine-handle-contract.test.ts: drop the reference to the absent task-6-report.md planning artifact. Comment/test-name only; no behavioral change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ach guard cannot short-circuit (review #20 final) Final whole-branch review noted that graal_detach_thread and graal_tear_down_isolate were dlsym'd but not in the required-symbol gate. The review #20 #1 bootstrap-detach failure path guards on those pointers (if (fn_detach_thread && ...) / fn_tear_down_isolate ? ...), so a NULL fn_detach_thread would short-circuit and fall through to a successful publish -- re-opening the exact phantom-attached-bootstrap- thread wedge #1 closes. Add both to the required-symbol check so init fails fast with a clear message and the guard's guarantee is unconditional. Practically unreachable (GraalVM co-exports these with graal_create_isolate) but makes the invariant explicit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e (review #21 #3 #4) #3: create_engine_with_resolver ABI post-isolate order is (resolverCallback, ctx) -- the spec had (ctx, trampoline) reversed at three sites, which would lead a C/FFI consumer to pass the context where a function pointer is expected. Show the correct order plus the full (isolateThread, resolverCallback, ctx) signature. #4: module-level cleanup() resolves once the teardown ATTEMPT completes (logical release), not once physical teardown finishes -- align 6.4 with the 6.2 retry-on-ordinary-failure / leak-on-unrecoverable-failure model, matching the wording already corrected in README.md and dataweave.ts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ls init instead of crashing createEngine (review #21 #2) fn_attach_thread is called unconditionally on the engine-creation, execution, and teardown paths (e.g. createEngine's fn_attach_thread(g_isolate, &thread) has no NULL guard), but the required-symbol gate checked only create_isolate/ free_cstring/detach/tear_down. A dwlib missing graal_attach_thread passed init and then invoked a NULL function pointer on the first createEngine(). Add it to the gate so init fails fast with a clear missing-symbol error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…o (review #21 #1 completeness) Final whole-branch review found one ordinary detach still unchecked: the engine_bridge_t calloc-failure rollback in napi_create_engine. A detach failure there strands a phantom thread that would wedge a later teardown -- the exact hang #1 eliminates everywhere else. Fold the capture+poison into the existing g_active_ops-- critical section, matching the other sites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
595f22e to
566c701
Compare
…ine surface The dataweave-addon-path.test.ts added by master's ca09d69 mocks the legacy singleton ffi surface (runScript/runWithResolver), which this branch removed. The multi-engine DataWeave.initialize() now calls ffi.createEngine()/ createEngineWithResolver(), so the mock threw "No createEngine export" and failed nodeTest after the rebase. Mock the current engine-handle surface instead (matching dataweave-initialize.test.ts). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Replace
native-lib's process-wideScriptRuntimesingleton (one engine, write-once resolver, first-caller-wins) with a handle-keyed registry of per-engineScriptRuntimeobjects living in one shared GraalVM isolate — closing W-23692110 for both the Node and Python bindings, which now drive the same model through the identical*_engineC ABI.DataWeaveinstance (Node or Python) owns an independent native engine — its own module resolver and script cache — addressed by an opaque handle, so multiple instances with different resolvers coexist in one process with no cross-talk.ScriptRuntimesingleton (getInstance()/defaultInstance) and the three legacy singleton C entrypoints (run_script,run_script_callback,run_script_input_output_callback) are removed. All execution is handle-addressed. This is an intentional pre-GA C-ABI break with no shims; dwlib is consumed only by this repo's own bindings, in lockstep.ResolveModuleCallback(thread, ctx, modulePath); resolver dispatch is per-engine via the opaquectx.Two bindings, one model
addon.c) hardened across many review rounds.native.pyimplements a module-level, reference-counted isolate with one engine handle perDataWeaveinstance; the public Python API is unchanged.Design
Consolidated design (both bindings, final state as shipped):
docs/superpowers/specs/2026-08-07-native-lib-multi-engine-design.md. Each Node hardening round and the Python unification are folded into that single doc (provenance map in its appendix).Post-review hardening
The C addon's lifecycle/concurrency and out-of-memory paths were hardened across many Node-binding review rounds — thread-spawn and thread-safe-function failure handling, N-API thread-affinity discipline for resolver bridges, coalesced
cleanup(), a re-init-during-pending-teardown deadlock fix (TEARDOWN_*state machine + live-isolate adoption), atomicg_mutexadmission for all run/stream/transform entrypoints, exhaustivenapi_get_value_*/napi_create_*status checks, OOM-safe setup and worker/callback allocations, and deferral of engine-registry removal until an engine's admitted ops drain.The Python unification then went through the same task-by-task + final-whole-branch review discipline. The final review caught and fixed a cross-thread isolate-teardown hang: the Python glue kept the isolate's bootstrap thread attached for the isolate's life, so a last-release
graal_tear_down_isolaterunning on a different OS thread (e.g. theatexitpath) would block forever. The fix mirrors the Node/Go bindings — detach the bootstrap thread immediately aftergraal_create_isolateand attach a fresh thread on demand for every native call — plus unregistering a resolver token on a failedinitialize().Test plan
./gradlew native-lib:test— Java registry isolation / cross-talk / built-ins-only engines pass; singleton (getInstance()) removed. BUILD SUCCESSFUL../gradlew native-lib:nativeCompile— newcreate_engine*/*_enginesymbols exported; legacy singleton entrypoints removed.native-libNode vitest suite — 952 passed / 32 skipped / 0 failed, incl.independent-engines, teardown-deadlock, and admission regressions plus TCK conformance (676 passed / 0 failed).native-libPython — unit 105 passed; integration 33 passed (incl. multi-instance refcount teardown, per-engine resolver isolation, and a foreign-thread no-hang regression); TCK 729 selected / 0 failed.🤖 Generated with Claude Code