Bug 40820

Summary: STOMP errors even when JobsNotificationMethod='polling'
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Pushed to oldstable --- QA Contact: Kyle M Hall (khall) <kyle>
Severity: normal    
Priority: P5 - low CC: andrew, baptiste.wojtkowski, dcook, jonathan.druart, kyle, michael.adamyk, tomascohen
Version: unspecifiedKeywords: rel_24_05_candidate, rel_24_11_candidate, rel_25_05_candidate
Hardware: All   
OS: All   
GIT URL: Initiative type: ---
Sponsorship status: --- Crowdfunding goal: 0
Patch complexity: Trivial patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.11.00,25.05.05,24.11.11
Circulation function:
Bug Depends on: 34070, 35655    
Bug Blocks:    
Attachments: Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling'
Bug 40820: (follow-up) es_indexer_daemon.pl missed on 34070
Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling'
Bug 40820: (follow-up) es_indexer_daemon.pl missed on 34070
Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling'
Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling'
Bug 40820: (follow-up) es_indexer_daemon.pl missed on 34070

Description Tomás Cohen Arazi (tcohen) 2025-09-16 18:05:15 UTC
We added the JobsNotificationMethod preference so people can disable the use of RabbitMQ explicitly and not get the logs flooded with connection errors.

But a patch on bug 34070 reintroduced the warn ♪┏(・o・)┛♪

e2e587273c14f092d71046f21266473445b219c5
Comment 1 Tomás Cohen Arazi (tcohen) 2025-09-16 18:36:29 UTC
Created attachment 186506 [details] [review]
Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling'

The background job workers are showing STOMP/RabbitMQ connection error
warnings even when the JobsNotificationMethod system preference was set
to 'polling', which doesn't require a message broker connection.

This patch adds checks for the JobsNotificationMethod preference before
displaying connection warnings, so warnings only appear when STOMP is
actually expected to be available.

Changes:
- misc/workers/background_jobs_worker.pl: Check syspref before warning
- misc/workers/es_indexer_daemon.pl: Check syspref before warning

Test plan:
1. Set JobsNotificationMethod to 'polling'
2. Run background_jobs_worker.pl - should not show STOMP warnings
3. Run es_indexer_daemon.pl - should not show STOMP warnings
4. Set JobsNotificationMethod to 'STOMP' without RabbitMQ running
5. Run workers - should show appropriate STOMP connection warnings
6. Sign off :-D

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi (tcohen) 2025-09-16 18:36:34 UTC
Created attachment 186507 [details] [review]
Bug 40820: (follow-up) es_indexer_daemon.pl missed on 34070

The es_indexer_daemon.pl was missing handling for the case where
Koha::BackgroundJob->connect() returns undef without throwing
an exception (which happens when connection fails silently).

This adds the missing warning for that case, matching the behavior
in background_jobs_worker.pl.

Test plan:
1. Set JobsNotificationMethod to 'STOMP'
2. Configure invalid STOMP settings that cause connect() to return undef
3. Run es_indexer_daemon.pl - should show connection warning
4. Verify jobs are still processed via database polling

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Comment 3 David Cook 2025-09-17 00:07:49 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #0)
> We added the JobsNotificationMethod preference so people can disable the use
> of RabbitMQ explicitly and not get the logs flooded with connection errors.
> 
> But a patch on bug 34070 reintroduced the warn ♪┏(・o・)┛♪

It took me a few minutes to wrap my head around this one but I see it now. Bug 34070 added the following:
'$error ||= "Cannot connect to the message broker, the jobs will be processed anyway" unless $conn;'

I didn't even know about bug 35655 until this moment.

> e2e587273c14f092d71046f21266473445b219c5

I don't get anything for that commit hash?

--

