Skip to content

refactor: serialize local-ab sidecar startup and drop the plugin bridges - #9831

Merged
dalkia merged 8 commits into
devfrom
refactor/local-ab-serial-sidecar
Sep 1, 2026
Merged

refactor: serialize local-ab sidecar startup and drop the plugin bridges#9831
dalkia merged 8 commits into
devfrom
refactor/local-ab-serial-sidecar

Conversation

@dalkia

@dalkia dalkia commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Pull Request Description

What does this PR change?

Restructures how the --local-ab abgen sidecar (#9704) is owned and started, eliminating the plugin indirection and the guess-then-correct override handling. Supersedes the fallback approach of item 2 in #9756 (ClearOptimizedAssetsOverride) — that method is never introduced here because the situation it corrected no longer exists.

Out of plugins. AbgenSidecarPlugin injected zero systems — InjectToWorld/Dispose were used purely as start/stop hooks — but its placement forced two bridges: the reserved URL threaded in via BootstrapContainer.LocalAbBaseUrl, and readiness threaded back out via DynamicWorldContainer.AbgenSidecarReadyAsync. Both are deleted. The lifecycle now lives in a plain AbgenSidecarBootstrap owned end to end by MainSceneLoader (DynamicWorldContainer has zero abgen involvement left).

Serial health-first startup. Previously the override was seeded at URL-source construction on the hope the server would come up, and a launch failure left a dead loopback port as the session's optimized-assets base (scene degrading per request, wearables/emotes losing bundles). Now the splash is shown first, then StartAsync brings the server to health serially — binary download on first run, launch, health poll — before the URL sources are built. The override is seeded only when the server is provably serving; on failure it is simply never set, so the session is byte-for-byte production with no correction step anywhere. The whole-scene warm-up still overlaps the rest of boot (WarmUpTask), and realm loading holds on it as before.

Fail fast on a dead child. The health poll previously sat out the full 15s timeout even when the child process had already exited (the realistic failure: bad binary, port bind failure). WaitHealthyAsync now checks ChildAlive() between polls — race-free since supervision only starts after health passes — dropping that case to ~250ms. A hung-but-alive server still pays the 15s, under a visible splash.

Disposal hardening for free. The child is killed from MainSceneLoader.Shutdown() (registered as a quit-cleanup candidate), which covers boot failures before the world/plugins ever built — the plugin's Dispose only ran if plugin teardown was reached.

Pin bump to abgen v0.17.11. Carries abgen#108 (corpus entries hard-link to the bundle cache instead of being copied — the LSD cache's physical footprint drops to ~half its apparent size, and hot-reload corpus rebuilds become metadata-only relinks) and abgen#110 (metadata deps follow the per-platform CDN casing contract). All four archive SHA-256s come from the release's published SHA256SUMS.txt; the darwin-arm64 archive was additionally downloaded, hashed and version-checked independently before pinning.

Net: one class instead of a plugin plus two container bridges, and one fewer failure mode.

Test Instructions

Steps (standard run):

metaforge explorer run 9831

Expected result:
Normal boot into Genesis City — the vanilla path is untouched apart from the splash screen appearing slightly earlier in boot.

Prerequisites

Test Steps

  1. Start the scene's preview server without the web client:
    npm run start -- --no-client
  2. Launch the local build against it with local asset bundles on:
    open Decentraland.app --args --realm http://127.0.0.1:8000 --local-scene true --local-ab
  3. Open the asset-bundles window (debug menu → AB conversion panel): the scene's files convert during warm-up (first run also shows the abgen v0.17.11 binary download) and end up built.
  4. Press the check button in the window: entries report green (served as locally converted bundles).
  5. Confirm the LSD cache footprint (the abgen#108 dedup):
    du -sh ~/Library/Application\ Support/Decentraland/Explorer/abgen-lsd
    should report roughly half of what Finder shows for the same folder (~0.9 GB real vs ~1.8 GB apparent for central-plaza) — corpus and bundle-cache entries are hard links to the same bytes.
  6. Relaunch without --local-ab and press the check button again: entries report red (no local bundles — raw-GLTF path), and no abgen process is spawned.

Additional Testing Notes

  • Failure fallback is the core of this PR: replace the pinned binary with a corrupt file (or block the download) and relaunch with --local-ab — boot continues after a short splash hold and the session behaves exactly as without the flag (production bundles, no dead-port requests in the logs). Worst-case splash hold is 15s (hung-but-alive server); a dead child fails in ~250ms.
  • Quit the explorer mid-session with the sidecar running: the abgen child process is killed (check the process list).
  • macOS and Windows both worth a pass; the pin bump changes the downloaded binary on first run.

Quality Checklist

  • Changes have been tested locally
  • Documentation has been updated (if required)
  • Performance impact has been considered
  • For SDK features: Test scene is included

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

@dalkia
dalkia requested review from a team as code owners August 21, 2026 12:54
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

🚦 CI Status

Build

New build in progress, come back later!

Lint

Warnings count reduced: 12183 => 12179

Warnings/errors in files changed by this PR (1)
Assets/DCL/Infrastructure/Global/Dynamic/BootstrapContainer.cs:182  CSharpWarnings::CS8604  Possible null reference argument for parameter 'identityCache' in 'DCL.PerformanceAndDiagnostics.Analytics.AnalyticsContainer.CreateAsync'

Tests

All Unity tests passed ✅

TESTS SUITE Result Passed Failed Skipped
EditMode ✅ Passed 25530 0 13
PlayMode ✅ Passed 248 0 37

@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.

Review — PR #9831: refactor: serialize local-ab sidecar startup and drop the plugin bridges

Step 1 — Context & Scope

Diff: 9 files changed (+166 −188, net −22 lines). New class AbgenSidecarBootstrap replaces AbgenSidecarPlugin and two container bridge properties (BootstrapContainer.LocalAbBaseUrl, DynamicWorldContainer.AbgenSidecarReadyAsync). Pin bump to abgen v0.17.1 with updated SHA-256 checksums. Fail-fast ChildAlive() check added to WaitHealthyAsync.

Surrounding files read: AbgenSidecar.cs (full, 831 lines), MainSceneLoader.cs (full, 1036 lines), BootstrapContainer.cs, DynamicWorldContainer.cs, RealmUrls.cs, RealmLaunchSettings.cs, AbgenSidecarShould.cs (test).

Repo searches: rg AbgenSidecarPlugin (0 remaining refs ✓), rg LocalAbBaseUrl (0 remaining refs ✓), rg AbgenSidecarReadyAsync (0 remaining refs ✓), rg AbgenSidecarBootstrap (3 refs — class def, MainSceneLoader field + constructor, AbgenSidecar doc comment ✓), rg LocalSceneDevelopmentRealmAsync (5 refs — unchanged consumers outside this diff ✓).


Step 2 — Root-cause check: PASS ✅

The diff fixes the real design flaw: the old code seeded the optimized-assets URL at URL-source construction time on the hope the sidecar would start, and a launch failure left a dead loopback port poisoning every AB request for the session. The new serial approach seeds the URL only after the server is provably healthy; on failure the override is simply never set, so the session is byte-for-byte production. This addresses the cause, not a symptom.


Step 3 — Design & integration: PASS ✅

Owner search — AbgenSidecarBootstrap:
The new class owns the abgen child process lifecycle (binary resolution, launch, health poll, warm-up, kill). Previously this lived in AbgenSidecarPlugin which abused InjectToWorld/Dispose as start/stop hooks without injecting any ECS systems — the plugin indirection was unjustified (CLAUDE.md §11: bridge/wrapper on same abstraction layer). The lifecycle is now owned by MainSceneLoader, which already orchestrates the entire boot sequence. Files searched: DynamicWorldContainer.cs (no abgen involvement left), BootstrapContainer.cs (bridge property deleted), AbgenSidecarPlugin.cs (deleted). Verdict: AbgenSidecarBootstrap is a genuine responsibility split — it encapsulates OS process management (~95 lines) so MainSceneLoader doesn't grow a second concern. Not a bridge/wrapper anti-pattern.

Realm resolution simplification:
The new code uses launchSettings.initialRealm == InitialRealm.Localhost ? IRealmNavigator.LOCALHOST : launchSettings.customRealm instead of the full RealmUrls.LocalSceneDevelopmentRealmAsync(ct). Verified via RealmLaunchSettings.cs: LSD mode is entered only when isLocalSceneDevelopmentRealm is true (CLI: SetLocalSceneDevelopmentRealm sets initialRealm = Custom and customRealm = targetRealm, with ExternalUrlPolicy.IsWebScheme enforced) OR initialRealm == Localhost (editor convenience). Both branches are exactly covered. Name resolution via realmNames.UrlFromNameAsync is unreachable (CLI requires web-scheme realm). Correct simplification.

Teardown/consumption trace:

  • lifeCycleCancellationTokenSource → cancelled and disposed in AbgenSidecarBootstrap.Dispose() via SafeCancelAndDispose()
  • sidecar (AbgenSidecar, IDisposable) → disposed in AbgenSidecarBootstrap.Dispose()
  • AbgenSidecarBootstrap itself → disposed in MainSceneLoader.Shutdown() line 164 ✓ (covers boot failures before plugins built)
  • WarmUpTask → awaited in MainSceneLoader.InitializeFlowAsync line 429-430 before realm loading ✓

Step 4 — Member audit: PASS ✅

New public members in AbgenSidecarBootstrap:

Member Consumers Assessment
BaseUrl (string) 1 (MainSceneLoader:287) Encapsulates the reserved loopback endpoint. Not a single-use-to-merge — it's the class's identity, read conditionally after StartAsync succeeds.
WarmUpTask (UniTask) 1 (MainSceneLoader:430) Exposes the background warm-up for the boot sequence to await. Pre-completed on failure path — safe to await unconditionally.
StartAsync(string) 1 (MainSceneLoader:286) Main API — brings server to health serially. Returns bool, never faults.
Dispose() 1 (MainSceneLoader:164) IDisposable contract.

All members are justified; none are single-use intermediaries that should be merged.


Step 5 — Line-level review: PASS ✅

No blocking issues found.

All changed lines checked against the blocking-issue categories:

  1. Code quality / CLAUDE.md: PascalCase types/methods/properties, camelCase locals ✓. SafeCancelAndDispose() for CTS ✓. ReportHub.LogException for error logging ✓. No LINQ ✓. No Debug.Log ✓. Comments describe what the code does, not caller behavior ✓.
  2. Bugs / runtime errors: None found. Exception handling in StartAsync and WarmUpAsync catches OperationCanceledException separately ✓. WarmUpTask is pre-completed (UniTask.CompletedTask) so never-started paths are safe ✓.
  3. Security: No new attack surface. Binary verification via SHA-256 unchanged. Loopback-only binding unchanged.
  4. Performance: No hot-path changes. ChildAlive() check in WaitHealthyAsync is a ~250ms fast exit improvement.
  5. Error handling: All failure paths return false (no faulting UniTasks). Exceptions logged via ReportHub ✓.
  6. Resource/subscription leaks: CTS created → cancelled+disposed in Dispose ✓. AbgenSidecar created → disposed in Dispose ✓. No subscriptions or event handlers added.
  7. Detached async: WarmUpTask is stored and awaited by MainSceneLoader before realm loading ✓. No essential .Forget() calls introduced.
  8. Nullability: AbgenSidecar? sidecar correctly nullable (null before creation and after failed creation) ✓. AbgenSidecarBootstrap? abgenSidecar in MainSceneLoader correctly nullable (null when not in local-ab mode) ✓.

ChildAlive() in WaitHealthyAsync (AbgenSidecar.cs:731-732): Correctly placed between the HTTP poll failure and the delay — checks process liveness before sleeping again. Race-free because supervision (SuperviseAsync) only starts after WaitHealthyAsync returns true. Good improvement.

Security notes (pre-existing, not introduced by this PR):

  • Low: ExtractTarGz guards .. but not absolute paths (e.g. /etc/foo). Mitigated by SHA-256 pinning.
  • Low: Environment.SetEnvironmentVariable mutates the parent process env (all ABGEN_-prefixed). Acceptable for dev-only tool.

Step 6 — Complexity: COMPLEX

Touches plugin/container wiring, async lifecycle management, process spawning, and the boot sequence across 9 files.

Step 7 — QA: YES

Modifies runtime boot code that controls asset-bundle URL source selection and sidecar process lifecycle. Affects what the user sees in local-AB development mode (splash screen timing, failure behavior).

Step 8 — Non-blocking warnings

None. Main scene file not modified.

Step 9 — Verdict

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies plugin/container wiring, async boot sequence, and child-process lifecycle management across the infrastructure and plugin-system layers
QA_REQUIRED: YES


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

@decentraland-bot

This comment has been minimized.

@decentraland-bot

This comment has been minimized.

@dalkia
dalkia marked this pull request as draft August 21, 2026 18:53
dalkia and others added 2 commits August 27, 2026 13:36
The abgen sidecar was mounted as a global plugin, which forced two
bridges: the reserved URL threaded in via BootstrapContainer.LocalAbBaseUrl
and readiness threaded back out via DynamicWorldContainer. It also seeded
the optimized-assets override before the server's fate was known, requiring
a correction step (ClearOptimizedAssetsOverride) on failure.

MainSceneLoader now owns the lifecycle directly: AbgenSidecarBootstrap is
brought up to health serially, under the splash, before the URL sources are
built — the override is seeded only when the server is provably serving, so
a failed launch leaves the session byte-for-byte production with no
correction step anywhere. The whole-scene warm-up runs in the background
and realm loading holds on it. The health poll now fails fast when the
child process dies instead of sitting out the 15s timeout.

Also bumps the pinned abgen to v0.17.1 (archive hashes verified locally
against the release).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dalkia
dalkia force-pushed the refactor/local-ab-serial-sidecar branch from 2a6d98f to 31ebafb Compare August 27, 2026 16:41
dalkia and others added 3 commits August 27, 2026 13:54
Archive hashes computed locally from the downloaded artifacts; all four
match the release's published SHA256SUMS.txt.

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

The extract/install lambda runs on the thread pool but evaluated
Application.persistentDataPath (and Application.platform via IsWindows)
inside it, so every first-run binary download failed at the install step
with "get_persistentDataPath can only be called from the main thread".
Latent since #9704 — only the download path hits it, so it surfaces on
first runs and pin bumps.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dalkia
dalkia marked this pull request as ready for review August 31, 2026 16:38
@github-actions
github-actions Bot requested a review from DafGreco August 31, 2026 16:38
@decentraland-bot
decentraland-bot self-requested a review August 31, 2026 16:38
@github-actions
github-actions Bot requested a review from pravusjif August 31, 2026 16:38

@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.

Review — PR #9831: refactor: serialize local-ab sidecar startup and drop the plugin bridges

Step 1 — Context & Scope

Diff: 9 files changed (+180 −201, net −21 lines). New class AbgenSidecarBootstrap replaces AbgenSidecarPlugin and two container bridge properties (BootstrapContainer.LocalAbBaseUrl, DynamicWorldContainer.AbgenSidecarReadyAsync). Pin bump to abgen v0.17.8 with updated SHA-256 checksums for all four platform targets. Fail-fast ChildAlive() check added to WaitHealthyAsync. Main-thread Unity API fix in DownloadAndInstallAsync.

Surrounding files read: AbgenSidecar.cs (full, ~830 lines — constructor, Launch, LaunchChild, WaitHealthyAsync, SuperviseAsync, DownloadAndInstallAsync, ExtractTarGz), MainSceneLoader.cs (full — InitializeFlowAsync, Shutdown), BootstrapContainer.cs, DynamicWorldContainer.cs, RealmUrls.cs (full — StartingRealmAsync, LocalSceneDevelopmentRealmAsync, CustomRealmAsync), RealmLaunchSettings.cs, IRealmNavigator.cs (LOCALHOST constant).

Repo searches: rg AbgenSidecarPlugin (0 remaining refs ✓), rg LocalAbBaseUrl (0 remaining refs ✓), rg AbgenSidecarReadyAsync (0 remaining refs ✓), rg AbgenSidecarBootstrap (3 refs — class def, MainSceneLoader field + constructor, AbgenSidecar doc comment ✓), rg LocalSceneDevelopmentRealmAsync (5 refs — unchanged consumers outside this diff ✓).


Step 2 — Root-cause check: PASS ✅

The diff fixes the real design flaw: the old code seeded the optimized-assets URL at URL-source construction time on the hope the sidecar would start, and a launch failure left a dead loopback port poisoning every AB request for the session. The new serial approach seeds the URL only after the server is provably healthy (StartAsync returns true); on failure the override is simply never set, so the session is byte-for-byte production. This addresses the cause, not a symptom.


Step 3 — Design & integration: PASS ✅

Owner search — AbgenSidecarBootstrap:
The new class owns the abgen child process lifecycle (binary resolution, launch, health poll, warm-up, kill). Previously this lived in AbgenSidecarPlugin which abused InjectToWorld/Dispose as start/stop hooks without injecting any ECS systems — the plugin indirection was unjustified (CLAUDE.md §11: bridge/wrapper on same abstraction layer). The lifecycle is now owned by MainSceneLoader, which already orchestrates the entire boot sequence. Files searched: DynamicWorldContainer.cs (no abgen involvement left), BootstrapContainer.cs (bridge property deleted), AbgenSidecarPlugin.cs (deleted). Verdict: AbgenSidecarBootstrap is a genuine responsibility split — it encapsulates OS process management (~96 lines) so MainSceneLoader doesn't grow a second concern. Not a bridge/wrapper anti-pattern.

Realm resolution simplification:
The new code uses launchSettings.initialRealm == InitialRealm.Localhost ? IRealmNavigator.LOCALHOST : launchSettings.customRealm instead of the full RealmUrls.LocalSceneDevelopmentRealmAsync(ct). Verified via RealmUrls.cs: LocalSceneDevelopmentRealmAsync delegates to StartingRealmAsync, which switches on initialRealm. For the Custom branch, CustomRealmAsync checks ExternalUrlPolicy.IsWebScheme and returns the realm directly if it's a web URL. Since LSD mode via CLI requires IsWebScheme validation before entering, realmNames.UrlFromNameAsync is unreachable in this path. The simplification is also necessary: RealmUrls requires IDecentralandUrlsSource and IRealmNamesMap, neither of which exist at the point where the sidecar must start. Correct simplification. (See P2 comment for drift-risk note.)

Teardown / consumption trace:

  • lifeCycleCancellationTokenSource → cancelled and disposed in AbgenSidecarBootstrap.Dispose() via SafeCancelAndDispose()
  • sidecar (AbgenSidecar, IDisposable) → disposed in AbgenSidecarBootstrap.Dispose()
  • AbgenSidecarBootstrap itself → disposed in MainSceneLoader.Shutdown() line 173, AFTER dynamicWorldContainer/staticContainer (no live references) but BEFORE bootstrapContainer — dependency-safe since bootstrap only captures a DecentralandEnvironment enum value ✓
  • WarmUpTask → awaited in MainSceneLoader.InitializeFlowAsync line ~490 before realm loading; pre-completed on failure path (safe to await unconditionally) ✓

Step 4 — Member audit: PASS ✅

New public members in AbgenSidecarBootstrap:

Member Consumers Assessment
BaseUrl (string) 1 (MainSceneLoader:339) The class's identity — the loopback endpoint, fixed at construction. Not a single-use intermediate.
WarmUpTask (UniTask) 1 (MainSceneLoader:490) Exposes the background warm-up for the boot sequence to await. Pre-completed on failure path — safe to await unconditionally.
StartAsync(string) 1 (MainSceneLoader:338) Main API — brings server to health serially. Returns bool, never faults.
Dispose() 1 (MainSceneLoader:173) IDisposable contract.

All members are justified; none are single-use intermediaries that re-check invariants.


Step 5 — Line-level review: PASS ✅

No blocking issues found.

All changed lines checked against the blocking-issue categories:

  1. Code quality / CLAUDE.md: PascalCase types/methods/properties, camelCase locals ✓. SafeCancelAndDispose() for CTS ✓. ReportHub.LogException for error logging ✓. No LINQ ✓. No Debug.Log ✓. #nullable enable removal is correct — nullable is enabled project-wide (verified: DynamicWorldContainer.cs and AbgenSidecarBootstrap.cs use nullable annotations without the directive) ✓. using System.Diagnostics moved from #if UNITY_EDITOR to unconditional — correct, Stopwatch is available on all platforms and Process usage is still behind #if UNITY_EDITOR ✓.
  2. Bugs / runtime errors: None found. Launch() parameter removal is safe (uses readonly executablePath field set in constructor). ChildAlive() placement is race-free (supervision only starts after health passes). WarmUpAsync is static to avoid capturing this. Exception handling catches OperationCanceledException separately ✓.
  3. Security: No new attack surface introduced. All pre-existing observations (tar extraction rooted-path guard, StreamingAssets override, global env-var mutation) are outside this PR's scope and mitigated by SHA-256 pinning. Binary verification via SHA-256 unchanged. Loopback-only binding unchanged.
  4. Performance: No hot-path changes. ChildAlive() check is a ~250ms fast-exit improvement for dead-child detection.
  5. Missing error handling: All failure paths return false (no faulting UniTasks). Exceptions logged via ReportHub ✓.
  6. Resource / subscription leaks: CTS created → cancelled+disposed in Dispose ✓. AbgenSidecar created → disposed in Dispose ✓. No subscriptions or event handlers added.
  7. Detached async: WarmUpTask is stored and awaited by MainSceneLoader before realm loading ✓. SuperviseAsync(ct).Forget() in AbgenSidecar.StartAsync is pre-existing and appropriately fire-and-forget (monitoring, not essential setup) ✓.
  8. Nullability: AbgenSidecar? sidecar correctly nullable (null before creation and after failed creation) ✓. AbgenSidecarBootstrap? abgenSidecar in MainSceneLoader correctly nullable (null when not in local-ab mode) ✓.
  9. Main-thread fix: Application.persistentDataPath and IsWindows (which reads Application.platform) resolved before DCLTask.RunOnThreadPool — verified no other main-thread Unity APIs remain inside the thread-pool block (only standard .NET IO: Directory.Exists, Directory.Delete, Directory.Move, ExtractTarGz, PosixChmod) ✓.

One P2 non-blocking observation posted as an inline comment.


Step 6 — Complexity: COMPLEX

Touches plugin/container wiring, async lifecycle management, process spawning, and the boot sequence across 9 files.

Step 7 — QA: YES

Modifies runtime boot code that controls asset-bundle URL source selection and sidecar process lifecycle. Affects what the user sees in local-AB development mode (splash screen timing, failure behavior).

Step 8 — Non-blocking warnings

None. Main scene file not modified.

Step 9 — Verdict

REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies plugin/container wiring, async boot sequence, and child-process lifecycle management across the infrastructure and plugin-system layers
QA_REQUIRED: YES


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

@dalkia
dalkia force-pushed the refactor/local-ab-serial-sidecar branch from 762de69 to cbfdfae Compare August 31, 2026 17:31
dalkia added a commit that referenced this pull request Aug 31, 2026
…source

Names RealmUrls.StartingRealmAsync and the branches being mirrored so
drift is checkable, per the review note on #9831.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@decentraland-bot

This comment has been minimized.

Carries the corpus/bundle-cache hard-link dedup (abgen#108) and the
metadata-dep CDN casing fix (abgen#110). Checksums from the release's
SHA256SUMS.txt; the darwin-arm64 archive was independently downloaded,
hashed and version-checked before pinning.

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

@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.

STEP 1 — Context & Scope

Loaded CLAUDE.md, docs/README.md, and the docs/abgen-sidecar.md subsystem doc.

Changed files (9 files, +180 −201 = net −21):

  • AbgenSidecar.cs — version bump, Launch() param cleanup, thread-safety fix, fail-fast health poll
  • AbgenSidecarBootstrap.cs (new) — lifecycle owner replacing the plugin
  • AbgenSidecarBootstrap.cs.meta (new)
  • BootstrapContainer.cs — removes LocalAbBaseUrl bridge property
  • DynamicWorldContainer.cs — removes AbgenSidecarReadyAsync bridge + plugin wiring
  • MainSceneLoader.cs — owns AbgenSidecarBootstrap, serial startup, splash moved earlier
  • AbgenSidecarPlugin.cs (deleted) — plugin replaced by bootstrap
  • AbgenSidecarPlugin.cs.meta (deleted)
  • docs/abgen-sidecar.md — updated to match new architecture

Surrounding files read in full: AbgenSidecar.cs, AbgenSidecarBootstrap.cs, MainSceneLoader.cs (shutdown + init flow), RealmUrls.cs, BootstrapContainer.cs, DynamicWorldContainer.cs. Searched for orphaned references with rg.


STEP 2 — Root-cause check

Problem: The plugin indirection forced two container bridges (BootstrapContainer.LocalAbBaseUrl and DynamicWorldContainer.AbgenSidecarReadyAsync) and a hope-based startup that seeded a loopback URL before the server was proven healthy — a launch failure left a dead port as the session's optimized-assets source.

Does the diff fix the cause? Yes. The URL is now seeded only after StartAsync returns true (server provably serving). The plugin indirection and both bridges are eliminated. This is a structural fix, not a symptom workaround.

Verdict: PASS


STEP 3 — Design & integration

Owner search for AbgenSidecarBootstrap

  1. Entity/resource managed: the abgen child process lifecycle (download, launch, health, warm-up, kill).
  2. Existing owners searched:
    • MainSceneLoader — the startup orchestrator (InitializeFlowAsync, Shutdown()). Found in MainSceneLoader.cs.
    • AbgenSidecarPlugin (deleted) — was the previous owner. It injected zero ECS systems; InjectToWorld/Dispose were used purely as start/stop hooks. Its placement in the plugin system forced two container bridges.
    • DynamicWorldContainer — registered the plugin and exposed AbgenSidecarReadyAsync. Found in DynamicWorldContainer.cs.
    • BootstrapContainer — carried the LocalAbBaseUrl bridge. Found in BootstrapContainer.cs.
  3. Can the logic run at existing creation/destruction points? Yes — and it does. MainSceneLoader is the natural lifecycle owner: it controls startup ordering, owns the splash screen, and already calls Shutdown() on quit. The new AbgenSidecarBootstrap is a cohesive unit owned by MainSceneLoader, not a redundant parallel mechanism.

The deleted AbgenSidecarPlugin was exactly the anti-pattern flagged in CLAUDE.md §11: "Bridge/wrapper classes on the same abstraction layer" — it existed only to forward lifecycle hooks, with no polymorphism, no second caller, and no test-isolation benefit. The two container bridges (LocalAbBaseUrl, AbgenSidecarReadyAsync) were the cost of that indirection.

Teardown/consumption trace

Opener Mirror Location
new CancellationTokenSource() (field init, AbgenSidecarBootstrap:19) SafeCancelAndDispose() AbgenSidecarBootstrap.Dispose():39
AbgenSidecar.TryCreate(...)sidecar sidecar?.Dispose() AbgenSidecarBootstrap.Dispose():40
new AbgenSidecarBootstrap(...)abgenSidecar abgenSidecar?.Dispose() MainSceneLoader.Shutdown():173

All subscriptions/resources paired. The Shutdown() disposal covers boot failures before the world/plugins ever built — an improvement over the plugin's Dispose which only ran if plugin teardown was reached.

Verdict: PASS


STEP 4 — Member audit

New public members in AbgenSidecarBootstrap

Member Consumers Assessment
BaseUrl (get-only) MainSceneLoader:339 (1 consumer) Legitimate encapsulation — centralizes access to the reserved URL; it is a thin forwarding accessor, not a derived predicate. ✅
WarmUpTask (get, private set) MainSceneLoader:487 (1 consumer) Exposes the warm-up continuation; pre-completed on failure, so the await is free outside LSD. ✅
StartAsync(string) MainSceneLoader:338 (1 consumer) Entry point; the class is the lifecycle owner, so single-call is expected. ✅
Dispose() MainSceneLoader:173 (1 consumer) IDisposable contract. ✅

No single-use derived predicates, no absent-≠-false issues, no redundant guards.


STEP 5 — Line-level review

Pass A — Blocking issues

Scanned all changed lines for: bugs, security, performance, error handling, resource leaks, detached async, nullability violations, false-intent conditions.

Thread-safety fix (AbgenSidecar.cs): Resolving Application.persistentDataPath and IsWindows before DCLTask.RunOnThreadPool is a correct fix — these are main-thread-only Unity APIs. ✅

Fail-fast ChildAlive() check (AbgenSidecar.cs): Race-free because supervision only starts after health passes. Drops dead-child detection from 15s to ~250ms. ✅

#nullable enable removal (AbgenSidecar.cs): NRT is enabled project-wide via csc.rsp (-nullable:enable), so the file-level directive was redundant. ✅

System.Diagnostics import unconditional (AbgenSidecar.cs): Previously gated under #if UNITY_EDITOR because only Process needed it there, but Stopwatch is used unconditionally. The import is now correct for both usages. ✅

Realm root resolution (MainSceneLoader.cs:331-334): Mirrors RealmUrls.StartingRealmAsync for the LSD-only subset (LocalhostIRealmNavigator.LOCALHOST, otherwise customRealm). Verified against RealmUrls.cs:35 — the mapping is identical. The bootstrap ordering constraint (RealmUrls needs URL sources that don't exist yet) justifies the duplication, and the comment documents it explicitly. ✅

Splash screen moved earlier (MainSceneLoader.cs:309-314): Now shown before the abgen serial wait so the user sees visual feedback during the health poll. The AltTester instantiation (depends on splash) moves with it. No ordering issues — assetsProvisioner is already initialized. ✅

No P0 or P1 issues found.

Pass B — Design, encapsulation & resource smells

No new magic values (the existing constants HEALTH_TIMEOUT_MS, HEALTH_POLL_MS are unchanged). No comments narrating caller behavior. Naming follows PascalCase conventions. WarmUpAsync is static — no accidental closures. No plugins mutating containers.

Findings

1 finding (P2) — reported as inline comment below.


Security review

  • Secrets: No credentials, tokens, or keys committed. SHA-256 hashes are public integrity checks from the release's SHA256SUMS.txt. ✅
  • Supply chain: Binary integrity enforced via compile-time SHA-256 per platform. Only the pinned version executes. StreamingAssets override is an explicit developer escape hatch, not a remote attack vector. ✅
  • Network: Loopback-only (127.0.0.1:5147). No new external endpoints exposed. ✅
  • Process spawning: Child process launched with controlled arguments, no user-supplied input in the command line. ABGEN_GPU_BACKEND=off is a hardcoded env var. ✅
  • Input validation: No new untrusted input handling. ✅
  • Auth/authz: No changes to authentication or authorization. ✅

No security issues found.


STEP 6 — Complexity

COMPLEX — modifies async startup flow, plugin/container wiring, process lifecycle management, and introduces a new lifecycle owner.

STEP 7 — QA assessment

QA_REQUIRED: YES — runtime code is modified (startup sequence, sidecar lifecycle, splash screen timing).

STEP 8 — Non-blocking warnings

No Main.unity modification. No warnings.


REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies async startup ordering, plugin-to-bootstrap lifecycle restructuring, process management, and container wiring across MainSceneLoader, BootstrapContainer, and DynamicWorldContainer.
QA_REQUIRED: YES


Reviewed by Jarvis 🤖 · Requested by dalkia via GitHub

Comment thread docs/abgen-sidecar.md Outdated
@decentraland-bot

Copy link
Copy Markdown
Contributor

PR #9831, run #33434475491

Overall: ✅ no significant changes

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

How to read this table
  • Each build is measured 3 times, interleaved with the other build (change, baseline, change, baseline, ...) in the same session, so both see the same world content and machine state. The values are the median, and (min–max) is the lowest and highest of those runs.
  • Δ is Change minus Baseline (a negative Δ means Change is faster).
  • 🟢 faster / 🔴 slower — a difference that passed every check: the runs are fully separated (every run of one build faster than every run of the other), and the median difference is at least 3% and at least 0.5 ms.
  • ⚪ within noise — the builds' runs overlap, or the difference is tiny; it cannot be told apart from random variation. Treat it as no change.
  • — informational — the 0.1% worst metrics average only the few worst frames of a run, so a single OS hiccup swings them by a lot; they are shown for context and never earn a verdict.
  • ⚠️ no verdict — the two builds' sessions were not comparable (very different sample counts, or too few usable runs), so no conclusion is drawn from them.
  • Exceptions per run — the average number of exceptions in a run's log, not counting teardown ones logged while the app quits. Flagged only on a difference of at least 2 per run and 1.5× the other build; exception kinds the baseline never threw are called out under the table. The Exception breakdown groups all of 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.
  • The Overall line at the top only reacts to a metric that moved on two or more machines, or by 10% or more on one — a single modest 🟢/🔴 cell can still be a statistical fluke.

Intel Core i5

Metric Baseline Change Δ Result
Samples 2368 (×3) 2381 (×3)
CPU average 37.8 ms (36.2–38.1) 37.4 ms (37.0–38.6) -0.4 ms ⚪ within noise
CPU 1% worst 331.8 ms (304.8–340.6) 314.2 ms (296.1–367.3) -17.6 ms ⚪ within noise
CPU 0.1% worst 374.3 ms (315.4–382.5) 346.0 ms (323.6–422.7) -28.3 ms — informational
GPU average 23.4 ms (23.0–23.5) 23.2 ms (22.5–24.5) -0.2 ms ⚪ within noise
GPU 1% worst 331.9 ms (311.0–346.1) 320.4 ms (299.2–371.8) -11.5 ms ⚪ within noise
GPU 0.1% worst 382.7 ms (322.4–385.1) 340.2 ms (332.6–424.1) -42.5 ms — informational
Exceptions per run 0 0 0 ⚪ no significant change

Apple M1

Metric Baseline Change Δ Result
Samples 3295 (×3) 3273 (×3)
CPU average 27.2 ms (26.3–27.7) 27.3 ms (27.0–27.5) 0.1 ms ⚪ within noise
CPU 1% worst 236.3 ms (234.0–238.4) 236.5 ms (231.6–239.2) 0.1 ms ⚪ within noise
CPU 0.1% worst 238.2 ms (236.4–242.5) 245.1 ms (242.5–245.6) 6.9 ms — informational
GPU average 19.1 ms (18.0–21.7) 19.3 ms (19.0–19.5) 0.2 ms ⚪ within noise
GPU 1% worst 54.6 ms (52.6–56.1) 55.7 ms (55.1–56.8) 1.1 ms ⚪ within noise
GPU 0.1% worst 58.2 ms (57.8–60.2) 59.5 ms (59.3–61.1) 1.3 ms — informational
Exceptions per run 0 0 0 ⚪ no significant change

@Ludmilafantaniella Ludmilafantaniella 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.

✅ QA Approved

Platforms tested: Windows & macOS

✅ Verified:

  • Standard boot (mf explorer run) — loads normally into Genesis City on both platforms.
  • --local-ab: AB conversion completes successfully, scene ready, 0 failed, scene loads visually fine with local bundles.
  • Without --local-ab: 0 conversions, no abgen process spawned, correctly falls back to production path.
  • Cache dedup (abgen#108) confirmed on Mac — abgen-lsd folder: 2.48GB apparent vs 1.4GB real disk usage, matches the ~2:1 ratio described in the PR.

🐛 Known issue (non-blocking, to be handled separately per @dalkia):

  • Failure fallback edge case: if the abgen binary is missing and there's no internet, the client can get stuck on the loading screen longer than the expected ~15s worst case. Workaround: closing and relaunching the explorer recovers it. Agreed with the dev this will be addressed in a follow-up PR.

Result: Approving from QA side.

windows.mp4
Mac.mp4

✅Smoke test performed:

  • ✔️ Backpack and wearables in world
  • ✔️ Emotes in world and in backpack
  • ✔️ Teleport with map/coordinates/Jump In
  • ✔️ Camera
  • ✔️ Skybox

Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com>
Signed-off-by: Juan Ignacio Molteni <juanignaciomolteni@gmail.com>
@dalkia
dalkia enabled auto-merge (squash) September 1, 2026 17:49
@dalkia
dalkia merged commit 939ed85 into dev Sep 1, 2026
24 of 25 checks passed
@dalkia
dalkia deleted the refactor/local-ab-serial-sidecar branch September 1, 2026 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants