Updated the scaling factors for domain warmup#25526
Conversation
WalkthroughReplaces EmailModel Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
🧹 Nitpick comments (1)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (1)
130-130: Remove redundant sorting.The
WARMUP_SCALING_TABLE.thresholdsarray is already sorted in ascending order by limit in the constant definition, so the.sort()call is unnecessary overhead.Apply this diff:
- for (const threshold of WARMUP_SCALING_TABLE.thresholds.sort((a, b) => a.limit - b.limit)) { + for (const threshold of WARMUP_SCALING_TABLE.thresholds) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/services/email-service/DomainWarmingService.ts(2 hunks)ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
ghost/core/core/server/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Core Ghost backend logic belongs in
ghost/core/core/server/, with database schema inghost/core/core/server/data/schema/, API routes inghost/core/core/server/api/, services inghost/core/core/server/services/, and models inghost/core/core/server/models/
Files:
ghost/core/core/server/services/email-service/DomainWarmingService.ts
🧠 Learnings (1)
📚 Learning: 2025-11-24T17:29:43.840Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.840Z
Learning: Applies to e2e/**/*.test.ts : Use Playwright's auto-waiting capabilities and run tests multiple times to ensure stability
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
🪛 ESLint
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
[error] 198-198: Multiple spaces found before '// 200 × 1.25 ...'.
(no-multi-spaces)
[error] 199-199: Multiple spaces found before '// 500 × 1.25 ...'.
(no-multi-spaces)
[error] 200-200: Multiple spaces found before '// 1000 × 1.25...'.
(no-multi-spaces)
[error] 201-201: Multiple spaces found before '// 2000 × 1.5 ...'.
(no-multi-spaces)
[error] 202-202: Multiple spaces found before '// 5000 × 1.5 ...'.
(no-multi-spaces)
[error] 203-203: Multiple spaces found before '// 50000 × 1.7...'.
(no-multi-spaces)
[error] 208-208: Multiple spaces found before '// min(800000 ...'.
(no-multi-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (7)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (4)
23-27: LGTM! Clean type extension.The
highVolumeproperty is well-typed and clearly expresses the intent for high-volume scaling configuration.
30-41: Excellent documentation!The table format clearly explains the scaling behavior at each tier, making the domain warming strategy easy to understand.
48-64: LGTM! Configuration matches the documented strategy.The updated thresholds and scaling factors implement the conservative warmup approach described in the PR objectives, with appropriate caps for high-volume senders.
123-128: LGTM! High-volume cap logic is correct.The implementation correctly applies
min(1.2×, +75k)for volumes above 400k, preventing excessive jumps while still allowing growth.ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (3)
109-112: LGTM! Test correctly validates the scaling logic.The test expectation of 1000 is correct: with
lastCount=1000, the calculated limit is 1250 (1.25× scale), so requesting 1000 emails returns the fullemailCount.
127-128: LGTM! Correct cap behavior.The test correctly verifies that when
emailCountexceeds the calculated limit, the service returns the limit (1250).
187-209: Excellent comprehensive test coverage!The test cases thoroughly validate all scaling tiers and the high-volume cap behavior. All expected values are mathematically correct based on the implementation.
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
Outdated
Show resolved
Hide resolved
sam-lord
force-pushed
the
improved-warmup-scaling
branch
from
6659042 to
c5f012b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (2)
30-41: Clarify 400k boundary in docs vshighVolume.thresholdlogicThe implementation treats 400k as still in the 2× band and only applies the capped high‑volume rule for
lastCount > 400_000, but the comments/tables currently say “400k+”:
- Line 33–40 table:
100k – 400krow plus a400k+row.- Line 123: “For high volume senders (400k+)…”.
Given the tests expect
{lastCount: 400000, expected: 800000}and{lastCount: 500000, expected: 575000}, the effective behaviour is:
≤ 400k→ 2×> 400k→min(1.2×, +75k)To avoid confusion for future maintainers, consider updating the wording to make the exclusivity explicit (e.g. “> 400k” in the high‑volume description, or “100k – ≤400k” vs “>400k”). This keeps the docs tightly aligned with the actual boundary.
Also applies to: 123-128
14-28: High-volume config and thresholds look sound; consider avoiding in-placesortThe new
highVolumeconfiguration and expanded thresholds are consistent with the intended progression and the tests:
- Base: 200 for
lastCount ≤ 100.- 1.25× up to 1k, 1.5× up to 5k, 1.75× up to 100k, 2× up to 400k.
lastCount > 400kcapped viamaxScaleandmaxAbsoluteIncrease.One small maintainability nit:
WARMUP_SCALING_TABLE.thresholdsis already declared in ascendinglimitorder, but#getTargetLimitstill does:for (const threshold of WARMUP_SCALING_TABLE.thresholds.sort((a, b) => a.limit - b.limit)) {This:
- Re-sorts on every call (tiny cost but unnecessary).
- Mutates the shared
thresholdsarray in place.Optional tidy-up:
- Drop the
sortentirely and rely on the declared order; or- Sort once at definition time; or
- Iterate over a sorted copy:
[...WARMUP_SCALING_TABLE.thresholds].sort(...).Behaviour is correct as-is, so this is purely a cleanliness/refactor suggestion.
Also applies to: 47-59, 60-64, 130-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/services/email-service/DomainWarmingService.ts(2 hunks)ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
ghost/core/core/server/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Core Ghost backend logic belongs in
ghost/core/core/server/, with database schema inghost/core/core/server/data/schema/, API routes inghost/core/core/server/api/, services inghost/core/core/server/services/, and models inghost/core/core/server/models/
Files:
ghost/core/core/server/services/email-service/DomainWarmingService.ts
🧠 Learnings (3)
📚 Learning: 2025-10-30T17:13:26.190Z
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 25303
File: ghost/core/core/server/services/email-service/BatchSendingService.js:19-19
Timestamp: 2025-10-30T17:13:26.190Z
Learning: In ghost/core/core/server/services/email-service/BatchSendingService.js and similar files in the Ghost codebase, prefer using `{...options}` spread syntax without explicit guards like `...(options || {})` when spreading potentially undefined objects, as the maintainer prefers cleaner syntax over defensive patterns when the behavior is safe.
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
📚 Learning: 2025-11-24T17:29:43.840Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.840Z
Learning: Applies to e2e/**/*.{ts,tsx} : Prefer less comments and give things clear names
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
📚 Learning: 2025-11-24T17:29:43.840Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.840Z
Learning: Applies to e2e/**/*.test.ts : Use Playwright's auto-waiting capabilities and run tests multiple times to ensure stability
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (2)
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (2)
97-113: Behavioural tests forgetWarmupLimitnow correctly cover below/above-limit casesThe two tests:
should return emailCount when it is less than calculated limitshould return calculated limit when emailCount is greaternow accurately reflect the new 1.25× tier for
lastCount = 1000:
- Calculated limit:
ceil(1000 × 1.25) = 1250.emailCount = 1000→ returns 1000.emailCount = 5000→ returns 1250.This cleanly documents the min(emailCount, targetLimit) behaviour around a boundary and aligns with the updated service logic.
Also applies to: 115-129
185-208: Warmup progression test thoroughly matches the new scaling tableThe “should return correct warmup progression through the stages” test cases line up with the service’s configuration:
- Base 200 for
lastCount0, 50, 100.- 1.25× at 200, 500, 1000 → 250, 625, 1250.
- 1.5× at 2000, 5000 → 3000, 7500.
- 1.75× at 50,000 and 100,000 → 87,500 and 175,000.
- 2× at 200,000 and 400,000 → 400,000 and 800,000.
- High-volume caps at 500,000 and 800,000 → 575,000 and 875,000 via
min(1.2×, +75k).Using a large
emailCountto force the calculated limit to dominate is a nice touch, and the inline commentary makes the intent of each band clear. This should make future changes to the scaling table much safer.
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.
sam-lord
force-pushed
the
improved-warmup-scaling
branch
from
c5f012b to
573a04e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (1)
199-209: ESLint formatting violations remain from previous review.The multiple spaces used for comment alignment still violate the
no-multi-spacesrule as flagged in the previous review.
🧹 Nitpick comments (2)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (2)
124-129: Edge case:lastCountexactly at 400,000 uses 2× scaling, not high-volume cap.The condition
lastCount > WARMUP_SCALING_TABLE.highVolume.thresholdmeans exactly 400,000 falls through to the threshold loop and gets 2× scaling (800,000), not the high-volume cap. This appears intentional based on the test case{lastCount: 400000, expected: 800000}, but the comment "400k+" and table documentation "400k+ → min(1.2×, +75k)" could be clarified.Consider updating the comment for precision:
- // For high volume senders (400k+), cap the increase at 20% or 75k absolute + // For high volume senders (>400k), cap the increase at 20% or 75k absolute
131-135: Minor: Sorting thresholds at runtime is unnecessary.The
WARMUP_SCALING_TABLE.thresholdsarray is already sorted in ascending order in the constant definition (lines 47-59). The.sort()call on every invocation creates a new array and adds marginal overhead.- for (const threshold of WARMUP_SCALING_TABLE.thresholds.sort((a, b) => a.limit - b.limit)) { + for (const threshold of WARMUP_SCALING_TABLE.thresholds) {If you prefer defensive coding to guard against future reordering, consider adding a comment or sorting once at module load time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/core/core/server/services/email-service/DomainWarmingService.ts(4 hunks)ghost/core/test/integration/services/email-service/domain-warming.test.js(5 hunks)ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts(9 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-24T17:29:43.840Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.840Z
Learning: Applies to e2e/**/*.test.ts : Use factory pattern for all test data creation instead of hard-coded data or direct database manipulation
Applied to files:
ghost/core/test/integration/services/email-service/domain-warming.test.jsghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
📚 Learning: 2025-10-30T17:13:26.190Z
Learnt from: sam-lord
Repo: TryGhost/Ghost PR: 25303
File: ghost/core/core/server/services/email-service/BatchSendingService.js:19-19
Timestamp: 2025-10-30T17:13:26.190Z
Learning: In ghost/core/core/server/services/email-service/BatchSendingService.js and similar files in the Ghost codebase, prefer using `{...options}` spread syntax without explicit guards like `...(options || {})` when spreading potentially undefined objects, as the maintainer prefers cleaner syntax over defensive patterns when the behavior is safe.
Applied to files:
ghost/core/test/integration/services/email-service/domain-warming.test.jsghost/core/core/server/services/email-service/DomainWarmingService.tsghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
📚 Learning: 2025-11-24T17:29:43.840Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: e2e/AGENTS.md:0-0
Timestamp: 2025-11-24T17:29:43.840Z
Learning: Applies to e2e/**/*.{ts,tsx} : Prefer less comments and give things clear names
Applied to files:
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts
🧬 Code graph analysis (1)
ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (1)
ghost/core/core/server/services/email-service/DomainWarmingService.ts (1)
DomainWarmingService(67-140)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup
- GitHub Check: Build & Push
- GitHub Check: Setup
🔇 Additional comments (7)
ghost/core/test/integration/services/email-service/domain-warming.test.js (2)
53-66: LGTM! Good approach for deterministic test timing.The fixed
baseDatewithshouldAdvanceTime: trueensures consistent day progression while allowing time to advance naturally within test execution. This improves test reliability.
220-241: Verify Day 3 expected value calculation.The comment says
250 × 1.25 = 313, but250 × 1.25 = 312.5, which rounds to313withMath.ceil. The assertion is correct—just noting the fractional arithmetic for clarity.ghost/core/test/unit/server/services/email-service/domain-warming-service.test.ts (3)
163-184: LGTM! Correct verification offindPagebehavior.The test properly verifies that
findPageis called with the expected filter, order, and limit parameters. The assertions correctly check forlimit: 1which aligns with the implementation's paginated query approach.
186-227: Comprehensive test coverage for scaling progression.The test cases thoroughly cover the warmup scaling logic including:
- Base case (≤100 → 200)
- Conservative early ramp (1.25× up to 1k)
- Moderate increase (1.5× up to 5k)
- Faster ramp (1.75× up to 100k)
- Standard growth (2× up to 400k)
- High-volume cap (min(1.2×, +75k) for 400k+)
10-21: Remove this review comment - the concern is unfounded.The
createModelClassutility correctly provides bothfindAllandfindPagemethods. ThefindPageimplementation (lines 117-127 in utils/index.ts) uses the sameoptions.findAlldata and returns it in the correct format{data: pageData.map(...)}, which matches exactly what theDomainWarmingServiceexpects. The tests that configurecreateModelClass({findAll: [...]})work correctly because the utility properly adapts the array through both method interfaces.ghost/core/core/server/services/email-service/DomainWarmingService.ts (2)
100-113: LGTM! Clean migration tofindPage.The paginated approach with
limit: 1andorder: 'csd_email_count DESC'efficiently retrieves the highest count without loading all records. The null coalescing on line 112 properly handles bothnullandundefinedvalues.
30-65: Well-documented scaling configuration.The inline table documentation clearly explains the volume ranges and their corresponding multipliers. The type-safe
WarmupScalingTablestructure ensures configuration integrity.
| 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); | ||
| } | ||
| return Math.ceil(count * 1.75); | ||
| }; |
There was a problem hiding this comment.
Mismatch between test helper and implementation scaling thresholds.
The getExpectedScale helper uses different threshold boundaries than DomainWarmingService.ts. The implementation uses lastCount <= threshold.limit checks, but this helper applies different ranges:
| Helper Range | Implementation Range |
|---|---|
count <= 100 → 200 |
lastCount <= 100 → 200 ✓ |
count <= 1000 → 1.25× |
lastCount <= 1000 → 1.25× ✓ |
count <= 5000 → 1.5× |
lastCount <= 5000 → 1.5× ✓ |
count > 5000 → 1.75× |
lastCount <= 100_000 → 1.75× |
The helper returns 1.75× for counts above 5000 but doesn't handle the 100k-400k range (2×) or high-volume (400k+) logic. This will produce incorrect expected values if previousCsdCount exceeds 5000 in the test loop.
Consider aligning with the full implementation:
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);
}
- return Math.ceil(count * 1.75);
+ 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);
};📝 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 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); | |
| } | |
| return Math.ceil(count * 1.75); | |
| }; | |
| 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); | |
| }; |
2 participants
closes GVA-605
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.