fix: double deinit of response buffer on non-200 HTTP responses - #112
Open
lukemacauley wants to merge 1 commit into
Open
fix: double deinit of response buffer on non-200 HTTP responses#112lukemacauley wants to merge 1 commit into
lukemacauley wants to merge 1 commit into
Conversation
HttpTransport.request and requestBatch called response_body.deinit() explicitly on the error paths and then returned an error, which also ran the errdefer deinit on the same Allocating writer -- a double free. Any JSON-RPC endpoint answering with a non-200 status (e.g. Alchemy's HTTP 429 capacity-limit responses) crashed the process with SIGSEGV in ReleaseFast instead of surfacing error.HttpError. Took down the PerpCity production liquidator in a once-a-minute crash loop for 19 hours on 2026-07-30/31. Drop the explicit deinits and let the errdefer own cleanup. Adds a regression test against a real local socket returning 429; the old code aborts on it with a double-free report from the testing allocator. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe HTTP transport removes duplicate response-buffer cleanup from HTTP error paths. A local-server integration test sends two HTTP 429 requests and verifies that both return ChangesHTTP error response cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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
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.
Summary
HttpTransport.requestandrequestBatchcallresponse_body.deinit()explicitly on the non-200 and connection-failure paths, then return an error — which also runs theerrdefer response_body.deinit()on the samestd.Io.Writer.Allocating. That's a double free. In ReleaseFast (how gator-liquidators ships) it's UB and manifests as a silent SIGSEGV instead oferror.HttpError.Impact
This took down the PerpCity production liquidator for ~19 hours on 2026-07-30/31: the team's Alchemy quota ran out, Alchemy started answering every call with HTTP status 429, and gator segfaulted (exit 139, no output) within seconds of each boot — a once-a-minute ECS crash loop for the entire outage window, during exactly the kind of event where a liquidator matters most. Any sustained non-200 from an RPC endpoint (rate limits, gateway 502s, provider incidents) triggers it.
Repro: point gator-liquidators at any endpoint returning a non-200 (Debug build reports the double free at
http_transport.zig; the crash address is the 0xAA undefined-fill pattern).Fix
Drop the explicit
deinit()calls and let theerrdeferown cleanup on all error paths. Two-line-pair deletion, no behavior change on success paths.Test
Adds a regression test that runs a real localhost TCP server answering
429 Too Many Requestsand assertsrequest()returnserror.HttpErrortwice in a row. On the old code the testing allocator aborts with a double-free report; on the fixed code all tests pass. Verified the fixed build end-to-end against the live capacity-limited Alchemy endpoint: cleanerror.HttpErrorcycles with backoff, no crash.After merge/release, gator-liquidators needs a bump from
eth.zig v0.8.1to pick this up.🤖 Generated with Claude Code
Summary by CodeRabbit