feat: request asset bundles by their digest-bearing file name#9442
feat: request asset bundles by their digest-bearing file name#9442dalkia wants to merge 27 commits into
Conversation
v49+ manifests list files as <hash>_<depsDigest>_<platform>, but the client was requesting the digest-less name and carrying the digest as a separate field threaded through every cache layer. Now the intention Hash itself resolves to the verbatim manifest entry, so the URL, the in-memory AB cache, the disk cache, Unity's web cache and the GLTF container cache all key off a single identity: - AssetBundleManifestVersion stores bare hash -> verbatim file name and exposes GetHashWithDigest / TryGetFileNameWithDigest - GLTF containers, AB dependencies, ISS LOD assets and scene emotes resolve their hash through the scene manifest at intention creation - GetAssetBundleIntention.DepsDigest is gone; equality, GetHashCode and the disk key use Hash alone - Unity cache keys dispatch on HasDepsDigests(): scene manifests (the only ones that fetch files[]) key on version|hash; wearables/emotes keep buildDate keying since their bundles are republished in place - DepsDigestKeyingEnabled and its feature-flag wiring are removed; the manifest version published by the backend is the rollout gate. FeatureId.AB_DEPS_DIGEST_CACHE_KEY stays as [Obsolete] to preserve enum ordering - Dead staticscene_ code removed (BuildInitialSceneStateURL, the disabled ISS bundle-mode HEAD probe) Verified against production: v49 manifests mix 2-part and 3-part names, digests are 32-hex, and the CDN serves both the digest-full and digest-less object names, so unmapped files keep working unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Build skipped — no changes detected under |
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: request asset bundles by their digest-bearing file name
STEP 2 — Root-cause check
Problem: v49+ asset-bundle manifests list their files with an embedded deps digest (<hash>_<depsDigest>_<platform>), but the client was requesting the digest-less name from the CDN and threading the digest as a separate DepsDigest field through four cache layers.
Does the diff fix the cause? Yes. The fundamental issue is an identity mismatch: the CDN serves files by their digest-bearing name, but the client was constructing a digest-less URL and tracking the digest separately. The PR correctly makes the digest part of the hash identity at the earliest possible moment (intention creation), eliminating the need to thread it through downstream caches. This is a causal fix, not a symptom treatment.
STEP 3 — Design & integration
The PR does not introduce new long-lived units. It modifies the existing AssetBundleManifestVersion class — which already owns the manifest file-name map — to expose GetHashWithDigest() (platform-suffixed lookup) and HasDepsDigests() (presence check). This is the correct home for this logic: the manifest version is the canonical source of file name resolution.
The DepsDigest field removal from GetAssetBundleIntention is clean: all four call sites now resolve through GetHashWithDigest before constructing the intention, so the digest travels inside Hash rather than alongside it. The call sites are:
GetSceneEmoteFromRealmIntention.CreateAndAddPromiseToWorld— scene emotesPrepareGltfAssetLoadingSystem.Prepare— GLTF containersLoadAssetBundleSystem.WaitForDependencyAsync— AB dependenciesResolveISSLODSystem.SpawnAssetPromises— ISS LOD assets
All four are consistent in their resolution pattern: compose hash + platform, resolve through manifest, use the result as the sole identity.
Cache dispatch change (SupportsDepsDigests() → HasDepsDigests()): This is a semantic improvement. SupportsDepsDigests() is a version check (≥ v49); HasDepsDigests() checks whether the deps map was actually injected. The distinction matters because wearable/emote manifests can be v49+ but never receive files[] — they must keep buildDate keying. Dispatching on the map's presence is more precise.
Feature flag removal (DepsDigestKeyingEnabled): The flag was at 100% in production and is now architecturally unnecessary — the rollout gate is the manifest version the backend publishes. The enum value is retained with [Obsolete] to preserve ordering. Clean lifecycle management.
Dead code removal (BuildInitialSceneStateURL, IsBundleReachableAsync, assetBundleURL parameter): Confirmed no callers. The LoadISSDescriptorSystem constructor correctly drops the unused assetBundleURL parameter, and GlobalWorldFactory.cs updates the injection call.
Teardown trace: No new subscriptions, callbacks, or resources are opened. The change only modifies hash resolution at creation sites.
STEP 4 — Member audit
GetHashWithDigest(string hash)— 4 consumers (listed above). Multi-use, correctly placed on the manifest owner. Strips the platform suffix, looks up the bare hash in the case-insensitive map, returns the verbatim manifest entry or the input unchanged. Sound API.HasDepsDigests()— 1 consumer (PrepareAssetBundleLoadingParametersSystemBase). Single-use, but it is a canonical predicate on the manifest's state (depsFiles != null), not a derived/re-checking property. Legitimate encapsulation.TryGetFileNameWithDigest(string bareHash, out string fileName)— 1 consumer (ComposeCacheKeyextension). Provides bare-hash lookup without platform suffix stripping — a different access pattern fromGetHashWithDigest. Legitimate API.ComposeCacheKeyextension — 2 consumers (ResolveISSLODSystem,GltfContainerPlugin). Multi-use, correctly bridges the manifest's file-name resolution to upper-layer caches.
No single-use-merge, absent≠false, or redundant-guard issues found.
STEP 5 — Line-level review
[P2] Stale comment (outside the diff) — GltfContainerPlugin.cs line 123:
The comment at Explorer/Assets/DCL/PluginSystem/World/GltfContainerPlugin.cs:123 still says:
when the scene's manifest has a deps digest for the hash, the key becomes "hash@digest", else the bare hash.
But after this PR, ComposeCacheKey now returns the verbatim digest-bearing manifest file name (e.g., bafk..._dda1af..._mac), not the old hash@digest format. Consider updating to:
when the scene's manifest has a digest-bearing file name for the hash, the key becomes that verbatim name, else the bare hash.
Security review: No security issues found. Hash parsing uses safe _-splitting on characters that cannot appear in valid CIDs. Disk cache keys are derived through Hash128.Compute(), so raw manifest strings never become file names. The CDN manifest shares the same trust boundary as the assets it describes. No credentials or secrets are present.
STEP 6 — Complexity assessment
COMPLEX. The diff touches the asset bundle loading pipeline across multiple ECS systems, the cache keying dispatch, the manifest version abstraction, and the intention equality/hashing contract. Changes span 14 files including 4 ECS systems and the core AssetBundleManifestVersion class.
STEP 7 — QA assessment
QA is required: the changes affect runtime asset bundle loading, cache keying, and CDN URL construction — all user-facing.
STEP 8 — Non-blocking warnings
No warnings (Main scene not modified).
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies asset bundle cache keying, manifest version abstraction, intention equality, and ECS loading systems across the AB pipeline
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
Call sites that hold a bare hash (GLTF containers, ISS LOD assets, scene emotes) no longer append the platform suffix and null-check the manifest themselves — a single null-safe extension composes the suffixed hash and resolves it to the digest-bearing manifest entry, keyed directly by the bare hash (no append-then-strip round trip). GetHashWithDigest stays for the one caller with an already-suffixed hash (AB dependencies from bundle metadata). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warnings not reduced: 14593 => 14601 — remove at least one warning to merge. Warnings/errors in files changed by this PR (165) |
Wearables, avatar emotes, thumbnails and LODs now compose their platform-suffixed hash through the same extension the scene paths use, so AB request-hash construction lives in a single place. Their manifests carry no deps map, so the output is unchanged (bare hash + platform suffix) — but if any of them ever ships digest-bearing file names, the URLs follow the manifest automatically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The method builds the exact hash requested from the CDN — the name now says so instead of describing its composition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pairs with GetCdnRequestHash: both produce the hash requested from the CDN — Get composes it from a bare hash, Resolve rewrites an already platform-suffixed one (dependency hashes from AB metadata). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both mapped a hash to the canonical CDN name — one from the manifest's files[] (digest-bearing entries), one from Qm entity content (casing fixes). They now share a single dictionary keyed by the digest-less platform-suffixed hash: - ResolveCdnRequestHash is a direct lookup (no more append-then-strip of the platform suffix) and fixes Qm casing too - CheckCasing is gone: every intention creation site already resolves through GetCdnRequestHash/ResolveCdnRequestHash, so the Prepare-time correction was redundant - HasDepsDigests is a dedicated bool so casing-only entries can never flip cache keying to the v49 scheme - InjectContent uses TryAdd so digest entries win regardless of injection order Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GetCdnRequestHash and ComposeCacheKey move from the extensions class into AssetBundleManifestVersion as static methods taking the nullable manifest, keeping the null handling and platform-suffix composition in one place. The extensions file is deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-nullable An AB can never be requested without a manifest, so the intention now requires one at creation instead of tolerating null until Prepare time: - Create/FromHash take the manifest (and Create the parentEntityID), so the scene PrepareAssetBundleLoadingParametersSystem no longer injects them per frame and lost its ISceneData dependency - TrimmedEntityDefinitionBase exposes AssetBundleManifestVersionOrFailed: the nullable wire-format field stays, but consumers get the failed sentinel the pipeline already treats as a dead end - the null-manifest error check in PrepareCommonArguments collapses to the failed-manifest check - GetCdnRequestHash / ComposeCacheKey become instance methods (callers always hold a manifest now), also restoring the platform suffix in GetCdnRequestHash's lookup - thumbnails with no manifest now fall back to the content-server texture path instead of spawning an AB promise doomed to fail Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The manifest owns every input of the decision (deps map presence, version, buildDate), so ComputeCacheHash lives there now and the prepare system just asks for it. ComputeHashV49/ComputeHashLegacy become private implementation details, and the tests exercise the real dispatch through manifests instead of calling the hash builders directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
InjectDepsDigests no longer reassembles the platform-suffixed key from file-name parts — the map now translates bare hash to canonical CDN file name directly. GetCdnRequestHash and ComposeCacheKey become plain lookups, and ResolveCdnRequestHash (suffixed-input variant) is deleted along with its platform-suffix stripping; the suffix is only appended in the fallback for files the manifest doesn't know. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both the org and zone CDNs only serve the nested {version}/{sceneID}/{file}
path for v49 — the flat path 404s — so v49+ must keep HasHashInPath until
the converter publishes a flat layout.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With ASSET_REUSE the converter uploads every bundle to
{version}/assets/{file} (no entity segment); the entity path only
survives as a Cloudflare Worker rewrite. Digest-mapped manifests now
request the canonical path directly, skipping the rewrite. Manifests
without the map (wearables/emotes, pre-v49 scenes, failed digest
fetches) carry digest-less hashes that only resolve through the
Worker's entity path, so they keep the legacy URL shapes.
Verified against the production CDN: both digest-bearing and 2-part
files serve under /v49/assets/, and the entity segment is ignored by
the rewrite (any value returns the same object).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The flag signals what the converter actually guarantees: only scenes fetch files[], and only scene bundles upload to the canonical assets/ prefix (asset-reuse is gated on entity.type === 'scene'). The name now says so, and the flag fires for any injected files[] — a reuse-converted scene can legitimately list only 2-part names and still live under assets/. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With asset reuse permanently on, a v49+ manifest's files[] are exactly the names bundles live under on the assets/ prefix — so InjectDepsDigests now maps every platform-suffixed entry (2-part textures/bins included), giving all scene files verbatim manifest names and casing. The injection also self-gates on v49+: hybrid/world flows inject files[] for whatever version they fetched, and a pre-v49 manifest flagged as canonical would 404 under assets/ where its entity-scoped bundles don't exist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t-circuit InjectDepsDigests self-gates on v49+ (it has a second, ungated caller in the hybrid flow), so the loader's check only exists to skip a wasted manifest download for pre-v49 scenes. Its null-check goes through the OrFailed accessor instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The v49+ check lives with the callers: the digests loader already gates (it also skips the manifest download), and the hybrid flow now gates its injection too. InjectDepsDigests just stores what it is given. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review Summary
STEP 2 — Root-cause check: PASS
The PR fixes the root cause: v49+ manifests list files as <hash>_<digest>_<platform>, but the client was requesting digest-less names from the CDN and threading the digest as a separate DepsDigest field through four cache layers (intention equality, disk cache, Unity web cache, GLTF container cache). The fix embeds the digest in the hash itself so URL and every cache layer share one identity. This is a genuine root-cause fix, not a workaround.
STEP 3 — Design & integration: PASS
The consolidation into AssetBundleManifestVersion is architecturally sound:
cdnFilesdictionary unifies the oldconvertedFilesHashSet (Qm casing) anddepsDigestsdictionary into a single bare-hash → canonical-CDN-file-name map. The two injection methods (InjectDepsDigestsfor manifest files[],InjectContentfor Qm casing) feed the same map, withTryAddensuring digest-bearing entries win regardless of injection order.AssetBundleManifestVersionis the natural owner — it already heldCheckCasingandTryGetDepsDigest; these are now unified intoGetCdnRequestHashandComposeCacheKey.- No new lifecycle units introduced — this PR removes complexity (extensions class deleted, per-frame manifest injection eliminated, feature-flag wiring deleted).
- The removal of per-frame manifest/parentID injection in
PrepareAssetBundleLoadingParametersSystemis correct: intentions now carry their manifest at creation time, eliminating per-frame reconciliation that the ECS anti-pattern guidance warns against. - Owner search:
AssetBundleManifestVersionis created inLoadHybridSceneSystemLogic.FetchRemoteAssetBundleVersionAsyncandAssetBundleManifestFallbackHelper. It is populated with content/digest data at those creation points. The new methods (GetCdnRequestHash,ComputeCacheHash,ComposeCacheKey) are consumed at intention-creation sites and inPrepareCommonArguments. No new unit duplicates an existing lifecycle.
STEP 4 — Member audit
| Member | Consumers | Assessment |
|---|---|---|
GetCdnRequestHash(bareHash) |
7 call sites (emotes, wearables, thumbnails, GLTF, LOD, ISS) | Well-named, single responsibility. Correctly falls back to platform-suffixed bare hash when not in the map. ✓ |
ComposeCacheKey(hash) |
2 (GltfContainerPlugin, ResolveISSLODSystem) | Migrated from extension method, same semantics. ✓ |
ComputeCacheHash(hash) |
1 (PrepareCommonArguments) | Replaces the split ComputeHashV49/ComputeHashLegacy dispatch. Correctly gates on HasCanonicalAssets(). ✓ |
HasCanonicalAssets() |
2 (URL construction, cache hash dispatch) | Correctly reflects whether files[] were injected. Gated by SupportsDepsDigests() at the injection site. ✓ |
AssetBundleManifestVersionOrFailed |
7 call sites | See finding 1 below — allocates per access when null. |
STEP 5 — Line-level findings
See inline comments below. All findings are P2 (minor improvements, not blocking).
Security review: PASS
No security issues found. URL construction consolidation reduces attack surface by funneling all paths through fewer methods. No secrets, injection risks, path traversal vectors, or unsafe input handling patterns introduced.
Consumer impact analysis
The GetAssetBundleIntention API changed (field renamed, DepsDigest removed, manifest now required). All consumers are internal to unity-explorer — no published package surface is affected. AssetBundleManifestVersionExtensions was deleted, but its only consumer (ResolveISSLODSystem) was migrated to the instance method. No cross-repo breakage.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies core AB loading pipeline — cache keying, URL construction, intention API, and feature-flag removal — across 26 files spanning ECS systems, plugins, LOD, thumbnails, and avatar rendering.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U04RC3JBQ96>) via Slack
- cache the failed-manifest sentinel instead of allocating per access - drop redundant null-forgiving operators in ComputeCacheHash - justify the remaining ! operators on the platform-info getters - reword the TryAdd comment to describe the line, not its callers Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review Summary
STEP 2 — Root-cause check ✅
The PR description accurately identifies the root cause: the client was requesting digest-less bundle names from the CDN while threading the digest as a separate DepsDigest field through four cache layers. This created a persistent identity mismatch — the URL, intention equality, disk cache, Unity web cache, and GLTF container cache each had to align on different pieces of the same identity, and the seams caused cache misses and "asset bundle already loaded" races.
The fix addresses the cause directly: the digest is now embedded inside the hash at intention-creation time via GetCdnRequestHash(), so every layer shares one canonical string. The DepsDigest field is deleted, and the AssetBundleManifestVersion class consolidates both the digest map and the old Qm-casing fix into a single cdnFiles dictionary. This is a cause-level fix, not a symptom workaround.
STEP 3 — Design & integration ✅
The PR consolidates all asset-bundle naming/keying logic into AssetBundleManifestVersion, which is the natural owner of this data — it holds the manifest's files[] and already owned the version/buildDate information. The previous design scattered the responsibility: PrepareAssetBundleLoadingParametersSystemBase held ComputeHashV49/ComputeHashLegacy, the extensions class held ComposeCacheKey, CheckCasing lived on the manifest but convertedFiles was a separate collection, and DepsDigest traveled as a separate field on GetAssetBundleIntention.
The new design moves all these into AssetBundleManifestVersion (GetCdnRequestHash, ComposeCacheKey, ComputeCacheHash, TryGetCdnFileName). This is correct — the manifest is the source of truth for how bundles are named and keyed.
Owner search:
GetCdnRequestHash— replaces the scatteredhash + PlatformUtils.GetCurrentPlatform()+CheckCasing+ manualTryGetDepsDigestcalls at each call site (scene emotes, wearables, emotes, thumbnails, GLTF containers, LODs). The manifest is the owner of this mapping.ComputeCacheHash— replaces the staticComputeHashV49/ComputeHashLegacythat lived inPrepareAssetBundleLoadingParametersSystemBase. Correctly moved to the manifest since it's the manifest that knows whether to use version-based or buildDate-based keying.HasCanonicalAssets— new predicate that drives URL shape dispatch. Only set whenInjectDepsDigestsis called with actual files, gated bySupportsDepsDigests()at the injection site.
The AssetBundleManifestVersionOrFailed sentinel pattern on TrimmedEntityDefinitionBase replaces nullable AssetBundleManifestVersion? threading. The FAILED_MANIFEST is a shared static readonly instance — confirmed no mutation paths exist after construction.
Teardown trace: No subscriptions, event hookups, connections, or disposables are added by this PR. The cdnFiles dictionary is allocated lazily and lives for the lifetime of the manifest instance (which is per-scene/entity definition). No leak risk.
STEP 4 — Member audit ✅
| Member | Consumers | Verdict |
|---|---|---|
AssetBundleManifest (field) |
All loading paths, LoadAssetBundleSystem, PrepareAssetBundleLoadingParametersSystemBase |
Replaces nullable AssetBundleManifestVersion? — every factory requires it. ✅ |
AssetBundleManifestVersionOrFailed (property) |
7 call sites across wearables, emotes, GLTF, LODs, digests loader, plugin | Single purpose: null-coalesce to failed sentinel. Not single-use (7 consumers). ✅ |
GetCdnRequestHash(bareHash) |
8 call sites across scene emotes, wearables, emotes, thumbnails, GLTF containers, LODs | Centralizes hash→CDN-name translation. Multi-consumer, clear intent. ✅ |
ComposeCacheKey(hash) |
2 call sites: ResolveISSLODSystem, GltfContainerPlugin |
Replaces the deleted extension method. ✅ |
ComputeCacheHash(hash) |
1 call site: PrepareAssetBundleLoadingParametersSystemBase |
Single-consumer but legitimately encapsulates the v49/legacy dispatch that only the manifest can decide. Not a re-derive. ✅ |
HasCanonicalAssets() |
2 call sites: PrepareAssetBundleLoadingParametersSystemBase, ComputeCacheHash |
Drives URL prefix and cache-key scheme dispatch. ✅ |
STEP 5 — Line-level review
Findings below, ordered by severity.
STEP 6 — Complexity
This PR touches the asset bundle loading pipeline (cache keying, URL construction, intention creation), modifies cross-assembly public APIs (GetAssetBundleIntention factory signatures), changes the GLTF container cache key composition, and removes a feature flag. 27 files changed, ~250 lines of meaningful logic changes across the AB loading, LOD, emote, wearable, and thumbnail subsystems.
STEP 7 — QA assessment
This change modifies runtime code that affects how every asset bundle is requested, cached, and keyed. Scene loading, wearables, emotes, thumbnails, and LODs are all affected. Manual testing is required to verify all these paths work correctly with both v49+ and pre-v49 manifests.
STEP 8 — Non-blocking warnings
No main scene modification detected. No warnings.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Touches the asset-bundle loading pipeline (cache keying, URL construction, intention equality), cross-assembly public API changes, feature flag removal, and modifications across LOD, emote, wearable, thumbnail, and GLTF container subsystems.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
|
||
| public AssetBundleManifestVersion? AssetBundleManifestVersion; | ||
| //Required by every factory — an AB can never be requested without a manifest. Only default-initialized structs (never flowed into loading) can carry null. | ||
| public AssetBundleManifestVersion AssetBundleManifest; |
There was a problem hiding this comment.
[P2] Nullability annotation: AssetBundleManifest is a reference type field on a struct, initialized to null via default. The comment says "Only default-initialized structs (never flowed into loading) can carry null", but the field is declared as non-nullable AssetBundleManifestVersion rather than AssetBundleManifestVersion?. This means a default-initialized GetAssetBundleIntention silently carries a null in a non-nullable slot — a nullability-contract lie per CLAUDE.md §11 ("Defensive null-checks against non-null declarations"). Since the internal (CommonLoadingArguments) constructor explicitly assigns CreateManualManifest(), and all public factories require it, the only path to null is default struct initialization.
Consider making the field nullable and adding an assertion in the loading path, or documenting the invariant with = null! (DTO exception pattern) since this is a struct field:
| public AssetBundleManifestVersion AssetBundleManifest; | |
| //Required by every factory — an AB can never be requested without a manifest. Only default-initialized structs (never flowed into loading) can carry null. | |
| public AssetBundleManifestVersion AssetBundleManifest = null!; |
| public string? GetAssetBundleManifestVersion() => | ||
| IPlatform.DEFAULT.Is(IPlatform.Kind.Windows) ? assets?.windows!.version : assets?.mac!.version; | ||
| //! safe: every factory (CreateFromFallback, CreateFailed, CreateManualManifest, CreateForLOD) sets the current platform's info, and deserialized manifests carry both platforms. | ||
| public string GetAssetBundleManifestVersion() => |
There was a problem hiding this comment.
[P2] Null-forgiving operators without justifying comment: assets?.windows!.version! and assets?.mac!.version! each use two ! operators. The //! safe: comment above covers the factory invariant, but per CLAUDE.md §11 the ! operators should have justification at the point of use, not just a nearby comment. The conditional access assets?. can return null when assets is null, and then .windows! would throw. Consider a guard or a more explicit pattern:
| public string GetAssetBundleManifestVersion() => | |
| //! safe: every factory (CreateFromFallback, CreateFailed, CreateManualManifest, CreateForLOD) sets the current platform's info, and deserialized manifests carry both platforms. | |
| public string GetAssetBundleManifestVersion() => | |
| (IPlatform.DEFAULT.Is(IPlatform.Kind.Windows) ? assets?.windows?.version : assets?.mac?.version) | |
| ?? throw new InvalidOperationException("AssetBundleManifestVersion.assets must be set by factory before use"); |
| public string? GetAssetBundleManifestBuildDate() => | ||
| IPlatform.DEFAULT.Is(IPlatform.Kind.Windows) ? assets?.windows!.buildDate : assets?.mac!.buildDate; | ||
| //! safe: same factory invariant as GetAssetBundleManifestVersion. | ||
| private string GetAssetBundleManifestBuildDate() => |
There was a problem hiding this comment.
[P2] Same null-forgiving pattern as GetAssetBundleManifestVersion() — assets?.windows!.buildDate! / assets?.mac!.buildDate!. Apply the same guard pattern:
| private string GetAssetBundleManifestBuildDate() => | |
| //! safe: same factory invariant as GetAssetBundleManifestVersion. | |
| private string GetAssetBundleManifestBuildDate() => | |
| (IPlatform.DEFAULT.Is(IPlatform.Kind.Windows) ? assets?.windows?.buildDate : assets?.mac?.buildDate) | |
| ?? throw new InvalidOperationException("AssetBundleManifestVersion.assets must be set by factory before use"); |
| } | ||
|
|
||
| string platformLODKey = $"{sceneDefinitionComponent.Definition.id.ToLower()}_{level.ToString()}{PlatformUtils.GetCurrentPlatform()}"; | ||
| AssetBundleManifestVersion lodManifest = AssetBundleManifestVersion.CreateForLOD($"LOD/{level.ToString()}", "dummyDate"); |
There was a problem hiding this comment.
[P2] Allocation on hot path: AssetBundleManifestVersion.CreateForLOD(...) allocates a new AssetBundleManifestVersion + AssetBundleManifestVersionPerPlatform + PlatformInfo every time a LOD promise is started. Since LOD levels are a small fixed set (0–4), consider caching these manifests. This is non-blocking since it only runs once per LOD transition (not every frame), but it's worth noting for future optimization.
| AssetBundleManifestVersion lodManifest = AssetBundleManifestVersion.CreateForLOD($"LOD/{level.ToString()}", "dummyDate"); | |
| AssetBundleManifestVersion lodManifest = AssetBundleManifestVersion.CreateForLOD($"LOD/{level}", "dummyDate"); |
Minor: .ToString() on level (a byte) is redundant inside an interpolated string — the compiler calls it implicitly.
…/AssetBundles/GetAssetBundleIntention.cs Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: Juan Ignacio Molteni <juanignaciomolteni@gmail.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review — feat: request asset bundles by their digest-bearing file name
STEP 2 — Root-cause check: ✅ PASS
The PR correctly addresses the root cause: the v49+ deps-digest was threaded as a separate DepsDigest field through four cache layers (intention equality, disk cache, Unity web cache, GLTF container cache), causing identity mismatches and complexity. The fix embeds the digest into the hash itself at intention-creation time, so every downstream layer shares one identity.
STEP 3 — Design & integration: ✅ PASS
No new long-lived units are introduced. The refactoring consolidates all AB naming/keying logic into AssetBundleManifestVersion (the class that already owns the manifest data), which is the correct home for these operations:
GetCdnRequestHash(bareHash)— translates a bare hash to the CDN filename (digest-bearing, correctly cased, or fallback). Centralizes logic previously scattered across 7+ call sites.ComposeCacheKey(hash)— GLTF container cache key. Replaces the deletedAssetBundleManifestVersionExtensionshelper.ComputeCacheHash(hash)— Unity-cache key dispatch (v49 vs legacy). Moves the twoComputeHashV49/ComputeHashLegacymethods fromPrepareAssetBundleLoadingParametersSystemBaseinto the manifest.HasCanonicalAssets()— URL-shape switch betweenassets/prefix and legacy entity-scoped paths.
The per-frame injection of manifest + parentEntityID in PrepareAssetBundleLoadingParametersSystem (via ISceneData) is correctly eliminated — these are now set at intention creation, removing the ISceneData dependency from the system.
Lifecycle owner search: AssetBundleManifestVersion is created by LoadHybridSceneSystemLogic, AssetBundleManifestFallbackHelper, LoadTrimmedElementsByIntentionSystem, and factory methods. Disposal is by GC (reference type with no native resources). The cdnFiles dictionary and hasCanonicalAssets flag are set during scene loading (via InjectDepsDigests/InjectContent) and read during intention creation. No lifecycle mismatch.
Teardown/consumption trace: No new subscriptions, event hookups, connections, or buffers are introduced. The PR removes DepsDigest, the convertedFiles HashSet, and the depsDigests dictionary — all correctly replaced by the unified cdnFiles dictionary. The TryAdd in InjectContent and unconditional set in InjectDepsDigests ensure digest entries always win regardless of injection order (verified by test PreferDigestEntriesOverContentCasingEntries).
STEP 4 — Member audit: ✅ PASS
| Member | Consumers | Verdict |
|---|---|---|
GetCdnRequestHash(string) |
Scene emotes, avatar emotes, wearables, thumbnails, GLTF prepare, ISS LOD, LOD system, tests | Multi-consumer, centralizes hash translation |
ComposeCacheKey(string) |
GLTF container cache, ISS LOD resolve | Multi-consumer, replaces extension method |
ComputeCacheHash(string) |
PrepareAssetBundleLoadingParametersSystemBase |
Single consumer, but legitimate encapsulation — dispatches v49 vs legacy keying based on manifest state |
HasCanonicalAssets() |
GetAssetBundleURL, ComputeCacheHash, tests |
Multi-consumer |
AssetBundleManifestVersionOrFailed |
PrepareGltfAsset, ResolveISSLOD, GltfContainerPlugin, DigestsLoader, EmotesLoader, WearablePolymorphic | Multi-consumer, eliminates null-checks at call sites |
No single-use intermediates, no absent≠false conflation, no re-derived values.
STEP 5 — Line-level review: ✅ No blocking issues
Two passes over all changed lines. No P0 or P1 issues found:
- No bugs or runtime errors — all code paths correctly handle the manifest being failed (short-circuits before accessing version/buildDate), LSD assets (gated by
IsLSDAssetchecks), and nullcdnFiles(fallback to platform-suffixed bare hash). - No resource leaks — no new subscriptions/connections/handles introduced.
- No security issues — URLs constructed from trusted manifest data, no injection vectors.
- No LINQ in hot paths — the existing
COMMON_SHADERS.Contains()inGetStreamingAssetsUrlis pre-existing and not in the diff. - No allocation in Update — the per-frame
PrepareCommonArgumentsno longer callssceneDataaccessors; hash translation happens at intention creation time. - Nullability —
AssetBundleManifestonGetAssetBundleIntentionis non-nullable with a justifying comment explaining that only default-initialized structs can carry null. The!operators onGetAssetBundleManifestVersion()andGetAssetBundleManifestBuildDate()have factory-invariant comments. - Static
FAILED_MANIFESTsingleton — mutable class shared across instances, but all consumers call only read-only methods (GetCdnRequestHash,ComposeCacheKey,ComputeCacheHash,SupportsDepsDigests). Lazy-cached booleans are idempotent. No mutation paths reach it. - Feature flag removal —
DepsDigestKeyingEnabledand itsFeaturesRegistrywiring are cleanly deleted.FeatureId.AB_DEPS_DIGEST_CACHE_KEY = 63kept with[Obsolete]to preserve enum ordering — correct. - Dead code removal — ISS bundle-mode HEAD probe (
IsBundleReachableAsync),staticscene_handling (BuildInitialSceneStateURL),CheckCasing/convertedFilesall removed with no remaining references (confirmed via grep). InjectDepsDigestsguard inLoadHybridSceneSystemLogic— pre-v49 bundles skip injection, preventing entity-scoped bundles from being wrongly flagged as canonical. Correct.- Comments — all justify non-obvious invariants (cache identity matching, CDN filename resolution, injection-order precedence). None narrate caller behavior.
- Git conventions — title
feat: request asset bundles by their digest-bearing file nameand branchfeat/ab-url-exact-deps-digestfollow ADR-6.
STEP 6 — Complexity: COMPLEX
Touches the asset bundle loading pipeline, cache keying scheme, multiple ECS systems (PrepareAssetBundleLoading, PrepareGltfAssetLoading, ResolveISSLOD, UpdateSceneLODInfo), plugin wiring, feature flags, and the AB manifest version class across 27 files.
STEP 7 — QA assessment: YES
Modifies runtime asset bundle loading, URL construction, and cache keying for all scene, wearable, emote, thumbnail, and LOD content. User-facing impact: any AB loading regression would manifest as missing scenes, wearables, or emotes.
STEP 8 — Non-blocking warnings
None. Main.unity is not in the changed files.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the asset bundle loading pipeline, cache keying scheme, manifest version class, and multiple ECS systems across 27 files
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
|
Tests: 236 passed, 0 failed ✅ |
default-initialized structs zero reference fields regardless of annotations, so the non-nullable AssetBundleManifest field could still observe null through default(GetAssetBundleIntention). It becomes a private nullable backing field with a readonly property falling back to the shared AssetBundleManifestVersion.FAILED sentinel — the same dead end PrepareCommonArguments already reports — making the annotation truthful for every observer. The sentinel is safe to share because injections no-op on failed manifests (they serve no file names). Every consumer now uses it (fallback helper, ISS descriptor intention, OrFailed accessor), so CreateFailed becomes a private detail of the sentinel. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The manifest owns all three URL shapes now (GetCdnRequestPath), so the prepare system just appends the relative path to the domain. That makes HasHashInPath private and dissolves HasCanonicalAssets into a private hasReusableAssets field — renamed because "canonical" was converter jargon; what the flag really tracks is that the bundles went through asset reuse, which is why they live under the shared assets/ prefix and cache-key on version+hash. Tests assert the path shapes behaviorally instead of poking the flags. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
Pull Request Description
What does this PR change?
v49+ asset-bundle manifests list their files as
<hash>_<depsDigest>_<platform>, but the client was requesting the digest-less name from the CDN and threading the digest as a separateDepsDigestfield through four cache layers (intention equality, disk cache, Unity web cache, GLTF container cache). This PR makes the digest part of the hash itself and, along the way, consolidates all asset-bundle naming/keying logic intoAssetBundleManifestVersion.Digest-bearing CDN requests
AssetBundleManifestVersionstores a single bare hash → canonical CDN file name map (cdnFiles), fed by the manifest'sfiles[].GetCdnRequestHash(bareHash)translates a bare hash to the digest-bearing, correctly-cased name; files the manifest doesn't list fall back to the platform-suffixed bare hash (both names verified live on the CDN).intention.Hash, so the URL and every cache layer share one identity —DepsDigestis deleted.CheckCasing/convertedFiles) is unified into the same map:InjectContentadds bare hash → corrected-case name entries (digest entries win regardless of injection order), and the per-frameCheckCasingcall in the prepare system is gone.Cache keying
ComputeCacheHash(hash)lives onAssetBundleManifestVersion: manifests that receivedfiles[](scenes only) key onversion|hash— shareable across CDN republishes since the digest is in the hash — while wearables/emotes keep the byte-identical legacybuildDate+hashkeying, because their bundles are republished in place and must invalidate.ComputeHashV49/ComputeHashLegacyare private implementation details.ComposeCacheKey(GLTF container cache) resolves through the same map, so the SDK runtime and LOD/ISS paths land in the same cache slots.Feature flag removed
DepsDigestKeyingEnabledand its wiring (bootstrap,FeaturesRegistryentry, FF string) are deleted — the flag is at 100% in production, and the manifest version published by the backend is the real rollout gate.FeatureId.AB_DEPS_DIGEST_CACHE_KEY = 63stays as[Obsolete]to preserve enum ordering. Also removes a leftover debug log inBootstraper.Intentions always carry a manifest
GetAssetBundleIntention.AssetBundleManifest(renamed) is non-nullable;Create/FromHashrequire it (andCreatetheparentEntityID), so the scene-worldPrepareAssetBundleLoadingParametersSystemno longer injects manifest/parentID per frame and lost itsISceneDatadependency.TrimmedEntityDefinitionBase.AssetBundleManifestVersionOrFailed, which hands out the failed sentinel the pipeline already treats as a dead end.Dead code removed
staticscene_handling (BuildInitialSceneStateURL, the disabled ISS bundle-mode HEAD probe and itsassetBundleURLdependency) — the AB type isn't currently supported.AssetBundleManifestVersionExtensionsfolded into the class.Known one-time cost
v49 Unity-cache keys change shape (
version|hash|digest→version|hash-with-digest), so previously cached v49 entries re-download once. Legacy (pre-v49 / wearable) keys are byte-identical and keep hitting.Test Instructions
Steps (standard run):
Expected result:
Genesis City scenes load normally. AB requests for v49 scenes hit digest-bearing URLs under the canonical prefix, e.g.
https://ab-cdn.decentraland.org/v49/assets/<hash>_<32hexdigest>_macinstead of.../v49/<sceneID>/<hash>_mac.Steps (fresh account):
Expected result:
Same as above from a cold cache — scenes, wearables, and emotes all load; no
Manifest version must be providedorasset bundle already loadederrors in logs.Automation (if applicable):
metaforge explorer test 9442Prerequisites
alfa-ab-deps-digest-cache-keyflag is no longer read)Test Steps
{version}/assets/prefix and contain the 32-hex digest segment for files listed with one inhttps://ab-cdn.decentraland.org/manifest/<sceneID>_<platform>.jsonAdditional Testing Notes
DepsDigestCacheKeyShouldcovers hash translation, Qm casing, injection-order precedence, cache-key dispatch and hash stability behaviorally through manifestsQuality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.
🤖 Generated with Claude Code