Skip to content

feat(health,version): add health and version endpoints#155

Merged
drtechie merged 13 commits intoPSMRI:mainfrom
DurgaPrasad-54:feat/health-version
Mar 2, 2026
Merged

feat(health,version): add health and version endpoints#155
drtechie merged 13 commits intoPSMRI:mainfrom
DurgaPrasad-54:feat/health-version

Conversation

@DurgaPrasad-54
Copy link
Contributor

@DurgaPrasad-54 DurgaPrasad-54 commented Feb 19, 2026

📋 Description

JIRA ID:

This PR implements /health and /version endpoints in the MMU-API as part of Issue PSMRI/AMRIT#102

What’s changed

  • Add /health endpoint
  • Add /version endpoint

Summary by CodeRabbit

  • New Features

    • Added /health (component-aware status with per-component results) and /version (build timestamp, version, branch, commit) endpoints.
  • Chores

    • Removed Spring Actuator dependency.
    • Made /health and /version publicly accessible.
    • Added build-time git commit metadata generation.
    • Adjusted Maven artifact naming.
    • Removed Checkstyle UTF-8 encoding setting.

@coderabbitai
Copy link

coderabbitai bot commented Feb 19, 2026

Warning

Rate limit exceeded

@DurgaPrasad-54 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 9 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📝 Walkthrough

Walkthrough

Adds /health and /version REST endpoints with a concurrent HealthService (MySQL + optional Redis); exposes those endpoints publicly in security and JWT filter; updates pom.xml (removes actuator, changes finalName, removes checkstyle encoding, adds git-commit-id-maven-plugin).

Changes

Cohort / File(s) Summary
Build Configuration
pom.xml
Removed spring-boot-starter-actuator; changed <finalName> to ${project.artifactId}-${project.version}; removed checkstyle <encoding>; added io.github.git-commit-id:git-commit-id-maven-plugin v7.0.0 with an initialize execution and selective properties.
Health Service
src/main/java/com/iemr/mmu/service/health/HealthService.java
New service performing concurrent health checks (lightweight MySQL query + optional Redis PING), with timeouts, cancellation, advanced MySQL checks (throttled/cached), per-component status/severity/timings, and public checkHealth() API.
Controllers
src/main/java/com/iemr/mmu/controller/health/HealthController.java, src/main/java/com/iemr/mmu/controller/version/VersionController.java
Added GET /health (aggregates HealthService result; maps DOWN→503 otherwise 200) and GET /version (loads git.properties from classpath; defaults to unknown on missing/error); include logging and OpenAPI annotations.
Security & Filters
src/main/java/com/iemr/mmu/utils/mapper/SecurityConfig.java, src/main/java/com/iemr/mmu/utils/JwtUserIdValidationFilter.java
Expanded public endpoints to include /health and /version; JWT filter skip logic updated to treat these endpoints as public.

Sequence Diagram(s)

sequenceDiagram
    actor Client
    participant HC as HealthController
    participant HS as HealthService
    participant DS as DataSource
    participant RT as RedisTemplate

    Client->>HC: GET /health
    HC->>HS: checkHealth()
    activate HS
    par concurrent checks
        HS->>DS: lightweight SQL (3s timeout)
        HS->>RT: PING (if configured, 3s timeout)
    end
    DS-->>HS: SQL result / error
    RT-->>HS: PONG / timeout / error
    HS->>HS: aggregate statuses, timings, severity
    deactivate HS
    HS-->>HC: health map (status, timestamp, components)
    HC-->>Client: ResponseEntity (200 or 503 + health JSON)
Loading
sequenceDiagram
    actor Client
    participant VC as VersionController
    participant Loader as GitPropertiesLoader
    participant Classpath as ClasspathResource

    Client->>VC: GET /version
    VC->>Loader: load git.properties from classpath
    Loader->>Classpath: read git.properties
    alt git.properties found
        Classpath-->>Loader: properties (git.build.time, git.build.version, git.branch, git.commit.id.abbrev)
        Loader-->>VC: populated map
    else not found / error
        Classpath-->>Loader: missing / error
        Loader-->>VC: defaults ("unknown")
    end
    VC-->>Client: ResponseEntity (200 + version JSON)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through lines and found two new gates,
Health checks racing DB and Redis for fates,
Version whispers from git tucked neat inside,
Maven trimmed, plugins added, endpoints open wide,
A rabbit cheers: ping, build, and let tests glide!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.35% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(health,version): add health and version endpoints' directly and clearly describes the main changes—the addition of /health and /version endpoints, which aligns with the primary objective and the substantial code additions for these features.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)

67-93: Health checks run sequentially despite having a thread pool.

checkHealth() calls performHealthCheck for MySQL then Redis in sequence (both synchronously via checker.get()). The CompletableFuture inside checkRedisHealth adds async wrapping but the caller blocks on future.get() anyway, so total latency is MySQL-timeout + Redis-timeout in the worst case (6s).

If concurrent checking was intended, consider submitting both checks via CompletableFuture and joining. If sequential was intentional, the CompletableFuture in checkRedisHealth only adds the timeout capability — consider documenting this or using a simpler Future approach.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines 67
- 93, checkHealth currently calls performHealthCheck sequentially for "MySQL"
and "Redis" (using checkMySQLHealth and checkRedisHealth) which blocks on each
checker.get() so checks run serially; to run concurrently, submit both health
checks as CompletableFutures (e.g., use CompletableFuture.supplyAsync or submit
the existing check lambdas to your thread pool inside checkHealth), store
futures keyed by component name, wait for all with
CompletableFuture.allOf(...).join(), then populate the components map from each
future result and compute overall status with isHealthy and
STATUS_KEY/STATUS_UP/STATUS_DOWN; alternatively, if sequential behavior is
intended, remove the unnecessary CompletableFuture wrapping from
checkRedisHealth or document that it only provides timeout behaviour.
src/main/java/com/iemr/mmu/controller/health/HealthController.java (1)

