Bug 32393 - background job worker explodes if JSON is incorrect
Summary: background job worker explodes if JSON is incorrect
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 32394 32573
Blocks: 32395 32594 32612 32709
  Show dependency treegraph
 
Reported: 2022-12-02 13:20 UTC by Jonathan Druart
Modified: 2023-01-31 14:12 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03


Attachments
Bug 32393: Prevent invalid job to block the job queue (3.80 KB, patch)
2022-12-02 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Prevent invalid job to block the job queue (3.81 KB, patch)
2022-12-14 08:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Remove fork (2.34 KB, patch)
2023-01-05 15:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Split into 2 try catch blocks (2.38 KB, patch)
2023-01-11 11:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Prevent invalid job to block the job queue (3.86 KB, patch)
2023-01-11 17:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32393: Split into 2 try catch blocks (2.43 KB, patch)
2023-01-11 17:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32393: Prevent invalid job to block the job queue (3.87 KB, patch)
2023-01-24 08:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Split into 2 try catch blocks (2.43 KB, patch)
2023-01-24 08:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Deal with the DB fallback part (1.15 KB, patch)
2023-01-24 08:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Prevent invalid job to block the job queue (3.87 KB, patch)
2023-01-24 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Split into 2 try catch blocks (2.43 KB, patch)
2023-01-24 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Deal with the DB fallback part (1.15 KB, patch)
2023-01-24 12:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32393: Prevent invalid job to block the job queue (3.97 KB, patch)
2023-01-24 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32393: Split into 2 try catch blocks (2.53 KB, patch)
2023-01-24 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32393: Deal with the DB fallback part (1.24 KB, patch)
2023-01-24 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32393: (QA follow-up) Add explicit undef response in two catch blocks (1.29 KB, patch)
2023-01-24 13:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-12-02 13:20:17 UTC
If a message that is not (or incorrect) JSON is received by the worker, the script will explode.
Additionally, it will restart and get the same frame, over and over again, which means no other job will be processed.
Comment 1 Jonathan Druart 2022-12-02 13:58:05 UTC
Created attachment 144401 [details] [review]
Bug 32393: Prevent invalid job to block the job queue

I have faced a problem when testing an incorrect version of bug 32370.
The frame sent to the message broker was not a correct JSON encoded
string, and its decoding was obviously failing, exploding the worker
script.
Additionally, as we don't send a ack for this frame, the next pull will
result in processing the same message, and so in the same explosion.
No more messages can be processed!

This patch is logging the error and ack the message to the broker, in
order to not get stuck.

Test plan:
0. Dont' apply this patch
1. Enqueue a bad message
  a. Apply 32370
  b. Comment the following line in Koha::BackgroundJob::enqueue
    $self->set_encoded_json_field( { data => $job_args,    field => 'data' } );
  c. restart_all
  d. Use the batch item modification tool to enqueue a new job
=> Notice the error in the log
=> Note that the status of the job is "new"
=> Inspect rabbitmq queue:
% rabbitmq-plugins enable rabbitmq_management
% rabbitmqadmin get queue=koha_kohadev-long_tasks
You will notice there is a message in the "long_tasks" queue
2. Enqueue a good message
  a. Remove the change from 1.b
  b. restart_all
  c. Enqueue another job
=> Same error in the log
=> Both jobs are new
=> Inspect rabbitmq, there are 2 messages
3. Apply this patch
4. restart_all
=> Second (good) job is finished
=> rabbitmq long_tasks queue is empty

We cannot mark the first job as done, we have no idea which job it was!

QA: Note that this patch is dealing with another problem, not tested in
this test plan. If an exception is not correctly caught by the ->process
method of the job, we won't crash the worker. The job will be marked as
failed.
Comment 2 Tomás Cohen Arazi 2022-12-06 13:13:19 UTC
Comment on attachment 144401 [details] [review]
Bug 32393: Prevent invalid job to block the job queue

Review of attachment 144401 [details] [review]:
-----------------------------------------------------------------

::: misc/background_jobs_worker.pl
@@ +104,5 @@
> +            Koha::Logger->get->warn(sprintf "Job and/or frame not processed - %s", $_);
> +        } finally {
> +            $job->status('failed')->store if $job;
> +            $conn->ack( { frame => $frame } );
> +        };

The finally block is always executed regardless of the exit status of the try block. It reads wrong to be setting the status as failed. That line belongs to the catch IMHO.
Comment 3 Jonathan Druart 2022-12-14 08:28:40 UTC
Created attachment 144569 [details] [review]
Bug 32393: Prevent invalid job to block the job queue

