Backlogsm7kni.io

Projects / sf2loki / SFL-0027

Task · SFL-0027

tests: k8s lease hold/acquire error branches are untested - the renew-409 CAS surrender has no covering test

Status
To Do
Labels
followup, phase-1
Milestone
Test-coverage backlog
Updated
2026-08-14

Description

What

src/sf2loki/coordinate/k8s_lease.py sits at 80% statement coverage (210 statements, 42 missed) and the missed set is concentrated in the two error-handling loops that implement the HA contract. Reproduce with:

uv run --with pytest-cov pytest tests/coordinate/ --cov=sf2loki.coordinate.k8s_lease --cov-report=term-missing -q
# src/sf2loki/coordinate/k8s_lease.py  210  42  80%
# Missing: 53, 204, 309, 311-314, 330, 332-335, 341, 385-389, 411, 422, 445-446, 454-468, 481-482, 485-486, 489-500, 503-518

(454-518 is the lazily-imported _RealLeaseAdapter/_default_api_factory, unreachable without the sf2loki[k8s] extra installed — out of scope here.)

The uncovered behavioural branches:

Because 385-389 never execute under the suite, any regression confined to those lines cannot change a single test outcome. That is a coverage fact, not an estimate: line 384’s predicate is exercised only with a False result (by the 404 and 500 tests).

Why it matters

k8s_lease.py:384-389 is the guard against the stale-leader split-brain that the fencing work in #47 and the observedTime work in #51 exist to prevent. A regression that removes the _CONFLICT arm, or reorders it after the _NOT_FOUND/transient handling, makes a lost renew CAS fall through to 411 and be tolerated as a blip: _is_leader stays True for up to a full lease_duration after another replica has legitimately taken the lease. Both instances’ check_fence() then pass, and the stale leader’s checkpoint commits race the new leader’s — exactly the failure #47 was filed against. The suite stays green.

The inverse regression is equally invisible: if the tolerate arm at 411 were changed to surrender on the first non-409, non-404 error, every Kubernetes API hiccup would demote the leader, causing leadership flapping and a full pipeline stop/start cycle (on_lose → on_acquire) on each one. Nothing in the suite would fail.

The misleading docstring at test_k8s_lease.py:530 compounds both: a maintainer auditing the hold loop reads the test list, sees a 409-on-renew test, and concludes the branch is pinned.

Proposed approach

Add tests to tests/coordinate/test_k8s_lease.py using the existing FakeClock / FakeMonotonic / ScriptedSleep / FakeLeaseAdapter / FakeApiException / _coord helpers already in that file — no new fixtures needed. The first two recipes below were validated out-of-tree against the current code; both pass and remove 385-389 and 411 from the term-missing set.

  1. test_hold_surrenders_on_renewal_cas_conflict (covers 384-389). Acquire normally, then replace adapter.replace_lease with a counting stub that raises FakeApiException(status=409). Leave read_lease returning our own holder so the pre-renew check at k8s_lease.py:362 passes — this is the whole point of the test and what distinguishes it from test_hold_surrenders_when_taken_over. Set coord._sleep = ScriptedSleep() so stop never fires and the clock never advances, mirroring the “returns promptly or loops forever” invariant already used by test_hold_surrenders_immediately_when_lease_deleted (test_k8s_lease.py:572). Assert _hold returns and the stub was called exactly once (no retry). To make a regression fail loudly instead of hanging the suite, have the stub set the stop event on its second call and then assert the call count is 1.

  2. test_hold_tolerates_transient_error_below_duration (covers 411). Acquire normally, then wrap replace_lease so the first call raises FakeApiException(status=500) and later calls delegate to the original. Script the sleeps as ScriptedSleep([lambda: clock.advance(5), None, stop.set]) — 5s is well under lease_duration 30. Assert _hold returns only because stop fired, replace_lease was called at least twice, and the lease’s holder is still ours.

  3. test_acquire_retries_on_non_conflict_create_error (covers 311-314). Absent lease; create_lease raises FakeApiException(status=500) on the first call then delegates. Script sleep #0 as a no-op back-off. Assert _acquire returns non-None and the stored holder is ours. Mirror the structure of test_lost_create_race_backs_off (test_k8s_lease.py:433).

  4. test_acquire_retries_on_non_conflict_replace_error (covers 332-335). Seed a foreign holder, advance the monotonic clock past lease_duration so the lease is observed stale, and have replace_lease raise status 500 once then delegate. Mirror test_lost_replace_race_backs_off (test_k8s_lease.py:468).

  5. test_read_error_is_treated_as_absent_without_clobbering (covers 445-446). Make read_lease raise FakeApiException(status=500), assert _read() returns None, and assert that a subsequent _acquire against a lease object that does in fact exist does not end up with our holder written (the create_lease 409 back-off keeps it safe). This documents the current behaviour and blocks a #50-style regression.

Also fix the docstring at test_k8s_lease.py:530 to describe what that test actually verifies — the pre-renew re-read surrender path at k8s_lease.py:361-368 — and rename it accordingly (for example test_hold_surrenders_when_reread_shows_foreign_holder), so the two surrender paths are distinguishable by name.

Imported from GitHub issue #111 on 2026-08-14, when this repo migrated from GitHub Issues to Backlog.md. The original issue has been deleted; its verbatim body, labels and comments are preserved in archive/issues-dump.json (jq '.[] | select(.number == 111)' archive/issues-dump.json).

Filed from the 2026-07-30 full-repo audit (11 finder lanes + adversarial verification per finding).

Acceptance Criteria

Definition of Done

References

View the source file on GitHub