Per-host download politeness, honest exits, and a bounded chroma FD budget - #42
Merged
Merged
Conversation
lfnothias
force-pushed
the
fix/download-politeness-and-chroma-fd-bound
branch
from
August 24, 2026 17:04
ff58db5 to
92fc09f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three related fixes to the ingest path, each one measured rather than reasoned about.
The chroma FD and RSS leak
Root cause is not an application-level collection cache — there is none; every
Collectioninsrc/perspicaciteis a local variable discarded at method exit. It is chromadb's rust HNSW segment cache, sized once at client construction asmax_file_handles // 5fromRLIMIT_NOFILEand never re-read. With the hostulimit -nat 1,048,576 that budget is 209,715 resident indices — effectively unbounded. Observed on a live server: 8,207 FDs, 7,924 of them underchroma_db/(1,981 collections x 4 files), ~11.5 GB RSS after 3h50m.ChromaVectorStorenow constructs through a bounded client, dropping the budget to 409 resident indices.A second, separate bug:
SharedSystemClientkeys its System cache on the persist-directory string, unnormalised.bibtex_kb.py:301passed a resolved absolute path and:452a possibly-relative one, so one directory got two Systems and two HNSW caches. Normalising the path in__init__collapses all four construction sites without touchingbibtex_kb.py.Lowering the soft FD limit naively is unsafe and this was found the hard way: if the process already holds more descriptors than the budget, chroma's rust binding does not merely bound its cache — it panics with EMFILE and the client cannot be constructed at all (reproduced at soft=100 and soft=300; soft=500 works). Since
web/state.pyandmcp/server.pyconstruct at boot, that would mean the server refuses to start. The helper therefore never lowers belowmax(1024, open_fds + 256)and never raises an operator's own ulimit.The plateau test nearly had no power: on this platform HNSW index files are memory-mapped, not held as descriptors, so
/dev/fdshows zero and a "count FDs under the directory" assertion would have passed vacuously forever.lsofshows them, at exactly 4 entries per collection — matching the production signature. The test compares a budgeted client against an unbounded control in the same process: 222 vs 483 entries for 120 collections. Sabotaging the bound makes it fail (483 vs 483, perfectly linear).bioRxiv 429s
There was no backoff, no per-host concurrency cap, and throttled responses were filed as permanent successes — a 17-byte body recorded as a completed fetch. A per-host politeness layer now spaces requests, retries only
{429, 500, 502, 503, 504}, honoursRetry-After, and raisesRateLimitedrather than handing a throttled body back as content.The host policy is a data table with exactly one row (
biorxiv.org, carrying the measured behaviour in a comment); the retry, spacing and cooldown machinery contains no host names at all. A URL with no parseable host gets an explicitunknownlimiter rather than silently bypassing rate limiting.Honest exits
content_typeis a storage format, not a transient value:rag/dynamic_kb.pypersists it into ChromaDB chunk metadata and selects the embedding model from it. The literal values are now named constants withis_abstract_only()/has_full_text()predicates, so callers stay correct without an equality test — and the values themselves are unchanged, deliberately.Downstream consumers (
search_to_kb, the MCP server, the KB router) now distinguish a retryable failure from a permanent one instead of collapsing both into "no full text".Note on the PDF size floor
MIN_PLAUSIBLE_PDF_BYTESunifies checks that were previously duplicated literals. It is set to 1024, the floorpdf_cachehas always enforced — not a tightened value. An earlier revision used 2048 and rejected legitimate small PDFs: the dropzone returned "failed to write PDF to cache" for a valid upload and a cached local PDF was reported as remote. Unification should not smuggle in a behaviour change.Verification
tests/unit: 2789 passed, 8 skipped. One test is deselected —test_web_search_telemetry_sink.py::test_list_telemetry_sink_receives_eventsmakes real SemanticScholar network calls and hangs. It was confirmed pre-existing by running it against unmodified HEAD in a separate worktree, where it hangs identically; note that it does transitively importretrieval.chroma_store, so source-level inspection alone was not sufficient to clear it.Ruff findings on the touched files drop from 56 to 51; no new ones.