Skip to content

Commit 7c8fb86

Browse files
czeiclaude
andcommitted
fix(http): fail fast when no HTTP client is available (PAL code-review)
A 3-model PAL review of the 0.8.2 changes flagged that the Phase 5 NetworkError("No HTTP client available") raise (added when a get/get_sync has neither an adafruit session nor urllib) was raised INSIDE the retry loop, so the broad `except Exception` caught it and retried 3× with backoff — burning up to ~6s of real time.sleep on the sync path for a permanent, un-retryable configuration state. post()'s no-client path also fell through to `self.urllib.Request(...)` with urllib=None, whose AttributeError got wrapped into a confusing NetworkError message. Add a fail-fast preflight (no adafruit session AND no urllib -> raise NetworkError immediately) before the retry loop in get()/get_sync() and before the try in post(). Desktop always has urllib, so this path only triggers on a genuinely clientless CircuitPython config; existing tests are unaffected (verified: 698 lib + 95 app green). Adds TestNoHttpClientFailsFast covering all three entry points (asserts no retry sleep, and post() raises a clean "No HTTP client available"). Also fixes a stale post() docstring that still referenced the removed `.error` response attribute. The review confirmed the load-bearing invariants held: EBUSY socket detach/close preserved, OTA (ok, reason) tuple contract intact, the web-settings flag handoff sound, and the shared sim-backend wiring order correct. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 2a181e1 commit 7c8fb86

2 files changed

Lines changed: 57 additions & 2 deletions

File tree

‎src/scrollkit/network/http_client.py‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,12 @@ async def get(self, url, headers=None, max_retries=3):
207207
if mock_resp is not None:
208208
return mock_resp
209209

210+
# No HTTP client at all (no adafruit session, no urllib) is a permanent
211+
# configuration state, not a transient blip — fail fast instead of
212+
# burning the retry loop's backoff on it.
213+
if not (self.using_adafruit and self.session) and not self.urllib:
214+
raise NetworkError("No HTTP client available")
215+
210216
while retry_count < max_retries:
211217
try:
212218
if self.using_adafruit and self.session:
@@ -383,7 +389,8 @@ async def post(self, url, data, headers=None):
383389
block the synchronous asyncio loop and trip the watchdog. POST is
384390
single-shot (no retry loop), but a failure is recorded via
385391
``_note_failure`` so a wedged session still gets rebuilt on the next
386-
request, and the cause is surfaced on ``.error`` / ``last_error``.
392+
request, raises ``NetworkError``, and the cause is retained on
393+
``last_error``.
387394
"""
388395
if headers is None:
389396
headers = {
@@ -392,6 +399,11 @@ async def post(self, url, data, headers=None):
392399
}
393400
if isinstance(data, dict):
394401
data = json.dumps(data)
402+
# Raise cleanly BEFORE the try so the "no client" NetworkError isn't
403+
# re-wrapped by the except below (and so the urllib branch never calls
404+
# None.Request(...) -> AttributeError when urllib is absent on device).
405+
if not (self.using_adafruit and self.session) and not self.urllib:
406+
raise NetworkError("No HTTP client available")
395407
try:
396408
if self.using_adafruit and self.session:
397409
resp = None
@@ -438,6 +450,11 @@ def get_sync(self, url, headers=None, max_retries=3):
438450
retry_count = 0
439451
last_error = None
440452

453+
# Fail fast on a permanent no-client state instead of sleeping through
454+
# the retry backoff (blocking real time.sleep on the sync path).
455+
if not (self.using_adafruit and self.session) and not self.urllib:
456+
raise NetworkError("No HTTP client available")
457+
441458
while retry_count < max_retries:
442459
try:
443460
if self.using_adafruit and self.session:

‎test/unit/network/test_http_client_errors.py‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,4 +108,42 @@ async def test_post_request_error(self):
108108
assert mock_logger.return_value.error.called
109109
# The cause is carried in the NetworkError message and on last_error.
110110
assert "Connection error" in str(exc.value)
111-
assert str(client.last_error) == "Connection error"
111+
assert str(client.last_error) == "Connection error"
112+
113+
114+
class TestNoHttpClientFailsFast:
115+
"""A permanent 'no HTTP client' state (no adafruit session AND no urllib)
116+
must raise NetworkError immediately, not burn the retry backoff on a
117+
condition no retry can fix (the sync path's time.sleep is real blocking)."""
118+
119+
def _no_client(self):
120+
client = HttpClient(session=None)
121+
client.using_adafruit = False
122+
client.session = None
123+
client.urllib = None
124+
return client
125+
126+
@pytest.mark.asyncio
127+
async def test_get_raises_immediately_without_retry(self):
128+
client = self._no_client()
129+
with patch('asyncio.sleep', new=AsyncMock()) as slept:
130+
with patch('scrollkit.network.http_client._logger'):
131+
with pytest.raises(NetworkError):
132+
await client.get("https://example.com/api", max_retries=3)
133+
slept.assert_not_called()
134+
135+
def test_get_sync_raises_immediately_without_retry(self):
136+
client = self._no_client()
137+
with patch('time.sleep') as slept:
138+
with patch('scrollkit.network.http_client._logger'):
139+
with pytest.raises(NetworkError):
140+
client.get_sync("https://example.com/api", max_retries=3)
141+
slept.assert_not_called()
142+
143+
@pytest.mark.asyncio
144+
async def test_post_raises_clean_network_error_not_attributeerror(self):
145+
client = self._no_client()
146+
with patch('scrollkit.network.http_client._logger'):
147+
with pytest.raises(NetworkError) as exc:
148+
await client.post("https://example.com/api", data={"a": 1})
149+
assert "No HTTP client available" in str(exc.value)

0 commit comments

Comments
 (0)