FIX: Falling back to primary PG server not reliable on Rails 7.1

This commit drops the reliance on `ActiveRecord::ConnectionAdapters::PostgreSQLAdapter#active?` in favor or using `ActiveRecord::ConnectionAdatpers::PostgreSQLAdatper#execute` to check if the app can connect to the primary PG server. This is because `ActiveRecord::ConnectionAdapters::PostgreSQLAdapter#active?` was changed in Rails 7.1 to return `false` if the connection has not be used to do something meaningful. Ref: rails/rails@8551e64. Due to this change, our fallback to primary checker will keep thinking that the primary PG server is down. Note that before Rails 7.1, `ActiveRecord::ConnectionAdapters::PostgreSQLAdapter#active?` will always execute a fake query to check if the query can be executed. Instead of relying on ActiveRecord's internal API, we will instead rely on `ActiveRecord::ConnectionAdapters::PostgreSQLAdapter#execute` to execute a blank query as our means of verifying if the PG server is up and ready to receive connections. This commit also updates the ActiveRecord tests to be more reliable by reducing the Unicorn worker processes to 1 so that we don't have to rely on flooding the app with requests to get all the Unicorn processes to failover/fallback. Co-authored-by: Loïc Guitaut <loic@discourse.org>
discourse · Jul 3, 2024 · af25fef · af25fef
1 parent b0dd7e7
commit af25fef
Show file tree

Hide file tree

Showing 20 changed files with 747 additions and 92 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -16,7 +16,7 @@ jobs:
       - name: Setup ruby
         uses: ruby/setup-ruby@v1
         with:
-          ruby-version: '3.2'
+          ruby-version: "3.2"
           bundler-cache: true
 
       - name: Rubocop
@@ -29,13 +29,13 @@ jobs:
           bundle exec stree check Gemfile rails_failover.gemspec $(git ls-files '*.rb')
 
   redis:
-    name: 'Redis (Ruby ${{ matrix.ruby }})'
+    name: "Redis (Ruby ${{ matrix.ruby }})"
     runs-on: ubuntu-latest
 
     strategy:
       fail-fast: false
       matrix:
-        ruby: ['3.4', '3.3', '3.2', '3.1']
+        ruby: ["3.4", "3.3", "3.2", "3.1"]
 
     steps:
       - uses: actions/checkout@v3
@@ -54,21 +54,21 @@ jobs:
 
   active_record:
     runs-on: ubuntu-latest
-    name: 'ActiveRecord ~>${{ matrix.rails }} (Ruby ${{ matrix.ruby }})'
+    name: "ActiveRecord ~>${{ matrix.rails }} (Ruby ${{ matrix.ruby }})"
 
     strategy:
       fail-fast: false
       matrix:
-        ruby: ['3.4', '3.3', '3.2', '3.1']
-        rails: ['7.1.0', '7.0.0']
+        ruby: ["3.4", "3.3", "3.2", "3.1"]
+        rails: ["7.1.0", "7.0.0"]
         include:
-          - ruby: '3.2'
-            rails: '6.1.0'
+          - ruby: "3.2"
+            rails: "6.1.0"
         exclude:
-          - ruby: '3.4'
-            rails: '7.0.0'
-          - ruby: '3.3'
-            rails: '7.0.0'
+          - ruby: "3.4"
+            rails: "7.0.0"
+          - ruby: "3.3"
+            rails: "7.0.0"
 
     steps:
       - uses: actions/checkout@v3
@@ -84,7 +84,6 @@ jobs:
       - name: Setup postgres
         run: |
           make setup_pg
-          make start_pg
 
       - name: ActiveRecord specs
         env:

diff --git a/.gitignore b/.gitignore
@@ -16,4 +16,3 @@
 *.rdb
 
 *.gem
-Gemfile.lock
diff --git a/.rspec b/.rspec
@@ -1,3 +1,4 @@
 --color
 --require spec_helper
 --format documentation
+--order random
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,7 +5,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [2.1.1] - 2024-07-02
+
+- FIX: Falling back to primary PG server not reliable on Rails 7.1
+
 ## [2.1.0] - 2024-05-29
+
 - DEV: Update dependencies to officially support Rails 7.1
 
 ## [2.0.1] - 2023-05-30

diff --git a/Gemfile.lock b/Gemfile.lock
@@ -0,0 +1,205 @@
+PATH
+  remote: .
+  specs:
+    rails_failover (2.1.1)
+      activerecord (>= 6.1, < 8.0)
+      concurrent-ruby
+      railties (>= 6.1, < 8.0)
+
+GEM
+  remote: https://rubygems.org/
+  specs:
+    actionpack (7.1.3.4)
+      actionview (= 7.1.3.4)
+      activesupport (= 7.1.3.4)
+      nokogiri (>= 1.8.5)
+      racc
+      rack (>= 2.2.4)
+      rack-session (>= 1.0.1)
+      rack-test (>= 0.6.3)
+      rails-dom-testing (~> 2.2)
+      rails-html-sanitizer (~> 1.6)
+    actionview (7.1.3.4)
+      activesupport (= 7.1.3.4)
+      builder (~> 3.1)
+      erubi (~> 1.11)
+      rails-dom-testing (~> 2.2)
+      rails-html-sanitizer (~> 1.6)
+    activemodel (7.1.3.4)
+      activesupport (= 7.1.3.4)
+    activerecord (7.1.3.4)
+      activemodel (= 7.1.3.4)
+      activesupport (= 7.1.3.4)
+      timeout (>= 0.4.0)
+    activesupport (7.1.3.4)
+      base64
+      bigdecimal
+      concurrent-ruby (~> 1.0, >= 1.0.2)
+      connection_pool (>= 2.2.5)
+      drb
+      i18n (>= 1.6, < 2)
+      minitest (>= 5.1)
+      mutex_m
+      tzinfo (~> 2.0)
+    ast (2.4.2)
+    base64 (0.2.0)
+    bigdecimal (3.1.8)
+    builder (3.3.0)
+    byebug (11.1.3)
+    concurrent-ruby (1.3.3)
+    connection_pool (2.4.1)
+    crass (1.0.6)
+    diff-lcs (1.5.1)
+    drb (2.2.1)
+    erubi (1.13.0)
+    i18n (1.14.5)
+      concurrent-ruby (~> 1.0)
+    io-console (0.7.2)
+    irb (1.13.2)
+      rdoc (>= 4.0.0)
+      reline (>= 0.4.2)
+    json (2.7.2)
+    language_server-protocol (3.17.0.3)
+    loofah (2.22.0)
+      crass (~> 1.0.2)
+      nokogiri (>= 1.12.0)
+    minitest (5.24.1)
+    mutex_m (0.2.0)
+    nokogiri (1.16.6-aarch64-linux)
+      racc (~> 1.4)
+    nokogiri (1.16.6-arm-linux)
+      racc (~> 1.4)
+    nokogiri (1.16.6-arm64-darwin)
+      racc (~> 1.4)
+    nokogiri (1.16.6-x86-linux)
+      racc (~> 1.4)
+    nokogiri (1.16.6-x86_64-darwin)
+      racc (~> 1.4)
+    nokogiri (1.16.6-x86_64-linux)
+      racc (~> 1.4)
+    parallel (1.25.1)
+    parser (3.3.3.0)
+      ast (~> 2.4.1)
+      racc
+    pg (1.5.6)
+    prettier_print (1.2.1)
+    psych (5.1.2)
+      stringio
+    racc (1.8.0)
+    rack (3.1.6)
+    rack-session (2.0.0)
+      rack (>= 3.0.0)
+    rack-test (2.1.0)
+      rack (>= 1.3)
+    rackup (2.1.0)
+      rack (>= 3)
+      webrick (~> 1.8)
+    rails-dom-testing (2.2.0)
+      activesupport (>= 5.0.0)
+      minitest
+      nokogiri (>= 1.6)
+    rails-html-sanitizer (1.6.0)
+      loofah (~> 2.21)
+      nokogiri (~> 1.14)
+    railties (7.1.3.4)
+      actionpack (= 7.1.3.4)
+      activesupport (= 7.1.3.4)
+      irb
+      rackup (>= 1.0.0)
+      rake (>= 12.2)
+      thor (~> 1.0, >= 1.2.2)
+      zeitwerk (~> 2.6)
+    rainbow (3.1.1)
+    rake (12.3.3)
+    rdoc (6.7.0)
+      psych (>= 4.0.0)
+    redis (4.8.1)
+    regexp_parser (2.9.2)
+    reline (0.5.9)
+      io-console (~> 0.5)
+    rexml (3.3.1)
+      strscan
+    rspec (3.13.0)
+      rspec-core (~> 3.13.0)
+      rspec-expectations (~> 3.13.0)
+      rspec-mocks (~> 3.13.0)
+    rspec-core (3.13.0)
+      rspec-support (~> 3.13.0)
+    rspec-expectations (3.13.1)
+      diff-lcs (>= 1.2.0, < 2.0)
+      rspec-support (~> 3.13.0)
+    rspec-mocks (3.13.1)
+      diff-lcs (>= 1.2.0, < 2.0)
+      rspec-support (~> 3.13.0)
+    rspec-support (3.13.1)
+    rubocop (1.64.1)
+      json (~> 2.3)
+      language_server-protocol (>= 3.17.0)
+      parallel (~> 1.10)
+      parser (>= 3.3.0.2)
+      rainbow (>= 2.2.2, < 4.0)
+      regexp_parser (>= 1.8, < 3.0)
+      rexml (>= 3.2.5, < 4.0)
+      rubocop-ast (>= 1.31.1, < 2.0)
+      ruby-progressbar (~> 1.7)
+      unicode-display_width (>= 2.4.0, < 3.0)
+    rubocop-ast (1.31.3)
+      parser (>= 3.3.1.0)
+    rubocop-capybara (2.21.0)
+      rubocop (~> 1.41)
+    rubocop-discourse (3.8.1)
+      activesupport (>= 6.1)
+      rubocop (>= 1.59.0)
+      rubocop-capybara (>= 2.0.0)
+      rubocop-factory_bot (>= 2.0.0)
+      rubocop-rails (>= 2.25.0)
+      rubocop-rspec (>= 3.0.1)
+      rubocop-rspec_rails (>= 2.30.0)
+    rubocop-factory_bot (2.26.1)
+      rubocop (~> 1.61)
+    rubocop-rails (2.25.1)
+      activesupport (>= 4.2.0)
+      rack (>= 1.1)
+      rubocop (>= 1.33.0, < 2.0)
+      rubocop-ast (>= 1.31.1, < 2.0)
+    rubocop-rspec (3.0.1)
+      rubocop (~> 1.61)
+    rubocop-rspec_rails (2.30.0)
+      rubocop (~> 1.61)
+      rubocop-rspec (~> 3, >= 3.0.1)
+    ruby-progressbar (1.13.0)
+    stringio (3.1.1)
+    strscan (3.1.0)
+    syntax_tree (6.2.0)
+      prettier_print (>= 1.2.0)
+    syntax_tree-disable_ternary (1.0.0)
+    thor (1.3.1)
+    timeout (0.4.1)
+    tzinfo (2.0.6)
+      concurrent-ruby (~> 1.0)
+    unicode-display_width (2.5.0)
+    webrick (1.8.1)
+    zeitwerk (2.6.16)
+
+PLATFORMS
+  aarch64-linux
+  arm-linux
+  arm64-darwin
+  x86-linux
+  x86_64-darwin
+  x86_64-linux
+
+DEPENDENCIES
+  byebug
+  pg (~> 1.2)
+  rack
+  rails_failover!
+  rake (~> 12.0)
+  redis (~> 4.1)
+  rspec (~> 3.0)
+  rubocop-discourse
+  syntax_tree
+  syntax_tree-disable_ternary
+
+BUNDLED WITH
+   2.5.11
diff --git a/README.md b/README.md
@@ -107,7 +107,6 @@ To install this gem onto your local machine, run `bundle exec rake install`. To
 The ActiveRecord failover tests are run against a dummy Rails server. Run the following commands to run the test:
 
 1. `make setup_pg`
