diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2cb573f9..958ecd59 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -753,6 +753,9 @@ jobs: FAULT_TIMEOUT: ${{ matrix.fault_timeout }} PYTEST_TIMEOUT: ${{ matrix.pytest_timeout }} PYTEST_WORKERS: ${{ matrix.pytest_workers }} + # BACKLOG #1260: empty on purpose -- see the comment above the run line. The wrapper's + # default clause names the pyodbc class, which is not established for this leg. + RETRY_NATIVE_CRASH_CAUSE: "" # `--ignore-glob` subtracts the web console package, which runs as its OWN step below. BACKLOG # #1027 added that package to the root `testpaths` so a bare local `pytest` stops silently # excluding it; without this subtraction the SAME 356 tests would then run TWICE on every leg. @@ -790,7 +793,18 @@ jobs: # what happens when the selection lives in the run body -- the obvious spelling (`--ignore`) was # the one that silently did nothing. One list, read in one place, asserted by # tests/test_tooling_partition.py, which also pins the two halves of this wiring. - run: pytest -q -n "$PYTEST_WORKERS" --dist loadfile -m 'not tooling' --ignore-glob='*messagefoundry-webconsole*' -o faulthandler_timeout="$FAULT_TIMEOUT" --timeout="$PYTEST_TIMEOUT" + # BACKLOG #1260: WRAPPED SO A NATIVE CRASH IS NOT REPORTED AS A TEST FAILURE. A segfault kills + # the interpreter, so pytest returns 139/134 with no verdict and THREE layers of naming then + # say "tests failed" -- the check name, the step name, and `steps.tests.outcome`, which is what + # `step_margin.py` below consumes. None of them is true: the engine was fine and a process died. + # The wrapper re-runs ONLY on 139/134 and re-raises exit 1 immediately, so it cannot mask a + # regression. + # + # RETRY_NATIVE_CRASH_CAUSE IS DELIBERATELY EMPTY HERE. The wrapper's default clause names the + # pyodbc py3.14 class, which is ESTABLISHED for the database legs and is NOT established for + # this one -- the item filing this refuses to conclude it. An empty value makes the annotation + # say CAUSE NOT ESTABLISHED rather than assert a mechanism nobody has measured on this leg. + run: bash scripts/ci/retry-native-crash.sh pytest -q -n "$PYTEST_WORKERS" --dist loadfile -m 'not tooling' --ignore-glob='*messagefoundry-webconsole*' -o faulthandler_timeout="$FAULT_TIMEOUT" --timeout="$PYTEST_TIMEOUT" # THE MARGIN CHECK (BACKLOG #344 proposal 1). Last in the job so a LOW margin cannot skip a suite # that has not run yet -- a step `if:` with no status function carries an implicit `success()`, diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 24a87b73..1be811ea 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -10999,7 +10999,7 @@ gate is the wrong shape, validation of the walk is the right one. ## 1224. The `upload.prune` audit row names the file's owner as the actor of an automated sweep -> 🔢 **Re-scored 2026-08-20 -> P3.** Value **2/10** · Difficulty **2/10** · _fill-in_. Attacked the shipped claim at both cited sites and it survived on the code limb. Site 1 (request-path opportunistic sweep) is now messagefoundry/api/app.py:3918-3936: `actor="system"`, `client` argument removed entirely, and detail carries file_id + uploader + uploader_id, with a comment naming BACKLOG #1224 and citing ADR 0150 decision 4 for dropping the client. Site 2 (background UploadRetentionRunner) is messagefoundry/api/app.py:5891-5907: the lifespan-owned `_audit_upload_prune` closure also writes `actor="system"` with uploader/uploader_id in detail. Enumerated prune call sites repo-wide: `prune_expired(` is called at app.py:3918 and uploads.py:648 (inside `run_once`) only, so at least those two are the whole surface and both are attributed to the system. Where the claim fails is the item's explicit third limb -- "assert the emitted row's actor is the system principal and its uploader detail is unchanged, for BOTH the request-path prune and the background runner". Only the request-path assertion exists (tests/test_upload_api.py:794-814, which also asserts the null client). The runner-path test (tests/test_uploads.py:389) injects a list-appending stub for `audit=` and asserts `[m.file_id for m in audited]`; it never constructs or inspects an audit row, so it would pass unchanged if `_audit_upload_prune` were reverted to `actor=meta.uploader`. That closure is reachable only from the API lifespan, and no test drives a lifespan, so the fixed line has no execution coverage at all. Under the not-deployed rule this is worded conditionally: the false attribution would no longer be written on a first deployment from either path, but the path the item singled out as the one a partial fix abandons is guarded by nothing. _(was 6/10 · 2/10.)_ +> ✅ **SHIPPED 2026-08-23 -- verified by content at `8b80ef30`, tests only, no engine code changed.** Both call sites were already correct; the open limb was coverage. **THIS ROW'S OWN ACCOUNT OF THE GAP IS FALSE AT HEAD.** It says no test drives a lifespan. Tests DO construct the real closure and DO reach `run_once` -- `test_asvs_gcm_invocation_bound.py` enters a managed app's lifespan. It prunes ZERO files, so the loop body carrying `actor="system"` never executes. **A PATH THAT RUNS BUT NEVER ENTERS ITS BRANCH IS INVISIBLE TO COVERAGE-BY-EXECUTION**, which is a different defect from an unreached path and the reason the old suite could not see it. **MUTATION-VERIFIED, ANCHOR CHECKED UNIQUE FIRST so it could not land on the wrong site:** the lifespan closure re-pointed to `actor=meta.uploader` reds the new guard while the sibling request-path test stays green; restoring makes both pass. Reverting the fix previously left 13,578 tests green. 🔢 **Re-scored 2026-08-20 -> P3.** Value **2/10** · Difficulty **2/10** · _fill-in_. Attacked the shipped claim at both cited sites and it survived on the code limb. Site 1 (request-path opportunistic sweep) is now messagefoundry/api/app.py:3918-3936: `actor="system"`, `client` argument removed entirely, and detail carries file_id + uploader + uploader_id, with a comment naming BACKLOG #1224 and citing ADR 0150 decision 4 for dropping the client. Site 2 (background UploadRetentionRunner) is messagefoundry/api/app.py:5891-5907: the lifespan-owned `_audit_upload_prune` closure also writes `actor="system"` with uploader/uploader_id in detail. Enumerated prune call sites repo-wide: `prune_expired(` is called at app.py:3918 and uploads.py:648 (inside `run_once`) only, so at least those two are the whole surface and both are attributed to the system. Where the claim fails is the item's explicit third limb -- "assert the emitted row's actor is the system principal and its uploader detail is unchanged, for BOTH the request-path prune and the background runner". Only the request-path assertion exists (tests/test_upload_api.py:794-814, which also asserts the null client). The runner-path test (tests/test_uploads.py:389) injects a list-appending stub for `audit=` and asserts `[m.file_id for m in audited]`; it never constructs or inspects an audit row, so it would pass unchanged if `_audit_upload_prune` were reverted to `actor=meta.uploader`. That closure is reachable only from the API lifespan, and no test drives a lifespan, so the fixed line has no execution coverage at all. Under the not-deployed rule this is worded conditionally: the false attribution would no longer be written on a first deployment from either path, but the path the item singled out as the one a partial fix abandons is guarded by nothing. _(was 6/10 · 2/10.)_ > > **Filed 2026-08-11 -- read off `origin/main`, TWO sites not one.** The retention sweep is **automated and owner-blind**, but its audit row attributes the deletion to the pruned file's uploader: > ``` @@ -12406,7 +12406,7 @@ BUILDS it.* ## 1260. a native crash in the main suite reports as a test failure, and the crash-retry wrapper does not cover that leg -> 🔢 **Re-scored 2026-08-20 -> P1.** Value **6/10** · Difficulty **2/10** · _quick win_. Gap stands on both halves: the main suite's run line at ci.yml:780 is a bare pytest with no wrapper while every retry-native-crash invocation sits in the database legs, and the only downstream signal is steps.tests.outcome fed to step_margin.py (:801-814), which cannot distinguish exit 139 from a test failure (value 6). The remainder is two small workflow edits, wrapping the step and surfacing the crash exit distinctly, plus the must-trip and must-not-retry arms the item specifies (difficulty 2). _(was 6/10 · 2/10.)_ +> ✅ **SHIPPED 2026-08-23 -- verified by content at `16ef31ea`.** **THE ROW NAMED TWO LIMBS AND THERE WERE FOUR.** The third changed the shape of the fix: `scripts/ci/retry-native-crash.sh` HARD-CODED the pyodbc attribution into every message it emits. That class is established for the nine database call sites (`ci.yml:1644`-`:1860`) and is NOT established for the main suite -- as this row itself refuses to conclude. **Wrapping the step as written would have fixed a legibility defect by printing an unmeasured mechanism onto the busiest leg in the repo:** a true observation carrying an invented cause. The clause is now per-caller and DEFAULTS to the pyodbc text, so all nine existing sites read exactly as before and only the main suite opts out; opt-IN would have silently stripped nine correct attributions to fix one wrong one. **AND THE OPT-OUT MESSAGE NAMES NO CLASS AT ALL** -- an earlier draft said "do not assume the pyodbc class", which puts the token in the annotation so a log grep for `pyodbc` matches the ONE leg where the class is explicitly not established. Its own test caught that. **CITATION DRIFT, re-derived from the symbol: the BANNER is right and the BODY has drifted** -- `ci.yml:780` is the run line as cited, but the step is at `:728` not `:648`/`:673`, the invocations are at `1644`-`1860` not the `1156`-`1374` band, and there are NINE, not ten. **FOUR MUTATIONS, each anchor verified unique**, each reddening one arm alone. **STATED LIMIT:** a process killed by `SIGSEGV` exits `139` and `139` is what the wrapper branches on, so the tests drive the contract it implements -- they do NOT show that a real crash produces `139`, which stays the wrapper's own documented premise. 🔢 **Re-scored 2026-08-20 -> P1.** Value **6/10** · Difficulty **2/10** · _quick win_. Gap stands on both halves: the main suite's run line at ci.yml:780 is a bare pytest with no wrapper while every retry-native-crash invocation sits in the database legs, and the only downstream signal is steps.tests.outcome fed to step_margin.py (:801-814), which cannot distinguish exit 139 from a test failure (value 6). The remainder is two small workflow edits, wrapping the step and surfacing the crash exit distinctly, plus the must-trip and must-not-retry arms the item specifies (difficulty 2). _(was 6/10 · 2/10.)_ **AMENDED: THERE WERE FOUR, and the fourth is a FUTURE-READER defect rather than a runtime one** (`01c03cee`). The wrapper's own removal note read *"remove this wrapper once mkleehammer/pyodbc#1459 ships"* and named ONE call site -- the throughput-invariant step -- when `ci.yml` had TEN. **`mkleehammer/pyodbc#1459` covers the DATABASE legs and says nothing about any other caller**, so a good-faith deletion on the strength of that upstream fix would have SILENTLY STRIPPED the main suite's crash handling: the one leg whose cause is explicitly NOT established. The note now states the discriminator IN THE WORKFLOW rather than in a memory -- any caller setting `RETRY_NATIVE_CRASH_CAUSE=""` has declared its cause unproven, so `mkleehammer/pyodbc#1459` does not license removing the wrapper from it -- and `test_the_removal_note_cannot_silently_strip_an_opted_out_caller` FAILS if an opted-out caller exists while the note stops saying so. > > **Filed 2026-08-14 - not started. THE COMPENSATING CONTROL EXISTS, IS CORRECTLY WRITTEN, AND DOES NOT COVER THE PATH THAT FAILED.** Two independent halves: a **reporting** defect that misnames a crash as a test failure, and a **coverage** gap in the retry wrapper. Either can be fixed without the other, and fixing only the second leaves every future crash still misreported. > **THREE LAYERS OF NAMING SAID "TESTS FAILED" AND NOT ONE OF THEM WAS TRUE.** Observed on a pull request whose Windows leg went red: the check is named `test (windows-2025, py3.14)`, the step is named `Tests (pytest)`, and the process exited **139** -- `128 + SIGSEGV(11)` -- printing *"Segmentation fault"*. **Zero tests failed.** The reporting seat proved the log was searchable before concluding absence: `pytest` appears **28 times** in a 100 KB log while there is **no pytest summary line and no FAILED test id anywhere in it**, because the process died before it could produce one. A reader at any of those three layers reaches for a test regression that does not exist. diff --git a/scripts/ci/retry-native-crash.sh b/scripts/ci/retry-native-crash.sh index 37e976d1..68e1bbac 100644 --- a/scripts/ci/retry-native-crash.sh +++ b/scripts/ci/retry-native-crash.sh @@ -22,17 +22,49 @@ # codes below. A genuine test failure exits 1 and is re-raised immediately, never retried. # Our own Python cannot cause a native segfault — a real logic regression surfaces as a # pytest assertion (exit 1), so this wrapper can never hide one. Each retry emits a visible -# ::warning:: (grep CI logs for "Native crash" to track the flake frequency against #1459). +# ::warning:: (grep CI logs for "NATIVE CRASH" to track the flake frequency against #1459). # -# REMOVE THIS WRAPPER once #1459 ships a fix and pyproject's pyodbc floor moves to the fixed -# release (the throughput-invariant step in .github/workflows/ci.yml calls this). +# REMOVING THIS WRAPPER IS NOT A BLANKET DELETION -- READ THE CALLER SET FIRST (BACKLOG #1260). +# #1459 covers the DATABASE legs. It says nothing about any other caller, and this note previously +# named one call site ("the throughput-invariant step") when ci.yml had TEN. +# +# THE DISCRIMINATOR IS IN THE WORKFLOW, NOT IN A MEMORY: any caller setting +# RETRY_NATIVE_CRASH_CAUSE="" has declared that ITS crash cause is NOT established as the pyodbc +# class. A fix to #1459 therefore does not license removing the wrapper from that leg -- doing so +# would silently strip its crash handling on the strength of an upstream fix that does not address +# it. Today the engine test suite is such a caller. +# +# SO: when #1459 ships and pyproject's pyodbc floor moves, remove the wrapper from the pyodbc +# callers, and decide each opted-out caller SEPARATELY on its own evidence. +# `tests/test_ci_retry_native_crash.py` fails if an opted-out caller exists and this note stops +# saying so, because the whole defect is a future reader deleting one line in good faith. +# +# THE ATTRIBUTION IS PER-CALLER, AND THAT IS THE POINT (BACKLOG #1260). The pyodbc class above is +# ESTABLISHED for the database legs and is NOT established anywhere else. A wrapper that names it +# unconditionally would print a cause it has not measured onto every leg it is ever added to -- a +# true observation (a native crash happened) carrying an invented mechanism, which is the harder +# error to catch because the part a reader checks is true. So callers where the class is NOT known +# set RETRY_NATIVE_CRASH_CAUSE="" and the message says so in words. # # Usage: scripts/ci/retry-native-crash.sh [args...] # Env: RETRY_NATIVE_CRASH_ATTEMPTS (default 3) +# RETRY_NATIVE_CRASH_CAUSE attribution clause; default names the pyodbc class (correct for +# the database legs). Set to "" on any leg where it is unproven. set -uo pipefail attempts="${RETRY_NATIVE_CRASH_ATTEMPTS:-3}" +# Defaulting to the pyodbc clause keeps the nine database-leg call sites saying exactly what they +# say today; only a caller that has NOT established the class has to opt out. +default_cause=" -- likely the pyodbc py3.14 parameter-binding segfault (mkleehammer/pyodbc#1459)" +cause="${RETRY_NATIVE_CRASH_CAUSE-$default_cause}" +if [ -z "$cause" ]; then + # DELIBERATELY NAMES NO CLASS, NOT EVEN TO WARN AGAINST ONE. An earlier draft said "do not + # assume the pyodbc class" and its own test caught it: that puts the token in the annotation, + # so a log grep for pyodbc matches the ONE leg where the class is explicitly not established. + cause=" -- CAUSE NOT ESTABLISHED for this leg; do not infer the database-leg crash class" +fi + # A process killed by signal N exits with 128+N. 139 = 128+SIGSEGV(11) (the observed # segfault); 134 = 128+SIGABRT(6) (the param/TVP path can abort() with a core dump instead). is_native_crash() { @@ -51,9 +83,9 @@ for n in $(seq 1 "$attempts"); do exit "$rc" fi if [ "$n" -lt "$attempts" ]; then - echo "::warning::Native crash (exit ${rc}) on attempt ${n}/${attempts} — likely the pyodbc py3.14 parameter-binding segfault (mkleehammer/pyodbc#1459); retrying." + echo "::warning::NATIVE CRASH (exit ${rc}) on attempt ${n}/${attempts}${cause}; retrying." fi done -echo "::error::Command still crashing after ${attempts} attempts (exit ${rc}); see mkleehammer/pyodbc#1459." +echo "::error::NATIVE CRASH persisted: still crashing after ${attempts} attempts (exit ${rc})${cause}." exit "$rc" diff --git a/tests/test_ci_retry_native_crash.py b/tests/test_ci_retry_native_crash.py new file mode 100644 index 00000000..5498a64c --- /dev/null +++ b/tests/test_ci_retry_native_crash.py @@ -0,0 +1,196 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""A native crash must not report as a test failure, and a test failure must never be retried (#1260). + +THE FILED DEFECT IS A NAMING ONE. A segfault kills the interpreter, so pytest returns 139 with no +verdict -- and THREE layers of naming then say "tests failed": the check name ``test (windows-2025, +py3.14)``, the step name ``Tests (pytest)``, and ``steps.tests.outcome``, which is what +``scripts/ci/step_margin.py`` consumes. **Not one of them is true.** The engine was fine; a process +died. + +THE ORDER OF THE TWO HALVES IS LOAD-BEARING, and the item says so. Wrapping the step makes the +failure RETRY; it does not make it LEGIBLE. If the retry then succeeds, the crash vanishes from view +entirely -- so the legibility arms below are the point, and the coverage arm is what makes them +reachable from the main suite at all. + +WHY ``exit 139`` RATHER THAN A REAL SEGFAULT. A process killed by SIGSEGV exits 128+11 = 139, and +139 is precisely what the wrapper branches on. Faulting a real process would test the operating +system; these arms test the contract the wrapper actually implements, on every runner, in +milliseconds. The one thing they cannot show is that a real crash produces 139 -- that is the +wrapper's own documented premise and is not re-derived here. +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parents[1] +_WRAPPER = _ROOT / "scripts" / "ci" / "retry-native-crash.sh" +_CI = _ROOT / ".github" / "workflows" / "ci.yml" + +#: RESOLVE BASH ONCE AND INVOKE THAT EXACT BINARY. Passing the bare name "bash" to subprocess lets +#: Windows resolve it against the child's PATH, which on a machine with WSL installed finds +#: System32/bash.exe -- a DIFFERENT interpreter that cannot see a C:/... path and reports +#: the wrapper as "No such file or directory". Measured here: bare `bash` launched WSL while +#: shutil.which returned Git Bash. So a skipif on shutil.which would have guarded one interpreter +#: while the test exercised another -- a control witnessing something other than what runs. +_BASH = shutil.which("bash") + +pytestmark = pytest.mark.skipif(_BASH is None, reason="the wrapper is a bash script") + + +def _run( + exit_code: int, *, attempts: str = "3", cause: str | None = None +) -> subprocess.CompletedProcess[str]: + """Drive the REAL wrapper against a command with a known exit code, counting invocations.""" + env = dict(os.environ, RETRY_NATIVE_CRASH_ATTEMPTS=attempts) + if cause is not None: + env["RETRY_NATIVE_CRASH_CAUSE"] = cause + return subprocess.run( + # as_posix, NOT str: a backslash path reaches bash as escapes and collapses to one + # mangled word. Forward slashes survive, and _BASH pins WHICH bash reads them. + [str(_BASH), _WRAPPER.as_posix(), "bash", "-c", f"echo ran; exit {exit_code}"], + capture_output=True, + text=True, + timeout=60, + check=False, + env=env, + ) + + +# --------------------------------------------------------------------------------------------- +# THE RETRY PAIR -- it must fire on a crash and must NEVER fire on a failure +# --------------------------------------------------------------------------------------------- + + +@pytest.mark.parametrize("code", [139, 134]) +def test_a_native_crash_retries_and_says_so(code: int) -> None: + """MUST FIRE. 139 is 128+SIGSEGV and 134 is 128+SIGABRT -- the two the wrapper branches on.""" + r = _run(code, attempts="3") + assert r.stdout.count("ran") == 3, f"expected 3 attempts, got {r.stdout!r}" + assert r.returncode == code, "the crash exit must survive the retries, not be flattened" + assert "NATIVE CRASH" in r.stdout + + +def test_an_ORDINARY_FAILURE_IS_NEVER_RETRIED(exit_code: int = 1) -> None: + """MUST NOT FIRE, AND THIS IS THE ARM THAT KEEPS THE WRAPPER FROM LAUNDERING A REGRESSION. + + Without it, "retry on crash" could be implemented as "retry on anything" and every arm above + would still pass -- while a real test failure got three chances to flake green.""" + r = _run(exit_code, attempts="3") + assert r.stdout.count("ran") == 1, "a test failure must run exactly once" + assert r.returncode == 1 + assert "not a native crash" in r.stdout + assert "NATIVE CRASH" not in r.stdout + + +def test_success_passes_straight_through() -> None: + """MUST NOT FIRE. A wrapper that re-ran a passing command would triple every green leg.""" + r = _run(0, attempts="3") + assert r.stdout.count("ran") == 1 + assert r.returncode == 0 + + +# --------------------------------------------------------------------------------------------- +# THE ATTRIBUTION PAIR -- the wrapper must not assert a cause it has not measured +# --------------------------------------------------------------------------------------------- + + +def test_a_leg_with_no_established_cause_says_CAUSE_NOT_ESTABLISHED() -> None: + """MUST NOT NAME pyodbc. The class is established for the DATABASE legs and for nothing else. + + A wrapper that named it unconditionally would print a mechanism it has not measured onto every + leg it is ever added to -- a true observation (a native crash happened) carrying an invented + cause, which is the harder error to catch because the part a reader checks is true.""" + r = _run(139, attempts="1", cause="") + assert "CAUSE NOT ESTABLISHED" in r.stdout + assert "pyodbc" not in r.stdout, "an unproven attribution must not reach the annotation" + + +def test_the_database_legs_keep_their_ESTABLISHED_attribution() -> None: + """MUST NAME pyodbc -- the twin, and the reason the default is not simply blank. + + Nine call sites have that class established against an upstream issue. Making the attribution + opt-IN would have silently stripped nine correct annotations to fix one wrong one.""" + r = _run(139, attempts="1") + assert "pyodbc" in r.stdout + assert "1459" in r.stdout + assert "CAUSE NOT ESTABLISHED" not in r.stdout + + +# --------------------------------------------------------------------------------------------- +# THE COVERAGE ARM -- the gap the item was filed for +# --------------------------------------------------------------------------------------------- + + +def test_the_main_suite_step_is_wrapped() -> None: + """THE FILED GAP. Every retry-native-crash invocation sat in the database legs; the main suite's + run line was a bare pytest, so the one leg most people read could not tell a crash from a + failure.""" + text = _CI.read_text(encoding="utf-8") + step = text.split("- name: Tests (pytest)", 1) + assert len(step) == 2, "the Tests (pytest) step is gone -- re-derive this from the workflow" + body = step[1].split("- name:", 1)[0] + assert "retry-native-crash.sh" in body, ( + "the main suite's run line is unwrapped again; a native crash there reports as a test failure" + ) + + +def test_the_main_suite_step_declares_its_cause_unproven() -> None: + """The coverage arm alone would let the pyodbc clause reach a leg where it is not established -- + which would fix the legibility defect by introducing an attribution one.""" + text = _CI.read_text(encoding="utf-8") + body = text.split("- name: Tests (pytest)", 1)[1].split("- name:", 1)[0] + assert 'RETRY_NATIVE_CRASH_CAUSE: ""' in body + + +def test_the_wrappers_own_grep_hint_matches_what_it_prints() -> None: + """The header tells a reader which string to grep CI logs for. If the annotation's wording drifts + from that hint, the documented search returns nothing and reads as 'no crashes ever happened' -- + the same silent-zero shape the surrounding items are about.""" + src = _WRAPPER.read_text(encoding="utf-8") + hint = [ln for ln in src.splitlines() if "grep CI logs for" in ln] + assert len(hint) == 1, hint + token = hint[0].split('"')[1] + assert f"::warning::{token}" in src or f"::warning::{token}" in src.replace( + "::warning::", "::warning::" + ) + assert token in src.split("is_native_crash", 1)[1], ( + "the hint names a string the messages do not print" + ) + + +def test_the_removal_note_cannot_silently_strip_an_opted_out_caller() -> None: + """THE DEFECT NEITHER DESIGN ADDRESSED, and it is a future-reader defect rather than a runtime one. + + The wrapper's header instructs whoever fixes upstream pyodbc #1459 to remove it. That is correct + for the database legs and WRONG for any caller whose crash cause is not that class -- and the + note previously named ONE call site while ci.yml had ten. + + So the failure mode is somebody deleting one line in good faith, on the strength of an upstream + fix that does not address the leg they are stripping. Nothing at runtime can catch that; the + only guard is that the instruction stays honest about its own caller set. + + THE PREDICATE IS THE WORKFLOW, NOT A HARDCODED NAME: if any step opts out of the pyodbc + attribution, the removal note must say removal is caller-dependent. Add such a caller without + updating the note and this reds.""" + ci = _CI.read_text(encoding="utf-8") + src = _WRAPPER.read_text(encoding="utf-8") + opted_out = 'RETRY_NATIVE_CRASH_CAUSE: ""' in ci + assert opted_out, ( + "no caller opts out any more -- if that is deliberate this arm is now vacuous and should be " + "deleted with a reason, not left passing over nothing" + ) + note = src.split("REMOVING THIS WRAPPER", 1) + assert len(note) == 2, ( + "the removal instruction no longer warns that removal depends on the caller set; a reader " + "fixing #1459 would strip the opted-out leg's crash handling silently (BACKLOG #1260)" + ) + assert "RETRY_NATIVE_CRASH_CAUSE" in note[1].split("set -uo", 1)[0], ( + "the note must name the DISCRIMINATOR a reader can check, not just caution them in prose" + ) diff --git a/tests/test_upload_api.py b/tests/test_upload_api.py index 964eead8..c40cc2b0 100644 --- a/tests/test_upload_api.py +++ b/tests/test_upload_api.py @@ -7,6 +7,7 @@ from __future__ import annotations +import asyncio import json from collections.abc import AsyncIterator from pathlib import Path @@ -870,3 +871,90 @@ async def test_uploaded_browse_moves_the_needle_into_a_post_body( browse = [a for a in await engine.store.list_audit() if a["action"] == "upload.browse"] joined = " ".join(str(a["detail"] or "") for a in browse) assert "MRN123" not in joined # the needle value is still never audited + + +async def test_the_RUNNER_prune_audit_row_also_names_the_system(tmp_path: Path) -> None: + """BACKLOG #1224, THE SECOND SITE -- the one the suite could not see. + + Two code paths prune expired uploads and BOTH audit as ``actor="system"``: the save-time sweep + inside ``upload_file`` (guarded by the sibling test above) and the background + ``UploadRetentionRunner`` wired in ``create_managed_app``'s lifespan. Only the first had a test. + + **MEASURED, WHICH IS WHY THIS EXISTS.** Reverting the lifespan closure to + ``actor=meta.uploader`` left the ENTIRE suite green -- 13578 passed, byte-identical to baseline -- + while the same mutation on the request-path site reddened exactly one test. So the runner's + attribution was unguarded, and the item's own scope names a test for it as the open limb. + + **WHY THE SUITE MISSED IT, and it is not that nothing drives a lifespan.** Tests DO construct the + real closure and DO reach ``run_once`` -- ``tests/test_asvs_gcm_invocation_bound.py`` enters a + managed app's lifespan. It prunes ZERO files, because nothing in those fixtures is aged, so the + loop body carrying ``actor="system"`` never executes. **A path that runs but never enters its + branch is invisible to coverage-by-execution**, which is the same green-and-blind shape the + surrounding items are about. + + **TWO LIFESPANS ARE LOAD-BEARING, not incidental.** ``_run`` calls ``run_once`` BEFORE its first + sleep, so the sweep happens at startup. The file therefore has to be aged BETWEEN two startups: + the first app creates it (the runner's sweep has already passed), the second app's sweep finds it + expired. One lifespan cannot both create and prune. + """ + import dataclasses + import time + + pytest.importorskip("psutil") + from messagefoundry.api import create_managed_app + from messagefoundry.store.crypto import generate_key + + uploads = tmp_path / "runner-uploads" + settings = StoreSettings( + path=str(tmp_path / "runner.db"), + encryption_key=generate_key(), + uploads_dir=str(uploads), + max_upload_bytes=1_000_000, + uploads_retention_days=30, + ) + + # PASS 1 -- create the file, then age it past the window. The runner's startup sweep for THIS app + # already ran before the file existed, so nothing is pruned here. + app1 = create_managed_app(store_settings=settings, poll_interval=0.05) + async with app1.router.lifespan_context(app1): + us = app1.state.upload_store + assert us is not None, "[store].uploads_dir was set -- the subsystem must be wired" + meta = await us.save( + data=BATCH.encode(), filename="acme.hl7", uploader="op", uploader_id="u-1" + ) + aged = dataclasses.replace(meta, uploaded_at=time.time() - 31 * 86_400) + (uploads / f"{meta.file_id}.meta").write_text( + us._encrypt_meta(aged), # noqa: SLF001 -- mirrors the sibling test's backdating + encoding="utf-8", + ) + assert await app1.state.engine.store.list_audit() is not None + + # PASS 2 -- a fresh app over the SAME db and uploads dir. Its startup sweep finds the aged file + # and calls the REAL _audit_upload_prune closure. + app2 = create_managed_app(store_settings=settings, poll_interval=0.05) + rows: list[dict[str, object]] = [] + async with app2.router.lifespan_context(app2): + # The sweep runs in a task the lifespan does not await, so poll rather than sleeping a fixed + # amount -- a fixed sleep is either flaky or slow, and this is neither. + for _ in range(200): + rows = [ + a + for a in await app2.state.engine.store.list_audit() + if a["action"] == "upload.prune" + ] + if rows: + break + await asyncio.sleep(0.02) + + assert len(rows) == 1, ( + f"the runner's sweep should have pruned exactly the aged file, got {rows}" + ) + assert rows[0]["actor"] == "system", ( + "the retention runner has no operator and no request behind it; naming the pruned file's " + "uploader attributes an automated deletion to someone who did not perform it" + ) + detail = json.loads(str(rows[0]["detail"])) + assert detail["uploader"] == "op", ( + "the owner must survive as DATA in detail -- dropping it would trade a false attribution " + "for an unreadable row" + ) diff --git a/tests/tooling_manifest.txt b/tests/tooling_manifest.txt index c74a3795..38371023 100644 --- a/tests/tooling_manifest.txt +++ b/tests/tooling_manifest.txt @@ -39,6 +39,7 @@ tests/test_blanket_stage_guard.py tests/test_ci_docs_only_detector.py tests/test_ci_engine_step_excludes_webconsole.py tests/test_ci_leg_data_class.py +tests/test_ci_retry_native_crash.py tests/test_ci_step_margin.py tests/test_ci_tooling_gate.py tests/test_ci_venv_pinning.py