fix(invitation): auto-delete expired invitations on a schedule#1744
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe invitation cleanup flow lists invitations expired beyond a seven-day retention window, removes their relations and rows, runs daily through cron, and is started and stopped with the server lifecycle. Expired invitation cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
whoAbhishekSah
force-pushed
the
feat/auto-delete-expired-invitations
branch
from
6ff8cc9 to
f9286aa
Compare
Nothing removed an invitation once it expired. Trying to accept an expired invite just fails with an error, and creating a new invite for the same user writes over the old one — neither path deletes the expired invite. So an invite that was never accepted or deleted kept its invitations row and both SpiceDB tuples (#user and #org) forever. Add a daily background job (robfig/cron, "0 0 * * *" UTC, same pattern as the domain-verification and session-cleanup jobs) that finds expired invitations and deletes them. The job deletes through the existing invitation.Delete, which removes both SpiceDB tuples and the invitations row together. This is important: the repository's old GarbageCollect ran a raw row DELETE, which would have left the SpiceDB tuples behind as orphans. GarbageCollect is removed and replaced with a read-only ListExpired that the service iterates over. - core/invitation: InitInvitationCleanup / DeleteExpiredInvitations / Close, and a ListExpired repository method. - cmd/serve: start the job at boot and stop it on shutdown, next to the other periodic jobs. - store/postgres: GarbageCollect -> ListExpired. - tests for the sweep and for ListExpired. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
whoAbhishekSah
force-pushed
the
feat/auto-delete-expired-invitations
branch
from
f9286aa to
b8824f2
Compare
…ention The domain-verification and session-cleanup jobs both declare their daily schedule as a package const named refreshTime. Use the same name and inline comment here instead of a bespoke const. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Coverage Report for CI Build 29073272998Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-0.003%) to 44.876%Details
Uncovered Changes
Coverage Regressions104 previously-covered lines in 4 files lost coverage.
Coverage Stats
💛 - Coveralls |
Coverage Report for CI Build 29002914971Coverage increased (+0.001%) to 44.88%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Only delete invitations that expired at least 7 days ago, instead of everything past its expiry. A recently expired invite still shows up in the list APIs; the daily job removes it once it crosses the retention window. The retention window lives in the service (business logic) as a cutoff time (now minus 7 days) that is passed to ListExpired, so the repository just filters by the given time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
How this was testedRan locally against a real Frontier (Postgres + SpiceDB). Set the cleanup cron to
The invite is removed through |
@whoAbhishekSah Can you validate these scenarios too -
|
|
@AmanGIT07 Validated both re-invite scenarios locally against a real Frontier (Postgres + SpiceDB). Checked the
For scenario 2 the cleanup was done through the same One extra case worth flaggingRe-inviting a user whose invite has expired but is still inside the 7-day keep window (not swept yet) creates a second row plus 2 new tuples, and leaves the old expired row in place. So for a short time there are 2 rows and 4 tuples for the same user in the same org. This is not new. |
![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
🧹 Nitpick comments (2)
core/invitation/service.go (1)
279-320: 🚀 Performance & Scalability | 🔵 TrivialCleanup job lifecycle logic looks correct.
InitInvitationCleanup/DeleteExpiredInvitations/Closecorrectly compute a UTC cutoff, log-and-continue per invitation onDeletefailure, and stop the scheduler cleanly. One scalability note:ListExpiredhas no batch limit, so a large backlog (e.g., first run after this job is deployed) is fetched and processed sequentially with one SpiceDB call + one DB delete per invitation, on a single goroutine. Given the daily cadence and small expected steady-state volume this is unlikely to bite in practice, but worth keeping in mind if invitation volume grows.internal/store/postgres/invitation_repository.go (1)
246-282: 🚀 Performance & Scalability | 🔵 TrivialCorrect replacement for GarbageCollect.
Query logic and error handling are correct and consistent with the existing
Listmethod. Two points worth a look, not blockers:
- No
LIMIT/batching on the query — if this ever needs to catch up on a large backlog, the whole result set is loaded and deleted in one run.- Confirm
expires_atis indexed so this daily scan stays cheap as the invitations table grows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b2fdbb1-cfef-42f8-86b7-27887be3585d
📒 Files selected for processing (6)
cmd/serve.gocore/invitation/mocks/repository.gocore/invitation/service.gocore/invitation/service_test.gointernal/store/postgres/invitation_repository.gointernal/store/postgres/invitation_repository_test.go
cron.New defaults to time.Local, so on a non-UTC host the job would run at local midnight instead of the midnight UTC the comment claims. Pass cron.WithLocation(time.UTC) so the schedule matches the comment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 participants
The problem
Expired invitations were not getting cleaned up.
If you try to accept an expired invite, it just fails. If someone sends a new invite to the same person, it writes over the old one. Neither of these actually removes the expired invite.
So an invite that is never accepted and never deleted stays around forever. Each invite is stored in three places: one row in the
invitationstable, and two records in SpiceDB (one links the invite to the user, one links it to the org). All three keep piling up over time.The fix
Add a job that runs once a day, at midnight UTC, and deletes expired invites. This matches how the domain and session cleanup jobs already work. If the job ever crashes, the crash is caught so it can't take down the server.
The job does not delete an invite the moment it expires. It waits until the invite has been expired for 7 days. That way a recently expired invite still shows up in the list for a week, and the job cleans it up after that.
To delete, the job uses the existing
invitation.Delete. That one call removes the table row and both SpiceDB records together.This is the important part. The code already had a
GarbageCollectfunction that deleted only the table row, with a plain SQL delete. That would have left the two SpiceDB records behind with nothing pointing to them. So I removed it and replaced it with a read-onlyListExpiredthat just finds the old invites. The job then deletes each one the safe way.If one invite fails to delete, the job logs it and moves on, so the rest still get cleaned up.
🤖 Generated with Claude Code