Repository navigation
perf(registry): key the related-skills cache on the request slug - #277
Merged
Merged
Conversation
The slug lookup ran ahead of the cache, so every request cost one or two D1 reads even on a hit. A crawler walking 14,100 Skill pages paid that read 14,100 times an hour however warm the cache was. On 2026-09-22 that floor saturated D1. Sentry recorded 3,730 `D1 DB is overloaded` events against this route, and artifact delivery, which shares the database, failed 100 resolutions with SERVICE_UNAVAILABLE after burning a full 15-minute retry budget. Caching the response was tried twice before, on 2026-08-04 and in SKILLD-1F, and could not help while the lookup sat outside it. Move findSkill inside the cached function and key on the slug, so a hit costs no D1 read. Cache the unresolved slug as a tagged `missing` variant on a short window, because a crawler sweep over dead slugs used to cost two reads each and cache nothing. Bump the key to v4: a v3 value is an untagged raw response and must never be read as a tagged entry.
harlan-github-agent
Bot
added
harlan-agent-running
Contributor
🤖 MERGED
GitHub merged this pull request. No material findings were recorded. 6d00f5ad-ac7e-4ed0-81a3-4662068cbe39 |
harlan-github-agent
Bot
added
harlan-agent-ready
harlan-github-agent
Bot
removed
the
harlan-agent-ready
This was referenced Sep 23, 2026
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
1 participant
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.
🔗 Linked issue
Resolves #192
❓ Type of change
📚 Description
Three fixes have gone at this already: the #158 retry, the #168 stale serve, and the response cache itself. Cold-key crawler sweeps still take D1 down, because none of them touched the part that runs on every request regardless.
findSkillsat in front of the cache. It is one D1 read, two when the slug needs the owner/repo/name fallback, and a cache hit paid it just the same. With 14,100 Skills in the registry, a crawler walking the catalog pays that read 14,100 times an hour no matter how warm the cache is. An unresolved slug was worse: two reads, a 404, and nothing cached, so the same dead URL cost the same thing forever.Moving the lookup inside the cached function and keying on the slug drops a hit to zero D1 reads. Unresolved slugs get cached as a tagged
missingon a 10 minute window, with no stale window so a newly indexed Skill is never served as absent.What made me chase this rather than file it behind the other three: artifact delivery shares the database. During the 07:00 UTC burst on 22 Sep, Sentry logged 3,730
D1 DB is overloadedevents withGET /api/skill-related/...as the top culprit by a factor of 40, and 100 resolutions failedSERVICE_UNAVAILABLEafter burning the full 15 minute queue retry budget against an overloaded D1. One of them: created 07:10:40, failed 07:26:34. Lifetime that is 90 ready against 114 failed, so the catalog crawl is taking down the paid path.The miss still costs five or six reads plus a Vectorize query. Precomputing the neighbour payload into a table would cut that to one indexed read, and that is the obvious follow-up, but it was pointless to do first while the per-request floor was still there.
Two things I am not sure about, and would rather a reviewer decided:
server/utils/db.tsas the target. I did not touch it. The retry there looked correct to me; the load reaching it was the problem. Happy to be told the retry still needs work.