Skip to content

feat: abgen-registry feature flag to switch AB registry + CDN - #9757

Open
dalkia wants to merge 10 commits into
devfrom
feat/abgen-registry-flag
Open

feat: abgen-registry feature flag to switch AB registry + CDN#9757
dalkia wants to merge 10 commits into
devfrom
feat/abgen-registry-flag

Conversation

@dalkia

@dalkia dalkia commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

What

Adds a feature flag that switches the asset-bundle registry and CDN to their parallel -abgen siblings — the client half of the effort to run an abgen-backed asset-bundle pipeline side-by-side with the production Unity one.

Changes

  • FeatureFlagsStrings.cs — new ABGEN_REGISTRY = "abgen-registry" flag constant.
  • DecentralandUrlsSource.cs — new ResolveAssetBundleUrl seam used by AssetBundleRegistry and AssetBundlesCDN. When the flag is on, it rewrites the host \*.decentraland.\*\*-abgen.decentraland.\* (e.g. asset-bundle-registry-abgen.decentraland.org, ab-cdn-abgen.decentraland.org) via InsertAbgenSubdomain.

Behavior

  • Switching the single AssetBundleRegistry base auto-repoints every registry-derived endpoint (/entities/versions, /entities/active, /profiles, world manifest) through ComposeRegistryUrl; the CDN host moves with it.
  • Takes precedence over OPTIMIZED_ASSETS so the whole AB surface moves together.
  • Returns CacheBehaviour.FeatureFlagsDependent so the host is re-resolved (not cached) until flags load.
  • The --optimized-assets-url CLI override still wins.
  • LODs are intentionally not routed — abgen's LOD lane is unimplemented, so LodGeneratorCDN keeps calling ResolveOptimizedAssetsUrl directly.

Rollout

Ship off-by-default, then enable per-wallet → percentage → default. Note the registry base is boot-time (ctor-injected into world factories) — the client must be restarted between flag changes, not just re-deeplinked.

🤖 Generated with Claude Code

eordano and others added 10 commits August 13, 2026 01:10
…mment

The PR status comment's build badge was a bare shields.io image (clicking it
opened the image itself), and finding the actual Unity Cloud build meant going
to cloud.unity.com and searching for the target and build id by hand.

- build.py captures the dashboard deep link (links.dashboard_summary /
  dashboard_log) from the first build response that carries one, prints it as
  a ::notice::, adds it to the step summary, and persists it to
  unity_cloud_build_info.env.
- build-unitycloud.yml uploads that file as a unity_build_info_* artifact.
  It is written as soon as the Unity-side build id is known, so it exists for
  failed builds too.
- pr-comment-artifact-url.yml adds "Unity Cloud build (Windows/Mac)" rows
  linking the build id to its Unity Cloud page, on both the success and
  failure comments, and wraps every badge in a link to the Actions run.
  The info files are produced by the PR-controlled build workflow, so ids and
  URLs are validated (numeric id, Unity dashboard origin, conservative
  charset) before being rendered into the comment.
- check-build-ran now also counts unity_build_info_* artifacts as evidence
  that a build ran, so a build that failed before producing player artifacts
  posts the failure comment (with the Unity Cloud link) instead of leaving
  the comment stuck on "Pending".

Note: the dashboard URL comes from the Unity Cloud Build API response and
contains the org/project slugs; it will be visible in PR comments.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses the security review on #9713:

- check-build-ran now emits two outputs: player-artifacts (Decentraland_* only)
  keeps gating the success/skipped split so comment-success never interpolates
  missing artifact ids, while build-ran (player or unity_build_info_*) widens
  only the failure path.
- The duplicated parse/compose logic moved into a composite action
  (.github/actions/ucb-build-links) used by both comment jobs, with the
  validation in one place, unique GITHUB_OUTPUT heredoc delimiters, no
  empty-label "[#](url)" rows on tampered input, and gh download errors
  surfaced in the log instead of swallowed.
- build.py only accepts absolute https:// dashboard hrefs (matching the
  consumer regex) and writes the info file immediately after the build id is
  known, not on the first poll.
- unity_build_info_* uploads use retention-days 7; URL charset allows fragments.

The org/project-slug exposure in public comments (finding 1) is accepted:
the ids grant no access without Unity org membership, and the deep link in
the comment is the point of the feature.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s, automation)

Extends the unified CI status comment into a navigation hub:

- Build rows now pair each target's Unity Cloud build page with its GitHub
  job log ("Windows build | Unity Cloud #id . GitHub job").
