fix(db): stop closing a read connection out from under a live query

Found by smoke-testing 2.1.4 and reading the log - which is only possible now
that #263 stopped one call producing 98.5% of it. Closing the Monitor logged:

    WidgetState.get_hardware_history - Cannot operate on a closed database
    sqlite3.ProgrammingError: Cannot operate on a closed database

Read connections are per-thread, and stale ones were evicted by checking the
owning thread id against threading.enumerate(). Qt worker threads never appear
there: enumerate() only lists threads the threading module knows about, and a
QThread that calls threading.get_ident() - which is all ours do - is never
registered. So the Monitor's graph worker was permanently "dead" by that test,
and the next _get_read_conn() from any other thread closed its connection
while it was still querying.

The obvious fix is wrong, and I verified rather than assumed: calling
threading.current_thread() does register the thread, but on CPython 3.11 the
resulting _DummyThread never leaves enumerate() when the native thread dies -
so nothing would ever be pruned and the connection leak that pruning exists to
prevent comes straight back.

Pruning is now by idle time. A thread mid-query has just fetched its
connection, so it cannot trip a threshold; a finished worker ages out and is
reclaimed as before. The threshold is deliberately generous - it only has to
exceed a live worker's refresh interval, and erring high costs one idle file
handle while erring low reintroduces the bug.

The existing leak test is kept and joined by the race it could not see.
Verified by reverting: the race test fails on the old logic.

Author: erez-c137

src/netspeedtray/core/widget_state.py

@@ -120,7 +120,7 @@ def __init__(self, config: Dict[str, Any], read_only: bool = False) -> None:
             self.maintenance_timer.start(60 * 60 * 1000) # Run maintenance every hour
             self.trigger_maintenance()
 
-        self._read_conns: Dict[int, sqlite3.Connection] = {}
+        self._read_conns: Dict[int, Tuple[sqlite3.Connection, float]] = {}   # tid -> (conn, last_used)
         self._read_conns_lock = threading.Lock()
 
         # Data-usage odometer (data-cap feature). Lazily loaded from the DB on first
@@ -135,21 +135,44 @@ def __init__(self, config: Dict[str, Any], read_only: bool = False) -> None:
         self.logger.debug("WidgetState initialized with threaded database worker.")
 
 
