Skip to content

chore: store release schedule without timezone #878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kitallis wants to merge 2 commits into
base: main
Choose a base branch
from
Open

chore: store release schedule without timezone #878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f1d746
refactor: make train scheduling timezone-agnostic using kickoff_time
claude Nov 18, 2025
ed2eb57
refactor: simplify next_run_at logic and centralize timezone conversion
claude Nov 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/components/release_list_component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ def no_release_empty_state
else
{
title: "Upcoming release",
text: "Your first scheduled release will automatically kick-off at #{train.kickoff_at.to_fs(:short)}. You can also manually run a new release by clicking the prepare button."
text: "Your first scheduled release will automatically kick-off at #{train.next_run_at.in_time_zone(train.app.timezone).to_fs(:short)}. You can also manually run a new release by clicking the prepare button."
}
end
else
Expand Down
8 changes: 4 additions & 4 deletions app/controllers/trains_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def train_params
:versioning_strategy,
:release_backmerge_branch,
:release_branch,
:kickoff_at,
:kickoff_time,
:repeat_duration_value,
:repeat_duration_unit,
:notification_channel,
Expand Down Expand Up @@ -166,7 +166,7 @@ def train_update_params
:build_queue_size,
:build_queue_wait_time_unit,
:build_queue_wait_time_value,
:kickoff_at,
:kickoff_time,
:repeat_duration_value,
:repeat_duration_unit,
:release_schedule_enabled,
Expand Down Expand Up @@ -244,15 +244,15 @@ def build_queue_config(config_params)
end

def release_schedule_config_params
[:release_schedule_enabled, :kickoff_at, :repeat_duration_value, :repeat_duration_unit, :stop_automatic_releases_on_failure]
[:release_schedule_enabled, :kickoff_time, :repeat_duration_value, :repeat_duration_unit, :stop_automatic_releases_on_failure]
end

def release_schedule_config(config_params)
return if config_params[:release_schedule_enabled].blank?

{
repeat_duration: parsed_duration(config_params[:repeat_duration_value], config_params[:repeat_duration_unit]),
kickoff_at: config_params[:kickoff_at]&.time_in_utc,
kickoff_time: config_params[:kickoff_time],
stop_automatic_releases_on_failure: config_params[:stop_automatic_releases_on_failure]
}
end
Expand Down
6 changes: 3 additions & 3 deletions app/helpers/trains_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
module TrainsHelper
def release_schedule(train)
if train.automatic?
date = time_format(train.kickoff_at, with_year: true, with_time: false)
duration = train.repeat_duration.inspect
time = train.kickoff_at.strftime("%I:%M%p (%Z)")
"Kickoff at #{date} – runs every #{duration} at #{time}"
time = train.kickoff_time.strftime("%I:%M%p")
timezone = train.app.timezone
"Runs every #{duration} at #{time} (#{timezone})"
else
"No release schedule"
end
Expand Down
51 changes: 37 additions & 14 deletions app/models/train.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
# description :string
# enable_changelog_linking_in_notifications :boolean default(FALSE)
# freeze_version :boolean default(FALSE)
# kickoff_at :datetime
# kickoff_time :time
# name :string not null
# notification_channel :jsonb
# notifications_release_specific_channel_enabled :boolean default(FALSE)
Expand Down Expand Up @@ -116,7 +116,7 @@
validates :release_branch, presence: true, if: -> { branching_strategy == "parallel_working" }
validate :version_compatibility, on: :create
validate :ready?, on: :create
validate :valid_schedule, if: -> { kickoff_at_changed? || repeat_duration_changed? }
validate :valid_schedule, if: -> { kickoff_time_changed? || repeat_duration_changed? }
validate :build_queue_config
validate :backmerge_config
validate :working_branch_presence, on: :create
Expand Down Expand Up @@ -146,7 +146,7 @@
before_update :create_default_notification_settings, if: -> do
notification_channel_changed? || notifications_release_specific_channel_enabled_changed?
end
after_update :schedule_release!, if: -> { kickoff_at.present? && kickoff_at_previously_was.blank? }
after_update :schedule_release!, if: -> { kickoff_time.present? && kickoff_time_previously_was.blank? }

def disable_copy_approvals
self.copy_approvals = false
Expand Down Expand Up @@ -213,28 +213,52 @@
end

def automatic?
kickoff_at.present? && repeat_duration.present?
kickoff_time.present? && repeat_duration.present?
end

def next_run_at
return unless automatic?

base_time = last_run_at
now = Time.current
# Get the app's timezone
tz = app.timezone

return base_time if now < base_time
# If we have previous scheduled releases, calculate next occurrence from last scheduled time
if scheduled_releases.any?
base_time = scheduled_releases.last.scheduled_at
next_time = base_time + repeat_duration

time_difference = now - base_time
elapsed_durations = (time_difference / repeat_duration.to_i).ceil
base_time + (repeat_duration.to_i * elapsed_durations)
# Keep adding repeat_duration until we get a future time
while next_time <= Time.current
next_time += repeat_duration
end

return next_time.utc
end

# First time: combine kickoff_time with today's date in app timezone
now_in_tz = Time.current.in_time_zone(tz)

# Parse kickoff_time as a time today in the app's timezone
# kickoff_time is stored as HH:MM:SS
kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime('%H:%M:%S')}")

Check failure on line 243 in app/models/train.rb

View workflow job for this annotation

GitHub Actions / lint 

Style/StringLiteralsInInterpolation: Prefer double-quoted strings inside interpolations.

# If kickoff_today already passed, start from the next occurrence
next_time = kickoff_today
while next_time <= now_in_tz
next_time += repeat_duration
end

next_time.utc
end
Comment on lines 219 to 230
Copy link

@coderabbitai coderabbitai bot Nov 19, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Ensure recurring runs stay at the same local time across DST + address lint hint

The new next_run_at correctly uses app.timezone for the first occurrence, but when there are existing scheduled_releases it does:

base_time = scheduled_releases.last.scheduled_at
next_time = base_time + repeat_duration

while next_time <= Time.current
  next_time += repeat_duration
end

Because scheduled_at is stored in UTC, subsequent runs are computed purely in UTC. On DST transitions this will shift the wall-clock time (e.g., daily runs may move by ±1 hour), which conflicts with the stated goal of “same local time regardless of DST”.

You can keep all recurrence math in the app’s timezone (for both first and subsequent runs) and only convert to UTC at the end. This also lets you reuse the same now_in_tz and fixes the string-literal-in-interpolation lint by switching to double quotes:

   def next_run_at
     return unless automatic?
 
-    # Get the app's timezone
-    tz = app.timezone
-
-    # If we have previous scheduled releases, calculate next occurrence from last scheduled time
-    if scheduled_releases.any?
-      base_time = scheduled_releases.last.scheduled_at
-      next_time = base_time + repeat_duration
-
-      # Keep adding repeat_duration until we get a future time
-      while next_time <= Time.current
-        next_time += repeat_duration
-      end
-
-      return next_time.utc
-    end
-
-    # First time: combine kickoff_time with today's date in app timezone
-    now_in_tz = Time.current.in_time_zone(tz)
-
-    # Parse kickoff_time as a time today in the app's timezone
-    # kickoff_time is stored as HH:MM:SS
-    kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime('%H:%M:%S')}")
-
-    # If kickoff_today already passed, start from the next occurrence
-    next_time = kickoff_today
-    while next_time <= now_in_tz
-      next_time += repeat_duration
-    end
-
-    next_time.utc
+    tz = app.timezone
+    now_in_tz = Time.current.in_time_zone(tz)
+
+    if scheduled_releases.any?
+      # Work in app timezone so recurring runs stay at the same local time
+      base_time_in_tz = scheduled_releases.last.scheduled_at.in_time_zone(tz)
+      next_time_in_tz = base_time_in_tz + repeat_duration
+
+      while next_time_in_tz <= now_in_tz
+        next_time_in_tz += repeat_duration
+      end
+
+      return next_time_in_tz.utc
+    end
+
+    # First time: combine kickoff_time with today's date in app timezone
+    kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime("%H:%M:%S")}")
+
+    next_time_in_tz = kickoff_today
+    while next_time_in_tz <= now_in_tz
+      next_time_in_tz += repeat_duration
+    end
+
+    next_time_in_tz.utc
   end

This keeps all scheduled runs anchored to the same local wall-clock time in each app’s timezone and also satisfies the StringLiteralsInInterpolation lint.

I'll verify the review comment by examining the actual code and understanding the timezone/DST concerns.

🏁 Script executed:

#!/bin/bash
# Examine the current next_run_at implementation
cat app/models/train.rb | sed -n '219,252p'

Length of output: 1030

🏁 Script executed:

#!/bin/bash
# Check the full context around the method and related timezone logic
cat app/models/train.rb | sed -n '200,260p'

Length of output: 1655

🏁 Script executed:

#!/bin/bash
# Find related timezone and scheduling code
rg -n "scheduled_releases|kickoff_time|repeat_duration" app/models/train.rb -B2 -A2

Length of output: 4059

🏁 Script executed:

#!/bin/bash
# Check if there are tests for next_run_at behavior
fd "train" app/models --type f | xargs rg -l "next_run_at" 2>/dev/null | head -5

Length of output: 19

🏁 Script executed:

#!/bin/bash
# Find test files for train model
fd ".*train.*test\|.*train.*spec" --type f | head -10

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check the Migration or schema to confirm scheduled_at storage
rg -n "scheduled_at" --type ruby app/models app/migrations -B2 -A2 | head -20

Length of output: 1078

🏁 Script executed:

#!/bin/bash
# Verify the Ruby lint rule about string literals in interpolation
rg -n "StringLiteralsInInterpolation" . --type ruby 2>/dev/null | head -5

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check repeat_duration data type and how it's used
rg -n "repeat_duration" app/models/train.rb -B3 -A3

Length of output: 2907

🏁 Script executed:

#!/bin/bash
# Check Time.current vs now_in_tz usage for consistency
rg -n "Time\.current|now_in_tz" app/models/train.rb -B1 -A1

Length of output: 544

🏁 Script executed:

#!/bin/bash
# Verify automatic? method definition and its dependencies
rg -n "def automatic\?|kickoff_time\.|repeat_duration\." app/models/train.rb -B1 -A2 | head -30

Length of output: 1127

🏁 Script executed:

#!/bin/bash
# Check if there's a timezone attribute on the app model
rg -n "def timezone|timezone" app/models/app.rb -B1 -A2 | head -20

Length of output: 240

🏁 Script executed:

#!/bin/bash
# Look for DST-related tests or comments in the codebase
rg -i "dst|daylight.*saving" app/models app/spec -C3 2>/dev/null

Length of output: 5199

🏁 Script executed:

#!/bin/bash
# Examine the scheduled_releases association and scheduled_at attribute
rg -n "has_many.*scheduled_releases|class ScheduledRelease" app/models/train.rb app/models/scheduled_release.rb -B2 -A5

Length of output: 1083

🏁 Script executed:

#!/bin/bash
# Check the ScheduledRelease model structure
test -f app/models/scheduled_release.rb && cat app/models/scheduled_release.rb | head -50

Length of output: 1358

🏁 Script executed:

#!/bin/bash
# Verify if scheduled_at is a datetime stored in UTC
rg -n "scheduled_at" app/models/scheduled_release.rb -B2 -A2

Length of output: 997

🏁 Script executed:

#!/bin/bash
# Look for any existing timezone conversion in the code
rg -n "in_time_zone|\.utc|\.local" app/models/train.rb -B1 -A1 | head -40

Length of output: 222

🏁 Script executed:

#!/bin/bash
# Check for Rails version and available timezone utilities
test -f Gemfile && grep -i "rails\|activesupport" Gemfile | head -5

Length of output: 144

🏁 Script executed:

#!/bin/bash
# Verify lint configuration
test -f .rubocop.yml && cat .rubocop.yml | grep -A10 "StringLiteralsInInterpolation" 2>/dev/null || echo "No .rubocop.yml or rule not found"

Length of output: 33

🏁 Script executed:

#!/bin/bash
# Check the actual diff to understand what changed
git diff HEAD^ app/models/train.rb | head -100

Length of output: 4062

🏁 Script executed:

#!/bin/bash
# Review the validation and callback for kickoff_time changes
rg -n "valid_schedule|schedule_release\!" app/models/train.rb -B3 -A5

Length of output: 130

🏁 Script executed:

#!/bin/bash
# Check if runnable? method uses next_run_at
rg -n "def runnable\?" app/models/train.rb -B2 -A10

Length of output: 312

🏁 Script executed:

#!/bin/bash
# Look for any existing DST handling or timezone-aware scheduling
rg -n "DST|next_occurrence|scheduled.*time" app/models/ -i --type ruby | head -20

Length of output: 1550

🏁 Script executed:

#!/bin/bash
# Verify the exact timezone attribute structure
rg -n "timezone" app/models/ -C2 --type ruby | grep -E "(attr|column|def)" | head -15

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check the parsing logic to ensure tz.parse is valid
rg -n "tz\.parse|ActiveSupport::TimeZone" app/models/train.rb -B1 -A1

Length of output: 143

🏁 Script executed:

#!/bin/bash
# Look at the proposed change more carefully - verify in_time_zone chaining
ast-grep --pattern 'in_time_zone($_)'

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Double-check if Time values stored in DB are UTC by default in Rails
rg -n "create_table.*scheduled_release\|t\.datetime" db/migrate -B5 -A10 | head -30

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Verify repeat_duration is stored as a duration/interval
rg -n "repeat_duration" app/models/train.rb | head -5

Length of output: 356

🏁 Script executed:

#!/bin/bash
# Check how repeat_duration is used in addition operations
rg -n "repeat_duration" app/models/train.rb -A5 -B1 | grep -E "(\+|-|\*|repeat)" | head -10

Length of output: 680

🏁 Script executed:

#!/bin/bash
# Check if there are any existing issues or TODOs related to timezone handling
rg -n "TODO|FIXME|BUG|XXX" app/models/train.rb | grep -i "time\|zone\|dst"

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Verify the exact line numbers and check if code matches the review
sed -n '219,252p' app/models/train.rb | wc -l

Length of output: 2

🏁 Script executed:

#!/bin/bash
# Get more context - check the entire next_run_at method
ast-grep --pattern 'def next_run_at { $$$ }'

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Last validation - verify strftime usage with quotes
grep -n "strftime.*%" app/models/train.rb | head -5

Length of output: 93

🏁 Script executed:

#!/bin/bash
# Check if the current branch/PR actually contains the method in question
git log --oneline -1 && git diff --name-only HEAD~1 HEAD 2>/dev/null | grep train.rb

Length of output: 97

🏁 Script executed:

#!/bin/bash
# See if the original comment hints at what was before this change
git show HEAD:app/models/train.rb 2>/dev/null | sed -n '219,252p' || echo "Cannot access previous version"

Length of output: 1030

🏁 Script executed:

#!/bin/bash
# Inspect the full train.rb file structure
wc -l app/models/train.rb

Length of output: 23

🏁 Script executed:

#!/bin/bash
# Ensure file exists
test -f app/models/train.rb && echo "File exists" || echo "File does not exist"

Length of output: 11

🏁 Script executed:

#!/bin/bash
# List the app/models directory
ls -la app/models/ | head -20

Length of output: 1287

🏁 Script executed:

#!/bin/bash
# Check if this is the right repo structure
pwd && ls -la | head -20

Length of output: 1222

🏁 Script executed:

#!/bin/bash
# Verify git status
git status --short 2>/dev/null | head -10

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Get repo info
git remote -v 2>/dev/null || echo "No git remote"

Length of output: 126

🏁 Script executed:

#!/bin/bash
# Check branch info
git branch -v 2>/dev/null || echo "Not a git repo or no branches"

Length of output: 172

🏁 Script executed:

#!/bin/bash
# Try to see recent changes
git log --oneline -5 2>/dev/null || echo "Cannot access git log"

Length of output: 334

🏁 Script executed:

#!/bin/bash
# List all Ruby model files
find app/models -name "*.rb" -type f 2>/dev/null

Length of output: 2898

🏁 Script executed:

#!/bin/bash
# Final check: examine the actual code structure
cat app/models/train.rb 2>/dev/null | head -50 || echo "Cannot read file"

Length of output: 3442

🏁 Script executed:

#!/bin/bash
# Alternative: use find to locate the file
find . -name "train.rb" -path "*/models/*" 2>/dev/null

Length of output: 21

🏁 Script executed:

#!/bin/bash
# Check current directory structure
ls -la

Length of output: 3149

🏁 Script executed:

#!/bin/bash
# Check if we have access to app directory
test -d app && ls -la app/ || echo "No app directory"

Length of output: 930

🏁 Script executed:

#!/bin/bash
# Final sanity check
pwd && find . -maxdepth 3 -type d -name models 2>/dev/null

Length of output: 45

🏁 Script executed:

#!/bin/bash
# List everything we can find
ls -R . | head -100 || echo "Cannot list"

Length of output: 1101

🏁 Script executed:

#!/bin/bash
# Check environment
env | grep -i ruby || echo "Check failed"

Length of output: 12

🏁 Script executed:

#!/bin/bash
# Simple directory listing
ls 2>/dev/null || pwd

Length of output: 281

Recurring runs will drift from kickoff time across DST transitions; consolidate timezone handling and fix lint issue

The new next_run_at correctly uses app.timezone for the first occurrence, but when scheduled_releases exist, it performs all recurrence math in UTC:

base_time = scheduled_releases.last.scheduled_at       # UTC datetime
next_time = base_time + repeat_duration                # UTC math
while next_time <= Time.current                         # UTC comparison

Because scheduled_at is stored as UTC, subsequent runs bypass timezone logic. On DST transitions this shifts the wall-clock time (e.g., a daily 9 AM run may become 10 AM after spring forward), contradicting the stated goal of "same local time regardless of DST".

Convert the scheduled_releases branch to use app timezone for all recurrence math and only convert to UTC at the end:

   def next_run_at
     return unless automatic?
 
-    # Get the app's timezone
-    tz = app.timezone
-
-    # If we have previous scheduled releases, calculate next occurrence from last scheduled time
-    if scheduled_releases.any?
-      base_time = scheduled_releases.last.scheduled_at
-      next_time = base_time + repeat_duration
-
-      # Keep adding repeat_duration until we get a future time
-      while next_time <= Time.current
-        next_time += repeat_duration
-      end
-
-      return next_time.utc
-    end
-
-    # First time: combine kickoff_time with today's date in app timezone
-    now_in_tz = Time.current.in_time_zone(tz)
-
-    # Parse kickoff_time as a time today in the app's timezone
-    # kickoff_time is stored as HH:MM:SS
-    kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime('%H:%M:%S')}")
-
-    # If kickoff_today already passed, start from the next occurrence
-    next_time = kickoff_today
-    while next_time <= now_in_tz
-      next_time += repeat_duration
-    end
-
-    next_time.utc
+    tz = app.timezone
+    now_in_tz = Time.current.in_time_zone(tz)
+
+    if scheduled_releases.any?
+      # Work in app timezone so recurring runs stay at the same local time
+      base_time_in_tz = scheduled_releases.last.scheduled_at.in_time_zone(tz)
+      next_time_in_tz = base_time_in_tz + repeat_duration
+
+      while next_time_in_tz <= now_in_tz
+        next_time_in_tz += repeat_duration
+      end
+
+      return next_time_in_tz.utc
+    end
+
+    # First time: combine kickoff_time with today's date in app timezone
+    kickoff_today = tz.parse("#{now_in_tz.to_date} #{kickoff_time.strftime("%H:%M:%S")}")
+
+    next_time_in_tz = kickoff_today
+    while next_time_in_tz <= now_in_tz
+      next_time_in_tz += repeat_duration
+    end
+
+    next_time_in_tz.utc
   end

This keeps all scheduled runs anchored to the same local wall-clock time in the app's timezone and also addresses the StringLiteralsInInterpolation lint hint by using double quotes.

🧰 Tools
🪛 GitHub Check: lint

[failure] 243-243:
Style/StringLiteralsInInterpolation: Prefer double-quoted strings inside interpolations.

🤖 Prompt for AI Agents 
In app/models/train.rb around lines 219-252, the scheduled_releases branch does
recurrence math in UTC which causes DST drift and the interpolation uses
single-quoted string literals; convert the last scheduled_at into the app
timezone and do all recurrence additions and comparisons in that Time.zone (e.g.
base_time_in_tz = scheduled_releases.last.scheduled_at.in_time_zone(tz)), loop
adding repeat_duration to that time until it's after now_in_tz (use now_in_tz =
Time.current.in_time_zone(tz)), then convert the resulting TimeWithZone to UTC
for return; also change any string literal in interpolation (e.g. the
kickoff_time.strftime format) to use double quotes to satisfy
StringLiteralsInInterpolation.


def runnable?
return false unless next_run_at
return true if last_run_at.nil? # First run is always runnable
next_run_at > last_run_at
end

def last_run_at
scheduled_releases.last&.scheduled_at || kickoff_at
scheduled_releases.last&.scheduled_at
end

def diff_since_last_release?
Expand Down Expand Up @@ -586,9 +610,8 @@
end

def valid_schedule
if kickoff_at.present? || repeat_duration.present?
errors.add(:repeat_duration, "invalid schedule, provide both kickoff and period for repeat") unless kickoff_at.present? && repeat_duration.present?
errors.add(:kickoff_at, "the schedule kickoff should be in the future") if kickoff_at && kickoff_at <= Time.current
if kickoff_time.present? || repeat_duration.present?
errors.add(:repeat_duration, "invalid schedule, provide both kickoff time and period for repeat") unless kickoff_time.present? && repeat_duration.present?
errors.add(:repeat_duration, "the repeat duration should be more than 1 day") if repeat_duration && repeat_duration < 1.day
end
end
Expand Down
9 changes: 4 additions & 5 deletions app/views/trains/_release_schedule_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,10 @@
<% component.with_child do %>
<div class="grid gap-y-4 gap-x-2 md:grid-cols-3">
<div class="col-span-1">
<%= form.labeled_datetime_field :kickoff_at,
"Kickoff at",
{min: Date.current,
data: {action: "domain--release-schedule-help#change",
domain__release_schedule_help_target: "kickoffDate"}} %>
<%= form.labeled_time_field :kickoff_time,
"Kickoff time",
{data: {action: "domain--release-schedule-help#change",
domain__release_schedule_help_target: "kickoffTime"}} %>
</div>
<div class="col-span-2">
<%= form.label_only :repeat_duration, "Every" %>
Expand Down
2 changes: 1 addition & 1 deletion db/data/20240305124235_connect_scheduled_releases_with_release.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ def up
return
ActiveRecord::Base.transaction do
# scan all automatic trains
Train.where.not(kickoff_at: nil).where.not(repeat_duration: nil).each do |train|
Train.where.not(kickoff_time: nil).where.not(repeat_duration: nil).each do |train|
# check redundantly to be sure
next unless train.automatic?

Expand Down
54 changes: 54 additions & 0 deletions db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
# frozen_string_literal: true

class ChangeKickoffAtToKickoffTime < ActiveRecord::Migration[7.0]
def up
# Step 1: Add new kickoff_time column (time without timezone)
add_column :trains, :kickoff_time, :time

# Step 2: Migrate existing data
# Convert kickoff_at (UTC datetime) to time in app's timezone
reversible do |dir|
dir.up do
Train.reset_column_information
Train.find_each do |train|
next if train.kickoff_at.blank?

# Convert kickoff_at (UTC) to app's timezone and extract time component
time_in_app_tz = train.kickoff_at.in_time_zone(train.app.timezone)
train.update_column(:kickoff_time, time_in_app_tz.strftime("%H:%M:%S"))
end
end
end
Comment on lines +10 to +21
Copy link

@coderabbitai coderabbitai bot Nov 19, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Down-migration parsing bug for kickoff_time + minor migration cleanup

Two points here:

  1. Down path: malformed parse string (bug)
    In the down direction you build the datetime with:

    kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")

    For a time column, train.kickoff_time will typically stringify with its own date and zone, so the resulting string is ambiguous (e.g., "2025-11-19 2000-01-01 11:00:00 UTC"), and tz.parse may misinterpret or fail. You already do the right thing in Train#next_run_at by formatting only the time portion.

    I’d strongly recommend mirroring that here:

    

    2. 



    

  • 

         kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")

    

    • 



    

  • 

         kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time.strftime("%H:%M:%S")}")

    

    
This makes the down migration deterministic and matches the semantics used in the model.


    

    • 



    

  1. 

    Optional: avoid N+1 on app lookups
    
Both up and down use Train.find_each and then hit train.app.timezone, which will issue an extra query per train. If you expect a large trains table, you can cheaply remove this N+1:
    

    

    2. 



    

  • 

       Train.reset_column_information

    

    • 

  • 

       Train.find_each do |train|

    

    • 



    

  • 

       Train.reset_column_information

    

    • 

  • 

       Train.includes(:app).find_each do |train|

    

    
(Same pattern in the down block.)

This keeps behavior identical but scales better on larger datasets.







    

    • 



Also applies to: 34-52




🏁 Script executed:


#!/bin/bash
# Retrieve the full migration file to inspect down block
find . -name "20251118165553_change_kickoff_at_to_kickoff_time.rb" -type f -exec cat {} \;


Length of output: 1764




🏁 Script executed:


#!/bin/bash
# Find Train model to check next_run_at method
fd -e rb -path "*/models/*" -name "*train*" -type f | head -20


Length of output: 168




🏁 Script executed:


#!/bin/bash
# Search for next_run_at method in Train model
rg -n "def next_run_at" --type=rb -A 5


Length of output: 26




🏁 Script executed:


#!/bin/bash
# Check how time columns are typically used in migrations/models
rg -n "\.strftime\(" --type=rb -C 2 | grep -A 2 -B 2 "time"


Length of output: 26




🏁 Script executed:


#!/bin/bash
# Find Train model files
find . -name "*train*" -type f | grep -E "models?/" | head -10


Length of output: 49




🏁 Script executed:


#!/bin/bash
# Search for next_run_at method without type flag
rg -n "def next_run_at" -A 5


Length of output: 649




🏁 Script executed:


#!/bin/bash
# Search for how time columns are used in codebase
rg -n "kickoff_time" -C 3


Length of output: 20885




🏁 Script executed:


#!/bin/bash
# Look for strftime usage with time values to understand pattern
rg "strftime.*%H:%M:%S" -C 2


Length of output: 8379






Down-migration parsing bug for kickoff_time + minor migration cleanup


Two points here:


    

  1. 

    Down path: malformed parse string (bug)
    
In the down direction you build the datetime with:
    

    kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")
    

    For a time column, train.kickoff_time when interpolated lacks explicit formatting. The codebase consistently uses strftime("%H:%M:%S") when parsing—as seen in Train#next_run_at (line 243) and throughout specs. Update to match this pattern:
    

    

    2. 



    

  • 

         kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")

    

    • 



    

  • 

         kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time.strftime("%H:%M:%S")}")

    

    
This ensures the down migration is deterministic and consistent with the model logic.


    

    • 



    

  1. 

    Optional: avoid N+1 on app lookups
    
Both up and down use Train.find_each and then hit train.app.timezone, which issues an extra query per train. Add eager loading:
    

    

    2. 



    

  • 

       Train.reset_column_information

    

    • 

  • 

       Train.find_each do |train|

    

    • 



    

  • 

       Train.reset_column_information

    

    • 

  • 

       Train.includes(:app).find_each do |train|

    

    
(Same pattern in the down block.)


    

    • 




🤖 Prompt for AI Agents

In db/migrate/20251118165553_change_kickoff_at_to_kickoff_time.rb around lines
10 to 21, the down migration builds a datetime using an unformatted kickoff_time
string which can parse inconsistently; change the interpolation to use the same
formatting pattern used elsewhere (strftime("%H:%M:%S")) when reconstructing the
time so parsing is deterministic, and to avoid N+1 queries preload apps for both
up and down (use a query that eager-loads the associated app before iterating,
e.g., include the app association) so train.app.timezone does not hit the DB per
record.






    

  
  




    


        

      
  
  

    
  
    
    

  

  

  



          

      
    








    


  









  


    


  		



    

    

    

  
    







    

    

    

  
        # Step 3: Remove old column






    

    

    

  
        safety_assured { remove_column :trains, :kickoff_at }






    

    

    

  
      end






    

    

    

  
    







    

    

    

  
      def down






    

    

    

  
        # Add back the datetime column






    

    

    

  
        add_column :trains, :kickoff_at, :datetime






    

    

    

  
    







    

    

    

  
        # Attempt to reverse the migration






    

    

    

  
        # Note: We can't perfectly reverse since we lose the date component






    

    

    

  
        # We'll set it to the next occurrence from now






    

    

    

  
        reversible do |dir|






    

    

    

  
          dir.down do






    

    

    

  
            Train.reset_column_information






    

    

    

  
            Train.find_each do |train|






    

    

    

  
              next if train.kickoff_time.blank?






    

    

    

  
    







    

    

    

  
              # Set to next occurrence from now in app's timezone






    

    

    

  
              tz = train.app.timezone






    

    

    

  
              now = Time.current.in_time_zone(tz)






    

    

    

  
              kickoff_today = tz.parse("#{now.to_date} #{train.kickoff_time}")






    

    

    

  
              kickoff_datetime = kickoff_today > now ? kickoff_today : kickoff_today + 1.day






    

    

    

  
    







    

    

    

  
              train.update_column(:kickoff_at, kickoff_datetime.utc)






    

    

    

  
            end






    

    

    

  
          end






    

    

    

  
        end






    

    

    

  
    







    

    

    

  
        # Remove the time column






    

    

    

  
        safety_assured { remove_column :trains, :kickoff_time }






    

    

    

  
      end






    

    

    

  
    end








                
              


          


    

  



    
  

    

      

        


          

            
            
          


          



          
            2 changes: 1 addition & 1 deletion
          
          

        

  lib/tasks/anonymize.rake

  
  
      
      
  





        
      


      

        











            


          

          

            
              

                
    

              

            
            
                            

                            


                              
   View file



                                    


                                  


                            


                            

                            

    Open in desktop



                
            
          

        

      

    



    



          

            




              
      
    
              

                
                  

                      Original file line number
                      Diff line number
                      Diff line change
                  

                
                
                      
      

    
          
            
          
          Expand Up
    
    @@ -49,7 +49,7 @@ namespace :anonymize do
  


    

    

    

  
            whitelist "name", "slug", "description", "status", "branching_strategy", "version_seeded_with", "version_current",






    

    

    

  
              "repeat_duration", "build_queue_wait_time", "build_queue_size", "backmerge_strategy", "manual_release",






    

    

    

  
              "tag_store_releases_with_platform_names", "tag_store_releases", "compact_build_notes", "tag_end_of_release", "build_queue_enabled",






    

    

    

  
              "kickoff_at", "versioning_strategy", "send_build_notes", "notifications_release_specific_channel_enabled",






    

    

    

  
              "kickoff_time", "versioning_strategy", "send_build_notes", "notifications_release_specific_channel_enabled",






    

    

    

  
              "tag_end_of_release_prefix", "tag_end_of_release_suffix", "version_bump_strategy", "enable_changelog_linking_in_notifications",






    

    

    

  
              "release_branch_pattern", "webhooks_enabled"






    

    

    

  
            whitelist_timestamps






  

    
          
            
          
          Expand Down
    
    
  



                
              


          


    

  



    
  

    

      

        


          

            
            
          


          



          
            16 changes: 8 additions & 8 deletions
          
          

        

  spec/components/scheduled_train_component_spec.rb

  
  
      
      
  





        
      


      

        











            


          

          

            
              

                
    

              

            
            
                            

                            


                              
   View file



                                    


                                  


                            


                            

                            

    Open in desktop



                
            
          

        

      

    



    



          

            




              
      
    
              

                
                  

                      Original file line number
                      Diff line number
                      Diff line change
                  

                
                
                      
      

    
          
            
          
          Expand Up
    
    @@ -41,7 +41,7 @@
  


    

    

    

  
                    version_seeded_with: "2025.01.01",






    

    

    

  
                    versioning_strategy: :calver,






    

    

    

  
                    repeat_duration: 1.day,






    

    

    

  
                    kickoff_at: creation_time + 1.hour)






    

    

    

  
                    kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S"))






    

    

    

  
    







    

    

    

  
                  create(:scheduled_release, train:, scheduled_at: scheduled_time)






    

    

    

  
                end






      

    
        
          
        
        Expand All
    
    @@ -66,7 +66,7 @@
  


    

    

    

  
                    version_seeded_with: "2025.01.01",






    

    

    

  
                    versioning_strategy: :calver,






    

    

    

  
                    repeat_duration:,






    

    

    

  
                    kickoff_at: creation_time + 1.hour)






    

    

    

  
                    kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S"))






    

    

    

  
                  create(:scheduled_release, train:, scheduled_at: scheduled_time)






    

    

    

  
                end






    

    

    

  
    







      

    
        
          
        
        Expand All
    
    @@ -93,7 +93,7 @@
  


    

    

    

  
                    version_seeded_with: "2025.01.01",






    

    

    

  
                    versioning_strategy: :calver,






    

    

    

  
                    repeat_duration: 1.day,






    

    

    

  
                    kickoff_at: creation_time + 1.hour)






    

    

    

  
                    kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S"))






    

    

    

  
                  scheduled_release = create(:scheduled_release, train:, scheduled_at: scheduled_time)






    

    

    

  
                  create(:release, :on_track, train:, original_release_version: "2025.02.01", scheduled_release:)






    

    

    

  
                end






      

    
        
          
        
        Expand All
    
    @@ -118,7 +118,7 @@
  


    

    

    

  
                    version_seeded_with: "2025.01.01",






    

    

    

  
                    versioning_strategy: :calver,






    

    

    

  
                    repeat_duration:,






    

    

    

  
                    kickoff_at: creation_time + 1.hour)






    

    

    

  
                    kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S"))






    

    

    

  
                  scheduled_release = create(:scheduled_release, train:, scheduled_at: scheduled_time)






    

    

    

  
                  create(:release, :on_track, train:, original_release_version: "2025.02.01", scheduled_release:)






    

    

    

  
                end






      

    
          
            
          
          Expand Down
          
            
          
          Expand Up
    
    @@ -171,7 +171,7 @@
  


    

    

    

  
                    version_seeded_with: "2025.01.01",






    

    

    

  
                    versioning_strategy: :calver,






    

    

    

  
                    repeat_duration: 1.day,






    

    

    

  
                    kickoff_at: creation_time + 1.hour)






    

    

    

  
                    kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S"))






    

    

    

  
    







    

    

    

  
                  create(:scheduled_release, train:, scheduled_at: scheduled_time)






    

    

    

  
                end






      

    
        
          
        
        Expand All
    
    @@ -196,7 +196,7 @@
  


    

    

    

  
                    version_seeded_with: "2025.01.01",






    

    

    

  
                    versioning_strategy: :calver,






    

    

    

  
                    repeat_duration:,






    

    

    

  
                    kickoff_at: creation_time + 1.hour)






    

    

    

  
                    kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S"))






    

    

    

  
                  create(:scheduled_release, train:, scheduled_at: scheduled_time)






    

    

    

  
                end






    

    

    

  
    







      

    
        
          
        
        Expand All
    
    @@ -223,7 +223,7 @@
  


    

    

    

  
                    version_seeded_with: "2025.01.01",






    

    

    

  
                    versioning_strategy: :calver,






    

    

    

  
                    repeat_duration: 1.day,






    

    

    

  
                    kickoff_at: creation_time + 1.hour)






    

    

    

  
                    kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S"))






    

    

    

  
                  scheduled_release = create(:scheduled_release, train:, scheduled_at: scheduled_time)






    

    

    

  
                  create(:release, :on_track, train:, original_release_version: "2025.02.01", scheduled_release:)






    

    

    

  
                end






      

    
        
          
        
        Expand All
    
    @@ -248,7 +248,7 @@
  


    

    

    

  
                    version_seeded_with: "2025.01.01",






    

    

    

  
                    versioning_strategy: :calver,






    

    

    

  
                    repeat_duration:,






    

    

    

  
                    kickoff_at: creation_time + 1.hour)






    

    

    

  
                    kickoff_time: (creation_time + 1.hour).strftime("%H:%M:%S"))






    

    

    

  
                  scheduled_release = create(:scheduled_release, train:, scheduled_at: scheduled_time)






    

    

    

  
                  create(:release, :on_track, train:, original_release_version: "2025.02.01", scheduled_release:)






    

    

    

  
                end






  

    
          
            
          
          Expand Down
    
    
  



                
              


          


    

  



    
  

    

      

        


          

            
            
          


          



          
            2 changes: 1 addition & 1 deletion
          
          

        

  spec/factories/trains.rb

  
  
      
      
  





        
      


      

        











            


          

          

            
              

                
    

              

            
            
                            

                              


                            


                              
   View file



                                    


                                  


                            


                            

                            

    Open in desktop



                
            
          

        

      

    



    



          

            




              
      
      
    
              

                
                  

                      Original file line number
                      Diff line number
                      Diff line change
                  

                
                
                      
      

    
          
            
          
          Expand Up
    
    @@ -49,7 +49,7 @@
  


    

    

    

  
    







    

    

    

  
        trait :with_schedule do






    

    

    

  
          branching_strategy { "almost_trunk" }






    

    

    

  
          kickoff_at { 2.hours.from_now }






    

    

    

  
          kickoff_time { (Time.current + 2.hours).strftime("%H:%M:%S") }





    

  
        

  

  
      
  
  
    Check failure
        on line 52
        in spec/factories/trains.rb
  




  

      
View workflow job for this annotation

  


  

    
    GitHub Actions / lint
  


  

    
      
    
  


  

    
Rails/DurationArithmetic: Do not add or subtract duration.

  






  		




    

    

    

  
          repeat_duration { 1.day }






    

    

    

  
        end






    

    

    

  
    







  

    
          
            
          
          Expand Down
    
    
  



                
              


          


    

  



    
  

    

      

        


          

            
            
          


          



          
            19 changes: 14 additions & 5 deletions
          
          

        

  spec/jobs/schedule_train_releases_job_spec.rb

  
  
      
      
  





        
      


      

        











            


          

          

            
              

                
    

              

            
            
                            

                            


                              
   View file



                                    


                                  


                            


                            

                            

    Open in desktop



                
            
          

        

      

    



    



          

            




              
      
    
              

                
                  

                      Original file line number
                      Diff line number
                      Diff line change
                  

                
                
                      
      

    
          
            
          
          Expand Up
    
    @@ -4,12 +4,16 @@
  


    

    

    

  
    describe ScheduleTrainReleasesJob do






    

    

    

  
      it "schedules release for active trains" do






    

    

    

  
        train = create(:train, :active, :with_schedule)






    

    

    

  
        train.scheduled_releases.create!(scheduled_at: train.kickoff_at)






    

    

    

  
        tz = train.app.timezone






    

    

    

  
    







    

    

    

  
        travel_to train.kickoff_at + 1.hour do






    

    

    

  
        # Create a scheduled release in the past






    

    

    

  
        first_scheduled = 2.hours.ago.in_time_zone(tz).change(hour: 14, min: 0, sec: 0).utc






    

    

    

  
        train.scheduled_releases.create!(scheduled_at: first_scheduled)






    

    

    

  
    







    

    

    

  
        travel_to first_scheduled + 1.hour do






    

    

    

  
          described_class.new.perform






    

    

    

  
          expect(train.reload.scheduled_releases.count).to eq(2)






    

    

    

  
          expect(train.reload.scheduled_releases.last.scheduled_at).to eq(train.kickoff_at + train.repeat_duration)






    

    

    

  
          expect(train.reload.scheduled_releases.last.scheduled_at).to eq(first_scheduled + train.repeat_duration)






    

    

    

  
        end






    

    

    

  
      end






    

    

    

  
    







      

    
        
          
        
        Expand All
    
    @@ -23,8 +27,13 @@
  


    

    

    

  
      end






    

    

    

  
    







    

    

    

  
      it "does not schedule release for active trains with nothing to schedule" do






    

    

    

  
        train = create(:train, :with_almost_trunk, :active, kickoff_at: Time.current + 2.hours, repeat_duration: 5.days)






    

    

    

  
        train.scheduled_releases.create!(scheduled_at: train.kickoff_at)






    

    

    

  
        future_time = (Time.current + 2.hours).strftime("%H:%M:%S")






    

    

    

  
        train = create(:train, :with_almost_trunk, :active, kickoff_time: future_time, repeat_duration: 5.days)






    

    

    

  
        tz = train.app.timezone






    

    

    

  
    







    

    

    

  
        # Create a scheduled release in the future






    

    

    

  
        future_scheduled = Time.current.in_time_zone(tz).change(hour: Time.current.hour + 2).utc






    

    

    

  
        train.scheduled_releases.create!(scheduled_at: future_scheduled)






    

    

    

  
    







    

    

    

  
        expect(train.scheduled_releases.count).to eq(1)






    

    

    

  
    







  

    
          
            
          
          Expand Down
    
    
  



                
              


          


    

  



  


    

  
  
    
      Loading


    

  




    

  
  
    
      Loading