Draft
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bex_engineby storing handles instead of rawHeapPtrsset_future_ready/fulfil_futureearly_yieldregression test that forces allocation pressure beforebaml.sys.sleep(1)completesRoot cause
The engine was carrying raw
HeapPtrs for async futures outside the VM while the VM could hit a GC safepoint duringAwait. If GC moved the future object before the async sys-op completed, the engine later calledfulfil_futurewith a stale pointer. That stale pointer could then drop the wrongPendingFuture, which matches the nondeterministic libmalloc abort seen in the VM double-free repro.Impact
This removes a GC-sensitive use-after-move in the async future fulfillment path. The practical effect is that long-running async/sys-op flows should stop aborting when a future completes after GC has run.
Validation
cargo test -p bex_engine --manifest-path baml_language/Cargo.toml --test early_yield -- --nocapturecargo test -p bex_engine --manifest-path baml_language/Cargo.toml --test early_yield sleep_future_survives_gc_safepoint -- --nocapturecargo build -p baml_cli --manifest-path baml_language/Cargo.tomlbaml-cli runrepro that allocates, awaitsbaml.sys.sleep(1), and completed successfully across repeated runsNote
Medium Risk
Touches the engine’s async/await fulfillment path and GC coordination by changing how future identities are tracked, which could impact correctness under load or cancellation. Changes are localized and include a regression test covering the previously failing GC-at-
Awaitscenario.Overview
Fixes async sys-op future completion across GC safepoints by storing a GC-stable
Handle(not a rawHeapPtr) for in-flight futures and resolving the current pointer right beforeset_future_ready/fulfil_future.Adds a new engine error (
FutureHandleInvalid) for cases where a future handle can no longer be resolved, and introduces anearly_yieldregression test (sleep_future_survives_gc_safepoint) that forces allocation pressure before awaitingbaml.sys.sleep(1)to ensure futures survive a GC atAwait.Reviewed by Cursor Bugbot for commit 933b2af. Bugbot is set up for automated code reviews on this repo. Configure here.