fix(cache): cache rules never matched data.sec.gov, or a mirror's own host - #989
fix(cache): cache rules never matched data.sec.gov, or a mirror's own host#989joseturegano wants to merge 4 commits into
Conversation
|
CI on this PR is red with 2 failures, and since both touch The failures are DNS, not assertions: Both retried 3 times and failed all 3 with the same resolver error. Locally, on this branch, with real network, both pass: Why I don't think the change can cause this: I can't rerun the job (no admin rights on this repo), so I can't demonstrate flakiness directly. If you'd rather have positive proof than my reasoning, a rerun of the failed jobs would settle it — and if it comes back red the same way, I'll dig further rather than leave it at "flaky". Worth noting the PR is a draft for a separate reason (the upstream clock-skew crash in the Blocking finding section), so there's no rush on this either way. |
dgunning
left a comment
There was a problem hiding this comment.
Verified the whole chain independently — the diagnosis and the fix both hold up. One correction to the history in the description, plus two small notes.
Confirmed
Reproduced against 0.6.0, the version pyproject.toml actually requires (worth flagging: system Python here had 0.3.0, so this needs checking inside the project env or you get the wrong answer):
edgartools CACHE_RULES key: ['.*www\\.sec\\.gov']
www.sec.gov /submissions/CIK0000320193.json -> 30
data.sec.gov /submissions/CIK0000320193.json -> None
data.sec.gov /api/xbrl/companyfacts/CIK0000320193.json -> None
controller.py:12 matches site_pattern against request_host, and urls.py:39/:45 put both endpoints on SEC_DATA_URL. So no rule under that key has applied to a data.sec.gov request. Your 5 tests pass on the branch, and 55 cache/httpclient tests pass with no regressions.
The blocking finding holds too. filecache/transport.py:116-118 raises unguarded on negative age, and :318 confirms fetched is the server's Date header, not the local clock. Agreed this blocks — trading a caching bug for a hard get_facts() failure on skewed clocks is a bad trade.
Correction: this is a dated regression, not a permanent condition
The
/submissionsTTL has never taken effect — including the deliberate 600s → 30s reduction in655f5d30
It did take effect, for about two weeks. Before bd5d2397 the key was a literal:
CACHE_RULES = {
r".*\.sec\.gov": { # <- matches data.sec.gov
"/submissions.*": MAX_SUBMISSIONS_AGE_SECONDS,Timeline:
- 2025-10-28
655f5d30cuts the TTL 600 → 30 for #471. Key is still.*\.sec\.gov, so data.sec.gov matches and the 30s does apply. - 2025-11-10
bd5d2397("Configure SEC urls via environment variable") replaces the literal with a domain derived fromSEC_BASE_URL, yielding.*www\.sec\.gov. data.sec.gov stops matching that day.
So the #471 fix worked as intended and was then silently undone ~9 months ago by an unrelated mirror-support change. That reframes two things:
- Your PR restores prior behaviour rather than enabling something new — a stronger and more defensible story for the CHANGELOG entry.
- It undercuts "reactivates age-based freshness checking for data.sec.gov for the first time" in the blocking section. That path was live before 2025-11-10, so the
# pragma: no covershield is ~9 months old, not permanent. Doesn't change the recommendation — the crash is still real and still gated on the upstream clamp — but the framing should match.
Corroborating from a second direction: upstream still ships httpxthrottlecache.EDGAR_CACHE_RULES keyed .*\.sec\.gov, and it resolves data.sec.gov + /submissions to 600 today. Upstream's own shape never had this bug; edgartools narrowed it locally.
Two smaller notes
www.sec.gov + /submissions goes 30 → None. Right in principle, since submissions is only served from the data host. Worth a deliberate nod for a mirror that splits the two hosts but serves submissions from the base one — the merge branch only covers mirrors where both collapse to one host.
Host patterns are unanchored at the end (pre-existing, not yours):
www.sec.gov.attacker.com -> matches '.*www\.sec\.gov'
data.sec.gov.evil.net -> matches '.*data\.sec\.gov'
Low severity — it only selects a cache TTL, and someone would have to point EDGAR_BASE_URL at such a host — but you're rewriting this exact function, so it's the cheap moment to add a $.
Routing
Agreed on landing the upstream clamp first. Please do open that PR against httpxthrottlecache — and once it ships I'd rather take the minimum-version pin here than carry a workaround, given >=0.6.0 is already pinned for the [httpx] extra.
Excellent write-up, incidentally. The "was excluding companyfacts deliberate?" section and the honest scoping of the batch benefit are the two things that made this quick to verify rather than quick to doubt.
🤖 Review assisted by Claude Code
75b4987 to
5f96c66
Compare
|
Force-pushed a refresh (
Also checked, since 44 commits is enough for a stale claim to go bad: On the two red CI tests from the first run: I said they looked like flaky DNS and I went looking for a way I could be wrong. Two things say I'm not. Neither Still a draft for the same reason as before, and only that reason: the clock-skew crash in the Blocking finding section. I've now opened it upstream with a fix and tests — paultiq/httpxthrottlecache#44, against paultiq/httpxthrottlecache#43, which he'd already accepted. Once it ships I'll pin a minimum version here and take this out of draft. |
|
paultiq/httpxthrottlecache#44 is merged, and httpxthrottlecache v0.6.1 is released. FYI: #490 is where data.sec.gov caching was broken. It's not mentioned in this discussion, and should be looked at closely to make sure this PR doesn't introduce a regression when someone uses SEC_DATA_URL. |
`_get_cache_rules()` builds its single top-level key from `SEC_BASE_URL` (`www.sec.gov`), but httpxthrottlecache matches the request HOST against that key before it ever looks at the path. `/submissions` and `/api/xbrl/companyfacts` are served from `SEC_DATA_URL` (`data.sec.gov`), so: re.match(r'.*www\.sec\.gov', 'data.sec.gov') -> None No rule under that key has ever applied to a data.sec.gov request. Verified live: a fresh process logs `No patterns matched data.sec.gov` and goes to the network every time. This means the `/submissions` TTL has never taken effect -- including the deliberate 600s -> 30s reduction in 655f5d3 for Issue dgunning#471. It also means `get_facts()` (companyfacts) re-downloads on every call in every process. Fix: build one key per host actually used by `edgar.urls` and file each rule under the host that really serves it. A custom mirror that points EDGAR_BASE_URL and EDGAR_DATA_URL at the same host merges into a single key, so mirror support is preserved. Also adds the missing `/api/xbrl/companyfacts/.*` rule under the data host, with the same 30s budget as `/submissions`: both are invalidated by the same event (a new filing lands), so a longer companyfacts-specific TTL would reintroduce exactly the staleness Issue dgunning#471 removed. Tests: `tests/issues/regression/test_data_sec_gov_cache_domain.py`, 5 cases. 3 of them fail against the pre-fix code (they pin the regression itself, not just the fixed behaviour); the other 2 are negative guards for www.sec.gov and an unrelated host, unaffected either way. CHANGELOG.md: entry under [Unreleased] / Fixed, per CONTRIBUTING.md.
… parser The previous commit fixed data.sec.gov by adding a second key, but kept the hand-built `.*<domain>` pattern. Two holes survived, both raised by @paultiq pointing at GH dgunning#490 as the origin: Unanchored keys leak across hosts. `.*mirror.example.org` also matches `data.mirror.example.org`, and `controller.get_rules` returns the FIRST key that matches, so a mirror configured as base=mirror.example.org / data=data.mirror.example.org resolved its submissions and companyfacts requests to the base rule set — which has no rule for them. That is this same bug, moved from sec.gov to mirrors. Hand-parsing disagrees with the matcher. The key is compared against `request.url.host`; `https?://([^/]+)` keeps the case, port, `user@` and percent-encoding that httpx normalises away, so a capitalised host, a port or credentials in EDGAR_DATA_URL produced a key no real request can match. `_host_key` now takes the host from `httpx.URL(...).host` — the same parser that produces it at match time — and matches it exactly, so the two sides cannot disagree and a rule written for one host cannot answer for another. A URL with no host is logged and matches nothing: a misconfigured mirror goes uncached rather than borrowing another host's rules. Also pins httpxthrottlecache>=0.6.1, whose clamp for the clock-skew crash (paultiq/httpxthrottlecache#43) is what caching data.sec.gov now reaches. Tests: 7 of the 12 fail on the previous commit — the three cross-host cases (including a lookalike host that inherited the cache-forever Archives rule) and the four parser-mismatch cases. Two of them only fail after fixing the test helper itself: it fell through to later keys when the real matcher stops at the first, which made every cross-host assertion unable to fail.
…le asserts The file checks _get_cache_rules through _rule_for, a replica of httpxthrottlecache's matcher. A replica can be wrong in the same direction as the code it checks, and this one already was once — so it is now pinned against controller.get_rule_for_request itself, over deliberately overlapping keys, which is the only shape where a fall-through replica and the library disagree. Agreement on the live (non-overlapping) rules is free and could not fail alone. Also: one case per rule instead of four asserts in one test, so a break names the rule it broke; /Archives cache-forever asserted by identity, apart from the TTL cases; and the percent-encoded mirror host from the PR table now has the test the other three URL forms already had. Verified: 23 pass, 8 fail against the previous commit's httpclient.py, all 23 pass in isolation and in reverse order.
5f96c66 to
b321b9a
Compare
|
@paultiq Thanks — that pointer found a live hole in this PR, not a hypothetical one. #490 is where it broke, and the mechanism that broke it was still in my fix. #490 is the commit. It replaced the literal What you were right to suspect. My fix added a second key but kept the hand-built
And a second one, which is why the fix is not just an anchor. The key is matched against So One consequence worth stating plainly, since it is a narrowing: exact matching drops Tests. 23 cases, offline and deterministic, and marked Also in this push: rebased onto Per CONTRIBUTING:
|
`scripts/check_regression_provenance.py` refuses a regression test whose module docstring does not link the issue, PR or bead it guards, and the fast job runs it before the tests — so the whole job failed in 65s without running a single test. The docstring already explained the bug at length; what the gate wants is the one canonical link, which here is the PR itself.
What
Cache rules are keyed by a regex that httpxthrottlecache matches against the request host, before it ever looks at the path (
controller.get_rules)._get_cache_rules()built one key, fromSEC_BASE_URL:/submissionsand/api/xbrl/companyfactsare served fromSEC_DATA_URL(edgar/urls.py:39and:45), so no rule under that key has ever applied to a data.sec.gov request, whatever its path. Verified live: a fresh process logsNo patterns matched data.sec.govand goes to the network every time.Two consequences:
/submissionsTTL has never taken effect — including the deliberate 600s → 30s reduction in655f5d30for 8-K reports available with a lag #471, made after a user reported same-day 8-Ks taking up to 10 minutes to appear.get_facts()re-downloads companyfacts on every call, in every process.This started at #490, which replaced the literal
r".*\.sec\.gov"— a pattern that did matchdata.sec.gov— with one derived from the base URL alone. Thanks to @paultiq for pointing there; it also answers the question below about whether excluding companyfacts was ever a decision.The fix
One key per host
edgar.urlsactually uses, each rule filed under the host that really serves it, and each key matching that host exactly:Exactness is not decoration. An unanchored key leaks across hosts —
.*mirror\.example\.orgalso matchesdata.mirror.example.org, andget_rulescommits to the FIRST key that matches — so a two-host mirror would resolve its data requests to the base rule set, which holds no rule for them. That is this same bug, moved from sec.gov to mirror users, i.e. to exactly the people #490 was written for.And the host is taken from
httpx.URL(...).host, the same parser that produces the value it will be compared against. A hand-rolledhttps?://([^/]+)keeps what httpx normalises away, so four valid ways to configure a mirror each produced a key no real request could match:EDGAR_DATA_URLrequest.url.hosthttps://DATA.mirror.example.orgdata.mirror.example.orghttps://data.mirror.example.org:8443data.mirror.example.orghttps://user:pw@data.mirror.example.orgdata.mirror.example.orghttps://mirr%C3%B6r.example.orgmirr%c3%b6r.example.orgA mirror pointing both env vars at one host still merges into a single key. A URL with no host at all is logged and matches nothing — a misconfigured mirror goes uncached, which is slow, rather than borrowing another host's rules, which would be wrong.
One deliberate narrowing: exact matching no longer covers
efts.sec.gov, the only other sec.gov host in the codebase (edgar/search/efts.py). Not a regression —EFTS_BASE_URLis hardcoded rather than configurable, its/LATEST/search-indexpath matches no rule in either rule set, andmain's.*www\.sec\.govkey does not match that host either. Uncached before, uncached after.The companyfacts TTL, and why not longer
It reuses
MAX_SUBMISSIONS_AGE_SECONDS(30s) rather than a new constant — and deliberately not the superseded 600s. companyfacts is invalidated by the same event as/submissions(a filing lands), so a longer budget would reintroduce exactly the staleness #471 removed, for a sibling endpoint.Honest scope of the benefit. A batch calling
get_facts()once per distinct CIK gains ~nothing from a 30s TTL — each CIK is requested once, far outside any 30s window. The real benefit is (a) consistency with the policy already set for/submissions, and (b) no duplicate round-trips for repeated or interactive calls to the same company in a short window, which is what #471 was about. I would rather state that than sell a speedup this does not deliver.Was excluding companyfacts deliberate?
I looked before proposing:
/submissions, the full-index files and/Archives/edgar/data. XBRL/companyfacts is not mentioned.edgar/entity/CLAUDE.mddescribes a "24 hour TTL for facts" that was never implemented (theuse_cacheparameter it references does not exist nearget_facts()) — a weak signal for caching, never wired up.Cache-Control,ETagorLast-Modified(checked live), so header-based revalidation is not an option either way.Conclusion: no documented deliberate exclusion — and #490 shows the mechanism by which it was lost.
Upstream dependency
Fixing the host matching reactivates age-based freshness checking for
data.sec.govfor the first time, andFileCache.get_if_freshraised uncaught there when the local clock trails the origin'sDateheader — taking down the wholeget_facts()call. Reproduced live with real skew (~117s), through the real request path.That was this PR's blocking finding. It is fixed upstream (paultiq/httpxthrottlecache#43, PR #44) and released in 0.6.1, which this PR now pins as the floor. Thank you @paultiq for turning it around quickly.
Tests
tests/issues/regression/test_data_sec_gov_cache_domain.py— 23 cases, offline and deterministic, so they run in the PR gate rather than only in the weekly regression job.8 of them fail on the previous commit: three cross-host cases (including a lookalike host that inherited the cache-forever
/Archivesrule) and five parser-mismatch cases. The rest are negative guards —www.sec.govrules unchanged, an unrelated host still uncached — which pass either way by design and exist to catch a future over-permissive key.One class is about the tests themselves: they check
_get_cache_rulesthrough a local replica of the library's matcher, and a replica can be wrong in the same direction as the code it checks. Mine was, briefly: it fell through to later keys where the library stops at the first, which made every cross-host assertion incapable of failing. It is now pinned againstcontroller.get_rule_for_requestitself, over deliberately overlapping keys — the only shape where a fall-through replica and the library disagree.Checked per CONTRIBUTING:
hatch run lintreports 1115 errors with and without this change (the count onmain),ruff checkandruff format --checkare clean on both changed files, andhatch run check-cassettespasses (214 files, none recorded or touched here).pytest -m fastis 4974 passed / 12 failed locally, and those 12 fail identically withedgar/httpclient.pyandpyproject.tomlreverted tomain— Windows-local, not this change.