fix(history): reject unsupported uniqueness and isolate package fixtures (#209)

This commit is contained in:
Vito Sansevero
2026-10-17 16:18:48 -07:00
parent 978fdfb2cc
commit c826b593b3
9 changed files with 421 additions and 49 deletions
+28 -2
View File
@@ -531,7 +531,7 @@ Keep your database optimized with built-in maintenance tools:
### Database Location
By default, the database is saved as `example_prompts.db` in the node directory. This file contains all your prompts and linked images and can be backed up or shared.
By default, the database is saved as `prompts.db` in the extension directory. It contains your prompt library and image links. See the [database path compatibility note](#database-path-compatibility-211) before upgrading a deployment with a custom path.
### Searching Prompts
@@ -773,6 +773,32 @@ Relative paths are resolved against the extension directory. Absolute paths are
}
```
### Database Path Compatibility (#211)
PromptManager now honors the `database.default_path` setting in `config.json`
when loaded as a ComfyUI custom node. Relative paths resolve against the extension
directory, not ComfyUI's working directory; absolute paths are used as-is. Earlier
versions could ignore this setting and use `<extension>/prompts.db` instead.
Honoring a different configured path can therefore show an empty or different
library. PromptManager does not automatically move or merge database files.
Before upgrading or changing this setting:
1. Identify the database your current installation actually uses and confirm its
library contents. Check both the configured path and the prior fallback
`<extension>/prompts.db`; keep track of which existing file you intend to use.
2. Preserve a consistent backup of the active database with the dashboard's
Backup/Restore backup action or SQLite's online backup API. Do not copy only
the main file of a live SQLite database: committed data may still be in its
`-wal` file. For a filesystem copy, stop ComfyUI cleanly first and preserve any
remaining `-wal` file together with the main database.
3. Set `database.default_path` to the existing intended database file. An absolute
path makes the selection explicit; do not move or overwrite either database
while choosing the path.
4. Restart ComfyUI, then verify that the expected library appears and the active
database path resolves to the file you selected. Keep the backup until both
checks succeed.
### Gallery & Monitoring Settings
Configure the automatic image detection and gallery system:
@@ -844,7 +870,7 @@ print(f"Average rating: {info['average_rating']}")
# Create backup
db.model.backup_database("backup_prompts.db")
# The database file can be copied directly for backup
# The online backup includes committed data still in the WAL.
```
## Development
+3 -3
View File
@@ -247,9 +247,9 @@ files first because snapshots use `git ls-files`. Never reset the implementation
checkout or point this harness at a live ComfyUI checkout.
The package-qualified config import also corrects #211: the configured database
path is honored when PromptManager runs as a ComfyUI custom node. Existing files
are not moved automatically; deployments that previously used the fallback path
should set the intended database path before restarting.
path is honored when PromptManager runs as a ComfyUI custom node. Follow the
[database path compatibility and safe backup sequence](README.md#database-path-compatibility-211)
before restarting an existing deployment.
The ledger records only new dequeued jobs and observed outcomes. Existing
counters, prompts, pairs and image links are preserved, with no reconstructed old
+1 -1
View File
@@ -125,7 +125,7 @@ class ExecutionHistoryOperations:
"(run_id,session_id,queue_item_id,job_id,storage_epoch,record_version,"
"observed_dequeue_at,outcome,diagnostics_omitted) "
"VALUES(?,?,?,?,?,1,?,'observed',0) "
"ON CONFLICT DO NOTHING",
"ON CONFLICT(session_id,queue_item_id) DO NOTHING",
(
command.run_id,
command.session_id,
+17
View File
@@ -133,6 +133,23 @@ def history_schema_error(
return f"{table} has invalid execution history checks"
if conn.execute(f"PRAGMA foreign_key_list({table})").fetchone():
return f"{table} has unexpected execution history foreign keys"
for index in conn.execute(f"PRAGMA index_list({table})"):
if not index[2]:
continue
if table == "execution_runs" and index[1] == "idx_execution_runs_identity":
continue
name = index[1].replace('"', '""')
keys = tuple(
row for row in conn.execute(f'PRAGMA index_xinfo("{name}")') if row[5]
)
primary = tuple(column[0] for column in expected if column[3])
if (
index[3] != "pk"
or index[4]
or tuple(key[2] for key in keys) != primary
or any(key[3] or key[4] != "BINARY" for key in keys)
):
return f"{table} has unexpected execution history uniqueness"
predicate = " AND ".join(f"({check})" for check in checks)
if conn.execute(
f"SELECT 1 FROM {table} WHERE NOT ({predicate}) LIMIT 1"
+78
View File
@@ -172,6 +172,84 @@ class HistoryDatabaseTests(unittest.TestCase):
self.assertEqual(self.count(db), 2)
self.assertEqual(db.get_execution_run(command.run_id)["outcome"], "observed")
def test_unrelated_unique_conflicts_are_not_acknowledged_as_recorded(self):
db = self.database()
first = self.start(db)
db.write_execution_start(first)
with db.model.get_connection() as conn:
conn.execute("CREATE UNIQUE INDEX unexpected_job ON execution_runs(job_id)")
with self.assertRaises(sqlite3.IntegrityError):
db.write_execution_start(self.start(db, item=1))
self.assertEqual(self.count(db), 1)
self.assertEqual(db.get_execution_run(first.run_id)["observed_dequeue_at"], 100)
def test_primary_key_collision_for_another_attempt_is_not_a_replay(self):
db = self.database()
first = self.start(db)
db.write_execution_start(first)
with self.assertRaises(sqlite3.IntegrityError):
db.write_execution_start(
replace(self.start(db, item=1), run_id=first.run_id)
)
self.assertEqual(self.count(db), 1)
def test_schema_rejects_extra_uniqueness_and_allows_nonunique_indexes(self):
from database.execution_history_schema import history_schema_error
for table, constraint in (
("execution_runs", "job_id"),
("execution_runs", "session_id, queue_item_id"),
("execution_history_metadata", "storage_epoch"),
):
for table_constraint in (False, True):
with self.subTest(
table=table,
table_constraint=table_constraint,
constraint=constraint,
):
path = str(
Path(self.temp.name) / f"unique-{len(self.databases)}.db"
)
db = self.database(path)
conn = db.model.get_connection()
with conn:
if table_constraint:
epoch = db.get_execution_epoch()
sql = conn.execute(
"SELECT sql FROM sqlite_master WHERE name=?", (table,)
).fetchone()[0]
conn.execute(f"DROP TABLE {table}")
conn.execute(sql[:-1] + f", UNIQUE({constraint}))")
if table == "execution_runs":
conn.execute(
"CREATE UNIQUE INDEX idx_execution_runs_identity "
"ON execution_runs(session_id, queue_item_id)"
)
conn.execute(
"CREATE INDEX idx_execution_runs_observed "
"ON execution_runs(observed_dequeue_at, run_id)"
)
else:
conn.execute(
"INSERT INTO execution_history_metadata "
"VALUES(1,1,?)",
(epoch,),
)
else:
conn.execute(
f"CREATE UNIQUE INDEX unexpected_unique "
f"ON {table}({constraint})"
)
self.assertIsNotNone(history_schema_error(conn))
db = self.database()
with db.model.get_connection() as conn:
conn.execute('CREATE INDEX "extra job lookup" ON execution_runs(job_id)')
conn.execute(
"CREATE INDEX extra_epoch_lookup "
"ON execution_history_metadata(storage_epoch)"
)
self.assertIsNone(history_schema_error(conn))
def test_absent_finish_never_fabricates_a_start(self):
db = self.database()
self.assertFalse(db.write_execution_finish(self.finish(self.start(db))))
+14 -15
View File
@@ -120,13 +120,11 @@ class HistoryHooksTests(unittest.TestCase):
def test_package_history_initializes_when_monitor_and_usage_setup_fail(self):
try:
from tests.test_init_package import (
close_every_database,
fake_server_module,
load_package,
)
except ImportError:
from test_init_package import (
close_every_database,
fake_server_module,
load_package,
)
@@ -148,19 +146,20 @@ class HistoryHooksTests(unittest.TestCase):
contextlib.redirect_stdout(io.StringIO()),
self.assertLogs("prompt_manager.init", level="ERROR"),
):
package, _ = load_package(name, self.db.model.db_path, output, server)
self.addCleanup(sys.modules.pop, name, None)
self.addCleanup(close_every_database)
registration = getattr(package, "_history_registration", None)
self.assertIsNotNone(registration, "history setup depended on another hook")
self.registrations.append(registration)
self.addCleanup(registration.dispose)
self.assertIsNone(package._global_db)
self.wait_for(lambda: registration.writer.status().accepting)
self.dequeue(0)
self.queue.task_done(0, {}, status())
self.drain(registration)
self.assertEqual(self.rows()[0]["outcome"], "succeeded")
with load_package(name, self.db.model.db_path, output, server) as (
package,
_,
):
registration = getattr(package, "_history_registration", None)
self.assertIsNotNone(
registration, "history setup depended on another hook"
)
self.assertIsNone(package._global_db)
self.wait_for(lambda: registration.writer.status().accepting)
self.dequeue(0)
self.queue.task_done(0, {}, status())
self.drain(registration)
self.assertEqual(self.rows()[0]["outcome"], "succeeded")
def test_distinct_actual_attempts_for_reused_job_and_cached_rerun(self):
registration = self.register()
+63
View File
@@ -155,6 +155,69 @@ class HistoryRestoreTests(unittest.TestCase):
self.assertEqual(db.get_execution_epoch(), epoch)
self.assertEqual(notices, [])
def test_extra_unique_constraints_reject_restore_without_touching_live_or_backup(
self,
):
from database.execution_history import subscribe_history_restore
db = self.database()
start = self.start(db)
db.write_execution_start(start)
row = db.get_execution_run(start.run_id)
with db.model.get_connection() as conn:
conn.execute("INSERT INTO prompts(text) VALUES('live library')")
notices = []
self.addCleanup(
subscribe_history_restore(self.path, lambda: notices.append(True))
)
for table_constraint in (False, True):
with self.subTest(table_constraint=table_constraint):
source = str(
Path(self.temp.name) / f"unique-backup-{table_constraint}.db"
)
self.assertTrue(db.model.backup_database(source))
with closing(sqlite3.connect(source)) as conn, conn:
if table_constraint:
sql = conn.execute(
"SELECT sql FROM sqlite_master "
"WHERE name='execution_runs'"
).fetchone()[0]
conn.execute("ALTER TABLE execution_runs RENAME TO old_runs")
conn.execute(sql[:-1] + ", UNIQUE(job_id))")
conn.execute(
"INSERT INTO execution_runs SELECT * FROM old_runs"
)
conn.execute("DROP TABLE old_runs")
conn.execute(
"CREATE UNIQUE INDEX idx_execution_runs_identity "
"ON execution_runs(session_id, queue_item_id)"
)
conn.execute(
"CREATE INDEX idx_execution_runs_observed "
"ON execution_runs(observed_dequeue_at, run_id)"
)
else:
conn.execute(
"CREATE UNIQUE INDEX unexpected_job_uniqueness "
"ON execution_runs(job_id)"
)
original = Path(source).read_bytes()
with self.assertRaisesRegex(ValueError, "uniqu"):
db.model.restore_from_file(source)
self.assertEqual(db.get_execution_epoch(), start.storage_epoch)
self.assertEqual(db.get_execution_run(start.run_id), row)
self.assertEqual(self.count(db), 1)
self.assertEqual(
db.model.get_connection()
.execute("SELECT text FROM prompts")
.fetchone()[0],
"live library",
)
self.assertEqual(Path(source).read_bytes(), original)
self.assertEqual(notices, [])
self.assertTrue(db.write_execution_start(self.start(db, item=1)))
self.assertEqual(self.count(db), 2)
def test_observer_failure_does_not_change_completed_restore(self):
from database.execution_history import subscribe_history_restore
+6 -12
View File
@@ -14,7 +14,7 @@ ROOT = os.path.dirname(TESTS_DIR)
sys.path.insert(0, ROOT)
sys.path.insert(0, TESTS_DIR)
from test_init_package import close_every_database, load_package # noqa: E402
from test_init_package import load_package # noqa: E402
class TestExtensionWebDir(unittest.TestCase):
@@ -23,21 +23,15 @@ class TestExtensionWebDir(unittest.TestCase):
@classmethod
def setUpClass(cls):
cls.tmp = tempfile.TemporaryDirectory()
cls.addClassCleanup(cls.tmp.cleanup)
output_dir = os.path.join(cls.tmp.name, "output")
os.makedirs(output_dir)
cls.package, _ = load_package(
cls.package_name, os.path.join(cls.tmp.name, "prompts.db"), output_dir
cls.package, _ = cls.enterClassContext(
load_package(
cls.package_name, os.path.join(cls.tmp.name, "prompts.db"), output_dir
)
)
@classmethod
def tearDownClass(cls):
monitor = getattr(cls.package, "_global_image_monitor", None)
if monitor is not None:
monitor.stop_monitoring()
sys.modules.pop(cls.package_name, None)
close_every_database() # the package's database must not hold the temp dir open
cls.tmp.cleanup()
def test_web_directory_is_the_canvas_extension_folder(self):
self.assertEqual(self.package.WEB_DIRECTORY, "web/comfy")
self.assertIn("WEB_DIRECTORY", self.package.__all__)
+211 -16
View File
@@ -4,7 +4,10 @@ The package is loaded the way ComfyUI loads it (``__init__`` with relative
imports) with ComfyUI's ``server`` and ``folder_paths`` modules stubbed.
"""
import contextlib
import importlib.util
import io
import logging
import json
import os
import sys
@@ -18,6 +21,10 @@ from aiohttp import web
ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
sys.path.insert(0, ROOT)
from utils.logging_config import get_logger_manager # noqa: E402
get_logger_manager()
try:
from tests.open_handles import open_handles_under # noqa: E402
except ImportError: # discovered with tests/ as the top-level directory
@@ -38,6 +45,7 @@ def fake_server_module():
routes=web.RouteTableDef(),
prompt_queue=FakePromptQueue(),
handlers=[],
app=types.SimpleNamespace(middlewares=[]),
)
instance.add_on_prompt_handler = instance.handlers.append
module = types.ModuleType("server")
@@ -54,10 +62,10 @@ def fake_folder_paths_module(output_dir):
return module
def load_package(name, db_path, output_dir, server=None):
def _import_package(name, db_path, output_dir, server=None):
"""Import ROOT/__init__.py as package ``name`` with ComfyUI stubbed.
Returns (package, server instance). The caller stops the image monitor.
Returns (package, server instance) to the scoped fixture.
"""
saved = {key: sys.modules.get(key) for key in ("server", "folder_paths")}
server = server or fake_server_module()
@@ -83,7 +91,7 @@ def load_package(name, db_path, output_dir, server=None):
try:
spec.loader.exec_module(package)
except BaseException:
sys.modules.pop(name, None)
cleanup_package(package)
raise
finally:
for key, module in saved.items():
@@ -94,6 +102,73 @@ def load_package(name, db_path, output_dir, server=None):
return package, server.PromptServer.instance
def cleanup_package(package):
"""Stop fixture-owned workers before releasing its other resources."""
registration = getattr(package, "_history_registration", None)
if registration is not None:
assert registration.dispose(3), "package history worker leaked"
monitor = getattr(package, "_global_image_monitor", None)
if monitor is not None:
monitor.stop_monitoring()
close_every_database()
sys.modules.pop(package.__name__, None)
@contextlib.contextmanager
def load_package(name, db_path, output_dir, server=None):
"""Dispose package workers before closing its isolated logging handlers."""
root_logger = logging.getLogger("prompt_manager")
loggers = [root_logger] + [
logger
for key, logger in list(logging.Logger.manager.loggerDict.items())
if key.startswith("prompt_manager.") and isinstance(logger, logging.Logger)
]
previous = {
logger: (
logger.handlers[:],
logger.level,
logger.propagate,
logger.disabled,
logger.filters[:],
)
for logger in loggers
}
prior_handlers = {handler for state in previous.values() for handler in state[0]}
root_logger.handlers = []
try:
package, instance = _import_package(name, db_path, output_dir, server)
# A previously imported package may reuse its existing logger singleton.
if not root_logger.handlers:
root_logger.handlers = previous[root_logger][0][:]
try:
yield package, instance
finally:
cleanup_package(package)
finally:
current_loggers = [root_logger] + [
logger
for key, logger in list(logging.Logger.manager.loggerDict.items())
if key.startswith("prompt_manager.") and isinstance(logger, logging.Logger)
]
created_handlers = {
handler
for logger in current_loggers
for handler in logger.handlers
if handler not in prior_handlers
}
for logger in current_loggers:
(
logger.handlers,
level,
logger.propagate,
logger.disabled,
logger.filters,
) = previous.get(logger, ([], logging.NOTSET, True, False, []))
logger.setLevel(level)
for handler in created_handlers:
handler.close()
def close_every_database():
"""Close connections of every PromptModel registry in the process.
@@ -111,11 +186,15 @@ class PackageTestCase(unittest.TestCase):
@classmethod
def setUpClass(cls):
cls.tmp = tempfile.TemporaryDirectory()
cls.addClassCleanup(cls.tmp.cleanup)
cls.addClassCleanup(cls.assert_no_open_handles)
cls.output_dir = os.path.join(cls.tmp.name, "output")
os.makedirs(cls.output_dir)
cls.db_path = cls.make_db_path(cls.tmp.name)
cls.package, cls.server = load_package(
cls.package_name, cls.db_path, cls.output_dir, server=cls.make_server()
cls.package, cls.server = cls.enterClassContext(
load_package(
cls.package_name, cls.db_path, cls.output_dir, server=cls.make_server()
)
)
@classmethod
@@ -127,14 +206,8 @@ class PackageTestCase(unittest.TestCase):
return fake_server_module()
@classmethod
def tearDownClass(cls):
monitor = getattr(cls.package, "_global_image_monitor", None)
if monitor is not None:
monitor.stop_monitoring()
sys.modules.pop(cls.package_name, None)
close_every_database()
def assert_no_open_handles(cls):
hits = open_handles_under(cls.tmp.name)
cls.tmp.cleanup()
assert not hits, f"files still open before temp dir cleanup: {hits}"
@@ -269,9 +342,6 @@ class TestConsoleBanner(unittest.TestCase):
"""Loading the node must not depend on the console being able to print emoji."""
def test_package_loads_with_a_cp1252_console(self):
import contextlib
import io
tmp = tempfile.TemporaryDirectory()
self.addCleanup(tmp.cleanup)
self.addCleanup(close_every_database)
@@ -281,7 +351,9 @@ class TestConsoleBanner(unittest.TestCase):
stream = io.TextIOWrapper(io.BytesIO(), encoding="cp1252", errors="strict")
with contextlib.redirect_stdout(stream):
package, _ = load_package(name, os.path.join(tmp.name, "p.db"), output_dir)
package, _ = self.enterContext(
load_package(name, os.path.join(tmp.name, "p.db"), output_dir)
)
self.addCleanup(sys.modules.pop, name, None)
monitor = getattr(package, "_global_image_monitor", None)
if monitor is not None:
@@ -290,3 +362,126 @@ class TestConsoleBanner(unittest.TestCase):
stream.flush()
banner = stream.buffer.getvalue().decode("cp1252")
self.assertIn("Loaded:", banner)
class TestPackageLoggerIsolation(unittest.TestCase):
def setUp(self):
self.tmp = tempfile.TemporaryDirectory()
self.addCleanup(self.tmp.cleanup)
self.logger = logging.getLogger("prompt_manager")
self.previous = (
self.logger.handlers[:],
self.logger.level,
self.logger.propagate,
self.logger.disabled,
)
self.addCleanup(self.restore_logging)
self.path = os.path.join(self.tmp.name, "prior.log")
self.handler = logging.FileHandler(self.path)
self.logger.handlers = [self.handler]
self.logger.setLevel(logging.DEBUG)
self.logger.propagate = True
self.logger.disabled = True
self.child = logging.getLogger("prompt_manager.database")
self.child_previous = self.child.level
self.addCleanup(self.child.setLevel, self.child_previous)
self.child.setLevel(logging.ERROR)
self.saved_modules = {
key: sys.modules.get(key) for key in ("server", "folder_paths")
}
self.saved_config = os.environ.get("PROMPT_MANAGER_CONFIG_PATH")
self.server = fake_server_module()
self.server.PromptServer.instance.prompt_queue.task_done = lambda *a, **k: None
def restore_logging(self):
for handler in self.logger.handlers:
if handler not in self.previous[0]:
handler.close()
self.handler.close()
(
self.logger.handlers,
self.logger.level,
self.logger.propagate,
self.logger.disabled,
) = self.previous
def assert_logging_restored(self, handlers, registration):
for key, module in self.saved_modules.items():
self.assertIs(sys.modules.get(key), module)
self.assertEqual(
os.environ.get("PROMPT_MANAGER_CONFIG_PATH"), self.saved_config
)
self.assertEqual(self.logger.handlers, [self.handler])
self.assertEqual(self.logger.level, logging.DEBUG)
self.assertTrue(self.logger.propagate)
self.assertTrue(self.logger.disabled)
self.assertEqual(self.child.level, logging.ERROR)
self.assertFalse(self.handler.stream.closed)
self.assertTrue(all(handler.stream is None for handler in handlers))
self.assertFalse(registration.writer.status().alive)
self.logger.disabled = False
self.logger.warning("prior logging still works")
self.logger.disabled = True
self.handler.flush()
with open(self.path, encoding="utf-8") as log:
self.assertIn("prior logging still works", log.read())
def test_package_scope_closes_its_handlers_after_worker_disposal(self):
with contextlib.redirect_stdout(io.StringIO()):
with load_package(
"prompt_manager_pkg_logger_scope",
os.path.join(self.tmp.name, "p.db"),
self.tmp.name,
self.server,
) as (package, _):
handlers = [
h
for h in self.logger.handlers
if isinstance(h, logging.FileHandler)
]
self.assertTrue(handlers)
registration = package._history_registration
original_dispose = registration.dispose
open_at_disposal = []
def dispose(timeout=2):
open_at_disposal.append(all(h.stream is not None for h in handlers))
return original_dispose(timeout)
registration.dispose = dispose
self.assertEqual(open_at_disposal, [True])
self.assert_logging_restored(handlers, registration)
def test_failed_package_import_closes_handlers_and_restores_logging(self):
name = "prompt_manager_pkg_logger_failure"
captured = {}
original_print = print
def fail_banner(*args, **kwargs):
if any("Total:" in str(arg) for arg in args):
captured["package"] = sys.modules[name]
captured["handlers"] = [
h
for h in self.logger.handlers
if isinstance(h, logging.FileHandler)
]
raise RuntimeError("injected banner failure")
return original_print(*args, **kwargs)
with (
contextlib.redirect_stdout(io.StringIO()),
mock.patch("builtins.print", side_effect=fail_banner),
):
with self.assertRaisesRegex(RuntimeError, "injected banner failure"):
with load_package(
name,
os.path.join(self.tmp.name, "p.db"),
self.tmp.name,
self.server,
):
self.fail("failed import yielded a package")
self.assertNotIn(name, sys.modules)
self.assertTrue(captured["handlers"])
self.assert_logging_restored(
captured["handlers"], captured["package"]._history_registration
)