-
Notifications
You must be signed in to change notification settings - Fork 93
Fix: limit stored request events to 1000 per app #2025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix: limit stored request events to 1000 per app #2025
Conversation
📝 WalkthroughWalkthroughAdds pruning of stored request events in the Nip47 event handler: introduces Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Handler as HandleEvent
participant Pruner as pruneRequestEvents
participant DB as Database
App->>Handler: Send request/event
Handler->>DB: Update app.last_used_at
Handler->>Pruner: Spawn async pruneRequestEvents(app.ID)
Note over Pruner,DB: Runs in goroutine
Pruner->>DB: Query request_events for app (ordered by id asc)
DB-->>Pruner: Return rows beyond MaxRequestEventsPerApp
Pruner->>DB: DELETE old request_events (LIMIT ...)
DB-->>Pruner: Deletion result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nip47/event_handler.go (1)
88-96: Pruning runs before the request is associated with the app.The goroutine fires right after
last_used_atupdate, butrequestEvent.app_idis set later. That means pruning can keep 1000 old rows, then the current request gets attached, leaving the app above the cap until the next request arrives. For low‑traffic apps, the cap can be violated indefinitely.Please move the prune call to immediately after the successful
app_idupdate for the request event (or after state update), so the just‑created row is included in the limit.🔧 Proposed fix (move prune after app_id update)
- } else { - go svc.pruneRequestEvents(app.ID) - } + } ... err = svc.db. Model(&requestEvent). Update("app_id", app.ID). Error if err != nil { ... return } +go svc.pruneRequestEvents(app.ID)
🧹 Nitpick comments (1)
nip47/prune_test.go (1)
13-48: Strengthen the test to assert the newest events are retained.Right now it only checks the count, so a pruning bug that deletes recent rows would still pass. Consider verifying the oldest/ newest remaining
NostrIdvalues.✅ Example assertion to verify retained rows
nip47Svc.pruneRequestEvents(app.ID) svc.DB.Model(&db.RequestEvent{}).Where("app_id = ?", app.ID).Count(&count) assert.Equal(t, int64(MaxRequestEventsPerApp), count) + +var remaining []db.RequestEvent +svc.DB.Where("app_id = ?", app.ID).Order("id asc").Find(&remaining) +assert.Equal(t, MaxRequestEventsPerApp, len(remaining)) +assert.Equal(t, "event_5", remaining[0].NostrId) +assert.Equal(t, "event_1004", remaining[len(remaining)-1].NostrId)
Fixes #21
Implemented automatic pruning to limit stored request events to 1000 per app.
Old events are automatically deleted in the background when new requests come in, preventing disk space issues from spammy apps.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.