Skip to content

Conversation

@kanishka0411
Copy link

@kanishka0411 kanishka0411 commented Jan 29, 2026

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

    • Request events are automatically pruned; each application retains only the most recent 1,000 request events.
  • Tests

    • Added test coverage validating that request event pruning keeps the expected 1,000 most recent entries.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 29, 2026

📝 Walkthrough

Walkthrough

Adds pruning of stored request events in the Nip47 event handler: introduces MaxRequestEventsPerApp = 1000, a pruneRequestEvents(appId uint) method, and calls it asynchronously after updating an app's last_used_at. A unit test verifies pruning keeps only the most recent 1000 events.

Changes

Cohort / File(s) Summary
Event Pruning Logic
nip47/event_handler.go
Added exported constant MaxRequestEventsPerApp = 1000, implemented pruneRequestEvents(appId uint) on nip47Service, and invoke it asynchronously from HandleEvent after successful last_used_at update to delete older request events beyond the cap.
Event Pruning Test
nip47/prune_test.go
Added TestPruneRequestEvents that inserts 1005 RequestEvent rows, calls pruneRequestEvents, and asserts only MaxRequestEventsPerApp most recent rows remain (checks first/last remaining NostId).

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰
I nibbled through a thousand logs,
Tossed the oldest in tidy clogs,
Now fields breathe easy, neat and bright,
Fresh hops onward, bounded flight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: limiting stored request events to 1000 per app, which is the primary objective of the changeset.
Linked Issues check ✅ Passed The PR implements the core requirement from issue #21 by capping stored request events to 1000 per app and automatically pruning old events in the background.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the request event limit; no unrelated or out-of-scope modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

🧪 Unit Test Generation v2 is now available!

We have significantly improved our unit test generation capabilities.

To enable: Add this to your .coderabbit.yaml configuration:

reviews:
  finishing_touches:
    unit_tests:
      enabled: true

Try it out by using the @coderabbitai generate unit tests command on your code files or under ✨ Finishing Touches on the walkthrough!

Have feedback? Share your thoughts on our Discord thread!


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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_at update, but requestEvent.app_id is 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_id update 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 NostrId values.

✅ 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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent out of control apps doing too many requests

1 participant