Skip to content

[Python] Preserve data channel failure causes - #40775

Merged
jrmccluskey merged 3 commits into
apache:masterfrom
bgeng777:xunguang-codex/fix-beam-python-data-channel-error-cause
Oct 26, 2026
Merged

jrmccluskey merged 3 commits into
apache:masterfrom
bgeng777:xunguang-codex/fix-beam-python-data-channel-error-cause

Conversation

@bgeng777

@bgeng777 bgeng777 commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #40774.

When a data channel closes after a read failure, input_elements hides the saved exception. Chain it as the cause of Channel closed prematurely; suppress the unrelated queue.Empty context when no cause exists.

Tests cover reader failure, premature EOF, normal completion and cancellation. All 9 data-plane tests pass; the two diagnostic regressions fail without the fix. Ruff and YAPF pass.

AI assistance: OpenAI Codex.

Chain premature-close errors from recorded reader failures so bundle
errors retain the original cause and omit unrelated queue timeouts.

Fixes apache#40774.

Generated-by: OpenAI Codex (GPT-6)
AI-Model: gpt-6

AI-Contributed/Feature: 3/3
AI-Contributed/UT: 50/50
@github-actions github-actions Bot added python runners size/s S: 10-49 lines changed labels Oct 25, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📋 Review Summary

This PR preserves the underlying data-plane exception as the explicit __cause__ when _GrpcDataChannel.input_elements raises RuntimeError('Channel closed prematurely.'), improving error diagnostics while suppressing the noisy queue.Empty context when no exception was recorded. The implementation is minimal, backwards-compatible, and well-covered by new unit tests.

🔍 General Feedback

  • Chaining from self._exception cleanly addresses the race/ordering between self._closed and self._exception in _read_inputs without altering existing exception types or abort semantics.
  • Consider mocking or shortening the queue poll timeout in the new unit tests, as 3 of the 4 added tests each wait on the hardcoded received.get(timeout=1) call (~3 seconds total).

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

Inject queue.Empty in the three timeout-driven tests instead of
waiting for each one-second queue poll. Keep the real reader and
consumer paths and their diagnostic assertions intact.

Generated-by: OpenAI Codex (GPT-6)
AI-Model: gpt-6

AI-Contributed/Feature: 0/0
AI-Contributed/UT: 11/11
Register the expected close failure with addCleanup and retain the
consumer cause assertion, removing redundant reader and close checks.

Generated-by: OpenAI Codex (gpt-6-astra)
AI-Model: gpt-6-astra

AI-Contributed/Feature: 0/0
AI-Contributed/UT: 27/27
@github-actions

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @jrmccluskey for label python.
Backup reviewer: shunping (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@codecov

codecov Bot commented Oct 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.90%. Comparing base (2213e54) to head (2e5ef38).
⚠️ Report is 23 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #40775       +/-   ##
===========================================
+ Coverage        0   79.90%   +79.90%     
===========================================
  Files           0      493      +493     
  Lines           0    83473    +83473     
===========================================
+ Hits            0    66696    +66696     
- Misses          0    16777    +16777     
Flag Coverage Δ
python 79.90% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jrmccluskey jrmccluskey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

@jrmccluskey
jrmccluskey merged commit 4550ac8 into apache:master Oct 26, 2026
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Python data channel masks saved reader exception with Channel closed prematurely

2 participants