From 63a94172874ee05a1d7419941009283e53c693eb Mon Sep 17 00:00:00 2001
From: Graham McIntire <graham@mcintire.me>
Date: Thu, 16 Apr 2026 14:58:37 -0500
Subject: [PATCH] fix(admin_task_worker): unknown feature now returns error
 instead of falling through

Backtest branch's unknown-feature guard was dead code: the if block's
{:error, _} return was discarded and execution fell through, running the
backtest with a just-created atom and writing a report for nonsense
features. Two compounding issues:

- function_exported?/3 returns false for modules not yet loaded in the
  BEAM, so even valid feature names triggered the warning path on a
  cold VM.
- String.to_atom/1 on caller-supplied strings created unbounded atoms.

Fix: Code.ensure_loaded?(Features) before the check, String.to_existing_atom/1
with rescue (Features exports __info__(:functions) so every valid function
atom already exists), and an explicit early return via a with pipeline so
the error tuple actually leaves the worker.
---
 .../workers/admin_task_worker.ex              | 60 ++++++++++++-------
 .../workers/admin_task_worker_test.exs        | 39 +++++-------
 2 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/lib/microwaveprop/workers/admin_task_worker.ex b/lib/microwaveprop/workers/admin_task_worker.ex
index 414b4c37..51a6cc97 100644
--- a/lib/microwaveprop/workers/admin_task_worker.ex
+++ b/lib/microwaveprop/workers/admin_task_worker.ex
@@ -47,29 +47,22 @@ defmodule Microwaveprop.Workers.AdminTaskWorker do
 
   def perform(%Oban.Job{args: %{"task" => "backtest", "feature" => feature_name} = args}) do
     sample_size = Map.get(args, "sample_size", 5000)
-    fun_atom = String.to_atom(feature_name)
 
-    if !function_exported?(Features, fun_atom, 3) do
-      Logger.error("AdminTask: unknown feature #{feature_name}")
-      {:error, "unknown feature: #{feature_name}"}
+    # `function_exported?/3` returns false for any module that hasn't
+    # been loaded yet in the BEAM — force-load `Features` first so valid
+    # feature names aren't rejected on a cold VM. Using
+    # `String.to_existing_atom/1` inside a rescue guards against
+    # unbounded atom creation (the admin UI is the only dispatcher, but
+    # cheap insurance).
+    with true <- Code.ensure_loaded?(Features),
+         {:ok, fun_atom} <- safe_to_existing_atom(feature_name),
+         true <- function_exported?(Features, fun_atom, 3) do
+      run_backtest(feature_name, fun_atom, sample_size)
+    else
+      _ ->
+        Logger.error("AdminTask: unknown feature #{feature_name}")
+        {:error, "unknown feature: #{feature_name}"}
     end
-
-    feature_fun = &apply(Features, fun_atom, [&1, &2, &3])
-    name = "Microwaveprop.Backtest.Features.#{feature_name}"
-
-    Logger.info("AdminTask: running backtest for #{feature_name}")
-
-    report = Backtest.evaluate(feature_fun, sample_size: sample_size, feature_name: name)
-    distance_bins = Backtest.lift_by_distance(feature_fun, sample_size: sample_size)
-    band_stats = Backtest.lift_by_band(feature_fun, sample_size: sample_size)
-    markdown = Backtest.to_markdown(report, distance_bins: distance_bins, band_stats: band_stats)
-
-    path = "priv/backtest_reports/#{feature_name}.md"
-    File.mkdir_p!(Path.dirname(path))
-    File.write!(path, markdown)
-
-    Logger.info("AdminTask: backtest for #{feature_name} complete, wrote #{path}")
-    :ok
   end
 
   def perform(%Oban.Job{args: %{"task" => "climatology"} = args}) do
@@ -224,4 +217,29 @@ defmodule Microwaveprop.Workers.AdminTaskWorker do
     Logger.error("AdminTask: unknown task #{task}")
     {:error, "unknown task: #{task}"}
   end
+
+  defp safe_to_existing_atom(name) do
+    {:ok, String.to_existing_atom(name)}
+  rescue
+    ArgumentError -> :error
+  end
+
+  defp run_backtest(feature_name, fun_atom, sample_size) do
+    feature_fun = &apply(Features, fun_atom, [&1, &2, &3])
+    name = "Microwaveprop.Backtest.Features.#{feature_name}"
+
+    Logger.info("AdminTask: running backtest for #{feature_name}")
+
+    report = Backtest.evaluate(feature_fun, sample_size: sample_size, feature_name: name)
+    distance_bins = Backtest.lift_by_distance(feature_fun, sample_size: sample_size)
+    band_stats = Backtest.lift_by_band(feature_fun, sample_size: sample_size)
+    markdown = Backtest.to_markdown(report, distance_bins: distance_bins, band_stats: band_stats)
+
+    path = "priv/backtest_reports/#{feature_name}.md"
+    File.mkdir_p!(Path.dirname(path))
+    File.write!(path, markdown)
+
+    Logger.info("AdminTask: backtest for #{feature_name} complete, wrote #{path}")
+    :ok
+  end
 end
diff --git a/test/microwaveprop/workers/admin_task_worker_test.exs b/test/microwaveprop/workers/admin_task_worker_test.exs
index 5682b02e..ed9fcb63 100644
--- a/test/microwaveprop/workers/admin_task_worker_test.exs
+++ b/test/microwaveprop/workers/admin_task_worker_test.exs
@@ -83,15 +83,11 @@ defmodule Microwaveprop.Workers.AdminTaskWorkerTest do
 
   describe "perform/1 — task=backtest" do
     test "writes a per-feature markdown report for a known feature" do
-      # Note: the worker logs "unknown feature time_of_day" here even
-      # though `time_of_day` is a legitimate public function on
-      # `Features`. The guard uses `function_exported?/3`, which returns
-      # false until the target module has been loaded — and
-      # `Microwaveprop.Backtest.Features` isn't referenced anywhere
-      # before this branch, so it's not yet loaded in a cold VM. A
-      # `Code.ensure_loaded?/1` call would fix the guard; see the
-      # "silently succeeds for an unknown feature" test below for the
-      # full latent-bug characterization.
+      # `time_of_day` is a legitimate public function on
+      # `Microwaveprop.Backtest.Features`. The guard uses
+      # `Code.ensure_loaded?/1` before `function_exported?/3`, so the
+      # module is forced-loaded and the guard does not spuriously reject
+      # valid feature names on a cold VM.
       with_tmp_cwd(fn tmp ->
         args = %{"task" => "backtest", "feature" => "time_of_day", "sample_size" => 10}
         assert :ok = AdminTaskWorker.perform(%Oban.Job{args: args})
@@ -104,23 +100,20 @@ defmodule Microwaveprop.Workers.AdminTaskWorkerTest do
       end)
     end
 
-    test "silently succeeds for an unknown feature when the QSO corpus is empty (latent bug)" do
-      # The worker's guard `if !function_exported?(Features, fun_atom, 3)`
-      # logs and evaluates `{:error, ...}` but the value is discarded and
-      # execution falls through. The closure `&apply(Features, fun_atom,
-      # [&1, &2, &3])` is only invoked if `Backtest.evaluate` has
-      # contacts to evaluate it against — with an empty QSO corpus (as
-      # in the sandbox), the undefined function is never called and the
-      # worker writes an empty report and returns `:ok`. This pins the
-      # current observable behavior; a proper fix should `{:error, _}`
-      # early rather than relying on downstream emptiness to mask the
-      # bug.
+    test "returns {:error, _} and writes no report for an unknown feature" do
+      # An unknown feature name must short-circuit: the worker returns
+      # `{:error, "unknown feature: ..."}` before invoking
+      # `Backtest.evaluate` and must NOT write a report file. Previously
+      # the guard's `{:error, _}` tuple was the `if` block's return
+      # value and got discarded, so execution fell through to write an
+      # empty report.
       with_tmp_cwd(fn tmp ->
         args = %{"task" => "backtest", "feature" => "does_not_exist_feature_xyz", "sample_size" => 10}
-        assert :ok = AdminTaskWorker.perform(%Oban.Job{args: args})
 
-        # File was still written — guard did not bail out.
-        assert File.exists?(Path.join(tmp, "priv/backtest_reports/does_not_exist_feature_xyz.md"))
+        assert {:error, "unknown feature: does_not_exist_feature_xyz"} =
+                 AdminTaskWorker.perform(%Oban.Job{args: args})
+
+        refute File.exists?(Path.join(tmp, "priv/backtest_reports/does_not_exist_feature_xyz.md"))
       end)
     end
   end