Skip to content

Dispose the ACME client and signer when context creation fails - #1222

Merged
shibayan merged 2 commits into
masterfrom
fix/acme-client-factory-leak
Jul 26, 2026
Merged

Dispose the ACME client and signer when context creation fails#1222
shibayan merged 2 commits into
masterfrom
fix/acme-client-factory-leak

Conversation

@shibayan

Copy link
Copy Markdown
Member

Problem

AcmeClientFactory.CreateClientCoreAsync constructs an AcmeClient — which owns an HttpClient and its handler — and only afterwards fetches the directory and creates or loads the account. The result is assigned to the cached field via _clientContext ??= await CreateClientCoreAsync(), so it is only reachable on success.

Any failure in between leaves the client and the signer unreachable and undisposed:

  • the directory fetch against an unreachable or misbehaving ACME endpoint
  • the PreconditionException thrown when the endpoint requires EAB and none is configured
  • an account creation failure or a state store write failure

Because CreateClientAsync is called from every activity, a persistently failing ACME endpoint accumulates one HttpClient and one SocketsHttpHandler per attempt for the lifetime of the worker.

Change

Wrap the construction in try/catch and dispose both the client and the signer on the failure path. AcmeClientContext.Dispose() disposes both, so the failure path is symmetric with the success path. The client is declared nullable so a throw from the AcmeClient constructor itself is covered too.

🤖 Generated with Claude Code

CreateClientCoreAsync built an AcmeClient, which owns an HttpClient, before
fetching the directory and creating the account. The context is only assigned
to the cached field on success, so any failure in between left the client and
its signer unreachable and undisposed. Against a persistently failing ACME
endpoint this accumulated one HttpClient per retry for the life of the worker.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 26, 2026 03:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves reliability of AcmeClientFactory.CreateClientCoreAsync by ensuring resources created during client-context construction are disposed when context creation fails, preventing repeated HttpClient/handler leaks on persistent failures.

Changes:

  • Wrap ACME client context construction in try/catch to dispose the AcmeClient and signer when intermediate steps fail.
  • Make the AcmeClient local nullable to cover exceptions thrown during the client constructor.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Acmebot.App/Acme/AcmeClientFactory.cs Outdated
@shibayan shibayan self-assigned this Jul 26, 2026
@shibayan shibayan added the bug Something isn't working label Jul 26, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 26, 2026 03:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@shibayan
shibayan merged commit 71e77ef into master Jul 26, 2026
11 checks passed
@shibayan
shibayan deleted the fix/acme-client-factory-leak branch July 26, 2026 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants