fix!: client leaks, log hygiene, CI gaps, upgraded dependencies and a Python 3.14 floor

[aditeyabaral]

📌 Description

Fixes four problems found while reviewing #155. All four predate that PR, and none of them fail a test today — which is why they survived.

#	Problem	Commit
1	httpx.AsyncClient leaked on every failed authentication	43ddb7d
2	Every /authenticate made two upstream CSRF fetches and discarded one	40b6107
3	Staging could not be redeployed by hand	f569aed
4	README typed /health's status as str; ruff targeted py314 against requires-python >=3.12	6204d6b
5	Nothing enforced a version bump, so releases were easy to forget	5e79b19
6	Client cleanup could mask the real error; shutdown leaked prefetched clients; a cold cache fetched under the lock	d4fe4e6
7	Every wrong password logged a full stack trace at ERROR	15458c3
8	Rejected prod deploy hooks reported success; all actions ran on deprecated Node 20	ebf70df
9	The validation handler logged submitted passwords in plaintext	1a39d82
10	Client cleanup was abandoned part-way if a second cancellation landed during it	00aa9ca
11	app/app.py:74-75 was the only uncovered code left in the project	ca68745
12	docker/login-action sat on v3 while everything around it moved; lint ran three identical jobs; three different ruff versions were in use at once	d10dcdb, 851dd89
13	httpx is abandoned at 0.28.1, and starlette 1.6.0 deprecates it	0614f2f
14	requires-python promised >=3.12 while the image shipped 3.14 only, so CI validated two interpreters nobody deploys	a31df98
15	The coverage gate never ran on a pull request	eb08fd5, 18c101a

1. Client leaked on failure paths — the only correctness bug here

authenticate() closed its client only as its last statement, so every raise before that stranded the client and its connection pool until GC. The common case is a wrong password, which is routine traffic rather than an edge case. Two more instances of the same bug were in the same file: _fetch_new_client_with_csrf_token() leaked when the CSRF meta tag was missing or the GET raised, and _prefetch_client_with_csrf_token() leaked the freshly fetched client if it never reached the cache.

_fetch_new_client_with_csrf_token deliberately uses except/raise rather than try/finally, because on success the client is the return value.

Six of the seven new tests fail against the previous code; the seventh pins the success path, which was already correct.

2. Two prefetches per request

Two independent prefetch paths both fired on every call — one in _get_client_with_csrf_token(), one queued by the endpoint via background_tasks. The second closed the client the first had just cached. Both were introduced together in 13c1475, so this has been the behaviour since async support landed.

Verified in a container built from this branch: one GET /Academy/ per request where there were previously two (2 at startup from lifespan + the refresh loop, then exactly 1 per request). CSRF_TOKEN_REFRESH_LOCK is also gone — lifespan() always bypassed it, so it never provided mutual exclusion; PESUAcademy._csrf_lock still guards every read and write of the (client, token) pair, and the prefetch still fetches outside that lock so a 10s upstream timeout can't block requests.

