fix: address coderabbit review feedback
This commit is contained in:
ajnart committed
1 parent
17aef0588f
commit
cf624e2f23
10 files changed
+256
-175
No files matched your search
@@ -7,13 +7,24 @@ import type { RedisClient } from "@homarr/core/infrastructure/redis";
|
||||
import type { CustomWidgetHttpRequest, CustomWidgetHttpResponse } from "@homarr/custom-widgets/server";
|
||||
|
||||
const ENTRY_PREFIX = "custom-widget:response-cache:v2:entry:";
|
||||
const GENERATION_KEY = "custom-widget:response-cache:v2:generation";
|
||||
const GENERATION_PREFIX = "custom-widget:response-cache:v2:generation:";
|
||||
const LRU_KEY = "custom-widget:response-cache:v2:lru";
|
||||
const SIZE_KEY = "custom-widget:response-cache:v2:sizes";
|
||||
const TOTAL_BYTES_KEY = "custom-widget:response-cache:v2:bytes";
|
||||
// Outlive the one-hour response TTL and 45-second request deadline so a
|
||||
// generation cannot reset while an entry from the previous one is reachable.
|
||||
const GENERATION_RETENTION_SECONDS = 2 * 60 * 60;
|
||||
const MAX_ENTRIES = 1_000;
|
||||
const MAX_BYTES = 64 * 1024 * 1024;
|
||||
|
||||
const INVALIDATE_GENERATIONS_SCRIPT = `
|
||||
for _, key in ipairs(KEYS) do
|
||||
redis.call('INCR', key)
|
||||
redis.call('EXPIRE', key, ARGV[1])
|
||||
end
|
||||
return #KEYS
|
||||
`;
|
||||
|
||||
const STORE_SCRIPT = `
|
||||
local previousSize = tonumber(redis.call('HGET', KEYS[2], ARGV[1]) or '0')
|
||||
if previousSize > 0 then redis.call('DECRBY', KEYS[3], previousSize) end
|
||||
@@ -34,7 +45,7 @@ return totalBytes
|
||||
`;
|
||||
|
||||
const logger = createLogger({ module: "custom-widget:response-cache" });
|
||||
const inFlight = new Map<string, Promise<CustomWidgetHttpResponse>>();
|
||||
const inFlight = new Map<string, { cacheKey: string; request: Promise<CustomWidgetHttpResponse> }>();
|
||||
let redis: RedisClient | undefined;
|
||||
|
||||
const useSharedCache = () => process.env.NODE_ENV !== "test";
|
||||
@@ -44,9 +55,26 @@ const getRedis = () => {
|
||||
return redis;
|
||||
};
|
||||
|
||||
const getGeneration = async (client: RedisClient) => Number((await client.get(GENERATION_KEY)) ?? 0);
|
||||
const getGenerationKey = (prefix: string) => `${GENERATION_PREFIX}${createHash("sha256").update(prefix).digest("hex")}`;
|
||||
|
||||
const getEntryKey = (generation: number, cacheKey: string) => {
|
||||
const getCacheKeyPrefixes = (cacheKey: string) => {
|
||||
const prefixes: string[] = [];
|
||||
let separatorIndex = cacheKey.indexOf(":");
|
||||
while (separatorIndex >= 0) {
|
||||
prefixes.push(cacheKey.slice(0, separatorIndex + 1));
|
||||
separatorIndex = cacheKey.indexOf(":", separatorIndex + 1);
|
||||
}
|
||||
prefixes.push(cacheKey);
|
||||
return prefixes;
|
||||
};
|
||||
|
||||
const getGeneration = async (client: RedisClient, cacheKey: string) => {
|
||||
const keys = getCacheKeyPrefixes(cacheKey).map(getGenerationKey);
|
||||
const generations = await client.mget(...keys);
|
||||
return generations.map((generation) => generation ?? "0").join(":");
|
||||
};
|
||||
|
||||
const getEntryKey = (generation: string, cacheKey: string) => {
|
||||
const digest = createHash("sha256").update(cacheKey).digest("hex");
|
||||
return `${ENTRY_PREFIX}${generation}:${digest}`;
|
||||
};
|
||||
@@ -96,10 +124,10 @@ export async function executeWithCustomWidgetResponseCache(
|
||||
if (!useSharedCache()) return execute(input);
|
||||
|
||||
let client: RedisClient;
|
||||
let generation: number;
|
||||
let generation: string;
|
||||
try {
|
||||
client = getRedis();
|
||||
generation = await getGeneration(client);
|
||||
generation = await getGeneration(client, cacheKey);
|
||||
if ((input.cacheTtlSeconds ?? 0) > 0) {
|
||||
const cached = await readCachedResponse(client, getEntryKey(generation, cacheKey));
|
||||
if (cached) return cached;
|
||||
@@ -113,7 +141,7 @@ export async function executeWithCustomWidgetResponseCache(
|
||||
|
||||
const pendingKey = `${generation}:${cacheKey}`;
|
||||
const pending = inFlight.get(pendingKey);
|
||||
if (pending) return pending;
|
||||
if (pending) return pending.request;
|
||||
|
||||
let request: Promise<CustomWidgetHttpResponse>;
|
||||
request = execute({ ...input, cacheKey: undefined })
|
||||
@@ -121,7 +149,7 @@ export async function executeWithCustomWidgetResponseCache(
|
||||
const ttlSeconds = input.cacheTtlSeconds ?? 0;
|
||||
if (!response.ok || ttlSeconds <= 0) return response;
|
||||
try {
|
||||
const currentGeneration = await getGeneration(client);
|
||||
const currentGeneration = await getGeneration(client, cacheKey);
|
||||
if (currentGeneration === generation) {
|
||||
await storeCachedResponse(client, getEntryKey(generation, cacheKey), response, ttlSeconds);
|
||||
}
|
||||
@@ -133,19 +161,21 @@ export async function executeWithCustomWidgetResponseCache(
|
||||
return response;
|
||||
})
|
||||
.finally(() => {
|
||||
if (inFlight.get(pendingKey) === request) inFlight.delete(pendingKey);
|
||||
if (inFlight.get(pendingKey)?.request === request) inFlight.delete(pendingKey);
|
||||
});
|
||||
inFlight.set(pendingKey, request);
|
||||
inFlight.set(pendingKey, { cacheKey, request });
|
||||
return request;
|
||||
}
|
||||
|
||||
export async function invalidateSharedCustomWidgetResponseCache(prefixes: readonly string[]) {
|
||||
if (prefixes.length === 0 || !useSharedCache()) return;
|
||||
for (const key of inFlight.keys()) {
|
||||
if (prefixes.some((prefix) => key.includes(prefix))) inFlight.delete(key);
|
||||
const uniquePrefixes = [...new Set(prefixes)];
|
||||
for (const [key, pending] of inFlight) {
|
||||
if (uniquePrefixes.some((prefix) => pending.cacheKey.startsWith(prefix))) inFlight.delete(key);
|
||||
}
|
||||
try {
|
||||
await getRedis().incr(GENERATION_KEY);
|
||||
const keys = uniquePrefixes.map(getGenerationKey);
|
||||
await getRedis().eval(INVALIDATE_GENERATIONS_SCRIPT, keys.length, ...keys, GENERATION_RETENTION_SECONDS);
|
||||
} catch (error) {
|
||||
logger.error("Failed to invalidate the shared custom widget response cache", {
|
||||
errorName: error instanceof Error ? error.name : "UnknownError",
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
// @vitest-environment node
|
||||
|
||||
import { describe, expect, test, vi } from "vitest";
|
||||
import { afterEach, describe, expect, test, vi } from "vitest";
|
||||
import { parse } from "superjson";
|
||||
|
||||
import type { Session } from "@homarr/auth";
|
||||
@@ -34,6 +34,14 @@ vi.mock("@homarr/core/infrastructure/logs", () => ({
|
||||
vi.mock("@homarr/core/infrastructure/db/env", () => ({
|
||||
dbEnv: { DRIVER: "better-sqlite3" },
|
||||
}));
|
||||
const mocks = vi.hoisted(() => ({ fetchWithTrustedCertificatesAsync: vi.fn() }));
|
||||
vi.mock("@homarr/core/infrastructure/http", () => ({
|
||||
fetchWithTrustedCertificatesAsync: mocks.fetchWithTrustedCertificatesAsync,
|
||||
}));
|
||||
|
||||
afterEach(() => {
|
||||
mocks.fetchWithTrustedCertificatesAsync.mockReset();
|
||||
});
|
||||
|
||||
const adminSession = {
|
||||
user: {
|
||||
@@ -169,30 +177,23 @@ describe("assistantRouter.updateConfiguration", () => {
|
||||
.set({ encryptedHeaders: null })
|
||||
.where(eq(assistantConfigurations.id, "default"));
|
||||
const caller = assistantRouter.createCaller({ db, deviceType: undefined, session: adminSession });
|
||||
vi.stubGlobal(
|
||||
"fetch",
|
||||
vi.fn().mockResolvedValue(
|
||||
Response.json({
|
||||
data: [
|
||||
{
|
||||
id: "deepseek/deepseek-v4-pro",
|
||||
name: "DeepSeek: DeepSeek V4 Pro",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
],
|
||||
}),
|
||||
),
|
||||
mocks.fetchWithTrustedCertificatesAsync.mockResolvedValue(
|
||||
Response.json({
|
||||
data: [
|
||||
{
|
||||
id: "deepseek/deepseek-v4-pro",
|
||||
name: "DeepSeek: DeepSeek V4 Pro",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
],
|
||||
}),
|
||||
);
|
||||
|
||||
try {
|
||||
await caller.updateConfiguration({
|
||||
enabled: true,
|
||||
modelId: "DeepSeek: DeepSeek V4 Pro (deepseek/deepseek-v4-pro)",
|
||||
});
|
||||
} finally {
|
||||
vi.unstubAllGlobals();
|
||||
}
|
||||
await caller.updateConfiguration({
|
||||
enabled: true,
|
||||
modelId: "DeepSeek: DeepSeek V4 Pro (deepseek/deepseek-v4-pro)",
|
||||
});
|
||||
|
||||
const [configuration] = await db.select().from(assistantConfigurations);
|
||||
expect(configuration?.modelId).toBe("deepseek/deepseek-v4-pro");
|
||||
@@ -221,31 +222,24 @@ describe("assistantRouter.updateConfiguration", () => {
|
||||
})
|
||||
.where(eq(assistantConfigurations.id, "default"));
|
||||
const caller = assistantRouter.createCaller({ db, deviceType: undefined, session: adminSession });
|
||||
vi.stubGlobal(
|
||||
"fetch",
|
||||
vi.fn().mockResolvedValue(
|
||||
Response.json({
|
||||
data: [
|
||||
{
|
||||
id: "homarr/model",
|
||||
name: "Homarr model",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
],
|
||||
}),
|
||||
),
|
||||
mocks.fetchWithTrustedCertificatesAsync.mockResolvedValue(
|
||||
Response.json({
|
||||
data: [
|
||||
{
|
||||
id: "homarr/model",
|
||||
name: "Homarr model",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
],
|
||||
}),
|
||||
);
|
||||
|
||||
try {
|
||||
await caller.updateConfiguration({
|
||||
enabled: true,
|
||||
modelId: "homarr/model",
|
||||
webSearchEnabled: true,
|
||||
});
|
||||
} finally {
|
||||
vi.unstubAllGlobals();
|
||||
}
|
||||
await caller.updateConfiguration({
|
||||
enabled: true,
|
||||
modelId: "homarr/model",
|
||||
webSearchEnabled: true,
|
||||
});
|
||||
|
||||
const [configuration] = await db.select().from(assistantConfigurations);
|
||||
expect(configuration?.webSearchEnabled).toBe(true);
|
||||
@@ -277,56 +271,49 @@ describe("assistant conversation features", () => {
|
||||
})
|
||||
.where(eq(assistantConfigurations.id, "default"));
|
||||
const caller = assistantRouter.createCaller({ db, deviceType: undefined, session: adminSession });
|
||||
vi.stubGlobal(
|
||||
"fetch",
|
||||
vi.fn().mockResolvedValue(
|
||||
Response.json({
|
||||
data: [
|
||||
{
|
||||
id: "example/model",
|
||||
name: "Example Model",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { input_modalities: ["text", "image"], output_modalities: ["text"] },
|
||||
},
|
||||
{
|
||||
id: "example/no-tools",
|
||||
name: "No tools",
|
||||
supported_parameters: [],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
{
|
||||
id: "example/unnamed",
|
||||
name: "",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
],
|
||||
}),
|
||||
),
|
||||
);
|
||||
|
||||
try {
|
||||
const options = await caller.getRuntimeOptions();
|
||||
|
||||
expect(options).toEqual({
|
||||
provider: "openrouter",
|
||||
defaultModelId: "example/model",
|
||||
models: [
|
||||
expect.objectContaining({
|
||||
mocks.fetchWithTrustedCertificatesAsync.mockResolvedValue(
|
||||
Response.json({
|
||||
data: [
|
||||
{
|
||||
id: "example/model",
|
||||
name: "Example Model",
|
||||
inputModalities: ["text", "image"],
|
||||
}),
|
||||
expect.objectContaining({
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { input_modalities: ["text", "image"], output_modalities: ["text"] },
|
||||
},
|
||||
{
|
||||
id: "example/no-tools",
|
||||
name: "No tools",
|
||||
supported_parameters: [],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
{
|
||||
id: "example/unnamed",
|
||||
name: "example/unnamed",
|
||||
}),
|
||||
name: "",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
],
|
||||
});
|
||||
expect(JSON.stringify(options)).not.toContain("encrypted");
|
||||
} finally {
|
||||
vi.unstubAllGlobals();
|
||||
}
|
||||
}),
|
||||
);
|
||||
|
||||
const options = await caller.getRuntimeOptions();
|
||||
|
||||
expect(options).toEqual({
|
||||
provider: "openrouter",
|
||||
defaultModelId: "example/model",
|
||||
models: [
|
||||
expect.objectContaining({
|
||||
id: "example/model",
|
||||
name: "Example Model",
|
||||
inputModalities: ["text", "image"],
|
||||
}),
|
||||
expect.objectContaining({
|
||||
id: "example/unnamed",
|
||||
name: "example/unnamed",
|
||||
}),
|
||||
],
|
||||
});
|
||||
expect(JSON.stringify(options)).not.toContain("encrypted");
|
||||
});
|
||||
|
||||
test("returns permission-checked entities for composer mentions", async () => {
|
||||
@@ -383,33 +370,26 @@ describe("assistant conversation features", () => {
|
||||
.where(eq(assistantConfigurations.id, "default"));
|
||||
const caller = assistantRouter.createCaller({ db, deviceType: undefined, session: adminSession });
|
||||
const thread = await caller.createThread();
|
||||
vi.stubGlobal(
|
||||
"fetch",
|
||||
vi.fn().mockResolvedValue(
|
||||
Response.json({
|
||||
data: [
|
||||
{
|
||||
id: "example/model",
|
||||
name: "Default",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
{
|
||||
id: "example/reasoning",
|
||||
name: "Reasoning",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
],
|
||||
}),
|
||||
),
|
||||
mocks.fetchWithTrustedCertificatesAsync.mockResolvedValue(
|
||||
Response.json({
|
||||
data: [
|
||||
{
|
||||
id: "example/model",
|
||||
name: "Default",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
{
|
||||
id: "example/reasoning",
|
||||
name: "Reasoning",
|
||||
supported_parameters: ["tools"],
|
||||
architecture: { output_modalities: ["text"] },
|
||||
},
|
||||
],
|
||||
}),
|
||||
);
|
||||
|
||||
try {
|
||||
await caller.updateThreadModel({ threadId: thread.id, modelId: "example/reasoning" });
|
||||
} finally {
|
||||
vi.unstubAllGlobals();
|
||||
}
|
||||
await caller.updateThreadModel({ threadId: thread.id, modelId: "example/reasoning" });
|
||||
|
||||
const storedThread = await db.query.assistantThreads.findFirst({
|
||||
where: eq(assistantThreads.id, thread.id),
|
||||
|
||||
Reference in new issue
Block a user