Skip to content

Fix/shared health check polling - #26434

Merged
oss-pr-review-agent-shin[bot] merged 3 commits into
BerriAI:shin_agent_oss_staging_05_09_2026from
Evernorth:fix/shared-health-check-polling
May 9, 2026
Merged

Fix/shared health check polling#26434
oss-pr-review-agent-shin[bot] merged 3 commits into
BerriAI:shin_agent_oss_staging_05_09_2026from
Evernorth:fix/shared-health-check-polling

Conversation

@noahnistler

@noahnistler noahnistler commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Relevant issues

Fixes shared health check coordination failing in multi-pod (e.g. ECS) deployments — every pod runs redundant health checks instead of sharing results.

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have Added testing in the tests/test_litellm/ directory, Adding at least 1 test is a hard requirement - see details
  • My PR passes all unit tests on make test-unit
  • My PR's scope is as isolated as possible, it only solves 1 specific problem
  • I have requested a Greptile review by commenting @greptileai and received a Confidence Score of at least 4/5 before requesting a maintainer review

Delays in PR merge?

If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).

CI (LiteLLM team)

CI status guideline:

  • 50-55 passing tests: main is stable with minor issues.
  • 45-49 passing tests: acceptable but needs attention
  • <= 40 passing tests: unstable; be careful with your merges and assess the risk.
  • Branch creation CI run
    Link:

  • CI run for the last commit
    Link:

  • Merge / cherry-pick CI run
    Links:

Screenshots / Proof of Fix

This fix changes Redis-coordinated polling logic between pods — it cannot be reproduced with a single-instance deployment. Proof is provided via unit tests that simulate the multi-pod coordination scenarios with mocked Redis.

image

Scenario coverage

Scenario Test Behavior
Cache hit (no work needed) test_perform_shared_health_check_with_cache Returns cached results, perform_health_check never called
Lock acquired (this pod runs check) test_perform_shared_health_check_with_lock_acquisition Runs health check, caches results, releases lock
Lock held, poll finds cache test_perform_shared_health_check_lock_failed_then_cache Polls at 5s interval, picks up cached results on first poll
Lock held, poll exhausted test_perform_shared_health_check_fallback Polls until lock_ttl exceeded, falls back to local health check
Lock disappears (holder crashed) test_perform_shared_health_check_early_exit_orphaned_lock Detects orphaned lock, exits poll early, falls back
Redis error during polling test_perform_shared_health_check_redis_error_during_polling Survives transient Redis error, continues polling, finds cache next iteration
No Redis configured test_perform_shared_health_check_no_redis_skips_polling Skips polling loop entirely, runs local health check immediately

What changed (before → after)

  • Before: Non-lock-holder waited a fixed 2s sleep, then always fell back to a redundant local health check — defeating the purpose of the shared lock
  • After: Non-lock-holder polls Redis at 5s intervals (up to lock_ttl) for cached results, with early exit if the lock disappears (crash recovery), and only falls back to local check if polling is exhausted

Type

🐛 Bug Fix

Changes

The SharedHealthCheckManager.perform_shared_health_check() non-leader branch only waited 2 seconds before falling back to a local health check. Since real health checks against multiple models typically take longer than 2s, the cache was almost never ready in time — causing every non-leader pod to run its own redundant health check and defeating the shared coordination.

Fix

Replace the single asyncio.sleep(2) with a polling loop:

  • Polls every 5s for cached results, up to lock_ttl (default 60s)
  • Exits early if the lock disappears without a cache write (crash recovery)
  • Defensive try/except around Redis calls during polling
  • Skips polling entirely when redis_cache is None (no 60s regression)
  • Falls back to local health check only after exhausting the wait

Not a breaking change

Same method signature, same return type, same external behavior.

The shared health check manager's non-leader branch only waited 2
seconds before falling back to a local health check. Since real health
checks against multiple models typically take longer, every non-leader
pod would fall through and run redundant checks — defeating the
shared coordination.

Replace the single 2-second sleep with a polling loop (5s interval,
up to lock_ttl) that gives the lock holder realistic time to finish.
Adds early exit when the lock disappears without a cache write (crash
recovery) and defensive error handling for Redis hiccups during
polling.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@CLAassistant

CLAassistant commented Apr 24, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 4 committers have signed the CLA.

✅ yuneng-berri
✅ noahnistler
❌ shin-berri
❌ mateo-berri
You have signed the CLA already but the status is still pending? Let us recheck it.

@veria-ai

veria-ai Bot commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Low: No security issues found

This PR changes the health check polling strategy from a single 2-second sleep to a bounded polling loop (up to lock_ttl seconds). The changes are purely internal infrastructure coordination between pods using Redis, with no user-facing input, no auth changes, and no sensitive data handling.


Status: 0 open
Risk: 1/10

Posted by Veria AI · 2026-04-24T15:14:38.100Z

@greptile-apps

greptile-apps Bot commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes shared health-check coordination in multi-pod deployments by replacing a single asyncio.sleep(2) with a polling loop that waits up to lock_ttl seconds (default 60 s, polling every 5 s) before falling back to a local health check. It also adds a fast-path for when redis_cache is None, an early-exit when the lock disappears without a cache write (crash recovery), and swallows transient Redis errors during polling to avoid crashing the loop.

Confidence Score: 5/5

Safe to merge; only a P2 style finding present, logic is correct and well-tested.

All findings are P2 (style/clarity). The core logic is correct: the redis_cache is None guard prevents the 60-second regression, the polling loop correctly bounds total wait to lock_ttl, and the early-exit on orphaned lock is sound. Four new mock-only tests cover the added paths comprehensively.

No files require special attention.

Important Files Changed

Filename Overview
litellm/proxy/health_check_utils/shared_health_check_manager.py Replaces single asyncio.sleep(2) with a proper polling loop (up to lock_ttl seconds, 5 s intervals); adds early-exit for crashed lock holders; guards redis_cache is None before entering the loop; handles transient Redis errors during polling gracefully.
tests/test_litellm/proxy/test_shared_health_check.py Adds four new mock-only tests covering: polling fallback exhaustion, early-exit on orphaned lock, Redis error resilience during polling, and no-polling path when redis_cache is None; updates existing tests to reflect 5 s poll interval. All tests are mock-based with no real network calls.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[perform_shared_health_check called] --> B{Cached results\navailable?}
    B -- Yes --> C[Return cached results]
    B -- No --> D[Try to acquire Redis lock]
    D -- Acquired --> E[Run perform_health_check]
    E --> F[Cache results in Redis]
    F --> G[Release lock]
    G --> H[Return results]
    D -- Not acquired --> I{redis_cache\nis None?}
    I -- Yes --> J[Run local health check immediately]
    I -- No --> K[Enter polling loop\nelapsed = 0]
    K --> L{elapsed < lock_ttl?}
    L -- No --> M[Log warning: exhausted wait]
    M --> J
    L -- Yes --> N[asyncio.sleep 5s\nelapsed += 5]
    N --> O{Cached results\navailable?}
    O -- Yes --> C
    O -- No --> P{Lock still held?\nasync_get_cache lock_key}
    P -- Redis error --> K
    P -- Still held --> K
    P -- Lock gone\ncurrent_owner is None --> Q[Break: orphaned lock detected]
    Q --> M
Loading

Reviews (3): Last reviewed commit: "fix: skip polling loop when redis_cache ..." | Re-trigger Greptile

Comment thread litellm/proxy/health_check_utils/shared_health_check_manager.py
Comment thread litellm/proxy/health_check_utils/shared_health_check_manager.py
@codecov

codecov Bot commented Apr 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Update two existing tests to match the new 5s polling interval
(previously asserted sleep(2)). Add two new tests:
- Early exit when lock disappears without cache (crash recovery)
- Redis error resilience during polling loop
When Redis is not configured, the polling loop would sleep for the
full lock_ttl (60s) with no chance of finding cached results. Add an
early return to fall back to local health check immediately.

Addresses Greptile review feedback on BerriAI#26434.
@noahnistler

Copy link
Copy Markdown
Contributor Author

@greptileai

@krrish-berri-2

Copy link
Copy Markdown
Contributor

@noahnistler — could you add a screenshot or short video showing that this change works as expected? It really helps reviewers verify the fix quickly. Thanks!

@noahnistler

Copy link
Copy Markdown
Contributor Author

@noahnistler — could you add a screenshot or short video showing that this change works as expected? It really helps reviewers verify the fix quickly. Thanks!

@krrish-berri-2 I Added test output screenshot and a scenario coverage table showing all 7 polling paths tested (cache hit, lock acquired, poll succeeds, poll exhausted, orphaned lock/crash recovery, Redis error during polling, no Redis configured). 31/31 tests pass.

@oss-pr-review-agent-shin
oss-pr-review-agent-shin Bot changed the base branch from litellm_internal_staging to shin_agent_oss_staging_05_09_2026 May 9, 2026 20:14
@oss-pr-review-agent-shin
oss-pr-review-agent-shin Bot merged commit 2b4beae into BerriAI:shin_agent_oss_staging_05_09_2026 May 9, 2026
43 checks passed
@oss-pr-review-agent-shin

Copy link
Copy Markdown
Contributor

🤖 litellm-agent: Squash-merged into staging branch shin_agent_oss_staging_05_09_2026. Staging PR: #27549


Triage Summary
Replaces the single asyncio.sleep(2) fallback in SharedHealthCheckManager.perform_shared_health_check() with a bounded polling loop that checks Redis for cached results every 5 seconds up to lock_ttl (default 60s). Adds early exit when the lock disappears without a cache write (crash recovery), guards against the polling loop when redis_cache is None, and swallows transient Redis errors during polling. Touches shared_health_check_manager.py and adds 131 lines of mock-based tests in test_shared_health_check.py covering seven coordination scenarios.

Merge Confidence: 5/5 ✅ READY
Ready to ship.

All checks green. Greptile 5/5, no blocking pattern findings, no CircleCI runs (OSS-typical).

fzowl pushed a commit to fzowl/litellm that referenced this pull request Jun 24, 2026
Squash-merged by litellm-agent from noahnistler's PR.
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.

4 participants