Add sanity health check for public asset service#34
Conversation
|
/claude-review |
|
Claude finished @galt-tr's task —— View job Code Review Complete
SummaryThe PR adds a sensible health check for the asset HTTP address during P2P server initialization. The test coverage is comprehensive and handles various edge cases well. Issues to Address:
Positive aspects:
Minor observations:
|
| // If the address is not reachable, automatically switch to listen-only mode | ||
| if s.settings.P2P.ListenMode == settings.ListenModeFull { | ||
| if !s.isAssetAddressReachable(ctx, s.AssetHTTPAddressURL) { | ||
| s.logger.Warnf("[Init] Asset HTTP address %s is not reachable. Automatically switching to listen-only mode to prevent peers from attempting to fetch data from this node. Please check your asset_httpPublicAddress configuration and ensure the endpoint is publicly accessible.", s.AssetHTTPAddressURL) |
There was a problem hiding this comment.
Issue: Modifying s.settings.P2P.ListenMode after server initialization can cause inconsistencies. Settings should generally be immutable after configuration.
Suggestion: Consider storing the effective listen mode in a separate field (e.g., s.effectiveListenMode) rather than mutating the original settings. This preserves the user's intended configuration and makes debugging easier.
| // Get genesis hash for the health check | ||
| var genesisHash string | ||
| if s.settings != nil && s.settings.ChainCfgParams != nil && s.settings.ChainCfgParams.GenesisHash != nil { | ||
| genesisHash = s.settings.ChainCfgParams.GenesisHash.String() |
There was a problem hiding this comment.
Issue: Using a hardcoded default genesis hash for regtest is brittle and may fail silently if the settings are unexpectedly nil.
Suggestion: Consider failing fast with an error if s.settings.ChainCfgParams.GenesisHash is nil, rather than falling back to a hardcoded value. This makes configuration issues visible immediately.
| s.logger.Debugf("[Init] Asset address %s health check responded with status %d", assetURL, resp.StatusCode) | ||
|
|
||
| // Check for offline indicators in 404 responses | ||
| if resp.StatusCode == 404 { |
There was a problem hiding this comment.
Question: Why limit to only the first 1024 bytes? If the response body is larger, the offline detection keywords might be missed if they appear later in the response.
Suggestion: Consider reading more (e.g., 8KB) or reading the entire response with a reasonable size limit to ensure reliable offline detection.
|
🤖 Claude Code Review Status: Complete Current Review: The implementation adds automatic asset address health checking during P2P server initialization, with comprehensive test coverage. The approach is sound and follows project patterns. Minor observations:
The health check logic is well-implemented and will prevent nodes from advertising unavailable asset endpoints to peers. |
|
|
This pull request adds an automatic health check for the asset HTTP address during server initialization in the P2P service. If the asset address is unreachable and the node is in full mode, the server now switches to listen-only mode to prevent other peers from attempting to fetch data from an unavailable node. The implementation includes robust detection of various failure scenarios and is thoroughly covered by new unit tests.
Asset address health check logic
Initmethod ofServerto verify the reachability of the configured asset HTTP address. If unreachable and the node is in full mode, the server switches to listen-only mode. [1] [2]isAssetAddressReachablemethod, which attempts to fetch the genesis block from the asset server with a timeout, and considers various HTTP status codes and offline indicators.iofor reading HTTP response bodies.Test coverage for health check
Server_test.goto cover scenarios including reachable/unreachable asset addresses, server errors, offline detection, normal 404 responses, skipping health checks in listen-only mode, and empty asset addresses.Fixes #2