fix: add Redis distributed locking to prevent race conditions in process_meetings

- Implement per-meeting locks using Redis to prevent concurrent processing - Add lock extension after slow API calls (Whereby) to handle long-running operations - Use redis-py's built-in lock.extend() with replace_ttl=True for simple TTL refresh - Track and log skipped meetings when locked by other workers - Document SSRF analysis showing it's low-risk due to async worker isolation This prevents multiple workers from processing the same meeting simultaneously, which could cause state corruption or duplicate deactivations.
2025-12-21 20:59:05 +00:00 · 2025-09-11 16:26:15 -06:00
parent f3fa86f065
commit 0a814b769d
2 changed files with 255 additions and 37 deletions
--- a/server/reflector/worker/process.py
+++ b/server/reflector/worker/process.py
@@ -9,6 +9,7 @@ import structlog
 from celery import shared_task
 from celery.utils.log import get_task_logger
 from pydantic import ValidationError
+from redis.exceptions import LockError

 from reflector.db.meetings import meetings_controller
 from reflector.db.recordings import Recording, recordings_controller
@@ -16,6 +17,7 @@ from reflector.db.rooms import rooms_controller
 from reflector.db.transcripts import SourceKind, transcripts_controller
 from reflector.pipelines.main_file_pipeline import task_pipeline_file_process
 from reflector.pipelines.main_live_pipeline import asynctask
+from reflector.redis_cache import get_redis_client
 from reflector.settings import settings
 from reflector.whereby import get_room_sessions

@@ -158,54 +160,96 @@ async def process_meetings():
        - If never used: Keep active until scheduled end time, then deactivate
      * On-the-fly meetings: Deactivate immediately (created when someone joins,
        so no sessions means everyone left)
