fix(server): bound request/statement time and raise ALB idle timeout - #87
Open
saturninoabril wants to merge 3 commits into
Open
fix(server): bound request/statement time and raise ALB idle timeout#87saturninoabril wants to merge 3 commits into
saturninoabril wants to merge 3 commits into
Conversation
Add request-path guardrails so a slow query can't hold a pool connection until the load balancer 504s the request (the observed pool-exhaustion cascade under polling load): - DB pool: per-statement timeout (TSIO_DB_STATEMENT_TIMEOUT_MS, default 30s) and configurable MaxConns (TSIO_DB_MAX_CONNS) via variadic pool options; a slow statement is aborted server-side and the connection returns to the pool. - Public read (GET) routes: per-request timeout (TSIO_READ_REQUEST_TIMEOUT, default 30s) via chi middleware, cancelling the request context so the in-flight query aborts. WebSocket and write/upload routes are exempt. - ALB: explicit idleTimeout of 120s (above the AWS 60s default) so a slow-but-completing request isn't guillotined while holding a backend connection; the app-level ~30s bounds are the real limit. Co-authored-by: saturnino <saturnino@mattermost.com>
Co-authored-by: saturnino <saturnino@mattermost.com>
saturninoabril
marked this pull request as ready for review
July 24, 2026 09:38
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds environment-configurable PostgreSQL pool and statement limits, public read request timeouts, WebSocket keepalive pings, and a 120-second ALB idle timeout. These settings are loaded, tested, wired into runtime components, and validated in infrastructure tests. ChangesTimeout and database guardrails
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)Public read routingsequenceDiagram
participant Client
participant HTTPServer
participant ReadTimeout
participant PublicReadHandlers
Client->>HTTPServer: request public API route
HTTPServer->>ReadTimeout: apply configured timeout
ReadTimeout->>PublicReadHandlers: dispatch report or orchestration read
PublicReadHandlers-->>Client: return read response
WebSocket keepalivesequenceDiagram
participant WebSocketClient
participant serve
participant WebSocketConnection
participant ConnectionContext
serve->>WebSocketConnection: start periodic Ping calls
WebSocketConnection-->>WebSocketClient: send ping
WebSocketConnection->>ConnectionContext: cancel on ping failure
ConnectionContext-->>serve: stop forwarding and close connection
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/internal/db/pool.go (1)
50-57: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep
MinConnsat or belowMaxConns.NewPoolhard-codesMinConns = 2, butWithMaxConns(1)is still allowed, which leaves the pool configured with a floor above its cap. ClampMinConnsafter applying options or reject smaller max values.🤖 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 `@apps/server/internal/db/pool.go` around lines 50 - 57, Update NewPool so the final configuration always keeps MinConns at or below MaxConns after all opts, including WithMaxConns, are applied. Clamp MinConns to MaxConns or reject invalid smaller max values, while preserving the existing defaults for valid configurations.Source: MCP tools
🤖 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 `@apps/server/internal/db/pool.go`:
- Around line 35-40: Update WithStatementTimeout so a zero ms value removes the
existing "statement_timeout" entry from cfg.ConnConfig.RuntimeParams, while
positive values continue setting it via strconv.Itoa. Preserve the current
behavior for positive timeouts.
In `@infra/lib/constructs/networking.ts`:
- Around line 133-136: Update the WebSocket handling associated with the
networking construct’s idleTimeout configuration to keep quiet sessions alive
beyond 120 seconds by adding periodic ping/pong heartbeat traffic, or raise
idleTimeout to the required session duration if heartbeats are not supported.
Preserve the existing request timeout behavior for regular backend connections.
---
Outside diff comments:
In `@apps/server/internal/db/pool.go`:
- Around line 50-57: Update NewPool so the final configuration always keeps
MinConns at or below MaxConns after all opts, including WithMaxConns, are
applied. Clamp MinConns to MaxConns or reject invalid smaller max values, while
preserving the existing defaults for valid configurations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9886953a-aec0-4526-b6b1-9aee1a84911e
📒 Files selected for processing (9)
.env.exampleapps/server/cmd/tsio/main.goapps/server/internal/config/config.goapps/server/internal/config/config_test.goapps/server/internal/db/pool.goapps/server/internal/db/pool_test.goapps/server/internal/server/server.goinfra/lib/constructs/networking.tsinfra/test/app_stack.test.ts
… keepalive - WithStatementTimeout(0) now sets statement_timeout=0 (explicit disable), overriding any value inherited from the connection string, so the documented '0 disables' contract holds; negative leaves the default. - Add a 30s WebSocket server ping so quiet subscriptions aren't dropped by the 120s ALB idle timeout before the next event arrives (coder/websocket Ping is concurrency-safe; a failed ping tears the connection down). Co-authored-by: saturnino <saturnino@mattermost.com>
saturninoabril
requested review from
yasserfaraazkhan
and removed request for
yasserfaraazkhan
July 24, 2026 13:42
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
2 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.
Summary
First PR in the sequenced 504-mitigation work. Adds request-path guardrails so a slow query cannot hold a
pgxpool connection until the load balancer 504s the request — the pool-exhaustion cascade behind the observed timeouts (heavy uncached reads competing with ingest/checkout for a 20-connection pool, no server-side time bound, ALB idle timeout at the AWS 60s default).Changes:
internal/db/pool.go): variadicPoolOptions —WithStatementTimeoutsets a per-statementstatement_timeout(default 30s viaTSIO_DB_STATEMENT_TIMEOUT_MS) so a slow statement is aborted server-side and its connection returns to the pool;WithMaxConnsmakes pool size configurable (TSIO_DB_MAX_CONNS, default 20). Per-statement, so background multi-statement work (JSON extractor) is unaffected.internal/server/server.go): report + orchestration-status GETs are grouped under a per-requestchitimeout (default 30s viaTSIO_READ_REQUEST_TIMEOUT), cancelling the request context so the in-flight query aborts. WebSocket (/ws) and write/upload routes are intentionally exempt.infra/lib/constructs/networking.ts): explicitidleTimeoutof 120s (above the 60s default) so a slow-but-completing request isn't guillotined while still holding a backend connection; the ~30s app-level bounds are the real limit.No response-shape or API-contract changes. All knobs default to safe values (behaviour unchanged unless configured), except the new statement/read timeouts and ALB idle timeout, which only bound pathological cases.
Test Plan
go build ./...,go vet, full non-e2e suite (15 packages) green; new unit tests for pool options and config defaults/overrides.SELECT pg_sleep(3)aborted at ~500ms withSQLSTATE 57014and the connection was reusable afterward; baseline (no timeout)pg_sleep(1)succeeded./ws-style route was unaffected.tsc --noEmit,oxlint, andvitest(15 tests incl. newidle_timeout.timeout_seconds=120assertion) all pass.Release Note