Skip to content

Fix async await exception propagation#386

Open
tusharhqq wants to merge 2 commits intopydantic:mainfrom
tusharhqq:codex/fixing-async_exec
Open

Fix async await exception propagation#386
tusharhqq wants to merge 2 commits intopydantic:mainfrom
tusharhqq:codex/fixing-async_exec

Conversation

@tusharhqq
Copy link
Copy Markdown

Summary

  • Fixes External async functions exceptions are not correctly caught inside the sandbox #323.
  • Route failed awaited futures back through normal VM exception handling so try/except around direct await expressions can catch them.
  • Preserve the original exception for asyncio.gather(...) failures instead of surfacing sibling task cancellation.
  • Add regression coverage for direct future failure, direct gather failure, and gathered coroutine failure.

Testing

  • PATH="$HOME/.cargo/bin:$PATH" make format-rs
  • make dev-py
  • uv run pytest crates/monty-python/tests/test_start.py::test_future_snapshot_resume_exception_caught_at_await crates/monty-python/tests/test_start.py::test_future_snapshot_resume_gather_exception_caught_at_await crates/monty-python/tests/test_start.py::test_future_snapshot_resume_gathered_coroutine_exception_caught_at_await -q
  • uv run pytest crates/monty-python/tests/test_start.py -q
  • make lint-rs
  • make lint-py
  • uv run pytest crates/monty-python/tests/test_async.py -q

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 22, 2026

Merging this PR will not alter performance

✅ 16 untouched benchmarks
⏩ 15 skipped benchmarks1


Comparing tusharhqq:codex/fixing-async_exec (3265538) with main (5c7cf2b)

Open in CodSpeed

Footnotes

  1. 15 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2026

Codecov Report

❌ Patch coverage is 78.12500% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/monty/src/bytecode/vm/async_exec.rs 75.00% 3 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3265538e7b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +802 to 805
let gather_mut = gather.get_mut(self.heap);
let task_ids = mem::take(&mut gather_mut.task_ids);
let waiter = gather_mut.waiter;
// Drop the HeapRead before cancel_task which may free heap objects
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove gather waiters before dropping failed gather

When a gathered coroutine fails in fail_future, this branch only takes task_ids/waiter and then later dec_refs the gather, but it never removes gather.pending_calls from scheduler.gather_waiters. In a mixed asyncio.gather(...) (coroutine + direct external future), if the host resumes with multiple results in one FutureSnapshot.resume call and processes the coroutine failure first, a later result for the sibling external future still hits the stale gather_waiters entry and resolve_future will try to heap.read a freed gather, causing a panic.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@tusharhqq
Copy link
Copy Markdown
Author

i will complete this pr by tomorrow, i am still working on it, i needed feedback from codex and devin, that's why i created the pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

External async functions exceptions are not correctly caught inside the sandbox

1 participant