It should be logging about the error on the `catch` block. I think it should be done on top of bug 36900, and also using Koha::Logger (i.e. both).
We are actually not supposed to hit that. If the job is correctly written it will deal with the different situations and log when need to be logged. But yes, we never know and we should at least log the exception to the log file.
(In reply to Jonathan Druart from comment #1) > We are actually not supposed to hit that. > > If the job is correctly written it will deal with the different situations > and log when need to be logged. > > But yes, we never know and we should at least log the exception to the log > file. Yes. All jobs that are core to Koha implement a try/catch block, but ultimately call ` $self->finish( $data );` which can fail! Something to revisit, as the job might have been successful but the worker marking it as failed because of DB connection issues.
Created attachment 166969 [details] [review] [DO NOT PUSH] Bug 36901: Simulate failure for testing purposes
Created attachment 166970 [details] [review] Bug 36901: Add logging for uncaught exceptions in background job classes This patch adds logging of unhandled exceptions that could occur. This is happening on busy production sites right now. This is also useful for plugin jobs that might not be 100% following the guidelines and would benefit from this. But as the [DO NOT PUSH] patch highlights, this is something we really want to have on our current codebase, as a database connection drop might make us reach that `catch` block we are adding logging to on this patch. To test: 1. Apply the [DO NOT PUSH] patch 2. Run: $ ktd --shell k$ restart_all ; tail -f /var/log/koha/kohadev/worker*.log 3. Pick a valid barcode on the staff UI 4. Use the 'Batch delete items' tool in the cataloguing section 5. Start the job for deleting the item => FAIL: The item got deleted, but the job marked as failed and no logs about the reasons 6. Apply this patch and repeat 2-5 => SUCCESS: Same scenario but there's a log with the error message 7. Sign off :-D
Created attachment 166992 [details] [review] Bug 36901: Simulate failure for testing purposes Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166993 [details] [review] Bug 36901: Add logging for uncaught exceptions in background job classes This patch adds logging of unhandled exceptions that could occur. This is happening on busy production sites right now. This is also useful for plugin jobs that might not be 100% following the guidelines and would benefit from this. But as the [DO NOT PUSH] patch highlights, this is something we really want to have on our current codebase, as a database connection drop might make us reach that `catch` block we are adding logging to on this patch. To test: 1. Apply the [DO NOT PUSH] patch 2. Run: $ ktd --shell k$ restart_all ; tail -f /var/log/koha/kohadev/worker*.log 3. Pick a valid barcode on the staff UI 4. Use the 'Batch delete items' tool in the cataloguing section 5. Start the job for deleting the item => FAIL: The item got deleted, but the job marked as failed and no logs about the reasons 6. Apply this patch and repeat 2-5 => SUCCESS: Same scenario but there's a log with the error message 7. Sign off :-D Signed-off-by: David Nind <david@davidnind.com>
Created attachment 168245 [details] [review] Bug 36901: Add logging for uncaught exceptions in background job classes This patch adds logging of unhandled exceptions that could occur. This is happening on busy production sites right now. This is also useful for plugin jobs that might not be 100% following the guidelines and would benefit from this. But as the [DO NOT PUSH] patch highlights, this is something we really want to have on our current codebase, as a database connection drop might make us reach that `catch` block we are adding logging to on this patch. To test: 1. Apply the [DO NOT PUSH] patch 2. Run: $ ktd --shell k$ restart_all ; tail -f /var/log/koha/kohadev/worker*.log 3. Pick a valid barcode on the staff UI 4. Use the 'Batch delete items' tool in the cataloguing section 5. Start the job for deleting the item => FAIL: The item got deleted, but the job marked as failed and no logs about the reasons 6. Apply this patch and repeat 2-5 => SUCCESS: Same scenario but there's a log with the error message 7. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Nice improvement.. I'd go so far as to say it's a minor bugfix to aid developers lives. Passing QA
Pushed for 24.11! Well done everyone, thank you!
Backported to 24.05.x for upcoming 24.05.06
Pushed to 23.11.x for 23.11.11