- Tests section: badge links the Unity Test run (where the dorny report
  lives), each suite links its job, a Time column shows suite duration, a
  collapsible "Slowest tests" top-10 is parsed from the NUnit XML, and a
  footer links the Test results artifacts (XML + editor logs). The extractor
  in test.yml now records duration and slowest tests; the trusted composer
  type-checks both before rendering (numeric seconds, single-line names).
- Lint section: badge links the lint run; footer links the run and the
  csharp-lint-reports artifact (the inline findings list is capped).
- New "automation" section in the status comment: defaults to an on-demand
  hint for /visual-tests, flips to Running when the suite dispatches, and
  lands on Passed/Failed with the Allure report + run links. The reusable
  workflow's own detailed comment is unchanged.
- ci-status-comment now appends a missing section fence to existing comments
  instead of resetting the whole comment to the skeleton (which would have
  wiped the other sections' state when the automation section first writes).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a fifth "performance" section to the unified CI status comment,
covering both perf lanes:

- Bare-metal benchmark (decentraland/performance-testing): when
  comment-success dispatches it after a successful build, the section flips
  to "Dispatched" linking the target workflow's run queue; the benchmark's
  own perf-test-summary comment (which links its run) remains the detailed
  result, as repository_dispatch returns no run id to link directly.
- In-repo Unity Performance Test (perf_test label): new companion
  pr-comment-perf.yml (workflow_run, trusted context) writes Passed/Failed
  with links to the run summary (which renders the benchmark report) and the
  JSON results + PDF report artifacts. The workflow fires on every PR event
  but its job gates on the label, so the companion checks the job actually
  ran (jobs API) before touching the section - a skipped run must not
  overwrite the bare-metal dispatch status.
- Section default documents both lanes, including that perf_test skips
  normal CI and blocks merge while set.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The repository-dispatch to decentraland/performance-testing has been failing
silently since ~2026-06-15 ("Repository not found, OR token has insufficient
permissions" — the PERFORMANCE_TESTING_PAT secret, last rotated 2026-05-20,
expired). The step turned every comment-success run red but left no trace on
the PR, so nobody noticed for two months. On dispatch failure the performance
section now shows a red "Dispatch failed" state pointing at the step log and
naming the likely cause.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mment

After a successful benchmark dispatch, probe the PAT's remaining lifetime
via the github-authentication-token-expiration response header and, when under
30 days, append a rotation warning to the performance section of the unified
CI status comment. This replaces the idea of a separate expiry-canary workflow
opening issues — the warning lives where people already look. The 2026-06..08
outage was exactly this token expiring with no warning anywhere visible.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two opt-in knobs on upsert-ci-status.sh for decentraland/performance-testing,
which will fold its benchmark report into the performance section instead of
posting a standalone perf-test-summary comment:

- SECTION_BODY_FILE: read the section body from a file (the report is too
  large to pass comfortably via env).
- NO_CREATE=1: exit 3 instead of creating the unified comment when it does
  not exist - a comment created with a foreign token would not be authored by
  github-actions[bot], later writers would not find it, and duplicates would
  accumulate. The caller falls back to its standalone comment instead.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes for the 10 open review threads:

- ucb-build-links: URL_RE now requires /builds/<digits> under the Unity hosts, mirroring build.py's producer check — query-string-only paths (open-redirect bait) no longer validate.
- pr-comment-artifact-url: artifacts list fetched with per_page=100 so unity_build_info_* cannot fall off page 1 and fake build-ran=false; the PAT expiry probe moved to gh api HEAD /rate_limit (token via env, never argv), gained continue-on-error + a 15s ceiling + a loud warning when the expiration header is absent; the dispatch-failed marker now fires on outcome != success so upstream failures (Find latest release, section upserts) no longer leave the performance section silent.
- pr-comment-test-failures: slowest/failed test names render inside inline code with backticks/pipes stripped — markdown-shaped names read as text instead of first-party links/images.
- test.yml: duration values are clamped to finite floats (float() admits nan/inf/1e999), keeping the timings JSON valid for the consumer's jq.
- visual-regression: the two comment-writing jobs drop to contents:read + pull-requests:write. Narrowing secrets:inherit needs run-visual-suite.yml to declare workflow_call secrets first (it currently instructs callers to use inherit), so that part stays.
- upsert-ci-status: fence-existence check is whole-line (grep -qxF) matching the awk matchers, so an embedded marker in a body line can no longer wedge a section; file-passed bodies are capped at 20k chars with a visible truncation note (GitHub's 65536 ceiling is shared by all sections); SECTION is validated against the fence set and fails fast; duplicate-comment GC is skipped for NO_CREATE callers; NO_CREATE waits one round before falling back to a standalone comment.

Committed via API because repository rules require verified signatures.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…urally

Round-6 review fixes:

- test.yml: the serialised duration is clamped too — per-case isfinite keeps each addend and the slowest list finite, but a sum of finite doubles (two 1e308s) still overflows to inf, and round(inf,1) would emit bare Infinity into the JSON. Failed test-case entries with neither fullname nor name fall back to "(unnamed)" so sorted(set(...)) cannot hit a None/str TypeError.
- upsert-ci-status.sh: the 20k cap now guards both body paths (env and file), and truncation closes any code fence or <details> the cut severed — an unterminated construct would render the rest of the comment inside it, visually eating the neighbouring sections.

Committed via API because repository rules require verified signatures.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the `abgen-registry` flag and a ResolveAssetBundleUrl seam in
DecentralandUrlsSource that, when enabled, routes AssetBundleRegistry and
AssetBundlesCDN (and all registry-derived endpoints) to their `-abgen`
sibling hosts, taking precedence over OPTIMIZED_ASSETS. FeatureFlagsDependent
caching; CLI --optimized-assets-url override still wins; LODs are not routed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@dalkia
dalkia requested review from a team as code owners August 14, 2026 18:05
@github-actions
github-actions Bot requested a review from anicalbano August 14, 2026 18:05
@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

🚦 CI Status

Build

Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below.

Name Link
Commit e91bc9f
Logs https://github.com/decentraland/unity-explorer/actions/runs/31827036122
Download Windows https://github.com/decentraland/unity-explorer/suites/86346713551/artifacts/9230376716
Download Windows S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/feat/abgen-registry-flag/pr-25154-e91bc9f/Decentraland_windows64.zip
Download Mac https://github.com/decentraland/unity-explorer/suites/86346713551/artifacts/9230379862
Download Mac S3 https://explorer-artifacts.decentraland.org/@dcl/unity-explorer/branch/feat/abgen-registry-flag/pr-25154-e91bc9f/Decentraland_macos.zip
Built on 2026-08-14T18:48:50Z

Lint

Warnings not reduced: 13156 => 13156 — remove at least 1 warning to merge.

Warnings/errors in files changed by this PR (1)
Assets/DCL/FeatureFlags/FeatureFlagsStrings.cs:190  CSharpWarnings::CS0618  CS0618: Constant 'DCL.FeatureFlags.FeatureFlagsStrings.GPUI_ENABLED' is obsolete: 'GPU Instancer Pro terrain is no longer optional so the flag is not needed'

Tests

All Unity tests passed ✅

TESTS SUITE Result Passed Failed Skipped
EditMode ✅ Passed 24971 0 13
PlayMode ✅ Passed 236 0 36

@claude

claude Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dalkia's task in 8m 26s —— View job


Security-focused review — dependency + workflow scope

  • Gather the PR diff and separate feature-flag changes from CI/workflow changes
  • STEP 1 — identify dependency changes (manifest/lockfile/binaries/asmdef/build scripts)
  • STEP W — workflow & prompt file review (W.1–W.10)
  • Review the C# feature-flag changes for correctness
  • Post findings + verdict

⚠️ First: the diff doesn't match the PR

The title and description describe a two-file change (FeatureFlagsStrings.cs, DecentralandUrlsSource.cs, +38/-2). The actual diff is 13 files / 760 additions across 10 commits, of which 9 commits and 11 files are unrelated CI work (Unity Cloud build links, the unified status-comment pipeline, a new performance-comment workflow, visual-regression status sections). git log origin/dev..HEAD shows they are not yet in dev, so they merge with this PR.

That matters for two reasons: the abgen change is 5% of the diff and easy to miss, and the CI half carries all of the security surface. Either rebase onto dev once the CI branch lands, or retitle so reviewers know what they're approving.

Also, the new-dependency label looks spurious — there is no Packages/manifest.json, packages-lock.json, binary, native plugin, or .asmdef change anywhere in this diff.


STEP 1 — Dependency changes

Dependency / file Version Source Type Scope Risk Confidence
Packages/manifest.json unchanged none HIGH
Packages/packages-lock.json unchanged none HIGH
Binary / native plugin additions none none HIGH
.asmdef changes none none HIGH
actions/upload-artifact @v6 (tag) GitHub, first-party action CI only LOW HIGH
actions/checkout @v6 (tag) GitHub, first-party action CI only LOW HIGH
peter-evans/repository-dispatch @v4 (tag) third party, pre-existing action CI, consumes PERFORMANCE_TESTING_PAT MEDIUM HIGH
.github/actions/ucb-build-links local in-repo (new) composite action CI only LOW HIGH
scripts/cloudbuild/build.py local in-repo build script CI only, not shipped LOW HIGH

STEP 1.5 (registry metadata): no new registry-sourced package exists in this PR, so there is nothing to look up. STEP 2 A–E / STEP 3: no runtime dependency is added, so there is no new attack surface in the shipped desktop client from a dependency standpoint. Nothing to classify above LOW except the pre-existing third-party action noted below.


STEP W — Workflow findings

No HIGH findings. W.1, W.3, W.4, W.5a/b/c, W.6, W.8a are all N/A or clean — none of the touched workflows invoke an LLM, none read a PR-side file into a prompt, none use pull_request_target, and all the comment writers use the trusted workflow_run pattern (so actions/checkout with no ref: resolves to the default branch, not PR head). W.2 is clean: every new $GITHUB_OUTPUT heredoc uses a randomised delimiter (EOF_$(uuidgen), UCB_EOF_${RANDOM}${RANDOM}_$$). W.9 is clean (retention-days: 7). W.10 is good — the two new visual-regression.yml jobs deliberately narrow to contents: read below the workflow-level contents: write ceiling, and pr-comment-perf.yml declares a minimal three-scope block.

The untrusted-input handling in the new code is genuinely careful and worth calling out: ucb-build-links validates artifact-derived build ids against ^[0-9]+$ and URLs against a host+/builds/<digits> regex before they reach a comment body; the test-failure and lint composers wrap artifact-derived test names in inline code with `/|/newlines stripped; and test.yml now clamps non-finite durations so json.dump can't emit bare NaN. Those are the right instincts.

MEDIUM

W.7a — PERFORMANCE_TESTING_PAT flows into a tag-pinned third-party action. pr-comment-artifact-url.yml:449 (peter-evans/repository-dispatch@v4) receives the PAT. The tag is mutable, so an upstream compromise is picked up on the next run. Pre-existing, but this PR touches that exact step (adds id: perf_dispatch) and adds a second consumer of the same secret right below it, which makes it the natural moment to SHA-pin. Same applies to the first-party @v6 pins, though those are lower concern. (I can't edit files under .github/workflows/ — GitHub App permissions don't allow workflow modifications — so this needs a human commit.)

W.7-adjacent — new PAT exposure surface. The Probe performance PAT expiry step puts secrets.PERFORMANCE_TESTING_PAT into a step env for an inline run: block. The implementation is careful — GH_TOKEN="$PAT" keeps it out of argv, 2>/dev/null suppresses error output, continue-on-error: true prevents it gating the section writes, and /rate_limit spends no quota. $exp is grep+head -1+cut-derived so it can't inject a second $GITHUB_ENV line. No defect found; flagging only because it widens where the PAT is materialised.

Truncation ceiling doesn't add up to its stated rationale. upsert-ci-status.sh:80 truncates a single section at 20000 chars, with the comment "keep one writer … from consuming the whole budget and failing an unrelated section's PATCH with an opaque 422". With five sections, 5 × 20000 = 100000 > GitHub's 65536 limit, so the 422 is still reachable. Either lower the per-section cap to ~12000 or check the assembled NEW_BODY length before the PATCH.

Pre-existing, low escalation: visual-regression.yml's actor gate and TOCTOU. The gate is author_association ∈ {OWNER, MEMBER, COLLABORATOR}, which fails closed for org members with private membership (they resolve as CONTRIBUTOR) — safe, just occasionally annoying. And resolve calls gh api …/pulls/N at run time without comparing head.repo.pushed_at against comment.created_at (W.3), so the reviewed SHA can shift between comment and dispatch. Because the gate already requires write access, this isn't a privilege escalation — noting it for completeness, not as a blocker.

Two concrete CI bugs (both silent)

  1. ucb-build-links job-URL lookup can never match. It selects Build (<target>), but build-unitycloud.yml:549 uses a bare name: Build — GitHub only appends matrix values when name: is omitted. Compare test.yml:542 (name: Test (${{ matrix.testMode }})), which is why the analogous lookup in pr-comment-test-failures.yml works. Every row loses its "· [GitHub job]" half, with no error. → inline comment · Fix this →
  2. _launcher is hardcoded in the artifact name. install_source is a choice over launcher | epic (build-unitycloud.yml:95-102), and the producer interpolates it into the artifact name. For an epic build the download misses and the rows vanish, while the sibling check-build-ran check (startswith("unity_build_info_")) is source-agnostic and still reports build-ran=true. → inline comment

Also minor: comment-skipped now fires on player-artifacts == 'false', so a successful run that produced unity_build_info_* but no Decentraland_* zips renders "Build skipped — no changes detected under Explorer/", which is provably false in that state. Narrow, but the message asserts something the new signal contradicts. And ${#SECTION_BODY} at upsert-ci-status.sh:80 will abort under set -u with a bare "unbound variable" if an external caller sets neither SECTION_BODY nor SECTION_BODY_FILE — worth the same friendly ::error:: treatment the section-name guard just below it got.


The abgen change itself

The design is sound and the caching reasoning holds up. I verified two non-obvious interactions that could have bitten and don't: returning FeatureFlagsDependent makes GatewayUrlsSource.RawUrl's guard (line 152) correctly skip gateway rewriting, so you don't get gateway.decentraland.org/ab-cdn-abgen; and ComposeRegistryUrl inherits that caching, so /entities/versions, /profiles and the world manifest all move with the base instead of pinning a pre-flag host. No hardcoded AB-host allowlist exists elsewhere that would reject the new host.

Three things to resolve — full detail in the inline comment:

  1. The flag is inert on Today, and no restart fixes it. MainSceneLoader seeds an empty flag config at line 234, constructs the urls source at 264, and only fetches real flags at 321 — so flags are always IsEmpty at construction. On Today the ctor pre-resolves the registry/CDN through ResolveOptimizedAssetsUrl's isTodayEnvironment branch, which returns a bare string (CacheBehaviour.Static) and gets cached permanently as https://ab-cdn.decentraland.today. The rollout note in the description ("restart between flag changes") doesn't cover this.
  2. InsertAbgenSubdomain fails open silently. Unanchored Replace returns the input untouched for any non-*.decentraland.* host, so the flag is ignored with nothing logged; the doc comment says "leading subdomain" but a global Replace would rewrite a .decentraland. inside a path too. Both current call sites are safe — a guard keeps it that way.
  3. -abgen hosts are synthesised for every environment, including .zone and .today, but the description only names .org siblings. If abgen is .org-only, enabling on zone breaks the AB surface at DNS — and zone is the natural place to start a per-wallet rollout.

Missing tests. Explorer/Assets/DCL/NetworkDefinitions/Browser/Tests/DecentralandUrlsSourceShould.cs already covers this resolver thoroughly — 15 tests including ApplyFlagArrivingAfterConstructionTimeProbing, ApplyFlagArrivingAfterEarlyResolutionOfRegistryComposedUrls, KeepTodayConstructionPinsWhenFlagsArriveLater, and gateway-interaction cases. The abgen path adds zero. InitializeFeatureFlags just needs an abgenRegistry parameter and the three interactions above become one-liners. This is the cheapest possible place to pin down precedence-over-OPTIMIZED_ASSETS, gateway skip, and the Today behaviour. Fix this →


Verdict

No high-risk dependency or workflow issue. There is no new runtime dependency, no binary, and no new secret-exposure or prompt-injection path — the workflow additions handle untrusted artifact input with real care. What holds this back from a clean pass is the pre-existing tag-pinned action carrying PERFORMANCE_TESTING_PAT, the two silent CI bugs above, and the unresolved rollout questions on the abgen host (Today inertness, non-.org environments) plus the absent test coverage in a file already built for exactly these cases.

Unknowns that need a human: whether -abgen siblings are deployed per environment or .org-only, and whether the abgen CDN serves the same /vNN/<entityId>/<hash>_<platform> layout that AssetBundleManifestVersion assumes.

DEPENDENCY_REVIEW: NEEDS_ATTENTION

--- · branch feat/abgen-registry-flag

target="${entry%%:*}"
label="${entry#*:}"
parse_info "$target"
job_url=$(jq -r --arg n "Build ($target)" '.jobs[]? | select(.name==$n) | .html_url // empty' <<< "$JOBS_JSON" | head -1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lookup can never match — the "· [GitHub job]" half of every row is silently dropped.

build-unitycloud.yml:549 names the matrix job with a bare literal:

  build:
    name: Build
    strategy:
      matrix:
        target: ${{ fromJSON(needs.prebuild.outputs.targets) }}

GitHub only appends matrix values to the display name when name: is omitted. With an explicit name: Build, every leg is reported as exactly Build, so select(.name=="Build (windows64)") yields nothing and job_url is always empty.

Contrast test.yml:542, which is why the analogous lookup in pr-comment-test-failures.yml works:

    name: Test (${{ matrix.testMode }})

Two ways out — either interpolate the target into the producer's job name (name: Build (${{ matrix.target }}) in build-unitycloud.yml), or match on the matrix leg instead of the display name here, e.g. select(.name | startswith("Build")) | select(.steps? // [] | ...). The first is cleaner and matches the existing convention. Note this fails silently (empty cell), so it won't show up as a red job.

REPLY_URL=""
if gh run download "$RUN_ID" \
--repo "$REPO_FULL" \
--name "unity_build_info_${target}_launcher" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_launcher is hardcoded, but install_source is an input with a second legal value.

The producer names the artifact with the resolved input (build-unitycloud.yml:986):

name: unity_build_info_${{ matrix.target }}_${{ needs.prebuild.outputs.install_source }}

and install_source is a choice over launcher | epic (build-unitycloud.yml:95-102), defaulting to launcher. For an epic build the download misses, parse_info emits only a note:, and the Unity Cloud rows vanish with no signal.

Worth noting the sibling check in pr-comment-artifact-url.yml is already source-agnostic (startswith("unity_build_info_")), so build-ran goes true while this action finds nothing — the two halves disagree. Suggest resolving the name by prefix instead of assuming the suffix:

name=$(gh api "/repos/$REPO_FULL/actions/runs/$RUN_ID/artifacts?per_page=100" \
  --jq --arg p "unity_build_info_${target}_" '[.artifacts[].name | select(startswith($p))][0] // empty')

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: feat: abgen-registry feature flag to switch AB registry + CDN

Step 2 — Root-cause check

PASS. This is a feature addition, not a bug fix. The PR correctly adds a URL resolution seam to route asset-bundle traffic to a parallel abgen pipeline via a feature flag. The approach — inserting a check in the existing URL resolver — addresses the requirement directly.

Step 3 — Design & integration

PASS. DecentralandUrlsSource is the established owner of all AB URL routing (ResolveOptimizedAssetsUrl, ComposeRegistryUrl, and the RawUrl switch). The new ResolveAssetBundleUrl is a private method on this same class — no new units, no new lifecycle, no persistent state introduced. It composes cleanly with the existing resolution chain:

  • CLI override (--optimized-assets-url) → abgen flagoptimized-assets flagdefault

The duplicate CLI override check in ResolveAssetBundleUrl (also present in ResolveOptimizedAssetsUrl) is warranted: without it, the abgen flag would take precedence over the CLI override when the flag is enabled, which contradicts the stated precedence. The fallback ResolveOptimizedAssetsUrl would catch it only when abgen is off.

Owner search: AssetBundlesCDN and AssetBundleRegistry URLs are resolved in DecentralandUrlsSource.RawUrl — this is their single owner. ResolveAssetBundleUrl slots into the resolution pipeline at the same layer as the existing ResolveOptimizedAssetsUrl. LodGeneratorCDN correctly stays on ResolveOptimizedAssetsUrl (abgen LOD lane is unimplemented).

Teardown trace: No subscriptions, event hookups, buffers, or resources introduced — nothing to unsubscribe or dispose.

ℹ️ Today-environment note (informational, pre-existing): In the isTodayEnvironment path, the constructor eagerly calls Url(AssetBundleRegistry) and Url(AssetBundlesCDN) before flags load. ResolveOptimizedAssetsUrl returns a Static UrlData for today when flags are empty, so these URLs get permanently cached with the "today" domain. The abgen flag (like OPTIMIZED_ASSETS) cannot take effect in this environment — this is a pre-existing architectural choice (today pins AB URLs during construction), not a regression introduced by this PR.

Step 4 — Member audit

Member Visibility Consumers Verdict
ABGEN_REGISTRY public const ResolveAssetBundleUrl (1 in diff) + feature-flags service (external) Correctly follows existing constant pattern
ResolveAssetBundleUrl private RawUrl switch: AssetBundlesCDN, AssetBundleRegistry (2) Appropriate scope, two consumers
InsertAbgenSubdomain private static ResolveAssetBundleUrl (1) Single-use but names the transform for readability; the method is a 1-line expression body — inlining would save nothing and lose the descriptive name

No single-use merge, absent≠false, or redundant-guard issues.

Step 5 — Line-level review

Pass A (blocking issues): None found.

  • Code quality: Follows CLAUDE.md standards. No LINQ, no allocations in the resolution path, no Debug.Log.
  • Bugs: InsertAbgenSubdomain uses String.Replace(".decentraland.", "-abgen.decentraland."). Both input URLs (https://ab-cdn.decentraland.{ENV}, https://asset-bundle-registry.decentraland.{ENV}) contain exactly one .decentraland. occurrence, and the {ENV} token is preserved for later substitution in Url(). Correct.
  • Security: URL rewriting is based on a server-controlled feature flag, not user input. No injection surface.
  • Performance: ResolveAssetBundleUrl is called during URL resolution (not per-frame). Single String.Replace call — negligible cost.
  • Nullability: dedicatedHostUrl is always a string interpolation literal from RawUrl; never null. FeatureFlagsConfiguration.Instance follows existing access pattern throughout the class.
  • Caching: FeatureFlagsDependent correctly prevents caching until flags load, then the Url() method resolves {ENV} and caches permanently. Consistent with existing flag-driven URL resolution.

Pass B (design smells): None found.

  • No new types, no constructors taking raw materials, no lazy initialization patterns.
  • Names are descriptive: ResolveAssetBundleUrl describes responsibility, InsertAbgenSubdomain describes the transform.
  • XML doc comments explain what the code does (not what callers do) and document intentional exclusions (LODs).

CI / Workflow changes

The PR bundles extensive CI improvements (new performance and automation sections in the unified status comment, Unity Cloud build links, performance dispatch/failure tracking, visual regression automation, test result enrichment with durations and slowest tests). These are orthogonal to the feature flag but well-implemented:

  • Defensive: upsert-ci-status.sh truncates bodies at 20k chars with proper fence-closing, validates section names, handles missing section fences in older comments.
  • Secure: ucb-build-links validates artifact URLs against a strict regex pinned to Unity Cloud domains with /builds/<digits>. build.py validates dashboard URLs with startswith('https://') and '/builds/' in candidate.
  • Robust: test.yml guards against NaN/Infinity in test durations with math.isfinite() before JSON serialization.

Security review

No security issues found.

  • No hardcoded secrets or credentials
  • No user-input-driven URL construction in the C# code
  • Shell scripts sanitize untrusted artifact content before rendering in PR comments
  • URL validation in ucb-build-links and build.py is adequate for the threat model

Step 6 — Complexity

COMPLEX — touches asset-bundle URL resolution (affects asset loading pipeline), 13 files changed across runtime C#, CI workflows, build scripts, and a new composite action.

Step 7 — QA

YES — the C# changes modify runtime URL resolution that determines which hosts serve asset bundles. Enabling the abgen-registry flag should be tested to verify AB loading works correctly against the -abgen hosts.

Step 8 — Warnings

No warnings. Main.unity is not modified.

Step 9 — Verdict

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies asset-bundle URL resolution in DecentralandUrlsSource (runtime) plus 10+ CI workflow files
QA_REQUIRED: YES


Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub

Comment on lines +226 to +247
private UrlData ResolveAssetBundleUrl(string dedicatedHostUrl)
{
if (optimizedAssetsBaseOverride is { Length: > 0 })
return new UrlData(CacheBehaviour.FeatureFlagsDependent, optimizedAssetsBaseOverride);

FeatureFlagsConfiguration featureFlags = FeatureFlagsConfiguration.Instance;

// FeatureFlagsDependent so the abgen host is re-resolved (not cached) until flags load — the
// {ENV} token is substituted in Url() once the environment domain is applied.
if (!featureFlags.IsEmpty && featureFlags.IsEnabled(FeatureFlagsStrings.ABGEN_REGISTRY))
return new UrlData(CacheBehaviour.FeatureFlagsDependent, InsertAbgenSubdomain(dedicatedHostUrl));

return ResolveOptimizedAssetsUrl(dedicatedHostUrl);
}

/// <summary>
/// Inserts the `-abgen` label into the leading subdomain of an AB host, leaving the {ENV} token
/// intact: `https://ab-cdn.decentraland.{ENV}` → `https://ab-cdn-abgen.decentraland.{ENV}`,
/// `https://asset-bundle-registry.decentraland.{ENV}` → `https://asset-bundle-registry-abgen.decentraland.{ENV}`.
/// </summary>
private static string InsertAbgenSubdomain(string hostUrl) =>
hostUrl.Replace(".decentraland.", "-abgen.decentraland.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things worth resolving before rollout.

1. The flag is permanently inert on the Today environment.

MainSceneLoader seeds an empty config before building the urls source, and only fetches real flags afterwards:

234:  FeatureFlagsConfiguration.Initialize(new FeatureFlagsConfiguration(FeatureFlagsResultDto.Empty));
264:  var decentralandUrlsSource = new GatewayUrlsSource(…);
321:  await bootstrap.InitializeFeatureFlagsAsync(…);

So flags are always IsEmpty at construction. On Today, the ctor eagerly pre-resolves AssetBundleRegistry / AssetBundlesCDN (lines 72-74) to pin them to .today before the domain flips to org. With flags empty, ResolveAssetBundleUrl falls through to ResolveOptimizedAssetsUrl, hits the isTodayEnvironment branch, and returns a bare string — i.e. CacheBehaviour.Static — which Url() then caches forever as https://ab-cdn.decentraland.today.

Net effect: on Today, abgen-registry does nothing, and no restart helps (the existing KeepTodayConstructionPinsWhenFlagsArriveLater test encodes exactly this pin for optimized-assets). That's arguably fine, but the PR description says only "the client must be restarted between flag changes" — if Today is where this gets validated first, the rollout plan needs a caveat, or the today branch needs an abgen-aware pin.

2. InsertAbgenSubdomain fails open, silently.

Replace is unanchored and returns the input unchanged when the marker is absent, so any host that isn't *.decentraland.* (a localhost sidecar, a future abcdn-style consolidated host, an assets.example.com variant payload) silently ignores the flag with nothing logged. The doc comment also says "leading subdomain", but a global Replace would rewrite every .decentraland. occurrence, including one inside a path. Both call sites are safe today; a guard makes it stay that way:

private const string ABGEN_MARKER = ".decentraland.";

private static string InsertAbgenSubdomain(string hostUrl)
{
    int marker = hostUrl.IndexOf(ABGEN_MARKER, StringComparison.Ordinal);

    if (marker < 0)
    {
        ReportHub.LogWarning(ReportCategory.STARTUP, $"abgen-registry is on but '{hostUrl}' is not an env-shaped decentraland host; leaving it untouched.");
        return hostUrl;
    }

    return string.Concat(hostUrl.AsSpan(0, marker), "-abgen", hostUrl.AsSpan(marker));
}

3. -abgen hosts are synthesised for every environment. The description names only .org siblings, but this also produces asset-bundle-registry-abgen.decentraland.zone / .today. If the abgen deployment is .org-only, enabling the flag on zone breaks the whole AB surface at DNS rather than degrading — and zone is where a per-wallet rollout would normally start. Worth confirming the sibling exists per environment.

One thing that does check out: because the abgen branch returns FeatureFlagsDependent, GatewayUrlsSource.RawUrl's guard at line 152 correctly skips gateway rewriting, so you don't end up requesting gateway.decentraland.org/ab-cdn-abgen. And ComposeRegistryUrl inherits that caching, so the derived endpoints move with the base and aren't pinned pre-flag. 👍

@decentraland-bot

Copy link
Copy Markdown
Contributor

PR #9757, run #31830453193

Builds: Windows change, Windows baseline, macOS change, macOS baseline

How to read this table
  • Each build is measured 3 times. The values are the median, and (min–max) is the lowest and highest of those runs — a wide range means the metric is noisy and small differences are not trustworthy.
  • Δ is Change minus Baseline (a negative Δ means Change is faster).
  • 🟢 faster / 🔴 slower — a real difference: larger than both 3% and the run-to-run range.
  • ⚪ within noise — the difference is smaller than how much the build varies between its own runs, so it cannot be told apart from random variation. Treat it as no change.
  • Exceptions per run — the average number of exceptions in a run's log; more than the baseline is flagged 🔴 even when frame times look fine. The Exception breakdown under each table groups them by the explorer's report category and exception type (as totals across the runs).
  • A run that logged unusually many exceptions (at least 10 and 5× the median of its build's runs — e.g. a service was down during it) is excluded from all numbers and called out under the table.

Apple M1

Metric Baseline Change Δ Result
Samples 4368 (×3) 4264 (×3)
CPU average 20.5 ms (20.3–21.6) 21.0 ms (20.9–21.2) 0.5 ms ⚪ within noise
CPU 1% worst 34.7 ms (33.9–34.7) 34.5 ms (34.3–34.7) -0.2 ms ⚪ within noise
CPU 0.1% worst 34.9 ms (34.9–35.3) 34.9 ms (34.8–35.1) -0.1 ms ⚪ within noise
GPU average 1.0 ms (0.1–1.6) 0.1 ms (0.0–5.1) -0.9 ms ⚪ within noise
GPU 1% worst 34.2 ms (7.7–34.8) 11.7 ms (4.5–36.2) -22.5 ms ⚪ within noise
GPU 0.1% worst 35.9 ms (35.1–37.2) 36.7 ms (34.3–37.3) 0.8 ms ⚪ within noise
Exceptions per run 0 0 0 ⚪ none new

⚠️ Intel Core i5 failed to produce results — see the run for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants