Migrated send-gift-reminders to the class-based jobs service - #30730
Merged
Merged
Conversation
ref https://linear.app/ghost/issue/HKG-1976 - The daily reminder job is moving to the class-based jobs service, whose handlers delegate to an injected service method, the shape clean-gifts already uses with cleanup() - The jobs service's lifecycle log carries no counts, so the sent, not due and rejected summary the legacy path logged needs a home of its own - A failed poll propagates so the jobs service reports a failure instead of an idle completion; nothing calls the method yet, so behaviour is unchanged
ref https://linear.app/ghost/issue/HKG-1976 - The legacy Bree worker did no work: it posted a domain event back to the main thread and reported done before a single reminder was sent, so job success and duration were meaningless - The worker and its legacy registration go in the same commit as the new class because the two files share a basename and the job must never be registered in both systems - Scheduling moves to initBackgroundServices because the gifts service initialises before the jobs service is started and its backend rejects recurring registrations until then; the random 00:00-05:59 daily cron and its guards are unchanged - The exact per-gift scheduler is deliberately untouched; both paths still converge on the row-locked, marker-guarded reminder code
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (18)
🧰 Additional context used📓 Path-based instructions (7)Review new or changed service boundaries for explicit dependency ownership, deterministic/idempotent initialisation, boot ordering, transaction and event semantics, cache coherence, and restart/multi-instance safety.⚙️ CodeRabbit configuration file Files:
Review whether tests prove changed behaviour, meaningful error/edge paths, and externally observable contracts without coupling to implementation details.⚙️ CodeRabbit configuration file Files:
New source files must be TypeScript: flag new JS files as a required change unless exempt (DB migrations, apps/ember-admin/, tool/config files, scripts/, docker/, generated code).⚙️ CodeRabbit configuration file Files:
Review lens: "where does this data become trusted?" Boundary data (HTTP input, external API/SDK responses, env/config, DB/filesystem reads, queue/webhook/event payloads) is `unknown` until validated — Zod by default.⚙️ CodeRabbit configuration file Files:
Prioritise concrete correctness, security, data-integrity, compatibility, and regression risks.⚙️ CodeRabbit configuration file Files:
Type-safe boundaries: Fail only if the PR: consumes boundary data (HTTP input, external API/SDK responses, env/config, DB/filesystem reads, queue/webhook/event payloads) without validating it first — Zod by default, another format only wher...📄 CodeRabbit inference engine (Custom checks) Files:
New files are TypeScript: Fail if the PR adds a new .js/.jsx/.cjs/.mjs source file, unless it is: a DB migration (ghost/core/core/server/data/migrations/), under apps/ember-admin/, a tool/config file, under scripts/ or docker/, or generated...📄 CodeRabbit inference engine (Custom checks) Files:
🧠 Learnings (1)📚 Learning: 2026-08-03T21:09:05.797ZApplied to files:
🔇 Additional comments (5)
WalkthroughGift reminders now run through the class-based jobs service in the main process. Boot schedules a typed recurring job. The jobs service invokes Suggested reviewers: Priority: ⬇️ Low Change: Refactor Merge Risk: ⚪ Minimal · up to Gift reminder scheduling follows the initialized jobs-service lifecycle, with no actionable current-head risk identified. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:integration |
✅ Succeeded | 4m 43s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 3m 47s | View ↗ |
nx run ghost:test:ci:e2e |
✅ Succeeded | 4m 21s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 22s | View ↗ |
nx run ghost:test:e2e |
✅ Succeeded | 2m 56s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 26s | View ↗ |
nx run-many -t test:unit -p ghost |
✅ Succeeded | 38s | View ↗ |
nx run-many -t lint -p ghost-monorepo,ghost |
✅ Succeeded | 1s | View ↗ |
Additional runs (4) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-09-15 16:35:18 UTC
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30730 +/- ##
==========================================
+ Coverage 67.73% 67.74% +0.01%
==========================================
Files 1677 1677
Lines 60608 60591 -17
Branches 10488 10483 -5
==========================================
- Hits 41052 41050 -2
+ Misses 17233 17221 -12
+ Partials 2323 2320 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
allouis
reviewed
Sep 15, 2026
allouis
approved these changes
Sep 15, 2026
allouis
left a comment
allouis
left a comment
Collaborator
There was a problem hiding this comment.
Looks good, but I would consider pulling the logging to where the work happens, rather than adding a wrapper
ref https://linear.app/ghost/issue/HKG-1976 - Review asked for the logging to live where the work happens rather than behind a wrapper only the job handler called - The jobs service lifecycle log carries no counts, so the poll still has to report its own sent, not due and rejected summary - The empty-poll early return went so every run reports a summary, which is what the daily job already did - The exact scheduler runs the same poll, so it now reports the same summary instead of the daily job alone
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
2 participants
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.
ref https://linear.app/ghost/issue/HKG-1976/migrate-send-gift-reminders-to-the-durable-jobs-interface
What
Migrates the daily
send-gift-remindersfallback job from its legacy Bree worker to the class-based jobs service, following theclean-giftsmigration (HKG-1975), which had the same shape: a worker thread that did no work itself, posted a domain event back to the main process and reported "done" before a single reminder was sent. This phase only moves the job behind the new interface; the backend stays in-memory and no durable queue is introduced.SendGiftRemindersJob(gifts/jobs/send-gift-reminders-job.ts): stable typesend-gift-reminders, no payload.register-job-handlers.tsbesideclean-gifts, a single line delegating togiftService.processReminders()on the already-injectedgiftService. A failed poll propagates so the jobs service reports a failure rather than an idle completion.GiftService.processReminders(): the jobs service's lifecycle log carries no counts, so the poll itself now logs a structuredsend_gift_reminders.completedevent (reminded_count,skipped_count,failed_count,duration_ms) with the line[Background Job] send-gift-reminders processed reminders: N sent, M not due, K rejected. Because the log lives where the work happens, it is emitted by every call toprocessReminders()— the daily job and the exact per-gift scheduler alike. The early return for an empty poll was removed so a run with nothing due still logs a zero-count summary.scheduleGiftReminderJob(jobsService)ingifts/jobs/index.jsmirrorsscheduleGiftCleanupJob: samerandomOffPeakDailyCron()(a random second/minute/hour between 00:00 and 05:59), the same once-per-process and test-environment guards, and the verbatim[Background Job] send-gift-reminders scheduled at <cron>log line. It is called frominitBackgroundServicesinboot.js(inside try/catch, aboveactivitypub.init()) because the gifts service initialises before the jobs service is started (its backend rejects recurring registrations untilstart(), which boot runs afterinitServices) — exactly whereclean-giftsscheduling lives.send-gift-reminders-job.js, thescheduleJob()/jobManager/pathplumbing ingifts/jobs/index.js, and thejobs.scheduleGiftReminderJob()call at the end ofgifts/index.ts#init. The worker and the new class share a basename, so they must never coexist in a commit (an extensionlessrequirefrom JS resolves the.jsfirst, andbuild:tscemits in place).sendReminderForGift()(forUpdaterow locks, theconsumes_soon_reminder_sent_atmarker committed before the email, missing /email_disabledmember skips), per-gift error containment inprocessReminders()(only its summary log and empty-poll early return changed, above), the email service, and the exact per-gift scheduler path (SignedFlushScheduler→PUT /gifts/flush_reminders/→StartGiftReminderFlushEvent→ the subscription ingifts/index.ts, whose own log lines are unchanged). Both paths still converge on the same locked, marker-guarded code, so neither can send a reminder the other already sent.Test approach
instanceof Job, empty serialisable payload), the scheduling guard (not scheduled underNODE_ENV=test*; scheduled exactly once outside it with an off-peak cron, the verbatim log line, and no legacyaddJobcall), the handler registration (delegates togiftService.processReminders()and propagates its failure), andGiftService.processReminders()(logs the structured completion event with its counts; propagates a failed poll and logs no completion).test/integration/services/gifts/send-gift-reminders.test.ts): dispatches the job through the real jobs service against a booted Ghost and waits for thejob.completedlifecycle event before asserting (the marker is committed before the email is sent, so polling the marker alone would race the mail mock): an eligible redeemed gift is marked once, its redeemer gets exactly one reminder with the expected subject, and the summary reports 1 sent; a second run after the first completes sends nothing more, leaves the marker untouched and logs a zero-count summary; a failed poll is logged as[Background Job] send-gift-reminders failed after …with no completion event and no summary.processReminders()integration suite (unchanged apart from its header comment, which referred to the deleted worker) and theflush_remindersendpoint tests still pass.Behaviour parity
Everything customer-visible is preserved: which gifts are selected, when the daily fallback runs, the locking and marker semantics, who gets emailed, and at-most-once delivery (the in-memory backend runs a tick once with no retries, like Bree ran the worker once).
Byte-identical log lines:
[Background Job] send-gift-reminders scheduled at <cron>,… started, and… failed after Xms(the jobs service emits exactly the lines the legacy subscription emitted).Accepted deltas (the same set every prior migration documented, plus the summary log):
[Background Job] send-gift-reminders completed in Xmsplus the structuredjob.completedevent replace the legacy subscription'scompleted in Xms: N sent, M not due, K rejectedline for the daily path, with the counts moving tosend_gift_reminders.completed. The Bree/JobManager lines (Adding offloaded job…,Scheduling job…,dispatched to main process,Worker for job "send-gift-reminders" online) disappear. Verified against the pro-infra Elastic alert rules: no rule matches any of these strings.send_gift_reminders.completedsummary (including when nothing was due), in addition to its existing, unchangedstarted/completed in Xms: …lines, so its counts appear twice per flush. Nothing is removed from that path.findPendingReminderquery can escapeprocessReminders) now also reaches Sentry with ajob_typetag and the backend'sdelivery failedline; the legacy path only logged.initBackgroundServices(after boot, inside try/catch) instead of synchronously and unguarded insidegiftService.init(), so a registration failure degrades to an error log instead of failing boot.processReminders()immediately, so a poll can wait behind other default-lane work — timing only.server:shutdownTimeoutand drops a queued-but-not-started tick; the legacy worker only signalled and exited.startedand'done'messages each made JobManager read thejobstable for a row namedsend-gift-reminders, twice per tick; no such row ever existed (only one-off jobs get one), so the reads were inert and simply disappear.Telling the two paths apart in logs:
send_gift_reminders.completed,… startedand… failed afterare emitted by both. The daily job is identified by the jobs service'sjob.completedevent withjob_type: send-gift-reminders(and its count-lesscompleted in Xmsline); the exact scheduler by itscompleted in Xms: N sent, M not due, K rejectedline.Production verification
Per the issue, the migrated job must be verified on staging and then production during its natural 00:00–05:59 window using the checklist in HKG-1976 (one migrated recurring registration and no legacy cron, exactly one reminder for a naturally eligible gift, no sends for already-marked / refunded / consumed / missing-member / email-disabled cases, the exact scheduler skipping an already-reminded gift, and the counts reconciled with provider logs).