Skip to content

feat(alerts): send the alert.resolved webhook - #6373

Open
claude[bot] wants to merge 1 commit into
ing-508-renewal-baselinefrom
ing-510-alert-resolved-webhook
Open

claude[bot] wants to merge 1 commit into
ing-508-renewal-baselinefrom
ing-510-alert-resolved-webhook

Conversation

@claude

@claude claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Requested by Felipe Rodrigues · Slack thread

Before: The recovery engine notices that a value has come back to the safe side and writes a resolved row, but nothing leaves Lago. A customer who blocks something when usage climbs is told about the crossing and never told about the return, so the block stays on until someone looks.

After: The same moment that records the recovery also sends alert.resolved. Only lines that asked to be watched are announced, and only when Lago has an alarm for them on record — that pairing is the one #6131 already applies before writing the row, so the webhook inherits it rather than repeating it. Every threshold that exists today notifies on triggered only, so nothing fires for anyone until they opt a line in. Seeded rows are written on a different path and never reach here.

How: delivery hangs off record_resolution in UsageMonitoring::ProcessAlertService, next to the alert.triggered call it mirrors, so the record and the send can never disagree. Webhooks::UsageMonitoring::AlertResolvedService mirrors the triggered service and is registered in SendWebhookJob::WEBHOOK_SERVICES; alert_resolved joins config/webhook_event_types.yml, which is what feeds the endpoint subscription list and the GraphQL EventTypeEnum (both schema dumps regenerated by hand — see the note below). The payload is TriggeredAlertSerializer plus the three things a resolution adds, via a subclass.

Merge order

This stacks on #6131 and must merge after it. Its base here is ing-509-recovery-engine, not main; GitHub will retarget once that lands. Nothing in the stack below changes, and #6131's own merge-order note still applies — in particular the renewal fix, since without it a period boundary can record a spurious resolution row, and this PR is what turns such a row into a delivered webhook.

Open questions for review

  • object_type stays triggered_alert. The record is a UsageMonitoring::TriggeredAlert with kind: resolved, and Lago's convention is that one object type carries several event names (invoice.created / invoice.one_off_created). Consumers separate the two on webhook_type. If you would rather the resolution get its own resolved_alert root, say so now — it is a one-line change here and a breaking one later.
  • resolved_at duplicates triggered_at. On a resolved row the triggered_at column holds the time the resolution was recorded. The payload keeps the inherited triggered_at and adds resolved_at with the same value, so a consumer reading the resolution does not have to know that. Happy to drop resolved_at if the duplication is worse than the ambiguity.
  • crossed_thresholds carries the recovered lines. Inherited from the triggered payload, where the name fits; on a resolution it lists the lines that were left behind. Renaming it would mean not reusing the serializer.
  • Ticket id is a guess. The stack runs ING-507 / ING-508 / ING-509 and nothing in the repo or in the sibling PR bodies names a ticket for webhook delivery, so the branch is ing-510-alert-resolved-webhook and the title carries no id. Please confirm the real one and I will rename.

Testing

I could not run the suite or Rubocop in this environment, so nothing here is verified by a green run. The container has Ruby 3.3.6 and the Gemfile requires 4.0.6, so bundle install refuses and neither rspec nor rubocop is installed; there is also no Postgres running. What I did instead: ruby -c on every changed Ruby file (clean, aside from the pre-existing 3.4 it warnings), a YAML parse of config/webhook_event_types.yml, and a JSON parse of schema.json. CI is the first real run — please treat a red build as expected information, not a surprise.

Two things worth a reviewer's eye because of that:

  • schema.graphql and schema.json were edited by hand rather than regenerated, since the app cannot boot here. spec/graphql/lago_api_schema_spec.rb compares both against the live schema, so it will say immediately if the hand edit is off.
  • spec/models/webhook_endpoint_spec.rb asserts WEBHOOK_EVENT_TYPES matches SendWebhookJob::WEBHOOK_SERVICES.keys, which is why the YAML entry is part of this PR and not a follow-up.

Specs added: a webhook-service spec and a serializer spec mirroring the triggered ones, and in process_alert_service_spec.rb the existing "does not send a webhook yet" example becomes an assertion that the resolution webhook is enqueued, with the not-opted-in context now also asserting nothing is sent.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RLys3y8nVyx1rgUqqXDFP2


Generated by Claude Code

@claude

claude Bot commented Sep 16, 2026

Copy link
Copy Markdown
Author

Heads-up on the red shard, not a request for anyone to act.

Run Spec (10, 7) failed on dbebf06 with one failing example: spec/services/daily_usages/compute_all_service_spec.rb:32. It is not this PR's. That spec and app/services/daily_usages/ are byte-identical here and on main, and the identical rebased base (ing-509-recovery-engine @ d722d79b) passed the same job in run 35099080808.

The assertion is order-dependent: job_wait_time is rand(scheduling_interval), so with the jitter env var at 60 a job's scheduled_at is enqueue_time + 0..59s, while the matcher's lower bound 0.seconds.from_now is evaluated after the enqueue. Any job whose jitter falls below the elapsed wall time misses the window, so the 60s context flakes at roughly elapsed/60 per subscription; the sibling contexts share the defect but hide it behind a 1800s window. The fix belongs on main — capture the time before compute_service.call and assert against that — so I have not pulled it into this PR, and no fix for it exists in the chain to port. I have re-run the job once.


Generated by Claude Code

@claude
claude Bot marked this pull request as ready for review September 17, 2026 14:59
@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: PASS · CI green

PASS — The resolution webhook is wired consistently with the existing alert webhook path, including endpoint registration and schema exposure. Focused specs cover enqueue/no-enqueue behavior, webhook creation, and partial and full resolution payloads.

@aquinofb
aquinofb requested review from mariohd and toommz September 17, 2026 17:35
Comment thread app/services/usage_monitoring/process_alert_service.rb Outdated
@aquinofb
aquinofb force-pushed the ing-510-alert-resolved-webhook branch from eceedf9 to 0d23b99 Compare September 23, 2026 15:27
@groyoh
groyoh changed the base branch from ing-509-recovery-engine to ing-508-renewal-baseline October 8, 2026 08:16
@groyoh
groyoh added this pull request to stack #6613 October 8, 2026 08:16
A threshold that opted in to resolution now emits alert.resolved when the value
comes back past it, carrying the same shape as the triggered event so a
consumer can pair the two by threshold code.

Rebuilt on the current recovery engine. The branch had repeatedly merged its
base instead of rebasing, so it carried three copies of the engine commit and
could not replay after the locking work was squashed into main; the content is
unchanged from the reviewed version.
@groyoh
groyoh force-pushed the ing-510-alert-resolved-webhook branch from 0d23b99 to d6c6feb Compare October 9, 2026 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants