feat(background-workers,platform-api): booking notification relay parity (TEC-81)#28
Merged
Merged
Conversation
…fication relay parity (TEC-81) Co-Authored-By: Paperclip <noreply@paperclip.ing>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds booking.created and booking.completed domain events, types, notification payloads, background-worker implementations with retry/backoff and DLQ support, test coverage, and integrates publication and relay logic into the bookings service and platform event publishers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BookingsService
participant DomainEventPublisher
participant RelayPublisher
participant BackgroundWorker
participant NotificationQueue
Client->>BookingsService: createBooking(...)
BookingsService->>BookingsService: persist booking
BookingsService->>DomainEventPublisher: publishBookingCreated(event)
DomainEventPublisher->>RelayPublisher: relay booking.created
loop attempt 1..N
RelayPublisher->>BackgroundWorker: consumeBookingCreatedAttempt(input)
BackgroundWorker->>BackgroundWorker: build envelope & log "started"
alt processed
BackgroundWorker->>NotificationQueue: enqueue email notification
BackgroundWorker->>NotificationQueue: enqueue push notification
BackgroundWorker-->>RelayPublisher: { status: "processed" }
else retry-scheduled
BackgroundWorker-->>RelayPublisher: { status: "retry-scheduled", nextAttemptAt }
else dead-letter
BackgroundWorker-->>RelayPublisher: { status: "dead-letter", dlq }
end
RelayPublisher->>RelayPublisher: log breadcrumb per attempt
end
sequenceDiagram
participant Client
participant BookingsService
participant DomainEventPublisher
participant RelayPublisher
participant BackgroundWorker
participant NotificationQueue
Client->>BookingsService: completeBooking(...)
alt replay path
BookingsService->>DomainEventPublisher: publishBookingCompleted(replayed: true, occurredAt)
else normal path
BookingsService->>DomainEventPublisher: publishBookingCompleted(replayed: false, occurredAt)
end
DomainEventPublisher->>RelayPublisher: relay booking.completed
loop attempt 1..N
RelayPublisher->>BackgroundWorker: consumeBookingCompletedAttempt(input)
BackgroundWorker->>BackgroundWorker: build envelope & log "started"
alt processed
BackgroundWorker->>NotificationQueue: enqueue email notification
BackgroundWorker->>NotificationQueue: enqueue push notification
BackgroundWorker-->>RelayPublisher: { status: "processed" }
else retry-scheduled
BackgroundWorker-->>RelayPublisher: { status: "retry-scheduled", nextAttemptAt }
else dead-letter
BackgroundWorker-->>RelayPublisher: { status: "dead-letter", dlq }
end
RelayPublisher->>RelayPublisher: log breadcrumb per attempt
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
services/platform-api/src/bookings/booking-accept-relay.integration.test.ts (1)
327-329: Isolate this assertion frombooking.createdrelay side effects.The
nextAttemptAtexpectations now depend on prior clock consumption bybooking.createdretries, not onlybooking.accepted. Consider scopingshouldFailAttempttoevent.eventName === 'booking.accepted'so this test stays focused and stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/bookings/booking-accept-relay.integration.test.ts` around lines 327 - 329, The test's nextAttemptAt assertions are affected by clock consumption from booking.created retries; update the shouldFailAttempt logic to only apply to booking.accepted events (e.g., change the predicate to check event.eventName === 'booking.accepted' before marking shouldFailAttempt true) so the test isolates booking.accepted behavior from booking.created side effects and remains stable; locate and modify the shouldFailAttempt usage in the test handling loop that processes relay events (reference the shouldFailAttempt variable and the event.eventName checks).packages/domain/src/index.ts (1)
105-149: Reduce duplicated envelope/metadata contracts to avoid drift.
BookingCreated*andBookingCompleted*retry/DLQ/envelope/notification types are structurally identical except event/queue literals. A shared generic base would make future policy/shape updates safer.Also applies to: 197-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/domain/src/index.ts` around lines 105 - 149, Replace the duplicated retry/DLQ/envelope contracts with shared generic types: create a single RetryBackoffMetadata (used by both BookingCreatedRetryBackoffMetadata and BookingCompletedRetryBackoffMetadata), a generic DlqMarker<QueueName> type to cover BookingCreatedDlqMarker and BookingCompletedDlqMarker, and a generic WorkerEnvelope<EventName, EventType, RetryMetadata, DlqMarker?> to cover BookingCreatedWorkerEnvelope and BookingCompletedWorkerEnvelope; then replace the duplicated types by aliasing the new generics with the appropriate literal parameters (e.g., 'booking.created'/'booking.completed' and their corresponding DLQ queue names) so future changes to backoff shape or dlq shape only need one edit.services/platform-api/src/orchestration/relay-attempt-policy.ts (1)
13-18: Consider renaming the policy context to match broadened scope.
BookingAcceptedRelayAttemptContextnow covers multiple event families. A neutral name (e.g.,BookingRelayAttemptContext) would better reflect usage and reduce future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/orchestration/relay-attempt-policy.ts` around lines 13 - 18, The context type BookingAcceptedRelayAttemptContext is misnamed given it now covers multiple event families; rename it to a neutral name like BookingRelayAttemptContext across the file (and any imports/exports) so the type accurately reflects its broadened scope, update all references (e.g., in the RelayAttemptPolicy declaration, function parameters, type aliases and tests) to use BookingRelayAttemptContext, and ensure the union type for event remains unchanged and correctly typed where the new name is used.services/platform-api/src/orchestration/relay-domain-event.publisher.ts (1)
59-128: Consider extracting the common relay loop into a generic helper.Both
publishBookingCreatedandpublishBookingCompleted(and similarlypublishBookingDeclinedandpublishPaymentCaptured) share nearly identical structure:
- Call logging publisher
- Initial consume attempt
- Log breadcrumb
- Retry loop while status is
'retry-scheduled'- Final breadcrumb
This pattern could be extracted into a reusable generic function parameterized by event type and consume function, reducing ~140 lines of duplication.
♻️ Sketch of a generic relay helper
// Example approach (outside changed line ranges) function relayDomainEvent<TEvent extends { correlationId: string; eventId: string }, TResult extends { status: string; correlationId: string; envelope: { retry: unknown; dlq?: unknown } }>( eventName: string, consume: (input: { event: TEvent; attempt: number; maxAttempts: number; baseBackoffMs: number; shouldFail: boolean; now: Date }) => TResult, event: TEvent, policy: BookingAcceptedRelayAttemptPolicy, clock: BookingAcceptedRelayClock, ): TResult { let result = consume({ event, attempt: 1, maxAttempts: relayMaxAttempts, baseBackoffMs: relayBaseBackoffMs, shouldFail: policy.shouldFailAttempt({ event, attempt: 1, maxAttempts: relayMaxAttempts }), now: clock.now() }); logStructuredBreadcrumb({ event: `${eventName}.domain-event.relay.attempt`, ... }); for (let attempt = 2; attempt <= relayMaxAttempts && result.status === 'retry-scheduled'; attempt++) { result = consume({ ... }); logStructuredBreadcrumb({ ... }); } logStructuredBreadcrumb({ event: `${eventName}.domain-event.relay`, ... }); return result; }Also applies to: 277-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/platform-api/src/orchestration/relay-domain-event.publisher.ts` around lines 59 - 128, The publishBookingCreated method duplicates a relay/retry loop present in publishBookingCompleted (and others); extract that pattern into a generic helper (e.g., relayDomainEvent) that accepts the eventName string, the consume function (e.g., consumeBookingCreatedAttempt / consumeBookingCompletedAttempt), the event, the attempt policy (relayAttemptPolicy), the clock (relayClock), and the shared constants relayMaxAttempts and relayBaseBackoffMs, and returns the final worker result; the helper should perform the initial consume call, log per-attempt breadcrumbs via logStructuredBreadcrumb using mapRelayAttemptStatus, loop retries while status === 'retry-scheduled', log the final breadcrumb, and then replace the duplicated blocks inside publishBookingCreated, publishBookingCompleted (and publishBookingDeclined / publishPaymentCaptured) to call this helper and forward the result.services/background-workers/src/workers/booking-completed.worker.ts (1)
30-48: Consider consolidating duplicated worker logic withbooking-created.worker.ts.The structure is nearly identical to
booking-created.worker.ts:
- Same backoff formula
- Same DLQ marking pattern
- Same consume flow with state machine
- Only differences are event names, DLQ queue names, and notification copy
A generic worker factory could parameterize these differences and eliminate ~150 lines of duplication across both files.
Also applies to: 50-62, 64-91, 93-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/background-workers/src/workers/booking-completed.worker.ts` around lines 30 - 48, The buildBookingCompletedWorkerEnvelope and the booking-created.worker.ts logic are duplicated; create a generic worker factory (e.g., buildBookingWorkerFactory or createBookingWorker) that encapsulates the shared backoff formula, retry object construction (use retryStrategy), DLQ marking logic, and consume/state-machine flow, and parameterize it with eventName, DLQ queue name, notification copy/content, and any small per-event transforms; replace buildBookingCompletedWorkerEnvelope and the booking-created equivalents to call the factory with their specific eventName ('booking.completed' / 'booking.created'), correlation mapping, and notification text, leaving types like BookingCompletedWorkerEnvelope/BookingCreatedWorkerEnvelope or a common BookingWorkerEnvelope to satisfy typings. Ensure the factory exposes helper(s) used in both files (e.g., computeBackoffMs, buildEnvelope, consumeHandler) so tests and imports require minimal changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform-api/src/bookings/bookings.service.ts`:
- Around line 489-490: Replace the non-authoritative fallback to session.userId
when building booking.completed events: use the persisted provider ID from the
booking/replayed payment instead of session.userId (e.g., use
bookingToComplete.providerUserId or replayedPayment.providerUserId where
available) and throw an error if that persisted providerUserId is missing so we
fail fast; update the two sites currently using
"bookingToComplete.providerUserId ?? session.userId" (and the analogous use at
the replay/completion branch) to require the persisted ID and remove the session
fallback.
---
Nitpick comments:
In `@packages/domain/src/index.ts`:
- Around line 105-149: Replace the duplicated retry/DLQ/envelope contracts with
shared generic types: create a single RetryBackoffMetadata (used by both
BookingCreatedRetryBackoffMetadata and BookingCompletedRetryBackoffMetadata), a
generic DlqMarker<QueueName> type to cover BookingCreatedDlqMarker and
BookingCompletedDlqMarker, and a generic WorkerEnvelope<EventName, EventType,
RetryMetadata, DlqMarker?> to cover BookingCreatedWorkerEnvelope and
BookingCompletedWorkerEnvelope; then replace the duplicated types by aliasing
the new generics with the appropriate literal parameters (e.g.,
'booking.created'/'booking.completed' and their corresponding DLQ queue names)
so future changes to backoff shape or dlq shape only need one edit.
In `@services/background-workers/src/workers/booking-completed.worker.ts`:
- Around line 30-48: The buildBookingCompletedWorkerEnvelope and the
booking-created.worker.ts logic are duplicated; create a generic worker factory
(e.g., buildBookingWorkerFactory or createBookingWorker) that encapsulates the
shared backoff formula, retry object construction (use retryStrategy), DLQ
marking logic, and consume/state-machine flow, and parameterize it with
eventName, DLQ queue name, notification copy/content, and any small per-event
transforms; replace buildBookingCompletedWorkerEnvelope and the booking-created
equivalents to call the factory with their specific eventName
('booking.completed' / 'booking.created'), correlation mapping, and notification
text, leaving types like
BookingCompletedWorkerEnvelope/BookingCreatedWorkerEnvelope or a common
BookingWorkerEnvelope to satisfy typings. Ensure the factory exposes helper(s)
used in both files (e.g., computeBackoffMs, buildEnvelope, consumeHandler) so
tests and imports require minimal changes.
In `@services/platform-api/src/bookings/booking-accept-relay.integration.test.ts`:
- Around line 327-329: The test's nextAttemptAt assertions are affected by clock
consumption from booking.created retries; update the shouldFailAttempt logic to
only apply to booking.accepted events (e.g., change the predicate to check
event.eventName === 'booking.accepted' before marking shouldFailAttempt true) so
the test isolates booking.accepted behavior from booking.created side effects
and remains stable; locate and modify the shouldFailAttempt usage in the test
handling loop that processes relay events (reference the shouldFailAttempt
variable and the event.eventName checks).
In `@services/platform-api/src/orchestration/relay-attempt-policy.ts`:
- Around line 13-18: The context type BookingAcceptedRelayAttemptContext is
misnamed given it now covers multiple event families; rename it to a neutral
name like BookingRelayAttemptContext across the file (and any imports/exports)
so the type accurately reflects its broadened scope, update all references
(e.g., in the RelayAttemptPolicy declaration, function parameters, type aliases
and tests) to use BookingRelayAttemptContext, and ensure the union type for
event remains unchanged and correctly typed where the new name is used.
In `@services/platform-api/src/orchestration/relay-domain-event.publisher.ts`:
- Around line 59-128: The publishBookingCreated method duplicates a relay/retry
loop present in publishBookingCompleted (and others); extract that pattern into
a generic helper (e.g., relayDomainEvent) that accepts the eventName string, the
consume function (e.g., consumeBookingCreatedAttempt /
consumeBookingCompletedAttempt), the event, the attempt policy
(relayAttemptPolicy), the clock (relayClock), and the shared constants
relayMaxAttempts and relayBaseBackoffMs, and returns the final worker result;
the helper should perform the initial consume call, log per-attempt breadcrumbs
via logStructuredBreadcrumb using mapRelayAttemptStatus, loop retries while
status === 'retry-scheduled', log the final breadcrumb, and then replace the
duplicated blocks inside publishBookingCreated, publishBookingCompleted (and
publishBookingDeclined / publishPaymentCaptured) to call this helper and forward
the result.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bebe86a4-08cb-4044-843d-af02fefd3cf4
📒 Files selected for processing (15)
packages/domain/src/index.tsservices/background-workers/src/index.tsservices/background-workers/src/workers/booking-completed.worker.test.tsservices/background-workers/src/workers/booking-completed.worker.tsservices/background-workers/src/workers/booking-created.worker.test.tsservices/background-workers/src/workers/booking-created.worker.tsservices/background-workers/src/workers/index.tsservices/platform-api/src/bookings/booking-accept-relay.integration.test.tsservices/platform-api/src/bookings/bookings.service.complete.test.tsservices/platform-api/src/bookings/bookings.service.test.tsservices/platform-api/src/bookings/bookings.service.tsservices/platform-api/src/orchestration/domain-event.publisher.tsservices/platform-api/src/orchestration/logging-domain-event.publisher.tsservices/platform-api/src/orchestration/relay-attempt-policy.tsservices/platform-api/src/orchestration/relay-domain-event.publisher.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
booking.created/booking.completedevents inpackages/domainservices/background-workersdomain-event.publisher+relay-domain-event.publisherTest plan
validatepassesbooking-createdandbooking-completedworker tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests