Repository navigation
fix(relay): treat database pool connect failures as transient, not director faults - #21243
Merged
Merged
Conversation
…ults pg-pool raises connection-acquire failures as a plain Error with no SQLSTATE, so the transient classifier matched only one of the three messages it can produce. The other two reached the routes unclassified and became HTTP 500s, which is what the rollout safety gate counts. The acquire boundary now marks the errors it produces, so "Connection terminated unexpectedly" counts as transient when the socket died during the handshake and stays a hard failure mid-statement, where a retry could repeat a commit whose outcome is unknown. /v1/regions and /v1/admin/evacuation-status gain the transient handling /v1/assign and /v1/resolve already had.
The query-failure event's connectionTimeout boolean matched one of the two messages connectionTimeoutMillis can produce, so the 210 dialling timeouts in the last day logged as false and were invisible to the field meant to find them. The pool-connect vocabulary now lives beside the acquire boundary that owns it, and both the router's classifier and the diagnostics read it from there, so the two cannot drift. The event also carries the routing verdict the caller already computed, making "how much of this burst reached users as a 500" one field.
The classifier now runs inside the query catch, where a thrown null or undefined would have turned a database failure into a TypeError that buried it. The diagnostics doc claimed transient maps to a 503 or a 500. Sweeps, startup reconciliation, and admin routes that answer 409 all emit the same event, so counting the false ones over-states user-facing hard failures.
Jinwoo-H
marked this pull request as ready for review
September 17, 2026 16:21
Contributor
|
This run was cancelled 🛑 The workflow was cancelled before completion. Please check the link below for details. |
Contributor
📝 WalkthroughWalkthroughThe change adds pool-acquire failure tracking and centralized PostgreSQL transient-error classification. Query diagnostics now include the caller-provided transient verdict. The regions and evacuation-status routes return 503 responses for transient database failures while rethrowing non-transient errors. Tests and diagnostics documentation cover the new classifications and responses. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Diagnostics can misidentify queue timeouts as connection dials, and the new evacuation retry response is not protected by a route-level failure test. These are bounded issues but should be addressed before relying on the change operationally. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 8 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 2b082c1e-12e9-4764-8879-123cfc0e0b2c
📒 Files selected for processing (9)
cloud/apps/relay/src/app.tscloud/apps/relay/src/database-transient-error.test.tscloud/apps/relay/src/database.tscloud/apps/relay/src/postgres-pool-pressure.tscloud/apps/relay/src/postgres-query-failure-postgres.test.tscloud/apps/relay/src/postgres-query-failure.test.tscloud/apps/relay/src/postgres-query-failure.tscloud/apps/relay/src/relay-region-app.test.tscloud/docs/relay-database-failure-diagnostics.md
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Comment on lines
+1318
to
+1328
| try { | ||
| const status = await operations.assignments.cellEvacuationStatus( | ||
| body.data.sourceCellId, | ||
| body.data.targetCellId, | ||
| body.data.completeReady | ||
| ) | ||
| return context.json({ v: 1, ...status }) | ||
| } catch (error) { | ||
| if (!isRelayDatabaseTransientError(error)) throw error | ||
| return context.json({ error: 'database_temporarily_unavailable' }, 503) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n -C 5 'evacuation-status|cellEvacuationStatus|database_temporarily_unavailable|verifyReadOnlyAdminToken' cloud/apps/relay/src --glob '*test.ts'
sed -n '1305,1335p' cloud/apps/relay/src/app.ts
sed -n '250,315p' cloud/apps/relay/src/relay-region-app.test.tsRepository: stablyai/orca
Length of output: 40202
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- route-focused blackbox tests ---'
sed -n '2040,2195p' cloud/apps/relay/src/relay.blackbox.test.ts
sed -n '2480,2655p' cloud/apps/relay/src/relay.blackbox.test.ts
printf '%s\n' '--- relay-region test structure and region route ---'
sed -n '1,120p' cloud/apps/relay/src/relay-region-app.test.ts
sed -n '210,325p' cloud/apps/relay/src/relay-region-app.test.ts
printf '%s\n' '--- transient classifier and route registration ---'
rg -n -C 8 'function isRelayDatabaseTransientError|isRelayDatabaseTransientError|app\.post.*/v1/admin/evacuation-status|cellEvacuationStatus' cloud/apps/relay/src --glob '*.ts'Repository: stablyai/orca
Length of output: 50369
Test the transient mapping on POST /v1/admin/evacuation-status. Existing tests cover successful requests and authentication responses, but none make cellEvacuationStatus() reject with a classified transient error. Add an authenticated request and assert status 503 with { error: 'database_temporarily_unavailable' }. The /v1/regions test stubs assignments.regionCatalog, so it cannot detect a regression in this handler.
Comment on lines
+13
to
+15
| time includes acquisition. An acquisition timeout can mean either waiting in the | ||
| queue or establishing a new connection; `connectionTimeout` covers both, and the | ||
| pool counts separate them: a queue wait has waiters, a dial does not. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '60,150p' cloud/apps/relay/src/postgres-pool-pressure.ts
sed -n '945,985p' cloud/apps/relay/src/database.ts
sed -n '1,90p' cloud/apps/relay/src/postgres-query-failure.ts
sed -n '8,20p' cloud/docs/relay-database-failure-diagnostics.md
rg -n -C 3 'databasePoolWaiting|waitingCount|databasePoolWaitersMax|databasePoolWaitMsMax|reportPostgresQueryFailure' cloud/apps/relay/srcRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- package bindings ---'
rg -n -C 2 '"(pg|pg-pool)"|from ['"'"']pg|from ['"'"']pg-pool' package.json package-lock.json pnpm-lock.yaml yarn.lock cloud package.json 2>/dev/null | head -120
printf '%s\n' '--- failure reporter tests ---'
sed -n '1,220p' cloud/apps/relay/src/postgres-query-failure.test.ts
printf '%s\n' '--- pressure tests ---'
sed -n '1,180p' cloud/apps/relay/src/postgres-pool-pressure.test.ts
printf '%s\n' '--- local package source candidates ---'
find . -path '*/node_modules/pg-pool/*' -o -path '*/node_modules/pg/lib/*' 2>/dev/null | head -80Repository: stablyai/orca
Length of output: 13280
🏁 Script executed:
printf '%s\n' '--- bindings ---'
rg -n -C 2 '"pg(-pool)?"|from ['"'"']pg' package.json package-lock.json pnpm-lock.yaml yarn.lock cloud/package.json cloud/apps/relay/package.json 2>/dev/null | head -160
printf '%s\n' '--- reporter test ---'
sed -n '1,180p' cloud/apps/relay/src/postgres-query-failure.test.ts
printf '%s\n' '--- pressure test ---'
sed -n '1,100p' cloud/apps/relay/src/postgres-pool-pressure.test.tsRepository: stablyai/orca
Length of output: 7718
🌐 Web query:
pg-pool source connectionTimeoutMillis pendingQueue waitingCount timeout response.callback
💡 Result:
<search_synthesis>
In the context of the node-postgres (pg-pool) library, these terms relate to pool configuration, internal queue management, and health monitoring [1][2][3]. connectionTimeoutMillis This is a configuration parameter for the Pool constructor [1][2]. It defines the number of milliseconds the pool will wait to establish a new connection to the database before timing out [1][2]. By default, it is set to 0, which means there is no timeout [2]. If a connection cannot be established within this time, the request will fail, typically with a timeout error [4]. pendingQueue Internally, pg-pool maintains a _pendingQueue (an array of PendingItem objects) [3]. This queue stores callbacks for requests that are waiting to check out a client from the pool when all available connections are currently in use [3]. waitingCount This is a public property exposed by the Pool instance (pool.waitingCount) [2][5]. It represents the current number of requests waiting in the _pendingQueue for an available client [2][3]. Monitoring this metric is a standard best practice for detecting pool saturation; if waitingCount is greater than zero, it indicates that the pool is at its maximum capacity (max) and incoming requests are queued [6][5]. response.callback In the context of internal pool operation, when a user calls pool.connect or pool.query, the library creates a task or "PendingItem" [3]. This object encapsulates the user's callback function (the "response" callback) which is executed once a connection is successfully acquired from the pool or rejected due to a timeout or other error [3]. Summary of Pool Health Metrics Developers typically monitor three primary properties to assess pool health [6][5]: 1. totalCount: The total number of client instances (idle + in use) [3][5]. 2. idleCount: The number of clients currently available in the pool [3][5]. 3. waitingCount: The number of queued requests waiting for a client [3][5]. If you observe waitingCount > 0 while idleCount is 0, your application is under load and waiting for database connections to become available [6]. Consistent saturation may indicate a need to increase the pool max size or optimize query execution time [2][6].
</search_synthesis>
<source_evidence>
Citations:
- 1: https://github.com/brianc/node-postgres/tree/master/packages/pg-pool
- 2: https://node-postgres.com/apis/pool
- 3: https://deepwiki.com/brianc/node-postgres/4-connection-pooling
- 4: GitHub issue 3543 in brianc/node-postgres (link omitted to avoid creating a cross-reference)
- 5: https://www.nazarboyko.com/articles/database-connection-pooling-in-nodejs
- 6: https://the-practical-developer.online/posts/database-connection-pool-leaks-nodejs-postgres/
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- lockfiles ---'
find . -maxdepth 3 -type f \( -name 'package-lock.json' -o -name 'pnpm-lock.yaml' -o -name 'yarn.lock' -o -name 'bun.lockb' -o -name 'bun.lock' \) -print
printf '%s\n' '--- pg dependency references ---'
rg -n -C 3 'pg-pool|pg@|/pg/|pg:|version: 8\.22|8\.22\.0' . --glob 'package-lock.json' --glob 'pnpm-lock.yaml' --glob 'yarn.lock' --glob 'package.json' --glob 'bun.lock' 2>/dev/null | head -240Repository: stablyai/orca
Length of output: 5074
🌐 Web query:
node-postgres pg-pool v3.10.1 source PendingItem timedOut connectionTimeoutMillis waitingCount
💡 Result:
<search_synthesis>
In pg-pool, PendingItem is an internal class used to track requests for a connection that are currently waiting in the pool's queue [1]. When you call pool.connect, if no idle connections are available and the pool is at its capacity limit, a new PendingItem is created and added to the pool's _pendingQueue [1]. The property waitingCount reflects the length of this _pendingQueue [2][3]. If your waitingCount is high, it indicates that many requests are blocked waiting for an available connection [4]. Regarding connectionTimeoutMillis and timedOut: 1. When connectionTimeoutMillis is configured, a timer is set for each acquisition attempt [1]. 2. If this timer expires before a connection is acquired, the PendingItem is marked with timedOut = true [1]. 3. Historically, this mechanism had significant flaws, particularly in version 3.10.1 and earlier, leading to connection leaks and "hanging" pool connections [5][6][7]. Specifically: - Race conditions allowed connections to be established and added to the pool even after the timeout had already triggered [8][7]. - If no specific client condition was met when the timer fired (e.g., during a stalled DNS lookup), the timer would do nothing, causing pool.connect to hang indefinitely [6]. These issues have been addressed in later versions of the library, where the timeout now explicitly rejects the checkout, cleans up the pending state, and ensures that late-completing connections are discarded rather than leaked [8][6][7]. If you are experiencing issues with connection timeouts or leaks, updating to a version newer than 3.10.1 is strongly recommended [9].
</search_synthesis>
<source_evidence>
Citations:
- 1: https://github.com/brianc/node-postgres/blob/9d2c977ce9b13f8f3b024759b1deaec165564a6a/packages/pg-pool/index.js
- 2: https://deepwiki.com/brianc/node-postgres/4-connection-pooling
- 3: https://cdn.jsdelivr.net/npm/@agentuity/postgres@3.1.18/src/pool.ts
- 4: GitHub issue 2366 in brianc/node-postgres (link omitted to avoid creating a cross-reference)
- 5: GitHub issue 3543 in brianc/node-postgres (link omitted to avoid creating a cross-reference)
- 6: GitHub issue 3197 in brianc/node-postgres (link omitted to avoid creating a cross-reference)
- 7: https://dev.to/malbolged/smashing-a-silent-connection-leak-in-node-postgres-pg-pool-4078
- 8: GitHub pull request 3711 in brianc/node-postgres (link omitted to avoid creating a cross-reference)
- 9: GitHub issue 3555 in brianc/node-postgres (link omitted to avoid creating a cross-reference)
Preserve acquire-time waiter state for diagnostics. PostgresDatabase.query() reports the failure after PostgresPoolPressure.connect() rejects. The custom waiter is removed in finally, and pg-pool@3.14.0 removes the timed-out request from its pending queue before rejecting it. reportPostgresQueryFailure() then reads pool.waitingCount, which can be zero for a queue timeout, just like a dial timeout. The event does not include the retained pressure high-water metrics. Preserve acquire-time queue state in the event, or remove this queue-versus-dial distinction from the documentation.
9 tasks done
ivanvykopal
pushed a commit
to ivanvykopal/orca
that referenced
this pull request
Sep 20, 2026
…rector faults (stablyai#21243) * fix(relay): treat pool connect failures as transient, not director faults pg-pool raises connection-acquire failures as a plain Error with no SQLSTATE, so the transient classifier matched only one of the three messages it can produce. The other two reached the routes unclassified and became HTTP 500s, which is what the rollout safety gate counts. The acquire boundary now marks the errors it produces, so "Connection terminated unexpectedly" counts as transient when the socket died during the handshake and stays a hard failure mid-statement, where a retry could repeat a commit whose outcome is unknown. /v1/regions and /v1/admin/evacuation-status gain the transient handling /v1/assign and /v1/resolve already had. * fix(relay): mirror the pool-connect verdict in failure diagnostics The query-failure event's connectionTimeout boolean matched one of the two messages connectionTimeoutMillis can produce, so the 210 dialling timeouts in the last day logged as false and were invisible to the field meant to find them. The pool-connect vocabulary now lives beside the acquire boundary that owns it, and both the router's classifier and the diagnostics read it from there, so the two cannot drift. The event also carries the routing verdict the caller already computed, making "how much of this burst reached users as a 500" one field. * fix(relay): null-safe transient classification and honest transient docs The classifier now runs inside the query catch, where a thrown null or undefined would have turned a database failure into a TypeError that buried it. The diagnostics doc claimed transient maps to a 503 or a 500. Sweeps, startup reconciliation, and admin routes that answer 409 all emit the same event, so counting the false ones over-states user-facing hard failures.
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.
ELI5
The relay's traffic director keeps a small set of open database connections and hands one out for each request. When it cannot hand one out in time, it used to report "the server is broken" (HTTP 500) instead of "I am busy, try again shortly" (HTTP 503). Nothing was actually broken, and the wrong answer made our deployment safety check think the service was failing. This teaches it to say "busy, try again" for exactly the cases where no database work had started yet, and fixes the log field that was supposed to make this easy to spot and did not.
What Changed
Before. In the last 24 hours of production, the director returned real HTTP 500s in bursts roughly every 10 minutes. 271 database-connection failures occurred; only 10 of them were recognised as temporary. The other 261 fell through as unhandled errors:
The 210 all failed while acquiring a connection, at about 2002 ms elapsed, which is exactly the 2-second pool connect timeout, with nothing queued behind them. Users hit these on assignment and lookup requests. Our rollout safety gate counts 500s and ignores 503s, so these bursts also made every deployment look unsafe.
After. The same failures return 503 with the retry and backoff semantics those routes already have, and callers retry instead of seeing a hard failure.
Mechanism. The connection pool library raises acquire failures as a plain error with no SQLSTATE code, so the message is the only thing to match on. The classifier that decides "temporary" matched exactly one message. It now matches all three, with one important distinction:
That vocabulary now lives beside the acquire boundary that owns it, rather than inline in the classifier, because a second place was reading it.
Two more routes gained the handling the assignment routes already had. The region catalog endpoint had no error handling at all, and the admin evacuation-status endpoint was uncaught.
Diagnostics. The database failure log has a
connectionTimeoutboolean whose documented job is to flag exactly this condition. It matched only one of the two messages the 2-second timeout can produce, so all 210 dialling timeouts were logged asconnectionTimeout: false. The field meant to find this burst was blind to 97% of it, which is why the messages had to be counted by hand. It now reads the same predicate the router does, so the two cannot drift.The event also gained a
transientboolean carrying the routing verdict the caller had already computed. Counting that field answers "how much of this burst reached users as a hard failure" directly. It is passed in rather than re-derived, so there is still one classifier.Why
These are not director faults. A brief inability to open a database connection is the textbook case for "try again," and the rest of the relay already treats it that way. Reporting it as a 500 told clients the wrong thing and poisoned the deployment safety signal.
Alternatives considered:
Linked Issue
No tracked issue. This was found by reading 24 hours of production director logs while investigating why the pre-roll safety gate kept tripping.
Fixes #
Visual Proof
N/A— server-side HTTP status code change with no user interface surface.Testing
pnpm testincloud/apps/relay, first with no database: 64 files / 661 tests pass, 21 files / 110 tests skipped (they need Postgres).pnpm testincloud/apps/relayagainst a freshpostgres:16-alpinecontainer withORCA_RELAY_TEST_POSTGRES_URLset, matching CI: 85 files / 772 tests pass, 1 skipped, nothing skipped at the file level.pnpm testincloud(the whole workspace, as CI runs it): all package suites pass plus 598 deployment-script tests, 0 failures.pnpm typecheckandpnpm buildincloud, which is how CI compiles this workspace: both clean.Reviewer steps: start Postgres, then run the relay suite.
New tests cover the classifier for the pool connect timeout, the acquire-gated versus mid-statement split for the early-ended socket, a failed acquire that is not transient (a bad password stays a hard failure), the region catalog route returning 503 with
Retry-Afterfor a transient failure while still failing loudly for a programming error, and the two diagnostics booleans across a dialling timeout, an early-ended socket, a rejected password, and each execute-phase code.One existing test was tightened rather than merely updated. It asserted three execute-phase codes together; it now carries the expected verdict per code, because
ECONNRESETafter a statement went out is deliberately not transient while57014and55P03are. That pins the mid-statement boundary this change must not cross.AI Disclosure
Review
Retry safety
Widening a "this is temporary" classifier is only safe if nothing retries a non-idempotent write on the strength of it. Every call site was checked.
Four of the five call sites are request handlers in
app.ts. None of them retries anything. They translate the error into a 503 response and the client decides. For the assignment write path, a 503 for the very same operation is already returned on the capacity branch, so the client-side retry semantics are unchanged in kind.The one place that does retry server-side is startup cell reconciliation, which retries up to 20 times inside a 45-second window. That work is a single transaction of
INSERT ... ON CONFLICT DO UPDATEover the configured cell list, so re-running it converges to the same state. The practical effect of the widening there is that a pool connect failure during startup now retries instead of throwing and killing the container.The retry loop that could genuinely repeat a write is the in-transaction retry inside the database layer. It is keyed on a separate function that matches SQLSTATE codes only, and this change does not touch it.
The acquire-boundary gate is what keeps the widening honest: an error that reaches a caller with a statement already in flight is never newly classified as temporary.
Compatibility
The region catalog is fetched by the desktop client, which throws on any non-OK status, so 500 and 503 are equivalent to it. The evacuation-status endpoint is called by three deployment scripts; two throw on any non-OK status, and the third already retries both 500 and 503 for this path. No caller regresses.
Cells are unaffected: every
/v1route returns 404 for the cell role.The diagnostics event gains a field and corrects one. Adding
transientis additive for any log reader. CorrectingconnectionTimeoutmakes it match its documented meaning, so a saved query counting it will start seeing the dialling timeouts it was always supposed to include. The doc is updated in the same commit.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Security: no change to authentication, authorization, or what any response body reveals. The 503 bodies name a condition, not an internal error string. The diagnostics event stays bounded to categories and booleans; no message, query text, parameter, or identifier is emitted, and a test asserts that private strings never reach the log.
Cross-platform, SSH, and mobile: server-side only. Mobile and desktop clients see a status code they already handle.
Backwards compatibility: no wire or schema change. Clients that treated 500 as fatal now get a retryable status for the same condition, which is strictly better for them.
Performance: one
WeakSetinsert on the connection-failure path only. Nothing on the success path.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)