Subscription keeps processing events without its advisory lock after the lock connection reconnects

Aperta
#330 3 commenti 0 reazioni 0 assegnatari Vedi su GitHub

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
42/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
elixir, postgresql
Ambito
backend, databases

Direzione di ricerca

Start with EventStore.AdvisoryLocks and EventStore.MonitoredServer, then trace how the Postgrex pool reconnects its lock connection and how subscriptions handle lock_released. Use the reproduction script’s pg_terminate_backend case to observe the lost lock and NodeB’s takeover. Done means NodeA receives the lock release notification and retries, so both nodes do not process the same events.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

NodeA and NodeB run the same subscription against one database. NodeA holds the subscription's advisory lock, and NodeB retries the lock every subscription_retry_interval. When NodeA's lock session ends and its lock connection reconnects, NodeA's subscription is not told: it stays subscribed and keeps processing events, although the new session holds no lock. All subscriptions of one event store instance hold their locks on this one connection, so they all lose them at once. At its next retry NodeB takes the free lock, and from then on both nodes process every event. EventStore logs nothing about the lost lock; the only related log line is Postgrex's disconnect error.

I would expect NodeA's subscription to receive {EventStore.AdvisoryLocks, :lock_released, ref, reason} and go back to retrying the lock. It does receive that message when the reconnect is refused.

When it happens

It happens whenever the lock session ends and the connection reconnects on a new session while its pool survives, for example after pg_terminate_backend, which is what the script below does. In a separate test, reconnects that hang led to the same state: behind a TCP proxy that accepted connections and stayed silent for 75 s, the lock connection's reconnect attempts failed four times, 15 s apart (three handshake timeouts, and the fourth attempt was cut off when the proxy forwarded again), and afterwards the subscription was still subscribed with no lock held.

Cause

EventStore.AdvisoryLocks learns about a lost lock only from EventStore.MonitoredServer, which monitors the process returned by Postgrex.start_link, the connection pool, and not the connection inside it. With backoff_type: :stop a disconnect stops the DBConnection.Connection process, but the pool's supervisor starts a new one on a new session and the pool process lives on, so no :DOWN reaches AdvisoryLocks and no lock_released reaches the subscription. The message is sent only when the pool itself dies, and that takes reconnects failing faster than the pool supervisor's restart intensity (3 restarts in 5 s by default), as refused connections do; a connect that times out after 15 s never gets there.

Effect with Commanded

When every node runs the same supervision tree, Commanded's default local registry lets each node run its own copy of every event handler and process manager, so there is always a NodeB waiting for the lock. Once both copies are subscribed, event handlers handle every event on both nodes, and process managers can dispatch their commands twice. If a command is not idempotent, the aggregate appends two events for one source event, both with the same causation_id, as the counter-1 stream below shows. Projectors built with commanded_ecto_projections still commit each event's changes once, because both copies update the same row in projection_versions and only one of the two transactions gets through (checked separately with commanded_ecto_projections 1.4.0).

This may be what the comments on #263 describe: event handlers running on multiple nodes after RDS failovers.

Reproduction

The script needs only PostgreSQL. NodeA and NodeB are two Commanded applications in one BEAM, each on its own instance of one EventStore module (started under two names) on the same database. Both start an event handler, Recorder, that records which node handled which event, and a process manager, IncrementOnPing, that turns every Pinged into an Increment command for a Counter aggregate, which appends one Incremented per command. NodeA subscribes first and takes the locks. The script then terminates NodeA's lock session with pg_terminate_backend, dispatches two Ping commands through NodeA and prints the result. It sets subscription_retry_interval: 1_000 to keep the run short; the default 60 s only delays the takeover.

Run it with PGHOST=localhost PGPORT=5432 PGUSER=postgres PGPASSWORD=postgres elixir repro.exs. It drops and recreates the database named by REPRO_DATABASE (default eventstore_lock_loss_repro). elixir repro.exs control runs the same steps without the termination.

