feat(webhook): make an outbound delivery survive a hard crash - #1402
Merged
Conversation
Fan-out is fire-and-forget from the projector, so a crash between persisting a message and completing its POST lost the delivery with no record in either mode, while the documented contract promises at-least-once. The in-memory inFlightDeliveries map already tracked exactly that set; this gives it a durable twin, mirroring ingress_events and its reconciler on the inbound side. - webhook_outbox_events records one delivery per (webhookId, idempotencyKey), written before the attempt. The payload is retired the moment an outcome is recorded, so only a replayable row carries a message body. - The record is closed as 'dispatched' once a durable owner has it: handed to BullMQ, or completed inline. Retiring on the ENQUEUE rather than on the POST is what stops the sweep duplicating work the queue already holds. A limiter rejection (capacity, shutdown drain) deliberately leaves the row pending, which is the case that used to vanish. - WebhookReconcilerService sweeps stranded rows on an unref'd interval, replaying each through the stored idempotency key so the retry is deduplicable at the receiver. A fresh delivery id is issued, because that names the attempt rather than the event. Overlap-guarded, batched, and bounded by a per-row budget: a stuck delivery goes terminal instead of looping. - Four knobs, mirroring the ingress ones, documented and forwarded through both compose files and the blank-shadow list. - docs/06 no longer claims a hard crash simply loses in-flight deliveries, and names the window that is still open: the record and the message insert are not yet one transaction. The new table joins the backup registry, which pulls it through the row type, both response DTOs, the importer and the FK-safe import order.
A hard crash cannot be staged inside a jest worker, but the state it freezes can: a delivery that never completed leaves a 'pending' row still carrying its payload. The suite proves the process really reaches that state while a receiver holds the response open, then proves a later instance drains it and that the replay carries the SAME idempotency key, which is what lets a receiver dedup the retry instead of seeing a second event. Writing it turned up a real defect: the entity dated createdAt through dateColumnType() plus the transformer, which never populated the column, so every outbox write failed the NOT NULL constraint and was swallowed by the best-effort catch. The record silently did not exist. It now uses the plain @CreateDateColumn the ingress entity uses, and the migration spells the two date columns separately, because they carry different decorators and TypeORM emits different SQLite types for them.
The record was written and settled but never removed, so the table grew by one row per webhook per event for the life of the deployment. The inbound twin has carried a retention sweep since it was introduced; this is the outbound half. - Settled rows older than WEBHOOK_OUTBOX_RETENTION_DAYS (default 7) are pruned daily. A 'pending' row is never pruned on age: it is a delivery that can still be replayed, which is the whole point of the table. - The window is deliberately independent of the delivery-failure retention switch, and a non-positive value falls back to the default with a warning. A settled row carries no payload and no audit value, so opting into unbounded growth buys nothing, which is the same call the inbound dedup log makes. - The reconciler records what it does NOT do: it takes no row claim, so two nodes on one database can both replay a delivery. The replay carries the stored idempotency key, so the cost is a duplicate the contract already tells consumers to expect, rather than a lock whose holder can die mid-flight. - docs/06 qualifies the failure table: a capacity-shed or drain-refused delivery keeps its record AND is replayed, so a row there can belong to an event that was later delivered.
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
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.
Webhook fan-out is fire-and-forget from the projector, so a crash between persisting a message and completing its POST lost the delivery with no record in either mode, while the documented contract promises at-least-once. With the queue off, which is the default, every in-flight and parked delivery went with it.
The in-memory
inFlightDeliveriesmap already tracked exactly that set. This gives it a durable twin, mirroringingress_eventsand its reconciler on the inbound side.How it works
webhook_outbox_eventsholds one row per(webhookId, idempotencyKey), written before the attempt. The payload is retired the moment an outcome is recorded, so only a replayable row ever carries a message body.dispatchedonce something durable owns the delivery: handed to BullMQ, or completed inline in direct mode. Retiring on the enqueue rather than on the POST is what stops the sweep duplicating work the queue already holds.pending. That is the case that used to vanish.WebhookReconcilerServicesweeps stranded rows on an unref'd interval and replays each one through its stored idempotency key, so the retry is deduplicable at the receiver; a fresh delivery id is issued, because that names the attempt rather than the event. Overlap-guarded, batched, and bounded by a per-row budget so a stuck delivery goes terminal instead of looping.pendingrow is never pruned on age, and the window is independent of the delivery-failure retention switch: a settled row carries no payload and no audit value, so letting an operator opt into unbounded growth buys nothing.Five knobs, mirroring the inbound ones, documented in
.env.exampleand forwarded through both compose files and the blank-shadow list.What is now true, and what is not
docs/06no longer claims a hard crash simply loses in-flight deliveries, and it names the window that is still open: the record and the message insert are not yet one transaction, so a crash between them still loses the delivery.Two properties are recorded in the source rather than left to be rediscovered:
Verification
The lint gates, all four test lanes, and an e2e that covers the crash window end to end: a delivery held open by the receiver really does leave a
pendingrow carrying its payload, and a later instance drains it with the same idempotency key and a new delivery id.Every assertion added here was checked by mutating the code it covers and confirming the run fails, ten mutations in total.
Writing that e2e is what caught the defect that would have shipped otherwise: the entity dated
createdAtthrough the transformer helper, which never populated the column, so every write failed the NOT NULL constraint and was swallowed by the best-effort catch. Fourteen unit tests passed against a stubbed repository while the record silently did not exist.Rollout
The job shape changed in the deliverOnce extraction, which shipped in 0.21.0, so this lands one release later as planned. The migration is additive and the table is new, so a rollback needs no data step.