fix: improve api lifecycle and prep errors handling for tracing - #72
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR improves the API service lifecycle handling (graceful shutdown + DB pool draining) and upgrades webhook error handling/logging to be more structured and trace-friendly.
Changes:
- Add graceful shutdown logic in the API entrypoint, with a shutdown timeout and
server.close(). - Drain the Postgres pool on Fastify
onCloseto avoid leaking connections on shutdown. - Improve Clerk webhook logging and error handling (structured logs, better warnings/errors), and harden the auth guard against missing
request.user.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/src/plugins/db.ts | Drain the shared DB pool via an onClose hook. |
| apps/api/src/db/db.ts | Export the pool so lifecycle hooks can close it. |
| apps/api/src/app.ts | Implement graceful shutdown flow and convert startup to async/await. |
| apps/api/src/modules/webhooks/clerk.ts | Add structured request-scoped logging and improved error handling for Clerk webhooks. |
| apps/api/src/lib/auth-guard.ts | Avoid accessing request.user.role when request.user wasn’t set. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
apps/api/src/app.ts:13
shutdown()can be triggered multiple times (e.g. repeated signals), which can lead to overlappingserver.close()calls and inconsistent exit codes/logging. Add an in-progress guard so shutdown is idempotent.
const SHUTDOWN_TIMEOUT_MS = Number(process.env.SHUTDOWN_TIMEOUT_MS) || 10_000;
const shutdown = async (signal: NodeJS.Signals) => {
server.log.info({ signal, timeoutMs: SHUTDOWN_TIMEOUT_MS }, "shutdown started");
apps/api/src/lib/auth-guard.ts:22
requireEventManageris throwingUnauthorizedError(401) when the user is authenticated but lacks the required role. That should be a 403 (ForbiddenError) so clients can distinguish unauthenticated vs. insufficient-permission cases.
const requireEventManager = async (request: FastifyRequest) => {
await requireAuth(request);
if (!isEventManager(request.user.role)) {
throw new UnauthorizedError();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (2)
apps/api/src/app.ts:15
- The shutdown handler can run multiple times (e.g., repeated SIGTERM/SIGINT), and on successful
server.close()it only setsprocess.exitCodethen clears the force-exit timer. If any handles remain after close, the process can hang indefinitely despite logging "shutdown complete".
const shutdown = async (signal: NodeJS.Signals) => {
server.log.info({ signal, timeoutMs: SHUTDOWN_TIMEOUT_MS }, "shutdown started");
const forceExit = setTimeout(() => {
server.log.error({ timeoutMs: SHUTDOWN_TIMEOUT_MS }, "shutdown timed out, forcing exit");
apps/api/src/lib/auth-guard.ts:22
requireEventManagercurrently throwsUnauthorizedError(401) when a signed-in user lacks the event-manager role. That should be a 403 (forbidden) to avoid misclassifying authenticated-but-not-authorized requests and to keep API semantics consistent with the rest of the codebase’sForbiddenErrorusage.
const requireEventManager = async (request: FastifyRequest) => {
await requireAuth(request);
if (!isEventManager(request.user.role)) {
throw new UnauthorizedError();
No description provided.