fix(storage): close the connection when close()'s commit fails 🤖🤖🤖

[sushant-mishra-dtu]

What this fixes

SQLiteStorageManager.close() marks itself closed before it does the closing
(src/nooa/storage/sqlite.py:859-878):

def close(self) -> None:
    if self._closed:
        return
    self._closed = True
    try:
        ...
        if conn is not None:
            if lock is not None:
                with lock:
                    conn.commit()      # if this raises...
                    conn.close()       # ...this never runs
            else:
                conn.commit()
                conn.close()
    finally:
        ...

sqlite3.Connection.commit() raises on a full disk, an I/O error, or a database another
process holds. When it does, conn.close() is skipped and the connection is left open --
and because self._closed is already True, the guard on the first line turns every
later close() into a no-op. Nothing can ever close it.

Why it matters

__exit__ is just self.close() (src/nooa/storage/sqlite.py:884), so this is reached by
the ordinary with SQLiteStorageManager(...) as sm: form. A block that fails to commit on
the way out leaks the connection and the file handle beneath it, and the caller's own
recovery path cannot reclaim them -- sm.close() in an except/finally returns
immediately, reporting success.

The finally: block does release the advisory session lock, so this is not a deadlock. It
is a connection and file-descriptor leak with no recovery, in exactly the situation where a
caller is most likely to be retrying.

Reproduction

sm = SQLiteStorageManager(":memory:")
fake = MagicMock(spec=sqlite3.Connection)
fake.commit.side_effect = sqlite3.OperationalError("disk I/O error")
sm._conn = fake

try:
    sm.close()
except sqlite3.OperationalError as e:
    print("close() raised     :", e)
print("manager._closed    :", sm._closed)
print("conn.close() called:", fake.close.called)
sm.close()   # a caller's error-recovery retry
print("after retry close():", fake.close.called)

On main:

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

On this branch:

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

The fix

Commit in a try / finally so the close always runs, on both the locked and unlocked
paths:

if lock is not None:
    with lock:
        try:
            conn.commit()
        finally:
            conn.close()
else:
    try:
        conn.commit()
    finally:
        conn.close()

The commit error still propagates unchanged -- the caller learns the commit failed, and now
the connection is closed either way. No signature change, no change on the success path.

Test

One test, test_close_closes_the_connection_when_commit_fails, in the existing
TestSQLiteStorageManager class. Verified to fail on the unfixed tree before being kept:

FAILED tests/unit/test_util_and_sqlite.py::TestSQLiteStorageManager::
       test_close_closes_the_connection_when_commit_fails
E   AssertionError: assert False
E    +  where False = <MagicMock name='mock.close'>.called

Scope

Only the commit/close ordering. The finally: block below it, which unlocks and closes
_lock_fd, is untouched -- that is the part #132 and #135 change for Windows, and this
branch deliberately stays out of their way.

Summary by CodeRabbit

• Bug Fixes

  • Improved database shutdown reliability by ensuring SQLite connections close even when saving changes encounters an error.

• Tests

  • Added coverage for connection cleanup when a database operation fails during shutdown.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1e30de61-4874-451f-a1bb-33954149b677

📥 Commits

Reviewing files that changed from the base of the PR and between e137e1b and 991fa60.

📒 Files selected for processing (2)

• src/nooa/storage/sqlite.py
• tests/unit/test_util_and_sqlite.py

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

SQLiteStorageManager.close() now closes the SQLite connection when commit() raises, with coverage for the failure path.

Changes

SQLite connection cleanup

Layer / File(s)	Summary
Guaranteed connection cleanup src/nooa/storage/sqlite.py, tests/unit/test_util_and_sqlite.py	close() uses try/finally blocks to close the connection after commit attempts. The regression test verifies cleanup when commit() raises sqlite3.OperationalError.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 991fa

SQLite connections are now closed when commit fails while preserving the original error behavior. The targeted regression test covers this failure path, with no remaining merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: closing the SQLite connection when commit() fails. The emojis add minor noise but do not obscure the change.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}