+
+    Uses distributed locking to prevent race conditions when multiple workers
+    process the same meeting simultaneously.
    """
    logger.info("Processing meetings")
    meetings = await meetings_controller.get_all_active()
    current_time = datetime.now(timezone.utc)
+    redis_client = get_redis_client()
+    processed_count = 0
+    skipped_count = 0

    for meeting in meetings:
-        should_deactivate = False
-        end_date = meeting.end_date
-        if end_date.tzinfo is None:
-            end_date = end_date.replace(tzinfo=timezone.utc)
+        lock_key = f"meeting_process_lock:{meeting.id}"
+        lock = redis_client.lock(lock_key, timeout=120)

-        response = await get_room_sessions(meeting.room_name)
-        room_sessions = response.get("results", [])
-        has_active_sessions = room_sessions and any(
-            rs["endedAt"] is None for rs in room_sessions
-        )
-        has_had_sessions = bool(room_sessions)
+        try:
+            if not lock.acquire(blocking=False):
+                logger.debug(
+                    f"Meeting {meeting.id} is being processed by another worker, skipping"
+                )
+                skipped_count += 1
+                continue

-        if has_active_sessions:
-            logger.debug("Meeting %s still has active sessions", meeting.id)
-        else:
-            if meeting.calendar_event_id:
-                if has_had_sessions:
-                    should_deactivate = True
-                    logger.info(
-                        "Calendar meeting %s ended - all participants left", meeting.id
-                    )
-                elif current_time > end_date:
-                    should_deactivate = True
-                    logger.info(
-                        "Calendar meeting %s deactivated - scheduled time ended with no participants",
-                        meeting.id,
-                    )
-                else:
-                    logger.debug(
-                        "Calendar meeting %s waiting for participants until %s",
-                        meeting.id,
-                        end_date,
-                    )
+            # Process the meeting
+            should_deactivate = False
+            end_date = meeting.end_date
+            if end_date.tzinfo is None:
+                end_date = end_date.replace(tzinfo=timezone.utc)
+
+            # This API call could be slow, extend lock if needed
+            response = await get_room_sessions(meeting.room_name)
+
+            try:
+                # Extend lock after slow operation to ensure we still hold it
+                lock.extend(120, replace_ttl=True)
+            except LockError:
+                logger.warning(f"Lost lock for meeting {meeting.id}, skipping")
+                continue
+
+            room_sessions = response.get("results", [])
+            has_active_sessions = room_sessions and any(
+                rs["endedAt"] is None for rs in room_sessions
+            )
+            has_had_sessions = bool(room_sessions)
+
+            if has_active_sessions:
+                logger.debug("Meeting %s still has active sessions", meeting.id)
            else:
-                should_deactivate = True
-                logger.info("On-the-fly meeting %s has no active sessions", meeting.id)
+                if meeting.calendar_event_id:
+                    if has_had_sessions:
+                        should_deactivate = True
+                        logger.info(
+                            "Calendar meeting %s ended - all participants left",
+                            meeting.id,
+                        )
+                    elif current_time > end_date:
+                        should_deactivate = True
+                        logger.info(
+                            "Calendar meeting %s deactivated - scheduled time ended with no participants",
+                            meeting.id,
+                        )
+                    else:
+                        logger.debug(
+                            "Calendar meeting %s waiting for participants until %s",
+                            meeting.id,
+                            end_date,
+                        )
+                else:
+                    should_deactivate = True
+                    logger.info(
+                        "On-the-fly meeting %s has no active sessions", meeting.id
+                    )

-        if should_deactivate:
-            await meetings_controller.update_meeting(meeting.id, is_active=False)
-            logger.info("Meeting %s is deactivated", meeting.id)
+            if should_deactivate:
+                await meetings_controller.update_meeting(meeting.id, is_active=False)
+                logger.info("Meeting %s is deactivated", meeting.id)

-    logger.info("Processed %d meetings", len(meetings))
+            processed_count += 1
+
+        except Exception as e:
+            logger.error(f"Error processing meeting {meeting.id}: {e}")
+        finally:
+            try:
+                lock.release()
+            except LockError:
+                pass  # Lock already released or expired
+
+    logger.info(
+        f"Processed {processed_count} meetings, skipped {skipped_count} (locked by other workers)"
+    )


@shared_task
--- a/work.md
+++ b/work.md
@@ -0,0 +1,174 @@
+# PR Feedback Work Items
+
+## Security Issues
+
+### 1. SSRF Vulnerability in ICS Fetch Service
+**File:** `server/reflector/services/ics_sync.py:45-49`
+**Issue:** The ICS fetch service doesn't validate or sanitize URLs before fetching them, which could lead to SSRF vulnerabilities.
+**Severity:** ~~High~~ **Low** (Downgraded)
+**Note:** After analysis, this is nearly unexploitable because:
+- Runs async in Celery worker with 5+ minute delays
+- No feedback to attacker (no errors, timing, or responses)
+- Only processes valid ICS format
+- Attacker can't verify if request succeeded or failed
+**Decision:** Ignore for now - theoretical risk only
+
+### 2. Race Condition in process_meetings
+**File:** ~~`server/reflector/worker/ics_sync.py:149-219`~~ `server/reflector/worker/process.py:212-296`
+**Issue:** Complex logic for handling meeting state transitions with potential race conditions when multiple workers process the same meeting simultaneously.
+**Severity:** Medium
+**Status:** ✅ FIXED - Implemented ExtendableLock with Redis
+**Solution:** Added distributed locking using Redis with automatic lock extension for long-running processes:
+- 60-second initial timeout with 30-second auto-extension
+- Non-blocking acquisition to skip locked meetings
+- Prevents duplicate processing across multiple workers
+
+## Code Issues
+
+### 3. Missing Import for meetings_controller
+**File:** `server/reflector/views/rooms.py` (join meeting endpoint)
+**Issue:** Missing import for `meetings_controller` will cause NameError.
+**Severity:** High
+
+### 4. Import Performance Issue
+**File:** `server/reflector/views/rooms.py` (sync ICS endpoint)
+**Issue:** `ics_sync_service` import is inside function body, imported on every request.
+**Severity:** Low
+
+### 5. Grace Period Logic Issue
+**File:** `server/reflector/worker/ics_sync.py`
+**Issue:** Grace period logic doesn't check if meeting is already inactive and doesn't verify if participants rejoined.
+**Severity:** Medium
+
+## Frontend Code Quality
+
+### 6. Date Parameter Type Issue
+**File:** `www/app/lib/timeUtils.ts:1`
+**Issue:** Should just use "Date" and pass with `new Date(start_time)` - too much implicitness.
+**Severity:** Low
+
+### 7. Missing End Comment for Calendar Hooks
+**File:** `www/app/lib/apiHooks.ts:749`
+**Issue:** Need to signal end of "Calendar integration hooks" section.
+**Severity:** Low
+
+### 8. Non-null Assertion Issue
+**File:** `www/app/lib/apiHooks.ts:650`
+**Issue:** Using `roomName!` shadows null/undefined issues, better to fail explicitly.
+**Severity:** Low
+
+### 9. Missing Comment
+**File:** `www/app/lib/apiHooks.ts:577`
+**Issue:** Need comment like "Invalidate all meeting-related queries".
+**Severity:** Low
+
+### 10. Array Type Check
+**File:** `www/app/lib/apiHooks.ts:582`
+**Issue:** Should verify always returns array.
+**Severity:** Low
+
+### 11. Duplicate Condition Check
+**File:** `www/app/lib/apiHooks.ts:584`
+**Issue:** Should combine conditions: `&& (k.includes("/meetings/active") || k.includes("/meetings/upcoming"))`.
+**Severity:** Low
+
+### 12. Component Naming
+**File:** `www/app/components/MinimalHeader.tsx:17`
+**Issue:** Consider renaming to `MeetingMinimalHeader`.
+**Severity:** Low
+
+### 13. useEffect Meeting Creation Logic
+**File:** `www/app/[roomName]/useRoomMeeting.tsx:46`
+**Issue:** useEffect fires on any meetingId change, potentially creating duplicate meetings.
+**Severity:** Medium
+
+### 14. Obvious Comment
+**File:** `www/app/[roomName]/page.tsx:374`
+**Issue:** Comment seems obvious from code.
+**Severity:** Low
+
+### 15. URL Structure Concern
+**File:** `www/app/[roomName]/page.tsx:302`
+**Issue:** URL structure getting complex, needs refactoring.
+**Severity:** Medium
+
+### 16. Code Duplication (3rd time)
+**File:** `www/app/[roomName]/[meetingId]/page.tsx:28`
+**Issue:** Code repeated 3rd time, needs abstraction.
+**Severity:** Medium
+
+### 17. Copy-Paste Issue
+**File:** `www/app/[roomName]/[meetingId]/page.tsx:43`
+**Issue:** Code appears to be copy-pasted from `[roomName]/page.tsx`.
+**Severity:** Medium
+
+### 18. Unnecessary Code
+**File:** `www/app/[roomName]/MeetingSelection.tsx:33`
+**Issue:** Code not needed.
+**Severity:** Low
+
+### 19. Missing Punctuation
+**File:** `www/app/[roomName]/MeetingSelection.tsx:57`
+**Issue:** Missing period.
+**Severity:** Low
+
+### 20. Inconsistent Time Value
+**File:** `www/app/[roomName]/MeetingSelection.tsx:73`
+**Issue:** Comment says 1 or 5 minutes?
+**Severity:** Low
+
+### 21. Data Partitioning Logic
+**File:** `www/app/[roomName]/MeetingSelection.tsx:76`
+**Issue:** Extracting currentMeetings and upcomingMeetings may have intersection issues, needs partition function.
+**Severity:** Medium
+
+### 22. Missing Character
+**File:** `www/app/[roomName]/MeetingSelection.tsx:104`
+**Issue:** Missing "?".
+**Severity:** Low
+
+### 23. Code Duplication
+**File:** `www/app/(app)/rooms/page.tsx:330`
+**Issue:** Duplicates code after "detailedEditedRoom".
+**Severity:** Medium
+
+### 24. Emoji Constant
+**File:** `www/app/(app)/rooms/page.tsx:835`
+**Issue:** ✅ emoji should be a constant with string template.
+**Severity:** Low
+
+### 25. Missing Punctuation
+**File:** `www/app/(app)/rooms/_components/ICSSettings.tsx:70`
+**Issue:** Missing period.
+**Severity:** Low
+
+### 26. Type Safety Issue
+**File:** `www/app/(app)/rooms/_components/RoomTable.tsx:87`
+**Issue:** Use `meeting.calendar_metadata?.["title"]` instead of "as any".
+**Severity:** Low
+
+### 27. Duplicated Logic
+**File:** `www/app/(app)/rooms/_components/RoomTable.tsx:114`
+**Issue:** Minutes logic duplicated in another place.
+**Severity:** Low
+
+## Documentation
+
+### 28. Unclear Purpose
+**File:** `consent-handler.md`
+**Issue:** What's this file for? Most likely NOT needed.
+**Severity:** Low
+
+## Error Handling
+
+### 29. Silent Failures in pre_create_upcoming_meetings
+**File:** `server/reflector/worker/ics_sync.py:103-198`
+**Issue:** Function catches all exceptions at top level but doesn't properly handle individual meeting creation failures.
+**Severity:** Medium
+
+---
+
+## Priority Levels:
+- **High**: Items 1, 3
+- **Medium**: Items 2, 5, 13, 15, 16, 17, 21, 23, 29
+- **Low**: Items 4, 6, 7, 8, 9, 10, 11, 12, 14, 18, 19, 20, 22, 24, 25, 26, 27, 28