Skip to content

Prevent false worker pruning from leaking concurrency permits#11

Open
tblais1224 wants to merge 3 commits into
mainfrom
fix/prevent-false-prune-concurrency-leak
Open

Prevent false worker pruning from leaking concurrency permits#11
tblais1224 wants to merge 3 commits 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
Comment thread app/models/solid_queue/process/prunable.rb
@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

@jpcamara jpcamara left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I’ve left the requested review feedback inline, with proposed fixes on each relevant line.

Comment thread app/models/solid_queue/claimed_execution.rb Outdated
Comment thread app/models/solid_queue/claimed_execution.rb Outdated
Comment thread app/models/solid_queue/process/prunable.rb
Comment thread test/models/solid_queue/claimed_execution_test.rb
* Split unblock_next_blocked_job into release_concurrency_permit and
  promote_next_blocked_job so promotion runs after the claim transaction
  commits, matching the failure isolation from 873760d
* Returning the permit inside the claim transaction keeps ownership atomic;
  promoting outside it means a promotion failure can no longer roll back a
  job that already finished or failed, and avoids holding the semaphore
  lock while waiting on the blocked row's lock
* Deregister a supervised process's row as soon as its supervisor reaps it,
  mirroring Process::Prunable#prune, so a dead child no longer stays
  permanently protected from pruning just because its supervisor is alive
* Update the two existing tests whose expected process counts depended on
  that dead row lingering
* Add non-transactional, multi-connection tests that exercise the actual
  FOR UPDATE serialization in ClaimedExecution finalization: both race
  outcomes (pruner wins, performer wins) across finish/fail/release paths,
  and a use_skip_locked=false regression guarding against the reverse lock
  order between the semaphore and the blocked row
@tblais1224 tblais1224 requested a review from jpcamara July 14, 2026 18:51
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.

2 participants