fix(engine): keep ModuleStore alive across builds (#889)

We have been attempting to improve our project compilation performance,
and in particular how that interacts with Expert. One weird issue we ran
into when testing rapid "save" calls on a file is that this could result
in a crash in Expert.

# Cause / Resolution

Engine.ModuleStore exited :normal after every project_compiled event. As
a permanent child of Engine.Supervisor that is one restart per build, so
the fourth build to finish within five seconds exceeded the default
restart intensity and shut the engine application down; the server then
stopped reading stdin once its main process was restarted. Four quick
saves that produce no-op builds are enough to trigger it since the edit
window dropped to 100 ms (#872). Stay alive and keep rebuilding the
store after each compile.

Full disclosure: The test was written with the assistance of Fable, I'm
pretty sure the stubs make sense, but if it doesn't seem right feel free
to just tell me to go away and do it properly :)

Author: benwilson512

apps/engine/lib/engine/module_store.ex

@@ -24,6 +24,10 @@ defmodule Engine.ModuleStore do
       {:done, token}
     end)
 
-    {:stop, :normal, state}
+    # This is a permanent child of Engine.Supervisor. Exiting here, even with
+    # :normal, means one restart per build, and the fourth build to finish
+    # within five seconds exceeds the supervisor's restart intensity and stops
+    # the whole engine application.
+    {:noreply, state}
   end
 end

apps/engine/test/engine/module_store_test.exs

@@ -0,0 +1,52 @@
+defmodule Engine.ModuleStoreTest do
+  use ExUnit.Case
+  use Patch
+
+  import Forge.EngineApi.Messages
+
+  alias ElixirSense.Providers.Plugins.ModuleStore, as: ElixirSenseModuleStore
+  alias Engine.Dispatch
+  alias Engine.ModuleStore
+
+  setup do
+    test_pid = self()
+    start_supervised!(Dispatch)
+
+    # Progress reports go to the manager node over erpc; keep them local.
+    patch(Dispatch, :erpc_call, fn
+      Expert.Progress, :begin, [_title, _opts] -> {:ok, System.unique_integer([:positive])}
+      Expert.Progress, :report, _args -> :ok
+    end)
+
+    patch(Dispatch, :erpc_cast, fn Expert.Progress, _function, _args -> true end)
+
+    patch(ElixirSenseModuleStore, :build, fn ->
+      send(test_pid, :module_store_built)
+      :ok
+    end)
+
+    pid = start_supervised!(ModuleStore)
+    {:ok, pid: pid}
+  end
+
+  test "rebuilds the module store when the project compiles, and stays up", %{pid: pid} do
+    ref = Process.monitor(pid)
+    Dispatch.broadcast(project_compiled(status: :success))
+
+    assert_receive :module_store_built
+    refute_receive {:DOWN, ^ref, :process, ^pid, _}
+  end
+
+  test "survives many builds in quick succession", %{pid: pid} do
+    # It used to exit :normal after each build. As a permanent child that meant
+    # one restart per build, so the fourth build within five seconds exceeded
+    # the supervisor's restart intensity and took the engine application down.
+    for _ <- 1..10 do
+      Dispatch.broadcast(project_compiled(status: :success))
+      assert_receive :module_store_built
+    end
+
+    assert Process.alive?(pid)
+    assert Dispatch.registered?(pid)
+  end
+end