Skip to content

Comments

fix: close HTTP response body in health check for core-keeper#5328

Merged
judehung merged 1 commit intoedgexfoundry:mainfrom
remo-lab:fix/registry-healthcheck-response-body-close
Dec 16, 2025
Merged

fix: close HTTP response body in health check for core-keeper#5328
judehung merged 1 commit intoedgexfoundry:mainfrom
remo-lab:fix/registry-healthcheck-response-body-close

Conversation

@remo-lab
Copy link
Contributor

@remo-lab remo-lab commented Dec 15, 2025

Fixes a resource leak in the Core Keeper registry health check where the HTTP response body was not closed.
Since health checks run periodically per registered service, this caused file descriptors to accumulate over time, potentially leading to “too many open files” errors.
The fix ensures the response body is always closed on all execution paths. Unit tests were added to validate the behavior.

Fixes #5327

✔️ ✔️

The Core Keeper registry health check performs periodic HTTP requests for each registered service but did not close the HTTP response body on any execution path.
Since these checks run continuously in background goroutines, the unclosed response bodies caused file descriptors to accumulate over time.
In production deployments with multiple services, this could lead to file descriptor exhaustion, resulting in failed HTTP requests, degraded health monitoring, or service instability.
Closing the response body on all paths prevents this resource leak without altering existing behavior.

PR Checklist

Please check if your PR fulfills the following requirements:

  • [ ❌] I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
  • [❌ ] I am not introducing a new dependency (add notes below if you are)
  • [✔️ ] I have added unit tests for the new feature or bug fix (if not, why?)
  • [✔️ ] I have fully tested (add details below) this the new feature or bug fix (if not, why?)
  • I have opened a PR for the related docs change (if not, why?)

Testing Instructions

Run unit tests:

go test ./internal/core/keeper/registry/... -v
screenshot of terminal after the fix:
<img width="1191" height="1250" alt="image" src="https://github.com/user-attachments/assets/778600b0-0d6b-4c12-b192-b72e03b1ae88" />

@remo-lab remo-lab force-pushed the fix/registry-healthcheck-response-body-close branch from 2e3e2a9 to 5514af4 Compare December 15, 2025 23:15
@cloudxxx8 cloudxxx8 requested a review from judehung December 15, 2025 23:30
@remo-lab
Copy link
Contributor Author

Hi @judehung , This PR addresses a file descriptor leak in the Core Keeper registry health check and includes unit tests to verify the fix. Please let me know if any changes are needed. Thanks!

Copy link
Member

@judehung judehung left a comment

Choose a reason for hiding this comment

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

@remo-lab

Thanks for the fix—your changes look reasonable to me. However, registry_test.go is not formatted according to gofmt, which is causing the PR checks to fail.

You can run the following command in your environment to format registry_test.go correctly.:

 gofmt -w ./internal/core/keeper/registry/registry_test.go

@@ -0,0 +1,422 @@
//
// Copyright (C) 2024 IOTech Ltd
Copy link
Member

Choose a reason for hiding this comment

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

Please update the copyright header to the correct year

Suggested change
// Copyright (C) 2024 IOTech Ltd
// Copyright (C) 2025 IOTech Ltd

@remo-lab remo-lab force-pushed the fix/registry-healthcheck-response-body-close branch from 0bb72fb to f3869a9 Compare December 16, 2025 10:28
@remo-lab
Copy link
Contributor Author

Hi @judehung ,Thanks for the review! I've addressed your feedback:

  • Updated the copyright year from 2024 to 2025 in registry_test.go
  • Verified formatting with gofmt -w - the file was already correctly formatted, so no changes were needed there.
    screenshot of terminal:
image

The copyright year is now updated. Please let me know if there's anything else that needs to be addressed.

@judehung
Copy link
Member

@remo-lab
To comply with Semantic PR requirements, please update the PR title and commit messages to:

fix: close HTTP response body in health check for core-keeper

Note that registry is not a valid scope as defined in this repository.

@remo-lab remo-lab changed the title fix(registry): close HTTP response body in health check fix: close HTTP response body in health check for core-keeper Dec 16, 2025
@remo-lab remo-lab force-pushed the fix/registry-healthcheck-response-body-close branch from f3869a9 to 14b6209 Compare December 16, 2025 11:12
@remo-lab
Copy link
Contributor Author

HI @judehung ,Thanks for the review and guidance. I’ve updated the PR title and squashed the commits to comply with the Semantic PR requirements. Please let me know if anything else needs adjustment.

Signed-off-by: remo-lab <remopanda7@gmail.com>
@remo-lab remo-lab force-pushed the fix/registry-healthcheck-response-body-close branch from 14b6209 to bf871de Compare December 16, 2025 11:16
@sonarqubecloud
Copy link

@judehung judehung self-requested a review December 16, 2025 11:35
Copy link
Member

@judehung judehung left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. LGTM

@judehung judehung merged commit 2d48adb into edgexfoundry:main Dec 16, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Core Keeper health check does not close HTTP response bodies

2 participants