Skip to content

refactor(webhooks): split webhook-delivery.service.ts by concern (#387) - #392

Merged
mohamedboukari merged 1 commit into
mainfrom
refactor/split-webhook-delivery
Nov 2, 2026
Merged

mohamedboukari merged 1 commit into
mainfrom
refactor/split-webhook-delivery

Conversation

@mohamedboukari

Copy link
Copy Markdown
Owner

Summary

Part of #387: src/modules/webhooks/services/webhook-delivery.service.ts (849 lines) is split by concern with no behaviour change. It stays the entry module (56 lines) re-exporting the same 10 symbols, so no importer or test changes.

Changes

File Lines Holds
webhook-delivery.service.ts 56 Entry: the original module JSDoc and the re-exports
webhook-delivery-schedule.service.ts 33 Retry backoff, attempt cap, nextAttemptAt
webhook-delivery-claim.service.ts 256 Fair claiming, the attempt lease, readWebhookSecret
webhook-delivery-attempt.service.ts 273 The HTTP attempt: SSRF re-validation, DNS pinning, caps, timeout
webhook-delivery-query.service.ts 147 List, get, replay
webhook-delivery-outcome.service.ts 79 recordAttempt
webhook-delivery-retention.service.ts 50 The retention purge
  • Every function moved whole; the only edits are export on two symbols the outcome file needs (not re-exported), one new JSDoc on the entry, and two stale comments (in webhook-dispatch.service.ts and docs/webhooks.md) that wrongly said signPayload was re-exported here.
  • Security checks byte for byte: every moved range is found intact as one block in its new file, including the whole HTTP attempt (SSRF re-validation via resolvePublicWebhookTarget, DNS pinning with Host and SNI, keepalive: false, decompress: false, redirect: "manual", the per-attempt timeout, refusal without a secret) and the 8 KB response read cap.
  • No module-level mutable state; no import cycles; load order through src/index.ts unchanged.
  • Docs: docs/webhooks.md, ARCHITECTURE.md, a comment in src/config/retention.ts, CHANGELOG.md.

Proof it is a pure move

  • Runtime exports (10) and strict type identity equal to main's, with a control that fails as expected.
  • Verbatim comparison: 758 lines each side, 0 differing, indentation kept.
  • A smoke delivery to a local receiver (16 cases: 200, 500, a redirect to a metadata address, gzip, a 100 KB body, NUL bytes, a timeout, a pinned host name, a private resolution, private and mapped literals, plain http, an invalid URL, an undecryptable secret) gives identical outcomes, fetch options and received requests on main and on this branch; refused cases never call fetch.
  • Every test passes without a test file being edited, including the tracing and schedule tests.

Environment

  • OS: macOS 27, plus a Linux container (oven/bun:1.3.14)
  • Bun version: 1.3.14
  • PostgreSQL version: 16
  • Docker: yes (local Postgres)

Test Plan

  • Unit (with random seeds), e2e and integration through the pre-commit hook, no test file edited
  • Types check, lint, knip, lint:mocks, prettier
  • Linux unit run: only the 3 known mx-transport.test.ts STARTTLS failures (need real Node in the container; pass on CI)
  • On the test server after merge: a real webhook delivery

Docs

  • CHANGELOG.md updated under [Unreleased]
  • Affected docs updated
  • No env var changes

Related Issues

Part of #387.

webhook-delivery.service.ts (849 lines) is now a 56-line entry module re-exporting the same 10 symbols, with the code moved whole into schedule, claim, attempt, query, outcome and retention files. Every SSRF and response-size check is byte-for-byte the same code.

No behaviour change: the same runtime exports and strictly identical types as main, 0 differing lines, identical results in a 16-case local delivery smoke run, and every test passes untouched.
@mohamedboukari
mohamedboukari merged commit a6c1cc5 into main Nov 2, 2026
10 checks passed
@mohamedboukari
mohamedboukari deleted the refactor/split-webhook-delivery branch November 2, 2026 05:11
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.

1 participant