Rate limit heartbeat exports#1374
Conversation
Greptile SummaryThis PR introduces a two-layer protection against duplicate heartbeat export jobs: a 10-minute per-user cooldown enforced in the controller (by querying recent
Confidence Score: 4/5Safe to merge; the rate limiting and concurrency guard work correctly for the normal flow. The core protection works as intended, but the controller's app/controllers/my/heartbeats_controller.rb — the Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant Controller as HeartbeatsController#export
participant DB as GoodJob::Job (DB)
participant Queue as GoodJob Queue
participant Job as HeartbeatExportJob
User->>Controller: POST /my/heartbeats/export
Controller->>DB: recent_export_requested?
alt No recent job found
DB-->>Controller: false
Controller->>Queue: perform_later(user_id, ...)
Queue->>DB: INSERT GoodJob::Job record
Note over Queue,Job: GoodJob concurrency check (total_limit: 1 per user key)
alt No concurrent job running
Queue->>Job: execute
Job-->>User: Email with export zip
else Concurrent job exists
Queue-->>Queue: drop job silently
end
Controller-->>User: Redirect with notice
else Recent job exists within 10 min
DB-->>Controller: true
Controller-->>User: Redirect with alert
end
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
app/controllers/my/heartbeats_controller.rb:75-84
**Rate limit applies to failed/discarded jobs**
`recent_export_requested?` queries all `GoodJob::Job` records (pending, running, finished, and failed) within the cooldown window. If a user's export job fails or is dropped by the GoodJob concurrency guard, they'll still be blocked for the full 10 minutes with no indication that anything went wrong. Consider filtering to non-failed states (e.g. excluding `error_event` IS NOT NULL or `discarded_at` IS NOT NULL) so that a failed export doesn't silently consume the user's rate limit slot.
### Issue 2 of 2
app/javascript/pages/Users/Settings/ImportsExports.svelte:339-340
**Hardcoded cooldown duration in UI copy**
The "10 minutes" string is hardcoded here while the authoritative value lives in `My::HeartbeatsController::EXPORT_COOLDOWN`. If the cooldown is ever adjusted, this copy will silently mismatch. Passing the cooldown value as an Inertia prop (e.g. `export_cooldown_minutes`) and interpolating it here would keep the two in sync automatically.
Reviews (1): Last reviewed commit: "Rate limit heartbeat exports" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def recent_export_requested? | ||
| GoodJob::Job | ||
| .where(job_class: "HeartbeatExportJob") | ||
| .where("created_at >= ?", EXPORT_COOLDOWN.ago) | ||
| .where( | ||
| "serialized_params -> 'arguments' -> 0 = to_jsonb(?::bigint)", | ||
| current_user.id | ||
| ) | ||
| .exists? | ||
| end |
There was a problem hiding this comment.
recent_export_requested? queries all GoodJob::Job records (pending, running, finished, and failed) within the cooldown window. If a user's export job fails or is dropped by the GoodJob concurrency guard, they'll still be blocked for the full 10 minutes with no indication that anything went wrong. Consider filtering to non-failed states (e.g. excluding error_event IS NOT NULL or discarded_at IS NOT NULL) so that a failed export doesn't silently consume the user's rate limit slot.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/controllers/my/heartbeats_controller.rb
Line: 75-84
Comment:
**Rate limit applies to failed/discarded jobs**
`recent_export_requested?` queries all `GoodJob::Job` records (pending, running, finished, and failed) within the cooldown window. If a user's export job fails or is dropped by the GoodJob concurrency guard, they'll still be blocked for the full 10 minutes with no indication that anything went wrong. Consider filtering to non-failed states (e.g. excluding `error_event` IS NOT NULL or `discarded_at` IS NOT NULL) so that a failed export doesn't silently consume the user's rate limit slot.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| <p class="mt-3 text-sm text-muted"> |
There was a problem hiding this comment.
The "10 minutes" string is hardcoded here while the authoritative value lives in My::HeartbeatsController::EXPORT_COOLDOWN. If the cooldown is ever adjusted, this copy will silently mismatch. Passing the cooldown value as an Inertia prop (e.g. export_cooldown_minutes) and interpolating it here would keep the two in sync automatically.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/javascript/pages/Users/Settings/ImportsExports.svelte
Line: 339-340
Comment:
**Hardcoded cooldown duration in UI copy**
The "10 minutes" string is hardcoded here while the authoritative value lives in `My::HeartbeatsController::EXPORT_COOLDOWN`. If the cooldown is ever adjusted, this copy will silently mismatch. Passing the cooldown value as an Inertia prop (e.g. `export_cooldown_minutes`) and interpolating it here would keep the two in sync automatically.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
This PR adds protections to prevent users from repeatedly enqueuing expensive heartbeat export jobs by introducing a per-user cooldown at request time and enforcing per-user job concurrency at execution time.
Changes:
- Added a 10-minute per-user cooldown in
My::HeartbeatsController#exportto block repeated export requests. - Added GoodJob concurrency controls to
HeartbeatExportJobto ensure only one export job per user runs at a time. - Updated UI copy and added controller + system test coverage for the rate limit behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/controllers/my/heartbeats_controller.rb |
Adds cooldown logic that checks recent GoodJob enqueues and blocks repeated export requests. |
app/jobs/heartbeat_export_job.rb |
Adds GoodJob concurrency control keyed by user id to prevent parallel exports per user. |
app/javascript/pages/Users/Settings/ImportsExports.svelte |
Updates settings page copy to communicate the 10-minute export cooldown. |
test/controllers/my/heartbeats_controller_test.rb |
Adds integration test coverage ensuring repeated export POSTs are blocked and don’t enqueue a second job. |
test/system/heartbeat_export_test.rb |
Adds end-to-end coverage for the cooldown behavior in the UI flow and a helper to wait for controls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def export_rate_limited? | ||
| return false unless recent_export_requested? | ||
|
|
||
| redirect_to my_settings_imports_exports_path, alert: "Export requests are limited to once every 10 minutes." | ||
| true | ||
| end | ||
|
|
||
| def recent_export_requested? | ||
| GoodJob::Job | ||
| .where(job_class: "HeartbeatExportJob") | ||
| .where("created_at >= ?", EXPORT_COOLDOWN.ago) | ||
| .where( | ||
| "serialized_params -> 'arguments' -> 0 = to_jsonb(?::bigint)", | ||
| current_user.id | ||
| ) | ||
| .exists? |
| return false unless recent_export_requested? | ||
|
|
||
| redirect_to my_settings_imports_exports_path, alert: "Export requests are limited to once every 10 minutes." | ||
| true | ||
| end | ||
|
|
||
| def recent_export_requested? | ||
| GoodJob::Job | ||
| .where(job_class: "HeartbeatExportJob") | ||
| .where("created_at >= ?", EXPORT_COOLDOWN.ago) | ||
| .where( | ||
| "serialized_params -> 'arguments' -> 0 = to_jsonb(?::bigint)", | ||
| current_user.id | ||
| ) | ||
| .exists? | ||
| end |
Summary of the problem
Users could create repeated heartbeat export jobs without any cooldown, which could enqueue duplicate expensive exports.
Describe your changes
HeartbeatExportJob.Validation:
docker compose exec web rails test test/controllers/my/heartbeats_controller_test.rb test/jobs/heartbeat_export_job_test.rbdocker compose exec web rails test test/system/heartbeat_export_test.rbScreenshots / Media
Not included.