From 361e57786402e54996ea562fd896b0c1be7719b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 00:27:43 +0200 Subject: [PATCH] fix(ops): refuse an account whose rows are not the thousandfold class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The provenance criterion proves where a row came from, not which build imported it. An account re-imported on the fixed build carries the same `EXPORT_XML_SOURCE_MAX` stamp and the same `stats:` external id with the right numbers and no audit row, so it is selected exactly like a broken one — and its rest days, under 200 m because the phone stayed at home, survive the plausibility check after another thousandfold multiply. So one candidate row that would leave the range now refuses the whole account: nothing written, no audit row, the offending rows and a printed reason instead. The idempotency guard only ever held against the script's own prior run; this holds against the case it could not see. The runbook says the same in words — a re-imported account must not be repaired, the re-import already did it — and says what the dry run is for: it is where the decision is made, not a formality in front of one already taken. Integration coverage for the re-imported account: correct rows, an `--apply` run, nothing changes and no audit row is left behind. Review finding H1. --- docs/ops/apple-health-distance-repair.md | 27 +++++- scripts/repair-apple-health-distance.ts | 25 ++++-- .../repair-apple-health-distance.ts | 71 ++++++++++++++-- .../apple-health-distance-repair.test.ts | 84 ++++++++++++++++++- 4 files changed, 189 insertions(+), 18 deletions(-) diff --git a/docs/ops/apple-health-distance-repair.md b/docs/ops/apple-health-distance-repair.md index 28b5c1c37..fd24ff16a 100644 --- a/docs/ops/apple-health-distance-repair.md +++ b/docs/ops/apple-health-distance-repair.md @@ -25,6 +25,16 @@ build rewrites every affected day with the right number, needs no flags, and also repairs rows this script deliberately leaves alone (see the criterion below). The script exists for accounts that no longer have the archive. +**An account that has been re-imported must not then be repaired.** The +re-import already fixed it. Its rows keep the same `EXPORT_XML_SOURCE_MAX` +stamp and the same `stats:` external id, and they carry no audit row, so the +criterion selects them exactly as it selects a broken account — the stamp +proves where a row came from, not which build imported it. Multiplying them +again turns a 2.484 km day into 2484 km. The script refuses such an account on +its own (any day whose value would leave the plausible range after the +multiply stops the whole account), but that guard fires on the big days: keep +the two apart yourself before you type `--apply`. + ## Which rows it touches Only rows carrying the stamp the archive importer itself writes: @@ -51,7 +61,13 @@ Deliberately out of scope: ## Run a dry-run first -Always. The default mode reports and never writes: +Always, and read it. The dry run is where the decision is made, not a +formality in front of a decision already taken: it is the only place the +script shows you real numbers from the account before it changes them. If the +worked example does not match what that person's Health app shows for that day, +the run is wrong and `--apply` will make it permanent. + +The default mode reports and never writes: ```bash # from a source checkout, with DATABASE_URL pointing at the instance @@ -94,9 +110,12 @@ clients pick the rows up on their next delta sync, and an audit row and the row count. The DAY/WEEK/MONTH/YEAR rollups and the cached status insights for the touched days are recomputed afterwards. -A row whose repaired value would leave the plausible range (0–200 000 m) is -reported and never written — several of those in one account mean the rows are -not the 1000x class and the run should be stopped. +A row whose repaired value would leave the plausible range (0–200 000 m) +refuses the **whole account**: nothing is written, the offending rows and the +reason are printed, and the run moves on to the next account. One such row is +evidence the account is not the 1000x class — a re-imported account looks +exactly like this — and repairing the rest of it would multiply days that were +already right. ## It runs once per account diff --git a/scripts/repair-apple-health-distance.ts b/scripts/repair-apple-health-distance.ts index 2abf0f45e..a7ef1fe5a 100644 --- a/scripts/repair-apple-health-distance.ts +++ b/scripts/repair-apple-health-distance.ts @@ -24,8 +24,10 @@ * - `--apply` writes, one transaction per account, and records an audit row * in that same transaction. An account carrying that row is skipped, so a * second run finds nothing. - * - A row whose repaired value would leave the plausibility range is - * reported and never written. + * - A row whose repaired value would leave the plausibility range refuses + * the WHOLE account: one such row is evidence the rows are not the 1000x + * class (an account re-imported on the fixed build looks exactly like + * this), and its rest days would otherwise be multiplied again. * - `--unit=mi` for an imperial archive (default `km`). The stored row keeps * no record of the archive's unit — that is the defect — so the operator * names it; a mismatched unit is why the flag exists rather than a guess. @@ -76,6 +78,7 @@ async function main(): Promise { let totalRepairable = 0; let totalUpdated = 0; let totalSkipped = 0; + let refused = 0; for (const plan of plans) { if (plan.alreadyRepairedAt) { @@ -107,7 +110,12 @@ async function main(): Promise { for (const row of plan.outOfRange) { console.log( ` ! ${row.id} (${row.measuredAt.toISOString().slice(0, 10)}): ` + - `${row.value} m x ${factor} leaves the plausible range — not written`, + `${row.value} m x ${factor} leaves the plausible range`, + ); + } + if (plan.outOfRange.length > 0) { + console.log( + " ! this account will be REFUSED — see the reason under --apply", ); } @@ -117,6 +125,11 @@ async function main(): Promise { }); totalUpdated += outcome.updated; totalSkipped += outcome.skipped; + if (outcome.refusedReason) { + refused += 1; + console.log(` ✗ refused: ${outcome.refusedReason}`); + continue; + } console.log( ` ✓ repaired ${outcome.updated} row(s), skipped ${outcome.skipped}`, ); @@ -124,9 +137,11 @@ async function main(): Promise { console.log( apply - ? `\nDone. ${totalUpdated} row(s) repaired, ${totalSkipped} skipped.` + ? `\nDone. ${totalUpdated} row(s) repaired, ${totalSkipped} skipped, ` + + `${refused} account(s) refused.` : `\nDry run: ${totalRepairable} row(s) would be repaired. ` + - "Re-run with --apply to write.", + "Read the worked examples above and decide before re-running with " + + "--apply.", ); } diff --git a/src/lib/measurements/repair-apple-health-distance.ts b/src/lib/measurements/repair-apple-health-distance.ts index 7257fa475..13d760cb7 100644 --- a/src/lib/measurements/repair-apple-health-distance.ts +++ b/src/lib/measurements/repair-apple-health-distance.ts @@ -35,6 +35,16 @@ * archive on the fixed build repairs those, and is the authoritative * repair for any account that still has its `export.zip`. * + * ── The refusal ──────────────────────────────────────────────────── + * + * The criterion proves an archive ORIGIN. It cannot prove the archive was + * imported on the broken build — an account re-imported since carries the + * same stamp with the right numbers, and its rest days still pass the + * plausibility check after another x1000. So any candidate whose repaired + * value would leave the range refuses the whole account: nothing is + * written, the rows are reported, and the operator decides. Re-importing + * the archive is the authoritative repair and needs no script. + * * ── Idempotency ──────────────────────────────────────────────────── * * Applying the repair writes one `AuditLog` row per account inside the @@ -81,8 +91,8 @@ export interface AccountRepairPlan { repairable: RepairCandidateRow[]; /** * Candidates whose repaired value would leave the plausibility range - * (`validateMeasurementRange`) — reported, never written. A row here is - * a sign the account's rows are NOT the 1000x class. + * (`validateMeasurementRange`). One row here refuses the whole account: + * it is evidence the rows are NOT the 1000x class. */ outOfRange: RepairCandidateRow[]; /** Non-null when this account already carries the repair's audit row. */ @@ -178,6 +188,35 @@ export interface RepairOutcome { userId: string; updated: number; skipped: number; + /** + * Why the account was left alone, when it was. Null on a run that wrote. + * The script prints it verbatim. + */ + refusedReason: string | null; + /** The out-of-range candidates — what a refusal was decided on. */ + skippedRows: RepairCandidateRow[]; +} + +/** + * The refusal an out-of-range candidate earns. + * + * The provenance criterion proves an archive origin; it cannot prove the + * archive was imported on the BROKEN build. An account that has since been + * re-imported on the fixed build carries the same stamp with the right + * numbers, and its rest days — under 200 m, phone left at home — pass the + * plausibility check after another x1000 and would be written. So a single + * row that would leave the range is treated as what it is: evidence the + * account is not the 1000x class. The whole account stops. + */ +function outOfRangeRefusal(plan: AccountRepairPlan, factor: number): string { + return ( + `${plan.outOfRange.length} of ${plan.candidateCount} candidate row(s) ` + + `leave the plausible range after x${factor}, so these rows are not the ` + + "1000x class — the account was most likely re-imported on the fixed " + + "build, or its archive was never in the affected unit. Nothing written. " + + "Check the dry run's worked examples against the account's Health app " + + "before forcing anything." + ); } /** @@ -192,8 +231,24 @@ export async function applyAppleHealthDistanceRepair( ): Promise { const factor = repairFactorFor(options.archiveUnit); const skipped = plan.outOfRange.length; - if (plan.alreadyRepairedAt || plan.repairable.length === 0) { - return { userId: plan.userId, updated: 0, skipped }; + const base = { + userId: plan.userId, + updated: 0, + skipped, + skippedRows: plan.outOfRange, + }; + if (plan.alreadyRepairedAt) { + return { + ...base, + refusedReason: `already repaired on ${plan.alreadyRepairedAt.toISOString()}`, + }; + } + // Refuse the ACCOUNT, not just the row: see `outOfRangeRefusal`. + if (plan.outOfRange.length > 0) { + return { ...base, refusedReason: outOfRangeRefusal(plan, factor) }; + } + if (plan.repairable.length === 0) { + return { ...base, refusedReason: "no repairable rows" }; } const ids = plan.repairable.map((row) => row.id); @@ -239,7 +294,13 @@ export async function applyAppleHealthDistanceRepair( "repair-apple-health-distance", ); - return { userId: plan.userId, updated: ids.length, skipped }; + return { + userId: plan.userId, + updated: ids.length, + skipped, + refusedReason: null, + skippedRows: plan.outOfRange, + }; } /** Narrowing helper for the script's `--unit` flag. */ diff --git a/tests/integration/apple-health-distance-repair.test.ts b/tests/integration/apple-health-distance-repair.test.ts index d4fa7bbea..b3ead4daf 100644 --- a/tests/integration/apple-health-distance-repair.test.ts +++ b/tests/integration/apple-health-distance-repair.test.ts @@ -187,25 +187,101 @@ describe("Apple Health export distance repair — integration", () => { expect((await readRow(prisma, rowId)).syncVersion).toBe(2); }); - it("uses the archive's own unit and never writes an implausible value", async () => { + it("uses the archive's own unit", async () => { const prisma = getPrismaClient(); const userId = await makeUser(prisma, "mi"); const mile = await seedRow(prisma, userId, { value: 1.543 }); - const absurd = await seedRow(prisma, userId, { value: 250 }); const plans = await planAppleHealthDistanceRepair(prisma, { archiveUnit: "mi", }); expect(plans[0].repairable.map((row) => row.id)).toEqual([mile]); + + const outcome = await applyAppleHealthDistanceRepair(prisma, plans[0], { + archiveUnit: "mi", + }); + await settleBackgroundTasks(); + expect(outcome).toMatchObject({ updated: 1, skipped: 0 }); + expect((await readRow(prisma, mile)).value).toBeCloseTo(2483.217792, 6); + }); + + it("refuses the whole account when one row would leave the range", async () => { + const prisma = getPrismaClient(); + const userId = await makeUser(prisma, "mixed"); + const plausible = await seedRow(prisma, userId, { value: 1.543 }); + const absurd = await seedRow(prisma, userId, { value: 250 }); + + const plans = await planAppleHealthDistanceRepair(prisma, { + archiveUnit: "mi", + }); + expect(plans[0].repairable.map((row) => row.id)).toEqual([plausible]); expect(plans[0].outOfRange.map((row) => row.id)).toEqual([absurd]); const outcome = await applyAppleHealthDistanceRepair(prisma, plans[0], { archiveUnit: "mi", }); await settleBackgroundTasks(); - expect(outcome).toMatchObject({ updated: 1, skipped: 1 }); - expect((await readRow(prisma, mile)).value).toBeCloseTo(2483.217792, 6); + expect(outcome.updated).toBe(0); + expect(outcome.skipped).toBe(1); + expect(outcome.skippedRows.map((row) => row.id)).toEqual([absurd]); + expect(outcome.refusedReason).toMatch(/leave the plausible range/); + + // Nothing written at all — not the "repairable" row, not the audit row + // that would make a later, correct run a no-op. + expect((await readRow(prisma, plausible)).value).toBe(1.543); + expect((await readRow(prisma, plausible)).syncVersion).toBe(1); expect((await readRow(prisma, absurd)).value).toBe(250); + expect( + await prisma.auditLog.count({ + where: { action: APPLE_HEALTH_DISTANCE_REPAIR_ACTION, userId }, + }), + ).toBe(0); + }); + + it("leaves an account re-imported on the fixed build untouched", async () => { + // The scenario the criterion cannot see: the operator re-imported + // `export.zip` on the fixed build, so the rows are already right. They + // still carry `EXPORT_XML_SOURCE_MAX` + the `stats:` external id and + // still have no audit row, so they are selected exactly like a broken + // account's rows. The rest day survives another x1000; the ordinary days + // do not, and that is what has to stop the account. + const prisma = getPrismaClient(); + const userId = await makeUser(prisma, "reimported"); + const ordinary = await seedRow(prisma, userId, { value: 2484 }); + const longWalk = await seedRow(prisma, userId, { value: 7020 }); + const restDay = await seedRow(prisma, userId, { value: 150 }); + + const plans = await planAppleHealthDistanceRepair(prisma, { + archiveUnit: "km", + }); + expect(plans).toHaveLength(1); + // The rest day would pass the plausibility check on its own. + expect(plans[0].repairable.map((row) => row.id)).toEqual([restDay]); + expect(plans[0].outOfRange.map((row) => row.id).sort()).toEqual( + [ordinary, longWalk].sort(), + ); + + const outcome = await applyAppleHealthDistanceRepair(prisma, plans[0], { + archiveUnit: "km", + }); + await settleBackgroundTasks(); + expect(outcome.updated).toBe(0); + expect(outcome.refusedReason).not.toBeNull(); + + for (const [id, value] of [ + [ordinary, 2484], + [longWalk, 7020], + [restDay, 150], + ] as const) { + const row = await readRow(prisma, id); + expect(row.value).toBe(value); + expect(row.syncVersion).toBe(1); + } + expect( + await prisma.auditLog.count({ + where: { action: APPLE_HEALTH_DISTANCE_REPAIR_ACTION, userId }, + }), + ).toBe(0); }); it("plans one entry per account", async () => {