feat(gateway): add webhook subscription reactivation endpoint - #1586
Conversation
Add POST /v1/webhook_subscriptions/:id/reactivate to recover a webhook subscription that the delivery circuit breaker (#912) marked dead, resetting delivery_status to healthy and consecutive_failures to 0 without disturbing the URL/secret/event-type configuration. Previously the only recovery path was delete + recreate, which churned the delivery secret and lost the event-type/severity filters.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds a new ChangesWebhook Subscription Reactivation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new endpoint POST /v1/webhook_subscriptions/:id/reactivate to reactivate dead webhook subscriptions by resetting their status to healthy and consecutive failures to zero, avoiding the need to delete and recreate them. Feedback suggests optimizing the handler with an early return if the subscription is already healthy to prevent redundant database writes and audit logs, and updating the OpenAPI documentation to accurately reflect the returned response body.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| record.delivery_status = "healthy".to_string(); | ||
| record.consecutive_failures = 0; | ||
|
|
||
| match state.storage.update_webhook_subscription(&record).await { | ||
| Ok(()) => { | ||
| let audit = AuditEventRecord { | ||
| id: Uuid::new_v4().to_string(), | ||
| tenant_id: tenant_id.clone(), | ||
| event_type: "webhook_subscription_reactivated".to_string(), | ||
| agent_id: None, | ||
| user_id: None, | ||
| run_id: None, | ||
| trace_id: None, | ||
| span_id: None, | ||
| skill: None, | ||
| action: Some("webhook_subscription_reactivated".to_string()), | ||
| resource: Some(id.clone()), | ||
| event_json: serde_json::to_string(&json!({ "subscription_id": id })) | ||
| .unwrap_or_default(), | ||
| input_hash: None, | ||
| output_hash: None, | ||
| decision_id: None, | ||
| approval_id: None, | ||
| created_at: Utc::now(), | ||
| }; | ||
| let _ = state.storage.insert_audit_event(&audit).await; | ||
| info!(subscription_id = %id, "Webhook subscription reactivated"); | ||
| ( | ||
| StatusCode::OK, | ||
| Json(json!({ | ||
| "id": record.id, | ||
| "delivery_status": record.delivery_status, | ||
| "consecutive_failures": record.consecutive_failures, | ||
| })), | ||
| ) | ||
| .into_response() | ||
| } |
There was a problem hiding this comment.
If the webhook subscription is already healthy and has no consecutive failures, performing a database update and writing an audit log is redundant. Adding an early return makes this endpoint a true no-op for healthy subscriptions, avoiding unnecessary database write locks and audit log pollution. Additionally, returning the full updated WebhookSubscriptionRecord is more consistent with other endpoints (like create and list) than returning a subset of fields.
if record.delivery_status == "healthy" && record.consecutive_failures == 0 {
return (StatusCode::OK, Json(record)).into_response();
}
record.delivery_status = "healthy".to_string();
record.consecutive_failures = 0;
match state.storage.update_webhook_subscription(&record).await {
Ok(()) => {
let audit = AuditEventRecord {
id: Uuid::new_v4().to_string(),
tenant_id: tenant_id.clone(),
event_type: "webhook_subscription_reactivated".to_string(),
agent_id: None,
user_id: None,
run_id: None,
trace_id: None,
span_id: None,
skill: None,
action: Some("webhook_subscription_reactivated".to_string()),
resource: Some(id.clone()),
event_json: serde_json::to_string(&json!({ "subscription_id": id }))
.unwrap_or_default(),
input_hash: None,
output_hash: None,
decision_id: None,
approval_id: None,
created_at: Utc::now(),
};
let _ = state.storage.insert_audit_event(&audit).await;
info!(subscription_id = %id, "Webhook subscription reactivated");
(StatusCode::OK, Json(record)).into_response()
}| ("id" = String, Path, description = "Webhook subscription ID") | ||
| ), | ||
| responses( | ||
| (status = 200, description = "Webhook subscription reactivated successfully"), |
There was a problem hiding this comment.
Document the response body of the reactivation endpoint in the OpenAPI specification to match the returned WebhookSubscriptionRecord.
| (status = 200, description = "Webhook subscription reactivated successfully"), | |
| (status = 200, description = "Webhook subscription reactivated successfully", body = WebhookSubscriptionRecord), |
Pre-existing rustfmt-version-dependent formatting drift in #912's test module (4 spots), unrelated to this PR's diff but blocking the Gateway (stable) CI fmt check for every PR based on current main.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/src/routes/tenant.rs (1)
719-768: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the non-
deadno-op case to this test.This only proves the
dead -> healthypath. It won't catch the current regression where adegradedsubscription also gets reset tohealthy/0, even though the endpoint is documented as a no-op unless the record is alreadydead.Based on learnings, use TDD (RED -> GREEN) for changes.
🤖 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 `@src/src/routes/tenant.rs` around lines 719 - 768, The `reactivate_webhook_subscription` test currently covers only the `dead -> healthy` path, so add a case in this same test that creates or forces a non-`dead` subscription state such as `degraded`, calls `reactivate_webhook_subscription`, and asserts the record is unchanged. Use the existing `state.storage`, `record_webhook_delivery_attempt`, and `get_webhook_subscription` flow to verify the endpoint is a no-op unless the subscription is already `dead`.Source: Learnings
🤖 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 `@src/src/routes/openapi.rs`:
- Around line 761-764: The OpenAPI response list for the webhook reactivation
endpoint is incomplete because the handler in webhooks.rs can also return an
internal server error. Update the responses declaration in openapi.rs for the
reactivation route to include a 500 entry alongside 200 and 404, using the same
StatusError response type so the documented contract matches the handler’s
StatusError::internal("Database error") behavior.
In `@src/src/routes/webhooks.rs`:
- Around line 537-559: The webhook reactivation path in
webhooks::recreate_webhook_subscription currently ignores failures from
insert_audit_event after update_webhook_subscription succeeds, so the endpoint
can return success without writing the required audit trail. Refactor the logic
around the update_webhook_subscription / insert_audit_event flow into a single
service or storage operation that persists both the subscription change and the
webhook_subscription_reactivated audit record together, or otherwise propagate
the audit error back to the handler instead of discarding it. Keep the handler
thin by moving this business logic out of the route and use the existing
AuditEventRecord creation as the place to wire the combined persistence
behavior.
- Around line 534-535: The recovery logic in the webhook route should only apply
to subscriptions currently marked as dead, not to every existing record. Update
the handler around the subscription status recovery path so it checks the
record’s delivery_status before resetting consecutive_failures and setting
delivery_status back to healthy, and leave non-dead subscriptions unchanged as a
no-op.
---
Nitpick comments:
In `@src/src/routes/tenant.rs`:
- Around line 719-768: The `reactivate_webhook_subscription` test currently
covers only the `dead -> healthy` path, so add a case in this same test that
creates or forces a non-`dead` subscription state such as `degraded`, calls
`reactivate_webhook_subscription`, and asserts the record is unchanged. Use the
existing `state.storage`, `record_webhook_delivery_attempt`, and
`get_webhook_subscription` flow to verify the endpoint is a no-op unless the
subscription is already `dead`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f17844e2-792a-4046-8264-c78dc0a0cfd3
📒 Files selected for processing (6)
CLAUDE.mdlib/storage/src/db/webhooks.rssrc/src/main.rssrc/src/routes/openapi.rssrc/src/routes/tenant.rssrc/src/routes/webhooks.rs
| responses( | ||
| (status = 200, description = "Webhook subscription reactivated successfully"), | ||
| (status = 404, description = "Subscription not found", body = StatusError) | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the 500 response here too.
The handler returns StatusError::internal("Database error") on lookup/update failures, but this operation is advertised as only 200/404. That leaves the generated API contract incomplete for clients.
Possible fix
responses(
(status = 200, description = "Webhook subscription reactivated successfully"),
- (status = 404, description = "Subscription not found", body = StatusError)
+ (status = 404, description = "Subscription not found", body = StatusError),
+ (status = 500, description = "Database error", body = StatusError)
)
)]Based on src/src/routes/webhooks.rs, this endpoint can also return 500.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| responses( | |
| (status = 200, description = "Webhook subscription reactivated successfully"), | |
| (status = 404, description = "Subscription not found", body = StatusError) | |
| ) | |
| responses( | |
| (status = 200, description = "Webhook subscription reactivated successfully"), | |
| (status = 404, description = "Subscription not found", body = StatusError), | |
| (status = 500, description = "Database error", body = StatusError) | |
| ) |
🤖 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 `@src/src/routes/openapi.rs` around lines 761 - 764, The OpenAPI response list
for the webhook reactivation endpoint is incomplete because the handler in
webhooks.rs can also return an internal server error. Update the responses
declaration in openapi.rs for the reactivation route to include a 500 entry
alongside 200 and 404, using the same StatusError response type so the
documented contract matches the handler’s StatusError::internal("Database
error") behavior.
| record.delivery_status = "healthy".to_string(); | ||
| record.consecutive_failures = 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Honor the documented no-op behavior for non-dead subscriptions.
This unconditionally clears consecutive_failures and forces delivery_status = "healthy" for any existing record, so a degraded subscription gets reactivated too. The endpoint contract here is specifically for subscriptions already marked dead.
Possible fix
- record.delivery_status = "healthy".to_string();
- record.consecutive_failures = 0;
+ if record.delivery_status != "dead" {
+ return (
+ StatusCode::OK,
+ Json(json!({
+ "id": record.id,
+ "delivery_status": record.delivery_status,
+ "consecutive_failures": record.consecutive_failures,
+ })),
+ )
+ .into_response();
+ }
+
+ record.delivery_status = "healthy".to_string();
+ record.consecutive_failures = 0;Based on PR objectives, only subscriptions marked delivery_status = "dead" should be recovered by this endpoint.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| record.delivery_status = "healthy".to_string(); | |
| record.consecutive_failures = 0; | |
| if record.delivery_status != "dead" { | |
| return ( | |
| StatusCode::OK, | |
| Json(json!({ | |
| "id": record.id, | |
| "delivery_status": record.delivery_status, | |
| "consecutive_failures": record.consecutive_failures, | |
| })), | |
| ) | |
| .into_response(); | |
| } | |
| record.delivery_status = "healthy".to_string(); | |
| record.consecutive_failures = 0; |
🤖 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 `@src/src/routes/webhooks.rs` around lines 534 - 535, The recovery logic in the
webhook route should only apply to subscriptions currently marked as dead, not
to every existing record. Update the handler around the subscription status
recovery path so it checks the record’s delivery_status before resetting
consecutive_failures and setting delivery_status back to healthy, and leave
non-dead subscriptions unchanged as a no-op.
| match state.storage.update_webhook_subscription(&record).await { | ||
| Ok(()) => { | ||
| let audit = AuditEventRecord { | ||
| id: Uuid::new_v4().to_string(), | ||
| tenant_id: tenant_id.clone(), | ||
| event_type: "webhook_subscription_reactivated".to_string(), | ||
| agent_id: None, | ||
| user_id: None, | ||
| run_id: None, | ||
| trace_id: None, | ||
| span_id: None, | ||
| skill: None, | ||
| action: Some("webhook_subscription_reactivated".to_string()), | ||
| resource: Some(id.clone()), | ||
| event_json: serde_json::to_string(&json!({ "subscription_id": id })) | ||
| .unwrap_or_default(), | ||
| input_hash: None, | ||
| output_hash: None, | ||
| decision_id: None, | ||
| approval_id: None, | ||
| created_at: Utc::now(), | ||
| }; | ||
| let _ = state.storage.insert_audit_event(&audit).await; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don't silently succeed when the audit write fails.
insert_audit_event is fire-and-forget here, so the endpoint can return 200 OK after mutating the subscription without recording the required audit trail. This should be one lib/service operation that persists the state change and audit record together, or at least surfaces the audit failure instead of discarding it.
As per coding guidelines, src/ handlers and gRPC implementations must be THIN: parse → service call → respond, with NO business logic; based on PR objectives, this endpoint must record webhook_subscription_reactivated.
🤖 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 `@src/src/routes/webhooks.rs` around lines 537 - 559, The webhook reactivation
path in webhooks::recreate_webhook_subscription currently ignores failures from
insert_audit_event after update_webhook_subscription succeeds, so the endpoint
can return success without writing the required audit trail. Refactor the logic
around the update_webhook_subscription / insert_audit_event flow into a single
service or storage operation that persists both the subscription change and the
webhook_subscription_reactivated audit record together, or otherwise propagate
the audit error back to the handler instead of discarding it. Keep the handler
thin by moving this business logic out of the route and use the existing
AuditEventRecord creation as the place to wire the combined persistence
behavior.
Source: Coding guidelines
1 participant
Summary
POST /v1/webhook_subscriptions/:id/reactivateto recover a webhook subscription that the delivery circuit breaker ([TASK-0066] Add circuit breaker for external callback URLs #912) tripped intodelivery_status = "dead".delivery_statusto"healthy"andconsecutive_failuresto0, without touching the subscription'surl,delivery_secret,event_types,min_severity, orformat.AuditEventRecord(webhook_subscription_reactivated) for the action.src/src/main.rs) and OpenAPI (src/src/routes/openapi.rs).Why
Closes #1584. Before this change, the only way to recover a dead webhook subscription was delete + recreate, which churns the
delivery_secret(breaking HMAC verification on the receiving end) and loses the configuredevent_types/min_severity/formatfilters.Test plan
test_reactivate_webhook_subscription_routeinsrc/src/routes/tenant.rs, confirmed it failed to compile before the handler existed.reactivate_webhook_subscriptioninsrc/src/routes/webhooks.rs; test drives a subscription todeadvia 10 consecutiverecord_webhook_delivery_attempt(false)calls, reactivates it, and assertsdelivery_status == "healthy",consecutive_failures == 0, and thaturl/delivery_secretare unchanged; also asserts 404 for a nonexistent subscription ID.cargo fmt --manifest-path src/Cargo.toml -- --checkcargo clippy --manifest-path src/Cargo.toml --all-targets -- -D warningscargo test --manifest-path src/Cargo.toml --workspace— full workspace green (793 tests across all crates, 3 ignored, 0 failed)Summary by CodeRabbit
New Features
Bug Fixes
Documentation