From bbd43bfe9e6c9b71ffe9d5b3b2e54af5e33e6ab0 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 13 Aug 2026 03:10:18 +0100 Subject: [PATCH] fix(coalesce): release waiters when primary fn panics In RequestCoalescer.Do, the primary path calls fn() before storing the response, deleting the inflight key and calling wg.Done(). If fn() panics, all three steps are skipped: the sync.Map entry leaks and every coalesced waiter remains blocked on wg.Wait() forever (permanent deadlock for that key). Route release through a defer so waiters are always released and the entry cleared, even on panic. Test: TestRequestCoalescer_PanicSafety (a panicking primary must not poison the key for subsequent calls). --- request_coalescing.go | 13 +++++++++---- request_coalescing_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/request_coalescing.go b/request_coalescing.go index 411a503..e9f1769 100644 --- a/request_coalescing.go +++ b/request_coalescing.go @@ -138,6 +138,15 @@ func (rc *RequestCoalescer) Do(key string, fn func() (*CoalescedResponse, error) rc.inflightCount.Add(1) defer rc.inflightCount.Add(-1) + // Always release waiters and clear the map entry, even if fn() panics. + // Without this, a panicking fn() leaves the key in the inflight map and + // the WaitGroup counter positive, so every coalesced waiter for that key + // blocks on wg.Wait() forever (permanent deadlock + leak). + defer func() { + rc.inflight.Delete(key) + inflight.wg.Done() + }() + // Execute the request response, err := fn() @@ -152,10 +161,6 @@ func (rc *RequestCoalescer) Do(key string, fn func() (*CoalescedResponse, error) } inflight.mu.Unlock() - // Clean up and notify waiters - rc.inflight.Delete(key) - inflight.wg.Done() - // Log statistics waiters := inflight.waiters.Load() duration := time.Since(inflight.createdAt) diff --git a/request_coalescing_test.go b/request_coalescing_test.go index df9728d..4f7f9ad 100644 --- a/request_coalescing_test.go +++ b/request_coalescing_test.go @@ -405,3 +405,40 @@ func TestMin(t *testing.T) { assert.Equal(t, tt.expected, result) } } + +// TestRequestCoalescer_PanicSafety verifies that when the primary fn() panics, +// the coalescer still releases waiters and clears the inflight key instead of +// permanently deadlocking that key (waiters hang on wg.Wait and the entry +// leaks). +func TestRequestCoalescer_PanicSafety(t *testing.T) { + rc := NewRequestCoalescer(true, nil, nil) + + // First call's fn() panics. Recover in a goroutine to emulate the + // caller catching/handling the panic. + panicDone := make(chan struct{}) + go func() { + defer close(panicDone) + defer func() { _ = recover() }() + _, _ = rc.Do("key", func() (*CoalescedResponse, error) { + panic("boom") + }) + }() + <-panicDone + + // A subsequent call for the same key must complete (not hang), proving the + // key was released and the entry dropped. + done := make(chan struct{}) + go func() { + defer close(done) + resp, err := rc.Do("key", func() (*CoalescedResponse, error) { + return &CoalescedResponse{StatusCode: 200}, nil + }) + assert.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode) + }() + select { + case <-done: + case <-time.After(3 * time.Second): + t.Fatal("coalescer key still blocked after primary fn() panic") + } +}