Skip to content

Stop a broker restart from silently killing a service's JMS consumer - #2033

Open
jcschaff wants to merge 4 commits into
masterfrom
fix/jms-consumer-death-2031
Open

Stop a broker restart from silently killing a service's JMS consumer#2033
jcschaff wants to merge 4 commits into
masterfrom
fix/jms-consumer-death-2031

Conversation

@jcschaff

Copy link
Copy Markdown
Member

Fixes #2031.

A broker restart could kill a service's JMS consumer thread permanently while the process stayed
up and the pod stayed Ready. Dev's submit service ran for 6h50m on 2026-08-24 in exactly
that state, consuming nothing.

What actually happened

The dev logs give the whole sequence, and it turned out to be two defects rather than one:

19:51:15.914Z  WARN   JmsFailoverWatchdog  JMS transport interrupted, failover reconnecting   (x3)
19:59:16.954Z  ERROR  ConsumerContextJms   Connection refused
19:59:16.957Z  FATAL  JmsFailoverWatchdog  JMS transport unrecoverable, invoking terminal handler
19:59:16.959Z  ERROR  ConsumerContextJms   Connection refused
19:59:16.960Z  INFO   ConsumerContextJms   ConsumerContextJms@6796aa72 consumer thread exiting.

481 seconds between interrupt and giving up, matching maxReconnectAttempts=20 with the 1s→30s
backoff. Then nothing at all for six hours.

1. The consumer misread a dead session as shutdown. ConsumerContextJms treated every
javax.jms.IllegalStateException out of receive() as deliberate shutdown and broke out of the
poll loop. Half of that is right — close() unblocks a thread parked in receive(), and looping
on a closed consumer would spin. But the provider raises the same exception when the session dies
underneath a consumer we are still meant to be polling, which is the case that most needs
reporting.

2. The terminal condition was detected and then ignored. d58cd1292e bounded the failover
reconnect budget on the reasoning that in K8s a pod restart is the right response to a sustained
broker outage, added jvmExitOnTerminal() as the escape hatch, and left every service on
logOnly() for a future caller to take up. git log -S setFailoverWatchdog returns exactly that
one commit — nobody ever did. So the FATAL above invoked a no-op.

Either defect alone is enough to produce the zombie: fixing only (1) leaves the process alive
around a connection it can never use again, and fixing only (2) leaves the consumer thread dying
silently wherever the watchdog is not attached — attach() installs nothing for a
non-ActiveMQConnection, which matters if the Artemis work ever moves off the OpenWire client.

The change

  • JmsFailoverWatchdog.onTerminalFailure(what, cause) — a route to the terminal action for a
    failure the caller detected itself. The existing transport path folds through it; passing
    "transport" reproduces today's log line exactly.
  • ConsumerContextJms now tests bProcessing alone to recognise shutdown (both closeAll() and
    stopAndClose() clear it before close(), so it is sufficient), and escalates a session lost
    while still processing instead of exiting quietly.
  • submit, sched, data and db build their messaging service with
    createForLongLivedConsumerService(), taking the escape hatch. Short-lived batch processes
    (SolverPreprocessor, SolverPostprocessor, JavaSimulationExecutable, OptimizationBatchServer)
    and the API server keep the log-only default — they outlive neither the broker outage nor their
    own task.
  • docs/MESSAGING.md §8 said the watchdog exits the JVM in production. That was the intent, not
    the behaviour; it now says who opts in.

A restarted pod does not crash-loop while the broker is still down: startupMaxReconnectAttempts=-1
leaves the initial connect unbounded, so the process waits at boot and picks up when the broker
returns.

Tests

ConsumerContextJmsTest — one regression case and two controls.

The failure is injected at the consumer rather than by stopping the broker on purpose. Stopping
the broker also drives the failover transport to the same terminal handler, so that version of
the test passes on the broken code for the wrong reason; here the transport stays healthy and the
consumer's own escalation is the only thing that can fire the latch.

Verified against the pre-fix bytecode (javap-checked, not just assumed):

before after
aSessionLostWhileStillPollingIsEscalated fails passes
anOrdinaryShutdownIsNotEscalated passes passes
aTransientPollFailureIsNotEscalated passes passes

vcell-server Fast group is otherwise unchanged.

Not covered here

Nothing outside the process would still have noticed. #2032 tracks the missing livenessProbe on
these four services — worth doing regardless, since this fix covers one way a consumer can die and
a probe covers all of them.


🤖 Generated with Claude Code

https://claude.ai/code/session_01PFAraxXXN2KvRqgo9GPmv5

jcschaff and others added 4 commits August 25, 2026 09:50
JmsFailoverWatchdog only reaches its terminal action from the TransportListener
that attach() installs, which leaves two gaps: a caller that detects a lost
session for itself has no way to escalate it, and attach() installs nothing at
all for a non-ActiveMQ connection, so any other provider has no route to the
terminal action.

