Skip to content

Conversation

@007harshmahajan
Copy link
Contributor

@007harshmahajan 007harshmahajan commented Nov 18, 2025

This PR make necessary changing for using new sessions table for storing user sessions and also removes the existing sessions from meta table

NEED TO BE MERGED POST RELEASE

@github-actions
Copy link
Contributor

Failed to generate code suggestions for PR

@007harshmahajan 007harshmahajan force-pushed the feat_session_migration branch 4 times, most recently from e8e8757 to a916c55 Compare November 25, 2025 12:50
@007harshmahajan 007harshmahajan marked this pull request as ready for review November 25, 2025 13:03
@007harshmahajan 007harshmahajan marked this pull request as draft November 25, 2025 13:03
@github-actions
Copy link
Contributor

Failed to generate code suggestions for PR

@007harshmahajan 007harshmahajan force-pushed the feat_session_migration branch 3 times, most recently from aa55718 to 5df537f Compare November 26, 2025 11:16
@007harshmahajan 007harshmahajan force-pushed the feat_session_migration branch 3 times, most recently from e823806 to 533034a Compare November 27, 2025 08:32
@007harshmahajan 007harshmahajan marked this pull request as ready for review November 27, 2025 13:53
@github-actions
Copy link
Contributor

Failed to generate code suggestions for PR

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 27, 2025

Greptile Overview

Greptile Summary

This PR migrates user session storage from the generic meta table to a dedicated sessions table with proper schema and indexing. The implementation adds a new sessions table with SeaORM entity model, CRUD operations, and two database migrations.

Key Changes:

  • Created new sessions table with session_id (unique), access_token, and timestamp fields
  • Added migration to create table and unique index on session_id
  • Implemented CRUD operations in infra/table/sessions.rs
  • Updated service/db/session.rs to use new table while maintaining distributed cache coordination
  • Deletes existing sessions from meta table without migration

Issues Found:

  • Missing copyright header in entity file violates repository policy
  • Race condition in set() method's check-then-act pattern could cause concurrent update conflicts
  • Import consolidation needed per codebase style guidelines
  • Existing sessions are deleted without migration - all users will be logged out during upgrade

Confidence Score: 3/5

  • This PR has important concurrency and compliance issues that should be addressed before merging
  • Score reflects one critical race condition in the set() method that could cause data inconsistency under concurrent load, a required copyright header violation, and the user impact of force-logout during upgrade. The migration structure is sound and the cache coordination logic is well-implemented, but the concurrency issue needs resolution.
  • Pay close attention to src/infra/src/table/sessions.rs (race condition in set method) and src/infra/src/table/entity/sessions.rs (missing copyright header)

Important Files Changed

File Analysis

Filename Score Overview
src/infra/src/table/entity/sessions.rs 3/5 new entity model for sessions table - missing required copyright header
src/infra/src/table/migration/m20251118_000003_delete_meta_sessions.rs 3/5 deletes existing sessions from meta table without migration - all users will be logged out on upgrade
src/infra/src/table/sessions.rs 2/5 implements CRUD operations for sessions - has race condition in set() method, imports should be consolidated
src/service/db/session.rs 4/5 updated to use new sessions table with coordinator sync for distributed cache

Sequence Diagram

sequenceDiagram
    participant Client
    participant Service as service/db/session
    participant Table as infra/table/sessions
    participant DB as Database (ORM)
    participant Coordinator as Cluster Coordinator
    participant Cache as USER_SESSIONS Cache

    Note over Client,Cache: Session Set Operation
    Client->>Service: set(session_id, access_token)
    Service->>Table: set(session_id, access_token)
    Table->>DB: find().filter(SessionId).one()
    DB-->>Table: Option<Model>
    alt Session exists
        Table->>Table: Convert to ActiveModel
        Table->>DB: update(access_token, updated_at)
    else Session not found
        Table->>Table: Create new ActiveModel
        Table->>DB: insert(new session)
    end
    DB-->>Table: Result
    Table-->>Service: Ok(())
    Service->>Coordinator: put_into_db_coordinator(key, empty_bytes)
    Service->>Cache: insert(session_id, access_token)
    Service-->>Client: Ok(())

    Note over Client,Cache: Session Get Operation
    Client->>Service: get(session_id)
    Service->>Cache: get(session_id)
    alt Cache hit
        Cache-->>Service: access_token
        Service-->>Client: Ok(access_token)
    else Cache miss
        Service->>Table: get(session_id)
        Table->>DB: find().filter(SessionId).one()
        DB-->>Table: Option<Model>
        alt Found
            Table-->>Service: Some(session)
            Service->>Cache: insert(session_id, access_token)
            Service-->>Client: Ok(access_token)
        else Not found
            Table-->>Service: None
            Service-->>Client: Error("Session not found")
        end
    end

    Note over Client,Cache: Session Watch & Sync
    Coordinator->>Service: watch() event
    alt Put Event
        Service->>Table: get(session_id)
        Table->>DB: find().filter(SessionId).one()
        DB-->>Table: Option<Model>
        Table-->>Service: Some(session)
        Service->>Cache: insert(session_id, access_token)
    else Delete Event
        Service->>Cache: remove(session_id)
    end
Loading

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.

10 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

@YashodhanJoshi1 YashodhanJoshi1 left a comment

Choose a reason for hiding this comment

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

some nitpicks, some comments

@007harshmahajan 007harshmahajan force-pushed the feat_session_migration branch 3 times, most recently from a8f32fc to e220165 Compare December 2, 2025 15:32
@007harshmahajan 007harshmahajan merged commit 32cd68e into main Dec 3, 2025
38 of 40 checks passed
@007harshmahajan 007harshmahajan deleted the feat_session_migration branch December 3, 2025 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants