From 3f047bbad104e9d1e321c4823d2272eaed7160ca Mon Sep 17 00:00:00 2001 From: npeter83 Date: Tue, 18 Aug 2026 23:46:48 +0200 Subject: [PATCH] refactor(config): config/state mutators flush, callers commit (E3 S3, B-core-10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restores CLAUDE.md's canonical transaction rule ("the handler commits; helpers flush; only quota owns its session") on the config path: sysconfig.set_value / sysconfig.reset and state.set_sync_paused now flush instead of committing, so a change lands in ONE transaction with the audit row the route records for it — previously the helper's internal commit split them, and a crash between left the mutation persisted with no audit trail. All callers already commit (the config PATCH/DELETE + audit, the sync pause/resume + audit, the scheduler maintenance PATCH); the setup wizard, which relied on the internal commit, gets an explicit db.commit() after its loop. apply_interval is deliberately left committing — it performs a live APScheduler reschedule that must follow the DB persist, so it owns its own unit of work (a legitimate exception, like the download service transitions). New test locks the flush-not-commit contract (a rollback before the caller commits discards the change). 368 pass. --- backend/app/routes/setup.py | 1 + backend/app/state.py | 4 +- backend/app/sysconfig.py | 11 ++++-- backend/tests/test_config_commit_ownership.py | 38 +++++++++++++++++++ 4 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 backend/tests/test_config_commit_ownership.py diff --git a/backend/app/routes/setup.py b/backend/app/routes/setup.py index baca1135..9add89a0 100644 --- a/backend/app/routes/setup.py +++ b/backend/app/routes/setup.py @@ -96,6 +96,7 @@ def setup_config(settings_map: dict[str, Any], db: Session = Depends(require_set except (ValueError, RuntimeError) as exc: raise HTTPException(status_code=400, detail=str(exc)) saved.append(key) + db.commit() # sysconfig.set_value only flushes now — persist the wizard's config in one commit return {"saved": saved} diff --git a/backend/app/state.py b/backend/app/state.py index 3664783c..28dd3586 100644 --- a/backend/app/state.py +++ b/backend/app/state.py @@ -28,10 +28,12 @@ def is_sync_paused(db: Session) -> bool: def set_sync_paused(db: Session, paused: bool) -> None: + # Flushes, doesn't commit — the caller owns the transaction so the change lands with its audit + # row in one transaction (B-core-10). row = _row(db) row.sync_paused = paused db.add(row) - db.commit() + db.flush() log.info("Background sync %s", "paused" if paused else "resumed") diff --git a/backend/app/sysconfig.py b/backend/app/sysconfig.py index de23e9ee..13c25d2f 100644 --- a/backend/app/sysconfig.py +++ b/backend/app/sysconfig.py @@ -186,7 +186,9 @@ def get_bool(db: Session, key: str) -> bool: def set_value(db: Session, key: str, value) -> None: - """Validate, coerce, and persist a DB override for `key`. Secrets are encrypted.""" + """Validate, coerce, and stage a DB override for `key` (secrets are encrypted). Flushes but does + NOT commit — the caller owns the transaction, so the change lands together with its audit row + (B-core-10). The staged value is visible to a same-session `get()` before the commit.""" s = _BY_KEY[key] if s.type == "int": v = int(value) @@ -212,15 +214,16 @@ def set_value(db: Session, key: str, value) -> None: db.add(row) row.value = stored row.encrypted = encrypted - db.commit() + db.flush() def reset(db: Session, key: str) -> None: - """Remove the DB override for `key` (fall back to the env/config default).""" + """Remove the DB override for `key` (fall back to the env/config default). Flushes but does NOT + commit — the caller owns the transaction (B-core-10).""" row = db.get(SystemConfig, key) if row is not None: db.delete(row) - db.commit() + db.flush() def describe(db: Session) -> dict: diff --git a/backend/tests/test_config_commit_ownership.py b/backend/tests/test_config_commit_ownership.py new file mode 100644 index 00000000..0cfbb814 --- /dev/null +++ b/backend/tests/test_config_commit_ownership.py @@ -0,0 +1,38 @@ +"""E3 S3 (B-core-10): the config/state mutators flush but do NOT self-commit, so the caller owns the +transaction and a change lands together with its audit row. These lock that contract — a rollback +before the caller commits discards the change (proving no hidden internal commit), while a same-session +read still sees it (flush visibility). DB lane (`db` fixture) — skipped without Postgres. +""" +from app import state, sysconfig + + +def test_set_value_flushes_but_leaves_the_commit_to_the_caller(db): + default = sysconfig.get_int(db, "quota_daily_budget") + new = default + 4321 + + sysconfig.set_value(db, "quota_daily_budget", new) + assert sysconfig.get_int(db, "quota_daily_budget") == new # visible in-session (flushed) + + db.rollback() # the caller never committed + assert sysconfig.get_int(db, "quota_daily_budget") == default # discarded — no hidden self-commit + + +def test_reset_flushes_but_leaves_the_commit_to_the_caller(db): + default = sysconfig.get_int(db, "quota_daily_budget") + sysconfig.set_value(db, "quota_daily_budget", default + 10) + db.commit() # a persisted override to reset + + sysconfig.reset(db, "quota_daily_budget") + assert sysconfig.get_int(db, "quota_daily_budget") == default # reset visible in-session + db.rollback() + assert sysconfig.get_int(db, "quota_daily_budget") == default + 10 # reset discarded until committed + + +def test_set_sync_paused_flushes_but_leaves_the_commit_to_the_caller(db): + assert state.is_sync_paused(db) is False + + state.set_sync_paused(db, True) + assert state.is_sync_paused(db) is True # visible in-session + + db.rollback() + assert state.is_sync_paused(db) is False # discarded — the caller owns the commit