From eadbfb77a7b513efa5986ab254a4b3fc6c8db320 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 14 Sep 2024 13:13:12 -0400 Subject: [PATCH] SQLite3Adapter: Ensure fork-safety Sqlite itself is not fork-safe, as documented in https://www.sqlite.org/howtocorrupt.html section 2.6. Any databases that are inherited from the parent process are unsafe to properly close and may impact either the parent or the child and potentially lead to database corruption. In this commit, a pre-fork callback `prepare_to_fork!` is introduced for the connection pools and adapters to take any necessary actions before forking. The callback is only implemented by the SQLite3Adapter which now invokes `disconnect!` to close all open prepared statements and database connections. As a side effect, the parent process will end up having to re-open database connections if it continues to do work, which may be a small performance overhead for some use cases, but is necessary in order to prevent database corruption. See https://github.com/rails/solid_queue/issues/324 for examples of the type of corruption that can occur. --- activerecord/CHANGELOG.md | 7 +++++++ .../connection_adapters/abstract/connection_pool.rb | 10 ++++++++++ .../connection_adapters/abstract_adapter.rb | 7 +++++++ .../connection_adapters/pool_config.rb | 13 +++++++++++++ .../connection_adapters/sqlite3_adapter.rb | 13 +++++++++++++ 5 files changed, 50 insertions(+) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 4372e9f139636..f5da038de649e 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,10 @@ +* SQLite3Adapter: Ensure fork-safety. + + Previously, forking a Rails process with open SQLite3 database connections could result in + database file corruption under certain circumstances. + + *Mike Dalessio* + * Add support for PostgreSQL `IF NOT EXISTS` via the `:if_not_exists` option on the `add_enum_value` method. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index 68af021a06122..8735689b4eea6 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -442,6 +442,16 @@ def disconnect(raise_on_acquisition_timeout = true) end end + # Notify connections that a fork is imminent, to allow them to take necessary measures to + # ensure fork-safety. + # + # See AbstractAdapter#prepare_to_fork! + def prepare_to_fork! # :nodoc: + @connections.each do |conn| + conn.prepare_to_fork! + end + end + # Disconnects all connections in the pool, and clears the pool. # # The pool first tries to gain ownership of all connections. If unable to diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index df1a2656b5db0..427ebf09b2f8b 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -696,6 +696,13 @@ def reconnect!(restore_transactions: false) end end + # Prepare the connection for an imminent fork. Some database adapters are not fork-safe + # (specifically sqlite3), and so this gives the adapter an opportunity to close or finalize + # any objects that the child process should not inherit. + def prepare_to_fork! + # This should be overridden by concrete adapters. + end + # Disconnects from the database if already connected. Otherwise, this # method does nothing. def disconnect! diff --git a/activerecord/lib/active_record/connection_adapters/pool_config.rb b/activerecord/lib/active_record/connection_adapters/pool_config.rb index 0010a48acd225..1db570fb32135 100644 --- a/activerecord/lib/active_record/connection_adapters/pool_config.rb +++ b/activerecord/lib/active_record/connection_adapters/pool_config.rb @@ -17,6 +17,10 @@ def schema_reflection private_constant :INSTANCES class << self + def prepare_to_fork! + INSTANCES.each_key(&:prepare_to_fork!) + end + def discard_pools! INSTANCES.each_key(&:discard_pool!) end @@ -66,6 +70,14 @@ def pool @pool || synchronize { @pool ||= ConnectionAdapters::ConnectionPool.new(self) } end + def prepare_to_fork! + synchronize do + return unless @pool + + @pool.prepare_to_fork! + end + end + def discard_pool! return unless @pool @@ -80,4 +92,5 @@ def discard_pool! end end +ActiveSupport::ForkTracker.before_fork { ActiveRecord::ConnectionAdapters::PoolConfig.prepare_to_fork! } ActiveSupport::ForkTracker.after_fork { ActiveRecord::ConnectionAdapters::PoolConfig.discard_pools! } diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 888741ba262c4..d73b0ee40155e 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -497,6 +497,19 @@ def initialize_type_map(m) TYPE_MAP = Type::TypeMap.new.tap { |m| initialize_type_map(m) } EXTENDED_TYPE_MAPS = Concurrent::Map.new + def prepare_to_fork! + # It's not safe to fork with an open sqlite connection, because when the object is GCed in + # the child process, it can interfere with existing or subsequent connections to the + # database in the child process, which can lead to corruption. + # + # Note that closing the database will raise a BusyException unless all related statements + # are also closed, which the AbstractAdapter's disconnect! method handles for us. + # + # More on sqlite fork-safety: https://www.sqlite.org/howtocorrupt.html section 2.6 + # More on Rails db corruption: https://github.com/rails/solid_queue/issues/324 + disconnect! + end + private # See https://www.sqlite.org/limits.html, # the default value is 999 when not configured.