bhoot found that download_cdfer 416-loops when a complete cdfer.sqlite3 already exists in the cache (e.g. a prior run downloaded it but died before conversion/cleanup): resuming a complete file sends Range: bytes=<size>- which the server answers with HTTP 416, and raise_for_status treated that as a retryable error, spinning until max_retries then failing. Fix: HEAD once for Content-Length up front and short-circuit when the local file already matches the full size; and in the loop, treat a 416 as "complete" when the size matches (else drop the stale partial and restart fresh). Remove the now -unused _head_last_modified helper (HEAD is done inline). Add a no-network test. Verified live: happy path still downloads+converts 616k parts (~50s); a pre-existing complete file now short-circuits in ~0.4s instead of looping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -314,18 +314,6 @@ def convert_source_sqlite(
|
|||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
def _head_last_modified(url: str) -> Optional[str]:
|
|
||||||
try:
|
|
||||||
import requests
|
|
||||||
|
|
||||||
resp = requests.head(url, allow_redirects=True, timeout=30)
|
|
||||||
if resp.ok:
|
|
||||||
return resp.headers.get("Last-Modified")
|
|
||||||
except Exception as exc: # network/dns/etc.
|
|
||||||
logger.debug(f"HEAD {url} failed: {exc}")
|
|
||||||
return None
|
|
||||||
|
|
||||||
|
|
||||||
def download_cdfer(
|
def download_cdfer(
|
||||||
cache_dir: Path, progress: ProgressFn = None, max_retries: int = 5
|
cache_dir: Path, progress: ProgressFn = None, max_retries: int = 5
|
||||||
) -> Tuple[Path, Optional[str]]:
|
) -> Tuple[Path, Optional[str]]:
|
||||||
@@ -340,7 +328,27 @@ def download_cdfer(
|
|||||||
|
|
||||||
cache_dir.mkdir(parents=True, exist_ok=True)
|
cache_dir.mkdir(parents=True, exist_ok=True)
|
||||||
dest = cache_dir / "cdfer.sqlite3"
|
dest = cache_dir / "cdfer.sqlite3"
|
||||||
last_modified = _head_last_modified(CDFER_SQLITE_URL)
|
|
||||||
|
# HEAD once for the freshness date AND the total size, so we can detect a
|
||||||
|
# cache file that is already complete (e.g. a prior run downloaded it but
|
||||||
|
# died before conversion/cleanup) and skip re-downloading. Without this,
|
||||||
|
# resuming a complete file sends Range: bytes=<size>- which the server
|
||||||
|
# answers with HTTP 416, and the retry loop would spin on it until failure.
|
||||||
|
last_modified: Optional[str] = None
|
||||||
|
total_size: Optional[int] = None
|
||||||
|
try:
|
||||||
|
head = requests.head(CDFER_SQLITE_URL, allow_redirects=True, timeout=30)
|
||||||
|
if head.ok:
|
||||||
|
last_modified = head.headers.get("Last-Modified")
|
||||||
|
cl = head.headers.get("Content-Length")
|
||||||
|
total_size = int(cl) if cl and cl.isdigit() else None
|
||||||
|
except requests.RequestException as exc:
|
||||||
|
logger.debug(f"HEAD {CDFER_SQLITE_URL} failed: {exc}")
|
||||||
|
|
||||||
|
if total_size and dest.exists() and dest.stat().st_size == total_size:
|
||||||
|
_progress(progress, "CDFER SQLite already fully downloaded; skipping download.")
|
||||||
|
return dest, last_modified
|
||||||
|
|
||||||
_progress(
|
_progress(
|
||||||
progress,
|
progress,
|
||||||
"Downloading CDFER prebuilt SQLite (~1.5 GB)"
|
"Downloading CDFER prebuilt SQLite (~1.5 GB)"
|
||||||
@@ -359,6 +367,16 @@ def download_cdfer(
|
|||||||
with requests.get(
|
with requests.get(
|
||||||
CDFER_SQLITE_URL, stream=True, timeout=(30, 120), headers=headers
|
CDFER_SQLITE_URL, stream=True, timeout=(30, 120), headers=headers
|
||||||
) as resp:
|
) as resp:
|
||||||
|
# HTTP 416: our partial is at/past the resource end. If it matches
|
||||||
|
# the known total it's genuinely complete — done. Otherwise the
|
||||||
|
# cached partial is stale/corrupt, so drop it and restart fresh.
|
||||||
|
if resume_from and resp.status_code == 416:
|
||||||
|
if total_size and dest.exists() and dest.stat().st_size == total_size:
|
||||||
|
_progress(progress, "Existing file already complete; download done.")
|
||||||
|
break
|
||||||
|
_progress(progress, "Cached partial invalid (range rejected); restarting.")
|
||||||
|
dest.unlink(missing_ok=True)
|
||||||
|
continue
|
||||||
# If we asked to resume but the server ignored Range (200, not
|
# If we asked to resume but the server ignored Range (200, not
|
||||||
# 206), start the file over to avoid a corrupt append.
|
# 206), start the file over to avoid a corrupt append.
|
||||||
if resume_from and resp.status_code == 200:
|
if resume_from and resp.status_code == 200:
|
||||||
|
|||||||
@@ -266,3 +266,39 @@ def test_result_flags_stale_catalog_and_leaves_fresh_alone(tmp_path, monkeypatch
|
|||||||
assert fresh.get("stale") is not True
|
assert fresh.get("stale") is not True
|
||||||
assert "warning" not in fresh
|
assert "warning" not in fresh
|
||||||
assert fresh["catalog_age_days"] <= 1
|
assert fresh["catalog_age_days"] <= 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_download_cdfer_skips_when_file_already_complete(tmp_path, monkeypatch):
|
||||||
|
"""A complete cache file must short-circuit, not re-download (no HTTP 416 loop).
|
||||||
|
|
||||||
|
Regression for the force/resume bug: resuming a complete file sends
|
||||||
|
Range: bytes=<size>- which the server answers with 416; the old code retried
|
||||||
|
that until failure. With a HEAD size check we return immediately and never
|
||||||
|
call requests.get.
|
||||||
|
"""
|
||||||
|
import requests
|
||||||
|
|
||||||
|
cache_dir = tmp_path / "cache"
|
||||||
|
cache_dir.mkdir()
|
||||||
|
dest = cache_dir / "cdfer.sqlite3"
|
||||||
|
payload = b"x" * 4096
|
||||||
|
dest.write_bytes(payload)
|
||||||
|
|
||||||
|
class _Head:
|
||||||
|
ok = True
|
||||||
|
headers = {
|
||||||
|
"Content-Length": str(len(payload)),
|
||||||
|
"Last-Modified": "Wed, 01 Apr 2026 00:00:00 GMT",
|
||||||
|
}
|
||||||
|
|
||||||
|
monkeypatch.setattr(requests, "head", lambda *a, **k: _Head())
|
||||||
|
|
||||||
|
def _no_get(*a, **k):
|
||||||
|
raise AssertionError("requests.get must not run when the file is already complete")
|
||||||
|
|
||||||
|
monkeypatch.setattr(requests, "get", _no_get)
|
||||||
|
|
||||||
|
path, last_mod = jlcpcb_downloader.download_cdfer(cache_dir)
|
||||||
|
assert path == dest
|
||||||
|
assert path.stat().st_size == len(payload) # untouched
|
||||||
|
assert last_mod.startswith("Wed, 01 Apr 2026")
|
||||||
|
|||||||
Reference in New Issue
Block a user