fix(runtime-broker): keep the HTTP exchange cancel off the deadline completion path - #13562

✅ Qwen Triage finished — CI landed green on 829c0d2 and the deferred approval was posted. finalize run

✅ Qwen Triage 已完成 —— 829c0d2 的 CI 全绿，延迟审批已提交。查看 finalize 运行

Thanks for the PR!

Template looks good ✓ — every required heading is present, the Tested-on table is filled in honestly (Windows marked ⚠️ rather than claimed), and the Chinese section mirrors the English one paragraph for paragraph.

Problem: observed, not theoretical. Three real windows-latest / Java 21 failures in one day on unrelated PRs (runs 37543910341, 37542230965, 37558689200), never on Linux or macOS, plus a minimal repro — a 1 s sleep inside the cancel callback pushes the caller's join() from 0.5 s to 1.5 s. That is a proper before/after, and the ordering claim survives checking against the code rather than just against the description: acknowledgeCsi returns post(...).thenApply(...), so the caller's stage is a dependent of returned registered after the cleanup whenComplete inside post(). On the exceptional path CompletableFuture drains the source's remaining dependents before the caller's own stage is completed, so the cancel genuinely sits inside the caller's measured latency. The 500 ms deadline was never the whole story.

Direction: aligned, and it earns its place beyond the flake. orTimeout runs its timeout task on a single JVM-wide delay thread, and this module leans on that thread hard — RuntimeBrokerService alone has seven more orTimeout deadlines sharing it. A ~3 s socket close parked there postpones every one of them, so this is a latency-isolation fix with a flaky test as the visible symptom. Nothing here touches a public contract, auth, sandbox, model selection, telemetry or release surface; deadline semantics, the error code and the connection close are all unchanged. CHANGELOG: no direct reference, and none expected — transport-internal threading.

Size: not applicable. packages/sdk-java/** is not a core path and this is a single package. 10 production lines (6 of them comments) + 115 test lines.

Approach: the scope feels right, and it matches what I'd have written independently — two one-token changes plus a comment at each site, no new field, no executor lifecycle to own, and no new test dependency. On that last point: the module is JUnit-only (no Mockito in runtime-broker/pom.xml), so the ~60-line HttpClient double is the honest option rather than a reuse miss — HttpClient is an abstract class with twelve abstract methods and there is nothing lighter already installed. Both sites in HttpRuntimeTransport are covered, which is the pair the flaky test actually drives.

Two things worth thinking about before merge. Neither blocks:

• whenCompleteAsync with no executor lands on ForkJoinPool.commonPool(), whose parallelism is availableProcessors - 1 — one thread on a 2-core runner. The PR's own argument is "don't let a slow cancel hog a shared thread", and a 3 s Windows cancel would hog the entire common pool for those 3 s, serialising concurrent deadline cancels and anything else the process runs there. The HttpClient already owns an executor reachable through client.executor(), so the cancel could run on the same pool that drove the exchange — per-client blast radius instead of JVM-wide. Worth noting the new stub returns Optional.empty() there, so that variant needs an orElse fallback to keep the test meaningful. This is strictly better than today either way; the delay thread is one thread for the whole JVM.
• KubernetesHttpRuntimeClient.exchangeBytes (around lines 165–170) carries the identical shape: .orTimeout(...) followed by a synchronous result.whenComplete(... exchange.cancel(true)), with the caller receiving result.exceptionallyCompose(...) — again a dependent registered after the cleanup. It isn't exercised by the flaky test, so leaving it out keeps this PR focused, but it is the same latent instance and a reasonable follow-up.

Risk: no elevated risk signals — neither changed file matches the revert-correlated path list. The honest caveat is evidential rather than structural: the job that matters most here (windows-latest / Java 21) is still queued, so the flake itself is not yet confirmed fixed by CI on this commit.

Moving on to code review. 🔍

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at 829c0d29e4bc822225405411027d9aa2b0033cf0 · re-run with @qwen-code /triage}

Code review

Independent proposal first (written from the title and "Why it's needed", before reading the diff): the cancel is running on the thread that completes the deadline, so it has to be handed to some other thread — and the cheapest correct way to say that in CompletableFuture is the *Async variant of the same stage. I'd have changed whenComplete to whenCompleteAsync at the deadline-cleanup site, kept the guard condition untouched, and added a test that makes the cancel deliberately slow so the ordering is observable rather than inferred. The diff does exactly that, at both sites in HttpRuntimeTransport rather than only the one the flaky test reaches. It matches my baseline; I did not find a simpler path it missed.

No critical blockers. I checked the things that could actually break here rather than the things that look unusual:

• The connection still gets closed. whenCompleteAsync fires on cancel exactly as whenComplete did — cancelling returned completes it, which drives the dependent — and the error != null || returned.isCancelled() guard is unchanged, so both the deadline path and the caller-cancel path still reach exchange.cancel(true). The existing HttpRuntimeTransportTest.closesTheConnectionOnTheDeadlineAndOnCallerCancel is the guard for this, and it is the right one: it loops over {deadline, caller cancel} and asserts the server observes the close within 2 s. Two seconds is far more slack than the few-microsecond deferral this introduces, so it should not become flaky in the other direction.
• The new test is load-bearing, not decorative. With the fix reverted, the injected cancel blocks the delay thread before the caller's stage completes, so join() stalls until the 5 s guard expires and cancelSawCallerReturn is false — the reported "Before" (5.675 s, expected: <true> but was: <false>) is what that path produces. With the fix, the cancel waits on a different thread and sees the latch already counted down. The assertion can only pass for the right reason. It also correctly returns Redirect.NEVER from the stub, which matters because acknowledgeCsi throws IllegalArgumentException on any other redirect policy before it ever reaches post().
• No dead stub surface. All twelve abstract HttpClient methods are implemented, every one of the ten new imports is used (UnusedImports is one of only two import checks this module's checkstyle enables — ImportOrder is commented out), and exchange in post() is the very object sendAsync returned, so the overridden cancel() is the one the production path calls.
• No documentation drift. Nothing in docs/ describes the deadline-cancel threading, and the BoundedBodySubscriber javadoc still reads correctly — it documents that a stalled body is bounded by the stage deadline, which this change does not alter.

Non-blocking observations, both already raised in the gate comment and repeated here only so they stay visible next to the code: the cancel now runs on ForkJoinPool.commonPool() (parallelism availableProcessors - 1, so one thread on a 2-core runner) while client.executor() would scope it to the client that owns the exchange; and KubernetesHttpRuntimeClient.exchangeBytes keeps the identical synchronous shape. Neither is a regression against today — a single JVM-wide delay thread is the worse of the two — and the duplicated three-line comment at both sites is warranted here, since the why is a JDK completion-ordering subtlety that reads like a no-op change otherwise.

Test evidence

This is an unattended CI run, so nothing here was built or executed by the review — PR-derived code is never run. The evidence below is this PR's own CI, read through the API for the reviewed commit. No check on 829c0d29e4bc822225405411027d9aa2b0033cf0 has a failure conclusion, so there is no failing-job log to quote.

The important caveat: the fix is not yet confirmed where it matters. windows-latest / Java 21 — the only job on which this test has ever failed, and the only platform that can demonstrate the flake is gone — is still queued. macos-latest / Java 21 is queued too. The Linux Java jobs are in_progress. Everything completed so far is orchestration (authorize, assign, label, Classify PR, Flyway migration version uniqueness); the Test (macos-latest/windows-latest, Node 22.x) and integration entries are skipped, which is expected for a Java-only diff. So the current green is real but says nothing about the change. The finalize job rewrites the region below in place once CI settles.

Final CI results for 829c0d2 (auto-updated by the triage finalize job after CI completed):

_{One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。}

Sandboxed verification would settle what CI cannot yet: @qwen-code /verify — that the new deadlineFailureDoesNotWaitForTheExchangeCancel is genuinely load-bearing, i.e. that it fails when the two whenCompleteAsync calls are reverted to whenComplete and passes with them in place. That A/B is the whole claim of this PR, and a green suite on the fixed tree alone does not distinguish it from a test that would pass either way. The author states the reverted run fails at 5.675 s with expected: <true> but was: <false>; that is the author's claim, not independently re-run here. Separately, the flake itself is only observable on windows-latest / Java 21, which is still queued — until it lands green on this commit, "the Windows flake is fixed" rests on the Linux and macOS timings the author reported plus the mechanism, not on a Windows run of this code.

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at 829c0d29e4bc822225405411027d9aa2b0033cf0 · re-run with @qwen-code /triage}

Confidence: 4/5 — the mechanism is verified against the code, the regression test can only pass for the right reason, and the change is as small as this fix can be; the two reservations are the common-pool destination and the fact that Windows CI hasn't spoken yet.

Stepping back: this is a good PR and it is the kind I'd want to maintain. The diagnosis is not hand-waved — it names a specific CompletableFuture propagation behaviour, and that behaviour checks out against the actual call shape (post(...).thenApply(...), cleanup registered before the caller's dependent). The fix is two tokens plus a comment at each site. The test injects a slow cancel and asserts an ordering, which is the only way to pin something like this; a test that merely re-ran the flaky one and hoped would have been worthless. And the author resisted the tempting over-fix: no executor abstraction, no retry policy, no drive-by refactor of the neighbouring KubernetesHttpRuntimeClient site that has the same shape — that stayed a noted follow-up instead of widening the diff.

My independent proposal and the diff converged, so there is no simpler path I'm withholding. If I had to maintain this in six months I'd thank the author, with one mild curse: whenCompleteAsync reads as "off the caller's path", but where it lands is an implementation default, and the common pool is JVM-wide with parallelism availableProcessors - 1. The comment above each site explains why the cancel moved off the delay thread; it doesn't say which thread it moved to, or that this is a deliberate trade rather than an oversight. One extra clause naming the pool would make the choice durable. Non-blocking — the trade is defensible and strictly better than the single delay thread it replaces.

On the evidence question I care most about: I verified the problem exists (three real failing runs, Windows-only, plus a minimal repro), and I verified the test is load-bearing by reasoning through both the reverted and fixed paths rather than trusting the reported numbers. What I could not verify is the thing the PR is ultimately for — that windows-latest / Java 21 now passes well under its 3 s bound — because that job is still queued on this commit. That is a CI-timing gap, not a doubt about the change, and it is exactly what deferring the approval closes: the mechanism is platform-independent (the cancel leaves the caller's path regardless of how long it takes), so Linux going green plus the ordering test is strong support, and the Windows job is confirmation rather than discovery.

Two PR CI workflow runs are still in flight on this commit, so approval is deferred until CI lands green on 829c0d29e4bc822225405411027d9aa2b0033cf0. The finalize job posts the commit-pinned approval once every check completes green, and withholds it if anything lands red or the head moves. Nothing here needs a maintainer decision — the follow-up on KubernetesHttpRuntimeClient.exchangeBytes can be its own PR whenever someone wants it.

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at 829c0d29e4bc822225405411027d9aa2b0033cf0 · re-run with @qwen-code /triage}

🔍 Local Maintainer Verification — PR #13562

Verdict: ✅ merge-ready — 754 assertions green, 0 failures (732 full-suite + 20 targeted-repeat + 1 mutation kill + 1 static gate; tally in assertions.json, verdict in verdict.txt).
Verified head: 829c0d29e4bc822225405411027d9aa2b0033cf0 · Base (merge-base with main): 4970bfa172077147adf7abc226e3d0462b179ac5

Environment

macOS (darwin, Apple Silicon) · OpenJDK 21.0.12 (Homebrew) · Maven 3.9.16 · isolated worktree at the PR head, differing from merge-base only in this PR's two files (verified: git diff <base> HEAD --stat = 2 files, +123/−2).

What was verified

A/B red–green proof

The PR touches exactly one production file, so the mutation matrix is that one file: reverted in place to the merge-base version (whenCompleteAsync count 2 → 0), reran the regression class, then restored (git status verified clean afterwards).

• Base (mutant) — red, with the bug's own signature: Tests run: 4, Failures: 1 — deadlineFailureDoesNotWaitForTheExchangeCancel fails after 5.839s: org.opentest4j.AssertionFailedError: the deadline failure waited for the exchange cancel ==> expected: <true> but was: <false>. This reproduces the PR's stated before-behavior (5.675s) almost exactly.
• Head — green: same class 4/4, new test ~0.7s. The mutant is killed only by the new regression test (the other three stay green on base), so the test provably pins the fixed behavior and nothing else.

Code review notes

• Scope: minimal and surgical — two whenComplete → whenCompleteAsync changes (attest ~L219, post ~L1109) plus an explanatory comment at each site; deadline semantics, error code (managed_runtime_unavailable), and the connection close itself are untouched, consistent with the PR's claim.
• Why the fix is correct: on a deadline, the stage completes on the JVM-wide CompletableFuture delay thread; the inline cancel previously ran there and ahead of the caller's continuation, so a slow JDK HttpClient cancel (observed ~3s on Windows) inflated the caller's wall-clock and stalled every other orTimeout in the process. The async continuation moves the socket close off both the caller's completion path and the shared timer thread.
• The new test is deterministic, not timing-based: the injected StalledExchangeClient never answers and blocks inside cancel() until the caller has returned (5s guard); it asserts ordering (caller released before cancel finishes), so it cannot flake the way the wall-clock assertion did.
• Downstream impact: callers of attest/post only observe the returned stage; nothing depends on the cancel running inline. The cancel still runs on every error/cancel path — just a few µs later, on the common pool.

CI cross-check

windows-latest / Java 21 (the formerly flaky job) ✅ 4m33s on this PR; macos-latest, ubuntu-latest (Java 11/17/21) all ✅. Hosted process fault gates / MySQL 8.4 / Java 21 ✅ 33m — per-step conclusions verified: step 12 Run Runtime Broker fault gates and all owner-failover E2Es are success, not skipped.

Not covered

• Windows local run (no Windows box here) — the platform where the flake appeared; covered by this PR's green windows-latest / Java 21 CI lane.
• Root cause of the ~3s JDK cancel on Windows — explicitly out of scope per the PR; the fix removes it from the caller's path regardless of cause.
• Non-runtime-broker modules and the TS CLI are untouched by this diff, so their gates were not run locally (CI's JS lanes are green/skipped per path filtering).

Methodology

1. Detached worktree at the PR head (829c0d29); merge-base with main (4970bfa1) confirmed as the base; the touched production file is identical between merge-base and current main.
2. mvn test full module suite + 5× targeted class runs on head; per-test timings read from surefire XML, not console estimates.
3. Mutation = in-place revert of the single production file to base, rerun, restore, verify clean.
4. checkstyle:check as the static gate.
5. Live terminal captures (node-pty → xterm.js) of the head run, the mutant run, and the recorded full-suite summary; artifacts under tmp/pr13562-verify-20261007/.

Evidence captures

Head — regression class green (live run):

Mutant (base transport) — red with the predicted signature (live run):

Full suite on head — 735/0/0/3 + key per-test timings:

Local verification performed by @wenshao's maintainer harness; artifacts (head-full-suite.log, mutant-run.log, head-targeted-run{1..5}.log, checkstyle.log, assertions.json, verdict.txt, PNGs) retained under tmp/pr13562-verify-20261007/.