Comfy-Org / Comfy-Org/comfy-api-proxy

v2 outputs silently drop non-file (value) node outputs

Open
#22 5 comments 1 reaction 0 assignees View on GitHub
Dominant language
Python
Stars
11
Forks
5
Avg merge
1d 6h
Merged PRs (30d)
3

Description

Production context is in the results comment on #18 (PR #21's conversation is locked); filed separately so the proposal has its own thread, the way #18 worked out.

## The gap, with a live repro

`_OUTPUT_KIND` maps `"text"`, but the item loop right below it requires `isinstance(it, dict) and "filename" in it`. `PreviewAny` history items are raw strings, so they are dropped. Submitted through the proxy against a real CPU-only ComfyUI instance:

- `LoadImage -> PreviewAny` via v2: `succeeded` with `outputs: []`
- the same prompt id in `/history`: `outputs` holds the node's `text` list

A value-producing job succeeds and delivers nothing, with no signal that anything was dropped. The events stream is consistent with the poll, for what it is worth: its snapshots build `outputs` through the same mapping, so an SSE listener sees the same empty list.

The spec seems to agree this is a bug rather than a choice: `OutputType`'s own description reads "Normalized output kind — nothing silently dropped" (`spec/openapi.yaml:846`), and the enum right above it lists `file`, which no `_OUTPUT_KIND` entry can currently emit either.

Why we care: two of our four production lanes are not renderers. One computes a face-embedding distance and reads back a scalar of a few dozen bytes; the other extracts pose keypoints and reads back a ~16 KB JSON frame. Both read `outputs[node]["text"][0]`; no file is ever written; the value is the output. Those lanes run hundreds of these jobs a week and stay on v1 for now, which works, but it leaves "same code, different base URL" with a workload-shaped hole: anything that computes rather than renders.

The obvious workaround hits the same wall, and we confirmed this one at runtime too: core ships `SaveText` (`comfy_extras/nodes_text.py`), and running it through the proxy gives `succeeded` with `outputs: []` while history holds both the raw text and a perfectly file-shaped entry (`{"filename": "pr21_savetext_00001.json", "subfolder": "", "type": "output"}`) under a `files` key that `_OUTPUT_KIND` does not map. That item would pass the filename filter; it never gets there. So the save-to-a-file escape route writes a real file to disk and v2 still cannot see it, which turns this from "one node type is unlucky" into "the key map is missing entries the core node set actively uses".

## The shape we would pitch

**Route value outputs through the machinery you already ship.** When `_outputs` meets a raw-string item under a mapped key, mint a signed output id the way file outputs get one, with (job, node, index) in the payload where files carry a filename, and emit the standard entry shape: `type: "text"`, `content_type: text/plain`, real `size_bytes`, and a `url` at the existing `GET /api/v2/assets/{id}/content`. For these ids the content handler would read through to `/history` at fetch time; evicted history answers with the same typed `404 output_unavailable` a file gets. No new endpoint, no new schema (the `Output.type` enum already lists `text`), no new storage: the proxy holds nothing, and the value has the same failure semantics as a file output, with no new durability promise. Nothing in the shape is proxy-local, so Cloud can honor the same contract however it stores outputs.

To defuse the obvious worries up front: `content_type` pinned to `text/plain` with nosniff, so this can never become an HTML or script delivery channel; and the size question does not arise the way it did for `metadata`, because these bytes are generated by graph execution and already sit in `/history` uncapped. The proxy would only be re-exposing them, not storing anything client-supplied.

If minting text assets deserves more design time than a quick fix allows, the minimal change is smaller: make the drop visible. A typed marker on the job (or a documented omission note in the outputs contract) so that a job with dropped outputs says so, instead of reporting a clean empty list.

Optionally, and freely declinable: an inline `value` mirror on the entry for values at or under your existing 1 KiB metadata constant, with typed overflow to the URL, never truncation. For precedent, Replicate and RunPod both return computed values inline in the polled job object, and AWS Step Functions carries inline state output up to a hard 256 KiB with a typed `States.DataLimitExceeded` beyond it. Our own keypoint frames would overflow to the URL anyway; the asset URL alone fully fixes the data loss, so this is ergonomics for poll-heavy readers, nothing more.

What we are not asking for: submit-time capture declarations, raw-history passthrough endpoints, or any uncapped inline channel.

## Offer

Happy to send the patch and the regression test (with a fake-upstream fixture that emits raw-string text items) rather than just describing them. We can also validate against the two production scan lanes that motivated this.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.