feat: Bot Startup Issues and Add Health Check with Smoke Test#275
feat: Bot Startup Issues and Add Health Check with Smoke Test#275Suvam-paul145 wants to merge 1 commit intoGauravKarakoti:mainfrom
Conversation
…, and a smoke test script.
|
@Suvam-paul145 is attempting to deploy a commit to the Gaurav's projects Team on Vercel. A member of the Team first needs to authorize it. |
✅ Deploy Preview for swapsmithminiapp canceled.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes bot startup sequencing issues and adds an HTTP readiness signal (/health) plus a smoke-test script to validate end-to-end startup.
Changes:
- Adds a global readiness flag and a
/healthendpoint that reportsstartingvsok. - Consolidates startup flow to start
orderMonitorbefore launching the bot, failing fast on startup errors. - Introduces a Node/TS smoke test that spawns the bot and polls
/healthuntil ready.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
bot/src/bot.ts |
Removes conflict artifacts, enforces startup order, and adds /health readiness endpoint. |
bot/scripts/smoke-test.ts |
New smoke test script that spawns the bot and polls /health for readiness. |
bot/package.json |
Adds test:smoke script entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Cleanup | ||
| console.log('\n🛑 Stopping bot process...'); | ||
| botProcess.kill('SIGTERM'); | ||
|
|
||
| // Windows might need stronger kill | ||
| if (process.platform === 'win32') { | ||
| spawn('taskkill', ['/pid', botProcess.pid!.toString(), '/f', '/t']); | ||
| } | ||
|
|
||
| process.exit(healthCheckPassed ? 0 : 1); |
There was a problem hiding this comment.
Cleanup sends SIGTERM and then immediately calls process.exit(...) without waiting for the spawned process (and any child processes created by npm run dev / ts-node-dev --respawn) to actually exit. This can leave orphaned processes and port contention in CI; wait for the child exit event with a short timeout, then force-kill the full process tree if needed before exiting the smoke test.
| const botProcess = spawn('npm', ['run', 'dev'], { | ||
| cwd: path.join(__dirname, '..'), | ||
| stdio: 'inherit', | ||
| shell: true, | ||
| env: { ...process.env, PORT: String(PORT) } | ||
| }); | ||
|
|
||
| let healthCheckPassed = false; | ||
|
|
||
| // Give it a moment to initialize spawn | ||
| await sleep(2000); | ||
|
|
||
| for (let i = 0; i < MAX_RETRIES; i++) { | ||
| process.stdout.write('.'); | ||
| if (await checkHealth()) { | ||
| console.log('\n✅ Health check passed!'); | ||
| healthCheckPassed = true; | ||
| break; | ||
| } | ||
| await sleep(CHECK_INTERVAL); | ||
| } |
There was a problem hiding this comment.
The polling loop doesn't check whether botProcess has already exited (e.g., startup crash). In that case the script waits the full timeout and reports a generic health timeout, which hides the real failure mode and slows CI; attach an exit/close handler and fail fast if the child exits before the health check passes.
| app.listen(PORT, () => console.log(`🌍 Server running on port ${PORT}`)); | ||
|
|
||
| (async () => { | ||
| <<<<<<< HEAD | ||
| await orderMonitor.loadPendingOrders(); | ||
| orderMonitor.start(); | ||
| bot.launch(); | ||
| logger.info('🤖 Bot running'); | ||
| })(); | ||
|
|
||
| try { | ||
| await orderMonitor.loadPendingOrders(); | ||
| orderMonitor.start(); | ||
| console.log('👀 Order Monitor started'); | ||
|
|
||
| await bot.launch(); | ||
| console.log('🤖 Bot launched successfully'); | ||
|
|
||
| isReady = true; | ||
| } catch (e) { | ||
| console.error('⚠️ Failed to start order monitor:', e); | ||
| console.error('⚠️ Failed to start:', e); | ||
| process.exit(1); |
There was a problem hiding this comment.
Startup logging here uses console.log/console.error even though the codebase already has a shared Winston logger (used broadly, e.g. src/services/order-monitor.ts). For consistent log formatting/levels (and to avoid missing logs in environments where stdout isn't captured), switch these startup messages to logger.info/logger.error (including the app.listen callback and the startup try/catch).
| "db:push": "drizzle-kit push", | ||
| "test:smoke": "ts-node scripts/smoke-test.ts" |
There was a problem hiding this comment.
The test:smoke script invokes ts-node, but ts-node is not listed as a direct dependency/devDependency in this package.json. Relying on transitive hoisting (via ts-node-dev) is brittle and can break in different npm install/hoist layouts; add ts-node explicitly (or change the script to use an existing direct tool like ts-node-dev/npx ts-node).
|
Go through the copilot suggestions |
|
yes @GauravKarakoti as I have have not vercel Authorization that's why it fails, other than it is ready for merge |
|
Ahh actually @Suvam-paul145 i just cross checked #211 was not assigned to you... please get assigned to issue before raising a PR for them |
This PR resolves bot startup failures caused by merge conflicts and improves observability by adding a health check endpoint and an automated smoke test. The changes ensure reliable startup sequencing and provide a clear signal of application readiness.
Problem Statement
bot/src/bot.tscaused unstable startup behavior.Solution
Changes Made
1. Startup Fixes
bot/src/bot.ts.orderMonitorstarts before the bot launches.isReadyflag to represent initialization state.2. Health Check Endpoint
GET /health503 Service Unavailablewith{"status":"starting"}during initialization.200 OKwith{"status":"ok"}once startup completes successfully.3. Smoke Test Script
bot/scripts/smoke-test.tsnpm run test:smoke/healthendpoint.200 OKis received or fails after a 30s timeout.Startup & Health Flow
flowchart LR A[Bot Process Starts] --> B[Initialize orderMonitor] B --> C[Bot Startup Logic] C --> D{isReady?} D -- No --> E[/health → 503 starting/] D -- Yes --> F[/health → 200 ok/] closes issues #211