Merge pull request #1470 from Shopify/configurable-presence-check-timeout

Make PRESENCE_CHECK_TIMEOUT configurable via env var

Author: DudeRandom21

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Unreleased
 
+# 0.45.1
+* Make `PRESENCE_CHECK_TIMEOUT` configurable via environment variable
+
 # 0.45.0
 * Shipit-engine now requires application to be at least on Rails 8.1.1.
 * Remove dependency on SaasC that is lo longer maintained. A vanilla CSS file now ships  with shipit-engine.

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    shipit-engine (0.45.0)
+    shipit-engine (0.45.1)
       active_model_serializers (~> 0.9.3)
       ansi_stream (~> 0.0.6)
       autoprefixer-rails (~> 6.4.1)
@@ -32,71 +32,71 @@ PATH
 GEM
   remote: https://rubygems.org/
   specs:
-    action_text-trix (2.1.15)
+    action_text-trix (2.1.17)
       railties
-    actioncable (8.1.1)
-      actionpack (= 8.1.1)
-      activesupport (= 8.1.1)
+    actioncable (8.1.2)
+      actionpack (= 8.1.2)
+      activesupport (= 8.1.2)
       nio4r (~> 2.0)
       websocket-driver (>= 0.6.1)
       zeitwerk (~> 2.6)
-    actionmailbox (8.1.1)
-      actionpack (= 8.1.1)
-      activejob (= 8.1.1)
-      activerecord (= 8.1.1)
-      activestorage (= 8.1.1)
-      activesupport (= 8.1.1)
+    actionmailbox (8.1.2)
+      actionpack (= 8.1.2)
+      activejob (= 8.1.2)
+      activerecord (= 8.1.2)
+      activestorage (= 8.1.2)
+      activesupport (= 8.1.2)
       mail (>= 2.8.0)
-    actionmailer (8.1.1)
-      actionpack (= 8.1.1)
-      actionview (= 8.1.1)
-      activejob (= 8.1.1)
-      activesupport (= 8.1.1)
+    actionmailer (8.1.2)
+      actionpack (= 8.1.2)
+      actionview (= 8.1.2)
+      activejob (= 8.1.2)
+      activesupport (= 8.1.2)
       mail (>= 2.8.0)
       rails-dom-testing (~> 2.2)
-    actionpack (8.1.1)
-      actionview (= 8.1.1)
-      activesupport (= 8.1.1)
+    actionpack (8.1.2)
+      actionview (= 8.1.2)
+      activesupport (= 8.1.2)
       nokogiri (>= 1.8.5)
       rack (>= 2.2.4)
       rack-session (>= 1.0.1)
       rack-test (>= 0.6.3)
       rails-dom-testing (~> 2.2)
       rails-html-sanitizer (~> 1.6)
       useragent (~> 0.16)
-    actiontext (8.1.1)
+    actiontext (8.1.2)
       action_text-trix (~> 2.1.15)
-      actionpack (= 8.1.1)
-      activerecord (= 8.1.1)
-      activestorage (= 8.1.1)
-      activesupport (= 8.1.1)
+      actionpack (= 8.1.2)
+      activerecord (= 8.1.2)
+      activestorage (= 8.1.2)
+      activesupport (= 8.1.2)
       globalid (>= 0.6.0)
       nokogiri (>= 1.8.5)
-    actionview (8.1.1)
-      activesupport (= 8.1.1)
+    actionview (8.1.2)
+      activesupport (= 8.1.2)
       builder (~> 3.1)
       erubi (~> 1.11)
       rails-dom-testing (~> 2.2)
       rails-html-sanitizer (~> 1.6)
     active_model_serializers (0.9.13)
       activemodel (>= 3.2)
       concurrent-ruby (~> 1.0)
-    activejob (8.1.1)
-      activesupport (= 8.1.1)
+    activejob (8.1.2)
+      activesupport (= 8.1.2)
       globalid (>= 0.3.6)
