From 86823db34e17bf12e4f92f737b59cb29f0eec173 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 10:04:06 +0200 Subject: [PATCH] fix(backups): a landed object stays a success when the ledger write does not The per-account ledger row shared the upload's try. A pool timeout or a connection reset on that upsert landed in the same catch as a refused upload: the account was counted as failed, its reason pushed into the run's failure list, and the console would go on to read never or stale for an account whose encrypted copy was sitting in the bucket the whole time. That is the same inversion the ledger was added to remove, on the write side. The upload's boundary now ends where the object is durable. uploadEncryptedBackup resolves only after the multipart completes, so the count and the largest-object figure are taken there, and the row is written afterwards under its own boundary. A write that fails warns with the account it belongs to and adds a run-level count, which is a different fact from a failed upload and reads as one. Two arms that were correct by construction and unasserted now have tests: a failed upload writes no success row, so the previous timestamp stands, and a run whose ledger write rejects after the object landed still reports it uploaded. --- src/lib/jobs/__tests__/offhost-backup.test.ts | 37 +++++++++++- src/lib/jobs/offhost-backup.ts | 60 +++++++++++++------ 2 files changed, 79 insertions(+), 18 deletions(-) diff --git a/src/lib/jobs/__tests__/offhost-backup.test.ts b/src/lib/jobs/__tests__/offhost-backup.test.ts index 3bd97c4df..118b0f1ea 100644 --- a/src/lib/jobs/__tests__/offhost-backup.test.ts +++ b/src/lib/jobs/__tests__/offhost-backup.test.ts @@ -306,8 +306,9 @@ describe("runOffhostBackup", () => { it("counts per-user failures without aborting the whole run", async () => { const s3 = makeS3Mock(); + const upsert = vi.fn(); const prisma = { - offhostBackupState: { upsert: vi.fn() }, + offhostBackupState: { upsert }, user: { findMany: vi.fn().mockResolvedValue([{ id: "u1" }, { id: "u2" }]), }, @@ -331,6 +332,40 @@ describe("runOffhostBackup", () => { expect(report.uploaded).toBe(1); expect(report.failed).toBe(1); + // The guarantee an operator leans on: an account whose upload failed keeps + // whatever `lastSuccessAt` it already had. A failed upload writes no + // success into the ledger, so yesterday's good copy is still what the + // console reports — the run does not overwrite it with today's failure. + expect(upsert).toHaveBeenCalledTimes(1); + expect( + upsert.mock.calls.map( + (call) => (call[0] as { where: { userId: string } }).where.userId, + ), + ).toEqual(["u1"]); + }); + + it("still reports a landed object as uploaded when the ledger write fails", async () => { + const s3 = makeS3Mock(); + const prisma = { + offhostBackupState: { + upsert: vi.fn().mockRejectedValue(new Error("pool timeout")), + }, + user: { findMany: vi.fn().mockResolvedValue([{ id: "u1" }]) }, + }; + + const report = await runOffhostBackup( + prisma as never, + s3, + new Date("2026-05-08T00:00:00Z"), + ); + + // The object is in the bucket. Only the record of it is missing, and that + // is a different fact: counting it as a failure would tell the operator + // their disaster-recovery copy does not exist when it does. + expect(report.uploaded).toBe(1); + expect(report.failed).toBe(0); + expect(report.failures).toEqual([]); + expect(s3.store.has("2026-05-08/user-u1.json.enc")).toBe(true); }); it("refuses an account whose object outgrows one multipart upload", async () => { diff --git a/src/lib/jobs/offhost-backup.ts b/src/lib/jobs/offhost-backup.ts index 4f3a2cc62..863c22c65 100644 --- a/src/lib/jobs/offhost-backup.ts +++ b/src/lib/jobs/offhost-backup.ts @@ -28,7 +28,7 @@ import { createGzip, gunzipSync, gzipSync } from "node:zlib"; import type { PrismaClient } from "@/generated/prisma/client"; import { createRawStreamEncryptor, decryptRawStream } from "@/lib/crypto"; import { streamFullBackupJson } from "@/lib/export/full-backup-stream"; -import { getEvent } from "@/lib/logging/context"; +import { annotate, getEvent } from "@/lib/logging/context"; const ALGORITHM = "aes-256-gcm"; const IV_LENGTH = 12; @@ -576,10 +576,12 @@ export async function runOffhostBackup( let oversized = 0; let largestObjectBytes = 0; const failures: Array<{ userId: string; message: string }> = []; + let ledgerWriteFailures = 0; const evt = getEvent(); for (const user of users) { + let objectBytes: number | null = null; try { - const objectBytes = await uploadEncryptedBackup( + objectBytes = await uploadEncryptedBackup( s3, `${dateKey}/user-${user.id}.json.enc`, cfg.encryptionKey, @@ -594,21 +596,6 @@ export async function runOffhostBackup( }), options, ); - // The per-account ledger, written the moment the object exists. The run - // itself only ever reported counts, and a count cannot say WHICH account - // has no copy — which is the one question an operator asks about a - // backup. `new Date()` rather than the run's `now`, because on a large - // cohort the two are hours apart and the row is meant to say when this - // account's object landed. - await prisma.offhostBackupState.upsert({ - where: { userId: user.id }, - update: { lastSuccessAt: new Date(), sizeBytes: objectBytes }, - create: { - userId: user.id, - lastSuccessAt: new Date(), - sizeBytes: objectBytes, - }, - }); largestObjectBytes = Math.max(largestObjectBytes, objectBytes); uploaded++; } catch (err) { @@ -622,6 +609,45 @@ export async function runOffhostBackup( `offhost-backup user ${user.id} failed: ${message.slice(0, 200)}`, ); } + + // The per-account ledger, and deliberately NOT inside the upload's `try`. + // `uploadEncryptedBackup` resolves only after the multipart completes, so + // by here the object is durably in the bucket and this account's backup + // has succeeded whatever the database does next. A pool timeout on the + // row would otherwise un-count a copy that exists — the run would report + // one failure too many and the console would read `never` for an account + // whose disaster-recovery object is fine, which is exactly the inversion + // the ledger exists to remove. + // + // `new Date()` rather than the run's `now`, because on a large cohort the + // two are hours apart and the row is meant to say when this account's + // object landed. + if (objectBytes !== null) { + const bytes = objectBytes; + try { + await prisma.offhostBackupState.upsert({ + where: { userId: user.id }, + update: { lastSuccessAt: new Date(), sizeBytes: bytes }, + create: { + userId: user.id, + lastSuccessAt: new Date(), + sizeBytes: bytes, + }, + }); + } catch (err) { + ledgerWriteFailures++; + const message = (err as Error).message ?? "unknown"; + evt?.addWarning( + `offhost-backup ledger write failed for ${user.id}: ${message.slice(0, 200)}`, + ); + } + } + } + if (ledgerWriteFailures > 0) { + // A distinct fact from a failed upload, and one an operator has to be able + // to tell apart: the copies are in the bucket, the console's per-account + // view of them is behind. + annotate({ meta: { offhost_ledger_write_failures: ledgerWriteFailures } }); } return {