Skip to content

fix(rag): ensure touched session registry updates are persisted during flush#569

Merged
FireFistisDead merged 1 commit into
FireFistisDead:masterfrom
Sandeep6135:fix/session-touch-registry-bypass
Jun 18, 2026
Merged

fix(rag): ensure touched session registry updates are persisted during flush#569
FireFistisDead merged 1 commit into
FireFistisDead:masterfrom
Sandeep6135:fix/session-touch-registry-bypass

Conversation

@Sandeep6135

Copy link
Copy Markdown
Contributor

Fixes #562

Pull Request: Resolve Session Expiration Registry Touch Persistence Bypass

📌 Classification & Priority

Metric Value
Change Type bug-fix
Severity Level high / critical
Target Service rag-service
System Reliability Impact exceptional-state-integrity
Fix Strategy Native Code Refactor

📖 Summary

Important

This PR resolves a critical session lifecycle bug where metadata touch updates (e.g. last_accessed and expires_at updates) for read-only sessions were silently ignored during background flushes, resulting in premature cache expiration and deletion of active sessions.

🔴 Problem

When a session is accessed (e.g., via /ask or /validate-session-write), it is touched to update its last_accessed time in memory. If a session is read-only (not modifying chat history or flashcards), the session ID is added only to _dirty_registry_sessions and not to _dirty_sessions.

However, _flush_dirty_sessions() had an early exit if not _dirty_sessions: return. If no new chat updates occurred, the function returned immediately, completely bypassing _dirty_registry_sessions. Thus, touch metadata updates were never persisted to session_registry.json. On server restart, active sessions reverted to their old timestamps, triggering premature garbage collection and deletion.

🟢 Solution

Refactored _flush_dirty_sessions to drain and process _dirty_registry_sessions independently of _dirty_sessions:

  1. Updated the early return check to verify that both dirty sets are empty before exiting.
  2. Unindented the registry updates block so that touched sessions are written to the database registry regardless of whether chat history updates occurred.

🧪 Steps to Reproduce

  1. Load a session and make several read-only requests (e.g. read session status or ask questions without chat updates).
  2. Verify that the session ID is added to _dirty_registry_sessions in memory.
  3. Wait for the background flush thread interval (default: 10s).
  4. Restart the Python RAG service.
  5. Check session_registry.json and observe that the last_accessed and expires_at fields contain the old, stale timestamps from before the read requests.

🔍 Expected Behaviour

Every read request should touch the session and correctly write the updated last_accessed and expires_at timestamps to session_registry.json on disk during the background flush loop.

❌ Actual Behaviour (Before Fix)

Touch timestamps were not persisted to session_registry.json unless a write request occurred simultaneously, causing read-only active sessions to expire and get deleted upon server restarts.


🛠️ Code Diff Walkthrough

rag-service/main.py

@@ -1587,10 +1587,13 @@
     so that new dirty marks made during the flush are picked up next cycle.
     """
     with sessions_lock:
-        if not _dirty_sessions:
+        if not _dirty_sessions and not _dirty_registry_sessions:
             return
         dirty = set(_dirty_sessions)
         _dirty_sessions.clear()
+        dirty_registry = set(_dirty_registry_sessions)
+        _dirty_registry_sessions.clear()
+
     for session_id in dirty:
         with sessions_lock:
             meta = sessions.get(session_id)
@@ -1597,45 +1597,37 @@
                 continue
             data = _snapshot_session_for_persistence(meta)
         _write_session_meta_file(session_id, data)
+
     if dirty:
         logger.debug("Flushed metadata for %d dirty session(s)", len(dirty))
-        registry_updates = {}
-
-        with sessions_lock:
-            dirty_registry = set(_dirty_registry_sessions)
-            _dirty_registry_sessions.clear()
-
-            for session_id in dirty_registry:
-                meta = sessions.get(session_id)
-
-                if not meta:
-                    continue
-
-                registry_updates[session_id] = {
-                    "created_at": meta.get("created_at"),
-                    "last_accessed": meta.get("last_accessed"),
-                    "expires_at": session_expires_at(
-                        meta.get("last_accessed", now_ts())
-                    ),
-                    "documents": list(meta.get("documents", [])),
-                    "session_dir": meta.get("session_dir"),
-                    "hashed_session_secret": meta.get("hashed_session_secret") or _hash_secret(meta.get("session_secret", "")),
-                }
-
-        if registry_updates:
-
-            with session_registry_lock():
-
-                registry = read_session_registry_unlocked()
-
-                registry.update(registry_updates)
+
+    registry_updates = {}
+    with sessions_lock:
+        for session_id in dirty_registry:
+            meta = sessions.get(session_id)
+            if not meta:
+                continue
+            registry_updates[session_id] = {
+                "created_at": meta.get("created_at"),
+                "last_accessed": meta.get("last_accessed"),
+                "expires_at": session_expires_at(
+                    meta.get("last_accessed", now_ts())
+                ),
+                "documents": list(meta.get("documents", [])),
+                "session_dir": meta.get("session_dir"),
+                "hashed_session_secret": meta.get("hashed_session_secret") or _hash_secret(meta.get("session_secret", "")),
+            }
 
-                write_session_registry_unlocked(registry)
+    if registry_updates:
+        with session_registry_lock():
+            registry = read_session_registry_unlocked()
+            registry.update(registry_updates)
+            write_session_registry_unlocked(registry)
 
-            logger.debug(
-                "Flushed registry metadata for %d session(s)",
-                len(registry_updates),
-            )
+        logger.debug(
+            "Flushed registry metadata for %d session(s)",
+            len(registry_updates),
+        )

Copilot AI review requested due to automatic review settings June 18, 2026 17:33
@vercel

vercel Bot commented Jun 18, 2026

Copy link
Copy Markdown

@Sandeep6135 is attempting to deploy a commit to the firefistisdead's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@Sandeep6135, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 51 minutes and 51 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5dda2272-7fbc-43ea-bb1a-f379ffd0c462

📥 Commits

Reviewing files that changed from the base of the PR and between 5590b87 and aa9b99e.

📒 Files selected for processing (1)
  • rag-service/main.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@github-actions github-actions Bot added bug Something isn't working enhancement New feature or request feature A new feature or improvement fix A targeted fix or cleanup level:critical question Further information is requested rag-service FastAPI / model service work labels Jun 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a persistence bug in the RAG service’s session lifecycle where “touch” updates (e.g., last_accessed / expires_at) for read-only session access were not being flushed to session_registry.json, causing active sessions to expire prematurely after restarts.

Changes:

  • Updates _flush_dirty_sessions() to only early-return when both _dirty_sessions and _dirty_registry_sessions are empty.
  • Drains _dirty_registry_sessions independently and persists touched-session metadata to session_registry.json even when there are no per-session metadata writes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread rag-service/main.py
Comment on lines +1604 to +1606
"documents": list(meta.get("documents", [])),
"session_dir": meta.get("session_dir"),
"hashed_session_secret": meta.get("hashed_session_secret") or _hash_secret(meta.get("session_secret", "")),
Comment thread rag-service/main.py
Comment on lines 1573 to +1579
with sessions_lock:
if not _dirty_sessions:
if not _dirty_sessions and not _dirty_registry_sessions:
return
dirty = set(_dirty_sessions)
_dirty_sessions.clear()
dirty_registry = set(_dirty_registry_sessions)
_dirty_registry_sessions.clear()
@FireFistisDead FireFistisDead merged commit b33d6eb into FireFistisDead:master Jun 18, 2026
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request feature A new feature or improvement fix A targeted fix or cleanup gssoc:approved level:critical quality:exceptional question Further information is requested rag-service FastAPI / model service work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Session Expiration Registry Touch Persistence Bypass

3 participants