cleanupRoutine that automatically deletes debate rooms that haven't been active for 24 hours.#304
cleanupRoutine that automatically deletes debate rooms that haven't been active for 24 hours.#304aniket866 wants to merge 3 commits into
cleanupRoutine that automatically deletes debate rooms that haven't been active for 24 hours.#304Conversation
📝 WalkthroughWalkthroughImplemented an automatic room cleanup system in the debate controller to address memory accumulation. Added background cleanup routine with configurable TTL that periodically removes inactive debate rooms. Enhanced DebateMessage and DebateRoom structures with timestamp and activity tracking fields to support the cleanup mechanism. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/controllers/debate_controller.go (3)
146-157:⚠️ Potential issue | 🔴 CriticalCritical: Path traversal vulnerability —
roomIDis unsanitised user input.
room.RoomIDoriginates from theroomquery parameter and is interpolated directly into a filesystem path. An attacker can submitroom=../../etc/cron.d/evilto write arbitrary files anywhere the process has permission.The comment on line 153 acknowledges this but does not mitigate it. Sanitise or reject invalid IDs before they enter the map.
🔒 Proposed fix — validate roomID at the handler entry point
Add a validation helper and call it early in both handlers:
import "regexp" var validRoomID = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`)Then in
SubmitDebateMessageHandler(and similarly inGetDebateTranscriptHandler):roomID := c.Query("room") if roomID == "" { c.JSON(http.StatusBadRequest, gin.H{"error": "room parameter required"}) return } + if !validRoomID.MatchString(roomID) { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid room ID"}) + return + }
149-156:⚠️ Potential issue | 🟠 MajorErrors are silently swallowed.
Both the
json.MarshalIndenterror (line 150) and theos.WriteFileerror (line 155 — emptyifbody) are discarded. In production you won't know when persistence is failing.At minimum, log the errors:
♻️ Proposed fix
data, err := json.MarshalIndent(room, "", " ") if err != nil { + log.Printf("[Persist] Failed to marshal room %s: %v", room.RoomID, err) return } - // Note: Ensure roomID is sanitized in production to prevent path traversal filename := fmt.Sprintf("room_%s.json", room.RoomID) - if err := os.WriteFile(filename, data, 0644); err != nil { + if err := os.WriteFile(filename, data, 0644); err != nil { + log.Printf("[Persist] Failed to write %s: %v", filename, err) }
130-143:⚠️ Potential issue | 🟠 MajorData race:
roomis serialized without holdingroom.Mutex.After
debateRoomsMutex.Unlock()at line 137,c.JSON(http.StatusOK, room)at line 143 readsroom.Messageswithout any lock. A concurrentSubmitDebateMessageHandlercan be appending toMessagesunderroom.Mutex(lines 113-115) at the same time, producing a data race on the slice header.Lock
room.Mutexaround the serialization:♻️ Proposed fix
if !exists { c.JSON(http.StatusNotFound, gin.H{"error": "room not found"}) return } + room.Mutex.Lock() + defer room.Mutex.Unlock() c.JSON(http.StatusOK, room)
🤖 Fix all issues with AI agents
In `@backend/controllers/debate_controller.go`:
- Around line 57-79: In cleanupOldRooms, when deleting entries from the
debateRooms map, also delete the persisted JSON file(s) written by
persistDebateRoom (e.g., "room_<id>.json") by calling os.Remove for that
filename and handle/log any removal error; alternatively, check a persisted flag
on room (if you track persistence) and only remove files for rooms that were
persisted; update cleanupOldRooms (and ensure debateRoomsMutex remains held
while removing map entries) to perform the file deletion and emit a log on
failure/success.
🧹 Nitpick comments (4)
backend/controllers/debate_controller.go (4)
15-19: Consider making these constants unexported.These constants are internal configuration for the cleanup routine and don't need to be part of the package's public API. Use lowercase names (
cleanupInterval,roomTTL) to keep the API surface small.
42-55: Cleanup goroutine has no shutdown mechanism and hurts testability.Starting a perpetual goroutine in
init()means:
- It cannot be stopped on server shutdown (goroutine leak, potential writes after shutdown).
- It runs on every package import, including in tests, which is surprising and uncontrollable.
Consider accepting a
context.Contextand usingticker+ctx.Done()select, started explicitly frommainrather thaninit().♻️ Suggested approach
-func init() { - go cleanupRoutine() -} - -func cleanupRoutine() { - ticker := time.NewTicker(CleanupInterval) - defer ticker.Stop() - - for range ticker.C { - cleanupOldRooms() - } -} +func StartCleanupRoutine(ctx context.Context) { + go func() { + ticker := time.NewTicker(cleanupInterval) + defer ticker.Stop() + for { + select { + case <-ticker.C: + cleanupOldRooms() + case <-ctx.Done(): + log.Println("[Memory Cleanup] Cleanup routine stopped.") + return + } + } + }() +}Then in
main():ctx, cancel := context.WithCancel(context.Background()) defer cancel() controllers.StartCleanupRoutine(ctx)
98-110: Redundanttime.Now()on room creation.Line 105 initialises
LastActivityfor new rooms, and line 109 unconditionally overwrites it. Remove the initialisation at line 105 or guard line 109 withif exists.♻️ Minor cleanup
room, exists := debateRooms[roomID] if !exists { room = &DebateRoom{ - RoomID: roomID, - Messages: []DebateMessage{}, - LastActivity: time.Now(), // Initialize timestamp + RoomID: roomID, + Messages: []DebateMessage{}, } debateRooms[roomID] = room } room.LastActivity = time.Now() // Update timestamp on new message
30-35:Mutexfield should be unexported.Exporting
sync.Mutexas a struct field (Mutex) exposes synchronization internals to callers. Use a lowercase name (muis idiomatic Go) to keep it package-private.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Fix: Implement a cleanup routine (Ticker) or an LRU cache to delete old rooms. Alternatively, offload storage to Redis/DB and remove from memory.
Why: The server will eventually crash with Out Of Memory (OOM) as traffic increases.
What Changed?
Added LastActivity: The DebateRoom struct now remembers the last time someone touched it.
Added init() and cleanupRoutine(): A background worker now wakes up every 1 hour.
Logic in cleanupOldRooms: It checks every room. If a room hasn't been touched in 24 hours, it is deleted from the map, freeing up the memory.
Video or screenshot : N/A logical changes
@bhavik-mangla Closes #303
Summary by CodeRabbit
New Features
Improvements