Skip to content

fix: harden Postgres async engine with pool_recycle and command_timeout to stop stale-connection 504s#4230

Open
mvanhorn wants to merge 2 commits into
bytedance:mainfrom
mvanhorn:fix/3882-postgres-engine-pool-recycle
Open

fix: harden Postgres async engine with pool_recycle and command_timeout to stop stale-connection 504s#4230
mvanhorn wants to merge 2 commits into
bytedance:mainfrom
mvanhorn:fix/3882-postgres-engine-pool-recycle

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

Fixes #3882

Why

The reported 504s came from a stale half-open Postgres connection hanging at SQLAlchemy pool checkout. The async engine sets pool_pre_ping=True but has no pool_recycle and no asyncpg command_timeout/keepalive, so a connection killed server-side can wedge a request until timeout.

What changed

The Postgres async engine is hardened with pool_recycle and an asyncpg command_timeout via a shared helper applied at both create_async_engine call sites, so stale connections are recycled instead of hanging.

Surface area

  • Frontend UI
  • Backend API
  • Agents / LangGraph
  • Sandbox
  • Skills
  • Dependencies
  • Default behavior change
  • Docs / tests / CI only

Screenshots / Recording

N/A - backend connection-pool hardening.

Bug fix verification

  • Test path that reproduces the bug: added a test asserting the engine is configured with pool_recycle and command_timeout at both call sites.
  • If a red test wasn't cheap to write: a true live-repro needs a killed Postgres connection under load; the added test pins the engine configuration that prevents the wedge.

Validation

Ran the backend engine/persistence tests covering the new configuration.

AI assistance

Tool(s) used: Codex

How you used it: Implemented the fix from a written plan (root cause pinned to the engine config); I reviewed the diff before submission.

  • I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

@github-actions github-actions Bot added area:backend Gateway / runtime / core backend under backend/ risk:medium Medium risk: regular code changes size/M PR changes 100-300 lines labels Jul 16, 2026

@fancyboi999 fancyboi999 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mvanhorn. I found one issue that should be addressed before this is ready.

Reviewed base SHA: b3a0dac8ad191fbfa248c0a1cecc8f2deeaa1c7d
Reviewed head SHA: e7b19240c556eae5edb30e62beca964201457c1d

[P1] Keep the database timeout inside the proxy deadline

  • Location: backend/packages/harness/deerflow/persistence/engine.py:22
  • Problem: POSTGRES_COMMAND_TIMEOUT_SECONDS is fixed at 60 seconds, which is the same deadline nginx applies to ordinary /api/* routes. A stale checkout or pre-ping that reaches this timeout still loses the race to nginx, so clients continue to receive the 504 this PR is meant to prevent. The constant also imposes an unconfigurable 60-second cap on every app-ORM command, including legitimate long-running operations.
  • Evidence: the Docker nginx locations for /api/agents, /api/skills, and the /api/ catch-all do not override proxy_read_timeout, so nginx uses its 60-second default. The issue evidence records rt=60.000 and rt=60.034 504s. The locked asyncpg 0.31.0 source defines command_timeout as the default timeout for operations on the connection, so setting it to the same 60-second boundary leaves no time for Gateway to turn the database timeout into an upstream response.
  • Suggested fix: make the app-ORM command timeout a database setting and choose a default comfortably below the proxy deadline (for example 30 seconds), while keeping the recycle policy. Preserve an explicit opt-out or larger override for deployments that intentionally run longer database commands.
  • Test: add a regression that stalls the asyncpg ping/command past the configured database deadline and asserts checkout/request handling terminates before nginx's 60-second boundary; also cover the config override so a legitimate longer timeout is not silently clamped.

…se setting

Default the app-ORM command timeout to 30s (below nginx's 60s proxy deadline), expose it as database.command_timeout with null to disable, and add regression coverage. Addresses P1 review feedback.

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@github-actions github-actions Bot added the area:docs Documentation and Markdown only label Jul 16, 2026
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Fixed in 3087049. The app-ORM command timeout is now a database.command_timeout setting defaulting to 30s (comfortably under nginx's 60s deadline), with null to disable it for intentionally long commands, and the recycle policy is unchanged. Added a regression that stalls the asyncpg command past the configured deadline and a case asserting a larger override isn't clamped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Gateway / runtime / core backend under backend/ area:docs Documentation and Markdown only risk:medium Medium risk: regular code changes size/M PR changes 100-300 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] nginx频繁报超时504错误

2 participants