Switched to time-based domain warmup#20
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| const count = result.data[0].get('csd_email_count'); | ||
| return count || 0; | ||
| return Math.ceil((Date.now() - new Date(res.data[0].get('created_at') as string).getTime()) / (1000 * 60 * 60 * 24)); |
There was a problem hiding this comment.
Bug: The use of Math.ceil() to calculate the warmup day causes the day count to increment prematurely, even for emails sent hours apart on the same calendar day.
Severity: MEDIUM
Suggested Fix
Replace Math.ceil() with Math.floor() when calculating the days elapsed. This will correctly treat any time within the first 24-hour period as day 0, the second 24-hour period as day 1, and so on, aligning with the intended warmup schedule.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: ghost/core/core/server/services/email-service/DomainWarmingService.ts#L75
Potential issue: The calculation for the number of days since the first email uses
`Math.ceil()`. This causes any fractional day to be rounded up to the next whole day.
For example, if a first email is sent at 10 AM and a second is sent at 2 PM on the same
calendar day, the time elapsed is a small fraction of a day (e.g., 0.167).
`Math.ceil(0.167)` results in `1`, incorrectly advancing the warmup schedule to day 1.
The intended behavior is for all emails sent within the same 24-hour period of the first
email to be considered day 0.
Did we get this right? 👍 / 👎 to inform future reviews.
| return Math.min(scaledIncrease, absoluteIncrease); | ||
| async getWarmupLimit(emailCount: number): Promise<number> { | ||
| const day = await this.#getDaysSinceFirstEmail() | ||
| if (day > this.#warmupConfig.totalDays) { |
There was a problem hiding this comment.
Bug: The condition day > this.#warmupConfig.totalDays is an off-by-one error. It should be day >= this.#warmupConfig.totalDays to correctly end the warmup period after 42 days.
Severity: MEDIUM
Suggested Fix
Change the condition at line 85 from day > this.#warmupConfig.totalDays to day >= this.#warmupConfig.totalDays. This ensures that on day 42 (and after), the function correctly returns Infinity, aligning with the intended 42-day warmup schedule.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: ghost/core/core/server/services/email-service/DomainWarmingService.ts#L85
Potential issue: The domain warming logic contains an off-by-one error. The check `if
(day > this.#warmupConfig.totalDays)` allows the limit calculation to run on day 42,
when the warmup period should have already completed. The warmup period is intended to
run for 42 days (indexed 0-41). On day 42, the service should return `Infinity` to
indicate the warmup is over, but instead it calculates a finite limit. This contradicts
the behavior explicitly expected by the unit tests.
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
Benchmark PR from qodo-benchmark#236