Skip to content

Conversation

@seanlion
Copy link
Contributor

@seanlion seanlion commented Oct 26, 2025

What was the problem

  • The Redis retriever's Shutdown() method was calling client.Shutdown(ctx), which sends a Redis SHUTDOWN command to the server. This command is intended to shut down the entire Redis server, not just close the client connection.
  • Missing error handling: The error from the shutdown call was ignored

Changes

  • Replaced client.Shutdown(ctx) with client.Close() to only close the client connection
  • Added nil check to handle shutdown before initialization
  • Set client to nil after closing to prevent reuse
  • Ignore redis.ErrClosed errors for idempotency

Why Close() instead of Shutdown():

  • Close(): Closes the client connection pool
  • Shutdown(): Sends SHUTDOWN command to Redis server

How to test the change

  • Added Test_Redis_Shutdown with test cases.

Related Issue

#4139

Checklist

  • I have tested this code
  • I have added unit test to cover this code
  • I have updated the documentation (README.md and /website/docs)
  • I have followed the contributing guide

@netlify
Copy link

netlify bot commented Oct 26, 2025

Deploy Preview for go-feature-flag-doc-preview canceled.

Name Link
🔨 Latest commit c15c70c
🔍 Latest deploy log https://app.netlify.com/projects/go-feature-flag-doc-preview/deploys/68fdf249ae722300085ab7c1

@seanlion
Copy link
Contributor Author

/gemini review

@codecov
Copy link

codecov bot commented Oct 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.93%. Comparing base (a027586) to head (c15c70c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4140      +/-   ##
==========================================
+ Coverage   84.83%   84.93%   +0.10%     
==========================================
  Files         139      138       -1     
  Lines        5677     5643      -34     
==========================================
- Hits         4816     4793      -23     
+ Misses        651      644       -7     
+ Partials      210      206       -4     

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly addresses a critical issue by replacing client.Shutdown() with client.Close(), preventing the accidental shutdown of the entire Redis server. The changes are robust, incorporating nil checks, setting the client to nil after closing to prevent reuse, and handling redis.ErrClosed for idempotency. The accompanying tests are thorough and cover the new logic well. I have provided a few suggestions to enhance code clarity and consistency.

@seanlion seanlion changed the title fix(redis): use Close() instead of Shutdown() fix: use redis Close() instead of Shutdown() Oct 26, 2025
thomaspoignant and others added 2 commits October 26, 2025 10:49
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

Hey @seanlion thanks a lot for this catch, I will merge this change and it will be part of the next release.

I've added some tests to validate all the cases of the shutdown function.

@sonarqubecloud
Copy link

@kodiakhq kodiakhq bot merged commit a28007f into thomaspoignant:main Oct 26, 2025
24 checks passed
@seanlion
Copy link
Contributor Author

Hey @seanlion thanks a lot for this catch, I will merge this change and it will be part of the next release.

I've added some tests to validate all the cases of the shutdown function.

@thomaspoignant Thanks for review!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants