fix(health): probe the database instead of reporting it as 'unused' (#376)

README.md

@@ -614,7 +614,7 @@ Returns the overall health of the service and its dependencies.
 | `redis.connected` | `true` when the Redis client is connected |
 | `jobs.price_refresh` | Health of the background price-refresh cron job |
 | `jobs.webhook_retry_worker` | Health of the webhook retry worker |
-| `database` | Reports `configured: true, checked: false, status: "unused"` — no active DB health probe |
+| `database` | Result of the database probe: `configured`, `checked`, `status` (`ok` / `error` / `unavailable`), plus `latency_ms` when probed and `error` when it failed |
 | `price_source_circuits` | Per-source circuit-breaker state (open/closed) |
 
 **Health states:**
@@ -668,7 +668,7 @@ Returns the overall health of the service and its dependencies.
       "total_retries_processed": 91
     }
   },
-  "database": { "configured": true, "checked": false, "status": "unused" },
+  "database": { "configured": true, "checked": true, "status": "ok", "latency_ms": 4 },
   "price_source_circuits": [
     { "source": "coingecko", "open": false, "openUntil": null },
     { "source": "coinmarketcap", "open": false, "openUntil": null }

src/config.js

@@ -231,7 +231,11 @@ if (process.env.NODE_ENV === 'production') {
   });
 }
 
-module.exports.reload = reload;
+// The exported object is what `config` (above) points at, and `reload()`
+// mutates it in place so every consumer that captured `require('../config')`
+// sees the new values. Assign onto it rather than replacing it, otherwise
+// `reload()` would update an object nobody holds a reference to.
+Object.assign(module.exports, {
   nodeEnv: env.NODE_ENV,
   port: env.PORT,
   databaseUrl: env.DATABASE_URL,
@@ -379,4 +383,6 @@ module.exports.reload = reload;
     maxConnections: parseInt(process.env.WS_MAX_CONNECTIONS, 10) || 100,
     maxConnectionsPerIp: parseInt(process.env.WS_MAX_CONNECTIONS_PER_IP, 10) || 5,
   },
-};
+});
+
+module.exports.reload = reload;

src/services/dbHealth.js

@@ -1,25 +1,96 @@
 'use strict';
 
 /**
- * Lightweight database configuration check.
+ * Database health probe (issue #376).
  *
- * The database (added for api_key_audit_logs, see migrations) is not yet on
- * any live request path — nothing in the app queries it at runtime. So
- * `/health` only reports whether a connection string is *configured*
- * (via the same resolved config the migration CLI uses, including its
- * dev/test defaults) rather than actually opening a connection: attempting
- * a real ping here would make `/health` depend on a dependency the app
- * doesn't actually use yet, and could flap the endpoint on a DB blip that
- * doesn't affect anything real.
+ * `/health` used to report `status: 'unused'` after checking only that a
+ * connection string was configured — a value that reads as "the database is
+ * fine" while saying nothing about whether it can be reached. The endpoint now
+ * runs a real probe (`SELECT 1` over a short-lived connection) and reports what
+ * happened:
+ *
+ *   - `unavailable` — no connection string configured, nothing to probe.
+ *   - `ok`          — the probe connected and answered; `latency_ms` says how
+ *                     long that took.
+ *   - `error`       — the probe failed or exceeded its timeout; `error` carries
+ *                     the reason.
+ *
+ * The probe is bounded by `timeoutMs` so a hung database cannot hang `/health`
+ * itself: whatever the database does, the endpoint still answers, and answering
+ * slowly is exactly the failure this check exists to expose.
+ *
+ * `checkDatabase` takes an injected `ping` so the probe's own behaviour can be
+ * tested without a database; `defaultPing` is the real implementation.
  */
 
+const { Client } = require('pg');
+
 const config = require('../config');
 
-function checkDatabase() {
+const DEFAULT_TIMEOUT_MS = 2000;
+
+/**
+ * Rejects after `ms`, so a probe that never settles is reported as an error
+ * instead of holding the request open. The timer is always cleared.
+ */
+function withTimeout(promise, ms) {
+  let timer;
+  const timeout = new Promise((resolve, reject) => {
+    timer = setTimeout(
+      () => reject(new Error(`database ping timed out after ${ms}ms`)),
+      ms,
+    );
+  });
+  return Promise.race([promise, timeout]).finally(() => clearTimeout(timer));
+}
+
+/**
+ * Opens a connection, runs `SELECT 1`, closes it. A fresh connection per probe
+ * is deliberate: it proves the database is reachable *now*, which an idle
+ * pooled connection does not.
+ */
+async function defaultPing(timeoutMs = DEFAULT_TIMEOUT_MS) {
+  const client = new Client({
+    connectionString: config.databaseUrl,
+    connectionTimeoutMillis: timeoutMs,
+  });
+
+  try {
+    await client.connect();
+    await client.query('SELECT 1');
+  } finally {
+    // A failed connect still has a socket to release; a failure here must not
+    // mask the error that caused it.
+    await client.end().catch(() => {});
+  }
+}
+
+async function checkDatabase(options = {}) {
+  const { ping = defaultPing, timeoutMs = DEFAULT_TIMEOUT_MS } = options;
+
   if (!config.databaseUrl) {
     return { configured: false, checked: false, status: 'unavailable' };
   }
-  return { configured: true, checked: false, status: 'unused' };
+
+  const startedAt = Date.now();
+
+  try {
+    await withTimeout(ping(timeoutMs), timeoutMs);
+    return {
+      configured: true,
+      checked: true,
+      status: 'ok',
+      latency_ms: Date.now() - startedAt,
+    };
+  } catch (err) {
+    return {
+      configured: true,
+      checked: true,
+      status: 'error',
+      latency_ms: Date.now() - startedAt,
+      error: err && err.message ? err.message : String(err),
+    };
+  }
 }
 
-module.exports = { checkDatabase };
+module.exports = { checkDatabase, DEFAULT_TIMEOUT_MS };

test/dbHealth.test.js

@@ -0,0 +1,132 @@
+'use strict';
+
+/**
+ * Unit tests for the database health probe (issue #376).
+ *
+ * The probe is tested through its injected `ping` for the states it has to
+ * report, and once through a mocked `pg` to prove the default probe actually
+ * opens a connection, runs `SELECT 1` and closes it.
+ */
+
+const mockClientInstances = [];
+
+jest.mock('../src/config', () => ({
+  databaseUrl: 'postgres://smartdrop:secret@localhost:5432/smartdrop_test',
+}));
+
+jest.mock('pg', () => ({
+  Client: jest.fn().mockImplementation((clientConfig) => {
+    const client = {
+      clientConfig,
+      connect: jest.fn(async () => {}),
+      query: jest.fn(async () => ({ rows: [{ '?column?': 1 }] })),
+      end: jest.fn(async () => {}),
+    };
+    mockClientInstances.push(client);
+    return client;
+  }),
+}));
+
+const { Client } = require('pg');
+const { checkDatabase, DEFAULT_TIMEOUT_MS } = require('../src/services/dbHealth');
+
+beforeEach(() => {
+  jest.clearAllMocks();
+  mockClientInstances.length = 0;
+});
+
+describe('checkDatabase – configured database', () => {
+  test('reports ok with a latency when the ping succeeds', async () => {
+    const result = await checkDatabase({ ping: async () => {} });
+
+    expect(result.configured).toBe(true);
+    expect(result.checked).toBe(true);
+    expect(result.status).toBe('ok');
+    expect(typeof result.latency_ms).toBe('number');
+    expect(result.latency_ms).toBeGreaterThanOrEqual(0);
+    expect(result.error).toBeUndefined();
+  });
+
+  test('reports error with the reason instead of throwing when the ping fails', async () => {
+    const result = await checkDatabase({
+      ping: async () => {
+        throw new Error('ECONNREFUSED 127.0.0.1:5432');
+      },
+    });
+
+    expect(result.status).toBe('error');
+    expect(result.checked).toBe(true);
+    expect(result.error).toBe('ECONNREFUSED 127.0.0.1:5432');
+  });
+
+  test('reports error when the ping hangs past the timeout, and resolves anyway', async () => {
+    const neverSettles = () => new Promise(() => {});
+    const startedAt = Date.now();
+
+    const result = await checkDatabase({ ping: neverSettles, timeoutMs: 50 });
+    const elapsed = Date.now() - startedAt;
+
+    expect(result.status).toBe('error');
+    expect(result.error).toMatch(/timed out after 50ms/);
+    // The endpoint must answer on its own terms, not the database's.
+    expect(elapsed).toBeLessThan(1000);
+  });
+});
+
+describe('checkDatabase – no database configured', () => {
+  test('reports unavailable and never runs a probe', async () => {
+    jest.resetModules();
+    jest.doMock('../src/config', () => ({ databaseUrl: null }));
+
+    const { checkDatabase: checkWithoutDatabase } = require('../src/services/dbHealth');
+    const ping = jest.fn();
+
+    const result = await checkWithoutDatabase({ ping });
+
+    expect(result).toEqual({ configured: false, checked: false, status: 'unavailable' });
+    expect(ping).not.toHaveBeenCalled();
+
+    jest.dontMock('../src/config');
+    jest.resetModules();
+  });
+});
+
+describe('defaultPing – the real probe', () => {
+  test('connects to the configured URL, runs SELECT 1 and closes the connection', async () => {
+    const result = await checkDatabase();
+
+    expect(result.status).toBe('ok');
+    expect(Client).toHaveBeenCalledTimes(1);
+    expect(mockClientInstances).toHaveLength(1);
+
+    const [client] = mockClientInstances;
+    expect(client.clientConfig.connectionString).toBe(
+      'postgres://smartdrop:secret@localhost:5432/smartdrop_test',
+    );
+    expect(client.clientConfig.connectionTimeoutMillis).toBe(DEFAULT_TIMEOUT_MS);
+    expect(client.connect).toHaveBeenCalledTimes(1);
+    expect(client.query).toHaveBeenCalledWith('SELECT 1');
+    expect(client.end).toHaveBeenCalledTimes(1);
+  });
+
+  test('closes the connection even when the query fails', async () => {
+    Client.mockImplementationOnce((clientConfig) => {
+      const client = {
+        clientConfig,
+        connect: jest.fn(async () => {}),
+        query: jest.fn(async () => {
+          throw new Error('relation does not exist');
+        }),
+        end: jest.fn(async () => {}),
+      };
+      mockClientInstances.push(client);
+      return client;
+    });
+
+    const result = await checkDatabase();
+
+    expect(result.status).toBe('error');
+    expect(result.error).toBe('relation does not exist');
+    expect(mockClientInstances[0].end).toHaveBeenCalledTimes(1);
+  });
+});

test/health.test.js

@@ -44,6 +44,17 @@ jest.mock('../src/ws/priceWebSocket', () => ({
   attach: jest.fn(),
 }));
 
+// The database probe opens a real connection (issue #376), so the endpoint
+// tests inject its outcome rather than pointing them at a database.
+jest.mock('../src/services/dbHealth', () => ({
+  checkDatabase: jest.fn(async () => ({
+    configured: true,
+    checked: true,
+    status: 'ok',
+    latency_ms: 1,
+  })),
+}));
+
 // ---------------------------------------------------------------------------
 // Helpers – reset modules between tests so mocks are applied cleanly
 // ---------------------------------------------------------------------------
@@ -109,19 +120,39 @@ describe('GET /health – response shape', () => {
     expect(res.body).toHaveProperty('price_source_circuits');
   });
 
-  test('database field reflects configured-but-unused state', async () => {
+  test('database field carries the probe result and its latency', async () => {
     jest.resetModules();
     const app = loadApp();
 
     const res = await request(app).get('/health');
 
     expect(res.body.database).toEqual({
       configured: true,
-      checked: false,
-      status: 'unused',
+      checked: true,
+      status: 'ok',
+      latency_ms: expect.any(Number),
     });
   });
 
+  test('a failed probe reports the reason and makes the overall status unhealthy', async () => {
+    jest.resetModules();
+    const { checkDatabase } = require('../src/services/dbHealth');
+    checkDatabase.mockResolvedValueOnce({
+      configured: true,
+      checked: true,
+      status: 'error',
+      latency_ms: 2001,
+      error: 'database ping timed out after 2000ms',
+    });
+
+    const app = loadApp();
+    const res = await request(app).get('/health');
+
+    expect(res.body.database.status).toBe('error');
+    expect(res.body.database.error).toBe('database ping timed out after 2000ms');
+    expect(res.body.status).toBe('unhealthy');
+  });
+
   test('jobs field contains price_refresh and webhook_retry_worker entries', async () => {
     jest.resetModules();
     const app = loadApp();
@@ -170,6 +201,14 @@ describe('GET /health – status computation', () => {
       tick: jest.fn(),
       getHealth: () => ({ healthy: true, lastSuccessAt: Date.now(), lastError: null, stalled: false }),
     }));
+    // Without this mock the airdrop job reports healthy=false (it never ran in
+    // the test process), which the endpoint turns into "degraded".
+    jest.mock('../src/jobs/airdropExpiry', () => ({
+      start: jest.fn(),
+      stop: jest.fn(),
+      tick: jest.fn(),
+      getHealth: () => ({ healthy: true, lastSuccessAt: Date.now(), lastError: null, stalled: false }),
+    }));
 
     const app = loadApp();
     const res = await request(app).get('/health');