Repository navigation
Fix unhandled spawn error reporting - #4129
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis change replaces parent-level fire-and-forget error propagation with GC-based unhandled-spawn reporting, adds engine shutdown coordination, extends the CFFI ABI, integrates callback handling across host SDKs, and updates CLI and runtime tests. ChangesUnhandled spawn lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SpawnedFuture
participant BexHeap
participant BexEngine
participant CFFIBridge
participant HostSDK
SpawnedFuture->>BexHeap: settle unobserved error
BexHeap->>BexHeap: preserve error during GC
BexHeap->>BexEngine: drain unhandled spawn error
BexEngine->>CFFIBridge: encode and dispatch error payload
CFFIBridge->>HostSDK: invoke registered callback
HostSDK->>HostSDK: report or raise host-native error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Binary size checks passed✅ 7 passed
Generated by |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
baml_language/crates/bridge_cffi/tests/unhandled_spawn.rs (1)
7-14: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the forwarded payload and cancellation metadata.
The test only verifies that one callback occurred, drops
_cancelled, and never validates theBamlOutboundResultbytes. A malformed payload or incorrect cancellation flag would still pass. Capture the flag, validate/decode the payload, and add a shutdown-cancellation case if needed.Also applies to: 17-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bridge_cffi/tests/unhandled_spawn.rs` around lines 7 - 14, Update the unhandled-spawn test callback capture and assertions around capture to retain the cancellation flag alongside each payload, decode the forwarded BamlOutboundResult bytes, and assert their expected contents and metadata rather than only checking callback count. Add coverage for shutdown cancellation, asserting the callback reports the expected cancellation value and payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/bridge_cffi/src/ffi/unhandled_spawn.rs`:
- Around line 14-34: Update the callback state management in the registration
function and dispatch so callback registration, pending-message draining, and
queueing are coordinated under one shared lock, preventing messages from being
enqueued after the queue is drained. Capture the callback and pending messages
while holding the lock, release it before invoking callbacks, and preserve
existing callback payload behavior. Add a Rust unit test covering the
registration/dispatch handoff and run cargo test --lib.
In `@baml_language/crates/bridge_cffi/src/lib_wasm.rs`:
- Around line 13-31: Add a host-facing drain for UNHANDLED_SPAWN_ERRORS in
lib_wasm.rs, integrating it with the existing callback registration or dispatch
path so queued payloads are reported and removed. Ensure pending errors are
drained when the host callback becomes available and also released during
runtime shutdown, matching the native fallback behavior.
In `@baml_language/sdks/cpp/bridge_cpp/include/baml/runtime.h`:
- Around line 61-71: Update baml_cpp_unhandled_spawn_error_trampoline to call
detail::report_unhandled_spawn_error synchronously instead of creating a
detached thread, preserving payload construction and cancelled conversion.
Explicitly guard the FFI boundary so any exception from reporting is contained
and cannot unwind through the extern "C" function.
In `@baml_language/sdks/go/baml_go/runtime.go`:
- Line 90: Update the initialization failure path immediately after
nativeRegisterUnhandledSpawnErrorCallback so nativeShutdown() is called before
nativeCloseAfterLoadFailure() on every post-registration failure path. Preserve
the callback registration order and ensure no registered callback remains active
when the native library is closed.
In `@baml_language/sdks/python/src/baml_bridge/baml_py.pyi`:
- Line 68: Update the cancellation example around call_function to pass the
attached _ctx variable as the ctx argument instead of ctx, ensuring
BamlCallContext.abort() can cancel the in-flight call.
In `@baml_language/sdks/typescript/bridge_typescript/src/unhandled_spawn.rs`:
- Around line 10-11: Update the shutdown callback delivery associated with
Callback and its deliver path to use ThreadsafeFunctionCallMode::Blocking rather
than NonBlocking, while preserving the weak tsfn configuration for the normal
idle/no-error case so shutdownRuntime waits for the unhandled-spawn report to be
delivered.
---
Nitpick comments:
In `@baml_language/crates/bridge_cffi/tests/unhandled_spawn.rs`:
- Around line 7-14: Update the unhandled-spawn test callback capture and
assertions around capture to retain the cancellation flag alongside each
payload, decode the forwarded BamlOutboundResult bytes, and assert their
expected contents and metadata rather than only checking callback count. Add
coverage for shutdown cancellation, asserting the callback reports the expected
cancellation value and payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d84bb99a-6487-486d-bebf-f894dc80baa7
⛔ Files ignored due to path filters (14)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/sdks/typescript/bridge_typescript/dist/exit_hook.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/typescript/bridge_typescript/dist/exit_hook.jsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/exit_hook.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/typescript/bridge_typescript/dist/index.d.tsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/typescript/bridge_typescript/dist/index.jsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/index.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/typescript/bridge_typescript/dist/native.d.tsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/native.jsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/unhandled_spawn.d.tsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/unhandled_spawn.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/typescript/bridge_typescript/dist/unhandled_spawn.jsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/unhandled_spawn.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (56)
baml_language/crates/baml_tests/tests/spawn_semantics.rsbaml_language/crates/bex_engine/Cargo.tomlbaml_language/crates/bex_engine/src/future.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/src/thread.rsbaml_language/crates/bex_engine/tests/fire_and_forget.rsbaml_language/crates/bex_engine/tests/prof_gate.rsbaml_language/crates/bex_heap/src/gc.rsbaml_language/crates/bex_heap/src/heap.rsbaml_language/crates/bex_heap/src/heap_debugger/real.rsbaml_language/crates/bex_heap/src/lib.rsbaml_language/crates/bex_project/src/bex.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_vm/src/package_baml/future.rsbaml_language/crates/bex_vm/src/vm.rsbaml_language/crates/bex_vm_types/src/types/future.rsbaml_language/crates/bridge_cffi/Cargo.tomlbaml_language/crates/bridge_cffi/cbindgen.tomlbaml_language/crates/bridge_cffi/include/baml_cffi.hbaml_language/crates/bridge_cffi/src/api.rsbaml_language/crates/bridge_cffi/src/baml_to_host.rsbaml_language/crates/bridge_cffi/src/ffi/mod.rsbaml_language/crates/bridge_cffi/src/ffi/runtime.rsbaml_language/crates/bridge_cffi/src/ffi/unhandled_spawn.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_cffi/src/lib_native.rsbaml_language/crates/bridge_cffi/src/lib_wasm.rsbaml_language/crates/bridge_cffi/tests/abi_assertions.hbaml_language/crates/bridge_cffi/tests/abi_layout.cbaml_language/crates/bridge_cffi/tests/abi_layout.rsbaml_language/crates/bridge_cffi/tests/unhandled_spawn.rsbaml_language/sdks/cpp/bridge_cpp/include/baml/detail/loader.hbaml_language/sdks/cpp/bridge_cpp/include/baml/runtime.hbaml_language/sdks/cpp/bridge_cpp/tests/runtime_smoke.ccbaml_language/sdks/go/baml_go/internal/cffi/include/baml_cffi.hbaml_language/sdks/go/baml_go/native_unix.gobaml_language/sdks/go/baml_go/native_unsupported.gobaml_language/sdks/go/baml_go/native_windows.gobaml_language/sdks/go/baml_go/runtime.gobaml_language/sdks/go/baml_go/runtime_test.gobaml_language/sdks/java/baml_bridge/src/main/java/baml_bridge/BamlFfi.javabaml_language/sdks/java/baml_bridge/src/main/java/baml_bridge/UnhandledSpawnErrors.javabaml_language/sdks/java/baml_bridge/src/test/java/baml_bridge/UnhandledSpawnErrorsTest.javabaml_language/sdks/java/bridge_java/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/lib.rsbaml_language/sdks/python/rust/bridge_python/src/unhandled_spawn.rsbaml_language/sdks/python/src/baml_bridge/__init__.pybaml_language/sdks/python/src/baml_bridge/baml_py.pyibaml_language/sdks/python/tests/test_engine.pybaml_language/sdks/typescript/bridge_typescript/src/lib.rsbaml_language/sdks/typescript/bridge_typescript/src/unhandled_spawn.rsbaml_language/sdks/typescript/bridge_typescript/tests/unhandled_spawn.test.tsbaml_language/sdks/typescript/bridge_typescript/typescript_src/exit_hook.tsbaml_language/sdks/typescript/bridge_typescript/typescript_src/index.tsbaml_language/sdks/typescript/bridge_typescript/typescript_src/native.d.tsbaml_language/sdks/typescript/bridge_typescript/typescript_src/unhandled_spawn.ts
💤 Files with no reviewable changes (1)
- baml_language/crates/bex_heap/src/heap_debugger/real.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/sdks/typescript/bridge_typescript/tests/unhandled_spawn.test.ts (1)
21-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStrengthen the child-process oracle.
The test never verifies that
maincompleted successfully, so any root-call failure can satisfy the expected non-zero exit. Also,spawnSyncreturnsstatus === nullon timeout or signal termination, whichnot.toBe(0)accepts.Emit/assert a success marker after
result()and verifychild.error/child.signalare clear before asserting a numeric non-zero exit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/typescript/bridge_typescript/tests/unhandled_spawn.test.ts` around lines 21 - 30, Strengthen the child-process assertions around the spawned script: emit a success marker only after callFunctionSync(...).result() completes, then assert the marker is present. Before checking the exit code, assert child.error and child.signal are clear, and require child.status to be a numeric non-zero value so timeouts or signal termination cannot satisfy the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@baml_language/sdks/typescript/bridge_typescript/tests/unhandled_spawn.test.ts`:
- Around line 21-30: Strengthen the child-process assertions around the spawned
script: emit a success marker only after callFunctionSync(...).result()
completes, then assert the marker is present. Before checking the exit code,
assert child.error and child.signal are clear, and require child.status to be a
numeric non-zero value so timeouts or signal termination cannot satisfy the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c055555e-293e-4b2f-b6bd-a4c391cc7606
⛔ Files ignored due to path filters (4)
baml_language/sdks/typescript/bridge_typescript/dist/index.d.tsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/typescript/bridge_typescript/dist/index.jsis excluded by!**/dist/**baml_language/sdks/typescript/bridge_typescript/dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (12)
baml_language/crates/baml_tests/tests/cancel_cascade.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bridge_cffi/src/ffi/unhandled_spawn.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_cffi/src/lib_wasm.rsbaml_language/sdks/cpp/bridge_cpp/include/baml/runtime.hbaml_language/sdks/go/baml_go/runtime.gobaml_language/sdks/python/rust/bridge_python/src/baml_call_context.rsbaml_language/sdks/python/src/baml_bridge/baml_py.pyibaml_language/sdks/typescript/bridge_typescript/src/unhandled_spawn.rsbaml_language/sdks/typescript/bridge_typescript/tests/unhandled_spawn.test.tsbaml_language/sdks/typescript/bridge_typescript/typescript_src/index.ts
💤 Files with no reviewable changes (2)
- baml_language/sdks/typescript/bridge_typescript/typescript_src/index.ts
- baml_language/crates/bridge_cffi/src/lib_wasm.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- baml_language/sdks/go/baml_go/runtime.go
- baml_language/sdks/typescript/bridge_typescript/src/unhandled_spawn.rs
- baml_language/crates/bridge_cffi/src/lib.rs
- baml_language/sdks/python/src/baml_bridge/baml_py.pyi
- baml_language/crates/bex_engine/src/lib.rs
…wn-errors # Conflicts: # baml_language/crates/bex_project/src/lib.rs # baml_language/sdks/go/baml_go/native_unix.go # baml_language/sdks/go/baml_go/native_unsupported.go # baml_language/sdks/go/baml_go/native_windows.go # baml_language/sdks/go/baml_go/runtime.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 2196-2208: The panic recovery in dispatch_unhandled_spawn_errors
must distinguish GC-triggered delivery from normal shutdown delivery. When
collect_garbage invokes it, catch handler panics, record or log the panic, and
do not call resume_unwind; preserve or explicitly discard the queued error so GC
completes without unwinding. Keep existing panic propagation for shutdown or
other paths where it remains required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef156a5c-105a-476b-b99c-3747cc142ced
⛔ Files ignored due to path filters (1)
baml_language/crates/baml_tests/snapshots/baml_src/spawn_throws.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
baml_language/crates/baml_cli/src/run_command.rsbaml_language/crates/baml_cli/src/test_command.rsbaml_language/crates/baml_pack_host/src/main.rsbaml_language/crates/baml_tests/baml_src/ns_spawn_throws/spawn_throws.bamlbaml_language/crates/bex_engine/src/future.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bex_engine/tests/fire_and_forget.rsbaml_language/crates/bex_heap/src/gc.rsbaml_language/crates/bex_heap/src/heap.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bex_project/src/project.rsbaml_language/crates/bex_vm_types/src/types/future.rsbaml_language/crates/bridge_cffi/src/baml_to_host.rsbaml_language/crates/bridge_cffi/src/lib.rsbaml_language/crates/bridge_cffi/tests/unhandled_spawn.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- baml_language/crates/bex_project/src/lib.rs
- baml_language/crates/bridge_cffi/tests/unhandled_spawn.rs
- baml_language/crates/bridge_cffi/src/baml_to_host.rs
- baml_language/crates/bridge_cffi/src/lib.rs
- baml_language/crates/bex_heap/src/gc.rs
- baml_language/crates/bex_engine/tests/fire_and_forget.rs
- baml_language/crates/bex_vm_types/src/types/future.rs
- baml_language/crates/bex_engine/src/future.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
baml_language/sdks/java/bridge_java/src/lib.rs (2)
160-170: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a shutdown timeout for
nativeShutdownRuntime.
nativeShutdownRuntimeblocks the Java shutdown-hook thread onruntime.block_on(bridge_cffi::shutdown_runtime()), andshutdown_runtime()awaitsruntime.shutdown()before draining unhandled spawn errors. If cleanup work can wait on arbitrary spawned futures, this can leave the hook never exiting; bound the wait and propagate any timeout instead of blocking indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/java/bridge_java/src/lib.rs` around lines 160 - 170, Update Java_baml_1bridge_BamlFfi_nativeShutdownRuntime to bound the runtime.block_on shutdown operation with a timeout, including the subsequent cleanup performed by bridge_cffi::shutdown_runtime. Propagate a timeout as an error through the existing throw_runtime_exception path, while preserving successful shutdown behavior.
160-170: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNo Rust tests to update; add
cargo test --libfor the Rust SDK crate.This package build artifact is a shared library, not a Rust test target. Run
cargo test --libfor any associated Rust tests in this project/package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdks/java/bridge_java/src/lib.rs` around lines 160 - 170, Update the Rust SDK package validation to run the library test target with cargo test --lib, rather than treating the shared-library artifact as a standalone test target. No Rust test changes are required; ensure the package’s test configuration or verification instructions invoke cargo test --lib for associated tests.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@baml_language/sdks/java/bridge_java/src/lib.rs`:
- Around line 160-170: Update Java_baml_1bridge_BamlFfi_nativeShutdownRuntime to
bound the runtime.block_on shutdown operation with a timeout, including the
subsequent cleanup performed by bridge_cffi::shutdown_runtime. Propagate a
timeout as an error through the existing throw_runtime_exception path, while
preserving successful shutdown behavior.
- Around line 160-170: Update the Rust SDK package validation to run the library
test target with cargo test --lib, rather than treating the shared-library
artifact as a standalone test target. No Rust test changes are required; ensure
the package’s test configuration or verification instructions invoke cargo test
--lib for associated tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 381c46eb-e8d5-4b86-b68a-1f114c5dbe34
📒 Files selected for processing (4)
baml_language/sdks/cpp/bridge_cpp/include/baml/runtime.hbaml_language/sdks/java/bridge_java/src/lib.rsbaml_language/sdks/python/src/baml_bridge/__init__.pybaml_language/sdks/python/tests/test_engine.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_cli/src/test_command.rs`:
- Around line 616-619: Restructure the list-mode flow around collect_tests, the
listing logic, and the cache-verification block so finish_engine(&run_ctx) runs
before any discovery cache write or return. Evaluate shutdown errors
consistently, return TestFailure when finishing fails, and ensure every
list-mode success or error path shuts down the engine before returning,
preventing failed discovery results from being cached.
In `@baml_language/sdks/python/tests/test_engine.py`:
- Around line 109-117: Update the subprocess test’s post-run fallback after the
expected termination check to exit with a distinct sentinel status instead of
raising AssertionError, while retaining the assertion that os._exit(1) produces
return code 1. This ensures a handler that prints “boom” and returns cannot
satisfy the test accidentally.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df43eac7-a714-47a9-8596-d91a8e7221ce
📒 Files selected for processing (4)
baml_language/crates/baml_cli/src/test_command.rsbaml_language/crates/baml_cli/tests/exit_code_e2e.rsbaml_language/sdks/python/src/baml_bridge/__init__.pybaml_language/sdks/python/tests/test_engine.py
…wn-errors # Conflicts: # baml_language/.cargo/size-gate.toml # baml_language/.ci/size-gate/aarch64-apple-darwin.toml # baml_language/.ci/size-gate/x86_64-pc-windows-msvc.toml # baml_language/crates/baml_cli/src/test_command.rs
Issue Reference
Addresses B-405.
Changes
Testing
PR Checklist
Summary by CodeRabbit