The prefetch task is now strongly referenced (a bare create_task can be GC'd mid-flight) and its exceptions are retrieved and logged instead of surfacing as "Task exception was never retrieved" at GC.

3. Manual staging deploys

Adding workflow_dispatch alone would have failed silently: on a dispatch every github.event.workflow_run.* expression is empty, so the old if evaluates false and the job always skips. The condition now branches on github.event_name, and the manual path additionally requires github.ref_name == 'dev' so a dispatch can't ship an arbitrary branch to staging.

🔄 Supersedes #150

This takes #150's genuinely working parts — the concurrency group (re-keyed; #150 keys it on head_branch, empty on a dispatch), the stale-deploy guard, and the git merge --abort removal in deploy-prod.yaml.

#150's auto-version-bump is deliberately dropped, because it cannot work as written. It git pushes bump commits to dev, but dev is protected (required_approving_review_count: 1, enforce_admins: false), so the github-actions[bot] push is rejected — and since the bump step is sequenced before the deploy step, staging would never deploy at all. The same applies to its sync-minor-to-dev job. Making it work would need an admin bypass credential in CI, which isn't a good trade for automatic version bumping. scripts/bump_version.py is not carried over.

Two bugs found in #150 while folding it in, both fixed here rather than inherited:

• Its stale-deploy guard is a no-op: a run: step ending in exit 0 succeeds and lets the deploy step run regardless, so it only skipped the bump lines that followed it in the same step. It's a step output consumed by if: here, so it actually skips.

Also fixed in the step being rewritten: curl -X POST without -f exits 0 on HTTP 4xx/5xx, so a rejected deploy hook reported "✅ deployment completed successfully". Now curl -fsS. The same latent false-green exists in the three curl calls in deploy-prod.yaml; left for a separate change.

5. Version bumps are now enforced rather than automated

Since #150's auto-bump can't work, this asks the author instead: version-check.yaml compares project.version against the base branch and fails the PR if it wasn't raised. Reading instead of writing sidesteps branch protection entirely, and it's more honest about semver — no script can tell a patch from a breaking change, but the person writing it can.

It also verifies uv.lock records the same version, since bumping pyproject.toml without re-running uv lock leaves the lockfile stale — a mistake easy to make by hand.

Deliberately not using a trigger-level paths: filter: a required status check whose workflow never runs stays pending forever and blocks the PR. The job always runs and decides inside itself.

Verified against all four outcomes — unchanged version fails, decreasing version fails, bumped-with-stale-lockfile fails, bumped-with-matching-lockfile passes.

Note

A workflow only reports until it is a required status check. version-bumped has since been
added to dev's branch protection, so it now blocks merges rather than merely reporting.

Version bumped (2602fdc, later raised to 4.1.0 — see the third round). The PR introducing the rule should be the first to follow it, not the first exempt from it.

🔁 Follow-up round

A self-review of the diff above turned up a failure mode this PR had itself introduced, which is fixed here along with the pre-existing issues found alongside it.

d4fe4e6 — client lifecycle. The new try/finally closed the client on every path, but if aclose() itself raised it replaced the body's exception, turning a routine 401 into a 500. All close sites now route through _close_client_quietly(), which logs instead of raising (CancelledError still propagates). Two more in the same area: close_client() left prefetch tasks running, so one could complete after the close and cache a client nobody would ever close — they are now cancelled and awaited first, and the close happens under _csrf_lock with both cache slots cleared. And _get_client_with_csrf_token no longer holds _csrf_lock across its cold-cache fetch, which used to queue every concurrent request behind a 10s upstream timeout for no benefit.

15458c3 — logging severity. Every wrong password wrote a full traceback at ERROR, which misrepresents a client mistake as a server fault, buries real errors, and pages anyone alerting on error rate. Severity now follows the status code: 4xx warns without exc_info, 5xx keeps the trace. Verified in a container — a wrong password and a malformed request each produce one WARNING line, and the whole log holds zero tracebacks where it previously held one per failed login. Also here: the unit TestClient fixture no longer makes a live network call per test (the integration one still does, deliberately).

ebf70df — CI. Both prod deploy-hook curls lacked -f, so a rejected hook reported "✅ deployment completed successfully". And every workflow ran on deprecated Node 20; bumped to actions/checkout@v5 / actions/setup-python@v6, confirmed by the warning count going 1 → 0 between runs.

After that round the suite stood at 99 tests, 99.43% coverage, app/pesu.py at 100%. One existing test was replaced rather than kept: test_prefetch_closes_new_client_when_caching_fails asserted that a broken cached client aborts a refresh, which is no longer true (and is better not being true) — it is replaced by a test for the broken-old-client path and one for the cancellation window it was really meant to protect.

1a39d82 — a plaintext password leak, self-inflicted and caught in review

Worth calling out plainly. 15458c3 made validation_exception_handler log exc.errors() so the failure was diagnosable. Each entry carries an "input" key, and for a missing required field pydantic sets it to the entire request body — so any request omitting username logged the caller's password:

Request data could not be validated: [{'type': 'missing', 'loc': ('body', 'username'),
  'msg': 'Field required', 'input': {'password': 'hunter2-actual-secret'}}]

Now only type, loc and msg are logged — all the diagnostic value, none of the submitted values. The response body was never affected (it is built from loc and msg only). There is a regression test that fails against the leaking version.

This is distinct from the username in failed-auth logs, which is deliberate and unchanged. Tracing a failed login back to an account is the point of that line; a password is not traceability data.

🔂 Third round

00aa9ca — the cancellation window this PR had left open. Every caller of
_close_client_quietly runs it from an except BaseException handler or a finally, which is
exactly where a second cancellation can land — a shutdown cancelling a task already unwinding
from its first one. A plain await client.aclose() there is abandoned part-way and the connection
pool is never released, so the leak this whole PR exists to fix came back under the one condition
the cleanup is most likely to run in. The close is now its own shielded task, held by a
module-level strong reference: it completes regardless, while CancelledError still propagates, so
cancellation semantics are unchanged. The new test drives the exact sequence — cancel while waiting
for the lock, then cancel again once the close is in flight — and fails against the previous
implementation, where the close never finished.

ca68745 — 100% coverage. The last uncovered lines were the branch reporting a refresh task
that fails its own cancellation. The test stubs the loop with one that turns CancelledError into a
RuntimeError, and asserts both that it is logged and that shutdown carries on past it to close the
client.

d10dcdb + 851dd89 — CI. docker/login-action v3 → v4, actions/checkout v5 → v7,
actions/setup-python v6 → v7. The earlier round bumped only what the Node 20 deprecation
warning named, which is why login-action was left behind and the others stopped one major short.

lint.yaml also loses its Python matrix. Ruff is a self-contained binary whose results depend on
its own version and on tool.ruff.target-version, not on the interpreter installed beside it,
so the 3.12, 3.13 and 3.14 legs were running byte-identical work three times. (pre-commit.yaml's
matrix went too — see the fourth round below.)

Underneath that: the repo had three ruff versions in play at once — an unpinned
pip install ruff in lint.yaml, v0.12.7 in .pre-commit-config.yaml, and >=0.15.14 in
pyproject.toml — so a ruff release adding a rule could fail a PR that changed nothing. The job now
runs the ruff from uv.lock, and pre-commit autoupdate brings the hook to the same version.

Note

Collapsing a matrix renames the reported check, which silently breaks branch protection: the old
context stays required, never reports, and blocks every PR. dev's required contexts have
already been patched to the 5 these workflows actually report — version-bumped,
pre-commit-checks, lint, build, validate-pr-origin — down from 9. #143 and #132 cannot
produce the renamed checks until they rebase onto post-merge dev, which they need to do anyway.

0614f2f — every dependency upgraded, and httpx migrated. uv lock --upgrade across the
board, with the pyproject.toml floors raised to what is actually locked so they stop understating
what the project needs. Notable: fastapi 0.136.1 → 0.141.1, starlette 1.0.1 → 1.6.0, uvicorn
0.47.0 → 0.52.4, pytest 9.0.3 → 9.1.1, ruff 0.15.14 → 0.16.7.

httpx is the one that is not a plain version bump. The distribution is frozen at 0.28.1; the
project continued under a new name at pydantic/httpx2, still
authored by Tom Christie, and starlette 1.6.0 imports httpx2 first and only falls back to httpx
with a StarletteDeprecationWarning. Staying put means staying on an abandoned distribution and
on the deprecated path through starlette's TestClient.

The API surface this project uses is unchanged — AsyncClient(follow_redirects, timeout), get,
post, aclose — so the migration is the import, the annotations and 29 test patch targets. No
application logic changed. Production picks up httpcore2 and truststore in place of httpcore;
httpx2-jsfetch resolves into the lockfile but is emscripten-only and never installs in the image.

Version bumped to 4.1.0 — minor rather than patch, because the runtime dependency set changes
even though no API does.

🔄 Fourth round — Python 3.14 only

requires-python said >=3.12 while the Dockerfile has always shipped python:3.14-alpine on
both stages. So CI spent three serial legs validating two interpreters that are never deployed,
and the project is not on PyPI, so nothing consumes it as a library with its own choice of
interpreter. It is a deployed service, and the standard for a service is to test what it ships.

The matrix also never produced a signal. Across every recorded failure of this workflow — 8,
going back to 2025 — the first leg failed and fail-fast cancelled the other two. Not once did a
leg fail while another passed. Combined with max-parallel: 1, the matrix only ever ran to
completion when it had nothing to report, and cancelled precisely the legs that could have revealed
a version-specific bug.

So the promise is narrowed to match reality (a31df98):

pyproject.toml	requires-python >=3.12 → >=3.14; ruff target-version back to py314
uv.lock	relocked at >=3.14
pre-commit.yaml	matrix and its now-meaningless max-parallel dropped; single 3.14 job
lint.yaml	sets up 3.14
README.md, CONTRIBUTING.md, PR template, bug-report template	every 3.12 reference → 3.14

target-version = "py314" reverses part of 6204d6b in this same PR, which lowered it to py312
to match the old floor — it is correct at py314 again now, and verified zero-churn (ruff check
and ruff format --check both clean).

uv rebuilt the local environment to 3.14.4 when the floor changed, so the suite genuinely ran
on the target interpreter rather than on 3.13: 101 passed, 100.00% coverage, all 12 pre-commit
hooks green. CI is down from 9 checks to 5.

Version stays 4.1.0. This project's version has tracked the HTTP API — 3.0.0 was camelCase keys,
4.0.0 was the KYCAS removal — and that is untouched here. This is a runtime-requirements change,
the same class as the httpx2 migration above. Flagged rather than assumed: raise it to 5.0.0 if you
would rather the dropped Python support read as major.

🔐 Fifth round — the gate that never ran, and secret blast radius

validate-pr-origin requires pull requests to come from a fork, and GitHub withholds secrets from
fork pull requests. So scripts/run_tests.py always took its no-secrets branch on a pull request —
and that branch passed no --cov-fail-under at all. Every pull request reported green having
enforced nothing about coverage and having skipped all 11 live tests. The gate and the live tests
only ever ran on a push to dev, after merge.

eb08fd5 + 18c101a — the gate applies on both paths. Measured before changing anything: the
no-secrets run is 90 passed / 11 deselected at 99.16%, so 95% holds with room to spare.

The first attempt at making the skip visible did not work, and the reason is worth recording:
pre-commit captures a hook's output and prints it only on failure, so the ::warning:: was
swallowed by the very mechanism it was meant to work around — confirmed against this PR's own run,
which created zero annotations. verbose: true on the hook fixes it, and as a side effect the
pytest summary and coverage total are now visible in CI at all, where they were previously hidden.
90 passed, 11 deselected no longer looks identical to a full 101-test run. Verified on this PR:
one warning: Live tests skipped annotation, and Total coverage: 99.16% in the log.

The live tests still do not run on a pull request, and that is now a deliberate choice. An
approval-gated pull_request_target job was built and then reverted in 247b803: it is the only
mechanism by which a fork pull request can see secrets, and it works by running untrusted code with
them. An approval gate reduces that to a judgement call a maintainer has to get right on every
push, for a narrow benefit — a broken live integration already fails the dev run, which blocks
the staging deploy before it reaches anything. CONTRIBUTING.md now says plainly that the live
tests are the contributor's job to run locally.

Also built in this round and then reverted in full at the maintainer's request: an
approval-gated pull_request_target job for the live tests (247b803), the staging and
production environments it depended on (956c8b8), and the per-job permissions declarations
that went with them (8c6df63). The deploy workflows now match dev exactly on permissions and
environments. What survives from this round is the coverage gate and the visibility fix above,
which stand on their own.

🧱 Type of Change

• 🐛 Bug fix – Non-breaking fix for a functional/logic error
• ✨ New feature – Adds functionality without breaking existing APIs
• ⚠️ Breaking change – Introduces backward-incompatible changes (API, schema, etc.)
  (No API or schema change. Breaking for a native install only: requires-python rises to
  >=3.14 and httpx becomes httpx2. The deployed image is unaffected — it already ran 3.14.)
• 📝 Documentation update – README, docstrings, OpenAPI tags, etc.
• 🧪 Test suite change – Adds/updates unit, functional, or integration tests
• ⚙️ CI/CD pipeline update – Modifies GitHub Actions, pre-commit, or Docker build
• 🐢 Performance improvement – Speeds up auth, scraping, or reduces I/O
• 🕵️ Debug/logging enhancement – Adds or improves logging/debug support

🧪 How Has This Been Tested?

• Unit Tests (tests/unit/)
• Functional Tests (tests/functional/)
• Integration Tests (tests/integration/)
• Manual Testing

⚙️ Test Configuration:

• OS: Linux
• Python: 3.14 locally and in the Docker image
• Docker build tested

• python scripts/run_tests.py with TEST_* populated: 101 passed (was 79), 100.00%
  coverage (was 99.38%) with no uncovered lines anywhere in app/. The 95% gate holds.
• The whole suite was re-run against the upgraded dependencies and against httpx2: 101 passed,
  100% coverage, ruff 0.16.7 clean. Warning count dropped 2 → 1 — starlette's httpx deprecation
  is gone, and the one left is an anyio alias inside starlette itself.
• pre-commit run --all-files: every hook passes. The ruff checks are validated against the version pinned in .pre-commit-config.yaml, which is now the same version pyproject.toml and the lint job resolve — py312 is zero-churn there too.
• Container built from this branch: one successful /authenticate → 200 with the full profile and exactly one unauthenticated CSRF prefetch; one wrong-password request → clean 401, one prefetch, no unclosed-client or unretrieved-task warnings.
• Each of the two behavioural fixes was verified to fail against the previous code, so the new tests are genuine regression pins rather than passing tautologies.

✅ Checklist

• My code follows the CONTRIBUTING.md guidelines
• I've performed a self-review of my changes
• I've added/updated necessary comments and docstrings
• I've updated relevant docs (README or endpoint docs)
• No new warnings introduced
• I've added tests for the fixed behaviour
• All tests pass locally (scripts/run_tests.py)
• I've run linting and formatting (pre-commit run --all-files)
• Docker image builds and runs correctly
• Changes are backwards compatible

🛠️ Affected API Behaviour

• app/app.py – Removed the endpoint's redundant CSRF prefetch and the app-level lock
• app/pesu.py – Client lifecycle and prefetch task management

🐳 DevOps & Config

• .github/workflows/deploy-staging.yaml – Manual dispatch, working stale-deploy guard
• .github/workflows/deploy-prod.yaml – Removed a stray git merge --abort
• pyproject.toml / uv.lock – ruff target-version aligned with requires-python; every
  dependency upgraded, floors raised to match the lockfile, and httpx migrated to httpx2
• .github/workflows/lint.yaml – one job instead of three identical ones, ruff taken from uv.lock
• .pre-commit-config.yaml – pre-commit autoupdate, which aligns the ruff hook with pyproject.toml

🧠 Additional Notes

Commits are deliberately kept one-per-concern so the correctness fix, the concurrency change, the
workflow change and the doc/config change can each be reverted independently.

One pre-existing issue is deliberately not touched, to keep this reviewable:

• The function-scoped TestClient fixture in tests/unit/test_app_unit.py enters the real
  lifespan per test, so each test makes a real network call. (The new
  test_authenticate_does_not_trigger_additional_prefetch mocks the singleton and makes none.)

🤖 Generated with Claude Code

https://claude.ai/code/session_01Coun9gitMaSfa58zivRJwQ