From add6ee56ba8d16d1b6ef23747103b894e957c075 Mon Sep 17 00:00:00 2001 From: ajnart Date: Tue, 28 Jul 2026 17:42:28 +0200 Subject: [PATCH] fix(updates): enforce a persisted daily check --- apps/tasks/src/on-start/index.ts | 2 - .../tasks/src/on-start/warm-update-checker.ts | 14 -- .../src/router/test/update-checker.spec.ts | 58 ++++++ packages/api/src/router/update-checker.ts | 4 +- packages/redis/src/index.ts | 1 + packages/redis/src/lib/channel.ts | 29 ++- .../src/test/update-checker-cache.spec.ts | 174 ++++++++++++++++++ .../request-handler/src/update-checker.ts | 94 +++++++++- 8 files changed, 349 insertions(+), 27 deletions(-) delete mode 100644 apps/tasks/src/on-start/warm-update-checker.ts create mode 100644 packages/api/src/router/test/update-checker.spec.ts create mode 100644 packages/request-handler/src/test/update-checker-cache.spec.ts diff --git a/apps/tasks/src/on-start/index.ts b/apps/tasks/src/on-start/index.ts index cc6967dce..15b873186 100644 --- a/apps/tasks/src/on-start/index.ts +++ b/apps/tasks/src/on-start/index.ts @@ -1,7 +1,5 @@ import { cleanupSessionsAsync } from "./session-cleanup"; -import { warmUpdateCheckerAsync } from "./warm-update-checker"; export async function onStartAsync() { await cleanupSessionsAsync(); - await warmUpdateCheckerAsync(); } diff --git a/apps/tasks/src/on-start/warm-update-checker.ts b/apps/tasks/src/on-start/warm-update-checker.ts deleted file mode 100644 index a3b9f94a9..000000000 --- a/apps/tasks/src/on-start/warm-update-checker.ts +++ /dev/null @@ -1,14 +0,0 @@ -import { createLogger } from "@homarr/core/infrastructure/logs"; -import { updateCheckerRequestHandler } from "@homarr/request-handler/update-checker"; - -const logger = createLogger({ module: "warmUpdateChecker" }); - -export async function warmUpdateCheckerAsync() { - try { - const handler = updateCheckerRequestHandler.handler({}); - await handler.getDataAsync(); - logger.debug("Update checker warmed"); - } catch (error) { - logger.error(new Error("Failed to warm update checker", { cause: error })); - } -} diff --git a/packages/api/src/router/test/update-checker.spec.ts b/packages/api/src/router/test/update-checker.spec.ts new file mode 100644 index 000000000..e35209f06 --- /dev/null +++ b/packages/api/src/router/test/update-checker.spec.ts @@ -0,0 +1,58 @@ +import { describe, expect, test, vi } from "vitest"; + +import type { Session } from "@homarr/auth"; +import type { Database } from "@homarr/db"; +import type { GroupPermissionKey } from "@homarr/definitions"; + +import { updateCheckerRouter } from "../update-checker"; + +const mocks = vi.hoisted(() => ({ + getDataAsync: vi.fn(async () => ({ + data: { availableUpdates: [{ tagName: "v99.0.0" }] }, + timestamp: new Date(), + })), +})); + +vi.mock("@homarr/request-handler/update-checker", () => ({ + updateCheckerRequestHandler: { + handler: () => ({ getDataAsync: mocks.getDataAsync }), + }, +})); + +const createSession = (permissions: GroupPermissionKey[]): Session => ({ + user: { + id: "user", + permissions, + colorScheme: "light", + }, + expires: new Date().toISOString(), +}); + +const createCaller = (session: Session | null) => + updateCheckerRouter.createCaller({ + db: null as unknown as Database, + deviceType: undefined, + session, + }); + +describe("update checker authorization", () => { + test("allows administrators to read the cached result", async () => { + await expect(createCaller(createSession(["admin"])).getAvailableUpdates()).resolves.toEqual([ + { tagName: "v99.0.0" }, + ]); + }); + + test("rejects authenticated users without admin permission", async () => { + await expect(createCaller(createSession([])).getAvailableUpdates()).rejects.toThrow("Permission denied"); + }); + + test("rejects anonymous users", async () => { + await expect(createCaller(null).getAvailableUpdates()).rejects.toThrow("UNAUTHORIZED"); + }); + + test("returns an empty result when the cache reader fails", async () => { + mocks.getDataAsync.mockRejectedValueOnce(new Error("Redis unavailable")); + + await expect(createCaller(createSession(["admin"])).getAvailableUpdates()).resolves.toEqual([]); + }); +}); diff --git a/packages/api/src/router/update-checker.ts b/packages/api/src/router/update-checker.ts index 8cb6c097e..cd7af0e56 100644 --- a/packages/api/src/router/update-checker.ts +++ b/packages/api/src/router/update-checker.ts @@ -12,8 +12,8 @@ export const updateCheckerRouter = createTRPCRouter({ const data = await handler.getDataAsync(); return data.data.availableUpdates; } catch (error) { - logger.error(new Error("Failed to get available updates", { cause: error })); - return undefined; // We return undefined to not show the indicator in the UI + logger.error(new Error("Failed to read the cached update check", { cause: error })); + return []; // An empty list hides the indicator without violating TanStack Query's data contract. } }), }); diff --git a/packages/redis/src/index.ts b/packages/redis/src/index.ts index 02258a2e0..90a854b9c 100644 --- a/packages/redis/src/index.ts +++ b/packages/redis/src/index.ts @@ -6,6 +6,7 @@ export { handshakeAsync, createSubPubChannel, createGetSetChannel, + createLockChannel, setQueryCacheAsync, getQueryCacheAsync, removeQueryCacheAsync, diff --git a/packages/redis/src/lib/channel.ts b/packages/redis/src/lib/channel.ts index 1b1e582c1..6f5771e83 100644 --- a/packages/redis/src/lib/channel.ts +++ b/packages/redis/src/lib/channel.ts @@ -114,10 +114,11 @@ export const createGetSetChannel = (name: string) => { * @param options optional TTL in seconds */ setAsync: async (data: TData, options?: { ttlSeconds?: number }) => { - await getSetClient.set(name, superjson.stringify(data)); if (options?.ttlSeconds) { - await getSetClient.expire(name, options.ttlSeconds); + await getSetClient.set(name, superjson.stringify(data), "EX", options.ttlSeconds); + return; } + await getSetClient.set(name, superjson.stringify(data)); }, /** * Remove data from the channel @@ -128,6 +129,30 @@ export const createGetSetChannel = (name: string) => { }; }; +/** + * Creates a short-lived distributed lock. + * + * The token prevents a process from releasing a lock that expired and was + * acquired by another process in the meantime. + */ +export const createLockChannel = (name: string) => { + return { + acquireAsync: async (ttlSeconds: number) => { + const token = createId(); + const result = await getSetClient.set(name, token, "EX", ttlSeconds, "NX"); + return result === "OK" ? token : null; + }, + releaseAsync: async (token: string) => { + await getSetClient.eval( + "if redis.call('get', KEYS[1]) == ARGV[1] then return redis.call('del', KEYS[1]) else return 0 end", + 1, + name, + token, + ); + }, + }; +}; + const queryCacheKey = (userId: string, boardId: string) => `qc:${userId}:${boardId}`; export const setQueryCacheAsync = async ( diff --git a/packages/request-handler/src/test/update-checker-cache.spec.ts b/packages/request-handler/src/test/update-checker-cache.spec.ts new file mode 100644 index 000000000..06264fb4c --- /dev/null +++ b/packages/request-handler/src/test/update-checker-cache.spec.ts @@ -0,0 +1,174 @@ +// @vitest-environment node + +import { beforeEach, describe, expect, test, vi } from "vitest"; + +const mocks = vi.hoisted(() => { + type StoredValue = { + expiresAt: number | null; + value: unknown; + }; + + const values = new Map(); + const locks = new Set(); + const listReleases = vi.fn(); + + return { + env: { NO_EXTERNAL_CONNECTION: false }, + listReleases, + values, + locks, + removeFreshResult() { + values.delete("update-checker:fresh:v1"); + }, + }; +}); + +vi.mock("@homarr/common/env", () => ({ env: mocks.env })); + +vi.mock("@homarr/redis", () => ({ + createGetSetChannel: (name: string) => ({ + getAsync: async () => { + const stored = mocks.values.get(name); + if (!stored) return null; + if (stored.expiresAt !== null && Date.now() >= stored.expiresAt) { + mocks.values.delete(name); + return null; + } + return stored.value; + }, + setAsync: async (value: unknown, options?: { ttlSeconds?: number }) => { + mocks.values.set(name, { + value, + expiresAt: options?.ttlSeconds ? Date.now() + options.ttlSeconds * 1_000 : null, + }); + }, + removeAsync: async () => { + mocks.values.delete(name); + }, + }), + createLockChannel: (name: string) => ({ + acquireAsync: async () => { + if (mocks.locks.has(name)) return null; + mocks.locks.add(name); + return "test-lock-token"; + }, + releaseAsync: async () => { + mocks.locks.delete(name); + }, + }), +})); + +vi.mock("octokit", () => ({ + Octokit: class { + rest = { + repos: { + listReleases: mocks.listReleases, + }, + }; + }, +})); + +const successfulResponse = { + data: [ + { + body_html: undefined, + html_url: "https://github.com/homarr-labs/homarr/releases/tag/v99.0.0", + name: "v99.0.0", + prerelease: false, + tag_name: "v99.0.0", + }, + ], + headers: {}, + status: 200, + url: "", +}; + +const loadHandlerAsync = async () => { + const { updateCheckerRequestHandler } = await import("../update-checker"); + return updateCheckerRequestHandler.handler({}); +}; + +describe("persisted update checker cache", () => { + beforeEach(() => { + vi.resetModules(); + vi.restoreAllMocks(); + mocks.env.NO_EXTERNAL_CONNECTION = false; + mocks.listReleases.mockReset(); + mocks.values.clear(); + mocks.locks.clear(); + }); + + test("checks GitHub once for repeated page loads inside 24 hours", async () => { + mocks.listReleases.mockResolvedValue(successfulResponse); + const handler = await loadHandlerAsync(); + + const first = await handler.getDataAsync(); + const second = await handler.getDataAsync(); + + expect(first.data).toEqual(second.data); + expect(mocks.listReleases).toHaveBeenCalledTimes(1); + }); + + test("deduplicates concurrent checks", async () => { + mocks.listReleases.mockImplementation(async () => { + await Promise.resolve(); + return successfulResponse; + }); + const handler = await loadHandlerAsync(); + + const results = await Promise.all(Array.from({ length: 10 }, async () => await handler.getDataAsync())); + + expect(results.every(({ data }) => data.availableUpdates.length === 1)).toBe(true); + expect(mocks.listReleases).toHaveBeenCalledTimes(1); + }); + + test("refreshes at the 24-hour boundary, not before it", async () => { + let now = 1_000; + vi.spyOn(Date, "now").mockImplementation(() => now); + mocks.listReleases.mockResolvedValue(successfulResponse); + const handler = await loadHandlerAsync(); + await handler.getDataAsync(); + + now += 24 * 60 * 60 * 1_000 - 1; + await handler.getDataAsync(); + expect(mocks.listReleases).toHaveBeenCalledTimes(1); + + now += 1; + await handler.getDataAsync(); + expect(mocks.listReleases).toHaveBeenCalledTimes(2); + }); + + test("reuses the Redis result after a simulated process restart", async () => { + mocks.listReleases.mockResolvedValue(successfulResponse); + const firstHandler = await loadHandlerAsync(); + await firstHandler.getDataAsync(); + + vi.resetModules(); + const restartedHandler = await loadHandlerAsync(); + await restartedHandler.getDataAsync(); + + expect(mocks.listReleases).toHaveBeenCalledTimes(1); + }); + + test("serves stale data and suppresses retries for 24 hours after a provider failure", async () => { + mocks.listReleases.mockResolvedValueOnce(successfulResponse).mockRejectedValue(new Error("GitHub unavailable")); + const handler = await loadHandlerAsync(); + const fresh = await handler.getDataAsync(); + mocks.removeFreshResult(); + + const stale = await handler.getDataAsync(); + const repeated = await handler.getDataAsync(); + + expect(stale.data).toEqual(fresh.data); + expect(repeated.data).toEqual(fresh.data); + expect(mocks.listReleases).toHaveBeenCalledTimes(2); + }); + + test("does not contact GitHub when external connections are disabled", async () => { + mocks.env.NO_EXTERNAL_CONNECTION = true; + const handler = await loadHandlerAsync(); + + await expect(handler.getDataAsync()).resolves.toMatchObject({ data: { availableUpdates: [] } }); + expect(mocks.listReleases).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/request-handler/src/update-checker.ts b/packages/request-handler/src/update-checker.ts index 1cf4dbd6f..c3f010eec 100644 --- a/packages/request-handler/src/update-checker.ts +++ b/packages/request-handler/src/update-checker.ts @@ -5,19 +5,99 @@ import { env } from "@homarr/common/env"; import { fetchWithTrustedCertificatesAsync } from "@homarr/core/infrastructure/http"; import { createLogger } from "@homarr/core/infrastructure/logs"; import { ErrorWithMetadata } from "@homarr/core/infrastructure/logs/error"; -import { createRequestHandler } from "./lib/request-handler"; +import { createGetSetChannel, createLockChannel } from "@homarr/redis"; import packageJson from "../../../package.json"; const logger = createLogger({ module: "updateCheckerRequestHandler" }); -export const updateCheckerRequestHandler = createRequestHandler({ - async requestAsync(_) { +const updateCheckTtlSeconds = 24 * 60 * 60; +const updateCheckLockTtlSeconds = 2 * 60; +const updateCheckWaitIntervalMs = 100; +const updateCheckWaitAttempts = 50; + +type UpdateCheckCacheEntry = { + availableUpdates: Update[]; + attemptedAt: number; + checkedAt: number | null; +}; + +const freshUpdateCheckChannel = createGetSetChannel("update-checker:fresh:v1"); +const staleUpdateCheckChannel = createGetSetChannel("update-checker:stale:v1"); +const updateCheckLock = createLockChannel("update-checker:lock:v1"); + +const waitAsync = async (durationMs: number) => + await new Promise((resolve) => { + setTimeout(resolve, durationMs); + }); + +const waitForConcurrentUpdateCheckAsync = async () => { + for (let attempt = 0; attempt < updateCheckWaitAttempts; attempt++) { + await waitAsync(updateCheckWaitIntervalMs); + const cached = await freshUpdateCheckChannel.getAsync(); + if (cached) return cached; + } + + return await staleUpdateCheckChannel.getAsync(); +}; + +const getCachedAvailableUpdatesAsync = async (): Promise => { + const cached = await freshUpdateCheckChannel.getAsync(); + if (cached) return cached; + + const lockToken = await updateCheckLock.acquireAsync(updateCheckLockTtlSeconds); + if (!lockToken) { + const concurrentResult = await waitForConcurrentUpdateCheckAsync(); + if (concurrentResult) return concurrentResult; + throw new Error("Timed out waiting for the update check"); + } + + try { + const attemptedAt = Date.now(); const availableUpdates = await getAvailableUpdatesAsync(packageJson.version); - return { availableUpdates }; - }, - cacheTtlMs: 30 * 60 * 1000, -}); + const result: UpdateCheckCacheEntry = { + availableUpdates, + attemptedAt, + checkedAt: attemptedAt, + }; + await staleUpdateCheckChannel.setAsync(result); + await freshUpdateCheckChannel.setAsync(result, { ttlSeconds: updateCheckTtlSeconds }); + return result; + } catch (error) { + const stale = await staleUpdateCheckChannel.getAsync(); + const result: UpdateCheckCacheEntry = { + availableUpdates: stale?.availableUpdates ?? [], + attemptedAt: Date.now(), + checkedAt: stale?.checkedAt ?? null, + }; + await freshUpdateCheckChannel.setAsync(result, { ttlSeconds: updateCheckTtlSeconds }); + logger.warn("Failed to fetch available updates; suppressing retries for 24 hours", { + error: error instanceof Error ? error.message : String(error), + servingStale: stale !== null, + }); + return result; + } finally { + try { + await updateCheckLock.releaseAsync(lockToken); + } catch (error) { + logger.warn("Failed to release the update-check lock", { + error: error instanceof Error ? error.message : String(error), + }); + } + } +}; + +export const updateCheckerRequestHandler = { + handler: (_input: Record) => ({ + getDataAsync: async () => { + const result = await getCachedAvailableUpdatesAsync(); + return { + data: { availableUpdates: result.availableUpdates }, + timestamp: new Date(result.checkedAt ?? result.attemptedAt), + }; + }, + }), +}; interface Update { name: string | null;