Code Review Bench PR #25526 - Updated the scaling factors for domain warmup#4
Code Review Bench PR #25526 - Updated the scaling factors for domain warmup#4ketkarameya wants to merge 2 commits intobase_pr_25526_20260125_9231from
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.
Code Review 🚫 Blocked 0 resolved / 4 findingsTwo critical off-by-one bugs in threshold boundary comparisons will cause incorrect email volume scaling at every tier boundary (1k, 5k, 100k, 400k), and all related unit tests will fail. The 🚨 Bug: Off-by-one:
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
![gitar-bot-staging[bot]](https://avatars.githubusercontent.com/in/812680?s=60&v=4){kind=small}
|
|
||
| for (const threshold of WARMUP_SCALING_TABLE.thresholds.sort((a, b) => a.limit - b.limit)) { | ||
| if (lastCount <= threshold.limit) { | ||
| if (lastCount < threshold.limit) { |
There was a problem hiding this comment.
🚨 Bug: Off-by-one: < should be <= in threshold comparison
The change from <= to < on line 132 causes every threshold boundary value to be assigned to the wrong scaling tier. For example:
lastCount = 1000:1000 < 1000is false, falls through to the 5k bucket → returns1500(1.5×) instead of the expected1250(1.25×)lastCount = 5000:5000 < 5000is false, falls through to 100k bucket → returns8750(1.75×) instead of7500(1.5×)lastCount = 100000: falls through to 400k bucket → returns200000(2×) instead of175000(1.75×)
This contradicts the documented behavior in the comment table (lines 33-40), the test expectations, and the PR's stated scaling tiers. The unit tests for boundary values will fail.
Fix: Change < back to <= on line 132:
if (lastCount <= threshold.limit) {Was this helpful? React with 👍 / 👎
| if (lastCount < threshold.limit) { | |
| if (lastCount <= threshold.limit) { |
- Apply suggested fix
| } | ||
|
|
||
| // For high volume senders (400k+), cap the increase at 20% or 75k absolute | ||
| if (lastCount >= WARMUP_SCALING_TABLE.highVolume.threshold) { |
There was a problem hiding this comment.
🚨 Bug: High volume check >= steals 400k from threshold table
On line 125, the high volume check uses >= which means lastCount = 400000 enters the high volume path instead of matching the 400k threshold (scale 2×) in the threshold table.
Trace for lastCount = 400000:
400000 >= 400000→ true → enters high volume pathscaledIncrease = Math.ceil(400000 * 1.2) = 480000absoluteIncrease = 400000 + 75000 = 475000- Returns
Math.min(480000, 475000) = 475000
But the test expects 800000 (400000 × 2), which is what the 400k threshold with scale 2 would produce.
The high volume cap is intended for volumes above 400k, not at exactly 400k. The comment on line 124 even says "400k+" but uses >= which includes 400k itself.
Fix: Change >= to > on line 125:
if (lastCount > WARMUP_SCALING_TABLE.highVolume.threshold) {Combined with the <= fix in the threshold loop, this ensures lastCount = 400000 matches the 400k threshold (2×) and only values strictly above 400k enter the high volume cap.
Was this helpful? React with 👍 / 👎
| if (lastCount >= WARMUP_SCALING_TABLE.highVolume.threshold) { | |
| if (lastCount > WARMUP_SCALING_TABLE.highVolume.threshold) { |
- Apply suggested fix
| 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]}`, |
There was a problem hiding this comment.
⚠️ Bug: Filter changed from < to <= may include today's emails
The filter was changed from created_at:<${today} to created_at:<=${today} (line 102), but the JSDoc on line 98 still says "excluding today."
The original < operator excluded all records from today. The new <= operator may include today's records depending on how the ORM compares timestamps against date-only strings. If the ORM interprets <=2026-02-15 as <= 2026-02-15T00:00:00.000Z, records created at midnight would be included. If it interprets it as <= 2026-02-15T23:59:59.999Z, all of today's records would be included.
Including today's emails in the "highest count" query could cause the warmup limit to reflect the current send rather than historical data, defeating the purpose of progressive warming. The test on line 181 also still asserts created_at:< (without =), so the test won't catch this.
Fix: Revert to < to maintain the documented "excluding today" behavior:
filter: `created_at:<${new Date().toISOString().split('T')[0]}`,Was this helpful? React with 👍 / 👎
| filter: `created_at:<=${new Date().toISOString().split('T')[0]}`, | |
| filter: `created_at:<${new Date().toISOString().split('T')[0]}`, |
- Apply suggested fix
3 participants
Benchmark PR for Gitar evaluation
Original PR: agentic-review-benchmarks#3
Updates the scaling factors used for domain warmup configuration.
Summary by Gitar
findOne()with paginatedfindPage()API for future scalability; addslimitparameter<to inclusive<=for accurate historical countcsd_email_countedge casesThis will update automatically on new commits.