fix(test): rewrite test_do_cleanup_logs_errors to eliminate caplog flake

[cmeans-claude-dev]

Closes #374.

Summary

tests/test_store.py::test_do_cleanup_logs_errors was failing intermittently on CI across multiple Python versions. Three failures blocked PR #373 (R3 migration-safety) twice in a row today alone. The code under test (_do_cleanup) is fully synchronous — the flake was in the pytest log-capture path, not in production code.

Rewrote the test to use the more reliable pattern:

• caplog.set_level(...) at fixture scope instead of the caplog.at_level(...) context manager
• Inspect caplog.records directly (logger name + level + message substring) instead of caplog.text string concatenation
• Richer assertion message that lists all captured records on failure, so the next flake (if any) is self-diagnosing

Verified 10 / 10 consecutive local runs pass post-fix.

Scope

2 files changed, +31, -7 (verified via git diff --shortstat origin/main).

File	±
tests/test_store.py	+28, -7 (one test rewritten in place)
CHANGELOG.md	+3, -0 (new ### Fixed section under [Unreleased])

No source code / config / workflow changes. Test-only.

Why the old pattern was fragile

with (
    caplog.at_level(logging.ERROR, logger="mcp_awareness.postgres_store"),
    patch.object(store._pool, "connection", side_effect=Exception("test error")),
):
    store._do_cleanup()
assert "Cleanup failed: Exception: test error" in caplog.text

Two quirks stack:

1. caplog.at_level(...) context manager has known subtle issues across pytest versions where the level doesn't propagate through the logger hierarchy before the first emission. caplog.set_level(...) (function, fixture-scoped) is the pytest-recommended pattern.
2. caplog.text substring matching relies on the text buffer being flushed synchronously after each emission — which depends on the formatter and buffer semantics, and has been observed to race under certain pytest-asyncio interactions even when the target code is synchronous.

The store fixture also spawns a background cleanup daemon thread during PostgresStore.__init__, so there are logger state transitions happening in parallel with the main test. The combination made the flake non-deterministic.

The new pattern avoids both quirks: set_level is known-good, and caplog.records is the authoritative capture list that doesn't depend on formatter flush timing.

Risks and unknowns

• The assertion is now tighter — it requires a matching record with the right logger name + level + message substring, rather than a substring anywhere in formatted text. If a future _do_cleanup change emits the error on a different logger, this test would catch that too (a feature, not a bug).
• The old test was ~7 lines; new test is ~25 lines including docstring. Verbosity is intentional — self-documenting so the next person doesn't wonder why we're not using at_level.

References

• Closes: test: flake — test_do_cleanup_logs_errors caplog assertion intermittent on 3.14 #374
• Currently blocking: test(rls): R3 — migration-safety test (apply N-1, seed, apply head, verify) #373 (R3 — migration-safety test). Once this lands, R3 can rebase onto main and re-run CI cleanly.

QA

Prerequisites

Docker (testcontainers spins up Postgres). Same as any other pytest run in this repo.

Automated checks

• test (3.10–3.14) — runs the rewritten test on every matrix entry; load-bearing
• lint / typecheck / codecov/patch — clean
• docker-smoke — not triggered (no Dockerfile / pyproject / uv.lock touched)
• CodeQL — test-only change; clean scan expected

Manual tests

1. • Run the fixed test in isolation:

   python -m pytest tests/test_store.py::test_do_cleanup_logs_errors -v
   

   Expected: passes in ~3s.

2. • Stability check (optional but recommended): Run the fixed test 10+ times in a row:

   for i in \$(seq 1 10); do python -m pytest tests/test_store.py::test_do_cleanup_logs_errors --tb=line -q 2>&1 | tail -1; done
   

   Expected: 10/10 pass. (Verified 10/10 locally before pushing.) QA: 15/15 observed in current session.

3. • Regression check: Full suite still passes.

   python -m pytest tests/ -q
   

   Expected: 988 passed, 7 skipped (same as before this PR).

4. • Scope — git diff --stat origin/main shows exactly tests/test_store.py (+28, -7) and CHANGELOG.md (+3, -0). Nothing else.

Acceptance

• ✅ CI green on all 5 matrix entries (3.10-3.14)
• ✅ 10/10 local stability runs pass
• ✅ ruff check, ruff format --check clean
• ✅ Single-concern; test-only
• ✅ Assertion tightened (logger + level + message) not loosened

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[cmeans]

QA review — PR #375

Test-only flake fix. Straightforward rewrite — exactly what the PR body says it is.

Verification performed

Step	Result
Scope	✅ git diff --shortstat origin/main → 2 files, +31, -7: tests/test_store.py +28/-7, CHANGELOG.md +3/-0. Matches claim exactly.
Rewritten test runs cleanly	✅ pytest tests/test_store.py::test_do_cleanup_logs_errors -v → 1 passed in 2.15s.
Stability check	✅ 15/15 consecutive runs passed (1.79–2.90s each, median ~1.85s). No flake observed. PR body claimed 10/10; exceeded.
Full suite regression	✅ pytest → 988 passed, 7 skipped, 4 warnings in 60.88s. Same total as pre-PR state.
Skipped tests	✅ Same 7 pre-existing Ollama-dependent skips (tests/test_embeddings.py × 5, tests/test_server.py × 2); no new skips introduced.
CHANGELOG entry	✅ New ### Fixed section under [Unreleased] (correct category for a flake fix); accurately describes the failure pattern, the fix, references #374.
CI rollup	✅ test (3.10–3.14), lint, typecheck, codecov/patch, CodeQL, license/cla — all green. docker-smoke correctly skipped.

Assertion comparison (old vs. new)

Dimension	Old (caplog.text substring)	New (caplog.records filter)	Change
Logger match	Any logger (captured text is flat)	Exact: r.name == "mcp_awareness.postgres_store"	Tighter
Level match	Filtered by at_level but checked via text	Exact: r.levelno == logging.ERROR	Tighter
Message match	Single exact substring "Cleanup failed: Exception: test error"	Two substrings: "Cleanup failed" AND "test error" must both appear	Slightly looser on format, but both phrases still required
Failure message	assert ... in caplog.text — no captured context	Dumps all records on failure	Tighter (self-diagnosing)

Net: assertion is on balance tighter. The PR body's "tightened not loosened" claim holds — the minor format-order flexibility in the message check is offset by the exact logger + level requirement, which the old pattern did not enforce.

Findings

None.

What's good

• Single-concern diff; pure test-only change, zero production-code touch.
• Problem correctly diagnosed (pytest-capture path, not the _do_cleanup code under test) and the fix targets that root cause — not a retry-masking workaround.
• New pattern is the pytest-recommended one (caplog.set_level + caplog.records direct inspection); the docstring explains why for anyone reading it in the future.
• Richer assertion failure message means the next flake (if any) is self-diagnosing — captured records are dumped with (name, level, message) tuples.
• Assertion is tighter on logger name and level, which catches an unrelated logger emitting similar text (a regression the old pattern would silently allow).
• 15/15 stability runs (exceeded PR body's 10/10 claim).
• CHANGELOG entry is in the right section, accurately describes the flake's observed behavior on CI across multiple Python versions, and closes #374.

Verdict

Ready for QA Signoff — clean flake fix with tightened assertions. Unblocks #373 (R3 migration-safety) per the PR body. Awaiting maintainer QA Approved.

[cmeans]

QA audit — transitioning label to Ready for QA Signoff.

Flake fix verified in current session:

• Scope matches claim exactly: 2 files, +31/-7 (test_store.py +28/-7, CHANGELOG.md +3/-0)
• Rewritten test passes in isolation (~2s)
• Stability: 15/15 consecutive runs passed (exceeded PR body's 10/10 claim)
• Full suite: 988 passed, 7 skipped, 4 warnings in 60.88s — same total as pre-PR state, no new skips
• Assertion comparison: tighter on logger name (now exact match vs. any) and level (exact match vs. any); message check split into two substrings that must both appear — on balance tighter, matches PR body's "tightened not loosened" claim
• CHANGELOG: new ### Fixed section under [Unreleased], accurate, closes test: flake — test_do_cleanup_logs_errors caplog assertion intermittent on 3.14 #374
• CI fully green on all 3.10–3.14 matrix entries

Zero findings. Awaiting maintainer to apply QA Approved. Unblocks PR #373 (R3 migration-safety) per PR body.

[cmeans]

LGTM