Skip to content

Commit 151cf41

Browse files
wshallwshallclaude
andcommitted
fix(store): a missing VIEW DEFINITION grant read as a missing claim proc
ADR 0114 §4 has always specified the startup gate's probe (a) as "OBJECT_ID of both procs", but the implementation folded (a) into (b) and inferred absence from a NULL OBJECT_DEFINITION. MEASURED: a principal holding only EXECUTE on the proc gets a non-NULL OBJECT_ID and a NULL OBJECT_DEFINITION, and the compat probe still passes. So a deployed, working, correct procedure was reported as *missing* and the operator was sent to grant CREATE PROCEDURE — neither the cause nor the cure. WITH ENCRYPTION produces the identical NULL. This is not hypothetical: the sub-lever B design note in the same module explicitly serves a fleet whose principal can never hold CREATE PROCEDURE (DBA-provisioned procs + a least-privilege app principal), which is exactly the posture that hits it. The probe now returns OBJECT_ID beside the definition and the two conditions get separate reasons; the actual cure, GRANT VIEW DEFINITION, is named. Both still DEGRADE — the gate hashes the body and cannot pass on one it cannot read — so no accept/reject behaviour changed, only the diagnosis. The offline stub pins the probe SQL by exact match (a typo'd probe must fail loudly rather than silently match); that pin moved with the SQL and stayed exact, and now also asserts both placeholders bind the same object. New offline legs cover both arms and fail without the fix. A live leg pins the premise no stub can show — that OBJECT_ID and OBJECT_DEFINITION genuinely disagree on a real server — using WITH ENCRYPTION, which needs no security principal; the permission half stays deferred with AC-10's other permission scenarios. Also adds the store-side half of AC-7's degraded gauge: a claim_proc_status() accessor on the store protocol (None on every backend without the lever and when the flag is off, so "not requested" stays distinguishable from "requested and degraded"). The surfaces that read it land next. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent dc3a763 commit 151cf41

6 files changed

Lines changed: 193 additions & 13 deletions

File tree

