mirror of
https://github.com/Comfy-Org/ComfyUI.git
synced 2026-08-31 05:16:01 +08:00
7e26c68d39
`POST /api/assets` treated "same bytes, same display name" as "you already have this" and handed back the existing record with `created_new=false`/HTTP 200. That silently discarded everything the second request supplied - its tags, its `user_metadata`, its `preview_id` - and made record identity depend on a display name, which rulings 5 and 8 say is a label and may legitimately repeat. An upload is a delivery event, so it now always mints its own record, exactly as the executed-output path already does when a cached rerun reuses content. Content dedup is untouched and is the only dedup left: identical bytes still share one `AssetContent` row, in both hashing modes. `lookup_for_upload_dedup` existed solely to find the same-name record and, with that arm gone, reduced to `lookup_for_view`, so ingest calls `lookup_for_view` directly and the function is deleted. USER RULING: this makes a retried upload non-idempotent - a client that resends after a timeout gets a second record. That is accepted deliberately, not overlooked. No idempotency key or retry protection is introduced here; making retry safe is a separate decision about a client-supplied identity, and the multipart parser still rejects a client-provided `id`. Concurrency safety - the content-reuse arm is a compare-and-swap, not a read-then-write: The old content-reuse arm selected a content row in one session, deleted the uploaded bytes, then opened a fresh session to attach a record to the now-detached id. A scanner pass or competing writer retiring that row in between produced a record pointing at content already gone, with no copy of the bytes left to fall back on. Removing the exact-record arm routes every same-name upload through this arm instead of only cross-name ones, so the fix ships in the same commit. A first version of this fix replaced the two-session gap with a same-session SELECT re-check (`content_still_qualified`) between the lookup and the insert. An adversarial review reproduced two concurrency holes that survived it: (1) the re-check never compared the row's hash, so a concurrent hash correction (e.g. `detect_content_change`) went undetected; (2) a plain SELECT does not hold SQLite's write lock, so a competing writer could still commit a retirement in the window between the re-check passing and this session's own commit - because pysqlite only opens an implicit transaction (acquiring the write lock) before a DML statement, never before a SELECT. `claim_qualified_content` (`lookup.py`) replaces that re-check with a conditional UPDATE: `UPDATE asset_contents SET is_missing = is_missing WHERE id = :id AND hash = :hash AND is_missing = 0`, a no-op write whose only job is to take the write lock and prove the row's state atomically. `rowcount == 1` means both facts were true at the instant of the write; the row's true hash is now part of the check, and because it is a write (not a read), SQLite holds this connection's lock continuously from that statement through this session's own commit. That lock is database-file-wide, not row-scoped - this app's default rollback-journal SQLite locking has no row-level granularity - so it briefly serializes every writer in the app, not only writers to this row, for the length of this short critical section (claim, filesystem re-check, record creation, commit). That breadth is what makes the guarantee hold, not an accident to narrow down. `refresh_qualified_content` then reruns the unchanged filesystem-level predicate (`_qualifies`: exists, stat-consistent, non-temp) against a forced re-read, since a DB transaction cannot make the filesystem itself atomic against an external process. Either check failing rolls back and falls through to the ordinary new-content path with the upload still on disk - unchanged from before. Proven with two tests using two independent file-backed SQLite connections (not `:memory:`, which is a single shared connection): one where a second connection commits a hash change between the lookup and the claim (must reject and fall back), one where a second connection's retirement attempt during the claim's held lock must itself fail (`sqlite3`/`OperationalError` naming lock contention specifically), not merely lose a race. Both were confirmed to reproduce against the prior (first-draft) mechanism via a standalone repro before being written as this shape. `openapi.yaml`: the multipart operation loses its HTTP 200 outcome (now unreachable) and its `id` field (dead spec - the parser rejects `id` with UNSUPPORTED_FIELD), and `created_new` is described as what it now is. Rollback: reverting restores the exact-record dedup arm (and with it the original race).