Repository navigation
fix(api): expire MongoDB heartbeats after 60 days - #1058
AchoArnold wants to merge 12 commits into
Conversation
Add a single-field TTL index on heartbeat timestamps while preserving the existing query index. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 1 critical |
🟢 Metrics 183 complexity · 152 duplication
Metric Results Complexity 183 Duplication 152
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
 [High impact] The reviewed changes appear safe to merge; no new blocking issue was found. SummaryAdds 60-day MongoDB heartbeat retention and keeps monitoring recoverable when history expires.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Heartbeat arrives] --> B[Save history and record heartbeat identity]
B --> C{Monitor needs a check?}
C -->|Yes| D[Claim and queue a check]
D --> E{History exists?}
E -->|No| F[Clear owned marker and recheck history]
E -->|Yes| G[Check age and queue successor]
G --> H[Queue offline event when late]
H --> I[Check current history and heartbeat identity]
I -->|Still current| J[Mark offline before notifying subscribers]
Reviews (10) · Last reviewed commit: "fix(api): retry emulator checks and stab..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟢 Approval recommended
The MongoDB heartbeat retention change has no active review issues.
No open issues 🎉
What changed in this PR
Adds automatic 60-day retention for MongoDB heartbeat records through a TTL index created during API startup.
Changes:
- Adds the named
heartbeats_retentionindex ontimestamp, allowing MongoDB to remove expired records in the background, including existing records. - Preserves the existing compound index used for heartbeat queries.
| File | Description |
|---|---|
api/pkg/repositories/mongodb.go |
Adds a single-field TTL index with a 60-day expiration period. |
Review effort: Balanced
Clear the queued-check marker when heartbeat history is missing. A new heartbeat restarts inactive monitoring without duplicating active checks. Preserve online/offline notifications and cover recovery with regression tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
Log restart failures without rejecting an already stored heartbeat. Continue processing online status after a restart failure. Move the empty queue marker check into HeartbeatMonitor.RequiresCheck. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
|
@greptileai please review the latest changes again. Heartbeat history expiration stops checks, new heartbeats restart inactive monitors, restart failures are logged while returning the saved heartbeat, and RequiresCheck includes the empty queue marker. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved heartbeat restart races and enqueue-failure handling can create duplicate monitoring chains or leave monitors without active checks, causing duplicate or missed offline alerts.
3 open issues
Review effort: Balanced
Atomically claim restarts and running checks, reject superseded tasks, and recheck history after stopping. Release failed restart claims without hiding saved heartbeats or delaying retries after an online status update. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
|
@greptile review |
Propagate heartbeat-check listener failures to the event endpoint without changing other event types. Retain incoming ownership in leased running and pending claims so queued redelivery can recover an interrupted worker or successor enqueue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
|
@greptile review |
Allow redelivered checks to claim an inactive monitor only after finding heartbeat history. Cover a crash after marker clearing with returning history, and verify missing history still stops without enqueueing any checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
|
@greptile review |
Share the restart claim lease, distinguish migrated checks from legacy tasks, and retry lost stop ownership. Propagate heartbeat enqueue timeouts rather than relying on the non-durable local fallback. Cover restart-only recovery, legacy migration, and retry paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
|
@greptile review |
Keep restart-only claims retryable until a successor is durably queued and recover expired claims atomically. Extract heartbeat lifecycle responsibilities into focused private helpers and persist the requested Clean Code and SOLID conventions in Copilot instructions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
|
@greptile review |
There was a problem hiding this comment.
🟡 Changes recommended
Critical marker and lease-finalization races can acknowledge heartbeat checks without a recoverable queued successor, stalling monitoring and preventing later heartbeats from restarting it.
4 open issues
1 resolved since last review
Review effort: Balanced
Keep stopped monitors restartable and reject restored incoming markers as successors. Return retryable errors when enqueue finalization fails or loses ownership. Deduplicate offline task creation with stable per-check IDs and named Cloud Tasks. Cover stop/redelivery races, lease recovery, and successor failures with regressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
|
@greptile review |
|
Authorized additional pass ended on 9763037: all four targeted findings are fixed and resolved, and all 18 review threads are resolved. Fresh Greptile review is 5/5; fresh Copilot review confirms no open issues but retains its human-assessment caveat. The original ten-minute deadline was preserved; no further round or merge was started. Codacy still requires action on the bufconn-only test server at api/pkg/services/google_cloud_push_queue_service_test.go:77. It is not network-exposed; this needs manual triage or a separately authorized harness adjustment, not an overridden check. Copilot's general stale-alert/lease concern is also preserved in the PR description rather than ignored despite zero inline findings. All affected package tests, vet, formatting, whitespace checks, incremental lint and normal hooks passed; ten CI checks passed and two deployments were skipped. The worktree is clean and pushed; no redundant MongoDB index or Dependabot change was added. |
Revalidate running ownership and history before offline detection and restart. Atomically record online arrivals and guard offline fanout with heartbeat-version CAS. Keep version writes independent of check leases and preserve fail-open heartbeat storage. Cover delayed delivery, lease takeover, concurrent arrivals, and version-name collisions. Narrowly suppress the Semgrep transport warning for the bufconn-only test server. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 264 |
| Duplication | 179 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Concrete assessment and fixes for the latest Copilot general warning are now in bc73e42 and documented in the PR description: revalidated lease ownership/history, heartbeat-version CAS before offline fanout, and deterministic interleaving/RPC regressions. Online/offline state transitions are linearized; exactly-once external delivery is not claimed. The single bufconn test warning uses a rule-specific line annotation with in-memory-only evidence. Checking fresh current-head CI and reviews before declaring completion. |
|
@greptile review |
There was a problem hiding this comment.
🟡 Changes recommended
Emulator delivery failures lack retries and can block recovery until the two-hour stale threshold, while nondeterministic selection of equal-timestamp heartbeats can discard valid offline alerts.
2 open issues
Review effort: Balanced
Retry failed event delivery with exponential backoff and a fresh request deadline. Resolve equal timestamps by UUID using bounded existing-index lookups, without new indexes. Cover failed named-task delivery and equal-timestamp ordering over an in-memory MongoDB peer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
|
Both new Copilot threads are fixed, replied to and resolved in 22adc51. Focused and affected package tests, vet, formatting/whitespace checks, incremental lint (0 new issues) and normal hooks passed. Integration was concretely investigated: unchanged bc73e42 rerun shifted the sending-after-SENT failure from OutstandingFlow to Encrypted. A deterministic interleaving reproduces the existing blind-save SMS race in paths unchanged from main; no unrelated SMS changes were made. Verifying current-head integration and fresh bot reviews before completion. |
|
@greptile review |
There was a problem hiding this comment.
🔵 Needs a closer look
Current-head integration results are pending, leaving cross-replica lease recovery, MongoDB TTL cleanup, PostgreSQL concurrency, and Cloud Tasks behavior unvalidated beyond simulated backends.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
|
Verified finishing result on 22adc51: all 10 current-head checks SUCCESS (including Integration Tests and Codacy with zero annotations), two Deploy jobs SKIPPED. Greptile 5/5 explicitly reviews this SHA; fresh Copilot review 5728941942 reports No open issues, and all 20 threads are resolved. Its pending-integration sentence is superseded by successful run 41323433874; actual live-infrastructure testing limits and the unchanged pre-existing SMS race investigation are documented, without claiming exactly-once delivery. Worktree and remote head match; no merge or #1059 changes. Completed two of this finishing cycles five authorized rounds, without timeout. |
|
Final verification for current head 22adc51: Integration Tests has now completed successfully, so the pending-integration note in the latest Copilot summary is no longer current. All non-deployment checks pass; the two deployment jobs are intentionally skipped for this PR. Greptile reports 5/5 for this exact head, Copilot has no open issues, and all 20 review threads are resolved. The branch is pushed and the worktree is clean. PR remains unmerged. |
Process offline events with an ordinary listener, including stale and repeated deliveries. Remove dispatcher filters and heartbeat-version fencing used only by the guard. Preserve heartbeat retention, check ownership, restart behavior, and fail-open storage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f27d8f52-9620-4ee2-a564-ec4870a5f840
|
Removed the offline-event guard and its heartbeat-version fencing per the explicit author decision to allow repeated and out-of-order offline processing. Restored ordinary offline listener handling; retained TTL, check ownership/recovery, and fail-open heartbeat storage. Focused regression tests, vet, incremental lint, formatting, and whitespace checks pass. Please do not reintroduce stale/duplicate offline-event filtering as a review fix; this behavior is intentional. |
Summary
Intentional offline-event behavior
Per the author's decision, offline events may be processed repeatedly or out of order. Removed the dispatcher filter, pre-delivery validation, heartbeat-version fencing, and their repository methods. Offline events now reach ordinary listeners, which update the phone status without rejecting stale events. Existing queue task naming/retry behavior is unchanged; it does not guarantee exactly-once delivery.
Validation
Live infrastructure and literal 60-day wall-clock expiration are not claimed. Previous review/CI results refer to earlier commits; the latest guard removal is pushed for fresh CI.