feat(health,version): add health and version endpoints#120
Merged
drtechie merged 8 commits intoPSMRI:mainfrom Mar 2, 2026
Merged
feat(health,version): add health and version endpoints#120drtechie merged 8 commits intoPSMRI:mainfrom
drtechie merged 8 commits intoPSMRI:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the git-commit-id Maven plugin config; refactors HealthController and VersionController to constructor-based logic and richer error/OpenAPI handling; implements an asynchronous, parallel HealthService with MySQL and Redis checks and diagnostics; and centralizes health/version endpoint constants in the JWT filter. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant HC as HealthController
participant HS as HealthService
participant MySQL as DataSource/MySQL
participant Redis as RedisTemplate
Client->>HC: GET /health
HC->>HS: checkHealth()
par ParallelChecks
HS->>MySQL: run MySQL health task (with timeout)
MySQL-->>HS: connection/result
HS->>Redis: run Redis PING (with timeout)
Redis-->>HS: PONG/result
end
HS->>HS: aggregate component results
HS-->>HC: health map (components, overallStatus, timestamp)
alt overallStatus == DOWN
HC-->>Client: 503 Service Unavailable (status: DOWN, timestamp)
else
HC-->>Client: 200 OK (health map)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pom.xml (1)
294-304:⚠️ Potential issue | 🟡 MinorDuplicate
maven-jar-plugindeclaration.
maven-jar-pluginversion3.0.2is declared twice (lines 294–298 and 300–304). Maven will emit warnings and the second declaration silently overrides the first. Remove one of them.Proposed fix — remove the duplicate block
- <plugin> - <groupId>org.apache.maven.plugins</groupId> - <artifactId>maven-jar-plugin</artifactId> - <version>3.0.2</version> - </plugin> <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-jar-plugin</artifactId> <version>3.0.2</version> </plugin>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 294 - 304, The pom.xml contains a duplicated plugin declaration for artifactId "maven-jar-plugin" with version "3.0.2"; remove one of the two <plugin> blocks so only a single maven-jar-plugin (version 3.0.2) declaration remains to avoid Maven warnings and unintended overrides.
🧹 Nitpick comments (6)
src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java (2)
42-47: Consider using DEBUG instead of INFO for monitoring endpoint skip log.Health endpoints are typically polled frequently (e.g., Kubernetes liveness/readiness probes every few seconds). Logging at INFO on every request will be noisy in production.
Proposed fix
- logger.info("Skipping JWT validation for monitoring endpoint: {}", path); + logger.debug("Skipping JWT validation for monitoring endpoint: {}", path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java` around lines 42 - 47, The log for skipping JWT validation in JwtUserIdValidationFilter currently uses logger.info for monitoring endpoints (the block checking path equals "/health"/"/version" and contextPath variants); change this to logger.debug (or logger.isDebugEnabled() guard plus logger.debug) so frequent health checks don't spam INFO logs, keeping the same message and calling filterChain.doFilter(servletRequest, servletResponse) and return as before.
88-90: Redundant path checks — these are unreachable.Lines 42–47 already match
contextPath + "/health"andcontextPath + "/version"and return early, so control never reaches these lines for those paths. Remove them to avoid confusion.Proposed fix
|| path.startsWith(contextPath + "/user/refreshToken") - || path.startsWith(contextPath + "/public") - || path.equals(contextPath + "/health") - || path.equals(contextPath + "/version")) { + || path.startsWith(contextPath + "/public")) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java` around lines 88 - 90, In JwtUserIdValidationFilter remove the redundant checks for contextPath + "/health" and contextPath + "/version" inside the block that also checks path.startsWith(contextPath + "/public"), because earlier in the same method the filter already returns for those exact paths; locate the method handling the HttpServletRequest (references: variable path, contextPath, and class JwtUserIdValidationFilter) and delete the two equals(...) conditions so only the necessary public-path check remains, keeping logic and short-circuit returns unchanged.src/main/java/com/iemr/admin/controller/version/VersionController.java (2)
67-76: Silent failure whengit.propertiesis missing.When
inputisnull(file not found on classpath), the method returns an emptyPropertiesobject without any log. This makes it hard to diagnose deployment issues where the git-commit-id plugin didn't run. Add a debug/warn log for this case.Proposed fix
private Properties loadGitProperties() throws IOException { Properties properties = new Properties(); try (InputStream input = getClass().getClassLoader() .getResourceAsStream("git.properties")) { if (input != null) { properties.load(input); + } else { + logger.warn("git.properties not found on classpath"); } } return properties; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/controller/version/VersionController.java` around lines 67 - 76, The loadGitProperties method in VersionController silently returns empty Properties when getResourceAsStream("git.properties") yields null; update loadGitProperties to detect input == null and log a warning or debug message (including that git.properties was not found and suggesting the git-commit-id plugin may not have run) via the controller's logger before returning, ensuring the log uses the existing logger instance and preserves the current behavior of returning Properties when present.
47-65: Cache git properties — they are immutable at runtime.
git.propertiesis baked into the JAR at build time and never changes. Loading and parsing it on every/versionrequest is wasteful. Load once at construction time (or lazily) and reuse.Also,
logger.infoon every call will be noisy if this endpoint is polled regularly. ConsiderDEBUG.Proposed refactor — load once in constructor
`@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(); + } `@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)); - response.put("version", gitProperties.getProperty("git.build.version", UNKNOWN_VALUE)); - response.put("branch", gitProperties.getProperty("git.branch", UNKNOWN_VALUE)); - response.put("commitHash", gitProperties.getProperty("git.commit.id.abbrev", UNKNOWN_VALUE)); - } catch (Exception e) { - logger.error("Failed to load version information", e); - response.put("buildTimestamp", UNKNOWN_VALUE); - response.put("version", UNKNOWN_VALUE); - response.put("branch", UNKNOWN_VALUE); - response.put("commitHash", UNKNOWN_VALUE); - } - logger.info("version Controller End"); - return ResponseEntity.ok(response); + logger.debug("Version endpoint called"); + return ResponseEntity.ok(versionInfo); + } + + 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; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/controller/version/VersionController.java` around lines 47 - 65, The versionInformation() endpoint currently calls loadGitProperties() on every request and logs at INFO; instead load and parse gitProperties once (either in the controller constructor or via a private lazy-initialized field) and store the resulting Properties/Map so versionInformation() simply returns the cached response (falling back to UNKNOWN_VALUE if load failed), and change the per-request logger.info calls to logger.debug to avoid noisy logs; update references to loadGitProperties(), versionInformation(), and UNKNOWN_VALUE accordingly and ensure thread-safe initialization if you choose lazy loading.src/main/java/com/iemr/admin/service/health/HealthService.java (2)
116-164: IfmysqlFuture.get()throwsExecutionException, Redis result is silently lost.The generic
catch (Exception e)at line 147 catchesExecutionExceptionfrommysqlFuture.get()but does not attempt to collectredisFuture.get(). WhileperformHealthCheckhas its own try-catch that should preventExecutionException, a defensive.get()call onredisFuturein the catch block (or restructuring with individual try-catches) would be more robust.Proposed fix — collect both futures independently
- long maxTimeout = Math.max(MYSQL_TIMEOUT_SECONDS, REDIS_TIMEOUT_SECONDS) + 1; - long deadlineNs = System.nanoTime() + TimeUnit.SECONDS.toNanos(maxTimeout); - try { - mysqlFuture.get(maxTimeout, TimeUnit.SECONDS); - long remainingNs = deadlineNs - System.nanoTime(); - if (remainingNs > 0) { - redisFuture.get(remainingNs, TimeUnit.NANOSECONDS); - } else { - redisFuture.cancel(true); - } - } catch (TimeoutException e) { - logger.warn("Health check aggregate timeout after {} seconds", maxTimeout); - mysqlFuture.cancel(true); - redisFuture.cancel(true); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - logger.warn("Health check was interrupted"); - mysqlFuture.cancel(true); - redisFuture.cancel(true); - } catch (Exception e) { - logger.warn("Health check execution error: {}", e.getMessage()); - } + long maxTimeout = Math.max(MYSQL_TIMEOUT_SECONDS, REDIS_TIMEOUT_SECONDS) + 1; + long deadlineNs = System.nanoTime() + TimeUnit.SECONDS.toNanos(maxTimeout); + collectFuture(mysqlFuture, maxTimeout, TimeUnit.SECONDS); + long remainingNs = deadlineNs - System.nanoTime(); + if (remainingNs > 0) { + collectFuture(redisFuture, remainingNs, TimeUnit.NANOSECONDS); + } else { + redisFuture.cancel(true); + }With a helper:
private void collectFuture(Future<?> future, long timeout, TimeUnit unit) { try { future.get(timeout, unit); } catch (TimeoutException e) { future.cancel(true); logger.warn("Health check component timed out"); } catch (InterruptedException e) { future.cancel(true); Thread.currentThread().interrupt(); } catch (Exception e) { logger.warn("Health check component error: {}", e.getMessage()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` around lines 116 - 164, The checkHealth method currently may lose Redis results if mysqlFuture.get(...) throws ExecutionException because the generic catch stops collecting redisFuture; fix by collecting each Future independently (e.g., add a helper like collectFuture(Future<?> future, long timeout, TimeUnit unit) or restructure try-catches) and call it for mysqlFuture and redisFuture using the computed per-component timeouts/deadline; ensure on TimeoutException/InterruptedException you cancel the specific future and preserve Thread.currentThread().interrupt() for interrupts, and still call ensurePopulated(...) and computeOverallStatus(...) afterward so both component maps are always populated.
92-97: Fixed thread pool of 2 can cause queuing under concurrent health-check requests.If multiple
/healthrequests arrive simultaneously, each submits 2 tasks to the same 2-thread pool. Tasks from concurrent requests will queue behind each other, inflating response times and potentially triggering the outer timeout.Consider using a cached or virtual-thread executor, or at minimum size the pool larger to handle a reasonable degree of concurrency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` around lines 92 - 97, The fixed two-thread executor created in HealthService's constructor (executorService = Executors.newFixedThreadPool(2)) can cause task queuing under concurrent /health requests; change the executor to a scalable option (e.g., Executors.newCachedThreadPool() or a virtual-thread executor via Executors.newVirtualThreadPerTaskExecutor() if using Java with Project Loom) or make the pool size configurable and larger, and initialize the executorService field accordingly so each incoming health check won't block waiting for threads.
🤖 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/admin/service/health/HealthService.java`:
- Around line 174-193: checkMySQLHealthSync currently runs the basic "SELECT 1"
on a pooled connection and then calls performAdvancedMySQLChecksWithThrottle
using that same Connection, which can hold the JDBC connection for many seconds;
change the flow so the short basic check returns the connection immediately and
advanced checks use a separate Connection or run asynchronously: after
stmt.executeQuery() and before returning the HealthCheckResult for the basic
check, close the basic PreparedStatement/ResultSet and Connection, then if
advanced checks are needed acquire a new Connection (or enqueue a background
task) and call performAdvancedMySQLChecksWithThrottle using that new connection
(or from the background task), ensuring resources are closed and timeouts
applied so the basic health path does not block the pool.
- Around line 393-417: The hasDeadlocks method currently returns true for any
historical deadlock; update hasDeadlocks(Connection) to inspect the "LATEST
DETECTED DEADLOCK" block (innodbStatus) and parse the timestamp within that
section (e.g., extract the datetime following the deadlock header or the "LATEST
DETECTED DEADLOCK" section metadata), then compare it to Instant.now() and only
return true if the parsed timestamp is within a recent threshold (configurable,
e.g., 5 minutes); on parse failure or if no recent timestamp is found, return
false. Preserve existing privilege handling (error codes 1142/1227) and logging
via logger.warn/debug, and ensure any exceptions during parsing fall back to
false and do not disable deadlock checking flag deadlockCheckDisabled.
---
Outside diff comments:
In `@pom.xml`:
- Around line 294-304: The pom.xml contains a duplicated plugin declaration for
artifactId "maven-jar-plugin" with version "3.0.2"; remove one of the two
<plugin> blocks so only a single maven-jar-plugin (version 3.0.2) declaration
remains to avoid Maven warnings and unintended overrides.
---
Nitpick comments:
In `@src/main/java/com/iemr/admin/controller/version/VersionController.java`:
- Around line 67-76: The loadGitProperties method in VersionController silently
returns empty Properties when getResourceAsStream("git.properties") yields null;
update loadGitProperties to detect input == null and log a warning or debug
message (including that git.properties was not found and suggesting the
git-commit-id plugin may not have run) via the controller's logger before
returning, ensuring the log uses the existing logger instance and preserves the
current behavior of returning Properties when present.
- Around line 47-65: The versionInformation() endpoint currently calls
loadGitProperties() on every request and logs at INFO; instead load and parse
gitProperties once (either in the controller constructor or via a private
lazy-initialized field) and store the resulting Properties/Map so
versionInformation() simply returns the cached response (falling back to
UNKNOWN_VALUE if load failed), and change the per-request logger.info calls to
logger.debug to avoid noisy logs; update references to loadGitProperties(),
versionInformation(), and UNKNOWN_VALUE accordingly and ensure thread-safe
initialization if you choose lazy loading.
In `@src/main/java/com/iemr/admin/service/health/HealthService.java`:
- Around line 116-164: The checkHealth method currently may lose Redis results
if mysqlFuture.get(...) throws ExecutionException because the generic catch
stops collecting redisFuture; fix by collecting each Future independently (e.g.,
add a helper like collectFuture(Future<?> future, long timeout, TimeUnit unit)
or restructure try-catches) and call it for mysqlFuture and redisFuture using
the computed per-component timeouts/deadline; ensure on
TimeoutException/InterruptedException you cancel the specific future and
preserve Thread.currentThread().interrupt() for interrupts, and still call
ensurePopulated(...) and computeOverallStatus(...) afterward so both component
maps are always populated.
- Around line 92-97: The fixed two-thread executor created in HealthService's
constructor (executorService = Executors.newFixedThreadPool(2)) can cause task
queuing under concurrent /health requests; change the executor to a scalable
option (e.g., Executors.newCachedThreadPool() or a virtual-thread executor via
Executors.newVirtualThreadPerTaskExecutor() if using Java with Project Loom) or
make the pool size configurable and larger, and initialize the executorService
field accordingly so each incoming health check won't block waiting for threads.
In `@src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java`:
- Around line 42-47: The log for skipping JWT validation in
JwtUserIdValidationFilter currently uses logger.info for monitoring endpoints
(the block checking path equals "/health"/"/version" and contextPath variants);
change this to logger.debug (or logger.isDebugEnabled() guard plus logger.debug)
so frequent health checks don't spam INFO logs, keeping the same message and
calling filterChain.doFilter(servletRequest, servletResponse) and return as
before.
- Around line 88-90: In JwtUserIdValidationFilter remove the redundant checks
for contextPath + "/health" and contextPath + "/version" inside the block that
also checks path.startsWith(contextPath + "/public"), because earlier in the
same method the filter already returns for those exact paths; locate the method
handling the HttpServletRequest (references: variable path, contextPath, and
class JwtUserIdValidationFilter) and delete the two equals(...) conditions so
only the necessary public-path check remains, keeping logic and short-circuit
returns unchanged.
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pom.xml (1)
316-319: Escape literal dots in the Java regex filter patterns.The plugin treats
includeOnlyPropertiesvalues as Java regexes, where.*is globbing and not plain*. Unescaped.matches any character, so^git.branch$would technically match strings likegitXbranch. While practically harmless given the actual Git property names, the patterns should use\.for literal dot matching:♻️ Proposed fix — escape dots in regex patterns
- <property>^git.branch$</property> - <property>^git.commit.id.abbrev$</property> - <property>^git.build.version$</property> - <property>^git.build.time$</property> + <property>^git\.branch$</property> + <property>^git\.commit\.id\.abbrev$</property> + <property>^git\.build\.version$</property> + <property>^git\.build\.time$</property>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 316 - 319, The regex patterns in the includeOnlyProperties entries (the values '^git.branch$', '^git.commit.id.abbrev$', '^git.build.version$', '^git.build.time$') use unescaped dots, which match any character; update each to escape the literal dots so they read '^git\.branch$', '^git\.commit\.id\.abbrev$', '^git\.build\.version$', '^git\.build\.time$' to ensure they only match the exact property names (update the property values in the pom.xml where these strings appear).src/main/java/com/iemr/admin/service/health/HealthService.java (1)
75-75:DIAGNOSTIC_DEADLOCKconstant is dead code.
hasDeadlocks()was correctly removed to fix the permanentDEGRADEDstate issue (addressed in a prior review), but theDIAGNOSTIC_DEADLOCKconstant was not cleaned up.- private static final String DIAGNOSTIC_DEADLOCK = "MYSQL_DEADLOCK";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` at line 75, The DIAGNOSTIC_DEADLOCK constant in HealthService (DIAGNOSTIC_DEADLOCK) is now unused after hasDeadlocks() was removed; delete the unused constant declaration from HealthService.java and remove any lingering references or imports related to it, then run a build/compile to ensure no remaining usages; if you want to keep a diagnostic enum or constants class, move any still-needed diagnostics there instead of leaving this dead symbol.
🤖 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/admin/service/health/HealthService.java`:
- Around line 118-169: The current TOCTOU arises because performHealthCheck
submits worker tasks that mutate shared ConcurrentHashMap instances
(mysqlStatus/redisStatus) while the main thread may cancel and then call
ensurePopulated, causing interleaved writes; fix by changing each worker to
return a Map<String,Object> (e.g., use CompletableFuture<Map<String,Object>> or
submit Callables that return immutable result maps from
checkMySQLHealthSync/checkRedisHealthSync) instead of mutating shared maps,
apply timeouts on those futures (cancel if needed), then after futures
complete/timeout compose final response by copying each result into new maps and
calling computeOverallStatus; remove or simplify ensurePopulated to operate on
the per-worker result/futures so no shared mutable maps are read/written
concurrently.
- Around line 372-390: The WHERE clause in hasLockWaits uses "AND user = USER()"
which never matches because PROCESSLIST.USER is just the username while USER()
returns "user@host", so hasLockWaits always returns false; update the SQL in
hasLockWaits (the PreparedStatement that queries INFORMATION_SCHEMA.PROCESSLIST)
to either remove the "AND user = USER()" filter entirely or change it to compare
the username portion only (e.g., use SUBSTRING_INDEX(USER(), '@', 1) or an
equivalent function) so the comparison matches PROCESSLIST.USER; adjust the
PreparedStatement text accordingly and keep the existing query timeout/result
handling as-is.
---
Duplicate comments:
In `@src/main/java/com/iemr/admin/service/health/HealthService.java`:
- Around line 172-191: The current checkMySQLHealthSync method holds the initial
JDBC Connection/PreparedStatement/ResultSet open while calling
performAdvancedMySQLChecksWithThrottle(), which causes two pooled connections to
be held concurrently; refactor checkMySQLHealthSync so the basic "SELECT 1 as
health_check" query is executed in a minimal try-with-resources block that fully
closes connection, stmt, and rs before invoking
performAdvancedMySQLChecksWithThrottle(), then call
performAdvancedMySQLChecksWithThrottle() after the resources are closed and
return the HealthCheckResult based on that result; keep the same method names
(checkMySQLHealthSync and performAdvancedMySQLChecksWithThrottle) and ensure any
return paths still create the correct HealthCheckResult without holding the
initial connection open.
---
Nitpick comments:
In `@pom.xml`:
- Around line 316-319: The regex patterns in the includeOnlyProperties entries
(the values '^git.branch$', '^git.commit.id.abbrev$', '^git.build.version$',
'^git.build.time$') use unescaped dots, which match any character; update each
to escape the literal dots so they read '^git\.branch$',
'^git\.commit\.id\.abbrev$', '^git\.build\.version$', '^git\.build\.time$' to
ensure they only match the exact property names (update the property values in
the pom.xml where these strings appear).
In `@src/main/java/com/iemr/admin/service/health/HealthService.java`:
- Line 75: The DIAGNOSTIC_DEADLOCK constant in HealthService
(DIAGNOSTIC_DEADLOCK) is now unused after hasDeadlocks() was removed; delete the
unused constant declaration from HealthService.java and remove any lingering
references or imports related to it, then run a build/compile to ensure no
remaining usages; if you want to keep a diagnostic enum or constants class, move
any still-needed diagnostics there instead of leaving this dead symbol.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/com/iemr/admin/service/health/HealthService.java (4)
92-97: Thread pool of 6 is oversized for 2 concurrent tasks.Only two tasks (MySQL + Redis) are ever submitted. A fixed pool of 6 holds 4 idle threads permanently. Consider
Executors.newFixedThreadPool(2)or a cached thread pool.♻️ Suggested fix
- 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/admin/service/health/HealthService.java` around lines 92 - 97, The HealthService constructor creates executorService with Executors.newFixedThreadPool(6) which is oversized for only two concurrent tasks; change the pool to Executors.newFixedThreadPool(2) (or switch to a cached/thread-per-task executor if you prefer dynamic sizing) by updating the executorService initialization in the HealthService(DataSource, RedisTemplate) constructor so it only allocates two threads instead of six.
162-174:awaitHealthCheckswaits sequentially — MySQL timeout starves Redis.If the MySQL future blocks for the full
maxTimeoutand then succeeds right at the deadline,remainingNsis ≤ 0 and the Redis future is unconditionally cancelled (line 172), even if it already completed successfully. The Redis result is discarded despite being available.A more robust approach is to use
CompletableFuture.allOf(or invokeAll) so both checks run and are awaited in parallel within a single deadline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` around lines 162 - 174, The awaitHealthChecks method currently waits sequentially on mysqlFuture then redisFuture so a long MySQL wait can consume the whole deadline and cause redisFuture to be cancelled or its result dropped; change awaitHealthChecks (method name) to wait for both checks in parallel by using a parallel waiting primitive (e.g., CompletableFuture.allOf for CompletableFutures or ExecutorService.invokeAll for Futures) with a single deadline derived from getMaxTimeout(), then propagate any TimeoutException/InterruptedException/ExecutionException after both are awaited; ensure you reference and await both mysqlFuture and redisFuture rather than cancelling redisFuture unconditionally when remainingNs <= 0 and preserve/inspect each future's result or exception.
499-517: Consider using Javarecordtypes for these simple data carriers.
AdvancedCheckResultandHealthCheckResultare immutable data holders. If the project targets Java 16+, records would reduce boilerplate and convey intent.♻️ Example
private record AdvancedCheckResult(boolean isDegraded) {} private record HealthCheckResult(boolean isHealthy, String error, boolean isDegraded) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` around lines 499 - 517, Replace the two immutable holder classes AdvancedCheckResult and HealthCheckResult with Java record types to reduce boilerplate and make intent explicit; specifically change AdvancedCheckResult to a record with a boolean component (isDegraded) and change HealthCheckResult to a record with components (isHealthy, error, isDegraded), ensuring the project is compiled with Java 16+ and updating any usages of the constructors to the record canonical constructors or compact form.
214-232: Redis health check whenredisTemplateis null reports UP — could be misleading.Line 216 returns
HealthCheckResult(true, ...)when Redis isn't configured, so the component appears asUPin the response. If Redis is a required dependency in some deployments, this silently masks a misconfiguration. Consider returning a distinct status (e.g.,UNKNOWNorSKIPPED) or excluding the component from the response entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` around lines 214 - 232, checkRedisHealthSync currently returns HealthCheckResult(true, ...) when redisTemplate is null, which reports Redis as UP; change this to not report UP: update checkRedisHealthSync to detect redisTemplate == null and return a "skipped" or "unknown" result instead (use the HealthCheckResult variant or flag that denotes skipped/unknown, or adjust the service to exclude the Redis component) so Redis is not shown as UP when not configured; refer to the checkRedisHealthSync method, the redisTemplate variable and the HealthCheckResult constructor/flags to implement the proper skipped/unknown semantics.
🤖 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/admin/service/health/HealthService.java`:
- Around line 140-159: The generic catch (Exception e) in performHealthChecks
currently logs errors from awaitHealthChecks (or other runtime exceptions) but
does not call cancelFutures, leaving mysqlFuture/redisFuture potentially running
and mutating shared maps; update the generic handler in performHealthChecks to
call cancelFutures(mysqlFuture, redisFuture) (same as the TimeoutException and
InterruptedException branches) after logging, ensuring any in-flight futures
submitted via executorService (mysqlFuture and redisFuture) are cancelled when
awaitHealthChecks or other unexpected exceptions occur.
---
Duplicate comments:
In `@src/main/java/com/iemr/admin/service/health/HealthService.java`:
- Around line 120-191: The worker threads currently mutate shared
ConcurrentHashMap instances (mysqlStatus/redisStatus) causing a TOCTOU race with
ensurePopulated; change performHealthChecks to submit
Callable<Map<String,Object>> tasks (use
performHealthCheck/checkMySQLHealthSync/checkRedisHealthSync to return an
immutable/result map instead of writing to shared maps), collect
Future<Map<String,Object>> for MySQL and Redis, wait via awaitHealthChecks (or a
revised await that returns each result or times out), and only after futures
resolve or time out compose the final components map—on timeout use
ensurePopulated-style default maps for that component and do not allow worker
threads to write into the shared response maps after cancellation.
- Around line 193-212: checkMySQLHealthSync() currently holds the basic JDBC
connection open while calling performAdvancedMySQLChecksWithThrottle(), causing
two pool connections to be used; fix by moving the
performAdvancedMySQLChecksWithThrottle() call outside the try-with-resources
that opens the PreparedStatement/Connection: execute the "SELECT 1" query inside
the try-with-resources, capture whether it succeeded into a local variable,
allow the try-with-resources to close the basic connection, then call
performAdvancedMySQLChecksWithThrottle() and combine its degraded flag with the
initial success to return the final HealthCheckResult; update the branches that
now require the advanced check to run after the resources are closed and keep
existing logging/exception handling in checkMySQLHealthSync().
---
Nitpick comments:
In `@src/main/java/com/iemr/admin/service/health/HealthService.java`:
- Around line 92-97: The HealthService constructor creates executorService with
Executors.newFixedThreadPool(6) which is oversized for only two concurrent
tasks; change the pool to Executors.newFixedThreadPool(2) (or switch to a
cached/thread-per-task executor if you prefer dynamic sizing) by updating the
executorService initialization in the HealthService(DataSource, RedisTemplate)
constructor so it only allocates two threads instead of six.
- Around line 162-174: The awaitHealthChecks method currently waits sequentially
on mysqlFuture then redisFuture so a long MySQL wait can consume the whole
deadline and cause redisFuture to be cancelled or its result dropped; change
awaitHealthChecks (method name) to wait for both checks in parallel by using a
parallel waiting primitive (e.g., CompletableFuture.allOf for CompletableFutures
or ExecutorService.invokeAll for Futures) with a single deadline derived from
getMaxTimeout(), then propagate any
TimeoutException/InterruptedException/ExecutionException after both are awaited;
ensure you reference and await both mysqlFuture and redisFuture rather than
cancelling redisFuture unconditionally when remainingNs <= 0 and
preserve/inspect each future's result or exception.
- Around line 499-517: Replace the two immutable holder classes
AdvancedCheckResult and HealthCheckResult with Java record types to reduce
boilerplate and make intent explicit; specifically change AdvancedCheckResult to
a record with a boolean component (isDegraded) and change HealthCheckResult to a
record with components (isHealthy, error, isDegraded), ensuring the project is
compiled with Java 16+ and updating any usages of the constructors to the record
canonical constructors or compact form.
- Around line 214-232: checkRedisHealthSync currently returns
HealthCheckResult(true, ...) when redisTemplate is null, which reports Redis as
UP; change this to not report UP: update checkRedisHealthSync to detect
redisTemplate == null and return a "skipped" or "unknown" result instead (use
the HealthCheckResult variant or flag that denotes skipped/unknown, or adjust
the service to exclude the Redis component) so Redis is not shown as UP when not
configured; refer to the checkRedisHealthSync method, the redisTemplate variable
and the HealthCheckResult constructor/flags to implement the proper
skipped/unknown semantics.
DurgaPrasad-54
changed the title
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/com/iemr/admin/service/health/HealthService.java (2)
194-213:⚠️ Potential issue | 🟠 Major
⚠️ Unresolved: Basic-check JDBC connection still held open duringperformAdvancedMySQLChecksWithThrottle().The outer try-with-resources keeps
connectionopen untilperformAdvancedMySQLChecksWithThrottle()returns (line 202). When the throttle cache is stale that method also opens a second connection (line 357), so the pool briefly holds two connections per health-check cycle. This worsens the very pool-exhaustion condition the check is trying to detect, and the pool metrics reported byhasConnectionPoolExhaustion()include both connections — inflating the "active connections" count.The fix suggested in the earlier review — closing the basic
PreparedStatement/Connectionbefore triggering advanced checks — is still not applied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` around lines 194 - 213, The basic health-check keeps the Connection/PreparedStatement open across the call to performAdvancedMySQLChecksWithThrottle(), which can open a second Connection and temporarily double-count active connections; modify checkMySQLHealthSync() so the simple "SELECT 1" result is read and the try-with-resources block (connection, stmt, rs) is closed before invoking performAdvancedMySQLChecksWithThrottle(); i.e., determine the boolean "basicOk" from the ResultSet, exit the resource block, then call performAdvancedMySQLChecksWithThrottle() and return the HealthCheckResult, ensuring no two concurrent connections from the same check are held.
120-128:⚠️ Potential issue | 🟠 Major
⚠️ Unresolved: TOCTOU race between cancelled futures andensurePopulated.
ensurePopulatedperforms a non-atomiccontainsKey→ 3×puton a sharedConcurrentHashMap. JDBC tasks do not respond to Java thread interrupts, so a cancelled worker thread can continue running and writeSTATUS_KEYbetween thecontainsKeycheck and the subsequentputcalls on the main thread. This can leave the map with mismatched fields (e.g., a worker'sRESPONSE_TIME_KEYalongside a main-threadERROR_KEY) and the wrong overall status reported to callers.The recommended fix from the earlier review — having each worker return an immutable
Map<String,Object>viaCompletableFuture<Map<String,Object>>and composing the final response only after both futures resolve or time out — remains unimplemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` around lines 120 - 128, The current TOCTOU arises because performHealthChecks mutates shared ConcurrentHashMaps and ensurePopulated does a non-atomic containsKey → put sequence, allowing cancelled JDBC worker threads to write partial fields between checks; fix by changing worker tasks to return an immutable Map<String,Object> (e.g., CompletableFuture<Map<String,Object>>) containing all keys (STATUS_KEY, RESPONSE_TIME_KEY, ERROR_KEY) rather than mutating shared maps, then in performHealthChecks compose/wait for both futures (with timeout) and merge their fully-formed maps into mysqlStatus and redisStatus only after completion (or populate default values if a future times out), removing ensurePopulated’s non-atomic mutations and preventing interleaving from interrupted threads.
🧹 Nitpick comments (2)
src/main/java/com/iemr/admin/service/health/HealthService.java (2)
96-96: Thread pool of 6 for only 2 concurrent tasks; consider naming threads.
Executors.newFixedThreadPool(6)keeps 6 threads alive permanently, but at most 2 tasks are submitted percheckHealth()invocation. A pool of 2 (or a small fixed number with a namedThreadFactory) would save resources and make thread dumps easier to read.♻️ Proposed refactor
- this.executorService = Executors.newFixedThreadPool(6); + this.executorService = Executors.newFixedThreadPool(2, + new java.util.concurrent.ThreadFactory() { + private final java.util.concurrent.atomic.AtomicInteger count = + new java.util.concurrent.atomic.AtomicInteger(0); + `@Override` public Thread newThread(Runnable r) { + Thread t = new Thread(r, "health-check-" + count.incrementAndGet()); + t.setDaemon(true); + return t; + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` at line 96, The executorService is created with Executors.newFixedThreadPool(6) while checkHealth() only submits up to 2 concurrent tasks; change the pool size to 2 (or a small bounded size) and supply a custom ThreadFactory that sets a clear name pattern (e.g., "health-check-%d") so threads are easier to identify; update the initialization of executorService (replace Executors.newFixedThreadPool(6)) and ensure any shutdown logic still applies to the renamed executorService.
327-330: Dead code:ADVANCED_HEALTH_CHECKS_ENABLEDis alwaystrue.
private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = true;is a compile-time constant; the early-return guard at lines 328–330 is unreachable. Either remove the constant and the guard, or make it runtime-configurable.♻️ Proposed refactor — make it configurable
- private static final boolean ADVANCED_HEALTH_CHECKS_ENABLED = true; + `@org.springframework.beans.factory.annotation.Value`("${health.advanced.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/admin/service/health/HealthService.java` around lines 327 - 330, The guard in performAdvancedMySQLChecksWithThrottle() is dead because ADVANCED_HEALTH_CHECKS_ENABLED is a compile-time true constant; update the implementation so the flag is runtime-configurable or remove the guard. Specifically, change the declaration of ADVANCED_HEALTH_CHECKS_ENABLED from a static final constant to a runtime-backed flag (e.g., read from configuration, System.getenv, or inject via configuration/constructor), update any callers if needed, and keep the early-return behavior based on that runtime value; alternatively, if you want the checks always enabled, remove ADVANCED_HEALTH_CHECKS_ENABLED and the if-return in performAdvancedMySQLChecksWithThrottle() to eliminate unreachable code. Ensure you update tests/configuration that assume the previous constant.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/com/iemr/admin/service/health/HealthService.java
🤖 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/admin/service/health/HealthService.java`:
- Around line 194-213: The basic health-check keeps the
Connection/PreparedStatement open across the call to
performAdvancedMySQLChecksWithThrottle(), which can open a second Connection and
temporarily double-count active connections; modify checkMySQLHealthSync() so
the simple "SELECT 1" result is read and the try-with-resources block
(connection, stmt, rs) is closed before invoking
performAdvancedMySQLChecksWithThrottle(); i.e., determine the boolean "basicOk"
from the ResultSet, exit the resource block, then call
performAdvancedMySQLChecksWithThrottle() and return the HealthCheckResult,
ensuring no two concurrent connections from the same check are held.
- Around line 120-128: The current TOCTOU arises because performHealthChecks
mutates shared ConcurrentHashMaps and ensurePopulated does a non-atomic
containsKey → put sequence, allowing cancelled JDBC worker threads to write
partial fields between checks; fix by changing worker tasks to return an
immutable Map<String,Object> (e.g., CompletableFuture<Map<String,Object>>)
containing all keys (STATUS_KEY, RESPONSE_TIME_KEY, ERROR_KEY) rather than
mutating shared maps, then in performHealthChecks compose/wait for both futures
(with timeout) and merge their fully-formed maps into mysqlStatus and
redisStatus only after completion (or populate default values if a future times
out), removing ensurePopulated’s non-atomic mutations and preventing
interleaving from interrupted threads.
---
Nitpick comments:
In `@src/main/java/com/iemr/admin/service/health/HealthService.java`:
- Line 96: The executorService is created with Executors.newFixedThreadPool(6)
while checkHealth() only submits up to 2 concurrent tasks; change the pool size
to 2 (or a small bounded size) and supply a custom ThreadFactory that sets a
clear name pattern (e.g., "health-check-%d") so threads are easier to identify;
update the initialization of executorService (replace
Executors.newFixedThreadPool(6)) and ensure any shutdown logic still applies to
the renamed executorService.
- Around line 327-330: The guard in performAdvancedMySQLChecksWithThrottle() is
dead because ADVANCED_HEALTH_CHECKS_ENABLED is a compile-time true constant;
update the implementation so the flag is runtime-configurable or remove the
guard. Specifically, change the declaration of ADVANCED_HEALTH_CHECKS_ENABLED
from a static final constant to a runtime-backed flag (e.g., read from
configuration, System.getenv, or inject via configuration/constructor), update
any callers if needed, and keep the early-return behavior based on that runtime
value; alternatively, if you want the checks always enabled, remove
ADVANCED_HEALTH_CHECKS_ENABLED and the if-return in
performAdvancedMySQLChecksWithThrottle() to eliminate unreachable code. Ensure
you update tests/configuration that assume the previous constant.
…ove shared-map race
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/com/iemr/admin/service/health/HealthService.java (1)
129-205:⚠️ Potential issue | 🟠 MajorShared status maps can still be mutated after timeouts/cancellation.
Cancelled tasks may continue and overwriteensurePopulated, and the response holds live map references, so results can change aftercheckHealth()returns. Prefer returning immutable per-task result maps (e.g., futures returning a map) and assemble the response from those, rather than sharing mutable maps between threads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/iemr/admin/service/health/HealthService.java` around lines 129 - 205, The current implementation shares mutable maps (mysqlStatus, redisStatus) between threads causing cancelled/slow tasks to mutate results after checkHealth() returns; change to have performHealthChecks submit Callables that return Map<String,Object> (e.g., Future<Map<String,Object>>) instead of mutating shared maps, update performHealthChecks and awaitHealthChecks to collect those Futures, call get() with timeouts, apply ensurePopulated to each returned map if null/empty, cancel timed-out futures via cancelFutures, and assemble immutable copies (e.g., unmodifiableMap/new LinkedHashMap copy) into the response in checkHealth() so the returned components cannot be mutated later.
🤖 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/admin/service/health/HealthService.java`:
- Around line 129-205: The current implementation shares mutable maps
(mysqlStatus, redisStatus) between threads causing cancelled/slow tasks to
mutate results after checkHealth() returns; change to have performHealthChecks
submit Callables that return Map<String,Object> (e.g.,
Future<Map<String,Object>>) instead of mutating shared maps, update
performHealthChecks and awaitHealthChecks to collect those Futures, call get()
with timeouts, apply ensurePopulated to each returned map if null/empty, cancel
timed-out futures via cancelFutures, and assemble immutable copies (e.g.,
unmodifiableMap/new LinkedHashMap copy) into the response in checkHealth() so
the returned components cannot be mutated later.
drtechie
approved these changes
Mar 2, 2026
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
2 participants
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.
📋 Description
JIRA ID:
This PR implements /health and /version endpoints in the Admin-API as part of Issue PSMRI/AMRIT#102
What’s changed
/healthendpoint/versionendpointSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores