Skip to content

Conversation

@waleedlatif1
Copy link
Collaborator

This reverts commit 833e700.

Summary

Brief description of what this PR does and why.

Fixes #(issue)

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation
  • Other: ___________

Testing

How has this been tested? What should reviewers focus on?

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

Screenshots/Videos

@vercel vercel bot temporarily deployed to Preview – docs October 11, 2025 01:01 Inactive
@vercel
Copy link

vercel bot commented Oct 11, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
sim Building Building Preview Comment Oct 11, 2025 1:01am
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
docs Skipped Skipped Oct 11, 2025 1:01am

@waleedlatif1 waleedlatif1 changed the title Revert "feat(sessions): add redis as priority option for session data (#1592)" feat(sessions): remove redis as priority option for session data Oct 11, 2025
@waleedlatif1 waleedlatif1 merged commit b5486bf into staging Oct 11, 2025
4 of 5 checks passed
@waleedlatif1 waleedlatif1 deleted the feat/redis branch October 11, 2025 01:03
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Summary

Reverts PR #1592 which attempted to add Redis as a priority session storage option but introduced critical bugs.

Key issues fixed by this revert:

  • Critical bug in OTP storage: The fallback OTP storage tried to access (global as any).inMemoryCache which was undefined (it's a module-level const in redis.ts, not a global), completely breaking OTP functionality when Redis was unavailable
  • Misuse of lock functions: Used releaseLock() to delete OTP cache entries instead of proper cache cleanup
  • Added complexity: The Redis secondary storage in betterAuth added an extra failure point without clear benefits

Changes:

  • otp/route.ts: Removes hacky fallback that accessed undefined global variables, restores proper Redis-only OTP storage with clean error handling
  • auth.ts: Removes secondaryStorage configuration, simplifying session management back to database-only
  • redis.ts: Restores proper in-memory cache encapsulation with fallback logic contained within exported functions

Confidence Score: 5/5

  • This PR is safe to merge - it fixes critical bugs by reverting problematic changes
  • Clean revert that removes broken code. The original PR #1592 introduced a critical bug where OTP storage accessed undefined (global as any).inMemoryCache, breaking authentication when Redis was unavailable. This revert restores working functionality with proper abstraction boundaries.
  • No files require special attention - all changes properly restore previous working state

Important Files Changed

File Analysis

Filename Score Overview
apps/sim/app/api/chat/[identifier]/otp/route.ts 1/5 Reverted hacky OTP storage implementation that accessed undefined global.inMemoryCache, breaking OTP functionality when Redis unavailable
apps/sim/lib/auth.ts 5/5 Removed Redis secondaryStorage configuration from betterAuth, reverting to database-only session storage
apps/sim/lib/redis.ts 4/5 Restored in-memory cache fallback for Redis operations with proper encapsulation

Sequence Diagram

sequenceDiagram
    participant Client
    participant OTPRoute as OTP Route
    participant Redis as Redis/Cache
    participant DB as Database (auth.ts)
    
    Note over OTPRoute,Redis: Before Revert (Broken)
    Client->>OTPRoute: POST /api/chat/[id]/otp
    OTPRoute->>Redis: Check Redis availability
    alt Redis available
        Redis-->>OTPRoute: Redis client
        OTPRoute->>Redis: Store OTP with expiry
    else Redis unavailable
        OTPRoute->>Redis: markMessageAsProcessed()
        Note over OTPRoute: Access (global as any).inMemoryCache
        Note over OTPRoute: ❌ undefined - breaks OTP storage
    end
    
    Note over OTPRoute,DB: After Revert (Fixed)
    Client->>OTPRoute: POST /api/chat/[id]/otp
    OTPRoute->>Redis: Check Redis availability
    alt Redis available
        Redis-->>OTPRoute: Redis client
        OTPRoute->>Redis: Store OTP with expiry
    else Redis unavailable
        OTPRoute->>Redis: markMessageAsProcessed()
        Note over Redis: ✓ Uses module-level inMemoryCache
        Redis-->>OTPRoute: OTP stored in fallback cache
    end
    
    Note over DB: Session Storage (Before Revert)
    Client->>DB: Session request
    DB->>Redis: Check secondaryStorage (Redis)
    alt Redis available
        Redis-->>DB: Session data
    else Redis fails
        DB->>DB: Fall back to database
    end
    
    Note over DB: Session Storage (After Revert)
    Client->>DB: Session request
    DB->>DB: Use database only
    Note over DB: Simpler, more reliable
Loading

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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