Skip to content

fix #296 - #297

Merged
marvin-j97 merged 3 commits into
mainfrom
fix/296
Jul 16, 2026
Merged

fix #296#297
marvin-j97 merged 3 commits into
mainfrom
fix/296

Conversation

@marvin-j97

@marvin-j97 marvin-j97 commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator

fixes #296

Summary by CodeRabbit

  • Bug Fixes

    • Improved error propagation across journal operations, including journal writer acquisition, persistence during recovery, and batch commit handling, so failures surface reliably instead of being potentially suppressed.
  • Breaking Changes

    • Journal-related operations now surface errors for previously infallible journal path and writer access, requiring callers to handle additional failure cases.

@marvin-j97 marvin-j97 added the fix label Jun 13, 2026
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6846b8a7-f815-4c20-9fbb-c71052ee29e9

📥 Commits

Reviewing files that changed from the base of the PR and between 0cadc55 and 77b302d.

📒 Files selected for processing (1)
  • src/batch/mod.rs

📝 Walkthrough

Walkthrough

Journal writer and path accessors now return Result. Batch, database, keyspace, worker, and test code propagates writer-acquisition errors with ?, while recovery also propagates persistence errors.

Changes

Journal Writer Error Handling

Layer / File(s) Summary
Journal API refactoring
src/journal/mod.rs
get_writer() maps poisoned mutexes to crate::Error::Poisoned; path() becomes fallible, and Debug, get_reader(), and persist() handle accessor errors.
Journal test updates
src/journal/test.rs
Rotation, recovery, and truncation tests use fallible writer and path accessors.
Database and batch error propagation
src/batch/mod.rs, src/db.rs, src/db_test.rs
Batch commit, database journal calculations, recovery persistence, and test rotation propagate writer-acquisition errors.
Keyspace operations
src/keyspace/mod.rs
Keyspace clearing, mutation, and memtable rotation methods propagate journal writer errors.
Worker pool message handling
src/worker_pool.rs
RotateMemtable and journal rotation during Flush propagate writer-acquisition errors from worker_tick.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is generic and only references the issue number, so it doesn't clearly describe the change. Use a descriptive title like "Handle poisoned journal lock without destructor panic".
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed The changes make poisoned journal lock access return errors and propagate them, addressing the destructor panic in #296.
Out of Scope Changes check ✅ Passed The changes stay focused on journal lock error handling and related tests, with no obvious unrelated additions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/296

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jun 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.84848% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/journal/mod.rs 54.54% 5 Missing ⚠️

📢 Thoughts on this report? Let us know!

@marvin-j97 marvin-j97 self-assigned this Jun 13, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/journal/mod.rs`:
- Around line 105-106: The #[expect(clippy::expect_used)] attribute on
get_writer is stale because get_writer no longer calls .expect(); remove the
attribute from the get_writer code so the clippy expectation is not left
unfulfilled and cargo clippy stops warning; locate the attribute attached to the
get_writer implementation that ends with self.writer.lock().map_err(|_|
crate::Error::Poisoned) and delete that single #[expect(clippy::expect_used)]
line.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c17236fc-31d9-4dca-b1ee-f49ba6c68af1

📥 Commits

Reviewing files that changed from the base of the PR and between 41bc213 and 8937f5b.

📒 Files selected for processing (7)
  • src/batch/mod.rs
  • src/db.rs
  • src/db_test.rs
  • src/journal/mod.rs
  • src/journal/test.rs
  • src/keyspace/mod.rs
  • src/worker_pool.rs

Comment thread src/journal/mod.rs Outdated
@marvin-j97
marvin-j97 merged commit 6187710 into main Jul 16, 2026
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Poisoned lock causes panic in destructor

1 participant