Repository navigation
feat(api): add observability readiness harness - #30
Conversation
|
Implemented Trello #35 in this PR. What changed:
Verification:
|
|
Vercel preview is ready: https://hivelore-98h4zi3gs-kareemn1936s-projects.vercel.app Commit: cdecd8a |
|
Remote checks are now green:
|
… request Fastify awaits the onError hook, and reportUnhandledError awaited a fetch with no timeout. A webhook that accepts the connection and never replies therefore kept every erroring request open until OS-level TCP timeouts — minutes. Proven against a deliberately unresponsive server: the call was still pending after 8s. Observability taking the service down with it is the worst version of this, and it would have bitten on the first day ERROR_TRACKING_ENABLED was turned on. - bound delivery with AbortSignal.timeout, default 2s, overridable per call - stop awaiting delivery in the onError hook, so the error response never waits on telemetry at all (reportUnhandledError swallows its own errors, so the floating promise cannot reject) - add a regression test using a server that accepts and never replies; it hangs against the unbounded version and passes in 150ms against this one Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mcfarhat
left a comment
mcfarhat
left a comment
There was a problem hiding this comment.
Approving, with one fix pushed to your branch (e3585c2).
The observability work is good — request IDs via randomUUID, structured logs, a /ready that returns 503 when degraded, and a sensible 1200-block lag threshold. The integration-db.ts extraction is a genuine improvement, and the webhook payload deliberately excludes headers and bodies, which is the right instinct.
What I fixed: a stalled webhook hung every failing request
app.ts app.addHook('onError', async (...) => { await reportUnhandledError(...) })
error-tracking await fetch(webhookUrl, {...}) // no timeout, no AbortSignal
Fastify awaits the onError hook, so the error response waited on that fetch. With no AbortSignal, a webhook that accepts the connection and never replies keeps it pending until OS-level TCP timeouts — minutes, not seconds.
I tested it rather than reasoning about it, pointing it at a server that accepts and never responds:
after 8s: reportUnhandledError resolved? false
elapsed: 8009ms — still pending
So a bad day at the webhook provider turns every API error into a hung request, and under load that exhausts the connection pool. Observability code taking the service down with it is the worst version of this bug, and it would have surfaced the first day ERROR_TRACKING_ENABLED was switched on — precisely when you'd least want it.
The fix, two parts:
AbortSignal.timeouton delivery, default 2s, overridable per call — so it can never hang unboundedly.- The
onErrorhook no longer awaits delivery. The error response shouldn't wait on telemetry at all.reportUnhandledErrorswallows its own errors, so the floating promise can't reject.
And a regression test, using a server that accepts and never replies. It hangs against the unbounded version and passes in ~150ms against the fixed one — I verified both directions by mutation, removing the AbortSignal and watching the suite hang.
Why the tests didn't catch it — the useful bit
The two existing tests cover serialization and the disabled path, both correct. But there were zero references to timeout, stall, hang or abort. That's the general lesson for anything doing outbound HTTP: the interesting failure isn't a bad response, it's no response. A mock that returns instantly can't model a peer that never answers.
Two smaller things, not blocking
ERROR_TRACKING_WEBHOOK_URLis validated as a well-formed URL but isn't required to be HTTPS, so stack traces could go out in cleartext. Worth tightening when a real endpoint is chosen.url: request.urlincludes the query string. Harmless today since auth is cookie-based, but it's the kind of field that quietly starts carrying tokens.
Merging.
2 participants
Summary
Trello
Verification
Note: the two live PostgreSQL integration tests skip locally when TEST_DATABASE_ADMIN_URL is not configured; CI provides that variable via the PostgreSQL service.