Skip to content

Prevent concurrency lock leaks that cause duplicate job execution#761

Merged
rosa merged 2 commits into
mainfrom
prevent-concurrency-permit-leak
Jul 23, 2026
Merged

Prevent concurrency lock leaks that cause duplicate job execution#761
rosa merged 2 commits into
mainfrom
prevent-concurrency-permit-leak

Conversation

@rosa

@rosa rosa commented Jul 21, 2026

Copy link
Copy Markdown
Member

Supersedes #689 and ports the exact-once fix from starburstlabs/solid_queue#11. Both target the same failure: a job with limits_concurrency whose semaphore count gets permanently corrupted, letting it run well past its configured limit.

Background

Two independent reports converged on the same class of bug:

The failure: a worker stays alive on a long-running job but its DB heartbeat fails transiently. Another supervisor prunes it as dead — failing its claimed execution, releasing its concurrency lock, and unblocking a replacement — but the original execution keeps running. When it finally returns, its stale ClaimedExecution finalizes the job and releases the same lock a second time, pushing the semaphore value above its limit and admitting ever more concurrent work.

Changes

1. Guard signal_all against exceeding the limit (from #689, @mhenrixon)

Semaphore#signal already caps its increment with value < limit, but the batch signal_all path — used when releasing concurrency locks in bulk, e.g. discarding ready jobs — did not, letting a semaphore's value climb past its limit. It's now grouped by limit and capped, matching the single-signal path.

2. Prevent wrong release of blocked jobs and semaphore corruption (from starburstlabs#11, @tblais1224 & @briceburg-wb)

The problem: a worker stays alive on a long-running job but its database heartbeat fails transiently. Another supervisor prunes it as dead, fails its claimed execution, releases its concurrency lock, and unblocks a replacement — but the original execution keeps running. When it finally returns, its stale ClaimedExecution, which exists only in memory, because the DB record was deleted by the supervisor prune, also goes through the semaphore and blocked execution release, potentially violating the limit and allowing more concurrent jobs.

With this change, we take a lock on the claimed execution row so that it's required that it still exists and we're the only ones acting over it, before running the finalization code: deleting the claimed execution row, signaling the semaphore and unblocking blocked jobs. In the scenario described above, we'd see the claimed execution record as gone and would do nothing.

Note that the semaphore signal + blocked jobs release is still done after the transaction that marks the job as finished/failed, so that these two actions can't rollback a job that finished/failed (see 873760d). Because this can only happen for the ones who got to clear the claimed execution row, we still guarantee the lock is released just once.

Scope

Credit to @mhenrixon (#689) and @tblais1224 & @briceburg-wb (starburstlabs#11). #689 can be closed in favor of this.

`Semaphore#signal` (the single-job path) already caps its increment with
`value < limit`, but the batch `signal_all` path — used when releasing
concurrency locks in bulk, e.g. when discarding ready jobs — did not. That
let a semaphore's value climb above the concurrency limit, effectively
leaking permits and admitting more concurrent executions than allowed.

Group the jobs by their concurrency limit and cap each group's increment
with `value < limit`, matching the single-job path.
@rosa
rosa force-pushed the prevent-concurrency-permit-leak branch 3 times, most recently from d296be1 to f640eb9 Compare July 21, 2026 14:26
@rosa rosa changed the title Prevent concurrency permit leaks that cause duplicate job execution Prevent concurrency lock leaks that cause duplicate job execution Jul 21, 2026
@rosa
rosa force-pushed the prevent-concurrency-permit-leak branch 4 times, most recently from 7e0f8bb to ccef90c Compare July 21, 2026 16:29
@tblais1224

Copy link
Copy Markdown
Contributor

Thanks for prioritizing this change @rosa. I can confirm that starburstlabs#11 has been running stable in production and has helped concurrency issues. I am however experiencing the issue that #689 seems to resolve. So I think joining these together was a great call, and should make this much more stable. 👏

@rosa
rosa force-pushed the prevent-concurrency-permit-leak branch from ccef90c to f835d9e Compare July 21, 2026 17:37
Ported from starburstlabs#11, a production-diagnosed fix for a
concurrency-limited job whose semaphore count could be permanently
corrupted, letting it run well past its configured limit.

The problem: a worker stays alive on a long-running job but its database
heartbeat fails transiently. Another supervisor prunes it as dead, fails its
claimed execution, releases its concurrency lock, and unblocks a replacement
— but the original execution keeps running. When it finally returns, its
stale `ClaimedExecution`, which exists only in memory, because the DB record
was deleted by the supervisor prune, also goes through the semaphore and
blocked execution release, potentially violating the limit and allowing
more concurrent jobs.

With this change, we take a lock on the claimed execution row so that
it's required that it still exists and we're the only ones acting over
it, before running the finalization code: deleting the claimed execution
row, signaling the semaphore and unblocking blocked jobs. In the
scenario described above, we'd see the claimed execution record as gone
and would do nothing.

Note that the semaphore signal + blocked jobs release is still done
after the transaction that marks the job as finished/failed, so that
these two actions can't rollback a job that finished/failed (see
873760d). Because this can only happen for the ones who got to clear the
claimed execution row, we still guarantee the lock is released just
once.

Co-Authored-By: Brice Burgess <brice.burgess@wealthbox.com>
Co-Authored-By: Tom Blais <tom.blais@wealthbox.com>
@rosa
rosa force-pushed the prevent-concurrency-permit-leak branch from f835d9e to 858fadb Compare July 21, 2026 17:54
@rosa

rosa commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

Thank you so much @tblais1224!

I'm going to run this one in production as well. It's not the whole change in starburstlabs#11, but a subset and with a small tweak. I described it in the commit 858fadb. I think this is enough to solve the issue you described in your PR.

@rosa

rosa commented Jul 22, 2026

Copy link
Copy Markdown
Member Author

We're running this in production now. I'll monitor for one day and will merge tomorrow; then I'll release a new version,

@rosa

rosa commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

This has been working without issues, so I'm going ahead with it. Thanks again @mhenrixon, @tblais1224 and @briceburg-wb!

@rosa
rosa merged commit 4f263b2 into main Jul 23, 2026
376 of 384 checks passed
@rosa
rosa deleted the prevent-concurrency-permit-leak branch July 23, 2026 10:01
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.

3 participants