diff --git a/plugins/orchestrator/src/model/orchestrator/orchestrator.ts b/plugins/orchestrator/src/model/orchestrator/orchestrator.ts index e78a7453..c4e70275 100644 --- a/plugins/orchestrator/src/model/orchestrator/orchestrator.ts +++ b/plugins/orchestrator/src/model/orchestrator/orchestrator.ts @@ -397,6 +397,27 @@ class Orchestrator { return new OrchestratorResult(buildParameters, output, true, true, false); } catch (error: any) { + // Release first: logging/status reporting below may itself throw, and no + // secondary failure should be able to strand a retained-workspace lock. + if ( + BuildParameters.shouldUseRetainedWorkspaceMode(Orchestrator.buildParameters) && + Orchestrator.lockedWorkspace + ) { + try { + await SharedWorkspaceLocking.ReleaseWorkspace( + Orchestrator.lockedWorkspace, + Orchestrator.buildParameters.buildGuid, + Orchestrator.buildParameters, + ); + } catch (releaseError: any) { + OrchestratorLogger.log( + `Failed to release workspace lock for ${Orchestrator.lockedWorkspace} after build failure: ${OrchestratorLogger.stringifyError(releaseError)}`, + ); + } finally { + Orchestrator.lockedWorkspace = ``; + } + } + OrchestratorLogger.log(OrchestratorLogger.stringifyError(error)); await GitHub.updateGitHubCheck( Orchestrator.buildParameters.buildGuid, @@ -405,6 +426,7 @@ class Orchestrator { `completed`, ); if (!Orchestrator.buildParameters.isCliMode) core.endGroup(); + await OrchestratorError.handleException( error, Orchestrator.buildParameters, 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 ffe9f8ec..91074550 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 @@ -368,6 +368,112 @@ describe('LocalCacheService', () => { }); }); + describe('sweepStaleLocks', () => { + it('should return 0 when cache root does not exist', () => { + (mockFs.existsSync as vi.Mock).mockReturnValue(false); + expect(LocalCacheService.sweepStaleLocks('/cache')).toBe(0); + }); + + it('should remove a lock whose owning PID is no longer alive', () => { + (mockFs.existsSync as vi.Mock).mockReturnValue(true); + (mockFs.readdirSync as vi.Mock).mockReturnValue([{ name: 'key1', isDirectory: () => true }]); + (mockFs.readFileSync as vi.Mock).mockReturnValue('99999'); + (mockFs.unlinkSync as vi.Mock).mockReturnValue(undefined); + + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('ESRCH'), { code: 'ESRCH' }); + }); + + try { + const removed = LocalCacheService.sweepStaleLocks('/cache'); + + expect(removed).toBe(1); + expect(mockFs.unlinkSync).toHaveBeenCalledWith( + path.join('/cache', 'key1', '.game-ci-cache-save.lock'), + ); + } finally { + killSpy.mockRestore(); + } + }); + + it('should leave a lock in place when the owning PID is still alive', () => { + (mockFs.existsSync as vi.Mock).mockReturnValue(true); + (mockFs.readdirSync as vi.Mock).mockReturnValue([{ name: 'key1', isDirectory: () => true }]); + (mockFs.readFileSync as vi.Mock).mockReturnValue(String(process.pid)); + (mockFs.unlinkSync as vi.Mock).mockReturnValue(undefined); + + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => true as any); + + try { + const removed = LocalCacheService.sweepStaleLocks('/cache'); + + expect(removed).toBe(0); + expect(mockFs.unlinkSync).not.toHaveBeenCalled(); + } finally { + killSpy.mockRestore(); + } + }); + + it('should leave a fresh pending lock in place while the child PID is being written', () => { + (mockFs.existsSync as vi.Mock).mockReturnValue(true); + (mockFs.readdirSync as vi.Mock).mockReturnValue([{ name: 'key1', isDirectory: () => true }]); + (mockFs.readFileSync as vi.Mock).mockReturnValue('pending'); + (mockFs.statSync as vi.Mock).mockReturnValue({ mtimeMs: Date.now() }); + (mockFs.unlinkSync as vi.Mock).mockReturnValue(undefined); + + const removed = LocalCacheService.sweepStaleLocks('/cache'); + + expect(removed).toBe(0); + expect(mockFs.unlinkSync).not.toHaveBeenCalled(); + }); + + it('should remove an incomplete lock after the background-save timeout', () => { + (mockFs.existsSync as vi.Mock).mockReturnValue(true); + (mockFs.readdirSync as vi.Mock).mockReturnValue([{ name: 'key1', isDirectory: () => true }]); + (mockFs.readFileSync as vi.Mock).mockReturnValue('pending'); + (mockFs.statSync as vi.Mock).mockReturnValue({ mtimeMs: Date.now() - 300_001 }); + (mockFs.unlinkSync as vi.Mock).mockReturnValue(undefined); + + const removed = LocalCacheService.sweepStaleLocks('/cache'); + + expect(removed).toBe(1); + expect(mockFs.unlinkSync).toHaveBeenCalledWith( + path.join('/cache', 'key1', '.game-ci-cache-save.lock'), + ); + }); + + it('should preserve a lock when the PID check fails with EPERM', () => { + (mockFs.existsSync as vi.Mock).mockReturnValue(true); + (mockFs.readdirSync as vi.Mock).mockReturnValue([{ name: 'key1', isDirectory: () => true }]); + (mockFs.readFileSync as vi.Mock).mockReturnValue('12345'); + + const killSpy = vi.spyOn(process, 'kill').mockImplementation(() => { + throw Object.assign(new Error('EPERM'), { code: 'EPERM' }); + }); + + try { + const removed = LocalCacheService.sweepStaleLocks('/cache'); + + expect(removed).toBe(0); + expect(mockFs.unlinkSync).not.toHaveBeenCalled(); + } finally { + killSpy.mockRestore(); + } + }); + + it('should skip cache-key directories with no lock file', () => { + (mockFs.existsSync as vi.Mock).mockImplementation( + (candidate: string) => !String(candidate).endsWith('.lock'), + ); + (mockFs.readdirSync as vi.Mock).mockReturnValue([{ name: 'key1', isDirectory: () => true }]); + + const removed = LocalCacheService.sweepStaleLocks('/cache'); + + expect(removed).toBe(0); + expect(mockFs.unlinkSync).not.toHaveBeenCalled(); + }); + }); + describe('garbageCollect', () => { it('should skip when cache root does not exist', 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 e4063e52..c86aa418 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 @@ -49,6 +49,7 @@ export interface LocalCacheSaveOptions { /** Marker file written during background cache saves, contains the PID. */ const BACKGROUND_LOCK_FILE = '.game-ci-cache-save.lock'; +const BACKGROUND_LOCK_GRACE_MS = 300_000; export class LocalCacheService { /** @@ -935,6 +936,80 @@ export class LocalCacheService { } } + /** + * Proactively sweep orphaned background-save lock files across every cache-key + * directory under cacheRoot. A lock is orphaned when its recorded PID is no + * longer alive, or when an incomplete/unparseable lock is older than the + * background-save timeout. Fresh incomplete locks are preserved because the + * writer briefly stores "pending" before replacing it with the child PID. + * + * waitForBackgroundLock() only self-heals reactively -- it checks a lock file + * when a later save/restore call happens to target that exact cache key. A + * background save killed mid-copy (runner crash, OOM kill) under a cache key + * this run never touches would otherwise leave its lock in place indefinitely, + * since nothing else in the process would ever look at it again. Call this once + * at the start of a build (before any restore) to catch that case too. + * + * Returns the number of stale locks removed. + */ + static sweepStaleLocks(cacheRoot: string): number { + if (!fs.existsSync(cacheRoot)) return 0; + + let cacheKeyDirs: string[]; + try { + cacheKeyDirs = fs + .readdirSync(cacheRoot, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name); + } catch (error: any) { + OrchestratorLogger.logWarning( + `[LocalCache] Failed to scan ${cacheRoot} for stale locks: ${error.message}`, + ); + + return 0; + } + + let swept = 0; + for (const cacheKeyDir of cacheKeyDirs) { + const lockPath = path.join(cacheRoot, cacheKeyDir, BACKGROUND_LOCK_FILE); + if (!fs.existsSync(lockPath)) continue; + + try { + const lockContents = fs.readFileSync(lockPath, 'utf8').trim(); + const pid = /^\d+$/.test(lockContents) ? Number(lockContents) : 0; + if (pid > 0) { + try { + process.kill(pid, 0); // Signal 0 = existence check + // Owning process is still alive -- a save is genuinely in progress. + continue; + } catch (error: any) { + if (error?.code !== 'ESRCH') { + // EPERM means the process exists but is owned by another user; + // unknown errors are likewise not proof that the lock is stale. + continue; + } + } + } else if (Date.now() - fs.statSync(lockPath).mtimeMs < BACKGROUND_LOCK_GRACE_MS) { + continue; + } + + fs.unlinkSync(lockPath); + swept++; + OrchestratorLogger.log(`[LocalCache] Swept stale background-save lock: ${lockPath}`); + } catch (error: any) { + OrchestratorLogger.logWarning( + `[LocalCache] Failed to sweep lock ${lockPath}: ${error.message}`, + ); + } + } + + if (swept > 0) { + OrchestratorLogger.log(`[LocalCache] Stale lock sweep complete: ${swept} lock(s) removed`); + } + + return swept; + } + /** * Wait for a background cache save lock to be released. * Polls the lock file for up to 5 minutes. @@ -951,11 +1026,17 @@ export class LocalCacheService { while (fs.existsSync(lockPath) && Date.now() - start < timeoutMs) { // Check if the PID is still alive try { - const pid = Number.parseInt(fs.readFileSync(lockPath, 'utf8').trim(), 10); + const lockContents = fs.readFileSync(lockPath, 'utf8').trim(); + const pid = /^\d+$/.test(lockContents) ? Number(lockContents) : 0; if (pid > 0) { try { process.kill(pid, 0); // Signal 0 = existence check - } catch { + } catch (error: any) { + if (error?.code !== 'ESRCH') { + // Lack of permission is evidence that the process exists, not + // that the lock is stale. Keep waiting in that case. + continue; + } // Process is gone, remove stale lock OrchestratorLogger.log( '[LocalCache] Background save process exited, removing stale lock', diff --git a/plugins/orchestrator/src/model/orchestrator/services/hooks/middleware-service.test.ts b/plugins/orchestrator/src/model/orchestrator/services/hooks/middleware-service.test.ts index 99979f53..407fa346 100644 --- a/plugins/orchestrator/src/model/orchestrator/services/hooks/middleware-service.test.ts +++ b/plugins/orchestrator/src/model/orchestrator/services/hooks/middleware-service.test.ts @@ -419,5 +419,60 @@ after: expect(result[1].name).toBe('medium'); expect(result[2].name).toBe('high'); }); + + it.each([ + ['command middleware on a container phase', 'command', '[pre-build]', 'cannot use phase'], + ['container middleware on a command phase', 'container', '[build]', 'cannot use phase'], + [ + 'middleware spanning incompatible phase kinds', + 'command', + '[build, post-build]', + 'cannot use phase', + ], + ])('should reject %s', (_description, type, phases, expectedMessage) => { + const yaml = ` +name: invalid-phase +type: ${type} +trigger: + phase: ${phases} +before: echo "test" +`; + + expect(() => MiddlewareService.getMiddleware(yaml)).toThrow(expectedMessage); + }); + + it('should reject allowFailure on command middleware', () => { + const yaml = ` +name: invalid-allow-failure +type: command +allowFailure: true +trigger: + phase: [build] +before: echo "test" +`; + + expect(() => MiddlewareService.getMiddleware(yaml)).toThrow( + 'allowFailure, which is supported only for container middleware', + ); + }); + + it('should reject middleware with no phase or commands', () => { + expect(() => + MiddlewareService.getMiddleware(` +name: missing-phase +type: command +before: echo "test" +`), + ).toThrow('must declare at least one trigger phase'); + + expect(() => + MiddlewareService.getMiddleware(` +name: missing-commands +type: command +trigger: + phase: [build] +`), + ).toThrow('must declare before and/or after commands'); + }); }); }); diff --git a/plugins/orchestrator/src/model/orchestrator/services/hooks/middleware-service.ts b/plugins/orchestrator/src/model/orchestrator/services/hooks/middleware-service.ts index 07af7430..075e4da3 100644 --- a/plugins/orchestrator/src/model/orchestrator/services/hooks/middleware-service.ts +++ b/plugins/orchestrator/src/model/orchestrator/services/hooks/middleware-service.ts @@ -35,6 +35,10 @@ export class MiddlewareService { // Load file-based definitions from game-ci/middleware/ middleware.push(...MiddlewareService.getMiddlewareFromFiles()); + for (const definition of middleware) { + MiddlewareService.validateMiddleware(definition); + } + // Sort by priority (lower = earlier) middleware.sort((a, b) => (a.priority ?? 100) - (b.priority ?? 100)); @@ -43,6 +47,51 @@ export class MiddlewareService { return middleware; } + /** + * Reject configurations that cannot be represented by the underlying hook + * systems. Command hooks are wired only to setup/build; container hooks are + * wired only to pre-build/post-build. + */ + private static validateMiddleware(middleware: Middleware): void { + const commandPhases = new Set(['setup', 'build']); + const containerPhases = new Set(['pre-build', 'post-build']); + const phasesForType = + middleware.type === 'command' + ? commandPhases + : middleware.type === 'container' + ? containerPhases + : undefined; + + if (!phasesForType) { + throw new Error( + `Middleware "${middleware.name}" has unsupported type "${middleware.type}"; expected "command" or "container"`, + ); + } + + if (!middleware.trigger.phase.length) { + throw new Error(`Middleware "${middleware.name}" must declare at least one trigger phase`); + } + + const incompatiblePhases = middleware.trigger.phase.filter( + (phase) => !phasesForType.has(phase), + ); + if (incompatiblePhases.length > 0) { + throw new Error( + `Middleware "${middleware.name}" of type "${middleware.type}" cannot use phase(s): ${incompatiblePhases.join(', ')}`, + ); + } + + if (!middleware.before && !middleware.after) { + throw new Error(`Middleware "${middleware.name}" must declare before and/or after commands`); + } + + if (middleware.type === 'command' && middleware.allowFailure) { + throw new Error( + `Middleware "${middleware.name}" sets allowFailure, which is supported only for container middleware`, + ); + } + } + /** * Resolve middleware to CommandHooks for a given phase and timing. * Filters by trigger conditions and converts to hooks. diff --git a/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.test.ts b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.test.ts index 563ae467..e2a98762 100644 --- a/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.test.ts +++ b/plugins/orchestrator/src/model/orchestrator/workflows/build-automation-workflow.test.ts @@ -227,7 +227,7 @@ after: expect(buildAfterIndex).toBeGreaterThan(buildRunIndex); }); - it('filters out middleware whose trigger phase does not match either slot', () => { + it('rejects command middleware configured for a container-only phase', () => { Orchestrator.buildParameters = makeBuildParameters({ middlewarePipeline: ` name: prebuild-only-mw @@ -239,9 +239,9 @@ before: `, }); - const script = getBuildWorkflow(); - - expect(script).not.toContain('should-not-appear'); + expect(() => getBuildWorkflow()).toThrow( + 'Middleware "prebuild-only-mw" of type "command" cannot use phase(s): pre-build', + ); }); it('filters out middleware whose trigger provider does not match providerStrategy', () => { diff --git a/plugins/orchestrator/src/plugin-lifecycle.ts b/plugins/orchestrator/src/plugin-lifecycle.ts index 691a2fc1..0e6b03a4 100644 --- a/plugins/orchestrator/src/plugin-lifecycle.ts +++ b/plugins/orchestrator/src/plugin-lifecycle.ts @@ -590,6 +590,11 @@ export function createPlugin(): OrchestratorPlugin { coreParams.branch || '', ) || ''; + // Sweep orphaned background-save locks (e.g. left by a killed process) + // before restoring, so a stale lock under a cache key this run doesn't + // touch isn't left to linger indefinitely. + LocalCacheService.sweepStaleLocks(cacheRoot); + localCacheState = { cacheRoot, cacheKey }; if (config.localCacheLfs) { @@ -834,6 +839,18 @@ export function createPlugin(): OrchestratorPlugin { ChildWorkspaceService.saveWorkspace(projectFullPath, childWorkspaceConfig); core.info(`Child workspace "${config.childWorkspaceName}" saved to cache`); + + // Age-based sweep for stale cached child workspaces. Reuses cacheRetentionDays + // (already used to gate LocalCacheService.garbageCollect above) rather than + // introducing a separate retention setting -- a cached child workspace and a + // local Library cache entry are the same kind of disk-space liability. + const childWorkspaceRetentionDays = Number(coreParams.cacheRetentionDays) || 0; + if (childWorkspaceRetentionDays > 0) { + ChildWorkspaceService.cleanStaleWorkspaces( + childWorkspaceConfig.parentCacheRoot, + childWorkspaceRetentionDays, + ); + } } // ── Sync revert ──────────────────────────────────────────── diff --git a/plugins/unity/dist/unity-builder/index.js b/plugins/unity/dist/unity-builder/index.js index 468805ba..61bc9d16 100644 --- a/plugins/unity/dist/unity-builder/index.js +++ b/plugins/unity/dist/unity-builder/index.js @@ -62,12 +62,12 @@ async function runMain() { ? await runLocalBuild(buildParameters, baseImage, workspace, actionFolder, plugin) : result.exitCode; } - else if (buildParameters.providerStrategy === 'local') { + else if (buildParameters.providerStrategy === "local") { exitCode = await runLocalBuild(buildParameters, baseImage, workspace, actionFolder, plugin); } else { throw new Error(`Provider strategy "${buildParameters.providerStrategy}" requires @game-ci/orchestrator. ` + - 'Install it via the game-ci/orchestrator action, or use providerStrategy=local.'); + "Install it via the game-ci/orchestrator action, or use providerStrategy=local."); } // Set core outputs await model_1.Output.setBuildVersion(buildParameters.buildVersion); @@ -84,16 +84,47 @@ async function runMain() { } } async function runLocalBuild(buildParameters, baseImage, workspace, actionFolder, plugin) { - await plugin?.beforeLocalBuild(workspace); - await platform_setup_1.default.setup(buildParameters, actionFolder); - const exitCode = process.platform === 'darwin' - ? await mac_builder_1.default.run(actionFolder) - : await model_1.Docker.run(baseImage.toString(), { - workspace, - actionFolder, - ...buildParameters, - }); - await plugin?.afterLocalBuild(workspace, exitCode); + // beforeLocalBuild() may have restored the local cache via a filesystem MOVE + // (localCacheMode=move-directory), which removes it from the cache root rather + // than copying it. afterLocalBuild() must always run to move the cache back, + // even when setup/build throws instead of returning an exit code -- otherwise + // the cache is lost with no surviving copy anywhere. See plugins/orchestrator + // LocalCacheService / ChildWorkspaceService for the move-based cache services. + let exitCode = -1; + let buildError; + try { + await plugin?.beforeLocalBuild(workspace); + await platform_setup_1.default.setup(buildParameters, actionFolder); + exitCode = + process.platform === "darwin" + ? await mac_builder_1.default.run(actionFolder) + : await model_1.Docker.run(baseImage.toString(), { + workspace, + actionFolder, + ...buildParameters, + }); + } + catch (error) { + buildError = error; + throw error; + } + finally { + try { + await plugin?.afterLocalBuild(workspace, exitCode); + } + catch (afterBuildError) { + if (buildError) { + // Preserve the primary setup/build failure while still surfacing the + // independent cleanup failure in the log. + core.warning(`afterLocalBuild failed: ${afterBuildError.message ?? afterBuildError}`); + } + else { + // A successful build with a failed move-directory save-back can leave + // the cache without a durable copy. Treat that as a real failure. + throw afterBuildError; + } + } + } return exitCode; } // Only auto-run when executed directly (subprocess/script invocation), not diff --git a/plugins/unity/src/unity-builder/index-plugin-features.test.ts b/plugins/unity/src/unity-builder/index-plugin-features.test.ts index ccd3dbd9..db6e4b7f 100644 --- a/plugins/unity/src/unity-builder/index-plugin-features.test.ts +++ b/plugins/unity/src/unity-builder/index-plugin-features.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach, vi, type Mock } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi, type Mock } from "vitest"; /** * Integration wiring tests for the plugin lifecycle in index.ts * @@ -10,8 +10,8 @@ import { describe, it, expect, beforeEach, afterEach, vi, type Mock } from 'vite * - When providerStrategy is non-local without a plugin, an error is thrown */ -import { BuildParameters, Docker } from './model'; -import * as core from '@actions/core'; +import { BuildParameters, Docker } from "./model"; +import * as core from "@actions/core"; // --------------------------------------------------------------------------- // Mock plugin @@ -34,16 +34,16 @@ const { mockPlugin, mockLoadPlugin } = vi.hoisted(() => { }; }); -vi.mock('./model/plugin', () => ({ +vi.mock("./model/plugin", () => ({ loadPlugin: mockLoadPlugin, })); -vi.mock('@actions/core'); -vi.mock('./model', () => ({ +vi.mock("@actions/core"); +vi.mock("./model", () => ({ Action: { checkCompatibility: vi.fn(), - workspace: '/workspace', - actionFolder: '/action', + workspace: "/workspace", + actionFolder: "/action", }, BuildParameters: { create: vi.fn(), @@ -57,26 +57,26 @@ vi.mock('./model', () => ({ // vitest 4 requires constructor mocks to use regular `function` (or // `class`); arrow fns aren't valid constructors. ImageTag: vi.fn(function () { - return { toString: () => 'mock-image:latest' }; + return { toString: () => "mock-image:latest" }; }), Output: { - setBuildVersion: vi.fn().mockResolvedValue(''), - setAndroidVersionCode: vi.fn().mockResolvedValue(''), - setEngineExitCode: vi.fn().mockResolvedValue(''), + setBuildVersion: vi.fn().mockResolvedValue(""), + setAndroidVersionCode: vi.fn().mockResolvedValue(""), + setEngineExitCode: vi.fn().mockResolvedValue(""), }, })); -vi.mock('./model/mac-builder', () => ({ +vi.mock("./model/mac-builder", () => ({ __esModule: true, default: { run: vi.fn().mockResolvedValue(0), }, })); -vi.mock('./model/platform-setup', () => ({ +vi.mock("./model/platform-setup", () => ({ __esModule: true, default: { - setup: vi.fn().mockResolvedValue(''), + setup: vi.fn().mockResolvedValue(""), }, })); @@ -84,14 +84,14 @@ const mockedBuildParametersCreate = BuildParameters.create as Mock; function createMockBuildParameters(overrides: Record = {}) { return { - providerStrategy: 'local', - targetPlatform: 'StandaloneLinux64', - editorVersion: '2021.3.1f1', - buildVersion: '1.0.0', - androidVersionCode: '1', - projectPath: '.', - branch: 'main', - runnerTempPath: '/tmp', + providerStrategy: "local", + targetPlatform: "StandaloneLinux64", + editorVersion: "2021.3.1f1", + buildVersion: "1.0.0", + androidVersionCode: "1", + projectPath: ".", + branch: "main", + runnerTempPath: "/tmp", ...overrides, }; } @@ -103,7 +103,7 @@ async function runIndex(overrides: Record = {}): Promise { // top-level execution + jest's `vi.isolateModules`, but vitest 4 dropped // that API). Calling the exported function directly is cleaner than // round-tripping through dynamic imports. - const { runMain } = await import('./index'); + const { runMain } = await import("./index"); await runMain(); } @@ -111,14 +111,14 @@ async function runIndex(overrides: Record = {}): Promise { // Tests // --------------------------------------------------------------------------- -describe('index.ts plugin lifecycle wiring', () => { +describe("index.ts plugin lifecycle wiring", () => { const originalPlatform = process.platform; const originalEnvironment = { ...process.env }; beforeEach(() => { vi.clearAllMocks(); - process.env.GITHUB_WORKSPACE = '/workspace'; - Object.defineProperty(process, 'platform', { value: 'linux' }); + process.env.GITHUB_WORKSPACE = "/workspace"; + Object.defineProperty(process, "platform", { value: "linux" }); // Reset plugin to default behavior mockPlugin.canHandleBuild.mockReturnValue(false); @@ -127,7 +127,7 @@ describe('index.ts plugin lifecycle wiring', () => { }); afterEach(() => { - Object.defineProperty(process, 'platform', { value: originalPlatform }); + Object.defineProperty(process, "platform", { value: originalPlatform }); process.env = { ...originalEnvironment }; }); @@ -135,72 +135,105 @@ describe('index.ts plugin lifecycle wiring', () => { // Local build with plugin // ----------------------------------------------------------------------- - describe('local build with plugin installed', () => { - it('should call lifecycle hooks in order: initialize -> beforeLocalBuild -> [build] -> afterLocalBuild -> handlePostBuild', async () => { + describe("local build with plugin installed", () => { + it("should call lifecycle hooks in order: initialize -> beforeLocalBuild -> [build] -> afterLocalBuild -> handlePostBuild", async () => { const callOrder: string[] = []; - mockPlugin.initialize.mockImplementation(async () => callOrder.push('initialize')); - mockPlugin.beforeLocalBuild.mockImplementation(async () => - callOrder.push('beforeLocalBuild'), - ); - mockPlugin.afterLocalBuild.mockImplementation(async () => callOrder.push('afterLocalBuild')); - mockPlugin.handlePostBuild.mockImplementation(async () => callOrder.push('handlePostBuild')); + mockPlugin.initialize.mockImplementation(async () => callOrder.push("initialize")); + mockPlugin.beforeLocalBuild.mockImplementation(async () => callOrder.push("beforeLocalBuild")); + mockPlugin.afterLocalBuild.mockImplementation(async () => callOrder.push("afterLocalBuild")); + mockPlugin.handlePostBuild.mockImplementation(async () => callOrder.push("handlePostBuild")); await runIndex(); - expect(callOrder).toEqual([ - 'initialize', - 'beforeLocalBuild', - 'afterLocalBuild', - 'handlePostBuild', - ]); + expect(callOrder).toEqual(["initialize", "beforeLocalBuild", "afterLocalBuild", "handlePostBuild"]); }); - it('should pass buildParameters and workspace to initialize', async () => { - await runIndex({ targetPlatform: 'WebGL' }); + it("should pass buildParameters and workspace to initialize", async () => { + await runIndex({ targetPlatform: "WebGL" }); expect(mockPlugin.initialize).toHaveBeenCalledWith( - expect.objectContaining({ targetPlatform: 'WebGL' }), - '/workspace', + expect.objectContaining({ targetPlatform: "WebGL" }), + "/workspace", ); }); - it('should pass workspace to beforeLocalBuild', async () => { + it("should pass workspace to beforeLocalBuild", async () => { await runIndex(); - expect(mockPlugin.beforeLocalBuild).toHaveBeenCalledWith('/workspace'); + expect(mockPlugin.beforeLocalBuild).toHaveBeenCalledWith("/workspace"); }); - it('should pass workspace and exit code to afterLocalBuild', async () => { + it("should pass workspace and exit code to afterLocalBuild", async () => { await runIndex(); - expect(mockPlugin.afterLocalBuild).toHaveBeenCalledWith('/workspace', 0); + expect(mockPlugin.afterLocalBuild).toHaveBeenCalledWith("/workspace", 0); }); - it('should pass exit code to handlePostBuild', async () => { + it("should pass exit code to handlePostBuild", async () => { await runIndex(); expect(mockPlugin.handlePostBuild).toHaveBeenCalledWith(0); }); + + it("should still call afterLocalBuild when the build throws, so a move-directory cache save-back is not lost", async () => { + (Docker.run as Mock).mockRejectedValueOnce(new Error("container crashed")); + + await runIndex(); + + // beforeLocalBuild may have MOVEd the cache into the workspace; afterLocalBuild + // must always run to move it back, even when the build itself throws instead + // of returning an exit code. + expect(mockPlugin.afterLocalBuild).toHaveBeenCalledWith("/workspace", -1); + expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining("container crashed")); + }); + + it("should not let an afterLocalBuild failure mask the original build error", async () => { + (Docker.run as Mock).mockRejectedValueOnce(new Error("container crashed")); + mockPlugin.afterLocalBuild.mockRejectedValueOnce(new Error("cache save-back failed")); + + await runIndex(); + + expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining("container crashed")); + expect(core.warning).toHaveBeenCalledWith(expect.stringContaining("cache save-back failed")); + }); + + it("should fail a successful build when afterLocalBuild fails", async () => { + mockPlugin.afterLocalBuild.mockRejectedValueOnce(new Error("cache save-back failed")); + + await runIndex(); + + expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining("cache save-back failed")); + expect(core.warning).not.toHaveBeenCalledWith(expect.stringContaining("cache save-back failed")); + }); + + it("should still call afterLocalBuild when beforeLocalBuild throws after a partial restore", async () => { + mockPlugin.beforeLocalBuild.mockRejectedValueOnce(new Error("restore failed")); + + await runIndex(); + + expect(mockPlugin.afterLocalBuild).toHaveBeenCalledWith("/workspace", -1); + expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining("restore failed")); + }); }); // ----------------------------------------------------------------------- // Plugin handles build entirely // ----------------------------------------------------------------------- - describe('plugin handles build (canHandleBuild = true)', () => { - it('should call handleBuild instead of Docker.run', async () => { + describe("plugin handles build (canHandleBuild = true)", () => { + it("should call handleBuild instead of Docker.run", async () => { mockPlugin.canHandleBuild.mockReturnValue(true); mockPlugin.handleBuild.mockResolvedValue({ exitCode: 0 }); await runIndex(); - expect(mockPlugin.handleBuild).toHaveBeenCalledWith('mock-image:latest'); + expect(mockPlugin.handleBuild).toHaveBeenCalledWith("mock-image:latest"); expect(Docker.run).not.toHaveBeenCalled(); expect(mockPlugin.beforeLocalBuild).not.toHaveBeenCalled(); expect(mockPlugin.afterLocalBuild).not.toHaveBeenCalled(); }); - it('should still call handlePostBuild after handleBuild', async () => { + it("should still call handlePostBuild after handleBuild", async () => { mockPlugin.canHandleBuild.mockReturnValue(true); mockPlugin.handleBuild.mockResolvedValue({ exitCode: 0 }); @@ -214,8 +247,8 @@ describe('index.ts plugin lifecycle wiring', () => { // Fallback to local // ----------------------------------------------------------------------- - describe('fallback to local build', () => { - it('should do a local build when handleBuild returns fallbackToLocal', async () => { + describe("fallback to local build", () => { + it("should do a local build when handleBuild returns fallbackToLocal", async () => { mockPlugin.canHandleBuild.mockReturnValue(true); mockPlugin.handleBuild.mockResolvedValue({ exitCode: -1, fallbackToLocal: true }); @@ -232,23 +265,21 @@ describe('index.ts plugin lifecycle wiring', () => { // No plugin installed // ----------------------------------------------------------------------- - describe('no plugin installed', () => { - it('should build locally without errors when providerStrategy is local', async () => { + describe("no plugin installed", () => { + it("should build locally without errors when providerStrategy is local", async () => { mockLoadPlugin.mockResolvedValue(undefined); - await runIndex({ providerStrategy: 'local' }); + await runIndex({ providerStrategy: "local" }); expect(Docker.run).toHaveBeenCalled(); }); - it('should error when providerStrategy is non-local and no plugin', async () => { + it("should error when providerStrategy is non-local and no plugin", async () => { mockLoadPlugin.mockResolvedValue(undefined); - await runIndex({ providerStrategy: 'aws' }); + await runIndex({ providerStrategy: "aws" }); - expect(core.setFailed).toHaveBeenCalledWith( - expect.stringContaining('requires @game-ci/orchestrator'), - ); + expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining("requires @game-ci/orchestrator")); }); }); @@ -256,17 +287,15 @@ describe('index.ts plugin lifecycle wiring', () => { // canHandleBuild = false with non-local provider // ----------------------------------------------------------------------- - describe('plugin installed but canHandleBuild returns false with non-local provider', () => { - it('should error when providerStrategy is non-local', async () => { + describe("plugin installed but canHandleBuild returns false with non-local provider", () => { + it("should error when providerStrategy is non-local", async () => { mockPlugin.canHandleBuild.mockReturnValue(false); - await runIndex({ providerStrategy: 'aws' }); + await runIndex({ providerStrategy: "aws" }); // The plugin is initialized but says it can't handle the build, // and providerStrategy is not local, so it falls to the error case - expect(core.setFailed).toHaveBeenCalledWith( - expect.stringContaining('requires @game-ci/orchestrator'), - ); + expect(core.setFailed).toHaveBeenCalledWith(expect.stringContaining("requires @game-ci/orchestrator")); }); }); }); diff --git a/plugins/unity/src/unity-builder/index.ts b/plugins/unity/src/unity-builder/index.ts index 3f8bfc99..dac2f2e6 100644 --- a/plugins/unity/src/unity-builder/index.ts +++ b/plugins/unity/src/unity-builder/index.ts @@ -1,8 +1,8 @@ -import * as core from '@actions/core'; -import { Action, BuildParameters, Cache, Docker, ImageTag, Output } from './model'; -import MacBuilder from './model/mac-builder'; -import PlatformSetup from './model/platform-setup'; -import { Plugin, loadPlugin } from './model/plugin'; +import * as core from "@actions/core"; +import { Action, BuildParameters, Cache, Docker, ImageTag, Output } from "./model"; +import MacBuilder from "./model/mac-builder"; +import PlatformSetup from "./model/platform-setup"; +import { Plugin, loadPlugin } from "./model/plugin"; // Exported so tests can drive the lifecycle directly without depending on // vitest's module re-loading (which changed in vitest 4). @@ -28,12 +28,12 @@ export async function runMain() { exitCode = result.fallbackToLocal ? await runLocalBuild(buildParameters, baseImage, workspace, actionFolder, plugin) : result.exitCode; - } else if (buildParameters.providerStrategy === 'local') { + } else if (buildParameters.providerStrategy === "local") { exitCode = await runLocalBuild(buildParameters, baseImage, workspace, actionFolder, plugin); } else { throw new Error( `Provider strategy "${buildParameters.providerStrategy}" requires @game-ci/orchestrator. ` + - 'Install it via the game-ci/orchestrator action, or use providerStrategy=local.', + "Install it via the game-ci/orchestrator action, or use providerStrategy=local.", ); } @@ -60,19 +60,43 @@ async function runLocalBuild( actionFolder: string, plugin?: Plugin, ): Promise { - await plugin?.beforeLocalBuild(workspace); - - await PlatformSetup.setup(buildParameters, actionFolder); - const exitCode = - process.platform === 'darwin' - ? await MacBuilder.run(actionFolder) - : await Docker.run(baseImage.toString(), { - workspace, - actionFolder, - ...buildParameters, - }); - - await plugin?.afterLocalBuild(workspace, exitCode); + // beforeLocalBuild() may have restored the local cache via a filesystem MOVE + // (localCacheMode=move-directory), which removes it from the cache root rather + // than copying it. afterLocalBuild() must always run to move the cache back, + // even when setup/build throws instead of returning an exit code -- otherwise + // the cache is lost with no surviving copy anywhere. See plugins/orchestrator + // LocalCacheService / ChildWorkspaceService for the move-based cache services. + let exitCode = -1; + let buildError: unknown; + try { + await plugin?.beforeLocalBuild(workspace); + await PlatformSetup.setup(buildParameters, actionFolder); + exitCode = + process.platform === "darwin" + ? await MacBuilder.run(actionFolder) + : await Docker.run(baseImage.toString(), { + workspace, + actionFolder, + ...buildParameters, + }); + } catch (error) { + buildError = error; + throw error; + } finally { + try { + await plugin?.afterLocalBuild(workspace, exitCode); + } catch (afterBuildError) { + if (buildError) { + // Preserve the primary setup/build failure while still surfacing the + // independent cleanup failure in the log. + core.warning(`afterLocalBuild failed: ${(afterBuildError as Error).message ?? afterBuildError}`); + } else { + // A successful build with a failed move-directory save-back can leave + // the cache without a durable copy. Treat that as a real failure. + throw afterBuildError; + } + } + } return exitCode; }