Conversation
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Greptile SummaryThis PR introduces a well-architected pg-boss-backed job scheduling system with typed job names, payload inference, a central Two issues need attention before the
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| src/server/services/jobService.ts | Core job service implementation — contains two significant bugs: (1) correlationId queue names use a colon separator that pg-boss disallows, breaking the feature entirely; (2) start() short-circuits in test mode leaving the boss unstarted, but run() still calls this.boss.send() on the unstarted instance. |
| src/server/jobs/registry.ts | Job registry with typed name lookup and duplicate-name guard at module load time — clean and well-designed; KnownJobName/RunnableJobName alias separation is forward-thinking for future per-environment filtering. |
| src/server/jobs/types.ts | Type definitions for jobs, payloads, and scheduling options — well-structured with appropriate conditional types for StartupSchedule; no issues. |
| src/server/jobs/definitions/cleanup-orphaned-images.job.ts | Stub job that intentionally throws with retryLimit: 0 and startup: undefined to prevent accidental use before the handler is implemented — appropriate placeholder design. |
| src/server/api/di-container.ts | Registers JobService as a singleton and bootstraps it at container creation with fire-and-forget error logging — deliberate design to avoid crashing the server on pg-boss startup failure. |
| src/test/integration/job-service.test.ts | Comprehensive integration test suite with good lifecycle, scheduling, and error-handling coverage; explicitly documents the correlationId colon-separator bug as a known failure case. |
| src/test/mocks/mock-job-service.ts | Minimal, correct mock implementation of IJobService with call recording — no issues. |
| src/test/test-container.ts | Correctly wires MockJobService into the test DI container as a pre-instantiated singleton so call history is inspectable across resolutions — no issues. |
Last reviewed commit: 746e707
| private getQueueName(jobName: KnownJobName, correlationId?: string): string { | ||
| if (!correlationId) return jobName; | ||
| const normalized = correlationId.trim(); | ||
| if (!normalized) { | ||
| throw new Error("correlationId must be a non-empty string."); | ||
| } | ||
| return `${jobName}:${normalized}`; | ||
| } |
There was a problem hiding this comment.
correlationId feature broken — colon is an invalid pg-boss queue name character
getQueueName produces names like jobs.cleanup-orphaned-images:tenant-1, but pg-boss only allows [A-Za-z0-9_\-\.\/] in queue names. As a result, every call to run(jobName, data, { cron, correlationId }) will fail with a "Name can only contain..." error at the createQueue/updateQueue step inside ensureWorkerRegistered. The integration test at line 276–287 explicitly documents this as a "known bug" and asserts the throw — which means this advertised feature is completely non-functional in its current form.
Consider using / (which pg-boss allows) or a hyphen/underscore as the separator:
private getQueueName(jobName: KnownJobName, correlationId?: string): string {
if (!correlationId) return jobName;
const normalized = correlationId.trim();
if (!normalized) {
throw new Error("correlationId must be a non-empty string.");
}
return `${jobName}/${normalized}`;
}The same separator change would need to be applied wherever correlated queue names are detected (e.g. the startsWith(${definition.name}:) filter in unschedule and registerWorkersForPersistedCorrelationSchedules).
| async start(): Promise<void> { | ||
| if (this.env.NODE_ENV === "test") return; | ||
| if (sharedBossState.isStarted) return; | ||
|
|
||
| sharedBossState.startPromise ??= this.bootstrap().catch((error) => { | ||
| sharedBossState.startPromise = undefined; | ||
| throw error; | ||
| }); | ||
| return sharedBossState.startPromise; |
There was a problem hiding this comment.
run() proceeds with an unstarted boss when NODE_ENV === "test"
start() returns early on line 81 without calling bootstrap(), so sharedBossState.isStarted remains false and the PgBoss instance is never started. run() then falls through to runWithStartedBoss(), which calls this.boss.send() on the unstarted instance — pg-boss will throw an internal "not started" error rather than giving a clear diagnostic.
The integration tests work around this by overriding NODE_ENV to "development", and MockJobService is used in unit tests, so this path is currently never hit in practice. However, if the real JobService is inadvertently resolved in a test environment (e.g. a missing mock registration), the developer will see a confusing pg-boss error instead of a clear signal.
A safer approach is to also guard runWithStartedBoss or make the early-return in start() propagate clearly:
async start(): Promise<void> {
if (this.env.NODE_ENV === "test") {
throw new Error(
"JobService.start() must not be called in test environments. Use MockJobService instead.",
);
}
...
}Alternatively, add a guard at the top of run() and unschedule() that checks sharedBossState.isStarted after start() returns.
| await this.boss.schedule( | ||
| queueName, | ||
| cron, | ||
| (data ?? null) as object | null, | ||
| scheduleOptions as any, | ||
| ); | ||
| this.trackRecurringQueue(jobName, queueName); | ||
| return null; |
There was a problem hiding this comment.
scheduleOptions passed as any may silently forward unsupported options
The destructuring on lines 182–188 intentionally strips cron, runAt, startAfter, and correlationId from mergedOptions, but the remainder is cast to any and passed directly to boss.schedule(). Fields like singletonKey, singletonSeconds, retryLimit, etc., in RunJobOptions may not be valid ScheduleOptions in pg-boss. Casting scheduleOptions as any suppresses type-checking that would catch such mismatches.
Consider keeping the as any only at the call site but narrowing the intermediate type (e.g. Pick<ScheduleOptions, ...>) so unsupported fields generate a compile-time error rather than silently being ignored or forwarded.
Summary
DATABASE_URL) and add a central job runtime service atsrc/server/services/jobService.ts.src/server/jobs:definitions/cleanup-orphaned-images.job.tsdefinitions/scheduled-test.job.tsregistry.tstypes.tsjobServicein DI (src/server/api/di-container.ts) and bootstrap it at server startup.src/test/mocks/mock-job-service.tsand wire it intosrc/test/test-container.ts.Key Design Changes
KnownJobName)JobPayload<TJobName>)run(jobName, data?, options?):runAt/startAfter)cron, optionalstartAt,endAt,tz)startupstartup.cronpresent => recurring startup schedulestartup.cronabsent => one-off startup enqueuerun(..., { cron, correlationId })creates correlation-specific recurring streamsunschedule(jobName, { correlationId })cancels only that streamunschedule(jobName)remains supported to cancel all schedules for a jobTest Plan
jobServiceboots without errors.jobs.cleanup-orphaned-images.jobs.scheduled-test.run()for:runAt)cron)unschedule(jobName, { correlationId })and verify only targeted recurring stream is removed.unschedule(jobName)and verify all recurring schedules for that job are removed.MockJobService.