Repository navigation
ROB-2346 resync robusta models if robusta model is not found - #1110
Conversation
This case probably means robusta models is out of sync
WalkthroughRemoved caching from Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant LLMRegistry as LLMModelRegistry
participant Configurer as configure_robusta_ai_model
participant Robusta as fetch_robusta_models
Caller->>LLMRegistry: get_model_params(model_key)
LLMRegistry->>LLMRegistry: acquire _lock
LLMRegistry->>LLMRegistry: lookup model_key
alt Found
LLMRegistry-->>Caller: return model_copy()
LLMRegistry->>LLMRegistry: release _lock
else Not found
LLMRegistry->>LLMRegistry: release _lock
alt model_key starts with "Robusta/"
Note right of LLMRegistry `#ffd9b3`: Robusta reconfigure-and-retry
LLMRegistry->>Configurer: configure()
Configurer->>Robusta: fetch_robusta_models (no cache)
Robusta-->>Configurer: fresh data
Configurer-->>LLMRegistry: replace _llms
LLMRegistry->>LLMRegistry: acquire _lock
LLMRegistry->>LLMRegistry: re-lookup model_key
alt Found after sync
LLMRegistry-->>Caller: return model_copy()
else Still not found
LLMRegistry-->>Caller: raise error / fallback to first
end
LLMRegistry->>LLMRegistry: release _lock
else Non-Robusta model
LLMRegistry-->>Caller: raise error / fallback to first
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holmes/core/llm.py (1)
640-666: Inconsistent use ofcopy()vsmodel_copy().Lines 640 and 648 use
model_copy()(correct for Pydantic v2), but lines 658 and 666 still usecopy(). This inconsistency could lead to issues ifcopy()doesn't exist or behaves differently.Apply this diff to use
model_copy()consistently:logging.info( f"Using default Robusta AI model: {self._default_robusta_model}" ) - return model_params.copy() + return model_params.model_copy() logging.error( f"Couldn't find default Robusta AI model: {self._default_robusta_model} in model list" ) model_key, first_model_params = next(iter(self._llms.items())) logging.debug(f"Using first available model: {model_key}") - return first_model_params.copy() + return first_model_params.model_copy()
🧹 Nitpick comments (1)
holmes/core/llm.py (1)
672-675: Consider returning a shallow copy for improved thread safety.The lock guards access to
_llms, but returning the dictionary directly exposes it to potential concurrent modifications by the caller. For stronger isolation, consider returning a shallow copy.@property def models(self) -> dict[str, ModelEntry]: with self._lock: - return self._llms + return self._llms.copy()Note: This adds a performance cost, so only apply if concurrent modification by callers is a realistic concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/llm.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/llm.py
🧠 Learnings (1)
📚 Learning: 2025-08-08T06:15:30.784Z
Learnt from: nilo19
Repo: robusta-dev/holmesgpt PR: 695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.784Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
holmes/core/llm.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: build
🔇 Additional comments (3)
holmes/core/llm.py (3)
4-4: LGTM: Threading import properly placed.The import is correctly positioned at the top of the file as required by the coding guidelines.
642-648: Good fallback logic for out-of-sync Robusta models.The resync mechanism appropriately handles the case where a Robusta model key is requested but not found in the registry. The prefix check and re-fetch logic align with the PR objective.
However, ensure the thread-safety issue with
configure_robusta_ai_model()(raised in the earlier comment on line 528) is addressed.
668-670: Lock acquisition protects dictionary access.The lock appropriately guards the lookup in
_llms. Note that the returnedModelEntryobject itself is not protected from concurrent modifications, but this is acceptable if the caller treats it as read-only.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
holmes/core/llm.py (1)
528-528: Thread-safety remains incomplete:configure_robusta_ai_model()still unprotected.The RLock initialization is correct, but the critical issue from the previous review persists:
configure_robusta_ai_model()(lines 565-600) modifiesself._llmswithout acquiringself._lock. While the call at line 645 is safe because it executes under the lock (RLock is reentrant),configure_robusta_ai_model()should defensively acquire the lock itself to protect all modification paths.Wrap the body of
configure_robusta_ai_model()with the lock:def configure_robusta_ai_model(self) -> None: with self._lock: try: if not self.config.cluster_name or not LOAD_ALL_ROBUSTA_MODELS: self._load_default_robusta_config() return # ... rest of method
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/llm.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/llm.py
🧠 Learnings (1)
📚 Learning: 2025-08-08T06:15:30.784Z
Learnt from: nilo19
Repo: robusta-dev/holmesgpt PR: 695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.784Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
holmes/core/llm.py
🪛 Ruff (0.14.4)
holmes/core/llm.py
635-635: Create your own exception
(TRY002)
635-635: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: build
🔇 Additional comments (2)
holmes/core/llm.py (2)
4-4: LGTM: Import correctly placed.The
threadingimport is properly placed at the top of the file as per coding guidelines.
633-667: Good thread-safe implementation with consistent defensive copying.The method correctly:
- Wraps all state access under
self._lock- Returns
model_copy()on all paths, ensuring callers receive defensive copies- Implements Robusta model resync logic (lines 643-649) safely within the lock
The lock granularity is appropriate for this method's complexity.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
holmes/core/llm.py (2)
528-528: Critical: Lock initialization incomplete—configure_robusta_ai_modelstill unguarded.While
self._lockis now initialized, the synchronization remains incomplete.configure_robusta_ai_model()(lines 565-603) modifiesself._llmsat line 594 without acquiring the lock, creating a race condition withget_model_params()which readsself._llmsunder the lock. This is especially problematic when the resync logic at line 648 callsconfigure_robusta_ai_model()concurrently.Wrap the model loading logic in
configure_robusta_ai_modelwith the lock:def configure_robusta_ai_model(self) -> None: + with self._lock: try: if not self.config.cluster_name or not LOAD_ALL_ROBUSTA_MODELS:And ensure the lock is held throughout the method body, including the assignment at line 594.
674-675: Critical: Exposes mutable dictionary, bypassing synchronization.Returning
self._llmsdirectly allows callers to mutate the dictionary (add, remove, or modify entries) without acquiring the lock, completely bypassing the synchronization mechanism. For example:registry.models["key"] = malicious_entrywould modify the internal state without thread-safety guarantees.Return a defensive copy to prevent external mutation:
@property def models(self) -> dict[str, ModelEntry]: with self._lock: - return self._llms + return {k: v.model_copy() for k, v in self._llms.items()}This ensures callers receive an isolated snapshot that cannot affect the registry's internal state.
🧹 Nitpick comments (1)
holmes/core/llm.py (1)
638-638: Consider defining a custom exception class.Static analysis (Ruff TRY002, TRY003) suggests creating a custom exception rather than raising a generic
Exceptionwith a string message. This improves error handling specificity and makes it easier for callers to catch specific error conditions.Based on static analysis hints.
Define a custom exception class (e.g., at module level):
class NoModelsLoadedError(Exception): """Raised when no LLM models have been loaded into the registry.""" passThen use it:
- raise Exception("No llm models were loaded") + raise NoModelsLoadedError("No llm models were loaded")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/llm.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/llm.py
🧠 Learnings (1)
📚 Learning: 2025-08-08T06:15:30.784Z
Learnt from: nilo19
Repo: robusta-dev/holmesgpt PR: 695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.784Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
holmes/core/llm.py
🪛 Ruff (0.14.4)
holmes/core/llm.py
638-638: Create your own exception
(TRY002)
638-638: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: build
🔇 Additional comments (1)
holmes/core/llm.py (1)
4-4: LGTM: Threading import correctly placed.The
threadingimport is properly positioned at the top of the file as required by the coding guidelines.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
holmes/core/llm.py (2)
528-528: Good choice of RLock for re-entrant locking, but thread-safety remains incomplete.Using
RLockcorrectly handles the resync scenario whereget_model_paramscalls_init_models()while holding the lock. However, critical thread-safety issues from previous reviews remain unaddressed:
- Line 672: The
modelsproperty returnsself._llmsdirectly, allowing callers to mutate the dictionary without lock protection- Lines 565-601:
configure_robusta_ai_model()modifiesself._llmsat line 587 without acquiring the lockWhile the RLock prevents deadlock during re-entrance, these issues still create race conditions.
Apply these fixes:
Fix 1: Return defensive copy in models property
@property def models(self) -> dict[str, ModelEntry]: with self._lock: - return self._llms + return {k: v.model_copy() for k, v in self._llms.items()}Fix 2: Guard mutations in configure_robusta_ai_model
def configure_robusta_ai_model(self) -> None: + with self._lock: try: if not self.config.cluster_name or not LOAD_ALL_ROBUSTA_MODELS:Based on learnings
670-672: Critical: Property still exposes mutable dictionary directly.Despite previous review feedback, the
modelsproperty returnsself._llmsdirectly, allowing callers to mutate the dictionary (add, remove, or modify entries) without acquiring the lock. This bypasses the synchronization mechanism and creates race conditions withconfigure_robusta_ai_model()and other operations.Return a defensive copy:
@property def models(self) -> dict[str, ModelEntry]: with self._lock: - return self._llms + return {k: v.model_copy() for k, v in self._llms.items()}
🧹 Nitpick comments (1)
holmes/core/llm.py (1)
633-667: Thread-safe implementation with correct defensive copying.The lock guards all reads from
self._llmsand all return paths use.model_copy()for defensive copying. The resync logic (lines 643-649) correctly relies on the RLock's re-entrant capability when callingself._init_models().Minor: Consider custom exception classes (optional)
Static analysis suggests creating custom exception classes for better error handling (lines 635, 651). This is a minor style improvement:
class LLMModelNotFoundError(Exception): """Raised when a requested LLM model is not found in the registry.""" pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
holmes/core/llm.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/llm.py
🧠 Learnings (1)
📚 Learning: 2025-08-08T06:15:30.784Z
Learnt from: nilo19
Repo: robusta-dev/holmesgpt PR: 695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.784Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
holmes/core/llm.py
🪛 Ruff (0.14.4)
holmes/core/llm.py
635-635: Create your own exception
(TRY002)
635-635: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
- GitHub Check: build
🔇 Additional comments (1)
holmes/core/llm.py (1)
4-4: LGTM: Threading import added correctly.The import is properly placed at the top of the file as per the coding guidelines.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
holmes/core/llm.py (2)
671-672: Critical: Returning mutable dictionary bypasses thread-safety.Returning
self._llmsdirectly allows callers to mutate the internal dictionary (add, remove, or replace entries) without acquiring the lock, completely bypassing the synchronization mechanism. This was flagged as critical in previous reviews.Return a defensive copy:
@property def models(self) -> dict[str, ModelEntry]: with self._lock: - return self._llms + return {k: v.model_copy() for k, v in self._llms.items()}Deep-copying the values (via
.model_copy()) ensures callers receive isolated snapshots and cannot mutate the registry state.
528-528: Critical: Lock initialization is insufficient without guarding allself._llmsmutations.While the RLock is now initialized, the thread-safety implementation remains incomplete as flagged in previous reviews:
Unguarded writes during initialization:
_init_models()(line 537) andconfigure_robusta_ai_model()(lines 587, 594-595) directly assign toself._llmswithout acquiringself._lock.Race condition window: The initial call from
__init__→_init_models()→configure_robusta_ai_model()executes without the lock, whileget_model_params()readsself._llmsunder the lock. This creates a classic reader-writer race condition.Resync path is safe by accident: The resync logic (line 645) calls
_init_models()while already holding the lock. Because RLock is reentrant, nested mutations work correctly, but this is fragile and inconsistent with the unlocked initialization path.Wrap all mutations to
self._llmswith the lock:def _init_models(self): - self._llms = self._parse_models_file(MODEL_LIST_FILE_LOCATION) + with self._lock: + self._llms = self._parse_models_file(MODEL_LIST_FILE_LOCATION) - if self._should_load_robusta_ai(): - self.configure_robusta_ai_model() + if self._should_load_robusta_ai(): + self.configure_robusta_ai_model() - if self._should_load_config_model(): - self._llms[self.config.model] = self._create_model_entry( - model=self.config.model, - model_name=self.config.model, - base_url=self.config.api_base, - is_robusta_model=False, - api_key=self.config.api_key, - api_version=self.config.api_version, - ) + if self._should_load_config_model(): + self._llms[self.config.model] = self._create_model_entry( + model=self.config.model, + model_name=self.config.model, + base_url=self.config.api_base, + is_robusta_model=False, + api_key=self.config.api_key, + api_version=self.config.api_version, + )Since
_init_models()will now acquire the lock, and the resync path (line 645) already holds the lock, you need to handle reentrant acquisition. With RLock this works automatically, but consider documenting that_init_models()and its callees (configure_robusta_ai_model()) should only be called while holding the lock, or refactor to separate "build new registry" from "swap registry under lock" logic.
🧹 Nitpick comments (1)
holmes/core/llm.py (1)
635-635: Consider using a custom exception class.Raising a generic
Exceptionwith a string message makes error handling less specific. Consider defining a custom exception or using a more specific built-in exception likeValueErrororRuntimeError.Based on coding guidelines (Ruff configuration).
- if not self._llms: - raise Exception("No llm models were loaded") + if not self._llms: + raise RuntimeError("No llm models were loaded")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
holmes/core/llm.py(3 hunks)tests/core/test_llm_model_registry_get_model_params.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for formatting and linting (configured in pyproject.toml) for all Python code
Type hints are required; code should pass mypy (configured in pyproject.toml)
ALWAYS place Python imports at the top of the file, not inside functions or methods
Files:
holmes/core/llm.pytests/core/test_llm_model_registry_get_model_params.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Only use pytest markers that are defined in pyproject.toml; never introduce undefined markers/tags
Files:
tests/core/test_llm_model_registry_get_model_params.py
tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Test files should mirror the source structure under tests/
Files:
tests/core/test_llm_model_registry_get_model_params.py
🧠 Learnings (1)
📚 Learning: 2025-08-08T06:15:30.784Z
Learnt from: nilo19
Repo: robusta-dev/holmesgpt PR: 695
File: holmes/core/transformers/registry.py:9-19
Timestamp: 2025-08-08T06:15:30.784Z
Learning: holmes/core/transformers/registry.py: TransformerRegistry is intentionally single-threaded and not designed to be thread-safe; avoid proposing locks unless multi-threaded access is introduced later.
Applied to files:
holmes/core/llm.py
🧬 Code graph analysis (1)
tests/core/test_llm_model_registry_get_model_params.py (2)
holmes/config.py (2)
Config(46-521)dal(121-124)holmes/core/llm.py (3)
LLMModelRegistry(522-715)ModelEntry(67-88)get_model_params(632-667)
🪛 Ruff (0.14.4)
holmes/core/llm.py
635-635: Create your own exception
(TRY002)
635-635: Avoid specifying long messages outside the exception class
(TRY003)
tests/core/test_llm_model_registry_get_model_params.py
55-55: Unused lambda argument: self
(ARG005)
55-55: Unused lambda argument: path
(ARG005)
71-71: Unused lambda argument: self
(ARG005)
71-71: Unused lambda argument: path
(ARG005)
87-87: Unused lambda argument: self
(ARG005)
87-87: Unused lambda argument: path
(ARG005)
97-97: Unused method argument: caplog
(ARG002)
105-105: Unused lambda argument: self
(ARG005)
105-105: Unused lambda argument: path
(ARG005)
110-110: Unused lambda argument: self
(ARG005)
110-110: Unused lambda argument: path
(ARG005)
132-132: Unused lambda argument: self
(ARG005)
132-132: Unused lambda argument: path
(ARG005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
- GitHub Check: build
🔇 Additional comments (3)
holmes/core/llm.py (1)
633-667: The lock acquisition and defensive copying are implemented correctly.This method properly:
- Acquires
self._lockfor the entire operation (line 633)- Returns
.model_copy()in all paths (lines 641, 649, 659, 667) for thread-safe defensive copying- Implements Robusta resync logic that detects missing models and retries after reloading
However, the correctness depends on
_init_models()(called on line 645) being properly synchronized, which requires the fix mentioned in the previous comment.tests/core/test_llm_model_registry_get_model_params.py (2)
12-46: Well-structured test class with reusable fixtures.The test organization is clean with appropriate fixtures for mock objects and test data. The fixtures properly mock dependencies (Config, DAL) and provide ModelEntry instances for testing different scenarios.
48-143: Comprehensive test coverage ofget_model_paramsbehavior.The test suite effectively covers:
- Valid model key retrieval (lines 48-61)
- Fallback to first available model when key is invalid (lines 63-77)
- Default Robusta model preference (lines 79-94)
- Robusta resync success path (lines 96-123)
- Robusta resync failure with fallback (lines 124-143)
The tests properly verify both return values and logging behavior, ensuring the new resync logic works as intended.
RoiGlinik
changed the title
2 participants
This case probably means robusta models is out of sync