Pull request overview

This PR fixes a concurrency bug in the JSON-RPC batch handling path where multiple batch sub-call goroutines could concurrently invoke the shared gzip “streaming flush” hook, racing on the underlying gzip.Writer and potentially crashing the node with a nil-pointer panic. The approach is to mask the gzip flush hook from the per-item batch goroutines (since they write to private buffers, not the HTTP response), and to add a regression test reproducing the prior race.

Changes:

• Add withoutGzipStreamingHook(ctx) to explicitly mask any inherited gzip streaming flush hook for contexts used by batch sub-calls.
• Apply this masking to the batch call processing context so concurrent batch items can’t call into the shared gzip response writer flush hook.
• Add a -race regression test that sends a real gzipped JSON-RPC batch of streamable calls through the gzip middleware and RPC server.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

		// Bounded parallelism pattern explanation https://blog.golang.org/pipelines#TOC_9.
		boundedConcurrency := make(chan struct{}, h.maxBatchConcurrency)
		defer close(boundedConcurrency)
		wg := sync.WaitGroup{}
		wg.Add(len(msgs))

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The fix itself is correct and well-scoped. Two changes before merge:

1. Regression test asserts nothing (node/rpcstack_gzip_batch_race_test.go). It calls ServeHTTP and checks no result, so it passes even on an error / empty / uncompressed response — its only failure signal is -race (or a lucky panic). Make each call's payload exceed minGzipBodySize so the fixed path still gzips, then assert HTTP 200, Content-Encoding: gzip, and that the decoded batch contains all n responses. (This is also what Copilot's line-65 comment asks for; its line-29 "needs encoding/json" comment is only true once you add the unmarshalling.)

2. Pre-existing deadlock in handleBatch (rpc/handler.go:207) — not introduced here, but it's in the function you're editing. wg.Add(len(msgs)) while only len(calls) goroutines are spawned. handleResponses drops response-shaped messages and _subscription notifications from calls, so a batch mixing a call with either — e.g. [{"id":1,"method":"eth_chainId"},{"id":2,"result":"0x1"}] — leaves the counter non-zero, wg.Wait() hangs forever, and h.close's callWG.Wait() hangs with it, leaking two goroutines plus the connection per request (unauthenticated DoS; WaitGroup.Wait ignores ctx cancellation, so it survives client disconnect). Fix is wg.Add(len(calls)). Fine to split into a separate PR if you'd rather keep this one focused — but please land it with a repro test (TDD). This is the same issue Copilot flagged as "low confidence"; it's real.

Nit (optional): guard the hook call in runMethod with ok && flush != nil, so the mechanism doesn't depend on the value always being stored as an untyped nil.

Approving. Verified the root cause is the concurrent gzip.Writer race (not the empty-flush theory in the issue — in Go 1.25 an empty single-goroutine Flush is safe), and ran the tests under -race: the node test reliably reproduces the race once the masking line is removed, so it's a genuine regression guard.

Two optional nits, neither blocking:

• The three guard layers (mask the hook + WithGzipStreamingHook nil-panic + runMethod's && flush != nil) are somewhat redundant — the untyped-nil masking already makes a typed-nil hook unreachable. Fine to keep given the severity, but the runMethod guard adds little over the panic.
• withoutGzipStreamingHook's doc comment runs a touch long for our comment policy; the last sentence (untyped vs typed nil) could go.