fix(storage): address PR #798 round-4 review (NOSONAR syntax + pg_advisory_lock + engine dispose + nits)

Addresses all 8 items in the round-4 bot review plus 4 remaining
SonarQube OPEN issues that were silently broken by round 3's
malformed NOSONAR markers.

NOSONAR syntax fix (clears the remaining 4 OPEN SQ issues)
----------------------------------------------------------
Round 3 used ``# NOSONAR S<rule_key>`` form. SonarQube Python doesn't
recognize the rule-key suffix — it treats the whole thing as a
malformed suppression directive (S7632) AND lets the underlying rule
keep firing (S7503 on ``_Cursor.__aenter__/__aexit__``).

Switch every marker to bare ``# NOSONAR``, with the rationale moved
into a preceding comment block. Affected sites:
- storage.py: ``_Cursor.__aenter__``, ``_Cursor.__aexit__``
- config.py: ``get_database_ssl()`` ``return False`` + ``ssl.create_default_context()``
- test_storage_logging.py: ``SENTINEL_PASSWORD_FRAGMENT`` constant
- test_storage_postgres.py: three ``bob_pw_v1`` / ``bob_pw_v2`` / ``carol_pw`` literals

Bot 🔴#1 — defensive NOSONAR on get_database_ssl `return False`
--------------------------------------------------------------
Bot predicted S4830 fires on the operator-opt-out path. SQ output
shows it doesn't currently fire, but bare NOSONAR added defensively
with rationale comment.

Bot 🔴#2 — defensive NOSONAR on f-string SQL
--------------------------------------------
``update_oauth_session`` builds its SET clause via ``f"{', '.join(update_fields)}"``;
``get_audit_logs`` builds its WHERE clause via string concatenation.
Both are safe (the fragments only come from this function's own
branches, no user input), but the patterns trip taint analysers.
Annotated both with bare NOSONAR + safety comment explaining the
hardcoded-fragments invariant. Note: S2077 doesn't currently fire
on these; defensive.

Bot 🟡#3 — pg_advisory_lock for concurrent migrations
-----------------------------------------------------
Without coordination, two pods rolling-updating simultaneously can
both observe ``has_alembic=False`` and both try to apply migrations
from scratch — the second crashes with "relation already exists".

New ``_migration_lock()`` async context manager:
- On Postgres: ``SELECT pg_advisory_lock(:lock_id)`` on a fresh
  connection (separate from the engine pool so it survives the
  ``to_thread.run_sync`` worker), held across BOTH the schema-inspect
  AND the migration call. Without that span, two pods could each
  observe "no alembic_version" before either started migrating,
  defeating the lock.
- On SQLite: yields immediately (file-level locking serializes
  writes natively).

Lock ID derived from
``sha256(b"nextcloud-mcp-server:migrations")[:8]`` as a stable signed
int64 so we can't collide with other apps sharing the same Postgres.

Bot 🟡#4 — RefreshTokenStorage.close() + lifespan wiring
--------------------------------------------------------
New idempotent ``close()`` method calls ``await engine.dispose()``,
nulls the engine, resets ``_initialized``. Wired into both
``app_lifespan_basic`` (BasicAuth) and the OAuth lifespan teardown,
each wrapped in ``try/except Exception`` with ``logger.warning`` so a
buggy dispose can't block SIGTERM. Without this, pooled asyncpg
connections leak server-side slots until
``idle_in_transaction_session_timeout`` reaps them — with small pool
defaults and frequent k8s rolling restarts this can starve
``max_connections``.

Bot 🟢#5 — is_sqlite_url docstring on :memory:
----------------------------------------------
Updated docstring to note both file-backed and in-memory forms are
recognized; caller is responsible for ``:memory:`` magic.

Bot 🟢#6 — db_path via make_url(...).database
---------------------------------------------
Replaced ``database_url.split("///", 1)[1]`` hack with SQLAlchemy's
own URL parsing. Naturally handles in-memory (``.database is None``
→ falls back to ``""``). Same lazy-import pattern as the existing
``mask_db_password`` to avoid module-import-time cost.

Bot 🟢#7 — _to_sync_url unrecognized-driver guard
-------------------------------------------------
Pulled ``_KNOWN_ASYNC_DRIVERS = ("aiosqlite", "asyncpg")`` into a
module constant. When an unrecognized ``+<driver>`` token survives
the strip, emits ``logger.warning`` with the known-supported list.
Behavior unchanged for valid URLs.

Bot 🟢#8 — get_audit_logs SELECT * → explicit columns
-----------------------------------------------------
Replaced ``SELECT *`` with explicit column list. Future schema
additions stay out of the dict return.

New tests
---------
- ``test_close_disposes_engine``: pins the public contract — engine
  nulled, state reset, second call is a no-op.
- ``test_concurrent_initialize_serialized_by_advisory_lock``: spawns
  3 concurrent inits against a fresh schema; asserts no "relation
  already exists" and exactly one ``alembic_version`` row at the end.
  Without the lock, this reliably fails on the second concurrent
  task.

Docs
----
- ADR-026: new "Concurrent migrations across pods" subsection
  documents the advisory-lock approach + lock-ID derivation.

Verification
------------
- ``uv run pytest tests/unit/`` — 1025 passed.
- ``TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres`` — 9 passed (was 7).
- ``ruff check && ruff format --check && ty check`` — clean.

Expected post-push: SQ scan reports 0 OPEN issues (was 4).

Tracked on Astrolabe Cloud POC board, card #99.

---

_This PR was generated with the help of AI, and reviewed by a Human_

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Chris Coutinho
2026-05-17 09:25:42 +02:00
co-authored by Claude Opus 4.7
parent 51419329b0
commit d717c64750
7 changed files with 292 additions and 51 deletions
@@ -136,6 +136,30 @@ estimate worst-case Postgres connection count as
`N × (pool_size + max_overflow)` and stay comfortably below the
server's `max_connections`.
### Concurrent migrations across pods
When `replicas: N` rolling-update restarts, multiple pods race
`RefreshTokenStorage.initialize()` simultaneously. Alembic's
version-table UPDATE isn't write-locked across connections by
default; without coordination, two pods can both observe
"no `alembic_version` table" and both try to apply migrations from
scratch — the second one crashes with `relation … already exists`.
We serialize this with a session-level Postgres advisory lock
(`SELECT pg_advisory_lock(:lock_id)`) acquired in `_migration_lock()`
and held across BOTH the schema inspection and the migration call.
The lock ID is a stable 64-bit integer derived from
`sha256("nextcloud-mcp-server:migrations")[:8]` so we can't collide
with other apps that happen to share the same Postgres instance.
The second pod blocks at the advisory-lock call until the first pod
finishes; it then re-inspects the schema, sees the now-populated
`alembic_version` table, and takes the no-op upgrade fast path.
SQLite needs no equivalent: file-level locking serializes writes
natively, so the second process waits on the file lock and then
sees the migrated schema. Covered by
`tests/integration/test_storage_postgres.py::test_concurrent_initialize_serialized_by_advisory_lock`.
### TLS for the Postgres backend
Two settings mirror the existing `NEXTCLOUD_VERIFY_SSL` /