fix(compare): skip a compare_list entry with no id instead of crashing (#5472)

Compare.run_all() indexed tariff["id"] directly, so a compare_list entry
missing its id line raised KeyError every 5-minute cycle - and because the
crash skipped predbat.py's compare_active reset, the comparison (and the
crash) kept re-triggering until the config was fixed.

The id lookup and its warning are now in their own get_tariff_id() method,
so the guard can be tested directly rather than only by inspecting run_all()'s
source text (code review for #5472 found the source-inspection test alone
would pass a guard that checked the wrong field, e.g. tariff.get("name")).

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

Author: chalfontchubby

apps/predbat/compare.py

@@ -592,6 +592,14 @@ def recompute_car_charging(self, car_charging_slots):
             if my_predbat.car_charging_planned[car_n] and my_predbat.car_charging_exclusive[car_n]:
                 break
 
+    def get_tariff_id(self, tariff):
+        """Return a compare_list entry's id, or None and a warning if it has none (#5472)."""
+        tariff_id = tariff.get("id")
+        if not tariff_id:
+            self.log("Warn: Compare tariff {} has no id, skipping - add an 'id:' line to this compare_list entry".format(tariff.get("name", "")))
+            return None
+        return tariff_id
+
     def run_all(self, debug=False, fetch_sensor=True):
         """
         Compare a comparison in prices across multiple energy tariffs and report results
@@ -672,7 +680,10 @@ def run_all(self, debug=False, fetch_sensor=True):
 
         try:
             for tariff in compare_list:
-                prior = results.get(tariff["id"], {})
+                tariff_id = self.get_tariff_id(tariff)
+                if not tariff_id:
+                    continue
+                prior = results.get(tariff_id, {})
                 if prior:
                     prior_date = prior.get("date", "")[:10]
                     if prior_date == today_date:
@@ -684,11 +695,11 @@ def run_all(self, debug=False, fetch_sensor=True):
                 else:
                     # First ever run for this tariff: start from actual midnight SoC
                     start_soc = soc_midnight_fallback
-                self.log("Compare tariff {} starting SoC: {:.2f} kWh".format(tariff.get("id", ""), start_soc))
+                self.log("Compare tariff {} starting SoC: {:.2f} kWh".format(tariff_id, start_soc))
                 try:
                     result_data = self.run_single(tariff, rate_import_base, rate_export_base, end_record, debug=debug, fetch_sensor=fetch_sensor, car_charging_slots=save_car_charging_slots, start_soc=start_soc, io_adjusted_base=io_adjusted_base)
                     if result_data is not None:
-                        results[tariff["id"]] = result_data
+                        results[tariff_id] = result_data
                 finally:
                     # Restore hardware settings after each tariff so overrides don't bleed into the next tariff;
                     # in a finally so a raised run_single() can't leave compare values applied to the live instance

apps/predbat/predbat.py

@@ -1430,10 +1430,13 @@ def update_pred(self, scheduled=True):
 
         if self.comparison:
             if (scheduled and self.minutes_now < RUN_EVERY) or self.get_arg("compare_active", False):
-                # Compare tariffs either when triggered or daily at midnight
+                # Compare tariffs either when triggered or daily at midnight; the reset must run even if
+                # run_all() raises, otherwise a crashed comparison re-triggers itself every cycle (#5472)
                 self.expose_config("compare_active", True)
-                self.comparison.run_all()
-                self.expose_config("compare_active", False)
+                try:
+                    self.comparison.run_all()
+                finally:
+                    self.expose_config("compare_active", False)
             else:
                 # Otherwise just update HA sensors to prevent then expiring
                 self.comparison.publish_only()

apps/predbat/tests/test_compare.py

@@ -717,6 +717,55 @@ def _mock_plan_iboost_smart():
     else:
         print("PASS T26: only a tariff reusing the live import rates keeps the live dispatch markers")
 
+    # ------------------------------------------------------------------
+    # T27: get_tariff_id() returns None (with a warning) for a compare_list
+    #      entry with no id, and the real id string otherwise (#5472 - run_all()
+    #      used to index tariff["id"] directly, raising KeyError every cycle; a
+    #      crash there also left compare_active stuck on, re-triggering the
+    #      comparison - and the crash - every 5-minute cycle)
+    # ------------------------------------------------------------------
+    cmp, pb = _make_compare()
+    logged = []
+    cmp.log = lambda msg: logged.append(msg)
+
+    no_id_result = cmp.get_tariff_id({"name": "No Id Tariff"})
+    has_id_result = cmp.get_tariff_id({"id": "has_id", "name": "Has Id Tariff"})
+
+    if no_id_result is not None:
+        print("ERROR T27: get_tariff_id() should return None for an entry with no id, got {}".format(no_id_result))
+        failed += 1
+    elif not any("No Id Tariff" in msg for msg in logged):
+        print("ERROR T27: get_tariff_id() should log a warning naming the id-less entry, got {}".format(logged))
+        failed += 1
+    elif has_id_result != "has_id":
+        print("ERROR T27: get_tariff_id() should return the real id for an entry that has one, got {}".format(has_id_result))
+        failed += 1
+    else:
+        print("PASS T27: get_tariff_id() returns None and warns for an id-less entry, the real id otherwise")
+
+    # ------------------------------------------------------------------
+    # T28: run_all() is actually wired to call get_tariff_id() inside its tariff
+    #      loop, rather than T27 only proving the helper works in isolation -
+    #      otherwise run_all() could still index tariff["id"] directly (#5472)
+    # ------------------------------------------------------------------
+    run_all_source = inspect.getsource(Compare.run_all)
+    loop_idx = run_all_source.find("for tariff in compare_list:")
+    guard_idx = run_all_source.find("self.get_tariff_id(tariff)", loop_idx + 1) if loop_idx >= 0 else -1
+    continue_idx = run_all_source.find("continue", guard_idx + 1) if guard_idx >= 0 else -1
+    first_direct_index_idx = run_all_source.find('tariff["id"]', loop_idx + 1) if loop_idx >= 0 else -1
+
+    if loop_idx < 0 or guard_idx < 0 or continue_idx < 0:
+        print("ERROR T28: could not locate a call to get_tariff_id() inside run_all()'s tariff loop - test may need updating")
+        failed += 1
+    elif not (loop_idx < guard_idx < continue_idx):
+        print("ERROR T28: run_all() does not skip an id-less tariff before using its id")
+        failed += 1
+    elif first_direct_index_idx != -1 and first_direct_index_idx < continue_idx:
+        print("ERROR T28: run_all() still indexes tariff[\"id\"] directly before the id guard takes effect")
+        failed += 1
+    else:
+        print("PASS T28: run_all() calls get_tariff_id() inside its tariff loop before using the id")
+
     if failed:
         print("**** compare tests FAILED: {} errors ****\n".format(failed))
     else: