feat(api): production reqwest (rustls) webhook delivery sink + worker - #177
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR wires a production HTTP transport (ReqwestSink, rustls-based) into the webhook delivery worker, adds an idempotent spawn-once guard, adds reqwest as a dependency with a license exception, updates public exports and documentation, and wires worker startup into the server. ChangesWebhook Delivery Transport
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Server as server.rs startup
participant Spawner as spawn_webhook_worker_once
participant Worker as delivery worker
participant Sink as ReqwestSink
participant Endpoint as Webhook endpoint
Server->>Server: build v1_webhooks_engine
Server->>Server: check tokio runtime handle
Server->>Sink: ReqwestSink::new()
Server->>Spawner: spawn_webhook_worker_once(bus, engine, sink, tick)
Spawner->>Spawner: check WORKER_STARTED guard
Spawner->>Worker: spawn worker task (once)
Worker->>Sink: post(prepared request)
Sink->>Endpoint: HTTP POST via reqwest client
Endpoint-->>Sink: HTTP response
Sink-->>Worker: DeliveryOutcome
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ry worker (§4.1) Registered webhooks previously enqueued deliveries as `pending` forever with no real transport. Add `ReqwestSink` (rustls-TLS only, no system OpenSSL) as the production `WebhookSink`, construct it once, and spawn the delivery worker at the server seam guarded to a live Tokio runtime and process-once (same idiom as the O4 depth sampler), so registered webhooks now actually POST to their operator URL under the engine's existing retry/backoff/HMAC discipline. Redirects are disabled so a 3xx can't silently route around the registration-time SSRF guard. Persistence remains the one documented deferral (in-memory registry).
264f5f3 to
9accf82
Compare
reqwest's rustls-tls backend pulls in webpki-roots, which ships the Mozilla CA root-store data under CDLA-Permissive-2.0 — a permissive data-only license. Scoped per-crate exception rather than a global allow, per the deliberate-per-dep policy in deny.toml. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BUh2DBnAPqThdYFZW5D8wx
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ergo-api/src/v1/webhooks/worker.rs (1)
189-208: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTransport failures are silently swallowed.
Err(_) => DeliveryOutcome::TransportErrordiscards the underlyingreqwest::Errorentirely. For a production external-call boundary that the engine will retry indefinitely on failure, there's no way to tell DNS failure, TLS handshake failure, connect refused, and timeout apart from logs/metrics — all collapse into the same opaque outcome.♻️ Suggested fix: log the error before discarding
- Err(_) => DeliveryOutcome::TransportError, + Err(err) => { + tracing::debug!( + delivery_id = %req.delivery_id, + webhook_id = %req.webhook_id, + error = %err, + "webhook delivery transport error" + ); + DeliveryOutcome::TransportError + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ergo-api/src/v1/webhooks/worker.rs` around lines 189 - 208, The transport error path in `WebhookWorker::post` is swallowing the underlying `reqwest::Error`, making all failures look identical. Update the `match builder.body(req.body.clone()).send().await` error branch to capture the error value instead of `_`, and log it with enough context before returning `DeliveryOutcome::TransportError`. Keep the existing `DeliveryOutcome` behavior, but ensure DNS, TLS, connect, and timeout failures are visible through the `post` method’s logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ergo-api/src/server.rs`:
- Around line 1264-1275: The webhook client initialization in the router startup
path is hard-failing via ReqwestSink::new().expect(...), which can bring down
router_with_mempool_and_wallet_and_security on client build errors. Update the
logic around the Handle::try_current() block to handle the ReqwestSink::new()
result gracefully: log the failure, skip spawn_webhook_worker_once, and leave
the webhook subsystem disabled so startup can continue. Use the existing
WebhooksState/webhooks_disabled pattern to keep the failure contained instead of
panicking.
---
Nitpick comments:
In `@ergo-api/src/v1/webhooks/worker.rs`:
- Around line 189-208: The transport error path in `WebhookWorker::post` is
swallowing the underlying `reqwest::Error`, making all failures look identical.
Update the `match builder.body(req.body.clone()).send().await` error branch to
capture the error value instead of `_`, and log it with enough context before
returning `DeliveryOutcome::TransportError`. Keep the existing `DeliveryOutcome`
behavior, but ensure DNS, TLS, connect, and timeout failures are visible through
the `post` method’s logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 135cd6c3-f1a7-4c42-a03d-28561a4ece0b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
deny.tomlergo-api/Cargo.tomlergo-api/src/server.rsergo-api/src/v1/mod.rsergo-api/src/v1/webhooks/mod.rsergo-api/src/v1/webhooks/worker.rs
…o build ReqwestSink::new().expect() in the router path would take the whole node down over an auxiliary subsystem's client-build failure. Route through the existing handle: None → 409 webhooks_disabled degrade path instead (a handle without a sink would accept registrations that never deliver), and log the build error. Also log the concrete transport error in ReqwestSink::post — DNS/TLS/connect/timeout failures all collapsed into an anonymous DeliveryOutcome::TransportError with no trace. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BUh2DBnAPqThdYFZW5D8wx
1 participant
What this is
The production HTTP delivery sink for the v1 webhooks subsystem — the deferred half of #174. The engine, retry/backoff, HMAC signing, and dedupe already existed behind an injected
WebhookSinktrait; this supplies the real network sink and spawns the delivery worker, so registered webhooks now actually deliver (they previously enqueued aspending).Supply-chain discipline (this is a consensus node)
reqwest = { version = "0.12", default-features = false, features = ["rustls-tls"] }— pure rustls, no OpenSSL. Verified:grep -i 'openssl\|native-tls' Cargo.lock→ no matches; the only new TLS crates are the expected rustls stack (hyper-rustls,tokio-rustls,rustls,ring,webpki-roots). Nojsonfeature (the signed body is already aString, sent via.body()). No system dependency, reproducible builds.Implementation
ReqwestSink(webhooks/worker.rs): one sharedreqwest::Client, 10s connect+overall timeout, redirects disabled (a 3xx must not bypass the registration-time SSRF guard). POSTs the signed body with the fullX-Ergo-*header set; reports the attempt outcome — the engine still owns retry/backoff/dead-letter, the sink does not reimplement it.server.rsseam inside the existing live-runtime guard, with a process-wideAtomicBoolonce-guard (mirrors the O4 depth sampler) — important here because this worker opens real outbound connections and the full-router tests build repeatedly under a runtime.Tests (no real external URLs)
ReqwestSink::new()builds against the rustls backend.#[tokio::test]spins a real127.0.0.1:0listener, fires one delivery, and asserts the received body +X-Ergo-Signature/X-Ergo-Webhook-Id/X-Ergo-Delivery-Idheaders withSuccess(200). The retry/backoff engine tests (injected fake sink) are untouched.Remaining deferral (documented): durable-across-restart registration still needs a DB schema (registry is in-memory).
Test plan
🤖 Generated with Claude Code
https://claude.ai/code/session_01BUh2DBnAPqThdYFZW5D8wx
Summary by CodeRabbit
New Features
Bug Fixes