fix(passthrough): bound the response cache by total bytes - #8
Merged
Conversation
The cache expired entries on age but never limited how much it held, so a client working through many pages inside one TTL window could pin every body it fetched at the same time. Non-HTML documents are kept as decoded bytes, so a few large ones cost far more than pages do. - Holds at most PASSTHROUGH_CACHE_MAX_BYTES (256 MB by default), evicting the entries closest to expiry first so eviction takes what was going to go anyway. - A body over a quarter of the ceiling is served but never cached, since admitting it would evict most of the cache for one document. - The completion log now reports whether the body was actually cached rather than whether it was eligible, which the byte cap can now change. - Adds the first browser-free tests for the passthrough, covering eviction order, the per-body ceiling, and the running total staying correct across replaces and expiries. Storing moved out of the request handler into _cache_store so the byte accounting has one owner and can be tested without a browser. Closes #7
The startup line divided the cap into whole megabytes, so any ceiling under 1 MB logged as "max 0 MB", which reads as caching being off rather than tight. Caught by a live run that set a deliberately small cap. Small caps now report in bytes.
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.
What changed and why
The passthrough response cache expired entries on age but never limited how much it held. A client working through many pages inside one TTL window could pin every body it fetched at the same time, and non-HTML documents are kept as decoded bytes, so a few large ones cost far more than pages do.
It now holds at most
PASSTHROUGH_CACHE_MAX_BYTES(256 MB by default), evicting the entries closest to expiry first. A body over a quarter of the ceiling is served but never cached, since admitting it would evict most of the cache for one document.Storing moved out of the request handler into
_cache_storeso the byte accounting has one owner and can be tested without a browser. That is what makes gate A able to fail here at all: there were no passthrough tests before this.Provenance
Filed by
/audit-scanon theresourcesdimension. The finding survived a refutation attempt: the comment atpassthrough.py:216-218shows growth was already reasoned about there, but what it fixes is different. Without that prune an entry was skipped once expired and never removed, pinning bodies for the process lifetime. That is fixed. What was left unbounded is how much can accumulate inside one TTL window.Scope covered
Every site the issue listed, re-checked with
grep -rn '_cache\b' src/ --include=*.py:src/passthrough.py:48—_cachedeclared with no bound; now paired with a_cache_bytesrunning totalsrc/passthrough.py:213-221— the unconditional insert; now_cache_storewith evictionsrc/passthrough.py:112— the PDF path that produces decoded bytes; covered by measuring bytes rather than entriessrc/config.py:211—passthrough_cache_ttl();passthrough_cache_max_bytes()added beside itOne site the issue did not list, found while implementing: the completion log said
, cachedbased on whether the body was eligible, which the byte cap can now falsify. It reports what actually happened.Gate A, browser-free suite
151 tests, OK. 138 before, plus 13 new covering eviction order, the per-body ceiling at its exact boundary (250 stored, 251 refused against a 1000 cap), the running total across replaces and expiries, and the zero-cap escape hatch.
Gate B, live solve tally against a same-window baseline
Two containers built and run interleaved, trial for trial: the change on 8291 and
origin/mainon 8391.Verdict: within noise, pass. The 0.5s gap sits inside the baseline's own spread (5.0 to 6.1 on stealth). Nothing in this change touches the solve path, so gate B is a no-regression check rather than a proof of the feature.
Gate C, the passthrough end to end
Run against a container with the passthrough enabled and a deliberately small cap, so eviction is reachable in a short run. Five distinct cache keys, each a real solve returning 8108 bytes, against a 33000-byte cap:
Five entries at 8108 exceed the cap at the fifth, the oldest was evicted, and the newest still served from cache. An earlier run at a 40000-byte cap also confirmed the per-body ceiling on real pages: bodies of 46 to 91 KB were served and refused by the cache, with only the 8 KB page admitted.
What was not covered
/loop-work's gate C as written describes abyparr-proxyin front of/v1, which is a different code path and would not have exercised this change at all. The skill needs that distinction; tracked separately._lock, and the tests are single-threaded.Review notes
Self-reviewed rather than run through
/pr-review, because this session carries a standing instruction not to spawn subagents unasked. One issue found and fixed inffa40fc: the new startup line divided the cap into whole megabytes, so any ceiling under 1 MB logged asmax 0 MB, which reads as caching being off rather than tight.One thing deliberately left: eviction sorts the key set on each store when over cap. At the default cap that is a few thousand keys, so roughly ten milliseconds against a solve that takes five to thirteen seconds. Not worth a heap or an ordered structure for 0.1% of the request.
Closes #7