Repository navigation
fix(alerts): guard anomaly tick resolves and lease escalation claims - #2315
Conversation
transitionRow runs a caller-built UPDATE guarded on the states the writer read and fails with TransitionConflictError when the row already moved.
Writers updated status columns by id alone, so one that read state earlier silently overwrote a concurrent transition. Every writer now guards its UPDATE on the state it decided from, through transitionRow. Error issues: applyTransition returns a 409 when it loses the race; the tick only regresses rows still done; alert and integration reopens land in regressed with the tick's bookkeeping. Investigations: late diagnoses on failed or resolved runs are dropped; manual status changes may only resolve a finished run or reopen it to its own verdict (MANUAL_INVESTIGATION_TRANSITIONS, 409 otherwise); restart refuses a running pass; one heartbeat-aware stale sweep; anomaly triage settles only once.
…iters The enqueue path's stale sweep now uses the shared heartbeat-aware sweep, markFailed is guarded on investigating, restart rescues a run whose start never claimed it, and the errors tick skips a wake or lease expiry that lost to a concurrent move (a renewed snooze included) instead of failing the org's tick. The investigation detail page disables resolve and restart on a run in flight, matching the server's rules.
…beat The lease clear is guarded on still being expired, and only an issue still in_progress drops to todo. The detail page lets an unclaimed run be run again, and a renewed snooze is covered by a test.
Anomaly tick resolves (returned to baseline, no-data sweep) now move only an incident that is still open, at the reopen count or staleness the tick read, so a manual resolve keeps its reason and an overlapping tick's reopen survives. The sweep clears detector pointers only for rows it actually resolved. Escalation claims take a lease (claimed_at) fenced by attempts; finalize is guarded on the claim and the status it expects, so overlapping ticks page once. A claim that dies before dispatch still spends an attempt and fails at the budget.
… resolve The baseline resolve also guards on the lastTriggeredAt the tick read, and a lost resolve on an incident that is open again keeps it in the tick's attach index instead of dropping it.
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 17 minutes. View limit details
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. |
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
… race A lost baseline resolve (an overlapping tick's lastTriggeredAt bump, or its resolve and reopen) keeps the incident open, the detector pointer, and the attach target for a co-onset fingerprint on the next tick.
|
Added a test for review finding F1 (baseline resolve guard and keepPointer retention), in It seeds an open error-spike incident whose only series is one healthy tick from
Opens run before resolves inside a tick, so nothing reads the kept in-memory attach target later in the same tick. That is why the attach is checked on the next tick. Mutation checks: removing the guard fails both cases. Removing only
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The migration was generated before main added alert_silences, leaving two migration leaves. Regenerated so it chains on the current leaf; the SQL is unchanged (nullable claimed_at column).
Maple review🟢 Confidence 10/10 · safe to merge. The only change since the last review is the regenerated snapshot, which chains to Guards the anomaly tick's resolves and turns escalation dispatch into a leased,
Before merge
|
|
Merged with 2 steps from "Before merge" still open. If they are done, tick them on the review comment; if not, now is the time.
|
Stacked on #2310 (uses its
transitionRowhelper).Anomaly incidents. The tick's two resolves (returned to baseline, no-data sweep) wrote by id only, so they could overwrite a manual resolve's reason and time, or re-resolve an incident an overlapping tick had just reopened or continued. They now go through one guarded resolve: still
open, plus the reopen count andlastTriggeredAtthe tick read (baseline) or the staleness cutoff (sweep). A lost race is a logged no-op. The sweep clears detector pointers only for rows it actually resolved, and a lost baseline resolve on an incident that is open again keeps its pointer and attach target.Escalations. The claim bumped
attemptsbut left the rowqueued, so a tick that read the row after another tick's claim could claim it again and page twice. Claims now take a lease (claimed_at, migrationescalation_claim_lease), only on a queued row with no live lease, andattemptsfences every later write:finalizeis guarded on the claim and on the status it expects, and dispatch is skipped if the pre-dispatchsentflip loses. A requeue releases the lease. A claim that dies before dispatch keeps its lease for 5 minutes, then is retried, and a row whose attempts already hitMAX_ATTEMPTSfails withmax_attempts_exceeded.Note: during the deploy rollout, an old worker's claim leaves
claimed_atnull, so a new worker could claim the same row once in that window.Tests (PGlite; races injected right before the tick's UPDATE):
manualand its time are keptlastTriggeredAt: incident stays open, detector pointer kepttsrs --noEmitis clean in packages/backend and packages/db.🤖 Generated with Claude Code
Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.