Report errors on segment close instead of silently discarding them#592
Merged
Report errors on segment close instead of silently discarding them#592
Conversation
5f43d98 to
83d1a7f
Compare
11 tasks
ee99a47 to
b9309d7
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes segment lifecycle handling so that ra_log_segment:close/1 in append mode propagates failures (from sync and file:close/1) instead of always returning ok, and updates the segment writer to rely on/assume successful closes in key flush paths.
Changes:
- Make
ra_log_segment:close/1(append mode) return sync/close errors to callers rather than silently discarding them. - Update segment writer flush logic to assert segment close succeeds, and defer pivot segment deletion until after the successor segment is opened.
- Minor call-site adjustments to ignore
close/1return values where appropriate (mostly read-mode handles).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/ra_log_segment_writer.erl |
Defers pivot deletion until after successor is opened; asserts successful close when flushing mem tables. |
src/ra_log_segment.erl |
Changes close/1 contract to propagate errors in append mode; updates some internal close call sites and types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ra_log_segment.erl
Outdated
Comment on lines
+796
to
+800
| -spec close(state()) -> ok | {error, file:posix()}. | ||
| close(#state{cfg = #cfg{fd = Fd, | ||
| mode = append, | ||
| file_advise = FileAdvise}} = State) -> | ||
| % close needs to be defensive and idempotent so we ignore the return | ||
| % values here | ||
| _ = sync(State), | ||
| _ = case is_full(State) of | ||
| true -> | ||
| file:advise(Fd, 0, 0, FileAdvise); | ||
| false -> | ||
| ok | ||
| end, | ||
| _ = file:close(Fd), | ||
| ok; | ||
| file_advise = FileAdvise}} = State0) -> | ||
| case sync(State0) of |
src/ra_log_segment.erl
Outdated
Comment on lines
+810
to
+811
| %TODO: if the sync completed ok do we care about errors during | ||
| % file:close/1? |
| {noreply, State0}; | ||
| Succ -> | ||
| _ = prim_file:delete(Pivot), | ||
| _ = ra_log_segment:close(Succ), |
src/ra_log_segment.erl
Outdated
Comment on lines
+796
to
+800
| -spec close(state()) -> ok | {error, file:posix()}. | ||
| close(#state{cfg = #cfg{fd = Fd, | ||
| mode = append, | ||
| file_advise = FileAdvise}} = State) -> | ||
| % close needs to be defensive and idempotent so we ignore the return | ||
| % values here | ||
| _ = sync(State), | ||
| _ = case is_full(State) of | ||
| true -> | ||
| file:advise(Fd, 0, 0, FileAdvise); | ||
| false -> | ||
| ok | ||
| end, | ||
| _ = file:close(Fd), | ||
| ok; | ||
| file_advise = FileAdvise}} = State0) -> | ||
| case sync(State0) of |
ra_log_segment:close/1 in append mode now propagates errors from sync and file:close instead of ignoring them. The segment writer asserts close succeeds when flushing mem tables and defers pivot file deletion until after the successor segment is opened.
b9309d7 to
f035238
Compare
michaelklishin
approved these changes
Mar 17, 2026
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.
ra_log_segment:close/1 in append mode now propagates errors from sync and file:close instead of ignoring them. The segment writer asserts close succeeds when flushing mem tables and defers pivot file deletion until after the successor segment is opened.