60-70: Consider DEBUG level for routine health-check access logging (Line 61).

Load balancers and monitoring systems typically poll /health every few seconds. Logging at INFO on every invocation will generate significant log noise. DEBUG is more appropriate for the entry log; the WARN/ERROR paths in HealthService already capture failures at the right level.

Proposed fix
-        logger.info("Health check endpoint called");
+        logger.debug("Health check endpoint called");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java` around
lines 60 - 70, The entry log in HealthController.checkHealth currently uses
logger.info and should be lowered to logger.debug to avoid noisy INFO-level logs
from frequent health checks; update the log call at the start of the checkHealth
method (the statement that currently logs "Health check endpoint called") to use
debug level while leaving the existing debug/warn/error logs in HealthService
and the method unchanged otherwise.
src/main/java/com/iemr/mmu/controller/version/VersionController.java (1)

49-67: git.properties is re-read from classpath on every request — consider loading once at startup.

The git properties are static for the lifetime of the application. Loading them in the constructor (or a @PostConstruct method) and caching the Properties object avoids repeated classpath I/O.

Proposed fix
 `@RestController`
 public class VersionController {
 
 	private final Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName());
 	private static final String UNKNOWN_VALUE = "unknown";
+	private final Map<String, String> versionInfo;
+
+	public VersionController() {
+		this.versionInfo = loadVersionInfo();
+	}
+
+	private Map<String, String> loadVersionInfo() {
+		Map<String, String> info = new LinkedHashMap<>();
+		try {
+			Properties gitProperties = loadGitProperties();
+			info.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE));
+			info.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE));
+			info.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE));
+			info.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE));
+		} catch (Exception e) {
+			logger.error("Failed to load version information", e);
+			info.put("buildTimestamp", UNKNOWN_VALUE);
+			info.put("version", UNKNOWN_VALUE);
+			info.put("branch", UNKNOWN_VALUE);
+			info.put("commitHash", UNKNOWN_VALUE);
+		}
+		return info;
+	}
 
 	`@Operation`(summary = "Get version information")
 	`@GetMapping`(value = "/version", produces = MediaType.APPLICATION_JSON_VALUE)
 	public ResponseEntity<Map<String, String>> versionInformation() {
-		Map<String, String> response = new LinkedHashMap<>();
-		try {
-			logger.info("version Controller Start");
-			Properties gitProperties = loadGitProperties();
-			response.put("buildTimestamp", gitProperties.getProperty("git.build.time", UNKNOWN_VALUE));
-			...
-		} catch (Exception e) { ... }
-		logger.info("version Controller End");
-		return ResponseEntity.ok(response);
+		return ResponseEntity.ok(versionInfo);
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/controller/version/VersionController.java` around
lines 49 - 67, The versionInformation() method currently calls
loadGitProperties() on every request; move loading of git.properties into a
one-time initialization and cache it on the controller (e.g., load in the
VersionController constructor or a `@PostConstruct` method into a private final
Properties field like cachedGitProperties), update versionInformation() to read
from cachedGitProperties and fall back to UNKNOWN_VALUE if null, and remove
repeated I/O by deleting the per-request call to loadGitProperties(); keep
existing keys (git.build.time, git.build.version, git.branch,
git.commit.id.abbrev) and existing error fallback logic referencing
UNKNOWN_VALUE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 109-111: In HealthService (checkMySQLHealth and checkRedisHealth)
avoid returning raw exception messages in HealthCheckResult.result/error and the
JSON status map; instead log the full exception with logger.warn or logger.error
including e (e.g., logger.warn("MySQL health check failed", e)) and set the
HealthCheckResult(error) to a sanitized generic string like "MySQL connection
failed" or "Redis connection failed"; also change where status.put("error",
safeError) is populated to use that sanitized message while keeping the full
exception only in logs (update usages of HealthCheckResult, result.error and the
status.put call to use the generic message).
- Around line 56-65: HealthService creates a fixed ExecutorService
(executorService) but never shuts it down; implement a lifecycle shutdown by
either having HealthService implement DisposableBean and overriding destroy() to
call executorService.shutdown() (and optionally await termination) or add a
`@PreDestroy-annotated` method (e.g., shutdownExecutor) that invokes
executorService.shutdown() and handles InterruptedException/timeout; update the
class constructor usage accordingly and ensure the executorService field is used
in that shutdown method to avoid thread leakage on application stop.

---

Nitpick comments:
In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java`:
- Around line 60-70: The entry log in HealthController.checkHealth currently
uses logger.info and should be lowered to logger.debug to avoid noisy INFO-level
logs from frequent health checks; update the log call at the start of the
checkHealth method (the statement that currently logs "Health check endpoint
called") to use debug level while leaving the existing debug/warn/error logs in
HealthService and the method unchanged otherwise.

In `@src/main/java/com/iemr/mmu/controller/version/VersionController.java`:
- Around line 49-67: The versionInformation() method currently calls
loadGitProperties() on every request; move loading of git.properties into a
one-time initialization and cache it on the controller (e.g., load in the
VersionController constructor or a `@PostConstruct` method into a private final
Properties field like cachedGitProperties), update versionInformation() to read
from cachedGitProperties and fall back to UNKNOWN_VALUE if null, and remove
repeated I/O by deleting the per-request call to loadGitProperties(); keep
existing keys (git.build.time, git.build.version, git.branch,
git.commit.id.abbrev) and existing error fallback logic referencing
UNKNOWN_VALUE.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 67-93: checkHealth currently calls performHealthCheck sequentially
for "MySQL" and "Redis" (using checkMySQLHealth and checkRedisHealth) which
blocks on each checker.get() so checks run serially; to run concurrently, submit
both health checks as CompletableFutures (e.g., use
CompletableFuture.supplyAsync or submit the existing check lambdas to your
thread pool inside checkHealth), store futures keyed by component name, wait for
all with CompletableFuture.allOf(...).join(), then populate the components map
from each future result and compute overall status with isHealthy and
STATUS_KEY/STATUS_UP/STATUS_DOWN; alternatively, if sequential behavior is
intended, remove the unnecessary CompletableFuture wrapping from
checkRedisHealth or document that it only provides timeout behaviour.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/main/java/com/iemr/mmu/service/health/HealthService.java (2)

150-152: performHealthCheck both mutates status and returns it — redundant return value.

All call sites in checkHealth() already hold a reference to the same map; the return value is never used. Changing the signature to void eliminates the implicit "this mutates in place AND returns" contract, which is easy to misread.

♻️ Proposed refactor
-private Map<String, Object> performHealthCheck(String componentName,
-                                                Map<String, Object> status,
-                                                Supplier<HealthCheckResult> checker) {
+private void performHealthCheck(String componentName,
+                                Map<String, Object> status,
+                                Supplier<HealthCheckResult> checker) {
     ...
-    return status;
     ...
-    return status;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines
150 - 152, performHealthCheck currently mutates the passed-in Map and also
returns it; change its signature from returning Map<String,Object> to void and
remove any return statements inside performHealthCheck, then update all call
sites in checkHealth() to stop using its return value (just call
performHealthCheck(componentName, status, checker)); ensure the method name
performHealthCheck and callers in checkHealth() are updated consistently and
compilation errors are fixed by removing any assignments expecting a return.

121-127: Swallowed exception in Redis PING lambda — actual failure cause is never logged.

The catch (Exception e) block on Lines 124-126 discards the exception entirely, returning null. When pong == null, checkRedisHealth reports "Redis PING failed" with no diagnostic detail in the logs — making it impossible to distinguish a connection error, an auth failure, or any other root cause.

🔍 Proposed fix — log before swallowing
 CompletableFuture<String> future = CompletableFuture.supplyAsync(() -> {
     try {
         return redisTemplate.execute((org.springframework.data.redis.core.RedisCallback<String>) connection -> connection.ping());
     } catch (Exception e) {
+        logger.warn("Redis PING execution failed: {}", e.getMessage());
         return null;
     }
 }, executorService);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines
121 - 127, The Redis PING lambda in CompletableFuture.supplyAsync swallows
exceptions, losing diagnostics; update the catch block inside the supplyAsync
lambda (the redisTemplate.execute lambda submitted to executorService in
checkRedisHealth) to log the caught Exception before returning null — e.g. use
the class logger (logger or log) to call error with a descriptive message and
the exception (include exception as parameter) so the root cause
(connection/auth/etc.) is visible in logs, then return null as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 137-139: In the TimeoutException catch block inside
HealthService's Redis health check, call future.cancel(true) before returning to
ensure the background CompletableFuture task is interrupted and frees the Redis
connection held by the executorService thread; update the catch for
TimeoutException to invoke future.cancel(true) (using the existing future
variable) and then log and return the HealthCheckResult as before.
- Around line 95-113: The MySQL health check in checkMySQLHealth currently calls
dataSource.getConnection() directly (and uses stmt.setQueryTimeout for query
execution), which can block the request thread while waiting for a connection;
wrap the whole check (connection acquisition, prepare/execute) in an
asynchronous task with a bounded deadline (e.g., CompletableFuture.supplyAsync
or runAsync plus .get(timeout, unit) or .orTimeout) similar to the Redis check
so the call fails fast; ensure you still set stmt.setQueryTimeout((int)
MYSQL_TIMEOUT_SECONDS) for query execution, catch and translate
timeout/ExecutionException/InterruptedException into a HealthCheckResult(false,
"...") and cancel the underlying task if the deadline is exceeded so resources
(including pending getConnection) are not left hanging.

---

Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 144-146: The catch block in HealthService is returning the raw
Redis exception message to the /health response; instead return a generic,
non-sensitive message (e.g., "Redis health check failed") in the
HealthCheckResult and stop exposing e.getMessage() there, while preserving
detailed diagnostics in logs by logging the exception object (e.g.,
logger.warn("Redis health check failed", e)) so the stack trace is available for
debugging; update the catch that constructs new HealthCheckResult(false, ...)
and the logger.warn call accordingly.
- Around line 56-65: The HealthService constructs a fixed-thread ExecutorService
(executorService) in the constructor but never shuts it down; add a lifecycle
shutdown to avoid thread leaks by implementing a destroy hook (e.g., add a
`@PreDestroy` method or implement DisposableBean#destroy) that calls shutdown()
and awaitTermination with a short timeout on the executorService, and force
shutdownNow() if it doesn’t terminate; reference the executorService field and
HealthService class/constructor when locating where to add the cleanup.
- Around line 109-111: The catch block in HealthService that currently returns
e.getMessage() exposes internal details on the public /health endpoint; instead
log the full exception (logger.warn("MySQL health check failed", e)) to capture
stacktrace, but return a non-sensitive generic message in the HealthCheckResult
such as "Database unavailable" or a sanitized error code; update the catch in
the health check method accordingly to stop returning e.getMessage() while
preserving internal logs for debugging.

---

Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 150-152: performHealthCheck currently mutates the passed-in Map
and also returns it; change its signature from returning Map<String,Object> to
void and remove any return statements inside performHealthCheck, then update all
call sites in checkHealth() to stop using its return value (just call
performHealthCheck(componentName, status, checker)); ensure the method name
performHealthCheck and callers in checkHealth() are updated consistently and
compilation errors are fixed by removing any assignments expecting a return.
- Around line 121-127: The Redis PING lambda in CompletableFuture.supplyAsync
swallows exceptions, losing diagnostics; update the catch block inside the
supplyAsync lambda (the redisTemplate.execute lambda submitted to
executorService in checkRedisHealth) to log the caught Exception before
returning null — e.g. use the class logger (logger or log) to call error with a
descriptive message and the exception (include exception as parameter) so the
root cause (connection/auth/etc.) is visible in logs, then return null as
before.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (3)
src/main/java/com/iemr/mmu/controller/health/HealthController.java (1)

68-68: Hardcoded "DOWN" literal duplicates the private STATUS_DOWN constant in HealthService.

If HealthService.STATUS_DOWN is ever renamed, this comparison silently breaks. Expose the constant (or extract a shared HealthConstants class) and reference it here.

♻️ Proposed refactor
// In HealthService.java
-    private static final String STATUS_DOWN = "DOWN";
+    static final String STATUS_DOWN = "DOWN";

// In HealthController.java
-        HttpStatus httpStatus = "DOWN".equals(overallStatus) ? HttpStatus.SERVICE_UNAVAILABLE : HttpStatus.OK;
+        HttpStatus httpStatus = HealthService.STATUS_DOWN.equals(overallStatus) ? HttpStatus.SERVICE_UNAVAILABLE : HttpStatus.OK;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java` at line
68, The controller currently compares overallStatus to a hardcoded "DOWN"
string; make this comparison use the central constant instead to avoid drift:
either change the private STATUS_DOWN in HealthService to a public static final
(e.g., HealthService.STATUS_DOWN) and update the comparison in HealthController
(replace "DOWN" with HealthService.STATUS_DOWN), or extract the constant into a
shared HealthConstants class (e.g., HealthConstants.STATUS_DOWN), import that in
HealthController and use HealthConstants.STATUS_DOWN in the HttpStatus selection
logic where overallStatus is compared; ensure you update references and imports
accordingly.
src/main/java/com/iemr/mmu/service/health/HealthService.java (2)

96-96: ADVANCED_HEALTH_CHECKS_ENABLED is dead code — cannot be toggled without recompiling.

The static final boolean = true constant makes the guard at Line 320 unreachable at compile time. Consider externalising it as a Spring property:

♻️ Proposed refactor
-    private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = true;
+    `@Value`("${health.advanced-checks.enabled:true}")
+    private boolean advancedHealthChecksEnabled;
-        if (!ADVANCED_HEALTH_CHECKS_ENABLED) {
+        if (!advancedHealthChecksEnabled) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` at line 96, The
ADVANCED_HEALTH_CHECKS_ENABLED static final flag is dead code; replace it with a
configurable Spring property to allow toggling at runtime. Remove the static
final ADVANCED_HEALTH_CHECKS_ENABLED constant in HealthService, add an instance
boolean field injected from configuration (e.g.
`@Value`("${app.health.advanced:false}") private boolean
advancedHealthChecksEnabled or constructor-inject `@Value`) and update the
guard(s) that reference ADVANCED_HEALTH_CHECKS_ENABLED (e.g., the check
currently guarding advanced checks in HealthService) to use the new instance
field so the behavior can be toggled via application properties.

200-218: Redis "not configured" is reported as UP — consider a SKIPPED status for clarity.

When redisTemplate is null, the component appears in the response as "status": "UP" with a message. Consumers (monitoring dashboards, load balancers) may incorrectly interpret this as a live Redis connection. A dedicated "SKIPPED" status (or omitting the component entirely) is less ambiguous.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines
200 - 218, The current checkRedisHealthSync method returns an "UP"
HealthCheckResult when redisTemplate is null, which is misleading; update
checkRedisHealthSync (and if needed the HealthCheckResult class) so that when
redisTemplate == null it returns a result that represents a SKIPPED status
(e.g., a dedicated SKIPPED enum/state or a skipped flag) instead of reporting UP
— change the redisTemplate null branch in checkRedisHealthSync to
construct/return a HealthCheckResult that conveys SKIPPED with the message
"Redis not configured — skipped" (and adjust HealthCheckResult
constructors/serialization if required) so monitoring consumers see SKIPPED for
the redis component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java`:
- Around line 60-62: In HealthController.checkHealth replace the noisy
logger.info("Health check endpoint called") with logger.debug(...) so
health-check probes no longer emit INFO logs; locate the method checkHealth in
class HealthController and change the logging call to logger.debug while leaving
the surrounding method behavior intact (no other code changes).

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 410-439: The hasDeadlocks(Connection) method currently treats any
historical "LATEST DETECTED DEADLOCK" as a current failure and uses fragile
message matching for permission errors; update it to either remove the check
entirely or make it time-bounded and robust: replace the message-based
permission detection with numeric SQL error code checks (e.getErrorCode() ==
1142 || e.getErrorCode() == 1227) in the SQLException handler, and if keeping
the deadlock check parse the InnoDB status blob from hasDeadlocks() to extract
the deadlock timestamp (or switch to querying performance_schema/innodb_metrics)
and only return true when the detected deadlock timestamp is within a recent
threshold (e.g., last N minutes); also keep the deadlockCheckDisabled boolean
behavior but set it only on numeric error codes. Ensure these changes are made
inside the hasDeadlocks method and reference the deadlockCheckDisabled flag and
the SHOW ENGINE INNODB STATUS handling code.
- Around line 122-168: The timeout path has two issues: shared LinkedHashMap
writes from background tasks (mysqlStatus/redisStatus) create a data-race, and
CompletableFuture.cancel(true) won’t interrupt worker threads so executor
threads can be starved; fix by making the health tasks return results and using
Future-based cancellation: change checkHealth to submit callables via
executorService.submit(...) that call performHealthCheck/checkMySQLHealthSync
and checkRedisHealthSync and return their status maps (use ConcurrentHashMap for
returned maps or construct new LinkedHashMap only on the caller thread), keep
the Future references, call future.get(timeout, TimeUnit.SECONDS) and on
TimeoutException call future.cancel(true), then merge each Future’s returned map
into mysqlStatus/redisStatus before calling ensurePopulated and
computeOverallStatus (refer to methods checkHealth, performHealthCheck,
checkMySQLHealthSync, checkRedisHealthSync, ensurePopulated,
computeOverallStatus, and executorService).

---

Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 194-196: The current catch block in HealthService correctly logs
the full exception via logger.warn("MySQL health check failed: {}",
e.getMessage(), e) while returning a sanitized, generic HealthCheckResult(false,
"MySQL connection failed", false); no code change is required—keep the existing
logger.warn usage and the generic HealthCheckResult return as-is to preserve
detailed logs and prevent information disclosure.
- Around line 105-120: The HealthService.shutdown method annotated with
`@PreDestroy` properly addresses the thread-leak by checking executorService,
calling executorService.shutdown(), awaiting termination for 5 seconds, falling
back to executorService.shutdownNow(), and handling InterruptedException by
re-interrupting the thread; no further code changes are required for this issue
(ensure the logic remains in the shutdown() method and references to
executorService, awaitTermination, shutdownNow, and the `@PreDestroy` annotation
are preserved).

---

Nitpick comments:
In `@src/main/java/com/iemr/mmu/controller/health/HealthController.java`:
- Line 68: The controller currently compares overallStatus to a hardcoded "DOWN"
string; make this comparison use the central constant instead to avoid drift:
either change the private STATUS_DOWN in HealthService to a public static final
(e.g., HealthService.STATUS_DOWN) and update the comparison in HealthController
(replace "DOWN" with HealthService.STATUS_DOWN), or extract the constant into a
shared HealthConstants class (e.g., HealthConstants.STATUS_DOWN), import that in
HealthController and use HealthConstants.STATUS_DOWN in the HttpStatus selection
logic where overallStatus is compared; ensure you update references and imports
accordingly.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Line 96: The ADVANCED_HEALTH_CHECKS_ENABLED static final flag is dead code;
replace it with a configurable Spring property to allow toggling at runtime.
Remove the static final ADVANCED_HEALTH_CHECKS_ENABLED constant in
HealthService, add an instance boolean field injected from configuration (e.g.
`@Value`("${app.health.advanced:false}") private boolean
advancedHealthChecksEnabled or constructor-inject `@Value`) and update the
guard(s) that reference ADVANCED_HEALTH_CHECKS_ENABLED (e.g., the check
currently guarding advanced checks in HealthService) to use the new instance
field so the behavior can be toggled via application properties.
- Around line 200-218: The current checkRedisHealthSync method returns an "UP"
HealthCheckResult when redisTemplate is null, which is misleading; update
checkRedisHealthSync (and if needed the HealthCheckResult class) so that when
redisTemplate == null it returns a result that represents a SKIPPED status
(e.g., a dedicated SKIPPED enum/state or a skipped flag) instead of reporting UP
— change the redisTemplate null branch in checkRedisHealthSync to
construct/return a HealthCheckResult that conveys SKIPPED with the message
"Redis not configured — skipped" (and adjust HealthCheckResult
constructors/serialization if required) so monitoring consumers see SKIPPED for
the redis component.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)

96-96: Consider externalising ADVANCED_HEALTH_CHECKS_ENABLED as a configurable property

The flag is hardcoded to true and cannot be disabled at deploy time without recompilation. If the diagnostic queries (INFORMATION_SCHEMA.PROCESSLIST, SHOW ENGINE INNODB STATUS) cause unexpected load on a production host, there's no runtime escape hatch.

♻️ Proposed refactor
-    private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = true;
+    `@org.springframework.beans.factory.annotation.Value`("${health.advanced-checks.enabled:true}")
+    private boolean advancedHealthChecksEnabled;

Then reference advancedHealthChecksEnabled in performAdvancedMySQLChecksWithThrottle() instead of the constant.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` at line 96,
Replace the hardcoded static constant ADVANCED_HEALTH_CHECKS_ENABLED with a
configurable instance property (e.g., advancedHealthChecksEnabled) that is
loaded from configuration at startup (application.properties/@Value,
Environment, or System.getenv/System.getProperty) with a sensible default
(true). Remove or rename the static constant and initialize
advancedHealthChecksEnabled in the HealthService constructor or via DI so it can
be changed at deploy time, then update performAdvancedMySQLChecksWithThrottle()
to reference advancedHealthChecksEnabled instead of
ADVANCED_HEALTH_CHECKS_ENABLED; ensure the new field is final/immutable after
initialization and has a fallback default if the config key is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 138-153: The two health-check futures (mysqlFuture and
redisFuture) currently each call get(maxTimeout, TimeUnit.SECONDS) which can
double the total wait; change to a shared deadline approach in HealthService:
record a deadline (e.g., using System.nanoTime() +
TimeUnit.SECONDS.toNanos(maxTimeout)), then when waiting for each future compute
the remaining time as deadline - System.nanoTime(), convert to the appropriate
TimeUnit and pass that remaining budget into future.get(...); if remaining <= 0,
treat as a timeout and cancel both futures (mysqlFuture.cancel(true),
redisFuture.cancel(true)) and handle the TimeoutException/cleanup as before so
total wall-clock wait never exceeds maxTimeout.

---

Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 412-439: hasDeadlocks currently flags any historical deadlock
because it looks for the literal "LATEST DETECTED DEADLOCK" in the output of
SHOW ENGINE INNODB STATUS; update hasDeadlocks to either (A) parse the timestamp
inside the "LATEST DETECTED DEADLOCK" block from the innodbStatus string and
only return true when that timestamp is within a recent configurable window
(e.g., last X minutes), or (B) replace the check entirely by querying
performance_schema.events_errors_summary_global_by_error (or another appropriate
performance_schema table) to detect recent deadlock events; implement one
approach, add a small helper (e.g., parseInnodbDeadlockTimestamp or
queryPerformanceSchemaForDeadlocks) and honor the existing deadlockCheckDisabled
flag and permission handling in hasDeadlocks.

---

Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Line 96: Replace the hardcoded static constant ADVANCED_HEALTH_CHECKS_ENABLED
with a configurable instance property (e.g., advancedHealthChecksEnabled) that
is loaded from configuration at startup (application.properties/@Value,
Environment, or System.getenv/System.getProperty) with a sensible default
(true). Remove or rename the static constant and initialize
advancedHealthChecksEnabled in the HealthService constructor or via DI so it can
be changed at deploy time, then update performAdvancedMySQLChecksWithThrottle()
to reference advancedHealthChecksEnabled instead of
ADVANCED_HEALTH_CHECKS_ENABLED; ensure the new field is final/immutable after
initialization and has a fallback default if the config key is absent.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)

350-351: writeLock().lock() ignores thread interrupts from cancel(true).

Under concurrent health check requests, two MySQL tasks from different callers can race on the write lock. If a cancel(true) fires while Thread-2 is blocked in writeLock().lock(), the interrupt flag is set but the thread stays blocked until Thread-1 releases the lock (potentially after running up to 4 × 2 s = 8 s of advanced queries). Use lockInterruptibly() so the outer deadline actually propagates into this critical section.

♻️ Proposed fix
-        advancedCheckLock.writeLock().lock();
-        try {
+        try {
+            advancedCheckLock.writeLock().lockInterruptibly();
+        } catch (InterruptedException ie) {
+            Thread.currentThread().interrupt();
+            // Return cached result if available, otherwise report not degraded
+            return cachedAdvancedCheckResult != null && cachedAdvancedCheckResult.isDegraded;
+        }
+        try {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines
350 - 351, Replace the blocking advancedCheckLock.writeLock().lock() call with
advancedCheckLock.writeLock().lockInterruptibly() in HealthService so the thread
responds to cancel(true); wrap the lock acquisition in a try that catches
InterruptedException, reasserts the interrupt
(Thread.currentThread().interrupt()) and aborts/returns (or throws a checked
exception) so the outer deadline propagates; ensure the existing finally block
still calls advancedCheckLock.writeLock().unlock() only when the lock was
successfully acquired (track a boolean locked flag) to avoid
IllegalMonitorStateException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 401-419: The hasLockWaits method currently queries
INFORMATION_SCHEMA.PROCESSLIST for lock waits across all DB users; change the
query to restrict to the application's DB user (e.g., add "AND user = ?" or use
"AND user = SUBSTRING_INDEX(USER(), '@', 1)") and bind the app DB username
before executing the PreparedStatement. Retrieve the effective username from the
Connection (via connection.getMetaData().getUserName() or a SELECT USER() query)
and use that value for the prepared statement parameter in hasLockWaits, keeping
the existing exclusions and timeout and preserving the existing error
handling/logging behavior in the catch block.

---

Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 421-448: The hasDeadlocks method currently treats any historical
"LATEST DETECTED DEADLOCK" as a permanent DEGRAGED signal; instead update
hasDeadlocks(Connection) to parse the timestamp of the latest deadlock from the
innodbStatus blob returned by "SHOW ENGINE INNODB STATUS" and only return true
if that timestamp is within a configurable recent window (e.g., 5 minutes); keep
the deadlockCheckDisabled handling and SQL error-code checks, extract the
timestamp by locating the "LATEST DETECTED DEADLOCK" block and parsing the
preceding "LATEST DETECTED DEADLOCK at YYYY-MM-DD HH:MM:SS" (or similar) string
into an Instant, compare to Instant.now() with Duration threshold, and fall back
to returning false if parsing fails (and log a debug message).

---

Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 350-351: Replace the blocking advancedCheckLock.writeLock().lock()
call with advancedCheckLock.writeLock().lockInterruptibly() in HealthService so
the thread responds to cancel(true); wrap the lock acquisition in a try that
catches InterruptedException, reasserts the interrupt
(Thread.currentThread().interrupt()) and aborts/returns (or throws a checked
exception) so the outer deadline propagates; ensure the existing finally block
still calls advancedCheckLock.writeLock().unlock() only when the lock was
successfully acquired (track a boolean locked flag) to avoid
IllegalMonitorStateException.

@DurgaPrasad-54
Copy link
Contributor Author

@coderabbitai resume

@coderabbitai
Copy link

coderabbitai bot commented Feb 22, 2026

✅ Actions performed

Reviews resumed.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (1)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)

450-466: hasSlowQueries is not scoped to the app's own DB user — inconsistent with hasLockWaits.

hasLockWaits was specifically scoped to the application user (per the commit message), but hasSlowQueries still uses a broad exclusion list. In a shared MySQL instance, other applications' long-running queries will cause MMU-API to report DEGRADED. If the intent is to only monitor this application's health, consider applying the same user-scoping here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines
450 - 466, hasSlowQueries currently checks slow queries across all DB users
which can surface other apps' long-running queries; update hasSlowQueries to
scope to the application DB user the same way hasLockWaits does: change the SQL
to filter by the app user (e.g., "AND user = ?") instead of the broad NOT IN
list, bind the app DB username (obtain it the same way hasLockWaits does or
accept the configured DB user) when preparing the statement, and keep existing
timeout/threshold logic and error handling; reference the hasSlowQueries method
and align its user-scoping with hasLockWaits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 160-162: The generic catch block for "catch (Exception e)"
currently only logs the error via logger.warn("Health check execution error:
{}", e.getMessage()) and fails to cancel the pending redisFuture; update that
catch to mirror the TimeoutException/InterruptedException branches by calling
redisFuture.cancel(true) (and optionally mysqlFuture.cancel(true) for symmetry)
and handle/suppress any InterruptedException when waiting/cleaning up so the
redis task is not left running after mysqlFuture.get() throws; modify the catch
surrounding mysqlFuture.get() (the catch for Exception e) to perform these
cancel operations before logging.
- Around line 189-209: checkMySQLHealthSync currently runs
performAdvancedMySQLChecksWithThrottle inline, which can block beyond the outer
~4s future and cause false DOWN reports; refactor so checkMySQLHealthSync only
executes the quick "SELECT 1" and reads a short-lived cached result for advanced
checks, while moving the heavy work in performAdvancedMySQLChecksWithThrottle
into a background ScheduledExecutorService task that runs on a fixed cadence
(e.g., every 30s), performs the multi-query checks with setQueryTimeout, and
updates a thread-safe cache/flag (AtomicBoolean or a small TTL cache) that
checkMySQLHealthSync consults; ensure the new background task handles exceptions
and logs but does not block the health-check hot path, and remove inline
long-running queries from checkMySQLHealthSync.
- Around line 401-419: The SQL in hasLockWaits currently compares
PROCESSLIST.user to USER(), which returns user@host and never matches; update
the query used in hasLockWaits(Connection) to compare PROCESSLIST.user to
SUBSTRING_INDEX(USER(), '@', 1) (i.e., replace "AND user = USER()" with "AND
user = SUBSTRING_INDEX(USER(), '@', 1)") so the username portion is matched
correctly; keep the same PreparedStatement, timeout, and ResultSet handling in
hasLockWaits and preserve the existing exception logging behavior.

---

Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 421-448: hasDeadlocks() currently treats any historical "LATEST
DETECTED DEADLOCK" as a permanent failure; instead parse the "LATEST DETECTED
DEADLOCK" block returned by "SHOW ENGINE INNODB STATUS" in hasDeadlocks(),
extract the deadlock timestamp from that block and compare it to a configurable
recency threshold (e.g., deadlockAgeThresholdMillis) so only recent deadlocks
mark DEGRADED; keep the existing permission handling (deadlockCheckDisabled,
errorCode checks 1142/1227) and, if no timestamp found or parsing fails, fall
back to ignoring the deadlock (return false) rather than treating it as
perpetual.

---

Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 450-466: hasSlowQueries currently checks slow queries across all
DB users which can surface other apps' long-running queries; update
hasSlowQueries to scope to the application DB user the same way hasLockWaits
does: change the SQL to filter by the app user (e.g., "AND user = ?") instead of
the broad NOT IN list, bind the app DB username (obtain it the same way
hasLockWaits does or accept the configured DB user) when preparing the
statement, and keep existing timeout/threshold logic and error handling;
reference the hasSlowQueries method and align its user-scoping with
hasLockWaits.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
src/main/java/com/iemr/mmu/service/health/HealthService.java (2)

488-506: Consider using Java record types for these simple data carriers.

Both HealthCheckResult and AdvancedCheckResult are immutable data holders with final fields — a natural fit for record classes, which would reduce boilerplate.

Example
-    private static class AdvancedCheckResult {
-        final boolean isDegraded;
-
-        AdvancedCheckResult(boolean isDegraded) {
-            this.isDegraded = isDegraded;
-        }
-    }
+    private record AdvancedCheckResult(boolean isDegraded) {}

-    private static class HealthCheckResult {
-        final boolean isHealthy;
-        final String error;
-        final boolean isDegraded;
-
-        HealthCheckResult(boolean isHealthy, String error, boolean isDegraded) {
-            this.isHealthy = isHealthy;
-            this.error = error;
-            this.isDegraded = isDegraded;
-        }
-    }
+    private record HealthCheckResult(boolean isHealthy, String error, boolean isDegraded) {}

Note: Record accessor names use the field name directly (e.g., result.isHealthy() instead of result.isHealthy), so usages would need updating.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines
488 - 506, Replace the two private static classes AdvancedCheckResult and
HealthCheckResult with Java record types to remove boilerplate: declare "private
static record AdvancedCheckResult(boolean isDegraded)" and "private static
record HealthCheckResult(boolean isHealthy, String error, boolean isDegraded)";
then update all usages of the old fields (e.g., result.isHealthy, result.error,
result.isDegraded) to use record accessors (result.isHealthy(), result.error(),
result.isDegraded()) throughout HealthService.

386-389: Catch-all marking isDegraded=true is overly conservative.

If any advanced sub-check throws an unexpected exception (e.g., a ClassCastException from JMX, or a transient SQL error in hasSlowQueries), the whole system is reported as DEGRADED for the next 30 seconds. Since these are supplementary diagnostics, a failure in the diagnostic machinery itself shouldn't degrade the reported health.

Suggested fix — fail open
         } catch (Exception e) {
-            logger.debug("Advanced MySQL checks encountered exception, marking degraded");
-            return new AdvancedCheckResult(true);
+            logger.debug("Advanced MySQL checks encountered exception, skipping", e);
+            return new AdvancedCheckResult(false);
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` around lines
386 - 389, The catch-all currently returns new AdvancedCheckResult(true) which
marks the system DEGRADED on any unexpected exception; change this to fail-open
by returning new AdvancedCheckResult(false) instead, while still logging the
exception (include e in the log call) so diagnostics are preserved—update the
catch block in HealthService (the catch handling Advanced MySQL checks) to log
the exception and return AdvancedCheckResult(false) rather than marking
degraded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 189-204: The health check currently calls
performAdvancedMySQLChecksWithThrottle inline, which can run multiple DB queries
and exceed the future timeout; instead, refactor so the health path in
HealthService reads a cached advanced-check result and does not execute
performAdvancedMySQLChecksWithThrottle synchronously. Implement a background
scheduled task (e.g., ScheduledExecutorService or framework `@Scheduled`) that
periodically invokes performAdvancedMySQLChecksWithThrottle and updates a
thread-safe cache/flag, ensure the main health-check method returns the cached
isDegraded value (with a safe default/fallback if the cache is empty), and
remove any inline invocation of performAdvancedMySQLChecksWithThrottle so the
future deadline cannot be exceeded.
- Around line 156-158: In the generic Exception catch block for the health check
(where you currently log "Health check execution error: {}" using
e.getMessage()), ensure the other pending task is cancelled to avoid orphaned
work: call redisFuture.cancel(true) (and also mysqlFuture.cancel(true) if
appropriate) before logging; update the catch(Exception e) handling around
mysqlFuture.get() to mirror the TimeoutException/InterruptedException branches
by cancelling the counterpart futures (redisFuture and mysqlFuture) and keep the
existing logging behavior.

---

Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 488-506: Replace the two private static classes
AdvancedCheckResult and HealthCheckResult with Java record types to remove
boilerplate: declare "private static record AdvancedCheckResult(boolean
isDegraded)" and "private static record HealthCheckResult(boolean isHealthy,
String error, boolean isDegraded)"; then update all usages of the old fields
(e.g., result.isHealthy, result.error, result.isDegraded) to use record
accessors (result.isHealthy(), result.error(), result.isDegraded()) throughout
HealthService.
- Around line 386-389: The catch-all currently returns new
AdvancedCheckResult(true) which marks the system DEGRADED on any unexpected
exception; change this to fail-open by returning new AdvancedCheckResult(false)
instead, while still logging the exception (include e in the log call) so
diagnostics are preserved—update the catch block in HealthService (the catch
handling Advanced MySQL checks) to log the exception and return
AdvancedCheckResult(false) rather than marking degraded.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/main/java/com/iemr/mmu/service/health/HealthService.java (1)

107-107: newFixedThreadPool(6) maintains 4 permanently idle threads.

checkHealth() submits exactly 2 tasks at a time (MySQL + Redis). A pool of 2 suffices and avoids wasting resources.

♻️ Proposed change
-        this.executorService = Executors.newFixedThreadPool(6);
+        this.executorService = Executors.newFixedThreadPool(2);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/com/iemr/mmu/service/health/HealthService.java` at line 107,
HealthService creates an executorService with Executors.newFixedThreadPool(6)
which keeps unnecessary idle threads; change the pool size to 2 (or make it
configurable) since checkHealth() only submits two tasks (MySQL + Redis). Update
the executorService initialization in the HealthService constructor (or the
field where executorService is created) from newFixedThreadPool(6) to
newFixedThreadPool(2) and, if desired, add a constant or config property for
pool size so future changes are configurable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 398-423: The method handleAdvancedChecksFuture has a missing
closing brace for the final catch (Exception ex) block causing a compile error;
fix it by adding the missing '}' to close that catch block before the method's
final closing brace so the method compiles correctly—locate
handleAdvancedChecksFuture (uses ADVANCED_CHECKS_TIMEOUT_MS,
AdvancedCheckResult, future.get(...)) and ensure the catch (Exception ex) block
is properly terminated (so performAdvancedMySQLChecks() follows as a separate
method).
- Around line 370-395: Multiple threads can pass the write-lock double-check and
race to submit and update the cache, causing lost-writes; introduce an
AtomicBoolean inFlightAdvancedCheck (or similarly named) as a CAS guard so only
one thread performs submit→wait→update while others return the existing
cachedAdvancedCheckResult (or re-check under read lock) instead of performing
the submit; modify the flow around advancedCheckLock,
advancedCheckExecutor.submit(this::performAdvancedMySQLChecks),
handleAdvancedChecksFuture, lastAdvancedCheckTime and cachedAdvancedCheckResult
so threads do: if (!inFlightAdvancedCheck.compareAndSet(false,true)) return
cached value (re-read under read lock), otherwise submit the task, handle the
future, then inside the write lock update lastAdvancedCheckTime and
cachedAdvancedCheckResult and finally inFlightAdvancedCheck.set(false) in a
finally block.

---

Duplicate comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Around line 176-178: The generic Exception catch in HealthService (around the
awaitHealthChecks call) currently logs the error but doesn't cancel running
tasks, leaving redisFuture/orphaned tasks in executorService; modify that catch
block to invoke the same cancellation logic used in the TimeoutException and
InterruptedException branches (call cancelFutures with the relevant futures such
as redisFuture and any other scheduled futures) before logging and
rethrowing/handling, so all branches consistently cancel outstanding futures
created by awaitHealthChecks.

---

Nitpick comments:
In `@src/main/java/com/iemr/mmu/service/health/HealthService.java`:
- Line 107: HealthService creates an executorService with
Executors.newFixedThreadPool(6) which keeps unnecessary idle threads; change the
pool size to 2 (or make it configurable) since checkHealth() only submits two
tasks (MySQL + Redis). Update the executorService initialization in the
HealthService constructor (or the field where executorService is created) from
newFixedThreadPool(6) to newFixedThreadPool(2) and, if desired, add a constant
or config property for pool size so future changes are configurable.

@sonarqubecloud
Copy link

@drtechie drtechie merged commit c2be201 into PSMRI:main Mar 2, 2026
3 of 4 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.

2 participants