Skip to content

fix: the defects the agents page turned up - #21

Merged
AlexeyShalaev merged 7 commits into
masterfrom
fix/agents-page-findings
Sep 6, 2026
Merged

fix: the defects the agents page turned up#21
AlexeyShalaev merged 7 commits into
masterfrom
fix/agents-page-findings

Conversation

@AlexeyShalaev

Copy link
Copy Markdown
Member

Writing docs/agents.md against the source turned up six defects. Every one was
reproduced before it was touched; each is below with the reproduction, the change, and
whether it breaks anything.

Nothing here is a breaking API change. Two behaviours do change for an existing caller:
asyncpg is now installed for you, and a string advisory-lock key hashes to a different
integer than it did — see finding 4.

1. import sqlalchemy_foundation_kit fails on a clean install

session/connection.py imports asyncpg at module scope (AsyncCConnection subclasses
asyncpg.Connection), the package __init__ imports it, and asyncpg was declared only
in the test dependency group.

$ uv venv clean && uv pip install --python clean/bin/python .
Installed 8 packages: annotated-types greenlet pydantic pydantic-core sqlalchemy
                      sqlalchemy-foundation-kit typing-extensions typing-inspection
$ clean/bin/python -c "import sqlalchemy_foundation_kit"
  File ".../sqlalchemy_foundation_kit/session/connection.py", line 5, in <module>
    import asyncpg
ModuleNotFoundError: No module named 'asyncpg'

Changed: asyncpg>=0.30.0,<1.0.0 added to dependencies. The library is
asyncpg-only by design — the DSN, the connection class and the connect_args in
create_async_session_manager are all asyncpg-specific — so declaring it is the honest
fix rather than making the import optional. README and the home page said only
sqlalchemy[asyncio] and pydantic were required; they now say what is true.

uv.lock changes by exactly the two lines this adds.

Guarded by tests/unit/test_distribution.py, which reads the installed metadata.

2. AsyncSessionManager.get_transaction() raised TypeError on every call

execution_options= was passed to the session factory unconditionally, and
Session.__init__ has no such keyword. No database needed:

m = AsyncSessionManager(url="postgresql+asyncpg://u:p@localhost:5432/db")
async with m.get_transaction() as s: ...
# TypeError: Session.__init__() got an unexpected keyword argument 'execution_options'

With and without isolation_level. The unit tests mocked the sessionmaker and asserted
the broken call shape; the integration suite never touched the manager at all.

Changed: the isolation level now travels with the connection checkout.
session.begin() does not provision a connection, so the session.connection() call
inside the block is the one that checks it out, and SQLAlchemy applies execution options
to a fresh connection before beginning its transaction.

Tests: one unit test now builds a session over a real engine — that path never
reaches the database, but it constructs the session, which is where the TypeError came
from — and get_transaction gains integration coverage for commit, rollback and the
isolation level, read back with SHOW transaction_isolation. Against the old
manager.py: 8 of the new tests fail.

3. Every isolation_level= argument on the unit of work raised

apply_isolation_level() awaited session.connection(), which checks a connection out
and begins its transaction, and only then set the level on it. PostgreSQL cannot change
the isolation level of a running transaction.

Against PostgreSQL 17:

transaction:      InvalidRequestError: This connection has already initialized a SQLAlchemy
                  Transaction() object via begin() or autobegin; isolation_level may not be
                  altered unless rollback() or commit() is called first.
managed_session:  InvalidRequestError: ...
query:            InvalidRequestError: ...

Changed: the level is handed to the session.connection() call that checks the
connection out. That call starts the session's transaction, so transaction() and
managed_session() join the transaction that is already open instead of calling
session.begin() on top of it — begin() refuses a second one. transaction() drives
its own commit/rollback for the same reason; the flush-before-commit behaviour is
unchanged.

Applying the level inside open_session() (rather than after the transaction starts) is
deliberate: the documented open_session override runs its own statements — a GUC, an
RLS context — and those would provision the connection first, at which point SQLAlchemy
would silently ignore the level rather than raise.

Tests: the integration suite now runs all three methods against PostgreSQL and reads
the level back, and covers commit and rollback with a level set. Against the old
uow/sqlalchemy.py: 17 tests fail, including all 7 new integration tests.

4. String advisory-lock keys did not lock across processes

try_advisory_xact_lock hashed a str with the built-in hash(), which is salted per
interpreter:

$ for i in 1 2 3; do python -c "from ...locks import _to_signed64; print(_to_signed64(hash('cleanup_job')))"; done
-2214943151135031673
2447750913918232069
2993441328200896075

Two replicas of a service asking for the same named lock took different locks and both
proceeded — the exact case guide/advanced.md recommends string keys for.

Changed: BLAKE2b truncated to 64 bits. Integer keys are untouched.
PostgresAdvisoryLockMixin.try_advisory_lock now types key as str | int, which is
what the guides have always passed it. The SupportsAdvisoryLock protocol still says
int — widening it would break anyone who has implemented it with key: int.

Not breaking, but worth knowing: the integer a given string maps to changes with this
release. Nothing could have depended on the old value across processes, but during a
rolling deploy an old replica and a new one hold different locks for the same name until
the rollout finishes.

Tests: three fresh interpreters must agree on the key, plus the string path through
try_advisory_xact_lock. With hash() put back behind the same name, the determinism
test fails.

5. contrib.di and contrib.dependency_injector failed on import with AttributeError

Both packages carry a check that raises a message naming the extra to install, and it
never ran: scope = Scope.APP and class BaseDIContainer(containers.DeclarativeContainer)
are evaluated in class bodies, which happens before __init_subclass__.

$ clean/bin/python -c "import sqlalchemy_foundation_kit.contrib.di"
AttributeError: 'NoneType' object has no attribute 'APP'
$ clean/bin/python -c "import sqlalchemy_foundation_kit.contrib.dependency_injector"
AttributeError: 'NoneType' object has no attribute 'DeclarativeContainer'

Changed: the None placeholders are now objects that run the check on first attribute
access, so the import fails with the intended ImportError.

Tests: each package is imported in a subprocess with its dependency hidden by a
meta-path blocker — the failure is at import time, so patching a flag in an
already-imported module cannot reach it. Both fail against the old _deps.py.

6. Documentation

  • require_optional("orjson", "json") told the user to install [json]. There is no
    such extra; it is [orjson]. That one is a code fix — the message is what a user sees.
  • README section 4 imported the unit of work from unit_of_work_kit.
  • session_manager.close(), .close(timeout=...) and .healthcheck() appear on four
    pages. The only close is aclose(), the timeout is the manager's dispose_timeout,
    and there is no healthcheck method — the library ships the query
    (DEFAULT_HEALTHCHECK_QUERY) and leaves the policy to the caller, which is what the DI
    providers do. The pages now show that.
  • retry_async_connection was shown as a decorator on two pages, with RetryConfig
    fields (max_attempts, initial_delay, max_delay, exponential_base, jitter)
    that do not exist. It is a coroutine function taking connect_func, service_name and
    config; the fields are max_retries, retry_delay, max_backoff_delay.
  • PostgresMetrics was documented with service_name and labels arguments it does not
    take, and with metric names missing the db_ segment they all carry.
  • The environment variables in the configuration guide were one underscore short.
    BasePostgresConfig declares no model_config, so the names come from the
    BaseSettings that holds it and every level is a double underscore:
    POSTGRES__CONNECTION__HOST. Every name in the new block was run against
    pydantic-settings, including the MY_APP_ prefix variant and
    BasePostgresMigrationsConfig.
  • The API reference listed create_async_session_manager under session.builder; it
    lives in session.factories.

docs/agents.md

The page's "Broken at 0.2.0" section listed exactly findings 2, 3 and 5, and rules 1, 8
and 13 stated the buggy behaviour as rules. The section is now "Fixed since 0.2.0" —
same table, reframed, keeping the workarounds for anyone pinned to 0.2.0, since the fixes
are not in a release yet. Rules 1, 8 and 13, the get_transaction row, the isolation-level
note on the unit-of-work table, the advisory-lock paragraph, the errors table and the
string-key entry under "Common mistakes" all follow the code. The Version row no longer
claims 0.2.0 semantics for behaviour that is not 0.2.0's.

Verification

$ make check
uv run ruff check .          All checks passed!
uv run ruff format --check . 94 files already formatted
uv run mypy ...              Success: no issues found in 44 source files

$ make test-unit             535 passed, 47 deselected
$ make test-integration      47 passed, 535 deselected
$ make test                  582 passed — total coverage 93.48% (threshold 90%)
$ uv run --no-dev --group docs zensical build --clean
                             No issues found

uv sync --frozen --group dev --all-extras is clean. uv.lock changed by the two lines
finding 1 requires and nothing else.

Clean-install check after the fixes: pip install sqlalchemy-foundation-kit pulls
asyncpg, import sqlalchemy_foundation_kit works, and both contrib DI packages raise
their intended ImportError.

AsyncCConnection subclasses asyncpg.Connection at module scope and the package
__init__ imports it, so a clean "pip install sqlalchemy-foundation-kit" followed
by "import sqlalchemy_foundation_kit" raised ModuleNotFoundError. asyncpg was
only listed in the test group.

The library is asyncpg-only by design -- the DSN, the connection class and the
connect_args in create_async_session_manager are all asyncpg-specific -- so the
honest fix is to declare it rather than make the import optional.
AsyncSessionManager.get_transaction() passed execution_options= to the session
factory on every call, with or without an isolation level, and Session.__init__
has no such keyword -- so every call raised TypeError before reaching the
database.

The isolation level now travels with the connection checkout instead:
session.begin() does not provision a connection, so the session.connection()
call inside the block is the one that checks it out, and it applies the level
before the connection begins its transaction.

The unit tests only asserted the shape of the call to a mocked sessionmaker,
which is why they agreed with the bug. One of them now builds a real session
over a real engine -- that path never reaches the database, but it does
construct the session, which is where the TypeError came from.
…saction

apply_isolation_level() awaited session.connection() -- which checks a
connection out and begins its transaction -- and only then set the level on it.
PostgreSQL cannot change the isolation level of a running transaction, so
SQLAlchemy raised InvalidRequestError. Every isolation_level= argument on
transaction(), managed_session() and query() was therefore unusable; only the
engine-level setting worked.

The level is now handed to the session.connection() call that checks the
connection out, which applies it and then begins. That call starts the session's
transaction, so transaction() and managed_session() join the transaction that is
already open instead of calling session.begin() on top of it -- begin() refuses
a second one.

Neither the unit tests (mocked sessionmaker) nor the integration tests (which
never passed an isolation level) covered this. The integration suite now runs
all three methods against PostgreSQL and reads back SHOW transaction_isolation,
and gains coverage for AsyncSessionManager.get_transaction(), which had none.
try_advisory_xact_lock() turned a str key into an integer with the built-in
hash(), which is salted per interpreter: three fresh processes produced three
different keys for the same string. Two replicas of a service asking for the
same named lock therefore took different locks and both proceeded -- the exact
case the documentation recommends string keys for.

BLAKE2b truncated to 64 bits replaces it, so a string is the same lock in every
process. Integer keys are untouched.

The key an existing process computes for a given string changes with this
release. Nothing could depend on the old value across processes, but during a
rolling deploy an old replica and a new one hold different locks for the same
name until the rollout finishes.

PostgresAdvisoryLockMixin.try_advisory_lock now types key as str | int, which
is what the guides have always passed it.
Importing contrib.di without dishka failed with "AttributeError: 'NoneType'
object has no attribute 'APP'", and contrib.dependency_injector without its
package with the same error for 'DeclarativeContainer'. Both packages carry a
check that raises a message naming the extra to install, but it never ran: the
class bodies read Scope.APP and subclass containers.DeclarativeContainer, which
happens before __init_subclass__.

The None placeholders are now objects that run that check on first attribute
access, so the import fails with the message the code meant to give.

Covered by importing each package in a subprocess with its dependency hidden --
the failure is at import time, so patching a flag in an already-imported module
cannot reach it.
…nstall

require_optional("orjson", "json") told the user to run
"pip install 'sqlalchemy-foundation-kit[json]'". There is no [json] extra; it
is [orjson].
Everything here was checked against the source or run:

- README imported the unit of work from unit_of_work_kit, a package that does not
  exist here.
- session_manager.close(), .close(timeout=...) and .healthcheck() appear on four
  pages. The only close is aclose(), the timeout is the manager's dispose_timeout,
  and there is no healthcheck method -- the library ships the query
  (DEFAULT_HEALTHCHECK_QUERY) and leaves the policy to the caller.
- retry_async_connection was shown as a decorator on two pages, with RetryConfig
  fields (max_attempts, initial_delay, max_delay, exponential_base, jitter) that
  do not exist. It is a coroutine function taking connect_func, service_name and
  config, and the fields are max_retries, retry_delay, max_backoff_delay.
- PostgresMetrics was documented with service_name and labels arguments it does
  not take, and with metric names missing the db_ segment they all carry.
- The environment variables in the configuration guide were one underscore short:
  BasePostgresConfig declares no model_config, so the names come from the
  BaseSettings holding it and every level is a double underscore --
  POSTGRES__CONNECTION__HOST. Verified against pydantic-settings.
- The API reference listed create_async_session_manager under session.builder; it
  lives in session.factories.
- README and the home page said only sqlalchemy and pydantic are required.

docs/agents.md follows the fixes in this pull request: the section that listed the
three broken entry points now says they are fixed and keeps the workarounds for
anyone still on 0.2.0, and rules 1, 8 and 13 describe the current behaviour.
@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

@AlexeyShalaev
AlexeyShalaev merged commit 9548c72 into master Sep 6, 2026
7 checks passed
@AlexeyShalaev
AlexeyShalaev deleted the fix/agents-page-findings branch September 6, 2026 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant