fix: prevent sender control future race - #357

Pull request overview

This PR addresses a control-message future race in the computer-core message-sending pipeline (specifically QueuedMessageSender) that could surface as The origin future must be null, and also tightens computer-test integration behavior to fail faster instead of stalling for long BSP timeouts.

Changes:

• Refactors control-message handling in QueuedMessageSender so each START/FINISH message carries its own CompletableFuture, and the in-flight control future is cleared before completion.
• Adds unit regressions for consecutive control messages and for transport failures completing the control future exceptionally.
• Updates sender integration tests to apply shorter BSP wait timeouts and to use a bounded waitForServices() helper.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

        public void sendFinishMessage(CompletableFuture<Void> future)
                                      throws TransportException {
            this.setControlFuture(future);
            try {
                this.client.finishSessionAsync().whenComplete((r, e) -> {

                                          .withRpcServerHost("127.0.0.1")
                                          .withRpcServerPort(8611)
                                          .withRpcServerPort(0)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Blocking: yes. Summary: An existing review thread covers the sender-executor termination risk, and the new fail-fast service wait can race past lifecycle cleanup. Evidence: exact-head static inspection by six independent lanes; git diff --check passed; visible exact-head checks are green.

Blocking: yes. Summary: Synchronous runtime failures can still strand an in-flight control future, and the fail-fast cleanup/tests leave concurrency gaps. Evidence: six independent exact-head review lanes; targeted QueuedMessageSenderTest passed; git diff --check passed; visible exact-head checks are green.

Blocking: yes. Summary: Two control-message failure windows can still strand work or hide a connection failure, and initialization failures can bypass the new cleanup path; latest-head Computer CI is also failing. Evidence: exact-head static interleaving analysis; local Maven validation stopped before the target test because the available JDK cannot compile computer-k8s; GitHub Actions run 30206151982 reports three unit-test failures.

Blocking: yes. Summary: Two fail-fast cleanup paths can still extend CI stalls and hide the original service failure. Evidence: six independent exact-head review lanes; QueuedMessageSenderTest passed 7/7 on JDK 11; git diff --check passed; visible exact-head checks were green.

Blocking: yes. Summary: A synchronous data-path failure can still terminate the sole sender and strand queued control futures, while two new cleanup tests can leak non-daemon threads on an early assertion failure. Evidence: six independent exact-head review lanes; QueuedMessageSenderTest passed 7/7 on JDK 11; git diff --check passed; visible exact-head checks are green.

Blocking: yes. Summary: An uncaught master Error can leave the service future pending until the five-minute timeout; two previously reported sender and cleanup risks also remain covered by existing threads. Evidence: six independent exact-head review lanes, static control-flow and interleaving analysis, QueuedMessageSenderTest passed 9/9, and visible exact-head checks are green.

Blocking: yes. Summary: A data-send failure can still be forgotten before FINISH is registered, allowing the step to succeed after silently dropping a message; the new timeout regression also has a minor timing-flake window. Evidence: six independent exact-head review lanes; static MessageSendManager/QueuedMessageSender interleaving analysis; targeted QueuedMessageSenderTest passed 9/9; git diff --check passed; visible exact-head checks are green.

Blocking: yes. Summary: A control-completion race can silently discard transport failures, and service cleanup errors can still be hidden after successful execution. Evidence: six independent exact-head review lanes; targeted QueuedMessageSenderTest passed 12/12 on JDK 11; git diff --check passed; visible exact-head checks are green.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

                                          .withRpcServerHost("127.0.0.1")
                                          .withRpcServerPort(8611)
                                        .withRpcServerPort(0)

Blocking: yes. Summary: Partial worker initialization can now omit the BSP close signal, and the integration tests have reverted to unbounded 24-hour BSP waits on failure. Evidence: six independent exact-head review lanes; QueuedMessageSenderTest passed 15/15 on JDK 11; git diff --check passed; visible exact-head checks are green.

Blocking: yes. Summary: Two test changes weaken coverage of sender backpressure and executor shutdown behavior. Evidence: six independent exact-head review lanes; targeted QueuedMessageSenderTest passed 15/15 on JDK 11; git diff --check passed; visible exact-head checks are green.

Blocking: yes. Summary: The partial-initialization regression test bypasses the production registration transition, so it cannot catch the lifecycle bug returning. Evidence: five completed independent exact-head review lanes; one additional lane ended without a result; QueuedMessageSenderTest passed 15/15 on JDK 11; git diff --check passed; visible exact-head checks are green.

Blocking: no. Summary: The sender control-future and partial worker-initialization paths are consistent at this head, with no actionable findings after independent review. Evidence: six exact-head review lanes; QueuedMessageSenderTest 15/15 and WorkerServiceTest#testInitFailsAfterRegistration 1/1 passed on JDK 11; git diff --check passed; all visible exact-head checks are green. Score: 8.6/10.

Blocking: no. Summary: The control-future rework is sound and its 15 unit regressions pass locally at this head, but two gaps remain on the failure paths the PR targets: partially initialized worker managers are still only half closed because Managers.closeAll() aborts at the first failing manager, and a fatal send-executor error leaves already-queued control futures unresolved so callers still wait the full 10s/80s transport timeout and get a misleading Timeout(...) instead of the recorded cause. Evidence: mvn install -DskipTests then java -cp <computer-test classpath> org.junit.runner.JUnitCore org.apache.hugegraph.computer.core.sender.QueuedMessageSenderTest -> OK (15 tests); WorkerServiceTest#testInitFailsAfterRegistration -> RESULT run=1 failures=0; two standalone probes compiled against 8492fd4 reproduce the two gaps (outputs quoted inline).

Blocking: no. Summary: The latest commit closes the queued-control-future and partial-cleanup gaps from the previous round, but two small failure-path gaps remain: an unexpected interrupt on an idle send-executor still exits without recording a fatal error, and WorkerInputManager still throws an NPE when closed after a partial init. Evidence: JDK 11 test-compile of computer-test at 5189d79; QueuedMessageSenderTest OK (17 tests); DataServerManagerTest#testCloseManagersAfterPartialInitFailure and WorkerServiceTest#testInitFailsAfterRegistration pass; two standalone probes against this head reproduce both gaps (outputs inline); all checks on this head are green.