Enhance ARI retry handling and improve webhook exception management#1184
Merged
Conversation
Add support for detecting ACME `alreadyReplaced` protocol errors and retry order creation without the ARI `replaces` hint when that race condition occurs. Webhook delivery is also hardened by wrapping payload creation and HTTP posting in exception handling, logging warnings instead of letting failures bubble. New unit tests cover both the ACME retry behavior and non-throwing webhook failure paths.
Add UTF-8 BOM markers to two test source files so their encoding matches project/editor defaults and avoids inconsistent file-header diffs.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request improves resiliency in two places: ACME order creation when ARI “replaces” is rejected as alreadyReplaced, and webhook delivery so failures don’t crash orchestration.
Changes:
- Adds
AcmeProtocolException.IsAlreadyReplacedand retries order creation withoutreplaceswhen that problem occurs. - Refactors
WebhookInvokerto build payloads via a factory and swallow/log exceptions during webhook delivery. - Adds unit tests covering the ARI retry fallback and webhook failure/exception handling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Acmebot.Acme/AcmeProtocolException.cs | Adds a convenience property to detect the alreadyReplaced ACME problem type. |
| src/Acmebot.App/Functions/Orchestration/AcmeOrderActivities.cs | Adds retry logic for alreadyReplaced by reissuing the order without ARI replaces. |
| src/Acmebot.App/Notifications/WebhookInvoker.cs | Ensures webhook payload creation and delivery exceptions are caught and logged (no orchestration failure). |
| tests/Acmebot.App.Tests/AcmeOrderActivitiesTests.cs | Verifies alreadyReplaced causes a retry without replaces in the second POST. |
| tests/Acmebot.App.Tests/WebhookInvokerTests.cs | Verifies webhook failures and payload builder exceptions do not throw. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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.
This pull request improves the robustness and reliability of ACME order handling and webhook delivery in the application. It introduces enhanced error handling for ACME order replacements, ensures webhook notifications do not throw on failure, and adds comprehensive tests for these scenarios.
ACME order handling improvements:
CreateOrderAsyncto detect when an ACME order replacement has already been consumed (using the newIsAlreadyReplacedproperty onAcmeProtocolException). If this occurs, the order is retried without thereplaceshint, and a warning is logged. (src/Acmebot.Acme/AcmeProtocolException.cs[1]src/Acmebot.App/Functions/Orchestration/AcmeOrderActivities.cs[2] [3]replacesfield. (tests/Acmebot.App.Tests/AcmeOrderActivitiesTests.cstests/Acmebot.App.Tests/AcmeOrderActivitiesTests.csR1-R162)Webhook notification reliability:
WebhookInvokerto use a payload factory and to catch and log any exceptions during webhook delivery, ensuring that failures or exceptions do not cause the orchestration to fail. (src/Acmebot.App/Notifications/WebhookInvoker.cs[1] [2]tests/Acmebot.App.Tests/WebhookInvokerTests.cstests/Acmebot.App.Tests/WebhookInvokerTests.csR1-R116)Fixes #1183