Skip to content

Prevent false worker pruning from leaking concurrency permits#11

Open
tblais1224 wants to merge 1 commit into
mainfrom
fix/prevent-false-prune-concurrency-leak
Open

Prevent false worker pruning from leaking concurrency permits#11
tblais1224 wants to merge 1 commit into
mainfrom
fix/prevent-false-prune-concurrency-leak

Conversation

@tblais1224

Copy link
Copy Markdown

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:

  1. The worker remained alive and continued processing a long-running calendar sync.
  2. Its heartbeat repeatedly failed while establishing a PostgreSQL connection with SSL error: unexpected eof while reading.
  3. After the heartbeat passed process_alive_threshold, another supervisor pruned the worker record as dead.
  4. Pruning failed the claimed execution, returned its concurrency permit, and promoted a replacement job, but it did not stop or fence the original Ruby execution.
  5. The original execution continued for approximately eight minutes after pruning. When it returned, its stale ClaimedExecution finalized the job and returned the same permit again.
  6. Each duplicate permit return admitted more work. Graceful worker replacement then re-readied the leaked claimed population without reacquiring permits, preserving the concurrency leak across deploys.

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?

  • Desktop web
  • Mobile web
  • iOS
  • Android
  • API
  • N/A

QA steps

Automated regression coverage verifies:

  1. A stale supervised worker and its claimed execution remain registered while its supervisor heartbeat is healthy.
  2. A stale worker is still pruned when its supervisor is also stale.
  3. A stale performer returning after pruning cannot mark the failed job finished, promote an additional blocked job, or return another semaphore permit.
  4. Existing worker-exit, graceful-release, failure, and semaphore behavior remains intact.

Verification completed:

  • Full SQLite suite: 254 runs, 1,493 assertions, 0 failures, 4 skips.
  • Focused PostgreSQL suite: 30 runs, 173 assertions, 0 failures.
  • Focused MySQL suite: 30 runs, 173 assertions, 0 failures.
  • RuboCop: 169 files inspected, no offenses.

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.

* 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))
}

@tblais1224 tblais1224 Jul 10, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tblais1224 tblais1224 Jul 10, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tblais1224

Copy link
Copy Markdown
Author

How the ClaimedExecution changes work

The central idea is that the database claim row is now the ownership token for terminal job bookkeeping. Holding an old ClaimedExecution Ruby object is no longer enough to finish or fail a job, return its concurrency permit, or promote blocked work.

The previous race

Before this change, pruning and normal completion could both return the same permit:

job A is running; jobs B and C are blocked
  → A's heartbeat becomes stale
  → the pruner fails A, deletes its claim, returns A's permit, and promotes B
  → A's Ruby thread is still alive and eventually returns
  → `perform`'s unconditional `ensure` returns A's permit again and promotes C

This was possible because the database claim had been deleted, but the original worker still held an in-memory ClaimedExecution object. The old ensure did not verify whether that execution still owned anything.

perform no longer signals unconditionally

app/models/solid_queue/claimed_execution.rb:64

def perform
  result = execute

  if result.success?
    finished
  else
    failed_with(result.error)
    raise result.error
  end
end

Removing unblock_next_job from ensure means completion must go through either finished or failed_with. Both paths now verify claim ownership before performing terminal bookkeeping.

Success and failure share one finalization path

app/models/solid_queue/claimed_execution.rb:88

def failed_with(error)
  finalize_claim do
    job.failed_with(error)
  end
end

app/models/solid_queue/claimed_execution.rb:102

def finished
  finalize_claim do
    job.finished!
  end
end

Both delegate to:

app/models/solid_queue/claimed_execution.rb:108

def finalize_claim
  with_claim do
    yield
    destroy!
    job.unblock_next_blocked_job
  end
end

This puts the complete terminal transition in one transaction:

  1. Verify and lock ownership of the claim.
  2. Mark the job finished or failed.
  3. Delete the claim.
  4. Return the semaphore permit and promote one blocked job.

If any step raises, the transaction rolls back. We no longer commit claim deletion separately from semaphore signaling.

with_claim makes ownership exact-once

app/models/solid_queue/claimed_execution.rb:116

def with_claim
  transaction do
    return false unless self.class.unscoped.lock.find_by(id: id)

    yield
  end
end

The lookup issues a SELECT ... FOR UPDATE against the claimed-execution row. If a performer and pruner race, only one can lock and finalize the claim:

winner locks claim → updates job → deletes claim → returns permit → commits
loser resumes       → claim no longer exists       → returns false → does nothing

find_by is deliberate because a missing claim is an expected lost-ownership result, not an application error.

unscoped is also deliberate. fail_all_with loads claims using includes(:job), and PostgreSQL rejects FOR UPDATE when Active Record carries that outer join into the lock query. The unscoped lookup locks only the solid_queue_claimed_executions row whose ownership matters.

Bulk failure no longer signals separately

app/models/solid_queue/claimed_execution.rb:39

executions.each do |execution|
  execution.failed_with(error)
end

failed_with now records the failure, deletes the claim, and returns the permit atomically. The caller no longer performs a second, separate unblock_next_job operation.

Graceful release also checks ownership

app/models/solid_queue/claimed_execution.rb:75

with_claim do
  job.dispatch_bypassing_concurrency_limits
  destroy!
end

The concurrency bypass remains intentional: a gracefully released job already owns a permit, so making it reacquire that same occupied permit could deadlock it. The new protection is that redispatch only happens if the claim row still exists. If pruning or completion already won, the stale release does nothing.

What this fixes—and what it does not

This prevents a stale execution from:

  • changing a pruned job from failed to finished;
  • returning the same concurrency permit twice;
  • promoting another blocked job;
  • turning one false prune into a persistent concurrency leak.

It cannot stop application work already running in the original Ruby thread. That is why this PR also changes process pruning: a stale supervised worker is protected while its owning supervisor remains healthy. The pruning change avoids starting the first replacement in the observed failure mode; the ClaimedExecution change prevents semaphore amplification if pruning and completion still race.

@tblais1224 tblais1224 marked this pull request as ready for review July 10, 2026 20:24
@tblais1224 tblais1224 self-assigned this Jul 10, 2026
@tblais1224 tblais1224 requested a review from jpcamara July 10, 2026 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant