test: the request commit check sees Core DML and row locks
A db.execute(update/delete) fires no flush and stages nothing, so a handler whose only write was Core DML could forget its commit and stay green. The check now asks Postgres whether the request opened a writing transaction. It found one real case: a repeated issue send returned with the session row still locked.
This commit is contained in:
@@ -220,6 +220,7 @@ def send_issue(sid: str, user: User = Depends(current_user), db: Session = Depen
|
|||||||
raise refused(409, "issue_sending")
|
raise refused(409, "issue_sending")
|
||||||
new = session.issue_url is None
|
new = session.issue_url is None
|
||||||
if not new and not debugsession.unfinished(session):
|
if not new and not debugsession.unfinished(session):
|
||||||
|
db.commit() # nothing to send: the lock ends here, not when the request's session closes
|
||||||
return {"ok": True, "session": _view(db, session)}
|
return {"ok": True, "session": _view(db, session)}
|
||||||
key = str(user.id)
|
key = str(user.id)
|
||||||
if new:
|
if new:
|
||||||
|
|||||||
@@ -105,6 +105,11 @@ def pytest_configure(config):
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def _write_xact(db) -> str | None:
|
||||||
|
"""The id of the open transaction if it has written anything, else None (a read never takes one)."""
|
||||||
|
return db.scalar(text("SELECT pg_current_xact_id_if_assigned()::text"))
|
||||||
|
|
||||||
|
|
||||||
def _request_dep(db, unfinished: list[str]):
|
def _request_dep(db, unfinished: list[str]):
|
||||||
"""`get_db` for a test request: the test's own session, plus the check a shared session hides.
|
"""`get_db` for a test request: the test's own session, plus the check a shared session hides.
|
||||||
|
|
||||||
@@ -124,6 +129,7 @@ def _request_dep(db, unfinished: list[str]):
|
|||||||
def dep(conn: HTTPConnection):
|
def dep(conn: HTTPConnection):
|
||||||
db.flush()
|
db.flush()
|
||||||
state = {"dirty": False}
|
state = {"dirty": False}
|
||||||
|
before = _write_xact(db)
|
||||||
|
|
||||||
def flushed(session, ctx):
|
def flushed(session, ctx):
|
||||||
state["dirty"] = True
|
state["dirty"] = True
|
||||||
@@ -138,7 +144,12 @@ def _request_dep(db, unfinished: list[str]):
|
|||||||
try:
|
try:
|
||||||
yield db
|
yield db
|
||||||
staged = db.new or db.deleted or any(db.is_modified(o) for o in db.dirty)
|
staged = db.new or db.deleted or any(db.is_modified(o) for o in db.dirty)
|
||||||
if state["dirty"] or staged:
|
# Core DML (`db.execute(update(...))`) fires no flush and stages nothing; the database
|
||||||
|
# itself says whether a write is open — a transaction that wrote and was not committed
|
||||||
|
# holds an id it did not hold when the request began (see `release_read_connection`). A
|
||||||
|
# row lock (`FOR UPDATE`) takes one too: the handler ends what it locked.
|
||||||
|
core_dml = (after := _write_xact(db)) is not None and after != before
|
||||||
|
if state["dirty"] or staged or core_dml:
|
||||||
unfinished.append(f"{conn.scope.get('method', 'WS')} {conn.url.path}")
|
unfinished.append(f"{conn.scope.get('method', 'WS')} {conn.url.path}")
|
||||||
except Exception:
|
except Exception:
|
||||||
db.rollback()
|
db.rollback()
|
||||||
|
|||||||
@@ -82,3 +82,34 @@ def test_a_request_that_raises_leaves_nothing_it_did_not_commit(db):
|
|||||||
request.throw(RuntimeError("the handler failed"))
|
request.throw(RuntimeError("the handler failed"))
|
||||||
assert db.scalar(select(User).where(User.email == "rolled-back-probe@example.com")) is None
|
assert db.scalar(select(User).where(User.email == "rolled-back-probe@example.com")) is None
|
||||||
assert db.info["unfinished"] == [] # a failing request is rolled back, not reported
|
assert db.info["unfinished"] == [] # a failing request is rolled back, not reported
|
||||||
|
|
||||||
|
|
||||||
|
def test_the_commit_check_sees_a_core_dml_write(db, make_user):
|
||||||
|
"""`db.execute(update(...))` flushes nothing and stages nothing, so the session's own bookkeeping
|
||||||
|
never sees it: a handler whose only write is Core DML and that forgot its commit passed. The
|
||||||
|
database's transaction id is what tells. A committed one must still pass."""
|
||||||
|
import pytest
|
||||||
|
from sqlalchemy import update
|
||||||
|
from starlette.requests import HTTPConnection
|
||||||
|
|
||||||
|
from app.models import User
|
||||||
|
|
||||||
|
user = make_user()
|
||||||
|
|
||||||
|
def drive(commit: bool) -> None:
|
||||||
|
conn = HTTPConnection(
|
||||||
|
{"type": "http", "method": "POST", "path": "/probe", "headers": [], "query_string": b""}
|
||||||
|
)
|
||||||
|
request = db.info["request"](conn)
|
||||||
|
session = next(request)
|
||||||
|
session.execute(update(User).where(User.id == user.id).values(display_name="dml"))
|
||||||
|
if commit:
|
||||||
|
session.commit()
|
||||||
|
with pytest.raises(StopIteration):
|
||||||
|
next(request)
|
||||||
|
|
||||||
|
drive(commit=True)
|
||||||
|
assert db.info["unfinished"] == []
|
||||||
|
drive(commit=False)
|
||||||
|
assert db.info["unfinished"] == ["POST /probe"]
|
||||||
|
db.info["unfinished"].clear() # recorded as it should be — not this test's failure
|
||||||
|
|||||||
Reference in New Issue
Block a user