feat: deploy v2 partial/resumable protocol (refresh of #459) - #486
feat: deploy v2 partial/resumable protocol (refresh of #459)#486pentreathm wants to merge 28 commits into
Conversation
…ward request opts to fetch
…est determinism, accept 2xx on upload)
Test this pull request
|
|
coverage: 77.949% (+7.8%) from 70.191% — feat/deploy-v2-refresh into master |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #486
feat: deploy v2 partial/resumable protocol (refresh of #459)
28 files changed (+1,690 −9) · CI all green (build, PR title, coveralls)
Verdict: Approve
Solid, well-structured implementation. The protocol design is sound, the v1 path is preserved byte-for-byte, backward compatibility is maintained via capability probing, and test coverage is comprehensive (109 tests across unit + integration). The error hierarchy is clean, the bounded concurrency via p-limit is correct, and the Uint8Array slice handling (byteOffset/byteLength) avoids the buffer-view bugs from #459.
No P0 or P1 issues found. Five P2 observations below.
P2 — Minor Observations
1. Probe cache caches transient failures permanently (src/client/protocol.ts:50-55)
createProbeCache stores the probe Promise with no TTL. If the first probe fails due to a transient network error, the server is permanently marked v1-only for the ContentClient's lifetime. For CLI/browser consumers this is fine (short-lived), but long-lived server processes (e.g. builder-server, linker-server) would never re-probe.
Suggestion: Consider caching negative results with a short TTL (60–120s) while caching positive results indefinitely. Not blocking for the initial Phase 1 rollout, but worth addressing before long-lived consumers adopt v22.
2. DeploymentOptions / DeploymentProgress / DeploymentProtocolVersion not exported from package root (src/index.ts)
The new types in src/client/types.ts aren't re-exported from src/index.ts. Consumers can infer DeploymentOptions from the deploy() signature, but can't import { DeploymentOptions } from 'dcl-catalyst-client' to type variables or wrapper functions. The error classes are correctly exported (export * from './client/errors'), but the option/progress types are missing.
Suggestion: Add export * from './client/types' or selectively re-export the new types. Note that DeploymentData is already re-exported via ./client/utils.
3. No validation of InitResult response shape (src/client/deploy-v2.ts:83)
initDeployment casts the server response as InitResult without runtime validation. If a server returns an unexpected shape (e.g. missingFiles is missing or not an array), the subsequent for...of loop would throw an untyped error rather than a DeploymentInitError.
Suggestion: A lightweight guard (check Array.isArray(result.missingFiles) and typeof result.deploymentToken === 'string') would give a clear DeploymentInitError instead of a confusing TypeError.
4. Deep import path for Response type (src/client/deploy-v2.ts:7)
import type { Response } from '@well-known-components/interfaces/dist/components/fetcher' reaches into the package's dist/ directory. If that package restructures its output, this import breaks.
Suggestion: Check if Response is available from the package root or a public subpath export. If not, a local type alias (type FetchResponse = Awaited<ReturnType<IFetchComponent['fetch']>>) is safer.
5. fileSizesManifest includes the entity file (src/client/deploy-v2.ts:35-37)
The manifest loop iterates over all entries in data.files including the entity file. If the server uses fileSizesManifest to track only content files (not the entity descriptor), this could cause a mismatch. Verify the server-side contract expects the entity hash in the manifest.
Consumer Impact
~30 downstream consumers found (marketplace, builder, creator-hub, js-sdk-toolchain, auth, linker-server, etc.). The API change is backward-compatible:
deploy()signature widened fromRequestOptionstoRequestOptions | DeploymentOptions(additive)- Default behavior unchanged (
'auto'probes, falls back to v1) - New exports are additive (error classes)
- Major version bump (22.0.0-rc.0) is appropriate and gives consumers explicit opt-in
No consumer will break at runtime.
Security
deploymentTokenis correctly scoped as a session correlator (not a security boundary), passed only in headers, never logged- Content addressing makes per-file signing redundant — the entity's signed hash list commits to all file hashes
- Protocol downgrade (MITM intercepting OPTIONS → 404) falls back to v1, which is the current production path — no security regression
- No hardcoded secrets, no injection vectors in URL construction (entity IDs and file hashes are content-addressed CIDs)
Test Coverage
Excellent. Unit tests cover each module in isolation (init, upload, finalize, orchestrator, protocol, retry, errors, probe, types). Integration tests use an in-process HTTP mock server covering: happy path, auto-fallback to legacy server, eviction recovery, intermittent 5xx with retries, and permanent 4xx propagation. The existing E2E test is correctly updated to account for the new OPTIONS probe call.
Reviewed by Jarvis 🤖 · Requested by Matias Pentreath via Slack
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review — PR #486
feat: deploy v2 partial/resumable protocol (refresh of #459)
28 files changed (+1,690 −9) · CI all green (build, PR title, coveralls — coverage +7.8% to 77.9%)
Verdict: Request Changes
Excellent implementation overall. The protocol design is sound, the v1 path is preserved byte-for-byte, backward compatibility is correct, concurrency model is well-structured (p-limit + early-exit flags), error hierarchy is clean, and test coverage is comprehensive (109 tests across unit + integration + E2E). The Uint8Array slice handling (byteOffset/byteLength) correctly addresses the buffer-view bugs from #459.
One P1 blocks merge (missing type exports from the package surface — easy fix). Eight P2 observations below.
P1 — Must Fix Before Merge
1. New public types not exported from package root (src/index.ts)
DeploymentOptions, DeploymentProgress, and DeploymentProtocolVersion are defined in src/client/types.ts but not re-exported from src/index.ts. On a 22.0.0 major release with ~30 downstream consumers, this means callers can't do import { DeploymentOptions } from 'dcl-catalyst-client' to type their wrapper functions, progress callbacks, or option objects.
The error classes are correctly exported via export * from './client/errors'. The types need the same treatment.
Fix: Add export type { DeploymentOptions, DeploymentProgress, DeploymentProtocolVersion } from './client/types' to src/index.ts.
P2 — Minor Observations
2. Probe cache caches negative results permanently (src/client/protocol.ts:50-55)
createProbeCache stores the probe Promise with no TTL. A transient network error on the first probe permanently marks the server as v1-only for the ContentClient's lifetime. For short-lived CLI/browser consumers this is fine, but long-lived server processes (builder-server, linker-server) would never re-probe. A transient MITM during a deployment window could also permanently poison the cache.
Suggestion: Cache negative results with a short TTL (60–120s); cache positives indefinitely. Not blocking for Phase 1, but worth addressing before long-lived consumers adopt v22.
3. No runtime validation of InitResult response shape (src/client/deploy-v2.ts:83)
initDeployment casts the server response as InitResult without guards. If a server returns an unexpected shape (e.g. missingFiles absent or not an array), the for...of loop throws an untyped TypeError instead of a DeploymentInitError.
Suggestion: Guard with Array.isArray(result.missingFiles) && typeof result.deploymentToken === 'string'.
4. Deep dist/ import path for Response type (src/client/deploy-v2.ts:7)
import type { Response } from '@well-known-components/interfaces/dist/components/fetcher' reaches into the package's dist/ directory. If that package restructures output, this breaks silently.
Suggestion: Use a derived type: type FetchResponse = Awaited<ReturnType<IFetchComponent['fetch']>>.
5. fileSizesManifest includes the entity file (src/client/deploy-v2.ts:35-37)
The manifest loop iterates all data.files entries including the entity descriptor. Verify the server-side contract (companion plan C) expects the entity hash in fileSizesManifest.
6. expiresAt and availableFiles received but unused (src/client/deploy-v2.ts)
Both fields are in InitResult and returned from the server, but never read by client code. deployV2 drives uploads from init.missingFiles only. If these are intentionally deferred (e.g., for a future "warn before expiry" feature), document that. Otherwise, consider omitting them from the client type to avoid confusion.
7. probe-entity-id.ts — YAGNI stub
An 18-line module with a test file, returning a hardcoded constant. The _serverUrl and _fetcher params are unused. This is an explicit extension point for future probe-id resolution, but for now it could be a constant in protocol.ts. Not blocking, but adds surface area.
8. Error cause set manually via (this as any).cause (src/client/errors.ts)
All three error constructors that accept cause assign it via (this as any).cause instead of super(message, { cause }). If the target supports ES2022 ErrorOptions, the native approach is cleaner and avoids the as any cast. If targeting older runtimes, this is the correct pattern — document the reason.
9. Duplicated error-body reading pattern
initDeployment, uploadFile, and finalizeDeployment each have a near-identical try { body = await resp.text() } catch { } block. A small safeReadText(resp) helper would reduce duplication.
Consumer Impact
~30 downstream consumers (marketplace, builder, creator-hub, js-sdk-toolchain, auth, linker-server, etc.). The API change is backward-compatible:
deploy()signature widened (additive optional fields)- Default behavior unchanged (
'auto'probes, falls back to v1) - New exports are additive
- Major version bump (22.0.0-rc.0) gives consumers explicit opt-in
No consumer will break at runtime.
Security
- No hardcoded secrets, no credentials in source
deploymentTokenis correctly scoped as a session correlator, passed only in headers, never logged- Content addressing makes per-file signing redundant — the entity's signed hash list commits to all file hashes
- Protocol downgrade (MITM intercepting OPTIONS → 404) falls back to v1, which is the current production path — no security regression
- URL construction uses content-addressed hashes (CIDs) that the caller provides, not external user input — low injection risk
p-limit@^3.1.0has no known CVEs; the^3pin correctly avoids ESM-only v4+
Git Conventions (ADR-6)
- PR title
feat: deploy v2 partial/resumable protocol (refresh of #459)✅ - Branch name
feat/deploy-v2-refresh✅
Test Coverage
Excellent. Unit tests cover each module in isolation (init, upload, finalize, orchestrator, protocol, retry, errors, probe, types). Integration tests use an in-process HTTP mock server covering: happy path, auto-fallback, eviction recovery, intermittent 5xx retries, permanent 4xx propagation. E2E test updated for the new OPTIONS probe call.
Reviewed by Jarvis 🤖 · Requested by Matias Pentreath via Slack
| export * from './client/CatalystClient' | ||
| export * from './client/ContentClient' | ||
| export * from './client/errors' | ||
| export * from './client/LambdasClient' |
There was a problem hiding this comment.
[P1] DeploymentOptions, DeploymentProgress, and DeploymentProtocolVersion from ./client/types are not re-exported here. Downstream consumers can't import these types from the package root.
Suggested fix:
export type { DeploymentOptions, DeploymentProgress, DeploymentProtocolVersion } from './client/types'| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[P2] The probe cache stores the Promise<boolean> with no TTL. A transient network error on first probe permanently marks the server as v1-only for the client's lifetime. For long-lived server processes (builder-server, linker-server), consider caching negative results with a short TTL (~60-120s) while caching positives indefinitely.
| bytes: Uint8Array | ||
| deploymentToken: string | ||
| } | ||
|
|
There was a problem hiding this comment.
[P2] The server response is cast to InitResult without runtime validation. If missingFiles is missing or not an array, the subsequent for...of will throw an untyped TypeError rather than a DeploymentInitError.
Suggested guard:
const parsed = await resp.json()
if (!Array.isArray(parsed.missingFiles) || typeof parsed.deploymentToken !== 'string') {
throw new DeploymentInitError(`Invalid init response: missing required fields`)
}
return parsed as InitResult| import { retryUpload } from './retry-upload' | ||
| import pLimit from 'p-limit' | ||
| import { DeploymentInitError, FileUploadError, FinalizeError } from './errors' | ||
| import type { Response } from '@well-known-components/interfaces/dist/components/fetcher' |
There was a problem hiding this comment.
[P2] Deep import reaching into the package's dist/ directory. This breaks if the package restructures its output.
Safer alternative:
type FetchResponse = Awaited<ReturnType<IFetchComponent['fetch']>>3 participants
Summary
dcl-catalyst-client. v1 codepath is preserved byte-for-byte for the ~30 downstream consumers; v2 activates on servers that advertise it via anOPTIONScapability probe.POST /entities/:idrather thanPUT.catalyst-api-specsOpenAPI additions, C =worlds-content-serverserver implementation, D =js-sdk-toolchainintegration.Protocol shape
Capability-based, no
/v2/URL namespace:POST /entities(Upload-Incomplete: ?1){availableFiles, missingFiles, deploymentToken, expiresAt}POST /entities/:entityId/files/:fileHashPOST /entities/:entityIdOPTIONS /entities/:id/statusSpec: design doc lives in
docs/superpowers/specs/2026-05-05-partial-resumable-deploy-protocol-design.mdin the workspace.Public API additions
client.deploy()returns the sameResponse-like shape on both v1 and v2 — no breaking change to consumers.Backward compatibility
ProtocolUnsupportedErrorBumped to
22.0.0-rc.0in package.json. Final tag will be picked by semantic-release on merge.Why this differs from #459
#459 had the right protocol shape but several bugs and design issues that came out in audit:
supportsDeploymentsV2didOPTIONSagainst a literal:entityId/:fileHashroute placeholder — replaced with proper probe + per-server cachePromise.allover file uploads (no concurrency cap, no retries) — replaced withp-limitbounded concurrency + retry-with-backoff helperfetch()calls bypassing the injected fetcher — all I/O now via the injectedIFetchComponentPUTwithbody: ''— finalize is nowPOST /entities/:idwith no body, matching the original shape docDeploymentInitError,FileUploadError,FinalizeError,ProtocolUnsupportedError)A senior code review caught 8 additional items addressed in this branch (data-correctness on
Uint8Arrayviews, options forwarding to the fetcher, v1 path field leakage, probe timeout, etc.).Test plan
resumeOnEviction: falseFileUploadError,deploymentTokenpropagationworlds-content-serverfeat/deploy-v2-refreshbranch (companion plan C, in flight) once availableMIGRATION.mdandDEPLOYMENT.mddescribe the new protocol and migration path for ~30 downstream consumers109/109 tests pass on CI-equivalent local run.
Out of scope here (tracked elsewhere)