+    # How long a read connection may sit untouched before another thread may close it. Generous on
+    # purpose: it only has to exceed the interval at which a live worker re-fetches its connection
+    # (the Monitor refreshes every few seconds), and erring high costs one idle file handle while
+    # erring low would close a connection out from under a slow query.
+    _READ_CONN_IDLE_SEC: float = 120.0
+
     def _get_read_conn(self) -> sqlite3.Connection:
         """Returns a thread-local read connection, pruning ones left behind by dead threads.
 
         Each worker thread (notably the Monitor's GraphDataWorker, recreated on every Monitor open) opens
         its own read connection here, removed otherwise only in cleanup() at exit. Without pruning, every
         Monitor open/close leaked a connection (a file handle + a WAL reader slot) for the whole session,
         and a recycled thread id could even hand a new worker a dead thread's stale connection. So evict
-        entries whose owning thread has exited (safe to close cross-thread: check_same_thread=False)."""
+        entries whose owning thread has stopped using them.
+
+        **Pruning is by IDLE TIME, not by thread liveness, and that is not a stylistic choice.** This
+        used to evict any connection whose thread id was missing from ``threading.enumerate()`` - but
+        Qt worker threads (``QThread``) never appear there unless they happen to call
+        ``threading.current_thread()``, and ours only call ``get_ident()``. So every Monitor graph
+        worker was invisible, and the next caller from any other thread closed its connection **while
+        it was still querying** - "Cannot operate on a closed database", caught and logged, once per
+        Monitor session.
+
+        Registering the thread instead (calling ``current_thread()``) is worse: on CPython 3.11 the
+        resulting ``_DummyThread`` never leaves ``enumerate()`` when the native thread dies, so
+        nothing would ever be pruned and the leak this exists to prevent comes straight back
+        (verified, not assumed).
+
+        A thread mid-query has just fetched its connection, so an idle threshold it cannot trip is
+        both simpler and correct. Closing cross-thread stays safe: check_same_thread=False."""
         thread_id = threading.get_ident()
+        now = time.monotonic()
         with self._read_conns_lock:
             if self._read_conns:
-                live = {t.ident for t in threading.enumerate()}
-                for dead in [tid for tid in self._read_conns if tid not in live and tid != thread_id]:
+                for stale in [tid for tid, (_c, seen) in self._read_conns.items()
+                              if tid != thread_id and (now - seen) > self._READ_CONN_IDLE_SEC]:
                     try:
-                        self._read_conns.pop(dead).close()
+                        self._read_conns.pop(stale)[0].close()
                     except Exception:
                         pass
             if thread_id not in self._read_conns:
@@ -164,8 +187,11 @@ def _get_read_conn(self) -> sqlite3.Connection:
                 conn.execute("PRAGMA cache_size = -8000;")
                 conn.execute("PRAGMA temp_store = MEMORY;")
                 conn.execute("PRAGMA mmap_size = 268435456;")
-                self._read_conns[thread_id] = conn
-            return self._read_conns[thread_id]
+                self._read_conns[thread_id] = (conn, now)
+                return conn
+            conn, _seen = self._read_conns[thread_id]
+            self._read_conns[thread_id] = (conn, now)   # touch: this thread is demonstrably alive
+            return conn
 
     def add_speed_data(self, speed_data: Dict[str, Tuple[float, float]], now: Optional[datetime] = None, aggregated_up: Optional[float] = None, aggregated_down: Optional[float] = None) -> None:
         """Adds new per-interface speed data."""
@@ -923,7 +949,7 @@ def cleanup(self) -> None:
 
         # Close persistent read connections
         with self._read_conns_lock:
-            for tid, conn in self._read_conns.items():
+            for tid, (conn, _seen) in self._read_conns.items():
                 try:
                     conn.close()
                 except:

src/netspeedtray/tests/unit/test_reliability_fixes.py

@@ -8,6 +8,7 @@
 import os
 import sqlite3
 import threading
+import time
 from pathlib import Path
 from unittest.mock import patch
 
@@ -39,26 +40,72 @@ def test_update_config_flags_instead_of_closing_pdh_handles(q_app):
 
 # --- M2: read-connection leak ------------------------------------------------
 
-def test_get_read_conn_prunes_dead_thread_connections(q_app, tmp_path: Path):
+def _make_state(tmp_path):
     cfg = dict(constants.config.defaults.DEFAULT_CONFIG)
-    with patch.object(QThread, "start", lambda self: None), \
-         patch("netspeedtray.core.widget_state.get_app_data_path", return_value=tmp_path):
+    with patch.object(QThread, "start", lambda self: None),          patch("netspeedtray.core.widget_state.get_app_data_path", return_value=tmp_path):
         ws = WidgetState(cfg)
     ws._db_path = tmp_path / "speed_history.db"
     ws.db_worker.db_path = ws._db_path
     ws.db_worker._initialize_connection()
     ws.db_worker._check_and_create_schema()
+    return ws
 
-    # Inject a connection as if a now-dead worker thread had opened it (a fake, non-live thread id).
-    dead_id = max(t.ident for t in threading.enumerate()) + 999_999
-    real = sqlite3.connect(":memory:")
-    ws._read_conns[dead_id] = real
 
-    ws._get_read_conn()                            # any call prunes dead-thread entries
+def test_get_read_conn_reclaims_idle_connections(q_app, tmp_path: Path):
+    """M2: a connection left behind by a finished worker must not leak for the whole session."""
+    ws = _make_state(tmp_path)
+    stale_id = max(t.ident for t in threading.enumerate()) + 999_999
+    leaked = sqlite3.connect(":memory:")
+    # Last used well beyond the idle threshold - i.e. nobody is querying on it.
+    ws._read_conns[stale_id] = (leaked, time.monotonic() - (ws._READ_CONN_IDLE_SEC + 60))
 
-    assert dead_id not in ws._read_conns, "stale dead-thread connection was not evicted"
-    with pytest.raises(sqlite3.ProgrammingError):  # the leaked connection was closed
-        real.execute("SELECT 1")
+    ws._get_read_conn()
+
+    assert stale_id not in ws._read_conns, "the idle connection was not evicted"
+    with pytest.raises(sqlite3.ProgrammingError):
+        leaked.execute("SELECT 1")                 # and it really was closed
+    ws.cleanup()
+
+
+def test_get_read_conn_never_closes_a_connection_still_in_use(q_app, tmp_path: Path):
+    """The race this replaced liveness-checking to fix.
+
+    Pruning used to evict any connection whose thread id was absent from `threading.enumerate()`.
+    Qt worker threads never appear there - they call `threading.get_ident()`, not
+    `threading.current_thread()` - so the Monitor's graph worker was permanently invisible, and the
+    next call from any other thread closed its connection *mid-query*:
+
+        WidgetState.get_hardware_history - Cannot operate on a closed database
+
+    Registering the thread instead is worse: on CPython 3.11 the `_DummyThread` never leaves
+    `enumerate()` once the native thread dies, so nothing would ever be pruned and the leak above
+    comes back. Idle time is the one signal a busy thread cannot trip.
+    """
+    ws = _make_state(tmp_path)
+    worker_id = max(t.ident for t in threading.enumerate()) + 999_999   # invisible, like a QThread
+    in_use = sqlite3.connect(":memory:")
+    ws._read_conns[worker_id] = (in_use, time.monotonic())              # just used it
+
+    ws._get_read_conn()                                                 # a call from another thread
+
+    assert worker_id in ws._read_conns, "an in-use connection was evicted"
+    assert in_use.execute("SELECT 1").fetchone() == (1,), (
+        "the connection was closed while its thread was still querying - this is the bug")
+    ws.cleanup()
+
+
+def test_reusing_a_connection_refreshes_its_idle_clock(q_app, tmp_path: Path):
+    """A long-lived worker must not age out just because it has been running a while."""
+    ws = _make_state(tmp_path)
+    first = ws._get_read_conn()
+    tid = threading.get_ident()
+    ws._read_conns[tid] = (first, time.monotonic() - (ws._READ_CONN_IDLE_SEC + 60))
+
+    again = ws._get_read_conn()
+
+    assert again is first, "the same thread should keep its own connection"
+    _conn, seen = ws._read_conns[tid]
+    assert (time.monotonic() - seen) < 1.0, "fetching a connection did not refresh its idle clock"
     ws.cleanup()