fix: reserve LightGBM worker ports until network init - #2595
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Hey Rana Singh (@ranadeepsingh) 👋! We use semantic commit messages to streamline the release process. Examples of commit messages with semantic prefixes:
To test your commit locally, please follow our guild on building from source. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
c50cdbf to
be4b995
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR fixes a LightGBM training race where JVM-selected worker ports could be taken by another process between topology exchange / data preparation and the native LGBM_NetworkInit bind, by reserving each worker port until immediately before LightGBM binds it. It updates the LightGBM NetworkManager and task lifecycle cleanup to maintain reservations across initialization paths, and adds regression tests covering contention, cleanup, and concurrency.
Changes:
- Add port-reservation lifecycle to
NetworkTopologyInfoand wire it throughNetworkManagerso onlyBindExceptiontriggers port fallback and reservations are held untilLGBM_NetworkInit. - Ensure reservations are released on helper, failure, and task cleanup paths in
BasePartitionTask. - Add a new
NetworkManagerSuitewith deterministic and concurrent regression coverage for reservations, contention, and cleanup behavior.
Show a summary per file
| File | Description |
|---|---|
| lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/NetworkManager.scala | Introduces port reservation/retention/release and narrows fallback to BindException; releases reservation immediately before LGBM_NetworkInit. |
| lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/BasePartitionTask.scala | Extends reservation cleanup to more lifecycle paths (including initialization failures and outer task cleanup). |
| lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/NetworkManagerSuite.scala | Adds regression tests for reservation behavior, contention handling, close retryability, and concurrent reservation uniqueness. |
Review details
Suppressed comments (1)
lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/BasePartitionTask.scala:208
- If
getTaskContext(or later initialization logic) throws,networkInfo.releasePortReservation()can throw in thefinallyand mask the original initialization failure. Preserve the primary exception by suppressing close failures when unwinding due to another throwable, while still propagating close failures when initialization otherwise succeeds.
var initializationSucceeded = false
try {
// Return booster only from main worker to reduce network communication overhead
val shouldReturnBooster = if (isEmptyPartition) false
else if (!shouldExecuteTraining) false
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2595 +/- ##
==========================================
+ Coverage 86.07% 86.32% +0.25%
==========================================
Files 331 331
Lines 17654 17742 +88
Branches 1646 1660 +14
==========================================
+ Hits 15195 15316 +121
+ Misses 2459 2426 -33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
be4b995 to
b936f20
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Preserve primary task failures when port-reservation cleanup also fails, retry socket closure while the reservation remains reachable, and hold the exact advertised port during native network-init backoff. ## Prompting Intent Resolve the two independent medium findings on PR microsoft#2595: prevent cleanup from masking primary failures or losing open sockets, and eliminate the native-init retry race without broad fallback catches. Add focused suppression, cleanup-retry, exact-port backoff, and contention regressions. ## Linked Sources - Replacement pull request: microsoft#2595 - Original pull request: microsoft#2233 - Original issue: microsoft#2230 - Original unresolved review: microsoft#2233 (comment) ## Rationale Retry only IOException failures declared by Socket.close, while a tightly scoped Throwable-preservation helper immediately rethrows the protected operation and records cleanup failures as suppressed. Re-reserve only the already-advertised port between native attempts; if contention wins, fail safely with the native error primary so Spark can renegotiate instead of retrying an unreserved or different port. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Carry the preceding native-init failure through backoff-reservation release, suppress close failures onto it, and continue the retry when the socket ultimately closes. ## Prompting Intent Resolve the remaining medium review finding on PR microsoft#2595 without weakening cleanup reporting: a first close may fail while the immediate retry succeeds, so the native retry must proceed without replacing its original failure. ## Linked Sources - Replacement pull request: microsoft#2595 - Original pull request: microsoft#2233 - Original issue: microsoft#2230 ## Rationale Use an explicit internal overload to preserve the existing test seam and pass the prior native failure only across the recursive handoff. Continue only after synchronized reservation state confirms the socket closed; otherwise throw the native failure with aggregated close errors suppressed so no unsafe bind occurs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Re-review requested for commit The remaining retry-handoff issue is fixed: the preceding native-init failure is carried through reservation release, close failures are suppressed onto it, and retry continues only when the socket ultimately closed. Added a regression where the first close throws and the second succeeds. Local targeted/integration/style checks passed, and Azure build 229247623 plus all 76 PR checks are green. |
|
Pushed Added to
No production code changed. Locally: 15/15 Review threads: both outstanding Copilot comments about exception masking in On the retry design — I verified against LightGBM's This also matches a live customer-reported incident on Fabric (France Central, starter pool) where distributed LightGBM training fails with repeated |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Correction to my earlier comment. I previously wrote that IcM 51000001093267 ( The incident's only error signature is The incident traces to a separate defect: the driver serves the topology exchange once and then closes its server socket, so every Spark task retry gets This PR still fixes a real and distinct bug — the executor-side listen port race — and the two are complementary. Apologies for the earlier conflation. |
* ci: preserve sbt retry helper during release replay ## Summary Stage the sbt retry helper outside the repository before switching to Spark release branches, and parameterize the shared cache template so it can invoke that stable path after rebase. ## Prompting Intent Investigate why Spark 3.5 and Spark 4.1 compatibility checks still failed after PR #2608, reproduce the failure with PR #2595 changes, implement the complete hotfix, and validate the real release replay path. ## Linked Sources - Failing PR: #2595 - Prior identity hotfix: #2608 - Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229969678 ## Rationale The rebased working tree intentionally comes from the Spark release branch, so master-only CI helpers cannot remain repository-relative. Copying the helper to Agent.TempDirectory preserves release-specific dependency resolution and avoids moving cache warming ahead of the rebase, where exact cache hits could hide missing release dependencies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: replay only release-relevant PR changes ## Summary Replace commit-history rebasing with a three-way application of the synthetic PR merge tree's release-relevant patch onto each Spark release branch. ## Prompting Intent Validate the compatibility hotfix with PR #2595's real source changes while ensuring CI-only commits do not conflict with old Spark branches that predate the current pipeline and helper files. ## Linked Sources - Validation PR source: #2595 - Prior identity hotfix: #2608 - Failed Azure build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229969678 ## Rationale The compatibility job needs the effective PR content on the release tree, not unrelated CI and documentation commits. Building the patch from the synthetic merge commit preserves GitHub's merge result, handles source branches behind master, retains three-way conflict detection, and avoids requiring commit identity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: focus release compatibility on Spark 4.1 compilation ## Summary Remove the redundant Spark 3.5 release matrix leg and replace broad Spark 4.1 runtime suites with full test compilation of the effective PR patch. ## Prompting Intent Explain why the release compatibility jobs exist and keep fixing the failures exposed by validation PR #2610, accounting for master already targeting Spark 3.5. ## Linked Sources - Original compatibility PR: #2550 - Streamlining PR: #2583 - Integration validation PR: #2610 - Azure validation build: https://msdata.visualstudio.com/b9b2accc-2d1c-45b3-9d24-0eb5d78cc47f/_build/results?buildId=229984834 ## Rationale Normal PR validation already compiles and tests master on Spark 3.5, so replaying onto the older spark3.5 maintenance snapshot duplicates coverage and introduces unrelated JVM drift. Spark 4.1 test compilation catches cross-version source and test API breakage, while the existing master test fan-out supplies runtime coverage without rerunning broad, memory-heavy suites on a constrained compatibility agent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: SynapseML CI <synapseml-ci@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
## Summary Keep each JVM socket reservation alive through topology exchange and data preparation, then release it immediately before native LightGBM network initialization. Clean up training, helper, and failure paths and add focused contention and cleanup regressions. ## Prompting Intent Recreate the still-valid fix from ancient PR microsoft#2233 on current master, resolve issue microsoft#2230 without broad exception handling or silent fallback, preserve public API compatibility, and validate port competition and cleanup using TDD. ## Linked Sources - Original pull request: microsoft#2233 - Original issue: microsoft#2230 - Unresolved review comment: microsoft#2233 (comment) ## Rationale Store the reservation as transient internal state on the existing three-field topology type so callers remain source compatible. Advance to another port only for BindException; propagate other failures. Release at the unavoidable JVM-to-native handoff because the Java socket cannot be transferred to LightGBM, with final cleanup covering initialization, preparation, helper, and failure paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Preserve primary task failures when port-reservation cleanup also fails, retry socket closure while the reservation remains reachable, and hold the exact advertised port during native network-init backoff. ## Prompting Intent Resolve the two independent medium findings on PR microsoft#2595: prevent cleanup from masking primary failures or losing open sockets, and eliminate the native-init retry race without broad fallback catches. Add focused suppression, cleanup-retry, exact-port backoff, and contention regressions. ## Linked Sources - Replacement pull request: microsoft#2595 - Original pull request: microsoft#2233 - Original issue: microsoft#2230 - Original unresolved review: microsoft#2233 (comment) ## Rationale Retry only IOException failures declared by Socket.close, while a tightly scoped Throwable-preservation helper immediately rethrows the protected operation and records cleanup failures as suppressed. Re-reserve only the already-advertised port between native attempts; if contention wins, fail safely with the native error primary so Spark can renegotiate instead of retrying an unreserved or different port. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Summary Carry the preceding native-init failure through backoff-reservation release, suppress close failures onto it, and continue the retry when the socket ultimately closes. ## Prompting Intent Resolve the remaining medium review finding on PR microsoft#2595 without weakening cleanup reporting: a first close may fail while the immediate retry succeeds, so the native retry must proceed without replacing its original failure. ## Linked Sources - Replacement pull request: microsoft#2595 - Original pull request: microsoft#2233 - Original issue: microsoft#2230 ## Rationale Use an explicit internal overload to preserve the existing test seam and pass the prior native failure only across the recursive handoff. Continue only after synchronized reservation state confirms the socket closed; otherwise throw the native failure with aggregated close errors suppressed so no unsafe bind occurs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rSuite Adds four regression tests for the error paths introduced by the port reservation handoff that had no coverage: - Native init is never invoked when the advertised port cannot be released before LGBM_NetworkInit; the close failure propagates with the retry close failure suppressed. - A native-init retry whose backoff reservation cannot be closed aborts with the original native failure (close failure suppressed) rather than binding a port that is still held. - Port scanning stops after its 1k-port contention window and never scans past MaxPort. - Out-of-range base ports are rejected before any socket is allocated. No production code changes: this only closes the patch-coverage gap in NetworkManager's release/retry paths. All 15 NetworkManagerSuite tests and lightgbm scalastyle (main + test) pass locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1272df23-c4f3-4048-8789-0320ea286628
438de70 to
5dab71c
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (1)
lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/NetworkManager.scala:467
- The exception text here is misleading: this branch is reached because the scan exceeded
MaxPort, not becausebasePortitself is out of range. Consider rewording the message to reflect port exhaustion so users can distinguish it from a misconfigured base port.
if (nextPort > LightGBMConstants.MaxPort) {
throw new Exception(s"Error: port $basePort out of range, " +
"possibly due to networking or firewall issues")
}
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Summary
LGBM_NetworkInitbinds the native listenerBindExceptionas port contention; propagate configuration and cleanup failures instead of silently falling backNetworkTopologyInfoAPIWhy this replaces the ancient PR
This is a current-master recreation of the still-valid race fix proposed in #2233 for #2230. The original PR is based on an ancient contributor branch and retained an unresolved broad
Exceptioncatch around socket cleanup. This implementation was rewritten against currentNetworkManager/BasePartitionTask, narrows fallback to actual bind contention, and adds the missing lifecycle and concurrency coverage. It does not close or modify the original PR.Fixes #2230
Validation
lightgbm/compilelightgbm/testOnly com.microsoft.azure.synapse.ml.lightgbm.split1.NetworkManagerSuite(5 tests)lightgbm/testOnly com.microsoft.azure.synapse.ml.lightgbm.split3.VerifyLightGBMClassifierStreamBasic -- -z "PimaIndian.csv"(2 tests; includes helper-task startup)scalastyleandtest:scalastyleacross all modulesblack==22.3.0 --check --extend-exclude 'docs/' .