Skip to content

Switched to time-based domain warmup#22

Open
hila-f-qodo wants to merge 2 commits intodevin_pr_code_review_bench_100_devin2_base_switched_to_time-based_domain_warmup_pr236from
devin_pr_code_review_bench_100_devin2_head_switched_to_time-based_domain_warmup_pr236
Open

Switched to time-based domain warmup#22
hila-f-qodo wants to merge 2 commits intodevin_pr_code_review_bench_100_devin2_base_switched_to_time-based_domain_warmup_pr236from
devin_pr_code_review_bench_100_devin2_head_switched_to_time-based_domain_warmup_pr236

Conversation

@hila-f-qodo
Copy link
Copy Markdown

@hila-f-qodo hila-f-qodo commented Jan 26, 2026

Benchmark PR from qodo-benchmark#236


Open with Devin

Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View issue and 5 additional flags in Devin Review.

Open in Devin Review

return Math.min(scaledIncrease, absoluteIncrease);
async getWarmupLimit(emailCount: number): Promise<number> {
const day = await this.#getDaysSinceFirstEmail()
if (day > this.#warmupConfig.totalDays) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Off-by-one error in warmup completion check causes day 42 to not return Infinity

The warmup completion check uses day > this.#warmupConfig.totalDays but should use >= to correctly mark warmup as complete.

Click to expand

Problem

With totalDays = 42, the formula day / (totalDays - 1) is designed so that:

  • Day 0 → 0/41 = 0 → returns start value (200)
  • Day 41 → 41/41 = 1 → returns end value (200000)
  • Day 42+ → should return Infinity (warmup complete)

However, the condition at line 85 is day > this.#warmupConfig.totalDays, which means:

  • 42 > 42 evaluates to false
  • Day 42 calculates: 200 * (1000)^(42/41) ≈ 236701 instead of returning Infinity

Actual vs Expected

// Day 42 with current code:
day > 42  // 42 > 42 = false, so continues to calculate
limit = 236701

// Day 42 with fix:
day >= 42  // 42 >= 42 = true, returns Infinity

Impact

After 42 days of warmup, the service should allow unlimited emails from the custom domain. Instead, it continues calculating limits, which could restrict email sending unnecessarily. The unit test at domain-warming-service.test.ts:284-301 expects Infinity for day 42 and would fail with the current implementation.

Recommendation: Change if (day > this.#warmupConfig.totalDays) to if (day >= this.#warmupConfig.totalDays) to correctly return Infinity when the warmup period is complete.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants