Prevent false worker pruning from leaking concurrency permits#11
Prevent false worker pruning from leaking concurrency permits#11tblais1224 wants to merge 1 commit into
Conversation
* Keep stale supervised workers while their supervisor heartbeat is healthy * Make claimed execution finalization own and release permits exactly once * Add regression coverage and document pruning behavior
|
|
||
| where(last_heartbeat_at: ..heartbeat_cutoff) | ||
| .where.not(id: protected_processes.select(:id)) | ||
| } |
There was a problem hiding this comment.
what now prevents an actually dead child process with a running supervisor from staying active indefinitely and holding a worker? Is just relaying on forced worker restarts / deploys enough?
There was a problem hiding this comment.
Maybe we implement an additional early return with a much longer heartbeat cutoff on the child, like 15 or 30 minutes to address the above concern. Wondering how all this may impact performance too.
There was a problem hiding this comment.
Codex reply:
An actually exited child should not remain protected indefinitely. The live supervisor checks the OS every second in lib/solid_queue/fork_supervisor.rb:27:
pid, status = ::Process.waitpid2(-1, ::Process::WNOHANG)
replace_fork(pid, status)Once it observes the exit, it fails the child’s claims with ProcessExitError and starts a replacement. Async mode similarly checks instance.alive?. test/unit/process_recovery_test.rb:18 covers killing a worker, having the supervisor reap it, failing its claim, and replacing it.
The remaining concern is slightly different: an OS process that is still alive but wedged, or whose heartbeat thread has stopped. The current change intentionally fails closed in that case, so it can hold its claims until the worker/supervisor is restarted.
A second 15–30 minute cutoff would bound that availability problem, but it would also reintroduce the assumption that a sufficiently stale heartbeat proves the execution is dead. If the worker is still running, pruning it would start one overlapping replacement. The new claim-ownership logic prevents that overlap from permanently corrupting the semaphore and amplifying to many workers, but it cannot prevent duplicate external side effects from the initial overlap.
I don’t think query performance is a significant concern here. Pruning runs every five minutes, the process table contains active registrations rather than job history, and both last_heartbeat_at and supervisor_id are indexed.
My preference is to keep this PR safety-first. If we need bounded recovery for alive-but-hung workers, a safer follow-up would have the owning supervisor terminate and reap the child after a longer threshold, then release its claim only after the OS confirms the child exited. That preserves the concurrency guarantee instead of returning the permit while the old process may still execute.
How the
|
What does this PR do? (required)
Prevents a stale Solid Queue heartbeat from turning one concurrency-limited execution into multiple overlapping executions and permanently corrupting the semaphore count.
Incident and failure sequence
The production investigation started with
Sync::ExternalCalendar::SyncJob, configured with a concurrency limit of one, running up to 11 times concurrently for the same calendar configuration.One confirmed execution followed this sequence:
SSL error: unexpected eof while reading.process_alive_threshold, another supervisor pruned the worker record as dead.ClaimedExecutionfinalized the job and returned the same permit again.The immediate database/network cause of the SSL EOF remains unknown. This PR instead makes that transient heartbeat failure safe.
Safeguards
Corroborate worker liveness with its supervisor
A supervised worker with a stale heartbeat is no longer pruned while its owning supervisor has a fresh heartbeat. The supervisor already detects actual fork or thread exits directly and fails those claims with
ProcessExitError, which is stronger evidence than a failed database heartbeat.If both worker and supervisor heartbeats become stale, the existing pruning recovery remains available. This intentionally favors preventing concurrent side effects while the owning supervisor is known to be alive.
Make terminal claim ownership exact-once
Finishing, failing, and graceful release now lock the claimed-execution row before changing job state. Only the actor that still owns that row can delete the claim or return its concurrency permit.
Job state changes, claim deletion, semaphore signaling, and blocked-job promotion occur in one transaction. If pruning already removed the claim, the stale in-memory performer performs no terminal bookkeeping and cannot signal the semaphore a second time.
Together, these changes prevent the observed false prune when the supervisor remains healthy and prevent permanent concurrency amplification even if pruning races with a live completion.
Link to Basecamp to-do, Trello card, New Relic or Honeybadger (required)
N/A — production investigation for external calendar configuration
58070.QA
What platforms should be included in QA?
QA steps
Automated regression coverage verifies:
Verification completed:
Screenshots (if appropriate)
N/A
Docs
The process lifecycle documentation now explains that a stale supervised process is protected while its supervisor heartbeat remains fresh.