From a7d789fdd50ac4ff8df67ae6eac9a031ea6e24d0 Mon Sep 17 00:00:00 2001 From: Maurice <61554723+mauriceboe@users.noreply.github.com> Date: Fri, 7 Aug 2026 21:47:21 +0200 Subject: [PATCH] refactor(server): move weather out of services/, give its cache sweep a lifecycle (#1835) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finishes be-weather-airports-fold. The README has listed weather as the reference implementation since the migration started, while 513 lines of it sat in the legacy layer. The file is nest/weather/weather.impl.ts now, and WeatherService owns what it could not own before: the cache sweep. That sweep was a bare setInterval at module scope, so every process that so much as imported the file started a five-minute timer and nothing ever stopped it — there was nothing to call. It is startCacheCleanup/stopCacheCleanup now, driven by onModuleInit/onModuleDestroy, and the handle is unref-ed so a cache sweep can never hold the process open. The old test file says as much: its second line is "Prevent the module-level setInterval from running during tests", with a vi.useFakeTimers() to do it. That workaround is no longer what keeps the timer quiet. The cache itself stays module state, deliberately. The MCP weather tools call getWeather directly from outside the container, and a cache is only worth having if every caller sees the same one. Hoisting it into the class would mean handing the registrar the container instance to get exactly what a module singleton already provides — the same trade the place-photo cache resolved the other way, because there the scheduler needed the instance anyway. Coverage is the reason this PR carries eleven new tests rather than none. Code in services/ is ungated; the moment it lands in src/nest it counts against the 80% branch threshold, and weather.impl arrived at 75%. The gap was in paths worth having: a failing archive lookup with and without an upstream reason, a far-future date whose climate rows are all null, an expired entry being re- fetched, and the sweep evicting one on its own. Plus the lifecycle itself — start is idempotent, stop is idempotent, the handle is unref-ed. 79% now. services/ is down to 11 files, from 44. --- server/src/mcp/tools/mapsWeather.ts | 2 +- .../plugins/host/plugin-host-deps.factory.ts | 2 +- server/src/nest/weather/weather.controller.ts | 2 +- .../weather/weather.impl.ts} | 29 ++++- server/src/nest/weather/weather.service.ts | 30 +++-- server/tests/e2e/weather.e2e.test.ts | 4 +- .../unit/mcp/tools-tags-maps-weather.test.ts | 2 +- .../unit/nest/weather.controller.test.ts | 2 +- .../weather/weather-cache-lifecycle.test.ts | 110 ++++++++++++++++++ .../weather/weather.impl.test.ts} | 72 +++++++++++- .../plugins/plugin-host-deps.factory.test.ts | 2 +- 11 files changed, 237 insertions(+), 20 deletions(-) rename server/src/{services/weatherService.ts => nest/weather/weather.impl.ts} (95%) create mode 100644 server/tests/unit/nest/weather/weather-cache-lifecycle.test.ts rename server/tests/unit/{services/weatherService.test.ts => nest/weather/weather.impl.test.ts} (90%) diff --git a/server/src/mcp/tools/mapsWeather.ts b/server/src/mcp/tools/mapsWeather.ts index 34219b352..72a5662ed 100644 --- a/server/src/mcp/tools/mapsWeather.ts +++ b/server/src/mcp/tools/mapsWeather.ts @@ -1,7 +1,7 @@ import { McpServer } from '@modelcontextprotocol/sdk/server/mcp'; import { z } from 'zod'; import { findByIata, searchAirports } from '../../nest/airports/airports.data'; -import { getWeather, getDetailedWeather } from '../../services/weatherService'; +import { getWeather, getDetailedWeather } from '../../nest/weather/weather.impl'; import { TOOL_ANNOTATIONS_READONLY, ok, diff --git a/server/src/nest/plugins/host/plugin-host-deps.factory.ts b/server/src/nest/plugins/host/plugin-host-deps.factory.ts index 5ad987a25..724cd8d8a 100644 --- a/server/src/nest/plugins/host/plugin-host-deps.factory.ts +++ b/server/src/nest/plugins/host/plugin-host-deps.factory.ts @@ -3,7 +3,7 @@ import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { readEnv } from '../../../app-config'; import { RealtimeService } from '../../realtime/realtime.service'; import { isUpdateConflict } from '../../common/conflictResult'; -import { getWeather } from '../../../services/weatherService'; +import { getWeather } from '../../weather/weather.impl'; import { BLOCKED_EXTENSIONS, filesDir } from '../../files/files.constants'; import { TripMembershipService } from '../../trip-membership/trip-membership.service'; import { NotificationsService } from '../../notifications/notifications.service'; diff --git a/server/src/nest/weather/weather.controller.ts b/server/src/nest/weather/weather.controller.ts index 447606250..41f9ca6ec 100644 --- a/server/src/nest/weather/weather.controller.ts +++ b/server/src/nest/weather/weather.controller.ts @@ -2,7 +2,7 @@ import { Controller, Get, HttpException, Query, UseGuards } from '@nestjs/common import type { WeatherResult } from '@trek/shared'; import { WeatherService } from './weather.service'; import { JwtAuthGuard } from '../auth/jwt-auth.guard'; -import { ApiError } from '../../services/weatherService'; +import { ApiError } from './weather.impl'; /** * /api/weather — first migrated leaf module (the pilot). diff --git a/server/src/services/weatherService.ts b/server/src/nest/weather/weather.impl.ts similarity index 95% rename from server/src/services/weatherService.ts rename to server/src/nest/weather/weather.impl.ts index 75ea5d5ee..76523b44c 100644 --- a/server/src/services/weatherService.ts +++ b/server/src/nest/weather/weather.impl.ts @@ -100,7 +100,8 @@ const CACHE_MAX_ENTRIES = 1000; const CACHE_PRUNE_TARGET = 500; const CACHE_CLEANUP_INTERVAL = 5 * 60 * 1000; // 5 minutes -setInterval(() => { +/** Drop expired entries, then trim the cache back if it is still over the cap. */ +function pruneCache(): void { const now = Date.now(); for (const [key, entry] of weatherCache) { if (now > entry.expiresAt) weatherCache.delete(key); @@ -110,7 +111,31 @@ setInterval(() => { const toDelete = entries.slice(0, entries.length - CACHE_PRUNE_TARGET); toDelete.forEach(([key]) => weatherCache.delete(key)); } -}, CACHE_CLEANUP_INTERVAL); +} + +let cleanupTimer: NodeJS.Timeout | null = null; + +/** + * Start the cache sweep. WeatherService calls this from onModuleInit. + * + * It used to be a bare setInterval at module scope, which meant every process + * that so much as imported this file — including every test worker — started a + * five-minute timer it never stopped. Nothing cleaned it up because there was + * nothing to call. + */ +export function startCacheCleanup(): void { + if (cleanupTimer) return; + cleanupTimer = setInterval(pruneCache, CACHE_CLEANUP_INTERVAL); + // Never hold the process open for a cache sweep. + cleanupTimer.unref?.(); +} + +/** Stop the sweep (onModuleDestroy). Idempotent. */ +export function stopCacheCleanup(): void { + if (!cleanupTimer) return; + clearInterval(cleanupTimer); + cleanupTimer = null; +} const TTL_FORECAST_MS = 60 * 60 * 1000; // 1 hour const TTL_CURRENT_MS = 15 * 60 * 1000; // 15 minutes diff --git a/server/src/nest/weather/weather.service.ts b/server/src/nest/weather/weather.service.ts index a9319d3af..22f1865cc 100644 --- a/server/src/nest/weather/weather.service.ts +++ b/server/src/nest/weather/weather.service.ts @@ -1,16 +1,30 @@ -import { Injectable } from '@nestjs/common'; +import { Injectable, OnModuleInit, OnModuleDestroy } from '@nestjs/common'; import type { WeatherResult } from '@trek/shared'; -import { getWeather, getDetailedWeather } from '../../services/weatherService'; +import { getWeather, getDetailedWeather, startCacheCleanup, stopCacheCleanup } from './weather.impl'; /** - * Thin Nest wrapper around the existing weather service. It delegates to the - * exact same `getWeather` / `getDetailedWeather` functions the legacy route and - * the MCP tools use, so behaviour — including the shared in-memory cache and the - * Open-Meteo calls — is identical. No logic is duplicated; the upstream service - * stays the single source of truth (still consumed by the MCP weather tools). + * The weather domain's container face, and the owner of the cache sweep's + * lifecycle. + * + * The Open-Meteo calls and the response shaping live in weather.impl.ts, which + * also holds the process-wide cache. That cache stays module state on purpose: + * the MCP weather tools call getWeather directly, from outside the container, + * and a cache is only worth having if every caller sees the same one. Moving it + * into this class would mean handing the registrar the container instance to + * achieve exactly what a module singleton already gives. + * + * What did have to move is the sweep timer — see startCacheCleanup. */ @Injectable() -export class WeatherService { +export class WeatherService implements OnModuleInit, OnModuleDestroy { + onModuleInit(): void { + startCacheCleanup(); + } + + onModuleDestroy(): void { + stopCacheCleanup(); + } + get(lat: string, lng: string, date: string | undefined, lang: string): Promise { return getWeather(lat, lng, date, lang) as Promise; } diff --git a/server/tests/e2e/weather.e2e.test.ts b/server/tests/e2e/weather.e2e.test.ts index fa470857d..7d8cc9769 100644 --- a/server/tests/e2e/weather.e2e.test.ts +++ b/server/tests/e2e/weather.e2e.test.ts @@ -23,8 +23,8 @@ const { db } = vi.hoisted(() => { vi.mock('../../src/db/database', () => ({ db, closeDb: () => {}, reinitialize: () => {} })); const { mockGet, mockGetDetailed } = vi.hoisted(() => ({ mockGet: vi.fn(), mockGetDetailed: vi.fn() })); -vi.mock('../../src/services/weatherService', async (importActual) => { - const actual = await importActual(); +vi.mock('../../src/nest/weather/weather.impl', async (importActual) => { + const actual = await importActual(); return { ...actual, getWeather: mockGet, getDetailedWeather: mockGetDetailed }; }); diff --git a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts index d92aa9de8..df214c250 100644 --- a/server/tests/unit/mcp/tools-tags-maps-weather.test.ts +++ b/server/tests/unit/mcp/tools-tags-maps-weather.test.ts @@ -35,7 +35,7 @@ vi.mock('../../../src/config', () => ({ const { broadcastMock } = vi.hoisted(() => ({ broadcastMock: vi.fn() })); vi.mock('../../../src/websocket', () => ({ broadcast: broadcastMock })); -vi.mock('../../../src/services/weatherService', () => ({ +vi.mock('../../../src/nest/weather/weather.impl', () => ({ getWeather: vi.fn().mockResolvedValue({ temp: 20, condition: 'sunny' }), getDetailedWeather: vi.fn().mockResolvedValue({ hourly: [] }), })); diff --git a/server/tests/unit/nest/weather.controller.test.ts b/server/tests/unit/nest/weather.controller.test.ts index d6dff1f4f..df1c5d21f 100644 --- a/server/tests/unit/nest/weather.controller.test.ts +++ b/server/tests/unit/nest/weather.controller.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi } from 'vitest'; import { HttpException } from '@nestjs/common'; import { WeatherController } from '../../../src/nest/weather/weather.controller'; -import { ApiError } from '../../../src/services/weatherService'; +import { ApiError } from '../../../src/nest/weather/weather.impl'; import type { WeatherService } from '../../../src/nest/weather/weather.service'; function makeController(svc: Partial) { diff --git a/server/tests/unit/nest/weather/weather-cache-lifecycle.test.ts b/server/tests/unit/nest/weather/weather-cache-lifecycle.test.ts new file mode 100644 index 000000000..3305ff659 --- /dev/null +++ b/server/tests/unit/nest/weather/weather-cache-lifecycle.test.ts @@ -0,0 +1,110 @@ +/** + * The weather cache sweep's lifecycle — WEATHER-SWEEP-001..005. + * + * The sweep used to be a bare setInterval at module scope: every process that + * imported weather.impl started a five-minute timer, and nothing could stop it + * because there was nothing to call. It is start/stop now, owned by + * WeatherService's onModuleInit/onModuleDestroy — so it needs tests that the + * old shape could not have had. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +import { startCacheCleanup, stopCacheCleanup, getWeather } from '../../../../src/nest/weather/weather.impl'; +import { WeatherService } from '../../../../src/nest/weather/weather.service'; + +beforeEach(() => { + vi.useFakeTimers(); + stopCacheCleanup(); // start from a known state whatever ran before +}); + +afterEach(() => { + stopCacheCleanup(); + vi.useRealTimers(); +}); + +describe('cache sweep lifecycle', () => { + it('WEATHER-SWEEP-001: start schedules exactly one interval', () => { + const setInterval = vi.spyOn(globalThis, 'setInterval'); + + startCacheCleanup(); + + expect(setInterval).toHaveBeenCalledOnce(); + expect(setInterval.mock.calls[0][1]).toBe(5 * 60 * 1000); + }); + + it('WEATHER-SWEEP-002: starting twice does not stack a second timer', () => { + const setInterval = vi.spyOn(globalThis, 'setInterval'); + + startCacheCleanup(); + startCacheCleanup(); + + expect(setInterval).toHaveBeenCalledOnce(); + }); + + it('WEATHER-SWEEP-003: the timer is unref-ed, so a sweep never holds the process open', () => { + const unref = vi.fn(); + vi.spyOn(globalThis, 'setInterval').mockReturnValue({ unref } as unknown as NodeJS.Timeout); + + startCacheCleanup(); + + expect(unref).toHaveBeenCalledOnce(); + }); + + it('WEATHER-SWEEP-004: stop clears the timer and is idempotent', () => { + startCacheCleanup(); + expect(vi.getTimerCount()).toBeGreaterThan(0); + + stopCacheCleanup(); + expect(vi.getTimerCount()).toBe(0); + + expect(() => stopCacheCleanup()).not.toThrow(); + }); + + it('WEATHER-SWEEP-005: stop then start schedules again', () => { + startCacheCleanup(); + stopCacheCleanup(); + startCacheCleanup(); + + expect(vi.getTimerCount()).toBeGreaterThan(0); + }); +}); + +describe('what the sweep actually does', () => { + it('WEATHER-SWEEP-007: an expired entry is gone after a sweep, without anyone asking for it', async () => { + const date = new Date(Date.now() + 4 * 24 * 60 * 60 * 1000).toISOString().slice(0, 10); + const body = { + daily: { time: [date], temperature_2m_max: [20], temperature_2m_min: [10], weathercode: [0] }, + }; + const fetchMock = vi.fn().mockResolvedValue({ + ok: true, status: 200, json: vi.fn().mockResolvedValue(body), + } as unknown as Response); + vi.stubGlobal('fetch', fetchMock); + + try { + await getWeather('44.44', '8.88', date, 'en'); + expect(fetchMock).toHaveBeenCalledTimes(1); + + startCacheCleanup(); + // Past the forecast TTL (1h) and past one sweep interval (5min), so the + // sweep — not a later read — is what evicts the entry. + vi.advanceTimersByTime(60 * 60 * 1000 + 5 * 60 * 1000 + 1000); + + await getWeather('44.44', '8.88', date, 'en'); + expect(fetchMock).toHaveBeenCalledTimes(2); + } finally { + vi.unstubAllGlobals(); + } + }); +}); + +describe('WeatherService lifecycle hooks', () => { + it('WEATHER-SWEEP-006: onModuleInit starts the sweep and onModuleDestroy stops it', () => { + const svc = new WeatherService(); + + svc.onModuleInit(); + expect(vi.getTimerCount()).toBeGreaterThan(0); + + svc.onModuleDestroy(); + expect(vi.getTimerCount()).toBe(0); + }); +}); diff --git a/server/tests/unit/services/weatherService.test.ts b/server/tests/unit/nest/weather/weather.impl.test.ts similarity index 90% rename from server/tests/unit/services/weatherService.test.ts rename to server/tests/unit/nest/weather/weather.impl.test.ts index 474b9e407..49c696de6 100644 --- a/server/tests/unit/services/weatherService.test.ts +++ b/server/tests/unit/nest/weather/weather.impl.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterAll } from 'vitest'; -// Prevent the module-level setInterval from running during tests +// Fake timers for the cache-expiry cases below. The module no longer starts a +// timer of its own on import — that moved to WeatherService's lifecycle. vi.useFakeTimers(); // Prevent real HTTP requests @@ -15,7 +16,7 @@ import { getDetailedWeather, ApiError, type WeatherResult, -} from '../../../src/services/weatherService'; +} from '../../../../src/nest/weather/weather.impl'; // ── estimateCondition ──────────────────────────────────────────────────────── @@ -717,3 +718,70 @@ describe('getDetailedWeather', () => { }); }); }); + +// ── error paths ────────────────────────────────────────────────────────────── +// +// Three branches the suite never reached. They came into the coverage gate with +// the file when it moved out of services/, and they are the paths a user hits +// when Open-Meteo is having a bad day. + +describe('getWeather error paths', () => { + beforeEach(() => { + vi.mocked(fetch).mockReset(); + }); + + it('WEATHER-ERR-001: a failing archive lookup surfaces as ApiError with the upstream reason', async () => { + const past = dateOffset(-30); + vi.mocked(fetch).mockResolvedValueOnce( + mockResponse({ error: true, reason: 'Archive temporarily unavailable' }, false, 503), + ); + + await expect(getWeather('41.10', '9.10', past, 'en')).rejects.toMatchObject({ + status: 503, + message: 'Archive temporarily unavailable', + }); + }); + + it('WEATHER-ERR-002: a failing archive lookup without a reason falls back to the generic message', async () => { + const past = dateOffset(-31); + vi.mocked(fetch).mockResolvedValueOnce(mockResponse({}, false, 500)); + + await expect(getWeather('41.20', '9.20', past, 'en')).rejects.toBeInstanceOf(ApiError); + }); + + it('WEATHER-ERR-003: a far-future date with no usable climate rows answers no_forecast', async () => { + const farOut = dateOffset(200); + // Climate API responds 200 but every row is null, so nothing can be averaged. + vi.mocked(fetch).mockResolvedValueOnce( + mockResponse({ + daily: { + time: [farOut], + temperature_2m_max: [null], + temperature_2m_min: [null], + precipitation_sum: [null], + }, + }), + ); + + const result = await getWeather('41.30', '9.30', farOut, 'en'); + expect(result.error).toBe('no_forecast'); + expect(result.temp).toBe(0); + }); + + it('WEATHER-ERR-004: an expired cache entry is dropped and the upstream is asked again', async () => { + const date = dateOffset(4); + const body = { + daily: { time: [date], temperature_2m_max: [20], temperature_2m_min: [10], weathercode: [0] }, + }; + vi.mocked(fetch).mockResolvedValue(mockResponse(body)); + + await getWeather('41.40', '9.40', date, 'en'); + expect(fetch).toHaveBeenCalledTimes(1); + + // Forecast entries live an hour; step past it. + vi.advanceTimersByTime(60 * 60 * 1000 + 1000); + + await getWeather('41.40', '9.40', date, 'en'); + expect(fetch).toHaveBeenCalledTimes(2); + }); +}); diff --git a/server/tests/unit/plugins/plugin-host-deps.factory.test.ts b/server/tests/unit/plugins/plugin-host-deps.factory.test.ts index 2a5e5591e..5c678732f 100644 --- a/server/tests/unit/plugins/plugin-host-deps.factory.test.ts +++ b/server/tests/unit/plugins/plugin-host-deps.factory.test.ts @@ -149,7 +149,7 @@ const packingStub = { setBagMembers: vi.fn((_tid: number | string, bagId: string, userIds: number[]) => (Number(bagId) === 404 ? null : userIds.map((u) => ({ user_id: u })))), } as unknown as PackingService; vi.mock('../../../src/nest/common/conflictResult', () => ({ isUpdateConflict: (r: unknown) => !!(r as { conflict?: boolean })?.conflict })); -vi.mock('../../../src/services/weatherService', () => ({ getWeather: vi.fn(async (lat: string, lng: string) => ({ lat, lng, temp: 20 })) })); +vi.mock('../../../src/nest/weather/weather.impl', () => ({ getWeather: vi.fn(async (lat: string, lng: string) => ({ lat, lng, temp: 20 })) })); const categoriesStub = { list: vi.fn(() => [{ id: 1, name: 'Food' }]) } as unknown as CategoriesService; const tagsStub = { list: vi.fn((uid: number) => [{ id: 1, user_id: uid, name: 'work' }]),