fix(drain-proof): bind acknowledgement coverage to session identity (#661)
Review 582 (REQUEST_CHANGES at95178349) found a residual fail-open of the same class the PR set out to close. Acknowledgement coverage was decided by comparing a count against a count: covers_live_sessions = live_count_known and acked_count >= sessions_live_other Nothing bound an acknowledgement to the identity of a session that actually owed one, so acknowledgements supplied for the requesting session and for a session that does not exist satisfied the obligations of two live sessions that never answered - minting a clean, correctly signed proof and an allow verdict from the restart gate. Coverage is now derived from authoritative impact-report evidence: - New `_required_ack_sessions()` derives the required session ids from the report itself, via `ack_state` keys and/or `affected_sessions` filtered on `live and not is_requester`. The requester is excluded only on explicit `is_requester` evidence, never inferred. - When both views are present they must name the same set, and the result is reconciled against `counts.sessions_live_other`. Missing, malformed, duplicated, contradictory, or unreconcilable identity evidence fails closed and outranks every permitting path, including the timeout policy. - Coverage requires every required id to carry an explicit acknowledgement token keyed by that id. Acknowledgements for the requester, for unknown ids, or for fabricated ids never increase coverage. - Caller-supplied acknowledgement cardinality is no longer proof of anything. Failure propagates unchanged through `acks_or_timeout` -> `proof.clean` -> `failed_checks` -> `gate_apply_restart` verdict `deny` / `allow=False`. The earlier missing-acknowledgement remediation is preserved in full: absent, None, non-mapping, empty, partial, stale, and unparseable acks still fail closed, `ack_timeout_policy_applied` stays strict `value is True`, and the legitimate zero-live-sessions and explicit-timeout paths still pass. Reviewer's reproduction, before and after this commit: sessions_live_other = 2 report ack_state = {'other-0': 'pending', 'other-1': 'pending'} supplied acks = {'req': 'ack', 'totally-bogus-session': 'ack'} before: acks_or_timeout = True | proof.clean = True | gate allow after: acks_or_timeout = False | proof.clean = False | gate deny Tests: 22 new cases in `AcknowledgementIdentityBindingTests` covering the reviewer's exact exploit, wrong-ids-with-sufficient-count, partial identity match, requester-only acks, fabricated ids, unproven per-session states, missing/malformed/contradictory identity evidence, count mismatch, and the preserved success paths. Verification: - `pytest tests/test_drain_proof.py` -> 61 passed, 56 subtests (baseline at95178349: 39 passed, 26 subtests) - Restart surface (6 modules) -> 154 passed, 68 subtests, exit 0 (baseline at95178349: 132 passed, 38 subtests) - Full `pytest tests/` -> 5177 passed vs baseline 5155 passed; the 23 failures are identical in both runs and pre-exist at95178349. Scope: drain_proof.py, tests/test_drain_proof.py. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01VRUZAf3Fr5n3kqhhiayN6C (cherry picked from commit 4193b63f415b066ee292386c2c89bc3d2651a0cc)
This commit is contained in:
@@ -592,5 +592,316 @@ class AcknowledgementFailClosedTests(unittest.TestCase):
|
||||
self.assertFalse(result.clean)
|
||||
|
||||
|
||||
def _identity_report(*, requester: str, others: tuple[str, ...]) -> dict:
|
||||
"""Report with explicitly named requester and other live sessions.
|
||||
|
||||
Unlike :func:`_drained_report_with_live_sessions`, the session ids are
|
||||
chosen by the caller so a test can supply acknowledgements for the *wrong*
|
||||
identities while keeping the count correct.
|
||||
"""
|
||||
|
||||
sessions = [
|
||||
{
|
||||
"session_id": requester,
|
||||
"role": "controller",
|
||||
"profile": "prgs-controller",
|
||||
"pid": _live_pid(),
|
||||
"status": "active",
|
||||
"last_heartbeat_at": NOW.isoformat(),
|
||||
}
|
||||
]
|
||||
for session_id in others:
|
||||
sessions.append(
|
||||
{
|
||||
"session_id": session_id,
|
||||
"role": "author",
|
||||
"profile": "prgs-author",
|
||||
"pid": _live_pid(),
|
||||
"status": "active",
|
||||
"last_heartbeat_at": NOW.isoformat(),
|
||||
}
|
||||
)
|
||||
report = rc.evaluate_restart_impact(
|
||||
{"sessions": sessions, "leases": [], "inventory_complete": True},
|
||||
now=NOW,
|
||||
requesting_session_id=requester,
|
||||
)
|
||||
return report.as_dict()
|
||||
|
||||
|
||||
class AcknowledgementIdentityBindingTests(unittest.TestCase):
|
||||
"""Acknowledgement coverage must be bound to session identity, not counted.
|
||||
|
||||
Regression cover for the second reviewed fail-open on PR #882 (review 582,
|
||||
blocker B1): coverage compared ``acked_count`` against
|
||||
``counts.sessions_live_other``, so acknowledgements supplied for the
|
||||
requesting session and for ids that do not exist satisfied the obligations
|
||||
of the live sessions that never answered. The required identities are
|
||||
carried by the report itself — ``ack_state`` keys and ``affected_sessions``
|
||||
filtered on ``live and not is_requester`` — and only an acknowledgement
|
||||
keyed by one of those ids may count for it.
|
||||
"""
|
||||
|
||||
def _state(self, **overrides) -> dict:
|
||||
state = _clean_drain_state()
|
||||
state.pop("acks", None)
|
||||
state["ack_timeout_policy_applied"] = False
|
||||
state.update(overrides)
|
||||
return state
|
||||
|
||||
def _acks_check(self, proof) -> dp.DrainCheck:
|
||||
return next(c for c in proof.checks if c.name == dp.CHECK_ACKS_OR_TIMEOUT)
|
||||
|
||||
def _build(self, report: dict, state: dict):
|
||||
return dp.build_drain_proof(
|
||||
impact_report=report, drain_state=state, now=NOW, secret=SECRET
|
||||
)
|
||||
|
||||
def assertAcksFailClosed(self, report: dict, state: dict) -> dp.DrainCheck:
|
||||
"""Failure must propagate through the check, the proof, and the gate."""
|
||||
|
||||
proof = self._build(report, state)
|
||||
check = self._acks_check(proof)
|
||||
self.assertFalse(check.passed)
|
||||
self.assertFalse(proof.clean)
|
||||
self.assertIn(dp.CHECK_ACKS_OR_TIMEOUT, proof.failed_checks)
|
||||
decision = dp.gate_apply_restart(
|
||||
proof=proof.as_dict(),
|
||||
now=NOW,
|
||||
secret=SECRET,
|
||||
expected_impact_fingerprint=dp.impact_fingerprint(report),
|
||||
)
|
||||
self.assertEqual(decision.verdict, dp.GATE_DENY)
|
||||
self.assertFalse(decision.allow)
|
||||
return check
|
||||
|
||||
# --- the reviewer's exact reproduction --------------------------------
|
||||
|
||||
def test_requester_plus_unknown_id_cannot_satisfy_two_live_sessions(self):
|
||||
"""Review 582 B1 verbatim: requester + a nonexistent session.
|
||||
|
||||
``sessions_live_other=2`` with ``ack_state`` naming ``other-0`` and
|
||||
``other-1``; the drain state supplies an acknowledgement from the
|
||||
requesting session itself and from a session that does not exist. The
|
||||
count matches, the identities do not.
|
||||
"""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0", "other-1"))
|
||||
self.assertEqual(report["counts"]["sessions_live_other"], 2)
|
||||
self.assertEqual(
|
||||
report["ack_state"], {"other-0": "pending", "other-1": "pending"}
|
||||
)
|
||||
state = self._state(acks={"req": "ack", "totally-bogus-session": "ack"})
|
||||
check = self.assertAcksFailClosed(report, state)
|
||||
self.assertIn("other-0", check.detail)
|
||||
self.assertIn("other-1", check.detail)
|
||||
self.assertIn("fail closed", check.detail)
|
||||
|
||||
# --- wrong / unknown / requester identities ---------------------------
|
||||
|
||||
def test_sufficient_count_of_wrong_ids_fails_closed(self):
|
||||
"""Right cardinality, wrong identities: two acks, neither required."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0", "other-1"))
|
||||
state = self._state(acks={"ghost-a": "ack", "ghost-b": "ack"})
|
||||
check = self.assertAcksFailClosed(report, state)
|
||||
self.assertIn("do not count", check.detail)
|
||||
|
||||
def test_more_acks_than_required_still_fails_on_wrong_ids(self):
|
||||
"""Coverage cannot be bought with volume: five acks, none required."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0", "other-1"))
|
||||
state = self._state(acks={f"ghost-{i}": "acknowledged" for i in range(5)})
|
||||
self.assertAcksFailClosed(report, state)
|
||||
|
||||
def test_partial_identity_match_fails_closed(self):
|
||||
"""One required id acknowledged, the rest padded with unknown ids."""
|
||||
|
||||
report = _identity_report(
|
||||
requester="req", others=("other-0", "other-1", "other-2")
|
||||
)
|
||||
state = self._state(
|
||||
acks={"other-0": "ack", "ghost-1": "ack", "ghost-2": "ack"}
|
||||
)
|
||||
check = self.assertAcksFailClosed(report, state)
|
||||
self.assertIn("other-1", check.detail)
|
||||
self.assertIn("other-2", check.detail)
|
||||
|
||||
def test_requester_ack_never_satisfies_another_sessions_obligation(self):
|
||||
"""The requester is excluded from the required set and stays excluded."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
requester_rows = [s for s in report["affected_sessions"] if s["is_requester"]]
|
||||
self.assertEqual([s["session_id"] for s in requester_rows], ["req"])
|
||||
self.assertNotIn("req", report["ack_state"])
|
||||
check = self.assertAcksFailClosed(report, self._state(acks={"req": "ack"}))
|
||||
self.assertIn("other-0", check.detail)
|
||||
|
||||
def test_fabricated_ids_do_not_count_toward_coverage(self):
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
for bogus in ("", " ", "other-0 extra", "OTHER-0", "other-01", "0"):
|
||||
with self.subTest(bogus=bogus):
|
||||
self.assertAcksFailClosed(report, self._state(acks={bogus: "ack"}))
|
||||
|
||||
# --- per-session state must be explicitly valid ------------------------
|
||||
|
||||
def test_unproven_per_session_states_fail_closed(self):
|
||||
"""A required id present but not explicitly acknowledged fails closed."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0", "other-1"))
|
||||
for value in ("pending", "stale", "unknown", "", None, True, 1, NOW):
|
||||
with self.subTest(value=value):
|
||||
self.assertAcksFailClosed(
|
||||
report,
|
||||
self._state(acks={"other-0": "ack", "other-1": value}),
|
||||
)
|
||||
|
||||
def test_report_ack_state_placeholder_is_never_read_as_an_ack(self):
|
||||
"""``ack_state`` values are the report's own placeholders, not evidence."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
report["ack_state"] = {"other-0": "ack"}
|
||||
self.assertAcksFailClosed(report, self._state())
|
||||
|
||||
# --- missing / malformed / contradictory identity evidence -------------
|
||||
|
||||
def test_missing_identity_evidence_fails_closed(self):
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
report.pop("ack_state", None)
|
||||
report.pop("affected_sessions", None)
|
||||
check = self.assertAcksFailClosed(report, self._state(acks={"other-0": "ack"}))
|
||||
self.assertIn("no session-identity evidence", check.detail)
|
||||
|
||||
def test_malformed_ack_state_fails_closed(self):
|
||||
for malformed in ([], "other-0", 7, None, ("other-0",)):
|
||||
with self.subTest(malformed=malformed):
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
report["ack_state"] = malformed
|
||||
self.assertAcksFailClosed(
|
||||
report, self._state(acks={"other-0": "ack"})
|
||||
)
|
||||
|
||||
def test_non_string_ack_state_key_fails_closed(self):
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
report["ack_state"] = {7: "pending"}
|
||||
self.assertAcksFailClosed(report, self._state(acks={"other-0": "ack"}))
|
||||
|
||||
def test_malformed_affected_sessions_fails_closed(self):
|
||||
for malformed in ("sessions", 7, {"session_id": "other-0"}, [None], [7]):
|
||||
with self.subTest(malformed=malformed):
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
report.pop("ack_state", None)
|
||||
report["affected_sessions"] = malformed
|
||||
self.assertAcksFailClosed(
|
||||
report, self._state(acks={"other-0": "ack"})
|
||||
)
|
||||
|
||||
def test_affected_sessions_without_explicit_booleans_fails_closed(self):
|
||||
"""``live``/``is_requester`` must be real booleans, never inferred."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
report.pop("ack_state", None)
|
||||
for row in report["affected_sessions"]:
|
||||
if row["session_id"] == "other-0":
|
||||
row["is_requester"] = "false"
|
||||
self.assertAcksFailClosed(report, self._state(acks={"other-0": "ack"}))
|
||||
|
||||
def test_affected_sessions_missing_live_flag_fails_closed(self):
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
report.pop("ack_state", None)
|
||||
for row in report["affected_sessions"]:
|
||||
row.pop("live", None)
|
||||
self.assertAcksFailClosed(report, self._state(acks={"other-0": "ack"}))
|
||||
|
||||
def test_contradictory_ack_state_and_affected_sessions_fails_closed(self):
|
||||
"""Both views present and disagreeing is unresolvable, not a tie-break."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0", "other-1"))
|
||||
report["ack_state"] = {"other-0": "pending", "other-9": "pending"}
|
||||
check = self.assertAcksFailClosed(
|
||||
report, self._state(acks={"other-0": "ack", "other-9": "ack"})
|
||||
)
|
||||
self.assertIn("contradicts itself", check.detail)
|
||||
|
||||
def test_identity_count_mismatch_fails_closed(self):
|
||||
"""Identity evidence that cannot be reconciled with the count denies."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0", "other-1"))
|
||||
report["counts"] = dict(report["counts"], sessions_live_other=1)
|
||||
check = self.assertAcksFailClosed(
|
||||
report, self._state(acks={"other-0": "ack", "other-1": "ack"})
|
||||
)
|
||||
self.assertIn("cannot be reconciled", check.detail)
|
||||
|
||||
def test_broken_identity_evidence_outranks_timeout_policy(self):
|
||||
"""The sanctioned timeout path cannot paper over an unreadable report."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
report["ack_state"] = "not-a-mapping"
|
||||
self.assertAcksFailClosed(report, self._state(ack_timeout_policy_applied=True))
|
||||
|
||||
# --- legitimate success is preserved -----------------------------------
|
||||
|
||||
def test_every_required_session_acknowledged_passes(self):
|
||||
report = _identity_report(
|
||||
requester="req", others=("other-0", "other-1", "other-2")
|
||||
)
|
||||
state = self._state(
|
||||
acks={
|
||||
"other-0": "ack",
|
||||
"other-1": "acked",
|
||||
"other-2": "acknowledged",
|
||||
}
|
||||
)
|
||||
proof = self._build(report, state)
|
||||
check = self._acks_check(proof)
|
||||
self.assertTrue(check.passed)
|
||||
self.assertTrue(proof.clean)
|
||||
self.assertEqual(proof.failed_checks, [])
|
||||
self.assertIn("acknowledged by identity", check.detail)
|
||||
decision = dp.gate_apply_restart(
|
||||
proof=proof.as_dict(),
|
||||
now=NOW,
|
||||
secret=SECRET,
|
||||
expected_impact_fingerprint=dp.impact_fingerprint(report),
|
||||
)
|
||||
self.assertEqual(decision.verdict, dp.GATE_ALLOW)
|
||||
self.assertTrue(decision.allow)
|
||||
|
||||
def test_required_session_ack_tolerates_surrounding_whitespace(self):
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
proof = self._build(report, self._state(acks={" other-0 ": " ACK "}))
|
||||
self.assertTrue(self._acks_check(proof).passed)
|
||||
self.assertTrue(proof.clean)
|
||||
|
||||
def test_no_other_live_sessions_still_passes_with_identity_evidence(self):
|
||||
report = _identity_report(requester="req", others=())
|
||||
self.assertEqual(report["counts"]["sessions_live_other"], 0)
|
||||
self.assertEqual(report["ack_state"], {})
|
||||
proof = self._build(report, self._state())
|
||||
check = self._acks_check(proof)
|
||||
self.assertTrue(check.passed)
|
||||
self.assertIn("sessions_live_other=0", check.detail)
|
||||
self.assertTrue(proof.clean)
|
||||
|
||||
def test_explicit_timeout_policy_retains_intended_behavior(self):
|
||||
"""Valid, correctly typed timeout evidence still permits the check."""
|
||||
|
||||
report = _identity_report(requester="req", others=("other-0", "other-1"))
|
||||
proof = self._build(report, self._state(ack_timeout_policy_applied=True))
|
||||
check = self._acks_check(proof)
|
||||
self.assertTrue(check.passed)
|
||||
self.assertIn("timeout policy", check.detail)
|
||||
self.assertTrue(proof.clean)
|
||||
|
||||
def test_timeout_policy_still_strictly_typed_under_identity_binding(self):
|
||||
report = _identity_report(requester="req", others=("other-0",))
|
||||
for value in (None, "true", "True", 1, [], {}):
|
||||
with self.subTest(value=value):
|
||||
self.assertAcksFailClosed(
|
||||
report, self._state(ack_timeout_policy_applied=value)
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user