[Test][Connector-V2] Release PayPalClientTest's arrival latch before the response body write

[DanielLeens]

Purpose of this pull request

PayPalClientTest#closeWakesRetryWait keeps failing on the unit-test (11, windows-latest) / (8, windows-latest) jobs of PRs that do not touch the PayPal connector, most recently on #11814 (2026-09-22) and #11215 (2026-09-18), always at the same assertion:

PayPalClientTest.closeWakesRetryWait:416 expected: <true> but was: <false>

That line is the first arrived.await(NETWORK_WAIT_SECONDS, ...), i.e. the test gives up waiting for the request to reach the embedded server. #12370 widened that wait from 3 s to 15 s and the failure came back at the same line, and the open #12381 proposes widening it again to 60 s. The wait budget is not the problem: the latch is never counted down.

Root cause

serve() in the test counts arrived down only after it has written the reply body:

exchange.getResponseBody().write(reply.body);
arrived.countDown();

closeWakesRetryWait replies 503 {}. PayPalClient#execute does not read the body of a transient status: it returns as soon as it has the status line, and its finally block calls request.abort(), which closes the socket. On Linux the server's 2-byte write still lands in the send buffer, so the countDown runs. On Windows a write into a socket the peer has already closed fails with an IOException; serve() swallows it in catch (IOException ignored) and never reaches the countDown, so the test waits out the whole budget for a request that arrived long ago. Whether the client's abort or the server's write wins is scheduling-dependent, which is why the failure is intermittent and only seen on the Windows legs.

Fix (test-only)

• Count arrived down before the body write for plain replies, so the client aborting the connection cannot skip it.
• Also count it down in the finally block, so no response-side failure can leave a test waiting on the latch. The latch is a CountDownLatch(1), so the second call is a no-op on the normal path.
• The bodyless and blocking branches keep their existing order: closeAbortsActiveBody relies on the first body byte having been flushed when the latch is released.

No assertion, expected value or wait budget changes. Compared with #12381 this removes the cause instead of widening the wait, which cannot help when the countDown is skipped.

Files

• seatunnel-connectors-v2/connector-http/connector-http-paypal/src/test/java/org/apache/seatunnel/connectors/seatunnel/paypal/source/PayPalClientTest.java

How was this patch tested?

Test-helper-only change, verified by this PR's GitHub CI (unit-test on all four matrix legs) per this initiative's no-local-build policy. Local checks were limited to ./mvnw spotless:apply -pl seatunnel-connectors-v2/connector-http/connector-http-paypal.

Check list

• No new Jar binary package is added.
• Documentation is not required.
• incompatible-changes.md is not required.

🤖 Generated with Claude Code

[DanielLeens]

Thanks for the contribution! The logic looks solid and the fix is a precise root-cause correction rather than another band-aid. Left the full analysis below, but I don't see any blockers — happy to approve once the unrelated CI noise clears.

What Problem Does This PR Solve?

• User pain point: PayPalClientTest#closeWakesRetryWait has been intermittently failing on windows-latest, timing out on the initial arrived.await(...), even on PRs that never touch the PayPal connector. A previous fix widened the wait budget to 15 seconds, but that turned out to be treating a symptom rather than the cause, and the flake came back.
• Fix approach: the embedded test server was only counting the arrived latch down after writing the response body. PayPalClient never reads the body of a transient (503) response — it returns as soon as it sees the status line, and its finally block then aborts the request, closing the socket. On Windows, writing into a socket the peer has already closed throws an IOException (where Linux would just buffer it), the existing catch (IOException ignored) swallowed that exception, and the countDown() that used to come after the write simply never ran. So the test sat there waiting the full budget for a request that had, in reality, already arrived.
• One-sentence summary: this moves the latch release ahead of the write that can fail, and adds a finally-block safety net, so the test can never again get stuck waiting on a signal that a swallowed exception prevented from firing.

1. Code Change Review

1.1 Core Logic Analysis

Change: one file, PayPalClientTest.java, in the private serve(HttpExchange) helper that backs the embedded test HTTP server (+8/-1 lines).

Before:

} else {
    exchange.getResponseBody().write(reply.body);
    arrived.countDown();
}
...
} finally {
    exchange.close();
}

After:

} else {
    // Release the latch before the body write. ...
    arrived.countDown();
    exchange.getResponseBody().write(reply.body);
}
...
} finally {
    // The request reached the server whichever branch failed above, so a test must
    // never keep waiting on the latch only because the response could not be written.
    arrived.countDown();
    exchange.close();
}

Key findings:

