From 4de06435d0c6fc904081b4da82253a2e025613a4 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Tue, 30 Apr 2019 13:36:44 +0100 Subject: [PATCH 1/9] Write test that exposes bug with Fugit gem --- spec/lib/gitlab/ci/cron_parser_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index 2a3f7807fdb..491e3fba9d9 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -174,6 +174,13 @@ describe Gitlab::Ci::CronParser do it { expect(subject).to be_nil } end + + context 'when cron is scheduled to a non existent day' do + let(:cron) { '0 12 31 2 *' } + let(:cron_timezone) { 'UTC' } + + it { expect(subject).to be_nil } + end end describe '#cron_valid?' do From 12712470b354c58860915e38c8a79a75623a3697 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Thu, 2 May 2019 13:29:51 +0100 Subject: [PATCH 2/9] Rescue RuntimeError when "too many loops" are reached by Fugit::Cron --- lib/gitlab/ci/cron_parser.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index 94f4a4e36c9..ae524654b7d 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -13,7 +13,7 @@ module Gitlab def next_time_from(time) @cron_line ||= try_parse_cron(@cron, @cron_timezone) - @cron_line.next_time(time).utc.in_time_zone(Time.zone) if @cron_line.present? + find_next_time(time) if @cron_line.present? end def cron_valid? @@ -49,6 +49,14 @@ module Gitlab def try_parse_cron(cron, cron_timezone) Fugit::Cron.parse("#{cron} #{cron_timezone}") end + + def find_next_time(time) + @cron_line.next_time(time).utc.in_time_zone(Time.zone) + rescue RuntimeError => error + raise error unless error.message =~ /too many loops/ + # Fugit::Cron raises a RuntimeError if :next_time does not find the next schedule + # given an invalid pattern - E.g. try_parse_cron('0 12 31 2 *') + end end end end From 9b3b7a58f0f2acf760c99ae595949b55212aa05b Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Tue, 30 Apr 2019 13:36:44 +0100 Subject: [PATCH 3/9] Write test that exposes bug with Fugit gem --- spec/lib/gitlab/ci/cron_parser_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index 2a3f7807fdb..491e3fba9d9 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -174,6 +174,13 @@ describe Gitlab::Ci::CronParser do it { expect(subject).to be_nil } end + + context 'when cron is scheduled to a non existent day' do + let(:cron) { '0 12 31 2 *' } + let(:cron_timezone) { 'UTC' } + + it { expect(subject).to be_nil } + end end describe '#cron_valid?' do From 6fba1c22dcf8b19b8e6155b6ff3b42acb012be0f Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Thu, 2 May 2019 13:29:51 +0100 Subject: [PATCH 4/9] Rescue RuntimeError when "too many loops" are reached by Fugit::Cron --- lib/gitlab/ci/cron_parser.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index 94f4a4e36c9..ae524654b7d 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -13,7 +13,7 @@ module Gitlab def next_time_from(time) @cron_line ||= try_parse_cron(@cron, @cron_timezone) - @cron_line.next_time(time).utc.in_time_zone(Time.zone) if @cron_line.present? + find_next_time(time) if @cron_line.present? end def cron_valid? @@ -49,6 +49,14 @@ module Gitlab def try_parse_cron(cron, cron_timezone) Fugit::Cron.parse("#{cron} #{cron_timezone}") end + + def find_next_time(time) + @cron_line.next_time(time).utc.in_time_zone(Time.zone) + rescue RuntimeError => error + raise error unless error.message =~ /too many loops/ + # Fugit::Cron raises a RuntimeError if :next_time does not find the next schedule + # given an invalid pattern - E.g. try_parse_cron('0 12 31 2 *') + end end end end From 0edd1e6714385a87f0cacb25497204d89535d35f Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Tue, 30 Apr 2019 13:36:44 +0100 Subject: [PATCH 5/9] Write test that exposes bug with Fugit gem --- spec/lib/gitlab/ci/cron_parser_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index 2a3f7807fdb..491e3fba9d9 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -174,6 +174,13 @@ describe Gitlab::Ci::CronParser do it { expect(subject).to be_nil } end + + context 'when cron is scheduled to a non existent day' do + let(:cron) { '0 12 31 2 *' } + let(:cron_timezone) { 'UTC' } + + it { expect(subject).to be_nil } + end end describe '#cron_valid?' do From 673ea5d2ac50e17a1839d0db83641e6851422f88 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Thu, 2 May 2019 13:29:51 +0100 Subject: [PATCH 6/9] Rescue RuntimeError when "too many loops" occur With this workaround we temporarily prevent an exception from Fugit gem to be raised in Gitlab::Ci::CronParser --- lib/gitlab/ci/cron_parser.rb | 10 +++++++++- spec/lib/gitlab/ci/cron_parser_spec.rb | 7 +++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index 94f4a4e36c9..ae524654b7d 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -13,7 +13,7 @@ module Gitlab def next_time_from(time) @cron_line ||= try_parse_cron(@cron, @cron_timezone) - @cron_line.next_time(time).utc.in_time_zone(Time.zone) if @cron_line.present? + find_next_time(time) if @cron_line.present? end def cron_valid? @@ -49,6 +49,14 @@ module Gitlab def try_parse_cron(cron, cron_timezone) Fugit::Cron.parse("#{cron} #{cron_timezone}") end + + def find_next_time(time) + @cron_line.next_time(time).utc.in_time_zone(Time.zone) + rescue RuntimeError => error + raise error unless error.message =~ /too many loops/ + # Fugit::Cron raises a RuntimeError if :next_time does not find the next schedule + # given an invalid pattern - E.g. try_parse_cron('0 12 31 2 *') + end end end end diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index 491e3fba9d9..a228334d53e 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -181,6 +181,13 @@ describe Gitlab::Ci::CronParser do it { expect(subject).to be_nil } end + + context 'when cron is scheduled to a non existent day' do + let(:cron) { '0 12 31 2 *' } + let(:cron_timezone) { 'UTC' } + + it { expect(subject).to be_nil } + end end describe '#cron_valid?' do From c75b8ad4dadfa8f9676e0339e06408af3281f512 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Tue, 7 May 2019 07:52:53 +0100 Subject: [PATCH 7/9] Remove workaround by upgrading Fugit gem Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/58241 * Upgrade Fugit gem to 1.2.1 which recognizes cron expressions for invalid days * Reverted previously implemented workaround * Leave test case which previously exposed the bug --- Gemfile | 2 +- Gemfile.lock | 8 ++++---- lib/gitlab/ci/cron_parser.rb | 10 +--------- spec/lib/gitlab/ci/cron_parser_spec.rb | 7 ------- 4 files changed, 6 insertions(+), 21 deletions(-) diff --git a/Gemfile b/Gemfile index 1282ff0e20d..a21f9f19458 100644 --- a/Gemfile +++ b/Gemfile @@ -166,7 +166,7 @@ gem 'redis-namespace', '~> 1.6.0' gem 'gitlab-sidekiq-fetcher', '~> 0.4.0', require: 'sidekiq-reliable-fetch' # Cron Parser -gem 'fugit', '~> 1.1' +gem 'fugit', '~> 1.2.1' # HTTP requests gem 'httparty', '~> 0.16.4' diff --git a/Gemfile.lock b/Gemfile.lock index 9b1a036030a..9522f8fa9e0 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -190,7 +190,7 @@ GEM equalizer (0.0.11) erubi (1.8.0) escape_utils (1.2.1) - et-orbi (1.1.7) + et-orbi (1.2.1) tzinfo eventmachine (1.2.7) excon (0.62.0) @@ -264,8 +264,8 @@ GEM foreman (0.84.0) thor (~> 0.19.1) formatador (0.2.5) - fugit (1.1.9) - et-orbi (~> 1.1, >= 1.1.7) + fugit (1.2.1) + et-orbi (~> 1.1, >= 1.1.8) raabro (~> 1.1) fuubar (2.2.0) rspec-core (~> 3.0) @@ -1056,7 +1056,7 @@ DEPENDENCIES fog-rackspace (~> 0.1.1) font-awesome-rails (~> 4.7) foreman (~> 0.84.0) - fugit (~> 1.1) + fugit (~> 1.2.1) fuubar (~> 2.2.0) gemojione (~> 3.3) gettext (~> 3.2.2) diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index ae524654b7d..94f4a4e36c9 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -13,7 +13,7 @@ module Gitlab def next_time_from(time) @cron_line ||= try_parse_cron(@cron, @cron_timezone) - find_next_time(time) if @cron_line.present? + @cron_line.next_time(time).utc.in_time_zone(Time.zone) if @cron_line.present? end def cron_valid? @@ -49,14 +49,6 @@ module Gitlab def try_parse_cron(cron, cron_timezone) Fugit::Cron.parse("#{cron} #{cron_timezone}") end - - def find_next_time(time) - @cron_line.next_time(time).utc.in_time_zone(Time.zone) - rescue RuntimeError => error - raise error unless error.message =~ /too many loops/ - # Fugit::Cron raises a RuntimeError if :next_time does not find the next schedule - # given an invalid pattern - E.g. try_parse_cron('0 12 31 2 *') - end end end end diff --git a/spec/lib/gitlab/ci/cron_parser_spec.rb b/spec/lib/gitlab/ci/cron_parser_spec.rb index a228334d53e..491e3fba9d9 100644 --- a/spec/lib/gitlab/ci/cron_parser_spec.rb +++ b/spec/lib/gitlab/ci/cron_parser_spec.rb @@ -181,13 +181,6 @@ describe Gitlab::Ci::CronParser do it { expect(subject).to be_nil } end - - context 'when cron is scheduled to a non existent day' do - let(:cron) { '0 12 31 2 *' } - let(:cron_timezone) { 'UTC' } - - it { expect(subject).to be_nil } - end end describe '#cron_valid?' do From 0eac37efea8206981749f64c3aae668ce95f1926 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Tue, 7 May 2019 08:14:34 +0100 Subject: [PATCH 8/9] Remove temporary workaround rescuing RuntimeError --- lib/gitlab/ci/cron_parser.rb | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/lib/gitlab/ci/cron_parser.rb b/lib/gitlab/ci/cron_parser.rb index ae524654b7d..94f4a4e36c9 100644 --- a/lib/gitlab/ci/cron_parser.rb +++ b/lib/gitlab/ci/cron_parser.rb @@ -13,7 +13,7 @@ module Gitlab def next_time_from(time) @cron_line ||= try_parse_cron(@cron, @cron_timezone) - find_next_time(time) if @cron_line.present? + @cron_line.next_time(time).utc.in_time_zone(Time.zone) if @cron_line.present? end def cron_valid? @@ -49,14 +49,6 @@ module Gitlab def try_parse_cron(cron, cron_timezone) Fugit::Cron.parse("#{cron} #{cron_timezone}") end - - def find_next_time(time) - @cron_line.next_time(time).utc.in_time_zone(Time.zone) - rescue RuntimeError => error - raise error unless error.message =~ /too many loops/ - # Fugit::Cron raises a RuntimeError if :next_time does not find the next schedule - # given an invalid pattern - E.g. try_parse_cron('0 12 31 2 *') - end end end end From ccde391648ce9aece94a3b63ccccbd7df67ce693 Mon Sep 17 00:00:00 2001 From: Fabio Pitino Date: Tue, 7 May 2019 08:23:48 +0100 Subject: [PATCH 9/9] add changelog --- changelogs/unreleased/fix-too-many-loops-cron-error.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/fix-too-many-loops-cron-error.yml diff --git a/changelogs/unreleased/fix-too-many-loops-cron-error.yml b/changelogs/unreleased/fix-too-many-loops-cron-error.yml new file mode 100644 index 00000000000..a9b5b761439 --- /dev/null +++ b/changelogs/unreleased/fix-too-many-loops-cron-error.yml @@ -0,0 +1,5 @@ +--- +title: Fix "too many loops" error by handling gracefully cron schedules for non existent days +merge_request: 28002 +author: +type: fixed