2025-06-11 21:43:39 +01:00
|
|
|
import logging
|
|
|
|
|
import os
|
|
|
|
|
import shutil
|
|
|
|
|
from app.logger import log_startup_warning
|
|
|
|
|
from utils.install_util import get_missing_requirements_message
|
2026-03-07 17:37:25 -08:00
|
|
|
from filelock import FileLock, Timeout
|
2026-08-25 07:19:03 +08:00
|
|
|
from comfy.cli_args import args, database_default_path
|
2025-06-11 21:43:39 +01:00
|
|
|
|
|
|
|
|
_DB_AVAILABLE = False
|
|
|
|
|
Session = None
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
try:
|
|
|
|
|
from alembic import command
|
|
|
|
|
from alembic.config import Config
|
|
|
|
|
from alembic.runtime.migration import MigrationContext
|
|
|
|
|
from alembic.script import ScriptDirectory
|
2026-03-07 17:37:25 -08:00
|
|
|
from sqlalchemy import create_engine, event
|
2025-06-11 21:43:39 +01:00
|
|
|
from sqlalchemy.orm import sessionmaker
|
2026-03-07 17:37:25 -08:00
|
|
|
from sqlalchemy.pool import StaticPool
|
|
|
|
|
|
|
|
|
|
from app.database.models import Base
|
|
|
|
|
import app.assets.database.models # noqa: F401 — register models with Base.metadata
|
feat(assets): split asset records from content (#16295)
* review-stack 1/4: code (37 files, +3217/-3958)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: path not under tests-unit/ or tests/
Question: Is the logic change right?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 2/4: tests-removed (24 files, +274/-8220)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: test file deleted, or modified with deleted/(added+deleted) >= 0.9
Question: For each dropped assertion: obsolete by a ruling, or covered by a tests-new test?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 3/4: tests-changed (13 files, +1043/-1218)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: remaining modified test files (incl. conftest.py / helpers)
Question: Did the edits weaken an existing check?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 4/4: tests-new (46 files, +8601/-0)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: test file added
Question: Is the code layer well covered?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 5/6: code (13 files, +351/-104)
Review-and-land stack for synap5e/feat/assets-di, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: path not under tests-unit/ or tests/
Question: Is the logic change right?
Source tip: eca2c74bffcbb4481e90f33d5b7b4efeeb05eb45
Merge-base: 20d59d2a5f8108d714a908b73a9c1754b9c36f2a
* review-stack 6/6: tests (8 files, +753/-238)
Review-and-land stack for synap5e/feat/assets-di, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: every changed file under tests-unit/ or tests/ (added, modified, or deleted)
Question: Is the code layer well covered, and did any edit weaken an existing check?
Source tip: eca2c74bffcbb4481e90f33d5b7b4efeeb05eb45
Merge-base: 20d59d2a5f8108d714a908b73a9c1754b9c36f2a
* review-stack 7/8: ported-fixes (42 files, +1361/-180)
Review-and-land stack for synap5e/feat/assets-di-v2, generated by
review-stack.py conventions (hand-built continuation layer; see the PR body).
Once approved, merges DOWN into the layer below (a fast-forward); only the
bottom layer squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: the 11 base-branch fix/docs commits 595cd6e4..94d7185b cherry-picked across the DI refactor (7efdd1d7 excluded, superseded by layer 8)
Question: was each base fix ported faithfully across the DI refactor?
Source tip: 6841881069284803b902b4a9e33bdcda13126771
Merge-base: 7fdfb40f4b1d04c5b4b26ac7ab2328c83a8cb126
* review-stack 8/8: defensive-parity (4 files, +36/-3)
Review-and-land stack for synap5e/feat/assets-di-v2, generated by
review-stack.py conventions (hand-built continuation layer; see the PR body).
Once approved, merges DOWN into the layer below (a fast-forward); only the
bottom layer squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: match-or-improve master's dependency defenses — NoAssets selection when DB deps unavailable (7efdd1d7's outcome via the DI seam), requirements warning before assets imports, blake3 in the guarded dependency set
Question: does each degradation path now match or improve master's behavior?
Source tip: ebc2cfeebc5a1ebae407d0cb975afb5f293b4111
Merge-base: 7fdfb40f4b1d04c5b4b26ac7ab2328c83a8cb126
* fix(assets): only discard content rows this operation actually inserted
CR-9: Enumerated all six create_content call sites. Only scanner seeding and the three ingest registration paths track IDs for failure cleanup.
* fix(assets): reject hash-only uploads with FEATURE_DISABLED when hashing is off
CodeRabbit finding CR-2: reject hash-only multipart uploads before create_from_hash when hashing is disabled.
* fix(assets): seed persists the stat it verified
CR-7: persist the fresh seed-time restat instead of walk-time spec values.
* fix(assets): route database lock failures to the lock guidance
CR-16: route file-lock startup failures through the existing lock guidance and exit path.
* fix(assets): drop the inaccurate temp-cleanup claim from the shutdown warning
References CR-10.
* fix(assets): walk the output root after execution so undeclared outputs register promptly
Custom nodes that write files into the output directory without declaring
them in output_ui only became assets when the next full walk happened - a
frontend GET /object_info or a restart. Headless and API-only sessions never
trigger either, so those files never converged into the asset database.
The post-execution hook now requests a FULL scan of the output root instead of
an enrich-only pass. The seeder's pending-request queue was generalised from
enrich-specific to carrying a scan phase, so the request starts immediately
when the seeder is idle and coalesces (escalating to FULL on a phase mismatch)
when a scan is already running. queue_output_enrichment is renamed to
queue_output_scan across the protocol, the NoAssets no-op and the call site.
References FIX-6.
* chore(assets): remove seeder paths orphaned by the output-scan change
45c2f96e rerouted both former enrich call sites to start()/enqueue_scan(),
leaving two seeder methods that look live but are not. Review round F2
raised this along with four smaller items; the user's disposition was to
fix all six here.
- Delete start_enrich: zero callers repo-wide after 45c2f96e.
- Delete enqueue_enrich: no production callers; its ~18 call sites in
tests/test_asset_seeder.py move to enqueue_scan(phase=ScanPhase.ENRICH)
with their semantics unchanged. The deletion forces the half-done class
renames (TestEnqueueEnrich* -> TestEnqueueScan*, consistent with the
already-renamed TestPendingScanDrain) and restores the module docstring
that was dropped rather than reworded.
- Document at manager.queue_output_scan that ScanPhase.FULL per debounce
window is the deliberate, user-ratified trade, so it is not optimised
back to ENRICH without revisiting the decision.
- Document that SeedAssetSpec.size_bytes/mtime_ns are walk-time
diagnostics only - production persists the seed-time restat since CR-7.
- Export create_content_reporting_insert from the queries facade and fold
scanner.py's direct-module import into the existing facade block.
- Harden test_queue_output_scan_does_not_duplicate_declared_output against
a vacuous pass: it now asserts the seeder finished without errors and
that an undeclared sibling written into the same directory WAS
registered by the same scan, proving the walk actually ran.
No production behaviour changes beyond the two deletions.
References F2-cleanup.
* chore: comment cleanup
Comment-Gate: 18 quarantined
* fix(assets): preserve pause across the seeder's pending-scan drain
pause() runs before every prompt, while pending-scan enqueue and resume only run inside the debounced gc-interval gate. If the active scan finishes just after the next prompt's pause, its finally block resets the seeder to idle and the pending drain starts a replacement with the run gate open, so resume becomes a no-op.
Capture pausedness under the lock before resetting to idle, then start the drained scan already paused. Setting the state and gate before launching the thread avoids the start-then-reclear window and lets resume release the existing scan checkpoints.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* test(assets): pin job_id absence for scan-discovered assets
Owner ruling, recorded 2026-09-03 in the stack-9-hardening planning notepad: scan-discovered assets — including undeclared outputs found by the post-execution walk — carry job_id = None, always; only emission-time registration (output_ui declaration) attributes a job; attributing walk finds to the most recent prompt would be a temporal-correlation guess that is wrong exactly when prompts interleave; None is honest provenance. Do NOT add proximity-based attribution heuristics to the scanner. Ratified against Jacob Segal's cross-job-attribution concern (2026-09-08 review meeting) — a wrongly-attributed asset could mean one user's cloud job sees another user's asset.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* [review-stack 10/10] assets-tests (#16218)
* test(execution): run the battery with assets enabled and assert asset-system health at teardown
* test(execution): cover list-shaped outputs registering assets
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
---------
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* [review-stack 11/11] review-fixes (#16261)
* fix(assets): only exit on database file-lock timeout when assets are enabled
* test(assets): pin live_contents_under_prefixes path-filtering semantics
* perf(assets): push live-content prefix filtering into SQL
* test(assets): declare per-entry intent in the path-prefix corpus
* test(assets): normalize POSIX-literal path expectations for Windows
* test(assets): force observable stat changes and close-before-mutate on Windows-sensitive rewrites
* test(assets): force an observable mtime change in the hash-mode split test
---------
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Co-authored-by: guill <jacob.e.segal@gmail.com>
2026-09-14 07:04:51 +12:00
|
|
|
import blake3 # noqa: F401 — verify the hard dependency is importable at startup
|
2025-06-11 21:43:39 +01:00
|
|
|
|
|
|
|
|
_DB_AVAILABLE = True
|
|
|
|
|
except ImportError as e:
|
|
|
|
|
log_startup_warning(
|
|
|
|
|
f"""
|
|
|
|
|
------------------------------------------------------------------------
|
|
|
|
|
Error importing dependencies: {e}
|
|
|
|
|
{get_missing_requirements_message()}
|
|
|
|
|
This error is happening because ComfyUI now uses a local sqlite database.
|
|
|
|
|
------------------------------------------------------------------------
|
|
|
|
|
""".strip()
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def dependencies_available():
|
|
|
|
|
"""
|
|
|
|
|
Temporary function to check if the dependencies are available
|
|
|
|
|
"""
|
|
|
|
|
return _DB_AVAILABLE
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def can_create_session():
|
|
|
|
|
"""
|
|
|
|
|
Temporary function to check if the database is available to create a session
|
|
|
|
|
During initial release there may be environmental issues (or missing dependencies) that prevent the database from being created
|
|
|
|
|
"""
|
|
|
|
|
return dependencies_available() and Session is not None
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def get_alembic_config():
|
|
|
|
|
root_path = os.path.join(os.path.dirname(__file__), "../..")
|
|
|
|
|
config_path = os.path.abspath(os.path.join(root_path, "alembic.ini"))
|
|
|
|
|
scripts_path = os.path.abspath(os.path.join(root_path, "alembic_db"))
|
|
|
|
|
|
|
|
|
|
config = Config(config_path)
|
|
|
|
|
config.set_main_option("script_location", scripts_path)
|
2026-08-25 07:19:03 +08:00
|
|
|
config.set_main_option("sqlalchemy.url", get_database_url())
|
2025-06-11 21:43:39 +01:00
|
|
|
|
|
|
|
|
return config
|
|
|
|
|
|
|
|
|
|
|
2026-08-25 07:19:03 +08:00
|
|
|
def get_database_url():
|
|
|
|
|
if args.database_url is not None:
|
|
|
|
|
return args.database_url
|
|
|
|
|
|
|
|
|
|
import folder_paths
|
|
|
|
|
|
|
|
|
|
db_path = os.path.join(folder_paths.get_user_directory(), "comfyui.db")
|
|
|
|
|
return f"sqlite:///{db_path}"
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def get_legacy_default_db_path():
|
|
|
|
|
return database_default_path
|
|
|
|
|
|
|
|
|
|
|
2025-06-11 21:43:39 +01:00
|
|
|
def get_db_path():
|
2026-08-25 07:19:03 +08:00
|
|
|
url = get_database_url()
|
2025-06-11 21:43:39 +01:00
|
|
|
if url.startswith("sqlite:///"):
|
2026-08-25 07:19:03 +08:00
|
|
|
return url.split("///", 1)[1]
|
2025-06-11 21:43:39 +01:00
|
|
|
else:
|
|
|
|
|
raise ValueError(f"Unsupported database URL '{url}'.")
|
|
|
|
|
|
|
|
|
|
|
2026-08-25 07:19:03 +08:00
|
|
|
def copy_legacy_default_db(db_path):
|
|
|
|
|
if args.database_url is not None:
|
|
|
|
|
return
|
|
|
|
|
|
|
|
|
|
legacy_db_path = get_legacy_default_db_path()
|
|
|
|
|
if legacy_db_path is None:
|
|
|
|
|
return
|
|
|
|
|
|
|
|
|
|
if os.path.abspath(legacy_db_path) == os.path.abspath(db_path):
|
|
|
|
|
return
|
|
|
|
|
|
|
|
|
|
if os.path.exists(db_path) or not os.path.exists(legacy_db_path):
|
|
|
|
|
return
|
|
|
|
|
|
|
|
|
|
backup_path = legacy_db_path + ".bak"
|
|
|
|
|
if os.path.exists(backup_path):
|
|
|
|
|
return
|
|
|
|
|
|
|
|
|
|
os.replace(legacy_db_path, backup_path)
|
|
|
|
|
shutil.copy(backup_path, db_path)
|
|
|
|
|
logging.info(
|
|
|
|
|
f"Renamed legacy database '{legacy_db_path}' to '{backup_path}' and copied it to '{db_path}'"
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def prepare_file_db_path(db_path):
|
|
|
|
|
db_dir = os.path.dirname(db_path)
|
|
|
|
|
if db_dir:
|
|
|
|
|
os.makedirs(db_dir, exist_ok=True)
|
|
|
|
|
|
|
|
|
|
copy_legacy_default_db(db_path)
|
|
|
|
|
|
|
|
|
|
|
2026-03-07 17:37:25 -08:00
|
|
|
_db_lock = None
|
|
|
|
|
|
|
|
|
|
def _acquire_file_lock(db_path):
|
|
|
|
|
"""Acquire an OS-level file lock to prevent multi-process access.
|
|
|
|
|
|
|
|
|
|
Uses filelock for cross-platform support (macOS, Linux, Windows).
|
|
|
|
|
The OS automatically releases the lock when the process exits, even on crashes.
|
|
|
|
|
"""
|
|
|
|
|
global _db_lock
|
|
|
|
|
lock_path = db_path + ".lock"
|
|
|
|
|
_db_lock = FileLock(lock_path)
|
|
|
|
|
try:
|
|
|
|
|
_db_lock.acquire(timeout=0)
|
|
|
|
|
except Timeout:
|
|
|
|
|
raise RuntimeError(
|
|
|
|
|
f"Could not acquire lock on database '{db_path}'. "
|
|
|
|
|
"Another ComfyUI process may already be using it. "
|
|
|
|
|
"Use --database-url to specify a separate database file."
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def _is_memory_db(db_url):
|
|
|
|
|
"""Check if the database URL refers to an in-memory SQLite database."""
|
|
|
|
|
return db_url in ("sqlite:///:memory:", "sqlite://")
|
|
|
|
|
|
|
|
|
|
|
2025-06-11 21:43:39 +01:00
|
|
|
def init_db():
|
2026-08-25 07:19:03 +08:00
|
|
|
db_url = get_database_url()
|
2025-06-11 21:43:39 +01:00
|
|
|
logging.debug(f"Database URL: {db_url}")
|
2026-03-07 17:37:25 -08:00
|
|
|
|
|
|
|
|
if _is_memory_db(db_url):
|
|
|
|
|
_init_memory_db(db_url)
|
|
|
|
|
else:
|
|
|
|
|
_init_file_db(db_url)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def _init_memory_db(db_url):
|
|
|
|
|
"""Initialize an in-memory SQLite database using metadata.create_all.
|
|
|
|
|
|
|
|
|
|
Alembic migrations don't work with in-memory SQLite because each
|
|
|
|
|
connection gets its own separate database — tables created by Alembic's
|
|
|
|
|
internal connection are lost immediately.
|
|
|
|
|
"""
|
|
|
|
|
engine = create_engine(
|
|
|
|
|
db_url,
|
|
|
|
|
poolclass=StaticPool,
|
|
|
|
|
connect_args={"check_same_thread": False},
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
@event.listens_for(engine, "connect")
|
|
|
|
|
def set_sqlite_pragma(dbapi_connection, connection_record):
|
|
|
|
|
cursor = dbapi_connection.cursor()
|
|
|
|
|
cursor.execute("PRAGMA foreign_keys=ON")
|
|
|
|
|
cursor.close()
|
|
|
|
|
|
|
|
|
|
Base.metadata.create_all(engine)
|
|
|
|
|
|
|
|
|
|
global Session
|
|
|
|
|
Session = sessionmaker(bind=engine)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def _init_file_db(db_url):
|
|
|
|
|
"""Initialize a file-backed SQLite database using Alembic migrations."""
|
2025-06-11 21:43:39 +01:00
|
|
|
db_path = get_db_path()
|
2026-08-25 07:19:03 +08:00
|
|
|
prepare_file_db_path(db_path)
|
2025-06-11 21:43:39 +01:00
|
|
|
db_exists = os.path.exists(db_path)
|
|
|
|
|
|
feat(assets): split asset records from content (#16295)
* review-stack 1/4: code (37 files, +3217/-3958)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: path not under tests-unit/ or tests/
Question: Is the logic change right?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 2/4: tests-removed (24 files, +274/-8220)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: test file deleted, or modified with deleted/(added+deleted) >= 0.9
Question: For each dropped assertion: obsolete by a ruling, or covered by a tests-new test?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 3/4: tests-changed (13 files, +1043/-1218)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: remaining modified test files (incl. conftest.py / helpers)
Question: Did the edits weaken an existing check?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 4/4: tests-new (46 files, +8601/-0)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: test file added
Question: Is the code layer well covered?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 5/6: code (13 files, +351/-104)
Review-and-land stack for synap5e/feat/assets-di, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: path not under tests-unit/ or tests/
Question: Is the logic change right?
Source tip: eca2c74bffcbb4481e90f33d5b7b4efeeb05eb45
Merge-base: 20d59d2a5f8108d714a908b73a9c1754b9c36f2a
* review-stack 6/6: tests (8 files, +753/-238)
Review-and-land stack for synap5e/feat/assets-di, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: every changed file under tests-unit/ or tests/ (added, modified, or deleted)
Question: Is the code layer well covered, and did any edit weaken an existing check?
Source tip: eca2c74bffcbb4481e90f33d5b7b4efeeb05eb45
Merge-base: 20d59d2a5f8108d714a908b73a9c1754b9c36f2a
* review-stack 7/8: ported-fixes (42 files, +1361/-180)
Review-and-land stack for synap5e/feat/assets-di-v2, generated by
review-stack.py conventions (hand-built continuation layer; see the PR body).
Once approved, merges DOWN into the layer below (a fast-forward); only the
bottom layer squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: the 11 base-branch fix/docs commits 595cd6e4..94d7185b cherry-picked across the DI refactor (7efdd1d7 excluded, superseded by layer 8)
Question: was each base fix ported faithfully across the DI refactor?
Source tip: 6841881069284803b902b4a9e33bdcda13126771
Merge-base: 7fdfb40f4b1d04c5b4b26ac7ab2328c83a8cb126
* review-stack 8/8: defensive-parity (4 files, +36/-3)
Review-and-land stack for synap5e/feat/assets-di-v2, generated by
review-stack.py conventions (hand-built continuation layer; see the PR body).
Once approved, merges DOWN into the layer below (a fast-forward); only the
bottom layer squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: match-or-improve master's dependency defenses — NoAssets selection when DB deps unavailable (7efdd1d7's outcome via the DI seam), requirements warning before assets imports, blake3 in the guarded dependency set
Question: does each degradation path now match or improve master's behavior?
Source tip: ebc2cfeebc5a1ebae407d0cb975afb5f293b4111
Merge-base: 7fdfb40f4b1d04c5b4b26ac7ab2328c83a8cb126
* fix(assets): only discard content rows this operation actually inserted
CR-9: Enumerated all six create_content call sites. Only scanner seeding and the three ingest registration paths track IDs for failure cleanup.
* fix(assets): reject hash-only uploads with FEATURE_DISABLED when hashing is off
CodeRabbit finding CR-2: reject hash-only multipart uploads before create_from_hash when hashing is disabled.
* fix(assets): seed persists the stat it verified
CR-7: persist the fresh seed-time restat instead of walk-time spec values.
* fix(assets): route database lock failures to the lock guidance
CR-16: route file-lock startup failures through the existing lock guidance and exit path.
* fix(assets): drop the inaccurate temp-cleanup claim from the shutdown warning
References CR-10.
* fix(assets): walk the output root after execution so undeclared outputs register promptly
Custom nodes that write files into the output directory without declaring
them in output_ui only became assets when the next full walk happened - a
frontend GET /object_info or a restart. Headless and API-only sessions never
trigger either, so those files never converged into the asset database.
The post-execution hook now requests a FULL scan of the output root instead of
an enrich-only pass. The seeder's pending-request queue was generalised from
enrich-specific to carrying a scan phase, so the request starts immediately
when the seeder is idle and coalesces (escalating to FULL on a phase mismatch)
when a scan is already running. queue_output_enrichment is renamed to
queue_output_scan across the protocol, the NoAssets no-op and the call site.
References FIX-6.
* chore(assets): remove seeder paths orphaned by the output-scan change
45c2f96e rerouted both former enrich call sites to start()/enqueue_scan(),
leaving two seeder methods that look live but are not. Review round F2
raised this along with four smaller items; the user's disposition was to
fix all six here.
- Delete start_enrich: zero callers repo-wide after 45c2f96e.
- Delete enqueue_enrich: no production callers; its ~18 call sites in
tests/test_asset_seeder.py move to enqueue_scan(phase=ScanPhase.ENRICH)
with their semantics unchanged. The deletion forces the half-done class
renames (TestEnqueueEnrich* -> TestEnqueueScan*, consistent with the
already-renamed TestPendingScanDrain) and restores the module docstring
that was dropped rather than reworded.
- Document at manager.queue_output_scan that ScanPhase.FULL per debounce
window is the deliberate, user-ratified trade, so it is not optimised
back to ENRICH without revisiting the decision.
- Document that SeedAssetSpec.size_bytes/mtime_ns are walk-time
diagnostics only - production persists the seed-time restat since CR-7.
- Export create_content_reporting_insert from the queries facade and fold
scanner.py's direct-module import into the existing facade block.
- Harden test_queue_output_scan_does_not_duplicate_declared_output against
a vacuous pass: it now asserts the seeder finished without errors and
that an undeclared sibling written into the same directory WAS
registered by the same scan, proving the walk actually ran.
No production behaviour changes beyond the two deletions.
References F2-cleanup.
* chore: comment cleanup
Comment-Gate: 18 quarantined
* fix(assets): preserve pause across the seeder's pending-scan drain
pause() runs before every prompt, while pending-scan enqueue and resume only run inside the debounced gc-interval gate. If the active scan finishes just after the next prompt's pause, its finally block resets the seeder to idle and the pending drain starts a replacement with the run gate open, so resume becomes a no-op.
Capture pausedness under the lock before resetting to idle, then start the drained scan already paused. Setting the state and gate before launching the thread avoids the start-then-reclear window and lets resume release the existing scan checkpoints.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* test(assets): pin job_id absence for scan-discovered assets
Owner ruling, recorded 2026-09-03 in the stack-9-hardening planning notepad: scan-discovered assets — including undeclared outputs found by the post-execution walk — carry job_id = None, always; only emission-time registration (output_ui declaration) attributes a job; attributing walk finds to the most recent prompt would be a temporal-correlation guess that is wrong exactly when prompts interleave; None is honest provenance. Do NOT add proximity-based attribution heuristics to the scanner. Ratified against Jacob Segal's cross-job-attribution concern (2026-09-08 review meeting) — a wrongly-attributed asset could mean one user's cloud job sees another user's asset.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* [review-stack 10/10] assets-tests (#16218)
* test(execution): run the battery with assets enabled and assert asset-system health at teardown
* test(execution): cover list-shaped outputs registering assets
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
---------
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* [review-stack 11/11] review-fixes (#16261)
* fix(assets): only exit on database file-lock timeout when assets are enabled
* test(assets): pin live_contents_under_prefixes path-filtering semantics
* perf(assets): push live-content prefix filtering into SQL
* test(assets): declare per-entry intent in the path-prefix corpus
* test(assets): normalize POSIX-literal path expectations for Windows
* test(assets): force observable stat changes and close-before-mutate on Windows-sensitive rewrites
* test(assets): force an observable mtime change in the hash-mode split test
---------
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Co-authored-by: guill <jacob.e.segal@gmail.com>
2026-09-14 07:04:51 +12:00
|
|
|
# Lock BEFORE any migration work — deliberately diverging from upstream master, whose
|
|
|
|
|
# "it would block Alembic" rationale is false (the lock guards a separate `<db>.lock`
|
|
|
|
|
# file). Only this order makes revision inspection, backup, upgrade and the failure-path
|
|
|
|
|
# restore mutually exclusive between processes.
|
|
|
|
|
_acquire_file_lock(db_path)
|
|
|
|
|
try:
|
|
|
|
|
_migrate_and_bind(db_url, db_path, db_exists)
|
|
|
|
|
except Exception:
|
|
|
|
|
_db_lock.release()
|
|
|
|
|
raise
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
_DESTRUCTIVE_REVISION = "0007_record_content_split"
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def _upgrade_discards_the_catalog(script, target_rev, current_rev):
|
|
|
|
|
return any(
|
|
|
|
|
revision.revision == _DESTRUCTIVE_REVISION
|
|
|
|
|
for revision in script.iterate_revisions(upper=target_rev, lower=current_rev)
|
|
|
|
|
)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def _migrate_and_bind(db_url, db_path, db_exists):
|
2025-06-11 21:43:39 +01:00
|
|
|
config = get_alembic_config()
|
|
|
|
|
|
|
|
|
|
# Check if we need to upgrade
|
|
|
|
|
engine = create_engine(db_url)
|
2026-03-07 17:37:25 -08:00
|
|
|
|
|
|
|
|
# Enable foreign key enforcement for SQLite
|
|
|
|
|
@event.listens_for(engine, "connect")
|
|
|
|
|
def set_sqlite_pragma(dbapi_connection, connection_record):
|
|
|
|
|
cursor = dbapi_connection.cursor()
|
|
|
|
|
cursor.execute("PRAGMA foreign_keys=ON")
|
|
|
|
|
cursor.close()
|
|
|
|
|
|
2025-06-11 21:43:39 +01:00
|
|
|
conn = engine.connect()
|
|
|
|
|
|
|
|
|
|
context = MigrationContext.configure(conn)
|
|
|
|
|
current_rev = context.get_current_revision()
|
|
|
|
|
|
|
|
|
|
script = ScriptDirectory.from_config(config)
|
|
|
|
|
target_rev = script.get_current_head()
|
|
|
|
|
|
|
|
|
|
if target_rev is None:
|
|
|
|
|
logging.warning("No target revision found.")
|
|
|
|
|
elif current_rev != target_rev:
|
|
|
|
|
# Backup the database pre upgrade
|
|
|
|
|
backup_path = db_path + ".bkp"
|
|
|
|
|
if db_exists:
|
|
|
|
|
shutil.copy(db_path, backup_path)
|
|
|
|
|
else:
|
|
|
|
|
backup_path = None
|
|
|
|
|
|
|
|
|
|
try:
|
|
|
|
|
command.upgrade(config, target_rev)
|
|
|
|
|
logging.info(f"Database upgraded from {current_rev} to {target_rev}")
|
|
|
|
|
except Exception as e:
|
|
|
|
|
if backup_path:
|
|
|
|
|
# Restore the database from backup if upgrade fails
|
|
|
|
|
shutil.copy(backup_path, db_path)
|
|
|
|
|
os.remove(backup_path)
|
|
|
|
|
logging.exception("Error upgrading database: ")
|
|
|
|
|
raise e
|
|
|
|
|
|
feat(assets): split asset records from content (#16295)
* review-stack 1/4: code (37 files, +3217/-3958)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: path not under tests-unit/ or tests/
Question: Is the logic change right?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 2/4: tests-removed (24 files, +274/-8220)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: test file deleted, or modified with deleted/(added+deleted) >= 0.9
Question: For each dropped assertion: obsolete by a ruling, or covered by a tests-new test?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 3/4: tests-changed (13 files, +1043/-1218)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: remaining modified test files (incl. conftest.py / helpers)
Question: Did the edits weaken an existing check?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 4/4: tests-new (46 files, +8601/-0)
Review-and-land stack for synap5e/feat/asset-record-content-split, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: test file added
Question: Is the code layer well covered?
Source tip: 7007d185825751a09f26bf7ba79f146dc2fbda74
Merge-base: 783545f689a0af730065994b46b382ae24844c99
* review-stack 5/6: code (13 files, +351/-104)
Review-and-land stack for synap5e/feat/assets-di, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: path not under tests-unit/ or tests/
Question: Is the logic change right?
Source tip: eca2c74bffcbb4481e90f33d5b7b4efeeb05eb45
Merge-base: 20d59d2a5f8108d714a908b73a9c1754b9c36f2a
* review-stack 6/6: tests (8 files, +753/-238)
Review-and-land stack for synap5e/feat/assets-di, generated by review-stack.py. Once approved,
merges DOWN into the layer below (a fast-forward); only the bottom layer
squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: every changed file under tests-unit/ or tests/ (added, modified, or deleted)
Question: Is the code layer well covered, and did any edit weaken an existing check?
Source tip: eca2c74bffcbb4481e90f33d5b7b4efeeb05eb45
Merge-base: 20d59d2a5f8108d714a908b73a9c1754b9c36f2a
* review-stack 7/8: ported-fixes (42 files, +1361/-180)
Review-and-land stack for synap5e/feat/assets-di-v2, generated by
review-stack.py conventions (hand-built continuation layer; see the PR body).
Once approved, merges DOWN into the layer below (a fast-forward); only the
bottom layer squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: the 11 base-branch fix/docs commits 595cd6e4..94d7185b cherry-picked across the DI refactor (7efdd1d7 excluded, superseded by layer 8)
Question: was each base fix ported faithfully across the DI refactor?
Source tip: 6841881069284803b902b4a9e33bdcda13126771
Merge-base: 7fdfb40f4b1d04c5b4b26ac7ab2328c83a8cb126
* review-stack 8/8: defensive-parity (4 files, +36/-3)
Review-and-land stack for synap5e/feat/assets-di-v2, generated by
review-stack.py conventions (hand-built continuation layer; see the PR body).
Once approved, merges DOWN into the layer below (a fast-forward); only the
bottom layer squash-merges into the real base. See ~/adocs/review-stack.md.
Rule: match-or-improve master's dependency defenses — NoAssets selection when DB deps unavailable (7efdd1d7's outcome via the DI seam), requirements warning before assets imports, blake3 in the guarded dependency set
Question: does each degradation path now match or improve master's behavior?
Source tip: ebc2cfeebc5a1ebae407d0cb975afb5f293b4111
Merge-base: 7fdfb40f4b1d04c5b4b26ac7ab2328c83a8cb126
* fix(assets): only discard content rows this operation actually inserted
CR-9: Enumerated all six create_content call sites. Only scanner seeding and the three ingest registration paths track IDs for failure cleanup.
* fix(assets): reject hash-only uploads with FEATURE_DISABLED when hashing is off
CodeRabbit finding CR-2: reject hash-only multipart uploads before create_from_hash when hashing is disabled.
* fix(assets): seed persists the stat it verified
CR-7: persist the fresh seed-time restat instead of walk-time spec values.
* fix(assets): route database lock failures to the lock guidance
CR-16: route file-lock startup failures through the existing lock guidance and exit path.
* fix(assets): drop the inaccurate temp-cleanup claim from the shutdown warning
References CR-10.
* fix(assets): walk the output root after execution so undeclared outputs register promptly
Custom nodes that write files into the output directory without declaring
them in output_ui only became assets when the next full walk happened - a
frontend GET /object_info or a restart. Headless and API-only sessions never
trigger either, so those files never converged into the asset database.
The post-execution hook now requests a FULL scan of the output root instead of
an enrich-only pass. The seeder's pending-request queue was generalised from
enrich-specific to carrying a scan phase, so the request starts immediately
when the seeder is idle and coalesces (escalating to FULL on a phase mismatch)
when a scan is already running. queue_output_enrichment is renamed to
queue_output_scan across the protocol, the NoAssets no-op and the call site.
References FIX-6.
* chore(assets): remove seeder paths orphaned by the output-scan change
45c2f96e rerouted both former enrich call sites to start()/enqueue_scan(),
leaving two seeder methods that look live but are not. Review round F2
raised this along with four smaller items; the user's disposition was to
fix all six here.
- Delete start_enrich: zero callers repo-wide after 45c2f96e.
- Delete enqueue_enrich: no production callers; its ~18 call sites in
tests/test_asset_seeder.py move to enqueue_scan(phase=ScanPhase.ENRICH)
with their semantics unchanged. The deletion forces the half-done class
renames (TestEnqueueEnrich* -> TestEnqueueScan*, consistent with the
already-renamed TestPendingScanDrain) and restores the module docstring
that was dropped rather than reworded.
- Document at manager.queue_output_scan that ScanPhase.FULL per debounce
window is the deliberate, user-ratified trade, so it is not optimised
back to ENRICH without revisiting the decision.
- Document that SeedAssetSpec.size_bytes/mtime_ns are walk-time
diagnostics only - production persists the seed-time restat since CR-7.
- Export create_content_reporting_insert from the queries facade and fold
scanner.py's direct-module import into the existing facade block.
- Harden test_queue_output_scan_does_not_duplicate_declared_output against
a vacuous pass: it now asserts the seeder finished without errors and
that an undeclared sibling written into the same directory WAS
registered by the same scan, proving the walk actually ran.
No production behaviour changes beyond the two deletions.
References F2-cleanup.
* chore: comment cleanup
Comment-Gate: 18 quarantined
* fix(assets): preserve pause across the seeder's pending-scan drain
pause() runs before every prompt, while pending-scan enqueue and resume only run inside the debounced gc-interval gate. If the active scan finishes just after the next prompt's pause, its finally block resets the seeder to idle and the pending drain starts a replacement with the run gate open, so resume becomes a no-op.
Capture pausedness under the lock before resetting to idle, then start the drained scan already paused. Setting the state and gate before launching the thread avoids the start-then-reclear window and lets resume release the existing scan checkpoints.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* test(assets): pin job_id absence for scan-discovered assets
Owner ruling, recorded 2026-09-03 in the stack-9-hardening planning notepad: scan-discovered assets — including undeclared outputs found by the post-execution walk — carry job_id = None, always; only emission-time registration (output_ui declaration) attributes a job; attributing walk finds to the most recent prompt would be a temporal-correlation guess that is wrong exactly when prompts interleave; None is honest provenance. Do NOT add proximity-based attribution heuristics to the scanner. Ratified against Jacob Segal's cross-job-attribution concern (2026-09-08 review meeting) — a wrongly-attributed asset could mean one user's cloud job sees another user's asset.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* [review-stack 10/10] assets-tests (#16218)
* test(execution): run the battery with assets enabled and assert asset-system health at teardown
* test(execution): cover list-shaped outputs registering assets
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
---------
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
* [review-stack 11/11] review-fixes (#16261)
* fix(assets): only exit on database file-lock timeout when assets are enabled
* test(assets): pin live_contents_under_prefixes path-filtering semantics
* perf(assets): push live-content prefix filtering into SQL
* test(assets): declare per-entry intent in the path-prefix corpus
* test(assets): normalize POSIX-literal path expectations for Windows
* test(assets): force observable stat changes and close-before-mutate on Windows-sensitive rewrites
* test(assets): force an observable mtime change in the hash-mode split test
---------
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Co-authored-by: guill <jacob.e.segal@gmail.com>
2026-09-14 07:04:51 +12:00
|
|
|
if backup_path and _upgrade_discards_the_catalog(script, target_rev, current_rev):
|
|
|
|
|
log_startup_warning(
|
|
|
|
|
f"The asset catalog was rebuilt from scratch by migration "
|
|
|
|
|
f"{_DESTRUCTIVE_REVISION}: manual tags, user metadata, previews, renames, "
|
|
|
|
|
f"API-created records and job_id links from the previous database were "
|
|
|
|
|
f"discarded. The database from before the upgrade was kept at {backup_path}."
|
|
|
|
|
)
|
|
|
|
|
|
2026-03-07 17:37:25 -08:00
|
|
|
conn.close()
|
|
|
|
|
|
2025-06-11 21:43:39 +01:00
|
|
|
global Session
|
|
|
|
|
Session = sessionmaker(bind=engine)
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
def create_session():
|
|
|
|
|
return Session()
|