Skip to content

fix(asyncio): preserve producer failures while draining consumers - #4490

Closed
sylvesterkaczmarek wants to merge 4 commits into
openai:mainfrom
sylvesterkaczmarek:fix/producer-consumer-primary-error
Closed

fix(asyncio): preserve producer failures while draining consumers#4490
sylvesterkaczmarek wants to merge 4 commits into
openai:mainfrom
sylvesterkaczmarek:fix/producer-consumer-primary-error

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown
Contributor

Summary

  • preserve the producer exception when the consumer also fails during producer-failure draining
  • continue waiting for the consumer task to settle before propagating the producer failure
  • keep consumer-first failure and parent-cancellation behaviour unchanged

run_producer_consumer() documents producer failures as primary while the consumer drains. Previously a secondary consumer exception raised during that drain replaced the producer exception.

Test plan

  • added focused regression coverage in tests/test_asyncio_tasks_primary_error.py
  • GitHub Actions

Issue number

N/A

@sylvesterkaczmarek
sylvesterkaczmarek marked this pull request as ready for review August 17, 2026 20:01
@seratch

seratch commented Aug 17, 2026

Copy link
Copy Markdown
Member

I traced run_producer_consumer() through its two current callers. Both dispatchers already catch and log ordinary Exceptions from on_stream, so the test's ConsumerError(Exception) cannot reach this helper through the supported Agent.as_tool() or Codex tool paths. The consumer failures that intentionally escape are BaseExceptions such as CancelledError; preserving that behavior was part of the #4198 and #4201 fix.

In a controlled Agent.as_tool(on_stream=...) interleaving, the released code propagates CancelledError, while this patch replaces it with the producer RuntimeError. With the default failure handler, that becomes a normal tool-error result and can let the outer run continue. Since no supported scenario demonstrates why producer failure should override consumer cancellation, I do not think this change is worth merging. I am going to close this PR for now.

@seratch seratch closed this Aug 17, 2026
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.

2 participants