replica reader: Handle nosuspend/noconnect failures - #225
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
|
I found this through some Jepsen testing following what we test in |
lukebakken
left a comment
lukebakken
left a comment
There was a problem hiding this comment.
High-effort review of the nosuspend/noconnect handling. The changes correctly stop the badmatch crash on a non-ok send return, but the root gap is that noconnect and nosuspend are collapsed into a single failure branch with no termination bound: neither has a retry cap, backoff, or exit path, and replica_pid is never monitored. Inline comments below, ranked by severity (three confirmed correctness issues, then a lost-escalation concern, an unthrottled-logging concern, and a comment/grammar nit).
| ok = erlang:send(RPid, {'$gen_cast', {committed_offset, Msg}}, | ||
| [noconnect, nosuspend]), | ||
| State#state{committed_offset = COffs}; | ||
| case erlang:send(RPid, {'$gen_cast', {committed_offset, Msg}}, |
There was a problem hiding this comment.
Unbounded retry with no backoff or escalation. The retry reuses [noconnect, nosuspend] and the failure branch never exits, backs off, or caps the attempts, so both nosuspend and noconnect results retry every 50ms indefinitely.
Scenario: the replica node is up but its inbound distribution buffer stays saturated under sustained backpressure, so erlang:send/3 keeps returning nosuspend. There is no retry counter, no deadline, and replica_pid is not monitored, so nothing bounds the loop — the reader wakes ~20x/sec (recomputing committed_chunk_id each time) and committed_offset never advances on that replica for the whole window.
Consider a capped/exponential backoff, and/or distinguishing the terminal noconnect case from the transient nosuspend case.
| ok -> | ||
| cancel_retry_timer(State#state{committed_offset = COffs}); | ||
| Reason -> | ||
| %% Cast couldn't be delivered. We will try again soon if |
There was a problem hiding this comment.
Comment contradicts the code. This says the reader will "exit if the replica pid is down from 'noconnect'", but a noconnect return lands in this same Reason -> branch, which only logs and calls set_retry_timer/1 — it never returns {stop, ...}. The reader monitors only leader_pid (monitor(process, LeaderPid) in init/1), never replica_pid.
On a real replica-node partition the reader therefore keeps retrying every 50ms rather than exiting; termination depends entirely on the link EXIT that osiris_replica establishes (link(RRPid)), which is delivered only once net_ticktime (up to ~60s) detects the node down.
| [noconnect, nosuspend]) of | ||
| ok -> | ||
| cancel_retry_timer(State#state{committed_offset = COffs}); | ||
| Reason -> |
There was a problem hiding this comment.
Lost escalation path (design tradeoff worth confirming). The old ok = erlang:send(...) badmatch crashed the reader on a non-ok return, which cascaded via the link to a replica restart (osiris_replica stops with {stop, {shutdown, Info}, State} on the reader EXIT). This branch replaces that with an indefinite retry, so a replica that is alive but persistently unable to receive the cast now stalls silently — only ?DEBUG_ logging, no ?ERROR_, no metric, no cap.
This is the right call for the transient nosuspend the PR targets (a reader restart wouldn't reset the shared inter-node dist buffer anyway). But the complete loss of any escalation plus the observability drop is worth a bounded retry or a metric so a truly persistent stall is visible.
| %% there is a 'osiris_offset' or 'more_data' message, exit | ||
| %% if the replica pid is down from 'noconnect', or retry | ||
| %% on a timer (as a backstop). | ||
| ?DEBUG_(Name, "failed to send committed_offset to ~w, " |
There was a problem hiding this comment.
Unthrottled DEBUG logging. This logs on every failed send, and the same failure recurs on the 50ms timer and on each osiris_offset / more_data. During a prolonged backpressure or disconnect window each reader emits this line ~20x/sec; across many streams/replicas that floods debug logs with no rate limiting — precisely when the node may already be overloaded. Consider logging once per failure episode (e.g. only when first arming the timer) rather than on every attempt.
|
For the rest of the review notes: I think they overestimate the chances of a I think the 🧞 is nitpicking a bit 😄 |
`erlang:send/3` can return `ok | noconnect | nosuspend` depending on the
options passed to it. In a network partition, a failed
`{committed_offset, _}` message could return `noconnect` and crash the
replica reader, even if the `{'EXIT',...}` message is already in the
mailbox.
`nosuspend` is trickier to handle: if it's followed by a period of
idleness then we need to retry the send in order to update the writer.
This change covers both with a retry timer on a short fuse.
e7c539f to
413635d
Compare
2 participants
erlang:send/3can returnok | noconnect | nosuspenddepending on the options passed to it. In a network partition, a failed{committed_offset, _}message could returnnoconnectand crash the replica reader, even if the{'EXIT',...}message is already in the mailbox.nosuspendis trickier to handle: if it's followed by a period of idleness then we need to retry the send in order to update the writer. This change covers both with a retry timer on a short fuse.