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

Merged
fengelniederhammer merged 3 commits into from
Mar 18, 2026
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02e9678
fix(lapis): health check: consider LAPIS "UP" even when SILO is down
fengelniederhammer Mar 12, 2026
164f64f
refactor
fengelniederhammer Mar 17, 2026
b06523d
refactor
fengelniederhammer Mar 17, 2026
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
56 changes: 26 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 @@ -5,41 +5,37 @@ import org.genspectrum.lapis.silo.SiloNotReachableException
import org.genspectrum.lapis.silo.SiloUnavailableException
import org.springframework.boot.actuate.health.Health
import org.springframework.boot.actuate.health.HealthIndicator
import org.springframework.boot.actuate.health.Status
import org.springframework.stereotype.Component

@Component
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
Comment thread
fengelniederhammer marked this conversation as resolved.
.withDetail("siloStatus", Status.UP)
.withDetail("dataVersion", info.dataVersion)
.withDetail("siloVersion", info.siloVersion ?: "unknown")
} catch (_: SiloNotReachableException) {
it
.withDetail("siloStatus", Status.DOWN)
.withDetail("error", "SILO not reachable")
} catch (e: SiloUnavailableException) {
it
.withDetail("siloStatus", Status.DOWN)
.withDetail("error", "SILO unavailable (HTTP 503)")
.withDetail("retryAfter", e.retryAfter)
Comment thread
fengelniederhammer marked this conversation as resolved.
} catch (_: Exception) {
it
.withDetail("siloStatus", Status.DOWN)
.withDetail("error", "Unexpected error checking SILO")
}
}
.build()
Comment on lines +16 to +40
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
Copy Markdown
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.

Copy link
Copy Markdown
Contributor Author

@fengelniederhammer fengelniederhammer Mar 17, 2026

Choose a reason for hiding this comment

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

The Lapis health is indicated automatically by Spring.

If we don't want to have SILO in here at all, then we should maybe completely remove the SiloHealthIndicator? I tried to make it "info only" without affecting the LAPIS health.

}