[damccorm]

Summary

Fixes PostCommit XVR JavaUsingPython Dataflow (beam_PostCommit_XVR_JavaUsingPython_Dataflow (3.14)) failure observed on master in run 42212921122.

Root Cause

1. In sdks/python/scripts/run_expansion_services.sh, start launched the Java and Python expansion services in the background and returned immediately after kill -0, before the Python expansion service finished importing apache_beam and binding to its gRPC port (~38 seconds on Python 3.14 when 8 test workers start concurrently). Additionally, child processes inherited file descriptor 200 (flock), holding the lock open until the services stopped.
2. In BaseExternalTest.waitForReady(), ManagedChannelBuilder.forTarget(expansionAddr).build() omitted .usePlaintext(), did not call channel.resetConnectBackoff() on TRANSIENT_FAILURE, and only waited 15 seconds (30 * 500ms). As a result, the first 5 of 20 ValidateRunnerXlangTest tests failed immediately with StatusRuntimeException: UNAVAILABLE: io exception / Connection refused: localhost/127.0.0.1:41321, while the remaining 15 tests passed once the service finished binding.

Changes

• Close FD 200 (200>&-) when launching backgrounded expansion services in run_expansion_services.sh and poll until both Java and Python expansion services are listening on their ports (up to 60s, failing fast with || exit 1 if startup fails).
• Add .usePlaintext(), resetConnectBackoff(), and a 60s readiness timeout (120 * 500ms) in BaseExternalTest.waitForReady().

Verification

• Triggered PostCommit XVR JavaUsingPython Dataflow workflow on branch fix/xvr-expansion-service-readiness: https://github.com/apache/beam/actions/runs/42258219149 (previous run https://github.com/apache/beam/actions/runs/42235263937 succeeded)

[github-actions]

📋 Review Summary

This PR fixes startup race conditions in cross-language ValidatesRunner tests by closing the leaked flock file descriptor (200) in child processes, polling for both expansion services to bind their ports in run_expansion_services.sh, and updating BaseExternalTest.waitForReady() to use plaintext channels, reset connection backoff, and wait up to 60 seconds.

🔍 General Feedback

• The root cause analysis and changes in BaseExternalTest.java and run_expansion_services.sh directly address the startup flakiness.
• In sdks/python/scripts/run_expansion_services.sh, the return status of wait_for_port is currently not checked and gets overwritten by the trailing flock -u 200 command, which prevents the script from failing fast with a non-zero exit code when an expansion service crashes or times out.

---

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

[damccorm]

/gemini review

[github-actions]

📋 Review Summary

This PR resolves startup race conditions in cross-language (XVR) tests by closing the inherited flock file descriptor in backgrounded expansion service processes, polling until the expansion service ports are listening, and fixing BaseExternalTest.waitForReady() to use plaintext gRPC with backoff resets and a 60s timeout.

🔍 General Feedback

• The gRPC readiness polling and resource cleanup updates in BaseExternalTest.java are clean and well-structured.
• In sdks/python/scripts/run_expansion_services.sh, wait_for_port failures are currently swallowed because set -e is not enabled and the trailing flock -u 200 resets the script's exit status to 0.

---

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

[damccorm]

/gemini review

[github-actions]

📋 Review Summary

This PR resolves a race condition in Cross-Language ValidatesRunner (XVR) tests by ensuring both Java and Python expansion services are actively listening on their gRPC ports before test execution proceeds, and by fixing file descriptor inheritance for flock. The updates to run_expansion_services.sh (including failing fast via || exit 1 on startup/readiness failure) and BaseExternalTest.waitForReady() (adding .usePlaintext(), backoff resets, and proper channel cleanup) are clean, well-targeted, and ready to merge.

🔍 General Feedback

• Closing FD 200 (200>&-) on backgrounded Java and Python expansion service processes properly prevents child processes from holding the flock lock open across the lifetime of the daemons.
• Adding || exit 1 to the wait_for_port checks ensures startup failures are not masked by the trailing flock -u 200 command.
• Configuring .usePlaintext(), calling resetConnectBackoff() on TRANSIENT_FAILURE, and ensuring channel.shutdownNow() runs in a finally block while restoring the thread interrupt flag in BaseExternalTest.waitForReady() makes the Java client readiness check much more resilient.

---

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.32%. Comparing base (3dd07de) to head (c0c09bd).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #41035      +/-   ##
============================================
+ Coverage     58.29%   58.32%   +0.03%     
  Complexity     2276     2276              
============================================
  Files          1136     1137       +1     
  Lines        169395   169512     +117     
  Branches       1482     1482              
============================================
+ Hits          98746    98867     +121     
+ Misses        68100    68096       -4     
  Partials       2549     2549              

Flag	Coverage Δ
python	80.19% <ø> (+0.03%)	⬆️

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.

[github-actions]

Assigning reviewers:

R: @jrmccluskey for label python.
Backup reviewer: claudevdm (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).

[jrmccluskey]

LGTM