Skip to content

Fix data race in quota group routing by cloning auth in selector - #20

Merged
nghyane merged 3 commits into
mainfrom
copilot/check-routing-logic-issues
Jan 3, 2026
Merged

nghyane merged 3 commits into
mainfrom
copilot/check-routing-logic-issues

Conversation

Copilot AI commented Jan 3, 2026 •

Copy link
Copy Markdown
Contributor

Race detector revealed concurrent access to auth.Runtime during quota group routing. Manager passed raw auth pointers to selector while MarkResult concurrently modified quota state.

Changes

  • manager.go:671 - Clone auth objects before passing to selector

    // Before: selector receives pointer to mutable auth
    candidates = append(candidates, candidate)
    
    // After: selector receives immutable snapshot
    candidates = append(candidates, candidate.Clone())
  • quota_group.go - Document that getOrCreateQuotaGroupIndex requires Manager lock (no code change)

  • quota_group_race_test.go - Add race detection tests simulating production patterns:

    • TestQuotaGroupIndexManagerProtection - Verifies proper RLock/Lock usage
    • TestConcurrentQuotaPropagation - Tests parallel quota updates
    • TestQuotaGroupIndexConsistency - Validates quota index synchronization
    • TestSelectorWithQuotaGroups - End-to-end concurrent workflow

Impact

Minimal performance overhead (shallow clone + map copies). Eliminates potential crashes and incorrect routing decisions under concurrent load.

All tests pass with -race flag enabled.

Original prompt

Kiểm tra lại cơ chế routing, usage rercod xem có đang gặp vấn đề về logic hoặc gap hay xung đột không


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI self-assigned this Jan 3, 2026
Copilot AI and others added 2 commits January 3, 2026 06:23
Co-authored-by: nghyane <59473462+nghyane@users.noreply.github.com>
Co-authored-by: nghyane <59473462+nghyane@users.noreply.github.com>
Copilot AI changed the title [WIP] Review routing mechanism for logic issues Fix data race in quota group routing by cloning auth in selector Jan 3, 2026
Copilot AI requested a review from nghyane January 3, 2026 06:30
@nghyane

nghyane commented Jan 3, 2026

Copy link
Copy Markdown
Owner

@copilot về cơ chế routing thì sao đã kiểm tra chưa?

Copilot AI commented Jan 3, 2026

Copy link
Copy Markdown
Contributor Author

@copilot về cơ chế routing thì sao đã kiểm tra chưa?

Đã kiểm tra đầy đủ cơ chế routing, bao gồm:

1. Provider Selection (selectProviders)

  • ✅ Circuit breaker filtering hoạt động đúng
  • ✅ Performance-based scoring cho multi-provider
  • ✅ Fallback khi tất cả breakers open

2. Auth Selection (pickNext → selector.Pick)

  • ✅ RoundRobinSelector: Round-robin + sticky sessions (60s TTL)
  • ✅ QuotaAwareSelector: Quota-aware + request counting
  • ✅ isAuthBlockedForModel: Kiểm tra cooldown, quota, disabled state
  • ✅ Quota group index: O(1) lookup cho models chưa init

3. Retry & Failover

  • ✅ Retry logic với retry budget (max 100 concurrent)
  • ✅ waitForAvailableAuth khi quota exceeded
  • ✅ shouldRetryAfterError xử lý user vs provider errors

4. Quota Group Propagation

  • ✅ propagateQuotaToGroup: Update ModelStates + quota index
  • ✅ clearQuotaGroupOnSuccess: Clear cả hai khi recover
  • ✅ Đồng bộ giữa quota index và ModelStates

Data race đã fix (commit 56e8078):

  • manager.go:671 - Clone auth trước khi pass cho selector
  • Loại bỏ concurrent read/write vào auth.Runtime

Tất cả tests pass với -race flag.

@nghyane
nghyane marked this pull request as ready for review January 3, 2026 06:45
Copilot AI review requested due to automatic review settings January 3, 2026 06:45
@nghyane
nghyane merged commit 90375ed into main Jan 3, 2026
5 checks passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a data race in quota group routing where concurrent access to auth.Runtime occurred between MarkResult operations (which modify quota state) and selector operations (which read quota state). The fix ensures thread-safety by cloning auth objects before passing them to selectors.

Key changes:

  • Clone auth objects in manager's pickNext method to provide selectors with immutable snapshots
  • Add comprehensive race detection tests covering concurrent quota operations
  • Document locking requirements for quota group index operations

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
internal/provider/manager.go Clones auth objects under RLock before passing to selector, preventing race on Runtime pointer
internal/provider/quota_group.go Documents that getOrCreateQuotaGroupIndex requires Manager lock protection
internal/provider/quota_group_race_test.go Adds 4 comprehensive race tests validating Manager lock protection, concurrent quota propagation, quota group consistency, and end-to-end selector workflow

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@nghyane

nghyane commented Jan 3, 2026

Copy link
Copy Markdown
Owner

@copilot Phân tích so sánh với các best practice roubin trong go, các repo api tương tự

@nghyane
nghyane deleted the copilot/check-routing-logic-issues branch January 6, 2026 02:39
nghyane added a commit that referenced this pull request Jan 8, 2026
* Initial plan

* Add race detection test exposing quota group index data race

Co-authored-by: nghyane <59473462+nghyane@users.noreply.github.com>

* Fix race condition in quota group routing by cloning auth in pickNext

Co-authored-by: nghyane <59473462+nghyane@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: nghyane <59473462+nghyane@users.noreply.github.com>
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.

3 participants