From e1c193c0aadef94b4422b3dd0695eb411762daa7 Mon Sep 17 00:00:00 2001 From: frostebite Date: Sat, 22 Aug 2026 22:12:44 +0100 Subject: [PATCH 1/2] feat: bank the Library cache as a floor when import succeeded despite a later failure (opt-in) A real external studio's production Unity CI has a proven pattern: a build/test crash occurring AFTER asset import already completed successfully shouldn't discard the Library cache import produced - only genuine import-time/Library corruption should block caching. Their mechanism distinguishes generic crash evidence (safe to bank if import finished) from independently-verified corruption-specific signals (block unconditionally, even with import otherwise complete). Two real gaps closed: 1. standardBuildAutomation() had no try/catch around runTaskInWorkflow - a failed build never reached the cache-save call at all, not gated, structurally unreachable. Now wrapped in try/catch: on failure, an opt-in cache-floor save is attempted before the original error is re-thrown unmodified - this never swallows or replaces the real failure, only banks a cache alongside it. 2. UnityBuildDiagnosticsService already computed importCompleted (log pattern match or Library/ArtifactDB mtime advancing past a pre-run baseline) and LocalCacheService.saveCacheFolder already had a skipOnCrashEvidence gate - neither was ever wired to the other. Now: on failure, diagnostics are computed from the same $LOG_FILE the retry feature already reads, and the decision is `importCompleted && !isCorruptionSpecificCategory(failureCategory)` (COMPILE/PACKAGE = corruption-specific, blocked unconditionally; CRASH/LICENSE/EXIT_NEG1/GENERIC = generic, bankable if import completed) - reusing the diagnostics service's existing classification, not a new detector. New --localCacheSaveOnFailure flag (default off, separate from --localCacheEnabled): banking a cache from a failed build is a real behavior change beyond merely enabling caching, matching the same caution already applied to --enableBuildRetry. Scoped to the bare local/local-system provider only. Does not touch CacheCheckpointService (a separate, pre-existing, cruder failure-save mechanism for the containerized S3/rclone cache path only - saves on any non-zero exit with no import-completion or corruption awareness) or MiddlewareService/hooks (confirmed structurally unable to reach this decision - hooks run inside the build's own shell script, this decision is made by the Node orchestrator process after that script exits). --- .../cli-plugin/build-parameters-adapter.ts | 2 + .../cli-plugin/orchestrator-options-plugin.ts | 13 + .../src/model/build-parameters.ts | 10 + .../cache/local-cache-service.test.ts | 61 ++++ .../services/cache/local-cache-service.ts | 41 ++- ...ld-automation-workflow.cache-floor.test.ts | 311 ++++++++++++++++++ .../workflows/build-automation-workflow.ts | 210 +++++++++++- 7 files changed, 630 insertions(+), 18 deletions(-) create mode 100644 plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.cache-floor.test.ts diff --git a/plugins/orchestrator/src/cli-plugin/build-parameters-adapter.ts b/plugins/orchestrator/src/cli-plugin/build-parameters-adapter.ts index 1d4574b5..ed31c8a9 100644 --- a/plugins/orchestrator/src/cli-plugin/build-parameters-adapter.ts +++ b/plugins/orchestrator/src/cli-plugin/build-parameters-adapter.ts @@ -120,6 +120,8 @@ export function createBuildParametersFromCliOptions(options: Record options.localCacheFallback === true || options.localCacheFallback === 'true'; bp.localCacheFallbackKeys = options.localCacheFallbackKeys || ''; bp.localCacheMode = options.localCacheMode || 'tar'; + bp.localCacheSaveOnFailure = + options.localCacheSaveOnFailure === true || options.localCacheSaveOnFailure === 'true'; bp.childWorkspacesEnabled = options.childWorkspacesEnabled === true || options.childWorkspacesEnabled === 'true'; bp.childWorkspaceName = options.childWorkspaceName || ''; diff --git a/plugins/orchestrator/src/cli-plugin/orchestrator-options-plugin.ts b/plugins/orchestrator/src/cli-plugin/orchestrator-options-plugin.ts index 0a26f91a..05c9382c 100644 --- a/plugins/orchestrator/src/cli-plugin/orchestrator-options-plugin.ts +++ b/plugins/orchestrator/src/cli-plugin/orchestrator-options-plugin.ts @@ -478,6 +478,19 @@ export function configureOrchestratorOptions(yargs: any): void { default: '', }); + yargs.option('localCacheSaveOnFailure', { + description: + 'Bank a "cache floor" Library/LFS save even when the build/test FAILS, for the bare-host ' + + '`local`/`local-system` provider strategy (requires localCacheEnabled). A save is only ' + + 'attempted if UnityBuildDiagnosticsService reports asset import completed and the failure is ' + + 'not corruption-specific (COMPILE/PACKAGE); a plain crash/license/generic failure after a ' + + 'clean import still banks. Default off: previously a failed build never touched the cache at ' + + 'all -- this is a meaningful behavior change existing users must opt into, matching the ' + + 'caution shown for enableBuildRetry.', + type: 'boolean', + default: false, + }); + yargs.option('localCacheFallback', { description: 'Allow restoring from a fallback cache key when the exact key misses', type: 'boolean', diff --git a/plugins/orchestrator/src/model/build-parameters.ts b/plugins/orchestrator/src/model/build-parameters.ts index 62de206c..c5344d06 100644 --- a/plugins/orchestrator/src/model/build-parameters.ts +++ b/plugins/orchestrator/src/model/build-parameters.ts @@ -125,6 +125,16 @@ class BuildParameters { localCacheFallback!: boolean; localCacheFallbackKeys!: string; localCacheMode!: string; + // Opt-in (default false) "cache floor" save: when a build/test on the + // bare-host `local`/`local-system` provider FAILS, still attempt a + // best-effort Library/LFS cache save if diagnostics show asset import + // completed and the failure isn't corruption-specific (see + // UnityBuildDiagnosticsService / isCorruptionSpecificCategory in + // BuildAutomationWorkflow). Off by default: previously a failed build + // never touched the cache at all, so this is a meaningful behavior + // change existing users must opt into -- same caution as + // enableBuildRetry. Requires localCacheEnabled to also be on. + localCacheSaveOnFailure!: boolean; childWorkspacesEnabled!: boolean; childWorkspaceName!: string; childWorkspaceCacheRoot!: string; diff --git a/plugins/orchestrator/src/model/orchestrator/services/cache/local-cache-service.test.ts b/plugins/orchestrator/src/model/orchestrator/services/cache/local-cache-service.test.ts index e5a8a88c..ffe9f8ec 100644 --- a/plugins/orchestrator/src/model/orchestrator/services/cache/local-cache-service.test.ts +++ b/plugins/orchestrator/src/model/orchestrator/services/cache/local-cache-service.test.ts @@ -230,6 +230,67 @@ describe('LocalCacheService', () => { }); }); + describe('saveEngineCache gating (skipOnCorruptionEvidence / skipOnCrashEvidence)', () => { + function mockCompleteLibraryFolder(): void { + (mockFs.existsSync as vi.Mock).mockReturnValue(true); + (mockFs.readdirSync as vi.Mock).mockImplementation((dirPath: string) => { + if (String(dirPath).includes('Library') && !String(dirPath).includes('cache')) { + return ['file1.asset', 'file2.asset']; + } + return []; + }); + (mockFs.statSync as vi.Mock).mockReturnValue({ mtimeMs: Date.now(), size: 1 }); + (mockFs.mkdirSync as vi.Mock).mockReturnValue(undefined); + } + + it('skips unconditionally when skipOnCorruptionEvidence is true, even with importCompleted true', async () => { + mockCompleteLibraryFolder(); + + await LocalCacheService.saveEngineCache('/project', '/cache', 'key1', { + skipOnCorruptionEvidence: true, + skipOnCrashEvidence: true, + diagnostics: { importCompleted: true, crashEvidenceFound: false }, + }); + + expect(OrchestratorSystem.Run).not.toHaveBeenCalled(); + }); + + it('skips when skipOnCrashEvidence is true and importCompleted is false', async () => { + mockCompleteLibraryFolder(); + + await LocalCacheService.saveEngineCache('/project', '/cache', 'key1', { + skipOnCrashEvidence: true, + diagnostics: { importCompleted: false, crashEvidenceFound: true }, + }); + + expect(OrchestratorSystem.Run).not.toHaveBeenCalled(); + }); + + it('saves when skipOnCrashEvidence is true but importCompleted is true and not corruption-specific', async () => { + mockCompleteLibraryFolder(); + OrchestratorSystem.Run.mockResolvedValue(''); + + await LocalCacheService.saveEngineCache('/project', '/cache', 'key1', { + skipOnCrashEvidence: true, + diagnostics: { importCompleted: true, crashEvidenceFound: true }, + }); + + expect(OrchestratorSystem.Run).toHaveBeenCalledWith(expect.stringContaining('tar -cf'), true); + }); + + it('the success path (no skipOnCrashEvidence/skipOnCorruptionEvidence set) saves exactly as before', async () => { + mockCompleteLibraryFolder(); + OrchestratorSystem.Run.mockResolvedValue(''); + + await LocalCacheService.saveEngineCache('/project', '/cache', 'key1', { + saveMode: 'tar', + maxCacheEntries: 2, + }); + + expect(OrchestratorSystem.Run).toHaveBeenCalledWith(expect.stringContaining('tar -cf'), true); + }); + }); + describe('restoreLfsCache', () => { it('should return false on cache miss', async () => { (mockFs.existsSync as vi.Mock).mockReturnValue(false); diff --git a/plugins/orchestrator/src/model/orchestrator/services/cache/local-cache-service.ts b/plugins/orchestrator/src/model/orchestrator/services/cache/local-cache-service.ts index 2c729d50..e4063e52 100644 --- a/plugins/orchestrator/src/model/orchestrator/services/cache/local-cache-service.ts +++ b/plugins/orchestrator/src/model/orchestrator/services/cache/local-cache-service.ts @@ -14,8 +14,33 @@ export interface LocalCacheRestoreOptions { } export interface LocalCacheSaveOptions { - diagnostics?: { crashEvidenceFound?: boolean }; + /** + * Diagnostics computed for the run that produced this save (see + * UnityBuildDiagnosticsService.analyzeRun). `importCompleted` gates the + * `skipOnCrashEvidence` decision below; `crashEvidenceFound` is carried + * through for logging/observability only -- it does not gate anything + * here by itself. + */ + diagnostics?: { crashEvidenceFound?: boolean; importCompleted?: boolean }; + /** + * This save is happening on a build/test FAILURE path (a "cache floor" + * save after a failed run, not the normal post-success save). Refuse to + * save unless `diagnostics.importCompleted` is true: a run that failed + * before asset import finished cannot be trusted to leave behind a usable + * Library, so there is nothing worth banking. Always overridden by + * `skipOnCorruptionEvidence` below when both are set. The success path + * must never set this flag. + */ skipOnCrashEvidence?: boolean; + /** + * The failure has been classified as corruption-specific (e.g. the + * COMPILE / PACKAGE UnityFailureCategory values -- see + * isCorruptionSpecificCategory in BuildAutomationWorkflow). Refuse to + * save UNCONDITIONALLY, even if `diagnostics.importCompleted` is true -- + * these categories indicate the Library/PackageCache content itself may + * be broken, not merely that the process crashed after a clean import. + */ + skipOnCorruptionEvidence?: boolean; skipOnLfsPointerPoisoning?: boolean; saveMode?: LocalCacheMode; maxCacheEntries?: number; @@ -285,9 +310,19 @@ export class LocalCacheService { const folderPath = path.join(projectPath, folder); try { - if (options.skipOnCrashEvidence && options.diagnostics?.crashEvidenceFound) { + if (options.skipOnCorruptionEvidence) { + OrchestratorLogger.logWarning( + `[LocalCache] ${folder} save skipped: corruption-specific failure evidence found ` + + `(unconditional block, regardless of import completion)`, + ); + + return; + } + + if (options.skipOnCrashEvidence && !options.diagnostics?.importCompleted) { OrchestratorLogger.logWarning( - `[LocalCache] ${folder} save skipped because Unity crash evidence was found`, + `[LocalCache] ${folder} save skipped: build/test failed before asset import completed` + + `${options.diagnostics?.crashEvidenceFound ? ' (crash evidence found)' : ''}`, ); return; diff --git a/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.cache-floor.test.ts b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.cache-floor.test.ts new file mode 100644 index 00000000..652b34e9 --- /dev/null +++ b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.cache-floor.test.ts @@ -0,0 +1,311 @@ +import fs from 'node:fs'; +import path from 'node:path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import BuildParameters from '../../build-parameters'; +import Orchestrator from '../orchestrator'; +import { ContainerHookService } from '../services/hooks/container-hook-service'; +import { OrchestratorStepParameters } from '../options/orchestrator-step-parameters'; + +// Same mocking approach as build-automation-workflow.local-cache.test.ts: the +// workflow dynamically imports LocalCacheService, so mock the module to +// assert call args/gating options without touching the filesystem or +// shelling out to `tar`. +vi.mock('../services/cache/local-cache-service', () => ({ + LocalCacheService: { + resolveCacheRoot: vi.fn(() => '/fake/cache/root'), + generateCacheKey: vi.fn(() => 'fake-cache-key'), + generateCacheKeyCandidates: vi.fn(() => ['fake-cache-key']), + restoreLfsCache: vi.fn(async () => true), + restoreEngineCache: vi.fn(async () => true), + saveEngineCache: vi.fn(async () => undefined), + saveLfsCache: vi.fn(async () => undefined), + }, +})); + +// eslint-disable-next-line import/first -- must follow vi.mock (hoisted anyway, but keep them adjacent) +import { BuildAutomationWorkflow } from './build-automation-workflow'; +// eslint-disable-next-line import/first +import { LocalCacheService } from '../services/cache/local-cache-service'; + +function makeBuildParameters(overrides: Partial = {}): BuildParameters { + const bp = new BuildParameters(); + bp.providerStrategy = 'local'; + bp.commandHooks = ''; + bp.preBuildContainerHooks = ''; + bp.postBuildContainerHooks = ''; + bp.cacheKey = 'test-cache-key'; + bp.projectPath = 'test-project'; + bp.targetPlatform = 'StandaloneLinux64'; + bp.editorVersion = '2021.3.0f1'; + bp.branch = 'main'; + bp.buildName = 'StandaloneLinux64'; + bp.buildPath = 'build/StandaloneLinux64'; + bp.buildFile = 'StandaloneLinux64'; + bp.buildMethod = ''; + bp.buildVersion = '0.0.1'; + bp.androidVersionCode = ''; + bp.chownFilesTo = ''; + bp.manualExit = false; + bp.buildProfile = ''; + bp.skipActivation = false; + bp.dockerWorkspacePath = '/github/workspace'; + bp.orchestratorRepoName = 'game-ci/orchestrator'; + bp.orchestratorBranch = 'main'; + bp.gitAuthMode = 'header'; + bp.logId = 'test-log-id'; + bp.buildGuid = 'test-build-guid'; + bp.maxRetainedWorkspaces = 0; + bp.repoPathOverride = ''; + bp.preflightSuite = ''; + bp.localCacheEnabled = false; + bp.localCacheLibrary = false; + bp.localCacheLfs = false; + bp.localCacheSaveOnFailure = false; + bp.localCacheMode = 'tar'; + bp.maxCacheEntries = 2; + + return Object.assign(bp, overrides); +} + +function makeStepState(): OrchestratorStepParameters { + return new OrchestratorStepParameters('test-image', [], []); +} + +const LOG_DIR = path.join(process.cwd(), 'temp'); +const LOG_FILE = path.join(LOG_DIR, 'job-log.txt'); + +function writeJobLog(content: string): void { + fs.mkdirSync(LOG_DIR, { recursive: true }); + fs.writeFileSync(LOG_FILE, content, 'utf8'); +} + +function stubProviderToFail(errorMessage = 'simulated build failure'): void { + Orchestrator.Provider = { + runTaskInWorkflow: vi.fn(async () => { + throw new Error(errorMessage); + }), + } as any; + + vi.spyOn(ContainerHookService, 'RunPreBuildSteps').mockResolvedValue(''); + vi.spyOn(ContainerHookService, 'RunPostBuildSteps').mockResolvedValue(''); +} + +function stubProviderToSucceed(): void { + Orchestrator.Provider = { + runTaskInWorkflow: vi.fn(async () => 'build output'), + } as any; + + vi.spyOn(ContainerHookService, 'RunPreBuildSteps').mockResolvedValue(''); + vi.spyOn(ContainerHookService, 'RunPostBuildSteps').mockResolvedValue(''); +} + +describe('BuildAutomationWorkflow cache-floor-on-failure wiring (isBareLocalProvider only)', () => { + afterEach(() => { + Orchestrator.buildParameters = undefined as any; + Orchestrator.Provider = undefined as any; + vi.clearAllMocks(); + vi.restoreAllMocks(); + // clearAllMocks() resets call history but not a mockRejectedValue/ + // mockResolvedValue override from a prior test -- restore the module's + // default resolved behavior explicitly so tests don't leak into each other. + (LocalCacheService.saveEngineCache as any).mockResolvedValue(undefined); + try { + fs.rmSync(LOG_FILE, { force: true }); + } catch { + // ignore + } + }); + + it('(f) always propagates the original build failure, even when a floor save happens', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + writeJobLog('AssetDatabase Refresh completed\nRUNSTEPS_EXIT_CODE:134\n'); + stubProviderToFail('boom: unity crashed'); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow( + 'boom: unity crashed', + ); + }); + + it('(a) banks the cache when import completed and the failure category is generic (CRASH)', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + // "Segmentation fault" -> crashEvidenceFound -> CRASH category. + // "Refresh completed" -> importCompleted true. + writeJobLog( + 'Unity Editor log\nAssetDatabase Refresh completed\nSegmentation fault\nRUNSTEPS_EXIT_CODE:139\n', + ); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + expect(options.skipOnCorruptionEvidence).toBe(false); + expect(options.skipOnCrashEvidence).toBe(true); + expect(options.diagnostics.importCompleted).toBe(true); + }); + + it('(a) banks the cache for a LICENSE failure with import completed', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + writeJobLog( + 'Unity Editor log\nAssetDatabase Refresh completed\nNo valid license\nRUNSTEPS_EXIT_CODE:1\n', + ); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + expect(options.skipOnCorruptionEvidence).toBe(false); + }); + + it('(b) does NOT bank the cache when import completed but the category is COMPILE (corruption-specific)', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + writeJobLog( + 'Unity Editor log\nAssetDatabase Refresh completed\nerror CS0246: some type not found\nRUNSTEPS_EXIT_CODE:1\n', + ); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + expect(options.skipOnCorruptionEvidence).toBe(true); + }); + + it('(b) does NOT bank the cache when import completed but the category is PACKAGE (corruption-specific)', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + writeJobLog( + 'Unity Editor log\nAssetDatabase Refresh completed\n' + + 'error CS0246: type or namespace not found Library/PackageCache/foo\n' + + 'RUNSTEPS_EXIT_CODE:1\n', + ); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + expect(options.skipOnCorruptionEvidence).toBe(true); + }); + + it('(c) does NOT bank the cache when import never completed, regardless of category (GENERIC)', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + // No "Refresh completed" marker and no projectPath-based ArtifactDB -> + // importCompleted stays false. No crash/license/compile signal -> GENERIC. + writeJobLog('Unity Editor log\nsomething went wrong\nRUNSTEPS_EXIT_CODE:1\n'); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + expect(options.diagnostics.importCompleted).toBe(false); + expect(options.skipOnCorruptionEvidence).toBe(false); + expect(options.skipOnCrashEvidence).toBe(true); + }); + + it('(e) --localCacheEnabled off means zero new failure-path behavior (regression guard)', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: false, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + writeJobLog('AssetDatabase Refresh completed\nRUNSTEPS_EXIT_CODE:139\n'); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).not.toHaveBeenCalled(); + expect(LocalCacheService.restoreEngineCache).not.toHaveBeenCalled(); + }); + + it('localCacheEnabled on but localCacheSaveOnFailure off means zero new failure-path behavior (opt-in guard)', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: false, + }); + writeJobLog('AssetDatabase Refresh completed\nRUNSTEPS_EXIT_CODE:139\n'); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).not.toHaveBeenCalled(); + }); + + it('(d) a successful build keeps calling saveEngineCache exactly as before, with no failure-path options (regression guard)', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + stubProviderToSucceed(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).resolves.toBeTypeOf('string'); + + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + expect(options.skipOnCrashEvidence).toBeUndefined(); + expect(options.skipOnCorruptionEvidence).toBeUndefined(); + expect(options.diagnostics).toBeUndefined(); + }); + + it('a floor save failure is logged and does not mask or replace the original build failure', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + writeJobLog('AssetDatabase Refresh completed\nRUNSTEPS_EXIT_CODE:139\n'); + stubProviderToFail('the real build failure'); + (LocalCacheService.saveEngineCache as any).mockRejectedValue( + new Error('simulated floor save failure'), + ); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow( + 'the real build failure', + ); + }); + + it('no LOG_FILE present -> diagnostics analysis still runs (empty log) and does not throw out of the workflow', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + }); + // Deliberately do not write a log file. + stubProviderToFail('build failed, no log file'); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow( + 'build failed, no log file', + ); + // Empty log -> importCompleted false -> not banked, but must not throw. + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + expect(options.diagnostics.importCompleted).toBe(false); + }); +}); diff --git a/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.ts b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.ts index 148c3b21..451315a0 100644 --- a/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.ts +++ b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.ts @@ -4,12 +4,18 @@ import { OrchestratorStepParameters } from '../options/orchestrator-step-paramet import { WorkflowInterface } from './workflow-interface'; import { CommandHookService } from '../services/hooks/command-hook-service'; import path from 'node:path'; +import fs from 'node:fs'; import Orchestrator from '../orchestrator'; import { ContainerHookService } from '../services/hooks/container-hook-service'; import { MiddlewareService } from '../services/hooks/middleware-service'; import { CacheCheckpointService } from '../services/cache/cache-checkpoint-service'; import { PreflightService } from '../services/preflight'; import { getEngine } from '../../engine'; +import { + UnityBuildDiagnosticsService, + UnityFailureCategory, + UnityRunDiagnostics, +} from '../services/reliability/unity-build-diagnostics-service'; export class BuildAutomationWorkflow implements WorkflowInterface { async run(orchestratorStepState: OrchestratorStepParameters) { @@ -47,21 +53,35 @@ export class BuildAutomationWorkflow implements WorkflowInterface { // (this code path is the standalone `game-ci orchestrate` CLI command). const localCacheState = await BuildAutomationWorkflow.restoreLocalCacheIfEnabled(); - output += await Orchestrator.Provider.runTaskInWorkflow( - Orchestrator.buildParameters.buildGuid, - baseImage.toString(), - BuildAutomationWorkflow.BuildWorkflow, - `/${OrchestratorFolders.buildVolumeFolder}`, - `/${OrchestratorFolders.buildVolumeFolder}/`, - orchestratorStepState.environment, - orchestratorStepState.secrets, - ); - OrchestratorLogger.logWithTime('Build time'); - - // Save step mirrors plugin-lifecycle.ts's afterLocalBuild() handling. - // Only reached if runTaskInWorkflow above resolved rather than throwing, - // i.e. Unity actually ran to completion. - await BuildAutomationWorkflow.saveLocalCacheIfEnabled(localCacheState); + try { + output += await Orchestrator.Provider.runTaskInWorkflow( + Orchestrator.buildParameters.buildGuid, + baseImage.toString(), + BuildAutomationWorkflow.BuildWorkflow, + `/${OrchestratorFolders.buildVolumeFolder}`, + `/${OrchestratorFolders.buildVolumeFolder}/`, + orchestratorStepState.environment, + orchestratorStepState.secrets, + ); + OrchestratorLogger.logWithTime('Build time'); + + // Save step mirrors plugin-lifecycle.ts's afterLocalBuild() handling. + // Only reached if runTaskInWorkflow above resolved rather than + // throwing, i.e. Unity actually ran to completion. Success-path + // behavior is unchanged by the failure-path addition below. + await BuildAutomationWorkflow.saveLocalCacheIfEnabled(localCacheState); + } catch (error) { + // runTaskInWorkflow threw -- Unity's build/test run failed. Previously + // saveLocalCacheIfEnabled was structurally unreachable from here (it + // sat after this call, on the success-only path), so a crash *after* + // import already completed discarded a Library that was otherwise + // fine to keep. Evaluate an opt-in "cache floor" save before + // re-throwing -- this never swallows or replaces the original + // failure, it only banks a cache alongside it. + await BuildAutomationWorkflow.saveLocalCacheOnFailureIfEnabled(localCacheState); + + throw error; + } output += await ContainerHookService.RunPostBuildSteps(orchestratorStepState); OrchestratorLogger.logWithTime('Configurable post build step(s) time'); @@ -239,6 +259,166 @@ export class BuildAutomationWorkflow implements WorkflowInterface { } } + /** + * UnityFailureCategory values that indicate the Library/PackageCache + * content itself may be broken, not just that the process crashed after a + * clean import -- conceptually the same class of risk as a corruption + * poison sentinel, distinct from a generic process-level failure: + * + * - COMPILE: compile errors can indicate a broken/incomplete import that + * still looks import-complete but fails downstream. + * - PACKAGE: PackageCache/GUID corruption (categorizeFailure's guidErrors + * check, itself detecting `error CS0246` under Library/PackageCache) -- + * a corrupted cache subsystem specifically, not a crashed process. + * + * CRASH, LICENSE, EXIT_NEG1 and GENERIC are process/environment-level + * failures unrelated to Library content, so they remain bankable as long + * as diagnostics.importCompleted is true. SKIP/SUCCESS are not reachable + * here -- this is only ever called on the failure path. + */ + private static isCorruptionSpecificCategory(category: UnityFailureCategory): boolean { + return category === 'COMPILE' || category === 'PACKAGE'; + } + + /** + * Re-read the same $LOG_FILE convention BuildWorkflow exports for the bare + * `local`/`local-system` provider (`$(pwd)/temp/job-log.txt`) and the exit + * code the bare-local BuildCommands branch writes into it + * (`RUNSTEPS_EXIT_CODE:$?`, appended right after runsteps.sh exits -- see + * BuildCommands' isBareLocalProvider branch), then classify the failed run + * via UnityBuildDiagnosticsService. Mirrors LocalOrchestrator.runWithRetry's + * own log-resolution approach (providers/local/index.ts) so this failure + * path and the retry feature agree on where the real Editor log content + * lives. + * + * Returns undefined (never throws) if the log can't be read/parsed -- + * callers must treat that as "cannot classify this failure" and therefore + * not bank anything. + */ + private static analyzeRunForCacheFloor(): UnityRunDiagnostics | undefined { + try { + const bp = Orchestrator.buildParameters; + const projectPath = path.isAbsolute(bp.projectPath || '') + ? bp.projectPath + : path.join(process.cwd(), bp.projectPath || '.'); + const logFilePath = path.join(process.cwd(), 'temp', 'job-log.txt'); + const logText = BuildAutomationWorkflow.readEditorLogForCacheFloor(logFilePath); + + const exitCodeMatch = logText.match(/RUNSTEPS_EXIT_CODE:(-?\d+)/); + // A failure with no recovered exit code still needs a nonzero + // placeholder -- categorizeFailure() has an exitCode === 0 branch + // (SUCCESS) that must never be hit here, since this is only invoked + // once runTaskInWorkflow has already thrown. + const exitCode = exitCodeMatch ? Number(exitCodeMatch[1]) : 1; + + return UnityBuildDiagnosticsService.analyzeRun({ + exitCode, + logText, + projectPath, + }); + } catch (error: any) { + OrchestratorLogger.logWarning( + `[LocalCache] Failure-path diagnostics analysis failed, skipping cache floor save: ${error.message}`, + ); + + return undefined; + } + } + + private static readEditorLogForCacheFloor(logFilePath: string): string { + try { + return fs.readFileSync(logFilePath, 'utf8'); + } catch { + return ''; + } + } + + /** + * Opt-in (--localCacheSaveOnFailure, default off) "cache floor" save: on + * the bare-host `local`/`local-system` provider strategy, when Unity's + * build/test run has just thrown, attempt a best-effort Library/LFS save + * anyway if diagnostics show the failure occurred after asset import + * already completed and is not corruption-specific (see + * isCorruptionSpecificCategory above). Mirrors the shape of + * saveLocalCacheIfEnabled but is only ever reached from the failure path + * (see the catch block in standardBuildAutomation) -- never called on + * success. + * + * Must never throw: any error here is logged and swallowed exactly like + * saveLocalCacheIfEnabled's own try/catch, so a failed cache-floor save can + * never mask or replace the real build failure the caller re-throws right + * after this returns. + */ + private static async saveLocalCacheOnFailureIfEnabled( + cacheState: { cacheRoot: string; cacheKey: string } | undefined, + ): Promise { + const bp = Orchestrator.buildParameters; + if ( + !BuildAutomationWorkflow.isBareLocalProvider || + !bp.localCacheEnabled || + !bp.localCacheSaveOnFailure || + !cacheState + ) { + return; + } + + try { + const diagnostics = BuildAutomationWorkflow.analyzeRunForCacheFloor(); + if (!diagnostics) { + return; + } + + const isCorruptionSpecific = BuildAutomationWorkflow.isCorruptionSpecificCategory( + diagnostics.failureCategory, + ); + const shouldBankAsFloor = diagnostics.importCompleted && !isCorruptionSpecific; + + OrchestratorLogger.log( + `[LocalCache] Cache floor evaluation: category=${diagnostics.failureCategory} ` + + `importCompleted=${diagnostics.importCompleted} corruptionSpecific=${isCorruptionSpecific} ` + + `-> ${shouldBankAsFloor ? 'eligible to bank as floor' : 'not eligible'}`, + ); + + const { LocalCacheService } = await import('../services/cache/local-cache-service'); + const { cacheRoot, cacheKey } = cacheState; + const workspacePath = process.cwd(); + const projectFullPath = path.join(workspacePath, bp.projectPath); + + if (bp.localCacheLibrary) { + // LocalCacheService.saveCacheFolder is the final arbiter: it enforces + // both the unconditional corruption-specific block and the + // import-completed requirement itself (see LocalCacheSaveOptions), + // so this is defense-in-depth alongside the shouldBankAsFloor log + // line above, not a second independent decision. + await LocalCacheService.saveEngineCache(projectFullPath, cacheRoot, cacheKey, { + saveMode: bp.localCacheMode as any, + skipOnLfsPointerPoisoning: true, + maxCacheEntries: bp.maxCacheEntries, + diagnostics: { + crashEvidenceFound: diagnostics.crashEvidenceFound, + importCompleted: diagnostics.importCompleted, + }, + skipOnCrashEvidence: true, + skipOnCorruptionEvidence: isCorruptionSpecific, + }); + } + + // saveLfsCache has no diagnostics-aware gate (LFS content isn't + // Library-corruption-specific and its save mechanics are intentionally + // left untouched here), so gate it directly with the same decision. + if (bp.localCacheLfs && shouldBankAsFloor) { + await LocalCacheService.saveLfsCache( + workspacePath, + cacheRoot, + cacheKey, + bp.maxCacheEntries, + ); + } + } catch (error: any) { + OrchestratorLogger.logWarning(`[LocalCache] Cache floor save failed: ${error.message}`); + } + } + private static get BuildWorkflow() { // Load middleware once per build (not once per phase/timing slot) and // merge its resolved command hooks in with the legacy command-hook list. From 35e8fbbdbc89584c9e13da5e67e962991a3932a7 Mon Sep 17 00:00:00 2001 From: frostebite Date: Sat, 22 Aug 2026 22:28:00 +0100 Subject: [PATCH 2/2] feat: make cache-floor corruption categories configurable Adds --localCacheFloorCorruptionCategories to override which UnityFailureCategory values are treated as corruption-specific (unconditionally blocking a cache-floor save) instead of hardcoding COMPILE/PACKAGE. Falls back to the built-in default when unset or when the override contains no recognized categories, with a warning logged for unrecognized entries. --- .../cli-plugin/build-parameters-adapter.ts | 1 + .../cli-plugin/orchestrator-options-plugin.ts | 10 +++ .../src/model/build-parameters.ts | 6 ++ ...ld-automation-workflow.cache-floor.test.ts | 63 +++++++++++++++++++ .../workflows/build-automation-workflow.ts | 51 ++++++++++++++- 5 files changed, 130 insertions(+), 1 deletion(-) diff --git a/plugins/orchestrator/src/cli-plugin/build-parameters-adapter.ts b/plugins/orchestrator/src/cli-plugin/build-parameters-adapter.ts index ed31c8a9..5405af89 100644 --- a/plugins/orchestrator/src/cli-plugin/build-parameters-adapter.ts +++ b/plugins/orchestrator/src/cli-plugin/build-parameters-adapter.ts @@ -122,6 +122,7 @@ export function createBuildParametersFromCliOptions(options: Record bp.localCacheMode = options.localCacheMode || 'tar'; bp.localCacheSaveOnFailure = options.localCacheSaveOnFailure === true || options.localCacheSaveOnFailure === 'true'; + bp.localCacheFloorCorruptionCategories = options.localCacheFloorCorruptionCategories || ''; bp.childWorkspacesEnabled = options.childWorkspacesEnabled === true || options.childWorkspacesEnabled === 'true'; bp.childWorkspaceName = options.childWorkspaceName || ''; diff --git a/plugins/orchestrator/src/cli-plugin/orchestrator-options-plugin.ts b/plugins/orchestrator/src/cli-plugin/orchestrator-options-plugin.ts index 05c9382c..d053686d 100644 --- a/plugins/orchestrator/src/cli-plugin/orchestrator-options-plugin.ts +++ b/plugins/orchestrator/src/cli-plugin/orchestrator-options-plugin.ts @@ -491,6 +491,16 @@ export function configureOrchestratorOptions(yargs: any): void { default: false, }); + yargs.option('localCacheFloorCorruptionCategories', { + description: + 'Comma-separated UnityFailureCategory list that blocks a --localCacheSaveOnFailure "cache ' + + 'floor" save unconditionally, even when asset import completed (default: COMPILE,PACKAGE -- ' + + 'categories: LICENSE, CRASH, COMPILE, PACKAGE, SKIP, EXIT_NEG1, GENERIC). Only meaningful ' + + 'together with localCacheSaveOnFailure.', + type: 'string', + default: '', + }); + yargs.option('localCacheFallback', { description: 'Allow restoring from a fallback cache key when the exact key misses', type: 'boolean', diff --git a/plugins/orchestrator/src/model/build-parameters.ts b/plugins/orchestrator/src/model/build-parameters.ts index c5344d06..52a819fc 100644 --- a/plugins/orchestrator/src/model/build-parameters.ts +++ b/plugins/orchestrator/src/model/build-parameters.ts @@ -135,6 +135,12 @@ class BuildParameters { // change existing users must opt into -- same caution as // enableBuildRetry. Requires localCacheEnabled to also be on. localCacheSaveOnFailure!: boolean; + // Overrides which UnityFailureCategory values block a cache-floor save + // unconditionally (see BuildAutomationWorkflow.corruptionSpecificCategories). + // Comma-separated, e.g. "COMPILE,PACKAGE". Empty/unset uses the built-in + // default (COMPILE, PACKAGE) -- this only needs setting to override that + // default for a specific environment's known failure characteristics. + localCacheFloorCorruptionCategories!: string; childWorkspacesEnabled!: boolean; childWorkspaceName!: string; childWorkspaceCacheRoot!: string; diff --git a/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.cache-floor.test.ts b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.cache-floor.test.ts index 652b34e9..dd60f2a8 100644 --- a/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.cache-floor.test.ts +++ b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.cache-floor.test.ts @@ -208,6 +208,69 @@ describe('BuildAutomationWorkflow cache-floor-on-failure wiring (isBareLocalProv expect(options.skipOnCorruptionEvidence).toBe(true); }); + it('localCacheFloorCorruptionCategories override narrows the default: PACKAGE removed -> banks despite the built-in default treating it as corruption-specific', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + localCacheFloorCorruptionCategories: 'COMPILE', + }); + writeJobLog( + 'Unity Editor log\nAssetDatabase Refresh completed\n' + + 'error CS0246: type or namespace not found Library/PackageCache/foo\n' + + 'RUNSTEPS_EXIT_CODE:1\n', + ); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + expect(options.skipOnCorruptionEvidence).toBe(false); + }); + + it('localCacheFloorCorruptionCategories override widens the default: CRASH added -> blocks a category the built-in default would bank', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + localCacheFloorCorruptionCategories: 'COMPILE, PACKAGE, CRASH', + }); + writeJobLog( + 'Unity Editor log\nAssetDatabase Refresh completed\nSegmentation fault\nRUNSTEPS_EXIT_CODE:139\n', + ); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + expect(options.skipOnCorruptionEvidence).toBe(true); + }); + + it('localCacheFloorCorruptionCategories with only unrecognized entries falls back to the built-in default rather than treating everything as bankable', async () => { + Orchestrator.buildParameters = makeBuildParameters({ + localCacheEnabled: true, + localCacheLibrary: true, + localCacheSaveOnFailure: true, + localCacheFloorCorruptionCategories: 'NOT_A_REAL_CATEGORY', + }); + writeJobLog( + 'Unity Editor log\nAssetDatabase Refresh completed\n' + + 'error CS0246: type or namespace not found Library/PackageCache/foo\n' + + 'RUNSTEPS_EXIT_CODE:1\n', + ); + stubProviderToFail(); + + await expect(new BuildAutomationWorkflow().run(makeStepState())).rejects.toThrow(); + + expect(LocalCacheService.saveEngineCache).toHaveBeenCalledTimes(1); + const [, , , options] = (LocalCacheService.saveEngineCache as any).mock.calls[0]; + // Falls back to the default (COMPILE, PACKAGE) since the override had + // nothing usable in it -- PACKAGE is still blocked. + expect(options.skipOnCorruptionEvidence).toBe(true); + }); + it('(c) does NOT bank the cache when import never completed, regardless of category (GENERIC)', async () => { Orchestrator.buildParameters = makeBuildParameters({ localCacheEnabled: true, diff --git a/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.ts b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.ts index 451315a0..3445b940 100644 --- a/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.ts +++ b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.ts @@ -275,9 +275,58 @@ export class BuildAutomationWorkflow implements WorkflowInterface { * failures unrelated to Library content, so they remain bankable as long * as diagnostics.importCompleted is true. SKIP/SUCCESS are not reachable * here -- this is only ever called on the failure path. + * + * This default is configurable, not hardcoded policy: --localCacheFloor + * -CorruptionCategories overrides it with a caller-supplied comma-separated + * UnityFailureCategory list (e.g. an environment that's confident PACKAGE + * failures never actually indicate corruption for its own project can + * narrow this to just "COMPILE"). Unrecognized category names in an + * override are ignored with a warning rather than silently mismatching. */ + private static readonly DEFAULT_CORRUPTION_SPECIFIC_CATEGORIES: readonly UnityFailureCategory[] = + ['COMPILE', 'PACKAGE']; + private static isCorruptionSpecificCategory(category: UnityFailureCategory): boolean { - return category === 'COMPILE' || category === 'PACKAGE'; + return BuildAutomationWorkflow.corruptionSpecificCategories().includes(category); + } + + private static corruptionSpecificCategories(): readonly UnityFailureCategory[] { + const override = Orchestrator.buildParameters?.localCacheFloorCorruptionCategories; + if (!override) { + return BuildAutomationWorkflow.DEFAULT_CORRUPTION_SPECIFIC_CATEGORIES; + } + + const knownCategories = new Set([ + 'LICENSE', + 'CRASH', + 'COMPILE', + 'PACKAGE', + 'SKIP', + 'EXIT_NEG1', + 'GENERIC', + 'SUCCESS', + ]); + + const parsed: UnityFailureCategory[] = []; + for (const raw of override.split(',')) { + const category = raw.trim().toUpperCase() as UnityFailureCategory; + if (!category) continue; + if (knownCategories.has(category)) { + parsed.push(category); + } else { + OrchestratorLogger.logWarning( + `[LocalCache] Ignoring unrecognized category "${raw.trim()}" in --localCacheFloorCorruptionCategories`, + ); + } + } + + // An override that resolves to nothing usable (e.g. all-unrecognized + // input) falls back to the default rather than silently treating every + // failure as bankable -- an empty corruption list is a meaningful, + // security-relevant choice that should be explicit, not accidental. + return parsed.length > 0 + ? parsed + : BuildAutomationWorkflow.DEFAULT_CORRUPTION_SPECIFIC_CATEGORIES; } /**