• The normal path does reach this: closeWakesRetryWait is the only test that both hits the else branch (a non-bodyless, non-blocking reply — the 503 case) and calls arrived.await(...).
• I traced every usage of the arrived latch in the file. Only two tests call arrived.await(...): closeAbortsActiveBody (uses the block branch, whose countDown-before-await ordering was already correct and untouched here) and closeWakesRetryWait (uses exactly the branch this PR touches).
• No test depends on the response body actually being written by the time arrived.await() returns — sendResponseHeaders(...) is already sent unconditionally before this if/else, so reordering the body write relative to the countDown has no observable effect on the client in the scenario this fix targets.
• This reads as a genuine root-cause fix rather than a defensive workaround: it removes the actual lost-signal bug instead of further inflating a timeout.
• The extra countDown() added to finally is safe: CountDownLatch.countDown() on an already-zero latch is a documented no-op, so the redundant call in the already-successful paths can't cause any test to observe an unexpected state.

In-depth correctness: the fix takes effect precisely in the else branch (non-bodyless, non-block replies) and is a no-op change for the bodyless/block branches, which already ordered countDown() safely. There's no production-code path involved at all — this is confined to a JUnit test fixture's synchronization between the server thread and the test thread, so there's no lifecycle, checkpoint, or recovery-path interaction to reason about.

1.2 Compatibility Impact

Fully compatible. This is a src/test/java only change. No API, configuration option, default value, protocol, or serialization format is touched, and there's no user-visible behavior change.

1.3 Performance / Side-Effect Analysis

No measurable impact — no new allocations or synchronization primitives, no change to production code. The added countDown() calls are idempotent once the latch reaches zero, so there's no new race or double-release hazard. Socket cleanup ordering (exchange.close() still last in finally) is unchanged.

1.4 Error Handling and Logging

The existing catch (InterruptedException e) and catch (IOException ignored) blocks are unchanged, and the comment on the IOException catch already documents that cancellation intentionally closes the peer socket. This PR doesn't change what's caught — it just makes sure a caught-and-swallowed exception can no longer leave the latch permanently un-decremented. No logging changes needed for a test-fixture fix like this.

No formal issues were found in this PR — No blocking issues found.

2. Code Quality Assessment

2.1 Coding Standards

The new code follows the file's existing style, and both added comments clearly explain the why (Windows peer-close semantics, and the defense-in-depth rationale for the finally-block countDown) rather than restating the code. No new methods or fields are introduced, so there's nothing additional needing Javadoc.

2.2 Test Coverage and Test Stability

This PR is itself a fix to test infrastructure, so no new test case is expected — the existing closeWakesRetryWait test is the regression guard, and it will now pass reliably.

Mandatory stability analysis (Section 5 §10.2):

• Assertion-risk / anti-pattern check: none found. The diff doesn't add any Thread.sleep/fixed-wait pattern, doesn't introduce any new dependency on clock, environment, or execution order, and doesn't add hard-coded ports or hostnames. If anything, it removes an existing source of flakiness rather than adding one.
• Stability rating: Stable. Evidence: PayPalClientTest.java's serve() method (the modified else branch and finally block) — the change only reorders an existing countDown() call ahead of a write documented to intermittently throw on Windows, and adds a provably-safe redundant countDown() in finally. I verified every consumer of the arrived latch in the file and confirmed the only affected test (closeWakesRetryWait) doesn't depend on body content being written before the latch releases.

2.3 Documentation Updates

Not applicable — no user-visible behavior changed, so no docs/en/docs/zh updates are required.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. It targets the actual mechanism of the flake (a lost countdown signal on an IOException path caused by platform-dependent socket-close semantics) rather than being a temporary workaround.

3.2 Maintainability

The comments make the reasoning easy to follow for the next person who touches this test, and the change is small and self-contained.

3.3 Extensibility

The finally-block safety net is a nice general pattern for this kind of test fixture — any future branch added to serve() inherits the guarantee that arrived will always be released, without needing to remember to add the countdown to every new branch.

3.4 Historical-Version Compatibility

Fully compatible with historical versions; no migration or upgrade action needed.

4. Issue Summary

No issues to list — no blocking or non-blocking findings were raised against this diff.

5. Merge Recommendation

Conclusion: Ready to merge

1. Blockers — must be fixed

   • None.

2. Recommended fixes — non-blocking

   • None.

Overall assessment: this is a clean, well-targeted root-cause fix confined to test code, with a clear before/after rationale and no side effects outside the test file it touches.

CI status: the current Build check on this PR is failing, but tracing it into the fork's actual run shows the two failing jobs are unit-test (11, ubuntu-latest) and unit-test (8, ubuntu-latest), both failing inside the unrelated seatunnel-engine-server module (TaskExecutionServiceTest.testStaleTaskDoneCleansOnlyOwnedGenerationResources and EngineStateStoreLogicalMetricExportsTest.collectShouldExportLogicalMetricsForSpecialStateStores, respectively) — neither anywhere near the connector-http-paypal module this PR touches, and both consistent with known cross-PR flakes in that module. This looks like CI noise unrelated to this PR's own diff rather than a regression it introduced. Retrying the two failing unit-test jobs (or rebasing onto the latest dev, which has moved a few commits ahead) should be the next step rather than any change to this PR.

Nice fix — thanks for chasing this all the way down to the actual platform-dependent root cause instead of just widening the timeout again!