LCORE-1599: Add retry with backoff to check_llama_stack_version#1409
Merged
tisnik merged 1 commit intoMar 27, 2026
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (startup)
participant Checker as check_llama_stack_version
participant Client as AsyncLlamaStackClient
participant Verifier as compare_versions
participant Async as asyncio.sleep
App->>Checker: call check_llama_stack_version(...)
loop until success or attempts exhausted
Checker->>Client: await inspect.version()
alt version returned
Client-->>Checker: VersionInfo
Checker->>Verifier: compare_versions(version, min, max)
Verifier-->>Checker: ok / raises
Checker-->>App: return
else APIConnectionError
Client-->>Checker: raises APIConnectionError
Checker->>App: log warning (attempt N)
Checker->>Async: await sleep(retry_delay)
Async-->>Checker: resume
end
end
alt exhausted
Checker-->>App: re-raise APIConnectionError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/utils/test_llama_stack_version.py (1)
121-139: Add the exhausted-retries test path.This test covers eventual success, but not the required behavior that
APIConnectionErroris re-raised after all retries are exhausted.Suggested companion test
+@pytest.mark.asyncio +async def test_check_llama_stack_version_raises_after_max_retries( + mocker: MockerFixture, +) -> None: + mock_client = mocker.AsyncMock() + mock_sleep = mocker.patch("utils.llama_stack_version.asyncio.sleep") + mock_client.inspect.version.side_effect = APIConnectionError(request=mocker.MagicMock()) + + with pytest.raises(APIConnectionError): + await check_llama_stack_version(mock_client, max_retries=3, retry_delay=1) + + assert mock_client.inspect.version.call_count == 3 + assert mock_sleep.call_count == 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/utils/test_llama_stack_version.py` around lines 121 - 139, Add a new unit test that verifies check_llama_stack_version re-raises APIConnectionError after all retries are exhausted: create an async test (similar to test_check_llama_stack_version_retries_on_connection_error) that sets mock_client.inspect.version.side_effect to raise APIConnectionError for every call, call await check_llama_stack_version(mock_client, max_retries=N, retry_delay=1) inside pytest.raises(APIConnectionError) and assert mock_client.inspect.version.call_count == N and mock_sleep.call_count == N-1 to confirm it retried the expected number of times before re-raising; reference the existing test name and the check_llama_stack_version function, as well as mock_client.inspect.version and APIConnectionError to locate and mirror behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/llama_stack_version.py`:
- Around line 32-34: The retry parameters allow max_retries to be zero or
negative which causes the retry loop to run zero times and skip the version/API
check; in the function that declares max_retries and retry_delay, validate
max_retries at the start (e.g., if max_retries < 1) and either raise a
ValueError with a clear message or coerce it to a minimum of 1 before entering
the retry loop so the version check runs at least once; reference the parameters
max_retries and retry_delay to locate the function and update the validation
accordingly.
- Around line 6-7: The import uses a private module: replace the internal import
"from llama_stack_client._client import AsyncLlamaStackClient" with the public
export from the package (import AsyncLlamaStackClient directly from
llama_stack_client), i.e. change the import to use the top-level export of
AsyncLlamaStackClient so code uses the public API surface instead of the private
_client module.
---
Nitpick comments:
In `@tests/unit/utils/test_llama_stack_version.py`:
- Around line 121-139: Add a new unit test that verifies
check_llama_stack_version re-raises APIConnectionError after all retries are
exhausted: create an async test (similar to
test_check_llama_stack_version_retries_on_connection_error) that sets
mock_client.inspect.version.side_effect to raise APIConnectionError for every
call, call await check_llama_stack_version(mock_client, max_retries=N,
retry_delay=1) inside pytest.raises(APIConnectionError) and assert
mock_client.inspect.version.call_count == N and mock_sleep.call_count == N-1 to
confirm it retried the expected number of times before re-raising; reference the
existing test name and the check_llama_stack_version function, as well as
mock_client.inspect.version and APIConnectionError to locate and mirror
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc949baa-c4b2-49dc-bdb7-8c3c309760b3
📒 Files selected for processing (2)
src/utils/llama_stack_version.pytests/unit/utils/test_llama_stack_version.py
savitojs
force-pushed
the
savitojs/LCORE-1599-retry-llama-stack-version-check
branch
from
96616f1 to
5cf466e
Compare
Signed-off-by: Savitoj Singh <savsingh@redhat.com>
savitojs
force-pushed
the
savitojs/LCORE-1599-retry-llama-stack-version-check
branch
from
5cf466e to
5b25619
Compare
asimurka
requested changes
Mar 27, 2026
asimurka
left a comment
asimurka
left a comment
Contributor
There was a problem hiding this comment.
LGTM in overall, please correct linter errors
savitojs
deleted the
savitojs/LCORE-1599-retry-llama-stack-version-check
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
check_llama_stack_version()now retries up to 5 times with a 2-second delay when it gets anAPIConnectionErrorfrom Llama Stack. Previously it made a single call and crashed immediately if Llama Stack wasn't ready. This handles the sidecar startup race condition where both containers start concurrently but Llama Stack needs a few extra seconds to initialize.After all retries are exhausted, the original error is still raised so misconfiguration isn't silently ignored.
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Release Notes