feat(abgen): local-ab follow-ups — reconversion mirroring, production fallback, orphan hardening - #9756
Open
dalkia wants to merge 4 commits into
Open
feat(abgen): local-ab follow-ups — reconversion mirroring, production fallback, orphan hardening#9756dalkia wants to merge 4 commits into
dalkia wants to merge 4 commits into
Conversation
The /progress mirroring only existed inside the boot warm-up, so a GLB edit reconverted correctly but the panel kept saying ALREADY CONVERTED (QA, both platforms). The LSD reload path now raises a consumable signal on AbgenConversionMetrics (carrying UpdateModel's src; null for whole-scene updates), and a session-long watcher consumes it and re-runs the manifest lane — coalescing with the server's rebuild, so elapsed/READY are accurate even when the rebuild outpaces the 500ms progress poll. The manifest+progress +census loop is extracted into MirrorManifestBuildAsync, shared verbatim with the warm-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ReserveBaseUrl seeds the optimized-assets override before the sidecar starts; on download/launch failure the dead loopback port previously stayed the base for the whole session — wearables degraded to raw GLTFs per request and the registry-composed profile/entities endpoints ate a dead-port round trip each. ReadyAsync now resolves a bool (healthy server = true; warm-up outcome does not matter, bundles JIT per request), and on false MainSceneLoader drops the override at the boot-hold — before any optimized-asset URL has resolved — via ClearOptimizedAssetsOverride, which evicts the flag-dependent URL cache class. The session then behaves exactly as if --local-ab had not been passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…Windows) With the fixed default port an abgen orphaned by a crashed session owns the endpoint the next session expects; the health check accepted any listener, so a stale server (old version, other workspace's realm) was silently adopted while our own child died on bind. StartAsync now verifies our child is alive after the health check and fails fast otherwise. On Windows (player and editor) every child is additionally assigned to a kill-on-close Job Object, so the kernel reaps it when the explorer dies for any reason — crash included. macOS keeps cooperative teardown; a parent-pid watchdog in abgen is the upstream complement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
🚦 CI Status
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 13156 => 13274 — remove at least 119 warnings to merge. Warnings/errors in files changed by this PR (11)All Unity tests passed ✅
|
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
PR Review — feat(abgen): local-ab follow-ups
STEP 2 — Root-cause check
The diff addresses three distinct root causes:
- Reconversion mirroring: the
/progressmirror was inlined in the boot warm-up with no event-driven continuation for content edits — fixed by extractingMirrorManifestBuildAsyncand addingWatchReconversionsAsyncdriven by a consumable signal. - Production fallback: a download/launch failure left the dead loopback port as the optimized-assets base for the whole session — fixed by returning a usability bool from
ReadyAsyncand clearing the override on failure. - Orphan hardening: teardown was purely cooperative, orphaning the server on crashes — fixed with Windows Job Objects and a child-alive check after the health check.
Root-cause verdict: PASS — the diff fixes causes, not symptoms.
STEP 3 — Design & integration
Owner search
| New unit | Owner searched | Files examined | Verdict |
|---|---|---|---|
WatchReconversionsAsync |
AbgenSidecar already owns HTTP communication, warm-up, progress, census |
AbgenSidecar.cs, AbgenSidecarPlugin.cs |
Correct owner — method on existing class, CancellationToken from plugin's lifecycle CTS (SafeCancelAndDispose()'d in Dispose()) |
MirrorManifestBuildAsync |
Private extraction from existing warm-up body | AbgenSidecar.cs |
Two callers, same class — clean extraction |
OnContentEdit/TryConsumeContentEdit |
AbgenConversionMetrics.INSTANCE — the established cross-assembly signal channel |
AbgenConversionMetrics.cs |
Identical pattern to existing RequestPanelOpen/TryConsumePanelOpenRequest (same lock, same consume-resets semantics) |
ClearOptimizedAssetsOverride |
DecentralandUrlsSource owns the override field |
DecentralandUrlsSource.cs, MainSceneLoader.cs |
URL source is the natural owner; MainSceneLoader is the correct caller (boot-hold, before any optimized-asset request resolves) |
TieChildLifetimeToUs / Job Object |
AbgenSidecar already owns the child process (handles, kill, dispose) |
AbgenSidecar.cs |
Job handle managed alongside process handle in KillChild()/Dispose() |
Teardown / consumption trace
contentEditSignaled/contentEditSrc: consumed (reset) byTryConsumeContentEditunder lock. Singleton lifetime — no disposal needed. ✓WatchReconversionsAsync: cancelled via CT fromlifeCycleCancellationTokenSource,SafeCancelAndDispose()'d inAbgenSidecarPlugin.Dispose(). ✓jobHandle: closed inKillChild()→ called fromDispose(). ✓warmedEntityId: nullable string field, written-before-read on same async chain. No teardown needed. ✓
Design verdict: PASS — all new logic lives on existing owners; no new lifecycle units; signal pattern matches existing precedent.
STEP 4 — Member audit
| Member | Consumers | Assessment |
|---|---|---|
OnContentEdit(string?) |
LocalSceneDevelopmentController (1) |
Producer half of produce/consume pair — 1 producer correct for single-pipeline singleton |
TryConsumeContentEdit(out string?) |
WatchReconversionsAsync (1) |
Consumer half — 1 consumer correct |
WatchReconversionsAsync(CancellationToken) |
AbgenSidecarPlugin.RunAsync (1) |
Public for cross-class call; self-contained exception handling justifies .Forget() |
MirrorManifestBuildAsync(string, CancellationToken) |
WarmUpLocalSceneAsync + WatchReconversionsAsync (2) |
Clean private extraction |
ClearOptimizedAssetsOverride() |
MainSceneLoader (1) |
Interface method; NSubstitute auto-implements for test fakes |
warmedEntityId field |
Written WarmUpLocalSceneAsync, read WatchReconversionsAsync |
Both on same async chain, write-before-read guaranteed |
No single-use-should-inline, absent≠false, or redundant-guard issues found.
STEP 5 — Line-level findings
No blocking issues found. Detailed verification:
- Thread safety: All shared state on
AbgenConversionMetricsislock(gate)guarded, consistent with every existing member. TheoptimizedAssetsBaseOverridemutation is main-thread-only (boot flow).OnContentEditis called from the thread-pool (websocket receive loop) — safe because it acquires the lock. - Exception handling:
OperationCanceledExceptionsuppressed at the right boundaries; all other exceptions reported viaReportHub.LogException. Inner-to-outer OCE propagation inWatchReconversionsAsync(rethrow → outer catch suppresses) is correct. - P/Invoke: Struct layouts match the Windows SDK.
JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE = 0x2000andJOB_OBJECT_INFO_CLASS_EXTENDED_LIMIT = 9are correct. The fullJOBOBJECT_EXTENDED_LIMIT_INFORMATIONstruct (includingIO_COUNTERS) is required for correctLayoutKind.Sequentialmarshaling. .Forget()onWatchReconversionsAsync: Acceptable — the method handles its own exceptions (§9 compliant), is not essential setup (server-side conversion is independent), and the boot-hold must release.- Bare
catch { … throw; }inMirrorManifestBuildAsync: Correctly handles both OCE and general exceptions by cleaning upconvertingFileand rethrowing to the caller. - LINQ in
ClearOptimizedAssetsOverride: Matches the existingResetRealmDependentUrlspattern; called at most once per session — not a hot path. AssignProcessToJobObjectreturn unchecked: Documented as best-effort; failure leaves behavior exactly as before.
Security review
- No secrets, tokens, or credentials exposed in the diff.
- Loopback-only binding (
127.0.0.1:5147): not network-exposed. - Orphan detection correctly refuses foreign processes: after health check,
ChildAlive()verifies our child is live; a foreign listener is rejected with an explicit milestone. - P/Invoke safety: all kernel32 handles created/used/closed in
KillChild. No unsafe memory operations. - No path traversal or injection: BaseUrl is a fixed loopback, entity IDs parsed from trusted server responses, env var names are compile-time constants.
No security issues found.
STEP 6 — Complexity
COMPLEX
STEP 7 — QA
YES — changes runtime code under Explorer/ that ships in the build; affects local scene development workflow, asset loading behavior (production fallback), and process management.
STEP 8 — Warnings
None. Main.unity is not in the changed files.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Async plugin wiring (UniTask return type change), cross-assembly consumable-signal pattern, kernel32 P/Invoke Job Objects, and URL resolution cache eviction across 9 files
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
This comment has been minimized.
This comment has been minimized.
Boot already holds until the warm-up's manifest request returns, then the scene lane re-fetched the same URL seconds later — paying abgen's content revalidation twice (~6s on a large scene under ABGEN_JIT_CONTENT_DIGEST). The sidecar now hands the awaited response over (AbgenManifestPrewarm, keyed by exact URL) and LoadAssetBundleManifestSystem reuses it; a content edit invalidates the entry (the census may change) and the reconversion watcher's re-fetch repopulates it, so post-edit reloads reuse it too. Any URL mismatch degrades silently to the normal fetch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
PR #9756, run #31831283599 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups to #9704 (explorer-owned abgen sidecar). Closes #9749, plus orphan-process hardening. Based on
main(where #9704 landed); to be rebased/retargeted as the trunks sync.1. AB panel mirrors content-edit reconversions (#9749)
QA-confirmed on Windows + macOS: replacing a GLB reconverted correctly, but the panel kept saying "SCENE ALREADY CONVERTED" — the
/progressmirroring only existed inside the boot warm-up.Now event-driven end to end:
LocalSceneDevelopmentController(the websocket handler that already receives the preview server's edit messages) raises a consumable signal onAbgenConversionMetrics— carrying the changed model's path forUpdateModel, null for whole-scene updates — and the sidecar's session-long watcher consumes it and re-runs the manifest lane itself. That request coalesces with (or front-runs) the server's rebuild and returns when the build finishes, so the panel flips to converting, names the edited file, and settles to READY with an accurate "reconverted in Ns" even when the rebuild outpaces the 500 ms progress poll. Named edits always report "reconverted"; unnamed (code-only) updates with no observed build report "revalidated — bundles already up to date". The manifest+progress+census loop is extracted intoMirrorManifestBuildAsync, shared verbatim with the boot warm-up. No new assembly wiring (SceneLifeCycle already references ECS.Unity); outside--local-abthe signal is never consumed and stays inert.Known limit: texture edits arrive as unnamed whole-scene updates (sdk-commands names only
.glb/.gltfinfile-watch-notifier.ts) — mirrored anonymously; widening that gate is a candidate js-sdk-toolchain follow-up.2. Clean production fallback when the sidecar can't be had (#9749)
ReserveBaseUrlseeds the optimized-assets override before the sidecar ever starts, and a download/launch failure used to leave the dead loopback port as the base for the whole session: the scene degraded per request, wearables/emotes lost their production-CDN bundles entirely (per-request GLTF fallback), and the registry-composedProfiles/ProfilesMetadata/EntitiesActiveElementsendpoints ate a dead-port round trip each.AbgenSidecarPlugin.ReadyAsyncis nowUniTask<bool>— true once the server is healthy (warm-up outcome doesn't gate it: bundles JIT per request), false when it never came up (download failure, launch failure, port owned by someone else, teardown before start). On false,MainSceneLoadercallsDecentralandUrlsSource.ClearOptimizedAssetsOverride()at the boot-hold — before any optimized-asset URL has resolved — which nulls the override and evicts theFeatureFlagsDependentURL cache class (the one every affected endpoint, registry-composed included, resolves under). The session then behaves byte-for-byte as if--local-abhad not been passed. A server that turns healthy and dies later keeps the override: supervision restarts it up to 3×, and per-request recovery remains the safety net.3. Orphan-process hardening
Teardown was purely cooperative (Dispose kills the child), so a hard crash of the explorer orphaned the server — and with #9704's fixed default port (
127.0.0.1:5147), the orphan owns the endpoint the next session expects. The health check accepted any listener, so the new session's child died on bind while a stale server (old version after a pin bump, or another workspace's realm) was silently adopted. Reproduced in the wild during Windows testing.StartAsyncverifies our own child is still alive. If the port is answered by anything else, it fails fast with an explicit milestone ("another process owns … — kill it and relaunch") instead of serving stale bundles — and with §2, the session then falls back to production cleanly.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, so the kernel reaps it when the explorer dies for any reason, crash included. Pure kernel32 P/Invoke, IL2CPP-safe, best-effort (failure leaves behavior exactly as before). macOS has no equivalent primitive; a parent-pid watchdog in abgen is the tracked upstream complement.4. Scene-entry manifest reuse (~6 s on a large scene)
Boot already holds until the warm-up's manifest request returns — then the scene lane re-fetched the exact same URL seconds later, paying abgen's content revalidation (
ABGEN_JIT_CONTENT_DIGEST) twice: ~6 s on a Genesis-Plaza-sized scene.The sidecar now hands the awaited response over (
AbgenManifestPrewarm, a single-entry holder keyed by the exact URL) andLoadAssetBundleManifestSystemconsumes it instead of re-fetching, logging the reuse. A content edit invalidates the entry immediately (the file census may have changed) and the reconversion watcher's re-fetch repopulates it, so post-edit reloads benefit too; a reload that races the watcher just misses and fetches fresh. Any URL mismatch degrades silently to the normal network path, and outside--local-abthe holder is never populated.Related upstream work (not in this PR)
ABGEN_HTTP_HOST/ABGEN_HTTP_PORTwith the legacy generic names kept as fallbacks. Once released and the pin is bumped, the explorer can return to a per-session free port — dissolving the single-instance limitation and shrinking the orphan-collision surface further.build_entity_into_corpusrewrites the bytesbundle()just cached) plus ~300 MB of mirrored source content. The corpus write becoming a hard link (the idiom that function already uses for the digest alias) halves the bundle storage; abgen PR to follow. The CLEAR CACHE button in the AB panel remains the manual relief valve.Test plan
--local-ab: no behavior change (signal unconsumed, readiness pre-completed true, no job object, no clear call).🤖 Generated with Claude Code