Context
Malachi.Cluster.Scrubber runs its whole pass inside its own GenServer loop: run/1 is called from handle_info(:tick, state) and from handle_call(:scrub_now, ...). Inside that pass, intact_peer/2 asks each peer to verify its copy through verify_segment/3, which is a GenServer.call to the peer's scrubber with a 30s timeout (@peer_timeout, lib/malachi/cluster/scrubber.ex:75).
A scrubber that is running a pass therefore cannot answer another scrubber's verify_segment until its own pass finishes. When node A and node B both find damage in the same window, A blocks calling B while B blocks calling A: a circular wait, resolved only by both 30s timeouts expiring. Both then conclude :no_intact_copy and leave the damage in place until the next cycle, where the same collision can repeat.
Two details make it worse than a one-off stall:
- The pass is on a timer with the same interval on every node, so the collision is not random. Nodes that boot together tick together.
- The scan itself is the expensive part (24 to 86ms per 64MB segment warm, more from cold storage), so even without a collision, a peer that is mid-scan delays every caller behind it.
This is the same shape Malachi.Cluster.ReplicationServer already removed from the replication path: the fix there was to stop doing blocking cross-node work inside the loop that also has to answer cross-node calls.
Plan
Options, in rough order of preference:
A. Answer verify_segment outside the loop. Have the scrubber spawn the scan (a Task, replying with GenServer.reply/2 from the task) so serving a peer never waits on this node's own pass. Directly breaks the cycle: a busy scrubber is still reachable. The scan is read-only and takes no state, so it is safe outside the loop, and the state update it needs (the damaged set) is only touched by this node's own pass.
B. Run this node's pass outside the loop too. Same treatment for run/1, keeping the GenServer as a small coordinator. Cleaner in the long run, but a larger change: scrub_now/1 is synchronous by contract and the tests depend on that.
C. Jitter the tick. Cheap, and reduces the probability of a collision without removing it. Worth doing regardless of A or B, so passes across a cluster do not stay in lockstep, but not a fix on its own.
D. Do nothing. The failure is bounded (a delayed repair, never data loss, since the safety rule keeps the local copy) and needs simultaneous damage on two nodes. Defensible if that combination is judged rare enough, but a scrub that stalls exactly when two copies are bad is stalled at the worst possible moment.
Recommendation to weigh: A plus C. A removes the cycle, C stops the passes lining up in the first place.
Risks and open questions
- Replying from a task means the scrubber must not assume its state is unchanged when the reply lands. The verify path is read-only today, so this holds, but it needs to stay true.
- An unbounded number of concurrent peer scans would let a cluster-wide repair storm turn into a disk-I/O storm. The task path probably needs its own concurrency cap.
@peer_timeout at 30s exists because the scan is genuinely slow; once serving a peer is off the loop, the right value may be smaller.
Verification
- A test with two scrubbers, each pointed at the other, both mid-pass, asserting both complete their repair rather than both reporting
:no_intact_copy after a timeout. The test must fail against the current code.
- Full suite,
mix credo --strict, mix dialyzer, mix docs --warnings-as-errors, single-node and 3-node loadtests with zero errors, plus a run of scripts/docker-storage-chaos.sh, which is where a real two-node repair is exercised.
Found during the CodeRabbit review of PR #44.
Context
Malachi.Cluster.Scrubberruns its whole pass inside its own GenServer loop:run/1is called fromhandle_info(:tick, state)and fromhandle_call(:scrub_now, ...). Inside that pass,intact_peer/2asks each peer to verify its copy throughverify_segment/3, which is aGenServer.callto the peer's scrubber with a 30s timeout (@peer_timeout,lib/malachi/cluster/scrubber.ex:75).A scrubber that is running a pass therefore cannot answer another scrubber's
verify_segmentuntil its own pass finishes. When node A and node B both find damage in the same window, A blocks calling B while B blocks calling A: a circular wait, resolved only by both 30s timeouts expiring. Both then conclude:no_intact_copyand leave the damage in place until the next cycle, where the same collision can repeat.Two details make it worse than a one-off stall:
This is the same shape
Malachi.Cluster.ReplicationServeralready removed from the replication path: the fix there was to stop doing blocking cross-node work inside the loop that also has to answer cross-node calls.Plan
Options, in rough order of preference:
A. Answer
verify_segmentoutside the loop. Have the scrubber spawn the scan (aTask, replying withGenServer.reply/2from the task) so serving a peer never waits on this node's own pass. Directly breaks the cycle: a busy scrubber is still reachable. The scan is read-only and takes no state, so it is safe outside the loop, and the state update it needs (the damaged set) is only touched by this node's own pass.B. Run this node's pass outside the loop too. Same treatment for
run/1, keeping the GenServer as a small coordinator. Cleaner in the long run, but a larger change:scrub_now/1is synchronous by contract and the tests depend on that.C. Jitter the tick. Cheap, and reduces the probability of a collision without removing it. Worth doing regardless of A or B, so passes across a cluster do not stay in lockstep, but not a fix on its own.
D. Do nothing. The failure is bounded (a delayed repair, never data loss, since the safety rule keeps the local copy) and needs simultaneous damage on two nodes. Defensible if that combination is judged rare enough, but a scrub that stalls exactly when two copies are bad is stalled at the worst possible moment.
Recommendation to weigh: A plus C. A removes the cycle, C stops the passes lining up in the first place.
Risks and open questions
@peer_timeoutat 30s exists because the scan is genuinely slow; once serving a peer is off the loop, the right value may be smaller.Verification
:no_intact_copyafter a timeout. The test must fail against the current code.mix credo --strict,mix dialyzer,mix docs --warnings-as-errors, single-node and 3-node loadtests with zero errors, plus a run ofscripts/docker-storage-chaos.sh, which is where a real two-node repair is exercised.Found during the CodeRabbit review of PR #44.