Repository navigation
feat(cli): direct-to-R2 asset upload via presigned URLs - #192
Merged
Merged
Conversation
Task 3 of the direct-to-R2 asset upload series. The CLI now computes
both BLAKE3 (existing CF-compatible storage key recipe) and MD5 (raw
content digest) for every asset in a single disk read, and emits the v2
manifest entry shape `{ blake3, md5, size, hash }` where `hash` is a
deprecated alias of `blake3` kept for one release.
The deploy manifest now carries `assetManifestVersion: 2` from all three
package builders (packageBuild, packageStaticBuild, packageFrameworkBuild).
Wire transport is unchanged in this PR — the multipart endpoint still
accepts the entries since the v1 `hash`/`size` fields remain present.
Task 4 will flip the CLI to direct-to-R2 + JSON-mode finalization where
the v2 fields become required.
MD5 is the wire-level upload-integrity primitive that R2 enforces via
`Content-MD5` (RFC 1864) on direct PUTs; BLAKE3 stays the canonical
content-addressed storage key. See meta/design-docs/0068.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc18919697
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Task 4 of the direct-to-R2 asset upload series (design doc 0068). Flips the CLI to upload asset bytes directly to R2 via presigned PUT URLs and finalize via the manifest-only JSON deploy endpoint. Removes asset streaming from the API Worker request path so big static deploys stop hitting the CF edge-to-Worker streaming-body timeout. CLI surface: - new client.requestUploadUrls(...) and client.finalizeDeploy(...) on PlatformClient. Both share the same NDJSON-stream-reader as the legacy multipart deploy() so the existing applyDeployEvent machinery works unchanged. - new --legacy-upload flag re-selects the multipart path as a one- release escape hatch; the default flow is now direct-to-R2. - new direct-r2-upload.ts performs the bounded-concurrency PUT pool (10 in flight, 5xx retry up to 3 times with 250/500/1000ms backoff, 4xx surfaced immediately) and tags the JSONL log with per-asset presign_request_*, r2_put_*, and finalize_* events for postmortems. Asset chain double-read fix (Codex Finding #2): - collectAndHashAssets already returned an in-memory buffer cache as part of Task 3; this PR threads it through every uploader. The packageBuild / packageStaticBuild / packageFrameworkBuild functions now accept an optional assetFiles?: Map<string, Buffer> and consume it via the new appendAssetsFromCache helper instead of re-reading from disk. The legacy multipart and the new direct-to-R2 paths both run with each asset read exactly once, end-to-end. - buildVoidManifest, buildStaticManifest, buildFrameworkManifest, and collectWorkerFiles are factored out as pure / single-pass helpers so the direct-to-R2 path can compute the manifest and worker bytes without rebuilding a FormData first. Tests: - test/unit/direct-r2-client.test.ts — verifies requestUploadUrls POSTs JSON to the right endpoint, finalizeDeploy uses application/ json (NOT multipart), and NDJSON events are parsed correctly across chunk boundaries. - test/unit/direct-r2-upload.test.ts — exercises the PUT pool: 4xx surfaces with path+hash, 5xx retries, network errors retry, bounded concurrency, empty-list short-circuit. - test/integration/direct-r2-deploy.test.ts — drives runStaticDeploy end-to-end against mocked platform + R2, verifies the JSONL log carries all the new events, the failure-mode message ends with the JSONL path, and (Codex Finding #2) each asset file is read from disk EXACTLY ONCE in both the direct-to-R2 and legacy multipart paths. Backward compat: - The multipart client.deploy() path stays unchanged; --legacy-upload exercises it. Old CLIs keep working against the platform's multipart endpoint (no platform-side break in this PR). Out of scope (per design doc 0068): - Removing the multipart endpoint (deferred to a later cleanup PR). - Cross-project content-addressed dedupe (still per-project). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
streamDirectR2Deploy held every asset's bytes in `assetFiles` until finalizeDeploy returned, even for entries preflight reported as already in R2. A 142 MB mostly-cached deploy paid full resident memory for nothing. Two-stage release: - Right after preflight, prune `assetFiles` to the `needed` subset so skipped entries become GC-eligible immediately. Pre/post sizes log to the JSONL trace as `asset_cache_pruned` for retrospect. - During upload, `uploadAssetsToR2` now clears `item.body` and invokes an `onAfterPut` callback after each successful PUT. The deploy driver passes `assetFiles.delete(item.path)` so peak memory under the bounded pool tracks `R2_PUT_CONCURRENCY × asset_size`, not the full bundle. Callback is suppressed on PUT failure so callers can still re-presign / retry without losing the buffer prematurely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Brooooooklyn
force-pushed
the
feat/cli-direct-r2-upload
branch
from
May 7, 2026 12:49
cc18919 to
f70264b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f70264b368
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f70264b. Configure here.
Two more sites in direct-r2-deploy.test.ts at lines 348 and 386 still passed `null` for the 6th positional arg to runStaticDeploy, which post-rebase is `gitMetadata?: GitDeployMetadata` (was `commit: string | null`). The other 9 sites were updated in the rebase commit; these two were missed because they sit inside helper functions whose calls weren't in the same edit batch. Pure type-only fix — no behavior change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Previously `streamDirectR2Deploy` iterated `needed` (paths) when building both `assetsToUpload` (presign request) and `items` (R2 PUT pool), so identical files (same `blake3`) produced one presign request and one PUT each. The platform's `upload-urls` handler tolerates duplicate blake3 by last-write-wins on the response map (which then collapses to one entry), but the CLI still re-fanned-out N PUTs to the same R2 key — wasted bandwidth, larger retry surface, and amplified load on bundles with prerendered/duplicated content. Now we walk `needed` once, collect the unique-by-blake3 path list, and both arrays iterate that. The `assetManifest` payload sent to `finalizeDeploy` is unchanged — every path stays so the platform builds the per-path serving manifest. The `presign_request_start` log gains a `dedupedFrom: needed.length` field for observability. `onAfterPut` still keys by path, so buffers for non-chosen-but-content- duplicate paths stay in `assetFiles` until the function returns. Bounded by bundle size, not a correctness issue, and pruning all sibling paths would require a hash→paths reverse map — left for a follow-up. Adds an integration test covering: presign request has unique-blake3 entries only, exactly one R2 PUT per unique hash, and `finalizeDeploy` payload's assetManifest still contains every path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…callers `buildVoidManifest`'s docstring claims "Pure shape transformation: reads no files, has no I/O", but the SPA-fallback branch was calling `onProgress?.(msg)` and `console.warn(...)` for the non-SPA-fallbacks mismatch. Cursor flagged the contract mismatch. The warning condition only depends on inputs the callers already have (`assetConfig`, `fallbackRules`), so factor it out to a pure helper `detectNonSpaFallbackWarning(assetConfig, fallbackRules) -> string | null` and have the two callers (`packageBuild` for the legacy multipart path, `runFullDeploy`'s direct-r2 branch for JSON-mode) emit it before invoking the builder. The `onProgress` parameter is dropped from `buildVoidManifest`'s opts. User-visible warning text and fanout (progress callback + `console.warn`) are unchanged. `buildFrameworkManifest` and `buildStaticManifest` were already clean — the bot's "Additional Locations: 1" hint was wrong. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…load The optional `vars?: Record<string, string>` field on `ManifestOnlyDeployPayload` was added with the original direct-to-R2 commit and never populated by any code path. Env vars travel inside `manifest.vars` via `buildVoidManifest`/`buildFrameworkManifest`. Drop the field to remove confusion about where vars are expected on the wire. The platform's `handleManifestOnlyDeploy` had a defensive `body.vars` reader for the same dead field; cleaned up in the platform commit that bumps this submodule pointer. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The direct-to-R2 deploy path (design doc 0068) is now the only supported flow. Drop the `--legacy-upload` flag and the entire multipart packager: - args/cli/help/docs: drop the `--legacy-upload` flag - deploy.ts: drop legacyUpload branches, streamDeploy(), appendDeployMetadata() - package.ts: drop packageBuild, packageStaticBuild, packageFrameworkBuild, appendAssetsFromCache, appendAssets — keep collectAndHashAssets, the manifest builders, and collectWorkerFiles for the direct-R2 flow - client.ts: drop the multipart deploy() method, keep finalizeDeploy(), requestUploadUrls, preflight, and friends - tests: drop the multipart cases from package.test.ts and direct-r2-deploy.test.ts; rewrite migration-binding-consistency to exercise buildVoidManifest / buildFrameworkManifest directly Old CLI versions on npm will fail against the new platform until users upgrade — accepted per the rollout plan, since CLI and platform ship together.
cpojer
pushed a commit
that referenced
this pull request
Sep 11, 2026
* feat(api/r2-presign): presigned R2 PUT URLs with SHA-256 enforcement
Adds packages/api/src/r2-presign.ts: a small module that generates
presigned PUT URLs for the direct-to-R2 asset upload flow described in
meta/design-docs/0068-direct-r2-asset-upload.md (Task 1).
The signed URL covers x-amz-checksum-sha256 in X-Amz-SignedHeaders, so
R2 verifies the uploaded body hashes to the declared digest at write
time. Tampering the header invalidates the signature, and a content
mismatch is rejected by R2 with HTTP 400 — no server-side rehash.
- Uses aws4fetch (signQuery + allHeaders) so Content-Type and the
checksum header are both included in the canonical request.
- Pure function: takes credentials/options as parameters, reads no env.
The endpoint that calls it (Task 2) loads R2_ACCESS_KEY_ID and
R2_SECRET_ACCESS_KEY from env and threads them through.
- Documents the two new secrets in env.ts and wrangler.jsonc.
- Tests cover URL shape, hex-to-base64 conversion, X-Amz-Expires TTL,
determinism via a now override, and that x-amz-checksum-sha256 is in
X-Amz-SignedHeaders.
* docs(operations): document R2 presigning secrets
Add R2_ACCESS_KEY_ID and R2_SECRET_ACCESS_KEY to the API worker
secrets table and the fresh-environment provisioning checklist; they
are required by the new R2 presigning utility used for direct-to-R2
asset uploads in the deploy flow.
* feat(api/r2-presign): switch from SHA-256 to Content-MD5 for R2 upload integrity
Cloudflare R2's S3 compatibility layer documents Content-MD5 (RFC 1864)
as the supported PutObject checksum, while x-amz-checksum-sha256 is not
enforced and would silently no-op. The previous SHA-256 design would
have shipped a signed header R2 ignored, breaking the "R2 rejects
mismatched bytes at write time" guarantee on the wire.
Switching to Content-MD5 keeps the same architectural property with a
documented, supported wire contract:
- presignAssetPut takes md5 (32 hex) instead of sha256 (64 hex)
- header signed becomes Content-Md5, value is base64 of the raw 16-byte
digest with standard padding (not URL-safe)
- aws4fetch's allHeaders: true keeps the header genuinely covered by
the V4 signature, so R2's edge enforces it
MD5's weaker collision resistance is acceptable: the hash is purely an
upload-integrity primitive, content addressing stays on BLAKE3, and
project-scoped namespacing means any collision is exploitable only
against the attacker's own project. The no-overwrite-guard concern
from adversarial review is also closed, because R2 rejects any PUT
whose body MD5 doesn't match the declared header — replays and buggy
CLIs cannot clobber live keys with mismatched bytes.
Design doc 0068 updated end-to-end (assetManifest v2 schema, wire
contracts, Task 1-4 specs, Risks section). All 6 unit tests pass with
the empty-string MD5 fixture (d41d8cd98f00b204e9800998ecf8427e ->
1B2M2Y8AsgTpgAmY7PhCfg==).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat(api/deploy): add direct-to-R2 upload endpoints + JSON-mode deploy
Task 2 of design doc 0068. Adds two endpoints to packages/api/src/routes/deploy.ts:
- POST /:id/deploy/upload-urls — issues batched presigned R2 PUT URLs
(Content-Md5 signed, 10-min TTL) so the CLI can upload asset bytes
directly to STATIC_ASSETS_R2 without going through the API Worker
request body. Auth/ownership mirrors /deploy/preflight.
- POST /:id/deploy now branches on Content-Type at handler entry. JSON
bodies route to handleManifestOnlyDeploy; multipart stays bit-for-bit
unchanged for old CLIs. The JSON-mode handler validates the v2 asset
manifest, head-checks each declared blake3 in R2 in bounded
concurrency, then threads through the same post-validate pipeline as
multipart (precheck-and-claim, dedupe short-circuit, status
transitions, NDJSON event stream, routing switch, retention cleanup,
placeholder reaper). For WfP deploys it hydrates assetsByHash by
fetching bytes back from R2 so the existing CF asset upload session
flow keeps working unchanged.
The post-parse pipeline is duplicated (carefully, with cross-reference
comments) rather than extracted into a helper. Extracting would have
required threading 15+ pieces of parse-loop-local state through a
shared function, with high risk of regressing the multipart path. The
duplication is mostly mechanical mirroring of the existing pipeline.
Adds R2_STATIC_ASSETS_BUCKET to wrangler vars (production + staging) so
the presigner can read the bucket NAME at runtime — STATIC_ASSETS_R2
binding only surfaces the R2Bucket object.
17 new integration tests in test/integration/deploy-direct-r2.test.ts
cover: presign-URL endpoint shape + validation + auth, JSON-mode static
deploy success path, missing-hash rejection, WfP deploy with
workerFiles, dedupe short-circuit, mid-pipeline cancellation,
v2-version enforcement, manifest validation, plus a multipart
backward-compat smoke test. All 506 existing API integration tests
still pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/deploy): three JSON-mode bugs from direct-to-R2 adversarial review
Three small but distinct bugs in the JSON-mode deploy handler introduced
in 4cc3524e (Task 2 of design-doc 0068). All three live in the same
handler so they ship as a single commit.
1. Carry-forward R2-head verification (static branch)
`handleManifestOnlyDeploy` previously R2-head'd every declared asset
hash and failed 400 if any were missing. But existing static deploys
may have small assets in KV (`STATIC_ASSETS`) — not R2 — and the
multipart path's preflight is happy to skip those without re-upload.
JSON mode now distinguishes carry-forward hashes (present in the
prior live deploy's `asset_hashes`) from new hashes (uploaded
direct-to-R2 this deploy). Only new hashes are R2-head'd. The
static-branch serving manifest also preserves the prior KV-vs-R2
placement for carry-forward entries; only newly uploaded hashes get
the `!{hash}` (R2) prefix. Mirrors the multipart static branch's
existing prev-prefix-preservation logic.
2. workerFiles rejection for spa/static manifests
The multipart path explicitly rejects `worker:*` parts when
`manifest.type` is `spa` or `static`. JSON mode silently accepted
them, then routed to the WfP branch — breaking the project type
invariants. Now reject 400 BEFORE the precheck-and-claim transaction
so a rejected request leaves no phantom placeholder row.
3. Canceling-placeholder reaper
After the placeholder row is inserted by the precheck-and-claim
transaction, several validation paths can early-return without going
through the catch handler (asset-verify failures, quota exceeded,
etc.). If `cancelDeployment` lands in that window, the row's status
flips to `canceling` and the existing finally-reaper — which only
handles `pending + manifest=null` — leaves the row stranded. Extend
both reapers (multipart + JSON) to also delete `canceling +
manifest=null` rows. Multipart had the same gap as a pre-existing
bug; fixed for consistency.
Tests added in `deploy-direct-r2.test.ts`:
- Carry-forward KV-stored asset survives a JSON-mode redeploy and the
new serving manifest preserves KV placement.
- workerFiles rejected 400 for `spa` and `static` types, and no
placeholder row remains.
- cancelDeployment racing the asset-verify early-return is reaped
cleanly (no row left in `canceling` or `pending`).
* fix(api/deploy): strip v2 manifest fields before Cloudflare WfP asset session
The framework CLI emits v2 assetManifest entries shaped as
{ blake3, md5, size, hash } where hash is a deprecated alias of blake3.
The platform accepts these via structural typing and propagates the
manifest mostly verbatim through the deploy pipeline. Cloudflare
documents the assets-upload-session body shape as { hash, size }
per entry only and may strictly validate.
Add normalizeForCfAssetSession in packages/api/src/assets.ts that
returns a fresh { hash, size } object per entry, and apply it at the
two CF-bound call sites in packages/api/src/routes/deploy.ts (the
multipart and JSON-mode WfP paths). The persisted manifest in the DB
asset_hashes column is unchanged so future preflight reasoning still
sees the full v2 shape.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/deploy): widen JSON body cap and lengthen presign TTL
Two regressions surfaced by the Codex adversarial review of the Task 4
direct-to-R2 deploy work:
1. MAX_JSON_DEPLOY_BODY_BYTES (32 MiB) was tighter than the per-module
raw cap (32 MiB) accounting for ~33% base64 expansion: a single
full-size worker module weighs ~43 MiB on the wire. Bump the cap to
64 MiB so a full-size module + manifest + assetManifest fits with
safety margin, still well under the 128 MiB Worker memory limit.
2. Presigned PUT TTL of 600s expired on the tail of slow deploys
(570 assets × concurrency 10 × residential uplink) before the
upload pool reached the asset, and R2's SignatureExpired 403 was
surfaced as terminal. Bump TTL to 1800s. Pathological multi-hour
deploys are deferred to the re-presign-on-expiry follow-up noted
inline.
Tests: assert X-Amz-Expires=1800 on issued upload URLs, and exercise
a JSON-mode deploy body comfortably above the legacy 32 MiB cap to
prove the regression is fixed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* chore: bump framework to rebased HEAD + propagate git fields on JSON-mode
Picks up the framework branch's rebased HEAD f70264b (post origin/main
rebase). Also extends the JSON-mode `handleManifestOnlyDeploy` to read
`repositoryUrl`, `commitUrl`, `gitRef` from the body and pass them
through to `updateDeploymentManifest` so the new deploy metadata fields
introduced on platform main land for direct-to-R2 deploys too — not
just multipart.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* chore: bump framework + drop dead body.vars reader on JSON-mode handler
Bumps framework submodule to 5c22706 — picks up:
- refactor(cli/package): buildVoidManifest pure, warning moved to callers
- refactor(cli/client): drop unused vars field on ManifestOnlyDeployPayload
Pairs the framework wire-shape cleanup with the platform side: removes
the defensive `body.vars` reader from `handleManifestOnlyDeploy`. No
released CLI ever sent top-level `body.vars` (env vars always travel
inside `manifest.vars`), so this is dead code, not a wire-compat
change. Replaced with a one-line comment documenting the contract.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* refactor(api): remove legacy multipart deploy upload path (bump framework)
The direct-to-R2 deploy path (design doc 0068) is now the only supported
flow. Drop the multipart streaming handler in `routes/deploy.ts` and its
supporting infrastructure:
- `routes/deploy.ts`: replace the multipart `app.post('/:id/deploy', ...)`
body with a direct dispatch to `handleManifestOnlyDeploy`. Drop the
multipart-only constants (`MAX_MANIFEST_BYTES`, `MAX_ASSET_MANIFEST_BYTES`,
`MAX_PREFLIGHT_BYTES`, `MAX_SMALL_TEXT_BYTES`, `STREAMED_ASSET_CLAIM_*`)
and helpers (`readPartBytes`, `readPartText`, `discardPartBody`,
`readPartAsBlob`, `streamedAssetClaimKey`).
- `assets.ts`: drop the `./multipart.ts` import. Inline the
`MultipartParseError` and `PartTooLargeError` shells locally — the
multipart streaming asset helpers (`verifyStreamedAssetPart`,
`streamStaticAssetToR2`, `readStaticAssetPart`, `uploadStaticAssetPart`,
`cleanupStreamedStaticAssets`) and `collectBufferedAsset` are now dead
code, but kept for now so this PR doesn't grow further; they'll be
swept in a follow-up since the JSON-mode handler no longer references
them.
- `multipart.ts` + `multipart.test.ts`: deleted outright.
- Test conversion: rewrite `api-deploy.test.ts` to use a JSON-mode
`buildDeployRequest` helper that pre-uploads asset bytes to
`STATIC_ASSETS_R2` and POSTs the manifest-only body. Drop the entire
"(streaming static)", "(streaming static direct-to-R2)", and "(edge
cases)" describe blocks — they tested multipart parser semantics that
no longer exist; manifest validation coverage stays in the
`manifest-rule-limits` unit test. Convert `api-sandbox.test.ts`,
`deploy-collision-warning.test.ts`, and `deploy-direct-r2.test.ts`
similarly. Drop the heartbeat test (the JSON-mode deploy is too fast
for the heartbeat-during-pipeline assertion to fire deterministically).
- `meta/design-docs/0068-direct-r2-asset-upload.md`: append a "Migration
complete" section noting the cleanup.
- Bump the framework submodule to the matching CLI commit.
Old CLI versions on npm will fail against the new platform with a clear
JSON 400 until users upgrade — accepted per the rollout plan.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* chore: bump framework submodule to origin/main HEAD
Framework PR #192 (direct-to-R2 asset upload + legacy-multipart removal)
landed on framework main as 5d5f609. Bump the submodule pointer so the
platform PR references the merged commit instead of the now-deleted
feature branch.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/upload-urls): streaming size cap before JSON parse
The `POST /:id/deploy/upload-urls` handler called `c.req.json()` directly,
which buffers arbitrary body bytes before parsing. A malicious or buggy
client could pin Worker memory by sending a huge JSON blob. Replace with
the same TransformStream + size-cap pattern used by
`handleManifestOnlyDeploy`, capped at 4 MiB — generous for 5000 asset
declaration tuples (~750 KB) with a 4× safety margin. Asset bytes never
travel this endpoint; they go direct-to-R2 via the presigned PUTs it issues.
Codex P2 review finding.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/deploy): stream JSON body via TransformStream to avoid OOM
The JSON body in `handleManifestOnlyDeploy` was previously buffered as a
chunks Array, copied into a Uint8Array, decoded into a UTF-16 string, and
parsed — peaking at ~4× the body size in residency. For a legitimate
64 MiB body this means 240+ MiB of peak memory, dangerously close to the
Worker isolate's 128 MiB hard limit.
Replace with a counting TransformStream + native `Response.json()`. The
TransformStream aborts the stream the moment the cap is exceeded; the
native parser consumes the stream incrementally with reused internal
buffers. The only retained allocation at the end is the parsed object.
Codex P1 review finding (high impact — every deploy goes through this).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* chore(api/assets): remove dead multipart-streaming helpers
The streaming-static and worker-backed multipart deploy paths were retired
when JSON-mode direct-to-R2 became the only deploy entry point. The
helpers and their backing error classes were left behind as dead code:
- `MultipartParseError`, `PartTooLargeError` (stub classes)
- `streamStaticAssetToR2`, `verifyStreamedAssetPart`
- `readStaticAssetPart`, `uploadStaticAssetPart`
- `cleanupStreamedStaticAssets`, `collectBufferedAsset`
- `readAssetBody`, `updateBase64Hash` (private helpers)
- `PreparedStaticAsset` interface
- unused `createHasher` import
Drop them along with the now-orphan `cleanupStreamedStaticAssets` test
block. `applyDeclaredAssetManifest`, `normalizeForCfAssetSession`,
`buildComparableAssetManifestForDedupe`, `uploadStaticAssets`,
`uploadAssetBuckets`, `createAssetUploadSession`, and the worker-bundle
helpers all remain — those are still on the live path.
Verified production code carries no remaining references; integration
test setup files mention the names only inside historical comments.
Cursor LOW review finding (sweep follow-up).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* refactor(api/deploy): remove unreachable preflight fill-in loop
In `handleManifestOnlyDeploy` the post-`applyDeclaredAssetManifest`
preflight fill-in loop was unreachable. The two arguments are always the
same `incomingAssetManifest` object, so the helper produced an output
manifest containing every key from the input — and the loop's
`if (!(path in assetManifest))` predicate could never be true. The
`getLastLiveAssetManifest` fallback (`?? await ...`) was equally
unreachable because `incomingAssetManifest` is always non-null in this
handler. Replace `applyDeclaredAssetManifest(...)` with `{ ...incomingAssetManifest }`,
the only effect that the call ever had at this site.
Update the surrounding comment block to reflect that JSON-mode CLIs
already send the full assetManifest (preflight-skipped paths included).
Add a defensive contract test pinning that behaviour.
`getLastLiveAssetManifest` and `applyDeclaredAssetManifest` remain
imported / exported — they're still used elsewhere
(`buildComparableAssetManifestForDedupe`, rollbacks, persistence
recovery, `applyDeclaredAssetManifest` unit tests).
Cursor MEDIUM review finding (dead code).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/upload-urls): reject deleting projects with 410
The `POST /:id/deploy/upload-urls` handler issued writable presigned R2
URLs after only ownership/existence checks, even for projects already
marked `status='deleting'`. Such projects can never finalize a deploy —
`POST /:id/deploy` returns 410 — so any bytes uploaded against those
URLs become orphaned `assets/{projectId}/*` blobs that survive teardown
and add storage churn.
Mirror the guard `handleManifestOnlyDeploy` already enforces, returning
410 immediately after the ownership check and before any presigning.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* perf(api/deploy): single-pass base64 decode for workerFiles
The JSON-mode deploy handler decoded each base64-encoded user worker
module with `atob` + a hand-rolled per-byte loop, materializing four
full-size copies of the bytes (UTF-16 source string, latin1 `bin`
string, `Uint8Array`, and the eventual `Blob` part). Iterating via
`for…of Object.entries(...)` also captured the entire `workerFiles`
map for the full loop, holding every base64 string live until the
loop exited. For a near-cap 32 MiB module that pushed peak isolate
residency to ~214 MiB — past the 128 MiB worker cap — and terminated
the deploy before the pipeline ever ran. This was a regression from
the multipart path, whose blob-first handling never paid the
intermediate-string cost.
Two changes drop peak residency to ~118 MiB for one near-cap module
(under the cap with margin):
1. Prefer the (TC39 stage-4) `Uint8Array.fromBase64` helper — single
allocation, no intermediate string. Production CF Workers
(compatibility_date=2026-02-12) ships the helper natively; the
previous `atob` loop is kept as a fallback for runtimes that don't
expose it (Node-based integration tests).
2. Iterate over `Object.keys(...)` and `delete` each source string
from the parsed body after decode, so V8 can reclaim the ~86 MiB
UTF-16 backing string before the next module's decode begins.
Adds a regression test exercising a multi-entry `workerFiles` map
that round-trips every decoded module byte-for-byte through the
persisted R2 worker bundle — would surface either iteration skips
caused by `delete`-as-we-go or any byte-level corruption from the
`fromBase64` helper.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/teardown): sweep assets/{projectId}/* during project deletion
Direct-to-R2 deploy lets the CLI upload bytes to `assets/{projectId}/{blake3}`
before finalizing. Bytes uploaded via presigned PUTs that never reach a
finalized deploy (TOCTOU race against teardown, abandoned upload, etc.) live
durably in R2 — retention cleanup only walks hashes referenced by retained
manifests, so without a teardown sweep these would leak permanently.
Add `deleteProjectAssets(r2, projectId)` mirroring `deleteWorkerBundle`
(paginated list + bounded batch delete) and call it from
`cleanupBestEffortResources`. Failures are warnings, matching the
surrounding best-effort semantics.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/deploy): extend retention cleanup with R2 keyspace sweep
The retention loop only deleted hashes named in EVICTED deployments'
asset_hashes. Bytes uploaded via direct-to-R2 presigned PUTs that never
finalized into a live deploy are not named in any manifest, so they
outlived eviction indefinitely.
Add `sweepProjectAssetOrphans(r2, projectId, retainedHashes)` and call
it at the end of both retention paths (WfP + static). Also build the
retained-hashes union unconditionally — previously skipped when
`toEvict.length === 0`, which is precisely when the orphan sweep is
the only cleanup running. Best-effort try/catch so a sweep failure
never fails the deploy.
Plus a TODO in upload-urls noting the active-project case (abandoned
upload that never deploys) is still only opportunistically GC'd by
the next retention run; periodic per-project sweep is follow-up work.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/upload-urls): serialize presign with teardown via FOR SHARE lock
The unlocked status check in `POST /:id/deploy/upload-urls` was a TOCTOU
gap: a teardown calling `lockAndMarkDeleting` (FOR UPDATE) could land
between the unlocked read and the presigner, after which presigned R2
PUT URLs would let the CLI write durable orphan bytes against a
project being deleted.
Wrap the presign loop in a transaction that takes FOR SHARE on the
project row and re-reads the project status inside the lock. FOR SHARE
is compatible with concurrent upload-urls / deploy-precheck flows
(also reading project rows) but blocks teardown's FOR UPDATE until the
presign batch completes. A 5000-entry batch holds the lock for
~250 ms (presigning is pure HMAC, no I/O), which is comfortably
shorter than the surrounding teardown timeline.
The unlocked early-out is kept as a fast 410 path for the common case
where the project was already marked deleting before the request
arrived; the in-tx re-read is the authoritative check.
Adds a regression test that simulates the race by intercepting the
second `getProject` call (in-tx, post FOR SHARE) so it sees a
status='deleting' that committed between the two reads. To support
the test, `setup.ts` exposes a `setGetProjectOverride` hook similar
to the existing `setTestDb`.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/assets): protect pending uploads in retention sweep with age window
Direct-to-R2 presigned PUTs land bytes in the asset keyspace before any
deploy manifest references them. The retention sweep walked every
`assets/${projectId}/*` key whose hash wasn't in the union of retained
manifests, which raced concurrent `upload-urls`-then-finalize flows on
the same project: a sweep firing between an upload-urls PUT and its
deploy's manifest persist would delete the bytes and break finalize with
"asset not found".
Add a 3600s pending-upload protection window (2× the 1800s presign TTL)
in `sweepProjectAssetOrphans`. R2 objects whose `uploaded` timestamp is
inside the window are preserved even when not in `retainedHashes`. The
tradeoff is documented in the function docstring and on the existing
`TODO(orphan-gc)` follow-up: orphans uploaded within the window only
clear on the next eligible deploy ≥ 1h later.
Also extends the test mock (`MemoryR2Bucket`) to expose
`uploaded` on `list()` responses and a `setUploadedAt` helper, so the
existing orphan-sweep regression can age its orphan past the window and
a new companion test covers the within-window preservation case.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* refactor(api/assets): extract shared paginated-list-and-delete helper
`deleteProjectAssets`, `sweepProjectAssetOrphans`, and `deleteWorkerBundle`
all repeated the same R2 walk-and-batch-delete loop. Pull it out into a
single `paginatedR2Delete(r2, prefix, { keepIf })` helper and let each
call site become a 1–4 line wrapper. The orphan sweep's hash filter and
pending-upload protection window are expressed as the `keepIf` predicate
so the helper stays generic.
Behaviour-preserving — every existing test (unit `assets.test.ts`,
integration `deploy-direct-r2.test.ts`, `api-delete.test.ts`,
`api-deploy.test.ts`) passes against the refactored implementations.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(api/r2-presign): split length vs character validation errors
`hexToBase64` reported \`got \${hex.length}\` regardless of which
constraint failed, so a 32-char string containing non-hex characters
produced the misleading \"got 32\" message — opaque during CLI
debugging. Split the check: length first, character-set second, with a
distinct message for each.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
cpojer
pushed a commit
that referenced
this pull request
Sep 11, 2026
* docs(ops): container deploys need a user-owned CF_API_TOKEN After Deploy-on-GitHub (#179) added the BuildManager container, wrangler deploy builds + pushes a container image and calls GET /containers/me first. Cloudflare's Containers/Cloudchamber gateway rejects account-owned API tokens there with 403 "Authentication error" — even with Containers read/write granted. Document that the GitHub Actions CF_API_TOKEN must be a User-owned token, add Containers + Cloudchamber:Edit to the permissions table, and add a "Diagnosing container-deploy 403" runbook. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * style(ops): oxfmt meta/operations.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a direct-to-R2 asset upload path to
void deploythat bypasses the multipart endpoint, eliminating the ~64.5s streaming-body cancel that bricked the memora deploy (outcome=canceled, cpuTime=817ms, wallTime=64534ms). Asset bytes now flow CLI → R2 directly via presigned PUTs; only the manifest goes through the API Worker.packages/void/src/cli/package.ts): single buffered read per asset → both BLAKE3 (storage key, Wrangler-compat) and MD5 (Content-MD5 R2 verification).assetManifestv2 fields{blake3, md5, size, hash (deprecated alias)}.packages/void/src/cli/direct-r2-upload.ts): bounded-concurrency PUT pool (10), 5xx retry with backoff, 4xx terminal, instrumented viacliLog. Buffer references dropped per-PUT viaonAfterPut.packages/void/src/cli/deploy.ts): preflight → presign → R2 PUTs → finalize.assetFilespruned to needed-only after preflight.--legacy-uploadescape hatch routes through the old multipart endpoint for rollback or env without R2 secrets configured.Test plan
asset-hashing.test.ts(137 LOC) — buffer→{blake3,md5} contractdirect-r2-client.test.ts(248 LOC) — request shapesdirect-r2-upload.test.ts(196 LOC) — pool concurrency, retry, terminal-error semanticsdirect-r2-deploy.test.ts(638 LOC) — preflight → presign → PUT → finalize end-to-endPairs with
void-platform#<TBD>— platform must merge first for the new wire endpoints to exist.Note
Medium Risk
Changes the deploy upload protocol and client-side deploy orchestration, so failures could block deployments or upload incorrect asset metadata (hashes/content-type) if the platform contract diverges.
Overview
Adds a new direct-to-R2 deploy path for
void deploy: the CLI now preflights assets, requests presigned R2 PUT URLs, uploads required assets to R2 with bounded concurrency/retries, then finalizes by POSTing a JSON manifest (plus optional base64 worker modules) to the platform.Upgrades asset hashing/manifesting to a v2
assetManifestshape including bothblake3(storage key) andmd5(signedContent-Md5integrity), adds MIME-type inference for signed PUTs, and refactors packaging into I/O-free manifest builders (buildVoidManifest/buildStaticManifest/buildFrameworkManifest) pluscollectWorkerFiles.Introduces
direct-r2-upload.ts(PUT pool + retries + structured logging), updatesPlatformClientwithrequestUploadUrls()andfinalizeDeploy()streaming, and adds comprehensive unit/integration tests covering hashing, presign/finalize request shapes, retry/error behavior, dedupe-by-hash uploads, and single-read-from-disk guarantees.Reviewed by Cursor Bugbot for commit 6e1f404. Bugbot is set up for automated code reviews on this repo. Configure here.