Harden Redis queue uniqueness with atomic Lua enqueue - #1446
Open
danielgerlag wants to merge 1 commit into
Open
Conversation
QueueWork used a non-atomic LINSERT/RPUSH/LREM sequence, so concurrent enqueues of a missing id could leave duplicate pending LIST entries. Keep the same LIST keys and algorithm, but run them in one EVAL so at most one pending occurrence exists per id. Dequeue stays a single LPOP. Co-authored-by: Daniel Gerlag <danielgerlag@users.noreply.github.com>
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
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.
Describe the change
RedisQueueProvider.QueueWorkde-duped pending ids with a non-atomicLINSERT+ conditionalRPUSH/LREM. Two concurrent enqueues of a missing id could both miss, bothRPUSH, and leave duplicate pending LIST entries. Dequeue was (and remains)LPOP.This change makes pending-id uniqueness atomic inside the Redis queue provider only.
IQueueProvideris unchanged. Issue #1417 / PR #1444 (QueueConsumer._secondPassesafter purge) are a separate consumer-side problem and are not addressed here.Describe your implementation or design
Smallest upgrade-safe design: keep the existing LIST keys and the existing LINSERT/RPUSH/LREM algorithm, but run that sequence in one Lua
EVALso Redis serializes it.Rejected alternative (SADD membership + RPUSH, LPOP+SREM): extra key, rolling-upgrade skew between old processes (LIST-only) and new processes (LIST+SET), and a crash window unless dequeue is also a single Lua. Safer for existing deployments is “no second key.”
DequeueWorkstays a singleLPOP. Because there is no companion SET, there is nothing to desync if a process dies after pop.Tests
Added
RedisQueueProviderFixtureinWorkflowCore.Tests.Redis(existing Squadron Redis collection — this repo does not use Testcontainers for Redis):QueueWorkof a missing id leaves exactly one pending entryQueueWorkwhile pending is a no-op and preserves FIFO positionQueueWorkand drain viaLPOPBreaking change
No required migration or flush. Same Redis keys, same intended uniqueness semantics, now actually atomic.
Compatibility
This design was chosen so existing Redis keys keep working. Details:
LPOPdrains them one by one). A newQueueWorkof that id does not add another occurrence. Operators do not need a one-time flush; optionally deleting{prefix}-workflows|events|indexwould only drop in-flight work (not recommended).RedisQueueProviderprocesses): Both generations share the same LIST. New processes enqueue atomically among themselves. An old process can still race its own multi-command sequence (or interleaveLINSERTmiss +RPUSHnext to a new process) and insert a duplicate until every node is upgraded. After the rollout completes, uniqueness holds.LPOP. An id cannot get stuck in a membership set or be lost because of a failed follow-upSREM.RPUSH. After this change, re-queue of an id that is still pending is always a no-op. After dequeue, the same id may be queued again. FIFO position of an already-pending id is unchanged (it is not moved to the tail).LINSERT(2.2+) and Lua (2.6+). NoLPOS/ Redis 6 requirement.IQueueProvider/ other providers: Unchanged. Production behavior outsideRedisQueueProvideris unchanged.Additional context
Queue key format is unchanged:
{prefix}-workflows,{prefix}-events,{prefix}-index.