Skip to content

fix: close pubsub connection on Subscribe error in CancelationPubSub#1104

Open
Bahtya wants to merge 2 commits intohibiken:masterfrom
Bahtya:fix/pubsub-connection-leak
Open

fix: close pubsub connection on Subscribe error in CancelationPubSub#1104
Bahtya wants to merge 2 commits intohibiken:masterfrom
Bahtya:fix/pubsub-connection-leak

Conversation

@Bahtya
Copy link
Copy Markdown

@Bahtya Bahtya commented Apr 9, 2026

Problem

Fixes #1095

When CancelationPubSub() in internal/rdb/rdb.go calls redis.Subscribe followed by Receive(), and Receive() fails, the pubsub object is not closed before returning the error. This causes a Redis connection leak.

The subscriber goroutine (subscriber.go) retries in an infinite loop on error, so each failed iteration leaks a new Redis connection. Over time, this exhausts the connection pool.

Fix

Add pubsub.Close() before the error return in CancelationPubSub():

pubsub := r.client.Subscribe(ctx, base.CancelChannel)
_, err := pubsub.Receive(ctx)
if err != nil {
    pubsub.Close()  // <-- added: prevent connection leak
    return nil, errors.E(op, errors.Unknown, fmt.Sprintf("redis pubsub receive error: %v", err))
}

This is a minimal, targeted fix that ensures the pubsub connection is always cleaned up when Receive() fails.

Testing

  • go build ./... passes
  • go vet ./internal/rdb/ passes
  • Integration tests requiring a Redis server cannot be run in this environment, but the change is straightforward and equivalent to the cleanup already done on the success path

This PR was created by an AI contributor. The fix has been verified with build and vet checks.

When redis.Subscribe succeeds but Receive() fails, the pubsub
connection was not being closed before returning the error. This caused
the subscriber goroutine to leak a Redis connection on each retry
iteration, eventually exhausting the connection pool.

Fixes hibiken#1095
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 9, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 63.32%. Comparing base (2fd155e) to head (dd3c923).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
internal/rdb/rdb.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1104      +/-   ##
==========================================
- Coverage   63.34%   63.32%   -0.02%     
==========================================
  Files          29       29              
  Lines        4984     4985       +1     
==========================================
  Hits         3157     3157              
- Misses       1549     1550       +1     
  Partials      278      278              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Add TestCancelationPubSubReceiveError to verify that when
Receive() fails in CancelationPubSub(), an error is returned
and the pubsub connection is not leaked.

This provides test coverage for the pubsub.Close() fix that
was missing in the previous commit.

Bahtya
@Bahtya
Copy link
Copy Markdown
Author

Bahtya commented Apr 9, 2026

Friendly ping on this PR. The fix correctly closes the pubsub connection in the Subscribe error path to prevent connection leaks. The codecov report notes 1 line is missing test coverage — I can add a test case for the error path in CancelationPubSub if that would help move this forward. Please let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Memory leak: subscriber goroutine retries SUBSCRIBE infinitely without closing PubSub on failure

1 participant