From 991fa60fe028e62ef774d9dca4aadf586ac6382e Mon Sep 17 00:00:00 2001
From: sushant-mishra-dtu <sushant.arh@gmail.com>
Date: Sun, 6 Sep 2026 10:33:51 +0530
Subject: [PATCH] =?UTF-8?q?fix(storage):=20close=20the=20connection=20when?=
 =?UTF-8?q?=20close()'s=20commit=20fails=20=F0=9F=A4=96=F0=9F=A4=96?=
 =?UTF-8?q?=F0=9F=A4=96?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

SQLiteStorageManager.close() sets self._closed = True before it commits,
then commits and closes inside the same statement pair:

    self._closed = True
    ...
    conn.commit()
    conn.close()

If commit() raises -- a full disk, an I/O error, a locked database --
conn.close() is never reached. The connection is left open, and because
_closed is already True the guard at the top of close() makes every
later close() a no-op, so nothing can ever close it:

    close() raised     : disk I/O error
    manager._closed    : True
    conn.close() called: False
    after retry close(): False

__exit__ calls close(), so a `with SQLiteStorageManager(...)` block that
fails to commit on the way out leaks its connection and the file handle
under it, and the caller's own recovery close() cannot reclaim it.

Commit in a try / finally so the close always runs. The commit error
still propagates, unchanged.

🤖🤖🤖

Signed-off-by: sushant-mishra-dtu <sushant.arh@gmail.com>
---
 src/nooa/storage/sqlite.py         | 10 +++++++---
 tests/unit/test_util_and_sqlite.py | 22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/nooa/storage/sqlite.py b/src/nooa/storage/sqlite.py
index ba6d5db75..024e89769 100644
--- a/src/nooa/storage/sqlite.py
+++ b/src/nooa/storage/sqlite.py
@@ -866,11 +866,15 @@ def close(self) -> None:
             if conn is not None:
                 if lock is not None:
                     with lock:
+                        try:
+                            conn.commit()
+                        finally:
+                            conn.close()
+                else:
+                    try:
                         conn.commit()
+                    finally:
                         conn.close()
-                else:
-                    conn.commit()
-                    conn.close()
         finally:
             if self._lock_fd is not None:
                 fcntl.flock(self._lock_fd, fcntl.LOCK_UN)
diff --git a/tests/unit/test_util_and_sqlite.py b/tests/unit/test_util_and_sqlite.py
index 0490cb361..124d97820 100644
--- a/tests/unit/test_util_and_sqlite.py
+++ b/tests/unit/test_util_and_sqlite.py
@@ -654,6 +654,28 @@ def test_backend_uses_insertion_counter(self):
         assert backend._insertion_counter == initial + 2
         sm.close()
 
+    def test_close_closes_the_connection_when_commit_fails(self):
+        """A raising commit() must not leak the connection.
+
+        close() sets _closed before committing, so a connection missed here can
+        never be closed by a later close() call -- it early-returns.
+        """
+        import sqlite3
+
+        from nooa.storage.sqlite import SQLiteStorageManager
+
+        sm = SQLiteStorageManager(":memory:")
+        real_conn = sm._conn
+        try:
+            fake_conn = MagicMock(spec=sqlite3.Connection)
+            fake_conn.commit.side_effect = sqlite3.OperationalError("disk I/O error")
+            sm._conn = fake_conn
+            with pytest.raises(sqlite3.OperationalError):
+                sm.close()
+            assert fake_conn.close.called
+        finally:
+            real_conn.close()
+
     def test_restore_snapshot_not_found_raises(self):
         from nooa.errors.storage import SnapshotNotFoundError
         from nooa.storage.sqlite import SQLiteStorageManager