Retry data reader init when retention deletes an index file

[lukebakken]

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 osiris_replica_reader with {bad_return_value,missing_file} when the index file the reader needs is deleted by retention in the window between the reader being asked to start and osiris_log:init_data_reader/2 opening that file.

init_data_reader/2 (via init_data_reader_from/3 -> offset_idx_scan/3) opens the index file through the local open/2 helper, which throws missing_file on enoent. Nothing on the data-reader path catches it, so the throw escapes init/1 and gen_server turns a callback that neither returns a valid tuple nor exits cleanly into {bad_return_value,missing_file}. The reader is temporary, so the stream coordinator restarts it and the cluster recovers with no message loss, but every occurrence logs a full crasher report at [error], indistinguishable from a real fault to log-based monitoring.

missing_file is already treated as an expected, retention-driven condition everywhere else it can occur: init_offset_reader/3 and resolve_offset_spec/3 both wrap their work in try ... catch missing_file -> retry (with the comment "Retention policies are likely being applied, let's try again"), and osiris_replica:init/1 handles a missing_file return from the writer as a clean stop. Only the data-reader path was unguarded.

Full analysis in #233.

Solution

Wrap init_data_reader/2 in a three-attempt retry that catches missing_file, mirroring the existing init_offset_reader/3, and handle the resulting {error, retries_exhausted} in osiris_replica_reader:init/1 as a clean {stop, normal}, like the sibling error clauses. init_data_reader lists the index files fresh on each attempt (sorted_index_files(Dir)), so a retry sees the post-retention directory state.

Also close the index fd on every exit path in last_valid_idx_record/1. It opens the fd and then calls file_size/1, which can throw missing_file for the same retention reason. Previously that throw crashed the process and the fd was reclaimed by the runtime; now that init_data_reader catches missing_file, the process survives, so the un-closed fd would leak, up to once per retry and permanently if a later attempt succeeds. A try ... after file:close/1 closes it regardless of how the body exits. This also fixes the latent version of the same leak reachable from init_offset_reader, which already catches the throw.

This is the same class of benign, race-driven osiris_replica_reader crash as #230 (fixed by #231): one code path handles the condition gracefully and a sibling path does not. Both were surfaced by the same high-throughput test, both leave the cluster healthy via the reader's temporary restart, and both are noisy only in the logs. This fix is independent of #231 and was verified on a run with the #231 sendfile guard already applied.

On testing

I have not added a test. The crash is an inherent TOCTOU: to fire the real throw(missing_file) the index file must be listed by sorted_index_files/1 and then gone when open/2 reads it within one synchronous call, so it is not reproducible on demand without mocking the file layer (and meck is not currently a test dependency of this repo). The retry it mirrors, init_offset_reader/3, ships without a dedicated test for the same reason. Happy to add a mock-based test if you would prefer one.

Two related points I considered and deliberately left alone, noted here so they are visible:

• The retry does not strip a cached index_files key from Config the way init_offset_reader/3 does with maps:remove(index_files, Conf). It does not need to, because init_data_reader0 lists via sorted_index_files(Dir) (the directory), not sorted_index_files(Config), so each attempt already rescans the directory. If a future change routes the reader context through the config-aware variant, this retry should adopt the same maps:remove/2.
• The retries fire back-to-back with no delay. This matches init_offset_reader/3 (which carries a standing TODO about limiting retries); I kept the two consistent rather than diverging here.

Closes #233

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request