perf(lsp): lazy load vulnerability checks in background#84
Conversation
Move vulnerability fetching from OSV.dev to a background task using tokio::spawn, allowing version hints to display immediately without blocking on security checks. Key changes: - Add fetch_vulnerabilities_background() static method - Store document state and publish diagnostics before vuln check - Refresh inlay hints immediately after registry fetch - Spawn background task for vulnerability check - Background task refreshes UI when complete This reduces perceived latency from 3-5s to 0-2s for initial display. Closes #72
📝 WalkthroughWalkthroughThe vulnerability fetching mechanism is refactored to execute asynchronously in the background rather than blocking the foreground document processing. The Changes
Sequence DiagramssequenceDiagram
participant Client
participant Backend
participant Cache
participant OSV as OSV Client
participant UI as LSP Client (UI)
Note over Client,UI: Old Flow (Synchronous)
Client->>Backend: process_document()
Backend->>Backend: Fetch version hints
Backend->>Cache: Query vuln_cache
Backend->>OSV: Query vulnerable packages
OSV-->>Backend: Vulnerability data
Backend->>Cache: Update version_cache
Backend->>UI: Refresh inlay hints
Backend->>UI: Publish diagnostics
Backend-->>Client: Return after all done
Note over Client,UI: New Flow (Background)
Client->>Backend: process_document()
Backend->>Backend: Fetch version hints
Backend->>Cache: Store DocumentState
Backend->>UI: Publish diagnostics (immediate)
Backend->>UI: Refresh inlay hints
Backend-->>Client: Return immediately
rect rgb(200, 220, 240)
Note over Backend,UI: Background Task (async, detached)
Backend->>Cache: Query vuln_cache
Backend->>OSV: Batch query packages
OSV-->>Backend: Vulnerability data
Backend->>Cache: Update version_cache
Backend->>UI: Refresh inlay hints (2nd)
Backend->>UI: Refresh diagnostics (2nd)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
dependi-lsp/src/backend.rs (4)
221-238: Non-atomic read-modify-write on cache entry.The pattern
get → mutate → insert(lines 221-238) is not atomic. If another task modifies the same cache key between these operations, updates could be lost. Since vulnerability enrichment is the only writer after version fetch completes, the practical risk is low, but consider using an atomic update method ifHybridCacheexposes one.
254-263: Consider logging when UI refresh requests fail.The
.ok()calls silently discard errors from the refresh requests. While fire-and-forget is appropriate here, a debug-level log on failure would help diagnose client communication issues.🔧 Suggested improvement
- client - .send_request::<request::InlayHintRefreshRequest>(()) - .await - .ok(); - client - .send_request::<request::WorkspaceDiagnosticRefresh>(()) - .await - .ok(); + if let Err(e) = client + .send_request::<request::InlayHintRefreshRequest>(()) + .await + { + tracing::debug!("Background: Failed to refresh inlay hints: {}", e); + } + if let Err(e) = client + .send_request::<request::WorkspaceDiagnosticRefresh>(()) + .await + { + tracing::debug!("Background: Failed to refresh diagnostics: {}", e); + }
404-423: Unnecessary clone ofdependencies.
dependenciesis not used after the spawn block, so it can be moved directly into the closure instead of cloned.♻️ Minor optimization
// Fetch vulnerabilities from OSV.dev in BACKGROUND (non-blocking) if security_enabled && !dependencies.is_empty() { - let dependencies_clone = dependencies.clone(); let cache_clone = Arc::clone(&self.version_cache); let osv_client_clone = Arc::clone(&self.osv_client); let vuln_cache_clone = Arc::clone(&self.vuln_cache); let client_clone = self.client.clone(); tokio::spawn(async move { Self::fetch_vulnerabilities_background( - dependencies_clone, + dependencies, file_type, cache_clone, osv_client_clone, vuln_cache_clone, client_clone, ) .await; }); }
363-378: Config extraction tuple growing - consider a struct for clarity.The 4-element tuple
(diagnostics_enabled, security_show_diags, min_severity, security_enabled)is approaching readability limits. A small config struct would make field access self-documenting.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dependi-lsp/src/backend.rs
🧰 Additional context used
🧬 Code graph analysis (1)
dependi-lsp/src/backend.rs (3)
dependi-lsp/src/vulnerabilities/cache.rs (2)
new(31-37)new(64-71)dependi-lsp/src/vulnerabilities/osv.rs (1)
new(31-41)dependi-lsp/src/file_types.rs (1)
cache_key(86-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: security-audit
- GitHub Check: test
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
dependi-lsp/src/backend.rs (3)
171-178: Good refactor to static method for background task compatibility.The signature change from
&selfto explicitArcparameters is the right approach for detachedtokio::spawntasks, avoiding lifetime issues withself. The ownedVec<Dependency>parameter prevents borrowing complications.
353-402: Immediate state publication before background work - good UX pattern.Document state is stored and diagnostics/inlay hints are published immediately after version fetch completes. This ensures users see version information in ~0–2s as specified in the PR objectives, with vulnerability badges appearing later.
412-422: Fire-and-forget spawn aligns with requirements but consider shutdown implications.The detached
tokio::spawncorrectly allows background vulnerability fetching without blocking. Per PR objectives, the task should "continue operating if the document is closed." However, during LSP shutdown, these tasks will be aborted when the tokio runtime drops. This is likely acceptable, but verify this doesn't cause issues in practice (e.g., partial cache writes, log spam).
1 participant
Summary
tokio::spawnChanges
fetch_vulnerabilities_background()static method that runs independentlyPerformance Impact
Before: User must wait 3-5 seconds to see ANY version information (registry + OSV blocking)
After: Version info shown in 0-2s, vulnerability badges appear 1-3 seconds later (progressive UI)
Test plan
cargo build)cargo test --lib)cargo clippy --all-targets -- -D warnings)cargo fmt --all -- --check)Closes #72
Summary by CodeRabbit
Release Notes
Performance
Changes
✏️ Tip: You can customize this high-level summary in your review settings.