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 scheduling system to handle restarts more gracefully by anchoring job schedules to database timestamps rather than relying solely on startup offsets. The system now uses the last created_at or refreshed_at timestamp from the database to calculate when the next job should run, making scheduling consistent across restarts and redeployments.
Changes:
- Replaced cron-based scheduling with dynamic setTimeout-based scheduling that anchors to database timestamps
- Startup offsets are now only used for fresh databases with no existing records
- Updated documentation to clarify that start offset environment variables only apply to the first run
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/environment-variables.md | Updated documentation for DEAL_START_OFFSET_SECONDS, RETRIEVAL_START_OFFSET_SECONDS, and METRICS_START_OFFSET_SECONDS to clarify they only apply when no database rows exist |
| apps/backend/src/scheduler/scheduler.service.ts | Replaced scheduleJobWithOffset with new dynamic scheduling methods (scheduleInitialRun, scheduleRunAt, executeScheduledJob) that query database timestamps; removed SchedulerRegistry dependency |
| apps/backend/src/scheduler/scheduler.service.spec.ts | Added tests for the new initial scheduling logic with both fresh and populated database scenarios |
| apps/backend/src/retrieval/retrieval.service.ts | Added getLastCreatedTime() method to query the most recent retrieval timestamp |
| apps/backend/src/metrics/services/metrics-scheduler.service.ts | Implemented similar dynamic scheduling pattern with three DB query methods for different timestamp sources; removed SchedulerRegistry dependency |
| apps/backend/src/metrics/services/metrics-scheduler.service.spec.ts | Added tests for metrics scheduling logic with fresh DB and timestamp-based scenarios |
| apps/backend/src/deal/deal.service.ts | Added getLastCreatedTime() method to query the most recent deal timestamp |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I have created a database for each instance. |
| try { | ||
| const lastRunAt = await getLastRunAt(); | ||
| if (lastRunAt && lastRunAt.getTime() >= runStartedAt.getTime()) { | ||
| nextRunAt = new Date(lastRunAt.getTime() + intervalSeconds * 1000); |
There was a problem hiding this comment.
With this- job frequency becomes unpredictable. Example - If Deal Creation job takes 15 mins to complete and the interval is 30 mins- now it'll be 45 mins interval for next Deal Creation. And, in web we show these infrastructure configuration at the top, which will not make sense anymore.
| ); | ||
| } | ||
|
|
||
| private async scheduleInitialRun({ |
There was a problem hiding this comment.
nit: looks unnecessary, increasing maintenance burden
|
converting to draft because we need a better solution.. |
|
i'm going to enable a default-off pg-boss implementation that should solve #119 and help us better manage scheduling between restarts/downtime |
fixes #119