Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
|
Warning Your comment is too long (maximum is 65536 characters), so the coverage report was not added. See the job log for how to reduce it. |
|
@all-hands-bot Please review the current head |
all-hands-bot
left a comment
There was a problem hiding this comment.
Review of 2031d65 — fix: tolerate transient SQLite write contention
Reviewed the full diff against main, ran the test suite and linters locally, and checked CI status for the head commit.
What the PR does
Configures every SQLite connection created by _create_sqlite_engine with three pragmas via SQLAlchemy's connect event listener on engine.sync_engine:
journal_mode=WAL— allows concurrent readers while a write is in progress, reducing contention.foreign_keys=ON— enforces referential integrity per connection (required on every connection since SQLite defaults this off).busy_timeout=30000— makes a writer wait up to 30 s for a lock rather than immediately raisingdatabase is locked.
The timeout connect arg is also set on the aiosqlite driver level, consistent with the pragma. No scheduler, dispatcher, or PostgreSQL paths are touched.
Assessment
- Correct approach: Using the
connectevent listener is the idiomatic SQLAlchemy pattern for per-connection pragmas. The listener is registered on each engine'ssync_engine, so there is no global state leakage across engine instances. - WAL persistence: WAL is a persistent database property; re-setting it on every connection is harmless (SQLite returns the current mode without rewriting).
- No behavioral risk for PostgreSQL: All changes are scoped to
_create_sqlite_engine; the asyncpg and GCP Cloud SQL paths are untouched. - Tests are meaningful:
test_configures_file_database_for_concurrent_service_tasksverifies all three pragmas.test_waits_for_a_short_lived_write_lockreproduces realBEGIN IMMEDIATEcontention with an externalsqlite3connection, asserts the service write blocks (not yet done after 100 ms), releases the lock, and confirms both writes commit. This directly exercises the failure mode described in the PR. - Local verification:
pytest tests/test_db.py— 29 passed.ruff checkandruff format --check— clean. CI for the head commit: all green (backend, unit-tests, lint).
No blocking findings. Approving.
Generated by OpenHands AI on behalf of the user.
HUMAN:
AGENT:
Why
Short-lived SQLite write contention can currently abort dispatcher bookkeeping after a Docker entrypoint has started. The run then remains
RUNNINGwithout the identifiers needed for normal verification and cleanup.Summary
Configure every local SQLite connection with a 30-second bounded busy timeout, WAL journaling, and foreign-key enforcement. This reuses SQLAlchemy's connection hook and changes no scheduler, dispatcher, or PostgreSQL path.
A regression holds a real
BEGIN IMMEDIATEwrite lock, starts a service write on another connection, releases the lock, and verifies that both writes commit. A separate check verifies the three connection pragmas.Issue Number
Closes #461.
How to Test
uv run pytest tests/ -q --ignore=tests/integration— 1742 passed, 7 skippeduv run ruff check openhands/automation/db.py tests/test_db.pyuv run ruff format --check openhands/automation/db.py tests/test_db.pyThe live failure was reproduced in an isolated Canvas factory while four UI-managed schedules and dispatcher bookkeeping wrote concurrently. The orphaned runtime was released through the SDK after its worker exited.
Relationship
Independent
main-based fix. It does not depend on the software-factory dispatch/profile stack.