feat(reflex-hosting-cli): upload deploy archives straight to storage - #6938
Conversation
Greptile SummaryThe PR changes managed deployments to reserve storage destinations, upload the frontend and backend archives concurrently, and submit the resulting deployment identifier while retaining the multipart fallback.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-hosting-cli/src/reflex_cli/utils/hosting.py | Implements the reserve-upload-submit workflow, concurrent archive transfers, one re-reservation attempt, multipart fallback, and shared presigned PUT handling; no eligible follow-up finding was established. |
| packages/reflex-hosting-cli/src/reflex_cli/utils/console.py | Adds a JSON-aware Rich progress display configured for byte transfers. |
| packages/reflex-hosting-cli/src/reflex_cli/utils/exceptions.py | Adds the dedicated user-facing archive upload exception. |
| tests/units/reflex_cli/utils/test_hosting.py | Covers direct uploads, exact headers and bytes, re-reservation, retry exhaustion, relay fallbacks, control-plane failures, and sibling-upload abandonment. |
Reviews (4): Last reviewed commit: "refactor(reflex-hosting-cli): one presig..." | Re-trigger Greptile
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Merging this PR will not alter performance
Comparing Footnotes
|
db180d9 to
90ddc2d
Compare
`reflex deploy` posted both zips as multipart to `POST /deployments`, where the web process relayed them into a blocking boto3 upload inside the request handler. The builder path stopped doing that in ENG-9725; the CLI path did not. The control-plane half is now in place, so this takes it. The CLI reserves, PUTs both archives to the signed keys it gets back, and submits only the id. Content-Length is set explicitly on each PUT: the exact byte count is part of the signature, and an unsized body would go up chunked and fail to match it. A 404 from `/deployments/reserve` is the route being absent rather than the app -- nothing the endpoint itself refuses with is a 404, since an unknown app and an out-of-range size are 400 and a missing permission is 403 -- so it falls back to relaying, as does a caller with no app id to reserve against. A 403 on a PUT is a lapsed signature. Recovery is a new reservation, and that mints a new deployment id naming a new prefix, so both archives go up again under it: the one that already succeeded is not somewhere the new deployment will look. Two windows, then the link is the problem and the user is told so. The two uploads run concurrently and report bytes, since a silent multi-minute PUT is worse than the relay it replaces, which at least streamed. Neither is worth finishing alone, though -- the build submits under one id -- so the first failure abandons its sibling at the next chunk boundary instead of leaving the user watching a large upload complete into a deployment that cannot happen. `transfer_progress` sits in this package rather than beside `progress` in reflex-base: a new name over there would raise this package's reflex-base floor, and the CLI is released on its own schedule. It needs nothing reflex-base does not already owe the CLI -- `is_json_mode` is what the re-exported `progress` already calls to stay quiet under JSON output. The multipart path is unchanged and stays for the overlap. ENG-11230
90ddc2d to
51e55ce
Compare
The mid-stream test slept 50ms per chunk and hoped the backend's failure landed first. Scheduling decided whether it did. Split into the two claims that can each be proven outright: the generator stops at the next chunk boundary once the flag is set (no threads), and an abandoned sibling never displaces the failure that caused it (the stream raises directly rather than being raced into).
The security review flow and the deploy upload each signed their own PUT, and only one of them documented Content-Length correctly. The scan path's comment said setting it manually breaks the signature; what breaks the signature is omitting it on a streamed body, which then goes up chunked. Verified against httpx 0.28.1: for a bytes body an explicit Content-Length is byte-identical to the derived one, so there was never a reason for the two to disagree. `presigned_put` holds the invariant once. The scan path picks up the per-operation timeouts with it, in place of a flat 120s covering connect and pool as well. Retry, progress and abandonment stay with the deploy path -- they follow from having two large archives, not from the upload being presigned.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
#6938 landed the presigned archive upload, which touches the same console module this branch restructures. Conflict resolution: - console.py: kept this branch's guarded import block and carried transfer_progress() forward. It gated its progress bar on reflex_base.utils.log.is_json_mode() directly, which no longer exists as a hard import, so it goes through reflex_cli.utils.log.is_json_mode() instead -- forwarded to reflex-base when installed, False otherwise, since JSON output is a reflex-base pipeline feature with nothing to stay quiet for without it. - hosting.py: the upload retry path called console.warn(), one of the legacy helpers this branch stopped re-exporting (deprecated in reflex-base by #6867). Now logger.warning(), matching every other message in the file. - transfer_progress's docstring justified living in this package by not wanting to raise the reflex-base floor. There is no floor now, so it says what is actually true: the CLI has to render it with or without reflex-base. Verified the merged upload path renders on real reflex 0.8.9 with no reflex-base installed, and added test_fallback_progress_bars to keep both progress bars covered on that path.
Closes ENG-11230. Client half of the presigned upload work; the control-plane half is flexgen#5113.
reflex deployposted both zips as multipart toPOST /deployments, where the web process relayed them into a blocking boto3upload_fileobjinside the request handler. The builder path stopped doing that in ENG-9725; the CLI path did not.It now calls
POST /deployments/reserve, PUTs both archives to the signed keys it gets back, and submits only the returned deployment id.Touches
packages/reflex-hosting-clionly — no reflex-base change, so this does not move the package's reflex-base floor.Decisions worth a second look
Content-Lengthis set explicitly on each PUT. The exact byte count is part of the signature, and httpx would otherwise send a generator body chunked, which the signature does not match.Request._prepareskipsTransfer-Encodingprecisely whenContent-Lengthis already set. Verified over a real socket rather than assumed — sized, not chunked, exact byte count.A 404 from
/deployments/reservemeans the route is absent, not the app. Nothing the endpoint itself refuses with is a 404: an unknown app and an out-of-range size are 400, a missing permission is 403. So a 404 falls back to relaying, as does a caller with no app id to reserve against. The multipart path is otherwise unchanged and stays for the overlap.A 403 on a PUT is a lapsed signature, and re-reserving is the recovery. A fresh reservation mints a fresh deployment id naming a fresh prefix, so both archives go up again under it — the one that already succeeded is not somewhere the new deployment will look. Budget is two windows; past that the link is the problem and the user is told so rather than looped.
The two uploads run concurrently, with a byte-oriented progress bar each. A silent multi-minute PUT is worse than the relay it replaces, which at least streamed. But neither archive is worth finishing alone — the build submits under one id — so the first failure abandons its sibling at the next chunk boundary rather than leaving the user watching a large upload complete into a deployment that cannot happen.
Both presigned upload paths now share one PUT.
submit_security_reviewsigned its own, and its comment claimed settingContent-Lengthmanually breaks the signature — the opposite of what this path needs. Verified against httpx 0.28.1: for a bytes body an explicit length is byte-identical to the derived one; for a stream, omitting it yields a chunked body the signature rejects.presigned_putholds that invariant once and both callers use it, so the scan path also picks up the per-operation timeouts in place of a flat 120s covering connect and pool. Retry, progress and abandonment deliberately stay with the deploy path — they follow from having two large archives, not from the upload being presigned.transfer_progresslives in this package, not besideprogressin reflex-base. Only the deploy upload wants it, and a new name in reflex-base would raise this package's reflex-base floor for no benefit — the CLI releases on its own schedule. It uses nothing reflex-base does not already owe the CLI:is_json_modeis what the re-exportedprogressalready calls to stay quiet under JSON output.Known tradeoffs
ArchiveSizeErrortext, which is accurate but worded differently.Testing
10 new tests in
tests/units/reflex_cli/utils/test_hosting.pycovering the reserve request shape, exact-bytes/exact-headers on both PUTs, expiry recovery re-uploading both archives under the new id, the retry budget, both relay fallbacks, a refused reservation, an unreachable control plane, and mid-stream abandonment. The two existingcreate_deploymenttests moved onto real archives since they now traverse the upload path.Full unit suite green (7656 passed, 17 skipped);
ruff check,ruff format --check,pyright, andscripts/check_min_deps.pyclean.Not included, per the ticket: moving
SUPPORTED_CLI_THRESHOLDserver-side, which waits until enough of the fleet has upgraded.