refactor: serialize local-ab sidecar startup and drop the plugin bridges - #9831
Conversation
🚦 CI StatusNew build in progress, come back later! Warnings count reduced: 12183 => 12179 Warnings/errors in files changed by this PR (1)All Unity tests passed ✅
|
decentraland-bot
left a comment
There was a problem hiding this comment.
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 inAbgenSidecarBootstrap.Dispose()viaSafeCancelAndDispose()✓sidecar(AbgenSidecar, IDisposable) → disposed inAbgenSidecarBootstrap.Dispose()✓AbgenSidecarBootstrapitself → disposed inMainSceneLoader.Shutdown()line 164 ✓ (covers boot failures before plugins built)WarmUpTask→ awaited inMainSceneLoader.InitializeFlowAsyncline 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:
- Code quality / CLAUDE.md: PascalCase types/methods/properties, camelCase locals ✓.
SafeCancelAndDispose()for CTS ✓.ReportHub.LogExceptionfor error logging ✓. No LINQ ✓. NoDebug.Log✓. Comments describe what the code does, not caller behavior ✓. - Bugs / runtime errors: None found. Exception handling in
StartAsyncandWarmUpAsynccatchesOperationCanceledExceptionseparately ✓.WarmUpTaskis pre-completed (UniTask.CompletedTask) so never-started paths are safe ✓. - Security: No new attack surface. Binary verification via SHA-256 unchanged. Loopback-only binding unchanged.
- Performance: No hot-path changes.
ChildAlive()check inWaitHealthyAsyncis a ~250ms fast exit improvement. - Error handling: All failure paths return
false(no faulting UniTasks). Exceptions logged viaReportHub✓. - Resource/subscription leaks: CTS created → cancelled+disposed in Dispose ✓. AbgenSidecar created → disposed in Dispose ✓. No subscriptions or event handlers added.
- Detached async:
WarmUpTaskis stored and awaited byMainSceneLoaderbefore realm loading ✓. No essential.Forget()calls introduced. - Nullability:
AbgenSidecar? sidecarcorrectly nullable (null before creation and after failed creation) ✓.AbgenSidecarBootstrap? abgenSidecarin 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:
ExtractTarGzguards..but not absolute paths (e.g./etc/foo). Mitigated by SHA-256 pinning. - Low:
Environment.SetEnvironmentVariablemutates the parent process env (allABGEN_-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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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>
2a6d98f to
31ebafb
Compare
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>
decentraland-bot
left a comment
There was a problem hiding this comment.
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 inAbgenSidecarBootstrap.Dispose()viaSafeCancelAndDispose()✓sidecar(AbgenSidecar, IDisposable) → disposed inAbgenSidecarBootstrap.Dispose()✓AbgenSidecarBootstrapitself → disposed inMainSceneLoader.Shutdown()line 173, AFTER dynamicWorldContainer/staticContainer (no live references) but BEFORE bootstrapContainer — dependency-safe since bootstrap only captures aDecentralandEnvironmentenum value ✓WarmUpTask→ awaited inMainSceneLoader.InitializeFlowAsyncline ~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:
- Code quality / CLAUDE.md: PascalCase types/methods/properties, camelCase locals ✓.
SafeCancelAndDispose()for CTS ✓.ReportHub.LogExceptionfor error logging ✓. No LINQ ✓. NoDebug.Log✓.#nullable enableremoval is correct — nullable is enabled project-wide (verified:DynamicWorldContainer.csandAbgenSidecarBootstrap.csuse nullable annotations without the directive) ✓.using System.Diagnosticsmoved from#if UNITY_EDITORto unconditional — correct,Stopwatchis available on all platforms andProcessusage is still behind#if UNITY_EDITOR✓. - Bugs / runtime errors: None found.
Launch()parameter removal is safe (uses readonlyexecutablePathfield set in constructor).ChildAlive()placement is race-free (supervision only starts after health passes).WarmUpAsyncis static to avoid capturingthis. Exception handling catchesOperationCanceledExceptionseparately ✓. - 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.
- Performance: No hot-path changes.
ChildAlive()check is a ~250ms fast-exit improvement for dead-child detection. - Missing error handling: All failure paths return
false(no faulting UniTasks). Exceptions logged viaReportHub✓. - Resource / subscription leaks: CTS created → cancelled+disposed in Dispose ✓. AbgenSidecar created → disposed in Dispose ✓. No subscriptions or event handlers added.
- Detached async:
WarmUpTaskis stored and awaited byMainSceneLoaderbefore realm loading ✓.SuperviseAsync(ct).Forget()inAbgenSidecar.StartAsyncis pre-existing and appropriately fire-and-forget (monitoring, not essential setup) ✓. - Nullability:
AbgenSidecar? sidecarcorrectly nullable (null before creation and after failed creation) ✓.AbgenSidecarBootstrap? abgenSidecarin MainSceneLoader correctly nullable (null when not in local-ab mode) ✓. - Main-thread fix:
Application.persistentDataPathandIsWindows(which readsApplication.platform) resolved beforeDCLTask.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
762de69 to
cbfdfae
Compare
…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>
This comment has been minimized.
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
left a comment
There was a problem hiding this comment.
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 pollAbgenSidecarBootstrap.cs(new) — lifecycle owner replacing the pluginAbgenSidecarBootstrap.cs.meta(new)BootstrapContainer.cs— removesLocalAbBaseUrlbridge propertyDynamicWorldContainer.cs— removesAbgenSidecarReadyAsyncbridge + plugin wiringMainSceneLoader.cs— ownsAbgenSidecarBootstrap, serial startup, splash moved earlierAbgenSidecarPlugin.cs(deleted) — plugin replaced by bootstrapAbgenSidecarPlugin.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
- Entity/resource managed: the abgen child process lifecycle (download, launch, health, warm-up, kill).
- Existing owners searched:
MainSceneLoader— the startup orchestrator (InitializeFlowAsync,Shutdown()). Found inMainSceneLoader.cs.AbgenSidecarPlugin(deleted) — was the previous owner. It injected zero ECS systems;InjectToWorld/Disposewere used purely as start/stop hooks. Its placement in the plugin system forced two container bridges.DynamicWorldContainer— registered the plugin and exposedAbgenSidecarReadyAsync. Found inDynamicWorldContainer.cs.BootstrapContainer— carried theLocalAbBaseUrlbridge. Found inBootstrapContainer.cs.
- Can the logic run at existing creation/destruction points? Yes — and it does.
MainSceneLoaderis the natural lifecycle owner: it controls startup ordering, owns the splash screen, and already callsShutdown()on quit. The newAbgenSidecarBootstrapis a cohesive unit owned byMainSceneLoader, 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 (Localhost → IRealmNavigator.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.
StreamingAssetsoverride 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=offis 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
|
PR #9831, run #33434475491 Overall: ✅ no significant changes Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Apple M1
|
Ludmilafantaniella
left a comment
There was a problem hiding this comment.
✅ 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>
Pull Request Description
What does this PR change?
Restructures how the
--local-ababgen 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.
AbgenSidecarPlugininjected zero systems —InjectToWorld/Disposewere used purely as start/stop hooks — but its placement forced two bridges: the reserved URL threaded in viaBootstrapContainer.LocalAbBaseUrl, and readiness threaded back out viaDynamicWorldContainer.AbgenSidecarReadyAsync. Both are deleted. The lifecycle now lives in a plainAbgenSidecarBootstrapowned end to end byMainSceneLoader(DynamicWorldContainerhas 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
StartAsyncbrings 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).
WaitHealthyAsyncnow checksChildAlive()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'sDisposeonly 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):
Expected result:
Normal boot into Genesis City — the vanilla path is untouched apart from the splash screen appearing slightly earlier in boot.
Prerequisites
central-plazasceneDecentraland.app)Test Steps
open Decentraland.app --args --realm http://127.0.0.1:8000 --local-scene true --local-ab--local-aband press the check button again: entries report red (no local bundles — raw-GLTF path), and no abgen process is spawned.Additional Testing Notes
--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.Quality 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