Skip to content

fix: close redis connections on unsubscribe#2917

Draft
dkorittki wants to merge 3 commits into
mainfrom
dominik/close_redis_connections_on_unsubscribe
Draft

fix: close redis connections on unsubscribe#2917
dkorittki wants to merge 3 commits into
mainfrom
dominik/close_redis_connections_on_unsubscribe

Conversation

@dkorittki

@dkorittki dkorittki commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability of Redis subscription cleanup so subscriptions are properly terminated even when request contexts are cancelled.
    • Enhanced teardown logging for Redis subscriptions to produce clearer, more actionable diagnostic messages during shutdown and error scenarios.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/docs-website.
  • I have read the Contributors Guide.

Open Source AI Manifesto

This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.

@github-actions github-actions Bot added the router label Jun 4, 2026
@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 06ab1301-67f5-44b6-be03-ccc2c5196567

📥 Commits

Reviewing files that changed from the base of the PR and between b273296 and 13eb7e7.

📒 Files selected for processing (1)
  • router/pkg/pubsub/redis/adapter.go

Walkthrough

The Redis adapter adds a time import, and the subscription cleanup now calls sub.PUnsubscribe with context.Background() using a 1-second timeout and logs sub.Close() errors by embedding the error into a formatted message string.

Changes

Redis subscription cleanup

Layer / File(s) Summary
Import and subscription cleanup update
router/pkg/pubsub/redis/adapter.go
Adds time import; cleanup calls sub.PUnsubscribe with context.Background() and a 1s timeout, and updates sub.Close() error logging to use fmt.Sprintf embedding the error via %v.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: close redis connections on unsubscribe' is directly related to the main change in the changeset, which modifies the Subscribe cleanup closure to perform PUnsubscribe with a separate context and timeout.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 4, 2026

Copy link
Copy Markdown

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-f74ded6acba14fa51528f573dd136a19cf66f77e

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@router/pkg/pubsub/redis/adapter.go`:
- Around line 108-111: The current call wraps zap.Error(err) inside fmt.Sprintf
with a %w verb which is invalid and breaks structured logging; replace the
fmt.Sprintf usage and call log.Error with a message string and pass
zap.Error(err) as a field (e.g., in the error handling after sub.Close() use
log.Error("error closing connection to redis", zap.Error(err))) so zap produces
structured error output; locate the block around sub.Close(), the log.Error
invocation, and the zap.Error(err) usage to make this change.
- Line 103: The call sub.PUnsubscribe(context.Background(), subConf.Channels...)
in the cleanup function can block Shutdown via p.closeWg.Wait(); replace the
background context with a context.WithTimeout (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), <reasonableDuration>); defer cancel())
and pass ctx to sub.PUnsubscribe so the unsubscribe is bounded, and add "time"
to the imports if you use a time constant; ensure cleanup still calls
p.closeWg.Done() after the unsubscribe attempt and that
ProviderAdapter.Shutdown() relies on the bounded wait.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b0dda8b6-6496-4515-a4f0-810bd12ee3d3

📥 Commits

Reviewing files that changed from the base of the PR and between 570cbf3 and 0c314e9.

📒 Files selected for processing (1)
  • router/pkg/pubsub/redis/adapter.go

Comment thread router/pkg/pubsub/redis/adapter.go Outdated
Comment thread router/pkg/pubsub/redis/adapter.go
@codecov

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.29%. Comparing base (85ada72) to head (13eb7e7).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
router/pkg/pubsub/redis/adapter.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2917      +/-   ##
==========================================
+ Coverage   66.26%   66.29%   +0.02%     
==========================================
  Files         258      258              
  Lines       27309    27314       +5     
==========================================
+ Hits        18097    18107      +10     
+ Misses       7773     7770       -3     
+ Partials     1439     1437       -2     
Files with missing lines Coverage Δ
router/pkg/pubsub/redis/adapter.go 65.03% <66.66%> (+2.71%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ehaynes99 added a commit to OpenPhone/cosmo that referenced this pull request Jun 4, 2026
… (#123)

* fix: close redis connections on unsubscribe

* fix: use correct format identifier in Sprintf

* fix: use fixed timeout for unsubscribe

---------

Co-authored-by: Dominik Korittki <23359034+dkorittki@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant