Skip to content
Open
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
35 changes: 35 additions & 0 deletions src/api/azure/services/azure-pipeline.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,41 @@ export class AzurePipelineService {
}
}

public async cancelBuild(buildId: number): Promise<void> {
try {
await retry(
async () => {
await this.client.patch(
`${this.project}/_apis/build/builds/${buildId}?${this.getApiVersionParam()}`,
{ status: 'cancelling' }
);
},
{
retries: 3,
minTimeout: 1000,
maxTimeout: 5000,
onRetry: (error: Error, attempt: number) => {
console.log(`[Azure] Retry ${attempt}/3 - Cancelling build ${buildId}: ${error.message}`);
},
}
);
Comment on lines +173 to +188
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Non-retryable errors should not trigger retries.

The retry mechanism will attempt 3 retries for all errors, including HTTP 400, 403, and 404 responses. These status codes indicate permanent failures (bad request, forbidden, not found) that won't succeed on retry, wasting time and resources.

Refactor to check the error status and bail early for non-retryable errors:

       await retry(
         async () => {
-          await this.client.patch(
+          try {
+            await this.client.patch(
               `${this.project}/_apis/build/builds/${buildId}?${this.getApiVersionParam()}`,
               { status: 'cancelling' }
             );
+          } catch (error: any) {
+            // Don't retry on permanent failures
+            if (error.response?.status && [400, 403, 404].includes(error.response.status)) {
+              throw new retry.AbortError(error);
+            }
+            throw error;
+          }
         },
         {
           retries: 3,

Committable suggestion skipped: line range outside the PR's diff.


console.log(`[Azure] Successfully cancelled build ${buildId}`);
} catch (error: any) {
// Handle specific error cases
if (error.response?.status === 404) {
throw new Error(`Build ${buildId} not found`);
}
if (error.response?.status === 403) {
throw new Error(`Insufficient permissions to cancel build ${buildId}`);
}
if (error.response?.status === 400) {
throw new Error(`Build ${buildId} cannot be cancelled (already completed or not cancellable)`);
}
throw new Error(`Failed to cancel build ${buildId}: ${error.message}`);
Comment on lines +191 to +202
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Use safer error type checking and avoid any.

The catch block uses error: any and accesses error.response?.status without verifying the error is an axios error. If the error comes from network failure, timeout, or retry exhaustion, error.response may be undefined, leading to incorrect error messages or undefined behavior.

Apply this diff to use proper type guards:

-    } catch (error: any) {
+    } catch (error: unknown) {
       // Handle specific error cases
-      if (error.response?.status === 404) {
+      if (axios.isAxiosError(error) && error.response?.status === 404) {
         throw new Error(`Build ${buildId} not found`);
       }
-      if (error.response?.status === 403) {
+      if (axios.isAxiosError(error) && error.response?.status === 403) {
         throw new Error(`Insufficient permissions to cancel build ${buildId}`);
       }
-      if (error.response?.status === 400) {
+      if (axios.isAxiosError(error) && error.response?.status === 400) {
         throw new Error(`Build ${buildId} cannot be cancelled (already completed or not cancellable)`);
       }
-      throw new Error(`Failed to cancel build ${buildId}: ${error.message}`);
+      const message = error instanceof Error ? error.message : String(error);
+      throw new Error(`Failed to cancel build ${buildId}: ${message}`);
     }
🤖 Prompt for AI Agents
In src/api/azure/services/azure-pipeline.service.ts around lines 191 to 202, the
catch block types the error as any and directly accesses error.response?.status;
change the catch parameter to error: unknown and use a proper type guard (e.g.,
axios.isAxiosError(error) or check (error as AxiosError)?.response) before
reading response.status, handle cases where response is undefined
(network/timeout) by throwing an appropriate message using error?.message or
JSON.stringify(error) as fallback, and avoid casting blindy to any — ensure
non-Axios errors fall through to a generic failure error with the original
message included.

}
}

public async getAllPipelines(): Promise<AzurePipelineDefinition[]> {
try {
const pipelines = await this.client.get<{ value: AzurePipelineDefinition[] }>(
Expand Down
70 changes: 70 additions & 0 deletions src/api/github/services/github-actions.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -590,4 +590,74 @@ export class GithubActionsService {
throw new GithubApiError(`Error finding workflow run for commit ${sha.substring(0, 7)}`, error.status, error);
}
}

/**
* Cancel a workflow run
*
* @param owner Repository owner
* @param repo Repository name
* @param runId Workflow run ID to cancel
* @returns Promise resolving when the workflow run is cancelled
* @throws GithubNotFoundError if the workflow run is not found
* @throws GithubApiError for other API errors
*/
async cancelWorkflowRun(
owner: string,
repo: string,
runId: number
): Promise<void> {
try {
await retry(
async () => {
await this.octokit.actions.cancelWorkflowRun({
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix the API path to include .rest.

All other methods in this service use this.octokit.rest.actions.* pattern (see lines 93, 100, 265, 321, 389, 517), but this line uses this.octokit.actions.* without the .rest segment. This inconsistency will likely cause a runtime error.

Apply this diff:

-          await this.octokit.actions.cancelWorkflowRun({
+          await this.octokit.rest.actions.cancelWorkflowRun({
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await this.octokit.actions.cancelWorkflowRun({
await this.octokit.rest.actions.cancelWorkflowRun({
🤖 Prompt for AI Agents
In src/api/github/services/github-actions.service.ts around line 612, the call
uses this.octokit.actions.cancelWorkflowRun which is inconsistent with the rest
of the file and will fail at runtime; change it to use
this.octokit.rest.actions.cancelWorkflowRun so it matches the other Octokit REST
calls (keep the same arguments and error handling).

owner,
repo,
run_id: runId,
});
},
{
retries: this.maxRetries,
minTimeout: this.minTimeout,
maxTimeout: this.maxTimeout,
factor: this.factor,
onRetry: (error: Error, attempt: number) => {
console.log(
`[GitHub Actions] Retry ${attempt}/${this.maxRetries} - Cancelling workflow run ${runId}: ${error.message}`
);
},
}
);

console.log(`[GitHub Actions] Successfully cancelled workflow run ${runId}`);
} catch (error: any) {
// Handle specific error cases
if (error.status === 404) {
throw new GithubNotFoundError(
'workflow run',
`${runId} in repository ${owner}/${repo}`
);
}

if (error.status === 403) {
throw new GithubApiError(
`Insufficient permissions to cancel workflow run ${runId}`,
error.status
);
}

if (error.status === 409) {
throw new GithubApiError(
`Workflow run ${runId} cannot be cancelled (already completed or not cancellable)`,
error.status
);
}

// Re-throw with more context
throw new GithubApiError(
`Failed to cancel workflow run ${runId}: ${error.message}`,
error.status || 500,
error
);
}
Comment on lines +610 to +661
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Use bail() to exit retry early for non-retryable errors.

The current implementation retries all errors, including 404 (not found), 403 (permissions), and 409 (already completed). These errors are non-retryable and should use the bail() function to exit early, as demonstrated in other methods (see lines 221, 280, 338). Retrying these errors wastes time and resources.

Apply this diff:

     try {
       await retry(
-        async () => {
+        async (bail) => {
           await this.octokit.rest.actions.cancelWorkflowRun({
             owner,
             repo,
             run_id: runId,
           });
+        },
+        {
+          retries: this.maxRetries,
+          minTimeout: this.minTimeout,
+          maxTimeout: this.maxTimeout,
+          factor: this.factor,
+          onRetry: (error: Error, attempt: number) => {
+            console.log(
+              `[GitHub Actions] Retry ${attempt}/${this.maxRetries} - Cancelling workflow run ${runId}: ${error.message}`
+            );
+          },
+        }
+      );
+
+      console.log(`[GitHub Actions] Successfully cancelled workflow run ${runId}`);
+    } catch (error: any) {
+      // Handle specific error cases - bail immediately for non-retryable errors
+      if (error.status === 404) {
+        throw new GithubNotFoundError(
+          'workflow run',
+          `${runId} in repository ${owner}/${repo}`
+        );
+      }
+
+      if (error.status === 403) {
+        throw new GithubApiError(
+          `Insufficient permissions to cancel workflow run ${runId}`,
+          error.status
+        );
+      }
+
+      if (error.status === 409) {
+        throw new GithubApiError(
+          `Workflow run ${runId} cannot be cancelled (already completed or not cancellable)`,
+          error.status
+        );
+      }
+
+      // Re-throw with more context
+      throw new GithubApiError(
+        `Failed to cancel workflow run ${runId}: ${error.message}`,
+        error.status || 500,
+        error
+      );
+    }

should be:

     try {
       await retry(
         async (bail) => {
-          await this.octokit.rest.actions.cancelWorkflowRun({
-            owner,
-            repo,
-            run_id: runId,
-          });
+          try {
+            await this.octokit.rest.actions.cancelWorkflowRun({
+              owner,
+              repo,
+              run_id: runId,
+            });
+          } catch (error: any) {
+            // Bail immediately for non-retryable errors
+            if (error.status === 404) {
+              bail(new GithubNotFoundError(
+                'workflow run',
+                `${runId} in repository ${owner}/${repo}`,
+                error.status
+              ));
+              return;
+            }
+            
+            if (error.status === 403) {
+              bail(new GithubApiError(
+                `Insufficient permissions to cancel workflow run ${runId}`,
+                error.status
+              ));
+              return;
+            }
+            
+            if (error.status === 409) {
+              bail(new GithubApiError(
+                `Workflow run ${runId} cannot be cancelled (already completed or not cancellable)`,
+                error.status
+              ));
+              return;
+            }
+            
+            // Throw for retryable errors
+            throw error;
+          }
         },
         {
           retries: this.maxRetries,
           minTimeout: this.minTimeout,
           maxTimeout: this.maxTimeout,
           factor: this.factor,
           onRetry: (error: Error, attempt: number) => {
             console.log(
               `[GitHub Actions] Retry ${attempt}/${this.maxRetries} - Cancelling workflow run ${runId}: ${error.message}`
             );
           },
         }
       );
 
       console.log(`[GitHub Actions] Successfully cancelled workflow run ${runId}`);
     } catch (error: any) {
-      // Handle specific error cases
-      if (error.status === 404) {
-        throw new GithubNotFoundError(
-          'workflow run',
-          `${runId} in repository ${owner}/${repo}`
-        );
-      }
-
-      if (error.status === 403) {
-        throw new GithubApiError(
-          `Insufficient permissions to cancel workflow run ${runId}`,
-          error.status
-        );
-      }
-
-      if (error.status === 409) {
-        throw new GithubApiError(
-          `Workflow run ${runId} cannot be cancelled (already completed or not cancellable)`,
-          error.status
-        );
-      }
-
-      // Re-throw with more context
+      // Add context to unknown errors
       throw new GithubApiError(
         `Failed to cancel workflow run ${runId}: ${error.message}`,
         error.status || 500,
         error
       );
     }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/api/github/services/github-actions.service.ts around lines 610-661, the
retry wrapper currently retries all errors including non-retryable 404/403/409;
modify the async function passed to retry to detect these statuses and call
bail(...) with the appropriate GithubNotFoundError or GithubApiError (matching
the existing error messages) so retry aborts immediately for those cases, and
only let other errors throw to trigger retry; keep the existing outer catch as a
fallback for unexpected errors.

}
}
19 changes: 19 additions & 0 deletions src/api/jenkins/services/jenkins-build.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,25 @@ export class JenkinsBuildService {
}
}

/**
* Stop/abort a running build
*/
async stopBuild(jobName: string, buildNumber: number, folderName?: string): Promise<void> {
try {
const path = JenkinsPathBuilder.buildBuildPath(
jobName,
buildNumber,
folderName,
'stop'
);

await this.httpClient.post(path, null);
} catch (error) {
// If build is not found or already stopped, throw specific error
throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName);
}
Comment on lines +181 to +184
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Preserve non-404 failures from stopBuild

Catching every failure and rethrowing JenkinsBuildNotFoundError makes a 403/500 look like a simple “build not found,” which is painful for diagnosing flaky cancellations in our E2E runs. Please only map actual 404s to JenkinsBuildNotFoundError and let other errors bubble so the caller can see the real cause.

-    } catch (error) {
-      // If build is not found or already stopped, throw specific error
-      throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName);
-    }
+    } catch (error) {
+      const status =
+        (error as { statusCode?: number })?.statusCode ??
+        (error as { response?: { status?: number } })?.response?.status;
+
+      if (status === 404) {
+        throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName);
+      }
+
+      throw error;
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
} catch (error) {
// If build is not found or already stopped, throw specific error
throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName);
}
} catch (error) {
const status =
(error as { statusCode?: number })?.statusCode ??
(error as { response?: { status?: number } })?.response?.status;
if (status === 404) {
throw new JenkinsBuildNotFoundError(jobName, buildNumber, folderName);
}
throw error;
}
🤖 Prompt for AI Agents
In src/api/jenkins/services/jenkins-build.service.ts around lines 181 to 184,
the catch block unconditionally converts every error from stopBuild into
JenkinsBuildNotFoundError; change it to detect a real 404 and only throw
JenkinsBuildNotFoundError for that case (e.g. check error.response?.status ===
404 or error.status === 404 depending on the HTTP client) and for any other
status or unexpected error rethrow the original error so the caller can observe
the true failure and stack.

}

/**
* Wait for a build to complete with timeout
*/
Expand Down
31 changes: 31 additions & 0 deletions src/api/ocp/kubeClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,37 @@ export class KubeClient {
}
}

/**
* Generic method to patch a resource with proper error handling
*
* @template T The resource type to return
* @param {K8sApiOptions} options - API options for the request (must include name)
* @param {any} patchData - The patch data to apply
* @returns {Promise<T>} The patched resource of type T
*/
public async patchResource<T>(options: K8sApiOptions, patchData: any): Promise<T> {
try {
if (!options.name) {
throw new Error('Resource name is required for patchResource');
}

const response = await this.customApi.patchNamespacedCustomObject({
group: options.group,
version: options.version,
namespace: options.namespace,
plural: options.plural,
name: options.name,
body: patchData,
});
return response as T;
} catch (error) {
console.error(
`Error patching resource '${options.name}' in namespace '${options.namespace}': ${error}`
);
throw new Error(`Failed to patch resource '${options.name}': ${error}`);
}
Comment on lines +276 to +296
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Set merge-patch Content-Type when patching

patchNamespacedCustomObject requires the merge-patch content type; without it the API answers 415 and Tekton cancellations never land, undermining the new cancelAllPipelines flow. Please set the header so the patch is accepted consistently.

       const response = await this.customApi.patchNamespacedCustomObject({
         group: options.group,
         version: options.version,
         namespace: options.namespace,
         plural: options.plural,
         name: options.name,
         body: patchData,
+        headers: {
+          'Content-Type': 'application/merge-patch+json',
+        },
       });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public async patchResource<T>(options: K8sApiOptions, patchData: any): Promise<T> {
try {
if (!options.name) {
throw new Error('Resource name is required for patchResource');
}
const response = await this.customApi.patchNamespacedCustomObject({
group: options.group,
version: options.version,
namespace: options.namespace,
plural: options.plural,
name: options.name,
body: patchData,
});
return response as T;
} catch (error) {
console.error(
`Error patching resource '${options.name}' in namespace '${options.namespace}': ${error}`
);
throw new Error(`Failed to patch resource '${options.name}': ${error}`);
}
public async patchResource<T>(options: K8sApiOptions, patchData: any): Promise<T> {
try {
if (!options.name) {
throw new Error('Resource name is required for patchResource');
}
const response = await this.customApi.patchNamespacedCustomObject({
group: options.group,
version: options.version,
namespace: options.namespace,
plural: options.plural,
name: options.name,
body: patchData,
headers: {
'Content-Type': 'application/merge-patch+json',
},
});
return response as T;
} catch (error) {
console.error(
`Error patching resource '${options.name}' in namespace '${options.namespace}': ${error}`
);
throw new Error(`Failed to patch resource '${options.name}': ${error}`);
}
}
🤖 Prompt for AI Agents
In src/api/ocp/kubeClient.ts around lines 276 to 296, the
patchNamespacedCustomObject call must include the merge-patch content type to
avoid 415 errors; update the API call to supply request headers with
"Content-Type": "application/merge-patch+json" (e.g., add a headers field to the
call parameters) so the patch is accepted consistently, preserve the existing
name/namespace/group/version/plural/body fields and keep the existing error
handling.

}

/**
* Retrieves logs from a pod or specific containers within a pod
* @param podName The name of the pod
Expand Down
30 changes: 30 additions & 0 deletions src/api/tekton/services/tekton-pipelinerun.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,36 @@ export class TektonPipelineRunService {
}
}

/**
* Cancel a running PipelineRun by patching its spec
*/
public async cancelPipelineRun(namespace: string, name: string): Promise<void> {
try {
const options = this.kubeClient.createApiOptions(
this.API_GROUP,
this.API_VERSION,
this.PIPELINE_RUNS_PLURAL,
namespace,
{ name }
);

// Patch the PipelineRun to set status to cancelled
const patchData = {
spec: {
status: 'PipelineRunCancelled'
}
};

await this.kubeClient.patchResource(options, patchData);

console.log(`Successfully cancelled PipelineRun: ${name} in namespace: ${namespace}`);
} catch (error: unknown) {
const errorMessage = (error as Error).message;
console.error(`Failed to cancel PipelineRun ${name}: ${errorMessage}`);
throw new Error(`Failed to cancel PipelineRun ${name}: ${errorMessage}`);
}
}

private findPipelineRunByEventType(
pipelineRuns: PipelineRunKind[],
eventType: string,
Expand Down
4 changes: 4 additions & 0 deletions src/api/tekton/tekton.client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ export class TektonClient {
return this.pipelineRunService.getPipelineRunLogs(namespace, pipelineRunName);
}

public async cancelPipelineRun(namespace: string, name: string): Promise<void> {
return this.pipelineRunService.cancelPipelineRun(namespace, name);
}

public get pipelineRuns(): TektonPipelineRunService {
return this.pipelineRunService;
}
Expand Down
21 changes: 19 additions & 2 deletions src/rhtap/core/integration/ci/baseCI.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
import { PullRequest } from '../git/models';
import { KubeClient } from './../../../../../src/api/ocp/kubeClient';
import { CI, CIType, EventType, Pipeline, PipelineStatus } from './ciInterface';
import {
CI,
CIType,
EventType,
Pipeline,
PipelineStatus,
CancelPipelineOptions,
CancelResult,
} from './ciInterface';
import retry from 'async-retry';

/**
Expand Down Expand Up @@ -118,7 +126,16 @@ export abstract class BaseCI implements CI {

public abstract waitForAllPipelineRunsToFinish(): Promise<void>;

public abstract cancelAllInitialPipelines(): Promise<void>;
/**
* Abstract method for cancelling all pipelines
* Must be implemented by each provider
*
* @param options Optional configuration for filtering and behavior
* @returns Promise resolving to detailed cancellation results
*/
public abstract cancelAllPipelines(
options?: CancelPipelineOptions
): Promise<CancelResult>;

public abstract getWebhookUrl(): Promise<string>;

Expand Down
Loading