Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🔧 fix(redis): Resolve Redis Standalone vs Cluster mode discrepancy for social logins #2848

Merged
merged 1 commit into from
May 24, 2024

Conversation

arthurian
Copy link
Contributor

@arthurian arthurian commented May 23, 2024

Summary

Resolves an issue with OpenID authentication requests hanging and timing out when using Redis in Standalone mode (see also #2843).

Steps to reproduce:

  • Set up a Standalone Redis server.
  • Set the USE_REDIS and REDIS_URI env vars.
  • Set the ALLOW_SOCIAL_LOGIN and OPENID_* env vars.
  • Navigate to login page.
  • Click Continue with OpenID.
  • Request times out.

This issue can be traced to the way social logins are configured at startup, since a RedisStore is configured for session storage with the client from api/cache/redis.js, but this client assumes that Redis is running in Cluster mode, and as a result it fails to connect (times out). The connection error is silently ignored, so it doesn't show up in the log output. By contrast, the api/cache/keyvRedis.js client successfully connects to Redis in Standalone mode and reports Redis initialized.

As it stands, neither redis.js nor keyvRedis.js support Cluster mode and doing so would require introducing additional configuration and logic, so the proposed fix is to resolve the immediate discrepancy to ensure that both work with Redis in Standalone mode. The suggestions proposed in #2843 to support Cluster mode could be considered for a subsequent enhancement.

Change Type

  • Bug fix (non-breaking change which fixes an issue)

Testing

Test Configuration:

I have confirmed that the proposed updates work with an AWS ElasticCache Redis installation running in Standalone mode. There's no additional configuration required to test this besides setting the environment variables to use redis and OpenID authentication.

Test Script:

The following script adapted from api/cache/redis.js can be used to show the problem when REDIS_URI points to a Standalone installation:

const Redis = require('ioredis');
const { REDIS_URI } = process.env; 

const redisCluster = new Redis.Cluster(REDIS_URI, { lazyConnect: true });
redisCluster.connect()
  .then(() => console.log('redis cluster initialized'))
  .catch((err) => console.error(`redis cluster error: ${err}`));

const redisStandalone = new Redis(REDIS_URI, { lazyConnect: true });
redisStandalone.connect()
  .then(() => console.log('redis standalone initialized'))
  .catch((err) => console.error(`redis standalone error: ${err}`));

The following output is expected when running the script:

redis cluster error: Error: `startupNodes` should contain at least one node.
redis standalone initialized

Checklist

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code
  • My changes do not introduce new warnings
  • Local unit tests pass with my changes

@danny-avila
Copy link
Owner

Thank you!

@danny-avila danny-avila merged commit dcd2e3e into danny-avila:main May 24, 2024
1 check passed
@arthurian arthurian deleted the fix/redis-session-store branch May 24, 2024 14:36
@danny-avila
Copy link
Owner

Unfortunately just noticed there are elevated errors when Redis is not configured:

image

The session caching is initialized like this: api/server/socialLogins.js

sessionOptions.store = new RedisStore({ client, prefix: 'librechat' });

Seems to be attempting connection even when it's not configured?

@arthurian
Copy link
Contributor Author

Ah yes, you're right. That's my fault -- the event handlers should be added only if USE_REDIS is enabled in api/cache/redis.js.

@danny-avila
Copy link
Owner

danny-avila commented May 24, 2024

@arthurian I reverted the changes and adding this to prevent wall of errors, let me know if this change works for you. This might be the most optimal solution if it does. It makes sense to use different clients for separate purposes

@danny-avila
Copy link
Owner

Actually made an error, here is the updated commit: 0f42793

@arthurian
Copy link
Contributor Author

Yeah, that would work for us. The only addendum I would suggest is to make sure the Redis standalone client is passed in to RedisStore, so change:

new RedisStore({ client, prefix: 'librechat' });

To:

new RedisStore({ client: redis, prefix: 'librechat' });

@arthurian
Copy link
Contributor Author

Actually nvm, I saw in your updated commit 0f42793 you renamed redis to client.

danny-avila pushed a commit that referenced this pull request May 27, 2024
danny-avila pushed a commit that referenced this pull request Aug 5, 2024
kenshinsamue pushed a commit to intelequia/LibreChat that referenced this pull request Sep 17, 2024
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.

2 participants