repro.exs
# An EventStore subscription keeps running after its advisory lock session ends, while another
# instance takes the lock, so both process every event.
#
# NodeA and NodeB are two Commanded applications in one BEAM standing in for two nodes. Each runs
# on its own instance of one EventStore module, both instances on the same database, and each
# starts the same event handler (Recorder) and process manager (IncrementOnPing). NodeA subscribes
# first and holds the advisory locks. The script terminates NodeA's lock session with
# pg_terminate_backend, dispatches two Ping commands and prints what happened. Both instances use
# subscription_retry_interval: 1_000, so NodeB retries the lock every second instead of every 60 s.
#
#   elixir repro.exs           # terminate NodeA's lock session
#   elixir repro.exs control   # the same steps without the termination
#
# Connection settings come from PGHOST, PGPORT, PGUSER and PGPASSWORD (defaults localhost, 5432,
# postgres, postgres). REPRO_DATABASE names the database (default eventstore_lock_loss_repro);
# the script drops and recreates it on every run.

Mix.install([
  {:commanded, "1.4.11"},
  {:commanded_eventstore_adapter, "1.4.2"},
  {:eventstore, "1.4.10"},
  {:postgrex, "0.22.4"},
  {:db_connection, "2.10.2"}
])

Logger.configure(level: :warning)

defmodule Ping do
  defstruct [:ping_id, :counter_id]
end

defmodule Pinged do
  defstruct [:ping_id, :counter_id]
end

defmodule Increment do
  defstruct [:counter_id]
end

defmodule Incremented do
  defstruct [:counter_id]
end

defmodule Pinger do
  defstruct [:ping_id]

  def execute(%Pinger{}, %Ping{ping_id: ping_id, counter_id: counter_id}), do: %Pinged{ping_id: ping_id, counter_id: counter_id}

  def apply(%Pinger{} = pinger, %Pinged{ping_id: ping_id}), do: %Pinger{pinger | ping_id: ping_id}
end

defmodule Counter do
  @moduledoc "Every Increment command appends one Incremented event: the command is not idempotent."
  defstruct count: 0

  def execute(%Counter{}, %Increment{counter_id: counter_id}), do: %Incremented{counter_id: counter_id}

  def apply(%Counter{count: count} = counter, %Incremented{}), do: %Counter{counter | count: count + 1}
end

defmodule TagDispatchingNode do
  @moduledoc "Writes the dispatching application into the metadata of every event a command produces."
  @behaviour Commanded.Middleware

  alias Commanded.Middleware.Pipeline

  def before_dispatch(%Pipeline{application: application} = pipeline) do
    Pipeline.assign_metadata(pipeline, "dispatched_by", inspect(application))
  end

  def after_dispatch(pipeline), do: pipeline
  def after_failure(pipeline), do: pipeline
end

defmodule Router do
  use Commanded.Commands.Router

  middleware(TagDispatchingNode)

  dispatch(Ping, to: Pinger, identity: :ping_id)
  dispatch(Increment, to: Counter, identity: :counter_id)
end

defmodule Store do
  use EventStore, otp_app: :repro
end

defmodule NodeA do
  use Commanded.Application, otp_app: :repro
  router(Router)
end

defmodule NodeB do
  use Commanded.Application, otp_app: :repro
  router(Router)
end

defmodule Journal do
  @moduledoc "What the handlers report, with the time in milliseconds since the script started."

  def start_link, do: Agent.start_link(fn -> [] end, name: __MODULE__)

  def record(entry) do
    elapsed_ms = elapsed_ms()
    Agent.update(__MODULE__, &[{elapsed_ms, entry} | &1])
  end

  def elapsed_ms, do: System.monotonic_time(:millisecond) - :persistent_term.get(:repro_started_at)

  def entries do
    __MODULE__
    |> Agent.get(& &1)
    |> Enum.reverse()
  end
end

defmodule Recorder do
  @moduledoc "Event handler that records when its subscription starts and which node handled which event."
  use Commanded.Event.Handler, name: "Recorder"

  def after_start(%{node: node} = state) do
    Journal.record({:subscribed, node})
    {:ok, state}
  end

  def handle(event, %{application: node, event_number: event_number}) do
    Journal.record({:handled, node, event_number, event.__struct__})
    :ok
  end
end

defmodule IncrementOnPing do
  @moduledoc "Process manager: every Pinged event dispatches one Increment command for its counter."
  use Commanded.ProcessManagers.ProcessManager, name: "IncrementOnPing"

  defstruct []

  def interested?(%Pinged{ping_id: ping_id}), do: {:start, ping_id}
  def interested?(_event), do: false

  def handle(%IncrementOnPing{}, %Pinged{counter_id: counter_id}), do: %Increment{counter_id: counter_id}
end

defmodule Database do
  @moduledoc "Database setup and the pg_locks, pg_stat_activity and pg_terminate_backend queries."

  def settings do
    [
      hostname: System.get_env("PGHOST", "localhost"),
      port: String.to_integer(System.get_env("PGPORT", "5432")),
      username: System.get_env("PGUSER", "postgres"),
      password: System.get_env("PGPASSWORD", "postgres")
    ]
  end

  def connect(database) do
    {:ok, conn} = Postgrex.start_link([database: database] ++ settings())
    conn
  end

  def recreate(admin, database) do
    Postgrex.query!(admin, ~s[DROP DATABASE IF EXISTS "#{database}" WITH (FORCE)], [])
    Postgrex.query!(admin, ~s[CREATE DATABASE "#{database}"], [])
    config = Store.config([database: database] ++ settings())
    :ok = EventStore.Tasks.Init.exec(config, quiet: true)
  end

  def server_version(admin) do
    %Postgrex.Result{rows: [[version]]} =
      Postgrex.query!(admin, "SELECT split_part(current_setting('server_version'), ' ', 1)", [])

    version
  end

  @doc "Advisory locks held in the database, as `{application_name, backend_pid, lock_count}`."
  def advisory_locks(admin, database) do
    sql = """
    SELECT a.application_name, l.pid, count(*)
    FROM pg_locks l JOIN pg_stat_activity a ON a.pid = l.pid
    WHERE l.locktype = 'advisory' AND l.granted AND a.datname = $1
    GROUP BY a.application_name, l.pid
    ORDER BY a.application_name, l.pid
    """

    %Postgrex.Result{rows: rows} = Postgrex.query!(admin, sql, [database])
    Enum.map(rows, &List.to_tuple/1)
  end

  def terminate_backend(admin, database, backend_pid) do
    sql = "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE pid = $1 AND datname = $2"
    %Postgrex.Result{rows: [[terminated?]]} = Postgrex.query!(admin, sql, [backend_pid, database])
    terminated?
  end
end

defmodule Repro do
  @versioned_apps [:eventstore, :commanded, :commanded_eventstore_adapter, :postgrex, :db_connection]
  @lock_count_per_node 2

  def run(mode) do
    :persistent_term.put(:repro_started_at, System.monotonic_time(:millisecond))
    database = System.get_env("REPRO_DATABASE", "eventstore_lock_loss_repro")
    admin = Database.connect("postgres")
    Database.recreate(admin, database)
    {:ok, _journal} = Journal.start_link()
    print_versions(admin)

    start_node(NodeA, :node_a, database)
    wait_for_locks(admin, database, "node_a", deadline_in(20_000))
    log("NodeA started. Advisory locks: #{describe_locks(admin, database)}")

    start_node(NodeB, :node_b, database)
    Process.sleep(3_000)
    log("NodeB started 3 s ago. Advisory locks: #{describe_locks(admin, database)}")
    log("NodeA lock connection: #{describe_lock_connection(:node_a)}")

    end_lock_session(mode, admin, database)
    Process.sleep(3_000)
    log("3 s later. Advisory locks: #{describe_locks(admin, database)}")
    log("NodeA lock connection: #{describe_lock_connection(:node_a)}")

    dispatch_ping(NodeA, "ping-1")
    Process.sleep(2_000)
    dispatch_ping(NodeA, "ping-2")
    Process.sleep(2_000)
    log("End. Advisory locks: #{describe_locks(admin, database)}")

    print_subscriptions()
    print_handled_events()
    print_counter_stream()
  end

  defp start_node(application, store_name, database) do
    event_store =
      [
        adapter: Commanded.EventStore.Adapters.EventStore,
        event_store: Store,
        name: store_name,
        serializer: EventStore.TermSerializer,
        database: database,
        subscription_retry_interval: 1_000,
        parameters: [application_name: Atom.to_string(store_name)]
      ] ++ Database.settings()

    {:ok, _application} = application.start_link(event_store: event_store)
    {:ok, _handler} = Recorder.start_link(application: application, state: %{node: application})
    {:ok, _process_manager} = IncrementOnPing.start_link(application: application)
  end

  defp end_lock_session(:control, _admin, _database), do: log("Control run: NodeA's lock session is left alone.")

  defp end_lock_session(:terminate, admin, database) do
    [{"node_a", lock_backend, @lock_count_per_node}] = Database.advisory_locks(admin, database)
    terminated? = Database.terminate_backend(admin, database, lock_backend)
    log("pg_terminate_backend(#{lock_backend}): #{terminated?}")
  end

  defp wait_for_locks(admin, database, application_name, deadline) do
    locks = Database.advisory_locks(admin, database)

    cond do
      Enum.any?(locks, &match?({^application_name, _backend, @lock_count_per_node}, &1)) -> :ok
      System.monotonic_time(:millisecond) > deadline -> raise "#{application_name} did not take its advisory locks"
      true -> retry_wait_for_locks(admin, database, application_name, deadline)
    end
  end

  defp retry_wait_for_locks(admin, database, application_name, deadline) do
    Process.sleep(100)
    wait_for_locks(admin, database, application_name, deadline)
  end

  defp deadline_in(timeout_ms), do: System.monotonic_time(:millisecond) + timeout_ms

  defp describe_locks(admin, database) do
    case Database.advisory_locks(admin, database) do
      [] -> "none"
      locks -> Enum.map_join(locks, ", ", &describe_lock_holder/1)
    end
  end

  defp describe_lock_holder({application_name, backend, count}), do: "#{application_name} backend #{backend} holds #{count}"

  # The pool process EventStore watches to detect a lost lock, and the database session behind it.
  defp describe_lock_connection(store_name) do
    pool_name = Module.concat([store_name, EventStore.AdvisoryLocks, Postgrex])
    %{pid: pool} = :sys.get_state(pool_name)
    session_backend = fetch_session_backend(pool_name, deadline_in(15_000))
    "pool process #{inspect(pool)}, session backend #{session_backend}"
  end

  defp fetch_session_backend(pool_name, deadline) do
    case Postgrex.query(pool_name, "SELECT pg_backend_pid()", []) do
      {:ok, %Postgrex.Result{rows: [[backend]]}} -> backend
      {:error, _reason} -> retry_fetch_session_backend(pool_name, deadline)
    end
  end

  defp retry_fetch_session_backend(pool_name, deadline) do
    if System.monotonic_time(:millisecond) > deadline,
      do: raise("the lock connection did not reconnect"),
      else: Process.sleep(250)

    fetch_session_backend(pool_name, deadline)
  end

  defp dispatch_ping(application, ping_id) do
    result = application.dispatch(%Ping{ping_id: ping_id, counter_id: "counter-1"})
    log("Dispatched Ping #{ping_id} through #{inspect(application)}: #{inspect(result)}")
  end

  defp print_versions(admin) do
    versions = Enum.map_join(@versioned_apps, ", ", &"#{&1} #{Application.spec(&1, :vsn)}")
    log("#{versions}, PostgreSQL #{Database.server_version(admin)}, Elixir #{System.version()}, OTP #{System.otp_release()}")
  end

  defp print_subscriptions do
    IO.puts("\nRecorder subscribed (after_start):")
    for {elapsed_ms, {:subscribed, node}} <- Journal.entries(), do: IO.puts("  #{inspect(node)} at #{format_seconds(elapsed_ms)}")
  end

  defp print_handled_events do
    IO.puts("\nEvents handled by Recorder:")

    handled =
      for {_elapsed_ms, {:handled, node, event_number, event_type}} <- Journal.entries(),
          do: {{event_number, event_type}, node}

    handled_by = Enum.group_by(handled, &elem(&1, 0), &elem(&1, 1))

    for {{event_number, event_type}, nodes} <- Enum.sort(handled_by) do
      node_names =
        nodes
        |> Enum.sort()
        |> Enum.map_join(", ", &inspect/1)

      IO.puts("  ##{event_number} #{inspect(event_type)}: #{node_names}")
    end
  end

  defp print_counter_stream do
    {:ok, all_events} = Store.read_all_streams_forward(0, 1_000, name: :node_a)

    ping_by_event_id =
      for %{data: %Pinged{ping_id: ping_id}, event_id: event_id} <- all_events, into: %{}, do: {event_id, ping_id}

    {:ok, counter_events} = Store.read_stream_forward("counter-1", 0, 1_000, name: :node_a)
    IO.puts("\nStream counter-1:")

    for event <- counter_events do
      ping = Map.fetch!(ping_by_event_id, event.causation_id)
      dispatched_by = event.metadata["dispatched_by"]

      IO.puts(
        "  version #{event.stream_version} #{inspect(event.data.__struct__)}, causation_id #{event.causation_id} (Pinged #{ping}), dispatched by #{dispatched_by}"
      )
    end
  end

  defp log(text), do: IO.puts("[#{format_seconds(Journal.elapsed_ms())}] #{text}")

  defp format_seconds(elapsed_ms), do: :erlang.float_to_binary(elapsed_ms / 1000, decimals: 2) <> "s"
