Skip to content

fix: make Writable trait async and remove the write_via_buf bridge function#6688

Merged
hanabi1224 merged 1 commit intomainfrom
hm/async-writable
Mar 4, 2026
Merged

fix: make Writable trait async and remove the write_via_buf bridge function#6688
hanabi1224 merged 1 commit intomainfrom
hm/async-writable

Conversation

@hanabi1224
Copy link
Copy Markdown
Contributor

@hanabi1224 hanabi1224 commented Mar 4, 2026

Summary of changes

Changes introduced in this pull request:

  • refactor Writable trait to avoid using write_via_buf to bridge Write and AsyncWrite

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Outside contributions

  • I have read and agree to the CONTRIBUTING document.
  • I have read and agree to the AI Policy document. I understand that failure to comply with the guidelines will lead to rejection of the pull request.

Summary by CodeRabbit

  • Refactor
    • Improved internal data writing operations to enhance system performance and reliability.

@hanabi1224 hanabi1224 marked this pull request as ready for review March 4, 2026 14:41
@hanabi1224 hanabi1224 requested a review from a team as a code owner March 4, 2026 14:41
@hanabi1224 hanabi1224 requested review from LesnyRumcajs and sudo-shashank and removed request for a team March 4, 2026 14:41
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 4, 2026

Walkthrough

Converts the write trait system from synchronous to asynchronous in the forest index module. Renames Writeable to Writable, converts write_to from a sync method to async, replaces Write with AsyncWrite-based I/O across all implementations, and updates test utilities to use Tokio runtime for async execution.

Changes

Cohort / File(s) Summary
Async Trait System Refactoring
src/db/car/forest/index/mod.rs
Renamed trait Writeable to Writable, converted write_to method signature to async with AsyncWrite + Unpin bounds, updated all trait implementations for Version, Slot, RawSlot, V1Header, and generic &T wrapper, replaced write_u64 with write_u64_le().await for little-endian encoding, adjusted helper functions (written_len) and round-trip tests to accommodate async writes, removed WriteBytesExt import while preserving ReadBytesExt.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • sudo-shashank
  • LesnyRumcajs
  • akaladarshi
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main changes: converting the Writable trait to async and removing the write_via_buf bridge function.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch hm/async-writable

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/db/car/forest/index/mod.rs (1)

759-762: Replace defensive Runtime::new() with existing block_on import to avoid repeated runtime construction in quickcheck test loops.

The round_trip function is called repeatedly in quickcheck property generators; constructing a tokio::runtime::Runtime on each invocation is unnecessary overhead. Since futures::executor::block_on is already imported at line 658 and used successfully elsewhere in the file (lines 670, 697) for the same AsyncWrite pattern with Vec<u8>, the refactor is safe:

-        let serialized =
-            write_to_vec(|v| tokio::runtime::Runtime::new()?.block_on(original.write_to(v)));
+        let serialized = write_to_vec(|v| block_on(original.write_to(v)));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/db/car/forest/index/mod.rs` around lines 759 - 762, The round_trip helper
currently constructs a new tokio::runtime::Runtime via
Runtime::new()?.block_on(...) for serializing, which is costly in quickcheck
loops; replace that call with the already-imported futures::executor::block_on
to run original.write_to(v) (i.e., change
Runtime::new()?.block_on(original.write_to(v)) to
block_on(original.write_to(v))) in the write_to_vec invocation inside round_trip
so you reuse the existing synchronous executor and avoid repeated runtime
construction.
🤖 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/db/car/forest/index/mod.rs`:
- Around line 759-762: The round_trip helper currently constructs a new
tokio::runtime::Runtime via Runtime::new()?.block_on(...) for serializing, which
is costly in quickcheck loops; replace that call with the already-imported
futures::executor::block_on to run original.write_to(v) (i.e., change
Runtime::new()?.block_on(original.write_to(v)) to
block_on(original.write_to(v))) in the write_to_vec invocation inside round_trip
so you reuse the existing synchronous executor and avoid repeated runtime
construction.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ec3bad47-cc2a-4247-acf0-648389b0de13

📥 Commits

Reviewing files that changed from the base of the PR and between 0dea2f2 and f844e5e.

📒 Files selected for processing (1)
  • src/db/car/forest/index/mod.rs

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 45.00000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.51%. Comparing base (0dea2f2) to head (f844e5e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/db/car/forest/index/mod.rs 45.00% 2 Missing and 9 partials ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
src/db/car/forest/index/mod.rs 88.55% <45.00%> (-1.19%) ⬇️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0dea2f2...f844e5e. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hanabi1224 hanabi1224 enabled auto-merge March 4, 2026 15:06
@hanabi1224 hanabi1224 added this pull request to the merge queue Mar 4, 2026
Merged via the queue into main with commit 57978f8 Mar 4, 2026
39 checks passed
@hanabi1224 hanabi1224 deleted the hm/async-writable branch March 4, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants