Updated the scaling factors for domain warmup #27
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 changes update the email domain warming service by replacing single-record database queries with paginated results, reorganizing warmup scaling thresholds with new intermediate milestones, and introducing high-volume scaling constraints that limit growth above 400,000 using both relative and absolute increase caps. 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
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 2
🤖 Fix all issues with AI agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts`:
- Around line 100-113: The method `#getHighestCount` currently builds a filter for
`#emailModel.findPage` using "<=YYYY-MM-DD", which includes today's emails; change
the filter operator to "<" so it excludes today as intended (keep the same date
expression new Date().toISOString().split('T')[0] and the rest of the findPage
options and return logic intact).
- Around line 131-135: The loop in DomainWarmingService that iterates
WARMUP_SCALING_TABLE.thresholds uses a strict `<` comparison which misclassifies
boundary values (e.g., lastCount === threshold.limit) and also calls sort() on
the thresholds on every invocation; change the comparison from `<` to `<=` so
boundary values map to the intended tier (affecting the branch that returns
Math.ceil(lastCount * threshold.scale)), and avoid mutating the original array
each call by pre-sorting thresholds once (e.g., sort
WARMUP_SCALING_TABLE.thresholds at initialization or use a non-mutating
toSorted() result) so the function uses an already-sorted thresholds list
instead of sorting in-place every time.
🧹 Nitpick comments (3)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (1)
124-129: Consider renaming variables for clarity.The variable names
scaledIncreaseandabsoluteIncreaseare misleading—both represent the target limit, not the increase amount. ConsiderscaledLimitandabsoluteCapLimitfor clarity.Suggested naming improvement
// 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); + const scaledLimit = Math.ceil(lastCount * WARMUP_SCALING_TABLE.highVolume.maxScale); + const absoluteCapLimit = lastCount + WARMUP_SCALING_TABLE.highVolume.maxAbsoluteIncrease; + return Math.min(scaledLimit, absoluteCapLimit); }ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (1)
178-184: Test assertion may not catch filter operator mismatch.The
includes()check on line 181 would pass for bothcreated_at:<andcreated_at:<=since the latter contains the former as a substring. Consider using a stricter assertion to verify the exact filter operator:Suggested fix
sinon.assert.calledOnce(findPageStub); const callArgs = findPageStub.firstCall.args[0]; assert.ok(callArgs.filter); - assert.ok(callArgs.filter.includes(`created_at:<${today}`)); + assert.equal(callArgs.filter, `created_at:<${today}`); assert.equal(callArgs.order, 'csd_email_count DESC'); assert.equal(callArgs.limit, 1);ghost/core/test/integration/services/email-service/domain-warming.test.js (1)
290-301: Helper is incomplete for higher volume tiers.The
getExpectedScalehelper only covers thresholds up to 1.75× and is missing the 2× tier (100k–400k) and high-volume cap (400k+). While this doesn't affect the current test (max ~800 recipients), consider completing it for consistency and future test expansion:Suggested improvement
const getExpectedScale = (count) => { if (count <= 100) { return 200; } if (count <= 1000) { return Math.ceil(count * 1.25); } if (count <= 5000) { return Math.ceil(count * 1.5); } + if (count <= 100000) { + return Math.ceil(count * 1.75); + } + if (count <= 400000) { + return Math.ceil(count * 2); + } + // High volume: min(1.2×, +75k) + return Math.min(Math.ceil(count * 1.2), count + 75000); - return Math.ceil(count * 1.75); };
📜 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 comments (6)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (2)
5-7: LGTM!The
EmailModeltype update to usefindPagewith paginated response structure is appropriate for fetching the highest count record.
30-65: Good documentation and configuration structure.The scaling table documentation clearly explains the volume-based multipliers, and the
highVolumecap logic prevents aggressive scaling at high volumes.ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (1)
186-227: Test expectations are correct; implementation boundary logic needs fixing.The test cases correctly expect boundary-inclusive behavior (e.g.,
lastCount=1000should use 1.25× scale). These tests will fail against the current implementation which uses<instead of<=. Once the implementation is fixed per the earlier comment, these tests should pass.ghost/core/test/integration/services/email-service/domain-warming.test.js (3)
53-67: Good use of fixed base time for deterministic test behavior.Using a fixed
baseDatewith hours set to noon avoids timezone-related flakiness in day boundary calculations.
217-241: LGTM!The deterministic day-by-day progression with explicit expected values (200 → 250 → 313) makes the test clear and easy to debug.
192-204: LGTM!The test correctly validates the 1.25× scaling factor for day 2 progression and handles both cases where recipients exceed or fall below the calculated limit.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async #getHighestCount(): Promise<number> { | ||
| const email = await this.#emailModel.findOne({ | ||
| 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', | ||
| limit: 1 | ||
| }); | ||
|
|
||
| if (!email) { | ||
| if (!result.data.length) { | ||
| return 0; | ||
| } | ||
|
|
||
| const count = email.get('csd_email_count'); | ||
| return count || 0; | ||
| const count = result.data[0].get('csd_email_count'); | ||
| return count != null ? count : 0; | ||
| } |
There was a problem hiding this comment.
Filter may include today's emails contrary to the documented intent.
The comment on line 98 states "excluding today", but the filter on line 102 uses <= which would include emails created today. If the intent is to exclude today's sends when calculating the next limit, the filter should use < instead.
Proposed fix
async `#getHighestCount`(): Promise<number> {
const result = await this.#emailModel.findPage({
- filter: `created_at:<=${new Date().toISOString().split('T')[0]}`,
+ filter: `created_at:<${new Date().toISOString().split('T')[0]}`,
order: 'csd_email_count DESC',
limit: 1
});📝 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.
| async #getHighestCount(): Promise<number> { | |
| const email = await this.#emailModel.findOne({ | |
| 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', | |
| limit: 1 | |
| }); | |
| if (!email) { | |
| if (!result.data.length) { | |
| return 0; | |
| } | |
| const count = email.get('csd_email_count'); | |
| return count || 0; | |
| const count = result.data[0].get('csd_email_count'); | |
| return count != null ? count : 0; | |
| } | |
| async `#getHighestCount`(): Promise<number> { | |
| const result = await this.#emailModel.findPage({ | |
| filter: `created_at:<${new Date().toISOString().split('T')[0]}`, | |
| order: 'csd_email_count DESC', | |
| limit: 1 | |
| }); | |
| if (!result.data.length) { | |
| return 0; | |
| } | |
| const count = result.data[0].get('csd_email_count'); | |
| return count != null ? count : 0; | |
| } |
🤖 Prompt for AI Agents
In `@ghost/core/core/server/services/email-service/DomainWarmingService.ts` around
lines 100 - 113, The method `#getHighestCount` currently builds a filter for
`#emailModel.findPage` using "<=YYYY-MM-DD", which includes today's emails; change
the filter operator to "<" so it excludes today as intended (keep the same date
expression new Date().toISOString().split('T')[0] and the rest of the findPage
options and return logic intact).
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
Boundary condition bug: threshold comparison should use <= not <.
The < comparison causes boundary values to fall into the wrong tier. For example, with lastCount = 1000:
- Current:
1000 < 1000is false → falls through to 1.5× tier → returns 1500 - Expected per docs and tests: 1000 should use 1.25× → returns 1250
The same issue affects all boundary values (1000, 5000, 100000, 400000).
Additionally, sort() mutates the array in-place on every call. Consider sorting once at initialization or using toSorted().
Proposed fix
- for (const threshold of WARMUP_SCALING_TABLE.thresholds.sort((a, b) => a.limit - b.limit)) {
- if (lastCount < threshold.limit) {
+ for (const threshold of [...WARMUP_SCALING_TABLE.thresholds].sort((a, b) => a.limit - b.limit)) {
+ if (lastCount <= threshold.limit) {
return Math.ceil(lastCount * threshold.scale);
}
}📝 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.
| 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); | |
| } | |
| } | |
| for (const threshold of [...WARMUP_SCALING_TABLE.thresholds].sort((a, b) => a.limit - b.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 131 - 135, The loop in DomainWarmingService that iterates
WARMUP_SCALING_TABLE.thresholds uses a strict `<` comparison which misclassifies
boundary values (e.g., lastCount === threshold.limit) and also calls sort() on
the thresholds on every invocation; change the comparison from `<` to `<=` so
boundary values map to the intended tier (affecting the branch that returns
Math.ceil(lastCount * threshold.scale)), and avoid mutating the original array
each call by pre-sorting thresholds once (e.g., sort
WARMUP_SCALING_TABLE.thresholds at initialization or use a non-mutating
toSorted() result) so the function uses an already-sorted thresholds list
instead of sorting in-place every time.
Benchmark PR from qodo-benchmark#242
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.