Updated the scaling factors for domain warmup#14
Conversation
closes [GVA-605](https://linear.app/ghost/issue/GVA-605/adjust-the-warmup-numbers-to-reflect-joeys-suggestions) Our initial scaling factors were based on some preliminary research, and were designed to be as simple as possible. After further discussions, we've decided to avoid having such a fast ramp-up at the early stage, and limit the growth at the later stages to 75k+ per send. That should still prevent any large sites from going back into warm-up between sends, but is more likely to avoid damaging the reputation by scaling too aggressively.
WalkthroughThe pull request refactors the email domain warming service's scaling logic and EmailModel API. It replaces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts`:
- Around line 111-112: The null-check using ==/!= triggers ESLint eqeqeq; in the
method that reads the query result (look for
result.data[0].get('csd_email_count') in DomainWarmingService.ts) replace the
ternary null check (count != null ? count : 0) with the nullish coalescing
operator (count ?? 0) so the behavior is identical but lint-clean.
- Around line 101-103: The query in DomainWarmingService using
this.#emailModel.findPage currently uses a filter `created_at:<=${...}` which
can include today's midnight rows; change the operator to `<` so the filter
reads `created_at:<${new Date().toISOString().split('T')[0]}` to strictly
exclude today’s records and satisfy the unit test and documentation that expect
"before today". Ensure the change is made where findPage is called in
DomainWarmingService (the block constructing the filter/order) so no other logic
is affected.
- Around line 124-139: The threshold comparison logic in DomainWarmingService
(function that computes the next warmup count) currently uses a mix of >= and <
which pushes exact boundary values into the next band; change the high-volume
check from ">= WARMUP_SCALING_TABLE.highVolume.threshold" to ">" so 400000 falls
in the previous band, and change the per-threshold check in the for-loop from
"if (lastCount < threshold.limit)" to "if (lastCount <= threshold.limit)" so
exact limits (1_000, 5_000, 100_000, etc.) are treated as inclusive upper
bounds; keep the final fallback as-is.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/core/core/server/services/email-service/DomainWarmingService.tsghost/core/test/integration/services/email-service/domain-warming.test.jsghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (2)
ghost/core/test/unit/server/services/email-service/batch-sending-service.test.js (22)
service(44-46)service(61-63)service(76-78)service(91-93)service(120-122)service(147-150)service(185-190)service(228-235)service(258-260)service(284-286)service(359-378)service(460-482)service(549-568)service(658-677)service(734-753)service(826-829)ghost/core/test/unit/server/services/email-service/utils/index.ts (1)
createModelClass(221-221)
🪛 ESLint
ghost/core/core/server/services/email-service/DomainWarmingService.ts
[error] 112-112: Expected '!==' and instead saw '!='.
(eqeqeq)
🔇 Additional comments (11)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (2)
5-7: Type update tofindPageis clear.The explicit
limitin the model contract matches the new pagination usage.
14-65: Updated scaling table structure reads well.The high‑volume caps and expanded thresholds are clearly documented and typed.
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (5)
10-12: Email model typing update looks good.The test type now reflects the new
findPageAPI surface.
66-161: Test updates for new scaling inputs look consistent.Covers zero/null/undefined counts and the new 1.25× baseline expectations.
163-184: Good coverage of the newfindPagequery path.Validates filter/order/limit wiring.
186-226: Comprehensive progression table is helpful.Nice to see explicit coverage through high‑volume caps.
19-21: No action needed.createModelClassexposesfindPageas an independently implemented method that defaults to usingfindAllwith an empty array fallback. The concern aboutTypeError: findPage is not a functionis not applicable.Likely an incorrect or invalid review comment.
ghost/core/test/integration/services/email-service/domain-warming.test.js (4)
53-66: Fixed‑clock setup improves determinism.Centralizing the base date and fake timers should stabilize day progression.
192-198: Updated day‑2 scaling assertion aligns with the 1.25× rule.
220-241: Multi‑day progression assertions match the new ramping rules.
290-301: Helper keeps max‑limit expectations readable.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const result = await this.#emailModel.findPage({ | ||
| filter: `created_at:<=${new Date().toISOString().split('T')[0]}`, | ||
| order: 'csd_email_count DESC', |
There was a problem hiding this comment.
Use < to truly exclude today’s records.
The method is documented as “excluding today,” and the unit test asserts a strict “before today” filter; <= can include midnight records and fails that expectation.
🛠️ Proposed fix
- filter: `created_at:<=${new Date().toISOString().split('T')[0]}`,
+ filter: `created_at:<${new Date().toISOString().split('T')[0]}`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = await this.#emailModel.findPage({ | |
| filter: `created_at:<=${new Date().toISOString().split('T')[0]}`, | |
| order: 'csd_email_count DESC', | |
| const result = await this.#emailModel.findPage({ | |
| filter: `created_at:<${new Date().toISOString().split('T')[0]}`, | |
| order: 'csd_email_count DESC', |
🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 101 - 103, The query in DomainWarmingService using
this.#emailModel.findPage currently uses a filter `created_at:<=${...}` which
can include today's midnight rows; change the operator to `<` so the filter
reads `created_at:<${new Date().toISOString().split('T')[0]}` to strictly
exclude today’s records and satisfy the unit test and documentation that expect
"before today". Ensure the change is made where findPage is called in
DomainWarmingService (the block constructing the filter/order) so no other logic
is affected.
| const count = result.data[0].get('csd_email_count'); | ||
| return count != null ? count : 0; |
There was a problem hiding this comment.
Fix ESLint eqeqeq error on null check.
count != null trips the configured rule; ?? 0 preserves intent and is lint‑clean.
🛠️ Proposed fix
- const count = result.data[0].get('csd_email_count');
- return count != null ? count : 0;
+ const count = result.data[0].get('csd_email_count');
+ return count ?? 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const count = result.data[0].get('csd_email_count'); | |
| return count != null ? count : 0; | |
| const count = result.data[0].get('csd_email_count'); | |
| return count ?? 0; |
🧰 Tools
🪛 ESLint
[error] 112-112: Expected '!==' and instead saw '!='.
(eqeqeq)
🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 111 - 112, The null-check using ==/!= triggers ESLint eqeqeq; in the
method that reads the query result (look for
result.data[0].get('csd_email_count') in DomainWarmingService.ts) replace the
ternary null check (count != null ? count : 0) with the nullish coalescing
operator (count ?? 0) so the behavior is identical but lint-clean.
| // For high volume senders (400k+), cap the increase at 20% or 75k absolute | ||
| if (lastCount >= WARMUP_SCALING_TABLE.highVolume.threshold) { | ||
| const scaledIncrease = Math.ceil(lastCount * WARMUP_SCALING_TABLE.highVolume.maxScale); | ||
| const absoluteIncrease = lastCount + WARMUP_SCALING_TABLE.highVolume.maxAbsoluteIncrease; | ||
| return Math.min(scaledIncrease, absoluteIncrease); | ||
| } | ||
|
|
||
| for (const threshold of WARMUP_SCALING_TABLE.thresholds.sort((a, b) => a.limit - b.limit)) { | ||
| if (lastCount <= threshold.limit) { | ||
| if (lastCount < threshold.limit) { | ||
| return Math.ceil(lastCount * threshold.scale); | ||
| } | ||
| } | ||
|
|
||
| return Math.ceil(lastCount * WARMUP_SCALING_TABLE.defaultScale); | ||
| // This should not be reached given the thresholds cover all cases up to highVolume.threshold | ||
| return Math.ceil(lastCount * WARMUP_SCALING_TABLE.highVolume.maxScale); | ||
| } |
There was a problem hiding this comment.
Threshold boundaries push exact limits into the next band.
With < and >=, values equal to 1_000/5_000/100_000/400_000 jump to the next tier (e.g., 1_000 → 1.5×), which conflicts with the table/test expectations. If upper bounds are meant to be inclusive, use <= and keep high‑volume for >400k (or bump the threshold).
🛠️ Proposed fix (inclusive upper bounds)
- if (lastCount >= WARMUP_SCALING_TABLE.highVolume.threshold) {
+ if (lastCount > WARMUP_SCALING_TABLE.highVolume.threshold) {
const scaledIncrease = Math.ceil(lastCount * WARMUP_SCALING_TABLE.highVolume.maxScale);
const absoluteIncrease = lastCount + WARMUP_SCALING_TABLE.highVolume.maxAbsoluteIncrease;
return Math.min(scaledIncrease, absoluteIncrease);
}
for (const threshold of WARMUP_SCALING_TABLE.thresholds.sort((a, b) => a.limit - b.limit)) {
- if (lastCount < threshold.limit) {
+ if (lastCount <= threshold.limit) {
return Math.ceil(lastCount * threshold.scale);
}
}🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 124 - 139, The threshold comparison logic in DomainWarmingService
(function that computes the next warmup count) currently uses a mix of >= and <
which pushes exact boundary values into the next band; change the high-volume
check from ">= WARMUP_SCALING_TABLE.highVolume.threshold" to ">" so 400000 falls
in the previous band, and change the per-threshold check in the for-loop from
"if (lastCount < threshold.limit)" to "if (lastCount <= threshold.limit)" so
exact limits (1_000, 5_000, 100_000, etc.) are treated as inclusive upper
bounds; keep the final fallback as-is.
2 participants
Benchmark PR from qodo-benchmark#242
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.