DO NOT MERGE: #34154: refactor(health) scope the health-check fan-out so a timeout cancels it

[fabrizzio-dotCMS]

The finding

HealthStateManager.runAllHealthChecksAndWait() fans out 15 health checks and waits for them:

CompletableFuture<Void> allChecks = CompletableFuture.allOf(
    futures.toArray(new CompletableFuture[0]));
allChecks.get(timeoutMs, TimeUnit.MILLISECONDS);

That timeout stops waiting. It does not stop the work. The executor is
Executors.newScheduledThreadPool(HealthCheckConfig.THREAD_POOL_SIZE) — bounded — so a wedged check
keeps one of those threads for as long as it stays wedged, with nothing reporting it, while the
method logs "some health checks did not complete" and carries on.

It is the failure mode of #37038 in a different subsystem: abandoning something that keeps
consuming a shared, bounded resource.

Run on JDK 25.0.2, four wedged tasks, a 300 ms timeout:

allOf().get(timeout)  ->  TimeoutException,  tasks still running: 4
scope closed          ->  TimeoutException,  tasks still running: 0

The change

try (var scope = StructuredTaskScope.open(Joiner.<Void>awaitAll(),
        cfg -> cfg.withTimeout(Duration.ofMillis(timeoutMs)).withName("health-checks"))) {

    allHealthChecks.forEach(check -> scope.fork(() -> { runSingleHealthCheckBlocking(check); return null; }));
    scope.join();
}

The block is the lifetime. On timeout the subtasks are cancelled, and close() does not return
until they are done — no work escapes the method.

Everything else is held constant on purpose: the same timeout arithmetic (longest individual check

• 5s, honouring health.force.refresh.timeout-ms), and the same continue with whatever arrived
  contract. That last point is why the joiner is awaitAll() and not awaitAllSuccessfulOrThrow()
  — one failing check must not cancel its siblings, and a test pins it.

What is deliberately not converted

waitForOngoingRefreshes keeps its CompletableFuture.allOf, with a comment saying why: it waits on
refreshes started elsewhere and held in the ongoingRefreshes map, and a scope can only wait on
subtasks it forked itself. That is not a limitation to work around — it is the property that makes
the lifetime meaningful. Converting it would mean moving where the refreshes start, which is a
different change.

Tests

Four unit tests, no container, no registered checks — they exercise the concurrency shapes directly:

allOf with timeout cancels nothing	4 wedged tasks still running afterwards
the scope cancels and waits	0 still running once the block exits
awaitAll keeps siblings alive	one check throws, the other three still complete
subtasks are virtual threads	health checks block on sockets

4/4 green; test-compile -pl :dotcms-core --am passes.

Why this stays DO NOT MERGE

StructuredTaskScope is preview in Java 25. It compiles here only because
maven.compiler.enablePreview is already true in parent/pom.xml and the shipped container
already runs --enable-preview (container/tomcat9/bin/setenv.sh). Nothing technically stops us —
which is the uncomfortable part. Its API has been reworked in every release since 21
(StructuredTaskScope.open + Joiner is the JEP 505 shape and does not resemble the JDK 21 one),
so shipping it means signing up to rewrite this method on each JDK upgrade until it is final.

Refs #34154

[claude]

Claude finished @fabrizzio-dotCMS's task in 1m 55s —— View job

---

Code Review

• Read both changed files and the diff
• Trace the production cancellation path through runSingleHealthCheckBlocking
• Check the test coverage against the production path

---

The refactor is clean and the intent is right: allOf(...).get(timeout) never stops the work, StructuredTaskScope does. awaitAll() (not awaitAllSuccessfulOrThrow()) is the correct joiner for the "continue with whatever arrived" contract, the timeout arithmetic is preserved, and leaving waitForOngoingRefreshes on CompletableFuture is well-reasoned. One substantive gap, though — the fix does not actually deliver its headline property on the production path.

New Issues

• 🟠 High: dotCMS/src/main/java/com/dotcms/health/service/HealthStateManager.java:625-629 — The scope cancels the wrong thing; the wedged work still escapes onto the bounded pool. The forked subtask calls runSingleHealthCheckBlocking(healthCheck), which does not run healthCheck.check() on the scope's virtual thread. It delegates to the bounded executor:

  // runSingleHealthCheckBlocking, lines 675-684
  CompletableFuture<HealthCheckResult> future =
      CompletableFuture.supplyAsync(() -> healthCheck.check(), executor);   // bounded pool
  result = future.get(timeoutMs, TimeUnit.MILLISECONDS);                    // virtual thread just waits here

  When the scope times out, it interrupts the virtual thread blocked in future.get(...) — so get() throws InterruptedException, gets caught at line 691, and the subtask returns "normally." But future is never cancelled (there is no future.cancel(true) on the timeout path), so healthCheck.check() keeps running on one of the HealthCheckConfig.THREAD_POOL_SIZE executor threads. That is exactly the bounded-resource leak the PR sets out to close (Reindex mapping guard leaks semaphore permits, permanently killing content indexing (regression in 26.07.13-01) #37038 failure mode) — it's just moved one layer down. close() waits for the virtual thread to finish, not for the executor task, so work still escapes the method.

  The four tests pass because they fork(this::wedgedCheck) directly into the scope — an interruptible Thread.sleep on the scope's own virtual thread — which is not the shape the production code uses. No test exercises runAllHealthChecksAndWait() / runSingleHealthCheckBlocking(), so the leak that survives the refactor is untested.

  To actually deliver the property, the check has to run on the forked thread (drop the inner supplyAsync/get and let the scope's timeout be the timeout), or the inner future must be cancelled on the timeout/interrupt path. Fix this →

• 🟡 Medium: dotCMS/src/main/java/com/dotcms/health/service/HealthStateManager.java:15,25 — Duplicate import java.time.Duration; (already imported at line 15, re-added at line 25 by this diff). Harmless to the compiler but redundant; drop line 25.

Notes (non-blocking)

• On scope cancellation the subtask's inner future.get(...) throws InterruptedException, which is caught by the generic catch (Exception e) at line 691 and recorded as "Health check timed out after {individualTimeout}ms" — attributing a scope-level cancellation to the individual check's timeout, and swallowing the interrupt without restoring the flag. Cosmetic, but the stored result will be slightly misleading in the timeout case.
• The DO NOT MERGE rationale (preview API churn across JDK 21→25) is sound and clearly documented.

The concurrency shape is a genuine improvement, but as written the change advertises a cancellation guarantee it doesn't provide for the actual checks. Worth resolving the High finding before this stops being DO NOT MERGE.

• issue-34154-java25-structured-concurrency