Bug 38156

Summary:	Auto renew cron job mangles digest notices when parallel processing is enabled
Product:	Koha	Reporter:	Emily Lamancusa (emlam) <emily.lamancusa>
Component:	Command-line Utilities	Assignee:	Emily Lamancusa (emlam) <emily.lamancusa>
Status:	RESOLVED FIXED	QA Contact:	Tomás Cohen Arazi (tcohen) <tomascohen>
Severity:	major
Priority:	P5 - low	CC:	dcook, lucas, martin.renvoize, nick, phil, robin
Version:	Main
Hardware:	All
OS:	All
Change sponsored?:	---	Patch complexity:	Small patch
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:	24.11.00
Circulation function:
Bug Depends on:	29507
Bug Blocks:
Attachments:	Bug 38156: [DO NOT PUSH] Test data to reproduce the issue Bug 38156: Sort issues by borrowernumber before parallel chunking Bug 38156: Sort issues by borrowernumber before parallel chunking Bug 38156: Sort issues by borrowernumber before parallel chunking

Description Emily Lamancusa (emlam) 2024-10-11 19:23:13 UTC

When the automatic renewal cron job is running multiple loops simultaneously, it aims to process all of the renewals for any given patron together in one chunk, in order to avoid data conflicts. To accomplish this, it starts a new data chunk each time it encounters a new patron.

However, in practice, all of a patron's checkouts for a given day might not end up being consecutive in the database table if multiple patrons are using different checkout stations at the same time. (Or if differing circulation rules cause checkouts from different days to auto-renew at the same time.) If a patron's checkouts are not all consecutive in the database, they will end up being split over separate chunks.

This causes digest notices not to generate correctly - depending on how the timing works out, a patron may get their renewals split over multiple digests, some renewed items may be missing from digests, or the digest may not generate at all.

Sorting the issues by borrowernumber before splitting them into chunks for processing should solve this.

Comment 1 Emily Lamancusa (emlam) 2024-10-11 20:15:57 UTC

Created attachment 172705 [details] [review]
Bug 38156: [DO NOT PUSH] Test data to reproduce the issue

Comment 2 Emily Lamancusa (emlam) 2024-10-11 20:15:59 UTC

Created attachment 172706 [details] [review]
Bug 38156: Sort issues by borrowernumber before parallel chunking

When the automatic renewal cron job is using parallel processing, it
aims to process all of the renewals for any given patron together in one
chunk to avoid data conflicts. To accomplish this, it starts a new data
chunk each time it encounters a new patron. However, if a patron's
renewing checkouts aren't all consecutive in the database, that patron's
data ends up split across multiple chunks.

We need to sort the issues by borrowernumber before attempting to chunk
them in order to make sure they are chunked and processed correctly.

To test (using KTD default test data):
Setup:
1. Edit the default circulation rule:
    - Set Automatic renewal to "Yes"
    - Set No automatic renewal before to 3
2. Open the following patron accounts in separate tabs:
    - Floyd Delgado
    - Joyce Gaines
    - Edna Acosta
    - Mary Burton
3. Perform the following patron account edits for each of the above
   patrons (and keep the tabs open):
    - Enable automatic renewal notices, and set them to digests only
    - Add a value to the email field
4. Enter the kshell (ktd --shell)
5. Edit /etc/koha/sites/kohadev/koha-conf.xml, and add the following
   lines near the end, just above the </config> and </yazgfs> closing
   tags:
<auto_renew_cronjob>
    <parallel_loops_count>2</parallel_loops_count>
</auto_renew_cronjob>
6. restart_all

Reproducing the issue:
7. Apply the test patch only
8. Run perl generate_checkouts.pl to generate test data
9. perl misc/cronjobs/automatic_renewals.pl -v -c
--> The test patch added output that will show how the renewals were
    chunked into "chunk 0" and "chunk 1" for the two parallel loops.
    Note that the issues for each borrower are not processed nicely in
    one chunk, but are separated across multiple chunks and alternated
    with other borrowers.
10. Check the checkouts for each of the four patrons from above
--> All checkouts should have renewed
11. Check the notices tab for each of the four patrons
--> Notice errors in the automatic renewal digest notices. A patron's
    renewals may be split across multiple digests, a digest may be
    missing renewals, or a patron may not have received a digest at all

Testing the patch:
12. Apply the second patch
13. Reset the due dates on all checkouts so that they will all be
    eligible for automatic renewal again:
    - koha-mysql kohadev
    - UPDATE issues SET date_due=<two days from today>;
14. perl misc/cronjobs/automatic_renewals.pl -v -c
--> Note that the renewals are now correctly chunked by patron
15. Check the checkouts and notices tab for each of the four patrons
--> All checkouts should have renewed, and all patrons should have a
    single new Auto Renewals Digest notice that correctly lists all of
    their renewed items

Comment 3 Phil Ringnalda 2024-10-15 02:24:24 UTC

Created attachment 172763 [details] [review]
Bug 38156: Sort issues by borrowernumber before parallel chunking

When the automatic renewal cron job is using parallel processing, it
aims to process all of the renewals for any given patron together in one
chunk to avoid data conflicts. To accomplish this, it starts a new data
chunk each time it encounters a new patron. However, if a patron's
renewing checkouts aren't all consecutive in the database, that patron's
data ends up split across multiple chunks.

We need to sort the issues by borrowernumber before attempting to chunk
them in order to make sure they are chunked and processed correctly.

To test (using KTD default test data):
Setup:
1. Edit the default circulation rule:
    - Set Automatic renewal to "Yes"
    - Set No automatic renewal before to 3
