Skip to content

Commit b4a4e7c

Browse files
authored
Add retry with jitter to create_issue safe-output handler (#26056)
1 parent 265e150 commit b4a4e7c

File tree

5 files changed

+167
-19
lines changed

5 files changed

+167
-19
lines changed

actions/setup/js/create_issue.cjs

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
4343
const { removeDuplicateTitleFromDescription } = require("./remove_duplicate_title.cjs");
4444
const { getErrorMessage } = require("./error_helpers.cjs");
4545
const { ERR_VALIDATION } = require("./error_codes.cjs");
46+
const { withRetry } = require("./error_recovery.cjs");
4647
const { renderTemplateFromFile } = require("./messages_core.cjs");
4748
const { createExpirationLine, addExpirationToFooter } = require("./ephemerals.cjs");
4849
const { MAX_SUB_ISSUES, getSubIssueCount } = require("./sub_issue_helpers.cjs");
@@ -167,13 +168,18 @@ async function findOrCreateParentIssue({ githubClient, groupId, owner, repo, tit
167168
core.info(`Creating new parent issue for group: ${groupId}`);
168169
try {
169170
const template = createParentIssueTemplate(groupId, titlePrefix, workflowName, workflowSourceURL, expiresHours);
170-
const { data: parentIssue } = await githubClient.rest.issues.create({
171-
owner,
172-
repo,
173-
title: template.title,
174-
body: template.body,
175-
labels: labels,
176-
});
171+
const { data: parentIssue } = await withRetry(
172+
() =>
173+
githubClient.rest.issues.create({
174+
owner,
175+
repo,
176+
title: template.title,
177+
body: template.body,
178+
labels: labels,
179+
}),
180+
{ initialDelayMs: 15000, maxDelayMs: 45000, jitterMs: 10000 },
181+
`create_parent_issue for group ${groupId}`
182+
);
177183

178184
core.info(`Created new parent issue #${parentIssue.number}: ${parentIssue.html_url}`);
179185
return parentIssue.number;
@@ -588,14 +594,19 @@ async function main(config = {}) {
588594
}
589595

590596
try {
591-
const { data: issue } = await githubClient.rest.issues.create({
592-
owner: repoParts.owner,
593-
repo: repoParts.repo,
594-
title,
595-
body,
596-
labels,
597-
assignees,
598-
});
597+
const { data: issue } = await withRetry(
598+
() =>
599+
githubClient.rest.issues.create({
600+
owner: repoParts.owner,
601+
repo: repoParts.repo,
602+
title,
603+
body,
604+
labels,
605+
assignees,
606+
}),
607+
{ initialDelayMs: 15000, maxDelayMs: 45000, jitterMs: 10000 },
608+
`create_issue in ${qualifiedItemRepo}`
609+
);
599610

600611
core.info(`Created issue ${qualifiedItemRepo}#${issue.number}: ${issue.html_url}`);
601612
createdIssues.push({ ...issue, _repo: qualifiedItemRepo });

actions/setup/js/create_issue.test.cjs

Lines changed: 92 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -452,7 +452,7 @@ describe("create_issue", () => {
452452
const result = await handler({ title: "Test" });
453453

454454
expect(result.success).toBe(false);
455-
expect(result.error).toBe("API Error");
455+
expect(result.error).toContain("API Error");
456456
});
457457
});
458458

@@ -667,4 +667,95 @@ describe("create_issue", () => {
667667
expect(mockGithub.rest.issues.create).not.toHaveBeenCalled();
668668
});
669669
});
670+
671+
describe("retry on rate limit errors", () => {
672+
beforeEach(() => {
673+
vi.useFakeTimers();
674+
});
675+
676+
afterEach(() => {
677+
vi.useRealTimers();
678+
});
679+
680+
it("should retry issue creation on transient rate limit error and succeed", async () => {
681+
mockGithub.rest.issues.create = vi
682+
.fn()
683+
.mockRejectedValueOnce(new Error("Secondary rate limit hit"))
684+
.mockResolvedValue({
685+
data: {
686+
number: 456,
687+
html_url: "https://github.com/owner/repo/issues/456",
688+
title: "Retried Issue",
689+
},
690+
});
691+
692+
const handler = await main({});
693+
const resultPromise = handler({
694+
title: "Retried Issue",
695+
body: "Test body",
696+
});
697+
698+
await vi.runAllTimersAsync();
699+
const result = await resultPromise;
700+
701+
expect(result.success).toBe(true);
702+
expect(result.number).toBe(456);
703+
expect(mockGithub.rest.issues.create).toHaveBeenCalledTimes(2);
704+
});
705+
706+
it("should fail after exhausting retries on persistent rate limit error", async () => {
707+
mockGithub.rest.issues.create = vi.fn().mockRejectedValue(new Error("Secondary rate limit hit"));
708+
709+
const handler = await main({});
710+
const resultPromise = handler({
711+
title: "Failing Issue",
712+
body: "Test body",
713+
});
714+
715+
await vi.runAllTimersAsync();
716+
const result = await resultPromise;
717+
718+
expect(result.success).toBe(false);
719+
expect(result.error).toBeDefined();
720+
// 1 initial + 3 retries = 4 calls
721+
expect(mockGithub.rest.issues.create).toHaveBeenCalledTimes(4);
722+
});
723+
724+
it("should have retry delays that never exceed maxDelayMs + jitterMs", async () => {
725+
const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout");
726+
727+
mockGithub.rest.issues.create = vi
728+
.fn()
729+
.mockRejectedValueOnce(new Error("Secondary rate limit hit"))
730+
.mockRejectedValueOnce(new Error("Secondary rate limit hit"))
731+
.mockResolvedValue({
732+
data: {
733+
number: 789,
734+
html_url: "https://github.com/owner/repo/issues/789",
735+
title: "Bounded Delay Issue",
736+
},
737+
});
738+
739+
const handler = await main({});
740+
const resultPromise = handler({
741+
title: "Bounded Delay Issue",
742+
body: "Test body",
743+
});
744+
745+
await vi.runAllTimersAsync();
746+
await resultPromise;
747+
748+
// create_issue uses { initialDelayMs: 15000, maxDelayMs: 45000, jitterMs: 10000 }
749+
// Maximum possible delay per retry = maxDelayMs + jitterMs = 55000ms
750+
const maxBound = 55000;
751+
// Filter out short setTimeout calls (e.g. from test infrastructure) to isolate retry delays
752+
const sleepDelays = setTimeoutSpy.mock.calls.filter(([, ms]) => ms > 1000).map(([, ms]) => ms);
753+
754+
for (const delay of sleepDelays) {
755+
expect(delay).toBeLessThanOrEqual(maxBound);
756+
}
757+
758+
setTimeoutSpy.mockRestore();
759+
});
760+
});
670761
});

actions/setup/js/error_recovery.cjs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ const { ERR_API } = require("./error_codes.cjs");
1616
* @property {number} initialDelayMs - Initial delay in milliseconds (default: 1000)
1717
* @property {number} maxDelayMs - Maximum delay in milliseconds (default: 10000)
1818
* @property {number} backoffMultiplier - Backoff multiplier for exponential backoff (default: 2)
19+
* @property {number} jitterMs - Maximum random jitter in milliseconds added to each retry delay (default: 100)
1920
* @property {(error: any) => boolean} shouldRetry - Function to determine if error is retryable
2021
*/
2122

@@ -28,6 +29,7 @@ const DEFAULT_RETRY_CONFIG = {
2829
initialDelayMs: 1000,
2930
maxDelayMs: 10000,
3031
backoffMultiplier: 2,
32+
jitterMs: 100,
3133
shouldRetry: isTransientError,
3234
};
3335

@@ -95,8 +97,10 @@ async function withRetry(operation, config = {}, operationName = "operation") {
9597
for (let attempt = 0; attempt <= fullConfig.maxRetries; attempt++) {
9698
try {
9799
if (attempt > 0) {
98-
core.info(`Retry attempt ${attempt}/${fullConfig.maxRetries} for ${operationName} after ${delay}ms delay`);
99-
await sleep(delay);
100+
const jitter = fullConfig.jitterMs > 0 ? Math.floor(Math.random() * fullConfig.jitterMs) : 0;
101+
const delayWithJitter = delay + jitter;
102+
core.info(`Retry attempt ${attempt}/${fullConfig.maxRetries} for ${operationName} after ${delayWithJitter}ms delay`);
103+
await sleep(delayWithJitter);
100104
}
101105

102106
const result = await operation();

actions/setup/js/error_recovery.test.cjs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ describe("error_recovery", () => {
110110
initialDelayMs: 100,
111111
backoffMultiplier: 2,
112112
maxDelayMs: 1000,
113+
jitterMs: 0,
113114
};
114115

115116
await withRetry(operation, config, "test-operation");
@@ -130,6 +131,7 @@ describe("error_recovery", () => {
130131
initialDelayMs: 1000,
131132
backoffMultiplier: 10,
132133
maxDelayMs: 2000, // Cap at 2000ms
134+
jitterMs: 0,
133135
};
134136

135137
await withRetry(operation, config, "test-operation");
@@ -147,6 +149,45 @@ describe("error_recovery", () => {
147149
expect(operation).toHaveBeenCalledTimes(1);
148150
expect(shouldRetry).toHaveBeenCalled();
149151
});
152+
153+
it("should add random jitter to delay when jitterMs is configured", async () => {
154+
const randomSpy = vi.spyOn(Math, "random").mockReturnValue(0.5);
155+
const operation = vi.fn().mockRejectedValueOnce(new Error("Network timeout")).mockResolvedValue("success");
156+
157+
const config = {
158+
maxRetries: 2,
159+
initialDelayMs: 100,
160+
backoffMultiplier: 2,
161+
maxDelayMs: 10000,
162+
jitterMs: 1000,
163+
};
164+
165+
await withRetry(operation, config, "test-operation");
166+
167+
// Base delay after first failure: 100 * 2 = 200ms
168+
// Jitter: Math.floor(0.5 * 1000) = 500ms
169+
// Total: 200 + 500 = 700ms
170+
expect(core.info).toHaveBeenCalledWith(expect.stringContaining("after 700ms delay"));
171+
172+
randomSpy.mockRestore();
173+
});
174+
175+
it("should not add jitter when jitterMs is 0", async () => {
176+
const operation = vi.fn().mockRejectedValueOnce(new Error("Network timeout")).mockResolvedValue("success");
177+
178+
const config = {
179+
maxRetries: 2,
180+
initialDelayMs: 100,
181+
backoffMultiplier: 2,
182+
maxDelayMs: 10000,
183+
jitterMs: 0,
184+
};
185+
186+
await withRetry(operation, config, "test-operation");
187+
188+
// Base delay after first failure: 100 * 2 = 200ms, no jitter
189+
expect(core.info).toHaveBeenCalledWith(expect.stringContaining("after 200ms delay"));
190+
});
150191
});
151192

152193
describe("enhanceError", () => {
@@ -288,6 +329,7 @@ describe("error_recovery", () => {
288329
expect(DEFAULT_RETRY_CONFIG.initialDelayMs).toBe(1000);
289330
expect(DEFAULT_RETRY_CONFIG.maxDelayMs).toBe(10000);
290331
expect(DEFAULT_RETRY_CONFIG.backoffMultiplier).toBe(2);
332+
expect(DEFAULT_RETRY_CONFIG.jitterMs).toBe(100);
291333
expect(DEFAULT_RETRY_CONFIG.shouldRetry).toBe(isTransientError);
292334
});
293335
});

cmd/gh-aw/argument_syntax_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func TestArgumentSyntaxConsistency(t *testing.T) {
6363
{
6464
name: "remove command has optional pattern",
6565
command: removeCmd,
66-
expectedUse: "remove [pattern]",
66+
expectedUse: "remove [filter]",
6767
argsValidator: "no validator (all optional)",
6868
shouldValidate: func(cmd *cobra.Command) error { return nil },
6969
},

0 commit comments

Comments
 (0)