Skip to content

Commit bbea43f

Browse files
committed
fix(dashboard): start_enactment + retry_enactment atomic with runner boot
`:start_enactment` persisted the row then started the runner. On runner boot failure the row stayed behind as a phantom enactment. Now compensates by deleting the just-inserted row when start fails so the operator never sees a running row without a live process. `:retry_enactment` had the inverse bug — it flipped state to `:running` before the runner restart, so a failed retry "recovered" an enactment that still had no process. Now restart first, only commit `:running` after success. Also bounded `TelemetryFeedStore.last_seq`: previously grew one entry per enactment id ever seen, leaking memory in long-lived /telemetry tabs. Prune the tracker entry when its enactment ages fully out of the 500-event window.
1 parent 8727411 commit bbea43f

6 files changed

Lines changed: 149 additions & 29 deletions

File tree

dashboard/lib/coloured_flow_dashboard_web/stores/enactment_detail_store.ex

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -108,11 +108,11 @@ defmodule ColouredFlowDashboardWeb.Stores.EnactmentDetailStore do
108108
GenServer (same hot path the runner uses internally after completions).
109109
Reply codes: `:ok | :not_running | :runner_error`.
110110
* `:retry_enactment` — combines two public main-repo surfaces to recover an
111-
`:exception`-state enactment: `Storage.retry_enactment/2` flips the DB
112-
row back to `:running` and writes a `:retried` log entry; then
113-
`Runner.Enactment.Supervisor.start_enactment/1` brings the GenServer
114-
back online (the supervisor returns `{:ok, pid}` for both fresh starts
115-
and `:already_started`). Reply codes:
111+
`:exception`-state enactment: `Runner.Enactment.Supervisor.start_enactment/1`
112+
first brings the GenServer back online (the supervisor returns `{:ok, pid}`
113+
for both fresh starts and `:already_started`); only after a successful
114+
boot does `Storage.retry_enactment/2` flip the DB row back to `:running`
115+
and write a `:retried` log entry. Reply codes:
116116
`:ok | :not_exception | :already_terminated | :runner_error`. The
117117
command rejects when the dashboard's current `summary.state` is not
118118
`:exception` so an operator can't toggle a healthy enactment off the
@@ -302,6 +302,7 @@ defmodule ColouredFlowDashboardWeb.Stores.EnactmentDetailStore do
302302
pubsub = Map.get(params, "pubsub_name", @default_pubsub)
303303
topic_prefix = Map.get(params, "topic_prefix", @default_topic_prefix)
304304
flow_cache = Map.get(params, "flow_cache", @default_flow_cache)
305+
runner_start_fun = Map.get(params, "runner_start_fun", &Runner.start_enactment/1)
305306
topic = Map.get(params, "topic", "#{topic_prefix}enactment:#{enactment_id}")
306307

307308
:ok = Phoenix.PubSub.subscribe(pubsub, topic)
@@ -351,6 +352,7 @@ defmodule ColouredFlowDashboardWeb.Stores.EnactmentDetailStore do
351352
|> assign(:pubsub_name, pubsub)
352353
|> assign(:topic, topic)
353354
|> assign(:flow_cache, flow_cache)
355+
|> assign(:runner_start_fun, runner_start_fun)
354356
|> assign(:summary, summary)
355357
|> assign(:transitions, transitions)
356358
|> assign(:diagram, diagram)
@@ -454,7 +456,8 @@ defmodule ColouredFlowDashboardWeb.Stores.EnactmentDetailStore do
454456
end
455457

456458
def handle_command(:retry_enactment, _payload, socket) do
457-
{:reply, retry_enactment_reply(socket.assigns.enactment_id), socket}
459+
{:reply, retry_enactment_reply(socket.assigns.enactment_id, socket.assigns.runner_start_fun),
460+
socket}
458461
end
459462

460463
def handle_command(:inspect_transition, payload, socket) when is_map(payload) do
@@ -1741,21 +1744,23 @@ defmodule ColouredFlowDashboardWeb.Stores.EnactmentDetailStore do
17411744
# lag behind a concurrent force-terminate; trusting it would let an
17421745
# already-`:terminated` row be resurrected back to `:running` by
17431746
# `Storage.retry_enactment/2`. Only an actual `:exception` row is retried.
1744-
defp retry_enactment_reply(enactment_id) when is_binary(enactment_id) do
1747+
defp retry_enactment_reply(enactment_id, runner_start_fun) when is_binary(enactment_id) do
17451748
case authoritative_enactment_state(enactment_id) do
1746-
{:ok, :exception} -> do_retry_enactment(enactment_id)
1749+
{:ok, :exception} -> do_retry_enactment(enactment_id, runner_start_fun)
17471750
{:ok, :running} -> %{code: :not_exception}
17481751
{:ok, :terminated} -> %{code: :already_terminated}
17491752
{:error, reason} -> %{code: :runner_error, message: inspect(reason)}
17501753
end
17511754
end
17521755

1753-
defp do_retry_enactment(enactment_id) do
1754-
:ok = Storage.retry_enactment(enactment_id, message: "operator-triggered")
1756+
defp do_retry_enactment(enactment_id, runner_start_fun) do
1757+
case runner_start_fun.(enactment_id) do
1758+
{:ok, _pid} ->
1759+
:ok = Storage.retry_enactment(enactment_id, message: "operator-triggered")
1760+
%{code: :ok}
17551761

1756-
case Runner.start_enactment(enactment_id) do
1757-
{:ok, _pid} -> %{code: :ok}
1758-
{:error, reason} -> %{code: :runner_error, message: inspect(reason)}
1762+
{:error, reason} ->
1763+
%{code: :runner_error, message: inspect(reason)}
17591764
end
17601765
rescue
17611766
error -> %{code: :runner_error, message: Exception.message(error)}

dashboard/lib/coloured_flow_dashboard_web/stores/flow_catalog_store.ex

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStore do
157157
def mount(params, socket) when is_map(params) do
158158
pubsub = Map.get(params, "pubsub_name", @default_pubsub)
159159
topic = Map.get(params, "topic", @default_topic)
160+
runner_start_fun = Map.get(params, "runner_start_fun", &Runner.start_enactment/1)
160161

161162
:ok = Phoenix.PubSub.subscribe(pubsub, topic)
162163

@@ -166,6 +167,7 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStore do
166167
socket
167168
|> assign(:pubsub_name, pubsub)
168169
|> assign(:topic, topic)
170+
|> assign(:runner_start_fun, runner_start_fun)
169171
|> assign(:counts, counts)
170172
|> assign(:flow_ids, ids)
171173
|> assign(:last_seq, %{})
@@ -182,7 +184,7 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStore do
182184
@impl Musubi.Store
183185
def handle_command(:start_enactment, payload, socket) when is_map(payload) do
184186
flow_id = Map.get(payload, "flow_id") || Map.get(payload, :flow_id)
185-
{:reply, start_enactment_reply(flow_id), socket}
187+
{:reply, start_enactment_reply(flow_id, socket.assigns.runner_start_fun), socket}
186188
end
187189

188190
def handle_command(:refresh_catalog, _payload, socket) do
@@ -555,12 +557,13 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStore do
555557
# :start_enactment command
556558
# ---------------------------------------------------------------------------
557559

558-
defp start_enactment_reply(nil), do: %{code: :unknown_flow, enactment_id: nil}
560+
defp start_enactment_reply(nil, _runner_start_fun),
561+
do: %{code: :unknown_flow, enactment_id: nil}
559562

560-
defp start_enactment_reply(flow_id) when is_binary(flow_id) do
563+
defp start_enactment_reply(flow_id, runner_start_fun) when is_binary(flow_id) do
561564
with {:ok, %{name: name} = flow} <- fetch_flow(flow_id),
562565
{:ok, seed} <- fetch_seed(name),
563-
{:ok, enactment_id} <- insert_and_start(flow, seed) do
566+
{:ok, enactment_id} <- insert_and_start(flow, seed, runner_start_fun) do
564567
%{code: :ok, enactment_id: enactment_id}
565568
else
566569
{:error, :unknown_flow} ->
@@ -577,7 +580,8 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStore do
577580
end
578581
end
579582

