fix: resolve all clippy warnings for strict -D warnings CI#5
Merged
Conversation
- Add #[expect] annotations for expect_used (29 sites), missing_panics_doc, significant_drop_in_scrutinee, cast_possible_truncation - Add #[allow(dead_code)] for unused fields/enums (3 items) - Add #[must_use] on 2 methods - Remove unused import (sync::Arc) - Fix explicit deref - Change pub(crate) type alias to pub - Add clippy.toml with allowed-duplicate-crates for hashbrown
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Clippy/lint expectation annotations across the crate, exposes a compaction-filter assigner type and a Builder method to set it, updates Clippy configuration, and adds a test helper to poll for asynchronous write-buffer drain before asserting zero. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/recovery.rs (1)
42-51:⚠️ Potential issue | 🟠 MajorAvoid panicking on malformed keyspace directory names during recovery.
The current extraction path still panics if a directory name is non-UTF8 or non-numeric. Recovery should be resilient here and skip invalid entries.
💡 Proposed fix
- #[expect( - clippy::expect_used, - reason = "keyspace dirs are always valid numeric names" - )] - let keyspace_id = dirent - .file_name() - .to_str() - .expect("should be valid keyspace name") - .parse::<InternalKeyspaceId>() - .expect("should be valid integer"); + let file_name = dirent.file_name(); + let Some(file_name) = file_name.to_str() else { + log::warn!( + "Skipping keyspace directory with non-UTF8 name: {:?}", + file_name + ); + continue; + }; + + let Ok(keyspace_id) = file_name.parse::<InternalKeyspaceId>() else { + log::warn!( + "Skipping non-keyspace directory in keyspaces folder: {}", + keyspace_path.display() + ); + continue; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/recovery.rs` around lines 42 - 51, The code currently uses expect on dirent.file_name().to_str() and .parse::<InternalKeyspaceId>() which will panic on non-UTF8 or non-numeric keyspace dir names; change the extraction in the recovery loop to gracefully handle errors by replacing the chained expects with fallible handling (e.g., if let Some(name) = dirent.file_name().to_str() and then match name.parse::<InternalKeyspaceId>() ) and when either step fails, log or warn about the invalid directory and skip/continue the loop instead of panicking; update references to keyspace_id, dirent, file_name, to_str, and InternalKeyspaceId accordingly.src/builder.rs (1)
163-185:⚠️ Potential issue | 🟡 MinorDoc test will panic due to
todo!()macro.The doc example at line 179 contains
todo!(), which will panic when the doc test runs, causing CI failures.Consider either:
- Adding
no_runto skip execution:```no_run- Providing a concrete implementation or returning
NoneSuggested fix (option 1 - no_run)
/// # Examples /// - /// ``` + /// ```no_run /// # use fjall::{PersistMode, Database, KeyspaceCreateOptions};Suggested fix (option 2 - return None)
/// .with_compaction_filter_factories( /// Arc::new(|keyspace| { /// // Match on the keyspace name to assign specific compaction filters - /// todo!() + /// None /// }) /// )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/builder.rs` around lines 163 - 185, The doc example for Database::builder uses todo!() inside the closure passed to with_compaction_filter_factories which will panic during doc tests; fix it by either marking the fenced code block as non-executable (add no_run to the triple backticks) or replacing todo!() with a safe example implementation (e.g., a closure that matches on keyspace.name and returns None or a concrete Factory value) so the example using with_compaction_filter_factories and Factory no longer panics in tests.
🧹 Nitpick comments (4)
src/db.rs (1)
714-715: Consider adding a reason attribute for consistency.Line 714 uses
#[expect(clippy::expect_used)]without areasonattribute, while the annotation at lines 808-811 in the same function includes a reason. Adding a reason would improve consistency.Suggested fix
- #[expect(clippy::expect_used)] + #[expect(clippy::expect_used, reason = "poisoned lock during recovery is unrecoverable")] let keyspaces = db.supervisor.keyspaces.read().expect("lock is poisoned");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/db.rs` around lines 714 - 715, The existing attribute on the lock unwrap is missing a reason: add a reason attribute to the #[expect(clippy::expect_used)] on the keyspace read to match the style used later in the function; specifically, update the attribute above the call to db.supervisor.keyspaces.read().expect("lock is poisoned") to include a reason string explaining why using expect is acceptable (e.g., that poisoning is unrecoverable or must abort), mirroring the rationale used for the other expect annotations in this function.src/worker_pool.rs (1)
159-168: Consider adding reason attributes for consistency.Lines 159 and 167 use
#[expect(clippy::expect_used)]without areasonattribute, while other annotations in this file (lines 115-116 and 216) include reasons. Adding reasons would maintain consistency.Suggested fix
- #[expect(clippy::expect_used)] + #[expect(clippy::expect_used, reason = "poisoned lock is unrecoverable")] let mut journal_manager = ctx .supervisor .journal_manager .write() .expect("lock is poisoned"); - #[expect(clippy::expect_used)] + #[expect(clippy::expect_used, reason = "poisoned lock is unrecoverable")] let keyspaces = ctx.supervisor.keyspaces.write().expect("lock is poisoned");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worker_pool.rs` around lines 159 - 168, The two attribute uses of #[expect(clippy::expect_used)] on the journal_manager and keyspaces write locks (around let mut journal_manager = ctx.supervisor.journal_manager.write() and let keyspaces = ctx.supervisor.keyspaces.write()) should include a reason string for consistency with other annotations in this file; update both attributes to #[expect(clippy::expect_used = "reason")] with a short reason explaining why using expect is acceptable here (e.g., "poisoned lock indicates unrecoverable supervisor state") so they match the style used at the other annotated sites.src/journal/mod.rs (1)
104-105: Consider adding a reason attribute for consistency.Other
#[expect(clippy::expect_used)]annotations in this file include areasonattribute (e.g., lines 59, 76-79), but this one does not. Adding a reason would maintain consistency and improve documentation.Suggested fix
- #[expect(clippy::expect_used)] + #[expect(clippy::expect_used, reason = "poisoned lock is unrecoverable")]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/journal/mod.rs` around lines 104 - 105, The expect annotation on the mutex lock call (the #[expect(clippy::expect_used)] immediately above self.writer.lock().expect("lock is poisoned")) is missing a reason attribute; update that attribute to include a concise reason matching the style used elsewhere in the file (e.g., explain that lock poisoning is unrecoverable/should abort here), so the annotation becomes consistent with the other expect annotations in this module and documents why using expect is acceptable for this lock acquisition.src/keyspace/mod.rs (1)
789-794: Consider adding a reason attribute for consistency.Line 789 uses
#[expect(clippy::expect_used)]without areasonattribute, while the annotation at line 765 in the same function includes a reason.Suggested fix
- #[expect(clippy::expect_used)] + #[expect(clippy::expect_used, reason = "poisoned lock is unrecoverable")] self.supervisor .journal_manager .write() .expect("lock is poisoned") .maintenance()?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keyspace/mod.rs` around lines 789 - 794, The #[expect(clippy::expect_used)] on the self.supervisor.journal_manager.write().expect(...) call should include a reason string for consistency with the earlier annotation; update the attribute on the lock acquisition (the expect around supervisor.journal_manager.write()) to #[expect(clippy::expect_used, reason = "...")] with a short justification (e.g., that lock poisoning is unrecoverable here or this is a deliberate panic point) so reviewers understand why expect is used in maintenance() invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/builder.rs`:
- Around line 163-185: The doc example for Database::builder uses todo!() inside
the closure passed to with_compaction_filter_factories which will panic during
doc tests; fix it by either marking the fenced code block as non-executable (add
no_run to the triple backticks) or replacing todo!() with a safe example
implementation (e.g., a closure that matches on keyspace.name and returns None
or a concrete Factory value) so the example using
with_compaction_filter_factories and Factory no longer panics in tests.
In `@src/recovery.rs`:
- Around line 42-51: The code currently uses expect on
dirent.file_name().to_str() and .parse::<InternalKeyspaceId>() which will panic
on non-UTF8 or non-numeric keyspace dir names; change the extraction in the
recovery loop to gracefully handle errors by replacing the chained expects with
fallible handling (e.g., if let Some(name) = dirent.file_name().to_str() and
then match name.parse::<InternalKeyspaceId>() ) and when either step fails, log
or warn about the invalid directory and skip/continue the loop instead of
panicking; update references to keyspace_id, dirent, file_name, to_str, and
InternalKeyspaceId accordingly.
---
Nitpick comments:
In `@src/db.rs`:
- Around line 714-715: The existing attribute on the lock unwrap is missing a
reason: add a reason attribute to the #[expect(clippy::expect_used)] on the
keyspace read to match the style used later in the function; specifically,
update the attribute above the call to
db.supervisor.keyspaces.read().expect("lock is poisoned") to include a reason
string explaining why using expect is acceptable (e.g., that poisoning is
unrecoverable or must abort), mirroring the rationale used for the other expect
annotations in this function.
In `@src/journal/mod.rs`:
- Around line 104-105: The expect annotation on the mutex lock call (the
#[expect(clippy::expect_used)] immediately above self.writer.lock().expect("lock
is poisoned")) is missing a reason attribute; update that attribute to include a
concise reason matching the style used elsewhere in the file (e.g., explain that
lock poisoning is unrecoverable/should abort here), so the annotation becomes
consistent with the other expect annotations in this module and documents why
using expect is acceptable for this lock acquisition.
In `@src/keyspace/mod.rs`:
- Around line 789-794: The #[expect(clippy::expect_used)] on the
self.supervisor.journal_manager.write().expect(...) call should include a reason
string for consistency with the earlier annotation; update the attribute on the
lock acquisition (the expect around supervisor.journal_manager.write()) to
#[expect(clippy::expect_used, reason = "...")] with a short justification (e.g.,
that lock poisoning is unrecoverable here or this is a deliberate panic point)
so reviewers understand why expect is used in maintenance() invocation.
In `@src/worker_pool.rs`:
- Around line 159-168: The two attribute uses of #[expect(clippy::expect_used)]
on the journal_manager and keyspaces write locks (around let mut journal_manager
= ctx.supervisor.journal_manager.write() and let keyspaces =
ctx.supervisor.keyspaces.write()) should include a reason string for consistency
with other annotations in this file; update both attributes to
#[expect(clippy::expect_used = "reason")] with a short reason explaining why
using expect is acceptable here (e.g., "poisoned lock indicates unrecoverable
supervisor state") so they match the style used at the other annotated sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 442d2a18-2aa6-402b-89a6-a39f84717f8a
📒 Files selected for processing (16)
clippy.tomlsrc/builder.rssrc/db.rssrc/db_config.rssrc/journal/entry.rssrc/journal/error.rssrc/journal/mod.rssrc/journal/writer.rssrc/keyspace/config/filter.rssrc/keyspace/config/pinning.rssrc/keyspace/mod.rssrc/lib.rssrc/meta_keyspace.rssrc/recovery.rssrc/supervisor.rssrc/worker_pool.rs
There was a problem hiding this comment.
Pull request overview
This PR tightens linting to support a strict cargo clippy --all-features -- -D warnings CI gate and addresses the resulting clippy findings across the crate.
Changes:
- Switch
clippy::expect_usedtodenyat the crate level and annotate unavoidable.expect()sites with scoped#[expect(...)]plus reasons. - Add targeted
#[allow(dead_code)],#[must_use], and doc-lint suppressions/adjustments to satisfy pedantic/nursery/cargo lints. - Add
clippy.tomlconfiguration to allow a known duplicate dependency (hashbrown).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Tightens lint settings (deny clippy::expect_used) to align with strict CI. |
| src/worker_pool.rs | Adds scoped #[expect(clippy::expect_used)] around poisoned-lock .expect() usage. |
| src/supervisor.rs | Allows an unused lock field reserved for future backpressure work. |
| src/recovery.rs | Removes an unused import; adds #[expect] around .expect() while parsing keyspace dirs. |
| src/meta_keyspace.rs | Adds #[expect] around poisoned-lock .expect() calls. |
| src/keyspace/mod.rs | Fixes explicit deref, adds #[must_use], and adds scoped lint expectations in maintenance. |
| src/keyspace/config/pinning.rs | Adds #[expect] for .expect() usage during decode. |
| src/keyspace/config/filter.rs | Adds scoped #[expect] for .expect() usage during encode. |
| src/journal/mod.rs | Adds #[expect] around lock poisoning and path parent .expect() usage. |
| src/journal/writer.rs | Adds #[expect] around .expect() calls during journal rotation path parsing. |
| src/journal/error.rs | Allows unused enum reserved for future recovery mode selection. |
| src/journal/entry.rs | Replaces a local unsafe lint directive with an #[expect(unsafe_code)] plus reason. |
| src/db_config.rs | Makes CompactionFilterAssigner public (for use in public APIs/builders). |
| src/db.rs | Adds scoped #[expect] for lock-poison .expect() and missing panics docs; minor doc updates. |
| src/builder.rs | Adds #[must_use] to a builder-style configuration method. |
| clippy.toml | Configures clippy to allow duplicate hashbrown versions. |
Comments suppressed due to low confidence (4)
src/keyspace/mod.rs:780
- The
#[expect(clippy::significant_drop_in_scrutinee)]here indicates the version-history lock guard returned byget_version_history_lock()is likely held for the entireif let(including the logging in the body). To avoid holding the lock longer than necessary (and reduce deadlock risk), compute themaintenance(...)result in a separate scope/letso the guard is dropped before theif letbody executes.
#[expect(
clippy::significant_drop_in_scrutinee,
reason = "version history lock is short-lived and does not risk deadlock here"
)]
if let Err(e) = keyspace.tree.get_version_history_lock().maintenance(
keyspace.path(),
self.supervisor.snapshot_tracker.get_seqno_safe_to_gc(),
) {
src/keyspace/config/pinning.rs:38
DecodeConfig::decodeuses.expect("cannot fail")onReadBytesExt::read_u8(), which will panic if the input slice is shorter than expected (e.g., corrupted/invalid config bytes). Since the function already returnscrate::Result, prefer propagating the read error (?) and returning a structured decode error instead of panicking; update thereasonaccordingly if this truly is an internal invariant.
#[expect(
clippy::expect_used,
reason = "reading from a byte slice with known length cannot fail"
)]
fn decode(mut bytes: &[u8]) -> crate::Result<Self> {
let len = bytes.read_u8().expect("cannot fail");
let mut v = vec![];
for _ in 0..len {
let b = bytes.read_u8().expect("cannot fail");
v.push(b == 1);
src/recovery.rs:51
- Parsing the keyspace directory name with
to_str().expect(...).parse(...).expect(...)will panic if a non-UTF8 or non-numeric directory exists under the keyspaces folder (e.g., manual edits, partial restores). Since the code already handles stray files, consider handling unexpected directory names by logging a warning andcontinue(or returning an error) rather than panicking during recovery.
#[expect(
clippy::expect_used,
reason = "keyspace dirs are always valid numeric names"
)]
let keyspace_id = dirent
.file_name()
.to_str()
.expect("should be valid keyspace name")
.parse::<InternalKeyspaceId>()
.expect("should be valid integer");
src/journal/writer.rs:106
file_name().expect(...).to_str().expect(...)can still panic on paths without a final component or with non-UTF8 file names, even though the code below already has a graceful error path for invalid.jnlnames. Consider converting theseexpects into fallible handling that returns the sameInvalidFileNameerror, to keep rotation robust and consistent with thestrip_suffix/parsechecks.
#[expect(
clippy::expect_used,
reason = "journal file names are always valid .jnl files"
)]
let Some(basename) = self
.path
.file_name()
.expect("should be valid file name")
.to_str()
.expect("should be valid utf-8")
.strip_suffix(".jnl")
else {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Fix "Returns error, if an IO error occurred." to "Returns an error if an IO error occurs." in 4 doc comments.
This was referenced Mar 15, 2026
Member
Author
|
@coderabbitai Both outside-diff-range findings are valid but out of scope for this clippy-fix PR:
|
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
Pull request overview
This PR aims to make the crate pass cargo clippy --all-features -- -D warnings cleanly by tightening lint levels and adding targeted suppressions/annotations across the codebase.
Changes:
- Switches
clippy::expect_usedtodenyand adds scoped#[expect(...)]annotations around intentionalexpect()usage. - Adds a builder hook for compaction filter factories and tweaks public API types/attributes (
pubtype alias,#[must_use]). - Introduces Clippy configuration (
clippy.toml) and several minor cleanups (unused import removal, deref simplification, doc wording tweaks).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib.rs | Tightens linting (deny clippy::expect_used) and adjusts crate-level Clippy warnings. |
| src/worker_pool.rs | Adds scoped #[expect] for poisoned-lock expect() usage. |
| src/supervisor.rs | Suppresses dead-code warning for a reserved mutex field. |
| src/recovery.rs | Removes an unused import and documents intentional expect() during recovery parsing. |
| src/meta_keyspace.rs | Adds scoped #[expect] around poisoned-lock expect() usage. |
| src/keyspace/mod.rs | Adds #[must_use], simplifies deref, and adds scoped lint suppressions in maintenance paths. |
| src/keyspace/config/pinning.rs | Scopes expect_used suppression for decoding from fixed-length byte slices. |
| src/keyspace/config/filter.rs | Adds a scoped expect_used suppression for repeated Vec writes. |
| src/journal/writer.rs | Adds scoped #[expect] for invariants around journal path/file-name parsing. |
| src/journal/mod.rs | Adds scoped #[expect] for poisoned locks and path-parent invariants. |
| src/journal/error.rs | Suppresses dead-code warning for a reserved public recovery-mode enum. |
| src/journal/entry.rs | Changes unsafe lint handling around uninitialized buffer allocation for LZ4 decompression. |
| src/db_config.rs | Makes CompactionFilterAssigner public. |
| src/db.rs | Adds scoped #[expect] for poisoned-lock expect() usage; updates docs and #[must_use]/lint annotations. |
| src/builder.rs | Marks the new builder method #[must_use]. |
| clippy.toml | Adds an allowed duplicate crate entry (hashbrown). |
You can also share your feedback on Copilot code review. Take the survey.
…aky test - #[allow(dead_code)] → #[expect(dead_code)] on 3 reserved fields/enums - SAFETY invariant documented on unsafe Slice::builder_unzeroed - write_buffer_size_blob: poll async counter after flush (Windows race)
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/keyspace/mod.rs (1)
789-794: Add areasonattribute for consistency.This
#[expect(clippy::expect_used)]annotation is missing thereasonattribute, unlike the one at line 765. For consistency and documentation purposes, consider adding it.♻️ Suggested fix
- #[expect(clippy::expect_used)] + #[expect(clippy::expect_used, reason = "poisoned lock is unrecoverable")] self.supervisor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/keyspace/mod.rs` around lines 789 - 794, The #[expect(clippy::expect_used)] on the write lock call for self.supervisor.journal_manager.write() is missing a reason attribute; update the attribute to #[expect(clippy::expect_used, reason = "exiting early on poisoned RwLock during maintenance"] (or a short explanatory string matching the style of the other expect) so the annotation documents why an expect is acceptable for the .expect("lock is poisoned") in the maintenance() call.tests/write_buffer_size.rs (1)
55-65: Consider applying the same polling pattern towrite_buffer_size_after_insert.The first test (
write_buffer_size_after_insert) at line 26 also callsrotate_memtable_and_wait()followed by an immediate assertion ondb.write_buffer_size(). If the async counter update behavior can cause flakiness inwrite_buffer_size_blob, it could potentially affectwrite_buffer_size_after_insertas well.♻️ Suggested change for consistency
Extract the polling logic into a helper and apply it to both tests:
fn wait_for_write_buffer_drain(db: &Database) { for _ in 0..50 { if db.write_buffer_size() == 0 { break; } std::thread::sleep(std::time::Duration::from_millis(10)); } }Then use it in both test functions before the final assertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/write_buffer_size.rs` around lines 55 - 65, Extract the polling loop that waits for db.write_buffer_size() to reach 0 into a helper function (e.g., fn wait_for_write_buffer_drain(db: &Database)) and call it from both tests (write_buffer_size_after_insert and write_buffer_size_blob) immediately after rotate_memtable_and_wait() and before the final assert; keep the loop logic and sleep timing identical to the existing implementation and reference Database::write_buffer_size and rotate_memtable_and_wait when locating where to insert the helper call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/keyspace/mod.rs`:
- Around line 789-794: The #[expect(clippy::expect_used)] on the write lock call
for self.supervisor.journal_manager.write() is missing a reason attribute;
update the attribute to #[expect(clippy::expect_used, reason = "exiting early on
poisoned RwLock during maintenance"] (or a short explanatory string matching the
style of the other expect) so the annotation documents why an expect is
acceptable for the .expect("lock is poisoned") in the maintenance() call.
In `@tests/write_buffer_size.rs`:
- Around line 55-65: Extract the polling loop that waits for
db.write_buffer_size() to reach 0 into a helper function (e.g., fn
wait_for_write_buffer_drain(db: &Database)) and call it from both tests
(write_buffer_size_after_insert and write_buffer_size_blob) immediately after
rotate_memtable_and_wait() and before the final assert; keep the loop logic and
sleep timing identical to the existing implementation and reference
Database::write_buffer_size and rotate_memtable_and_wait when locating where to
insert the helper call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51ddd343-4758-4206-bfec-8fbef8de6b36
📒 Files selected for processing (5)
src/journal/entry.rssrc/journal/error.rssrc/keyspace/mod.rssrc/supervisor.rstests/write_buffer_size.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/journal/error.rs
- src/journal/entry.rs
…poll helper - Add reason = "poisoned lock is unrecoverable" to #[expect(clippy::expect_used)] in keyspace maintenance (line 789) - Extract wait_for_write_buffer_drain helper, apply to both tests
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/write_buffer_size.rs (1)
6-13: Prefer deadline-based waiting with explicit timeout failureLine 6–13 currently use a fixed retry count and return silently on timeout. Consider a deadline loop that fails with a clear message when draining takes too long; this makes CI flakes easier to diagnose.
Proposed improvement
fn wait_for_write_buffer_drain(db: &Database) { - for _ in 0..50 { - if db.write_buffer_size() == 0 { - break; - } - std::thread::sleep(std::time::Duration::from_millis(10)); - } + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(2); + while std::time::Instant::now() < deadline { + if db.write_buffer_size() == 0 { + return; + } + std::thread::sleep(std::time::Duration::from_millis(10)); + } + panic!( + "write buffer did not drain before timeout; current={}", + db.write_buffer_size() + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/write_buffer_size.rs` around lines 6 - 13, The wait_for_write_buffer_drain helper uses a fixed retry count and returns silently on timeout; replace it with a deadline-based loop using std::time::Instant to compute a hard timeout (e.g., 500–1000 ms), poll db.write_buffer_size() until zero or until Instant::now() > deadline, sleeping between polls, and on timeout fail loudly (panic or assert!) with a clear message including the last observed write_buffer_size and that the drain timed out so CI failures are diagnosable; update the function wait_for_write_buffer_drain and any tests that call it accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/write_buffer_size.rs`:
- Around line 6-13: The wait_for_write_buffer_drain helper uses a fixed retry
count and returns silently on timeout; replace it with a deadline-based loop
using std::time::Instant to compute a hard timeout (e.g., 500–1000 ms), poll
db.write_buffer_size() until zero or until Instant::now() > deadline, sleeping
between polls, and on timeout fail loudly (panic or assert!) with a clear
message including the last observed write_buffer_size and that the drain timed
out so CI failures are diagnosable; update the function
wait_for_write_buffer_drain and any tests that call it accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0432f083-81a3-4d29-af6e-8f40be1e4bef
📒 Files selected for processing (2)
src/keyspace/mod.rstests/write_buffer_size.rs
…helper Replace fixed iteration count with Instant deadline (2s). Panic with current write_buffer_size value on timeout for easier CI flake diagnosis.
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate clippy warnings so the crate passes cargo clippy --all-features -- -D warnings cleanly, aligning with the project’s strict linting posture.
Changes:
- Tighten and satisfy linting by switching
clippy::expect_usedtodenyand adding scoped#[expect(...)]annotations (plus some#[must_use]/ dead-code expectations). - Improve test stability by waiting for asynchronous write-buffer accounting to drain before asserting zero.
- Adjust clippy configuration to allow a known duplicate crate (
hashbrown).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/write_buffer_size.rs | Adds a helper to wait for write-buffer counter drain to reduce timing flakiness. |
| src/worker_pool.rs | Adds scoped #[expect(clippy::expect_used)] for poisoned-lock invariants. |
| src/supervisor.rs | Marks a reserved field with #[expect(dead_code)] to satisfy strict warnings. |
| src/recovery.rs | Removes unused Arc import and adds #[expect(clippy::expect_used)] for directory-name parsing. |
| src/meta_keyspace.rs | Adds #[expect(clippy::expect_used)] for poisoned-lock invariants. |
| src/lib.rs | Makes clippy::expect_used a deny-level lint (strict CI posture). |
| src/keyspace/mod.rs | Fixes redundant deref, adds #[must_use] for metrics, and scopes lint expectations in maintenance. |
| src/keyspace/config/pinning.rs | Adds scoped #[expect(clippy::expect_used)] for invariant decoding expects. |
| src/keyspace/config/filter.rs | Scopes #[expect(clippy::expect_used)] over expect-heavy encoding logic. |
| src/journal/writer.rs | Adds reasons/scoped #[expect(clippy::expect_used)] for path/name invariants. |
| src/journal/mod.rs | Adds scoped #[expect(clippy::expect_used)] around lock poisoning and path parent invariants. |
| src/journal/error.rs | Marks RecoveryMode as reserved via #[expect(dead_code)]. |
| src/journal/entry.rs | Replaces unsafe lint handling with #[expect(unsafe_code)] and adds a // SAFETY: justification. |
| src/db_config.rs | Makes CompactionFilterAssigner pub to support public builder API usage. |
| src/db.rs | Adds #[expect] annotations and doc fixes to satisfy strict clippy and docs-related lints. |
| src/builder.rs | Marks with_compaction_filter_factories as #[must_use]. |
| clippy.toml | Allows a duplicate hashbrown crate version under clippy cargo lints. |
Comments suppressed due to low confidence (1)
src/recovery.rs:51
recover_keyspaceswill panic ifKEYSPACES_FOLDERcontains a directory whose name is not valid UTF-8 or not parseable asInternalKeyspaceId(both.to_str().expect(...)and.parse().expect(...)). This can make database recovery crash on stray/malformed directories (e.g. leftover temp folders). Prefer handling these cases gracefully (e.g. log+continue or return a typed recovery error) instead ofexpect()in recovery paths.
#[expect(
clippy::expect_used,
reason = "keyspace dirs are always valid numeric names"
)]
let keyspace_id = dirent
.file_name()
.to_str()
.expect("should be valid keyspace name")
.parse::<InternalKeyspaceId>()
.expect("should be valid integer");
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
This PR aims to make the crate pass cargo clippy --all-features -- -D warnings cleanly by tightening/adjusting lint levels and adding scoped lint expectations, plus a small test stabilization change for write-buffer accounting.
Changes:
- Switched
clippy::expect_usedtodenyand added many scoped#[expect(...)]suppressions (mostly for lock poisoning / invariants). - Added
clippy.tomlconfiguration to allow a duplicatehashbrowncrate version. - Stabilized
write_buffer_sizetests by waiting for asynchronous write-buffer accounting to drain after flush.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/write_buffer_size.rs | Adds a helper to wait for async write-buffer counter drain to reduce test flakiness. |
| src/worker_pool.rs | Adds scoped #[expect(clippy::expect_used)] for poisoned-lock .expect() usage. |
| src/supervisor.rs | Marks an unused mutex field as intentionally unused for future throttling. |
| src/recovery.rs | Removes unused Arc import and adds scoped expect around recovery parsing. |
| src/meta_keyspace.rs | Adds scoped expects for poisoned-lock .expect() usage. |
| src/lib.rs | Tightens lint policy (deny clippy::expect_used) and removes explicit multiple_crate_versions override. |
| src/keyspace/mod.rs | Adds #[must_use] to metrics(), cleans deref, and adds scoped expects around maintenance paths. |
| src/keyspace/config/pinning.rs | Adds scoped expect around expect() calls in decode. |
| src/keyspace/config/filter.rs | Adds scoped expect annotation in encode loop. |
| src/journal/writer.rs | Adds scoped expects for path invariants during journal rotation. |
| src/journal/mod.rs | Adds scoped expect for poisoned-lock access and path parent invariant. |
| src/journal/error.rs | Marks RecoveryMode as intentionally unused for future selection support. |
| src/journal/entry.rs | Replaces lint override with #[expect(unsafe_code)] and adds a SAFETY comment for unzeroed buffer. |
| src/db_config.rs | Changes CompactionFilterAssigner visibility to avoid clippy warning in a private module context. |
| src/db.rs | Adds #[must_use]/doc lint expectations and scopes .expect() suppressions for poisoned locks. |
| src/builder.rs | Adds #[must_use] to with_compaction_filter_factories. |
| clippy.toml | Allows duplicate hashbrown to satisfy clippy::multiple_crate_versions checks. |
You can also share your feedback on Copilot code review. Take the survey.
…ad keyspace dirs
- pinning.rs: replace .expect("cannot fail") with ? operator in DecodeConfig
- recovery.rs: replace .expect() chain with let-else + log::warn + continue
for non-UTF-8 and non-numeric keyspace directory names
Closes #6
- recovery_skips_non_numeric_keyspace_dir: non-numeric dir name in keyspaces/ - recovery_skips_non_utf8_keyspace_dir: invalid UTF-8 dir name (Linux only) Covers error paths added in recovery.rs graceful skip logic.
…fjall into fix/#1-clippy-warnings
There was a problem hiding this comment.
Pull request overview
This PR aims to make the crate pass cargo clippy --all-features -- -D warnings cleanly by tightening lint settings and adding targeted suppressions/annotations, while also addressing a flaky write-buffer test assertion due to asynchronous counter updates (notably on Windows).
Changes:
- Tighten and standardize lint behavior (e.g., deny
clippy::expect_usedat the crate level) and add scoped#[expect(...)]annotations with reasons for justified.expect()/unsafeusage. - Improve robustness in recovery and configuration decoding (skip invalid keyspace directory names; propagate decode errors instead of panicking).
- Stabilize write-buffer tests by waiting for the buffer-size counter to drain; add
clippy.tomlconfig for allowed duplicate crates.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/write_buffer_size.rs | Adds a bounded wait helper to avoid flaky assertions on async write-buffer counter updates. |
| src/worker_pool.rs | Adds scoped #[expect(clippy::expect_used, reason = ...)] for poisoned-lock .expect() calls. |
| src/supervisor.rs | Adds a reserved backpressure_lock field with a dead_code expectation for future work. |
| src/recovery.rs | Avoids panicking on non-UTF8 / non-numeric keyspace directory names; logs and skips instead. |
| src/meta_keyspace.rs | Adds poisoned-lock expect_used expectations with reasons. |
| src/lib.rs | Changes crate-level linting to #![deny(clippy::expect_used)]. |
| src/keyspace/mod.rs | Marks metrics() as #[must_use], removes redundant deref, and adds scoped lint expectations. |
| src/keyspace/config/pinning.rs | Replaces “cannot fail” .expect() reads with fallible reads (?) for decode. |
| src/keyspace/config/filter.rs | Adds a scoped expect_used expectation for Vec write .expect() calls inside the loop. |
| src/journal/writer.rs | Adds scoped expect_used expectations (with reasons) for path invariants during rotation. |
| src/journal/mod.rs | Adds scoped poisoned-lock expect_used expectation for journal writer locking. |
| src/journal/error.rs | Adds dead_code expectation for a future-facing recovery mode enum. |
| src/journal/entry.rs | Replaces #[warn(unsafe_code)] with scoped #[expect(unsafe_code)] and adds a // SAFETY: justification. |
| src/db_config.rs | Changes compaction-filter assigner type alias visibility to satisfy strict linting and documents rationale. |
| src/db.rs | Adds scoped #[expect] annotations for lock-poisoning panics and improves error doc phrasing. |
| src/builder.rs | Marks with_compaction_filter_factories as #[must_use]. |
| clippy.toml | Allows a known duplicate crate (hashbrown) to keep clippy::multiple_crate_versions clean under strict CI. |
You can also share your feedback on Copilot code review. Take the survey.
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.
Summary
Fix all 45 clippy warnings so that
cargo clippy --all-features -- -D warningspasses clean.Changes
#[expect(clippy::expect_used)]— 29 lock/parse.expect()calls#[expect(clippy::missing_panics_doc)]— 4 public functions#[allow(dead_code)]— 3 unused fields/enums#[must_use]— 2 methodssync::Arc)&**x→x)pub(crate)type alias topubclippy.toml— allow duplicatehashbrownTest plan
cargo clippy --all-features -- -D warnings— 0 errorscargo test— all tests passSummary by CodeRabbit
New Features
Chores
Tests