Merge pull request #325 from amazon-mq/jepsen-member-churn-nemesis

Jepsen: Add member-churn nemesis for the manifest-replica leak

Author: lukebakken

jepsen/BACKLOG.md

@@ -6,16 +6,32 @@ Follow-up work for the `rabbitmq_stream_s3` Jepsen harness, in rough priority or
 
 A `force-trim` nemesis that drops a stream's local retention past the upload seam `n`, paired with a checker scoped to the `[f, n)` range, so loss is only flagged inside the durable range the plugin actually promises (see `../docs/invariants.md`, "Non-guarantee"). The current durability checker assumes retention is wide enough that nothing legitimately drops, which is why retention is configured generously today.
 
-## Hard-kill / stream-churn nemesis to reproduce the manifest-replica leak
+## member-churn nemesis to reproduce the manifest-replica leak
 
-The `:replica` manifest-replica consistency checker's `:leaked-replica-row` assertion (a cache row on a non-leader node with no registered replica context) only *fires* when an osiris member departs a node permanently mid-sync: a sync arriving after the member's `DOWN` re-creates a cache row with no monitor to reclaim it. Graceful `leader-move` (which transfers leadership, leaving the member set intact) does not cause this, so the assertion ships as a cheap always-on guard rather than something the current faults reproduce.
+The `:replica` manifest-replica consistency checker's `:leaked-replica-row` assertion (a cache row on a non-leader node with no registered replica context) only *fires* when an osiris member departs a node **permanently** mid-sync: a sync arriving after the member's `DOWN` re-creates a cache row with no monitor to reclaim it.
 
-Reliably exercising it needs a nemesis that makes members genuinely leave a node mid-sync, for example:
+`leader-move` does not reproduce this, but not for the reason first assumed. `transfer_leadership` calls `rabbit_stream_coordinator:restart_stream/2`, which sets `target = stopped` on *every* member (writer and all replicas) and restarts them — so the osiris members genuinely go `DOWN`, and the DOWN-then-racing-sync window is exercised (this is what the `syncs_dropped_no_context` counter records under `leader-move`). What `leader-move` leaves intact is the **node set**: every member restarts *in place* on the same node, so each departed member re-registers a context that both drops the racing sync (post-fix) and would reclaim an orphan row (even pre-fix). Nothing stays orphaned at quiesce.
 
-- a hard kill (SIGKILL) of a broker while uploads/syncs are in flight, then a restart that relocates the member, or
-- stream churn (delete and re-create streams, or add/remove replicas) under the S3-outage fault so syncs race the member teardown
+Reproducing a *persistent* leak therefore needs a member to leave a node and not come back. Since a stream spans all cluster nodes by default (`initial_cluster_size(undefined) -> length(rabbit_nodes:list_members())`, five members in the five-node cluster), `delete_replica` is the clean way to do this: it permanently removes a member from a node, so a sync racing the teardown has no re-registration to reclaim it.
 
