3 Commits

Include renames

Author	SHA1	Message	Date
Simon Pinfoldandguill	02ba106f97	fix(assets): harden the asset catalogue's failure paths (#16393) ... * chore(assets): drop the unused asset_meta table from migration 0007 * fix(assets): drop asset_meta when downgrading a database that already created it * docs(assets): clarify asset schema docstring * test(assets): assert alembic and ORM index parity for the surviving asset tables * test(assets): cover asset system state index parity * refactor(tests): hoist migration-0007 test imports to module scope * fix(assets): guard the hashing dependency and chain the real import error * fix(assets): always resume background scanning when prompt handling fails * fix(api): derive the assets feature flag from the selected manager * fix(assets): paginate enrichment by id cursor so failures cannot starve or overflow the query * refactor(assets): extract prompt_worker so its resume contract is testable in-process * refactor(assets): test the blake3 import guard in-process instead of via subprocess * fix(assets): advance the enrichment cursor only past rows the batch attempted * fix(assets): track the scan pause across prompt worker iterations * chore(assets): address review follow-ups in the hashing guard, feature flags, and pagination pin * docs(assets): describe enrichment rows as attempted rather than selected The cursor holds at the last row a batch actually attempted, so a pause ends a batch early and the rows behind it are selected again when the scan resumes. Only the attempt is capped at once per pass. * test(api): derive the expected assets flag from the manager under test The assertion asked for the flag with no argument, so it read the parameter default rather than anything the manager reported - in a test whose subject is the two agreeing. Passing the manager's own state keeps it honest if the setup ever yields an enabled manager. * test(assets): let a broken prompt worker import fail instead of skipping The fixture wrapped importlib.import_module in a bare except that called pytest.skip, so a circular import, a missing dependency or a syntax error in app/prompt_worker.py would retire all four resume-contract tests while CI stayed green. The whole premise of extracting the module is that main.py can import it, so an import failure has to be a collection error. The CPU guard is genuinely load-bearing — comfy.model_management selects its device at import time and a CUDA build with no driver raises there — so it is kept, but as a precondition rather than an exception handler, matching the args.cpu-before-import convention already used by the comfy_test and comfy_api_nodes_test modules. Nothing is caught now. * docs(assets): name the unattempted rows instead of the ones behind the cursor The cursor moves forward through ascending ids, so 'the rows behind it' reads as the rows already passed - the opposite of what is selected again. * fix(assets): only absorb duplicate-path races when seeding scanned assets * fix(db): copy the legacy database inside the process lock * fix(assets): drop watch-list entries on stat errors instead of aborting the scan * fix(assets): clean up temp uploads on validation failures Multipart parsing writes the uploaded bytes to a temporary file before it validates the remaining form fields, so a request rejected after its file part had already been read left the temp file and its uuid directory on disk. Routing those removals through delete_temp_file_if_exists also changes the success path. The previous helper returned early when the temp file was already gone, so it never reached the parent rmdir; the shared helper attempts the rmdir unconditionally. Moving the upload to its destination leaves the temp path absent, so a successful upload now also discards its empty uuid directory, closing a pre-existing leak. * fix(assets): keep updated_at stable on no-op renames * docs(assets): make module docstrings and the rebuild warning truthful * fix(assets): correct event-log status snapshots and failure telemetry * test(assets): make the keyset tie-breaker and temp-exclusion tests falsifiable * chore: comment cleanup Comment-Gate: 3 quarantined * fix(assets): keep unreadable filesystem metadata from failing a whole scan batch * fix(assets): remove temporary uploads on non-UploadError failures * fix(assets): preserve successful specs when a scan batch fault propagates * test(db): drop the inert legacy-copy patch from the path preparation tests prepare_file_db_path no longer copies the legacy database - that moved inside the process lock in _init_file_db - so patching copy_legacy_default_db here did nothing. Leaving it implied a side effect the function does not have, and would have masked one if it were reintroduced. * fix(assets): report specs committed before a batch fault and preserve the fault itself * fix(assets): distinguish partial batch insert failures * refactor(assets): collapse the duplicated batch fault deferral into seed_asset_specs * fix(assets): reject duplicate file parts instead of stranding the first upload * refactor(assets): reap empty upload directories without importing the API layer * fix(assets): emit the invalid-mtime event once per scan Every other per-file emit on this scan path is gated -- mark_emitted( "stat_failed:enrich"), "hash_discarded_modified", "hash_failed", "enrich_failed" -- but scanner.invalid_mtime fired per file, so a restored archive or a FAT volume of pre-epoch mtimes put one structured event per file into the stream the closed vocabulary exists to keep parseable. Counted and emitted once, carrying the count. seed_asset_specs receives no _ScanProgress object and neither does insert_asset_specs above it, so routing this through mark_emitted would mean changing both signatures plus the seeder call site; the count form needs neither and the event is now strictly more informative than N identical fieldless lines. The per-file logging.warning is unchanged, and the emit stays inside seed_asset_specs so the static call-site manifest still matches. test_seed_skips_negative_fresh_mtime_with_warning_and_telemetry now pins the full list of invalid_mtime lines to exactly ["... count=1"] instead of asserting one such line exists -- a strictly stronger assertion, and the only change the new field required. * fix(assets): keep spec construction failures from wedging the watch list get_name_and_tags_from_asset_path raises ValueError by contract when a path stops resolving to a configured root, and it sat outside the guard, as did compute_loader_path and mimetypes.guess_type. An escape skipped the _WATCH_LIST[:] = remaining write at the end, so drained entries stayed on the list and were re-attempted every tick while entries past the fault never reached the increment _WATCH_SCAN_RETRIES needs to retire them. The list wedged permanently. Spec construction is now inside a guard that drops just the offending entry, and the list write moved into a finally so no future escape can skip it. The loop walks an iterator rather than the list, so the finally can put back the entries it never reached instead of discarding them. New event name rather than reusing one: scanner.watch_seed_failed is emitted only when seed_asset_specs returns an error, and widening it to also mean "never got as far as seeding" would make it lie -- a consumer treating it as a database-health signal would get false positives from what is really a path layout problem. scanner.watch_spec_failed is registered in ALLOWED_EVENTS and in the static call-site manifest. * refactor(assets): export the live-path conflict check as public API scanner.py reached past the package's own re-export surface to import _is_live_path_conflict directly out of records.py. The underscore said module-private while the import said otherwise, and records.py deliberately publishes its public names through app.assets.database.queries -- which the same import block three lines above was already using. The use is correct and unchanged; only the name and the route change. Renamed to is_live_path_conflict, listed in the package __init__ import and __all__ alongside its siblings, and scanner.py now takes it from the package like everything else it imports from there. * docs(db): restore the rationale for locking before migration Commit 1dbcdcd7 and the comment-cleanup pass 8205022f reduced this to "All database reads and writes, including the legacy import, run under the lock", dropping the part that did the work: upstream master locks after migrating and justifies it with "Alembic uses its own connection, so we must wait until it's done before locking -- otherwise our own lock blocks the migration". That is false, the lock is on a separate <db>.lock file, and the surviving sentence said nothing to stop a contributor "fixing" the ordering back. Restored and adapted rather than pasted: the legacy copy and the db_exists probe now happen inside the lock, which the original text predates, so both are named in the list of things the ordering makes mutually exclusive. * fix(assets): stop a scan on memory exhaustion instead of deferring it MemoryError is an Exception, so the per-spec and per-batch handlers stored it alongside ordinary faults and carried on - allocating for every remaining spec and then every remaining batch while the process was already out of memory. Both handlers now let it through, and the scan records a failure and stops. * docs(assets): document the prune failure response and its None result The route gained a 500 PRUNE_FAILED branch and the seeder method gained a None return, both so a prune that did not run cannot be reported as a clean one. Neither contract was written down. * test(assets): assert the surviving spec count after a propagated fault * docs(db): shorten the lock-ordering comment while keeping its rationale * docs(tests): drop the cross-module justification from the import-order comment * test(assets): restore the cpu flag after the guarded prompt worker import * test(assets): restore the cpu flag even when the prompt worker import fails * fix(assets): drop asset_meta in a new migration instead of editing 0007 0007 shipped in v0.36.0, v0.37.0 and v0.37.1, so editing it would leave two installs at that revision with different schemas depending on when they upgraded. Restore 0007 to its released form and drop the unused asset_meta table in 0008 instead. Nothing reads or writes asset_meta; asset metadata lives in the JSON columns on assets. 0008 downgrades by recreating the table and its four indexes exactly as 0007 creates them. * refactor(assets): keep prompt_worker in main.py Custom nodes may reference main.prompt_worker, and tests can already import main (test_db_init_locking does), so the function stays where it was. Its body keeps the resume-on-failure handling and the pause flag that persists across loop iterations; the tests now call main.prompt_worker. * fix(assets): report the selected manager through the existing feature flags * fix(assets): record a failed prune in the scan status A failed prune left the scan's error list empty, so the run looked clean. Record it instead and let discovery continue as before. * test(assets): test the feature flag API directly instead of copying the startup write Both parity tests wrote SERVER_FEATURE_FLAGS["assets"] themselves, so they passed whatever startup did. Pin the one real claim - a no-argument get_server_features() reports the flag - in the feature flags tests, and drop the disabled case, which default_asset_manager already covers. * Bring #16486's watch-list batching and seed logging into this branch The merge before this commit is `git merge -X ours origin/master`: in conflicting hunks it keeps this branch's side. This commit ports what #16486 changed in those hunks. - tick_watch_list takes no session. It collects settled entries and seeds them in one batch through insert_asset_specs, keeping this branch's per-entry stat and spec handling and the finally that always rewrites the watch list. A failed seed is reported once per batch, since the batch only returns its first error. - seed_asset_specs no longer warns again for a skipped spec; observe_asset_specs already logged why. - Tests call tick_watch_list() without a session, bind the write session where they seed for real, and fake insert_asset_specs with its (created, error) return. The mid-drain fault now comes from stat, because seeding runs after the loop. * Note where the assets core flag is set and why prompt_worker catches BaseException --------- Co-authored-by: guill <jacob.e.segal@gmail.com>	2026-09-24 15:51:10 -07:00
Simon Pinfold	9d3bfc393b	Take the SQLite write lock up front for asset scan and output-registration writes (#16480) ... * Take the SQLite write lock up front for scan and output-registration writes The scanner's seeding and reference sync, and executed-output registration, write through a separate engine whose transactions open with BEGIN IMMEDIATE, and the database runs in WAL mode. On those paths stat, hashing and metadata extraction now happen before the write transaction opens, and reference-sync results are applied only to rows unchanged since they were observed. busy_timeout stays at pysqlite's 5s default. A fast-scan batch now commits as one transaction, so an unexpected error partway through discards the whole batch; the next scan recreates it. Enrichment, verification, uploads and tagging still write through the existing sessions. Migration backups use SQLite's backup API, and a legacy database is checkpointed before it is relocated, since in WAL mode committed pages can live in the -wal file that a plain file copy misses. * Skip relocating a legacy database whose WAL cannot be checkpointed The checkpoint can report busy without raising; moving the file then would leave committed pages behind in the -wal. Also keep the source's file mode on SQLite backups, as the plain copy did. * Replace run_write_txn with a create_write_session factory Write paths open the write engine's session the same way the rest of the code opens create_session(), and commit explicitly. Executed-output registration reads the new record's fields before committing, so expiry does not reload them in a second write transaction. * Document the scanner's pre-transaction observation types * Document that write sessions must not nest * Warn that a nested write session looks like lock contention * Seed a hashed spec with the stat its hash was verified against * Bound the SQLite backup so a locked destination cannot hang startup * Time out a backup only while it is blocked	2026-09-23 15:17:11 -07:00
Constantineandguill	5f0c4e18cb	Fix default database path for custom user directory (#14539) ... * fix: respect user directory for default database * refactor: use None default for --database-url instead of argv scan Per review: default --database-url to None and treat a non-None value as explicit at resolution time. Removes the sys.argv scan and the database_url_explicit attribute. database_default_path now serves directly as the legacy copy source. Adds regression tests for the unchanged no-flag default path and explicit URLs at the legacy location. * refactor: rename legacy database to .bak after copy Per review: after copying the legacy install-dir database to the effective user directory, rename the original to comfyui.db.bak so a later launch without --user-directory cannot silently fall back to a diverged copy, while keeping the file around for recovery. Also hoist the database_default_path import to module scope. * fix: guard legacy migration on existing .bak and rename before copy Per review: bail out of the legacy migration when comfyui.db.bak already exists, so only the first run migrates and later launches with a fresh --user-directory cannot grab a database another instance is using. Rename before copy so os.replace fails fast if the legacy DB is held open by a running instance. --------- Co-authored-by: guill <jacob.e.segal@gmail.com>	2026-08-24 16:19:03 -07:00