W-23692110: Multiple isolated DataWeave engines per process (Node) by mlischetti · Pull Request #157 · mulesoft/data-weave-cli

mlischetti · 2026-08-10T14:28:54Z

Summary

Replace native-lib's process-wide ScriptRuntime singleton (one engine, write-once resolver, first-caller-wins) with a handle-keyed registry of per-engine ScriptRuntime objects living in one shared GraalVM isolate — closing W-23692110 for the Node binding.
Each DataWeave Node instance now 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.
Legacy singleton entrypoints (run_script, run_script_callback, run_script_input_output_callback) and ScriptRuntime.getInstance() are unchanged, so the Python binding is unaffected. Python's own migration to per-instance engines is a separate follow-up.

Design

Original design: docs/superpowers/specs/2026-08-07-native-lib-multi-engine-design.md (commit 729ed19). Each subsequent hardening round has its own committed spec under docs/superpowers/specs/.

Post-review hardening

After the initial implementation, the C addon's lifecycle/concurrency and out-of-memory paths were hardened across nine follow-up 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), atomic g_mutex admission for all run/stream/transform entrypoints, exhaustive napi_get_value_*/napi_create_* status checks, OOM-safe setup and worker/callback allocations, and deferral of the engine registry removal until an engine's admitted ops drain (closing the "Unknown engine handle" admission race). Every round was fixed Node-binding-only (Python surface and legacy singletons untouched), documented in a per-round spec under docs/superpowers/specs/, and re-verified against a green Node suite.

Test plan

