feat: persist accepted events to deduplicate redeliveries and expose events that matched nothing by VascoSch92 · Pull Request #381 · OpenHands/automation

VascoSch92 · 2026-08-24T09:07:03Z

Closes #361 — phase 4 of 5, part of #363.

Stacked on #378 (phase 2). Base branch is vasco/provider-descriptor-registry;
review only the commit on top of it.

One new table, one new nullable-free column set, no behaviour change for any
event that is not a redelivery.

What changed

accept_event() verified nothing about the delivery itself. It matched
triggers, created runs, committed, and forgot. If an event matched no
automation it created no runs, and so left no trace at all — "did my webhook
arrive, or is my filter wrong?" had no answer anywhere in the service.

IntegrationEvent is written inside accept_event(), in the same transaction
as the runs:

integration_events
  id, org_id, source, provider_event_id, event_key,
  payload, matched_count, received_at

  UNIQUE (org_id, source, provider_event_id) WHERE provider_event_id IS NOT NULL

The insert happens first, before the match pass, so it doubles as the
dedupe gate: a redelivery costs one failed INSERT instead of a full routing
pass. A unique violation rolls back to a savepoint — without which the failure
would poison the transaction the caller still has to commit — and returns
AcceptResult(duplicate=True). The HTTP handler still answers 200: from the
provider's side the delivery succeeded, and anything else only invites more
retries.

Transports supply the id, the same way they supply authentication.
Provider gains event_id_header, and the HTTP receiver reads it
(X-GitHub-Delivery for GitHub). A stream transport is handed the envelope and
sets AcceptedEvent.provider_event_id from it directly — which is how #360 gets
multi-replica dedupe for free.

Pruning goes in the watchdog loop, bounded by integration_event_retention_days
(default 14) and by PRUNE_BATCH_SIZE per scan.

Five decisions worth reviewing

1. The dedupe key is scoped by org.

The issue sketches PARTIAL UNIQUE (source, provider_event_id). That is unsafe
for custom webhooks: source is only unique per org
(ix_custom_webhooks_org_source), so two orgs each running a webhook they both
call ci, each numbering deliveries from 1, would deduplicate against each
other — the second org's first event would vanish silently.

Adding org_id weakens nothing. A delivery belongs to exactly one org either
way, so GitHub and Slack dedupe identically. Test:
test_accept_event_deduplicates_within_one_org_only.

2. processing_status and error are not in the table.

The issue lists them. With the async routing worker explicitly out of scope,
neither column has a writer: routing is synchronous, so a failure to create runs
rolls the whole transaction back and there is no row to put an error on, and
processing_status would be one constant value on every row ever written.

This is the issue's own reversibility argument applied one level down — the
worker that gives those columns meaning will need its own migration anyway (a
claim/lease column at minimum), and adding two nullable columns then is cheaper
than carrying two dead ones now. Happy to add them if you'd rather have the
shape settled up front.

3. The delivery id is a dedupe key, not replay protection.

X-GitHub-Delivery is not covered by the signature, so a replayer who alters
it gets a fresh id. That is worth being explicit about, because problem #2 in
the issue is "replay is unsafe" and this only partly addresses it: verbatim
replays now no-op, crafted ones do not. Real replay protection is the
timestamp-signing schemes phase 2 added (standard_webhooks, slack_v0). The
module docstring says so rather than implying the hole is closed.

Confirmed: built-in deliveries do not currently carry this header, so it is
inert for them today. Built-in sources do not reach us straight from the
provider — they are re-enveloped and re-signed upstream, and that hop forwards
only the content type and the signature. I checked; no delivery id survives it.

So for now every forwarded built-in event gets provider_event_id = NULL:
recorded, routed, not deduplicated — today's behaviour plus the row.
test_github_event_without_a_delivery_header_is_still_recorded pins exactly
that. Acceptance criterion 1 is therefore not met end-to-end by this PR
alone, and a reviewer should read it that way: the mechanism here is complete
and tested, the producer upstream is missing. Tracked separately.

When that producer is added, a header is the wrong place to put it. The
signature covers only the body, so a header is tamperable, whereas the envelope
that hop already builds is signed. Reading the id out of the body is also what
a Slack envelope needs. Expect event_id_header to gain a body-expression
sibling rather than be replaced.

4. A violation with no id is re-raised, not reported as a duplicate.

The index is partial, so a row with provider_event_id IS NULL cannot conflict
on it. If an IntegrityError still arrives, something else failed, and
swallowing it as a duplicate would drop a genuine event without a trace.

5. Pruning rides the watchdog rather than getting its own loop.

It is the service's only periodic janitor. A second loop with its own interval,
task and shutdown handling buys nothing for one bounded DELETE.

Two indexes, not three

The partial unique index and received_at (which pruning uses). An
(org_id, received_at) index is what a browse-events UI will want, and it can
land with the query surface that needs it — this phase makes the data exist.

Acceptance criteria

Criterion	Test
Same GitHub delivery ID twice → runs once, 2XX both times	`test_redelivered_github_event_creates_runs_once`
Same id via two processes against a shared DB → runs once	`test_accept_event_deduplicates_through_the_database`
Event matching nothing recorded with `matched_count = 0`	`test_accept_event_records_events_that_match_nothing`
Events without an id recorded, not deduplicated	`test_accept_event_does_not_deduplicate_without_an_id`
Event row and runs commit atomically	`test_accept_event_records_and_creates_runs_atomically`
Pruning in place and bounded	`TestPruneIntegrationEvents`

Plus test_accept_event_dedupe_survives_a_rolled_back_delivery: a delivery that
failed to route can be retried under the same id, because the dedupe row rolls
back with everything else. Without that, one transient failure would swallow
the event permanently.

Out of scope

Async routing worker (per the issue). Any UI for browsing events. Dedupe for
custom webhooks — the standard_webhooks scheme signs its webhook-id, which
would make it a replay-proof key rather than merely a dedupe one, and that is
a natural follow-up.

Verification

Full suite: 1454 passed (re-run after merging main).
pre-commit run --all-files: clean (ruff format, ruff lint, pycodestyle, pyright).
Migration 019 checked offline against PostgreSQL; the partial unique index
emits as expected. SQLite covered by the existing alembic upgrade head test.
Deployed to the shared OSS automation VM (OH_AUTOMATION_GIT_REF), which
runs SQLite with 30 automations and ~109k automation_runs. The event-log
migration applied on startup with no downtime beyond the normal restart, the
table and partial index landed, and the stack came back healthy. A
webhook-driven test automation there confirmed end to end that a matching
event creates a run and is recorded, and that an event filtered out by
JMESPath is recorded with matched_count = 0.
That same VM test also demonstrated the gap above from the other direction: a
custom-source webhook fed a real X-GitHub-Delivery header still recorded
provider_event_id = NULL and created a second run on redelivery, because
only a registered Provider supplies a delivery id. Custom webhooks having
no dedupe is per Persist accepted events to deduplicate redeliveries and expose events that matched nothing #361, but it is worth seeing rather than assuming.
Merged main in. Phases 1 and 2 are now squash-merged there (refactor: extract a transport-neutral accept_event() from the webhook handler #367, feat: replace the parse-only source registry with a provider descriptor and verifier registry #378), so
the branch's own copies of those commits conflicted with their squashed
equivalents; every conflicted file was byte-identical between the two sides,
so the branch version stands. main also landed 017_add_run_metadata, which
pushed the webhook-signature migration to 018 — so this PR's migration is
renumbered 019_add_integration_events (down_revision = "018"). The chain
is linear 016→019 with a single head, which tests/test_db.py pins by running
a real alembic upgrade head.

HUMAN: Deployed this branch to the shared OSS automation VM and drove real webhooks
through it end to end, rather than trusting the test suite alone.

A matching webhook creates a run and is recorded; an event filtered out by
JMESPath is recorded with matched_count = 0.
A custom-source webhook carrying an X-GitHub-Delivery header still recorded
provider_event_id = NULL and created a second run on redelivery — the gap
described above, observed rather than assumed.
Migrations applied on startup against the VM's live SQLite database (30
automations, ~109k automation_runs) with no downtime beyond the restart.

