refactor(server): move weather out of services/, give its cache sweep a lifecycle (#1835)

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

@@ -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,

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';

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).

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

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<WeatherResult> {
     return getWeather(lat, lng, date, lang) as Promise<WeatherResult>;
   }

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<typeof import('../../src/services/weatherService')>();
+vi.mock('../../src/nest/weather/weather.impl', async (importActual) => {
+  const actual = await importActual<typeof import('../../src/nest/weather/weather.impl')>();
   return { ...actual, getWeather: mockGet, getDetailedWeather: mockGetDetailed };
 });
 

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: [] }),
 }));

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<WeatherService>) {

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);
+  });
+});

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);
+  });
+});

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' }]),