I Tightened Parameters One at a Time, Got Breached Twice, and Stopped Tightening
I went in to add a shape check to a sibling route and found that was not the point. The cache key was the whole RequestURI, so every way of spelling the same response became its own entry. I gave up hit rate and set a 64MB total ceiling, and then review found three more holes in the ceiling itself
The disclosure-summary backend holds public GET responses in an in-memory cache for a few minutes. What I went in to fix that day was one missing date-shape check on a sibling route, nothing more. Measuring before I added it showed the cache filling with separate entries for requests that returned byte-identical responses.
What I set out to do
The disclosure-summary backend keeps public GET responses in an in-memory cache for a few minutes, so a burst of identical requests does not reach the database.
What I went in to fix that day was small. The daily-aggregate route validates the shape of its date, but the sibling route ?hasSummary=true&rceptDt= did not. Adding the same check there looked like the whole job.
How I asked
Before adding it, I asked for measurements. I wanted to see, in numbers, how many entries each tightened parameter actually removed. So I had it run a local server, feed in variant requests, and print the cache entry count alongside the response size.
That table scrapped the plan.
?hasSummary=true&limit=20 / 21 / 99999 / 020 → all identical 12,562B, 4 entries
?hasSummary=true&limit=20&zzz=1 / zzz=2 → identical 12,562B, 2 entries
?hasSummary=true&limit=20&rceptDt=lolwat → 3B, its own entryThe handler clamps limit to 20 anyway. Four spellings still became four entries. Adding a zzz that does not exist anywhere added another.
Where I got stuck
Opening the cache code, two lines were looking at different things.
// rules match on the decoded path
r.URL.Path
// the key is the whole raw string
key := r.URL.RequestURI()Matching by meaning, storing by spelling. Do that and you get one entry per way of spelling the same response. The number of spellings is effectively unbounded.
So the rceptDt check I came to add was not the point. The expensive side was limit and unknown parameters, which each produce a 12.5KB list, while rceptDt=lolwat was a three-byte empty array. I had gone to plug the cheapest hole.
The worse part was the lifespan of the approach. A defense that tightens parameters one at a time gets breached again every time a new parameter appears. And it already had, twice, in the previous pull request alone. Once because the same value could be written several ways. Once because validation only looked at part of the value and let it through.
Writing a third defense the same way meant getting breached a third time.
How I worked it out
I stopped tightening the input and put a ceiling on the storage instead. A 64MB total for the store. Over it, sweep the expired entries first, and if it is still over, drop arbitrary entries down to three quarters. Accounting runs on an atomic counter, with the add and subtract paired on expiry deletion and overwrite.
The reason it is not an LRU went into the commit.
Everything in here is a public GET with a 1 to 10 minute TTL, so anything pushed out gets refilled on the next request. What I am protecting here is not the hit rate, it is the memory ceiling.
I wrote down the risk I took on in exchange. Noise can now push out legitimate entries, and while it does those requests go to the database. Still, slower beats dead.
Then I corrected that risk assessment the same day. A single IP cannot reach this ceiling. The read rate limiter blocks more than half, which puts the steady state around 45MB. Feed in 100 noise requests and 56 come back 429. The ceiling actually bites with multiple IPs or when large responses are in the mix. The reason to keep it is that without it, that 45MB scales with the number of IPs.
That was my part. The real work came after. Review found three P1 issues in the ceiling itself.
First, the entry being stored sat outside the ceiling. The size is added before eviction runs, and eviction cannot remove an entry that is not in the store yet. If one response alone exceeds the ceiling, the store gets emptied and that response goes in anyway. I added a separate 1MB per-entry cap. The reasoning: the normal maximum is a day of sitemap projection at 210KB measured in production, so five times headroom, and at one sixty-fourth of the total the overshoot stays small even when several land at once.
Digging into that premise turned up the real problem. The date-range query branch had no LIMIT. It is a public anonymous route, and calling it with an empty range returns the whole table as one response. Locally that was 62,024 rows at 17.7MB, with server RSS reaching 141MB. At production’s 341,593 rows it works out to about 97MB, and it would sit in the cache for a minute. Every sibling branch had a limit. Only this one did not. Capping it at 100 by default and 1,000 maximum took 17.7MB to 30KB and RSS from 141MB to 24MB.
Second, what got deleted and what got subtracted were different. When two requests read the same expired key and one writes a new value, the later one deletes that new value while subtracting the size of the old value it read. Accumulate that and the ceiling quietly stops meaning anything. I switched to an operation that only deletes when the value is the one I read. This one reproduced in a test: 64 goroutines, 300 rounds each, TTL widened to a microsecond, and the drift showed up three times out of three.
Third, one was bounded but the sum was not. When misses on different keys enter the store section at the same time, each finishes its own eviction and each inserts, so the sum can exceed the ceiling. I inverted the order and serialized just that section. Measure after inserting, and eviction can drop what was just added, which is fine for a cache.
What the agent did on this third one is what I liked. It fixed it and then wrote this in the commit.
Honestly: I could not reproduce this defect with a test. The test passes on the pre-fix code too.
The ordering is logically possible but could not be constructed through the public API. So the reason for the fix was not that a test caught it but that the invariant should hold structurally, and it noted in the test comment that this is a regression guard, not a reproduction test.
The ceiling’s own verification stayed honest too. The unit test pushes 900 entries at about 90MB and confirms it gets cut to 572 entries at 56.1MB. But end to end against a local server the ceiling could not be reproduced. The rate limiter blocked more than half, so 6,000 requests only loaded about 33MB. The commit states plainly that the evidence for the ceiling is the middleware unit test.
What I learned
- What you came to fix and what needs fixing can be different things. I came to add a shape check and found the key design was the problem. Measuring before adding is what did it.
- If you are writing the same defense a third time, the design is wrong, not the defense. Parameter tightening had been breached twice and was due for a third.
- Matching and storing must not look at different things. Choose rules by meaning and build keys from spelling and you get one entry per spelling.
- Decide first whether you are protecting hit rate or a ceiling. Once you have, skipping LRU is a choice rather than laziness.
- Adding a ceiling makes the ceiling a new surface. The pending entry, the accounting pairs, the sum of concurrent inserts. Review found all three.
- Write down which defects you could not reproduce. Otherwise the next person mistakes that test for a reproduction test.