fix(lapis): set a low timeout on calling SILO info so that the health endpoint doesn't block if SILO doesn't respond - #1593
Merged
Conversation
… endpoint doesn't block if SILO doesn't respond
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
fengelniederhammer
marked this pull request as ready for review
March 12, 2026 10:47
fengelniederhammer
pushed a commit
that referenced
this pull request
Mar 17, 2026
🤖 I have created a release *beep* *boop* --- ## [0.7.2](v0.7.1...v0.7.2) (2026-03-16) ### Bug Fixes * **lapis:** set a low timeout on calling SILO info so that the health endpoint doesn't block if SILO doesn't respond ([#1593](#1593)) ([c501e9d](c501e9d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
corneliusroemer-agent
added a commit
to corneliusroemer-agent/LAPIS
that referenced
this pull request
Sep 10, 2026
callInfo() has had a hard-coded 100 ms request timeout since GenSpectrum#1593, picked so that the health endpoint cannot block when SILO does not respond. That same budget governs /sample/info, which deployments use as a readiness probe, so a SILO that is merely slow rather than down makes LAPIS report itself unhealthy and get taken out of service. Expose it as silo.infoTimeout, a Duration defaulting to 100ms, next to the existing silo.url. Nothing changes unless the property is set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PRvw1U8cSHWotT9kL5r8FC
corneliusroemer-agent
added a commit
to corneliusroemer-agent/LAPIS
that referenced
this pull request
Sep 10, 2026
callInfo() has had a hard-coded 100 ms request timeout since GenSpectrum#1593, picked so that the health endpoint cannot block when SILO does not respond. That same budget governs /sample/info, which deployments use as a readiness probe, so a SILO that is merely slow rather than down makes LAPIS report itself unhealthy and get taken out of service. Expose it as silo.infoTimeout, a Duration defaulting to 100ms, next to the existing silo.url. Nothing changes unless the property is set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PRvw1U8cSHWotT9kL5r8FC
corneliusroemer-agent
added a commit
to corneliusroemer-agent/LAPIS
that referenced
this pull request
Sep 10, 2026
callInfo() has had a hard-coded 100 ms request timeout since GenSpectrum#1593, picked so that the health endpoint cannot block when SILO does not respond. That same budget governs /sample/info, which deployments use as a readiness probe, so a SILO that is merely slow rather than down makes LAPIS report itself unhealthy and get taken out of service. Expose it as silo.infoTimeout, a Duration defaulting to 100ms, next to the existing silo.url. Nothing changes unless the property is set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PRvw1U8cSHWotT9kL5r8FC
corneliusroemer
left a comment
corneliusroemer
left a comment
Contributor
There was a problem hiding this comment.
This PR caused new flakes on Loculus CI because sometimes the info call does take longer than 100ms due to high load.
I understand that the LAPIS health endpoint should not stall when SILO blocks - but shouldn't this be handled in the health endpoint's call to this function here rather than the function itself?
A side-effect, and I don't think an intended one, is that now all callers to /sample/info get a 503 when this call takes >100ms. But there's nothing intrinsically wrong about taking >100ms - it can happen under load. Callers can set their own timeouts however they like, some might be happy to wait for a second but now they get a 503 instead.
Maybe moving where the timeout lives (to the caller that wants it) is better than #1866 (which is only a narrow workaround for the underlying problem - if my understanding above is correct).
corneliusroemer-agent
added a commit
to corneliusroemer-agent/LAPIS
that referenced
this pull request
Sep 10, 2026
The 100 ms timeout added in GenSpectrum#1593 sits on callInfo(), but only the health endpoint wants it. The same call also serves /sample/info, the once-a-second data version poll, and query parsing, and all of those inherit a budget that was never chosen for them: a SILO that is merely slow makes /sample/info fail rather than answer. Move the timeout to the caller that wants it. callInfo() takes an optional timeout, SiloHealthIndicator passes silo.healthCheckTimeout (default 100ms, so the health path is unchanged), and the other callers no longer carry a health check's deadline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PRvw1U8cSHWotT9kL5r8FC
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
3 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.
I tested it locally by start SILO locally with a "sleep one minute" in the info endpoint.
PR Checklist
- [ ] All necessary documentation has been adapted.- [ ] All necessary changes are explained in thellms.txt.- [ ] The implemented feature is covered by an appropriate test.