cleanup: remove dead guard on _CONFIG_RELOAD_LOCK, fix lazy lock init, and resolve double assert_safe_id

[SckyzO]

cleanup: remove dead guard on _CONFIG_RELOAD_LOCK, fix lazy lock init in PrometheusClient, and resolve double assert_safe_id + misplaced docstring in replace_rack_devices

Symptom

Three small code-hygiene issues identified during the architectural audit, bundled into one PR because each is independently a 1-3 line change and they share the same review surface (defensive coding / dead-code cleanup).

None of these are bugs in the sense of producing wrong output — they are copy-paste artifacts, dead guards, and fragile lazy-init patterns. Left alone, they obscure the readers' understanding of the actual control flow and would mask a real bug if introduced later.

Technical Analysis

AP-01 — replace_rack_devices: misplaced docstring + duplicated assert_safe_id

src/rackscope/api/routers/topology.py:855-857

async def replace_rack_devices(
    rack_id: str,
    payload: RackDevicesUpdate,
    app_config: Annotated[AppConfig, Depends(get_app_config)],
    catalog: Annotated[Catalog, Depends(get_catalog)],
    topology: Annotated[Topology, Depends(get_topology)],
):
    assert_safe_id(rack_id, "rack_id")     # ← duplicate call
    """Replace all devices in a rack."""    # ← misplaced: this string is no longer a docstring,
                                            #    it's an evaluated expression with no effect
    assert_safe_id(rack_id, "rack_id")     # ← original intended call
    # ... rest of the function

Symptoms of a copy-paste error during a previous refactor. Effects:

• The docstring is no longer attached to the function (Python parses it as a no-op string expression). Tools that read docstrings (Sphinx, FastAPI's OpenAPI generation, IDEs) see no docstring.
• The first assert_safe_id runs before the docstring slot, the second one after. Functionally, the validation runs twice — harmless but wasteful and confusing on read.

AP-03 — Dead guard on _CONFIG_RELOAD_LOCK

src/rackscope/api/app.py:84 and :104-105

# Line 84 — eager initialisation
_CONFIG_RELOAD_LOCK: asyncio.Lock = asyncio.Lock()  # eager init prevents lazy-init race

# Line 98-105 — apply_config()
global \
    TOPOLOGY, \
    CATALOG, \
    ...
    _CONFIG_RELOAD_LOCK
if _CONFIG_RELOAD_LOCK is None:        # ← always False
    _CONFIG_RELOAD_LOCK = asyncio.Lock()
async with _CONFIG_RELOAD_LOCK:
    ...

The type annotation is asyncio.Lock (not Optional[asyncio.Lock]) and the value is constructed eagerly at module load. The guard if _CONFIG_RELOAD_LOCK is None cannot evaluate to True. The global _CONFIG_RELOAD_LOCK declaration is similarly unnecessary because the value is never reassigned in the normal flow.

AP-04 — Lazy lock init in PrometheusClient

src/rackscope/telemetry/prometheus.py:42 (declaration) and :138, :153 (lazy creation)

class PrometheusClient:
    def __init__(self, ...):
        ...
        self._lock: Optional[asyncio.Lock] = None    # ← lazy init
        ...

    async def query(self, ...):
        if self._lock is None:
            self._lock = asyncio.Lock()              # ← race-prone
        async with self._lock:
            ...

The lazy-init pattern is fragile in asyncio: two coroutines that reach the is None check before either has assigned the new lock will each construct their own asyncio.Lock and lock different objects. The GIL protects the test+set at the bytecode level in CPython today, but the pattern violates asyncio guidance and the project's own convention (see AP-03 above, which acknowledges the race risk explicitly in the comment "eager init prevents lazy-init race").

Proposed Fix

Fix AP-01

 async def replace_rack_devices(
     rack_id: str,
     payload: RackDevicesUpdate,
     app_config: Annotated[AppConfig, Depends(get_app_config)],
     catalog: Annotated[Catalog, Depends(get_catalog)],
     topology: Annotated[Topology, Depends(get_topology)],
 ):
-    assert_safe_id(rack_id, "rack_id")
-    """Replace all devices in a rack."""
-    assert_safe_id(rack_id, "rack_id")
+    """Replace all devices in a rack."""
+    assert_safe_id(rack_id, "rack_id")
     # ... rest of the function unchanged

Docstring moved to the canonical first-statement slot. Duplicate assert_safe_id removed.

Fix AP-03

 async def apply_config(app_config: AppConfig) -> None:
     """..."""
     global \
         TOPOLOGY, \
         CATALOG, \
         CHECKS_LIBRARY, \
         METRICS_LIBRARY, \
         APP_CONFIG, \
         PLANNER, \
-        TARGETS_BY_CHECK, \
-        _CONFIG_RELOAD_LOCK
-    if _CONFIG_RELOAD_LOCK is None:
-        _CONFIG_RELOAD_LOCK = asyncio.Lock()
+        TARGETS_BY_CHECK
     async with _CONFIG_RELOAD_LOCK:
         await _do_apply_config(app_config)

Three lines deleted, _CONFIG_RELOAD_LOCK removed from the global declaration list (it is not reassigned in this function's normal flow).

Fix AP-04

 class PrometheusClient:
     def __init__(self, ...):
         ...
-        self._lock: Optional[asyncio.Lock] = None
+        self._lock: asyncio.Lock = asyncio.Lock()  # eager init prevents lazy-init race
         ...

     async def query(self, ...):
-        if self._lock is None:
-            self._lock = asyncio.Lock()
         async with self._lock:
             ...

Same eager-init pattern as _CONFIG_RELOAD_LOCK in app.py. Comment matches the existing convention.

Test Checklist

• Existing test suite passes unchanged (pytest tests/, make test)
• make lint returns 0 errors (ruff may catch the dead-code branches we removed; verify no new warnings)
• make typecheck returns 0 errors (mypy will flag the changed type annotation on self._lock)
• OpenAPI schema: replace_rack_devices now has the docstring attached. Check GET /api/openapi.json returns the docstring as the endpoint description (it did not before).
• Optional new test: introspect PrometheusClient()._lock immediately after construction → not None (proves the eager init). Same for _CONFIG_RELOAD_LOCK after module import.
• No new flake on the existing tests targeting concurrent PrometheusClient.query calls (if any) — confirm no implicit dependency on the lazy-init pattern existed.

Impact and Severity

• Audience affected: contributors and future readers. Users see no functional difference.
• Severity: low individually, medium in aggregate. Each item is a small papercut; together they signal that the code was not consistently reviewed for hygiene. Fixing them now is cheap and prevents the patterns from being copy-pasted into new code.
• Priority: Sprint 2 of the audit roadmap. Ships when convenient. Good "low-risk PR" for a new contributor to take on if onboarding is a goal.

Breaking Changes

None. All changes are internal hygiene. No API contract, no runtime behaviour change, no log output change.

The only observable difference is the replace_rack_devices endpoint now exposes its docstring via OpenAPI, which is an additive improvement.

Related

• Audit ref: AP-01, AP-03, AP-04 in AUDIT_ARCHITECTURAL.md
• AP-02 (silent except: pass in plugins hot-reload) is the fourth anti-pattern from the audit, already filed as Issue fix(plugins): log hot-reload failures instead of silencing them #32 — kept separate because it has a different review surface (response shape change + frontend impact).
• AP-05 was invalidated during audit verification (StadeToulousainOverlay is a documented easter egg, not vendor hardcoding).

Out of Scope

• Audit of other Optional[asyncio.Lock] = None patterns in the codebase — scope-limited to PrometheusClient here. A follow-up grep could surface more if any exist.
• Migration of any other global declarations in app.py — only the dead _CONFIG_RELOAD_LOCK entry is touched.
• Broader docstring audit on routers — out of scope; only the one with the misplaced docstring is fixed.