From a7faae8ded7e11ebd71aeedf1f0349ba767ea5e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 10:32:33 +0200 Subject: [PATCH] refactor(admin): read the off-host card from evidence, sort the worst row first Four smaller things around the same card. The configured flag came from the environment of whichever process serves the request. A hand-rolled split that gives BACKUP_S3_* to the worker and not to the web process would have shown an operator "nothing leaves this host" over a ledger full of fresh rows. The ledger is queried either way now, and a row in it outranks an environment read: a row is proof an upload happened. Rows sorted by username put the one account that needs attention wherever the alphabet leaves it, which on a hundred-account cohort is the crowd problem the card exists to end, moved from a count into a list. Worst verdict first, alphabetical within a verdict, decided server-side so the tests and the accessibility fixture see the order the operator does. The card's docblock claimed the worker holds PutObject and GetObject. The runbook is right and the multipart path needs AbortMultipartUpload too, so the prose no longer documents a grant that would break the uploader. And the byte formatter moved to lib. The off-host card was importing it out of a thousand-line client component, which costs nothing while both cards render on one route and becomes a cost the moment either moves. --- .../api/admin/backups/__tests__/route.test.ts | 56 +++++++++++---- src/app/api/admin/backups/route.ts | 68 +++++++++++++------ src/components/admin/backups-section.tsx | 16 +---- .../admin/offhost-backups-section.tsx | 10 +-- src/lib/format/bytes.ts | 22 ++++++ src/types/backups.ts | 2 +- 6 files changed, 120 insertions(+), 54 deletions(-) create mode 100644 src/lib/format/bytes.ts diff --git a/src/app/api/admin/backups/__tests__/route.test.ts b/src/app/api/admin/backups/__tests__/route.test.ts index e182e8e17..70b583451 100644 --- a/src/app/api/admin/backups/__tests__/route.test.ts +++ b/src/app/api/admin/backups/__tests__/route.test.ts @@ -8,7 +8,7 @@ * * The bucket is never touched here, and that is the assertion that matters — * the verdicts come from the ledger the worker writes, so an operator whose - * worker holds PutObject and nothing else still gets a true answer. + * worker holds no listing grant on the bucket still gets a true answer. */ import { beforeEach, describe, expect, it, vi } from "vitest"; @@ -107,28 +107,35 @@ describe("GET /api/admin/backups — off-host freshness", () => { expect(body.offhost.configured).toBe(true); expect(body.offhost.periodHours).toBe(24); + // Worst first, alphabetical within a verdict. The database hands them + // back by username; the one account that needs attention must not be + // wherever the alphabet leaves it on a hundred-account cohort. expect( body.offhost.rows.map((row) => [row.username, row.freshness]), ).toEqual([ - ["account-one", "fresh"], - ["account-two", "due"], ["account-three", "stale"], + ["account-two", "due"], ["account-four", "never"], ["account-five", "unknown"], + ["account-one", "fresh"], ]); - expect(body.offhost.rows[0]?.sizeBytes).toBe(4096); - expect(typeof body.offhost.rows[0]?.sizeBytes).toBe("number"); - expect(body.offhost.rows[0]?.ageHours).toBe(3); - expect(body.offhost.rows[3]?.lastSuccessAt).toBeNull(); - expect(body.offhost.rows[3]?.sizeBytes).toBeNull(); - expect(body.offhost.rows[3]?.lastAttemptAt).not.toBeNull(); + const byName = new Map(body.offhost.rows.map((row) => [row.username, row])); + expect(byName.get("account-one")?.sizeBytes).toBe(4096); + expect(typeof byName.get("account-one")?.sizeBytes).toBe("number"); + expect(byName.get("account-one")?.ageHours).toBe(3); + expect(byName.get("account-four")?.lastSuccessAt).toBeNull(); + expect(byName.get("account-four")?.sizeBytes).toBeNull(); + expect(byName.get("account-four")?.lastAttemptAt).not.toBeNull(); // `unknown` is the honest one: the ledger has no history for this // account, which is not the same claim as "the bucket is empty". - expect(body.offhost.rows[4]?.lastAttemptAt).toBeNull(); + expect(byName.get("account-five")?.lastAttemptAt).toBeNull(); }); - it("says so and reads no account when off-host backup is not configured", async () => { + it("says so when neither the environment nor the ledger knows of any upload", async () => { configuredMock.mockReturnValue(false); + findManyMock.mockResolvedValue([ + { id: "u1", username: "account-one", offhostBackupState: null }, + ]); const body = await read(); @@ -137,9 +144,30 @@ describe("GET /api/admin/backups — off-host freshness", () => { periodHours: 24, rows: [], }); - // Not merely empty output — the query never runs, so an unconfigured host - // does not pay for a table scan to be told nothing is set up. - expect(findManyMock).not.toHaveBeenCalled(); + }); + + it("lets a ledger row outrank this process's own environment", async () => { + // A hand-rolled split deployment can give BACKUP_S3_* to the worker and + // not to the web process. The rows are proof uploads are happening; + // saying "nothing leaves this host" over them would be this process + // reporting on a job it cannot see. + configuredMock.mockReturnValue(false); + findManyMock.mockResolvedValue([ + { + id: "u1", + username: "account-one", + offhostBackupState: { + lastAttemptAt: new Date(), + lastSuccessAt: new Date(), + sizeBytes: BigInt(4096), + }, + }, + ]); + + const body = await read(); + + expect(body.offhost.configured).toBe(true); + expect(body.offhost.rows.map((row) => row.freshness)).toEqual(["fresh"]); }); it("never ships a credential or an object key with the verdicts", async () => { diff --git a/src/app/api/admin/backups/route.ts b/src/app/api/admin/backups/route.ts index b8f38b14e..3aab2ff45 100644 --- a/src/app/api/admin/backups/route.ts +++ b/src/app/api/admin/backups/route.ts @@ -18,7 +18,7 @@ * host last put that account's encrypted copy in the operator's bucket and how * stale that is against the nightly schedule. It comes from the ledger the * worker writes, never from a bucket listing — the answer has to hold on a host - * whose worker grant is PutObject and nothing else. + * whose worker holds no listing grant on the bucket. */ import { prisma } from "@/lib/db"; import { apiHandler, requireAdmin } from "@/lib/api-handler"; @@ -31,6 +31,7 @@ import { offhostBackupConfigured } from "@/lib/jobs/offhost-backup"; import { classifyOffhostBackup, OFFHOST_BACKUP_PERIOD_HOURS, + type OffhostBackupFreshness, } from "@/lib/jobs/offhost-backup-freshness"; // v1.4.41 W-ORG — `BackupRow` / `BackupsList` moved to `src/types/backups.ts` // so callers (in particular `components/admin/backups-section.tsx`) don't @@ -84,27 +85,35 @@ export const GET = apiHandler(async () => { // The off-host leg, per account, read from the ledger the nightly worker // writes rather than from the bucket. The page must be able to answer "which - // account has no recent copy off-host" on a host whose worker grant is - // PutObject and nothing else, and a listing call from a page render would - // put the operator's credentials on the request path of every page view. + // account has no recent copy off-host" on a host whose worker holds the + // three grants the runbook documents and no listing grant at all, and a + // listing call from a page render would put the operator's credentials on + // the request path of every page view. const now = new Date(); - const configured = offhostBackupConfigured(); - const offhostAccounts = configured - ? await prisma.user.findMany({ + const offhostAccounts = await prisma.user.findMany({ + select: { + id: true, + username: true, + offhostBackupState: { select: { - id: true, - username: true, - offhostBackupState: { - select: { - lastAttemptAt: true, - lastSuccessAt: true, - sizeBytes: true, - }, - }, + lastAttemptAt: true, + lastSuccessAt: true, + sizeBytes: true, }, - orderBy: { username: "asc" }, - }) - : []; + }, + }, + orderBy: { username: "asc" }, + }); + + // Evidence beats this process's environment. `offhostBackupConfigured()` + // reads the variables of whichever process serves this request, and a + // hand-rolled split that gives `BACKUP_S3_*` only to the worker would make + // the web process say "nothing leaves this host" over a ledger full of + // fresh rows. A row in that table is proof an upload happened; no + // environment read can outrank it. + const configured = + offhostBackupConfigured() || + offhostAccounts.some((account) => account.offhostBackupState !== null); const offhostRows: OffhostAccountRow[] = offhostAccounts.map((account) => { const state = account.offhostBackupState; @@ -130,6 +139,23 @@ export const GET = apiHandler(async () => { }; }); + // Worst first. Sorting by username puts the one account that needs + // attention wherever the alphabet happens to leave it, which on a + // hundred-account cohort is the same "one bad row hides in the crowd" + // problem this card was built to end, moved out of a count and into a list. + const FRESHNESS_ORDER: Record = { + stale: 0, + due: 1, + never: 2, + unknown: 3, + fresh: 4, + }; + offhostRows.sort( + (a, b) => + FRESHNESS_ORDER[a.freshness] - FRESHNESS_ORDER[b.freshness] || + a.username.localeCompare(b.username), + ); + const payload: BackupsList = { rows: list, // Matches the retention window the backup-prune job enforces so the @@ -145,7 +171,9 @@ export const GET = apiHandler(async () => { offhost: { configured, periodHours: OFFHOST_BACKUP_PERIOD_HOURS, - rows: offhostRows, + // An unconfigured host has nothing to list, and the card says so rather + // than painting an empty table that reads as "no problems". + rows: configured ? offhostRows : [], }, }; diff --git a/src/components/admin/backups-section.tsx b/src/components/admin/backups-section.tsx index d776da699..fb065f101 100644 --- a/src/components/admin/backups-section.tsx +++ b/src/components/admin/backups-section.tsx @@ -47,6 +47,7 @@ import { SettingsCard } from "@/components/settings/settings-card"; import { SettingsCardHeader } from "@/components/settings/_card-header"; import { ListRow } from "@/components/ui/list-row"; import { useFormatters, useTranslations } from "@/lib/i18n/context"; +import { formatBytes } from "@/lib/format/bytes"; import { queryKeys } from "@/lib/query-keys"; import { randomId } from "@/lib/random-id"; import type { BackupRow, BackupsList } from "@/types/backups"; @@ -322,21 +323,6 @@ function RestoreRowDialog({ ); } -/** - * Exported for the off-host card, which reads the same sizes out of the same - * response and must not spell "1.25 MB" a second way two cards apart. - */ -export function formatBytes( - bytes: number, - fmt: ReturnType, -) { - if (bytes < 1024) return `${bytes} B`; - if (bytes < 1024 * 1024) { - return `${fmt.integer(Math.round(bytes / 1024))} KB`; - } - return `${fmt.number(bytes / 1024 / 1024, 2)} MB`; -} - /** * Render the `DataBackup.type` enum (`WEEKLY_AUTO` / `MANUAL`) as a * human label. Unknown values fall through to the raw enum so a future diff --git a/src/components/admin/offhost-backups-section.tsx b/src/components/admin/offhost-backups-section.tsx index cf3de6837..88e8cda76 100644 --- a/src/components/admin/offhost-backups-section.tsx +++ b/src/components/admin/offhost-backups-section.tsx @@ -11,9 +11,11 @@ * been refusing the worker's signature since March. * * Every verdict is computed server-side from the ledger the worker writes. The - * page never lists the bucket: the worker's grant is deliberately PutObject + - * GetObject, and putting a credentialed listing call on the render path of an - * admin page would be a poor trade for a number the database already holds. + * page never lists the bucket: the worker's grant is deliberately PutObject, + * GetObject and AbortMultipartUpload — the three the runbook documents, the + * abort being what the multipart path needs — and putting a credentialed + * listing call on the render path of an admin page would be a poor trade for + * a number the database already holds. */ import { useQuery } from "@tanstack/react-query"; @@ -31,7 +33,7 @@ import { useFormatters, useTranslations } from "@/lib/i18n/context"; import { queryKeys } from "@/lib/query-keys"; import type { OffhostBackupFreshness } from "@/lib/jobs/offhost-backup-freshness"; import type { BackupsList, OffhostAccountRow } from "@/types/backups"; -import { formatBytes } from "./backups-section"; +import { formatBytes } from "@/lib/format/bytes"; /** * Status wash per verdict. `never` and `unknown` are deliberately neutral diff --git a/src/lib/format/bytes.ts b/src/lib/format/bytes.ts new file mode 100644 index 000000000..c59732ce2 --- /dev/null +++ b/src/lib/format/bytes.ts @@ -0,0 +1,22 @@ +import type { useFormatters } from "@/lib/i18n/context"; + +/** + * One spelling of "1.25 MB" for every surface that shows a byte count from + * the admin backups response. + * + * It lives here rather than beside the card that happened to need it first: + * the second reader was importing a formatter out of a thousand-line client + * component, which is a dependency on a page's worth of code for four lines of + * arithmetic. Both cards render on the same route today, so it costs nothing + * today — and it becomes a cost the moment either one moves. + */ +export function formatBytes( + bytes: number, + fmt: ReturnType, +): string { + if (bytes < 1024) return `${bytes} B`; + if (bytes < 1024 * 1024) { + return `${fmt.integer(Math.round(bytes / 1024))} KB`; + } + return `${fmt.number(bytes / 1024 / 1024, 2)} MB`; +} diff --git a/src/types/backups.ts b/src/types/backups.ts index 0cd2a20ad..635b65c27 100644 --- a/src/types/backups.ts +++ b/src/types/backups.ts @@ -49,7 +49,7 @@ export interface OffhostAccountRow { * * Computed from the ledger the nightly worker writes, never from the bucket: * the page must be able to answer "does this account have a recent copy - * off-host" on a host whose worker holds PutObject and nothing else. + * off-host" on a host whose worker holds no listing grant on the bucket. */ export interface OffhostBackupOverview { /** Whether this host has the variables the nightly job needs. */