Add Comfy-Usage-Source pass-through for API node requests#14404
Conversation
robinjhuang
requested review from
Kosinkadink,
alexisrolland,
comfyanonymous,
guill,
kijai and
rattus128
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new hidden input COMFY_USAGE_SOURCE (Hidden.comfy_usage_source / comfy_usage_source) accepted by HiddenHolder and registered for API nodes. The server copies an incoming Comfy-Usage-Source request header into extra_data when missing; execution reads that into hidden inputs (V3 and legacy). get_usage_source and get_comfy_api_headers expose the value (fallback "comfyui-api"), and client, Sonilo, and cloud-download code use get_comfy_api_headers so outbound requests include a Comfy-Usage-Source header. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Capture the Comfy-Usage-Source header (or extra_data.comfy_usage_source) on POST /prompt and forward it on API nodes' outbound requests to api.comfy.org, defaulting to comfyui-server when absent.
robinjhuang
force-pushed
the
add-comfy-usage-source-header
branch
from
76977f4 to
d3878ed
Compare
|
I am concerned this could be redundant or create confusion with the
What do you think @robinjhuang ? |
So usage source is the surface that the user is initiating the call from. |
…-Org#12772) Adds `comfy_usage_source: 'comfyui-frontend'` to the prompt body's `extra_data`. The backend forwards this to API nodes' upstream requests via the `Comfy-Usage-Source` header, so partner node API usage can be attributed to the frontend. Used in Comfy-Org/ComfyUI#14404
The definition of environment (where it's installed) vs surface (from where it's used) make sense, but then we would need to adjust the values a bit:
I think it would make sense after adjusting the values:
|
|
Overall looks fine, only was able to find that Comfy-Env is not sent to everything + the manual requirement to add the header to sonilo. Adding a helper to make sure Comfy-Env + Comfy-Usage-Source will be accounted for universally would help, in case we add more headers in the future so we don't forget anything. |
|
Posted by an AI agent (Amp) on behalf of @Kosinkadink. Nice clean pass-through! One suggestion: the three outbound call sites that hit the Comfy API now build an overlapping header set by hand, and they've drifted — Consider factoring the common set into a single helper in def get_comfy_api_headers(node_cls: type[IO.ComfyNode]) -> dict[str, str]:
"""Common headers (auth, deploy environment, usage source) for Comfy API requests.
Centralizes these headers so every Comfy API request sends a consistent set and new
shared headers only need to be added in one place. Intended for relative/cloud URLs
resolved against ``default_base_url()``; because the result includes auth, callers must
not attach it to arbitrary absolute/presigned URLs.
"""
return {
**get_auth_header(node_cls),
"Comfy-Env": get_deploy_environment(),
"Comfy-Usage-Source": get_usage_source(node_cls),
}Then the call sites collapse to one line each: # client.py (relative URLs only)
payload_headers.update(get_comfy_api_headers(cfg.node_cls))
# nodes_sonilo.py
headers = get_comfy_api_headers(cls)
headers.update(endpoint.headers)
# download_helpers.py (relative 'cloud' URLs only)
headers = get_comfy_api_headers(cls)Note this also adds |
Yes! Comfy-Usage-Source: comfyui-server is for direct api calls. Updated to send as |
|
@Kosinkadink Done in b58af42 — shared headers (auth, Comfy-Env, Comfy-Usage-Source) are now centralized in |
|
nice, lgtm |
|
@alexisrolland any further comments on this? |
…nto add-comfy-usage-source-header
* feat(assets): add job_ids filter to GET /api/assets (Comfy-Org#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 Comfy-Org#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> * feat(assets): include asset id in executed WebSocket message (Comfy-Org#13862) * feat(assets): enrich executed WS message with asset metadata When --enable-assets is set, each file-type output entry in the `executed` WebSocket message now includes id, name, asset_hash, size, and mime_type — matching the shape already returned by /upload/image. The enrichment lives in comfy_execution/asset_enrichment.py (no torch dependency) and is called from both send sites in execution.py: freshly executed nodes register the file inline via register_file_in_place; cached node re-sends look up the existing AssetReference by file path to avoid re-hashing. Errors are caught per-entry so a failure never blocks the WS message from sending. * fix(assets): inject only id in executed WS message per Asset Identity RFC Per the Asset Identity RFC, the executed WebSocket payload should carry id alone — hash is already encoded in the filename, and name/preview_url/ size belong behind GET /api/assets/{id} rather than being pushed eagerly. Simplifies the DB lookup path: we only need ref.id, so the asset.hash null-check is no longer required as a fallback trigger. * fix(assets): reject path traversal when resolving output abs_path Subfolder/filename were joined and absolutized without containment check, so '..' segments or an absolute filename could escape the type's base directory and register an unrelated on-disk file as an asset. Add commonpath-based containment check; skip enrichment (warn, leave entry unchanged) when the resolved path escapes base. Catches ValueError from cross-drive paths on Windows. * docs(assets): drop Asset Identity RFC reference from docstring * docs(assets): trim docstring to what enrichment does, not what it doesn't * test(assets): use real platform paths so containment check works on Windows The previous test setup patched os.path.abspath to identity and used a POSIX-style '/output' base, which collided with Windows path separators in os.path.commonpath. Drop the abspath/join patches and use a real tempdir-rooted base so the containment check runs against actual platform paths. * refactor(assets): enrich at output-processing time, not in the WS send path Per review: enrichment lived inside the client_id-guarded send sites, so a headless run (no websocket client) never registered assets at all, and ui_outputs/history stored the un-enriched entries. Now output_ui is enriched once, right after the node produces it and before it is stored in ui_outputs — so registration happens regardless of connected clients, and the asset id flows into history and the execution cache for free. _send_cached_ui re-sends the stored (already-enriched) dict verbatim, which lets the DB-lookup-by-path fallback be deleted: every enrichment is now a fresh output, and register_file_in_place re-hashes on upsert so an overwritten path can never carry a stale id. * revert(assets): drop job_ids filter from GET /api/assets (Comfy-Org#14408) The job_ids query filter added in Comfy-Org#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 Comfy-Org#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. * chore(openapi): sync shared API contract from cloud@e3c52ad (Comfy-Org#14406) * I don't think this actually works anymore. (Comfy-Org#14403) * ops: tolerate already force casted dynamic weight (Comfy-Org#14410) Some custom nodes .to weights completely out of load context which can wreak havoc if its for a model that is not active. Detect this condition and just let it fall-through to the non-dynamic loader straight up. * Improve context window resizing for SCAIL2 (CORE-286) (Comfy-Org#14394) * Fix SCAIL-2 reference mask background convention (Comfy-Org#14415) * [Partner Nodes] fix(GPT Image): handle mismatched image sizes returned when size="auto" (Comfy-Org#14414) Signed-off-by: bigcat88 <bigcat88@icloud.com> * [Partner Nodes] fix(KlingTextToVideoNode): validation error for "kling-v2-master" model (Comfy-Org#14418) Signed-off-by: bigcat88 <bigcat88@icloud.com> * Make --enable-manager-legacy-ui imply --enable-manager (Comfy-Org#14421) * Don't crash when using flux kv cache with split batches. (Comfy-Org#14422) * Add Comfy-Usage-Source pass-through for API node requests (Comfy-Org#14404) * [Partner Nodes] feat: enable Bria Replace Background node (Comfy-Org#14397) * Fix potential dtype issue with ideogram 4. (Comfy-Org#14436) * add --high-ram option (Comfy-Org#14437) Add this option for users who know they have so much ram they want to pin everything or have a pagefile that outruns their disk speed. The removes the RAM pressure caps completely and pins behind the primary model load forcing all models to be permanently comitted to RAM. * [Partner Nodes] feat: add Runway Aleph2 node (Comfy-Org#14306) Signed-off-by: bigcat88 <bigcat88@icloud.com> * Use comfy kitchen apply rope in omnigen2 model. (Comfy-Org#14442) * Add 10-bit video support (Comfy-Org#14452) Create Video gets a bit_depth option (8-bit/10-bit); the selected depth is carried by the video and applied when it gets encoded. Save Video and Video Slice now keep the source bit depth instead of always quantizing to 8-bit, so 10-bit videos stay 10-bit. 10-bit uses h264 with the yuv420p10le pixel format,so there's no new codec or container. Signed-off-by: bigcat88 <bigcat88@icloud.com> * Expose deploy_environment in /system_stats (Comfy-Org#14402) * Remove the comfy python path append. * Revert last commit. Last time I use this stupid GitHub app. * Fix nondeterministic video decode at unaligned widths (CORE-299) (Comfy-Org#14438) * [Partner Nodes] feat(Tripo3d): add new "Import 3D" node (Comfy-Org#14466) Signed-off-by: bigcat88 <bigcat88@icloud.com> * bump manager version to 4.2.2 (Comfy-Org#14471) * This is already auto enabled by default. (Comfy-Org#14476) * chore: update embedded docs to v0.5.4 (Comfy-Org#14478) * fix(ruff): exclude vendored ComfyUI tree from lint (scope to PMOVES code) * fix(ruff): resolve PMOVES-owned lint in pmoves_health Scoped ruff (after excluding vendored ComfyUI) surfaced PMOVES-owned violations: remove unused imports (F401), fix undefined FastAPI annotation (F821) by importing it at module scope, and # noqa intentional demo/CLI prints (T201). * fix(ruff): resolve PMOVES-owned lint in pmoves_announcer Scoped ruff (after excluding vendored ComfyUI) surfaced PMOVES-owned violations: remove unused imports (F401), fix undefined FastAPI annotation (F821) by importing it at module scope, and # noqa intentional demo/CLI prints (T201). * fix(ruff): resolve PMOVES-owned lint in pmoves_registry Scoped ruff (after excluding vendored ComfyUI) surfaced PMOVES-owned violations: remove unused imports (F401), fix undefined FastAPI annotation (F821) by importing it at module scope, and # noqa intentional demo/CLI prints (T201). --------- Signed-off-by: bigcat88 <bigcat88@icloud.com> Co-authored-by: Matt Miller <mattmiller@comfy.org> Co-authored-by: guill <jacob.e.segal@gmail.com> Co-authored-by: Comfy Org PR Bot <snomiao+comfy-pr@gmail.com> Co-authored-by: comfyanonymous <121283862+comfyanonymous@users.noreply.github.com> Co-authored-by: rattus <46076784+rattus128@users.noreply.github.com> Co-authored-by: Barish Ozbay <17261091+drozbay@users.noreply.github.com> Co-authored-by: Jukka Seppänen <40791699+kijai@users.noreply.github.com> Co-authored-by: Alexander Piskun <13381981+bigcat88@users.noreply.github.com> Co-authored-by: Jedrzej Kosinski <kosinkadink1@gmail.com> Co-authored-by: Robin Huang <robin.j.huang@gmail.com> Co-authored-by: John Pollock <pollockjj@users.noreply.github.com> Co-authored-by: Dr.Lt.Data <128333288+ltdrdata@users.noreply.github.com> Co-authored-by: Daxiong (Lin) <contact@comfyui-wiki.com> Co-authored-by: pmoves-fork-sync[bot] <pmoves-fork-sync[bot]@users.noreply.github.com> Co-authored-by: POWERFULMOVES <142271328+POWERFULMOVES@users.noreply.github.com>
3 participants
Captures the
Comfy-Usage-Sourceheader (orextra_data.comfy_usage_source) onPOST /promptand forwards it on API nodes' requests to api.comfy.org, defaulting tocomfyui-apifor direct API calls that don't identify themselves. Lets partner node API usage be attributed to the originating client. Shared outbound headers (auth,Comfy-Env,Comfy-Usage-Source) are centralized in aget_comfy_api_headers()helper — note this also addsComfy-Envto the Sonilo streaming and relative download paths, which previously only sent auth.API Node PR Checklist
Scope
Pricing & Billing
If Need pricing update:
QA
Comms