Skip to content

fix(ci): admit intentional async trait implementations - #35

Merged
farhan-syah merged 1 commit into
NodeDB-Lab:mainfrom
presempathy-awb:draft/upstream-clippy-1-98-20260915
Sep 16, 2026
Merged

farhan-syah merged 1 commit into
NodeDB-Lab:mainfrom
presempathy-awb:draft/upstream-clippy-1-98-20260915

Conversation

@presempathy-awb

Copy link
Copy Markdown
Contributor

Summary

Restore the repository's cargo clippy --all-targets --all-features -- -D warnings gate on Rust/Clippy 1.98 by acknowledging the small set of async implementations that are intentionally synchronous today.

This is a CI-compatibility change only. It does not change the Vfs or VfsFile contracts, does not change any future's observable behavior, and does not weaken Clippy globally. The new lint is allowed only at the specific production and test implementations where an async trait contract is deliberately preserved even though the current body can complete without awaiting.

What failed

The latest two upstream CI runs fail only in Test Suite / Lint, Format & Docs after the hosted toolchain began reporting Clippy's unused_async_trait_impl lint. The current gate promotes all warnings to errors, so otherwise-correct implementations now stop the build.

The failures are not ordinary async fn functions that can safely become synchronous. They occur in implementations of PageDB's shared async storage surface:

  • the in-memory Vfs and VfsFile backend;
  • native refusal shims for OPFS and WASI;
  • the synchronous scripted file used by vectored-write contract tests; and
  • pager methods whose generic async shape is intentionally retained across backends.

These implementations happen not to suspend in the affected configurations. Their peers do suspend, and callers rely on one backend-independent interface.

Change

This PR adds clippy::unused_async_trait_impl to the existing, narrow allowances on the affected pager methods and to the specific trait impl blocks that Clippy identifies.

Comments at the less-obvious sites record why the allowance is part of the abstraction contract:

  • memory operations complete under an in-process lock but implement the same async storage API as operating-system backends;
  • non-target OPFS/WASI shims return Unsupported immediately but must still implement the portable trait; and
  • the scripted test double models the production async contract even though its local bookkeeping is synchronous.

No crate-level or module-level allowance is introduced. A new occurrence elsewhere will still fail CI and require its own review.

Why retain async here

Removing async would not be a local warning cleanup. For trait implementations it would stop implementing the trait. Splitting the trait into synchronous and asynchronous variants would multiply the generic surface across the pager, transaction layer, tests, and every VFS backend without improving runtime behavior. Wrapping each body in a manufactured pending operation would silence static analysis by adding useless runtime machinery.

The scoped allowance states the actual invariant directly: these particular implementations are allowed to finish immediately while remaining members of an asynchronous portability contract.

Alternatives considered

Allow the lint crate-wide

Rejected. A crate-wide allowance would conceal accidental async functions in unrelated code and make the -D warnings gate less useful. This PR keeps the warning active everywhere except the reviewed contract sites.

Change Clippy or Rust versions in CI

Rejected. Pinning an older toolchain would make the gate green by avoiding a useful new diagnostic, and it would defer the same failure until the next upgrade. The code should state its intent instead.

Remove async or split the traits

Rejected. The affected methods participate in an intentionally uniform interface. A backend-specific synchronous API would spread conditional behavior into callers and tests for no correctness or performance benefit.

Add artificial awaits

Rejected. Yielding or awaiting an immediately-ready future solely to satisfy a lint would add behavior and overhead while obscuring why the implementation is synchronous.

Compatibility and risk

  • On-disk format: unchanged.
  • Public API: unchanged.
  • Runtime behavior: unchanged; no new await or scheduling point is added.
  • Platform behavior: unchanged on native, WASM/OPFS, and WASI targets.
  • Dependencies and MSRV: unchanged.
  • Release behavior: unchanged.

The primary risk is future code being added inside an allowed impl block and becoming unintentionally synchronous. That is bounded to the few whole-impl allowances required by the lint's placement. The accompanying comments make the intent reviewable, and other Clippy lints remain active inside those implementations.

Verification

Run on the exact upstream base plus this commit:

cargo fmt --all -- --check
cargo clippy -p pagedb --all-targets --all-features -- -D warnings
cargo check -p pagedb --target wasm32-unknown-unknown --lib --features opfs
cargo check -p pagedb --target wasm32-wasip1 --lib

All four commands pass with Rust/Clippy 1.98.1 and target standard libraries from the same pinned toolchain. Before the patch, the Clippy command reproduces the hosted CI failure; after the patch, the gate completes without suppressing unrelated warnings. Both browser-facing target checks also pass, covering the OPFS implementation and the WASI shim touched by the scoped allowances.

The live Sep 8 GitHub run (34231800975) provides the baseline separation: every test, feature, platform, WASM, benchmark, invariant, and audit job passed, while only Test Suite / Lint, Format & Docs failed on unused_async_trait_impl (102079528103). This PR addresses that exact failing gate and no green job's behavior.

The same commit was then used as the prerequisite under both functional candidates prepared alongside it. Their complete PageDB suites passed independently: 791/791 tests for the key-formatting stack and 789/789 tests for the header-capability stack, with the repository's same ten slow tests skipped by default in each run.

The change is annotation-only, so no behavior-specific regression test is appropriate: a test cannot distinguish an accepted lint annotation from the pre-existing implementation. The relevant regression guard is the exact CI command itself, which continues to compile every target and feature combination under -D warnings.

Review guide

The useful review question for each allowance is: "Must this method retain the asynchronous interface even though this implementation currently does not await?" The answer should be yes at every touched site. If a site can leave the shared async contract without widening the API or platform-specific branching, that site should be removed from this PR rather than permitted speculatively.

Non-goals

This PR does not redesign the VFS traits, change executor behavior, alter backend selection, or normalize every existing lint attribute. It restores the existing strict gate with the smallest contract-accurate change.

@farhan-syah
farhan-syah merged commit 5f3a73b into NodeDB-Lab:main Sep 16, 2026
19 checks passed
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