I have faced a problem when testing an incorrect version of bug 32370.
The frame sent to the message broker was not a correct JSON encoded
string, and its decoding was obviously failing, exploding the worker
script.
Additionally, as we don't send a ack for this frame, the next pull will
result in processing the same message, and so in the same explosion.
No more messages can be processed!

This patch is logging the error and ack the message to the broker, in
order to not get stuck.

Test plan:
0. Dont' apply this patch
1. Enqueue a bad message
  a. Apply 32370
  b. Comment the following line in Koha::BackgroundJob::enqueue
    $self->set_encoded_json_field( { data => $job_args,    field => 'data' } );
  c. restart_all
  d. Use the batch item modification tool to enqueue a new job
=> Notice the error in the log
=> Note that the status of the job is "new"
=> Inspect rabbitmq queue:
% rabbitmq-plugins enable rabbitmq_management
% rabbitmqadmin get queue=koha_kohadev-long_tasks
You will notice there is a message in the "long_tasks" queue
2. Enqueue a good message
  a. Remove the change from 1.b
  b. restart_all
  c. Enqueue another job
=> Same error in the log
=> Both jobs are new
=> Inspect rabbitmq, there are 2 messages
3. Apply this patch
4. restart_all
=> Second (good) job is finished
=> rabbitmq long_tasks queue is empty

We cannot mark the first job as done, we have no idea which job it was!

QA: Note that this patch is dealing with another problem, not tested in
this test plan. If an exception is not correctly caught by the ->process
method of the job, we won't crash the worker. The job will be marked as
failed.
Comment 4 Jonathan Druart 2022-12-14 08:29:19 UTC
I've amended the patch with the following change:
-            $job->status('failed')->store if $job;
+            $job->status('failed')->store if $job && @_;
Comment 5 Tomás Cohen Arazi 2022-12-16 12:27:19 UTC
Hi

I tried this:

kohadev-koha@kohadevbox:/kohadevbox/koha$ perl -MTry::Tiny -e 'try { die "Boo"; } catch { warn "catch"; } finally { warn "finally" if $@; };'
catch at -e line 1.
kohadev-koha@kohadevbox:/kohadevbox/koha$ perl -MTry::Tiny -e 'try { die "Boo"; } catch { warn "catch"; } finally { warn "finally" if $_; };'
catch at -e line 1.


In both cases it never prints "finally", so I guess the error buffer is cleared after the catch block. Do you think it would work if we just move the $job->status('failed') to the catch block? It looks cleaner too.
Comment 6 Jonathan Druart 2023-01-04 11:16:01 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> Hi
> 
> I tried this:
> 
> kohadev-koha@kohadevbox:/kohadevbox/koha$ perl -MTry::Tiny -e 'try { die
> "Boo"; } catch { warn "catch"; } finally { warn "finally" if $@; };'
> catch at -e line 1.
> kohadev-koha@kohadevbox:/kohadevbox/koha$ perl -MTry::Tiny -e 'try { die
> "Boo"; } catch { warn "catch"; } finally { warn "finally" if $_; };'
> catch at -e line 1.
> 
> 
> In both cases it never prints "finally", so I guess the error buffer is
> cleared after the catch block. Do you think it would work if we just move
> the $job->status('failed') to the catch block? It looks cleaner too.

It's @_

% perl -MTry::Tiny -e 'try { die "Boo"; } catch { warn "catch"; } finally { warn "finally" if @_; };'
catch at -e line 1.
finally at -e line 1.

