-
Notifications
You must be signed in to change notification settings - Fork 0
chore: ♻️ update type definitions and improve shutdown handling #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add @types/pg for better type-safety - Refactor shutdown process in main.ts to use async/await for app and database termination.
WalkthroughReplaced inline SIGINT/SIGTERM handlers with a centralized async Changes
Sequence Diagram(s)sequenceDiagram
actor Process
participant App
participant DB as "DB Client"
Note over Process: SIGINT / SIGTERM
Process->>App: gracefulShutdown(app, signal)
activate App
App->>App: guard concurrent shutdowns
App->>App: log signal received
App->>Process: start 10s forced-exit timer
App->>App: await app.stop()
App->>DB: await DB.$client.end()
alt success
App->>Process: clear timer
App->>Process: exit(0)
else error
App->>Process: clear timer
App->>Process: log error
App->>Process: exit(1)
end
deactivate App
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main.ts (1)
59-70: Async shutdown is good; add error handling around stop/closeAwaiting
app.stop()anddb.$client.end()in the signal handlers is a solid improvement for graceful shutdown. The remaining gap is lack of error handling: if either rejects, you get a noisy/unclear failure and may not reachprocess.exit(0).Consider wrapping both awaits in
try/catch/finallyso errors are logged and the process still terminates deterministically, e.g.:-process.once('SIGINT', async () => { - log.info('SIGINT received, shutting down...'); - await app.stop(); - await db.$client.end(); - process.exit(0); -}); +process.once('SIGINT', async () => { + log.info('SIGINT received, shutting down...'); + try { + await app.stop(); + await db.$client.end(); + } catch (error: unknown) { + log.error({ err: error }, 'Error during SIGINT shutdown'); + } finally { + process.exit(0); + } +}); -process.once('SIGTERM', async () => { - log.info('SIGTERM received, shutting down...'); - await app.stop(); - await db.$client.end(); - process.exit(0); -}); +process.once('SIGTERM', async () => { + log.info('SIGTERM received, shutting down...'); + try { + await app.stop(); + await db.$client.end(); + } catch (error: unknown) { + log.error({ err: error }, 'Error during SIGTERM shutdown'); + } finally { + process.exit(0); + } +});If you want to go further, you could also extract a shared
shutdownhelper to avoid repeating the same sequence for both signals. Please also re‑run Biome (bun run lint:fix) and existing tests after this change, as per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
package.json(1 hunks)src/main.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx,js,jsx}: Strictly follow Biome linter rules and runbun run lint:fixto auto-fix issues
Use explicit imports instead of wildcard imports
Files:
src/main.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: TypeScript strict mode is enabled. Do not useanytypes unless absolutely necessary
Usesrc/common/config.tsfor environment variables with Envalid. Do not useprocess.envdirectly
All new features must include unit/integration tests
Files:
src/main.ts
🧠 Learnings (1)
📚 Learning: 2025-11-27T13:14:34.764Z
Learnt from: CR
Repo: truehazker/elysia-boilerplate PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T13:14:34.764Z
Learning: Applies to src/**/*.{ts,tsx,js,jsx} : Strictly follow Biome linter rules and run `bun run lint:fix` to auto-fix issues
Applied to files:
package.json
🔇 Additional comments (1)
package.json (1)
36-42: Dev dependency updates align with existing stackAdding
@types/pgon top of the existingpgruntime dependency and bumpingpino-prettyin a patch version both look correct and low‑risk; no changes needed on this file. Please just ensure the lockfile and CI (lint/tests) are rerun so tooling stays in sync.
- Improve error handling during shutdown in main.ts for SIGINT and SIGTERM signals. - Update pino-pretty to version 13.1.3 and cookie to version 1.1.1 in bun.lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/main.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx,js,jsx}: Strictly follow Biome linter rules and runbun run lint:fixto auto-fix issues
Use explicit imports instead of wildcard imports
Files:
src/main.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: TypeScript strict mode is enabled. Do not useanytypes unless absolutely necessary
Usesrc/common/config.tsfor environment variables with Envalid. Do not useprocess.envdirectly
All new features must include unit/integration tests
Files:
src/main.ts
🧬 Code graph analysis (1)
src/main.ts (1)
src/common/logger.ts (1)
log(4-19)
- Replace inline shutdown logic for SIGINT and SIGTERM with a dedicated gracefulShutdown utility function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/util/graceful-shutdown.ts (1)
1-26: Graceful shutdown logic looks solid; consider guarding against double invocation and tightening types.The shutdown sequence (log → timeout →
app.stop()→db.$client.end()→ clear timeout → exit) is clear and addresses the previous concerns about duplication, error handling, and hanging shutdowns. Usingimport type { App }avoids a runtime circular dependency withsrc/main.ts, which is appropriate here.Two small, non-blocking improvements you might consider:
- Prevent concurrent shutdown attempts
If the process receives bothSIGINTandSIGTERMin quick succession,gracefulShutdowncould be invoked twice. A simple module‑scoped guard avoids overlapping shutdowns:-import { log } from 'src/common/logger'; -import db from 'src/db'; -import type { App } from 'src/main'; - -export async function gracefulShutdown( - app: App, - signal: string, -): Promise<void> { +import { log } from 'src/common/logger'; +import db from 'src/db'; +import type { App } from 'src/main'; + +let isShuttingDown = false; + +export async function gracefulShutdown( + app: App, + signal: NodeJS.Signals | string, +): Promise<void> { + if (isShuttingDown) { + log.warn(`Shutdown already in progress, ignoring ${signal}`); + return; + } + isShuttingDown = true;
- Narrow the signal type
Instead ofsignal: string, you can useNodeJS.Signals(or a union of the ones you actually use) for better type‑safety:export async function gracefulShutdown( app: App, signal: NodeJS.Signals, ): Promise<void> {Also, given the guideline that new behavior should be covered by tests, please ensure you have tests (unit or integration) that validate at least:
- Successful shutdown (logs,
app.stopanddb.$client.endcalled, exit code 0).- Failing shutdown (error logged, exit code 1).
- Timeout behavior when shutdown takes too long (exit code 1).
If those are already present elsewhere, just confirm; otherwise, I can help sketch a test approach using spies on
process.exit,app.stop, anddb.$client.end.src/main.ts (1)
59-62: App type export and signal wiring look correct; optional micro‑refactor possible.The
export type App = typeof app;alias is a neat way to share the concrete app type with the shutdown utility without leaking Elysia’s generic type parameters everywhere, and theprocess.oncehandlers correctly delegate togracefulShutdownwith the runningappinstance.If you want to remove the tiny bit of duplication and keep the signal list in one place, you could do:
-export type App = typeof app; - -process.once('SIGINT', () => gracefulShutdown(app, 'SIGINT')); -process.once('SIGTERM', () => gracefulShutdown(app, 'SIGTERM')); +export type App = typeof app; + +const handleSignal = (signal: NodeJS.Signals) => gracefulShutdown(app, signal); + +process.once('SIGINT', () => handleSignal('SIGINT')); +process.once('SIGTERM', () => handleSignal('SIGTERM'));Not required, but it centralizes the signal handling pattern slightly and plays well with future extension (e.g., adding other signals).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main.ts(2 hunks)src/util/graceful-shutdown.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx,js,jsx}: Strictly follow Biome linter rules and runbun run lint:fixto auto-fix issues
Use explicit imports instead of wildcard imports
Files:
src/util/graceful-shutdown.tssrc/main.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: TypeScript strict mode is enabled. Do not useanytypes unless absolutely necessary
Usesrc/common/config.tsfor environment variables with Envalid. Do not useprocess.envdirectly
All new features must include unit/integration tests
Files:
src/util/graceful-shutdown.tssrc/main.ts
🧬 Code graph analysis (2)
src/util/graceful-shutdown.ts (2)
src/main.ts (1)
App(59-59)src/common/logger.ts (1)
log(4-19)
src/main.ts (1)
src/util/graceful-shutdown.ts (1)
gracefulShutdown(5-26)
🔇 Additional comments (1)
src/main.ts (1)
7-7: Good decoupling by importing centralized gracefulShutdown.Importing
gracefulShutdownhere and moving the logic tosrc/util/graceful-shutdown.tscleans upmain.tsand addresses the earlier duplication and exit‑flow concerns from past reviews. This keepsmain.tsfocused on composition and wiring rather than lifecycle details.
Summary by CodeRabbit
Chores
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.