mirror of
https://github.com/willmiao/ComfyUI-Lora-Manager.git
synced 2026-09-21 03:01:27 -03:00
fix(cache): make shared cache state survive a second instance
Installing a second LoRA Manager instance (standalone or a second ComfyUI install) that shares the settings directory puts two processes on the same cache databases. Three things made that unsafe. - The updater preserved cache/ and model_cache/ but not a legacy recipe_cache/ directory, so a portable install predating the cache/ move lost its recipe database on a git-based update. Add it to _PRESERVE_DIRS and to .gitignore. - Cache connections used the sqlite3 default 5s timeout, which a scanning instance can exceed, turning a concurrent write into "database is locked". Route every shared cache connection through connect_cache_db(), which raises the timeout to 30s and sets busy_timeout + synchronous=NORMAL to match the existing WAL mode. App-private databases (download queue, update history) are unchanged. - A full-table cache replace is a read-modify-write that SQLite cannot make atomic across processes, so two instances could interleave and one snapshot could overwrite the other. Guard the recipe and model save_cache paths with a cross-process advisory lock (flock on POSIX, msvcrt on Windows). Locking is best-effort: if it is unavailable the call proceeds and the SQLite busy timeout is the fallback. The lock file is a hidden sibling of the database and is deliberately never unlinked, so a second process cannot lock a fresh inode.
This commit is contained in:
Will Miao
@@ -0,0 +1,107 @@
|
||||
"""Tests for the shared cache SQLite connection settings (:mod:`py.utils.cache_db`).
|
||||
|
||||
Two LoRA Manager processes can share one settings directory, so cache
|
||||
connections must tolerate a competing writer instead of failing immediately
|
||||
with "database is locked".
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import sqlite3
|
||||
import threading
|
||||
import time
|
||||
|
||||
from py.utils.cache_db import CONCURRENT_TIMEOUT_SECONDS, connect_cache_db
|
||||
|
||||
|
||||
def test_busy_timeout_pragma_is_applied(tmp_path):
|
||||
"""The connection must retry inside SQLite, not just at connect() time."""
|
||||
conn = connect_cache_db(str(tmp_path / "cache.sqlite"))
|
||||
try:
|
||||
value = conn.execute("PRAGMA busy_timeout").fetchone()[0]
|
||||
finally:
|
||||
conn.close()
|
||||
assert value == int(CONCURRENT_TIMEOUT_SECONDS * 1000)
|
||||
|
||||
|
||||
def test_waiting_writer_succeeds_after_competing_writer_commits(tmp_path):
|
||||
"""A blocked writer waits for the lock instead of raising."""
|
||||
db_path = str(tmp_path / "cache.sqlite")
|
||||
|
||||
holder = connect_cache_db(db_path)
|
||||
holder.execute("CREATE TABLE t (v INTEGER)")
|
||||
holder.commit()
|
||||
holder.execute("BEGIN IMMEDIATE")
|
||||
|
||||
def release_after_delay() -> None:
|
||||
time.sleep(0.5)
|
||||
holder.commit()
|
||||
|
||||
releaser = threading.Thread(target=release_after_delay)
|
||||
releaser.start()
|
||||
try:
|
||||
waiter = connect_cache_db(db_path)
|
||||
try:
|
||||
# Under the old 5s default this still worked, but an immediate
|
||||
# failure is what low-timeout connections produced; assert the
|
||||
# write lands rather than propagating "database is locked".
|
||||
waiter.execute("INSERT INTO t VALUES (1)")
|
||||
waiter.commit()
|
||||
finally:
|
||||
waiter.close()
|
||||
finally:
|
||||
releaser.join()
|
||||
holder.close()
|
||||
|
||||
check = connect_cache_db(db_path)
|
||||
try:
|
||||
assert check.execute("SELECT COUNT(*) FROM t").fetchone()[0] == 1
|
||||
finally:
|
||||
check.close()
|
||||
|
||||
|
||||
def test_readwrite_connection_uses_row_factory(tmp_path):
|
||||
conn = connect_cache_db(str(tmp_path / "cache.sqlite"), row_factory=sqlite3.Row)
|
||||
try:
|
||||
conn.execute("CREATE TABLE t (v INTEGER)")
|
||||
conn.execute("INSERT INTO t VALUES (7)")
|
||||
conn.commit()
|
||||
row = conn.execute("SELECT v FROM t").fetchone()
|
||||
assert row["v"] == 7
|
||||
finally:
|
||||
conn.close()
|
||||
|
||||
|
||||
def test_readonly_connection_reads_without_writing(tmp_path):
|
||||
db_path = str(tmp_path / "cache.sqlite")
|
||||
writer = connect_cache_db(db_path)
|
||||
writer.execute("CREATE TABLE t (v INTEGER)")
|
||||
writer.execute("INSERT INTO t VALUES (1)")
|
||||
writer.commit()
|
||||
writer.close()
|
||||
|
||||
conn = connect_cache_db(db_path, readonly=True)
|
||||
try:
|
||||
assert conn.execute("SELECT v FROM t").fetchone()[0] == 1
|
||||
finally:
|
||||
conn.close()
|
||||
|
||||
|
||||
def test_readonly_connection_rejects_writes(tmp_path):
|
||||
db_path = str(tmp_path / "cache.sqlite")
|
||||
writer = connect_cache_db(db_path)
|
||||
writer.execute("CREATE TABLE t (v INTEGER)")
|
||||
writer.commit()
|
||||
writer.close()
|
||||
|
||||
conn = connect_cache_db(db_path, readonly=True)
|
||||
try:
|
||||
try:
|
||||
conn.execute("INSERT INTO t VALUES (1)")
|
||||
conn.commit()
|
||||
except sqlite3.OperationalError:
|
||||
pass
|
||||
else: # pragma: no cover - would mean mode=ro was not applied
|
||||
raise AssertionError("read-only connection accepted a write")
|
||||
finally:
|
||||
conn.close()
|
||||
@@ -0,0 +1,118 @@
|
||||
"""Tests for the cross-process advisory lock (:mod:`py.utils.file_lock`)."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import time
|
||||
|
||||
import pytest
|
||||
|
||||
from py.utils.file_lock import (
|
||||
CrossProcessLock,
|
||||
FileLockUnavailable,
|
||||
exclusive_lock,
|
||||
lock_path_for,
|
||||
)
|
||||
|
||||
|
||||
def test_lock_path_is_a_sibling_of_the_resource(tmp_path):
|
||||
db_path = str(tmp_path / "recipe" / "default.sqlite")
|
||||
lock_path = lock_path_for(db_path)
|
||||
|
||||
assert os.path.dirname(lock_path) == os.path.dirname(db_path)
|
||||
assert os.path.basename(lock_path) == ".default.sqlite.lock"
|
||||
|
||||
|
||||
def test_acquire_and_release_round_trip(tmp_path):
|
||||
lock = exclusive_lock(str(tmp_path / "cache.sqlite"))
|
||||
|
||||
assert lock.acquire() is True
|
||||
lock.release()
|
||||
# Releasing twice must be safe.
|
||||
lock.release()
|
||||
# ...and the lock is reusable afterwards.
|
||||
assert lock.acquire() is True
|
||||
lock.release()
|
||||
|
||||
|
||||
def test_second_lock_holder_waits_until_release(tmp_path):
|
||||
"""A held lock blocks a competing holder for the same resource."""
|
||||
db_path = str(tmp_path / "cache.sqlite")
|
||||
first = exclusive_lock(db_path)
|
||||
second = CrossProcessLock(lock_path_for(db_path), timeout=0.2)
|
||||
|
||||
assert first.acquire() is True
|
||||
try:
|
||||
started = time.monotonic()
|
||||
assert second.acquire() is False
|
||||
# It must have waited for the timeout rather than failing instantly.
|
||||
assert time.monotonic() - started >= 0.15
|
||||
finally:
|
||||
first.release()
|
||||
|
||||
# Once released, the contender gets the lock.
|
||||
assert second.acquire() is True
|
||||
second.release()
|
||||
|
||||
|
||||
def test_context_manager_releases_on_exception(tmp_path):
|
||||
lock = exclusive_lock(str(tmp_path / "cache.sqlite"))
|
||||
contender = CrossProcessLock(lock.path, timeout=0.2)
|
||||
|
||||
with pytest.raises(RuntimeError):
|
||||
with lock:
|
||||
raise RuntimeError("boom")
|
||||
|
||||
assert contender.acquire() is True
|
||||
contender.release()
|
||||
|
||||
|
||||
def test_lock_file_is_not_deleted(tmp_path):
|
||||
"""Deleting the lock file would let a second process lock a fresh inode."""
|
||||
lock = exclusive_lock(str(tmp_path / "cache.sqlite"))
|
||||
assert lock.acquire() is True
|
||||
lock.release()
|
||||
|
||||
assert os.path.exists(lock.path)
|
||||
|
||||
|
||||
def test_unsupported_platform_degrades_gracefully(tmp_path, monkeypatch):
|
||||
"""Without a platform primitive the lock reports failure instead of raising."""
|
||||
import py.utils.file_lock as file_lock_module
|
||||
|
||||
monkeypatch.setattr(file_lock_module, "fcntl", None)
|
||||
monkeypatch.setattr(file_lock_module, "msvcrt", None)
|
||||
|
||||
lock = exclusive_lock(str(tmp_path / "cache.sqlite"))
|
||||
assert lock.acquire() is False
|
||||
# Callers use it as a context manager and continue without the lock.
|
||||
with exclusive_lock(str(tmp_path / "cache.sqlite")):
|
||||
pass
|
||||
|
||||
|
||||
def test_file_lock_unavailable_is_exported():
|
||||
assert issubclass(FileLockUnavailable, RuntimeError)
|
||||
|
||||
|
||||
def test_save_cache_creates_lock_next_to_database(tmp_path):
|
||||
"""The recipe cache write path actually takes the cross-process lock."""
|
||||
from py.services.persistent_recipe_cache import PersistentRecipeCache
|
||||
|
||||
db_path = tmp_path / "recipe_cache.sqlite"
|
||||
cache = PersistentRecipeCache(db_path=str(db_path))
|
||||
assert cache.save_cache([{"id": "r1", "title": "One"}], {"r1": "/tmp/r1.json"})
|
||||
|
||||
assert os.path.exists(lock_path_for(str(db_path)))
|
||||
|
||||
|
||||
def test_save_cache_releases_lock_after_write(tmp_path):
|
||||
"""A second writer must not be blocked once the first has finished."""
|
||||
from py.services.persistent_recipe_cache import PersistentRecipeCache
|
||||
|
||||
db_path = tmp_path / "recipe_cache.sqlite"
|
||||
cache = PersistentRecipeCache(db_path=str(db_path))
|
||||
cache.save_cache([{"id": "r1", "title": "One"}], {"r1": "/tmp/r1.json"})
|
||||
|
||||
contender = CrossProcessLock(lock_path_for(str(db_path)), timeout=0.2)
|
||||
assert contender.acquire() is True
|
||||
contender.release()
|
||||
Reference in New Issue
Block a user