From 028e3e0a846884dd3874dfe5d6acf492c0a175a9 Mon Sep 17 00:00:00 2001 From: jubnl <66769052+jubnl@users.noreply.github.com> Date: Mon, 15 Jun 2026 09:32:42 +0200 Subject: [PATCH] fix(server): lengthen idempotency key TTL to survive multi-day offline (H6) (#1182) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nightly cleanup deleted idempotency keys older than 24h. The TREK client replays queued mutations with their X-Idempotency-Key on reconnect, so a device offline longer than a day had its keys GC'd before it returned — the replayed POST was then treated as new and created a duplicate. - raise the TTL to 30 days (DEFAULT_IDEMPOTENCY_TTL_SECONDS), overridable via IDEMPOTENCY_TTL_SECONDS - extract purgeExpiredIdempotencyKeys(now, ttl, db) (mirrors cleanupOldBackups) with an injectable db, and have the cron job call it - tests: 30-day default eviction, 25-day key retained (was dropped at 24h), env override H7 (exactly-once across the lost-response window) is deferred: a correct fix must store the response in the same DB transaction as the entity write. Doing it in the generic interceptor (reserve-before-handler) cannot store the real response body for the crash case, which would break the client's temp->real id remapping on replay (mutationQueue.flush relies on the entity in the body). It needs a per-service change and is tracked separately. --- server/src/scheduler.ts | 39 ++++++++++--- server/tests/unit/idempotency-cleanup.test.ts | 58 +++++++++++++++++++ 2 files changed, 90 insertions(+), 7 deletions(-) create mode 100644 server/tests/unit/idempotency-cleanup.test.ts diff --git a/server/src/scheduler.ts b/server/src/scheduler.ts index 5884c03e..b9f7fb87 100644 --- a/server/src/scheduler.ts +++ b/server/src/scheduler.ts @@ -291,20 +291,45 @@ function startVersionCheck(): void { }, { timezone: tz }); } -// Idempotency key cleanup: nightly at 3 AM — delete keys older than 24 hours +// Idempotency key cleanup: nightly at 3 AM — delete keys past their TTL. +// The TTL must exceed any realistic offline window: the TREK client replays +// queued mutations with their X-Idempotency-Key when it reconnects, so a key +// GC'd before the device comes back online would let the replay create a +// duplicate. 24h was far too short for a multi-day offline trip; default 30d, +// overridable via IDEMPOTENCY_TTL_SECONDS. +const DEFAULT_IDEMPOTENCY_TTL_SECONDS = 30 * 24 * 60 * 60; // 30 days let idempotencyCleanupTask: ScheduledTask | null = null; +function idempotencyTtlSeconds(): number { + const n = Number(process.env.IDEMPOTENCY_TTL_SECONDS); + return Number.isFinite(n) && n > 0 ? n : DEFAULT_IDEMPOTENCY_TTL_SECONDS; +} + +interface PurgeDb { + prepare(sql: string): { run(...args: unknown[]): { changes: number } }; +} + +/** Delete idempotency keys older than the configured TTL. Returns rows removed. + * The db is injectable for testing; the cron job uses the default. */ +function purgeExpiredIdempotencyKeys( + now: number = Date.now(), + ttlSeconds: number = idempotencyTtlSeconds(), + database: PurgeDb = require('./db/database').db, +): number { + const cutoff = Math.floor(now / 1000) - ttlSeconds; + const result = database.prepare('DELETE FROM idempotency_keys WHERE created_at < ?').run(cutoff); + return result.changes; +} + function startIdempotencyCleanup(): void { if (idempotencyCleanupTask) { idempotencyCleanupTask.stop(); idempotencyCleanupTask = null; } const tz = process.env.TZ || 'UTC'; idempotencyCleanupTask = cron.schedule('0 3 * * *', () => { try { - const { db } = require('./db/database'); - const cutoff = Math.floor(Date.now() / 1000) - 86400; - const result = db.prepare('DELETE FROM idempotency_keys WHERE created_at < ?').run(cutoff); - if (result.changes > 0) { - logInfo(`Idempotency cleanup: removed ${result.changes} expired key(s)`); + const removed = purgeExpiredIdempotencyKeys(); + if (removed > 0) { + logInfo(`Idempotency cleanup: removed ${removed} expired key(s)`); } } catch (err: unknown) { logError(`Idempotency cleanup: ${err instanceof Error ? err.message : err}`); @@ -394,4 +419,4 @@ function stop(): void { if (airtrailSyncTask) { airtrailSyncTask.stop(); airtrailSyncTask = null; } } -export { start, stop, startDemoReset, startTripReminders, startTodoReminders, startVersionCheck, startIdempotencyCleanup, startTrekPhotoCacheCleanup, startPlacePhotoCacheCleanup, startAirTrailSync, loadSettings, saveSettings, VALID_INTERVALS }; +export { start, stop, startDemoReset, startTripReminders, startTodoReminders, startVersionCheck, startIdempotencyCleanup, purgeExpiredIdempotencyKeys, startTrekPhotoCacheCleanup, startPlacePhotoCacheCleanup, startAirTrailSync, loadSettings, saveSettings, VALID_INTERVALS }; diff --git a/server/tests/unit/idempotency-cleanup.test.ts b/server/tests/unit/idempotency-cleanup.test.ts new file mode 100644 index 00000000..d0b99dd8 --- /dev/null +++ b/server/tests/unit/idempotency-cleanup.test.ts @@ -0,0 +1,58 @@ +/** + * Idempotency key TTL cleanup (H6). + * + * The TREK client replays queued mutations with their X-Idempotency-Key on + * reconnect, so the server must keep keys long enough to cover a realistic + * offline window — otherwise a key GC'd before the device returns lets the + * replay create a duplicate. The TTL was raised from 24h to 30d (overridable). + */ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { db } from '../../src/db/database'; +import { purgeExpiredIdempotencyKeys } from '../../src/scheduler'; + +const DAY = 24 * 60 * 60; +const NOW = 2_000_000_000_000; // fixed ms so the test is deterministic +const NOW_SEC = Math.floor(NOW / 1000); + +function insertKey(key: string, ageSeconds: number): void { + db.prepare( + `INSERT INTO idempotency_keys (key, user_id, method, path, status_code, response_body, created_at) + VALUES (?, 1, 'POST', '/x', 200, '{}', ?)`, + ).run(key, NOW_SEC - ageSeconds); +} + +beforeEach(() => { + db.pragma('foreign_keys = OFF'); // fixtures reference a user we don't seed here + db.prepare('DELETE FROM idempotency_keys').run(); +}); + +afterEach(() => { + db.prepare('DELETE FROM idempotency_keys').run(); + db.pragma('foreign_keys = ON'); + delete process.env.IDEMPOTENCY_TTL_SECONDS; +}); + +describe('purgeExpiredIdempotencyKeys', () => { + it('removes keys older than the 30-day default, keeps recent ones', () => { + insertKey('old', 31 * DAY); + insertKey('fresh', 5 * DAY); + + const removed = purgeExpiredIdempotencyKeys(NOW, undefined, db); + + expect(removed).toBe(1); + const keys = db.prepare('SELECT key FROM idempotency_keys').all().map((r: { key: string }) => r.key); + expect(keys).toEqual(['fresh']); + }); + + it('keeps a 25-day-old key that the old 24h TTL would have dropped', () => { + insertKey('offline-trip', 25 * DAY); + expect(purgeExpiredIdempotencyKeys(NOW, undefined, db)).toBe(0); + expect(db.prepare('SELECT COUNT(*) c FROM idempotency_keys').get()).toMatchObject({ c: 1 }); + }); + + it('respects the IDEMPOTENCY_TTL_SECONDS override', () => { + process.env.IDEMPOTENCY_TTL_SECONDS = String(DAY); + insertKey('twoDays', 2 * DAY); + expect(purgeExpiredIdempotencyKeys(NOW, undefined, db)).toBe(1); + }); +});