Skip to content

Conversation

@dani-garcia
Copy link
Member

@dani-garcia dani-garcia commented Nov 10, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28026

📔 Objective

Clippy by default lints when trying to hold an RwLock/Mutex guard across an await point, but that doesn't apply to custom types by default. This PR configures clippy to also apply that lint to our own types that wrap lock guards, which at the moment is only KeyStoreContext.

We only have two instances that needed fixing and both were in tests, but some of the SM code I wrote already had to deal with it by creating short lived contexts, so I assume it might be a problem in the future as well:
https://github.com/bitwarden/sdk-internal/blob/main/bitwarden_license/bitwarden-sm/src/secrets/create.rs#L41-L64

The lint docs:
https://rust-lang.github.io/rust-clippy/master/index.html#await_holding_invalid_type

This is what the lint looks like:
image

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
    team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
    issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2025

Logo
Checkmarx One – Scan Summary & Details4f8175d7-aac7-424f-8f9e-92c87580c6dc

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.69%. Comparing base (b033edd) to head (a4c8b21).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #551      +/-   ##
==========================================
- Coverage   79.70%   79.69%   -0.01%     
==========================================
  Files         300      300              
  Lines       32227    32220       -7     
==========================================
- Hits        25686    25679       -7     
  Misses       6541     6541              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dani-garcia dani-garcia marked this pull request as ready for review November 10, 2025 16:28
@dani-garcia dani-garcia requested review from a team as code owners November 10, 2025 16:28
@claude
Copy link

claude bot commented Nov 10, 2025

Claude finished @dani-garcia's task —— View job


Code Review Complete

Tasks

  • Read changed files
  • Analyze clippy configuration changes
  • Review code refactoring in test files
  • Search for other potential instances
  • Provide final feedback

Summary

This PR appropriately configures Clippy to detect KeyStoreContext held across await points and fixes the two instances found in test code. The changes align well with existing patterns in production code (e.g., bitwarden-sm/src/secrets/create.rs:41-64).

Findings

None. The PR is clean - no major or minor issues, no technical debt introduced, and no significant improvements needed.

The clippy configuration is correctly formatted, the reason message is helpful, and the test refactoring properly scopes the context to drop before await points without changing test behavior.


@dani-garcia dani-garcia merged commit 5052afd into main Nov 11, 2025
57 checks passed
@dani-garcia dani-garcia deleted the ps/clippy-lint-await-keystorecontext branch November 11, 2025 18:07
bw-ghapp bot pushed a commit to bitwarden/sdk-swift that referenced this pull request Nov 11, 2025
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.

4 participants