-Until then the convergence and stale-floor assertions carry the meaningful verdict for this scenario.
+Implemented as the **member-churn** nemesis (`nemesis.clj`, `db.clj`): each tick it `delete_replica`s a random replica from a random stream, bounded to keep every stream at leader + >=1 replica so durability holds for the final read. It is delete-only by design — re-adding the member would re-register a context that reclaims (masks) an orphan before the checker's end-of-run snapshot. Pair it with `s3-outage`/`s3-latency` to widen the window in which a sync races the teardown:
+
+```sh
+FAULTS=member-churn,s3-latency ./run.sh
+```
+
+Validated green on a fresh five-node cluster (`member-churn,s3-latency`, 150s): overall `:valid? true`, `:replica` with `:diverged`, `:stale-floors` and `:leaked-rows` all empty, `:durability` clean, and `syncs-dropped-no-context` at 395 — so `delete_replica` genuinely drove the sync-context guard end to end, not merely present.
+
+One implementation note the run surfaced: `delete_replica` restarts the stream's writer, so it drifts the client's send offsets exactly like `leader-move` (the coordinator sets `target = stopped` on every running member, not just the removed one — see `rabbit_stream_coordinator:update_stream0/3`). The kafka workload checker therefore has to be downgraded to advisory under `member-churn` too, with durability carrying the safety verdict (`core.clj`). Also mind cluster reuse: because the nemesis is delete-only, replicas do not come back, so re-running against the same cluster churns already-thinned streams — run it on a fresh cluster, as CI does.
+
+Not in CI until the coordinator writer-liveness fix lands. Under `delete_replica` churn a stream can be left leaderless — every member stranded in `current = {starting, _}` with no running writer — and never re-elect. This is the stream coordinator writer-liveness bug [#16822](https://github.com/rabbitmq/rabbitmq-server/issues/16822), fixed by [#16881](https://github.com/rabbitmq/rabbitmq-server/pull/16881), which adds a `maybe_reelect_leader` backstop in `evaluate_stream` and clears stale `starting` actions on epoch transition, gated at coordinator machine version 8. Our broker is still at machine version 7 (`rabbit_stream_coordinator:version/0`), so it carries the bug and member-churn can intermittently wedge the cluster (a writer-less stream hangs its consumers, which hangs the poll). Bounded graceful churn usually self-heals in seconds, so short runs mostly pass, but the hazard is real. Keep `member-churn` out of the `jepsen.yaml` CI matrix until the fix is in the broker we build against (machine version 8); at that point member-churn doubles as a regression test for #16822.
+
+Two things still open here:
+
+- The assertion can only be made to positively *fire* (proving it has teeth) against a build with the fix removed — a negative control — since with the fix in place even the departing-member race is handled and the row is never created.
+- A hard kill (SIGKILL) of a broker that then relocates the member is a stronger, more abrupt variant still worth adding; `delete_replica` is a graceful, coordinator-driven departure.
+
+Observation surfaced while designing this: post-delete, a sync that races the teardown records a `pending_resync[stream] = writer` entry on the departed node that is never consumed (no context ever re-registers there without a re-add). Post-fix this is a small, permanent map entry rather than a leaked ETS row — harmless for correctness, but unbounded under heavy churn; worth having the delete/forget path clear it.
 
 ## Regression check for the manifest-replica fix (validated)
 

jepsen/README.md

@@ -58,11 +58,11 @@ FAULTS=leader-move,trim TIME_LIMIT=300 ./run.sh
 
 The partition nemesis runs against a 5-node cluster with the kafka anomaly checkers and retention wide. A run under partition faults reports `:valid? true`, with fault-induced failures correctly classified as indeterminate rather than lost or duplicate, and broker logs collected back to the control node.
 
-The storage-tier nemeses make data really tier to S3 and exercise the read-from-S3 path under fault. Small segments and fragments plus padded payloads force segments to roll, upload and trim within a short run; MinIO needs a KMS key for the plugin's SSE uploads (see `docker/docker-compose.yml`). The `--faults` option selects `s3-outage` and `s3-latency` (via Toxiproxy), `trim` (periodic `evaluate_local_retention`, which trims uploaded segments locally so reads of older offsets fall to the S3 tier), and `leader-move` (each tick, transfer a random stream's leader to a replica, relocating that writer and bumping the stream epoch so a deposed writer's in-flight upload must be fenced).
+The storage-tier nemeses make data really tier to S3 and exercise the read-from-S3 path under fault. Small segments and fragments plus padded payloads force segments to roll, upload and trim within a short run; MinIO needs a KMS key for the plugin's SSE uploads (see `docker/docker-compose.yml`). The `--faults` option selects `s3-outage` and `s3-latency` (via Toxiproxy), `trim` (periodic `evaluate_local_retention`, which trims uploaded segments locally so reads of older offsets fall to the S3 tier), `leader-move` (each tick, transfer a random stream's leader to a replica, relocating that writer and bumping the stream epoch so a deposed writer's in-flight upload must be fenced), and `member-churn` (each tick, `delete_replica` a random replica from a random stream so an osiris member departs its node for good mid-sync; bounded to keep every stream at leader + at least one replica).
 
 Three extra checkers run alongside the kafka anomaly checkers. The `:tiering` coverage checker scrapes the plugin's S3 counters before teardown and fails the run unless fragments were uploaded, and with `trim` that reads were served from S3, and with `leader-move` that the epoch advanced; this keeps a run from passing green while silently not using the remote tier or not moving leaders. The `s3-outage` fault is instead proven injected by the Toxiproxy status check in the nemesis (it fails on a non-2xx response), since the plugin tolerates an outage gracefully and need not surface an error counter. The `:durability` checker reads every stream end-to-end after the run and fails if any acknowledged write is lost or duplicated; it is authoritative under `leader-move`, where the kafka offset analyzers are downgraded to advisory (see "Why the Kafka workload").
 
-The `:replica` manifest-replica consistency checker snapshots each node's per-stream manifest cache (the table `rabbitmq_stream_s3_manifest_cache`) after the run quiesces and asserts three things: every stream's cached floor is identical across the nodes that cache it (convergence); the cross-node-agreed floor never sits beyond the stream's committed offset (a stale or corrupt floor would claim data the remote tier does not hold); and no cache row on a non-leader node lacks a registered replica context (a contextless replica row is one a sync re-created after the owning osiris member's `DOWN`, with no monitor to reclaim it). The cached epoch is reported alongside each floor for diagnosis but is deliberately left out of the convergence equality: a deposed leader's cache legitimately keeps the older epoch on an idle stream until the next edit, and the plugin's GC explicitly tolerates a cache that lags the committed epoch (`get_manifest_and_epoch/1`), so an epoch lag is not a convergence violation. The leader's own row is written by the writer path with no replica context, so the leader node is excluded from the last test to avoid a false positive. The convergence and stale-floor assertions are genuinely exercised by the `s3-outage`, `leader-move` and `partition` faults; the leaked-row assertion is a cheap always-on guard that only *fires* when a member departs a node permanently mid-sync, which graceful `leader-move` does not cause — reliably reproducing the leak needs a hard-kill / stream-churn nemesis (see `BACKLOG.md`).
+The `:replica` manifest-replica consistency checker snapshots each node's per-stream manifest cache (the table `rabbitmq_stream_s3_manifest_cache`) after the run quiesces and asserts three things: every stream's cached floor is identical across the nodes that cache it (convergence); the cross-node-agreed floor never sits beyond the stream's committed offset (a stale or corrupt floor would claim data the remote tier does not hold); and no cache row on a non-leader node lacks a registered replica context (a contextless replica row is one a sync re-created after the owning osiris member's `DOWN`, with no monitor to reclaim it). The cached epoch is reported alongside each floor for diagnosis but is deliberately left out of the convergence equality: a deposed leader's cache legitimately keeps the older epoch on an idle stream until the next edit, and the plugin's GC explicitly tolerates a cache that lags the committed epoch (`get_manifest_and_epoch/1`), so an epoch lag is not a convergence violation. The leader's own row is written by the writer path with no replica context, so the leader node is excluded from the last test to avoid a false positive. The convergence and stale-floor assertions are genuinely exercised by the `s3-outage`, `leader-move` and `partition` faults; the leaked-row assertion only *fires* when a member departs a node permanently mid-sync. `leader-move` does not cause that — it restarts every member in place on the same node set, so each departed member re-registers a context that reclaims any orphan (the DOWN-then-racing-sync window it does exercise is what the `syncs_dropped_no_context` counter records). The `member-churn` fault targets the leaked-row assertion directly: it `delete_replica`s a member so it leaves its node for good, leaving a sync-raced orphan visible at the end-of-run snapshot (see `BACKLOG.md`; note the assertion can only be made to positively fire against a build with the fix removed).
 
 A run under `s3-outage,leader-move` drives manifest churn (uploads stall and resume while leaders relocate and epochs bump) and stays `:valid? true` with the caches converged:
 
@@ -82,6 +82,12 @@ A run under `leader-move,trim` stays `:valid? true` with the durability checker
 FAULTS=leader-move,trim TIME_LIMIT=150 RATE=50 CONCURRENCY=10 ./run.sh
 ```
 
+A run under `member-churn,s3-latency` permanently removes replicas while syncs are delayed, exercising the manifest-replica leaked-row assertion (a member leaves its node for good while a sync races its teardown) and staying `:valid? true` with the caches converged:
+
+```sh
+FAULTS=member-churn,s3-latency TIME_LIMIT=150 RATE=50 CONCURRENCY=10 ./run.sh
+```
+
 GitHub Actions runs these scenarios with the `Jepsen` workflow (`.github/workflows/jepsen.yaml`), on a schedule and on demand. It clones the server, checks the plugin out into the umbrella, and invokes `ci/run-jepsen.sh`, which composes the same `up.sh`, `run.sh` and `down.sh`, building the tarball from the clean tree and failing the job if the checker reports anomalies.
 
 ## Planned

jepsen/jepsen.streams3/src/jepsen/streams3/core.clj

@@ -58,12 +58,18 @@
                     {:perf     (checker/perf)
                      :timeline (timeline/html)
                      ;; The kafka workload checker is authoritative on its own,
-                     ;; except under leader-move: there our send offsets drift
-                     ;; (see client.clj), so its offset-consistency analyzers go
-                     ;; red on artifacts. We downgrade it to advisory in that case
-                     ;; and let the durability checker carry the safety verdict.
+                     ;; except when a fault restarts the stream's writer: producer
+                     ;; recovery then drifts our send offsets (see client.clj), so
+                     ;; its offset-consistency analyzers go red on artifacts. Both
+                     ;; leader-move and member-churn do this — the latter because
+                     ;; the coordinator's delete_replica stops and restarts every
+                     ;; member, not just the removed one. We downgrade the checker
+                     ;; to advisory in those cases and let the durability checker
+                     ;; carry the safety verdict.
                      :workload (s3-checker/downgrade-when
-                                 (fn [t] (contains? (nem/faults t) "leader-move"))
+                                 (fn [t] (let [fs (nem/faults t)]
+                                           (or (contains? fs "leader-move")
+                                               (contains? fs "member-churn"))))
                                  (:checker wl))
                      ;; Authoritative no-loss / no-duplicate via an end-to-end
                      ;; read, sound even when send offsets are unreliable.

jepsen/jepsen.streams3/src/jepsen/streams3/db.clj

@@ -202,6 +202,48 @@
   [node]
   (util/meh (rabbitmqctl-eval node leader-move-eval)))
 
+;; One rabbitmqctl eval that permanently removes ONE randomly chosen replica from
+;; a randomly chosen jepsen-<k> stream, so that osiris member departs its node and
+;; does NOT return. This is the condition the manifest-replica sync-context guard
+;; protects against: a sync that races the member's teardown finds no reader
+;; context, and (without the guard) would re-create a cache row with no monitor to
+;; ever reclaim it. leader-move cannot exercise this because it restarts every
+;; member in place on the same node set, so each departed member re-registers a
+;; context that reclaims the row; only a member that leaves a node for good leaves
+;; an orphan visible at quiesce.
+;;
+;; Delete-only, and deliberately never re-added: add_replica would re-register a
+;; context on the node and reclaim (mask) any orphaned row before the replica
+;; checker's end-of-run snapshot, defeating the leaked-replica-row assertion. To
+;; keep every stream durable for the final read, a stream is a candidate only
+;; while it still has two or more replicas, so a delete always leaves the leader
+;; plus at least one replica. The coordinator is cluster-global, so this runs on a
+;; single node.
+(def ^:private member-churn-eval
+  (str "Names = [element(4, amqqueue:get_name(Q)) "
+       "|| Q <- rabbit_amqqueue:list(<<\"/\">>)], "
+       "Jeps = [N || N <- Names, byte_size(N) >= 7, binary:part(N, 0, 7) =:= <<\"jepsen-\">>], "
+       "Lookup = fun(N) -> element(2, rabbit_amqqueue:lookup(rabbit_misc:r(<<\"/\">>, queue, N))) end, "
+       "Reps = fun(N) -> maps:get(replica_nodes, amqqueue:get_type_state(Lookup(N)), []) end, "
+       "Cand = [N || N <- Jeps, length(Reps(N)) >= 2], "
+       "case Cand of "
+       "[] -> ok; "
+       "_ -> "
+       "N = lists:nth(rand:uniform(length(Cand)), Cand), "
+       "Rs = Reps(N), "
+       "R = lists:nth(rand:uniform(length(Rs)), Rs), "
+       "catch rabbit_stream_queue:delete_replica(<<\"/\">>, N, R) "
+       "end, ok."))
+
+(defn force-member-churn!
+  "Permanently removes one replica from a randomly chosen jepsen stream, so an
+  osiris member departs its node and does not return — the departing-member race
+  the manifest-replica sync-context guard protects against. Bounded to keep every
+  stream at leader + >=1 replica so durability holds for the final read. Run on a
+  single node (the coordinator is cluster-global). Runs in an SSH context."
+  [node]
+  (util/meh (rabbitmqctl-eval node member-churn-eval)))
+
 ;; The plugin's read/write paths are instrumented, not logged, so these
 ;; counters are how the test proves S3 was actually exercised (see the
 ;; coverage checker). The management/Prometheus listener is on 15692.