Skip to content

Adds a cop for rescued exceptions in BBM job classes

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 or BatchedMigrationJob
  • 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

  1. Go to lib/gitlab/background_migration/set_correct_vulnerability_state.rb - or any other batch background migration job class
  2. 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
  1. run bundle exec rubocop lib/gitlab/background_migration
  2. 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
  1. Update the perform again. Add the raise statement:
def perform
  each_sub_batch do |sub_batch|
    sub_batch.update_all(state: DISMISSED_STATE)
  rescue StandardError => error
    puts error

    raise
  end
end
  1. run bundle exec rubocop lib/gitlab/background_migration again.
  2. 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.

Related to #403073 (closed)

Edited by Leonardo da Rosa

Merge request reports