Updated the scaling factors for domain warmup#3
Open
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.
📝 WalkthroughWalkthroughThe domain warming service is refactored to use paged email queries with 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
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}
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 131-138: The threshold comparison in the warm-up scaling loop is
exclusive which misclassifies exact boundary counts; update the check inside the
for-loop iterating over WARMUP_SCALING_TABLE.thresholds (and any similar logic)
to use an inclusive comparison (<=) when comparing lastCount with
threshold.limit so exact boundary values fall into the intended tier, and ensure
the final fallback still uses WARMUP_SCALING_TABLE.highVolume.maxScale
unchanged.
- Around line 101-112: Update the date filter in
DomainWarmingService.#getHighestCount to exclude today by using '<' instead of
'<=' when calling this.#emailModel.findPage and replace the loose null check on
the retrieved value from result.data[0].get('csd_email_count') with a strict
nullish check (e.g., use !== null/!== undefined or the nullish coalescing
operator) so you return the actual numeric count or 0 without violating eqeqeq.
In
`@ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts`:
- Around line 186-210: The expected value for the 400000 case is wrong given the
service's highVolume.threshold = 400_000; update the test in the "should return
correct warmup progression through the stages" unit (the testCases array) so the
entry {lastCount: 400000, expected: 800000} becomes {lastCount: 400000,
expected: 475000} (min(1.2×, lastCount + 75k) => min(480000, 475000) = 475000);
keep the rest of the cases unchanged.
Comment on lines
+101
to
+112
| 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.
Exclude today correctly and satisfy eqeqeq in #getHighestCount.
The filter currently uses <=, which doesn’t match the “excluding today” intent and fails the unit test expecting <. Also, count != null trips eqeqeq. Consider strict null checks.
🛠️ Proposed fix
- const result = await this.#emailModel.findPage({
- filter: `created_at:<=${new Date().toISOString().split('T')[0]}`,
+ const result = await this.#emailModel.findPage({
+ filter: `created_at:<${new Date().toISOString().split('T')[0]}`,
order: 'csd_email_count DESC',
limit: 1
});
@@
- 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 === null || count === undefined) ? 0 : count;📝 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.
Suggested change
| 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; | |
| 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 === undefined) ? 0 : count; |
🧰 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 101 - 112, Update the date filter in DomainWarmingService.#getHighestCount
to exclude today by using '<' instead of '<=' when calling
this.#emailModel.findPage and replace the loose null check on the retrieved
value from result.data[0].get('csd_email_count') with a strict nullish check
(e.g., use !== null/!== undefined or the nullish coalescing operator) so you
return the actual numeric count or 0 without violating eqeqeq.
Comment on lines
131
to
+138
| 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.
Make threshold checks inclusive to match docs/tests.
Using < shifts exact boundary values (e.g., 1,000) into the next tier, which contradicts the doc table and unit test expectations.
🛠️ Proposed fix
- 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 131 - 138, The threshold comparison in the warm-up scaling loop is
exclusive which misclassifies exact boundary counts; update the check inside the
for-loop iterating over WARMUP_SCALING_TABLE.thresholds (and any similar logic)
to use an inclusive comparison (<=) when comparing lastCount with
threshold.limit so exact boundary values fall into the intended tier, and ensure
the final fallback still uses WARMUP_SCALING_TABLE.highVolume.maxScale
unchanged.
Comment on lines
186
to
210
| it('should return correct warmup progression through the stages', async function () { | ||
| // Test the complete warmup progression | ||
| // New conservative scaling: | ||
| // - Base: 200 for counts ≤100 | ||
| // - 1.25× until 1k (conservative early ramp) | ||
| // - 1.5× until 5k (moderate increase) | ||
| // - 1.75× until 100k (faster ramp after proving deliverability) | ||
| // - 2× until 400k | ||
| // - High volume (400k+): min(1.2×, lastCount + 75k) to avoid huge jumps | ||
| const testCases = [ | ||
| {lastCount: 0, expected: 200}, | ||
| {lastCount: 50, expected: 200}, | ||
| {lastCount: 100, expected: 200}, | ||
| {lastCount: 200, expected: 400}, | ||
| {lastCount: 500, expected: 1000}, | ||
| {lastCount: 1000, expected: 2000}, | ||
| {lastCount: 50000, expected: 100000}, | ||
| {lastCount: 100000, expected: 200000}, | ||
| {lastCount: 200000, expected: 300000}, | ||
| {lastCount: 400000, expected: 600000}, | ||
| {lastCount: 500000, expected: 625000}, | ||
| {lastCount: 800000, expected: 1000000} | ||
| {lastCount: 200, expected: 250}, // 200 × 1.25 = 250 | ||
| {lastCount: 500, expected: 625}, // 500 × 1.25 = 625 | ||
| {lastCount: 1000, expected: 1250}, // 1000 × 1.25 = 1250 | ||
| {lastCount: 2000, expected: 3000}, // 2000 × 1.5 = 3000 | ||
| {lastCount: 5000, expected: 7500}, // 5000 × 1.5 = 7500 | ||
| {lastCount: 50000, expected: 87500}, // 50000 × 1.75 = 87500 | ||
| {lastCount: 100000, expected: 175000}, // 100000 × 1.75 = 175000 | ||
| {lastCount: 200000, expected: 400000}, // 200000 × 2 = 400000 | ||
| {lastCount: 400000, expected: 800000}, // 400000 × 2 = 800000 | ||
| {lastCount: 500000, expected: 575000}, // min(500000 × 1.2, 500000 + 75000) = min(600000, 575000) | ||
| {lastCount: 800000, expected: 875000} // min(800000 × 1.2, 800000 + 75000) = min(960000, 875000) | ||
| ]; |
There was a problem hiding this comment.
400k boundary expectation conflicts with the high‑volume cap.
With highVolume.threshold = 400_000, a lastCount of 400k should be capped to min(1.2×, +75k) (475k), not doubled. If you intended 2× at 400k, the service threshold/condition should change instead.
🛠️ Proposed fix (align with current service config)
- // - 2× until 400k
- // - High volume (400k+): min(1.2×, lastCount + 75k) to avoid huge jumps
+ // - 2× until <400k
+ // - High volume (>=400k): min(1.2×, lastCount + 75k) to avoid huge jumps
@@
- {lastCount: 400000, expected: 800000}, // 400000 × 2 = 800000
+ {lastCount: 400000, expected: 475000}, // min(400000 × 1.2, 400000 + 75000) = 475000📝 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.
Suggested change
| it('should return correct warmup progression through the stages', async function () { | |
| // Test the complete warmup progression | |
| // New conservative scaling: | |
| // - Base: 200 for counts ≤100 | |
| // - 1.25× until 1k (conservative early ramp) | |
| // - 1.5× until 5k (moderate increase) | |
| // - 1.75× until 100k (faster ramp after proving deliverability) | |
| // - 2× until 400k | |
| // - High volume (400k+): min(1.2×, lastCount + 75k) to avoid huge jumps | |
| const testCases = [ | |
| {lastCount: 0, expected: 200}, | |
| {lastCount: 50, expected: 200}, | |
| {lastCount: 100, expected: 200}, | |
| {lastCount: 200, expected: 400}, | |
| {lastCount: 500, expected: 1000}, | |
| {lastCount: 1000, expected: 2000}, | |
| {lastCount: 50000, expected: 100000}, | |
| {lastCount: 100000, expected: 200000}, | |
| {lastCount: 200000, expected: 300000}, | |
| {lastCount: 400000, expected: 600000}, | |
| {lastCount: 500000, expected: 625000}, | |
| {lastCount: 800000, expected: 1000000} | |
| {lastCount: 200, expected: 250}, // 200 × 1.25 = 250 | |
| {lastCount: 500, expected: 625}, // 500 × 1.25 = 625 | |
| {lastCount: 1000, expected: 1250}, // 1000 × 1.25 = 1250 | |
| {lastCount: 2000, expected: 3000}, // 2000 × 1.5 = 3000 | |
| {lastCount: 5000, expected: 7500}, // 5000 × 1.5 = 7500 | |
| {lastCount: 50000, expected: 87500}, // 50000 × 1.75 = 87500 | |
| {lastCount: 100000, expected: 175000}, // 100000 × 1.75 = 175000 | |
| {lastCount: 200000, expected: 400000}, // 200000 × 2 = 400000 | |
| {lastCount: 400000, expected: 800000}, // 400000 × 2 = 800000 | |
| {lastCount: 500000, expected: 575000}, // min(500000 × 1.2, 500000 + 75000) = min(600000, 575000) | |
| {lastCount: 800000, expected: 875000} // min(800000 × 1.2, 800000 + 75000) = min(960000, 875000) | |
| ]; | |
| it('should return correct warmup progression through the stages', async function () { | |
| // Test the complete warmup progression | |
| // New conservative scaling: | |
| // - Base: 200 for counts ≤100 | |
| // - 1.25× until 1k (conservative early ramp) | |
| // - 1.5× until 5k (moderate increase) | |
| // - 1.75× until 100k (faster ramp after proving deliverability) | |
| // - 2× until <400k | |
| // - High volume (>=400k): min(1.2×, lastCount + 75k) to avoid huge jumps | |
| const testCases = [ | |
| {lastCount: 0, expected: 200}, | |
| {lastCount: 50, expected: 200}, | |
| {lastCount: 100, expected: 200}, | |
| {lastCount: 200, expected: 250}, // 200 × 1.25 = 250 | |
| {lastCount: 500, expected: 625}, // 500 × 1.25 = 625 | |
| {lastCount: 1000, expected: 1250}, // 1000 × 1.25 = 1250 | |
| {lastCount: 2000, expected: 3000}, // 2000 × 1.5 = 3000 | |
| {lastCount: 5000, expected: 7500}, // 5000 × 1.5 = 7500 | |
| {lastCount: 50000, expected: 87500}, // 50000 × 1.75 = 87500 | |
| {lastCount: 100000, expected: 175000}, // 100000 × 1.75 = 175000 | |
| {lastCount: 200000, expected: 400000}, // 200000 × 2 = 400000 | |
| {lastCount: 400000, expected: 475000}, // min(400000 × 1.2, 400000 + 75000) = 475000 | |
| {lastCount: 500000, expected: 575000}, // min(500000 × 1.2, 500000 + 75000) = min(600000, 575000) | |
| {lastCount: 800000, expected: 875000} // min(800000 × 1.2, 800000 + 75000) = min(960000, 875000) | |
| ]; |
🤖 Prompt for AI Agents
In
`@ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts`
around lines 186 - 210, The expected value for the 400000 case is wrong given
the service's highVolume.threshold = 400_000; update the test in the "should
return correct warmup progression through the stages" unit (the testCases array)
so the entry {lastCount: 400000, expected: 800000} becomes {lastCount: 400000,
expected: 475000} (min(1.2×, lastCount + 75k) => min(480000, 475000) = 475000);
keep the rest of the cases unchanged.
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.
Benchmark PR from agentic-review-benchmarks#3
Summary by CodeRabbit
New Features
Bug Fixes
Word count: 28 words
✏️ Tip: You can customize this high-level summary in your review settings.