diff --git a/docs/CLAIMS_TIMEZONE_HANDLING.md b/docs/CLAIMS_TIMEZONE_HANDLING.md deleted file mode 100644 index b8eadbb..0000000 --- a/docs/CLAIMS_TIMEZONE_HANDLING.md +++ /dev/null @@ -1,52 +0,0 @@ -# Claims Module Timezone Handling (#756) - -## Problem - -Claim processing compared the current time against the policy's -`endTime` using `new Date()`. A JS `Date` stores its value as a UTC -epoch (so the comparison against Prisma's UTC-stored `endTime` is -already timezone-correct at the instant level), but the intent was not -explicit and the code read as if the server's local timezone were -involved. That made the boundary hard to audit and left room for -off-by-one-day errors to be introduced by future edits that construct -dates from local-time strings. - -## Fix - -All date construction in the claims module now goes through the UTC -helpers in `src/common/utils/time.utils.ts`: - -- `utcNow()` — current instant, named to make the UTC basis explicit. -- `utcNowMs()` — current epoch milliseconds (for numeric comparisons). -- `utcPlusMs(now, ms)` — offset a UTC instant (used by the worker's - 1-hour lookahead window). - -Comparisons are always performed on epoch milliseconds -(`Date.prototype.getTime()` / `Date.now()`), which are -timezone-independent. No claim-window comparison constructs a date -from a local-time string. - -## Audit of date usage in the claims module - -| Location | Operand | Source | Timezone basis | -|---|---|---|---| -| `claims.service.ts` — `autoProcess` expired check | `policy.endTime` | Prisma (UTC) | Compared against `utcNow()` as epoch ms | -| `claims.service.ts` — `autoProcess` pending-window check | `policy.endTime` | Prisma (UTC) | Compared against `utcNow()` as epoch ms | -| `claims.service.ts` — `submitClaim` expired check | `policy.endTime` | Prisma (UTC) | Compared against `utcNow()` as epoch ms | -| `claims.service.ts` — `recoverStuckProcessingPolicies` cutoff | `policy.updatedAt` | Prisma (UTC) | Derived from `utcNow()` default param as epoch ms | -| `claims.worker.ts` — lookahead window | `policy.endTime` | Prisma (UTC) | `utcNow()` + `utcPlusMs(now, 1h)` | -| `policy-state-cache.service.ts` — `getDerivedPolicyState` | `policy.startTime` / `endTime` | Prisma (UTC) | Epoch ms via `getTime()`; default `now` is current instant | -| `claims.service.ts` — `processedAt` writes | `new Date()` (now) | Server clock | Written as UTC instants (Prisma stores `DateTime` as UTC) | - -No comparison in the claims module relies on the server's local -timezone. Timestamps written to `processedAt` / `submittedAt` are -UTC instants; Prisma materialises `DateTime` columns as UTC `Date` -objects and stores them as UTC. - -## Tests - -- `src/common/utils/time.utils.spec.ts` covers the helpers. -- `src/claims/claims.service.spec.ts` includes a `UTC date comparisons` - block that pins the behaviour at a UTC midnight boundary and confirms - the expired/pending outcomes are consistent regardless of how the - instant is described, including a simulated non-UTC host timezone. diff --git a/src/claims/claims.service.spec.ts b/src/claims/claims.service.spec.ts index 9011142..1169561 100644 --- a/src/claims/claims.service.spec.ts +++ b/src/claims/claims.service.spec.ts @@ -9,7 +9,6 @@ import { PrismaService } from '../prisma/prisma.service'; import { StatusEventsService } from '../common/events/status-events.service'; import { WebhooksService } from '../common/events/webhooks.service'; import { Prisma } from '@prisma/client'; -import { ErrorCode } from '../common/errors/error-codes'; describe('ClaimsService', () => { let service: ClaimsService; @@ -970,144 +969,4 @@ describe('ClaimsService', () => { expect(summary).toEqual(expect.objectContaining({ scanned: 2, revertedToActive: 1, skipped: 1 })); }); }); - - // #756 — date comparisons in claim processing must use UTC against the - // UTC-stored policy.endTime so a server in a non-UTC timezone can't shift - // the coverage boundary by up to a day. These tests pin the comparison at - // a UTC midnight boundary and confirm the outcome is the same regardless - // of how the instant is described. - describe('UTC date comparisons (#756)', () => { - const originalGetTimezoneOffset = Date.prototype.getTimezoneOffset; - - afterEach(() => { - // Restore the real timezone offset in case a test mocked it. - Date.prototype.getTimezoneOffset = originalGetTimezoneOffset; - }); - - // helper: build an active policy whose endTime is a UTC midnight boundary. - const policyEndingAt = (iso: string) => ({ - ...ACTIVE_POLICY, - status: 'ACTIVE' as const, - endTime: new Date(iso), - }); - - it('autoProcess returns Expired exactly at a UTC midnight boundary, not a day early', async () => { - // endTime is 2026-06-01T00:00:00.000Z. The instant one millisecond - // before it must be PendingFinalPeriod; exactly at it must be Expired. - // Simulate a server whose local clock is 5 hours behind UTC (UTC-5), - // so a naive local-time comparison would see "2026-05-31T19:00:00" - // and wrongly treat the policy as still active. - Date.prototype.getTimezoneOffset = () => 300; // UTC-5 returns +300 - const base = new Date('2026-06-01T00:00:00.000Z'); - - mockPrismaService.policy.findUnique.mockResolvedValue(policyEndingAt('2026-06-01T00:00:00.000Z')); - - // Force the comparison instant: temporarily override Date constructor - // behaviour by passing a policy whose endTime equals `base`. The - // service calls utcNow() internally; we assert the boundary using the - // recoverStuckProcessingPolicies path below, which accepts `now` - // explicitly, to avoid patching the global clock. - const summary = await service.recoverStuckProcessingPolicies(base); - // Not expired path — just confirm the helper doesn't throw and the - // comparison is consistent. The boundary semantics are asserted in the - // explicit-instant tests below. - expect(summary).toBeDefined(); - }); - - it('recoverStuckProcessingPolicies uses UTC for the cutoff, not local time', async () => { - // A server at UTC-5 with a wall-clock of 2026-09-24T07:00:00 local - // has a UTC instant of 2026-09-24T12:00:00. The 30-minute cutoff must - // therefore be 2026-09-24T11:30:00 UTC, not 2026-09-24T06:30:00 - // local (which would be 2026-09-24T11:30:00 UTC anyway because the - // cutoff is derived from the epoch). This test pins the expectation. - const nowUtc = new Date('2026-09-24T12:00:00.000Z'); - mockPrismaService.policy.findMany.mockResolvedValue([]); - - await service.recoverStuckProcessingPolicies(nowUtc); - - expect(mockPrismaService.policy.findMany).toHaveBeenCalledWith(expect.objectContaining({ - where: { status: 'PROCESSING', updatedAt: { lt: new Date('2026-09-24T11:30:00.000Z') } }, - })); - }); - - it('autoProcess: a policy whose endTime is exactly now is Expired (UTC boundary, no off-by-one)', async () => { - // Build a policy whose endTime is exactly `now`. autoProcess should - // return Expired (not PendingFinalPeriod), proving the comparison is - // `now > endTime` using the UTC instant rather than a local-day offset. - const endTime = new Date('2026-06-01T00:00:00.000Z'); - mockPrismaService.policy.findUnique.mockResolvedValue({ - ...ACTIVE_POLICY, - status: 'ACTIVE', - endTime, - }); - - // Override utcNow via the Date prototype so the internal utcNow() - // returns exactly `endTime`. We do this by making `new Date()` return - // a fixed instant. - const realDate = Date; - const fixedMs = endTime.getTime(); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (global as any).Date = class extends realDate { - constructor(...args: unknown[]) { - if (args.length === 0) { - super(fixedMs); - } else { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - super(...(args as any)); - } - } - static now() { - return fixedMs; - } - }; - - try { - const result = await service.autoProcess(POLICY_ID); - // endTime == now → not strictly greater than endTime for the Expired - // check, so the next guard (now < endTime) is also false, and the - // flow proceeds to the reservation step. This is the documented - // behaviour: the window is inclusive of endTime. What we're pinning - // is that the comparison is consistent and timezone-independent. - expect(result).not.toBe('PendingFinalPeriod'); - } finally { - (global as any).Date = realDate; - } - }); - - it('submitClaim rejects with CLAIM_POLICY_EXPIRED when endTime has passed in UTC', async () => { - // endTime one second before the fixed now → expired in any timezone. - const now = new Date('2026-06-01T00:00:01.000Z'); - const endTime = new Date('2026-06-01T00:00:00.000Z'); - mockPrismaService.policy.findUnique.mockResolvedValue({ - ...ACTIVE_POLICY, - status: 'ACTIVE', - endTime, - }); - - const realDate = Date; - const fixedMs = now.getTime(); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (global as any).Date = class extends realDate { - constructor(...args: unknown[]) { - if (args.length === 0) { - super(fixedMs); - } else { - // eslint-disable-next-line @typescript-eslint/no-explicit-any - super(...(args as any)); - } - } - static now() { - return fixedMs; - } - }; - - try { - await expect(service.submitClaim(CLAIMANT, POLICY_ID)).rejects.toMatchObject({ - response: expect.objectContaining({ errorCode: ErrorCode.CLAIM_POLICY_EXPIRED }), - }); - } finally { - (global as any).Date = realDate; - } - }); - }); }); diff --git a/src/claims/claims.service.ts b/src/claims/claims.service.ts index 1f50518..d4c5bac 100644 --- a/src/claims/claims.service.ts +++ b/src/claims/claims.service.ts @@ -13,10 +13,6 @@ import { ErrorCode } from '../common/errors/error-codes'; import { compareScaledToThreshold, parseDecimalScaled, parseFixedPointBigInt } from '../common/utils/decimal'; import { financialBatch, financialTransaction, isSerializationFailure } from '../prisma/financial-transaction'; import { PolicyStateCacheService, DerivedPolicyState } from './policy-state-cache.service'; -// #756 — UTC helpers centralise date construction so claim-window -// comparisons against Prisma's UTC-stored timestamps are auditable and -// never accidentally rely on server-local timezone behaviour. -import { utcNow } from '../common/utils/time.utils'; export type ClaimResult = 'Paid' | 'Rejected' | 'Expired' | 'AlreadyClaimed' | 'AlreadyProcessed' | 'PolicyNotActive' | 'PendingFinalPeriod'; @@ -127,14 +123,7 @@ export class ClaimsService { // Expired check — policy.endTime is the authoritative reference, not the // lazily-updated status column (the EXPIRED cron runs at best once an hour). - // #756 — `now` is constructed via utcNow() so the comparison is explicit - // about its UTC basis; policy.endTime is materialised by Prisma as UTC, - // and both operands are compared as epoch milliseconds, which are - // timezone-independent. Using utcNow() here keeps server-local timezone - // out of the comparison and prevents off-by-one-day errors when the - // server's local clock is in a non-UTC zone. - const now = utcNow(); - if (now > policy.endTime) { + if (new Date() > policy.endTime) { this.logger.warn(`Policy ${policyId} coverage period ended at ${policy.endTime.toISOString()}`); return 'Expired'; } @@ -147,7 +136,7 @@ export class ClaimsService { // touching any state until endTime has actually passed — the next hourly // tick will pick this policy back up (it's still ACTIVE with the same // endTime) once the full period's data is available. - if (now < policy.endTime) { + if (new Date() < policy.endTime) { this.logger.log( `Policy ${policyId} endTime (${policy.endTime.toISOString()}) not yet reached — deferring trigger evaluation`, ); @@ -523,7 +512,7 @@ export class ClaimsService { * Every write is guarded on status = PROCESSING and the same updatedAt * cutoff, so a policy that makes progress concurrently is left alone. */ - async recoverStuckProcessingPolicies(now: Date = utcNow()): Promise { + async recoverStuckProcessingPolicies(now: Date = new Date()): Promise { const configured = parseInt(this.config.get('CLAIMS_STUCK_PROCESSING_MINUTES') ?? '', 10); const stuckMinutes = Number.isFinite(configured) && configured > 0 ? configured : DEFAULT_STUCK_PROCESSING_MINUTES; const cutoff = new Date(now.getTime() - stuckMinutes * 60 * 1000); @@ -667,10 +656,7 @@ export class ClaimsService { // Expired check — policy.endTime is the authoritative reference, not the // lazily-updated status column (the EXPIRED cron runs at best once an hour). - // #756 — comparison uses utcNow() against the UTC-stored policy.endTime so - // a server running in a non-UTC timezone can't push a claim past the - // coverage boundary a day early or late. - if (utcNow() > policy.endTime) { + if (new Date() > policy.endTime) { throw new ConflictException({ message: `Policy ${policyId} coverage period ended at ${policy.endTime.toISOString()}`, errorCode: ErrorCode.CLAIM_POLICY_EXPIRED, diff --git a/src/claims/claims.worker.ts b/src/claims/claims.worker.ts index da7e4eb..64a55ab 100644 --- a/src/claims/claims.worker.ts +++ b/src/claims/claims.worker.ts @@ -10,8 +10,6 @@ import { transition } from '../policy/policy-status.machine'; import { StatusEventsService } from '../common/events/status-events.service'; import { recordWorkerHeartbeat } from '../common/worker-heartbeat'; import { DistributedLockService } from '../redis/distributed-lock.service'; -// #756 — UTC helpers make the timezone basis of date comparisons explicit. -import { utcNow, utcPlusMs } from '../common/utils/time.utils'; const BATCH_SIZE = 10; const PRODUCTS_PAGE_SIZE = 100; @@ -108,12 +106,8 @@ export class ClaimsWorker { this.logger.error('Stuck PROCESSING recovery failed', err); } - // #756 — `now` and `oneHourOut` are constructed via utcNow()/utcPlusMs() - // so the lookahead window is anchored to the same UTC basis as the - // UTC-stored policy.endTime; without this a server in a non-UTC zone - // could shift the scan window by up to a day. - const now = utcNow(); - const oneHourOut = utcPlusMs(now, 60 * 60 * 1000); + const now = new Date(); + const oneHourOut = new Date(now.getTime() + 60 * 60 * 1000); const expiringPolicies = await this.prisma.policy.findMany({ where: { diff --git a/src/claims/policy-state-cache.service.ts b/src/claims/policy-state-cache.service.ts index cc0ce72..aa478c0 100644 --- a/src/claims/policy-state-cache.service.ts +++ b/src/claims/policy-state-cache.service.ts @@ -206,11 +206,6 @@ export class PolicyStateCacheService implements OnModuleInit, OnModuleDestroy { * Never caches failed computations. */ async getDerivedPolicyState(policyId: string, now: Date = new Date()): Promise { - // #756 — default `now` is the current instant; comparisons below use - // epoch milliseconds (getTime / Date.now), which are timezone- - // independent, so the expiry evaluation is correct regardless of the - // server's local timezone. Callers may pass an explicit UTC Date for - // deterministic testing. const nowMs = now.getTime(); // 1. Check in-flight computation (stampede protection) diff --git a/src/common/telemetry/opentelemetry.spec.ts b/src/common/telemetry/opentelemetry.spec.ts index eb096f1..34e5f41 100644 --- a/src/common/telemetry/opentelemetry.spec.ts +++ b/src/common/telemetry/opentelemetry.spec.ts @@ -4,7 +4,6 @@ import { _resetOpenTelemetryState, registerOtelSignalHandlers, OpenTelemetryShutdownHook, - parseIgnoreUrls, } from './opentelemetry'; describe('OpenTelemetry Graceful Shutdown (#715)', () => { @@ -139,47 +138,3 @@ describe('OpenTelemetry Graceful Shutdown (#715)', () => { expect(mockSdk.shutdown).toHaveBeenCalled(); }); }); - -// #754 — health check endpoints must not generate OpenTelemetry spans so -// load-balancer probes don't drown real request traces in noise. -describe('OpenTelemetry health probe exclusion (#754)', () => { - const originalEnv = process.env; - - beforeEach(() => { - process.env = { ...originalEnv }; - delete process.env.OTEL_HTTP_INSTRUMENTATION_IGNORE_URLS; - }); - - afterEach(() => { - process.env = originalEnv; - }); - - it('ignores the default health probe paths', () => { - const urls = parseIgnoreUrls(); - expect(urls).toContain('/api/v1/health'); - expect(urls).toContain('/health'); - }); - - it('merges additional patterns from OTEL_HTTP_INSTRUMENTATION_IGNORE_URLS without losing defaults', () => { - process.env.OTEL_HTTP_INSTRUMENTATION_IGNORE_URLS = '/ready,/live'; - const urls = parseIgnoreUrls(); - expect(urls).toContain('/api/v1/health'); - expect(urls).toContain('/health'); - expect(urls).toContain('/ready'); - expect(urls).toContain('/live'); - }); - - it('dedupes overlapping patterns', () => { - process.env.OTEL_HTTP_INSTRUMENTATION_IGNORE_URLS = '/api/v1/health,/metrics'; - const urls = parseIgnoreUrls(); - expect(urls.filter((u) => u === '/api/v1/health')).toHaveLength(1); - expect(urls).toContain('/metrics'); - }); - - it('always excludes the built-in health endpoints even when the env var is empty', () => { - process.env.OTEL_HTTP_INSTRUMENTATION_IGNORE_URLS = ''; - const urls = parseIgnoreUrls(); - expect(urls).toContain('/api/v1/health'); - expect(urls).toContain('/health'); - }); -}); diff --git a/src/common/telemetry/opentelemetry.ts b/src/common/telemetry/opentelemetry.ts index 1b531d4..44108eb 100644 --- a/src/common/telemetry/opentelemetry.ts +++ b/src/common/telemetry/opentelemetry.ts @@ -2,34 +2,6 @@ import { Logger, Injectable, BeforeApplicationShutdown } from '@nestjs/common'; export const DEFAULT_OTEL_SHUTDOWN_TIMEOUT_MS = 5000; -// #754 — health probes are polled every few seconds by load balancers and -// orchestrators. Each probe would otherwise produce a span and flood the -// trace backend with noise that drowns out real request traces. The -// default ignore list covers the global prefix (/api/v1/health) and the -// bare controller path (/health) so both routed and unrouted probes are -// excluded. Operators can extend this via -// OTEL_HTTP_INSTRUMENTATION_IGNORE_URLS (comma-separated URL patterns). -const DEFAULT_HEALTH_PROBE_IGNORE_URLS = ['/api/v1/health', '/health']; - -/** - * #754 — resolve the HTTP instrumentation ignore list. - * - * The configured list always includes the default health probe paths so - * deployments that only set OTEL_HTTP_INSTRUMENTATION_IGNORE_URLS for - * additional probes still exclude the built-in health endpoints. The env - * override is merged in rather than replacing the defaults, so an operator - * can never accidentally re-enable tracing on health checks. - */ -export function parseIgnoreUrls(env: NodeJS.ProcessEnv = process.env): string[] { - const configured = env['OTEL_HTTP_INSTRUMENTATION_IGNORE_URLS']; - if (!configured) return [...DEFAULT_HEALTH_PROBE_IGNORE_URLS]; - const extra = configured - .split(',') - .map((url) => url.trim()) - .filter((url) => url.length > 0); - return [...new Set([...DEFAULT_HEALTH_PROBE_IGNORE_URLS, ...extra])]; -} - interface OpenTelemetrySdk { start: () => Promise; shutdown: () => Promise; @@ -76,20 +48,7 @@ export async function initializeOpenTelemetry(): Promise { : undefined, }) : undefined, - // #754 — health check endpoints are polled every few seconds by load - // balancers and orchestrators. Without an ignore list every probe - // produced a span, drowning real request traces in noise. The HTTP - // instrumentation is configured to skip spans for /api/v1/health and - // /health so probes stop generating traces. The list is overridable - // via OTEL_HTTP_INSTRUMENTATION_IGNORE_URLS (comma-separated) for - // deployments that also expose other probe paths. - instrumentations: [ - getNodeAutoInstrumentations({ - '@opentelemetry/instrumentation-http': { - ignoreUrls: parseIgnoreUrls(), - }, - }), - ], + instrumentations: [getNodeAutoInstrumentations()], }); await sdk.start(); diff --git a/src/common/utils/time.utils.spec.ts b/src/common/utils/time.utils.spec.ts deleted file mode 100644 index 6e07afa..0000000 --- a/src/common/utils/time.utils.spec.ts +++ /dev/null @@ -1,47 +0,0 @@ -import { utcNow, utcNowMs, utcPlusMs } from './time.utils'; - -// #756 — the UTC helpers exist to make the timezone basis of date -// comparisons explicit and auditable across the claims module. -describe('UTC time helpers (#756)', () => { - it('utcNow returns a Date within a millisecond of Date.now', () => { - const before = Date.now(); - const now = utcNow(); - const after = Date.now(); - expect(now.getTime()).toBeGreaterThanOrEqual(before); - expect(now.getTime()).toBeLessThanOrEqual(after); - }); - - it('utcNowMs matches Date.now within tolerance', () => { - const before = Date.now(); - const ms = utcNowMs(); - const after = Date.now(); - expect(ms).toBeGreaterThanOrEqual(before); - expect(ms).toBeLessThanOrEqual(after); - }); - - it('utcPlusMs offsets a UTC instant by the given milliseconds', () => { - const base = new Date('2026-06-01T00:00:00.000Z'); - const oneHourLater = utcPlusMs(base, 60 * 60 * 1000); - expect(oneHourLater.toISOString()).toBe('2026-06-01T01:00:00.000Z'); - - const oneHourEarlier = utcPlusMs(base, -60 * 60 * 1000); - expect(oneHourEarlier.toISOString()).toBe('2026-05-31T23:00:00.000Z'); - }); - - it('utcPlusMs does not mutate the input Date', () => { - const base = new Date('2026-06-01T00:00:00.000Z'); - utcPlusMs(base, 1000); - expect(base.toISOString()).toBe('2026-06-01T00:00:00.000Z'); - }); - - it('utcNow is timezone-independent: same epoch regardless of host TZ', () => { - // The epoch ms of utcNow must not depend on the server's local timezone. - // We can't change process TZ in a unit test, but we can assert the - // returned Date's getTime() is a plain epoch (no offset applied). - const now = utcNow(); - expect(typeof now.getTime()).toBe('number'); - expect(now.getTime()).toBeGreaterThan(0); - // A Date constructed from the same epoch must be equal. - expect(new Date(now.getTime()).getTime()).toBe(now.getTime()); - }); -}); diff --git a/src/common/utils/time.utils.ts b/src/common/utils/time.utils.ts deleted file mode 100644 index 7aa8f44..0000000 --- a/src/common/utils/time.utils.ts +++ /dev/null @@ -1,52 +0,0 @@ -/** - * Time helpers for the claims/policy modules. - * - * #756 — date comparisons in claim processing previously used `new Date()` - * directly. While a JS `Date` stores its value as a UTC epoch (so - * comparisons against `policy.endTime` — which Prisma reads back as UTC — - * are already timezone-correct at the instant level), the intent was not - * explicit and the code read as if local time were involved. Centralising - * UTC date construction here makes the contract obvious, gives every - * comparison a single auditable place to inspect, and keeps timestamps - * written to `processedAt`/`submittedAt` derived from the same UTC source - * as the comparison operands. - * - * All comparisons in the claims module go through epoch milliseconds - * (Date.prototype.getTime / Date.now), which are timezone-independent. - * The helpers below exist for readability and auditability, not to - * correct a runtime offset. - */ - -/** - * Returns a Date representing the current instant in UTC. - * - * Equivalent to `new Date()` but named to make the UTC intent explicit at - * every call site. Prefer this in modules that compare against DB-stored - * timestamps (which Prisma materialises as UTC) so reviewers don't have to - * reason about server-local timezone behaviour. - */ -export function utcNow(): Date { - return new Date(); -} - -/** - * Returns the current UTC epoch milliseconds. - * - * Use this when only the numeric comparison is needed; it avoids - * allocating a Date object on every call and makes the UTC basis - * unambiguous. - */ -export function utcNowMs(): number { - return Date.now(); -} - -/** - * Returns a Date offset from a UTC instant by a number of milliseconds. - * - * Mirrors the `new Date(now.getTime() + ms)` pattern used by the worker - * (e.g. the 1-hour lookahead window) while keeping the UTC origin - * explicit. - */ -export function utcPlusMs(now: Date, ms: number): Date { - return new Date(now.getTime() + ms); -}