Skip to content

test(runtime): make the store atomicity fakes fail at sqlite's commit - #1472

Open
doublewhy wants to merge 3 commits into
devfrom
store-commit-failure-fakes
Open

doublewhy wants to merge 3 commits into
devfrom
store-commit-failure-fakes

Conversation

@doublewhy

@doublewhy doublewhy commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Part of #1344, the fixture-provenance audit (finding ST-2 in the record #1461 proposes). Two tests that claim to cover a failed store commit never reach a commit. Each wraps a store write, runs the real write, then raises OSError("commit failed") inside the transaction body:

  • test_failed_atomic_control_commit_exposes_no_partial_transition (test_run_310_supervisory_lifecycle.py:865-874 on dev) wraps _upsert_record. Since feat(runtime): define control-plane operation lifecycle contract #1235, record_participant_control claims a running record (participant_control_mediation.py:112) before it calls _commit_control_transition (:115). The first _upsert_record call is now that claim, so the test fails the claim and never reaches the atomic control commit it names.
  • test_local_control_plane_store_rolls_back_snapshot_transaction_failure (test_runtime_control_plane_api.py:1864-1887) wraps _upsert_snapshot. It also saves RuntimeSnapshot() and compares only the snapshot, and an empty snapshot equals the default one, so even a committed write would pass (M3 and M5 below).

An error raised in the body takes the except branch of transaction() (control_plane_store_local_codec.py:13-24), which calls rollback(). A real commit failure is a sqlite3 error raised by commit() in the else branch. transaction() re-raises it without a rollback, and _connection() (control_plane_store_local.py:358-363) closes the connection, which discards the open transaction. The audit recorded this on SQLite 3.50.4 with a deferred foreign key: commit() raised sqlite3.IntegrityError, the transaction stayed open, and nothing persisted after close().

Both fakes now fail where production fails. A small helper adds a deferred foreign-key violation to the open transaction, in a temporary table local to the connection, so SQLite's own COMMIT raises sqlite3.IntegrityError: FOREIGN KEY constraint failed. The supervisory fake does this after the control transition's last write, its audit row, so the snapshot, record and audit are all pending when COMMIT fails. No production code changes.

Requirement UIDs

  • None. Test-only change with no requirement scope.

Related Issues

Closes #1559

ADR Impact

  • None.

Changes

  • implementations/python/tests/store_commit_failure_fixtures.py (new): the helper fail_sqlite_commit(connection). It creates a temporary table with a deferred self-referencing foreign key and inserts a row whose parent does not exist. The module only defines that function, so importing it has no side effects.
  • implementations/python/tests/test_run_310_supervisory_lifecycle.py: test_failed_atomic_control_commit_exposes_no_partial_transition wraps _insert_audit instead of _upsert_record and expects sqlite3.IntegrityError. The running claim is durable when the transition commit fails, and the runtime cannot pair it with a terminal cut (control_plane_durability.py:145-169). So it serves no state until restart, and the test asserts that (get_snapshot() raises requires restart) instead of reading an empty history. The restarted store holds no control history, no audit event and one record, the claim, still RUNNING. The restart requirement is the documented contract:
    • A RUNNING operation left by an uncertain commit stays claimed for governed recovery or fail-closed handling (docs/decisions/issue-1181-unified-control-plane-mutations-preflight.md:137-140). After the claim and before the terminal commit, one durable RUNNING record remains, and an injected terminal-transaction failure rolls back the whole terminal cut (:342-344).
    • A runtime whose durable reload fails is poisoned and rejects every new call until it is closed and restarted (docs/decisions/issue-1092-local-control-plane-durability-preflight.md:262-263). _resynchronize_after_store_error poisons the runtime the same way when the reloaded record lacks the terminal cut the commit required (control_plane_durability.py:163-173).
    • test_runtime_poisoned_by_non_terminal_operation_after_uncertain_terminal_commit already asserts this after a failed provisioning terminal commit (implementations/python/tests/test_issue_1092_control_plane_crash_consistency.py:808). The opposite case, a transition commit that lands and then reports an error, resyncs without poisoning (:2179).
  • implementations/python/tests/test_runtime_control_plane_api.py: test_local_control_plane_store_rolls_back_snapshot_transaction_failure keeps wrapping _upsert_snapshot, now with the commit failure, and expects sqlite3.IntegrityError. It saves RuntimeSnapshot(metadata={"must": "roll back"}) and asserts that the whole SnapshotState, snapshot and revision, is unchanged.

Commits: 21fcf57c changes both fakes. 4d79a194 builds that snapshot before the pytest.raises block so that only the save runs inside it; SonarCloud reported python:S5778 on 21fcf57c, where the constructor call sat inside the block. 6058bbe7 moves the helper, which both test modules defined identically, into the shared module and imports it from both.

Test Plan

  • Unit tests pass
  • Integration tests pass if applicable
  • Full completion suite required in CI before merge
  • No coverage regression

On 6058bbe7, from implementations/python, uv run --frozen --all-extras python -m pytest tests/test_run_310_supervisory_lifecycle.py tests/test_runtime_control_plane_api.py -q -p no:cacheprovider passed (104 passed). With -m integration and with -m fuzz, both modules select no tests (104 deselected each). Local versions: SQLite 3.50.4, CPython 3.14.4.

Mutation runs, repeated on 6058bbe7 with M3 added (the other rows did not change). A pytest plugin outside the repository patched each broken variant in at runtime; no broken code was written to the package. The old fakes come from dev's copies of the two test modules; the 1092 column is test_startup_recovery_commit_failure_aborts_readiness_and_resumes_without_record_only_writes in test_issue_1092_control_plane_crash_consistency.py, unchanged:

Variant supervisory, old fake supervisory, new fake store, old fake store, new fake 1092 recovery crash
production pass pass pass pass pass
M1: transaction() rolls back a failed COMMIT but does not re-raise pass fail pass fail pass
M2: the control transition commits its snapshot before its record and audit pass fail pass pass pass
M3: transaction() does nothing, so every statement commits on its own fail fail pass fail fail
M5: transaction() commits instead of rolling back when the body raises fail pass pass pass fail
M4: transaction() drops its rollback when the body raises pass pass pass pass pass
  • M1 is the broken rollback the old fakes cannot see. With it, record_participant_control returns an accepted receipt while the store keeps only the running claim, and save_snapshot returns normally. Both new tests fail with DID NOT RAISE IntegrityError.
  • M2 breaks the transition's atomicity. The new supervisory test fails on assert not restarted.load_snapshot().participant_control_history, because the restarted store holds the history event while the record is still the running claim and no audit exists.
  • M3 drops the transaction, so each write commits as soon as it runs. The old store test passes, because the empty snapshot it saves equals the default one. The new store test fails at assert store.load_snapshot_state() == before (test_runtime_control_plane_api.py:1888): the snapshot write committed before the failing statement, so the store holds revision 1 instead of 0. The supervisory test fails with either fake, and the 1092 recovery-crash test fails too. With the new fake, the transition's snapshot, SUCCEEDED record and audit all persist, so get_snapshot() serves state instead of raising requires restart (test_run_310_supervisory_lifecycle.py:885-886).
  • M5 never enters the commit branch, so the new fakes pass. The 1092 recovery-crash test still catches it, so the suite keeps that coverage. The old store test passes M5 too, because of the empty-snapshot comparison.
  • M4 passes everywhere: _connection() closes the connection after each operation and SQLite discards the open transaction. This matches the audit's conclusion that atomicity holds on real SQLite because of that close.
Mutation plugin (run with PYTHONPATH=<dir> ST2_MUTATION=M1 ... pytest -p st2_mutations)
import os
import sqlite3
from contextlib import contextmanager

import pytest
from raes_runtime import (
    control_plane_store_local,
    control_plane_store_local_codec,
    control_plane_store_local_records,
    control_plane_store_local_snapshot,
    control_plane_store_maintenance,
)
from raes_runtime.control_plane_store_local import LocalControlPlaneStore

_TRANSACTION_NAMES = (
    (control_plane_store_local_codec, "transaction"),
    (control_plane_store_local, "_transaction"),
    (control_plane_store_local_records, "_transaction"),
    (control_plane_store_local_snapshot, "transaction"),
    (control_plane_store_maintenance, "transaction"),
)


@contextmanager
def _m1_swallows_failed_commit(connection):
    connection.execute("BEGIN IMMEDIATE")
    try:
        yield
    except BaseException:
        connection.rollback()
        raise
    else:
        try:
            connection.commit()
        except sqlite3.Error:
            connection.rollback()


@contextmanager
def _m3_no_transaction(connection):
    yield


@contextmanager
def _m4_no_rollback(connection):
    connection.execute("BEGIN IMMEDIATE")
    try:
        yield
    except BaseException:
        raise
    else:
        connection.commit()


@contextmanager
def _m5_commits_on_failure(connection):
    connection.execute("BEGIN IMMEDIATE")
    try:
        yield
    except BaseException:
        connection.commit()
        raise
    else:
        connection.commit()


_TRANSACTIONS = {
    "M1": _m1_swallows_failed_commit,
    "M3": _m3_no_transaction,
    "M4": _m4_no_rollback,
    "M5": _m5_commits_on_failure,
}


@pytest.fixture(autouse=True)
def _apply_mutation(monkeypatch):
    mutation = os.environ.get("ST2_MUTATION", "none")
    if mutation in _TRANSACTIONS:
        for module, name in _TRANSACTION_NAMES:
            monkeypatch.setattr(module, name, _TRANSACTIONS[mutation])
    elif mutation == "M2":
        real_upsert_record = LocalControlPlaneStore._upsert_record

        def upsert_record_after_committing_snapshot(connection, record):
            connection.commit()
            connection.execute("BEGIN IMMEDIATE")
            real_upsert_record(connection, record)

        monkeypatch.setattr(LocalControlPlaneStore, "_upsert_record", staticmethod(upsert_record_after_committing_snapshot))
    elif mutation != "none":
        raise AssertionError(f"unknown mutation {mutation}")
    yield

A trace on dev, with a plugin that logs each store write, shows which call the old supervisory fake interrupts: the call stack at the failing write is record_participant_control, _claim_record, claim_record, claim_record, fail_record_upsert.

On 6058bbe7, nox -s verify-fast-feedback -- --base-rev origin/dev passed: every stage PASS or SKIP, and the two changed test modules ran (104 passed). nox -s lint passed. Run from implementations/python, the pinned tooling ruff format left the three changed files unchanged and ruff check passed. make policy passed 15 stages. Requirement governance was skipped because the branch has no requirement UID and no scope file.

No package code changes and the PR only edits tests, so package line coverage cannot drop. The CI coverage lane is authoritative.

On 6058bbe7, CI run 38051689522 passed and all 14 required checks pass (28 checks pass, deploy skipped). SonarCloud's quality gate is OK with no new issues.

Ground Control Checks

  • Repository policy command passes
  • Each new fake fails on the broken commit-failure handling (M1, and M2 for the transition) that passes with the old fake, and the new store test also fails on M3, which the old one passes, per the mutation runs above

make policy and the test runs are recorded in the Test Plan.

Traceability

  • IMPLEMENTS: (none)
  • TESTS: implementations/python/tests/test_run_310_supervisory_lifecycle.py, implementations/python/tests/test_runtime_control_plane_api.py, with the shared helper in implementations/python/tests/store_commit_failure_fixtures.py
  • Covered code: implementations/python/packages/raes_runtime/control_plane_store_local_codec.py (transaction), control_plane_store_local.py (_connection, commit_participant_transition), control_plane_store_local_snapshot.py (save_snapshot)
  • Already traced, and no traceability lines change: API-404 lists the codec module as IMPLEMENTS (docs/requirements/API-404/requirement.md:291) and both test modules as TESTS (:269, :318). RUN-310 traces the supervisory module (docs/requirements/RUN-310/requirement.md:72).

Checklist

  • Code follows the project coding standards
  • FM: not applicable, no semantic change (two tests and one shared test helper); no executable or runtime adoption is claimed.
  • No production module, published schema, research capture or evidence-bound file changes.
  • PR title is a Conventional Commit; test does not cut a release.

Both fakes raised OSError inside the transaction body after a real
upsert, so they exercised the rollback branch of transaction(). A real
commit failure is a sqlite3 error raised by commit() itself, which
transaction() re-raises without a rollback; the per-operation
connection close then discards the open transaction. The supervisory
fake also failed the running claim that #1235 put ahead of the control
transition, so it never reached the atomic commit it names.

Add a deferred foreign-key violation to the transaction so SQLite's
own COMMIT fails with sqlite3.IntegrityError: after the control
transition's last write in the supervisory test, and after the
snapshot upsert in the store test. The store test now saves a
non-empty snapshot and compares the whole snapshot state, because an
empty snapshot equals the default and hid a committed write.
Keep a single call inside pytest.raises, as SonarCloud rule S5778
requires: construct the snapshot first, as the test did before.
Both store atomicity tests defined the same fake. Move it to
tests/store_commit_failure_fixtures.py, a support module with no import
side effects, and import it from both modules.
@doublewhy
doublewhy marked this pull request as ready for review October 10, 2026 14:09
doublewhy added a commit that referenced this pull request Oct 12, 2026
…ter entry

Each open entry in the divergence register now names its issue: #1525
to #1557 for the production changes, and #1521 for the smaller
corrections. The entries that open pull requests now address move to
"Addressed by pull requests" with their issues: GH-7 (#1491, #1490),
NS-1 (#1476, #1475), NS-2 (#1480, #1479), MF-2 (#1483, #1482), CL-2
(#1474, #1473), ST-2 (#1472, #1559) and NS-7 and NS-8 (#1488, #1560
and #1561). ST-1 names #1558, GH-3 records that #1415 merged, and
"What remains" names #1522 for the unverified and unexamined
boundaries.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant