fix(workers): monitor dead-letter queues without consuming jobs - #324
henrique221 wants to merge 6 commits into
Conversation
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Add an explicit offline policy migration that preserves retained jobs and refuses pending work. Report policy drift for both exclusive worker queues, avoid redundant creation updates, read DLQ stats concurrently, and bound monitor shutdown. Cover the migration with real PostgreSQL integration tests. Refs: #324
kaseywright
left a comment
kaseywright
left a comment
There was a problem hiding this comment.
Re-reviewed after the latest push (aa96cd0) — the queue-policy migration script, expanded runbook, and added tests resolve the earlier findings well. No correctness bugs found; three cleanup-only items below worth a look before merge.
kaseywright
left a comment
kaseywright
left a comment
There was a problem hiding this comment.
Re-reviewed after 4faf47d — the earlier three findings (hardcoded queue names, unconditional updateQueue writes, missing schema-version guard) are all properly resolved with test coverage. A few new items from this commit worth a look before merge.
| logger.info(`${signal} received, shutting down server`); | ||
| try { | ||
| if (audioReclaimInterval) clearInterval(audioReclaimInterval); | ||
| await stopDeadLetterMonitor(); |
There was a problem hiding this comment.
await stopDeadLetterMonitor() runs before server.close() in the shutdown sequence. Since the monitor can take up to DLQ_SHUTDOWN_TIMEOUT_MS (5s) to resolve, this delays the HTTP listener closing (and rejecting new connections) by up to 5s, eating into the orchestrator's shutdown grace period for no benefit — the sweep could drain concurrently with server.close() instead. Can you confirm this ordering is intentional?
| if (apply) { | ||
| // Includes all partitions. No sender, worker or maintenance process can | ||
| // change jobs between the pending-work check and the policy update. | ||
| await tx`LOCK TABLE pgboss.queue, pgboss.job IN ACCESS EXCLUSIVE MODE`; |
There was a problem hiding this comment.
This ACCESS EXCLUSIVE lock on pgboss.queue/pgboss.job is acquired before the no-op check below (queue.policy === 'exclusive'). The runbook documents re-running the migration after success as a no-op, but as written it still stalls every queue's job fetch/complete/send system-wide for up to the 5s lock timeout just to discover there's nothing to do. Worth moving the policy check before the lock? Can you confirm this is intentional?
| /** Time to investigate new DLQ entries before pg-boss maintenance removes them. */ | ||
| export const DLQ_RETENTION_SECONDS = 30 * 24 * 60 * 60; | ||
| export const DLQ_SHUTDOWN_TIMEOUT_MS = 5_000; | ||
| const PG_BOSS_SCHEMA_VERSION = 26; |
There was a problem hiding this comment.
PG_BOSS_SCHEMA_VERSION = 26 is now hardcoded independently here and in exclusive-worker-queue-migration.ts (also literal 26). A future pg-boss schema bump could get one file updated and not the other, silently disabling DLQ monitoring while the migration script still runs (or vice versa). Worth extracting a shared constant? Can you confirm this is intentional?
| retryBackoff: true, | ||
| expireInSeconds: 600, | ||
| deadLetter: QUEUE_NAMES.USFM_EXPORT_DLQ, | ||
| } as const; |
There was a problem hiding this comment.
QUEUE_NAMES.USFM_EXPORT_DLQ is still dead in production code — ensureWorkerQueue derives the DLQ name itself as ${name}-dlq rather than consuming this constant, so only a test file references it. Flagged in an earlier round too; leaving two independent spellings of the same fact risks drift if either is renamed. Can you confirm this is intentional?
2 participants
I added a shared dead-letter queue convention for export, AI suggestions and DBL workers. The API reports DLQ depth every minute through the existing logger, including when the export worker cannot boot because R2 is unavailable. The monitor never consumes or replays jobs. Queue reads run concurrently, failures stay isolated, and monitor shutdown waits at most five seconds.
New DLQ entries get at least 30 days of retention. Longer queue settings and existing job rows stay intact during startup. Both export and AI queues report a policy mismatch. Queue setup creates fresh queues with their final settings and updates existing queues without deleting them.
For legacy non-exclusive queues, I added an explicit offline migration and runbook. It locks the queue/job tables, refuses pending work, and changes policy metadata without deleting queues or history. It supports shared and dedicated pg-boss 12.1.1 partitions and preserves IDs, payloads, errors, states, retry counters, routing and deadlines. Deployment does not run this migration automatically.
Validation passed: 618 unit tests, 8 PostgreSQL 16 integration tests, typecheck, lint, formatting, build and docs checks. Lint has three existing verse-audio warnings. The integration suite runs the real export/AI worker handlers with external services replaced by fixtures, and verifies retry exhaustion, recovery, timeout, retention and preserved messages. It also proves migration refusal with pending work, unchanged history apart from policy metadata, idempotence, and singleton dedupe after both partition migrations. The production logger test verifies flat Application Insights dimensions without sending telemetry.
Live Azure alert rules still need to be configured by the environment owner. No production queue or infrastructure was accessed.
Closes #256.
Screenshots
Local backend smoke on
edf3bfd, captured with Playwright from a report of real PostgreSQL 16.13 snapshots and events emitted by the PR's monitor. Controlled pg-boss failures use one immediate retry; R2 and AI services are not called in this capture.1) Retry stays out of the DLQ
The source job remains in
retry. The DLQ has no row, and the monitor reportsdepth: 0at info level.2) Terminal failure becomes observable
Exhausting retries creates a retained DLQ row and a structured warning with
depth: 1. Two monitor sweeps preserve the captured fields, including state, payload, failure output and the 30-day deadline.3) Existing evidence is preserved
Repeated queue setup preserves the existing DLQ message and its original deadline. A legacy export queue keeps its failed-job history and reports the policy mismatch.