fix(run-engine): stop a '*' concurrency key stranding its whole base queue - #4628
Open
1stvamp wants to merge 1 commit into
Open
fix(run-engine): stop a '*' concurrency key stranding its whole base queue#46281stvamp wants to merge 1 commit into
1stvamp wants to merge 1 commit into
Conversation
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (20)
WalkthroughThe run queue preserves the master-queue wildcard entry when the concurrency key is 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1stvamp
force-pushed
the
fix/ck-wildcard-master-queue-strand
branch
from
August 15, 2026 17:58
2cb7d5b to
2b2a3c3
Compare
…queue A concurrency key is an unrestricted client string, so '*' reaches the queue unescaped, and queueKey renders it as a variant name byte-identical to the wildcard member the CK scripts keep in the master queue for that base queue. Each CK script rebalances the master queue with that wildcard member and then removes the 'old-format' entry for the variant it just touched. Where the variant IS the wildcard those two calls name the same member, so the cleanup undid the rebalance and took the base queue's only master-queue entry with it. Nothing then pointed at the queue, so every concurrency key on it silently stopped being dequeued until some later write happened to re-add the member. Guards the cleanup in all 10 CK scripts (4 enqueue, 6 ack/nack/dead-letter). No key-format change, so state already in Redis is repaired by the next write rather than needing a migration. Tests cover the enqueue, ack and nack paths, and fail without the guard.
1stvamp
marked this pull request as ready for review
August 15, 2026 18:16
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1stvamp
force-pushed
the
fix/ck-wildcard-master-queue-strand
branch
from
August 15, 2026 18:20
2b2a3c3 to
5cd8c9a
Compare
1stvamp
added a commit
that referenced
this pull request
Aug 15, 2026
Carries the fix from #4628 into the three CK scripts this branch adds, which do not exist on main and so could not be covered there. A concurrency key of '*' renders a variant name identical to the wildcard member the master queue uses for the base queue, and the unguarded transition cleanup then removed the entry the rebalance had just written, stranding every concurrency key on that queue. The pre-existing scripts are fixed in #4628; this is the same one-line guard applied to enqueueMessageCkVtimeTracked, enqueueMessageWithTtlCkVtimeTracked and nackMessageCkVtimeTracked.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
A concurrency key is an unrestricted client string (
ConcurrencyKeySchemaisz.union([z.string(), z.number()]).transform(String)), andconcurrencyKeySectiondoes no escaping, so*reaches the queue raw.queueKeythen renders it as...:queue:<q>:ck:*, which is byte-identical to the wildcard member the CK scripts keep in the master queue to mean "this base queue has concurrency-key work".Every CK script ends with the same pair:
ckWildcardNameistoCkWildcard(message.queue), and for a*-keyed run that returns the identical string, so the cleanup on the second line deletes what the rebalance on the first line just wrote.The master queue then has no entry for that base queue, while
ckIndexand the variant queues still hold the work. Every concurrency key on the queue stops being dequeued, not just the*one. It is silent, and it only recovers if some later write happens to re-add the member.Reproduced before the fix:
Blast radius is bounded to the environment that triggers it, so it is self-inflicted rather than cross-tenant, but a single trigger stalls the queue for everything on it.
The fix
Guard the cleanup so it never removes the wildcard member:
Applied to all 10 CK scripts (4 enqueue, 6 ack/nack/dead-letter). No key-format change and no migration: a queue already stranded in Redis is repaired by its next write.
I considered rejecting
*at the API boundary instead and rejected it. Existing Redis state andTaskRun.concurrencyKeyrows already hold raw:-bearing and*keys, so changing key construction would orphan in-flight messages and split concurrency accounting mid-deploy. Boundary validation would still be reasonable as belt-and-braces later, but the Lua guard alone fixes it including for state already out there.Testing
ckWildcardKey.test.tscovers the enqueue, ack and nack paths. All three pass with the guard and all three fail without it, verified by reverting. Fullsrc/run-queue/suite is green (166 tests).Note for #4367
The virtual-time branch adds three more CK scripts with the same pattern (
enqueueMessageCkVtimeTracked,enqueueMessageWithTtlCkVtimeTracked,nackMessageCkVtimeTracked). They do not exist on main so they are not in this PR; the same guard needs applying there, and I will do that on that branch.