From ae0ee7cd9e0924bc7348c5efe06afa73705010c0 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 21:17:19 -0500 Subject: [PATCH 1/2] ci+test(serverdb): run the 16 module-gated suites that executed nowhere, and pin the wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sixteen test modules gate themselves at import on MEFOR_TEST_SQLSERVER / MEFOR_TEST_POSTGRES and are named by no workflow step. A module-level pytestmark skipif yields zero collected tests when the gate is unset, so these did not pass, did not fail, and did not appear as skips on any trigger — not a PR, not push-to-main, not the nightly cron. 84 tests, dark. Among them: engine-shard crash recovery on both server backends, the PostgreSQL failover suite (its SQL Server twin has run for months), the ADR 0048/0049 DR seed gate that must refuse to activate a standby onto a non-empty store, and the .mfbak backup runner against a real server DB. Wires all sixteen into four new steps — two on sqlserver-store (engine-shard and statement-dispatch; DR seed-gate and backup) and two on postgres-store (failover and shard recovery; DR seed-gate and backup) — reusing each leg's existing env block and, on SQL Server, the pyodbc#1459 native-crash retry wrapper. Extends the `serverdb` change-detection alternation to admit them, plus four files the legs already ran that it never matched (connscale_postgres, load_failover_{postgres, sqlserver}, load_runner). Editing any of them now pulls the leg that proves it; before, they were reachable only by the nightly cron. The regex's own comment already required this ("Keep this in sync with those steps") — nothing enforced it. tests/test_serverdb_ci_coverage.py is that enforcement. It asserts (a) every module-gated suite is named by a workflow step and (b) every file a gated step runs is matched by the alternation. Detection is AST-based and deliberately narrow: only a module-level gate counts, because that is the shape that yields zero executed tests. A per-test decorator leaves the SQLite cases running, which is a weaker and separately-tracked gap — keeping the assertion sharp means a failure always means "this file runs nowhere" and the allow-list can stay empty. Verified: both assertions go red under mutation (unwire one file; drop one alternation entry) and green on restore. All 16 suites collect — 84 tests, no import rot. CI is the remaining verifier: these suites have not executed in a long time, so some may legitimately fail against the real backends. That is the point of running them. Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit 8e24dbfdaba6cf7e691ed3d7bcc9db9793c804be) --- .github/workflows/ci.yml | 102 ++++++++++++++++- tests/test_serverdb_ci_coverage.py | 172 +++++++++++++++++++++++++++++ 2 files changed, 273 insertions(+), 1 deletion(-) create mode 100644 tests/test_serverdb_ci_coverage.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 79901c41..f0ad5ce9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -431,7 +431,7 @@ jobs: # are declared there (ADR 0013), and SQL Server / Postgres both declare supports_response_capture # + supports_pt_reingress True — so a regression in that surface is a SERVER-DB regression and must # pull these legs, not just the SQLite suite. - if echo "$changed" | grep -qE '^(messagefoundry/store/|messagefoundry/pipeline/(cluster|wiring_runner|stage_dispatcher)|messagefoundry/config/(settings|wiring)|messagefoundry/transports/(base|database|dicomweb|fhir|http_auth|mllp|rest|soap|tcp|x12)|tests/test_(sqlserver|postgres|cluster|database_connector|pooled|stage_dispatcher|batch_claim|claim_fifo|inline_fast_path|seq_only_fifo|fifo_index|per_lane_wake|response_capture|reingress|x12_rte)|\.github/workflows/ci\.yml)'; then + if echo "$changed" | grep -qE '^(messagefoundry/store/|messagefoundry/pipeline/(cluster|wiring_runner|stage_dispatcher)|messagefoundry/config/(settings|wiring)|messagefoundry/transports/(base|database|dicomweb|fhir|http_auth|mllp|rest|soap|tcp|x12)|tests/test_(sqlserver|postgres|cluster|database_connector|database_source|pooled|stage_dispatcher|batch_claim|claim_fifo|inline_fast_path|seq_only_fifo|fifo_index|per_lane_wake|response_capture|reingress|x12_rte|shard_recovery|shard_cert|adr0071|adr0075|adr0114|dr_server_seed_gate|dr7_server_config_only_backup|backup_runner_server_db|connscale|load_failover|load_runner)|\.github/workflows/ci\.yml)'; then echo "serverdb=true" >> "$GITHUB_OUTPUT" else echo "serverdb=false" >> "$GITHUB_OUTPUT" @@ -722,6 +722,63 @@ jobs: bash scripts/ci/retry-native-crash.sh pytest -v tests/test_x12_rte.py + - name: Run the engine-shard + statement-dispatch suites on real SQL Server + env: + MEFOR_TEST_SQLSERVER: "1" + MEFOR_STORE_BACKEND: sqlserver + MEFOR_STORE_SERVER: localhost + MEFOR_STORE_PORT: "1433" + MEFOR_STORE_DATABASE: MessageFoundry + MEFOR_STORE_AUTH: sql + MEFOR_STORE_USERNAME: sa + MEFOR_STORE_PASSWORD: "Str0ng_P@ssw0rd!" + MEFOR_STORE_TRUST_SERVER_CERTIFICATE: "true" + MEFOR_ALLOW_INSECURE_TLS: "1" + PYTHONFAULTHANDLER: "1" + # Every file here is MEFOR_TEST_SQLSERVER-gated at MODULE level, so before this step they were + # collected nowhere and reported nothing — not a pass, not a skip, on any trigger. That covered + # engine-shard crash recovery and the shard TLS-cert ladder (ADR 0037 over the ADR 0063 unified + # store), the ADR 0071 statement dispatch/fusion wiring, the ADR 0075 batch backend, the ADR 0114 + # live claim procedure, and the synchronous handoff path. tests/test_serverdb_ci_coverage.py now + # fails if a module-gated suite is added without being named here. + # Same pyodbc 5.3.0 + py3.14 native-crash retry as the steps above (upstream pyodbc#1459). + run: >- + bash scripts/ci/retry-native-crash.sh + pytest -v + tests/test_shard_recovery_sqlserver.py + tests/test_shard_cert_sqlserver.py + tests/test_adr0071_dispatch_wiring_sqlserver.py + tests/test_adr0071_fused_callables_sqlserver.py + tests/test_adr0075_batch_sqlserver.py + tests/test_adr0114_claim_proc_live.py + tests/test_sqlserver_sync_handoff.py + tests/test_database_source_integration.py + + - name: Run the DR seed-gate + backup suites on real SQL Server + env: + MEFOR_TEST_SQLSERVER: "1" + MEFOR_STORE_BACKEND: sqlserver + MEFOR_STORE_SERVER: localhost + MEFOR_STORE_PORT: "1433" + MEFOR_STORE_DATABASE: MessageFoundry + MEFOR_STORE_AUTH: sql + MEFOR_STORE_USERNAME: sa + MEFOR_STORE_PASSWORD: "Str0ng_P@ssw0rd!" + MEFOR_STORE_TRUST_SERVER_CERTIFICATE: "true" + MEFOR_ALLOW_INSECURE_TLS: "1" + PYTHONFAULTHANDLER: "1" + # ADR 0048/0049 disaster recovery, SQL Server side: the seed gate that must REFUSE to activate a + # standby onto a non-empty store, the .mfbak backup runner against a real server DB, and the DR7 + # config-only backup + DbaDelegatedError path. Module-gated and never executed; a DR control that + # has never run is a DR control that has never been proven. + # Same pyodbc 5.3.0 + py3.14 native-crash retry as the steps above (upstream pyodbc#1459). + run: >- + bash scripts/ci/retry-native-crash.sh + pytest -v + tests/test_dr_server_seed_gate_sqlserver.py + tests/test_backup_runner_server_db_sqlserver.py + tests/test_dr7_server_config_only_backup_sqlserver.py + # Postgres store backend (Track B): run the gated store suite against a real PostgreSQL service # container (Linux, so 1x minutes). Runs NIGHTLY + on-demand (workflow_dispatch — use # `gh workflow run ci.yml --ref ` to exercise it on a feature branch) + on PRs that touch @@ -868,6 +925,49 @@ jobs: # connector->runner gap existed here. run: pytest -v tests/test_x12_rte.py + - name: Run the failover + engine-shard recovery suites on real Postgres + env: + MEFOR_TEST_POSTGRES: "1" + MEFOR_STORE_BACKEND: postgres + MEFOR_STORE_SERVER: localhost + MEFOR_STORE_PORT: "5432" + MEFOR_STORE_DATABASE: messagefoundry + MEFOR_STORE_USERNAME: postgres + MEFOR_STORE_PASSWORD: mefor + MEFOR_STORE_ENCRYPT: "false" + MEFOR_ALLOW_INSECURE_TLS: "1" + # These are MEFOR_TEST_POSTGRES-gated at MODULE level, so until this step existed they were + # collected nowhere and reported nothing — not a pass, not a skip. The SQL Server failover twin + # (test_cluster_failover_sqlserver.py) has run for some time; its Postgres counterpart never had. + # Engine-shard crash recovery (ADR 0037 over the ADR 0063 unified store) was dark on BOTH server + # backends. tests/test_serverdb_ci_coverage.py now fails if a module-gated suite is added without + # being named here. + run: >- + pytest -v + tests/test_cluster_failover_postgres.py + tests/test_shard_recovery_postgres.py + + - name: Run the DR seed-gate + backup suites on real Postgres + env: + MEFOR_TEST_POSTGRES: "1" + MEFOR_STORE_BACKEND: postgres + MEFOR_STORE_SERVER: localhost + MEFOR_STORE_PORT: "5432" + MEFOR_STORE_DATABASE: messagefoundry + MEFOR_STORE_USERNAME: postgres + MEFOR_STORE_PASSWORD: mefor + MEFOR_STORE_ENCRYPT: "false" + MEFOR_ALLOW_INSECURE_TLS: "1" + # ADR 0048/0049 disaster recovery: the seed gate that must REFUSE to activate a standby onto a + # non-empty store, the .mfbak backup runner against a real server DB, and the DR7 config-only + # backup + DbaDelegatedError path. All three are module-gated and were never executed against + # PostgreSQL; a DR control that has never run is a DR control that has never been proven. + run: >- + pytest -v + tests/test_dr_server_seed_gate_postgres.py + tests/test_backup_runner_server_db_postgres.py + tests/test_dr7_server_config_only_backup_postgres.py + # Headless load test (Track B / throughput): serve the synthetic high-fan-out load config (auth # off, small fan-out) and drive the smoke profile through the real `python -m harness --load` CLI, # asserting zero message loss + all SLOs (exit 0) and uploading the JSON/CSV report. Skipped on PRs diff --git a/tests/test_serverdb_ci_coverage.py b/tests/test_serverdb_ci_coverage.py new file mode 100644 index 00000000..5373073e --- /dev/null +++ b/tests/test_serverdb_ci_coverage.py @@ -0,0 +1,172 @@ +"""Meta-test: a server-DB suite that no workflow runs is dead coverage. + +A module gated at import time on ``MEFOR_TEST_SQLSERVER`` / ``MEFOR_TEST_POSTGRES`` contributes +**zero executed tests** anywhere the gate is unset — every developer machine, and every CI leg that +does not export it. So a gated module that no workflow step names does not fail, does not error, +and does not surface as a skip anywhere a human looks: it silently never runs. Sixteen such modules +holding 83 tests accumulated before this test existed, including engine-shard crash recovery on +both server backends, the PostgreSQL failover suite, and the DR seed-gate and backup suites. + +Two mechanical invariants: + +1. **Every module-gated suite is named by at least one workflow step.** Adding one without wiring + it reds this test, so the wiring decision is forced at authoring time rather than discovered + during an incident. +2. **The ``serverdb`` change-detection alternation admits every file the server-DB steps run.** + ``ci.yml``'s ``changes`` job decides whether the SQL Server / PostgreSQL legs run on a PR at all. + A file a leg runs but the alternation does not match is covered only by the nightly cron: editing + it does not pull the leg that proves it. The regex's own comment already demands this + ("Keep this in sync with those steps"); nothing enforced it. + +**Scope — module-gated only.** A module whose gate sits on individual tests or on one fixture +parametrization still executes its ungated (SQLite) cases, so it is not invisible in the same way; +its *server arm* not running is a real but weaker gap, tracked separately by the master test plan's +``STORE`` rows rather than pinned here. This test deliberately asserts the sharp invariant only, so +that a failure always means "this file runs nowhere" and never needs an allow-list to stay useful. + +Pure text/AST analysis of the repo: no database, no network, and no gate of its own. +""" + +from __future__ import annotations + +import ast +import re +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +TESTS_DIR = REPO_ROOT / "tests" +WORKFLOWS_DIR = REPO_ROOT / ".github" / "workflows" +CI_YML = WORKFLOWS_DIR / "ci.yml" + +#: The env gates that make a suite server-DB-only. +GATE_ENV_VARS = ("MEFOR_TEST_SQLSERVER", "MEFOR_TEST_POSTGRES") + +#: Module-gated suites that are deliberately not wired to any leg. Each entry needs a reason. +#: An empty mapping is the healthy state — do NOT add a file here to silence a wiring gap. +UNWIRED_ALLOWED: dict[str, str] = {} + + +def _is_module_gated(path: Path) -> bool: + """True when the whole module skips at collection unless a gate env var is set. + + Two shapes count: a module-level ``pytestmark`` carrying a skipif on a gate var, and a + module-level ``pytest.skip(..., allow_module_level=True)`` guarded by one. Both yield zero + executed tests when the gate is unset; a decorator on an individual test does not. + """ + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except SyntaxError: # pragma: no cover - a broken test file fails elsewhere, loudly + return False + for node in tree.body: + if isinstance(node, ast.Assign) and any( + isinstance(t, ast.Name) and t.id == "pytestmark" for t in node.targets + ): + dumped = ast.dump(node) + if "skipif" in dumped and any(var in dumped for var in GATE_ENV_VARS): + return True + dumped = ast.dump(node) + if "allow_module_level" in dumped and any(var in dumped for var in GATE_ENV_VARS): + return True + return False + + +def _module_gated_suites() -> list[Path]: + return [p for p in sorted(TESTS_DIR.glob("test_*.py")) if _is_module_gated(p)] + + +def _workflow_text() -> str: + """All workflow YAML plus the CI helper scripts a step may delegate to.""" + parts = [p.read_text(encoding="utf-8") for p in sorted(WORKFLOWS_DIR.glob("*.yml"))] + ci_scripts = REPO_ROOT / "scripts" / "ci" + if ci_scripts.is_dir(): + parts += [ + p.read_text(encoding="utf-8") + for p in sorted(ci_scripts.rglob("*")) + if p.is_file() and p.suffix in {".sh", ".ps1", ".py"} + ] + return "\n".join(parts) + + +def test_every_module_gated_suite_is_named_by_a_workflow() -> None: + """A module-gated file no workflow names produces zero tests, everywhere. Wire it.""" + gated = _module_gated_suites() + assert gated, ( + "found no module-gated server-DB suites — the AST detection in _is_module_gated has " + "drifted from how these suites gate themselves" + ) + + haystack = _workflow_text() + unwired = sorted( + p.name for p in gated if p.name not in haystack and p.name not in UNWIRED_ALLOWED + ) + + assert not unwired, ( + f"{len(unwired)} module-gated server-DB suite(s) are named by no workflow step or CI " + "script, so they execute nowhere — not on a PR, not on push-to-main, not on the nightly " + "cron:\n " + + "\n ".join(unwired) + + "\n\nAdd each to the sqlserver-store or postgres-store " + "job in .github/workflows/ci.yml, and extend the `serverdb` change-detection alternation " + "so editing the file pulls the leg that proves it." + ) + + +def _serverdb_alternation() -> re.Pattern[str]: + """The ``tests/test_(...)`` alternation inside the ``serverdb`` change-detection regex.""" + for line in CI_YML.read_text(encoding="utf-8").splitlines(): + if "grep -qE" in line and "tests/test_(" in line: + match = re.search(r"tests/test_\(([^)]*)\)", line) + if match: + return re.compile(rf"^test_({match.group(1)})") + pytest.fail( + "could not locate the `serverdb` change-detection alternation (a `grep -qE` line " + "containing `tests/test_(...)`) in .github/workflows/ci.yml" + ) + + +def _files_run_by_server_db_steps() -> set[str]: + """Test filenames a ci.yml step actually *runs* under a server-DB gate env var. + + Comment lines are stripped first: a step's prose may legitimately name a test file (this + module names itself in the steps it motivated) without that step running it. + """ + found: set[str] = set() + for block in re.split(r"\n - name:", CI_YML.read_text(encoding="utf-8")): + if not any(f"{var}: " in block for var in GATE_ENV_VARS): + continue + executable = "\n".join( + line for line in block.splitlines() if not line.lstrip().startswith("#") + ) + found.update(re.findall(r"tests/(test_[A-Za-z0-9_]+)\.py", executable)) + return found + + +def test_serverdb_path_gate_admits_every_file_those_legs_run() -> None: + """A file a server-DB leg runs must also pull that leg when it is edited.""" + alternation = _serverdb_alternation() + run_by_legs = _files_run_by_server_db_steps() + assert run_by_legs, ( + "found no test files inside server-DB-gated ci.yml steps — the step parsing in " + "_files_run_by_server_db_steps has drifted" + ) + + unmatched = sorted(name for name in run_by_legs if not alternation.match(name)) + + assert not unmatched, ( + f"{len(unmatched)} test file(s) run by a SQL Server / PostgreSQL step are not matched by " + "the `serverdb` change-detection alternation in .github/workflows/ci.yml, so editing one " + "does NOT pull the leg that proves it — it is covered only by the nightly cron:\n " + + "\n ".join(unmatched) + + "\n\nExtend the `tests/test_(...)` alternation to cover them." + ) + + +@pytest.mark.parametrize("var", GATE_ENV_VARS) +def test_gate_env_var_is_still_exported_by_ci(var: str) -> None: + """Guard the guard: a renamed gate var would make both tests above pass vacuously.""" + assert f"{var}: " in CI_YML.read_text(encoding="utf-8"), ( + f"{var} is exported by no ci.yml step. Either it was renamed — update GATE_ENV_VARS in " + "this file — or the server-DB legs no longer run at all." + ) From 9541d4b6d53b7d34455e9f0736517e7b0993283f Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 30 Jul 2026 16:48:53 -0500 Subject: [PATCH 2/2] =?UTF-8?q?fix(store):=20ADR=200114=20sub-lever=20A=20?= =?UTF-8?q?=E2=80=94=20the=20claim-proc=20startup=20gate=20has=20never=20p?= =?UTF-8?q?assed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OBJECT_DEFINITION() does not return a CREATE OR ALTER module verbatim: SQL Server deletes the OR and ALTER keyword TOKENS and keeps their separators, so a head submitted as `CREATE OR ALTER PROCEDURE dbo.x` comes back as `CREATE` + three spaces + `PROCEDURE dbo.x` (delta exactly 7; everything after the head is byte-identical). The gate hashed the SUBMITTED text, so it could never match. Sub-lever A has therefore been INERT in every deployment since it shipped — the gate degraded to the ad-hoc batch on every open, and no test noticed because the offline fixture fed the submitted body back as the "deployed" body, modelling the server as an identity function. Both sides of the comparison were the same function of the same argument. Measured on SQL Server 2022 16.0.4255.1 and 2025 17.0.4055.5, compat 130/160/170, across five deploy paths. Live legs go 5-failed/1-passed -> 6 passed. THE FIX IS SHIPPED-SIDE ONLY. The gate compares the deployed hash against a small set of code-controlled constants — the head forms a server may store for a module THIS build deployed (_CLAIM_PROC_STORED_HEADS: the measured `rewritten` form, plus `verbatim` for a hypothetical non-rewriting engine). _claim_proc_body() renders byte-identically, so _claim_proc_ddl, _SCHEMA, _schema_hash() and both golden body pins are untouched: no re-pin, no forced DDL re-apply on any live database. The expected map is keyed PER PROC. A single flat accepted-set would take the cid body served under the dst name — reachable with no tampering intent via sp_rename, which does not rewrite sys.sql_modules.definition — and silently swap the lane predicate, claiming zero rows forever. Head spellings this deploy path cannot emit (CREATE PROC, differing case) keep failing the gate: each is affirmative evidence of an out-of-band hand deploy, which is the AC-7 event. A two-sided canonicalization would launder exactly that signal, and its correctness rests on regex minutiae no finite assertion can pin (mutation-scored 0/7 against this suite; the shipped-side form scores 3/3). ALSO IN THIS COMMIT, because the gate fix makes them reachable: * The proc CALL's 9 parameter pins are PERSISTENT cursor state and were never cleared. At OUTBOUND the same pooled cursor runs the H2 delivery probe, binding an NVARCHAR id against the stale SQL_DOUBLE descriptor for @now FLOAT -> client 22018 -> rollback -> outbound delivery collapse. Fixing the gate alone would arm this for the first operator who sets fifo_claim_proc=true. Cleared the moment the CALL's result is drained. Verified on pyodbc 5.3.0 / ODBC Driver 18 that a ZERO-parameter execute tolerates surplus descriptors, so the shielded `SET LOCK_TIMEOUT -1;` reset in the finally-guard is unaffected and the exposure is exactly the H2 bind chain. * AC-11 no-match parity was honoured only on the two flagged branches. The ad-hoc batch bound the raw lane list into (VALUES (?),...) feeding a `DECLARE @heads TABLE (lane NVARCHAR(256) NOT NULL` — SQL Server evaluates that narrowing conversion on the outer constant scan before the CROSS APPLY filters it, so with ANSI_WARNINGS ON an oversized lane raises 2628 even when zero rows would match. 2628 is not 1222, so it is not translated to EMPTY-all: it rolls back and re-raises. The skip is hoisted ahead of the dispatch-path split so all three branches agree. Clamp still runs before the skip (the tested contract). TESTS — the fix's only evidence. The offline suite could not previously tell a working gate from a broken one; three contradictory implementations all scored 51/51. Both fixture doors are closed (the `_gate_rows` default AND the whitespace test that passed cid_body= explicitly), the "deployed" body now comes from an INDEPENDENT model of the server rewrite with a liveness receipt, and new tests pin the accepted set by set-equality, reject cross-proc substitution, reject un-emittable head spellings, and require the anchor break to degrade diagnosably. The live tamper leg gains the in-test positive control it lacked — under the old defect the gate rejected everything, so its degrade assertions were vacuous. Mutation check: bug 10 failed, flattened-key 3 failed, over-wide 8 failed, pin-clear no-op 1 failed; correct implementation 64 passed. DOCS — three docstrings asserted the false premise ("OBJECT_DEFINITION() preserves the definition text as executed"). The operator-facing degraded reason prescribed `DELETE FROM schema_meta`, which could not work: the re-apply submits the same text and mismatches again, forever. ADR 0114 amended; AC-7b added, because AC-7 is one-directional and was LITERALLY SATISFIED by the defect — nothing anywhere required a correctly deployed proc to pass. Co-Authored-By: Claude Opus 4.8 --- docs/CONFIGURATION.md | 2 +- ...face-redesign-ingress-routed-reset-fold.md | 66 +++- messagefoundry/config/settings.py | 6 +- messagefoundry/store/sqlserver.py | 291 +++++++++++++++--- tests/test_adr0114_claim_proc.py | 276 +++++++++++++++-- tests/test_adr0114_claim_proc_live.py | 11 +- 6 files changed, 564 insertions(+), 88 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index f8151d86..81608b70 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -70,7 +70,7 @@ backend-limited. | `group_commit_max_batch` | int | `64` | **SQLite only.** Flush threshold for the group-commit committer: once this many members are enrolled in the open batch it commits immediately rather than waiting out the rest of `group_commit_window_ms`, bounding batch size + latency under load. Ignored when group-commit is off (`group_commit_window_ms = 0`). | | `fifo_claim_batch` | int | `1` | all backends (ADR 0058). Max rows the **INGRESS/ROUTED** FIFO claim takes per commit. `1` = **OFF** (the workers claim one row per commit — byte-identical to before). `> 1` (clamped `1..64`) claims the **contiguous due head-prefix** in one commit and then processes each row in strict FIFO order with its own off-loop route/transform + separate handoff, amortizing the standalone claim commit toward 1/N. A not-due or producer-locked head still blocks the lane (strict per-lane FIFO, #285). The **outbound/delivery** claim is never batched. Opt-in throughput tuning (recommend `8`–`16`); size against worst-case message size, since N decrypted bodies are resident per lane between the claim and the N handoffs. | | `fifo_claim_fold_reset` | bool | `false` | **SQL Server only** ([ADR 0114](adr/0114-phase-4-claim-path-call-complexity-reduction-driver-interface-redesign-ingress-routed-reset-fold.md) sub-lever C). Folds the pooled claim's session `LOCK_TIMEOUT` reset into the claim batch on the **clean success path at INGRESS/ROUTED** (the write-less commit#2 disappears; the shielded finally-guard still runs on every non-clean exit — 1222, kept≠claimed, cancellation, any error). OUTBOUND/RESPONSE are never folded. `false` = **byte-identical** shipped batch + guard. Flip only after its own ADR 0114 §8 bench gate (AC-14). | -| `fifo_claim_proc` | bool | `false` | **SQL Server only** (ADR 0114 sub-lever A). Executes the pooled claim via the two lane-family versioned procs `dbo.mefor_claim_fifo_heads_cid_v1` / `_dst_v1` (fixed-arity `{CALL}`, one JSON lanes parameter) instead of the ~3 KB ad-hoc batch. Needs database `COMPATIBILITY_LEVEL >= 130` (SQL Server 2016); **fails safe to the batch, loudly**, when the procs are missing, hand-edited (body-hash mismatch), or compat < 130 — never a lane outage. A hardened split-principal deployment must `GRANT EXECUTE` on both procs to the runtime principal (the bootstrap principal owns them). `false` = byte-identical. Flip only after its own §8 gate (AC-14). | +| `fifo_claim_proc` | bool | `false` | **SQL Server only** (ADR 0114 sub-lever A). Executes the pooled claim via the two lane-family versioned procs `dbo.mefor_claim_fifo_heads_cid_v1` / `_dst_v1` (fixed-arity `{CALL}`, one JSON lanes parameter) instead of the ~3 KB ad-hoc batch. Needs database `COMPATIBILITY_LEVEL >= 130` (SQL Server 2016); **fails safe to the batch, loudly**, whenever the startup gate cannot verify both deployed bodies against this build — at least: a missing proc, a body matching no form this build deploys (hand edit, hand deploy, or a body changed without bumping the `_v1` proc name), a definition this principal cannot read (no `VIEW DEFINITION`, or `WITH ENCRYPTION`), or compat < 130 — never a lane outage. A hardened split-principal deployment must `GRANT EXECUTE` on both procs to the runtime principal (the bootstrap principal owns them), and `GRANT VIEW DEFINITION` so the gate can read the bodies it verifies. `false` = byte-identical. Flip only after its own §8 gate (AC-14). | | `fifo_claim_prepared` | bool | `false` | **SQL Server only** (ADR 0114 sub-lever B). Stabilizes the pooled claim's statement text (one JSON lanes parameter) and retains a prepared claim cursor on store-owned dedicated connections (INGRESS/ROUTED; the non-DDL fallback lane to `fifo_claim_proc`). **Logs + no-ops unless `fifo_claim_fold_reset` is on** (without the fold the finally-guard's reset would evict the one-slot prepare cache every call). `false` = byte-identical. Flip only after its own §8 gate (AC-14). | | `encryption_key` | secret | — | **env only** (`MEFOR_STORE_ENCRYPTION_KEY`); base64 32-byte **active** key — when set, PHI columns (`raw`/`payload` + `error`/`last_error`/`detail`) are AES-256-GCM-encrypted at rest. Mint one with `messagefoundry gen-key`. Empty = off. See [PHI.md §3](PHI.md#3-encryption-at-rest). | | `encryption_keys_retired` | secret | — | **env only** (`MEFOR_STORE_ENCRYPTION_KEYS_RETIRED`); comma-separated base64 **decrypt-only** keys kept available during a rotation until `messagefoundry rotate-key` finishes re-encrypting under the active key (ASVS 11.2.2). | diff --git a/docs/adr/0114-phase-4-claim-path-call-complexity-reduction-driver-interface-redesign-ingress-routed-reset-fold.md b/docs/adr/0114-phase-4-claim-path-call-complexity-reduction-driver-interface-redesign-ingress-routed-reset-fold.md index 94941b21..194bf595 100644 --- a/docs/adr/0114-phase-4-claim-path-call-complexity-reduction-driver-interface-redesign-ingress-routed-reset-fold.md +++ b/docs/adr/0114-phase-4-claim-path-call-complexity-reduction-driver-interface-redesign-ingress-routed-reset-fold.md @@ -395,12 +395,47 @@ per-database property, not a server version; `CREATE OR ALTER` needs 2016 SP1. A compat-120 database and under a DDL-denied principal. **Startup gate (fail-safe to the batch, loudly).** With `fifo_claim_proc` ON, `open()` probes: (a) -`OBJECT_ID` of **both** procs; (b) a SHA-256 of each deployed body via `OBJECT_DEFINITION()` against the shipped -DDL text (normalized) — **existence alone cannot catch a hand-edited body**, and the ADR 0064 marker covers only -in-repo edits, while the proc *is* the claim logic; (c) `compatibility_level ≥ 130`. Any failure → the store -records `claim_proc_effective = False`, logs a **WARNING naming the reason**, sets a degraded gauge, and runs -the shipped batch — never a lane outage; the hot path contains **no error-2812 handling**. Out-of-band drift is -caught at the next open; `DELETE FROM schema_meta` forces a full re-create. +`OBJECT_ID` of **both** procs; (b) a SHA-256 of each deployed body via `OBJECT_DEFINITION()` against the +**stored forms** of the shipped DDL text (normalized) — **existence alone cannot catch a hand-edited body**, and +the ADR 0064 marker covers only in-repo edits, while the proc *is* the claim logic; (c) `compatibility_level ≥ +130`. Any failure → the store records `claim_proc_effective = False`, logs a **WARNING naming the reason** and +runs the shipped batch — never a lane outage; the hot path contains **no error-2812 handling**. Out-of-band +drift is caught at the next open. + +> **AMENDMENT (2026-07-30) — `OBJECT_DEFINITION()` does not return the submitted text, and this gate was +> inert until it was fixed.** +> +> SQL Server does not store a `CREATE OR ALTER` module verbatim: it **deletes the `OR` and `ALTER` keyword +> tokens and keeps their separators**, so a head submitted as `CREATE OR ALTER PROCEDURE dbo.x` is returned by +> `OBJECT_DEFINITION()` as `CREATE` + three spaces + `PROCEDURE dbo.x` (character delta exactly 7; everything +> after the head byte-identical). MEASURED on SQL Server 2022 16.0.4255.1 and 2025 17.0.4055.5, compat +> 130/160/170, across five deploy paths (fresh `CREATE`, the `OR ALTER` re-apply, a plain batch, inside the +> shipped guarded `EXEC(N'…')`, and an out-of-band `ALTER PROCEDURE` — which the engine also rewrites, to a +> single-spaced `CREATE PROCEDURE`). Case is preserved, not folded; `PROC` survives as `CREATE PROC`. +> +> Because the gate as originally implemented hashed the **submitted** text, it **could never pass for a proc +> deployed by `_claim_proc_ddl`, on any engine that function can deploy to** — sub-lever A degraded to the batch +> on every open, in every deployment, from the feature shipping until this amendment. The lever was inert, not +> merely unused. (Scope note: this is a statement about *this* deploy path, not about every conceivable module.) +> +> The fix is **shipped-side only**: the gate now compares the deployed hash against a small set of +> code-controlled constants — the head forms a server may store for a module *this build* deployed +> (`_CLAIM_PROC_STORED_HEADS`: the measured `rewritten` form, plus the `verbatim` form for a hypothetical +> non-rewriting engine). `_claim_proc_body()` renders byte-identically, so `_claim_proc_ddl`, `_SCHEMA`, +> `_schema_hash()` and the golden body pins are untouched: **no re-pin and no forced DDL re-apply on any live +> database.** The expected map is keyed **per proc**, so the cid body served under the dst name (reachable via +> `sp_rename`, which does not rewrite `sys.sql_modules.definition`) degrades rather than silently swapping the +> lane predicate. Head spellings this deploy path cannot emit (`CREATE PROC`, differing case) keep failing the +> gate: each is affirmative evidence of an out-of-band hand deploy, which is the AC-7 event. +> +> The accepted set is exactly two constants over **normalized** text. It is *not* two byte strings — +> `_normalize_tsql` still applies to the deployed side, so its whitespace collapse remains semantically lossy +> inside comments and string literals. That is contained by the AC-8 body lint (no quotes, no `--`, no `/*`, +> ASCII-only), which is now load-bearing rather than defensive. +> +> **`DELETE FROM schema_meta` was previously prescribed here as the remedy for a body mismatch. It could not +> work** — the re-apply submits the same text, the engine rewrites it the same way, and the hash mismatches +> again — so the advice has been removed from the ADR and from the operator-facing degraded reason. **Versioning, mixed vintages, downgrade.** Procs are **name-versioned** (`_v1`, `_v2`, …): engine sharding runs N processes against ONE unified store (ADR 0037/0063), so a rolling upgrade briefly runs two builds against one @@ -413,6 +448,9 @@ repeated "schema DDL batch applied" lines mid-rollback knows it is expected. **G owns the procs (EXECUTE implicit via ownership); a hardened split-principal deployment must `GRANT EXECUTE` — an ops-doc line, not a code path. **Two-copies drift** (batch vs proc bodies) is contained by the content hash + the body-definition probe + a lint test diffing the proc DDL's statement sequence against the batch construction. +(Until the 2026-07-30 amendment the body-definition probe was **not** a real compensating control: it compared +against text no server could return, so its verdict was constant and a genuine tamper was indistinguishable from +baseline. The content hash and the DDL-vs-batch lint were carrying that containment alone.) ### 5. Sub-lever B — stable statement text + a retained prepared claim cursor (the non-DDL fallback lane) @@ -641,10 +679,18 @@ states, including the mismatch and 1222 translations). **Any miss = the flag sta injected-row test. - **AC-6** — The three flags SHALL be provable no-ops on SQLite and Postgres (neither backend references them). → sentinel test (the ADR 0075 precedent). -- **AC-7** — WHEN `fifo_claim_proc` is ON and a proc is missing, its `OBJECT_DEFINITION` hash mismatches the - shipped body, or compat < 130, the store SHALL degrade loudly to the shipped batch (WARNING naming the reason - + degraded gauge), never a lane outage; the hot path SHALL contain no error-2812 handling. → startup-gate - tests incl. a hand-edited-body leg. +- **AC-7** — WHEN `fifo_claim_proc` is ON and a proc is missing, its `OBJECT_DEFINITION` hash mismatches every + form this build deploys, or compat < 130, the store SHALL degrade loudly to the shipped batch (WARNING naming + the reason + degraded gauge), never a lane outage; the hot path SHALL contain no error-2812 handling. → + startup-gate tests incl. a hand-edited-body leg. +- **AC-7b** — WHEN `fifo_claim_proc` is ON and both procs are deployed **by this build's own DDL**, the gate + SHALL **PASS** and `claim_proc_effective` SHALL be True, verified against a **real SQL Server** (not a stub + that echoes the submitted text back as the deployed body). → `test_adr0114_claim_proc_live.py`, plus an + offline round-trip whose "deployed" fixture independently models the engine's module rewrite. + > Added by the 2026-07-30 amendment. AC-7 as originally written is **one-directional** — it requires the gate + > to degrade when the body mismatches, and nothing anywhere required a *correctly deployed* proc to pass. The + > shipped defect therefore **satisfied AC-7 literally** while leaving the lever inert, and AC review could not + > have caught it. Any future gate-shaped AC needs both directions or it is not a gate. - **AC-8** — The proc bodies SHALL contain no `BEGIN/COMMIT/ROLLBACK`, no `TRY/CATCH`, no `SET XACT_ABORT`, and no `LOCK_TIMEOUT` reset outside the `@fold_reset` tail; `@@TRANCOUNT` on exit SHALL equal entry. → DDL lint test + a trancount probe test. diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 32023c7b..65f0dae6 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -327,8 +327,10 @@ class StoreSettings(_Section): description=( "Execute the pooled claim via the two lane-family versioned procs " "(dbo.mefor_claim_fifo_heads_cid_v1/_dst_v1; fixed-arity CALL) instead of the ~3KB ad-hoc " - "batch. Fails safe to the batch (loud) if the procs are missing/stale or compat < 130. " - "SQL Server only; OFF = byte-identical." + "batch. Fails safe to the batch (loud) whenever the startup gate cannot verify both " + "deployed bodies against this build — at least: a missing proc, a body matching no form " + "this build deploys, a definition this principal cannot read (no VIEW DEFINITION, or " + "WITH ENCRYPTION), or compat < 130. SQL Server only; OFF = byte-identical." ), ) fifo_claim_prepared: bool = Field( diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index 03cca7c7..e51966c5 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -167,9 +167,23 @@ def _utf16_units(text: str) -> int: return len(text.encode("utf-16-le")) // 2 -def _encode_proc_lanes(lanes: Sequence[str]) -> str: - """Encode the (deduped, chunk-clamped) lane list as the proc's one JSON-array parameter. - ``json.dumps`` default escaping — no delimiter contract is ever imposed on connection names.""" +def _keep_matchable_lanes(lanes: Sequence[str]) -> list[str]: + """Drop requested lane names that exceed the NVARCHAR(256) lane column (AC-11). + + Such a lane can never equal a stored lane, so skipping it client-side is a pure no-op on the + RESULT — but it is NOT optional, and it must run for EVERY dispatch path. The ad-hoc batch + binds the lane list into a ``(VALUES (?),…)`` derived table that lands in a + ``DECLARE @heads TABLE (lane NVARCHAR(256) NOT NULL`` — SQL Server evaluates that narrowing + conversion on the outer constant scan BEFORE the CROSS APPLY filters it, and with ANSI_WARNINGS + ON it raises 2628 ("String or binary data would be truncated") even when zero rows would have + matched. So an unfiltered oversized lane makes the batch RAISE where the contract says it must + claim nothing — no-match parity broken, and 2628 is not 1222 so it is not translated to + EMPTY-all either: it rolls back and re-raises to the dispatcher. + + Applied once in ``claim_fifo_heads`` ahead of the dispatch-path split, so the proc, prepared and + batch branches cannot disagree. (Before this, only the two flagged branches filtered, via + ``_encode_proc_lanes`` — and the gap was unreachable in practice only because sub-lever A's + startup gate never passed, so the parity test never reached its batch arm.)""" kept = [] for lane in lanes: units = _utf16_units(lane) @@ -183,7 +197,15 @@ def _encode_proc_lanes(lanes: Sequence[str]) -> str: ) continue kept.append(lane) - return json.dumps(kept) + return kept + + +def _encode_proc_lanes(lanes: Sequence[str]) -> str: + """Encode the (deduped, filtered, chunk-clamped) lane list as the proc's one JSON-array + parameter. ``json.dumps`` default escaping — no delimiter contract is ever imposed on + connection names. Oversized lanes are removed upstream by ``_keep_matchable_lanes``; the call + here is idempotent and kept so this encoder is safe to use on an unfiltered list.""" + return json.dumps(_keep_matchable_lanes(lanes)) def _claim_proc_param_pins() -> list[tuple[int, int, int]]: @@ -802,11 +824,22 @@ def _fifo_heads_steps(*, lane_col: str, lane_source: str, epoch_guard: str) -> s " WHERE ll.lease_key = @lease_key) <= @leader_epoch)" ) +# The module head the shipped deploy path emits — the ONE place this literal lives in production +# code. ``_claim_proc_body`` renders it and ``_claim_proc_stored_forms`` anchors on it, so an edit +# to the head is mechanically an edit to BOTH sides of the gate's comparison. The anchor is +# load-bearing: a leading ``--`` comment or ``;`` in the body would otherwise blind the gate +# silently, so ``_claim_proc_stored_forms`` RAISES rather than falling through. +# tests/test_adr0114_claim_proc.py re-states this literal independently on purpose — do NOT +# collapse that assertion onto this constant, or the check stops being a check. +_CLAIM_PROC_HEAD: Final[str] = "CREATE OR ALTER PROCEDURE dbo." + def _claim_proc_body(proc_name: str, lane_col: str) -> str: """The full ``CREATE OR ALTER PROCEDURE`` statement for one lane family — the text inside the - guarded ``EXEC(N'...')`` and the text ``OBJECT_DEFINITION()`` returns for the startup gate's - normalized-hash comparison. The body is the shipped batch verbatim (via ``_fifo_heads_steps``) + guarded ``EXEC(N'...')``. This is the text SUBMITTED, which is NOT the text + ``OBJECT_DEFINITION()`` returns: the engine deletes the ``OR`` and ``ALTER`` tokens from the + stored module, so the startup gate compares against ``_claim_proc_stored_forms()`` — never + against this string directly. The body is the shipped batch verbatim (via ``_fifo_heads_steps``) with exactly the ADR 0114 §4 mechanical substitutions: the DECLARE block becomes the parameter list, the VALUES lane list becomes the one-JSON-parameter OPENJSON decode, the spliced epoch guard becomes the fixed nullable form, plus the conditional ``@fold_reset`` tail (sub-lever C's @@ -823,7 +856,7 @@ def _claim_proc_body(proc_name: str, lane_col: str) -> str: honestly-stated delta from the batch (the outbound post-proc H2 statements may emit rowcount DONE tokens; harmless for the execute/fetchone consumers — ADR 0114 §3).""" return ( - f"CREATE OR ALTER PROCEDURE dbo.{proc_name}" + f"{_CLAIM_PROC_HEAD}{proc_name}" " @now FLOAT, @stage NVARCHAR(16), @k INT," " @pending NVARCHAR(32), @inflight NVARCHAR(32)," " @lanes NVARCHAR(MAX)," @@ -882,20 +915,92 @@ def _claim_proc_ddl(proc_name: str, lane_col: str) -> str: def _normalize_tsql(text: str) -> str: """Whitespace-normalize a T-SQL module body for the startup gate's hash comparison: collapse - every whitespace run to one space and strip. OBJECT_DEFINITION() preserves the definition text - as executed, but line endings / trailing whitespace may differ across deployment paths.""" + every whitespace run to one space and strip — line endings and interior spacing differ across + deployment paths and across the engine's own module rewrite. + + LOAD-BEARING, do not "simplify" to a line-ending normalizer: the engine deletes the ``OR`` and + ``ALTER`` TOKENS from a ``CREATE OR ALTER`` head and KEEPS their separators, so the stored head + comes back as ``CREATE`` + three spaces + ``PROCEDURE``. Collapsing runs is what folds that + residual spacing away; without it the gate re-breaks even with the head expansion in place. + + Note this normalization is applied to the DEPLOYED text too, so it is semantically lossy inside + string literals and comments. The compensating control is the AC-8 body lint + (``test_ac8_proc_body_hard_rules``), which pins the shipped body free of quotes, ``--``, ``/*`` + and non-ASCII — keep those assertions.""" return " ".join(text.split()) -def _claim_proc_shipped_hashes() -> dict[str, str]: - """proc name -> SHA-256 of the normalized shipped body (the startup gate's expected values).""" +# Every module head a SQL Server may STORE for a module THIS code deployed as _CLAIM_PROC_HEAD. +# +# OBJECT_DEFINITION() does not return a CREATE OR ALTER module verbatim: the engine deletes the OR +# and ALTER keyword TOKENS and keeps their delimiting separators, so the submitted head comes back +# as "CREATE" + three spaces + "PROCEDURE" (char delta exactly 7). MEASURED on SQL Server 2022 +# 16.0.4255.1 and 2025 17.0.4055.5, compat 130/160/170, across five deploy paths: fresh CREATE, the +# OR ALTER re-apply, a plain batch, the shipped guarded EXEC(N'...') wrapper, and an out-of-band +# ALTER PROCEDURE (which the engine ALSO rewrites, to a single-spaced CREATE PROCEDURE). Case is +# preserved, not folded. _normalize_tsql collapses the spacing, so the rewritten head is keyed here +# in its collapsed spelling. THIS GATE WAS INERT IN EVERY DEPLOYMENT since it shipped, because it +# compared only against the verbatim form — which no SQL Server can return. +# +# The VERBATIM head is retained as a second accepted form for an engine that does not rewrite. It +# is the text this code SUBMITTED, so accepting it asserts strictly LESS than accepting the rewrite +# does: an engine handing back our own bytes is zero drift, not a tamper event. Which form was +# observed is recorded and logged, so a non-rewriting engine stays DISCOVERABLE — it just is not +# alarmed on. +# +# RULE for any future entry — a head belongs here ONLY if a server can produce it from the text +# _claim_proc_body() renders. `CREATE PROC`, a lower-cased head, or any other spelling this deploy +# path cannot emit is positive evidence of an out-of-band hand deploy, which is precisely the AC-7 +# signal, and MUST keep failing the gate. This is not a compatibility grab-bag. +_CLAIM_PROC_STORED_HEADS: Final[tuple[tuple[str, str], ...]] = ( + ("rewritten", "CREATE PROCEDURE dbo."), + ("verbatim", _CLAIM_PROC_HEAD), +) + + +def _claim_proc_stored_forms(proc_name: str, lane_col: str) -> dict[str, str]: + """label -> the normalized text ``OBJECT_DEFINITION()`` may return for OUR deployed module. + + SHIPPED-SIDE ONLY: the deployed text is still hashed as ``_normalize_tsql(deployed)``, + untouched. The accepted set is therefore a small collection of code-controlled CONSTANTS rather + than a preimage class of a lossy transform over server-supplied text — a wrong constant can only + make the gate expect the wrong body (degrade loudly), never widen what it accepts. + + Raises ValueError if the shipped body no longer starts with the anchor; ``_gate_claim_proc``'s + blanket ``except Exception`` turns that into a loud degrade, never an open() failure.""" + shipped = _claim_proc_body(proc_name, lane_col) + if not shipped.startswith(_CLAIM_PROC_HEAD): + raise ValueError( + f"_claim_proc_body({proc_name!r}) no longer starts with {_CLAIM_PROC_HEAD!r}: the AC-7" + " startup gate anchors its stored-head expansion on that literal, so a leading comment," + " semicolon or SET statement would silently blind the gate. Change _claim_proc_body," + " _CLAIM_PROC_HEAD and _CLAIM_PROC_STORED_HEADS together." + ) + tail = shipped[len(_CLAIM_PROC_HEAD) :] + return {label: _normalize_tsql(head + tail) for label, head in _CLAIM_PROC_STORED_HEADS} + + +def _claim_proc_shipped_hashes() -> dict[str, dict[str, str]]: + """proc name -> {SHA-256 of an accepted normalized body: which stored head form it is}. + + Keyed PER PROC, deliberately: ONE flat set across both procs would accept the cid body served + under the dst name (``sp_rename`` does not rewrite ``sys.sql_modules.definition``, so a rename + or a botched blue/green swap reaches that state with no tampering intent) — a silent cross-lane + predicate swap that claims zero rows forever. Do NOT flatten. + + NEVER memoize this at module scope. The ValueError from ``_claim_proc_stored_forms`` is caught + by ``_gate_claim_proc`` and becomes a loud degrade; evaluated at import time it would be an + ImportError for the whole module — a hard outage. (``_SCHEMA`` already calls ``_claim_proc_ddl`` + at import time, so that refactor is plausible; this one must not follow it.)""" return { - _CLAIM_PROC_CID: hashlib.sha256( - _normalize_tsql(_claim_proc_body(_CLAIM_PROC_CID, "channel_id")).encode() - ).hexdigest(), - _CLAIM_PROC_DST: hashlib.sha256( - _normalize_tsql(_claim_proc_body(_CLAIM_PROC_DST, "destination_name")).encode() - ).hexdigest(), + proc_name: { + hashlib.sha256(text.encode()).hexdigest(): label + for label, text in _claim_proc_stored_forms(proc_name, lane_col).items() + } + for proc_name, lane_col in ( + (_CLAIM_PROC_CID, "channel_id"), + (_CLAIM_PROC_DST, "destination_name"), + ) } @@ -1555,6 +1660,10 @@ def __init__( self._claim_proc_effective = False self._claim_proc_degraded_reason: str | None = None self._claim_proc_input_sizes: list[tuple[int, int, int]] | None = None + # proc name -> which _CLAIM_PROC_STORED_HEADS form the deployed module actually matched + # ("rewritten" on every engine measured to date; "verbatim" would mean this server does NOT + # rewrite CREATE OR ALTER — an engine difference worth knowing about, not a tamper event). + self._claim_proc_head_forms: dict[str, str] = {} self._claim_proc_setinputsizes_warned = False # ADR 0114 sub-lever B (fifo_claim_prepared): stable claim text + a retained prepared # cursor on store-owned dedicated connections (INGRESS/ROUTED). Read ONCE at open; the @@ -1644,12 +1753,16 @@ async def _gate_claim_proc(self) -> None: """ADR 0114 §4 startup gate — fail-safe to the batch, loudly (AC-7). With ``fifo_claim_proc`` ON, open() probes (a) both procs exist, (b) each deployed body's normalized SHA-256 (via OBJECT_DEFINITION — existence alone cannot catch a hand-edited - body, and the proc IS the claim logic) matches the shipped DDL text, and (c) - compatibility_level >= 130 (OPENJSON). Any miss records the reason, logs a WARNING, and + body, and the proc IS the claim logic) matches one of ``_claim_proc_stored_forms()``, and + (c) compatibility_level >= 130 (OPENJSON). Any miss records the reason, logs a WARNING, and leaves ``_claim_proc_effective`` False — the shipped batch runs; NEVER a lane outage. - Out-of-band drift is caught at the next open; ``DELETE FROM schema_meta`` forces a full - re-create.""" + + The comparison is against the STORED forms, not against ``_claim_proc_body()`` directly: + the engine rewrites the ``CREATE OR ALTER`` head when it stores the module, so comparing + with the submitted text can never match (the defect that left this gate inert in every + deployment from the feature shipping until this commit).""" reason: str | None = None + head_forms: dict[str, str] = {} try: row = await self._fetchone( "SELECT compatibility_level FROM sys.databases WHERE name = DB_NAME()" @@ -1672,13 +1785,19 @@ async def _gate_claim_proc(self) -> None: ) break got = hashlib.sha256(_normalize_tsql(deployed).encode()).hexdigest() - if got != expected[proc_name]: + matched = expected[proc_name].get(got) + if matched is None: reason = ( - f"stored procedure dbo.{proc_name} body does not match the shipped" - " definition (out-of-band edit? DELETE FROM schema_meta forces a" - " re-create at next open)" + f"stored procedure dbo.{proc_name} body matches no form this build" + " deploys — an out-of-band edit, a hand deploy (a head spelling this" + " code cannot emit, e.g. CREATE PROC or a differing case), a renamed" + " proc (sp_rename does not rewrite the stored definition), or a build" + " whose body was changed without bumping the _v1 proc name. The" + " shipped batch runs. Compare OBJECT_DEFINITION(OBJECT_ID('dbo." + f"{proc_name}')) against this build's own definition to see the drift" ) break + head_forms[proc_name] = matched except Exception as exc: # noqa: BLE001 - §4: ANY gate failure degrades, never an outage # A transient probe failure (e.g. a hiccup on the metadata read) must not fail the # open — the ADR's rule is total: any gate miss runs the shipped batch, loudly. @@ -1687,12 +1806,25 @@ async def _gate_claim_proc(self) -> None: self._claim_proc_effective = True self._claim_proc_degraded_reason = None self._claim_proc_input_sizes = _claim_proc_param_pins() + self._claim_proc_head_forms = head_forms log.info( "fifo_claim_proc: startup gate PASSED — pooled claims will use" - " dbo.%s / dbo.%s (ADR 0114 sub-lever A)", + " dbo.%s / dbo.%s (ADR 0114 sub-lever A); stored head forms: %s", _CLAIM_PROC_CID, _CLAIM_PROC_DST, + head_forms, ) + if any(form == "verbatim" for form in head_forms.values()): + # Not a fault: this engine stores CREATE OR ALTER as submitted rather than deleting + # the tokens. Every engine measured to date rewrites, so this is worth surfacing — + # it means the compatibility assumption in _CLAIM_PROC_STORED_HEADS has a live + # counterexample and the ADR should record it. + log.info( + "fifo_claim_proc: this server stored the CREATE OR ALTER head VERBATIM" + " (%s) — no engine measured to date does this; please report it, the gate" + " accepts it deliberately", + head_forms, + ) else: self._claim_proc_effective = False self._claim_proc_degraded_reason = reason @@ -1703,38 +1835,73 @@ async def _gate_claim_proc(self) -> None: reason, ) - def _apply_claim_input_sizes(self, cur: Any, sizes: list[tuple[int, int, int]] | None) -> None: - """Pin the claim-path parameter descriptors on ``cur`` (ADR 0114 §4/§5 NULL-typing - hazard; 9 descriptors on the proc path, 8 on the prepared stable-text path). - ``setinputsizes`` is pure client-side descriptor state (no I/O), so the SYNC - pyodbc call is loop-safe — but the surface matters: aioodbc 0.5.0 wraps it as - ``async def setinputsizes`` (cursor.py:148, executor-routed), so calling the WRAPPER - synchronously would merely create a never-awaited coroutine and silently apply nothing - (the adversarial-review finding this method is shaped around). Therefore the underlying - pyodbc cursor (``_impl``) is preferred FIRST; the wrapper attribute is used only when it - is a plain sync callable (a bare pyodbc cursor, or a test fake). If neither surface is - reachable, warn ONCE and proceed unpinned (the G-A0 wire trace decides whether describe - traffic appears — degraded observability, never an outage).""" - if sizes is None: - return + @staticmethod + def _sync_setinputsizes(cur: Any) -> Any | None: + """The SYNC ``setinputsizes`` callable for ``cur``, or None if unreachable. The surface + matters: aioodbc 0.5.0 wraps ``setinputsizes`` as ``async def`` (cursor.py:148, + executor-routed), so calling the WRAPPER synchronously merely creates a never-awaited + coroutine that does NOTHING. So the underlying pyodbc cursor (``_impl``) is preferred + FIRST; the wrapper attribute is used only when it is a plain sync callable (a bare pyodbc + cursor, or a test fake). ``setinputsizes`` is pure client-side descriptor state (no I/O), + so the sync call is loop-safe.""" raw = getattr(cur, "_impl", None) target = getattr(raw, "setinputsizes", None) if target is None: candidate = getattr(cur, "setinputsizes", None) if candidate is not None and not inspect.iscoroutinefunction(candidate): target = candidate + return target + + def _warn_setinputsizes_unreachable(self) -> None: + if not self._claim_proc_setinputsizes_warned: + self._claim_proc_setinputsizes_warned = True + log.warning( + "ADR 0114 claim path: no synchronous setinputsizes is reachable through this" + " cursor stack — proceeding without parameter-descriptor pin/clear (NULL params" + " may incur SQLDescribeParam round trips; the ADR 0114 G-A0/G-B0 preflights" + " measure this)" + ) + + def _apply_claim_input_sizes(self, cur: Any, sizes: list[tuple[int, int, int]] | None) -> None: + """Pin the claim-path parameter descriptors on ``cur`` (ADR 0114 §4/§5 NULL-typing + hazard; 9 descriptors on the proc path, 8 on the prepared stable-text path). If no sync + surface is reachable, warn ONCE and proceed unpinned (the G-A0 wire trace decides whether + describe traffic appears — degraded observability, never an outage).""" + if sizes is None: + return + target = self._sync_setinputsizes(cur) if target is None: - if not self._claim_proc_setinputsizes_warned: - self._claim_proc_setinputsizes_warned = True - log.warning( - "ADR 0114 claim path: no synchronous setinputsizes is reachable through this" - " cursor stack — proceeding without parameter-descriptor pins (NULL params" - " may incur SQLDescribeParam round trips; the ADR 0114 G-A0/G-B0 preflights" - " measure this)" - ) + self._warn_setinputsizes_unreachable() return target(sizes) + def _clear_claim_input_sizes(self, cur: Any) -> None: + """Clear the claim-CALL parameter pins on ``cur`` before it runs the H2 DELIVERY DML. + + ``setinputsizes`` is PERSISTENT cursor state. On the proc path the pooled claim cursor is + pinned for the ``{CALL}`` (descriptor[0] = ``SQL_DOUBLE`` for ``@now FLOAT``) and then, at + OUTBOUND, the SAME cursor runs the H2 ``SELECT 1 FROM delivered_keys WHERE outbox_id=?`` — + binding the NVARCHAR ``d["id"]`` against the stale ``SQL_DOUBLE`` descriptor throws a + client-side ``22018`` cast error, rolls the claim back, and collapses outbound delivery. + The pins' only purpose was the CALL's NULL-fence describe-avoidance, so clear them the + moment the CALL's result is drained. + + Only PARAMETERIZED statements are affected — a zero-parameter execute (notably the shielded + ``SET LOCK_TIMEOUT -1;`` reset in this method's ``finally``) tolerates surplus descriptors, + measured on pyodbc 5.3.0 / ODBC Driver 18. So the exposure is exactly the H2 bind chain and + clearing here covers all of it. + + ``setinputsizes(None)`` reverts all params to default inference (pyodbc 5.3.0), on the SYNC + ``_impl`` surface — a bare ``cur.setinputsizes(None)`` on the async wrapper is a + never-awaited no-op, and ``_apply_claim_input_sizes(cur, None)`` early-returns on its None + guard, so neither clears anything. Best-effort: if no sync surface is reachable the pins + were never applied either, so there is nothing to clear.""" + target = self._sync_setinputsizes(cur) + if target is None: + self._warn_setinputsizes_unreachable() + return + target(None) + @property def claim_prepared_effective(self) -> bool: """Whether the ADR 0114 prepared claim path is EFFECTIVELY active this run: the @@ -6309,8 +6476,20 @@ async def claim_fifo_heads( # outstanding-head retry semantics — exactly as ADR 0058 excludes them from batching. per_lane_limit = 1 # Dedupe (preserving request order; duplicate lanes would violate @heads' PRIMARY KEY) + - # chunk clamp; the caller covers the remainder with a second call. - lane_list = list(dict.fromkeys(lanes))[:_FIFO_HEADS_LANE_CHUNK] + # chunk clamp; the caller covers the remainder with a second call. THEN drop lanes too long + # to ever match (AC-11). The clamp deliberately runs BEFORE the skip: an oversized lane + # occupies a chunk slot it can never match, which is the pre-existing, tested contract + # (test_prepared_lane_encoding_shares_the_proc_rules pins 499, not 500). Reordering these + # would serve more lanes per call, but that is a separate decision and not part of this fix. + # + # The skip sits here, ahead of the dispatch-path split below, so the proc, prepared and + # ad-hoc batch branches cannot disagree about it. Previously only the two flagged branches + # filtered (inside _encode_proc_lanes) and the batch bound the raw list — where an oversized + # lane is not merely useless but FATAL (2628 on the @heads narrowing conversion; see + # _keep_matchable_lanes). That gap was unreachable in practice only because sub-lever A's + # startup gate never passed, so the AC-11 parity test never reached its batch arm. + # The existing empty guard then covers a request that was entirely oversized. + lane_list = _keep_matchable_lanes(list(dict.fromkeys(lanes))[:_FIFO_HEADS_LANE_CHUNK]) if not lane_list: return ClaimedHeads(by_lane={}, rearm=frozenset()) # H1 FENCING TOKEN — identical to the single claim, applied to the probe AND the UPDATE so a @@ -6494,6 +6673,18 @@ async def claim_fifo_heads( # before the connection returns to the pool (no-MARS). rows = await cur.fetchall() decoded = [dict(zip(columns, r)) for r in rows] # noqa: B905 + if use_proc: + # The proc CALL pinned 9 parameter descriptors on this POOLED cursor + # (descriptor[0] = SQL_DOUBLE for @now FLOAT); those pins are PERSISTENT cursor + # state, and the H2 delivery DML below runs on the SAME cursor at OUTBOUND — + # binding the NVARCHAR d["id"] against the stale SQL_DOUBLE descriptor throws a + # client 22018 cast error and collapses delivery. Clear the pins now, the moment + # the CALL's result is drained and before any H2 bind. (use_prepared is + # INGRESS/ROUTED-only, where the H2 branch is a no-op — its retained-cursor pins + # are never poisoned AND must persist for reuse across calls, so they are NOT + # cleared here. On ingress/routed the proc's own H2 branch is also a no-op, so + # this clear is simply harmless there.) + self._clear_claim_input_sizes(cur) if any(d["id"] is None for d in decoded): # kept != claimed (ADR 0066 §3.2 STEP 5) — fail closed: roll the whole call # back, claim nothing. Reachable via an ordinary fence race, not only a bug: diff --git a/tests/test_adr0114_claim_proc.py b/tests/test_adr0114_claim_proc.py index 498a4eb8..0b5b48dc 100644 --- a/tests/test_adr0114_claim_proc.py +++ b/tests/test_adr0114_claim_proc.py @@ -11,9 +11,15 @@ - **AC-8** proc-body hard rules: no ``BEGIN/COMMIT/ROLLBACK``, no ``TRY/CATCH``, no ``SET XACT_ABORT``; ``SET LOCK_TIMEOUT`` appears ONLY as the opener (0) and the conditional ``@fold_reset`` tail (-1), the tail is the final statement. -- **AC-7** startup gate: accept (whitespace-tolerant body hash), degrade on missing proc / - hand-edited body / compat < 130 — each with a WARNING naming the reason, the degraded gauge set, - and the claim staying on the batch path (never a lane outage). +- **AC-7 / AC-7b** startup gate, BOTH directions. It ACCEPTS the body as SQL Server actually + stores it — the engine deletes the ``OR``/``ALTER`` tokens from a ``CREATE OR ALTER`` head, so + the deployed text never equals the submitted text and the stub models that rewrite independently + (``_as_object_definition``, with a liveness receipt). And it DEGRADES on a missing proc, a body + matching no form this build deploys, a head spelling this deploy path cannot emit, the other + lane family's body, a broken head anchor, or compat < 130 — each with a WARNING naming the + reason and the claim staying on the batch path (never a lane outage). + The accept direction is the one that was missing: AC-7 as originally written is satisfied by a + gate that rejects EVERYTHING, which is exactly what shipped. - **AC-9** the proc result contract: column-identical 10-column result; the post-``execute`` code path (drain, kept==claimed adjudication, H2, commit, guard) is the SAME code — differential drive proc-vs-batch over identical fake rows yields identical results and identical @@ -39,6 +45,7 @@ from __future__ import annotations +import hashlib import inspect import json import logging @@ -68,6 +75,12 @@ _FAKE_PINS = [(1, 0, 0)] * 9 # stands in for _claim_proc_param_pins() (pyodbc-free CI) +# (proc name, lane column) for both lane families — the gate's two subjects. +_PROCS = ( + ("mefor_claim_fifo_heads_cid_v1", "channel_id"), + ("mefor_claim_fifo_heads_dst_v1", "destination_name"), +) + def _proc_store( *, @@ -201,6 +214,21 @@ def test_ac8_proc_body_hard_rules() -> None: # No single quotes in the body: the EXEC(N'...') embedding and OBJECT_DEFINITION hash # comparison stay exact (the escaping in _claim_proc_ddl is defensive, not load-bearing). assert "'" not in body + # No COMMENTS, for the same reason and a sharper one. `_normalize_tsql` collapses every + # whitespace run — including newlines — on BOTH sides of the gate's comparison. That is + # only safe while the body has no line comment: `-- note\n` and `-- note \n` + # normalize to the SAME text, but in the second the tail is swallowed by the comment. A + # `--` in the body would therefore make two semantically different modules hash equal. + assert "--" not in body, f"{proc_name}: a line comment makes the gate hash-blind" + assert "/*" not in body, f"{proc_name}: a block comment makes the gate hash-blind" + # ASCII only: Python's str.split() treats U+00A0 as whitespace and collapses it, while the + # server preserves it — an NBSP in the body is another hash-blinding hole. It is symmetric + # (and so harmless) only while no such character exists. + assert body.isascii(), f"{proc_name}: non-ASCII in the body can blind the gate hash" + # The head anchor the startup gate expands on. Stated INDEPENDENTLY of _CLAIM_PROC_HEAD on + # purpose — collapsing this onto the constant would make the check tautological, and a + # leading comment / semicolon / SET here silently breaks the gate's stored-form expansion. + assert body.startswith(f"CREATE OR ALTER PROCEDURE dbo.{proc_name} ") def test_ac10_guarded_ddl_rides_schema() -> None: @@ -229,21 +257,73 @@ def test_ac10_guarded_ddl_rides_schema() -> None: # --- AC-7: the startup gate (stubbed _fetchone; no server) --------------------------------------- +def _as_object_definition(submitted: str) -> str: + """What OBJECT_DEFINITION() returns for a module deployed from ``submitted``. + + An INDEPENDENT statement of server behaviour: never derive this from _normalize_tsql, + _CLAIM_PROC_STORED_HEADS or any other production helper, or the round-trip below stops being a + round-trip and the gate tests go back to comparing a function against itself. + + MEASURED (SQL Server 2022 16.0.4255.1 and 2025 17.0.4055.5, compat 130/160/170, five deploy + paths): the engine deletes the OR and ALTER keyword TOKENS and KEEPS their separators, so a + submitted `CREATE OR ALTER PROCEDURE` head is stored as `CREATE` + three spaces + `PROCEDURE`. + """ + head = "CREATE OR ALTER PROCEDURE" + if not submitted.startswith(head): + return submitted + return "CREATE PROCEDURE" + submitted[len(head) :] # exactly three spaces + + +def test_object_definition_fixture_actually_perturbs() -> None: + """Liveness receipt for _as_object_definition. + + Without this, the fixture could silently degrade to the identity function and every gate test + below would quietly go back to being a tautology — which is precisely how the startup gate + shipped inert and survived review. + """ + for proc, col in _PROCS: + submitted = ss._claim_proc_body(proc, col) + stored = _as_object_definition(submitted) + assert stored != submitted, "the fixture must model a real transform" + assert len(submitted) - len(stored) == 7, "OR + ALTER deleted, separators kept" + assert stored.startswith("CREATE PROCEDURE dbo."), stored[:40] + assert submitted.split()[3:] == stored.split()[1:], "only the head differs" + + def _gate_rows( *, compat: int = 150, - cid_body: str | None = "SHIPPED", - dst_body: str | None = "SHIPPED", + cid_body: str | None = "STORED", + dst_body: str | None = "STORED", ) -> dict[str, dict[str, Any] | None]: - """Build the _fetchone stub's answers. "SHIPPED" resolves to the real shipped body text.""" - if cid_body == "SHIPPED": - cid_body = ss._claim_proc_body("mefor_claim_fifo_heads_cid_v1", "channel_id") - if dst_body == "SHIPPED": - dst_body = ss._claim_proc_body("mefor_claim_fifo_heads_dst_v1", "destination_name") + """Build the _fetchone stub's answers. + + "STORED" (the DEFAULT) resolves to the shipped body as OBJECT_DEFINITION actually returns it — + the form a real server can produce. "SUBMITTED" resolves to the verbatim text this code sends, + which no measured SQL Server stores; it models a hypothetical non-rewriting engine and must be + requested explicitly. + + The old default fed SUBMITTED text back as the DEPLOYED body, modelling the server as an + identity function. Both sides of the gate's comparison were then the same function of the same + argument, so the suite could not distinguish a working gate from a broken one — the defect that + let ADR 0114 sub-lever A ship inert. Do not restore it. + """ + + def resolve(value: str | None, proc: str, col: str) -> str | None: + if value == "SUBMITTED": + return ss._claim_proc_body(proc, col) + if value == "STORED": + return _as_object_definition(ss._claim_proc_body(proc, col)) + return value + return { "compat": {"compatibility_level": compat}, - "dbo.mefor_claim_fifo_heads_cid_v1": {"body": cid_body}, - "dbo.mefor_claim_fifo_heads_dst_v1": {"body": dst_body}, + "dbo.mefor_claim_fifo_heads_cid_v1": { + "body": resolve(cid_body, "mefor_claim_fifo_heads_cid_v1", "channel_id") + }, + "dbo.mefor_claim_fifo_heads_dst_v1": { + "body": resolve(dst_body, "mefor_claim_fifo_heads_dst_v1", "destination_name") + }, } @@ -287,12 +367,117 @@ async def test_ac7_gate_accepts_shipped_bodies( async def test_ac7_gate_tolerates_whitespace_differences(monkeypatch: pytest.MonkeyPatch) -> None: # OBJECT_DEFINITION may differ in line endings / whitespace across deployment paths — the # comparison is over the NORMALIZED text (ADR §4 "normalized"). - shipped = ss._claim_proc_body("mefor_claim_fifo_heads_cid_v1", "channel_id") - mangled = shipped.replace(" SET LOCK_TIMEOUT 0;", "\r\n SET LOCK_TIMEOUT 0;\n") + # + # NOTE the base text is the STORED form, not the submitted one. This test passes cid_body= + # explicitly, so it bypasses _gate_rows' default — it was a SECOND door through which the + # verbatim `CREATE OR ALTER` head reached the gate as a fake "deployed" body, and it kept the + # tautology alive even after the default was fixed. Mangle the server form, not our own text. + stored = _as_object_definition( + ss._claim_proc_body("mefor_claim_fifo_heads_cid_v1", "channel_id") + ) + mangled = stored.replace(" SET LOCK_TIMEOUT 0;", "\r\n SET LOCK_TIMEOUT 0;\n") + assert mangled != stored, "the mangle must actually perturb (non-vacuity)" store = await _gate(_gate_rows(cid_body=mangled), monkeypatch) assert store.claim_proc_effective is True +@pytest.mark.parametrize(("proc_name", "lane_col"), _PROCS) +def test_ac7_expected_hashes_are_exactly_two_named_constants(proc_name: str, lane_col: str) -> None: + """The TOTAL-BEHAVIOUR pin: acceptance is `sha256(_normalize_tsql(deployed)) in expected`, so a + set-equality assertion on `expected` completely determines the gate over every possible + deployed text. No equivalent assertion exists for a regex/two-sided design — that is the single + strongest reason this shape was chosen. A third head silently appended later turns this RED.""" + tail = ss._claim_proc_body(proc_name, lane_col)[len(ss._CLAIM_PROC_HEAD) :] + want = { + hashlib.sha256( + ss._normalize_tsql("CREATE PROCEDURE dbo." + tail).encode() + ).hexdigest(): "rewritten", + hashlib.sha256( + ss._normalize_tsql("CREATE OR ALTER PROCEDURE dbo." + tail).encode() + ).hexdigest(): "verbatim", + } + assert ss._claim_proc_shipped_hashes()[proc_name] == want + + +async def test_ac7_gate_accepts_the_body_as_sql_server_actually_stores_it( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """THE regression test. RED under the shipped defect and under any no-op "fix"; green only if + the `rewritten` constant is correct. This is the assertion whose absence let the gate ship + inert — it degraded on EVERY open in EVERY deployment and nothing offline noticed.""" + store = await _gate(_gate_rows(), monkeypatch) # default = STORED + assert store.claim_proc_effective is True + assert store.claim_proc_degraded_reason is None + assert store._claim_proc_head_forms == { + "mefor_claim_fifo_heads_cid_v1": "rewritten", + "mefor_claim_fifo_heads_dst_v1": "rewritten", + } + + +async def test_ac7_gate_accepts_a_non_rewriting_engines_verbatim_storage( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Retained from the pre-fix suite, but it no longer masquerades as a server round-trip: it + models an engine that stores CREATE OR ALTER VERBATIM (the Azure family / pre-2019 flavours the + _claim_proc_ddl guard admits but for which no image was obtainable — UNTESTED, inference only). + The default fixture is now the MEASURED rewrite; this is the explicit exception.""" + store = await _gate(_gate_rows(cid_body="SUBMITTED", dst_body="SUBMITTED"), monkeypatch) + assert store.claim_proc_effective is True + assert set(store._claim_proc_head_forms.values()) == {"verbatim"} + + +@pytest.mark.parametrize( + "head", ["CREATE PROC dbo.", "create procedure dbo.", "CrEaTe pRoCeDuRe dbo."] +) +async def test_ac7_gate_rejects_head_forms_this_deploy_path_cannot_emit( + head: str, monkeypatch: pytest.MonkeyPatch +) -> None: + """ANTI-OVER-WIDE guard. `CREATE PROC` and case-differing heads are MEASURED to survive the + server round-trip, and `_claim_proc_ddl` provably cannot emit them — so each is affirmative + evidence of an out-of-band hand deploy, which IS the AC-7 event. A two-sided regex + canonicalization accepts all three and launders exactly the signal the gate exists to raise.""" + tail = ss._claim_proc_body("mefor_claim_fifo_heads_cid_v1", "channel_id")[ + len(ss._CLAIM_PROC_HEAD) : + ] + store = await _gate(_gate_rows(cid_body=head + tail), monkeypatch) + assert store.claim_proc_effective is False + assert "matches no form this build deploys" in (store.claim_proc_degraded_reason or "") + + +async def test_ac7_gate_rejects_the_cid_body_served_under_the_dst_name( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The expected map must stay keyed PER PROC. A single flat accepted-set across both procs + would take the cid body under the dst name — reachable with no tampering intent via sp_rename + (which does NOT rewrite sys.sql_modules.definition) or a botched blue/green swap — and silently + swap the lane predicate, claiming zero rows forever. RED the moment anyone flattens it.""" + cid_stored = _as_object_definition( + ss._claim_proc_body("mefor_claim_fifo_heads_cid_v1", "channel_id") + ) + store = await _gate(_gate_rows(dst_body=cid_stored), monkeypatch) + assert store.claim_proc_effective is False + assert "mefor_claim_fifo_heads_dst_v1" in (store.claim_proc_degraded_reason or "") + + +@pytest.mark.parametrize("prefix", ["-- note\n", ";", "SET ANSI_NULLS ON; "]) +async def test_ac7_gate_degrades_loudly_when_the_body_head_anchor_breaks( + prefix: str, monkeypatch: pytest.MonkeyPatch +) -> None: + """The MEASURED anchor fragility, pinned fail-closed and DIAGNOSABLE. A leading comment or + semicolon in _claim_proc_body breaks the head expansion; the anchor check raises, the gate's + blanket except turns it into a distinct 'probe failed' reason naming the constant to fix. A + regex design no-ops silently here and reports a generic body mismatch instead — the one reason + string indistinguishable from a real tamper.""" + real = ss._claim_proc_body + + monkeypatch.setattr(ss, "_claim_proc_body", lambda p, c: prefix + real(p, c)) + store = await _gate(_gate_rows(cid_body=None, dst_body=None), monkeypatch) + assert store.claim_proc_effective is False + reason = store.claim_proc_degraded_reason or "" + assert "startup-gate probe failed" in reason + assert "_CLAIM_PROC_HEAD" in reason, "the raise must name the constant to change" + + @pytest.mark.parametrize( ("answers_kw", "reason_fragment"), [ @@ -302,7 +487,7 @@ async def test_ac7_gate_tolerates_whitespace_differences(monkeypatch: pytest.Mon { "cid_body": "CREATE OR ALTER PROCEDURE dbo.mefor_claim_fifo_heads_cid_v1 AS SELECT 1;" }, - "does not match the shipped definition", + "matches no form this build deploys", ), ({"compat": 120}, "compatibility_level 120 < 130"), ], @@ -350,19 +535,31 @@ async def boom(sql: str, params: tuple[Any, ...] = ()) -> dict[str, Any] | None: assert "startup-gate probe failed" in store.claim_proc_degraded_reason -def test_ac9_no_proc_branch_after_execute() -> None: - # AC-9's structural half: everything after cur.execute is the SAME code on both paths — the - # method must not branch on use_proc after the execute statement (the setinputsizes pin is - # legitimately before it). +def test_ac9_result_processing_is_one_shared_path_after_the_pin_clear() -> None: + # AC-9's structural half: the RESULT PROCESSING is the SAME code on both paths — the drain, the + # kept==claimed adjudication, the H2 loop, commit, and the guard must not branch on use_proc. + # The ONE legitimate use_proc branch after the execute is the parameter-pin clear (cursor-state + # hygiene: drop the proc CALL's descriptor pins before the shared H2 DML re-binds on the same + # cursor — NOT a divergence in the result logic). So the invariant is: no use_proc appears + # AFTER the clear. source = inspect.getsource(SqlServerStore.claim_fifo_heads) + clear_at = source.index("self._clear_claim_input_sizes(cur)") execute_at = source.index("await cur.execute(sql, args)") - assert "use_proc" not in source[execute_at:], ( - "AC-9: no use_proc-conditional code may exist after the execute — the drain," - " adjudication, H2, commit, and guard are one shared path" + # In [execute, clear) the ONLY use_proc token is the single `if use_proc:` clear guard — so a + # smuggled `elif use_proc:` / `x = use_proc` / `if use_proc and ...:` in that window is caught. + window = source[execute_at:clear_at] + assert window.count("use_proc") == 1 and "if use_proc:" in window, ( + "AC-9: the only use_proc branch between the execute and the pin-clear is the clear guard" + ) + tail = source[clear_at + len("self._clear_claim_input_sizes(cur)") :] + assert "use_proc" not in tail, ( + "AC-9: after the pin-clear, no use_proc-conditional code may exist — the adjudication, H2," + " commit, and guard are one shared path" ) -# Golden pins for the two proc BODIES (the OBJECT_DEFINITION-comparable text): any drift in the +# Golden pins for the two proc BODIES (the text this build SUBMITS — note the server stores a +# rewritten head, so this is NOT the OBJECT_DEFINITION text; see _as_object_definition): drift in the # shared fragments, the OPENJSON lane source, the fixed-nullable epoch guard, the signature, or # the @fold_reset tail fails here and must be a reviewed, deliberate change (re-pin + the ADR 0064 # hash re-applies the DDL; the startup gate's expected hashes follow automatically). @@ -484,7 +681,9 @@ async def test_setinputsizes_pins_via_impl_on_the_aioodbc_shape() -> None: conn = _FakeConn(ops) _wire(store, cur, conn) await store.claim_fifo_heads("ingress", ["lane-0"], now=_NOW) - assert cur._impl.pins == [_FAKE_PINS] + # The pins are applied for the CALL and then CLEARED (None) once the CALL drains, so the H2 + # delivery bind on this same pooled cursor cannot inherit descriptor[0] = SQL_DOUBLE. + assert cur._impl.pins == [_FAKE_PINS, None] assert cur.wrapper_calls == 0, "the async aioodbc wrapper must never be called synchronously" assert ops[0][1] == _CALL_CID @@ -584,6 +783,35 @@ async def test_ac11_oversized_lane_skipped_with_warning(caplog: pytest.LogCaptur assert result == ClaimedHeads(by_lane={}, rearm=frozenset()) +async def test_ac11_oversized_lane_is_skipped_on_the_AD_HOC_BATCH_too( + caplog: pytest.LogCaptureFixture, +) -> None: + """AC-11 no-match parity applies to ALL THREE dispatch paths, and the batch is where it BITES. + + The batch binds the lane list into ``(VALUES (?),…)`` feeding a + ``DECLARE @heads TABLE (lane NVARCHAR(256) NOT NULL``. SQL Server evaluates that narrowing + conversion on the outer constant scan BEFORE the CROSS APPLY can filter it, and with + ANSI_WARNINGS ON it raises 2628 even when zero rows would match — so an unfiltered oversized + lane makes the batch RAISE where the contract says "claim zero rows on both paths". 2628 is + not 1222, so it is not translated to EMPTY-all either: it rolls back and re-raises. + + Only the two FLAGGED branches used to filter (inside ``_encode_proc_lanes``). The gap was + unreachable in practice solely because sub-lever A's startup gate never passed — fixing the + gate is what exposed it, live, as a 2628 in the AC-11 parity leg. Offline guard so the live + leg is not the only thing standing between this and a regression. + """ + lanes = ["ok-lane", "x" * 257, "another-ok"] + with caplog.at_level(logging.WARNING, logger="messagefoundry.store.sqlserver"): + ops, _, result = await _drive_proc("ingress", lanes, store=_proc_store(proc=False)) + sql, args = ops[0][1], ops[0][2] + # TWO slots for three requested lanes — the oversized name never reaches the constant scan. + assert "(VALUES (?),(?))" in sql, f"the oversized lane must not get a VALUES slot: {sql[:200]}" + assert "x" * 257 not in args, "the oversized lane must never be bound on the batch" + assert args[5:] == ("ok-lane", "another-ok"), args[5:] + assert any("257-UTF-16-unit requested lane name" in r.getMessage() for r in caplog.records) + assert result == ClaimedHeads(by_lane={}, rearm=frozenset()) + + async def test_ac11_oversized_measured_in_utf16_units_not_code_points( caplog: pytest.LogCaptureFixture, ) -> None: diff --git a/tests/test_adr0114_claim_proc_live.py b/tests/test_adr0114_claim_proc_live.py index f59c0b45..4e8e90d9 100644 --- a/tests/test_adr0114_claim_proc_live.py +++ b/tests/test_adr0114_claim_proc_live.py @@ -137,6 +137,15 @@ async def test_live_differential_proc_vs_batch(proc_store: SqlServerStore) -> No async def test_ac7_tampered_body_degrades_next_open(proc_store: SqlServerStore) -> None: # Hand-edit one proc body out-of-band, then re-open: the gate must degrade loudly (the # OBJECT_DEFINITION hash mismatch), and the tampered store must still claim on the batch. + # + # POSITIVE CONTROL FIRST — this is not optional. Until the head-form fix landed, the gate + # rejected EVERY body on every server, so this test's degrade assertions were satisfied by a + # gate that had never matched anything: it passed while proving nothing. Pinning "the gate is + # green on the untampered proc" in the same test is what makes the degrade below evidence. + assert proc_store.claim_proc_effective is True, ( + "positive control: the gate must ACCEPT the correctly deployed proc, else the tamper" + " assertions below are vacuous" + ) await proc_store._execute( f"ALTER PROCEDURE dbo.{ss._CLAIM_PROC_CID} @now FLOAT, @stage NVARCHAR(16), @k INT," " @pending NVARCHAR(32), @inflight NVARCHAR(32), @lanes NVARCHAR(MAX)," @@ -148,7 +157,7 @@ async def test_ac7_tampered_body_degrades_next_open(proc_store: SqlServerStore) try: assert tampered.claim_proc_effective is False assert tampered.claim_proc_degraded_reason is not None - assert "does not match the shipped definition" in tampered.claim_proc_degraded_reason + assert "matches no form this build deploys" in tampered.claim_proc_degraded_reason # Still claims (batch path — never a lane outage). result = await tampered.claim_fifo_heads(Stage.INGRESS.value, ["IB_NONE"]) assert result.by_lane == {}