fix: set HTTP server read/write/idle timeouts#446
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/api.go`:
- Around line 43-48: The server in api.go currently uses hardcoded timeout
values (ReadHeaderTimeout, ReadTimeout, WriteTimeout, IdleTimeout) when
constructing the http.Server; make these configurable by adding corresponding
timeout fields to the API configuration struct (e.g., ReadHeaderTimeout,
ReadTimeout, WriteTimeout, IdleTimeout of type time.Duration) in
conf/configuration.go (with sensible defaults and env/config tags), load them
into the API config, and replace the hardcoded values in the server
initialization (the http.Server literal that sets Addr, Handler a.handler,
ReadHeaderTimeout, ReadTimeout, WriteTimeout, IdleTimeout) to use the config
values instead. Ensure parsing/decoding supports duration strings so operators
can set env vars like GOTRUE_API_WRITE_TIMEOUT=600s.
- Line 47: The server's fixed http.Server.WriteTimeout = 60 * time.Second can
abort long admin endpoints (e.g., handlers that call sendJSON which
json.Marshal's []*UserForExport) — make the endpoints safe by (1)
validating/capping pagination input (use defaultPerPage and enforce a maxPerPage
constant, validate page/per_page in the admin handlers that produce large
payloads such as /admin/users and /admin/users/export), (2) avoid buffering
entire payloads by replacing the sendJSON call in those handlers with a
streaming/chunked JSON encoder or by implementing an async export job for very
large exports, and (3) remove reliance on the global hard WriteTimeout for these
routes (either make it configurable or rely on request-context deadlines) so
long-running DB + marshal work isn't killed mid-response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| Addr: hostAndPort, | ||
| Handler: a.handler, | ||
| ReadHeaderTimeout: 10 * time.Second, | ||
| ReadTimeout: 30 * time.Second, | ||
| WriteTimeout: 60 * time.Second, | ||
| IdleTimeout: 120 * time.Second, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Make HTTP server timeouts configurable.
The timeout values are currently hardcoded, which prevents operators from tuning them based on their deployment characteristics (e.g., network conditions, workload patterns, security requirements). Different deployments may need different timeout values.
♻️ Proposed approach to make timeouts configurable
Add timeout configuration fields to the API configuration struct in conf/configuration.go:
API struct {
Host string
Port int `envconfig:"PORT" default:"8081"`
ReadHeaderTimeout time.Duration `split_words:"true" default:"10s"`
ReadTimeout time.Duration `split_words:"true" default:"30s"`
WriteTimeout time.Duration `split_words:"true" default:"300s"` // 5 minutes default
IdleTimeout time.Duration `split_words:"true" default:"120s"`
// ... existing fields
}Then update the server initialization to use these values:
server := &http.Server{
Addr: hostAndPort,
Handler: a.handler,
- ReadHeaderTimeout: 10 * time.Second,
- ReadTimeout: 30 * time.Second,
- WriteTimeout: 60 * time.Second,
- IdleTimeout: 120 * time.Second,
+ ReadHeaderTimeout: a.config.API.ReadHeaderTimeout,
+ ReadTimeout: a.config.API.ReadTimeout,
+ WriteTimeout: a.config.API.WriteTimeout,
+ IdleTimeout: a.config.API.IdleTimeout,
}This allows operators to configure timeouts via environment variables (e.g., GOTRUE_API_WRITE_TIMEOUT=600s) or configuration files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/api.go` around lines 43 - 48, The server in api.go currently uses
hardcoded timeout values (ReadHeaderTimeout, ReadTimeout, WriteTimeout,
IdleTimeout) when constructing the http.Server; make these configurable by
adding corresponding timeout fields to the API configuration struct (e.g.,
ReadHeaderTimeout, ReadTimeout, WriteTimeout, IdleTimeout of type time.Duration)
in conf/configuration.go (with sensible defaults and env/config tags), load them
into the API config, and replace the hardcoded values in the server
initialization (the http.Server literal that sets Addr, Handler a.handler,
ReadHeaderTimeout, ReadTimeout, WriteTimeout, IdleTimeout) to use the config
values instead. Ensure parsing/decoding supports duration strings so operators
can set env vars like GOTRUE_API_WRITE_TIMEOUT=600s.
| Handler: a.handler, | ||
| ReadHeaderTimeout: 10 * time.Second, | ||
| ReadTimeout: 30 * time.Second, | ||
| WriteTimeout: 60 * time.Second, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for patterns that indicate potentially long-running operations
echo "=== Searching for database query patterns that might be slow ==="
rg -n --type=go -C2 'db\.(Query|Find|Select)' api/
echo -e "\n=== Searching for external API calls or HTTP clients ==="
rg -n --type=go -C2 'http\.(Get|Post|Do|Client)' api/
echo -e "\n=== Searching for email sending operations ==="
rg -n --type=go -C2 '(SendMail|mailer\.|Email)' api/
echo -e "\n=== Checking export endpoint implementation ==="
rg -n --type=go -A20 'func.*adminExportUsers' api/
echo -e "\n=== Searching for time.Sleep or context.WithTimeout ==="
rg -n --type=go -C2 'time\.(Sleep|After)|context\.WithTimeout' api/Repository: netlify/gotrue
Length of output: 49225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== api/api.go: http.Server initialization around timeouts ==="
rg -n "http\.Server|ReadHeaderTimeout|ReadTimeout|WriteTimeout|IdleTimeout" api/api.go
echo
sed -n '1,120p' api/api.go
echo
sed -n '120,240p' api/api.go
echo "=== api/admin.go: adminExportUsers implementation (export endpoint) ==="
rg -n "func \\(a \\*API\\) adminExportUsers|/export|EXPORT_SECRET" api/admin.go
echo
sed -n '1,220p' api/admin.go
echo "=== api/api.go: where server is created/started (ListenAndServe) ==="
rg -n "ListenAndServe|Serve\\(|server\\.Serve\\(|Shutdown\\(|waitForTermination" api/api.go
echo
sed -n '1,120p' api/api.goRepository: netlify/gotrue
Length of output: 17421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== paginate() implementation + defaults ==="
rg -n "func paginate\\(" -S api
rg -n "paginate\\(" api/api.go api/admin.go api -S --glob='*.go'
# Print the paginate function
rg -n "func paginate\\(" api -S --glob='*.go' -m 20 | head -n 5
# Use fd/rg to locate file and then show a window
PFILE=$(rg -n "func paginate\\(" api -S --glob='*.go' | head -n1 | cut -d: -f1)
PLINE=$(rg -n "func paginate\\(" api -S --glob='*.go' | head -n1 | cut -d: -f2)
echo "paginate located at $PFILE:$PLINE"
sed -n "${PLINE},$((PLINE+160))p" "$PFILE"
echo
echo "=== sendJSON() implementation ==="
rg -n "func sendJSON\\(" api -S --glob='*.go'
SFILE=$(rg -n "func sendJSON\\(" api -S --glob='*.go' | head -n1 | cut -d: -f1)
SLINE=$(rg -n "func sendJSON\\(" api -S --glob='*.go' | head -n1 | cut -d: -f2)
echo "sendJSON located at $SFILE:$SLINE"
sed -n "${SLINE},$((SLINE+120))p" "$SFILE"
echo
echo "=== FindUsersForExportInAudience() implementation ==="
rg -n "FindUsersForExportInAudience" -S .
EFILE=$(rg -n "func \\(.*\\) FindUsersForExportInAudience|func FindUsersForExportInAudience" -S . --glob='*.go' | head -n1 | cut -d: -f1)
ELINE=$(rg -n "func \\(.*\\) FindUsersForExportInAudience|func FindUsersForExportInAudience" -S . --glob='*.go' | head -n1 | cut -d: -f2)
echo "export query located at $EFILE:$ELINE"
sed -n "${ELINE},$((ELINE+220))p" "$EFILE"
echo
echo "=== Query helper functions used by export query (optional hints) ==="
rg -n "FindUsersForExportInAudience" -S . --glob='*.go' -C2 | head -n 80Repository: netlify/gotrue
Length of output: 7174
🌐 Web query:
Go net/http http.Server WriteTimeout semantics includes time before first Write/handler execution
💡 Result:
The semantics of WriteTimeout in Go's net/http.Server are generally designed to cover the lifetime of the ServeHTTP handler stack [1][2], but they include nuances depending on the protocol and implementation details. In standard HTTP/1.x operation, WriteTimeout sets a write deadline on the underlying network connection immediately after the request headers have been read [3][2]. Consequently, for non-HTTPS connections, it covers the time from the completion of the header read until the completion of the response write [1][2]. This duration effectively encompasses the time spent executing the handler, including any processing, database calls, or internal operations that occur before the response is written [4]. There are two critical exceptions to this general behavior: 1. HTTPS/TLS Connections: For HTTPS connections, the server calls SetWriteDeadline immediately after the connection is accepted to also cover the TLS handshake [2]. In this specific case, WriteTimeout includes the time spent reading the request headers and waiting for the first byte of the request, in addition to the standard post-header handler execution time [2]. 2. Implementation Nuances: WriteTimeout is an absolute deadline imposed on the connection [4]. It is not a per-write timeout or an idle timeout [4][2]. Because it is implemented by setting a deadline on the connection, if the handler takes longer to process the request than the configured WriteTimeout, the server will time out when it eventually attempts to write to the response [1][4]. Because WriteTimeout is not a per-request handler timeout, it is often recommended to use http.TimeoutHandler or context-based timeouts if you require finer control over request execution time, as relying solely on WriteTimeout can be imprecise and may not trigger until the handler attempts a write [5][4][6].
Citations:
- 1: https://adam-p.ca/blog/2022/01/golang-http-server-timeouts/
- 2: https://blog.cloudflare.com/the-complete-guide-to-golang-net-http-timeouts/
- 3: https://github.com/golang/go/blob/b8c7fddd58432b6cbe12af221d275935a1ffc8f0/src/net/http/server.go
- 4: net/http: WriteTimeout times out writes before they're written golang/go#24461
- 5: https://ieftimov.com/posts/make-resilient-golang-net-http-servers-using-timeouts-deadlines-context-cancellation/
- 6: net/http: ResponseWriter.Write does not error after WriteTimeout nor is ErrorLog used golang/go#21389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "defaultPerPage" api/pagination.go api -S
echo "---- api/pagination.go ----"
sed -n '1,120p' api/pagination.go
echo "---- models/Pagination type (if present) ----"
rg -n "type Pagination" -S models api | head -n 20Repository: netlify/gotrue
Length of output: 1990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for per_page validation/caps ==="
rg -n "per_page|PerPage|maxPer|DefaultPer|cap.*per|limit.*per" api models -S --glob='*.go'
echo
echo "=== Inspect UserForExport struct and export-related model code ==="
# find UserForExport definition
ufile=$(rg -n "type UserForExport" -S models --glob='*.go' | head -n1 | cut -d: -f1)
uline=$(rg -n "type UserForExport" -S models --glob='*.go' | head -n1 | cut -d: -f2)
echo "UserForExport at $ufile:$uline"
sed -n "${uline},$((uline+200))p" "$ufile"
echo
echo "=== Inspect models FindUsersForExportInAudience surrounding code ==="
# find the function in models/user.go already known; re-open larger window
u2line=$(rg -n "func FindUsersForExportInAudience" -S models --glob='user.go' | head -n1 | cut -d: -f2)
sed -n "${u2line-40},$((u2line+120))p" "$ufile"Repository: netlify/gotrue
Length of output: 8416
Make http.Server.WriteTimeout: 60s safe for /admin/users/export (and other paginated admin JSON endpoints)
WriteTimeout acts as a hard deadline for the response write on the connection. This matters here because /admin/users/export builds the full payload in-memory: it queries users, then returns via sendJSON, which json.Marshals the entire []*UserForExport before the first Write. UserForExport includes large fields like app_metadata and user_metadata, so DB + marshal time can exceed 60s.
Also, per_page is client-controlled with no visible upper bound (defaultPerPage = 50), so a single request with a large per_page can create payloads/queries slow enough to hit the 60s deadline. This is also applicable to /admin/users since it uses the same pagination + sendJSON pattern.
Consider:
- Cap/validate
per_page(and possiblypage) for these admin list/export endpoints. - Make the timeout configurable (or use a context/request-level deadline) instead of a fixed
WriteTimeoutthat can kill long handlers mid-response. - For genuinely large exports, switch to streaming/chunked responses or an async/background export job rather than buffering the full JSON.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/api.go` at line 47, The server's fixed http.Server.WriteTimeout = 60 *
time.Second can abort long admin endpoints (e.g., handlers that call sendJSON
which json.Marshal's []*UserForExport) — make the endpoints safe by (1)
validating/capping pagination input (use defaultPerPage and enforce a maxPerPage
constant, validate page/per_page in the admin handlers that produce large
payloads such as /admin/users and /admin/users/export), (2) avoid buffering
entire payloads by replacing the sendJSON call in those handlers with a
streaming/chunked JSON encoder or by implementing an async export job for very
large exports, and (3) remove reliance on the global hard WriteTimeout for these
routes (either make it configurable or rely on request-context deadlines) so
long-running DB + marshal work isn't killed mid-response.
2 participants
- Summary
http.Serverwas constructed with no timeouts, which means Go uses its zero values (no timeout) and a slow client can hold a connection open indefinitely. This setsReadHeaderTimeout,ReadTimeout,WriteTimeout, andIdleTimeoutto standard values (10s, 30s, 60s, 120s) so we drop slowloris-style attacks before they tie up workers.- Test plan
- Description for the changelog
Add read, write, and idle timeouts to the HTTP server.
- A picture of a cute animal (not mandatory but encouraged)