fix: cache lifecycle can lose or leak state on build failure (#94)

* fix: cache lifecycle can lose or leak state on build failure

Four reliability gaps in the move-based caching and retained-workspace
locking, all variants of the same root cause: a cleanup step that only
runs on the success path, or a staleness check with no liveness signal.

- runLocalBuild() called afterLocalBuild() (which moves a
  localCacheMode=move-directory cache back to the cache root) only when
  setup/build returned normally. A thrown error skipped it entirely, so
  a cache already moved out of the cache root by beforeLocalBuild() was
  lost with no surviving copy anywhere. Now wrapped in try/finally, with
  the afterLocalBuild call itself guarded so a cache-save failure can't
  mask the original build error.

- Orchestrator.runWithProvider() released a retained-workspace lock only
  in the success path. IsWorkspaceLocked() has no TTL, so a build that
  threw before reaching that line leaked the lock permanently, shrinking
  the retained-workspace pool by one on every crash. Now also released in
  the failure path (idempotent, so this is safe regardless of whether the
  success path already ran).

- The background-save lock (.game-ci-cache-save.lock) was only checked
  reactively, when a later save/restore happened to target the same
  cache key. A process killed mid-save left its lock in place
  indefinitely for any cache key a run doesn't revisit.
  LocalCacheService.sweepStaleLocks() proactively sweeps every cache-key
  directory under the cache root at the start of a build, treating a
  lock as stale when its recorded PID is no longer alive.

- ChildWorkspaceService.cleanStaleWorkspaces() existed and was tested but
  was never wired into plugin-lifecycle.ts, so cached child workspaces
  accumulated forever. Wired into afterLocalBuild, reusing the existing
  cacheRetentionDays setting rather than adding a new one.

* chore: rebuild plugins/unity dist to include the runLocalBuild try/finally fix

CI's dist-drift check caught this - src/unity-builder/index.ts was updated
but dist/unity-builder/index.js wasn't rebuilt to match.

* fix: harden cache cleanup and middleware validation

Author: frostebite

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,

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);

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',

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');
+    });
   });
 });

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.