fix: poll the rootCID that was actually deployed after a catalyst retry - #1496
Conversation
Publishing to LAND could hang in "Publishing..." for an hour without ever
showing success or an error, even though the scene was live in-world.
When a /deploy attempt fails, the deploy thunk picks another catalyst and
respawns the CLI deploy server, which rebuilds the scene. The rebuild is not
byte-identical (autogenerated thumbnail, build outputs), so it produces a new
rootCID. The thunk tracked that new info internally but returned deployFn's
result, which is void, so it never reported which entity actually landed.
executeDeployment destructured `info` from the deployment *before* dispatching
the deploy, and its fetchStatus closure captured that stale value. After a
successful retry it therefore polled the asset-bundle-registry for the
first attempt's rootCID -- an entity that was never deployed anywhere -- and
got a 404 forever.
checkDeploymentStatus treats a throwing fetchStatus as a soft retry, leaving
componentsStatus at all-idle. deriveOverallStatus of all-idle is 'idle', never
'failed', so the cancel path never triggered and it burned all 360 retries
(60 minutes) before surfacing MAX_RETRIES.
The deploy thunk now returns { info: currentInfo } so callers can poll the
entity that actually deployed, and executeDeployment consumes it instead of
the pre-deploy capture. `info` is dropped from the earlier destructure so the
stale value is unreachable.
Also closes a fall-through at the end of the retry loop: exiting the while
without deploying returned undefined and *fulfilled* the thunk, sending the
caller off to poll a deployment that never happened. It now rejects with
CATALYST_SERVERS_EXHAUSTED.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review Summary
Excellent bug fix — well-analyzed root cause, minimal and targeted changes, and genuine regression tests that verify the fix.
What this fixes
When a catalyst /deploy attempt fails and retries against a different catalyst, the CLI server rebuilds the scene, producing a new rootCID. The old code had two problems:
- Stale closure in
executeDeployment:infowas destructured from the deployment before dispatchingdeploy, so thefetchStatusclosure captured the pre-retry rootCID. After a successful retry, the poller queried the asset-bundle-registry for an entity that was never deployed, getting 404s forever. - Void return from
deploythunk:deployFnreturnsvoid, so the thunk couldn't communicate which entity actually landed.
Analysis of the fix
deploy thunk — now returns { info: currentInfo } on success instead of deployFn's void result. This is the right seam: the thunk is the only place that tracks which catalyst attempt succeeded via its local currentInfo variable, which is updated on each retry via updateDeploymentTarget.
executeDeployment thunk — drops info from the pre-deploy destructure (eliminating the stale binding entirely) and destructures it from the deploy result instead. The fetchStatus closure now captures the correct post-deploy info.
Defensive rejectWithValue after while-loop — unreachable in practice (the loop always exits via return or rejectWithValue inside the catch), but prevents the function from silently fulfilling with undefined if the control flow ever changes. Good defensive coding.
Correctness verification
- ✅
currentInfotracks the correct info throughout the retry loop — starts asdeployment.info, updated toresult.infoafter eachupdateDeploymentTarget - ✅ The returned
{ info }is consumed correctly byexecuteDeploymentfor both thefetchStatusclosure and the fulfilled payload - ✅ The
executeDeployment.fulfilledreducer usesaction.payload.infoto updatedeployment.infoin state — now receives the correct post-deploy info - ✅ The
deploythunk is internal (not exported inactions), so no external callers are affected by the return type change - ✅ If
deployrejects,.unwrap()throws before the destructure, and the catch block handles it correctly - ✅ Redux state is also updated via
updateDeploymentTarget.fulfilledreducer during retries — no conflict with the returned info
Test quality
- ✅ Two new targeted tests that directly verify the fix:
- Extracts the
fetchStatuscallback passed tocheckDeploymentStatusand verifies it callsfetchDeploymentStatuswith the rebuilt rootCID - Verifies the returned
infohas the rebuilt rootCID
- Extracts the
- ✅ Tests are genuine regression tests — the PR description confirms they fail when the bug is reintroduced
- ✅
fetchDeploymentStatusmock correctly added to both thevi.mockblock and the import - ✅
REBUILT_SCENE_INFOis clearly distinguished fromTEST_SCENE_INFOvia a differentrootCID - ✅ Follows existing test patterns (fake timers,
advanceRetryTimers,mockDeployWithRetryOnce)
Security
No security issues found. The changes are purely internal control-flow and data-flow fixes — no new inputs, no auth changes, no secrets, no dependency changes.
Git conventions
- ✅ PR title follows semantic commit format:
fix: poll the rootCID that was actually deployed after a catalyst retry
CI Status
CI checks (lint, typecheck, unit tests) are still running at time of review.
No P0, P1, or P2 issues found. Clean, well-reasoned fix.
Reviewed by Jarvis 🤖 · Requested by RocioCM via GitHub
Test this pull request on windows-latestDownload the correct version for your architecture: |
Test this pull request on macos-latestDownload the correct version for your architecture:Click here if you don't know which version to downloadFor running this unsigned version of the app, you will need to run the xattr command on it:
|
3 participants
Problem
Publishing a scene to LAND could sit in "Publishing..." indefinitely — the upload step never ticked over to success, and no error was ever shown. The scene was actually live in-world the whole time.
Repro: publish to LAND when the first catalyst rejects the upload. In the observed case the initial
/deployreturned400 { message: "Could not upload content: <html>...404 Not Found..." }frompeer.decentral.io, then again frompeer-ap1.decentraland.org, and finally succeeded onpeer.uadevops.com.Cause
When a
/deployattempt fails, thedeploythunk selects another catalyst and dispatchesupdateDeploymentTarget, which respawns the CLI deploy server and rebuilds the scene. That rebuild is not byte-identical (autogenerated thumbnail, build outputs), so each retry yields a different rootCID:bafkreib3uzdc…vfjqzybafkreidveasa…ov7jubafkreigie7tx…kliryThe thunk tracked the new info internally via
currentInfo, but returneddeployFn's result — which isvoid— so it never reported which entity actually landed.Meanwhile
executeDeploymentdestructuredinfofrom the deployment before dispatching the deploy, and itsfetchStatusclosure captured that stale value. After the successful third attempt it polled the asset-bundle-registry for the attempt-1 rootCID — an entity never deployed anywhere — and got a 404 forever.Why it hung rather than failing:
checkDeploymentStatustreats a throwingfetchStatusas a soft retry, socomponentsStatusstayed at all-idle.deriveOverallStatusof all-idle is'idle', never'failed', soisCancellednever flipped and the abort path never fired. It burned all 360 retries × 10s — a full hour — before surfacingMAX_RETRIES.Fix
deployreturns{ info: currentInfo }on success, so callers know which entity actually deployed.executeDeploymentconsumes that returnedinfoforfetchStatusand the fulfilled payload.infois dropped from the pre-deploy destructure so the stale value is unreachable rather than merely unused.whilewithout deploying returnedundefinedand fulfilled the thunk, sending the caller off to poll a deployment that never happened. It now rejects withCATALYST_SERVERS_EXHAUSTED.The deploy thunk is the right seam — it is the only place that knows which catalyst attempt succeeded, and it already tracked the correct info; it just wasn't handing it back.
Tests
Added a
when a retry rebuilds the scene under a new rootCIDblock with two cases: one asserting the poller queries the deployed rootCID, one asserting the returnedinfois the rebuilt one.The existing retry tests could not catch this —
fetchInfowas mocked to return the same info on every call, so the CID never changed and the stale capture was indistinguishable from the correct one.Verified these are genuine regression tests by reintroducing the bug: both fail with
expected 'QmTest123' to be 'QmRebuilt456', then pass once reverted.Full creator-hub unit suite passes (185/185: main 28, preload 17, renderer 127, shared 13). Typecheck and ESLint clean.
Not addressed here: the upstream reason catalysts return
404 Not Foundon upload is server-side and outside this repo. This fix means a successful retry now completes correctly instead of hanging. Separately worth considering — a prolonged registry 404 still consumes the full 60-minute retry window before surfacing; escalating earlier would be a reasonable follow-up.🤖 Generated with Claude Code