Skip to content

fix(lapis): health check: consider LAPIS "UP" even when SILO is down #1594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fengelniederhammer wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(lapis): health check: consider LAPIS "UP" even when SILO is down #1594

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 27 additions & 30 deletions lapis/src/main/kotlin/org/genspectrum/lapis/health/SiloHealthIndicator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,34 +12,31 @@ class SiloHealthIndicator(
private val cachedSiloClient: CachedSiloClient,
) : HealthIndicator {
override fun health(): Health =
try {
val info = cachedSiloClient.callInfo()
Health
.up()
.withDetail("dataVersion", info.dataVersion)
.withDetail("siloVersion", info.siloVersion ?: "unknown")
.build()
} catch (e: SiloNotReachableException) {
Health
.down()
.withDetail("error", "SILO not reachable")
.withDetail("message", e.message)
.withException(e)
.build()
} catch (e: SiloUnavailableException) {
Health
.down()
.withDetail("error", "SILO unavailable (HTTP 503)")
.withDetail("message", e.message)
.withDetail("retryAfter", e.retryAfter)
.withException(e)
.build()
} catch (e: Exception) {
Health
.down()
.withDetail("error", "Unexpected error checking SILO")
.withDetail("message", e.message)
.withException(e)
.build()
}
Health
.up() // LAPIS should always be "up", independent of SILO.
.let {
try {
val info = cachedSiloClient.callInfo()
it
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

When SILO is reachable, the response doesn’t include a siloStatus detail, but it is present (as "DOWN") in all failure cases. This makes the health JSON schema inconsistent for consumers; consider always setting siloStatus (e.g., "UP" on success, "DOWN" on failure) so clients don’t have to infer from missing fields.

Suggested change
it
it
.withDetail("siloStatus", "UP")

Copilot uses AI. Check for mistakes.
.withDetail("dataVersion", info.dataVersion)
.withDetail("siloVersion", info.siloVersion ?: "unknown")
} catch (e: SiloNotReachableException) {
it
.withDetail("siloStatus", "DOWN")
.withDetail("error", "SILO not reachable")
.withDetail("message", e.message)
} catch (e: SiloUnavailableException) {
it
.withDetail("siloStatus", "DOWN")
.withDetail("error", "SILO unavailable (HTTP 503)")
.withDetail("message", e.message)
.withDetail("retryAfter", e.retryAfter)
Comment on lines +24 to +33
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The health endpoint is configured to show-details=always and /actuator/** is publicly accessible, so including raw exception messages (e.message) here can leak internal connectivity details (URIs, exception classes, etc.). Consider removing message from the public health response or replacing it with a sanitized/static value and logging the full exception server-side instead.

Copilot uses AI. Check for mistakes.
} catch (e: Exception) {
it
.withDetail("siloStatus", "DOWN")
.withDetail("error", "Unexpected error checking SILO")
Comment on lines +25 to +37
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

"DOWN" is hard-coded multiple times for siloStatus. To avoid typos and keep alignment with Spring Actuator status codes, consider using the Actuator Status constants (e.g., Status.DOWN.code / Status.UP.code) or a single local constant.

Copilot uses AI. Check for mistakes.
.withDetail("message", e.message)
}
}
.build()
Comment on lines +15 to +41
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

SiloHealthIndicator now always reports Spring Actuator status UP (even when SILO is down) and only encodes SILO failure via details. This is easy to misinterpret because /actuator/health components will still show this indicator as UP; if the intent is to keep overall LAPIS health UP, consider moving SILO checks into a separate health group/contributor (e.g., readiness) or otherwise returning a real DOWN status for SILO while excluding it from the liveness group.

Suggested change
Health
.up() // LAPIS should always be "up", independent of SILO.
.let {
try {
val info = cachedSiloClient.callInfo()
it
.withDetail("dataVersion", info.dataVersion)
.withDetail("siloVersion", info.siloVersion ?: "unknown")
} catch (e: SiloNotReachableException) {
it
.withDetail("siloStatus", "DOWN")
.withDetail("error", "SILO not reachable")
.withDetail("message", e.message)
} catch (e: SiloUnavailableException) {
it
.withDetail("siloStatus", "DOWN")
.withDetail("error", "SILO unavailable (HTTP 503)")
.withDetail("message", e.message)
.withDetail("retryAfter", e.retryAfter)
} catch (e: Exception) {
it
.withDetail("siloStatus", "DOWN")
.withDetail("error", "Unexpected error checking SILO")
.withDetail("message", e.message)
}
}
.build()
try {
val info = cachedSiloClient.callInfo()
Health.up()
.withDetail("dataVersion", info.dataVersion)
.withDetail("siloVersion", info.siloVersion ?: "unknown")
.build()
} catch (e: SiloNotReachableException) {
Health.down()
.withDetail("siloStatus", "DOWN")
.withDetail("error", "SILO not reachable")
.withDetail("message", e.message)
.build()
} catch (e: SiloUnavailableException) {
Health.down()
.withDetail("siloStatus", "DOWN")
.withDetail("error", "SILO unavailable (HTTP 503)")
.withDetail("message", e.message)
.withDetail("retryAfter", e.retryAfter)
.build()
} catch (e: Exception) {
Health.down()
.withDetail("siloStatus", "DOWN")
.withDetail("error", "Unexpected error checking SILO")
.withDetail("message", e.message)
.build()
}

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@fhennig fhennig Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This sounds valid; we're in the 'SiloHealthIndicator', shouldn't there be a LapisHealthIndicator somewhere maybe?

Edit: This was in response to what ChatGPT said above.

}
Loading