Skip to content

feat: add retry logic to health check#69

Closed
vlordier wants to merge 8 commits intoLiquid4All:mainfrom
vlordier:feat/health-check-retry
Closed

feat: add retry logic to health check#69
vlordier wants to merge 8 commits intoLiquid4All:mainfrom
vlordier:feat/health-check-retry

Conversation

@vlordier
Copy link
Copy Markdown

@vlordier vlordier commented Mar 6, 2026

Summary

Improves on PR #58 (health monitoring) by adding retry logic:

  • Add 3 attempts with exponential backoff (200ms, 400ms, 800ms) to health_check()
  • Handles transient failures (temporary network issues, slow server startup)
  • Reduces false negatives when model server is temporarily unavailable

This makes health monitoring more reliable, especially during model server restarts.

Testing

  • Clippy clean

vlordier added 8 commits March 6, 2026 21:46
- Tool Result Compression: smart summarization for large tool outputs
  (directory listings, search results, JSON data)
- Request Deduplication: 500ms debounce on send button to prevent
  duplicate requests from rapid clicks
- Config Hot Reload: poll config file for changes, reload without
  restart, show toast notification
- Error Boundary: timeout wrapper (120s) around tool execution,
  graceful error messages instead of crashing agent loop
- Add 13 tests for settings.rs (AppSettings, SamplingConfig, config hot reload)
- Add 12 tests for chat.rs compression functions (truncate, compress directory, search, JSON)
- Total tests: 392 (up from 365)
- Coverage: 42.5% (up from 39.5%)
- Add ModelStatus tests (serialization, healthy/unhealthy states)
- Add SamplingOverrides serialization tests
- Add InferenceClient tests for:
  - LM Studio URL construction and model selection
  - Tool call format (NativeJson vs Pythonic)
  - Fallback chain exhaustion (AllModelsUnavailable)
  - is_retriable for various HTTP status codes
  - Error repair from malformed tool calls
- Total tests: 417 (up from 392)
- Add tests for data_dir(), cache_dir(), resolve_db_path()
- Add rotate_log_file() tests (creates rotated copies, handles missing files)
- Add filter_by_enabled_servers() tests (filters correctly, handles missing/invalid config)
- Add resolve_vision_model() tests (returns None without config)
- Add filter_tools_by_allowlist() test (doesn't panic without config)
- Add load_override_file() tests (parses config, returns empty for missing)
- Total tests: 430 (up from 417)
- Coverage: 43.3% (up from 42.5%)
- Add helpful debug message when LM Studio port connection fails
- Distinguish between connection errors and timeouts in health_check
- Document default LM Studio port (1234) in environment configuration
- Add GitHub Actions workflow for shellcheck
- Add shellcheck validation to pre-commit hook for staged .sh files

This improves on PR Liquid4All#55 by adding automated shellcheck validation.
- Add 3 attempts with exponential backoff to health_check()
- Improves reliability of PR Liquid4All#58 health monitoring by handling transient failures

This helps avoid false negatives when model server is temporarily slow.
Copilot AI review requested due to automatic review settings March 6, 2026 22:44
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR is titled "feat: add retry logic to health check" but the actual diff contains a large initial commit of the entire LocalCowork application codebase, including Rust backend (Tauri), TypeScript/React frontend, configuration files, and CI workflows. The PR description mentions retry logic for health checks, but no health check file or retry logic is visible in the diffs.

Changes:

  • Full initial codebase commit for the LocalCowork Tauri desktop app (Rust backend + React frontend)
  • Frontend store (settingsStore.ts), UI components (MessageInput.tsx, App.tsx), and Tauri configuration
  • Rust modules for MCP client, inference engine, agent core, and Tauri IPC commands

Reviewed changes

Copilot reviewed 51 out of 58 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/stores/settingsStore.ts Zustand store for settings, config watching, permissions, and sampling config
src/components/Chat/MessageInput.tsx Chat input component with debounce and auto-resize
src/App.tsx Root app component with onboarding, settings panel, and config reload notification
src-tauri/tauri.conf.json Tauri app configuration including window, security, and bundle settings
src-tauri/src/mcp_client/ MCP client modules: types, transport, lifecycle, registry, errors, discovery, client
src-tauri/src/inference/ Inference client modules: types, errors, config, streaming, mod
src-tauri/src/commands/ Tauri IPC commands: session, python_env, ollama, model_download, hardware, filesystem, mod
src-tauri/src/agent_core/ Agent core modules: types, tool_router, tokens, response_analysis, permissions, plan_parser, plan_templates, tool_prefilter, errors, mod
src-tauri/src/main.rs Tauri app entrypoint
src-tauri/Cargo.toml Rust dependencies
src-tauri/build.rs Tauri build script
src-tauri/mcp-servers.json Empty MCP server config
src-tauri/entitlements.plist macOS entitlements
src-tauri/capabilities/default.json Tauri capability permissions
examples/localcowork/.github/workflows/shellcheck.yml CI workflow for shellcheck
examples/localcowork/.git-hooks/pre-commit Pre-commit hook with shellcheck integration
examples/localcowork/.env.example Environment variable documentation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +142 to +143
if (interval) {
clearInterval(interval);
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stopConfigWatch destructures configWatchInterval from state but then checks the undeclared variable interval, which will cause a runtime ReferenceError. The condition on line 142 should reference configWatchInterval instead of interval.

Suggested change
if (interval) {
clearInterval(interval);
if (configWatchInterval) {
clearInterval(configWatchInterval);

Copilot uses AI. Check for mistakes.
const [value, setValue] = useState("");
const textareaRef = useRef<HTMLTextAreaElement>(null);
const lastSendTimeRef = useRef<number>(0);
const [isDebouncing, setIsDebouncing] = useState(false);
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isDebouncing is set to true when a duplicate send is detected, but there is no path that sets it back to false in that case — the setTimeout callback on line 39 does call setIsDebouncing(false), however the debounce guard returns early before lastSendTimeRef is updated, so the spinner will show for 500ms even though no send was in progress. More critically, isDebouncing is never set to true for a successful send, so the "Please wait..." placeholder and spinner only appear during the debounce-rejection window, not during an actual in-progress send. The state name isDebouncing is misleading for this behavior; consider renaming or rethinking the intended UX.

Copilot uses AI. Check for mistakes.
Comment thread src/App.tsx
Comment on lines +52 to +54
<button className="toast-close" aria-label="Dismiss">
×
</button>
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Dismiss" button inside the toast has no onClick handler of its own. The parent <div> on line 48 has onClick={clearConfigReloadNotification}, but since the button does not call stopPropagation, clicking the button will trigger the parent's handler via event bubbling — this works, but it also means any click anywhere on the toast dismisses it, which may not be intended. More importantly, the button itself should have an explicit onClick handler so its behavior is clear and not accidentally dependent on event propagation.

Copilot uses AI. Check for mistakes.
@Paulescu
Copy link
Copy Markdown
Collaborator

Hi @vlordier

The intent of this PR is not clear to me.
Could you please explain it to me using your own words?

Pau

@vlordier vlordier closed this Apr 28, 2026
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.

3 participants