diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index c89e697c98d29..6dceaaf678eb0 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -1473,12 +1473,22 @@ def write_txn(conn: sqlite3.Connection): Use for any multi-statement write (creating a task + link, claiming a task + recording an event, etc.). A claim CAS inside this context is atomic -- at most one concurrent writer can succeed. + + The explicit ROLLBACK on exception is wrapped in try/except so that + a SQLite auto-rollback (which leaves no active transaction) does not + shadow the original exception with a spurious rollback error. """ conn.execute("BEGIN IMMEDIATE") try: yield conn except Exception: - conn.execute("ROLLBACK") + try: + conn.execute("ROLLBACK") + except sqlite3.OperationalError: + # SQLite has already auto-rolled-back the transaction (typical + # under EIO, lock contention, or corruption). Nothing to undo; + # do not let this secondary failure shadow the real one. + pass raise else: conn.execute("COMMIT") diff --git a/scripts/release.py b/scripts/release.py index dc7d6c4fe5e21..f10cff2c0705c 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -1284,6 +1284,7 @@ "erik.engervall@gmail.com": "erikengervall", # PR #28774 (firecrawl integration tag) "egilewski@egilewski.com": "egilewski", # PR #30432 (MEDIA path traversal fix, GHSA-jmf9-9729-7pp8) "edison@mcclean.codes": "McClean-Edison", # PR #29817 (register_auxiliary_task plugin API) + "steveonjava@gmail.com": "steveonjava", # PR #31310 (write_txn rollback guard) "zhangsamuel12@gmail.com": "SamuelZ12", # PR #7480 (show recap after in-session resume) "490408354@qq.com": "daizhonggeng", # PR #9020 (numbered /resume selection) } diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index 883cf8f4d5db0..9a207ff8cb0cb 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/tests/hermes_cli/test_kanban_db.py @@ -3339,3 +3339,61 @@ def test_maybe_emit_scratch_tip_skips_non_scratch_workspaces(kanban_home, caplog ).fetchall() assert "tip_scratch_workspace" not in [e["kind"] for e in events] + +# write_txn — rollback handler must not mask the original exception +# --------------------------------------------------------------------------- + + +def test_write_txn_preserves_original_exception_when_rollback_fails(kanban_home): + """When a write inside write_txn raises an OperationalError that SQLite + has already auto-rolled-back (e.g. ``disk I/O error``, + ``database is locked``, ``database disk image is malformed``), the + explicit ROLLBACK in ``write_txn.__exit__`` itself raises + ``cannot rollback - no transaction is active``. The original cause + must NOT be masked by the secondary rollback failure — operators rely + on the original cause to diagnose the underlying issue. + """ + + class FailingConnWrapper: + """Delegate to a real connection, simulating an EIO during an INSERT + that SQLite has already auto-rolled-back.""" + + def __init__(self, real): + self._real = real + self._fail_armed = True + + def execute(self, sql, *args, **kwargs): + if ( + self._fail_armed + and sql.lstrip().upper().startswith("INSERT") + and "task_events" in sql.lower() + ): + self._fail_armed = False # one-shot + # Simulate SQLite auto-rolling back the transaction by + # issuing a real ROLLBACK now. After this, BEGIN IMMEDIATE + # is no longer active and an explicit ROLLBACK would error. + try: + self._real.execute("ROLLBACK") + except sqlite3.OperationalError: + pass + raise sqlite3.OperationalError("disk I/O error") + return self._real.execute(sql, *args, **kwargs) + + def __getattr__(self, name): + return getattr(self._real, name) + + with kb.connect() as conn: + wrapper = FailingConnWrapper(conn) + with pytest.raises(sqlite3.OperationalError) as excinfo: + with kb.write_txn(wrapper): + kb._append_event(wrapper, "t_bogus", "promoted", None) + + msg = str(excinfo.value) + assert "disk I/O error" in msg, ( + f"write_txn masked the original exception with rollback failure; " + f"got {msg!r} (expected to contain 'disk I/O error')" + ) + assert "cannot rollback" not in msg, ( + f"write_txn surfaced the rollback failure instead of the original " + f"OperationalError; got {msg!r}" + )