diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f4968d..90e6e52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,27 @@ notes for each version. ### Added +- `Job.get_logs()` / `AsyncJob.get_logs()` fetch a job's execution log — what + the run printed — returning a `JobLogs` (`text`, `truncated`, `captured_at`, + `complete`) or `None`. Fetched on demand and never cached: submitting and + polling a job downloads no log, and each call re-reads rather than replaying + the first, so polling for a log that has not landed yet works. `None` is an + ordinary answer covering every reason there is nothing to read — the + deployment captures no logs at all (Comfy Cloud and self-hosted never do; + only serverless deployments have them), the job has not finished, it predates + capture, the run was killed before the worker could report an outcome (an + out-of-memory kill, a crashed worker, a timeout, a job past its maximum + runtime), capture failed, or the job ran on the public demo deployment, + which captures a log but withholds it from anonymous callers — and the + contract deliberately does not distinguish them. Do not branch on which; a + job that has not finished may have a log once it has, so read again after a + terminal status. On a surface that offers a logs link a missing job still + raises `NotFound`; where there is no link there is no request, so that job + raises nothing and returns `None` too. +- `Output.node_id` is documented as possibly empty — the workflow node that + reported a file, or `""` when the worker named none. `Job.get_outputs()` + filters on this value, so an output the worker named no node for is not + reachable by any real node id. - Every exception `client.models.run()` raises **for a failed call** now carries the `Idempotency-Key` it was made under, on `.idempotency_key` — the typed `RouterError` buckets, a `RouterError` whose `error_type` this version diff --git a/README.md b/README.md index ac04659..374a69d 100644 --- a/README.md +++ b/README.md @@ -319,6 +319,57 @@ controls — jobs submitted through this SDK always get `"api"` today, since v2 submission has no version-pinning fields yet. (`AsyncJob.get_workflow()` mirrors this with `await`.) +## Reading what a run printed + +`get_logs()` fetches the job's execution log. It is a resource of its own, not +a field on the job, so running and polling a job never downloads it — you pay +for a log only when you ask for one: + +```python +# submit() + wait(), not run(): run() raises JobFailed on a failed job and the +# exception carries no handle, so there would be nothing left to ask for a log +# — which is exactly when you want one. +job = client.submit(wf) +job.wait() + +logs = job.get_logs() +if logs is not None: + print(logs.text) +``` + +`None` is an ordinary answer, not an error, and the reasons are deliberately +not distinguished: the deployment does not capture logs at all (Comfy Cloud +and self-hosted never do — only serverless deployments have them), the job has +not finished, it predates log capture, the run was killed before the worker +could report an outcome (an out-of-memory kill, a crashed worker, a timeout, a +job past its maximum runtime), capture failed, or the job ran on the +public demo deployment, which captures a log but withholds it from the +anonymous callers that surface accepts. + +The killed-run case is a known gap rather than an oversight: a log is read +back off the worker, so a run the platform killed never produced one — the +failures you most want a log for are the ones least likely to have left one. + +Do not branch on which one it is — a `None` never says. But one of them +resolves itself: a job that has not finished may have a log once it does, so +call again after a terminal status. `None` on a job that has already finished +is final, and so is `None` from a deployment that offers no log link at all. + +Nothing is cached — each call re-reads, so that retry works. On a log that did +land: + +- `text` is untrusted output. A workflow chooses what goes in it, so render it + as plain text rather than interpreting it. +- `truncated` means the *beginning* was dropped and you have the tail, which is + where a failure normally is. True with an empty `text` means the log was + captured and then shed entirely to fit — which is not the same as never + having had one. +- `complete` means nothing more will be appended. Always true today, since a + log is read back off the worker once, when the run ends. + +Live log streaming is not available yet. (`AsyncJob.get_logs()` mirrors this +with `await`.) + ## Downloading outputs A finished job exposes its results as `Output` handles — `job.outputs`, or diff --git a/spec/openapi.yaml b/spec/openapi.yaml index 7d38013..f7a9bcf 100644 --- a/spec/openapi.yaml +++ b/spec/openapi.yaml @@ -516,6 +516,134 @@ paths: $ref: '#/components/responses/RateLimited' '500': $ref: '#/components/responses/UpstreamError' + /api/v2/jobs/{id}/logs: + get: + operationId: getJobLogs + tags: + - jobs + summary: What the run printed + description: 'Returns the job''s captured execution log. Fetched on demand: a log + + is a debugging artifact a caller wants occasionally, while + + `GET /api/v2/jobs/{id}` is polled to terminal on every run, so the + + log is a resource of its own rather than a field that would ride + + every one of those polls to be read at most once. + + + Captured whenever the worker reports its own outcome, success and + + failure alike, since a job that succeeds while producing the wrong + + thing is exactly what a failure-only log cannot explain. A run the + + platform or the provider killed — out of memory, a crashed worker, a + + timeout, a job past its maximum runtime — never gets that far, so it + + reaches a terminal status carrying no log at all. That is a real gap + + and worth stating: the failures a caller most wants a log for are + + the ones least likely to have produced one. + + + **`204` is the normal answer for a job with no log**, and the cases + + behind it are deliberately not distinguished: this surface does not + + capture logs at all, the job has not finished, the job predates log + + capture, the run was killed before the worker could report one, + + capture was attempted and failed, or the job ran on the public demo + + deployment, which captures and stores the log like every other + + serverless deployment but withholds it on read, because that surface + + takes callers with no credential and a job id would otherwise be the + + only thing between one anonymous caller and another''s run. + + + Because a `204` never says which of those it is, do not branch on the + + reason — but do note that one of them resolves itself. A job that has + + not finished may have a log once it does, so a caller that wants one + + reads again after a terminal status. A `204` on a job already in a + + terminal state is final, and so is a missing `urls.logs`; both mean + + stop asking. + + + **Only jobs run on the serverless platform** (a + + `{deployment}.run.comfy.app` host) have one today. An implementation + + that captures no logs must still serve this operation, answering + + `204` for every job it can read, so that the two answers stay + + distinct — Comfy Cloud does. A self-hosted deployment on a build + + predating this operation has not implemented it yet and will answer + + a routing `404` instead, which is the case `job.urls.logs` exists to + + keep a client out of: its absence says the surface has no logs at + + all, without a request. + + + Tied to the job''s own retention: this `404`s under the same + + conditions `GET /api/v2/jobs/{id}` does (unknown, not-yours, or past + + its retention deadline). Nothing ages a log out ahead of the job''s + + own `expires_at`, so a job never outlives its log. + + + Live tailing is not offered here yet. When it is, it arrives on this + + same path under `Accept: text/event-stream`, leaving this + + JSON snapshot the default; its resume semantics will be defined + + then, against a capture that is incremental. Until then the SSE + + `log` event on `GET /api/v2/jobs/{id}/events` is the reserved live + + rail, and this is the authoritative snapshot it reconciles against. + + ' + parameters: + - $ref: '#/components/parameters/JobId' + responses: + '200': + description: The captured log. + content: + application/json: + schema: + $ref: '#/components/schemas/JobLogs' + '204': + description: This job has no log. A normal answer, not an error — see the description for the cases it covers. + '401': + $ref: '#/components/responses/Unauthorized' + '403': + $ref: '#/components/responses/Forbidden' + '404': + $ref: '#/components/responses/NotFound' + '429': + $ref: '#/components/responses/RateLimited' + '500': + $ref: '#/components/responses/UpstreamError' /api/v2/jobs/{id}/events: get: operationId: getJobEvents @@ -556,7 +684,7 @@ paths: description: 'Emitted the moment each output asset is committed, carrying the same `Output` object that appears on `job.outputs[]`. A latency optimization only: it lets a client render each result as it lands instead of waiting for the terminal `status` event. It is delivered best-effort over the live broadcast path — an output whose durable asset record is not yet resolvable when its node finishes may be delivered on a slightly later event or, failing that, only in the terminal `status` snapshot — so the authoritative, complete set of outputs is always `job.outputs[]` on `GET /api/v2/jobs/{id}` and on the terminal `status` event. A client must therefore treat these as additive hints and must not assume it receives one per output.' schema: '#/components/schemas/Output' log: - description: Selected execution log lines. Best-effort diagnostics; the one event type with no snapshot equivalent. NOT YET EMITTED by the server in the first iteration — reserved in the catalog so the wire contract is stable. Clients must not depend on receiving this event yet. + description: 'Selected execution log lines, carried while the run is still going. Best-effort diagnostics, and lossy by the same rule as the rest of this stream: lines emitted while a client was disconnected are gone and no `Last-Event-ID` replays them. The authoritative, complete log is the snapshot at `GET /api/v2/jobs/{id}/logs`, which a client re-reads after a terminal status to reconcile whatever it missed — on a surface that captures logs at all. Comfy Cloud does not, and answers `204` there for every job, so this event has nothing to be the live view of; see that operation for what a self-hosted deployment answers. NOT YET EMITTED by the server in the first iteration — reserved in the catalog so the wire contract is stable. Clients must not depend on receiving this event yet: to get a log today, stream to a terminal status and read the snapshot.' x-sse-not-yet-emitted: true schema: '#/components/schemas/LogEvent' parameters: @@ -821,6 +949,28 @@ components: execution_ms: 42000 urls: $ref: '#/components/schemas/JobUrls' + JobLogs: + type: object + description: 'A job''s captured execution log — the body of `GET /api/v2/jobs/{id}/logs`. Diagnostics, not a contract on content: this is whatever the workflow''s own code and nodes wrote to standard output, in the order they wrote it, so nothing about its shape is stable between runs or between releases of a build. It is **untrusted text** — a workflow chooses what goes in it — and must be rendered as plain text rather than interpreted.' + required: + - text + - truncated + - captured_at + - complete + properties: + text: + type: string + description: The captured output. + truncated: + type: boolean + description: 'The BEGINNING of the captured output was discarded — `text` is the TAIL of a longer run. Implementations bound what they capture and store, so a workflow that prints megabytes keeps its last lines, where a failure normally is, instead of being dropped whole. True with an empty `text` means the log was captured and then shed entirely to fit. This describes the stored log, never the response: it does not mean a caller asked for part of one.' + captured_at: + type: string + format: date-time + description: When the run's output was read back off the worker. + complete: + type: boolean + description: No further output will be appended to this log. Always `true` today, because a log is read back off the worker once, when the run ends, so a log that exists is already whole. Sent so that a surface which later captures output while a run is still going can say so, and a client written now against `false` keeps working when it does. `false` does not promise that more output will arrive, only that this snapshot may not be the last one. JobWorkflowResponse: type: object description: The workflow behind a job. See GET /api/v2/jobs/{id}/workflow's description for exactly when `format` is `save` vs `api`. @@ -872,6 +1022,12 @@ components: cancel: type: string format: uri-reference + logs: + type: string + format: uri-reference + description: 'Where to read what this run printed. Present on any surface that captures execution logs, which is why it is the one link here that is optional: absent means this surface captures none, for any job, so a client can stop looking without spending a request on an answer it already has. + + Follow this link rather than building the path from the job id. The two are not interchangeable: a surface may be mounted under a prefix this link already carries and a hand-built path would not, and a surface that does not implement the operation at all answers a routing `404` — indistinguishable, to the client, from the `404` that means the job itself is gone. Present does NOT mean this job has a log, and it is deliberately not a signal about one: a surface that captures logs offers the link on every job, including those it will answer `204` for and those whose log it withholds. Read the log, not the link.' Progress: type: object description: Server-computed progress snapshot (node-count and sampler-step weighted). Complete per snapshot — one fully re-syncs a client. @@ -929,6 +1085,7 @@ components: properties: node_id: type: string + description: The workflow node that reported this file; empty when the worker named none. example: '9' name: type: string diff --git a/src/comfy_low/__init__.py b/src/comfy_low/__init__.py index 75d65d8..1f73dc9 100644 --- a/src/comfy_low/__init__.py +++ b/src/comfy_low/__init__.py @@ -46,6 +46,7 @@ "postJobs", "getJob", "getJobWorkflow", + "getJobLogs", "getJobEvents", "cancelJob", } @@ -62,6 +63,7 @@ "postJobs": "post_jobs", "getJob": "get_job", "getJobWorkflow": "get_job_workflow", + "getJobLogs": "get_job_logs", "getJobEvents": "get_job_events", "cancelJob": "cancel_job", } diff --git a/src/comfy_low/models/__init__.py b/src/comfy_low/models/__init__.py index e8b3b86..e10c15c 100644 --- a/src/comfy_low/models/__init__.py +++ b/src/comfy_low/models/__init__.py @@ -20,6 +20,7 @@ Format, Job, JobError, + JobLogs, JobStatus, JobUrls, JobWorkflowResponse, @@ -39,6 +40,7 @@ "Format", "Job", "JobError", + "JobLogs", "JobStatus", "JobUrls", "JobWorkflowResponse", diff --git a/src/comfy_low/models/_generated.py b/src/comfy_low/models/_generated.py index c188060..b1d6e68 100644 --- a/src/comfy_low/models/_generated.py +++ b/src/comfy_low/models/_generated.py @@ -49,6 +49,30 @@ class Asset(BaseModel): ] = None +class JobLogs(BaseModel): + """ + A job's captured execution log — the body of `GET /api/v2/jobs/{id}/logs`. Diagnostics, not a contract on content: this is whatever the workflow's own code and nodes wrote to standard output, in the order they wrote it, so nothing about its shape is stable between runs or between releases of a build. It is **untrusted text** — a workflow chooses what goes in it — and must be rendered as plain text rather than interpreted. + """ + + text: Annotated[str, Field(description='The captured output.')] + truncated: Annotated[ + bool, + Field( + description='The BEGINNING of the captured output was discarded — `text` is the TAIL of a longer run. Implementations bound what they capture and store, so a workflow that prints megabytes keeps its last lines, where a failure normally is, instead of being dropped whole. True with an empty `text` means the log was captured and then shed entirely to fit. This describes the stored log, never the response: it does not mean a caller asked for part of one.' + ), + ] + captured_at: Annotated[ + AwareDatetime, + Field(description="When the run's output was read back off the worker."), + ] + complete: Annotated[ + bool, + Field( + description='No further output will be appended to this log. Always `true` today, because a log is read back off the worker once, when the run ends, so a log that exists is already whole. Sent so that a surface which later captures output while a run is still going can say so, and a client written now against `false` keeps working when it does. `false` does not promise that more output will arrive, only that this snapshot may not be the last one.' + ), + ] + + class Format(Enum): """ Discriminates the `workflow` field's shape. `save`: the original authoring workflow JSON, at the version pinned to the job. `api`: the executed API-format prompt graph. @@ -100,6 +124,12 @@ class JobUrls(BaseModel): self: str events: str cancel: str + logs: Annotated[ + str | None, + Field( + description='Where to read what this run printed. Present on any surface that captures execution logs, which is why it is the one link here that is optional: absent means this surface captures none, for any job, so a client can stop looking without spending a request on an answer it already has.\nFollow this link rather than building the path from the job id. The two are not interchangeable: a surface may be mounted under a prefix this link already carries and a hand-built path would not, and a surface that does not implement the operation at all answers a routing `404` — indistinguishable, to the client, from the `404` that means the job itself is gone. Present does NOT mean this job has a log, and it is deliberately not a signal about one: a surface that captures logs offers the link on every job, including those it will answer `204` for and those whose log it withholds. Read the log, not the link.' + ), + ] = None class Progress(BaseModel): @@ -251,7 +281,13 @@ class Output(BaseModel): A committed job output. Outputs are assets: `id` is the asset UUID, retrievable via GET /api/v2/assets/{id} for as long as the job is retained. `hash` is lazily computed and may be null on the retrieval hot path. """ - node_id: Annotated[str, Field(examples=['9'])] + node_id: Annotated[ + str, + Field( + description='The workflow node that reported this file; empty when the worker named none.', + examples=['9'], + ), + ] name: Annotated[str, Field(examples=['ComfyUI_00001_.png'])] type: OutputType content_type: Annotated[str, Field(examples=['image/png'])] diff --git a/src/comfy_low/transport.py b/src/comfy_low/transport.py index 8b564d7..43ad7c5 100644 --- a/src/comfy_low/transport.py +++ b/src/comfy_low/transport.py @@ -45,7 +45,7 @@ from . import _multipart from .errors import ApiError, clean_request_id, error_from_envelope -from .models import Asset, Job, JobWorkflowResponse +from .models import Asset, Job, JobLogs, JobWorkflowResponse from .sse import RawEvent, SSEDecoder _API = "/api/v2" @@ -757,6 +757,19 @@ def get_job_workflow(self, job_id_or_url: str, *, timeout: Any = _UNSET) -> JobW resp = self.raw_request("GET", path, timeout=timeout) return JobWorkflowResponse.model_validate(self._p.parse_or_raise(resp, (200,))) + def get_job_logs(self, job_id_or_url: str, *, timeout: Any = _UNSET) -> JobLogs | None: + """GET /api/v2/jobs/{id}/logs — what the run printed, or None on 204. + + 204 is the contract's normal answer for a job with no log, so it is a + value here rather than an error; every other status still raises. + """ + path = job_id_or_url if _looks_like_path(job_id_or_url) else f"/jobs/{job_id_or_url}/logs" + resp = self.raw_request("GET", path, timeout=timeout) + payload = self._p.parse_or_raise(resp, (200, 204)) + if resp.status_code == 204: + return None + return JobLogs.model_validate(payload) + # -- models ----------------------------------------------------------- def post_model_run( self, @@ -1094,6 +1107,15 @@ async def get_job_workflow( resp = await self.raw_request("GET", path, timeout=timeout) return JobWorkflowResponse.model_validate(self._p.parse_or_raise(resp, (200,))) + async def get_job_logs(self, job_id_or_url: str, *, timeout: Any = _UNSET) -> JobLogs | None: + """Async :meth:`ComfyLow.get_job_logs`.""" + path = job_id_or_url if _looks_like_path(job_id_or_url) else f"/jobs/{job_id_or_url}/logs" + resp = await self.raw_request("GET", path, timeout=timeout) + payload = self._p.parse_or_raise(resp, (200, 204)) + if resp.status_code == 204: + return None + return JobLogs.model_validate(payload) + # -- models ----------------------------------------------------------- async def post_model_run( self, diff --git a/src/comfy_sdk/__init__.py b/src/comfy_sdk/__init__.py index b2ea25d..d8c7bab 100644 --- a/src/comfy_sdk/__init__.py +++ b/src/comfy_sdk/__init__.py @@ -65,7 +65,7 @@ Unauthorized, WorkflowFormatUi, ) -from .jobs import AsyncJob, Job, JobWorkflow +from .jobs import AsyncJob, Job, JobLogs, JobWorkflow from .outputs import AsyncOutput, DownloadUrl, Output from .retry import DEFAULT_RETRY, NO_RETRY, RetryPolicy from .workflows import Workflow, WorkflowFactory @@ -96,6 +96,7 @@ "WorkflowFactory", "Job", "AsyncJob", + "JobLogs", "JobWorkflow", "Output", "AsyncOutput", diff --git a/src/comfy_sdk/jobs.py b/src/comfy_sdk/jobs.py index fb61cb1..c3ab629 100644 --- a/src/comfy_sdk/jobs.py +++ b/src/comfy_sdk/jobs.py @@ -13,6 +13,7 @@ import time from collections.abc import AsyncIterator, Iterator from dataclasses import dataclass +from datetime import datetime from typing import Any, Literal import httpx @@ -48,6 +49,29 @@ class JobWorkflow: format: Literal["save", "api"] +@dataclass(frozen=True) +class JobLogs: + """What a run printed — see :meth:`Job.get_logs`. + + Untrusted text: a workflow chooses what goes in it, so render it as plain + text rather than interpreting it. + + ``truncated`` says the BEGINNING was discarded and this is the tail of a + longer run — where a failure normally is. It describes the stored log, not + the response, so it never means a caller asked for part of one; a true + ``truncated`` with an empty ``text`` means the log was captured and then + shed entirely to fit. + + ``complete`` says no more output will be appended. Always true today, + because a log is read back off the worker once, when the run ends. + """ + + text: str + truncated: bool + captured_at: datetime + complete: bool + + class Job: """Synchronous job handle.""" @@ -140,6 +164,58 @@ def get_workflow(self) -> JobWorkflow: data = self._low.get_job_workflow(self._model.id) return JobWorkflow(graph=data.workflow, format=data.format.value) + def get_logs(self) -> JobLogs | None: + """Fetch what this run printed, or None if there is no log to fetch. + + Fetched on demand and never cached: nothing is downloaded until you + call this, and a second call re-reads rather than replaying the first, + so an early ``None`` on a job that had not finished cannot mask the log + it went on to produce. + + ``None`` is every reason there is nothing to read, which the contract + deliberately does not distinguish: this deployment does not capture + logs at all (Comfy Cloud and self-hosted never do), the job has not + finished, it predates log capture, the run was killed before the worker + could report an outcome (an out-of-memory kill, a crashed worker, a + timeout, a job past its maximum runtime), capture was attempted and + failed, or the job ran on the public demo deployment, which captures a + log but withholds it from anonymous callers. Do not branch on which — + but a job that has not finished may have a log once it has, so a caller + that wants one reads again after a terminal status. + + The killed-run case is a known gap rather than an oversight: a log is + read back off the worker, so a run the platform killed never produced + one. The failures a caller most wants a log for are the ones least + likely to have left one. + + On a surface that offers a logs link, a missing job raises the SDK's + normal :class:`~comfy_sdk.exceptions.NotFound`, as with any other read. + Where there is no link there is no request, so a job that has expired + or never existed returns ``None`` too — the answer "this deployment has + no logs" is reached without asking about the job. + """ + url = self._model.urls.logs + if not url: + # No link means the surface serves no logs for any job, so the + # answer is known without a request. Never synthesized: the URL is + # the surface's to give, and building one here would turn a + # deployment that cannot answer into a 404 that looks like a + # missing job. Falsy, not `is None`: a server that serializes an + # absent optional link as "" rather than omitting it would + # otherwise reach the transport and have `/jobs//logs` built for + # it, which is that same 404. + return None + with translating(): + data = self._low.get_job_logs(url) + if data is None: + return None + return JobLogs( + text=data.text, + truncated=data.truncated, + captured_at=data.captured_at, + complete=data.complete, + ) + # -- live events (best-effort, reconnecting) -------------------------- def events(self) -> Iterator[Event]: """Typed live event iterator. Auto-reconnects with no replay; falls back @@ -255,6 +331,22 @@ async def get_workflow(self) -> JobWorkflow: data = await self._low.get_job_workflow(self._model.id) return JobWorkflow(graph=data.workflow, format=data.format.value) + async def get_logs(self) -> JobLogs | None: + """Async :meth:`Job.get_logs`.""" + url = self._model.urls.logs + if not url: + return None + with translating(): + data = await self._low.get_job_logs(url) + if data is None: + return None + return JobLogs( + text=data.text, + truncated=data.truncated, + captured_at=data.captured_at, + complete=data.complete, + ) + async def events(self) -> AsyncIterator[Event]: """Async :meth:`Job.events` — typed live stream, auto-reconnecting with no replay and the poll path as its backstop. diff --git a/tests/conftest.py b/tests/conftest.py index 799351a..9173ac2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -90,6 +90,23 @@ class ServerState: ) job_workflow_format: str = "api" job_workflow_not_found: bool = False + # GET /jobs/{id}/logs response body; None answers 204, the contract's normal + # "this job has no log". `job_logs_not_found=True` answers 404 instead, for + # the missing-job path. `omit_logs_link=True` drops `urls.logs` from every + # job, standing in for a surface that serves no logs at all. + job_logs: dict[str, Any] | None = None + job_logs_not_found: bool = False + omit_logs_link: bool = False + # Serializes `urls.logs` as "" instead of omitting it, the shape a server + # that forgot an omit-empty tag would emit. + empty_logs_link: bool = False + # Serves `urls.logs` at a path a synthesized `/jobs/{id}/logs` would never + # produce, so a test can prove the SDK followed the link rather than built + # the path — at the suite's root mount the two otherwise coincide. + logs_link_path: str | None = None + job_logs_request_count: int = 0 + # Path of the most recent logs request, so a test can assert WHICH url was used. + last_job_logs_path: str | None = None # --- POST /v2/models/{provider}/{model} (the awaited model run) --- # The provider's native payload the run resolves to. Deliberately not a @@ -235,7 +252,15 @@ def _asset_json(asset_id: str, hash_: str, created_new: bool, size: int) -> dict } -def _job_json(job_id: str, status: str, outputs: list[dict] | None = None) -> dict: +def _job_json( + job_id: str, + status: str, + outputs: list[dict] | None = None, + *, + omit_logs_link: bool = False, + empty_logs_link: bool = False, + logs_link_path: str | None = None, +) -> dict: return { "id": job_id, "status": status, @@ -252,6 +277,15 @@ def _job_json(job_id: str, status: str, outputs: list[dict] | None = None) -> di "self": f"/api/v2/jobs/{job_id}", "events": f"/api/v2/jobs/{job_id}/events", "cancel": f"/api/v2/jobs/{job_id}/cancel", + **( + {} + if omit_logs_link + else { + "logs": "" + if empty_logs_link + else (logs_link_path or f"/api/v2/jobs/{job_id}/logs") + } + ), }, } @@ -379,6 +413,11 @@ def do_GET(self) -> None: if m: self._serve_events(m.group(1)) return + m = re.match(r"/api/v2/jobs/([^/]+)/logs$", self.path) + if m: + state.last_job_logs_path = self.path + self._serve_job_logs() + return m = re.match(r"/api/v2/jobs/([^/]+)/workflow$", self.path) if m: self._serve_job_workflow(m.group(1)) @@ -432,7 +471,28 @@ def _serve_job(self, job_id: str) -> None: else: status = "running" outputs = [] - self._json(200, _job_json(job_id, status, outputs)) + self._json( + 200, + _job_json( + job_id, + status, + outputs, + omit_logs_link=state.omit_logs_link, + empty_logs_link=state.empty_logs_link, + logs_link_path=state.logs_link_path, + ), + ) + + def _serve_job_logs(self) -> None: + state.job_logs_request_count += 1 + if state.job_logs_not_found: + self._err(404, "job_not_found", "no such job") + return + if state.job_logs is None: + self.send_response(204) + self.end_headers() + return + self._json(200, state.job_logs) def _serve_job_workflow(self, job_id: str) -> None: if state.job_workflow_not_found: @@ -505,7 +565,16 @@ def do_POST(self) -> None: return m = re.match(r"/api/v2/jobs/([^/]+)/cancel$", self.path) if m: - self._json(200, _job_json(m.group(1), "canceling")) + self._json( + 200, + _job_json( + m.group(1), + "canceling", + omit_logs_link=state.omit_logs_link, + empty_logs_link=state.empty_logs_link, + logs_link_path=state.logs_link_path, + ), + ) return self._read_body() self._err(404, "not_found") @@ -708,7 +777,16 @@ def _post_jobs(self) -> None: job_id = f"job_{state.submit_count:02d}" if key: state.idempotency[key] = job_id - self._json(201, _job_json(job_id, "queued")) + self._json( + 201, + _job_json( + job_id, + "queued", + omit_logs_link=state.omit_logs_link, + empty_logs_link=state.empty_logs_link, + logs_link_path=state.logs_link_path, + ), + ) return Handler diff --git a/tests/test_follow_up_links.py b/tests/test_follow_up_links.py index 76fd09a..59da5be 100644 --- a/tests/test_follow_up_links.py +++ b/tests/test_follow_up_links.py @@ -52,3 +52,15 @@ def test_auth_still_attaches_to_origin_resolved_links() -> None: p = _Prepared(PATH_MOUNTED_BASE, "comfyui-k") url = p.url("/deployment/dep_123/api/v2/jobs/j1") assert p.headers(url)["Authorization"] == "Bearer comfyui-k" + + +def test_gateway_logs_link_resolves_against_origin() -> None: + # The reason get_logs() follows urls.logs instead of building + # /jobs/{id}/logs: on a path-mounted proxy the server's link already carries + # the mount prefix, and a synthesized path would resolve under base_url and + # double it. Pinned here because the stub server in the suite is mounted at + # the root, where the two forms coincide and a regression would be silent. + p = _Prepared(PATH_MOUNTED_BASE, "comfyui-k") + link = "/deployment/dep_123/api/v2/jobs/j1/logs" + assert p.url(link) == "https://proxy.example" + link + assert p.url("/jobs/j1/logs") == PATH_MOUNTED_BASE + "/api/v2/jobs/j1/logs" diff --git a/tests/test_logs.py b/tests/test_logs.py new file mode 100644 index 0000000..d8fea13 --- /dev/null +++ b/tests/test_logs.py @@ -0,0 +1,169 @@ +"""On-demand execution logs: what a run printed, fetched only when asked. + +The log is a resource of its own rather than a field on the job, so these +cover the two things that follow from that: a job read never carries it, and +"no log" is an ordinary answer rather than an error. +""" + +from __future__ import annotations + +import pytest + +from comfy_sdk import AsyncComfy, Comfy, NotFound + +_CAPTURED = { + "text": "got prompt\nPrompt executed in 4.62 seconds\n", + "truncated": False, + "captured_at": "2026-07-10T18:25:00Z", + "complete": True, +} + + +def _wf(client): + return client.workflows.from_json({"3": {"class_type": "KSampler", "inputs": {}}}) + + +def test_get_logs_returns_what_the_run_printed(server) -> None: + server.state.job_logs = _CAPTURED + with Comfy() as client: + job = client.run(_wf(client)) + logs = job.get_logs() + + assert logs is not None + assert logs.text == "got prompt\nPrompt executed in 4.62 seconds\n" + # Read off the wire, not hardcoded: this is the whole log, so `truncated` + # is false here and true in the shed-entirely case below. + assert logs.truncated is False + assert logs.complete is True + assert logs.captured_at.year == 2026 + + +def test_running_a_job_never_fetches_the_log(server) -> None: + # The whole point of the resource: submitting and polling to terminal must + # not pay for a log the caller did not ask for. + server.state.job_logs = _CAPTURED + with Comfy() as client: + client.run(_wf(client)) + + assert server.state.job_logs_request_count == 0 + + +def test_get_logs_is_none_when_the_job_has_no_log(server) -> None: + server.state.job_logs = None + with Comfy() as client: + job = client.run(_wf(client)) + assert job.get_logs() is None + assert server.state.job_logs_request_count == 1 + + +def test_get_logs_is_none_without_a_link_and_makes_no_request(server) -> None: + # A surface that serves no logs at all (Comfy Cloud, self-hosted) omits + # urls.logs. The answer is known from its absence, so the SDK must not + # construct the URL and ask anyway. + server.state.omit_logs_link = True + server.state.job_logs = _CAPTURED + with Comfy() as client: + job = client.run(_wf(client)) + assert job.get_logs() is None + assert server.state.job_logs_request_count == 0 + + +def test_get_logs_follows_the_link_rather_than_building_the_path(server) -> None: + # The invariant the whole design rests on. The stub is mounted at the root, + # so a synthesized `/jobs/{id}/logs` and the served link normally resolve to + # the same URL and a regression would be silent; serving the link at a path + # no synthesis could produce is what makes the difference observable. A + # path-mounted proxy is the real case this protects. + server.state.logs_link_path = "/api/v2/jobs/link-only-token/logs" + server.state.job_logs = _CAPTURED + with Comfy() as client: + job = client.run(_wf(client)) + assert job.get_logs() is not None + assert server.state.last_job_logs_path == "/api/v2/jobs/link-only-token/logs" + + +def test_get_logs_treats_an_empty_link_as_no_link(server) -> None: + # A server that serializes the absent optional link as "" rather than + # omitting the key must not send the SDK to `/jobs//logs`, which would + # surface as a NotFound for a job that exists. + server.state.empty_logs_link = True + server.state.job_logs = _CAPTURED + with Comfy() as client: + job = client.run(_wf(client)) + assert job.get_logs() is None + assert server.state.job_logs_request_count == 0 + + +def test_get_logs_refetches_every_call(server) -> None: + # Deliberately uncached: an early None on a job that had not finished must + # not mask the log it goes on to produce. + server.state.job_logs = None + with Comfy() as client: + job = client.run(_wf(client)) + assert job.get_logs() is None + + server.state.job_logs = _CAPTURED + logs = job.get_logs() + + assert logs is not None + assert logs.text.startswith("got prompt") + assert server.state.job_logs_request_count == 2 + + +def test_get_logs_raises_the_usual_error_for_a_missing_job(server) -> None: + server.state.job_logs_not_found = True + with Comfy() as client: + job = client.run(_wf(client)) + with pytest.raises(NotFound): + job.get_logs() + + +def test_a_captured_empty_log_is_not_absence(server) -> None: + # truncated with an empty text is how a log shed entirely to fit says so, + # which is a different answer from never having had one. + server.state.job_logs = { + "text": "", + "truncated": True, + "captured_at": "2026-07-10T18:25:00Z", + "complete": True, + } + with Comfy() as client: + job = client.run(_wf(client)) + logs = job.get_logs() + + assert logs is not None + assert logs.text == "" + assert logs.truncated is True + + +async def test_async_get_logs_mirrors_the_sync_surface(server) -> None: + server.state.job_logs = _CAPTURED + async with AsyncComfy() as client: + wf = client.workflows.from_json({"3": {"class_type": "KSampler", "inputs": {}}}) + job = await client.run(wf) + logs = await job.get_logs() + + assert logs is not None + assert logs.text == "got prompt\nPrompt executed in 4.62 seconds\n" + assert logs.complete is True + + +async def test_async_get_logs_is_none_without_a_link(server) -> None: + server.state.omit_logs_link = True + async with AsyncComfy() as client: + wf = client.workflows.from_json({"3": {"class_type": "KSampler", "inputs": {}}}) + job = await client.run(wf) + assert await job.get_logs() is None + assert server.state.job_logs_request_count == 0 + + +async def test_async_get_logs_is_none_when_the_server_answers_204(server) -> None: + # The async half is hand-duplicated and the parity test compares names, + # not behaviour, so the 204 branch needs its own async exercise or it is + # only ever run on the sync side. + server.state.job_logs = None + async with AsyncComfy() as client: + wf = client.workflows.from_json({"3": {"class_type": "KSampler", "inputs": {}}}) + job = await client.run(wf) + assert await job.get_logs() is None + assert server.state.job_logs_request_count == 1 diff --git a/tests/test_transport_security.py b/tests/test_transport_security.py index 24983d5..8460eb5 100644 --- a/tests/test_transport_security.py +++ b/tests/test_transport_security.py @@ -1,9 +1,12 @@ """The bearer token must never leak to a host other than the configured ``base_url``. -``job.urls.self`` / ``job.urls.events`` / ``job.urls.cancel`` are server-returned -absolute URLs that ``Job.refresh()`` / ``events()`` / ``cancel()`` hand straight -to the transport (see ``comfy_sdk/jobs.py``). Before this fix, ``_Prepared`` +``job.urls.self`` / ``job.urls.events`` / ``job.urls.cancel`` / ``job.urls.logs`` +are server-returned absolute URLs that ``Job.refresh()`` / ``events()`` / +``cancel()`` / ``get_logs()`` hand straight to the transport (see +``comfy_sdk/jobs.py``). The guard is generic — it keys on the resolved origin in +``_Prepared.headers``, not on which link produced the URL — so a new follow-up +link is covered the moment it is added. Before this fix, ``_Prepared`` attached ``Authorization: Bearer `` to *any* absolute URL with no origin check — a malicious or misconfigured server could point a job's follow-up link at an attacker-controlled host and have the client hand it the credential.