Fix peer.py catching httpx exceptions around requests calls (#131)#181
Open
ClaydeCode wants to merge 4 commits into
Open
Fix peer.py catching httpx exceptions around requests calls (#131)#181ClaydeCode wants to merge 4 commits into
ClaydeCode wants to merge 4 commits into
Conversation
update_peer_meta calls requests.get and then response.raise_for_status(), which raises requests.exceptions.HTTPError, but the handler caught httpx.HTTPStatusError. Any non-2xx peer response therefore escaped the handler instead of marking the peer unreachable. Catch the correct exception type and drop the now-unused httpx import. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
update_all_peer_pubkeys gathered update_peer_meta for every peer without return_exceptions, so one peer raising (e.g. an unexpected error not caught inside update_peer_meta) aborted refreshing all remaining peers for that cycle. Collect exceptions instead and log each, so one bad peer cannot starve the others. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Two tests over update_all_peer_pubkeys: a peer returning 500 is marked unreachable without blocking other peers' updates (covers the corrected requests.HTTPError handling), and a peer raising an unexpected error does not abort the refresh cycle for the others (covers gather return_exceptions). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
- Use requests.HTTPError alias to read symmetrically with the adjacent requests.ConnectionError handler. - Insert the good peer as is_reachable=False so the success path's flip to True is actually asserted (was decorative). - Assert both whoareyou requests fired, pinning the HTTPError branch rather than a silent ConnectionError fallback. - Assert the unexpected-error peer is left untouched and that the swallowed failure is surfaced via a warning log. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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
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.
Fixes #131.
Problem
shard_core/service/peer.pyupdate_peer_metacallsrequests.get(...)and thenresponse.raise_for_status(), which raisesrequests.exceptions.HTTPError— but the handler caughthttpx.HTTPStatusError. So any non-2xx peer response escaped the handler, propagated through theasyncio.gatherinupdate_all_peer_pubkeys(which had noreturn_exceptions), and aborted the whole peer-refresh cycle for that tick (only contained byPeriodicTaskretrying a minute later).Change
requests.HTTPErrorinstead ofhttpx.HTTPStatusError, so a non-2xx peer response marks that peer unreachable (as intended) instead of escaping. Dropped the now-unusedhttpximport.return_exceptions=Trueto thegather, with a per-peer warning log, so any unexpected exception from one peer (e.g. a wrong-identityKeyError) can no longer abort refreshing the others.tests/test_peer_refresh.pywith two regression tests overupdate_all_peer_pubkeys.Verification
tests/test_peer_refresh.py— 2 passed. Full suite: 227 passed; the one unrelated failure (test_app_lifecycle.py::test_app_starts_and_stops, a Docker container-status timing test) also fails on a cleanorigin/mainon this runner, so it is pre-existing and not caused by this change.Both new tests were proven to have teeth (each fails when the corresponding half of the fix is reverted):
test_unreachable_peer_does_not_block_othersfails (bad peer no longer marked unreachable),return_exceptions=True→test_unexpected_peer_error_does_not_block_othersfails (gather re-raises),test_unexpected_peer_error_does_not_block_othersfails (warning assertion).Review panel
Three reviewers ran (adversarial always-on; test-adversary because the diff adds real logic + tests; DevEx because it touches tests). No blocking findings from any. Advisories addressed:
is_reachable is Trueassertion (test-adversary, adversarial, DevEx) — fixed: the good peer is now insertedis_reachable=False, so the success path's flip toTrueis actually asserted.assert len(rsps.calls) == 2pins that both whoareyou requests fired.short_idwas emitted.requests.HTTPErrorto read symmetrically with the adjacentrequests.ConnectionErrorhandler.Advisories intentionally not addressed (out of scope for #131, pre-existing): other
requestsfailure modes (Timeout,SSLError) don't mark a peer unreachable;_on_peer_writecallsupdate_peer_metaoutside the guarded gather. Neither is introduced or worsened by this diff.🤖 Generated with Claude Code