Add onTerminalFailure(what, cause) as that route, and fold the existing
transport path through it. Passing "transport" reproduces the previous log
line exactly, so nothing reading logs today has to change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFAraxXXN2KvRqgo9GPmv5
ConsumerContextJms treated every javax.jms.IllegalStateException out of
receive() as deliberate shutdown and broke out of the poll loop. Half of that
is right: close() unblocks a thread parked in receive(), and looping on a
closed consumer would spin. But the provider raises the same exception when
the session dies underneath a consumer we are still meant to be polling, and
that is not shutdown -- it is the case that most needs reporting.

The thread then ends for the life of the process. Nothing else notices: the
process stays up, the pod stays Ready, and the service consumes nothing. Dev's
submit service sat in exactly that state for 6h50m on 2026-08-24 after a
network outage restarted the brokers, and the only external symptom was the
sim health check timing out, which reads as a compute problem. It took a
log-volume comparison against prod to find (issue #2031).

bProcessing alone already identifies deliberate shutdown -- closeAll() and
stopAndClose() both clear it before close() -- so test it alone, and route a
session lost while still processing to the failover watchdog's terminal
handler.

The test injects the failure at the consumer rather than by stopping the
broker: stopping the broker also drives the failover transport to the same
terminal handler, so that version of the test passes on the broken code for
the wrong reason. Verified against the pre-fix bytecode -- the regression case
fails, and the two controls (ordinary shutdown, transient poll failure) pass
both before and after.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFAraxXXN2KvRqgo9GPmv5
…n it

Section 8 said the watchdog exits the JVM in production. That was the
intent, not the behaviour -- no service had taken the escape hatch, so the
terminal action was a no-op everywhere. Say which services opt in, and
describe the consumer-side route that covers a session lost without the
transport noticing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFAraxXXN2KvRqgo9GPmv5
The failover transport gives up after maxReconnectAttempts, and d58cd12
made that deliberate: in K8s a pod restart is the right response to a
sustained broker outage. It added jvmExitOnTerminal() as the escape hatch and
left every service on the logOnly() default, to be taken up when a service
wanted it.

Nothing ever took it up. The result is that the terminal condition is detected
and logged correctly and then nothing happens -- dev's submit service logged

  FATAL JMS transport unrecoverable, invoking terminal handler

at 19:59:16Z on 2026-08-24 and kept running for another six hours around a
connection that could never be used again.

Take the hatch for the four processes whose whole job is consuming from the
broker. Short-lived batch processes (SolverPreprocessor, SolverPostprocessor,
JavaSimulationExecutable) and the API server keep the log-only default; they
outlive neither the broker outage nor their own task.

A restarted pod does not crash-loop while the broker is still down:
startupMaxReconnectAttempts=-1 leaves the initial connect unbounded, so the
process waits at boot and picks up when the broker returns.

Refs #2031

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFAraxXXN2KvRqgo9GPmv5
@jcschaff
jcschaff force-pushed the fix/jms-consumer-death-2031 branch from d1d558f to 6c597ad Compare August 25, 2026 17:52
jcschaff added a commit that referenced this pull request Sep 4, 2026
Fixes #2034.

The should-run gate skips the fast lane when a push changed only documentation. It
picked its comparison base like this:

    github.event.before, if it resolves      -> the previous tip of this ref
    otherwise HEAD^1                         -> the previous COMMIT

On a push that CREATES a branch, github.event.before is all-zeros and does not resolve,
so it fell through to HEAD^1 and judged the entire branch by its final commit. A branch
ending in a docs commit therefore skipped build, every Fast shard and Quarkus - and
reported CI-Test-group-Fast as pass in about two seconds. PR #2033 shipped that way:
four commits, three of them Java across vcell-server, a docs commit last.

The failure mode is the bad kind. It is silent, it looks identical to a pass, and
regression.yml does not run on pull_request either, so the merge queue was the first
place the code would ever be compiled. It also gets MORE likely with better commit
hygiene, since ending a branch with a docs commit is a normal shape.

Base selection now distinguishes the three cases rather than two:

  - a real github.event.before  -> use it, still the most precise answer for a push to
    an existing ref. An all-zeros value now fails this test explicitly, since it means
    the ref did not exist before and is not a base at all.
  - a merge commit (HEAD^2 exists, i.e. pull_request or merge_group) -> HEAD^1, which
    is exactly the PR's contents, as before.
  - otherwise -> git merge-base with the default branch, which spans the branch however
    many commits it has and reduces to HEAD^1 for a single-commit branch.

Verified against a scratch repository, all four cases:

    new branch, code then docs last   fix: RUN    old: SKIP   <- the bug
    new branch, docs only             fix: SKIP               <- optimisation kept
    push to existing branch, docs     fix: SKIP               <- before still wins
    merge commit                      fix: RUN                <- first parent still used

The gate also now writes its decision to the job summary. A skip was otherwise
indistinguishable from a pass, which is what let this go unnoticed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D71LBYmQNf5J94wPqr81Jx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JMS consumer thread dies silently when a broker restarts, and the pod stays Ready

1 participant