nodes/ComfyAssets-ComfyUI_PromptManager
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(system): never close another thread's library connection during a switch

Browse Source 
Move library closed every thread's old connection from the mover thread,
including one whose owner was inside a statement waiting for the old
file's write lock. Closing a sqlite3 connection under a running
sqlite3_step is a use-after-free: Windows CI died with "access violation"
in record_prompt_use, and on Linux the writer got a MemoryError once its
busy timeout ran out.

Managed connections now count the calls their owner is inside.
Retiring one from another thread closes it at once only when idle;
otherwise it is interrupted (sqlite3_interrupt is thread-safe) and its
owner closes it as the call ends. A retired connection refuses new work
and never commits, so a write that waited for the lock is rolled back
and survive_library_switch runs it again on the new library. The mover
retires connections while it holds the locks and waits for the owners
to close them only after releasing them, before the old files are
renamed or a failed copy is removed. Cursors are closed before the
connection so Python 3.10/3.11 release the file handle too.
This commit is contained in:
Vito Sansevero
2026-10-24 16:42:29 -07:00
parent 8bf759d6f8
commit 1dcebd640c
6 changed files with 700 additions and 91 deletions
Download Patch File Download Diff File Expand all files Collapse all files
database/library_move.py
+32 -9
View File
@@ -438,8 +438,8 @@ def switch_process(old_db: str, location, detached: list) -> None:
"""Point this process at ``location``: resolver, every model, observers.
Adds the old file's connections, detached and still open, to
``detached`` as soon as they are detached, so the mover closes them
(``PromptModel.close_detached``) whether or not the rest succeeds.
``detached`` as soon as they are detached, so the mover retires them
(``PromptModel.retire_detached``) whether or not the rest succeeds.
"""
from .execution_history import rekey_history_restore
from .review_changes import rekey_review_changes
@@ -474,6 +474,8 @@ class _Move:
self.published: List[str] = []
self.marker: Optional[str] = None
self.moved = False
# Retired connections still in use; awaited once the locks are free.
self.retiring: list = []
def temp(self, suffix: str) -> str:
path = os.path.join(
@@ -704,7 +706,9 @@ def _switch_guarded(move: _Move, location) -> list:
The copy's write lock keeps every repointed thread from writing to it
until the switch is whole, so undoing a partial switch loses nothing.
Returns the old file's detached connections (still open).
Returns the old file's detached connections (still open). On failure
every detached connection is retired; those still in use are added to
``move.retiring`` for the caller to await once the locks are released.
"""
plan = move.plan
guard = sqlite3.connect(
@@ -718,15 +722,19 @@ def _switch_guarded(move: _Move, location) -> list:
except Exception as error:
logger.exception("Library move: switching failed; undoing it")
move.moved = False # the copy and its marker are removed
# Retire, never wait, here: a connection still in use may be
# waiting on the very locks this thread holds.
try:
PromptModel.close_detached(
move.unswitch(plan.db_path, location.db_path), logger
move.retiring.extend(
PromptModel.retire_detached(
move.unswitch(plan.db_path, location.db_path), logger
)
)
except Exception:
logger.exception("Library move: undoing the switch failed")
# The old file's connections a partial switch detached: the
# models reopen it on next use, so close them now, not at GC.
PromptModel.close_detached(detached, logger)
move.retiring.extend(PromptModel.retire_detached(detached, logger))
raise MoveFailed("switch-failed") from error
move.checkpoint("switched")
return detached
@@ -737,6 +745,20 @@ def _switch_guarded(move: _Move, location) -> list:
def _copy_and_switch(move: _Move, location, attempts: int):
"""Copy, then switch under both locks; retired connections are awaited.
A connection that was inside a statement when it was retired is closed
by its own thread once the statement ends, which may need the locks
released first; the old files are renamed (and a failed copy removed)
only after that, since Windows refuses to rename or delete an open file.
"""
try:
return _copy_and_switch_locked(move, location, attempts)
finally:
PromptModel.await_detached(move.retiring, logger)
def _copy_and_switch_locked(move: _Move, location, attempts: int):
plan = move.plan
for attempt in range(1, attempts + 1):
source = sqlite3.connect(
@@ -756,9 +778,10 @@ def _copy_and_switch(move: _Move, location, attempts: int):
files = move.publish(temp_db, copies)
detached = _switch_guarded(move, location)
# Still holding the old file's lock: a write that was waiting
# on it fails when its connection closes (and runs again on
# the new library) instead of landing in the old file.
PromptModel.close_detached(detached, logger)
# on it is interrupted and cannot commit, so it fails (and
# runs again on the new library) instead of landing in the
# old file. Its own thread closes the connection.
move.retiring.extend(PromptModel.retire_detached(detached, logger))
finally:
if source.in_transaction:
source.execute("ROLLBACK")
database/managed_connection.py
+260
View File
@@ -0,0 +1,260 @@
"""A SQLite connection that is never closed under the thread using it.
``PromptModel`` gives each thread its own connection, but Move library and
``close_all`` must retire every thread's connection from one thread. Closing
a ``sqlite3.Connection`` while its owner is inside a statement (Python runs
``sqlite3_step`` without the GIL) frees memory the owner then reads: on
Windows the process dies with "access violation", elsewhere the owner sees
garbage such as a ``MemoryError``.
``ManagedConnection`` counts the calls its owner is inside. ``retire()`` is
safe from any thread: an idle connection is closed at once; one in use is
interrupted (``sqlite3_interrupt`` is thread-safe) and closed by its owner as
the call ends. A retired connection refuses new work with the error a closed
connection raises, and never commits: a write that started before the
retirement and waited for the database lock is rolled back, so it cannot
land in a file the process has switched away from.
"""
from __future__ import annotations
import contextlib
import logging
import threading
import time
import weakref
from typing import Iterable, List, Optional
import sqlite3
CLOSED_MESSAGE = "Cannot operate on a closed database."
class ManagedCursor(sqlite3.Cursor):
"""A cursor whose calls into SQLite count as its connection being in use."""
def execute(self, sql, parameters=(), /):
conn = self.connection
conn._enter()
try:
return super().execute(sql, parameters)
finally:
conn._leave()
def executemany(self, sql, seq_of_parameters, /):
conn = self.connection
conn._enter()
try:
return super().executemany(sql, seq_of_parameters)
finally:
conn._leave()
def executescript(self, sql_script, /):
conn = self.connection
conn._enter()
try:
return super().executescript(sql_script)
finally:
conn._leave()
def fetchone(self):
conn = self.connection
conn._enter()
try:
return super().fetchone()
finally:
conn._leave()
def fetchmany(self, *args, **kwargs):
conn = self.connection
conn._enter()
try:
return super().fetchmany(*args, **kwargs)
finally:
conn._leave()
def fetchall(self):
conn = self.connection
conn._enter()
try:
return super().fetchall()
finally:
conn._leave()
def __next__(self):
conn = self.connection
conn._enter()
try:
return super().__next__()
finally:
conn._leave()
def close(self):
conn = self.connection
try:
conn._enter()
except sqlite3.ProgrammingError:
return super().close() # the connection is closed or closing
try:
return super().close()
finally:
conn._leave()
class ManagedConnection(sqlite3.Connection):
"""A connection only its owner closes while a call into it is running."""
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self._use_lock = threading.Lock()
self._depth = 0
self._retired = False
self._released = threading.Event()
# Python before 3.12 keeps the file open after close() while a cursor
# still holds a statement; these are closed first (see _close_now).
self._cursors: "weakref.WeakSet[sqlite3.Cursor]" = weakref.WeakSet()
self.close_logger: Optional[logging.Logger] = None
# ── in use ───────────────────────────────────────────────────────
def _enter(self) -> None:
with self._use_lock:
if self._retired:
raise sqlite3.ProgrammingError(CLOSED_MESSAGE)
self._depth += 1
def _leave(self) -> None:
with self._use_lock:
self._depth -= 1
close_now = self._retired and self._depth == 0
if close_now:
self._close_now()
def cursor(self, factory=None):
cursor = super().cursor(factory or ManagedCursor)
self._cursors.add(cursor)
return cursor
def execute(self, sql, parameters=(), /):
return self.cursor().execute(sql, parameters)
def executemany(self, sql, seq_of_parameters, /):
return self.cursor().executemany(sql, seq_of_parameters)
def executescript(self, sql_script, /):
return self.cursor().executescript(sql_script)
def commit(self):
self._enter()
try:
return super().commit()
finally:
self._leave()
def rollback(self):
self._enter() # a closed connection raises, as sqlite3's does
try:
return super().rollback()
finally:
self._leave()
def backup(self, target, **kwargs):
self._enter()
try:
return super().backup(target, **kwargs)
finally:
self._leave()
def __exit__(self, exc_type, exc_value, traceback):
# The C implementation calls its own commit/rollback, not these.
if exc_type is None:
try:
self.commit()
except BaseException:
with contextlib.suppress(sqlite3.Error):
self.rollback()
raise
else:
self.rollback()
return False
# ── closing ──────────────────────────────────────────────────────
def close(self) -> None:
"""Close now if idle, else as soon as the running call ends (any thread)."""
self.retire()
def retire(self) -> bool:
"""Refuse new work and close; True when the connection is closed already."""
with self._use_lock:
if not self._retired:
self._retired = True
if self._depth:
# Aborts the running statement, or the one waiting for
# the lock as soon as it gets it. Thread-safe; the owner
# cannot close the connection while this lock is held.
self.interrupt()
return False
elif self._depth:
return False
else:
return self._released.wait(0)
self._close_now()
return True
def wait_closed(self, timeout: float) -> bool:
return self._released.wait(timeout)
def _close_now(self) -> None:
# Only ever called with no call into the connection running, and a
# retired connection refuses new ones: its cursors are idle.
for cursor in list(self._cursors):
with contextlib.suppress(sqlite3.Error):
sqlite3.Cursor.close(cursor)
try:
super().close()
except sqlite3.Error as error:
if self.close_logger is not None:
self.close_logger.warning(f"Error closing database connection: {error}")
finally:
self._released.set()
def retire_connections(
connections: Iterable[sqlite3.Connection], logger=None
) -> List[ManagedConnection]:
"""Retire every connection; return those whose owner has yet to close them.
Safe while holding a lock the owners wait on: nothing here waits for them.
"""
pending = []
for conn in connections:
if isinstance(conn, ManagedConnection):
if logger is not None:
conn.close_logger = logger
if not conn.retire():
pending.append(conn)
continue
try:
conn.close()
except sqlite3.Error as error:
if logger is not None:
logger.warning(f"Error closing database connection: {error}")
return pending
def await_closed(
connections: Iterable[ManagedConnection], timeout: float, logger=None
) -> bool:
"""Wait until the owners closed ``connections`` (retired); False on timeout."""
deadline = time.monotonic() + timeout
still_open = 0
for conn in connections:
if not conn.wait_closed(max(0.0, deadline - time.monotonic())):
still_open += 1
if still_open and logger is not None:
logger.warning(
f"{still_open} database connection(s) still in use after "
f"{timeout:g}s; they close when their statement ends"
)
return not still_open
database/models.py
+42 -18
View File
@@ -15,6 +15,11 @@ from contextlib import closing
from pathlib import Path
from typing import Optional, Set, Tuple
from .managed_connection import (
ManagedConnection,
await_closed,
retire_connections,
)
from .library_schema import library_schema_error, migrate_library_state
from .image_favorites import image_favorites_schema_error, migrate_image_favorites
from .annotation_schema import annotation_schema_error, migrate_annotation_state
@@ -615,10 +620,15 @@ class PromptModel:
# meanwhile: otherwise it would write to the old file.
with self._conn_lock:
path, generation = self.db_path, self._generation
# check_same_thread=False only so close_all can clean up other
# threads' connections; each connection is still used by one thread.
# check_same_thread=False only so close_all and Move library can
# retire other threads' connections; each connection is still
# used by one thread, and ManagedConnection never closes it under
# a running statement (see managed_connection).
conn = sqlite3.connect(
path, check_same_thread=False, timeout=BUSY_TIMEOUT_SECONDS
path,
check_same_thread=False,
timeout=BUSY_TIMEOUT_SECONDS,
factory=ManagedConnection,
)
try:
conn.row_factory = sqlite3.Row
@@ -654,9 +664,9 @@ class PromptModel:
Every thread opens the new file on its next ``get_connection``. The
old connections are detached, not closed, and returned: the caller
closes them once nothing can block on the new file any more. Closing
a connection another thread is using waits for its statement; a write
waiting on the mover's lock then fails (and, through
retires them (``retire_detached``) while it still holds the old
file's write lock. A write waiting on that lock is then interrupted
and can never commit, so it fails (and, through
``PromptDatabase.survive_library_switch``, runs again on the new file).
"""
old_key = os.path.normcase(os.path.realpath(old_path))
@@ -671,14 +681,31 @@ class PromptModel:
return detached
@staticmethod
def close_detached(connections, logger=None) -> None:
"""Close connections ``repoint_instances`` detached."""
for conn in connections:
try:
conn.close()
except sqlite3.Error as e:
if logger is not None:
logger.warning(f"Error closing database connection: {e}")
def retire_detached(connections, logger=None) -> list:
"""Retire detached connections without waiting for their owners.
Idle ones close now. One whose owner thread is inside a statement is
never closed from here (that frees memory the owner is using: an
access violation on Windows); it is interrupted, refuses to commit and
is closed by its owner as the statement ends. Returns those still
open, for ``await_detached`` once no lock the owners wait on is held.
"""
return retire_connections(connections, logger)
@staticmethod
def await_detached(connections, logger=None) -> bool:
"""Wait until retired connections are closed; False if some are not."""
return await_closed(connections, BUSY_TIMEOUT_SECONDS, logger)
@staticmethod
def close_detached(connections, logger=None) -> bool:
"""Retire connections ``repoint_instances`` detached and await them.
Do not call it while holding a lock their owners may wait on; retire
them there (``retire_detached``) and await them after releasing it.
"""
pending = retire_connections(connections, logger)
return await_closed(pending, BUSY_TIMEOUT_SECONDS, logger)
def close_all(self) -> None:
"""Close every thread's connection; they reopen lazily on next use.
@@ -714,10 +741,7 @@ class PromptModel:
self._local.conn = None
with self._conn_lock:
self._connections.discard(conn)
try:
conn.close()
except sqlite3.Error as e:
self.logger.warning(f"Error closing database connection: {e}")
self.close_detached([conn], self.logger)
def _migrate_workflow_name_removal(self, conn: sqlite3.Connection) -> None:
"""