Skip to content

Commit fe99abe

Browse files
committed
feat: wire UnityRetryService into the local provider's build retry (opt-in)
UnityBuildDiagnosticsService (classifier) + UnityRecoveryService (budget- gated decision) + UnityRetryService (the retry-loop driver, including real Library backup/nuke recovery actions) were all fully built with zero live callers anywhere in the pipeline - no lifecycle point existed between a Unity build's exit code and orchestrator's success/failure decision, so every non-zero exit was treated identically regardless of whether it was a known-transient condition (licensing race, stale ScriptAssemblies, etc.) or a real failure. OrchestratorSystem.Run gets one purely-additive optional trailing parameter, onExitCode?: (code: number) => void, invoked with the real exit code right before the promise settles either way. Every existing caller (212 call sites across 28 files, confirmed via grep) that doesn't pass it gets identical behavior - same thrown string on failure, same resolved output on success. LocalOrchestrator.runTaskInWorkflow wraps this in a retry loop entirely encapsulated inside the provider when --enableBuildRetry is set (default false - a meaningful behavior change like automatic Library-nuking recovery shouldn't silently activate for existing users). The ProviderInterface contract and standardBuildAutomation's call site are completely untouched - no ripple into aws/k8s/gcp-cloud-run/azure-aci/ github-actions/gitlab-ci/remote-powershell/ansible/cli providers. logText for classification reads the real Unity Editor log content ($LOG_FILE, exported by BuildAutomationWorkflow.BuildWorkflow for the bare-local path and populated by remote-cli-log-stream from unity-editor's own -logfile output) rather than just the wrapper's stdout capture, since UnityBuildDiagnosticsService's crash/license/compile patterns need to match against genuine Editor log content. Scoped to build only (not test), local/local-system provider only, using UnityRecoveryService's existing default budgets (no new budget-config CLI surface added). Adds the previously-missing test coverage: unity-recovery-service.test.ts and unity-retry-service.test.ts (neither existed before this).
1 parent bc0ac81 commit fe99abe

8 files changed

Lines changed: 792 additions & 3 deletions

File tree

plugins/orchestrator/src/cli-plugin/build-parameters-adapter.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,7 @@ export function createBuildParametersFromCliOptions(options: Record<string, any>
153153
// ── reliability ───────────────────────────────────────────────────
154154
bp.gitIntegrityCheck = options.gitIntegrityCheck === true || options.gitIntegrityCheck === 'true';
155155
bp.gitAutoRecover = options.gitAutoRecover === true || options.gitAutoRecover === 'true';
156+
bp.enableBuildRetry = options.enableBuildRetry === true || options.enableBuildRetry === 'true';
156157
bp.cleanReservedFilenames =
157158
options.cleanReservedFilenames === true || options.cleanReservedFilenames === 'true';
158159
bp.buildArchiveEnabled =

plugins/orchestrator/src/cli-plugin/orchestrator-options-plugin.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,16 @@ export function configureOrchestratorOptions(yargs: any): void {
346346
default: false,
347347
});
348348

349+
yargs.option('enableBuildRetry', {
350+
description:
351+
'Enable automatic classify/decide/retry recovery for failed Unity builds on the bare-host ' +
352+
'`local`/`local-system` provider strategy (UnityRetryService, budget-gated). Default off: a ' +
353+
'single failed attempt still throws exactly as before -- retry can back up or nuke the Library ' +
354+
'folder as a recovery action, which is a meaningful behavior change existing users must opt into.',
355+
type: 'boolean',
356+
default: false,
357+
});
358+
349359
yargs.option('skipInContainerClone', {
350360
description:
351361
'Skip the in-container git clone and reuse a pre-hydrated workspace bind-mounted by the caller. ' +

plugins/orchestrator/src/model/build-parameters.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,12 @@ class BuildParameters {
150150
// ── reliability ─────────────────────────────────────────────────────
151151
gitIntegrityCheck!: boolean;
152152
gitAutoRecover!: boolean;
153+
// Opt-in (default false) automatic classify -> decide -> retry loop for
154+
// failed Unity runs on the bare-host `local`/`local-system` provider's
155+
// build path only (see UnityRetryService). Off by default because retry
156+
// can nuke/backup the Library folder as a recovery action -- a meaningful
157+
// behavior change that must not silently activate for existing users.
158+
enableBuildRetry!: boolean;
153159
cleanReservedFilenames!: boolean;
154160
buildArchiveEnabled!: boolean;
155161
buildArchivePath!: string;
Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,204 @@
1+
import { describe, it, expect, beforeEach, afterEach, vi, type Mocked } from 'vitest';
2+
import fs from 'node:fs';
3+
import path from 'node:path';
4+
5+
vi.mock('node:fs');
6+
vi.mock('../../services/core/orchestrator-system', () => ({
7+
OrchestratorSystem: { Run: vi.fn() },
8+
}));
9+
vi.mock('../../services/reliability/unity-retry-service', () => ({
10+
UnityRetryService: { executeWithRetry: vi.fn() },
11+
}));
12+
vi.mock('../../services/reliability/unity-recovery-service', () => ({
13+
UnityRecoveryService: { createDefaultBudgets: vi.fn(() => ({ mocked: true })) },
14+
}));
15+
16+
import LocalOrchestrator from './index';
17+
import { OrchestratorSystem } from '../../services/core/orchestrator-system';
18+
import { UnityRetryService } from '../../services/reliability/unity-retry-service';
19+
import { UnityRecoveryService } from '../../services/reliability/unity-recovery-service';
20+
import Orchestrator from '../../orchestrator';
21+
import BuildParameters from '../../../build-parameters';
22+
23+
const mockFs = fs as Mocked<typeof fs>;
24+
const mockRun = OrchestratorSystem.Run as unknown as Mocked<typeof OrchestratorSystem.Run>;
25+
const mockExecuteWithRetry = UnityRetryService.executeWithRetry as unknown as Mocked<
26+
typeof UnityRetryService.executeWithRetry
27+
>;
28+
29+
/**
30+
* Integration coverage for the --enableBuildRetry wiring point: the
31+
* isBareLocalProvider path inside LocalOrchestrator.runTaskInWorkflow that
32+
* decides between "single attempt, throw on failure" (today's behavior,
33+
* default) and "delegate to UnityRetryService.executeWithRetry" (opt-in).
34+
* UnityRetryService's own retry/recovery algorithm is covered exhaustively
35+
* in unity-retry-service.test.ts -- this file only asserts the call pattern
36+
* difference at the provider boundary.
37+
*/
38+
describe('LocalOrchestrator.runTaskInWorkflow -- enableBuildRetry wiring', () => {
39+
const originalPlatform = process.platform;
40+
const provider = new LocalOrchestrator();
41+
42+
const runTask = () =>
43+
provider.runTaskInWorkflow('build-guid', 'unity-image', 'echo hi', '/mnt', '/mnt/', [], []);
44+
45+
beforeEach(() => {
46+
vi.clearAllMocks();
47+
Object.defineProperty(process, 'platform', { value: 'linux' });
48+
mockFs.readFileSync.mockImplementation(() => {
49+
throw new Error('ENOENT');
50+
});
51+
});
52+
53+
afterEach(() => {
54+
Object.defineProperty(process, 'platform', { value: originalPlatform });
55+
});
56+
57+
it('enableBuildRetry=false (default): calls OrchestratorSystem.Run directly once and never touches UnityRetryService', async () => {
58+
Orchestrator.buildParameters = { enableBuildRetry: false, projectPath: '.' } as BuildParameters;
59+
mockRun.mockResolvedValue('build output');
60+
61+
const result = await runTask();
62+
63+
expect(result).toBe('build output');
64+
expect(mockRun).toHaveBeenCalledTimes(1);
65+
expect(mockRun).toHaveBeenCalledWith('echo hi');
66+
expect(mockExecuteWithRetry).not.toHaveBeenCalled();
67+
});
68+
69+
it('enableBuildRetry left unset behaves identically to false (safe default for hosts not on this build parameters version)', async () => {
70+
Orchestrator.buildParameters = { projectPath: '.' } as BuildParameters;
71+
mockRun.mockResolvedValue('build output');
72+
73+
await runTask();
74+
75+
expect(mockRun).toHaveBeenCalledTimes(1);
76+
expect(mockExecuteWithRetry).not.toHaveBeenCalled();
77+
});
78+
79+
it('enableBuildRetry=false still throws exactly as before on failure -- zero behavior change for the common case', async () => {
80+
Orchestrator.buildParameters = { enableBuildRetry: false, projectPath: '.' } as BuildParameters;
81+
mockRun.mockRejectedValue('raw wrapper output');
82+
83+
await expect(runTask()).rejects.toBe('raw wrapper output');
84+
expect(mockExecuteWithRetry).not.toHaveBeenCalled();
85+
});
86+
87+
it('enableBuildRetry=true: delegates to UnityRetryService.executeWithRetry with UnityRecoveryService default budgets instead of calling OrchestratorSystem.Run directly', async () => {
88+
Orchestrator.buildParameters = {
89+
enableBuildRetry: true,
90+
projectPath: 'MyProject',
91+
} as BuildParameters;
92+
mockExecuteWithRetry.mockResolvedValue({
93+
succeeded: true,
94+
attempts: 2,
95+
lastDiagnostics: { failureCategory: 'SUCCESS' } as any,
96+
actionsPerformed: ['retry-licensing'],
97+
});
98+
99+
const result = await runTask();
100+
101+
expect(result).toBe('');
102+
expect(mockRun).not.toHaveBeenCalled();
103+
expect(mockExecuteWithRetry).toHaveBeenCalledTimes(1);
104+
expect(UnityRecoveryService.createDefaultBudgets).toHaveBeenCalledTimes(1);
105+
106+
const [projectPathArgument, runUnityArgument, optionsArgument] =
107+
mockExecuteWithRetry.mock.calls[0];
108+
expect(projectPathArgument).toBe(path.join(process.cwd(), 'MyProject'));
109+
expect(typeof runUnityArgument).toBe('function');
110+
expect(optionsArgument).toEqual({ budgets: { mocked: true } });
111+
});
112+
113+
it('enableBuildRetry=true and the retry loop ultimately fails: throws a descriptive Error (not the raw output string)', async () => {
114+
Orchestrator.buildParameters = { enableBuildRetry: true, projectPath: '.' } as BuildParameters;
115+
mockExecuteWithRetry.mockResolvedValue({
116+
succeeded: false,
117+
attempts: 6,
118+
lastDiagnostics: {
119+
failureCategory: 'CRASH',
120+
failureSummary: { remediationHint: 'Clear ScriptAssemblies or nuke Library.' },
121+
} as any,
122+
actionsPerformed: ['nuke-library'],
123+
});
124+
125+
await expect(runTask()).rejects.toThrow(
126+
/Unity build failed after 6 attempt\(s\).*\[CRASH\].*nuke-library/s,
127+
);
128+
});
129+
130+
it('the runUnity callback passed to UnityRetryService runs the command with suppressError=true, captures the real exit code, and prefers the on-disk Editor log over wrapper stdout', async () => {
131+
Orchestrator.buildParameters = { enableBuildRetry: true, projectPath: '.' } as BuildParameters;
132+
133+
let capturedRunUnity:
134+
| (() => Promise<{
135+
exitCode: number;
136+
logText: string;
137+
runtimeSeconds: number;
138+
}>)
139+
| undefined;
140+
141+
mockExecuteWithRetry.mockImplementation(async (_projectPath, runUnity) => {
142+
capturedRunUnity = runUnity;
143+
144+
return {
145+
succeeded: true,
146+
attempts: 1,
147+
lastDiagnostics: { failureCategory: 'SUCCESS' } as any,
148+
actionsPerformed: [],
149+
};
150+
});
151+
mockRun.mockImplementation(
152+
async (
153+
_command: string,
154+
_suppressError?: boolean,
155+
_suppressLogs?: boolean,
156+
_outputCallback?: (output: string) => void,
157+
onExitCode?: (code: number) => void,
158+
) => {
159+
onExitCode?.(1);
160+
161+
return 'wrapper stdout only';
162+
},
163+
);
164+
mockFs.readFileSync.mockReturnValue('genuine Unity Editor log content' as any);
165+
166+
await runTask();
167+
168+
expect(capturedRunUnity).toBeDefined();
169+
const attemptResult = await capturedRunUnity!();
170+
171+
expect(mockRun).toHaveBeenCalledWith('echo hi', true, false, undefined, expect.any(Function));
172+
expect(attemptResult.exitCode).toBe(1);
173+
expect(attemptResult.logText).toBe('genuine Unity Editor log content');
174+
expect(mockFs.readFileSync).toHaveBeenCalledWith(
175+
path.join(process.cwd(), 'temp', 'job-log.txt'),
176+
'utf8',
177+
);
178+
});
179+
180+
it('falls back to wrapper stdout as logText when the Editor log file cannot be read', async () => {
181+
Orchestrator.buildParameters = { enableBuildRetry: true, projectPath: '.' } as BuildParameters;
182+
183+
let capturedRunUnity: (() => Promise<{ exitCode: number; logText: string }>) | undefined;
184+
mockExecuteWithRetry.mockImplementation(async (_projectPath, runUnity) => {
185+
capturedRunUnity = runUnity as any;
186+
187+
return {
188+
succeeded: true,
189+
attempts: 1,
190+
lastDiagnostics: { failureCategory: 'SUCCESS' } as any,
191+
actionsPerformed: [],
192+
};
193+
});
194+
mockRun.mockResolvedValue('wrapper stdout only');
195+
mockFs.readFileSync.mockImplementation(() => {
196+
throw new Error('ENOENT: no such file');
197+
});
198+
199+
await runTask();
200+
const attemptResult = await capturedRunUnity!();
201+
202+
expect(attemptResult.logText).toBe('wrapper stdout only');
203+
});
204+
});

plugins/orchestrator/src/model/orchestrator/providers/local/index.ts

Lines changed: 91 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import fs from 'node:fs';
2+
import path from 'node:path';
13
import BuildParameters from '../../../build-parameters';
24
import { OrchestratorSystem } from '../../services/core/orchestrator-system';
35
import OrchestratorEnvironmentVariable from '../../options/orchestrator-environment-variable';
@@ -7,6 +9,9 @@ import OrchestratorSecret from '../../options/orchestrator-secret';
79
import { ProviderResource } from '../provider-resource';
810
import { ProviderWorkflow } from '../provider-workflow';
911
import { quote } from 'shell-quote';
12+
import Orchestrator from '../../orchestrator';
13+
import { UnityRetryService } from '../../services/reliability/unity-retry-service';
14+
import { UnityRecoveryService } from '../../services/reliability/unity-recovery-service';
1015

1116
class LocalOrchestrator implements ProviderInterface {
1217
listResources(): Promise<ProviderResource[]> {
@@ -89,6 +94,7 @@ class LocalOrchestrator implements ProviderInterface {
8994
OrchestratorLogger.log(commands);
9095

9196
// On Windows, many built-in hooks use POSIX shell syntax. Execute via bash if available.
97+
let command = commands;
9298
if (process.platform === 'win32') {
9399
const inline = commands
94100
.replace(/\r/g, '')
@@ -97,12 +103,94 @@ class LocalOrchestrator implements ProviderInterface {
97103
.join(' ; ');
98104

99105
// Use shell-quote to properly escape the command string, preventing command injection
100-
const bashWrapped = `bash -lc ${quote([inline])}`;
106+
command = `bash -lc ${quote([inline])}`;
107+
}
101108

102-
return await OrchestratorSystem.Run(bashWrapped);
109+
// Opt-in (--enableBuildRetry, default off): wrap the whole invocation in
110+
// UnityRetryService's classify -> decide -> retry loop, entirely
111+
// encapsulated inside this provider. Every provider strategy other than
112+
// local/local-system goes through their own OrchestratorSystem.Run calls
113+
// untouched, and the ProviderInterface contract is unchanged -- callers
114+
// above (e.g. BuildAutomationWorkflow) keep awaiting a plain
115+
// Promise<string>, same as before this feature existed.
116+
if (Orchestrator.buildParameters?.enableBuildRetry) {
117+
return await LocalOrchestrator.runWithRetry(command);
103118
}
104119

105-
return await OrchestratorSystem.Run(commands);
120+
return await OrchestratorSystem.Run(command);
121+
}
122+
123+
/**
124+
* Runs `command` under UnityRetryService.executeWithRetry: on a non-zero
125+
* exit, classify the failure (UnityBuildDiagnosticsService), decide on a
126+
* budget-gated recovery action (UnityRecoveryService -- may back up/nuke
127+
* the Library folder or clear subfolders), perform it, then retry the
128+
* same command again, up to UnityRetryService's own attempt cap.
129+
*
130+
* `logText` is read from the real Unity Editor log rather than only the
131+
* wrapper's captured stdout/stderr: dist/platforms/ubuntu/steps/runsteps.sh
132+
* (via build.sh/activate.sh) invokes `unity-editor -logfile /dev/stdout`,
133+
* and BuildAutomationWorkflow.BuildWorkflow pipes that whole pipeline
134+
* through `node <builder> -m remote-cli-log-stream --logFile "$LOG_FILE"`,
135+
* which appends every line to $LOG_FILE. For the bare-local provider,
136+
* $LOG_FILE is exported as `$(pwd)/temp/job-log.txt` (see
137+
* BuildAutomationWorkflow.BuildWorkflow), so that file accumulates the
138+
* genuine Editor log content that UnityBuildDiagnosticsService's crash/
139+
* license/compile patterns are meant to match against -- not just whatever
140+
* subset `command`'s own stdout capture happens to contain.
141+
*/
142+
private static async runWithRetry(command: string): Promise<string> {
143+
const bp = Orchestrator.buildParameters;
144+
const projectPath = path.isAbsolute(bp.projectPath || '')
145+
? bp.projectPath
146+
: path.join(process.cwd(), bp.projectPath || '.');
147+
const logFilePath = path.join(process.cwd(), 'temp', 'job-log.txt');
148+
149+
let lastOutput = '';
150+
151+
const result = await UnityRetryService.executeWithRetry(
152+
projectPath,
153+
async () => {
154+
const startedAtMs = Date.now();
155+
let exitCode = 0;
156+
157+
// suppressError=true: on a non-zero exit, OrchestratorSystem.Run
158+
// resolves with the captured output instead of throwing, so the
159+
// retry loop can inspect the result and decide whether to retry --
160+
// exactly mirroring how the non-retry path above lets a throw
161+
// propagate on the single, non-retried attempt.
162+
const stdout = await OrchestratorSystem.Run(command, true, false, undefined, (code) => {
163+
exitCode = code;
164+
});
165+
lastOutput = stdout;
166+
167+
const runtimeSeconds = (Date.now() - startedAtMs) / 1000;
168+
const logText = LocalOrchestrator.readEditorLog(logFilePath) || stdout;
169+
170+
return { exitCode, logText, runtimeSeconds };
171+
},
172+
{ budgets: UnityRecoveryService.createDefaultBudgets() },
173+
);
174+
175+
if (!result.succeeded) {
176+
const category = result.lastDiagnostics?.failureCategory ?? 'UNKNOWN';
177+
const hint = result.lastDiagnostics?.failureSummary?.remediationHint ?? '';
178+
throw new Error(
179+
`Unity build failed after ${result.attempts} attempt(s) via UnityRetryService ` +
180+
`[${category}]${hint ? `: ${hint}` : ''}. Actions performed: ` +
181+
`${result.actionsPerformed.join(', ') || 'none'}.\n${lastOutput}`,
182+
);
183+
}
184+
185+
return lastOutput;
186+
}
187+
188+
private static readEditorLog(logFilePath: string): string {
189+
try {
190+
return fs.readFileSync(logFilePath, 'utf8');
191+
} catch {
192+
return '';
193+
}
106194
}
107195
}
108196
export default LocalOrchestrator;

plugins/orchestrator/src/model/orchestrator/services/core/orchestrator-system.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,13 @@ export class OrchestratorSystem {
2222
suppressLogs = false,
2323
// eslint-disable-next-line no-unused-vars
2424
outputCallback?: (output: string) => void,
25+
// Invoked with the real (non-normalized) child process exit code right
26+
// before the promise settles -- on both the success and failure paths.
27+
// Purely additive: existing callers that don't pass this get identical
28+
// behavior (same thrown string, same resolved output) to before this
29+
// parameter was added.
30+
// eslint-disable-next-line no-unused-vars
31+
onExitCode?: (code: number) => void,
2532
) {
2633
for (const element of command.split(`\n`)) {
2734
if (!suppressLogs) {
@@ -53,6 +60,7 @@ export class OrchestratorSystem {
5360
if (!suppressLogs) {
5461
RemoteClientLogger.log(`[${code}]`);
5562
}
63+
onExitCode?.(code ?? -1);
5664
if (code !== 0 && !suppressError) {
5765
throwError(output);
5866
}

0 commit comments

Comments
 (0)