From 18372c5b7c4073f760de8e55a976b3fe2a61e9e8 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 2 Dec 2022 14:23:38 +0100 Subject: [PATCH] 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. --- misc/background_jobs_worker.pl | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/misc/background_jobs_worker.pl b/misc/background_jobs_worker.pl index ee5e7293bd2..0a068575ea6 100755 --- a/misc/background_jobs_worker.pl +++ b/misc/background_jobs_worker.pl @@ -49,10 +49,11 @@ The different values available are: use Modern::Perl; use JSON qw( decode_json ); -use Try::Tiny qw( catch try ); +use Try::Tiny; use Pod::Usage; use Getopt::Long; +use Koha::Logger; use Koha::BackgroundJobs; my ( $help, @queues ); @@ -89,15 +90,22 @@ while (1) { next; # will reconnect automatically } - 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}); - - process_job( $job, $args ); - $conn->ack( { frame => $frame } ); # FIXME depending on success? + 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 } ); + }; } else { my $jobs = Koha::BackgroundJobs->search({ status => 'new', queue => \@queues }); -- 2.25.1