reduce companion redis key timeouts to 10 min#6357
Conversation
🦋 Changeset detectedLatest commit: 9c26b46 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
not sure if it's a breaking change. in theory it should break anything so i'm inclined to say minor |
In theory this shouldn't affect users
There was a problem hiding this comment.
Pull request overview
This PR reduces how long Uppy Companion keeps certain Redis-backed state around, aiming to prevent Redis space growth from long-lived keys created during uploads and (optionally) Express sessions.
Changes:
- Reduce Express session cookie lifetime (for the Grant OAuth redirect flow) from 1 day to 10 minutes.
- Reduce Redis TTL for per-upload progress/state keys (
companion:<uuid>) from 24 hours to 10 minutes. - Centralize the auth-token cookie name logic via a shared
getCookieName()helper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/@uppy/companion/src/standalone/index.ts | Reduces session cookie maxAge to 10 minutes (currently only when COMPANION_COOKIE_DOMAIN is set). |
| packages/@uppy/companion/src/server/Uploader.ts | Reduces Redis TTL for upload state keys to 10 minutes. |
| packages/@uppy/companion/src/server/middlewares.ts | Uses shared helper to compute the auth cookie name. |
| packages/@uppy/companion/src/server/helpers/jwt.ts | Exports getCookieName() for reuse. |
| .changeset/purple-vans-act.md | Adds a changeset entry for the patch release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mifi can you fix lint ? |
qxprakash
left a comment
qxprakash
left a comment
There was a problem hiding this comment.
code wise lgtm ! , locally tested. I just have a doubt about 1 possible edgecase which can come with reduced TTL.
| saveState(state: { action: string; payload: unknown }): void { | ||
| if (!this.storage) return | ||
| // make sure the keys get cleaned up. | ||
| // We don't need more than 10 minutes because progress events should make sure that it doesn't expire before the upload is done. |
There was a problem hiding this comment.
We don't need more than 10 minutes because progress events should make sure that it doesn't expire before the upload is done.
but this is only true while the same Companion process is alive and actively uploading right ? the moment that process dies or a reconnect lands on a different instance (behind a load balancer) redis is the only source of truth ? in that case 10 min TTL might be too small ?
I tested this locally by reducing the TTL to 15 seconds: paused an upload, waited 15 seconds, restarted Companion server , the upload stalled , can this be a bug in production ?
https://github.com/transloadit/api2/pull/8345#pullrequestreview-4552806932
See also #3748 and #4394