IMO it's better in the finally block, in case we add more stuff in catch later.
Comment 7 Jonathan Druart 2023-01-05 13:44:56 UTC
There are 2 ack sent, because of the fork I guess.
Comment 8 Jonathan Druart 2023-01-05 15:40:06 UTC
Created attachment 145064 [details] [review]
Bug 32393: Remove fork
Comment 9 Kyle M Hall 2023-01-05 19:22:37 UTC
(In reply to Jonathan Druart from comment #8)
> Created attachment 145064 [details] [review] [review]
> Bug 32393: Remove fork

Just a thought; removing that fork will cause the workers memory footprint to balloon as soon as the plugins require is evaluated which obviates the bug patch where you introduce the require. I think this pushes us farther in the direction of needing to ack each request before handling the job.
Comment 10 Martin Renvoize 2023-01-06 09:10:23 UTC
I agree that removing the fork is a bad idea.. it was implimented for the reasoning Kyle suggests I believe.. to keep the memory footprint from balooning.
Comment 11 Jonathan Druart 2023-01-06 09:17:04 UTC
I left the patch here yesterday after my day work.
I was working on bug 32481, and thought that this first patch was a good base to work on.
So far I am only trying to make things work correctly. IMO we should not add anything new to the worker, but focus on fixing its behaviours instead.

From bug 32481 comment 22
"The best solution I found is the 2 patches from bug 32393."

I didn't want to add the patches there and add more confusion so just dropped them here.

I still don't have anything good to suggest, and remove the fork is indeed a very bad idea. But fixing the worker's behaviour is a higher priority than memory footprint right now anyway.
Comment 12 Martin Renvoize 2023-01-06 09:34:16 UTC
There's scope creep here.. the try encompasses the process_job which means it's also catching all sorts of other possible failures from each and every processor that's written.

I think we should limit the scope of this bug to only catch bad JSON encoding and leave error handling for individual tasks to the tasks themselves.
Comment 13 Martin Renvoize 2023-01-06 09:52:41 UTC
Also.. if we do want to only acknowledge on process completion, should we not do it within the forked worker?
Comment 14 Jonathan Druart 2023-01-06 12:55:33 UTC
(In reply to Martin Renvoize from comment #12)
> I think we should limit the scope of this bug to only catch bad JSON
> encoding and leave error handling for individual tasks to the tasks
> themselves.

We should not trust the job and catch potential crashes IMO, to prevent a bad job to take the worker down.
Comment 15 David Cook 2023-01-08 23:42:05 UTC
We need to keep the fork, and catching all exceptions makes sense to me. You could say that the background job worker is really a background job server and servers really shouldn't go down. They should be reporting on the failures. 

Of course the tough part here is that the job ID is in the JSON, so if the JSON can't be decoded, there is no way to fail the job and it just stays stuck in "new" forever. 

I haven't been following the bad JSON bugs carefully enough though. Since we control both ends, we shouldn't be creating and transmitting bad JSON...
Comment 16 Marcel de Rooy 2023-01-10 12:35:18 UTC
Just a simple observation:

+        my $job_id;

You are not using this variable (after all).
Comment 17 Marcel de Rooy 2023-01-10 12:40:37 UTC
        } finally {
            $job->status('failed')->store if $job && @_;

Looks like a typo. Did you mean $@ ? And would testing that be enough or even appropriate in a finally block ?
Comment 18 Marcel de Rooy 2023-01-10 12:42:31 UTC
(In reply to Marcel de Rooy from comment #17)
>         } finally {
>             $job->status('failed')->store if $job && @_;
> 
> Looks like a typo. Did you mean $@ ? And would testing that be enough or
> even appropriate in a finally block ?

Seeing comment6 now btw ;)
Comment 19 Marcel de Rooy 2023-01-10 12:54:46 UTC
(In reply to Kyle M Hall from comment #9)
> Just a thought; removing that fork will cause the workers memory footprint
> to balloon as soon as the plugins require is evaluated which obviates the
> bug patch where you introduce the require. I think this pushes us farther in
> the direction of needing to ack each request before handling the job.

Looks like there is consensus on keeping the fork. The problem does not seem to be related to forking.
Did you consider moving the ->ack before the process start as Kyle suggested? It is simpler than the current code with finally. And it is still possible to run the worker without rabbitmq to recover if really needed?
Comment 20 Jonathan Druart 2023-01-10 13:34:29 UTC
(In reply to Marcel de Rooy from comment #19)
> (In reply to Kyle M Hall from comment #9)
> > Just a thought; removing that fork will cause the workers memory footprint
> > to balloon as soon as the plugins require is evaluated which obviates the
> > bug patch where you introduce the require. I think this pushes us farther in
> > the direction of needing to ack each request before handling the job.
> 
> Looks like there is consensus on keeping the fork. The problem does not seem
> to be related to forking.
> Did you consider moving the ->ack before the process start as Kyle
> suggested? It is simpler than the current code with finally. And it is still
> possible to run the worker without rabbitmq to recover if really needed?

See bug 32573.
Comment 21 David Cook 2023-01-10 23:44:29 UTC
(In reply to Martin Renvoize from comment #12)
> There's scope creep here.. the try encompasses the process_job which means
> it's also catching all sorts of other possible failures from each and every
> processor that's written.
> 
> I think we should limit the scope of this bug to only catch bad JSON
> encoding and leave error handling for individual tasks to the tasks
> themselves.

Actually, after looking at the code again, I think that Martin might be right.

I'd have to test it, but in theory the forked process won't hit the "exit" in process_job if it throws a fatal error. 

Ah and that will be why you get that 2nd ACK.
Comment 22 David Cook 2023-01-10 23:52:21 UTC
So I think we should just wrap try{} around the code for getting $job.

If we catch an error retrieving $job, then we log a warning. 

--

If we want to fail a job based on a fatal error in $job->process(), then we need to put a try/catch in the "process_job" function. 

It does need to be separate after all. I think we could rename this bug to be "Add exception handling to background jobs worker".

(Note that this doesn't stop individual jobs from doing their own exception handling within the "process" function. This would just provide default exception handling.)
Comment 23 Jonathan Druart 2023-01-11 11:53:32 UTC
Created attachment 145207 [details] [review]
Bug 32393: Split into 2 try catch blocks
Comment 24 Jonathan Druart 2023-01-11 11:55:49 UTC
I don't understand what you are asking for guys. I've written a follow-up patch that I think does what you want, but I don't see how it is better.

Note that now we are acking early.

Please provide your own version if you are still disagreeing with the patches.
Comment 25 Nick Clemens 2023-01-11 17:07:24 UTC
Created attachment 145216 [details] [review]
Bug 32393: Prevent invalid job to block the job queue

I have faced a problem when testing an incorrect version of bug 32370.
The frame sent to the message broker was not a correct JSON encoded
string, and its decoding was obviously failing, exploding the worker
script.
Additionally, as we don't send a ack for this frame, the next pull will
result in processing the same message, and so in the same explosion.
No more messages can be processed!

This patch is logging the error and ack the message to the broker, in
order to not get stuck.

Test plan:
0. Dont' apply this patch
1. Enqueue a bad message
  a. Apply 32370
  b. Comment the following line in Koha::BackgroundJob::enqueue
    $self->set_encoded_json_field( { data => $job_args,    field => 'data' } );
  c. restart_all
  d. Use the batch item modification tool to enqueue a new job
=> Notice the error in the log
=> Note that the status of the job is "new"
=> Inspect rabbitmq queue:
% rabbitmq-plugins enable rabbitmq_management
% rabbitmqadmin get queue=koha_kohadev-long_tasks
You will notice there is a message in the "long_tasks" queue
2. Enqueue a good message
  a. Remove the change from 1.b
  b. restart_all
  c. Enqueue another job
=> Same error in the log
=> Both jobs are new
=> Inspect rabbitmq, there are 2 messages
3. Apply this patch
4. restart_all
=> Second (good) job is finished
=> rabbitmq long_tasks queue is empty

We cannot mark the first job as done, we have no idea which job it was!

QA: Note that this patch is dealing with another problem, not tested in
this test plan. If an exception is not correctly caught by the ->process
method of the job, we won't crash the worker. The job will be marked as
failed.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 26 Nick Clemens 2023-01-11 17:07:29 UTC
Created attachment 145217 [details] [review]
Bug 32393: Split into 2 try catch blocks

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens 2023-01-11 17:09:35 UTC
Testing notes:
1.b didn't work for me, I did:
-    my $json_args = $self->_json->encode($job_args);
+    #my $json_args = $self->_json->encode($job_args);
+    my $json_args=$job_args;

To check the queues I just did:
rabbitmqctl list_queues
Comment 28 David Cook 2023-01-15 23:36:27 UTC
(In reply to Jonathan Druart from comment #24)
> I don't understand what you are asking for guys. I've written a follow-up
> patch that I think does what you want, but I don't see how it is better.

Looks good. Thanks for that.
Comment 29 Jonathan Druart 2023-01-17 06:52:48 UTC
Upping the severity as jobs can get stuck and worker explodes.
Comment 30 Marcel de Rooy 2023-01-20 10:18:38 UTC
For me this statement is also in the scope of this report (but not touched yet):

        while ( my $job = $jobs->next ) {
            my $args = $job->json->decode($job->data);
Comment 31 Marcel de Rooy 2023-01-20 10:23:18 UTC
Note that I agree with acking earlier. We do this implicitly here. This would obsolete the separate report (32573).
Comment 32 Jonathan Druart 2023-01-24 08:22:53 UTC
Created attachment 145596 [details] [review]
Bug 32393: Prevent invalid job to block the job queue

I have faced a problem when testing an incorrect version of bug 32370.
The frame sent to the message broker was not a correct JSON encoded
string, and its decoding was obviously failing, exploding the worker
script.
Additionally, as we don't send a ack for this frame, the next pull will
result in processing the same message, and so in the same explosion.
No more messages can be processed!

This patch is logging the error and ack the message to the broker, in
order to not get stuck.

Test plan:
0. Dont' apply this patch
1. Enqueue a bad message
  a. Apply 32370
  b. Comment the following line in Koha::BackgroundJob::enqueue
    $self->set_encoded_json_field( { data => $job_args,    field => 'data' } );
  c. restart_all
  d. Use the batch item modification tool to enqueue a new job
=> Notice the error in the log
=> Note that the status of the job is "new"
=> Inspect rabbitmq queue:
% rabbitmq-plugins enable rabbitmq_management
% rabbitmqadmin get queue=koha_kohadev-long_tasks
You will notice there is a message in the "long_tasks" queue
2. Enqueue a good message
  a. Remove the change from 1.b
  b. restart_all
  c. Enqueue another job
=> Same error in the log
=> Both jobs are new
=> Inspect rabbitmq, there are 2 messages
3. Apply this patch
4. restart_all
=> Second (good) job is finished
=> rabbitmq long_tasks queue is empty

We cannot mark the first job as done, we have no idea which job it was!

QA: Note that this patch is dealing with another problem, not tested in
this test plan. If an exception is not correctly caught by the ->process
method of the job, we won't crash the worker. The job will be marked as
failed.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Jonathan Druart 2023-01-24 08:23:00 UTC
Created attachment 145597 [details] [review]
Bug 32393: Split into 2 try catch blocks

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 34 Jonathan Druart 2023-01-24 08:23:06 UTC
Created attachment 145598 [details] [review]
Bug 32393: Deal with the DB fallback part
Comment 35 Jonathan Druart 2023-01-24 08:23:25 UTC
(In reply to Marcel de Rooy from comment #31)
> Note that I agree with acking earlier. We do this implicitly here. This
> would obsolete the separate report (32573).

Added the dependency.
Comment 36 Jonathan Druart 2023-01-24 08:23:50 UTC
(In reply to Marcel de Rooy from comment #30)
> For me this statement is also in the scope of this report (but not touched
> yet):
> 
>         while ( my $job = $jobs->next ) {
>             my $args = $job->json->decode($job->data);

Done.
Comment 37 Marcel de Rooy 2023-01-24 12:30:39 UTC
Applying: Bug 32393: Prevent invalid job to block the job queue
Using index info to reconstruct a base tree...
M       misc/background_jobs_worker.pl
Falling back to patching base and 3-way merge...
Auto-merging misc/background_jobs_worker.pl
CONFLICT (content): Merge conflict in misc/background_jobs_worker.pl

++<<<<<<< HEAD
 +        my $body = $frame->body;
 +        my $args = decode_json($body); # TODO Should this be from_json? Check utf8 flag.
 +
 +        # FIXME This means we need to have create the DB entry before
 +        # It could work in a first step, but then we will want to handle job that will be created from the message received
 +        my $job = Koha::BackgroundJobs->find($args->{job_id});
 +
 +        $conn->ack( { frame => $frame } ); # Acknowledge the message was received
 +        process_job( $job, $args );
++=======
+         my $job;
+         try {
+             my $body = $frame->body;
+             my $args = decode_json($body); # TODO Should this be from_json? Check utf8 flag.
+
+             # FIXME This means we need to have create the DB entry before
+             # It could work in a first step, but then we will want to handle job that will be created from the message received
+             $job = Koha::BackgroundJobs->find($args->{job_id});
+
+             process_job( $job, $args );
+         } catch {
+             Koha::Logger->get->warn(sprintf "Job and/or frame not processed - %s", $_);
+         } finally {
+             $job->status('failed')->store if $job && @_;
+             $conn->ack( { frame => $frame } );
+         };
++>>>>>>> Bug 32393: Prevent invalid job to block the job queue

Here ack and process_job are reversing order again..
Comment 38 Jonathan Druart 2023-01-24 12:55:40 UTC
Created attachment 145609 [details] [review]
Bug 32393: Prevent invalid job to block the job queue

I have faced a problem when testing an incorrect version of bug 32370.
The frame sent to the message broker was not a correct JSON encoded
string, and its decoding was obviously failing, exploding the worker
script.
Additionally, as we don't send a ack for this frame, the next pull will
result in processing the same message, and so in the same explosion.
No more messages can be processed!

This patch is logging the error and ack the message to the broker, in
order to not get stuck.

Test plan:
0. Dont' apply this patch
1. Enqueue a bad message
  a. Apply 32370
  b. Comment the following line in Koha::BackgroundJob::enqueue
    $self->set_encoded_json_field( { data => $job_args,    field => 'data' } );
  c. restart_all
  d. Use the batch item modification tool to enqueue a new job
=> Notice the error in the log
=> Note that the status of the job is "new"
=> Inspect rabbitmq queue:
% rabbitmq-plugins enable rabbitmq_management
% rabbitmqadmin get queue=koha_kohadev-long_tasks
You will notice there is a message in the "long_tasks" queue
2. Enqueue a good message
  a. Remove the change from 1.b
  b. restart_all
  c. Enqueue another job
=> Same error in the log
=> Both jobs are new
=> Inspect rabbitmq, there are 2 messages
3. Apply this patch
4. restart_all
=> Second (good) job is finished
=> rabbitmq long_tasks queue is empty

We cannot mark the first job as done, we have no idea which job it was!

QA: Note that this patch is dealing with another problem, not tested in
this test plan. If an exception is not correctly caught by the ->process
method of the job, we won't crash the worker. The job will be marked as
failed.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 39 Jonathan Druart 2023-01-24 12:55:45 UTC
Created attachment 145610 [details] [review]
Bug 32393: Split into 2 try catch blocks

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 40 Jonathan Druart 2023-01-24 12:55:50 UTC
Created attachment 145611 [details] [review]
Bug 32393: Deal with the DB fallback part
Comment 41 Marcel de Rooy 2023-01-24 13:38:30 UTC
QAing
Comment 42 Marcel de Rooy 2023-01-24 13:52:30 UTC
Created attachment 145613 [details] [review]
Bug 32393: Prevent invalid job to block the job queue

I have faced a problem when testing an incorrect version of bug 32370.
The frame sent to the message broker was not a correct JSON encoded
string, and its decoding was obviously failing, exploding the worker
script.
Additionally, as we don't send a ack for this frame, the next pull will
result in processing the same message, and so in the same explosion.
No more messages can be processed!

This patch is logging the error and ack the message to the broker, in
order to not get stuck.

Test plan:
0. Dont' apply this patch
1. Enqueue a bad message
  a. Apply 32370
  b. Comment the following line in Koha::BackgroundJob::enqueue
    $self->set_encoded_json_field( { data => $job_args,    field => 'data' } );
  c. restart_all
  d. Use the batch item modification tool to enqueue a new job
=> Notice the error in the log
=> Note that the status of the job is "new"
=> Inspect rabbitmq queue:
% rabbitmq-plugins enable rabbitmq_management
% rabbitmqadmin get queue=koha_kohadev-long_tasks
You will notice there is a message in the "long_tasks" queue
2. Enqueue a good message
  a. Remove the change from 1.b
  b. restart_all
  c. Enqueue another job
=> Same error in the log
=> Both jobs are new
=> Inspect rabbitmq, there are 2 messages
3. Apply this patch
4. restart_all
=> Second (good) job is finished
=> rabbitmq long_tasks queue is empty

We cannot mark the first job as done, we have no idea which job it was!

QA: Note that this patch is dealing with another problem, not tested in
this test plan. If an exception is not correctly caught by the ->process
method of the job, we won't crash the worker. The job will be marked as
failed.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2023-01-24 13:52:34 UTC
Created attachment 145614 [details] [review]
Bug 32393: Split into 2 try catch blocks

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2023-01-24 13:52:40 UTC
Created attachment 145615 [details] [review]
Bug 32393: Deal with the DB fallback part

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2023-01-24 13:52:45 UTC
Created attachment 145616 [details] [review]
Bug 32393: (QA follow-up) Add explicit undef response in two catch blocks

Do not implicitly depend on last statement returning nothing.
Make it explicit. We want $args to be null here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2023-01-24 13:54:13 UTC
And now we still need the job list to not crash on bad job data ;) I thought that there was a report too?
Comment 47 Jonathan Druart 2023-01-24 14:39:30 UTC
(In reply to Marcel de Rooy from comment #46)
> And now we still need the job list to not crash on bad job data ;) I thought
> that there was a report too?

See bug 32709.
Comment 48 Tomás Cohen Arazi 2023-01-27 18:21:55 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 49 Matt Blenkinsop 2023-01-31 14:12:53 UTC
Nice work everyone!

Pushed to stable for 22.11.x