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
This commit is contained in:
Thomas Camlong committed 2026-08-07 19:04:53 +02:00
1 parent 018f98e318
commit 6b45aca5d5
6 files changed
+25 -16

No files matched your search

@@ -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
-1
View File
@@ -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");
});
-1
View File
@@ -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;
+8 -2
View File
@@ -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<T> {
data: T;
@@ -19,6 +21,10 @@ export const getCachedIntegrationData = async <T>(
fetcher: () => Promise<T>,
ttlMs = DEFAULT_TTL_MS,
): Promise<T> => {
if (isMultiInstance) {
return fetcher();
}
const key = cacheKey(integrationId, queryKey);
const existing = cache.get(key) as CachedEntry<T> | undefined;
const now = Date.now();
+2
View File
@@ -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")
@@ -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<void> {
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
}