… handler `receive_event()` fused two unrelated halves into one function: an HTTP front half that turns an untrusted request into a trustworthy, interpreted event, and a back half that routes that event to automations and creates runs. Only the first half is HTTP-specific, so a second transport had nowhere to attach — it had to duplicate matching and run creation, or fake an HTTP request to reach them. Add `openhands/automation/ingest.py` with `AcceptedEvent`, `AcceptResult` and `accept_event()`. The routing half moves there unchanged; `receive_event()` keeps acquisition, authentication and interpretation, then calls `accept_event()` and maps the result onto `EventResponse`. Authentication stays with the transport by design: `accept_event()` receives an already-authenticated event and never asks how it was authenticated. Behaviour is unchanged. The existing `event_router` test suite passes unmodified, and the generated OpenAPI spec is byte-identical. `AcceptedEvent.provider_event_id`, `.subject` and `.occurred_at` are reserved for later phases: present in the signature, but not populated or read. `.parsed_event` carries the typed `WebhookEvent` the HTTP handler already builds, which is what preserves the run `event_payload` shape exactly — `model_dump(mode="json")` for Pydantic-parsed events, the raw payload for transports that have no model. No schema change, no new dependency, and no change to `trigger_matcher.py`, `create_automation_run()`, or the dispatcher.

Cut the module, class and function docstrings down to what is not already obvious from the code, and drop the step-numbering and restated-logic comments. Make `EventSubject`, `AcceptedEvent` and `AcceptResult` `frozen=True, slots=True`: an accepted event is a value handed across the seam, so nothing downstream should be able to rewrite it, and slots keeps it cheap. Two tests cover both properties.

…emetry Issue #358 says non-HTTP callers can simply omit `request` and "all existing telemetry events keep firing". They do not. Telemetry resolves a backend distinct id from the database, and with no request, session or session_factory `get_automation_backend_distinct_id()` returns None (telemetry.py:75-77); `_capture_automation_event` then returns before the POST. `capture_automation_event` swallows everything, so the loss is silent. A transport calling `accept_event(..., request=None)` therefore loses all three events per run. The HTTP path is unaffected — it reaches the factory through `request.app.state.session_factory` — so this is latent until phase 3. Add an optional `session_factory` and forward it to the three telemetry calls. `receive_event()` passes nothing, so the HTTP path is byte-identical; the OpenAPI spec still matches main exactly. Not `session=`: telemetry writes its id row, and sharing the caller's session would move that INSERT into the caller's transaction, where a failure would be hidden by the same broad except and poison the run creation that follows. Two tests cover it — the full event sequence for a request-less caller, and that routing still succeeds while telemetry is dropped when neither a request nor a factory is given, which is what makes the bug easy to miss.

…or and verifier registry A provider was described in three places: BUILTIN_SOURCES mapped a slug to a secret extractor, the event_schemas _PARSERS registry mapped the same slug to a parser, and RESERVED_SOURCES listed the same slugs by hand. Adding one meant editing all three. Verification was not a registry at all: verify_signature() was hard-wired to hex HMAC-SHA256 over the raw body, and a custom webhook could configure only the header name. Standard Webhooks and Slack could not be onboarded. Add openhands/automation/providers.py holding both registries. Provider carries the parser, verifier name, signature header, secret accessor, capabilities, and the subject/handshake hooks reserved for later phases. VERIFIERS resolves a scheme by name: hmac_sha256_hex (unchanged), standard_webhooks (from PR #247) and slack_v0, the last two rejecting deliveries outside a 5-minute window. event_router resolves a verifier instead of calling verify_signature directly, parse_event reads the descriptor, and RESERVED_SOURCES is derived. Add custom_webhooks.signature_scheme as a nullable column; NULL reads as hmac_sha256_hex, the behaviour the row was created with. slack is deliberately not registered as a provider: the OSS VM delivers to that source through a CustomWebhook row, and reserving the name would break it. Slack becomes a provider in #360. Also fix two order-dependent tests in test_router.py that asserted on a browser-supplied telemetry distinct id without forcing local mode; they passed only while an earlier test file left a local-mode config in the cache.

Delete the `# ====` banner comments, in providers.py, event_schemas and utils/webhook.py (where the banner predated this branch) and in the tests. Hoist every constant in providers.py to the top of the file and type them Final[str]/Final[int]. Drop `from __future__ import annotations`. It was carrying nothing: the two TYPE_CHECKING names are reached through string forward references inside `Callable[...]` type aliases, which are runtime expressions the future import does not touch. Import, pyright and the suite are unaffected without it. Correct the lazy-import comment in `parse_event()`. The import is genuinely required, but the cycle it breaks is not the one the comment named: moving it to module level fails on providers.py's own `from ...event_schemas import WebhookEvent`, not on the parsers the submodules define. Drop the issue and PR numbers from code comments and test docstrings. Trim the docstrings throughout to a single line where the signature already says the rest, including the Args/Returns block `verify_signature()` carried over from utils/webhook.py, and cut the migration docstring down to the one-line summary the other migrations use. No behaviour change: 1422 passed, pre-commit clean.

Move VERIFIERS to the top of providers.py alongside PROVIDERS. Neither can hold its entries there -- the values are instances of classes defined further down, and a module-level annotation is evaluated at runtime -- so both are declared empty next to the type contracts and filled at the bottom, which is what PROVIDERS already did. Fix a crash on a non-ASCII signature header. `hmac.compare_digest` raises TypeError rather than returning False when either str argument is non-ASCII, and header bytes reach an app latin-1 decoded, so a signature header of b"\xff" took the request out through an unhandled exception -- a 500 where a 401 belongs. Reachable on every scheme: it was already true of `verify_signature` before this branch, and the two new verifiers inherited it. All three now compare bytes. Correct three comments and a migration docstring that claimed rows predating the column read as NULL. They do not: the ALTER TABLE carries a server_default, so existing rows are backfilled to "hmac_sha256_hex". NULL is reachable, but through a PATCH that clears the field, so the fallback stays and the reason it exists is now stated accurately. The test that covered this is renamed to match what it actually sets up. New tests: a non-ASCII signature is refused by each verifier and returns 401 end to end; PATCHing signature_scheme persists and changes which signature the delivery path accepts, which nothing covered; and standard_webhooks_key pins that a literal secret which happens to parse as base64 is still decoded. 1428 passed, pre-commit clean.

Remove the module logger and its import: nothing in providers.py ever logged. `except (binascii.Error, ValueError)` around b64decode is one clause written twice -- binascii.Error subclasses ValueError -- so catch ValueError and drop the binascii import. `except (TypeError, ValueError)` around int(timestamp) had a dead arm. The guard above it rules out None, and int() of a str raises only ValueError. Both timestamped verifiers carried the same six lines of parse-and-compare. They now share `_within_tolerance()`, which also names the concept the two schemes have in common. `get_header` tried an exact hit, then a lowercased hit, then a full scan. The scan already subsumes the lowercased hit, so that middle branch was three lines buying nothing. 1428 passed, pre-commit clean.

…what matched nothing An event that matched no automation left no trace at all: the only durable record of an incoming delivery was the AutomationRun rows it happened to create. `IntegrationEvent` is written by `accept_event()` in the same transaction as those runs, so the row and its runs commit together. Two things follow. A delivery the transport can name -- GitHub's X-GitHub-Delivery, Slack's envelope event_id -- is deduplicated by a partial unique index on (org_id, source, provider_event_id), which is what makes a redelivery safe across replicas where an in-process set cannot see the first one. And `matched_count = 0` finally distinguishes "your filter is wrong" from "it never arrived". The index is partial because a NULL provider_event_id means the provider does not identify its deliveries, not that the id is unknown; those events are recorded and routed, just never deduplicated. The watchdog prunes the table, in bounded batches, past a configurable retention window.

…anch The base branch picked up three review commits (301097d, c6aee4c, d597b39) after this branch was cut, which rewrote the same comments this branch edits. Resolved two conflicts, both in comment text: - event_router.py: keep the base's tightened replay-window bullet, and drop the "consider tracking delivery IDs" and "current risk is acceptable" bullets, which this branch makes obsolete by implementing the dedup. - providers.py: keep the base's one-line `subject` comment and trimmed registration comment, keep this branch's `event_id_header` field and the three-tuple registration loop that populates it.

github-actions · 2026-08-24T14:24:19Z

all-hands-bot · 2026-08-24T15:05:18Z

👋 This PR needs a couple of things fixed before OpenHands can review it:

the PR description's HUMAN: section needs at least 20 characters describing what you tested, not just the template placeholder

Push an update once this is addressed and this check re-runs automatically.

This is an automated check - no AI was used to generate this comment.

malhotra5 · 2026-08-24T17:04:55Z

+            session.add(record)
+            await session.flush()
+    except IntegrityError:
+        if event.provider_event_id is None:


NIT: we can avoid having none type for the provider event id by generating an id when unspecified. a quick solution would be to hash the json payload to generate an id

I think it is already the whole traffic today that... So should we change that?

Not a problem just to know :-)

