diff --git a/CHANGELOG.md b/CHANGELOG.md index a15813119..9b7ce6e44 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,44 @@ ## [Unreleased] +## [1.32.1] — 2026-07-23 + +- **The Personal Health Score reads "not enough data yet" instead of zero.** + An account with records that do not yet meet any score pillar's minimum + (for example a single mood entry, no paired blood pressure, one weight + reading) showed a red 0 rather than an honest absence. The score is now + withheld until at least one pillar is computable; a genuinely computed 0 is + still shown. +- **The Coach stops refusing ordinary questions about your own scores.** A + question about a Sleep Score or Health Score no longer trips the + fabricated-clinical-risk refusal; the guard now matches the specific risk + calculator it was meant to catch, not the plain word "score", and still + blocks a genuinely invented risk figure. +- **The Apple Health import no longer stalls at "unpacking".** A large export + is now unpacked as a stream rather than decompressed whole into memory on + the shared event loop, and a job left mid-unpack by a restart is picked up + by a periodic reconcile instead of sitting stuck with no error. A byte + ceiling still guards against a decompression bomb. +- **The Pulse insights page shows pulse, not resting heart rate.** It no + longer silently swaps its main chart to resting heart rate when any resting + data exists; resting heart rate appears as a clearly labelled second series + when present, and the resting-calibrated target band no longer paints + behind raw pulse readings. +- **Dashboard layout changes can no longer be reverted by a concurrent + score-ring save.** The instant score-ring toggle now sends only the ring + fields instead of resending a cached snapshot of the whole layout, closing + a race where an in-flight ring update could land after a tile or chart Save + and silently restore the older layout. +- **Measurement-reminder cadence edits recompute against the cadence that + was actually saved.** Clearing an interval or a recurrence rule to switch a + reminder's schedule no longer leaves the next-due date computed from the + just-replaced value for one cycle. +- **ntfy notification settings keep a saved auth token across unrelated + edits.** Toggling the channel, or editing the server URL or topic, no + longer silently clears a previously configured token. + +No migrations. No breaking changes. + ## [1.32.0] — 2026-07-22 - **Provider ingestion and daily reactions now preserve the correct identity.** diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index fd80bfd26..7b9a6f3b2 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -1,7 +1,7 @@ openapi: 3.1.0 info: title: HealthLog API - version: 1.32.0 + version: 1.32.1 description: >- Self-hosted personal-health-tracking PWA — public API surface for the iOS native client and external ingest. diff --git a/package.json b/package.json index ddd249a8f..487cf7c9e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "healthlog", - "version": "1.32.0", + "version": "1.32.1", "description": "Self-hosted personal-health-tracking PWA with Withings integration, AI insights, and doctor-report PDF export.", "license": "PolyForm-Noncommercial-1.0.0", "homepage": "https://healthlog.dev", diff --git a/public/sw.js b/public/sw.js index 476db13b9..97e178033 100644 --- a/public/sw.js +++ b/public/sw.js @@ -36,7 +36,7 @@ try { // v1.4.38.4 → v1.4.42. Do not hand-edit; bump `package.json` and rebuild. const CACHE_VERSION = (typeof self !== "undefined" && self.__APP_VERSION__) || - /* @sw-version-fallback */ "v1.32.0"; + /* @sw-version-fallback */ "v1.32.1"; const STATIC_CACHE = `healthlog-static-${CACHE_VERSION}`; const PAGE_CACHE = `healthlog-pages-${CACHE_VERSION}`; // v1.18.6 — read-only data cache for a curated allowlist of safe GET `/api/*` diff --git a/src/app/api/dashboard/widgets/__tests__/route.test.ts b/src/app/api/dashboard/widgets/__tests__/route.test.ts index 5611f3fc3..258346380 100644 --- a/src/app/api/dashboard/widgets/__tests__/route.test.ts +++ b/src/app/api/dashboard/widgets/__tests__/route.test.ts @@ -62,7 +62,13 @@ import { DASHBOARD_WIDGET_IDS, DASHBOARD_IOS_ONLY_WIDGET_IDS, DASHBOARD_WIDGET_CATALOGUE_IDS, + DEFAULT_DASHBOARD_LAYOUT, serializeDashboardLayout, + // v1.32.1 — the exact payload builder `ringMutation.mutationFn` calls + // (see the settings component). Importing the shipped function rather + // than re-implementing its shape means this test exercises the REAL + // client contract against the REAL route, not a hand-rolled stand-in. + buildRingMutationPayload, type DashboardLayout, } from "@/lib/dashboard-layout"; @@ -636,3 +642,102 @@ describe("dashboard widgets — preserve-when-absent on PUT", () => { } }); }); + +/** + * v1.32.1 — regression for issue #581: dashboard layout changes silently + * overwritten by a concurrent score-ring save. + * + * The scenario from the report: the user edits tile/chart visibility + * (local draft B), then reorders/toggles a hero score ring before hitting + * Save. The ring PUT (`ringMutation`) fires immediately, built from + * whatever layout the client had cached (layout A — the state BEFORE the + * draft edit). The normal Save button then PUTs the full draft (layout B) + * and commits first. If the earlier-fired ring request resolves AFTER + * Save, its body — built from the stale A snapshot — used to carry + * `widgets: A` explicitly, and an explicitly-present field always wins on + * write. Save's B silently reverted to A even though the ring PUT reported + * 200 and only meant to touch the ring selection. + * + * The fix is `buildRingMutationPayload` (imported from the real component + * module, not re-implemented here): it never includes `widgets` at all. + * This test proves the SERVER half of the fix — a PUT shaped exactly like + * the shipped ring mutation's body preserves whatever layout is CURRENTLY + * stored, so the request that resolves last can no longer matter. + */ +describe("dashboard widgets — ring-only PUT cannot race a concurrent full-layout Save (regression #581)", () => { + it("preserves the widgets a concurrent Save already committed, even though the ring request started from a stale snapshot", async () => { + // The state AFTER the normal Save committed layout B — a tile turned + // off that was on in the stale snapshot A the ring mutation started + // from. Starts from the FULL default widget catalogue (rather than a + // single-widget array) so `resolveDashboardLayout`'s auto-upgrade + // append (missing catalogue ids get seeded invisible) is a no-op here + // and the persisted array can be compared for exact equality below. + const savedLayoutB: DashboardLayout = serializeDashboardLayout({ + version: 1, + widgets: DEFAULT_DASHBOARD_LAYOUT.widgets.map((w) => + w.id === "weight" ? { ...w, visible: false, tileVisible: false } : w, + ), + comparisonBaseline: "lastYear", + selectedScoreRings: ["MED_COMPLIANCE"], + heroRingOrder: ["HEALTH_SCORE", "MED_COMPLIANCE"], + }); + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + dashboardWidgetsJson: savedLayoutB, + } as never); + vi.mocked(prisma.user.update).mockResolvedValue({} as never); + + // The ring mutation's real, shipped payload — built without any + // knowledge of layout B. If this were still `{...staleA, ...}` the + // stale `widgets`/`comparisonBaseline` would land here explicitly and + // overwrite B; `buildRingMutationPayload` never carries them. + const ringOnlyBody = buildRingMutationPayload({ + selectedScoreRings: ["READINESS"], + heroRingOrder: ["HEALTH_SCORE", "READINESS"], + }); + + const res = await callPut(makeReq(ringOnlyBody)); + expect(res.status).toBe(200); + + const updateArg = vi.mocked(prisma.user.update).mock + .calls[0]?.[0] as unknown as { + data: { dashboardWidgetsJson: DashboardLayout }; + }; + // B's widgets (and comparisonBaseline) survive — the ring PUT never + // touched them, so nothing it carries can outrace the Save that + // already committed. + expect(updateArg.data.dashboardWidgetsJson.widgets).toEqual( + savedLayoutB.widgets, + ); + expect(updateArg.data.dashboardWidgetsJson.comparisonBaseline).toBe( + "lastYear", + ); + // The ring selection itself DID apply. + expect(updateArg.data.dashboardWidgetsJson.selectedScoreRings).toEqual([ + "READINESS", + ]); + + const body = (await res.json()) as { data: DashboardLayout }; + expect(body.data.widgets[0].visible).toBe(false); + expect(body.data.widgets[0].tileVisible).toBe(false); + expect(body.data.selectedScoreRings).toEqual(["READINESS"]); + }); + + it("still accepts a normal full-layout Save (widgets present + replace semantics unchanged)", async () => { + vi.mocked(prisma.user.findUnique).mockResolvedValue({ + dashboardWidgetsJson: null, + } as never); + vi.mocked(prisma.user.update).mockResolvedValue({} as never); + + const res = await callPut( + makeReq({ + version: 1, + widgets: [ + { id: "weight", visible: false, tileVisible: false, order: 0 }, + ], + }), + ); + expect(res.status).toBe(200); + const body = (await res.json()) as { data: DashboardLayout }; + expect(body.data.widgets[0].visible).toBe(false); + }); +}); diff --git a/src/app/api/dashboard/widgets/route.ts b/src/app/api/dashboard/widgets/route.ts index 790501f5e..d7c8c97f6 100644 --- a/src/app/api/dashboard/widgets/route.ts +++ b/src/app/api/dashboard/widgets/route.ts @@ -84,7 +84,15 @@ const layoutSchema = z.object({ // catalogue (42 ids). The cap only has to clear the catalogue size — // the enum bounds each entry and the id set is closed — so it carries // headroom rather than tracking the count exactly. - .max(50), + .max(50) + // v1.32.1 — optional + preserve-when-absent (issue #581). The + // Settings page's instant score-ring toggle now PUTs ONLY the ring + // fields; omitting `widgets` lets the server carry forward whatever + // is CURRENTLY stored instead of the ring mutation resending a + // client-held snapshot that can go stale mid-flight and race a + // concurrent full-layout Save. A normal Save still always sends + // `widgets` — this only widens what a PARTIAL PUT can leave out. + .optional(), // v1.4.16 phase B8 — comparison baseline (Vormonat / Vorjahr) rides // on the layout blob per research §7 Q3 (no Prisma migration). Optional // so v1.4.15 clients that don't know the field can still PUT. @@ -285,7 +293,11 @@ export const PUT = apiHandler(async (request: NextRequest) => { // visibility / order, while chart prefs are PUT through their own route // (`/api/dashboard/chart-overlay-prefs`), the hero rings come from // Settings, and `comparisonBaseline` is web-only — an omission from any - // one surface must not reset a choice made on another. + // one surface must not reset a choice made on another. `widgets` itself + // joined this set in v1.32.1 (issue #581): the Settings page's instant + // score-ring PUT omits it entirely so it always carries forward + // whatever is CURRENTLY stored, rather than a client-held snapshot that + // could be stale relative to a concurrent full-layout Save. // // The preserve set is not a list maintained here: it is derived from // `LAYOUT_FIELD_MERGE_DISPOSITION`, which the type system forces to cover diff --git a/src/app/api/measurement-reminders/[id]/__tests__/route.test.ts b/src/app/api/measurement-reminders/[id]/__tests__/route.test.ts new file mode 100644 index 000000000..1a9ac856f --- /dev/null +++ b/src/app/api/measurement-reminders/[id]/__tests__/route.test.ts @@ -0,0 +1,229 @@ +/** + * v1.32.1 — regression for iOS coordination issue #62: the `nextDueAt` + * recomputation on `PATCH /api/measurement-reminders/{id}` used to read an + * explicit `null` cadence clear as "field omitted" (`?? existing.field` + * cannot distinguish the two — both are nullish) and recompute against the + * STALE cadence for one cycle, even though the persisted row itself already + * carried the correct cleared value. + * + * Every test below drives the REAL `computeReminderNextDueAt` (not a mock) + * so the "correct" and "stale" comparison values are genuine recurrence- + * engine output, not hand-picked dates — and asserts the fixtures actually + * differ before trusting the route's answer, so a fixture collision can't + * hide a regression. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { NextRequest } from "next/server"; + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: unknown>(fn: T) => fn, + requireAuth: vi.fn(async () => ({ user: { id: "u1", locale: "en" } })), +})); + +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn() })); + +const findFirstMock = vi.fn(); +const updateMock = vi.fn(); +const findUserMock = vi.fn(); +const auditLogCreateMock = vi.fn(); +vi.mock("@/lib/db", () => ({ + prisma: { + measurementReminder: { + findFirst: (...a: unknown[]) => findFirstMock(...a), + update: (...a: unknown[]) => updateMock(...a), + }, + user: { findUnique: (...a: unknown[]) => findUserMock(...a) }, + auditLog: { create: (...a: unknown[]) => auditLogCreateMock(...a) }, + }, +})); + +import { PATCH } from "../route"; +import { computeReminderNextDueAt } from "@/lib/measurement-reminders/scheduling"; + +const BASE_ROW = { + id: "r1", + userId: "u1", + label: "Blutdruck messen", + measurementType: "BLOOD_PRESSURE_SYS", + intervalDays: 30, + rrule: null as string | null, + anchorDate: null as Date | null, + endsOn: null, + origin: "VORSORGE", + notifyHour: 9, + location: null, + nextDueAt: new Date("2026-01-31T09:00:00Z"), + lastSatisfiedAt: null as Date | null, + enabled: true, + createdAt: new Date("2026-01-01T00:00:00Z"), + updatedAt: new Date("2026-01-01T00:00:00Z"), + deletedAt: null, +}; + +const NOW = new Date("2026-06-15T08:00:00.000Z"); +const TZ = "Europe/Berlin"; + +function makeRequest(body: unknown): NextRequest { + return new NextRequest("http://localhost/api/measurement-reminders/r1", { + method: "PATCH", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +const params = { params: Promise.resolve({ id: "r1" }) }; + +beforeEach(() => { + vi.clearAllMocks(); + findUserMock.mockResolvedValue({ timezone: TZ }); + updateMock.mockImplementation( + async ({ data }: { data: Record }) => ({ + ...BASE_ROW, + ...data, + }), + ); + vi.useFakeTimers(); + vi.setSystemTime(NOW); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +describe("PATCH /api/measurement-reminders/[id] — nextDueAt honours explicit-null cadence clears (regression iOS #62)", () => { + it("interval → RRULE: recomputes off the NEW rrule, not the just-cleared interval", async () => { + // Existing rolling reminder: intervalDays=30, rrule=null. + findFirstMock.mockResolvedValue(BASE_ROW); + + const res = await PATCH( + makeRequest({ intervalDays: null, rrule: "FREQ=YEARLY" }), + params, + ); + expect(res.status).toBe(200); + + const persisted = updateMock.mock.calls[0]?.[0]?.data as Record< + string, + unknown + >; + // The explicit-null clear + the new rrule both persist correctly — + // this half was never broken. + expect(persisted.intervalDays).toBeNull(); + expect(persisted.rrule).toBe("FREQ=YEARLY"); + + const correct = computeReminderNextDueAt( + { ...BASE_ROW, intervalDays: null, rrule: "FREQ=YEARLY" }, + TZ, + NOW, + ); + // The bug: `?? existing` reads the cleared intervalDays as "omitted" + // and recomputes against the OLD rolling cadence for one cycle. + const staleBug = computeReminderNextDueAt( + { ...BASE_ROW, intervalDays: 30, rrule: null }, + TZ, + NOW, + ); + // Mutation-check guard: the fixtures must actually diverge, or this + // test can't tell a fix from a no-op. + expect(correct).not.toEqual(staleBug); + + expect(persisted.nextDueAt).toEqual(correct); + expect(persisted.nextDueAt).not.toEqual(staleBug); + }); + + it("RRULE → interval: recomputes off the NEW interval, not the just-cleared rrule (incl. its BYHOUR)", async () => { + findFirstMock.mockResolvedValue({ + ...BASE_ROW, + intervalDays: null, + rrule: "FREQ=DAILY;BYHOUR=7,19", + }); + + const res = await PATCH( + makeRequest({ rrule: null, intervalDays: 14 }), + params, + ); + expect(res.status).toBe(200); + + const persisted = updateMock.mock.calls[0]?.[0]?.data as Record< + string, + unknown + >; + expect(persisted.rrule).toBeNull(); + expect(persisted.intervalDays).toBe(14); + + const correct = computeReminderNextDueAt( + { ...BASE_ROW, intervalDays: 14, rrule: null }, + TZ, + NOW, + ); + const staleBug = computeReminderNextDueAt( + { ...BASE_ROW, intervalDays: null, rrule: "FREQ=DAILY;BYHOUR=7,19" }, + TZ, + NOW, + ); + expect(correct).not.toEqual(staleBug); + + expect(persisted.nextDueAt).toEqual(correct); + expect(persisted.nextDueAt).not.toEqual(staleBug); + }); + + it("explicit anchorDate: null clear recomputes off no-anchor, not the stale anchor", async () => { + findFirstMock.mockResolvedValue({ + ...BASE_ROW, + intervalDays: 30, + anchorDate: new Date("2026-01-15T00:00:00Z"), + }); + + const res = await PATCH(makeRequest({ anchorDate: null }), params); + expect(res.status).toBe(200); + + const persisted = updateMock.mock.calls[0]?.[0]?.data as Record< + string, + unknown + >; + expect(persisted.anchorDate).toBeNull(); + + const correct = computeReminderNextDueAt( + { ...BASE_ROW, intervalDays: 30, anchorDate: null }, + TZ, + NOW, + ); + const staleBug = computeReminderNextDueAt( + { + ...BASE_ROW, + intervalDays: 30, + anchorDate: new Date("2026-01-15T00:00:00Z"), + }, + TZ, + NOW, + ); + expect(correct).not.toEqual(staleBug); + + expect(persisted.nextDueAt).toEqual(correct); + expect(persisted.nextDueAt).not.toEqual(staleBug); + }); + + it("omitted cadence fields on a label-only edit leave the cadence — and nextDueAt — unchanged", async () => { + findFirstMock.mockResolvedValue(BASE_ROW); + + const res = await PATCH(makeRequest({ label: "New label" }), params); + expect(res.status).toBe(200); + + const persisted = updateMock.mock.calls[0]?.[0]?.data as Record< + string, + unknown + >; + expect(persisted.label).toBe("New label"); + // The cadence keys are never written at all on a label-only edit — no + // clear, no touch. + expect("intervalDays" in persisted).toBe(false); + expect("rrule" in persisted).toBe(false); + expect("anchorDate" in persisted).toBe(false); + + const expected = computeReminderNextDueAt(BASE_ROW, TZ, NOW); + expect(persisted.nextDueAt).toEqual(expected); + }); +}); diff --git a/src/app/api/measurement-reminders/[id]/route.ts b/src/app/api/measurement-reminders/[id]/route.ts index b163194a6..aad5e46e0 100644 --- a/src/app/api/measurement-reminders/[id]/route.ts +++ b/src/app/api/measurement-reminders/[id]/route.ts @@ -133,18 +133,35 @@ export const PATCH = apiHandler( // Recompute next-due against the merged cadence. Floor the search at // the last-satisfied instant (or now) so a cadence edit re-anchors // cleanly off the user's last fulfilment. + // + // v1.32.1 — `Object.hasOwn(updateData, field)` replaces a `?? existing` + // fallback here (issue #62). `??` cannot tell an explicit `null` clear + // apart from an omitted field — both are nullish — so a PATCH that + // cleared `intervalDays` to switch a reminder onto an RRULE still + // recomputed `nextDueAt` against the OLD rolling interval for one + // cycle (rolling cadence wins the dispatcher's precedence order, so the + // stale interval silently overrode the just-set RRULE). The same gap + // hit the RRULE → interval direction and an explicit `anchorDate: null` + // clear. `updateData` already encodes the field-by-field truth of what + // is ACTUALLY being persisted — including the mutual-exclusivity + // auto-clear a few lines up — so keying off its own keys (present, even + // when the value is `null`) can never diverge from what the database + // ends up holding. const timezone = await resolveTimezone(user.id); const now = new Date(); const merged: ReminderScheduleInput = { - intervalDays: - (updateData.intervalDays as number | null | undefined) ?? - existing.intervalDays, - rrule: (updateData.rrule as string | null | undefined) ?? existing.rrule, - anchorDate: - (updateData.anchorDate as Date | null | undefined) ?? - existing.anchorDate, - notifyHour: - (updateData.notifyHour as number | undefined) ?? existing.notifyHour, + intervalDays: Object.hasOwn(updateData, "intervalDays") + ? (updateData.intervalDays as number | null) + : existing.intervalDays, + rrule: Object.hasOwn(updateData, "rrule") + ? (updateData.rrule as string | null) + : existing.rrule, + anchorDate: Object.hasOwn(updateData, "anchorDate") + ? (updateData.anchorDate as Date | null) + : existing.anchorDate, + notifyHour: Object.hasOwn(updateData, "notifyHour") + ? (updateData.notifyHour as number) + : existing.notifyHour, lastSatisfiedAt: existing.lastSatisfiedAt, createdAt: existing.createdAt, // v1.18.1 — preserve the course window so an edit of a finite diff --git a/src/app/api/measurements/batch/__tests__/hr-bucket-observability.test.ts b/src/app/api/measurements/batch/__tests__/hr-bucket-observability.test.ts new file mode 100644 index 000000000..35300709e --- /dev/null +++ b/src/app/api/measurements/batch/__tests__/hr-bucket-observability.test.ts @@ -0,0 +1,269 @@ +/** + * v1.32.1 (issue #585) — wide-event observability for the go-forward + * aggregated 10-min heart-rate bucket wire contract. + * + * The intraday pulse chart reads exactly the `stats:HKQuantityTypeIdentifier + * HeartRate:<10-min-Z>` rows this batch route accepts (or rejects). A report + * of "the chart looks sparser after the ZIP-import cutoff" was previously + * undiagnosable from server logs: the baseline `measurement.batch.ingest` + * annotation carried only a total `skipped` count with no reason breakdown + * and no visibility into how many of a batch's entries specifically targeted + * an HR bucket. These tests pin the new `measurement.batch.hr-bucket` + * annotation (fires only when the batch actually carried an HR-bucket entry) + * and the `skipped_by_reason` breakdown on the baseline annotation. + */ +import { describe, expect, it, vi, beforeEach } from "vitest"; +import { NextRequest } from "next/server"; + +const mocks = vi.hoisted(() => ({ annotate: vi.fn() })); + +vi.mock("@/lib/db", () => ({ + prisma: { + user: { + update: vi.fn(), + }, + measurement: { + findMany: vi.fn(), + createManyAndReturn: vi.fn(), + updateMany: vi.fn(), + }, + $transaction: vi.fn(async (fn: unknown) => { + if (typeof fn === "function") { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (fn as any)(prisma as unknown as { measurement: unknown }); + } + }), + }, +})); + +vi.mock("@/lib/logging/context", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, annotate: mocks.annotate }; +}); + +vi.mock("@/lib/measurements/reconcile-external-measurement", () => ({ + reconcileExternalMeasurement: async ( + tx: { + measurement: { + findMany: (args: unknown) => Promise< + Array<{ + id?: string; + type?: string; + source?: string; + externalId?: string | null; + }> + >; + createManyAndReturn: (args: { + data: Record; + }) => Promise>>; + updateMany: (args: { + where: Record; + data: Record; + }) => Promise; + }; + }, + input: Record & { + userId: string; + type: string; + source: string; + externalId: string; + }, + options: { exactExternalMatch?: "update" | "duplicate" } = {}, + ) => { + const existing = await tx.measurement.findMany({}); + const exact = existing.find( + (row: { type?: string; source?: string; externalId?: string | null }) => + row.type === input.type && + row.source === input.source && + row.externalId === input.externalId, + ); + if (exact && options.exactExternalMatch !== "update") { + return { status: "duplicate", row: exact }; + } + if (exact) { + await tx.measurement.updateMany({ + where: { id: exact.id }, + data: { ...input, deletedAt: null }, + }); + return { status: "updated", row: { ...exact, ...input } }; + } + const [inserted] = await tx.measurement.createManyAndReturn({ + data: input, + }); + if (inserted) return { status: "inserted", row: inserted }; + if (options.exactExternalMatch === "update") { + await tx.measurement.updateMany({ where: {}, data: input }); + return { status: "updated", row: input }; + } + return { status: "duplicate" }; + }, +})); + +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn() })); +vi.mock("@/lib/jobs/pr-detection", () => ({ + enqueuePrDetection: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/jobs/reminder-satisfy", () => ({ + enqueueReminderSatisfy: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/cache/invalidate", () => ({ + invalidateUserMeasurements: vi.fn(), +})); +vi.mock("@/lib/insights/comprehensive-generate", () => ({ + invalidateStatusInsightsForTypes: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/rollups/measurement-rollups", () => ({ + recomputeBucketsForMeasurement: vi.fn().mockResolvedValue(undefined), + collapseToTypeDayKeys: vi.fn(() => []), +})); + +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { POST } from "../route"; +import { prisma } from "@/lib/db"; +import { getSession } from "@/lib/auth/session"; +import { checkRateLimit } from "@/lib/rate-limit"; + +const SESSION_OK = { + session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, + user: { id: "user-1", username: "testuser", role: "USER" as const }, +}; + +const FRESH_BUCKET_ID = + "stats:HKQuantityTypeIdentifierHeartRate:2026-06-21T14:00:00.000Z"; +const REPOST_BUCKET_ID = + "stats:HKQuantityTypeIdentifierHeartRate:2026-06-21T15:00:00.000Z"; +const MALFORMED_BUCKET_ID = + "stats:HKQuantityTypeIdentifierHeartRate:2026-06-21T14:35:00.000Z"; +const SAMPLE_ID = "C0FFEE00-0000-4000-8000-000000000000"; + +function makeRequest(body: unknown): NextRequest { + return new NextRequest("http://localhost/api/measurements/batch", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +function hrEntry(externalId: string, value: number) { + return { + hkIdentifier: "HKQuantityTypeIdentifierHeartRate", + value, + unit: "count/min", + startDate: "2026-06-21T14:00:00.000Z", + endDate: "2026-06-21T14:59:59.000Z", + externalId, + }; +} + +function annotateCallsFor(actionName: string) { + return mocks.annotate.mock.calls.filter( + ([arg]) => + (arg as { action?: { name?: string } })?.action?.name === actionName, + ); +} + +beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: true, + remaining: 60, + resetAt: Date.now() + 60_000, + }); + vi.mocked(prisma.measurement.createManyAndReturn).mockImplementation((async ( + args: unknown, + ) => { + const { data } = args as { + data: + | { type: unknown; source: string; externalId: string | null } + | Array<{ type: unknown; source: string; externalId: string | null }>; + }; + const rows = Array.isArray(data) ? data : [data]; + return rows.map(({ type, source, externalId }) => ({ + type, + source, + externalId, + })); + }) as never); + vi.mocked(prisma.measurement.updateMany).mockResolvedValue({ count: 1 }); +}); + +describe("POST /api/measurements/batch — HR-bucket observability (issue #585)", () => { + it("reports a per-outcome breakdown covering only HR-bucket entries", async () => { + // REPOST_BUCKET_ID already exists — the reconcile mock overwrites it. + vi.mocked(prisma.measurement.findMany).mockResolvedValue([ + { + type: "PULSE", + source: "APPLE_HEALTH", + externalId: REPOST_BUCKET_ID, + }, + ] as never); + + const res = await POST( + makeRequest({ + entries: [ + hrEntry(FRESH_BUCKET_ID, 72), // inserted + hrEntry(REPOST_BUCKET_ID, 78), // updated (overwrite) + hrEntry(MALFORMED_BUCKET_ID, 70), // skipped (malformed) + hrEntry(SAMPLE_ID, 64), // NOT an HR bucket — must be excluded + ], + }), + ); + expect(res.status).toBe(200); + + const calls = annotateCallsFor("measurement.batch.hr-bucket"); + expect(calls).toHaveLength(1); + const meta = (calls[0][0] as { meta: Record }).meta; + expect(meta).toEqual({ + attempted: 3, + inserted: 1, + updated: 1, + duplicate: 0, + skipped: 1, + failed: 0, + processed: 4, + }); + }); + + it("never fires the HR-bucket annotation for a batch with no bucket entries", async () => { + vi.mocked(prisma.measurement.findMany).mockResolvedValue([]); + const res = await POST(makeRequest({ entries: [hrEntry(SAMPLE_ID, 64)] })); + expect(res.status).toBe(200); + expect(annotateCallsFor("measurement.batch.hr-bucket")).toHaveLength(0); + }); + + it("breaks the baseline ingest annotation's skip count down by reason", async () => { + vi.mocked(prisma.measurement.findMany).mockResolvedValue([]); + const res = await POST( + makeRequest({ + entries: [ + hrEntry(MALFORMED_BUCKET_ID, 70), // skipped: malformed_hr_bucket_id + ], + }), + ); + expect(res.status).toBe(200); + + const calls = annotateCallsFor("measurement.batch.ingest"); + expect(calls).toHaveLength(1); + const meta = calls[0][0] as { meta: { skipped_by_reason: unknown } }; + expect(meta.meta.skipped_by_reason).toEqual({ + malformed_hr_bucket_id: 1, + }); + }); +}); diff --git a/src/app/api/measurements/batch/route.ts b/src/app/api/measurements/batch/route.ts index b739ed6b4..a48cf1c66 100644 --- a/src/app/api/measurements/batch/route.ts +++ b/src/app/api/measurements/batch/route.ts @@ -603,6 +603,18 @@ async function postBatch(request: NextRequest): Promise { .filter((r) => r.status === "skipped") .map((r) => ({ index: r.index, reason: r.reason ?? "unknown" })); + // v1.32.1 (issue #585) — per-reason skip breakdown. The baseline + // annotation below only ever carried the total `skipped` count, so an + // operator investigating "why does this account's uploaded data look + // sparser than expected" had no way to see WHICH validation gate an + // entry tripped (malformed bucket id, out-of-range value, unmappable + // identifier, …) without pulling raw rows. Grouping by reason turns + // that into a one-glance wide-event field. + const skippedByReason: Record = {}; + for (const s of skipped) { + skippedByReason[s.reason] = (skippedByReason[s.reason] ?? 0) + 1; + } + await auditLog("measurement.batch.ingest", { userId: user.id, ipAddress: getClientIp(request), @@ -648,6 +660,64 @@ async function postBatch(request: NextRequest): Promise { }); } + // v1.32.1 (issue #585) — dedicated outcome breakdown for entries + // targeting the go-forward aggregated 10-min heart-rate bucket prefix + // (`stats:HKQuantityTypeIdentifierHeartRate:<10-min-Z>`). The intraday + // pulse chart reads exactly these rows for a live-synced day; a report + // of "the chart looks sparser after the ZIP import cutoff" is + // unanswerable from the baseline ingest trace alone, since it never + // distinguished an HR-bucket write from any other entry in the batch. + // Grepping `measurement.batch.hr-bucket` now shows, per sync, how many + // bucket entries the client sent and what happened to each — confirming + // (or ruling out) server-side rejection/dedup as the density-gap cause. + // Only fires when the batch actually carried at least one HR-bucket + // entry so the baseline trace is unchanged for every other sync. + let hrBucketInserted = 0; + let hrBucketUpdated = 0; + let hrBucketDuplicate = 0; + let hrBucketSkipped = 0; + let hrBucketFailed = 0; + for (let index = 0; index < entries.length; index++) { + if (!targetsAggregatedBucket(entries[index].externalId)) continue; + switch (results[index]?.status) { + case "inserted": + hrBucketInserted++; + break; + case "updated": + hrBucketUpdated++; + break; + case "duplicate": + hrBucketDuplicate++; + break; + case "skipped": + hrBucketSkipped++; + break; + case "failed": + hrBucketFailed++; + break; + } + } + const hrBucketAttempted = + hrBucketInserted + + hrBucketUpdated + + hrBucketDuplicate + + hrBucketSkipped + + hrBucketFailed; + if (hrBucketAttempted > 0) { + annotate({ + action: { name: "measurement.batch.hr-bucket" }, + meta: { + attempted: hrBucketAttempted, + inserted: hrBucketInserted, + updated: hrBucketUpdated, + duplicate: hrBucketDuplicate, + skipped: hrBucketSkipped, + failed: hrBucketFailed, + processed: entries.length, + }, + }); + } + // v1.4.25 W16c — kick off PR detection for this user. We always // enqueue when at least one row was written (or the batch had any // measurements to consider) so a single off-day reading still gets @@ -693,6 +763,9 @@ async function postBatch(request: NextRequest): Promise { updated: updatedCount, duplicates: duplicateCount, skipped: skipped.length, + // v1.32.1 (issue #585) — per-reason skip breakdown; see the + // `skippedByReason` build above. + skipped_by_reason: skippedByReason, failed: failedCount, }, }); diff --git a/src/app/api/settings/__tests__/channel-enabled-routes.test.ts b/src/app/api/settings/__tests__/channel-enabled-routes.test.ts index c5cc73548..c09dc31e9 100644 --- a/src/app/api/settings/__tests__/channel-enabled-routes.test.ts +++ b/src/app/api/settings/__tests__/channel-enabled-routes.test.ts @@ -298,3 +298,121 @@ describe("notification channel enable-only routes", () => { }, ); }); + +/** + * v1.32.1 — regression for iOS coordination issue #63: `PUT + * /api/settings/ntfy` reconstructed the entire stored config from the + * request body. `GET` never returns the write-only `authToken` (only + * `hasAuthToken: true/false`), so both native and web clients correctly + * OMIT — or round-trip an empty string through — the field on any + * unrelated edit. Rebuilding `config` without a preserve fallback + * silently dropped a working token on the next save. + * + * The fix mirrors the pre-existing `headerValue` preserve-on-empty + * contract in `src/app/api/settings/webhook/route.ts` (asserted here too, + * as a parity pin — that route was never broken, but any future drift + * between the two should fail the same suite). Covers exactly the four + * cases the report asks for. + */ +describe("PUT /api/settings/ntfy — write-only authToken preserve-on-omit contract", () => { + function upsertConfig() { + const call = vi.mocked(prisma.notificationChannel.upsert).mock + .calls[0]?.[0] as { + create: { config: string }; + update: { config: string }; + }; + // The route always upserts — assert both arms agree, then decode one. + expect(call.update.config).toBe(call.create.config); + return JSON.parse(call.update.config.replace(/^encrypted:/, "")) as { + serverUrl: string; + topic: string; + authToken?: string; + }; + } + + it("an omitted authToken preserves the stored secret", async () => { + vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue({ + config: + 'encrypted:{"serverUrl":"https://ntfy.sh","topic":"saved-topic","authToken":"secret"}', + } as never); + + const response = await put("ntfy", { + serverUrl: "https://ntfy.sh", + topic: "saved-topic", + enabled: true, + }); + + expect(response.status).toBe(200); + expect(upsertConfig().authToken).toBe("secret"); + }); + + it("an explicit empty-string authToken preserves the stored secret (matches current clients)", async () => { + vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue({ + config: + 'encrypted:{"serverUrl":"https://ntfy.sh","topic":"saved-topic","authToken":"secret"}', + } as never); + + const response = await put("ntfy", { + serverUrl: "https://ntfy.sh", + topic: "saved-topic", + authToken: "", + enabled: true, + }); + + expect(response.status).toBe(200); + expect(upsertConfig().authToken).toBe("secret"); + }); + + it("a non-empty authToken replaces the stored secret", async () => { + vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue({ + config: + 'encrypted:{"serverUrl":"https://ntfy.sh","topic":"saved-topic","authToken":"old-secret"}', + } as never); + + const response = await put("ntfy", { + serverUrl: "https://ntfy.sh", + topic: "saved-topic", + authToken: "new-secret", + enabled: true, + }); + + expect(response.status).toBe(200); + expect(upsertConfig().authToken).toBe("new-secret"); + }); + + it("creating without a token remains valid where anonymous ntfy is allowed", async () => { + vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue(null); + + const response = await put("ntfy", { + serverUrl: "https://ntfy.sh", + topic: "public-topic", + enabled: false, + }); + + expect(response.status).toBe(200); + const config = upsertConfig(); + expect(config.authToken).toBeUndefined(); + expect(config.topic).toBe("public-topic"); + }); + + it("parity pin: the webhook headerValue preserve-on-empty contract this fix mirrors is intact", async () => { + vi.mocked(prisma.notificationChannel.findUnique).mockResolvedValue({ + config: + 'encrypted:{"url":"https://saved.example/hook","headerName":"Authorization","headerValue":"secret"}', + } as never); + + const response = await put("webhook", { + url: "https://saved.example/hook", + headerName: "Authorization", + enabled: true, + }); + + expect(response.status).toBe(200); + const call = vi.mocked(prisma.notificationChannel.upsert).mock + .calls[0]?.[0] as { update: { config: string } }; + const config = JSON.parse( + call.update.config.replace(/^encrypted:/, ""), + ) as { headerValue?: string }; + expect(config.headerValue).toBe("secret"); + }); +}); diff --git a/src/app/api/settings/ntfy/route.ts b/src/app/api/settings/ntfy/route.ts index 0d916384f..f9dc4f043 100644 --- a/src/app/api/settings/ntfy/route.ts +++ b/src/app/api/settings/ntfy/route.ts @@ -113,10 +113,31 @@ export const PUT = apiHandler(async (request: NextRequest) => { ); } + // v1.32.1 — preserve an existing auth token when the client sends an + // empty one (issue #63). GET never returns the secret (`hasAuthToken` + // only), so both native and web clients correctly omit — or round-trip + // an empty string through — the field on any unrelated edit (e.g. + // toggling `enabled`). Reconstructing `config` from scratch without this + // fallback silently dropped the token on every such save. Mirrors the + // identical `headerValue` preserve-on-empty contract in + // `src/app/api/settings/webhook/route.ts`. A non-empty value replaces it. + let nextAuthToken = authToken || undefined; + if (!nextAuthToken) { + const existing = await prisma.notificationChannel.findUnique({ + where: { userId_type: { userId: user.id, type: "NTFY" } }, + }); + if (existing) { + const prev = JSON.parse(decrypt(existing.config)) as { + authToken?: string; + }; + nextAuthToken = prev.authToken || undefined; + } + } + const config = JSON.stringify({ serverUrl: serverUrl || "https://ntfy.sh", topic: topic || "", - ...(authToken ? { authToken } : {}), + ...(nextAuthToken ? { authToken: nextAuthToken } : {}), }); const encryptedConfig = encrypt(config); diff --git a/src/app/insights/pulse/__tests__/page.test.tsx b/src/app/insights/pulse/__tests__/page.test.tsx new file mode 100644 index 000000000..13e2432ff --- /dev/null +++ b/src/app/insights/pulse/__tests__/page.test.tsx @@ -0,0 +1,144 @@ +import { describe, expect, it, vi, beforeEach } from "vitest"; +import { renderToStaticMarkup } from "react-dom/server"; + +/** + * v1.32.1 (issue #584) — `/insights/pulse` must chart the metric its own + * title, capture action, and stat strip claim. `hasRestingHr` used to swap + * the ENTIRE primary chart + Coach read strip to RESTING_HEART_RATE the + * moment any resting-HR row existed, so the page could show a resting-only + * series while every other surface (title, stat strip, capture action, + * "show all values") stayed labelled "Pulse" — a silent full swap. + * RESTING_HEART_RATE may now appear only as a second, clearly-labelled + * series alongside PULSE (the same multi-series pattern the blood-pressure + * page uses for systolic/diastolic) — never a swap. + * + * `SubPageShell` is stubbed to a probe recording its props (including the + * `children` array holding the chart element) so these assert the resolved + * chart/coach-strip props rather than rendered text — responsive classes + * have broken text-based queries in this repo. + */ + +const shellProps = vi.fn(); +vi.mock("@/components/insights/sub-page-shell", () => ({ + SubPageShell: (props: Record) => { + shellProps(props); + return null; + }, +})); + +vi.mock("@/lib/i18n/context", () => ({ + useTranslations: () => ({ + t: (key: string) => key, + }), +})); + +vi.mock("@/hooks/use-auth", () => ({ + useAuth: () => ({ + isAuthenticated: true, + user: { dateOfBirth: null, gender: null, timezone: "UTC" }, + }), +})); + +const analytics = { + current: { summaries: {} as Record }, +}; +vi.mock("@/hooks/use-insights-analytics", () => ({ + useInsightsAnalytics: () => ({ + data: analytics.current, + isLoading: false, + isEmpty: false, + error: null, + refetch: () => {}, + }), +})); + +// `useInsightsLayoutPrefs` is the one real hook left that reaches +// `@tanstack/react-query`; stub `useQuery` so the page renders without a +// `QueryClientProvider` ancestor (mirrors the coach-page-launch-params test). +vi.mock("@tanstack/react-query", () => ({ + useQuery: () => ({ data: undefined, isLoading: false, error: null }), +})); + +import InsightsPulsPage from "../page"; + +interface CapturedShellProps { + captureType?: string; + showAllValuesType?: string; + coachReadStrip?: { props?: Record }; + children?: unknown; +} + +function renderPage( + summaries: Record, +): CapturedShellProps { + analytics.current = { summaries }; + renderToStaticMarkup(); + const calls = shellProps.mock.calls; + return calls[calls.length - 1][0] as CapturedShellProps; +} + +/** Locate the primary `` element among the page's + * children by its distinctive `types` array prop. Never rendered — the + * `SubPageShell` stub does not paint `children` — so this reads the plain + * React element object the page constructed. */ +function findChartElement( + children: unknown, +): { props: { types?: string[] } } | undefined { + const arr = Array.isArray(children) ? children : [children]; + return arr.find( + (el): el is { props: { types?: string[] } } => + Boolean(el) && + typeof el === "object" && + el !== null && + "props" in el && + Array.isArray((el as { props?: { types?: unknown } }).props?.types), + ); +} + +describe("/insights/pulse page — chart identity (issue #584)", () => { + beforeEach(() => { + shellProps.mockClear(); + }); + + it("charts PULSE as the primary (first) series even when resting-HR data exists", () => { + const props = renderPage({ + PULSE: { count: 50 }, + RESTING_HEART_RATE: { count: 30 }, + }); + const chart = findChartElement(props.children); + expect(chart).toBeDefined(); + expect(chart!.props.types).toEqual(["PULSE", "RESTING_HEART_RATE"]); + }); + + it("never swaps the chart to a RESTING_HEART_RATE-only series", () => { + const props = renderPage({ + PULSE: { count: 50 }, + RESTING_HEART_RATE: { count: 30 }, + }); + const chart = findChartElement(props.children); + expect(chart!.props.types).not.toEqual(["RESTING_HEART_RATE"]); + }); + + it("charts PULSE alone when there is no resting-HR data", () => { + const props = renderPage({ PULSE: { count: 50 } }); + const chart = findChartElement(props.children); + expect(chart!.props.types).toEqual(["PULSE"]); + }); + + it("keeps the Coach read strip pinned to PULSE regardless of resting-HR data", () => { + const props = renderPage({ + PULSE: { count: 50 }, + RESTING_HEART_RATE: { count: 30 }, + }); + expect(props.coachReadStrip?.props?.metricType).toBe("PULSE"); + }); + + it("keeps captureType and showAllValuesType pinned to PULSE", () => { + const props = renderPage({ + PULSE: { count: 50 }, + RESTING_HEART_RATE: { count: 30 }, + }); + expect(props.captureType).toBe("PULSE"); + expect(props.showAllValuesType).toBe("PULSE"); + }); +}); diff --git a/src/app/insights/pulse/page.tsx b/src/app/insights/pulse/page.tsx index 0b1b1d313..1f63ab298 100644 --- a/src/app/insights/pulse/page.tsx +++ b/src/app/insights/pulse/page.tsx @@ -35,20 +35,23 @@ const IntradayPulseChart = dynamic( })), { ssr: false, loading: () => }, ); -import { - getAgeFromDateOfBirth, - getPersonalizedPulseTarget, -} from "@/lib/analytics/pulse-targets"; /** * v1.4.25 W4 — `/insights/pulse`. * - * Routed Pulse sub-page. Renders the pulse chart with the personalized - * Karvonen-derived target band plus the per-section AI assessment. - * Note: `chartKey="pulse"` so the chart-cog can override the - * comparison-overlay independently from the dashboard pulse card; the - * MeasurementType filter is `PULSE` (the same field used elsewhere - * in the codebase). + * Routed Pulse sub-page. Renders the raw/workout-inclusive pulse chart + * plus the per-section AI assessment. Note: `chartKey="pulse"` so the + * chart-cog can override the comparison-overlay independently from the + * dashboard pulse card; the MeasurementType filter is `PULSE` (the same + * field used elsewhere in the codebase). + * + * v1.32.1 (issue #584) — this page no longer paints the personalized + * resting-pulse target band (`getPersonalizedPulseTarget()`, CDC/NCHS + * resting-pulse percentiles) behind the chart: that band is calibrated + * to steady-state resting readings and would misreport an expected + * workout spike in the raw PULSE series as "out of target". The + * personal target + its band belong on the dedicated + * `/insights/resting-pulse` page, against the clean resting series. * * Analytics fetch + empty-state branch consume `useInsightsAnalytics()` * + ``. VO₂ max is a cardio-fitness metric, so the page @@ -66,11 +69,16 @@ export default function InsightsPulsPage() { // dedicated cardio-fitness page. const hasVo2 = (analytics?.summaries?.VO2_MAX?.count ?? 0) > 0; const pulseSummary = analytics?.summaries?.PULSE ?? null; - // v1.15.12 A2 — the resting-pulse band is judged against - // RESTING_HEART_RATE (Apple's clean daily resting figure). When the - // user has resting rows the chart shows that series against the band; - // otherwise it charts raw heart rate WITHOUT the resting-band overlay - // (which would flag expected-high workout HR as "outside target"). + // v1.32.1 (issue #584) — this page is titled, captured, and stat-stripped + // as PULSE throughout; the chart must chart PULSE too. `hasRestingHr` used + // to swap the ENTIRE primary chart + Coach read to RESTING_HEART_RATE + // whenever any resting row existed, which silently showed a different + // metric than the page's own title/stat-strip/capture action claimed. + // RESTING_HEART_RATE now only ever appears as a second, clearly-labelled + // series alongside PULSE (same multi-series pattern the blood-pressure + // page uses for systolic/diastolic) — never a full swap. The dedicated + // `/insights/resting-pulse` page owns the resting-only view + its target + // band. const hasRestingHr = (analytics?.summaries?.RESTING_HEART_RATE?.count ?? 0) > 0; @@ -108,44 +116,6 @@ export default function InsightsPulsPage() { ); } - const pulseAge = getAgeFromDateOfBirth(user?.dateOfBirth ?? null); - const pulseTarget = getPersonalizedPulseTarget( - pulseAge, - (user?.gender as "MALE" | "FEMALE" | null | undefined) ?? null, - ); - const pulseBands = [ - { - min: 30, - max: pulseTarget.orangeMin, - color: "var(--destructive)", - opacity: 0.16, - }, - { - min: pulseTarget.orangeMin, - max: pulseTarget.greenMin, - color: "var(--warning)", - opacity: 0.18, - }, - { - min: pulseTarget.greenMin, - max: pulseTarget.greenMax, - color: "var(--success)", - opacity: 0.2, - }, - { - min: pulseTarget.greenMax, - max: pulseTarget.orangeMax, - color: "var(--warning)", - opacity: 0.18, - }, - { - min: pulseTarget.orangeMax, - max: 220, - color: "var(--destructive)", - opacity: 0.16, - }, - ].filter((band) => band.max > band.min); - return ( } coachReadStrip={ - + } diversityNudge={ — health-score anchor + reorder (v1.27.27)" // The order is the single source of truth: selection is derived from it. expect(src).toMatch(/id !== HEALTH_SCORE_RING_ID/); }); + + it("the ring mutation builds its body via buildRingMutationPayload, not a `...remote` spread (regression #581 source pin)", () => { + const src = readFileSync( + join( + process.cwd(), + "src/components/settings/dashboard-layout-section.tsx", + ), + "utf8", + ); + expect(src).toContain("buildRingMutationPayload(next)"); + // The pre-fix shape must never come back: spreading a query-cache + // snapshot into the ring PUT body is exactly the stale-`widgets` + // race issue #581 reported. + expect(src).not.toMatch(/\.\.\.remote,/); + }); }); diff --git a/src/components/settings/dashboard-layout-section.tsx b/src/components/settings/dashboard-layout-section.tsx index 20a4bb507..ddaf3bd5e 100644 --- a/src/components/settings/dashboard-layout-section.tsx +++ b/src/components/settings/dashboard-layout-section.tsx @@ -47,6 +47,7 @@ import { MAX_SELECTED_SCORE_RINGS, HEALTH_SCORE_RING_ID, resolveHeroRingOrder, + buildRingMutationPayload, } from "@/lib/dashboard-layout"; import { Select, @@ -327,23 +328,34 @@ export function DashboardLayoutSection({ id }: { id: string }) { * machine the rest of the section keeps. The first cut routed ring * toggles through the draft, and the combination of an unflushed * draft, the server snapshot cache, and client staleness read as - * "flipping does nothing, rings appear later". The mutation PUTs the - * SERVER layout copy with only `selectedScoreRings` changed (an open - * draft's other edits stay unsent and untouched), optimistically - * updates the widgets query, and invalidates the dashboard snapshot on - * settle so the hero reflects the choice on the next visit. + * "flipping does nothing, rings appear later". The mutation + * optimistically updates the widgets query and invalidates the + * dashboard snapshot on settle so the hero reflects the choice on the + * next visit. + * + * v1.32.1 — the PUT body used to be the FULL server layout copy, + * spreading this component's `remote` query-cache snapshot in ahead of + * the two ring fields. That snapshot can be stale by the time the + * request actually lands: if the normal + * tile/chart Save button committed a newer layout first, this + * request's explicit — present, not omitted — stale `widgets` (and + * `comparisonBaseline` / `chartOverlayPrefs`) still won on write and + * silently reverted the just-saved layout (issue #581). The payload + * now carries ONLY the ring fields; the server's preserve-when-absent + * merge (`LAYOUT_FIELD_MERGE_DISPOSITION` — `widgets` joined it in the + * same release) carries every omitted field forward from whatever is + * CURRENTLY stored, so this request can no longer race a concurrent + * Save no matter which one lands last. */ const ringMutation = useMutation({ mutationFn: async (next: { selectedScoreRings: ScoreRingId[]; heroRingOrder: HeroRingId[]; }) => { - if (!remote) throw new Error("layout not loaded"); - return apiPut("/api/dashboard/widgets", { - ...remote, - selectedScoreRings: next.selectedScoreRings, - heroRingOrder: next.heroRingOrder, - }); + return apiPut( + "/api/dashboard/widgets", + buildRingMutationPayload(next), + ); }, onMutate: async (next) => { await queryClient.cancelQueries({ diff --git a/src/lib/__tests__/dashboard-layout.test.ts b/src/lib/__tests__/dashboard-layout.test.ts index 8379f5c0c..0e091ce05 100644 --- a/src/lib/__tests__/dashboard-layout.test.ts +++ b/src/lib/__tests__/dashboard-layout.test.ts @@ -13,6 +13,7 @@ import { PRESERVED_LAYOUT_FIELDS, layoutNeedsPreserveRead, mergePreservedLayoutFields, + buildRingMutationPayload, type DashboardLayout, } from "@/lib/dashboard-layout"; @@ -1006,20 +1007,24 @@ describe("layout field merge disposition", () => { } }); - it("preserves comparisonBaseline alongside the other client-owned fields", () => { + it("preserves comparisonBaseline and widgets alongside the other client-owned fields", () => { // The regression itself: `comparisonBaseline` must be in the preserve - // set, not merely present in the disposition map. + // set, not merely present in the disposition map. `widgets` joined it in + // v1.32.1 (issue #581) — see the dedicated describe block below. expect([...PRESERVED_LAYOUT_FIELDS].sort()).toEqual([ "chartOverlayPrefs", "comparisonBaseline", "heroRingOrder", "selectedScoreRings", + "widgets", ]); }); - it("replaces the fields the request itself owns", () => { + it("replaces only the field every request must state", () => { + // `version` is the sole `"replace"` field left once `widgets` joined + // the preserve set — a PUT that omits it is rejected by Zod, so there + // is never a stored value to fall back to. expect(LAYOUT_FIELD_MERGE_DISPOSITION.version).toBe("replace"); - expect(LAYOUT_FIELD_MERGE_DISPOSITION.widgets).toBe("replace"); }); it("skips the stored-layout read when the client sent every preserved field", () => { @@ -1086,3 +1091,51 @@ describe("layout field merge disposition", () => { expect(merged.widgets).toEqual(fullyPopulatedLayout.widgets); }); }); + +/** + * v1.32.1 — regression for issue #581 (dashboard layout race). The + * Settings page's instant score-ring PUT used to resend the FULL layout, + * built from the `remote` query-cache snapshot + * (`{...remote, selectedScoreRings, heroRingOrder}`). That snapshot can be + * stale relative to an in-flight or already-committed tile/chart Save; + * because `widgets` was a `"replace"`-disposition field on the server, an + * explicitly-present stale copy always won on write and silently reverted + * the just-saved layout. + * + * `buildRingMutationPayload` is the exact function + * `ringMutation.mutationFn` (in `dashboard-layout-section.tsx`) calls to + * build its request body. Pinning its shape here — no `widgets`, no + * `comparisonBaseline`, no `chartOverlayPrefs` — is what makes the race + * structurally impossible: a payload that never carries those fields + * cannot overwrite them no matter which request lands last. See + * `src/app/api/dashboard/widgets/__tests__/route.test.ts` for the + * server-side half — a PUT shaped like this preserves whatever layout is + * CURRENTLY stored, not a client-held snapshot. + */ +describe("buildRingMutationPayload — instant score-ring PUT payload (regression #581)", () => { + it("carries only version + the two ring fields", () => { + const payload = buildRingMutationPayload({ + selectedScoreRings: ["READINESS"], + heroRingOrder: ["HEALTH_SCORE", "READINESS"], + }); + expect(Object.keys(payload).sort()).toEqual([ + "heroRingOrder", + "selectedScoreRings", + "version", + ]); + expect(payload.version).toBe(1); + expect(payload.selectedScoreRings).toEqual(["READINESS"]); + expect(payload.heroRingOrder).toEqual(["HEALTH_SCORE", "READINESS"]); + }); + + it("never includes widgets, comparisonBaseline, or chartOverlayPrefs — the fields a concurrent Save owns", () => { + const payload = buildRingMutationPayload({ + selectedScoreRings: ["READINESS"], + heroRingOrder: ["HEALTH_SCORE", "READINESS"], + }); + expect(payload.widgets).toBeUndefined(); + expect(payload.comparisonBaseline).toBeUndefined(); + expect(payload.chartOverlayPrefs).toBeUndefined(); + expect("widgets" in payload).toBe(false); + }); +}); diff --git a/src/lib/ai/coach/__tests__/outbound-guard.test.ts b/src/lib/ai/coach/__tests__/outbound-guard.test.ts index 6f4e20214..e429fc47a 100644 --- a/src/lib/ai/coach/__tests__/outbound-guard.test.ts +++ b/src/lib/ai/coach/__tests__/outbound-guard.test.ts @@ -67,6 +67,34 @@ describe("screenCoachReply — risk score", () => { }); }); +// Issue #587 — a grounded Sleep Score explanation was replaced with the +// generic clinical-risk refusal because the named-risk-engine pattern +// matched bare "score", not just the SCORE2 risk calculator it was meant to +// name. +describe("screenCoachReply — issue #587 ordinary wellness-score wording", () => { + const allowed = [ + "Your Sleep Score is based on sleep duration and stage balance — it's currently in a healthy range for you.", + "Your Readiness Score is lower than your recent baseline, likely from the shorter sleep window.", + "This Health Score is based on the available tracked pillars: BP, weight, mood, and compliance.", + ]; + for (const reply of allowed) { + it(`allows: ${reply.slice(0, 40)}`, () => { + const d = screenCoachReply(reply, "en"); + expect(d.block).toBe(false); + expect(d.reason).toBeNull(); + }); + } + + it("still blocks a reply naming the SCORE2 risk engine", () => { + const d = screenCoachReply( + "Based on SCORE2, your cardiovascular risk profile looks elevated.", + "en", + ); + expect(d.block).toBe(true); + expect(d.reason).toBe("risk_score"); + }); +}); + describe("screenCoachReply — clean replies", () => { it("passes a grounded, non-prescriptive reply", () => { const reply = diff --git a/src/lib/ai/safety/__tests__/outbound-screen.test.ts b/src/lib/ai/safety/__tests__/outbound-screen.test.ts index 27f66b697..be4a54b3a 100644 --- a/src/lib/ai/safety/__tests__/outbound-screen.test.ts +++ b/src/lib/ai/safety/__tests__/outbound-screen.test.ts @@ -107,6 +107,54 @@ describe("screenModelOutput — risk-score, all six locales", () => { } }); +// ── issue #587 — bare "score" is not a fabricated clinical risk score ───── + +/** + * The named-risk-engine pattern used `score2?` (the `2` optional), so it + * matched bare "score" too — any mention of one of THIS APP's own computed, + * documented scores (Sleep Score, Readiness Score, Health Score, …) tripped + * the fabricated-risk-engine bank meant for SCORE2/Framingham/ASCVD/QRISK. + */ +const WELLNESS_SCORE_CLEAN: readonly string[] = [ + "Your Sleep Score is based on sleep duration and stage balance.", + "Your Readiness Score is lower than your recent baseline.", + "This Health Score is based on the available tracked pillars.", + "Your Recovery Score and Stress Score both moved this week.", + "The Strain Score reflects yesterday's training load.", +]; + +describe("screenModelOutput — issue #587 ordinary wellness-score wording", () => { + for (const text of WELLNESS_SCORE_CLEAN) { + it(`passes: ${text}`, () => { + const d = screenModelOutput(text, "en", CONVERSATIONAL_CONTRACTS); + expect(d.block).toBe(false); + expect(d.reason).toBeNull(); + }); + } + + it("still blocks the literal named risk engine SCORE2", () => { + const d = screenModelOutput( + "Based on SCORE2, your cardiovascular risk profile looks elevated.", + "en", + CONVERSATIONAL_CONTRACTS, + ); + expect(d.block).toBe(true); + expect(d.reason).toBe("risk_score"); + }); + + it("still blocks Framingham, ASCVD, and QRISK by name", () => { + for (const engine of [ + "your Framingham result was concerning", + "the ASCVD estimate points to elevated risk", + "your QRISK figure came back high", + ]) { + const d = screenModelOutput(engine, "en", CONVERSATIONAL_CONTRACTS); + expect(d.block).toBe(true); + expect(d.reason).toBe("risk_score"); + } + }); +}); + // ── causal claims (GROUND RULE 12) ─────────────────────────────────────── const CAUSAL_VIOLATIONS: Record = { diff --git a/src/lib/ai/safety/outbound-screen.ts b/src/lib/ai/safety/outbound-screen.ts index 899f5dd78..1be3a7210 100644 --- a/src/lib/ai/safety/outbound-screen.ts +++ b/src/lib/ai/safety/outbound-screen.ts @@ -215,8 +215,14 @@ const RISK_PATTERNS: Record = { /\b\d{1,3}\s*%\s+(?:risk|chance|probability|likelihood)\b/i, /\b(?:risk|chance|probability|likelihood)\s+(?:of|is|at)\s+(?:about\s+|roughly\s+|~)?\d{1,3}\s*%/i, /\b(?:10[- ]year|ten[- ]year|lifetime)\s+(?:cardiovascular|cardiac|heart|stroke|mortality|cvd|ascvd)\s+risk\b/i, - // Named risk engines are fabrications on any surface — the server runs none. - /\b(?:framingham|ascvd|score2?|qrisk)\b/i, + // Named risk engines are fabrications on any surface — the server runs + // none. `score2?` (the `2` optional) was the #587 bug: it matched bare + // "score" too, so every mention of this app's OWN computed scores + // (Sleep Score, Readiness Score, Health Score, …) tripped the named- + // engine bank. SCORE2 is a specific clinical risk calculator (like + // Framingham/ASCVD/QRISK) — only the literal name is a fabrication + // signal; the bare word "score" carries none on its own. + /\b(?:framingham|ascvd|score2|qrisk)\b/i, ], de: [ /\brisiko\s+(?:von|bei|liegt\s+bei)\s+(?:etwa\s+|ungefähr\s+|~)?\d{1,3}\s*%/i, diff --git a/src/lib/analytics/__tests__/health-score-fast-path.test.ts b/src/lib/analytics/__tests__/health-score-fast-path.test.ts index 38b9cf4aa..fd6d575f3 100644 --- a/src/lib/analytics/__tests__/health-score-fast-path.test.ts +++ b/src/lib/analytics/__tests__/health-score-fast-path.test.ts @@ -444,6 +444,132 @@ describe("computeUserHealthScoreFastPath", () => { }); }); + /** + * Issue #582 — a raw-record-presence check let a `0`/red score through + * when every pillar still failed its own eligibility bar (weight needs + * >= 2 readings, mood needs >= 5 entries, BP needs a paired reading, + * compliance needs an active scheduled medication). The fix checks the + * COMPUTED result's components instead of raw row counts. + */ + describe("insufficient-data guard (#582)", () => { + it("returns null — not a 0 score — when only one mood entry exists and no other pillar is eligible", async () => { + const coverage = new Map([["WEIGHT", false]]); + FULLY_COVERED.mockReturnValue(false); + PROBE.mockResolvedValue(coverage); + const now = new Date("2026-06-07T12:00:00.000Z"); + + // No weight rows, no BP rows, no medications. + MEASUREMENT_FIND_MANY.mockResolvedValueOnce([]).mockResolvedValueOnce([]); + // Exactly one mood entry — below the 5-entry stability floor, so + // moodStability() resolves to null even though the raw row count + // is nonzero. + MOOD_FIND_MANY.mockResolvedValue([{ score: 4, moodLoggedAt: now }]); + + const result = await computeUserHealthScoreFastPath({ + userId: "user-one-mood-entry", + bpInTargetPct: null, + heightCm: null, + now, + coverage, + }); + + expect(result).toBeNull(); + const calls = ANNOTATE.mock.calls.map((c) => c[0]); + const nullCall = calls.find( + (c) => c?.meta?.healthScore?.reason === "no_components_available", + ); + expect(nullCall).toBeDefined(); + }); + + it("returns null — not a 0 score — when only one weight reading exists and no other pillar is eligible", async () => { + const coverage = new Map([["WEIGHT", false]]); + FULLY_COVERED.mockReturnValue(false); + PROBE.mockResolvedValue(coverage); + const now = new Date("2026-06-07T12:00:00.000Z"); + + // Exactly one weight reading — below the 2-reading regression floor, + // so weightTrendAlignment() resolves to null. + MEASUREMENT_FIND_MANY.mockResolvedValueOnce([ + { + measuredAt: new Date("2026-06-05T08:00:00.000Z"), + value: 82.5, + source: "MANUAL", + }, + ]).mockResolvedValueOnce([]); // BP-SYS source attribution, empty. + + const result = await computeUserHealthScoreFastPath({ + userId: "user-one-weight-reading", + bpInTargetPct: null, + heightCm: null, + now, + coverage, + }); + + expect(result).toBeNull(); + const calls = ANNOTATE.mock.calls.map((c) => c[0]); + const nullCall = calls.find( + (c) => c?.meta?.healthScore?.reason === "no_components_available", + ); + expect(nullCall).toBeDefined(); + }); + + it("returns null when raw records exist across multiple pillars but none clears its own minimum", async () => { + const coverage = new Map([["WEIGHT", false]]); + FULLY_COVERED.mockReturnValue(false); + PROBE.mockResolvedValue(coverage); + const now = new Date("2026-06-07T12:00:00.000Z"); + + // One weight reading (< 2) + one mood entry (< 5) + no BP + no + // active medications. Every pillar has SOME data, but none is + // individually eligible. + MEASUREMENT_FIND_MANY.mockResolvedValueOnce([ + { + measuredAt: new Date("2026-06-05T08:00:00.000Z"), + value: 82.5, + source: "MANUAL", + }, + ]).mockResolvedValueOnce([]); + MOOD_FIND_MANY.mockResolvedValue([{ score: 3, moodLoggedAt: now }]); + + const result = await computeUserHealthScoreFastPath({ + userId: "user-multi-pillar-ineligible", + bpInTargetPct: null, + heightCm: null, + now, + coverage, + }); + + expect(result).toBeNull(); + }); + + it("preserves a legitimately computed score of 0 rather than treating it as insufficient data", async () => { + const coverage = new Map([["WEIGHT", false]]); + FULLY_COVERED.mockReturnValue(false); + PROBE.mockResolvedValue(coverage); + const now = new Date("2026-06-07T12:00:00.000Z"); + + // No weight, no mood, no medications — but a real, computed BP + // pillar that lands on exactly 0. This must stay a genuine `0` + // score, not get swept into the null branch. + MEASUREMENT_FIND_MANY.mockResolvedValueOnce([]).mockResolvedValueOnce([ + { measuredAt: new Date("2026-06-05T08:00:00.000Z"), source: "MANUAL" }, + ]); + + const result = await computeUserHealthScoreFastPath({ + userId: "user-legit-zero", + bpInTargetPct: 0, + bpGradedScore: 0, + heightCm: null, + now, + coverage, + }); + + expect(result).not.toBeNull(); + expect(result?.components.bp.value).toBe(0); + expect(result?.score).toBe(0); + }); + }); + describe("coverage probing", () => { it("probes coverage when the caller omits the map", async () => { const coverage = new Map([]); diff --git a/src/lib/analytics/__tests__/intraday-pulse-io.test.ts b/src/lib/analytics/__tests__/intraday-pulse-io.test.ts index 867c0b48c..57450a5b4 100644 --- a/src/lib/analytics/__tests__/intraday-pulse-io.test.ts +++ b/src/lib/analytics/__tests__/intraday-pulse-io.test.ts @@ -13,6 +13,7 @@ import { describe, expect, it, vi, beforeEach } from "vitest"; const findManyMock = vi.fn(); const workoutFindManyMock = vi.fn(); +const { annotateMock } = vi.hoisted(() => ({ annotateMock: vi.fn() })); vi.mock("@/lib/db", () => ({ prisma: { @@ -21,6 +22,8 @@ vi.mock("@/lib/db", () => ({ }, })); +vi.mock("@/lib/logging/context", () => ({ annotate: annotateMock })); + import { loadIntradayPulse } from "@/lib/analytics/intraday-pulse-io"; import { resolveRestingPulseSeries, @@ -42,6 +45,7 @@ describe("loadIntradayPulse", () => { findManyMock.mockReset(); workoutFindManyMock.mockReset(); workoutFindManyMock.mockResolvedValue([]); + annotateMock.mockReset(); }); it("M2 — queries PULSE/step rows against the exact local-day window, not the padded lead/trail superset", async () => { @@ -209,4 +213,59 @@ describe("loadIntradayPulse", () => { { startMinute: 490, mean: 75, count: 2, min: 70, max: 81 }, ]); }); + + it("v1.32.1 (issue #585) — annotates the per-shape row/bucket breakdown for observability", async () => { + const tz = "UTC"; + const dateKey = "2026-06-15"; + + const restingRows = Array.from({ length: 20 }, (_, i) => ({ + value: 55, + measuredAt: new Date( + `2026-05-${String((i % 28) + 1).padStart(2, "0")}T06:00:00.000Z`, + ), + })); + // One uploaded 10-min bucket row and one raw per-sample row sharing the + // day — the DTO merges them (bucket overlays raw on a slot collision), + // but the annotation must still report each SHAPE's own row count, not + // just the merged bucket total. + const uploadedBucket = { + value: 72, + valueMin: 64, + valueMax: 88, + measuredAt: new Date("2026-06-15T08:09:00.000Z"), + externalId: + "stats:HKQuantityTypeIdentifierHeartRate:2026-06-15T08:00:00.000Z", + }; + const rawSample = { + value: 65, + valueMin: null, + valueMax: null, + measuredAt: new Date("2026-06-15T09:05:00.000Z"), + externalId: "sample:abc123", + }; + + findManyMock.mockImplementation( + async (args: { + where: { type: string; measuredAt?: unknown }; + take?: number; + }) => { + if (args.where.type === "RESTING_HEART_RATE") return restingRows; + if (args.where.type === "PULSE" && args.where.measuredAt) { + return [uploadedBucket, rawSample]; + } + return []; + }, + ); + + await loadIntradayPulse("user-1", tz, dateKey); + + expect(annotateMock).toHaveBeenCalledWith({ + meta: { + intraday_raw_sample_rows: 1, + intraday_uploaded_bucket_rows: 1, + intraday_folded_hourly_rows: 0, + intraday_ten_min_buckets: 2, + }, + }); + }); }); diff --git a/src/lib/analytics/health-score-fast-path.ts b/src/lib/analytics/health-score-fast-path.ts index a74b3f929..2a85fada3 100644 --- a/src/lib/analytics/health-score-fast-path.ts +++ b/src/lib/analytics/health-score-fast-path.ts @@ -465,33 +465,6 @@ export async function computeUserHealthScoreFastPath( score: r.score, })); - // Empty-path early-out — nothing computable, hero hides cleanly. - if ( - bpInTargetPct === null && - weightSeriesLast30d.length === 0 && - moodSeriesLast30d.length === 0 && - medicationCompliance30.length === 0 - ) { - annotate({ - meta: { - healthScore: { - score: null, - reason: "no_components_available", - path: weightCovered ? "rollup" : "live", - weightLongWindow: - weightLongWindowMean !== null - ? { - mean: Math.round(weightLongWindowMean * 100) / 100, - granularity: weightLongWindowGranularity, - buckets: weightLongWindowBucketCount, - } - : null, - }, - }, - }); - return null; - } - const windowEndAt = now.toISOString(); const bpSourceTokens = uniqueComponentSources( @@ -557,6 +530,49 @@ export async function computeUserHealthScoreFastPath( }; const result = computeHealthScore(current, previous); + + // No-pillar-eligible early-out — nothing computable, hero hides cleanly. + // + // The naive raw-record check this replaced only asked whether any pillar + // had a nonzero row count, not whether that count cleared the pillar's own + // eligibility bar: weight needs >= 2 readings, mood needs >= 5 entries, BP + // needs a paired reading, compliance needs an active scheduled medication. + // An account that clears the row-count check but misses every pillar's + // bar (e.g. exactly one mood entry, or a single weight reading) fell + // through to `computeHealthScore`, which sums no components and returns a + // raw `0` — indistinguishable from a genuinely poor score. Checking the + // COMPUTED result's components (the same eligibility rules the score + // itself already applies) instead of re-deriving the thresholds here means + // this can never drift out of sync with `computeHealthScore`. A component + // that is legitimately computed as `0` still has `value: 0`, not `null`, + // so it is never swept into this branch. + const noComponentEligible = + result.components.bp.value === null && + result.components.weight.value === null && + result.components.mood.value === null && + result.components.compliance.value === null; + + if (noComponentEligible) { + annotate({ + meta: { + healthScore: { + score: null, + reason: "no_components_available", + path: weightCovered ? "rollup" : "live", + weightLongWindow: + weightLongWindowMean !== null + ? { + mean: Math.round(weightLongWindowMean * 100) / 100, + granularity: weightLongWindowGranularity, + buckets: weightLongWindowBucketCount, + } + : null, + }, + }, + }); + return null; + } + annotate({ meta: { healthScore: { diff --git a/src/lib/analytics/intraday-pulse-io.ts b/src/lib/analytics/intraday-pulse-io.ts index 9a4df4243..ec7987b34 100644 --- a/src/lib/analytics/intraday-pulse-io.ts +++ b/src/lib/analytics/intraday-pulse-io.ts @@ -16,6 +16,7 @@ * digest's `tension_window` builder, so the signal is computed one way. */ import { prisma } from "@/lib/db"; +import { annotate } from "@/lib/logging/context"; import { percentile } from "@/lib/insights/strain-score"; import { resolveRestingPulseSeries } from "@/lib/analytics/resting-pulse"; import { localDayWindow } from "@/lib/measurements/consolidation-tz"; @@ -349,6 +350,26 @@ export async function loadIntradayPulse( }) : null; + // v1.32.1 (issue #585) — per-shape row/bucket breakdown. A report of + // "the chart looks sparser after the ZIP-import cutoff" is otherwise + // undiagnosable from server logs: the route's own annotation only ever + // surfaced the FINAL bucket count, with no visibility into which + // ingestion shape (raw ZIP-imported samples vs uploaded 10-min + // aggregates) the day's data actually came from, nor how many DB rows + // fed each shape before bucketing. Grepping `insights.pulse.intraday` + // now shows both — e.g. `uploaded_bucket_rows: 6` next to + // `ten_min_buckets: 6` confirms the uploaded shape IS being read and + // placed 1:1, narrowing a future report to "the client uploaded too few + // buckets" rather than leaving the read path itself as a live suspect. + annotate({ + meta: { + intraday_raw_sample_rows: rawPulseRows.length, + intraday_uploaded_bucket_rows: uploadedBucketRows.length, + intraday_folded_hourly_rows: foldedPulseRows.length, + intraday_ten_min_buckets: tenMinSeries.length, + }, + }); + return { dateKey, timezone, diff --git a/src/lib/dashboard-layout.ts b/src/lib/dashboard-layout.ts index 54447cc5d..4c4e790a4 100644 --- a/src/lib/dashboard-layout.ts +++ b/src/lib/dashboard-layout.ts @@ -574,8 +574,8 @@ export interface DashboardLayout { * omits it. * * - `"replace"` — the field is the point of the request. A PUT that omits it - * is either rejected by Zod (`version`, `widgets` are required) or means - * the caller genuinely has nothing to say. + * is rejected by Zod (`version` is required) — the caller always has + * something to say about a `"replace"` field. * - `"preserve"` — the field is owned by a surface OTHER than the one making * this request, so an omission means "I don't know about this", never * "clear it". The route reads the stored layout and carries the existing @@ -591,6 +591,18 @@ export interface DashboardLayout { * clamps a missing value to `"none"`, so every native layout save wiped the * baseline the user had picked on the web. * + * `widgets` moved from `"replace"` to `"preserve"` in v1.32.1 (issue #581). + * The Settings page's instant score-ring toggle used to resend the FULL + * layout — including `widgets` — built from the query cache's `remote` + * snapshot. That snapshot can be stale by the time the ring request lands: + * if a normal tile/chart Save committed a newer layout first, the + * ring request's explicit (present, not omitted) stale `widgets` value + * still won on write and silently reverted the just-saved layout. Making + * `widgets` optional + preserve-when-absent lets the ring mutation omit it + * entirely, so the field it never touches can no longer race a concurrent + * Save — the route always carries forward whatever is CURRENTLY stored, + * not a client-held copy. + * * `satisfies Record` is the load-bearing part. * Adding a field to `DashboardLayout` without adding it here is a TYPE error, * so the next field cannot repeat the `comparisonBaseline` mistake by @@ -598,7 +610,7 @@ export interface DashboardLayout { */ export const LAYOUT_FIELD_MERGE_DISPOSITION = { version: "replace", - widgets: "replace", + widgets: "preserve", comparisonBaseline: "preserve", chartOverlayPrefs: "preserve", selectedScoreRings: "preserve", @@ -647,6 +659,38 @@ export function mergePreservedLayoutFields( return merged; } +/** + * v1.32.1 — pure payload builder for the Settings page's instant score-ring + * PUT (`ringMutation` in `dashboard-layout-section.tsx`). Lives in this + * client-safe module (imported by both the client component and the server + * route already) rather than the "use client" component file, so a + * server-side regression test can import it without pulling the component's + * UI dependency graph across the client/server boundary — see + * `src/app/api/dashboard/widgets/__tests__/route.test.ts`. + * + * Deliberately returns ONLY the ring fields (plus `version`) — no + * `widgets`, `comparisonBaseline`, or `chartOverlayPrefs`. Before v1.32.1 the + * ring mutation resent the FULL layout built from the client's `remote` + * query-cache snapshot, which can be stale by the time the request lands: if + * a normal tile/chart Save committed a newer layout first, the ring + * request's explicit — present, not omitted — stale `widgets` still won on + * write and silently reverted the just-saved layout (issue #581). Those + * three fields are all `"preserve"`-disposition here + * (`LAYOUT_FIELD_MERGE_DISPOSITION`): omitting them means the route always + * carries forward whatever is CURRENTLY stored, so this payload can no + * longer race a concurrent Save no matter which request lands last. + */ +export function buildRingMutationPayload(next: { + selectedScoreRings: ScoreRingId[]; + heroRingOrder: HeroRingId[]; +}): Partial { + return { + version: DASHBOARD_LAYOUT_VERSION, + selectedScoreRings: next.selectedScoreRings, + heroRingOrder: next.heroRingOrder, + }; +} + const DASHBOARD_LAYOUT_VERSION = 1; /** diff --git a/src/lib/import/__tests__/unzip-export-xml.test.ts b/src/lib/import/__tests__/unzip-export-xml.test.ts index 3151558c5..3863f79b8 100644 --- a/src/lib/import/__tests__/unzip-export-xml.test.ts +++ b/src/lib/import/__tests__/unzip-export-xml.test.ts @@ -3,16 +3,27 @@ import { writeFileSync, mkdtempSync, readFileSync } from "node:fs"; import { join } from "node:path"; import { tmpdir } from "node:os"; import { deflateRawSync } from "node:zlib"; +import { pipeline } from "node:stream/promises"; +import { Readable } from "node:stream"; -import { extractExportXml, readCentralDirectory } from "../unzip-export-xml"; +import { + createByteCap, + extractExportXml, + readCentralDirectory, +} from "../unzip-export-xml"; /** - * Hand-build a single-entry deflate-compressed ZIP archive with the - * supplied filename + payload. Enough to exercise the central-directory - * walker + extractor without pulling in a full zip library. + * Hand-build a single-entry ZIP archive with the supplied filename + + * payload. Enough to exercise the central-directory walker + extractor + * without pulling in a full zip library. `method` selects compression + * method 8 (deflate, default) or 0 (stored — bytes copied verbatim). */ -function buildMinimalZip(filename: string, payload: Buffer): Buffer { - const compressed = deflateRawSync(payload); +function buildMinimalZip( + filename: string, + payload: Buffer, + method: 0 | 8 = 8, +): Buffer { + const compressed = method === 8 ? deflateRawSync(payload) : payload; const crc32 = (() => { // Pre-computed lookup table for CRC-32. node:zlib has crc32 in 22+ // but we keep the function inline to stay version-agnostic. @@ -38,7 +49,7 @@ function buildMinimalZip(filename: string, payload: Buffer): Buffer { localHeader.writeUInt32LE(0x04034b50, 0); localHeader.writeUInt16LE(20, 4); // version localHeader.writeUInt16LE(0, 6); // flags - localHeader.writeUInt16LE(8, 8); // method: deflate + localHeader.writeUInt16LE(method, 8); // method localHeader.writeUInt16LE(0, 10); // time localHeader.writeUInt16LE(0, 12); // date localHeader.writeUInt32LE(crc32, 14); @@ -53,7 +64,7 @@ function buildMinimalZip(filename: string, payload: Buffer): Buffer { cdh.writeUInt16LE(20, 4); // version made by cdh.writeUInt16LE(20, 6); // version needed cdh.writeUInt16LE(0, 8); // flags - cdh.writeUInt16LE(8, 10); // method + cdh.writeUInt16LE(method, 10); // method cdh.writeUInt16LE(0, 12); // time cdh.writeUInt16LE(0, 14); // date cdh.writeUInt32LE(crc32, 16); @@ -95,7 +106,7 @@ describe("readCentralDirectory", () => { }); describe("extractExportXml", () => { - it("extracts the export.xml member to a temp file", () => { + it("extracts a deflated export.xml member to a temp file", async () => { const payload = Buffer.from( ``, ); @@ -104,7 +115,7 @@ describe("extractExportXml", () => { const zipPath = join(tmp, "export.zip"); writeFileSync(zipPath, zip); - const out = extractExportXml(zipPath); + const out = await extractExportXml(zipPath); expect(out.xmlBytes).toBe(payload.length); expect(out.otherMembers).toHaveLength(0); @@ -112,13 +123,65 @@ describe("extractExportXml", () => { expect(extracted.toString("utf8")).toBe(payload.toString("utf8")); }); - it("throws when the export.xml member is missing", () => { + // v1.32.1 (issue #588) — the stored (uncompressed) path now runs + // through the same streaming pipeline as deflate; a stored member + // must still round-trip byte-exact through it. + it("extracts a stored (uncompressed) export.xml member to a temp file", async () => { + const payload = Buffer.from( + ``, + ); + const zip = buildMinimalZip("apple_health_export/export.xml", payload, 0); + const tmp = mkdtempSync(join(tmpdir(), "healthlog-unzip-")); + const zipPath = join(tmp, "export.zip"); + writeFileSync(zipPath, zip); + + const out = await extractExportXml(zipPath); + expect(out.xmlBytes).toBe(payload.length); + + const extracted = readFileSync(out.xmlPath); + expect(extracted.toString("utf8")).toBe(payload.toString("utf8")); + }); + + it("throws when the export.xml member is missing", async () => { const payload = Buffer.from("noop"); const zip = buildMinimalZip("other-file.txt", payload); const tmp = mkdtempSync(join(tmpdir(), "healthlog-unzip-")); const zipPath = join(tmp, "export.zip"); writeFileSync(zipPath, zip); - expect(() => extractExportXml(zipPath)).toThrow(/missing/); + await expect(extractExportXml(zipPath)).rejects.toThrow(/missing/); + }); +}); + +// v1.32.1 (issue #588) — the streaming inflate Transform has no +// `maxOutputLength` option, so the zip-bomb defence moved to a +// hand-rolled byte-counting stage. Exercise it directly at a tiny +// limit; the real 8 GiB ceiling is not practical to trip from a +// fixture. +describe("createByteCap", () => { + it("passes bytes through unchanged while under the limit", async () => { + const chunks: Buffer[] = []; + await pipeline( + Readable.from([Buffer.from("hello"), Buffer.from("world")]), + createByteCap(10), + async function collect(source) { + for await (const chunk of source) chunks.push(chunk as Buffer); + }, + ); + expect(Buffer.concat(chunks).toString("utf8")).toBe("helloworld"); + }); + + it("rejects the stream once the running total exceeds the limit", async () => { + await expect( + pipeline( + Readable.from([Buffer.from("hello"), Buffer.from("world")]), + createByteCap(6), + async function drain(source) { + for await (const chunk of source) { + void chunk; + } + }, + ), + ).rejects.toThrow(/exceeds the 6-byte cap/); }); }); diff --git a/src/lib/import/unzip-export-xml.ts b/src/lib/import/unzip-export-xml.ts index 7333f2d09..6379ce0ec 100644 --- a/src/lib/import/unzip-export-xml.ts +++ b/src/lib/import/unzip-export-xml.ts @@ -7,21 +7,49 @@ * deflate-compressed ZIP archive. Pulling in `yauzl` / `adm-zip` / etc. * for one file we only ever read once would balloon the build graph * (and `yauzl` ships a non-trivial number of transitive deps). Node 22 - * ships `node:zlib` `inflateRawSync` already; the only missing piece - * is a tiny ZIP central-directory walker. + * ships `node:zlib` already; the only missing piece is a tiny ZIP + * central-directory walker. * * Coverage scope: * - Stored entries (compression method 0) — copy bytes verbatim. - * - Deflated entries (compression method 8) — `inflateRawSync`. + * - Deflated entries (compression method 8) — streamed through + * `zlib.createInflateRaw()`. * - Encrypted entries → unsupported (Apple does not encrypt the * export.zip; reject with a clear error). * - Zip64 — supported for the central directory record (Apple * exports easily push past the 4 GB limit on multi-year accounts). * + * v1.32.1 (issue #588) — extracting the member used to run through + * `inflateRawSync()` on a single in-memory buffer holding the WHOLE + * inflated `export.xml` (up to `MAX_DECOMPRESSED_BYTES`) at the same + * time the compressed-archive buffer was still resident, then + * `writeFileSync()`'d the result in one blocking call. `extractExportXml` + * runs on the same event loop as the web server in the default + * single-container deployment (`src/instrumentation.ts` starts the + * pg-boss worker in-process), so that synchronous path could pin the + * Node main thread for minutes on a real multi-year export — freezing + * every other request the app was serving — while briefly needing + * roughly 2x the decompressed size in JS heap. A user hitting this saw + * an import that never leaves "Unpacking the archive…": the phase + * label is written to `ImportJob.status` BEFORE extraction starts and + * only advances once `extractExportXml()` returns, so a process that + * dies mid-extraction (OOM, restart) before that return leaves the row + * silently stuck with no failure ever recorded. The member now streams + * through `zlib.createInflateRaw()` into a file write stream — + * decompression work moves off the JS event loop onto the libuv + * threadpool in bounded chunks, and peak memory is bounded by the + * chunk size rather than the whole decompressed payload. The + * `MAX_DECOMPRESSED_BYTES` zip-bomb defence moves with it: the + * streaming inflate API has no `maxOutputLength` option, so a + * byte-counting transform enforces the cap on the real output as it + * flows. + * * Locks per `.planning/research/v1434-r-1-xml-import.md` §6.1. */ -import { readFileSync, writeFileSync } from "node:fs"; -import { inflateRawSync } from "node:zlib"; +import { readFileSync, createWriteStream, statSync, unlinkSync } from "node:fs"; +import { createInflateRaw } from "node:zlib"; +import { Readable, Transform } from "node:stream"; +import { pipeline } from "node:stream/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -44,11 +72,15 @@ const MAX_DECOMPRESSED_BYTES = 8 * 1024 * 1024 * 1024; * Pre-flight refusal threshold for the central-directory's advertised * ratio. Legitimate Apple Health XML compresses at maybe 10–20× under * DEFLATE; anything claiming a 200× expansion is a synthesized bomb. - * This is a coarse signal — the `maxOutputLength` ceiling on the - * inflate call below is the load-bearing defence. + * This is a coarse signal — the byte-counting cap on the streamed + * inflate output (`streamEntryToFile()` below) is the load-bearing + * defence. */ const MAX_COMPRESSION_RATIO = 200; +/** Chunk size fed into the inflate/write pipeline per `write()` call. */ +const STREAM_CHUNK_BYTES = 1 * 1024 * 1024; + /** A single entry within the archive's central directory. */ interface CentralDirectoryEntry { fileName: string; @@ -78,7 +110,9 @@ export interface UnzipResult { * Throws when the member is missing, encrypted, or compressed with * an unsupported method. */ -export function extractExportXml(archivePath: string): UnzipResult { +export async function extractExportXml( + archivePath: string, +): Promise { const buf = readFileSync(archivePath); const entries = readCentralDirectory(buf); @@ -105,8 +139,8 @@ export function extractExportXml(archivePath: string): UnzipResult { // Pre-flight zip-bomb defence. The central directory's advertised // `uncompressedSize` is attacker-controlled (a malicious archive can // lie), so this catches honest-but-oversized payloads early; the - // load-bearing defence is the `maxOutputLength` cap inside - // `extractEntry()` which trips on the actual inflate output. + // load-bearing defence is the byte-counting cap inside + // `streamEntryToFile()` which trips on the actual inflate output. if (exportXmlEntry.uncompressedSize > MAX_DECOMPRESSED_BYTES) { throw new Error( `export.xml declares an uncompressed size of ${exportXmlEntry.uncompressedSize} bytes` + @@ -126,13 +160,11 @@ export function extractExportXml(archivePath: string): UnzipResult { ); } - const xmlBytes = extractEntry(buf, exportXmlEntry); - const xmlPath = join( tmpdir(), `healthlog-import-${Date.now()}-${Math.random().toString(36).slice(2, 10)}.xml`, ); - writeFileSync(xmlPath, xmlBytes); + const xmlBytes = await streamEntryToFile(buf, exportXmlEntry, xmlPath); const otherMembers = entries .filter((e) => e !== exportXmlEntry) @@ -140,7 +172,7 @@ export function extractExportXml(archivePath: string): UnzipResult { return { xmlPath, - xmlBytes: xmlBytes.length, + xmlBytes, otherMembers, }; } @@ -260,8 +292,59 @@ export function readCentralDirectory(buf: Buffer): CentralDirectoryEntry[] { return entries; } -/** Extract the bytes of a single ZIP entry into a Buffer. */ -function extractEntry(buf: Buffer, entry: CentralDirectoryEntry): Buffer { +/** + * Split a Buffer into fixed-size slices (no copying — `subarray` views + * share the backing memory) so the inflate/write pipeline processes the + * entry in bounded chunks instead of one giant `write()` call. + */ +function* chunkBuffer(buf: Buffer, size: number): Generator { + for (let offset = 0; offset < buf.length; offset += size) { + yield buf.subarray(offset, Math.min(offset + size, buf.length)); + } +} + +/** + * Byte-counting passthrough that refuses to forward more than `limit` + * bytes — the streaming equivalent of `inflateRawSync`'s + * `maxOutputLength`, since the Transform-based zlib API exposes no such + * option. Trips on the actual bytes flowing through even when the + * central-directory metadata lied about the uncompressed size. Exported + * so a unit test can exercise the cap directly against a small limit — + * the real `MAX_DECOMPRESSED_BYTES` ceiling (8 GiB) is not practical to + * trip from a test fixture. + */ +export function createByteCap(limit: number): Transform { + let total = 0; + return new Transform({ + transform(chunk: Buffer, _enc, callback) { + total += chunk.length; + if (total > limit) { + callback( + new Error( + `Decompressed export.xml exceeds the ${limit}-byte cap` + + " — refusing as a suspected zip bomb.", + ), + ); + return; + } + callback(null, chunk); + }, + }); +} + +/** + * Stream a single ZIP entry's bytes out to `destPath`, decompressing + * through `zlib.createInflateRaw()` when needed. Runs entirely off the + * JS main thread (stream I/O + the libuv-threadpool-backed zlib + * binding), so a multi-GB member no longer blocks the event loop the + * way the old `inflateRawSync()` + `writeFileSync()` pair did. Returns + * the number of bytes written. + */ +async function streamEntryToFile( + buf: Buffer, + entry: CentralDirectoryEntry, + destPath: string, +): Promise { const local = entry.localHeaderOffset; if (buf.readUInt32LE(local) !== LOCAL_FILE_HEADER) { throw new Error( @@ -271,18 +354,32 @@ function extractEntry(buf: Buffer, entry: CentralDirectoryEntry): Buffer { const localFileNameLen = buf.readUInt16LE(local + 26); const localExtraLen = buf.readUInt16LE(local + 28); const dataStart = local + 30 + localFileNameLen + localExtraLen; - const compressed = buf.slice(dataStart, dataStart + entry.compressedSize); + const compressed = buf.subarray(dataStart, dataStart + entry.compressedSize); - if (entry.compressionMethod === 0) { - return compressed; + const source = Readable.from(chunkBuffer(compressed, STREAM_CHUNK_BYTES)); + const dest = createWriteStream(destPath); + const cap = createByteCap(MAX_DECOMPRESSED_BYTES); + + try { + if (entry.compressionMethod === 0) { + await pipeline(source, cap, dest); + } else { + // Method 8 — DEFLATE, streamed through the async zlib Transform. + await pipeline(source, createInflateRaw(), cap, dest); + } + } catch (err) { + // A mid-stream failure (byte cap trip, corrupt deflate stream) can + // leave a partial file on disk — the old sync path only ever wrote + // once inflate had already fully succeeded. Clean up so a rejected + // archive doesn't leak a partial `/tmp` file. + try { + unlinkSync(destPath); + } catch { + // ignore — best-effort + } + throw err; } - // Method 8 — DEFLATE. `maxOutputLength` instructs `node:zlib` to - // refuse expansion past the cap with a thrown error, defeating - // zip-bomb amplification even when the central-directory metadata - // lies about the uncompressed size. - return inflateRawSync(compressed, { - maxOutputLength: MAX_DECOMPRESSED_BYTES, - }); + return statSync(destPath).size; } /** diff --git a/src/lib/jobs/__tests__/apple-health-import-reconcile.test.ts b/src/lib/jobs/__tests__/apple-health-import-reconcile.test.ts new file mode 100644 index 000000000..13e240628 --- /dev/null +++ b/src/lib/jobs/__tests__/apple-health-import-reconcile.test.ts @@ -0,0 +1,186 @@ +/** + * v1.32.1 (issue #588) — periodic orphan-`ImportJob` reconcile. + * + * Before this change `reconcileOrphanImportJobs()` only ran once, at + * worker boot. A worker that crashed mid-extraction (OOM, restart) and + * came back up BEFORE the stuck row's heartbeat went stale (30 min) or + * pg-boss stopped reporting the backing job as live left that row + * stuck in `unpacking` / `parsing` / `upserting` forever — an import + * that never leaves "Unpacking the archive… 0 rows imported" with no + * failure ever surfaced, because nothing re-evaluated the row after + * that one boot-time pass. These tests cover both the periodic-tick + * wiring (queue provisioned, cron scheduled, handler bound) and the + * reconcile logic itself (a genuinely-abandoned row flips to `failed`; + * a row another worker is still actively running does not). + */ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const mocks = vi.hoisted(() => ({ + findMany: vi.fn(), + updateMany: vi.fn(), + getGlobalBoss: vi.fn(), + getJobById: vi.fn(), +})); + +vi.mock("@/lib/db", () => ({ + prisma: { + importJob: { + findMany: mocks.findMany, + updateMany: mocks.updateMany, + }, + }, + toJson: (value: unknown) => value, +})); + +vi.mock("@/lib/jobs/boss-instance", () => ({ + getGlobalBoss: mocks.getGlobalBoss, +})); + +import { + APPLE_HEALTH_IMPORT_PARSER_REVISION, + APPLE_HEALTH_IMPORT_V2_QUEUE, + IMPORT_JOB_RECONCILE_CRON, + IMPORT_JOB_RECONCILE_QUEUE, + handleImportJobReconcileTick, + reconcileOrphanImportJobs, +} from "../apple-health-import-worker"; + +const maintenanceSource = readFileSync( + join(process.cwd(), "src/lib/jobs/reminder/register-maintenance.ts"), + "utf8", +); + +beforeEach(() => { + vi.resetAllMocks(); + mocks.getGlobalBoss.mockReturnValue({ getJobById: mocks.getJobById }); + mocks.updateMany.mockResolvedValue({ count: 0 }); +}); + +describe("periodic reconcile — wiring", () => { + it("provisions the queue, schedules a 15-minute cron, and binds the handler", () => { + expect(IMPORT_JOB_RECONCILE_QUEUE).toBe("apple-health-import-reconcile"); + expect(IMPORT_JOB_RECONCILE_CRON).toBe("*/15 * * * *"); + + const allQueues = maintenanceSource.match( + /const allQueues\s*=\s*\[([\s\S]*?)\];/, + ); + expect(allQueues).not.toBeNull(); + expect(allQueues![1]).toMatch(/\bIMPORT_JOB_RECONCILE_QUEUE\b/); + + const schedules = maintenanceSource.match( + /const schedules[\s\S]*?=\s*\[([\s\S]*?)\];/, + ); + expect(schedules).not.toBeNull(); + expect(schedules![1]).toMatch( + /\[IMPORT_JOB_RECONCILE_QUEUE,\s*IMPORT_JOB_RECONCILE_CRON\]/, + ); + + expect(maintenanceSource).toMatch( + /boss\.work\(\s*IMPORT_JOB_RECONCILE_QUEUE[\s\S]{0,120}handleImportJobReconcileTick/, + ); + }); +}); + +describe("reconcileOrphanImportJobs", () => { + it("flips a row to failed once its heartbeat has gone stale", async () => { + mocks.findMany.mockResolvedValue([ + { + id: "import-stale", + pgBossJobId: "boss-1", + updatedAt: new Date(Date.now() - 31 * 60 * 1000), + }, + ]); + mocks.getJobById.mockResolvedValue({ state: "active" }); + + await reconcileOrphanImportJobs(); + + expect(mocks.updateMany).toHaveBeenCalledWith({ + where: { id: { in: ["import-stale"] } }, + data: { + status: "failed", + failureReason: "interrupted_by_restart", + completedAt: expect.any(Date), + }, + }); + }); + + it("leaves a row alone while its heartbeat is fresh and pg-boss reports the job active", async () => { + mocks.findMany.mockResolvedValue([ + { + id: "import-live", + pgBossJobId: "boss-2", + updatedAt: new Date(Date.now() - 2 * 60 * 1000), + }, + ]); + mocks.getJobById.mockResolvedValue({ state: "active" }); + + await reconcileOrphanImportJobs(); + + expect(mocks.updateMany).not.toHaveBeenCalled(); + expect(mocks.getJobById).toHaveBeenCalledWith( + APPLE_HEALTH_IMPORT_V2_QUEUE, + "boss-2", + ); + }); + + it("flips a row whose backing pg-boss job is gone even with a fresh heartbeat", async () => { + mocks.findMany.mockResolvedValue([ + { + id: "import-gone", + pgBossJobId: "boss-3", + updatedAt: new Date(Date.now() - 60 * 1000), + }, + ]); + mocks.getJobById.mockResolvedValue(null); + + await reconcileOrphanImportJobs(); + + expect(mocks.updateMany).toHaveBeenCalledWith({ + where: { id: { in: ["import-gone"] } }, + data: { + status: "failed", + failureReason: "interrupted_by_restart", + completedAt: expect.any(Date), + }, + }); + }); + + it("only ever scopes the candidate query to the current parser revision", async () => { + mocks.findMany.mockResolvedValue([]); + + await reconcileOrphanImportJobs(); + + expect(mocks.findMany).toHaveBeenCalledWith({ + where: { + parserRevision: APPLE_HEALTH_IMPORT_PARSER_REVISION, + status: { in: ["unpacking", "parsing", "upserting"] }, + }, + select: { id: true, pgBossJobId: true, updatedAt: true }, + }); + }); +}); + +describe("handleImportJobReconcileTick", () => { + it("delegates to reconcileOrphanImportJobs and resolves on success", async () => { + mocks.findMany.mockResolvedValue([]); + + await expect( + handleImportJobReconcileTick([] as never), + ).resolves.toBeUndefined(); + expect(mocks.findMany).toHaveBeenCalledTimes(1); + }); + + it("swallows a reconcile failure instead of throwing", async () => { + mocks.findMany.mockRejectedValue(new Error("db unavailable")); + + // A background sweep this cheap must never spam pg-boss's + // retry/backoff machinery — it just re-runs on the next 15-minute + // tick. The handler must resolve even when the underlying reconcile + // pass throws. + await expect( + handleImportJobReconcileTick([] as never), + ).resolves.toBeUndefined(); + }); +}); diff --git a/src/lib/jobs/apple-health-import-worker.ts b/src/lib/jobs/apple-health-import-worker.ts index 662b144c2..9bf197017 100644 --- a/src/lib/jobs/apple-health-import-worker.ts +++ b/src/lib/jobs/apple-health-import-worker.ts @@ -37,6 +37,55 @@ import { type ImportJobResult, } from "@/lib/measurements/import-apple-health-export"; import { recomputeUserRollups } from "@/lib/rollups/measurement-rollups"; +import { withBackgroundEvent } from "@/lib/logging/background"; + +/** + * Queue + cron for the periodic orphan-ImportJob sweep. v1.32.1 + * (issue #588) — `reconcileOrphanImportJobs()` used to run only once, + * at worker boot. `restart: unless-stopped` in `docker-compose.yml` + * DOES bring a crashed/OOM-killed worker back up, so the boot-time + * pass usually catches a dead run — but only when that restart lands + * AFTER the row's heartbeat has already gone stale (30 minutes) or + * pg-boss no longer reports the backing job as live. A fast restart + * lands neither condition true yet, and because the pass runs + * exactly once per boot, a worker that then stays up and healthy + * NEVER re-evaluates that row — it is stuck in `unpacking` / + * `parsing` / `upserting` with "0 rows imported" forever and no + * failure ever surfaced to the status poll. A 15-minute cron closes + * that gap: within two ticks of the heartbeat going stale, the same + * idempotent reconcile flips the row to `failed` with an honest + * reason, so the UI's "Unpacking the archive…" spinner eventually + * turns into a visible, actionable error instead of an indefinite + * silent stall. + */ +export const IMPORT_JOB_RECONCILE_QUEUE = "apple-health-import-reconcile"; +export const IMPORT_JOB_RECONCILE_CRON = "*/15 * * * *"; + +/** + * Cron handler for the periodic orphan-ImportJob sweep. Delegates to + * the same `reconcileOrphanImportJobs()` the boot path calls — the + * heartbeat + live pg-boss-job check already make it safe to re-run + * on a healthy worker (a live import's fresh heartbeat and `active` + * pg-boss state both keep its row untouched). Errors are logged, not + * rethrown — a reconcile miss must never spam pg-boss's retry/backoff + * machinery for a background sweep this cheap to just re-run in 15 + * minutes. + */ +export async function handleImportJobReconcileTick( + jobs: Job[], +): Promise { + void jobs; + await withBackgroundEvent( + "job.apple_health_import_reconcile", + async (evt) => { + try { + await reconcileOrphanImportJobs(); + } catch (err) { + evt.addWarning(`apple-health-import-reconcile failed: ${err}`); + } + }, + ); +} /** Queue name isolated from revision-1 workers during rolling deployments. */ export const APPLE_HEALTH_IMPORT_V2_QUEUE = "apple-health-import-v2"; @@ -260,7 +309,7 @@ export async function handleAppleHealthImport( elapsedMs: 0, }); - const unzip = extractExportXml(uploadPath); + const unzip = await extractExportXml(uploadPath); extractedXmlPath = unzip.xmlPath; // Phase 2 + 3: parsing + upserting (the parser tracks both diff --git a/src/lib/jobs/reminder/register-maintenance.ts b/src/lib/jobs/reminder/register-maintenance.ts index 198784ad9..7008e2e90 100644 --- a/src/lib/jobs/reminder/register-maintenance.ts +++ b/src/lib/jobs/reminder/register-maintenance.ts @@ -41,7 +41,10 @@ import { APPLE_HEALTH_IMPORT_V2_QUEUE, APPLE_HEALTH_IMPORT_LEGACY_QUEUE, APPLE_HEALTH_IMPORT_CONCURRENCY, + IMPORT_JOB_RECONCILE_QUEUE, + IMPORT_JOB_RECONCILE_CRON, handleAppleHealthImport, + handleImportJobReconcileTick, migrateLegacyAppleHealthImport, type AppleHealthImportPayload, } from "@/lib/jobs/apple-health-import-worker"; @@ -314,6 +317,11 @@ const allQueues = [ INTAKE_AUTO_SKIP_QUEUE, APPLE_HEALTH_IMPORT_V2_QUEUE, APPLE_HEALTH_IMPORT_LEGACY_QUEUE, + // v1.32.1 (issue #588) — periodic sweep for ImportJob rows orphaned by a + // worker crash/restart mid-run. Without this entry the 15-minute cron + // below silently no-ops and a stuck "unpacking"/"parsing"/"upserting" row + // that survives past the next worker boot is never revisited. + IMPORT_JOB_RECONCILE_QUEUE, MEDICATION_INTAKE_IMPORT_QUEUE, // v1.8.2 — one-time duplicate dose-slot cleanup. Boot discovery enqueues // one job per user holding two live intake rows that snap to the same @@ -462,6 +470,13 @@ const schedules: ScheduleEntry[] = [ // Document vault — daily 04:10 Europe/Berlin purge for tombstoned // documents past the 30-day undo grace. [DOCUMENT_PURGE_QUEUE, DOCUMENT_PURGE_CRON], + // v1.32.1 (issue #588) — every-15-minute orphan-ImportJob sweep. Re-runs + // the same reconcile the boot path uses, so a stuck "unpacking" row + // whose worker crashed/restarted without the boot-time pass catching it + // (heartbeat not yet stale, pg-boss job not yet expired) still converges + // to a visible `failed` state within two ticks instead of staying stuck + // forever on an otherwise-healthy worker. + [IMPORT_JOB_RECONCILE_QUEUE, IMPORT_JOB_RECONCILE_CRON], ]; /** @@ -706,6 +721,14 @@ export async function registerMaintenanceQueues( } }, ); + // v1.32.1 (issue #588) — periodic orphan-ImportJob sweep. Single-flight: + // the underlying `updateMany` is idempotent and two ticks racing the same + // read-then-write pass would just be wasted work. + await boss.work( + IMPORT_JOB_RECONCILE_QUEUE, + { localConcurrency: 1 }, + handleImportJobReconcileTick, + ); await boss.work( MEDICATION_INTAKE_IMPORT_QUEUE, { diff --git a/src/lib/multipart/__tests__/stream-to-disk.test.ts b/src/lib/multipart/__tests__/stream-to-disk.test.ts index 890b46319..42f8329c3 100644 --- a/src/lib/multipart/__tests__/stream-to-disk.test.ts +++ b/src/lib/multipart/__tests__/stream-to-disk.test.ts @@ -431,7 +431,7 @@ describe("streamMultipartToDisk — large multi-chunk binary round-trip", () => expect(written.equals(zip), `zip byte-exact for ${sizes}`).toBe(true); // The load-bearing assertion: the saved file is still a parseable // archive (EOCD intact) and the member round-trips. - const extracted = extractExportXml(result.filePath); + const extracted = await extractExportXml(result.filePath); xmlPath = extracted.xmlPath; expect(readFileSync(extracted.xmlPath).equals(xmlBuf)).toBe(true); } finally {