From bca15fb29d6dd3d0c8baba61793372d1d078b6b8 Mon Sep 17 00:00:00 2001 From: Guide Fari Date: Sat, 11 Jul 2026 01:43:09 +0200 Subject: [PATCH] feat(vps): port health to HttpApiBuilder.group (Step 3a) Step 3a of the Hono -> Effect HttpApi migration (docs/migration-effect-http-api.md). First real HttpApi group taking over live traffic from the Hono fallback -- health was chosen first because it's small and low-risk to get wrong. - packages/api/src/api.ts: composed Api = HttpApi.make('gbfm').add(HealthGroup). - apps/vps/src/http/health.handlers.ts: HttpApiBuilder.group implementation. Readiness is cached for 5s via Effect.cachedWithTTL, injected as a dependency so tests can force the failure path without a real DB outage. - apps/vps/src/http/routes.ts: HealthLive mounted alongside the existing fallback/auth routes; AppLoggerLive provided so handler errors reach the app's real Pino+Sentry logger, not Effect's bare default logger. - apps/vps/src/app.ts: deleted the old Hono health routes and their module-level cache/error-class scaffolding. - Deleted apps/vps/src/health.blackbox.test.ts (tested the Hono app directly; those routes no longer exist there). Equivalent coverage now lives in routes.blackbox.test.ts against the new handler. Two real bugs were found and fixed via adversarial review before this was pushed anywhere (subagent review, not self-review): 1. The first implementation used a module-level readinessCache variable with a check-then-write race -- concurrent requests on a cold cache could each independently hit the DB, and writes could land out of order under a flapping DB. Replaced with Effect.cachedWithTTL, which memoizes the in-flight fiber so concurrent callers share one computation. Verified empirically (not just by reading the source): 50-way concurrent stress through the real handler stays at exactly 1 DB check; failures are cached the same way successes are; TTL expiry still triggers a fresh check afterward. 2. The DB check's real error (its cause) was being discarded when converting to ReadinessCheckFailedError, with no logging -- equivalent to the old HealthCheckError's cause field vanishing. Fixed by logging the cause via Effect.tapError before mapping to the wire-safe tagged error (the cause itself must never reach the response body -- a public /health endpoint leaking internal DB error detail would be an information-disclosure issue). A follow-up review caught that this log wasn't reaching the app's actual Pino+Sentry pipeline (AppLoggerLive wasn't provided in the router's layer chain) -- fixed and verified end-to-end against a genuinely unreachable DB. health.handlers.failure.test.ts is a separate file from routes.blackbox.test.ts specifically to test the failure/concurrency paths without interference from the happy-path tests -- not because the cache is shared module state anymore (it isn't; each createWebHandler call gets its own cache), but because it's a natural home for the failure-path assertions. Full apps/vps suite: 141/141 passing, 15 files. --- apps/vps/src/app.ts | 47 +----------- apps/vps/src/health.blackbox.test.ts | 53 -------------- .../src/http/health.handlers.failure.test.ts | 71 +++++++++++++++++++ apps/vps/src/http/health.handlers.ts | 60 ++++++++++++++++ apps/vps/src/http/routes.blackbox.test.ts | 61 +++++++++++----- apps/vps/src/http/routes.ts | 28 +++++++- packages/api/package.json | 1 + packages/api/src/api.ts | 4 ++ 8 files changed, 206 insertions(+), 119 deletions(-) delete mode 100644 apps/vps/src/health.blackbox.test.ts create mode 100644 apps/vps/src/http/health.handlers.failure.test.ts create mode 100644 apps/vps/src/http/health.handlers.ts create mode 100644 packages/api/src/api.ts diff --git a/apps/vps/src/app.ts b/apps/vps/src/app.ts index 8299426a..37a2cf0b 100644 --- a/apps/vps/src/app.ts +++ b/apps/vps/src/app.ts @@ -1,6 +1,4 @@ -import { sql } from 'drizzle-orm' -import { Data, Duration, Effect, Schedule } from 'effect' -import type { Context } from 'hono' +import { Duration, Effect, Schedule } from 'effect' import configureOpenAPI from '@/lib/configure-open-api' import { createAppEffect } from '@/lib/create-app' import admin from '@/routes/admin/admin.index' @@ -22,25 +20,12 @@ import spotify from '@/routes/spotify/spotify.index' import upload from '@/routes/upload/upload.index' import uploadMultipart from '@/routes/upload-multipart/upload-multipart.index' import user from '@/routes/user/user.index' -import { db } from './db' import { regenerateSitemap } from './routes/redirect/seo/sitemap' import { runApp, runAppFork } from './runtime' import { processPendingReminders, queryNextDueReminder } from './services/reminder-processor' import { ReminderSignalService } from './services/reminder-signal.service' import { SentryService } from './services/sentry.service' -class HealthCheckError extends Data.TaggedError('HealthCheckError')<{ - cause?: unknown -}> {} - -const healthCheckEffect = Effect.tryPromise({ - try: () => db.execute(sql.raw('SELECT 1')), - catch: (cause) => new HealthCheckError({ cause }) -}) - -const READINESS_CACHE_MS = 5_000 -let readinessCache: { checkedAt: number; status: 200 | 500 } | undefined - const setupRoutesEffect = Effect.gen(function* () { yield* SentryService const app = yield* createAppEffect @@ -66,38 +51,12 @@ const setupRoutesEffect = Effect.gen(function* () { app.route('/api/music-reminders', musicReminders) // Kept at root, not under /api: these are externally-referenced public URLs. - // /auth is handled by the Effect router directly (apps/vps/src/http/routes.ts, - // step 2c) -- not mounted here. + // /auth and /health are handled by the Effect router directly + // (apps/vps/src/http/routes.ts, steps 2c/3a) -- not mounted here. app.route('/s', shareRouter) app.route('', rss) app.route('', seoRouter) - app.get('/health/live', (c) => c.json({ ok: true }, 200)) - - const readinessHealthRoute = async (c: Context) => { - const cache = readinessCache - const cachedStatus = cache && Date.now() - cache.checkedAt < READINESS_CACHE_MS - - if (cachedStatus) { - return c.json({ dbConnected: cache.status === 200 }, cache.status) - } - - const program = healthCheckEffect.pipe( - Effect.map(() => ({ data: { dbConnected: true }, status: 200 as const })), - Effect.catch(() => Effect.succeed({ data: { dbConnected: false }, status: 500 as const })) - ) - - const result = await runApp(program) - readinessCache = { - checkedAt: Date.now(), - status: result.status - } - return c.json(result.data, result.status) - } - - app.get('/health/ready', readinessHealthRoute) - app.get('/health', readinessHealthRoute) - return app }) diff --git a/apps/vps/src/health.blackbox.test.ts b/apps/vps/src/health.blackbox.test.ts deleted file mode 100644 index e06a7016..00000000 --- a/apps/vps/src/health.blackbox.test.ts +++ /dev/null @@ -1,53 +0,0 @@ -import { HealthLiveResponse, HealthReadyResponse } from '@gbfm/api/health' -import { decodeResponseBody } from '@gbfm/api/testing' -import { beforeAll, describe, expect, it } from 'vitest' -import type { AppType } from '@/app' - -// Asserts only the wire contract (status, JSON body against @gbfm/api schemas) so these same assertions can run unchanged once health is ported to HttpApiBuilder (step 3a). -let app: AppType - -beforeAll(async () => { - const mod = await import('@/app') - app = mod.default -}) - -describe('Health endpoints', () => { - it('GET /health/live returns 200 with liveness JSON', async () => { - const res = await app.request('/health/live') - - expect(res.status).toBe(200) - await expect(decodeResponseBody(HealthLiveResponse, res)).resolves.toEqual({ ok: true }) - }) - - it('GET /health/ready returns 200 with readiness JSON when the database is reachable', async () => { - const res = await app.request('/health/ready') - - expect(res.status).toBe(200) - await expect(decodeResponseBody(HealthReadyResponse, res)).resolves.toEqual({ - dbConnected: true - }) - }) - - it('GET /health returns the same readiness result as /health/ready', async () => { - const res = await app.request('/health') - - expect(res.status).toBe(200) - await expect(decodeResponseBody(HealthReadyResponse, res)).resolves.toEqual({ - dbConnected: true - }) - }) - - it('repeated readiness calls within the cache window keep returning 200', async () => { - const first = await app.request('/health/ready') - const second = await app.request('/health/ready') - - expect(first.status).toBe(200) - expect(second.status).toBe(200) - }) - - it('responds 404 to unsupported methods on health paths', async () => { - const res = await app.request('/health/live', { method: 'POST' }) - - expect(res.status).toBe(404) - }) -}) diff --git a/apps/vps/src/http/health.handlers.failure.test.ts b/apps/vps/src/http/health.handlers.failure.test.ts new file mode 100644 index 00000000..e6b48771 --- /dev/null +++ b/apps/vps/src/http/health.handlers.failure.test.ts @@ -0,0 +1,71 @@ +import { ReadinessCheckFailedError } from '@gbfm/api/errors' +import { decodeResponseBody } from '@gbfm/api/testing' +import { Effect } from 'effect' +import { describe, expect, it } from 'vitest' +import type { AppType } from '@/app' +import { createWebHandler } from './routes' + +// Separate file (docs/migration-effect-http-api.md, step 3a): each +// createWebHandler builds its own cached readiness check (health.handlers.ts, +// Effect.cachedWithTTL inside makeHealthHandlers), so this doesn't strictly +// need isolation from routes.blackbox.test.ts anymore -- kept in its own file +// as a dedicated home for the failure/concurrency paths. +describe('health readiness failure + cache', () => { + it('caches a failing readiness check and does not re-run it within the window', async () => { + const mod = await import('@/app') + const app: AppType = mod.default + + let checks = 0 + const scoped = createWebHandler(app, { + healthDatabaseCheck: Effect.sync(() => { + checks += 1 + }).pipe( + Effect.flatMap(() => Effect.fail(new ReadinessCheckFailedError({ dbConnected: false }))) + ) + }) + + try { + const first = await scoped.handler(new Request('http://localhost/health/ready')) + const second = await scoped.handler(new Request('http://localhost/health/ready')) + + expect(first.status).toBe(500) + expect(second.status).toBe(500) + + const decoded = await decodeResponseBody(ReadinessCheckFailedError, first) + expect(decoded._tag).toBe('ReadinessCheckFailedError') + expect(decoded.dbConnected).toBe(false) + + // Second call must reuse the cached failure, not re-run the check. + expect(checks).toBe(1) + } finally { + await scoped.dispose() + } + }) + + it('concurrent requests on a cold cache share one in-flight check, not one each', async () => { + const mod = await import('@/app') + const app: AppType = mod.default + + let checks = 0 + const scoped = createWebHandler(app, { + healthDatabaseCheck: Effect.sync(() => { + checks += 1 + }).pipe(Effect.delay('20 millis')) + }) + + try { + const [first, second] = await Promise.all([ + scoped.handler(new Request('http://localhost/health/ready')), + scoped.handler(new Request('http://localhost/health/ready')) + ]) + + expect(first.status).toBe(200) + expect(second.status).toBe(200) + // Both requests arrived before the (delayed) check completed -- they + // must share the one in-flight computation, not race to run it twice. + expect(checks).toBe(1) + } finally { + await scoped.dispose() + } + }) +}) diff --git a/apps/vps/src/http/health.handlers.ts b/apps/vps/src/http/health.handlers.ts new file mode 100644 index 00000000..b12f141d --- /dev/null +++ b/apps/vps/src/http/health.handlers.ts @@ -0,0 +1,60 @@ +import { Api } from '@gbfm/api/api' +import { ReadinessCheckFailedError } from '@gbfm/api/errors' +import { sql } from 'drizzle-orm' +import { Effect, Layer } from 'effect' +import { HttpApiBuilder } from 'effect/unstable/httpapi' +import { db } from '@/db' + +const READINESS_CACHE_MS = 5_000 + +// Injectable so tests can force the failure path without a real DB outage +// (docs/migration-effect-http-api.md, step 1b noted this gap; step 3a closes it). +// The cause is logged server-side only -- it must never reach the response +// body (a public /health endpoint leaking internal DB error detail is a real +// information-disclosure risk), but it also must not be silently discarded. +export const checkDatabase = Effect.tryPromise({ + try: () => db.execute(sql.raw('SELECT 1')), + catch: (cause) => cause +}).pipe( + Effect.tapError((cause) => Effect.logError('[health] readiness check failed', cause)), + Effect.mapError(() => new ReadinessCheckFailedError({ dbConnected: false })), + Effect.asVoid +) + +type ReadinessResult = { readonly dbConnected: true } | { readonly dbConnected: false } + +// Effect.cachedWithTTL memoizes the *pending* computation, not just its result: +// concurrent requests that arrive while a check is in flight share that one +// fiber instead of each independently hitting the database and racing to +// write a cache slot (the module-level `let` this replaced had that race). +const readinessResult = (check: Effect.Effect) => + check.pipe( + Effect.as({ dbConnected: true }), + Effect.catch(() => Effect.succeed({ dbConnected: false })), + Effect.cachedWithTTL(`${READINESS_CACHE_MS} millis`) + ) + +const readiness = (cachedCheck: Effect.Effect) => + Effect.gen(function* () { + const result = yield* cachedCheck + if (!result.dbConnected) { + return yield* new ReadinessCheckFailedError({ dbConnected: false }) + } + return result + }) + +export const makeHealthHandlers = (check: Effect.Effect) => + Layer.unwrap( + Effect.gen(function* () { + const cachedCheck = yield* readinessResult(check) + + return HttpApiBuilder.group(Api, 'health', (handlers) => + handlers + .handle('live', () => Effect.succeed({ ok: true as const })) + .handle('ready', () => readiness(cachedCheck)) + .handle('check', () => readiness(cachedCheck)) + ) + }) + ) + +export const HealthHandlersLive = makeHealthHandlers(checkDatabase) diff --git a/apps/vps/src/http/routes.blackbox.test.ts b/apps/vps/src/http/routes.blackbox.test.ts index 51635d5d..cc2685a5 100644 --- a/apps/vps/src/http/routes.blackbox.test.ts +++ b/apps/vps/src/http/routes.blackbox.test.ts @@ -4,9 +4,9 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest' import type { AppType } from '@/app' import { createWebHandler } from './routes' -// Step 2a (docs/migration-effect-http-api.md): the Effect toWebHandler + Hono -// fallback must behave identically to the plain Hono app it wraps. Reuses the -// same @gbfm/api schemas as the 1b blackbox suite so both point at one contract. +// Blackbox suite asserting only the wire contract, so these assertions keep +// working as more groups move from the Hono fallback onto the Effect router +// (docs/migration-effect-http-api.md). let app: AppType let webHandler: ReturnType @@ -21,22 +21,6 @@ afterAll(async () => { }) describe('Effect toWebHandler fallback', () => { - it('GET /health/live matches the Hono app', async () => { - const res = await webHandler.handler(new Request('http://localhost/health/live')) - - expect(res.status).toBe(200) - await expect(decodeResponseBody(HealthLiveResponse, res)).resolves.toEqual({ ok: true }) - }) - - it('GET /health/ready matches the Hono app', async () => { - const res = await webHandler.handler(new Request('http://localhost/health/ready')) - - expect(res.status).toBe(200) - await expect(decodeResponseBody(HealthReadyResponse, res)).resolves.toEqual({ - dbConnected: true - }) - }) - it('GET /api/music/artists returns the same response as the plain Hono app', async () => { const [viaHandler, viaHono] = await Promise.all([ webHandler.handler(new Request('http://localhost/api/music/artists')), @@ -75,3 +59,42 @@ describe('better-auth route (Step 2c)', () => { expect(await withAuth.text()).not.toEqual(await withoutAuth.text()) }) }) + +describe('health (HttpApiBuilder group, Step 3a)', () => { + it('GET /health/live returns 200 without checking the database', async () => { + const res = await webHandler.handler(new Request('http://localhost/health/live')) + + expect(res.status).toBe(200) + await expect(decodeResponseBody(HealthLiveResponse, res)).resolves.toEqual({ ok: true }) + }) + + it('GET /health/ready and /health return 200 when the database check succeeds', async () => { + const res = await webHandler.handler(new Request('http://localhost/health/ready')) + const checkRes = await webHandler.handler(new Request('http://localhost/health')) + + expect(res.status).toBe(200) + expect(checkRes.status).toBe(200) + await expect(decodeResponseBody(HealthReadyResponse, res)).resolves.toEqual({ + dbConnected: true + }) + await expect(decodeResponseBody(HealthReadyResponse, checkRes)).resolves.toEqual({ + dbConnected: true + }) + }) + + it('repeated readiness calls within the cache window keep returning 200', async () => { + const first = await webHandler.handler(new Request('http://localhost/health/ready')) + const second = await webHandler.handler(new Request('http://localhost/health/ready')) + + expect(first.status).toBe(200) + expect(second.status).toBe(200) + }) + + it('responds 404 to unsupported methods on health paths', async () => { + const res = await webHandler.handler( + new Request('http://localhost/health/live', { method: 'POST' }) + ) + + expect(res.status).toBe(404) + }) +}) diff --git a/apps/vps/src/http/routes.ts b/apps/vps/src/http/routes.ts index f9556ef0..91019142 100644 --- a/apps/vps/src/http/routes.ts +++ b/apps/vps/src/http/routes.ts @@ -1,8 +1,13 @@ +import { Api } from '@gbfm/api/api' +import type { ReadinessCheckFailedError } from '@gbfm/api/errors' import { Effect, Layer } from 'effect' import { HttpRouter, HttpServer, HttpServerRequest, HttpServerResponse } from 'effect/unstable/http' +import { HttpApiBuilder } from 'effect/unstable/httpapi' import type { AppType } from '@/app' +import { checkDatabase, makeHealthHandlers } from '@/http/health.handlers' import { auth } from '@/lib/auth' import { prepareAuthRequest } from '@/routes/user/better-auth.routes' +import { AppLoggerLive } from '@/services/logger.service' // Routes every request to the existing Hono app unchanged. Removed one HttpApi group at a time as routes are ported (docs/migration-effect-http-api.md). export const honoFallback = (honoApp: AppType) => @@ -26,7 +31,24 @@ const betterAuthRoute = HttpRouter.add('*', '/auth/*', (request) => }) ) -export const createWebHandler = (honoApp: AppType) => - HttpRouter.toWebHandler( - Layer.mergeAll(betterAuthRoute, honoFallback(honoApp), HttpServer.layerServices) +// Step 3a (docs/migration-effect-http-api.md): first real HttpApi group taking +// over live traffic from the Hono fallback. Test seams accept an alternate +// database check so tests can force the failure/cache paths. +export const createWebHandler = ( + honoApp: AppType, + options?: { readonly healthDatabaseCheck?: Effect.Effect } +) => { + const HealthLive = HttpApiBuilder.layer(Api).pipe( + Layer.provide(makeHealthHandlers(options?.healthDatabaseCheck ?? checkDatabase)) ) + + return HttpRouter.toWebHandler( + Layer.mergeAll(HealthLive, betterAuthRoute, honoFallback(honoApp)).pipe( + Layer.provideMerge(HttpServer.layerServices), + // Effect.logError inside health handlers must reach the app's real + // Pino + Sentry logger, not Effect's bare default console logger -- + // otherwise a DB outage's cause is logged nowhere on-call looks. + Layer.provide(AppLoggerLive) + ) + ) +} diff --git a/packages/api/package.json b/packages/api/package.json index 6a83b711..31e579e1 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -3,6 +3,7 @@ "version": "0.0.1", "type": "module", "exports": { + "./api": "./src/api.ts", "./health": "./src/health.ts", "./errors": "./src/errors.ts", "./testing": "./src/testing.ts" diff --git a/packages/api/src/api.ts b/packages/api/src/api.ts new file mode 100644 index 00000000..75c9f947 --- /dev/null +++ b/packages/api/src/api.ts @@ -0,0 +1,4 @@ +import { HttpApi } from 'effect/unstable/httpapi' +import { HealthGroup } from './health' + +export const Api = HttpApi.make('gbfm').add(HealthGroup)