introduce db timeout

[wintonzheng]

Important

Introduce database statement timeout configuration in config.py and apply it in client.py for database connections.

• Configuration:
  • Add DATABASE_STATEMENT_TIMEOUT_MS in config.py with default 60000 ms.
• Database Client:
  • Update DB_CONNECT_ARGS in client.py to include statement_timeout using DATABASE_STATEMENT_TIMEOUT_MS.
  • Modify create_async_engine in AgentDB to use DB_CONNECT_ARGS.

^{This description was created by for 542d30e. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Incremental review on 942020f in 20 seconds

More details

• Looked at 41 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. skyvern/forge/sdk/db/client.py:81

• Draft comment:
  Consider adding an else clause to handle cases where the database string does not contain postgresql+asyncpg or postgresql+psycopg. This will ensure that DB_CONNECT_ARGS is always defined appropriately.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The current implementation sets 'DB_CONNECT_ARGS' with a default value and overrides it if 'postgresql+asyncpg' is in the database string. The comment suggests that there might be other cases that need specific handling, but without evidence of such cases, the default setting seems sufficient. The comment is speculative without strong evidence of an issue.
  I might be overlooking specific database configurations that require different 'DB_CONNECT_ARGS'. However, without evidence of such cases, the default setting appears to be a reasonable fallback.
  The default setting for 'DB_CONNECT_ARGS' should cover most cases unless there is specific evidence of other configurations needing different settings.
  The comment is speculative and lacks strong evidence of an issue. The default setting for 'DB_CONNECT_ARGS' should suffice unless specific cases are identified.

Workflow ID: wflow_av1ras7Rpf6wdanP

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 942020f in 23 seconds

More details

• Looked at 41 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_qJwLPaIApJ0j48UQ

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

Consider using DB_CONNECT_ARGS.update(...) instead of overwriting the dictionary to ensure other connection arguments are preserved.

Suggested change

	DB_CONNECT_ARGS = {"server_settings": {"statement_timeout": str(settings.DATABASE_STATEMENT_TIMEOUT_MS)}}
	DB_CONNECT_ARGS.update({"server_settings": {"statement_timeout": str(settings.DATABASE_STATEMENT_TIMEOUT_MS)}})

[ellipsis-dev]

👍 Looks good to me! Incremental review on 542d30e in 11 seconds

More details

• Looked at 20 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. skyvern/forge/sdk/db/client.py:82

• Draft comment:
  The statement_timeout value is already a string in settings.DATABASE_STATEMENT_TIMEOUT_MS, so there's no need to convert it to a string again. Consider removing the str() conversion.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a database statement timeout configuration. The implementation in client.py correctly updates the DB_CONNECT_ARGS based on the database string. However, the statement_timeout value for asyncpg is converted to a string, which is unnecessary since it is already a string in the settings.

Workflow ID: wflow_oC3h2mqOlUMqwngM

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.