chore: build-time quick wins — rsp DAG rebuild fix, CI queue hygiene, symbols retention, LF scripts - #9616
Conversation
Every cloud build wrote Assets/csc.rsp (+4 siblings) fresh onto a clean checkout; Bee logged 'Rebuilding DAG because FileSignature timestamp changed: Assets/csc.rsp' and re-ran IL2CPP+Usym (~267s) on no-change builds. Committed rsp files keep a stable checkout state; the script now rewrites only on content drift. Their .meta files are committed too, and Explorer/Assets/*.rsp is pinned to eol=lf so CRLF-converting Windows builders cannot defeat the byte-compare. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
UBA allows one pending build per target and the org concurrency cap counts queued builds, so superseded or orphaned builds inflate everyone's queue. - workflow: the Unity-side cancel now runs first-after-build with a 2-min timeout — GitHub force-kills a cancelled job 5 minutes after the request, and a large-log upload ahead of the DELETE could eat the window. Log steps gate on the log existing (in-queue cancels never write one). - build.py: persist the target before the build POST and let --cancel fall back to the target's latest non-terminal build, so a runner death between POST and id write cannot leave a queued build holding a slot; the cancel DELETE gets a 30s timeout. A 90s push debounce was considered and rejected: flat latency tax on every push build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tention The *_debug_symbols artifacts (546-935 MB per platform) uploaded on every build at default retention with zero consumers: Sentry symbolication runs against the on-disk build/ folder in the same job, and release drafting only downloaded them because its Decentraland_.* regex accidentally matched before attaching just the four player zips. Gate the upload to release builds, cap retention at 7 days, and anchor the drafting regex to the player zips. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Windows cloud builders check out with CRLF conversion, so the UBA preBuildScript (scripts/generate-ignore-warnings.sh) failed with "$'\r': command not found" on every Windows build — preBuildScriptFailsBuild=false hid it. All committed *.sh blobs are already LF; this only pins the checkout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Windows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. |
This comment has been minimized.
This comment has been minimized.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Step 1 — Context & Scope
PR type: CI/build infrastructure only — no runtime C# code.
Changed files: .gitattributes, two GitHub Actions workflows, five committed .rsp files + .meta files, build.py (Python), generate-ignore-warnings.sh (Bash).
Target branch: dev
Loaded: CLAUDE.md, docs/README.md (via review protocol), repo review instructions, full file context for all changed files.
Step 2 — Root-Cause Check
All four changes address root causes, not symptoms:
- rsp DAG rebuild → Committed files arrive with stable checkout state; script writes only on content drift. Fixes the cause (unconditional timestamp-changing rewrite).
- Cancel step ordering → Moved the UBA DELETE before log/report steps so it wins the 5-minute cancellation grace window. Fixes the cause (cancel running too late).
- Orphaned builds → Pre-POST target persist + latest-non-terminal fallback. Fixes the cause (build ID not persisted before the POST that creates it).
- Debug symbols / artifact regex → Conditional upload + anchored regex. Fixes the cause (wasteful upload + over-broad download pattern).
- LF scripts →
eol=lfin.gitattributes. Fixes the cause (CRLF corruption on Windows builders).
Verdict: PASS — no symptom-masking.
Step 3 — Design & Integration
No new long-lived units (systems, plugins, managers, services). All changes modify existing CI infrastructure scripts and workflow definitions. Owner search: N/A.
Verdict: PASS.
Step 4 — Member Audit
No public properties or accessors added or changed. N/A.
Step 5 — Line-Level Findings
1 × P2 finding (details in inline comment below).
Verified correct:
.gitattributes—eol=lffor*.rspand*.shis appropriate; committed blobs are already LF, this pins checkout only.- Artifact regex —
Decentraland_(windows64|macos)(_epic)?$correctly matches the four player zips and rejects*_debug_symbols. Verified against all artifact names inbuild-unitycloud.yml. No other workflow usesDecentraland_.*as a regex (others use exact names). - Cancel step —
cancelled() || failure()+timeout-minutes: 2+|| trueis correct. Moved ahead ofalways()log steps so the DELETE wins the grace window. hashFilesgating —hashFiles('unity_cloud_log.log') != ''correctly returns empty string for non-existent files; gates upload/print/extract steps.build.pypre-POST persist —persist_build_info(target, None)beforerun_build()correctly closes the window where a runner death loses the queued build. The--cancelpath readsidfrom the persisted file;id is Nonetriggers the latest-non-terminal fallback, scoped to the correct target.build.pyDELETE timeout —timeout=30bounds a previously unbounded request. Pre-cancel GET already hadtimeout=30.- rsp file content — All five
.rspfiles contain identical suppression lists matching thewarnings_to_ignorearray in the shell script. The.metafiles have unique GUIDs.
Security Review
No security issues introduced. The tighter artifact regex, added HTTP timeout, and reduced debug symbol retention are net positives. No new secrets, permission changes, or third-party actions.
Step 6 — Complexity
SIMPLE — CI/build workflow and script changes only. No runtime C#, ECS, async, or Unity systems.
Step 7 — QA Assessment
QA_REQUIRED: NO — No runtime code changes. All changes are to CI workflows, build scripts, .gitattributes, and compiler response files. No user-facing behavior affected.
Step 8 — Non-Blocking Warnings
None. Main scene not modified.
Step 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: SIMPLE
COMPLEXITY_REASON: CI/build workflow and script changes only — no runtime C#, ECS, async, or Unity systems touched.
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔒 Jarvis reviewed this PR — sensitive paths modified ( |
github-actions
Bot
added
the
no QA needed
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
No C# files changed — lint ratchet skipped. |
|
All Unity tests passed ✅
|
This comment has been minimized.
This comment has been minimized.
…ection
The existing STALE_POLL_THRESHOLD watchdog (600 s) only monitors queue
statuses (created / queued / sentToBuilder). Once a build transitions to
`started` the watchdog goes blind — a deadlocked builder keeps reporting
`started` indefinitely, leaving BUILD_TIMEOUT (3 h) as the only backstop.
This change adds a log-growth liveness signal for the active phase:
• New `get_log_byte_count(id)` probes `/builds/{id}/log` via HEAD (then a
single-byte Range request as fallback) to return the current log size
without downloading the full log.
• `run_poll_loop` tracks `last_log_byte_count` / `last_log_growth`. On
every poll tick while `status in ACTIVE_STATUSES` the log size is checked;
if it has not grown for `LOG_STALL_THRESHOLD` seconds (default 900 s /
15 min) the build is cancelled and `log_stall` is returned.
• `log_stall` is handled identically to `build_timeout`: build_info is
deleted, the log downloaded for debugging, and exit code 99 triggers
nick-fields/retry on a fresh builder VM.
• `LOG_STALL_THRESHOLD: 900` is exposed in build-unitycloud.yml alongside
the existing `STALE_POLL_THRESHOLD`.
Closes #9628
This comment has been minimized.
This comment has been minimized.
…add drift CI check - generate-ignore-warnings.sh: temp file via mktemp outside Assets/ (an interrupted run no longer leaves a stray file for Unity to import), explicit write/copy error handling replacing the stale $? idiom, cp keeps the destination inode, distinct unchanged/updated messages. - build.py: the --cancel fallback only cancels a build still in an explicit queue status — targets are shared (release pool, consecutive runs per branch), so an already-started build may belong to a concurrent run; a missing status is treated as not-cancellable. get_latest_build gains timeout=30: it now sits on the 2-minute cancellation critical path. - test.yml: new rsp-drift job regenerates the rsp files and fails on diff, so generator drift can't silently reintroduce the per-build Bee DAG rebuild. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
…hardening, drift-guard gaps - build.py watchdog: the stall cancel now arms only after one observed log-size increase, so a probe reporting a constant value (proxy answering HEAD with Content-Length: 0, endpoint dead during builds) reads as inactive instead of cancelling every build at 15 min. A size decrease (restarted builds can truncate the log) resets the clock. First probe result is printed once; non-2xx probe responses are surfaced once instead of silently returning None. Threshold evidence: longest log silence across 18 preserved warm builds is 99 s (IL2CPP), giving the 900 s threshold a ~9x margin. - get_log_byte_count: allow_redirects=True on HEAD (requests defaults HEAD to not following redirects; the endpoint may 302 to signed storage), timeouts lowered 30 -> 10 s on the poll critical path. - test.yml: generator added to the pull_request paths filter (an edit to warnings_to_ignore alone never triggered the workflow); drift check uses git status --porcelain so a new untracked rsp file counts as drift; rsp-drift job gets timeout-minutes and contents: read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
PR Review: chore: build-time quick wins
STEP 2 — Root-cause check: PASS ✅
The PR addresses six distinct root causes identified through systematic build log analysis (17 logs, 2026-07-10 → 2026-08-05):
| Fix | Root cause |
|---|---|
| Committed rsp files + drift-check | generate-ignore-warnings.sh rewrote rsp files unconditionally → Bee DAG rebuild (~267 s) on every no-change build |
| Cancel step moved before log/report steps | DELETE ran after log upload → 5-min grace window exhausted before queue slot freed |
| Pre-POST persist + cancel fallback | Runner crash between build POST and id write → permanently orphaned queued build |
| Debug symbols gated to release builds | *_debug_symbols (546–935 MB/platform) uploaded on PR builds with zero downstream consumers |
LF enforcement for .sh/.rsp via .gitattributes |
CRLF conversion on Windows builders → $'\r': command not found |
| Log-stall watchdog | Deadlocked builder keeps status=started forever → detected only by 50-min BUILD_TIMEOUT backstop |
Each fix targets the actual cause, not a symptom. Evidence is thorough and verifiable against the cited build logs.
STEP 3 — Design & integration: PASS ✅
No runtime code changes. All modifications are in CI/CD infrastructure:
- GitHub Actions workflows (step ordering, conditions, artifact retention)
- Python build orchestration script (watchdog, cancel fallback, timeouts)
- Bash generator script (drift-check optimization)
- Git configuration (
.gitattributesline-ending rules) - Committed compiler response files (
Explorer/Assets/*.rspand.meta)
No new persistent state, no ECS changes, no lifecycle concerns. The changes are self-contained within the build pipeline. The new rsp-drift CI job correctly validates that committed rsp files stay in sync with the generator.
STEP 4 — Member audit
New functions in build.py:
get_log_byte_count(id)— single consumer (poll loop watchdog). Correctly uses HEAD-first, Range-fallback strategy. ReturnsNoneon any error to avoid false-positive cancellation. The_non_2xx_loggedfunction attribute provides one-time diagnostic visibility.- Cancel fallback logic (inline in
--cancelpath) — well-guarded withQUEUE_STATUSEScheck. Only cancels builds still in queue, never active builds that may belong to concurrent runs.
New poll loop state:
last_log_byte_count,last_log_growth— track log size and last growth timestamp.log_growth_observed— prevents false stall detection when probes return a constant value (excellent guard).log_probe_logged— first-probe visibility for debugging.
All new state is scoped to the poll loop's lifetime. No global mutable state introduced.
STEP 5 — Line-level findings
No P0 or P1 issues found.
After four parallel review agents (security, Python, architecture, shell) and thorough manual analysis, no blocking issues were identified.
Security review summary (clean):
- No expression injection in workflow
${{ }}expressions — only safe built-in functions and SHA-pinned refs used mktemp+trap EXITpattern is correct and secure- API credentials stay in headers, never in logged URLs or exception messages
- Regex
Decentraland_(windows64|macos)(_epic)?$is simple alternation — no ReDoS risk - New rsp-drift CI job uses
ref: ${{ github.event.pull_request.head.sha || github.sha }}— safe checkout
Architecture review summary (sound):
- Cancel step ordering (
cancelled() || failure()before log steps) is correct — the cancel function handles terminal-status builds idempotently via pre-cancel status check hashFiles('unity_cloud_log.log')gate on log steps eliminates guaranteed-failing uploads on in-queue cancellations — no race condition (steps are sequential within a job)- The
log_growth_observedarming flag correctly prevents false stall detection from constant-value probes, proxies, or builds that haven't started writing yet - Log size decrease handling (reset clock) covers
restartedbuilds that replace/truncate the log - The 900 s threshold has ~9× margin over the measured maximum log silence (99 s, IL2CPP phase)
Python review summary (clean):
get_log_byte_count:allow_redirects=Trueon HEAD handles 302 to signed storage;timeout=10is appropriate for poll-loop critical path;stream=True+resp.close()on Range GET avoids body download; header access after close is valid (headers already received)- Cancel fallback TOCTOU window (build could start between status check and cancel) is mitigated by restricting to
QUEUE_STATUSES— the comment explicitly acknowledges the trade-off and the window is a single API round-trip if log_bytes:correctly treats bothNoneand0as falsy — a zero-length log (not yet written) shouldn't feed the watchdog- Exit code 99 for
log_stallcorrectly triggersnick-fields/retryjust likebuild_timeoutandqueue_timeout
Shell script review summary (clean):
mktemp→trap EXIT→cmp -s→cppattern is the standard secure approach for drift-checkingcmp -s ... 2>/dev/nullcorrectly handles the case where the target file doesn't exist (exits non-zero, stderr suppressed)cpto existing destination preserves inode and permissions as documented- The
.gitattributeseol=lfentry is the necessary companion — without it, CRLF-converting builders would defeat the byte-compare
STEP 6 — Complexity: COMPLEX
16 files changed with significant new logic in build.py (log-stall watchdog with multi-level arming, cancel fallback with TOCTOU mitigation), workflow step reordering, and a new CI validation job.
STEP 7 — QA: NO
No runtime code changes. All changes are CI/build scripts, workflow YAML, .gitattributes, and committed compiler response files. The player binary is byte-identical in intent.
STEP 8 — Warnings
None. Main scene not modified.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Significant new logic in build.py (log-stall watchdog, cancel fallback), workflow step reordering, new CI job, spread across 16 files
QA_REQUIRED: NO
Reviewed by Jarvis 🤖 · Requested by mikhail-dcl via GitHub
Observed on this branch's own CI (run 31087915798): the Windows build sat sentToBuilder for 9 minutes, UBA canceled it platform-side (builder provisioning failure; the same window produced a transient HTTP 500 from the API), and build.py mapped the external 'canceled' to exit 1 - which nick-fields/retry does not retry, so an infra flake became a hard red job with the second attempt unused. This run's own cancellations exit through the watchdog/timeout branches, so a 'canceled' final outcome is always external: exit 99 after deleting build_info, letting the retry create a fresh build. Bounded by max_attempts: 2, so a supersede race cannot loop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
This PR also fixes #9649 (Unity Cloud Build Windows builds failing due to CRLF line endings in shell scripts) — the |
This comment has been minimized.
This comment has been minimized.
The unconditional canceled->retry turned a supersede on the shared release pool into a ping-pong: the superseded run's retry re-POSTs, hits 'already a build pending', cancels the superseding run's build, and hands it the same exit 99 - both runs burn a queue+build cycle and one still ends red (and set_parameters clobbers the winner's env vars). Build numbers are monotonic per target: if the target's newest build is newer than ours we were superseded and exit 1 without retrying; if ours is still the newest, the cancel was platform-side (builder provisioning) and the retry stands. Also download the log best-effort on this path - with the log steps now gated on the log existing, an externally-canceled run would otherwise finish with no log artifact at all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Two residuals in the canceled-outcome guard: - run_build cancels the pending build and only re-POSTs ~30 s later, so a supersede is invisible to a probe landing inside that gap (~coin flip with POLL_TIME=60). When the first probe does not show a newer build, sleep 35 s and re-probe once before deciding to retry. - get_latest_build was the one bare call site on this path: a transient RequestException would traceback to exit 1 - skipping build_info cleanup and the log download, and converting the provisioning-flake retry this branch exists for into a red job. The probe now fails open (latest = None -> retry), matching the non-200 behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @mikhail-dcl's task in 2m 44s —— View job Security review — dependency + workflow scan (re-run at
|
| Round | Finding | Status |
|---|---|---|
| LOW | Supersede window open during run_build's 30 s pre-re-POST gap |
✅ Fixed (:896-900) |
| LOW | Bare get_latest_build → socket error tracebacks past cleanup |
✅ Fixed (:885-892 + cleanup reordered ahead of the decision) |
| MEDIUM | canceled ⇒ unconditional retry ping-pongs on the shared release pool |
✅ Fixed |
| LOW | External cancel produced no log artifact under the new hashFiles gate |
✅ Fixed |
| HIGH | Watchdog armed on first observation, not first observed growth | ✅ Fixed |
| LOW | requests.head not following redirects; 30 s poll-path timeouts |
✅ Fixed |
| LOW | Probe never reported a permanently-dead endpoint | ✅ Fixed |
| MEDIUM | rsp-drift inherited packages: write / checks: write, no timeout |
✅ Fixed |
| MEDIUM | Drift guard never fired on a generator-only PR | ✅ Fixed |
| MEDIUM | git diff --exit-code blind to a new untracked .rsp |
✅ Fixed |
| MEDIUM | --cancel fallback could kill a concurrent run's build |
✅ Fixed |
Verdict
No security regression. 790d4f1d is a correctness refinement on an already-sound guard, with no new attack surface — no dependency change, no .github/ change, no new capability. Nothing blocks; the two LOWs above are optional polish.
The NEEDS_ATTENTION below is carried by the two deferred action-pinning MEDIUMs (W.7a/W.7b) only, both pre-existing and explicitly scoped to a follow-up by the author. Per the protocol a MEDIUM without a HIGH cannot emit PASS — this is not a signal against merging this PR.
DEPENDENCY_REVIEW: NEEDS_ATTENTION
Reviewed at 790d4f1d: the re-probe and fail-open in the canceled branch, the cleanup/decision ordering, the winner/loser interleaving in run_build, and a re-scan of the dependency and .github/ surfaces. python3 -c "ast.parse(...)" and bash -n were blocked by the sandbox this run, so the syntax check is by inspection only — the two prior rounds validated both files. No code changes were made — review-only.
4 participants
Pull Request Description
What does this PR change?
Five low-risk build/CI fixes derived from an analysis of UBA build logs and API history (last ~3 weeks of builds). No runtime code is touched.
Closes #9628
generate-ignore-warnings.shrewrites only on driftAssets/csc.rsp(+4 siblings) on every build, changing its timestamp. Bee loggedRebuilding DAG because FileSignature timestamp changed: Assets/csc.rspand re-ran IL2CPP + Usym (~267 s) with zero code changes. Committed files arrive with stable checkout state; the script now writes only when content actually differs.Explorer/Assets/*.rspis pinnedeol=lfso CRLF-converting Windows builders can't defeat the byte-compare, and their.metafiles are committed so Unity stops regenerating them each builder boot.build.py: persist target before the build POST;--cancelfalls back to latest non-terminal build; DELETE gets a 30 s timeout--cancel— permanent on idle PR targets, holding an org concurrency slot. The pre-POST persist plus the latest-non-terminal fallback close that window.*_debug_symbolsartifacts (546–935 MB per platform) had zero consumers: Sentry symbolication reads the on-diskbuild/folder in the same job, and release drafting only downloaded them becauseDecentraland_.*accidentally matched — it attaches only the four player zips (regex verified against all artifact names).*.shforced to LF checkoutgenerate-ignore-warnings.shfailed with$'\r': command not foundon every Windows build, hidden bypreBuildScriptFailsBuild: false. All committed.shblobs are already LF — this pins checkout only.LOG_STALL_THRESHOLD, 15 min) — cherry-picked from #9629, closes #9628BUILD_TIMEOUTbackstop) to ≤15 min, with automatic retrystatus=startedforever while its log goes silent (real incident 2026-08-05). The poll loop now probes the UBA log size (HEAD, Range fallback, never downloads the body) and cancels + retries on a fresh VM (exit 99) when the log stops growing. Probe failures skip silently — no false-positive cancels.Deliberately not included: a 90 s push debounce (flat latency tax on every push build), release-pool cache seeding (pools are warm and stay warm), and the Addressables
BuildAddressablesWithPlayerBuildchange.Test Instructions
No runtime changes — the player binary is byte-identical in intent; verification is on CI behavior:
Steps (standard run):
metaforge explorer run XXXX # ← replace with this PR number; sanity only, no runtime delta expectedExpected result: client runs as on
dev.Test Steps
unity_log, confirm there is noRebuilding DAG because FileSignature timestamp changed: Assets/csc.rspline, and the preBuildScript reports all five rsp files unchanged (also on the Windows builder, where it previously failed with$'\r': command not found).Cancel Unity Cloud buildstep runs immediately after the build step and the UBA dashboard shows the buildcanceled; the log-upload step is skipped (no red failed step) when no log exists.*_debug_symbolson a PR build. Trigger a release build: symbols present with 7-day retention.windows64,macos,windows64_epic,macos_epic).Additional Testing Notes
.sh/.gitattributesline-ending conversion warnings once; committed blobs are unchanged.generate-ignore-warnings.shlocally: delete untrackedExplorer/Assets/*.rsp*before pulling this branch — git refuses to overwrite them, and locally generated.metaGUIDs differ from the committed ones (one-time reimport after checkout).Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting.
Build-time evidence (summary)
Full reports with log excerpts, line references and preserved UBA logs: see attached archive. Three documents, 17 build logs analyzed (2026-07-10 → 2026-08-05, all pre-fix):
Assets/csc.rsptimestamp churn re-running the Bee DAG twice per build — the thing this PR fixes.clean-buildPR label; the sole automatic exception is the nightly profile lane (clean_build: true, cron 02:00 UTC, shared dev targets). Side finding: first builds on copy-seeded targets show an incoherent-UPM-state anomaly (248 mangledCould not restore immutable package assetwarnings) — measured build-time impact: none (≤1 s burst + one 15 s re-resolve).windows64-devqualifies (after moving the nightly off it); the flat $153.60/month/target fee rules out release, per-branch and template targets.Conclusions: the historical "git packages rebuild everything" theory is refuted for warm builds; the pipeline's real recurring waste was rsp timestamp churn (fixed here), symbols upload (fixed here), and queue-slot leaks on cancel (fixed here). Remaining levers, evidence-ranked: machine tier (2.7× measured), Boost Disk pilot on
windows64-dev, cache-size instrumentation.