Skip to content

Fix Kafka AppHost health-check producer lifetime - #20092

Merged
James Newton-King (JamesNK) merged 1 commit into
mainfrom
davidfowl-kafka-health-check-lifetime
Sep 14, 2026
Merged

James Newton-King (JamesNK) merged 1 commit into
mainfrom
davidfowl-kafka-health-check-lifetime

Conversation

@davidfowl

Copy link
Copy Markdown
Collaborator

Description

Kafka AppHost health checks currently create a new producer on every execution without disposing it, accumulating polling threads over time. Register each resource's KafkaHealthCheck as a DI-owned keyed singleton so checks reuse their producer and AppHost disposal releases it.

This preserves independent configuration for multiple Kafka resources and deferred connection-string initialization, following the existing client integration pattern. No public API, dependency, or producer-default changes are required.

Fixes: #20091

Validation

  • Added public-API regression coverage for DI ownership, reuse across scopes, resource isolation, and connection-string availability. No private reflection.
  • All 28 selected Kafka hosting tests passed, including broker-backed health and WaitFor readiness tests.
  • Ran standalone AppHosts with two real Kafka 8.2.0 brokers each, comparing released 13.5.3 with the patched source. Both used Confluent.Kafka 2.15.0, normal resource monitoring, and 40 additional rounds through HealthCheckService. All 80 additional probe results were healthy.
Live measurement Released 13.5.3 Patched
Total health-check executions 84 84
Distinct health-check instances 84 2
Producer polling threads after the checks 84 2
Producer polling threads after AppHost disposal 84 0

Polling threads were identified using dotnet-stack while the process remained alive after AppHost disposal. The standalone verification used no private reflection. Detailed evidence is attached to #20091. This verifies the lifetime leak, not the original report's exact CPU percentage.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No

@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20092

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 20092"

@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 13, 2026
@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot September 13, 2026 14:30
@github-actions

This comment has been minimized.

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.

Copilot review overview

Review tier: Balanced
Findings: None


💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

@davidfowl

Copy link
Copy Markdown
Collaborator Author

PR Testing Report

PR Information

  • PR: Fix Kafka AppHost health-check producer lifetime #20092 - Fix Kafka AppHost health-check producer lifetime
  • Head commit: 15c2c650290d5e6bd9b702f8b7c7a197b615ce4b
  • Tested at: September 13, 2026, approximately 15:05-15:09 UTC
  • Environment: macOS ARM64, Docker Engine 28.5.1, local isolated PR CLI install, two real Kafka 8.2.0 containers.

Artifact Version Verification

  • Installed CLI: 13.6.0-pr.20092.g15c2c650, matching the PR head.
  • Kafka package: Aspire.Hosting.Kafka.13.6.0-pr.20092.g15c2c650.nupkg from the downloaded PR hive.
  • Package repository commit: 4848901472371aa592bec6e697abadea5a30cd06. This is the CI merge commit with parents 47a04482488cd63c5b844091c154fcdc018dc718 and the PR head 15c2c650290d5e6bd9b702f8b7c7a197b615ce4b, not a stale package.
  • Artifact-producing run: https://github.com/microsoft/aspire/actions/runs/34762863086
  • Status: Verified. The PR head was checked again after testing and had not changed.

Changes Analyzed

  • src/Aspire.Hosting.Kafka/KafkaBuilderExtensions.cs: registers a DI-owned keyed singleton per Kafka resource and resolves it from the health-check factory.
  • tests/Aspire.Hosting.Kafka.Tests/AddKafkaTests.cs: public-API coverage for ownership, scoped reuse, distinct resources, and deferred connection availability.
  • Categories: Kafka hosting integration and tests only. No CLI, dashboard, template, client integration, extension, or CI-infrastructure changes.

Test Scenario Executed

Fresh packaged AppHost lifecycle, including outage and recovery

Objective: Validate the shipped PR artifacts, real resource orchestration, dashboard state, health-check reuse, and disposal without private reflection.

Status: Passed.

Created a fresh file-based C# AppHost using the installed PR CLI and the explicit PR template hive:

"$cli" new aspire-empty --name KafkaPrSmoke --output "$temp/KafkaPrSmoke" \
  --source "$hive" --version 13.6.0-pr.20092.g15c2c650 \
  --language csharp --localhost-tld false --suppress-agent-init --non-interactive

Added the PR-version Kafka package and two AddKafka resources. Started the app with:

"$cli" start --apphost "$apphost" --launch-profile http --isolated --format Json --non-interactive

The temporary harness observed public health-check factory results and captured managed stacks using dotnet-stack. It did not replace the checks, inspect private fields, or change producer configuration. It set the public health-check registration timeout to three seconds to bound unavailable-broker probes. Normal Aspire health monitoring remained enabled.

All phases deliberately used the same fresh AppHost so reuse could be verified across failure and recovery:

Phase Coverage Expected and observed result
Before connection availability Boundary Resolving each check throws InvalidOperationException("Connection string is unavailable"). AppHost subsequently starts successfully.
Startup and dashboard Happy path Both real Kafka containers reach Running/Healthy. Dashboard displays both resources.
Repeated healthy probes Happy path 40 rounds, checking both brokers each time, all return Healthy. Exactly one check instance per resource and two producer polling threads.
Stop broker A Unhappy path aspire resource kafka-a stop moves A to Exited; B remains Running/Healthy. Dashboard confirms the isolated failure state.
Repeated unavailable-broker probes Unhappy path Four rounds consistently return A=Unhealthy and B=Healthy. Still one instance per resource and two polling threads.
Restart and recovery Recovery aspire resource kafka-a start, then aspire wait kafka-a --status healthy --timeout 180, succeeds in about 5.7 seconds. Ten further rounds return Healthy for both resources without replacing the checks. Original endpoint allocations are retained.
AppHost disposal Lifecycle StopAsync and DisposeAsync remove both containers. While the console process is still alive, polling threads drop from two to zero.

All CLI follow-up commands supplied the explicit --apphost path.

Lifetime observations

Counts include normal background monitoring as well as explicit probe rounds.

Checkpoint Factory executions Distinct checks Kafka producer polling threads
Ready 3 2 2
After 40 healthy rounds 87 2 2
After four outage rounds 98 2 2
After ten recovery rounds 122 2 2
After AppHost disposal 124 2 0

The captured thread stacks contain Confluent.Kafka.Producer<,>...<StartPollTask>b__0. These are thread-lifetime observations, not a reproduction of the original report's exact CPU percentage.

Evidence

Preserved under the session's files/pr20092-evidence/:

  • apphost.cs: standalone packaged-PR harness.
  • apphost-run.log: sanitized phase/probe output, including PRTEST PASS all scenarios.
  • observations.json and checkpoint *.done files: exact counts.
  • ready.stacks.txt, healthy-probes.stacks.txt, outage-probes.stacks.txt, recovery-probes.stacks.txt, disposed.stacks.txt: live managed stack captures.
  • dashboard-healthy.png, dashboard-outage.png, dashboard-recovered.png: dashboard screenshots.
  • dashboard-outage.txt, dashboard-recovered.txt: browser accessibility snapshots.

Installation, template creation, broker lifecycle, and aspire describe logs are retained alongside this folder as pr20092-*.log. Temporary login-bearing browser/startup logs were removed.

Summary

Validation Status
CLI and Kafka package provenance Passed
Fresh PR-template project and real broker startup Passed
Missing connection-string boundary Passed
Repeated healthy probes and independent resource instances Passed
Unavailable broker isolation Passed
Broker recovery using the same checks Passed
Producer polling-thread cleanup on AppHost disposal Passed

Overall Result

PR Kafka behavior verified against the packaged artifacts.

At the end of this test run, CI had failed VS Code browser-debugger E2E jobs on Linux and Windows, with the aggregate test/final checks consequently failed. Those jobs are outside the changed Kafka area and were not diagnosed by this focused run; their cause is not established here.

Cleanup

The AppHost and detached CLI processes exited; both owned broker containers were removed; the isolated headless browser was closed. The temporary project and PR installation were deleted. Evidence and the harness were retained in session artifacts. The repository working tree remained clean.

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.

Copilot review overview

🟢 Approval recommended

The focused implementation follows the established Kafka client pattern and includes appropriate regression coverage.

Review tier: Balanced
Findings: None

@davidfowl

Copy link
Copy Markdown
Collaborator Author

/backport to release/13.5

@github-actions

Copy link
Copy Markdown
Contributor

Started backporting to release/13.5 (link to workflow run)

Register a DI-owned keyed singleton per Kafka resource so repeated health checks reuse their producer and AppHost disposal releases it. Add public-API coverage for singleton ownership, resource isolation, and deferred connection-string availability.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

Tests selector

2 / 99 PR test projects · 2 PR jobs · 0 advisory-only targets, from 2 changed files.

Selected PR test projects (2 / 99)

Aspire.Hosting.Kafka.Tests, Aspire.Playground.Tests

Selected PR jobs (2)

extension-e2e, typescript-api-compat

Advisory workflow impact (0)

none


How these were chosen — grouped by what changed

🔧 src/Aspire.Hosting.Kafka/KafkaBuilderExtensions.cs (changed source)
1 directly: Aspire.Hosting.Kafka.Tests
1 via the project graph: Aspire.Playground.Tests (2 hops)

🧪 tests/Aspire.Hosting.Kafka.Tests/AddKafkaTests.cs (changed test)
1 directly: Aspire.Hosting.Kafka.Tests

Job reasons

Job Triggered by
extension-e2e src/Aspire.Hosting.Kafka/KafkaBuilderExtensions.cs
• affected project Aspire.Hosting.Kafka
typescript-api-compat affected project Aspire.Hosting.Kafka

Selection computed for commit f1aa806.

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.

Copilot review overview

🟢 Approval recommended

The focused lifecycle fix follows existing Kafka integration patterns and has appropriate regression coverage.

Review tier: Balanced
Findings: None

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed the Kafka health-check lifetime change and regression coverage. 0 issues found.

@JamesNK
James Newton-King (JamesNK) merged commit 3ae6d1f into main Sep 14, 2026
240 of 246 checks passed
@JamesNK
James Newton-King (JamesNK) deleted the davidfowl-kafka-health-check-lifetime branch September 14, 2026 03:33
@github-actions github-actions Bot added this to the 13.6 milestone Sep 14, 2026
This was referenced Sep 16, 2026
@mahdiyar021

Copy link
Copy Markdown

I think this PR may also fixes this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Kafka AppHost health checks leak producer threads and cause high CPU

4 participants