Skip to content

Conversation

@itzlambda
Copy link
Collaborator

@itzlambda itzlambda commented Dec 22, 2025

Summary

  • Reduce log verbosity by downgrading JWT/auth middleware logs from debug to trace level
  • Fix VIP rental handling: skip provider status refresh (uses CSV/database) and fall back to user's registered SSH key when rental lacks one

Summary by CodeRabbit

Release Notes

  • New Features

    • Added VIP rental support with specialized handling
    • Added SSH key fallback for VIP rentals
    • Added JWKS cache management functions
  • Bug Fixes

    • Enhanced error handling for rental queries and audience validation
  • Chores

    • Improved logging instrumentation for better observability

✏️ Tip: You can customize this high-level summary in your review settings.

- Downgrade JWT validation and auth middleware logs from debug to trace
- Downgrade GPU offerings refresh log from debug to trace
- Skip VIP rentals when refreshing deployment status (they use CSV/database)
- Fall back to user's registered SSH key for VIP rentals without ssh_public_key
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

Walkthrough

This pull request enhances logging instrumentation across authentication and JWT validation flows, implements full HTTP retrieval logic with caching for JWKS endpoint, and introduces VIP rental handling with selective provider refresh and SSH key fallback logic.

Changes

Cohort / File(s) Summary
JWT validation and authentication instrumentation
crates/basilica-api/src/api/auth/jwt_validator.rs, crates/basilica-api/src/api/middleware/auth.rs, crates/basilica-api/src/server.rs
Logging level adjustments from debug to trace across JWT validation, JWKS caching, and key ID extraction. fetch_jwks gains full async HTTP client logic with request handling, JSON parsing, validation, and caching. Two new public functions added: clear_jwks_cache() and get_cache_stats(). Enhanced error messages in verify_audience and verify_issuer paths.
VIP rental handling
crates/basilica-api/src/api/routes/secure_cloud.rs, crates/basilica-cli/src/cli/handlers/gpu_rental_helpers.rs
Added is_vip column selection and conditional skip logic for VIP rentals in provider refresh loop. fetch_secure_ssh_info now implements VIP fallback: attempts to fetch user's registered SSH key via API when rental's key is absent and rental is marked VIP. Enhanced error handling for database queries.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas requiring extra attention:

  • crates/basilica-api/src/api/auth/jwt_validator.rs: Async HTTP client implementation in fetch_jwks, cache invalidation logic, and proper error propagation from HTTP and JSON parsing operations
  • crates/basilica-cli/src/cli/handlers/gpu_rental_helpers.rs: VIP fallback logic correctness and error handling paths when user SSH key fetch succeeds/fails
  • crates/basilica-api/src/api/routes/secure_cloud.rs: Verify VIP tuple destructuring and skip logic do not inadvertently affect rental refresh behavior for non-VIP rentals

Possibly related PRs

Suggested reviewers

  • distributedstatemachine

Poem

🐰 Hops through logs with trace-level care,
JWKS cached beyond compare!
VIP rabbits skip the refresh,
SSH keys fallback—oh so fresh!
Auth flows gleam with detail clear! 🔐✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main objectives: reducing auth log noise (downgrading logs to trace level) and improving VIP rental handling (skip provider refresh and SSH key fallback).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch auth-logging-and-vip-fixes

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93ec3b3 and eca274a.

📒 Files selected for processing (5)
  • crates/basilica-api/src/api/auth/jwt_validator.rs
  • crates/basilica-api/src/api/middleware/auth.rs
  • crates/basilica-api/src/api/routes/secure_cloud.rs
  • crates/basilica-api/src/server.rs
  • crates/basilica-cli/src/cli/handlers/gpu_rental_helpers.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-13T15:34:46.544Z
Learnt from: epappas
Repo: one-covenant/basilica PR: 184
File: crates/basilica-validator/src/rental/billing.rs:129-130
Timestamp: 2025-10-13T15:34:46.544Z
Learning: In the basilica-validator crate, rental_id is a String type (not UUID) throughout the system - in RentalInfo struct, database rentals.id column, and all rental operations. The persistence layer methods should accept &str for rental_id parameters, not &Uuid, since they immediately convert to String for SQL binding anyway.

Applied to files:

  • crates/basilica-api/src/api/routes/secure_cloud.rs
🧬 Code graph analysis (2)
crates/basilica-api/src/server.rs (1)
crates/basilica-common/src/persistence/traits.rs (2)
  • count (40-40)
  • count (144-144)
crates/basilica-api/src/api/routes/secure_cloud.rs (2)
crates/basilica-api/src/server.rs (2)
  • sqlx (915-915)
  • sqlx (1141-1141)
crates/basilica-api/src/api/extractors/ownership.rs (5)
  • sqlx (99-99)
  • sqlx (225-225)
  • sqlx (376-376)
  • sqlx (390-390)
  • sqlx (412-412)
⏰ 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: lint-complex
  • GitHub Check: build-api (stable)
  • GitHub Check: build-cli (stable)
🔇 Additional comments (6)
crates/basilica-api/src/server.rs (1)

976-976: LGTM - Log level reduction aligns with PR objectives.

Downgrading from debug! to trace! for routine GPU offerings refresh success messages appropriately reduces log noise while still preserving the information for detailed debugging when needed.

crates/basilica-cli/src/cli/handlers/gpu_rental_helpers.rs (1)

758-775: LGTM - VIP SSH key fallback logic is well-implemented.

The fallback mechanism properly handles three cases:

  1. SSH key already present → use it
  2. VIP rental without key → fetch user's registered SSH key
  3. Non-VIP without key → preserve original None behavior

Error handling gracefully degrades by logging a warning and returning None rather than failing the entire operation.

crates/basilica-api/src/api/middleware/auth.rs (1)

14-14: LGTM - Authentication logging appropriately downgraded to trace level.

The changes correctly:

  • Use trace for routine success paths (JWT attempt, JWT success)
  • Retain warn level for authentication failures (API key failure, JWKS fetch failure, JWT validation failure, audience/issuer mismatch)

This reduces log noise in production while preserving visibility into authentication failures that may indicate security issues.

Also applies to: 151-151, 211-215

crates/basilica-api/src/api/auth/jwt_validator.rs (1)

72-76: LGTM - JWT validation logging appropriately uses trace level.

The instrumentation changes consistently:

  • Use trace for routine validation steps (JWKS fetch, cache hits, key ID extraction, validation success)
  • Retain warn for security-relevant failures (audience/issuer mismatch, audience not found)
  • Retain error for invalid formats

The #[instrument(level = "trace")] annotations ensure spans don't pollute logs at higher verbosity levels.

Also applies to: 80-80, 122-122, 134-134, 140-140, 149-149, 189-193, 199-202, 212-214, 219-219, 241-241, 264-266, 269-269

crates/basilica-api/src/api/routes/secure_cloud.rs (2)

152-169: LGTM - VIP rental handling correctly skips provider refresh.

The implementation properly:

  1. Fetches the is_vip flag in the initial query
  2. Skips provider refresh for VIP rentals since their status comes from CSV/database
  3. Continues normal refresh behavior for non-VIP active rentals

This avoids unnecessary provider API calls for manually-managed VIP machines while maintaining the existing refresh logic for regular rentals.


165-179: Consistent tuple destructuring and VIP logic.

The loop correctly:

  • Destructures the three-tuple (rental_id, status, is_vip)
  • Uses *is_vip to dereference the boolean reference
  • Applies the VIP skip before the status check, avoiding unnecessary string comparisons for VIP rentals

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.

❤️ Share

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

@itzlambda itzlambda marked this pull request as ready for review December 22, 2025 16:15
@itzlambda itzlambda merged commit 6c9f420 into main Dec 22, 2025
14 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