messagefoundry/store/base.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
AlertInstance,
4949
CapturedResponse,
5050
ClaimedHeads,
51+
ClaimProcStatus,
5152
ConnectionEvent,
5253
ConnectionMetrics,
5354
DbStatus,
@@ -1294,6 +1295,17 @@ def pool_status(self) -> PoolStatus | None:
12941295
on SQLite (no pool)."""
12951296
...
12961297

1298+
def claim_proc_status(self) -> ClaimProcStatus | None:
1299+
"""The ADR 0114 sub-lever A stored-procedure-claim startup-gate verdict, or ``None`` when
1300+
this backend has no such lever (AC-6: SQL Server is the only one that reads its flag, whose
1301+
literal name this module therefore does not write) or that flag is off. AC-7's **degraded
1302+
gauge** — the surface an operator can actually see the degraded
1303+
state on (``/status``, ``/metrics``, the console store panel); before it existed the whole
1304+
signal was one WARNING at ``open()``. Synchronous + free (three attributes the gate recorded
1305+
once at open — no DB round-trip), read-only, and additive: the ``/status`` field defaults
1306+
``None``, so an older client deserializes it unchanged."""
1307+
...
1308+
12971309
async def integrity_check(self) -> tuple[bool, str]: ...
12981310

12991311
async def connection_metrics(

messagefoundry/store/postgres.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@
108108
AlertInstance,
109109
CapturedResponse,
110110
ClaimedHeads,
111+
ClaimProcStatus,
111112
ConnectionEvent,
112113
ConnectionMetrics,
113114
DbStatus,
@@ -1274,6 +1275,11 @@ def pool_status(self) -> PoolStatus | None:
12741275
acquire_wait=self._acquire_wait.summary(),
12751276
)
12761277

1278+
def claim_proc_status(self) -> ClaimProcStatus | None:
1279+
"""``None``: the ADR 0114 sub-lever A stored-procedure claim path is SQL-Server-only (AC-6 —
1280+
this backend never reads its flag), so there is no gate verdict to report here."""
1281+
return None
1282+
12771283
async def _fetchall(self, sql: str, *params: Any) -> list[Any]:
12781284
return list(await self._pool.fetch(sql, *params))
12791285

messagefoundry/store/sqlserver.py

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@
8686
AlertInstance,
8787
CapturedResponse,
8888
ClaimedHeads,
89+
ClaimProcStatus,
8990
ConnectionEvent,
9091
ConnectionMetrics,
9192
DbStatus,
@@ -1757,6 +1758,14 @@ async def _gate_claim_proc(self) -> None:
17571758
(c) compatibility_level >= 130 (OPENJSON). Any miss records the reason, logs a WARNING, and
17581759
leaves ``_claim_proc_effective`` False — the shipped batch runs; NEVER a lane outage.
17591760

1761+
(a) and (b) are probed in ONE statement that returns ``OBJECT_ID`` beside the definition,
1762+
because a NULL body has two very different causes: the proc is absent, or it is deployed
1763+
and this principal simply cannot READ it (no ``VIEW DEFINITION``, or ``WITH ENCRYPTION``).
1764+
Both degrade — the gate cannot hash a body it cannot see — but they need opposite remedies,
1765+
and the second is the exact posture the sub-lever B design note above anticipates — a fleet
1766+
whose DB principal can never hold CREATE PROCEDURE, i.e. DBA-provisioned procs plus a
1767+
least-privilege app principal.
1768+
17601769
The comparison is against the STORED forms, not against ``_claim_proc_body()`` directly:
17611770
the engine rewrites the ``CREATE OR ALTER`` head when it stores the module, so comparing
17621771
with the submitted text can never match (the defect that left this gate inert in every
@@ -1773,16 +1782,34 @@ async def _gate_claim_proc(self) -> None:
17731782
else:
17741783
expected = _claim_proc_shipped_hashes()
17751784
for proc_name in (_CLAIM_PROC_CID, _CLAIM_PROC_DST):
1785+
# OBJECT_ID rides along so a NULL body can be told apart from an ABSENT proc.
1786+
# MEASURED: a principal holding only EXECUTE on the proc gets a non-NULL
1787+
# OBJECT_ID and a NULL OBJECT_DEFINITION — the module is deployed and working,
1788+
# and the compat probe above still passes. Without the id, that reads as
1789+
# "missing" and sends the operator to grant CREATE PROCEDURE, which is not the
1790+
# problem and does not fix it. WITH ENCRYPTION produces the identical NULL.
17761791
row = await self._fetchone(
1777-
"SELECT OBJECT_DEFINITION(OBJECT_ID(?)) AS body", (f"dbo.{proc_name}",)
1792+
"SELECT OBJECT_ID(?) AS oid, OBJECT_DEFINITION(OBJECT_ID(?)) AS body",
1793+
(f"dbo.{proc_name}", f"dbo.{proc_name}"),
17781794
)
17791795
deployed = row["body"] if row else None
17801796
if not deployed:
1781-
reason = (
1782-
f"stored procedure dbo.{proc_name} is missing (guarded DDL skipped —"
1783-
" CREATE PROCEDURE / ALTER-on-schema denied, or a pre-2016-SP1"
1784-
" engine?)"
1785-
)
1797+
if (row["oid"] if row else None) is None:
1798+
reason = (
1799+
f"stored procedure dbo.{proc_name} is missing (guarded DDL skipped —"
1800+
" CREATE PROCEDURE / ALTER-on-schema denied, or a pre-2016-SP1"
1801+
" engine?)"
1802+
)
1803+
else:
1804+
reason = (
1805+
f"stored procedure dbo.{proc_name} is DEPLOYED but its definition is"
1806+
" unreadable (OBJECT_ID resolves, OBJECT_DEFINITION is NULL) — the"
1807+
" proc is not missing and CREATE PROCEDURE is not the fix. Either"
1808+
" this principal lacks VIEW DEFINITION on it (GRANT VIEW DEFINITION"
1809+
f" ON OBJECT::dbo.{proc_name} TO <the engine's principal>) or the"
1810+
" module was created WITH ENCRYPTION. The gate compares the body"
1811+
" hash, so it cannot pass on a body it cannot read"
1812+
)
17861813
break
17871814
got = hashlib.sha256(_normalize_tsql(deployed).encode()).hexdigest()
17881815
matched = expected[proc_name].get(got)
@@ -2908,6 +2935,22 @@ def pool_status(self) -> PoolStatus | None:
29082935
claim_pool=claim_pool,
29092936
)
29102937

2938+
def claim_proc_status(self) -> ClaimProcStatus | None:
2939+
"""The ADR 0114 sub-lever A startup-gate verdict — AC-7's **degraded gauge** (``/status``,
2940+
``/metrics``, the console's store panel).
2941+
2942+
``None`` when ``fifo_claim_proc`` is off, so "not requested" stays distinguishable from
2943+
"requested and degraded"; otherwise the gate's own recorded outcome. Synchronous and free —
2944+
it copies three attributes ``open()`` set once, no DB round-trip. Read-only: nothing here
2945+
feeds the claim path, and the accept/degrade decision is not re-evaluated."""
2946+
if not self._fifo_claim_proc:
2947+
return None
2948+
return ClaimProcStatus(
2949+
effective=self._claim_proc_effective,
2950+
degraded_reason=self._claim_proc_degraded_reason,
2951+
head_forms=dict(self._claim_proc_head_forms),
2952+
)
2953+
29112954
@asynccontextmanager
29122955
async def _cursor(self, conn: Any) -> AsyncIterator[Any]:
29132956
"""Yield a cursor that is ALWAYS closed before its connection returns to the pool (EF-6).