At a glance I think these patches should make sense...
Comment 4 Tomás Cohen Arazi (tcohen) 2025-09-17 11:48:22 UTC
(In reply to David Cook from comment #3)
> (In reply to Tomás Cohen Arazi (tcohen) from comment #0)
> > We added the JobsNotificationMethod preference so people can disable the use
> > of RabbitMQ explicitly and not get the logs flooded with connection errors.
> > 
> > But a patch on bug 34070 reintroduced the warn ♪┏(・o・)┛♪
> 
> It took me a few minutes to wrap my head around this one but I see it now.
> Bug 34070 added the following:
> '$error ||= "Cannot connect to the message broker, the jobs will be
> processed anyway" unless $conn;'
>
> I didn't even know about bug 35655 until this moment.
> 
> > e2e587273c14f092d71046f21266473445b219c5
> 
> I don't get anything for that commit hash?

In a hurry, I just grabbed the hash from the submitted patch on bug 34070 https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182574

but you guessed right.

I believe 34070 patches were wrong because they treated the 'skip if polling' behavior as an error by not acknowledging bug 35655. The code has been there for a good while and this caused a regression.

> At a glance I think these patches should make sense...

The patches are trivial and just wrap things around a check on JobsNotificationMethod='STOMP' which is also correct. But if we don't call ->connect() unless STOMP, then we don't need the code handling non-STOMP in ->connect. So any of the refactoring efforts should clear this for good I'd say. I might try to resurrect those patches...
Comment 5 Jonathan Druart 2025-09-19 08:46:29 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2025-09-19 08:53:32 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2025-09-19 08:57:40 UTC
I don't understand the discussion. 

> I might try to resurrect those patches...

The patches look good to me and I am ready to signoff. What's missing?
Comment 8 Tomás Cohen Arazi (tcohen) 2025-09-19 11:33:40 UTC
(In reply to Jonathan Druart from comment #7)
> I don't understand the discussion. 
> 
> > I might try to resurrect those patches...
> 
> The patches look good to me and I am ready to signoff. What's missing?

On this bug, there's nothing missing.
Comment 9 Jonathan Druart 2025-09-19 12:12:32 UTC
Created attachment 186620 [details] [review]
Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling'

The background job workers are showing STOMP/RabbitMQ connection error
warnings even when the JobsNotificationMethod system preference was set
to 'polling', which doesn't require a message broker connection.

This patch adds checks for the JobsNotificationMethod preference before
displaying connection warnings, so warnings only appear when STOMP is
actually expected to be available.

Changes:
- misc/workers/background_jobs_worker.pl: Check syspref before warning
- misc/workers/es_indexer_daemon.pl: Check syspref before warning

Test plan:
1. Set JobsNotificationMethod to 'polling'
2. Run background_jobs_worker.pl - should not show STOMP warnings
3. Run es_indexer_daemon.pl - should not show STOMP warnings
4. Set JobsNotificationMethod to 'STOMP' without RabbitMQ running
5. Run workers - should show appropriate STOMP connection warnings
6. Sign off :-D

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2025-09-19 12:12:34 UTC
Created attachment 186621 [details] [review]
Bug 40820: (follow-up) es_indexer_daemon.pl missed on 34070

The es_indexer_daemon.pl was missing handling for the case where
Koha::BackgroundJob->connect() returns undef without throwing
an exception (which happens when connection fails silently).

This adds the missing warning for that case, matching the behavior
in background_jobs_worker.pl.

Test plan:
1. Set JobsNotificationMethod to 'STOMP'
2. Configure invalid STOMP settings that cause connect() to return undef
3. Run es_indexer_daemon.pl - should show connection warning
4. Verify jobs are still processed via database polling

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Kyle M Hall (khall) 2025-09-23 14:48:52 UTC
Created attachment 186816 [details] [review]
Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling'

The background job workers are showing STOMP/RabbitMQ connection error
warnings even when the JobsNotificationMethod system preference was set
to 'polling', which doesn't require a message broker connection.

This patch adds checks for the JobsNotificationMethod preference before
displaying connection warnings, so warnings only appear when STOMP is
actually expected to be available.

Changes:
- misc/workers/background_jobs_worker.pl: Check syspref before warning
- misc/workers/es_indexer_daemon.pl: Check syspref before warning

Test plan:
1. Set JobsNotificationMethod to 'polling'
2. Run background_jobs_worker.pl - should not show STOMP warnings
3. Run es_indexer_daemon.pl - should not show STOMP warnings
4. Set JobsNotificationMethod to 'STOMP' without RabbitMQ running
5. Run workers - should show appropriate STOMP connection warnings
6. Sign off :-D

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall (khall) 2025-09-23 14:50:07 UTC
Created attachment 186817 [details] [review]
Bug 40820: Fix STOMP connection warnings when JobsNotificationMethod is 'polling'

The background job workers are showing STOMP/RabbitMQ connection error
warnings even when the JobsNotificationMethod system preference was set
to 'polling', which doesn't require a message broker connection.

This patch adds checks for the JobsNotificationMethod preference before
displaying connection warnings, so warnings only appear when STOMP is
actually expected to be available.

Changes:
- misc/workers/background_jobs_worker.pl: Check syspref before warning
- misc/workers/es_indexer_daemon.pl: Check syspref before warning

Test plan:
1. Set JobsNotificationMethod to 'polling'
2. Run background_jobs_worker.pl - should not show STOMP warnings
3. Run es_indexer_daemon.pl - should not show STOMP warnings
4. Set JobsNotificationMethod to 'STOMP' without RabbitMQ running
5. Run workers - should show appropriate STOMP connection warnings
6. Sign off :-D

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall (khall) 2025-09-23 14:50:15 UTC
Created attachment 186818 [details] [review]
Bug 40820: (follow-up) es_indexer_daemon.pl missed on 34070

The es_indexer_daemon.pl was missing handling for the case where
Koha::BackgroundJob->connect() returns undef without throwing
an exception (which happens when connection fails silently).

This adds the missing warning for that case, matching the behavior
in background_jobs_worker.pl.

Test plan:
1. Set JobsNotificationMethod to 'STOMP'
2. Configure invalid STOMP settings that cause connect() to return undef
3. Run es_indexer_daemon.pl - should show connection warning
4. Verify jobs are still processed via database polling

Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Lucas Gass (lukeg) 2025-09-26 16:41:20 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 15 Paul Derscheid 2025-10-23 21:11:04 UTC
Nice work everyone!

Pushed to 25.05.x
Comment 16 Baptiste Wojtkowski (bwoj) 2025-10-31 14:54:17 UTC
Pushed into 24.11.x for 24.11.11 nice work everyone