Bugfix server crash#201
Merged
Merged
Conversation
Contributor
Reviewer's GuideMakes the Open-Meteo client thread-local and resilient to SQLite cache corruption, and hardens API response handling against missing current data while bumping the package patch version. Sequence diagram for resilient Open-Meteo weather_api callssequenceDiagram
participant Caller
participant ResilientClient as _ResilientClient
participant ThreadLocal as _thread_local
participant Builder as _build_client
participant OMClient as openmeteo_requests.Client
Caller->>ResilientClient: weather_api(url, params)
ResilientClient->>ThreadLocal: _get_thread_client()
alt [no client on thread]
ThreadLocal->>Builder: _build_client()
Builder-->>ThreadLocal: openmeteo_requests.Client
ThreadLocal-->>ResilientClient: client
else [client exists]
ThreadLocal-->>ResilientClient: client
end
ResilientClient->>OMClient: weather_api(url, params)
alt [sqlite3.DatabaseError]
ResilientClient->>Builder: _build_client()
Builder-->>ResilientClient: new_client
ResilientClient->>OMClient: weather_api(url, params)
OMClient-->>ResilientClient: response
else [success]
OMClient-->>ResilientClient: response
end
ResilientClient-->>Caller: response
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
_ResilientClientonly wrapsweather_api, so if other client methods are used now or in the future they will bypass the retry/rebuild logic; consider either exposing and wrapping the underlying client more generically or documenting that onlyweather_apiis supported. - In
get_uv,ocean_information, andcurrent_wind_temp, the newcurrent is Nonebranches change the return types (e.g., string vs numeric, lists of zeros vs the usual float values), which may be surprising to callers; consider using a sentinel likeNoneor a typed dataclass/struct so the API remains type-consistent. - When catching
sqlite3.DatabaseErrorin_ResilientClient.weather_api, it may be helpful to at least log the error before recreating the client so that underlying cache/database issues are observable rather than silently masked.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_ResilientClient` only wraps `weather_api`, so if other client methods are used now or in the future they will bypass the retry/rebuild logic; consider either exposing and wrapping the underlying client more generically or documenting that only `weather_api` is supported.
- In `get_uv`, `ocean_information`, and `current_wind_temp`, the new `current is None` branches change the return types (e.g., string vs numeric, lists of zeros vs the usual float values), which may be surprising to callers; consider using a sentinel like `None` or a typed dataclass/struct so the API remains type-consistent.
- When catching `sqlite3.DatabaseError` in `_ResilientClient.weather_api`, it may be helpful to at least log the error before recreating the client so that underlying cache/database issues are observable rather than silently masked.
## Individual Comments
### Comment 1
<location path="src/open_meteo.py" line_range="28-24" />
<code_context>
+ return _thread_local.client
+
+
+class _ResilientClient:
+ def weather_api(self, url, params=None):
+ client = _get_thread_client()
+ try:
+ return client.weather_api(url, params=params)
+ except sqlite3.DatabaseError:
+ _thread_local.client = _build_client()
+ return _thread_local.client.weather_api(url, params=params)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching all sqlite3.DatabaseError and blindly rebuilding the client risks masking persistent cache/DB issues.
Rebuilding the client on any sqlite3.DatabaseError with the same cache path could just hit the same failure again if the DB is corrupted or the filesystem is unhealthy. Consider catching a narrower exception (e.g. OperationalError) and/or adding logic to clear/recreate the cache file, cap the number of rebuild attempts, or ultimately propagate the error so callers can handle a persistent failure appropriately.
</issue_to_address>
### Comment 2
<location path="src/open_meteo.py" line_range="22-25" />
<code_context>
-openmeteo_client = _create_client()
+def _get_thread_client() -> openmeteo_requests.Client:
+ if not hasattr(_thread_local, "client"):
+ _thread_local.client = _build_client()
+ return _thread_local.client
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Thread-local client recreation on error may leave underlying sessions/connections unclosed.
When `_thread_local.client` is replaced after a `DatabaseError`, the previous client (and its session/cache backend) is discarded without explicit cleanup. If `openmeteo_requests.Client`/`requests_cache` hold SQLite connections or file descriptors, this can leak resources under repeated errors. Consider a helper that calls a `close()`/context-exit on the old client (if available) before assigning a new one.
Suggested implementation:
```python
def _set_thread_client(client: openmeteo_requests.Client) -> openmeteo_requests.Client:
"""Set the thread-local client, closing any existing client if it exposes a close() method.
This helps avoid leaking resources (e.g. SQLite connections/file descriptors) when
the thread-local client is recreated after errors.
"""
old_client = getattr(_thread_local, "client", None)
if old_client is not None:
# Best-effort cleanup of the previous client; ignore cleanup errors.
close = getattr(old_client, "close", None)
if callable(close):
try:
close()
except Exception:
# Intentionally ignore to avoid masking the original error path
pass
_thread_local.client = client
return client
def _get_thread_client() -> openmeteo_requests.Client:
if not hasattr(_thread_local, "client"):
# First-time initialization for this thread; no previous client to clean up.
return _set_thread_client(_build_client())
return _thread_local.client
```
To fully address the resource-leak concern on error-triggered client recreation, any code paths that currently do something like:
```python
_thread_local.client = _build_client()
```
for recovery (e.g. in a `DatabaseError` handler) should be updated to instead call:
```python
_set_thread_client(_build_client())
```
This ensures the old client is explicitly cleaned up before being replaced in all cases, not only during first-time initialization in `_get_thread_client()`.
</issue_to_address>
### Comment 3
<location path="src/api.py" line_range="109-110" />
<code_context>
# Current values. The order of variables needs to be the same as requested.
current = response.Current()
+ if current is None:
+ return "No data"
current_uv_index = round(current.Variables(0).Value(), decimal)
</code_context>
<issue_to_address>
**issue (bug_risk):** Returning a string when `current` is None can make the `get_uv` return type inconsistent and brittle for callers.
This change makes callers handle both numeric and string return values, which is easy to misuse (e.g. arithmetic or formatting expecting a number). Prefer a numeric-compatible sentinel (e.g. `None`, `math.nan`, or a reserved numeric code) or handle the "no data" case at a higher layer so this function always returns a consistent type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| openmeteo_client = _create_client() | ||
| def _get_thread_client() -> openmeteo_requests.Client: | ||
| if not hasattr(_thread_local, "client"): | ||
| _thread_local.client = _build_client() |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Catching all sqlite3.DatabaseError and blindly rebuilding the client risks masking persistent cache/DB issues.
Rebuilding the client on any sqlite3.DatabaseError with the same cache path could just hit the same failure again if the DB is corrupted or the filesystem is unhealthy. Consider catching a narrower exception (e.g. OperationalError) and/or adding logic to clear/recreate the cache file, cap the number of rebuild attempts, or ultimately propagate the error so callers can handle a persistent failure appropriately.
Comment on lines
+22
to
+25
| def _get_thread_client() -> openmeteo_requests.Client: | ||
| if not hasattr(_thread_local, "client"): | ||
| _thread_local.client = _build_client() | ||
| return _thread_local.client |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Thread-local client recreation on error may leave underlying sessions/connections unclosed.
When _thread_local.client is replaced after a DatabaseError, the previous client (and its session/cache backend) is discarded without explicit cleanup. If openmeteo_requests.Client/requests_cache hold SQLite connections or file descriptors, this can leak resources under repeated errors. Consider a helper that calls a close()/context-exit on the old client (if available) before assigning a new one.
Suggested implementation:
def _set_thread_client(client: openmeteo_requests.Client) -> openmeteo_requests.Client:
"""Set the thread-local client, closing any existing client if it exposes a close() method.
This helps avoid leaking resources (e.g. SQLite connections/file descriptors) when
the thread-local client is recreated after errors.
"""
old_client = getattr(_thread_local, "client", None)
if old_client is not None:
# Best-effort cleanup of the previous client; ignore cleanup errors.
close = getattr(old_client, "close", None)
if callable(close):
try:
close()
except Exception:
# Intentionally ignore to avoid masking the original error path
pass
_thread_local.client = client
return client
def _get_thread_client() -> openmeteo_requests.Client:
if not hasattr(_thread_local, "client"):
# First-time initialization for this thread; no previous client to clean up.
return _set_thread_client(_build_client())
return _thread_local.clientTo fully address the resource-leak concern on error-triggered client recreation, any code paths that currently do something like:
_thread_local.client = _build_client()for recovery (e.g. in a DatabaseError handler) should be updated to instead call:
_set_thread_client(_build_client())This ensures the old client is explicitly cleaned up before being replaced in all cases, not only during first-time initialization in _get_thread_client().
Comment on lines
+109
to
+110
| if current is None: | ||
| return "No data" |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Returning a string when current is None can make the get_uv return type inconsistent and brittle for callers.
This change makes callers handle both numeric and string return values, which is easy to misuse (e.g. arithmetic or formatting expecting a number). Prefer a numeric-compatible sentinel (e.g. None, math.nan, or a reserved numeric code) or handle the "no data" case at a higher layer so this function always returns a consistent type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
General:
Code:
Summary by Sourcery
Fix server crashes when querying Open-Meteo data by making the HTTP client more resilient and handling empty responses safely.
Bug Fixes:
Enhancements:
Build: