Skip to content
This repository was archived by the owner on Jul 10, 2026. It is now read-only.

fix(networking): catch real exceptions in get_external_ip fallback chain - #3318

Merged
basfroman merged 5 commits into
RaoFoundation:stagingfrom
ArtificialXai:fix/artificialxai/networking-ip-fallback-chain
Apr 30, 2026
Merged

fix(networking): catch real exceptions in get_external_ip fallback chain#3318
basfroman merged 5 commits into
RaoFoundation:stagingfrom
ArtificialXai:fix/artificialxai/networking-ip-fallback-chain

Conversation

@ArtificialXai

Copy link
Copy Markdown
Contributor

Summary

Closes #3309.

Each provider in get_external_ip is wrapped in except ExternalIPNotFound, but none of the underlying calls (requests.get, ip_to_int, os.popen + readline, urllib.request.urlopen, json.loads, the Wikipedia header lookup) ever raise ExternalIPNotFound. As a result the first failing provider crashes the entire function instead of falling through to the next one — exactly the symptom reported in the issue (a requests.exceptions.ConnectionError propagates out when AWS is unreachable, rather than the curl fallback being attempted).

Fix

Replace each except ExternalIPNotFound: with a tuple of exceptions that the providers actually raise:

Exception Where it originates
requests.exceptions.RequestException requests.get(...) (AWS, Wikipedia)
netaddr.core.AddrFormatError ip_to_int(...) with malformed input
OSError os.popen(...) + readline() (ifconfig.me, ipinfo.io, dnsomatic)
urllib.error.URLError urllib_request.urlopen(...) (ident.me)
ValueError json.loads(...) parse failures, int(...) parse failures
KeyError Wikipedia response missing X-Client-IP header
AssertionError assert isinstance(ip_to_int(...), int) guard
ExternalIPNotFound Kept for backward compatibility with any wrapper that raises it

The set is defined once at module scope as _IP_LOOKUP_EXCEPTIONS so the providers stay in sync.

Tests

Three new regression tests in tests/unit_tests/utils/test_networking.py:

  • test_get_external_ip_aws_connection_error_falls_through_to_curl — AWS requests.get raises ConnectionError, curl fallback returns a valid IP, function returns that IP (was: raised ConnectionError).
  • test_get_external_ip_malformed_ip_falls_through — AWS returns a non-IP string (triggers AddrFormatError in ip_to_int), curl fallback succeeds.
  • test_get_external_ip_all_providers_exhausted_raises — every provider raises its real exception; function must raise ExternalIPNotFound (not leak the last underlying exception).

All 22 tests in tests/unit_tests/utils/test_networking.py pass locally.

Test plan

  • pytest tests/unit_tests/utils/test_networking.py — all 22 pass
  • CI run on PR

Notes

  • The existing assert isinstance(ip_to_int(external_ip), int) line is tautological (ip_to_int either returns int or raises), but left unchanged to keep this PR narrow and focused on the reported bug.
  • No behavior change when every provider succeeds — only the error-path taxonomy is corrected.

@basfroman basfroman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ArtificialXai, pls read CONTRIBUTING.md to move forward with your pr.

@ArtificialXai
ArtificialXai force-pushed the fix/artificialxai/networking-ip-fallback-chain branch from 3dea36d to 7f31808 Compare April 20, 2026 21:13
@ArtificialXai
ArtificialXai changed the base branch from master to staging April 20, 2026 21:13
@ArtificialXai

Copy link
Copy Markdown
Contributor Author

Thanks for the pointer, @basfroman. Read CONTRIBUTING.md and updated the PR:

  • Rebased onto staging and changed the PR base (was master).
  • Rewrote the commit message to match the 6-rule style (subject <50 chars, body wrapped at 72, explains what and why).
  • PR is still <50 files (2 files, 96/+6/-); the 3 regression tests in tests/unit_tests/utils/test_networking.py cover the real exception paths.

Ready for another look when you have a minute.

@thewhaleking

Copy link
Copy Markdown
Contributor

Thanks for the pointer, @basfroman. Read CONTRIBUTING.md and updated the PR:

  • Rebased onto staging and changed the PR base (was master).
  • Rewrote the commit message to match the 6-rule style (subject <50 chars, body wrapped at 72, explains what and why).
  • PR is still <50 files (2 files, 96/+6/-); the 3 regression tests in tests/unit_tests/utils/test_networking.py cover the real exception paths.

Ready for another look when you have a minute.

Your commit still isn't signed

Closes RaoFoundation#3309.

Each provider in `get_external_ip` was wrapped in
`except ExternalIPNotFound`, but none of the underlying calls
(`requests.get`, `ip_to_int`, `os.popen`+`readline`,
`urllib.request.urlopen`, `json.loads`, wikipedia header lookup)
ever raise `ExternalIPNotFound`. As a result the first failing
provider crashed the entire function instead of falling through
to the next one.

Catch the exceptions that can actually occur:
  * RequestException  — for `requests.get` failures
  * AddrFormatError   — for malformed IPs
  * OSError           — for `os.popen`/curl failures
  * URLError          — for urllib failures
  * ValueError        — for json + int parse failures
  * KeyError          — for the wikipedia header
  * AssertionError    — for the isinstance guards
  * ExternalIPNotFound — kept for backward compat

Three regression tests cover: AWS connection error → curl
fallthrough, malformed AWS IP → curl fallthrough, and
all-providers-exhausted → ExternalIPNotFound.
@ArtificialXai
ArtificialXai force-pushed the fix/artificialxai/networking-ip-fallback-chain branch from 7f31808 to d361f28 Compare April 24, 2026 15:08
@ArtificialXai

Copy link
Copy Markdown
Contributor Author

Signed and re-pushed (d361f287).

@thewhaleking
thewhaleking requested a review from basfroman April 24, 2026 15:10
@ArtificialXai

Copy link
Copy Markdown
Contributor Author

Hey @basfroman, friendly ping — addressed all your feedback in the 2026-04-20 comment (rebased onto staging, commit message follows 6-rule style, tests cover the exceptions). Commit is now signed as of today (2026-04-24). Could you take another look when you get a moment? Thanks!

@basfroman
basfroman merged commit 5e832b1 into RaoFoundation:staging Apr 30, 2026
695 checks passed
@ibraheem-abe ibraheem-abe mentioned this pull request May 6, 2026
RUNECTZ33 added a commit to RUNECTZ33/bittensor that referenced this pull request May 13, 2026
`AxonMiddleware.preprocess` previously wrapped any `Exception` from URL
path parsing and `Synapse.from_headers` as `InvalidRequestNameError` /
`SynapseParsingError`. This swallowed unrelated infrastructure failures
(e.g. `RuntimeError`, `MemoryError`, real bugs in starlette or pydantic
plugins) and lost their original traceback, surfacing them as misleading
"malformed request" errors.

- Narrow the path-parse catch to `(IndexError, AttributeError)` — the
  only failures `request.url.path.split("/")[1]` can realistically
  produce.
- Narrow the synapse-parse catch to `(ValidationError, TypeError,
  ValueError)` — the realistic failure modes of pydantic model
  construction from header inputs.
- `raise ... from e` on both paths so the original exception remains on
  `__cause__` for debugging.
- Regression tests assert that `ValidationError` is wrapped with
  `__cause__` set and that an unrelated `RuntimeError` propagates
  unchanged.

Mirrors the exception-narrowing pattern used in RaoFoundation#3318 for
`get_external_ip`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_external_ip fallback chain never triggers — all attempts catch ExternalIPNotFound but none of the called functions raise it

3 participants