Repository navigation
Add GroupId to queues (tenant/group key with SQS fair-queue, FIFO, and Service Bus session support) - #579
Conversation
Adds QueueEntryOptions.GroupId, IQueueEntry.GroupId and SharedQueueOptions.GroupIdResolver. The resolver runs before Enqueuing handlers, never mutates caller options, and TraceState no longer overwrites caller properties. Shared harness tests and docs included.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765a7c3af9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Updated 2026-10-06: reviewed repairs are now pushed at Repair and local validation evidenceFollow-up review: a nonempty explicit GroupId wins; empty/null invokes the resolver. The repair clarifies the docs and adds the empty-value-plus-resolver regression. Empty GroupId normalization and null Properties normalization are already implemented, so those bot findings are false positives. Service Bus documentation now states the producer-only session scope. Local Release queue fixture: 59 passed, 2 existing serialization skips; focused GroupId selection: 9 passed. These local checks ran on 2026-10-05. Application/test source is unchanged from those validated repair commits; this follow-through added only the approved workflow guard/pin. YAML parsing and diff checks passed. No new local application test run is claimed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a28d0f736d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…claims - QueueEntryOptions.GroupId stores empty as null, so an Enqueuing handler can no longer hand providers an empty group id; QueueBase only copies options when the resolver supplies a value, and keeps the caller's Properties comparer - The unsupported-provider debug message no longer claims every provider stores the value, since older or custom providers may drop it - Docs and skill no longer list Azure Service Bus as changing delivery: it sends SessionId but cannot receive from session-enabled queues yet
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c63b0ca10c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
EnqueueAsync now copies the caller's options once (including Properties with its comparer) and applies the GroupId resolver plus Activity CorrelationId/TraceState to the copy before EnqueueImplAsync. Previously only the resolver path copied, so a reused options instance kept the first message's CorrelationId and TraceState, and Enqueuing handler edits leaked back to the caller. Providers can now validate the final options before Enqueuing handlers run. Move provider-neutral tests into QueueTestBase: Enqueuing handler clearing GroupId, reused options isolation, and Enqueued entry GroupId. Restore CI package publishing for non-main branches so provider PRs can consume a nightly that matches this branch.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
… SQS docs The shared test now checks only what every provider can honor: the caller's options are unchanged and a second enqueue does not fail. CorrelationId round-trips are already covered by other tests, so legacy Azure Storage no longer needs to skip it. Docs now say UniqueId is sent to SQS only on FIFO queues, and describe FIFO dead-letter naming and deletion when no dead-letter queue exists.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The existing retry test used a zero delay, so providers that re-send the message for a delayed retry (Service Bus) never exercised GroupId on that path. The same assertion now runs with a one second retry delay.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a098e6a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Without sessions, Service Bus does not guarantee ordered processing with concurrent workers, and AzureServiceBusQueue cannot receive from session-enabled queues.
…y, move resolver tests to QueueTestBase - QueueTestBase.GetQueue gains a groupIdResolver parameter (breaking for TestHarness overrides) - Resolver tests moved from InMemory to virtual base tests - CopyProperties preserves only Dictionary comparers - Docs: Service Bus session ordering, SQS delay clamping and FIFO rejection, KMS default, remove version-gating notes - Add cross-repo-validation agent skill
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47e207d2c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| protected virtual IQueue<SimpleWorkItem>? GetQueue(int retries = 1, TimeSpan? workItemTimeout = null, TimeSpan? retryDelay = null, int[]? retryMultipliers = null, int deadLetterMaxItems = 100, bool runQueueMaintenance = true, TimeProvider? timeProvider = null, ISerializer? serializer = null) | ||
| protected virtual IQueue<SimpleWorkItem>? GetQueue(int retries = 1, TimeSpan? workItemTimeout = null, TimeSpan? retryDelay = null, int[]? retryMultipliers = null, int deadLetterMaxItems = 100, bool runQueueMaintenance = true, TimeProvider? timeProvider = null, ISerializer? serializer = null, Func<SimpleWorkItem, string?>? groupIdResolver = null) |
There was a problem hiding this comment.
Adding an optional parameter still changes the method signature, so every existing provider or consumer test suite that overrides the previous eight-parameter GetQueue method will fail to compile with CS0115 when it updates Foundatio.TestHarness. Keep the existing virtual signature and introduce a separate overload or resolver hook so consumers can adopt group-ID testing without a source-breaking package update.
AGENTS.md reference: AGENTS.md:L175-L175
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is an intentional breaking change for the next major. Every provider override (Redis, AWS, Azure Storage, Azure Service Bus) is updated in its paired PR, and the resolver tests in the shared base depend on it. An overload would mean overrides that ignore the resolver could silently skip those tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8b67b2852
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
* Round-trip GroupId in Redis queue envelope Depends on FoundatioFx/Foundatio#579 * Register the empty GroupId queue regression * Pin guarded build workflow for feature branch validation * Consume the queue-group-id nightly of Foundatio and Foundatio.TestHarness The GroupId APIs are not in 13.0.4. Use the nightly until a release is available. * Count and back off on worker dequeue errors The Redis worker loop logged dequeue errors and immediately looped again without counting them. It now counts them, ignores cancellation only when its own token is cancelled, and waits one second before retrying. Documents why the delayed-retry GroupId test is skipped: Redis requeues delayed retries in the minute-interval maintenance task. * Restore main worker loop; keep GroupId plumbing only * Use matching GroupId core and TestHarness preview * Accept groupIdResolver in GetQueue, run base resolver tests, use core preview .0.21 * Update Foundatio to 13.0.5-preview.0.10 and point build workflow back to main * Remove archived Minio provider from README * Retry test flush after refreshing cluster topology when a cached primary became a replica * Format
* Round-trip GroupId in Azure Storage queue envelope Depends on FoundatioFx/Foundatio#579 * Align legacy enqueue event metadata with stored messages * Pin guarded build workflow for feature branch validation * Consume the queue-group-id nightly of Foundatio and Foundatio.TestHarness The GroupId APIs are not in 13.0.4. Use the nightly until a release is available. * Forward the original body to the dead-letter queue and back off on worker errors Dead-lettering rebuilt the message from the deserialized value, which a handler may have changed. It now forwards the original body, keeping the payload, GroupId, CorrelationId and properties exactly. The worker loop logs SDK timeouts, waits at least one second after a dequeue error, and does not abandon during shutdown. The metadata test no longer asserts on missing config and deletes the queue it creates. Legacy mode now runs the reused-options test. * Limit queue changes to GroupId; restore main worker loop and dead-letter envelope * Use matching GroupId core and TestHarness preview * Accept groupIdResolver in GetQueue, run base resolver tests (legacy asserts GroupId is not persisted), use core preview .0.21 * Update Foundatio to 13.0.5-preview.0.10 and point build workflow back to main * Remove archived Minio provider from README * Include persisted CorrelationId and Properties on the Enqueued entry in Default mode
…y, fast emulator drain (#103) * Map GroupId to SessionId and preserve retry message fields Depends on FoundatioFx/Foundatio#579 * Register empty GroupId and isolate session guard tests * Pin guarded build workflow for feature branch validation * Stop claiming GroupId affects delivery Service Bus ignores SessionId unless the queue requires sessions, and this provider cannot receive from session-enabled queues, so GroupId cannot change delivery. Remove SupportsGroupId so the base queue logs that, and add a regression test. Also consume the queue-group-id nightly of Foundatio and Foundatio.TestHarness until a release is available. * Schedule the delayed retry before completing the original message A failed send after completing the original lost the message. The retry is now sent first, and gets a new MessageId when duplicate detection is on so it is not dropped as a duplicate. The worker loop now logs SDK timeouts and waits at least one second after a dequeue error. Adds the delayed-retry GroupId test. * Limit retry changes to SessionId and send-before-complete; restore main worker loop * Use matching GroupId core and TestHarness preview * Add session receive for session-enabled queues; make emulator drain state-driven and settle in-flight entries on delete Drain peeks the queue instead of sleeping for 5s per call, waits only for a peeked scheduled message's due time (the emulator ignores CancelScheduledMessagesAsync), and DeleteQueueAsync completes messages this instance dequeued but never settled so their locks cannot leak into later tests. Emulator suite: ~19 min with cross-test failures -> 39s green. * Update Foundatio to 13.0.5-preview.0.10 and point build workflow back to main * Keep session receiver until settlement succeeds and untrack cancelled session entries * Address review feedback: pattern matching, shared stats/untrack helpers, drain scan type, worker error backoff, TimeProvider for schedules * Use pattern matching for failure reason checks * Harden sessions: keep group order on abandon, validate GroupId before send, release expired unsettled entries, share accept/receive deadline, avoid retry copies with duplicate detection, count copy redeliveries * Detect duplicate detection on existing queues, compare-and-remove tracked entries, grace period before releasing expired locks, document RetryDelay * Refresh duplicate-detection state on create and delete, abandon entries that fail to settle during delete, let Enqueuing handlers assign a missing session GroupId * Include the exception message in unsettled-message log entries * Add missing blank lines between members * Add blank line after stale session receiver disposal
* Add SQS GroupId, FIFO and dead-letter fixes Depends on FoundatioFx/Foundatio#579 * Retain SQS messages on dead-letter lookup failure * Pin guarded build workflow for feature branch validation * Keep the SQS worker alive when dequeue or abandon fails A transient AWS error or AccessDenied during a dead-letter lookup escaped the worker loop and permanently stopped it. Log and count the error, wait one dequeue interval, and keep working; the message becomes visible again after its visibility timeout. Also consume the queue-group-id nightly of Foundatio and Foundatio.TestHarness until a release is available. * Validate SQS options again after Enqueuing handlers and back off on worker errors Handlers can change GroupId, so the SQS limits and the FIFO GroupId requirement are checked again before sending and surface as QueueException/ArgumentException instead of a raw AWS error. The worker loop only treats cancellation as a stop signal when its own token is cancelled, so SDK timeouts are logged and counted, and waits at least one second after a dequeue error so it cannot spin hot. * Delay worker dequeue errors by DequeueInterval and keep cancellation handling from main * Use matching GroupId core and TestHarness preview * Accept groupIdResolver in GetQueue, run base resolver tests, use core preview .0.21 * Allow zero DeliveryDelay on FIFO queues and skip DelaySeconds for FIFO * Update Foundatio to 13.0.5-preview.0.10 and point build workflow back to main * Back off at least one second after dequeue errors in the SQS worker * Address review feedback: DLQ resolution and name validation, SQS id/attribute validation, dequeue error backoff, emulator tests replace synthetic tests; remove archived Minio from README * Remove unused HttpClientFactory option (only needed by removed synthetic tests); use pattern matching for null/equality checks * Share queue stats construction and simplify dead-letter move * Harden SQS dead lettering: use queue only after redrive setup, fall back to created DLQ during policy propagation, DLQ inherits encryption, delete conventional DLQ, clamp retry visibility to 12h, clear queue url on delete * Resume partial queue setup, reuse existing DLQ with different attributes, limit redrive fallback to propagation window, delete source queue on unused instance, worker re-ensures queue * Capture DLQ propagation fallback before the policy read, honor content-based deduplication on existing FIFO queues, forget a deleted queue's cached url * Refresh FIFO content-based deduplication periodically and retry once with a generated id if SQS rejects a send after it is turned off * Read content-based deduplication on the first implicit-id send of a created queue and forget the queue url when the refresh finds it deleted * Add missing blank lines between members * Add blank lines after braceless if bodies in tests * Inline the dequeue error backoff delay instead of adding a SafeDelay extension * Share the CorrelationId attribute name constant and assert group id is not exposed as a property * Apply suggestion from @niemyjski * Extract FIFO deduplication into a helper, tolerate a failed dedup refresh, log queue deletes, and restore the attribute TODO and deadRequest * Resolve ContentBasedDeduplication once instead of polling, require the propagation fallback with caller-read attributes, warn when an exhausted message is discarded, and move new private helpers to the bottom * Settle ContentBasedDeduplication from the next send when it can't be read, and forget a deleted queue's url on stats and the dedup retry * Simplify the CorrelationId property check and fix the delivery-delay test race with the long poll * Only delete a dead letter queue the source policy confirms, mark setup pending before creating the source, forget a deleted queue on redrive lookup, and drop GroupId from trace logs * Update Microsoft.NET.Test.Sdk, xunit.v3.mtp-v2, and xunit.runner.visualstudio package versions * Revalidate resolved dead letter ownership on delete and tolerate missing attribute access Only a dead letter queue this instance created is deleted without checking the source's current redrive policy; one resolved from the policy is re-read before deletion. A failed policy read (e.g. AccessDenied) now skips dead letter cleanup instead of blocking deletion of the source queue. * Upgraded Foundatio to 13.0.5-preview.0.16 * Keep a confirmed dead letter queue url so a retried delete still removes it
1 participant
What this adds
A provider-neutral way to tag queued messages with a group, such as a tenant, customer, or order id:
The value comes back on
IQueueEntry.GroupId. What each provider does with it:GroupIddoesMessageGroupId, which enables SQS fair queues (noisy-neighbor protection). No ordering.fifo)MessageGroupIdfor per-group orderingSessionId. On session-enabled queues (RequiresSession), ordered per group, one message at a time per groupBehavior details
GroupIdwins over the resolver. An empty value is stored asnulland invokes the resolver. A resolver should returnnullfor no group; an empty string is treated the same.Enqueuinghandlers, so handlers see the final value.GroupIdis kept when a message is retried after abandon. Queue job activities include aGroupIdtag.GroupIdfor delivery log one debug message the first time one is enqueued (protected virtual bool SupportsGroupId).Behavior change:
EnqueueAsynccopies the optionsBefore this PR,
EnqueueAsyncmutated the caller'sQueueEntryOptions: it setCorrelationIdand calledProperties.Add("TraceState"). As a result:ActivitywithTraceStatewas active.Propertiesthrew.GroupIdwritten back to the caller's options would stick for every later message.Now every enqueue works on a copy (
with, so derived records such asAzureServiceBusQueueEntryOptionskeep their type), and theActivitydefaults are applied beforeEnqueueImplAsync.OnEnqueuingAsyncoverrides andEnqueuinghandlers see the same values as before. Only external code that relied on its own options instance being mutated will notice.Propertiesis copied into a writableDictionary<string, string>; aDictionarykey comparer is preserved. A caller-suppliedTraceStateproperty now wins (TryAdd).Breaking change:
QueueTestBase.GetQueueGetQueuegains a last parameter,Func<SimpleWorkItem, string?>? groupIdResolver = null. Provider test classes that override it must add the parameter and apply it (if (groupIdResolver is not null) o.GroupId(groupIdResolver);). The resolver tests are now virtual base tests, so every provider runs them.API additions
QueueEntryOptions.GroupIdIQueueEntry.GroupId(default interface member that returnsnull, so existing implementations still compile)QueueEntry.GroupId { get; init; }SharedQueueOptions.GroupIdResolverand theGroupId(...)builder methodprotected virtual bool SupportsGroupIdonQueueBaseDocs
queues.md: Message Groups section.provider-behavioral-gaps.md(now in the sidebar): GroupId, ordering, delivery delay and retry-delay tables.aws.md: fair queues, FIFO dead-letter naming, SQS delay clamping and FIFO delay rejection.azure.md:RequiresSession.cross-repo-validationagent skill, which builds and tests the provider repos against local core.Tests
Related PRs
The provider PRs consume the matching core/TestHarness preview
13.0.5-preview.queue-group-id.0.21: