fix(alerts): clear lastError and lastNotify* fields on alert rule reset (#2027) - #2080
Open
Gaurav-meena95 wants to merge 128 commits into
Open
fix(alerts): clear lastError and lastNotify* fields on alert rule reset (#2027)#2080Gaurav-meena95 wants to merge 128 commits into
Gaurav-meena95 wants to merge 128 commits into
Conversation
Severity, status and comparison live in one place so the API, the worker and the UI cannot drift on the strings they persist. The second migration adds the failure-state columns the scheduler needs to tell a rule that has never run from one whose last run failed.
The example advertised total_tokens and keyed metadata/contains conditions and cited a whitelist file that does not exist on this branch. The editor offers only environment with =/!=, and the evaluator fetches only environment, so the comment now says that and points at the real code.
count(user_id) counts traces that carry a user id, not distinct users, so offering count on a measure labelled unique reads as the wrong number. Row counting stays with the count measure; the string measures keep uniq only. The dropdown already renders disabled at one option, so no form change.
A gap in the data is not one thing. For most rules it decides nothing and the rule should stand on its last judgement, which is what every rule does today. For a rule counting rows, an empty window is honestly zero and the threshold should still run. For a rule watching a source that must keep speaking, the silence is the incident and both the gap and its return are worth a page. The column carries that choice per rule, NOT NULL with the database defaulting it to HOLD, so no rule already stored and no rule mid-flight changes meaning and the migration needs no backfill.
… charts An alert threshold has to mean the number the user saw when they built the rule, so the measure, the filters and the bucketing resolve through the widget query service rather than a second code path. The tooltip now describes tokens per second as the expression actually computes it: the divide is by the millisecond duration, so a 200ms span reports a rate and only a zero duration reads NULL. Renderers separate the aggregations with a meaningful zero (count, sum, uniq) from those without (avg, min, max, percentiles), because filling a gap with zero is right for the first group and a lie for the second. The widget builder and the alert form also had their own copies of the field, operator and value controls. They now render the shared ones at a compact size, so a fix to the value control reaches every surface at once. The dashboard filter's own defect, where the value control swapped to a dropdown after the first keystroke, is not fixed here. It is in production today and ships as its own PR against main so it can be reviewed and released on its own.
An entity seen in two buckets is one entity, not two, so summing uniq's per-bucket values overstates every legend entry and the Total whenever anything spans buckets or categories. uniq stays on the additive side for gap fill (an empty bucket really has 0 distinct); a separate summable predicate now decides what the legend may add up, giving uniq the same averaged-entries, no-Total treatment as the percentiles.
The parameter is dead outside line/area compiles, so the old shape accepted a meaningless width under the bucket cap and 422ed over it. This compiler already fails loud on parameters that don't fit the request's shape (the key-on-an-unkeyed-field guard), so the width now gets the same treatment, and the schema comment says rejected instead of ignored. Also first coverage for the explicit-bucket path: the toStartOfInterval compile, the cap boundary, the millisecond granularity meta and the router pass-through.
A breach message carries the rule, the observed value against the threshold, the window it was measured over and a link back to the alert. Recovery and no-data reuse the same layout so an operator reads one shape of message.
The write path validates a rule against the same field and operator registry the query engine runs, so a rule that saves is a rule the evaluator can execute. The serializer is the only place a row becomes JSON, so the list, the detail and the worker read one shape. Editing the measured thing has to reset what was measured: a rule whose view, measure, aggregation, filters, window or comparison moved is a different question than the one whose answer is on the row, so its severity and both clocks go back to never-evaluated rather than carrying a verdict about the old rule.
The routes are the project's own surface for its rules: create and list, read one, pause or resume it, and delete it. The per-project cap is advisory and the code now says so: the POST counts and then creates, so two creates at 99 both pass. The cap bounds one tenant's share of the scheduler rather than protecting an invariant, so an overrun of a slot or two is cheaper than serializing every create.
A measurement becomes a severity, and a severity change becomes a decision about whether anyone hears about it. OK, ALERT and NO_DATA are the three outcomes; what NO_DATA does is the rule's own choice, and the default is that it displays without notifying, since an absent measurement is not a verdict about the threshold. Two clocks stay deliberately separate. severityChangedAt moves only on a severity change and alertedAt only on an emission, because renotify measures from the last notification and collapsing them would reset the interval on every evaluation. NO_DATA no longer swallows a recovery or manufactures a page. A gap now carries an outstanding page across it, so a rule that went ALERT -> NO_DATA -> OK announces the all-clear the user was waiting for, where before that path was silent and a paged operator never heard it end. The same carry makes the return leg obey renotify: ALERT -> NO_DATA -> ALERT used to page on every re-entry regardless of the mode, so a source flapping in and out of reach paged once a gap. A rule that merely lost its data while OK still says nothing on either leg, since there is no page outstanding to clear. That is HOLD, the default the column carries, and it is what runs when a caller names nothing. ZERO instead puts the empty window to the threshold as a zero, for a measure whose absence is itself a number. NOTIFY treats the silence as the incident and pages on the gap and again on its end. Whatever the mode, a gap nobody was paged for that returns breaching is a fresh breach and is never held back by renotify.
The conditional update is the mutex: two schedulers reading the same due row both attempt it and only the one whose claim token still holds takes the rule. The scan is wider than the budget and dealt out one rule per project at a time, so a single tenant's backlog cannot take a whole tick with it. An emission is compensated by what it wrote, not by the claim token. Every tick rewrites lastClaimedAt on every ACTIVE rule, so the token a queued notification carried went stale a minute after it was minted while the delivery budget runs for about thirty minutes. The revert therefore matched no rows on exactly the path it exists for, leaving a breach recorded as paged and, with renotify off, never announced. It now matches on the severity and alertedAt the emission wrote, which nothing but a later emission or evaluation moves. The completion write closes the same gap from the other side. A compensation deliberately leaves the claim token alone, so the token cannot see one: a rollback landing while its tick was still waiting on the evaluator was written straight back over by a transition decided from the state that rollback had just retracted, and with renotify off nobody was ever paged for that breach at all. The write now matches on the alertedAt the evaluation read as well. On that field and not the severity, because a renotify emission and its rollback move only alertedAt, and because the severity is parsed rather than read back: a spelling this build does not know would wedge the rule instead of failing one write. A row that cannot be parsed leaves its reason on the row, like every other path that gives up on a run. Its nextRunAt still advances, so it is read and discarded once a minute for as long as it exists, while the owner sees a severity that will never move, an empty error column and no sign the rule cannot fire. A notify outcome can report on an emission the rule has already replaced, and those settle out of order: a job that gives up half an hour late lands behind the page that replaced it. Such an outcome is recorded as SUPERSEDED, and only if nothing has been recorded since the emission it describes, so a page that did deliver cannot be made to read as undelivered by a straggler behind it.
The single internal router had grown to cover auth, ingest, usage, detectors and now alerts. Each surface gets its own module behind the same package name, so the alert evaluation endpoint lands beside its peers instead of extending a file that already did too much. The detector endpoint fixture follows the package the router now lives in. (cherry picked from commit e02a735b5e4fbc07e02c8b7c8527e735d3d1a99d)
The scheduler runs in the Node worker, which has no ClickHouse client, so the measurement it compares a threshold against has to be taken here. It is taken through the widget query service, which is what makes the evaluated number the same number the builder drew. The window width and how far behind the clock it may end are capped in the schema rather than at the caller, so a rule carrying a window past the ceiling fails loudly instead of quietly reading more history than the plan grants. An unknown view or measure is reported as that one alert's error, so one bad rule does not cost the batch it shares a request with. This lands before the router that serves it: the endpoint imports this module, so the reverse order would leave a commit whose rest.main cannot be imported. (cherry picked from commit 056ba94cbac00b7e43a25dfe98d8a6c8d26e9198)
Delivery is a queued job rather than part of the tick, so a slow or rate-limited Slack cannot hold up the evaluation of every other rule, and a failed send is retried on its own budget. A rule paused or deleted while its notification sat queued no longer pages. The job re-checked the project but never the rule, so a rule paused during a Slack backoff still paged minutes later and a deleted one shipped a message whose deep link 404s. Which of those a non-delivery is decides what happens to the rule's state. A failure a later emission could get past gives the page back, so the next tick raises the breach afresh. A permanent one does not: rolling back restores exactly the severity the state machine reads as a fresh breach, so a rule whose Slack is misconfigured emitted, rolled back and re-emitted every minute for as long as the breach lasted, two row writes and a job a minute, and never a page. Saving a rule without connecting Slack is enough to reach that. Those are recorded the way a paused rule is: the outcome and its reason land on the row, the severity stays where the evaluation put it, and the rule reads as in ALERT with a notification that names the setting to fix. Nor does a job send for an emission a later evaluation has replaced. Attempts run out to half an hour, so a recovery still retrying when the breach returns would otherwise leave "recovered" as the channel's last word on a rule in ALERT. It is also what makes the scheduler's kill switch safe to cycle: the queue has no TTL, so a restart replays jobs against windows hours old. (cherry picked from commit 0d82e511b22060d840a3a16128a8e01871c24895)
The form's rule and the widget spec it previews are derived from one model, so the chart the user reads and the rule that gets saved cannot describe different questions. The measure documentation is data rather than prose scattered through the panels, and the capacity helper reads the same per-project ceiling the API enforces so the form can say what is left before a create is refused. (cherry picked from commit 0544c6badf6126baf7b3211c0f1ace902d983846)
The scheduler shares the detector worker and runs one tick a minute: claim what is due, group the claims into evaluation requests, apply the state machine and queue whatever has to be said. Edges come from the floored minute boundary rather than from each rule's own clock, so two rules in one tick compare identical windows against the same data. An unreadable ALERTS_SCHEDULER_ENABLED turns alerting off. Unset still means on, but a value that is set and unreadable is a typo in a deliberate act, and the only reason to touch this switch mid-incident is to stop the paging. It is a log rather than a throw because it is read at boot beside three unrelated workers. The prod compose file passes the switch through, which is what makes it settable at all: without that line an operator has no way to stop the paging. The environment sample deliberately does not list it. A saved rule is active the moment it is saved, and a sample listing the switch reads as a step somebody has to take before alerting works. It is a kill switch, not setup, so it lives where it is reached from rather than where a project is configured. The tick reads each rule's own reading of an empty window, so a rule that asked for the silence to page, or for the empty window to count as zero, gets that instead of the default. The tick re-reads the switch rather than holding the value it booted with. Read once at boot, stopping the paging meant restarting the worker, which takes the three detector consumers with it — a poor thing to ask of an operator who reached for a switch whose stated purpose is to stop paging mid-incident. Two limits are stated in the comments rather than left to be found at the time: the value still comes from the process environment, so under compose it changes only when the container is recreated; and notifications already queued keep delivering on their own retry budget, because the delivery consumer is gated at boot alone. The completion is handed the state its transition was decided from, so a delivery that gave up while the evaluator was answering is not overwritten by the result of the tick it raced. (cherry picked from commit 6e72ff0374976de978bf1133c9b6d33f7cddfdba)
node-cron's stop() is only clearTimeout, so the handle could not see a tick already running: shutdown proceeded through prisma.$disconnect() and process.exit() while the tick was still writing. A process that died after completeAlertEvaluation committed but before the enqueue left the row reading ALERT with alertedAt set, and the restarted worker's next evaluation saw an outstanding page and suppressed the first notification of the incident for good. The scheduler now returns a handle whose waitForIdle(timeoutMs) resolves when the in-flight tick settles, and shutdown drains it before any of the closes. The wait is bounded at 5s: a tick can legitimately run tens of seconds while compose's default stop grace is 10s, so waiting it out only converts a clean exit into a SIGKILL. A callback node-cron already dispatched before stop() now also returns without claiming.
…he preview source map
The form is three questions - what to measure, when it counts as wrong, and who hears about it - so it is three panels rather than one long column. Each owns its own controls and reads the shared filter controls at a compact size, which keeps the form's layout out of the widget builder's way. (cherry picked from commit 4611c48bc45161843e22aa01eaee2d029cf9bbd9)
The threshold is a number, and a number is only meaningful next to the series it is drawn against. The preview runs the rule's own measure through the widget query the dashboards draw with, so a user picks a threshold against the same history the evaluator will compare to. (cherry picked from commit 7b204ed7cca5daa426890f25a178db7562ecbbe7)
evaluate_alerts ran a serial list comprehension of up to two ClickHouse queries per alert — a 25-alert chunk was up to 50 serial queries against the worker's 30s request abort and the 10s per-query server cap, a stable failure that landed hardest on the highest-volume projects. Two halves: - compile_widget_query grows include_row_count (number display only): the count(*) sentinel becomes 'count() AS row_count' in the metric's own SELECT, over exactly the same FROM/WHERE — halving what evaluation asks of ClickHouse and making measure/count divergence structurally impossible. - the per-alert loop runs on a ThreadPoolExecutor (13 workers: a full 25-alert chunk is at most two 10s waves, inside the 30s budget with margin). The shared client is sessionless and pooled, so overlapping queries are its normal operating mode; map preserves request order. Tests updated to the one-query shape; a Barrier test pins the overlap so a regression to serial evaluation fails loudly rather than slowly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JcKGf8Zx34mmyFP8yMBrQX
Evaluation now issues one two-column query per alert (see the measure-service layer); the single-column canned result read as a query failure here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JcKGf8Zx34mmyFP8yMBrQX
…ault docker-compose.prod.yml already carries ALERTS_SCHEDULER_ENABLED, but Helm is what runs production and the chart never set it — with the code defaulting on, the first deploy after the alerts stack merges would have started paging every workspace with a rule, with no lever to stop it. detector.alertsSchedulerEnabled ships "false" so alerting is turned on deliberately; flipping it back off mid-incident stops the paging without a rollback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JcKGf8Zx34mmyFP8yMBrQX
AlertRecord requires it; vitest does not typecheck so CI stayed green, but tsc --noEmit flagged the fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JcKGf8Zx34mmyFP8yMBrQX
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JcKGf8Zx34mmyFP8yMBrQX
![trident-sentinel[bot]](https://avatars.githubusercontent.com/in/3681545?s=60&v=4){kind=small}
Comment on lines
+322
to
+363
| const sendable = await checkAlertStillSendable(payload); | ||
| if (!sendable.ok) { | ||
| logInfo(`slack skip ${tag} reason=${sendable.outcome.error}`); | ||
| await recordAlertNotifyOutcome(sendable.outcome); | ||
| return; | ||
| } | ||
|
|
||
| const resolution = await resolveAlertChannel(payload.projectId); | ||
| if (!resolution.ok) { | ||
| logInfo(`slack skip ${tag} reason=${resolution.reason}`); | ||
| await recordNonDelivery(payload, resolution.reason); | ||
| return; | ||
| } | ||
|
|
||
| let botToken: string; | ||
| try { | ||
| botToken = decryptKey(resolution.target.encryptedBotToken); | ||
| } catch (error) { | ||
| logError(`slack skip ${tag} reason=bot-token-undecryptable`, error); | ||
| await recordNonDelivery(payload, "bot-token-undecryptable"); | ||
| return; | ||
| } | ||
|
|
||
| const message = buildAlertBlocks({ | ||
| appBaseUrl: APP_BASE_URL, | ||
| projectId: payload.projectId, | ||
| alertId: payload.alertId, | ||
| name: payload.name, | ||
| severity: payload.severity, | ||
| previousSeverity: payload.previousSeverity, | ||
| value: payload.value, | ||
| threshold: payload.threshold, | ||
| thresholdOperator: payload.thresholdOperator, | ||
| measure: payload.measure, | ||
| aggregation: payload.aggregation, | ||
| window: payload.window, | ||
| windowStart: new Date(payload.windowStart), | ||
| windowEnd: new Date(payload.windowEnd), | ||
| }); | ||
|
|
||
| try { | ||
| await createSlackClient(botToken).chat.postMessage({ |
There was a problem hiding this comment.
Low: Pause race sends stale notifications
sendAlertNotification checks checkAlertStillSendable at line 322, then resolves the channel and calls Slack postMessage at line 363 without revalidating the alert state. An authenticated project member can pause a rule after the check but before the send, for example with PATCH /api/projects/p1/alerts/a1/pause and body {"status":"PAUSED"}, while a queued retry is being processed. The worker still sends the alert value and threshold to the configured Slack channel after the user disabled the rule, bypassing the pause control and potentially disclosing stale alert data. Make notification delivery use a revocable emission or state version that is checked atomically with pause, or otherwise cancel and invalidate queued emissions when the rule is paused.
PR overviewThis pull request adds alert rules, evaluation, scheduling, Slack delivery, and UI support while resetting stale evaluation and notification state when rules change. One security concern remains open: an authenticated project member can pause an alert during delivery and still receive its queued Slack notification. Exploitation requires precise timing and access to pause the alert; the impact is limited to stale alert delivery. Open issues (1)
Scanned with Checkov · TruffleHog · Trident review. Semgrep failed to run. View in Trident Fixed/addressed: 0 · PR risk: 4/10 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
38 issues found across 129 files
Confidence score: 2/5
- The alert compensation paths in
frontend/worker/src/alerts/scheduler.ts,frontend/worker/src/alerts/claim.ts,frontend/worker/src/alerts/emission.ts, andfrontend/worker/src/notifications/alert-slack.tscan roll back state after Slack has delivered a page, causing duplicate alerts on a later tick. Make each rollback verify the notification outcome and timestamp with a CAS before reverting. - The stale-state CAS in
frontend/worker/src/alerts/claim.tsand the reset race infrontend/ui/src/app/api/projects/[projectId]/alerts/rule-state.tscan overwrite edits or repopulate cleared notification history. Add a generation/version or equivalent state-sensitive CAS covering rule changes and resets. - The billing predicate in
backend/rest/routers/internal/usage.pycan count a retried trace, span, or detector run twice when the retry crosses a billing-period boundary. Deduplicate replacement chains before applying period filters. frontend/worker/src/detector-main.tsstarts the BullMQ worker without anerrorlistener, so Redis or worker connection failures may stop alert consumption or become unhandled process errors. Attach explicit worker error handling and recovery behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/worker/src/alerts/emission.ts">
<violation number="1" location="frontend/worker/src/alerts/emission.ts:13">
P1: When a queue add times out after BullMQ has accepted the job, the worker can record `DELIVERED` before this compensation runs. This call can still restore the prior state because its CAS ignores notification outcomes, causing the next tick to re-emit a page that was already delivered; guard the rollback on a notification outcome predicate newer than `emittedAt`.
(Based on your team's feedback about guarding alert rollback after delivery.) [dda42ea7-335e-4286-b4db-a62f2c67b83a]</violation>
</file>
<file name="frontend/ui/src/features/alerts/components/alerts-onboarding.tsx">
<violation number="1" location="frontend/ui/src/features/alerts/components/alerts-onboarding.tsx:20">
P2: When `useProject` fails, this component silently removes the only Slack setup link while still exposing `New Alert`. Read `isError` from `useProject` and render an explicit reload/error state instead of treating the failure as an ordinary pre-workspace state.
(Based on your team's feedback about preserving explicit project and integration query error states.)</violation>
<violation number="2" location="frontend/ui/src/features/alerts/components/alerts-onboarding.tsx:22">
P2: While the Slack status request is loading or has failed, this code renders `Connect Slack` because `slack` is undefined. Keep a loading placeholder and explicit error branch, and show the connect link only after a successful status response.
(Based on your team's feedback about preserving the Slack status loading guard and explicit error branch.)</violation>
</file>
<file name="frontend/ui/src/features/dashboards/types.ts">
<violation number="1" location="frontend/ui/src/features/dashboards/types.ts:134">
P2: When an explicit bucket is used, the backend returns a numeric millisecond width, but the chart formatter does not interpret numeric granularity. Format numeric widths according to their precision (including seconds), or normalize them to a supported granularity before rendering.</violation>
</file>
<file name="frontend/ui/src/app/api/projects/[projectId]/alerts/route.test.ts">
<violation number="1" location="frontend/ui/src/app/api/projects/[projectId]/alerts/route.test.ts:64">
P2: The alertFindMany mock ignores the caller's `orderBy` and returns rows in store insertion order, while the GET route orders by `createTime: "asc"`. Because the mock silently drops ordering, a future change to the route's ordering (or its loss) is undetectable to these tests. Apply the passed `orderBy` in the mock so ordering regressions surface.</violation>
</file>
<file name="frontend/ui/src/features/alerts/components/notifications-section.tsx">
<violation number="1" location="frontend/ui/src/features/alerts/components/notifications-section.tsx:54">
P2: When the Slack-status request fails, this branch renders `SlackIntegrationLink` with `isConnected={false}`, falsely showing “Connect Slack.” Destructure `isError` from `useSlackStatus` and render an error state before the link.</violation>
</file>
<file name="backend/rest/routers/internal/ingest.py">
<violation number="1" location="backend/rest/routers/internal/ingest.py:121">
P2: Each detector trace request blocks the FastAPI event loop during client initialization and both ClickHouse inserts, delaying unrelated requests while ClickHouse is slow. Run the synchronous client operations in a worker thread before awaiting them.</violation>
</file>
<file name="frontend/ui/src/features/alerts/hooks/use-alerts.ts">
<violation number="1" location="frontend/ui/src/features/alerts/hooks/use-alerts.ts:174">
P2: When `projectId` changes, this placeholder displays the previous project's alerts under the new project and routes their actions through the new `projectId`. Use `isPlaceholderData` in `AlertsPage` to dim or withhold placeholder rows during project transitions.
(Based on your team's feedback about preserving shared list placeholders.)</violation>
</file>
<file name="frontend/ui/src/features/alerts/components/alert-display.ts">
<violation number="1" location="frontend/ui/src/features/alerts/components/alert-display.ts:90">
P2: When `lastNotifyStatus` is `SUPERSEDED`, the worker skipped an obsolete page because a newer evaluation replaced it and did not attempt rollback. This branch says rollback failed, so the badge gives a false explanation; handle `SUPERSEDED` separately with a replacement message.</violation>
</file>
<file name="frontend/ui/src/app/api/projects/[projectId]/alerts/rule-state.ts">
<violation number="1" location="frontend/ui/src/app/api/projects/[projectId]/alerts/rule-state.ts:37">
P2: When a rule reset races with an in-flight notification job, `recordAlertNotifyOutcome` can repopulate the cleared notification history with the old rule’s result. Add a reset generation or equivalent CAS that notification outcomes must match before writing `lastNotify*`.</violation>
</file>
<file name="frontend/ui/src/app/api/projects/[projectId]/alerts/route.ts">
<violation number="1" location="frontend/ui/src/app/api/projects/[projectId]/alerts/route.ts:86">
P2: When multiple members create alerts concurrently near the limit, each request can pass this count before any request creates its row, bypassing the 100-alert cap and potentially adding unbounded scheduler load. Enforce the capacity check and reservation atomically, for example with a per-project lock or transactional counter.</violation>
</file>
<file name="frontend/ui/src/app/api/projects/[projectId]/alerts/[alertId]/pause/route.ts">
<violation number="1" location="frontend/ui/src/app/api/projects/[projectId]/alerts/[alertId]/pause/route.ts:32">
P2: When a pre-reset notification job is still in flight, clearing these fields does not persist: `recordAlertNotifyOutcome` can write the old delivery result back by alert ID after this update. Carry a reset generation/timestamp into delivery outcomes and reject outcomes from before the reset.</violation>
</file>
<file name="frontend/ui/src/features/alerts/components/alert-form.tsx">
<violation number="1" location="frontend/ui/src/features/alerts/components/alert-form.tsx:122">
P2: When an alert uses a non-default no-data mode, this form cannot configure or change it because the draft and submitted rule omit `noDataMode`. Carry the stored mode through the draft and render/send a mode control so alert edits can change no-data behavior.
(Based on your team's feedback about aligning alert no-data types.)</violation>
</file>
<file name="frontend/worker/src/detector-main.ts">
<violation number="1" location="frontend/worker/src/detector-main.ts:46">
P2: When shutdown arrives during an active Slack delivery, `alertNotificationWorker.close()` can wait beyond the 10-second stop grace because the notification worker has no bounded close. Bound or force-close this worker so a stalled Slack request cannot turn graceful shutdown into SIGKILL.</violation>
<violation number="2" location="frontend/worker/src/detector-main.ts:104">
P1: When alerting is enabled, this starts a BullMQ worker without an `error` listener. A Redis or worker connection error can therefore stop the alert consumer or become an unhandled process error; attach an error handler in `startAlertNotificationWorker` before enabling it here.</violation>
</file>
<file name="frontend/worker/src/alerts/scheduler.ts">
<violation number="1" location="frontend/worker/src/alerts/scheduler.ts:206">
P1: When Redis accepts a notification but the enqueue timeout still rejects, this unconditional rollback can run after Slack delivers the page. Guard the compensation with the notification outcome/CAS before reverting, otherwise the delivered page is erased and the breach can be re-emitted on a later tick.
(Based on your team's feedback about guarding alert rollback after delivery.)</violation>
</file>
<file name="frontend/ui/src/app/api/projects/[projectId]/alerts/schema.ts">
<violation number="1" location="frontend/ui/src/app/api/projects/[projectId]/alerts/schema.ts:47">
P2: Metadata keys between 129 and 256 characters are valid to the backend evaluator but this schema rejects them. Use the backend key limit for keyed filters (or a shared alert/backend constant) instead of reusing the 128-character token limit.</violation>
</file>
<file name="frontend/ui/src/components/search-filter-bar.test.tsx">
<violation number="1" location="frontend/ui/src/components/search-filter-bar.test.tsx:35">
P3: This new test duplicates the existing 'omits the date filter entirely when the caller has no time range to filter by' test a few lines below. Both render SearchFilterBar with no dateFilter and assert the date filter is not shown, so the same behavior is covered twice. Remove this test and keep the existing, more thorough one (or fold the no-button assertion into it).</violation>
</file>
<file name="frontend/ui/src/features/alerts/components/delete-alert-dialog.tsx">
<violation number="1" location="frontend/ui/src/features/alerts/components/delete-alert-dialog.tsx:34">
P2: While `isDeleting` is true, Escape, overlay clicks, and the still-enabled Cancel button close the dialog while deletion continues. Guard `handleClose` during deletion so a failed in-flight request cannot leave the user without feedback.</violation>
<violation number="2" location="frontend/ui/src/features/alerts/components/delete-alert-dialog.tsx:63">
P2: After a failed delete, reopening this dialog for another alert renders the previous `deleteMutation.error` because the page keeps the mutation instance and never resets it. Clear the mutation error when closing or opening a new delete attempt, or scope the displayed error to the current attempt.</violation>
</file>
<file name="backend/rest/routers/internal/usage.py">
<violation number="1" location="backend/rest/routers/internal/usage.py:99">
P2: `get_usage_details` is async but performs three blocking `ClickHouseClient.query` calls directly. Execute each synchronous read with `asyncio.to_thread` or use an asynchronous ClickHouse client so slow billing scans cannot stall unrelated requests.</violation>
<violation number="2" location="backend/rest/routers/internal/usage.py:142">
P1: When a trace, span, or detector run is retried across a billing-period boundary, this predicate filters the superseded and replacement rows separately, so the same logical event is billed twice. Collapse each ReplacingMergeTree key to its canonical row before applying the interval filter.</violation>
</file>
<file name="frontend/ui/src/app/projects/[projectId]/alerts/page.tsx">
<violation number="1" location="frontend/ui/src/app/projects/[projectId]/alerts/page.tsx:34">
P1: When navigating between projects, this page renders the previous project's alerts while the new list request is pending. Read `isPlaceholderData` and dim or disable the list until the new response arrives, so users cannot act on rows from the wrong project.
(Based on your team's feedback about preserving previous-data placeholders during project transitions.) [9ec1092d-399c-46e7-a6e3-7b9438d9a0e0]</violation>
</file>
<file name="frontend/worker/src/alerts/claim.ts">
<violation number="1" location="frontend/worker/src/alerts/claim.ts:45">
P1: When an active rule with `lastClaimedAt = null` is edited between `findMany` and this update, this CAS still matches because the reset also writes `lastClaimedAt: null`. It can evaluate the stale rule and overwrite the reset when both old and reset `alertedAt` values are null; include an update timestamp or version in the claim and completion CAS.</violation>
<violation number="2" location="frontend/worker/src/alerts/claim.ts:114">
P2: When more than `ALERT_CLAIM_SCAN_LIMIT` due rows precede a project, that project never reaches `shareBudgetAcrossProjects` despite available fair-budget capacity. Replenished due rows can repeatedly delay later projects; fetch bounded per-project slices or paginate the due scan before applying the global budget.
(Based on your team's feedback about preventing alert-rule starvation.) .</violation>
<violation number="3" location="frontend/worker/src/alerts/claim.ts:242">
P1: When enqueue compensation races a notification that already delivered, this rollback can still restore the pre-emission state because its CAS ignores notification outcomes. Include the delivery status and timestamp for this emission in the CAS, and skip rollback after a delivered outcome.
(Based on your team's feedback about guarding alert rollback after delivery.) .</violation>
</file>
<file name="frontend/ui/src/features/alerts/components/edit-alert-page.test.tsx">
<violation number="1" location="frontend/ui/src/features/alerts/components/edit-alert-page.test.tsx:86">
P2: The test's own comment calls `isAlertGone` "the classification under test", but the 403 role-denial branch (`Requires X role or higher`) is never exercised. That is the distinguishing case: a role denial must render "Alert could not be edited", not "Alert not found". Add a case with `new ApiError(403, "Requires ADMIN role or higher")` asserting it is treated as a server fault, and keep the generic 403 revoked-access case separate.</violation>
</file>
<file name="frontend/ui/src/features/alerts/components/alert-form.test.tsx">
<violation number="1" location="frontend/ui/src/features/alerts/components/alert-form.test.tsx:1">
P3: This test file is unrelated to the PR's stated change. The PR only modifies the backend `alertStateReset()` to clear `lastError`/`lastNotify*` columns, yet this diff adds a large frontend fixture for `AlertForm` (form state, recharts, fetch stubs, capacity gating). Keeping unrelated files in a bugfix PR bloats the diff and makes the backend change harder to review/revert. Move this test into the PR that introduced the alert form, or split it out of this change.</violation>
</file>
<file name="frontend/ui/src/features/dashboards/components/WidgetBuilderPage.tsx">
<violation number="1" location="frontend/ui/src/features/dashboards/components/WidgetBuilderPage.tsx:136">
P2: When editing a previously saved widget that references a field now marked `inBuilder: false`, this predicate removes its current field from the dropdown. Keep currently referenced metric, breakdown, and filter fields in the options while excluding them from new drafts, so legacy widgets remain understandable and editable.</violation>
</file>
<file name="frontend/ui/src/features/dashboards/components/renderers.additive.test.tsx">
<violation number="1" location="frontend/ui/src/features/dashboards/components/renderers.additive.test.tsx:37">
P3: The test description "calls exactly the aggregations the backend treats as non-additive a gap" is garbled — the test filters AGGS rather than calling anything, and "a gap" is a leftover fragment. Reword it so future readers can tell what the test asserts, e.g. "matches the backend's non-additive agg set exactly".</violation>
</file>
<file name="frontend/worker/src/alerts/__tests__/rule.test.ts">
<violation number="1" location="frontend/worker/src/alerts/__tests__/rule.test.ts:58">
P2: These new assertions pin parseAlertRule to drop the entire alert (return null) for an incompatible aggregation like "median" and for non-scalar/malformed stored filters. The team's established direction is to keep such rules evaluable and let the backend evaluator report the problem per alert instead of rejecting the whole rule locally. If the evaluator-per-alert fix lands in rule.ts, these assertions will need to be rewritten (and until then they cement the dropping behavior and silently starve the affected rule). Route these inputs to the evaluator rather than asserting local null returns.</violation>
</file>
<file name="frontend/worker/src/notifications/alert-slack.ts">
<violation number="1" location="frontend/worker/src/notifications/alert-slack.ts:146">
P2: When an older queued job arrives after a newer page delivered and the rule was paused, these branches overwrite the newer `DELIVERED` status because they omit the emission timestamp guard. Pass `payload.emission?.evaluatedAt` as `notAfter` for paused/deleted outcomes.</violation>
<violation number="2" location="frontend/worker/src/notifications/alert-slack.ts:406">
P1: When an enqueue acknowledgement or delivery races with compensation, this rollback can restore the pre-emission state after Slack has delivered the page, so the next tick emits a duplicate. Guard `revertAlertEmissionState` with the notification outcome columns and skip rollback once this emission has delivered.</violation>
</file>
<file name="frontend/worker/src/alerts/concurrency.ts">
<violation number="1" location="frontend/worker/src/alerts/concurrency.ts:9">
P2: When `limit` is `NaN`, `Math.floor(limit)` keeps it `NaN`, so the helper creates zero workers and silently skips every item. Normalize or reject `NaN` before deriving the worker width.</violation>
</file>
<file name="frontend/ui/src/features/alerts/components/alert-preview.tsx">
<violation number="1" location="frontend/ui/src/features/alerts/components/alert-preview.tsx:68">
P2: When a custom end precedes its start, this returns an inverted range and the preview silently replaces it with one minute after the start. Reject invalid custom bounds before applying them so the chart cannot show data unrelated to the range the control displays.</violation>
<violation number="2" location="frontend/ui/src/features/alerts/components/alert-preview.tsx:262">
P2: When the selected range exceeds 500 rule buckets, this passes no bucket to the server, which switches to hourly or daily buckets. The preview then shows aggregates at a grain the alert never evaluates; show a too-wide-range state or constrain the range instead of silently changing the rule's grain.</violation>
</file>
<file name="frontend/ui/src/app/api/projects/[projectId]/alerts/[alertId]/route.ts">
<violation number="1" location="frontend/ui/src/app/api/projects/[projectId]/alerts/[alertId]/route.ts:111">
P2: When concurrent PATCHes edit different query fields, both validate against the same old rule, then this unconditioned write combines their fields without revalidating. A valid measure edit and filters edit can persist an unevaluable rule; make validation and mutation atomic or reject stale writes.</violation>
</file>
<file name="frontend/packages/slack/src/alert-blocks.ts">
<violation number="1" location="frontend/packages/slack/src/alert-blocks.ts:76">
P2: When an alert evaluates a small nonzero value, `formatNumber` rounds it to `0`, misreporting the observation and potentially displaying `0 above the 0 threshold` for a real breach. Preserve sub-cent/sub-unit values when two-decimal rounding would produce zero.</violation>
</file>
Architecture diagram
sequenceDiagram
participant UI as Alert UI (Next.js)
participant API as API Routes (alerts CRUD)
participant DB as Prisma/Postgres (alerts table)
participant Worker as Node Worker (Scheduler)
participant REST as FastAPI (Internal)
participant CH as ClickHouse
participant Slack as Slack
Note over UI,DB: Alert Rule Management
UI->>API: POST/PATCH/PUT /alerts
API->>API: Validate + canonicalize filters
API->>DB: Create/Update alert rule
API->>DB: Check rule changed (rule snapshot)
alt Rule changed (not just name)
API->>DB: alertStateReset(): clear severity, clocks, lastError, lastNotify*
DB-->>API: state cleared
end
API-->>UI: Serialized alert
Note over Worker,Slack: Evaluation & Notification
loop Every minute (if ALERTS_SCHEDULER_ENABLED)
Worker->>Worker: Claim due alerts (CAS on lastClaimedAt)
Worker->>REST: POST /internal/alert-evaluate (X-Internal-Secret)
REST->>REST: Verify secret (fails closed if unset)
REST->>CH: Run widget queries (parallel, max 13)
CH-->>REST: Scalar values + row counts
REST-->>Worker: AlertEvaluationResult per rule
Worker->>DB: Update severity, evaluatedAt, lastError
alt Threshold breached AND notify needed
Worker->>Slack: Send notification (retry budget)
Slack-->>Worker: Success/Failure
Worker->>DB: Update lastNotifyStatus, lastNotifyError
end
end
Note over API,DB: Pause/Resume
UI->>API: PATCH /pause
API->>DB: Update status + alertStateReset() (ACTIVE only)
DB-->>API: Updated alert
API-->>UI: Serialized alert
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| projectId: string, | ||
| reason: string, | ||
| ): Promise<boolean> { | ||
| const reverted = await revertAlertEmissionState({ |
Contributor
There was a problem hiding this comment.
P1: When a queue add times out after BullMQ has accepted the job, the worker can record DELIVERED before this compensation runs. This call can still restore the prior state because its CAS ignores notification outcomes, causing the next tick to re-emit a page that was already delivered; guard the rollback on a notification outcome predicate newer than emittedAt.
(Based on your team's feedback about guarding alert rollback after delivery.) [dda42ea7-335e-4286-b4db-a62f2c67b83a]
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/worker/src/alerts/emission.ts, line 13:
<comment>When a queue add times out after BullMQ has accepted the job, the worker can record `DELIVERED` before this compensation runs. This call can still restore the prior state because its CAS ignores notification outcomes, causing the next tick to re-emit a page that was already delivered; guard the rollback on a notification outcome predicate newer than `emittedAt`.
(Based on your team's feedback about guarding alert rollback after delivery.) [dda42ea7-335e-4286-b4db-a62f2c67b83a]</comment>
<file context>
@@ -0,0 +1,24 @@
+ projectId: string,
+ reason: string,
+): Promise<boolean> {
+ const reverted = await revertAlertEmissionState({
+ ...emission,
+ error: { message: `notification not delivered (${reason})`, at: new Date() },
</file context>
| // already queued — bounded by each job's retry budget, and by the staleness | ||
| // check that drops a job whose emission the rule has since moved past. | ||
| if (isAlertsSchedulerEnabled()) { | ||
| alertNotificationWorker = startAlertNotificationWorker(); |
Contributor
There was a problem hiding this comment.
P1: When alerting is enabled, this starts a BullMQ worker without an error listener. A Redis or worker connection error can therefore stop the alert consumer or become an unhandled process error; attach an error handler in startAlertNotificationWorker before enabling it here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/worker/src/detector-main.ts, line 104:
<comment>When alerting is enabled, this starts a BullMQ worker without an `error` listener. A Redis or worker connection error can therefore stop the alert consumer or become an unhandled process error; attach an error handler in `startAlertNotificationWorker` before enabling it here.</comment>
<file context>
@@ -73,6 +93,20 @@ async function main(): Promise<void> {
+ // already queued — bounded by each job's retry budget, and by the staleness
+ // check that drops a job whose emission the rule has since moved past.
+ if (isAlertsSchedulerEnabled()) {
+ alertNotificationWorker = startAlertNotificationWorker();
+ alertScheduler = startAlertScheduler();
+ } else {
</file context>
| `notification enqueue failed, reverting state alert=${rule.id} project=${rule.projectId} severity=${severity}`, | ||
| error, | ||
| ); | ||
| await revertAlertEmission( |
Contributor
There was a problem hiding this comment.
P1: When Redis accepts a notification but the enqueue timeout still rejects, this unconditional rollback can run after Slack delivers the page. Guard the compensation with the notification outcome/CAS before reverting, otherwise the delivered page is erased and the breach can be re-emitted on a later tick.
(Based on your team's feedback about guarding alert rollback after delivery.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/worker/src/alerts/scheduler.ts, line 206:
<comment>When Redis accepts a notification but the enqueue timeout still rejects, this unconditional rollback can run after Slack delivers the page. Guard the compensation with the notification outcome/CAS before reverting, otherwise the delivered page is erased and the breach can be re-emitted on a later tick.
(Based on your team's feedback about guarding alert rollback after delivery.) </comment>
<file context>
@@ -0,0 +1,402 @@
+ `notification enqueue failed, reverting state alert=${rule.id} project=${rule.projectId} severity=${severity}`,
+ error,
+ );
+ await revertAlertEmission(
+ {
+ alertId: rule.id,
</file context>
| SELECT uniqExact(run_id) as total | ||
| FROM detector_runs | ||
| WHERE project_id IN {project_ids:Array(String)} | ||
| AND timestamp >= {start:String} |
Contributor
There was a problem hiding this comment.
P1: When a trace, span, or detector run is retried across a billing-period boundary, this predicate filters the superseded and replacement rows separately, so the same logical event is billed twice. Collapse each ReplacingMergeTree key to its canonical row before applying the interval filter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/rest/routers/internal/usage.py, line 142:
<comment>When a trace, span, or detector run is retried across a billing-period boundary, this predicate filters the superseded and replacement rows separately, so the same logical event is billed twice. Collapse each ReplacingMergeTree key to its canonical row before applying the interval filter.</comment>
<file context>
@@ -0,0 +1,155 @@
+ SELECT uniqExact(run_id) as total
+ FROM detector_runs
+ WHERE project_id IN {project_ids:Array(String)}
+ AND timestamp >= {start:String}
+ AND timestamp < {end:String}
+ """,
</file context>
|
|
||
| const { state, queryOptions, updateKeyword, updateLimit, goToPage } = useListPageState(); | ||
|
|
||
| const { data, isLoading, error } = useAlertList(projectId, { |
Contributor
There was a problem hiding this comment.
P1: When navigating between projects, this page renders the previous project's alerts while the new list request is pending. Read isPlaceholderData and dim or disable the list until the new response arrives, so users cannot act on rows from the wrong project.
(Based on your team's feedback about preserving previous-data placeholders during project transitions.) [9ec1092d-399c-46e7-a6e3-7b9438d9a0e0]
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/ui/src/app/projects/[projectId]/alerts/page.tsx, line 34:
<comment>When navigating between projects, this page renders the previous project's alerts while the new list request is pending. Read `isPlaceholderData` and dim or disable the list until the new response arrives, so users cannot act on rows from the wrong project.
(Based on your team's feedback about preserving previous-data placeholders during project transitions.) [9ec1092d-399c-46e7-a6e3-7b9438d9a0e0]</comment>
<file context>
@@ -0,0 +1,176 @@
+
+ const { state, queryOptions, updateKeyword, updateLimit, goToPage } = useListPageState();
+
+ const { data, isLoading, error } = useAlertList(projectId, {
+ page: queryOptions.page,
+ limit: queryOptions.limit,
</file context>
|
|
||
| // Scoped write rather than a write on `id` alone: the project scope is the | ||
| // tenancy check, so it belongs on the statement that mutates. | ||
| const { count } = await prisma.alert.updateMany({ where: { id: alertId, projectId }, data }); |
Contributor
There was a problem hiding this comment.
P2: When concurrent PATCHes edit different query fields, both validate against the same old rule, then this unconditioned write combines their fields without revalidating. A valid measure edit and filters edit can persist an unevaluable rule; make validation and mutation atomic or reject stale writes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/ui/src/app/api/projects/[projectId]/alerts/[alertId]/route.ts, line 111:
<comment>When concurrent PATCHes edit different query fields, both validate against the same old rule, then this unconditioned write combines their fields without revalidating. A valid measure edit and filters edit can persist an unevaluable rule; make validation and mutation atomic or reject stale writes.</comment>
<file context>
@@ -0,0 +1,132 @@
+
+ // Scoped write rather than a write on `id` alone: the project scope is the
+ // tenancy check, so it belongs on the statement that mutates.
+ const { count } = await prisma.alert.updateMany({ where: { id: alertId, projectId }, data });
+ if (count === 0) return errorResponse("Alert not found", 404);
+
</file context>
| function formatNumber(value: number): string { | ||
| if (!Number.isFinite(value)) return String(value); | ||
| if (Number.isInteger(value)) return String(value); | ||
| return String(Math.round(value * 100) / 100); |
Contributor
There was a problem hiding this comment.
P2: When an alert evaluates a small nonzero value, formatNumber rounds it to 0, misreporting the observation and potentially displaying 0 above the 0 threshold for a real breach. Preserve sub-cent/sub-unit values when two-decimal rounding would produce zero.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/packages/slack/src/alert-blocks.ts, line 76:
<comment>When an alert evaluates a small nonzero value, `formatNumber` rounds it to `0`, misreporting the observation and potentially displaying `0 above the 0 threshold` for a real breach. Preserve sub-cent/sub-unit values when two-decimal rounding would produce zero.</comment>
<file context>
@@ -0,0 +1,136 @@
+function formatNumber(value: number): string {
+ if (!Number.isFinite(value)) return String(value);
+ if (Number.isInteger(value)) return String(value);
+ return String(Math.round(value * 100) / 100);
+}
+
</file context>
Suggested change
| return String(Math.round(value * 100) / 100); | |
| return String(Math.round(value * 100) / 100 || value); |
| expect(screen.getByText(DATE_FILTER_OPTIONS[0].label)).toBeTruthy(); | ||
| }); | ||
|
|
||
| it("leaves out the date filter when a surface has no time range", () => { |
Contributor
There was a problem hiding this comment.
P3: This new test duplicates the existing 'omits the date filter entirely when the caller has no time range to filter by' test a few lines below. Both render SearchFilterBar with no dateFilter and assert the date filter is not shown, so the same behavior is covered twice. Remove this test and keep the existing, more thorough one (or fold the no-button assertion into it).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/ui/src/components/search-filter-bar.test.tsx, line 35:
<comment>This new test duplicates the existing 'omits the date filter entirely when the caller has no time range to filter by' test a few lines below. Both render SearchFilterBar with no dateFilter and assert the date filter is not shown, so the same behavior is covered twice. Remove this test and keep the existing, more thorough one (or fold the no-button assertion into it).</comment>
<file context>
@@ -32,6 +32,13 @@ describe("SearchFilterBar", () => {
expect(screen.getByText(DATE_FILTER_OPTIONS[0].label)).toBeTruthy();
});
+ it("leaves out the date filter when a surface has no time range", () => {
+ render(<SearchFilterBar searchValue="" onSearchChange={vi.fn()} />);
+
</file context>
| @@ -0,0 +1,425 @@ | |||
| // @vitest-environment jsdom | |||
Contributor
There was a problem hiding this comment.
P3: This test file is unrelated to the PR's stated change. The PR only modifies the backend alertStateReset() to clear lastError/lastNotify* columns, yet this diff adds a large frontend fixture for AlertForm (form state, recharts, fetch stubs, capacity gating). Keeping unrelated files in a bugfix PR bloats the diff and makes the backend change harder to review/revert. Move this test into the PR that introduced the alert form, or split it out of this change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/ui/src/features/alerts/components/alert-form.test.tsx, line 1:
<comment>This test file is unrelated to the PR's stated change. The PR only modifies the backend `alertStateReset()` to clear `lastError`/`lastNotify*` columns, yet this diff adds a large frontend fixture for `AlertForm` (form state, recharts, fetch stubs, capacity gating). Keeping unrelated files in a bugfix PR bloats the diff and makes the backend change harder to review/revert. Move this test into the PR that introduced the alert form, or split it out of this change.</comment>
<file context>
@@ -0,0 +1,425 @@
+// @vitest-environment jsdom
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { render, cleanup, screen, fireEvent, waitFor } from "@testing-library/react";
</file context>
| const BACKEND_NON_ADDITIVE = ["avg", "min", "max", "p50", "p75", "p90", "p95", "p99"]; | ||
|
|
||
| describe("isAdditiveAgg", () => { | ||
| it("calls exactly the aggregations the backend treats as non-additive a gap", () => { |
Contributor
There was a problem hiding this comment.
P3: The test description "calls exactly the aggregations the backend treats as non-additive a gap" is garbled — the test filters AGGS rather than calling anything, and "a gap" is a leftover fragment. Reword it so future readers can tell what the test asserts, e.g. "matches the backend's non-additive agg set exactly".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/ui/src/features/dashboards/components/renderers.additive.test.tsx, line 37:
<comment>The test description "calls exactly the aggregations the backend treats as non-additive a gap" is garbled — the test filters AGGS rather than calling anything, and "a gap" is a leftover fragment. Reword it so future readers can tell what the test asserts, e.g. "matches the backend's non-additive agg set exactly".</comment>
<file context>
@@ -0,0 +1,109 @@
+const BACKEND_NON_ADDITIVE = ["avg", "min", "max", "p50", "p75", "p90", "p95", "p99"];
+
+describe("isAdditiveAgg", () => {
+ it("calls exactly the aggregations the backend treats as non-additive a gap", () => {
+ const nonAdditive = AGGS.filter((agg) => !isAdditiveAgg(agg));
+ expect([...nonAdditive].sort()).toEqual([...BACKEND_NON_ADDITIVE].sort());
</file context>
Suggested change
| it("calls exactly the aggregations the backend treats as non-additive a gap", () => { | |
| it("matches the backend's non-additive agg set exactly", () => { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2027
Summary
When an alert rule is edited or resumed,
alertStateReset()resets severity, evaluation clocks, and claim tokens to cold start defaults. However, it previously leftlastError,lastErrorAt, and thelastNotify*columns untouched.This resulted in:
"Failing: <old error>"until the next successful evaluation cycle.lastNotifyStatus = FAILED) persisted indefinitely on reset rules since delivery columns are only updated by the delivery worker.Fix
alertStateReset()by setting them tonull:lastError: nulllastErrorAt: nulllastNotifyStatus: nulllastNotifyError: nulllastNotifyAt: nullTesting
rule-state.test.tsto verify thatalertStateReset()returns all 11 reset fields set tonull/cold start defaults.pnpm --filter traceroot-ui test rule-state.test.ts(17 tests passed).pnpm --filter traceroot-ui test(236 test files / 2,278 tests passed).Before
After
Summary by cubic
Adds the alerts feature end-to-end and stops edited or resumed rules from showing stale failure state:
alertStateReset()now also clears thelastErrorandlastNotify*columns, so a rule that was failing and then fixed no longer renders"Failing: <old error>"or a failed-delivery badge until its next run.Alerts
HOLD,ZERO, orNOTIFY).p75/p90/uniqaggregations, keyed metadata filters, and explicit bucket widths;uniqseries are no longer summed in the legend.Migration
alertstable ships as one squashed migration; dev databases that applied any earlier alert migration must reset or mark20260827000000_add_alertsapplied by hand.docker-compose.prod.yml; flipalertsSchedulerEnabledto turn it on.INTERNAL_API_SECRETthat now fails closed; an unset secret rejects all requests instead of allowing anonymous writes.Written for commit e31822d. Summary will update on new commits.