./gradlew native-lib:test — registry isolation, cross-talk, and built-ins-only engines pass; legacy getInstance() tests unaffected.
./gradlew native-lib:nativeCompile — new create_engine*/*_engine symbols exported.
native-lib Node vitest suite — 878 passed / 59 skipped / 0 failed, including the independent-engines, teardown-deadlock, and admission regressions plus TCK conformance.
./gradlew native-lib:pythonTest — passing, confirming the legacy Python-facing surface is untouched.
Implemented and reviewed task-by-task, plus a final whole-branch review per round.

🤖 Generated with Claude Code

mlischetti · 2026-08-11T11:35:56Z

Pushed remediation for the two code reviews (docs/reviews/pr-157-code-review-andy.md, docs/reviews/pr-157-code-review.md, both now removed from the repo). All 7 findings (F1–F7) were validated against source before fixing:

F1/F2 (High): bridge use-after-free during in-flight streaming/transform + cross-Worker N-API misuse in cleanup. Added in_flight/destroy_pending accounting on engine_bridge_t, deferred free until the owner thread's completion sentinel drains, and per-env napi_add_env_cleanup_hook so each Worker disposes only its own refs.
F3/F4 (Low/Medium): resolver-buffer OOM leak on tracking-node allocation failure, and a handle <= 0 construction failure silently accepted into the registry. Both now fail closed.
F5 (Medium): per-engine ABI symbols are now required at load time with a clear compatibility error, instead of failing later per-call.
F6 (Medium): added throwing-resolver, resolver-backed reinit, cleanup-during-streaming (F1 regression guard), and destroyed-handle tests.
F7 (Minor): Java-side resolver-exception logging now matches the C-side's DATAWEAVE_RESOLVER_DEBUG-gated, content-free-by-default policy.

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>

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>

…vers

…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.

… path The handle <= 0 rejection path did manual napi_delete_reference + free(bridge) instead of bridge_finalize, so any resolver-callback buffers already tracked via resolver_results_track (if resolve_module_callback ran during a failed eager module setup before construction was reported as failed) were leaked. bridge_finalize already frees tracked buffers before freeing the struct and is a safe drop-in here since the bridge was never linked into g_bridges or given a cleanup hook at this point.

Node addon.c: fail initialize() with a clear message when dwlib lacks the per-engine symbols (create_engine, create_engine_with_resolver, destroy_engine, run_script_engine, run_script_callback_engine, run_script_input_output_callback_engine) instead of deferring to a confusing per-call error, since every initialize() now creates an engine. CallbackWeaveResourceResolver.resolve(): suppress exception detail by default and only log e.getMessage() when DATAWEAVE_RESOLVER_DEBUG=1, matching the C-side resolve_module_callback policy in addon.c. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…free The default (non-debug) branch of CallbackWeaveResourceResolver.resolve()'s catch block still logged the module path unconditionally, which is dynamic, resolver-controlled content. Drop path too in the default branch so the log line is fully static, matching the C-side resolve_module_callback's actual default behavior in addon.c. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Adds four resolver-backed integration tests to dataweave-resolver.test.ts that exercise paths untested by prior remediation commits: - a throwing resolveModule() causes run() to fail cleanly (success:false) rather than crash, exercising resolve_module_callback's exception catch/clear/log-gated-by-DATAWEAVE_RESOLVER_DEBUG path. - a resolver-backed instance's initialize -> cleanup -> initialize cycle still resolves a custom module afterwards (fresh engine_bridge_t). - cleanup() raced against an in-flight resolver-backed runStreaming() does not crash -- the regression test for the F1 in-flight-refcount fix, started deterministically by calling gen.next() without awaiting it before calling cleanup(), so the native call is already handed to the libuv worker thread when cleanup() runs on the JS thread. - run() after cleanup() throws DataWeaveError via dataweave.ts's ensureInitialized() guard (the TS-level half of the destroyed/unknown engine handle contract).

Extracts the "Unknown engine handle" JSON literal shared by run_script_engine, run_script_callback_engine, and run_script_input_output_callback_engine into a single package-visible constant (NativeLib.UNKNOWN_ENGINE_HANDLE_JSON), so the exact error contract can be asserted from a plain JVM unit test. The @centrypoint methods themselves can't be exercised directly from a JVM test since their GraalVM word-type parameters (IsolateThread, CCharPointer) only resolve inside a compiled native image. Adds ScriptRuntimeTest#unknownEngineHandleProducesExactErrorJson, which combines that constant assertion with the existing proof that ScriptRuntime.get() returns null for an unregistered handle.

These were internal review artifacts incidentally committed during remediation work (one references a local temp worktree path); they aren't product documentation and shouldn't ship in the repo.

The follow-up PR-157 review found that DataWeave.cleanup() can deadlock the process when called while a runStreaming()/runTransform() operation is still in flight: isolate teardown blocks the JS thread that a mid-delivery worker's threadsafe-function call depends on. This design makes teardown async and wait for active ops to drain via a dedicated waiter thread, instead of blocking inline.

…completion wakeups Changed uv_cond_signal to uv_cond_broadcast in the op-completion sentinels (call_js_write and call_js_transform_write) to prevent the signal from being stolen by a concurrent initialize() waiter, which would cause a deadlock where teardown_waiter_thread_fn never receives the wakeup it needs to detect g_active_ops reached 0.

…thread, not the JS-thread callback

…n failure If uv_thread_create_ex() fails for the streaming or transform background worker, nothing ever ran to decrement g_active_ops or release the resolver bridge hold, permanently wedging cleanup(). Capture the spawn return value and, on failure, unwind everything committed since the promise was created (g_active_ops decrement, bridge_end_op, threadsafe function release, deferred resolution with an error sentinel, and frees) in the same order as the existing completion branches, minus the thread join. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

napi_cleanup case 5 ignored uv_thread_create_ex's return value when spawning the teardown waiter thread. If the spawn fails, g_teardown_pending would stay true forever, permanently blocking every future initialize() and cleanup() call. Capture the spawn result and, on failure, roll back g_teardown_pending, detach the enqueued waiter, resolve its promise inline, release its threadsafe function, and restore g_ref_count to 1 so the isolate is correctly treated as still live. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

call_js_read early-returned without signaling req->cond when N-API invokes it with env == NULL during environment teardown (e.g. a Worker terminating mid-transform) while data is non-NULL. transform_read_cb blocks synchronously on that same condition variable, so the early return left it hung forever, stranding the worker thread's isolate detach. Restructure to treat env == NULL (with live data) as a terminal read error: set bytes_read = -1 and fall through to the existing signal block, so the blocked waiter always wakes exactly once. The data == NULL branch (nothing to signal) is untouched. Also added confirming comments on call_js_write and call_js_transform_write noting their env == NULL early-returns are not the same bug: their completion path is driven by a separately-enqueued sentinel chunk, not a synchronously-blocked waiter.

mlischetti · 2026-08-20T16:59:04Z

Round 12 — worker ref-leak & teardown-race hardening

Pushed 6865803..765c273 (14 commits). Remediates the findings from the latest follow-up review of the multi-engine Node binding. Node-binding-only (addon.c + dataweave.ts + tests); the Java/native-image layer is untouched, so the existing dwlib stays valid.

#	Finding	Fix
#2	Abandoned-Worker init-ref leak (env-cleanup path never released the isolate ref)	`isolate_ref_release_core_locked` + `deferred_ref_release` (`1abe0a8`, prep `2cbe637`, doc `c55f4b7`)
#3	Registry-attach vs isolate-teardown race in `bridge_finalize`	split into `bridge_finalize_registry` (transient `g_active_ops` reservation, check+increment in one `g_mutex` hold) + `bridge_finalize_free` (`81ad661`)
#4	`runTransform` could use a stale handle after async input pre-buffering	`ensureReady()` re-check after `createChunkReader` await (`c81f38f`)
#6	Engine creation ignored `napi_add_env_cleanup_hook` failure	all-or-nothing rollback + throw (`b6ab957`); fix round dropped a double-release of the init ref (`41794ec`)
#5	Module-level `cleanup()` had no coalescing for overlapping calls	module-scoped `cleanupPromise` + per-instance `cleaningInstance` guard (`82fd69c`, + final-review fix in `765c273`)
#7	README `cleanup()` doc bugs	doc fix (`e1b9ee0`)
#8	Weak run-vs-destroy admitted-ordering test	require `success` + full chunks, drop the "or Unknown handle" tolerance (`94cb479`)
#9	No real `worker_threads` coverage	new `worker-lifecycle.test.ts`, 5 tests (`5173a6f`, hardened in `765c273`)

Concurrency invariants (all re-derived and confirmed in a whole-branch review): exactly-one g_ref_count release per initialize(); g_active_ops (isolate teardown gate) vs per-engine in_flight (registry-removal gate) kept distinct; teardown state machine transitions in one g_mutex critical section; napi_env/napi_ref/napi_deferred/napi_threadsafe_function thread-affinity preserved; fn_destroy_engine called exactly once per handle.

Tests: 895 passed / 59 skipped / 0 failed.

…p, review #5) Fixes follow-up review #5: g_ref_count is a bare global with no notion of which napi_env owns each reference, so a raw initialize()-once + createEngine()-N consumer's abandoned env fires N per-engine release hooks against a count of 1, tearing the isolate down under still-live engines (potentially in another env). Design tracks init-reference ownership per env (g_ref_count == sum of per-env init_refs), stops the per-engine hook from releasing the isolate reference, and gates cleanup() on the calling env's ownership. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…prep) Introduces g_env_recs (one env_init_rec_t per napi_env that took an init reference) plus env_init_rec_find_locked / env_init_rec_acquire_locked, both g_mutex-guarded. No behavior change yet -- wired into initialize()/cleanup()/ env death in the following tasks. Establishes the invariant to hold: g_ref_count == sum of per-env init_refs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…aps n=1 (round 13 #5 prep) Extracts the reached-zero teardown decision into isolate_ref_release_n_locked(n) so a multi-reference release (an env's whole balance) makes the teardown/waiter decision exactly once instead of re-entering it per reference. isolate_ref_release_core_locked becomes a thin wrapper over n=1 -- behavior-preserving; suite unchanged at 895/59/0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…hree sites (round 13 #5) initialize()'s adoption, fast, and create paths now find-or-create the calling env's init record and increment its init_refs alongside g_ref_count++, and register one env-death hook per env on first use (all-or-nothing: a calloc or hook-registration failure rolls back and throws without bumping g_ref_count). env_init_cleanup body follows in the next task. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…s references (round 13 #5) A dead env's outstanding init references are released here, all at once, from a single env-scoped decision point via isolate_ref_release_n_locked. LIFO hook ordering guarantees this runs after every per-engine bridge_env_cleanup, so engine bridges finalize while the isolate is still alive. Paired with the next task, which removes the now-duplicate per-engine ref release. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…reference (round 13 #5) The isolate reference belongs to initialize() (isolate lifetime), not to an engine (Java-registry-entry lifetime). Releasing it per engine let a raw initialize()-once + createEngine()-N consumer's abandoned env fire N releases against a count of 1, tearing the isolate down under still-live engines. The reference is now owned per env (Tasks 1-4) and released only by that env's cleanup() or its env-death hook. Removes deferred_ref_release and the per-engine releases in bridge_env_cleanup/bridge_end_op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…hip (round 13 #5) release_isolate_ref_locked now decrements g_ref_count only when the calling env's init record shows an outstanding reference; a cleanup() with no matching initialize() on this env (or a double-cleanup()) is an explicit no-op instead of an unconditional decrement floored at zero. Closes the symmetric UAF where a raw over-cleanup() from one env could tear the isolate down under another. Sanctioned 1:1 usage is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ound 13 #5) Raw-ffi tests via the ref-count proxy: (1) one initialize() with multiple engines stays live when a single engine is destroyed -- the isolate reference belongs to initialize(), not to an engine; (2) a second cleanup() on an env that owns no reference is a no-op that does not corrupt the count (proven by a subsequent balanced init/run/cleanup cycle still tearing down to zero). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…rd acquire failure (round 13 #5) If env_init_acquire_and_hook() fails after init_thread_fn built the isolate but before g_initialized=1, throwing left g_isolate!=NULL && g_initialized==0 -- which traps the next initialize() forever in the wait loop's uv_cond_wait (nothing broadcasts g_teardown_cond in TEARDOWN_NONE). Tear the isolate back down before throwing, restoring the recoverable g_isolate==NULL state the sibling init error paths already leave. Corrects the design-spec recoverability note. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

mlischetti · 2026-08-21T15:01:15Z

Round 13 — per-env init-reference ownership (fixes review #5)

Pushed 765c273..bd68c70 (8 commits: design spec + 7 implementation commits).

What changed. Replaced the process-global init-reference model — which assumed a strict 1 initialize() ↔ 1 engine ↔ 1 cleanup() pairing — with per-napi_env init-reference ownership, establishing the invariant g_ref_count == Σ (per-env init_refs). This closes finding #5 of the previous review: a raw consumer doing initialize() once + createEngine() N times, then abandoning the env, previously fired N per-engine references against a count of 1 and could tear the shared GraalVM isolate down under a still-live env (UAF / premature teardown). The symmetric hole — an over-cleanup() from one env stealing another env's reference — is closed too.

Mechanism.

A per-napi_env record (env_init_rec_t in a g_mutex-guarded list) tracks each env's net initialize()-minus-cleanup() balance.
All three napi_initialize sites acquire the env's reference and register an env-death hook on the env's first initialize() (so Node's LIFO ordering runs it after every per-engine bridge finalizes on a live isolate).
The per-engine finalize path (bridge_env_cleanup/bridge_end_op) no longer mutates g_ref_count — the core fix.
cleanup() now decrements only when the calling env owns a reference (double-cleanup / cleanup-without-initialize is an explicit no-op).
Bounded isolate_ref_release_n_locked(n) releases a dead env's whole balance and makes the reached-zero teardown decision at most once. Both spawn-failure rollbacks now restore g_ref_count to the true remaining Σ init_refs (not a hardcoded 1).

Review. Each task passed a spec + quality gate; the whole-branch final review (on the most capable model) independently re-derived the g_ref_count == Σ init_refs invariant and confirmed it holds at every g_mutex release, with the per-engine path provably no longer touching the count. Final review surfaced one Important regression — a create-path acquire failure (OOM/hook-registration) after the isolate was built but before g_initialized=1 could orphan the isolate and hang the next initialize(); fixed by tearing the just-built isolate back down before throwing, and re-reviewed clean.

Tests. Full Node suite 897 passed / 59 skipped / 0 failed (added 2 integration tests). The Java layer and dwlib are unchanged this round (addon.c + tests + spec only).

Known coverage gap (documented in the test file). The two new env-init-ownership.test.ts cases are single-env smoke tests, relabeled honestly — a review confirmed they pass unchanged on the pre-fix addon, because #5 is a cross-env bug that a single-env test cannot distinguish. Cross-env behavior is exercised indirectly by worker-lifecycle.test.ts; a dedicated cross-env regression test that goes RED on the pre-fix addon remains a follow-up. The fix's correctness rests on the invariant re-derivation, which is stronger evidence than a smoke test.

…gine-design

Engine-creation admission race (#1 High), teardown-failure recovery via a g_mutex-guarded retry flag (#2/#3 Medium), cross-env Worker regression test (#4), Worker helper strictness (#5), cleanup() ref-leak on destroyEngine throw (#6), and resolver-example cleanup docs (#7). Preserves g_ref_count == Σ per-env init_refs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…eservation (round 14 #1) napi_create_engine / napi_create_engine_with_resolver now require, in one g_mutex critical section, a live isolate not past the point of no return, that the calling env owns an init reference, and a g_active_ops reservation pinning the isolate across the attach/create. The reservation is balanced on every exit after it is taken (success, invalid-handle, alloc-fail, hook-fail). Closes the race where a non-owning env attaches to an isolate being torn down. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…rphan a live isolate (round 14 #2/#3) Add a g_mutex-guarded retry signal g_teardown_needed (NOT a reference: g_ref_count stays 0, invariant g_ref_count == sum(init_refs) preserved), armed when a reached-zero teardown cannot be carried out (waiter alloc/spawn failure, cleanup_thread_fn attach failure) with the isolate left live and owner-less. retry_stranded_teardown_locked() retries the synchronous teardown at the streaming/transform op-completion drain points; adoption in initialize() clears the flag. Closes the two paths that stranded a live isolate with no owner to retry cleanup. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

… (round 14 follow-up) Case 4's synchronous g_active_ops==0 teardown path left cleanup_thread_fn spawn/attach failure un-armed, stranding a live isolate with zero owners and no retry signal -- the exact defect this task closes, just missed in its structural twin. Mirror isolate_ref_release_n_locked's sync-failure arm exactly: same guard (g_isolate != NULL && g_ref_count == 0), same else-if chaining onto the existing torn_down check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…() throws (round 14 #6) DataWeave.doCleanup() called ffi.destroyEngine() before ffi.cleanup(); a throwing destroyEngine (e.g. wrong-thread destruction) skipped ffi.cleanup() and leaked this env's native init reference. Now capture the primary error, clear the handle, always run ffi.cleanup() to release the reference, and re-throw the primary error. Unit test with a mocked throwing destroyEngine. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…d 14 #5) runWorker resolved as soon as the Worker posted a message, hiding a later nonzero exit (e.g. an env-cleanup-hook failure after the success result was posted). Now wait for exit: reject every nonzero code, treat a zero exit with no posted result as a distinct failure, and resolve only on a clean exit that posted a message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…-release (round 14 #4) A Worker initializes once, creates N engines, and exits without cleanup(); the main-thread engine must still run afterward (round-13 releases exactly one reference per abandoned env regardless of engine count). Goes RED on round-12 (N per-engine releases tore the isolate down under the live main engine) and passes at round 13+. Updates the env-init-ownership.test.ts coverage-gap note to point at this test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…d 14 #7) Both resolver-backed quick-start examples now wrap run() in try/finally with await dw.cleanup(), matching the documented requirement that uncleaned instances retain their engine and resolver closure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

mlischetti · 2026-08-21T17:50:06Z

Round 14 — review #5 remediation (all 7 findings)

Addresses every finding from follow-up code review #5 (reviewed head bd68c70). Range e8a0f7f..7017ded (design spec + 7 fix commits). Full Node suite green on HEAD: 899 passed / 59 skipped / 0 failed (+1 unit, +1 integration vs. the pre-round baseline).

#	Sev	Finding	Fix	Commit
1	High	engine creation could attach to an isolate being torn down	gate both `create` functions on per-env ownership + teardown-state + a `g_active_ops` reservation in one `g_mutex` critical section	`3073cce`
2/3	Med	a failed last-reference teardown could strand a live owner-less isolate	`g_teardown_needed` retry signal (not a reference), armed in 5 failure branches when the isolate is left live with 0 owners, retried at the streaming/transform op-drain points, cleared on all 3 `initialize()` success paths	`ecadc85`, `3b206a2`
6	Med	`DataWeave.cleanup()` leaked its init reference if `destroyEngine()` threw	`doCleanup()` now always runs `ffi.cleanup()` even when `destroyEngine()` throws, preserving/re-throwing the primary error	`eda947a`
5	Med	Worker test helper hid a nonzero exit	`runWorker` rejects every nonzero exit and a zero-exit-without-message distinctly	`a462bfa`
4	Med	round-13 tests didn't pin the round-12 cross-env over-release	real-addon cross-env regression: a Worker inits once, creates N=3 engines, exits without cleanup; asserts the live main engine survives, then balances to exactly zero. Goes RED on round-12, green on round-13+	`97128e3`
7	Low	resolver quick-start docs omitted cleanup	both examples wrapped in `try/finally` with `await dw.cleanup()`	`7017ded`

Invariant preserved throughout: g_ref_count == Σ per-env init_refs at every g_mutex release; g_teardown_needed is a retry signal, never a reference (set only when g_ref_count == 0). Round 14 introduces no new g_ref_count mutation; the admission block takes g_active_ops, balanced on every post-reservation exit.

Java side and the legacy singleton entrypoints are untouched this round.

🤖 Generated with Claude Code

Covers all 8 code findings (#1 poisoned singleton, #2 hung stream, #3 unchecked teardown return, #4 waiter attach-failure strand, #5 zero-op drain via init-driven teardown completion, #6/#7/#8 test hardening). #5 uses the chosen init-driven-completion approach with documented lingering-until-process-exit residual; #9 (Python scope) left as-is with a PR note. Preserves the g_ref_count invariant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…cklog) PR follow-up code-review notes and the GA cleanup backlog are local working notes, not deliverables. Add gitignore rules and untrack ga-cleanup-backlog.md (local copy retained). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

A failed first getGlobalInstance() init previously left a poisoned, uninitialized singleton that made every later run*() fail. Build+init a local candidate and publish only on success. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…eview #6 #2) streamFromNative only wired the fulfilled branch of start(); a rejection left done=false so parked consumers hung forever and the rejection was unhandled. Handle both branches: wake all waiters, drain buffered chunks, then throw the start error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

… (review #6 #3/#4) cleanup_thread_fn and teardown_waiter_thread_fn treated a nonzero graal_tear_down_isolate as success, orphaning a live isolate. Set torn_down only on a 0 return. In the async-waiter last-release path, arm g_teardown_needed when teardown did not happen and the isolate is stranded with zero owners, instead of leaving it with no retry. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…eview #6 #5) Zero-op teardown-failure paths armed g_teardown_needed, but retries fire only at op completion -- with no pending op the isolate was never reclaimed and a naive adoption discarded the pending teardown. Call retry_stranded_teardown_locked() at the top of napi_initialize so a pending teardown is completed (or retried) before adopt/create. Document the residual: with no later op or init, the isolate lingers to process exit (OS reclaims it). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…ate (review #6 #6/#7) The cleanup:true worker path swallowed destroyEngine errors, letting a broken destroy pass as a clean lifecycle; fold the error into the posted message with cleanup() still in finally. Wrap the cross-env test in try/finally so a mid-test failure cannot strand a live isolate + held reference for sibling tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…eview #6 #8) toHaveBeenLastCalledWith() false-passed because the first init already called createEngine() with the same args. Clear the mock before the second init and assert it was called exactly once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

mlischetti · 2026-08-21T20:36:39Z

Round 15 — external review #6 remediation

Addressed all 8 code findings from the latest follow-up review (pr-157-follow-up-code-review-6.md), validated against 7017ded. Six commits (ca135a9..aaeafb9), each mapped to a finding. Full Node suite green throughout: 902 passed / 59 skipped / 0 failed.

#	Sev	Fix	Commit
1	High	`getGlobalInstance()` builds+inits a local candidate and publishes the singleton only on success — a failed first init no longer leaves a poisoned, uninitialized singleton. Regression added.	`ca135a9`
2	High	`streamFromNative` now handles the rejected-`start()` branch: wakes parked consumers, drains buffered chunks, then throws — no more hang / unhandled rejection. 2 regressions added.	`092c665`
3	Med	`cleanup_thread_fn` / `teardown_waiter_thread_fn` set `torn_down` only when `graal_tear_down_isolate` returns 0 — a nonzero teardown no longer clears globals and orphans a live isolate.	`73de951`
4	Med	The async teardown-waiter's last-release path arms `g_teardown_needed` when teardown didn't happen and the isolate is stranded with zero owners (mirrors the sync twin).	`73de951`
5	Med	`napi_initialize` drives a pending stranded teardown to completion via `retry_stranded_teardown_locked()` before adopting/creating, so a zero-op strand is reclaimed instead of silently discarded. Chosen approach: init-driven completion, no new async infrastructure.	`62868f8`
6	Med	Worker `cleanup:true` path surfaces `destroyEngine` errors (folded into the posted message) instead of swallowing them; `cleanup()` stays in `finally`.	`2ef9272`
7	Med	Cross-env regression test wrapped in `try/finally` so a mid-test failure can't strand a live isolate + held reference for sibling tests. Survival assertions stay inside `try` (RED-on-round-12 property preserved).	`2ef9272`
8	Low	Reinitialization unit test tightened (`mockClear()` + `toHaveBeenCalledTimes(1)`) so it can't false-pass when re-init is a no-op.	`aaeafb9`

On #5's residual (accepted, documented in-code): if a teardown fails and no later initialize() or streaming/transform op ever runs, the isolate lingers until process exit where the OS reclaims it — benign (single process-lifetime isolate, no ref-count violation). This is the deliberate tradeoff for not adding event-loop-affine async retry infrastructure to this concurrency-sensitive path.

#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 #3 → #4 → #5 arm-and-consume loop and confirmed the g_ref_count == Σ init_refs invariant holds at every g_mutex release, with no orphan-flag state, no premature clear, and no new deadlock (join-under-lock matches the existing Case-4 pattern).

mlischetti requested a review from a team as a code owner August 10, 2026 14:28

mlischetti and others added 27 commits August 19, 2026 10:25

W-23692110: handle-keyed ScriptRuntime registry with per-engine resol…

31dada3

…vers

W-23692110: per-engine resolver bridge and handle-based N-API methods

75f02df

W-23692110: document independent per-instance engines

fe9596b

chore: remove PR-157 code review process notes from repo

d13d472

These were internal review artifacts incidentally committed during remediation work (one references a local temp worktree path); they aren't product documentation and shouldn't ship in the repo.

Add process-global active-op accounting for streaming/transform

abd8fd0

Make napi_cleanup async: defer isolate teardown until active ops drain

423d2d9

Block initialize() while an isolate teardown is pending

7f68422

Change DataWeave.cleanup() to return Promise<void>

0c37175

Await the now-async DataWeave.cleanup() in existing tests

161083d

Add cleanup()-during-active-stream/transform deadlock regression tests

ba4a30c

Fix napi_initialize deadlock: decrement g_active_ops from the worker …

32376b2

…thread, not the JS-thread callback

Document DataWeave.cleanup()'s Promise<void> signature

e1f37f5

mlischetti and others added 9 commits August 20, 2026 17:13

mlischetti and others added 9 commits August 21, 2026 12:18

Merge remote-tracking branch 'origin/master' into w-23692110-multi-en…

212424d

…gine-design

mlischetti and others added 8 commits August 21, 2026 15:39

Navigation Menu

Uh oh!

W-23692110: Multiple isolated DataWeave engines per process (Node) - #157

W-23692110: Multiple isolated DataWeave engines per process (Node)#157
mlischetti wants to merge 112 commits into
masterfrom
w-23692110-multi-engine-design

mlischetti commented Aug 10, 2026 •

edited

Loading

Uh oh!

mlischetti commented Aug 11, 2026

Uh oh!

mlischetti commented Aug 20, 2026

Uh oh!

mlischetti commented Aug 21, 2026

Uh oh!

mlischetti commented Aug 21, 2026

Uh oh!

mlischetti commented Aug 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

mlischetti commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Design

Post-review hardening

Test plan

Uh oh!

mlischetti commented Aug 11, 2026

Uh oh!

mlischetti commented Aug 20, 2026

Round 12 — worker ref-leak & teardown-race hardening

Uh oh!

mlischetti commented Aug 21, 2026

Round 13 — per-env init-reference ownership (fixes review #5)

Uh oh!

mlischetti commented Aug 21, 2026

Round 14 — review #5 remediation (all 7 findings)

Uh oh!

mlischetti commented Aug 21, 2026

Round 15 — external review #6 remediation

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mlischetti commented Aug 10, 2026 •

edited

Loading