fix(web): close unary Zoekt gRPC clients - #1591
Conversation
This comment has been minimized.
This comment has been minimized.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesZoekt search cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Summary
Finding
zoektSearch created a new grpc-js WebserverService client for every unary request but never closed it. grpc-js registers each InternalChannel in its process-global channelz registry and only unregisters it when Client.close() runs, so dropping JavaScript references does not make those channels collectible.
With the repository pinned grpc-js 1.14.4, a forced-GC reproduction created and dropped 20,000 clients. Channelz still reported all 20,000 channels afterward; retained V8 heap grew by 36,170,288 bytes (about 1.81 KiB per call) and RSS grew by 318 MiB. This path is used by unary search, code navigation, and search-backed tools. Live counters show it is not the dominant source of the current browse-crawler incident, but it is a concrete traffic-proportional leak.
Remediation
The unary call and response transformation now run inside try/finally, and the client is closed in finally. This guarantees channelz cleanup after a successful response, an RPC error, a synchronous call failure, or a later database/response transformation error.
Test plan
Note
Low Risk
Narrow lifecycle fix around existing unary search behavior; closing the client after each request is the intended grpc-js usage and should not change search semantics.
Overview
Fixes a traffic-proportional memory leak on unary Zoekt searches: each call created a new
@grpc/grpc-jsclient but never calledclient.close(), so channels stayed in grpc-js’s process-wide channelz registry until explicitly torn down.zoektSearchnow runs the RPC and response transformation insidetry/finallyand alwaysclient.close()infinally, so cleanup happens on success, RPC errors, and failures while enriching results (e.g. Prisma lookups). Streaming search (zoektStreamSearch) is unchanged; it already closed clients on stream end/error/cancel.Adds
zoektSearcher.test.tswith mocked gRPC to assertcloseruns on those three paths, plus an Unreleased CHANGELOG entry.Reviewed by Cursor Bugbot for commit 6b1626e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Documentation