Add periodic toolset status refresh for server mode - #1354
Conversation
|
|
📂 Previous Runs📜 Run @ 45bec40 (#21032504026)✅ Results of HolmesGPT evalsAutomatically triggered by commit 45bec40 on branch 📜 Run @ 796a89c (#20877126940)✅ Results of HolmesGPT evalsAutomatically triggered by commit 796a89c on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/fix-toolset-status-check-W0yHB' Status: Success - 12 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📜 Run @ 67efa45 (#20876371951)✅ Results of HolmesGPT evalsAutomatically triggered by commit 67efa45 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/fix-toolset-status-check-W0yHB' Status: Success - 14 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📜 Run @ 995a93f (#20875249835)✅ Results of HolmesGPT evalsAutomatically triggered by commit 995a93f on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/fix-toolset-status-check-W0yHB' Status: Success - 14 test/model combinations loaded Experiments compared (30):
Comparison indicators:
✅ Results of HolmesGPT evalsAutomatically triggered by commit 21199d8 on branch Results of HolmesGPT evals
Time/Cost columns show % change vs historical average (↑slower/costlier, ↓faster/cheaper). Changes under 10% shown as ±0%. Historical Comparison DetailsFilter: excluding branch 'claude/fix-toolset-status-check-W0yHB' Status: Success - 16 test/model combinations loaded Experiments compared (30):
Comparison indicators:
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" 🏷️ Valid markers
Commands: CLI: |
WalkthroughAdds an environment-configurable background loop that periodically refreshes server toolset statuses, a new env var, a ToolsetManager method to compute status changes, a Config method to apply them, and silent-mode prerequisite checks to reduce logging during listing. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant Bg as BackgroundThread
participant Config
participant TM as ToolsetManager
participant DAL
Server->>Bg: start refresh loop (interval from env)
loop every interval
Bg->>Config: refresh_server_tool_executor(dal)
Config->>Config: inspect current server executor toolsets
Config->>TM: refresh_server_toolsets_and_get_changes(current_toolsets, dal)
TM->>DAL: list server toolsets (may check prerequisites, silent)
TM-->>Config: (new_toolsets, changes)
alt changes exist
Config->>Config: rebuild server_tool_executor with new_toolsets
end
Config-->>Bg: return changes
Bg->>Bg: log changes or "no changes"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:9e53564
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:9e53564 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:9e53564
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:9e53564Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:9e53564Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:9e53564 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @server.py:
- Around line 114-146: Move the "import threading" out of
_toolset_status_refresh_loop and place it at module-level with the other
imports; remove the in-function import so threading.Thread is referenced from
the top-level import. Additionally, if you want the first refresh to run
immediately instead of waiting the full interval, call
config.refresh_server_tool_executor(dal) once (handling and logging
changes/exceptions exactly as done in refresh_loop) before entering the while
True: time.sleep(interval) loop inside the refresh_loop function.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 35276a8 and 995a93fbb2b4a09c29bc2cee91c185631927ed21.
📒 Files selected for processing (4)
holmes/common/env_vars.pyholmes/config.pyholmes/core/toolset_manager.pyserver.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
holmes/core/toolset_manager.pyholmes/common/env_vars.pyserver.pyholmes/config.py
🧬 Code graph analysis (3)
holmes/core/toolset_manager.py (1)
holmes/core/tools.py (3)
Toolset(525-769)ToolsetStatusEnum(123-126)check_prerequisites(674-748)
server.py (1)
holmes/config.py (2)
refresh_server_tool_executor(286-311)dal(123-126)
holmes/config.py (2)
holmes/core/toolset_manager.py (1)
refresh_server_toolsets_and_get_changes(385-421)holmes/core/tools_utils/tool_executor.py (1)
ToolExecutor(14-58)
⏰ 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). (5)
- GitHub Check: build
- GitHub Check: llm_evals
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
🔇 Additional comments (5)
holmes/common/env_vars.py (1)
132-137: LGTM! Clean addition of the refresh interval configuration.The constant follows the existing pattern in the file, has clear documentation, and the 5-minute default interval is reasonable for a background health check task.
holmes/core/toolset_manager.py (1)
385-422: LGTM! Well-implemented refresh and change detection method.The logic correctly:
- Captures the current state of all toolsets
- Re-runs prerequisite checks via
_list_all_toolsets- Detects and reports status transitions (e.g., FAILED → ENABLED)
Note: The method only reports changes for toolsets that already exist (Line 418's
old_status is not Nonecheck). Newly discovered toolsets won't appear in the changes list, which aligns with the PR's focus on detecting status changes rather than toolset discovery.holmes/config.py (1)
286-311: LGTM! Clean integration of the refresh mechanism.The method properly:
- Handles the bootstrap case when no executor exists yet (Lines 295-298)
- Delegates change detection to
ToolsetManager- Only recreates the executor when changes are detected (optimization)
- Returns a clean, serializable format with string values instead of enums
server.py (2)
41-41: LGTM! Import correctly placed at module level.
486-486: LGTM! Correct placement in startup sequence.The refresh loop is started after the initial toolset sync (line 485) and before the server begins accepting requests. The daemon thread ensures it won't prevent clean shutdown.
995a93f to
67efa45
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @holmes/config.py:
- Around line 286-303: The method refresh_server_tool_executor reads and
replaces self._server_tool_executor without synchronization, risking race
conditions; add a threading.Lock attribute (e.g., _server_tool_executor_lock) to
the Config class and use it to protect both the read of
self._server_tool_executor.toolsets in refresh_server_tool_executor and the
write that assigns self._server_tool_executor = ToolExecutor(new_toolsets); also
acquire the same lock around any other accesses (notably create_tool_executor
and any request-handling getters) to ensure consistent reads/writes to
_server_tool_executor.
🧹 Nitpick comments (1)
holmes/core/toolset_manager.py (1)
385-407: Consider documenting performance characteristics.The method re-checks prerequisites for all toolsets on each call, which involves I/O operations (network checks, command execution, etc.). While the 5-minute default interval makes this acceptable, users reducing
TOOLSET_STATUS_REFRESH_INTERVAL_SECONDSsignificantly might experience performance issues. Consider adding a comment documenting this behavior or suggesting a minimum safe interval.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 995a93fbb2b4a09c29bc2cee91c185631927ed21 and 67efa45499eac5fc63f73be92e0298476dda5e54.
📒 Files selected for processing (4)
holmes/common/env_vars.pyholmes/config.pyholmes/core/toolset_manager.pyserver.py
🚧 Files skipped from review as they are similar to previous changes (1)
- server.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
holmes/common/env_vars.pyholmes/core/toolset_manager.pyholmes/config.py
🧬 Code graph analysis (2)
holmes/core/toolset_manager.py (1)
holmes/core/tools.py (2)
Toolset(525-769)ToolsetStatusEnum(123-126)
holmes/config.py (2)
holmes/core/toolset_manager.py (1)
refresh_server_toolsets_and_get_changes(385-407)holmes/core/tools_utils/tool_executor.py (1)
ToolExecutor(14-58)
⏰ 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). (4)
- GitHub Check: llm_evals
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
🔇 Additional comments (2)
holmes/common/env_vars.py (1)
132-137: LGTM! Well-documented environment variable.The constant follows the established pattern, has a reasonable default (5 minutes), and includes clear documentation about the disable-via-zero behavior.
holmes/core/toolset_manager.py (1)
385-407: LGTM! Logic correctly detects toolset status changes.The method appropriately:
- Maps old statuses from current toolsets
- Re-checks prerequisites via
_list_all_toolsets()- Identifies only actual status changes (ignoring new toolsets)
- Returns both updated toolsets and the change list
One observation: removed toolsets (present in
current_toolsetsbut absent innew_toolsets) won't be reported. If this is intentional for server mode, consider adding a comment to clarify.
Toolset availability can change after server startup (e.g., a database becoming available after Holmes starts). This adds a background task that periodically re-checks toolset prerequisites and updates the ToolExecutor when status changes. Changes: - Add TOOLSET_STATUS_REFRESH_INTERVAL_SECONDS env var (default 300s) - Add refresh_server_toolsets_and_get_changes() to detect status changes - Add refresh_server_tool_executor() to Config for updating toolsets - Add background refresh thread that logs when toolset states change - Add silent parameter to check_prerequisites() to suppress logs during periodic refresh (only status changes are logged) - Set interval to 0 to disable periodic refresh Signed-off-by: Claude <noreply@anthropic.com>
67efa45 to
796a89c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
holmes/config.py (1)
286-303: Consider thread safety for concurrent access.The
_server_tool_executoris accessed from multiple places (API endpoints viacreate_tool_executorand the background refresh thread). While Python's GIL makes the reference assignment atomic, there's a potential race where an ongoing request could be using the old executor while the refresh is happening.This is likely acceptable for this use case since:
- The old executor remains valid until garbage collected
- Requests in flight will complete with the old executor
- New requests will pick up the new executor
However, if you want stronger guarantees, consider adding a
threading.Lockaround the executor access/assignment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 67efa45499eac5fc63f73be92e0298476dda5e54 and 796a89c.
📒 Files selected for processing (5)
holmes/common/env_vars.pyholmes/config.pyholmes/core/tools.pyholmes/core/toolset_manager.pyserver.py
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/common/env_vars.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
holmes/core/tools.pyholmes/core/toolset_manager.pyserver.pyholmes/config.py
🧠 Learnings (1)
📚 Learning: 2026-01-05T11:14:20.222Z
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-05T11:14:20.222Z
Learning: Applies to holmes/plugins/toolsets/**/*.py : Include health check in prerequisites_callable() method for Python toolsets
Applied to files:
holmes/core/tools.pyholmes/core/toolset_manager.py
🧬 Code graph analysis (2)
holmes/core/toolset_manager.py (1)
holmes/core/tools.py (3)
Toolset(525-771)check_prerequisites(674-750)ToolsetStatusEnum(123-126)
holmes/config.py (2)
holmes/core/toolset_manager.py (1)
refresh_server_toolsets_and_get_changes(386-409)holmes/core/tools_utils/tool_executor.py (1)
ToolExecutor(14-58)
⏰ 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). (4)
- GitHub Check: build (3.12)
- GitHub Check: llm_evals
- GitHub Check: build (3.10)
- GitHub Check: build (3.11)
🔇 Additional comments (6)
holmes/core/tools.py (1)
674-750: LGTM!The
silentparameter addition is well-implemented with a sensible default ofFalseto maintain backward compatibility. The guards around logging statements (if not silent) correctly suppress both failure and success messages during silent mode, which is appropriate for the periodic refresh use case to avoid log spam.server.py (2)
115-143: LGTM!The background refresh loop implementation is well-structured:
- Correctly checks for disabled state (
interval <= 0) and returns early- Uses a daemon thread, ensuring it won't block server shutdown
- Sleeps before the first check, avoiding redundant work right after startup
- Has proper exception handling with
exc_info=Truefor debugging- Logs changes at INFO level and no-changes at DEBUG level, which is appropriate to avoid log noise
483-485: LGTM!The placement of
_toolset_status_refresh_loop()is correct - it runs aftersync_before_server_start()completes (which does initial toolset sync) and before the server starts accepting requests.holmes/core/toolset_manager.py (3)
95-102: LGTM!The
silentparameter is properly added to_list_all_toolsetswith a backward-compatible default ofFalse.
175-183: LGTM!The
silentparameter is correctly propagated throughcheck_toolset_prerequisitesto each individual toolset'scheck_prerequisitescall.
386-409: Verify that not reporting new/removed toolsets is intentional.The change detection logic only reports status changes for toolsets that existed in both the old and new sets. If a toolset is newly added or removed entirely, it won't appear in the changes list.
This is likely intentional since:
- New toolsets would have no "old status" to compare against
- Removed toolsets wouldn't be in the new list to iterate over
If you want to also log when toolsets are added or removed, additional logic would be needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server.py`:
- Around line 115-143: The refresh loop can race with request handlers because
_server_tool_executor is replaced without synchronization; add a dedicated
threading.Lock (e.g., _server_tool_executor_lock) and use it to guard all
accesses: acquire the lock around the check-and-cache logic in
create_tool_executor and around the read-and-replace logic inside
refresh_server_tool_executor (the code invoked by _toolset_status_refresh_loop),
ensuring the refresh thread holds the lock while updating _server_tool_executor
and request threads hold the lock while reading/caching it so no concurrent
read/write occurs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always place Python imports at the top of the file, not inside functions or methods
Use Ruff for formatting and linting with configuration in pyproject.toml
Type hints required (mypy configuration in pyproject.toml)
Pre-commit hooks enforce quality checks on Python files
Files:
server.py
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php}: Use semantic, descriptive names for variables, functions, and components
Write clear, concise comments that explain 'why' rather than 'what'
Files:
server.py
🧬 Code graph analysis (1)
server.py (1)
holmes/config.py (2)
refresh_server_tool_executor(286-303)dal(123-126)
🔇 Additional comments (2)
server.py (2)
26-26: LGTM!The
threadingimport has been correctly moved to module level as per the coding guidelines, and the new environment variable import is appropriately placed with other imports fromholmes.common.env_vars.Also applies to: 42-42
487-489: The refresh loop runs in the production deployment as intended.The codebase is deployed via
python3 -u server.py(as shown in the Helm chart), which executes the__main__block and starts the refresh loop as a daemon thread. This is the documented deployment method. There is no evidence of support for alternative deployment methods (e.g.,uvicorn server:appor gunicorn), so the hypothetical concern does not apply.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
3 participants
Toolset availability can change after server startup (e.g., a database becoming available after Holmes starts). This adds a background task that periodically re-checks toolset prerequisites and updates the ToolExecutor when status changes.
Changes:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.