Conversation
Contributor
|
👋 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. |
`AUTOMATION_DB_URL=postgresql+asyncpg://...` is the documented way to point a self-hosted deployment at Postgres, but `migrations/env.py` handed that URL straight to the sync `create_engine()`, so `alembic upgrade head` failed with `sqlalchemy.exc.MissingGreenlet`. Only the `sqlite+aiosqlite` prefix was rewritten to a sync driver. Generalize the helper into `normalize_url_for_alembic()`, which also maps `postgresql+asyncpg` to `postgresql+pg8000` — already a dependency, and already the driver `env.py` uses for its host/port path — and apply it everywhere a configured URL reaches alembic. A URL-configured Postgres engine now gets the same `AUTOMATION_DB_SSL_MODE` connect args as the host/port path. Claude-Session: https://claude.ai/code/session_01T5LhMv1aKbckQ6SLd4sPGZ
lkshrk
force-pushed
the
fix/alembic-async-url-normalization
branch
from
September 13, 2026 05:59
082ad5f to
30730df
Compare
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.
HUMAN:
Reproduced against a self-hosted deployment on Kubernetes with a CloudNativePG Postgres and
AUTOMATION_DB_URL=postgresql+asyncpg://…:alembic upgrade headfails withMissingGreenlet, and hand-rewriting the URL topostgresql+pg8000://makes it pass. Ranuv run pytest tests/test_db.pyand pre-commit locally on this branch.AGENT:
Problem
AUTOMATION_DB_URLis the documented way to point a self-hosted deployment at adatabase, and the Postgres form of it is
postgresql+asyncpg://…— that is whatthe agent-canvas Helm chart sets, and what
db.pyexpects for the applicationengine.
Alembic cannot use it.
migrations/env.pyget_engine()readsAUTOMATION_DB_URLfrom the environment and passes it straight to the syncsqlalchemy.create_engine(), normalizing only the SQLite prefix:With an asyncpg URL,
alembic upgrade headfails:Rewriting the URL by hand to
postgresql+pg8000://…makes the same commandsucceed, which is the shape of the fix: pg8000 is already a dependency and is
already the driver
env.pyuses for its host/port and Cloud SQL paths.Fix
db.py:normalize_sqlite_url_for_alembic()becomesnormalize_url_for_alembic(), driven by a smallALEMBIC_SYNC_DRIVERSmap —sqlite+aiosqlite→sqlite(unchanged behaviour) andpostgresql+asyncpg→postgresql+pg8000. Only a leading driver prefix isrewritten, so a URL whose password happens to contain the driver name is left
alone. Every other URL passes through untouched.
migrations/env.py:get_engine()andrun_migrations_offline()both run aconfigured
AUTOMATION_DB_URLthrough the normalizer. A URL that resolves topg8000 now also gets
_build_pg8000_connect_args(DB_SSL_MODE), soAUTOMATION_DB_SSL_MODEfinally applies to the URL path the same way italready applies to the host/port path.
app.py: uses the renamed helper when it setssqlalchemy.url.The helper is renamed rather than aliased, because an alias could not stay
backward compatible: the old name's contract was "asyncpg URLs are unchanged",
which is precisely the bug. Both call sites and the tests are updated; nothing
outside this repo imports it.
Verification
uv run pytest tests/test_db.py— 30 passed. The normalizer suite gainedcases for asyncpg → pg8000, prefix-only rewriting, a non-async Postgres
driver, and the empty string.
pre-commit(ruff format, ruff lint, pycodestyle,pyright) clean on the changed files.
AUTOMATION_DB_URL=postgresql+asyncpg://… alembic upgrade head --sqlrendersPostgres DDL, and the previously failing online path now builds a pg8000
engine instead of an asyncpg one.
migrations/env.pyexecutesrun_migrations_online()at import time, so it hasno direct unit test; the behaviour is covered through the shared helper.
No schema change, no new dependency, no behaviour change for SQLite or for
deployments configured via
AUTOMATION_DB_HOST/AUTOMATION_DB_PORT.