Fix flaky CI tests: HTTP/2 hangs, broker/txn ordering races, and BrokerPool shutdown safety

[duncdrum]

Addresses frequent causes of CI flake that PRs have been struggling with. Most changes are test-hardening and infrastructure fixes; the WebSocket work includes deliberate production fixes for cancel/timeout reliability under concurrent load.

Summary

Six categories of flaky-test fix, each with a distinct root cause:

1. BrokerPool / TransactionManager shutdown safety
2. HTTP/2 RST_STREAM hangs in test HTTP clients
3. DBBroker / Txn try-with-resources close-order races
4. WebSocket query-cancel visibility and lifecycle races
5. WebSocket cancel protocol and serialize-path polling
6. WebSocket max-execution-time wall-clock backstop (+ split unit/integration tests)

Root Causes and Fixes

1 — BrokerPool shutdown safety (BrokerPool.java, TransactionManager.java)

BrokerPool.get() checked only isShuttingDown() (terminal SHUTDOWN state) in its wait loop. Threads waiting for a free broker during SHUTTING_DOWN_SYSTEM_MODE — where securityManager is already nulled — could spin indefinitely. Fix: throw EXistException immediately when the pool is in any shutdown state (isShuttingDownOrDown()), and tighten the loop guard the same way.

TransactionManager.processSystemTasks() called pool.getSecurityManager() without checking pool state first, causing NPE during SHUTTING_DOWN_SYSTEM_MODE. Fix: early return guarded by isShuttingDownOrDown().

TransactionManagerTestHelper updated to expect the new isShuttingDownOrDown() call, which broke the EasyMock strict mock.

2 — RST_STREAM / HTTP/2 hang (AbstractHttpTest.java, MoveResourceTest.java)

JDK HttpClient defaults to HTTP/2. Under load the embedded Jetty sends RST_STREAM frames that silently stall connections. This caused GetParameterTest to hang for 38 minutes on CI, triggering the 45-minute Maven unit-test step timeout.

AbstractHttpTest.newHttpClient() is the shared factory for 11 RESTTest subclasses — forcing HTTP_1_1 there fixes all of them. MoveResourceTest.CheckThread had its own inline client with the same default; same fix applied. Also catch IOException in CheckThread's retry loop — transient connection failures arrive as IOException before any HTTP status, so the existing HTTP 5xx retry never fired.

XMLDBAuthenticateTest and AttributeTest were investigated but deliberately left unchanged. Both use CookieManager for session persistence; forcing HTTP/1.1 on those clients caused session-invalidation tests to return HTTP 500 instead of 200. Those tests are not affected by RST_STREAM hangs, so the fix does not apply there.

3 — DBBroker/Txn close-order race (DomEnhancingNodeProxyAdapterTest, DeepEmbeddedBackupRestoreTest, XQueryTrigger*Test, SendEmailIT)

BrokerPool.get() is thread-local: it reuses the broker already active on the thread, or draws a fresh one if none is active. Java closes try-with-resources variables in reverse declaration order, so declaring Txn before DBBroker can leave Txn.close() calling pool.getBroker() after the thread-local broker was already returned — IllegalStateException in NativeBroker.removeCurrentTransaction().

Fix: declare DBBroker before Txn in every affected block so the thread holds a broker across the entire transaction lifecycle.

XQueryUpdateTest uses a withBroker() helper plus Assume.assumeFalse(pool.isShuttingDownOrDown()) so a pool failure in one test does not cascade through subsequent @Before/@After calls.

4 — WebSocket cancel visibility (XQueryWatchDog.java, EvalWebSocketEndpointTest.java)

EvalWebSocketEndpointTest.cancellation was coin-flip pass/fail under CI.

JMM data race on terminate: kill() runs on the WebSocket message-handler thread; proceed() reads terminate on the query-execution thread. The field was a plain boolean. Fix: private volatile boolean terminate.

Cancel sent before watchdog registered: cancelQuery() looks up the watchdog in a registry; if cancel arrives before registration, it is silently dropped. Fix: wait on a progressLatch (signalled when progress / evaluating arrives) before sending cancel.

GC pressure in the test query: string($i) over hundreds of millions of iterations caused multi-second STW GC pauses on CI, during which proceed() could not run and cancel appeared late. Fix: bounded FLWOR with return () (no per-iteration allocation).

5 — watchdog.reset() race (QueryExecutor.java)

xquery.execute(resetContext=true) calls watchDog.reset() after the evaluating signal is sent. On slow CI, a client cancel roundtrip could complete between kill() and that internal reset(), clearing terminate and dropping the cancel. Fix: call watchDog.reset() before registerQuery(), pass resetContext=false to execute(), and call context.reset() in finally.

6 — Cancel protocol, serialize polling, and wall-clock timeout (EvalProtocol.java, EvalWebSocketEndpoint.java, QueryExecutor.java, WallClockQueryTimeout.java)

Cancel was fire-and-forget: no response when the query id was unknown; kill() during non-streaming serialize was never observed because proceed() only runs during evaluation. Fix: send error / cancelling acks on cancel; check isTerminating() / proceed() before serializeAll().

Cooperative timeout starvation: max-execution-time is only checked inside proceed(). Under CPU pressure the eval thread may not run for long stretches after evaluating, so the client never received a terminal message (the failure mode behind intermittent maxExecutionTime CI failures). Fix: schedule a daemon wall-clock timer when max-execution-time > 0; on expiry kill() the query and send a single terminal error if no other response was sent yet (terminalResponseSent gate prevents duplicates).

Test split: unit tests cover proceed() timeout and WallClockQueryTimeout scheduling; the WebSocket maxExecutionTime test is a short smoke (1s limit + 4s client wait) that relies on the server backstop rather than long client latch slack.

Known limitations (intentional scope)

• Wall-clock backstop is WebSocket /ws/eval only — REST (XQueryServlet) still uses cooperative timeout only. No REST flakes were reported in this work.
• max-execution-time applies from watchdog reset() at the start of evaluation (after compile), not from receipt of the WebSocket message — consistent with XQuery.execute() elsewhere.
• Dual enforcement: cooperative watchdog when proceed() runs, plus wall-clock for client response guarantee.

What Changed

File	Change
BrokerPool.java	Throw on shutdown in fast-path; isShuttingDownOrDown() in wait loop
TransactionManager.java	Early return in processSystemTasks() when pool is shutting down
TransactionManagerTestHelper.java	Add isShuttingDownOrDown() mock expectation
AbstractHttpTest.java	Force HTTP_1_1 in newHttpClient()
MoveResourceTest.java	Force HTTP_1_1 in CheckThread; catch IOException in retry
XQueryWatchDog.java	volatile boolean terminate
EvalProtocol.java	PHASE_CANCELLING
EvalWebSocketEndpoint.java	Cancel ack: error if unknown query, cancelling progress if accepted
QueryExecutor.java	resetContext=false; serialize-path watchdog polling; wall-clock timeout + terminal gate
WallClockQueryTimeout.java	Daemon scheduled backstop for max-execution-time
EvalWebSocketEndpointTest.java	Bounded queries, cancel/timeout smoke tests, connection-cleanup hygiene
WallClockQueryTimeoutTest.java	Unit tests for schedule / cancel
XQueryWatchDogTest.java	Unit test for proceed() timeout
XQueryUpdateTest.java	withBroker() helper + shutdown guard
DomEnhancingNodeProxyAdapterTest.java	Swap DBBroker/Txn declaration order
DeepEmbeddedBackupRestoreTest.java	Swap DBBroker/Txn declaration order
XQueryTrigger*Test.java	Swap DBBroker/Txn declaration order
SendEmailIT.java	Swap DBBroker/Txn declaration order
AGENTS.md	Update known-build-issues / excluded-test notes
exist-core/pom.xml	Correct FragmentsTest exclusion comment

Test Plan

• TxnTest (7) + ReusableTxnTest (9) — 16/16 pass locally
• DomEnhancingNodeProxyAdapterTest — 6/6 pass locally
• XQueryTriggerSetUidTest, XQueryTriggerSetGidTest, XQueryTriggerChainTest — 3/3 pass locally
• DeepEmbeddedBackupRestoreTest — passes locally
• XQueryWatchDogTest — including proceed() timeout
• WallClockQueryTimeoutTest
• EvalWebSocketEndpointTest — 23/23 pass locally (cancellation, maxExecutionTime, rapidCancel, streaming, etc.)
• FragmentsTest — confirmed still hangs locally; exclusion retained
• XMLDBAuthenticateTest, AttributeTest — HTTP/1.1 fix tested and reverted
• CI ubuntu unit job after rebase push

[joewiz]

I've never been so happy to see a green checkmark.

[duncdrum]

this isn't the final shape, the green run was lucky, I m working on it

[duncdrum]

ok should be good to go now, had multiple all green runs during rebases with force pushes.

[dizzzz]

@reinhapa 😀

[duncdrum]

🧙 🪄 Fluctuo Stabilis

[joewiz]

[This response was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]

High-effort review (against the current diff); findings independently verified against the branch code. The HTTP/2-pinning de-flake is reasonable. The main concerns are concurrency defects in the new WebSocket cancel/timeout machinery.

1. Concurrent sendText on the non-thread-safe Basic remote → uncaught IllegalStateException

EvalWebSocketEndpoint.java (handleCancel) / QueryExecutor.java

The eval runs on a separate ws-eval-worker thread pool (Executors.newCachedThreadPool), and its sendProgress/sendResult calls go through session.getBasicRemote().sendText(...). handleCancel runs on the WebSocket container's message thread (onMessage) and also calls session.getBasicRemote().sendText(...). The Basic remote is not thread-safe, so a client that cancels while its query is still streaming (the normal case) can drive two concurrent sendText calls on the same remote → IllegalStateException ("remote endpoint was in state [TEXT_FULL_WRITING]"). The send helpers / handleCancel catch only IOException, so the RuntimeException propagates out of onMessage and tears down the session / corrupts the frame. Previously handleCancel only logged — so this is a regression. The Basic remote needs single-threaded access (serialize all sends through the eval worker, or use a per-session send lock / the async remote).

