fix(url): add timeout to is_valid_url reachability HEAD request - #892
fix(url): add timeout to is_valid_url reachability HEAD request#892eeshsaxena wants to merge 1 commit into
Conversation
requests.head had no timeout, so is_valid_url(url) with the default check_reachability=True could block indefinitely on a slow or unresponsive host. Add a 10s timeout so the reachability check fails cleanly instead of hanging the caller.
|
PR author is not in the allowed authors list. |
Walkthrough
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/notte-core/src/notte_core/utils/url.py`:
- Line 58: Update the reachability logic in is_valid_url to enforce a single
10-second wall-clock deadline across redirect handling and slow HEAD responses,
rather than relying only on requests.head’s per-operation timeout. Add a
regression test that verifies the reachability check returns within the hard
deadline when redirects or a trickle-streaming response exceed it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fd7c5b0d-d030-4a9c-b2bc-2336f59ab7f8
📒 Files selected for processing (1)
packages/notte-core/src/notte_core/utils/url.py
| return True | ||
| # Send a HEAD request to the URL | ||
| response = requests.head(url, allow_redirects=True) | ||
| response = requests.head(url, allow_redirects=True, timeout=10) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'is_valid_url|requests\.head|allow_redirects|timeout=10|deadline|redirect' \
packages/notte-coreRepository: nottelabs/notte
Length of output: 1871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- url.py ---\n'
sed -n '1,90p' packages/notte-core/src/notte_core/utils/url.py
printf '\n--- references to is_valid_url ---\n'
rg -n -C 4 'is_valid_url\(' .
printf '\n--- dependency/version constraints mentioning requests ---\n'
rg -n -C 3 'requests|notte-core|python|dependencies' pyproject.toml packages/notte-core/pyproject.toml packages/notte-core/Pipfile packages/notte-core/requirements*.txt setup.cfg setup.py 2>/dev/null || true
printf '\n--- read-only semantic probe for requests timeout tuple scalar semantics ---\n'
python3 - <<'PY'
import inspect
try:
import requests
except Exception as exc:
print('requests_import_failed', type(exc).__name__, str(exc))
raise SystemExit(0)
print('requests_version', getattr(requests, '__version__', '<unknown>'))
for name in ('Session.request', 'Session.send', 'ConnectionError',):
print(f'hasattr_{name}', hasattr(requests, name))
print(inspect.signature(requests.head))
# The scalar timeout value is the API concern; no request is sent.
PYRepository: nottelabs/notte
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- url.py ---'
sed -n '1,90p' packages/notte-core/src/notte_core/utils/url.py
echo
echo '--- references to is_valid_url ---'
rg -n -C 4 'is_valid_url\(' . || true
echo
echo '--- dependency/version constraints mentioning requests ---'
rg -n -C 3 'requests|notte-core|python|dependencies' pyproject.toml packages/notte-core/pyproject.toml packages/notte-core/Pipfile packages/notte-core/requirements*.txt setup.cfg setup.py 2>/dev/null || true
echo
echo '--- read-only semantic probe for requests timeout scalar semantics ---'
python3 - <<'PY'
try:
import requests
except Exception as exc:
print('requests_import_failed', type(exc).__name__, str(exc))
raise SystemExit(0)
print('requests_version', getattr(requests, '__version__', '<unknown>'))
print('requests.head_signature', requests.head.__globals__.get('Session', None))
import inspect
print(inspect.signature(requests.head))
PYRepository: nottelabs/notte
Length of output: 7103
🌐 Web query:
Python requests head timeout allow_redirects scalar timeout documentation connect read total redirect
💡 Result:
In the Python requests library, the head method and the timeout and allow_redirects parameters function as follows: Timeout The timeout parameter determines how long the client waits for the server to respond before raising a requests.exceptions.Timeout exception [1]. It is not a limit on the entire download time, but rather a limit on how long the client will wait for the server to send data (specifically, the time between received bytes, though in practice it is usually the time until the first byte is received) [2][1]. - Scalar Value: Providing a single float or int (e.g., timeout=5) applies that value to both the connect and read timeouts [2][3]. - Tuple Value: Providing a tuple (e.g., timeout=(3.05, 27)) allows you to set the connect and read timeouts independently, where the first element is the connect timeout and the second is the read timeout [4][2][3]. - Default: By default, requests have no timeout; they will wait indefinitely [2][1]. It is recommended to always specify a timeout in production code [2][1]. Allow_Redirects The allow_redirects parameter is a boolean that enables or disables automatic following of HTTP redirects [4][5]. - Default Behavior: For most methods (e.g., get, post), the default is True [4][6]. - HEAD Method: The requests.head method explicitly overrides this default, setting allow_redirects to False unless otherwise specified [4][7]. You can manually enable redirection for a HEAD request by setting allow_redirects=True [5]. Summary of Parameters - timeout: Optional; float/int (scalar) or (connect, read) tuple [4][6]. - allow_redirects: Optional; bool; defaults to False for head, True for others [4][7][5].
Citations:
- 1: https://requests.readthedocs.io/en/latest/user/quickstart/?highlight=timeout
- 2: https://requests.readthedocs.io/en/latest/user/advanced/?highlight=timeout
- 3: https://requests.readthedocs.io/en/master/user/advanced/
- 4: https://requests.readthedocs.io/en/latest/api/index.html
- 5: https://docs.python-requests.org/en/latest/user/quickstart/
- 6: https://requests.readthedocs.io/en/stable/api/
- 7: https://github.com/psf/requests/blob/main/src/requests/api.py
Enforce a wall-clock bound for the reachability check.
timeout=10 limits connect/read inactivity per request, not the full time that is_valid_url(..., check_reachability=True) can spend following redirects or a trickle-streaming HEAD response. If callers require a hard 10-second reachability deadline, add an explicit deadline implementation across redirects and cover it with a regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/notte-core/src/notte_core/utils/url.py` at line 58, Update the
reachability logic in is_valid_url to enforce a single 10-second wall-clock
deadline across redirect handling and slow HEAD responses, rather than relying
only on requests.head’s per-operation timeout. Add a regression test that
verifies the reachability check returns within the hard deadline when redirects
or a trickle-streaming response exceed it.
1 participant
Problem
is_valid_urlinnotte_core/utils/url.pydoes its reachability check withrequests.head(url, allow_redirects=True)and notimeout.requestshas no default timeout, so the call can block indefinitely on a slow or unresponsive host.The one internal caller (
notte-browser/window.py) passescheck_reachability=False, so it does not hit this path, butcheck_reachability=Trueis the function's default, so any caller using the default (including SDK users) is exposed.Fix
Add a
timeout=10to the HEAD request so an unresponsive URL fails the reachability check cleanly instead of hanging the caller.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit