From 5b619f492a0f598f2694bf2747b8511729f71137 Mon Sep 17 00:00:00 2001 From: Chili Palmer Date: Fri, 29 May 2026 09:49:54 +0200 Subject: [PATCH] fix: A1-11 graceful preview shutdown drains inflight requests before stopping --- SPECGAPS.md | 2 +- lib/bds/preview.ex | 108 ++++++++++++++++++++++++++++++++------ test/bds/preview_test.exs | 38 ++++++++++++++ 3 files changed, 130 insertions(+), 18 deletions(-) diff --git a/SPECGAPS.md b/SPECGAPS.md index 922821a..6a5191b 100644 --- a/SPECGAPS.md +++ b/SPECGAPS.md @@ -20,7 +20,7 @@ Gap categories: **SC** = spec correct, fix code | **CS** = code correct, update | A1-8 | ~~`ValidateLiquid`/`ValidateScript` before publish~~ | template.allium:110, script.allium:165 | `publish_template` validates Liquid via `Liquex.parse`, `publish_script` validates Lua via `BDS.Scripting.validate` | **Resolved:** validation gates added to `publish_template/1` and `publish_script/1`, invalid content returns `{:error, {:invalid_liquid|:invalid_script, reason}}`, 4 tests added | | A1-9 | ~~17 preset colors + custom hex in tag picker~~ | editor_tags.allium | `ColourPicker` hook + popover with 17 preset swatches grid and custom hex input, wired to both create and edit forms | **Resolved:** replaced native `` with `ColourPickerPopover` component (17 presets, custom hex #RRGGBB, immediate selection), JS hook for click-away dismiss, 1 test added | | A1-10 | ~~Template file written on create~~ | engine_side_effects.allium:151-153 | `create_template` now computes `file_path` and writes template file with YAML frontmatter on create | **Resolved:** `create_template/1` writes `templates/{slug}.liquid` on create, `next_template_file_path` always computes path, 1 test added | -| A1-11 | Graceful shutdown with inflight request tracking | preview.allium:47-48 | Kills acceptor process, no inflight tracking | Fix code: track inflight requests, drain before shutdown | +| A1-11 | ~~Graceful shutdown with inflight request tracking~~ | preview.allium:47-48 | `stop_preview` now closes the listener, parks the reply, and drains monitored inflight request tasks before reporting stopped | **Resolved:** acceptor transfers socket ownership to each request task; GenServer monitors inflight tasks, `begin_graceful_stop` stops accepting and finalizes via `:DOWN`/`:drain_timeout` (5s force-kill cap), 1 test added | | A1-12 | Real Pagefind integration for search | generation.allium:208 | Stub only: `pagefind-ui.js` is one-liner, `PagefindUI` never defined, search-runtime.js silently bails, client-side search non-functional | Fix code: bundle real Pagefind, build proper fragment index, wire PagefindUI | | A1-13 | Git sidebar shows only "Working tree" placeholder | sidebar_views.allium:651-770 | `sidebar.ex:782-798` returns single entity_list item; `BDS.Git` has full status/diff/commit/history/fetch/pull/push/prune_lfs but sidebar doesn't use it | Fix code: wire sidebar `git_view/0` to `BDS.Git` — render branch, ahead/behind, status file list, commit input, history entries, action buttons per spec | | A1-14 | Embedding uses TF-IDF hash projection instead of real neural model | embedding.allium:44-53, invariants ModelCaching/VectorCacheInDb | `backends/in_app.ex` hashes terms into sparse vectors via `:erlang.phash2`; no ONNX model, no `"query: "` prefix, no mean pooling, vectors stored as JSON text not Float32Array BLOB, snapshot-based neighbor lookup instead of USearch HNSW index | Fix code: (1) add Bumblebee + ONNX runtime deps to run `Xenova/multilingual-e5-small`, (2) implement lazy model download + cache in app data dir, (3) `"query: "` prefix + mean pooling + L2 norm in backend, (4) store vectors as binary BLOB (1536 bytes), (5) replace JSON snapshot with USearch HNSW index (cosine, M=16, ef=128/64, 5s debounce), (6) cross-language semantic similarity must work | diff --git a/lib/bds/preview.ex b/lib/bds/preview.ex index ab00398..3841f51 100644 --- a/lib/bds/preview.ex +++ b/lib/bds/preview.ex @@ -14,6 +14,10 @@ defmodule BDS.Preview do @host "127.0.0.1" @port 4123 + # Max time to wait for inflight requests to finish during graceful shutdown + # before remaining request tasks are forcibly terminated. + @drain_timeout 5_000 + def start_link(_opts) do GenServer.start_link(__MODULE__, %{}, name: __MODULE__) end @@ -56,7 +60,7 @@ defmodule BDS.Preview do @impl true def init(_state) do - {:ok, %{current: nil}} + {:ok, %{current: nil, stopping: nil}} end @impl true @@ -78,15 +82,12 @@ defmodule BDS.Preview do {:reply, reply, next_state} end - def handle_call({:stop_preview, project_id}, _from, state) do - next_state = - if match?(%{project_id: ^project_id}, state.current) do - stop_current_server(state) - else - state - end - - {:reply, :ok, next_state} + def handle_call({:stop_preview, project_id}, from, state) do + if match?(%{project_id: ^project_id}, state.current) do + begin_graceful_stop(state, from) + else + {:reply, :ok, state} + end end def handle_call({:request, project_id, request_path, query_params}, _from, state) do @@ -141,6 +142,25 @@ defmodule BDS.Preview do end @impl true + def handle_cast({:track_request, pid}, %{current: %{} = current} = state) when is_pid(pid) do + ref = Process.monitor(pid) + inflight = Map.put(current.inflight, ref, pid) + {:noreply, %{state | current: %{current | inflight: inflight}}} + end + + def handle_cast({:track_request, _pid}, state), do: {:noreply, state} + + @impl true + def handle_info({:DOWN, ref, :process, _pid, _reason}, %{current: %{} = current} = state) do + inflight = Map.delete(current.inflight, ref) + state = %{state | current: %{current | inflight: inflight}} + {:noreply, maybe_finalize_stop(state)} + end + + def handle_info(:drain_timeout, state) do + {:noreply, force_finalize_stop(state)} + end + def handle_info(_msg, state) do {:noreply, state} end @@ -287,9 +307,18 @@ defmodule BDS.Preview do defp accept_loop(listener, project_id) do case :gen_tcp.accept(listener) do {:ok, socket} -> - Task.Supervisor.start_child(BDS.TCP.TaskSupervisor, fn -> - serve_client(socket, project_id) - end) + case Task.Supervisor.start_child(BDS.TCP.TaskSupervisor, fn -> + serve_client(socket, project_id) + end) do + {:ok, pid} -> + # Hand the socket to the request task so an inflight request survives + # the acceptor being shut down (it would otherwise close the socket). + _ = :gen_tcp.controlling_process(socket, pid) + GenServer.cast(__MODULE__, {:track_request, pid}) + + _other -> + :ok + end accept_loop(listener, project_id) @@ -412,14 +441,58 @@ defmodule BDS.Preview do end end - defp stop_current_server(%{current: %{listener: listener, acceptor_pid: acceptor_pid}} = state) do - _ = :gen_tcp.close(listener) - if is_pid(acceptor_pid), do: Process.exit(acceptor_pid, :normal) + # Graceful shutdown: stop accepting new connections, then wait for inflight + # request tasks to finish before reporting the server stopped. The stop call + # is parked (no immediate reply) and finalized from the :DOWN handlers, so the + # GenServer stays available to serve the requests it is draining. + defp begin_graceful_stop(%{current: current} = state, from) do + _ = :gen_tcp.close(current.listener) + if is_pid(current.acceptor_pid), do: Process.exit(current.acceptor_pid, :normal) + + if map_size(current.inflight) == 0 do + {:reply, :ok, %{state | current: nil, stopping: nil}} + else + timer = Process.send_after(self(), :drain_timeout, @drain_timeout) + {:noreply, %{state | stopping: %{from: from, timer: timer}}} + end + end + + defp maybe_finalize_stop( + %{stopping: %{from: from, timer: timer}, current: %{inflight: inflight}} = state + ) + when map_size(inflight) == 0 do + if is_reference(timer), do: Process.cancel_timer(timer) + GenServer.reply(from, :ok) + %{state | current: nil, stopping: nil} + end + + defp maybe_finalize_stop(state), do: state + + defp force_finalize_stop(%{stopping: %{from: from}, current: %{inflight: inflight}} = state) do + kill_inflight(inflight) + GenServer.reply(from, :ok) + %{state | current: nil, stopping: nil} + end + + defp force_finalize_stop(state), do: state + + # Hard stop used when restarting the server in place (no graceful drain). + defp stop_current_server(%{current: %{} = current} = state) do + _ = :gen_tcp.close(current.listener) + if is_pid(current.acceptor_pid), do: Process.exit(current.acceptor_pid, :normal) + kill_inflight(current.inflight) %{state | current: nil} end defp stop_current_server(state), do: state + defp kill_inflight(inflight) do + Enum.each(inflight, fn {ref, pid} -> + Process.demonitor(ref, [:flush]) + if is_pid(pid), do: Process.exit(pid, :kill) + end) + end + defp start_server(state, project_id, data_dir, owner_pid) do state = stop_current_server(state) maybe_allow_repo(owner_pid) @@ -442,7 +515,8 @@ defmodule BDS.Preview do port: @port, is_running: true, listener: listener, - acceptor_pid: acceptor_pid + acceptor_pid: acceptor_pid, + inflight: %{} } {{:ok, public_server(server)}, %{state | current: server}} diff --git a/test/bds/preview_test.exs b/test/bds/preview_test.exs index 6d8b7b7..bb93e50 100644 --- a/test/bds/preview_test.exs +++ b/test/bds/preview_test.exs @@ -706,6 +706,44 @@ defmodule BDS.PreviewTest do assert :ok = BDS.Preview.stop_preview(project.id) end + test "stop_preview drains an inflight request before completing", %{project: project} do + assert {:ok, _metadata} = + Metadata.update_project_metadata(project.id, %{ + main_language: "en", + blog_languages: ["en"] + }) + + assert {:ok, server} = BDS.Preview.start_preview(project.id) + + # Open a raw connection and hold it open without sending the request line, + # so the server has a request task blocked in recv (an inflight request). + {:ok, socket} = + :gen_tcp.connect(to_charlist(server.host), server.port, [ + :binary, + packet: :raw, + active: false + ]) + + # Give the acceptor time to accept the connection and register the task. + Process.sleep(100) + + test_pid = self() + + spawn(fn -> + send(test_pid, {:stopped, BDS.Preview.stop_preview(project.id)}) + end) + + # Shutdown must not complete while the request is still inflight. + refute_receive {:stopped, _}, 300 + + # Completing the request lets the server drain and finish shutting down. + :ok = :gen_tcp.send(socket, "GET / HTTP/1.1\r\nhost: localhost\r\n\r\n") + + assert_receive {:stopped, :ok}, 2_000 + + :gen_tcp.close(socket) + end + test "preview query params can override the rendered theme for generated and draft pages", %{ project: project } do