Repository navigation
fix(channels): stop dispatch when journal write fails - #7040
Conversation
Reflow the record() doc comment to one sentence per line per the project's prose-wrapping rule; content is unchanged.
houko
left a comment
There was a problem hiding this comment.
No changelog.d/ fragment was added for this behavior change (dispatch now aborts when the journal write fails, instead of proceeding on an unrecoverable write). Sibling same-day PRs in this batch (#7036, #7038, #7039) each include one under changelog.d/fixed/ or changelog.d/changed/. Worth adding changelog.d/fixed/7040-message-journal-record-errors.md per the format in changelog.d/README.md, or confirming it's intentionally omitted.
Generated by Claude Code
houko
left a comment
There was a problem hiding this comment.
Automated daily review pass: found the PR missing its changelog.d fragment (fixed directly, see db1c74b) and one moderate-confidence test-coverage gap left as an inline comment. Everything else (commit messages, prose wrapping, lock/deadlock safety, error-handling idiom consistency) checked out.
Generated by Claude Code
| next_retry_after: None, | ||
| }; | ||
| j.record(entry).await; | ||
| if !j.record(entry).await { |
There was a problem hiding this comment.
The record() return-value contract is well covered at the MessageJournal level (record_reports_failure_and_does_not_index_unpersisted_entry in message_journal.rs), but neither dispatch_message nor dispatch_with_blocks has a test that exercises this abort path directly — i.e. that a failing record() actually short-circuits before the typing indicator / lifecycle reactions / router dispatch, and that the user-facing retry message goes out.
There's no existing ChannelAdapter mock in this test module (only MockHandle for ChannelBridgeHandle), so adding one is nontrivial — flagging rather than fixing directly. Worth a small mock adapter + a MessageJournal::open() pointed at a pre-created directory (same trick as the message_journal.rs test) to assert the router/agent is never invoked when the journal write fails, for both call sites.
Generated by Claude Code
|
Correction from this automated review pass: two commits it pushed here ( Generated by Claude Code |
Summary
Tests
CARGO_TARGET_DIR=/tmp/librefang-sidecar-delete/target cargo test -p librefang-channels record_reports_failure_and_does_not_index_unpersisted_entry --libCARGO_TARGET_DIR=/tmp/librefang-sidecar-delete/target cargo test -p librefang-channels message_journal::tests --libCARGO_TARGET_DIR=/tmp/librefang-sidecar-delete/target cargo clippy -p librefang-channels --all-targets -- -D warningscargo fmt --all -- --checkgit diff --checkOut of scope