refactor(service-rpc): the idempotency LRU tracks entries, not a joined string#141
Conversation
…ed string The store flattened (method, key) into a delimited string to use it as an LRU Map key, and picked NUL as the delimiter because neither part can contain one. That literal NUL is what made git treat serve.ts as binary (fixed in 808a8ce by escaping it) — but the delimiter was the real smell. Remove the flattening entirely: a completed entry carries its own method and key, and the LRU is a Set of entries in insertion order. Eviction reads the oldest entry's own location. No delimiter, no lruKey helper, no composite Map key. Same behavior — dedup, single-flight, replay, and LRU eviction tests all pass unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRefactors 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
A NUL used as an LRU-key separator made a file read as binary and hid its entire diff on review. Record the rule so no agent reaches for a control byte again: compose keys with a data structure, not a delimited string; if a delimiter is unavoidable, use a printable character. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Rewrite the rule to lead with the actual reason, not the one symptom I hit: a NUL is the string terminator — a reserved control signal across the whole stack — so "it can't collide" is exactly backwards. It is collision-free only because every correct system refuses to put one in data, and it silently truncates or corrupts in every downstream layer it reaches (C strings, DB drivers, serializers, logs, transports); the hidden git diff is one instance, not the reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Why
The idempotency store flattened
(method, key)into a delimited string to use as an LRUMapkey, and picked a NUL byte as the delimiter ("a method name or a UUID can't contain one, so it can't collide"). That literal NUL made git classifyserve.tsas binary — GitHub rendered "Binary file not shown" and hid the whole file in #140's diff.808a8ceescaped the byte (\0) as a stopgap, but the delimiter itself was the smell: flattening a tuple into a string is what invited a "clever" separator in the first place.What
Remove the flattening. A completed cache entry now carries its own
methodandkey, and the LRU is aSet<CompletedEntry>in insertion order (a Set preserves it, so delete-then-add moves an entry to the newest end). Eviction reads the oldest entry's own location. No delimiter, nolruKeyhelper, no compositeMapkey — andserve.tsis plain text again.Behavior
Unchanged: dedup, single-flight, replay-within-TTL, and LRU eviction at
REPLAY_CACHE_MAX_ENTRIES. All 55 service-rpc tests pass, including the eviction and cross-method-isolation cases. typecheck clean, lint clean.🤖 Generated with Claude Code