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

💾 feat: Production-ready Memory Store for express-session #5212

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

lkiesow
Copy link
Contributor

@lkiesow lkiesow commented Jan 8, 2025

The express-session library comes with a session storage meant for testing by default. That is why you get a message like this when you start up LibreChat with OIDC enabled:

Warning: connect.session() MemoryStore is not
designed for a production environment, as it will leak
memory, and will not scale past a single process.

LibreChat can already use Redis as a session storage, although Redis support is still marked as experimental. It also makes the set-up more complex, since you will need to configure and run yet another service.

This pull request provides a simple alternative by using a in-memory session store marked as a production-ready alternative by the guys from express-session¹. You can still configure Redis, but this provides a simple, good default for everyone else.

See also #1014

¹⁾ https://github.com/expressjs/session?tab=readme-ov-file#compatible-session-stores

Change Type

Please delete any irrelevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Translation update

Testing

  • Configure OIDC
  • Running without this patch: You should see the memry leak warning from above
  • Running with this patch applied: No warning should show up
  • In any case, the login via OIDC should still work

The `express-session` library comes with a session storage meant for
testing by default. That is why you get a message like this when you
start up LibreChat with OIDC enabled:

    Warning: connect.session() MemoryStore is not
    designed for a production environment, as it will leak
    memory, and will not scale past a single process.

LibreChat can already use Redis as a session storage, although Redis support
is still marked as experimental. It also makes the set-up more complex, since
you will need to configure and run yet another service.

This pull request provides a simple alternative by using a in-memory session
store marked as a production-ready alternative by the guys from
`express-session`¹. You can still configure Redis, but this provides a simple,
good default for everyone else.

See also danny-avila#1014

¹⁾ https://github.com/expressjs/session?tab=readme-ov-file#compatible-session-stores
@dennis531
Copy link
Contributor

We have the problem that the session is undefined sometimes. I found a similar problem on https://stackoverflow.com/questions/63259184/node-with-express-session-issue but there is no working solution. It seems to be a race condition. I hope the new memory store solves this problem.

Logs

LibreChat | Error: did not find expected authorization request details in session, req.session["oidc:studip.de"] is undefined
LibreChat | at /app/node_modules/openid-client/lib/passport_strategy.js:132:13
LibreChat | at OpenIDConnectStrategy.authenticate (/app/node_modules/openid-client/lib/passport_strategy.js:191:5)
LibreChat | at attempt (/app/node_modules/passport/lib/middleware/authenticate.js:369:16)
LibreChat | at authenticate (/app/node_modules/passport/lib/middleware/authenticate.js:370:7)
LibreChat | at Layer.handle [as handle_request] (/app/node_modules/express/lib/router/layer.js:95:5)
LibreChat | at next (/app/node_modules/express/lib/router/route.js:149:13)
LibreChat | at Route.dispatch (/app/node_modules/express/lib/router/route.js:119:3)
LibreChat | at Layer.handle [as handle_request] (/app/node_modules/express/lib/router/layer.js:95:5)
LibreChat | at /app/node_modules/express/lib/router/index.js:284:15
LibreChat | at Function.process_params (/app/node_modules/express/lib/router/index.js:346:12)

@danny-avila danny-avila changed the title Provide production-ready memory store for eypress-session 💾 feat: Production-ready Memory Store for express-session Jan 8, 2025
@danny-avila danny-avila merged commit dd92758 into danny-avila:main Jan 9, 2025
1 check passed
danny-avila added a commit that referenced this pull request Jan 10, 2025
danny-avila added a commit that referenced this pull request Jan 10, 2025
* ✨ feat: Add OpenWeather Tool for Weather Data Retrieval 🌤️

* chore: linting

* chore: move test files

* fix: tool icon, allow user-provided keys, conform to app key assignment pattern

* chore: linting not included in #5212

---------

Co-authored-by: Jonathan Addington <[email protected]>
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.

3 participants