Skip to content

fix: preserve the real cause when a LightGBM task retry cannot rejoin the network - #2612

Merged
Rana Singh (ranadeepsingh) merged 9 commits into
microsoft:masterfrom
ranadeepsingh:fix/lightgbm-driver-socket-retry
Aug 12, 2026
Merged

fix: preserve the real cause when a LightGBM task retry cannot rejoin the network#2612
Rana Singh (ranadeepsingh) merged 9 commits into
microsoft:masterfrom
ranadeepsingh:fix/lightgbm-driver-socket-retry

Conversation

@ranadeepsingh

@ranadeepsingh Rana Singh (ranadeepsingh) commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Problem

Distributed LightGBM training exchanges network topology with the driver exactly once per training round, then closes driverServerSocket. Any Spark task retried after that point reconnects to an endpoint that is no longer listening, so every retry fails with a bare java.net.ConnectException: Connection refused.

Because Spark only reports the most recent attempt, the failure that actually caused the retry is never surfaced:

org.apache.spark.SparkException: Job aborted due to stage failure: Task 4 in stage 103.0 failed 4 times,
most recent failure: Lost task 4.3 in stage 103.0 (TID 1954) (vm-... executor 2):
java.net.ConnectException: Connection refused

This makes such failures undiagnosable from the job output alone. It is reproduced end to end in DriverSocketRetryE2ESuite, which fails one task after it completes the driver handshake and produces exactly the signature above on unpatched code.

Barrier execution mode is affected too, and more seriously: a barrier stage restarts in its entirety, which is the only recovery a fixed LightGBM network can support — but the restarted stage also found the port closed, so barrier mode could not actually recover from anything.

Changes

Stop masking the real cause — a terminal ConnectException in getGlobalNetworkInfo is translated into an actionable error. On a retry it states that the topology exchange is already closed, that a partial task retry can never rejoin the network, and that the real cause is in the logs of the first failed attempt of that partition. On a first attempt it points at driver reachability instead.

Make barrier stage restarts work — the driver now keeps serving topology rounds in barrier mode until training is done. Task messages carry the Spark stage attempt number so the driver can tell attempts apart: a newer attempt discards the previous attempt's partial topology, and a straggler from a superseded attempt is rejected. This matters because a topology containing a dead host:port makes every surviving task hang trying to connect to it. Message parsing accepts both the old and new formats. A barrier round completes only after the current attempt has both its finished marker and every unique partition report; retried reports replace the prior socket instead of inflating the count.

Preserve the public message API — stage-attempt metadata is carried by a private wire message while TaskMessageInfo keeps its public five-field constructor, extractor, product shape, and string representation.

Fix the socket and thread lifecycle

  • The driver network thread closes its sockets in a finally, so the server socket is no longer leaked when a round throws or times out.
  • closeConnections is idempotent and synchronized, closes each socket independently, and closes the accepted-but-not-yet-registered socket plus the load-only helper sockets that were leaked on every fit().
  • LightGBMBase.executeTraining closes the network manager in a finally. Previously a failure in the partition tasks left the network thread parked in accept() holding the driver port for up to the configured timeout (20 minutes by default).
  • The barrier finished-marker connection is closed rather than leaked.
  • Cleanup warnings retain the throwable and stack trace.

Algorithm and performance

No change to the training algorithm, the LightGBM parameters, or the data path. The new error handling only runs on a path that was already failing. Per-report work is a constant-time partition-map update; topology strings are built once per completed round. getGlobalNetworkInfo runs in initialize, before preparePartitionData, so a failing retry costs ~800ms and does not redo data preparation.

Tests

Suite Covers
DriverSocketRetryE2ESuite Full Spark job: a task fails after the handshake; asserts the reported error explains the cascade instead of Connection refused
DriverSocketRetrySuite Refused retry, duplicate partition replacement, public message compatibility, helper socket release, idempotent close, unexpected close, and accept/shutdown races
BarrierNetworkRecoverySuite Restarted stage isolation, stale markers, both marker/report orderings, and duplicate reports before or after completion signaling

Validated locally: 68 tests across 8 suites pass, including VerifyLightGBMClassifierStreamBasic (barrier mode), VerifyLightGBMRegressorBulk, VerifyLightGBMRegressorStream and VerifyLightGBMRankerBulk. Latest focused validation: 19/19 socket lifecycle and barrier recovery tests. scalastyle and Test/scalastyle are clean.

Related

Distinct from #2595, which fixes a native executor-to-executor listen port race. That failure surfaces via LightGBMUtils.validate as a generic Exception, not ConnectException, so the two are complementary rather than overlapping.

Update: barrier round completion fix

Review of this branch found a hang introduced by the earlier round-completion change, now fixed in this PR.

roundComplete required reportedTaskCount == numTasks. numTasks is the requested task count, not the number of tasks a stage actually runs: determineNumTasks never clamps a user-set numTasks, and barrier-mode prepareDataframe only repartitions downwards. So setNumTasks(16) on an 8-partition DataFrame ran 8 tasks, the equality never held, and the driver waited on Spark's barrier timeout (365 days by default) while every executor blocked on a read with no timeout.

Partition 0 now reports the stage's real task count (BarrierTaskContext.getTaskInfos().length) in the finished marker, and the driver completes the round once that many unique partitions have reported. Completing on the marker alone -- the pre-PR behavior -- has its own race: reports still sitting in the accept backlog get dropped from the topology.

Compatibility: the count is appended to the existing marker, so a sender that omits it still parses and the marker alone completes the round (previous behavior, never a hang).

The wire protocol moved to WorkerMessage.scala; NetworkManager.scala was at 797 of the 800-line scalastyle limit.

Validation: 19 tests across 3 lightgbm suites pass on Java 11; the new A barrier stage smaller than numTasks completes instead of hanging test was confirmed to fail against the unfixed condition.

Copilot AI lite review requested due to automatic review settings August 6, 2026 22:13
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.77778% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.99%. Comparing base (7088a6a) to head (686e86e).

Files with missing lines Patch % Lines
...oft/azure/synapse/ml/lightgbm/NetworkManager.scala 91.72% 12 Missing ⚠️
...apse/ml/lightgbm/NetworkManagerSocketSupport.scala 98.14% 1 Missing ⚠️
...soft/azure/synapse/ml/lightgbm/WorkerMessage.scala 95.45% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2612      +/-   ##
==========================================
- Coverage   86.81%   82.99%   -3.83%     
==========================================
  Files         336      338       +2     
  Lines       18275    18384     +109     
  Branches     1673     1697      +24     
==========================================
- Hits        15866    15258     -608     
- Misses       2409     3126     +717     

☔ 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.

Copilot AI 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.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Improves diagnosability and recovery of distributed LightGBM training when Spark retries tasks after the driver’s topology exchange socket has closed, with special handling to enable barrier-stage restarts.

Changes:

  • Adds clearer error translation for topology handshake ConnectException so retries don’t mask the original failure.
  • Extends the driver topology server lifecycle for barrier mode by serving multiple topology rounds and tracking Spark stage attempts.
  • Fixes socket/thread lifecycle cleanup, and adds targeted unit + E2E coverage plus documentation guidance.
Show a summary per file
File Description
lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/NetworkManager.scala Adds retry-aware ConnectException messaging, stage-attempt-aware handshake protocol, barrier restart support, and more robust socket lifecycle management.
lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/LightGBMBase.scala Ensures driver network resources are released in a finally block when training fails.
lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/DriverSocketRetrySuite.scala Adds unit tests for refused retries, helper socket cleanup, idempotent close, and early-failure cleanup.
lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/DriverSocketRetryE2ESuite.scala Adds an E2E Spark test that verifies retries surface an actionable message rather than only “Connection refused”.
lightgbm/src/test/scala/com/microsoft/azure/synapse/ml/lightgbm/split1/BarrierNetworkRecoverySuite.scala Adds tests validating barrier stage restart gets a fresh topology and stragglers are excluded.
docs/Explore Algorithms/LightGBM/Overview.md Documents why non-barrier retries can’t rejoin the network, and how to diagnose “Connection refused”.

Review details

  • Files reviewed: 6/6 changed files
  • Comments generated: 3
  • Review effort level: Lite

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

Rana Singh (ranadeepsingh) pushed a commit to ranadeepsingh/SynapseML that referenced this pull request Aug 7, 2026
## Summary
Complete LightGBM barrier rounds only after the current attempt has both its finished marker and every unique partition report, replace duplicate reports safely, preserve the public TaskMessageInfo shape, and close all accepted and retained sockets on every shutdown path.

## Prompting Intent
Rebase PR microsoft#2612 onto current master, resolve every active review comment, and prepare it for merge without weakening recovery semantics or adding overhead to the training data path. Keep the network protocol backward compatible, make retry behavior deterministic, and cover ordering, duplicate, cleanup, and compatibility edge cases.

## Linked Sources
- Pull request: microsoft#2612
- Barrier completion review: microsoft#2612 (comment)
- Socket cleanup review: microsoft#2612 (comment)
- Cleanup logging review: microsoft#2612 (comment)

## Rationale
A partition-keyed connection map prevents retried reports from inflating the task count and lets the newest socket replace stale topology data. A private wire message carries the stage attempt while retaining the established five-field public case class and extractor. Tracking the just-accepted socket closes the accept-to-registration race, and unconditional idempotent cleanup avoids leaks even when the server socket was already closed. The driver still performs only constant-time map updates per report and builds topology strings once per round.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 81d39bfc-927c-418a-90a8-e0f2cd8fc128
@ranadeepsingh
Rana Singh (ranadeepsingh) force-pushed the fix/lightgbm-driver-socket-retry branch from 64a0b66 to 94b4bb1 Compare August 7, 2026 09:11
Copilot AI review requested due to automatic review settings August 7, 2026 09:11
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI 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 details

Suppressed comments (1)

lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/NetworkManager.scala:517

  • handleNextWorkerConnection() assumes reader.readLine() always returns a non-null message. If a worker connects and closes without sending (or the connection is otherwise truncated), messageStr will be null and parseWorkerProtocolMessage will throw a NullPointerException, failing the driver network thread with a non-actionable error. Handle null explicitly by closing the socket and treating the connection as a non-report.
        val reader = new BufferedReader(new InputStreamReader(socket.getInputStream))
        val messageStr = reader.readLine()
        log.info(s"received worker message string: $messageStr")
        processWorkerConnection(socket, NetworkManager.parseWorkerProtocolMessage(messageStr))
      } catch {
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Rana Singh (ranadeepsingh) pushed a commit to ranadeepsingh/SynapseML that referenced this pull request Aug 7, 2026
Complete LightGBM barrier rounds only after the current attempt has both its finished marker and every unique partition report, replace duplicate reports safely, preserve the public TaskMessageInfo shape, and close all accepted and retained sockets on every shutdown path.

Rebase PR microsoft#2612 onto current master, resolve every active review comment, and prepare it for merge without weakening recovery semantics or adding overhead to the training data path. Keep the network protocol backward compatible, make retry behavior deterministic, and cover ordering, duplicate, cleanup, and compatibility edge cases.

- Pull request: microsoft#2612
- Barrier completion review: microsoft#2612 (comment)
- Socket cleanup review: microsoft#2612 (comment)
- Cleanup logging review: microsoft#2612 (comment)

A partition-keyed connection map prevents retried reports from inflating the task count and lets the newest socket replace stale topology data. A private wire message carries the stage attempt while retaining the established five-field public case class and extractor. Tracking the just-accepted socket closes the accept-to-registration race, and unconditional idempotent cleanup avoids leaks even when the server socket was already closed. The driver still performs only constant-time map updates per report and builds topology strings once per round.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 81d39bfc-927c-418a-90a8-e0f2cd8fc128
Rana Singh (ranadeepsingh) pushed a commit to ranadeepsingh/SynapseML that referenced this pull request Aug 7, 2026
## Summary
Remove the duplicate NetworkManager companion-object declaration introduced while integrating the merged port-reservation implementation with the retry-topology redesign.

## Prompting Intent
Rebase PR microsoft#2612 onto current master after PR microsoft#2595 merged, preserve both networking behaviors, and leave the branch compiling and ready for renewed validation.

## Linked Sources
- Merged port-reservation fix: microsoft#2595
- Rebased retry-topology PR: microsoft#2612

## Rationale
Keep the merged socket-reservation helpers and the retry-round state machine in one companion object. A second declaration is invalid Scala and was purely a mechanical conflict-resolution artifact; removing it is the smallest behavior-neutral correction.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 81d39bfc-927c-418a-90a8-e0f2cd8fc128
Copilot AI review requested due to automatic review settings August 7, 2026 22:57
@ranadeepsingh
Rana Singh (ranadeepsingh) force-pushed the fix/lightgbm-driver-socket-retry branch from 94b4bb1 to db87e6c Compare August 7, 2026 22:57
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI 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 details

Suppressed comments (1)

lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/NetworkManager.scala:762

  • handleNextWorkerConnection assumes the worker will always send a line of text. If the accepted socket is closed before a message is written (e.g., shutdown racing with read, or a peer that connects and closes), reader.readLine() returns null and parseWorkerProtocolMessage will throw a NullPointerException. This can make shutdown/recovery paths fail with an unrelated NPE instead of a controlled SocketException (which barrier-mode already treats as expected when shutting down).
        val reader = new BufferedReader(new InputStreamReader(socket.getInputStream))
        val messageStr = reader.readLine()
        log.info(s"received worker message string: $messageStr")
        processWorkerConnection(socket, NetworkManager.parseWorkerProtocolMessage(messageStr))
      } catch {
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Rana Singh (ranadeepsingh) pushed a commit to ranadeepsingh/SynapseML that referenced this pull request Aug 7, 2026
## Summary
Move port reservation and cleanup implementations into a package-private support object while retaining the existing NetworkManager entry points. This keeps the integrated networking implementation below the repository's 800-line source-file limit without changing behavior.

## Prompting Intent
Rebase PR microsoft#2612 after microsoft#2595 merged, preserve both LightGBM networking fixes, resolve resulting CI failures, and keep the implementation lean, reviewable, and fully validated.

## Linked Sources
- Rebased retry-topology PR: microsoft#2612
- Merged port-reservation PR: microsoft#2595
- Failed style check: https://github.com/microsoft/SynapseML/actions/runs/31225571403/job/93019061030

## Rationale
The two fixes are behaviorally complementary but together made NetworkManager.scala 909 lines. Extracting cohesive socket lifecycle mechanics avoids a scalastyle suppression or behavior reduction, preserves package-level test seams through thin delegates, and leaves NetworkManager.scala below 800 lines. Combined style checks and all 31 networking tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 81d39bfc-927c-418a-90a8-e0f2cd8fc128
Copilot AI review requested due to automatic review settings August 7, 2026 23:10
@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI 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 details

  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

SynapseML CI and others added 9 commits August 11, 2026 10:28
… the network

Distributed LightGBM training exchanges its network topology with the driver
exactly once per training round, then closes the driver server socket. Any Spark
task that is retried after that point reconnects to an endpoint that is no longer
listening, so every retry fails with a bare "java.net.ConnectException: Connection
refused". Because Spark only reports the most recent attempt, the failure that
actually caused the retry is never surfaced, which makes these incidents
undiagnosable from the job output alone.

Changes:

- NetworkManager.getGlobalNetworkInfo now translates a terminal ConnectException
  into an actionable error. On a retry it states that the topology exchange is
  already closed, that partial task retry cannot rejoin the LightGBM network, and
  that the real cause is in the logs of the first failed attempt of that
  partition. On the first attempt it points at driver reachability instead.

- The driver network thread closes its sockets in a finally block, so the server
  socket is no longer leaked when the topology round throws or times out.

- closeConnections is now idempotent, synchronized, closes the load-only helper
  sockets that were previously leaked on every fit, and closes each socket
  independently so one failure cannot strand the rest.

- LightGBMBase.executeTraining closes the network manager in a finally block.
  Previously a failure in the partition tasks left the network thread blocked in
  accept() holding the driver port for up to the configured timeout.

Tests:

- DriverSocketRetrySuite covers the socket lifecycle directly: the refused retry,
  helper socket release, idempotent close, and release when a round never
  completes.
- DriverSocketRetryE2ESuite reproduces the failure through a real Spark job by
  failing the first attempt of a task after the topology handshake, and asserts
  the reported error explains the cascade instead of just "Connection refused".

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1272df23-c4f3-4048-8789-0320ea286628
A LightGBM network is negotiated once and then fixed, so an individual Spark task
retry can never rejoin it. The only recovery that can work is a full stage restart,
which is exactly what barrier execution mode provides. The driver, however, served
a single topology round and then stopped listening, so a restarted barrier stage
found a closed port and died with "connection refused". Barrier mode therefore
could not actually recover from anything.

The driver now keeps serving topology rounds in barrier mode until training says it
is done, so each stage attempt negotiates its own network.

Mixing attempts would be worse than failing, because a topology containing a dead
host:port makes every surviving task hang trying to connect to it. Task messages
now carry the Spark stage attempt number so the driver can tell attempts apart:

- A message from a newer attempt discards everything collected for the previous one.
- A straggler from a superseded attempt is rejected and its socket closed.
- The finished marker used by barrier mode carries the attempt number as well, so a
  late marker cannot end a round belonging to a newer attempt.

Parsing accepts messages both with and without the attempt number.

Supporting changes:

- The non-barrier round now counts the tasks it has recorded rather than the
  connections it has seen, so discarded connections cannot end a round early. This
  also lets a non-barrier stage restart re-report into a round that is still open.
- The topology strings became defs, since they differ per round.
- Round state is reset between rounds, releasing that round's sockets.
- The barrier round tolerates an accept timeout instead of failing, because training
  can easily outlast the socket timeout while the driver sits idle.
- The finished marker's short-lived connection is now closed instead of leaked.

Tests: BarrierNetworkRecoverySuite covers a restarted stage getting a clean
topology, a superseded straggler being kept out of it, and a normal round being
unaffected. Verified against the existing barrier-mode training suite.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1272df23-c4f3-4048-8789-0320ea286628
Complete LightGBM barrier rounds only after the current attempt has both its finished marker and every unique partition report, replace duplicate reports safely, preserve the public TaskMessageInfo shape, and close all accepted and retained sockets on every shutdown path.

Rebase PR microsoft#2612 onto current master, resolve every active review comment, and prepare it for merge without weakening recovery semantics or adding overhead to the training data path. Keep the network protocol backward compatible, make retry behavior deterministic, and cover ordering, duplicate, cleanup, and compatibility edge cases.

- Pull request: microsoft#2612
- Barrier completion review: microsoft#2612 (comment)
- Socket cleanup review: microsoft#2612 (comment)
- Cleanup logging review: microsoft#2612 (comment)

A partition-keyed connection map prevents retried reports from inflating the task count and lets the newest socket replace stale topology data. A private wire message carries the stage attempt while retaining the established five-field public case class and extractor. Tracking the just-accepted socket closes the accept-to-registration race, and unconditional idempotent cleanup avoids leaks even when the server socket was already closed. The driver still performs only constant-time map updates per report and builds topology strings once per round.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 81d39bfc-927c-418a-90a8-e0f2cd8fc128
## Summary
Remove the duplicate NetworkManager companion-object declaration introduced while integrating the merged port-reservation implementation with the retry-topology redesign.

## Prompting Intent
Rebase PR microsoft#2612 onto current master after PR microsoft#2595 merged, preserve both networking behaviors, and leave the branch compiling and ready for renewed validation.

## Linked Sources
- Merged port-reservation fix: microsoft#2595
- Rebased retry-topology PR: microsoft#2612

## Rationale
Keep the merged socket-reservation helpers and the retry-round state machine in one companion object. A second declaration is invalid Scala and was purely a mechanical conflict-resolution artifact; removing it is the smallest behavior-neutral correction.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 81d39bfc-927c-418a-90a8-e0f2cd8fc128
## Summary
Move port reservation and cleanup implementations into a package-private support object while retaining the existing NetworkManager entry points. This keeps the integrated networking implementation below the repository's 800-line source-file limit without changing behavior.

## Prompting Intent
Rebase PR microsoft#2612 after microsoft#2595 merged, preserve both LightGBM networking fixes, resolve resulting CI failures, and keep the implementation lean, reviewable, and fully validated.

## Linked Sources
- Rebased retry-topology PR: microsoft#2612
- Merged port-reservation PR: microsoft#2595
- Failed style check: https://github.com/microsoft/SynapseML/actions/runs/31225571403/job/93019061030

## Rationale
The two fixes are behaviorally complementary but together made NetworkManager.scala 909 lines. Extracting cohesive socket lifecycle mechanics avoids a scalastyle suppression or behavior reduction, preserves package-level test seams through thin delegates, and leaves NetworkManager.scala below 800 lines. Combined style checks and all 31 networking tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 81d39bfc-927c-418a-90a8-e0f2cd8fc128
Address two issues raised in review of the driver socket retry work:

- parseWorkerProtocolMessage dereferenced the string returned by
  BufferedReader.readLine(), which is null when a worker connects and then
  closes or crashes before sending its status line. That produced a
  NullPointerException which masked the real socket failure, defeating the
  purpose of this change. It now throws a targeted IOException; the caller
  already closes the accepted socket and rethrows.

- The network communication thread ran on a single-thread executor that was
  never shut down, leaking a non-daemon thread per NetworkManager instance on
  long-lived drivers that run repeated fits. The executor is now held and shut
  down in the existing finally block. shutdown() is an orderly shutdown, so
  calling it from a task running on that executor is safe.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… test

The regression test intentionally passes null to parseWorkerMessage to model
BufferedReader.readLine() returning null at EOF. Use the repo's standard
//scalastyle:ignore null suppression, as other suites do.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Non-barrier mode retries a task fine if it fails before it joins the
LightGBM network; barrier mode is only required for failures after the
topology exchange. Say that instead of claiming barrier mode is the only
mode that can recover from any task failure.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…count

The barrier round completion check required reportedTaskCount == numTasks.
numTasks is the user-requested task count, not the number of tasks the stage
actually runs: determineNumTasks returns a user-set numTasks unclamped, and
barrier-mode prepareDataframe only repartitions downwards. Calling
setNumTasks(16) on an 8-partition DataFrame therefore ran 8 tasks, the count
never matched, and the driver waited until Spark's barrier timeout (365 days
by default) while every executor blocked on a read with no timeout.

Have partition 0 include the real barrier task count from
BarrierTaskContext.getTaskInfos() in the finished marker, and complete the
round once that many unique partitions have reported. The sender is the only
participant that knows how many tasks the stage actually ran. Completing on
the marker alone would reintroduce the race where reports still queued in the
accept backlog get dropped from the topology.

The extra field is appended to the existing marker, so a sender that omits it
still parses; in that case the marker alone completes the round, matching the
previous behavior rather than hanging.

Move the wire protocol into WorkerMessage.scala. NetworkManager was at 797 of
the 800-line scalastyle limit, and parsing/formatting is a self-contained
concern.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 11, 2026 17:28
@ranadeepsingh
Rana Singh (ranadeepsingh) force-pushed the fix/lightgbm-driver-socket-retry branch from 5ab1ac2 to 686e86e Compare August 11, 2026 17:28

Copilot AI 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 details

Suppressed comments (1)

lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/WorkerMessage.scala:48

  • WorkerMessage.parse throws a generic Exception for unexpected task-message field counts. This makes it harder to distinguish parse/protocol errors from other failures higher up the stack (and loses the expected-vs-actual context). Using an IOException (or IllegalArgumentException) with a more actionable message would improve diagnosability without changing control flow.
      if (components.length != TaskMessageFieldCount && components.length != TaskMessageFieldCountWithStageAttempt) {
        throw new Exception(s"Unexpected message: $message")
      }
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@ranadeepsingh

Copy link
Copy Markdown
Collaborator Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

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.

4 participants