fix: restart wrapper monitors in forked processes without closing the parent's connections - #196
Open
aaron-congo wants to merge 15 commits into
Open
aaron-congo wants to merge 15 commits into
aaron-congo wants to merge 15 commits into
Conversation
…gresql PostgreSQL refuses to drop a database that other sessions are connected to. ActiveRecord disconnects its own connections before dropping, but the wrapper's topology monitor, which is shared by the cluster's connections, kept its own connections to the database it was started for. db:drop, db:reset, db:test:prepare, and db:purge therefore failed with "database is being accessed by other users" whenever an Aurora dialect was in use. AwsPostgreSQLAdapter#drop_database now stops the topology monitor through the new WrapperPgConnection#stop_topology_monitor before dropping. The next connection that needs topology starts a new monitor. MySQL does not refuse to drop a database in use, so aws_mysql2 is unchanged.
bin/rails test checks the schema from the test process, which starts a topology monitor there, then clears all ActiveRecord connections and runs db:test:prepare in a child process to drop and reload the test database. The monitor in the parent process kept its connection to the test database, so the child's DROP DATABASE failed with "database is being accessed by other users". Stopping the monitor in drop_database cannot help, because that runs in the child. The ActiveRecord adapters now prepend a ConnectionHandler extension that, after clear_all_connections!, stops the wrapper's monitors and clears the cached topology, so the next connection starts a new monitor straight away. StorageService#registered? lets it skip the cache when no wrapper connection has registered it.
…opology-monitor # Conflicts: # CHANGELOG.md
…atabase The Blue/Green status providers keep their own monitoring connections to the database, outside ActiveRecord's pools and outside the monitor service, so with the bg plugin enabled they blocked DROP DATABASE the same way the topology monitor did. drop_database and the clear_all_connections! extension now also stop them through BlueGreenPlugin.clean_up_providers, and the extension clears the cached Blue/Green status along with the cached topology. The next initial connection starts a new provider.
… parent's connections Only the forking thread survives a fork, so a forked child (Puma preload_app!, Unicorn, Resque) inherited monitors whose threads were dead. run_if_absent kept returning them, the topology never refreshed, and failover in the child could not find the new writer. The child's at_exit shutdown also closed the inherited monitoring connections, ending the parent's sessions. A Process._fork hook now runs in the child only: it forgets inherited monitors and Blue/Green providers, abandons their connections without closing them on the server (pg: socket to /dev/null; mysql2: automatic_close off), and restarts the monitor service, event publisher, and storage cleanup threads.
…ailover spec The spec warmed the topology cache before establish_connection, but clear_all_connections! now stops the monitors and clears that cache, so the warm-up was lost. If the new monitor's first connection raced with the reader outage, it never learned the writer and failover timed out (seen on MySQL in CI). Wait for the connection's own monitor to cache every instance before cutting connectivity instead.
sophia-bq
reviewed
Oct 6, 2026
| @lock.synchronize { @caches.values }.each do |container| | ||
| container.cache.entries.each_key { |key| container.cache.remove(key)&.release_after_fork } | ||
| end | ||
| @running = true |
Contributor
There was a problem hiding this comment.
I'm curious on the locking pattern around @running, it is set without a lock here and in the storage_service but it is updated under lock in BatchingEventPublisher
Contributor
Author
There was a problem hiding this comment.
Good point - MonitorService and StorageService set and read @running without the lock while BatchingEventPublisher used its lock, so I've made all three consistent: each now uses a Concurrent::AtomicBoolean (the same pattern Monitoring::Monitor uses for its stop flag), so the flag is thread-safe everywhere without taking a lock. The publisher's @lock still guards its subscribers and event queue.
sophia-bq
reviewed
Oct 6, 2026
|
|
||
| # Forgets the providers inherited by a forked child so the child starts its own on first use. | ||
| def self.release_providers_after_fork | ||
| PROVIDERS.each_key { |k| PROVIDERS.delete(k)&.release_after_fork } |
Contributor
There was a problem hiding this comment.
are we concerned that a delete could raise? if it does the remaining providers will not be addressed
Contributor
Author
There was a problem hiding this comment.
Good catch, that is a great point. Releasing providers and monitors now logs and continues per item (a failed one is still forgotten), and the fork hook logs any remaining error instead of raising it.
…ground services MonitorService and StorageService set and read @running without a lock, while BatchingEventPublisher took its lock for every access. All three now use a Concurrent::AtomicBoolean, so the flag is thread-safe without a lock. The publisher's lock still guards its subscribers and event queue.
sophia-bq
previously approved these changes
Oct 6, 2026
…itor' into fix/fork-safe-monitors # Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
JuanLeee
approved these changes
Oct 6, 2026
JuanLeee
previously approved these changes
Oct 6, 2026
- Forget Secrets Manager fetches in flight at fork time, so a forked child does not wait on a future whose thread did not survive the fork. - Raise the timeout error when a Secrets Manager fetch does not finish in time. Future#value! returns nil on timeout instead of raising, so the fetch previously failed later with a generic auth error. - Drop the references to abandoned monitor connections, so a later close cannot send COM_QUIT on a socket shared with the parent. - Keep a still-running background thread in restart_after_fork, so calling after_fork again does not start duplicate threads. - Kill a hung forked child in the fork safety spec after a timeout instead of stalling the suite.
sophia-bq
approved these changes
Oct 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix forked processes (Puma
preload_app!, Unicorn, Resque) inheriting dead wrapper monitors, so the topology never refreshed in the child and failover there could not find the new writer. Also stop a forked child from closing the parent's monitoring connections when it exits.Description
Stacked on #195 - review and merge that first.
Only the thread that calls
forksurvives into the child, so every wrapper background thread is gone there. Two problems followed:run_if_absentkept returning them even though their threads were dead, soforce_refresh_host_list?timed out and returnedfalse, and failover in the worker could not find the new writer. The shared background threads were also dead (event publisher, storage cleanup, monitor-service cleanup). Loading the ActiveRecord adapter at boot starts these in the parent, so every preloaded worker lacked them even without any database use.at_exitshutdown stopped the inherited monitors, which closed their connections and ended the parent's sessions. Garbage collection in the child does the same: pg's free sends Terminate and mysql2'sautomatic_closesends COM_QUIT.Changes:
Process._forkhook (AwsAdvancedRubyDriverWrapper::ForkHook) callsAwsAdvancedRubyDriverWrapper.after_forkin the child only.Process._forkis the single entry point forKernel#fork,Process.forkandIO.popen('-').after_forkforgets the inherited monitors (MonitorService#restart_after_fork) and Blue/Green providers (BlueGreenPlugin.release_providers_after_fork), so the child starts its own on first use. It then restarts the monitor service, event publisher and storage cleanup threads. Cached data such as topology is kept.Monitor#release_after_forkand anabandon_connectionshook implemented byClusterTopologyMonitorand the Blue/GreenStatusMonitor. The newDriverDialect#abandon_connectiondoes the per-driver work:/dev/null, as ActiveRecord'sdiscard!does.automatic_close = false, so the socket is invalidated instead of sending COM_QUIT.Testing:
spec/unit/fork_safety_spec.rbuses a realfork. It covers: a fresh monitor in the child instead of the dead inherited one; the parent's monitor still running; all three core threads restarted; BG providers forgotten without being stopped; andshutdownin the child (theat_exitpath) not closing anything inherited. The examples fail without the hook.abandon_connection(pg and mysql2),MonitorConnection#abandonandClusterTopologyMonitor#release_after_fork.bundle exec rspec spec/unit: 3136 examples, 0 failures.bundle exec rubocop lib spec: no offenses.force_refresh_host_list?returnedfalseafter the 5s timeout, all three core threads were dead, and on MySQL the parent's monitor session was gone from the processlist after the child exited.force_refresh_host_list?returnstruein the child in about 1s, the child's threads are alive, and the parent's monitor sessions stay open after the child exits. The parent keeps refreshing and querying normally.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.