Conversation
niemyjski
changed the title
1ab2804 to
37db1e7
Compare
37db1e7 to
6d5694b
Compare
niemyjski
changed the title
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. |
niemyjski
changed the title
![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: 5cd0b5df37
ℹ️ 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".
| { | ||
| using var cleanup = new CancellationTokenSource(TimeSpan.FromSeconds(10)); | ||
| await admin.QueueDeleteAsync(queueName, cancellationToken: cleanup.Token); | ||
| await admin.ExchangeDeleteAsync(topic, cancellationToken: cleanup.Token); |
There was a problem hiding this comment.
When RabbitMQ infrastructure is available, every CreateBus() still uses the default SharedMessageBusOptions.Topic because the local topic is never passed to the builder, so the named exchange is never declared. ExchangeDeleteAsync(topic) therefore attempts to delete a nonexistent exchange and raises a channel-level not-found exception from the finally block, causing an otherwise successful priority test to fail; add .Topic(topic) to CreateBus() or clean up the exchange actually configured.
Useful? React with 👍 / 👎.
ce9c3fe to
aeaf61a
Compare
aeaf61a to
73064e3
Compare
niemyjski
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
UseMessagePriority()configures a classic queue's maximum priority. Combining it with an explicitly configured quorum queue now fails locally, before connecting. Per-message priorities remain supported in both queue modes.Breaking change: remove
UseMessagePriority()orMaxPriorityfrom quorum configurations. Builder calls reject the combination in either order; direct options reject it when the bus is constructed. SettingMaxPriority = 0throws immediately. Classic direct limits of 1–255 and the builder's existing 1–32 range remain supported.Broker upgrade impact: RabbitMQ 4.3 changes quorum ordering. Priorities 5 and 10 previously shared the high-priority group; they now order separately. Lower priorities lose their guaranteed share and can starve under sustained higher-priority traffic. An omitted priority defaults to 4; RabbitMQ.Client 7.2.2 also omits zero on the wire. Classic priority ordering and already-delivered messages are unaffected by these quorum scheduling changes.
Validation: Release build and formatting passed. Nine new, separate routing and priority cases passed without skips on RabbitMQ 3.13.7, 4.2.5, and 4.3.6, at both this PR and the final aggregate. Both updated-head Linux checks passed. The push run passed 145 tests with 122 infrastructure skips; the local matrix supplies broker evidence for the nine new skipped cases.
Verification and implementation details
The version matrix compares fresh brokers. A rolling upgrade with existing queue data was not exercised.
The untouched pre-PR
PublishAsync_WithPriority_DeliversHighPriorityFirstwas run against both 4.2.5 and 4.3.6. Both reject its quorum queue with406 PRECONDITION_FAILEDbecauseUseMessagePriority()suppliesx-max-priority. Its old green CI run skipped all five inherited copies for missing infrastructure. An earlier publish-before-subscribe reproduction removed the invalid option; it was a modified variant, not the untouched test.The existing method was rewritten earlier in this PR to subscribe first and hold a warm-up delivery while confirmed messages accumulate. It retains its four strict ordering assertions and skips before 4.3. This latest addition does not edit existing methods again: independent tests cover publication before subscription, in-flight delivery, omitted/zero priority, queued priority levels, and quorum fairness. The new cases run on older brokers with their actual expectations, rather than skipping those versions. A shared helper serves three queued-message scenarios using one bus, fluent configuration, and base cleanup. No raw channels or topology declarations are added.
The warm-up gate fills the single prefetch slot so the broker compares queued messages. Without a backlog, a lower-priority message may already be delivered before higher-priority publication. It belongs in the RabbitMQ provider tests: the shared message-bus contract does not promise priority scheduling, and an in-memory publisher that awaits handlers could deadlock on this arrangement. Base cleanup disposes the bus; persistent quorum queues are removed when the owned broker is torn down.
Seven option tests cover both builder orders, direct options, dictionary mutation before construction, classic limits through 255, and immediate rejection of zero. The
fieldsetter validates the scalar value; construction validates the cross-option combination.IsQuorumQueuechecks an explicit local string argument, not broker/vhost defaults or existing queue metadata. Post-construction mutation checks belong to #105.Broker references: 4.2 priority groups, 4.3 priority scheduling. The local results cover priority and routing only, not all 4.3 features. #104 adds owned 4.2.5 and 4.3.6 priority runs to CI. Merge before #104, then #105 and #100.