580-
defp start_enactment_reply(_other), do: %{code: :unknown_flow, enactment_id: nil}
583+
defp start_enactment_reply(_other, _runner_start_fun),
584+
do: %{code: :unknown_flow, enactment_id: nil}
581585

582586
defp fetch_flow(flow_id) do
583587
case Storage.__storage__() do
@@ -618,10 +622,16 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStore do
618622
end
619623
end
620624

621-
defp insert_and_start(flow, seed) do
622-
with {:ok, enactment_id} <- insert_enactment(flow, seed.initial_markings),
623-
{:ok, _pid} <- start_runner(enactment_id) do
624-
{:ok, enactment_id}
625+
defp insert_and_start(flow, seed, runner_start_fun) do
626+
with {:ok, enactment_id} <- insert_enactment(flow, seed.initial_markings) do
627+
case start_runner(enactment_id, runner_start_fun) do
628+
{:ok, _pid} ->
629+
{:ok, enactment_id}
630+
631+
{:error, _reason} = error ->
632+
_rollback = rollback_enactment_insert(flow, enactment_id)
633+
error
634+
end
625635
end
626636
end
627637

@@ -643,8 +653,8 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStore do
643653
kind, reason -> {:error, {:storage, Exception.format(kind, reason)}}
644654
end
645655

646-
defp start_runner(enactment_id) do
647-
case Runner.start_enactment(enactment_id) do
656+
defp start_runner(enactment_id, runner_start_fun) do
657+
case runner_start_fun.(enactment_id) do
648658
{:ok, pid} when is_pid(pid) -> {:ok, pid}
649659
{:ok, pid, _info} when is_pid(pid) -> {:ok, pid}
650660
:ignore -> {:error, {:runner, ":ignore"}}
@@ -656,6 +666,28 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStore do
656666
kind, reason -> {:error, {:runner, Exception.format(kind, reason)}}
657667
end
658668

669+
defp rollback_enactment_insert(%{in_memory_record: _record}, enactment_id) do
670+
case GenServer.whereis(InMemory) do
671+
pid when is_pid(pid) ->
672+
_result = GenServer.call(InMemory, {:delete, {:enactment, enactment_id}})
673+
:ok
674+
675+
nil ->
676+
:ok
677+
end
678+
end
679+
680+
defp rollback_enactment_insert(%{id: _flow_id}, enactment_id) do
681+
case Repo.get(Schemas.Enactment, enactment_id) do
682+
%Schemas.Enactment{} = row ->
683+
_result = Repo.delete(row)
684+
:ok
685+
686+
nil ->
687+
:ok
688+
end
689+
end
690+
659691
# ---------------------------------------------------------------------------
660692
# :fetch_flow_detail command
661693
# ---------------------------------------------------------------------------

dashboard/lib/coloured_flow_dashboard_web/stores/telemetry_feed_store.ex

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ defmodule ColouredFlowDashboardWeb.Stores.TelemetryFeedStore do
8787
|> assign(:oldest_seq, nil)
8888
|> assign(:newest_seq, nil)
8989
|> assign(:last_seq, %{})
90+
|> assign(:entry_enactments, %{})
9091
# Ordered `[{id, seq}]` list — newest (highest seq) at head, matching
9192
# the stream's `at: 0` insertion semantics. Position-based insert keeps
9293
# the feed sorted by bridge seq even when the bridge's per-event Task
@@ -155,10 +156,12 @@ defmodule ColouredFlowDashboardWeb.Stores.TelemetryFeedStore do
155156
index = socket.assigns.entries_index
156157
position = insert_position(index, seq)
157158
next_index = List.insert_at(index, position, {id, seq})
159+
next_entry_enactments = Map.put(socket.assigns.entry_enactments, id, event.enactment_id)
158160

159161
socket
160162
|> stream_insert(:entries, entry, at: position)
161163
|> assign(:entries_index, next_index)
164+
|> assign(:entry_enactments, next_entry_enactments)
162165
|> assign(:total_events, socket.assigns.total_events + 1)
163166
|> assign(:entries_in_window, length(next_index))
164167
|> assign(:newest_seq, head_seq(next_index))
@@ -183,17 +186,34 @@ defmodule ColouredFlowDashboardWeb.Stores.TelemetryFeedStore do
183186
socket
184187
else
185188
{{drop_id, _drop_seq}, next_index} = List.pop_at(index, -1)
189+
drop_eid = Map.get(socket.assigns.entry_enactments, drop_id)
190+
next_entry_enactments = Map.delete(socket.assigns.entry_enactments, drop_id)
191+
next_last_seq = prune_last_seq(socket.assigns.last_seq, next_entry_enactments, drop_eid)
186192

187193
socket
188194
|> stream_delete_by_item_key(:entries, drop_id)
189195
|> assign(:entries_index, next_index)
196+
|> assign(:entry_enactments, next_entry_enactments)
197+
|> assign(:last_seq, next_last_seq)
190198
|> assign(:entries_in_window, length(next_index))
191199
|> assign(:newest_seq, head_seq(next_index))
192200
|> assign(:oldest_seq, tail_seq(next_index))
193201
|> trim_window()
194202
end
195203
end
196204

205+
defp prune_last_seq(last_seq, _entry_enactments, nil), do: last_seq
206+
207+
defp prune_last_seq(last_seq, entry_enactments, enactment_id) do
208+
if Enum.any?(entry_enactments, fn {_id, current_enactment_id} ->
209+
current_enactment_id == enactment_id
210+
end) do
211+
last_seq
212+
else
213+
Map.delete(last_seq, enactment_id)
214+
end
215+
end
216+
197217
defp head_seq([{_id, seq} | _rest]), do: seq
198218
defp head_seq([]), do: nil
199219

dashboard/test/coloured_flow_dashboard_web/stores/enactment_detail_store_test.exs

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ defmodule ColouredFlowDashboardWeb.Stores.EnactmentDetailStoreTest do
88
alias ColouredFlow.Enactment.BindingElement
99
alias ColouredFlow.Runner.Enactment, as: RunnerEnactment
1010
alias ColouredFlow.Runner.Enactment.Workitem, as: RunnerWorkitem
11+
alias ColouredFlow.Runner.Storage.Schemas
1112
alias ColouredFlowDashboard.Seed
1213
alias ColouredFlowDashboard.Seeds.ApprovalFlow
1314
alias ColouredFlowDashboard.TelemetryBridge
@@ -268,6 +269,24 @@ defmodule ColouredFlowDashboardWeb.Stores.EnactmentDetailStoreTest do
268269
Musubi.Testing.dispatch_command(page, :retry_enactment, %{})
269270
end
270271

272+
test "keeps the storage row in :exception when runner restart fails",
273+
%{topic_prefix: topic_prefix, flow_cache: flow_cache} do
274+
enactment = insert_enactment_with_state!(:exception)
275+
276+
page =
277+
mount_store(enactment.id, topic_prefix, flow_cache,
278+
runner_start_fun: fn enactment_id ->
279+
assert enactment_id == enactment.id
280+
{:error, :runner_down}
281+
end
282+
)
283+
284+
assert {:ok, %{code: :runner_error}} =
285+
Musubi.Testing.dispatch_command(page, :retry_enactment, %{})
286+
287+
assert %Schemas.Enactment{state: :exception} = Repo.get!(Schemas.Enactment, enactment.id)
288+
end
289+
271290
# Race pin (P20 HIGH): another operator force-terminated the row, but
272291
# this page's cached `summary.state` is still `:exception` because the
273292
# `:enactment_terminate` event has not been broadcast/applied yet. The
@@ -1422,12 +1441,20 @@ defmodule ColouredFlowDashboardWeb.Stores.EnactmentDetailStoreTest do
14221441
# credo:disable-for-next-line Credo.Check.Warning.UnsafeToAtom
14231442
defp unique_cache_atom(name) when is_binary(name), do: String.to_atom(name)
14241443

1425-
defp mount_store(enactment_id, topic_prefix, flow_cache) do
1426-
Musubi.Testing.mount(EnactmentDetailStore, %{
1444+
defp mount_store(enactment_id, topic_prefix, flow_cache, opts \\ []) do
1445+
params = %{
14271446
"id" => enactment_id,
14281447
"topic_prefix" => topic_prefix,
14291448
"flow_cache" => flow_cache
1430-
})
1449+
}
1450+
1451+
params =
1452+
case Keyword.fetch(opts, :runner_start_fun) do
1453+
{:ok, runner_start_fun} -> Map.put(params, "runner_start_fun", runner_start_fun)
1454+
:error -> params
1455+
end
1456+
1457+
Musubi.Testing.mount(EnactmentDetailStore, params)
14311458
end
14321459

14331460
# Inserts a real `enactments` row in the requested state so the

dashboard/test/coloured_flow_dashboard_web/stores/flow_catalog_store_test.exs

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,18 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStoreTest do
178178
assert {:ok, %{code: :unknown_flow, enactment_id: nil}} =
179179
Musubi.Testing.dispatch_command(page, :start_enactment, %{flow_id: bogus})
180180
end
181+
182+
test "rolls back the inserted row when Runner.start_enactment/1 fails", %{topic: topic} do
183+
flow_record = InMemory.insert_flow!(ApprovalFlow.cpnet())
184+
flow_id = InMemory.flow(flow_record, :id)
185+
before_ids = current_enactment_ids()
186+
page = mount_store(topic, fn _enactment_id -> {:error, :runner_down} end)
187+
188+
assert {:ok, %{code: :runner_error, enactment_id: nil}} =
189+
Musubi.Testing.dispatch_command(page, :start_enactment, %{flow_id: flow_id})
190+
191+
assert current_enactment_ids() == before_ids
192+
end
181193
end
182194

183195
describe ":refresh_catalog command" do
@@ -290,8 +302,11 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStoreTest do
290302
defp discriminator(context),
291303
do: Integer.to_string(:erlang.phash2({context.module, context.test}))
292304

293-
defp mount_store(topic) do
294-
Musubi.Testing.mount(FlowCatalogStore, %{"topic" => topic})
305+
defp mount_store(topic, runner_start_fun \\ &ColouredFlow.Runner.start_enactment/1) do
306+
Musubi.Testing.mount(FlowCatalogStore, %{
307+
"topic" => topic,
308+
"runner_start_fun" => runner_start_fun
309+
})
295310
end
296311

297312
defp broadcast!(topic, %Event{} = event) do
@@ -310,6 +325,16 @@ defmodule ColouredFlowDashboardWeb.Stores.FlowCatalogStoreTest do
310325
_error -> 0
311326
end
312327

328+
defp current_enactment_ids do
329+
InMemory
330+
|> Module.safe_concat("Enactment")
331+
|> :ets.tab2list()
332+
|> Enum.map(&InMemory.enactment(&1, :id))
333+
|> MapSet.new()
334+
rescue
335+
_error -> MapSet.new()
336+
end
337+
313338
# Reads the FlowSummary item out of the mount-time patch envelope's
314339
# stream_ops. The catalog stream emits an `insert` op per flow with the
315340
# native FlowSummary struct as the `item` field. Mailbox is drained in

dashboard/test/coloured_flow_dashboard_web/stores/telemetry_feed_store_test.exs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,17 @@ defmodule ColouredFlowDashboardWeb.Stores.TelemetryFeedStoreTest do
132132
assert assigns.newest_seq == 5
133133
end
134134

135+
test "bounds last_seq under distinct-enactment churn", %{topic: topic, page: page} do
136+
for seq <- 1..1_200 do
137+
broadcast!(topic, build_event(:produce_workitems_stop, Ecto.UUID.generate(), seq))
138+
end
139+
140+
assigns = Musubi.Testing.assigns(page)
141+
142+
assert assigns.entries_in_window == 5
143+
assert map_size(assigns.last_seq) <= 5
144+
end
145+
135146
test "two enactments interleave without stale-drop crosstalk",
136147
%{topic: topic, page: page} do
137148
eid_a = Ecto.UUID.generate()

0 commit comments

Comments
 (0)