queue-proxy can crash with concurrent map iteration and map write during HTTP/2 response finalization #16656

Activity

1. shashankvarma499 commented on Sep 13, 2026

   

   shashankvarma499

   on Sep 13, 2026

   Last edited by shashankvarma499

   More actions

   Thanks for the detailed report and the pinned stack trace. I traced the suspected path and I agree the race is real, but the right fix has a couple of valid shapes, so I wanted to lay out the cause and a proposed direction before sending a PR.

   Root cause

   timeoutWriter.Header() in pkg/http/handler/timeout.go returns the underlying writer's map without taking tw.mu:

   func (tw *timeoutWriter) Header() http.Header { return tw.w.Header() }

   Every mutating method (Write, WriteHeader, Flush, and the try* helpers) holds tw.mu, but Header() hands the raw http.Header map back to the caller. A caller on the handler goroutine can then do tw.Header().Set(...) while the HTTP/2 server's writer goroutine iterates the same map in promoteUndeclaredTrailers during writeChunk/handlerDone. That is the "concurrent map iteration and map write" in your stack.

   There is a second aggravating factor in timeoutHandler.ServeHTTP: on the timeout branch it returns as soon as tryTimeoutAndWriteError succeeds, without waiting for the handler goroutine to finish. The inner handler keeps running (and can keep mutating headers / writing) after ServeHTTP has already returned and the HTTP/2 server has begun finalizing the response. The timedOut flag already makes post-timeout Write/WriteHeader/Flush no-ops, but Header() bypasses that guard entirely, which is why header mutation is the path that still races.

   Proposed direction

   The challenge is that simply locking tw.mu around the Header() return does not help, because the caller mutates the returned map after the method returns, and the HTTP/2 server iterates the underlying map directly rather than through the wrapper. Two directions seem viable:

   1. Make post-timeout header mutation a no-op, matching the existing timedOut handling for Write/WriteHeader/Flush. That means Header() returns a header view that refuses (or ignores) mutations once tw.timedOut is set. This is the smallest change and preserves the current abandon-the-handler-on-timeout design.
   2. Ensure the handler goroutine is drained before ServeHTTP returns on the timeout path (wait for done). This removes the concurrent access entirely but changes timeout semantics and could block the timeout path, so it is a larger behavioural change.

   I lean toward (1) as it reuses the existing timedOut mechanism rather than introducing a new synchronization scheme, but (2) is arguably the more complete fix.

   Tests

   • A go test -race unit test in pkg/http/handler that exercises a handler which calls Header().Set after a timeout while the response is being finalized, asserting the write is dropped and no race is reported.
   • Keep the existing timeout tests green, and add a case confirming the pre-timeout Header().Set path is unchanged.

   Defer

   If this is the same underlying issue as #15850, or if there is already an in-flight fix, I will leave it alone. Otherwise I can put up a PR for direction (1) once the approach is confirmed.

   👍React with 👍1Reacted by junyicc