mirror of https://github.com/Comfy-Org/ComfyUI.git synced 2026-08-30 21:06:11 +08:00

Files

Simon Pinfold 7e26c68d39 fix(assets): multipart upload always mints a delivery record; dedup reuses content only

`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).

2026-08-28 21:50:44 -07:00