messagefoundry/store/store.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -651,6 +651,31 @@ class ConnectionMetrics:
651651
destinations: dict[tuple[str, str], DestinationMetrics] # by (channel_id, destination_name)
652652

653653

654+
@dataclass(frozen=True)
655+
class ClaimProcStatus:
656+
"""The ADR 0114 sub-lever A stored-procedure-claim startup-gate verdict — AC-7's **degraded
657+
gauge**, as an operator-readable snapshot.
658+
659+
``None`` from :meth:`~messagefoundry.store.base.QueueStore.claim_proc_status` on any backend
660+
without the lever and on SQL Server when its flag is off, so "not requested" is a distinct
661+
state from "requested and degraded" rather than an indistinguishable ``False``. (The flag's
662+
name is deliberately not written in this module — AC-6's sentinel proves the lever is a no-op
663+
here by the absence of that literal.)
664+
665+
AC-7's compensating-control story assumes an operator can SEE the degraded state. Until this
666+
existed the whole signal was one WARNING at ``open()``, in a log nobody was watching — which is
667+
a load-bearing part of why the gate could degrade on every open, in every deployment, for the
668+
entire life of the feature without anyone noticing.
669+
"""
670+
671+
effective: bool # the gate passed and pooled claims run through the procs
672+
degraded_reason: str | None # why it fell back to the shipped batch; None when effective
673+
# proc name -> which stored head form the deployed module matched ("rewritten" | "verbatim").
674+
# Populated only when effective. "verbatim" means this engine does NOT rewrite CREATE OR ALTER —
675+
# no engine measured to date does, so it is worth reporting (not a fault).
676+
head_forms: Mapping[str, str] = field(default_factory=dict)
677+
678+
654679
@dataclass(frozen=True)
655680
class MessageSearchResult:
656681
"""The outcome of a scan-and-decrypt content search (ADR 0046 #51). ``rows`` are matched message
@@ -8251,6 +8276,11 @@ def pool_status(self) -> PoolStatus | None:
82518276
measures does not exist on this backend."""
82528277
return None
82538278

8279+
def claim_proc_status(self) -> ClaimProcStatus | None:
8280+
"""``None``: the ADR 0114 sub-lever A stored-procedure claim path is SQL-Server-only (AC-6 —
8281+
no other backend so much as reads its flag), so there is no gate verdict to report here."""
8282+
return None
8283+
82548284
async def integrity_check(self) -> tuple[bool, str]:
82558285
"""Run ``PRAGMA quick_check`` (can be slow on a large DB — call on demand only). Runs on a
82568286
pooled read-only connection so a long check never blocks the writer (lockfree-reads)."""

tests/test_adr0114_claim_proc.py

Lines changed: 69 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,8 @@ def _gate_rows(
295295
compat: int = 150,
296296
cid_body: str | None = "STORED",
297297
dst_body: str | None = "STORED",
298+
cid_oid: int | None = None,
299+
dst_oid: int | None = None,
298300
) -> dict[str, dict[str, Any] | None]:
299301
"""Build the _fetchone stub's answers.
300302
@@ -307,6 +309,12 @@ def _gate_rows(
307309
identity function. Both sides of the gate's comparison were then the same function of the same
308310
argument, so the suite could not distinguish a working gate from a broken one — the defect that
309311
let ADR 0114 sub-lever A ship inert. Do not restore it.
312+
313+
``*_oid`` is the ``OBJECT_ID`` the same probe returns. It DEFAULTS to "present iff the body is",
314+
which is the server's behaviour for the two ordinary cases (deployed-and-readable, absent) and
315+
keeps ``cid_body=None`` meaning "genuinely missing". Pass an id WITH a ``None`` body to model
316+
the third case a real server produces: deployed, but its definition unreadable by this
317+
principal (no VIEW DEFINITION, or WITH ENCRYPTION).
310318
"""
311319

312320
def resolve(value: str | None, proc: str, col: str) -> str | None:
@@ -316,14 +324,18 @@ def resolve(value: str | None, proc: str, col: str) -> str | None:
316324
return _as_object_definition(ss._claim_proc_body(proc, col))
317325
return value
318326

327+
def answer(value: str | None, oid: int | None, proc: str, col: str) -> dict[str, Any]:
328+
body = resolve(value, proc, col)
329+
return {"oid": oid if oid is not None else (917578307 if body else None), "body": body}
330+
319331
return {
320332
"compat": {"compatibility_level": compat},
321-
"dbo.mefor_claim_fifo_heads_cid_v1": {
322-
"body": resolve(cid_body, "mefor_claim_fifo_heads_cid_v1", "channel_id")
323-
},
324-
"dbo.mefor_claim_fifo_heads_dst_v1": {
325-
"body": resolve(dst_body, "mefor_claim_fifo_heads_dst_v1", "destination_name")
326-
},
333+
"dbo.mefor_claim_fifo_heads_cid_v1": answer(
334+
cid_body, cid_oid, "mefor_claim_fifo_heads_cid_v1", "channel_id"
335+
),
336+
"dbo.mefor_claim_fifo_heads_dst_v1": answer(
337+
dst_body, dst_oid, "mefor_claim_fifo_heads_dst_v1", "destination_name"
338+
),
327339
}
328340

329341

@@ -333,7 +345,12 @@ def _stub_fetchone(store: SqlServerStore, answers: dict[str, dict[str, Any] | No
333345
async def fake_fetchone(sql: str, params: tuple[Any, ...] = ()) -> dict[str, Any] | None:
334346
if sql == "SELECT compatibility_level FROM sys.databases WHERE name = DB_NAME()":
335347
return answers["compat"]
336-
assert sql == "SELECT OBJECT_DEFINITION(OBJECT_ID(?)) AS body", f"unexpected probe: {sql}"
348+
assert sql == "SELECT OBJECT_ID(?) AS oid, OBJECT_DEFINITION(OBJECT_ID(?)) AS body", (
349+
f"unexpected probe: {sql}"
350+
)
351+
# Both placeholders bind the SAME name — a probe that bound two different objects would
352+
# report one proc's id against another's body.
353+
assert params[0] == params[1], f"the probe must bind one object: {params!r}"
337354
return answers[params[0]]
338355

339356
store._fetchone = fake_fetchone # type: ignore[method-assign]
@@ -510,6 +527,51 @@ async def test_ac7_gate_degrades_loudly(
510527
assert ops[0][1].startswith("SET NOCOUNT ON;")
511528

512529

530+
@pytest.mark.parametrize(
531+
"answers_kw",
532+
[{"cid_body": None, "cid_oid": 917578307}, {"dst_body": None, "dst_oid": 917578307}],
533+
)
534+
async def test_ac7_gate_names_view_definition_when_the_proc_is_deployed_but_unreadable(
535+
answers_kw: dict[str, Any],
536+
monkeypatch: pytest.MonkeyPatch,
537+
caplog: pytest.LogCaptureFixture,
538+
) -> None:
539+
"""MEASURED: a principal holding only EXECUTE on the proc gets a non-NULL ``OBJECT_ID`` and a
540+
NULL ``OBJECT_DEFINITION`` — the module is deployed and working. Before the id rode along on
541+
the probe, that fired the MISSING arm and sent the operator to grant CREATE PROCEDURE, which is
542+
neither the cause nor the cure (the cure is GRANT VIEW DEFINITION; WITH ENCRYPTION produces the
543+
identical NULL). This is not a hypothetical posture: the sub-lever B design comment in the same
544+
module explicitly designs for a fleet whose principal can never hold CREATE PROCEDURE.
545+
546+
RED without the fix — the old arm keys on the body alone, so it cannot see the id at all."""
547+
with caplog.at_level(logging.WARNING, logger="messagefoundry.store.sqlserver"):
548+
store = await _gate(_gate_rows(**answers_kw), monkeypatch)
549+
assert store.claim_proc_effective is False
550+
reason = store.claim_proc_degraded_reason or ""
551+
assert "VIEW DEFINITION" in reason, "the reason must name the grant that actually fixes it"
552+
assert "WITH ENCRYPTION" in reason, "the other cause of the identical NULL"
553+
assert "is missing" not in reason, "a deployed proc must not be reported as absent"
554+
assert any("DEGRADED to the shipped ad-hoc batch" in r.getMessage() for r in caplog.records)
555+
# Still a degrade, not an outage: the claim runs on the shipped batch.
556+
ops, _, _ = await _drive_proc("ingress", ["lane-0"], store=store)
557+
assert ops[0][1].startswith("SET NOCOUNT ON;")
558+
559+
560+
async def test_ac7_gate_still_reports_a_genuinely_absent_proc_as_missing(
561+
monkeypatch: pytest.MonkeyPatch,
562+
) -> None:
563+
"""The other side of the split. OBJECT_ID NULL is a real absence and must keep pointing at the
564+
DDL/permission cause — the fix above must not relabel every NULL body as a readability problem.
565+
"""
566+
store = await _gate(
567+
_gate_rows(cid_body=None), monkeypatch
568+
) # oid defaults to None with the body
569+
reason = store.claim_proc_degraded_reason or ""
570+
assert "is missing" in reason
571+
assert "CREATE PROCEDURE" in reason
572+
assert "VIEW DEFINITION" not in reason
573+
574+
513575
async def test_ac7_no_error_2812_handling_on_the_hot_path() -> None:
514576
# The hot path carries no missing-proc (error 2812) handling: the gate decides at open, the
515577
# claim never falls back mid-flight.

tests/test_adr0114_claim_proc_live.py

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,33 @@ async def test_open_deploys_procs_and_gate_passes(proc_store: SqlServerStore) ->
8989
assert row is not None and row["body"], f"dbo.{proc} not deployed"
9090

9191

92+
async def test_a_deployed_proc_can_return_a_null_definition(proc_store: SqlServerStore) -> None:
93+
"""The premise the gate's missing-vs-unreadable split rests on, and the one thing no offline
94+
stub can show: on a real engine ``OBJECT_ID`` and ``OBJECT_DEFINITION`` genuinely disagree — a
95+
procedure can be DEPLOYED and its definition still come back NULL.
96+
97+
Before the gate probed the id it read that NULL as "the proc is missing" and sent the operator
98+
to grant ``CREATE PROCEDURE`` — neither the cause nor the cure.
99+
100+
``WITH ENCRYPTION`` is used because it needs no security principal, so this leg creates no login
101+
or user and impersonates nobody; it drops its own proc. The OTHER cause the reason string names
102+
— a principal with ``EXECUTE`` but no ``VIEW DEFINITION`` — produces the byte-identical NULL and
103+
is deferred with AC-10's other permission scenarios to a purpose-configured server.
104+
"""
105+
name = "mefor_gate_null_definition_probe"
106+
await proc_store._execute(f"CREATE PROCEDURE dbo.{name} WITH ENCRYPTION AS SELECT 1;") # noqa: S608
107+
try:
108+
probe = await proc_store._fetchone(
109+
"SELECT OBJECT_ID(?) AS oid, OBJECT_DEFINITION(OBJECT_ID(?)) AS body",
110+
(f"dbo.{name}", f"dbo.{name}"),
111+
)
112+
assert probe is not None
113+
assert probe["oid"] is not None, "the proc is deployed — this is the PRESENT half"
114+
assert probe["body"] is None, "and its definition is unreadable — the NULL half"
115+
finally:
116+
await proc_store._execute(f"DROP PROCEDURE IF EXISTS dbo.{name};") # noqa: S608
117+
118+
92119
async def test_ac8_trancount_on_exit_equals_entry(proc_store: SqlServerStore) -> None:
93120
# Execute the proc inside an open transaction and read @@TRANCOUNT before/after: the proc
94121
# must not BEGIN/COMMIT/ROLLBACK (it runs inside the client's autocommit=False txn).

0 commit comments

Comments
 (0)