fix(audit): break audit_log_path/_session_id deadlock
Pre-existing latent bug sinced9e32e5: ``audit_log_path()`` acquires the non-reentrant ``_LOCK`` and, while holding it, calls ``_session_id()`` which also takes ``_LOCK``. On a clean module state (both ``_LOG_PATH`` and ``_SESSION_ID`` unset) the first caller deadlocks. ``log_session_start`` triggers it in practice — it's the first GUI call after import and the ``log_file=str(audit_log_path())`` arg is evaluated before any ``log_event`` has had a chance to lazy-init the session id. Strong candidate contributor to the blank-pages symptom the kill switch was put back to mask: the writer thread (and any producer reaching ``audit_log_path``) would freeze forever, and Ctrl+C would not free the GIL — matches the launcher-can't-be-killed behaviour reported in1caedbb. Fix: resolve the session id BEFORE acquiring ``_LOCK`` in ``audit_log_path``. ``_session_id`` already double-checks under its own lock, so the call is safe and self-synchronising. Test fixture in ``tests/test_audit.py`` now bypasses the kill switch via ``monkeypatch.setattr(audit, "_DISABLED", False)`` — env vars are captured at import time and ``monkeypatch.setenv`` won't reach the module-level flag. With the fix in place, all 6 tests pass in 0.15s; without it, ``test_session_start_renders`` (and any test exercising the log_session_start path) hangs indefinitely. Kill switch behaviour is unchanged in production (`_DISABLED = True` in the shipped module); this is purely a correctness fix for the code path that gets exercised when the switch is off. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -26,9 +26,14 @@ import pytest
|
||||
@pytest.fixture
|
||||
def isolated_audit(monkeypatch, tmp_path):
|
||||
"""Redirect audit writes into ``tmp_path`` and reset module state
|
||||
so each test starts fresh."""
|
||||
so each test starts fresh.
|
||||
|
||||
The kill switch is bypassed for the duration of the test by
|
||||
patching the module-level constant directly — these tests need
|
||||
the real producer path to run."""
|
||||
monkeypatch.setenv("DATATOOLS_AUDIT_DIR", str(tmp_path))
|
||||
from src import audit
|
||||
monkeypatch.setattr(audit, "_DISABLED", False)
|
||||
audit.reset_for_tests()
|
||||
yield audit
|
||||
# Best-effort cleanup so a runaway writer thread doesn't keep
|
||||
@@ -115,6 +120,7 @@ class TestUnwritableTargetDoesntCrash:
|
||||
not_a_dir.write_text("hi")
|
||||
monkeypatch.setenv("DATATOOLS_AUDIT_DIR", str(not_a_dir))
|
||||
from src import audit
|
||||
monkeypatch.setattr(audit, "_DISABLED", False)
|
||||
audit.reset_for_tests()
|
||||
try:
|
||||
start = time.perf_counter()
|
||||
|
||||
Reference in New Issue
Block a user