Skip to content

feat(deploy): add the asset and job data-plane clients - #804

Open
guill wants to merge 3 commits into
stack/06-deploy-control-planefrom
stack/07-deploy-data-plane
Open

feat(deploy): add the asset and job data-plane clients#804
guill wants to merge 3 commits into
stack/06-deploy-control-planefrom
stack/07-deploy-data-plane

Conversation

@guill

@guill guill commented Aug 27, 2026

Copy link
Copy Markdown
Member
Stack of 7 — review bottom-up (this is 804)
  1. feat(build): add packaging, sync and Builder client primitives #801feat(build): packaging, sync and Builder client primitives — base main
  2. feat(build)!: replace the legacy build and distribution surface #802feat(build)!: replace the legacy build and distribution surface (breaking)
  3. feat(deploy): add the deployment control plane #803feat(deploy): the deployment control plane
  4. feat(deploy): add the asset and job data-plane clients #804feat(deploy): the asset and job data-plane clients   ← this PR
  5. feat(deploy): add comfy deploy run #805feat(deploy): comfy deploy run
  6. fix(build): a pull no longer deletes definition data silently #810fix(build): a pull no longer deletes definition data silently
  7. fix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10 #824fix(cli): unbreak the interactive pickers and Go timestamp parsing on 3.10

Each PR is based on the branch below it, so its own diff is only its commit. Merge in order.

Rebased onto main @ c1fa1f4. The original bottom PR (#800, raising the Python floor to 3.11) was closed — see that thread for why; the stack is 3.10-compatible instead.

TL;DR: The clients a workflow submission needs — asset dedupe/upload, job submit/poll/cancel, the event stream, and output download — with no command wired to them yet.

Stacked on #803. Review from the bottom up; this PR's diff is the 15 files below.

What changed

  • hashing.py (new): blake3 content hashing. New blake3 dependency, with the lockfiles bumped in the same change so no layer imports an undeclared package.
  • deploy_assets.py (new): dedupe local assets by digest, upload what is missing.
  • deploy_jobs.py (new): submit a job, poll it, cancel it.
  • deploy_events.py (new): the job event stream.
  • deploy_download.py (new): fetch completed outputs.
  • transfer.py: _stream_http_one takes an injectable opener so the download path can reuse the shared streaming logic under its own HTTP-only opener.

Error registry: registers the 9 codes these clients raise. deploy_endpoint_unknown moves here from the control-plane group — its only call site is deploy_download — while deploy_job_canceled and deploy_workflow_format_ui stay out, because only deploy run and the workflow reader raise them.

A 422 missing_asset maps to deploy_asset_missing rather than falling through to the generic 422 row. The API lists it beside invalid_workflow, and without its own entry a run whose workflow is perfectly valid — but which names an asset id this account cannot mint — was reported as an invalid workflow, sending the reader off to edit a graph that has nothing wrong with it. deploy_asset_missing's registry text widens to name both of its causes.

None of these modules emits a command= envelope label, so none depends on a Typer verb that does not exist yet.

Python 3.10 compatibility: deploy_events.py's private _WatchState uses (str, Enum) — the existing house pattern — rather than the 3.11-only enum.StrEnum. It is compared only by identity in the match statement and never stringified, so the str() difference between the two does not apply. assert_never comes from typing_extensions here and in test_deploy_assets.py. The declared floor stays >=3.10.

Validation

  • CI (build job, Python 3.10) at this commit: 6731 passed, 38 skipped, 0 failed.
  • ruff check . / ruff format --diff . clean at CI's pin (0.15.15).

Contradicts: nothing.

@dosubot dosubot Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Aug 27, 2026
@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d85985b2-87ea-4251-86d4-50f91999852d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

@dosubot dosubot Bot added the enhancement New feature or request label Aug 27, 2026
@socket-security

socket-security Bot commented Aug 27, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedblake3@​1.0.9100100100100100

View full report

Comment thread tests/comfy_cli/command/test_deploy_download.py Fixed
@guill
guill force-pushed the stack/07-deploy-data-plane branch 2 times, most recently from d02f65b to cdee048 Compare August 27, 2026 19:52
@guill
guill force-pushed the stack/07-deploy-data-plane branch from cdee048 to 34dd70f Compare August 27, 2026 20:34
The clients a workflow submission needs, with no command wired to them yet:

- hashing: blake3 content hashing (new blake3 dependency, with the lockfiles
  bumped in the same change so no layer imports an undeclared package).
- deploy_assets: dedupe local assets by digest, upload what is missing.
- deploy_jobs: submit a job, poll it, cancel it.
- deploy_events: the job event stream.
- deploy_download: fetch completed outputs.

transfer._stream_http_one takes an injectable opener so the download path can
reuse the shared streaming logic under its own HTTP-only opener.

Registers the 9 codes these clients raise. deploy_endpoint_unknown moves here
from the control-plane group in the registry -- its only call site is
deploy_download -- while deploy_job_canceled and deploy_workflow_format_ui
stay out, because only `deploy run` and the workflow reader raise them.

A 422 `missing_asset` maps to deploy_asset_missing rather than falling through
to the generic 422 row. The API lists it beside invalid_workflow, and without
its own entry a run whose workflow is perfectly valid — but which names an
asset id this account cannot mint — was reported as an invalid workflow,
sending the reader to edit a graph that has nothing wrong with it.
deploy_asset_missing's registry text widens to name both of its causes.

None of these modules emits a `command=` envelope label, so none of them
depends on a Typer verb that does not exist yet.

@skishore23 skishore23 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. This is the layer that had to be right, and it is — verified against the platform-gateway checkout (2026-08-25) and by adversarial testing on the branch: digest is blake3:<64 hex> matching the gateway's canonical regex; uploads stream in 1 MiB chunks with Content-Length equal to the streamed bytes, multipart field order the gateway requires, filename quotes/CR/LF neutralised, Bearer only to the endpoint; download origin checks are exact set membership (34 hostile URLs — subdomain suffixes, userinfo, \\@, %2E, punycode, ports, http, file:, private IPv6 — all rejected), the redirect handler refuses non-https/untrusted origins and strips auth on every hop, and the atomic part-file write, 10 GB cap, no-overwrite suffixing and symlink refusal are reused from transfer.py. The CodeQL hit is on the test's in str(...) assertion, not production. SSE parser is bounded (3 MiB lines, no terminator, bad UTF-8, deep nesting, comment floods) and the 30 s idle timeout is sound against the gateway's 15 s pings.

Three non-blocking notes:

  1. Connection-level failures escape the error mappingdeploy_jobs.py:164, deploy_assets.py:200,223,250, deploy_events.py:210,267 catch (TimeoutError, URLError), but urllib only wraps request(); a socket closed before the status line raises http.client.RemoteDisconnected and a garbled one raises BadStatusLine (reproduced on loopback). On submit_job that's the LB-drain case the registry reserves for deploy_job_submit_unknown ("do not resubmit"), but #805's catch-all reports it as deploy_server_error, and BadStatusLine is caught by nothing. Same pattern in #802/#803 — one shared except tuple for http.client.HTTPException/ConnectionError would close all of them.
  2. uv.lock / pylock.toml carry cp311–cp314 blake3 wheels only, no cp310 (PyPI has them), while CI's matrix is ["3.10"] and requires-python >= 3.10. uv sync --locked on 3.10 builds blake3 from the Rust sdist (confirmed with --no-build-package blake3 --dry-run); CI passed on a warm cache. uv lock --upgrade-package blake3 + regenerate pylock.
  3. deploy_jobs._retry_after accepts any non-negative Retry-After and _sleep is uncapped, while deploy_events._retry_after caps at 10 s — a Retry-After: 86400 on a 429 parks the CLI for a day. Dormant today (gateway sends none on job 429s) but worth matching.

Minor: no cancel client lands here (it's in #805) despite the body's "submit/poll/cancel". Tests on the branch: 214 passed, 1 skipped; ruff clean.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Aug 28, 2026
guill added a commit that referenced this pull request Aug 29, 2026
Two defects from the #802 review, and a third of the same shape found
while fixing them.

`--from-workflow` shipped the user's workflow path to analytics verbatim.
Redaction is keyed on the Typer parameter name and `_scrub_value` returns
a bare local path unchanged by design, so a path is redacted by being
named in `_SENSITIVE_EXACT` or not at all. `from_snapshot` was named; its
interchangeable sibling was not.

`update --from-snapshot` discarded the base image the importer resolved.
`merge_definition` carried every stored key outside the scanned set
forward *over* the incoming value, so a snapshot resolving `cuda12-py312`
kept building on the stored `cuda12-py311` -- and the diff reported no
change at all, so nothing said so, while the advisory reported that
image's Python as satisfied. The carry now tests the incoming definition
rather than the key set alone: a local scan states no `baseImage`, so the
stored one still survives a plain `update`, but an importer that resolves
one now wins and the change appears in the diff the user confirms.
`--base-image` is applied after the merge and still outranks both.

`_merge_entry` carried entries the same way, and a snapshot resolves each
pack's `commit`, which no scan reports. The stored SHA outranked the
imported one and the entry then read as unchanged, so an import could
never refresh a pinned commit. Same fix, same property: the scan path is
byte-identical because a scan states only scanner-owned keys.

Not addressed here, all called non-blocking in review: connection-level
failures escaping the error mapping (one shared except tuple spanning
this PR, #803 and #804 -- better as one focused change than three
partial ones); the unguarded `CliRunner(mix_stderr=False)` call sites,
which CI's locked resolve pins away from; and `_unrenderable` pointing at
a `--json` payload that carries only rendered advisories.

The CodeQL chmod alerts on these tests are false positives: every flagged
line is a `finally` restoring owner-only bits, not a world-readable mask.
guill added 2 commits August 29, 2026 19:07
Two non-blocking notes from the #804 review.

`uv.lock` carried cp311-cp314 blake3 wheels and no cp310, while
`requires-python` is >=3.10 and every CI job pins 3.10 -- so
`uv sync --locked` had to build blake3 from the Rust sdist on the only
Python the suite actually runs on. PyPI publishes 13 cp310 wheels for
1.0.9; the lock simply lacked them. Confirmed before and after with
`uv sync --locked --python 3.10 --no-build-package blake3 --dry-run`,
which refused with "has no binary distribution" and now resolves. No
package version moves; the only other edit is uv narrowing
`exceptiongroup`'s `typing-extensions` marker to `python_full_version <
'3.11'`, a no-op given both referents are already gated below 3.11.

`pylock.toml` is regenerated with the command its own header records.
That diff is larger than the blake3 rows because the committed file was
already stale against the committed `uv.lock` -- five versions behind and
missing four packages -- so it moves onto `uv.lock` rather than resolving
anything independently.

`deploy_jobs._retry_after` accepted any non-negative `Retry-After` and
slept it unbounded, so a `Retry-After: 86400` on a 429 would park a
foreground `submit_job` for a day with no output. It is now clamped to the
same ceiling `deploy_events` already applied to its own.

Deferred to known-issues.md as items 15-16: the shared except tuple for
connection-level failures, which spans this PR, #802 and #803 and belongs
in one focused change; and the duplicated clamp literal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants