mirror of
https://github.com/Comfy-Org/ComfyUI.git
synced 2026-10-03 03:17:58 -05:00
cloud-openapi-projection
123
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Simon Pinfold
|
2e0316ea50 |
fix(assets): don't take the database lock when assets are off; warn when it's held (#16742)
* fix(assets): don't take the database lock when assets are off An assets-off ComfyUI no longer initialises the database or takes its lock, so it can't block a later assets-on start on the same install. If another process holds the lock, it prints a startup warning saying a future version will refuse to start the second instance, and keeps running. * fix(assets): never let the lock check stop startup; tighten the lock tests * fix(assets): keep the lock check's release inside its guard * fix(assets): reword the shared-database startup warning |
||
|
Simon Pinfoldandguill
|
30f15eaf5c |
feat(assets): report scan CPU and paused time, and classify scan failures (#16719)
seeder.scan_completed reported only elapsed_ms, which is wall-clock time
and includes every pause taken while prompts ran. It now also carries:
- cpu_ms: the scan thread's CPU time (time.thread_time()).
- paused_ms: time blocked at the pause gate, summed across pauses.
- dirs_listed_count: directories the input/output walk or the output
rescan's folder listing listed.
- files_statted_count: os.stat calls on files in the scan's per-file
loops (reference sync, the listing check, discovery, admission, seed,
watch list and enrich).
Scan failure events carried only the exception class, so every SQLite
failure read as OperationalError. seeder.scan_failed, batch_insert_failed
and scanner.{fast_scan,temp_sync,mark_missing,stat,watch_stat,watch_seed}
_failed now also carry error_kind, one of a closed set:
expression_tree_too_large, too_many_variables, database_locked,
disk_full, disk_io, unable_to_open, database_corrupt, permission_denied,
file_locked, read_only, other.
It is a closed enum rather than a scrubbed message because str() of a
SQLAlchemy error includes the statement and its bound parameters, which
are file paths. Classification reads SQLite's result code where the
driver exposes it (Python 3.11+), then SQLite's fixed message on the
driver exception (exc.orig), then errno and, on Windows, winerror.
Co-authored-by: guill <jacob.e.segal@gmail.com>
|
||
|
Simon Pinfold
|
65787d6683 |
fix(assets): batch prefix filters so scans work with many model folders (#16645)
* fix(assets): batch prefix filters so scans work with many model folders Every scan prefix (one per model folder, including each extra_model_paths base) adds terms to a single OR, and SQLite rejects an expression tree deeper than 1000. From about 500 prefixes every models scan failed with "Expression tree is too large" and the catalog stayed empty. Run the prefix filter in batches of at most 200 prefixes and merge the results, deduping rows that nested or overlapping prefixes put in more than one batch. With 200 prefixes or fewer the statement is unchanged. The enrichment candidate query merges each batch's keyset page, which yields the same page a single statement would. * fix(assets): filter enrich candidates in one pass above one prefix batch Paging each prefix batch separately made every page scan to the end of the table for any batch with few matches, which is quadratic in catalog size. Above one batch, read the candidates in id order once and apply the same prefix test in Python, stopping at the page limit. At 200 prefixes or fewer the statement is unchanged. Run the many-prefix tests under a 999 bound-variable cap too, the limit of SQLite before 3.32, which each 200-prefix batch stays under. * fix(assets): yield the GIL while filtering enrich candidates in Python Above one prefix batch the enrich candidates are filtered in a Python loop, which could hold the GIL across many rows outside the prefixes. Yield it per row as the other scan loops do, so the UI stays responsive. * Revert "fix(assets): yield the GIL while filtering enrich candidates in Python" Measured, it bought nothing: with a 60k-asset catalogue at 450 and 2000 prefixes, p99 lateness of a 1 ms sleeper thread is the same within noise either way, because the Python work between 500-row fetches is short and sqlite3 releases the GIL during each fetch. The yield made the loop 30-90% slower. * test(assets): import db at module level in the many-prefix scan test |
||
|
Simon Pinfold
|
e6beab52ef |
fix(assets): recover records when a drive comes back with hashing off (#16646)
* fix(assets): recover records when a drive comes back with hashing off A scan that runs while a drive is offline marks every row on it missing. With hashing off (the default), nothing could recover those rows when the drive returned, so the scan created new records and the user's names, tags, metadata and job links stayed on the orphaned originals. - With hashing off, a file that reappears at a path no live row occupies recovers the missing row there whose size and mtime match exactly. If several match, the newest recovers. Hashing on is unchanged. - A reference stat that fails with an I/O error other than not-found leaves the row live, as the output listing rescan already does. - seeder.marked_missing is emitted per root when a fast scan marks rows missing, and scan_completed carries missing_marked_count and recovered_count. * fix(assets): read recovery candidates before the write transaction; count recoveries only once committed * test(assets): a pruned model folder recovers its records when it is registered again * fix(assets): never recover a missing row whose records were all deleted * docs(assets): scope the multiple-match rule to hashing on * docs(assets): only new content splits a same-path edit * test(assets): scan_completed carries nonzero missing and recovered counts * perf(assets): let the partial live-path index serve the hashing-off recovery check Written as IS 0, the check that no live row occupies the path scanned asset_contents once per recovered file, inside the batch's write transaction. At 50k rows that stretched each batch's lock window to about 2.5s and made concurrent output registrations time out. * ci: run the checks against master * test(assets): import folder_paths at module level |
||
|
Simon Pinfold
|
ebd432a78a |
fix(assets): don't let a model category that can't be listed abort the scan (#16658)
folder_paths.get_filename_list raises when a folder it cached earlier has gone away (or on an OSError other than not-found), and collect_models_files let that abort the whole asset scan, so nothing in models, input or output was catalogued. Fall back to a fresh listing of the category, which skips a folder that's gone; if that raises too, skip the category with a warning and scan the rest. |
||
|
Simon Pinfold
|
56c50055cf |
perf(assets): keep the UI responsive during asset scans and rescan output by listing folders (#16546)
With --enable-assets, the scans that keep the asset database in sync could starve the web server's event loop and burn CPU on large libraries. Three changes: - First scan yields the GIL. The loop that derives names and tags for new files now sleeps briefly every 2 ms of work, so the event loop keeps running. At 51k files, time to a usable UI on the first run drops from 4.7 s to 2.0 s (1.5 s with assets off); the first scan takes about 3% longer. (was #16546) - Output rescans list folders instead of stat'ing every file. The post-prompt output rescan compares directory listings against the database instead of stat'ing each catalogued file. That per-file stat was 70–95% of the rescan's cost: ~5.8 s per rescan at 200k outputs on local SSD, ~57 s at 10k on NFS. Any row the listing can't vouch for gets an individual stat before it is retired, and only "file not found" retires it: case-insensitive or normalizing filesystems, hidden folders, folders that fail to list, symlink aliases, and permission or I/O errors keep their rows. The one behaviour left to the next full scan is a file overwritten in place under the same name. (was #16599) - Rescan loops pause. The listing and comparison loops pause every 10 ms of work, so prompts start and the UI stays responsive during a long rescan. At 200k outputs, without this a prompt could wait ~1 s to start. This costs 7–15% more rescan time. (was #16600) Assets-only: none of this runs with assets off. |
||
|
Simon Pinfold
|
d669bcfeb1 |
fix(assets): Start without the assets packages; --enable-assets reports what is missing and runs with assets disabled (#16580)
ComfyUI failed to start when the assets database packages (sqlalchemy, alembic, blake3) were missing, e.g. in a venv not re-synced after requirements.txt changed. That happened with assets off too, although that mode never needs them. - The asset imports in lifecycle.py, manager.py and server.py are guarded by db.py's existing dependencies_available() check. Without the packages, NoAssets registers no asset routes and temp cleanup still runs. - --enable-assets without the packages logs which packages are missing, prints the install command, and continues with assets disabled. - /view resolves blake3: filenames only when assets are enabled. Trade-off: if assets are turned off on an install whose saved workflows hold blake3: widget values, those previews return 404. Running such workflows already fails, since execution never resolves hashes. |
||
|
Simon Pinfold
|
a35fcbac26 |
fix(db): wait briefly for the database lock at startup (#16602)
* fix(db): wait briefly for the database lock at startup A relaunch can start while the previous process is still exiting and holding the lock. Wait up to 5 seconds for it to be released before treating the database as in use, and log how long the wait took. The lock is never taken from a holder. * fix(db): log the lock wait at info without guessing its cause * fix(db): say when startup waits for the database lock Log once before the wait instead of after it, so a lock that stays held explains the pause before the error. Document the wait in the docstring, and make the wait test release the lock only after the wait has started. |
||
|
Simon Pinfoldandguill
|
7897b4ee35 |
perf(assets): replace pathlib prefix checks in the asset scan (#16543)
* Speed up the startup prune's owned-prefix check
mark_contents_missing_outside_prefixes tested every live AssetContent row
against every owned prefix with Path.is_relative_to, which walks the path's
parents on each call: rows x prefixes x depth. At 9k rows and 150 prefixes
that was 9.5s (15.5s with deeper output paths).
path_prefix_matcher normalizes the prefixes once and checks each row with a
normcase'd, separator-bounded str.startswith over a tuple, keeping
is_relative_to's component bounds and platform case rules. The same case
now takes about 0.04s, flat in prefix count and depth.
* Move path_prefix_matcher next to the SQL containment predicate
path_utils needs the same check for per-file tagging, and scanner_changes
already imports path_utils, so the matcher moves to app.assets.helpers
beside sql_path_under_prefix.
* Speed up asset tag derivation during scans
get_backend_system_tags_from_path checked every scanned file against every
model base with Path.is_relative_to, which dominated the fast scan: 37s of
build_asset_specs for 100k output files. Using path_prefix_matcher, the
separator-bounded string check the startup prune already uses, brings that
to about 7s.
* Keep path_prefix_matcher's parity for paths starting with two slashes
abspath keeps exactly two leading slashes, and pathlib treats that '//' as an
anchor of its own, so Path('//server/f').is_relative_to('/') is False, but
the string check accepted it. Prefixes are now split once by whether their
anchor is '//', and a candidate is only checked against prefixes with the
same anchor. The per-row check is still a single str.startswith.
* Build tag-derivation prefix matchers once per folder config
get_backend_system_tags_from_path built a path_prefix_matcher for input,
output, temp and every model category on each call, so most of its cost was
normalizing the same prefixes again for every scanned file. cached_prefix_matcher
memoizes construction on the raw prefix tuple, which the callers pass as
absolute folder paths; a folder-config change is a new key.
100k files, 58 model bases: tag derivation 81.8 -> 27.7 us/file, and
build_asset_specs CPU 11.55 -> 6.40s.
* State cached_prefix_matcher's absolute-prefix precondition
The docstring presented absolute prefixes as a property of the callers. It
is a precondition the caller must meet: folder_paths stores what it is given,
and a relative prefix is resolved once, on first use, and then frozen.
---------
Co-authored-by: guill <jacob.e.segal@gmail.com>
|
||
|
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 |
||
|
Simon Pinfold
|
f427c3a285 |
Move file reads out of the remaining asset write transactions (#16486)
* 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 * Commit each drained entry before hashing the next drain_pending_verifications and drain_transition_queue ran every entry in one session, so an entry that wrote (marking a vanished file missing, say) left a transaction open while the next entry's file was stat'ed and hashed. Commit at the top of each entry instead, so the hash runs with no transaction open. * Seed settled watch-list entries through insert_asset_specs tick_watch_list seeded each settled file through seed_asset_specs in the caller's session, where the enrich phase still held drain_pending's writes open, and each seed ran in a deferred savepoint that reads before it writes. Collect the settled specs and hand them to insert_asset_specs, which stats and hashes before opening one write session. The seeder commits the pending verifications before ticking, so no transaction is open while the watch list stats or waits for the write lock. * Read upload metadata before the claim transaction _create_upload_record read the file for system metadata after the content claim had opened a write transaction. Callers now extract the metadata before opening their session (or, when reusing content, before claiming it) and pass it in. The claim's own stat re-check stays inside the transaction: that is what makes the claim sound. * Keep the upgrade error when restoring the backup also fails If restoring the pre-upgrade backup raised, that exception replaced the upgrade's, and the backup's location was never logged. Log the upgrade error first, log where the pre-upgrade copy is kept if the restore or its cleanup fails, and re-raise the upgrade error either way. * Note the drains' session requirement and word the restore log for either failure The drains' per-entry commit only leaves no transaction open on a create_session() session. The restore log now covers a failed backup removal as well as a failed restore. The watch-list admission test patches insert_asset_specs, the seam tick_watch_list now calls. * Log the real error when a seed spec cannot be read observe_asset_specs treated every OSError as a vanished file, so a permission error or an I/O error on a file that still exists was reported only as "Skipping vanished asset during scan". A missing file is still handled as before; any other OSError is now also logged through _log_scan_error before the spec is skipped. The vanished-path test's fake now raises FileNotFoundError, the error a vanished file produces. * Report an unreadable seed spec once, without also calling it vanished * Skip a pending verification whose row changed while it was hashed Committing per entry means no lock is held while the file is hashed, so another writer can retire or replace the row in that time. The drain would then store a hash on a missing row, or split it and attach a record to the other writer's row. Re-read the row after hashing and skip it unless it is still live with the hash, size and mtime it was loaded with, as apply_reference_observations does. * Remove the stored upload file in the reupload claim test The test cleaned up its temp files but left the first upload's stored file in the output directory. |
||
|
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 |
||
|
|
f42b24efbe |
feat: structured event log lines for the assets system (#16306)
* feat(assets): structured event log lines for the assets system * test(assets): AST check that tagged event lines stay inside the closed vocabulary * feat(assets): structured event logging for the seeder lifecycle * feat(assets): structured event logging for scanner and ingest failure paths * feat(assets): structured event logging for API request failures * fix(seeder): finalize checkpoint no longer parks completed scans * fix(scanner): enrich_failed counts exceptions, not benign races * fix(seeder): idle reset survives emit/assert failures * refactor(assets): private emit-once bookkeeping helper; clean Progress DTO * test(event-log): call-site registry as frozenset; drop dead pending machinery * feat(event-log): scanner.stat_failed with emit-once discipline; drop dead vocabulary * refactor(assets): drop API request failure events * refactor(assets): drop ingest failure events * refactor(assets): narrow event vocabulary to scan pipeline * test(assets): per-call-site event tests collapse to mechanism-once * refactor(assets): event log lines as logfmt, matching the log's own idiom * test(assets): adapt hash-failure log assertion to privacy-safe scan-error logging The rebase onto master picked up the privacy-safe _log_scan_error from PR 16096, which no longer includes the file path in the log message. Assert on the generic 'Asset scan error' message and that the path does not leak, instead of asserting the path is present. * fix(assets): enforce event vocabulary and scan counters --------- Co-authored-by: Simon Pinfold <synap5e@users.noreply.github.com> Co-authored-by: guill <jacob.e.segal@gmail.com> |
||
|
|
19e1058f4c |
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: |
||
|
Christian Byrne
|
d43a5fa20c | Log typed asset scanner filesystem errors (#16096) | ||
|
comfyanonymous
|
d9726bb098 | Make comfyui handle missing workflow media properly. (#16079) | ||
|
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> |
||
|
Simon Pinfoldandguill
|
3aba3daef3 |
Derive asset preview URLs from the file path (#15509)
preview_url was assembled from a /api/view link whose type was chosen by matching the asset's tags against "input" then "output". Anything written anywhere else - temp above all, where preview nodes put their images - fell off the end of that chain and came back with no preview at all. Tags are user-editable, so removing one also silently destroyed the URL. Derive the URL from where the file actually sits instead. That covers every root /api/view serves, temp included, and no longer depends on tags or on a filename in user_metadata. A file outside those roots, or content no client can render from its own bytes, gets no preview URL rather than one that cannot work. Nominated previews are resolved a page at a time rather than per row, so a list costs one extra query however long it is. A preview that is soft-deleted or not visible to the caller drops out of that lookup and is no longer advertised. Co-authored-by: guill <jacob.e.segal@gmail.com> |
||
|
Simon Pinfold
|
cc0fc21fea |
Keep temp-directory assets visible while their files exist (#15510)
* Keep temp-directory assets visible while their files exist Assets written to the temp directory were flagged as missing and dropped from GET /api/assets, even with the file sitting on disk. One list of directories was answering two different questions -- where the scanner looks for new files, and which files ComfyUI considers its own -- and temp belongs only in the second, so every temp reference was disowned by the prune that runs at startup and on POST /api/assets/prune. Ownership now covers temp. Discovery still does not: the temp directory is wiped before the scan runs, and assets written there are already registered with a hash, mime type and dimensions, so walking it would find nothing. Temp references are instead reconciled against the filesystem directly, so a temp file that really is gone is still retired rather than lingering as a broken entry. get_prefixes_for_root becomes get_scan_prefixes_for_root so the two questions are told apart by name rather than by comment. * Cover the unhashed temp asset in the reconciliation tests The existing temp tests all registered hashed assets, so they never exercised the path an unhashed asset takes when its file is gone: the orphaned rows are removed rather than kept as missing, exactly as under any other root. |
||
|
Simon Pinfold
|
34744cd29e |
Add tags_all / tags_any / tags_none tag filters to the assets list API (#15332)
* Implement tags_all/tags_any/tags_none on the assets list API (BE-6600) Adds the three canonically-named tag filter params to GET /api/assets and GET /api/assets/tags/refine: - tags_all: asset carries every tag (replaces include_tags) - tags_any: asset carries at least one tag (new) - tags_none: asset carries no tag (replaces exclude_tags) Clauses intersect; tags_none always wins. include_tags/exclude_tags remain as permanent deprecated aliases and behave exactly as before when used on their own. Invalid combinations return 400 INVALID_TAG_FILTER, but only when the request uses at least one new-name parameter (non-empty after normalisation): - mixed spellings of one slot (include_tags with tags_all, exclude_tags with tags_none) - the same tag in the effective all-list and none-list (query can never match) Old-names-only requests gain no new error paths: include_tags=a&exclude_tags=a still returns an empty 200. tags_any/tags_none overlap stays valid (dead term, not a dead query). * Address review findings: positional-compat, deprecation metadata, test matrix - Move any_tags to the end of the four touched signatures: inserting it mid-signature silently misbound pre-existing positional callers (e.g. a caller passing name_contains positionally would have it consumed as any_tags). - Mark include_tags/exclude_tags Field(deprecated=True) on both list schemas so generated schema metadata matches the contract, not just a comment (schemas_out.py already uses this form for Asset.name). - Add tests: legal cross-slot old/new combinations, repeated query-key concatenation (pins Core behavior; outside the cross-platform contract), tags_any two-page cursor consistency (total/has_more/ no-overlap), refine-route mixed-spelling rejection + legacy-conflict preservation, and schema deprecation metadata. * Pin tag-value opacity: case-sensitive matching, byte-exact conflict check The prod tag survey (~/comfy/prod-model-tag-shape.md) found live case-distinct tag pairs (SEEDVR2/seedvr2) that resolve differently, so the contract now states tag values are opaque byte-strings. Pin that: case-distinct tags filter separately, and a case-distinct all/none pair is not an INVALID_TAG_FILTER conflict. * Document tags_all/tags_any/tags_none in openapi.yaml, deprecate aliases Add the three tag-filter parameters to both listAssets and getAssetTagHistogram parameter blocks and mark include_tags/exclude_tags deprecated: true, keeping the spec in step with the runtime schemas so generated clients can discover the new filters while the aliases stay present for existing consumers. * Move schemas_in import to module scope in test_list_filter Review feedback: no import cycle requires the local import. * Silence per-request DeprecationWarning in the tag-filter remap shim Reading the deprecated include_tags/exclude_tags fields by attribute fires pydantic's DeprecationWarning on every list/refine request even for callers using only the new names. The warning is aimed at API clients, not the server's own remap; read via model_dump instead. * Cap tag-filter lists at 100 entries, all spellings Review finding: unbounded tag lists fan out into one correlated EXISTS per tag on both page and count statements. Cap each list at 100 normalized entries with 400 INVALID_TAG_FILTER naming the parameter. Applies to the legacy spellings as well — a deliberate, decided exception to the old-names-behave-identically rule, since a cap only on new names would leave the same fan-out reachable through the aliases. * Strip process narration from comments Comments carried decision dates, contract cross-references, and review context. Keep only the constraints the code cannot show, one line each. |
||
|
Oliver Freyermuth
|
b53e247c94 |
rename comfy/logging.py to comfy/internal_logging.py (#15231)
This avoids name collision (circular imports) for external custom nodes, for which the comfy path is pushed into sys.path so Python's own logging module is shadowed otherwise. fixes: #15229 |
||
|
comfyanonymous
|
9b3aa08967 | Don't enable comfyui_detail.log by default. (#15159) | ||
|
Matt Miller
|
9cf91339b7 |
Fix SVG previews broken by the stored-XSS forced-download (#15149)
* Fix SVG previews broken by the stored-XSS forced-download /view and the assets download route force every SVG to application/octet-stream + attachment. That blocks the stored XSS from GHSA-779p-m5rp-r4h4, but it also breaks the SVG node output and Media Assets previews, which request the file with a plain <img>. Exempt only that case. An SVG referenced by an <img> loads in secure static mode with scripting and external references disabled, so the payload cannot fire. The attack needs the SVG to become a document, which arrives with a different Sec-Fetch-Dest. Browsers set that header themselves and page script cannot override it. A missing header, from a non-browser client or a proxy that strips it, fails closed. The blocklist itself is unchanged; this is a call-site gate. * Don't let a cache replay the inline SVG into document context The Sec-Fetch-Dest exemption makes /view and the assets content route vary their Content-Type and Content-Disposition on a request header, but neither response said so. FileResponse emits Last-Modified/ETag and the cache_control middleware skips /view (the filename is in the query string, not the path), so the inline image/svg+xml variant is heuristically cacheable. A cache keyed on the URL alone could hand an entry primed by an <img> load to a later top-level navigation of the same URL, turning the SVG back into a document and re-enabling the stored XSS the forced download blocks. Set Vary: Sec-Fetch-Dest and Cache-Control: no-store on both branches, not just the exempt one: a cached attachment replayed to an <img> would re-break the preview this fix exists to restore. Also strip parameters from content_type before building the assets response. mime_type there is uploader-supplied and unvalidated, and aiohttp rejects a charset in the content_type argument with ValueError, so a stored "image/svg+xml; charset=utf-8" turned a valid inline SVG into a 500. Route-level guards now pin the headers on both branches and the parameterised mime type; all three fail against the previous commit. |
||
|
kaalibro
|
c65f9f169c | Fix user.css loading broken by #14734 (#15000) | ||
|
rattus
|
fbe6d3ca8f | Add configurable DETAIL logging side channel (#15064) | ||
|
Simon Pinfoldandguill
|
412aaab0e2 |
feat(api): expose registered extension filters on /experiment/models (#14797)
Each folder in the listing now carries its registered extension
allowlist verbatim; an empty array means the folder accepts any
extension (match-all), mirroring filter_files_extensions semantics.
Gives consumers the filtering rule itself rather than just its output:
/models/{folder} lists files by the per-folder rule but the rule is not
exposed anywhere, and /experiment/models/{folder} filters everything by
the global supported_pt_extensions regardless of registration.
Presentation-level filtering of match-all folders (e.g. hiding
README/config noise that repository-downloading custom nodes leave in
model directories) is deliberately left to the consumer.
Co-authored-by: guill <jacob.e.segal@gmail.com>
|
||
|
Simon Pinfoldandguill
|
55a15f87ce |
feat(assets): add namespaced model_type tags and align tag semantics (#14511)
* feat(assets): add namespaced model type tags * fix(assets): mark path-derived upload tags automatic * fix(assets): merge duplicate scan specs * test(assets): make duplicate path normalization portable * feat(assets): add loader_path as the authoritative loader locator (#14796) * fix(assets): filter model_type tags by bucket extension sets Buckets sharing a base directory (e.g. diffusion_models and a custom unet_gguf) tagged every file in the directory regardless of whether the bucket could load it, so .safetensors files were tagged model_type:unet_gguf and vice versa. Carry each bucket's registered extension set through get_comfy_models_folders and only emit a model_type tag when the file extension matches, keeping the empty-set match-all convention from folder_paths.filter_files_extensions. Files under a model base matching no bucket now keep only the models tag instead of every directory-matching model_type tag. * feat(assets): replace response file_path with persisted loader_path The old file_path response field was a namespaced storage locator (models/checkpoints/foo.safetensors): not an absolute path, not unique identity, and not the value a loader consumes. Nothing needs that shape on the wire (hash/ID-based locating is the long-term direction), so it is dropped rather than renamed; the storage-root matching stays internal, powering display_name. What loaders DO need is the in-root loader path (category dropped: models/checkpoints/foo/bar.safetensors -> foo/bar.safetensors). Serve it as a first-class loader_path field, persisted on asset_references (migration 0006) and written by every ingest pipeline at insert, so responses read the column verbatim. Like the model_type tags, loader_path is a seed-time derivative of the model folder registry, maintained by the same scan lifecycle (new files seed fresh values, pruning retires rows whose bucket disappeared). Rows predating the column serve a null loader_path; databases from before this stack already need recreating for the base branch's tag changes. loader_path resolves every registered base including extra_model_paths entries; display_name only the canonical storage roots. A file can therefore be loadable with no display name (extra-path models) or the reverse (unregistered files under the models root), and loader_path is null exactly when no loader can resolve the file. * test(assets): lock loader_path matrix (asymmetry, null, persist/read) Cover the behaviour that has no production change but is easy to regress: the extra-path asymmetry (loadable but no storage namespace), null loader_path persistence for orphan files, and the response reading the stored column with a compute fallback for un-backfilled rows. * fix(assets): persist subfolder-qualified loader_path for ingested outputs ingest_existing_file built its seed spec with the file's basename, so outputs saved into a subfolder persisted loader_path (and the user_metadata filename that preview URLs split for their subfolder param) as just the basename: the served locator pointed at a file that does not exist at that path. Scanner and seeder specs already derive fname via compute_loader_path; use the same derivation here. * fix(assets): only extension-matching buckets contribute a loader_path The model-base match in get_asset_category_and_relative_path ignored each bucket's extension set, so a file inside a registered base whose extension the bucket cannot load (e.g. a .txt uploaded into model_type:checkpoints) advertised a loader_path that no loader list would ever resolve, while the tag side of the same stack already excluded it. Apply the extension check used for backend tags (empty set accepts any extension), keeping loader_path null exactly when no loader can resolve the file. * fix(assets): refresh loader_path when re-ingesting an existing reference upsert_reference only wrote loader_path on the INSERT branch, so re-ingesting an existing reference (an output overwritten in place, or a file re-registered after its loader_path derivation changed) kept the stale or NULL value forever. Write it on the UPDATE branch too, with a null-safe change guard so a loader_path difference alone is enough to trigger the update, and identical values stay a no-op. * fix(assets): repair semantic merge breakage from #14796 and master Two textually-clean but semantically-broken merges: - routes.py lost its folder_paths import when #14796's import block superseded the base's, while the content-type hardening added via the base's master merge still calls folder_paths.is_dangerous_content_type. - master's SVG download-hardening test uploads with the pre-namespacing bare checkpoints tag, which this branch's destination validation rejects; use model_type:checkpoints. --------- Co-authored-by: guill <jacob.e.segal@gmail.com> |
||
|
Matt Miller
|
96e0e3585b |
security: fix four vulnerabilities (GHSA-779p-m5rp-r4h4) (#14734)
* security: fix five vulnerabilities (GHSA-779p-m5rp-r4h4)
- CVE-2026-56670: force download of SVG/XML responses on /view to prevent stored XSS
- CVE-2026-56671: contain /experiment/models/preview reads within the model folder
- CVE-2026-56672: stop inline rendering of uploaded /userdata/{file} content
- CVE-2026-56673: prevent path traversal in get_annotated_filepath (LoadImage /prompt input)
- CVE-2026-56674: reject opaque/null Origin to close the CSRF middleware bypass
Adds regression tests under tests-unit/security_test/ covering all five.
* security: address review feedback on GHSA-779p fixes
- Fix Windows CI failure in test_get_annotated_filepath: compare against
os.path.abspath(...) to match the intentional abspath normalization added
by the traversal hardening (abspath prepends the drive letter on Windows).
- origin_check: narrow the bare `except:` in is_loopback() to ValueError so
genuine interrupts aren't swallowed (review nit).
- origin_check: guard .port access in is_cross_origin_forbidden() so a
malformed/out-of-range port (e.g. Origin: http://127.0.0.1:99999) fails
closed with a 403 instead of surfacing an uncaught 500 in the middleware.
- server /view: escape backslash/quote in the Content-Disposition filename
(RFC 6266 quoted-string) so a filename containing a double quote can't
malform the response header.
* security: address CodeRabbit review feedback on GHSA-779p tests
- test #3: guard the symlink-escape test with a try/except skip so it no
longer errors on Windows CI where os.symlink needs elevated privileges /
Developer Mode (mirrors the guard in the sibling test #2).
- test #5: refresh the stale module docstring to describe the actual /view
gating (view_image closure calling folder_paths.is_dangerous_content_type,
the normalising check) instead of the bypassable raw set-membership test.
* revert(security): drop CVE-2026-56674 Origin: null CSRF change
Per maintainer review, the reported CSRF is already mitigated by the pre-existing
Sec-Fetch-Site: cross-site check for current browsers, and the null-origin
rejection risked breaking legitimate sandboxed-iframe embeds. Restores
origin_only_middleware and is_loopback in server.py to their prior state
(the Sec-Fetch-Site check is retained) and removes utils/origin_check.py and its
regression test. The other four GHSA-779p fixes are unaffected.
|
||
|
Matt Miller
|
431a1888d3 |
revert(assets): drop job_ids filter from GET /api/assets (#14408)
The job_ids query filter added in #13998 has no live consumer: the frontend Generated tab kept sourcing from GET /jobs, and the cloud side removed its equivalent filter from the shared asset spec. Carrying it on the local server only re-introduces Core<->Cloud drift on the shared contract, so remove it to match. Removed: the job_ids field + validator on ListAssetsQuery, the IN(...) clauses in list_references_page, the service/route passthrough, and the filter-only tests. Kept: the canonical-UUID prompt_id enforcement at job creation (also landed in #13998). It stands on its own -- job ids are matched verbatim by history keys, websocket correlation, and /interrupt -- and cloud inherits it by running core for execution, so no divergence is created. |
||
|
Matt Millerandguill
|
e5b7140dcc |
feat(assets): add job_ids filter to GET /api/assets (#13998)
* feat(assets): add job_ids filter to GET /api/assets Mirrors the existing cloud `job_ids` query param on the local Python server: clients can pass a comma-separated list (or repeated query params) of UUIDs to filter assets by their associated job. The `AssetReference.job_id` column already exists, so no migration is needed — this just plumbs the filter through schema → service → query. Marks the parameter as available in both runtimes by dropping the `[cloud-only]` description prefix and the `x-runtime: [cloud]` tag from the OpenAPI spec, per the OSS field-drift convention (absent runtime tag = populated by both local and cloud). * fix(assets): tighten job_ids — array schema, max_length, narrow except From cursor-reviews on the parent commit: - OpenAPI: declare job_ids as `type: array, items: string format: uuid` with `style: form, explode: true` so it matches the documented contract (and matches sibling include_tags/exclude_tags shape). Description now states both accepted shapes explicitly. - Schema: cap `job_ids` at 500 entries (max_length on the Pydantic field) so a client can't splice an unbounded list into the IN clauses. - Schema: drop `AttributeError` from the except — `raw` only contains `str` items by construction, so `uuid.UUID(<str>)` raises `ValueError` exclusively; the second clause was dead code. * fix(assets): tighten job_ids validator + add schema-level tests Aligns with the parallel hardening from draft PR #13848 (now closed as a duplicate). The validator now: - Raises ValueError on non-string list items (was: silently dropped). - Raises ValueError on non-string / non-list top-level values like dict or int (was: silently passed through to Pydantic's downstream coercion). Adds tests-unit/assets_test/queries/test_list_assets_query.py covering the validator end-to-end: CSV canonicalization, dedup order, default empty, invalid UUID, non-string list item, non-string non-list value, and the max_length=500 boundary. * feat(prompt): enforce canonical UUID prompt_id at job creation POST /prompt previously accepted any client-supplied prompt_id verbatim, str()-coercing even non-strings, and minting the literal job id "None" for an explicit JSON null. The new GET /api/assets job_ids filter matches stored job ids as canonical UUIDs exactly, so a non-UUID id minted a job whose assets could never be filtered. - validate_job_id (comfy_execution/jobs.py): requires a string in the canonical lowercase hyphenated UUID form; raises ValueError otherwise, including parseable-but-non-canonical spellings (uppercase, braced, URN, bare hex), which would otherwise be silently rewritten and then miss every exact-match lookup downstream (history keys, websocket correlation, /interrupt, the assets job_ids filter). - POST /prompt: absent or null prompt_id means the server mints uuid4; invalid means 400 invalid_prompt_id on the standard error envelope. - openapi.yaml: document the request-side prompt_id (format uuid, nullable) on PromptRequest. - tests: unit matrix for validate_job_id; integration tests against the booted server covering rejection, acceptance, and null handling. --------- Co-authored-by: guill <jacob.e.segal@gmail.com> |
||
|
Matt Miller
|
039ed38ed1 |
fix(assets): remove unused delete_content param from deleteAsset (#14241)
* fix(assets): remove unused delete_content param from deleteAsset
The delete_content query param on DELETE /api/assets/{id} was introduced
in #12125 and had its default flipped to false in #12621. In practice no
client sends it: the frontend issues a bare DELETE /assets/{id}, so every
real caller already gets the default soft-delete (the reference is hidden,
content preserved). The only thing that set delete_content=true was this
repo's own test teardown.
Remove the param from the route and the OpenAPI spec so the contract
matches what clients actually use (and lines up with the cloud surface).
The route now always soft-deletes. The underlying delete_asset_reference
helper keeps its delete_content_if_orphan option, so orphan reclamation
remains available internally for a future GC path — it's just no longer
exposed on the public endpoint. Tests that used delete_content=true for
hard cleanup now soft-delete; test_delete_upon_reference_count asserts
content preservation instead of orphan removal.
* test/docs: address review on deleteAsset delete_content removal
- Rename test_delete_upon_reference_count ->
test_soft_delete_preserves_asset_identity_across_references; the old name
implied last-ref cleanup, but it now verifies the opposite (soft delete
preserves identity across references).
- Strengthen the re-association assertion: also check asset_hash == src_hash
so it proves content reuse rather than relying on the now-tautological
created_new is False.
- Document delete_asset_reference: the orphan-reclamation branch is
intentionally internal-only; the public endpoint always soft-deletes.
- Normalize the soft-delete comment phrasing.
* test(assets): make seed content unique per test for isolation
Removing the delete_content param means delete is always a soft delete, so
content created by one test now survives into the next. The suite had been
relying on hard-delete teardown for isolation, so shared fixed-content
fixtures started colliding: seeded_asset (b"A"*4096) and
make_asset_bytes (deterministic on name) produced the same hash every test,
so the second seed deduped to the surviving asset and returned 200 instead
of 201, cascading into ~14 failures/errors.
Salt both fixtures with a per-test uuid so each test creates fresh content
(created_new True, 201), while keeping content deterministic within a test
(same name/size -> same bytes) and preserving exact byte length so size-based
list/sort assertions are unaffected.
|
||
|
Matt Millerandguill
|
84e0692a3d |
feat(assets): cursor-based pagination on GET /api/assets (#14014)
* spec(assets): add cursor pagination params to GET /api/assets
Add 'after' query param and 'next_cursor' response field for keyset
pagination. Matches the cloud Go implementation (BE-893) so frontend
sees a unified contract across runtimes. Offset/limit remain as a
deprecated fallback.
* feat(assets): add cursor encode/decode helpers for keyset pagination
Port of cloud common/pagination/cursor.go. Wire format is base64url of
{"s", "v", "id"} JSON; times are Unix microseconds UTC to match
PostgreSQL timestamp precision.
Includes a byte-identity fixture pinned against the cloud Go wire
format so cross-runtime FE pagination can't silently drift.
* feat(assets): thread cursor through schemas, service, and query layer
list_assets_page accepts an opaque 'after' cursor and returns
next_cursor when more pages are available. The query applies a keyset
WHERE clause and a secondary ORDER BY id for deterministic tiebreak.
Cursor sort field is validated against the request sort, and a
last_access_time sort (OSS-only) falls back to offset/limit. Offset is
ignored whenever a cursor is supplied.
* feat(assets): wire cursor pagination through GET /api/assets handler
Adds integration tests for: full cursor walk, invalid-cursor 400,
sort/cursor mismatch 400, cursor-wins-over-offset, absent next_cursor
when no more results, and pagination stability across deletes.
* fix(assets): address cursor-review verified findings
- Mint next_cursor on every cursor-supported sort, not only when 'after'
was supplied. A first request (no 'after') previously returned
next_cursor=None, leaving cursor mode unreachable from a clean start.
- Over-fetch limit+1 so an exactly-full terminal page doesn't mint a
spurious cursor pointing at a phantom next page.
- Map crafted out-of-range microsecond cursors (OverflowError / OSError
in datetime construction) to 400 INVALID_CURSOR instead of leaking 500.
- Bump MAX_CURSOR_VALUE_LENGTH 256 -> 512 to match the AssetReference
name column max; without this, a long-named asset minted a cursor the
same server then refused on the next request. Cross-runtime byte
identity with cloud is unaffected because no cloud cursor ever carries
a value > 256 (cloud schema doesn't permit it).
- Return None from _encode_next_cursor when the boundary row carries a
NULL sort value (e.g. an Asset without size_bytes backfilled), instead
of silently encoding 0 and mis-positioning the keyset.
- Fix schemas_in.py comment so it matches actual handler behavior
(last_access_time + 'after' raises 400, does not fall back).
- Add AssetsApiError schema + 400 response to GET /api/assets in
openapi.yaml so generated clients know the INVALID_CURSOR envelope.
- Extend integration coverage: first-page mint, exact-multiple terminal
page, cursor walks for created_at/updated_at/size sorts, datetime
overflow surfaces as 400 not 500.
- Add unit coverage for datetime overflow and 512-char round-trip.
* feat(assets): bind cursor to sort order + Go-compat JSON escaping
Address three needs-judgment items from the cursor-review judge synthesis:
1. Cursor wire format now includes an "o" key carrying the sort
direction ("asc" / "desc") it was minted under. A request that
replays the cursor with a flipped `order` parameter is rejected
with 400 INVALID_CURSOR instead of silently walking the wrong
direction. Legacy cursors without "o" still decode (the binding
is best-effort until cloud mirrors the field — follow-up filed
separately).
2. JSON serialization now escapes `<`, `>`, `&`, U+2028, U+2029
to mirror Go's default `json.Marshal` behavior. Without this, an
asset name containing those characters produced different bytes on
Python vs cloud Go. The escaped form is what both runtimes emit.
3. Add direct query-layer tests for the keyset tiebreaker — the secondary
ORDER BY id branch was previously unexercised. Two scenarios: all
rows share a primary sort value, and mixed ties straddle page
boundaries. Both assert no row is dropped or duplicated across the
walk.
Wire-format note: Python cursors now differ from current cloud cursors
by exactly the "o" key. Cloud follow-up will bring the two back into
byte alignment.
* fix(assets): address bot review comments
- Soften offset param prose: it's not deprecated, just not preferred for
sequential walks. Random-access UIs (jump-to-page, item count displays)
legitimately still want offset, so dropping the 'deprecated' framing
rather than promoting it to a machine-readable deprecated:true flag.
- Add explicit HTTP status assertions before every json() / next_cursor
read in test_list_cursor.py so a failing request surfaces as an HTTP
error instead of a confusing KeyError on a 4xx/5xx body.
* feat(assets): require cursor o field, drop legacy permissive path
Cursor pagination hasn't shipped on either runtime yet — this PR is
still draft and cloud's mirror is just behind it — so there are no
legacy no-o cursors in the wild. Make o mandatory from day one
rather than landing permissive and tightening later.
decode_cursor now rejects any payload without o (or with a non-string
o) as malformed. CursorPayload.order becomes a required str. Tests
that constructed CursorPayload directly now pass order="desc";
test_legacy_cursor_without_order_accepted flips to
test_cursor_without_order_rejected.
* chore(assets): drop cross-repo prose from cursor comments
Strip prose references to sibling Go implementations and external
ticket IDs from cursor.py, the cursor tests, the keyset integration
tests, asset_management's sort-field comment, and the legacy
prompt_id alias comment. Pure docstring/comment scrub — no behavior
or wire-format changes. x-runtime: [cloud] field annotations in
openapi.yaml are unchanged; those are the spec's structural
cross-runtime convention, not internal references.
* test(assets): include 'o' in microsecond-boundary cursor payload
The boundary test was building a cursor without the required `o` key, so
decode failed on the missing-order branch before reaching the µs-overflow
path the test is asserting. Both paths return 400 INVALID_CURSOR so the
assertion passed for the wrong reason. Add `o` to the payload and matching
`order=` to the request so the decode reaches the intended branch.
* fix(assets): address ultrareview findings on cursor pagination
Six fact-checked findings from the multi-model review pass:
- Encoder/decoder length asymmetry: encode_cursor now rejects empty id,
oversized id (>128), oversized value (>512), and invalid order tokens
symmetrically with decode_cursor. Prevents the same server from minting
a cursor it then 400s on the next request (e.g. a filesystem-scanned
asset name >512 chars). The bad-order path now raises InvalidCursorError
(still subclasses ValueError) so route-layer handling stays uniform.
- Raw U+2028/U+2029 in cursor.py source: ripgrep treated those lines as
line-terminators, confirming the bytes were the actual separators. Any
editor save / autoformat / git tooling that normalizes invisibles would
silently break the encoder. Replaced with explicit
/
Python escape sequences.
- set(seen) == set(names) hid ordering regressions: a cursor walk that
dropped a row at a page boundary or returned duplicates could pass.
Reworked the assertion to (1) reject duplicates, (2) require full
coverage, and (3) assert strict positional order for size sort, the
only field with a clock-independent ordering.
- Flaky time.sleep(0.05) between inserts: Windows CI clock resolution is
~15ms, so back-to-back inserts under load could collide and exercise
the tiebreaker instead of the documented path. Removed the sleep and
let the strengthened assertion above carry coverage / no-duplicates,
with size sort carrying strict order.
- Cursor error envelope diverged from the rest of routes.py: cursor 400s
emitted {error: {code, message}} while every other 400 in the file
emits {error: {code, message, details}} via _build_error_response.
Switched to _build_error_response and added the details field to the
AssetsApiError schema in openapi.yaml.
- "Byte-identity fixtures" only checked substring containment, defeating
the test class's stated purpose of pinning the wire format. Switched
to exact-bytes equality against an inline expected payload string per
fixture, so any whitespace / key-order / escape drift fails loudly.
Also dropped Go / json.Marshal references from docstrings — the byte
format is the contract, not the runtime that mints it.
* fix(assets): cap cursors by encoded wire size, not just char count
Char-count guards on value/id can still let multibyte or escape-heavy
inputs blow past MAX_ENCODED_CURSOR_LENGTH once UTF-8 + escape expansion
+ base64url runs. A 512-character name of 'é' (2 bytes UTF-8) or '<'
(serializes to the 6-byte '<' escape) passes the char check, mints
a ~1500-byte cursor, then 400s when handed back on the next request.
Compute the final encoded form and reject it before returning if it
exceeds the wire cap. Adds regression tests for both inflation paths.
* refactor(assets): extract cursor JSON escaping helper; size wire cap above per-field caps
Addresses review feedback on cursor.py:
- Extract the inline escape chain into _apply_wire_compatible_json_escapes()
with a comment pinning it to the wire format's escape set, so the parity
intent is explicit rather than reading as an ad-hoc transform.
- Raise MAX_ENCODED_CURSOR_LENGTH to 8192 (comfortably above the ~5.2KB
worst-case the per-field caps can produce) and drop the mint-time length
guard. Encoder/decoder symmetry now holds by construction: the encoder
can't produce a cursor the decode path rejects, so there is no confusing
user-visible 'cursor too long' failure at mint time.
- Rewrite the two over-wire-cap tests to assert worst-case multibyte and
escape-heavy values mint and round-trip, instead of being rejected.
* refactor(assets): drop cross-runtime cursor escaping; cursors are opaque
The custom JSON escaping of <, >, &, U+2028, and U+2029 existed only to
keep the encoded cursor byte-identical with the Cloud implementation of
the same payload format. Cursors are opaque tokens, so byte-level
compatibility across implementations is not needed — plain json.dumps
output is sufficient. Remove the escaping helper and the byte-identity
test fixtures that pinned the wire format; keep round-trip coverage for
the affected characters.
---------
Co-authored-by: guill <jacob.e.segal@gmail.com>
|
||
|
Matt Millerandguill
|
a76bb4380e |
chore(assets): drop vestigial tags.tag_type column (#14248)
tag_type was always "user" in practice — no code path ever set it to anything else (no system/seeded classification was wired up) and nothing queried it. The column, its ix_tags_tag_type index, and the TagUsage.type API field were dead weight, so they're removed. Adds alembic migration 0004 to drop the column and index. Verified: asset-seeder tests pass; migration applies cleanly on a fresh SQLite (tags retains only name; tag_type column + index dropped). Co-authored-by: guill <jacob.e.segal@gmail.com> |
||
|
Matt Millerandguill
|
a0a055bc4e |
feat(assets): extract image dimensions at ingest and emit on asset responses (#13991)
* feat(assets): extract image dimensions at ingest and emit on asset responses
Image assets now carry width/height under the existing `metadata` field on
asset responses, shaped as `{"kind": "image", "width": W, "height": H}`.
This lets consumers get original dimensions (e.g. for clients that render
server-side thumbnails and can't recover them from naturalWidth/Height)
without an extra round-trip.
Dimensions are written to AssetReference.system_metadata across three
ingest paths:
- Direct file ingest (upload, in-place registration): Pillow reads the
image header right after hashing, while the file is still in OS page
cache. Non-image MIME types are skipped without touching the file.
- From-hash registration: this path never reads the file bytes, so
dimensions are best-effort copied from any prior sibling reference of
the same asset that already carries kind=image metadata. Missing
siblings, non-image siblings, or absent dimension keys leave the new
reference's metadata unchanged.
- Scanner enrichment: extends the existing system_metadata write in
enrich_asset so scanner-registered images get the same treatment as
uploaded ones.
Existing system_metadata keys (e.g. safetensors fields written by the
enricher, download provenance) are preserved through merge. Existing
assets ingested before this change retain their current metadata — no
automatic backfill in this PR.
Tests cover image emission, non-image no-op, merge preservation, and the
from-hash sibling back-fill (including the no-sibling and non-image-sibling
cases).
* fix(assets): validate sibling dimensions before backfilling
Per CodeRabbit review on #13991: the previous loop accepted any sibling
with `kind == "image"` and copied whichever dimension keys happened to
be present, then returned. A partial sibling (kind set but missing or
invalid width/height) could persist incomplete metadata onto the new
reference even when a later sibling had valid dimensions.
Now we validate that the sibling has both width and height as positive
integers before adopting its dimensions, and continue scanning to the
next sibling otherwise.
* fix(assets): reject booleans in sibling dimension validation (use type-is)
Per CodeRabbit follow-up on #13991: bool is a subclass of int in Python,
so isinstance(True, int) is True. The previous strict-int gate would
have accepted width=True (truthy + > 0) as a valid dimension.
Realistic occurrence is low (extract_image_dimensions returns proper
ints, JSON doesn't serialize bools as numbers), but the validation gate
exists for defense-in-depth so it should be actually strict.
---------
Co-authored-by: guill <jacob.e.segal@gmail.com>
|
||
|
comfyanonymous
|
da49b7d0b6 | Remove useless annotations imports. (#14105) | ||
|
|
6de7fc063b |
Emit hash alongside asset_hash on all Asset responses (#13739)
* Emit `hash` alongside `asset_hash` on all Asset responses Add a `hash` field to the Asset response schema that carries the same value as the existing `asset_hash` field. Both fields are now populated in _build_asset_response, so every Asset-returning endpoint (GET, POST, PUT) includes both. No existing fields are removed. Tests updated to assert both fields. Co-authored-by: Matt Miller <MillerMedia@users.noreply.github.com> * Tighten hash field tests and DRY response builder - Extract assert_hash_fields_consistent() helper that verifies presence parity and value equality, replacing body.get()-based assertions that treated missing keys and explicit nulls identically. - Conftest seeded_asset fixture and seed-asset list assertions now check key absence directly, so a regression that surfaces null fields would be caught (validates exclude_none behavior). - DRY duplicate hash expression in _build_asset_response. - Add list-endpoint coverage asserting hash is present and consistent on populated assets. - Add schema-level test asserting AssetCreated inherits the hash field from Asset, guarding against future inheritance drift. --------- Co-authored-by: Matt Miller <MillerMedia@users.noreply.github.com> Co-authored-by: guill <jacob.e.segal@gmail.com> |
||
|
Talmaj
|
63bcaec5d1 | Add colored logs (#14036) | ||
|
rattus
|
f48c32871b | fe: Consolidate warnings (#13970) | ||
|
Christian Byrne
|
b112f68681 | Generalize frontend version warning to all comfy* requirements.txt entries (#13875) | ||
|
Jedrzej Kosinski
|
c8673542f7 | fix: make NodeReplaceManager.register() idempotent (#13596) | ||
|
comfyanonymous
|
df7bf1d3dc | Update warning message for ComfyUI frontend installation. (#13796) | ||
|
Luke Mino-Altherr
|
2b63add0ad | fix: return millisecond timestamps from get_file_info() (#12996) | ||
|
Luke Mino-Altherr
|
3eba2dcf2d | fix(assets): recognize temp directory in asset category resolution (#13159) | ||
|
Luke Mino-Altherr
|
7d5534d8e5 | feat(assets): register output files as assets after prompt execution (#12812) | ||
|
Christian Byrne
|
9a870b5102 |
fix: atomic writes for userdata to prevent data loss on crash (#12987)
Write to a temp file in the same directory then os.replace() onto the target path. If the process crashes mid-write, the original file is left intact instead of being truncated to zero bytes. Fixes #11298 |
||
|
|
2bd4d82b4f |
feat(assets): align local API with cloud spec (#12863)
* feat(assets): align local API with cloud spec Unify response models, add missing fields, and align input schemas with the cloud OpenAPI spec at cloud.comfy.org/openapi. - Replace AssetSummary/AssetDetail/AssetUpdated with single Asset model - Add is_immutable, metadata (system_metadata), prompt_id fields - Support mime_type and preview_id in update endpoint - Make CreateFromHashBody.name optional, add mime_type, require >=1 tag - Add id/mime_type/preview_id to upload, relax tags to optional - Rename total_tags → tags in tag add/remove responses - Add GET /api/assets/tags/refine histogram endpoint - Add DB migration for system_metadata and prompt_id columns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Fix review issues: tags validation, size nullability, type annotation, hash mismatch check, and add tag histogram tests - Remove contradictory min_length=1 from CreateFromHashBody.tags default - Restore size field to int|None=None for proper null semantics - Add Union type annotation to _build_asset_response result param - Add hash mismatch validation on idempotent upload path (409 HASH_MISMATCH) - Add unit tests for list_tag_histogram service function Amp-Thread-ID: https://ampcode.com/threads/T-019cd993-f43c-704e-b3d7-6cfc3d4d4a80 Co-authored-by: Amp <amp@ampcode.com> * Add preview_url to /assets API response using /api/view endpoint For input and output assets, generate a preview_url pointing to the existing /api/view endpoint using the asset's filename and tag-derived type (input/output). Handles subdirectories via subfolder param and URL-encodes filenames with spaces, unicode, and special characters. This aligns the OSS backend response with the frontend AssetCard expectation for thumbnail rendering. Amp-Thread-ID: https://ampcode.com/threads/T-019cda3f-5c2c-751a-a906-ac6c9153ac5c Co-authored-by: Amp <amp@ampcode.com> * chore: remove unused imports from asset_reference queries Amp-Thread-ID: https://ampcode.com/threads/T-019cda7d-cb21-77b4-a51b-b965af60208c Co-authored-by: Amp <amp@ampcode.com> * feat: resolve blake3 hashes in /view endpoint via asset database Amp-Thread-ID: https://ampcode.com/threads/T-019cda7d-cb21-77b4-a51b-b965af60208c Co-authored-by: Amp <amp@ampcode.com> * Register uploaded images in asset database when --enable-assets is set Add register_file_in_place() service function to ingest module for registering already-saved files without moving them. Call it from the /upload/image endpoint to return asset metadata in the response. Amp-Thread-ID: https://ampcode.com/threads/T-019ce023-3384-7560-bacf-de40b0de0dd2 Co-authored-by: Amp <amp@ampcode.com> * Exclude None fields from asset API JSON responses Add exclude_none=True to model_dump() calls across asset routes to keep response payloads clean by omitting unset optional fields. Amp-Thread-ID: https://ampcode.com/threads/T-019ce023-3384-7560-bacf-de40b0de0dd2 Co-authored-by: Amp <amp@ampcode.com> * Add comment explaining why /view resolves blake3 hashes Amp-Thread-ID: https://ampcode.com/threads/T-019ce023-3384-7560-bacf-de40b0de0dd2 Co-authored-by: Amp <amp@ampcode.com> * Move blake3 hash resolution to asset_management service Extract resolve_hash_to_path() into asset_management.py and remove _resolve_blake3_to_path from server.py. Also revert loopback origin check to original logic. Amp-Thread-ID: https://ampcode.com/threads/T-019ce023-3384-7560-bacf-de40b0de0dd2 Co-authored-by: Amp <amp@ampcode.com> * Require at least one tag in UploadAssetSpec Enforce non-empty tags at the Pydantic validation layer so uploads with no tags are rejected with a 400 before reaching ingest. Adds test_upload_empty_tags_rejected to cover this case. Amp-Thread-ID: https://ampcode.com/threads/T-019ce377-8bde-7048-bc28-a9df063409f9 Co-authored-by: Amp <amp@ampcode.com> * Add owner_id check to resolve_hash_to_path Filter asset references by owner visibility so the /view endpoint only resolves hashes for assets the requesting user can access. Adds table-driven tests for owner visibility cases. Amp-Thread-ID: https://ampcode.com/threads/T-019ce377-8bde-7048-bc28-a9df063409f9 Co-authored-by: Amp <amp@ampcode.com> * Make ReferenceData.created_at and updated_at required Remove None defaults and type: ignore comments. Move fields before optional fields to satisfy dataclass ordering. Amp-Thread-ID: https://ampcode.com/threads/T-019ce377-8bde-7048-bc28-a9df063409f9 Co-authored-by: Amp <amp@ampcode.com> * Fix double commit in create_from_hash Move mime_type update into _register_existing_asset so it shares a single transaction with reference creation. Log a warning when the hash is not found instead of silently returning None. Amp-Thread-ID: https://ampcode.com/threads/T-019ce377-8bde-7048-bc28-a9df063409f9 Co-authored-by: Amp <amp@ampcode.com> * Add exclude_none=True to create/upload responses Align with get/update/list endpoints for consistent JSON output. Amp-Thread-ID: https://ampcode.com/threads/T-019ce377-8bde-7048-bc28-a9df063409f9 Co-authored-by: Amp <amp@ampcode.com> * Change preview_id to reference asset by reference ID, not content ID Clients receive preview_id in API responses but could not dereference it through public routes (which use reference IDs). Now preview_id is a self-referential FK to asset_references.id so the value is directly usable in the public API. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Filter soft-deleted and missing refs from visibility queries list_references_by_asset_id and list_tags_with_usage were not filtering out deleted_at/is_missing refs, allowing /view?filename=blake3:... to serve files through hidden references and inflating tag usage counts. Add list_all_file_paths_by_asset_id for orphan cleanup which intentionally needs unfiltered access. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Pass preview_id and mime_type through all asset creation fast paths The duplicate-content upload path and hash-based creation paths were silently dropping preview_id and mime_type. This wires both fields through _register_existing_asset, create_from_hash, and all route call sites so behavior is consistent regardless of whether the asset content already exists. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Remove unimplemented client-provided ID from upload API The `id` field on UploadAssetSpec was advertised for idempotent creation but never actually honored when creating new references. Remove it rather than implementing the feature. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Make asset mime_type immutable after first ingest Prevents cross-tenant metadata mutation when multiple references share the same content-addressed Asset row. mime_type can now only be set when NULL (first ingest); subsequent attempts to change it are silently ignored. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Use resolved content_type from asset lookup in /view endpoint The /view endpoint was discarding the content_type computed by resolve_hash_to_path() and re-guessing from the filename, which produced wrong results for extensionless files or mismatched extensions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Merge system+user metadata into filter projection Extract rebuild_metadata_projection() to build AssetReferenceMeta rows from {**system_metadata, **user_metadata}, so system-generated metadata is queryable via metadata_filter and user keys override system keys. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Standardize tag ordering to alphabetical across all endpoints Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Derive subfolder tags from path in register_file_in_place Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Reject client-provided id, fix preview URLs, rename tags→total_tags - Reject 'id' field in multipart upload with 400 UNSUPPORTED_FIELD instead of silently ignoring it - Build preview URL from the preview asset's own metadata rather than the parent asset's - Rename 'tags' to 'total_tags' in TagsAdd/TagsRemove response schemas for clarity Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: SQLite migration 0003 FK drop fails on file-backed DBs (MB-2) Add naming_convention to Base.metadata so Alembic batch-mode reflection can match unnamed FK constraints created by migration 0002. Pass naming_convention and render_as_batch=True through env.py online config. Add migration roundtrip tests (upgrade/downgrade/cycle from baseline). Amp-Thread-ID: https://ampcode.com/threads/T-019ce466-1683-7471-b6e1-bb078223cda0 Co-authored-by: Amp <amp@ampcode.com> * Fix missing tag count for is_missing references and update test for total_tags field - Allow is_missing=True references to be counted in list_tags_with_usage when the tag is 'missing', so the missing tag count reflects all references that have been tagged as missing - Add update_is_missing_by_asset_id query helper for bulk updates by asset - Update test_add_and_remove_tags to use 'total_tags' matching the API schema Amp-Thread-ID: https://ampcode.com/threads/T-019ce482-05e7-7324-a1b0-a56a929cc7ef Co-authored-by: Amp <amp@ampcode.com> * Remove unused imports in scanner.py Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Rename prompt_id to job_id on asset_references Rename the column in the DB model, migration, and service schemas. The API response emits both job_id and prompt_id (deprecated alias) for backward compatibility with the cloud API. Amp-Thread-ID: https://ampcode.com/threads/T-019cef41-60b0-752a-aa3c-ed7f20fda2f7 Co-authored-by: Amp <amp@ampcode.com> * Add index on asset_references.preview_id for FK cascade performance Amp-Thread-ID: https://ampcode.com/threads/T-019cef45-a4d2-7548-86d2-d46bcd3db419 Co-authored-by: Amp <amp@ampcode.com> * Add clarifying comments for Asset/AssetReference naming and preview_id Amp-Thread-ID: https://ampcode.com/threads/T-019cef49-f94e-7348-bf23-9a19ebf65e0d Co-authored-by: Amp <amp@ampcode.com> * Disallow all-null meta rows: add CHECK constraint, skip null values on write - convert_metadata_to_rows returns [] for None values instead of an all-null row - Remove dead None branch from _scalar_to_row - Simplify null filter in common.py to just check for row absence - Add CHECK constraint ck_asset_reference_meta_has_value to model and migration 0003 Amp-Thread-ID: https://ampcode.com/threads/T-019cef4e-5240-7749-bb25-1f17fcf9c09c Co-authored-by: Amp <amp@ampcode.com> * Remove dead None guards on result.asset in upload handler register_file_in_place guarantees a non-None asset, so the 'if result.asset else None' checks were unreachable. Amp-Thread-ID: https://ampcode.com/threads/T-019cef5b-4cf8-723c-8a98-8fb8f333c133 Co-authored-by: Amp <amp@ampcode.com> * Remove mime_type from asset update API Clients can no longer modify mime_type after asset creation via the PUT /api/assets/{id} endpoint. This reduces the risk of mime_type spoofing. The internal update_asset_hash_and_mime function remains available for server-side use (e.g., enrichment). Amp-Thread-ID: https://ampcode.com/threads/T-019cef5d-8d61-75cc-a1c6-2841ac395648 Co-authored-by: Amp <amp@ampcode.com> * Fix migration constraint naming double-prefix and NULL in mixed metadata lists - Use fully-rendered constraint names in migration 0003 to avoid the naming convention doubling the ck_ prefix on batch operations. - Add table_args to downgrade so SQLite batch mode can find the CHECK constraint (not exposed by SQLite reflection). - Fix model CheckConstraint name to use bare 'has_value' (convention auto-prefixes). - Skip None items when converting metadata lists to rows, preventing all-NULL rows that violate the has_value check constraint. Amp-Thread-ID: https://ampcode.com/threads/T-019cef87-94f9-7172-a6af-c6282290ce4f Co-authored-by: Amp <amp@ampcode.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Amp <amp@ampcode.com> |
||
|
comfyanonymous
|
e4b0bb8305 | Import assets seeder later, print some package versions. (#12841) | ||
|
Luke Mino-Altherr
|
29b24cb517 | refactor(assets): modular architecture + async two-phase scanner & background seeder (#12621) | ||
|
comfyanonymous
|
17106cb124 | Move parsing of requirements logic to function. (#12701) | ||
|
Jedrzej Kosinski
|
33f83d53ae |
Fix KeyError when prompt entries lack class_type key (#12595)
Skip entries in the prompt dict that don't contain a class_type key in apply_replacements(), preventing crashes on metadata or non-node entries. Fixes Comfy-Org/ComfyUI#12517 |