Adds a cop for rescued exceptions in BBM job classes
requested to merge 403073-rescuing-exceptions-inside-bbm-job-class-causing-job-to-not-fail-properly-2 into master
What does this MR do and why?
Adds a new cop for Batched Background Migration Jobs.
Matches:
- If is a sub-class of:
Gitlab::BackgroundMigration::BatchedMigrationJob
orBatchedMigrationJob
- If rescue statements inside methods:
rescue => e
,rescue StandardError => e
,JSON::ParserError, ActiveRecord::StatementTimeout => error
Bad:
module Gitlab
module BackgroundMigration
class MyClass < ::Gitlab::BackgroundMigration::BatchedMigrationJob
def perform
execute
rescue => error
logger.error(message: error.message, class: self.class.name)
end
end
end
end
Bad:
module Gitlab
module BackgroundMigration
class MyClass < ::Gitlab::BackgroundMigration::BatchedMigrationJob
def perform
execute
rescue StandardError => error
logger.error(message: error.message, class: self.class.name)
end
end
end
end
Good:
module Gitlab
module BackgroundMigration
class MyClass < ::Gitlab::BackgroundMigration::BatchedMigrationJob
def perform
execute
rescue StandardError => error
logger.error(message: error.message, class: self.class.name)
raise
end
end
end
end
Example: https://gitlab.com/gitlab-org/gitlab/-/jobs/4473111652
How to set up and validate locally
- Go to
lib/gitlab/background_migration/set_correct_vulnerability_state.rb
- or any other batch background migration job class - Update the
perform
method to:
def perform
each_sub_batch do |sub_batch|
sub_batch.update_all(state: DISMISSED_STATE)
rescue StandardError => error
puts error
end
end
- run
bundle exec rubocop lib/gitlab/background_migration
- The file must be in the offenses report
Inspecting 151 files
................................................................................................................................................C......
Offenses:
lib/gitlab/background_migration/set_correct_vulnerability_state.rb:16:9: C: BackgroundMigration/AvoidSilentRescueExceptions: Avoid rescuing exceptions inside job classes. Jobs can be erroneously marked as successful, even when they fail. Consider re-raising the error if rescuing this exception is needed.
rescue StandardError => error
^^^^^^^^^^^^^^^^^^^^
151 files inspected, 1 offense detected
- Update the
perform
again. Add theraise
statement:
def perform
each_sub_batch do |sub_batch|
sub_batch.update_all(state: DISMISSED_STATE)
rescue StandardError => error
puts error
raise
end
end
- run
bundle exec rubocop lib/gitlab/background_migration
again. - The file must be left out of the offenses report
Inspecting 151 files
.......................................................................................................................................................
151 files inspected, no offenses detected
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #403073 (closed)
Edited by Leonardo da Rosa