timeinterval: fix negative days_of_month across DST transitions - #5447
Merged
Conversation
…ions daysInMonth derived the month length by dividing the elapsed duration between the first of the month and the first of the next month by 24 hours. In a location whose month contains a spring-forward daylight saving transition that span is 743 wall-clock hours, not 744, so the truncating division returns 30 for a 31-day month. ContainsTime feeds that count into negative days_of_month resolution (for example -1 for the last day of the month), so with a non-UTC location set, a "last day" mute resolves to the 30th during a spring-forward month: it fails to fire on the true last day (31st) and fires a day early on the 30th. Compute the count with calendar arithmetic instead: day 0 of the following month normalizes to the last day of the current month, which is DST-independent. Add a regression case covering -1 in America/New_York across the March spring-forward transition. Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesCalendar Month Length
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change corrects negative month-day resolution across spring-forward DST transitions and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
SoloJacobs
requested changes
Aug 21, 2026
SoloJacobs
left a comment
SoloJacobs
left a comment
Contributor
There was a problem hiding this comment.
Overall a nice change, just few nits on my part
| // Day 0 of the following month normalizes to the last day of t's month, | ||
| // so its day-of-month is the number of days in the month. This is | ||
| // calendar arithmetic and stays correct across daylight-saving | ||
| // transitions. Deriving the count from an elapsed duration instead |
Contributor
There was a problem hiding this comment.
There is no need to explain the previous method.
| // transitions. Deriving the count from an elapsed duration instead | ||
| // (monthEnd.Sub(monthStart).Hours()/24) undercounts by one in a | ||
| // spring-forward month, which spans 743 wall-clock hours rather than 744. | ||
| return time.Date(t.Year(), t.Month()+1, 0, 0, 0, 0, 0, t.Location()).Day() |
Contributor
There was a problem hiding this comment.
Let's do
return time.Date(t.Year(), t.Month()+1, 0, 12, 0, 0, 0, t.Location()).Day()Better safe than sorry.
| }, | ||
| }, | ||
| { | ||
| // Negative days of month must resolve against the true month length, |
Contributor
There was a problem hiding this comment.
I don't think this comment adds much. It should be concise and readable without the context of the PR
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
Contributor
Author
|
Thanks, pushed 3fa310a to address the review nits: removed the extra explanatory comments and changed daysInMonth to use noon for the day-zero calculation. I reran:
|
SoloJacobs
approved these changes
Aug 22, 2026
suprjinx
pushed a commit
to suprjinx/alertmanager
that referenced
this pull request
Aug 31, 2026
…etheus#5447) daysInMonth derived the month length by dividing the elapsed duration between the first of the month and the first of the next month by 24 hours. In a location whose month contains a spring-forward daylight saving transition that span is 743 wall-clock hours, not 744, so the truncating division returns 30 for a 31-day month. ContainsTime feeds that count into negative days_of_month resolution (for example -1 for the last day of the month), so with a non-UTC location set, a "last day" mute resolves to the 30th during a spring-forward month: it fails to fire on the true last day (31st) and fires a day early on the 30th. Compute the count with calendar arithmetic instead: day 0 of the following month normalizes to the last day of the current month, which is DST-independent. Add a regression case covering -1 in America/New_York across the March spring-forward transition. Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
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
3 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.
daysInMonthcomputes a month's length by dividing the elapsed duration between the first of the month and the first of the next month by 24 hours. When alocationis set and that month contains a spring-forward DST transition, the span is 743 wall-clock hours rather than 744, so the truncating division returns 30 for a 31-day month.ContainsTimeuses that count to resolve negativedays_of_monthentries such as-1(the last day of the month). So a "last day of the month" mute with a non-UTC location resolves to the 30th during a spring-forward month: it does not fire on the actual last day (the 31st) and fires a day early on the 30th.Reproduced with
days_of_month: ['-1']andlocation: America/New_Yorkfor March 2021 (spring-forward on the 14th): before the changeContainsTime(Mar 31)is false andContainsTime(Mar 30)is true; after, it is the reverse. A non-DST month (May) is unaffected either way.The fix computes the count with calendar arithmetic — day 0 of the following month is the last day of the current month — which does not depend on DST. I added a regression case to
timeIntervalTestCasescovering-1inAmerica/New_Yorkacross the March transition; it fails before and passes after.Tested with
go test ./timeinterval/...plus the dependentnotify,config, anddispatchpackages.