From 272d4bcdc911d9bade3cae529b83a32e91cec616 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Wed, 18 Mar 2026 14:54:52 -0500 Subject: [PATCH] fix: check state never persisted due to missing cast fields (#73) Check.changeset/2 did not include state fields (current_state, current_state_type, current_check_attempt, last_check_at, etc.) in its cast list. Ecto silently dropped all state updates, so update_check_state succeeded but wrote nothing to the database. Added Check.state_changeset/2 for internal state machine updates, keeping the regular changeset safe from user input. Updated Monitoring.update_check_state/3 to use it directly. Reviewed-on: https://git.mcintire.me/graham/towerops-web/pulls/73 --- lib/towerops/monitoring.ex | 12 ++++- lib/towerops/monitoring/check.ex | 20 ++++++++ lib/towerops/workers/check_executor_worker.ex | 10 ++-- test/towerops/monitoring_test.exs | 46 ++++++++++++++----- 4 files changed, 70 insertions(+), 18 deletions(-) diff --git a/lib/towerops/monitoring.ex b/lib/towerops/monitoring.ex index 0ab168b9..cc2a08ba 100644 --- a/lib/towerops/monitoring.ex +++ b/lib/towerops/monitoring.ex @@ -287,7 +287,17 @@ defmodule Towerops.Monitoring do attrs end - update_check(check, attrs) + result = + check + |> Check.state_changeset(attrs) + |> Repo.update() + + case result do + {:ok, updated} -> broadcast_check_change(updated) + _ -> :ok + end + + result end defp calculate_state_transition(check, _new_status, true = _state_changed) do diff --git a/lib/towerops/monitoring/check.ex b/lib/towerops/monitoring/check.ex index e451cc3c..3647ac8c 100644 --- a/lib/towerops/monitoring/check.ex +++ b/lib/towerops/monitoring/check.ex @@ -197,6 +197,26 @@ defmodule Towerops.Monitoring.Check do |> Base.url_encode64(padding: false) end + @doc """ + Changeset for internal state updates (soft/hard state transitions). + + Allows updating check execution state fields that are not permitted + through the regular changeset (which protects them from user input). + """ + def state_changeset(check, attrs) do + cast(check, attrs, [ + :current_state, + :current_state_type, + :current_check_attempt, + :last_check_at, + :last_state_change_at, + :last_hard_state_change_at, + :is_flapping, + :flapping_state_changes, + :flapping_window_start_at + ]) + end + def state_label(0), do: "OK" def state_label(1), do: "WARNING" def state_label(2), do: "CRITICAL" diff --git a/lib/towerops/workers/check_executor_worker.ex b/lib/towerops/workers/check_executor_worker.ex index e5108d36..210c20d9 100644 --- a/lib/towerops/workers/check_executor_worker.ex +++ b/lib/towerops/workers/check_executor_worker.ex @@ -143,7 +143,7 @@ defmodule Towerops.Workers.CheckExecutorWorker do {:ok, response_time, output} -> {:ok, %{ - value: nil, + value: response_time, status: 0, output: output, response_time_ms: response_time @@ -160,7 +160,7 @@ defmodule Towerops.Workers.CheckExecutorWorker do {:ok, response_time, output} -> {:ok, %{ - value: nil, + value: response_time, status: 0, output: output, response_time_ms: response_time @@ -177,7 +177,7 @@ defmodule Towerops.Workers.CheckExecutorWorker do {:ok, response_time, output} -> {:ok, %{ - value: nil, + value: response_time, status: 0, output: output, response_time_ms: response_time @@ -194,7 +194,7 @@ defmodule Towerops.Workers.CheckExecutorWorker do {:ok, status, response_time, output} -> {:ok, %{ - value: nil, + value: response_time, status: status, output: output, response_time_ms: response_time @@ -211,7 +211,7 @@ defmodule Towerops.Workers.CheckExecutorWorker do {:ok, response_time, output} -> {:ok, %{ - value: nil, + value: response_time, status: 0, output: output, response_time_ms: response_time diff --git a/test/towerops/monitoring_test.exs b/test/towerops/monitoring_test.exs index 4ce3f0ce..4e019940 100644 --- a/test/towerops/monitoring_test.exs +++ b/test/towerops/monitoring_test.exs @@ -423,27 +423,49 @@ defmodule Towerops.MonitoringTest do end describe "update_check_state/3" do - # NOTE: Check.changeset does not cast state fields (current_state, - # current_state_type, current_check_attempt, last_check_at, etc.). - # update_check_state computes correct transitions but the changeset - # silently drops them. These tests document the actual behavior. - - test "succeeds but state fields are not persisted via changeset", %{organization: org} do + test "persists state fields to database", %{organization: org} do {:ok, check} = Monitoring.create_check(valid_check_attrs(org.id)) - # The function returns {:ok, check} but state fields remain at defaults assert {:ok, updated} = Monitoring.update_check_state(check, 0, "OK") - # current_state stays at 3 (default) because changeset doesn't cast it - assert updated.current_state == 3 - assert updated.current_state_type == "soft" + assert updated.current_state == 0 + assert updated.last_check_at + + # Verify persisted to DB + reloaded = Monitoring.get_check!(check.id) + assert reloaded.current_state == 0 + assert reloaded.last_check_at end - test "does not crash on repeated calls", %{organization: org} do + test "tracks soft/hard state transitions", %{organization: org} do + {:ok, check} = Monitoring.create_check(valid_check_attrs(org.id)) + # Default: state=3 (UNKNOWN), type="soft", attempt=1, max_attempts=3 + + # First call: state changes 3->0, attempt=2, still soft + assert {:ok, check} = Monitoring.update_check_state(check, 0, "OK") + assert check.current_state == 0 + assert check.current_state_type == "soft" + assert check.current_check_attempt == 2 + + # Second call: same state (0->0), attempt reaches 3 (max), becomes hard + assert {:ok, check} = Monitoring.update_check_state(check, 0, "OK") + assert check.current_state == 0 + assert check.current_state_type == "hard" + assert check.current_check_attempt == 3 + + # Third call: hard state stays hard + assert {:ok, check} = Monitoring.update_check_state(check, 0, "OK") + assert check.current_state == 0 + assert check.current_state_type == "hard" + end + + test "repeated calls work correctly", %{organization: org} do {:ok, check} = Monitoring.create_check(valid_check_attrs(org.id)) assert {:ok, check} = Monitoring.update_check_state(check, 0, "OK") assert {:ok, check} = Monitoring.update_check_state(check, 2, "CRITICAL") - assert {:ok, _check} = Monitoring.update_check_state(check, 0, "OK again") + assert check.current_state == 2 + assert {:ok, check} = Monitoring.update_check_state(check, 0, "OK again") + assert check.current_state == 0 end end