From 34be4a3b793d9f8c4cc231fd6e554149d7ceff4f Mon Sep 17 00:00:00 2001
From: "Jonathan D.A. Jewell" <6759885+hyperpolymath@users.noreply.github.com>
Date: Tue, 21 Jul 2026 07:40:03 +0100
Subject: [PATCH] fix(test): register the supervisor restore BEFORE mutating
 (#643)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`take_supervised/1` removed a child from the live application supervisor and
only THEN registered the `on_exit` that puts it back:

    Supervisor.terminate_child(Hypatia.Supervisor, module)
    Supervisor.delete_child(Hypatia.Supervisor, module)
    start_supervised!(module)          # <- if this raises...
    on_exit(fn -> ...restore... end)   # <- ...this never registers

If anything between the delete and the on_exit fails, the child is deleted
from Hypatia.Supervisor and NEVER restored — for the whole rest of the suite.
Every later test needing that module then fails too. That is why one root
failure cascaded into ~20, and why the failing set changed with the seed.

Fix: register the restore first (on_exit still runs when setup raises), and
make it idempotent — clear any stale spec, re-add, and inspect the return
rather than discarding it, so a restore failure is loud instead of silently
poisoning everything downstream.

PROOF of the mechanism (load-independent, by injecting a raise after the
delete and asking the supervisor what children it has):

    [[PROOF]] OLD  -> child present after raise: false     <- lost forever
    [[PROOF]] NEW  -> restore registered, runs on_exit

Verified:
  mix test test/reflexive_test.exs                    -> 16 tests, 0 failures
  mix test reflexive_test + safety_test               -> 42 tests, 0 failures
  reflexive_test followed by an intactness assertion
    (all four modules still children of Hypatia.Supervisor
     with live pids)                                  -> 17 tests, 0 failures

SCOPE — deliberately honest: this fixes the cascade, NOT the whole suite.
`mix test` remains red for other reasons. Whole-suite A/B across seeds was
inconclusive here because reflexive_test (and others) use `:timer.sleep/1`
for synchronisation, so counts swing with machine load — this box was busy.
The remaining order- and timing-dependent failures are separate work and are
tracked in #643, which this commit updates rather than closes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 test/reflexive_test.exs | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/test/reflexive_test.exs b/test/reflexive_test.exs
index 5d9c39df..7581c869 100644
--- a/test/reflexive_test.exs
+++ b/test/reflexive_test.exs
@@ -25,15 +25,45 @@ defmodule Hypatia.ReflexiveTest do
 
   # Take a GenServer away from the Application supervisor so start_supervised!/1
   # can start a clean, isolated instance for the test. Restores the child on exit.
+  #
+  # The restore is registered BEFORE the mutation, deliberately. Previously it
+  # was registered after `start_supervised!/1`, so if that raised — or anything
+  # between the delete and the on_exit failed — the child was deleted from
+  # Hypatia.Supervisor and NEVER restored. Every later test needing that module
+  # then failed too, which is why one root failure cascaded into ~20 and why the
+  # failing set changed with the seed. on_exit callbacks still run when setup
+  # raises, so registering first makes the restore unconditional.
   defp take_supervised(module) do
+    on_exit(fn -> restore_supervised(module) end)
+
     Supervisor.terminate_child(Hypatia.Supervisor, module)
     Supervisor.delete_child(Hypatia.Supervisor, module)
     start_supervised!(module)
+  end
 
-    on_exit(fn ->
-      if pid = Process.whereis(module), do: GenServer.stop(pid, :normal, 5_000)
-      Supervisor.start_child(Hypatia.Supervisor, module)
-    end)
+  # Idempotent: safe whether the child spec is absent, present-but-stopped, or
+  # already running, and safe to call twice. Return values are inspected rather
+  # than discarded — a silent restore failure is what made the original cascade
+  # invisible.
+  defp restore_supervised(module) do
+    if pid = Process.whereis(module), do: GenServer.stop(pid, :normal, 5_000)
+
+    # Clear any stale spec left by this test before re-adding it.
+    Supervisor.terminate_child(Hypatia.Supervisor, module)
+    Supervisor.delete_child(Hypatia.Supervisor, module)
+
+    case Supervisor.start_child(Hypatia.Supervisor, module) do
+      {:ok, _pid} -> :ok
+      {:ok, _pid, _info} -> :ok
+      {:error, {:already_started, _pid}} -> :ok
+      {:error, :already_present} -> :ok
+      other ->
+        raise """
+        reflexive_test could not restore #{inspect(module)} to Hypatia.Supervisor: \
+        #{inspect(other)}. Leaving it absent would fail every later test that \
+        depends on it, so failing loudly here instead.
+        """
+    end
   end
 
   # ---------------------------------------------------------------------------