Repository navigation
perf(alerting): fence a rule's state flush in one statement, run the tick tail together - #1552
Conversation
Maple review🟢 Confidence 8/10 · likely safe to merge Replaces the per-rule state flush with one claim-fenced statement and runs the tick's five tail steps concurrently. The fence keeps the edit path's lock order and the new tests pin both lost-claim cases, so this is safe to merge.
Production impactProduction traffic of the changed files (last 7 days)
What was checked
Observability coverage: 1 of 2 changes observable
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
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 39 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title accurately summarizes both main changes: the fenced single-statement rule-state flush and concurrent tick-tail execution. It is specific and concise. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files.
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
…tick tail together flushRuleStates went through writeUnderClaim: BEGIN, claim SELECT FOR UPDATE, rule SELECT FOR UPDATE, the upsert, COMMIT. Five dependent round trips per rule, 1.2-1.3 s each in slow fires, 53-68 s of lane time per tick. The flush is now one statement. Two locking CTEs take FOR SHARE on the claim row, then the rule row (the edit path's lock order), matching the current claim and rule version; the upsert reads its rows from jsonb_to_recordset and runs only when the fence matched. A concurrent re-claim or edit still waits on the lock and then finds no fence row. A lost fence keeps the typed AlertRuleClaimLostError; only that path reads the claim again to say why. The steps after the rule pool (release unstarted claims, state housekeeping, push digests, the disabled-rule sweep, alert_checks ingest) do not read each other's writes, so they run concurrently, and each failure still fails the tick. Ingest concurrency goes from 4 to 6, the Worker's outbound connection limit.
0fd7694 to
4bb573b
Compare
Maple review🟡 Confidence 6/10 · needs attention Fences each rule's state flush into one
Findings🟠 Warning · F1 · Checks ingest at 6 takes the outbound slots the tail's Postgres work needsperformance · This tick's tail now runs these six Tinybird POSTs ( 🤖 Prompt to fix this finding with an AI agentProduction impactProduction traffic of the changed files (last 7 days)
What was checked
Observability coverage: 2 of 2 changes observable
|
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
…res steps The tail now runs the checks ingest beside housekeeping and the disabled-rule sweep. At 6 the POSTs could hold all six outbound slots and queue those statements; 4 leaves them two.
Maple review🟢 Confidence 9/10 · safe to merge Replaces the alerting state flush with one claim-fenced
Fixed since the last review
Production impactProduction traffic of the changed files (last 7 days)
What was checked
Observability coverage: 2 of 2 changes observable
|
Maple: after this shipped✅ Shipped clean. Nothing this change touches got worse in the hour after it deployed. Deployed to
Maple looks once, an hour after the first deploy that carries the merge. |
1 participant
Problem
US alerting ticks are bimodal: normal fires take 1.9-4 s, slow fires end at 28.5-30 s (2026-10-18, traces
2ea98b74a6330960b2828c8e2edfee38,95eb784d9208c79c6af97bf9d0fd6350,69562acd5d2372b724b2907ee11ddd86,5750418cd0f9e0000ad53141db847126,c663bffbe1d8e36b3fa8c1888d5f338a). In slow fires every round trip has a floor of about 240 ms. The claim-fenced state write then costs 1,189-1,330 ms per rule (p50), 53-68 s of lane time per tick, more than the warehouse evaluations. That pushes rules past the 25 s deadline: 9-42 rules deferred per slow tick. The tail after the deadline adds another 3.5-5.5 s of dependent round trips. Normal ticks pay the same shape at a smaller floor (fence p50 26-35 ms).Cause
flushRuleStateswrote throughwriteUnderClaim(AlertsService.ts:624-668on main): BEGIN,SELECT ... FOR UPDATEon the claim,SELECT ... FOR UPDATEon the rule, the upsert, COMMIT. That is five dependent round trips for one write per rule.releaseClaims,flushRuleStateHousekeeping, push digests, the disabled-rule sweep, andalert_checksingest at concurrency 4 (:4419-4530), thenprocessQueuedDeliveries.Fix
upsertStatesUnderClaim). AclaimCTE takesFOR SHAREon the claim row matching this tick's claim time. AruleCTE takesFOR SHAREon the rule row matching the loadedupdatedAt, only ifclaimmatched. The upsert reads its rows from onejsonb_to_recordsetparameter and inserts onlywhere exists (select 1 from rule). The statement returns the fence count. 0 means lost, which keeps the existingAlertRuleClaimLostError. Only that path reads the claim again, to say whether the rule was re-claimed or edited. The retry-once on transient errors is unchanged.FOR SHARE, so the statement waits for it. Postgres then re-checks the CTE condition against the committed row (READ COMMITTED), finds the new claim time orupdatedAt, and writes nothing. A re-claim or edit that starts after the statement waits for it, as it waited for the old transaction. Incident writes still usewriteUnderClaim; they happen only on transitions.Effect.exitand the first failure is raised after all finish, so a failure still fails the tick without cutting the others short. Deliveries still run after, so the sweep's resolve notifications go out in the same tick.Before / After
Database.executeand per ingestDatabase.executeMethod: all local, PGlite, through the real
AlertsService.runSchedulerTickwiring. A scratch live-clock test (not committed) adds 240 ms (the slow-fire floor) to every PGlite statement, or to everyDatabase.executecall and every warehouse ingest for the tail rows, after setup. Durations come from recorded span times. The same test ran against origin/main and this branch, 3 runs each; the medians are shown and the runs varied by under 20 ms. Production fence time was 1.2-1.3 s, and the one-rule local Before matches it (1,212 ms). Limits: PGlite has one connection and serializes every statement, so the 6-rule per-statement tick overstates lane contention on both sides. Only the per-execute mode can show the tail's concurrency, and it has one org. No preview deploy: a preview runs no alerting crons (cronGateFor), so it cannot exercise the tick.Expected in production: slow fires lose about 1 s of lane time per evaluated rule (about 95-160 rules per slow tick, across 5 lanes), which should remove most deferred rules. Normal ticks save about 20 ms per rule (fence p50 26-35 ms becomes one statement), about 0.6 s over 166 rules on 5 lanes.
Tests:
AlertsService fenced state flushhas three PGlite cases through the tick. Unchanged claim and rule: the state row is written. A later tick re-claimed the rule mid-evaluation: nothing is written. The rule was edited mid-evaluation: nothing is written. Both negative cases fail when the fence condition is removed (checked by mutation). The existingclaim fencetest (a re-claim during a slow notification) still passes. A lock wait cannot be tested on PGlite: its single backend cannot release a prepared transaction while a statement waits on it, andlock_timeoutdoes not fire there.bunx vitest run src/services/alerts/AlertsService.test.tspasses (111, after the rebase on #1548).bun run --filter=@maple/backend typecheckpasses.How to verify after deploy
Per 6 h, US: ticks over 25 s and 30 s, and deferred rules:
flush_p50_msshould drop to about one round trip (under 10 ms normal, about 250 ms in slow fires). Also check thatAlert rule claim lostwarnings do not rise.Need help on this PR? Tag
@codesmith-botwith what you need.Summary by CodeRabbit