Repository navigation
feat: support partial (multi-request) scene uploads - #504
Open
LautaroPetaccio wants to merge 59 commits into
Open
LautaroPetaccio wants to merge 59 commits into
LautaroPetaccio wants to merge 59 commits into
Conversation
allow a world scene's content to be uploaded across several POST /entities requests via an optional `partial=true` form field. content is staged in a standalone pending_scenes table until every referenced file is present, at which point the completing request runs the full validation + deploy and the world goes live (auto-finalize); earlier requests return 202 with the still-missing hashes. - new pending_scenes table (migration 0025) + pending-scenes-manager. it is standalone (no fk to worlds) so a half-uploaded world never leaks into listings or validity checks before it is live - new partial-deployments logic component - validateFiles split into validateUploadedFiles (both phases) and validateNoMissingFiles (finalize only); new validator.validateStaging runs everything except content-completeness and the storage-backed size check; size is checked cumulatively as content accumulates - at most one pending scene per world + overlapping parcels; a newer partial deploy replaces it - deployment-ttl check anchored on the pending row's created_at so an upload can span longer than the ttl; a successful deploy removes the pending row - garbage collection keeps non-expired pending hashes; the eviction job also removes expired pending rows (PENDING_DEPLOYMENT_TTL, default 24h) - requests without the flag behave exactly as before
|
coverage: 92.128% (+0.6%) from 91.527% — feat/partial-deployments into main |
add integration tests for concurrent staging of the same entity: two distinct content batches uploaded in parallel, and two requests completing the content set at the same time (concurrent finalize, exercising the 23505 idempotent path). both assert the world is deployed exactly once with no leftover pending row and no server errors.
- reject a stale pending upload that would finalize over a newer scene on the same parcels (checked at staging entry and again right before deploy), mirroring catalyst's newer-deployment guard; a resumed hours-old upload can no longer silently roll back a newer deployment - garbage collection re-checks the current pending scenes before each delete batch, so content staged by an upload that starts mid-sweep isn't reclaimed - run the cumulative size check before the pending upsert, so an over-budget request no longer destroys another deployer's in-flight overlapping upload - restore the combined uploaded+missing file error report in the full validation path (validateAll short-circuited after the split) - parallelize staged file storage, collapse redundant storage lookups, only delete the pending row on a vanilla deploy when one exists (best-effort, so a cleanup error can't fail an already-committed deploy) - centralize PENDING_DEPLOYMENT_TTL in the pending-scenes manager (deleteExpired and getActivePendingKeys), removing the duplicated constant from the eviction job and GC handler adds regression tests for the stale-overwrite and over-budget cases.
each batch of a multi-request upload was re-running the slow external deployment-permission check. resume batches now skip it, but only when the signer is the deployer who created the pending record — creating it required passing the check, uploaded bytes are hash-verified against the staged manifest, and finalize re-runs the full validation before anything goes live. any other signer takes the full staging validation, so a third party can't ride an existing upload's fast path. the pending row is still upserted on every batch (always before storing files): re-asserting it resurrects the row if a competing overlapping upload replaced it between requests, keeping this batch's staged content protected from garbage collection. also moves the pre-finalize newer-deployment check to immediately before the deploy write (after the slow full validation) to minimize the window in which a concurrent newer deploy could be silently undeployed, and centralizes GC pending-key computation in the pending-scenes manager. adds tests: permission check runs once at staging plus once at finalize, and a different signer resuming someone else's upload is rejected.
…lize worlds is not exposed to the "trade mid-upload" gap fixed on catalyst: its deployment-permission check resolves the CURRENT name owner at validation time, so the finalize full-validation already rejects a deployer who traded the name away while the upload was in flight. this test pins that property so a future switch to timestamp-anchored permission checks can't silently reintroduce the gap.
- skip the newer-deployment check on resume batches: intermediate batches never deploy by themselves, so the pre-finalize re-check (kept right before deployScene) is the load-bearing guard; this removes a world_scenes query from every batch of a multi-request upload - project only the content hashes out of the pending scenes' entity jsonb in getActivePendingKeys, instead of shipping every staged manifest to the GC once per delete batch - restructure the partial-deploy integration spec per the dcl-testing standard: all setup and actions in beforeEach, it() blocks assert-only, flows expressed as nested describes (14 scenarios -> 32 assert-only cases)
- getActivePendingKeys: guard the jsonb content projection with jsonb_typeof(entity->'content') = 'array'. jsonb_array_elements raises on a scalar/object/json-null, so one pending row with a non-array content would fail the whole query and wedge garbage collection server-wide until it expired; the JS code this replaced tolerated it via `?? []` - storeFiles: store every uploaded file unconditionally instead of skipping from a pre-upsert storage snapshot, which could discard bytes uploaded in a batch if the file was swept before the store; content-addressed re-store is an idempotent no-op and the client already omits already-present files
- pending upsert now gives the single per-parcel-set slot to the NEWEST scene (deployment ordering: entity.timestamp, tie-break entity id). A strictly-newer overlapping upload is rejected instead of replacing, so a stale/older upload can't evict a newer competitor's staged content and two clients can't ping-pong evicting each other. A resume (same entity id) never conflicts. - cap the number of concurrent non-expired pending uploads per deployer (MAX_PENDING_DEPLOYMENTS_PER_DEPLOYER, default 10) so one account can't pin storage across many parcel-sets for the full TTL. Only new uploads count; a resume reuses its slot. (The comms rate limiter is a failed-auth limiter and doesn't fit a multi-request upload, so the cap is the abuse bound here.)
Addresses two races and a doc gap from review:
- Newest-wins was not atomic: the partial finalize checked for a newer scene and
then deployed in a separate transaction, so a newer vanilla deploy landing in
between was silently undeployed. Enforce ordering inside deployScene (the write
shared by vanilla and partial finalize): a per-world advisory lock + a reject
when a strictly-newer overlapping scene already holds the parcels, then replace
only older ones. Drop the now-redundant, racy pre-deploy re-check in the partial
component (the early new-upload check is kept as a fast-fail only).
- The per-deployer concurrent-pending cap was counted separately from the insert
and the upsert's lock is per-world, so a deployer's concurrent uploads to
different worlds could all pass the count and insert. Enforce the cap inside
upsert's transaction under a per-deployer advisory lock (taken before the
per-world lock, a deadlock-free order).
- Document the partial protocol in docs/openapi.yaml: the optional partial=true
field and the 202 { missing } response, and correct the description.
decentraland-bot
approved these changes
Jul 13, 2026
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Code Review — PR #504: feat: support partial (multi-request) scene uploads
This is an excellent PR that adds a well-designed partial deployment protocol to worlds-content-server, enabling scenes too large for a single request to be uploaded across multiple POST /entities requests. The architecture is thoughtful, concurrency handling is thorough, and tests are comprehensive.
Architecture & Design
The component boundaries are clean and well-separated:
PendingScenesManager— pure DB adapter, handles CRUD and locking forpending_scenesPartialDeploymentsComponent— orchestration logic (auth, validation, staging, finalize)deploy-entity-handler— HTTP-layer branching between partial and vanilla paths
Key design decisions I verified as sound:
-
Standalone
pending_scenestable with no FK toworlds— prevents half-uploaded worlds from leaking into listings and validity checks. Correct isolation choice. -
Lock ordering — deployer lock (
pending_deployer:X) acquired before world lock (pending_scenes:Y), and the separateworld_scene_deploy:Ylock indeployScene. These are distinct namespaces with consistent acquisition order — no deadlock risk. -
Deployment ordering — newer-wins semantics (timestamp, then entity-id tiebreak) enforced at three levels: (a) early fast-fail in
assertNoNewerDeployment, (b) atomically inpending_scenesupsert, (c) atomically under advisory lock indeployScene. Belt-and-suspenders, but the right call for data integrity. -
GC protection — the
deleteBatchre-queriesgetActivePendingKeys()before each batch to protect content from partial uploads that started after the initial snapshot. Combined with the invariant thatupsertruns BEFOREstoreFiles, this closes the race window. -
Concurrent finalize — two requests completing the same upload race to
deployScene; the loser catches the unique violation and returns idempotent success. Clean handling. -
Size budget checked BEFORE upsert — prevents an over-budget request from destroying a competing deployer's in-flight pending upload via the overlap-replace logic. Tested explicitly.
Security
- ✅ All database queries use
sql-template-strings(parameterized) — no injection vectors - ✅
skipPermissionCheckfast-path is safe: only applies when the signer matches the pending row's deployer, and finalize always re-runs the full validation including permission checks. Different signers go through full staging validation - ✅ Per-deployer concurrent pending cap (
MAX_PENDING_DEPLOYMENTS_PER_DEPLOYER) prevents storage-pinning abuse - ✅ Cumulative size budget prevents storage exhaustion via partial uploads
- ✅ No hardcoded secrets, no sensitive data in logs or error messages
- ✅ Input validation:
partialfield only activates on'true' - ✅ No unconsumed fetch response bodies
- No security issues found.
API Compatibility
The changes to POST /entities are fully backward-compatible:
- The
partialform field is optional — requests without it behave identically to today - The new
202response only occurs whenpartial=trueis sent - Existing clients (builder, unity-explorer, bevy-explorer, hammurabi-headless) never see the new behavior
- No existing exports, types, or response shapes are changed
Validation Split
The refactoring of validateFiles → validateUploadedFiles + validateNoMissingFiles is clean. The composed validateFiles preserves the original behavior of reporting both error types together (no short-circuit). The staging path correctly runs only validateUploadedFiles since content completeness can't be asserted mid-upload.
Test Coverage
The integration test suite (partial-deploy.spec.ts, 696 lines) is thorough, covering:
- Multi-batch upload, entity file omission on resume, single-request auto-finalize
- Resume fast-path (same deployer), different-signer rejection, permission loss mid-upload
- Idempotent replay, missing entity file rejection
- Concurrent distinct batches, concurrent duplicate completing requests
- Stale finalize over newer deployment, newer-replaces-older pending, older-can't-replace-newer
- Over-budget request not destroying competing upload
- Per-deployer concurrent cap enforcement
- Vanilla deploy ordering (atomically enforced in
deployScene)
Unit tests updated for the validation split and eviction job changes.
P2 Suggestions (non-blocking)
-
[P2]
StageDeploymentResultcould be a discriminated union for stronger compile-time guarantees:type StageDeploymentResult = | { complete: true; result: DeploymentResult } | { complete: false; missing: string[] }
Currently the optional fields work fine in practice since
completeis always checked first, but a union would make invalid states unrepresentable. -
[P2] Global expired-row cleanup inside
upsert(line ~97 ofpending-scenes-manager.ts) — theDELETE FROM pending_scenes WHERE created_at < ${expiryCutoff}clause removes ALL expired rows globally on every upsert, not just those for the current world. Givenpending_scenesis expected to stay small this is fine, but if the table grows, consider scoping the opportunistic cleanup to the current world or removing it (the eviction job already handles expiry).
Git Conventions (ADR-6)
- ✅ PR title:
feat: support partial (multi-request) scene uploads— correct semantic format - ✅ Branch:
feat/partial-deployments— correct<type>/<summary>pattern
Verdict: ✅ APPROVE
No P0 or P1 issues found. The feature is well-architected, handles concurrency correctly, protects against abuse, and has excellent test coverage.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U3BMPDZ0F>) via Slack
- Bound entity-file memory: the entity manifest is small, so cap it (10 MB) and check the stored size before buffering it on a resume request — a resume body is tiny and isn't covered by the multipart in-flight budget, so without this cap many concurrent resumes could each buffer a large stored entity and exhaust memory. - Guard the GC-vs-finalize race: re-verify every referenced content file exists immediately before the deploy commits and return incomplete (client resumes) if any is missing, so a scene can't commit referencing a file a concurrent GC sweep reclaimed. Shrinks the DB-row-vs-object-storage window to the gap between the check and the commit. - Fix the per-deployer cap accounting: decide it on the NET row-count change inside upsert, under the deployer lock and after the overlap-replace, instead of a stale "is this new?" flag. A resume, or a newer scene that replaces one of the deployer's own overlapping rows, no longer counts as an increase. - Avoid orphaned content on rejected deploys: add worldsManager.hasNewerDeployedScene and call it in entity-deployer before writing anything, so an older deploy that will be rejected doesn't leave content/entity/auth objects stored until GC. deployScene keeps the authoritative race-free check (shared SQL helper). - Index pending_scenes(deployer, created_at) to back the cap query.
…load A completed partial upload can be finalized by several requests at once (the client's parallel worker pool and retries), and each would independently run the expensive full validation (incl. the external permission check) and attempt the deploy — today only the world_scenes PK insert stops the duplicate, after the work is already done. Add a lease to pending_scenes (status UPLOADING/FINALIZING + finalizing_at): a completing request atomically flips the row to FINALIZING and only then runs validation + deploy; concurrent completers fail to claim the lease and either see the scene already deployed (idempotent 200) or report in-progress (202) and let the client's resume loop converge. A lease older than a 2-minute TTL is treated as stale and can be taken over, so a crashed finalizer doesn't wedge the upload. The lease is released on a finalize that fails without deploying; the PK-insert idempotency remains the correctness backstop if two requests ever race.
…es.ts Follows the WKC per-component file layout for the two partial-upload components: - pending-scenes-manager becomes a directory (component.ts / types.ts / index.ts) instead of a single file. PendingScene, UpsertPendingScene and IPendingScenesManager move into its types.ts. - partial-deployments gains a types.ts holding StageDeploymentInput, StageDeploymentResult and IPartialDeploymentsComponent. The six types are removed from the central src/types.ts, which now imports the two component interfaces (type-only, so no runtime import cycle through their component.ts factories) for AppComponents. Behavior is unchanged.
- add hasNewerDeployedScene to the worlds-manager test mock so the test project compiles again (ci was red: ts2741) - revert the finalization lease: correctness already rests on the world_scenes primary key, and the lease's 2-min ttl vastly exceeds the client's ~7s resume budget, so a crashed/slow holder turned a succeeding upload into a client failure; its unfenced release could also clobber a live takeover's lease - cast entity->>'timestamp' as numeric, not bigint: the schema allows a fractional timestamp, and the bigint cast errored every overlapping staging/deploy query on such a stored entity - bound the deployment timestamp forwards (15 min tolerance) so a future-dated scene can't permanently win the parcels it occupies - enforce the per-deployer pending cap only when the upsert creates a new row, so lowering the cap never wedges in-flight resumes - delete a previously-undeployed self-row inside deployScene before the insert, so redeploying an undeployed entity no longer loops on a pk violation - enforce the resume entity-file size cap while streaming, covering the unknown-size (null) case instead of buffering first - delegate the new-upload ordering check to hasNewerDeployedScene and scope the expired-row purge to the upserted entity
correctness / security: - deployScene: exclude the entity's own row from the overlap soft-delete, so a concurrent finalize (or client retry) of an already-deployed entity collides on the world_scenes primary key (the idempotency signal) instead of soft-deleting then re-deploying — which double-published the SNS event and double-counted the deployment - verify the auth-chain signature over the entity id BEFORE reading the entity back from storage on a resume request, so an unauthenticated caller can't make the server retrieve and buffer up to 10mb per request (the resume read is not covered by the multipart in-flight budget) - garbage collection: also subtract the keys of scenes deployed since the sweep began, so a partial upload that finalizes mid-sweep (pending row gone, world_scenes snapshot stale) can't have its reused content reclaimed - apply the 10mb entity-file cap only on the partial path; the vanilla single-request path is bounded by the multipart budget, so capping it there wrongly rejected large-but-legitimate manifests that deployed before performance / cleanup: - stop shipping the multi-mb entity jsonb between postgres and node on every staging batch: drop it from the getByEntityId select and the upsert returning (no caller reads it), and only insert it on the first batch — a resume just bumps updated_at - run the independent pending-row and content-metadata lookups concurrently - drop the redundant storage head on the resume path (the capped read enforces the size limit on its own) - fix a stale finalization-lease comment and a vacuous self-exclusion in the per-deployer cap count
Reconcile the partial (multi-request) deployment feature with main's deployment hardening + performance work (#500–#514). Conflict resolutions: - validations/validator.ts: keep main's before/after-storage phasing (trackStage/trackWorker, signal-aware runValidations) and re-add the staging path (validateStaging + createStagingValidateFns) that the auto-merge dropped. Structural scene checks (incl. #514's relative-path thumbnail) run in both full and staging paths. - validations/common.ts: split into validateUploadedFiles (per-request hash/CIDv1 check) + validateNoMissingFiles (completeness), with the combined validateFiles threading main's concurrency/signal/trackWorker into the uploaded-file hashing. - worlds-manager.ts deployScene: route the advisory lock, newer-scene ordering check and self-row DELETE through main's withDeploymentTransaction query closure so, on the signal path, they run on the same dedicated connection as the writes (database.query would use a different connection and defeat the lock). The query closure now returns results so the in-transaction ordering SELECT can read rowCount. - entity-deployer.ts: keep the pre-storage hasNewerDeployedScene fast-fail ahead of main's concurrent trackStage('storage') upload. - deploy-entity-handler.ts: fold the partial/resume branch into main's signal-wrapped deployEntityWithSignal (createAbortContext, 499/408 mapping); union the handler component list; the resume-path inline DeploymentFile now implements the required getHash. - types.ts: keep both pendingCreatedAt (TTL anchoring) and main's contentFileInfos/signal + SceneDeploymentData; deployScene keeps hasNewerDeployedScene plus the deployment? parameter. - partial-deployments/component.ts: pass the cumulative size to entityDeployer.deployEntity (main made deploymentSize required). - test/components.ts: adopt @dcl/metrics (#502) while keeping the new partial-deployment component imports. Test harness: unit mocks updated for the new worldsManager.hasNewerDeployedScene and the handler's pendingScenesManager dependency. Verified: tsc --noEmit clean, eslint clean, 873 unit + 485 integration tests pass.
…path Post-merge review findings: - The finalize path persisted the start-of-request size estimate as world_scenes.size. Files stored by sibling requests after that snapshot was taken counted as 0, undercounting the scene size and skewing wallet quota accounting (getTotalWorldSize / getDeployedSceneSizeForParcels). The completeness check now fetches metadata (fileInfoMultiple) instead of bare existence and the persisted size is computed from it — sizes are immutable per content hash, so completeness-time metadata is exact. - That same metadata is passed to the finalize full validation as contentFileInfos, which removes createValidateSize's fallback of one sequential storage read per non-batch file and stops a GC-reclaimed file from surfacing as a terminal 400 mid-validation (the pre-commit re-check now catches it and returns the retriable incomplete instead). - The partial path ignored the deployment-processing abort context, so a client disconnect or the processing deadline never cancelled staging work (validation, hashing, file storing, the finalize deploy), and a genuine failure after the deadline fired was misclassified as 408/499. StageDeploymentInput now carries signal/deadlineAt and stage() threads them through validateStaging, storeFiles, and the finalize deployEntity; the pending row survives cancellation so the client simply resumes. Adds unit coverage for the sibling-upload size scenario and the signal threading/cancellation behavior.
…-deploy handling Second review round findings: - The partial-resume read-back was gated only by the local signature check, which any keypair holder passes for any entity id — cheap requests could each trigger a 10 MB storage read and multi-copy heap buffering, invisible to the multipart byte budget, exhausting the shared concurrency slots. The read now also requires a live pending upload for the entity created by the same signer: pending rows only exist for permission-validated deployers and are capped per deployer, so read-backs are bounded to in-flight uploads by their own deployer. Any other signer (or an expired upload) is told to re-send the entity file, which re-enters the fully-validated path. - A vanilla deploy racing a concurrent duplicate of the same entity (most commonly a client retry after a lost response, or this entity's partial finalize) surfaced the loser's world_scenes primary-key violation as a 5xx. The handler now verifies the scene is deployed and answers with an idempotent success, mirroring the partial finalize. - GC's per-batch deployed-since re-check (updated_at >= sweep start, status-agnostic) had no serving index — every 1000-key delete batch sequentially scanned world_scenes. Adds a btree index on world_scenes(updated_at) (migration 0026). - Folder-based storage's fileInfo can reject (exist->stat ENOENT window) when a file is reclaimed mid-check, turning the finalize completeness gate into a 500 on the very GC race it absorbs; S3 returns undefined instead. The gate now degrades to the error-hardened existence probe and reports the upload as still incomplete. - storeFiles used a bare Promise.all, which rejects on the first store failure while sibling reads of request-scoped temp files are still in flight; switched to mapWithConcurrency so started uploads settle before the error is rethrown (and dropped the batch barrier). - stage() short-circuits already-cancelled requests before any I/O, and the eviction job isolates its two clean-up steps so one failing does not stall the other for a cycle. Extends unit coverage: resume-gate authorization (no pending row, foreign deployer, own upload), duplicate-deploy idempotency and its not-deployed rejection path, and the no-state-written cancellation guarantee with the production abort-reason type.
…uracy
Third review round findings:
- update-owner-job: a partial or silent name-resolution degradation
(subgraph lag, per-call RPC gaps — findOwners reports undefined without
throwing) NULLed world owners in the DB (orphaning them from quota
accounting) and mass-unblocked wallets whose quota was never
re-evaluated, emitting spurious ACCESS_RESTORED events. Unresolved
names now keep their stored owner and their wallets are protected from
the stale-record cleanup until a run that can evaluate them.
- name-ownership: JSON-RPC batch responses were index-matched, but the
spec allows servers to return batch responses in any order — a
reordering provider would silently attribute owners to the wrong
names. Responses are now aligned to request order by id whenever every
request id is answered (positional fallback for out-of-spec providers).
- whitelist: normalize whitelist keys to lowercase on fetch — world
names are compared lowercased everywhere, so a mixed-case key in the
source silently dropped that world's paid limits and quota exemption.
- content-file-handler: the range path's MIME-sniff read is now
best-effort — a reclaimed object or transient storage error during the
sniff no longer turns an already-retrieved, servable 206 into a 500.
- partial finalize: the completeness gate retries the metadata read once
(the folder-storage exist->stat ENOENT race resolves deterministically
on re-read) instead of degrading to a self-contradictory
202 {missing: []}; a persistent failure now surfaces honestly.
- duplicate-deploy verification (vanilla and partial) propagates the
original unique-violation when the confirmation query itself fails,
and both idempotent success paths now return the full deployment
message (parcels + play URL, shared via buildSceneDeploymentMessage)
that the CLI surfaces as preview instructions.
- docs: openapi.yaml now documents the resume preconditions (live
same-signer pending upload, else re-send the entity file), the 503
Retry-After header, content-endpoint range/206/416/400 responses and
MIME sniffing, and the actual /available-content request/response
schema; database-schema.md catches up with migrations 0017/0023/0024/
0025/0026 (world settings and denormalized stats columns, scene
status/updated_at, all missing indexes, pending_scenes section and
diagram entry).
Extends unit coverage: unresolved-owner and total-resolution-failure
runs of update-owner-job (no NULL owner write, no mass-unblock).
…cases Fourth review round (verifying the round-3 fixes + a holistic data-flow sweep): - limits-manager: the round-3 whitelist-key lowercasing only fixed the blocking consumer; the limits consumer still looked up with the raw, possibly mixed-case world name. A mixed-case whitelisted world kept losing its paid size/parcel/SDK6 limits on the vanilla path, and its partial staging gate (which lowercases) disagreed with its finalize validation (which didn't) — a large multi-request upload was accepted batch-by-batch then rejected at finalize. Lowercase the lookup key so every caller agrees. - partial-deployments: the start-of-request fileInfoMultiple was left unguarded against the same folder-storage exist->stat ENOENT race that the finalize gate was hardened against (a resume batch could 500). Extracted a fileInfoMultipleWithRetry helper and applied it to both reads. - deploy-entity-handler: drop a pending row on EVERY vanilla success, not only when the early lookup saw one — a concurrent partial request could create the row after that snapshot, leaving it to linger until TTL and hold a slot of the deployer's pending cap. deleteByEntityId is an idempotent indexed delete, best-effort. - update-owner-job: don't protect an owner from stale-record cleanup on account of one unresolved world when it was still fully evaluated via another world it owns that did resolve (walletStats sums all of a wallet's worlds) — that wallet stays eligible for a legitimate unblock. - partial idempotent-finalize message now uses the same scene parcels as the normal deploy message (was canonicalized pointers) so a client that lands on the race path gets identical preview info. Adds a name-ownership test that exercises the JSON-RPC batch id-alignment with a provider that reorders responses (previously only the positional fallback was covered).
Main's scene-replacement authorization (#521) and the partial-upload path needed reconciling: - deployScene keeps main's authorization-scoped undeploy, with the branch's `entity_id != scene.id` exclusion applied to both modes and to the overlap re-check, so a re-deploy of a DEPLOYED entity still falls through to the primary-key collision that signals idempotent success instead of a 409. - The partial finalize now holds its DeploymentToValidate and forwards `sceneReplacementAuthorization` to deployEntity, which main made mandatory for scenes. - Migrations: main took 0025, so the pending-scenes table and the world_scenes updated_at index became 0026/0027 with descriptive ids and idempotent DDL. - Kept `rowCount` on main's generified transaction query type, which newerDeployedSceneExists counts through.
…exist A partial upload is now identified by its signed entity id alone. Uploads on overlapping parcels no longer replace each other; publication follows entity timestamp order, so an older entity never overwrites a newer one. Staging is bounded by per-account and server-wide byte reservations, a per-account accepted-bytes-per-minute window and the upload count cap. Expired uploads stay charged until their content is physically deleted. Uploads hold a shared advisory lock through publication and GC deletes under the exclusive one, on a dedicated pool. Stored-content receipts replace per-batch storage probes, and completion receipts make a retried completing request return the original result. Migration 0028 adds the accounting and receipt tables; 0026 drops the world_name and parcels indexes the overlap check needed.
decentraland-bot
requested changes
Sep 24, 2026
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-review found three remaining correctness and availability issues in the partial-upload path.
Findings
- P1 — Completion retries can still fail after a successful publish. The completion receipt is checked before acquiring the entity lock. A duplicate completion can pass that check, wait while the first request publishes and commits its receipt, then continue and attempt another
world_scenesinsert. The partial path does not translate that verified unique conflict to its stored completion result, so it returns a 5xx and leaves a fresh pending reservation. Re-check the completion receipt after acquiring the entity lock, or make partial finalization handle the verified duplicate atomically; add a concurrent completion-retry test. - P1 — Resuming an upload charges and re-stores a manifest that was not sent. The handler injects the storage-read entity file into
uploadedFiles; staging then treats it as incoming bytes and writes it again. Multipart-only resume batches can therefore consume the per-minute allowance from repeated manifest bytes and fail valid large/many-batch uploads. Keep reconstructed manifest data separate from request-supplied files, and only reserve/rate-count/store the latter. - P1 — Invalid requests can occupy the dedicated lock pool before authentication. The outer handler obtains the shared/advisory locks before
deployEntityWithSignalvalidates the partial auth chain. Requests targeting an entity currently being processed queue on its entity advisory lock while holding aCONTENT_LOCK_CONNECTIONSconnection. A small invalid-request flood can consume the pool and deny uploads/GC. Authenticate and perform cheap request validation before acquiring the lock, and avoid unbounded lock waits holding a pool connection.
The API addition is backward-compatible for existing POST /entities clients. I checked the Jarvis dependency graph: inbound users include builder, creator-hub, unity-explorer, and others; this optional partial=true behavior does not alter their existing request contract.
CI checks are passing. Local build was not runnable in this checkout because the TypeScript compiler is unavailable.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
- Authenticate requests before taking any content lock, and wait for a busy entity lock by retrying pg_try_advisory_lock with backoff instead of blocking, so waiters never hold a lock-pool connection. - A resume batch that omits the manifest validates the copy read back from storage but no longer charges, rate-counts or re-stores it. - A deployed entity is never staged again, so a completion retry by another signer or after the receipt expires gets a 400 instead of a 5xx and a leftover reservation. A unique conflict at publication is mapped to the stored receipt and drops this request's staging state.
Reservations and byte-rate usage are charged to the upload's creator, so a different signer continuing someone's live upload by resending the manifest could exhaust the creator's quotas. Such batches are now rejected with 400, matching the catalyst implementation.
decentraland-bot
requested changes
Sep 24, 2026
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-review: the previous completion-retry and resume-manifest issues are addressed. One admission-state bug and the lock-pool issue remain.
Findings
-
P1 — Rejected partial batches permanently consume a pending-upload slot.
stage()commitsupsert()beforereserve(), but these are separate transactions. Whenreserve()rejects due to the global/account budget or the per-minute rate, its receipt/rate transaction rolls back while the newly created zero-bytepending_scenesrow remains. Repeating rejected batches with distinct entity IDs lets an otherwise authorized deployer fill its 10-upload cap until the 24-hour cleanup, even though none of the batches were accepted. Make creation plus admission atomic, or remove a newly created pending row when admission fails; add coverage for budget/rate rejection followed by a new upload. -
P1 — Invalid vanilla requests still take connections from the dedicated content-lock pool before authentication.
authenticateRequest()only verifies a partial request. A normalPOST /entitiesproceeds intowithRead()without signature/authorization or even manifest validation; those checks happen later in the deployment validator. Invalid small requests can therefore queue at the shared/exclusive gate while consuming the limitedCONTENT_LOCK_CONNECTIONSpool, so the availability issue from the prior round remains for the non-partial path. Authenticate/perform cheap structural validation before acquiring the content lock, or keep unauthenticated requests off this pool.
The optional partial=true API addition remains backward-compatible for existing POST /entities callers. CI is passing.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
- Regular deployments are now authenticated before any content lock, with the same local auth chain, signer and signature checks the validator runs. - Uploads try the shared GC gate without blocking and retry with backoff, so they never hold a lock connection while GC runs a batch. - A first batch that fails admission discards the upload it created, so rejected batches can't fill the deployer's upload cap. - The per-minute byte charge is committed on its own before admission, so a batch rejected by the budget or rate limit still counts.
decentraland-bot
requested changes
Sep 24, 2026
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Changes requested
P1 findings:
• Expired completion receipts are not refreshed when the same entity is undeployed and deployed again, so a lost response from that later deployment cannot be retried successfully.
• Expired pending uploads are cleaned at only 100 per daily job invocation; a larger expired backlog can retain the global reservation budget for days and deny new uploads.
• The new index is created with a write-blocking PostgreSQL lock, which can stop scene deployments during rollout on a populated table.
The earlier completion-race, resume-manifest accounting, unauthenticated lock-pool, and rejected-admission-slot findings are addressed. CI is passing. The new partial=true behavior is additive to POST /entities; no downstream compatibility break was identified.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
An upload's lifetime and entity freshness were anchored on different instants: freshness on the validation clock, created_at on the database clock after metadata lookups and lock waits. The upload's liveness was also checked only at the start of a batch, so a completing batch admitted just before expiry could still publish after it, including after cleanup had removed the upload. The first batch now captures one admission instant before any I/O and uses it for both the freshness check and created_at. Publication passes the upload's expiry to the scene transaction, which refuses to commit once the pending row is gone or the expiry has passed, answering 400 with the regular expired-upload message.
decentraland-bot
requested changes
Sep 28, 2026
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-review found one remaining rollout-safety blocker.\n\n### Finding\n\n- P1 — The migration advisory lock can still be lost while migration SQL continues. retains the session advisory lock on , but , , and the migration-record insert all use the general pool (). If that idle lock connection drops while migration SQL is in flight, PostgreSQL releases the lock and another replica can acquire it and execute the still-unrecorded migration concurrently. The check happens only before each iteration, and the current test explicitly records the migration after simulating lock loss (). protects only the bookkeeping row, not DDL such as . Run migration queries and recording through the lock-owning checked-out session (or reliably abort the in-flight migration before another executor can proceed), and add a two-executor test that drops the holder during a delayed query.\n\nThe optional behavior remains additive to ; existing callers retain their request/response contract. I checked the Jarvis graph's 12 inbound HTTP consumers and found no compatibility break. Hosted CI is passing; local build/tests were not run because dependencies are not installed in the review checkout.\n\n---\nReviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
decentraland-bot
requested changes
Sep 28, 2026
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-review found one remaining rollout-safety blocker.
Finding
- P1 — The migration advisory lock can still be lost while migration SQL continues.
start()retains the session advisory lock onclient, butprepareMigrationsTable(),migration.run(components), and the migration-record insert all use the generaldatabasepool (src/adapters/migration-executor.ts:54-89). If that idle lock connection drops while migration SQL is in flight, PostgreSQL releases the lock and another replica can acquire it and execute the still-unrecorded migration concurrently. Thereusablecheck happens only before each iteration, and the current test explicitly records the migration after simulating lock loss (test/unit/migration-executor.spec.ts:167-185).ON CONFLICTprotects only the bookkeeping row, not DDL such asCREATE INDEX CONCURRENTLY. Run migration queries and recording through the lock-owning checked-out session (or reliably abort the in-flight migration before another executor can proceed), and add a two-executor test that drops the holder during a delayed query.
The optional partial=true behavior remains additive to POST /entities; existing callers retain their request/response contract. I checked the Jarvis graph's 12 inbound HTTP consumers and found no compatibility break. Hosted CI is passing; local build/tests were not run because dependencies are not installed in the review checkout.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
A POST /entities body that never completes is never authenticated or counted, so neither the deployment limits nor the partial-upload quotas bound it: one client could hold every slot and byte of the process-wide upload budget with slow bodies until the upload timeout. Every POST /entities request, partial or not, now takes a share of its client source's in-flight allowance before the parser runs (MAX_CONCURRENT_UPLOADS_PER_SOURCE, default 4, and MAX_IN_FLIGHT_UPLOAD_BYTES_PER_SOURCE, default one maximum-size upload; startup fails if lower), charged by declared Content-Length capped at the route maximum, and held until the request ends. Over the share answers 429 with Retry-After: 5. PUT /world/:name/settings shares the same parser budget, so it takes the same share. The source is cf-connecting-ip and the state is process-local. A request without that header is not limited per source, so internal or direct callers can't lock each other out in one shared share; only the process-wide budget bounds it, and it is counted in multipart_upload_unattributed.
The upload's admission instant was taken inside stage(), after the body had been read and parsed, so a slow first batch started its lifetime and freshness window late. Liveness was also not re-checked before storing a batch's files, and a batch that read a live upload which cleanup then removed would re-create it on upsert. POST /entities now stamps each request on arrival, before the body is read, and a new upload is admitted at that instant. Right before storing a batch's files its upload's liveness is re-checked (400 expired). A batch that saw the upload live tells upsert so, and upsert answers expired instead of re-inserting a row that is gone.
GC and expired-upload cleanup took the exclusive gate with a blocking pg_advisory_lock under a 10s lock_timeout. While such a writer waited behind an upload holding the shared gate, Postgres queued it, and every new upload's pg_try_advisory_lock_shared failed behind the queued exclusive request, so each writer attempt delayed all new uploads. Writers now use the non-queuing pg_try_advisory_lock, retried with exponential backoff (50ms up to 1s), so a writer that hasn't acquired the gate neither blocks readers nor holds a connection. The in-process one-writer queue and the bounded writerMaxWaitMs remain; past it the writer logs a warning and fails with ContentLockTimeoutError. Constant uploads may starve GC, which is periodic and retries next cycle. SET/RESET lock_timeout is gone.
Uploads without cf-connecting-ip now increment multipart_upload_unattributed, so asserting that no metric at all is incremented no longer tests what these cases describe: that the world deployments counter stays untouched on a rejected deploy.
decentraland-bot
requested changes
Sep 28, 2026
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-review found one remaining admission-control bypass. The earlier migration-lock-loss finding is addressed by running the migration SQL and receipt write on the lock-owning session.
Finding
- P1 — Duplicate multipart fields bypass partial-upload byte accounting. The multipart parser stores files in an object keyed by field name, so a later part with the same CID silently replaces the earlier one. By the time
stage()sumsfiles, only the final copy is counted againstMAX_PARTIAL_UPLOAD_BYTES_PER_MINUTEand stored-content reservations, although the server already accepted, parsed, and wrote every duplicate to temporary storage. An authenticated caller can therefore send repeated copies of one valid content CID in each batch and exceed the advertised per-account accepted-byte limit by an arbitrary factor. Reject duplicate file field names during multipart parsing, or preserve/count all parsed part bytes for rate/admission accounting; add a contract/integration test for duplicate CID parts.
The partial=true extension remains additive to POST /entities; existing callers retain their request/response contract. Hosted CI is passing.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
Per-source upload limits and the comms shared-secret rate limit both identify a client by the address the edge proxy reports. That header was hardcoded; it now comes from TRUSTED_CLIENT_IP_HEADER (same name as Catalyst), defaulting to cf-connecting-ip because Worlds always runs behind Cloudflare, which sets CF-Connecting-IP to the connecting client's address. The resolver is a small config-bound component shared by both callers. An invalid header name fails startup. A request without the header keeps today's behaviour: it is not limited per source and is counted in multipart_upload_unattributed, since with Cloudflare in front only internal callers lack it.
A creator hitting a 408 only saw "The multipart upload timed out." or a
millisecond deadline, with nothing actionable to report. Both 408s now
explain themselves in the existing { error, message } body:
- upload deadline: "The upload did not finish within N s: received X
of Y (about Z KiB/s). Retry on a faster connection or send smaller
batches." (Y only when Content-Length was declared)
- post-body processing deadline: "The server could not finish
processing the deployment within N s after receiving it. Retry the
deployment; if it keeps timing out, report it."
Worlds has no minimum receive-rate check, only the whole-body deadline,
so there is no slow-body 408 to reword.
The four partial-upload quotas (uploads per account, staged bytes per account, staged bytes per server, accepted bytes per minute) rejected with 400, so clients treated a temporary condition as a bad request. They now throw a typed PartialUploadQuotaExceededError that the deploy handler maps to 429 with Retry-After; every other 400 stays a 400. Retry-After, always at least 1 s: - bytes per minute: until the account's one-minute window resets - other quotas: until the oldest upload holding that quota expires (the account's for the per-account ones, the server's for the global one; only uploads with reserved bytes count for byte quotas), or 60 s when it has already expired, since only the unscheduled cleanup can free it then Messages state the measured and allowed values.
Replace MAX_CONCURRENT_UPLOADS (40) and MAX_IN_FLIGHT_UPLOAD_FILES (40,000) with byte charges against MAX_IN_FLIGHT_UPLOAD_BYTES: - every request reserves at least MIN_UPLOAD_RESERVATION_BYTES (16 MiB) for fields, parser buffers, its temp directory and socket, so the 4 GiB budget admits 256 small requests; - every file part adds UPLOAD_FILE_OVERHEAD_BYTES (16 KiB) on top of its bytes, so the budget also bounds temp files (262,144). Large requests charge their real size. Startup fails unless the budget fits one maximum-size upload plus its file-count cap times 16 KiB. Rejected-body drains are capped at budget / minimum reservation.
The entity id is the hash of the entity file, so a live entity is exactly what any uploader of that id wanted. A partial batch for a published entity, from any signer and with or without the manifest, now answers 200 with the publication's creationTimestamp instead of 400 "already deployed"; the concurrent-publication (23505) path answers the same way. Completion receipts still answer the original finalizer first, so its replay after an undeploy keeps returning the original result. deployScene now stamps world_scenes.created_at with the timestamp it returns and records in the receipt, taken under the world lock, and the regular deploy's collision path returns that timestamp instead of now.
Lower the default PENDING_DEPLOYMENT_TTL from 24 h to 1 h and move the expired-upload sweep out of the daily eviction job into its own job, partial-upload-cleanup-job, which runs at start and then every PARTIAL_UPLOAD_CLEANUP_INTERVAL_MS (default 5 minutes) after the previous run; the job component stops with the lifecycle. Now that the schedule is known, a 429 whose quota is held only by already-expired uploads answers Retry-After with the time until this replica's next sweep instead of a fixed 60 s. Completion receipts keep their independent 24 h COMPLETED_UPLOAD_TTL.
Operators could see staged bytes and accepted batches, but not how
many uploads start, publish or expire, how long they take, why
batches get a 429, or whether cleanup and GC keep succeeding.
- Lifecycle: partial_uploads_started/completed, partial_uploads_pending
{state}, partial_upload_duration_seconds, and
partial_upload_batches_per_upload (new pending_scenes.batches column,
migration 0029).
- Batches: partial_upload_requests{outcome} and
partial_upload_throttled{reason}.
- Capacity gauges for the staging cap and the multipart budget.
- Cleanup and GC: runs{outcome}, duration, last-success timestamp,
expired uploads and removed keys; content_lock_writer_timeouts.
The parser keyed files by name, so a repeated part replaced the earlier one after every copy had already been written to temp storage. stage() sums only surviving files, so repeating a hash let a caller send bytes never charged against MAX_PARTIAL_UPLOAD_BYTES_PER_MINUTE or the staged byte reservation. Every form name may now appear once, whether file or field. A repeated name is rejected with a 400 as soon as the part's headers arrive, before any of its bytes are written or charged. The settings route opts `categories` in as a repeatable field, since it carries one value per part.
upsert inserted a new upload at its admission instant without checking that admittedAt + PENDING_DEPLOYMENT_TTL was still ahead, and reserve charged the byte rate and staged-byte reservation without checking the upload was live. A first batch delayed by validation or lock waits could create an already-expired row and charge quota that stayed held until cleanup, even though the later store step rejected the batch. upsert now refuses to insert an upload whose lifetime has ended, and reserve checks liveness before the rate charge and again after taking the budget lock, so an expired upload is never charged.
The settings route is the one multipart caller that repeats a field name on purpose, one `categories` part per value. Now that every other repeated name is rejected, pin that the route still accepts and stores all of them.
LautaroPetaccio
commented
Sep 30, 2026
decentraland-bot
approved these changes
Sep 30, 2026
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
Re-review: approved
This round treats the PR description's Assumptions and decisions as settled: single instance behind Cloudflare, process-local limits, hashing before auth bounded by the budgets, try-lock GC deferral, the 1h TTL with a 5-minute sweep, 200 for any batch on a published entity, and the CONCURRENTLY index. Findings that only argue with those decisions were dropped.
Prior findings: all verified fixed
Each "Fixed in " reply was checked against the code at e53f0bc:
- Completion retries: a completion retry that hits a unique conflict maps to the live publication (a68f049).
- Resume manifest: the manifest read back on a resume is validated but never charged or stored again (a68f049).
- Authentication before locks: requests are authenticated before
withRead, and the entity lock is a try-lock with backoff (a68f049, bc09efa). - Rejected first batch:
discardUnadmittedremoves a first batch thatreserve()rejects (bc09efa). - Completion receipts: only a partial finalization writes a receipt, with its own signer, and a regular publication deletes it. Both happen in the publication transaction (70f26f8, dcd5fc0).
- Cleanup: the sweep runs in its own 5-minute job (39b38c8).
- Index migration: the index is built
CONCURRENTLY, and an invalid index is dropped before retrying (d7a32b0). - Lock connection health: a failed operation still unlocks and returns its connection (9d0c036).
- Vanilla TTL: regular deploys validate the TTL against now (70f26f8).
- Migration lock: the lock and the migration SQL run on one pinned session, and
migrations.namehas a unique index (d751298, dd59b7b). - Duplicate multipart names: a repeated name is rejected from its part headers. The only intentional repeat,
categorieson the settings route, is allowlisted (64c7125).
Reservation accounting was also traced end to end. reserved_bytes is recomputed from pending_scene_files, so it can't leak or be released twice. GC and the expiry sweep re-read references under the exclusive lock, so live content can't be deleted.
Consumer impact
POST /entities stays backward compatible for existing deployers (builder and creator-hub via dcl-catalyst-client, per the jarvis graph.yaml inbound edges). Status codes and the { creationTimestamp, message } shape are unchanged. The client sends each content hash once, so duplicate-name rejection doesn't affect it. MAX_CONCURRENT_UPLOADS and MAX_IN_FLIGHT_UPLOAD_FILES are no longer referenced anywhere in the repo.
Remaining items (P2, non-blocking)
- [P2] Docs contradict the 429 behavior.
docs/openapi.yaml:353says "Exceeding a budget returns400", anddocs/partial-deployment-contract.md:20says400covers admission failures. The code returns429withRetry-After.- The contract doc is the spec Catalyst will implement, so fix it before the ADR-325 counterpart is written.
- [P2] Stale rollout section.
docs/partial-deployment-contract.md:45-47still asks operators to quiesce GC, drain old uploads and coordinate replicas. That contradicts the single-instance "normal deploy" rollout. The PR body also lists0026–0028, but0029_pending_scene_batchesships too. - [P2] Uploads larger than the account budget retry forever.
- The account budget is 1 GiB by default, and a reservation charges the whole manifest, including content already in storage (
pending-scenes-manager/component.ts:197-213). - A whitelisted world with a scene limit above 1 GiB can't upload a scene that big partially. It gets a
bytes_per_account429 on every attempt, so the client keeps retrying a deploy that can never succeed. - Answer a permanent 4xx when the scene exceeds the account budget, or cap the effective limit at it.
- The account budget is 1 GiB by default, and a reservation charges the whole manifest, including content already in storage (
- [P2] Permission is checked inside the lock.
validateStaginghashes files, then checks deployment permission, all insidewithRead(deploy-entity-handler.ts:418,validator.ts:137).- Signed but unauthorized requests from a few IPs (4 × 4 per-source) can hold all 16
CONTENT_LOCK_CONNECTIONSwhile hashing. Legitimate deploys and settings updates then wait for connections until they time out with408. - This is within the settled "bounded by budgets" decision, but the lock pool is a much smaller limit than the byte budgets. Consider running the permission check before
withRead.
- [P2] Future timestamps can block the owner.
- The new newer-scene check in
deployScene(worlds-manager.ts:553-561) compares signer-chosen timestamps, which may be up to 15 minutes in the future. - A collaborator deploying at now+15min can repeatedly block the world owner's redeploys on those parcels. The owner recovers only by revoking the collaborator's permission.
- The new newer-scene check in
- [P2] Settings lock wait isn't cancellable.
routes.ts:304callswithReadwithout a signal, so a settings request whose client has disconnected keeps polling for a lock connection. Passctx.request.signal. - [P2] The 0027 index has no query to serve. Nothing filters
world_scenesonupdated_atalone now thatgetReferencedContentKeysdropped the sweep-start filter. Update the comment or drop the index. - [P2] Cleanup.
- Unconsumed
fetchbodies intest/integration/source-upload-limits.spec.ts:43,134and severalpartial-deploy.spec.tscalls. - Duplicate
isUniqueViolation. MAX_ENTITY_FILE_SIZE_BYTESis 5 MiB but commented as 10 MB (deploy-entity-handler.ts:62).createStagingValidateFnsre-runsvalidateSupportedEntityTypeandcreateValidateFileCount.- Unused exports:
PartialUploadQuota,CompletedUpload,getActivePendingKeys.
- Unconsumed
CI: all checks passing.
Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack
A batch larger than MAX_PARTIAL_UPLOAD_BYTES_PER_MINUTE, or an upload whose own reservation exceeds MAX_PENDING_BYTES_PER_DEPLOYER, now fails with PartialUploadTooLargeError (400, no Retry-After) instead of a 429 the client would retry forever. The rate window is still charged first, so repeating rejected batches can't escape the rate limit. Budgets full because of other uploads or traffic keep answering 429 + Retry-After. Startup now rejects a per-minute limit below the maximum request size and a server budget below the per-account budget, which also makes the account check cover the server budget for a single upload.
Quota rejections were documented as 400. A budget full because of other uploads or traffic answers 429 with Retry-After; validation, expiry and batches or uploads that alone exceed a budget answer 400. Replace the stale multi-replica rollout steps: migrations run at startup and Worlds runs as a single instance, so a normal deploy is enough. Note that publication order uses the signer-chosen entity timestamp, which may be up to 15 minutes in the future, bounding how long a collaborator can hold off a redeploy on the same parcels.
…ects The settings route took the shared content lock without the request signal, so a request whose client had gone kept retrying the lock until it was granted and then ran the update anyway. Pass the request signal, as the deploy route does, so the wait ends on disconnect and the handler never runs.
Migration 0027 added a plain updated_at index for garbage collection's deployed-since re-check. That query no longer exists: references are read in one snapshot that never filters on updated_at alone, and the eviction job is served by the UNDEPLOYED partial index. The migration was never released to main, so it is removed rather than reverted.
- Integration tests consume or cancel every fetch response body. - Share one isUniqueViolation from logic/utils. - Use the single 5 MiB MAX_ENTITY_FILE_SIZE_IN_BYTES (a stale alias was commented "10 MB") and merge the two identical entity-file checks; partial batches now get the same message as vanilla deploys. - Drop staging validations already run by commonValidations (validateSupportedEntityType, createValidateFileCount). - Remove the unused getActivePendingKeys and an unneeded export.
POST /entities shared the generic multipart limits of 100 fields of up to 1 MiB each, so one request could hold 100 MiB of form fields in memory before any signature check, while the upload budget only charges it the 16 MiB per-request minimum. A deployment sends entityId, partial and three fields per auth-chain link. Allowing chains of up to 10 links (Catalyst's cap) gives 32 fields; the largest value is an EIP-1654 signature of a few KiB, so 32 KiB per field leaves wide headroom. A request now holds at most 1 MiB of fields. The over-limit messages name the limit, and a value of exactly the field size is accepted (busboy flags one that reaches it as truncated).
Content already in storage when an upload starts used to be charged against the per-account and server staging budgets, so a small edit to a big scene was charged the whole scene. pending_scene_files gets a charged flag (amended into the unreleased 0028 migration). Inventory receipts are uncharged, received and stored files are charged, and a file re-uploaded after markMissing becomes charged. reserved_bytes and the cleanup gauges sum charged bytes only; the scene size limit still counts every file. A batch file already stored, before the upload or by an earlier batch, is dropped without being stored or charged again; it still counts against the byte rate.
A validly signed but unauthorized client could hold one of the CONTENT_LOCK_CONNECTIONS while its upload was hashed and its permission looked up, since everything after the signature check ran inside it. Partial batches carrying the manifest now run their staging validation (hashing, structure, TTL, banned names, permission) before the lock, anchored on a pending row read on the main pool before the replay checks. Resume batches of the signer's own upload pre-hash their files. Regular deployments run validateBeforeStorage first. If the signer's upload seen before the lock is gone under it, staging answers that it expired instead of starting a new upload without a permission check.
Multipart limit violations answered 400 like a malformed body. They now answer 413 Payload Too Large, matching Catalyst: a declared Content-Length or received body over the cap, a file or field value over its size limit, and too many files, fields or parts. Malformed bodies, duplicate names, non-multipart requests and a bad Content-Length stay 400. The parser throws a typed PayloadTooLargeError that a middleware after the shared errorHandler maps to 413, so both POST /entities and the settings PUT get it. Files also get busboy's extra byte, as fields already did, so a file of exactly the per-file limit is accepted.
With batched uploads a DCL-name world scene is no longer bounded by a single request, so its size limit becomes min(MAX_SCENE_SIZE, owner's remaining allowance). MAX_SCENE_SIZE is in MiB, defaults to 500 and must be a positive integer. Whitelisted max_size_in_mb still overrides the cap, ENS names keep ENS_MAX_SIZE and the ownership-bypass fixture keeps MAX_SIZE. Startup fails if MAX_PENDING_BYTES_PER_DEPLOYER cannot stage one capped scene.
This branch has not been deployed
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
3 participants
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.
What
Adds partial (multi-request) scene uploads to
POST /entities, so scenes too large for one request can be deployed in batches. The protocol is specified in ADR-325 and shared with the Catalyst content server.How
partial=truestages files for the entity it carries. The signed entity id is the upload session; there is no create or commit endpoint.202 { missing }until the content set is complete; the completing request runs the full validation, publishes, and answers200.429with aRetry-Afterof the earliest time a retry can succeed, while a batch or upload that alone exceeds a limit answers400. Uploads expire 1h after their first request arrives (PENDING_DEPLOYMENT_TTL), a cleanup job reclaims expired uploads every 5 minutes, nothing is stored or published after that, and they stay charged until their content is deleted.MAX_CONCURRENT_UPLOADS_PER_SOURCEuploads andMAX_IN_FLIGHT_UPLOAD_BYTES_PER_SOURCEdeclared bytes in flight, checked before the body is read (429). Clients are keyed byTRUSTED_CLIENT_IP_HEADER(defaultcf-connecting-ip, since Worlds is always behind Cloudflare); requests without it only count against the process-wide budget.CONTENT_LOCK_CONNECTIONS). A per-entity lock serializes batches of one upload.MIN_UPLOAD_RESERVATION_BYTES) and each file 16 KiB (UPLOAD_FILE_OVERHEAD_BYTES), replacing the fixedMAX_CONCURRENT_UPLOADS/MAX_IN_FLIGHT_UPLOAD_FILEScaps (4 GiB allows 256 small requests or 262,144 files).200with the publication'screationTimestamp.0026,0028and0029createpending_scenes, the accounting and receipt tables, and the batch counter.MAX_SCENE_SIZE(500 MiB by default) or the owner's remaining storage allowance, whichever is smaller; whitelisted worlds keep their explicit size and ENS names stay at 36 MB. Scenes over the 350 MiB single-request limit deploy through partial uploads.Rollout
Worlds runs as a single instance, so this is a normal deploy. Drop
MAX_CONCURRENT_UPLOADSandMAX_IN_FLIGHT_UPLOAD_FILESfrom deploy configs if set. Limits are in.env.default; details indocs/partial-deployment-contract.md.Testing
Unit and integration suites, including the shared HTTP contract suite (
test/contracts/partial-deployment.ts) and new accounting and locking tests intest/integration/partial-upload-accounting.spec.ts.Assumptions and decisions
These are accepted for this PR; reviews should treat them as settled.
cf-connecting-ip; requests without it are internal callers, bounded by the process-wide upload budget.CREATE INDEX CONCURRENTLYmigration forworld_scenes(updated_at)is not a rollout concern.