-    activemodel (8.1.1)
-      activesupport (= 8.1.1)
-    activerecord (8.1.1)
-      activemodel (= 8.1.1)
-      activesupport (= 8.1.1)
+    activemodel (8.1.2)
+      activesupport (= 8.1.2)
+    activerecord (8.1.2)
+      activemodel (= 8.1.2)
+      activesupport (= 8.1.2)
       timeout (>= 0.4.0)
-    activestorage (8.1.1)
-      actionpack (= 8.1.1)
-      activejob (= 8.1.1)
-      activerecord (= 8.1.1)
-      activesupport (= 8.1.1)
+    activestorage (8.1.2)
+      actionpack (= 8.1.2)
+      activejob (= 8.1.2)
+      activerecord (= 8.1.2)
+      activesupport (= 8.1.2)
       marcel (~> 1.0)
-    activesupport (8.1.1)
+    activesupport (8.1.2)
       base64
       bigdecimal
       concurrent-ruby (~> 1.0, >= 1.3.1)
@@ -262,8 +262,7 @@ GEM
     parser (3.3.8.0)
       ast (~> 2.4.1)
       racc
-    pg (1.6.3-arm64-darwin)
-    pg (1.6.3-x86_64-linux)
+    pg (1.3.3)
     prism (1.4.0)
     pry (0.14.1)
       coderay (~> 1.1)
@@ -276,28 +275,28 @@ GEM
       rack
       redis (~> 4.0)
     racc (1.8.1)
-    rack (2.2.22)
+    rack (2.2.17)
     rack-session (1.0.2)
       rack (< 3)
     rack-test (2.2.0)
       rack (>= 1.3)
     rackup (1.0.0)
       rack (< 3)
       webrick
-    rails (8.1.1)
-      actioncable (= 8.1.1)
-      actionmailbox (= 8.1.1)
-      actionmailer (= 8.1.1)
-      actionpack (= 8.1.1)
-      actiontext (= 8.1.1)
-      actionview (= 8.1.1)
-      activejob (= 8.1.1)
-      activemodel (= 8.1.1)
-      activerecord (= 8.1.1)
-      activestorage (= 8.1.1)
-      activesupport (= 8.1.1)
+    rails (8.1.2)
+      actioncable (= 8.1.2)
+      actionmailbox (= 8.1.2)
+      actionmailer (= 8.1.2)
+      actionpack (= 8.1.2)
+      actiontext (= 8.1.2)
+      actionview (= 8.1.2)
+      activejob (= 8.1.2)
+      activemodel (= 8.1.2)
+      activerecord (= 8.1.2)
+      activestorage (= 8.1.2)
+      activesupport (= 8.1.2)
       bundler (>= 1.15.0)
-      railties (= 8.1.1)
+      railties (= 8.1.2)
     rails-dom-testing (2.3.0)
       activesupport (>= 5.0.0)
       minitest
@@ -312,9 +311,9 @@ GEM
       actionview (> 3.1)
       activesupport (> 3.1)
       railties (> 3.1)
-    railties (8.1.1)
-      actionpack (= 8.1.1)
-      activesupport (= 8.1.1)
+    railties (8.1.2)
+      actionpack (= 8.1.2)
+      activesupport (= 8.1.2)
       irb (~> 1.13)
       rackup (>= 1.0.0)
       rake (>= 12.2)

app/models/shipit/task.rb

@@ -10,7 +10,6 @@ def message
       end
     end
 
-    PRESENCE_CHECK_TIMEOUT = 30
     ACTIVE_STATUSES = %w[pending running aborting].freeze
     COMPLETED_STATUSES = %w[success flapping faulty validating].freeze
     UNSUCCESSFUL_STATUSES = %w[error failed aborted flapping timedout faulty].freeze
@@ -327,15 +326,15 @@ def finished?
     end
 
     def ping
-      Shipit.redis.set(status_key, 'alive', ex: PRESENCE_CHECK_TIMEOUT)
+      Shipit.redis.set(status_key, 'alive', ex: Shipit.presence_check_timeout)
     end
 
     def alive?
       Shipit.redis.get(status_key) == 'alive'
     end
 
     def report_dead!
