From 6b45aca5d52935136a66535297a8c89529ca1ba8 Mon Sep 17 00:00:00 2001 From: Thomas Camlong Date: Fri, 7 Aug 2026 19:04:53 +0200 Subject: [PATCH] fix: address Bugbot review findings - Skip module-level integration cache for multi-instance (REDIS_IS_EXTERNAL) - Add invalidateBoardCache to permission mutations (prevents future leak) - Remove unused cacheTags.integrationData - Replace KEYS with SCAN in Redis cache handler refreshTags() - Document REDIS_CACHE_DB env var --- .../advanced/environment-variables/index.mdx | 1 + packages/api/src/cache-tags.spec.ts | 1 - packages/api/src/cache-tags.ts | 1 - packages/api/src/integration-data-cache.ts | 10 +++++-- packages/api/src/router/board.ts | 2 ++ .../next-cache-handler/src/redis-handler.ts | 26 ++++++++++--------- 6 files changed, 25 insertions(+), 16 deletions(-) diff --git a/apps/docs/docs/advanced/environment-variables/index.mdx b/apps/docs/docs/advanced/environment-variables/index.mdx index 023ffd34a..d47660b68 100644 --- a/apps/docs/docs/advanced/environment-variables/index.mdx +++ b/apps/docs/docs/advanced/environment-variables/index.mdx @@ -90,6 +90,7 @@ External redis is currently not supported with Proxmox Community Scripts | `REDIS_USERNAME` | Username to connect to the Redis instance. | Any valid username | - | | `REDIS_PASSWORD` | Password to connect to the Redis instance. | Any valid password | - | | `REDIS_DATABASE_INDEX` | Select which database of your redis instance should be used. | Any numeric index | - | +| `REDIS_CACHE_DB` | Redis database number for the Next.js cache handler. Separate from the main Redis DB to avoid key collisions. | Any numeric index | `1` | | `REDIS_TLS_CA` | CA certificate for Redis TLS connections. If a certificate is specified the connection will use TLS. | Any valid CA certificate | - | ## Proxy diff --git a/packages/api/src/cache-tags.spec.ts b/packages/api/src/cache-tags.spec.ts index d98706d8e..4c929dae3 100644 --- a/packages/api/src/cache-tags.spec.ts +++ b/packages/api/src/cache-tags.spec.ts @@ -10,7 +10,6 @@ describe("cacheTags", () => { expect(cacheTags.boardList()).toBe("board-list"); expect(cacheTags.user("u1")).toBe("user:u1"); expect(cacheTags.integration("i1")).toBe("integration:i1"); - expect(cacheTags.integrationData("i1", "downloads")).toBe("integration-data:i1:downloads"); expect(cacheTags.app("a1")).toBe("app:a1"); }); diff --git a/packages/api/src/cache-tags.ts b/packages/api/src/cache-tags.ts index c534e52a6..d22fe22f4 100644 --- a/packages/api/src/cache-tags.ts +++ b/packages/api/src/cache-tags.ts @@ -5,6 +5,5 @@ export const cacheTags = { boardList: () => "board-list" as const, user: (userId: string) => `user:${userId}` as const, integration: (integrationId: string) => `integration:${integrationId}` as const, - integrationData: (integrationId: string, query: string) => `integration-data:${integrationId}:${query}` as const, app: (appId: string) => `app:${appId}` as const, } as const; diff --git a/packages/api/src/integration-data-cache.ts b/packages/api/src/integration-data-cache.ts index 896630f7f..d55fa8735 100644 --- a/packages/api/src/integration-data-cache.ts +++ b/packages/api/src/integration-data-cache.ts @@ -1,5 +1,7 @@ -// ponytail: module-level cache — one snapshot per integration per query key. -// Ceiling: stale data for up to `ttlMs` (60s default). Upgrade: Redis-backed with pub/sub invalidation. +// ponytail: module-level cache is single-instance only. +// Multi-instance deployments (REDIS_IS_EXTERNAL=true) bypass this and rely on +// the Redis-backed Next cache handler for cross-instance coordination. +const isMultiInstance = process.env.REDIS_IS_EXTERNAL === "true"; interface CachedEntry { data: T; @@ -19,6 +21,10 @@ export const getCachedIntegrationData = async ( fetcher: () => Promise, ttlMs = DEFAULT_TTL_MS, ): Promise => { + if (isMultiInstance) { + return fetcher(); + } + const key = cacheKey(integrationId, queryKey); const existing = cache.get(key) as CachedEntry | undefined; const now = Date.now(); diff --git a/packages/api/src/router/board.ts b/packages/api/src/router/board.ts index 2f5cabe26..b425da89b 100644 --- a/packages/api/src/router/board.ts +++ b/packages/api/src/router/board.ts @@ -1426,6 +1426,7 @@ export const boardRouter = createTRPCRouter({ for (const userId of userIds) { invalidateUserCache(userId); } + invalidateBoardCache(input.entityId); }), saveGroupBoardPermissions: protectedProcedure.input(boardSavePermissionsSchema).mutation(async ({ input, ctx }) => { await throwIfActionForbiddenAsync(ctx, eq(boards.id, input.entityId), "full"); @@ -1488,6 +1489,7 @@ export const boardRouter = createTRPCRouter({ invalidateUserCache(member.userId); } } + invalidateBoardCache(input.entityId); }), importOldmarrConfig: permissionRequiredProcedure .requiresPermission("board-create") diff --git a/packages/next-cache-handler/src/redis-handler.ts b/packages/next-cache-handler/src/redis-handler.ts index 6018ccb0e..965f6274f 100644 --- a/packages/next-cache-handler/src/redis-handler.ts +++ b/packages/next-cache-handler/src/redis-handler.ts @@ -23,7 +23,7 @@ export class RedisCacheHandler implements CacheHandler { }); this.redis.on("error", () => { if (!this.loggedError) { - console.warn("[next-cache-handler] Redis connection error, falling back to memory"); + console.warn("[next-cache-handler] Redis connection error, cache will degrade to always-miss"); this.loggedError = true; } }); @@ -130,18 +130,20 @@ export class RedisCacheHandler implements CacheHandler { async refreshTags(): Promise { try { await this.redis.connect().catch(() => {}); - const keys = await this.redis.keys("nextCache:tag:*"); - if (keys.length === 0) { - this.tagCache.clear(); - return; - } - const values = await this.redis.mget(...keys); this.tagCache.clear(); - for (let i = 0; i < keys.length; i++) { - const tag = keys[i]!.replace("nextCache:tag:", ""); - const val = values[i]; - if (val) this.tagCache.set(tag, Number(val)); - } + let cursor = "0"; + do { + const [nextCursor, keys] = await this.redis.scan(cursor, "MATCH", "nextCache:tag:*", "COUNT", 100); + cursor = nextCursor; + if (keys.length > 0) { + const values = await this.redis.mget(...keys); + for (let i = 0; i < keys.length; i++) { + const tag = keys[i]!.replace("nextCache:tag:", ""); + const val = values[i]; + if (val) this.tagCache.set(tag, Number(val)); + } + } + } while (cursor !== "0"); } catch { // Fail open — stale tag cache is better than crash }