feat: serve asset bundles in preview through an abgen sidecar - #1498
feat: serve asset bundles in preview through an abgen sidecar#1498eordano wants to merge 33 commits into
Conversation
Deploying js-sdk-toolchain with
|
| Latest commit: |
b197017
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://03d467f4.js-sdk-toolchain.pages.dev |
| Branch Preview URL: | https://feat-abgen-preview.js-sdk-toolchain.pages.dev |
Test this pull request
|
`sdk-commands start --asset-bundles` runs an abgen sidecar: an ab-cdn-compatible server that JIT-converts the scene into Unity asset bundles, reading it through the preview's own /content endpoints. The CLI prints the sidecar URL and adds `optimized-assets-url` to the Desktop Explorer deep link. Off by default. The sidecar binary resolves from ABGEN_BIN, then a cached copy of the pinned release (v0.11.3), then abgen on the PATH; only when none exist is the release downloaded (sha256-verified). When everything fails the preview keeps working with a warning. Releases extract into a staging dir and publish via atomic rename so concurrent resolvers never race. Converted bundles land in .dcl-optimized-assets/ next to scene.json, durable across preview restarts and inside the default dcl-ignore. The preview content server gets base64url id helpers (encode + decode, with round-trip tests) so its ids stay safe inside URL paths and asset-bundle directory names. exec() learns shell-less spawning with single-settlement error handling, so a missing binary fails fast instead of hanging the promise. Also fixes all outstanding lint findings across packages and inverted negative assertions in explorer-alpha.spec.ts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
707a353 to
8a4ebd0
Compare
…for remote entities, host-platform builds, local-ab deeplink flag Four defaults tuned from end-to-end testing against a client with local-scene bundle support (unity-explorer#9459) and the Creator Hub toggle (creator-hub#1396); every one remains overridable via env: - Prefer port 5147 (abgen's default and the Explorer's default optimized-assets-url): a connected Unity Editor finds the sidecar with no configuration; scans upward when taken. - Remote entities (wearables/emotes) stream prebuilt from the production ab-cdn (ABGEN_UPSTREAM_AB_CDN) instead of being converted locally per scene: the worlds content fallback is set to 'off', so only the previewed scene is ever built and .dcl-optimized-assets contains only scene artifacts. NOTE: needs the abgen release carrying decentraland/abgen#20 (upstream lane); with the current v0.11.3 pin, wearable requests would 404 through the sidecar. - Eager registry builds restricted to the host platform (ABGEN_INDEX_BUILD_PLATFORMS): a local sidecar never serves the other platform, halving wearable/emote index work. - Deeplink gains local-ab=true next to optimized-assets-url: the flag that makes the scene itself load bundles in local-scene mode (unity-explorer#9459); the url alone only re-bases wearable/registry traffic.
…les) sdk-commands now owns the abgen sidecar (decentraland/js-sdk-toolchain#1498): it resolves/downloads the binary, boots it against its own preview server, and injects local-ab + optimized-assets-url into the deeplink it fires. The hub's job shrinks to mapping the "Optimized Assets" toggle to the opt-in --asset-bundles flag (feature-detected in the scene's installed sdk-commands, skipped with a warning when unsupported). Deleted: the hub-owned sidecar (abgen.ts), the <userData>/abgen binary location, the preview-port pre-picking, and the deeplink re-fire after capture. Option changes on a running preview now only flip the local-ab param (and strip the url when toggled off). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Carries decentraland/abgen#20: content revalidation (auto-on for loopback content servers), upstream ab-cdn read-through, and bounded storage names (fixes silently dropped assets on macOS creator-hub scene paths, which overflowed NAME_MAX with v0.11.3). Completes the CDN-passthrough default from the previous commit.
The sidecar writes into .dcl-optimized-assets on every manifest request (content revalidation), and chokidar tests absolute paths against the ignore patterns, so the bare '.*' entry never prunes the directory. A watched write there loops forever: reload → manifest request → revalidation write → SCENE_UPDATE → reload. Reproduced on a 70-parcel scene with a connected client; zero reload messages after the fix.
The sidecar-cache test replaced the existing *.map coverage instead of extending it; this brings back the original three tests alongside it.
The explorer re-bases its ab-cdn registry POSTs (/entities/active) to the sidecar, which resolves the wearable/emote pointers through the preview server (it proxies unknown pointers to a real catalyst) and then eagerly tries to JIT-build each one — but the preview server only serves the local scene's files, so every build fails with a write-back 404. Those entities stream prebuilt from the production CDN via the upstream read-through, so the eager lane can never do useful work here; turn it off. The previewed scene still converts on its manifest request.
Brings the upstream-first dispatch (no doomed JIT probes for remote entities) and the asset-bundle registry pass-through (registry records answer with the versions the production CDN actually serves, so wearables and emotes load instead of 404ing on fabricated URLs).
A repo-wide prettier run swept pre-existing formatting drift on main into the base commit; restored to main's state so the diff is only the feature.
First-time conversion of a large scene could outlive the explorer's manifest request timeout, degrading the first preview to raw GLTFs until a re-run. The sidecar holds a manifest request open until the conversion finishes, so start now awaits one — with a progress heartbeat — before printing the deeplink: the explorer's own manifest fetch is always a cache hit. Conversions persist in .dcl-optimized-assets, so only the first run of a scene pays the wait (Genesis Plaza central-plaza: 91s cold, 3s warm).
The sidecar's per-file ABGEN_BUILD telemetry and INFO tracing drowned the progress lines: the sidecar now runs silent with warn-level logging by default, and the heartbeat prints through the styled progress logger. Exporting RUST_LOG restores the full sidecar output.
… sidecar dies A single held manifest request can die a transport death minutes into a long conversion, so the pre-warm now retries until its deadline — the conversion keeps running server-side and a retried request attaches to the in-flight build or hits the finished cache. If the sidecar itself is gone (process exited or readyz unreachable) retrying is pointless: warn and let the explorer take the raw-GLTF degrade instead of burning the full 15-minute budget against a dead port.
silent: true left the child's stdout/stderr pipes unread, so a chatty child (the abgen sidecar prints a telemetry line per converted file) filled the ~64KB pipe buffer and blocked on its next write — the conversion wedged at 0% CPU with readyz still answering. Resume the streams so silent output is discarded instead of accumulating.
Polls the sidecar's /progress/{entity} route and prints
'[n/total] converting <file>' instead of a blind elapsed-time
heartbeat; sidecars without the route fall back to the old heartbeat.
runExplorerAlpha opened the client unconditionally, so a wrapper that spawns start --no-browser to pre-convert in the background (Creator Hub's warmup) had the explorer pop open on its own the moment conversion finished. The deeplink line still prints — wrappers rely on capturing it.
|
Pushed the first-load-timeout work discussed in review, plus fixes it surfaced:
|
…browser gate) Creator Hub always captures the printed deeplink, adjusts it, and fires it itself — the sdk self-launch under --hub was at best a double open with unadjusted params, at worst an explorer popping up during a background conversion. Gating on --hub keeps --no-browser meaning what it says (the web browser) and needs no new flag.
Brings the /progress/{entity} route, so the pre-conversion progress
lines ([n/total] converting <file>) now come from the pinned release
rather than only from ABGEN_BIN builds.
Lookups are exact on the pinned version, so old release dirs are dead weight (~35MB each) that accumulated forever; they are removed once the pinned one resolves. Staging dirs are left alone (a concurrent resolver may be mid-extract), and pruning failures never affect the resolve.
The pruning spec hardcoded the darwin-arm64 dist as the kept entry, so on CI's linux-x64 it counted as superseded and the assertion failed.
Brings the precise xn- prune: hot-reload edits reconvert only the changed assets instead of sweeping every collapsed-name entry (half the corpus on scenes with deep paths).
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Code Review: feat: serve asset bundles in preview through an abgen sidecar
Verdict: Approve — No P0 or P1 issues found. Well-constructed PR with strong security posture and thorough error handling.
Summary
This PR adds an opt-in --asset-bundles flag to sdk-commands start that spawns an abgen sidecar for JIT asset-bundle conversion during preview. The implementation is clean: sha256-verified binary downloads, shell: false execution, localhost-only binding, atomic staging for concurrent safety, graceful degradation at every failure point, and a prewarm step that eliminates first-load timeouts.
CI Status
All 8 checks pass (lint, build, test, docs, CLI E2E, deployment notification, dev dep check, Cloudflare Pages).
Security Review
No security issues found. Positive highlights:
- sha256 pinning against hardcoded per-platform hashes before extraction
shell: falsefor all new spawns — eliminates shell injection; args array is empty[]- tar extraction without
-P— both GNU and BSD tar refuse absolute/..paths by default 127.0.0.1binding — sidecar is not network-reachable- Path traversal guard on
/content/contents/:hashviapath.resolve()+startsWith(project.workingDirectory) - No secrets or credentials in any changed file
Consumer Impact
The base64url encoding switch affects content IDs served by the preview server. Since the same process encodes and decodes, the preview server is self-consistent. External consumers (explorer clients) need to accept -/_ in b64- hashes — companion PRs are documented and in progress:
- bevy-explorer: decentraland/bevy-explorer#995
- hammurabi-headless: branch
fix/accept-base64url-content-hashespushed (PR pending)
b64HashingFunction (standard base64) is intentionally retained — it's still used for analytics projectHash in 5 commands (start, deploy, build, pack-smart-wearable, export-static).
Git Conventions
PR title (feat: serve asset bundles in preview through an abgen sidecar) and branch name (feat/abgen-preview) follow ADR-6 semantic commit format.
Findings (P2 only — none blocking)
[P2] Heartbeat async IIFE missing .catch() — asset-bundles.ts:129
The void (async () => { ... })() inside setInterval suppresses the lint rule but doesn't handle errors. While fetchBuildProgress has internal try/catch, the printProgressStep/printProgressInfo calls are unguarded. If either threw, it would become an unhandled rejection. Consider appending .catch(() => {}).
[P2] close event code typed as number instead of number | null — exec.ts:59
Node.js close event signature is (code: number | null, signal: string | null). When a child is killed by signal, code is null, producing the message "exited with code null". The rejection behavior is correct, just the message is confusing. Pre-existing pattern made more relevant by the new shell: false codepath.
[P2] Test coverage gaps in error/recovery paths
The new tests establish good baseline coverage for the resolve-order, sidecar wiring, and b64url round-trip. Paths with missing coverage (all non-blocking — the code itself is correct):
- exec.ts double-settlement guard (no test fires both
error+close) downloadReleasehappy path (sha256-matched archive → extract → rename)- Prewarm retry loop (transport failure, sidecar death, timeout, non-zero exitCode)
asset-bundles.spec.ts:74— weakexpect(logger.warn).toHaveBeenCalled()assertion would pass from the exec.catchwarn alone
[P2] ABGEN_BIN env var bypasses sha256 verification — abgen-binary.ts:61
By design for developer override. The threat model (local workstation) makes this acceptable. A log line when the override is active would be a nice-to-have.
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
Signed-off-by: Esteban Ordano <42750+eordano@users.noreply.github.com>
ReviewReviewed together with decentraland/creator-hub#1396. This half is in good shape: pinned + sha256-verified download (checked before anything touches disk, with a test asserting no write/extract on mismatch), staged extract with atomic rename and loser-detection, shell-less spawn, loopback-only bind. The P1 — old hub + new sdk: first Preview press opens nothingThe (The mirror case — new hub + old sdk double-launch — is flagged on the hub PR.) P2 — orphaned sidecar on ready-timeoutIf P3 —
|
…he --no-browser gate)" This reverts commit c7f7e5c. Product call: Creator Hub drops the background (toggle-time) asset conversion — conversion now runs inline with the Preview press, so there is no longer a deeplink to hold and the hub no longer fires it on fresh spawns. The sdk self-opening the client at the end of start is again the single launch path on every version, which also removes the version-skew double-open between old/new sdk-commands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This reverts commit 257aca2. The gate existed solely so Creator Hub's background warmup could pre-convert with the client held closed; the hub dropped that flow (conversion now runs inline with the Preview press) and no longer passes --no-browser to start. Dropping the gate returns runApp's launch path to exactly main's behavior — the PR's delta is now just the sidecar — and --no-browser goes back to meaning what it documents (the web browser). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nicoecheza
left a comment
nicoecheza
left a comment
There was a problem hiding this comment.
Re-reviewed at b402ade2.
The two reverts (4d270edf, b402ade2) resolve the version-skew P1: runApp self-opens the client unconditionally again, so this PR no longer changes who launches the explorer for any hub or non-hub session. Combined with creator-hub#1396 dropping its fresh-spawn dclDeepLink fire, the launch contract is now exactly what shipped before — no double-launch, no first-press-opens-nothing, in either direction of version skew. That was the only blocker.
Everything else I checked before still holds up: sha256 verified before anything touches disk, staged extract + atomic rename, shell-less spawn, loopback bind, the exec() pipe-drain and settled fixes, and base64url decode staying back-compatible with existing b64- hashes.
The three follow-ups from my earlier pass are unchanged and still worth doing at some point, none blocking:
- Orphaned sidecar on ready-timeout — after the 15s
/readyzdeadline,runAssetBundlesSidecarwarns and returnsundefined, but the spawned abgen keeps converting and holding its port until CLI exit, sinceexec()hands back no kill handle. A slow first boot (Windows Defender scanning a freshly downloaded binary) is the realistic trigger. pruneStaleReleasesvs concurrent versions — two scenes on different sdk-commands pins previewing at once will each prune the other's release dir, pullingtemplate//shader/out from under a running sidecar. An mtime guard would cover it.- Log-format coupling — Creator Hub parses
printProgressStep's[n/total] <log>shape and the literalstill converting... (Ns)string. A comment at those call sites would stop an innocent reformat from silently degrading hub progress.
Approving.
… lane
Explorer requests v49+ scene bundles by their digest-bearing file name under
the CDN's shared {version}/assets/ prefix (unity-explorer#9442). abgen serves
those files through its flat /assets/{file} lane (bundle-index lookup) but has
no version-prefixed route, so the proxy strips the version segment on the way
through. Drop once abgen serves GET /{version}/assets/{file} natively.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…IT lane
The flat /assets/{file} lane only serves bundles already in the sidecar's
index: anything rebuilt or not yet built after a scene edit 404s, since only
the legacy {version}/{entity}/{file} lane resolves digests tolerantly and
JIT-builds on miss. Rewrite there instead, deriving the entity id the same
way the preview content server does and dropping the deps digest from the
file name (bare b64 hashes never contain '_').
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lane The per-entity lane resolves the exact digest-bearing name straight from disk (immune to the stale boot-time bundle index that broke the flat /assets/ rewrite) and JIT-builds on miss — so only the entity segment needs rewriting. Preserving the digest keeps the served bundle exactly the one the manifest advertised instead of whatever same-hash build is on disk. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Deprecated in favour of decentraland/unity-explorer#9704 |
4 participants
What
sdk-commands start --asset-bundlesruns an abgen sidecar: an ab-cdn-compatible server that JIT-converts the scene being previewed into Unity asset bundles, reading it through the preview's own/contentendpoints. The CLI prints the sidecar URL and addsoptimized-assets-urlto the Desktop Explorer deep link. Off by default — the flag opts in.$ABGEN_BIN→ cached copy of the pinned release (v0.11.3) →abgenon the PATH → only then a one-time, sha256-verified download → graceful degradation with a warning..dcl-optimized-assets/next toscene.json(inside the default dcl-ignore).Review findings addressed since #1448
settledguard soerror+closecan't both settle the promise.--no-absolute-nameswas suggested but is rejected by both GNU tar 1.35 and bsdtar 3.8.8 (verified locally — it would break extraction everywhere). Both tars already strip absolute paths and refuse..members unless-Pis passed; documented in place.b64UrlHashDecodingFunction(now used by the/content/contents/:hashroute) with encode→decode round-trip tests covering base64url-specific characters (-,_) and unicode paths.expect.arrayContaining([expect.not.stringContaining(…)])→expect.not.arrayContaining([expect.stringContaining(…)])in explorer-alpha.spec.ts (all three occurrences).Cross-component P1s (companion PRs)
The base64url switch needs consumers to accept
-/_inb64-hashes:fix/accept-base64url-content-hashesis pushed with the regex fix + tests (PR pending).🤖 Generated with Claude Code