Address PR #1749 review B1/B8/m1 — best-effort put_batch with short-circuit

- Switch put_batch/3 loop to reduce_while (B1 Path A)
- Prepend + reverse effects accumulator (m1, removes O(n²))
- Walk back "atomic" claim in moduledoc, docstring, controller, guide
- Replace toothless atomicity test with short-circuit proof (B8)

Author: Flo0807

guides/live_resource/user-preferences.md

@@ -49,7 +49,7 @@ the session; every setting is routed independently.
 │   Router → adapter(s) → side effects                                     │
 │            │                                                             │
 │            ▼                                                             │
-│   All-or-nothing apply: {ok: true} or {ok: false, errors: […]}           │
+│   Best-effort apply: {ok: true} or {ok: false, error: {key, reason}}     │
 │                                                                          │
 └──────────────────────────────────────────────────────────────────────────┘
 ```
@@ -204,10 +204,19 @@ Implement `Backpex.Preferences.Adapter`. Three callbacks:
   map}]` when you need the caller to update the session).
 
 The side-effect protocol is what keeps adapters pure. They don't touch
-`Plug.Conn` — they describe what the caller should do. This is what lets the
-controller implement all-or-nothing batch writes and lets server-side code
+`Plug.Conn` — they describe what the caller should do. This is what lets
+the controller compose cross-adapter batch writes and lets server-side code
 dispatch the same adapters without an HTTP request.
 
+Batch writes are **best-effort, first-error-wins**: on the first adapter
+error the dispatcher halts, returns `{:error, {key, reason}}`, and the
+controller responds `422 {ok: false, error: %{key: _, reason: _}}` without
+applying any session-backed side effects collected earlier in the batch.
+Adapters that persist eagerly (e.g. a DB-backed adapter that wrote via
+`Repo.insert!`) may have already committed earlier writes — the adapter
+behaviour has no rollback primitive, so callers should treat partial
+success as possible.
+
 ### In-memory test adapter
 
 Useful when exercising preferences in integration tests without spinning up

lib/backpex/controllers/preferences_controller.ex

@@ -21,10 +21,15 @@ defmodule Backpex.PreferencesController do
         {"key": "global.sidebar_open", "value": true}
       ]}
 
-  The batch form is **all-or-nothing**: if any adapter refuses a write, no
-  side effects are applied, the response is `422 {ok: false, errors: [...]}`,
-  and the session cookie is left unchanged. A partial-success state for
-  preferences is more confusing than a clean failure.
+  The batch form is **best-effort, first-error-wins**: if any adapter refuses
+  a write, the dispatcher halts at that entry, no further adapters are
+  called, and the response is `422 {ok: false, error: %{key: _, reason: _}}`.
+  Session-backed effects from earlier successful entries in the same batch
+  are also dropped (the controller never applies them on the error path), so
+  the session cookie is left unchanged. However, adapters that persist
+  eagerly (e.g. a DB-backed adapter that wrote via `Repo.insert!`) may have
+  already committed earlier writes — the adapter behaviour has no rollback
+  primitive, so callers should treat partial success as possible.
   """
 
   use Phoenix.Controller, formats: [:json]
@@ -51,10 +56,10 @@ defmodule Backpex.PreferencesController do
         |> Preferences.apply_effects_on_conn(effects)
         |> json(%{ok: true})
 
-      {:error, errors} ->
+      {:error, {key, reason}} ->
         conn
         |> put_status(422)
-        |> json(%{ok: false, errors: Enum.map(errors, &format_error/1)})
+        |> json(%{ok: false, error: format_error({key, reason})})
     end
   end
 

lib/backpex/preferences.ex

@@ -39,6 +39,8 @@ defmodule Backpex.Preferences do
     * `get/3` — read a single preference.
     * `get_map/3` — read every value under a prefix as a nested map.
     * `put_async/4` — write from a LiveView socket or `%Plug.Conn{}`.
+    * `put_batch/3` — dispatch a list of writes (best-effort, first-error-wins;
+      see the function docs for the partial-success caveat).
   """
 
   alias Backpex.Preferences.Adapters
@@ -174,16 +176,25 @@ defmodule Backpex.Preferences do
 
   @doc """
   Dispatches a batch of writes through their adapters and returns the
-  collected side effects, or an error list (all-or-nothing).
+  collected side effects, or the first error encountered.
 
-  Used by `Backpex.PreferencesController` to implement cross-adapter batch
-  writes with a clean failure mode.
+  Used by `Backpex.PreferencesController` to dispatch cross-adapter batch
+  writes.
 
   Threads the accumulated session state through each adapter call so that
   writes under the same session key compose correctly. The caller applies
   the returned effects in order; for `:put_session` effects targeting the
   same key, the last effect holds the fully-merged value.
 
+  ## Semantics
+
+  This is **best-effort, first-error-wins**. On the first adapter that
+  returns `{:error, reason}` the loop halts and returns
+  `{:error, {key, reason}}` — subsequent entries are not dispatched. Earlier
+  successful writes may already have been committed by their adapters (e.g.
+  a DB-backed adapter that writes eagerly). The adapter behaviour has no
+  rollback primitive, so callers should treat partial success as possible.
+
   ## Examples
 
       ctx = Backpex.Preferences.Context.from_conn(conn)
@@ -195,26 +206,29 @@ defmodule Backpex.Preferences do
       #=> {:ok, [{:put_session, "backpex_preferences", %{...}}]}
   """
   @spec put_batch(Context.t(), [{String.t(), term()}], keyword()) ::
-          {:ok, [Backpex.Preferences.Adapter.side_effect()]} | {:error, [term()]}
+          {:ok, [Backpex.Preferences.Adapter.side_effect()]} | {:error, {String.t(), term()}}
   def put_batch(%Context{} = ctx, entries, opts \\ []) when is_list(entries) do
     ctx = resolve_identity(ctx)
 
-    {effects, errors, _final_ctx} =
-      Enum.reduce(entries, {[], [], ctx}, fn {key, value}, {effects_acc, errors_acc, current_ctx} ->
+    # Accumulate effects by prepending each adapter's effects in reverse, then
+    # reverse the whole list at the end — preserves the original left-to-right
+    # order while staying O(n) in batch size.
+    result =
+      Enum.reduce_while(entries, {[], ctx}, fn {key, value}, {reversed_acc, current_ctx} ->
         {module, adapter_opts} = Router.resolve(key)
 
         case module.put(current_ctx, key, value, merge_opts(adapter_opts, opts)) do
           {:ok, fx} ->
-            {effects_acc ++ fx, errors_acc, apply_effects_to_ctx(current_ctx, fx)}
+            {:cont, {:lists.reverse(fx, reversed_acc), apply_effects_to_ctx(current_ctx, fx)}}
 
           {:error, reason} ->
-            {effects_acc, [{key, reason} | errors_acc], current_ctx}
+            {:halt, {:error, {key, reason}}}
         end
       end)
 
-    case errors do
-      [] -> {:ok, effects}
-      errs -> {:error, Enum.reverse(errs)}
+    case result do
+      {:error, _reason} = err -> err
+      {reversed_acc, _ctx} -> {:ok, Enum.reverse(reversed_acc)}
     end
   end
 

test/controllers/preferences_controller_test.exs

@@ -5,6 +5,7 @@ defmodule Backpex.PreferencesControllerTest do
 
   alias Backpex.Preferences
   alias Backpex.PreferencesController
+  alias Backpex.Test.InMemoryPreferencesAdapter, as: InMemory
 
   setup do
     conn =
@@ -57,7 +58,7 @@ defmodule Backpex.PreferencesControllerTest do
   end
 
   describe "update/2 with a batch" do
-    test "persists all entries atomically", %{conn: conn} do
+    test "persists every entry", %{conn: conn} do
       params = %{
         "preferences" => [
           %{"key" => "global.theme", "value" => "dark"},
@@ -108,14 +109,19 @@ defmodule Backpex.PreferencesControllerTest do
     end
   end
 
-  describe "update/2 with an adapter that refuses the write (all-or-nothing)" do
+  describe "update/2 when an adapter refuses the write (best-effort, first-error-wins)" do
     setup do
-      # Route :test prefix to a stub adapter that always errors.
+      InMemory.reset()
       prior = Application.get_env(:backpex, Backpex.Preferences)
 
+      # Route:
+      #   ok.* → in-memory adapter (eager: writes to ETS on put/4)
+      #   fail.* → rejecting adapter (always errors)
+      #   :default → session
       Application.put_env(:backpex, Backpex.Preferences,
         adapters: [
-          {"test.*", Backpex.PreferencesControllerTest.RejectingAdapter, []},
+          {"ok.*", InMemory, []},
+          {"fail.*", Backpex.PreferencesControllerTest.RejectingAdapter, []},
           {:default, Backpex.Preferences.Adapters.Session, []}
         ]
       )
@@ -130,11 +136,16 @@ defmodule Backpex.PreferencesControllerTest do
       :ok
     end
 
-    test "returns 422 with {ok: false, errors: [...]} and leaves the session untouched", %{conn: conn} do
+    test "halts at the first error and does not dispatch later entries (short-circuit)", %{conn: conn} do
+      # Three-entry batch:
+      #   1. ok.before   → InMemory (succeeds, writes ETS row eagerly)
+      #   2. fail.middle → Rejecting (fails)
+      #   3. ok.after    → InMemory (MUST NOT be dispatched — would write ETS row)
       params = %{
         "preferences" => [
-          %{"key" => "global.theme", "value" => "dark"},
-          %{"key" => "test.thing", "value" => "nope"}
+          %{"key" => "ok.before", "value" => "committed"},
+          %{"key" => "fail.middle", "value" => "nope"},
+          %{"key" => "ok.after", "value" => "should_not_run"}
         ]
       }
 
@@ -143,9 +154,22 @@ defmodule Backpex.PreferencesControllerTest do
       assert conn.status == 422
       body = Jason.decode!(conn.resp_body)
       assert body["ok"] == false
-      assert [%{"key" => "test.thing", "reason" => _reason}] = body["errors"]
+      assert body["error"] == %{"key" => "fail.middle", "reason" => "rejected"}
+
+      stored = InMemory.dump()
+
+      # Path A: the earlier eager write committed — we do NOT claim rollback.
+      assert Map.has_key?(stored, "ok.before")
+      assert stored["ok.before"] == "committed"
+
+      # Short-circuit: the entry after the failure was never dispatched, so
+      # its ETS row was never written. Under the old Enum.reduce/3 it would
+      # have been written and this assertion would fail.
+      refute Map.has_key?(stored, "ok.after")
 
-      # Because the batch failed, nothing was written — session is untouched.
+      # Session-backed effects collected before the failure are never applied
+      # (the controller only calls apply_effects_on_conn on the :ok branch),
+      # so the cookie is untouched.
       assert Plug.Conn.get_session(conn, Preferences.session_key()) == nil
     end
   end

test/preferences_test.exs

@@ -93,11 +93,10 @@ defmodule Backpex.PreferencesTest do
       assert merged == %{"global" => %{"theme" => "dark", "sidebar_open" => true}}
     end
 
-    test "returns {:error, list} when any adapter refuses a write (all-or-nothing)" do
+    test "returns {:error, {key, reason}} on the first adapter refusal (best-effort, first-error-wins)" do
       # :mount source → Session adapter returns :requires_http.
       ctx = Context.from_mount(%{})
-      assert {:error, errors} = Preferences.put_batch(ctx, [{"global.theme", "dark"}])
-      assert [{"global.theme", :requires_http}] = errors
+      assert {:error, {"global.theme", :requires_http}} = Preferences.put_batch(ctx, [{"global.theme", "dark"}])
     end
   end