2. One shared blocking timeout scheduler → a stalled client blocks every query's kill

WallClockQueryTimeout.java

private static final ScheduledExecutorService EXECUTOR = Executors.newSingleThreadScheduledExecutor(...) — a single shared scheduler runs every query's timeout callback, and handleWallClockTimeout does watchDog.kill(0) followed by a blocking sendError(...) → getBasicRemote().sendText. If one client's connection stalls, that send blocks the single scheduler thread indefinitely, so every other query's timeout callback queues behind it and their watchDog.kill(0) never runs — runaway queries server-wide are not terminated at their max-execution-time, defeating the very backstop this PR adds. The timeout callback must not block on client I/O.

3. Late cancel after success reports a successful query as failed

EvalWebSocketEndpoint.java (handleCancel)

handleCancel() sends an error frame ("Query not found or already completed") when cancelQuery() returns false, instead of only LOG.debug. Since execute()'s finally unregisters the query on success, a cancel arriving just after (or racing) normal completion takes the false branch — so the client receives an extra error frame for a query that actually returned its full result.

Lower-severity items

• BrokerPool.get() throws during shutdown — the broker-creation path now throws EXistException whenever isShuttingDownOrDown(); in-flight requests, scheduled jobs, and unguarded test teardowns calling get() during shutdown can intermittently throw (potentially the same CI flakiness this targets).
• Cancelled timeout futures retain their closures — newSingleThreadScheduledExecutor has removeOnCancelPolicy=false, so cancelled ScheduledFutures stay in the delay queue until their full max-execution-time, retaining session/query-string references (heap growth under load). Use ScheduledThreadPoolExecutor(1) + setRemoveOnCancelPolicy(true).
• Backstop is ws/eval-only — the can't-fire-under-CPU-pressure weakness exists for every watchDog.setTimeout() consumer (REST, RESTXQ, scheduler, XML-RPC); generalizing it in XQueryWatchDog would cover all entry points.
• Test masking / coverage loss — XQueryUpdateTest wraps every test in Assume.assumeFalse(isShuttingDownOrDown()) (a lifecycle regression now silently skips instead of failing); AbstractHttpTest.newHttpClient() pins HTTP_1_1, removing HTTP/2 coverage from every test using the helper.
• Broker/txn ordering worked around — fixed by manual try-with-resources reordering across ~6 test files plus a "caller must hold an active broker" precondition, rather than fixing Txn.close()'s ambient-broker dependency at the source.
• UK spelling — EvalProtocol.PHASE_CANCELLING = "cancelling" (US is "canceling"). Note this is a wire-protocol constant value, so changing it later is client-visible — worth settling now.

Bottom line

Findings 1–3 are real concurrency defects in the new cancel/timeout code; the first two (unsynchronized concurrent sendText, and a shared blocking scheduler that defeats the backstop) are serious enough to address before merge.

[duncdrum]

Thx @joewiz mostly good findings. Making sure our codebase fully runs on http 2 is out of scope here. Our ci is stalling because of http 2 rst-stream, this affects every local build and ci run. Getting reliable builds back is top priority. Http 2 feature support is something to pick up in a separate pr.

[duncdrum]

Thanks @joewiz — addressed findings 1–3 and two of the lower-severity items. The HTTP/2 coverage point is out of scope as noted.

Concurrent sendText (finding 1) + false error on late cancel (finding 3) — handleCancel no longer calls getBasicRemote().sendText() from the WebSocket container's message thread. It now just calls cancelQuery() + debug-log. The eval worker already owns all terminal frames; a cancel after completion sends nothing rather than a spurious error. (ea6b756)

Blocking wall-clock scheduler (finding 2) — handleWallClockTimeout switched from getBasicRemote().sendText() to getAsyncRemote().sendText(), so the shared scheduler thread never blocks on client I/O. Also enabled setRemoveOnCancelPolicy(true) to release session/query closures immediately on cancel. (ea6b756)

Assume.assumeFalse masking — removed from XQueryUpdateTest.withBroker(); the BrokerPool.get() guard added in this PR throws EXistException if the pool is down, producing a visible failure instead of a silent skip. The @After teardown retains its own guard (correct — teardown should not compound an upstream failure). (3637820)

Wall-clock backstop generalized into XQueryWatchDog — setTimeout() and setTimeoutFromOption() now schedule a kill(0) on a shared daemon-thread ScheduledThreadPoolExecutor (removeOnCancelPolicy=true). This covers REST, RESTXQ, XML-RPC, and the scheduler automatically; the WebSocket WallClockQueryTimeout still handles the transport-specific async error send. (c71fb4b)

[line-o]

I can reliably run the unit tests on my local machine again.
From everything I can see this does increase overall reliability and stability of exist-core
Thanks @duncdrum!