Feat/issue 8 webhook delivery queue

[Joycejay17]

[FEAT] Webhook Delivery Queue with Retry, Dead Letter, and Ordering

Closes #8

Summary

Replaces the previous fire-and-forget webhook dispatch with a Redis-backed
delivery queue that delivers merchant webhooks reliably: prioritised,
retried with backoff, strictly ordered per session, and dead-lettered when they
cannot be completed.

dispatchWebhook() now enqueues a job; a single in-process worker
(WebhookQueueService) polls Redis and delivers jobs, keeping all queue state in
Redis so it survives restarts.

What's included

1. Job queue architecture

• Redis-backed queue (scheduled zset + per-session zsets + job hashes) under the
  orbitstream:webhook namespace. A custom implementation was chosen over Bull to
  match the codebase's existing Redis patterns (see PaymentCursorService) and to
  support strict per-session ordering, which Bull does not provide out of the box.
• Jobs are enqueued by dispatchWebhook() and consumed by an in-process
  @Injectable worker started in onModuleInit (poll interval / concurrency are
  env-configurable; can be disabled via WEBHOOK_WORKER_DISABLED for a dedicated
  worker instance).

2. Priority levels

payment.confirmed → 1, session.expired/payment.failed → 2,
session.created/session.cancelled → 3. Higher priority is dispatched first.

3. Retry with exponential backoff + jitter

• Schedule: 1m → 5m → 30m → 2h → 12h, max 5 attempts.
• ±20% random jitter on every interval to avoid a thundering herd.
• Pure baseBackoffMs / applyJitter / backoffDelayMs helpers for testability.

4. Dead-letter queue

• webhook_dead_letters table stores the original payload, all attempts (with
  timestamps + error messages), merchant id, event, and the dead-letter reason.
• Entries are viewable and manageable via the management API below.

5. Idempotency

• Each delivery carries a stable X-OrbitStream-Delivery-Id (UUID v4) and
  X-OrbitStream-Timestamp (ISO 8601), reused across all retries.
• The HMAC-SHA256 signature covers delivery_id + timestamp + payload.
• Documented for merchants in docs/webhooks.md.

6. Webhook ordering

• webhook_deliveries gains a per-session sequence column.
• The worker processes at most one job per session at a time and only ever runs
  the lowest-sequence job of a session, so same-session webhooks always arrive in
  order — even while an earlier delivery is mid-retry. Different sessions run
  concurrently.

7. Network handling

• 10s HTTP timeout per attempt.
• 2xx → delivered; generic 4xx → dead-letter immediately (no retry);
  408/429/5xx/network/timeout → retry with backoff. All attempts are logged with
  full error details.

8. API endpoints (JWT, per-merchant)

• GET /v1/webhooks/deliveries
• GET /v1/webhooks/dead-letter
• POST /v1/webhooks/dead-letter/:id/retry
• DELETE /v1/webhooks/dead-letter/:id

9. Testing

72 tests pass. New coverage:

• Unit — backoff schedule + jitter envelope, priority ordering, HTTP status
  classification, HMAC signing.
• Delivery — 2xx/4xx/5xx/network outcomes, required headers, stable signature
  across retries (mocked HTTP).
• Queue integration — priority dispatch, per-session ordering (including across
  a retry), 4xx → dead-letter (no retry), 5xx → retry → dead-letter after max
  attempts, unique/stable delivery ids, manual dead-letter retry, and a load test
  of 100 concurrent deliveries.

Database

New schema is applied with Drizzle:

npm run drizzle:push

Adds columns to webhook_deliveries (session_id, sequence, delivery_id,
priority, status, attempt_log) and the new webhook_dead_letters table.

Checks

• npm run lint — passes (no errors)
• npm run build — passes
• npm test — 72 passed

Notes / follow-ups

• The worker runs in-process and is authoritative for ordering on a single
  instance. Running multiple worker instances would require a Redis-level
  per-session lock (the scheduling state is already in Redis); gated behind
  WEBHOOK_WORKER_DISABLED so a dedicated worker process can be split out later.

[oomokaro1]

PR Review: #23 — Webhook Delivery Queue with Retry, Dead Letter, and Ordering

Overall Assessment

Well-structured PR with solid test coverage (72 tests) and clean separation of concerns. The architecture (queue service → delivery service, Redis state, PostgreSQL persistence) is sound. There are several issues to address, ranging from critical to minor.

---

Critical Issues

1. RedisService DI resolution will fail at runtime

WebhookQueueService (webhook-queue.service.ts:6) imports RedisService from ../redis/redis.service, but the existing RedisService lives at src/auth/redis.service.ts and is only provided inside AuthModule. The WebhookModule does not import any module that provides RedisService, so NestJS dependency injection will throw at runtime.

The tests pass because they mock RedisService directly, and npm run build passes because TypeScript does not validate DI containers at compile time.

Fix: Either move RedisService to a shared RedisModule (e.g., src/redis/redis.module.ts) marked @Global(), or import AuthModule into WebhookModule, or register RedisService as a provider in WebhookModule.

---

2. Non-atomic DB + Redis writes in enqueue()

webhook-queue.service.ts:120-139 does a DB insert followed by Redis writes (hset, zadd) without a transaction. If the process crashes between the DB insert and the Redis write, you get an orphaned webhook_deliveries row with no corresponding Redis job — it will never be delivered. Conversely, if Redis succeeds but DB fails, the job exists in Redis but there is no DB record.

Fix: Consider wrapping in a DB transaction, or at minimum add idempotency on the DB side (e.g., check if a deliveryId already exists before insert) so retries can recover.

---

3. Redis key leakage — no TTLs

• orbitstream:webhook:job:{id} hashes are never expired — orphaned jobs (e.g., process crash mid-process()) leak keys permanently.
• orbitstream:webhook:seq:{sessionId} counters are never cleaned up after a session webhooks complete.
• orbitstream:webhook:session:{sid} zsets are cleaned up by cleanup() but only on successful completion or dead-letter.

Fix: Add TTLs to Redis keys (e.g., 24h for jobs, matching the max backoff + buffer). Add a periodic cleanup sweep for stale session keys.

---

Moderate Issues

4. Route prefix change is a breaking change

The controller prefix changed from @Controller("webhooks") to @Controller("v1/webhooks"). Any existing dashboard or integration using /webhooks/retry will break. The old retryFailed endpoint was removed entirely. This should be mentioned in the PR description or handled via a redirect/backward-compatible route.

---

5. No input validation on listDeliveries limit

webhook.controller.ts:35: limit ? Number(limit) : 50 — no upper bound. A request with ?limit=999999 would dump the entire table. Add Math.min(Number(limit), 100) or similar.

---

6. recordDelivery has a read-then-update race

webhook-queue.service.ts:319-342 reads the row, appends to attemptLog, then updates. The attemptLog append-then-set pattern could lose updates under high concurrency. Consider using a Postgres-level jsonb_append or wrapping in a transaction.

---

7. Dead-letter sessionId foreign key is missing

webhookDeadLetters.sessionId (schema.ts:103) is defined as uuid("session_id") without a .references() clause, unlike webhookDeliveries.sessionId which references checkoutSessions.id. Dead-letter rows can reference non-existent session IDs.

---

Minor Issues

8. as any casts throughout enqueue() and recordDelivery()

webhook-queue.service.ts:129,337 — The as any casts on the DB insert/update suggest the Drizzle schema types are not aligning with the actual column definitions. This could hide type errors.

---

9. No controller tests

All 72 tests cover the queue service, delivery service, and constants. There are zero tests for the WebhookController endpoints (listDeliveries, listDeadLetter, retryDeadLetter, dismissDeadLetter). At minimum, test that the JWT guard is applied and that merchant scoping works.

---

10. dispatchWebhook is only called with "payment.confirmed"

The codebase has priority mapping for 5 event types but only payment.confirmed is ever dispatched. The priority/ordering logic is well-tested but currently dead code in production. Not a blocker — this is future-proofing — but worth noting.

---

11. seqKey is never deleted

orbitstream:webhook:seq:{sessionId} uses INCR to generate sequence numbers but is never cleaned up. Over many sessions, this accumulates keys.

---

What is Done Well

• Clean separation: WebhookDeliveryService (HTTP only) vs WebhookQueueService (scheduling/retry/ordering) vs WebhookService (API facade).
• Per-session ordering with sequence numbers is correctly implemented — the test for "does not deliver a later session event while an earlier one is still retrying" is particularly good.
• Stable HMAC signature across retries via fixed deliveryId + timestamp — well-thought-out idempotency design.
• Dead-letter queue with manual retry is a nice operational feature.
• Test coverage is thorough: backoff jitter envelope, priority ordering, status classification, load test of 100 concurrent deliveries.
• The WEBHOOK_WORKER_DISABLED flag is a clean escape hatch for multi-instance deployments.

---

Recommended Priority

Priority	Issue	Effort
P0	Fix RedisService DI (will crash at runtime)	Small
P0	Add TTLs to Redis keys	Small
P1	Handle non-atomic DB+Redis writes	Medium
P1	Cap listDeliveries limit	Tiny
P1	Document route prefix breaking change	Tiny
P2	Add FK to dead-letter sessionId	Tiny
P2	Add controller tests	Medium
P2	Clean up seqKey after session completion	Small

[Joycejay17]

[FEAT] Webhook Delivery Queue with Retry, Dead Letter, and Ordering

Closes #8

Summary

Replaces the previous fire-and-forget webhook dispatch with a Redis-backed
delivery queue that delivers merchant webhooks reliably: prioritised, retried
with backoff, strictly ordered per session, and dead-lettered when they cannot be
completed.

dispatchWebhook() now enqueues a job; a single in-process worker
(WebhookQueueService) polls Redis and delivers jobs, keeping all queue state in
Redis so it survives restarts.

What's included

1. Job queue architecture

• Redis-backed queue (scheduled zset + per-session zsets + job hashes) under the
  orbitstream:webhook namespace. A custom implementation was chosen over Bull to
  match the codebase's existing Redis patterns (see PaymentCursorService) and to
  support strict per-session ordering, which Bull does not provide out of the box.
• Jobs are enqueued by dispatchWebhook() and consumed by an in-process
  @Injectable worker started in onModuleInit (poll interval / concurrency are
  env-configurable; can be disabled via WEBHOOK_WORKER_DISABLED).

2. Priority levels

payment.confirmed → 1, session.expired/payment.failed → 2,
session.created/session.cancelled → 3. Higher priority is dispatched first.

3. Retry with exponential backoff + jitter

• Schedule: 1m → 5m → 30m → 2h → 12h, max 5 attempts.
• ±20% random jitter on every interval to avoid a thundering herd.
• Pure baseBackoffMs / applyJitter / backoffDelayMs helpers for testability.

4. Dead-letter queue

• webhook_dead_letters table stores the original payload, all attempts (with
  timestamps + error messages), merchant id, event, and the dead-letter reason.

5. Idempotency

• Each delivery carries a stable X-OrbitStream-Delivery-Id (UUID v4) and
  X-OrbitStream-Timestamp (ISO 8601), reused across all retries.
• The HMAC-SHA256 signature covers delivery_id + timestamp + payload.
• Documented for merchants in docs/webhooks.md.

6. Webhook ordering

• webhook_deliveries gains a per-session sequence column.
• The worker processes at most one job per session at a time and only ever runs the
  lowest-sequence job of a session, so same-session webhooks always arrive in order
  — even while an earlier delivery is mid-retry. Different sessions run concurrently.

7. Network handling

• 10s HTTP timeout per attempt.
• 2xx → delivered; generic 4xx → dead-letter immediately (no retry);
  408/429/5xx/network/timeout → retry with backoff. All attempts are logged.

8. API endpoints (JWT, per-merchant)

• GET /v1/webhooks/deliveries
• GET /v1/webhooks/dead-letter
• POST /v1/webhooks/dead-letter/:id/retry
• DELETE /v1/webhooks/dead-letter/:id

9. Testing

85 tests pass — unit (backoff/jitter, priority, status classification, signing),
HTTP delivery (2xx/4xx/5xx/network, headers, stable signature), queue integration
(priority, per-session ordering across a retry, 4xx → DLQ, 5xx → retry → DLQ,
idempotent delivery ids, manual DLQ retry, 100 concurrent deliveries), and
controller (JWT guard, merchant scoping, limit clamping, retry/dismiss).

Database

New schema is applied with Drizzle:

npm run drizzle:push

Adds columns to webhook_deliveries (session_id, sequence, delivery_id,
priority, status, attempt_log) and the new webhook_dead_letters table.

Checks

• npm run lint — passes (no errors)
• npm run build — passes
• npm test — 85 passed

Breaking changes

• Webhook routes are now under /v1/webhooks (per the issue spec). The previous
  ad-hoc POST /webhooks/retry endpoint is removed; manual retries now go through
  POST /v1/webhooks/dead-letter/:id/retry.

Notes / follow-ups

• The worker runs in-process and is authoritative for ordering on a single
  instance. Running multiple workers would require a Redis-level per-session lock
  (scheduling state already lives in Redis); gated behind WEBHOOK_WORKER_DISABLED
  so a dedicated worker process can be split out later.
• DB + Redis writes in enqueue() are not transactional (different stores). The
  delivery_id UNIQUE constraint makes inserts idempotent; full crash-recovery of a
  pending row that lost its Redis job is left to a future reconciliation sweep.
• Redis queue keys carry a 48h TTL (refreshed on every write) so crash-orphaned keys
  are reclaimed automatically.

Review follow-ups addressed

• Redis key TTLs — added to job hashes, session zsets, and sequence counters.
• ?limit cap — clamped to [1, 100] (default 50).
• Controller tests — added.
• RedisService DI — verified correct: RedisService is provided by the
  @Global() RedisModule (src/redis/redis.module.ts) imported in AppModule, so
  it injects into WebhookModule without extra wiring (no runtime DI failure).
• Dead-letter session_id FK — intentionally omitted: dead-letter rows are a
  retained audit/recovery record that must outlive a cascade-deleted session.

[Joycejay17]

Thanks for the thorough review — really appreciate the detail. I've pushed fixes (c9a2ec7); point-by-point below.

1. RedisService DI (Critical) — couldn't reproduce; I think this was against a different state of the tree. On this branch there's no src/auth/redis.service.ts. RedisService lives at src/redis/redis.service.ts and is provided + exported by a @global() RedisModule:

// src/redis/redis.module.ts
@global()
@module({ providers: [RedisService], exports: [RedisService] })
export class RedisModule {}
…which AppModule imports (src/app.module.ts), so it injects into WebhookModule without extra wiring. The app boots and resolves the queue worker fine. Happy to add an explicit import to WebhookModule if you'd prefer it not to rely on the global module — let me know.

2. Non-atomic DB+Redis writes — fair, this is inherent at-least-once across two stores. delivery_id already has a UNIQUE constraint (inserts are idempotent), and I reordered enqueue() so the scheduled zset add (the dispatch trigger) is the final step. Full crash-recovery of a pending row that lost its Redis job needs a reconciliation sweep — I've noted that as a follow-up rather than fake a cross-store transaction.

3 & 11. Redis key TTLs — fixed. 48h TTL (> the ~14.6h full retry lifecycle) on job hashes, per-session zsets, and seq:{sessionId} counters, refreshed on every write so live jobs never expire but crash-orphans get reclaimed.

4. Route prefix — /v1/webhooks is required by the issue spec (§8); the old /webhooks/retry is superseded by POST /v1/webhooks/dead-letter/:id/retry. Now called out under "Breaking changes" in the PR description.

5. ?limit cap — fixed, clamped to [1, 100] (default 50).

6. recordDelivery read-then-update — not a real race here: a deliveryId maps to one job processed serially by a single worker (guarded by the processing set + one-in-flight-per-session), so the row is never written concurrently with itself. Left as-is.

7. Dead-letter session_id FK — this one's intentional: dead-letter rows are a retained audit/recovery record that should survive a cascade-deleted session, so a cascading FK would defeat the purpose. Documented the rationale; can revisit if you'd rather have referential integrity here.

8. as any casts — consistent with the existing services (merchants/payments do the same on Drizzle inserts with defaults). Left for consistency.

9. Controller tests — added (webhook-controller.spec.ts): JWT guard, merchant scoping, NotFound paths, limit clamping, retry/dismiss.

10. Only payment.confirmed dispatched today — agreed, the other events are future-proofing.

Build + lint clean, 85 tests pass (was 72). Thanks again!

[oomokaro1]

Updated PR Review: #23 — Webhook Delivery Queue

What Changed Since Last Review

The new commit fix(webhook): add Redis key TTLs, cap deliveries limit, add controller tests addresses 3 of my previous concerns:

Previous Issue	Status
P0 — Redis key leakage / no TTLs	Fixed — WEBHOOK_KEY_TTL_S = 48h applied to job hashes, session zsets, and seq counters
P1 — No limit validation on listDeliveries	Fixed — clampLimit() bounds to [1, 100], defaults to 50
P2 — No controller tests	Fixed — 9 tests covering guard metadata, merchant scoping, limit clamping, dead-letter retry/dismiss
P0 — RedisService DI resolution	Still broken
P1 — Non-atomic DB+Redis writes	Acknowledged with comment, not resolved
P1 — Route prefix breaking change	Not addressed
P2 — Dead-letter sessionId FK missing	Not addressed

---

Remaining Issues

1. [P0] RedisService DI will crash at runtime — UNCHANGED

WebhookQueueService (webhook-queue.service.ts:6) imports RedisService from ../redis/redis.service, but:

• The existing RedisService is at src/auth/redis.service.ts, only provided in AuthModule
• WebhookModule (webhook.module.ts) does not import any module that provides RedisService
• NestJS will throw Nest can not resolve dependencies of the WebhookQueueService at startup

Tests pass because they inject RedisService manually. Build passes because TS does not check DI. This will fail in production.

Fix: Add AuthModule to WebhookModule.imports, or create a shared RedisModule with @Global(), or provide RedisService directly in WebhookModule.

---

2. [P1] Non-atomic DB + Redis writes — acknowledged but not resolved

enqueue() (lines 1081-1115) does a DB insert then Redis writes. The new comment (line 1077-1080) correctly identifies the issue and notes deliveryId is UNIQUE, but the actual scenario remains:

• DB insert succeeds, process crashes before Redis write, orphaned DB row, job never delivered
• The comment says "a future reconciliation sweep can re-enqueue any pending row" — this does not exist yet

Suggestion: At minimum, add a startup sweep in onModuleInit that finds status=pending rows with no matching Redis job and re-enqueues them. Or use a DB transaction + Redis pipeline in a try/retry pattern.

---

3. [P1] Route prefix breaking change — not documented

Controller changed from @Controller("webhooks") to @Controller("v1/webhooks"). The old POST /webhooks/retry endpoint was removed entirely. If any dashboard or integration calls the old routes, they will 404. The PR description does not mention this.

---

4. [P2] Dead-letter sessionId foreign key still missing

webhookDeadLetters.sessionId (schema.ts:103) is uuid("session_id") with no .references() clause. Dead-letter rows can reference non-existent session IDs. The webhookDeliveries.sessionId correctly has .references(() => checkoutSessions.id, { onDelete: "cascade" }).

---

5. [P2] as any casts remain on DB operations

webhook-queue.service.ts:1092,1333,1359,1384 — Multiple as any casts on db.insert() and db.update().set() calls. This hides potential type mismatches between the Drizzle schema and the actual values being written. The attemptLog and payload jsonb fields likely need explicit type annotations in the schema.

---

What is Still Good

Everything from the original review stands. The new additions are clean:

• clampLimit() is well-tested with parametrized cases including edge cases (NaN, negative, overflow)
• Controller tests verify JWT guard metadata, merchant scoping, and all endpoint happy/sad paths
• TTLs are set at 48h which comfortably exceeds the full retry lifecycle (~14.6h)
• The comment on non-atomic writes is honest and documents the known limitation

---

Updated Priority

Priority	Issue	Status
P0	Fix RedisService DI (runtime crash)	Still broken
P1	Non-atomic DB+Redis writes	Acknowledged, needs reconciliation sweep
P1	Document route prefix breaking change	Not addressed
P2	Dead-letter sessionId FK	Not addressed
P2	as any casts on DB operations	Not addressed

The P0 RedisService DI issue is the only blocker — everything else is incremental.

[Joycejay17]

Thanks for the re-review. Pushed 58433e0.

RedisService DI (P0): I dug into this and can't reproduce a DI failure — I think the review is against a stale tree. There is no src/auth/redis.service.ts in this branch (ls src/auth → no redis file); RedisService is defined only in src/redis/redis.service.ts and provided by a @global() RedisModule that AppModule imports. To remove any doubt I (a) added an explicit RedisModule import to WebhookModule, and (b) added webhook-module.spec.ts, which compiles the real WebhookModule graph (no manual provider overrides) and asserts WebhookQueueService resolves with RedisService injected — i.e. Nest constructs the graph without throwing. If you're still seeing Nest can't resolve dependencies of WebhookQueueService, could you share the branch/SHA and stack trace? It would help me see what differs.

Non-atomic DB+Redis writes (P1): implemented the reconciliation sweep I'd referenced — recoverPending() runs in onModuleInit and re-enqueues any pending/failed delivery whose Redis job is missing. Two tests cover the orphan-recovery and the no-op-when-present cases.

Route prefix (P1): intentional — /v1/webhooks is mandated by the issue spec (§8), and /webhooks/retry is superseded by POST /v1/webhooks/dead-letter/:id/retry. Now called out under "Breaking changes" in the PR description.

Dead-letter session_id FK (P2): added with ON DELETE SET NULL so the audit row survives a deleted session.

as any on DB ops (P2): left as-is for consistency — the existing merchants/payments services use the same pattern on Drizzle inserts with defaulted columns; happy to tackle it repo-wide as a separate cleanup.

89 tests pass; build + lint clean.

[oomokaro1]

LGTM. All P0/P1 blockers resolved — DI, crash recovery, and FK all addressed. Approving and merging.