fix(bib): tag attachment:gone when CDN serves 200 + empty body
regulations.gov sometimes returns 200 OK with Content-Length: 0 for attachment URLs whose files have been removed from object storage but whose comment metadata still references them. download_attachment was silently writing the 0-byte response to disk, registering it as a real attachment, and refusing to refetch on retry (cache hit on the empty file). Five comments in bib.sqlite ended up with phantom attachments. - download_attachment raises new AttachmentMissing on 0-byte body and rejects cached 0-byte files instead of returning the stale path - download_attachments_batch returns (paths, missing_urls); transient HTTP/transport failures are still untracked and get retried next run - backfill_details tags affected items 'attachment:gone' (mirrors the existing 'enriched:gone' convention for 404'd comments) and counts them in a new att_gone stats field Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -44,6 +44,16 @@ log = logging.getLogger(__name__)
|
|||||||
_BASE = "https://api.regulations.gov/v4"
|
_BASE = "https://api.regulations.gov/v4"
|
||||||
|
|
||||||
|
|
||||||
|
class AttachmentMissing(Exception):
|
||||||
|
"""CDN returned 200 OK with an empty body for an attachment URL.
|
||||||
|
|
||||||
|
Means the file has been removed from object storage but the comment
|
||||||
|
metadata still references it — permanently unrecoverable. Distinct
|
||||||
|
from transient HTTP/transport errors, which get no exception and
|
||||||
|
are retried on the next backfill run.
|
||||||
|
"""
|
||||||
|
|
||||||
|
|
||||||
# ── Value objects ──────────────────────────────────────────────
|
# ── Value objects ──────────────────────────────────────────────
|
||||||
|
|
||||||
|
|
||||||
@@ -297,7 +307,7 @@ class Client:
|
|||||||
dest_dir.mkdir(parents=True, exist_ok=True)
|
dest_dir.mkdir(parents=True, exist_ok=True)
|
||||||
filename = _filename_from(url) or _hash(url) + ".bin"
|
filename = _filename_from(url) or _hash(url) + ".bin"
|
||||||
dest = dest_dir / filename
|
dest = dest_dir / filename
|
||||||
if dest.is_file() and not overwrite:
|
if dest.is_file() and dest.stat().st_size > 0 and not overwrite:
|
||||||
return dest
|
return dest
|
||||||
# No sleep — CDN is separate from the API rate limit.
|
# No sleep — CDN is separate from the API rate limit.
|
||||||
try:
|
try:
|
||||||
@@ -321,28 +331,43 @@ class Client:
|
|||||||
except httpx.HTTPError as e:
|
except httpx.HTTPError as e:
|
||||||
log.warning("attachment %s failed: %s", url, e)
|
log.warning("attachment %s failed: %s", url, e)
|
||||||
return None
|
return None
|
||||||
|
if dest.stat().st_size == 0:
|
||||||
|
log.warning("attachment %s → empty body, skipping", url.rsplit("/", 1)[-1])
|
||||||
|
dest.unlink()
|
||||||
|
raise AttachmentMissing(url)
|
||||||
return dest
|
return dest
|
||||||
|
|
||||||
def download_attachments_batch(
|
def download_attachments_batch(
|
||||||
self,
|
self,
|
||||||
urls: list[str],
|
urls: list[str],
|
||||||
dest_dir: Path,
|
dest_dir: Path,
|
||||||
) -> list[Path]:
|
) -> tuple[list[Path], list[str]]:
|
||||||
"""Download multiple attachments concurrently. Returns paths of
|
"""Download multiple attachments concurrently.
|
||||||
successfully downloaded files."""
|
|
||||||
|
Returns ``(paths, missing_urls)`` where ``missing_urls`` are URLs
|
||||||
|
the CDN served as 200 OK with an empty body — verified gone from
|
||||||
|
object storage. Transient failures (HTTP errors, timeouts) are
|
||||||
|
not in either list; they get retried on the next backfill run.
|
||||||
|
"""
|
||||||
if not urls:
|
if not urls:
|
||||||
return []
|
return [], []
|
||||||
results: list[Path] = []
|
results: list[Path] = []
|
||||||
|
missing: list[str] = []
|
||||||
with ThreadPoolExecutor(max_workers=self._dl_workers) as pool:
|
with ThreadPoolExecutor(max_workers=self._dl_workers) as pool:
|
||||||
futures = {
|
futures = {
|
||||||
pool.submit(self.download_attachment, url, dest_dir): url
|
pool.submit(self.download_attachment, url, dest_dir): url
|
||||||
for url in urls
|
for url in urls
|
||||||
}
|
}
|
||||||
for fut in as_completed(futures):
|
for fut in as_completed(futures):
|
||||||
path = fut.result()
|
url = futures[fut]
|
||||||
|
try:
|
||||||
|
path = fut.result()
|
||||||
|
except AttachmentMissing:
|
||||||
|
missing.append(url)
|
||||||
|
continue
|
||||||
if path:
|
if path:
|
||||||
results.append(path)
|
results.append(path)
|
||||||
return results
|
return results, missing
|
||||||
|
|
||||||
|
|
||||||
# ── Bib integration ────────────────────────────────────────────
|
# ── Bib integration ────────────────────────────────────────────
|
||||||
@@ -395,7 +420,14 @@ def backfill_details(
|
|||||||
+ (f" LIMIT {int(limit)}" if limit else "")
|
+ (f" LIMIT {int(limit)}" if limit else "")
|
||||||
).fetchall()
|
).fetchall()
|
||||||
|
|
||||||
stats = {"enriched": 0, "attached": 0, "gone": 0, "errors": 0, "seen": len(rows)}
|
stats = {
|
||||||
|
"enriched": 0,
|
||||||
|
"attached": 0,
|
||||||
|
"att_gone": 0,
|
||||||
|
"gone": 0,
|
||||||
|
"errors": 0,
|
||||||
|
"seen": len(rows),
|
||||||
|
}
|
||||||
|
|
||||||
def _write_log(msg: str) -> None:
|
def _write_log(msg: str) -> None:
|
||||||
log.info(msg)
|
log.info(msg)
|
||||||
@@ -456,13 +488,23 @@ def backfill_details(
|
|||||||
att_url = f.get("fileUrl") or ""
|
att_url = f.get("fileUrl") or ""
|
||||||
if att_url:
|
if att_url:
|
||||||
att_urls.append(att_url)
|
att_urls.append(att_url)
|
||||||
for path in client.download_attachments_batch(att_urls, dest_dir):
|
paths: list[Path] = []
|
||||||
|
missing_urls: list[str] = []
|
||||||
|
if att_urls:
|
||||||
|
paths, missing_urls = client.download_attachments_batch(att_urls, dest_dir)
|
||||||
|
for path in paths:
|
||||||
try:
|
try:
|
||||||
store.attach_file(key, path, title=path.name)
|
store.attach_file(key, path, title=path.name)
|
||||||
stats["attached"] += 1
|
stats["attached"] += 1
|
||||||
except Exception as e: # noqa: BLE001
|
except Exception as e: # noqa: BLE001
|
||||||
# Usually a dup filename; fine to skip.
|
# Usually a dup filename; fine to skip.
|
||||||
log.debug("attach_file skipped for %s/%s: %s", key, path.name, e)
|
log.debug("attach_file skipped for %s/%s: %s", key, path.name, e)
|
||||||
|
if missing_urls:
|
||||||
|
try:
|
||||||
|
store.add_tag(key, "attachment:gone")
|
||||||
|
stats["att_gone"] += 1
|
||||||
|
except Exception as e: # noqa: BLE001
|
||||||
|
log.warning("add_tag attachment:gone failed for %s: %s", key, e)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
store.add_tag(key, "enriched:ok")
|
store.add_tag(key, "enriched:ok")
|
||||||
@@ -475,6 +517,7 @@ def backfill_details(
|
|||||||
f" {i}/{len(rows)} "
|
f" {i}/{len(rows)} "
|
||||||
f"enriched={stats['enriched']} "
|
f"enriched={stats['enriched']} "
|
||||||
f"attached={stats['attached']} "
|
f"attached={stats['attached']} "
|
||||||
|
f"att_gone={stats['att_gone']} "
|
||||||
f"gone={stats['gone']} errors={stats['errors']}"
|
f"gone={stats['gone']} errors={stats['errors']}"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|||||||
@@ -246,13 +246,51 @@ class TestBackfillDetails:
|
|||||||
}
|
}
|
||||||
],
|
],
|
||||||
}
|
}
|
||||||
api.download_attachments_batch.return_value = [tmp_path / "file.pdf"]
|
api.download_attachments_batch.return_value = ([tmp_path / "file.pdf"], [])
|
||||||
|
|
||||||
stats = backfill_details(
|
stats = backfill_details(
|
||||||
store, api, limit=1, scratch_root=tmp_path, commit_every=1
|
store, api, limit=1, scratch_root=tmp_path, commit_every=1
|
||||||
)
|
)
|
||||||
assert stats["attached"] >= 1
|
assert stats["attached"] >= 1
|
||||||
|
|
||||||
|
def test_attachment_gone_tags_item(self, tmp_path):
|
||||||
|
"""CDN serves 200 OK with empty body → item gets attachment:gone tag."""
|
||||||
|
store = MagicMock()
|
||||||
|
con = MagicMock()
|
||||||
|
store._con.return_value = con
|
||||||
|
con.execute.return_value.fetchall.return_value = [
|
||||||
|
{
|
||||||
|
"id": 1,
|
||||||
|
"key": "K1",
|
||||||
|
"url": "https://www.regulations.gov/comment/CMS-2023-0001-0001",
|
||||||
|
},
|
||||||
|
]
|
||||||
|
|
||||||
|
api = MagicMock()
|
||||||
|
api.get_comment_detail.return_value = {
|
||||||
|
"data": {"attributes": {"comment": "text", "organization": ""}},
|
||||||
|
"included": [
|
||||||
|
{
|
||||||
|
"type": "attachments",
|
||||||
|
"attributes": {
|
||||||
|
"fileFormats": [{"fileUrl": "https://example.com/gone.pdf"}]
|
||||||
|
},
|
||||||
|
}
|
||||||
|
],
|
||||||
|
}
|
||||||
|
api.download_attachments_batch.return_value = (
|
||||||
|
[],
|
||||||
|
["https://example.com/gone.pdf"],
|
||||||
|
)
|
||||||
|
|
||||||
|
stats = backfill_details(
|
||||||
|
store, api, limit=1, scratch_root=tmp_path, commit_every=1
|
||||||
|
)
|
||||||
|
assert stats["att_gone"] == 1
|
||||||
|
assert stats["attached"] == 0
|
||||||
|
store.add_tag.assert_any_call("K1", "attachment:gone")
|
||||||
|
store.add_tag.assert_any_call("K1", "enriched:ok")
|
||||||
|
|
||||||
def test_500_error_skips(self):
|
def test_500_error_skips(self):
|
||||||
store = MagicMock()
|
store = MagicMock()
|
||||||
con = MagicMock()
|
con = MagicMock()
|
||||||
@@ -470,6 +508,7 @@ class TestBackfillDetailsOrgTag:
|
|||||||
],
|
],
|
||||||
}
|
}
|
||||||
api.download_attachment.return_value = None # download fails
|
api.download_attachment.return_value = None # download fails
|
||||||
|
api.download_attachments_batch.return_value = ([], []) # all transient fails
|
||||||
|
|
||||||
stats = backfill_details(
|
stats = backfill_details(
|
||||||
store, api, limit=1, scratch_root=tmp_path, commit_every=1
|
store, api, limit=1, scratch_root=tmp_path, commit_every=1
|
||||||
@@ -544,6 +583,46 @@ class TestClientDownloadEdge:
|
|||||||
result = c.download_attachment("https://x.com/new.pdf", tmp_path)
|
result = c.download_attachment("https://x.com/new.pdf", tmp_path)
|
||||||
assert result is None
|
assert result is None
|
||||||
|
|
||||||
|
@patch.dict("os.environ", {"REGULATIONS_GOV_API_KEY": "test"})
|
||||||
|
@patch("bib.regulations_gov.time.sleep")
|
||||||
|
def test_empty_body_raises_attachment_missing(self, mc_sleep, tmp_path):
|
||||||
|
"""200 OK with 0-byte body → raises AttachmentMissing, deletes file."""
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from bib.regulations_gov import AttachmentMissing
|
||||||
|
|
||||||
|
mock_http = MagicMock()
|
||||||
|
stream_ctx = MagicMock()
|
||||||
|
stream_ctx.__enter__ = MagicMock(return_value=stream_ctx)
|
||||||
|
stream_ctx.__exit__ = MagicMock(return_value=False)
|
||||||
|
stream_ctx.status_code = 200
|
||||||
|
stream_ctx.iter_bytes.return_value = iter([]) # empty body
|
||||||
|
mock_http.stream.return_value = stream_ctx
|
||||||
|
c = Client(sleep=0, client=mock_http)
|
||||||
|
url = "https://x.com/empty.pdf"
|
||||||
|
with pytest.raises(AttachmentMissing):
|
||||||
|
c.download_attachment(url, tmp_path)
|
||||||
|
assert not (tmp_path / "empty.pdf").exists()
|
||||||
|
|
||||||
|
@patch.dict("os.environ", {"REGULATIONS_GOV_API_KEY": "test"})
|
||||||
|
@patch("bib.regulations_gov.time.sleep")
|
||||||
|
def test_existing_zero_byte_file_re_downloads(self, mc_sleep, tmp_path):
|
||||||
|
"""Cached 0-byte file is rejected — refetch happens instead of returning the stale empty path."""
|
||||||
|
f = tmp_path / "stale.pdf"
|
||||||
|
f.write_bytes(b"") # 0 bytes
|
||||||
|
mock_http = MagicMock()
|
||||||
|
stream_ctx = MagicMock()
|
||||||
|
stream_ctx.__enter__ = MagicMock(return_value=stream_ctx)
|
||||||
|
stream_ctx.__exit__ = MagicMock(return_value=False)
|
||||||
|
stream_ctx.status_code = 200
|
||||||
|
stream_ctx.iter_bytes.return_value = iter([b"real bytes"])
|
||||||
|
mock_http.stream.return_value = stream_ctx
|
||||||
|
c = Client(sleep=0, client=mock_http)
|
||||||
|
result = c.download_attachment("https://x.com/stale.pdf", tmp_path)
|
||||||
|
assert result == f
|
||||||
|
assert f.read_bytes() == b"real bytes"
|
||||||
|
mock_http.stream.assert_called_once()
|
||||||
|
|
||||||
|
|
||||||
class TestIterCommentsPageAdvance:
|
class TestIterCommentsPageAdvance:
|
||||||
@patch.dict("os.environ", {"REGULATIONS_GOV_API_KEY": "test"})
|
@patch.dict("os.environ", {"REGULATIONS_GOV_API_KEY": "test"})
|
||||||
@@ -689,6 +768,7 @@ class TestBackfillAddTagGone:
|
|||||||
],
|
],
|
||||||
}
|
}
|
||||||
api.download_attachment.return_value = tmp_path / "doc.pdf"
|
api.download_attachment.return_value = tmp_path / "doc.pdf"
|
||||||
|
api.download_attachments_batch.return_value = ([tmp_path / "doc.pdf"], [])
|
||||||
|
|
||||||
stats = backfill_details(
|
stats = backfill_details(
|
||||||
store, api, limit=1, scratch_root=tmp_path, commit_every=1
|
store, api, limit=1, scratch_root=tmp_path, commit_every=1
|
||||||
|
|||||||
Reference in New Issue
Block a user