fix: gracefully handle invalid family timezone instead of crashing (#2821)

* fix: gracefully handle invalid family timezone instead of crashing

Family#timezone is a free-text IANA zone name with no validation on
write. If it becomes stale (e.g. tzdata renames a zone, like the
historical Europe/Kiev -> Europe/Kyiv switch) or a migration meant to
remap legacy names never ran, Localize#switch_timezone passed the raw
string straight to Time.use_zone, which raises ArgumentError for any
unrecognized zone.

Since switch_timezone runs as an around_action on every request, this
crashed the entire app for the affected family, including the login
page.

Now validates the zone via ActiveSupport::TimeZone[] first and falls
back to the app default (logging a DebugLogEntry) instead of raising.
The log write is debounced per (family, bad value) via Rails.cache
(once per day) so an affected family doesn't write one DebugLogEntry
row per page view indefinitely.

Fixes #390

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix: address review feedback on timezone fallback

- Make the invalid-timezone debounce lease atomic. Rails.cache.fetch
  is read-then-write, not atomic, so two concurrent requests could
  both observe a cache miss and both log before either write landed.
  Rails.cache.write(unless_exist: true) maps to Redis's atomic SET NX
  in production, so only one request ever wins the lease.
  (via CodeRabbit)

- Stop using "Europe/Kiev" as the invalid-timezone value in tests.
  Whether ActiveSupport::TimeZone still resolves that legacy alias
  depends on the host's installed tzdata version (tzinfo-data is
  Windows/JRuby-only per Gemfile), so the test's pass/fail behavior
  wasn't deterministic across machines/CI. Use a deliberately
  nonexistent name instead.
  (via Codex)

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix: validate Family#timezone on write to address root cause of #390

The previous commit made the *crash* graceful, but left the actual
defect in place: nothing stopped an unrecognized IANA zone name from
being written to Family#timezone in the first place (direct DB/API
access, an old dump predating a tzdata rename, or a future rename of
a currently-valid zone).

Add a Family-level validation using the same ActiveSupport::TimeZone[]
lookup Localize#resolved_timezone uses at request time, so "valid at
save" and "valid when rendering" can't drift apart.

Deliberately not `inclusion: { in: ActiveSupport::TimeZone.all.map(&:name) }`,
matching the neighboring locale/date_format validations: verified
empirically that the settings form submits `tz.tzinfo.identifier` (e.g.
"America/New_York"), not `tz.name` (e.g. "Eastern Time (US & Canada)"),
and those differ for all 150 zones Rails ships. An inclusion check
against `.name` would have rejected every legitimate value the form
submits.

