fix(routing): avoid deadlock when exclusion log throttle expires

[ormandj]

When a routing target is excluded repeatedly, the first exclusion after its 60-second log throttle expires can block the request worker indefinitely before provider dispatch. In should_log_exclusion, the DashMap::get() guard used as the match scrutinee remains alive in the fallback arm, where insert() attempts to acquire the same shard's write lock. This also prevents the request from reaching the upstream timeout and can accumulate blocked workers during an outage.

Use DashMap::entry() to check and update the timestamp under one guard. This removes the lock upgrade and ensures concurrent callers emit at most one exclusion log per group/model/reason per interval. Routing order, cooldown settings, and public interfaces remain unchanged.

Validation:

• The expiry regression reproduces the original deadlock and fails through a separate two-second watchdog instead of hanging the test suite.
• Concurrent fresh-entry and expired-entry tests require exactly one caller to pass the throttle. A further regression verifies suppressed calls do not refresh the timestamp and postpone logging indefinitely; deliberately introducing that defect makes the test fail.
• All 1,209 proxy unit tests pass on upstream main, including 35 health tests and 76 routing tests. All 33 health tests and 68 routing tests also pass against v1.1.0 with the same fix.
• cargo fmt --all -- --check and git diff --check pass.

Summary by CodeRabbit

• Bug Fixes

  • Improved rate limiting for health-related logging to prevent race conditions and deadlocks.
  • Ensured suppressed log attempts do not extend the throttle window.
  • Corrected throttling behavior so the gate reopens after the configured interval.
  • Limited concurrent logging to one permitted writer per interval.

• Tests

  • Added coverage for throttle expiration, suppressed calls, and concurrent access.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4b1dbe57-a01f-4f2c-a3cf-d116f72016c4

📥 Commits

Reviewing files that changed from the base of the PR and between 28b9e97 and bdb8aad.

📒 Files selected for processing (1)

• crates/aisix-proxy/src/health.rs

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

should_log_exclusion now updates the DashMap entry atomically. Tests cover timestamp preservation, interval reopening, deadlock detection, and concurrent writer exclusivity.

Changes

Exclusion log throttling

Layer / File(s)	Summary
Atomic gate and concurrency validation crates/aisix-proxy/src/health.rs	should_log_exclusion uses one entry() guard to check and update timestamps. Tests verify that suppressed calls do not extend the window, the gate reopens after expiry, and only one concurrent writer succeeds per window.

Priority: ➖ Normal — Schedule this routing deadlock fix because an expired exclusion-log throttle can block request workers before provider dispatch.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to bdb8a

Exclusion-log throttling now updates timestamps atomically, preventing request-worker stalls when an exclusion interval expires. The change preserves the throttle behavior and is ready to merge.

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
E2e Test Quality Review	⚠️ Warning	Blocking issue: the added regression tests are unit tests in crates/aisix-proxy/src/health.rs. They call should_log_exclusion directly and do not exercise the request API, routing, or provider dis…	Add an E2E test that sends requests through the proxy with a cooled routing target and a surviving provider. Cover the post-throttle-expiry request path with a watchdog, assert that the request completes and reaches the surviving provider, …

✅ Passed checks (5 passed)

Check name	Status	Explanation
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.
Security Check	✅ Passed	No security vulnerability from this pull request. The complete diff from base changes only crates/aisix-proxy/src/health.rs: it replaces a DashMap::get()/insert() lock pattern with one entry()…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing a routing deadlock when the exclusion-log throttle expires.

Full details: E2e Test Quality Review

Explanation

Blocking issue: the added regression tests are unit tests in crates/aisix-proxy/src/health.rs. They call should_log_exclusion directly and do not exercise the request API, routing, or provider dispatch. The existing E2E cooldown test covers a fresh exclusion, but it does not cover the expired 60-second gate or concurrent requests. Therefore the deadlock fix is not validated through the full business flow.

Resolution

Add an E2E test that sends requests through the proxy with a cooled routing target and a surviving provider. Cover the post-throttle-expiry request path with a watchdog, assert that the request completes and reaches the surviving provider, and verify the expected single exclusion log under concurrent requests. Use a test-controlled interval or clock if needed to avoid a 60-second test delay.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use path_filters to narrow the review scope.

---

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

[ormandj]

@jarvis9443 @nic-6443 — could you review this routing deadlock fix? Repeated target exclusions can permanently block request workers once the 60-second logging throttle expires, before provider dispatch or the upstream timeout. The regression reproduces the original deadlock, the fix and concurrent/suppression tests pass, and all 1,209 proxy unit tests pass.

The CLA clarification questions on #1047 also remain open. Thanks!

[jarvis9443]

Verified independently, approving.

On main, should_log_exclusion keeps the DashMap::get() read guard alive as the match scrutinee temporary into the fallback arm that calls insert() on the same shard; dashmap 6.1.0's shard lock is a non-reentrant RwLock, so the first exclusion after the 60s window expires parks that worker forever, and every later write to the same shard parks behind it. Applying only your tests onto main's implementation reproduces it (the expiry test times out, the concurrency test sees 3 writers instead of 1); both pass on this branch. I also swept every DashMap/DashSet use in the workspace and this was the only get-then-mutate-under-guard site, so nothing else needs the same treatment.

The red e2e (work-stealing) leg failed on metric-label-selection-e2e startup validation with an assertion text identical to a failure on an unrelated branch yesterday, i.e. a known flake; re-running it.

[jarvis9443]

Merged as 7d6d14b — thanks for the thorough report and the fix; it will ship in the next release.

[ormandj]

Merged as 7d6d14b — thanks for the thorough report and the fix; it will ship in the next release.

Anytime, I look forward to contributing more over time! Thank you for creating this project and all of your hard work!