fix: correct exception handling in get_external_ip fallback chain#3311
fix: correct exception handling in get_external_ip fallback chain#3311Grizouforever wants to merge 2 commits intolatent-to:masterfrom
Conversation
The fallback chain in get_external_ip() catches ExternalIPNotFound but none of the code inside the try blocks raises that exception. The actual exceptions (requests.RequestException, AssertionError, OSError, json.JSONDecodeError, etc.) propagate unhandled, preventing fallthrough to the next IP provider. Changes: - Replace all `except ExternalIPNotFound` with `except Exception` to properly catch real exceptions and allow fallthrough - Replace deprecated `os.popen()` calls with `subprocess.run()` - Replace bare `assert` validation with direct `ip_to_int()` calls which raise on invalid input regardless of -O flag Fixes latent-to#3309
The implementation in bittensor/utils/networking.py was updated in this
PR to replace os.popen("curl ...") with subprocess.run(...), but the
test test_get_external_ip_os_request_urllib_broken still mocked os.popen
which no longer intercepts any calls. Update the test to mock
subprocess.run and bittensor.utils.networking.urllib_request.urlopen so
all fallback paths are properly blocked and the expected ExternalIPNotFound
exception is raised.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Read CONTRIBUTING |
| @@ -1,6 +1,7 @@ | |||
| """Utils for handling local network with ip and ports.""" | |||
|
|
|||
| import os | |||
| """Utils for handling local network with ip and ports.""" | ||
|
|
||
| import os | ||
| import subprocess |
There was a problem hiding this comment.
why use subprocess when requests is available?
| result = subprocess.run( | ||
| ["curl", "-s", "ifconfig.me"], capture_output=True, text=True, timeout=10 | ||
| ) |
There was a problem hiding this comment.
subprocess + curl -> requests.get
| def mock_subprocess_run(*args, **kwargs): | ||
| raise OSError("subprocess.run mocked to fail") | ||
|
|
||
| class FakeResponse: |
There was a problem hiding this comment.
isn't FakeResponse dead now?
There was a problem hiding this comment.
Two things missing on the test side:
First: test_get_external_ip_os_broken still patches os.popen, but this PR removes all os.popen usage from production code. That mock now does nothing; the test passes solely because requests.get is mocked to succeed on the first try. The test name and docstring say "when os.popen is broken," but it no longer tests that scenario.
Pls update it to mock subprocess.run failing (or whichever replacement we land on) while keeping requests.get succeeding, so it actually validates "first method works, broken fallback methods don't interfere."
Second: there's no test for the cascade itself. For example: mock requests.get to raise, mock the first subprocess.run call to return a valid IP -> assert the function returns that IP. This is the core contract of get_external_ip() - "try multiple sources in order" - and it has zero coverage today.
3 participants
Summary
Fixes #3309
The fallback chain in
get_external_ip()catchesExternalIPNotFound, but none of the code inside the try blocks actually raises that exception. Real exceptions (requests.RequestException,AssertionError,OSError,json.JSONDecodeError, etc.) propagate unhandled, preventing fallthrough to the next IP provider. This means if the first provider (AWS checkip) fails, the function crashes instead of trying the remaining 5 providers.Changes
bittensor/utils/networking.py:except ExternalIPNotFoundwithexcept Exceptionto properly catch real exceptionsos.popen()calls withsubprocess.run()(Python 3.0+ recommended)assert isinstance(...)validation with directip_to_int()calls that raise on invalid input regardless of-Ooptimization flagContext
PR #3262 previously attempted this fix but was closed due to being based on the wrong branch and author not responding to review. This PR is based on
master(the current default branch) and incorporates the additional improvements suggested.