The validation only runs when timezone is actually being changed
(if: :timezone_changed?). A family with a pre-existing bad value (the
exact #390 scenario) must still be able to save unrelated changes --
otherwise this would turn a previously-harmless bad value into a
blocker for any other settings update or background job touching that
family's record.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

app/controllers/concerns/localize.rb

@@ -111,7 +111,49 @@ module Localize
     end
 
     def switch_timezone(&action)
-      timezone = Current.family.try(:timezone) || Time.zone
-      Time.use_zone(timezone, &action)
+      Time.use_zone(resolved_timezone, &action)
+    end
+
+    # How often to write a DebugLogEntry for the same (family, bad value) pair.
+    # switch_timezone runs on every request, so without this an affected
+    # family would write one row per page view forever.
+    INVALID_TIMEZONE_LOG_INTERVAL = 1.day
+
+    # Family#timezone is a free-text IANA name (e.g. from an older DB dump, or
+    # a zone the tzdata maintainers later renamed, like the historical
+    # "Europe/Kiev" -> "Europe/Kyiv" switch). `Time.use_zone` raises
+    # ArgumentError on anything it doesn't recognize, which would otherwise
+    # take down every request/render for the affected family -- including the
+    # login page, since this runs on every request. Validate first and fall
+    # back to the app default instead of crashing.
+    def resolved_timezone
+      family = Current.family
+      requested = family.try(:timezone)
+      return Time.zone if requested.blank?
+
+      zone = ActiveSupport::TimeZone[requested]
+      return zone if zone.present?
+
+      log_invalid_timezone_once(family, requested)
+      Time.zone
+    end
+
+    def log_invalid_timezone_once(family, requested)
+      cache_key = [ "invalid_family_timezone", family.id, requested ]
+
+      # `fetch` is read-then-write, not atomic -- two concurrent requests could
+      # both see a miss and both log. `write(unless_exist: true)` maps to
+      # Redis's atomic SET NX in production, so only one request ever wins the
+      # lease and logs.
+      lease_acquired = Rails.cache.write(cache_key, true, expires_in: INVALID_TIMEZONE_LOG_INTERVAL, unless_exist: true)
+      return unless lease_acquired
+
+      DebugLogEntry.capture(
+        category: "other",
+        level: "warn",
+        message: "Invalid family timezone #{requested.inspect}, falling back to #{Time.zone.name}",
+        source: "Localize#switch_timezone",
+        family: family
+      )
     end
 end

app/models/family.rb

@@ -144,6 +144,7 @@ class Family < ApplicationRecord
   validates :moniker, inclusion: { in: MONIKERS }
   validates :assistant_type, inclusion: { in: ASSISTANT_TYPES }
   validates :default_account_sharing, inclusion: { in: SHARING_DEFAULTS }
+  validate :timezone_must_be_a_known_zone, if: :timezone_changed?
 
   before_validation :normalize_enabled_currencies!
 
@@ -498,4 +499,28 @@ class Family < ApplicationRecord
     rescue Money::Currency::UnknownCurrencyError, ArgumentError
       nil
     end
+
+    # Not a plain `inclusion: { in: ActiveSupport::TimeZone.all.map(&:name) }`
+    # on purpose: the settings form submits `tz.tzinfo.identifier` (e.g.
+    # "America/New_York"), not `tz.name` (e.g. "Eastern Time (US & Canada)")
+    # -- see LanguagesHelper#timezone_options. For every zone Rails ships,
+    # those two differ, so an inclusion check against `.name` would reject
+    # every legitimate value the form actually submits. `ActiveSupport::TimeZone[]`
+    # resolves both forms, and is the same lookup `Localize#resolved_timezone`
+    # uses at request time, so "valid at save time" and "valid when rendering"
+    # can't drift apart.
+    #
+    # Only runs when timezone is actually being changed (see the `if:` on the
+    # `validate` call above). A family that already has a stale value from
+    # before this validation existed (the exact case in #390) must still be
+    # able to save unrelated changes -- e.g. a settings update, or any
+    # background job touching the record -- without being blocked by a field
+    # nobody is currently trying to set. That value still can't crash a
+    # request either way, since Localize#resolved_timezone falls back safely
+    # regardless of whether this validation ever ran.
+    def timezone_must_be_a_known_zone
+      return if timezone.blank?
+
+      errors.add(:timezone, :invalid) if ActiveSupport::TimeZone[timezone].blank?
+    end
 end

test/controllers/concerns/localize_test.rb

@@ -54,4 +54,63 @@ class LocalizeTest < ActionDispatch::IntegrationTest
     assert_response :success
     assert_select "h1", text: /Configure your preferences/i
   end
+
+  test "falls back to default timezone and logs a warning when family timezone is unrecognized" do
+    user = users(:family_admin)
+    # A deliberately nonexistent zone name, standing in for a stale/renamed IANA
+    # value slipping past validation (e.g. the historical "Europe/Kiev" ->
+    # "Europe/Kyiv" rename, or a migration that never ran). We can't use a real
+    # legacy alias like "Europe/Kiev" here: whether tzinfo still recognizes it
+    # depends on the host's installed tzdata version, which would make this
+    # test non-deterministic across machines/CI.
+    user.family.update_column(:timezone, "Invalid/Timezone")
+    sign_in user
+
+    assert_difference "DebugLogEntry.count", 1 do
+      get root_url
+    end
+
+    assert_response :success
+
+    entry = DebugLogEntry.order(:created_at).last
+    assert_equal "warn", entry.level
+    assert_includes entry.message, "Invalid/Timezone"
+    assert_equal user.family, entry.family
+  end
+
+  test "does not log when family timezone is valid" do
+    user = users(:family_admin)
+    user.family.update_column(:timezone, "America/New_York")
+    sign_in user
+
+    assert_no_difference "DebugLogEntry.count" do
+      get root_url
+    end
+
+    assert_response :success
+  end
+
+  test "does not log again on a second request within the debounce window" do
+    user = users(:family_admin)
+    user.family.update_column(:timezone, "Invalid/Timezone")
+    sign_in user
+
+    # The test environment's cache store is :null_store (config/environments/test.rb),
+    # which never actually caches anything -- every write is a no-op and every
+    # key looks nonexistent. Swap in a real store for this test so the
+    # debounce lease (Rails.cache.write unless_exist:) is meaningfully
+    # exercised instead of trivially passing.
+    original_cache = Rails.cache
+    Rails.cache = ActiveSupport::Cache::MemoryStore.new
+
+    assert_difference "DebugLogEntry.count", 1 do
+      get root_url
+      get root_url
+      get root_url
+    end
+
+    assert_response :success
+  ensure
+    Rails.cache = original_cache
+  end
 end

test/models/family_test.rb

@@ -352,6 +352,45 @@ class FamilyTest < ActiveSupport::TestCase
     assert_not_includes Family.with_preview_features, family
   end
 
+  test "rejects a timezone ActiveSupport::TimeZone doesn't recognize" do
+    family = families(:dylan_family)
+    family.timezone = "Invalid/Timezone"
+
+    assert_not family.valid?
+    assert_includes family.errors[:timezone], "is invalid"
+  end
+
+  test "accepts a timezone identifier, the form the settings dropdown actually submits" do
+    family = families(:dylan_family)
+    # LanguagesHelper#timezone_options submits tz.tzinfo.identifier (e.g.
+    # "America/New_York"), not tz.name (e.g. "Eastern Time (US & Canada)") --
+    # these differ for every zone Rails ships, so this is the case that
+    # actually matters, not just the display name.
+    family.timezone = "America/New_York"
+
+    assert family.valid?
+  end
+
+  test "allows a blank timezone" do
+    family = families(:dylan_family)
+    family.timezone = nil
+
+    assert family.valid?
+  end
+
+  test "does not re-validate an existing invalid timezone when saving unrelated changes" do
+    family = families(:dylan_family)
+    # Bypasses validations, simulating data that predates this validation --
+    # e.g. the exact #390 scenario (a stale/renamed IANA zone already sitting
+    # in the DB).
+    family.update_column(:timezone, "Invalid/Timezone")
+
+    family.name = "Updated name, timezone untouched"
+
+    assert family.valid?, "an unrelated change must not be blocked by a pre-existing bad timezone"
+    assert family.save
+  end
+
   private
     def set_preview_features(user, enabled)
       user.update!(preferences: (user.preferences || {}).merge("preview_features_enabled" => enabled))