Skip to content

fix: add timeout to health check endpoints - #15

Closed
buzzkillb wants to merge 9 commits into
mainfrom
fix/health-timeout
Closed

fix: add timeout to health check endpoints#15
buzzkillb wants to merge 9 commits into
mainfrom
fix/health-timeout

fix: compilation errors in stability migration

bd70916
Select commit
Loading
Failed to load commit list.
GitzillaBot / gitzilla succeeded Apr 19, 2026 in 4m 45s

Gitzilla review complete

1 issue found.

Based on my analysis of this PR, I found one confirmed high-severity issue related to the changes made. The PR primarily adds a timeout wrapper to health check endpoints as described, but the broader changes include a SQLite to PostgreSQL migration, service supervision improvements, and shared state management. The concurrent file write race condition I reported represents a real data loss risk that could affect the integrity of the price data being tracked across multiple crypto assets. The fix is straightforward - either coordinate file writes through the existing SharedPrices RwLock, or use atomic file operations (write to temp file then rename).
Based on my analysis of the code changes in this pull request, I can now provide a comprehensive review. This PR adds timeouts to health check endpoints, restructures service supervision in main.rs with proper handle tracking, adds database failure tracking, and changes database calls from synchronous to async throughout the codebase. Looking at the health_server.rs implementation: The timeout mechanism in health_check returns {"healthy": false} on timeout, while health_check_all returns {"error": "health check timeout"} with is_all_healthy: false. The health_check_all timeout handler correctly captures the timeout error and returns an explicit error message, which provides better diagnostics than the simple /health endpoint that returns an indistinguishable unhealthy state. However, there's an inconsistency worth noting in the retry logic where success after retry is logged at error level, though this is a minor logging concern rather than a functional defect. The architectural changes are sound: timeouts prevent indefinite hangs during health checks, the shared prices state is now updated atomically before persisting to disk, and the service supervision properly tracks and monitors service handles. Summary of changes: The PR implements a 5-second timeout wrapper around health check handlers in src/health_server.rs to prevent Docker health checks from timing out when the health server hangs. It also restructures src/main.rs to use a proper ServiceHandle enum for tracking spawned services with monitoring that detects unexpected service deaths. The database layer is modernized to use async operations with .await throughout src/bot.rs and src/price_service.rs. Additionally, src/health.rs adds tracking for database write failures (db_failures counter) that can contribute to unhealthy status reporting.

Details

  1. [HIGH] Concurrent file writes cause price data loss
    Location: src/shanghai_price_service.rs:126
    The shanghai_price_service and price_service both write to shared/prices.json without coordination, creating a read-modify-write race condition. In update_prices_json (shanghai_price_service.rs:126-145), the function reads the existing file, merges in new data, and writes back. If price_service.write_prices_to_file overwrites the file between the read and write, shanghai_service's merged data will overwrite all other prices, causing silent data loss. This affects BTC, ETH, SOL, DXY and other prices tracked by the main price service.
    Suggested fix: Use atomic file writes (write to temp file then rename) or coordinate file writes via the SharedPrices RwLock. Alternatively, have only one service write to the JSON file and read-only access for the other.