2. Open the following patron accounts in separate tabs:
    - Floyd Delgado
    - Joyce Gaines
    - Edna Acosta
    - Mary Burton
3. Perform the following patron account edits for each of the above
   patrons (and keep the tabs open):
    - Enable automatic renewal notices, and set them to digests only
    - Add a value to the email field
4. Enter the kshell (ktd --shell)
5. Edit /etc/koha/sites/kohadev/koha-conf.xml, and add the following
   lines near the end, just above the </config> and </yazgfs> closing
   tags:
<auto_renew_cronjob>
    <parallel_loops_count>2</parallel_loops_count>
</auto_renew_cronjob>
6. restart_all

Reproducing the issue:
7. Apply the test patch only
8. Run perl generate_checkouts.pl to generate test data
9. perl misc/cronjobs/automatic_renewals.pl -v -c
--> The test patch added output that will show how the renewals were
    chunked into "chunk 0" and "chunk 1" for the two parallel loops.
    Note that the issues for each borrower are not processed nicely in
    one chunk, but are separated across multiple chunks and alternated
    with other borrowers.
10. Check the checkouts for each of the four patrons from above
--> All checkouts should have renewed
11. Check the notices tab for each of the four patrons
--> Notice errors in the automatic renewal digest notices. A patron's
    renewals may be split across multiple digests, a digest may be
    missing renewals, or a patron may not have received a digest at all

Testing the patch:
12. Apply the second patch
13. Reset the due dates on all checkouts so that they will all be
    eligible for automatic renewal again:
    - koha-mysql kohadev
    - UPDATE issues SET date_due=<two days from today>;
14. perl misc/cronjobs/automatic_renewals.pl -v -c
--> Note that the renewals are now correctly chunked by patron
15. Check the checkouts and notices tab for each of the four patrons
--> All checkouts should have renewed, and all patrons should have a
    single new Auto Renewals Digest notice that correctly lists all of
    their renewed items

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Comment 4 Phil Ringnalda 2024-10-15 02:25:00 UTC

Do we have an award for Best Test Plan of the Month? I have a nomination.

Comment 5 Nick Clemens (kidclamp) 2024-10-15 16:36:27 UTC

Ah, the sorting was lost during a rebase, excellent spotting

Comment 6 Tomás Cohen Arazi (tcohen) 2024-10-16 19:04:35 UTC

Created attachment 172831 [details] [review]
Bug 38156: Sort issues by borrowernumber before parallel chunking

When the automatic renewal cron job is using parallel processing, it
aims to process all of the renewals for any given patron together in one
chunk to avoid data conflicts. To accomplish this, it starts a new data
chunk each time it encounters a new patron. However, if a patron's
renewing checkouts aren't all consecutive in the database, that patron's
data ends up split across multiple chunks.

We need to sort the issues by borrowernumber before attempting to chunk
them in order to make sure they are chunked and processed correctly.

To test (using KTD default test data):
Setup:
1. Edit the default circulation rule:
    - Set Automatic renewal to "Yes"
    - Set No automatic renewal before to 3
2. Open the following patron accounts in separate tabs:
    - Floyd Delgado
    - Joyce Gaines
    - Edna Acosta
    - Mary Burton
3. Perform the following patron account edits for each of the above
   patrons (and keep the tabs open):
    - Enable automatic renewal notices, and set them to digests only
    - Add a value to the email field
4. Enter the kshell (ktd --shell)
5. Edit /etc/koha/sites/kohadev/koha-conf.xml, and add the following
   lines near the end, just above the </config> and </yazgfs> closing
   tags:
<auto_renew_cronjob>
    <parallel_loops_count>2</parallel_loops_count>
</auto_renew_cronjob>
6. restart_all

Reproducing the issue:
7. Apply the test patch only
8. Run perl generate_checkouts.pl to generate test data
9. perl misc/cronjobs/automatic_renewals.pl -v -c
--> The test patch added output that will show how the renewals were
    chunked into "chunk 0" and "chunk 1" for the two parallel loops.
    Note that the issues for each borrower are not processed nicely in
    one chunk, but are separated across multiple chunks and alternated
    with other borrowers.
10. Check the checkouts for each of the four patrons from above
--> All checkouts should have renewed
11. Check the notices tab for each of the four patrons
--> Notice errors in the automatic renewal digest notices. A patron's
    renewals may be split across multiple digests, a digest may be
    missing renewals, or a patron may not have received a digest at all

Testing the patch:
12. Apply the second patch
13. Reset the due dates on all checkouts so that they will all be
    eligible for automatic renewal again:
    - koha-mysql kohadev
    - UPDATE issues SET date_due=<two days from today>;
14. perl misc/cronjobs/automatic_renewals.pl -v -c
--> Note that the renewals are now correctly chunked by patron
15. Check the checkouts and notices tab for each of the four patrons
--> All checkouts should have renewed, and all patrons should have a
    single new Auto Renewals Digest notice that correctly lists all of
    their renewed items

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edit: tidied the code block inline (tcohen)

Comment 7 Katrin Fischer 2024-10-18 11:35:11 UTC

(In reply to Phil Ringnalda from comment #4)
> Do we have an award for Best Test Plan of the Month? I have a nomination.

Fully agreee, also for the problem description! Emily, you rock!

Comment 8 Katrin Fischer 2024-10-18 12:27:47 UTC

Pushed for 24.11!

Well done everyone, thank you!

Comment 9 Lucas Gass (lukeg) 2024-11-19 15:53:31 UTC

Doens't apply cleanly to 24.05.x, any chance for a rebase here?

Comment 10 Lucas Gass (lukeg) 2024-11-20 14:56:41 UTC

Missing 24.05.x deps, so no backport.