[fix] handle label creation race condition and log errors to Sentry instead of throwing (#2265)
## Summary
- Fix race condition in `ensureLabelExists` where concurrent requests
both get a 404, then both try to create the label, causing an
"already_exists" validation error
- Add `isLabelAlreadyExistsError` helper function to properly detect
GitHub's 422 validation error for duplicate labels
- Change error handling from throwing to logging to Sentry, preventing
the entire PR creation flow from failing due to label issues
- Update tests to verify new Sentry logging behavior and race condition
handling
## Problem
When multiple concurrent requests call `ensureLabelExists`:
1. Both check if label exists → both get 404
2. Both try to create the label
3. First succeeds, second fails with
`{"resource":"Label","code":"already_exists","field":"name"}`
4. This error was thrown and caused the entire suggest-pr-changes flow
to fail
## Solution
- Catch the `already_exists` error during label creation and silently
ignore it (label exists, which is the desired state)
- Log other errors to Sentry instead of throwing, so label issues don't
block PR creation
FIxes CF-1017
This commit is contained in:
parent
22675393a2
commit
6b12255143
2 changed files with 106 additions and 48 deletions
|
|
@ -84,6 +84,16 @@ export async function getInstallationOctokitByOwner(
|
|||
}
|
||||
}
|
||||
|
||||
function isLabelAlreadyExistsError(error: any): boolean {
|
||||
return (
|
||||
error?.status === 422 &&
|
||||
Array.isArray(error?.response?.data?.errors) &&
|
||||
error.response.data.errors.some(
|
||||
(e: any) => e.resource === "Label" && e.code === "already_exists" && e.field === "name",
|
||||
)
|
||||
)
|
||||
}
|
||||
|
||||
// Ensures that a label exists on a repository, creating it if it doesn't
|
||||
export async function ensureLabelExists(
|
||||
installationOctokit: Octokit,
|
||||
|
|
@ -102,16 +112,27 @@ export async function ensureLabelExists(
|
|||
} catch (error: any) {
|
||||
if (error.status === 404) {
|
||||
// Label does not exist, create it
|
||||
await installationOctokit.rest.issues.createLabel({
|
||||
owner,
|
||||
repo,
|
||||
name: labelName,
|
||||
color,
|
||||
description,
|
||||
})
|
||||
try {
|
||||
await installationOctokit.rest.issues.createLabel({
|
||||
owner,
|
||||
repo,
|
||||
name: labelName,
|
||||
color,
|
||||
description,
|
||||
})
|
||||
} catch (createError: any) {
|
||||
// Ignore "already_exists" error - another concurrent request may have created it
|
||||
if (!isLabelAlreadyExistsError(createError)) {
|
||||
Sentry.captureException(createError, {
|
||||
extra: { owner, repo, labelName, context: "ensureLabelExists - createLabel failed" },
|
||||
})
|
||||
}
|
||||
}
|
||||
} else {
|
||||
// An error occurred that wasn't a 404, rethrow it
|
||||
throw error
|
||||
// Log to Sentry and prevent throwing the error
|
||||
Sentry.captureException(error, {
|
||||
extra: { owner, repo, labelName, context: "ensureLabelExists - getLabel failed" },
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,12 +1,22 @@
|
|||
import { jest, describe, it, expect, beforeEach, afterEach } from "@jest/globals"
|
||||
import {
|
||||
isUserCollaborator,
|
||||
getInstallationOctokitByOwner,
|
||||
|
||||
// Mock Sentry before importing modules that use it
|
||||
const mockCaptureException = jest.fn()
|
||||
jest.unstable_mockModule("@sentry/node", () => ({
|
||||
captureException: mockCaptureException,
|
||||
captureMessage: jest.fn(),
|
||||
addBreadcrumb: jest.fn(),
|
||||
}))
|
||||
|
||||
// Dynamic import after mock setup
|
||||
const {
|
||||
ensureLabelExists,
|
||||
addLabelToPullRequest,
|
||||
isUserCollaborator,
|
||||
getInstallationOctokitByOwner,
|
||||
setGithubUtilsDependencies,
|
||||
resetGithubUtilsDependencies,
|
||||
} from "../github-utils"
|
||||
} = await import("../github-utils.js")
|
||||
|
||||
describe("GitHub Utils", () => {
|
||||
let mockOctokit: any
|
||||
|
|
@ -314,26 +324,7 @@ describe("GitHub Utils", () => {
|
|||
})
|
||||
})
|
||||
|
||||
it("should throw error for non-404 API errors", async () => {
|
||||
const error = new Error("Internal Server Error")
|
||||
;(error as any).status = 500
|
||||
;(mockOctokit.rest.issues.getLabel as jest.MockedFunction<any>).mockRejectedValue(error)
|
||||
|
||||
await expect(
|
||||
ensureLabelExists(
|
||||
mockOctokit,
|
||||
"test-owner",
|
||||
"test-repo",
|
||||
"test-label",
|
||||
"FF0000",
|
||||
"Test label description",
|
||||
),
|
||||
).rejects.toThrow("Internal Server Error")
|
||||
|
||||
expect(mockOctokit.rest.issues.createLabel).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should handle label creation failure", async () => {
|
||||
it("should log to Sentry on label creation failure instead of throwing", async () => {
|
||||
const getError = new Error("Not Found")
|
||||
;(getError as any).status = 404
|
||||
;(mockOctokit.rest.issues.getLabel as jest.MockedFunction<any>).mockRejectedValue(getError)
|
||||
|
|
@ -343,16 +334,51 @@ describe("GitHub Utils", () => {
|
|||
createError,
|
||||
)
|
||||
|
||||
await expect(
|
||||
ensureLabelExists(
|
||||
mockOctokit,
|
||||
"test-owner",
|
||||
"test-repo",
|
||||
"test-label",
|
||||
"FF0000",
|
||||
"Test label description",
|
||||
),
|
||||
).rejects.toThrow("Label creation failed")
|
||||
await ensureLabelExists(
|
||||
mockOctokit,
|
||||
"test-owner",
|
||||
"test-repo",
|
||||
"test-label",
|
||||
"FF0000",
|
||||
"Test label description",
|
||||
)
|
||||
|
||||
expect(mockCaptureException).toHaveBeenCalledWith(createError, {
|
||||
extra: {
|
||||
owner: "test-owner",
|
||||
repo: "test-repo",
|
||||
labelName: "test-label",
|
||||
context: "ensureLabelExists - createLabel failed",
|
||||
},
|
||||
})
|
||||
})
|
||||
|
||||
it("should not log to Sentry when label already exists (race condition)", async () => {
|
||||
const getError = new Error("Not Found")
|
||||
;(getError as any).status = 404
|
||||
;(mockOctokit.rest.issues.getLabel as jest.MockedFunction<any>).mockRejectedValue(getError)
|
||||
|
||||
const alreadyExistsError = new Error("Validation Failed")
|
||||
;(alreadyExistsError as any).status = 422
|
||||
;(alreadyExistsError as any).response = {
|
||||
data: {
|
||||
errors: [{ resource: "Label", code: "already_exists", field: "name" }],
|
||||
},
|
||||
}
|
||||
;(mockOctokit.rest.issues.createLabel as jest.MockedFunction<any>).mockRejectedValue(
|
||||
alreadyExistsError,
|
||||
)
|
||||
|
||||
await ensureLabelExists(
|
||||
mockOctokit,
|
||||
"test-owner",
|
||||
"test-repo",
|
||||
"test-label",
|
||||
"FF0000",
|
||||
"Test label description",
|
||||
)
|
||||
|
||||
expect(mockCaptureException).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should handle special characters in label name", async () => {
|
||||
|
|
@ -486,16 +512,27 @@ describe("GitHub Utils", () => {
|
|||
).rejects.toThrow("Failed to add labels")
|
||||
})
|
||||
|
||||
it("should handle ensureLabelExists failure", async () => {
|
||||
it("should continue to add labels even when ensureLabelExists logs error to Sentry", async () => {
|
||||
const error = new Error("Label operation failed")
|
||||
;(error as any).status = 500
|
||||
;(mockOctokit.rest.issues.getLabel as jest.MockedFunction<any>).mockRejectedValue(error)
|
||||
|
||||
await expect(
|
||||
addLabelToPullRequest(mockOctokit, "test-owner", "test-repo", 123),
|
||||
).rejects.toThrow("Label operation failed")
|
||||
await addLabelToPullRequest(mockOctokit, "test-owner", "test-repo", 123)
|
||||
|
||||
expect(mockOctokit.rest.issues.addLabels).not.toHaveBeenCalled()
|
||||
expect(mockCaptureException).toHaveBeenCalledWith(error, {
|
||||
extra: {
|
||||
owner: "test-owner",
|
||||
repo: "test-repo",
|
||||
labelName: "⚡️ codeflash",
|
||||
context: "ensureLabelExists - getLabel failed",
|
||||
},
|
||||
})
|
||||
expect(mockOctokit.rest.issues.addLabels).toHaveBeenCalledWith({
|
||||
owner: "test-owner",
|
||||
repo: "test-repo",
|
||||
issue_number: 123,
|
||||
labels: ["⚡️ codeflash"],
|
||||
})
|
||||
})
|
||||
|
||||
it("should handle zero pull request number", async () => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue