Skip to content

fix: run delayed retries, manual creates and aggregation in the subscription's work group - #326

Merged
hamzahalq merged 1 commit into
releases/r10.0from
hamza/fix/work-group-routing
Sep 27, 2026
Merged

hamzahalq merged 1 commit into
releases/r10.0from
hamza/fix/work-group-routing

Conversation

@hamzahalq

Copy link
Copy Markdown
Contributor

Delayed retries, manual "create exchange" and aggregation loaded the subscription without its work group. So those exchanges ran in the ungrouped queue, with no error, even when the subscription had a work group. Their results still went to the right group's result queue.

These three paths now load the work group the same way normal runs and manual retries already do. Subscriptions without a work group still go to ungrouped, as before.

Tests: WorkGroupRoutingTests covers each path on a fresh DbContext. All three got 0Ungrouped before the fix and pass now, and the related retry and aggregation test classes (84 tests) pass.

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: simplify9/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: d4eff42d-450d-475f-af49-8abd2b2a9638

📥 Commits

Reviewing files that changed from the base of the PR and between c37f633 and 9e623a8.

📒 Files selected for processing (4)
  • SW.Bitween.Api/Resources/Xchanges/Create.cs
  • SW.Bitween.Api/Services/AggregationJob.cs
  • SW.Bitween.Api/Services/XchangeService.cs
  • SW.Bitween.IntegrationTests/Tests/WorkGroupRoutingTests.cs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: vuln-gate / check

📝 Summary

Summary

  • Changed delayed retries, manual subscription-based creation, and aggregation to load subscriptions through Subscriptions() instead of Set<Subscription>(). This routes those operations through the subscription’s work group, consistent with normal runs and manual retries.
  • Added integration tests for all three paths. The PR description reports that 84 tests in related retry and aggregation test classes pass.

Risk

risk:low. The changes are limited to subscription queries and routing coverage.

Security-sensitive areas

No authentication, authorization, or sensitive-data handling changes are shown.

Deployment and operations

No schema or configuration changes are shown. No migration or rollback steps are indicated.

Walkthrough

Three subscription lookups now use Subscriptions(). New integration tests exercise delayed retries, manual creation, and aggregation with work-group subscriptions, and check the published exchange queue name.

Changes

Work-group routing

Layer / File(s) Summary
Subscription lookup updates
SW.Bitween.Api/Resources/Xchanges/Create.cs, SW.Bitween.Api/Services/AggregationJob.cs, SW.Bitween.Api/Services/XchangeService.cs
Manual creation, aggregation, and delayed retry now retrieve subscriptions through Subscriptions() instead of Set<Subscription>().
Work-group routing coverage
SW.Bitween.IntegrationTests/Tests/WorkGroupRoutingTests.cs
Test helpers capture queue names for matching exchanges. Integration tests cover delayed retry, manual creation, and aggregation, and assert that each records the subscription’s work-group queue name.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested labels: database, testing, risk:high

Suggested reviewers: mmalkhatib

Merge Risk: ⚪ Minimal · up to 9e623

The subscription lookups and routing tests show no identified issue requiring a fix before merge.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 9e623

Routing now follows each subscription’s work group rather than the ungrouped queue. The changed lookups preserve subscription selection, but authorization for manual creation and the isolation properties of work-group workers remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The routing change applies to exchanges created through the three affected paths for subscriptions with work groups. The evidence does not establish whether those queues use distinct credentials, tenant boundaries, or worker privileges.

Trust Boundaries and Controls

  • observed — The work-group queue name comes from the loaded subscription’s WorkGroup through the exchange event, rather than from a work-group value supplied in the manual-create request. Effective authorization of that request remains unverified.

Hardening Proposals

  • proposed — Verify the manual-create route’s authorization and subscription-ownership checks, and confirm that work-group consumers have the intended isolation and privileges before relying on queue placement as a security boundary.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the three affected execution paths and the work-group routing fix.
Description check ✅ Passed The description directly explains the routing defect, the implemented fix, and the test coverage.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@hamzahalq
hamzahalq merged commit 971350f into releases/r10.0 Sep 27, 2026
5 checks passed
@hamzahalq
hamzahalq deleted the hamza/fix/work-group-routing branch September 27, 2026 08:34
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.

2 participants