feat(admin): Cap admin HTTP requests with a web-layer timeout#8169
Draft
pbhandari wants to merge 1 commit into
Draft
feat(admin): Cap admin HTTP requests with a web-layer timeout#8169pbhandari wants to merge 1 commit into
pbhandari wants to merge 1 commit into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
What
Adds a hard cap on how long a single snuba-admin HTTP request may run at the web layer. Admin is served by Granian (WSGI), which — unlike gunicorn — exposes no per-request timeout, so this is enforced inside the admin Flask app itself and is therefore scoped to admin only.
How
RequestTimeoutMiddleware(snuba/admin/request_timeout.py) wrapsapplication.wsgi_app. It runs each request in a daemon worker thread and joins with a deadline:408 Request Timeoutwith the same JSON error shape the other admin handlers use.Timeout is configurable via
ADMIN_REQUEST_TIMEOUT_SECONDS(env var, default 30s), set just above ClickHouse's existing 25s query cap so CH's own timeout fires first in the normal case and this stays a backstop.Known limitation
WSGI can't preempt a running thread, so on timeout the client gets its
408immediately but the abandoned worker thread runs to completion. This is acceptable here because ClickHouse queries are themselves capped at 25s, so the thread unwinds on its own shortly after. The middleware abandons the request; it does not kill the underlying work.Potential alternative:
pebbleIf we ever want true termination (actually killing in-flight work rather than abandoning it),
pebbleis the actively-maintained option — it runs work in a separate process that can be terminated on timeout. The tradeoff is that every request would have to pickle its args/results across a process boundary, which isn't worth it for a WSGI hot path given ClickHouse already self-caps. Noting it here for future reference.🤖 Generated with Claude Code