Conversation
rails#778 stops a process whose heartbeats keep failing, but both of its paths hang off a raise. A heartbeat can also never return at all: Process#heartbeat does a real round-trip, and on a half-open socket to a database that stopped answering, the thread blocks in the driver indefinitely. Nothing is raised, so presumed_dead? is never reached. Concurrent::TimerTask does not help either, because it reschedules the task and notifies its observers only after the task returns - so the heartbeat thread goes quiet permanently, having neither succeeded nor failed. Its timeout_interval is a no-op that warns it was never implementable. So record when the heartbeat last returned, in an ensure so that success and failure both count, and have a second timer stop the process once that goes older than the alive threshold. The watchdog reads nothing but memory, so it cannot block the way the heartbeat it watches can. This only covers the case where the run loop is still able to act on being unregistered. A process whose run loop is itself blocked needs something harsher, which I have deliberately left out of this change.
Same defect as the heartbeat watchdog, one level up. Pruning is how supervisors notice dead processes, and launch_maintenance_task runs it in a Concurrent::TimerTask, so a prune blocked on an unresponsive database stops this supervisor pruning ever again - the mechanism meant to notice dead processes is built from the same material as the processes it watches, and fails at exactly the moment they do. Track when maintenance last returned and stop the supervisor once that goes past MAINTENANCE_STALL_FACTOR times the alive threshold, which leaves a full missed cycle of slack since the task's own interval is the alive threshold. Stopping rather than arranging replacement, because a supervisor cannot replace itself - its run loop breaks on stopped?, so whatever runs it gets to start a new one. Supervisors need this separately from the heartbeat watchdog in Registrable: stop_to_be_replaced only unregisters and wakes the loop, and unlike Runnable#shutting_down?, Supervisor#supervise never checks registered?.
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
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.
(Written by Claude on @julik's behalf)
Speculative, for #808. #778 stops a process whose heartbeats keep failing, but both of its paths hang off a raise, and a heartbeat can also never return at all:
Process#heartbeatdoes a real round-trip, and on a half-open socket - a pooler that stopped serving while its kernel keeps the connection - the thread blocks in the driver indefinitely, sopresumed_dead?is never reached.Concurrent::TimerTaskdoes not help either, since it reschedules the task and notifies its observers only after the task returns; the heartbeat thread just goes quiet forever, having neither succeeded nor failed.timeout_intervalis a no-op that warns it was never implementable, so there is nothing to configure.Both commits have the same shape: record when the watched call last returned, in an
ensureso success and failure both count, then watch that from a secondTimerTaskthat reads nothing but memory and therefore cannot block the way the thing it watches can. Recording in theensureis what keeps this orthogonal to #778 - that change owns "keeps failing", this one owns "stopped coming back", and they do not double-fire.They differ in what they do about it, and the second is not a tidier version of the first:
stop_to_be_replaced, as Stop processes whose heartbeats keep failing past the alive threshold #778 does.Processes::BaseincludesRegistrable:stop_to_be_replacedonly unregisters and wakes the run loop, and unlikeRunnable#shutting_down?,Supervisor#supervisebreaks onstopped?without ever checkingregistered?. Pruning blocking is also its own failure - it is how supervisors notice dead processes, so it fails at exactly the moment they do.The supervisor threshold is
MAINTENANCE_STALL_FACTOR(2) times the alive threshold rather than a bare comparison, because the maintenance task's ownexecution_intervalis the alive threshold - one threshold of gap between healthy returns is normal, so a bare comparison would fire every cycle. Both tests redefine the blocking method by hand and restore it inensure, since mocha hasraisesbut no blocking equivalent.What this does not cover, and the reason it is a draft: a process whose run loop is itself blocked will not exit even once unregistered, and in fork mode a child is only replaced when it exits. Closing that needs something harsher than
stop-exit!after a grace period, roughly how Puma culls a hung worker - and I would rather hear what you think of the shape before writing it. Pruning deleting rows without signalling anything is related and also untouched here.Ran under sqlite: both new tests fail without their lib change and pass with it, each commit is green standing alone, and
async_supervisor_test+worker_test+process_recovery_testcome out at 30 runs / 104 assertions / 0 failures.Watchdogs watching watchdogs, but at least this one never talks to the database.