fix(tests): give the metrics concurrency test real connections — 2.1-safe - #139
Merged
Merged
Conversation
…safe SQLAlchemy 2.1.0 (released 2026-09-24) turned test_increment_is_safe_under_concurrent_callers red on every branch — 50 concurrent increments landed at 9 to 48 (runs 36135730761, 36136050957). CI installs requirements.txt unpinned and picked 2.1.0 up by itself; the image installs requirements.lock, so production is still on 2.0.52. The defect was in the harness, not in app/core/metrics.py: the test ran its 50 sessions on the module's in-memory StaticPool engine, i.e. one shared connection and one shared transaction. Under 2.1's aiosqlite adapter a session's pool-return rollback can land between another session's UPDATE and COMMIT and throw it away; on 2.0.x the same harness ran all 50 writes in one transaction with a single commit, so it never exercised concurrent writers at all. Production never shares a connection between sessions. The test now gets its own engine — a file database in tmp_path with NullPool and a 30 s busy timeout — so each writer has a real connection and SQLite serialises them. Because increment() swallows DB errors, the test checks the metrics log first, so a lock timeout on a slow runner fails as itself, not as a lost increment. Checked with a scratch harness on the same setup: increment() reaches 50/50 in five trials, a naive read-then-write implementation 1 to 3 of 50, so the test still catches what it exists to catch. The reviewer's controlled A/B (only SQLAlchemy switched) showed 2.1.0 losing increments on StaticPool (6-15/50) and the new test passing 10/10 on 2.1.0. Rejected: capping sqlalchemy<2.1 — it would hide a harness artefact behind a version pin and need a Dependabot ignore rule; retrying or lowering N — it would weaken the lost-update guard. Full suite 1177 green (60 skipped); the test 10/10 locally; ruff + format clean. Reviewed by security-auditor (pass) and code-reviewer (approve). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Sep 25, 2026
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
1 participant
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.
What
tests/test_daily_metrics.py::test_increment_is_safe_under_concurrent_callersnow gets its own engine: a file SQLite database intmp_pathwithNullPooland a 30 s busy timeout. Each of the 50 writers therefore has a real connection. The test also checks the metrics log before counting, so a lock timeout fails with its own message and doesn't look like a lost increment.Why
SQLAlchemy 2.1.0 (released 2026-09-24) turned this test red on every branch. 50 concurrent increments landed at 9–48. Examples: #137 runs 36135730761 attempts 1 and 2, and
pr-dashboard-key-authrun 36136050957.requirements.txtunpinned, so it picked up 2.1.0 on its own.requirements.lockand is still on 2.0.52.The fault was in the test setup, not in
app/core/metrics.py.StaticPoolengine, so they shared one connection and one transaction.UPDATEandCOMMITand discard it.Verification
increment()reached 50/50 every time.security-auditorpassed it andcode-reviewerapproved it. Both review notes are applied: the log check and precise docstrings.Rejected alternative
Capping
sqlalchemy<2.1. That would hide a flaw in the test behind a version pin, and it would need a Dependabot ignore rule.This PR also unblocks #137 and every other open PR, which currently fail on this test.
🤖 Generated with Claude Code