Code review findings: missing require, error handling, and test gaps

[Copilot]

Reviewed SWR implementation per maintainer request. Identified critical missing dependency, error handling gaps, and insufficient test coverage for edge cases.

Critical Issues

Missing require "time" - RailsCacheAdapter uses Time.parse without requiring the time library, causing runtime NoMethodError:

# lib/langfuse/rails_cache_adapter.rb
require "concurrent"
require "json"
require "time"  # ← Missing

Silent error swallowing in background refresh - Exceptions during async refresh are not caught, potentially causing thread pool instability:

# Current implementation (line 218-223)
thread_pool.post do
  value = yield
  set_with_metadata(key, value)
ensure
  release_lock(refresh_lock_key)
end

# Should wrap in rescue to log errors

Test Coverage Gaps

• Background refresh error handling - No tests for exceptions during async refresh
• Lock contention - Missing tests for concurrent refresh attempts on same stale key
• Thread pool saturation - No coverage for queue full scenario (max_queue: 50, fallback_policy: :discard)
• Time object handling - get_entry_with_metadata only tests string timestamps, not Time objects

Logic Simplifications

Redundant type checks in get_entry_with_metadata (lines 247-248):

# Current
parsed["fresh_until"] = Time.parse(parsed["fresh_until"]) if parsed["fresh_until"].is_a?(String)

# Simpler: Time.parse handles Time objects
parsed["fresh_until"] = Time.parse(parsed["fresh_until"].to_s)

Example missing unless_exist support - MockRailsCache in swr_cache_example.rb lacks atomic write-if-not-exists, breaking refresh lock demonstration.

Recommendation

Address critical issues (missing require, error handling) before merge. Test gaps are lower priority but should be filled for production readiness.

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.