Bug 31342

Summary: process_message_queue can run over the top of itself causing double-up emails
Product: Koha Reporter: Liz Rea <wizzyrea>
Component: Command-line UtilitiesAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: enhancement    
Priority: P5 - low CC: arthur.suzuki, barton, kyle, lucas, m.de.rooy, martin.renvoize, nick, robin, tomascohen, victor, wizzyrea
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.07, 21.11.14
Attachments: Bug 31342: Add execution locking to process_message_queue.pl
Bug 31342: Add execution locking to process_message_queue.pl
Bug 31342: Add execution locking to process_message_queue.pl

Description Liz Rea 2022-08-11 15:56:33 UTC
IF a process message queue takes too long and happens to hit the next scheduled run, messages will be duplicated and eventually sent twice.

It would be nice to prevent that without changing the cron definition.

Liz

(PS I am aware of the existence of 15529, Tomas asked me to file this new bug as he has a different idea and didn't want to wade into all of the existing drama around the solution on 15529)
Comment 1 Tomás Cohen Arazi 2022-08-12 09:49:57 UTC
*** Bug 15529 has been marked as a duplicate of this bug. ***
Comment 2 Tomás Cohen Arazi 2022-08-12 09:56:55 UTC
Created attachment 139045 [details] [review]
Bug 31342: Add execution locking to process_message_queue.pl

This patch makes the script use an execution lock as provided by
Koha::Script. Previous attempt got too complex and contentious, and
given we have a simple way to achieve the same thing, I decided to
submit an alternative approach.

To test:
1. Apply this patch
2. Add the following content to misc/cronjobs/process_message_queue.pl
   on line 83:

  sleep 100;

3. Save the file
4. Have two terminals open
5. On the first one, run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl
6. On the second one, run the same
=> SUCCESS: The second one dies telling it had to skip the run
7. Undo your changes:
   $ git checkout misc/cronjobs/process_message_queue.pl
8. Sign off :-D

Sponsored-by: ByWater Solutions
Comment 3 Liz Rea 2022-08-12 14:31:57 UTC
Created attachment 139057 [details] [review]
Bug 31342: Add execution locking to process_message_queue.pl

This patch makes the script use an execution lock as provided by
Koha::Script. Previous attempt got too complex and contentious, and
given we have a simple way to achieve the same thing, I decided to
submit an alternative approach.

To test:
1. Apply this patch
2. Add the following content to misc/cronjobs/process_message_queue.pl
   on line 83:

  sleep 100;

3. Save the file
4. Have two terminals open
5. On the first one, run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl
6. On the second one, run the same
=> SUCCESS: The second one dies telling it had to skip the run
7. Undo your changes:
   $ git checkout misc/cronjobs/process_message_queue.pl
8. Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Liz Rea <liz@bywatersolutions.com>
Comment 4 Marcel de Rooy 2022-08-17 11:30:12 UTC
I do not really mind about pushing this dev. But I wonder if we really address the problem here. Could we do something with status in msg queue itself?

What now might happen, is not getting a lock for some reason resulting in no mails from the system at all.
This sounds worse than receiving two mails to me.
Comment 5 Liz Rea 2022-08-24 14:26:07 UTC
As someone who has experienced both conditions, from the point of view of libraries I've had experience with, they'd rather get no mails than two. 

Double mails says to library patrons "the library doesn't know what they are doing and their system sucks" - no mail says "the internet is glitchy," which is true.
Comment 6 Liz Rea 2022-08-24 14:31:53 UTC
More succinctly: double mail is a PR problem, no mail is less so.
Comment 7 Kyle M Hall 2022-09-16 18:08:58 UTC
Created attachment 140733 [details] [review]
Bug 31342: Add execution locking to process_message_queue.pl

This patch makes the script use an execution lock as provided by
Koha::Script. Previous attempt got too complex and contentious, and
given we have a simple way to achieve the same thing, I decided to
submit an alternative approach.

To test:
1. Apply this patch
2. Add the following content to misc/cronjobs/process_message_queue.pl
   on line 83:

  sleep 100;

3. Save the file
4. Have two terminals open
5. On the first one, run:
   $ kshell
  k$ misc/cronjobs/process_message_queue.pl
6. On the second one, run the same
=> SUCCESS: The second one dies telling it had to skip the run
7. Undo your changes:
   $ git checkout misc/cronjobs/process_message_queue.pl
8. Sign off :-D

Sponsored-by: ByWater Solutions
Signed-off-by: Liz Rea <liz@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Tomás Cohen Arazi 2022-09-19 18:17:55 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 9 Lucas Gass 2022-10-31 22:20:28 UTC
I don't see this as an enhancement so I am backporting it to 22.05.x for the upcoming 22.05.07 release
Comment 10 Arthur Suzuki 2022-11-14 16:17:34 UTC
applied to 21.11 for 21.11.14
Comment 11 Victor Grousset/tuxayo 2022-11-24 20:54:44 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.