feat(vps): port health to HttpApiBuilder.group (Step 3a)#149
Open
guidefari wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this exists: first real HttpApi group taking over live traffic from the Hono fallback. Everything before this (steps 1-2c) was scaffolding and plumbing that never touched a real endpoint's behavior. This is where the migration actually starts replacing something users hit.
Stacked on #147 (merge #142 -> ... -> #147 first).
Why health first
Small, well-understood, low blast radius if wrong -- exactly the kind of thing you want to get the real pattern (contract -> handler -> caching -> error mapping -> router mounting) working on before doing it on something that matters more.
Process note
Ran two rounds of adversarial subagent review on this before pushing (per your instruction to do this at checkpoints). Round 1 found two real bugs: a race condition in the readiness cache, and a swallowed DB error with no logging. Both got fixed. Round 2 verified the fixes were real (not just different) and caught that the error log, even after being added, wasn't reaching the app's actual Pino+Sentry logging pipeline -- fixed that too, verified end-to-end against a real unreachable DB. All of this happened before anything was pushed; the commit history reflects the final state, not the back-and-forth.
Verification