Return no_process from init_reader when the member is gone - #236
Open
lukebakken wants to merge 1 commit into
Open
Return no_process from init_reader when the member is gone#236lukebakken wants to merge 1 commit into
no_process from init_reader when the member is gone#236lukebakken wants to merge 1 commit into
Conversation
osiris:init_reader/4 and resolve_offset_spec/3 call
osiris_util:get_reader_context/1, which does a bare gen:call to the member
process. When that process has gone (for example a retention resync
restarted it in the same instant a consumer subscribes), gen:call exits
with noproc. Neither function catches it, so the exit escapes and crashes
the caller: rabbit_stream_reader terminates with a noisy [error] crash
report and its connection supervisor hits reached_max_restart_intensity,
even though the condition is a benign race the client recovers from by
retrying.
Wrap the get_reader_context/1 call in both functions and return
{error, no_process} on a noproc exit, matching how osiris_writer:overview/1
and init_data_reader/3 already handle the same condition and the shape
rabbit_stream_reader:init_reader/6 already turns into a STREAM_NOT_AVAILABLE
response. Add {error, no_process} to both specs.
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This PR was prepared by Claude (Anthropic's Claude Code) under the direction of @lukebakken, who reviewed the change before opening it. The fix was surfaced by a long-running high-throughput test. The code and analysis are AI-drafted and human-reviewed.
Problem
A 2-hour high-throughput test crashed a
rabbit_stream_readerconnection withexit:noprocraised fromosiris:init_reader/4when a consumer subscribed to a stream whose local member process had just gone (a retention resync restarted it in the same instant).osiris:init_reader/4andresolve_offset_spec/3callosiris_util:get_reader_context/1, which does a baregen:call/4to the member process. When that process is not alive,gen:callexits withnoproc. Neither function catches it, so the exit escapes:rabbit_stream_reader:init_reader/6callsosiris:init_reader/4inside acasethat handles{ok, _}and{error, _}but not an exit, so the connection process crashes with a[error]gen_statemcrash report, and itsrabbit_stream_connection_supthen hitsreached_max_restart_intensity. The condition is a benign race (the client reconnects and the run completes with no message loss), but it is loud in the logs and indistinguishable from a real fault.This is inconsistent with how the same condition is handled elsewhere in osiris:
osiris_writer:overview/1andinit_data_reader/3guardget_reader_context/1withis_process_alive/1and return{error, no_process}, whichosiris_replica_reader:init/1handles as a clean stop. The two member-facing reader entry points inosiris.erlare the ones that neither guard the call nor catch the exit.init_reader/4's own-specpromises{ok, _} | {error, _}, so the leaked exit also violates the declared contract.Full analysis in #235.
Solution
Wrap the
get_reader_context/1call in bothinit_reader/4andresolve_offset_spec/3and return{error, no_process}on anoprocexit, via a small sharedreader_context/1helper. This matches howosiris_writeralready reports the same condition, and{error, no_process}is a shaperabbit_stream_reader:init_reader/6already handles: it turns it into aSTREAM_NOT_AVAILABLEresponse the client retries.{error, no_process}is added to both specs.I catch both
exit:noproc(agen:callto a dead local pid) andexit:{noproc, _}(the tagged form agen:callinto a terminating gen_server can raise). Thetrycovers only theget_reader_context/1call, so errors from the downstreaminit_offset_reader/resolve_offset_specare not masked.get_reader_context/1and its other callers (get_stats/1, the twoosiris_writersites) are left unchanged: they either guard the call already or need the raw context, so narrowing the fix to the two unguarded reader entries avoids changing their behaviour.This is the same class of benign, race-driven crash as #230 (fixed by #231) and #233 (fixed by #234): one code path handles the condition gracefully and a sibling path leaks an uncaught exception under retention churn. This fix is independent of those and was verified on a run with the #231 and #234 fixes both applied.
On testing
I have not added a test. The crash is an inherent race: to raise the real
noprocthe member pid must die in the window between the subscribe and thegen:call, so it is not reproducible on demand without mocking. The sibling guarded callers (osiris_writer:overview/1,init_data_reader/3) ship without dedicated tests for this condition, and #234 took the same approach for the analogous retention race. Happy to add a mock-based test if you would prefer one.Closes #235