Repository navigation
Conversation
AzureBlobObjectStore and VercelBlobObjectStore built a fresh httpx.Client per call and never closed it, so repeated run_remote_sync leaked FDs until cyclic GC. Add ObjectStore.close(), implement it for Azure/Vercel (honoring _owns_client), no-op for S3-family/GCS, and call it from run_remote_sync in a finally block. Fixes Tracer-Cloud#6989
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
 [Medium impact] The PR appears safe to merge; the remaining test-coverage suggestion is non-blocking. Findings
SummaryThe PR closes owned Azure and Vercel HTTP clients after remote sync.
Reviews (2) · Last reviewed commit: "fix(filestorage): close ObjectStore fake..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| report = run_remote_sync() | ||
| assert report is not None | ||
| assert stores and stores[0].closed is True |
There was a problem hiding this comment.
test_run_remote_sync_closes_store only checks a successful sync. It would still pass if cleanup moved out of finally, leaving clients open whenever a sync fails. Add a case where run_sync raises, then assert that the store closes and the original error reaches the caller.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
CI cli-runtime-3 failed because _FakeObjectStore lacked close(), so the finally in run_remote_sync raised AttributeError and masked the expected RuntimeError. Add the no-op on the fake and tolerate a missing close on legacy stores so a sync failure stays visible.
Summary
Fixes #6989.
AzureBlobObjectStoreandVercelBlobObjectStoreeach built a freshhttpx.Clientperrun_remote_sync()call and never closed it. Repeated syncs in a long-lived process therefore leaked sockets/FDs until cyclic GC happened to run.VercelBlobObjectStorealready tracked_owns_clientbut never read it.Changes
ObjectStore.close()to the protocol._owns_clientand close only when the store owns the client.close()using the existing_owns_clientflag.close()(no ownedhttpx.Client).run_remote_sync()callsstore.close()in afinallyblock.Test plan
pytest tests/filestorage/— 293 passedrun_remote_synccloses the built storeAI assistance
AI tools helped draft this change. I reviewed every line, understand the resource ownership, and verified the tests above.