From bae760d8239175ed4dbcef9e5bddf04f450bc192 Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 1 Jul 2026 15:02:41 +0100 Subject: [PATCH 1/5] test(oracle): add graceful/brutal/rolling restart strategies The oracle property test could restart the stack every N batches (RESTART_SERVER_EVERY) but only ever did a graceful shutdown. Introduce a Support.OracleHarness.RestartStrategy behaviour with three implementations selected via RESTART_TYPE: - graceful (default) - clean stop_supervised + restore from disk - brutal - Process.exit(pid, :kill) crash, then recover same stack_id - rolling - a new stack contends on the same replication slot via the advisory lock, then the old stack is gracefully stopped so the new one takes over (faithful to cloud rolling deploys) start_stack_supervisor! is now public with :id/:await_ready? opts, and Bandit setup is factored into IntegrationSetup.start_bandit_client/2 with a shared Finch pool so a second HTTP server can coexist during a rolling deploy. --- .../test/integration/oracle_property_test.exs | 5 + .../test/support/component_setup.ex | 217 +++++++++++++++++- .../test/support/integration_setup.ex | 84 +++++-- .../test/support/oracle_harness.ex | 38 ++- .../oracle_harness/restart_strategy.ex | 39 ++++ .../oracle_harness/restart_strategy/brutal.ex | 12 + .../restart_strategy/graceful.ex | 11 + .../restart_strategy/rolling_deploy.ex | 15 ++ 8 files changed, 385 insertions(+), 36 deletions(-) create mode 100644 packages/sync-service/test/support/oracle_harness/restart_strategy.ex create mode 100644 packages/sync-service/test/support/oracle_harness/restart_strategy/brutal.ex create mode 100644 packages/sync-service/test/support/oracle_harness/restart_strategy/graceful.ex create mode 100644 packages/sync-service/test/support/oracle_harness/restart_strategy/rolling_deploy.ex diff --git a/packages/sync-service/test/integration/oracle_property_test.exs b/packages/sync-service/test/integration/oracle_property_test.exs index 643fbd84ef..97c5785523 100644 --- a/packages/sync-service/test/integration/oracle_property_test.exs +++ b/packages/sync-service/test/integration/oracle_property_test.exs @@ -16,6 +16,11 @@ defmodule Electric.Integration.OraclePropertyTest do test server-side restore-from-file (default: 0, disabled). After each restart, fresh clients reconnect and check_initial_state asserts the restored state matches the oracle. + - RESTART_TYPE: How the RESTART_SERVER_EVERY restart is performed: + "graceful" (default, clean shutdown + restore from disk), "brutal" + (kill -9 style crash + recover), or "rolling" (rolling deploy: a new + stack takes over the replication slot before the old one is stopped). + See `Support.OracleHarness.RestartStrategy`. - RESTART_CLIENT_EVERY: Throw away clients (poll cursors, materialized rows) and reconnect every M batches to test that fresh polls correctly assemble snapshot + log (default: 0, disabled). Independent of diff --git a/packages/sync-service/test/support/component_setup.ex b/packages/sync-service/test/support/component_setup.ex index 5ba6348cc9..768329b9db 100644 --- a/packages/sync-service/test/support/component_setup.ex +++ b/packages/sync-service/test/support/component_setup.ex @@ -484,14 +484,212 @@ defmodule Support.ComponentSetup do %{stack_supervisor: stack_supervisor} end - defp start_stack_supervisor!( - ctx, - stack_id, - kv, - storage, - stack_events_registry, - publication_name - ) do + @doc """ + Like `restart_complete_stack/1`, but simulates a crash: the running + `Electric.StackSupervisor` is killed outright (`Process.exit(pid, :kill)`, + no terminate callbacks) rather than shut down gracefully. A fresh stack is + then started on the same `stack_id`/storage so recovery from on-disk state + (possibly torn writes, an orphaned replication slot, a stale advisory lock) + is exercised. + + Returns a map with the updated `:stack_supervisor` pid. + """ + def brutally_restart_complete_stack(ctx) do + pid = ctx.stack_supervisor + ref = Process.monitor(pid) + Process.exit(pid, :kill) + + receive do + {:DOWN, ^ref, :process, ^pid, _reason} -> :ok + after + 5_000 -> flunk("stack supervisor #{inspect(pid)} did not die after :kill") + end + + # The child was `restart: :temporary`, so ExUnit does not restart it; its + # spec lingers with an :undefined pid. Remove it so the id is free to reuse. + case stop_supervised(Electric.StackSupervisor) do + :ok -> :ok + {:error, :not_found} -> :ok + end + + # A brutal kill propagates asynchronously; wait for the stack's registered + # processes (and their ETS tables) to actually be gone before starting a + # new stack with the same stack_id, else the new stack races name/table + # reuse and crashes on boot. + wait_for_stack_processes_down(ctx.stack_id) + + stack_supervisor = + start_stack_supervisor!( + ctx, + ctx.stack_id, + ctx.persistent_kv, + ctx.storage, + ctx.stack_events_registry, + ctx.publication_name + ) + + # Generous timeout: worst case the stale advisory lock is only cleared on + # the periodic lock-breaker cycle (~10s), not immediately on kill. + :ok = Electric.StatusMonitor.wait_until_active(ctx.stack_id, timeout: 15_000) + + %{stack_supervisor: stack_supervisor} + end + + @doc """ + Performs a rolling deploy: a brand new stack (its own `stack_id`, storage and + HTTP server) is brought up that contends on the *same* replication slot as + the running stack via the Postgres advisory lock. The new stack boots to + `read_only` (blocked acquiring the lock the old stack holds), then the old + stack is gracefully stopped — releasing the lock and leaving the persistent + slot and publication in place — and the new stack takes over as the single + active writer. + + Returns a ctx-merge map swapping in the new stack: `stack_id`, `storage`, + `persistent_kv`, `registry`, `inspector`, `shape_cache`, `stack_supervisor`, + `stack_supervisor_id`, plus a new `client`/`base_url`/`server_pid`/`port` and + `server_id`/`rolling_gen` bookkeeping. + """ + def rolling_restart_complete_stack(ctx) do + old_stack_id = ctx.stack_id + old_sup_id = Map.get(ctx, :stack_supervisor_id, Electric.StackSupervisor) + old_server_id = Map.get(ctx, :server_id, Bandit) + + # The old stack derived its slot name from its stack_id in + # start_stack_supervisor!/7; force the new stack onto the same slot so they + # contend on one advisory lock. + old_slot_name = "electric_test_slot_#{:erlang.phash2(old_stack_id)}" + + gen = Map.get(ctx, :rolling_gen, 0) + 1 + new_stack_id = "#{old_stack_id}_roll#{gen}" + new_sup_id = {Electric.StackSupervisor, new_stack_id} + new_server_id = {Bandit, new_stack_id} + + new_kv = %Electric.PersistentKV.Memory{ + parent: self(), + pid: + start_supervised!(Electric.PersistentKV.Memory, + id: {Electric.PersistentKV.Memory, new_stack_id}, + restart: :temporary + ) + } + + new_storage = {PureFileStorage, stack_id: new_stack_id, storage_dir: ctx.tmp_dir} + + # (1) Start the new stack forced onto the old slot + same publication. It + # will block acquiring the advisory lock (held by the old stack), so it + # never emits :ready — don't wait for it. + start_ctx = + Map.put( + ctx, + :replication_opts_overrides, + Keyword.merge( + List.wrap(ctx[:replication_opts_overrides]), + slot_name: old_slot_name, + slot_temporary?: false + ) + ) + + new_stack_supervisor = + start_stack_supervisor!( + start_ctx, + new_stack_id, + new_kv, + new_storage, + ctx.stack_events_registry, + ctx.publication_name, + id: new_sup_id, + await_ready?: false + ) + + # (2) read_only is reachable without the lock (shape metadata is loaded + # before lock acquisition), confirming the new tree booted. + {:ok, :read_only} = + Electric.StatusMonitor.wait_until(new_stack_id, :read_only, timeout: 5_000) + + new_ctx = + Map.merge(ctx, %{ + stack_id: new_stack_id, + storage: new_storage, + persistent_kv: new_kv, + registry: Electric.StackSupervisor.registry_name(new_stack_id), + inspector: + {EtsInspector, + stack_id: new_stack_id, server: EtsInspector.name(stack_id: new_stack_id)}, + shape_cache: {ShapeCache, [stack_id: new_stack_id]}, + stack_supervisor: new_stack_supervisor, + stack_supervisor_id: new_sup_id + }) + + # (3) New Bandit + client pointed at the new stack (no readiness wait — the + # new stack is still lock-blocked). Reuse the shared Finch pool. + server = + Support.IntegrationSetup.start_bandit_client(new_ctx, + id: new_server_id, + router_opts: Keyword.get(Map.get(ctx, :electric_client_opts, []), :router_opts, []), + client_opts: Support.IntegrationSetup.finch_client_opts(Map.get(ctx, :finch_name)) + ) + + # (4) Gracefully stop the old stack: releases the advisory lock; the + # persistent slot and publication survive (no drop on a normal stop). + :ok = stop_supervised(old_sup_id) + _ = stop_supervised(old_server_id) + + # (5) The new stack now acquires the lock, finds the existing slot + + # publication (duplicate → no shape purge) and becomes active. + :ok = Electric.StatusMonitor.wait_until_active(new_stack_id, timeout: 10_000) + + new_ctx + |> Map.merge(server) + |> Map.merge(%{server_id: new_server_id, rolling_gen: gen}) + end + + # Polls until the stack's registered processes are gone. Uses + # ProcessRegistry.alive?/2 which tolerates the per-stack registry itself + # being torn down (returns false rather than raising). + defp wait_for_stack_processes_down(stack_id, deadline \\ nil) do + deadline = deadline || System.monotonic_time(:millisecond) + 5_000 + + down? = + not Electric.ProcessRegistry.alive?(stack_id, Electric.StatusMonitor) and + not Electric.ProcessRegistry.alive?(stack_id, Electric.Connection.Manager) and + GenServer.whereis(Electric.ProcessRegistry.registry_name(stack_id)) == nil + + cond do + down? -> + :ok + + System.monotonic_time(:millisecond) > deadline -> + flunk("stack #{stack_id} did not fully terminate after brutal kill") + + true -> + Process.sleep(10) + wait_for_stack_processes_down(stack_id, deadline) + end + end + + @doc """ + Starts an `Electric.StackSupervisor` under the test supervision tree. + + Options: + - `:id` - ExUnit child id (default `Electric.StackSupervisor`). Must be + distinct when running two stacks concurrently (e.g. a rolling deploy). + - `:await_ready?` - block until the `{:stack_status, :ready}` event fires + (default `true`). Pass `false` for a stack that is expected to block + acquiring the replication advisory lock and therefore never reach `:ready` + until another stack releases it. + """ + def start_stack_supervisor!( + ctx, + stack_id, + kv, + storage, + stack_events_registry, + publication_name, + opts \\ [] + ) do + id = Keyword.get(opts, :id, Electric.StackSupervisor) + await_ready? = Keyword.get(opts, :await_ready?, true) + ref = Electric.StackSupervisor.subscribe_to_stack_events(stack_id) connection_opts = @@ -541,13 +739,14 @@ defmodule Support.ComponentSetup do shape_db_opts: [ storage_dir: ctx.tmp_dir ]}, + id: id, restart: :temporary, significant: false ) # allow a reasonable time for full stack setup to account for # potential CI slowness, including PG - assert_receive {:stack_status, ^ref, :ready}, 2000 + if await_ready?, do: assert_receive({:stack_status, ^ref, :ready}, 2000) stack_supervisor end diff --git a/packages/sync-service/test/support/integration_setup.ex b/packages/sync-service/test/support/integration_setup.ex index 2873c80bbb..bb061a5f2e 100644 --- a/packages/sync-service/test/support/integration_setup.ex +++ b/packages/sync-service/test/support/integration_setup.ex @@ -13,46 +13,86 @@ defmodule Support.IntegrationSetup do - `base_url` - The base URL of the server - `server_pid` - The Bandit server process - `port` - The port the server is listening on + - `finch_name` - the shared Finch pool name (or nil), so later HTTP servers + (e.g. a rolling deploy's replacement stack) can reuse it + - `electric_client_opts` - the opts this was called with, so a replacement + server can be built with the same router/client configuration """ def with_electric_client(ctx, opts \\ []) do :ok = Electric.StatusMonitor.wait_until_active(ctx.stack_id, timeout: 2000) - router_opts = build_router_opts(ctx, Keyword.get(opts, :router_opts, [])) num_clients = Keyword.get(opts, :num_clients, 1) + # Start a shared Finch pool once (when pooling is needed) so that a second + # HTTP server started later in the test — e.g. the replacement stack in a + # rolling deploy — can reuse it rather than starting a second Finch under + # the same child id. + finch_name = + if num_clients > 1 do + finch_name = :"Electric.Client.Finch.Test.#{System.unique_integer([:positive])}" + + {:ok, _} = + ExUnit.Callbacks.start_supervised( + {Finch, name: finch_name, pools: %{default: [size: num_clients]}} + ) + + finch_name + end + + server = + start_bandit_client(ctx, + id: Bandit, + router_opts: Keyword.get(opts, :router_opts, []), + client_opts: finch_client_opts(finch_name) + ) + + ctx + |> Map.merge(server) + |> Map.merge(%{finch_name: finch_name, electric_client_opts: opts}) + end + + @doc """ + Starts a Bandit HTTP server bound to `ctx`'s stack and an `Electric.Client` + pointed at it. Unlike `with_electric_client/2` this does NOT wait for the + stack to be active (the caller manages readiness) and takes an explicit child + `:id` so several servers can coexist in one test. + + Options: + - `:id` - Bandit child id (default `Bandit`) + - `:router_opts` - extra router opts merged into `build_router_opts/2` + - `:client_opts` - extra opts passed to `Electric.Client.new/1` + + Returns `%{client:, base_url:, server_pid:, port:}`. + """ + def start_bandit_client(ctx, opts \\ []) do + id = Keyword.get(opts, :id, Bandit) + router_opts = build_router_opts(ctx, Keyword.get(opts, :router_opts, [])) + client_opts = Keyword.get(opts, :client_opts, []) + {:ok, server_pid} = ExUnit.Callbacks.start_supervised( {Bandit, plug: {Electric.Plug.Router, router_opts}, port: 0, ip: :loopback, - thousand_island_options: [num_acceptors: 1]} + thousand_island_options: [num_acceptors: 1]}, + id: id ) {:ok, {_ip, port}} = ThousandIsland.listener_info(server_pid) base_url = "http://localhost:#{port}" - client_opts = - if num_clients > 1 do - finch_name = :"Electric.Client.Finch.Test.#{System.unique_integer([:positive])}" - - {:ok, _} = - ExUnit.Callbacks.start_supervised( - {Finch, name: finch_name, pools: %{default: [size: num_clients]}} - ) - - [fetch: {Electric.Client.Fetch.HTTP, [request: [finch: finch_name]]}] - else - [] - end - {:ok, client} = Electric.Client.new([base_url: base_url] ++ client_opts) - Map.merge(ctx, %{ - client: client, - base_url: base_url, - server_pid: server_pid, - port: port - }) + %{client: client, base_url: base_url, server_pid: server_pid, port: port} end + + @doc """ + Builds the `Electric.Client` opts needed to route requests through a shared + Finch pool, or `[]` when there is no pool. + """ + def finch_client_opts(nil), do: [] + + def finch_client_opts(finch_name), + do: [fetch: {Electric.Client.Fetch.HTTP, [request: [finch: finch_name]]}] end diff --git a/packages/sync-service/test/support/oracle_harness.ex b/packages/sync-service/test/support/oracle_harness.ex index c9b5d92cf7..2978809dc9 100644 --- a/packages/sync-service/test/support/oracle_harness.ex +++ b/packages/sync-service/test/support/oracle_harness.ex @@ -34,6 +34,7 @@ defmodule Support.OracleHarness do """ alias Support.OracleHarness.ShapeChecker + alias Support.OracleHarness.RestartStrategy @type shape :: %{ name: String.t(), @@ -56,7 +57,10 @@ defmodule Support.OracleHarness do # ---------------------------------------------------------------------------- def default_opts_from_env do - [oracle_pool_size: env_int("ORACLE_POOL_SIZE") || @default_oracle_pool_size] + [ + oracle_pool_size: env_int("ORACLE_POOL_SIZE") || @default_oracle_pool_size, + restart_type: env_string("RESTART_TYPE") || "graceful" + ] end @doc """ @@ -73,6 +77,9 @@ defmodule Support.OracleHarness do - :timeout_ms - timeout for waiting on shapes (default: 10_000) - :restart_server_every - restart the StackSupervisor every N batches to exercise restore-from-disk (default: 0, disabled) + - :restart_type - how the server restart is performed: "graceful" (default), + "brutal" (crash + recover), or "rolling" (rolling deploy). See + `Support.OracleHarness.RestartStrategy`. Env: RESTART_TYPE. - :restart_client_every - throw away and recreate the shape clients every M batches to exercise fresh-poll consistency (default: 0, disabled) """ @@ -82,6 +89,7 @@ defmodule Support.OracleHarness do timeout_ms = opts[:timeout_ms] || env_int("CHECK_TIMEOUT") || @default_timeout_ms restart_server_every = opts[:restart_server_every] || 0 restart_client_every = opts[:restart_client_every] || 0 + restart_strategy = RestartStrategy.for_type(opts[:restart_type] || "graceful") log_test_config(shapes, batches) @@ -121,7 +129,15 @@ defmodule Support.OracleHarness do # server about to go down) and recreates them after the stack is # back up. If a client restart is also due this batch, it's # subsumed by the recreate that follows the server restart. - restart_server(ctx, pids, shapes, oracle_pool, timeout_ms, batch_idx) + restart_server( + ctx, + pids, + shapes, + oracle_pool, + timeout_ms, + batch_idx, + restart_strategy + ) restart_client? -> new_pids = recreate_checkers(ctx, pids, shapes, oracle_pool, timeout_ms, batch_idx) @@ -172,12 +188,12 @@ defmodule Support.OracleHarness do # Restarts the Electric stack (server-side restore-from-file test) and # reconnects the clients. Old checkers are stopped because their polls are # against the server that is about to go down. - defp restart_server(ctx, pids, shapes, oracle_pool, timeout_ms, batch_idx) do - log("Restarting server after batch_#{batch_idx}") + defp restart_server(ctx, pids, shapes, oracle_pool, timeout_ms, batch_idx, strategy) do + log("Restarting server (#{inspect(strategy)}) after batch_#{batch_idx}") Enum.each(pids, &GenServer.stop/1) - new_ctx = Map.merge(ctx, Support.ComponentSetup.restart_complete_stack(ctx)) + new_ctx = Map.merge(ctx, strategy.restart(ctx)) new_pids = recreate_checkers(new_ctx, [], shapes, oracle_pool, timeout_ms, batch_idx) @@ -305,6 +321,18 @@ defmodule Support.OracleHarness do end end + @doc """ + Reads an environment variable as a string. + Returns nil if unset or empty. + """ + def env_string(name) do + case System.get_env(name) do + nil -> nil + "" -> nil + value -> value + end + end + defp log(message) do IO.puts("[oracle] #{message}") end diff --git a/packages/sync-service/test/support/oracle_harness/restart_strategy.ex b/packages/sync-service/test/support/oracle_harness/restart_strategy.ex new file mode 100644 index 0000000000..0b954d297b --- /dev/null +++ b/packages/sync-service/test/support/oracle_harness/restart_strategy.ex @@ -0,0 +1,39 @@ +defmodule Support.OracleHarness.RestartStrategy do + @moduledoc """ + Strategy for restarting the Electric stack part-way through an oracle test run. + + The oracle harness restarts the stack every `RESTART_SERVER_EVERY` batches to + exercise recovery. This behaviour abstracts *how* that restart happens so the + same test can cover several failure/deploy modes, selected with the + `RESTART_TYPE` env var: + + - `graceful` (default) — clean `stop_supervised` then restart the same + `stack_id`, restoring from disk. See `#{inspect(__MODULE__.Graceful)}`. + - `brutal` — simulate a crash (`Process.exit(pid, :kill)`, no terminate + callbacks) then recover the same `stack_id` from on-disk state. See + `#{inspect(__MODULE__.Brutal)}`. + - `rolling` — rolling deploy: a new stack boots and contends on the same + Postgres replication slot via the advisory lock, then the old stack is + gracefully stopped so the new one takes over. See + `#{inspect(__MODULE__.RollingDeploy)}`. + + `restart/1` receives the current test `ctx` and returns a map that is merged + back into `ctx`. At minimum it updates `:stack_supervisor`; the rolling + strategy also swaps in a new `:stack_id`, `:storage`, `:client`, etc. so the + harness reconnects its checkers to the new stack. + """ + + @callback restart(ctx :: map()) :: map() + + @spec for_type(String.t()) :: module() + def for_type("graceful"), do: __MODULE__.Graceful + def for_type("brutal"), do: __MODULE__.Brutal + def for_type("rolling"), do: __MODULE__.RollingDeploy + + def for_type(other), + do: + raise( + ArgumentError, + "unknown RESTART_TYPE=#{inspect(other)} (expected \"graceful\", \"brutal\" or \"rolling\")" + ) +end diff --git a/packages/sync-service/test/support/oracle_harness/restart_strategy/brutal.ex b/packages/sync-service/test/support/oracle_harness/restart_strategy/brutal.ex new file mode 100644 index 0000000000..e411956c58 --- /dev/null +++ b/packages/sync-service/test/support/oracle_harness/restart_strategy/brutal.ex @@ -0,0 +1,12 @@ +defmodule Support.OracleHarness.RestartStrategy.Brutal do + @moduledoc """ + Brutal restart: kill the running stack outright (`Process.exit(pid, :kill)`, + no terminate callbacks, possibly mid-write) then bring a fresh stack back up + on the same `stack_id`/storage. Exercises crash recovery — stale advisory + lock, torn on-disk log, orphaned replication slot. + """ + @behaviour Support.OracleHarness.RestartStrategy + + @impl true + def restart(ctx), do: Support.ComponentSetup.brutally_restart_complete_stack(ctx) +end diff --git a/packages/sync-service/test/support/oracle_harness/restart_strategy/graceful.ex b/packages/sync-service/test/support/oracle_harness/restart_strategy/graceful.ex new file mode 100644 index 0000000000..ad4a6f3b29 --- /dev/null +++ b/packages/sync-service/test/support/oracle_harness/restart_strategy/graceful.ex @@ -0,0 +1,11 @@ +defmodule Support.OracleHarness.RestartStrategy.Graceful do + @moduledoc """ + Graceful restart: cleanly stop the running `Electric.StackSupervisor` and + start a fresh one with the same `stack_id`, storage and slot, so the new + stack restores everything the old one persisted to disk. + """ + @behaviour Support.OracleHarness.RestartStrategy + + @impl true + def restart(ctx), do: Support.ComponentSetup.restart_complete_stack(ctx) +end diff --git a/packages/sync-service/test/support/oracle_harness/restart_strategy/rolling_deploy.ex b/packages/sync-service/test/support/oracle_harness/restart_strategy/rolling_deploy.ex new file mode 100644 index 0000000000..dbdb83b264 --- /dev/null +++ b/packages/sync-service/test/support/oracle_harness/restart_strategy/rolling_deploy.ex @@ -0,0 +1,15 @@ +defmodule Support.OracleHarness.RestartStrategy.RollingDeploy do + @moduledoc """ + Rolling deploy: bring up a brand new stack (its own `stack_id`, storage and + HTTP server) that contends on the *same* Postgres replication slot as the + running stack via the advisory lock. The new stack boots to `read_only` + (blocked acquiring the lock the old stack holds), then the old stack is + gracefully stopped — releasing the lock and leaving the persistent slot and + publication in place — and the new stack takes over as the single active + writer. Faithful to how Electric-cloud performs zero-downtime deploys. + """ + @behaviour Support.OracleHarness.RestartStrategy + + @impl true + def restart(ctx), do: Support.ComponentSetup.rolling_restart_complete_stack(ctx) +end From 6250ce57bfcf9c53683bdbfd0fd0637497a9a5d6 Mon Sep 17 00:00:00 2001 From: rob Date: Thu, 2 Jul 2026 09:48:49 +0100 Subject: [PATCH 2/5] test(oracle): opt-in retry of transient poll errors Add RETRY_TRANSIENT_ERRORS (default off). When enabled, the shape checker retries a transient poll error (5xx response or a connection-level failure) within the check timeout instead of failing immediately. These are availability blips that production hides from clients during a restart/deploy (HTTP drains before teardown; the load balancer switches after the new instance is ready), so they are not data-consistency signals. Default behaviour is unchanged: any poll error fails. Regardless of the flag, 409/must-refetch, 4xx errors and data mismatches still fail, so real bugs are never masked. --- .../test/integration/oracle_property_test.exs | 6 +++ .../support/oracle_harness/shape_checker.ex | 42 +++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/packages/sync-service/test/integration/oracle_property_test.exs b/packages/sync-service/test/integration/oracle_property_test.exs index 97c5785523..b8032e73b9 100644 --- a/packages/sync-service/test/integration/oracle_property_test.exs +++ b/packages/sync-service/test/integration/oracle_property_test.exs @@ -21,6 +21,12 @@ defmodule Electric.Integration.OraclePropertyTest do (kill -9 style crash + recover), or "rolling" (rolling deploy: a new stack takes over the replication slot before the old one is stopped). See `Support.OracleHarness.RestartStrategy`. + - RETRY_TRANSIENT_ERRORS: When "true"/"1", a transient poll error (5xx or a + connection error) is retried within the check timeout instead of failing + the test. These are availability blips that production hides from clients + during a restart/deploy, so they are not consistency signals. Default off + (any poll error fails, preserving current behaviour). 409/must-refetch and + 4xx errors and data mismatches always fail regardless of this flag. - RESTART_CLIENT_EVERY: Throw away clients (poll cursors, materialized rows) and reconnect every M batches to test that fresh polls correctly assemble snapshot + log (default: 0, disabled). Independent of diff --git a/packages/sync-service/test/support/oracle_harness/shape_checker.ex b/packages/sync-service/test/support/oracle_harness/shape_checker.ex index 82161fc2cb..d7749279f7 100644 --- a/packages/sync-service/test/support/oracle_harness/shape_checker.ex +++ b/packages/sync-service/test/support/oracle_harness/shape_checker.ex @@ -43,9 +43,16 @@ defmodule Support.OracleHarness.ShapeChecker do poll_state: nil, rows: %{}, # Cached oracle state from previous check (used as "before" for next check) - oracle_before: nil + oracle_before: nil, + # When true, transient poll errors (5xx / connection errors) are retried + # within timeout_ms instead of failing immediately. Opt-in via + # RETRY_TRANSIENT_ERRORS; see do_await/2. + retry_transient_errors?: false ] + # Backoff between retries of a transient poll error. + @transient_retry_backoff_ms 50 + # --------------------------------------------------------------------------- # Public API # --------------------------------------------------------------------------- @@ -101,7 +108,8 @@ defmodule Support.OracleHarness.ShapeChecker do shape_def: shape_def, oracle_pool: oracle_pool, timeout_ms: timeout_ms, - poll_state: ShapeState.new() + poll_state: ShapeState.new(), + retry_transient_errors?: retry_transient_errors?() } {:ok, state} @@ -181,11 +189,39 @@ defmodule Support.OracleHarness.ShapeChecker do do_await(state, start_ms) {:error, error} -> - flunk("Poll error for shape=#{state.name} where=#{state.where}: #{inspect(error)}") + # A 5xx or a connection error is an availability signal, never a + # data-consistency one. Production hides restart/deploy windows from + # clients (HTTP drains before teardown; the LB switches after the new + # instance is ready), so a transient error here does not reflect a + # production bug. When opted in, retry within timeout_ms; a genuine + # persistent error still surfaces as a consistency failure once the + # window is exhausted. 409s are handled separately (:must_refetch) and + # 4xx errors are treated as real and still fail immediately. + if state.retry_transient_errors? and transient_error?(error) do + Process.sleep(@transient_retry_backoff_ms) + do_await(state, start_ms) + else + flunk("Poll error for shape=#{state.name} where=#{state.where}: #{inspect(error)}") + end end end end + # A 5xx response or a transport-level failure (no HTTP status) is transient. + # Any other HTTP status (e.g. 4xx) is treated as a real error. + defp transient_error?(%Client.Error{resp: %{status: status}}) when is_integer(status), + do: status in 500..599 + + defp transient_error?(%Client.Error{}), do: true + defp transient_error?(_), do: false + + defp retry_transient_errors? do + case System.get_env("RETRY_TRANSIENT_ERRORS") do + value when value in ["1", "true", "TRUE", "yes", "YES"] -> true + _ -> false + end + end + defp handle_up_to_date(state, start_ms) do oracle_rows = query_oracle(state) materialized = materialized_rows(state) From 7da830fe356058645b210ffe667cb0686f10993e Mon Sep 17 00:00:00 2001 From: rob Date: Thu, 2 Jul 2026 10:41:33 +0100 Subject: [PATCH 3/5] test(oracle): collapse RestartStrategy behaviour into a case dispatch Each strategy module was a one-line delegate to Support.ComponentSetup with no runtime polymorphism (selection is a static RESTART_TYPE string), so the behaviour + four modules were unnecessary indirection. Replace them with a private restart_stack/2 dispatch in oracle_harness.ex that pattern-matches the type and calls the ComponentSetup function directly; an unknown type raises. --- .../test/integration/oracle_property_test.exs | 1 - .../test/support/oracle_harness.ex | 34 ++++++++++++---- .../oracle_harness/restart_strategy.ex | 39 ------------------- .../oracle_harness/restart_strategy/brutal.ex | 12 ------ .../restart_strategy/graceful.ex | 11 ------ .../restart_strategy/rolling_deploy.ex | 15 ------- 6 files changed, 27 insertions(+), 85 deletions(-) delete mode 100644 packages/sync-service/test/support/oracle_harness/restart_strategy.ex delete mode 100644 packages/sync-service/test/support/oracle_harness/restart_strategy/brutal.ex delete mode 100644 packages/sync-service/test/support/oracle_harness/restart_strategy/graceful.ex delete mode 100644 packages/sync-service/test/support/oracle_harness/restart_strategy/rolling_deploy.ex diff --git a/packages/sync-service/test/integration/oracle_property_test.exs b/packages/sync-service/test/integration/oracle_property_test.exs index b8032e73b9..95ee2bd29f 100644 --- a/packages/sync-service/test/integration/oracle_property_test.exs +++ b/packages/sync-service/test/integration/oracle_property_test.exs @@ -20,7 +20,6 @@ defmodule Electric.Integration.OraclePropertyTest do "graceful" (default, clean shutdown + restore from disk), "brutal" (kill -9 style crash + recover), or "rolling" (rolling deploy: a new stack takes over the replication slot before the old one is stopped). - See `Support.OracleHarness.RestartStrategy`. - RETRY_TRANSIENT_ERRORS: When "true"/"1", a transient poll error (5xx or a connection error) is retried within the check timeout instead of failing the test. These are availability blips that production hides from clients diff --git a/packages/sync-service/test/support/oracle_harness.ex b/packages/sync-service/test/support/oracle_harness.ex index 2978809dc9..37103e054a 100644 --- a/packages/sync-service/test/support/oracle_harness.ex +++ b/packages/sync-service/test/support/oracle_harness.ex @@ -34,7 +34,6 @@ defmodule Support.OracleHarness do """ alias Support.OracleHarness.ShapeChecker - alias Support.OracleHarness.RestartStrategy @type shape :: %{ name: String.t(), @@ -79,7 +78,7 @@ defmodule Support.OracleHarness do exercise restore-from-disk (default: 0, disabled) - :restart_type - how the server restart is performed: "graceful" (default), "brutal" (crash + recover), or "rolling" (rolling deploy). See - `Support.OracleHarness.RestartStrategy`. Env: RESTART_TYPE. + `restart_stack/2`. Env: RESTART_TYPE. - :restart_client_every - throw away and recreate the shape clients every M batches to exercise fresh-poll consistency (default: 0, disabled) """ @@ -89,7 +88,7 @@ defmodule Support.OracleHarness do timeout_ms = opts[:timeout_ms] || env_int("CHECK_TIMEOUT") || @default_timeout_ms restart_server_every = opts[:restart_server_every] || 0 restart_client_every = opts[:restart_client_every] || 0 - restart_strategy = RestartStrategy.for_type(opts[:restart_type] || "graceful") + restart_type = opts[:restart_type] || "graceful" log_test_config(shapes, batches) @@ -136,7 +135,7 @@ defmodule Support.OracleHarness do oracle_pool, timeout_ms, batch_idx, - restart_strategy + restart_type ) restart_client? -> @@ -188,18 +187,39 @@ defmodule Support.OracleHarness do # Restarts the Electric stack (server-side restore-from-file test) and # reconnects the clients. Old checkers are stopped because their polls are # against the server that is about to go down. - defp restart_server(ctx, pids, shapes, oracle_pool, timeout_ms, batch_idx, strategy) do - log("Restarting server (#{inspect(strategy)}) after batch_#{batch_idx}") + defp restart_server(ctx, pids, shapes, oracle_pool, timeout_ms, batch_idx, restart_type) do + log("Restarting server (#{restart_type}) after batch_#{batch_idx}") Enum.each(pids, &GenServer.stop/1) - new_ctx = Map.merge(ctx, strategy.restart(ctx)) + new_ctx = Map.merge(ctx, restart_stack(restart_type, ctx)) new_pids = recreate_checkers(new_ctx, [], shapes, oracle_pool, timeout_ms, batch_idx) {new_pids, new_ctx} end + # Restarts the Electric stack according to RESTART_TYPE, returning a ctx-merge + # map. Each variant is implemented in `Support.ComponentSetup`: + # - "graceful" - clean stop + restore from disk (default) + # - "brutal" - kill -9 style crash, then recover the same stack_id + # - "rolling" - rolling deploy: a new stack takes over the replication slot + # before the old one is stopped + defp restart_stack("graceful", ctx), do: Support.ComponentSetup.restart_complete_stack(ctx) + + defp restart_stack("brutal", ctx), + do: Support.ComponentSetup.brutally_restart_complete_stack(ctx) + + defp restart_stack("rolling", ctx), + do: Support.ComponentSetup.rolling_restart_complete_stack(ctx) + + defp restart_stack(other, _ctx), + do: + raise( + ArgumentError, + "unknown RESTART_TYPE=#{inspect(other)} (expected \"graceful\", \"brutal\" or \"rolling\")" + ) + # Throws away the existing checkers and creates fresh ones (client-side # resync test). The new checkers do an initial snapshot poll and assert # consistency against the oracle. diff --git a/packages/sync-service/test/support/oracle_harness/restart_strategy.ex b/packages/sync-service/test/support/oracle_harness/restart_strategy.ex deleted file mode 100644 index 0b954d297b..0000000000 --- a/packages/sync-service/test/support/oracle_harness/restart_strategy.ex +++ /dev/null @@ -1,39 +0,0 @@ -defmodule Support.OracleHarness.RestartStrategy do - @moduledoc """ - Strategy for restarting the Electric stack part-way through an oracle test run. - - The oracle harness restarts the stack every `RESTART_SERVER_EVERY` batches to - exercise recovery. This behaviour abstracts *how* that restart happens so the - same test can cover several failure/deploy modes, selected with the - `RESTART_TYPE` env var: - - - `graceful` (default) — clean `stop_supervised` then restart the same - `stack_id`, restoring from disk. See `#{inspect(__MODULE__.Graceful)}`. - - `brutal` — simulate a crash (`Process.exit(pid, :kill)`, no terminate - callbacks) then recover the same `stack_id` from on-disk state. See - `#{inspect(__MODULE__.Brutal)}`. - - `rolling` — rolling deploy: a new stack boots and contends on the same - Postgres replication slot via the advisory lock, then the old stack is - gracefully stopped so the new one takes over. See - `#{inspect(__MODULE__.RollingDeploy)}`. - - `restart/1` receives the current test `ctx` and returns a map that is merged - back into `ctx`. At minimum it updates `:stack_supervisor`; the rolling - strategy also swaps in a new `:stack_id`, `:storage`, `:client`, etc. so the - harness reconnects its checkers to the new stack. - """ - - @callback restart(ctx :: map()) :: map() - - @spec for_type(String.t()) :: module() - def for_type("graceful"), do: __MODULE__.Graceful - def for_type("brutal"), do: __MODULE__.Brutal - def for_type("rolling"), do: __MODULE__.RollingDeploy - - def for_type(other), - do: - raise( - ArgumentError, - "unknown RESTART_TYPE=#{inspect(other)} (expected \"graceful\", \"brutal\" or \"rolling\")" - ) -end diff --git a/packages/sync-service/test/support/oracle_harness/restart_strategy/brutal.ex b/packages/sync-service/test/support/oracle_harness/restart_strategy/brutal.ex deleted file mode 100644 index e411956c58..0000000000 --- a/packages/sync-service/test/support/oracle_harness/restart_strategy/brutal.ex +++ /dev/null @@ -1,12 +0,0 @@ -defmodule Support.OracleHarness.RestartStrategy.Brutal do - @moduledoc """ - Brutal restart: kill the running stack outright (`Process.exit(pid, :kill)`, - no terminate callbacks, possibly mid-write) then bring a fresh stack back up - on the same `stack_id`/storage. Exercises crash recovery — stale advisory - lock, torn on-disk log, orphaned replication slot. - """ - @behaviour Support.OracleHarness.RestartStrategy - - @impl true - def restart(ctx), do: Support.ComponentSetup.brutally_restart_complete_stack(ctx) -end diff --git a/packages/sync-service/test/support/oracle_harness/restart_strategy/graceful.ex b/packages/sync-service/test/support/oracle_harness/restart_strategy/graceful.ex deleted file mode 100644 index ad4a6f3b29..0000000000 --- a/packages/sync-service/test/support/oracle_harness/restart_strategy/graceful.ex +++ /dev/null @@ -1,11 +0,0 @@ -defmodule Support.OracleHarness.RestartStrategy.Graceful do - @moduledoc """ - Graceful restart: cleanly stop the running `Electric.StackSupervisor` and - start a fresh one with the same `stack_id`, storage and slot, so the new - stack restores everything the old one persisted to disk. - """ - @behaviour Support.OracleHarness.RestartStrategy - - @impl true - def restart(ctx), do: Support.ComponentSetup.restart_complete_stack(ctx) -end diff --git a/packages/sync-service/test/support/oracle_harness/restart_strategy/rolling_deploy.ex b/packages/sync-service/test/support/oracle_harness/restart_strategy/rolling_deploy.ex deleted file mode 100644 index dbdb83b264..0000000000 --- a/packages/sync-service/test/support/oracle_harness/restart_strategy/rolling_deploy.ex +++ /dev/null @@ -1,15 +0,0 @@ -defmodule Support.OracleHarness.RestartStrategy.RollingDeploy do - @moduledoc """ - Rolling deploy: bring up a brand new stack (its own `stack_id`, storage and - HTTP server) that contends on the *same* Postgres replication slot as the - running stack via the advisory lock. The new stack boots to `read_only` - (blocked acquiring the lock the old stack holds), then the old stack is - gracefully stopped — releasing the lock and leaving the persistent slot and - publication in place — and the new stack takes over as the single active - writer. Faithful to how Electric-cloud performs zero-downtime deploys. - """ - @behaviour Support.OracleHarness.RestartStrategy - - @impl true - def restart(ctx), do: Support.ComponentSetup.rolling_restart_complete_stack(ctx) -end From f66c2d6748daf7217b45a67fd7101e43130a5abd Mon Sep 17 00:00:00 2001 From: rob Date: Thu, 2 Jul 2026 10:49:24 +0100 Subject: [PATCH 4/5] test(oracle): fail when a retried transient error never recovers With RETRY_TRANSIENT_ERRORS on, a window that timed out without a single successful poll previously returned stale state to assert_consistent!, which could pass silently if the oracle happened to be unchanged for that check. Thread a polled_ok? flag through do_await/3 and fail explicitly when the retry window completes no successful poll, so a genuinely-down server always fails rather than incidentally passing. Default behaviour (flag off) is unchanged. --- .../test/integration/oracle_property_test.exs | 6 ++-- .../support/oracle_harness/shape_checker.ex | 35 +++++++++++++------ 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/packages/sync-service/test/integration/oracle_property_test.exs b/packages/sync-service/test/integration/oracle_property_test.exs index 95ee2bd29f..9729f80bc9 100644 --- a/packages/sync-service/test/integration/oracle_property_test.exs +++ b/packages/sync-service/test/integration/oracle_property_test.exs @@ -24,8 +24,10 @@ defmodule Electric.Integration.OraclePropertyTest do connection error) is retried within the check timeout instead of failing the test. These are availability blips that production hides from clients during a restart/deploy, so they are not consistency signals. Default off - (any poll error fails, preserving current behaviour). 409/must-refetch and - 4xx errors and data mismatches always fail regardless of this flag. + (any poll error fails, preserving current behaviour). A server that never + recovers within the timeout still fails (a window with no successful poll + is treated as an error, not a pass). 409/must-refetch, 4xx errors and data + mismatches always fail regardless of this flag. - RESTART_CLIENT_EVERY: Throw away clients (poll cursors, materialized rows) and reconnect every M batches to test that fresh polls correctly assemble snapshot + log (default: 0, disabled). Independent of diff --git a/packages/sync-service/test/support/oracle_harness/shape_checker.ex b/packages/sync-service/test/support/oracle_harness/shape_checker.ex index d7749279f7..8f4f856349 100644 --- a/packages/sync-service/test/support/oracle_harness/shape_checker.ex +++ b/packages/sync-service/test/support/oracle_harness/shape_checker.ex @@ -46,7 +46,7 @@ defmodule Support.OracleHarness.ShapeChecker do oracle_before: nil, # When true, transient poll errors (5xx / connection errors) are retried # within timeout_ms instead of failing immediately. Opt-in via - # RETRY_TRANSIENT_ERRORS; see do_await/2. + # RETRY_TRANSIENT_ERRORS; see do_await/3. retry_transient_errors?: false ] @@ -156,14 +156,27 @@ defmodule Support.OracleHarness.ShapeChecker do # --------------------------------------------------------------------------- defp await_up_to_date(state) do - do_await(state, System.monotonic_time(:millisecond)) + # polled_ok? tracks whether at least one poll completed (vs only transient + # errors) in this window, so a persistently-down server can't pass silently. + do_await(state, System.monotonic_time(:millisecond), false) end - defp do_await(state, start_ms) do + defp do_await(state, start_ms, polled_ok?) do elapsed = System.monotonic_time(:millisecond) - start_ms if elapsed >= state.timeout_ms do - # Timed out — return state and let assert_consistent! report the mismatch + # When retrying transient errors, a window that timed out without a single + # successful poll means the server was unreachable the whole time. Returning + # stale state would let a genuinely-down server pass if the oracle happens to + # be unchanged for this check, so fail explicitly instead. Otherwise return + # state and let assert_consistent! report any data mismatch. + if state.retry_transient_errors? and not polled_ok? do + flunk( + "shape=#{state.name}: no successful poll within #{state.timeout_ms}ms " <> + "(server unreachable for the whole window)" + ) + end + state else case Client.poll(state.client, state.shape_def, state.poll_state, replica: :full) do @@ -174,7 +187,7 @@ defmodule Support.OracleHarness.ShapeChecker do if new_state.up_to_date? do handle_up_to_date(state, start_ms) else - do_await(state, start_ms) + do_await(state, start_ms, true) end {:must_refetch, messages, new_state} -> @@ -186,7 +199,7 @@ defmodule Support.OracleHarness.ShapeChecker do state = %{state | poll_state: new_state, rows: %{}} state = apply_messages(state, messages) - do_await(state, start_ms) + do_await(state, start_ms, true) {:error, error} -> # A 5xx or a connection error is an availability signal, never a @@ -194,12 +207,12 @@ defmodule Support.OracleHarness.ShapeChecker do # clients (HTTP drains before teardown; the LB switches after the new # instance is ready), so a transient error here does not reflect a # production bug. When opted in, retry within timeout_ms; a genuine - # persistent error still surfaces as a consistency failure once the - # window is exhausted. 409s are handled separately (:must_refetch) and - # 4xx errors are treated as real and still fail immediately. + # persistent error still fails via the no-successful-poll guard above + # (or, once some poll succeeded, as a consistency mismatch). 409s are + # handled separately (:must_refetch) and 4xx errors fail immediately. if state.retry_transient_errors? and transient_error?(error) do Process.sleep(@transient_retry_backoff_ms) - do_await(state, start_ms) + do_await(state, start_ms, polled_ok?) else flunk("Poll error for shape=#{state.name} where=#{state.where}: #{inspect(error)}") end @@ -230,7 +243,7 @@ defmodule Support.OracleHarness.ShapeChecker do state else # Electric reported up_to_date but data doesn't match yet - keep polling - do_await(state, start_ms) + do_await(state, start_ms, true) end end From e85698daf958edcf8aa8d37a186b80f7cf3fd51a Mon Sep 17 00:00:00 2001 From: rob Date: Thu, 2 Jul 2026 10:53:43 +0100 Subject: [PATCH 5/5] test(oracle): resolve stack_supervisor_id from ctx in all restart strategies graceful and brutal hardcoded the Electric.StackSupervisor child id while rolling read Map.get(ctx, :stack_supervisor_id, Electric.StackSupervisor). Make graceful/brutal use the same pattern (and preserve the id in their return merge) so the helpers stay correct if strategies are ever composed. No change in the common single-strategy case, where the id defaults to Electric.StackSupervisor. --- .../sync-service/test/support/component_setup.ex | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/sync-service/test/support/component_setup.ex b/packages/sync-service/test/support/component_setup.ex index 768329b9db..38ab0ce917 100644 --- a/packages/sync-service/test/support/component_setup.ex +++ b/packages/sync-service/test/support/component_setup.ex @@ -463,7 +463,8 @@ defmodule Support.ComponentSetup do (stack_id, persistent_kv, storage, registry, etc.) are unchanged. """ def restart_complete_stack(ctx) do - :ok = stop_supervised(Electric.StackSupervisor) + sup_id = Map.get(ctx, :stack_supervisor_id, Electric.StackSupervisor) + :ok = stop_supervised(sup_id) stack_supervisor = start_stack_supervisor!( @@ -472,7 +473,8 @@ defmodule Support.ComponentSetup do ctx.persistent_kv, ctx.storage, ctx.stack_events_registry, - ctx.publication_name + ctx.publication_name, + id: sup_id ) # The :stack_status :ready event fires before the connection pools and @@ -481,7 +483,7 @@ defmodule Support.ComponentSetup do # :active level which requires all readiness conditions to be met. :ok = Electric.StatusMonitor.wait_until_active(ctx.stack_id, timeout: 5000) - %{stack_supervisor: stack_supervisor} + %{stack_supervisor: stack_supervisor, stack_supervisor_id: sup_id} end @doc """ @@ -495,6 +497,7 @@ defmodule Support.ComponentSetup do Returns a map with the updated `:stack_supervisor` pid. """ def brutally_restart_complete_stack(ctx) do + sup_id = Map.get(ctx, :stack_supervisor_id, Electric.StackSupervisor) pid = ctx.stack_supervisor ref = Process.monitor(pid) Process.exit(pid, :kill) @@ -507,7 +510,7 @@ defmodule Support.ComponentSetup do # The child was `restart: :temporary`, so ExUnit does not restart it; its # spec lingers with an :undefined pid. Remove it so the id is free to reuse. - case stop_supervised(Electric.StackSupervisor) do + case stop_supervised(sup_id) do :ok -> :ok {:error, :not_found} -> :ok end @@ -525,14 +528,15 @@ defmodule Support.ComponentSetup do ctx.persistent_kv, ctx.storage, ctx.stack_events_registry, - ctx.publication_name + ctx.publication_name, + id: sup_id ) # Generous timeout: worst case the stale advisory lock is only cleared on # the periodic lock-breaker cycle (~10s), not immediately on kill. :ok = Electric.StatusMonitor.wait_until_active(ctx.stack_id, timeout: 15_000) - %{stack_supervisor: stack_supervisor} + %{stack_supervisor: stack_supervisor, stack_supervisor_id: sup_id} end @doc """