-2. `make start_pg`
 3. `bin/rspec active_record`. You may also run the tests with more unicorn workers by adding the `UNICORN_WORKERS` env variable.
 
 #### Redis

diff --git a/lib/rails_failover/active_record/handler.rb b/lib/rails_failover/active_record/handler.rb
@@ -64,7 +64,8 @@ def initiate_fallback_to_primary
                 spec_name,
                 role: handler_key,
               )
-            connection_active = connection.active?
+
+            connection_active = connection.verify!
           rescue => e
             logger.debug "#{Process.pid} Connection to server for '#{handler_key} #{spec_name}' failed with '#{e.message}'"
           ensure

diff --git a/lib/rails_failover/active_record/railtie.rb b/lib/rails_failover/active_record/railtie.rb
@@ -11,7 +11,7 @@ class Railtie < ::Rails::Railtie
         app.config.active_record_rails_failover = true
         ::ActiveSupport.on_load(:active_record) do
           begin
-            ::ActiveRecord::Base.connection
+            ::ActiveRecord::Base.connection.verify!
           rescue ::ActiveRecord::NoDatabaseError
             # Do nothing since database hasn't been created
           rescue ::PG::Error, ::ActiveRecord::ConnectionNotEstablished

diff --git a/lib/rails_failover/version.rb b/lib/rails_failover/version.rb
@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module RailsFailover
-  VERSION = "2.1.0"
+  VERSION = "2.1.1"
 end
