Skip to content

Rescue too many loops cron error

Fabio Pitino requested to merge fix-too-many-loops-cron-error into master

What does this MR do?

This is a workaround which fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/58241.

I'm not entirely happy with this fix because there are many tradeoffs:

  • Fugit::Chron#next_time raises a RuntimeError when too many loops are executed to find the next schedule. Ideally a more specific error class would be safer. So, I resorted into comparing the error.message to avoid rescuing any RuntimeError, which is far from an ideal implementation.
  • The new test case adds about 6 seconds to the overall test suite because we have to run next_time in order to raise the error. This is my primary concern.
BEFORE: Finished in 2.37 seconds (files took 0.904 seconds to load)
AFTER:  Finished in 8.14 seconds (files took 0.88169 seconds to load)
  • Submitting a PR for Fugit would take too much time given the complexity of the bug. I've created an issue anyway so we can follow up with a cleaner solution: https://github.com/floraison/fugit/issues/21
    UPDATE: Looks like we may take advantage of the proposed solution that is being implemented Could the parse method return nil directly when invalid combination of month-day is detected? - This will return nil immediately and we won't add 6 secs to the test suite.

Does this MR meet the acceptance criteria?

Conformity

Performance and testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Robert Speicher

Merge request reports