Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 2 additions & 44 deletions backend/src/services/dockerRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,33 +89,6 @@ export async function pullRunnerImage(
): Promise<string> {
const d = initDocker();
const platform = `linux/${architecture}`;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exported functions now unused in production code

Low Severity

normalizeImageArchitecture and assertImageArchitecture are still exported but their only production callers were inside pullRunnerImage, which this commit removed. They are now dead code in production, used only by dockerRunner.test.ts.

Fix in Cursor Fix in Web

const splitImageRef = (ref: string): { repo: string; tag: string } => {
const lastColon = ref.lastIndexOf(':');
const lastSlash = ref.lastIndexOf('/');
if (lastColon > lastSlash) {
return { repo: ref.slice(0, lastColon), tag: ref.slice(lastColon + 1) };
}
return { repo: ref, tag: 'latest' };
};

const { repo, tag: originalTag } = splitImageRef(RUNNER_IMAGE);
const archTag = `${originalTag}-${architecture}`;
const platformTag = `${repo}:${archTag}`;

// Check if we already have the platform-specific tag with correct architecture
try {
const existingImage = await d.getImage(platformTag).inspect();
const imageArch = normalizeImageArchitecture(existingImage.Architecture);

if (imageArch === architecture) {
console.log(`Image ${platformTag} already exists with correct architecture (${existingImage.Architecture})`);
return platformTag;
}
console.log(`Image ${platformTag} exists but has wrong architecture (${existingImage.Architecture}), re-pulling...`);
} catch {
// Image doesn't exist, need to pull
}

console.log(`Pulling image ${RUNNER_IMAGE} for ${platform}...`);

Expand All @@ -140,23 +113,8 @@ export async function pullRunnerImage(
});
});
});

// Verify the pulled image has the correct architecture
const pulledImage = await d.getImage(RUNNER_IMAGE).inspect();
console.log(`Pulled image architecture: ${pulledImage.Architecture}`);
assertImageArchitecture(RUNNER_IMAGE, pulledImage.Architecture, architecture);

// Tag the pulled image with architecture-specific tag
console.log(`Tagging image as ${platformTag}...`);
const image = d.getImage(RUNNER_IMAGE);
await image.tag({ repo, tag: archTag });

// Verify the tagged image
const taggedImage = await d.getImage(platformTag).inspect();
console.log(`Tagged image ${platformTag} architecture: ${taggedImage.Architecture}`);
assertImageArchitecture(platformTag, taggedImage.Architecture, architecture);

return platformTag;

return RUNNER_IMAGE;
}

/**
Expand Down
9 changes: 9 additions & 0 deletions backend/src/services/reconciler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ const getAllEnabledPools = db.prepare('SELECT * FROM runner_pools WHERE enabled
const deleteRunner = db.prepare('DELETE FROM runners WHERE id = ?');
const updateRunnerStatus = db.prepare('UPDATE runners SET status = ?, updated_at = datetime(\'now\') WHERE id = ?');

export function shouldSkipGitHubExistenceCheck(status: string): boolean {
return status === 'pending' || status === 'configuring';
}

/**
* Start the periodic reconciliation service
*/
Expand Down Expand Up @@ -157,6 +161,11 @@ async function reconcileRunnersInternal(): Promise<void> {
for (const runner of localRunners) {
stats.checked++;

// Newly provisioning runners are expected to not exist in GitHub yet.
if (shouldSkipGitHubExistenceCheck(runner.status)) {
continue;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stuck provisioning runners are never cleaned up

Medium Severity

shouldSkipGitHubExistenceCheck unconditionally skips runners in pending/configuring status with no time-based bound. If provisioning hangs or the creating process crashes without transitioning the status to error, these runners remain in the database indefinitely — the reconciler never cleans them up, since neither the orphan check nor the stale-heartbeat query (which only matches online/busy) covers them.

Fix in Cursor Fix in Web


// Check if runner exists in GitHub (by ID or name)
const existsInGitHub =
(runner.github_runner_id && ghRunnerIds.has(runner.github_runner_id)) ||
Expand Down
7 changes: 6 additions & 1 deletion backend/src/services/runnerManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,12 @@ export async function downloadRunner(
}

// Cleanup archive
await fs.unlink(archivePath);
await fs.unlink(archivePath).catch((error: unknown) => {
const code = (error as NodeJS.ErrnoException).code;
if (code !== 'ENOENT') {
throw error;
}
});

// Update runner directory in database
updateRunnerDir.run(runnerDir, runnerId);
Expand Down
13 changes: 13 additions & 0 deletions backend/tests/reconciler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import fs from 'fs/promises';
import path from 'path';
import os from 'os';
import { db } from '../src/db/index.js';
import { shouldSkipGitHubExistenceCheck } from '../src/services/reconciler.js';

// Test directory for runner cleanup tests
const TEST_RUNNERS_DIR = path.join(os.tmpdir(), 'action-packer-test-runners');
Expand All @@ -16,6 +17,18 @@ let cleanupOrphanedDirectories: () => Promise<number>;
let withTimeout: <T>(promise: Promise<T>, ms: number, operation: string) => Promise<T>;

describe('Reconciler', () => {
describe('shouldSkipGitHubExistenceCheck', () => {
it('skips pending and configuring runners', () => {
expect(shouldSkipGitHubExistenceCheck('pending')).toBe(true);
expect(shouldSkipGitHubExistenceCheck('configuring')).toBe(true);
});

it('does not skip online/offline runners', () => {
expect(shouldSkipGitHubExistenceCheck('online')).toBe(false);
expect(shouldSkipGitHubExistenceCheck('offline')).toBe(false);
});
});

describe('withTimeout', () => {
beforeEach(async () => {
// Dynamically import to get fresh module
Expand Down