diff --git a/makefile b/makefile
@@ -3,19 +3,7 @@ include redis.mk
 
 all: redis
 
-active_record: teardown_dummy_rails_server setup_dummy_rails_server test_active_record
+active_record: test_active_record
 
 test_active_record:
-	@ACTIVE_RECORD=1 bundle exec rspec --tag type:active_record ${RSPEC_PATH}
-
-setup_dummy_rails_server:
-	@cd spec/support/dummy_app && BUNDLE_GEMFILE=Gemfile bundle install --quiet && BUNDLE_GEMFILE=Gemfile RAILS_ENV=production $(BUNDLER_BIN) exec rails db:create db:migrate db:seed
-
-start_dummy_rails_server:
-	@cd spec/support/dummy_app && BUNDLE_GEMFILE=Gemfile SECRET_KEY_BASE=somekey bundle exec unicorn -c config/unicorn.conf.rb -D -E production
-
-stop_dummy_rails_server:
-	@kill -TERM $(shell cat spec/support/dummy_app/tmp/pids/unicorn.pid)
-
-teardown_dummy_rails_server:
-	@cd spec/support/dummy_app && (! (bundle check > /dev/null 2>&1) || BUNDLE_GEMFILE=Gemfile DISABLE_DATABASE_ENVIRONMENT_CHECK=1 RAILS_ENV=production $(BUNDLER_BIN) exec rails db:drop)
+	@ACTIVE_RECORD=1 bundle exec rspec --tag type:active_record ${RSPEC_PATH}
diff --git a/postgresql.mk b/postgresql.mk
@@ -41,9 +41,6 @@ start_pg_primary:
 start_pg_replica:
 	@$(PG_BIN_DIR)/pg_ctl --silent --log /dev/null -w -D $(PG_REPLICA_DATA_DIR) -o "-p $(PG_REPLICA_PORT)" -o "-k $(PG_REPLICA_RUN_DIR)" start
 
-restart_pg_primary:
-	@$(PG_BIN_DIR)/pg_ctl --silent --log /dev/null -w -D $(PG_PRIMARY_DATA_DIR) -o "-p $(PG_PRIMARY_PORT)" -o "-k $(PG_PRIMARY_RUN_DIR)" restart
-
 stop_pg_primary:
 	@$(PG_BIN_DIR)/pg_ctl --silent --log /dev/null -w -D $(PG_PRIMARY_DATA_DIR) -o "-p $(PG_PRIMARY_PORT)" -o "-k $(PG_PRIMARY_RUN_DIR)" stop
 

diff --git a/spec/helpers/generic_helper.rb b/spec/helpers/generic_helper.rb
@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+module GenericHelper
+  def wait_for(timeout:, &blk)
+    till = Time.now + (timeout.to_f / 1000)
+    sleep 0.001 while Time.now < till && !blk.call
+  end
+end