-      write("ERROR: Background job hasn't reported back in #{PRESENCE_CHECK_TIMEOUT} seconds.")
+      write("ERROR: Background job hasn't reported back in #{Shipit.presence_check_timeout} seconds.")
       error!
     end
 

docs/setup.md

@@ -171,6 +171,14 @@ production:
   git_progress_output: true
 ```
 
+### Environment Variables
+
+**`SHIPIT_PRESENCE_CHECK_TIMEOUT`** is the duration (in seconds) after which a deploy task is considered dead if the background job hasn't reported back. Defaults to `30`. Increase this if deploy tasks are being reaped during long-running steps like `bundle install` with native extension compilation.
+
+```bash
+export SHIPIT_PRESENCE_CHECK_TIMEOUT=120
+```
+
 ### Using Multiple Github Applications
 
 A Github application can only authenticate to the Github organization it's installed in. If you want to deploy code from multiple Github organizations the `github` section of your `config/secrets.yml` will need to be formatted differently. The top-level keys should be the name of each Github organization, and the following sub-keys are the Github app details for that particular organization.

lib/shipit.rb

@@ -91,6 +91,14 @@ def enable_samesite_middleware?
     ENV['SHIPIT_ENABLE_SAMESITE_NONE'].present?
   end
 
+  def presence_check_timeout
+    raw = ENV.fetch('SHIPIT_PRESENCE_CHECK_TIMEOUT', '30')
+    timeout = Integer(raw)
+    raise ArgumentError, "SHIPIT_PRESENCE_CHECK_TIMEOUT must be a positive integer, got #{raw.inspect}" if timeout <= 0
+
+    timeout
+  end
+
   def app_name
     @app_name ||= secrets.app_name || Rails.application.class.name.split(':').first || 'Shipit'
   end

lib/shipit/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module Shipit
-  VERSION = '0.45.0'
+  VERSION = '0.45.1'
 end

test/unit/shipit_test.rb

@@ -30,6 +30,40 @@ class ShipitTest < ActiveSupport::TestCase
       assert_equal(['shopify/developers'], Shipit.github_teams.map(&:handle))
     end
 
+    test ".presence_check_timeout defaults to 30" do
+      assert_equal 30, Shipit.presence_check_timeout
+    end
+
+    test ".presence_check_timeout returns the configured value from ENV" do
+      ENV['SHIPIT_PRESENCE_CHECK_TIMEOUT'] = '120'
+      assert_equal 120, Shipit.presence_check_timeout
+    ensure
+      ENV.delete('SHIPIT_PRESENCE_CHECK_TIMEOUT')
+    end
+
+    test ".presence_check_timeout raises on non-numeric string" do
+      ENV['SHIPIT_PRESENCE_CHECK_TIMEOUT'] = 'abc'
+      assert_raises(ArgumentError) { Shipit.presence_check_timeout }
+    ensure
+      ENV.delete('SHIPIT_PRESENCE_CHECK_TIMEOUT')
+    end
+
+    test ".presence_check_timeout raises on zero" do
+      ENV['SHIPIT_PRESENCE_CHECK_TIMEOUT'] = '0'
+      error = assert_raises(ArgumentError) { Shipit.presence_check_timeout }
+      assert_match(/must be a positive integer/, error.message)
+    ensure
+      ENV.delete('SHIPIT_PRESENCE_CHECK_TIMEOUT')
+    end
+
+    test ".presence_check_timeout raises on negative value" do
+      ENV['SHIPIT_PRESENCE_CHECK_TIMEOUT'] = '-5'
+      error = assert_raises(ArgumentError) { Shipit.presence_check_timeout }
+      assert_match(/must be a positive integer/, error.message)
+    ensure
+      ENV.delete('SHIPIT_PRESENCE_CHECK_TIMEOUT')
+    end
+
     class RedisTest < self
       setup do
         @client = mock(:client)