Replace forked processes that fail to boot - #760
Conversation
ef70db9 to
b5aee2b
Compare
b5aee2b to
b99502f
Compare
|
Hi @rosa 👋 Thanks for rebasing this onto current I looked into the three failing jobs and I'm fairly confident they're pre-existing flakes rather than something introduced by this patch:
The previous head of this branch was green across all 64 checks with the same substantive change. A rerun of the three jobs should clear them — I don't have permissions to retrigger them myself. On the approach itself: happy to discuss if you'd prefer a different shape for this, e.g. a smaller supervisor-side contract instead of the readiness pipe. Just let me know and I can adjust the scope. |
|
Hey @sapandiwakar, yes! These failures are flaky tests, nothing to do with your changes. Nice catch and fix on this bug! I'm making some changes to the solution, mostly around how the callback works. It doesn't seem right to pass over a callback named I tried to keep I'll push some changes to see what you think. |
|
Ok, I've pushed 3 commits with some changes. I've renamed Then, I've refactored the code considerably, leaving the |
|
I might still do some smaller changes. The config option is the only public change so that one needs to stay when merging, but the rest can change. |
|
Going to test this in our app in production to make sure there aren't any issues we might have missed. |
c32d091 to
f616adf
Compare
The tests killed the whole process group to clean up, but the supervisor under test is actively replacing its stalled forks, and a group KILL races that forking: a fork spawned concurrently with the kill can escape it, wake up from its stalled boot long after the test has finished, register itself and start polling, holding the test database's write lock and cascading lock timeouts through unrelated tests, as seen on CI with SQLite. Terminating the supervisor gracefully instead lets it clean up its own forks: it stops replacing them first, and a fork stuck in boot is covered by the QUIT escalation, as its signal handlers are installed before the boot callbacks where it stalls. This also lets go of the replacement fork's liveness assertion: the replacement stalls too, so whether it's alive depends on where its own boot timeout is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move the startup-tracking machinery out of ForkSupervisor into boot guards with a common interface, created by each process when it starts, according to how it runs: a forked process pipes its boot completion to the supervisor through a BootGuards::ForkGuard, created before forking so it survives it, while processes running as threads of their supervisor, or inline, use a BootGuards::NullGuard, which completes on boot without any monitoring. Booting itself completes the guard, so starting a process remains just boot and run. ForkSupervisor checks its forks' boot guards while checking for terminated processes to replace, terminating any fork whose boot has timed out so that it's reaped and replaced as usual, and closing each guard together with the rest of the fork's cleanup, asking the process instances themselves, like the async supervisor does when it checks whether they're alive. The configurable timeout becomes fork_boot_timeout, as it only applies to forked processes: supervised processes running as threads aren't monitored while booting, and the supervisor itself has no parent within Solid Queue that could enforce a timeout on it. The instrumentation event follows suit. Also reuse Timer's monotonic clock instead of redefining it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
f616adf to
3504c4e
Compare
2 participants
What
Track each forked process until its boot callbacks finish. If a child does not report readiness within a configurable startup timeout, terminate it and let the existing reap-and-replacement path replace that configured process.
This applies only to fork mode. The default startup timeout is five minutes.
Why
ForkSupervisorcurrently replaces children only whenwaitpid2(..., WNOHANG)reports that they exited. A child that remains alive while blocked during boot or registration is never returned bywaitpid2, so the supervisor can retain an unusable child indefinitely without replacing it.For a dispatcher, this means the process can remain present while never reaching its polling loop. Due scheduled jobs then remain in
solid_queue_scheduled_executionsinstead of being moved to ready executions.This addresses the concrete recovery gap observed in #751. It does not attempt to detect a process that successfully booted and later stopped making progress.
Reproduction
A deterministic local reproduction is to block a dispatcher in an existing startup hook:
Start Solid Queue in fork mode with a dispatcher configured.
On
main, the dispatcher child remains alive but never reaches its polling loop. Because it has not exited,waitpid2(..., WNOHANG)does not return its PID and the supervisor never replaces it.On this branch, the behavior can be observed quickly by temporarily lowering the timeout:
The supervisor logs the startup timeout, terminates that dispatcher PID, and starts its replacement. The replacement also times out while the hook remains blocked, demonstrating that each configured slot is continuously supervised.
The regression test uses the same failure shape by blocking a worker during registration. It verifies that the stalled child is terminated and replaced while a healthy sibling keeps the same PID.
How
A timed-out child is killed rather than gracefully terminated because a process blocked during boot cannot reach its run loop to complete a graceful stop.
Tests
Relates to #751.