fix(rabbitmq): bound a publish so it cannot hold a transaction open (#7630) - #7631
fix(rabbitmq): bound a publish so it cannot hold a transaction open (#7630)#7631Romuald Lemesle (RomuDeuxfois) wants to merge 1 commit into
Conversation
|
📖 Documentation check — 3 functional file(s) changed, 0 documentation file(s) changed. Suggestions (non-blocking)
|
There was a problem hiding this comment.
Pull request overview
This PR adds containment mechanisms around RabbitMQ publishing so that a stalled broker can’t indefinitely block basicPublish and keep database transactions (and therefore row locks / the Hikari pool) held open, preventing platform-wide outages during broker disk/memory alarm conditions.
Changes:
- Add connection-level timeouts/heartbeat on the shared RabbitMQ
ConnectionFactory. - Run
RabbitmqService.publish()on a separate executor and enforce a publish timeout (configurable viaopenaev.rabbitmq.publish-timeout-ms). - Add a focused test suite for publish timeout vs healthy/error broker behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openaev-model/src/main/java/io/openaev/service/RabbitmqService.java | Adds bounded-time publish via an executor + timeout and shuts the executor down on bean destroy. |
| openaev-model/src/main/java/io/openaev/driver/RabbitmqDriver.java | Adds connection timeout / handshake timeout / heartbeat to fail fast on unresponsive brokers. |
| openaev-api/src/test/java/io/openaev/service/RabbitmqServicePublishTest.java | New unit tests validating timeout behavior, success path, and surfacing broker IO errors. |
| openaev-api/src/main/resources/application.properties | Introduces openaev.rabbitmq.publish-timeout-ms with documentation. |
Suppressed comments (2)
openaev-model/src/main/java/io/openaev/service/RabbitmqService.java:85
- issue (blocking):
newFixedThreadPooldoesn't actually cap the number of pending publishes (only the number of active threads). Use aThreadPoolExecutorwith a bounded queue (or aSynchronousQueue) so publish requests fail fast once the pool is saturated, keeping memory and downstream load bounded during a broker stall.
private final ExecutorService publishExecutor =
Executors.newFixedThreadPool(
PUBLISH_THREADS,
runnable -> {
Thread thread = new Thread(runnable, "rabbitmq-publish");
openaev-model/src/main/java/io/openaev/service/RabbitmqService.java:147
- issue (blocking): once the publish executor is saturated (all threads stuck + queue full),
submit(...)will throwRejectedExecutionException. Today that would bubble as an unchecked exception; it’s better to convert this into a controlled failure (e.g.,TimeoutException) so callers consistently roll back the inject instead of failing with an unexpected runtime error.
Future<Void> pending =
publishExecutor.submit(
() -> {
doPublish(injectType, publishedJson);
return null;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| import java.util.concurrent.ExecutionException; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.Executors; | ||
| import java.util.concurrent.Future; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.TimeoutException; |
| @Test | ||
| @DisplayName("given a broker that never answers should give up instead of blocking the caller") | ||
| void given_brokerThatNeverAnswers_should_giveUp() throws Exception { | ||
| // A broker under a memory or disk alarm accepts the connection then stops draining: the publish | ||
| // never returns on its own. | ||
| CountDownLatch release = new CountDownLatch(1); | ||
| when(connectionFactory.newConnection()) | ||
| .thenAnswer( | ||
| invocation -> { | ||
| release.await(30, TimeUnit.SECONDS); | ||
| throw new IOException("released by the test"); | ||
| }); | ||
|
|
||
| long startedAt = System.nanoTime(); | ||
| assertThrows(TimeoutException.class, () -> rabbitmqService.publish("http", "{}")); | ||
| long elapsedMs = (System.nanoTime() - startedAt) / 1_000_000; | ||
|
|
||
| // The caller holds the inject's database transaction: it must come back on its own. | ||
| assertTrue(elapsedMs < 10_000, "publish returned after " + elapsedMs + " ms"); | ||
| release.countDown(); | ||
| } |
33ea5b8 to
5ac9ed6
Compare
|
✅ Container vulnerability scan — Passed Previously reported findings are no longer present.
View workflow run · Standard JSON report · UBI9 JSON report Updated from CI run attempt 1. |
5ac9ed6 to
99a60eb
Compare
Executor.executeExternal publishes inside the inject's transaction. A broker under a memory or disk alarm accepts the connection then stops draining its sockets, so basicPublish blocks - and Java has no write timeout to bound it. The job thread parks, the transaction stays open with its row locks, the Hikari pool drains and the whole platform stops answering, since the session store is in the same database. Run the publish on an executor and give up after openaev.rabbitmq.publish-timeout-ms. The worker stays parked - a socket write is not interruptible - but the caller returns, its transaction rolls back and the inject fails instead of the platform. The executor is sized on the database pool, which already bounds how many publishes can run at once. Also drop the connection timeout from the client default of 60s to 10s: publish opens a connection per call, so an unreachable broker would tie a worker up for a full minute.
99a60eb to
71a1220
Compare
Proposed changes
Sending an inject to RabbitMQ happens inside its database transaction, and nothing made that send give up. Java has no socket write timeout, so a broker that stops reading parks the thread forever, holding a connection until the pool is empty and the platform stops answering.
publish()runs on a separate thread and gives up afteropenaev.rabbitmq.publish-timeout-ms, 30 s. Only the inject fails.Contains the damage, does not remove the cause (#7630).
Checks done
ExecutorTest,HealthCheckServiceTestandSimulationInjectApiTestpass. Nothing tests the timeout itself.