Hmm fair

We can't rely on hashing just a fixed length substring as that's not reliable, so yep okay with not doing this

main now carries phases 1 and 2 as squash merges (#367, #378), so the branch's own copies of those commits conflicted with their squashed equivalents. Every conflicted file was byte-identical between the two sides' phase-2 content, so the branch version stands. Migration 018 was renumbered to 019: main landed 017_add_run_metadata and pushed the webhook signature scheme migration to 018.

VascoSch92 added 8 commits August 23, 2026 13:17

github-actions Bot added the type: feat A new feature label Aug 24, 2026

VascoSch92 and others added 2 commits August 24, 2026 11:21

docs: trim the docstrings added by this change to one-liners

5e65a64

VascoSch92 marked this pull request as ready for review August 24, 2026 14:22

VascoSch92 requested a review from malhotra5 August 24, 2026 16:56

malhotra5 reviewed Aug 24, 2026

View reviewed changes

VascoSch92 force-pushed the vasco/provider-descriptor-registry branch from d597b39 to 8c3e584 Compare August 24, 2026 17:11

VascoSch92 changed the base branch from vasco/provider-descriptor-registry to main August 24, 2026 18:54

malhotra5 approved these changes Aug 24, 2026

View reviewed changes

VascoSch92 merged commit 2b4714a into main Aug 24, 2026
16 checks passed

openhands-release-bot Bot mentioned this pull request Aug 24, 2026

chore(main): release 1.9.0 #351

Open

Navigation Menu

Uh oh!

feat: persist accepted events to deduplicate redeliveries and expose events that matched nothing - #381

feat: persist accepted events to deduplicate redeliveries and expose events that matched nothing#381
VascoSch92 merged 11 commits into
mainfrom
vasco/integration-event-log

VascoSch92 commented Aug 24, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 24, 2026 •

edited

Loading

Uh oh!

all-hands-bot commented Aug 24, 2026

Uh oh!

malhotra5 Aug 24, 2026

Uh oh!

VascoSch92 Aug 24, 2026

Uh oh!

malhotra5 Aug 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

VascoSch92 commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Five decisions worth reviewing

Two indexes, not three

Acceptance criteria

Out of scope

Verification

Uh oh!

github-actions Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

all-hands-bot commented Aug 24, 2026

Uh oh!

malhotra5 Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

VascoSch92 Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

malhotra5 Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

VascoSch92 commented Aug 24, 2026 •

edited

Loading

github-actions Bot commented Aug 24, 2026 •

edited

Loading