end

mode =
  case System.argv() do
    [] -> :terminate
    ["control"] -> :control
  end

Repro.run(mode)

The output below is from one of two runs, which differed only in ids, timings, which node won each append and whether the second race logged a warning. The NodeA lock connection lines show the same pool process before and after the termination, the second time on a new session that holds no lock. The two warnings come from the two Increment commands racing on counter-1; each time the losing append is retried and goes through.

[0.24s] eventstore 1.4.10, commanded 1.4.11, commanded_eventstore_adapter 1.4.2, postgrex 0.22.4, db_connection 2.10.2, PostgreSQL 18.6, Elixir 1.18.4, OTP 27
[0.37s] NodeA started. Advisory locks: node_a backend 78846 holds 2
[3.38s] NodeB started 3 s ago. Advisory locks: node_a backend 78846 holds 2
[3.38s] NodeA lock connection: pool process #PID<0.292.0>, session backend 78846
[3.38s] pg_terminate_backend(78846): true

16:01:16.800 [error] Postgrex.Protocol (#PID<0.294.0> ({Postgrex.Protocol, "eventstore_lock_loss_issue1_repro"})) disconnected: ** (Postgrex.Error) FATAL 57P01 (admin_shutdown) terminating connection due to administrator command
[6.39s] 3 s later. Advisory locks: node_b backend 78859 holds 2
[6.40s] NodeA lock connection: pool process #PID<0.292.0>, session backend 79334
[6.43s] Dispatched Ping ping-1 through NodeA: :ok

16:01:17.987 [warning] Failed to append events to stream "counter-1" due to: :duplicate_stream_uuid
[8.45s] Dispatched Ping ping-2 through NodeA: :ok

16:01:20.003 [warning] Failed to append events to stream "counter-1" due to: :wrong_expected_version
[10.45s] End. Advisory locks: node_b backend 78859 holds 2

Recorder subscribed (after_start):
  NodeA at 0.27s
  NodeB at 3.47s

Events handled by Recorder:
  #1 Pinged: NodeA, NodeB
  #2 Incremented: NodeA, NodeB
  #3 Incremented: NodeA, NodeB
  #4 Pinged: NodeA, NodeB
  #5 Incremented: NodeA, NodeB
  #6 Incremented: NodeA, NodeB

Stream counter-1:
  version 1 Incremented, causation_id 6fb6e546-5b0a-4ada-bc0f-feee8710ddc5 (Pinged ping-1), dispatched by NodeB
  version 2 Incremented, causation_id 6fb6e546-5b0a-4ada-bc0f-feee8710ddc5 (Pinged ping-1), dispatched by NodeA
  version 3 Incremented, causation_id 3d0c53dd-3bd2-4420-8bb0-141d9da26a1c (Pinged ping-2), dispatched by NodeA
  version 4 Incremented, causation_id 3d0c53dd-3bd2-4420-8bb0-141d9da26a1c (Pinged ping-2), dispatched by NodeB

The control run, without the termination, ends with NodeB never subscribed and one Incremented per Pinged:

[10.40s] End. Advisory locks: node_a backend 81220 holds 2

Recorder subscribed (after_start):
  NodeA at 0.25s

Events handled by Recorder:
  #1 Pinged: NodeA
  #2 Incremented: NodeA
  #3 Pinged: NodeA
  #4 Incremented: NodeA

Stream counter-1:
  version 1 Incremented, causation_id 41de3751-4db6-4f7f-b467-adc47c69f94c (Pinged ping-1), dispatched by NodeA
  version 2 Incremented, causation_id 56fd900a-896c-4172-a711-fb173f102f57 (Pinged ping-2), dispatched by NodeA
Versions

eventstore 1.4.10 (the same code as master at 49ba9f9), commanded 1.4.11, commanded_eventstore_adapter 1.4.2, postgrex 0.22.4, db_connection 2.10.2, PostgreSQL 18.6, Elixir 1.18.4, OTP 27.

Lingua principale
Elixir
Stelle
1.2k
Fork
156
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Preparare l'ambiente

  • Include un Dockerfile o un file Docker Compose
  • Nessun modello di pull request
  • Nessuna guida per i contributori

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di commanded/eventstore

Tutte le issue di commanded/eventstore

Issue simili

Altre issue su Elixir

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.